From ddd7073651117bad5be66bec7b7473ecec0de9b8 Mon Sep 17 00:00:00 2001 From: NicolaasGrobler Date: Tue, 9 Jun 2026 12:06:47 +0800 Subject: [PATCH 1/6] fix: release execution guard on cancel and pin running spinner to its 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) --- CHANGELOG.md | 2 + src/components/layout/MainContent.tsx | 143 +++++++++++++++++++------- 2 files changed, 109 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d000d18..f92d270 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable changes to QueryDen are documented here. This project adheres to [Se ### Fixed - **[#217](https://github.com/openidle-dev/queryden/issues/217) — Running a query now gives instant feedback and can't be triggered twice.** Pressing `Ctrl+Enter` previously showed no "running" state until *after* the database connection handshake (`Database.load`), which can take seconds on a cold connection — so the editor looked idle and a second press would run the query again. `setIsExecuting(true)` now fires synchronously at the start of execution (the spinner and tab status glyph appear the instant you press the shortcut), and a re-entrancy guard drops a duplicate trigger while a run is already in flight. +- **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.) - **You can now close the last remaining query tab.** The tab close button was hidden whenever only one tab was open, leaving no way to close the final editor. The button is always available now; closing the last tab falls back to the empty-state launcher. - **PSQL Console error messages no longer flash and disappear.** Errors in the CLI execution path (PostgreSQL version unknown, download cancelled, download failed, psql not found, `\watch` errors) are now committed as persistent `psqlConsoleEntry` entries before the live-output grace period expires, preventing the output from going blank after 300ms. The `\watch` loop-end path also commits accumulated output. Fixes Windows WebView2 race where `showLiveGrace` expired without entries. - **"Open in Explorer" now works reliably on Windows.** Empty tabs no longer silently fail — the auto-save directory is resolved as a fallback parent path. All catch blocks now show toast feedback (`showToastMessage`) instead of silent failure. diff --git a/src/components/layout/MainContent.tsx b/src/components/layout/MainContent.tsx index e7d82dd..0f3f04c 100644 --- a/src/components/layout/MainContent.tsx +++ b/src/components/layout/MainContent.tsx @@ -12,6 +12,7 @@ import { EmptyStateLauncher } from "./EmptyStateLauncher"; import { logger } from "../../utils/logger"; import { getDefaultDatabaseName } from "../../config/app"; import { splitStatements } from "../../utils/splitStatements"; +import { mapSelectionStatementsToDocumentLines, mergeGlyphResults } from "../../utils/statementGlyphs"; import { applyQueryLimit } from "../../utils/applyQueryLimit"; import { VariableSubstitutionDialog, extractVariables, substituteVariables, VariableValues } from "../ui/VariableSubstitutionDialog"; import { useLocalHistory } from "../../store/localHistoryStore"; @@ -92,6 +93,15 @@ export interface PsqlConsoleEntry { } export interface StatementResult { + /** + * Stable id assigned when the glyph is accumulated into a tab's results. + * The editor keys its sticky Monaco decorations off this so a glyph can + * follow edits (and be pruned when its block is destroyed) without being + * re-pinned to a frozen line number on every re-render. Optional because + * the per-run results built during execution don't have one yet — it's + * assigned by `appendGlyphResults`. + */ + id?: string; lineNumber: number; status: 'running' | 'success' | 'error'; rowsAffected?: number; @@ -122,6 +132,13 @@ export function MainContent() { const [activeTabId, setActiveTabId] = useState(null); const [results, setResults] = useState([]); const [isExecuting, setIsExecuting] = useState(false); + // Which tab owns the in-flight query. `isExecuting` is a single global flag + // (only one query runs at a time), but the running spinner must point at the + // tab that actually launched the query — not whichever tab happens to be + // active — otherwise the indicator appears to "follow" you when you switch + // tabs. Set when a run starts; cleared when it ends, is cancelled, or is + // superseded by a newer run. + const [executingTabId, setExecutingTabId] = useState(null); const [error, setError] = useState(null); const [success, setSuccess] = useState(null); const [multiResults, setMultiResults] = useState([]); @@ -714,6 +731,28 @@ export function MainContent() { setQueryTabs(prev => prev.map(t => t.id === tabId ? { ...t, ...updates } : t)); }, []); + // Accumulate run-status gutter glyphs for a tab. Each run's results are + // merged into whatever the tab already has (one glyph per block that's been + // run; re-running a block replaces its glyph — see mergeGlyphResults), rather + // than wiping the lot every execution. Runs through the functional setState so + // it always merges into the freshest results, including the editor's own + // sticky-position writebacks. Ids are assigned here so the editor can key its + // decorations stably. + const appendGlyphResults = useCallback((tabId: string, newResults: StatementResult[]) => { + if (newResults.length === 0) return; + const withIds = newResults.map(r => ({ ...r, id: crypto.randomUUID() })); + setQueryTabs(prev => prev.map(t => t.id === tabId + ? { ...t, statementResults: mergeGlyphResults(t.statementResults ?? [], withIds) } + : t)); + }, []); + + // Replace the accumulated glyph set wholesale — used by the editor's writeback + // when it prunes destroyed glyphs or refreshes their line numbers as the text + // is edited. + const setGlyphResults = useCallback((tabId: string, results: StatementResult[]) => { + setQueryTabs(prev => prev.map(t => t.id === tabId ? { ...t, statementResults: results } : t)); + }, []); + const addNewTab = useCallback(( query = "", name = "", @@ -879,11 +918,19 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l // commands into a prepared statement", and the libpq path further down // sends one execute() per statement only when isRunAll is set. if (!isRunAll && typeof specificQuery === "string") { - const parts = splitStatements(queryToRun); - if (parts.length > 1) { + // Split the *original* selection (not the trimmed queryToRun) and map each + // statement back to its document-absolute line. `statementInfo.lineNumber` + // is the document line where the selection (char 0 of specificQuery) began, + // so a statement reported at relative line N sits on document line + // baseLine + (N - 1). Using splitStatements(queryToRun) here was the bug + // that put glyphs on the wrong block: those line numbers are relative to + // the selection, not the document. + 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 })); } } runningCmdRef.current = queryToRun; @@ -959,11 +1006,12 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l setMultiResults([]); setRunningTimeMs(0); cancelFlagRef.current = false; - - // Clear statement results when starting new execution (glyphs will appear after execution completes) - if (currentTabId) { - updateTabState(currentTabId, { statementResults: [] }); - } + + // Note: we intentionally do NOT clear statementResults here. Glyphs now + // accumulate (one per block that's been run) and follow the text as it's + // edited, so wiping them on every run would defeat the feature. Each run + // merges its result into the existing set via appendGlyphResults; re-running + // a block replaces just that block's glyph. const startTime = Date.now(); let intervalId: any = null; @@ -977,6 +1025,9 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l // per-path `setIsExecuting(true)` calls below are now redundant no-ops. setIsExecuting(true); isExecutingRef.current = true; + // Pin the running indicator to the tab that launched this run so it + // doesn't follow tab switches (see executingTabId). + setExecutingTabId(currentTabId || null); // Declare execution state at the top so both libpq and CLI paths can reference them let rows: any[] = []; @@ -1392,7 +1443,7 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l error: mr.error, executionTime: 0 }))); - if (currentTabId) updateTabState(currentTabId, { statementResults }); + if (currentTabId) appendGlyphResults(currentTabId, statementResults); const selectResults = multiResults.filter(r => r.rows && r.rows.length > 0); if (selectResults.length > 0) { rows = selectResults[0].rows || []; @@ -1509,8 +1560,8 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l rows = cliRows ?? []; rowsAffected = rows.length; statementResults.push({ lineNumber: stmtInfo.lineNumber, status: 'success', rowCount: rowsAffected, executionTime: Date.now() - stmtStartTime }); - // Store columns for the ResultPanel - if (currentTabId) updateTabState(currentTabId, { statementResults, columns: cliCols ?? [] }); + // Store columns for the ResultPanel (glyphs are appended below). + if (currentTabId) updateTabState(currentTabId, { columns: cliCols ?? [] }); setLastColumns(cliCols ?? []); } else { const { rowsAffected: affected } = await cliExecStmt(queryToRun, false, currentPsqlExpanded, currentCliDatabase || ""); @@ -1518,7 +1569,6 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l setSuccess(`Query executed successfully. ${rowsAffected} rows affected.`); rows = []; statementResults.push({ lineNumber: stmtInfo.lineNumber, status: 'success', rowsAffected, executionTime: Date.now() - stmtStartTime }); - if (currentTabId) updateTabState(currentTabId, { statementResults }); } } catch (stmtErr: any) { // Show the error in the psql terminal @@ -1546,7 +1596,7 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l isExecutingRef.current = false; return; } - if (currentTabId) updateTabState(currentTabId, { statementResults }); + if (currentTabId) appendGlyphResults(currentTabId, statementResults); } // Skip the libpq block entirely @@ -1733,9 +1783,9 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l // Store statement results for gutter glyphs if (currentTabId) { - updateTabState(currentTabId, { statementResults }); + appendGlyphResults(currentTabId, statementResults); } - + // Combine results - use first SELECT result, or show counts for all const selectResults = multiResults.filter(r => r.rows && r.rows.length > 0); if (selectResults.length > 0) { @@ -1775,7 +1825,7 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l // Store statement results for gutter glyphs if (currentTabId) { - updateTabState(currentTabId, { statementResults }); + appendGlyphResults(currentTabId, statementResults); } } @@ -2030,20 +2080,18 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l error: errorMsg, executionTime: duration }; - - // If we already have partial results from multi-statement execution, update them - // Note: statementResults is inside the inner try block, not accessible here. - // Start with the error result. - const updatedStatementResults: StatementResult[] = [errorStatementResult]; - - updateTabState(currentTabId, { - error: errorMsg, - success: null, + + // Accumulate the error glyph onto whatever the tab already has (it + // replaces any prior glyph on the same line — see mergeGlyphResults). + appendGlyphResults(currentTabId, [errorStatementResult]); + + updateTabState(currentTabId, { + error: errorMsg, + success: null, executionTime: duration, - statementResults: updatedStatementResults, - lastExecutedStatement: { - lineNumber: errorLineNumber, - status: 'error' + lastExecutedStatement: { + lineNumber: errorLineNumber, + status: 'error' } }); } @@ -2065,13 +2113,27 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l cancelFlagRef.current = false; setIsExecuting(false); isExecutingRef.current = false; + setExecutingTabId(null); } } }, [activeConnection, selectedDatabase, addQuery, currentDb, vaultCredentials, settings, confirmDialog]); const cancelQuery = useCallback(() => { cancelFlagRef.current = true; + // Release the re-entrancy guard NOW. libpq has no real query cancel: the + // in-flight `db.select` keeps running server-side and only resets the + // executing flags in its `finally` when it eventually returns — which for + // a heavy query can be a long time, or effectively never. Until then + // `isExecutingRef` would stay true and the guard at the top of + // executeQuery silently swallows every new run, so the results bar never + // comes up after a cancel. Clearing the flag lets the next query start. + // Bumping the generation makes the abandoned run a stale generation, so + // its late resolution no-ops at the gen checkpoints/finally instead of + // clobbering the state of whatever ran after the cancel. + executionGenRef.current++; + isExecutingRef.current = false; setIsExecuting(false); + setExecutingTabId(null); setError("Query execution cancelled by user."); setExecutionTime(runningTimeMs); if (activeTabId) { @@ -3037,6 +3099,12 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l executeQueryRef.current = executeQuery; }); + // True only when the *active* tab is the one running a query. The editor + // header, psql window, and results-panel loading state render for the active + // tab only, so they use this rather than the global `isExecuting` — otherwise + // they'd show a spinner after switching away to a tab that isn't running. + const activeTabIsExecuting = isExecuting && executingTabId === activeTabId; + return (
{/* Variable Substitution Dialog */} @@ -3298,8 +3366,10 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l ? tabConnectionName.substring(0, 10) + "..." : tabConnectionName; - // Determine status for this tab - const tabIsExecuting = activeTabId === tab.id && isExecuting; + // Determine status for this tab. Executing status is keyed to the + // tab that actually launched the query (executingTabId), not the + // active tab, so the spinner stays on its own tab when you switch. + const tabIsExecuting = isExecuting && executingTabId === tab.id; const tabHasError = tab.error && activeTabId === tab.id; const tabHasSuccess = tab.success && activeTabId === tab.id && !tab.error; @@ -3481,8 +3551,8 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l 0 ? psqlOutput : stashPsqlOutputRef.current} - runningCommand={isExecuting ? (runningCmdRef.current || activeTab.query || "") : null} - isExecuting={isExecuting} + runningCommand={activeTabIsExecuting ? (runningCmdRef.current || activeTab.query || "") : null} + isExecuting={activeTabIsExecuting} executionTime={executionTime} onRun={(q: string) => executeQuery(q)} onClear={() => { @@ -3519,10 +3589,11 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l databaseName={activeTab?.target?.database || selectedDatabase || undefined} tabId={activeTabId!} tabName={activeTab?.name} - isExecuting={isExecuting} + isExecuting={activeTabIsExecuting} hasError={!!error} hasSuccess={!!success} statementResults={activeTab?.statementResults} + onStatementResultsChange={(rs) => activeTabId && setGlyphResults(activeTabId, rs)} /> ) @@ -3545,7 +3616,7 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l error={error} successMessage={success} multiResults={multiResults} - isLoading={isExecuting} + isLoading={activeTabIsExecuting} executionTime={executionTime} tableName={activeTableName || undefined} columnTypes={tableColumnTypes?.types} From 5cb029c6f9bfcbde5905ddc58910223a5fa76f99 Mon Sep 17 00:00:00 2001 From: NicolaasGrobler Date: Tue, 9 Jun 2026 12:16:06 +0800 Subject: [PATCH 2/6] wip backup: full glyph+spinner combined state (safety net) --- src/components/editor/QueryEditor.tsx | 279 +++++++++++++++++++------- src/utils/statementGlyphs.test.ts | 65 ++++++ src/utils/statementGlyphs.ts | 60 ++++++ 3 files changed, 330 insertions(+), 74 deletions(-) create mode 100644 src/utils/statementGlyphs.test.ts create mode 100644 src/utils/statementGlyphs.ts diff --git a/src/components/editor/QueryEditor.tsx b/src/components/editor/QueryEditor.tsx index f4f965d..22dc6fd 100644 --- a/src/components/editor/QueryEditor.tsx +++ b/src/components/editor/QueryEditor.tsx @@ -110,9 +110,19 @@ interface QueryEditorProps { status: 'running' | 'success' | 'error'; }; statementResults?: StatementResult[]; + /** + * Writeback for the run-status glyphs. The editor owns the *positions* of the + * glyphs once they exist (they're sticky Monaco decorations that follow + * edits), so when it prunes a glyph whose block was destroyed or refreshes a + * glyph's line number after an edit, it reports the new accumulated set back + * up so the tab state stays in sync (and survives a tab switch / remount). + */ + onStatementResultsChange?: (results: StatementResult[]) => void; } export interface StatementResult { + /** Stable id used to key the sticky gutter decoration to this result. */ + id?: string; lineNumber: number; status: 'running' | 'success' | 'error'; rowsAffected?: number; @@ -121,6 +131,44 @@ export interface StatementResult { executionTime?: number; } +// Build the Monaco glyph-margin decoration options for a run result. +// `NeverGrowsWhenTypingAtEdges` keeps the zero-width glyph range from absorbing +// text typed right at the line start, so the glyph tracks the line cleanly. +function buildGlyphDecoration(monaco: any, result: StatementResult, lineNumber: number) { + const { status, rowCount, rowsAffected, error, executionTime } = result; + + let glyphClassName = 'statement-glyph-running'; + let hoverMessage = 'Query running...'; + let tooltip = ''; + + if (status === 'success') { + glyphClassName = 'statement-glyph-success'; + hoverMessage = 'Query succeeded'; + if (rowCount !== undefined) { + tooltip = `${rowCount} row${rowCount !== 1 ? 's' : ''} retrieved`; + } else if (rowsAffected !== undefined) { + tooltip = `${rowsAffected} row${rowsAffected !== 1 ? 's' : ''} affected`; + } + if (executionTime !== undefined && executionTime > 0) { + tooltip += tooltip ? ` in ${executionTime}ms` : `${executionTime}ms`; + } + } else if (status === 'error') { + glyphClassName = 'statement-glyph-error'; + hoverMessage = 'Query failed'; + tooltip = error || 'Error executing query'; + } + + return { + range: new monaco.Range(lineNumber, 1, lineNumber, 1), + options: { + isWholeLine: false, + glyphMarginClassName: glyphClassName, + glyphMarginHoverMessage: { value: tooltip || hoverMessage }, + stickiness: monaco.editor.TrackedRangeStickiness.NeverGrowsWhenTypingAtEdges, + }, + }; +} + // Show intention actions popup (Alt+Enter) const showIntentionActions = (editor: any, monaco: any, onRunRef: React.MutableRefObject) => { const model = editor.getModel(); @@ -245,18 +293,33 @@ export const QueryEditor = memo(function QueryEditor({ hasError, hasSuccess, lastExecutedStatement: _lastExecutedStatement, - statementResults + statementResults, + onStatementResultsChange }: QueryEditorProps) { const { theme } = useTheme(); const settings = useSettings(); const editorRef = useRef(null); const monacoRef = useRef(null); - const decorationsRef = useRef([]); const onRunRef = useRef(null); const lastSnapshotRef = useRef(""); const snapshotTimerRef = useRef(null); const { schemaItems } = useConnections(); - + + // ─── Run-status gutter glyphs (green/red checkmarks per executed block) ───── + // These are sticky Monaco decorations: once created they follow edits (insert + // a line above a block and its glyph moves down with it) and are pruned when + // their block is destroyed. We key them by the StatementResult id so a glyph + // is never re-pinned to a frozen line number on re-render — the only source + // of truth for a glyph's *position* is Monaco itself. + const glyphDecoRef = useRef>(new Map()); // result id -> monaco decoration id + const glyphAnchorRef = useRef>(new Map()); // result id -> trimmed text of its anchor line + // Latest props captured into refs so the mount-time content-change handler + // (a stable closure) can read them without stale values. + const statementResultsRef = useRef(statementResults); + const onStatementResultsChangeRef = useRef(onStatementResultsChange); + statementResultsRef.current = statementResults; + onStatementResultsChangeRef.current = onStatementResultsChange; + onRunRef.current = onRun; // Auto-snapshot: debounce editor changes and save to local history @@ -282,97 +345,163 @@ export const QueryEditor = memo(function QueryEditor({ lastSchemaHash = ""; // Force cache miss }, [schemaItems]); - // Create a stable fingerprint of statementResults so the effect fires reliably - // even when React batches state updates and the array reference doesn't change. + // Keep isExecuting in a ref so reconcileGlyphs (called from the mount-time + // closure) reads the live value. + const isExecutingRef = useRef(isExecuting); + isExecutingRef.current = isExecuting; + + // Fingerprint that re-triggers reconcile only on meaningful changes. Keyed on + // id so a re-run (new id at the same line) re-fires, and on line number so a + // writeback that refreshes positions re-fires; executionTime is excluded so + // identical reruns don't thrash. const statementResultsFingerprint = useMemo(() => { if (!statementResults || statementResults.length === 0) return ''; - return statementResults.map(r => `${r.lineNumber}:${r.status}:${r.executionTime || 0}`).join(','); + return statementResults.map(r => `${r.id ?? '?'}:${r.status}:${r.lineNumber}`).join(','); }, [statementResults]); - // Effect to update Monaco decorations when statementResults changes (DataGrip-style gutter glyphs) - useEffect(() => { - if (!editorRef.current || !monacoRef.current) return; - + // Reconcile the sticky glyph decorations to match statementResults, keyed by + // id. Surviving glyphs are left exactly where Monaco has tracked them to (so + // they keep following edits); only newly-added ids get a decoration created, + // and removed ids get theirs cleared. This never re-pins a surviving glyph to + // a stale line number — that was the core "checkmark on the wrong block" bug. + const reconcileGlyphs = useCallback(() => { const editor = editorRef.current; const monaco = monacoRef.current; - - // Always clear existing decorations first - if (decorationsRef.current.length > 0) { - decorationsRef.current = editor.deltaDecorations(decorationsRef.current, []); + if (!editor || !monaco) return; + const model = editor.getModel(); + if (!model) return; + // Mid-execution we leave whatever is on screen; results land after it ends. + if (isExecutingRef.current) return; + + const results = statementResultsRef.current ?? []; + const known = glyphDecoRef.current; + const presentIds = new Set(results.map(r => r.id).filter(Boolean) as string[]); + + // Remove decorations whose result is gone (e.g. a re-run replaced the entry). + for (const [id, decoId] of [...known]) { + if (!presentIds.has(id)) { + model.deltaDecorations([decoId], []); + known.delete(id); + glyphAnchorRef.current.delete(id); + } } - - // During execution, show no glyph — wait for results - if (isExecuting) return; - - // Only create gutter glyphs when statementResults has data (after execution completes) - if (!statementResults || statementResults.length === 0) { - return; + + // Create decorations for results that don't have one yet (fresh runs + + // remount seeding). Surviving ids are skipped so Monaco keeps their live + // positions instead of being yanked back to a stored line number. + const lineCount = model.getLineCount(); + const newErrorLines: number[] = []; + for (const res of results) { + if (!res.id || known.has(res.id)) continue; + const line = Math.min(Math.max(res.lineNumber, 1), lineCount); + const [decoId] = model.deltaDecorations([], [buildGlyphDecoration(monaco, res, line)]); + known.set(res.id, decoId); + glyphAnchorRef.current.set(res.id, (model.getLineContent(line) || '').trim()); + if (res.status === 'error') newErrorLines.push(line); } - - // Create decorations for all statement results (DataGrip-style) - const decorations: any[] = []; - - statementResults.forEach((result) => { - const { lineNumber, status, rowCount, rowsAffected, error, executionTime } = result; - - let glyphClassName = 'statement-glyph-running'; - let hoverMessage = 'Query running...'; - let tooltip = ''; - - if (status === 'success') { - glyphClassName = 'statement-glyph-success'; - hoverMessage = 'Query succeeded'; - if (rowCount !== undefined) { - tooltip = `${rowCount} row${rowCount !== 1 ? 's' : ''} retrieved`; - } else if (rowsAffected !== undefined) { - tooltip = `${rowsAffected} row${rowsAffected !== 1 ? 's' : ''} affected`; - } - if (executionTime !== undefined && executionTime > 0) { - tooltip += tooltip ? ` in ${executionTime}ms` : `${executionTime}ms`; - } - } else if (status === 'error') { - glyphClassName = 'statement-glyph-error'; - hoverMessage = 'Query failed'; - tooltip = error || 'Error executing query'; - } - - // Add gutter glyph decoration - decorations.push({ - range: new monaco.Range(lineNumber, 1, lineNumber, 1), - options: { - isWholeLine: false, - glyphMarginClassName: glyphClassName, - glyphMarginHoverMessage: { value: tooltip || hoverMessage }, - } - }); - }); - - // Apply all decorations - if (decorations.length > 0) { - decorationsRef.current = editor.deltaDecorations([], decorations); + + // Reveal a freshly-failed statement — but only for newly-created error + // glyphs, so an edit-driven writeback never yanks the viewport around. + if (newErrorLines.length > 0) { + editor.revealLineInCenter(newErrorLines[newErrorLines.length - 1]); } - - // Scroll the last statement into view if there are errors - const hasErrors = statementResults.some(r => r.status === 'error'); - if (hasErrors) { - const lastError = [...statementResults].reverse().find(r => r.status === 'error'); - if (lastError) { - editor.revealLineInCenter(lastError.lineNumber); + }, []); + + // Prune/refresh glyphs as the text changes: drop a glyph whose anchor line no + // longer reads as the block that produced it (edited or cut away) and keep the + // survivors' line numbers fresh. The new set is reported up so tab state — + // which must survive a tab switch / remount — stays in sync. + const pruneGlyphs = useCallback(() => { + const editor = editorRef.current; + if (!editor) return; + const model = editor.getModel(); + if (!model) return; + + const known = glyphDecoRef.current; + if (known.size === 0) return; + + const removed = new Set(); + const lineById = new Map(); + + for (const [id, decoId] of [...known]) { + const range = model.getDecorationRange(decoId); + if (!range) { + known.delete(id); + glyphAnchorRef.current.delete(id); + removed.add(id); + continue; + } + const line = range.startLineNumber; + const lineText = (model.getLineContent(line) || '').trim(); + const anchor = glyphAnchorRef.current.get(id); + // Destroyed if the anchor line is now blank or its text changed (the block + // was edited or cut). Following in place — inserting/deleting *other* lines + // — leaves this line's text identical, so the glyph survives and shifts. + if (!lineText || (anchor !== undefined && lineText !== anchor)) { + model.deltaDecorations([decoId], []); + known.delete(id); + glyphAnchorRef.current.delete(id); + removed.add(id); + } else { + lineById.set(id, line); } } - - }, [statementResultsFingerprint, isExecuting]); + + if (removed.size === 0 && lineById.size === 0) return; + + const current = statementResultsRef.current ?? []; + let changed = removed.size > 0; + const next = current + .filter(r => !(r.id && removed.has(r.id))) + .map(r => { + const newLine = r.id ? lineById.get(r.id) : undefined; + if (newLine !== undefined && newLine !== r.lineNumber) { + changed = true; + return { ...r, lineNumber: newLine }; + } + return r; + }); + + if (changed) onStatementResultsChangeRef.current?.(next); + }, []); + + // Drive reconcile from React state changes (new runs, writebacks). Mount-time + // seeding is also kicked from handleEditorMount in case onMount fires after + // this effect's first pass. + useEffect(() => { + reconcileGlyphs(); + }, [statementResultsFingerprint, isExecuting, reconcileGlyphs]); const handleEditorMount: OnMount = (editor, monaco) => { monacoRef.current = monaco; editorRef.current = editor; - + + // Seed any persisted run-status glyphs now that the editor exists. The + // statementResults effect may have run its first pass before onMount fired + // (editorRef was still null then), so a tab reopened with prior glyphs would + // otherwise show none until the next run. + reconcileGlyphs(); + + // Prune/refresh the sticky glyphs as the text changes (throttled so large + // files stay responsive). Glyphs follow edits via Monaco's own tracking; + // this only removes ones whose block was destroyed and keeps line numbers + // fresh in tab state. + let glyphPruneThrottle: ReturnType | null = null; + const throttledGlyphPrune = () => { + if (glyphPruneThrottle !== null) return; + glyphPruneThrottle = setTimeout(() => { + glyphPruneThrottle = null; + pruneGlyphs(); + }, 200); + }; + const glyphPruneDisposable = editor.onDidChangeModelContent(() => throttledGlyphPrune()); + // Focus after a short delay to ensure UI is ready setTimeout(() => editor.focus(), 100); const focusHandler = () => editor.focus(); const formatHandler = () => editor.getAction('editor.action.formatDocument')?.run(); - + window.addEventListener("focus-editor", focusHandler); window.addEventListener("format-sql", formatHandler); @@ -667,12 +796,14 @@ export const QueryEditor = memo(function QueryEditor({ contentChangeDisposable?.dispose(); cursorMoveDisposable?.dispose(); cursorContentDisposable?.dispose(); + glyphPruneDisposable?.dispose(); window.removeEventListener("focus-editor", focusHandler); window.removeEventListener("format-sql", formatHandler); window.removeEventListener("run-query-smart", handleRunSmart); window.removeEventListener("run-query-all", handleRunAll); if (domNode) domNode.removeEventListener("contextmenu", handleContextMenu); if (varDecoThrottle !== null) clearTimeout(varDecoThrottle); + if (glyphPruneThrottle !== null) clearTimeout(glyphPruneThrottle); }); // NOTE: Ctrl+Shift+F is intentionally NOT bound to formatDocument here. diff --git a/src/utils/statementGlyphs.test.ts b/src/utils/statementGlyphs.test.ts new file mode 100644 index 0000000..abb869e --- /dev/null +++ b/src/utils/statementGlyphs.test.ts @@ -0,0 +1,65 @@ +import { describe, it, expect } from "vitest"; +import { + mapSelectionStatementsToDocumentLines, + mergeGlyphResults, +} from "./statementGlyphs"; + +describe("mapSelectionStatementsToDocumentLines", () => { + it("maps a single-statement selection to its document line", () => { + // Selection that starts on document line 6. + const out = mapSelectionStatementsToDocumentLines("SELECT 1", 6); + expect(out).toEqual([{ text: "SELECT 1", lineNumber: 6 }]); + }); + + it("offsets every statement's line by the selection's start line", () => { + // Two statements; relative lines 1 and 3 inside the selection. + const selection = "SELECT 1;\n\nSELECT 2"; + const out = mapSelectionStatementsToDocumentLines(selection, 6); + // The regression: previously these came back as 1 and 3 (relative to the + // selection) so the glyphs landed on document lines 1 and 3 instead of 6/8. + expect(out).toEqual([ + { text: "SELECT 1", lineNumber: 6 }, + { text: "SELECT 2", lineNumber: 8 }, + ]); + }); + + it("treats a selection starting on line 1 as document-absolute already", () => { + const out = mapSelectionStatementsToDocumentLines("SELECT 1;\nSELECT 2", 1); + expect(out).toEqual([ + { text: "SELECT 1", lineNumber: 1 }, + { text: "SELECT 2", lineNumber: 2 }, + ]); + }); + + it("returns an empty list for whitespace-only selections", () => { + expect(mapSelectionStatementsToDocumentLines(" \n ", 4)).toEqual([]); + }); +}); + +describe("mergeGlyphResults", () => { + it("appends a glyph for a newly-run block, keeping the others", () => { + const existing = [{ lineNumber: 1, status: "success" }]; + const incoming = [{ lineNumber: 6, status: "success" }]; + expect(mergeGlyphResults(existing, incoming)).toEqual([ + { lineNumber: 1, status: "success" }, + { lineNumber: 6, status: "success" }, + ]); + }); + + it("replaces the glyph on a re-run of the same line", () => { + const existing = [ + { lineNumber: 1, status: "success", executionTime: 5 }, + { lineNumber: 6, status: "error", executionTime: 9 }, + ]; + const incoming = [{ lineNumber: 6, status: "success", executionTime: 2 }]; + expect(mergeGlyphResults(existing, incoming)).toEqual([ + { lineNumber: 1, status: "success", executionTime: 5 }, + { lineNumber: 6, status: "success", executionTime: 2 }, + ]); + }); + + it("returns existing unchanged when there's nothing incoming", () => { + const existing = [{ lineNumber: 1, status: "success" }]; + expect(mergeGlyphResults(existing, [])).toBe(existing); + }); +}); diff --git a/src/utils/statementGlyphs.ts b/src/utils/statementGlyphs.ts new file mode 100644 index 0000000..4fed613 --- /dev/null +++ b/src/utils/statementGlyphs.ts @@ -0,0 +1,60 @@ +/** + * Pure helpers for the "run-status gutter glyphs" feature (the green/red + * checkmarks shown next to each SQL block that has been run). + * + * The hard parts of that feature — placing a glyph on the right line and + * accumulating one glyph per block across runs — used to be done with + * frozen, run-time line numbers and got out of sync the moment the editor + * text changed or a multi-statement *selection* was run (the substring's + * line numbers were 1-based from the selection, not the document). These + * helpers isolate the two bits of logic that are easy to get wrong so they + * can be unit-tested without a live Monaco editor. + */ +import { splitStatements } from "./splitStatements"; + +export interface DocumentStatement { + /** Trimmed statement text, without the terminating semicolon. */ + text: string; + /** 1-based line number in the *document* where the statement begins. */ + lineNumber: number; +} + +/** + * Split a selection's text into top-level statements and map each one back to + * its document-absolute 1-based line number. + * + * Why this exists: when the user runs a selection (or a multi-statement block), + * the only text available downstream is the selected substring. Splitting that + * substring gives line numbers relative to the *selection* (statement 2 might + * report line 4 even though it sits on document line 14). The glyph then lands + * on the wrong block. `selectionStartLine` is the document line where char 0 of + * `selectionText` lives, so the document line is simply + * `selectionStartLine + (relativeLine - 1)`. + */ +export function mapSelectionStatementsToDocumentLines( + selectionText: string, + selectionStartLine: number, +): DocumentStatement[] { + return splitStatements(selectionText).map((s) => ({ + text: s.text, + lineNumber: selectionStartLine + (s.lineNumber - 1), + })); +} + +/** + * Accumulate run-status glyphs across runs. + * + * Each new run replaces any existing glyph that sits on the same line (it's a + * re-run of that block) and appends the rest, so a tab keeps exactly one glyph + * per distinct block that has been run rather than wiping the others every time + * a single block is executed. + */ +export function mergeGlyphResults( + existing: T[], + incoming: T[], +): T[] { + if (incoming.length === 0) return existing; + const incomingLines = new Set(incoming.map((r) => r.lineNumber)); + const kept = existing.filter((r) => !incomingLines.has(r.lineNumber)); + return [...kept, ...incoming]; +} From 6b737d8315ab2ed2e33ff33ed7323329859cfaf6 Mon Sep 17 00:00:00 2001 From: NicolaasGrobler Date: Tue, 9 Jun 2026 13:28:21 +0800 Subject: [PATCH 3/6] fix: show + enable toolbar, results, and save for tab-target connections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/components/layout/MainContent.tsx | 61 ++++++++++++++++++--------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/src/components/layout/MainContent.tsx b/src/components/layout/MainContent.tsx index 0f3f04c..4795436 100644 --- a/src/components/layout/MainContent.tsx +++ b/src/components/layout/MainContent.tsx @@ -123,10 +123,6 @@ export function MainContent() { const { connections, folders, activeConnection, selectedDatabase, currentDb, vaultCredentials, databases: globalDatabases, connectToDatabase, initialLoadDone } = useConnections(); const { addQuery } = useQueryHistory(); const settings = useSettings(); - // Gates the query toolbar, tab strip, and results panel. Until a database - // is picked, the main pane shows EmptyStateLauncher instead of disabled - // chrome. See #84. - const isDatabaseReady = !!activeConnection && !!selectedDatabase; const [showServices, setShowServices] = useState(true); const [queryTabs, setQueryTabs] = useState([]); const [activeTabId, setActiveTabId] = useState(null); @@ -297,6 +293,22 @@ export function MainContent() { }, [connections, vaultCredentials, tabDatabases]); const activeTab = queryTabs.find((t) => t.id === activeTabId); + // Resolve the connection the active tab will actually use: its explicit + // `target` (explorer-opened / session-restored tabs) overrides the + // sidebar-selected context connection. Mirrors executeQuery's resolution so + // toolbar/results visibility and the Run/Save buttons agree with what a run + // would actually do. + const activeTabConnection = activeTab?.target + ? connections.find((c) => c.id === activeTab.target!.connectionId) ?? null + : activeConnection; + const activeTabDatabase = activeTab?.target?.database ?? selectedDatabase; + // The toolbar and results panel show whenever the active tab resolves to a + // real connection + database — not only when a DB is picked in the sidebar. + // Gating purely on the sidebar selection hid both surfaces for target-scoped + // tabs even though the query executes fine via the target, so the Cancel + // button and the results bar never appeared. + const activeTabRunnable = + !!activeTab && !!activeTabConnection && !!activeTabDatabase; const prevActiveTabId = useRef(null); // Keep refs in sync with latest values to avoid stale closures @@ -666,10 +678,16 @@ export function MainContent() { return; } - if (!activeConnection) { + // Resolve the tab's connection (target overrides the sidebar selection) + // so Ctrl+S saves from a target-scoped tab instead of erroring. + const saveConn = activeTab?.target + ? connections.find((c) => c.id === activeTab.target!.connectionId) ?? null + : activeConnection; + if (!saveConn) { setError("No connection — connect to a database before saving queries."); return; } + const saveDb = activeTab?.target?.database ?? selectedDatabase; const queryToSave = activeTab?.query || currentQueryRef.current; if (!queryToSave || queryToSave.trim() === "") { setError("Query is empty — type a SQL statement before saving."); @@ -694,14 +712,14 @@ export function MainContent() { addSavedQuery({ name, query: queryToSave, - database: selectedDatabase || "", - connectionId: activeConnection.id + database: saveDb || "", + connectionId: saveConn.id }); } useLocalHistory.getState().addEntry( `saved-queries/${name}`, queryToSave, - `Saved: ${name} — ${activeConnection.name}` + `Saved: ${name} — ${saveConn.name}` ); // Mark the active tab as in-sync with persisted state so the // unsaved-changes prompt on app exit (#121) won't fire for it. @@ -725,7 +743,7 @@ export function MainContent() { window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); - }, [activeConnection, selectedDatabase, activeTab, addSavedQuery, confirmDialog]); + }, [activeConnection, selectedDatabase, activeTab, connections, addSavedQuery, confirmDialog]); const updateTabState = useCallback((tabId: string, updates: Partial) => { setQueryTabs(prev => prev.map(t => t.id === tabId ? { ...t, ...updates } : t)); @@ -3191,10 +3209,11 @@ const executeQuery = useCallback(async (specificQuery?: any, statementInfo?: { l {activeTab?.name || "No Active Tab"}
- {/* Combined Tool Window Bar - Top — only when a database is selected. - Without a target DB, every action (Run, Format, Explain, Compare, - Clone, Activity, AI, Save) is either disabled or pointless. See #84. */} - {isDatabaseReady && ( + {/* Combined Tool Window Bar - Top — shown whenever the active tab can run + (sidebar-selected DB OR the tab's own target connection). Previously + gated on the sidebar selection alone, which hid Run/Cancel for + target-scoped tabs even though they execute fine. See #84. */} + {activeTabRunnable && (
{isExecuting ? (