feat(webui): Disable guided inputs while query running; Replace redundant "from" state with "dataset" state from native UI.#1465
Conversation
WalkthroughMigrates the Presto Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Guided Controls UI
participant PrestoState as PrestoSearchState (zustand)
participant GlobalStore as SearchStore (useSearchStore)
Note over UI,PrestoState: Old flow (before changes)
UI->>PrestoState: read `from`, `select`, `where`, `orderBy`, `timestampKey`
PrestoState-->>UI: returns values
UI->>UI: controlled inputs bind to Presto state
UI->>PrestoState: update handlers call setters (e.g., updateFrom)
Note over UI,GlobalStore: New flow (after changes)
UI->>PrestoState: read `select`, `where`, `orderBy`, `timestampKey`
UI->>GlobalStore: read `selectDataset` (as `from`) via getState / hooks
UI-->>UI: DatasetSelect & TimestampKeySelect rendered uncontrolled (no state binding)
GlobalStore-->>UI: provides `searchUiState` for disabling controls when QUERY_ID_PENDING/QUERYING
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Heterogeneous edits: state-shape change, multiple components updated to use a different store, and UI disabling logic applied consistently; requires checking state migrations, call-sites, and null-handling. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
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: 1
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/SqlSearchButton/RunButton/GuidedRunButton.tsx (1)
96-97: Appliquer la correction de guideline et corriger l'incohérence d'état désactivé.Deux problèmes confirmés :
Violation de guideline confirmée : La ligne 96 utilise
!isQueryReadyau lieu du format requisfalse == isQueryReady.Incohérence confirmée : Tous les autres contrôles guidés (Select, Where, OrderBy, FreeformButton, GuidedButton) désactivent pendant les deux états :
QUERY_ID_PENDINGetQUERYING. GuidedRunButton ne désactive que pendantQUERY_ID_PENDING, créant une incohérence comportementale.Appliquer les corrections :
- disabled={!isQueryReady || - searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING} + disabled={false === isQueryReady || + searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || + searchUiState === SEARCH_UI_STATE.QUERYING}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx(1 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(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/FreeformButton/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- components/webui/client/src/pages/SearchPage/SearchState/Presto/index.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/SqlSearchButton/RunButton/GuidedRunButton.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/FreeformButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/index.tsx
🧬 Code graph analysis (3)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx (1)
components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (1)
components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/FreeformButton/index.tsx (1)
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). (1)
- GitHub Check: conventional-commits
🔇 Additional comments (8)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Where.tsx (1)
17-26: LGTM! Disabled state correctly applied.The WHERE input now properly respects the query execution state, preventing user input during pending or active queries.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx (1)
17-29: LGTM! Guided button correctly disabled during query execution.The implementation follows the same pattern as FreeformButton and properly prevents interface switching while a query is running.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/OrderBy.tsx (1)
17-26: LGTM! ORDER BY input correctly gated by query state.The implementation is consistent with other guided controls and properly disables the input during query execution.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/Select.tsx (1)
17-26: LGTM! SELECT input correctly disabled during queries.Consistent implementation with other guided controls.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/From.tsx (1)
11-22: No action required—DatasetSelect properly synchronizes with the global store.The verification confirms that DatasetSelect manages its own state through
useSearchStore:
- It retrieves the current value via
useSearchStore((state) => state.selectDataset)- It updates the store via the
handleDatasetChangefunction, which callsupdateDataset(value)- The internal
Selectcomponent is controlled with bothvalue={dataset}andonChange={handleDatasetChange}From.tsx correctly delegates state management to DatasetSelect by passing only the
classNameprop. When a user selects a dataset, the component immediately synchronizes the selection to the global store, making it available for GuidedRunButton.tsx to read viauseSearchStore.getState().selectDataset.components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/index.tsx (1)
11-22: Review comment is incorrect.TimestampKeySelect already properly manages and syncs with Presto state internally—it reads
timestampKeyandupdateTimestampKeyfromusePrestoSearchStateand implements ahandleTimestampKeyChangehandler that updates state on user selection. The component is controlled internally, not uncontrolled. Rendering it without explicitvalue/onChangeprops in the parent is a valid pattern; no changes needed.Likely an incorrect or invalid review comment.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (1)
37-37: Based on my verification of the codebase, I can confirm thatDatasetSelectis properly synchronizing its selection touseSearchStore.selectDataset. The component:
- Reads the current value from the store (line 22 of DatasetSelect.tsx)
- Obtains the
updateSelectDatasetfunction from the store (line 23)- Updates the store when the user changes the selection through the
handleDatasetChangecallback (line 68)- Maintains a controlled Select component with both
valueandonChangehandlers connected to the storeThe GuidedRunButton can safely access
selectDatasetviauseSearchStore.getState()at line 37, as the store is kept in sync whenever the DatasetSelect component changes. No issues detected.components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (1)
69-70: Verification confirms null handling is correct; no changes required.The
selectDatasetfield is typed asstring | null(line 85 of SearchState/index.tsx), with a default value ofnull(line 28). The strict equality checknull === fromat line 72 properly handles all possible values. Since the type does not includeundefined, the current implementation is type-safe and follows TypeScript best practices. The state migration is complete and correct.
| const searchUiState = useSearchStore((state) => state.searchUiState); | ||
| const disabled = searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING || | ||
| searchUiState === SEARCH_UI_STATE.QUERYING; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the disabled state computation into a custom hook.
The same disabled state logic appears identically in at least 5 files (FreeformButton, GuidedButton, Where.tsx, OrderBy.tsx, Select.tsx). This code duplication could be eliminated by creating a custom hook like useIsQueryRunning() or useGuidedControlsDisabled().
Example refactor:
Create a new hook file useGuidedControlsDisabled.ts:
import useSearchStore from "../../SearchState/index";
import {SEARCH_UI_STATE} from "../../SearchState/typings";
/**
* Returns whether guided controls should be disabled based on query execution state.
*/
const useGuidedControlsDisabled = (): boolean => {
const searchUiState = useSearchStore((state) => state.searchUiState);
return searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING ||
searchUiState === SEARCH_UI_STATE.QUERYING;
};
export default useGuidedControlsDisabled;Then in each component:
- const searchUiState = useSearchStore((state) => state.searchUiState);
- const disabled = searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING ||
- searchUiState === SEARCH_UI_STATE.QUERYING;
+ const disabled = useGuidedControlsDisabled();
hoophalab
left a comment
There was a problem hiding this comment.
looks good to me.
Validations: the inputs are disabled before query completes
|
We probably need to fix the dataset name overflow issue before the release. This feature is required by a user. Since we are already modifying guided inputs in this PR, how about patching it quickly? The following change fixes the issue -- any better ideas? +++ b/components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/index.module.css
@@ -31,7 +31,8 @@
}
.widthSelect {
- width: 100%;
+ width: 0;
+ flex-grow: 1;
} |
|
thanks - I made ur change |
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @hoophalab 's review
…dant "from" state with "dataset" state from native UI. (y-scope#1465)
Description
Sets disabled states while querying for guided components. Remove from state and use dataset state instead, since the from is not neccesary.
Checklist
breaking change.
Validation performed
Components become disabled
Summary by CodeRabbit
New Features
Refactor