Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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