feat(webui): Add disabled state and theme to SqlEditor; Focus SqlEditor on page load and after query termination; Remove autocomplete.
#1244
Conversation
WalkthroughAdded a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Page as SearchPage.SqlQueryInput
participant Editor as SqlEditor
participant Monaco as Monaco Editor
Note over Page,Editor: Render with props (disabled by UI state, onEditorReady, ref)
Page->>Editor: render SqlEditor(disabled, onEditorReady, ref)
Editor->>Monaco: create editor instance (mount)
Monaco-->>Editor: editor mounted
Editor->>Page: call onEditorReady()
Editor->>Page: expose ref.focus()
alt Page decides to focus
Page->>Page: if isEditorReady && UI state ∈ {DEFAULT,DONE,FAILED}
Page->>Editor: ref.focus()
end
Note over Editor,Monaco: Disabled handling
Editor->>Monaco: setTheme(disabled-theme or light)
Editor-->>Page: pointer-events blocked when disabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/client/src/components/SqlEditor/index.tsx (1)
85-102: Make the “Current” suggestion reliably first; mark list complete.Comment says “make current input the first suggestion,” but it’s appended, so sorting is undefined. Use
sortTextandpreselectto ensure it’s first, and returnincomplete: falseto avoid re-queries.- const suggestions = sqlLanguage.keywords.map((keyword: string) => ({ + const suggestions = sqlLanguage.keywords.map((keyword: string) => ({ detail: "Presto SQL (CLP)", insertText: `${keyword} `, kind: monacoEditor.languages.CompletionItemKind.Keyword, label: keyword, range: range, + sortText: "1", })); @@ - // we make the current input the first suggestion. Users can then use arrow keys + // we explicitly make the current input the first suggestion. Users can then use arrow keys // to select a keyword if needed. @@ - suggestions.push({ + suggestions.unshift({ detail: "Current", insertText: `${typedWord}\n`, - kind: monaco.languages.CompletionItemKind.Text, + kind: monacoEditor.languages.CompletionItemKind.Text, label: typedWord, range: range, + sortText: "0", + preselect: true, }); } @@ - return { - suggestions: suggestions, - incomplete: true, - }; + return { + suggestions, + incomplete: false, + }; @@ triggerCharacters: [ " ", ],Note: also switched the
kindreference to the same Monaco instance for consistency.Also applies to: 103-117, 119-122, 124-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
components/webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
components/webui/client/package.json(1 hunks)components/webui/client/src/components/SqlEditor/index.tsx(4 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsxcomponents/webui/client/src/components/SqlEditor/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)
components/webui/client/src/components/SqlEditor/index.tsx (1)
SqlEditorRef(174-174)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
🔇 Additional comments (3)
components/webui/client/package.json (1)
29-29: Reevaluatecolordependency in SqlEditorWe confirmed that:
color@5.0.0is published as an ESM module (type: "module"), so it will load under Vite/React 19 without transpilation issues.- The only usages are in
components/webui/client/src/components/SqlEditor/index.tsxat lines 67–68 converting AntD token colours via.hexa().- Monaco’s theming API accepts any valid CSS colour string (including hex and
rgba()), so you can feed the raw AntD tokens directly without the extra conversion.Recommendations:
- Drop the
colordependency and use the tokens straight in your theme definition, for example:monaco.editor.defineTheme("myTheme", { inherit: true, rules: [], colors: { "editor.background": token.colorBgContainerDisabled, "editor.foreground": token.colorTextDisabled, // … }, });- If you still need to normalise RGBA to 8-digit hex, consider a tiny helper function instead of the full
colorlibrary.- If you choose to keep
color, you’ve already verified v5’s ESM support and its permissive MIT licence—no further action required.[optional_refactors_recommended】
components/webui/client/src/components/SqlEditor/index.tsx (1)
124-127: LGTM: removing newline as a trigger is the right call.This avoids accidental accept-on-Enter surprises while still allowing explicit invocation via space or Ctrl/Cmd+Space.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (1)
52-54: LGTM: height, onChange wiring, and onEditorReady handshake.This wiring aligns with the new SqlEditor API and the focus-after-ready requirement.
Also applies to: 59-59
| import {theme} from "antd"; | ||
| import color from "color"; | ||
| import {language as sqlLanguage} from "monaco-editor/esm/vs/basic-languages/sql/sql.js"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify Monaco instance usage and naming.
You both import monaco and use const monacoEditor = useMonaco(). Mixing the two can lead to confusion and hard-to-debug mismatches. Pick one (prefer the useMonaco() instance) and name it monaco or monacoApi to avoid misleading “Editor” terminology.
I can send a follow-up diff to rename monacoEditor -> monaco and remove the top-level import * as monaco if you want.
Also applies to: 41-43, 85-87, 113-114
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/index.tsx around lines 13-15
(and also apply changes at 41-43, 85-87, 113-114), the file mixes a top-level
monaco import with the useMonaco() hook variable named monacoEditor which is
confusing; remove the top-level import of monaco, rename the hook variable
monacoEditor to monaco (or monacoApi) across the file, and update all references
to use this single instance so the component consistently uses the useMonaco()
instance and avoids misleading “Editor” naming.
| // Define disabled theme for monaco editor | ||
| useEffect(() => { | ||
| if (null === monacoEditor) { | ||
| return; | ||
| } | ||
| monacoEditor.editor.defineTheme("disabled-theme", { | ||
| base: "vs", | ||
| inherit: true, | ||
| rules: [], | ||
| colors: { | ||
| "editor.background": color(token.colorBgContainerDisabled).hexa(), | ||
| "editor.foreground": color(token.colorTextDisabled).hexa(), | ||
|
|
||
| // transparent | ||
| "focusBorder": "#00000000", | ||
| }, | ||
| }); | ||
| }, [ | ||
| monacoEditor, | ||
| token | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid the color lib if tokens are already valid CSS; Monaco accepts rgba().
If token.colorBgContainerDisabled and token.colorTextDisabled are valid CSS colour strings (often hex or rgba), you can pass them directly. This would let you remove the color dependency.
- "editor.background": color(token.colorBgContainerDisabled).hexa(),
- "editor.foreground": color(token.colorTextDisabled).hexa(),
+ "editor.background": token.colorBgContainerDisabled,
+ "editor.foreground": token.colorTextDisabled,If you’ve seen cases where tokens are in non-standard formats, keep the dependency and ignore this comment.
Also applies to: 67-69
🤖 Prompt for AI Agents
components/webui/client/src/components/SqlEditor/index.tsx around lines 57 to 77
(and specifically lines 67-69): the code wraps token.colorBgContainerDisabled
and token.colorTextDisabled with the external color(...) library to call
.hexa(), but Monaco accepts standard CSS color strings (hex, rgba) directly; if
these token values are already valid CSS colors, remove the color(...) wrapper
and pass token.colorBgContainerDisabled and token.colorTextDisabled directly to
the theme colors, and then remove the unused import of the color library; if
tokens can be nonstandard, leave the wrapper and add a short defensive comment
explaining why it’s required.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
looks good to me. I made a few nitpicking changes.
- I vaguely remember taht we wanted the regular fontSize.
- How about making the height dynamic instead of having the component user specify it?
- I've removed the white space on the left by setting
folding: false.
@@ -40,6 +43,7 @@ const SqlEditor = (props: SqlEditorProps) => {
const editorRef = useRef<monaco.editor.IStandaloneCodeEditor>(null);
const monacoEditor = useMonaco();
const {token} = theme.useToken();
+ const [lineHeight, setLineHeight] = useState<Nullable<number>>(null);
useImperativeHandle(ref, () => ({
focus: () => {
@@ -52,6 +56,8 @@ const SqlEditor = (props: SqlEditorProps) => {
) => {
editorRef.current = editor;
onEditorReady?.();
+
+ setLineHeight(editor.getOption(monaco.editor.EditorOption.lineHeight))
}, [onEditorReady]);
// Define disabled theme for monaco editor
@@ -71,6 +77,7 @@ const SqlEditor = (props: SqlEditorProps) => {
"focusBorder": "#00000000",
},
});
+
}, [
monacoEditor,
token
@@ -140,6 +147,7 @@ const SqlEditor = (props: SqlEditorProps) => {
}
>
<Editor
+ height={null === lineHeight ? "" : 5*lineHeight}
language={"sql"}
loading={
<div
@@ -151,9 +159,8 @@ const SqlEditor = (props: SqlEditorProps) => {
}
options={{
automaticLayout: true,
- fontSize: 20,
- lineHeight: 30,
lineNumbers: "off",
+ folding: false,
minimap: {enabled: false},
overviewRulerBorder: false,
placeholder: "Enter your SQL query",
davemarco
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
components/webui/client/src/components/SqlEditor/index.tsx (6)
1-6: Critical: ref forwarding is broken; focus() is never exposed to parentsFunction components don’t receive a
refprop. WithoutforwardRef,useImperativeHandle(ref, …)is a no-op and the publicfocus()API won’t work. This blocks the PR’s focus-on-load/termination objective. Fix by wrapping the component withforwardRefand removingReact.RefAttributesfrom props.Apply this diff:
@@ -import { - useCallback, - useEffect, - useImperativeHandle, - useRef, -} from "react"; +import { + forwardRef, + useCallback, + useEffect, + useImperativeHandle, + useRef, +} from "react"; @@ -type SqlEditorProps = Omit<EditorProps, "language"> & React.RefAttributes<SqlEditorRef> & { +type SqlEditorProps = Omit<EditorProps, "language"> & { disabled: boolean; /** Callback when the editor is mounted and ref is ready to use. */ onEditorReady?: () => void; }; @@ -const SqlEditor = (props: SqlEditorProps) => { - const {ref, disabled, onEditorReady, ...editorProps} = props; +const SqlEditor = forwardRef<SqlEditorRef, SqlEditorProps>((props, ref) => { + const {disabled, onEditorReady, ...editorProps} = props; const editorRef = useRef<monaco.editor.IStandaloneCodeEditor>(null); @@ - useImperativeHandle(ref, () => ({ + useImperativeHandle(ref, () => ({ focus: () => { editorRef.current?.focus(); }, }), []); @@ - return ( + return ( <div style={ disabled ? {pointerEvents: "none"} : {} } > <Editor onMount={handleEditorDidMount} {...editorProps}/> </div> - ); -}; + ); +});Also applies to: 24-29, 37-48
8-16: Unify Monaco usage: remove runtime import, use type-only import, and rename hook variableYou mix a direct
monacoimport withuseMonaco(). Keep one runtime source (preferuseMonaco()), and import types only to avoid bundle bloat. Also renamemonacoEditortomonaco(ormonacoApi) for clarity.Apply this diff:
@@ -import {theme} from "antd"; -import color from "color"; -import * as monaco from "monaco-editor/esm/vs/editor/editor.api.js"; +import {theme} from "antd"; +import color from "color"; +import type * as Monaco from "monaco-editor"; @@ - const editorRef = useRef<monaco.editor.IStandaloneCodeEditor>(null); - const monacoEditor = useMonaco(); + const editorRef = useRef<Monaco.editor.IStandaloneCodeEditor>(null); + const monaco = useMonaco(); @@ - useEffect(() => { - if (null === monacoEditor) { + useEffect(() => { + if (null === monaco) { return; } - monacoEditor.editor.defineTheme("disabled-theme", { + monaco.editor.defineTheme("disabled-theme", { base: "vs", inherit: true, rules: [], colors: { "editor.background": color(token.colorBgContainerDisabled).hexa(), "editor.foreground": color(token.colorTextDisabled).hexa(), // transparent "focusBorder": "#00000000", }, }); - }, [ - monacoEditor, - token - ]); + }, [monaco, token]);If you’d rather avoid using the same identifier for the hook variable, rename to
monacoApiconsistently.Also applies to: 40-41, 56-76
108-110: Use Monaco’s canonical light theme id (vs)Monaco’s built-in light theme id is
vs. Some wrappers alias"light", some don’t. Usevsto avoid surprises.Apply this diff:
- theme={disabled ? - "disabled-theme" : - "light"} + theme={disabled ? "disabled-theme" : "vs"}
79-85: A11y: addaria-disabledto convey the disabled state semanticallyPointer-events and visuals aren’t sufficient for assistive tech. Add
aria-disabled={disabled}to the wrapper.Apply this diff:
- <div + <div + aria-disabled={disabled} style={ disabled ? {pointerEvents: "none"} : {} } >
96-107: Make the editor truly non-editable when disabledIf the editor already has focus, keyboard input can still modify content despite
pointer-events: "none". Set Monaco’sreadOnly(and optionallydomReadOnly) based ondisabled.Apply this diff:
options={{ automaticLayout: true, folding: false, fontSize: 16, lineNumbers: "off", minimap: {enabled: false}, overviewRulerBorder: false, placeholder: "Enter your SQL query", renderLineHighlightOnlyWhenFocus: true, scrollBeyondLastLine: false, wordWrap: "on", + readOnly: disabled, + // If your Monaco version supports it, also: + // domReadOnly: disabled, }}
66-67: Consider removing thecolordependency if tokens are already valid CSSMonaco accepts standard CSS colours (hex/rgba). If your Ant Design tokens are already valid CSS strings, pass them directly and drop the
colorlib.Apply this diff:
- "editor.background": color(token.colorBgContainerDisabled).hexa(), - "editor.foreground": color(token.colorTextDisabled).hexa(), + "editor.background": token.colorBgContainerDisabled, + "editor.foreground": token.colorTextDisabled,And, if fully removed:
-import color from "color";Retain
colorif your tokens can be non-standard; otherwise, this simplifies the bundle.Also applies to: 14-14
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)
35-47: Tidy focus effect: compute a boolean and early-returnCondense the condition into a
shouldFocusboolean for readability and maintainability.Apply this diff:
useEffect(() => { - if ( - isEditorReady && - (searchUiState === SEARCH_UI_STATE.DEFAULT || - searchUiState === SEARCH_UI_STATE.DONE || - searchUiState === SEARCH_UI_STATE.FAILED) - ) { - editorRef.current?.focus(); - } + const shouldFocus = + isEditorReady && + (searchUiState === SEARCH_UI_STATE.DEFAULT || + searchUiState === SEARCH_UI_STATE.DONE || + searchUiState === SEARCH_UI_STATE.FAILED); + if (false == shouldFocus) { return; } + editorRef.current?.focus(); }, [ searchUiState, isEditorReady ]);
52-59: ExtractisQueryingto centralize the disabled conditionKeeps the intent clear and the state check in one place.
Apply this diff:
- <SqlEditor + <SqlEditor height={"120px"} ref={editorRef} - disabled={ - searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || - searchUiState === SEARCH_UI_STATE.QUERYING - } + disabled={(() => { + const isQuerying = + searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || + searchUiState === SEARCH_UI_STATE.QUERYING; + return isQuerying; + })()} onChange={handleChange} onEditorReady={handleEditorReady}/>Alternatively, define above JSX:
const isQuerying = searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || searchUiState === SEARCH_UI_STATE.QUERYING;Then pass
disabled={isQuerying}.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/webui/client/src/components/SqlEditor/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsxcomponents/webui/client/src/components/SqlEditor/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)
components/webui/client/src/components/SqlEditor/index.tsx (1)
SqlEditorRef(118-118)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-check (macos-15)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
Show resolved
Hide resolved
I tested with regular and compromised to 16px from 20px. Now it is the same as run button
The code just looked a bit confusing. I lowered size to 120px.
sure this looks great Note i also removed the auto complete stuff. It was a bit annoying, and was slightly innacurate. maybe we will add back later with a better design |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (3)
22-24: Focus ref won’t work until SqlEditor forwards the ref (blocker in child).
editorRef.current?.focus()depends on the child exposing an imperativefocus()viaforwardRef. The current SqlEditor implementation does not forward the ref, so this ref will remain null and focus calls will be a no‑op. See my blocking comment in SqlEditor/index.tsx with a concrete fix.Run to verify the child has proper ref forwarding:
#!/bin/bash set -euo pipefail rg -nP 'forwardRef<\s*SqlEditorRef' components/webui/client/src/components/SqlEditor/index.tsx || { echo "Missing forwardRef<SqlEditorRef>"; exit 1; } rg -n 'useImperativeHandle\(' components/webui/client/src/components/SqlEditor/index.tsx || { echo "Missing useImperativeHandle(ref, …)"; exit 1; }
35-47: Nit: computeshouldFocusonce and early-return for clarity.This reduces nesting and duplication of the condition.
useEffect(() => { - if ( - isEditorReady && - (searchUiState === SEARCH_UI_STATE.DEFAULT || - searchUiState === SEARCH_UI_STATE.DONE || - searchUiState === SEARCH_UI_STATE.FAILED) - ) { - editorRef.current?.focus(); - } + const shouldFocus = + isEditorReady && + (searchUiState === SEARCH_UI_STATE.DEFAULT || + searchUiState === SEARCH_UI_STATE.DONE || + searchUiState === SEARCH_UI_STATE.FAILED); + if (false == shouldFocus) { return; } + editorRef.current?.focus(); }, [ searchUiState, isEditorReady, ]);
55-57: Nit: extract anisQueryingboolean and reuse.Keeps the disabled condition in one place and reads better in JSX.
- disabled={ - searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || - searchUiState === SEARCH_UI_STATE.QUERYING - } + disabled={(() => { + const isQuerying = + searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || + searchUiState === SEARCH_UI_STATE.QUERYING; + return isQuerying; + })()}Or define
const isQuerying = …above and passdisabled={isQuerying}.components/webui/client/src/components/SqlEditor/index.tsx (8)
1-6: Blocker: missingforwardRefimport (required for exposing focus).You call
useImperativeHandle, but withoutforwardRefwrapping the component, the ref never reaches the function component.-import { - useCallback, - useEffect, - useImperativeHandle, - useRef, -} from "react"; +import { + forwardRef, + useCallback, + useEffect, + useImperativeHandle, + useRef, +} from "react";
24-29: Ref types belong onforwardRef, not in props.Including
React.RefAttributes<SqlEditorRef>in props implies the ref is passed as a prop, which is not how React refs work for function components. Remove it and rely onforwardRef’s generic instead.-type SqlEditorProps = Omit<EditorProps, "language"> & React.RefAttributes<SqlEditorRef> & { +type SqlEditorProps = Omit<EditorProps, "language"> & { disabled: boolean; /** Callback when the editor is mounted and ref is ready to use. */ onEditorReady?: () => void; };
37-47: Critical: ref is read from props; focus() will never be exposed.Function components don’t receive
refvia props. Wrap withforwardRefand use the forwardedrefwhen callinguseImperativeHandle.-const SqlEditor = (props: SqlEditorProps) => { - const {ref, disabled, onEditorReady, ...editorProps} = props; +const SqlEditor = forwardRef<SqlEditorRef, SqlEditorProps>((props, ref) => { + const {disabled, onEditorReady, ...editorProps} = props; const editorRef = useRef<monaco.editor.IStandaloneCodeEditor>(null); const monacoEditor = useMonaco(); const {token} = theme.useToken(); - useImperativeHandle(ref, () => ({ + useImperativeHandle(ref, () => ({ focus: () => { editorRef.current?.focus(); }, }), []);Remember to close the component with
});at the bottom.
78-86: A11y: advertise disabled state semantically.Add
aria-disabled={disabled}on the wrapper so assistive tech can detect the disabled state; visuals and pointer-events alone aren’t sufficient.- <div + <div + aria-disabled={disabled} style={ disabled ? {pointerEvents: "none"} : {} } >
96-107: Disabled via CSS still allows keyboard edits; set MonacoreadOnly.If the editor is already focused,
pointer-events: nonedoesn’t stop typing. AddreadOnly(and optionallydomReadOnly) to harden the disabled state.options={{ automaticLayout: true, folding: false, fontSize: 16, lineNumbers: "off", minimap: {enabled: false}, overviewRulerBorder: false, placeholder: "Enter your SQL query", renderLineHighlightOnlyWhenFocus: true, scrollBeyondLastLine: false, wordWrap: "on", + readOnly: disabled, + domReadOnly: disabled, }}
108-110: Use Monaco’s canonical light theme id (vs).
"light"isn’t Monaco’s standard id; use"vs"for the default light theme.- theme={disabled ? - "disabled-theme" : - "light"} + theme={disabled ? "disabled-theme" : "vs"}
61-76: Optional: dropcolordependency if tokens are valid CSS colours.If
token.colorBgContainerDisabled/token.colorTextDisabledare already valid CSS strings, pass them directly and remove thecolorpackage usage.- colors: { - "editor.background": color(token.colorBgContainerDisabled).hexa(), - "editor.foreground": color(token.colorTextDisabled).hexa(), + colors: { + "editor.background": token.colorBgContainerDisabled, + "editor.foreground": token.colorTextDisabled,And remove the import if unused:
-import color from "color";If tokens can be non-standard, keep
color(...)and add a short comment explaining why.
15-16: Avoid mixing the top-levelmonacoimport withuseMonaco(); unify on one.Using both sources is confusing. Prefer the instance from
useMonaco()for runtime API calls, and import types only for typing.-import * as monaco from "monaco-editor/esm/vs/editor/editor.api.js"; +import type * as Monaco from "monaco-editor"; @@ - const editorRef = useRef<monaco.editor.IStandaloneCodeEditor>(null); + const editorRef = useRef<Monaco.editor.IStandaloneCodeEditor>(null);Optionally rename
monacoEditortomonacofor clarity:- const monacoEditor = useMonaco(); + const monaco = useMonaco(); @@ - if (null === monacoEditor) { + if (null === monaco) { return; } - monacoEditor.editor.defineTheme("disabled-theme", { + monaco.editor.defineTheme("disabled-theme", {Also applies to: 40-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/webui/client/src/components/SqlEditor/index.tsx(1 hunks)components/webui/client/src/components/SqlEditor/monaco-loader.ts(0 hunks)components/webui/client/src/components/SqlEditor/monaco-sql.d.ts(0 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- components/webui/client/src/components/SqlEditor/monaco-sql.d.ts
- components/webui/client/src/components/SqlEditor/monaco-loader.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsxcomponents/webui/client/src/components/SqlEditor/index.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#1108
File: components/webui/client/src/components/SqlEditor/monaco-config.ts:6-8
Timestamp: 2025-07-21T14:37:57.648Z
Learning: In Monaco Editor configurations for SQL editing, the import of "monaco-editor/esm/vs/editor/editor.all.js" is required for SQL autocomplete functionality to work properly, even when using "editor.api" and "sql.contribution.js". Removing "editor.all.js" breaks the autocomplete feature.
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)
components/webui/client/src/components/SqlEditor/index.tsx (1)
SqlEditorRef(118-118)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (1)
31-33: LGTM: readiness gate is correct.Calling the focus logic only after the editor signals readiness is the right guard.
components/webui/client/src/components/SqlEditor/index.tsx (1)
49-55: LGTM:onEditorReadydispatch afteronMountis correct.Capturing the editor instance and then invoking the user callback ensures the parent’s
isEditorReadystate reflects a usable editor.
| import color from "color"; | ||
| import * as monaco from "monaco-editor/esm/vs/editor/editor.api.js"; | ||
|
|
||
| import "./monaco-loader"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity-check: ensure SQL language contributions remain loaded.
Using our retrieved learnings: we previously broke autocomplete when editor.all.js wasn’t imported in the Monaco setup. Since this PR removes some Monaco contributions/custom SQL declarations, please verify the loader still pulls in the required language and completion contributions (or that you explicitly set the options to achieve the same).
🏁 Script executed:
#!/bin/bash
# Look for language contributions and editor.all.js usage
rg -nP 'editor\.all\.js|sql\.contribution|monaco-editor/esm/vs/editor/editor\.all\.js|basic-languages/sql' -S
# Confirm we still use @monaco-editor/react loader
rg -n 'monaco-loader' components/webui/client/src/components/SqlEditorLength of output: 409
Add missing editor.all.js import in Monaco loader
The loader currently only brings in the SQL contribution, but our previous findings show that without editor.all.js the SQL autocomplete will break. Please update monaco-loader.ts to import the full editor API before the SQL contribution:
• File: components/webui/client/src/components/SqlEditor/monaco-loader.ts
// ensure the full editor API is loaded first
+import "monaco-editor/esm/vs/editor/editor.all.js";
// then register SQL language support
import "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js";This will restore the required SQL autocomplete capabilities.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import "./monaco-loader"; | |
| // ensure the full editor API is loaded first | |
| import "monaco-editor/esm/vs/editor/editor.all.js"; | |
| // then register SQL language support | |
| import "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js"; |
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/index.tsx around line 17 and
in components/webui/client/src/components/SqlEditor/monaco-loader.ts, the Monaco
loader currently only imports the SQL contribution which causes SQL autocomplete
to break; update monaco-loader.ts to import the full Monaco editor API
(editor.all) before importing the SQL contribution so the complete editor
runtime is available for SQL autocomplete to work correctly. Ensure the editor
all import appears earlier than any vs/language or SQL contribution imports and
keep module resolution consistent with the project’s Monaco import style.
| return ( | ||
| <Editor | ||
| language={"sql"} | ||
|
|
||
| // Use white background while loading (default is grey) so transition to editor with | ||
| // white background is less jarring. | ||
| loading={<div style={{backgroundColor: "white", height: "100%", width: "100%"}}/>} | ||
| options={{ | ||
| automaticLayout: true, | ||
| folding: isContentMultiline, | ||
| fontSize: 20, | ||
| lineHeight: 30, | ||
| lineNumbers: isContentMultiline ? | ||
| "on" : | ||
| "off", | ||
| lineNumbersMinChars: 2, | ||
| minimap: {enabled: false}, | ||
| overviewRulerBorder: false, | ||
| placeholder: "Enter your SQL query", | ||
| renderLineHighlightOnlyWhenFocus: true, | ||
| scrollBeyondLastLine: false, | ||
| wordWrap: "on", | ||
| }} | ||
| onMount={handleMonacoMount} | ||
| {...props}/> | ||
| <div | ||
| style={ | ||
| disabled ? | ||
| {pointerEvents: "none"} : | ||
| {} | ||
| } | ||
| > | ||
| <Editor |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
UX consideration: pointer-events disables scrolling; verify that’s desired.
While querying, users cannot scroll or select text with the mouse. If read-only is sufficient, consider dropping pointerEvents: "none" to keep scrolling/selecting available.
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/index.tsx around lines 78 to
86, the current use of style { pointerEvents: "none" } when disabled prevents
scrolling and selection; replace this approach by removing pointerEvents and
instead pass a readOnly (or disabled) prop to the Editor component when
disabled, and if you need to prevent editing but allow selection/scrolling
ensure the wrapper does not set pointerEvents:none and use editor config
(readOnly: true) or CSS (e.g., user-select, overflow) to preserve scrolling and
selection while disabling edits.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
components/webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
components/webui/client/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#1108
File: components/webui/client/src/components/SqlEditor/monaco-config.ts:6-8
Timestamp: 2025-07-21T14:37:57.648Z
Learning: In Monaco Editor configurations for SQL editing, the import of "monaco-editor/esm/vs/editor/editor.all.js" is required for SQL autocomplete functionality to work properly, even when using "editor.api" and "sql.contribution.js". Removing "editor.all.js" breaks the autocomplete feature.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
components/webui/client/package.json (1)
29-29: Supply chain hygiene verified for “color” packageLockfiles are present and committed across the repo, and the “color” dependency metadata aligns with expectations:
- Lockfiles found at:
/components/webui/client/package-lock.json/components/webui/package-lock.json/components/webui/server/package-lock.json- “color” version ^5.0.0 corresponds to the latest 5.0.0 release
- License: MIT; Engines requirement: Node >= 18
No further changes are needed.
|
how about this title: |
davemarco
changed the title
SqlEditor; Focus SqlEditor on page load and after query termination; Remove autocomplete.
|
@junhaoliao for approval |
There was a problem hiding this comment.
Deferring to @hoophalab's review & commit message discussed offline.
|
Please hold off on merging this. We need to add a multiline commit message. |
3 participants
Description
PR does a few things.
Checklist
breaking change.
Validation performed
Focuses correctly. Theme changes during query.
Summary by CodeRabbit
New Features
Chores