fix: release execution guard on cancel and pin running spinner to its tab#224
Open
NicolaasGrobler wants to merge 1 commit into
Open
fix: release execution guard on cancel and pin running spinner to its tab#224NicolaasGrobler wants to merge 1 commit into
NicolaasGrobler wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 42 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fixes two related query execution issues: canceling a running query no longer freezes subsequent executions, and the "running" spinner remains pinned to the tab that launched the query even when the user switches tabs mid-execution. The changes introduce per-tab execution tracking, rework glyph accumulation to preserve results across runs, and update all execution paths and UI components to respect launching-tab state. ChangesQuery Execution & Cancellation Fixes
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related Issues
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 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
CHANGELOG.md (1)
15-16: ⚡ Quick winAdd issue number links for consistency.
Both entries document fixes that close specific issues (
#212and#222per the PR objectives), but they don't include the standard[#N](...)link format used elsewhere in the changelog. For example, line 14 uses the full**[#217](...)**format.📝 Suggested addition
-- **Cancelling a query no longer freezes query execution.** The re-entrancy guard added in `#217` keys off `isExecutingRef`, which every code path cleared on completion — except `cancelQuery`, which only flipped the visible spinner off. 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 pressing **Cancel** the toolbar showed "Run" but the guard silently swallowed every subsequent run and the results bar never came up. `cancelQuery` now clears the executing flag immediately (and bumps the execution generation so the abandoned run no-ops on its late return instead of clobbering whatever ran after the cancel). -- **The "running" spinner now stays on the tab that launched the query.** Execution state was a single global `isExecuting` flag, and every indicator (the tab status glyph, the SQL Editor header bar, the results-panel loading state) was gated on `activeTabId === tab.id` — so the spinner painted onto whichever tab was *active* and appeared to "follow" you when you switched tabs, even though a different tab owned the running query. A new `executingTabId` records which tab actually started the run: the tab glyph is keyed to that tab, and the editor header / psql window / results-panel loading only show "running" when the active tab is the executing one. (The top toolbar's Run/Cancel control stays global, since QueryDen runs one query at a time and you can cancel it from anywhere.) +- **[`#212`](https://github.com/openidle-dev/queryden/issues/212) — Cancelling a query no longer freezes query execution.** The re-entrancy guard added in `#217` keys off `isExecutingRef`, which every code path cleared on completion — except `cancelQuery`, which only flipped the visible spinner off. 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 pressing **Cancel** the toolbar showed "Run" but the guard silently swallowed every subsequent run and the results bar never came up. `cancelQuery` now clears the executing flag immediately (and bumps the execution generation so the abandoned run no-ops on its late return instead of clobbering whatever ran after the cancel). +- **[`#222`](https://github.com/openidle-dev/queryden/issues/222) — The "running" spinner now stays on the tab that launched the query.** Execution state was a single global `isExecuting` flag, and every indicator (the tab status glyph, the SQL Editor header bar, the results-panel loading state) was gated on `activeTabId === tab.id` — so the spinner painted onto whichever tab was *active* and appeared to "follow" you when you switched tabs, even though a different tab owned the running query. A new `executingTabId` records which tab actually started the run: the tab glyph is keyed to that tab, and the editor header / psql window / results-panel loading only show "running" when the active tab is the executing one. (The top toolbar's Run/Cancel control stays global, since QueryDen runs one query at a time and you can cancel it from anywhere.)🤖 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 `@CHANGELOG.md` around lines 15 - 16, The changelog entries describing fixes for issues `#212` and `#222` lack the standardized issue-link format used elsewhere (e.g., **[`#217`](...)**); update those two bullet points so each referenced issue uses the same `[`#N`](URL)` markdown link pattern (replace N with 212 and 222 and point to the corresponding issue URLs), ensuring the link text and formatting match the existing **[`#217`](...)** style and keeping the rest of the entry text unchanged.
🤖 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.
Inline comments:
In `@src/components/layout/MainContent.tsx`:
- Around line 928-934: The variable mapped is inferred as any[] causing implicit
any for parameter p; fix by giving mapped an explicit element type (e.g., const
mapped: { text: string; lineNumber: number }[] =
mapSelectionStatementsToDocumentLines(specificQuery, baseLine)) or by adding a
proper return type to mapSelectionStatementsToDocumentLines so callers (and the
mapped variable) get a typed array; then the mapped.map calls that populate
statementsToRun and statementInfos will have correctly typed p objects.
- Line 15: MainContent.tsx imports mapSelectionStatementsToDocumentLines and
mergeGlyphResults from a non-existent ../../utils/statementGlyphs — locate where
those functions are used in MainContent and either add a new
src/utils/statementGlyphs.ts that exports those exact names or change the import
to the correct existing module that exports them (ensure exported function names
match). Also stop passing the undeclared prop onStatementResultsChange into
<QueryEditor /> or add it to QueryEditorProps in QueryEditor.tsx: declare
onStatementResultsChange?: (results: StatementResult[]) => void (or the
appropriate signature) and forward/use it inside QueryEditor, or remove the prop
usage in MainContent and use the existing statementResults prop instead. Ensure
the symbols mapSelectionStatementsToDocumentLines, mergeGlyphResults,
QueryEditor, QueryEditorProps, onStatementResultsChange, and statementResults
are updated consistently.
- Around line 3592-3596: MainContent passes onStatementResultsChange to
QueryEditor but QueryEditorProps lacks that prop so the callback is never typed
or invoked; add onStatementResultsChange?: (results: StatementResult[]) => void
to the QueryEditorProps interface in QueryEditor.tsx and, wherever
statementResults are updated inside the QueryEditor component (the state updater
or effect that sets statementResults), call
props.onStatementResultsChange?.(newStatementResults); keep MainContent’s usage
(onStatementResultsChange={(rs) => activeTabId && setGlyphResults(activeTabId,
rs)}) and ensure types for StatementResult are imported/used consistently so
setGlyphResults and activeTabId integration compiles.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 15-16: The changelog entries describing fixes for issues `#212` and
`#222` lack the standardized issue-link format used elsewhere (e.g.,
**[`#217`](...)**); update those two bullet points so each referenced issue uses
the same `[`#N`](URL)` markdown link pattern (replace N with 212 and 222 and point
to the corresponding issue URLs), ensuring the link text and formatting match
the existing **[`#217`](...)** style and keeping the rest of the entry text
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27808037-58c8-4910-9da8-8c8711f203f5
📒 Files selected for processing (2)
CHANGELOG.mdsrc/components/layout/MainContent.tsx
| import { logger } from "../../utils/logger"; | ||
| import { getDefaultDatabaseName } from "../../config/app"; | ||
| import { splitStatements } from "../../utils/splitStatements"; | ||
| import { mapSelectionStatementsToDocumentLines, mergeGlyphResults } from "../../utils/statementGlyphs"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the module exists and check for similar files
echo "Searching for statementGlyphs module..."
fd -e ts -e tsx statementGlyphs
echo ""
echo "Checking utils directory contents..."
fd . src/utils --type f 2>/dev/null | head -20
echo ""
echo "Searching for mergeGlyphResults definition anywhere..."
rg -n "export.*mergeGlyphResults" --type tsRepository: openidle-dev/queryden
Length of output: 765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for any references to statementGlyphs..."
rg -n "statementGlyphs" -S --glob='*.ts' --glob='*.tsx' .
echo
echo "Searching for mapSelectionStatementsToDocumentLines definitions/usages..."
rg -n "mapSelectionStatementsToDocumentLines" -S --glob='*.ts' --glob='*.tsx' .
echo
echo "Searching for mergeGlyphResults definitions/usages..."
rg -n "mergeGlyphResults" -S --glob='*.ts' --glob='*.tsx' .Repository: openidle-dev/queryden
Length of output: 1198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find QueryEditorProps definition =="
rg -n "interface QueryEditorProps|type QueryEditorProps|QueryEditorProps" -S --glob='*.ts' --glob='*.tsx' src
echo
echo "== Check where onStatementResultsChange is passed =="
rg -n "onStatementResultsChange" -S --glob='*.ts' --glob='*.tsx' src
echo
echo "== Inspect MainContent around the prop usage (near 3596) =="
# Show a narrow window around the line number mentioned in the review
nl -ba src/components/layout/MainContent.tsx | sed -n '3570,3630p'
echo
echo "== Confirm statementGlyphs module file presence =="
fd statementGlyphs src --type fRepository: openidle-dev/queryden
Length of output: 560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect QueryEditor.tsx: QueryEditorProps interface/type =="
sed -n '70,130p' src/components/editor/QueryEditor.tsx
echo
sed -n '130,220p' src/components/editor/QueryEditor.tsx
echo
echo "== Inspect MainContent.tsx around the onStatementResultsChange usage =="
# Using cat -n to get line context without nl
cat -n src/components/layout/MainContent.tsx | sed -n '3560,3635p'
echo
echo "== Inspect MainContent.tsx import area for statementGlyphs path =="
sed -n '1,40p' src/components/layout/MainContent.tsx
echo
echo "== Confirm statementGlyphs file presence under src/ (name match) =="
fd -i "statementGlyphs" src --type fRepository: openidle-dev/queryden
Length of output: 12967
Fix broken statementGlyphs import and invalid QueryEditor prop
src/components/layout/MainContent.tsximports{ mapSelectionStatementsToDocumentLines, mergeGlyphResults }from../../utils/statementGlyphs, but there is nosrc/utils/statementGlyphs.*file in the repo (so the build will fail). (line 15)
import { mapSelectionStatementsToDocumentLines, mergeGlyphResults } from "../../utils/statementGlyphs";src/components/layout/MainContent.tsxpassesonStatementResultsChangeto<QueryEditor />, butQueryEditorPropsinsrc/components/editor/QueryEditor.tsxdoes not define that prop (it only hasstatementResults?: StatementResult[]). (line 3596)
🧰 Tools
🪛 GitHub Check: Frontend (typecheck + test + build)
[failure] 15-15:
Cannot find module '../../utils/statementGlyphs' or its corresponding type declarations.
🤖 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 15, MainContent.tsx imports
mapSelectionStatementsToDocumentLines and mergeGlyphResults from a non-existent
../../utils/statementGlyphs — locate where those functions are used in
MainContent and either add a new src/utils/statementGlyphs.ts that exports those
exact names or change the import to the correct existing module that exports
them (ensure exported function names match). Also stop passing the undeclared
prop onStatementResultsChange into <QueryEditor /> or add it to QueryEditorProps
in QueryEditor.tsx: declare onStatementResultsChange?: (results:
StatementResult[]) => void (or the appropriate signature) and forward/use it
inside QueryEditor, or remove the prop usage in MainContent and use the existing
statementResults prop instead. Ensure the symbols
mapSelectionStatementsToDocumentLines, mergeGlyphResults, QueryEditor,
QueryEditorProps, onStatementResultsChange, and statementResults are updated
consistently.
Source: Linters/SAST tools
Comment on lines
928
to
934
| const baseLine = statementInfo?.lineNumber ?? 1; | ||
| const mapped = mapSelectionStatementsToDocumentLines(specificQuery, baseLine); | ||
| if (mapped.length > 1) { | ||
| isRunAll = true; | ||
| statementsToRun = parts.map(p => p.text); | ||
| statementInfos = parts.map(p => ({ lineNumber: p.lineNumber, statementText: p.text })); | ||
| statementsToRun = mapped.map(p => p.text); | ||
| statementInfos = mapped.map(p => ({ lineNumber: p.lineNumber, statementText: p.text })); | ||
| } |
There was a problem hiding this comment.
Add explicit type for mapped array elements to fix implicit any.
TypeScript reports that parameter p implicitly has an any type. Once the statementGlyphs module is created, ensure its return type is properly typed, or add an inline type assertion.
- const mapped = mapSelectionStatementsToDocumentLines(specificQuery, baseLine);
+ const mapped = mapSelectionStatementsToDocumentLines(specificQuery, baseLine) as { text: string; lineNumber: number }[];
if (mapped.length > 1) {
isRunAll = true;
- statementsToRun = mapped.map(p => p.text);
- statementInfos = mapped.map(p => ({ lineNumber: p.lineNumber, statementText: p.text }));
+ statementsToRun = mapped.map((p) => p.text);
+ statementInfos = mapped.map((p) => ({ lineNumber: p.lineNumber, statementText: p.text }));
}Alternatively, define a proper return type in the module export.
🧰 Tools
🪛 GitHub Check: Frontend (typecheck + test + build)
[failure] 933-933:
Parameter 'p' implicitly has an 'any' type.
[failure] 932-932:
Parameter 'p' implicitly has an 'any' type.
🤖 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 928 - 934, The variable
mapped is inferred as any[] causing implicit any for parameter p; fix by giving
mapped an explicit element type (e.g., const mapped: { text: string; lineNumber:
number }[] = mapSelectionStatementsToDocumentLines(specificQuery, baseLine)) or
by adding a proper return type to mapSelectionStatementsToDocumentLines so
callers (and the mapped variable) get a typed array; then the mapped.map calls
that populate statementsToRun and statementInfos will have correctly typed p
objects.
Source: Linters/SAST tools
Comment on lines
+3592
to
+3596
| isExecuting={activeTabIsExecuting} | ||
| hasError={!!error} | ||
| hasSuccess={!!success} | ||
| statementResults={activeTab?.statementResults} | ||
| onStatementResultsChange={(rs) => activeTabId && setGlyphResults(activeTabId, rs)} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if QueryEditor uses onStatementResultsChange
echo "Checking QueryEditor for onStatementResultsChange usage..."
rg -n "onStatementResultsChange" src/components/editor/QueryEditor.tsx
echo ""
echo "Checking QueryEditorProps definition..."
rg -A 20 "interface QueryEditorProps" src/components/editor/QueryEditor.tsxRepository: openidle-dev/queryden
Length of output: 750
Fix onStatementResultsChange prop mismatch between MainContent and QueryEditor.
src/components/layout/MainContent.tsx passes onStatementResultsChange={(rs) => activeTabId && setGlyphResults(activeTabId, rs)} to <QueryEditor />, but src/components/editor/QueryEditor.tsx’s QueryEditorProps does not declare onStatementResultsChange (and there are no references to it), so this should cause a TypeScript JSX props error and the callback will never be called.
// QueryEditor.tsx
interface QueryEditorProps {
// ...
onStatementResultsChange?: (results: StatementResult[]) => void;
}Then invoke onStatementResultsChange in QueryEditor.tsx at the point where statementResults are updated (or remove the prop from MainContent).
🧰 Tools
🪛 GitHub Check: Frontend (typecheck + test + build)
[failure] 3596-3596:
Parameter 'rs' implicitly has an 'any' type.
[failure] 3596-3596:
Type '{ key: string; value: string; onChange: (query: string) => void; onRun: (q: string) => Promise; connectionName: string | undefined; databaseName: string | undefined; tabId: string; ... 5 more ...; onStatementResultsChange: (rs: any) => void | ... 1 more ... | null; }' is not assignable to type 'IntrinsicAttributes & QueryEditorProps'.
🤖 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 3592 - 3596, MainContent
passes onStatementResultsChange to QueryEditor but QueryEditorProps lacks that
prop so the callback is never typed or invoked; add onStatementResultsChange?:
(results: StatementResult[]) => void to the QueryEditorProps interface in
QueryEditor.tsx and, wherever statementResults are updated inside the
QueryEditor component (the state updater or effect that sets statementResults),
call props.onStatementResultsChange?.(newStatementResults); keep MainContent’s
usage (onStatementResultsChange={(rs) => activeTabId &&
setGlyphResults(activeTabId, rs)}) and ensure types for StatementResult are
imported/used consistently so setGlyphResults and activeTabId integration
compiles.
Source: Linters/SAST tools
… 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>
NicolaasGrobler
force-pushed
the
fix/cancel-guard-release-and-per-tab-spinner
branch
from
ddd7073 to
319cfc2
Compare
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.
Fixes two execution-state bugs in
MainContent.tsx. They share the samecancelQuery/finallycode, so they're cleanest in one PR.1. Cancelling a query froze all further execution — closes #212
The re-entrancy guard added in #217 keys off
isExecutingRef:Every run-end path clears both
setIsExecuting(false)andisExecutingRef.current = false— exceptcancelQuery, which only flipped the visible spinner off. libpq has no real query cancel, so the in-flightdb.selectkeeps running server-side and only clears the flag in itsfinallywhen it eventually returns. Until then the toolbar showed "Run" but the guard silently swallowed every subsequent run, and the results bar never came up (the reporter on #212 had to restart the app).Fix:
cancelQuerynow clearsisExecutingRefimmediately and bumps the execution generation (executionGenRef) 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 — closes #222
Execution state was a single global
isExecutingflag, and every indicator (tab status glyph, SQL Editor header bar, results-panel loading) was gated onactiveTabId === tab.id. So the spinner painted onto whichever tab was active, not the tab that launched the query — switch tabs mid-run and it followed you.Fix: a new
executingTabIdrecords the tab that started the run. The tab glyph keys offexecutingTabId === tab.id; the editor header / psql window / results-panel loading useisExecuting && executingTabId === activeTabIdso they only show "running" when the active tab is the executing one. The top toolbar Run/Cancel stays global (QueryDen runs one query at a time and you can cancel it from any tab).Verification
npx tsc --noEmitclean;npm test174/174 pass.tauri-plugin-sqlconnection. Manual test once running: (1) run a slow query → Cancel → run another → results appear; (2) run a slow query on tab A, switch to B → spinner stays on A, B's header shows none.🤖 Generated with Claude Code
Summary by CodeRabbit