feat(webui): Display notification for errors from Presto. #1188
Conversation
WalkthroughAdds an explicit FAILED UI state and PRESTO_SEARCH_SIGNAL enum value; records Presto errorName/errorMsg in server metadata; client shows notifications and danger styling for FAILED; allows resubmission after failure and treats FAILED like DONE for progress cleanup and input focus. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Client (Web UI)
participant S as Server (Presto/Search APIs)
participant DB as Metadata Store
U->>C: Submit search
C->>S: POST /search or /presto-search
S->>DB: Insert metadata (lastSignal, errorName=null)
S-->>C: Ack
alt Presto progress
loop Signals
S->>DB: Update lastSignal (PRESTO_SEARCH_SIGNAL enum)
end
end
alt Success
S->>DB: Update lastSignal=FINISHED/RESP_DONE
C->>C: set UI state DONE
else Failure
S->>DB: Update errorMsg/errorName, lastSignal=FAILED
C->>C: set UI state FAILED
C->>U: Show error notification
U->>C: Retry submit
C->>S: New request (allowed when UI state is DEFAULT/DONE/FAILED)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchQueryStatus/Results.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/typings.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts(2 hunks)components/webui/common/index.ts(2 hunks)components/webui/server/src/routes/api/presto-search/index.ts(4 hunks)components/webui/server/src/routes/api/search/index.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/QueryInput/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchQueryStatus/Results.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchState/typings.tscomponents/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.tscomponents/webui/server/src/routes/api/search/index.tscomponents/webui/common/index.tscomponents/webui/server/src/routes/api/presto-search/index.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
🧬 Code Graph Analysis (2)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1)
components/webui/common/index.ts (1)
SEARCH_SIGNAL(135-135)
components/webui/server/src/routes/api/search/index.ts (1)
components/webui/common/index.ts (1)
SEARCH_SIGNAL(135-135)
🔇 Additional comments (15)
components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
25-28: All SEARCH_UI_STATE.FAILED cases are accounted for
A project-wide scan found no switch statements or guards missing theFAILEDcase. All existing references toSEARCH_UI_STATE.FAILEDare handled appropriately—no further changes needed.components/webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx (2)
60-63: Including FAILED in progress cleanup is correctEnsures the pseudo-progress is cleared after failures, avoiding a stuck progress bar.
78-80: Focusing input on FAILED (in addition to DEFAULT/DONE) is a good UX touchThis allows immediate resubmission after an error, consistent with the new terminal state semantics.
components/webui/client/src/pages/SearchPage/SearchQueryStatus/Results.tsx (1)
49-51: FAILED → danger mapping aligns with the intended visual signalRenders failures in red, matching the new error state semantics in the UI.
components/webui/server/src/routes/api/search/index.ts (2)
118-119: Adding errorName: null at insert is consistent with the updated metadata schemaThis keeps documents shape-stable across engines and allows later population by engine-specific flows.
211-213: Cancellation update preserves RESP_DONE semanticsSetting errorMsg and lastSignal: RESP_DONE on cancel matches existing behaviour; no issues.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
21-24: Guard now correctly allows resubmission after failureAllowing submissions when the UI state is DEFAULT, DONE, or FAILED matches the new UX. Looks good.
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (3)
29-39: Failure handling and notification are wired correctlySetting UI state to FAILED and surfacing antd notification with errorName and errorMsg is clear and user-friendly. The keyed notification prevents duplicates if metadata updates bounce.
3-7: Importing PRESTO_SEARCH_SIGNAL and notification is appropriateThe hook now has the right dependencies to react to Presto-specific failures.
31-39: No action needed:showProgressis supported in antd v5.24.5The project is using antd version
^5.24.5, and theshowProgressproperty was introduced in v5.18.0. You can safely keep theshowProgress: trueconfig without any type errors or no-ops.components/webui/server/src/routes/api/presto-search/index.ts (2)
139-145: First-time insert initialises new fields correctlyInitialising errorName to null and using the typed Presto state for lastSignal aligns with the new client expectations.
153-155: Subsequent metadata updates via typed lastSignal look goodUpdating lastSignal with the validated enum preserves type integrity and simplifies client handling.
components/webui/common/index.ts (3)
94-106: Switch to a real enum for PRESTO signals is a solid choiceHaving PRESTO_SEARCH_SIGNAL as a value enables consistent imports across client/server and improves type safety.
127-127: Adding errorName to SearchResultsMetadataDocument matches server/client usageGood schema evolution. This enables more informative notifications without overloading errorMsg.
133-136: Public exports are coherent with the enum changeExporting PRESTO_SEARCH_SIGNAL as a value and keeping types cleanly separated is correct.
components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1)
16-17: Docstring now aligns with enum (FAILED) — resolvedThanks for updating the comment to use “FAILED”; this addresses the earlier review concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.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/SearchState/useUpdateStateWithMetadata.ts
🧬 Code Graph Analysis (1)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (2)
components/webui/common/index.ts (2)
SEARCH_SIGNAL(135-135)PRESTO_SEARCH_SIGNAL(134-134)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). (2)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (2)
23-23: LGTM on early-return null checkThe null guard prevents unnecessary state updates before metadata is available.
31-39: AntD v5.24.5 supportspauseOnHover&showProgress– no changes required
We’re on Ant Design ^5.24.5 (per components/webui/client/package.json), which introduced bothpauseOnHoverandshowProgressin the Notification API. TypeScript types include these props and no other usage exists in the repo, so no fallback or feature-flagging is needed.
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (2)
71-73: Prevent UI from getting stuck on QUERY_ID_PENDING and surface the error to usersIf submitQuery rejects (e.g., no queryId ever produced), the UI remains at QUERY_ID_PENDING and users see no notification. Set UI to FAILED and emit a notification in the catch handler.
Apply this diff:
- .catch((err: unknown) => { - console.error("Failed to submit query:", err); - }); + .catch((err: unknown) => { + updateSearchUiState(SEARCH_UI_STATE.FAILED); + console.error("Failed to submit query:", err); + const e: any = err as any; + const errorName: string | null = + ("string" === typeof e?.errorName) + ? e.errorName + : (("string" === typeof e?.name) ? e.name : null); + const errorMsg: string = + ("string" === typeof e?.errorMsg) + ? e.errorMsg + : (e?.message ? String(e.message) : "Presto query failed"); + notification.error({ + message: errorName ?? "Presto query failed", + description: errorMsg, + }); + });Add the import (outside the selected range):
// at top-level imports import {notification} from "antd";
23-27: Avoid noisy error logs when there is no prior job to clearWhen a previous submission failed before a jobId existed, clearing results is a no-op. Downgrade the log level or silently return to reduce noise.
Apply this diff:
- if (null === searchJobId) { - console.error("Cannot clear results: searchJobId is not set."); - - return; - } + if (null === searchJobId) { + console.debug("No results to clear: searchJobId is not set."); + return; + }components/webui/server/src/routes/api/presto-search/index.ts (1)
166-169: Return a structured 500 with errorName/errorMsg when failure occurs before stateCurrently the handler rethrows, producing a generic 500. Return a body matching ErrorSchema so the client can display a meaningful notification even when no metadata doc exists.
Apply this diff:
- } catch (error) { - request.log.error(error, "Failed to submit Presto query"); - throw error; - } + } catch (error) { + request.log.error(error, "Failed to submit Presto query"); + const e: any = error as any; + const errorName: string | null = + ("string" === typeof e?.errorName) + ? e.errorName + : (("string" === typeof e?.name) ? e.name : null); + const errorMsg: string = + ("string" === typeof e?.errorMsg) + ? e.errorMsg + : (e?.message ? String(e.message) : "Presto search failed"); + reply.code(StatusCodes.INTERNAL_SERVER_ERROR); + return reply.send({errorName, errorMsg}); + }
♻️ Duplicate comments (2)
components/webui/server/src/routes/api/presto-search/index.ts (2)
130-135: Validate incoming state before casting to enumDefensively guard against unexpected strings to avoid persisting invalid states.
Apply this diff:
- // Type cast `presto-client` string literal type to our enum type. - const newState = stats.state as PRESTO_SEARCH_SIGNAL; + // Validate then cast `presto-client` state to our enum type. + if (false === Object.values(PRESTO_SEARCH_SIGNAL) + .includes(stats.state as PRESTO_SEARCH_SIGNAL)) { + request.log.warn({state: stats.state}, "Unknown Presto state; skipping metadata update"); + return; + } + const newState = stats.state as PRESTO_SEARCH_SIGNAL;
109-120: Upsert error metadata and derive a robust errorName fallbackWithout upsert, failures that occur before the first state insert will not persist error metadata; clients won’t be notified. Also prefer engine-provided errorName, then standard Error.name.
Apply this diff:
- searchResultsMetadataCollection.updateOne( - {_id: searchJobId}, - { - $set: { - errorMsg: error.message, - errorName: ("errorName" in error) ? - error.errorName : - null, - lastSignal: PRESTO_SEARCH_SIGNAL.FAILED, - }, - } - ).catch((err: unknown) => { + searchResultsMetadataCollection.updateOne( + {_id: searchJobId}, + { + $set: { + errorMsg: error.message, + errorName: + ("string" === typeof (error as any)?.errorName) + ? (error as any).errorName + : (("string" === typeof (error as any)?.name) + ? (error as any).name + : null), + lastSignal: PRESTO_SEARCH_SIGNAL.FAILED, + }, + }, + {upsert: true} + ).catch((err: unknown) => {
📜 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-search-requests.ts(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(4 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/server/src/routes/api/presto-search/index.ts
🧬 Code Graph Analysis (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
components/webui/server/src/routes/api/presto-search/index.ts (1)
components/webui/common/index.ts (1)
PRESTO_SEARCH_SIGNAL(134-134)
⏰ 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: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
46-50: Allowing resubmission from FAILED is correctTreating FAILED as a terminal (non-blocking) state aligns with the UX intent and keeps behaviour consistent with DONE and DEFAULT.
4 participants
Description
PR add error info to results metadata in mongo, so we can display presto errors to users on the front end. Error shows up as antd noticification. I expect most common error will be syntax error.
It also adds a new
failedstate to front end, so we can display red instead of green for num search results. I thought it looked weird for it to be green when there was an error. failed state is only used for presto at the moment.Checklist
breaking change.
Validation performed
Tested popup appeared on error
Summary by CodeRabbit
New Features
Bug Fixes
Chores