From 5653b477fae47b878b39a2fd23ba505ef6641e96 Mon Sep 17 00:00:00 2001 From: emrberk Date: Thu, 27 Aug 2026 17:26:02 +0300 Subject: [PATCH 01/12] feat: add full query text support to run with selection option --- e2e/commands.js | 30 +++ e2e/questdb | 2 +- e2e/tests/console/editor.spec.js | 198 +++++++++++++++--- src/components/EditorSettingsModal/index.tsx | 65 ++++-- src/providers/LocalStorageProvider/index.tsx | 30 +-- src/providers/LocalStorageProvider/types.ts | 4 +- .../LocalStorageProvider/utils.test.ts | 69 ++---- src/providers/LocalStorageProvider/utils.ts | 10 + src/scenes/Editor/Monaco/index.tsx | 39 ++-- src/scenes/Editor/Monaco/utils.test.ts | 76 +++++-- src/scenes/Editor/Monaco/utils.ts | 58 +++-- .../Notebook/cells/useCellRunActions.ts | 25 ++- .../Editor/Notebook/result-table/TabBar.tsx | 15 +- 13 files changed, 454 insertions(+), 167 deletions(-) diff --git a/e2e/commands.js b/e2e/commands.js index b02401e76..f51ab8e31 100644 --- a/e2e/commands.js +++ b/e2e/commands.js @@ -443,6 +443,36 @@ Cypress.Commands.add("selectRange", (startPos, endPos) => { }) }) +Cypress.Commands.add("createNotebook", () => { + cy.get(".chrome-tabs .new-tab-button").click() + cy.getByDataHook("new-tab-notebook").click() + cy.getByDataHook("notebook-toolbar").should("be.visible") + cy.getByDataHook("cell-editor-shimmer").should("not.exist") + cy.getByDataHook("cell-grid-shimmer").should("not.exist") + cy.get("[data-notebook-cell] .monaco-editor textarea").should("exist") +}) + +Cypress.Commands.add("focusNotebookCell", () => { + cy.get("[data-notebook-cell] .monaco-editor .view-lines").first().click() + cy.focused().should("have.class", "inputarea") +}) + +Cypress.Commands.add("withFocusedEditor", (fn) => { + cy.window().should((win) => { + const hasFocusedEditor = win.monaco.editor + .getEditors() + .some((candidate) => candidate.hasTextFocus()) + expect(hasFocusedEditor, "a Monaco editor has focus").to.eq(true) + }) + cy.window().then((win) => + fn( + win.monaco.editor + .getEditors() + .find((candidate) => candidate.hasTextFocus()), + ), + ) +}) + Cypress.Commands.add("getVisibleLines", () => cy.get(".view-lines")) Cypress.Commands.add("expandNotifications", () => diff --git a/e2e/questdb b/e2e/questdb index 9b59a9211..2d9244fec 160000 --- a/e2e/questdb +++ b/e2e/questdb @@ -1 +1 @@ -Subproject commit 9b59a921165af573cedd22bf8b12613de19cb8bd +Subproject commit 2d9244fec3e5a17e3de1b7319d090a6b1cd94f4a diff --git a/e2e/tests/console/editor.spec.js b/e2e/tests/console/editor.spec.js index 114d3f92d..ceffa8034 100644 --- a/e2e/tests/console/editor.spec.js +++ b/e2e/tests/console/editor.spec.js @@ -1005,7 +1005,8 @@ describe("&query URL param", () => { .then((clip) => clip.readText()) .should("eq", expectedUrl("SELECT 1;")) - // When — glyph dropdown copies a selection inside a single query + // When — glyph dropdown copies the complete query even with a + // fragment selected inside it cy.selectRange({ lineNumber: 2, column: 1 }, { lineNumber: 2, column: 7 }) cy.getByDataHook("button-run-query").should("contain", "Run selected query") cy.openRunDropdownInLine(2) @@ -1015,7 +1016,7 @@ describe("&query URL param", () => { cy.window() .its("navigator.clipboard") .then((clip) => clip.readText()) - .should("eq", expectedUrl("SELECT;")) + .should("eq", expectedUrl("SELECT 2;")) // When — Alt+L copies single query at cursor cy.clickLine(3) @@ -2127,17 +2128,17 @@ describe("editor settings", () => { cy.getByDataHook("editor-settings-modal").should("be.visible") } - it("toggles 'Run with selection' from the modal and persists it", () => { - // Given the setting defaults to on + it("changes 'Run with selection' from the modal and persists it", () => { + // Given the setting defaults to partial openEditorSettings() cy.getByDataHook("editor-settings-run-with-selection").should( - "have.attr", - "aria-checked", - "true", + "contain", + "Partial queries", ) - // When it is switched off and saved + // When it is set to off and saved cy.getByDataHook("editor-settings-run-with-selection").click() + cy.getByDataHook("run-with-selection-off").click() cy.getByDataHook("editor-settings-save").click() cy.getByDataHook("editor-settings-modal").should("not.exist") @@ -2145,66 +2146,200 @@ describe("editor settings", () => { cy.window() .its("localStorage") .invoke("getItem", "editor.runWithSelection") - .should("eq", "false") + .should("eq", "off") // And reopening the modal shows it still off openEditorSettings() cy.getByDataHook("editor-settings-run-with-selection").should( - "have.attr", - "aria-checked", - "false", + "contain", + "Off", ) cy.getByDataHook("editor-settings-cancel").click() cy.getByDataHook("editor-settings-modal").should("not.exist") }) - it("runs the selected fragment when enabled and the whole query once disabled", () => { + it("resolves the selection per mode: partial fragment, complete query, ignored when off", () => { const table = runWithSelectionTable const query = `select a from ${table}` const startColumn = query.indexOf(table) + 1 const endColumn = startColumn + table.length + const selectTableName = () => + cy.selectRange( + { lineNumber: 1, column: startColumn }, + { lineNumber: 1, column: endColumn }, + ) + const setMode = (mode) => { + openEditorSettings() + cy.getByDataHook("editor-settings-run-with-selection").click() + cy.getByDataHook(`run-with-selection-${mode}`).click() + cy.getByDataHook("editor-settings-save").click() + } // Given a query selecting one column, with the table name highlighted cy.typeQueryDirectly(query) - cy.selectRange( - { lineNumber: 1, column: startColumn }, - { lineNumber: 1, column: endColumn }, - ) + selectTableName() - // When 'Run with selection' is enabled (default), the selection runs + // When the mode is partial (default), the selection runs as-is cy.getByDataHook("button-run-query").should("contain", "Run selected query") cy.clickRunQuery() // Then the bare table name returns every column cy.get("[data-hook='grid-header-name']").should("have.length", 3) - // When 'Run with selection' is turned off - openEditorSettings() - cy.getByDataHook("editor-settings-run-with-selection").click() - cy.getByDataHook("editor-settings-save").click() + // When the mode is set to complete + setMode("complete") // Then the run button reflects the new setting without any further // editor interaction cy.getByDataHook("button-run-query").should("contain", "Run query") + // And the same fragment expands to the whole query, returning one column + selectTableName() + cy.getByDataHook("button-run-query").should("contain", "Run query") + cy.clickRunQuery() + cy.get("[data-hook='grid-header-name']").should("have.length", 1) + + // When the mode is set to off + setMode("off") + // And the same fragment is selected and run again - cy.selectRange( - { lineNumber: 1, column: startColumn }, - { lineNumber: 1, column: endColumn }, - ) + selectTableName() cy.getByDataHook("button-run-query").should("contain", "Run query") cy.clickRunQuery() // Then the whole cursor query runs, returning only the single column cy.get("[data-hook='grid-header-name']").should("have.length", 1) }) - it("runs the share-link selection even when 'Run with selection' is off", () => { + it("expands a cross-statement selection to both whole queries in complete mode", () => { + // Given the mode is set to complete + openEditorSettings() + cy.getByDataHook("editor-settings-run-with-selection").click() + cy.getByDataHook("run-with-selection-complete").click() + cy.getByDataHook("editor-settings-save").click() + + // And two statements with a selection cutting into both + cy.typeQueryDirectly("select 1; select 2;") + cy.selectRange({ lineNumber: 1, column: 8 }, { lineNumber: 1, column: 14 }) + + // When both expanded queries run + cy.getByDataHook("button-run-query").should( + "contain", + "Run 2 selected queries", + ) + cy.clickRunQuery() + + // Then the second whole query ran last, so its result shows + cy.getGridRows().should("have.length", 1) + cy.getGridRow(0).should("contain", "2") + }) + + it("resolves the notebook cell selection per mode: partial, complete, off", () => { + const table = runWithSelectionTable + const sql = `select a from ${table}; select 33` + const tableStart = sql.indexOf(table) + 1 + const tableEnd = tableStart + table.length + // Reaches into the second statement's "sele", cutting both statements. + const crossEnd = sql.indexOf("; select") + 8 + + const setMode = (mode) => { + openEditorSettings() + cy.getByDataHook("editor-settings-run-with-selection").click() + cy.getByDataHook(`run-with-selection-${mode}`).click() + cy.getByDataHook("editor-settings-save").click() + cy.getByDataHook("editor-settings-modal").should("not.exist") + } + const selectCellRange = (startColumn, endColumn) => + cy.withFocusedEditor((editor) => + editor.setSelection({ + startLineNumber: 1, + startColumn, + endLineNumber: 1, + endColumn, + }), + ) + const runCellAtCursor = () => + cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) + const cellResultTabs = () => cy.get("[role='tablist'] [role='tab']") + + // Given exec responses slow enough to observe the run loaders + cy.intercept("/exec*", (req) => { + req.on("response", (res) => { + res.setDelay(300) + }) + }) + + // And a resolved notebook whose focused cell holds two statements + cy.createNotebook() + cy.focusNotebookCell() + cy.focused().type(sql, { delay: 0 }) + cy.withFocusedEditor((editor) => { + expect(editor.getValue()).to.eq(sql) + }) + + // When the mode is partial (default) and the table name is selected + selectCellRange(tableStart, tableEnd) + runCellAtCursor() + + // Then the table toggle spins while the fragment runs + cy.get("[aria-label='View table'][aria-busy='true']").should("exist") + cy.get("[aria-label='View table'][aria-busy='true']").should("not.exist") + + // And only the bare fragment ran: a single result with every column + cy.get("[data-hook='grid-header-name']:visible").should("have.length", 3) + cellResultTabs().should("not.exist") + + // When the mode is complete and the selection cuts into both statements + setMode("complete") + cy.focusNotebookCell() + selectCellRange(tableStart, crossEnd) + runCellAtCursor() + + // Then both whole statements run to completion, first statement active + cy.get("[role='tablist'] [data-hook='result-tab-success']").should( + "have.length", + 2, + ) + cy.getByDataHook("result-tab-loading").should("not.exist") + cellResultTabs() + .eq(0) + .should("have.attr", "title", `select a from ${table}`) + .and("have.attr", "aria-selected", "true") + cellResultTabs().eq(1).should("have.attr", "title", "select 33") + + // When the mode is off and the same selection is made + setMode("off") + cy.focusNotebookCell() + selectCellRange(tableStart, crossEnd) + runCellAtCursor() + + // Then only the statement at the cursor runs, ignoring the selection + cy.get("[role='tablist'] [data-hook='result-tab-success']").should( + "have.length", + 1, + ) + cy.get("[role='tablist'] [data-hook='result-tab-not-run']").should( + "have.length", + 1, + ) + cy.get("[role='tablist'] [role='tab'][aria-selected='true']").should( + "have.attr", + "title", + "select 33", + ) + cy.get("[data-hook='grid-header-name']:visible") + .should("have.length", 1) + .and("contain", "33") + }) + + it("expands the share-link fragment to the complete query, with the setting off", () => { // Given the setting is turned off openEditorSettings() cy.getByDataHook("editor-settings-run-with-selection").click() + cy.getByDataHook("run-with-selection-off").click() cy.getByDataHook("editor-settings-save").click() - // And a buffer whose statement contains the shared fragment + // And a persisted buffer whose statement contains the shared fragment cy.typeQueryDirectly("select 1 union all select 2;") + cy.wait(1000) // When a share link for the fragment auto-runs cy.visit( @@ -2212,9 +2347,10 @@ describe("editor settings", () => { ) cy.getEditorContent().should("be.visible") - // Then only the selected fragment runs, not the containing statement - cy.getGridRows().should("have.length", 1) - cy.getGridRow(0).should("contain", "2") + // Then the complete containing statement runs, not just the fragment + cy.getGridRows().should("have.length", 2) + cy.getGridRow(0).should("contain", "1") + cy.getGridRow(1).should("contain", "2") }) it("caps column width at the configured maximum and returns to auto", () => { diff --git a/src/components/EditorSettingsModal/index.tsx b/src/components/EditorSettingsModal/index.tsx index 4fb4d7fe7..601214950 100644 --- a/src/components/EditorSettingsModal/index.tsx +++ b/src/components/EditorSettingsModal/index.tsx @@ -5,9 +5,10 @@ import { Overlay } from "../Overlay" import { ForwardRef } from "../ForwardRef" import { Text } from "../Text" import { Button } from "../Button" -import { Switch } from "../Switch" +import { SelectMenuControl } from "../SelectMenu" import { Input } from "../Input" import { useLocalStorage } from "../../providers/LocalStorageProvider" +import type { RunWithSelectionMode } from "../../providers/LocalStorageProvider/types" import { isMaxColumnWidthDraftValid, parseMaxColumnWidth, @@ -77,6 +78,36 @@ const WidthInput = styled(Input)` text-align: right; ` +const ModeSelectField = styled.div` + width: 16rem; +` + +const RUN_WITH_SELECTION_OPTIONS: { + label: string + value: RunWithSelectionMode + description: string + dataHook: string +}[] = [ + { + label: "Partial queries", + value: "partial", + description: "Selecting part of a query runs only that part.", + dataHook: "run-with-selection-partial", + }, + { + label: "Complete queries", + value: "complete", + description: "Selecting part of a query runs the whole query.", + dataHook: "run-with-selection-complete", + }, + { + label: "Off", + value: "off", + description: "The selection is ignored. The query at the cursor runs.", + dataHook: "run-with-selection-off", + }, +] + type SettingRowProps = { label: string description: string @@ -117,9 +148,10 @@ const MAX_COLUMN_WIDTH_ID = "editor-settings-max-column-width" const MAX_COLUMN_WIDTH_ERROR_ID = `${MAX_COLUMN_WIDTH_ID}-error` const EditorSettingsForm = ({ onClose }: { onClose: () => void }) => { - const { runWithSelection, maxColumnWidth, updateSettings } = useLocalStorage() - const [runWithSelectionDraft, setRunWithSelectionDraft] = - useState(runWithSelection) + const { runWithSelectionMode, maxColumnWidth, updateSettings } = + useLocalStorage() + const [runWithSelectionModeDraft, setRunWithSelectionModeDraft] = + useState(runWithSelectionMode) const [maxColumnWidthDraft, setMaxColumnWidthDraft] = useState( maxColumnWidth === "auto" ? "" : String(maxColumnWidth), ) @@ -132,7 +164,7 @@ const EditorSettingsForm = ({ onClose }: { onClose: () => void }) => { widthInputRef.current?.focus() return } - updateSettings(StoreKey.RUN_WITH_SELECTION, runWithSelectionDraft) + updateSettings(StoreKey.RUN_WITH_SELECTION, runWithSelectionModeDraft) updateSettings( StoreKey.MAX_COLUMN_WIDTH, parseMaxColumnWidth(maxColumnWidthDraft), @@ -150,16 +182,23 @@ const EditorSettingsForm = ({ onClose }: { onClose: () => void }) => { - + + + setRunWithSelectionModeDraft(value as RunWithSelectionMode) + } + options={RUN_WITH_SELECTION_OPTIONS} + /> + void @@ -183,7 +189,7 @@ const defaultValues: ContextProps = { autoRefreshTables: true, useNewGrid: true, useQuickVis: false, - runWithSelection: true, + runWithSelectionMode: "partial", maxColumnWidth: "auto", leftPanelState: defaultConfig.leftPanelState, updateLeftPanelState: (_state: LeftPanelState) => undefined, @@ -238,12 +244,10 @@ export const LocalStorageProvider = ({ getInitialBooleanFeature(QUICK_VIS_OVERRIDE), ) - const [runWithSelection, setRunWithSelection] = useState( - parseBoolean( - getValue(StoreKey.RUN_WITH_SELECTION), - defaultConfig.runWithSelection, - ), - ) + const [runWithSelectionMode, setRunWithSelectionMode] = + useState( + parseRunWithSelectionMode(getValue(StoreKey.RUN_WITH_SELECTION)), + ) const [maxColumnWidth, setMaxColumnWidth] = useState( parseMaxColumnWidth(getValue(StoreKey.MAX_COLUMN_WIDTH)), @@ -353,7 +357,7 @@ export const LocalStorageProvider = ({ setUseQuickVis(value === "true") break case StoreKey.RUN_WITH_SELECTION: - setRunWithSelection(value === "true") + setRunWithSelectionMode(parseRunWithSelectionMode(value)) break case StoreKey.MAX_COLUMN_WIDTH: setMaxColumnWidth(parseMaxColumnWidth(value)) @@ -388,7 +392,7 @@ export const LocalStorageProvider = ({ autoRefreshTables, useNewGrid, useQuickVis, - runWithSelection, + runWithSelectionMode, maxColumnWidth, leftPanelState, updateLeftPanelState, @@ -408,7 +412,7 @@ export const LocalStorageProvider = ({ autoRefreshTables, useNewGrid, useQuickVis, - runWithSelection, + runWithSelectionMode, maxColumnWidth, leftPanelState, updateLeftPanelState, diff --git a/src/providers/LocalStorageProvider/types.ts b/src/providers/LocalStorageProvider/types.ts index 7135eafe7..aad36d921 100644 --- a/src/providers/LocalStorageProvider/types.ts +++ b/src/providers/LocalStorageProvider/types.ts @@ -29,6 +29,8 @@ export type AiAssistantSettings = { export type SettingsType = string | boolean | number | AiAssistantSettings +export type RunWithSelectionMode = "partial" | "complete" | "off" + export enum LeftPanelType { DATASOURCES = "datasources", SEARCH = "search", @@ -55,7 +57,7 @@ export type LocalConfig = { autoRefreshTables: boolean useNewGrid: boolean useQuickVis: boolean - runWithSelection: boolean + runWithSelectionMode: RunWithSelectionMode maxColumnWidth: MaxColumnWidth leftPanelState: LeftPanelState aiAssistantSettings: AiAssistantSettings diff --git a/src/providers/LocalStorageProvider/utils.test.ts b/src/providers/LocalStorageProvider/utils.test.ts index c32141145..3f777cd71 100644 --- a/src/providers/LocalStorageProvider/utils.test.ts +++ b/src/providers/LocalStorageProvider/utils.test.ts @@ -1,55 +1,20 @@ import { describe, it, expect } from "vitest" -import { isMaxColumnWidthDraftValid, parseMaxColumnWidth } from "./utils" - -describe("parseMaxColumnWidth", () => { - it("parses a stored number", () => { - expect(parseMaxColumnWidth("550")).toBe(550) - }) - - it("falls back to auto for a missing value", () => { - expect(parseMaxColumnWidth("")).toBe("auto") - }) - - it("falls back to auto for the stored auto keyword", () => { - expect(parseMaxColumnWidth("auto")).toBe("auto") - }) - - it("falls back to auto for garbage", () => { - expect(parseMaxColumnWidth("wide")).toBe("auto") - }) - - it("clamps values below the minimum", () => { - expect(parseMaxColumnWidth("10")).toBe(60) - }) - - it("clamps values above the maximum", () => { - expect(parseMaxColumnWidth("99999")).toBe(4000) - }) -}) - -describe("isMaxColumnWidthDraftValid", () => { - it("accepts an empty draft as auto", () => { - expect(isMaxColumnWidthDraftValid("")).toBe(true) - }) - - it("accepts a whole number within the bounds", () => { - expect(isMaxColumnWidthDraftValid("250")).toBe(true) - }) - - it("rejects numbers outside the bounds", () => { - expect(isMaxColumnWidthDraftValid("10")).toBe(false) - expect(isMaxColumnWidthDraftValid("99999")).toBe(false) - }) - - it("rejects locale-formatted and decimal numbers", () => { - expect(isMaxColumnWidthDraftValid("1,500")).toBe(false) - expect(isMaxColumnWidthDraftValid("1.500")).toBe(false) - expect(isMaxColumnWidthDraftValid("250.5")).toBe(false) - }) - - it("rejects non-numeric input", () => { - expect(isMaxColumnWidthDraftValid("wide")).toBe(false) - expect(isMaxColumnWidthDraftValid("-250")).toBe(false) - expect(isMaxColumnWidthDraftValid("1e3")).toBe(false) +import { parseRunWithSelectionMode } from "./utils" + +describe("parseRunWithSelectionMode", () => { + it("migrates the legacy boolean values", () => { + // Given values stored by the old on/off switch + // When parsing them + // Then true maps to partial and false maps to off + expect(parseRunWithSelectionMode("true")).toBe("partial") + expect(parseRunWithSelectionMode("false")).toBe("off") + }) + + it("falls back to partial for missing or unknown values", () => { + // Given no stored value or a corrupted one + // When parsing them + // Then the default partial mode applies + expect(parseRunWithSelectionMode("")).toBe("partial") + expect(parseRunWithSelectionMode("garbage")).toBe("partial") }) }) diff --git a/src/providers/LocalStorageProvider/utils.ts b/src/providers/LocalStorageProvider/utils.ts index 1793d18c5..2184d5a52 100644 --- a/src/providers/LocalStorageProvider/utils.ts +++ b/src/providers/LocalStorageProvider/utils.ts @@ -24,10 +24,20 @@ import { MAX_COLUMN_WIDTH_BOUNDS } from "../../components/ResultGrid/dimensions" import type { MaxColumnWidth } from "../../components/ResultGrid/types" +import type { RunWithSelectionMode } from "./types" export const parseBoolean = (value: string, defaultValue: boolean): boolean => value ? value === "true" : defaultValue +export const parseRunWithSelectionMode = ( + value: string, +): RunWithSelectionMode => { + if (value === "partial" || value === "complete" || value === "off") + return value + if (value === "false") return "off" + return "partial" +} + export const parseInteger = (value: string, defaultValue: number): number => isNaN(parseInt(value)) ? defaultValue : parseInt(value) diff --git a/src/scenes/Editor/Monaco/index.tsx b/src/scenes/Editor/Monaco/index.tsx index bf4d47418..08031c97c 100644 --- a/src/scenes/Editor/Monaco/index.tsx +++ b/src/scenes/Editor/Monaco/index.tsx @@ -31,6 +31,7 @@ import { } from "../../../providers/AIStatusProvider" import { useAIConversationActions } from "../../../providers/AIConversationProvider" import { useLocalStorage } from "../../../providers/LocalStorageProvider" +import type { RunWithSelectionMode } from "../../../providers/LocalStorageProvider/types" import { actions, selectors } from "../../../store" import { RunningType } from "../../../store/Query/types" import { MAX_CELL_LINES } from "../../../store/notebook" @@ -282,7 +283,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { } = editorContext const { quest, questExecution } = useContext(QuestContext) const { canUse: canUseAI, status: aiStatus } = useAIStatus() - const { runWithSelection } = useLocalStorage() + const { runWithSelectionMode } = useLocalStorage() const { handleGlyphClick, hasConversationForQuery, @@ -328,7 +329,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const queryNotificationsRef = useRef(queryNotifications) const activeNotificationRef = useRef(activeNotification) const canUseAIRef = useRef(canUseAI) - const runWithSelectionRef = useRef(runWithSelection) + const runWithSelectionModeRef = useRef(runWithSelectionMode) const shareLinkSelectionRunRef = useRef(false) const hasConversationForQueryRef = useRef(hasConversationForQuery) const shiftQueryKeysForBufferRef = useRef(shiftQueryKeysForBuffer) @@ -522,13 +523,14 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { runQueryAction(query, RunningType.EXPLAIN) } + // Share links always carry complete queries, never selection fragments. const buildAndCopyShareLink = (requests: Request[]) => { if (requests.length === 0) { toast.error("Nothing to copy") return } const sql = requests - .map((r) => (r.selection ? r.selection.queryText : r.query)) + .map((r) => r.query) .join(";\n\n") .concat(";") @@ -553,12 +555,12 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const syncQueriesToRun = ( editor: editor.IStandaloneCodeEditor, - runWithSelection: boolean, + selectionMode: RunWithSelectionMode, ): Request[] => { const queriesToRun = getQueriesToRun( editor, queryOffsetsRef.current ?? [], - runWithSelection, + selectionMode, ) queriesToRunRef.current = queriesToRun dispatch(actions.query.setQueriesToRun(queriesToRun)) @@ -571,10 +573,10 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { }) const editor = editorRef.current if (!editor) return - // Link sharing always honors the selection, independent of the - // run-with-selection setting. + // Link sharing always expands the selection to complete queries, + // independent of the run-with-selection setting. buildAndCopyShareLink( - getQueriesToRun(editor, queryOffsetsRef.current ?? [], true), + getQueriesToRun(editor, queryOffsetsRef.current ?? [], "complete"), ) } @@ -992,7 +994,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { } cursorChangeTimeoutRef.current = window.setTimeout(() => { - syncQueriesToRun(editor, runWithSelectionRef.current) + syncQueriesToRun(editor, runWithSelectionModeRef.current) if (monacoRef.current && editorRef.current) { applyLineMarkings(monaco, editor, e.source) @@ -1166,7 +1168,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { applyGlyphsAndLineMarkings(monaco, editor) } - syncQueriesToRun(editor, runWithSelectionRef.current) + syncQueriesToRun(editor, runWithSelectionModeRef.current) contentJustChangedRef.current = false if (notificationKeyUpdates.size > 0) { @@ -1299,12 +1301,13 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { // Initial decoration setup applyGlyphsAndLineMarkings(monaco, editor) - // A ?query link selects its statements to run them all; that selection - // must be honored even when the run-with-selection setting is off. + // A ?query link selects its statements to run them all; the selection + // always expands to complete queries, even when the run-with-selection + // setting is off. const runsShareLinkSelection = Boolean(query && executeQuery) const queriesToRun = syncQueriesToRun( editor, - runsShareLinkSelection || runWithSelectionRef.current, + runsShareLinkSelection ? "complete" : runWithSelectionModeRef.current, ) if (!query || !executeQuery) { @@ -1771,11 +1774,11 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { } useEffect(() => { - runWithSelectionRef.current = runWithSelection + runWithSelectionModeRef.current = runWithSelectionMode const editor = editorRef.current if (!editor) return - syncQueriesToRun(editor, runWithSelection) - }, [runWithSelection]) + syncQueriesToRun(editor, runWithSelectionMode) + }, [runWithSelectionMode]) useEffect(() => { canUseAIRef.current = canUseAI @@ -1858,7 +1861,9 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { ? getQueryRequestFromLastExecutedQuery(lastExecutedQuery) : getQueryRequestFromEditor( editor, - honorShareLinkSelection || runWithSelectionRef.current, + honorShareLinkSelection + ? "complete" + : runWithSelectionModeRef.current, ) const isRunningExplain = running === RunningType.EXPLAIN diff --git a/src/scenes/Editor/Monaco/utils.test.ts b/src/scenes/Editor/Monaco/utils.test.ts index 97ebbaa08..fa60f2195 100644 --- a/src/scenes/Editor/Monaco/utils.test.ts +++ b/src/scenes/Editor/Monaco/utils.test.ts @@ -379,34 +379,68 @@ describe("isCursorInQuotedIdentifier", () => { }) }) -describe("run with selection gating", () => { +describe("run with selection modes", () => { // "SELECT 11" spans columns 1..10 on the single line; the cursor sits inside it. const TEXT = "SELECT 11; SELECT 22" const FIRST_STATEMENT_SELECTION = { startColumn: 1, endColumn: 10 } + const PARTIAL_FIRST_STATEMENT_SELECTION = { startColumn: 4, endColumn: 10 } + // "11; SELE" — cuts into both statements without covering either fully. + const CROSS_STATEMENT_SELECTION = { startColumn: 8, endColumn: 16 } const QUERY_OFFSETS = [ { startOffset: 0, endOffset: 9 }, { startOffset: 11, endOffset: 20 }, ] describe("getQueriesToRun", () => { - it("runs only the selected text when enabled", () => { - // Given the first statement is selected and running with selection is enabled + it("runs only the selected text in partial mode", () => { + // Given the first statement is selected in partial mode const editor = makeSingleLineEditor(TEXT, 3, FIRST_STATEMENT_SELECTION) // When resolving the queries to run - const result = getQueriesToRun(editor, QUERY_OFFSETS, true) + const result = getQueriesToRun(editor, QUERY_OFFSETS, "partial") // Then the run carries the selection expect(result.length).toBeGreaterThan(0) expect(result.some((request) => request.selection)).toBe(true) }) - it("ignores the selection and runs the cursor query when disabled", () => { - // Given the first statement is selected but running with selection is disabled + it("expands a partial cross-statement selection to both whole queries in complete mode", () => { + // Given a selection cutting into both statements in complete mode + const editor = makeSingleLineEditor(TEXT, 3, CROSS_STATEMENT_SELECTION) + + // When resolving the queries to run + const result = getQueriesToRun(editor, QUERY_OFFSETS, "complete") + + // Then both statements run fully with no selection attached + expect(result.map((request) => request.query)).toEqual([ + "SELECT 11", + "SELECT 22", + ]) + expect(result.every((request) => !request.selection)).toBe(true) + }) + + it("expands a partial single-statement selection to its whole query in complete mode", () => { + // Given a selection covering part of the first statement in complete mode + const editor = makeSingleLineEditor( + TEXT, + 3, + PARTIAL_FIRST_STATEMENT_SELECTION, + ) + + // When resolving the queries to run + const result = getQueriesToRun(editor, QUERY_OFFSETS, "complete") + + // Then only that statement runs fully with no selection attached + expect(result.map((request) => request.query)).toEqual(["SELECT 11"]) + expect(result.every((request) => !request.selection)).toBe(true) + }) + + it("ignores the selection and runs the cursor query when off", () => { + // Given the first statement is selected but the mode is off const editor = makeSingleLineEditor(TEXT, 3, FIRST_STATEMENT_SELECTION) // When resolving the queries to run - const result = getQueriesToRun(editor, QUERY_OFFSETS, false) + const result = getQueriesToRun(editor, QUERY_OFFSETS, "off") // Then it falls back to the cursor query with no selection attached expect(result).toEqual([getQueryFromCursor(editor)]) @@ -415,23 +449,39 @@ describe("run with selection gating", () => { }) describe("getQueryRequestFromEditor", () => { - it("builds a selection request when enabled", () => { - // Given the first statement is selected and running with selection is enabled + it("builds a selection request in partial mode", () => { + // Given the first statement is selected in partial mode const editor = makeSingleLineEditor(TEXT, 3, FIRST_STATEMENT_SELECTION) // When building the request from the editor - const request = getQueryRequestFromEditor(editor, true) + const request = getQueryRequestFromEditor(editor, "partial") // Then the request carries the selection expect(request?.selection).toBeDefined() }) - it("ignores the selection and uses the cursor query when disabled", () => { - // Given the first statement is selected but running with selection is disabled + it("builds a whole-query request from a partial selection in complete mode", () => { + // Given a selection covering part of the first statement in complete mode + const editor = makeSingleLineEditor( + TEXT, + 3, + PARTIAL_FIRST_STATEMENT_SELECTION, + ) + + // When building the request from the editor + const request = getQueryRequestFromEditor(editor, "complete") + + // Then the request holds the full statement with no selection + expect(request?.query).toBe("SELECT 11") + expect(request?.selection).toBeUndefined() + }) + + it("ignores the selection and uses the cursor query when off", () => { + // Given the first statement is selected but the mode is off const editor = makeSingleLineEditor(TEXT, 3, FIRST_STATEMENT_SELECTION) // When building the request from the editor - const request = getQueryRequestFromEditor(editor, false) + const request = getQueryRequestFromEditor(editor, "off") // Then the request has no selection expect(request?.selection).toBeUndefined() diff --git a/src/scenes/Editor/Monaco/utils.ts b/src/scenes/Editor/Monaco/utils.ts index 5a5ca1dea..5ab757841 100644 --- a/src/scenes/Editor/Monaco/utils.ts +++ b/src/scenes/Editor/Monaco/utils.ts @@ -26,6 +26,7 @@ import type { Monaco } from "@monaco-editor/react" import type { ErrorResult } from "../../../utils" import { hashString } from "../../../utils" import type { ValidateQueryResult } from "../../../utils/questdb" +import type { RunWithSelectionMode } from "../../../providers/LocalStorageProvider/types" type IStandaloneCodeEditor = editor.IStandaloneCodeEditor @@ -138,14 +139,14 @@ export const getSelectedText = ( export const getQueriesToRun = ( editor: IStandaloneCodeEditor, queryOffsets: { startOffset: number; endOffset: number }[], - runWithSelection: boolean, + selectionMode: RunWithSelectionMode, ): Request[] => { const model = editor.getModel() if (!model) return [] const selection = editor.getSelection() const selectedText = selection ? model.getValueInRange(selection) : undefined - if (!runWithSelection || !selection || !selectedText) { + if (selectionMode === "off" || !selection || !selectedText) { const queryInCursor = getQueryFromCursor(editor) if (queryInCursor) { return [queryInCursor] @@ -190,13 +191,20 @@ export const getQueriesToRun = ( }), }) const clampedSelectionText = model.getValueInRange(clampedSelection) - return stripSQLComments(normalizeQueryText(clampedSelectionText)) - ? { - query: query.query, - row: query.row, - column: query.column, - endRow: query.endRow, - endColumn: query.endColumn, + if (!stripSQLComments(normalizeQueryText(clampedSelectionText))) { + return undefined + } + const fullQueryRequest = { + query: query.query, + row: query.row, + column: query.column, + endRow: query.endRow, + endColumn: query.endColumn, + } + return selectionMode === "complete" + ? fullQueryRequest + : { + ...fullQueryRequest, selection: { startOffset: model.getOffsetAt({ lineNumber: clampedSelection.startLineNumber, @@ -209,7 +217,6 @@ export const getQueriesToRun = ( queryText: clampedSelectionText, }, } - : undefined }) return requests.filter(Boolean) as Request[] } @@ -649,6 +656,24 @@ export const getQueriesInRange = ( return [...stackQueries, ...(nextSqlQuery ? [nextSqlQuery] : [])] } +export const getStatementOffsets = ( + editor: IStandaloneCodeEditor, +): { startOffset: number; endOffset: number }[] => { + const model = editor.getModel() + if (!model) return [] + + return getAllQueries(editor).map((query) => ({ + startOffset: model.getOffsetAt({ + lineNumber: query.row + 1, + column: query.column, + }), + endOffset: model.getOffsetAt({ + lineNumber: query.endRow + 1, + column: query.endColumn, + }), + })) +} + export const getQueriesStartingFromLine = ( editor: IStandaloneCodeEditor, lineNumber: number, @@ -726,7 +751,7 @@ export const getQueryFromSelection = ( export const getQueryRequestFromEditor = ( editor: IStandaloneCodeEditor, - runWithSelection: boolean, + selectionMode: RunWithSelectionMode, ): Request | undefined => { let request: Request | undefined const selectedText = getSelectedText(editor) @@ -734,8 +759,17 @@ export const getQueryRequestFromEditor = ( ? stripSQLComments(normalizeQueryText(selectedText)) : undefined - if (runWithSelection && strippedNormalizedSelectedText) { + if (selectionMode !== "off" && strippedNormalizedSelectedText) { request = getQueryFromSelection(editor) + if (selectionMode === "complete" && request?.selection) { + request = { + query: request.query, + row: request.row, + column: request.column, + endRow: request.endRow, + endColumn: request.endColumn, + } + } } else { request = getQueryFromCursor(editor) } diff --git a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts index 53fc2985f..e432bd951 100644 --- a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts +++ b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts @@ -5,7 +5,12 @@ import { useNotebookActions, useNotebookBufferId } from "../NotebookProvider" import { useCellRefresh } from "../cellRefresh/CellRefreshContext" import { useLocalStorage } from "../../../../providers/LocalStorageProvider" import { useValidateWithGlobals } from "../globals/useValidateWithGlobals" -import { getQueryFromCursor, normalizeQueryText } from "../../Monaco/utils" +import { + getQueriesToRun, + getQueryFromCursor, + getStatementOffsets, + normalizeQueryText, +} from "../../Monaco/utils" import { resolveActiveStatementSql, resolveRunAction } from "../notebookUtils" import { emitUserAction, @@ -51,7 +56,7 @@ export const useCellRunActions = ({ } = useNotebookActions() const bufferIdForEvents = useNotebookBufferId() const validateWithGlobals = useValidateWithGlobals() - const { runWithSelection } = useLocalStorage() + const { runWithSelectionMode } = useLocalStorage() const isDrawMode = cell.mode === "draw" // A run from the Run toggle spins the Run segment; a run from the refresh @@ -109,24 +114,28 @@ export const useCellRunActions = ({ ]) const tryRunSelection = useCallback(async (): Promise => { - if (!runWithSelection) return false + if (runWithSelectionMode === "off") return false const ed = editorRef.current if (!ed) return false const selection = ed.getSelection() const model = ed.getModel() if (!selection || !model || selection.isEmpty()) return false - const selectedText = model.getValueInRange(selection) - const normalized = normalizeQueryText(selectedText) - if (!normalized) return false + const sql = + runWithSelectionMode === "complete" + ? getQueriesToRun(ed, getStatementOffsets(ed), "complete") + .map((request) => normalizeQueryText(request.query)) + .join(";\n") + : normalizeQueryText(model.getValueInRange(selection)) + if (!sql) return false clearHighlight() void trackEvent(ConsoleEvent.NOTEBOOK_CELL_RUN) - const { ok } = await runCell(cell.id, normalized) + const { ok } = await runCell(cell.id, sql) applyHighlight(ok) return true }, [ - runWithSelection, + runWithSelectionMode, cell.id, runCell, editorRef, diff --git a/src/scenes/Editor/Notebook/result-table/TabBar.tsx b/src/scenes/Editor/Notebook/result-table/TabBar.tsx index a6e79e291..7ed12364f 100644 --- a/src/scenes/Editor/Notebook/result-table/TabBar.tsx +++ b/src/scenes/Editor/Notebook/result-table/TabBar.tsx @@ -27,21 +27,21 @@ const truncateQuery = (query: string, maxLen = 30): string => { const SlotIcon: React.FC<{ slot: StatementSlotView }> = ({ slot }) => { if (slot.refreshing || slot.result?.type === "running") { return ( - + ) } if (slot.refreshError !== undefined) { return ( - + ) } if (slot.result === null) { return ( - + ) @@ -49,20 +49,23 @@ const SlotIcon: React.FC<{ slot: StatementSlotView }> = ({ slot }) => { const { type } = slot.result if (type === "queued") { return ( - + ) } if (type === "cancelled") { return ( - + ) } return ( - + {type === "error" ? ( ) : ( From 2da0c9546c070f3fe899c23de91bcff246e500a9 Mon Sep 17 00:00:00 2001 From: emrberk Date: Fri, 28 Aug 2026 10:21:22 +0300 Subject: [PATCH 02/12] address reviews --- e2e/tests/console/editor.spec.js | 20 ++++-- .../LocalStorageProvider/utils.test.ts | 66 ++++++++++++++++++- src/scenes/Editor/Monaco/utils.test.ts | 20 ++++++ src/scenes/Editor/Monaco/utils.ts | 3 + .../Notebook/cells/useCellRunActions.ts | 9 ++- 5 files changed, 108 insertions(+), 10 deletions(-) diff --git a/e2e/tests/console/editor.spec.js b/e2e/tests/console/editor.spec.js index ceffa8034..88c0d547e 100644 --- a/e2e/tests/console/editor.spec.js +++ b/e2e/tests/console/editor.spec.js @@ -2260,12 +2260,15 @@ describe("editor settings", () => { cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) const cellResultTabs = () => cy.get("[role='tablist'] [role='tab']") - // Given exec responses slow enough to observe the run loaders - cy.intercept("/exec*", (req) => { - req.on("response", (res) => { - res.setDelay(300) - }) - }) + // Given the first exec request is held until the loader is observed + let releaseFirstExec + cy.intercept( + { url: "/exec*", times: 1 }, + () => + new Cypress.Promise((resolve) => { + releaseFirstExec = resolve + }), + ).as("heldExec") // And a resolved notebook whose focused cell holds two statements cy.createNotebook() @@ -2281,6 +2284,11 @@ describe("editor settings", () => { // Then the table toggle spins while the fragment runs cy.get("[aria-label='View table'][aria-busy='true']").should("exist") + cy.wrap(null).should(() => { + expect(releaseFirstExec).to.be.a("function") + }) + cy.then(() => releaseFirstExec()) + cy.wait("@heldExec") cy.get("[aria-label='View table'][aria-busy='true']").should("not.exist") // And only the bare fragment ran: a single result with every column diff --git a/src/providers/LocalStorageProvider/utils.test.ts b/src/providers/LocalStorageProvider/utils.test.ts index 3f777cd71..dfbd12b2d 100644 --- a/src/providers/LocalStorageProvider/utils.test.ts +++ b/src/providers/LocalStorageProvider/utils.test.ts @@ -1,7 +1,71 @@ import { describe, it, expect } from "vitest" -import { parseRunWithSelectionMode } from "./utils" +import { + isMaxColumnWidthDraftValid, + parseMaxColumnWidth, + parseRunWithSelectionMode, +} from "./utils" + +describe("parseMaxColumnWidth", () => { + it("parses a stored number", () => { + expect(parseMaxColumnWidth("550")).toBe(550) + }) + + it("falls back to auto for a missing value", () => { + expect(parseMaxColumnWidth("")).toBe("auto") + }) + + it("falls back to auto for the stored auto keyword", () => { + expect(parseMaxColumnWidth("auto")).toBe("auto") + }) + + it("falls back to auto for garbage", () => { + expect(parseMaxColumnWidth("wide")).toBe("auto") + }) + + it("clamps values below the minimum", () => { + expect(parseMaxColumnWidth("10")).toBe(60) + }) + + it("clamps values above the maximum", () => { + expect(parseMaxColumnWidth("99999")).toBe(4000) + }) +}) + +describe("isMaxColumnWidthDraftValid", () => { + it("accepts an empty draft as auto", () => { + expect(isMaxColumnWidthDraftValid("")).toBe(true) + }) + + it("accepts a whole number within the bounds", () => { + expect(isMaxColumnWidthDraftValid("250")).toBe(true) + }) + + it("rejects numbers outside the bounds", () => { + expect(isMaxColumnWidthDraftValid("10")).toBe(false) + expect(isMaxColumnWidthDraftValid("99999")).toBe(false) + }) + + it("rejects locale-formatted and decimal numbers", () => { + expect(isMaxColumnWidthDraftValid("1,500")).toBe(false) + expect(isMaxColumnWidthDraftValid("1.500")).toBe(false) + expect(isMaxColumnWidthDraftValid("250.5")).toBe(false) + }) + + it("rejects non-numeric input", () => { + expect(isMaxColumnWidthDraftValid("wide")).toBe(false) + expect(isMaxColumnWidthDraftValid("-250")).toBe(false) + expect(isMaxColumnWidthDraftValid("1e3")).toBe(false) + }) +}) describe("parseRunWithSelectionMode", () => { + it.each(["partial", "complete", "off"] as const)( + "keeps the stored %s mode", + (mode) => { + expect(parseRunWithSelectionMode(mode)).toBe(mode) + }, + ) + it("migrates the legacy boolean values", () => { // Given values stored by the old on/off switch // When parsing them diff --git a/src/scenes/Editor/Monaco/utils.test.ts b/src/scenes/Editor/Monaco/utils.test.ts index fa60f2195..95dccaa61 100644 --- a/src/scenes/Editor/Monaco/utils.test.ts +++ b/src/scenes/Editor/Monaco/utils.test.ts @@ -13,6 +13,7 @@ import { isInflightQueryStillInPlace, shiftSelection, applyQueryKeyUpdates, + joinQueryTexts, } from "./utils" type SingleLineSelection = { startColumn: number; endColumn: number } @@ -140,6 +141,25 @@ SELECT * FROM sampled;` }) }) +describe("joinQueryTexts", () => { + it("keeps separators outside trailing line comments", () => { + const queries = [ + "select ts, price\nfrom trades -- last hour", + "select count(*) from trades", + "select max(price) from trades -- final aggregate", + ] + + const sql = joinQueryTexts(queries) + + expect(sql).toBe( + "select ts, price\nfrom trades -- last hour\n;\n" + + "select count(*) from trades\n;\n" + + "select max(price) from trades -- final aggregate", + ) + expect(getQueriesFromText(sql)).toEqual(queries) + }) +}) + describe("isCursorInComment", () => { it("returns false when cursor is in normal SQL", () => { const text = "SELECT * FROM table" diff --git a/src/scenes/Editor/Monaco/utils.ts b/src/scenes/Editor/Monaco/utils.ts index 5ab757841..40efe2e7b 100644 --- a/src/scenes/Editor/Monaco/utils.ts +++ b/src/scenes/Editor/Monaco/utils.ts @@ -1009,6 +1009,9 @@ export const normalizeQueryText = (query: string) => { return result.trim() } +export const joinQueryTexts = (queries: string[]): string => + queries.join("\n;\n") + export const findMatches = (model: editor.ITextModel, needle: string) => model.findMatches(needle, true, false, true, null, true) ?? null diff --git a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts index e432bd951..9996a4a96 100644 --- a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts +++ b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts @@ -9,6 +9,7 @@ import { getQueriesToRun, getQueryFromCursor, getStatementOffsets, + joinQueryTexts, normalizeQueryText, } from "../../Monaco/utils" import { resolveActiveStatementSql, resolveRunAction } from "../notebookUtils" @@ -123,9 +124,11 @@ export const useCellRunActions = ({ const sql = runWithSelectionMode === "complete" - ? getQueriesToRun(ed, getStatementOffsets(ed), "complete") - .map((request) => normalizeQueryText(request.query)) - .join(";\n") + ? joinQueryTexts( + getQueriesToRun(ed, getStatementOffsets(ed), "complete").map( + (request) => normalizeQueryText(request.query), + ), + ) : normalizeQueryText(model.getValueInRange(selection)) if (!sql) return false From ce0c10b5621a61b29090df115f637530698a0ffe Mon Sep 17 00:00:00 2001 From: emrberk Date: Fri, 28 Aug 2026 15:58:53 +0300 Subject: [PATCH 03/12] address reviews --- e2e/questdb | 2 +- e2e/tests/console/editor.spec.js | 89 +++++++---- src/scenes/Editor/Monaco/QueryDropdown.tsx | 16 +- src/scenes/Editor/Monaco/index.tsx | 13 +- .../Editor/Monaco/queryDropdownUtils.test.ts | 31 ++++ .../Editor/Monaco/queryDropdownUtils.ts | 16 ++ src/scenes/Editor/Monaco/utils.test.ts | 141 +++++++++++++++++- src/scenes/Editor/Monaco/utils.ts | 78 +++++++--- .../Notebook/cells/useCellRunActions.ts | 7 +- 9 files changed, 331 insertions(+), 62 deletions(-) create mode 100644 src/scenes/Editor/Monaco/queryDropdownUtils.test.ts create mode 100644 src/scenes/Editor/Monaco/queryDropdownUtils.ts diff --git a/e2e/questdb b/e2e/questdb index 2d9244fec..ee18e3667 160000 --- a/e2e/questdb +++ b/e2e/questdb @@ -1 +1 @@ -Subproject commit 2d9244fec3e5a17e3de1b7319d090a6b1cd94f4a +Subproject commit ee18e3667f4c5ef033ac07d2b49dcd6165b609d2 diff --git a/e2e/tests/console/editor.spec.js b/e2e/tests/console/editor.spec.js index 88c0d547e..4f7ab8e01 100644 --- a/e2e/tests/console/editor.spec.js +++ b/e2e/tests/console/editor.spec.js @@ -994,29 +994,36 @@ describe("&query URL param", () => { params.set("executeQuery", "true") return `${baseUrl}/?${params.toString()}` } + const expectClipboardWrite = (callCount, sql) => { + cy.get("@clipboardWrite").should((writeText) => { + expect(writeText).to.have.callCount(callCount) + expect(writeText.lastCall.args).to.deep.eq([expectedUrl(sql)]) + }) + } + cy.window().then((win) => { + cy.stub(win.navigator.clipboard, "writeText") + .as("clipboardWrite") + .resolves() + }) // When — glyph dropdown copies single full query cy.openRunDropdownInLine(1) cy.getByDataHook("dropdown-item-copy-query-link").click() // Then - cy.window() - .its("navigator.clipboard") - .then((clip) => clip.readText()) - .should("eq", expectedUrl("SELECT 1;")) + expectClipboardWrite(1, "SELECT 1;") // When — glyph dropdown copies the complete query even with a // fragment selected inside it cy.selectRange({ lineNumber: 2, column: 1 }, { lineNumber: 2, column: 7 }) cy.getByDataHook("button-run-query").should("contain", "Run selected query") cy.openRunDropdownInLine(2) - cy.getByDataHook("dropdown-item-copy-query-link").click() + cy.getByDataHook("dropdown-item-copy-query-link") + .should("contain", 'Copy link to "SELECT 2"') + .click() // Then - cy.window() - .its("navigator.clipboard") - .then((clip) => clip.readText()) - .should("eq", expectedUrl("SELECT 2;")) + expectClipboardWrite(2, "SELECT 2;") // When — Alt+L copies single query at cursor cy.clickLine(3) @@ -1024,10 +1031,7 @@ describe("&query URL param", () => { cy.realPress(["Alt", "L"]) // Then - cy.window() - .its("navigator.clipboard") - .then((clip) => clip.readText()) - .should("eq", expectedUrl("SELECT 3;")) + expectClipboardWrite(3, "SELECT 3;") // When — Alt+L copies selection spanning multiple queries cy.selectRange({ lineNumber: 1, column: 1 }, { lineNumber: 2, column: 9 }) @@ -1038,19 +1042,13 @@ describe("&query URL param", () => { cy.realPress(["Alt", "L"]) // Then - cy.window() - .its("navigator.clipboard") - .then((clip) => clip.readText()) - .should("eq", expectedUrl("SELECT 1;\n\nSELECT 2;")) + expectClipboardWrite(4, "SELECT 1;\n\nSELECT 2;") // When — Alt+Shift+L copies all queries in tab cy.realPress(["Alt", "Shift", "L"]) // Then - cy.window() - .its("navigator.clipboard") - .then((clip) => clip.readText()) - .should("eq", expectedUrl("SELECT 1;\n\nSELECT 2;\n\nSELECT 3;")) + expectClipboardWrite(5, "SELECT 1;\n\nSELECT 2;\n\nSELECT 3;") }) }) @@ -2312,6 +2310,9 @@ describe("editor settings", () => { .should("have.attr", "title", `select a from ${table}`) .and("have.attr", "aria-selected", "true") cellResultTabs().eq(1).should("have.attr", "title", "select 33") + cy.get(".selectionSuccessHighlight, .selectionErrorHighlight").should( + "not.exist", + ) // When the mode is off and the same selection is made setMode("off") @@ -2338,7 +2339,40 @@ describe("editor settings", () => { .and("contain", "33") }) - it("expands the share-link fragment to the complete query, with the setting off", () => { + it("does not run a notebook cell when complete selection has no query", () => { + // Given complete selection mode and a cell containing SQL plus a comment + openEditorSettings() + cy.getByDataHook("editor-settings-run-with-selection").click() + cy.getByDataHook("run-with-selection-complete").click() + cy.getByDataHook("editor-settings-save").click() + + cy.createNotebook() + cy.focusNotebookCell() + cy.focused().type("select 1;\n-- note", { delay: 0 }) + cy.withFocusedEditor((editor) => + editor.setSelection({ + startLineNumber: 2, + startColumn: 1, + endLineNumber: 2, + endColumn: 8, + }), + ) + + // When the comment-only selection is run + let execRequests = 0 + cy.intercept({ url: "/exec*" }, (request) => { + execRequests += 1 + request.continue() + }) + cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) + + // Then it is a handled no-op rather than falling through to the whole cell + cy.wait(500) + cy.then(() => expect(execRequests).to.eq(0)) + cy.get("[data-hook='result-grid-tanstack']:visible").should("not.exist") + }) + + it("isolates a shared fragment that only matches part of a query", () => { // Given the setting is turned off openEditorSettings() cy.getByDataHook("editor-settings-run-with-selection").click() @@ -2355,10 +2389,13 @@ describe("editor settings", () => { ) cy.getEditorContent().should("be.visible") - // Then the complete containing statement runs, not just the fragment - cy.getGridRows().should("have.length", 2) - cy.getGridRow(0).should("contain", "1") - cy.getGridRow(1).should("contain", "2") + // Then the fragment opens in its own buffer and only that SQL runs + cy.getEditorTabByTitle("Shared Query") + .should("be.visible") + .should("have.attr", "active") + cy.getEditorContent().should("have.value", "select 2") + cy.getGridRows().should("have.length", 1) + cy.getGridRow(0).should("contain", "2") }) it("caps column width at the configured maximum and returns to auto", () => { diff --git a/src/scenes/Editor/Monaco/QueryDropdown.tsx b/src/scenes/Editor/Monaco/QueryDropdown.tsx index 8e265261c..1a90716d9 100644 --- a/src/scenes/Editor/Monaco/QueryDropdown.tsx +++ b/src/scenes/Editor/Monaco/QueryDropdown.tsx @@ -6,6 +6,10 @@ import { DropdownMenu } from "../../../components/DropdownMenu" import { PlayFilled } from "../../../components/icons/play-filled" import { AISparkle } from "../../../components/AISparkle" import type { Request } from "./utils" +import { + extractFullQueryText, + extractQueryTextToRun, +} from "./queryDropdownUtils" const HiddenTrigger = styled.div<{ style?: { top: string; left: string } }>` position: fixed; @@ -48,14 +52,6 @@ export const QueryDropdown: React.FC = ({ onOpenChange(isOpen) } - const extractQueryTextToRun = (query: Request) => { - if (!query) return "query" - const queryText = query.selection ? query.selection.queryText : query.query - return queryText.length > 30 - ? `"${queryText.substring(0, 30)}..."` - : `"${queryText}"` - } - const isExplainDisabled = (query: Request) => { if (!query) return false const queryText = query.selection ? query.selection.queryText : query.query @@ -124,7 +120,7 @@ export const QueryDropdown: React.FC = ({ data-hook={`dropdown-item-copy-query-link-${index}`} icon={} > - Copy link to {extractQueryTextToRun(query)} + Copy link to {extractFullQueryText(query)} , ) } @@ -159,7 +155,7 @@ export const QueryDropdown: React.FC = ({ data-hook="dropdown-item-copy-query-link" icon={} > - Copy link to {extractQueryTextToRun(queriesRef.current[0])} + Copy link to {extractFullQueryText(queriesRef.current[0])} , ]} diff --git a/src/scenes/Editor/Monaco/index.tsx b/src/scenes/Editor/Monaco/index.tsx index 08031c97c..25650e26e 100644 --- a/src/scenes/Editor/Monaco/index.tsx +++ b/src/scenes/Editor/Monaco/index.tsx @@ -53,6 +53,7 @@ import { clearModelMarkers, clearValidationMarkers, findMatches, + isFullQueryMatch, getErrorRange, getQueryFromCursor, getQueryRequestFromEditor, @@ -523,7 +524,6 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { runQueryAction(query, RunningType.EXPLAIN) } - // Share links always carry complete queries, never selection fragments. const buildAndCopyShareLink = (requests: Request[]) => { if (requests.length === 0) { toast.error("Nothing to copy") @@ -1273,11 +1273,14 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const trimmedQuery = query.trim() // Find if the query is already in the editor const matches = findMatches(model, trimmedQuery) - if (matches && matches.length > 0) { - editor.setSelection(matches[0].range) + const fullQueryMatch = matches?.find((match) => + isFullQueryMatch(editor, match.range), + ) + if (fullQueryMatch) { + editor.setSelection(fullQueryMatch.range) editor.revealPositionInCenter({ - lineNumber: matches[0].range.startLineNumber, - column: matches[0].range.startColumn, + lineNumber: fullQueryMatch.range.startLineNumber, + column: fullQueryMatch.range.startColumn, }) // otherwise, open the query in a new buffer } else { diff --git a/src/scenes/Editor/Monaco/queryDropdownUtils.test.ts b/src/scenes/Editor/Monaco/queryDropdownUtils.test.ts new file mode 100644 index 000000000..06b80bec0 --- /dev/null +++ b/src/scenes/Editor/Monaco/queryDropdownUtils.test.ts @@ -0,0 +1,31 @@ +import { + extractFullQueryText, + extractQueryTextToRun, +} from "./queryDropdownUtils" + +describe("query dropdown labels", () => { + it("handles the initial state before a query is available", () => { + expect(extractQueryTextToRun(undefined)).toBe("query") + expect(extractFullQueryText(undefined)).toBe("query") + }) + + it("uses the selection for run labels and the whole query for link labels", () => { + const query = { + query: "SELECT first, second FROM long_table_name", + row: 0, + column: 1, + endRow: 0, + endColumn: 42, + selection: { + startOffset: 7, + endOffset: 12, + queryText: "first", + }, + } + + expect(extractQueryTextToRun(query)).toBe('"first"') + expect(extractFullQueryText(query)).toBe( + '"SELECT first, second FROM long..."', + ) + }) +}) diff --git a/src/scenes/Editor/Monaco/queryDropdownUtils.ts b/src/scenes/Editor/Monaco/queryDropdownUtils.ts new file mode 100644 index 000000000..179b2a117 --- /dev/null +++ b/src/scenes/Editor/Monaco/queryDropdownUtils.ts @@ -0,0 +1,16 @@ +import type { Request } from "./utils" + +const formatQueryText = (queryText: string) => + queryText.length > 30 + ? `"${queryText.substring(0, 30)}..."` + : `"${queryText}"` + +export const extractQueryTextToRun = (query?: Request) => { + if (!query) return "query" + return formatQueryText( + query.selection ? query.selection.queryText : query.query, + ) +} + +export const extractFullQueryText = (query?: Request) => + query ? formatQueryText(query.query) : "query" diff --git a/src/scenes/Editor/Monaco/utils.test.ts b/src/scenes/Editor/Monaco/utils.test.ts index 95dccaa61..f6380387b 100644 --- a/src/scenes/Editor/Monaco/utils.test.ts +++ b/src/scenes/Editor/Monaco/utils.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest" -import type { editor } from "monaco-editor" +import type { editor, IRange } from "monaco-editor" import { getQueriesFromText, isCursorInComment, @@ -13,6 +13,8 @@ import { isInflightQueryStillInPlace, shiftSelection, applyQueryKeyUpdates, + getStatementOffsets, + isFullQueryMatch, joinQueryTexts, } from "./utils" @@ -40,6 +42,8 @@ const makeSingleLineEditor = ( text.substring(range.startColumn - 1, range.endColumn - 1), getOffsetAt: (position: { column: number }) => position.column - 1, getPositionAt: (offset: number) => ({ lineNumber: 1, column: offset + 1 }), + getLineCount: () => 1, + getLineContent: () => text, } return { @@ -57,6 +61,53 @@ const makeSingleLineEditor = ( } as unknown as editor.IStandaloneCodeEditor } +const makeMultiLineEditor = (text: string) => { + const lines = text.split("\n") + const getOffsetAt = (position: { lineNumber: number; column: number }) => + lines + .slice(0, position.lineNumber - 1) + .reduce((offset, line) => offset + line.length + 1, 0) + + position.column - + 1 + const getPositionAt = (offset: number) => { + let remaining = offset + for (let index = 0; index < lines.length; index++) { + if (remaining <= lines[index].length) { + return { lineNumber: index + 1, column: remaining + 1 } + } + remaining -= lines[index].length + 1 + } + return { + lineNumber: lines.length, + column: lines[lines.length - 1].length + 1, + } + } + const model = { + getOffsetAt, + getPositionAt, + getValueInRange: (range: IRange) => + text.substring( + getOffsetAt({ + lineNumber: range.startLineNumber, + column: range.startColumn, + }), + getOffsetAt({ + lineNumber: range.endLineNumber, + column: range.endColumn, + }), + ), + getLineCount: () => lines.length, + getLineContent: (lineNumber: number) => lines[lineNumber - 1], + } + + return { + getModel: () => model, + getValue: () => text, + getPosition: () => getPositionAt(text.length), + getSelection: () => null, + } as unknown as editor.IStandaloneCodeEditor +} + describe("getQueriesFromText", () => { it("splits two simple statements", () => { expect(getQueriesFromText("SELECT 1; SELECT 2;")).toEqual([ @@ -466,6 +517,25 @@ describe("run with selection modes", () => { expect(result).toEqual([getQueryFromCursor(editor)]) expect(result.every((request) => !request.selection)).toBe(true) }) + + it("keeps off and complete distinct for a cross-statement selection", () => { + // Given a valid selection that starts at the cursor and crosses into the + // second statement + const selection = { startColumn: 3, endColumn: 16 } + const editor = makeSingleLineEditor(TEXT, 3, selection) + + // When resolving the same selection in each mode + const complete = getQueriesToRun(editor, QUERY_OFFSETS, "complete") + const off = getQueriesToRun(editor, QUERY_OFFSETS, "off") + + // Then complete expands both touched statements while off uses only the + // cursor statement + expect(complete.map((request) => request.query)).toEqual([ + "SELECT 11", + "SELECT 22", + ]) + expect(off.map((request) => request.query)).toEqual(["SELECT 11"]) + }) }) describe("getQueryRequestFromEditor", () => { @@ -509,6 +579,75 @@ describe("run with selection modes", () => { }) }) +describe("statement boundaries", () => { + it("accepts a match that covers a complete statement", () => { + const text = "SELECT 1; SELECT 2;" + const startColumn = text.indexOf("SELECT 2") + 1 + const editor = makeSingleLineEditor(text, startColumn, null) + + expect( + isFullQueryMatch(editor, { + startLineNumber: 1, + startColumn, + endLineNumber: 1, + endColumn: text.length + 1, + }), + ).toBe(true) + }) + + it("accepts complete statements followed by comments", () => { + const text = "SELECT 1;\n-- shared note" + const editor = makeMultiLineEditor(text) + + expect( + isFullQueryMatch(editor, { + startLineNumber: 1, + startColumn: 1, + endLineNumber: 2, + endColumn: 15, + }), + ).toBe(true) + }) + + it("accepts a match spanning multiple complete statements", () => { + const text = "SELECT 1;\n\nSELECT 2;" + const editor = makeMultiLineEditor(text) + + expect( + isFullQueryMatch(editor, { + startLineNumber: 1, + startColumn: 1, + endLineNumber: 3, + endColumn: 10, + }), + ).toBe(true) + }) + + it("rejects a match contained inside a larger statement", () => { + const text = "SELECT 1 UNION ALL SELECT 2;" + const startColumn = text.indexOf("SELECT 2") + 1 + const editor = makeSingleLineEditor(text, startColumn, null) + + expect( + isFullQueryMatch(editor, { + startLineNumber: 1, + startColumn, + endLineNumber: 1, + endColumn: text.length + 1, + }), + ).toBe(false) + }) + + it("calculates offsets for multiline statements", () => { + const editor = makeMultiLineEditor("SELECT\n 11;\nSELECT\n 22;") + + expect(getStatementOffsets(editor)).toEqual([ + { startOffset: 0, endOffset: 11 }, + { startOffset: 13, endOffset: 24 }, + ]) + }) +}) + describe("isQueryTextAtOffset", () => { it("matches when the query is still at its offset", () => { // Given a buffer holding the query at a known offset diff --git a/src/scenes/Editor/Monaco/utils.ts b/src/scenes/Editor/Monaco/utils.ts index 40efe2e7b..3bd9eaf86 100644 --- a/src/scenes/Editor/Monaco/utils.ts +++ b/src/scenes/Editor/Monaco/utils.ts @@ -88,6 +88,11 @@ export type Request = Readonly<{ } }> +const toCompleteQueryRequest = (request: Request): Request => { + const { selection: _selection, ...completeQueryRequest } = request + return completeQueryRequest +} + type SqlTextItem = { row: number col: number @@ -194,17 +199,10 @@ export const getQueriesToRun = ( if (!stripSQLComments(normalizeQueryText(clampedSelectionText))) { return undefined } - const fullQueryRequest = { - query: query.query, - row: query.row, - column: query.column, - endRow: query.endRow, - endColumn: query.endColumn, - } return selectionMode === "complete" - ? fullQueryRequest + ? toCompleteQueryRequest(query) : { - ...fullQueryRequest, + ...query, selection: { startOffset: model.getOffsetAt({ lineNumber: clampedSelection.startLineNumber, @@ -761,14 +759,8 @@ export const getQueryRequestFromEditor = ( if (selectionMode !== "off" && strippedNormalizedSelectedText) { request = getQueryFromSelection(editor) - if (selectionMode === "complete" && request?.selection) { - request = { - query: request.query, - row: request.row, - column: request.column, - endRow: request.endRow, - endColumn: request.endColumn, - } + if (selectionMode === "complete" && request) { + request = toCompleteQueryRequest(request) } } else { request = getQueryFromCursor(editor) @@ -1015,6 +1007,58 @@ export const joinQueryTexts = (queries: string[]): string => export const findMatches = (model: editor.ITextModel, needle: string) => model.findMatches(needle, true, false, true, null, true) ?? null +export const isFullQueryMatch = ( + editor: IStandaloneCodeEditor, + range: IRange, +): boolean => { + const model = editor.getModel() + if (!model) return false + + const matchStartOffset = model.getOffsetAt({ + lineNumber: range.startLineNumber, + column: range.startColumn, + }) + const matchEndOffset = model.getOffsetAt({ + lineNumber: range.endLineNumber, + column: range.endColumn, + }) + const queryRanges = getAllQueries(editor) + .map((query) => ({ + startOffset: model.getOffsetAt({ + lineNumber: query.row + 1, + column: query.column, + }), + endOffset: model.getOffsetAt({ + lineNumber: query.endRow + 1, + column: query.endColumn, + }), + })) + .filter( + ({ startOffset, endOffset }) => + startOffset < matchEndOffset && endOffset > matchStartOffset, + ) + + if (queryRanges.length === 0) return false + + const firstQuery = queryRanges[0] + const lastQuery = queryRanges[queryRanges.length - 1] + if ( + matchStartOffset !== firstQuery.startOffset || + matchEndOffset < lastQuery.endOffset + ) { + return false + } + + const trailingStart = model.getPositionAt(lastQuery.endOffset) + const trailingText = model.getValueInRange({ + startLineNumber: trailingStart.lineNumber, + startColumn: trailingStart.column, + endLineNumber: range.endLineNumber, + endColumn: range.endColumn, + }) + return getQueriesFromText(trailingText).length === 0 +} + export const getLastPosition = ( editor: IStandaloneCodeEditor, ): IPosition | undefined => { diff --git a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts index 9996a4a96..efb0ca771 100644 --- a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts +++ b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts @@ -130,12 +130,15 @@ export const useCellRunActions = ({ ), ) : normalizeQueryText(model.getValueInRange(selection)) - if (!sql) return false + if (!sql) { + if (runWithSelectionMode === "complete") clearHighlight() + return runWithSelectionMode === "complete" + } clearHighlight() void trackEvent(ConsoleEvent.NOTEBOOK_CELL_RUN) const { ok } = await runCell(cell.id, sql) - applyHighlight(ok) + if (runWithSelectionMode === "partial") applyHighlight(ok) return true }, [ runWithSelectionMode, From 47fccaef5f641eb49a747f7b0eb0ee67a83abc65 Mon Sep 17 00:00:00 2001 From: emrberk Date: Sun, 30 Aug 2026 11:21:20 +0300 Subject: [PATCH 04/12] separate single-run vs multi-run completely, fix selection run issues --- e2e/questdb | 2 +- e2e/tests/console/editor.spec.js | 156 +++++++++++++++-- src/scenes/Editor/Monaco/utils.test.ts | 160 +++++++++++++++++- src/scenes/Editor/Monaco/utils.ts | 41 +++++ .../Editor/Notebook/NotebookProvider.tsx | 5 +- src/scenes/Editor/Notebook/cells/Cell.tsx | 43 +++-- .../Notebook/cells/CellRunDrawToggles.tsx | 4 +- .../Notebook/cells/useCellRunActions.ts | 142 ++++++++-------- .../Editor/Notebook/notebookUtils.test.ts | 43 ++++- src/scenes/Editor/Notebook/notebookUtils.ts | 76 ++++++--- 10 files changed, 539 insertions(+), 133 deletions(-) diff --git a/e2e/questdb b/e2e/questdb index ee18e3667..e40ec59da 160000 --- a/e2e/questdb +++ b/e2e/questdb @@ -1 +1 @@ -Subproject commit ee18e3667f4c5ef033ac07d2b49dcd6165b609d2 +Subproject commit e40ec59dac6237ec702333feb6825ffd09a15531 diff --git a/e2e/tests/console/editor.spec.js b/e2e/tests/console/editor.spec.js index 4f7ab8e01..a7d022326 100644 --- a/e2e/tests/console/editor.spec.js +++ b/e2e/tests/console/editor.spec.js @@ -2230,6 +2230,22 @@ describe("editor settings", () => { cy.getGridRow(0).should("contain", "2") }) + it("disables run while the selection covers only a comment between statements", () => { + // Given a commented-out statement between two live statements + cy.typeQueryDirectly("select 1;\n-- old: delete from t\nselect 2;") + + // When a word inside the comment is selected + cy.selectRange({ lineNumber: 2, column: 9 }, { lineNumber: 2, column: 15 }) + + // Then the run button disables instead of targeting a neighbouring + // statement + cy.getByDataHook("button-run-query").should("be.disabled") + + // And moving the cursor back into a statement enables it again + cy.clickLine(1) + cy.getByDataHook("button-run-query").should("not.be.disabled") + }) + it("resolves the notebook cell selection per mode: partial, complete, off", () => { const table = runWithSelectionTable const sql = `select a from ${table}; select 33` @@ -2339,16 +2355,11 @@ describe("editor settings", () => { .and("contain", "33") }) - it("does not run a notebook cell when complete selection has no query", () => { - // Given complete selection mode and a cell containing SQL plus a comment - openEditorSettings() - cy.getByDataHook("editor-settings-run-with-selection").click() - cy.getByDataHook("run-with-selection-complete").click() - cy.getByDataHook("editor-settings-save").click() - + it("runs the whole notebook cell from Run cell and Run All regardless of selection", () => { + // Given a cell with two statements and a comment-only selection cy.createNotebook() cy.focusNotebookCell() - cy.focused().type("select 1;\n-- note", { delay: 0 }) + cy.focused().type("select 1;\n-- note\nselect 2;", { delay: 0 }) cy.withFocusedEditor((editor) => editor.setSelection({ startLineNumber: 2, @@ -2358,18 +2369,137 @@ describe("editor settings", () => { }), ) - // When the comment-only selection is run - let execRequests = 0 + const execQueries = [] + cy.intercept({ url: "/exec*" }, (request) => { + execQueries.push(new URL(request.url).searchParams.get("query")) + request.continue() + }) + + // When Run cell is clicked, the selection does not narrow its scope + cy.get("[data-notebook-cell] button[aria-label='Run cell']") + .should("contain", "Run") + .and("have.attr", "aria-disabled", "false") + .click() + + // Then every statement runs + cy.get("[role='tablist'] [data-hook='result-tab-success']").should( + "have.length", + 2, + ) + cy.wrap(null).should(() => { + expect([...execQueries].sort()).to.deep.eq(["select 1", "select 2"]) + }) + + // And Cmd+Shift+Enter runs the whole cell even from the result area + cy.get("[role='tablist'] [role='tab']").eq(1).click().should("have.focus") + cy.then(() => { + execQueries.length = 0 + }) + cy.window().then((win) => { + win.dispatchEvent( + new win.KeyboardEvent("keydown", { + key: "Enter", + shiftKey: true, + metaKey: Cypress.platform === "darwin", + ctrlKey: Cypress.platform !== "darwin", + bubbles: true, + }), + ) + }) + cy.wrap(null).should(() => { + expect([...execQueries].sort()).to.deep.eq(["select 1", "select 2"]) + }) + }) + + it("never widens notebook Cmd+Enter beyond its focused query", () => { + // Given a fresh cell whose cursor is in the gap between two statements + cy.createNotebook() + cy.focusNotebookCell() + cy.focused().type("select 1;\n-- note\nselect 2;", { delay: 0 }) + cy.withFocusedEditor((editor) => + editor.setPosition({ lineNumber: 2, column: 4 }), + ) + + const execQueries = [] cy.intercept({ url: "/exec*" }, (request) => { - execRequests += 1 + execQueries.push(new URL(request.url).searchParams.get("query")) request.continue() }) + + // When Cmd+Enter is pressed in Monaco with no query at the cursor cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) - // Then it is a handled no-op rather than falling through to the whole cell + // Then it is a no-op, never an implicit Run All cy.wait(500) - cy.then(() => expect(execRequests).to.eq(0)) + cy.then(() => expect(execQueries).to.deep.eq([])) cy.get("[data-hook='result-grid-tanstack']:visible").should("not.exist") + + // When only the second statement is run + cy.withFocusedEditor((editor) => + editor.setPosition({ lineNumber: 3, column: 4 }), + ) + cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) + + // Then both statement tabs fit alongside a fully visible result row + cy.wrap(null).should(() => { + expect(execQueries).to.deep.eq(["select 2"]) + }) + cy.get("[role='tablist'] [data-hook='result-tab-not-run']").should( + "have.length", + 1, + ) + cy.get("[role='tablist'] [data-hook='result-tab-success']").should( + "have.length", + 1, + ) + cy.getGridRow(0).should("be.visible").and("contain", "2") + cy.then(() => { + execQueries.length = 0 + }) + + // Given an explicit Run All has created a result tab for each statement + cy.withFocusedEditor((editor) => editor.getAction("notebook-run-all").run()) + cy.get("[role='tablist'] [data-hook='result-tab-success']").should( + "have.length", + 2, + ) + cy.then(() => { + execQueries.length = 0 + }) + + // When the second result tab has focus and Cmd+Enter is pressed + cy.get("[role='tablist'] [role='tab']") + .eq(1) + .click() + .should("have.focus") + .and("have.attr", "title", "select 2") + cy.window().then((win) => { + win.dispatchEvent( + new win.KeyboardEvent("keydown", { + key: "Enter", + metaKey: Cypress.platform === "darwin", + ctrlKey: Cypress.platform !== "darwin", + bubbles: true, + }), + ) + }) + + // Then only that tab's statement runs + cy.wrap(null).should(() => { + expect(execQueries).to.deep.eq(["select 2"]) + }) + + // And the same active tab never becomes a fallback for Monaco + cy.focusNotebookCell() + cy.withFocusedEditor((editor) => + editor.setPosition({ lineNumber: 2, column: 4 }), + ) + cy.then(() => { + execQueries.length = 0 + }) + cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) + cy.wait(500) + cy.then(() => expect(execQueries).to.deep.eq([])) }) it("isolates a shared fragment that only matches part of a query", () => { diff --git a/src/scenes/Editor/Monaco/utils.test.ts b/src/scenes/Editor/Monaco/utils.test.ts index f6380387b..1d02e0689 100644 --- a/src/scenes/Editor/Monaco/utils.test.ts +++ b/src/scenes/Editor/Monaco/utils.test.ts @@ -16,6 +16,7 @@ import { getStatementOffsets, isFullQueryMatch, joinQueryTexts, + resolveSelectionRun, } from "./utils" type SingleLineSelection = { startColumn: number; endColumn: number } @@ -61,7 +62,7 @@ const makeSingleLineEditor = ( } as unknown as editor.IStandaloneCodeEditor } -const makeMultiLineEditor = (text: string) => { +const makeMultiLineEditor = (text: string, selection?: IRange) => { const lines = text.split("\n") const getOffsetAt = (position: { lineNumber: number; column: number }) => lines @@ -104,7 +105,23 @@ const makeMultiLineEditor = (text: string) => { getModel: () => model, getValue: () => text, getPosition: () => getPositionAt(text.length), - getSelection: () => null, + getSelection: () => + selection + ? { + ...selection, + getStartPosition: () => ({ + lineNumber: selection.startLineNumber, + column: selection.startColumn, + }), + getEndPosition: () => ({ + lineNumber: selection.endLineNumber, + column: selection.endColumn, + }), + isEmpty: () => + selection.startLineNumber === selection.endLineNumber && + selection.startColumn === selection.endColumn, + } + : null, } as unknown as editor.IStandaloneCodeEditor } @@ -536,6 +553,34 @@ describe("run with selection modes", () => { ]) expect(off.map((request) => request.query)).toEqual(["SELECT 11"]) }) + + it("returns nothing when the selection covers only a comment line between statements", () => { + // Given a commented-out statement between two live statements, with a + // word inside the comment selected + const text = + "SELECT * FROM trades;\n-- old: DELETE FROM trades\nDROP TABLE trades;" + const commentWordSelection = { + startLineNumber: 2, + startColumn: 9, + endLineNumber: 2, + endColumn: 15, + } + const makeEditor = () => makeMultiLineEditor(text, commentWordSelection) + const offsets = getStatementOffsets(makeEditor()) + + // When resolving the queries to run in each selection mode + const complete = getQueriesToRun(makeEditor(), offsets, "complete") + const partial = getQueriesToRun(makeEditor(), offsets, "partial") + const off = getQueriesToRun(makeEditor(), offsets, "off") + + // Then no mode runs or expands into a statement the user never touched; + // off falls back to the query at the cursor as always + expect(complete).toEqual([]) + expect(partial).toEqual([]) + expect(off.map((request) => request.query)).toEqual([ + "DROP TABLE trades", + ]) + }) }) describe("getQueryRequestFromEditor", () => { @@ -579,6 +624,117 @@ describe("run with selection modes", () => { }) }) +describe("resolveSelectionRun", () => { + const TEXT = "SELECT 11; SELECT 22" + const FRAGMENT_SELECTION = { startColumn: 4, endColumn: 10 } + + it("reports no selection when the mode is off", () => { + // Given a real selection but the mode is off + const editor = makeSingleLineEditor(TEXT, 3, FRAGMENT_SELECTION) + + // When resolving the selection run + // Then the selection is ignored + expect(resolveSelectionRun(editor, "off")).toEqual({ + kind: "no-selection", + }) + }) + + it("reports no selection for a collapsed cursor", () => { + // Given a collapsed selection (a bare cursor) + const editor = makeSingleLineEditor(TEXT, 3, { + startColumn: 3, + endColumn: 3, + }) + + // When resolving in partial and complete modes + // Then neither treats the cursor as a selection + expect(resolveSelectionRun(editor, "partial")).toEqual({ + kind: "no-selection", + }) + expect(resolveSelectionRun(editor, "complete")).toEqual({ + kind: "no-selection", + }) + }) + + it("runs the fragment in partial mode", () => { + // Given a fragment of the first statement selected in partial mode + const editor = makeSingleLineEditor(TEXT, 3, FRAGMENT_SELECTION) + + // When resolving the selection run + // Then the fragment itself is the sql + expect(resolveSelectionRun(editor, "partial")).toEqual({ + kind: "run", + sql: "ECT 11", + }) + }) + + it("blocks a comment-only selection in partial mode", () => { + // Given only a line comment is selected + const text = "SELECT 1;\n-- a note" + const editor = makeMultiLineEditor(text, { + startLineNumber: 2, + startColumn: 1, + endLineNumber: 2, + endColumn: 10, + }) + + // When resolving the selection run + // Then there is nothing to run and the run is blocked + expect(resolveSelectionRun(editor, "partial")).toEqual({ + kind: "no-query", + }) + }) + + it("expands a cross-statement fragment in complete mode", () => { + // Given a selection cutting into both statements in complete mode + const editor = makeSingleLineEditor(TEXT, 3, { + startColumn: 8, + endColumn: 16, + }) + + // When resolving the selection run + // Then both whole statements join into one run + expect(resolveSelectionRun(editor, "complete")).toEqual({ + kind: "run", + sql: "SELECT 11\n;\nSELECT 22", + }) + }) + + it("blocks a comment-only selection in complete mode", () => { + // Given only the trailing comment line is selected in complete mode + const text = "SELECT 1;\n-- a note" + const editor = makeMultiLineEditor(text, { + startLineNumber: 2, + startColumn: 1, + endLineNumber: 2, + endColumn: 10, + }) + + // When resolving the selection run + // Then no statement is touched and the run is blocked + expect(resolveSelectionRun(editor, "complete")).toEqual({ + kind: "no-query", + }) + }) + + it("blocks a comment-only selection between statements in complete mode", () => { + // Given a comment line between two statements, fully selected + const text = "SELECT 1;\n-- old: DELETE FROM t\nSELECT 2;" + const editor = makeMultiLineEditor(text, { + startLineNumber: 2, + startColumn: 1, + endLineNumber: 2, + endColumn: 22, + }) + + // When resolving the selection run in complete mode + // Then it does not expand into either neighbouring statement + expect(resolveSelectionRun(editor, "complete")).toEqual({ + kind: "no-query", + }) + }) +}) + describe("statement boundaries", () => { it("accepts a match that covers a complete statement", () => { const text = "SELECT 1; SELECT 2;" diff --git a/src/scenes/Editor/Monaco/utils.ts b/src/scenes/Editor/Monaco/utils.ts index 3bd9eaf86..5f627a743 100644 --- a/src/scenes/Editor/Monaco/utils.ts +++ b/src/scenes/Editor/Monaco/utils.ts @@ -179,6 +179,12 @@ export const getQueriesToRun = ( return [] } + // The lookups invert when the selection sits entirely in the gap between + // two statements (a comment line, blank lines) — no statement is touched. + if (firstQueryOffsets.startOffset > lastQueryOffsets.endOffset) { + return [] + } + const queries = getQueriesInRange( editor, model.getPositionAt(firstQueryOffsets.startOffset), @@ -1004,6 +1010,41 @@ export const normalizeQueryText = (query: string) => { export const joinQueryTexts = (queries: string[]): string => queries.join("\n;\n") +export type SelectionRunResolution = + | { kind: "no-selection" } + | { kind: "no-query" } + | { kind: "run"; sql: string } + +export const resolveSelectionRun = ( + editor: IStandaloneCodeEditor, + selectionMode: RunWithSelectionMode, +): SelectionRunResolution => { + if (selectionMode === "off") return { kind: "no-selection" } + const selection = editor.getSelection() + const model = editor.getModel() + if (!selection || !model || selection.isEmpty()) { + return { kind: "no-selection" } + } + + if (selectionMode === "complete") { + const queries = getQueriesToRun( + editor, + getStatementOffsets(editor), + "complete", + ) + if (queries.length === 0) return { kind: "no-query" } + return { + kind: "run", + sql: joinQueryTexts( + queries.map((request) => normalizeQueryText(request.query)), + ), + } + } + + const sql = normalizeQueryText(model.getValueInRange(selection)) + return stripSQLComments(sql) ? { kind: "run", sql } : { kind: "no-query" } +} + export const findMatches = (model: editor.ITextModel, needle: string) => model.findMatches(needle, true, false, true, null, true) ?? null diff --git a/src/scenes/Editor/Notebook/NotebookProvider.tsx b/src/scenes/Editor/Notebook/NotebookProvider.tsx index 6adc0367c..9daeee7ab 100644 --- a/src/scenes/Editor/Notebook/NotebookProvider.tsx +++ b/src/scenes/Editor/Notebook/NotebookProvider.tsx @@ -621,7 +621,10 @@ export const NotebookProvider: React.FC<{ !cell.bottomResized ) { store.updateCell(cellId, { - bottomHeight: computeResultBottomHeight(cell.result), + bottomHeight: computeResultBottomHeight( + cell.result, + getQueriesFromText(cell.value), + ), }) } diff --git a/src/scenes/Editor/Notebook/cells/Cell.tsx b/src/scenes/Editor/Notebook/cells/Cell.tsx index 88fe7d161..3abbe7e22 100644 --- a/src/scenes/Editor/Notebook/cells/Cell.tsx +++ b/src/scenes/Editor/Notebook/cells/Cell.tsx @@ -245,7 +245,7 @@ const CellInner: React.FC = ({ (state) => updateCell(cell.id, { editorViewState: state }), [cell.id, updateCell], ), - onRunAtCursor: () => runSingle(), + onRunAtCursor: () => runSingleFromEditor(), onRunAll: () => runAll(), onContentHeightChange: handleContentHeightChange, validate: validateWithGlobals, @@ -281,16 +281,21 @@ const CellInner: React.FC = ({ editorRef.current?.getContentHeight() ?? null }, [editorRef]) - const { runAll, runSingle, handleDrawClick, isGridLoading } = - useCellRunActions({ - cell, - isRunning, - isCompactTier, - showBottomSlot, - editorRef, - applyHighlight, - clearHighlight, - }) + const { + runAll, + runSingleFromEditor, + runSingleFromResult, + handleDrawClick, + isGridLoading, + } = useCellRunActions({ + cell, + isRunning, + isCompactTier, + showBottomSlot, + editorRef, + applyHighlight, + clearHighlight, + }) const isExternalSyncRef = useRef(false) @@ -372,13 +377,21 @@ const CellInner: React.FC = ({ // Focus inside Monaco: its own action handles the key (same resolver), so // bail to avoid running twice. if (editorContainerRef.current?.contains(document.activeElement)) return - e.preventDefault() - if (e.shiftKey) runAll() - else runSingle() + if (e.shiftKey) { + e.preventDefault() + runAll() + return + } + // Outside Monaco, a single-query shortcut has a target only while focus + // is in the result area. Other cell chrome never widens Cmd+Enter. + if (resultRef.current?.contains(document.activeElement)) { + e.preventDefault() + runSingleFromResult() + } } window.addEventListener("keydown", onKey) return () => window.removeEventListener("keydown", onKey) - }, [isFocused, isMaximized, runAll, runSingle]) + }, [isFocused, isMaximized, runAll, runSingleFromResult]) const cellEl = ( = ({ }) => ( = ({ } onRun() }} - aria-label={runActive ? "Hide result" : "Run"} + aria-label={runActive ? "Hide result" : "Run cell"} > {showLabels && "Run"} diff --git a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts index efb0ca771..f8c717089 100644 --- a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts +++ b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts @@ -6,11 +6,9 @@ import { useCellRefresh } from "../cellRefresh/CellRefreshContext" import { useLocalStorage } from "../../../../providers/LocalStorageProvider" import { useValidateWithGlobals } from "../globals/useValidateWithGlobals" import { - getQueriesToRun, getQueryFromCursor, - getStatementOffsets, - joinQueryTexts, normalizeQueryText, + resolveSelectionRun, } from "../../Monaco/utils" import { resolveActiveStatementSql, resolveRunAction } from "../notebookUtils" import { @@ -35,6 +33,10 @@ type Options = { clearHighlight: () => void } +type SingleRunSource = "editor" | "result" + +type RunRequest = { kind: "all" } | { kind: "single"; source: SingleRunSource } + // Run / draw orchestration for a cell: resolves what a Run gesture means for // the current mode and view, runs it, and emits the agent-facing events. The // toolbar buttons, Monaco commands, keyboard shortcuts, and the RUN/DRAW @@ -115,29 +117,15 @@ export const useCellRunActions = ({ ]) const tryRunSelection = useCallback(async (): Promise => { - if (runWithSelectionMode === "off") return false const ed = editorRef.current if (!ed) return false - const selection = ed.getSelection() - const model = ed.getModel() - if (!selection || !model || selection.isEmpty()) return false - - const sql = - runWithSelectionMode === "complete" - ? joinQueryTexts( - getQueriesToRun(ed, getStatementOffsets(ed), "complete").map( - (request) => normalizeQueryText(request.query), - ), - ) - : normalizeQueryText(model.getValueInRange(selection)) - if (!sql) { - if (runWithSelectionMode === "complete") clearHighlight() - return runWithSelectionMode === "complete" - } - + const resolution = resolveSelectionRun(ed, runWithSelectionMode) + if (resolution.kind === "no-selection") return false clearHighlight() + if (resolution.kind === "no-query") return true + void trackEvent(ConsoleEvent.NOTEBOOK_CELL_RUN) - const { ok } = await runCell(cell.id, sql) + const { ok } = await runCell(cell.id, resolution.sql) if (runWithSelectionMode === "partial") applyHighlight(ok) return true }, [ @@ -162,22 +150,56 @@ export const useCellRunActions = ({ [bufferIdForEvents, cell.id], ) - const handleRunAll = useCallback( - async (ignoreSelection = false) => { - if (editorRef.current) { - if (!ignoreSelection && (await tryRunSelection())) return - clearHighlight() + const handleRunAll = useCallback(async () => { + if (editorRef.current) clearHighlight() + + const priorResult = + getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null + const { ok } = await runCell(cell.id) + const freshResult = + getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null + emitRanEvent(createRunStatus(priorResult, freshResult, ok)) + }, [ + cell.id, + runCell, + editorRef, + clearHighlight, + emitRanEvent, + getCellsSnapshot, + ]) + + const handleRunSingle = useCallback( + async (source: SingleRunSource) => { + let sql: string | undefined + if (source === "editor") { + const ed = editorRef.current + if (!ed) return + // Capture the cursor's statement before any await — revealing a compact + // cell can unmount Monaco during this same gesture. + const cursorQuery = getQueryFromCursor(ed)?.query + if (await tryRunSelection()) return + sql = cursorQuery + } else { + // Result focus deliberately targets the active statement tab, including + // a "Not run" tab. It is not a fallback for an unresolved editor cursor. + sql = resolveActiveStatementSql(cell.value, cell.result) } + if (!sql?.trim()) { + return + } + clearHighlight() const priorResult = getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null - const { ok } = await runCell(cell.id) + const { ok } = await runCell(cell.id, normalizeQueryText(sql)) const freshResult = getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null emitRanEvent(createRunStatus(priorResult, freshResult, ok)) }, [ cell.id, + cell.value, + cell.result, runCell, tryRunSelection, editorRef, @@ -187,46 +209,11 @@ export const useCellRunActions = ({ ], ) - const handleRunSingle = useCallback(async () => { - const ed = editorRef.current - // Capture the cursor's statement before any await — a reveal in this same - // gesture can unmount the editor, and reading it afterwards loses it. - const cursorQuery = ed ? getQueryFromCursor(ed)?.query : undefined - if (ed && (await tryRunSelection())) return - // Cursor first; otherwise the active tab's statement — resolved from the - // same frame the tabs render, so a "Not run" tab runs its own SQL and a - // single run never silently expands into running every statement. - const activeQuery = resolveActiveStatementSql(cell.value, cell.result) - const sql = cursorQuery ?? activeQuery - if (!sql?.trim()) { - await handleRunAll() - return - } - clearHighlight() - const priorResult = - getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null - const { ok } = await runCell(cell.id, normalizeQueryText(sql)) - const freshResult = - getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null - emitRanEvent(createRunStatus(priorResult, freshResult, ok)) - }, [ - cell.id, - cell.value, - cell.result, - runCell, - tryRunSelection, - editorRef, - clearHighlight, - handleRunAll, - emitRanEvent, - getCellsSnapshot, - ]) - const runResolved = useCallback( - (intent: "all" | "single", ignoreSelection = false) => { + (request: RunRequest) => { const plan = resolveRunAction( { mode: cell.mode, result: cell.result }, - { isCompactTier, showBottomSlot, intent }, + { isCompactTier, showBottomSlot, intent: request.kind }, ) if (plan.kind === "noop") return if (plan.kind === "chart") { @@ -245,8 +232,8 @@ export const useCellRunActions = ({ // Start the run before revealing: under React 17 a reveal fired from a // native key event re-renders synchronously and unmounts the editor, so // the run must read the cursor first. - if (plan.kind === "run-all") void handleRunAll(ignoreSelection) - else void handleRunSingle() + if (plan.kind === "run-all") void handleRunAll() + else if (request.kind === "single") void handleRunSingle(request.source) if (plan.reveal) setCellViewMaximized(cell.id, true) }, [ @@ -262,8 +249,15 @@ export const useCellRunActions = ({ handleRunSingle, ], ) - const runAll = useCallback(() => runResolved("all"), [runResolved]) - const runSingle = useCallback(() => runResolved("single"), [runResolved]) + const runAll = useCallback(() => runResolved({ kind: "all" }), [runResolved]) + const runSingleFromEditor = useCallback( + () => runResolved({ kind: "single", source: "editor" }), + [runResolved], + ) + const runSingleFromResult = useCallback( + () => runResolved({ kind: "single", source: "result" }), + [runResolved], + ) const cellRefresh = useCellRefresh() // Refresh on a classified non-write grid routes to the engine: every // statement refreshes in parallel while the old rows stay visible. A write @@ -289,7 +283,7 @@ export const useCellRunActions = ({ }) return } - runResolved("all", true) + runResolved({ kind: "all" }) }, [cell.id, cell.mode, cell.result, cellRefresh, emitRanEvent, runResolved]) useEffect(() => { @@ -320,5 +314,11 @@ export const useCellRunActions = ({ const isGridLoading = isRunning && firstRunRef.current - return { runAll, runSingle, handleDrawClick, isGridLoading } + return { + runAll, + runSingleFromEditor, + runSingleFromResult, + handleDrawClick, + isGridLoading, + } } diff --git a/src/scenes/Editor/Notebook/notebookUtils.test.ts b/src/scenes/Editor/Notebook/notebookUtils.test.ts index deaa6f9b9..9abd5c9ab 100644 --- a/src/scenes/Editor/Notebook/notebookUtils.test.ts +++ b/src/scenes/Editor/Notebook/notebookUtils.test.ts @@ -1453,6 +1453,42 @@ describe("computeResultBottomHeight", () => { expect(computeResultBottomHeight(make(50))).toBe(424) }) + it("one executed DQL in a multi-statement cell adds tabs but tight-fits its rows", () => { + // The result array is compact (only SELECT 2 ran), but the rendered frame + // has two tabs: SELECT 1 is "Not run" and SELECT 2 owns the result. + const result = { + results: [ + { + type: "dql" as const, + query: "select 2", + columns: [{ name: "2", type: "INT" }], + dataset: [[2]], + count: 1, + }, + ], + activeResultIndex: 0, + timestamp: 0, + } + + // 40 tab + 44 notification + 36 actions + 44 header + 1*30 row = 194. + expect(computeResultBottomHeight(result, ["select 1", "select 2"])).toBe( + 194, + ) + }) + + it("one executed non-grid result in a multi-statement cell still includes its tabs", () => { + expect( + computeResultBottomHeight( + { + results: [{ type: "ddl", query: "create table x (n int)" }], + activeResultIndex: 0, + timestamp: 0, + }, + ["create table x (n int)", "select * from x"], + ), + ).toBe(84) + }) + it("multi-statement, first DQL with rows → tab + notification + header + 10 rows", () => { // 40 + 44 + 36 + 44 + 10*30 = 464 expect( @@ -1473,8 +1509,9 @@ describe("computeResultBottomHeight", () => { ).toBe(464) }) - it("multi-statement, first is error → tab + notification only (no grid to show)", () => { - // 40 + 44 = 84 + it("multi-statement with any DQL tab reserves the full grid block", () => { + // The first tab is an error, but the active second tab renders a grid. + // Reserve the same stable multi-tab height: 40 + 44 + 36 + 44 + 10*30. expect( computeResultBottomHeight({ results: [ @@ -1490,7 +1527,7 @@ describe("computeResultBottomHeight", () => { activeResultIndex: 1, timestamp: 0, }), - ).toBe(84) + ).toBe(464) }) it("multi-statement, first DQL with columns but 0 rows → tab + full grid block", () => { diff --git a/src/scenes/Editor/Notebook/notebookUtils.ts b/src/scenes/Editor/Notebook/notebookUtils.ts index fd78e7b03..ded1de253 100644 --- a/src/scenes/Editor/Notebook/notebookUtils.ts +++ b/src/scenes/Editor/Notebook/notebookUtils.ts @@ -1370,7 +1370,12 @@ export const releaseCellResultPatch = ( lastRunStatus: carriedRunStatus(cell), lastRunError: carriedRunError(cell), ...(cell.mode !== "draw" && cell.bottomHeight == null && cell.result != null - ? { bottomHeight: computeResultBottomHeight(cell.result) } + ? { + bottomHeight: computeResultBottomHeight( + cell.result, + getQueriesFromText(cell.value), + ), + } : {}), }) @@ -1380,7 +1385,10 @@ const isDqlWithColumns = (r: SingleQueryResult): boolean => const dqlRowCount = (r: SingleQueryResult): number => r.type === "dql" ? r.dataset.length : 0 -// Computes the bottom slot height for a result based on its content. +// Computes the bottom slot height for the same statement frame rendered by +// InlineResultTable. `statements` is the editor's current statement list; a +// partial run can have one result while still rendering multiple statement +// tabs (the unexecuted statements appear as "Not run"). // // Rules: // 1. Single-statement, no grid (error / DDL / DML / notice): just the @@ -1389,26 +1397,33 @@ const dqlRowCount = (r: SingleQueryResult): number => // header + min(N, 10) rows. A 0-row DQL still shows its column headers, so // it reserves the header with no row space. Shrinks for small results, // caps at 10 for large ones. -// 3. Multi-statement (script) run: -// - tab bar always visible. -// - If the first result is non-DQL (error / DDL / DML / notice), height -// is just tab bar + notification (no grid to show). -// - Otherwise reserve a full 10 rows worth of space regardless of how -// many rows the active tab actually has (avoids jitter when switching -// between tabs that have different row counts). +// 3. Multiple rendered statement slots add the tab bar, including when all +// but one slot are "Not run". +// 4. Multiple executed results reserve a full 10 rows whenever any result +// has a DQL grid (avoids clipping and jitter when switching result tabs). +// A single executed result still tight-fits its own row count. export const computeResultBottomHeight = ( result: CellResult | null | undefined, + statements?: string[], ): number => { if (!result || result.results.length === 0) return NOTIFICATION_PX - const isMulti = result.results.length > 1 - const tabBar = isMulti ? TAB_BAR_PX : 0 - - if (isMulti) { - const first = result.results[0] - if (!first || !isDqlWithColumns(first)) { - // First query failed / wasn't DQL — there is no grid to show, no point - // reserving 10 rows of space. The tab bar still shows so the user can - // click through other tabs. + const frame = + (statements + ? deriveStatementFrame(statements, result) + : deriveStatementFrame( + result.results.map((r) => r.query), + result, + )) ?? derivePositionalFrame(result) + const slots = frame?.slots ?? [] + const hasMultipleTabs = slots.length > 1 + const hasMultipleResults = result.results.length > 1 + const tabBar = hasMultipleTabs ? TAB_BAR_PX : 0 + + if (hasMultipleResults) { + const hasGrid = slots.some( + (slot) => slot.result && isDqlWithColumns(slot.result), + ) + if (!hasGrid) { return tabBar + NOTIFICATION_PX } return ( @@ -1420,14 +1435,19 @@ export const computeResultBottomHeight = ( ) } - // Single-statement: tight-fit up to 10 rows. - const only = result.results[0] + // Single executed result: tight-fit up to 10 rows. The tab bar is still + // included when the editor contributes additional "Not run" slots. + const only = frame?.slots[frame.activeSlotIndex]?.result ?? result.results[0] if (!only || !isDqlWithColumns(only)) { - return NOTIFICATION_PX + return tabBar + NOTIFICATION_PX } const rows = Math.min(MAX_RESERVED_ROWS, dqlRowCount(only)) return ( - NOTIFICATION_PX + RESULT_ACTIONS_BAR_PX + HEADER_HEIGHT + rows * ROW_HEIGHT + tabBar + + NOTIFICATION_PX + + RESULT_ACTIONS_BAR_PX + + HEADER_HEIGHT + + rows * ROW_HEIGHT ) } @@ -1438,7 +1458,7 @@ export const computeResultBottomHeight = ( export const defaultBottomHeightFor = (cell: NotebookCell): number => cell.mode === "draw" ? DEFAULT_CHART_BOTTOM_HEIGHT - : computeResultBottomHeight(cell.result) + : computeResultBottomHeight(cell.result, getQueriesFromText(cell.value)) // True iff this cell occupies vertical space for a bottom slot — i.e. its // total height includes bottomHeight. This includes the chart-expanded case @@ -1461,7 +1481,10 @@ export const modeChangeBottomHeightPatch = ( mode === "draw" ? DEFAULT_CHART_BOTTOM_HEIGHT : cell?.result - ? computeResultBottomHeight(cell.result) + ? computeResultBottomHeight( + cell.result, + getQueriesFromText(cell.value), + ) : undefined, } } @@ -1495,7 +1518,10 @@ export const patchCellRunResult = ( cell.mode !== "draw" && cell.type !== "markdown" ) { - next.bottomHeight = computeResultBottomHeight(result) + next.bottomHeight = computeResultBottomHeight( + result, + getQueriesFromText(cell.value), + ) } return next }) From 378636e5c0f613c3ae924d9937f6924a4fb356fc Mon Sep 17 00:00:00 2001 From: emrberk Date: Sun, 30 Aug 2026 11:24:06 +0300 Subject: [PATCH 05/12] remove for control from run with selection label --- src/components/EditorSettingsModal/index.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/components/EditorSettingsModal/index.tsx b/src/components/EditorSettingsModal/index.tsx index 601214950..78f0fc8c4 100644 --- a/src/components/EditorSettingsModal/index.tsx +++ b/src/components/EditorSettingsModal/index.tsx @@ -112,6 +112,7 @@ type SettingRowProps = { label: string description: string controlId: string + omitHtmlFor?: boolean children: ReactNode } @@ -121,6 +122,7 @@ const SettingRow = ({ label, description, controlId, + omitHtmlFor, children, }: SettingRowProps) => ( @@ -129,7 +131,7 @@ const SettingRow = ({ color="contentPrimary" lineHeight="1" type="label" - htmlFor={controlId} + htmlFor={omitHtmlFor !== true ? controlId : undefined} > {label} @@ -184,6 +186,7 @@ const EditorSettingsForm = ({ onClose }: { onClose: () => void }) => { label="Run with selection" description="Controls how run actions apply your text selection." controlId={RUN_WITH_SELECTION_ID} + omitHtmlFor > Date: Sun, 30 Aug 2026 12:45:31 +0300 Subject: [PATCH 06/12] address reviews --- e2e/commands.js | 56 +++++++++++++ e2e/tests/console/editor.spec.js | 51 +++++++---- src/scenes/Editor/Monaco/index.tsx | 4 +- src/scenes/Editor/Monaco/utils.test.ts | 84 ++++++++++++------- src/scenes/Editor/Monaco/utils.ts | 28 ++----- src/scenes/Editor/Notebook/cells/Cell.tsx | 2 - .../Notebook/cells/useCellRunActions.ts | 57 +++++++------ .../Editor/Notebook/notebookUtils.test.ts | 36 ++++---- src/scenes/Editor/Notebook/notebookUtils.ts | 14 ++-- 9 files changed, 215 insertions(+), 117 deletions(-) diff --git a/e2e/commands.js b/e2e/commands.js index f51ab8e31..fbc3ee031 100644 --- a/e2e/commands.js +++ b/e2e/commands.js @@ -264,6 +264,62 @@ Cypress.Commands.add("typeQueryDirectly", (query) => { }) }) +Cypress.Commands.add("waitForActiveBufferValue", (expectedValue) => { + cy.window().then((win) => { + const readActiveBufferValue = () => + new Cypress.Promise((resolve, reject) => { + const openRequest = win.indexedDB.open("web-console") + openRequest.onerror = () => reject(openRequest.error) + openRequest.onsuccess = () => { + const database = openRequest.result + const transaction = database.transaction( + ["editor_settings", "buffers"], + "readonly", + ) + const activeBufferRequest = transaction + .objectStore("editor_settings") + .index("key") + .get("activeBufferId") + + activeBufferRequest.onerror = () => { + database.close() + reject(activeBufferRequest.error) + } + activeBufferRequest.onsuccess = () => { + const bufferRequest = transaction + .objectStore("buffers") + .get(activeBufferRequest.result.value) + bufferRequest.onerror = () => { + database.close() + reject(bufferRequest.error) + } + bufferRequest.onsuccess = () => { + const value = bufferRequest.result?.value + database.close() + resolve(value) + } + } + } + }) + + const deadline = Date.now() + 10000 + const poll = () => + readActiveBufferValue().then((value) => { + if (value === expectedValue) return + if (Date.now() >= deadline) { + throw new Error( + `Active buffer did not persist ${JSON.stringify(expectedValue)}`, + ) + } + return new Cypress.Promise((resolve) => + win.setTimeout(resolve, 50), + ).then(poll) + }) + + return poll() + }) +}) + Cypress.Commands.add("runLine", () => { cy.intercept("/exec*").as("exec") cy.typeQuery(`${ctrlOrCmd}{enter}`) diff --git a/e2e/tests/console/editor.spec.js b/e2e/tests/console/editor.spec.js index a7d022326..9660a5096 100644 --- a/e2e/tests/console/editor.spec.js +++ b/e2e/tests/console/editor.spec.js @@ -2274,7 +2274,15 @@ describe("editor settings", () => { cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) const cellResultTabs = () => cy.get("[role='tablist'] [role='tab']") - // Given the first exec request is held until the loader is observed + // And a resolved notebook whose focused cell holds two statements + cy.createNotebook() + cy.focusNotebookCell() + cy.focused().type(sql, { delay: 0 }) + cy.withFocusedEditor((editor) => { + expect(editor.getValue()).to.eq(sql) + }) + + // Given the first cell exec request is held until the loader is observed let releaseFirstExec cy.intercept( { url: "/exec*", times: 1 }, @@ -2284,14 +2292,6 @@ describe("editor settings", () => { }), ).as("heldExec") - // And a resolved notebook whose focused cell holds two statements - cy.createNotebook() - cy.focusNotebookCell() - cy.focused().type(sql, { delay: 0 }) - cy.withFocusedEditor((editor) => { - expect(editor.getValue()).to.eq(sql) - }) - // When the mode is partial (default) and the table name is selected selectCellRange(tableStart, tableEnd) runCellAtCursor() @@ -2308,6 +2308,7 @@ describe("editor settings", () => { // And only the bare fragment ran: a single result with every column cy.get("[data-hook='grid-header-name']:visible").should("have.length", 3) cellResultTabs().should("not.exist") + cy.get(".selectionSuccessHighlight").should("exist") // When the mode is complete and the selection cuts into both statements setMode("complete") @@ -2370,8 +2371,10 @@ describe("editor settings", () => { ) const execQueries = [] + const cellQueries = new Set(["select 1", "select 2"]) cy.intercept({ url: "/exec*" }, (request) => { - execQueries.push(new URL(request.url).searchParams.get("query")) + const query = new URL(request.url).searchParams.get("query") + if (cellQueries.has(query)) execQueries.push(query) request.continue() }) @@ -2391,7 +2394,8 @@ describe("editor settings", () => { }) // And Cmd+Shift+Enter runs the whole cell even from the result area - cy.get("[role='tablist'] [role='tab']").eq(1).click().should("have.focus") + cy.get("[role='tablist'] [role='tab']").eq(1).click() + cy.get("[role='tablist'] [role='tab']").eq(1).should("have.focus") cy.then(() => { execQueries.length = 0 }) @@ -2421,20 +2425,30 @@ describe("editor settings", () => { ) const execQueries = [] + const cellQueries = new Set(["select 1", "select 2"]) cy.intercept({ url: "/exec*" }, (request) => { - execQueries.push(new URL(request.url).searchParams.get("query")) + const query = new URL(request.url).searchParams.get("query") + if (cellQueries.has(query)) execQueries.push(query) request.continue() }) + const expectNothingToRun = () => { + cy.contains(".Toastify__toast--error", "Nothing to run") + .should("be.visible") + .within(() => { + cy.get("button[aria-label='close']").click() + }) + cy.then(() => expect(execQueries).to.deep.eq([])) + } // When Cmd+Enter is pressed in Monaco with no query at the cursor cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) // Then it is a no-op, never an implicit Run All - cy.wait(500) - cy.then(() => expect(execQueries).to.deep.eq([])) + expectNothingToRun() cy.get("[data-hook='result-grid-tanstack']:visible").should("not.exist") // When only the second statement is run + cy.focusNotebookCell() cy.withFocusedEditor((editor) => editor.setPosition({ lineNumber: 3, column: 4 }), ) @@ -2468,9 +2482,9 @@ describe("editor settings", () => { }) // When the second result tab has focus and Cmd+Enter is pressed + cy.get("[role='tablist'] [role='tab']").eq(1).click() cy.get("[role='tablist'] [role='tab']") .eq(1) - .click() .should("have.focus") .and("have.attr", "title", "select 2") cy.window().then((win) => { @@ -2498,8 +2512,7 @@ describe("editor settings", () => { execQueries.length = 0 }) cy.withFocusedEditor((editor) => editor.getAction("notebook-run").run()) - cy.wait(500) - cy.then(() => expect(execQueries).to.deep.eq([])) + expectNothingToRun() }) it("isolates a shared fragment that only matches part of a query", () => { @@ -2511,7 +2524,9 @@ describe("editor settings", () => { // And a persisted buffer whose statement contains the shared fragment cy.typeQueryDirectly("select 1 union all select 2;") - cy.wait(1000) + cy.waitForActiveBufferValue("select 1 union all select 2;") + cy.reload() + cy.getEditorContent().should("have.value", "select 1 union all select 2;") // When a share link for the fragment auto-runs cy.visit( diff --git a/src/scenes/Editor/Monaco/index.tsx b/src/scenes/Editor/Monaco/index.tsx index 25650e26e..28a656a88 100644 --- a/src/scenes/Editor/Monaco/index.tsx +++ b/src/scenes/Editor/Monaco/index.tsx @@ -77,6 +77,7 @@ import { getQueryStartOffset, getQueriesToRun, getQueriesStartingFromLine, + getStatementOffsets, readShareLinkParams, clearShareLinkParams, buildShareLinkUrl, @@ -1273,8 +1274,9 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const trimmedQuery = query.trim() // Find if the query is already in the editor const matches = findMatches(model, trimmedQuery) + const statementOffsets = getStatementOffsets(editor) const fullQueryMatch = matches?.find((match) => - isFullQueryMatch(editor, match.range), + isFullQueryMatch(editor, match.range, statementOffsets), ) if (fullQueryMatch) { editor.setSelection(fullQueryMatch.range) diff --git a/src/scenes/Editor/Monaco/utils.test.ts b/src/scenes/Editor/Monaco/utils.test.ts index 1d02e0689..b390ff03a 100644 --- a/src/scenes/Editor/Monaco/utils.test.ts +++ b/src/scenes/Editor/Monaco/utils.test.ts @@ -27,6 +27,7 @@ const makeSingleLineEditor = ( selection: SingleLineSelection | null, ) => { let currentSelection = selection + let currentCursorColumn = cursorColumn const toSelection = (sel: SingleLineSelection) => ({ startLineNumber: 1, @@ -50,7 +51,7 @@ const makeSingleLineEditor = ( return { getModel: () => model, getValue: () => text, - getPosition: () => ({ lineNumber: 1, column: cursorColumn }), + getPosition: () => ({ lineNumber: 1, column: currentCursorColumn }), getSelection: () => currentSelection ? toSelection(currentSelection) : null, setSelection: (range: { startColumn: number; endColumn: number }) => { @@ -58,6 +59,7 @@ const makeSingleLineEditor = ( startColumn: range.startColumn, endColumn: range.endColumn, } + currentCursorColumn = range.endColumn }, } as unknown as editor.IStandaloneCodeEditor } @@ -577,9 +579,7 @@ describe("run with selection modes", () => { // off falls back to the query at the cursor as always expect(complete).toEqual([]) expect(partial).toEqual([]) - expect(off.map((request) => request.query)).toEqual([ - "DROP TABLE trades", - ]) + expect(off.map((request) => request.query)).toEqual(["DROP TABLE trades"]) }) }) @@ -611,6 +611,18 @@ describe("run with selection modes", () => { expect(request?.selection).toBeUndefined() }) + it("keeps a selection ending in a comment attached to the preceding statement", () => { + const editor = makeSingleLineEditor("select 1; /* note */ select 2;", 3, { + startColumn: 1, + endColumn: 21, + }) + + const request = getQueryRequestFromEditor(editor, "complete") + + expect(request?.query).toBe("select 1") + expect(request?.selection).toBeUndefined() + }) + it("ignores the selection and uses the cursor query when off", () => { // Given the first statement is selected but the mode is off const editor = makeSingleLineEditor(TEXT, 3, FIRST_STATEMENT_SELECTION) @@ -742,12 +754,16 @@ describe("statement boundaries", () => { const editor = makeSingleLineEditor(text, startColumn, null) expect( - isFullQueryMatch(editor, { - startLineNumber: 1, - startColumn, - endLineNumber: 1, - endColumn: text.length + 1, - }), + isFullQueryMatch( + editor, + { + startLineNumber: 1, + startColumn, + endLineNumber: 1, + endColumn: text.length + 1, + }, + getStatementOffsets(editor), + ), ).toBe(true) }) @@ -756,12 +772,16 @@ describe("statement boundaries", () => { const editor = makeMultiLineEditor(text) expect( - isFullQueryMatch(editor, { - startLineNumber: 1, - startColumn: 1, - endLineNumber: 2, - endColumn: 15, - }), + isFullQueryMatch( + editor, + { + startLineNumber: 1, + startColumn: 1, + endLineNumber: 2, + endColumn: 15, + }, + getStatementOffsets(editor), + ), ).toBe(true) }) @@ -770,12 +790,16 @@ describe("statement boundaries", () => { const editor = makeMultiLineEditor(text) expect( - isFullQueryMatch(editor, { - startLineNumber: 1, - startColumn: 1, - endLineNumber: 3, - endColumn: 10, - }), + isFullQueryMatch( + editor, + { + startLineNumber: 1, + startColumn: 1, + endLineNumber: 3, + endColumn: 10, + }, + getStatementOffsets(editor), + ), ).toBe(true) }) @@ -785,12 +809,16 @@ describe("statement boundaries", () => { const editor = makeSingleLineEditor(text, startColumn, null) expect( - isFullQueryMatch(editor, { - startLineNumber: 1, - startColumn, - endLineNumber: 1, - endColumn: text.length + 1, - }), + isFullQueryMatch( + editor, + { + startLineNumber: 1, + startColumn, + endLineNumber: 1, + endColumn: text.length + 1, + }, + getStatementOffsets(editor), + ), ).toBe(false) }) diff --git a/src/scenes/Editor/Monaco/utils.ts b/src/scenes/Editor/Monaco/utils.ts index 5f627a743..83f3aed08 100644 --- a/src/scenes/Editor/Monaco/utils.ts +++ b/src/scenes/Editor/Monaco/utils.ts @@ -764,10 +764,10 @@ export const getQueryRequestFromEditor = ( : undefined if (selectionMode !== "off" && strippedNormalizedSelectedText) { - request = getQueryFromSelection(editor) - if (selectionMode === "complete" && request) { - request = toCompleteQueryRequest(request) - } + request = + selectionMode === "complete" + ? getQueriesToRun(editor, getStatementOffsets(editor), "complete")[0] + : getQueryFromSelection(editor) } else { request = getQueryFromCursor(editor) } @@ -1051,6 +1051,7 @@ export const findMatches = (model: editor.ITextModel, needle: string) => export const isFullQueryMatch = ( editor: IStandaloneCodeEditor, range: IRange, + statementOffsets: { startOffset: number; endOffset: number }[], ): boolean => { const model = editor.getModel() if (!model) return false @@ -1063,21 +1064,10 @@ export const isFullQueryMatch = ( lineNumber: range.endLineNumber, column: range.endColumn, }) - const queryRanges = getAllQueries(editor) - .map((query) => ({ - startOffset: model.getOffsetAt({ - lineNumber: query.row + 1, - column: query.column, - }), - endOffset: model.getOffsetAt({ - lineNumber: query.endRow + 1, - column: query.endColumn, - }), - })) - .filter( - ({ startOffset, endOffset }) => - startOffset < matchEndOffset && endOffset > matchStartOffset, - ) + const queryRanges = statementOffsets.filter( + ({ startOffset, endOffset }) => + startOffset < matchEndOffset && endOffset > matchStartOffset, + ) if (queryRanges.length === 0) return false diff --git a/src/scenes/Editor/Notebook/cells/Cell.tsx b/src/scenes/Editor/Notebook/cells/Cell.tsx index 3abbe7e22..b5ae87894 100644 --- a/src/scenes/Editor/Notebook/cells/Cell.tsx +++ b/src/scenes/Editor/Notebook/cells/Cell.tsx @@ -382,8 +382,6 @@ const CellInner: React.FC = ({ runAll() return } - // Outside Monaco, a single-query shortcut has a target only while focus - // is in the result area. Other cell chrome never widens Cmd+Enter. if (resultRef.current?.contains(document.activeElement)) { e.preventDefault() runSingleFromResult() diff --git a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts index f8c717089..b30560e17 100644 --- a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts +++ b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts @@ -34,6 +34,7 @@ type Options = { } type SingleRunSource = "editor" | "result" +type SelectionRunStart = "no-selection" | "no-query" | "started" type RunRequest = { kind: "all" } | { kind: "single"; source: SingleRunSource } @@ -116,18 +117,19 @@ export const useCellRunActions = ({ validateWithGlobals, ]) - const tryRunSelection = useCallback(async (): Promise => { + const tryRunSelection = useCallback((): SelectionRunStart => { const ed = editorRef.current - if (!ed) return false + if (!ed) return "no-selection" const resolution = resolveSelectionRun(ed, runWithSelectionMode) - if (resolution.kind === "no-selection") return false + if (resolution.kind === "no-selection") return "no-selection" clearHighlight() - if (resolution.kind === "no-query") return true + if (resolution.kind === "no-query") return "no-query" void trackEvent(ConsoleEvent.NOTEBOOK_CELL_RUN) - const { ok } = await runCell(cell.id, resolution.sql) - if (runWithSelectionMode === "partial") applyHighlight(ok) - return true + void runCell(cell.id, resolution.sql).then(({ ok }) => { + if (runWithSelectionMode === "partial") applyHighlight(ok) + }) + return "started" }, [ runWithSelectionMode, cell.id, @@ -169,32 +171,36 @@ export const useCellRunActions = ({ ]) const handleRunSingle = useCallback( - async (source: SingleRunSource) => { + (source: SingleRunSource): boolean => { let sql: string | undefined if (source === "editor") { const ed = editorRef.current - if (!ed) return - // Capture the cursor's statement before any await — revealing a compact - // cell can unmount Monaco during this same gesture. + if (!ed) return false const cursorQuery = getQueryFromCursor(ed)?.query - if (await tryRunSelection()) return + const selectionRun = tryRunSelection() + if (selectionRun === "started") return true + if (selectionRun === "no-query") { + toast.error("Nothing to run") + return false + } sql = cursorQuery } else { - // Result focus deliberately targets the active statement tab, including - // a "Not run" tab. It is not a fallback for an unresolved editor cursor. sql = resolveActiveStatementSql(cell.value, cell.result) } if (!sql?.trim()) { - return + toast.error("Nothing to run") + return false } clearHighlight() const priorResult = getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null - const { ok } = await runCell(cell.id, normalizeQueryText(sql)) - const freshResult = - getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null - emitRanEvent(createRunStatus(priorResult, freshResult, ok)) + void runCell(cell.id, normalizeQueryText(sql)).then(({ ok }) => { + const freshResult = + getCellsSnapshot().find((c) => c.id === cell.id)?.result ?? null + emitRanEvent(createRunStatus(priorResult, freshResult, ok)) + }) + return true }, [ cell.id, @@ -229,12 +235,13 @@ export const useCellRunActions = ({ setCellMode(cell.id, "run") } firstRunRef.current = cell.result == null - // Start the run before revealing: under React 17 a reveal fired from a - // native key event re-renders synchronously and unmounts the editor, so - // the run must read the cursor first. - if (plan.kind === "run-all") void handleRunAll() - else if (request.kind === "single") void handleRunSingle(request.source) - if (plan.reveal) setCellViewMaximized(cell.id, true) + if (plan.kind === "run-all") { + void handleRunAll() + if (plan.reveal) setCellViewMaximized(cell.id, true) + } else if (request.kind === "single") { + const started = handleRunSingle(request.source) + if (started && plan.reveal) setCellViewMaximized(cell.id, true) + } }, [ cell.id, diff --git a/src/scenes/Editor/Notebook/notebookUtils.test.ts b/src/scenes/Editor/Notebook/notebookUtils.test.ts index 9abd5c9ab..40b16a718 100644 --- a/src/scenes/Editor/Notebook/notebookUtils.test.ts +++ b/src/scenes/Editor/Notebook/notebookUtils.test.ts @@ -1358,11 +1358,17 @@ describe("computeResultBottomHeight", () => { // ROW_HEIGHT = 30 // MAX_RESERVED_ROWS = 10 + const heightForResult = (result: CellResult | null | undefined) => + computeResultBottomHeight( + result, + result?.results.map(({ query }) => query) ?? [], + ) + it("null/undefined/empty result → notification-only", () => { - expect(computeResultBottomHeight(null)).toBe(44) - expect(computeResultBottomHeight(undefined)).toBe(44) + expect(heightForResult(null)).toBe(44) + expect(heightForResult(undefined)).toBe(44) expect( - computeResultBottomHeight({ + heightForResult({ results: [], activeResultIndex: 0, timestamp: 0, @@ -1372,7 +1378,7 @@ describe("computeResultBottomHeight", () => { it("single error → notification-only", () => { expect( - computeResultBottomHeight({ + heightForResult({ results: [{ type: "error", query: "X", error: "boom" }], activeResultIndex: 0, timestamp: 0, @@ -1383,7 +1389,7 @@ describe("computeResultBottomHeight", () => { it("single DDL/DML/notice → notification-only", () => { for (const type of ["ddl", "dml"] as const) { expect( - computeResultBottomHeight({ + heightForResult({ results: [{ type, query: "X" }], activeResultIndex: 0, timestamp: 0, @@ -1395,7 +1401,7 @@ describe("computeResultBottomHeight", () => { it("single DQL with columns but 0 rows → notification + actions bar + header (no rows)", () => { // The column headers show even with no rows: 44 + 36 + 44 + 0*30 = 124. expect( - computeResultBottomHeight({ + heightForResult({ results: [ { type: "dql", @@ -1413,7 +1419,7 @@ describe("computeResultBottomHeight", () => { it("single DQL with no columns → notification-only", () => { expect( - computeResultBottomHeight({ + heightForResult({ results: [ { type: "dql", @@ -1444,13 +1450,13 @@ describe("computeResultBottomHeight", () => { timestamp: 0, }) // 1 row: 44 + 36 + 44 + 1*30 = 154 - expect(computeResultBottomHeight(make(1))).toBe(154) + expect(heightForResult(make(1))).toBe(154) // 5 rows: 44 + 36 + 44 + 5*30 = 274 - expect(computeResultBottomHeight(make(5))).toBe(274) + expect(heightForResult(make(5))).toBe(274) // 10 rows: 44 + 36 + 44 + 10*30 = 424 - expect(computeResultBottomHeight(make(10))).toBe(424) + expect(heightForResult(make(10))).toBe(424) // 50 rows: cap at 10 → still 424 - expect(computeResultBottomHeight(make(50))).toBe(424) + expect(heightForResult(make(50))).toBe(424) }) it("one executed DQL in a multi-statement cell adds tabs but tight-fits its rows", () => { @@ -1492,7 +1498,7 @@ describe("computeResultBottomHeight", () => { it("multi-statement, first DQL with rows → tab + notification + header + 10 rows", () => { // 40 + 44 + 36 + 44 + 10*30 = 464 expect( - computeResultBottomHeight({ + heightForResult({ results: [ { type: "dql", @@ -1513,7 +1519,7 @@ describe("computeResultBottomHeight", () => { // The first tab is an error, but the active second tab renders a grid. // Reserve the same stable multi-tab height: 40 + 44 + 36 + 44 + 10*30. expect( - computeResultBottomHeight({ + heightForResult({ results: [ { type: "error", query: "Q1", error: "boom" }, { @@ -1534,7 +1540,7 @@ describe("computeResultBottomHeight", () => { // The first query shows its column headers, so we reserve the grid block // like any DQL-first script: 40 + 44 + 36 + 44 + 10*30 = 464. expect( - computeResultBottomHeight({ + heightForResult({ results: [ { type: "dql", @@ -2114,7 +2120,7 @@ describe("releaseCellResultPatch", () => { expect(patch).toEqual({ result: undefined, lastRunStatus: "success", - bottomHeight: computeResultBottomHeight(threeRowResult), + bottomHeight: computeResultBottomHeight(threeRowResult, ["select 1"]), }) }) diff --git a/src/scenes/Editor/Notebook/notebookUtils.ts b/src/scenes/Editor/Notebook/notebookUtils.ts index ded1de253..f83b00ba9 100644 --- a/src/scenes/Editor/Notebook/notebookUtils.ts +++ b/src/scenes/Editor/Notebook/notebookUtils.ts @@ -1404,17 +1404,13 @@ const dqlRowCount = (r: SingleQueryResult): number => // A single executed result still tight-fits its own row count. export const computeResultBottomHeight = ( result: CellResult | null | undefined, - statements?: string[], + statements: string[], ): number => { if (!result || result.results.length === 0) return NOTIFICATION_PX const frame = - (statements - ? deriveStatementFrame(statements, result) - : deriveStatementFrame( - result.results.map((r) => r.query), - result, - )) ?? derivePositionalFrame(result) - const slots = frame?.slots ?? [] + deriveStatementFrame(statements, result) ?? derivePositionalFrame(result) + if (!frame) return NOTIFICATION_PX + const slots = frame.slots const hasMultipleTabs = slots.length > 1 const hasMultipleResults = result.results.length > 1 const tabBar = hasMultipleTabs ? TAB_BAR_PX : 0 @@ -1437,7 +1433,7 @@ export const computeResultBottomHeight = ( // Single executed result: tight-fit up to 10 rows. The tab bar is still // included when the editor contributes additional "Not run" slots. - const only = frame?.slots[frame.activeSlotIndex]?.result ?? result.results[0] + const only = frame.slots[frame.activeSlotIndex]?.result ?? result.results[0] if (!only || !isDqlWithColumns(only)) { return tabBar + NOTIFICATION_PX } From ea812064df64ef2ad02f4e775d0a73a557073fe8 Mon Sep 17 00:00:00 2001 From: emrberk Date: Sun, 30 Aug 2026 14:54:16 +0300 Subject: [PATCH 07/12] reviews --- e2e/commands.js | 68 ++++++++++++------- e2e/tests/console/editor.spec.js | 33 +++++++++ src/components/EditorSettingsModal/index.tsx | 2 +- src/scenes/Editor/Monaco/index.tsx | 2 + src/scenes/Editor/Monaco/utils.test.ts | 54 +++++++++++++++ src/scenes/Editor/Monaco/utils.ts | 27 +++++--- .../Editor/Notebook/NotebookProvider.tsx | 5 +- .../Notebook/cells/useCellRunActions.ts | 23 ++++--- .../Editor/Notebook/notebookUtils.test.ts | 34 ++++++++-- src/scenes/Editor/Notebook/notebookUtils.ts | 26 +++---- 10 files changed, 201 insertions(+), 73 deletions(-) diff --git a/e2e/commands.js b/e2e/commands.js index fbc3ee031..905bbf2bf 100644 --- a/e2e/commands.js +++ b/e2e/commands.js @@ -270,45 +270,63 @@ Cypress.Commands.add("waitForActiveBufferValue", (expectedValue) => { new Cypress.Promise((resolve, reject) => { const openRequest = win.indexedDB.open("web-console") openRequest.onerror = () => reject(openRequest.error) + openRequest.onblocked = () => + reject(new Error("web-console IndexedDB open is blocked")) openRequest.onsuccess = () => { const database = openRequest.result - const transaction = database.transaction( - ["editor_settings", "buffers"], - "readonly", - ) - const activeBufferRequest = transaction - .objectStore("editor_settings") - .index("key") - .get("activeBufferId") - - activeBufferRequest.onerror = () => { + const fail = (error) => { database.close() - reject(activeBufferRequest.error) + reject(error) } - activeBufferRequest.onsuccess = () => { - const bufferRequest = transaction - .objectStore("buffers") - .get(activeBufferRequest.result.value) - bufferRequest.onerror = () => { - database.close() - reject(bufferRequest.error) - } - bufferRequest.onsuccess = () => { - const value = bufferRequest.result?.value - database.close() - resolve(value) + try { + const transaction = database.transaction( + ["editor_settings", "buffers"], + "readonly", + ) + transaction.onerror = () => fail(transaction.error) + const activeBufferRequest = transaction + .objectStore("editor_settings") + .index("key") + .get("activeBufferId") + + activeBufferRequest.onerror = () => fail(activeBufferRequest.error) + activeBufferRequest.onsuccess = () => { + try { + const activeBufferId = activeBufferRequest.result?.value + if (activeBufferId === undefined) { + fail(new Error("no activeBufferId row in editor_settings")) + return + } + const bufferRequest = transaction + .objectStore("buffers") + .get(activeBufferId) + bufferRequest.onerror = () => fail(bufferRequest.error) + bufferRequest.onsuccess = () => { + const value = bufferRequest.result?.value + database.close() + resolve(value) + } + } catch (error) { + fail(error) + } } + } catch (error) { + fail(error) } } }) - const deadline = Date.now() + 10000 + // Poll well inside Cypress's own command timeout so this command's + // message wins over a generic cy.then() timeout. + const deadline = Date.now() + 8000 const poll = () => readActiveBufferValue().then((value) => { if (value === expectedValue) return if (Date.now() >= deadline) { throw new Error( - `Active buffer did not persist ${JSON.stringify(expectedValue)}`, + `Active buffer did not persist ${JSON.stringify( + expectedValue, + )} — last read ${JSON.stringify(value)}`, ) } return new Cypress.Promise((resolve) => diff --git a/e2e/tests/console/editor.spec.js b/e2e/tests/console/editor.spec.js index 9660a5096..d997fc997 100644 --- a/e2e/tests/console/editor.spec.js +++ b/e2e/tests/console/editor.spec.js @@ -2230,6 +2230,39 @@ describe("editor settings", () => { cy.getGridRow(0).should("contain", "2") }) + it("runs the glyph's own query in complete mode while a selection spans both statements", () => { + // Given the mode is set to complete + openEditorSettings() + cy.getByDataHook("editor-settings-run-with-selection").click() + cy.getByDataHook("run-with-selection-complete").click() + cy.getByDataHook("editor-settings-save").click() + + const execQueries = [] + const statements = new Set(["select 1", "select 2"]) + cy.intercept({ url: "/exec*" }, (request) => { + const query = new URL(request.url).searchParams.get("query") + if (statements.has(query)) execQueries.push(query) + request.continue() + }) + + // And two statements with a selection whose caret ends inside the second + cy.typeQueryDirectly("select 1;\nselect 2;") + cy.selectRange({ lineNumber: 1, column: 8 }, { lineNumber: 2, column: 9 }) + cy.getByDataHook("button-run-query").should( + "contain", + "Run 2 selected queries", + ) + + // When the second statement's run glyph is clicked + cy.getRunIconInLine(2).click({ force: true }) + + // Then that statement runs, never the first one the selection reaches back into + cy.wrap(null).should(() => { + expect(execQueries).to.deep.eq(["select 2"]) + }) + cy.getGridRow(0).should("contain", "2") + }) + it("disables run while the selection covers only a comment between statements", () => { // Given a commented-out statement between two live statements cy.typeQueryDirectly("select 1;\n-- old: delete from t\nselect 2;") diff --git a/src/components/EditorSettingsModal/index.tsx b/src/components/EditorSettingsModal/index.tsx index 78f0fc8c4..d7ef08cf4 100644 --- a/src/components/EditorSettingsModal/index.tsx +++ b/src/components/EditorSettingsModal/index.tsx @@ -131,7 +131,7 @@ const SettingRow = ({ color="contentPrimary" lineHeight="1" type="label" - htmlFor={omitHtmlFor !== true ? controlId : undefined} + htmlFor={!omitHtmlFor ? controlId : undefined} > {label} diff --git a/src/scenes/Editor/Monaco/index.tsx b/src/scenes/Editor/Monaco/index.tsx index 28a656a88..e1e4fa48d 100644 --- a/src/scenes/Editor/Monaco/index.tsx +++ b/src/scenes/Editor/Monaco/index.tsx @@ -459,9 +459,11 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { endColumn: endPosition.column, }) } else { + const selection = editor.getSelection() const queryInCursor = getQueryFromCursor(editor) if ( queryInCursor && + (!selection || selection.isEmpty()) && createQueryKeyFromRequest(editor, queryInCursor) === createQueryKeyFromRequest(editor, query) ) { diff --git a/src/scenes/Editor/Monaco/utils.test.ts b/src/scenes/Editor/Monaco/utils.test.ts index b390ff03a..ffa0806ca 100644 --- a/src/scenes/Editor/Monaco/utils.test.ts +++ b/src/scenes/Editor/Monaco/utils.test.ts @@ -537,6 +537,24 @@ describe("run with selection modes", () => { expect(result.every((request) => !request.selection)).toBe(true) }) + it("falls back to the cursor query when the selection is only whitespace", () => { + // Given a selection covering nothing but a space inside the statement + const editor = makeSingleLineEditor(TEXT, 3, { + startColumn: 7, + endColumn: 8, + }) + + // When resolving the queries to run in each honouring mode + // Then the selection carries no statement content, so the run stays + // enabled on the cursor query instead of resolving to nothing + expect(getQueriesToRun(editor, QUERY_OFFSETS, "partial")).toEqual([ + getQueryFromCursor(editor), + ]) + expect(getQueriesToRun(editor, QUERY_OFFSETS, "complete")).toEqual([ + getQueryFromCursor(editor), + ]) + }) + it("keeps off and complete distinct for a cross-statement selection", () => { // Given a valid selection that starts at the cursor and crosses into the // second statement @@ -697,6 +715,42 @@ describe("resolveSelectionRun", () => { }) }) + it("reports no selection when only whitespace is selected", () => { + // Given a selection covering the indentation of a statement, with the + // cursor still inside that statement + const text = "SELECT\n 1\nFROM long_sequence(1)" + const editor = makeMultiLineEditor(text, { + startLineNumber: 2, + startColumn: 1, + endLineNumber: 2, + endColumn: 4, + }) + + // When resolving the selection run in both honouring modes + // Then the selection is treated as absent, so the caller falls back to the + // cursor statement rather than refusing the run + expect(resolveSelectionRun(editor, "partial")).toEqual({ + kind: "no-selection", + }) + expect(resolveSelectionRun(editor, "complete")).toEqual({ + kind: "no-selection", + }) + }) + + it("reports no selection when only a statement separator is selected", () => { + // Given a selection covering just the trailing semicolon + const editor = makeSingleLineEditor(TEXT, 3, { + startColumn: 10, + endColumn: 11, + }) + + // When resolving the selection run + // Then it carries no statement content, so it is not a selection run + expect(resolveSelectionRun(editor, "partial")).toEqual({ + kind: "no-selection", + }) + }) + it("expands a cross-statement fragment in complete mode", () => { // Given a selection cutting into both statements in complete mode const editor = makeSingleLineEditor(TEXT, 3, { diff --git a/src/scenes/Editor/Monaco/utils.ts b/src/scenes/Editor/Monaco/utils.ts index 83f3aed08..11f309fbd 100644 --- a/src/scenes/Editor/Monaco/utils.ts +++ b/src/scenes/Editor/Monaco/utils.ts @@ -88,11 +88,6 @@ export type Request = Readonly<{ } }> -const toCompleteQueryRequest = (request: Request): Request => { - const { selection: _selection, ...completeQueryRequest } = request - return completeQueryRequest -} - type SqlTextItem = { row: number col: number @@ -151,7 +146,15 @@ export const getQueriesToRun = ( const selection = editor.getSelection() const selectedText = selection ? model.getValueInRange(selection) : undefined - if (selectionMode === "off" || !selection || !selectedText) { + const normalizedSelectedText = selectedText + ? normalizeQueryText(selectedText) + : "" + if ( + selectionMode === "off" || + !selection || + !selectedText || + !normalizedSelectedText + ) { const queryInCursor = getQueryFromCursor(editor) if (queryInCursor) { return [queryInCursor] @@ -161,8 +164,6 @@ export const getQueriesToRun = ( let selectionStartOffset = model.getOffsetAt(selection.getStartPosition()) let selectionEndOffset = model.getOffsetAt(selection.getEndPosition()) - const normalizedSelectedText = normalizeQueryText(selectedText) - if (stripSQLComments(normalizedSelectedText).length > 0) { selectionStartOffset += selectedText.indexOf(normalizedSelectedText) selectionEndOffset = selectionStartOffset + normalizedSelectedText.length @@ -206,7 +207,7 @@ export const getQueriesToRun = ( return undefined } return selectionMode === "complete" - ? toCompleteQueryRequest(query) + ? query : { ...query, selection: { @@ -1026,6 +1027,9 @@ export const resolveSelectionRun = ( return { kind: "no-selection" } } + const selectedText = normalizeQueryText(model.getValueInRange(selection)) + if (!selectedText) return { kind: "no-selection" } + if (selectionMode === "complete") { const queries = getQueriesToRun( editor, @@ -1041,8 +1045,9 @@ export const resolveSelectionRun = ( } } - const sql = normalizeQueryText(model.getValueInRange(selection)) - return stripSQLComments(sql) ? { kind: "run", sql } : { kind: "no-query" } + return stripSQLComments(selectedText) + ? { kind: "run", sql: selectedText } + : { kind: "no-query" } } export const findMatches = (model: editor.ITextModel, needle: string) => diff --git a/src/scenes/Editor/Notebook/NotebookProvider.tsx b/src/scenes/Editor/Notebook/NotebookProvider.tsx index 9daeee7ab..7ef84f11c 100644 --- a/src/scenes/Editor/Notebook/NotebookProvider.tsx +++ b/src/scenes/Editor/Notebook/NotebookProvider.tsx @@ -621,10 +621,7 @@ export const NotebookProvider: React.FC<{ !cell.bottomResized ) { store.updateCell(cellId, { - bottomHeight: computeResultBottomHeight( - cell.result, - getQueriesFromText(cell.value), - ), + bottomHeight: computeResultBottomHeight(cell.result, cell.value), }) } diff --git a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts index b30560e17..06085116b 100644 --- a/src/scenes/Editor/Notebook/cells/useCellRunActions.ts +++ b/src/scenes/Editor/Notebook/cells/useCellRunActions.ts @@ -9,6 +9,7 @@ import { getQueryFromCursor, normalizeQueryText, resolveSelectionRun, + type SelectionRunResolution, } from "../../Monaco/utils" import { resolveActiveStatementSql, resolveRunAction } from "../notebookUtils" import { @@ -34,7 +35,6 @@ type Options = { } type SingleRunSource = "editor" | "result" -type SelectionRunStart = "no-selection" | "no-query" | "started" type RunRequest = { kind: "all" } | { kind: "single"; source: SingleRunSource } @@ -117,7 +117,7 @@ export const useCellRunActions = ({ validateWithGlobals, ]) - const tryRunSelection = useCallback((): SelectionRunStart => { + const tryRunSelection = useCallback((): SelectionRunResolution["kind"] => { const ed = editorRef.current if (!ed) return "no-selection" const resolution = resolveSelectionRun(ed, runWithSelectionMode) @@ -129,7 +129,7 @@ export const useCellRunActions = ({ void runCell(cell.id, resolution.sql).then(({ ok }) => { if (runWithSelectionMode === "partial") applyHighlight(ok) }) - return "started" + return "run" }, [ runWithSelectionMode, cell.id, @@ -178,7 +178,7 @@ export const useCellRunActions = ({ if (!ed) return false const cursorQuery = getQueryFromCursor(ed)?.query const selectionRun = tryRunSelection() - if (selectionRun === "started") return true + if (selectionRun === "run") return true if (selectionRun === "no-query") { toast.error("Nothing to run") return false @@ -230,18 +230,23 @@ export const useCellRunActions = ({ }) return } - if (plan.exitDraw) { + const exitDrawMode = () => { + if (!plan.exitDraw) return signalUserEdit(bufferIdForEvents) setCellMode(cell.id, "run") } firstRunRef.current = cell.result == null - if (plan.kind === "run-all") { + if (request.kind === "all") { + exitDrawMode() void handleRunAll() if (plan.reveal) setCellViewMaximized(cell.id, true) - } else if (request.kind === "single") { - const started = handleRunSingle(request.source) - if (started && plan.reveal) setCellViewMaximized(cell.id, true) + return } + // A single run that finds nothing to run leaves the cell untouched — + // a draw cell keeps its chart instead of dropping to the grid. + if (!handleRunSingle(request.source)) return + exitDrawMode() + if (plan.reveal) setCellViewMaximized(cell.id, true) }, [ cell.id, diff --git a/src/scenes/Editor/Notebook/notebookUtils.test.ts b/src/scenes/Editor/Notebook/notebookUtils.test.ts index 40b16a718..9ac85852d 100644 --- a/src/scenes/Editor/Notebook/notebookUtils.test.ts +++ b/src/scenes/Editor/Notebook/notebookUtils.test.ts @@ -1361,7 +1361,7 @@ describe("computeResultBottomHeight", () => { const heightForResult = (result: CellResult | null | undefined) => computeResultBottomHeight( result, - result?.results.map(({ query }) => query) ?? [], + (result?.results ?? []).map(({ query }) => query).join(";\n"), ) it("null/undefined/empty result → notification-only", () => { @@ -1477,9 +1477,31 @@ describe("computeResultBottomHeight", () => { } // 40 tab + 44 notification + 36 actions + 44 header + 1*30 row = 194. - expect(computeResultBottomHeight(result, ["select 1", "select 2"])).toBe( - 194, - ) + expect(computeResultBottomHeight(result, "select 1;\nselect 2")).toBe(194) + }) + + it("keeps the grid block reserved while the active tab is an unexecuted statement", () => { + // Given a two-statement cell where only the second statement ran, and the + // user has selected the "Not run" tab, so the active slot holds no result + const result = { + results: [ + { + type: "dql" as const, + query: "select 2", + columns: [{ name: "2", type: "INT" }], + dataset: [[2]], + count: 1, + }, + ], + activeResultIndex: 0, + activeStatementKey: statementKeysFor(["select 1"])[0], + timestamp: 0, + } + + // When sizing the bottom slot for that frame + // Then it still reserves the executed result's grid rather than collapsing + // to tab bar + notification (84), which would clip the visible grid. + expect(computeResultBottomHeight(result, "select 1;\nselect 2")).toBe(194) }) it("one executed non-grid result in a multi-statement cell still includes its tabs", () => { @@ -1490,7 +1512,7 @@ describe("computeResultBottomHeight", () => { activeResultIndex: 0, timestamp: 0, }, - ["create table x (n int)", "select * from x"], + "create table x (n int);\nselect * from x", ), ).toBe(84) }) @@ -2120,7 +2142,7 @@ describe("releaseCellResultPatch", () => { expect(patch).toEqual({ result: undefined, lastRunStatus: "success", - bottomHeight: computeResultBottomHeight(threeRowResult, ["select 1"]), + bottomHeight: computeResultBottomHeight(threeRowResult, "select 1"), }) }) diff --git a/src/scenes/Editor/Notebook/notebookUtils.ts b/src/scenes/Editor/Notebook/notebookUtils.ts index f83b00ba9..2ab42a702 100644 --- a/src/scenes/Editor/Notebook/notebookUtils.ts +++ b/src/scenes/Editor/Notebook/notebookUtils.ts @@ -1371,10 +1371,7 @@ export const releaseCellResultPatch = ( lastRunError: carriedRunError(cell), ...(cell.mode !== "draw" && cell.bottomHeight == null && cell.result != null ? { - bottomHeight: computeResultBottomHeight( - cell.result, - getQueriesFromText(cell.value), - ), + bottomHeight: computeResultBottomHeight(cell.result, cell.value), } : {}), }) @@ -1386,9 +1383,9 @@ const dqlRowCount = (r: SingleQueryResult): number => r.type === "dql" ? r.dataset.length : 0 // Computes the bottom slot height for the same statement frame rendered by -// InlineResultTable. `statements` is the editor's current statement list; a -// partial run can have one result while still rendering multiple statement -// tabs (the unexecuted statements appear as "Not run"). +// InlineResultTable. `value` is the cell's current SQL; a partial run can have +// one result while still rendering multiple statement tabs (the unexecuted +// statements appear as "Not run"). // // Rules: // 1. Single-statement, no grid (error / DDL / DML / notice): just the @@ -1404,9 +1401,10 @@ const dqlRowCount = (r: SingleQueryResult): number => // A single executed result still tight-fits its own row count. export const computeResultBottomHeight = ( result: CellResult | null | undefined, - statements: string[], + value: string, ): number => { if (!result || result.results.length === 0) return NOTIFICATION_PX + const statements = getQueriesFromText(value) const frame = deriveStatementFrame(statements, result) ?? derivePositionalFrame(result) if (!frame) return NOTIFICATION_PX @@ -1454,7 +1452,7 @@ export const computeResultBottomHeight = ( export const defaultBottomHeightFor = (cell: NotebookCell): number => cell.mode === "draw" ? DEFAULT_CHART_BOTTOM_HEIGHT - : computeResultBottomHeight(cell.result, getQueriesFromText(cell.value)) + : computeResultBottomHeight(cell.result, cell.value) // True iff this cell occupies vertical space for a bottom slot — i.e. its // total height includes bottomHeight. This includes the chart-expanded case @@ -1477,10 +1475,7 @@ export const modeChangeBottomHeightPatch = ( mode === "draw" ? DEFAULT_CHART_BOTTOM_HEIGHT : cell?.result - ? computeResultBottomHeight( - cell.result, - getQueriesFromText(cell.value), - ) + ? computeResultBottomHeight(cell.result, cell.value) : undefined, } } @@ -1514,10 +1509,7 @@ export const patchCellRunResult = ( cell.mode !== "draw" && cell.type !== "markdown" ) { - next.bottomHeight = computeResultBottomHeight( - result, - getQueriesFromText(cell.value), - ) + next.bottomHeight = computeResultBottomHeight(result, cell.value) } return next }) From cda7c901b6e1ead551eb0005e656f89cc22569e5 Mon Sep 17 00:00:00 2001 From: emrberk Date: Mon, 31 Aug 2026 09:40:22 +0300 Subject: [PATCH 08/12] preserve selection on query run in off mode --- src/scenes/Editor/Monaco/index.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/scenes/Editor/Monaco/index.tsx b/src/scenes/Editor/Monaco/index.tsx index e1e4fa48d..82d0b5b0f 100644 --- a/src/scenes/Editor/Monaco/index.tsx +++ b/src/scenes/Editor/Monaco/index.tsx @@ -463,7 +463,9 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const queryInCursor = getQueryFromCursor(editor) if ( queryInCursor && - (!selection || selection.isEmpty()) && + (runWithSelectionModeRef.current === "off" || + !selection || + selection.isEmpty()) && createQueryKeyFromRequest(editor, queryInCursor) === createQueryKeyFromRequest(editor, query) ) { From 402b6e7cecb1d9406e0a3f92898e1d07bfb0202f Mon Sep 17 00:00:00 2001 From: emrberk Date: Mon, 31 Aug 2026 13:16:49 +0300 Subject: [PATCH 09/12] fix(console): resolve selected queries at execution time --- e2e/tests/console/editor.spec.js | 51 +++++++- src/scenes/Editor/ButtonBar/index.tsx | 12 +- src/scenes/Editor/Monaco/index.tsx | 172 +++++++++++++++++++------ src/scenes/Editor/Monaco/utils.test.ts | 50 +++++++ src/scenes/Editor/Monaco/utils.ts | 39 +++++- 5 files changed, 266 insertions(+), 58 deletions(-) diff --git a/e2e/tests/console/editor.spec.js b/e2e/tests/console/editor.spec.js index d997fc997..394d3c16a 100644 --- a/e2e/tests/console/editor.spec.js +++ b/e2e/tests/console/editor.spec.js @@ -186,7 +186,7 @@ describe("run query with selection", () => { // Then cy.getByDataHook("button-run-query").should( "contain", - "Run 2 selected queries", + "Run selected queries", ) // When @@ -344,6 +344,43 @@ describe("run query with selection", () => { .should("match", /Running completed in .+ with\s+2 successful\s+queries/) }) + it("should queue selected queries when starting a run while a script is busy", () => { + // Given a script is running + cy.intercept("/exec*", (req) => { + req.on("response", (res) => { + res.setDelay(1000) + }) + }) + cy.typeQuery("select 1;\nselect 2;\nselect 3;") + cy.clickRunScript() + cy.getByDataHook("loading-notification").should( + "contain", + 'Running query "select 1"', + ) + + // When a strict subset is selected and run with the primary shortcut + cy.selectQueries({ + startLineNumber: 1, + startColumn: 1, + endLineNumber: 2, + endColumn: 10, + }) + cy.focused().type(`${ctrlOrCmd}{enter}`) + + // Then the selected run is queued for confirmation instead of discarded + cy.getByRole("dialog").should("be.visible") + cy.getByRole("dialog").should("contain", "Run selected queries") + cy.getByRole("dialog").should("contain", "2 selected queries") + + // When confirmed, the active script is cancelled and the selection runs + cy.getByDataHook("run-all-queries-confirm").click() + cy.contains('[data-hook="success-notification"]', "Running completed", { + timeout: 20000, + }) + .invoke("text") + .should("match", /Running completed in .+ with\s+2 successful\s+queries/) + }) + it("should run all queries when starting a run-all while a query is busy", () => { // Given a single query is running cy.intercept("/exec*", (req) => { @@ -402,7 +439,7 @@ describe("run all queries in tab", () => { // Then cy.getByDataHook("button-run-query").should( "contain", - "Run 6 selected queries", + "Run selected queries", ) // When @@ -1037,7 +1074,7 @@ describe("&query URL param", () => { cy.selectRange({ lineNumber: 1, column: 1 }, { lineNumber: 2, column: 9 }) cy.getByDataHook("button-run-query").should( "contain", - "Run 2 selected queries", + "Run selected queries", ) cy.realPress(["Alt", "L"]) @@ -2221,7 +2258,7 @@ describe("editor settings", () => { // When both expanded queries run cy.getByDataHook("button-run-query").should( "contain", - "Run 2 selected queries", + "Run selected queries", ) cy.clickRunQuery() @@ -2250,7 +2287,7 @@ describe("editor settings", () => { cy.selectRange({ lineNumber: 1, column: 8 }, { lineNumber: 2, column: 9 }) cy.getByDataHook("button-run-query").should( "contain", - "Run 2 selected queries", + "Run selected queries", ) // When the second statement's run glyph is clicked @@ -2274,6 +2311,10 @@ describe("editor settings", () => { // statement cy.getByDataHook("button-run-query").should("be.disabled") + // And the keyboard shortcut is also a no-op + cy.focused().type(`${ctrlOrCmd}{enter}`) + cy.getByDataHook("success-notification").should("not.exist") + // And moving the cursor back into a statement enables it again cy.clickLine(1) cy.getByDataHook("button-run-query").should("not.be.disabled") diff --git a/src/scenes/Editor/ButtonBar/index.tsx b/src/scenes/Editor/ButtonBar/index.tsx index cb33e2c77..157752e3b 100644 --- a/src/scenes/Editor/ButtonBar/index.tsx +++ b/src/scenes/Editor/ButtonBar/index.tsx @@ -12,6 +12,7 @@ import { RunningType } from "../../../store/Query/types" import { useQueryExecutionState } from "../../../hooks/useQueryExecutionState" type ButtonBarProps = { + onRunQuery: () => void onTriggerRunScript: (runAll?: boolean) => void onCopyLinkAllQueries: () => void isTemporary: boolean | undefined @@ -133,6 +134,7 @@ const shortcutTitles = { const copyLinkShortcutTitle = `Copy query link (${altOption}+Shift+L)` const ButtonBar = ({ + onRunQuery, onTriggerRunScript, onCopyLinkAllQueries, isTemporary, @@ -153,12 +155,8 @@ const ButtonBar = ({ dispatch(actions.query.toggleRunning()) return } - if (queriesToRun.length > 1) { - onTriggerRunScript() - } else { - dispatch(actions.query.toggleRunning()) - } - }, [dispatch, running, queriesToRun, onTriggerRunScript]) + onRunQuery() + }, [dispatch, running, onRunQuery]) const handleClickScriptButton = useCallback(() => { onTriggerRunScript(true) @@ -291,7 +289,7 @@ const ButtonBar = ({ return queriesToRun[0].selection ? "Run selected query" : "Run query" } if (numQueries > 1) { - return `Run ${numQueries} selected queries` + return "Run selected queries" } return "Run query" } diff --git a/src/scenes/Editor/Monaco/index.tsx b/src/scenes/Editor/Monaco/index.tsx index 82d0b5b0f..fdc2cc9e7 100644 --- a/src/scenes/Editor/Monaco/index.tsx +++ b/src/scenes/Editor/Monaco/index.tsx @@ -56,6 +56,7 @@ import { isFullQueryMatch, getErrorRange, getQueryFromCursor, + getSelectedText, getQueryRequestFromEditor, getQueryRequestFromLastExecutedQuery, QuestDBLanguageName, @@ -106,6 +107,14 @@ type IndividualQueryResult = { | null } +type ScriptRunPlan = Readonly<{ + runAll: boolean + queries?: readonly Request[] + bufferId: number + model: editor.ITextModel + modelVersionId: number +}> + export const LINE_NUMBER_HARD_LIMIT = MAX_CELL_LINES const Content = styled(PaneContent)<{ $hidden?: boolean }>` @@ -318,7 +327,9 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const queryOffsetsRef = useRef< { startOffset: number; endOffset: number }[] | null >([]) - const pendingScriptRunRef = useRef<{ runAll: boolean } | undefined>(undefined) + const pendingScriptRunRef = useRef(undefined) + const scriptRunPlanRef = useRef(undefined) + const pendingQueryRequestRef = useRef(undefined) const queriesToRunRef = useRef([]) const scriptStopRef = useRef(false) const stopAfterFailureRef = useRef(true) @@ -332,7 +343,6 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const activeNotificationRef = useRef(activeNotification) const canUseAIRef = useRef(canUseAI) const runWithSelectionModeRef = useRef(runWithSelectionMode) - const shareLinkSelectionRunRef = useRef(false) const hasConversationForQueryRef = useRef(hasConversationForQuery) const shiftQueryKeysForBufferRef = useRef(shiftQueryKeysForBuffer) const findQueryByConversationIdRef = useRef(findQueryByConversationId) @@ -518,9 +528,9 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { } } - const handleRunQuery = (query: Request) => { + const handleRunQuery = (query: Request, preserveSelection = false) => { setDropdownOpen(false) - runQueryAction(query, RunningType.QUERY) + runQueryAction(query, RunningType.QUERY, preserveSelection) } const handleExplainQuery = (query: Request) => { @@ -572,6 +582,48 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { return queriesToRun } + const createScriptRunPlan = ( + runAll: boolean, + queries?: readonly Request[], + ): ScriptRunPlan | undefined => { + const model = editorRef.current?.getModel() + if (!model) return + + return { + runAll, + queries: queries ? [...queries] : undefined, + bufferId: activeBufferRef.current.id as number, + model, + modelVersionId: model.getVersionId(), + } + } + + const handlePrimaryRun = () => { + const editor = editorRef.current + if (!editor) return + + const selectionMode = runWithSelectionModeRef.current + const selectedText = getSelectedText(editor) + const resolvesSelection = Boolean( + selectionMode !== "off" && + selectedText && + normalizeQueryText(selectedText), + ) + const queries = resolvesSelection + ? getQueriesToRun(editor, getStatementOffsets(editor), selectionMode, { + queryOffsetsAreFresh: true, + }) + : [getQueryRequestFromEditor(editor, selectionMode)].filter( + (request): request is Request => request !== undefined, + ) + + if (queries.length === 1) { + handleRunQuery(queries[0]) + } else if (queries.length > 1) { + handleTriggerRunScript(false, queries) + } + } + const handleCopyLinkSelection = () => { void trackEvent(ConsoleEvent.EDITOR_COPY_QUERY_LINK, { from: "shortcut", @@ -597,6 +649,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const runQueryAction = ( query: Request, type: RunningType.QUERY | RunningType.EXPLAIN, + preserveSelection = false, ) => { const editor = editorRef.current const model = editor?.getModel() @@ -615,6 +668,8 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { } const targetBufferId = activeBufferRef.current.id as number + const targetModel = model + const targetModelVersionId = model.getVersionId() const queryKey = createQueryKeyFromRequest(editor, query) questExecution.requestExecution({ @@ -626,7 +681,21 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { }, bufferId: targetBufferId, execute: () => { - setCursorBeforeRunning(query) + if ( + activeBufferRef.current.id !== targetBufferId || + editorRef.current?.getModel() !== targetModel || + targetModel.getVersionId() !== targetModelVersionId + ) { + questExecution.releaseExecution(queryKey) + toast.error( + "The editor changed before the query could run. Run it again.", + ) + return + } + pendingQueryRequestRef.current = query + if (!preserveSelection) { + setCursorBeforeRunning(query) + } toggleRunning(type) }, queryKey, @@ -639,9 +708,17 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { pendingScriptRunRef.current = undefined - if (pending.runAll) { - queriesToRunRef.current = [] + if ( + activeBufferRef.current.id !== pending.bufferId || + editorRef.current.getModel() !== pending.model || + pending.model.getVersionId() !== pending.modelVersionId + ) { + toast.error( + "The editor changed before the queries could run. Run them again.", + ) + return } + scriptRunPlanRef.current = pending dispatch(actions.query.toggleRunning(RunningType.SCRIPT)) } @@ -961,11 +1038,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { editor, monaco, runQuery: () => { - if (queriesToRunRef.current.length === 1) { - handleRunQuery(queriesToRunRef.current[0]) - } else if (queriesToRunRef.current.length > 1) { - handleTriggerRunScript() - } + handlePrimaryRun() }, runScript: () => { handleTriggerRunScript(true) @@ -1324,10 +1397,9 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { } else if (queriesToRun.length > 0) { const runQueries = () => { if (queriesToRun.length > 1) { - handleTriggerRunScript() + handleTriggerRunScript(false, queriesToRun) } else { - shareLinkSelectionRunRef.current = true - toggleRunning() + handleRunQuery(queriesToRun[0], true) } } @@ -1544,33 +1616,41 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { setScriptConfirmationOpen(open) } - const handleTriggerRunScript = (runAll?: boolean) => { - if (running === RunningType.SCRIPT) { + const handleTriggerRunScript = ( + runAll?: boolean, + selectedQueries?: readonly Request[], + ) => { + if (runningValueRef.current === RunningType.SCRIPT && runAll) { dispatch(actions.query.toggleRunning()) return } + const runPlan = createScriptRunPlan(Boolean(runAll), selectedQueries) + if (!runPlan) return + + if (!runPlan.runAll && (!runPlan.queries || runPlan.queries.length < 2)) { + return + } + void trackEvent(ConsoleEvent.EDITOR_RUN_MULTIPLE, { - queryCount: queriesToRunRef.current?.length ?? 0, + queryCount: runPlan.queries?.length ?? 0, runAll, }) - const hasMultipleSelection = queriesToRunRef.current.length > 1 - const runsAllQueries = Boolean(runAll) || !hasMultipleSelection - if ( runningValueRef.current === RunningType.NONE && !questExecution.isAnyRunning() ) { - if (runsAllQueries) { + if (runPlan.runAll) { setScriptConfirmation(true) } else { + scriptRunPlanRef.current = runPlan dispatch(actions.query.toggleRunning(RunningType.SCRIPT)) } return } - pendingScriptRunRef.current = { runAll: runsAllQueries } + pendingScriptRunRef.current = runPlan setScriptConfirmation(true) } @@ -1588,7 +1668,9 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { return } - queriesToRunRef.current = [] + const runPlan = createScriptRunPlan(true) + if (!runPlan) return + scriptRunPlanRef.current = runPlan dispatch(actions.query.toggleRunning(RunningType.SCRIPT)) } @@ -1624,10 +1706,25 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { const editor = editorRef.current const monaco = monacoRef.current if (!editor || !monaco) return - const queriesToRun = - queriesToRunRef.current && queriesToRunRef.current.length > 1 - ? queriesToRunRef.current - : undefined + const runPlan = scriptRunPlanRef.current + scriptRunPlanRef.current = undefined + if ( + !runPlan || + activeBufferRef.current.id !== runPlan.bufferId || + editor.getModel() !== runPlan.model || + runPlan.model.getVersionId() !== runPlan.modelVersionId + ) { + toast.error( + "The editor changed before the queries could run. Run them again.", + ) + dispatch(actions.query.stopRunning()) + if (scriptQueryKeyRef.current !== null) { + questExecution.releaseExecution(scriptQueryKeyRef.current) + scriptQueryKeyRef.current = null + } + return + } + const queriesToRun = runPlan.runAll ? undefined : runPlan.queries const runningAllQueries = !queriesToRun // Clear all notifications & execution refs for the buffer const activeBufferId = activeBuffer.id as number @@ -1767,7 +1864,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { ? NotificationType.SUCCESS : NotificationType.ERROR, }, - activeBufferRef.current.id as number, + activeBufferRef.current.id, ), ) setTabsDisabled(false) @@ -1861,19 +1958,13 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { if (![RunningType.NONE, RunningType.SCRIPT].includes(running)) { applyGlyphsAndLineMarkings(monaco, editor) - // Consumed once: only the share-link mount path sets it, right before - // dispatching this run. - const honorShareLinkSelection = shareLinkSelectionRunRef.current - shareLinkSelectionRunRef.current = false + const capturedRequest = pendingQueryRequestRef.current + pendingQueryRequestRef.current = undefined const request = running === RunningType.REFRESH ? getQueryRequestFromLastExecutedQuery(lastExecutedQuery) - : getQueryRequestFromEditor( - editor, - honorShareLinkSelection - ? "complete" - : runWithSelectionModeRef.current, - ) + : (capturedRequest ?? + getQueryRequestFromEditor(editor, runWithSelectionModeRef.current)) const isRunningExplain = running === RunningType.EXPLAIN @@ -2368,6 +2459,7 @@ const MonacoEditor = ({ hidden = false }: { hidden?: boolean }) => { {!hidden && (