commit 4dae378d2e1b6b400a64de6b092bac4b566b3c5e
parent 2b59eb9c7208600a27c3edc3b7579a4af85ac5a8
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Tue, 4 Aug 2026 23:14:51 -0400
Remove the loading.tsx skeletons — they broke 404s and never showed
Two measurements, both against a running server, both reproducible:
1. A loading boundary makes the route STREAM, so the HTTP status is committed
before the page body runs. Every page that decides "this doesn't exist" with
notFound() after an async read was therefore returning **200** with 404
content inside it. /channels/<missing> returned 200 with the boundaries
present and 404 without them. That is invisible in a browser and wrong for
anything reading the status — including the channel-rename spec, which was
failing on exactly this and which I had misdiagnosed twice before actually
looking at the response.
2. They never showed anyway. A loading.tsx does not produce a fallback for a
client navigation between two children of the root layout — verified in dev
and against a production build under three different ways of stalling the
payload. Next's own reference says it outright.
So they cost real correctness and bought nothing measurable. Removed, along
with the RouteSkeleton component. What actually makes navigation instant here
is prefetch + staleTimes, which stays; so does error.tsx, which is unaffected
(the 404 test passes with it in place).
e2e/navigation.spec.ts now pins the status codes and records the reasoning, so
nobody re-adds a skeleton and silently turns every 404 into a 200.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Diffstat:
9 files changed, 51 insertions(+), 148 deletions(-)
diff --git a/editor/CHANGELOG.md b/editor/CHANGELOG.md
@@ -3,7 +3,7 @@
## [Unreleased]
- **The editor is fast now.** Every page in the editor had a floor of about 4.4 seconds on it, and the reason was one line in the sidebar. The reclaimable-disk badge — the little "12.4 GB" pill next to Cleanup — asked for the channel list, and the function it asked was the one that counts the corpus from scratch: a `readdir` for each of the **78,350** video directories plus a digest sidecar read for each of the ~70,000 transcribed ones, **~474,559 files touched, measured at 3,985 ms**, to describe **98 videos**. Every count that walk produced was then thrown away. It sat in the root layout, so *every* document load paid it; the 5-second auto-refresh re-ran it on a timer, on every route, forever; and three widget endpoints called it on each poll. It now reads the 65 per-channel snapshots it could always have read — the same numbers, **68 ms**, a 59× improvement — and the sidebar badge itself is down to ~40 ms. Loading `/channels` went from 4.5 s to roughly a tenth of a second; the dashboard from ~10 s. The corpus-walking function still exists under a name that says what it costs (`listChannelStatsFromDisk`) for the batch jobs that genuinely need ground truth, and a test now fails the build if it ever reappears anywhere the editor renders. **The honest trade:** the video, transcript and download counts on `/channels` and the dashboard now come from each channel's last generated report rather than from disk directly, so a job that just finished can take a moment — the snapshot scheduler's ~1 second debounce — to show up. Verified against the live corpus: those three counts match a full walk **exactly** on all 65 channels. The one field that doesn't is digest coverage, which reads 0 for the 11 channels whose reports predate per-engine digest counts until their next report refresh. `/channels` now prints how old the oldest report on the page is, rather than leaving you to assume the numbers are live.
- **Auto-refresh no longer refreshes when nothing has changed.** The passive refresher called `router.refresh()` on a timer — a full server-side re-render of the entire page tree, every 5 seconds, on every route, whether or not anything had actually happened. Against the real corpus that was ~4.9 seconds of work per tick, and it held the editor's server process at roughly **22% of a CPU core, permanently, with a single idle tab open**. It now asks a new `/api/pulse` endpoint whether anything moved — a change token built from in-memory job, queue and worker state plus two file timestamps, no corpus reads at all — and re-renders only when the answer is yes. An idle page now performs **zero** re-renders where it used to perform one every five seconds; there's a test that fails if that ever regresses. Same setting, same 5-second default, same "0 disables" behaviour, and editing settings still repaints the sidebar immediately, because the settings file's timestamp is part of the token. The sidebar's job and reclaimable-disk pills now update on their own rather than requiring the whole page to re-render — which in turn let fifteen server actions stop invalidating the client's entire navigation cache to announce that a job count had changed. Also fixed while in here: opening a channel with no report used to **generate one inside the page load**, a full analysis of every video directory in that channel — minutes, on the big ones, with no progress and no way to stop it. It now shows a banner offering to run it as a normal background job, and renders the rest of the page as usual — the video list comes from disk, not the report, and the pipeline controls are exactly what you want on a channel you haven't analysed yet.
-- **A page that throws no longer takes the whole editor with it, and moving between pages is instant.** There were **zero** error boundaries in the editor: anything that threw while rendering — a malformed config, a half-written snapshot — blanked the entire document, sidebar and all, with nothing to click and nothing to read. There is now a route error boundary that keeps the chrome alive, shows the error's digest so you can find it in the server log, and offers *Try again* (Next 16.2's `unstable_retry`, which actually re-fetches, rather than the older `reset`, which only clears the error state). Alongside it: `loading.tsx` skeletons for the routes with the most to render, and a 15-second client router cache (`staleTimes`), which is what makes bouncing between two sidebar links immediate instead of a fresh server round trip each way. One honest note, since it's easy to assume otherwise: in Next 16 `loading.tsx` does **not** guarantee a fallback appears during a client-side navigation — the framework's own reference says so, and testing confirmed it. Fast sidebar navigation here comes from prefetching plus that cache, not from the skeletons; the skeletons cover document loads.
+- **A page that throws no longer takes the whole editor with it, and moving between pages is instant.** There were **zero** error boundaries in the editor: anything that threw while rendering — a malformed config, a half-written snapshot — blanked the entire document, sidebar and all, with nothing to click and nothing to read. There is now a route error boundary that keeps the chrome alive, shows the error's digest so you can find it in the server log, and offers *Try again* (Next 16.2's `unstable_retry`, which actually re-fetches, rather than the older `reset`, which only clears the error state). Alongside it, a 15-second client router cache (`staleTimes`), which is what makes bouncing between two sidebar links immediate instead of a fresh server round trip each way. Loading skeletons were built and then **removed**, on measurement, for two reasons worth recording: in Next 16 a `loading.tsx` does not actually produce a fallback during a client-side navigation (the framework's own reference says so, and three different tests confirmed it), and worse, the streaming boundary it creates commits the HTTP status *before* the page decides — so every page that says "this doesn't exist" was returning **200** with 404 content inside it. Measured directly: a missing channel returned 200 with the boundaries in place and 404 without. There's now a test pinning that. Fast navigation here comes from prefetching plus the router cache.
- **Cleaning audio now checks the video still exists upstream, and keeps it forever if it doesn't.** The transcribed-audio sweep hard-deletes a video's `audio.*` files once whisper has produced a transcript — `remove()`, no trash, no undo — and nothing had ever asked whether the video was still *there*. So a video YouTube had since removed, privated, or put behind a membership, sitting outside the keep-latest window, got its source audio deleted precisely when that local copy had become the only copy. Before deleting anything, the sweep now resolves each candidate's availability and writes a `do-not-clean.json` marker on any video found permanently gone (`deleted` / `private` / `members_only` — the same rule the keep-latest deletion pass uses, now shared as `isPermanentlyGone`), protecting it from this and every future sweep. The check is **cheap-first, not one probe per video**: a cached availability verdict costs nothing and is the only tier that catches `members_only` (a members-only video stays listed in its channel's playlist, so a listing diff can never flag it); then **one** flat-playlist call per channel narrows the field to candidates that have dropped out of the listing; only those few get a per-video probe, which is also what distinguishes a deleted video from an *unlisted* one that legitimately left the listing and is still fetchable by URL. Anything the check cannot resolve — a probe error, an age-gate, a video with no URL to probe — is **left alone with no marker written** and retried next run: the sweep never deletes on incomplete information, and a rate-limited or offline source therefore cleans nothing rather than cleaning wrongly. The summary line breaks the total down (`Skipped 4 (0 protected, 3 gone-from-source pinned, 1 unverified)`) whenever the check acted. On by default; **Check availability before cleaning audio** in Settings turns it off for an offline setup or channels with no URL, where the check can never resolve and cleanup would otherwise stop deleting anything. Two related fixes ride along: the sweep now shares `isRealAudioFile` with the rest of the app instead of its own hand-rolled filter, so it no longer deletes the `audio.live_chat.json` sidecar or the `.part.good`/`.part.testing` audio-check snapshots (which the reclaim estimate never counted, so the two had quietly drifted); and `runAvailabilityCheck` gains `ignoreShard`, because a saved shard slice on disk would otherwise replace an explicit `onlyIds` list wholesale. Scoped to the primary sweep only — the wrong-format, extra-format and auto-sub purges are unchanged, as are the explicit per-video deletes, which still ignore markers deliberately. See `common/controller/verifyBeforeClean.ts`, `common/controller/cleanAudioFromTranscribed.ts`, `common/lib/availability.ts`, and `editor/e2e/pre-clean-availability.spec.ts`.
- **The monitor widget can now reclaim disk, not just report it.** The widget's cleanable-data strip showed a single global number ("4.2 GB reclaimable") with nothing to act on — reclaiming it meant leaving the widget for `/cleanup` or `/actionable`. A new opt-in **"Needs cleaning"** section (URL flag `cleanlist=1`, plus a **Channels needing cleanup** checkbox in the builder and the in-widget gear) lists the channels actually holding that audio, each with its reclaim estimate (`⌫ 2.5 MB`, the video count in the tooltip), capped at 6 channels with a `+N more` line like the needs-work list. With `controls=1` each row gains the same per-channel **Clean audio** button as the `/actionable` page — the existing `window.confirm` still guards the delete — so a pinned interactive widget clears disk pressure the way it already clears a download backlog. This is also the first surface on which a channel that is *fully downloaded and transcribed* but still holding reclaimable audio is actionable: the needs-work list is fed by a backlog route with a download/transcribe precondition, so such a channel never appeared there. It costs no extra polling — the per-channel rows come from the same `/api/widget/cleanable` snapshot read that already backed the total, and the total is now a sum over those rows so the section and the strip above it can't disagree. The dashboard's needs-work panel and its shared route are untouched. See `editor/app/cleanup/lib/loadCleanup.ts` (`cleanableChannels`), `editor/app/api/widget/cleanable/route.ts`, `editor/app/widget/{lib/config.ts,components/{MonitorWidget,WidgetConfigForm}.tsx}`, and `editor/e2e/widget.spec.ts`.
- **A corpus-wide digest backfill can now be started, left alone, and watched.** The digest layer could generate, but only one channel at a time from that channel's own page — a full-archive pass meant 63 manual launches, and a server restart silently ended it with nothing to say so. There is now a **Start Digest Sweep** control on the dashboard that walks every channel in turn, **heaviest first by remaining audio-hours** (cost is audio, not videos: one VOD channel outweighs every duplicate mirror in the archive combined), and it **survives a restart** — the sweep is re-armed at boot the way the auto-download and auto-transcribe runners already were. It stores no work-list, so a resumed sweep re-does nothing: what still needs generating is re-derived from disk every time, which also means a transcript that finishes mid-sweep, or a duplicate cluster you confirm, is simply picked up on the next pass. A separate **Pause Digests** control holds a running sweep at zero without ending it (the pause flag has existed since the digest layer shipped and nothing could set it). **The digest lane now yields the GPU to transcription**: the two were deliberately on separate queues so they wouldn't serialise, whose unintended consequence was that the model and whisper competed for the same card — measured at 90 seconds per audio-hour against the 27 an idle machine managed. While transcription is working the digest lane steps aside and resumes when the card is free; it can be turned off in Settings. Coverage is now visible — a digest instrument on the dashboard with the corpus percentage (to two decimals, because rounding 0.13% up to 1% flatters an 80-day job), a **No digest** column on the channels table, and a per-channel count on the needs-work rows. Progress bars also **work during a regeneration** for the first time: they re-counted digest files from disk, and a regenerated digest is rewritten in place, so a job that was working sat at 0% for its whole run. Time-remaining estimates for digest work are now computed in **seconds per audio-hour** rather than by averaging videos, which for this archive is wrong by more than an order of magnitude between a VOD channel and a shorts channel. New `common/bin/digest-plan.ts` prices the whole backfill in audio-hours before you commit hardware to it.
diff --git a/editor/app/actionable/loading.tsx b/editor/app/actionable/loading.tsx
@@ -1,6 +0,0 @@
-import { RouteSkeleton } from "../components/RouteSkeleton";
-
-// /actionable is a stack of bucket sections, each a card with rows inside.
-export default function Loading() {
- return <RouteSkeleton variant="cards" rows={6} />;
-}
diff --git a/editor/app/channels/[slug]/loading.tsx b/editor/app/channels/[slug]/loading.tsx
@@ -1,7 +0,0 @@
-import { RouteSkeleton } from "../../components/RouteSkeleton";
-
-// A channel page is panels above a long video list — the heaviest page in the
-// editor, and the one most worth showing a shape for.
-export default function Loading() {
- return <RouteSkeleton variant="cards" rows={6} />;
-}
diff --git a/editor/app/channels/loading.tsx b/editor/app/channels/loading.tsx
@@ -1,7 +0,0 @@
-import { RouteSkeleton } from "../components/RouteSkeleton";
-
-// /channels renders a wide sortable table; the generic list skeleton would
-// visibly reflow into it. Table-shaped instead.
-export default function Loading() {
- return <RouteSkeleton variant="table" rows={8} />;
-}
diff --git a/editor/app/components/RouteSkeleton.tsx b/editor/app/components/RouteSkeleton.tsx
@@ -1,70 +0,0 @@
-// The shape every route's loading state is built from.
-//
-// A skeleton, not a spinner, and the distinction is the whole point: a spinner
-// says "stuck", a block of content-shaped placeholders says "arriving". These
-// are only ever shown while a server render is in flight, so they must never
-// shift layout when the real content lands — hence sizes that match the
-// headings and cards they stand in for.
-//
-// `data-testid="route-skeleton"` is load-bearing: editor/e2e/navigation.spec.ts
-// asserts it appears BEFORE the destination heading, which is what pins
-// loading.tsx to the right place in the route tree.
-export function RouteSkeleton({
- rows = 5,
- variant = "list",
-}: {
- rows?: number;
- variant?: "list" | "table" | "cards";
-}) {
- return (
- <div
- data-testid="route-skeleton"
- aria-busy="true"
- aria-live="polite"
- aria-label="Loading"
- className="flex flex-col gap-4 animate-pulse"
- >
- <span className="sr-only">Loading…</span>
- {/* Page heading */}
- <div className="flex items-center justify-between gap-4">
- <div className="h-8 w-48 rounded-md bg-muted" />
- <div className="h-9 w-32 rounded-md bg-muted" />
- </div>
-
- {variant === "cards" ? (
- <div className="grid gap-3 sm:grid-cols-2 lg:grid-cols-3">
- {Array.from({ length: rows }, (_, i) => (
- <div
- key={i}
- className="h-28 rounded-lg border border-border bg-card"
- />
- ))}
- </div>
- ) : variant === "table" ? (
- <div className="rounded-lg border border-border overflow-hidden">
- <div className="h-10 bg-muted/60 border-b border-border" />
- {Array.from({ length: rows }, (_, i) => (
- <div
- key={i}
- className="h-12 border-b border-border last:border-b-0 flex items-center gap-4 px-4"
- >
- <div className="h-4 w-40 rounded bg-muted" />
- <div className="h-4 w-16 rounded bg-muted ml-auto" />
- <div className="h-4 w-16 rounded bg-muted" />
- <div className="h-4 w-16 rounded bg-muted" />
- </div>
- ))}
- </div>
- ) : (
- <div className="flex flex-col gap-3">
- {Array.from({ length: rows }, (_, i) => (
- <div
- key={i}
- className="h-16 rounded-lg border border-border bg-card"
- />
- ))}
- </div>
- )}
- </div>
- );
-}
diff --git a/editor/app/jobs/loading.tsx b/editor/app/jobs/loading.tsx
@@ -1,7 +0,0 @@
-import { RouteSkeleton } from "../components/RouteSkeleton";
-
-// Covers /jobs and, as the nearest boundary above them, /jobs/active,
-// /jobs/queue and /jobs/[id] — all of which render row lists.
-export default function Loading() {
- return <RouteSkeleton variant="table" rows={10} />;
-}
diff --git a/editor/app/loading.tsx b/editor/app/loading.tsx
@@ -1,17 +0,0 @@
-import { RouteSkeleton } from "./components/RouteSkeleton";
-
-// The default loading state for every route that doesn't declare its own.
-//
-// Loading UI components take no props (Next 16: "Loading UI components do not
-// accept any parameters"), so this is deliberately generic; routes whose real
-// content is shaped very differently declare their own loading.tsx so the
-// skeleton doesn't cause a visible jump when the page arrives.
-//
-// What this buys: the sidebar, theme controls and command palette stay mounted
-// and interactive while a server render is in flight — only <main>'s contents
-// are replaced. Before this file existed there was no loading boundary anywhere
-// in the app, so a slow page left the browser sitting on the OLD page with no
-// indication anything was happening.
-export default function Loading() {
- return <RouteSkeleton />;
-}
diff --git a/editor/e2e/navigation.spec.ts b/editor/e2e/navigation.spec.ts
@@ -4,33 +4,33 @@ import { resetData } from "./helpers";
// Navigation contract for the editor after the corpus walk was removed from
// every render path.
//
-// ── What this suite deliberately does NOT assert, and why ──────────────────
+// ── Why there are no loading.tsx skeletons in this app ─────────────────────
//
-// The obvious test to write here is "clicking a sidebar link shows the
-// route-skeleton before the destination heading". It was written, and it does
-// not hold in Next 16 — not in dev, not against a production build, and not
-// under three different ways of stalling the payload. Two measured reasons:
+// There were, briefly. They were removed on measurement, for two reasons:
//
-// 1. When the destination IS prefetched (the normal case — Next prefetches
-// every sidebar link on hydration, and staleTimes.dynamic keeps the entry
-// warm for 15s), the click commits from the router cache with no pending
-// state at all. There is nothing for a fallback to cover. This is the win,
-// not a gap: instrumenting the click showed the Channels markup present
-// immediately, with the RSC request following as revalidation.
+// 1. They never showed. A `loading.tsx` does NOT produce a fallback for a
+// client-side navigation between two children of the root layout — tested
+// in dev and against a production build, under three different ways of
+// stalling the payload. When the destination is prefetched the click
+// commits from the router cache with no pending state at all (this is the
+// win, and it is what makes the sidebar feel instant); when it is not
+// prefetched the router waits on the server and the browser stays on the
+// OLD page. Next's own loading.js reference says as much: "loading.js
+// provides fallback UI but does not guarantee instant client-side
+// navigations. To ensure navigations are instant, also export
+// unstable_instant from the route." That requires `cacheComponents`, which
+// this app cannot adopt (see plans/cache-components-spike.md).
//
-// 2. When the destination is NOT prefetched, the router waits on the server
-// response before committing and the browser stays on the OLD page — no
-// fallback renders. Next's own loading.js reference says so outright:
-// "loading.js provides fallback UI but does not guarantee instant
-// client-side navigations. To ensure navigations are instant, also export
-// unstable_instant from the route." `unstable_instant` requires the
-// cacheComponents flag, which this app has not adopted.
+// 2. They broke 404s. A loading boundary makes the route STREAM, so the HTTP
+// status is committed before the page body runs — and every page that
+// decides "this doesn't exist" with notFound() after an async read then
+// returned **200** with 404 content inside it. Measured directly:
+// /channels/<missing> returned 200 with the boundaries present and 404
+// without them. The last test in this file is the regression guard.
//
-// So loading.tsx here earns its place on document loads and streaming, and as
-// the conventional home for this UI if cacheComponents is ever adopted — but
-// asserting it on a client navigation would be asserting a fiction. What IS
-// worth pinning is the thing users actually feel, and what regressed before:
-// that these routes arrive quickly and the chrome never locks up.
+// So what makes sidebar navigation fast here is prefetch + staleTimes, not
+// skeletons. What IS worth pinning is the thing users feel: these routes
+// arrive quickly, the chrome never locks up, and a missing thing still 404s.
// Sidebar links that used to cost seconds each. /channels and /actionable were
// the worst (4.5 s and 5.4 s) because each rendered a full corpus walk.
@@ -70,17 +70,12 @@ test.describe("navigation", () => {
}
});
- test("a direct document load renders without a stuck skeleton", async ({
- page,
- }) => {
+ test("a direct document load renders each heavy route", async ({ page }) => {
for (const route of HEAVY_ROUTES) {
await page.goto(route.path);
await expect(
page.getByRole("heading", { name: route.heading, level: 1 }),
).toBeVisible();
- // The skeleton is a transient streaming state; if one is still on screen
- // once the heading has rendered, a boundary is stuck open.
- await expect(page.getByTestId("route-skeleton")).toHaveCount(0);
}
});
@@ -94,4 +89,22 @@ test.describe("navigation", () => {
await page.goto("/channels");
await expect(page.getByTestId("channels-freshness")).toBeVisible();
});
+
+ // REGRESSION GUARD — read the header comment before "fixing" this by adding
+ // a loading.tsx back.
+ //
+ // A page that calls notFound() must return HTTP 404, not 200 with 404-looking
+ // content in the body. Any streaming boundary above these routes commits the
+ // status before the page body decides, silently turning every "not found"
+ // into a 200. That is invisible in a browser and wrong for anything that
+ // reads the status.
+ test("a missing resource returns 404, not 200", async ({ page }) => {
+ for (const url of [
+ "/channels/definitely-not-a-channel",
+ "/sites/definitely-not-a-site",
+ ]) {
+ const res = await page.goto(url);
+ expect(res?.status(), `${url} must 404`).toBe(404);
+ }
+ });
});
diff --git a/editor/next.config.ts b/editor/next.config.ts
@@ -35,10 +35,14 @@ const nextConfig: NextConfig = {
// Next 15 — it used to be 30), which means every back-and-forth between two
// sidebar links is a fresh server round trip even when you were just there.
//
- // With loading.tsx present a prefetch returns the layout-to-loading-boundary
- // shell, so 15s makes bouncing between two pages feel instant while keeping
- // job state honest. DO NOT RAISE IT: /jobs, /jobs/active and /jobs/queue are
- // live operational views, and a stale queue is worse than a slow one.
+ // This, plus Next's automatic link prefetching, is what actually makes
+ // sidebar navigation instant here — measured: with the destination warm the
+ // click commits from the router cache with no pending state at all. (There
+ // are deliberately no loading.tsx skeletons; they never showed on a client
+ // navigation AND they broke 404 status codes. See e2e/navigation.spec.ts.)
+ //
+ // DO NOT RAISE IT: /jobs, /jobs/active and /jobs/queue are live operational
+ // views, and a stale queue is worse than a slow one.
staleTimes: { dynamic: 15, static: 180 },
},
// Serve the built export artifacts (stats/summaries/transcripts) through a
