-
Notifications
You must be signed in to change notification settings - Fork 83
feat(webui): Add SQL input boxes to guided presto UI. #1300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 25 commits
004a12d
7194d78
d5fc91f
dc79169
52e83a3
630a360
aa3968c
fb61505
af4a2b0
202d002
6aa31be
2cc9f2f
e30211e
a8daeb2
148379c
d8ec46c
84a5840
6c3a065
8da8133
cfae7cc
7a3b7f9
8d41f91
10f9a2d
1f2a97d
96b46a1
7eb61ac
847da48
0631199
96e8965
0db25b1
a18abc3
c5d1256
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,11 @@ | ||||||||||||||
| .label { | ||||||||||||||
| height: 100%; | ||||||||||||||
| display: flex; | ||||||||||||||
| padding-left: 10px; | ||||||||||||||
| padding-right: 10px; | ||||||||||||||
| align-items: center; | ||||||||||||||
| justify-content: center; | ||||||||||||||
| border-top-right-radius: 0; | ||||||||||||||
| border-bottom-right-radius: 0; | ||||||||||||||
| white-space: nowrap; | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Prevent label overflow on narrow layouts
.label {
@@
- white-space: nowrap;
+ white-space: nowrap;
+ overflow: hidden;
+ text-overflow: ellipsis;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| } | ||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,27 +9,28 @@ import styles from "./index.module.css"; | |||||||||||||||||||||||||||||||||
| const {Text} = Typography; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * 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 | ||||||||||||||||||||||||||||||||||
|
Comment on lines
11
to
16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Document a11y props. Add docs for /**
- * 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| const DatasetLabel = () => { | ||||||||||||||||||||||||||||||||||
| const InputLabel = ({children}: {children: React.ReactNode}) => { | ||||||||||||||||||||||||||||||||||
| const {token} = theme.useToken(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||
| <Text | ||||||||||||||||||||||||||||||||||
| className={styles["datasetLabel"] || ""} | ||||||||||||||||||||||||||||||||||
| className={styles["label"] || ""} | ||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Remove CSS modules always yield a string; the fallback is unnecessary and was removed in the diff above. 🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Drop redundant fallback
- className={styles["label"] || ""}
+ className={styles.label}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| 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> | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+18
to
33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 🧩 Analysis chainVerify 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)
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| export default DatasetLabel; | ||||||||||||||||||||||||||||||||||
| export default InputLabel; | ||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this change directly related to the current PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,3 @@ | ||||||||||||||||||||||||||||||||||||
| .customButton { | ||||||||||||||||||||||||||||||||||||
| height: 22px; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| .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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| } | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| .editor { | ||
| height: 100%; | ||
| min-height: 0; | ||
| min-width: 0; | ||
| width: 100%; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,13 +12,16 @@ import {theme} from "antd"; | |
| import color from "color"; | ||
| import * as monaco from "monaco-editor/esm/vs/editor/editor.api.js"; | ||
|
|
||
| import styles from "./index.module.css"; | ||
|
|
||
| import "./monaco-loader"; | ||
|
|
||
|
|
||
| type SqlEditorType = monaco.editor.IStandaloneCodeEditor; | ||
|
|
||
| type SqlEditorProps = Omit<EditorProps, "language"> & { | ||
| disabled: boolean; | ||
| className?: string; | ||
|
|
||
| /** Callback when the editor is mounted and ref is ready to use. */ | ||
| onEditorReady?: (editor: SqlEditorType) => void; | ||
|
|
@@ -31,7 +34,7 @@ type SqlEditorProps = Omit<EditorProps, "language"> & { | |
| * @return | ||
| */ | ||
| const SqlEditor = (props: SqlEditorProps) => { | ||
| const {disabled, onEditorReady, ...editorProps} = props; | ||
| const {disabled, onEditorReady, className, ...editorProps} = props; | ||
| const monacoEditor = useMonaco(); | ||
| const {token} = theme.useToken(); | ||
|
|
||
|
|
@@ -75,6 +78,8 @@ const SqlEditor = (props: SqlEditorProps) => { | |
|
|
||
| return ( | ||
| <div | ||
| className={[styles["editor"], | ||
| className].filter(Boolean).join(" ")} | ||
|
Comment on lines
+81
to
+82
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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)}
🤖 Prompt for AI Agents |
||
| style={{ | ||
| border: `1px solid ${token.colorBorder}`, | ||
| borderRadius: token.borderRadius, | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,9 +6,9 @@ import { | |||||||||||
| Select, | ||||||||||||
| } from "antd"; | ||||||||||||
|
|
||||||||||||
| import InputLabel from "../../../../components/InputLabel"; | ||||||||||||
| import useSearchStore from "../../SearchState/index"; | ||||||||||||
| import {SEARCH_UI_STATE} from "../../SearchState/typings"; | ||||||||||||
| import DatasetLabel from "./DatasetLabel"; | ||||||||||||
| import styles from "./index.module.css"; | ||||||||||||
| import {fetchDatasetNames} from "./sql"; | ||||||||||||
|
|
||||||||||||
|
|
@@ -79,14 +79,14 @@ const Dataset = () => { | |||||||||||
| return ( | ||||||||||||
| <div className={styles["datasetContainer"]}> | ||||||||||||
| {contextHolder} | ||||||||||||
| <DatasetLabel/> | ||||||||||||
| <InputLabel>Dataset</InputLabel> | ||||||||||||
| <Select | ||||||||||||
|
Comment on lines
+82
to
83
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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 📝 Committable suggestion
Suggested change
|
||||||||||||
| className={styles["select"] || ""} | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Remove redundant fallback in className.
- className={styles["select"] || ""}
+ className={styles["select"]}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Drop redundant fallback in className. CSS modules yield a string; the Apply: - className={styles["select"] || ""}
+ className={styles.select}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| loading={isPending} | ||||||||||||
| options={(data || []).map((option) => ({label: option, value: option}))} | ||||||||||||
| placeholder={"None"} | ||||||||||||
| showSearch={true} | ||||||||||||
| size={"large"} | ||||||||||||
| size={"middle"} | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also set the size to "middle" in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isnt that just this file, where it is already set?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry i think i was using an older build. I confirmed that the visuals are fine in the latest commit |
||||||||||||
| value={dataset} | ||||||||||||
| disabled={ | ||||||||||||
| searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || | ||||||||||||
|
|
||||||||||||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||||||||||
| import InputLabel from "../../../../../components/InputLabel"; | ||||||||||||||||||||||
| import SqlInput from "../../../../../components/SqlInput"; | ||||||||||||||||||||||
| import guidedGrid from "./index.module.css"; | ||||||||||||||||||||||
|
Comment on lines
+1
to
+3
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Prefer import aliases. Same rationale as above. 🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Renders the FROM SQL input field. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @return | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
|
Comment on lines
+6
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| const From = () => ( | ||||||||||||||||||||||
| <div className={guidedGrid["from"]}> | ||||||||||||||||||||||
| <InputLabel>FROM</InputLabel> | ||||||||||||||||||||||
| <SqlInput | ||||||||||||||||||||||
| className={guidedGrid["noLeftBorderRadius"] || ""} | ||||||||||||||||||||||
| disabled={false}/> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export default From; | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import {Select} from "antd"; | ||
|
|
||
| import InputLabel from "../../../../../components/InputLabel"; | ||
| import guidedGrid from "./index.module.css"; | ||
|
|
||
|
|
||
| const LIMIT_OPTIONS = [ | ||
| {value: 10, label: "10"}, | ||
| {value: 50, label: "50"}, | ||
| {value: 100, label: "100"}, | ||
| {value: 500, label: "500"}, | ||
| {value: 1000, label: "1000"}, | ||
| ]; | ||
|
Comment on lines
+7
to
+13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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 |
||
|
|
||
| /** | ||
| * Renders the LIMIT SQL input field. | ||
| * | ||
| * @return | ||
| */ | ||
| const Limit = () => ( | ||
| <div className={guidedGrid["limit"]}> | ||
| <InputLabel>LIMIT</InputLabel> | ||
| <Select | ||
| defaultValue={LIMIT_OPTIONS[0]?.value} | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| options={LIMIT_OPTIONS} | ||
| className={ | ||
| `${guidedGrid["noLeftBorderRadiusSelect"]} ${ | ||
| guidedGrid["widthSelect"]}` | ||
| }/> | ||
| </div> | ||
| ); | ||
|
|
||
| export default Limit; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||||||||||
| import InputLabel from "../../../../../components/InputLabel"; | ||||||||||||||||||||||
| import SqlInput from "../../../../../components/SqlInput"; | ||||||||||||||||||||||
| import guidedGrid from "./index.module.css"; | ||||||||||||||||||||||
|
Comment on lines
+1
to
+3
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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 |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Renders the ORDER BY SQL input field. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @return | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
|
Comment on lines
+6
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| const OrderBy = () => ( | ||||||||||||||||||||||
| <div className={guidedGrid["order"]}> | ||||||||||||||||||||||
| <InputLabel>ORDER BY</InputLabel> | ||||||||||||||||||||||
| <SqlInput | ||||||||||||||||||||||
| className={guidedGrid["noLeftBorderRadius"] || ""} | ||||||||||||||||||||||
| disabled={false}/> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
|
Comment on lines
+11
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 🧩 Analysis chainDerive 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.
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export default OrderBy; | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import InputLabel from "../../../../../components/InputLabel"; | ||
| import SqlInput from "../../../../../components/SqlInput"; | ||
| import guidedGrid from "./index.module.css"; | ||
|
Comment on lines
+1
to
+3
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Prefer import aliases. Same rationale as in Where.tsx. See tsconfig check script there. 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| /** | ||
| * Renders the SELECT SQL input field. | ||
| * | ||
| * @return | ||
| */ | ||
| const Select = () => ( | ||
| <div className={guidedGrid["select"]}> | ||
| <InputLabel>SELECT</InputLabel> | ||
| <SqlInput | ||
| className={guidedGrid["noLeftBorderRadius"] || ""} | ||
| disabled={false}/> | ||
| </div> | ||
|
Comment on lines
+11
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 🧩 Analysis chainDerive 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 |
||
| ); | ||
|
|
||
| export default Select; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import InputLabel from "../../../../../components/InputLabel"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import SqlInput from "../../../../../components/SqlInput"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import guidedGrid from "./index.module.css"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Renders the WHERE SQL input field. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const Where = () => ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className={guidedGrid["where"]}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <InputLabel>WHERE</InputLabel> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <SqlInput | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| className={guidedGrid["noLeftBorderRadius"] || ""} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disabled={false}/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+11
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export default Where; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| .gridContainer { | ||
| display: grid; | ||
| grid-template-columns: repeat(auto-fit, minmax(200px, 1fr)); | ||
| grid-template-rows: 32px; | ||
| gap: 5px; | ||
| } | ||
|
|
||
| .select { | ||
| grid-column: span 3; | ||
| display: flex; | ||
| } | ||
|
|
||
| .from { | ||
| grid-column: span 1; | ||
| display: flex; | ||
| } | ||
|
|
||
| .where { | ||
| grid-column: span 2; | ||
| display: flex; | ||
| } | ||
|
|
||
| .order { | ||
| grid-column: span 1; | ||
| display: flex; | ||
| } | ||
|
|
||
| .limit { | ||
| grid-column: span 1; | ||
| display: flex; | ||
| } | ||
|
|
||
| .widthSelect { | ||
| width: 100%; | ||
| } | ||
|
|
||
| .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; | ||
| } | ||
|
Comment on lines
+37
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import searchStyles from "../../index.module.css"; | ||
| import QueryStatus from "../../QueryStatus"; | ||
| import SqlInterfaceButton from "../SqlInterfaceButton"; | ||
| import SqlSearchButton from "../SqlSearchButton"; | ||
| import From from "./From"; | ||
| import guidedGrid from "./index.module.css"; | ||
| import Limit from "./Limit"; | ||
| import OrderBy from "./OrderBy"; | ||
| import Select from "./Select"; | ||
| import Where from "./Where"; | ||
|
|
||
|
|
||
| /** | ||
| * Renders controls and status for guided sql. | ||
| * | ||
| * @return | ||
| */ | ||
| const GuidedControls = () => ( | ||
| <div className={searchStyles["searchControlsContainer"]}> | ||
| <div className={guidedGrid["gridContainer"]}> | ||
| <Select/> | ||
| <From/> | ||
| <Where/> | ||
| <OrderBy/> | ||
|
Comment on lines
+21
to
+24
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we remove the left border radius? e.g., |
||
| <Limit/> | ||
| </div> | ||
|
Comment on lines
+20
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Grid + Monaco: verify track sizing to avoid overflow and layout thrash Ensure the grid container and items establish explicit heights (e.g., via grid-auto-rows or minmax) and set min-height: 0 on flex/grid wrappers so Monaco’s automaticLayout works reliably on resize. 🤖 Prompt for AI Agents |
||
| <div className={searchStyles["statusAndButtonsRow"]}> | ||
| <div className={searchStyles["status"]}> | ||
| <QueryStatus/> | ||
| </div> | ||
| <div className={searchStyles["buttons"]}> | ||
| <SqlInterfaceButton/> | ||
| <SqlSearchButton/> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
|
|
||
| export default GuidedControls; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Minor: use padding shorthand
Same effect, simpler.
🤖 Prompt for AI Agents