Repository navigation
fix(frontend): give API transport failures messages users can act on - #1384
Conversation
The API client built its errors from developer text: a bare "HTTP 502" when the server sent no message, "Request to /api/x timed out after 90000ms" on timeout, the browser's "Failed to fetch" when the network dropped, and "Unexpected token '<'" when a proxy answered a 2xx with an HTML page. Dozens of toasts and inline errors interpolate err.message, so users saw that text with no next step. Login, password reset and first-admin setup call fetch() directly and had the same problems; setupAdmin parsed a 502 HTML body with no catch. The new api/errors.ts maps each case to a sentence (can't reach CUDly, took too long, temporarily unavailable, unexpected response, plus 401, 403, 404 and 429 wording). The client uses it at the source, so every existing caller benefits without changes, and the direct-fetch auth calls use the same helpers. A specific server message still wins (and the MFA sentinel codes still work); only the backend's catch-all "Internal server error" is replaced. The raw text stays on the new ApiError.detail field for support, statuses are unchanged (0 for transport failures), and a caller's own abort is rethrown untouched. Refs LeanerCloud#748 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Deleting a user, group, API key or registration, revoking an API key,
rejecting a registration, and saving bulk overrides all replaced the
server's error with a fixed string ("Failed to delete user", "Check
your connection and try again"), so an admin never learned why the
action was refused. These now append friendlyError(err): the server's
reason when it sent one, otherwise the transport wording.
Also adds a Playwright spec for the transport wording on the dashboard:
a 502 HTML page, a dropped connection and an HTML page served with 200.
Refs LeanerCloud#748
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 16 billable files and costs up to $4.00. Or wait 59 minutes for your next included review. View limit details
Comment |
Conflicts: - frontend/src/api/auth.ts (setupAdmin): kept this branch's responseError(). It covers main's fix (LeanerCloud#1326: a non-JSON 502 body no longer surfaces as a SyntaxError, a JSON error still wins) and also gives 5xx the friendly status sentence and keeps the raw HTTP status in ApiError.detail. Main's "Failed to create admin (HTTP 502)" text is therefore replaced by "CUDly is temporarily unavailable...". - frontend/src/__tests__/api.test.ts (setupAdmin): kept this branch's 502/400/offline tests, main's "JSON error body still surfaces the server message" test and main's new base64Encode suite. Dropped main's "non-JSON error body yields Failed to create admin (HTTP 502)" test: the same scenario is asserted by this branch's friendly-502 test. - frontend/src/groups/groupActions.ts: imports from both sides (friendlyError and main's systemGroups); deleteGroup's catch keeps main's intent (show the server's reason, LeanerCloud#919) through this branch's friendlyError(), which also words transport failures and an empty message instead of falling back to a bare "Failed to delete group". - frontend/src/settings.ts: imports from both sides (friendlyError and main's settings-account-delete). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE Blocking:
|
|
Local verification at
|
|
Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at |
When a proxy returned 502, the network dropped or a request timed out, users saw developer text such as "Failed to load dashboard: HTTP 502", "Failed to fetch", "Request to /api/... timed out after 90000ms" or "Unexpected token '<'", and several admin actions replaced the server's reason for a refusal with a fixed string. This PR rewrites transport, timeout, status and parse errors into plain sentences at the API client (so every existing caller benefits) and makes the listed admin actions show the server's reason.
Refs #748
Partial: the suggested error-state card with a Retry button and a
<details>block for raw detail on inline load failures is not included (a per-page UI change across dashboard, plans, history and inventory), and a few directfetch()calls are not converted yet (see Notes for review).What changed
frontend/src/api/errors.ts: friendly messages and helpers (httpStatusMessage,makeApiError,isNetworkFailure,friendlyError, plusfetchOrNetworkError/responseError/readJsonfor the auth calls that usefetch()directly).frontend/src/api/client.tsraises friendlyApiErrors at the source: status 0 with "Can't reach CUDly. Check your connection and try again." for network failures, "CUDly took too long to respond..." for timeouts, per-status sentences when the server sent no message (5xx unavailable, 500 unexpected error, 401/403/404/429), and "CUDly sent an unexpected response..." for unparseable 2xx bodies. A specific server message still wins (and the MFA sentinel codes still work); only the backend's catch-all "Internal server error" is replaced. The raw text is kept on a newApiError.detailfield (api/types.ts), and a caller's own abort is rethrown untouched.frontend/src/api/auth.ts: login, forgot/reset password andsetupAdminuse the same helpers. Main (fix(frontend): sign-in, ladder save and registrations review fixes #1326) already stopped a 502 HTML page during setup from surfacing as a JSON parse error, with "Failed to create admin (HTTP 502)"; this PR replaces that with "CUDly is temporarily unavailable..." and keeps the raw status ondetail.users/userActions.ts,modules/registrations.ts(delete and reject),apikeys.ts(delete and revoke) andsettings.ts(bulk overrides) appendfriendlyError(err)to their failure messages instead of dropping the reason or blaming the connection.groups/groupActions.tsdeleteGroupalready shows the server's reason on main (fix(frontend): hide Edit and Delete on system-managed groups #919); here it goes throughfriendlyError(err), which adds the friendly transport wording and replaces the empty-message fallback.api-errors.test.ts(13);api.test.tsadds network failure, caller-abort passthrough, 6 status mappings, the catch-all 500, server-message-wins and 3setupAdmincases (502 HTML, 400 reason kept, offline), with the old timeout/parse tests updated to the new wording. The 502 HTML case replaces main's "non-JSON error body yields Failed to create admin (HTTP 502)" test; new reason assertions inapikeys,groups,usersandsettings-accountstests; newtests-e2e/friendly-api-errors.spec.ts(3).Commits:
d40cf0df(client and auth wording),90290b8e(server reasons at the call sites, e2e spec), and the merge of main5c6ba46e.Why this approach
Fixing the wording once in the API client covers the roughly 30 interpolation sites listed in the issue (dashboard, history toasts, plans, inventory, API keys) without touching each one, and keeps statuses and
error.detailsunchanged, so existing branching onerr.statuskeeps working. Putting the server message first preserves validation reasons; treating only the exact catch-all 500 string as generic avoids hiding real reasons. Raw detail onApiError.detailkeeps support information available without showing it.Screenshots
Screenshots were taken before this branch was merged with main; before = main at a14dc8f. After is this branch. Both are headless Chromium at 1440x900 against the repo's mocked e2e API.
Verification
Run on this branch's head
5c6ba46e, fromfrontend/:frontend/src/api/auth.ts,frontend/src/__tests__/api.test.ts,frontend/src/groups/groupActions.tsandfrontend/src/settings.tsresolved, see the merge commit.npm run lint: 0 errors (existing warnings only). Typecheck and build pass.--randomizeon theapi,api-errors,apikeys,groupsandsettings-accountssuites: 360/360.users.test.tsfails 9 tests under one random seed with and without this branch's test changes (existing order dependence in the group-toggle/bulk-handler tests).e2e-local.sh): 205 passed, including the newfriendly-api-errors.spec.ts(502 HTML page, dropped connection, HTML page served with 200).scripts/test-git-secrets-allowlist.sh: 137 PASS.Notes for review
<details>block for raw detail on inline load failures; per-site rewording of the ~15history.tstoasts (they get the friendly text automatically but keep their "Failed to X:" prefixes); other directfetch()calls (getResetTokenStatus,checkAdminExists,getPublicInfo, deployment info,commitmentOptions).api/errors.ts, all phrased around "CUDly" (e.g. "CUDly is temporarily unavailable. Try again in a moment."). 401 says "Your session has expired. Sign in again.", which applies only when the server sends no message.init-error.ts) expects the oldHTTP <status>message, so with this branch a failed init reads "Details: HTTP 502: CUDly is temporarily unavailable. Try again in a moment." Showingerr.detailthere would be cleaner; not changed here.err.messageno longer contains the endpoint and milliseconds (moved toerr.detail), and a parse failure is now anApiErrorinstead of aSyntaxError. No source code depended on either.Overlaps
api.test.tsandapi/types.ts, and feat(frontend): Load more on the History view, Approval Queue from all loaded pages #676 editsapi/types.ts. Both merge cleanly with this branch.Note for the UI owner
This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.
🤖 Generated with Claude Code