Altinity / Altinity/altinity-sql-browser

anchoredPopover.close() can clobber a newer popover sharing the same dom refKey

Open
#603 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

inbox
Dominant language
TypeScript
Stars
8
Forks
2
Avg merge
1h 34m
Merged PRs (30d)
6

Description

anchoredPopover's close() (now in src/ui/popover.ts, createAnchoredPopovers) removes whatever node currently occupies app.dom[refKey] without verifying that node is the one it opened:

close() {
  if (getRef(refKey)) { ... /* removes it unconditionally */ }
}

Repro: open popover A on refKey, open popover B on the same refKey (A's slot is now B), then invoke A's retained close() handle — B gets removed/clobbered even though A never touched it, and the keyboard owner it releases may no longer belong to A either (keyboard release itself is idempotent via the released flag in acquireKeyboardOwner; the DOM removal is not).

This predates the #588 phase-4 composition-root refactor (the code moved verbatim from src/ui/app.ts, byte-identical) — surfaced during that refactor's readiness review, not introduced by it. Deliberately not fixed there, per this repo's "surface out-of-scope findings, don't bury them" discipline: fixing it would have been a behavior change inside a PR whose scope and review budget were both set for a pure structural extraction.

A regression test pinning the current (buggy) behavior lives in tests/unit/popover.test.ts (search for "I-21" / "stale-clobber").

Suggested fix direction: give each open() call a token/generation and have close() no-op if the ref slot's current occupant token doesn't match the token the closer was handed.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/ui/popover.ts at createAnchoredPopovers and compare the close path with the I-21/stale-clobber regression in tests/unit/popover.test.ts. Run that test first, then update the behavior so a retained close handle cannot remove a newer popover occupying the same refKey. Done means the newer popover remains present while the original can still close safely.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.