feat(webui): Reset states when switching between freeform and guided.#1482
Conversation
WalkthroughCentralizes Presto SQL interface switching into exported handler functions and updates button click handlers to call them; adds an early guard to the metadata update effect to skip processing when search UI state is DEFAULT. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GuidedBtn as GuidedButton
participant FreeformBtn as FreeformButton
participant Handlers as Presto Handlers
participant Store as Search Store
participant Effect as useUpdateStateWithMetadata
User->>GuidedBtn: Click "Guided"
GuidedBtn->>Handlers: handleSwitchToGuided()
Handlers->>Store: clear results if needed
Handlers->>Store: reset searchJobId & result counters to SEARCH_STATE_DEFAULT
Handlers->>Store: set search UI state = DEFAULT
Handlers->>Store: set PRESTO_SQL_INTERFACE = GUIDED
activate Effect
Effect->>Effect: is searchUiState == DEFAULT?
Effect-->>Effect: early return (skip metadata)
deactivate Effect
User->>FreeformBtn: Click "Freeform"
FreeformBtn->>Handlers: handleSwitchToFreeform()
Handlers->>Store: clear guided results
Handlers->>Store: reset Presto/search fields to PRESTO_SEARCH_STATE_DEFAULT
Handlers->>Store: update error/cached query and query drawer state
Handlers->>Store: set PRESTO_SQL_INTERFACE = FREEFORM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential review focal points:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
|
||
| const handleClick = () => { | ||
| handleSwitchToGuided(); | ||
| setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED); |
There was a problem hiding this comment.
do we want to move setSqlInterface into handleSwitchToGuided?
| setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED); |
|
|
||
| const handleClick = () => { | ||
| handleSwitchToFreeform(); | ||
| setSqlInterface(PRESTO_SQL_INTERFACE.FREEFORM); |
There was a problem hiding this comment.
| setSqlInterface(PRESTO_SQL_INTERFACE.FREEFORM); |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/FreeformButton/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(2 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/SearchControls/Presto/presto-search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
handleSwitchToGuided(143-143)
⏰ 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: lint-check (macos-15)
🔇 Additional comments (6)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/GuidedButton/index.tsx (2)
6-6: Good refactoring to centralized handler.The import of
handleSwitchToGuidedproperly delegates state management to a centralized location, improving maintainability.
19-21: LGTM - Clean delegation pattern.The click handler correctly delegates to the centralized
handleSwitchToGuided()function, addressing the past review comment about movingsetSqlInterfaceinto the handler.components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlInterfaceButton/FreeformButton/index.tsx (1)
6-6: Excellent consistency with GuidedButton.The refactoring mirrors the approach in GuidedButton, providing a consistent pattern for both interface switches and improving code maintainability.
Also applies to: 19-21
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
12-13: LGTM - Necessary imports.The imports support the new
handleSwitchToGuidedfunction correctly.components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (2)
14-15: LGTM - Necessary imports.The imports properly support the new
handleSwitchToFreeformfunction.
216-255: Original review comment does not correspond to actual changes in this PR.The git diff shows no modifications to
presto-guided-search-requests.ts—the code snippet provided is the current HEAD state unchanged. The original review references "critical fixes above" and asks verification "after applying the critical fixes," but no such fixes exist in this file within the PR. The current code structure (DEFAULT state guard, state transition sequence, effect dependencies) appears logically sound, but the review comment's premise is unfounded.Likely an incorrect or invalid review comment.
| const handleSwitchToGuided = () => { | ||
| const { | ||
| searchUiState, | ||
| updateSearchUiState, | ||
| updateSearchJobId, | ||
| updateNumSearchResultsTable, | ||
| updateNumSearchResultsMetadata, | ||
| } = useSearchStore.getState(); | ||
| const {setSqlInterface} = usePrestoSearchState.getState(); | ||
|
|
||
| if (searchUiState === SEARCH_UI_STATE.DEFAULT) { | ||
| return; | ||
| } | ||
|
|
||
| handlePrestoClearResults(); | ||
|
|
||
| updateSearchJobId(SEARCH_STATE_DEFAULT.searchJobId); | ||
| updateNumSearchResultsTable(SEARCH_STATE_DEFAULT.numSearchResultsTable); | ||
| updateNumSearchResultsMetadata(SEARCH_STATE_DEFAULT.numSearchResultsMetadata); | ||
|
|
||
| updateSearchUiState(SEARCH_UI_STATE.DEFAULT); | ||
|
|
||
| setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED); | ||
| }; |
There was a problem hiding this comment.
Critical: Early return prevents interface switch in DEFAULT state.
The early return on Line 125 prevents setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED) (Line 137) from executing when searchUiState === DEFAULT. Since the Guided button is enabled in DEFAULT state, users expect clicking it to switch the interface. Currently, clicking the button when in DEFAULT state has no effect.
Additionally, handlePrestoClearResults() already guards against clearing in DEFAULT state (Line 24), making the early return redundant for its intended purpose.
Apply this diff to fix the logic:
const handleSwitchToGuided = () => {
const {
searchUiState,
updateSearchUiState,
updateSearchJobId,
updateNumSearchResultsTable,
updateNumSearchResultsMetadata,
} = useSearchStore.getState();
const {setSqlInterface} = usePrestoSearchState.getState();
- if (searchUiState === SEARCH_UI_STATE.DEFAULT) {
- return;
- }
-
- handlePrestoClearResults();
-
- updateSearchJobId(SEARCH_STATE_DEFAULT.searchJobId);
- updateNumSearchResultsTable(SEARCH_STATE_DEFAULT.numSearchResultsTable);
- updateNumSearchResultsMetadata(SEARCH_STATE_DEFAULT.numSearchResultsMetadata);
-
- updateSearchUiState(SEARCH_UI_STATE.DEFAULT);
+ // Only clear and reset if there's an active search
+ if (searchUiState !== SEARCH_UI_STATE.DEFAULT) {
+ handlePrestoClearResults();
+ updateSearchJobId(SEARCH_STATE_DEFAULT.searchJobId);
+ updateNumSearchResultsTable(SEARCH_STATE_DEFAULT.numSearchResultsTable);
+ updateNumSearchResultsMetadata(SEARCH_STATE_DEFAULT.numSearchResultsMetadata);
+ updateSearchUiState(SEARCH_UI_STATE.DEFAULT);
+ }
setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED);
};🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
around lines 115 to 138, the early return when searchUiState ===
SEARCH_UI_STATE.DEFAULT prevents setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED)
from running, so clicking Guided in DEFAULT does nothing; remove or relocate
that early return so the function always executes the state-reset and calls
setSqlInterface(PRESTO_SQL_INTERFACE.GUIDED) for DEFAULT, relying on
handlePrestoClearResults() (which already guards against clearing in DEFAULT) to
avoid any unwanted clearing.
|
these two coderabbit comments are probably valid? |
|
@hoophalab see latest |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(2 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/presto-search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts
🧬 Code graph analysis (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (2)
components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (3)
components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)components/webui/client/src/pages/SearchPage/SearchState/Presto/index.tsx (1)
PRESTO_SEARCH_STATE_DEFAULT(110-110)
⏰ 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: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
112-138: The critical issue has been resolved.The interface switch now executes before the early return, ensuring that clicking the Guided button in DEFAULT state correctly switches the interface. The logic is sound: the interface switch happens unconditionally, while result clearing and state resetting only occur when there's an active search to clean up.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (1)
213-255: The critical issue has been resolved.The interface switch now executes before the early return, correctly handling the switch to freeform mode even when in DEFAULT state. The conditional clearing and resetting logic is appropriate—it only performs cleanup when transitioning away from an active search.
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @hoophalab 's review
Description
Resets states and cancels queries when switching modes.
Checklist
breaking change.
Validation performed
I tested before, but I had to make a significant change due to the linter failing after merging main. I had to add another effect dependency that could brake things. I can no longer test since it lost my old package build, and can no longer build package on my machine due to package build changes. Can you confirm the that it still properly transitions between states?
If not we might have to remove searchUiState from the effect, and suppress the lint
Summary by CodeRabbit