feat(webui): Set up guided UI for query submission; Remove testing inputs.#1361
Conversation
WalkthroughRemoves a testing inputs component, expands Presto search state (select, from, where, orderBy, limit), converts multiple guided controls to controlled components bound to state, refactors Run button to select between freeform and guided modes and adds dedicated RunButton components, and tweaks TimeRange input CSS width. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RunButton as RunButton (index)
participant Store as usePrestoSearchState
participant Freeform as FreeformRunButton
participant Guided as GuidedRunButton
Note over RunButton,Store: Render time
User->>RunButton: Open Search Controls
RunButton->>Store: read sqlInterface
alt sqlInterface == GUIDED
RunButton-->>User: Render GuidedRunButton
else
RunButton-->>User: Render FreeformRunButton
end
sequenceDiagram
autonumber
actor User
participant Guided as GuidedRunButton
participant PStore as Presto Search State
participant UIStore as Search UI State (time, pending)
participant Builder as buildSearchQuery
participant Submit as handlePrestoQuerySubmit
User->>Guided: Click Run
Guided->>PStore: read select, from, where, orderBy, limit, timestampKey
Guided->>UIStore: read start/end, pending
alt Not ready or pending
Guided-->>User: Disabled / Tooltip
else
Guided->>Builder: build({selectItemList, databaseName, booleanExpression, sortItemList, limitValue, timestampKey, startTimestamp, endTimestamp})
Builder-->>Guided: queryString
Guided->>Submit: handlePrestoQuerySubmit(queryString)
Submit-->>Guided: result / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/BuildSqlTestingInputs.tsx(0 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/FreeformRunButton.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx(2 hunks)components/webui/client/src/sql-parser/index.ts(2 hunks)components/webui/client/src/sql-parser/typings.ts(1 hunks)components/webui/server/settings.json(1 hunks)tools/yscope-dev-utils(1 hunks)
💤 Files with no reviewable changes (1)
- components/webui/client/src/pages/SearchPage/SearchControls/Presto/BuildSqlTestingInputs.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/TimestampKey/index.tsxcomponents/webui/client/src/sql-parser/typings.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsxcomponents/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/FreeformRunButton.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsxcomponents/webui/client/src/sql-parser/index.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/index.tsx
🧬 Code graph analysis (2)
components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx (1)
components/webui/common/src/utility-types.ts (1)
Nullable(3-3)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (3)
components/webui/client/src/sql-parser/index.ts (1)
buildSearchQuery(193-193)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
handlePrestoQuerySubmit(112-112)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
🪛 GitHub Actions: clp-rust-checks
tools/yscope-dev-utils
[error] 1-1: cp: cannot stat '/home/runner/work/clp/clp/tools/yscope-dev-utils/exports/lint-configs/.rustfmt.toml': No such file or directory
⏰ 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: antlr-code-committed (macos-15)
🔇 Additional comments (18)
components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css (1)
15-15: LGTM!The reduced max-width tightens the layout of the range picker component, aligning with the broader UI refinements in this PR.
components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx (3)
11-17: LGTM!The default values for the new Presto search state fields are reasonable and well-chosen:
from: nullappropriately indicates no table selection yetlimit: 10provides a safe default to prevent unbounded queriesselect: "*"follows standard SQL convention- Empty strings for
orderByandwherecorrectly represent no ordering/filtering
21-54: LGTM!The interface additions are well-documented and type-safe. Field types align with the defaults, and the use of
Nullable<string>forfromcorrectly models optional table selection.
70-87: LGTM!The zustand store updater implementations are straightforward and consistent. The methods directly update state without validation, which is appropriate for a presentation-layer store. Input validation (e.g., ensuring
limit > 0) can be handled at the component or submission level if needed.components/webui/server/settings.json (1)
21-21: No issues found with “clp-s” engine configuration
“clp-s” is defined in the CLP_QUERY_ENGINES enum (components/webui/common/src/config.ts:6), included in build/taskfiles and CMake targets, and covered by unit and integration tests. The server’s search route handles it via generic job submission. Ensure the deployment environment includes the clp-s binary.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/index.tsx (1)
12-28: LGTM! Controlled input pattern correctly implemented.The component properly reads from and writes to the centralized Presto search state. The controlled input pattern with
valueandonChangeis correctly wired.components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
40-42: LGTM! Simplified render structure.The removal of
BuildSqlTestingInputsaligns with the PR objective to remove testing inputs. The simplified structure is cleaner and maintains the same functional behavior.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Limit.tsx (1)
21-38: LGTM! Controlled Select properly wired to state.The component correctly implements the controlled input pattern, binding the Select's
valueto state and wiringonChangeto the state updater. The antd Select component will handle edge cases appropriately.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx (1)
12-28: LGTM! Controlled SqlInput with proper null handling.The component correctly implements the controlled input pattern with appropriate fallback handling for falsy values (
value || ""), ensuring the state updater receives a valid string.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (1)
12-28: LGTM! Controlled SqlInput with consistent null handling.The component correctly implements the controlled input pattern, consistent with the Where component. The fallback to empty string (
value || "") ensures proper handling of falsy values.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx (1)
13-26: LGTM – controlled ORDER BY wiring looks solid.State read/write flows cleanly through the store and keeps SqlInput controlled.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (1)
13-27: LGTM – FROM selector now shares central state.DatasetSelect is properly controlled through the store updater; matches the new guided pattern.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/index.tsx (1)
13-17: LGTM – interface switch yields the right run button.The render split on PRESTO_SQL_INTERFACE keeps guided/freeform flows isolated cleanly.
components/webui/client/src/sql-parser/index.ts (2)
109-115: LGTM: Guard logic updated to match required field types.The changes correctly shift from checking
undefinedto checking empty strings for optional clauses, and LIMIT is now always appended using the required numericlimitValue. This aligns with the updatedBuildSearchQueryPropsinterface.
153-155: AlignbooleanExpressionguard with its optional type
Ripgrep found no internal calls tobuildTimelineQuery; please verify whetherbooleanExpressioncan ever beundefinedat runtime. If it can, update the guard to handle bothundefinedand""(for example,if (!booleanExpression) …), or tightenBuildTimelineQueryPropssobooleanExpressionis always a non-empty string.components/webui/client/src/sql-parser/typings.ts (1)
4-6: LGTM: Type signature updates enforce explicit values.The changes require callers to provide explicit values for
booleanExpression,sortItemList, andlimitValue(which is now numeric). Empty strings serve as sentinel values for omitted clauses, improving type safety over optional/undefined.Note: This is a breaking change for existing callers of
buildSearchQuery.components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/FreeformRunButton.tsx (2)
20-32: LGTM: Component logic is clean and correct.The button correctly disables when the query string is empty or a query is pending, and displays appropriate tooltip feedback. The callback properly submits the query string.
33-49: LGTM: Button rendering is correct.The JSX properly wires up the disabled state, tooltip, styling, and click handler. The button is appropriately disabled when the query is empty or pending.
| const handleClick = useCallback(() => { | ||
| if (null === from) { | ||
| console.error("Cannot build guided query: from input is missing"); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (null === timestampKey) { | ||
| console.error("Cannot build guided query: timestampKey input is missing"); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const queryString = buildSearchQuery({ | ||
| booleanExpression: where.trim(), | ||
| databaseName: from, | ||
| endTimestamp: endTimestamp.unix(), | ||
| limitValue: limit, | ||
| selectItemList: select.trim(), | ||
| sortItemList: orderBy.trim(), | ||
| startTimestamp: startTimestamp.unix(), | ||
| timestampKey: timestampKey, | ||
| }); | ||
|
|
||
| handlePrestoQuerySubmit({queryString}); | ||
| } catch (err) { | ||
| console.error("Failed to build guided query:", err); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider surfacing query build errors to the user.
The error handling logs failures to the console but doesn't notify the user. Consider displaying an error message or toast notification when query construction fails, so users understand why their query didn't run.
Example approach:
} catch (err) {
console.error("Failed to build guided query:", err);
+ // Consider adding user-facing error notification here
+ // e.g., message.error("Failed to build query. Please check your inputs.");
}📝 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 handleClick = useCallback(() => { | |
| if (null === from) { | |
| console.error("Cannot build guided query: from input is missing"); | |
| return; | |
| } | |
| if (null === timestampKey) { | |
| console.error("Cannot build guided query: timestampKey input is missing"); | |
| return; | |
| } | |
| try { | |
| const queryString = buildSearchQuery({ | |
| booleanExpression: where.trim(), | |
| databaseName: from, | |
| endTimestamp: endTimestamp.unix(), | |
| limitValue: limit, | |
| selectItemList: select.trim(), | |
| sortItemList: orderBy.trim(), | |
| startTimestamp: startTimestamp.unix(), | |
| timestampKey: timestampKey, | |
| }); | |
| handlePrestoQuerySubmit({queryString}); | |
| } catch (err) { | |
| console.error("Failed to build guided query:", err); | |
| } | |
| const handleClick = useCallback(() => { | |
| if (null === from) { | |
| console.error("Cannot build guided query: from input is missing"); | |
| return; | |
| } | |
| if (null === timestampKey) { | |
| console.error("Cannot build guided query: timestampKey input is missing"); | |
| return; | |
| } | |
| try { | |
| const queryString = buildSearchQuery({ | |
| booleanExpression: where.trim(), | |
| databaseName: from, | |
| endTimestamp: endTimestamp.unix(), | |
| limitValue: limit, | |
| selectItemList: select.trim(), | |
| sortItemList: orderBy.trim(), | |
| startTimestamp: startTimestamp.unix(), | |
| timestampKey: timestampKey, | |
| }); | |
| handlePrestoQuerySubmit({queryString}); | |
| } catch (err) { | |
| console.error("Failed to build guided query:", err); | |
| // Consider adding user-facing error notification here | |
| // e.g., message.error("Failed to build query. Please check your inputs."); | |
| } | |
| }, [from, timestampKey, where, select, orderBy, startTimestamp, endTimestamp, limit, handlePrestoQuerySubmit]); |
| disabled={!isQueryReady || | ||
| searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING} |
There was a problem hiding this comment.
Apply coding guideline: prefer false == over negation.
Line 86 uses !isQueryReady which violates the coding guideline preferring false == <expression> over !<expression>.
As per coding guidelines, apply this diff:
- disabled={!isQueryReady ||
+ disabled={false == isQueryReady ||
searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING}📝 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.
| disabled={!isQueryReady || | |
| searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING} | |
| disabled={false == isQueryReady || | |
| searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING} |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx
around lines 86-87, the disabled prop uses the negation `!isQueryReady`; replace
that with the coding-guideline preferred form `false == isQueryReady` so the
prop becomes disabled={false == isQueryReady || searchUiState ===
SEARCH_UI_STATE.QUERY_ID_PENDING}; keep the rest of the expression unchanged and
ensure spacing/parentheses match project style.
| updateFrom: (database: Nullable<string>) => void; | ||
| updateLimit: (value: number) => void; | ||
| updateOrderBy: (items: string) => void; | ||
| updateSelect: (items: string) => void; | ||
| updateTimestampKey: (key: Nullable<string>) => void; | ||
| updateWhere: (expression: string) => void; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
The updater method signatures follow consistent conventions and type-safe patterns. Minor observation: the parameter name database in updateFrom might be slightly misleading since SQL FROM clauses typically reference table names, but this is a pedantic detail.
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx
around lines 57 to 62 the updateFrom parameter is named `database`, which is
misleading for an SQL FROM clause; rename the parameter to `table` or
`tableName` (and update its Nullable<string> type if needed), update the
function signature and all internal references and call sites across the
codebase to use the new name, and run type checks/compile to ensure no broken
references remain.
| @@ -1 +1 @@ | |||
| Subproject commit f1202b9c21dac05d77632e4d3e1768b11e0dcf47 | |||
| Subproject commit b965f19f3e11c548f32bf7ab8e28ff38906be018 | |||
There was a problem hiding this comment.
Restore exported lint config path
Updating the submodule pointer drops tools/yscope-dev-utils/exports/lint-configs/.rustfmt.toml, which the clp-rust-checks Action copies during formatting. The CI job now fails (cp: cannot stat .../.rustfmt.toml). Please either restore the missing file in the new submodule commit or revert to a commit that still provides it so the formatter can run again.
🧰 Tools
🪛 GitHub Actions: clp-rust-checks
[error] 1-1: cp: cannot stat '/home/runner/work/clp/clp/tools/yscope-dev-utils/exports/lint-configs/.rustfmt.toml': No such file or directory
Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (2)
29-32: EnsurefromandtimestampKeyare non-empty strings.The readiness check only guards against
nullbut permits empty strings. IffromortimestampKeyare empty strings, the query will fail at build time or produce invalid SQL.Apply this diff to validate non-empty strings:
const isQueryReady = "" !== select.trim() && - null !== from && - null !== timestampKey; + null !== from && "" !== from.trim() && + null !== timestampKey && "" !== timestampKey.trim();
88-89: Apply coding guideline: preferfalse ==over negation.Line 88 uses
!isQueryReady, which violates the coding guideline preferringfalse == <expression>over!<expression>.As per coding guidelines.
Apply this diff:
- disabled={!isQueryReady || + disabled={false === isQueryReady || searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx(1 hunks)components/webui/client/src/sql-parser/index.ts(2 hunks)components/webui/client/src/sql-parser/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/sql-parser/index.tscomponents/webui/client/src/sql-parser/typings.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-17T22:51:15.765Z
Learnt from: hoophalab
PR: y-scope/clp#1304
File: components/webui/client/src/sql-parser/index.ts:98-115
Timestamp: 2025-09-17T22:51:15.765Z
Learning: In the CLP webui codebase, when designing APIs that accept optional string parameters, developers should omit the value (pass undefined) rather than pass empty strings. The API contract should be clear that empty strings are not valid inputs - only undefined should be used to indicate omission of optional clauses.
Applied to files:
components/webui/client/src/sql-parser/typings.ts
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (3)
components/webui/client/src/sql-parser/index.ts (1)
buildSearchQuery(193-193)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
handlePrestoQuerySubmit(112-112)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
🔇 Additional comments (4)
components/webui/client/src/sql-parser/index.ts (2)
109-114: LGTM: Undefined checks correctly handle optional parameters.The guards now properly check for
undefinedrather than relying on implicit falsy checks, which aligns with the API contract that optional parameters should be undefined rather than empty strings.Based on learnings.
115-115: LGTM: LIMIT clause is now always appended.The LIMIT clause is consistently included in all search queries, which aligns with the change making
limitValuea required parameter in the typings.components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (1)
53-63: Well-structured query building with proper optional parameter handling.The
getUndefinedIfEmptyhelper correctly converts empty strings toundefinedfor optional SQL clauses, aligning with the API contract that optional parameters should beundefinedrather than empty strings. The conversion oflimitto a string (line 58) properly handles the requiredlimitValueparameter.Based on learnings.
components/webui/client/src/sql-parser/typings.ts (1)
6-6: No issues with makinglimitValuerequired. The only call tobuildSearchQueryis in GuidedRunButton.tsx (lines 54–63), and it always supplies a string forlimitValue, so requiring it poses no breakages.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (3)
29-32: Verify thatfromandtimestampKeycannot be empty strings.The readiness check validates that
fromandtimestampKeyare notnull, but doesn't verify they are non-empty strings. If these can be empty strings, the query will fail at build time. Consider adding trim checks:from?.trim() !== ""andtimestampKey?.trim() !== "".
68-70: Consider surfacing query build errors to the user.The error handling logs failures to the console but doesn't notify the user. Consider displaying an error message or toast notification when query construction fails.
90-91: Apply coding guideline: preferfalse ==over negation.Line 90 uses
!isQueryReadywhich violates the coding guideline preferringfalse == <expression>over!<expression>.As per coding guidelines, apply this diff:
- disabled={!isQueryReady || + disabled={false == isQueryReady || searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.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/SqlSearchButton/RunButton/GuidedRunButton.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). (5)
- GitHub Check: package-image
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: lint-check (macos-15)
| return ( | ||
| <Tooltip title={tooltipTitle}> | ||
| <Button | ||
| className={styles["runButton"] || ""} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify className assignment.
The || "" fallback is unnecessary since className accepts undefined.
Apply this diff:
- className={styles["runButton"] || ""}
+ className={styles["runButton"]}📝 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["runButton"] || ""} | |
| className={styles["runButton"]} |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx
around line 85, the className is assigned with an unnecessary fallback using `||
""`; remove the fallback so className is set directly to styles["runButton"]
(allowing undefined) to simplify the code.
|
@hoophalab rereview |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (4)
29-32: Empty string validation still needed forfromandtimestampKey.The readiness check validates
selectis non-empty but only checks thatfromandtimestampKeyare notnull. Empty strings would pass this check but cause query build failures. Consider updating to also verify these fields are non-empty trimmed strings.Apply this diff to strengthen the validation:
- const isQueryReady = - "" !== select.trim() && - null !== from && - null !== timestampKey; + const isQueryReady = + "" !== select.trim() && + null !== from && + "" !== from.trim() && + null !== timestampKey && + "" !== timestampKey.trim();
68-70: Consider surfacing query build errors to the user.Query construction failures are logged to the console but not shown to users. Consider displaying an error notification so users understand why their query didn't run.
85-85: Simplify className assignment.The
|| ""fallback is unnecessary sinceclassNameacceptsundefined.Apply this diff:
- className={styles["runButton"] || ""} + className={styles["runButton"]}
90-91: Apply coding guideline: preferfalse ==over negation.The disabled prop uses
!isQueryReadywhich violates the coding guideline preferringfalse == <expression>over!<expression>.As per coding guidelines, apply this diff:
- disabled={!isQueryReady || + disabled={false == isQueryReady || searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.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/SqlSearchButton/RunButton/GuidedRunButton.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (3)
components/webui/client/src/sql-parser/index.ts (1)
buildSearchQuery(195-195)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
handlePrestoQuerySubmit(112-112)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
⏰ 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). (6)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
🔇 Additional comments (3)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (3)
1-14: LGTM!Import statements are clean and all dependencies are utilized appropriately in the component.
34-36: LGTM!Tooltip logic correctly follows the coding guideline by using
false === isQueryReadyinstead of negation.
100-100: LGTM!Component export is correct.
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @hoophalab 's review
Description
Connects to new UI components to zustand states, and sets up the new components and run button to submit injected queries to the server.
Does not include timeline support
Checklist
breaking change.
Validation performed
Queries run using new guided ui
Summary by CodeRabbit
New Features
Style
Chores