Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion components/webui/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
"build": "tsc -b && vite build",
"lint:check": "eslint",
"lint:fix": "eslint --fix",
"start": "vite"
"start": "vite",
"guided": "VITE_GUIDED_DEV=true npm run start"
Comment on lines 7 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add a production build variant for the guided mode.

Ensure the flag is exercised in CI and producible bundles.

   "scripts": {
     "build": "tsc -b && vite build",
+    "build:guided": "cross-env VITE_GUIDED_DEV=true vite build",

Please confirm any runtime checks use string coercion, e.g., import.meta.env.VITE_GUIDED_DEV === "true".

📝 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.

Suggested change
"build": "tsc -b && vite build",
"lint:check": "eslint",
"lint:fix": "eslint --fix",
"start": "vite"
"start": "vite",
"guided": "VITE_GUIDED_DEV=true npm run start"
"scripts": {
"build": "tsc -b && vite build",
"build:guided": "cross-env VITE_GUIDED_DEV=true vite build",
"lint:check": "eslint",
"lint:fix": "eslint --fix",
"start": "vite",
"guided": "VITE_GUIDED_DEV=true npm run start"
}
🤖 Prompt for AI Agents
In components/webui/client/package.json around lines 7 to 11, add a production
build variant for guided mode and ensure CI builds it using a string-typed env
flag; add a script such as "build:guided" that sets VITE_GUIDED_DEV=true and
runs the normal build, update CI to run that new script, and in runtime checks
ensure you compare import.meta.env.VITE_GUIDED_DEV === "true" (use strict string
coercion) wherever the guided flag is tested.

},
"author": "YScope Inc. <[email protected]>",
"license": "Apache-2.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import {EditOutlined} from "@ant-design/icons";
import {Button} from "antd";

import usePrestoSearchState from "../../../../SearchState/Presto";
import {PRESTO_SQL_INTERFACE} from "../../../../SearchState/Presto/typings";


/**
* Renders a button to switch to Freeform SQL interface.
*
* @return
*/
const FreeformButton = () => {
const setSqlInterface = usePrestoSearchState((state) => state.setSqlInterface);

const handleClick = () => {
setSqlInterface(PRESTO_SQL_INTERFACE.FREEFORM);
};

return (
<Button
block={true}
color={"default"}
icon={<EditOutlined/>}
size={"middle"}
variant={"filled"}
onClick={handleClick}
>
Freeform
</Button>
);
};


export default FreeformButton;
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import {AppstoreOutlined} from "@ant-design/icons";
import {Button} from "antd";

import usePrestoSearchState from "../../../../SearchState/Presto";
import {PRESTO_SQL_INTERFACE} from "../../../../SearchState/Presto/typings";


/**
* Renders a button to switch to Guided SQL interface.
*
* @return
*/
const GuidedButton = () => {
const setSqlInterface = usePrestoSearchState((state) => state.setSqlInterface);

const handleClick = () => {
setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED);
};
Comment on lines +13 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add accessible pressed state and avoid redundant no-op clicks.

Expose aria-pressed so screen readers know the active state; also avoid re-setting the same value.

Apply:

-const GuidedButton = () => {
-    const setSqlInterface = usePrestoSearchState((state) => state.setSqlInterface);
+const GuidedButton = () => {
+    const sqlInterface = usePrestoSearchState((s) => s.sqlInterface);
+    const setSqlInterface = usePrestoSearchState((s) => s.setSqlInterface);
 
-    const handleClick = () => {
-        setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED);
-    };
+    const handleClick = () => {
+        if (sqlInterface !== PRESTO_SQL_INTERFACE.GUIDED) {
+            setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED);
+        }
+    };

And in the Button:

-        <Button
+        <Button
+            aria-pressed={sqlInterface === PRESTO_SQL_INTERFACE.GUIDED}
📝 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.

Suggested change
const GuidedButton = () => {
const setSqlInterface = usePrestoSearchState((state) => state.setSqlInterface);
const handleClick = () => {
setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED);
};
const GuidedButton = () => {
const sqlInterface = usePrestoSearchState((s) => s.sqlInterface);
const setSqlInterface = usePrestoSearchState((s) => s.setSqlInterface);
const handleClick = () => {
if (sqlInterface !== PRESTO_SQL_INTERFACE.GUIDED) {
setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED);
}
};
return (
<Button
aria-pressed={sqlInterface === PRESTO_SQL_INTERFACE.GUIDED}
onClick={handleClick}
>
{/* ... */}
</Button>
);
};
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx
around lines 13 to 18, the button lacks an accessible pressed state and can call
the setter redundantly; read the current sql interface from usePrestoSearchState
in addition to the setter, compute an `isActive` boolean (sqlInterface ===
PRESTO_SQL_INTERFACE.GUIDED), pass `aria-pressed={isActive}` to the Button, and
in handleClick return early if isActive to avoid re-setting the same value; keep
the action of calling setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED) only when not
already active.


return (
<Button
block={true}
color={"default"}
icon={<AppstoreOutlined/>}
size={"middle"}
variant={"filled"}
onClick={handleClick}
>
Guided
</Button>
);
};

export default GuidedButton;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.sqlInterfaceButton {
width: 100px;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import usePrestoSearchState from "../../../SearchState/Presto";
import {PRESTO_SQL_INTERFACE} from "../../../SearchState/Presto/typings";
import FreeformButton from "./FreeformButton";
import GuidedButton from "./GuidedButton";
import styles from "./index.module.css";


/**
* Renders the button to switch Presto SQL interface.
*
* @return
*/
const SqlInterfaceButton = () => {
const sqlInterface = usePrestoSearchState((state) => state.sqlInterface);

return (
<div className={styles["sqlInterfaceButton"]}>
{sqlInterface === PRESTO_SQL_INTERFACE.GUIDED ?
<FreeformButton/> :
<GuidedButton/>}
</div>
);
};

export default SqlInterfaceButton;
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,19 @@
gap: 10px;
}

.buttonAndStatusRow {
.status {
margin-left: 2px;
}

.statusAndButtonsRow {
display: flex;
align-items: flex-start;
justify-content: space-between;
gap: 10px;
}

.status {
margin-left: 2px;
.buttons {
display: flex;
flex-direction: row;
gap: 5px;
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
} from "../../../config";
import Dataset from "./Dataset";
import styles from "./index.module.css";
import SqlInterfaceButton from "./Presto/SqlInterfaceButton";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Code-split the guided button

Avoid loading guided-mode UI when the flag is false by lazy-loading the component.

-import SqlInterfaceButton from "./Presto/SqlInterfaceButton";
+import { lazy, Suspense } from "react";

Outside the selected range, add the lazy binding near the imports:

const SqlInterfaceButton = lazy(() => import("./Presto/SqlInterfaceButton"));
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/SearchControls/index.tsx around
line 9, the guided-mode SqlInterfaceButton is always imported; change to
code-split it by replacing the static import with a lazy dynamic import and
render it inside a Suspense boundary only when the guided-mode flag is true. Add
the lazy binding near the other imports (e.g. const SqlInterfaceButton = lazy(()
=> import("./Presto/SqlInterfaceButton"))) and ensure you import React.lazy and
Suspense (or use an existing Suspense) and conditionally render <Suspense
fallback={...}><SqlInterfaceButton .../></Suspense> only when the feature flag
enables guided mode.

import SqlQueryInput from "./Presto/SqlQueryInput";
import SqlSearchButton from "./Presto/SqlSearchButton";
import QueryInput from "./QueryInput";
Expand All @@ -29,6 +30,10 @@ const handleSubmit = (ev: React.FormEvent<HTMLFormElement>) => {
* @return
*/
const SearchControls = () => {
/* eslint-disable-next-line no-warning-comments */
// TODO: Remove flag and related logic when the new guide UI is fully implemented.
const isGuidedEnabled = "true" === import.meta.env.VITE_GUIDED_DEV;

return (
<form onSubmit={handleSubmit}>
{SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO ?
Expand All @@ -48,11 +53,14 @@ const SearchControls = () => {
(
<div className={styles["searchControlsContainer"]}>
<SqlQueryInput/>
<div className={styles["buttonAndStatusRow"]}>
<div className={styles["statusAndButtonsRow"]}>
<div className={styles["status"]}>
<QueryStatus/>
</div>
<SqlSearchButton/>
<div className={styles["buttons"]}>
{isGuidedEnabled && <SqlInterfaceButton/>}
<SqlSearchButton/>
</div>
Comment on lines +60 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Wrap lazy component in Suspense

Pairing with the lazy import above.

-                                {isGuidedEnabled && <SqlInterfaceButton/>}
-                                <SqlSearchButton/>
+                                {isGuidedEnabled && (
+                                    <Suspense fallback={null}>
+                                        <SqlInterfaceButton/>
+                                    </Suspense>
+                                )}
+                                <SqlSearchButton/>
📝 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.

Suggested change
<div className={styles["buttons"]}>
{isGuidedEnabled && <SqlInterfaceButton/>}
<SqlSearchButton/>
</div>
<div className={styles["buttons"]}>
{isGuidedEnabled && (
<Suspense fallback={null}>
<SqlInterfaceButton/>
</Suspense>
)}
<SqlSearchButton/>
</div>
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/SearchControls/index.tsx around
lines 60-63, the lazily imported SqlInterfaceButton must be rendered inside a
React.Suspense boundary; wrap the conditional isGuidedEnabled &&
<SqlInterfaceButton/> with <Suspense fallback={/* small fallback e.g. null or a
spinner */}>...</Suspense>, ensure Suspense is imported from React (or use
React.Suspense), and keep SqlSearchButton outside or inside the same boundary as
desired so lazy loading is handled without throwing during render.

</div>
</div>
)}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import {create} from "zustand";

import {PRESTO_SQL_INTERFACE} from "./typings";


/**
* Default values of the Presto search state.
*/
const PRESTO_SEARCH_STATE_DEFAULT = Object.freeze({
sqlInterface: PRESTO_SQL_INTERFACE.FREEFORM,
});

interface PrestoSearchState {
/**
* Presto SQL interface.
*/
sqlInterface: PRESTO_SQL_INTERFACE;

setSqlInterface: (iface: PRESTO_SQL_INTERFACE) => void;
}

const usePrestoSearchState = create<PrestoSearchState>((set) => ({
...PRESTO_SEARCH_STATE_DEFAULT,
setSqlInterface: (iface) => {
set({sqlInterface: iface});
},
}));

export {PRESTO_SEARCH_STATE_DEFAULT};
export default usePrestoSearchState;
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Presto SQL interface types.
*/
export enum PRESTO_SQL_INTERFACE {
/**
* Guided interface with limited SQL dialect.
*/
GUIDED = "guided",

/**
* Freeform interface with SQL editor.
*/
FREEFORM = "freeform",
}
Loading