From 3d83e026f9700687cdff76bdb351f06fd3b29309 Mon Sep 17 00:00:00 2001 From: Charis <26616127+charislam@users.noreply.github.com> Date: Tue, 21 Jul 2026 16:22:47 -0400 Subject: [PATCH] refactor(sql-editor): finish EditorController/DiffController port (Step 2) (#48166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Step 2 of the SQL Editor testability plan. `SQLEditorContext` already wrapped the Monaco refs and exposed a few semantic imperative helpers (`getEditorSql`, `clearHighlights`, `applyErrorHighlight`, `refocusEditor`, …). This finishes that abstraction so no hook or controller touches `editorRef.current`/`diffEditorRef.current` directly anymore — they only call the port. The port is what will let Step 3's test harness inject a real in-memory editor adapter instead of mocking Monaco; production wires it to the real Monaco refs, unchanged. - Extends the context value with two semantic controllers, backed by the existing refs: - `editor: EditorController` — `isReady`, `getValue`, `getSelectionStartLine`, `getSql` (today's `getEditorSql`), `replaceAll` (wraps the repeated `executeEdits(...)` pattern), `focus`, `revealLineInCenter`, `highlightErrorLine` (today's `applyErrorHighlight`), `clearHighlights`. - `diff: DiffController` — `isMounted`, `getModifiedValue`, `setDiff` (the diff-sync effect body), `attach` (today's `handleDiffEditorMount`). - Migrates every touch point off raw refs onto the port: `useSqlEditorExecution`, `usePrettifyQuery`, `useSqlEditorShortcuts`, `SQLEditorControllers`' `readEditorSql`, and `useSqlEditorAi`'s `acceptAiHandler`/`drainDiffRequest`/`handleDiffEditorMount`/diff-sync effect. - `SQLEditorEditorPanel.tsx` is intentionally left untouched — it wires the raw refs into the real Monaco/DiffEditor React components for rendering, which isn't decision logic to abstract. Behavior-preserving. ## Test plan - [x] `pnpm --filter studio typecheck` - [x] `pnpm test:studio -- SQLEditor` (265 tests passing) - [x] `pnpm --filter studio run lint:ratchet` --- .../interfaces/SQLEditor/SQLEditor.types.ts | 29 ++++ .../interfaces/SQLEditor/SQLEditorContext.tsx | 130 ++++++++++++++---- .../SQLEditor/SQLEditorControllers.tsx | 6 +- .../interfaces/SQLEditor/usePrettifyQuery.ts | 25 ++-- .../interfaces/SQLEditor/useSqlEditorAi.ts | 62 +++------ .../SQLEditor/useSqlEditorExecution.ts | 17 +-- .../SQLEditor/useSqlEditorShortcuts.ts | 6 +- 7 files changed, 175 insertions(+), 100 deletions(-) diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts b/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts index 67467a1d55e..bb3c8581a1e 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts @@ -1,4 +1,5 @@ import type { DiffOnMount, OnMount } from '@monaco-editor/react' +import type { UntrustedSqlFragment } from '@supabase/pg-meta' import { Dispatch, SetStateAction } from 'react' export interface SQLTemplate { @@ -17,6 +18,34 @@ export type ContentDiff = { modified: string } +/** + * Semantic, Monaco-agnostic port onto the main editor. Hooks/controllers call + * this instead of touching `editorRef.current` directly, so the editor is + * swappable for a real in-memory adapter in tests without mocking. + */ +export type EditorController = { + isReady: () => boolean + getValue: () => string | undefined + getSelectionStartLine: () => number | undefined + getSql: (snippetContent?: UntrustedSqlFragment) => UntrustedSqlFragment | undefined + replaceAll: (text: string, source: string) => void + focus: () => void + revealLineInCenter: (line: number) => void + highlightErrorLine: ( + error: { position?: unknown; formattedError?: string }, + hasSelection: boolean + ) => void + clearHighlights: () => void +} + +/** Semantic, Monaco-agnostic port onto the diff editor. */ +export type DiffController = { + isMounted: () => boolean + getModifiedValue: () => string | undefined + setDiff: (diff: ContentDiff, revealLine: number) => void + attach: (editor: IStandaloneDiffEditor) => void +} + export type SQLEditorContextValues = { aiInput: string setAiInput: Dispatch> diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditorContext.tsx b/apps/studio/components/interfaces/SQLEditor/SQLEditorContext.tsx index e8f568f0eed..a5be74739e1 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditorContext.tsx +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditorContext.tsx @@ -10,7 +10,13 @@ import { type RefObject, } from 'react' -import type { IStandaloneCodeEditor, IStandaloneDiffEditor } from './SQLEditor.types' +import type { + ContentDiff, + DiffController, + EditorController, + IStandaloneCodeEditor, + IStandaloneDiffEditor, +} from './SQLEditor.types' import { computeErrorHighlightLine, getEditorSql } from './SQLEditor.utils' type SQLEditorContextValue = { @@ -26,14 +32,10 @@ type SQLEditorContextValue = { markRefocusAfterRun: () => void /** Refocus the editor iff a run-refocus was requested, then clear the flag. */ refocusEditorAfterRunIfNeeded: () => void - getEditorSql: (snippetContent?: UntrustedSqlFragment) => UntrustedSqlFragment | undefined - /** Clear any active error-highlight decorations. */ - clearHighlights: () => void - /** Highlight and reveal the line referenced by an execute error's position. */ - applyErrorHighlight: ( - error: { position?: unknown; formattedError?: string }, - hasSelection: boolean - ) => void + /** Semantic port onto the main editor — no hook should touch `editorRef.current` directly. */ + editor: EditorController + /** Semantic port onto the diff editor — no hook should touch `diffEditorRef.current` directly. */ + diff: DiffController } const SQLEditorContext = createContext(null) @@ -77,10 +79,34 @@ export const SQLEditorProvider = ({ children }: PropsWithChildren) => { refocusEditor() }, [refocusEditor]) - const getEditorSqlFromEditor = useCallback((snippetContent?: UntrustedSqlFragment) => { - const editor = editorRef.current - if (!editor) return undefined - return getEditorSql(editor, snippetContent) + const isEditorReady = useCallback(() => editorRef.current !== null, []) + + const getEditorValue = useCallback(() => editorRef.current?.getValue(), []) + + const getSelectionStartLine = useCallback( + () => editorRef.current?.getSelection()?.startLineNumber, + [] + ) + + const getSqlFromEditor = useCallback((snippetContent?: UntrustedSqlFragment) => { + const editorInstance = editorRef.current + if (!editorInstance) return undefined + return getEditorSql(editorInstance, snippetContent) + }, []) + + const replaceAll = useCallback((text: string, source: string) => { + const editorInstance = editorRef.current + const model = editorInstance?.getModel() + if (!editorInstance || !model) return + editorInstance.executeEdits(source, [{ text, range: model.getFullModelRange() }]) + }, []) + + const focusEditor = useCallback(() => { + editorRef.current?.focus() + }, []) + + const revealLineInCenter = useCallback((line: number) => { + editorRef.current?.revealLineInCenter(line) }, []) const clearHighlights = useCallback(() => { @@ -90,17 +116,19 @@ export const SQLEditorProvider = ({ children }: PropsWithChildren) => { } }, []) - const applyErrorHighlight = useCallback( + const highlightErrorLine = useCallback( (error: { position?: unknown; formattedError?: string }, hasSelection: boolean) => { if (!error.position || !monacoRef.current) return - const editor = editorRef.current + const editorInstance = editorRef.current const monaco = monacoRef.current - const startLineNumber = hasSelection ? (editor?.getSelection()?.startLineNumber ?? 0) : 0 + const startLineNumber = hasSelection + ? (editorInstance?.getSelection()?.startLineNumber ?? 0) + : 0 const line = computeErrorHighlightLine(error, startLineNumber) if (isNaN(line)) return - const decorations = editor?.deltaDecorations( + const decorations = editorInstance?.deltaDecorations( [], [ { @@ -113,13 +141,69 @@ export const SQLEditorProvider = ({ children }: PropsWithChildren) => { ] ) if (decorations) { - editor?.revealLineInCenter(line) + editorInstance?.revealLineInCenter(line) lineHighlightsRef.current = decorations } }, [] ) + const editor = useMemo( + () => ({ + isReady: isEditorReady, + getValue: getEditorValue, + getSelectionStartLine, + getSql: getSqlFromEditor, + replaceAll, + focus: focusEditor, + revealLineInCenter, + highlightErrorLine, + clearHighlights, + }), + [ + isEditorReady, + getEditorValue, + getSelectionStartLine, + getSqlFromEditor, + replaceAll, + focusEditor, + revealLineInCenter, + highlightErrorLine, + clearHighlights, + ] + ) + + const isDiffMounted = useCallback(() => diffEditorRef.current !== null, []) + + const getModifiedValue = useCallback( + () => diffEditorRef.current?.getModel()?.modified.getValue(), + [] + ) + + const setDiff = useCallback((contentDiff: ContentDiff, revealLine: number) => { + const diffEditorInstance = diffEditorRef.current + const model = diffEditorInstance?.getModel() + if (!diffEditorInstance || !model || !model.original || !model.modified) return + + model.original.setValue(contentDiff.original) + model.modified.setValue(contentDiff.modified) + diffEditorInstance.getModifiedEditor().revealLineInCenter(revealLine) + }, []) + + const attachDiffEditor = useCallback((editorInstance: IStandaloneDiffEditor) => { + diffEditorRef.current = editorInstance + }, []) + + const diff = useMemo( + () => ({ + isMounted: isDiffMounted, + getModifiedValue, + setDiff, + attach: attachDiffEditor, + }), + [isDiffMounted, getModifiedValue, setDiff, attachDiffEditor] + ) + const value = useMemo( () => ({ editorRef, @@ -130,18 +214,16 @@ export const SQLEditorProvider = ({ children }: PropsWithChildren) => { clearPendingRunRefocus, markRefocusAfterRun, refocusEditorAfterRunIfNeeded, - getEditorSql: getEditorSqlFromEditor, - clearHighlights, - applyErrorHighlight, + editor, + diff, }), [ refocusEditor, clearPendingRunRefocus, markRefocusAfterRun, refocusEditorAfterRunIfNeeded, - getEditorSqlFromEditor, - clearHighlights, - applyErrorHighlight, + editor, + diff, ] ) diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditorControllers.tsx b/apps/studio/components/interfaces/SQLEditor/SQLEditorControllers.tsx index f85aaa6f3d5..03aeffa3f24 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditorControllers.tsx +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditorControllers.tsx @@ -111,7 +111,7 @@ export const useSqlEditorRun = () => useGuardedContext(RunContext, 'useSqlEditor export const useSqlEditorUi = () => useGuardedContext(UiContext, 'useSqlEditorUi') export const SQLEditorControllersProvider = ({ children }: PropsWithChildren) => { - const { monacoRef, scrollTopRef, getEditorSql: getEditorSqlFromEditor } = useSQLEditorContext() + const { monacoRef, scrollTopRef, editor } = useSQLEditorContext() const { ref } = useParams() const { data: project } = useSelectedProjectQuery() @@ -148,8 +148,8 @@ export const SQLEditorControllersProvider = ({ children }: PropsWithChildren) => // to safety (acceptUntrustedSql) happens at each user-action site, never here. const readEditorSql = useCallback((): UntrustedSqlFragment | undefined => { const snippet = getSqlEditorV2StateSnapshot().snippets[id] - return getEditorSqlFromEditor(snippet?.snippet.content?.unchecked_sql) - }, [getEditorSqlFromEditor, id]) + return editor.getSql(snippet?.snippet.content?.unchecked_sql) + }, [editor, id]) const { executeQuery, isExecuting, potentialIssues, resetPotentialIssues } = useSqlEditorExecution({ diff --git a/apps/studio/components/interfaces/SQLEditor/usePrettifyQuery.ts b/apps/studio/components/interfaces/SQLEditor/usePrettifyQuery.ts index 1ded7a6c129..60c415e79d3 100644 --- a/apps/studio/components/interfaces/SQLEditor/usePrettifyQuery.ts +++ b/apps/studio/components/interfaces/SQLEditor/usePrettifyQuery.ts @@ -1,6 +1,5 @@ import { useCallback } from 'react' -import { getEditorSql } from './SQLEditor.utils' import { useSQLEditorContext } from './SQLEditorContext' import { useSelectedProjectQuery } from '@/hooks/misc/useSelectedProject' import { formatSql } from '@/lib/formatSql' @@ -14,7 +13,7 @@ import { * the formatted SQL back to the snippet store. No-op while a diff is open. */ export function usePrettifyQuery({ id, isDiffOpen }: { id: string; isDiffOpen: boolean }) { - const { editorRef } = useSQLEditorContext() + const { editor } = useSQLEditorContext() const { data: project } = useSelectedProjectQuery() const snapV2 = useSqlEditorV2StateSnapshot() @@ -25,21 +24,13 @@ export function usePrettifyQuery({ id, isDiffOpen }: { id: string; isDiffOpen: b const state = getSqlEditorV2StateSnapshot() const snippet = state.snippets[id] - if (editorRef.current && project) { - const editor = editorRef.current - const sql = getEditorSql(editor, snippet?.snippet.content?.unchecked_sql) - const formattedSql = formatSql(sql) + if (editor.isReady() && project) { + const sql = editor.getSql(snippet?.snippet.content?.unchecked_sql) + if (sql === undefined) return - const editorModel = editorRef?.current?.getModel() - if (editorRef.current && editorModel) { - editorRef.current.executeEdits('apply-prettify-edit', [ - { - text: formattedSql, - range: editorModel.getFullModelRange(), - }, - ]) - snapV2.setSql({ id, sql: formattedSql }) - } + const formattedSql = formatSql(sql) + editor.replaceAll(formattedSql, 'apply-prettify-edit') + snapV2.setSql({ id, sql: formattedSql }) } - }, [editorRef, id, isDiffOpen, project, snapV2]) + }, [editor, id, isDiffOpen, project, snapV2]) } diff --git a/apps/studio/components/interfaces/SQLEditor/useSqlEditorAi.ts b/apps/studio/components/interfaces/SQLEditor/useSqlEditorAi.ts index cc457c1bf5e..5b41e45e647 100644 --- a/apps/studio/components/interfaces/SQLEditor/useSqlEditorAi.ts +++ b/apps/studio/components/interfaces/SQLEditor/useSqlEditorAi.ts @@ -58,7 +58,7 @@ export function useSqlEditorAi({ id, editorMountCount, diff, prompt }: UseSqlEdi } = diff const { promptState, setPromptState, resetPrompt } = prompt - const { editorRef, diffEditorRef, refocusEditor } = useSQLEditorContext() + const { editor, diff: diffController, refocusEditor } = useSQLEditorContext() const router = useRouter() const { ref } = useParams() @@ -131,25 +131,16 @@ export function useSqlEditorAi({ id, editorMountCount, diff, prompt }: UseSqlEdi setIsAcceptDiffLoading(true) // TODO: show error if undefined - if (!sourceSqlDiff || !editorRef.current || !diffEditorRef.current) return + if (!sourceSqlDiff || !editor.isReady() || !diffController.isMounted()) return - const editorModel = editorRef.current.getModel() - const diffModel = diffEditorRef.current.getModel() - - if (!editorModel || !diffModel) return - - const sql = diffModel.modified.getValue() + const sql = diffController.getModifiedValue() + if (sql === undefined) return if (selectedDiffType === DiffType.NewSnippet) { const { title } = await generateSqlTitle({ sql }) await handleNewQuery(sql, title) } else { - editorRef.current.executeEdits('apply-ai-edit', [ - { - text: sql, - range: editorModel.getFullModelRange(), - }, - ]) + editor.replaceAll(sql, 'apply-ai-edit') } track('assistant_sql_diff_handler_evaluated', { handlerAccepted: true }) @@ -162,8 +153,8 @@ export function useSqlEditorAi({ id, editorMountCount, diff, prompt }: UseSqlEdi setIsAcceptDiffLoading(false) } }, [ - editorRef, - diffEditorRef, + editor, + diffController, sourceSqlDiff, selectedDiffType, generateSqlTitle, @@ -284,11 +275,11 @@ export function useSqlEditorAi({ id, editorMountCount, diff, prompt }: UseSqlEdi ) const handleDiffEditorMount = useCallback( - (editor: IStandaloneDiffEditor) => { - diffEditorRef.current = editor + (mountedDiffEditor: IStandaloneDiffEditor) => { + diffController.attach(mountedDiffEditor) setIsDiffEditorMounted(true) }, - [diffEditorRef] + [diffController] ) const resetDiff = useEffectEvent(() => { @@ -303,19 +294,14 @@ export function useSqlEditorAi({ id, editorMountCount, diff, prompt }: UseSqlEdi // eslint-disable-next-line react-hooks/exhaustive-deps }, [id]) - useEffect(() => { + const syncDiffEditor = useEffectEvent(() => { if (isDiffOpen) { - const diffEditor = diffEditorRef.current - const model = diffEditor?.getModel() - if (model && model.original && model.modified) { - model.original.setValue(defaultSqlDiff.original) - model.modified.setValue(defaultSqlDiff.modified) - // scroll to the start line of the modification - const modifiedEditor = diffEditor!.getModifiedEditor() - const startLine = promptState.startLineNumber - modifiedEditor.revealLineInCenter(startLine) - } + diffController.setDiff(defaultSqlDiff, promptState.startLineNumber) } + }) + useEffect(() => { + syncDiffEditor() + // Temporary until we update eslint to ignore useEffectEvent // eslint-disable-next-line react-hooks/exhaustive-deps }, [selectedDiffType, sourceSqlDiff]) @@ -323,21 +309,15 @@ export function useSqlEditorAi({ id, editorMountCount, diff, prompt }: UseSqlEdi const request = diffRequest.pending if (request === undefined) return - const editorModel = editorRef.current?.getModel() // Editor isn't ready yet; leave the request pending. editorMountCount bumps // on mount and re-runs this effect, so the request applies once mounted. - if (!editorModel) return + if (!editor.isReady()) return - const existingValue = editorRef.current?.getValue() ?? '' + const existingValue = editor.getValue() ?? '' const plan = planDiffRequestApplication({ existingValue, request }) if (plan.kind === 'replace') { // if the editor is empty, just copy over the code - editorRef.current?.executeEdits('apply-ai-message', [ - { - text: plan.text, - range: editorModel.getFullModelRange(), - }, - ]) + editor.replaceAll(plan.text, 'apply-ai-message') } else { setSourceSqlDiff(plan.diff) setSelectedDiffType(plan.diffType) @@ -358,11 +338,11 @@ export function useSqlEditorAi({ id, editorMountCount, diff, prompt }: UseSqlEdi if (!isDiffOpen) { setIsDiffEditorMounted(false) setShowWidget(false) - } else if (diffEditorRef.current && isDiffEditorMounted) { + } else if (diffController.isMounted() && isDiffEditorMounted) { setShowWidget(true) return () => setShowWidget(false) } - }, [diffEditorRef, isDiffOpen, isDiffEditorMounted]) + }, [diffController, isDiffOpen, isDiffEditorMounted]) return useMemo( () => ({ diff --git a/apps/studio/components/interfaces/SQLEditor/useSqlEditorExecution.ts b/apps/studio/components/interfaces/SQLEditor/useSqlEditorExecution.ts index 1cc661c0db7..7ded54949db 100644 --- a/apps/studio/components/interfaces/SQLEditor/useSqlEditorExecution.ts +++ b/apps/studio/components/interfaces/SQLEditor/useSqlEditorExecution.ts @@ -40,13 +40,7 @@ export function useSqlEditorExecution({ setAiTitle, }: UseSqlEditorExecutionArgs) { const { ref } = useParams() - const { - editorRef, - clearPendingRunRefocus, - refocusEditorAfterRunIfNeeded, - clearHighlights, - applyErrorHighlight, - } = useSQLEditorContext() + const { editor, clearPendingRunRefocus, refocusEditorAfterRunIfNeeded } = useSQLEditorContext() const { data: project } = useSelectedProjectQuery() const queryClient = useQueryClient() @@ -80,7 +74,7 @@ export function useSqlEditorExecution({ }, onError(error: any, vars) { if (id) { - applyErrorHighlight(error, hasSelection) + editor.highlightErrorLine(error, hasSelection) sessionSnap.addResultError(id, error, vars.autoLimit) } @@ -95,7 +89,7 @@ export function useSqlEditorExecution({ return } - if (editorRef.current === null || isExecuting || project === undefined) { + if (!editor.isReady() || isExecuting || project === undefined) { clearPendingRunRefocus() return } @@ -120,7 +114,7 @@ export function useSqlEditorExecution({ setAiTitle(id, sql) } - clearHighlights() + editor.clearHighlights() const impersonatedRoleState = getImpersonatedRoleState() const connectionString = resolveConnectionString( @@ -148,8 +142,7 @@ export function useSqlEditorExecution({ track('sql_editor_query_run_button_clicked') }, [ - editorRef, - clearHighlights, + editor, clearPendingRunRefocus, isDiffOpen, id, diff --git a/apps/studio/components/interfaces/SQLEditor/useSqlEditorShortcuts.ts b/apps/studio/components/interfaces/SQLEditor/useSqlEditorShortcuts.ts index d0996726b10..cb4fff44419 100644 --- a/apps/studio/components/interfaces/SQLEditor/useSqlEditorShortcuts.ts +++ b/apps/studio/components/interfaces/SQLEditor/useSqlEditorShortcuts.ts @@ -33,7 +33,7 @@ export function useSqlEditorShortcuts({ const os = detectOS() const router = useRouter() const { ref } = useParams() - const { editorRef, refocusEditor } = useSQLEditorContext() + const { editor, refocusEditor } = useSQLEditorContext() const openNewSnippet = useCallback(() => { if (!ref) return @@ -67,7 +67,7 @@ export function useSqlEditorShortcuts({ case 'escape': if (action.shouldDiscard) discardAiHandler() resetPrompt() - editorRef.current?.focus() + editor.focus() return case 'none': return @@ -75,5 +75,5 @@ export function useSqlEditorShortcuts({ } window.addEventListener('keydown', handler) return () => window.removeEventListener('keydown', handler) - }, [editorRef, os, isDiffOpen, isPromptOpen, acceptAiHandler, discardAiHandler, resetPrompt]) + }, [editor, os, isDiffOpen, isPromptOpen, acceptAiHandler, discardAiHandler, resetPrompt]) }