Query execution UX: cancel/spinner fixes, target-tab support, run-status glyphs#226
Open
NicolaasGrobler wants to merge 6 commits into
Open
Query execution UX: cancel/spinner fixes, target-tab support, run-status glyphs#226NicolaasGrobler wants to merge 6 commits into
NicolaasGrobler wants to merge 6 commits into
Conversation
… tab Two execution-state bugs that left query running broken or misleading: 1. Cancelling a query froze all further execution (#212). The re-entrancy guard added in #217 keys off `isExecutingRef`, which every run-end path cleared except `cancelQuery`. Because libpq has no real query cancel, the in-flight `db.select` keeps running server-side and only clears the flag when it eventually returns -- so after Cancel the toolbar showed "Run" but the guard silently swallowed every subsequent run and the results bar never came up. `cancelQuery` now clears `isExecutingRef` immediately and bumps the execution generation so the abandoned run no-ops on its late return instead of clobbering whatever ran after the cancel. 2. The "running" spinner followed the active tab (#222). Execution state was a single global `isExecuting` flag gated on `activeTabId === tab.id`, so the tab glyph, the SQL Editor header bar, and the results-panel loading state painted onto whichever tab was active rather than the one that launched the query. A new `executingTabId` records the launching tab; the tab glyph keys off it, and the editor/psql/results loading only show "running" when the active tab is the executing one. The top toolbar Run/Cancel stays global (one query runs at a time; cancel from anywhere). Closes #212 Closes #222 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The top toolbar (Run/Cancel/Format/…), the bottom results panel, and the save-query paths were all gated on the sidebar-selected connection (activeConnection + selectedDatabase). A tab opened from the explorer or restored from a session runs through its own `target` connection in executeQuery without ever setting the context connection, so those gates stayed false: the query executed (the tab spinner showed) but the results bar never appeared, there was no Cancel button, the Run button was greyed out, and Save silently no-opped. Resolve the active tab's connection once (`activeTabConnection` / `activeTabDatabase` — target overrides context, mirroring executeQuery) and drive off it: - `activeTabRunnable` gates the toolbar (incl. Run/Cancel) and the results panel. - the Run button's `disabled` uses `activeTabRunnable`. - the toolbar Save button and the global Ctrl+S handler save via the resolved connection/database instead of erroring. Removes the now-unused `isDatabaseReady`. Inline row editing and transactions still assume the context connection — a deliberately separate, larger follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
addNewTab resolved a tab's target from explicit params or the sidebar selection only. When you work through tab `target`s with nothing selected in the sidebar, a new tab got no target and came up un-runnable (no toolbar, no results, Run greyed out). Add the active tab's own target as the final fallback in the resolution chain (explicit → sidebar → active tab), so opening a new query window inherits the DB you're currently in — matching DataGrip/DBeaver/pgAdmin, where a new editor opens against the current data source. Sidebar precedence is unchanged, so an explicit sidebar selection still wins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The only running indicator while a query ran was a spinner in the editor header bar (next to "[conn] SQL Editor"), detached from the statement actually executing. Show it where it belongs: an amber spinning arc in the gutter next to the running block. It can't ride the existing run-status glyph reconcile — that intentionally freezes mid-run (returns early while isExecuting). So it's a separate, transient Monaco decoration in QueryEditor driven directly by `isExecuting` + `lastExecutedStatement`: it appears at the executing block's line and clears the instant execution ends (success, error, or cancel), at which point the reconciled ✓/✗ takes over. - MainContent passes `lastExecutedStatement` to the editor. - `.statement-glyph-running` is now an animated spinner (was a static dot). - The header-bar spinner is removed (running state now reads from the gutter glyph + tab glyph + Cancel button). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…right block The editor passes the statement's document line via onRun's second arg, but the MainContent wrappers were `(q) => executeQuery(q)` — dropping it. So executeQuery always saw statementInfo=undefined, statementInfos=[], and every line fell back to 1: the run-status ✓/✗ glyphs landed on line 1 instead of the executed block, and lastExecutedStatement was never updated to 'running', so the new running spinner never appeared. - Forward the arg: `(q, info) => executeQuery(q, info)` (editor + psql wrappers). - Set the 'running' indicator at the top of the try, before any await, so the editor reliably renders it the instant you Run (a status set only just before db.select could be overwritten by the result before a render happened). - Running glyph CSS is a pulsing amber dot (opacity animation), since Monaco doesn't honor transform on glyph-margin elements. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces sticky statement-execution gutter glyphs that persist across editor edits and refactors query execution to track per-tab state, so the "running" spinner stays on the tab that initiated the query. New utility helpers map multi-statement selections to document-absolute line numbers and merge glyph results across runs. QueryEditor now reconciles id-keyed decorations without repinning survivors, while MainContent coordinates per-tab execution state, wires components to tab-specific visibility, and improves cancel behavior to prevent stale results from clobbering UI. ChangesSticky Query Statement Glyphs and Per-Tab Execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/layout/MainContent.tsx (3)
3618-3618:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward
statementInfofromQueryEditorintoexecuteQuery.Line 3618 drops the second
onRunargument, so execution falls back to default line metadata and run-status glyphs can pin to the wrong statement.💡 Suggested fix
- onRun={(q: string) => executeQuery(q)} + onRun={(q: string, info?: { lineNumber: number; statementText: string }) => executeQuery(q, info)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/MainContent.tsx` at line 3618, The onRun handler for QueryEditor currently calls executeQuery with only the query string, dropping the second argument statementInfo; update the onRun invocation so it forwards both parameters (pass statementInfo through) to executeQuery (ensure executeQuery's signature is used as-is), so QueryEditor -> onRun -> executeQuery preserves statementInfo and corrects run-status/glyph mapping.
2147-2167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel updates the wrong tab when user cancels from a different tab.
At Line 2165, cancellation state is written to
activeTabIdinstead of the tab that owns the in-flight execution. With per-tab execution tracking, this misattributes the cancel error/time.💡 Suggested fix
const cancelQuery = useCallback(() => { cancelFlagRef.current = true; @@ - if (activeTabId) { - updateTabState(activeTabId, { error: "Query execution cancelled by user.", executionTime: runningTimeMs }); + const ownerTabId = executingTabId ?? activeTabId; + if (ownerTabId) { + updateTabState(ownerTabId, { error: "Query execution cancelled by user.", executionTime: runningTimeMs }); } - }, [runningTimeMs, activeTabId, updateTabState]); + }, [runningTimeMs, activeTabId, executingTabId, updateTabState]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/MainContent.tsx` around lines 2147 - 2167, The cancelQuery handler is writing cancellation state to activeTabId (the currently selected tab) instead of the tab that initiated the running query; change the code in cancelQuery to record the error and executionTime against the executing tab identifier (e.g., use executingTabId or executingTabIdRef/current that tracks the tab which owns the in-flight execution) rather than activeTabId, and only clear the executing tab via setExecutingTabId(null) after updating that executing tab; also guard the updateTabState call with a null check on the executing tab id so you don't try to update when there is none.
865-2168: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd regression tests for the execution ownership + line-mapping bug-fix paths.
This PR changes critical bug-fix behavior (per-tab execution ownership and statement-line glyph targeting), but no targeted failing regression test is included for these flows.
As per coding guidelines, “Every bug fix should land with a failing test that proves it; for bugs that can't be cleanly fixed in the same PR, use Vitest's
it.fails(...)pattern to document the bug in executable form.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/MainContent.tsx` around lines 865 - 2168, Add regression tests exercising executeQuery's execution ownership and line-mapping fixes: write Vitest tests that (1) verify the re-entrancy guard and generation bumping (isExecutingRef.current, executionGenRef.current) prevents double-execution on rapid triggers and that a cancelled run increments executionGenRef so a later run wins; (2) verify mapSelectionStatementsToDocumentLines + the promotion-to-isRunAll path correctly maps statementInfos back to document lines and that appendGlyphResults and lastExecutedStatement are applied to the correct document line when multiple statements are selected; (3) cover the CLI vs libpq branching behavior (useCliPath) to ensure glyphs/psqlEntries are created on the originating tab (runningCmdRef, psqlOutputRef current tab updates); and if any behavior cannot be reproduced yet, add it as an it.fails(...) test documenting the regression. Target tests at executeQuery, mapSelectionStatementsToDocumentLines, appendGlyphResults, and the cancelQuery interaction to lock the regression into CI.Source: Coding guidelines
🧹 Nitpick comments (1)
src/styles/globals.css (1)
341-345: ⚡ Quick winRespect reduced-motion for the pulsing running glyph.
Line 341 introduces a perpetual pulse, but there’s no
prefers-reduced-motionfallback. Add a reduced-motion override so sensitive users don’t get forced animation.Suggested patch
.statement-glyph-running { /* Pulsing amber dot next to the block that is currently executing. Uses the same solid-fill SVG technique as the ✓/✗ glyphs (which render reliably in the glyph margin) and animates opacity rather than transform, which Monaco does not honor on glyph-margin elements. */ background: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"><circle cx="8" cy="8" r="6" fill="%23f59e0b"/></svg>') no-repeat center center !important; background-size: 12px !important; animation: statement-glyph-pulse 1s ease-in-out infinite; } `@keyframes` statement-glyph-pulse { 0%, 100% { opacity: 1; } 50% { opacity: 0.2; } } +@media (prefers-reduced-motion: reduce) { + .statement-glyph-running { + animation: none; + opacity: 1; + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/styles/globals.css` around lines 341 - 345, The pulsing animation for the running glyph (animation: statement-glyph-pulse and `@keyframes` statement-glyph-pulse) lacks a prefers-reduced-motion override; add a media query `@media` (prefers-reduced-motion: reduce) that targets the same selector(s) and disables the animation (e.g., set animation: none and remove/override any transitions) and, if needed, adjust opacity directly so the glyph remains visible without motion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/layout/MainContent.tsx`:
- Line 3618: The onRun handler for QueryEditor currently calls executeQuery with
only the query string, dropping the second argument statementInfo; update the
onRun invocation so it forwards both parameters (pass statementInfo through) to
executeQuery (ensure executeQuery's signature is used as-is), so QueryEditor ->
onRun -> executeQuery preserves statementInfo and corrects run-status/glyph
mapping.
- Around line 2147-2167: The cancelQuery handler is writing cancellation state
to activeTabId (the currently selected tab) instead of the tab that initiated
the running query; change the code in cancelQuery to record the error and
executionTime against the executing tab identifier (e.g., use executingTabId or
executingTabIdRef/current that tracks the tab which owns the in-flight
execution) rather than activeTabId, and only clear the executing tab via
setExecutingTabId(null) after updating that executing tab; also guard the
updateTabState call with a null check on the executing tab id so you don't try
to update when there is none.
- Around line 865-2168: Add regression tests exercising executeQuery's execution
ownership and line-mapping fixes: write Vitest tests that (1) verify the
re-entrancy guard and generation bumping (isExecutingRef.current,
executionGenRef.current) prevents double-execution on rapid triggers and that a
cancelled run increments executionGenRef so a later run wins; (2) verify
mapSelectionStatementsToDocumentLines + the promotion-to-isRunAll path correctly
maps statementInfos back to document lines and that appendGlyphResults and
lastExecutedStatement are applied to the correct document line when multiple
statements are selected; (3) cover the CLI vs libpq branching behavior
(useCliPath) to ensure glyphs/psqlEntries are created on the originating tab
(runningCmdRef, psqlOutputRef current tab updates); and if any behavior cannot
be reproduced yet, add it as an it.fails(...) test documenting the regression.
Target tests at executeQuery, mapSelectionStatementsToDocumentLines,
appendGlyphResults, and the cancelQuery interaction to lock the regression into
CI.
---
Nitpick comments:
In `@src/styles/globals.css`:
- Around line 341-345: The pulsing animation for the running glyph (animation:
statement-glyph-pulse and `@keyframes` statement-glyph-pulse) lacks a
prefers-reduced-motion override; add a media query `@media`
(prefers-reduced-motion: reduce) that targets the same selector(s) and disables
the animation (e.g., set animation: none and remove/override any transitions)
and, if needed, adjust opacity directly so the glyph remains visible without
motion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c7a4dad-8d20-4efd-9bd5-92d995a95ea7
📒 Files selected for processing (6)
CHANGELOG.mdsrc/components/editor/QueryEditor.tsxsrc/components/layout/MainContent.tsxsrc/styles/globals.csssrc/utils/statementGlyphs.test.tssrc/utils/statementGlyphs.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comprehensive branch bundling this session's query-execution fixes plus the run-status glyph work. The branches got entangled across parallel work, so this is one PR rather than several; PR #224 remains the clean cancel/spinner subset if you'd rather land that separately first.
What's in here
Cancel + per-tab running state (also in #224)
cancelQuerynow releases the re-entrancy guard (isExecutingRef) and bumps the execution generation, so after a Cancel you can immediately run again — fixes the freeze in Cancel querie #212.executingTabId), not the active tab, so the spinner stops following tab switches — Running-query spinner follows the active tab instead of staying on the tab that launched the query #222.Target-scoped tabs (the "results bar never comes up" root cause)
isDatabaseReady). A tab running through its owntargetconnection kept them hidden even though the query executed. Now gated onactiveTabRunnable(resolves the tab's own connection), and the Run button + both Save paths use the resolved connection too.New tab inherits the current DB
addNewTabnow falls back to the active tab'starget(chain: explicit → sidebar → active tab), so a new query window opens against the DB you're already in — matching DataGrip/DBeaver/pgAdmin.Run-status gutter glyphs + live running spinner
onRunwrappers were(q) => executeQuery(q), dropping the statement-line argument, sostatementInfoswas always empty and every glyph defaulted to line 1. Forwarding the arg fixes the ✓/✗ landing on the right block.awaitso it renders reliably. Header-bar spinner removed.Testing status (honest)
onRunforwarding) was the last change and the live test loop was cut short — it typechecks and the logic is verified by the earlier[running-glyph]debug trace, but a final visual confirm of the gutter spinner is still pending.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests