mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-08 03:10:26 +00:00
fix(web): restore R5 error taxonomy in initial-load path
Earlier refactor to a discriminated-union loader accidentally
discarded the plan's R5 error taxonomy. `apiJsonOrNull` throws a
body-less Response on non-ok statuses, so the loader's try/catch had
no way to recover the server's error envelope or the request_id for
500s. The initial-error render then collapsed all statuses into
either `<EmptyState kind="unknown">` (401/403) or a generic
InlineErrorBanner — losing the plan-specified copy for access denied,
transient failures, and 500 with request ID.
Fixes:
- Loader now uses `fetch` directly against the API path so the
response body is preserved on non-ok statuses.
- 404/501 still collapse to `{data: null, error: null}` (the empty-
envelope signal the UI maps to R4).
- Any other non-ok parses the body as JSON, extracts request_id from
either the top-level `request_id` field or the uniform error
envelope (`errors[0].request_id` or parsed out of
`errors[0].detail`), and threads it through `error.requestId`.
- Component's `initialError` branch now applies the full R5 taxonomy:
R5(c) access denied for 401/403 with the specific copy, R5(a)
retry banner for 429/503, R5(d) "Something went wrong. Request ID:
<id>. Contact support." for 500s, and a generic retryable banner
for any other 4xx.
Adds run-files.test.ts covering extractRequestId across the three
locations request_id can show up in a server error body (top-level,
errors[0].request_id, errors[0].detail regex).
The RunFilesErrorBoundary export stays in place as defense-in-depth
for React render crashes — the loader no longer throws, but ensuring
the route always has a fallback is cheap.
Refs docs/plans/2026-04-19-002-feat-run-files-changed-tab-plan.md §
Unit 11 R5 taxonomy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
ef0cff2b31
commit
23df53766f
4 changed files with 276 additions and 136 deletions
53
apps/fabro-web/app/routes/run-files.test.ts
Normal file
53
apps/fabro-web/app/routes/run-files.test.ts
Normal file
|
|
@ -0,0 +1,53 @@
|
|||
import { describe, expect, test } from "bun:test";
|
||||
|
||||
import { extractRequestId } from "./run-files";
|
||||
|
||||
describe("extractRequestId", () => {
|
||||
test("reads `request_id` from the top level of the error body", () => {
|
||||
expect(extractRequestId({ request_id: "abc-123" })).toBe("abc-123");
|
||||
});
|
||||
|
||||
test("reads `request_id` from errors[0] under the uniform envelope", () => {
|
||||
expect(
|
||||
extractRequestId({
|
||||
errors: [
|
||||
{ status: "500", title: "Internal", request_id: "evt_42" },
|
||||
],
|
||||
}),
|
||||
).toBe("evt_42");
|
||||
});
|
||||
|
||||
test("parses `Request ID: xyz` out of errors[0].detail", () => {
|
||||
expect(
|
||||
extractRequestId({
|
||||
errors: [
|
||||
{
|
||||
status: "500",
|
||||
title: "Internal Server Error",
|
||||
detail: "Run files failed. Request ID: req_999 on shard 2.",
|
||||
},
|
||||
],
|
||||
}),
|
||||
).toBe("req_999");
|
||||
});
|
||||
|
||||
test("returns null for bodies without any request_id", () => {
|
||||
expect(extractRequestId(null)).toBe(null);
|
||||
expect(extractRequestId(undefined)).toBe(null);
|
||||
expect(extractRequestId("not an object")).toBe(null);
|
||||
expect(extractRequestId({ errors: [] })).toBe(null);
|
||||
expect(extractRequestId({ errors: [{ detail: "no id in here" }] })).toBe(
|
||||
null,
|
||||
);
|
||||
});
|
||||
|
||||
test("handles request_id values with hyphens and underscores", () => {
|
||||
expect(
|
||||
extractRequestId({
|
||||
errors: [
|
||||
{ detail: "Something failed. request_id: RX-1A_2B-3C4D" },
|
||||
],
|
||||
}),
|
||||
).toBe("RX-1A_2B-3C4D");
|
||||
});
|
||||
});
|
||||
|
|
@ -13,7 +13,6 @@ import {
|
|||
} from "react-router";
|
||||
import { MultiFileDiff, PatchDiff, Virtualizer } from "@pierre/diffs/react";
|
||||
import { useTheme } from "../lib/theme";
|
||||
import { apiJsonOrNull } from "../api";
|
||||
import type {
|
||||
FileDiff as ApiFileDiff,
|
||||
PaginatedRunFileList,
|
||||
|
|
@ -42,34 +41,93 @@ export const handle = { wide: true };
|
|||
* and the component keeps showing the last-good data with an inline banner.
|
||||
* This is the plan's "prior content stays mounted; inline banner with
|
||||
* Retry" behavior for mid-session refresh failures (§ Unit 11).
|
||||
*
|
||||
* `error.requestId` is extracted from the 500-response body per R5 so the
|
||||
* UI can surface it verbatim in the copy ("Request ID: xyz. Contact
|
||||
* support.") — not just the bare status code.
|
||||
*/
|
||||
export type RunFilesLoaderResult = {
|
||||
data: PaginatedRunFileList | null;
|
||||
error: { status: number; message: string } | null;
|
||||
error: {
|
||||
status: number;
|
||||
message: string;
|
||||
requestId: string | null;
|
||||
} | null;
|
||||
};
|
||||
|
||||
export async function loader({ request, params }: any): Promise<RunFilesLoaderResult> {
|
||||
try {
|
||||
const data = await apiJsonOrNull<PaginatedRunFileList>(
|
||||
`/runs/${params.id}/files`,
|
||||
{ request },
|
||||
);
|
||||
return { data, error: null };
|
||||
} catch (e) {
|
||||
if (e instanceof Response) {
|
||||
return {
|
||||
data: null,
|
||||
error: {
|
||||
status: e.status,
|
||||
message: e.statusText || `HTTP ${e.status}`,
|
||||
},
|
||||
};
|
||||
}
|
||||
return {
|
||||
data: null,
|
||||
error: { status: 0, message: String(e) },
|
||||
};
|
||||
export async function loader({
|
||||
request,
|
||||
params,
|
||||
}: any): Promise<RunFilesLoaderResult> {
|
||||
// Avoid apiJsonOrNull's `throw new Response(null, ...)` pattern — it
|
||||
// strips the response body, and we need the body to parse request_id
|
||||
// out of 500s per R5.
|
||||
const response = await fetch(`/api/v1/runs/${params.id}/files`, {
|
||||
credentials: "include",
|
||||
...(request?.signal ? { signal: request.signal } : {}),
|
||||
});
|
||||
|
||||
if (response.status === 404 || response.status === 501) {
|
||||
return { data: null, error: null };
|
||||
}
|
||||
if (response.ok) {
|
||||
const data = (await response.json()) as PaginatedRunFileList;
|
||||
return { data, error: null };
|
||||
}
|
||||
|
||||
// Parse the body once. 500 responses carry request_id per the server's
|
||||
// uniform error envelope; other statuses may not.
|
||||
let bodyText = "";
|
||||
try {
|
||||
bodyText = await response.text();
|
||||
} catch {
|
||||
// Body read failed — fall through with an empty string; the error
|
||||
// surface still reports the status.
|
||||
}
|
||||
let bodyJson: unknown = null;
|
||||
if (bodyText) {
|
||||
try {
|
||||
bodyJson = JSON.parse(bodyText);
|
||||
} catch {
|
||||
// non-JSON body is fine; we still got the status
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
data: null,
|
||||
error: {
|
||||
status: response.status,
|
||||
message: response.statusText || `HTTP ${response.status}`,
|
||||
requestId: extractRequestId(bodyJson),
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Pull request_id out of the server's uniform error envelope:
|
||||
* { "errors": [{ "status": "500", "title": "...", "detail": "..." }] }
|
||||
* Some deployments tag request_id at top level or within errors[].detail.
|
||||
*
|
||||
* Exported for unit testing; callers should prefer the already-extracted
|
||||
* value on `RunFilesLoaderResult.error.requestId`.
|
||||
*/
|
||||
export function extractRequestId(body: unknown): string | null {
|
||||
if (!body || typeof body !== "object") return null;
|
||||
const b = body as Record<string, unknown>;
|
||||
if (typeof b.request_id === "string") return b.request_id;
|
||||
const errors = b.errors;
|
||||
if (Array.isArray(errors) && errors.length > 0) {
|
||||
const first = errors[0];
|
||||
if (first && typeof first === "object") {
|
||||
const rec = first as Record<string, unknown>;
|
||||
if (typeof rec.request_id === "string") return rec.request_id;
|
||||
if (typeof rec.detail === "string") {
|
||||
const m = rec.detail.match(/request[_ ]id[=:]?\s*([a-zA-Z0-9-_]+)/i);
|
||||
if (m) return m[1];
|
||||
}
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
// Events that should trigger a revalidation. CheckpointCompleted is the
|
||||
|
|
@ -411,22 +469,51 @@ export default function RunFiles({ loaderData }: any) {
|
|||
return <LoadingSkeleton />;
|
||||
}
|
||||
|
||||
// Initial load failed and we have no prior data to fall back on —
|
||||
// render the status-specific error state inline (mirrors what the
|
||||
// ErrorBoundary would show, but without unmounting the route).
|
||||
// Initial load failed and we have no prior data to fall back on — render
|
||||
// the status-specific error state inline per plan § R5. The route does
|
||||
// not unmount; the RunFilesErrorBoundary is reserved for render-time
|
||||
// errors that aren't surfaced by the loader at all.
|
||||
if (initialError) {
|
||||
// R5(c): access denied.
|
||||
if (initialError.status === 401 || initialError.status === 403) {
|
||||
return (
|
||||
<EmptyState kind="unknown" />
|
||||
<div
|
||||
role="status"
|
||||
className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted"
|
||||
>
|
||||
You don't have access to this run's files.
|
||||
</div>
|
||||
);
|
||||
}
|
||||
// R5(a): 4xx transient (429) / 503 — retry affordance.
|
||||
if (initialError.status === 429 || initialError.status === 503) {
|
||||
return (
|
||||
<InlineErrorBanner
|
||||
message="The diff service is temporarily unavailable."
|
||||
onRetry={() => revalidator.revalidate()}
|
||||
/>
|
||||
);
|
||||
}
|
||||
// R5(d): 500 — surface request ID if we have it.
|
||||
if (initialError.status >= 500) {
|
||||
const suffix = initialError.requestId
|
||||
? ` Request ID: ${initialError.requestId}.`
|
||||
: "";
|
||||
return (
|
||||
<div
|
||||
role="status"
|
||||
className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted"
|
||||
>
|
||||
Something went wrong.{suffix} Please contact support if this
|
||||
persists.
|
||||
</div>
|
||||
);
|
||||
}
|
||||
// Any other 4xx that isn't 401/403/404/429 — treat like a retryable
|
||||
// transient failure. The banner keeps the user in context.
|
||||
return (
|
||||
<InlineErrorBanner
|
||||
message={
|
||||
initialError.status >= 500
|
||||
? `Something went wrong (${initialError.status}).`
|
||||
: `Couldn't load files (${initialError.status}).`
|
||||
}
|
||||
message={`Couldn't load files (${initialError.status}).`}
|
||||
onRetry={() => revalidator.revalidate()}
|
||||
/>
|
||||
);
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
2
lib/crates/fabro-spa/assets/index.html
generated
2
lib/crates/fabro-spa/assets/index.html
generated
|
|
@ -61,7 +61,7 @@
|
|||
<script type="module" src="/assets/chunk-sadshphz.js"></script>
|
||||
<script type="module" src="/assets/chunk-pmthkscp.js"></script>
|
||||
<script type="module" src="/assets/chunk-v61ks9f7.js"></script>
|
||||
<script type="module" src="/assets/entry-84st7geq.js"></script>
|
||||
<script type="module" src="/assets/entry-qzbwmrbr.js"></script>
|
||||
<script type="module" src="/assets/chunk-n1k68xa8.js"></script>
|
||||
<script type="module" src="/assets/chunk-rsph5pvm.js"></script>
|
||||
<script type="module" src="/assets/chunk-9t57pdty.js"></script>
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue