feat(webui): Add guided query mode button; Add a temporary env var to enable the guided query mode. #1276
Conversation
WalkthroughAdds a guided vs freeform Presto SQL interface toggle behind VITE_GUIDED_DEV, a PRESTO_SQL_INTERFACE enum, a Zustand store hook for sqlInterface, Guided/Freeform button components and wrapper, layout/CSS updates in SearchControls, and an npm script to launch guided mode. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SearchControls
participant SqlInterfaceButton
participant PrestoStore
participant UI
Note over SearchControls: Render when VITE_GUIDED_DEV === "true"
SearchControls->>SqlInterfaceButton: mount
SqlInterfaceButton->>PrestoStore: read sqlInterface
rect rgba(220,240,255,0.35)
Note right of User: Select "Guided"
User->>SqlInterfaceButton: click Guided
SqlInterfaceButton->>PrestoStore: setSqlInterface(GUIDED)
PrestoStore-->>UI: state { sqlInterface: GUIDED }
UI-->>SqlInterfaceButton: re-render (shows Freeform)
end
rect rgba(220,255,220,0.35)
Note right of User: Select "Freeform"
User->>SqlInterfaceButton: click Freeform
SqlInterfaceButton->>PrestoStore: setSqlInterface(FREEFORM)
PrestoStore-->>UI: state { sqlInterface: FREEFORM }
UI-->>SqlInterfaceButton: re-render (shows Guided)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
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/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx (1)
1-35: Show the other mode’s button, not the active one. Incomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/index.tsx, you currently renderGuidedButtonwhensqlInterface === PRESTO_SQL_INTERFACE.GUIDED(andFreeformButtonotherwise), so clicking the visible button does nothing. Swap the branches—or switch to an AntdSegmentedcontrol—so users see and can click the opposite mode’s button.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
components/webui/client/package.json(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/FreeformButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/Presto/typings.ts(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/SqlInterfaceButton/FreeformButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchState/Presto/typings.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx
🔇 Additional comments (4)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (2)
61-63: Guardrail: toggle may be non-functional due to inverted logic in SqlInterfaceButtonAs implemented, the switch renders the current mode’s button, which sets the state to the same value. This prevents switching. I’ve flagged and proposed a fix in
Presto/SqlInterfaceButton/index.tsx.After applying that fix, please click the toggle and confirm the interface actually flips modes.
56-64: CSS class definitions present –.statusAndButtonsRow(line 16) and.buttons(line 23) are defined inindex.module.css.components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/FreeformButton/index.tsx (1)
21-27: No changes required – your project uses AntD v5.24.5, which (since v5.21.0) supports the Buttonvariantandcolorprops. (github.com)components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx (1)
9-11: Sane defaultDefaulting to
FREEFORMlooks right and aligns with current UI expectations.
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/index.tsx
Outdated
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/Presto/typings.ts
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/index.tsx
Outdated
Show resolved
Hide resolved
|
How about this title: |
…esto/SqlInterfaceButton/index.tsx Co-authored-by: hoophalab <[email protected]>
davemarco
changed the title
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @hoophalab 's review. for the title, maybe this is more clear:
feat(webui): Add guided query mode button; Add a temporary env var to enable the query mode.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/client/package.json (1)
11-11: Make the guided script cross-platform and call Vite directly.Inline env vars break on Windows shells. Use cross-env and avoid the npm indirection.
"scripts": { "build": "tsc -b && vite build", "lint:check": "eslint", "lint:fix": "eslint --fix", "start": "vite", - "guided": "VITE_GUIDED_DEV=true npm run start" + "guided": "cross-env VITE_GUIDED_DEV=true vite" }, @@ "devDependencies": { + "cross-env": "^7.0.3", "@types/node": "22.15.31",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/webui/client/package.json(1 hunks)
⏰ 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). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
| "build": "tsc -b && vite build", | ||
| "lint:check": "eslint", | ||
| "lint:fix": "eslint --fix", | ||
| "start": "vite" | ||
| "start": "vite", | ||
| "guided": "VITE_GUIDED_DEV=true npm run start" |
There was a problem hiding this comment.
🧹 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.
| "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.
|
@junhaoliao - can you re-review this? There was a minor build error, so i needed to commit something |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
33-36: Feature flag: dot-access for tree-shaking + typed read + TODO with issue IDUse dot access so Vite can constant-fold and DCE the flagged branch; tighten typing; add an issue ID to the TODO.
- /* 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`]; + /* eslint-disable-next-line no-warning-comments */ + // TODO(GH-XXXX): Remove flag and related logic when the new guided UI is fully implemented. + const isGuidedEnabled = "true" === (import.meta.env.VITE_GUIDED_DEV as string | undefined);Optional (outside this file) to lock types:
// env.d.ts interface ImportMetaEnv { readonly VITE_GUIDED_DEV?: "true" | "false"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/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). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
56-64: No issues found: CSS classes and guided script verified
Both.statusAndButtonsRowand.buttonsexist inindex.module.css, and theguidedscript (VITE_GUIDED_DEV=true npm run start) is defined inpackage.json.
| } from "../../../config"; | ||
| import Dataset from "./Dataset"; | ||
| import styles from "./index.module.css"; | ||
| import SqlInterfaceButton from "./Presto/SqlInterfaceButton"; |
There was a problem hiding this comment.
🧹 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.
| <div className={styles["buttons"]}> | ||
| {isGuidedEnabled && <SqlInterfaceButton/>} | ||
| <SqlSearchButton/> | ||
| </div> |
There was a problem hiding this comment.
🧹 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.
| <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.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @hoophalab 's review
3 participants
Description
Adds a new npm script to start with guided mode option (phase 2). This allows the phase 1 webui to not offer guided mode until phase II is fully complete (in case we want to release phase 1). It also adds guided mode button so we can start phase 2.
can start phase II UI with
npm run guidedChecklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Style