diff --git a/Cargo.lock b/Cargo.lock index 038a547ff..11d0a6285 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1931,6 +1931,7 @@ name = "fabro-server" version = "0.208.0-nightly.1" dependencies = [ "anyhow", + "async-trait", "axum", "axum-extra", "base64", @@ -1963,6 +1964,7 @@ dependencies = [ "fabro-vault", "fabro-workflow", "futures-util", + "globset", "hex", "hmac", "http-body-util", @@ -1983,11 +1985,13 @@ dependencies = [ "thiserror 2.0.18", "tokio", "tokio-stream", + "tokio-util", "toml 0.8.23", "toml_edit", "tower", "tower-http", "tracing", + "tracing-subscriber", "ulid", "uuid", "walkdir", diff --git a/Cargo.toml b/Cargo.toml index 58d237d59..4be15b04d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,6 +46,7 @@ walkdir = "2" regex = "1" semver = "1" aho-corasick = "1" +globset = "0.4" dirs = "6" mac_address = "1" md5 = "0.7" diff --git a/apps/fabro-web/app/routes/run-detail.tsx b/apps/fabro-web/app/routes/run-detail.tsx index 6a083ab7d..776689534 100644 --- a/apps/fabro-web/app/routes/run-detail.tsx +++ b/apps/fabro-web/app/routes/run-detail.tsx @@ -11,6 +11,7 @@ import type { PreviewUrlResponse } from "@qltysh/fabro-api-client"; const allTabs = [ { name: "Overview", path: "", count: null, demoOnly: false }, { name: "Stages", path: "/stages", count: null, demoOnly: false }, + { name: "Files Changed", path: "/files", count: null, demoOnly: false }, { name: "Graph", path: "/graph", count: null, demoOnly: false }, { name: "Billing", path: "/billing", count: null, demoOnly: false }, ]; diff --git a/apps/fabro-web/app/routes/run-files.test.ts b/apps/fabro-web/app/routes/run-files.test.ts new file mode 100644 index 000000000..649258893 --- /dev/null +++ b/apps/fabro-web/app/routes/run-files.test.ts @@ -0,0 +1,178 @@ +import { afterEach, describe, expect, test } from "bun:test"; + +import { extractRequestId, loader } from "./run-files"; + +type StubResponseInit = { + status: number; + body?: string; + headers?: Record; +}; + +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", () => { + 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"); + }); +}); + +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: "oops" }); + 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); + }); +}); diff --git a/apps/fabro-web/app/routes/run-files.tsx b/apps/fabro-web/app/routes/run-files.tsx index c8da42c63..ebd11df8c 100644 --- a/apps/fabro-web/app/routes/run-files.tsx +++ b/apps/fabro-web/app/routes/run-files.tsx @@ -1,544 +1,580 @@ -import { useCallback, useEffect, useRef, useState } from "react"; import { - MultiFileDiff, - type AnnotationSide, - type DiffLineAnnotation, -} from "@pierre/diffs/react"; + useCallback, + useEffect, + useRef, + useState, + type ReactElement, +} from "react"; +import { + useMatches, + useNavigation, + useParams, + useRevalidator, +} from "react-router"; +import { MultiFileDiff, PatchDiff, Virtualizer } from "@pierre/diffs/react"; import { useTheme } from "../lib/theme"; -import { apiJson } from "../api"; -import type { PaginatedRunFileList } from "@qltysh/fabro-api-client"; +import type { + FileDiff as ApiFileDiff, + PaginatedRunFileList, +} from "@qltysh/fabro-api-client"; +import { + DegradedBanner, + pickPlaceholder, +} from "./run-files/placeholders"; +import { + deriveEmptyKind, + EmptyState, + InlineErrorBanner, + LoadingSkeleton, + renderStatusError, + RunFilesErrorBoundary, + Toast, +} from "./run-files/states"; +import { useFileKeyboardNav } from "./run-files/keyboard"; +import { Toolbar, type DiffStyle } from "./run-files/toolbar"; export const handle = { wide: true }; -export async function loader({ request, params }: any) { - const data = await apiJson(`/runs/${params.id}/files`, { request }); - return data; -} +/** + * Loader return type. Both initial loads and revalidations flow through the + * same discriminated union so a revalidation failure does NOT unmount to + * the route ErrorBoundary — it stays in-band as `{ data: null, error }` + * and the component keeps showing the last-good data with an inline + * banner. + * + * `error.requestId` is extracted from the 500-response body so the UI can + * surface it verbatim ("Request ID: xyz. Contact support.") rather than + * just the bare status code. + */ +export type RunFilesLoaderResult = { + data: PaginatedRunFileList | null; + error: { + status: number; + message: string; + requestId: string | null; + } | null; +}; -const fallbackFiles = [ - { - oldFile: { - name: "src/commands/run.ts", - contents: `import { parseArgs } from "node:util"; -import { loadConfig } from "../config.js"; -import { execute } from "../executor.js"; - -interface RunOptions { - config: string; - dryRun: boolean; -} - -export async function run(argv: string[]) { - const { values } = parseArgs({ - args: argv, - options: { - config: { type: "string", short: "c", default: ".fabro/project.toml" }, - "dry-run": { type: "boolean", default: false }, - }, +export async function loader({ + request, + params, +}: any): Promise { + // 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 } : {}), }); - const opts: RunOptions = { - config: values.config ?? ".fabro/project.toml", - dryRun: values["dry-run"] ?? false, - }; - - const config = await loadConfig(opts.config); - const result = await execute(config, { dryRun: opts.dryRun }); - - if (result.success) { - console.log("Run completed successfully."); - } else { - console.error("Run failed:", result.error); - process.exitCode = 1; + if (response.status === 404 || response.status === 501) { + return { data: null, error: null }; } -} -`, - }, - newFile: { - name: "src/commands/run.ts", - contents: `import { parseArgs } from "node:util"; -import { loadConfig } from "../config.js"; -import { execute } from "../executor.js"; -import { createLogger, type Logger } from "../logger.js"; - -interface RunOptions { - config: string; - dryRun: boolean; - verbose: boolean; -} - -export async function run(argv: string[]) { - const { values } = parseArgs({ - args: argv, - options: { - config: { type: "string", short: "c", default: ".fabro/project.toml" }, - "dry-run": { type: "boolean", default: false }, - verbose: { type: "boolean", short: "v", default: false }, - }, - }); - - const opts: RunOptions = { - config: values.config ?? ".fabro/project.toml", - dryRun: values["dry-run"] ?? false, - verbose: values.verbose ?? false, - }; - - const logger: Logger = createLogger({ verbose: opts.verbose }); - - const config = await loadConfig(opts.config); - logger.debug("Loaded config from %s", opts.config); - - const result = await execute(config, { dryRun: opts.dryRun, logger }); - logger.debug("Execution finished in %dms", result.elapsed); - - if (result.success) { - console.log("Run completed successfully."); - } else { - console.error("Run failed:", result.error); - process.exitCode = 1; + if (response.ok) { + const data = (await response.json()) as PaginatedRunFileList; + return { data, error: null }; } -} -`, - }, - }, - { - oldFile: { - name: "src/logger.ts", - contents: "", - }, - newFile: { - name: "src/logger.ts", - contents: `export interface Logger { - info(message: string, ...args: unknown[]): void; - debug(message: string, ...args: unknown[]): void; - error(message: string, ...args: unknown[]): void; -} -interface LoggerOptions { - verbose: boolean; -} + // 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 + } + } -export function createLogger({ verbose }: LoggerOptions): Logger { return { - info(message, ...args) { - console.log(message, ...args); - }, - debug(message, ...args) { - if (verbose) { - console.log("[debug]", message, ...args); - } - }, - error(message, ...args) { - console.error(message, ...args); + data: null, + error: { + status: response.status, + message: response.statusText || `HTTP ${response.status}`, + requestId: extractRequestId(bodyJson), }, }; } -`, - }, - }, - { - oldFile: { - name: "src/executor.ts", - contents: `import type { Config } from "./config.js"; -interface ExecuteOptions { - dryRun: boolean; -} - -interface ExecuteResult { - success: boolean; - error?: string; -} - -export async function execute( - config: Config, - options: ExecuteOptions, -): Promise { - if (options.dryRun) { - console.log("Dry run — skipping execution."); - return { success: true }; - } - - try { - for (const step of config.steps) { - await step.run(); - } - return { success: true }; - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - return { success: false, error: message }; - } -} -`, - }, - newFile: { - name: "src/executor.ts", - contents: `import type { Config } from "./config.js"; -import type { Logger } from "./logger.js"; - -interface ExecuteOptions { - dryRun: boolean; - logger: Logger; -} - -interface ExecuteResult { - success: boolean; - elapsed: number; - error?: string; -} - -export async function execute( - config: Config, - options: ExecuteOptions, -): Promise { - const start = performance.now(); - - if (options.dryRun) { - options.logger.info("Dry run — skipping execution."); - return { success: true, elapsed: performance.now() - start }; - } - - try { - for (const step of config.steps) { - options.logger.debug("Running step: %s", step.name); - await step.run(); - } - return { success: true, elapsed: performance.now() - start }; - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - return { success: false, elapsed: performance.now() - start, error: message }; - } -} -`, - }, - }, -]; - -interface SteerAnnotation { - fileName: string; - lineNumber: number; - side: AnnotationSide; -} - -function steerKey(fileName: string, side: AnnotationSide, lineNumber: number) { - return `${fileName}:${side}:${lineNumber}`; -} - -function DiffHeaderToggles({ - diffStyle, - onDiffStyleChange, - disableBackground, - onDisableBackgroundChange, -}: { - diffStyle: "split" | "unified"; - onDiffStyleChange: (style: "split" | "unified") => void; - disableBackground: boolean; - onDisableBackgroundChange: (disabled: boolean) => void; -}) { - return ( -
- - -
- ); -} - -function DiffWithSteer({ - oldFile, - newFile, - openSteers, - submittedSteers, - onSteer, - onSubmit, - onCancel, -}: { - oldFile: { name: string; contents: string }; - newFile: { name: string; contents: string }; - openSteers: Map; - submittedSteers: Map; - onSteer: (annotation: SteerAnnotation) => void; - onSubmit: (annotation: SteerAnnotation, text: string) => void; - onCancel: (annotation: SteerAnnotation) => void; -}) { - const [diffStyle, setDiffStyle] = useState<"split" | "unified">("split"); - const [disableBackground, setDisableBackground] = useState(false); - const { theme } = useTheme(); - - const containerRef = useRef(null); - const buttonRef = useRef(null); - const hoveredRef = useRef<{ lineNumber: number; side: AnnotationSide } | null>(null); - const leaveTimeoutRef = useRef(0); - - const showButton = useCallback((lineElement: HTMLElement, lineNumber: number, side: AnnotationSide) => { - clearTimeout(leaveTimeoutRef.current); - hoveredRef.current = { lineNumber, side }; - - const btn = buttonRef.current; - const container = containerRef.current; - if (!btn || !container) return; - - const containerRect = container.getBoundingClientRect(); - const lineRect = lineElement.getBoundingClientRect(); - - btn.style.top = `${lineRect.top - containerRect.top}px`; - btn.style.height = `${lineRect.height}px`; - btn.style.display = "flex"; - }, []); - - const hideButton = useCallback(() => { - leaveTimeoutRef.current = window.setTimeout(() => { - hoveredRef.current = null; - if (buttonRef.current) { - buttonRef.current.style.display = "none"; +/** + * 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; + 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; + 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]; } - }, 100); - }, []); - - return ( -
- - oldFile={oldFile} - newFile={newFile} - options={{ - diffStyle, - disableBackground, - theme: theme === "dark" ? "pierre-dark" : "pierre-light", - lineDiffType: "word", - onLineEnter({ lineNumber, annotationSide, lineElement }) { - showButton(lineElement, lineNumber, annotationSide); - }, - onLineLeave() { - hideButton(); - }, - }} - renderHeaderMetadata={() => ( - - )} - lineAnnotations={buildAnnotationsForFile( - newFile.name, - openSteers, - submittedSteers, - )} - renderAnnotation={(annotation) => { - const meta = annotation.metadata; - if ("text" in meta && meta.text != null) { - return ; - } - return ( - onCancel(meta)} - /> - ); - }} - /> -
clearTimeout(leaveTimeoutRef.current)} - onMouseLeave={() => { - hoveredRef.current = null; - if (buttonRef.current) { - buttonRef.current.style.display = "none"; - } - }} - > - -
-
- ); + } + } + return null; } -function SteerCommentForm({ - annotation, - onSubmit, - onCancel, -}: { - annotation: SteerAnnotation; - onSubmit: (annotation: SteerAnnotation, text: string) => void; - onCancel: () => void; -}) { - const [text, setText] = useState(""); - const textareaRef = useRef(null); +// Events that should trigger a revalidation. CheckpointCompleted is the +// canonical signal; terminal events cover the final-state transitions too. +const REFRESH_EVENTS = new Set([ + "checkpoint.completed", + "run.completed", + "run.failed", +]); +const MD_BREAKPOINT_PX = 768; +const DIFF_STYLE_STORAGE_KEY = "fabro.run-files.diff-style"; + +export const ErrorBoundary = RunFilesErrorBoundary; + +function useNarrowViewport(): boolean { + const [narrow, setNarrow] = useState(false); useEffect(() => { - textareaRef.current?.focus(); + if (typeof window === "undefined") return; + const mql = window.matchMedia(`(max-width: ${MD_BREAKPOINT_PX - 1}px)`); + const apply = () => setNarrow(mql.matches); + apply(); + mql.addEventListener("change", apply); + return () => mql.removeEventListener("change", apply); }, []); - - return ( -
-