diff --git a/tools/web-ux/browser-tests/ticket_shell_test.ts b/tools/web-ux/browser-tests/ticket_shell_test.ts index 15ad79bdd..9f8b9f00e 100644 --- a/tools/web-ux/browser-tests/ticket_shell_test.ts +++ b/tools/web-ux/browser-tests/ticket_shell_test.ts @@ -72,6 +72,49 @@ async function queryRequests(base: string) { return requests.filter((row: { path: string }) => row.path === "/tickets/query"); } +Deno.test("Ticket parser failures stay local, retain Workspace navigation, and recover on retry", async () => { + await withFixture(async (browser, base) => { + const page = await browser.newPage(); + const wire = await detail(base, "T-1"); + delete wire.initial_post; + let invalid = true; + await page.route("**/api/w/tickets/tickets/T-1", async (route) => { + if (invalid) await route.fulfill({ status: 200, json: wire }); + else await route.continue(); + }); + await page.goto(`${base}/w/tickets/tickets/T-1`); + await page.getByRole("alert").filter({ hasText: "Ticket detail.initial_post must be an object" }).waitFor(); + assertEquals(await page.getByText("The selected Workspace cannot be opened", { exact: true }).count(), 0); + assertEquals(await page.getByText("Workspace unavailable", { exact: true }).count(), 0); + assert(await page.getByRole("navigation", { name: "Current workspace location" }).isVisible()); + invalid = false; + await page.getByRole("button", { name: "Retry", exact: true }).click(); + await page.locator(".ticket-detail-header").waitFor(); + assertEquals(await page.getByRole("alert").filter({ hasText: "initial_post must be an object" }).count(), 0); + await page.close(); + }); +}); + +Deno.test("Workspace HTTP failure retains its real status and recovers without recreating the Workspace", async () => { + await withFixture(async (browser, base) => { + const page = await browser.newPage(); + let unavailable = true; + await page.route("**/api/w/tickets/workspace", async (route) => { + if (unavailable) await route.fulfill({ status: 503, body: "private server failure" }); + else await route.continue(); + }); + await page.goto(`${base}/w/tickets/tickets/T-1`); + await page.getByRole("heading", { name: "Workspace details could not be loaded" }).waitFor(); + assert(await page.locator(".workspace-page [role=alert]").filter({ hasText: "HTTP 503" }).isVisible()); + assertEquals(await page.getByText("The selected Workspace cannot be opened", { exact: true }).count(), 0); + assertEquals(await page.getByText("private server failure", { exact: true }).count(), 0); + unavailable = false; + await page.getByRole("button", { name: "Retry", exact: true }).click(); + await page.locator(".ticket-detail-header").waitFor(); + await page.close(); + }); +}); + Deno.test("single Ticket query searches beyond loaded rows and preserves qualifiers on pagination", async () => { await withFixture(async (browser, base) => { const page = await browser.newPage(); diff --git a/web/workspace/src/lib/workspace/api/http.ts b/web/workspace/src/lib/workspace/api/http.ts index c90ae3352..27773b1bc 100644 --- a/web/workspace/src/lib/workspace/api/http.ts +++ b/web/workspace/src/lib/workspace/api/http.ts @@ -12,6 +12,8 @@ import { export type ApiResult = { data: T | null; error: string | null; + /** HTTP failure status, when a response was received; absent for local/transport failures. */ + status?: number; }; export type { SkillCatalogResponse, SkillDetailResponse }; @@ -19,6 +21,8 @@ export type { SkillCatalogResponse, SkillDetailResponse }; export type JsonLoadPolicy = { diagnosticLabel: string; maxResponseBytes: number; + /** Only explicitly trusted validator errors may expose bounded field diagnostics. */ + describeValidationError?: (error: unknown) => string | undefined; }; const SKILL_API_LOAD_POLICY: JsonLoadPolicy = { @@ -96,6 +100,7 @@ export async function loadJson( parse: (value: unknown) => T = (value) => value as T, policy?: JsonLoadPolicy, ): Promise> { + let stage: "request" | "decode" | "validate" = "request"; try { const response = await fetchFn(path, init); if (!response.ok) { @@ -103,6 +108,7 @@ export async function loadJson( await response.body?.cancel(); return { data: null, + status: response.status, error: `${policy.diagnosticLabel} request failed with HTTP ${response.status}`, }; @@ -110,19 +116,31 @@ export async function loadJson( const text = await response.text(); return { data: null, + status: response.status, error: text || `${path} request failed (${response.status})`, }; } + stage = "decode"; const payload: unknown = policy ? await readBoundedJson(response, policy.maxResponseBytes) : await response.json(); + stage = "validate"; return { data: parse(payload), error: null }; } catch (error) { if (policy) { + const validationDiagnostic = stage === "validate" + ? policy.describeValidationError?.(error) + : undefined; const diagnostic = error instanceof SkillApiContractError ? error.message + : validationDiagnostic !== undefined + ? `${policy.diagnosticLabel}: ${validationDiagnostic}` : error instanceof ResponseByteLimitError ? `${policy.diagnosticLabel} response exceeds its byte limit` + : stage === "request" + ? `${policy.diagnosticLabel} request could not be completed` + : stage === "decode" + ? `${policy.diagnosticLabel} response could not be decoded as JSON` : `${policy.diagnosticLabel} response is invalid`; return { data: null, error: diagnostic.slice(0, 256) }; } diff --git a/web/workspace/src/lib/workspace/api/ticket-browser.test.ts b/web/workspace/src/lib/workspace/api/ticket-browser.test.ts index e5c0c2cd6..5df30634b 100644 --- a/web/workspace/src/lib/workspace/api/ticket-browser.test.ts +++ b/web/workspace/src/lib/workspace/api/ticket-browser.test.ts @@ -2,8 +2,10 @@ declare const Deno: { test(name: string, fn: () => Promise | void): void; }; +import { loadJson } from "./http.ts"; import { fixturePost } from "../home/dashboard.test-fixtures.ts"; import { + TICKET_BROWSER_API_LOAD_POLICY, parseTicketEvent, parseMergeRequestDetailResponse, parseMergeRequestListResponse, @@ -64,12 +66,37 @@ Deno.test("Ticket post parser rejects absent safety fields and unknown wire fiel } assertThrows(() => parseTicketEvent({ ...fixturePost("Post"), editable: "yes" }), "editable"); assertThrows(() => parseTicketEvent({ ...fixturePost("Post"), edited_by: 7 }), "edited_by"); - assertThrows(() => parseTicketEvent({ ...fixturePost("Post"), surprise: true }), "unknown field surprise"); + assertThrows(() => parseTicketEvent({ ...fixturePost("Post"), surprise: true }), "unknown field"); const detail: Record = { ...ticketDetailFixture() }; delete detail.initial_post; assertThrows(() => parseTicketDetail(detail), "initial_post"); }); +Deno.test("Ticket load diagnostics retain the failed field without leaking response contents", async () => { + const secret = "PRIVATE_RESPONSE_TEXT"; + const wire: Record = { ...ticketDetailFixture(), body: secret }; + delete wire.initial_post; + const load = (payload: unknown) => loadJson( + (async () => Response.json(payload)) as typeof fetch, + "/api/w/workspace-a/tickets/T-1", + undefined, + parseTicketDetail, + TICKET_BROWSER_API_LOAD_POLICY, + ); + const missing = await load(wire); + assertEquals(missing.data, null); + assertEquals(missing.error, "Ticket Browser API: Ticket detail.initial_post must be an object"); + assertEquals(missing.status, undefined); + const unknown = await load({ ...ticketDetailFixture(), [secret]: true }); + assertEquals(unknown.error, "Ticket Browser API: Ticket detail contains unknown field"); + const nested = ticketDetailFixture(); + const malformed = await load({ ...nested, initial_post: { ...nested.initial_post, attributes: { [secret]: 5 } } }); + assertEquals(malformed.error, "Ticket Browser API: Ticket detail.initial_post.attributes value must be a string"); + for (const result of [missing, unknown, malformed]) { + assertEquals(result.error?.includes(secret), false); + } +}); + Deno.test("Ticket Browser parser accepts the generated Ticket list shape", () => { const parsed = parseTicketListResponse({ workspace_id: "workspace-a", @@ -105,7 +132,7 @@ Deno.test("Ticket Browser parser rejects unknown fields and out-of-range limits" }; assertThrows( () => parseTicketListResponse({ ...fixture, stale: true }), - "unknown field stale", + "unknown field", ); assertThrows( () => parseTicketListResponse({ ...fixture, limit: 1001 }), @@ -296,7 +323,7 @@ Deno.test("Ticket Browser parser accepts target collections and rejects singular repository_key: "main", ref_selector: "develop", }), - "unknown field repository_key", + "unknown field", ); }); @@ -393,11 +420,11 @@ Deno.test("Merge Request summaries reject missing and malformed tagged source ob { status: "unavailable", code: "x".repeat(262_145) }, "code exceeds its length limit", ], - [{ status: "observed", code: "stale" }, "unknown field code"], - [{ status: "not_required", code: "stale" }, "unknown field code"], + [{ status: "observed", code: "stale" }, "unknown field"], + [{ status: "not_required", code: "stale" }, "unknown field"], [ { status: "unavailable", code: "missing", extra: true }, - "unknown field extra", + "unknown field", ], ] as const ) { @@ -529,7 +556,7 @@ Deno.test("Ticket Browser parser validates the canonical Merge Request thread", ...parsed, thread: [{ ...parsed.thread[0], unexpected: true }], }), - "unknown field unexpected", + "unknown field", ); }); @@ -692,7 +719,7 @@ Deno.test("Ticket tag projections reject missing collections and malformed ident "last_operation_key must be a string", ], [{ ...tag, deleted: "false" }, "deleted must be a boolean"], - [{ ...tag, state: "closed" }, "unknown field state"], + [{ ...tag, state: "closed" }, "unknown field"], ] as const ) { assertThrows(() => parse([value]), message); diff --git a/web/workspace/src/lib/workspace/api/ticket-browser.ts b/web/workspace/src/lib/workspace/api/ticket-browser.ts index b92635f4c..955925b35 100644 --- a/web/workspace/src/lib/workspace/api/ticket-browser.ts +++ b/web/workspace/src/lib/workspace/api/ticket-browser.ts @@ -54,10 +54,14 @@ import type { WorkspaceTag, } from "#lib/generated/ticket-api.ts"; +export class TicketBrowserContractError extends Error {} + export const TICKET_BROWSER_API_MAX_RESPONSE_BYTES = 4 * 1024 * 1024; export const TICKET_BROWSER_API_LOAD_POLICY = { diagnosticLabel: "Ticket Browser API", maxResponseBytes: TICKET_BROWSER_API_MAX_RESPONSE_BYTES, + describeValidationError: (error: unknown) => + error instanceof TicketBrowserContractError ? error.message : undefined, } as const; const MAX_STRING_LENGTH = 262_144; @@ -66,7 +70,7 @@ const MAX_MAP_ENTRIES = 4_096; function object(value: unknown, label: string): Record { if (typeof value !== "object" || value === null || Array.isArray(value)) { - throw new Error(`${label} must be an object`); + throw new TicketBrowserContractError(`${label} must be an object`); } return value as Record; } @@ -78,20 +82,20 @@ function exact( ): void { const unexpected = Object.keys(value).find((key) => !allowed.includes(key)); if (unexpected !== undefined) { - throw new Error(`${label} contains unknown field ${unexpected}`); + throw new TicketBrowserContractError(`${label} contains unknown field`); } } function string(value: unknown, label: string): string { - if (typeof value !== "string") throw new Error(`${label} must be a string`); + if (typeof value !== "string") throw new TicketBrowserContractError(`${label} must be a string`); if (value.length > MAX_STRING_LENGTH) { - throw new Error(`${label} exceeds its length limit`); + throw new TicketBrowserContractError(`${label} exceeds its length limit`); } return value; } function boolean(value: unknown, label: string): boolean { - if (typeof value !== "boolean") throw new Error(`${label} must be a boolean`); + if (typeof value !== "boolean") throw new TicketBrowserContractError(`${label} must be a boolean`); return value; } @@ -105,7 +109,7 @@ function integer( typeof value !== "number" || !Number.isSafeInteger(value) || value < minimum || value > maximum ) { - throw new Error( + throw new TicketBrowserContractError( `${label} must be an integer from ${minimum} through ${maximum}`, ); } @@ -128,9 +132,9 @@ function array( label: string, parse: (item: unknown, label: string) => T, ): T[] { - if (!Array.isArray(value)) throw new Error(`${label} must be an array`); + if (!Array.isArray(value)) throw new TicketBrowserContractError(`${label} must be an array`); if (value.length > MAX_COLLECTION_LENGTH) { - throw new Error(`${label} exceeds its item limit`); + throw new TicketBrowserContractError(`${label} exceeds its item limit`); } return value.map((item, index) => parse(item, `${label}[${index}]`)); } @@ -143,12 +147,12 @@ function stringMap(value: unknown, label: string): Record { const source = object(value, label); const entries = Object.entries(source); if (entries.length > MAX_MAP_ENTRIES) { - throw new Error(`${label} exceeds its entry limit`); + throw new TicketBrowserContractError(`${label} exceeds its entry limit`); } return Object.fromEntries( entries.map(([key, item]) => [ string(key, `${label} key`), - string(item, `${label}.${key}`), + string(item, `${label} value`), ]), ); } @@ -199,7 +203,7 @@ function parseInvalidProjectRecord( function parseTicketState(value: unknown, label: string): BrowserTicketState { if (value !== "open" && value !== "closed") { - throw new Error(`${label} is invalid`); + throw new TicketBrowserContractError(`${label} is invalid`); } return value; } @@ -226,7 +230,7 @@ function parseCaseAction(value: unknown, label: string): TicketCaseAction { return { action: "reopen" }; } if (item.action !== "close" && item.action !== "correct_closure") { - throw new Error(`${label}.action is invalid`); + throw new TicketBrowserContractError(`${label}.action is invalid`); } exact(item, ["action", "reason"], label); return { @@ -351,7 +355,7 @@ export function parseWorkspaceTag( ], label); const id = string(item.id, `${label}.id`); if (id.trim().length === 0 || new TextEncoder().encode(id).length > 128) { - throw new Error(`${label}.id must be nonempty and at most 128 UTF-8 bytes`); + throw new TicketBrowserContractError(`${label}.id must be nonempty and at most 128 UTF-8 bytes`); } return { workspace_id: string(item.workspace_id, `${label}.workspace_id`), @@ -588,7 +592,7 @@ function parseTicketAssignmentPrincipal( exact(item, ["agent_key", "kind"], label); return { kind, agent_key: string(item.agent_key, `${label}.agent_key`) }; } - throw new Error(`${label}.kind is invalid`); + throw new TicketBrowserContractError(`${label}.kind is invalid`); } function parseTicketAssignmentSummary( @@ -653,7 +657,7 @@ function parseTicketRoleAssignmentRecord( ], label); const role = string(item.role, `${label}.role`); if (!["orchestrator", "worker", "owner", "contributor"].includes(role)) { - throw new Error(`${label}.role is invalid`); + throw new TicketBrowserContractError(`${label}.role is invalid`); } return { assigned_at: string(item.assigned_at, `${label}.assigned_at`), @@ -681,7 +685,7 @@ export function parseAssignmentTracking( ); const status = string(item.status, `${label}.status`); if (status !== "waiting" && status !== "inprogress" && status !== "done") { - throw new Error(`${label}.status is invalid`); + throw new TicketBrowserContractError(`${label}.status is invalid`); } return { status, @@ -715,7 +719,7 @@ function parseAssignmentRelease( ], label); const action = string(item.action, `${label}.action`); if (action !== "unassigned" && action !== "replaced") { - throw new Error(`${label}.action is invalid`); + throw new TicketBrowserContractError(`${label}.action is invalid`); } return { action, @@ -839,7 +843,7 @@ function parseSourceRefObservation( exact(item, ["status", "code"], label); return { status, code: string(item.code, `${label}.code`) }; } - throw new Error(`${label}.status is invalid`); + throw new TicketBrowserContractError(`${label}.status is invalid`); } function parseTicketMergeRequestSummary( @@ -1085,7 +1089,7 @@ function parseTicketTarget(value: unknown, label: string): TicketTarget { exact(item, ["access", "ref_selector", "repository_key"], label); const access = string(item.access, `${label}.access`); if (access !== "read_only" && access !== "read_write") { - throw new Error(`${label}.access is invalid`); + throw new TicketBrowserContractError(`${label}.access is invalid`); } return { access, @@ -1234,7 +1238,7 @@ export function parseTicketRecordRef(value: unknown): TicketRef { exact(item, ["id", "resource_key", "slug", "status"], label); const status = string(item.status, `${label}.status`); if (status !== "Open" && status !== "Closed") { - throw new Error(`${label}.status is invalid`); + throw new TicketBrowserContractError(`${label}.status is invalid`); } return { id: string(item.id, `${label}.id`), @@ -1502,7 +1506,7 @@ function parseReviewFinding(value: unknown, label: string): ReviewFinding { exact(item, ["body", "code", "line", "path", "severity"], label); const severity = string(item.severity, `${label}.severity`); if (!["blocker", "major", "minor", "note"].includes(severity)) { - throw new Error(`${label}.severity is invalid`); + throw new TicketBrowserContractError(`${label}.severity is invalid`); } return { body: string(item.body, `${label}.body`), @@ -1576,7 +1580,7 @@ function parseMergeRequestThreadEvent( ], label); const decision = string(item.decision, `${label}.decision`); if (!["approve", "request_changes"].includes(decision)) { - throw new Error(`${label}.decision is invalid`); + throw new TicketBrowserContractError(`${label}.decision is invalid`); } return { ...eventBase, @@ -1658,10 +1662,10 @@ function parseMergeRequestThreadEvent( const strategy = string(item.strategy, `${label}.strategy`); const resolution = string(item.resolution, `${label}.resolution`); if (!["fast_forward", "merge"].includes(strategy)) { - throw new Error(`${label}.strategy is invalid`); + throw new TicketBrowserContractError(`${label}.strategy is invalid`); } if (!["none", "clean", "conflicts_resolved"].includes(resolution)) { - throw new Error(`${label}.resolution is invalid`); + throw new TicketBrowserContractError(`${label}.resolution is invalid`); } return { ...eventBase, @@ -1688,7 +1692,7 @@ function parseMergeRequestThreadEvent( ), }; } - throw new Error(`${label}.kind is invalid`); + throw new TicketBrowserContractError(`${label}.kind is invalid`); } function parseMergeRequestRef( @@ -1743,7 +1747,7 @@ export function parseMergeRequestDetailResponse( ], label); const state = string(item.state, `${label}.state`); if (!["open", "merged", "closed"].includes(state)) { - throw new Error(`${label}.state is invalid`); + throw new TicketBrowserContractError(`${label}.state is invalid`); } return { created_at: string(item.created_at, `${label}.created_at`), diff --git a/web/workspace/src/lib/workspace/navigation/DeferredPage.svelte b/web/workspace/src/lib/workspace/navigation/DeferredPage.svelte index e69690534..9779b65ec 100644 --- a/web/workspace/src/lib/workspace/navigation/DeferredPage.svelte +++ b/web/workspace/src/lib/workspace/navigation/DeferredPage.svelte @@ -13,6 +13,31 @@ let resolved = $state.raw<{ path: string; value: ResolvedPageData } | null>(null); let failure = $state.raw<{ path: string; value: DeferredFailure } | null>(null); let loading = $state(true); + let renderFailure = $state.raw<{ path: string; error: unknown } | null>(null); + let recoveryVersion = $state(0); + let retrying = $state(false); + let retryFailure = $state(null); + const errorMessage = (error: unknown) => { + // SvelteKit may transform render exceptions into a serialized App.Error. + if (error !== null && typeof error === 'object' && 'message' in error && typeof error.message === 'string') { + return error.message; + } + return String(error); + }; + + async function retry() { + if (retrying || loading) return; + const requestedPath = path; + retrying = true; + retryFailure = null; + try { + await invalidateAll(); + } catch (error) { + if (path === requestedPath) retryFailure = errorMessage(error); + } finally { + retrying = false; + } + } const path = $derived(identity ?? page.url.pathname); const current = $derived(resolved?.path === path ? resolved.value : null); const currentFailure = $derived(failure?.path === path ? failure.value : null); @@ -23,9 +48,14 @@ let active = true; loading = true; failure = null; + retryFailure = null; void resolvePageData(requestedData).then(async (result) => { if (!active) return; if (result.ok) { + // Never reset a failed boundary against stale data, including while Retry + // invalidation is pending. Remount only after this load has fresh data. + if (renderFailure?.path === requestedPath) recoveryVersion += 1; + renderFailure = null; resolved = { path: requestedPath, value: result.data }; loading = false; } else if (result.failure.redirect) { @@ -51,7 +81,7 @@ - {#if currentFailure}{title} · Unavailable + {#if currentFailure || renderFailure?.path === path}{title} · Error {:else if !current}{title} · Loading{/if} @@ -59,11 +89,26 @@

{title}

{currentFailure.message}

- + {#if retryFailure}

{retryFailure}

{/if} +
{:else if current} {#if loading}

Updating…

{/if} - {#key path}{@render children(current)}{/key} + {#key path} + {#key recoveryVersion} + { renderFailure = { path, error }; }}> + {@render children(current)} + {#snippet failed(error)} +
+

{title}

+

{errorMessage(error)}

+ {#if retryFailure}

{retryFailure}

{/if} + +
+ {/snippet} +
+ {/key} + {/key} {:else}

{title}

diff --git a/web/workspace/src/lib/workspace/navigation/DeferredPageHarness.svelte b/web/workspace/src/lib/workspace/navigation/DeferredPageHarness.svelte new file mode 100644 index 000000000..f488da41d --- /dev/null +++ b/web/workspace/src/lib/workspace/navigation/DeferredPageHarness.svelte @@ -0,0 +1,31 @@ + + + +
Valid Workspace
+ +
+ + {#snippet children(content)} + + {/snippet} + +
+ {#snippet failed()} +

Workspace unavailable

+

Workspace removed

+ + {/snippet} +
diff --git a/web/workspace/src/lib/workspace/navigation/DeferredPageTestContent.svelte b/web/workspace/src/lib/workspace/navigation/DeferredPageTestContent.svelte new file mode 100644 index 000000000..60da1b2e2 --- /dev/null +++ b/web/workspace/src/lib/workspace/navigation/DeferredPageTestContent.svelte @@ -0,0 +1,19 @@ + + +

{content.label}

+ diff --git a/web/workspace/src/lib/workspace/navigation/deferred-page.browser.spec.ts b/web/workspace/src/lib/workspace/navigation/deferred-page.browser.spec.ts index d514afa01..43cc05feb 100644 --- a/web/workspace/src/lib/workspace/navigation/deferred-page.browser.spec.ts +++ b/web/workspace/src/lib/workspace/navigation/deferred-page.browser.spec.ts @@ -1,8 +1,9 @@ // @vitest-environment happy-dom -import { cleanup, fireEvent, render, waitFor } from "@testing-library/svelte"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/svelte"; import { afterEach, expect, test, vi } from "vitest"; import { createRawSnippet, tick } from "svelte"; import DeferredPage from "./DeferredPage.svelte"; +import DeferredPageHarness from "./DeferredPageHarness.svelte"; import { type DeferredResult, settle } from "./deferred.ts"; const navigation = vi.hoisted(() => ({ @@ -19,6 +20,7 @@ afterEach(() => { cleanup(); vi.clearAllMocks(); navigation.to = null; + navigation.invalidateAll.mockImplementation(async () => {}); }); function gate() { let resolve!: (value: T) => void; @@ -141,3 +143,148 @@ test("old canonical redirect cannot overtake a navigation still loading its rout await tick(); expect(navigation.goto).not.toHaveBeenCalled(); }); + +function expectWorkspacePreserved() { + expect(screen.getByText("Valid Workspace")).toBeTruthy(); + expect(screen.getByRole("complementary", { name: "Workspace sidebar" })).toBeTruthy(); + expect(screen.queryByText("Workspace unavailable")).toBeNull(); + expect(screen.queryByText("Workspace removed")).toBeNull(); + expect(screen.queryByRole("button", { name: "Create Workspace" })).toBeNull(); +} + +const renderError = 'Ticket lookup failed: '; +const brokenData = () => ({ deferredPage: settle({ label: "Broken ticket", message: renderError }) }); + +test("child constructor error stays local, with the actual message escaped below the page title", async () => { + const view = render(DeferredPageHarness, { data: brokenData(), identity: "team:ticket-a" }); + const alert = await view.findByRole("alert"); + expect(view.getByRole("heading", { name: "Ticket" })).toBeTruthy(); + expect(alert.textContent).toBe(renderError); + expect(alert.querySelector("img")).toBeNull(); + expect(view.container.querySelector("main h1 + p")).toBe(alert); + expect(view.queryByLabelText("Draft")).toBeNull(); + expectWorkspacePreserved(); +}); + +test("serialized SvelteKit App.Error displays its message, not object coercion", async () => { + const view = render(DeferredPageHarness, { + data: { deferredPage: settle({ label: "Broken ticket", message: renderError, serializedError: true }) }, + identity: "team:ticket-a", + }); + const alert = await view.findByRole("alert"); + expect(alert.textContent).toBe(renderError); + expect(alert.querySelector("img")).toBeNull(); + expectWorkspacePreserved(); +}); + +test("Retry waits for new successful deferred data before remounting a failed child", async () => { + const onConstruct = vi.fn(); + const view = render(DeferredPageHarness, { data: brokenData(), identity: "team:ticket-a", onConstruct }); + await view.findByRole("alert"); + const invalidation = gate(); + navigation.invalidateAll.mockImplementationOnce(() => invalidation.promise); + await fireEvent.click(view.getByRole("button", { name: "Retry" })); + expect(navigation.invalidateAll).toHaveBeenCalledOnce(); + expect(onConstruct).toHaveBeenCalledTimes(1); + expect((view.getByRole("button", { name: "Retry" }) as HTMLButtonElement).disabled).toBe(true); + + const refresh = gate>(); + await view.rerender({ data: { deferredPage: refresh.promise }, identity: "team:ticket-a", onConstruct }); + invalidation.resolve(); + await tick(); + expect(view.getByRole("alert").textContent).toBe(renderError); + expect(onConstruct).toHaveBeenCalledTimes(1); + expect((view.getByRole("button", { name: "Retry" }) as HTMLButtonElement).disabled).toBe(true); + expectWorkspacePreserved(); + + refresh.resolve({ ok: true, data: { label: "Recovered ticket" } }); + await view.findByText("Recovered ticket"); + expect(onConstruct).toHaveBeenCalledTimes(2); + expect(view.queryByRole("alert")).toBeNull(); + expect(view.queryByRole("status")).toBeNull(); + expect(view.getByLabelText("Draft")).toBeTruthy(); + expectWorkspacePreserved(); +}); + +test("invalidation without new data never resets the boundary against stale failed data", async () => { + const onConstruct = vi.fn(); + const view = render(DeferredPageHarness, { data: brokenData(), identity: "team:ticket-a", onConstruct }); + await view.findByRole("alert"); + await fireEvent.click(view.getByRole("button", { name: "Retry" })); + await tick(); + expect(onConstruct).toHaveBeenCalledTimes(1); + expect(view.getByRole("alert").textContent).toBe(renderError); + expectWorkspacePreserved(); +}); + +test("failed replacement data remains local and another fresh result can recover", async () => { + const onConstruct = vi.fn(); + const view = render(DeferredPageHarness, { data: brokenData(), identity: "team:ticket-a", onConstruct }); + await view.findByRole("alert"); + await view.rerender({ + data: { deferredPage: settle({ label: "Still broken", message: "Ticket still missing" }) }, + identity: "team:ticket-a", onConstruct, + }); + await waitFor(() => expect(view.getByRole("alert").textContent).toBe("Ticket still missing")); + expect(onConstruct).toHaveBeenCalledTimes(2); + expectWorkspacePreserved(); + await view.rerender({ data: { deferredPage: settle({ label: "Recovered" }) }, identity: "team:ticket-a", onConstruct }); + await view.findByText("Recovered"); + expect(view.queryByRole("alert")).toBeNull(); +}); + +test("changing route identity clears a failed boundary and rejects late cross-workspace results", async () => { + const onConstruct = vi.fn(); + const view = render(DeferredPageHarness, { data: brokenData(), identity: "team:ticket-a", onConstruct }); + await view.findByRole("alert"); + const stale = gate>(); + await view.rerender({ data: { deferredPage: stale.promise }, identity: "team:ticket-a", onConstruct }); + const workspace = gate>(); + const destination = gate>(); + await view.rerender({ + data: { deferredWorkspace: workspace.promise, deferredPage: destination.promise }, + identity: "other-team:ticket-b", onConstruct, + }); + expect(view.queryByRole("alert")).toBeNull(); + expect(view.getByRole("status").textContent).toBe("Loading…"); + expect(view.queryByLabelText("Draft")).toBeNull(); + destination.resolve({ ok: true, data: { label: "Other Workspace ticket" } }); + stale.resolve({ ok: true, data: { label: "Stale team ticket" } }); + await tick(); + expect(onConstruct).toHaveBeenCalledTimes(1); + expect(view.queryByText("Stale team ticket")).toBeNull(); + workspace.resolve({ ok: true, data: { workspace: "other-team" } }); + await view.findByText("Other Workspace ticket"); + expect(onConstruct.mock.calls.map(([label]) => label)).toEqual(["Broken ticket", "Other Workspace ticket"]); + expect(view.queryByRole("alert")).toBeNull(); + expectWorkspacePreserved(); +}); + +test("a failed deferred Retry never reconstructs the old failed child", async () => { + const onConstruct = vi.fn(); + const view = render(DeferredPageHarness, { data: brokenData(), identity: "team:ticket-a", onConstruct }); + await view.findByRole("alert"); + await view.rerender({ + data: { deferredPage: Promise.resolve({ ok: false, failure: { message: "Ticket access denied", status: 403, redirect: null } }) }, + identity: "team:ticket-a", onConstruct, + }); + await waitFor(() => expect(view.getByRole("alert").textContent).toBe("Ticket access denied")); + expect(onConstruct).toHaveBeenCalledTimes(1); + expect(view.queryByLabelText("Draft")).toBeNull(); + expectWorkspacePreserved(); + await view.rerender({ data: { deferredPage: settle({ label: "Authorized ticket" }) }, identity: "team:ticket-a", onConstruct }); + await view.findByText("Authorized ticket"); + expect(onConstruct).toHaveBeenCalledTimes(2); + expect(view.queryByRole("alert")).toBeNull(); +}); + +test("a failed Retry request is handled locally without losing the original render error", async () => { + const view = render(DeferredPageHarness, { data: brokenData(), identity: "team:ticket-a" }); + await view.findByRole("alert"); + navigation.invalidateAll.mockRejectedValueOnce(new Error("Reload failed")); + await fireEvent.click(view.getByRole("button", { name: "Retry" })); + await view.findByText("Reload failed"); + expect(view.getByText(renderError)).toBeTruthy(); + expect((view.getByRole("button", { name: "Retry" }) as HTMLButtonElement).disabled).toBe(false); + expectWorkspacePreserved(); +}); diff --git a/web/workspace/src/routes/w/[workspaceId]/+error.svelte b/web/workspace/src/routes/w/[workspaceId]/+error.svelte index b4aff2608..2aae93ee9 100644 --- a/web/workspace/src/routes/w/[workspaceId]/+error.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/+error.svelte @@ -1,23 +1,16 @@ -Workspace unavailable · Yoi +Page error · Yoi -
-
-

Workspace unavailable

-

The selected Workspace cannot be opened

-

- {workspaceId} may have been removed, become inaccessible, or no longer exist on - this Backend. No state from a previously selected Workspace was retained. -

- -
-
+
+

Unable to display this page

+

{page.error?.message ?? "An unexpected error occurred."}

+

Error {page.status}

+
+ + Home +
+
diff --git a/web/workspace/src/routes/w/[workspaceId]/+layout.svelte b/web/workspace/src/routes/w/[workspaceId]/+layout.svelte index 08b752345..237b3198f 100644 --- a/web/workspace/src/routes/w/[workspaceId]/+layout.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/+layout.svelte @@ -91,13 +91,13 @@ /> - {#if workspaceError}Workspace · Unavailable + {#if workspaceError}Workspace · Load error {:else if !workspace}Workspace · Loading{/if} {#if workspaceError}
-

Workspace unavailable

+

Workspace details could not be loaded

{workspaceError}

diff --git a/web/workspace/src/routes/w/[workspaceId]/+layout.ts b/web/workspace/src/routes/w/[workspaceId]/+layout.ts index bb7ec0ebd..69e9eb9c1 100644 --- a/web/workspace/src/routes/w/[workspaceId]/+layout.ts +++ b/web/workspace/src/routes/w/[workspaceId]/+layout.ts @@ -25,9 +25,13 @@ async function loadWorkspace(fetch: Parameters[0]["fetch"], workspac const workspace = workspaceResult.data; const workspaceError = workspaceResult.error; if (!workspace) { - error(404, { - message: workspaceError ?? `Workspace ${workspaceId} is unavailable`, - }); + const message = workspaceError ?? "Workspace details could not be loaded"; + const status = workspaceResult.status; + if (status !== undefined && status >= 400 && status <= 599) { + error(status, message); + } + // Transport/decoding failures do not prove that the Workspace was removed. + throw new Error(message); } const repositories = repositoryResult.data; diff --git a/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts b/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts index ad3b3121b..e0f8f2bd2 100644 --- a/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts +++ b/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts @@ -1,4 +1,4 @@ -import { redirect } from "@sveltejs/kit"; +import { error, redirect } from "@sveltejs/kit"; import { loadJson, workspaceApiPath } from "#lib/workspace/api/http.ts"; import { loadWorkspaceRepositoryList } from "#lib/workspace/api/repositories.ts"; import { parseBrowserWorkspaceOrchestratorResponse } from "#lib/workspace/api/workers.ts"; @@ -36,6 +36,14 @@ export const load = deferPage(async ({ fetch, params }: Parameters[0]) parseBrowserWorkspaceOrchestratorResponse, ), ]); + if (!ticket.data) { + const message = ticket.error ?? "Ticket details could not be loaded"; + if (ticket.status !== undefined && ticket.status >= 400 && ticket.status <= 599) { + error(ticket.status, message); + } + // Expected read failures belong to the deferred loader, not component construction. + throw new Error(message); + } if (ticket.data) { const canonical = canonicalResourceReference( ticket.data.resource_key, diff --git a/web/workspace/src/routes/w/[workspaceId]/workspace-error.browser.spec.ts b/web/workspace/src/routes/w/[workspaceId]/workspace-error.browser.spec.ts new file mode 100644 index 000000000..272a06a35 --- /dev/null +++ b/web/workspace/src/routes/w/[workspaceId]/workspace-error.browser.spec.ts @@ -0,0 +1,58 @@ +// @vitest-environment happy-dom +import { cleanup, fireEvent, render } from "@testing-library/svelte"; +import { afterEach, expect, test, vi } from "vitest"; +import WorkspaceError from "./+error.svelte"; +import { load } from "./+layout.ts"; + +const state = vi.hoisted(() => ({ + page: { params: { workspaceId: "workspace-a" }, status: 500, error: { message: "Ticket Browser API response is invalid" } }, +})); +vi.mock("$app/state", () => state); +afterEach(() => { cleanup(); vi.restoreAllMocks(); }); + +test("a child page error preserves its message and never claims Workspace deletion", async () => { + const reload = vi.spyOn(window.location, "reload").mockImplementation(() => {}); + const view = render(WorkspaceError); + expect(view.getByRole("alert").textContent).toBe("Ticket Browser API response is invalid"); + expect(view.getByText("Error 500")).toBeTruthy(); + expect(view.queryByText("The selected Workspace cannot be opened")).toBeNull(); + expect(view.queryByText("Workspace unavailable")).toBeNull(); + expect(view.queryByText("Create Workspace")).toBeNull(); + expect(view.queryByText("Choose another Workspace")).toBeNull(); + await fireEvent.click(view.getByRole("button", { name: "Reload page" })); + expect(reload).toHaveBeenCalledOnce(); +}); + +async function loadFailure(workspaceFetch: () => Promise) { + const fetcher = vi.fn(async (input: RequestInfo | URL) => { + if (String(input).endsWith("/workspace")) return await workspaceFetch(); + return new Response("repository lookup unavailable", { status: 503 }); + }); + return await load({ fetch: fetcher, params: { workspaceId: "workspace-a" } } as unknown as Parameters[0]).deferredWorkspace; +} + +test.each([401, 403, 404, 500, 503])("Workspace HTTP %i is not rewritten as 404", async (status) => { + const result = await loadFailure(async () => new Response("private backend body", { status })); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected a failed Workspace load"); + expect(result.failure.status).toBe(status); + expect(result.failure.message).toBe(`Workspace API request failed with HTTP ${status}`); + expect(result.failure.message).not.toContain("private backend body"); +}); + +test.each(["network", "json", "contract"] as const)("Workspace %s failures do not imply a missing Workspace", async (kind) => { + const result = await loadFailure(async () => { + if (kind === "network") throw new TypeError("Failed to fetch"); + return kind === "json" ? new Response("not json") : Response.json({}); + }); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected a failed Workspace load"); + expect(result.failure.status).toBeNull(); + expect(result.failure.message).toBe({ + network: "Workspace API request could not be completed", + json: "Workspace API response could not be decoded as JSON", + contract: "Workspace API response is invalid", + }[kind]); + expect(result.failure.message).not.toContain("removed"); + expect(result.failure.message).not.toContain("404"); +});