feat(webui): Add SQL input boxes to guided presto UI. #1300
Conversation
WalkthroughRefactors and generalizes InputLabel; introduces CSS modules for SqlEditor and CaseSensitive button; adjusts UI sizes to "middle"; replaces legacy Presto GuidedControls with a new modular grid-based implementation (Select, From, Where, OrderBy, Limit) and a composed index. Minor SqlInput/SqlEditor prop and styling updates; wiring updates in Dataset control. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant GuidedControls as GuidedControls (Presto)
participant Select as SELECT
participant From as FROM
participant Where as WHERE
participant Order as ORDER BY
participant Limit as LIMIT
participant SqlInput as SqlInput
participant Monaco as Monaco Editor
participant Status as QueryStatus
participant Buttons as Action Buttons
User->>GuidedControls: Open guided mode
GuidedControls->>Select: Render InputLabel + SqlInput
GuidedControls->>From: Render InputLabel + SqlInput
GuidedControls->>Where: Render InputLabel + SqlInput
GuidedControls->>Order: Render InputLabel + SqlInput
GuidedControls->>Limit: Render InputLabel + Select
note over SqlInput,Monaco: SqlEditor uses CSS module .editor and automaticLayout
User->>Buttons: Click Search/Cancel
Buttons->>Status: Update status
Status-->>User: Show query status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
components/webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx (3)
35-40: Follow project guideline: avoid logical NOT.Replace the negation with an explicit comparison.
Apply:
- const handleButtonClick = useCallback(() => { - onCaseSensitiveChange(!isCaseSensitive); - }, [ + const handleButtonClick = useCallback(() => { + onCaseSensitiveChange(false === isCaseSensitive); + }, [ isCaseSensitive, onCaseSensitiveChange, ]);
56-66: Accessibility: indicate toggle state.Expose state to assistive tech via aria-pressed and provide an aria-label.
Apply:
- <Button + <Button className={styles["customButton"] || ""} disabled={disabled} size={"small"} variant={buttonVariant} color={isCaseSensitive ? "primary" : "default"} + aria-pressed={isCaseSensitive} + aria-label={"Match case"} onClick={handleButtonClick} >
1-1: Typo in filename: rename for consistency.Consider renaming the file to CaseSensitiveToggle.tsx and updating imports to match the component name.
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (1)
91-91: Align control size with the rest of the UI.Most controls moved to
size="middle". Consider updating the dataset<Select>for visual consistency unless there’s a deliberate exception.- size={"large"} + size={"middle"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
components/webui/client/src/components/InputLabel/index.module.css(1 hunks)components/webui/client/src/components/InputLabel/index.tsx(1 hunks)components/webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx(2 hunks)components/webui/client/src/components/QueryBox/InputWithCaseSensitive/index.module.css(1 hunks)components/webui/client/src/components/SqlEditor/index.tsx(1 hunks)components/webui/client/src/components/SqlInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx(0 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx
🧰 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/QueryInput/index.tsxcomponents/webui/client/src/components/SqlInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsxcomponents/webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsxcomponents/webui/client/src/components/SqlEditor/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsxcomponents/webui/client/src/components/InputLabel/index.tsx
🔇 Additional comments (14)
components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)
64-64: Consistent control density — OK.Switching both controls to size "middle" aligns with the rest of the Search controls and has no behavioural impact.
Also applies to: 78-78
components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx (1)
45-45: Consistent sizing — OK.size="middle" matches the rest of the toolbar and preserves behaviour.
components/webui/client/src/components/SqlEditor/index.tsx (1)
81-84: Fill-and-shrink fix — good change.height/width 100% with minHeight/minWidth 0 resolves grid/flex shrink issues for Monaco.
Also applies to: 87-87
components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx (1)
86-86: Consistent sizing — OK.Submit button size aligns with the rest; logic unchanged.
components/webui/client/src/components/InputLabel/index.module.css (1)
1-12: Label padding/no-wrap — OK.Padding and nowrap improve alignment in tight grids. Looks good.
components/webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx (1)
67-67: Confirm QueryBox supports size="middle".AntD Input supports "middle", but QueryBox is custom. Please verify it maps size through correctly and doesn’t regress vertical rhythm with neighbouring controls.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (1)
9-9: Import re-order is fine.The
Nullableimport relocation is safe; no behavioural impact.components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
9-9: Explicit index import looks good.Module resolution should be identical in dev/prod for this path.
components/webui/client/src/components/SqlInput/index.tsx (1)
34-34: Enabling automaticLayout is the right call.Matches Monaco best practices and the new responsive grid.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.tsx (1)
18-37: GuidedControls composition looks good.Stateless wrapper with grid and status/buttons reads clean and aligns with the modular direction.
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (1)
84-84: Label refactor: looks good.Switch to
<InputLabel>with children reads cleaner and matches the new shared label component.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx (1)
1-3: Add tsconfig path aliases to avoid deep relative imports
- Deep relative imports (e.g. ../../../../../components/InputLabel) are brittle — add compilerOptions.baseUrl and paths (e.g. "@components/": ["components/"]) and update imports.
- Automated check couldn't parse these tsconfig files: components/webui/client/tsconfig.json, components/webui/client/tsconfig/tsconfig.base.json, components/webui/client/tsconfig/tsconfig.app.json, components/webui/client/tsconfig/tsconfig.node.json, components/webui/common/tsconfig.json, components/webui/server/tsconfig.json — verify compilerOptions.baseUrl/paths (files contain JSONC/comments; run a JSONC-aware inspection).
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (1)
11-15: A11y linkage for Select.Make InputLabel render label semantics and attach it to the SqlInput by adding htmlFor on InputLabel and a matching id on SqlInput; verify InputLabel forwards/accepts htmlFor (or supports as="label") and SqlInput forwards id to the underlying input.
File: components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (1)
11-15: A11y: link FROM label to its input.Use native label semantics and add an id on the input:
- <InputLabel>FROM</InputLabel> - <SqlInput /> + <InputLabel as="label" htmlFor="presto-from">FROM</InputLabel> + <SqlInput id="presto-from" />Confirmed CSS: .select, .from, .where exist in components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css. Ensure InputLabel supports the polymorphic
as/htmlForprops and SqlInput forwardsid; if not, render a native with the module class instead.
| /** | ||
| * Renders a label for the dataset selector. | ||
| * Renders a label for an input field. | ||
| * | ||
| * @param props | ||
| * @param props.children The label text to display. | ||
| * @return |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document a11y props.
Add docs for as and htmlFor to clarify usage as a real label.
/**
- * Renders a label for an input field.
+ * Renders a label for an input field.
*
- * @param props
- * @param props.children The label text to display.
+ * @param props
+ * @param props.children The label text to display.
+ * @param props.as Render as a native "label" or as a "span" (Typography.Text). Default: "span".
+ * @param props.htmlFor When as="label", associates the label to a control id.📝 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.
| /** | |
| * Renders a label for the dataset selector. | |
| * Renders a label for an input field. | |
| * | |
| * @param props | |
| * @param props.children The label text to display. | |
| * @return | |
| /** | |
| * Renders a label for an input field. | |
| * | |
| * @param props | |
| * @param props.children The label text to display. | |
| * @param props.as Render as a native "label" or as a "span" (Typography.Text). Default: "span". | |
| * @param props.htmlFor When as="label", associates the label to a control id. | |
| * @return | |
| */ |
🤖 Prompt for AI Agents
In components/webui/client/src/components/InputLabel/index.tsx around lines 11
to 16, the JSDoc block for the component is missing documentation for the
accessible props `as` and `htmlFor`; update the comment to describe that `as`
lets callers render a custom element type (e.g., 'label' or 'div') and that
`htmlFor` should be used when rendering a real <label> to associate the label
with an input by id, including expected types and accessibility notes (e.g.,
when to pass `htmlFor` and that it must match the input id).
| const InputLabel = ({children}: {children: React.ReactNode}) => { | ||
| const {token} = theme.useToken(); | ||
|
|
||
| return ( | ||
| <Text | ||
| className={styles["datasetLabel"] || ""} | ||
| className={styles["label"] || ""} | ||
| style={{ | ||
| backgroundColor: token.colorFillTertiary, | ||
| borderBottomLeftRadius: `${token.borderRadius}px`, | ||
| borderColor: token.colorBorder, | ||
| borderTopLeftRadius: `${token.borderRadius}px`, | ||
| fontSize: token.fontSizeLG, | ||
| fontSize: token.fontSize, | ||
| }} | ||
| > | ||
| Dataset | ||
| {children} | ||
| </Text> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable semantic rendering and clean className handling.
This improves accessibility and removes the unnecessary || "".
-const InputLabel = ({children}: {children: React.ReactNode}) => {
+type InputLabelProps = {
+ children: React.ReactNode;
+ as?: "span" | "label";
+ htmlFor?: string;
+ id?: string;
+ className?: string;
+};
+
+const InputLabel = ({children, as = "span", htmlFor, id, className}: InputLabelProps) => {
const {token} = theme.useToken();
- return (
- <Text
- className={styles["label"] || ""}
- style={{
- backgroundColor: token.colorFillTertiary,
- borderBottomLeftRadius: `${token.borderRadius}px`,
- borderColor: token.colorBorder,
- borderTopLeftRadius: `${token.borderRadius}px`,
- fontSize: token.fontSize,
- }}
- >
- {children}
- </Text>
- );
+ const commonProps = {
+ id,
+ className: `${styles.label} ${className ?? ""}`.trim(),
+ style: {
+ backgroundColor: token.colorFillTertiary,
+ borderBottomLeftRadius: `${token.borderRadius}px`,
+ borderColor: token.colorBorder,
+ borderTopLeftRadius: `${token.borderRadius}px`,
+ fontSize: token.fontSize,
+ } as React.CSSProperties,
+ };
+
+ return as === "label" ? (
+ <label {...commonProps} htmlFor={htmlFor}>
+ {children}
+ </label>
+ ) : (
+ <Text {...commonProps}>
+ {children}
+ </Text>
+ );📝 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 InputLabel = ({children}: {children: React.ReactNode}) => { | |
| const {token} = theme.useToken(); | |
| return ( | |
| <Text | |
| className={styles["datasetLabel"] || ""} | |
| className={styles["label"] || ""} | |
| style={{ | |
| backgroundColor: token.colorFillTertiary, | |
| borderBottomLeftRadius: `${token.borderRadius}px`, | |
| borderColor: token.colorBorder, | |
| borderTopLeftRadius: `${token.borderRadius}px`, | |
| fontSize: token.fontSizeLG, | |
| fontSize: token.fontSize, | |
| }} | |
| > | |
| Dataset | |
| {children} | |
| </Text> | |
| ); | |
| type InputLabelProps = { | |
| children: React.ReactNode; | |
| as?: "span" | "label"; | |
| htmlFor?: string; | |
| id?: string; | |
| className?: string; | |
| }; | |
| const InputLabel = ({children, as = "span", htmlFor, id, className}: InputLabelProps) => { | |
| const {token} = theme.useToken(); | |
| const commonProps = { | |
| id, | |
| className: `${styles.label} ${className ?? ""}`.trim(), | |
| style: { | |
| backgroundColor: token.colorFillTertiary, | |
| borderBottomLeftRadius: `${token.borderRadius}px`, | |
| borderColor: token.colorBorder, | |
| borderTopLeftRadius: `${token.borderRadius}px`, | |
| fontSize: token.fontSize, | |
| } as React.CSSProperties, | |
| }; | |
| return as === "label" ? ( | |
| <label {...commonProps} htmlFor={htmlFor}> | |
| {children} | |
| </label> | |
| ) : ( | |
| <Text {...commonProps}> | |
| {children} | |
| </Text> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In components/webui/client/src/components/InputLabel/index.tsx around lines 18
to 34, replace the non-semantic Text element with a semantic label element and
remove the unnecessary fallback for className; specifically render a native
<label> (or a Text/typography component configured to render as a label) so the
component is accessible, pass through the same inline styles, and use
className={styles["label"]} (no "|| ''" fallback). If the label needs to target
an input, accept and forward an optional htmlFor prop and apply it to the label.
| return ( | ||
| <Text | ||
| className={styles["datasetLabel"] || ""} | ||
| className={styles["label"] || ""} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove || "" fallback.
CSS modules always yield a string; the fallback is unnecessary and was removed in the diff above.
🤖 Prompt for AI Agents
In components/webui/client/src/components/InputLabel/index.tsx around line 23,
remove the unnecessary fallback "|| ''" from the className assignment so it uses
the CSS module value directly (e.g., className={styles["label"]}); update the
line to stop providing the empty-string fallback since CSS modules always return
a string.
| .customButton { | ||
| height: 22px; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid text clipping on small buttons.
Hard-coding only height can cause vertical misalignment/clipping across fonts/OS. Set line-height to match.
Apply:
.customButton {
- height: 22px;
+ height: 22px;
+ line-height: 22px;
}📝 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.
| .customButton { | |
| height: 22px; | |
| } | |
| .customButton { | |
| height: 22px; | |
| line-height: 22px; | |
| } |
🤖 Prompt for AI Agents
In
components/webui/client/src/components/QueryBox/InputWithCaseSensitive/index.module.css
around lines 1-3, the .customButton rule only sets height which can cause text
clipping on some fonts/OS; add line-height: 22px to match the height so text
vertically centers and doesn't clip, and optionally ensure display is
inline-block (or block) and vertical-align: middle if needed for alignment.
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx
Outdated
Show resolved
Hide resolved
| <InputLabel> ORDER BY </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wire disabled state to search UI state.
Hard-coding disabled={false} can allow edits during QUERYING. Recommend deriving disabled from the store for consistency with other controls.
Apply this diff:
import InputLabel from "../../../../../components/InputLabel";
import SqlInput from "../../../../../components/SqlInput";
import guidedGrid from "./index.module.css";
+import useSearchStore from "../../../SearchState/index";
+import {SEARCH_UI_STATE} from "../../../SearchState/typings";
-const OrderBy = () => (
- <div className={guidedGrid["order"]}>
- <InputLabel> ORDER BY </InputLabel>
- <SqlInput disabled={false}/>
- </div>
-);
+const OrderBy = () => {
+ const searchUiState = useSearchStore((state) => state.searchUiState);
+ const disabled =
+ searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING ||
+ searchUiState === SEARCH_UI_STATE.QUERYING;
+ return (
+ <div className={guidedGrid["order"]}>
+ <InputLabel>ORDER BY</InputLabel>
+ <SqlInput disabled={disabled}/>
+ </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.
| <InputLabel> ORDER BY </InputLabel> | |
| <SqlInput disabled={false}/> | |
| </div> | |
| import InputLabel from "../../../../../components/InputLabel"; | |
| import SqlInput from "../../../../../components/SqlInput"; | |
| import guidedGrid from "./index.module.css"; | |
| import useSearchStore from "../../../SearchState/index"; | |
| import {SEARCH_UI_STATE} from "../../../SearchState/typings"; | |
| const OrderBy = () => { | |
| const searchUiState = useSearchStore((state) => state.searchUiState); | |
| const disabled = | |
| searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || | |
| searchUiState === SEARCH_UI_STATE.QUERYING; | |
| return ( | |
| <div className={guidedGrid["order"]}> | |
| <InputLabel>ORDER BY</InputLabel> | |
| <SqlInput disabled={disabled}/> | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx
around lines 13-15, the SqlInput is hard-coded with disabled={false}; instead,
read the disabled state from the search UI store (e.g.,
isQuerying/controlsDisabled via the existing selector or hook used by other
controls) and pass that value into SqlInput so the control is disabled during
querying; ensure you import and use the same selector/hook pattern as the other
controls for consistency.
| import InputLabel from "../../../../../components/InputLabel"; | ||
| import SqlInput from "../../../../../components/SqlInput"; | ||
| import guidedGrid from "./index.module.css"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer import aliases.
Same rationale as in Where.tsx. See tsconfig check script there.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx
lines 1-3, the three deep relative imports should be converted to the project
path-alias style used in Where.tsx; replace the "../../../.." relative paths
with the same alias-based imports (matching the aliases defined in tsconfig and
the imports in Where.tsx) for InputLabel, SqlInput and the CSS module, and then
run the tsconfig check script to confirm the aliases resolve.
| <div className={guidedGrid["select"]}> | ||
| <InputLabel> SELECT </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Same cleanups as WHERE: remove padding spaces, redundant prop, and use dot-notation.
- <div className={guidedGrid["select"]}>
- <InputLabel> SELECT </InputLabel>
- <SqlInput disabled={false}/>
+ <div className={guidedGrid.select}>
+ <InputLabel>SELECT</InputLabel>
+ <SqlInput />📝 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={guidedGrid["select"]}> | |
| <InputLabel> SELECT </InputLabel> | |
| <SqlInput disabled={false}/> | |
| </div> | |
| <div className={guidedGrid.select}> | |
| <InputLabel>SELECT</InputLabel> | |
| <SqlInput /> | |
| </div> |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx
around lines 12 to 15, clean up the JSX: remove the extra spaces inside the
InputLabel text (change " SELECT " to "SELECT"), remove the redundant prop
disabled={false} from SqlInput, and replace bracket notation
guidedGrid["select"] with dot-notation guidedGrid.select; make these minimal
edits in-place.
| const Where = () => ( | ||
| <div className={guidedGrid["where"]}> | ||
| <InputLabel> WHERE </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
A11y: make the label programmatic and associate it to the input.
Once InputLabel supports rendering a native label (see comment in InputLabel file), associate them via htmlFor/id.
- <InputLabel>WHERE</InputLabel>
- <SqlInput />
+ <InputLabel as="label" htmlFor="presto-where">WHERE</InputLabel>
+ <SqlInput id="presto-where" />Verify SqlInput forwards id to the focusable element:
🏁 Script executed:
#!/bin/bash
# Check SqlInput props and id forwarding
rg -n -C3 'export default function SqlInput|function SqlInput|const SqlInput' components/webui/client/src/components/SqlInput
rg -n -C2 '\bid\b' components/webui/client/src/components/SqlInputLength of output: 687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SQL_INPUT="components/webui/client/src/components/SqlInput/index.tsx"
WHERE_FILE="components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx"
echo "== SqlInput file check =="
if [ -f "$SQL_INPUT" ]; then
echo "file: $SQL_INPUT"
nl -ba "$SQL_INPUT" | sed -n '1,240p'
else
echo "NOT FOUND: $SQL_INPUT"
fi
echo
echo "== look for 'id' occurrences inside SqlInput =="
if [ -f "$SQL_INPUT" ]; then
rg -n --hidden --no-ignore-vcs '\bid\b' "$SQL_INPUT" || echo "no 'id' matches in $SQL_INPUT"
fi
echo
echo "== look for props spread / rest forwarding in SqlInput =="
if [ -f "$SQL_INPUT" ]; then
rg -n --hidden --no-ignore-vcs '\.\.\.props|\.\.\.rest|\{[^}]*\.\.\.rest' "$SQL_INPUT" || echo "no obvious props spread patterns in $SQL_INPUT"
rg -n --hidden --no-ignore-vcs 'forwardRef|React\.forwardRef' "$SQL_INPUT" || true
fi
echo
echo "== locate SqlEditorProps type/interface definition in repo =="
rg -n --hidden --no-ignore-vcs 'SqlEditorProps' || echo "SqlEditorProps not found in repo search"
echo
echo "== show Where.tsx referenced in review =="
if [ -f "$WHERE_FILE" ]; then
nl -ba "$WHERE_FILE" | sed -n '1,240p'
else
echo "NOT FOUND: $WHERE_FILE"
fiLength of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SQL_INPUT="components/webui/client/src/components/SqlInput/index.tsx"
WHERE_FILE="components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx"
echo "== SqlInput file existence =="
if [ -f "$SQL_INPUT" ]; then echo "FOUND: $SQL_INPUT"; else echo "NOT FOUND: $SQL_INPUT"; fi
echo
echo "== SqlInput (first 400 lines) =="
if [ -f "$SQL_INPUT" ]; then sed -n '1,400p' "$SQL_INPUT"; fi
echo
echo "== 'id' occurrences inside SqlInput =="
if [ -f "$SQL_INPUT" ]; then rg -n '\bid\b' "$SQL_INPUT" || echo "no 'id' matches"; fi
echo
echo "== props spread / forwardRef patterns in SqlInput =="
if [ -f "$SQL_INPUT" ]; then
rg -n '\.\.\.props|\.\.\.rest|{\s*\.\.\.rest|forwardRef|React\.forwardRef' "$SQL_INPUT" || true
fi
echo
echo "== Find SqlEditorProps definition in repo =="
rg -n --hidden --no-ignore-vcs 'type SqlEditorProps|interface SqlEditorProps|SqlEditorProps =' || echo "SqlEditorProps not found by simple pattern"
echo
echo "== Underlying editor / focusable element references in SqlInput =="
if [ -f "$SQL_INPUT" ]; then
rg -n 'SqlEditor|Monaco|Ace|CodeMirror|textarea|contentEditable|codemirror|editorRef|getEditor' "$SQL_INPUT" || true
fi
echo
echo "== Where.tsx (context) =="
if [ -f "$WHERE_FILE" ]; then sed -n '1,240p' "$WHERE_FILE"; else echo "NOT FOUND: $WHERE_FILE"; fi
echo
echo "== Locate InputLabel component(s) =="
rg -n --hidden --no-ignore-vcs 'function InputLabel|const InputLabel|export .*InputLabel' components/webui/client/src || true
echo
echo "== Print candidate InputLabel files (first 240 lines) =="
rg -n --hidden --no-ignore-vcs 'function InputLabel|const InputLabel|export .*InputLabel' components/webui/client/src | cut -d: -f1 | sort -u | while read -r f; do
[ -f "$f" ] && { echo "== FILE: $f =="; sed -n '1,240p' "$f"; } || echo "SKIP: $f (not a file)";
doneLength of output: 4610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SE_FILE="components/webui/client/src/components/SqlEditor/index.tsx"
WHERE_FILE="components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx"
INPUT_LABEL="components/webui/client/src/components/InputLabel/index.tsx"
echo "== SqlEditor exists? =="
[ -f "$SE_FILE" ] && echo "FOUND: $SE_FILE" || { echo "NOT FOUND: $SE_FILE"; exit 0; }
echo
echo "== SqlEditor (first 400 lines) =="
sed -n '1,400p' "$SE_FILE"
echo
echo "== 'id' occurrences in SqlEditor =="
rg -n --hidden --no-ignore-vcs '\bid\b|id\s*=' "$SE_FILE" || echo "no 'id' occurrences"
echo
echo "== props spread / forwardRef patterns in SqlEditor =="
rg -n --hidden --no-ignore-vcs '\.\.\.props|{\s*\.\.\.rest|forwardRef|React\.forwardRef' "$SE_FILE" || true
echo
echo "== underlying editor / container references in SqlEditor =="
rg -n --hidden --no-ignore-vcs 'Monaco|monaco|monaco-editor|monacoRef|editorRef|onMount|onEditorMount|textarea|contentEditable|divRef' "$SE_FILE" || true
echo
echo "== Where.tsx (for context) =="
sed -n '1,240p' "$WHERE_FILE" || true
echo
echo "== InputLabel (for context) =="
sed -n '1,240p' "$INPUT_LABEL" || trueLength of output: 5334
A11y: associate label with the SQL input — implement in InputLabel + SqlEditor first
InputLabel currently renders antd Typography.Text (no htmlFor/as props) and SqlInput simply spreads props to SqlEditor → Editor; @monaco-editor/react does not expose a way to set an HTML id on the editor DOM (see issue). (github.com)
- Add htmlFor/as support to InputLabel so it can render a native . — components/webui/client/src/components/InputLabel/index.tsx
- Ensure the editor wrapper receives the id: modify components/webui/client/src/components/SqlEditor/index.tsx to apply the incoming id (or htmlId) to the outer . SqlInput already forwards props to SqlEditor (so no change needed in SqlInput).
After those two changes, apply this Where.tsx update:
- <InputLabel>WHERE</InputLabel>
- <SqlInput />
+ <InputLabel as="label" htmlFor="presto-where">WHERE</InputLabel>
+ <SqlInput id="presto-where" />Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx
around lines 11-15, the InputLabel is not associated with the SQL editor because
InputLabel currently renders Typography.Text and SqlEditor does not expose an id
on its DOM; fix by (1) updating
components/webui/client/src/components/InputLabel/index.tsx to accept htmlFor
(or an "as" prop) and render a native <label> when htmlFor is provided (preserve
existing styling and typings), (2) updating
components/webui/client/src/components/SqlEditor/index.tsx to accept an id or
htmlId prop and apply that id to the outer wrapper <div> that contains the
monaco editor so the label can target it, and then (3) change this Where.tsx to
pass a matching id/htmlId to SqlInput (e.g., <InputLabel htmlFor="where-sql">
WHERE </InputLabel> and <SqlInput id="where-sql" />) so the label is
programmatically associated with the editor.
| <div className={guidedGrid["where"]}> | ||
| <InputLabel> WHERE </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Trim label whitespace, drop redundant prop, prefer dot-notation.
- Avoid relying on literal spaces for layout; let CSS handle padding.
disabled={false}is redundant.guidedGrid.wherereads cleaner than bracket access.
- <div className={guidedGrid["where"]}>
- <InputLabel> WHERE </InputLabel>
- <SqlInput disabled={false}/>
+ <div className={guidedGrid.where}>
+ <InputLabel>WHERE</InputLabel>
+ <SqlInput />📝 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={guidedGrid["where"]}> | |
| <InputLabel> WHERE </InputLabel> | |
| <SqlInput disabled={false}/> | |
| </div> | |
| <div className={guidedGrid.where}> | |
| <InputLabel>WHERE</InputLabel> | |
| <SqlInput /> | |
| </div> |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx
around lines 12 to 15, trim the literal spaces inside the InputLabel, remove the
redundant disabled={false} prop from SqlInput, and use dot-notation for the
className (guidedGrid.where) instead of bracket access; rely on CSS for any
needed padding/spacing rather than literal spaces in the label.
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (1)
50-50: Minor: drop redundant || "" on CSS module usage.
styles["input"]is already a string; the fallback is unnecessary.- <div className={styles["input"] || ""}> + <div className={styles["input"]}>components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx (1)
73-83: Avoid rendering an empty Tooltip wrapper.Passing "" still wraps the button. Prefer null/undefined when no title.
- let tooltipTitle = ""; + let tooltipTitle: string | undefined = undefined; @@ - <Tooltip title={tooltipTitle}> + <Tooltip title={tooltipTitle}>Or conditionally render Tooltip only when tooltipTitle is set.
components/webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx (2)
35-40: Follow repo guideline: avoid!expr; prefer comparing to false.Replace
!isCaseSensitivewith a comparison to false.- onCaseSensitiveChange(!isCaseSensitive); + onCaseSensitiveChange(false === isCaseSensitive);
55-67: Add toggle semantics for accessibility.Expose state via aria-pressed and an explicit label.
<Button className={styles["customButton"] || ""} disabled={disabled} size={"small"} variant={buttonVariant} + aria-pressed={isCaseSensitive} + aria-label={"Match case"} color={isCaseSensitive ? "primary" : "default"} onClick={handleButtonClick}components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)
45-47: Apply code style: compare against false instead of using!.Matches the project’s guideline for boolean checks.
- if (!isValidDateRange(dates)) { + if (false === isValidDateRange(dates)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
components/webui/client/src/components/InputLabel/index.module.css(1 hunks)components/webui/client/src/components/InputLabel/index.tsx(1 hunks)components/webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx(2 hunks)components/webui/client/src/components/QueryBox/InputWithCaseSensitive/index.module.css(1 hunks)components/webui/client/src/components/SqlEditor/index.tsx(1 hunks)components/webui/client/src/components/SqlInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx(0 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls.tsx
🧰 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/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsxcomponents/webui/client/src/components/SqlEditor/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsxcomponents/webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsxcomponents/webui/client/src/components/InputLabel/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsxcomponents/webui/client/src/components/SqlInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.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). (1)
- GitHub Check: package-image
🔇 Additional comments (15)
components/webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx (2)
67-67: Sizedown to “middle” looks consistent.No functional concerns. Good alignment with the rest of the SearchControls.
61-75: Verify QueryBox supports the “middle” size end-to-end.Confirm that QueryBox forwards size={"middle"} to its underlying input so spacing stays consistent with neighbouring controls.
components/webui/client/src/components/SqlEditor/index.tsx (2)
81-88: Wrapper 100% sizing with minHeight/minWidth=0 is the right fix for grid/flex overflow.This should resolve Monaco’s layout issues in CSS grids. Nice.
76-88: ```shell
#!/usr/bin/env bash
set -euo pipefailFind all files containing "<SqlEditor" and report usages missing an explicit height= attribute on the tag.
Exclude node_modules and common build dirs.
matches=$(rg -n --hidden -g '!.git' -g '!/node_modules/' -g '!/dist/' -g '!/build/' '<SqlEditor' || true)
if [ -z "$matches" ]; then
echo "NO_SQLEDITOR_MATCHES"
exit 0
fipython3 - <<'PY'
import re, sys, pathlib
matches = """$matches""".splitlines()
reported = False
for line in matches:
parts = line.split(':', 2)
if len(parts) < 2:
continue
path = parts[0]
p = pathlib.Path(path)
try:
txt = p.read_text(encoding='utf-8')
except Exception as e:
print(f"ERROR reading {path}: {e}", file=sys.stderr)
continue
for m in re.finditer(r'<SqlEditor\b[^>]>', txt, re.S):
tag = m.group(0)
start_line = txt.count('\n', 0, m.start()) + 1
if re.search(r'\bheight\s=', tag) is None:
print(f"{path}:{start_line}:{tag}")
reported = True
if not reported:
print("ALL_SQLEDITOR_TAGS_HAVE_EXPLICIT_HEIGHT")
PY</blockquote></details> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (2)</summary><blockquote> `9-9`: **Import tidy-up looks good.** No behavioural change. --- `50-57`: **Height vs. container sizing: double-check layout.** You set height={"120px"} here while SqlEditor’s wrapper is now 100% tall. Ensure the parent of styles["input"] defines a non-zero height when you later remove this explicit height. </blockquote></details> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx (1)</summary><blockquote> `86-86`: **Sizedown to “middle” is consistent.** Matches other controls. </blockquote></details> <details> <summary>components/webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx (1)</summary><blockquote> `9-10`: **CSS module wiring looks fine.** No logic changes; safe. </blockquote></details> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)</summary><blockquote> `64-66`: **Sizedown to “middle” for Select and RangePicker looks good.** Keeps the controls visually compact. Also applies to: 78-79 </blockquote></details> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx (1)</summary><blockquote> `45-45`: **Sizedown to “middle” is consistent.** No behavioural change. </blockquote></details> <details> <summary>components/webui/client/src/components/SqlInput/index.tsx (1)</summary><blockquote> `33-36`: **Enabling Monaco automaticLayout is correct; verify container sizing to prevent zero-height editors** Ensure the grid cell/parent sets an explicit height and uses min-height: 0 so ResizeObserver can compute dimensions. If visibility toggles (e.g., CSS breakpoints), call editor.layout() after the toggle. </blockquote></details> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (2)</summary><blockquote> `9-9`: **Import swap to InputLabel looks good** Refactor aligns with the new shared label component. --- `82-95`: **Hook the label to the Select for accessibility** Associate the label and control to improve screen‑reader UX; confirm InputLabel forwards id and that Select accepts aria‑labelledby (or use htmlFor/id on a native label/input). ```diff - <InputLabel>Dataset</InputLabel> - <Select + <InputLabel id="dataset-label">Dataset</InputLabel> + <Select aria-labelledby="dataset-label" className={styles["select"] || ""}components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
9-9: Confirm import resolution for the new GuidedControls indexVerify the old file was removed and there’s no naming collision so bundlers resolve the folder’s index.tsx as intended.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css (1)
9-34: Verify column spans at narrow widths
grid-column: span 3/2/1withauto-fitcan push items awkwardly on small screens. Confirm desired wrapping and adjust spans or add breakpoints if needed.Would you like me to propose responsive media queries that collapse
selectfromspan 3tospan 2/1under specific widths?
| padding-left: 10px; | ||
| padding-right: 10px; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: use padding shorthand
Same effect, simpler.
- padding-left: 10px;
- padding-right: 10px;
+ padding: 0 10px;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/webui/client/src/components/InputLabel/index.module.css around
lines 4 to 5, the two separate declarations padding-left and padding-right
should be replaced with a single padding shorthand to simplify the CSS; update
the declarations to use something like padding: 0 10px; (keeping vertical
padding as needed) so the left/right padding remains 10px while reducing
verbosity.
| border-top-right-radius: 0px; | ||
| border-bottom-right-radius: 0px; | ||
| white-space: nowrap; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent label overflow on narrow layouts
white-space: nowrap can cause overflow/clipping. Add ellipsis and hide overflow.
.label {
@@
- white-space: nowrap;
+ white-space: nowrap;
+ overflow: hidden;
+ text-overflow: ellipsis;
}📝 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.
| white-space: nowrap; | |
| .label { | |
| white-space: nowrap; | |
| overflow: hidden; | |
| text-overflow: ellipsis; | |
| } |
🤖 Prompt for AI Agents
In components/webui/client/src/components/InputLabel/index.module.css around
line 10, the rule using "white-space: nowrap" can cause label overflow/clipping
on narrow layouts; update the CSS to prevent overflow by adding "overflow:
hidden;" and "text-overflow: ellipsis;" alongside or replacing the existing
property so long labels are truncated with an ellipsis instead of overflowing.
| return ( | ||
| <Text | ||
| className={styles["datasetLabel"] || ""} | ||
| className={styles["label"] || ""} | ||
| style={{ | ||
| backgroundColor: token.colorFillTertiary, | ||
| borderBottomLeftRadius: `${token.borderRadius}px`, | ||
| borderColor: token.colorBorder, | ||
| borderTopLeftRadius: `${token.borderRadius}px`, | ||
| fontSize: token.fontSizeLG, | ||
| fontSize: token.fontSize, | ||
| }} | ||
| > | ||
| Dataset | ||
| {children} | ||
| </Text> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use semantic and add a visible border
Improves accessibility (associable via htmlFor) and ensures the border actually renders (colour without width/style is a no-op).
-const InputLabel = ({children}: {children: React.ReactNode}) => {
+type InputLabelProps = {
+ children: React.ReactNode;
+ htmlFor?: string;
+ className?: string;
+};
+
+const InputLabel = ({children, htmlFor, className}: InputLabelProps) => {
const {token} = theme.useToken();
return (
- <Text
- className={styles["label"] || ""}
+ <label
+ htmlFor={htmlFor}
+ className={`${styles.label} ${className ?? ""}`}
style={{
backgroundColor: token.colorFillTertiary,
borderBottomLeftRadius: `${token.borderRadius}px`,
- borderColor: token.colorBorder,
+ border: `1px solid ${token.colorBorder}`,
borderTopLeftRadius: `${token.borderRadius}px`,
fontSize: token.fontSize,
}}
>
{children}
- </Text>
+ </label>
);
};📝 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.
| return ( | |
| <Text | |
| className={styles["datasetLabel"] || ""} | |
| className={styles["label"] || ""} | |
| style={{ | |
| backgroundColor: token.colorFillTertiary, | |
| borderBottomLeftRadius: `${token.borderRadius}px`, | |
| borderColor: token.colorBorder, | |
| borderTopLeftRadius: `${token.borderRadius}px`, | |
| fontSize: token.fontSizeLG, | |
| fontSize: token.fontSize, | |
| }} | |
| > | |
| Dataset | |
| {children} | |
| </Text> | |
| ); | |
| type InputLabelProps = { | |
| children: React.ReactNode; | |
| htmlFor?: string; | |
| className?: string; | |
| }; | |
| const InputLabel = ({children, htmlFor, className}: InputLabelProps) => { | |
| const {token} = theme.useToken(); | |
| return ( | |
| <label | |
| htmlFor={htmlFor} | |
| className={`${styles.label} ${className ?? ""}`} | |
| style={{ | |
| backgroundColor: token.colorFillTertiary, | |
| borderBottomLeftRadius: `${token.borderRadius}px`, | |
| border: `1px solid ${token.colorBorder}`, | |
| borderTopLeftRadius: `${token.borderRadius}px`, | |
| fontSize: token.fontSize, | |
| }} | |
| > | |
| {children} | |
| </label> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In components/webui/client/src/components/InputLabel/index.tsx around lines
21-34, replace the non-semantic Text wrapper with a semantic <label> element (or
render Text as a label) so the component can accept and forward an htmlFor prop
for accessibility, and change the border-only color style to a concrete border
(e.g. borderWidth and borderStyle or a full border string like "1px solid
{token.colorBorder}") so the border actually renders; update the component
props/type to include htmlFor (and forward it to the rendered label), keep
existing className/style and token-based values, and ensure children remain
inside the label.
| return ( | ||
| <Text | ||
| className={styles["datasetLabel"] || ""} | ||
| className={styles["label"] || ""} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Drop redundant fallback
className accepts undefined; || "" is unnecessary.
- className={styles["label"] || ""}
+ className={styles.label}📝 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.
| className={styles["label"] || ""} | |
| className={styles.label} |
🤖 Prompt for AI Agents
In components/webui/client/src/components/InputLabel/index.tsx around line 23,
remove the redundant fallback "|| ''" from the className assignment because
React's className accepts undefined; change the line to use the styles lookup
directly (i.e., className={styles["label"]}) so the property can be undefined
instead of forcing an empty string.
| .customButton { | ||
| height: 22px; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid fixed pixel height; use Ant Design token to match theme.
Use the small control height variable so this stays in sync across themes.
-.customButton {
- height: 22px;
-}
+.customButton {
+ height: var(--ant-control-height-sm);
+ line-height: var(--ant-control-height-sm);
+}📝 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.
| .customButton { | |
| height: 22px; | |
| } | |
| .customButton { | |
| height: var(--ant-control-height-sm); | |
| line-height: var(--ant-control-height-sm); | |
| } |
🤖 Prompt for AI Agents
In
components/webui/client/src/components/QueryBox/InputWithCaseSensitive/index.module.css
around lines 1–3, the .customButton rule uses a fixed height: 22px; replace this
with the Ant Design small control height token so the button follows theme
changes (e.g., use the ANTD token/variable for control height small via CSS
variable or Less variable such as the small control height token), ensure the
selector preserves specificity and add any necessary line-height/vertical-align
adjustments so the button content remains centered.
| <InputLabel> ORDER BY </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Trim spaces and remove redundant prop
Consistent labelling; fewer needless props.
- <InputLabel> ORDER BY </InputLabel>
- <SqlInput disabled={false}/>
+ <InputLabel>ORDER BY</InputLabel>
+ <SqlInput/>📝 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.
| <InputLabel> ORDER BY </InputLabel> | |
| <SqlInput disabled={false}/> | |
| </div> | |
| <InputLabel>ORDER BY</InputLabel> | |
| <SqlInput/> | |
| </div> |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx
around lines 13 to 15, the ORDER BY label contains extra surrounding spaces and
the SqlInput includes a redundant disabled={false} prop; trim the label text to
"ORDER BY" (no leading/trailing spaces) and remove the disabled={false} prop
from the SqlInput so the markup is consistent and free of needless props.
| <div className={guidedGrid["select"]}> | ||
| <InputLabel> SELECT </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add accessible labelling for the editor
If SqlInput forwards ARIA props, link the label and input (e.g., id on InputLabel + aria-labelledby on SqlInput). Otherwise, consider extending SqlInput to accept aria-label/aria-labelledby.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx
around lines 12–15, the label and the SqlInput are not programmatically
associated; update the markup so the InputLabel has a unique id and the SqlInput
is given an accessible reference (either aria-labelledby pointing to that id or
an aria-label). If SqlInput currently doesn't forward ARIA props, modify the
SqlInput component to accept and forward aria-label/aria-labelledby (and id if
needed) to the underlying editable element, then wire InputLabel's id and
SqlInput's aria-labelledby in this file.
| <InputLabel> SELECT </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Trim extra spaces in label and drop redundant disabled prop
Removes noise; behaviour unchanged.
- <InputLabel> SELECT </InputLabel>
- <SqlInput disabled={false}/>
+ <InputLabel>SELECT</InputLabel>
+ <SqlInput/>📝 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.
| <InputLabel> SELECT </InputLabel> | |
| <SqlInput disabled={false}/> | |
| </div> | |
| <InputLabel>SELECT</InputLabel> | |
| <SqlInput/> | |
| </div> |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx
around lines 13 to 15, the InputLabel contains extra surrounding spaces ("
SELECT ") and SqlInput includes a redundant disabled={false} prop; remove the
extra spaces so the label reads "SELECT" and delete the disabled prop to rely on
the default behaviour.
| <div className={guidedGrid["where"]}> | ||
| <InputLabel> WHERE </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Apply a11y linkage for the editor
Use id on InputLabel and aria-labelledby (or aria-label) on SqlInput to make the control discoverable to assistive tech.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx
around lines 12 to 15, the InputLabel and SqlInput are not linked for
accessibility; add a unique id to the InputLabel (e.g. id="where-label" or
generated unique id) and set the SqlInput to reference it via
aria-labelledby="where-label" (or aria-label with same text if label reference
isn’t possible). Ensure the id is stable/unique in this component, update any
props passed to SqlInput to accept aria attributes, and verify the label text
matches the referenced id.
| <InputLabel> WHERE </InputLabel> | ||
| <SqlInput disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tidy label and remove redundant prop
Same cleanup as other inputs.
- <InputLabel> WHERE </InputLabel>
- <SqlInput disabled={false}/>
+ <InputLabel>WHERE</InputLabel>
+ <SqlInput/>📝 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.
| <InputLabel> WHERE </InputLabel> | |
| <SqlInput disabled={false}/> | |
| </div> | |
| <InputLabel>WHERE</InputLabel> | |
| <SqlInput/> | |
| </div> |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx
around lines 13-15, tidy the label by removing extra surrounding spaces and
match casing used by other inputs (e.g., change "<InputLabel> WHERE
</InputLabel>" to "<InputLabel>Where</InputLabel>"), and remove the redundant
disabled={false} prop from the SqlInput so it relies on the default enabled
state; keep formatting consistent with the other input components.
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 (3)
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (3)
82-95: Wire the label to the Select for accessibility.Associate the label with the control so screen readers announce context. If
InputLabelforwardsidto a DOM element, bind witharia-labelledby.- <InputLabel>Dataset</InputLabel> - <Select + <InputLabel id="dataset-label">Dataset</InputLabel> + <Select + aria-labelledby="dataset-label"If
InputLabeldoesn’t forwardid, add that support there or fall back toaria-label="Dataset"on the Select.
89-89: Standardise control size to “middle” to match the rest of the UI.- size={"large"} + size="middle"
28-31: Type the query result and simplify the initialisation guard.Add a generic for type safety and replace the
typeofcheck with a length check.- const {data, isPending, isSuccess, error} = useQuery({ + const {data = [], isPending, isSuccess, error} = useQuery<string[]>({ queryKey: ["datasets"], queryFn: fetchDatasetNames, });- if (isSuccess) { - if ("undefined" !== typeof data[0] && null === dataset) { - updateDataset(data[0]); - } - } + if (isSuccess && null === dataset && data.length > 0) { + updateDataset(data[0]); + }Also applies to: 36-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/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/Dataset/index.tsx
🔇 Additional comments (1)
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (1)
9-9: Switch to shared InputLabel looks good.Nice consolidation onto the common label component; this also resolves the prior unused
theme/tokenlint issue noted earlier.
| <DatasetLabel/> | ||
| <InputLabel>Dataset</InputLabel> | ||
| <Select | ||
| className={styles["select"] || ""} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant fallback in className.
styles["select"] is already a string; the || "" is unnecessary.
- className={styles["select"] || ""}
+ className={styles["select"]}📝 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.
| className={styles["select"] || ""} | |
| className={styles["select"]} |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx
around line 84, remove the redundant fallback in the className prop —
styles["select"] is already a string so drop the "|| ''" and set className to
styles["select"] directly to simplify the JSX.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (2)
36-39: Potential runtime error: unsafe access of data[0] when data may be undefined.Accessing data[0] before confirming Array.isArray(data) can throw. Simplify and guard the condition.
Apply:
- if (isSuccess) { - if ("undefined" !== typeof data[0] && null === dataset) { - updateDataset(data[0]); - } - } + if (true === isSuccess && Array.isArray(data) && 0 < data.length && null === dataset) { + updateDataset(data[0]); + }
60-69: Same unsafe access: data.length without verifying data is an array.Guard with Array.isArray to avoid crashes on unexpected values.
Apply:
- if (isSuccess && 0 === data.length) { + if (true === isSuccess && Array.isArray(data) && 0 === data.length) { messageApi.warning({ key: "noData", content: "No data has been ingested. Please ingest data to search.", }); // If all datasets were deleted, the dataset state must be reset to null to disable // submit button since dataset option required to query clp-s. updateDataset(null); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/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/Dataset/index.tsx
🔇 Additional comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (2)
9-9: Good swap to shared InputLabel.Importing the shared InputLabel reduces duplication and keeps styling consistent.
91-91: Normalise null to undefined for AntD Select value.
AntD Select handles undefined for "no value" more cleanly than null and avoids React controlled/uncontrolled warnings.
Apply:- value={dataset} + value={null === dataset ? undefined : dataset}
| <InputLabel>Dataset</InputLabel> | ||
| <Select |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Wire up accessible labelling between label and Select.
Expose the label via id and reference it from Select for screen readers.
Apply:
- <InputLabel>Dataset</InputLabel>
+ <InputLabel id="dataset-label">Dataset</InputLabel>
<Select
+ aria-labelledby="dataset-label"Please confirm InputLabel forwards the id prop to a semantic element and that no conflicting ids exist on the page.
📝 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.
| <InputLabel>Dataset</InputLabel> | |
| <Select | |
| <InputLabel id="dataset-label">Dataset</InputLabel> | |
| <Select | |
| aria-labelledby="dataset-label" |
| <DatasetLabel/> | ||
| <InputLabel>Dataset</InputLabel> | ||
| <Select | ||
| className={styles["select"] || ""} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Drop redundant fallback in className.
CSS modules yield a string; the || "" is unnecessary.
Apply:
- className={styles["select"] || ""}
+ className={styles.select}📝 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.
| className={styles["select"] || ""} | |
| className={styles.select} |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx
around line 84, the className uses a redundant fallback (`|| ""`) which is
unnecessary because CSS modules always return a string; remove the `|| ""`
fallback so the className is simply styles["select"] (or the equivalent property
access used elsewhere) to clean up the code.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
looks good, and even nicer after some tweaks
also, can we let color of the label background match the color of the input/select boxes, or the other way around?
here's a demo of the suggestion:
| @@ -0,0 +1,3 @@ | |||
| .customButton { | |||
| height: 22px; | |||
| } No newline at end of file | |||
| <AntdSelect | ||
| defaultValue={LIMIT_OPTIONS[0]?.value} | ||
| options={LIMIT_OPTIONS} | ||
| style={{width: "100%"}}/> |
There was a problem hiding this comment.
possible to create a class for this?
| border-top-right-radius: 0px; | ||
| border-bottom-right-radius: 0px; |
There was a problem hiding this comment.
unrelated to the PR, but since we modified a few attributes in this class
| border-top-right-radius: 0px; | |
| border-bottom-right-radius: 0px; | |
| border-top-right-radius: 0; | |
| border-bottom-right-radius: 0; |
There was a problem hiding this comment.
is this change directly related to the current PR?
There was a problem hiding this comment.
Indirectly related but needed. Kirk decided he prefers the smaller 14 font. So i'm moving both UI to the smaller font, which will simplify code and components reuse. Unfortunately, this icon is too big for the smaller input, so it needed to be a bit smaller.
| height: "100%", | ||
| minHeight: 0, | ||
| minWidth: 0, |
There was a problem hiding this comment.
possible to create css classes for these static attributes?
| display: grid; | ||
| grid-template-columns: repeat(auto-fit, minmax(200px, 1fr)); | ||
| grid-template-rows: repeat(auto-fit, 30px); | ||
| max-width: 1250px; |
There was a problem hiding this comment.
if not strictly needed, i think it's better to remove the width limit. since all boxes are stacked together, it should be fine even if all of them show up together on a very long line
| max-width: 1250px; |
| } from "react"; | ||
|
|
||
| import SqlEditor, {SqlEditorType} from "../../../../../components/SqlEditor"; | ||
| import {Nullable} from "../../../../../typings/common"; |
There was a problem hiding this comment.
if not a result of linter run, let's revert this
| @@ -0,0 +1,30 @@ | |||
| import {Select as AntdSelect} from "antd"; | |||
There was a problem hiding this comment.
do you think it would be better to change the SQL clause components as SelectBox / SelectInput for example, so that their purposes (to accept user inputs rather than displaying some output) is more obvious? Then we don't have to do this rename here as well
that said, the rename here is fine if you want to keep
There was a problem hiding this comment.
I think i can remove the rename but leave the name the same. I had it in a different file before thats why was renamed but not dont actually need to rename.
| const Limit = () => ( | ||
| <div className={guidedGrid["limit"]}> | ||
| <InputLabel> LIMIT </InputLabel> | ||
| <AntdSelect |
There was a problem hiding this comment.
can we remove the left border radius? e.g.,
.noLeftBorderRadius :global(.ant-select-selector) {
border-top-left-radius: 0!important;
border-bottom-left-radius: 0!important;
}
| <Select/> | ||
| <From/> | ||
| <Where/> | ||
| <OrderBy/> |
There was a problem hiding this comment.
can we remove the left border radius? e.g.,
{
border-top-left-radius: 0!important;
border-bottom-left-radius: 0!important;
}
There was a problem hiding this comment.
Actionable comments posted: 4
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 (3)
100-101: onEditorReady can be skipped when consumer supplies onMount. Compose both.Because {...editorProps} comes after onMount in the original code, a user-provided onMount overrides your handler and prevents onEditorReady from firing.
Apply this diff to compose callbacks and ensure precedence:
- theme={disabled ? - "disabled-theme" : - "default-theme"} - onMount={handleEditorDidMount} - {...editorProps}/> + theme={disabled ? + "disabled-theme" : + "default-theme"} + {...editorProps} + onMount={(editor, m) => { + handleEditorDidMount(editor); + editorProps.onMount?.(editor, m); + }} + />
23-27: Avoid forwarding className to both wrapper and Editor.className is currently applied to the wrapper and also forwarded to via {...editorProps}, which can cause styling conflicts. Treat className as container-only.
Apply this minimal change:
-type SqlEditorProps = Omit<EditorProps, "language"> & { +type SqlEditorProps = Omit<EditorProps, "language"> & { disabled: boolean; className?: string;- const {disabled, onEditorReady, ...editorProps} = props; + const {disabled, onEditorReady, className, ...editorProps} = props;- className={[styles["editor"], props.className].filter(Boolean).join(" ")} + className={[styles["editor"], className].filter(Boolean).join(" ")}Optionally, for stricter typing and clearer API, change the props to:
- Omit "className" from EditorProps, and
- Introduce editorClassName for the inner Editor.
I can provide that follow-up diff if desired.Also applies to: 36-36, 80-81, 101-101
57-61: Focus outline removed; check a11y.Setting "focusBorder": "#0000" eliminates the focus ring. Ensure Monaco still presents a visible focus indicator for keyboard users in both themes.
If visibility is inadequate, prefer reducing contrast instead of removing it, or scope focus styling to non-editor elements only.
Also applies to: 67-72
♻️ Duplicate comments (16)
components/webui/client/src/components/SqlEditor/index.tsx (2)
84-85: Don’t disable pointer events; enforce disabled via Monaco readOnly.Using pointer-events: none blocks selection/copy and harms a11y. Keep interactions and set readOnly on the editor instead.
Apply this diff:
- pointerEvents: disabled ? "none" : "auto", + // Keep text selection/copy even when disabled. + pointerEvents: "auto",And ensure readOnly is enforced (place after the spread so it wins):
<Editor language={"sql"} loading={ <div style={{ backgroundColor: token.colorBgContainer, height: "100%", width: "100%", }}/> } theme={disabled ? "disabled-theme" : "default-theme"} - onMount={handleEditorDidMount} - {...editorProps}/> + {...editorProps} + options={{ + ...(editorProps.options ?? {}), + readOnly: disabled, + }} + onMount={handleEditorDidMount} + />Also applies to: 97-102
81-85: Consider moving static styles to CSS with vars; keep tokens dynamic.To address prior feedback about static attributes in CSS, you can shift border/borderRadius into the CSS module via CSS variables while keeping token-driven values dynamic.
Example:
/* index.module.css */ .editor { height: 100%; min-height: 0; min-width: 0; width: 100%; + border: 1px solid var(--sql-editor-border, transparent); + border-radius: var(--sql-editor-radius, 8px); }- style={{ - border: `1px solid ${token.colorBorder}`, - borderRadius: token.borderRadius, - pointerEvents: "auto", - }} + style={{ + // dynamic theme tokens + ["--sql-editor-border"]: token.colorBorder, + ["--sql-editor-radius"]: `${token.borderRadius}px`, + pointerEvents: "auto", + }}components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx (2)
12-18: Trim label whitespace, fix className lookups, and derive disabled from UI stateRemoves space padding, avoids fallback/className template noise, and disables during querying.
-import InputLabel from "../../../../../components/InputLabel"; -import SqlInput from "../../../../../components/SqlInput"; -import guidedGrid from "./index.module.css"; +import InputLabel from "../../../../../components/InputLabel"; +import SqlInput from "../../../../../components/SqlInput"; +import guidedGrid from "./index.module.css"; +import useSearchStore from "../../../SearchState"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; @@ -const OrderBy = () => ( - <div className={guidedGrid["order"]}> - <InputLabel> ORDER BY </InputLabel> - <SqlInput - className={guidedGrid[`noLeftBorderRadius`] || ""} - disabled={false}/> - </div> -); +const OrderBy = () => { + const ui = useSearchStore((s) => s.searchUiState); + const disabled = + ui === SEARCH_UI_STATE.QUERY_ID_PENDING || + ui === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.order}> + <InputLabel as="label" htmlFor="presto-order-by">ORDER BY</InputLabel> + <SqlInput id="presto-order-by" className={guidedGrid.noLeftBorderRadius} disabled={disabled}/> + </div> + ); +};
13-16: Associate label with editor for a11yUse a semantic label and id on the editor container so assistive tech announces context. Requires InputLabel to support as/htmlFor and SqlInput to forward id to its focusable/root element.
If SqlInput doesn’t forward id, I can patch SqlEditor/SqlInput to apply it to the outer div.
#!/bin/bash # Verify SqlInput forwards 'id' (read-only) fd SqlInput --full-path components/webui/client/src -t f -e tsx -x sed -n '1,200p' {} rg -n -C2 '\bid\b|forwardRef|\.\.\.props' components/webui/client/src/components/SqlInputcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx (2)
7-13: Type LIMIT options explicitlyImproves readability and prevents accidental shape drift.
-const LIMIT_OPTIONS = [ +const LIMIT_OPTIONS: { value: number; label: string }[] = [ {value: 10, label: "10"}, {value: 50, label: "50"}, {value: 100, label: "100"}, {value: 500, label: "500"}, {value: 1000, label: "1000"}, ];
20-31: Trim label, remove unnecessary optional chaining, wire disabled, and add a11y linkageKeep the control consistent with other inputs and disable during querying.
-import {Select} from "antd"; +import {Select} from "antd"; @@ -import InputLabel from "../../../../../components/InputLabel"; -import guidedGrid from "./index.module.css"; +import InputLabel from "../../../../../components/InputLabel"; +import guidedGrid from "./index.module.css"; +import useSearchStore from "../../../SearchState"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; @@ -const Limit = () => ( - <div className={guidedGrid["limit"]}> - <InputLabel> LIMIT </InputLabel> - <Select - defaultValue={LIMIT_OPTIONS[0]?.value} - options={LIMIT_OPTIONS} - className={ - guidedGrid["noLeftBorderRadiusSelect"] + " " + - guidedGrid["widthSelect"] - } - /> - </div> -); +const Limit = () => { + const ui = useSearchStore((s) => s.searchUiState); + const disabled = + ui === SEARCH_UI_STATE.QUERY_ID_PENDING || + ui === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.limit}> + <InputLabel id="limit-label">LIMIT</InputLabel> + <Select + aria-labelledby="limit-label" + size="middle" + defaultValue={LIMIT_OPTIONS[0].value} + options={LIMIT_OPTIONS} + disabled={disabled} + className={`${guidedGrid.noLeftBorderRadiusSelect} ${guidedGrid.widthSelect}`} + /> + </div> + ); +};components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (2)
1-3: Adopt path aliases for importsReplace deep relative paths with configured aliases to simplify maintenance.
Confirm the alias names (e.g., @components/InputLabel, @pages/SearchPage/...) and I’ll provide a precise diff.
12-18: Clean JSX, fix className lookups, and disable during querying; add a11y associationAligns with other controls and improves accessibility.
-import guidedGrid from "./index.module.css"; +import guidedGrid from "./index.module.css"; +import useSearchStore from "../../../SearchState"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; @@ -const Select = () => ( - <div className={guidedGrid["select"]}> - <InputLabel> SELECT </InputLabel> - <SqlInput - className={guidedGrid[`noLeftBorderRadius`] || ""} - disabled={false}/> - </div> -); +const Select = () => { + const ui = useSearchStore((s) => s.searchUiState); + const disabled = + ui === SEARCH_UI_STATE.QUERY_ID_PENDING || + ui === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.select}> + <InputLabel as="label" htmlFor="presto-select">SELECT</InputLabel> + <SqlInput id="presto-select" className={guidedGrid.noLeftBorderRadius} disabled={disabled}/> + </div> + ); +};components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx (2)
1-3: Use path aliasesSame rationale as other guided controls.
12-18: Whitespace, className, disabled-by-state, and a11y linkageKeep consistent casing/spacing; tie disabled to UI state; programmatically associate label.
-import guidedGrid from "./index.module.css"; +import guidedGrid from "./index.module.css"; +import useSearchStore from "../../../SearchState"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; @@ -const Where = () => ( - <div className={guidedGrid["where"]}> - <InputLabel> WHERE </InputLabel> - <SqlInput - className={guidedGrid[`noLeftBorderRadius`] || ""} - disabled={false}/> - </div> -); +const Where = () => { + const ui = useSearchStore((s) => s.searchUiState); + const disabled = + ui === SEARCH_UI_STATE.QUERY_ID_PENDING || + ui === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.where}> + <InputLabel as="label" htmlFor="presto-where">WHERE</InputLabel> + <SqlInput id="presto-where" className={guidedGrid.noLeftBorderRadius} disabled={disabled}/> + </div> + ); +};components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (2)
1-3: Prefer path-alias importsAvoid brittle deep relatives.
12-18: Consistent label text, className access, disabled state, and a11yMatches other clauses; disables during querying; adds proper association.
-import guidedGrid from "./index.module.css"; +import guidedGrid from "./index.module.css"; +import useSearchStore from "../../../SearchState"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; @@ -const From = () => ( - <div className={guidedGrid["from"]}> - <InputLabel> FROM </InputLabel> - <SqlInput - className={guidedGrid[`noLeftBorderRadius`] || ""} - disabled={false}/> - </div> -); +const From = () => { + const ui = useSearchStore((s) => s.searchUiState); + const disabled = + ui === SEARCH_UI_STATE.QUERY_ID_PENDING || + ui === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.from}> + <InputLabel as="label" htmlFor="presto-from">FROM</InputLabel> + <SqlInput id="presto-from" className={guidedGrid.noLeftBorderRadius} disabled={disabled}/> + </div> + ); +};components/webui/client/src/components/InputLabel/index.tsx (3)
11-16: Document props including semantic/as and htmlForAdd prop docs so consumers can render a real label and associate it to controls.
/** - * Renders a label for an input field. + * Renders a label for an input field. * - * @param props - * @param props.children The label text to display. + * @param props + * @param props.children The label text to display. + * @param props.as Render as "label" (semantic) or "span" (Typography). Default: "span". + * @param props.htmlFor When as="label", associates the label to a control id. + * @param props.id Optional id for ARIA linkage (e.g., aria-labelledby). + * @param props.className Optional extra class names.
18-33: Make InputLabel semantic; fix background/border and drop redundant fallbacksCurrent background uses token.colorBorder (likely a mistake) and no visible border is set. Provide a semantic option and clean className usage.
-const InputLabel = ({children}: {children: React.ReactNode}) => { +type InputLabelProps = { + children: React.ReactNode; + as?: "span" | "label"; + htmlFor?: string; + id?: string; + className?: string; +}; + +const InputLabel = ({children, as = "span", htmlFor, id, className}: InputLabelProps) => { const {token} = theme.useToken(); - return ( - <Text - className={styles["label"] || ""} - style={{ - backgroundColor: token.colorBorder, - borderBottomLeftRadius: `${token.borderRadius}px`, - borderTopLeftRadius: `${token.borderRadius}px`, - fontSize: token.fontSize, - }} - > - {children} - </Text> - ); + const common = { + id, + className: `${styles.label} ${className ?? ""}`.trim(), + style: { + backgroundColor: token.colorFillTertiary, + border: `1px solid ${token.colorBorder}`, + borderBottomLeftRadius: `${token.borderRadius}px`, + borderTopLeftRadius: `${token.borderRadius}px`, + fontSize: token.fontSize, + } as React.CSSProperties, + }; + + return as === "label" ? ( + <label {...common} htmlFor={htmlFor}>{children}</label> + ) : ( + <Text {...common}>{children}</Text> + ); };
23-23: Remove|| ""on classNameCSS modules return strings; the fallback is noise.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css (1)
1-6: Use grid-auto-rows for implicit row sizing; optionally pack denselygrid-template-rows won’t size implicit rows. grid-auto-rows ensures consistent height; dense packing reduces gaps on wrap.
.gridContainer { display: grid; grid-template-columns: repeat(auto-fit, minmax(200px, 1fr)); - grid-template-rows: 32px; + grid-auto-rows: 32px; + grid-auto-flow: row dense; gap: 5px; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
components/webui/client/src/components/InputLabel/index.tsx(1 hunks)components/webui/client/src/components/SqlEditor/index.module.css(1 hunks)components/webui/client/src/components/SqlEditor/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css(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/Where.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/components/InputLabel/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsxcomponents/webui/client/src/components/SqlEditor/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
⏰ 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 (macos-15)
🔇 Additional comments (1)
components/webui/client/src/components/SqlEditor/index.module.css (1)
1-6: LGTM on container sizing.The 100% fill with min-* zeroes is correct for flex/grid parents and Monaco’s auto layout.
| const InputLabel = ({children}: {children: React.ReactNode}) => { | ||
| const {token} = theme.useToken(); | ||
|
|
||
| return ( | ||
| <Text | ||
| className={styles["datasetLabel"] || ""} | ||
| className={styles["label"] || ""} | ||
| style={{ | ||
| backgroundColor: token.colorFillTertiary, | ||
| backgroundColor: token.colorBorder, | ||
| borderBottomLeftRadius: `${token.borderRadius}px`, | ||
| borderColor: token.colorBorder, | ||
| borderTopLeftRadius: `${token.borderRadius}px`, | ||
| fontSize: token.fontSizeLG, | ||
| fontSize: token.fontSize, | ||
| }} | ||
| > | ||
| Dataset | ||
| {children} | ||
| </Text> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Verify SqlInput/SqlEditor id forwarding to enable htmlFor usage
Ensure the id passed to SqlInput lands on the actual DOM node for the label to target.
🏁 Script executed:
#!/bin/bash
# Verify id is forwarded in SqlInput/SqlEditor (read-only)
rg -n -C2 'export default function SqlInput|const SqlInput' components/webui/client/src/components/SqlInput
rg -n -C2 '\bid\b|forwardRef|\.\.\.props' components/webui/client/src/components/SqlInput components/webui/client/src/components/SqlEditorLength of output: 931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== SqlInput ==="
nl -ba components/webui/client/src/components/SqlInput/index.tsx | sed -n '1,240p' || true
echo
echo "=== Search for SqlEditor occurrences ==="
rg -n --hidden --no-ignore -g 'components/webui/client/src/components/**' 'SqlEditor' || true
echo
echo "=== SqlEditor source (if present) ==="
if [ -f components/webui/client/src/components/SqlEditor/index.tsx ]; then
nl -ba components/webui/client/src/components/SqlEditor/index.tsx | sed -n '1,400p'
else
rg -n --hidden --no-ignore 'export default function SqlEditor|const SqlEditor|forwardRef' components/webui/client/src/components || true
fiLength of output: 1630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== SqlInput (first 240 lines) ==="
sed -n '1,240p' components/webui/client/src/components/SqlInput/index.tsx || true
echo
echo "=== SqlEditor (first 400 lines) ==="
sed -n '1,400p' components/webui/client/src/components/SqlEditor/index.tsx || true
echo
echo "=== rg searches in SqlEditor ==="
rg -n -C3 'id\b|id=|wrapperProps|forwardRef|forwardRef\(|useRef\(|\bref\b|onEditorReady|editorDidMount|monaco|createEditor' components/webui/client/src/components/SqlEditor/index.tsx || trueLength of output: 7111
Forward id to Editor wrapper so htmlFor works
SqlInput forwards props → SqlEditor, and SqlEditor spreads the remaining props into ; @monaco-editor/react applies DOM attributes via EditorProps.wrapperProps (not a top-level id) and SqlEditor’s outer wrapper currently does not receive an id. (app.unpkg.com)
- Change: add id?: string to SqlEditorProps (and accept it in SqlInput) and forward it into Editor via wrapperProps (merge with any existing wrapperProps).
- Files: components/webui/client/src/components/SqlEditor/index.tsx (where is rendered) and components/webui/client/src/components/SqlInput/index.tsx (prop passthrough).
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/index.tsx and
components/webui/client/src/components/SqlInput/index.tsx (changes around where
props are defined and where <Editor/> is rendered), add id?: string to the
SqlEditorProps type and accept id in SqlInput props, then forward that id into
the monaco <Editor/> via wrapperProps: merge the incoming id with any existing
wrapperProps object (preserving other wrapperProps keys) so the outer DOM
wrapper receives the id; ensure types reflect wrapperProps possibly being
undefined and do not overwrite existing wrapperProps when merging.
| import * as monaco from "monaco-editor/esm/vs/editor/editor.api.js"; | ||
|
|
||
| import "./monaco-loader"; | ||
| import styles from "./index.module.css"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify SQL autocomplete prerequisites are imported (editor.all.js).
Per prior learning on this repo, SQL IntelliSense requires importing monaco-editor/esm/vs/editor/editor.all.js. index.tsx imports editor.api and ./monaco-loader; please confirm that monaco-loader (or elsewhere) brings in editor.all.js, otherwise autocomplete will regress.
Run:
🏁 Script executed:
#!/bin/bash
# Confirm editor.all.js and sql.contribution are imported somewhere in the webui.
rg -nP 'monaco-editor/esm/vs/editor/editor\.all\.js|basic-languages/sql/sql\.contribution' components/webuiLength of output: 138
Add monaco-editor/esm/vs/editor/editor.all.js to restore SQL IntelliSense.
index.tsx imports "monaco-editor/esm/vs/editor/editor.api.js" and monaco-loader.ts imports "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js" (components/webui/client/src/components/SqlEditor/monaco-loader.ts:6), but there is no import of "monaco-editor/esm/vs/editor/editor.all.js" under components/webui — add:
import "monaco-editor/esm/vs/editor/editor.all.js";
to components/webui/client/src/components/SqlEditor/monaco-loader.ts before the sql.contribution import.
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/monaco-loader.ts around line
6, SQL IntelliSense is missing because we only import editor.api.js and
sql.contribution.js; add an import of
"monaco-editor/esm/vs/editor/editor.all.js" before the sql.contribution import
to load the full Monaco editor bundle so SQL language features and IntelliSense
are registered correctly.
| .noLeftBorderRadiusSelect :global(.ant-select-selector) { | ||
| border-top-left-radius: 0!important; | ||
| border-bottom-left-radius: 0!important; | ||
| } | ||
|
|
||
| .noLeftBorderRadius { | ||
| border-top-left-radius: 0!important; | ||
| border-bottom-left-radius: 0!important; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Confirm left-radius overrides apply to actual controls
For non-AntD inputs (e.g., Monaco), .noLeftBorderRadius may not affect inner wrappers. If SqlInput wraps Monaco, consider a :global() rule targeting the editor container.
I can propose precise selectors after confirming SqlInput/SqlEditor DOM structure.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css
around lines 37–45, the .noLeftBorderRadius rule may not reach inner editor
wrappers (e.g., Monaco) used by SqlInput/SqlEditor, so add a :global() selector
that targets the editor's container/selector (for example the Monaco editor
wrapper or the specific SqlEditor inner wrapper) and apply the same
border-top-left-radius and border-bottom-left-radius: 0!important overrides
there; ensure the selector matches the actual DOM class names used by
SqlInput/SqlEditor and include both the AntD .ant-select-selector rule and the
editor-specific :global(...) rule so the left-radius is removed for both AntD
and non-AntD inputs.
| import InputLabel from "../../../../../components/InputLabel"; | ||
| import SqlInput from "../../../../../components/SqlInput"; | ||
| import guidedGrid from "./index.module.css"; |
There was a problem hiding this comment.
🧹 Nitpick
Switch deep relative imports to path aliases
Use the repo’s configured TS path aliases for resilience to moves and shorter imports.
What alias should these resolve to in this project (e.g., @components/InputLabel)? I can provide a committable diff once confirmed.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx
lines 1-3, replace the deep relative imports with TS path aliases: import
InputLabel from "@components/InputLabel" and import SqlInput from
"@components/SqlInput"; also replace the CSS import with the page-scoped alias
import guidedGrid from
"@pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css" so
imports are resilient to file moves and shorter.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (18)
components/webui/client/src/components/SqlEditor/index.tsx (2)
86-89: Don’t disable pointer events; use Monaco readOnly instead.Disabling pointer events blocks selection/copy and harms a11y. Keep interactions and enforce non‑editable via readOnly.
Apply this diff:
style={{ border: `1px solid ${token.colorBorder}`, borderRadius: token.borderRadius, - pointerEvents: disabled ? - "none" : - "auto", }} > <Editor language={"sql"} + options={{ + ...(editorProps.options ?? {}), + readOnly: disabled, + }} loading={ <div style={{ backgroundColor: token.colorBgContainer, height: "100%", width: "100%", }}/> } theme={disabled ? "disabled-theme" : "default-theme"} onMount={handleEditorDidMount} - {...editorProps}/> + {...editorProps}/>Also applies to: 101-105
17-18: Ensure editor.all.js is imported to preserve SQL IntelliSense.Per prior repo learning, SQL autocomplete requires monaco-editor/esm/vs/editor/editor.all.js in the loader. Please confirm monaco-loader imports it.
#!/bin/bash # Verify Monaco SQL prerequisites are imported somewhere in webui. rg -nP 'monaco-editor/esm/vs/editor/editor\.all\.js|basic-languages/sql/sql\.contribution' components/webuicomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx (1)
12-16: A11y linkage + small cleanups: associate label with editor, trim whitespace, drop redundant props, use dot notation.Label isn’t programmatically associated with the SQL editor, and there’s avoidable noise (
disabled={false},|| "", bracket access). Apply the following and verify aria props reach the DOM.- <div className={guidedGrid["where"]}> - <InputLabel> WHERE </InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> + <div className={guidedGrid.where}> + <InputLabel id="presto-where-label">WHERE</InputLabel> + <SqlInput + className={guidedGrid.noLeftBorderRadius} + aria-labelledby="presto-where-label" + />Run to confirm
aria-*/idare forwarded to a real DOM node (SqlInput → SqlEditor → container). If not, wire them in SqlEditor’s outer div.#!/bin/bash set -euo pipefail rg -n -C2 'export default function SqlInput|function SqlInput|const SqlInput' components/webui/client/src/components/SqlInput rg -n -C2 '\.\.\.props|forwardRef|aria-|className|id\b' components/webui/client/src/components/SqlInput rg -n -C2 'export default function SqlEditor|function SqlEditor|const SqlEditor' components/webui/client/src/components/SqlEditor rg -n -C3 '\.\.\.props|{\s*\.\.\.rest|aria-|id\b|className' components/webui/client/src/components/SqlEditor fd InputLabel components/webui/client/src -t f -e tsx -x rg -n 'export|function|const|forwardRef|props' {}components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (5)
1-3: Prefer path aliases over deep relative imports.Use the project’s configured TS path aliases for
InputLabelandSqlInputto reduce fragility on file moves.Example diff (adjust alias names to match your tsconfig):
-import InputLabel from "../../../../../components/InputLabel"; -import SqlInput from "../../../../../components/SqlInput"; +import InputLabel from "@components/InputLabel"; +import SqlInput from "@components/SqlInput";Run to confirm alias availability:
#!/bin/bash # Show tsconfig path aliases fd -a 'tsconfig*.json' | while read f; do echo "== $f =="; cat "$f"; echo; done | rg -n "paths|baseUrl" -n -C2
12-12: Use CSS‑module dot notation for simple identifiers.- <div className={guidedGrid["from"]}> + <div className={guidedGrid.from}>
13-13: Remove extra whitespace in visible label.- <InputLabel> FROM </InputLabel> + <InputLabel>FROM</InputLabel>
11-18: Add programmatic association between label and editor (a11y).Use a stable id (e.g.,
useId) and wirearia-labelledbyso assistive tech can relate the label to the Monaco editor wrapper.Apply within this block:
-const From = () => ( - <div className={guidedGrid["from"]}> - <InputLabel> FROM </InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> - </div> -); +const From = (): JSX.Element => { + const uid = useId(); + return ( + <div className={guidedGrid.from}> + <InputLabel id={`${uid}-label`}>FROM</InputLabel> + <SqlInput + id={`${uid}-editor`} + aria-labelledby={`${uid}-label`} + className={guidedGrid.noLeftBorderRadius} + /> + </div> + ); +};Add the import (outside this hunk):
import { useId } from "react";Note: If
InputLabelsupportshtmlFor, also sethtmlFor={${uid}-editor}on it. Please confirmSqlInputforwardsid/aria-*to the focusable element; if not, expose a prop for this.
15-16: Simplify className and drop redundant prop.
|| ""is unnecessary;undefinedis fine forclassName.disabled={false}is the default.- className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> + className={guidedGrid.noLeftBorderRadius} + />components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx (4)
7-13: TypeLIMIT_OPTIONSfor safety and editor help.Explicitly type the options so
Select<number>is strongly typed and consumers don’t getany.-const LIMIT_OPTIONS = [ +const LIMIT_OPTIONS: { value: number; label: string }[] = [ {value: 10, label: "10"}, {value: 50, label: "50"}, {value: 100, label: "100"}, {value: 500, label: "500"}, {value: 1000, label: "1000"}, ];
22-22: Remove label whitespace; rely on CSS.Keeps i18n keys clean and avoids layout surprises.
- <InputLabel> LIMIT </InputLabel> + <InputLabel>LIMIT</InputLabel>
24-24: Drop unnecessary optional chaining.Array literal guarantees index 0.
- defaultValue={LIMIT_OPTIONS[0]?.value} + defaultValue={LIMIT_OPTIONS[0].value}
21-31: Make the control controllable, add a11y, size, and optional disabled prop.Enables state lifting, consistent sizing, and proper labelling. Avoids mixing controlled/uncontrolled patterns.
-const Limit = () => ( - <div className={guidedGrid["limit"]}> - <InputLabel> LIMIT </InputLabel> - <Select - defaultValue={LIMIT_OPTIONS[0]?.value} - options={LIMIT_OPTIONS} - className={ - `${guidedGrid["noLeftBorderRadiusSelect"]} ${ - guidedGrid["widthSelect"]}` - }/> - </div> -); +type LimitProps = { + value?: number; + onChange?: (v: number) => void; + disabled?: boolean; +}; + +const Limit = ({value, onChange, disabled}: LimitProps) => ( + <div className={guidedGrid["limit"]}> + <InputLabel id="limit-label">LIMIT</InputLabel> + <Select<number> + aria-labelledby="limit-label" + value={value ?? LIMIT_OPTIONS[0].value} + onChange={(v) => onChange?.(v as number)} + options={LIMIT_OPTIONS} + size="middle" + disabled={disabled} + className={`${guidedGrid["noLeftBorderRadiusSelect"]} ${guidedGrid["widthSelect"]}`} + /> + </div> +);If you’d like this disabled state derived from search status, wire it to the store (adjust import paths as needed):
// outside the selected lines (supporting snippet) import useSearchStore from "../../../SearchState"; import {SEARCH_UI_STATE} from "../../../SearchState/typings"; // inside component: const searchUiState = useSearchStore((s) => s.searchUiState); const computedDisabled = disabled ?? (searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || searchUiState === SEARCH_UI_STATE.QUERYING);components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx (3)
13-13: Trim label whitespace; control spacing via CSS.Remove the hard‑coded spaces; apply spacing via styles on the container/label.
- <InputLabel> ORDER BY </InputLabel> + <InputLabel>ORDER BY</InputLabel>
11-18: A11y linkage + disable while querying + minor cleanups in one pass.
- Add aria linkage (label ↔ editor) via useId.
- Derive disabled from search UI state.
- Drop redundant className fallback and prefer dot access for CSS modules.
-const OrderBy = () => ( - <div className={guidedGrid["order"]}> - <InputLabel> ORDER BY </InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> - </div> -); +const OrderBy = () => { + const labelId = useId(); + const searchUiState = useSearchStore((s) => s.searchUiState); + const disabled = + searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || + searchUiState === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.order}> + <InputLabel id={labelId}>ORDER BY</InputLabel> + <SqlInput + className={guidedGrid.noLeftBorderRadius} + aria-labelledby={labelId} + disabled={disabled} + /> + </div> + ); +};Add the necessary imports (adjust paths if different in your tree):
+import {useId} from "react"; +import useSearchStore from "../../../SearchState/index"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings";Verification script to confirm SqlInput forwards ARIA props (expect to see aria‑* in its props or ...rest forwarded):
#!/bin/bash # Locate SqlInput and inspect its props/forwarding fd -t f -e tsx -e ts 'SqlInput' | sed -n '1,200p' -n rg -n -C3 -P '(interface|type)\s+.*SqlInput.*Props|forwardRef|...rest|aria-'
1-3: Switch deep relative imports to TS path aliases (if configured).Use aliases for resilience to moves and shorter imports. If aliases aren’t set, keep as‑is.
-import InputLabel from "../../../../../components/InputLabel"; -import SqlInput from "../../../../../components/SqlInput"; -import guidedGrid from "./index.module.css"; +import InputLabel from "@components/InputLabel"; +import SqlInput from "@components/SqlInput"; +import guidedGrid from "@pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css";Check alias config:
#!/bin/bash # Verify path aliases in tsconfig rg -nC3 '"paths"\s*:' tsconfig*.jsoncomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (3)
1-3: Prefer path aliases over deep relative imports.Match the alias style used in Where.tsx for InputLabel, SqlInput, and the CSS module.
Run to discover the alias pattern in Where.tsx and confirm tsconfig paths:
#!/bin/bash set -euo pipefail echo "== Locate GuidedControls/Where.tsx imports ==" fd -a --type f 'Where.tsx' | while read -r f; do echo "--- $f" rg -nP '^\s*import\s+.*(InputLabel|SqlInput|index\.module\.css)' "$f" || true done echo "== Inspect tsconfig path aliases ==" fd -a --type f '^tsconfig.*\.json$' | while read -r t; do echo "--- $t" rg -n '"paths"\s*:' -n -C2 "$t" || true done
12-16: Tidy JSX: dot-notation for CSS modules, trim label, drop redundant props.Removes noise and avoids unnecessary defaults.
Apply:
- <div className={guidedGrid["select"]}> - <InputLabel> SELECT </InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> + <div className={guidedGrid.select}> + <InputLabel>SELECT</InputLabel> + <SqlInput className={guidedGrid.noLeftBorderRadius} />
13-16: Add programmatic labelling for accessibility.Associate the label and editor via aria-labelledby (or aria-label if SqlInput can’t forward).
Apply:
- <InputLabel>SELECT</InputLabel> - <SqlInput className={guidedGrid.noLeftBorderRadius} /> + <InputLabel id="select-label">SELECT</InputLabel> + <SqlInput + className={guidedGrid.noLeftBorderRadius} + aria-labelledby="select-label" + />Verify SqlInput forwards ARIA props to the underlying editable element:
#!/bin/bash set -euo pipefail echo "== Locate SqlInput implementation ==" fd -a --type f 'SqlInput.*\.(tsx|ts|jsx|js)$' echo "== Check for ARIA forwarding or rest props ==" rg -nP '(aria-|ariaLabel|aria-labelledby|\.{3}rest|\{\.{3}props)' $(fd -a --type f 'SqlInput.*\.(tsx|ts|jsx|js)$') || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
components/webui/client/src/components/SqlEditor/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/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/Where.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsxcomponents/webui/client/src/components/SqlEditor/index.tsx
🧠 Learnings (3)
📚 Learning: 2025-05-29T20:33:40.653Z
Learnt from: junhaoliao
PR: y-scope/clp#937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx
📚 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
📚 Learning: 2025-08-26T13:45:46.445Z
Learnt from: davemarco
PR: y-scope/clp#1244
File: components/webui/client/package.json:29-29
Timestamp: 2025-08-26T13:45:46.445Z
Learning: The "color" package in components/webui/client/package.json is used in the SqlEditor component to convert Ant Design token colors to hex format for Monaco editor themes, specifically for the disabled state styling.
Applied to files:
components/webui/client/src/components/SqlEditor/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 (macos-15)
🔇 Additional comments (6)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx (1)
9-9: Consolidate Nullable import — good change.Using the shared @webui/common/utility-types centralizes typings and reduces local alias drift.
components/webui/client/src/components/SqlEditor/index.tsx (2)
22-24: Expose className on SqlEditor — LGTM.This unlocks composition with layout containers (e.g., the new grid) without leaking internal styles.
81-83: Class merge is correct; verify height semantics with fixed Editor height.Wrapper uses height: 100% via CSS, while consumers (SqlQueryInput) pass height: "120px" to Editor. Please sanity‑check there’s no overflow/collapse in the grid.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx (1)
6-10: Type clarity (optional): annotate return type for the component.Add an explicit return type to help tooling and consistency with other guided controls.
-const Where = () => ( +const Where: React.FC = () => (⛔ Skipped due to learnings
Learnt from: junhaoliao PR: y-scope/clp#937 File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24 Timestamp: 2025-05-29T20:33:40.653Z Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx (1)
26-29: Verified: CSS module keys exist.
BothnoLeftBorderRadiusSelectandwidthSelectare defined in components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css (lines 33 and 37).components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (1)
11-11: Annotate return type for clarity.Helps TS catch accidental return changes.
Apply:
-const Select = () => ( +const Select = (): JSX.Element => (⛔ Skipped due to learnings
Learnt from: junhaoliao PR: y-scope/clp#937 File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24 Timestamp: 2025-05-29T20:33:40.653Z Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
| /** | ||
| * Renders the FROM SQL input field. | ||
| * | ||
| * @return | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick
Complete the JSDoc and align with return type.
Document the return type; optionally annotate the function for stronger typing.
/**
* Renders the FROM SQL input field.
*
- * @return
+ * @return JSX.Element
*/📝 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.
| /** | |
| * Renders the FROM SQL input field. | |
| * | |
| * @return | |
| */ | |
| /** | |
| * Renders the FROM SQL input field. | |
| * | |
| * @return JSX.Element | |
| */ |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx
around lines 6 to 10, the JSDoc header is incomplete and doesn't state the
return type; update the JSDoc to include a descriptive summary and an explicit
@returns tag (e.g., @returns {JSX.Element}) and, optionally, annotate the
function signature with an explicit React return type (e.g., : JSX.Element or
use React.FC) to align doc and TypeScript typings; ensure imports/types are
available and the doc matches the actual return value.
| const LIMIT_OPTIONS = [ | ||
| {value: 10, label: "10"}, | ||
| {value: 50, label: "50"}, | ||
| {value: 100, label: "100"}, | ||
| {value: 500, label: "500"}, | ||
| {value: 1000, label: "1000"}, | ||
| ]; |
There was a problem hiding this comment.
🧹 Nitpick
Optional: expose and reuse a DEFAULT_LIMIT constant.
Reduces magic numbers and keeps builders/tests consistent.
const LIMIT_OPTIONS: { value: number; label: string }[] = [
{value: 10, label: "10"},
@@
];
+
+export const DEFAULT_LIMIT = 10;And use it:
- <Select<number>
+ <Select<number>
aria-labelledby="limit-label"
- value={value ?? LIMIT_OPTIONS[0].value}
+ value={value ?? DEFAULT_LIMIT}Also applies to: 21-31
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx
around lines 7 to 13 (and similarly lines 21 to 31), extract a DEFAULT_LIMIT
constant (e.g., numeric default like 100) at the top of the module and replace
the hard-coded magic numbers in LIMIT_OPTIONS and any other usages with that
constant; update builders/tests to import/use DEFAULT_LIMIT so the default is
centralized and consistent across the file.
| /** | ||
| * Renders the ORDER BY SQL input field. | ||
| * | ||
| * @return | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick
Tighten JSDoc: specify return type.
-/**
- * Renders the ORDER BY SQL input field.
- *
- * @return
- */
+/**
+ * Renders the ORDER BY SQL input field.
+ *
+ * @returns JSX.Element
+ */📝 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.
| /** | |
| * Renders the ORDER BY SQL input field. | |
| * | |
| * @return | |
| */ | |
| /** | |
| * Renders the ORDER BY SQL input field. | |
| * | |
| * @returns JSX.Element | |
| */ |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx
around lines 6 to 10, the JSDoc block for the component's return value is too
vague; update the JSDoc to include a specific return type (for example "@returns
{JSX.Element}" or "@returns {React.ReactElement}") and a short phrase describing
what is returned (e.g., "the ORDER BY SQL input field JSX element"), keeping the
rest of the doc intact.
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
done junhao review
There was a problem hiding this comment.
Indirectly related but needed. Kirk decided he prefers the smaller 14 font. So i'm moving both UI to the smaller font, which will simplify code and components reuse. Unfortunately, this icon is too big for the smaller input, so it needed to be a bit smaller.
| height: "100%", | ||
| minHeight: 0, | ||
| minWidth: 0, |
| @@ -0,0 +1,30 @@ | |||
| import {Select as AntdSelect} from "antd"; | |||
There was a problem hiding this comment.
I think i can remove the rename but leave the name the same. I had it in a different file before thats why was renamed but not dont actually need to rename.
There was a problem hiding this comment.
Actionable comments posted: 3
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)
81-89: Make editor readOnly instead of disabling pointer events; keep selection/copy.Use Monaco’s readOnly; always allow pointer interactions; add aria-disabled for a11y.
- <div - className={[styles["editor"], - className].filter(Boolean).join(" ")} - style={{ - border: `1px solid ${token.colorBorder}`, - borderRadius: token.borderRadius, - pointerEvents: disabled ? - "none" : - "auto", - }} - > + <div + className={[styles.editor, className].filter(Boolean).join(" ")} + aria-disabled={disabled} + style={{ + border: `1px solid ${token.colorBorder}`, + borderRadius: token.borderRadius, + }} + > <Editor language={"sql"} + {...editorProps} loading={ <div style={{ backgroundColor: token.colorBgContainer, height: "100%", width: "100%", }}/> } theme={disabled ? "disabled-theme" : "default-theme"} onMount={handleEditorDidMount} - {...editorProps}/> + options={{ + ...(editorProps.options ?? {}), + readOnly: disabled, + }} + />Also applies to: 101-105
♻️ Duplicate comments (11)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx (1)
1-3: Optional: use path aliases for imports if available.#!/bin/bash # Show tsconfig paths for potential aliases like @components/* jq '.compilerOptions.paths // {}' -r tsconfig.json 2>/dev/null || truecomponents/webui/client/src/components/InputLabel/index.module.css (2)
10-11: Prevent label overflow on narrow layouts.Add truncation to avoid clipping when space is tight.
- white-space: nowrap; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis;
4-5: CSS tidy: use padding shorthand.- padding-left: 10px; - padding-right: 10px; + padding: 0 10px;components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx (2)
7-13: Optional: expose and reuse a DEFAULT_LIMIT const.Avoid magic numbers; reuse in defaultValue and builders.
const LIMIT_OPTIONS = [ {value: 10, label: "10"}, @@ ]; + +export const DEFAULT_LIMIT = 10;
20-29: Disable during querying; set size; fix unnecessary optional chaining; add a11y linkage.Brings behaviour in line with other controls and improves accessibility.
-import {Select} from "antd"; +import {Select} from "antd"; +import useSearchStore from "../../../SearchState/index"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; @@ -const Limit = () => ( - <div className={guidedGrid["limit"]}> - <InputLabel>LIMIT</InputLabel> - <Select - defaultValue={LIMIT_OPTIONS[0]?.value} - options={LIMIT_OPTIONS} - className={ - `${guidedGrid["noLeftBorderRadiusSelect"]} ${ - guidedGrid["widthSelect"]}` - }/> - </div> -); +const Limit = () => { + const searchUiState = useSearchStore((s) => s.searchUiState); + const disabled = + searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || + searchUiState === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.limit}> + <InputLabel id="limit-label">LIMIT</InputLabel> + <Select + aria-labelledby="limit-label" + defaultValue={LIMIT_OPTIONS[0].value} + options={LIMIT_OPTIONS} + size={"middle"} + disabled={disabled} + className={`${guidedGrid.noLeftBorderRadiusSelect} ${guidedGrid.widthSelect}`} + /> + </div> + ); +};components/webui/client/src/components/SqlEditor/index.tsx (1)
17-18: Add missing import: monaco-editor/esm/vs/editor/editor.all.jscomponents/webui/client/src/components/SqlEditor/monaco-loader.ts imports "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js" but does not import "monaco-editor/esm/vs/editor/editor.all.js". Add import "monaco-editor/esm/vs/editor/editor.all.js" (before sql.contribution.js) so index.tsx → monaco-loader enables Monaco SQL IntelliSense.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (5)
1-3: Prefer path aliases over deep relative imports.Use project aliases for resilience to file moves.
Apply this diff if
@components/*is configured:-import InputLabel from "../../../../../components/InputLabel"; -import SqlInput from "../../../../../components/SqlInput"; +import InputLabel from "@components/InputLabel"; +import SqlInput from "@components/SqlInput";Verify aliases:
#!/bin/bash # Inspect tsconfig paths and confirm @components/* is defined fd -a -H 'tsconfig*.json' | while read -r f; do echo "== $f ==" cat "$f" | jq '.compilerOptions.paths' done
6-10: Complete the JSDoc@returns.Keep type annotations off the component per team preference; just fix the doc.
/** * Renders the FROM SQL input field. * - * @return + * @returns {JSX.Element} */
15-16: Remove redundant fallback anddisabled={false}; prefer dot notation.
classNamecan be undefined; default disabled is false.- <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> + <SqlInput className={guidedGrid.noLeftBorderRadius} />
1-4: Associate label with editor for a11y (useId + aria-labelledby).Ensure AT users can perceive the label–field relationship.
+import { useId } from "react"; @@ -const From = () => ( - <div className={guidedGrid["from"]}> - <InputLabel>FROM</InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> - </div> -); +const From = () => { + const labelId = useId(); + return ( + <div className={guidedGrid["from"]} role="group" aria-labelledby={labelId}> + <InputLabel> + <span id={labelId}>FROM</span> + </InputLabel> + <SqlInput + className={guidedGrid["noLeftBorderRadius"] || ""} + disabled={false} + /> + </div> + ); +};Also applies to: 11-18
12-12: Use dot notation for CSS module keys.Cleaner and type‑friendlier.
- <div className={guidedGrid["from"]}> + <div className={guidedGrid.from}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
components/webui/client/src/components/InputLabel/index.module.css(1 hunks)components/webui/client/src/components/SqlEditor/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.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/Where.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsxcomponents/webui/client/src/components/SqlEditor/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx
🧠 Learnings (3)
📚 Learning: 2025-05-29T20:33:40.653Z
Learnt from: junhaoliao
PR: y-scope/clp#937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx
📚 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
📚 Learning: 2025-08-26T13:45:46.445Z
Learnt from: davemarco
PR: y-scope/clp#1244
File: components/webui/client/package.json:29-29
Timestamp: 2025-08-26T13:45:46.445Z
Learning: The "color" package in components/webui/client/package.json is used in the SqlEditor component to convert Ant Design token colors to hex format for Monaco editor themes, specifically for the disabled state styling.
Applied to files:
components/webui/client/src/components/SqlEditor/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). (1)
- GitHub Check: package-image
🔇 Additional comments (4)
components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx (1)
48-53: Size change to "middle" is consistent; OK to merge.Matches the UI standardization in this PR. No functional impact.
components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (2)
82-95: A11y: programmatically associate the label with the Select — verify InputLabel forwards idSearch of components/webui/client/src/components/InputLabel/index.tsx didn't show id forwarding; confirm and apply one of the options below.
- <InputLabel>Dataset</InputLabel> + <InputLabel id="dataset-label">Dataset</InputLabel> <Select - className={styles["select"] || ""} + className={styles.select} + aria-labelledby="dataset-label" loading={isPending} options={(data || []).map((option) => ({label: option, value: option}))} placeholder={"None"} showSearch={true} size={"middle"} value={dataset}If InputLabel doesn’t forward id: update components/webui/client/src/components/InputLabel/index.tsx to accept/forward the id (extend React.HTMLAttributes and forward id) or, as a fallback, set aria-label on the Select.
9-9: Optional: replace deep relative import with a TS path alias if available.Using a path alias (e.g., @components/InputLabel) shortens imports and makes them resilient to file moves. Verification inconclusive — tsconfig.json files couldn't be parsed in the sandbox; confirm compilerOptions.baseUrl/paths and update the import if supported.
Location: components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx — import InputLabel from "../../../../components/InputLabel";
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (1)
20-20: LGTM on component structure and export.Simple, cohesive, and matches the new grid layout.
| const OrderBy = () => ( | ||
| <div className={guidedGrid["order"]}> | ||
| <InputLabel>ORDER BY</InputLabel> | ||
| <SqlInput | ||
| className={guidedGrid["noLeftBorderRadius"] || ""} | ||
| disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Derive disabled state from store; clean className; add a11y label link.
Hard-coding disabled={false} allows edits while querying. Also remove redundant fallback and use dot-notation.
-import InputLabel from "../../../../../components/InputLabel";
-import SqlInput from "../../../../../components/SqlInput";
-import guidedGrid from "./index.module.css";
+import InputLabel from "../../../../../components/InputLabel";
+import SqlInput from "../../../../../components/SqlInput";
+import guidedGrid from "./index.module.css";
+import useSearchStore from "../../../SearchState/index";
+import {SEARCH_UI_STATE} from "../../../SearchState/typings";
@@
-const OrderBy = () => (
- <div className={guidedGrid["order"]}>
- <InputLabel>ORDER BY</InputLabel>
- <SqlInput
- className={guidedGrid["noLeftBorderRadius"] || ""}
- disabled={false}/>
- </div>
-);
+const OrderBy = () => {
+ const searchUiState = useSearchStore((s) => s.searchUiState);
+ const disabled =
+ searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING ||
+ searchUiState === SEARCH_UI_STATE.QUERYING;
+ return (
+ <div className={guidedGrid.order}>
+ <InputLabel id="orderby-label">ORDER BY</InputLabel>
+ <SqlInput
+ aria-labelledby="orderby-label"
+ className={guidedGrid.noLeftBorderRadius}
+ disabled={disabled}
+ />
+ </div>
+ );
+};Note: If SqlInput doesn’t forward aria-* props to its wrapper/editable element, update it (or SqlEditor) accordingly.
🏁 Script executed:
#!/bin/bash
# Does SqlInput forward arbitrary props (e.g., aria-*)?
fd -a 'components/webui/client/src/components/SqlInput' | xargs -I{} sed -n '1,240p' {}
rg -n '...props|{[^}]*\.\.\.' components/webui/client/src/components/SqlInputLength of output: 599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repo for occurrences of 'SqlInput'..."
rg -n --hidden -S '\bSqlInput\b' || true
echo
echo "Collecting unique files that reference 'SqlInput'..."
rg -n --hidden -S '\bSqlInput\b' | awk -F: '{print $1}' | sort -u > /tmp/sql_files || true
if [ ! -s /tmp/sql_files ]; then
echo "No files found containing 'SqlInput'."
exit 0
fi
while IFS= read -r f; do
echo
echo "---- FILE: $f ----"
sed -n '1,400p' "$f" || true
echo
echo "Matches for prop-spread or aria usage in this file:"
rg -n --hidden -S '\.\.\.[A-Za-z_][A-Za-z0-9_]*|aria-' "$f" || true
done < /tmp/sql_filesLength of output: 6188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for SqlEditor definitions and related files ==="
rg -n --hidden -S '\bSqlEditor\b' || true
echo
echo "=== Locating files that define SqlEditor ==="
sql_editor_defs=$(rg -n --hidden -S 'export default .*SqlEditor|const SqlEditor|function SqlEditor|class SqlEditor' -g 'components/**' -l || true)
echo "Found: $sql_editor_defs"
if [ -n "$sql_editor_defs" ]; then
for f in $sql_editor_defs; do
echo
echo "---- FILE: $f ----"
sed -n '1,400p' "$f" || true
echo
echo "Prop-spread or aria- usage in this file:"
rg -n --hidden -S '\.\.\.[A-Za-z_][A-Za-z0-9_]*|aria-' "$f" || true
done
else
echo "No direct SqlEditor definition file found by those patterns; listing files that mention SqlEditor:"
rg -n --hidden -S '\bSqlEditor\b' -g 'components/**' || true
files=$(rg -n --hidden -S '\bSqlEditor\b' -g 'components/**' -l || true)
for f in $files; do
echo
echo "---- FILE: $f ----"
sed -n '1,400p' "$f" || true
echo
rg -n --hidden -S '\.\.\.[A-Za-z_][A-Za-z0-9_]*|aria-' "$f" || true
done
fi
echo
echo "=== Inspecting SqlInput (already found earlier) to confirm prop spread ==="
if [ -f components/webui/client/src/components/SqlInput/index.tsx ]; then
sed -n '1,240p' components/webui/client/src/components/SqlInput/index.tsx || true
echo
rg -n --hidden -S '\.\.\.[A-Za-z_][A-Za-z0-9_]*|aria-' components/webui/client/src/components/SqlInput/index.tsx || true
else
echo "SqlInput file not found at expected path."
fi
echo
echo "=== Finding InputLabel implementation ==="
rg -n --hidden -S '\bInputLabel\b' || true
input_label_files=$(rg -n --hidden -S '\bInputLabel\b' -g 'components/**' -l || true)
echo "InputLabel files: $input_label_files"
for f in $input_label_files; do
echo
echo "---- FILE: $f ----"
sed -n '1,240p' "$f" || true
echo
rg -n --hidden -S 'id=|htmlFor|htmlfor|for=|props|...props' "$f" || true
done
echo
echo "=== Locating search state (useSearchStore / SEARCH_UI_STATE) ==="
rg -n --hidden -S 'useSearchStore' || true
rg -n --hidden -S 'SEARCH_UI_STATE' || true
state_files=$(rg -n --hidden -S 'useSearchStore|SEARCH_UI_STATE' -g 'components/**' -l || true)
echo "State-related files: $state_files"
for f in $state_files; do
echo
echo "---- FILE: $f ----"
sed -n '1,240p' "$f" || true
done
echo
echo "=== Inspecting GuidedControls CSS module(s) ==="
rg -n --hidden -S 'components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/.+index.module.css' || true
css_files=$(rg -n --hidden -S 'index.module.css' components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls -g 'components/**' -l || true)
echo "CSS module files: $css_files"
for f in $css_files; do
echo
echo "---- FILE: $f ----"
sed -n '1,240p' "$f" || true
done
echo
echo "=== Also listing the GuidedControls components that use SqlInput (Select/From/Where/OrderBy) ==="
for f in components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/*.{tsx,ts}; do
if [ -f "$f" ]; then
echo
echo "---- FILE: $f ----"
sed -n '1,240p' "$f" || true
rg -n --hidden -S '\.\.\.[A-Za-z_][A-Za-z0-9_]*|aria-|disabled=' "$f" || true
fi
doneLength of output: 107374
Derive disabled state from store; clean className; add accessible label & forward aria props.
- Replace hard-coded disabled={false} with a derived value from useSearchStore() (SEARCH_UI_STATE.QUERY_ID_PENDING / QUERYING) in guided inputs: components/.../GuidedControls/{Select.tsx,From.tsx,Where.tsx,OrderBy.tsx}. Also switch CSS access to dot-notation (guidedGrid.noLeftBorderRadius / guidedGrid.order) and remove the redundant || "" fallback.
- Accessibility: link label → input by giving the label an id and the input aria-labelledby. Update InputLabel (components/webui/client/src/components/InputLabel/index.tsx) to accept an optional id?: string and forward it to the rendered (id={id}).
- Forward aria-* props: SqlInput already spreads {...props} into SqlEditor (components/webui/client/src/components/SqlInput/index.tsx), but SqlEditor (components/webui/client/src/components/SqlEditor/index.tsx) only spreads remaining props into and does not apply them to the outer wrapper div. Either spread editorProps onto the wrapper div or explicitly forward aria-* attributes to that wrapper so aria-labelledby is present on the visible/editable element.
- SqlInput change not required beyond passing aria-labelledby once InputLabel and SqlEditor are updated.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx
around lines 11-17, replace the hard-coded disabled={false} with a derived value
from useSearchStore() that checks for SEARCH_UI_STATE.QUERY_ID_PENDING or
SEARCH_UI_STATE.QUERYING, change guidedGrid["order"] and
guidedGrid["noLeftBorderRadius"] to dot-notation (guidedGrid.order /
guidedGrid.noLeftBorderRadius) and remove the redundant || "" fallback, pass an
id prop to InputLabel and set aria-labelledby on SqlInput to that id; update
InputLabel (components/.../InputLabel/index.tsx) to accept an optional id?:
string and forward it to the rendered <Text> as id={id}, and update SqlEditor
(components/.../SqlEditor/index.tsx) to either spread editorProps (or the aria-*
attributes) onto the outer wrapper div so aria-labelledby is present on the
visible editable element (no changes needed to SqlInput beyond passing
aria-labelledby).
| const Select = () => ( | ||
| <div className={guidedGrid["select"]}> | ||
| <InputLabel>SELECT</InputLabel> | ||
| <SqlInput | ||
| className={guidedGrid["noLeftBorderRadius"] || ""} | ||
| disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Derive disabled state; clean className; add a11y linkage.
Match behaviour with other controls and tidy props.
-import InputLabel from "../../../../../components/InputLabel";
-import SqlInput from "../../../../../components/SqlInput";
-import guidedGrid from "./index.module.css";
+import InputLabel from "../../../../../components/InputLabel";
+import SqlInput from "../../../../../components/SqlInput";
+import guidedGrid from "./index.module.css";
+import useSearchStore from "../../../SearchState/index";
+import {SEARCH_UI_STATE} from "../../../SearchState/typings";
@@
-const Select = () => (
- <div className={guidedGrid["select"]}>
- <InputLabel>SELECT</InputLabel>
- <SqlInput
- className={guidedGrid["noLeftBorderRadius"] || ""}
- disabled={false}/>
- </div>
-);
+const Select = () => {
+ const searchUiState = useSearchStore((s) => s.searchUiState);
+ const disabled =
+ searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING ||
+ searchUiState === SEARCH_UI_STATE.QUERYING;
+ return (
+ <div className={guidedGrid.select}>
+ <InputLabel id="select-label">SELECT</InputLabel>
+ <SqlInput
+ aria-labelledby="select-label"
+ className={guidedGrid.noLeftBorderRadius}
+ disabled={disabled}
+ />
+ </div>
+ );
+};If SqlInput doesn’t forward aria-*, adjust SqlEditor wrapper to accept aria-labelledby and apply it to the container div.
🏁 Script executed:
#!/bin/bash
# Check SqlEditor props for wrapper-level aria support
sed -n '1,160p' components/webui/client/src/components/SqlEditor/index.tsx
rg -n 'aria-' components/webui/client/src/components/SqlEditor/index.tsxLength of output: 3029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="components/webui/client/src/components/SqlInput/index.tsx"
if [ -f "$file" ]; then
sed -n '1,240p' "$file" || true
echo "----"
rg -n "aria-" "$file" || true
echo "----"
rg -n "SqlEditor" "$file" || true
else
echo "MISSING: $file"
rg -n --hidden --no-ignore -S "SqlInput" components/webui || true
fiLength of output: 2330
Derive disabled state; clean className; add a11y linkage.
Compute disabled from the search UI state, use guidedGrid.select / guidedGrid.noLeftBorderRadius, add an id on InputLabel and pass aria-labelledby on SqlInput (Select file: components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx). SqlInput correctly forwards props to SqlEditor, but components/webui/client/src/components/SqlEditor/index.tsx currently only forwards unknown props into and does not apply aria-* to its outer wrapper — update SqlEditor to accept and apply aria-labelledby (or spread React.AriaAttributes) onto the container div so the label properly targets the editor container.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx
around lines 11-17, derive the disabled value from the search UI state
(import/use the relevant hook/selectors), pass the proper className using
guidedGrid.select and guidedGrid.noLeftBorderRadius (not the || ""), add an id
on the InputLabel (e.g. id="select-label") and pass
aria-labelledby="select-label" to SqlInput; also update
components/webui/client/src/components/SqlEditor/index.tsx to accept and apply
React.AriaAttributes (at least aria-labelledby) by spreading aria props onto the
outer container div so the label correctly targets the editor container.
| const Where = () => ( | ||
| <div className={guidedGrid["where"]}> | ||
| <InputLabel>WHERE</InputLabel> | ||
| <SqlInput | ||
| className={guidedGrid["noLeftBorderRadius"] || ""} | ||
| disabled={false}/> | ||
| </div> |
There was a problem hiding this comment.
Derive disabled state; clean className; add a11y linkage.
Same concerns as SELECT/ORDER BY.
-import InputLabel from "../../../../../components/InputLabel";
-import SqlInput from "../../../../../components/SqlInput";
-import guidedGrid from "./index.module.css";
+import InputLabel from "../../../../../components/InputLabel";
+import SqlInput from "../../../../../components/SqlInput";
+import guidedGrid from "./index.module.css";
+import useSearchStore from "../../../SearchState/index";
+import {SEARCH_UI_STATE} from "../../../SearchState/typings";
@@
-const Where = () => (
- <div className={guidedGrid["where"]}>
- <InputLabel>WHERE</InputLabel>
- <SqlInput
- className={guidedGrid["noLeftBorderRadius"] || ""}
- disabled={false}/>
- </div>
-);
+const Where = () => {
+ const searchUiState = useSearchStore((s) => s.searchUiState);
+ const disabled =
+ searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING ||
+ searchUiState === SEARCH_UI_STATE.QUERYING;
+ return (
+ <div className={guidedGrid.where}>
+ <InputLabel id="where-label">WHERE</InputLabel>
+ <SqlInput
+ aria-labelledby="where-label"
+ className={guidedGrid.noLeftBorderRadius}
+ disabled={disabled}
+ />
+ </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 Where = () => ( | |
| <div className={guidedGrid["where"]}> | |
| <InputLabel>WHERE</InputLabel> | |
| <SqlInput | |
| className={guidedGrid["noLeftBorderRadius"] || ""} | |
| disabled={false}/> | |
| </div> | |
| import InputLabel from "../../../../../components/InputLabel"; | |
| import SqlInput from "../../../../../components/SqlInput"; | |
| import guidedGrid from "./index.module.css"; | |
| import useSearchStore from "../../../SearchState/index"; | |
| import {SEARCH_UI_STATE} from "../../../SearchState/typings"; | |
| const Where = () => { | |
| const searchUiState = useSearchStore((s) => s.searchUiState); | |
| const disabled = | |
| searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || | |
| searchUiState === SEARCH_UI_STATE.QUERYING; | |
| return ( | |
| <div className={guidedGrid.where}> | |
| <InputLabel id="where-label">WHERE</InputLabel> | |
| <SqlInput | |
| aria-labelledby="where-label" | |
| className={guidedGrid.noLeftBorderRadius} | |
| disabled={disabled} | |
| /> | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx
around lines 11-17, derive the SqlInput disabled state from the component's
props/context instead of hardcoding false, clean up the className by using a
proper conditional join (or clsx) rather than using "|| ''", and add proper
accessibility linkage by giving the SqlInput a stable id and setting
InputLabel's htmlFor to that id (mirror the same approach used for SELECT/ORDER
BY); update the component to accept/obtain the disabled flag, compute className
cleanly, generate or pass an id, and wire htmlFor to that id.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx (1)
24-39: Guard against undefined and non-numeric IDs before cancelling.Strict null checks won’t catch
undefined, andNumber(undefined)becomesNaN. Add explicitundefinedguards and validate numerics before callinghandleQueryCancel.- if (null === searchJobId) { + if (null === searchJobId || undefined === searchJobId) { console.error("Cannot cancel query: searchJobId is not set."); return; } - if (null === aggregationJobId) { + if (null === aggregationJobId || undefined === aggregationJobId) { console.error("Cannot cancel query: aggregationJobId is not set."); return; } - handleQueryCancel( - { - searchJobId: Number(searchJobId), - aggregationJobId: Number(aggregationJobId), - } - ); + const sid = Number(searchJobId); + const aid = Number(aggregationJobId); + if (false == Number.isFinite(sid) || false == Number.isFinite(aid)) { + console.error("Cannot cancel query: job ids are not numeric."); + return; + } + handleQueryCancel({searchJobId: sid, aggregationJobId: aid});components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (1)
36-40: Simplify initial-selection checkAvoid indexing
data[0]; check length explicitly.- useEffect(() => { - if (isSuccess) { - if ("undefined" !== typeof data[0] && null === dataset) { - updateDataset(data[0]); - } - } - }, [isSuccess, - data, - dataset, - updateDataset]); + useEffect(() => { + if (isSuccess && data.length > 0 && dataset === null) { + updateDataset(data[0]); + } + }, [isSuccess, data, dataset, updateDataset]);
♻️ Duplicate comments (24)
components/webui/client/src/components/QueryBox/InputWithCaseSensitive/index.module.css (1)
1-3: Use theme token and set line-height to avoid clipping; align with AntD sizing.Hard-coded 22px can drift from theme and clip text. Prefer AntD token and match line-height; using inline-flex centres content reliably.
.customButton { - height: 22px; + height: var(--ant-control-height-sm); + line-height: var(--ant-control-height-sm); + display: inline-flex; + align-items: center; }components/webui/client/src/components/SqlEditor/index.tsx (2)
86-89: Don’t disable pointer events; use Monaco readOnly so users can still select/copy.Make the editor non-editable via options.readOnly and keep pointer events enabled.
style={{ border: `1px solid ${token.colorBorder}`, borderRadius: token.borderRadius, - pointerEvents: disabled ? - "none" : - "auto", }} > <Editor language={"sql"} @@ - onMount={handleEditorDidMount} - {...editorProps}/> + onMount={handleEditorDidMount} + {...editorProps} + options={{ + ...(editorProps.options ?? {}), + readOnly: disabled, + }} + />
17-17: Ensure SQL IntelliSense prerequisites are loaded (editor.all.js).Per prior repo learning, SQL autocomplete requires importing monaco-editor/esm/vs/editor/editor.all.js in your loader. Please confirm monaco-loader includes it; if not, add it.
#!/bin/bash # Verify editor.all.js and SQL contribution are imported somewhere in webui rg -nP 'monaco-editor/esm/vs/editor/editor\.all\.js|basic-languages/sql/sql\.contribution' components/webuiIf missing, update monaco-loader:
// components/webui/client/src/components/SqlEditor/monaco-loader.ts +import "monaco-editor/esm/vs/editor/editor.all.js"; import "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js";components/webui/client/src/components/InputLabel/index.module.css (2)
4-5: Use padding shorthandSame effect, fewer declarations.
- padding-left: 10px; - padding-right: 10px; + padding: 0 10px;
10-10: Prevent label overflow on narrow layoutsTruncate long labels with an ellipsis to avoid clipping.
- white-space: nowrap; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis;components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx (2)
82-90: Link the label to the Select for a11yExpose the label via id and reference it from Select.
- <InputLabel>Dataset</InputLabel> + <InputLabel id="dataset-label">Dataset</InputLabel> <Select + aria-labelledby="dataset-label"Please confirm InputLabel forwards arbitrary DOM props (id) to a semantic element.
#!/bin/bash # Verify InputLabel forwards 'id' to a DOM node rg -nC2 'function InputLabel|const InputLabel' components/webui/client/src/components/InputLabel rg -nC2 '\bprops\.id\b|\{.*id[,}]' components/webui/client/src/components/InputLabel
80-85: Remove redundant fallbacks; prefer dot notation for CSS modulesCSS modules return strings;
|| ""is unnecessary. Dot notation is cleaner.- <div className={styles["datasetContainer"]}> + <div className={styles.datasetContainer}> @@ - <Select - className={styles["select"] || ""} + <Select + className={styles.select}components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx (4)
1-3: Prefer path aliases over deep relative importsSwitch to configured TS path aliases for resilience to moves.
-import InputLabel from "../../../../../components/InputLabel"; -import SqlInput from "../../../../../components/SqlInput"; -import guidedGrid from "./index.module.css"; +import InputLabel from "@components/InputLabel"; +import SqlInput from "@components/SqlInput"; +import guidedGrid from "@pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css";Confirm the alias names available in tsconfig.json before applying.
#!/bin/bash jq '.compilerOptions.paths' -r tsconfig.json 2>/dev/null || rg -n '"paths"\s*:' -n tsconfig*.json
11-18: Derive disabled state from store and add a11y labellingDisable edits while querying and associate label to editor.
-const OrderBy = () => ( - <div className={guidedGrid["order"]}> - <InputLabel>ORDER BY</InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> - </div> -); +import useSearchStore from "../../../SearchState"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; + +const OrderBy = () => { + const state = useSearchStore((s) => s.searchUiState); + const disabled = + state === SEARCH_UI_STATE.QUERY_ID_PENDING || + state === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.order}> + <InputLabel id="orderby-label">ORDER BY</InputLabel> + <SqlInput + aria-labelledby="orderby-label" + className={guidedGrid.noLeftBorderRadius} + disabled={disabled}/> + </div> + ); +};
6-10: Tighten JSDoc tagUse @returns for clarity; keep code return type implicit.
/** * Renders the ORDER BY SQL input field. * - * @return + * @returns JSX.Element */
15-16: Drop redundant props/fallbacks
disabled={false}is unnecessary, and CSS module class is never falsy.- <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> + <SqlInput className={guidedGrid.noLeftBorderRadius}/>components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx (3)
15-16: Drop redundant props/fallbacksSame cleanups as OrderBy.
- <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> + <SqlInput className={guidedGrid.noLeftBorderRadius}/>
12-16: Wire disabled state to store and add a11y labellingPrevent edits during querying and associate label to editor.
-const Where = () => ( - <div className={guidedGrid["where"]}> - <InputLabel>WHERE</InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> - </div> -); +import useSearchStore from "../../../SearchState"; +import {SEARCH_UI_STATE} from "../../../SearchState/typings"; + +const Where = () => { + const state = useSearchStore((s) => s.searchUiState); + const disabled = + state === SEARCH_UI_STATE.QUERY_ID_PENDING || + state === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid.where}> + <InputLabel id="where-label">WHERE</InputLabel> + <SqlInput + aria-labelledby="where-label" + className={guidedGrid.noLeftBorderRadius} + disabled={disabled}/> + </div> + ); +};
6-10: Tighten JSDoc tagUse @returns for consistency.
/** * Renders the WHERE SQL input field. * - * @return + * @returns JSX.Element */components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx (4)
22-22: Label whitespace fix looks good.Removing the surrounding spaces resolves the earlier nit and avoids i18n/layout surprises.
15-31: Make LIMIT controllable, disabled by search state, accessible, and size=middle.Persisting the value, disabling during query, and adding an accessible label improves UX and aligns with the PR’s “middle” sizing. This also matches earlier feedback on this component.
Apply this diff within the component:
-/** - * Renders the LIMIT SQL input field. - * - * @return - */ -const Limit = () => ( - <div className={guidedGrid["limit"]}> - <InputLabel>LIMIT</InputLabel> - <Select - defaultValue={LIMIT_OPTIONS[0]?.value} - options={LIMIT_OPTIONS} - className={ - `${guidedGrid["noLeftBorderRadiusSelect"]} ${ - guidedGrid["widthSelect"]}` - }/> - </div> -); +type LimitProps = { + value?: number; + onChange?: (v: number) => void; +}; + +const Limit = ({value, onChange}: LimitProps) => { + const searchUiState = useSearchStore((s) => s.searchUiState); + const disabled = + searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || + searchUiState === SEARCH_UI_STATE.QUERYING; + return ( + <div className={guidedGrid["limit"]}> + <InputLabel>LIMIT</InputLabel> + <Select<number> + aria-label="LIMIT" + value={value ?? DEFAULT_LIMIT} + onChange={onChange} + options={LIMIT_OPTIONS} + size="middle" + disabled={disabled} + className={ + `${guidedGrid["noLeftBorderRadiusSelect"]} ${guidedGrid["widthSelect"]}` + }/> + </div> + ); +};Add the required imports outside this range:
import useSearchStore from "../../../SearchState"; import {SEARCH_UI_STATE} from "../../../SearchState/typings";
24-24: Drop unnecessary optional chaining.Index 0 is guaranteed by the array literal;
?.is redundant.- defaultValue={LIMIT_OPTIONS[0]?.value} + defaultValue={LIMIT_OPTIONS[0].value}
7-14: Type the options and expose a DEFAULT_LIMIT.Makes intent explicit and avoids “magic 10” in consumers/tests.
-const LIMIT_OPTIONS = [ +const LIMIT_OPTIONS: { value: number; label: string }[] = [ {value: 10, label: "10"}, {value: 50, label: "50"}, {value: 100, label: "100"}, {value: 500, label: "500"}, {value: 1000, label: "1000"}, ]; + +export const DEFAULT_LIMIT = 10;components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (1)
1-3: Use path aliases instead of deep relatives (match Where.tsx).Align these imports with the alias style used elsewhere to avoid brittle paths and ease refactors.
Run to compare with Where.tsx and confirm tsconfig paths:
#!/bin/bash # Show how Where.tsx imports components/CSS fd Where.tsx -t f | xargs -I{} sh -c 'echo "== {} =="; sed -n "1,20p" "{}"' # Inspect tsconfig path mappings fd tsconfig.json -t f | xargs -I{} sh -c 'echo "== {} =="; cat "{}" | jq ".compilerOptions.paths"'components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (5)
13-13: Label text cleanup looks good.Whitespace fixed around “FROM”.
1-3: Use path aliases instead of deep relative imports.Shorter, move‑resilient imports.
-import InputLabel from "../../../../../components/InputLabel"; -import SqlInput from "../../../../../components/SqlInput"; +import InputLabel from "@components/InputLabel"; +import SqlInput from "@components/SqlInput";If aliases differ, I can adjust the patch.
6-10: Complete JSDoc; use @returns.Also keep no explicit TS return type per our React preference.
/** * Renders the FROM SQL input field. * - * @return + * @returns {JSX.Element} */ ```<!-- review_comment_end --> --- `15-16`: **Drop redundant className fallback and disabled={false}.** Both are unnecessary. ```diff - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> + className={guidedGrid["noLeftBorderRadius"]}/> ```<!-- review_comment_end --> --- `12-15`: **Prefer dot notation with CSS module keys.** Cleaner and typed; keys are identifiers. ```diff - <div className={guidedGrid["from"]}> + <div className={guidedGrid.from}> @@ - className={guidedGrid["noLeftBorderRadius"]}/> + className={guidedGrid.noLeftBorderRadius}/> ```<!-- review_comment_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: ASSERTIVE **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 10f9a2d27bdc4929dc4f4677dd2f69031de889ef and 0db25b1c6f4d8921b9b0aa3a55d48253df400eb0. </details> <details> <summary>📒 Files selected for processing (11)</summary> * `components/webui/client/src/components/InputLabel/index.module.css` (1 hunks) * `components/webui/client/src/components/QueryBox/InputWithCaseSensitive/index.module.css` (1 hunks) * `components/webui/client/src/components/SqlEditor/index.module.css` (1 hunks) * `components/webui/client/src/components/SqlEditor/index.tsx` (3 hunks) * `components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx` (2 hunks) * `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx` (1 hunks) * `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx` (1 hunks) * `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx` (1 hunks) * `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx` (1 hunks) * `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx` (1 hunks) * `components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}</summary> **⚙️ CodeRabbit configuration file** > - Prefer `false == <expression>` rather than `!<expression>`. Files: - `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx` - `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx` - `components/webui/client/src/components/SqlEditor/index.tsx` - `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx` - `components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx` - `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx` - `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx` - `components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx` </details> </details><details> <summary>🧠 Learnings (3)</summary> <details> <summary>📚 Learning: 2025-05-29T20:33:40.653Z</summary>Learnt from: junhaoliao
PR: #937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like: JSX.Element) are unnecessary and not preferred for React components in JSX/TSX files.**Applied to files:** - `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx` - `components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx` </details> <details> <summary>📚 Learning: 2025-07-21T14:37:57.648Z</summary>Learnt from: davemarco
PR: #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` </details> <details> <summary>📚 Learning: 2025-08-26T13:45:46.445Z</summary>Learnt from: davemarco
PR: #1244
File: components/webui/client/package.json:29-29
Timestamp: 2025-08-26T13:45:46.445Z
Learning: The "color" package in components/webui/client/package.json is used in the SqlEditor component to convert Ant Design token colors to hex format for Monaco editor themes, specifically for the disabled state styling.**Applied to files:** - `components/webui/client/src/components/SqlEditor/index.tsx` </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: package-image </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx (1)</summary><blockquote> `48-48`: **UI density alignment: size="middle" change looks good.** Matches the PR’s standardization to “middle”; verify spacing with adjacent controls in SearchControls. </blockquote></details> <details> <summary>components/webui/client/src/components/SqlEditor/index.tsx (3)</summary><blockquote> `15-16`: **CSS module import looks good.** Scoped styling via CSS modules for the wrapper is appropriate. --- `24-24`: **Public className is a good addition.** Allows external layout control without leaking internals. --- `37-37`: **Prop destructuring LGTM.** Clean separation of wrapper props vs Editor props. </blockquote></details> <details> <summary>components/webui/client/src/components/InputLabel/index.module.css (1)</summary><blockquote> `8-9`: **RHS radii reset looks good** Matches adjacent control styling. </blockquote></details> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx (1)</summary><blockquote> `1-4`: **Incorrect — no `AntdSelect` aliasing found; revise review** Search of GuidedControls shows: - components/.../GuidedControls/Limit.tsx imports { Select } from "antd" (line 1). - components/.../GuidedControls/Select.tsx defines and exports a local default Select; index.tsx imports that local Select (import Select from "./Select"). - No imports using `Select as AntdSelect` were found. Action: either alias the antd import where both are needed (import { Select as AntdSelect } from "antd") or rename the local component to avoid name collision. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (1)</summary><blockquote> `11-18`: **Clean JSX, drop redundant props, and add accessible labelling.** Switch to CSS-module dot notation; remove `disabled={false}`; link label → editor with `aria-labelledby`; add explicit return type. File: components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (lines 11-18) ```diff +import { useId } from "react"; import InputLabel from "../../../../../components/InputLabel"; import SqlInput from "../../../../../components/SqlInput"; import guidedGrid from "./index.module.css"; -const Select = () => ( - <div className={guidedGrid["select"]}> - <InputLabel>SELECT</InputLabel> - <SqlInput - className={guidedGrid["noLeftBorderRadius"] || ""} - disabled={false}/> - </div> -); +const Select = (): JSX.Element => { + const labelId = useId(); + return ( + <div className={guidedGrid.select}> + <InputLabel id={labelId}>SELECT</InputLabel> + <SqlInput + className={guidedGrid.noLeftBorderRadius} + aria-labelledby={labelId} + /> + </div> + ); +};Could not locate
SqlInputin repo search — confirmSqlInputforwards aria-* props / acceptsaria-labelledby(forwardRef/props forwarding). If it doesn't, I will provide the wiring.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (1)
11-18: Associate label and editor: add stable id/htmlFor and confirm it reaches the focusable editor
- In components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx use useId(), set and pass id={inputId} to .
- SqlInput (components/webui/client/src/components/SqlInput/index.tsx) currently spreads props into SqlEditor, and SqlEditor (components/webui/client/src/components/SqlEditor/index.tsx) spreads remaining props into @monaco-editor/react’s . Confirm the Editor actually applies a received id to the focusable DOM node.
- If the Editor does not forward id, modify SqlEditor to forward it to the real DOM wrapper (e.g. set id on the surrounding
or pass wrapperProps={{ id }} to ), or use aria-labelledby on the editor container and give the label an id.- Verify keyboard focus and screen-reader announcement after the change.
| .editor { | ||
| height: 100%; | ||
| min-height: 0; | ||
| min-width: 0; | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Add box-sizing to prevent overflow when borders are applied.
With height/width at 100% and a 1px border added inline on the wrapper, content-box sizing can overflow its grid cell. Use border-box.
.editor {
height: 100%;
min-height: 0;
min-width: 0;
width: 100%;
+ box-sizing: border-box;
}📝 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.
| .editor { | |
| height: 100%; | |
| min-height: 0; | |
| min-width: 0; | |
| width: 100%; | |
| } | |
| .editor { | |
| height: 100%; | |
| min-height: 0; | |
| min-width: 0; | |
| width: 100%; | |
| box-sizing: border-box; | |
| } |
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/index.module.css around
lines 1 to 6, the .editor class uses 100% height/width and can overflow when a
1px border is applied because the default content-box sizing excludes borders;
change the box model to border-box by adding box-sizing: border-box to the
.editor selector so borders are included in the element's dimensions and prevent
grid-cell overflow.
| className={[styles["editor"], | ||
| className].filter(Boolean).join(" ")} |
There was a problem hiding this comment.
🧹 Nitpick
Optional: simplify class merge and prefer dot access.
If clsx (or classnames) is already in the repo, use it; otherwise dot access alone improves readability.
- className={[styles["editor"],
- className].filter(Boolean).join(" ")}
+ className={[styles.editor, className].filter(Boolean).join(" ")}
+ // or: className={clsx(styles.editor, className)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/index.tsx around lines
81-82, the className construction uses array filter/join and bracket property
access which is verbose; if the repo already includes clsx or classnames,
replace the array/filter/join with a single clsx(...) call (e.g.,
clsx(styles.editor, className)); otherwise simplify to dot access and
conditional concatenation using styles.editor and className (e.g., styles.editor
+ (className ? ` ${className}` : '')). Ensure you import clsx if used and remove
the unnecessary array/filter/join pattern.
| placeholder={"None"} | ||
| showSearch={true} | ||
| size={"large"} | ||
| size={"middle"} |
There was a problem hiding this comment.
Let's also set the size to "middle" in components/webui/client/src/pages/SearchPage/SearchControls/Dataset/index.tsx so they match
There was a problem hiding this comment.
isnt that just this file, where it is already set?
There was a problem hiding this comment.
Sorry i think i was using an older build. I confirmed that the visuals are fine in the latest commit
Description
PR adds dynamically adjusting css grid to hold single line monaco editor components. Was tricky getting this to resize correctly, let me know if u see issues. Should resize automically with page size. Also adds a new label components.
I also had to change the font sizes of the native ui. It was too annoying to work with 2 different font sizes and i think kirk will prefer
Timerange and timestamp key will come later. I want to put next to run button outside of grid
Checklist
breaking change.
Validation performed
Summary by CodeRabbit