fix(web,server): address three P2 gaps from review

1. Empty-state taxonomy was reading the wrong field. The parent Run
   Detail loader returns status as `run.lifecycleStatus`, not
   `run.status` (apps/fabro-web/app/data/runs.ts:86). resolveRunStatus
   looked for `status` and always fell back to `unknown`, so R4(a)
   starting / R4(c1) failed_before_checkpoint / R4(c2) diff_lost were
   unreachable in the real route. Fixed to read `lifecycleStatus`.

2. Revalidation error state was dead code — the UI rendered
   InlineErrorBanner from `revalidationError` but nothing ever set it
   to non-null. Fixed by changing the loader contract to a
   discriminated union `{ data, error }` that catches Response throws
   and returns them in-band. This lets both initial-load and
   revalidation errors flow through the same render path:
   - Initial load with error + no prior data → inline error render
     (no unmount, no ErrorBoundary trip)
   - Revalidation error with prior data → keep prior data mounted,
     show InlineErrorBanner + Retry
   The plan's intent (§ Unit 11) was specifically "prior content stays
   mounted" on mid-session failures; this finally implements it.

3. Live diff path skipped the planned stream_blob_metadata phase. A
   single malformed blob in --batch output was collapsing the whole
   fetch to an empty map and flagging every file in the response as
   truncated. Two-phase fetch:
   - Phase 1: stream_blob_metadata to identify oversized blobs by
     size before any content fetch.
   - Phase 2: stream_blobs on only the remaining under-cap SHAs.
   A phase-2 parse error now only affects its own SHAs;
   phase-1-classified oversized entries keep their correct
   classification rather than all flipping to undifferentiated
   truncated placeholders.

Refs plan docs/plans/2026-04-19-002-feat-run-files-changed-tab-plan.md

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-19 18:18:54 -04:00
parent e2d258089e
commit ef0cff2b31
No known key found for this signature in database
5 changed files with 290 additions and 166 deletions

View file

@ -35,12 +35,41 @@ import { Toolbar, type DiffStyle } from "./run-files/toolbar";
export const handle = { wide: true };
export async function loader({ request, params }: any) {
const data = await apiJsonOrNull<PaginatedRunFileList>(
`/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.
* This is the plan's "prior content stays mounted; inline banner with
* Retry" behavior for mid-session refresh failures (§ Unit 11).
*/
export type RunFilesLoaderResult = {
data: PaginatedRunFileList | null;
error: { status: number; message: string } | 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) },
};
}
}
// Events that should trigger a revalidation. CheckpointCompleted is the
@ -168,13 +197,25 @@ function decodeDeepLinkFile(hash: string): string | null {
}
}
/** Normalize run status from the parent loader into a lowercase string. */
/**
* Extract the lifecycle status from whichever ancestor match carries it.
* The Run Detail loader (apps/fabro-web/app/routes/run-detail.tsx) returns
* `{ run: { lifecycleStatus: string | null, ... } }` where
* `lifecycleStatus` is the raw workflow status — "submitted", "running",
* "succeeded", "failed", etc. `run.status` on the same object is the
* ColumnStatus derived from checks and is NOT the right field to drive the
* empty-state taxonomy from.
*/
function resolveRunStatus(matches: ReturnType<typeof useMatches>): string | undefined {
for (const match of matches) {
const data = match.data as any;
if (!data) continue;
if (typeof data?.run?.status === "string") return data.run.status as string;
if (typeof data?.status === "string") return data.status as string;
if (typeof data?.run?.lifecycleStatus === "string") {
return data.run.lifecycleStatus as string;
}
if (typeof data?.lifecycleStatus === "string") {
return data.lifecycleStatus as string;
}
}
return undefined;
}
@ -185,50 +226,59 @@ export default function RunFiles({ loaderData }: any) {
const navigation = useNavigation();
const revalidator = useRevalidator();
const matches = useMatches();
const data = loaderData as PaginatedRunFileList | null;
const result = loaderData as RunFilesLoaderResult | null;
const narrow = useNarrowViewport();
const runStatus = resolveRunStatus(matches);
// Preserve the last successful payload so a failed revalidation can keep
// rendering the previous files while surfacing an inline banner. On the
// very first failure (no prior good data), we render the error state
// equivalent of the ErrorBoundary inline.
const lastGoodDataRef = useRef<PaginatedRunFileList | null>(null);
if (result?.data) {
lastGoodDataRef.current = result.data;
}
const data: PaginatedRunFileList | null =
result?.data ?? lastGoodDataRef.current;
const lastFetchedAtRef = useRef<number | null>(null);
const lastToShaRef = useRef<string | null>(null);
const previousDataLengthRef = useRef<number | null>(null);
const [revalidationError, setRevalidationError] = useState<string | null>(
null,
);
const [emptyToast, setEmptyToast] = useState<string | null>(null);
const [deepLinkToast, setDeepLinkToast] = useState<string | null>(null);
useEffect(() => {
if (!result?.data) return;
lastFetchedAtRef.current = Date.now();
const currentToSha = (data?.meta?.to_sha ?? null) as string | null;
const currentToSha = (result.data.meta?.to_sha ?? null) as string | null;
const prevLen = previousDataLengthRef.current;
if (prevLen !== null && prevLen > 0 && (data?.data?.length ?? 0) === 0) {
if (prevLen !== null && prevLen > 0 && result.data.data.length === 0) {
// Revalidation-now-empty toast: the user was looking at files, the
// latest fetch shows none.
setEmptyToast("No changes in this run.");
const id = setTimeout(() => setEmptyToast(null), 3500);
return () => clearTimeout(id);
}
previousDataLengthRef.current = data?.data?.length ?? 0;
previousDataLengthRef.current = result.data.data.length;
lastToShaRef.current = currentToSha;
return undefined;
}, [data]);
}, [result?.data]);
useSseRevalidation(params.id);
const isInitialLoading = navigation.state === "loading" && !loaderData;
const isRevalidating = revalidator.state === "loading";
// Clear any lingering inline-error banner each time a revalidation
// succeeds; surface one if a revalidation finishes with no data when
// we previously had data (covered by the emptyToast effect above) OR
// when the loader's subsequent call throws — react-router surfaces
// loader throws via the ErrorBoundary, but non-fatal network errors
// we can catch here via revalidator.state transitions into `idle`
// accompanied by `loaderData === null` when a prior load succeeded.
useEffect(() => {
if (isRevalidating) setRevalidationError(null);
}, [isRevalidating]);
// Revalidation error is whatever the most recent loader call returned;
// the inline banner renders when we still have prior data to show. When
// there's no prior data AND this is the initial load, we render a
// full-panel error state instead (the Toolbar would have nothing to act
// on with no data).
const revalidationError =
result?.error && lastGoodDataRef.current
? `Couldn't refresh (${result.error.status}).`
: null;
const initialError = result?.error && !lastGoodDataRef.current ? result.error : null;
const freshness = useFreshness(data?.meta ?? null, lastFetchedAtRef.current);
@ -361,6 +411,27 @@ 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).
if (initialError) {
if (initialError.status === 401 || initialError.status === 403) {
return (
<EmptyState kind="unknown" />
);
}
return (
<InlineErrorBanner
message={
initialError.status >= 500
? `Something went wrong (${initialError.status}).`
: `Couldn't load files (${initialError.status}).`
}
onRetry={() => revalidator.revalidate()}
/>
);
}
if (!data) {
return (
<EmptyState
@ -403,10 +474,7 @@ export default function RunFiles({ loaderData }: any) {
{revalidationError ? (
<InlineErrorBanner
message={revalidationError}
onRetry={() => {
setRevalidationError(null);
revalidator.revalidate();
}}
onRetry={() => revalidator.revalidate()}
/>
) : null}
<DegradedBanner reason={meta.degraded_reason} />
@ -455,10 +523,7 @@ export default function RunFiles({ loaderData }: any) {
{revalidationError ? (
<InlineErrorBanner
message={revalidationError}
onRetry={() => {
setRevalidationError(null);
revalidator.revalidate();
}}
onRetry={() => revalidator.revalidate()}
/>
) : null}
{body}

View file

@ -33,7 +33,7 @@ use fabro_sandbox::reconnect::reconnect;
use fabro_types::RunId;
use fabro_workflow::sandbox_git::{
DiffError, RawDiffEntry, SubmoduleChange, SymlinkChange, list_binary_paths,
list_changed_files_raw, stream_blobs,
list_changed_files_raw, stream_blob_metadata, stream_blobs,
};
use futures_util::FutureExt;
use serde::Deserialize;
@ -926,11 +926,26 @@ fn collect_blob_shas(classified: &[ClassifiedEntry]) -> Vec<String> {
out
}
/// Fetch all requested blobs in one batched `cat-file --batch` call. Returns
/// a SHA → contents map where `None` means the blob exceeded the per-file
/// byte cap (caller flags that entry truncated). Permanent errors in the
/// sandbox yield an empty table (the handler falls through to the degraded
/// branch upstream).
/// Fetch blob contents for the `NeedsFetch` entries in two phases: first
/// `cat-file --batch-check` to learn sizes, then `cat-file --batch` on only
/// the blobs that fit under the per-file cap.
///
/// Phase 1 (metadata): cheap, returns sizes reliably; used to pre-filter
/// oversized blobs so phase 2 never pulls them. If a later phase-2 parse
/// error poisons the whole stream, the oversized-by-metadata entries
/// stay correctly classified rather than collapsing to undifferentiated
/// truncated placeholders.
///
/// Phase 2 (contents): bulk `cat-file --batch` on the remaining SHAs.
///
/// Failure modes:
/// - Phase 1 permanent error: fall through with an empty size map; phase 2
/// runs against the full SHA list (current behavior before this split).
/// - Phase 1 transient error: 503 to the client.
/// - Phase 2 permanent error (malformed blob in stream): only the
/// phase-2 SHAs get `None`; phase-1-classified oversized SHAs keep their
/// `None` entries but with a semantically-accurate cause.
/// - Phase 2 transient error: 503 to the client.
async fn fetch_blob_table(
sandbox: &dyn Sandbox,
shas: &[String],
@ -938,13 +953,60 @@ async fn fetch_blob_table(
if shas.is_empty() {
return Ok(HashMap::new());
}
let contents = match stream_blobs(sandbox, shas, PER_FILE_BYTES_CAP).await {
Ok(v) => v,
let mut table: HashMap<String, Option<String>> = HashMap::with_capacity(shas.len());
// Phase 1: --batch-check for sizes.
let oversized: HashSet<String> = match stream_blob_metadata(sandbox, shas).await {
Ok(metas) => {
let mut set = HashSet::new();
for meta in metas {
if let Some(size) = meta.size {
if size > PER_FILE_BYTES_CAP {
set.insert(meta.sha);
}
}
}
set
}
Err(DiffError::Permanent { .. }) => HashSet::new(),
Err(DiffError::Transient { message }) => {
return Err(ApiError::new(
StatusCode::SERVICE_UNAVAILABLE,
format!("Sandbox git cat-file --batch-check failed: {message}"),
));
}
};
// Record oversized blobs in the table as `None` so the caller emits
// `file_too_large` regardless of what phase 2 does.
for sha in &oversized {
table.insert(sha.clone(), None);
}
// Phase 2: --batch for the rest.
let shas_to_fetch: Vec<String> = shas
.iter()
.filter(|sha| !oversized.contains(*sha))
.cloned()
.collect();
if shas_to_fetch.is_empty() {
return Ok(table);
}
match stream_blobs(sandbox, &shas_to_fetch, PER_FILE_BYTES_CAP).await {
Ok(contents) => {
for (sha, content) in shas_to_fetch.iter().zip(contents) {
table.insert(sha.clone(), content);
}
}
Err(DiffError::Permanent { .. }) => {
// Surface nothing; every NeedsFetch entry will resolve to empty
// contents, triggering `file_too_large` on modified/renamed and
// empty sides on added/deleted.
return Ok(HashMap::new());
// Malformed output (e.g. non-UTF-8 blob bytes) — record the
// phase-2 SHAs as unavailable. Oversized-by-metadata entries
// stay correctly marked from the earlier loop.
for sha in shas_to_fetch {
table.entry(sha).or_insert(None);
}
}
Err(DiffError::Transient { message }) => {
return Err(ApiError::new(
@ -952,11 +1014,8 @@ async fn fetch_blob_table(
format!("Sandbox git cat-file --batch failed: {message}"),
));
}
};
let mut table = HashMap::with_capacity(shas.len());
for (sha, content) in shas.iter().zip(contents) {
table.insert(sha.clone(), content);
}
Ok(table)
}

File diff suppressed because one or more lines are too long

View file

@ -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-rc6hmkqw.js"></script>
<script type="module" src="/assets/entry-84st7geq.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>