Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR improves how API/network errors are parsed and rendered across the UI by extracting structured error metadata (status/title, request/trace IDs, URL/method, response body, issues) and displaying it consistently in ResponseError and page-level PageError, with added unit + Playwright coverage for common error scenarios.
Changes:
- Add
extractErrorDetails+ refactorprepareCommonErrorMessageto derive consistent primary messages and structured metadata from errors. - Refactor
ResponseError/PageErrorUI to show titles, optional subtitles, and expandable details (fields/issues/response). - Add a new Playwright
errorDisplaysuite (+ snapshots) and expand unit tests for response/query error guards and error parsing.
Reviewed changes
Copilot reviewed 22 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/suites/tenant/diagnostics/tabs/OperationsModel.ts | Updates E2E locator to new .response-error markup. |
| tests/suites/errorDisplay/errorDisplayMocks.ts | Adds Playwright route mocks to simulate varied error shapes (HTTP, network, HTML, issues). |
| tests/suites/errorDisplay/errorDisplay.test.ts | Adds E2E coverage for inline/full-page/access-denied error rendering + screenshots. |
| tests/suites/errorDisplay/ErrorDisplayModel.ts | Adds a page-object model for interacting with the new error UI. |
| tests/suites/errorDisplay/errorDisplay.test.ts-snapshots/error-tablet-400-code-only-chromium-linux.png | New snapshot for tablet 400 (code-only) error case. |
| tests/suites/errorDisplay/errorDisplay.test.ts-snapshots/error-pdisk-503-empty-safari-linux.png | New snapshot for PDisk 503 empty-body error case (Safari). |
| tests/suites/errorDisplay/errorDisplay.test.ts-snapshots/error-pdisk-503-empty-chromium-linux.png | New snapshot for PDisk 503 empty-body error case (Chromium). |
| tests/suites/errorDisplay/errorDisplay.test.ts-snapshots/error-node-network-safari-linux.png | New snapshot for node network error case (Safari). |
| tests/suites/errorDisplay/errorDisplay.test.ts-snapshots/error-node-network-chromium-linux.png | New snapshot for node network error case (Chromium). |
| tests/suites/errorDisplay/errorDisplay.test.ts-snapshots/error-cluster-400-plain-text-chromium-linux.png | New snapshot for cluster 400 plain-text error case. |
| src/utils/errors/index.ts | Exposes extractErrorDetails and updates common error message preparation logic. |
| src/utils/errors/extractErrorDetails.ts | Introduces structured metadata extraction from error objects (headers/config/body/issues). |
| src/utils/errors/test/prepareCommonErrorMessage.test.ts | Adds unit coverage for message selection behavior across error types. |
| src/utils/errors/test/extractErrorDetails.test.ts | Adds unit coverage for header/config/body/issues extraction behavior. |
| src/utils/test/response.test.ts | Adds tests for additional response/network/access type-guards. |
| src/utils/test/query.test.ts | Adds tests for query error response type-guards. |
| src/components/Errors/shared/IssuesSection.tsx | Adds a reusable disclosure section for rendering issues lists. |
| src/components/Errors/shared/ErrorFieldsList.tsx | Adds a reusable definition-list for rendering extracted error metadata fields. |
| src/components/Errors/i18n/ru.json | Removes Russian keyset file for Errors component (ru disabled globally). |
| src/components/Errors/i18n/index.ts | Registers Errors i18n keyset with en only. |
| src/components/Errors/i18n/en.json | Extends English Errors keyset with new error-details labels. |
| src/components/Errors/ResponseError/ResponseError.tsx | Refactors ResponseError to use common message + extracted details UI. |
| src/components/Errors/ResponseError/ResponseError.scss | Adds styles for new ResponseError layout, fields, disclosures, and body block. |
| src/components/Errors/ResponseError/ErrorDetails.tsx | Adds expandable error details rendering (fields/response body/issues). |
| src/components/Errors/PageError/PageError.tsx | Forwards optional defaultMessage and fixes className composition for PageError. |
| src/components/Errors/PageError/PageError.scss | Adjusts PageError layout to better fit detailed error content. |
| src/components/EmptyState/EmptyState.scss | Adds size_l and layout tweaks to accommodate detailed error content. |
| scripts/playwright-docker.sh | Minor whitespace/formatting tweak. |
| .gitignore | Adds plans to ignored files and normalizes formatting. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
The test currently passes (likely because VDisk internally fetches storage group data), but this is fragile — a future routing change in the VDisk page could silently break this test's intent while the mock continues to intercept the wrong call. Consider adding a Prompt To Fix With AIThis is a comment left during a code review.
Path: tests/suites/errorDisplay/errorDisplayMocks.ts
Line: 844-853
Comment:
**VDisk mock intercepts storage groups endpoint, not a VDisk-specific route**
`setupVDisk429WithIssuesMock` intercepts `ROUTES.storageGroups` (`/storage/groups*`), yet the test navigates to the VDisk page (`vDisk?nodeId=1&vDiskId=1-0-0-0-0`). If the VDisk page loads data from a dedicated endpoint (e.g. `/vdisk/info*`), that call goes unmocked and may resolve or fail silently.
The test currently passes (likely because VDisk internally fetches storage group data), but this is fragile — a future routing change in the VDisk page could silently break this test's intent while the mock continues to intercept the wrong call. Consider adding a `vdiskInfo` route to the `ROUTES` map and mocking it explicitly for VDisk-specific tests.
How can I resolve this? If you propose a fix, please make it concise. |
src/containers/Tenant/Query/QueryResult/components/QueryResultError/QueryResultError.tsx
Show resolved
Hide resolved
|
@greptile review |
|
@greptile review |
|
@greptile review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
The isUnauthenticatedError check was catching all 401 errors including those with an authUrl that indicate a redirect-to-auth flow. Previously, PageError handled this by checking isRedirectToAuth first and returning null to allow the browser redirect to proceed. Without that guard, a 401 response carrying an authUrl incorrectly rendered the Unauthenticated component instead of letting the redirect happen. Add the isRedirectToAuth check before the 401 handling in both Operations.tsx and renderPaginatedTableErrorMessage.tsx, matching the pattern already used in PageError and Content components.
… on non-Result tabs For execute queries with errors, non-Result tabs (Schema, Explain, Stats) should show a simple 'Query Failed' message with a redirect to the Result tab, instead of rendering the full QueryResultError with ResponseError inline. The previous commit (13fa015) simplified the error branching and lost the isExecute check, causing all non-cancelled errors on non-Result tabs to render QueryResultError regardless of query type. This was confusing because the Result tab already shows the detailed error — seeing a full error panel on Schema/Stats tabs instead of the 'see the Result tab' message lost the helpful redirect. Changes: - Restore renderCommonErrorView with isExecute/isStopped branching - Restore i18n keys error.title and error.description - Update E2E test name to reflect correct behavior
…Message
A 403 response with data containing a code field (e.g. {code: 'FORBIDDEN'})
would return the raw code string instead of the localized 'Access forbidden'
message because extractDataMessage ran before the 403 check.
Move the 403 check above extractDataMessage so the localized message always
takes priority for 403 responses.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Stand
Closes #3487
Note
Medium Risk
Touches shared error rendering and streaming query error propagation; mistakes could hide errors or break auth/forbidden routing across multiple pages.
Overview
Improves error diagnostics and presentation across the UI.
ResponseErroris redesigned to render a structured alert with extracted metadata (URL/method, status/code, trace/request IDs, proxy/worker headers, network context) and optional expandable sections for response body and issues.Refactors full-page error handling.
PageErrornow distinguishes401vs403(newUnauthenticatedstate), supports a "Report a problem" action viauiFactory.getReportProblemUrl, and reuses the newuseErrorInfo/ResponseErrorMessageflow.Hardens streaming query errors.
StreamingAPInow converts non-OK responses and mid-stream/connection failures into enrichedErrorobjects (including useful headers and anerrorPhase) so they display consistently in the new diagnostics UI.Unifies illustrations and removes custom SVG loading. Deletes the dynamic
Illustrationcomponent and bundled SVG assets, replacing usage withgetIllustration()backed by@gravity-ui/illustrationswith optional overrides viauiFactory.illustrations.Adds extensive unit tests for error parsing/extraction and a new Playwright suite covering inline/full-page error scenarios; also tweaks
EmptyStatelayout options and Playwright screenshot expectations.Written by Cursor Bugbot for commit e1f1f29. This will update automatically on new commits. Configure here.
CI Results
Test Status: ❌ FAILED
📊 Full Report
Test Changes Summary ✨21
✨ New Tests (21)
Bundle Size: 🔺
Current: 62.98 MB | Main: 62.93 MB
Diff: +0.05 MB (0.08%)
ℹ️ CI Information
Greptile Summary
This PR is a substantial overhaul of error diagnostics across the UI:
ResponseErrorandPageErrorare redesigned to surface structured title/subtitle/expandable-details panels; a newextractErrorDetailspipeline extracts trace IDs, proxy headers, request URL, network context, and issues from heterogeneous error shapes; and the streaming fetch path is hardened to preserve non-JSON bodies, enriched errors with phase metadata, and realErrorinstances with stack traces.Key changes:
extractErrorDetails(+extractDataMessage,extractHeaders,extractConfig) converts any error shape into a typedErrorDetailsbag;prepareCommonErrorMessageanduseErrorInfoconsume it for consistent title/subtitle rendering across all error surfaces.isAccessErroris replaced byisUnauthenticatedError(401) andisForbiddenError(403); a newUnauthenticatedempty-state is added;isRedirectToAuthcorrectly short-circuits before both.Illustrationcomponent are removed;getIllustration(name)resolves from@gravity-ui/illustrationsdefaults withuiFactory.illustrationsoverrides.fetchfailures, non-OK responses, and mid-stream parse errors are all now enriched withconfig,headers, anderrorPhase;text()→JSON.parsechain replaces the previousresponse.json()call to preserve plain-text bodies.Two correctness issues found:
lib.tsmissing export —GetReportProblemUrlis added toUIFactorybut not re-exported fromlib.ts, preventing library consumers from typing theirgetReportProblemUrlimplementation without reaching into internal paths.pageTitleprop override inPageError—pageTitle={errorPageTitle}is placed after{...restProps}in all three branches (Unauthenticated, AccessDenied, PageErrorContent), so anypageTitlea caller passes is silently dropped whenerrorPageTitleisundefined.Confidence Score: 2/5
getReportProblemUrlimplementation without internal imports, and (2) thepageTitleprop override affects all three error state branches, silently breaking caller-provided page titles. Both are straightforward to fix but must be addressed before merge. The streaming and error-extraction logic itself is well-tested and sound.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Error received] --> B{isRedirectToAuth?} B -- yes --> C[return null\nredirect happens elsewhere] B -- no --> D{isUnauthenticatedError\nstatus 401?} D -- yes --> E[Unauthenticated\nempty state] D -- no --> F{isForbiddenError\nstatus 403?} F -- yes --> G[AccessDenied\nempty state] F -- no --> H[PageErrorContent /\nResponseError] H --> I[useErrorInfo] I --> J[prepareCommonErrorMessage\nfallbackMessage] I --> K[extractErrorDetails] K --> L{Error shape?} L -- HTTP response --> M[extractBasicProperties\nstatus, statusText, dataMessage, responseBody] L -- streaming error --> N[extractDiagnosticFields\nerrorPhase, message] L -- any --> O[extractHeaders\ntraceId, requestId, proxyName, workerName] L -- any --> P[extractConfig\nrequestUrl, method] L -- issues error --> Q[extractIssues\nhasIssues, issues array] M & N & O & P & Q --> R[ErrorDetails object] R --> S[enrichWithNetworkContext\nadd networkOnline/effectiveType\nfor network errors] S --> T[Render title + subtitle +\nErrorDetailsContent] T --> U{hasVisibleFields?} U -- yes --> V[ErrorFieldsList\nURL, traceId, requestId, etc.] U -- no --> W[skip fields] T --> X{has response body\nor issues?} X -- yes --> Y[Toggle button\nResponse / Issues] Y -- expanded --> Z[ResponseBodySection\nor IssuesSection]Comments Outside Diff (2)
src/lib.ts, line 36 (link)GetReportProblemUrltype not exportedGetReportProblemUrlwas added toUIFactoryas the type for the newgetReportProblemUrlfield, but it is not re-exported fromlib.ts. Library consumers who want to implementuiFactory.getReportProblemUrlwill not be able to correctly type their implementation without reaching into the internaluiFactory/typespath.IllustrationComponentandIllustrationNameare already exported here —GetReportProblemUrlshould follow the same pattern:src/components/Errors/PageError/PageError.tsx, line 40-85 (link)Caller-provided
pageTitlesilently overridden whenerrorPageTitleis undefinedpageTitleis part ofEmptyStatePropsand flows intorestPropswhen callers provide it. However,pageTitle={errorPageTitle}is specified after the spread, so whenerrorPageTitleisundefined(the default), it overwrites the caller's value.This occurs in three branches:
pageTitle={errorPageTitle}after{...restProps}pageTitle={errorPageTitle}after{...restProps}pageTitle={errorPageTitle}after{...restProps}Example:
<PageError error={err} pageTitle="Cluster" />— the"Cluster"title is lost.Fix: Use the caller's
pageTitleas a fallback whenerrorPageTitleis not provided:Or, for cleaner code, explicitly destructure
pageTitle:Last reviewed commit: e1f1f29