Altinity / Altinity/altinity-sql-browser
anchoredPopover.close() can clobber a newer popover sharing the same dom refKey
Nobody has claimed this yet.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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