mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-07 03:00:29 +00:00
fix(web): complete lifecycle-status coverage + loader integration tests
Two follow-ups from internal review:
1. deriveEmptyKind was incomplete. The full RunStatus enum (per
fabro-types/src/status.rs and apps/fabro-web/app/data/runs.ts) has
ten values — submitted, queued, starting, running, blocked,
paused, removing, succeeded, failed, dead. My decision table
covered only six and incorrectly included "partialsuccess" which
is a stage status, not a run status. Unhandled statuses
(blocked, paused, removing, dead) silently fell through to the
"diff_lost" branch, which showed users the alarmist "the diff for
this run is no longer available" copy for runs that are merely
paused or being torn down.
New table:
- submitted / queued / starting → R4(a) "starting"
- running / blocked / paused → R4(b) "no_changes" (yet — user
can refresh)
- failed / dead → R4(c1) "failed before checkpoint"
(R4b-equivalent when a degraded
patch did survive)
- succeeded / removing → R4(c2) "diff_lost" if
total_changed > 0, else R4(b)
- unknown future status → R4 "unknown" fallback
Test suite now drives each documented status through a regression
guard that asserts no known status collapses to "unknown" when a
more-specific kind should apply.
2. Loader integration tests. The `extractRequestId` unit test covers
only the extractor; nothing exercised the full fetch → body-read
→ requestId → error chain. Added 8 loader tests covering:
- 200 OK returns the parsed envelope
- 404 / 501 collapse to the empty-envelope signal (null + null)
- 500 with `request_id` in errors[0] populates error.requestId
- 500 without a request_id leaves it null
- 500 with non-JSON body still surfaces the status
- 503 populates error without requestId
- 401 surfaces as an error (no in-loader redirect — that concern
lives in apiFetch, which the Files loader deliberately bypasses
to preserve error bodies)
The tests stub globalThis.fetch; the loader already accepts the
cancellation-signal-only `request` object.
Refs docs/plans/2026-04-19-002-feat-run-files-changed-tab-plan.md §
Unit 11 R4/R5 taxonomies.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
23df53766f
commit
c44c93ff32
5 changed files with 262 additions and 67 deletions
|
|
@ -1,6 +1,30 @@
|
|||
import { describe, expect, test } from "bun:test";
|
||||
import { afterEach, describe, expect, test } from "bun:test";
|
||||
|
||||
import { extractRequestId } from "./run-files";
|
||||
import { extractRequestId, loader } from "./run-files";
|
||||
|
||||
type StubResponseInit = {
|
||||
status: number;
|
||||
body?: string;
|
||||
headers?: Record<string, string>;
|
||||
};
|
||||
|
||||
function stubFetchOnce(init: StubResponseInit) {
|
||||
const original = globalThis.fetch;
|
||||
globalThis.fetch = (() => {
|
||||
const response = new Response(init.body ?? "", {
|
||||
status: init.status,
|
||||
headers: init.headers,
|
||||
// `Response` constructor derives statusText from status for known
|
||||
// codes; providing it explicitly keeps tests deterministic across
|
||||
// engines that disagree on the default message.
|
||||
statusText: init.status === 500 ? "Internal Server Error" : "",
|
||||
});
|
||||
return Promise.resolve(response);
|
||||
}) as typeof fetch;
|
||||
return () => {
|
||||
globalThis.fetch = original;
|
||||
};
|
||||
}
|
||||
|
||||
describe("extractRequestId", () => {
|
||||
test("reads `request_id` from the top level of the error body", () => {
|
||||
|
|
@ -51,3 +75,104 @@ describe("extractRequestId", () => {
|
|||
).toBe("RX-1A_2B-3C4D");
|
||||
});
|
||||
});
|
||||
|
||||
describe("loader", () => {
|
||||
let restoreFetch: (() => void) | undefined;
|
||||
|
||||
afterEach(() => {
|
||||
restoreFetch?.();
|
||||
restoreFetch = undefined;
|
||||
});
|
||||
|
||||
// Bun's Request constructor needs a URL; tests use a relative path
|
||||
// because the loader only reads `request?.signal`.
|
||||
const dummyRequest = { signal: undefined } as any;
|
||||
const dummyParams = { id: "01ARZ3NDEKTSV4RRFFQ69G5FAV" };
|
||||
|
||||
test("200 OK returns { data, error: null } with parsed envelope", async () => {
|
||||
const envelope = {
|
||||
data: [],
|
||||
meta: { truncated: false, total_changed: 0 },
|
||||
};
|
||||
restoreFetch = stubFetchOnce({
|
||||
status: 200,
|
||||
body: JSON.stringify(envelope),
|
||||
});
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.error).toBeNull();
|
||||
expect(result.data).toEqual(envelope);
|
||||
});
|
||||
|
||||
test("404 returns empty-envelope signal { data: null, error: null }", async () => {
|
||||
restoreFetch = stubFetchOnce({ status: 404 });
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.data).toBeNull();
|
||||
expect(result.error).toBeNull();
|
||||
});
|
||||
|
||||
test("501 returns empty-envelope signal { data: null, error: null }", async () => {
|
||||
restoreFetch = stubFetchOnce({ status: 501 });
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.data).toBeNull();
|
||||
expect(result.error).toBeNull();
|
||||
});
|
||||
|
||||
test("500 populates error.requestId from the uniform error envelope", async () => {
|
||||
restoreFetch = stubFetchOnce({
|
||||
status: 500,
|
||||
body: JSON.stringify({
|
||||
errors: [
|
||||
{
|
||||
status: "500",
|
||||
title: "Internal Server Error",
|
||||
detail: "Run files materialization panicked.",
|
||||
request_id: "req_deadbeef",
|
||||
},
|
||||
],
|
||||
}),
|
||||
});
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.data).toBeNull();
|
||||
expect(result.error).not.toBeNull();
|
||||
expect(result.error!.status).toBe(500);
|
||||
expect(result.error!.requestId).toBe("req_deadbeef");
|
||||
});
|
||||
|
||||
test("500 with no request_id leaves error.requestId as null", async () => {
|
||||
restoreFetch = stubFetchOnce({
|
||||
status: 500,
|
||||
body: JSON.stringify({
|
||||
errors: [{ status: "500", title: "Internal", detail: "whoops" }],
|
||||
}),
|
||||
});
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.error).not.toBeNull();
|
||||
expect(result.error!.status).toBe(500);
|
||||
expect(result.error!.requestId).toBeNull();
|
||||
});
|
||||
|
||||
test("500 with non-JSON body still surfaces the status", async () => {
|
||||
restoreFetch = stubFetchOnce({ status: 500, body: "<html>oops</html>" });
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.error).not.toBeNull();
|
||||
expect(result.error!.status).toBe(500);
|
||||
expect(result.error!.requestId).toBeNull();
|
||||
});
|
||||
|
||||
test("503 populates error without requestId", async () => {
|
||||
restoreFetch = stubFetchOnce({
|
||||
status: 503,
|
||||
body: JSON.stringify({ errors: [{ detail: "rate limited" }] }),
|
||||
});
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.error).not.toBeNull();
|
||||
expect(result.error!.status).toBe(503);
|
||||
});
|
||||
|
||||
test("401 still surfaces as an error (no in-loader redirect)", async () => {
|
||||
restoreFetch = stubFetchOnce({ status: 401 });
|
||||
const result = await loader({ request: dummyRequest, params: dummyParams });
|
||||
expect(result.error).not.toBeNull();
|
||||
expect(result.error!.status).toBe(401);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -15,8 +15,10 @@ function renderToJson(element: React.ReactElement): any {
|
|||
}
|
||||
|
||||
describe("deriveEmptyKind", () => {
|
||||
test("submitted / starting / queued map to R4(a) 'starting'", () => {
|
||||
for (const status of ["submitted", "Submitted", "starting", "queued"]) {
|
||||
// Pre-work states → R4(a) "starting"
|
||||
test.each(["submitted", "Submitted", "starting", "queued"])(
|
||||
"%s maps to R4(a) 'starting'",
|
||||
(status) => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: status,
|
||||
|
|
@ -24,48 +26,64 @@ describe("deriveEmptyKind", () => {
|
|||
degraded: false,
|
||||
}),
|
||||
).toBe("starting");
|
||||
}
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
test("failed run without degraded fallback is R4(c1)", () => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: "failed",
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("failed_before_checkpoint");
|
||||
});
|
||||
// Actively-in-progress states → R4(b) "no_changes yet"
|
||||
test.each(["running", "blocked", "paused"])(
|
||||
"%s with no files yet is R4(b) 'no_changes'",
|
||||
(status) => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: status,
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("no_changes");
|
||||
},
|
||||
);
|
||||
|
||||
test("succeeded run with changes but no data is R4(c2) 'diff_lost'", () => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: "succeeded",
|
||||
totalChanged: 3,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("diff_lost");
|
||||
});
|
||||
// Terminal-failure states → R4(c1) when no degraded patch available
|
||||
test.each(["failed", "dead"])(
|
||||
"%s without degraded fallback is R4(c1) 'failed_before_checkpoint'",
|
||||
(status) => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: status,
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("failed_before_checkpoint");
|
||||
},
|
||||
);
|
||||
|
||||
test("succeeded run with no changes is R4(b)", () => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: "succeeded",
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("no_changes");
|
||||
});
|
||||
// Terminal-success + teardown states → R4(b) or R4(c2) depending on
|
||||
// whether files were ever changed
|
||||
test.each(["succeeded", "removing"])(
|
||||
"%s with changes but no data is R4(c2) 'diff_lost'",
|
||||
(status) => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: status,
|
||||
totalChanged: 3,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("diff_lost");
|
||||
},
|
||||
);
|
||||
|
||||
test("running run with no changes is R4(b)", () => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: "running",
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("no_changes");
|
||||
});
|
||||
test.each(["succeeded", "removing"])(
|
||||
"%s with no changes is R4(b)",
|
||||
(status) => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: status,
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("no_changes");
|
||||
},
|
||||
);
|
||||
|
||||
test("missing runStatus collapses to 'unknown'", () => {
|
||||
expect(
|
||||
|
|
@ -76,6 +94,43 @@ describe("deriveEmptyKind", () => {
|
|||
}),
|
||||
).toBe("unknown");
|
||||
});
|
||||
|
||||
test("unknown future status collapses to 'unknown'", () => {
|
||||
expect(
|
||||
deriveEmptyKind({
|
||||
runStatus: "some_future_state",
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
}),
|
||||
).toBe("unknown");
|
||||
});
|
||||
|
||||
test("every documented RunStatus gets a non-unknown empty kind when applicable", () => {
|
||||
// Regression guard: if a new RunStatus appears in
|
||||
// `apps/fabro-web/app/data/runs.ts` without a matching branch here,
|
||||
// the decision table silently returns "unknown" ("not available right
|
||||
// now") — misleading copy for a cancelled or paused run.
|
||||
const knownRunStatuses = [
|
||||
"submitted",
|
||||
"queued",
|
||||
"starting",
|
||||
"running",
|
||||
"blocked",
|
||||
"paused",
|
||||
"removing",
|
||||
"succeeded",
|
||||
"failed",
|
||||
"dead",
|
||||
];
|
||||
for (const status of knownRunStatuses) {
|
||||
const result = deriveEmptyKind({
|
||||
runStatus: status,
|
||||
totalChanged: 0,
|
||||
degraded: false,
|
||||
});
|
||||
expect(result).not.toBe("unknown");
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("emptyStateCopy", () => {
|
||||
|
|
|
|||
|
|
@ -43,8 +43,14 @@ export function emptyStateCopy(kind: EmptyKind): string {
|
|||
}
|
||||
|
||||
/// Derive the empty-state variant from the full loader context. `runStatus`
|
||||
/// comes from the parent run loader; its absence collapses to the "unknown"
|
||||
/// catchall so the empty state never displays misleading copy.
|
||||
/// comes from the parent run loader (`run.lifecycleStatus`); its absence
|
||||
/// collapses to the "unknown" catchall so the empty state never displays
|
||||
/// misleading copy.
|
||||
///
|
||||
/// The full RunStatus enum (per fabro-types/src/status.rs) is:
|
||||
/// submitted, queued, starting, running, blocked, paused, removing,
|
||||
/// succeeded, failed, dead
|
||||
/// partial_success is a stage status, not a run status.
|
||||
export function deriveEmptyKind(args: {
|
||||
runStatus: string | undefined;
|
||||
totalChanged: number;
|
||||
|
|
@ -54,32 +60,41 @@ export function deriveEmptyKind(args: {
|
|||
if (!runStatus) {
|
||||
return "unknown";
|
||||
}
|
||||
const normalized = runStatus.toLowerCase();
|
||||
if (
|
||||
normalized === "submitted" ||
|
||||
normalized === "starting" ||
|
||||
normalized === "queued"
|
||||
) {
|
||||
const s = runStatus.toLowerCase();
|
||||
|
||||
// Pre-work states: run has no base_sha / hasn't started producing a diff.
|
||||
if (s === "submitted" || s === "queued" || s === "starting") {
|
||||
return "starting";
|
||||
}
|
||||
if (normalized === "failed" && !degraded) {
|
||||
return "failed_before_checkpoint";
|
||||
|
||||
// Actively-in-progress states: the run is running but just hasn't
|
||||
// changed any files yet. Avoid alarmist "diff lost" copy here — the
|
||||
// user may refresh and see files appear.
|
||||
if (s === "running" || s === "blocked" || s === "paused") {
|
||||
return "no_changes";
|
||||
}
|
||||
if (
|
||||
(normalized === "succeeded" || normalized === "partialsuccess") &&
|
||||
!degraded
|
||||
) {
|
||||
// If the run ran successfully and we still have no diff data, the
|
||||
// projection's final_patch was never captured (or was lost). R4(c2).
|
||||
if (totalChanged > 0) {
|
||||
return "diff_lost";
|
||||
|
||||
// Terminal-failure states: Failed and Dead both mean the run stopped
|
||||
// without a clean conclusion. If the degraded-fallback branch also
|
||||
// couldn't surface a patch, we never captured a diff at all.
|
||||
if (s === "failed" || s === "dead") {
|
||||
return degraded ? "unknown" : "failed_before_checkpoint";
|
||||
}
|
||||
|
||||
// Terminal-success + teardown states: the run ran to completion or is
|
||||
// shutting down. Distinguish "succeeded with no changes" (R4b) from
|
||||
// "succeeded but diff lost" (R4c2) via total_changed.
|
||||
if (s === "succeeded" || s === "removing") {
|
||||
if (degraded) {
|
||||
// We have a patch; the component renders PatchDiff instead of an
|
||||
// empty state. Shouldn't reach here in practice.
|
||||
return "unknown";
|
||||
}
|
||||
return "no_changes";
|
||||
return totalChanged > 0 ? "diff_lost" : "no_changes";
|
||||
}
|
||||
if (totalChanged === 0) {
|
||||
return "no_changes";
|
||||
}
|
||||
return "diff_lost";
|
||||
|
||||
// Unknown future status — fail conservative.
|
||||
return "unknown";
|
||||
}
|
||||
|
||||
export function LoadingSkeleton() {
|
||||
|
|
|
|||
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-qzbwmrbr.js"></script>
|
||||
<script type="module" src="/assets/entry-8hk343fy.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