Repository navigation
fix(dashboard): surface real analytics error instead of generic placeholder (#3356) #3361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * Extract a human-readable message from a failed `fetch` Response body. | ||
| * | ||
| * Handles both response shapes OmniRoute routes emit: | ||
| * - OpenAI-style `{ error: { message, type, code } }` (from `buildErrorBody`) | ||
| * - legacy `{ error: "..." }` string bodies | ||
| * | ||
| * The server already sanitizes these messages (stack traces / absolute paths | ||
| * stripped via `sanitizeErrorMessage`), so surfacing them in the UI is safe. | ||
| * Falls back to `fallback` when the body is absent, unparseable, or carries no | ||
| * usable message. Never throws -- safe to call directly inside a fetch guard. | ||
| */ | ||
| export async function readFetchErrorMessage(res: Response, fallback: string): Promise<string> { | ||
| try { | ||
| const body = (await res.json()) as unknown; | ||
| const err = (body as { error?: unknown } | null)?.error; | ||
| if (typeof err === "string" && err.trim()) return err.trim(); | ||
| if (err && typeof err === "object") { | ||
| const message = (err as { message?: unknown }).message; | ||
| if (typeof message === "string" && message.trim()) return message.trim(); | ||
| } | ||
| } catch { | ||
| // Non-JSON body (e.g. an HTML 500 page) or a read failure -> use the fallback. | ||
| } | ||
| return fallback; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
|
|
||
| import { readFetchErrorMessage } from "../../src/shared/utils/fetchError.ts"; | ||
|
|
||
| const FALLBACK = "An error occurred"; | ||
|
|
||
| function jsonResponse(body: unknown, status = 500) { | ||
| return new Response(JSON.stringify(body), { | ||
| status, | ||
| headers: { "Content-Type": "application/json" }, | ||
| }); | ||
| } | ||
|
|
||
| // #3356: the Analytics page rendered a generic "An error occurred" because it | ||
| // discarded the server's error body. The page must surface the real (already | ||
| // sanitized server-side) message instead. | ||
|
|
||
| test("reads the OpenAI-style { error: { message } } shape from buildErrorBody", async () => { | ||
| const res = jsonResponse({ error: { message: "Failed to compute analytics", type: "api_error" } }); | ||
| assert.equal(await readFetchErrorMessage(res, FALLBACK), "Failed to compute analytics"); | ||
| }); | ||
|
|
||
| test("reads the legacy { error: '...' } string shape", async () => { | ||
| const res = jsonResponse({ error: "no such column: combo_name" }); | ||
| assert.equal(await readFetchErrorMessage(res, FALLBACK), "no such column: combo_name"); | ||
| }); | ||
|
|
||
| test("trims surrounding whitespace from the extracted message", async () => { | ||
| const res = jsonResponse({ error: { message: " boom " } }); | ||
| assert.equal(await readFetchErrorMessage(res, FALLBACK), "boom"); | ||
| }); | ||
|
|
||
| test("falls back when the error message is blank", async () => { | ||
| const res = jsonResponse({ error: { message: " " } }); | ||
| assert.equal(await readFetchErrorMessage(res, FALLBACK), FALLBACK); | ||
| }); | ||
|
|
||
| test("falls back when there is no error field", async () => { | ||
| const res = jsonResponse({ data: 1 }); | ||
| assert.equal(await readFetchErrorMessage(res, FALLBACK), FALLBACK); | ||
| }); | ||
|
|
||
| test("falls back on a non-JSON body without throwing", async () => { | ||
| const res = new Response("<html>500 Internal Server Error</html>", { status: 500 }); | ||
| assert.equal(await readFetchErrorMessage(res, FALLBACK), FALLBACK); | ||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Robustness Issue in Error Handling
There are two main concerns with the current error handling implementation in the
catchblock:errorisnullorundefined(which can happen if a promise is rejected with a falsy value orthrow nullis executed),String(error)will evaluate to"null"or"undefined". Since these are truthy strings, the fallback"Failed to compute analytics"will not be used, and the API will return an unhelpful"null"or"undefined"error message to the client. Additionally, if a plain object with amessageproperty is thrown, it will be serialized as"[object Object]".catch: Performing a dynamicawait import(...)inside acatchblock is risky. If the system is under memory pressure or there is a module resolution issue, the import itself can fail, throwing a secondary error that completely masks the original error and prevents a clean 500 response from being returned.Recommendation
buildErrorBodyat the top of the file.messageproperty.