feat(webui): Add single-line Monaco Editor component; Refactor search controls into separate components for native and Presto. #1285
Conversation
WalkthroughRefactors SqlEditor’s public API to use an onEditorReady callback returning the Monaco editor instance and removes the ref-based API. Adds a single-line SqlInput component. Reorganizes SearchControls into Native, Presto Freeform, and Presto Guided variants, with runtime selection. Updates SqlQueryInput to the new SqlEditor API and configures editor options. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SearchControls
participant EngineConfig as Engine Config
participant PrestoState as Presto Search State
participant NativeControls
participant FreeformControls
participant GuidedControls
User->>SearchControls: Load page
SearchControls->>EngineConfig: Read SETTINGS_QUERY_ENGINE
alt Engine != PRESTO
SearchControls->>NativeControls: Render
else Engine == PRESTO
SearchControls->>PrestoState: Read sqlInterface
alt sqlInterface == GUIDED
SearchControls->>GuidedControls: Render
else
SearchControls->>FreeformControls: Render
end
end
sequenceDiagram
autonumber
participant Consumer as SqlQueryInput / SqlInput
participant SqlEditor
participant Monaco as Monaco Editor Instance
Consumer->>SqlEditor: Mount with onEditorReady(handler)
SqlEditor-->>Consumer: onEditorReady(editor)
note over Consumer: Store editor ref, set "ready"
Consumer->>Monaco: Register listeners / set options
opt SqlInput only
Monaco-->>Consumer: onDidChangeCursorPosition
Consumer->>Monaco: Normalize to single line, reposition cursor
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
davemarco
changed the title
davemarco
changed the title
davemarco
changed the title
davemarco
changed the title
davemarco
changed the title
![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 (3)
components/webui/client/src/components/SqlEditor/index.tsx (2)
50-71: Theme definitions: consider guarding re-definitionMonaco allows redefining themes, but re-registering on every token change can be avoided with a simple “already defined” guard if tokens are stable enough. Optional.
13-21: Load full Monaco editor and use type-only import
In components/webui/client/src/components/SqlEditor/monaco-loader.ts, add
import "monaco-editor/esm/vs/editor/editor.all.js";before the existing Monaco imports to ensure the full editor and SQL contribution are registered exactly once.
In components/webui/client/src/components/SqlEditor/index.tsx, replace:
-import * as monaco from "monaco-editor/esm/vs/editor/editor.api.js"; +import type * as Monaco from "monaco-editor/esm/vs/editor/editor.api";and update
type SqlEditorType = monaco.editor.IStandaloneCodeEditor;to
type SqlEditorType = Monaco.editor.IStandaloneCodeEditor;so the runtime API isn’t bundled again.
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
17-19: Type-only React import needed for FormEvent.
React.FormEventrequires a React type import under the new JSX transform. Use a type-only import and referenceFormEventdirectly to avoid “Cannot find namespace 'React'” TS errors.Apply this diff:
+import type {FormEvent} from "react"; @@ -const handleSubmit = (ev: React.FormEvent<HTMLFormElement>) => { +const handleSubmit = (ev: FormEvent<HTMLFormElement>) => { ev.preventDefault(); }
📜 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 (8)
components/webui/client/src/components/SqlEditor/index.tsx(3 hunks)components/webui/client/src/components/SqlInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/NativeControls.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/FreeformControls.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/index.tsx(1 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/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/NativeControls.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/FreeformControls.tsxcomponents/webui/client/src/components/SqlInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsxcomponents/webui/client/src/components/SqlEditor/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-21T14:37:57.648Z
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.
Applied to files:
components/webui/client/src/components/SqlEditor/index.tsx
🧬 Code graph analysis (4)
components/webui/client/src/pages/SearchPage/SearchControls/NativeControls.tsx (1)
components/webui/client/src/config/index.ts (2)
CLP_STORAGE_ENGINES(26-26)SETTINGS_STORAGE_ENGINE(28-28)
components/webui/client/src/components/SqlInput/index.tsx (1)
components/webui/client/src/components/SqlEditor/index.tsx (2)
SqlEditorProps(107-107)SqlEditorType(108-108)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
components/webui/client/src/config/index.ts (2)
SETTINGS_QUERY_ENGINE(27-27)CLP_QUERY_ENGINES(25-25)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (1)
components/webui/client/src/components/SqlEditor/index.tsx (1)
SqlEditorType(108-108)
🔇 Additional comments (6)
components/webui/client/src/pages/SearchPage/index.tsx (1)
29-30: LGTM — no functional changeThe extra blank line is benign. If the formatter flags it later, we can remove it; otherwise fine as-is.
components/webui/client/src/pages/SearchPage/SearchControls/NativeControls.tsx (2)
18-30: LGTM: clean, focused presentational component.Composition and CSS-module usage look good. No issues spotted with the basic structure.
21-21: Render logic is correct for static engine configuration
SETTINGS_STORAGE_ENGINE is loaded once at startup (from settings.ClpStorageEngine) and isn’t runtime-configurable, so this conditional behaves as intended. If you add a live storage-engine toggle later, wire it through a store selector or pass a prop.Likely an incorrect or invalid review comment.
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
30-37: Three-mode controls routing reads clearly.The
controlsbranching is simple and easy to follow. Nice consolidation versus scattered conditionals.components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)
29-33: onEditorReady handoff looks good.Assigning the editor instance via the callback aligns with the new SqlEditor API and removes ref coupling.
56-70: Confirm Monaco options compatibility.Ensure
placeholder,padding, andrenderLineHighlight: "none"are supported by the monaco version bundled in your app. Otherwise, guard or strip unsupported options.Would you like me to check your monaco-editor version and map these options to the documented API surface?
| const {disabled, onEditorReady, ...editorProps} = props; | ||
| const monacoEditor = useMonaco(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disabled state is cosmetic only; make editor truly read-only and accessible when disabled
Pointer-events + theme does not prevent keyboard input or programmatic focus. Set readOnly via editor options and expose aria-disabled.
-const {disabled, onEditorReady, ...editorProps} = props;
+const {disabled, onEditorReady, ...editorProps} = props;
+const mergedOptions = {
+ ...(editorProps.options ?? {}),
+ readOnly: disabled,
+};
@@
- <div
+ <div
+ aria-disabled={disabled}
style={{
border: `1px solid ${token.colorBorder}`,
borderRadius: token.borderRadius,
pointerEvents: disabled ?
"none" :
"auto",
}}
>
<Editor
language={"sql"}
@@
onMount={handleEditorDidMount}
- {...editorProps}/>
+ options={mergedOptions}
+ {...editorProps}/>Also applies to: 76-85, 86-101
| const handleEditorReady = useCallback((editor: SqlEditorType) => { | ||
| // Prevent multi-line input by repositioning cursor and replacing newlines with empty | ||
| // string. | ||
| editor.onDidChangeCursorPosition((e) => { | ||
| if (1 < e.position.lineNumber) { | ||
| editor.setValue(editor.getValue().replace(/\r?\n/g, "")); | ||
| editor.setPosition({ | ||
| lineNumber: 1, | ||
| column: Infinity, | ||
| }); | ||
| } | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Single-line enforcement: avoid Infinity and handle Enter/paste directly
Using Infinity for the column relies on clamping; use getLineMaxColumn(1). Also intercept Enter and sanitise pasted multi-line text to reduce cursor jumps and undo noise.
- const handleEditorReady = useCallback((editor: SqlEditorType) => {
- // Prevent multi-line input by repositioning cursor and replacing newlines with empty
- // string.
- editor.onDidChangeCursorPosition((e) => {
- if (1 < e.position.lineNumber) {
- editor.setValue(editor.getValue().replace(/\r?\n/g, ""));
- editor.setPosition({
- lineNumber: 1,
- column: Infinity,
- });
- }
- });
- }, []);
+ const handleEditorReady = useCallback((editor: SqlEditorType) => {
+ const model = editor.getModel();
+ if (null == model) {
+ return;
+ }
+ // Block Enter key before a newline is inserted.
+ editor.onKeyDown((e) => {
+ const k = e.browserEvent?.key;
+ if ("Enter" === k) {
+ e.preventDefault();
+ }
+ });
+ // Sanitize any newlines from paste or programmatic updates.
+ editor.onDidChangeModelContent(() => {
+ const value = model.getValue();
+ const sanitized = value.replace(/\r?\n/g, "");
+ if (sanitized !== value) {
+ model.setValue(sanitized);
+ const col = model.getLineMaxColumn(1);
+ editor.setPosition({lineNumber: 1, column: col});
+ }
+ });
+ }, []);📝 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.
| const handleEditorReady = useCallback((editor: SqlEditorType) => { | |
| // Prevent multi-line input by repositioning cursor and replacing newlines with empty | |
| // string. | |
| editor.onDidChangeCursorPosition((e) => { | |
| if (1 < e.position.lineNumber) { | |
| editor.setValue(editor.getValue().replace(/\r?\n/g, "")); | |
| editor.setPosition({ | |
| lineNumber: 1, | |
| column: Infinity, | |
| }); | |
| } | |
| }); | |
| }, []); | |
| const handleEditorReady = useCallback((editor: SqlEditorType) => { | |
| const model = editor.getModel(); | |
| if (null == model) { | |
| return; | |
| } | |
| // Block Enter key before a newline is inserted. | |
| editor.onKeyDown((e) => { | |
| const k = e.browserEvent?.key; | |
| if ("Enter" === k) { | |
| e.preventDefault(); | |
| } | |
| }); | |
| // Sanitize any newlines from paste or programmatic updates. | |
| editor.onDidChangeModelContent(() => { | |
| const value = model.getValue(); | |
| const sanitized = value.replace(/\r?\n/g, ""); | |
| if (sanitized !== value) { | |
| model.setValue(sanitized); | |
| const col = model.getLineMaxColumn(1); | |
| editor.setPosition({ lineNumber: 1, column: col }); | |
| } | |
| }); | |
| }, []); |
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlInput/index.tsx around lines 17 to
29, the cursor reposition uses Infinity and only reacts to cursor moves which
causes clamping, cursor jumps, and noisy undo behavior; change to use
editor.getModel().getLineMaxColumn(1) when setting position instead of Infinity,
add an explicit keybinding/command handler for the Enter key to prevent
inserting newlines, and listen for paste events (or intercept clipboard input)
to sanitize pasted text by stripping CR/LF before inserting so multi-line input
is removed without extra cursor jumps or undo noise.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/FreeformControls.tsx
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/FreeformControls.tsx
Show resolved
Hide resolved
| <SqlInput disabled={false}/> | ||
| <div className={styles["statusAndButtonsRow"]}> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Drive disabled state from search/query status instead of hardcoding false
Wire SqlInput.disabled to the relevant “busy/running” state so keyboard input is blocked during execution.
Example (adapt names to your state hook):
- <SqlInput disabled={false}/>
+ <SqlInput disabled={state.isRunning}/>📝 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.
| <SqlInput disabled={false}/> | |
| <div className={styles["statusAndButtonsRow"]}> | |
| <SqlInput disabled={state.isRunning}/> | |
| <div className={styles["statusAndButtonsRow"]}> |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx
around lines 15-16, SqlInput.disabled is hardcoded to false; instead read the
component's search/query running state (e.g., isRunning, isExecuting, or similar
from the relevant hook or context) and pass that boolean into the
SqlInput.disabled prop so the input is disabled while a query is in progress;
ensure you import/use the correct state hook or context, map its value to the
prop (negate only if your state is named inversely), and update any prop types
if necessary.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
Outdated
Show resolved
Hide resolved
| ]); | ||
|
|
||
| return ( | ||
| <div className={styles["input"] || ""}> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify className expression.
styles["input"] || "" is redundant with CSS modules; the key should always exist. Prefer direct access.
Apply this diff:
- <div className={styles["input"] || ""}>
+ <div className={styles.input}>📝 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.
| <div className={styles["input"] || ""}> | |
| <div className={styles.input}> |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
around line 49, the className uses a redundant fallback styles["input"] || "";
replace this with direct access to the CSS module key (e.g.,
className={styles.input}) to simplify and clarify the expression.
Yes i now removes those old files |
There was a problem hiding this comment.
Actionable comments posted: 1
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)
45-75: Ensure custom theme applies on first render.DefineTheme runs post-mount; set the theme when monaco loads/toggles disabled to avoid fallback to “vs”.
useEffect(() => { if (null === monacoEditor) { return; } @@ }, [ monacoEditor, token, ]); + + useEffect(() => { + if (null === monacoEditor) { + return; + } + monacoEditor.editor.setTheme(disabled ? "disabled-theme" : "default-theme"); + }, [monacoEditor, disabled]);
♻️ Duplicate comments (9)
components/webui/client/src/components/SqlEditor/index.tsx (1)
34-35: Make disabled state functional and accessible (readOnly + aria-disabled).Pointer-events + theme does not prevent keyboard input or programmatic focus. Set readOnly via options and expose aria-disabled so SRs pick it up.
Apply:
- const {disabled, onEditorReady, ...editorProps} = props; + const {disabled, onEditorReady, options: passedOptions, ...editorProps} = props; @@ - <div + <div + aria-disabled={disabled} style={{ border: `1px solid ${token.colorBorder}`, borderRadius: token.borderRadius, pointerEvents: disabled ? "none" : "auto", }} > <Editor language={"sql"} @@ - onMount={handleEditorDidMount} - {...editorProps}/> + onMount={handleEditorDidMount} + options={{ + ...(passedOptions ?? {}), + readOnly: disabled, + }} + {...editorProps}/>Also applies to: 77-85, 86-101
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx (1)
15-15: Don't hardcode disabled=false; bind to query running stateWire SqlInput.disabled to the Presto search/query busy state so input is read-only during execution.
- <SqlInput disabled={false}/> + <SqlInput disabled={isRunning}/>Additional change outside this hunk (imports and component body):
import usePrestoSearchState from "../SearchState/Presto"; const GuidedControls = () => { const isRunning = usePrestoSearchState((s) => s.isRunning /* or s.isExecuting */); return ( <div className={styles["searchControlsContainer"]}> <SqlInput disabled={isRunning}/> ... </div> ); };components/webui/client/src/components/SqlInput/index.tsx (2)
17-29: Enforce single-line robustly (block Enter, sanitise on content change, avoid Infinity)Current approach causes cursor jumps and relies on clamping. Use keydown + content sanitisation and getLineMaxColumn(1).
- const handleEditorReady = useCallback((editor: SqlEditorType) => { - // Prevent multi-line input by repositioning cursor and replacing newlines with empty - // string. - editor.onDidChangeCursorPosition((e) => { - if (1 < e.position.lineNumber) { - editor.setValue(editor.getValue().replace(/\r?\n/g, "")); - editor.setPosition({ - lineNumber: 1, - column: Infinity, - }); - } - }); - }, []); + const handleEditorReady = useCallback((editor: SqlEditorType) => { + const model = editor.getModel(); + if (null == model) { + return; + } + // Block Enter before newline insertion. + editor.onKeyDown((e) => { + if ("Enter" === e.browserEvent?.key) { + e.preventDefault(); + } + }); + // Strip any CR/LF from paste/programmatic updates and place cursor at end of line 1. + editor.onDidChangeModelContent(() => { + const value = model.getValue(); + const sanitized = value.replace(/\r?\n/g, ""); + if (sanitized !== value) { + model.setValue(sanitized); + const col = model.getLineMaxColumn(1); + editor.setPosition({ lineNumber: 1, column: col }); + } + }); + // Allow callers to extend behaviour. + props.onEditorReady?.(editor); + }, [props.onEditorReady]);
31-66: Prevent callers from overriding enforced single-line optionsProps spread last lets consumers bypass single-line settings. Merge props.options under enforced options and spread props earlier.
- <SqlEditor - height={30} - options={{ + <SqlEditor + {...props} + height={30} + options={{ + ...(props.options ?? {}), find: { addExtraSpaceOnTop: false, autoFindInSelection: "never", seedSearchStringFromSelection: "never", }, fixedOverflowWidgets: true, folding: false, glyphMargin: false, hideCursorInOverviewRuler: true, lineNumbers: "off", lineNumbersMinChars: 0, minimap: {enabled: false}, overviewRulerBorder: false, overviewRulerLanes: 0, padding: { top: 4, bottom: 4, }, renderLineHighlight: "none", roundedSelection: false, scrollBeyondLastColumn: 0, scrollBeyondLastLine: false, scrollbar: { horizontal: "hidden", vertical: "hidden", alwaysConsumeMouseWheel: false, }, wordWrap: "off", }} - onEditorReady={handleEditorReady} - {...props}/> + onEditorReady={handleEditorReady}/>components/webui/client/src/pages/SearchPage/SearchControls/Presto/FreeformControls.tsx (2)
10-10: Use Vite env dot-notationIdiomatic and consistent: import.meta.env.VITE_GUIDED_DEV.
-const isGuidedEnabled = "true" === import.meta.env["VITE_GUIDED_DEV"]; +const isGuidedEnabled = "true" === import.meta.env.VITE_GUIDED_DEV;
25-25: Centralise the guided toggle sourceRead the toggle from the same central state/selector that SearchControls uses so removal is one change later.
Would you like me to wire this to the shared selector? If so, point me at the source used by SearchControls, or I can scan and propose the exact import.
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
31-37: Make engine gating reactive (avoid config snapshot)SETTINGS_QUERY_ENGINE is static at load; UI won’t switch if users change engine at runtime. Read engine from a reactive store/hook.
#!/bin/bash # Find reactive engine sources to replace SETTINGS_QUERY_ENGINE rg -n -C2 --hidden --glob '!**/dist/**' \ '\b(use.*engine|engine.*store|settings.*store|queryEngine\b|SETTINGS_QUERY_ENGINE\b)'Expected: a hook/selector exposing current query engine (e.g., useSettings((s) => s.queryEngine)). Replace the constant check with the reactive value.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)
49-49: Simplify className access (CSS modules).The fallback is unnecessary.
- <div className={styles["input"] || ""}> + <div className={styles.input}>
21-21: Fix useRef nullability type (strictNullChecks).Initialising with null requires a nullable generic.
- const editorRef = useRef<SqlEditorType>(null); + const editorRef = useRef<SqlEditorType | null>(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
components/webui/client/src/components/SqlEditor/index.tsx(3 hunks)components/webui/client/src/components/SqlInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/NativeControls.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/FreeformControls.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/index.tsx(1 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/GuidedControls.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/NativeControls.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/FreeformControls.tsxcomponents/webui/client/src/components/SqlInput/index.tsxcomponents/webui/client/src/pages/SearchPage/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/client/src/components/SqlEditor/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
⏰ 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). (2)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (6)
components/webui/client/src/pages/SearchPage/SearchControls/NativeControls.tsx (1)
18-30: Well-scoped extraction; dataset gating looks correct for CLP‑S.The composition and conditional Dataset render read cleanly and match the CLP_S requirement.
components/webui/client/src/components/SqlEditor/index.tsx (1)
18-25: API change LGTM.New SqlEditorType alias and onEditorReady(editor) shape are clear and reduce ref coupling.
Also applies to: 106-109
components/webui/client/src/pages/SearchPage/index.tsx (1)
29-31: LGTM (whitespace only).No functional changes; safe to keep.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (3)
8-8: LGTM: SqlEditor import and type usage match the new API.
29-32: LGTM: onEditorReady wiring is correct.Ref assignment + readiness flag are appropriate.
56-70: Could you share your package.json (or at least themonaco-editorversion listed in your dependencies) so I can confirm whether that version’sIStandaloneEditorConstructionOptionsincludes aplaceholderfield?
| const GuidedControls = () => ( | ||
| <div className={styles["searchControlsContainer"]}> | ||
| <SqlInput disabled={false}/> | ||
| <div className={styles["statusAndButtonsRow"]}> | ||
| <div className={styles["status"]}> | ||
| <QueryStatus/> | ||
| </div> | ||
| <div className={styles["buttons"]}> | ||
| <SqlInterfaceButton/> | ||
| <SqlSearchButton/> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert to function body to use hooks
You’ll need a block body to read state (see previous comment). Wrap the JSX in a function body and return it.
-const GuidedControls = () => (
- <div className={styles["searchControlsContainer"]}>
- <SqlInput disabled={false}/>
- <div className={styles["statusAndButtonsRow"]}>
- <div className={styles["status"]}>
- <QueryStatus/>
- </div>
- <div className={styles["buttons"]}>
- <SqlInterfaceButton/>
- <SqlSearchButton/>
- </div>
- </div>
- </div>
-);
+const GuidedControls = () => {
+ const isRunning = usePrestoSearchState((s) => s.isRunning /* verify name */);
+ return (
+ <div className={styles["searchControlsContainer"]}>
+ <SqlInput disabled={isRunning}/>
+ <div className={styles["statusAndButtonsRow"]}>
+ <div className={styles["status"]}>
+ <QueryStatus/>
+ </div>
+ <div className={styles["buttons"]}>
+ <SqlInterfaceButton/>
+ <SqlSearchButton/>
+ </div>
+ </div>
+ </div>
+ );
+};📝 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.
| const GuidedControls = () => ( | |
| <div className={styles["searchControlsContainer"]}> | |
| <SqlInput disabled={false}/> | |
| <div className={styles["statusAndButtonsRow"]}> | |
| <div className={styles["status"]}> | |
| <QueryStatus/> | |
| </div> | |
| <div className={styles["buttons"]}> | |
| <SqlInterfaceButton/> | |
| <SqlSearchButton/> | |
| </div> | |
| </div> | |
| </div> | |
| ); | |
| const GuidedControls = () => { | |
| const isRunning = usePrestoSearchState((s) => s.isRunning /* verify name */); | |
| return ( | |
| <div className={styles["searchControlsContainer"]}> | |
| <SqlInput disabled={isRunning}/> | |
| <div className={styles["statusAndButtonsRow"]}> | |
| <div className={styles["status"]}> | |
| <QueryStatus/> | |
| </div> | |
| <div className={styles["buttons"]}> | |
| <SqlInterfaceButton/> | |
| <SqlSearchButton/> | |
| </div> | |
| </div> | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx
around lines 13 to 26, the component is currently an implicit-return arrow
function (no block body) which prevents adding hooks or reading state; change it
to a block-bodied function (e.g., const GuidedControls = () => { /* use
hooks/state here */ return ( ...JSX... ); };) so you can call hooks before
returning, keeping the existing JSX structure and classNames intact.
| const SqlQueryInput = () => { | ||
| const searchUiState = useSearchStore((state) => state.searchUiState); | ||
| const editorRef = useRef<Nullable<SqlEditorRef>>(null); | ||
| const editorRef = useRef<SqlEditorType>(null); |
There was a problem hiding this comment.
How about
| const editorRef = useRef<SqlEditorType>(null); | |
| const editorRef = useRef<Nullable<SqlEditorType>>(null); |
There was a problem hiding this comment.
i think the useRef adds the nullable type automatically so not neccesary
Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
I replied with reasoning in chat |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)
22-22: Good fix: ref is correctly nullable now.Matches
strictNullChecksand resolves the earlier review concern.
50-50: Simplify className access with CSS modules.Indexing plus fallback is unnecessary.
- <div className={styles["input"] || ""}> + <div className={styles.input}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(3 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.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 (1)
components/webui/client/src/components/SqlEditor/index.tsx (1)
SqlEditorType(108-108)
⏰ 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: package-image
🔇 Additional comments (3)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (3)
12-12: LGTM on Nullable import usage.
30-33: onEditorReady flow looks solid.Storing the editor instance and gating focus on readiness is clean.
57-71: Ensure Monaco Editor version ≥ 0.51.0
Theplaceholderoption was introduced in monaco-editor v0.51.0—verify your package.json dependency is at least this version or remove theplaceholdersetting.
| import {Nullable} from "src/typings/common"; | ||
|
|
||
| import SqlEditor, {SqlEditorRef} from "../../../../../components/SqlEditor"; | ||
| import SqlEditor, {SqlEditorType} from "../../../../../components/SqlEditor"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer a type-only import for SqlEditorType.
Avoids a potential runtime import and helps treeshaking.
-import SqlEditor, {SqlEditorType} from "../../../../../components/SqlEditor";
+import SqlEditor, { type SqlEditorType } from "../../../../../components/SqlEditor";📝 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 SqlEditor, {SqlEditorType} from "../../../../../components/SqlEditor"; | |
| import SqlEditor, { type SqlEditorType } from "../../../../../components/SqlEditor"; |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
around line 8, the current import brings SqlEditorType at runtime; change it to
a type-only import so the type is erased at runtime (e.g. keep the default/sql
component import as-is and add/import SqlEditorType using a TypeScript type-only
import) to avoid a runtime import and improve tree-shaking.
💡 Verification agent
🧩 Analysis chain
Verify Monaco SQL autocomplete prerequisite remains in place.
Per your prior note (2025-07-21), SQL autocomplete breaks without importing monaco-editor/esm/vs/editor/editor.all.js. Confirm the refactor to SqlEditor still includes this import (likely in its Monaco config).
I can help you check with this script:
🏁 Script executed:
#!/bin/bash
# Find any import of editor.all.js to ensure autocomplete stays enabled
rg -nP 'monaco-editor/esm/vs/editor/editor\.all\.js' -g '!**/node_modules/**' -C2Length of output: 148
🏁 Script executed:
#!/bin/bash
# Locate monaco-config.ts and verify import of editor.all.js
find . -type f -name monaco-config.ts | while read p; do
echo
echo "Inspecting $p"
grep -n "editor\.all\.js" -C2 "$p" || echo "No occurrence of editor.all.js in $p"
done
# Also search entire SqlEditor folder for any editor.all.js reference
echo
echo "Searching entire SqlEditor folder for editor.all.js imports"
rg -n "editor\.all\.js" -C2 components/webui/client/src/components/SqlEditor || echo "No editor.all.js imports found in SqlEditor folder"Length of output: 111
🏁 Script executed:
#!/bin/bash
# Locate any files with “monaco” in their names to find where configuration may live
find . -type f | grep -i monaco
# Search for any import of monaco-editor across the codebase (excluding node_modules)
rg -n "import .*monaco-editor" -g '!**/node_modules/**' -C2Length of output: 216
🏁 Script executed:
#!/bin/bash
# Inspect monaco-loader.ts for any reference to editor.all.js or editor.all
file="components/webui/client/src/components/SqlEditor/monaco-loader.ts"
echo "Inspecting $file for editor.all.js import"
grep -nP "editor\.all\.js" -C2 "$file" || echo "No direct import of editor.all.js in $file"
echo
echo "Inspecting $file for 'editor.all' usage"
grep -n "editor\.all" -C2 "$file" || echo "No 'editor.all' references in $file"Length of output: 401
Restore missing editor.all.js import in Monaco loader
The import of monaco-editor/esm/vs/editor/editor.all.js was removed during the SqlEditor refactor and is required for SQL autocomplete to function (see monaco-loader.ts). Re-add this import at the top of components/webui/client/src/components/SqlEditor/monaco-loader.ts before any other Monaco imports.
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/monaco-loader.ts at the top
of the file before any other Monaco imports, re-add the missing import statement
for monaco-editor/esm/vs/editor/editor.all.js so the Monaco loader includes the
editor.all bundle required for SQL autocomplete; place this import as the very
first import in the module (above other 'monaco-editor/...' imports) to ensure
the full editor features are registered before the rest of the Monaco
initialization code runs.
| const handleChange = useCallback((value: string | undefined) => { | ||
| const {updateQueryString} = useSearchStore.getState(); | ||
| updateQueryString(value || ""); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use nullish coalescing for clarity.
Avoids truthiness pitfalls and documents intent.
- updateQueryString(value || "");
+ updateQueryString(value ?? "");📝 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.
| const handleChange = useCallback((value: string | undefined) => { | |
| const {updateQueryString} = useSearchStore.getState(); | |
| updateQueryString(value || ""); | |
| }, []); | |
| const handleChange = useCallback((value: string | undefined) => { | |
| const {updateQueryString} = useSearchStore.getState(); | |
| updateQueryString(value ?? ""); | |
| }, []); |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
around lines 25 to 28, the handler currently uses a truthiness fallback (value
|| "") which can mis-handle empty strings or other falsy but valid values;
replace that with a nullish coalescing fallback (value ?? "") when calling
updateQueryString to explicitly only default on null or undefined and preserve
valid falsy inputs.
| options={{ | ||
| automaticLayout: true, | ||
| folding: false, | ||
| lineNumbers: "off", | ||
| minimap: {enabled: false}, | ||
| overviewRulerBorder: false, | ||
| padding: { | ||
| top: 8, | ||
| bottom: 8, | ||
| }, | ||
| placeholder: "Enter your SQL query", | ||
| renderLineHighlight: "none", | ||
| scrollBeyondLastLine: false, | ||
| wordWrap: "on", | ||
| }} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add an accessible label to the editor.
Improves screen-reader UX without behaviour changes.
options={{
automaticLayout: true,
folding: false,
lineNumbers: "off",
minimap: {enabled: false},
overviewRulerBorder: false,
+ ariaLabel: "SQL query input",
padding: {
top: 8,
bottom: 8,
},
placeholder: "Enter your SQL query",
renderLineHighlight: "none",
scrollBeyondLastLine: false,
wordWrap: "on",
}}📝 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.
| options={{ | |
| automaticLayout: true, | |
| folding: false, | |
| lineNumbers: "off", | |
| minimap: {enabled: false}, | |
| overviewRulerBorder: false, | |
| padding: { | |
| top: 8, | |
| bottom: 8, | |
| }, | |
| placeholder: "Enter your SQL query", | |
| renderLineHighlight: "none", | |
| scrollBeyondLastLine: false, | |
| wordWrap: "on", | |
| }} | |
| options={{ | |
| automaticLayout: true, | |
| folding: false, | |
| lineNumbers: "off", | |
| minimap: { enabled: false }, | |
| overviewRulerBorder: false, | |
| ariaLabel: "SQL query input", | |
| padding: { | |
| top: 8, | |
| bottom: 8, | |
| }, | |
| placeholder: "Enter your SQL query", | |
| renderLineHighlight: "none", | |
| scrollBeyondLastLine: false, | |
| wordWrap: "on", | |
| }} |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
around lines 57 to 71, the Monaco editor options lack an accessible name for
screen readers; add an accessible label by adding an ariaLabel option (e.g.,
ariaLabel: "SQL query editor") to the options object, or alternatively add
ariaLabelledBy pointing to a visually-hidden <label> element that you create
above the editor and give that element the matching id; ensure the chosen
approach provides a clear, concise name for the editor without changing visual
behavior.
3 participants
Description
Adds a single line editor for monaco.
Specifically it disables many features in the monaco editor, and events that would cause a newline, and removes the newline. Such events are like pressing enter or pasting content with new line.
I also refactored the search controls components.
To see PR, u need to run
npm run guidedChecklist
breaking change.
Validation performed
Pressed enter, and pasted multiline content into editor
Summary by CodeRabbit
New Features
Refactor