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.

Assessment

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

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.

Description

inbox

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.

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

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.

More from Altinity/altinity-sql-browser

All issues in Altinity/altinity-sql-browser

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.