fix: cancel running query reliably and preserve selection when running while busy

[emrberk]

Problems

• Cancel didn't cancel. With all SQL selected (e.g. to share the console), clicking the red Cancel button on a running query opened the "Run all queries" modal instead of cancelling, because Run and Cancel shared a handler that branched on the selection.
• Selection dropped. Triggering a multi-statement selection run while a query was already running silently ran all queries instead of the selected ones.
• New-tab button clickable while disabled. During a script run the "+" tab button looked greyed out but was still clickable (CSS-only guard, plus the library re-enabling pointer-events).
• Dead pending-action code and a ref-sync race in the run-confirmation flow.

Fixes

• Cancel now always cancels while a query is running, regardless of selection.
• The selection is carried into the pending run; the confirmation dialog reflects it ("Run selected queries").
• New-tab creation is gated by logic (button + Alt+T) and CSS.
• Removed the unreachable pending-query branch; the confirmation ref is now set synchronously.

Tests

Added e2e coverage for the cancel-while-all-selected flow (incl. run-while-busy -> modal -> confirm -> old query cancelled, selected queries run one by one) and the new-tab guard during a script run.

🤖 Generated with Claude Code

[emrberk]

Issues

No critical, production-blocking issues found. Every draft finding from the parallel review was traced to source and dismissed (see False-positives). The three fixes in this PR are correct and verified:

Fix	Verified behavior
Cancel always cancels while running	ButtonBar/index.tsx:204-214 early-returns toggleRunning() when running !== NONE. That dispatch sets running → NONE, and the effect at Monaco/index.tsx:1784-1790 aborts the in-flight request via quest.abort(). Reachable for QUERY/EXPLAIN/REFRESH states — all correctly cancel now instead of branching to the run-all modal.
Selection preserved when running while busy	executePendingScriptRun (Monaco/index.tsx:598-608) clears queriesToRunRef.current only when pending.runAll === true; the selection path leaves it intact, so the confirmed run executes the selected queries, not all.
New-tab blocked during script run	Four-layer guard, all verified: handleNewTab early-return (tabs.tsx:431-433), the Alt+T addBuffer command guard (index.tsx:937-940), the targeted SCSS pointer-events: none (_react-chrome-tabs.scss), and the Root wrapper style.

Non-blocking observation (not critical, not a must-fix): the two new e2e tests lean on fixed cy.wait(150) and a cy.clickRunScript() that doesn't wait for the request to start before asserting the disabled state. Cypress .should() retries paper over most of it, but if you touch these later, gate on the button text changing to "Run 3 selected queries" instead of a hard wait. Does not block production.

---

False-positives

Category	Finding	Why it's a false positive
State management & async	executePendingScriptRun returns early without clearing pendingScriptRunRef when editorRef.current is null → stale pending leaks into a later run	editorRef.current is set once on mount (index.tsx:895) and only nulled in the unmount cleanup (index.tsx:2247). There is no null→non-null transition mid-life, so no later call can consume a leaked ref. The guard pattern is also identical to the pre-existing executePendingAction — not introduced here.
State management & async	"Run selected while busy" could accidentally run ALL queries (data loss)	queriesToRunRef is cleared only on the runAll branch; selection path preserves it. Traced confirm → executePendingScriptRun → handleRunScript. Correct.
React correctness	isPendingSelectionRun reads a ref in render → could show stale title/count	Every writer of pendingScriptRunRef is immediately paired with setScriptConfirmation(), which sets state and forces a re-render. Ref is always current when the dialog paints.
React correctness	Removing the scriptConfirmationOpenRef-sync effect could let the ref diverge from state	All four writers of scriptConfirmationOpen now route through setScriptConfirmation() (index.tsx:1503-1506), which sets ref + state atomically. No divergence path exists.
Styling / state	tabsDisabled could get stuck true (tabs + editor permanently locked)	The script path always reaches setTabsDisabled(false) (index.tsx:1732); the share-link path resets in both .then and .catch. runIndividualQuery swallows errors, so the loop can't throw out. No new stuck-state risk.
Component usage	data-hook rename (editor-tabs → conditional editor-tabs-disabled) breaks selectors	Grep of src/ and e2e/ shows getEditorTabs keys off .chrome-tab, not the data-hook; the only editor-tabs references are the new tests, which expect both states correctly.
Performance / race	tabsDisabledRef lags tabsDisabled by one render → Alt+T race	Window is a single render tick at script start (multi-second operation); CSS pointer-events:none is a second barrier. Not realistically reachable.
UX	Hiding "Stop after failure" checkbox for selection runs is a regression	handleRunScript's break condition requires runningAllQueries (index.tsx:1633-1638), which is false for selections — stopAfterFailureRef is already ignored for selection runs, so hiding the checkbox is consistent, not a regression.
Dead-code removal	Removing validateQueryAtOffset / the pending-QUERY/EXPLAIN branch drops behavior	Git history confirms pendingActionRef was only ever assigned { type: SCRIPT }; the QUERY/EXPLAIN union branch and validateQueryAtOffset were unreachable. No remaining references in src/ or e2e/. Safe removal.

---

Summary

Verdict: Approve. No production blockers.

• Quality checks: typecheck, lint, build (24.6s), and unit tests all pass.
• The three claimed fixes are real and correctly implemented, and the refactor (removing dead pendingActionRef/validateQueryAtOffset, synchronous ref-setting) drops no live behavior — verified against git history.
• No regressions or risky tradeoffs. The only soft spot is test robustness (fixed waits), which is non-blocking.
• 9 draft findings verified, 9 dismissed as false positives (0 confirmed critical). The single candidate critical (stale pendingScriptRunRef on null editorRef) was disproven: editorRef only nulls at unmount, so the leak is unreachable, and the pattern predates this PR.

[bluestreak01]

Review: PR #567 (level 3 — full mission-critical pass)

Scope: ButtonBar cancel handler, Monaco script-run/confirmation refactor, new-tab guard during script runs, dead-code removal, 2 new e2e tests. Reviewed at HEAD 8749b07.

The three claimed fixes are real and correctly implemented — corroborated independently by the query-execution, async, cross-context, and fresh-context adversarial passes, plus an end-to-end trace of the cancellation path (cancelActive → client.abort(queryId), the abort effect, and the exactly-once executePendingScriptRun guard). No critical or production-blocking product bug found. The substantive findings are concentrated in the new e2e tests, which in their current form do not actually fail when the fixes regress.

Issues

#	Issue	Category	Severity	Location	Description	Suggested fix
1	Selection-preservation test can't fail	Test coverage	Moderate	in-diff (editor.spec.js:283-316)	The "run-selected-while-busy" test selects the entire buffer ({1,1}→{3,10} over select 1;/2;/3;), so queriesToRun = all 3. executePendingScriptRun differs only by clearing vs preserving queriesToRunRef; with a full-buffer selection both branches run the same 3 queries. The /3 successful/ assertion passes whether or not the fix works — the core fix has no failing-test protection.	Select a strict subset while busy (e.g. lines 1-2) and assert exactly 2 successful queries, distinguishing "run selected" (2) from regressed "run all" (3).
2	Fixed cy.wait(150) can no-op the regression	Test coverage	Moderate	in-diff (editor.spec.js:267,293)	Both scenarios rely on a 150ms sleep for the 50ms-debounced cursor handler to push the multi-selection into queriesToRun. If it hasn't propagated (slow CI), queriesToRun is still 1 and the regression path (needs >1) is never exercised — the test passes trivially.	Poll redux getQueriesToRun().length > 1 (or assert the "Run 3 selected queries" label) before triggering, instead of a magic sleep.
3	Instantaneous negative tab-count assertion	Test coverage	Moderate	in-diff (editor.spec.js:507)	After the forced "+" click, getEditorTabs().should("have.length", initialTabCount) is already true immediately. addBuffer adds a tab asynchronously (IndexedDB → useLiveQuery), so a regression that adds a tab ~tens of ms later passes on the first retry and is never caught.	Settle on the script's first /exec (or a deterministic wait) before asserting, and re-assert the count stays equal.
4	Keyboard guard paths untested	Test coverage	Moderate	in-diff (missing)	The two keyboard paths this PR actually guards are untested: Alt+T addBuffer (index.tsx:937, gated by the effect-lagged tabsDisabledRef) and the run-all-while-busy confirm path (pendingScriptRunRef={runAll:true} → clears queriesToRunRef). Only mouse force-click and run-selected-while-busy are covered.	Add: run a script then cy.realPress(["Alt","T"]) asserting tab count unchanged; and a run-all-while-busy case asserting the "Run all queries" dialog + all buffer queries execute.
5	Stale "will be aborted" warning	React/Async	Minor	in-diff (index.tsx:2343)	The warning box is gated on pendingScriptRunRef.current, not on "something is actually running". If the in-flight query completes on its own while the dialog is open, .finally skips clearing (gated by scriptConfirmationOpenRef), so the dialog keeps showing "Current query execution will be aborted." though nothing is running. Confirm still behaves correctly — cosmetic only.	Gate the warning on real running state (running !== NONE || questExecution.isAnyRunning()).
6	New-tab button: disabled state not conveyed to AT	A11y/UX	Minor	in-diff (tabs.tsx/chrome-tabs.ts:47)	During a script run the new-tab <button> stays in tab order with no disabled/aria-disabled; cues are pointer-events:none+opacity only. A keyboard/SR user can focus it, it announces as enabled, and Enter is silently swallowed. Functionally safe but inconsistent.	When tabsDisabled, set aria-disabled/disabled (+tabindex={-1}) and add aria-label="New tab" (the icon-only button has no accessible name).
7	tabsDisabledRef one-render lag	React	Minor	in-diff (index.tsx:1762-1764,937)	tabsDisabledRef is synced via effect, so between setTabsDisabled(true) (:1601) and the effect there is a sub-frame window where the Alt+T guard still reads false. Net improvement over pre-PR (no guard existed); not realistically hittable by a human within one frame.	Mirror the scriptConfirmationOpenRef pattern — set tabsDisabledRef.current synchronously next to each setTabsDisabled(...).
8	runAll field shadows runAll param	Structure/types	Minor	in-diff (index.tsx:1508,1534)	pendingScriptRunRef.current = { runAll: runsAllQueries } where runsAllQueries = Boolean(runAll) || !hasMultipleSelection — the stored runAll ≠ the runAll argument. Reusing the name for two different booleans is a readability trap.	Name the field runsAllQueries to match its meaning.

False-positives (verified and dismissed)

Candidate finding	Why it's a false positive
executePendingScriptRun leaks pendingScriptRunRef when editorRef.current is null	editorRef.current is nulled only in unmount cleanup (:2247); no null→non-null transition mid-life, so no later call consumes a leaked ref.
Removing the scriptConfirmationOpenRef sync effect lets the ref diverge	Only writers of scriptConfirmationOpen are useState + setScriptConfirmation (:1505), which writes ref+state atomically. Now strictly fresher — an improvement.
Stale/aborted query result overwrites the new run's grid	Aborted fetch → .catch → never setResult. A query resolving just as confirm fires settles in a microtask before executePendingScriptRun, and the script's terminal setResult runs later — the new run always wins.
Pending run dropped or run twice (.finally + script-stop + confirm)	executePendingScriptRun clears the ref before dispatching and has no await — idempotent; callers are also mutually exclusive by running state. Exactly-once holds.
ButtonBar "always cancel" branch conflicts with a rendered button state	Cancel-query button renders only for QUERY/EXPLAIN/REFRESH, run button only for running===NONE; running===SCRIPT uses a different handler. Branch matches render conditions in every state.
handleNewTab unmemoized → stale closure defeats the guard	The newTab listener effect deps [listeners.onNewTab]; handleNewTab is a fresh ref each render, so the listener re-binds and always captures live tabsDisabled. Leak-free (effect has cleanup).
New pointer-events:none rule doesn't win / doesn't block the click	Specificity (0,3,0) beats pointer-events:auto (0,2,0); no inline pointer-events on the wrapper. Real clicks blocked; force:true/keyboard caught by JS guards.
Removing validateQueryAtOffset/QUERY-EXPLAIN pending branch drops behavior	On the base, pendingActionRef was only ever {type:SCRIPT}/undefined (single-query-while-busy goes through requestExecution); validateQueryAtOffset has zero remaining refs; utils unit tests 34/34 pass.
handleNewTab rebinding causes a re-render storm	One removeEventListener+addEventListener on a single node per Tabs render; effects don't trigger renders. Negligible.

Summary

Verdict: Approve, with test improvements requested before merge. No production-blocking product defect.

• The three fixes are correct and verified end-to-end. The dead-code removal and the synchronous scriptConfirmationOpenRef are safe and a genuine improvement.
• No regressions. The fresh-context adversarial pass independently failed to construct any reachable bug.
• The real weakness is the PR's own test net (findings wip test for error range #1-add test for query run when cursor position is next to ending semicolon #4): the new e2e tests can stay green even if the fixes regress. Moderate, because these query-execution flows are e2e-only — weak tests are the only safety net.
• Quality gate: editor utils unit tests pass (34/34). yarn typecheck/lint/build fail only on a pre-existing, unrelated @questdb/sql-parser StoragePolicy mismatch in files this PR does not touch — an environment/dependency issue, not introduced here. No PR-changed file produces type or lint errors.
• Counts: 8 findings verified (4 Moderate test-quality, 4 Minor); 9 candidates dismissed as false positives. In-diff vs out-of-diff: 8 in-diff, 0 out-of-diff — the cross-context pass walked all 6 callsites and returned 6/6 SAFE (localized, self-contained refactor; no external consumers' contracts altered).

_{🤖 Generated via the review-pr skill (level 3: 13 review agents + per-finding verification).}

[bluestreak01]

Review: PR #567 (level 3 — full mission-critical pass)

Scope: ButtonBar cancel handler, Monaco run/confirmation refactor (pendingActionRef → pendingScriptRunRef, synchronous setScriptConfirmation), new-tab guard during script runs, dead-code removal (validateQueryAtOffset), and 5 new e2e tests + test infra (window.__store__, waitForSelectedQueries). Reviewed on HEAD f854b283 against main.

The three claimed product fixes are real and correctly implemented — corroborated by the React/async, cross-context, and fresh-context adversarial passes plus an end-to-end trace of the cancel path (toggleRunning() → running=NONE → abort effect → quest.abort) and the selection-preserving executePendingScriptRun (clears queriesToRunRef only for runAll). No critical or production-blocking product bug. The substantive findings are concentrated in the new tests — one of which asserts a notification the code tears down.

Issues

#	Issue	Category	Severity	Location	Description	Suggested fix
1	Run-all-while-busy test asserts a notification that gets wiped	Test review / Async	Moderate	in-diff (editor.spec.js:342; mechanism Monaco/index.tsx:1596, reducers.ts:101)	The new "run all queries while busy" test asserts "Cancelled by user" after confirm. Trace: confirm → toggleRunning(NONE) aborts the query → its .catch adds the cancel notice (keyed queryNotifications[bufferId][parentQueryKey]) → .finally → executePendingScriptRun (runAll:true clears queriesToRunRef) → running=SCRIPT → handleRunScript runs cleanupBufferNotifications(activeBufferId) which deletes that buffer's notifications, including the cancel notice, ~one frame after it appears. The assertion races the teardown. (Selection-while-busy sibling test is reliable — selection runs skip the cleanup.) Product behavior is unchanged from base; only the new assertion is wrong. Verified by code trace; e2e not executed here.	Don't assert the cancel notice for run-all (it's intentionally cleared); assert only the dialog + "3 successful queries". If cancel feedback is desired for run-all, re-add the notice after cleanup.
2	Run-all-while-busy test can't isolate the runAll logic	Test review	Moderate	in-diff (editor.spec.js:342)	Cursor stays on line 1, so queriesToRun.length===1 → runsAllQueries is true regardless of the runAll flag, and clear-on-runAll (1→0 vs leave-1) both give runningAllQueries=true. The risky path — a multi-statement selection + Ctrl/Cmd+Shift+Enter expecting all queries — is untested; a regression running only the selection wouldn't be caught.	Add a run-all-while-busy variant that first establishes a 2-query selection (waitForSelectedQueries(2)), then Ctrl+Shift+Enter, asserting 3 successful queries.
3	Disabled new-tab button not exposed to assistive tech	Accessibility & UX	Minor	in-diff (tabs.tsx, _react-chrome-tabs.scss:49; markup chrome-tabs.ts:47)	During a script run the "+" is disabled only via pointer-events:none + JS guards. It stays in tab order with no disabled/aria-disabled/tabindex=-1; a keyboard/SR user can focus it, it announces as enabled, and Enter is silently swallowed. (Icon-only button also lacks an accessible name — pre-existing markup.)	When tabsDisabled, set aria-disabled="true" + tabindex={-1}; add aria-label="New tab".
4	New-tab guard tests rely on fixed cy.wait(500)	Test review	Minor	in-diff (editor.spec.js:566,602)	The new-tab/Alt+T tests force the action then cy.wait(500) then assert the count is unchanged. addBuffer inserts async (IndexedDB → useLiveQuery); deterministic for passing code, but as a regression detector a regressed insert landing after 500ms on a slow runner yields a false pass.	Spy/intercept the buffer insert (or assert the count stays equal across polls) instead of a fixed wait.
5	tabsDisabledRef synced via lagging effect	React correctness	Minor	in-diff (Monaco/index.tsx:1762, :937)	tabsDisabledRef is synced via a passive effect (lags one render) — yet this PR deliberately made the sibling scriptConfirmationOpenRef synchronous. The Alt+T addBuffer command reads the lagging ref with no CSS backstop for keyboard. Not realistically exploitable (single thread + setTimeout(0) drain), but inconsistent.	Mirror the synchronous pattern: set tabsDisabledRef.current next to each setTabsDisabled(...).
6	runAll field name shadows the param	Code structure & types	Minor	in-diff (Monaco/index.tsx:1534)	pendingScriptRunRef.current = { runAll: runsAllQueries } stores Boolean(runAll) || !hasMultipleSelection under a field named runAll, whose meaning differs from the runAll parameter. Reusing the name (then isPendingSelectionRun = ...runAll === false) is a readability trap.	Rename the field to runsAllQueries.
7	Stale "will be aborted" warning when query self-completes	React/Async (UX)	Minor	in-diff (Monaco/index.tsx:2343)	The warning is gated on pendingScriptRunRef.current, not on real running state. If the in-flight query finishes on its own while the dialog is open, .finally skips clearing (gated by scriptConfirmationOpenRef), so the warning keeps showing though nothing is running. Confirm still behaves correctly — cosmetic.	Gate the warning on running !== NONE || questExecution.isAnyRunning().
8	Vestigial editorRef.current guard	Code structure	Minor	in-diff (Monaco/index.tsx:600)	executePendingScriptRun still guards !editorRef.current, but the rewritten body never uses the editor. Harmless leftover from the old executePendingAction.	Drop the check, or keep it as an explicit "still-mounted" guard with a comment.
9	handleRunScript lacks try/finally around tab lock	Cross-context caller impact	Minor	out-of-diff, pre-existing (Monaco/index.tsx:1601→:1732)	setTabsDisabled(true/false) bracket the script loop with no try/finally, and void handleRunScript() has no .catch. If an unguarded dispatch/eventBus/editor.* call in that span throws, tabs + editor stay locked until reload — and now the new-tab button stays permanently dead too. Pre-existing; this PR enlarges the blast radius.	Wrap the body in try { … } finally { setTabsDisabled(false); editor.updateOptions({readOnly:false}) }.
10	EXPLAIN/REFRESH cancel paths untested	Test review	Minor	in-diff (missing)	The cancel early-return fires for any running !== NONE (QUERY/EXPLAIN/REFRESH), but only QUERY-cancel is e2e-tested. Logic is uniform so risk is low.	Add a cancel test for EXPLAIN (and/or refresh).

False-positives (verified and dismissed)

Candidate finding	Why it's a false positive
executePendingScriptRun leaks pendingScriptRunRef when editorRef.current is null	editorRef.current is nulled only in unmount cleanup; no null→non-null transition mid-life.
Removing the scriptConfirmationOpenRef sync effect lets the ref diverge	Only writer of setScriptConfirmationOpen is now setScriptConfirmation, which sets ref + state together — strictly safer.
Stale/aborted query result overwrites the new run's grid	Aborted query → .catch → never setResult; the new run's terminal setResult always wins.
Pending run dropped or run twice (.finally + script-stop + confirm)	executePendingScriptRun clears the ref before dispatching and has no await — idempotent; callers mutually exclusive by running + scriptConfirmationOpenRef.
Selection-run-while-busy could run all queries via cursor-move + 50ms debounce	handleRunScript reads queriesToRunRef in the passive effect right after the SCRIPT dispatch — before the 50ms debounce can recompute it; getErrorRange for a cancelled query is often null. Not reachable.
data-hook editor-tabs → editor-tabs-disabled breaks selectors	getEditorTabs keys off .chrome-tab; the conditional hook is pre-existing (#511); only tests reference these hooks.
Unmemoized EditorProvider context value → re-render storm from the new tabsDisabled read	Real but pre-existing and not worsened — tabsDisabled already drove this churn.
tabsDisabled could get stuck true on the share-link path	unlockEditor() is the first statement of both .then and .catch; always resets.
New pointer-events:none rule doesn't win / doesn't block the click	Specificity (0,3,0) beats base (0,2,0); no inline override. force:true/keyboard caught by JS guards.
Removing validateQueryAtOffset / QUERY-EXPLAIN pending branch drops behavior	On base, pendingActionRef was only ever {type:SCRIPT}/undefined; both were unreachable. normalizeQueryText/getQueriesInRange remain used.
ButtonBar "always cancel" branch conflicts with a rendered button state	Cancel-query button renders only for QUERY/EXPLAIN/REFRESH, run button only for running===NONE, SCRIPT uses a different handler — branch matches render conditions in every state.

Summary

Verdict: Approve, with test fixes requested before merge. No production-blocking product defect.

• The three fixes are correct and verified end-to-end. The dead-code removal and the synchronous setScriptConfirmation (replacing the lagging effect) are safe and a genuine improvement.
• No product regressions. The fresh-context adversarial pass found no reachable behavior bug; its strongest hit (wip test for error range #1) is a test-quality issue, not a behavior change — run-all has always cleared the buffer's notifications.
• The real weakness is the PR's own test net (wip test for error range #1, init react components package #2, add test for query run when cursor position is next to ending semicolon #4): the run-all-while-busy test asserts a torn-down notification and can't isolate the run-all logic. These flows are e2e-only, so weak tests are the only safety net — hence Moderate.
• Quality gate: ESLint passes clean on all PR-changed files; yarn typecheck fails only on a pre-existing, unrelated @questdb/sql-parser StoragePolicy mismatch in two files this PR does not touch; build + unit tests are green.
• Counts: 10 findings verified (2 Moderate test-quality, 8 Minor); 11 candidates dismissed as false positives. In-diff vs out-of-diff: 9 in-diff, 1 out-of-diff — a genuinely localized, self-contained refactor; the cross-context pass returned SAFE for every external consumer contract.

_{🤖 Generated via the review-pr skill (level 3: 13 review agents + per-finding verification).}