Skip to content

Let the Repairs panel report a failure, and stop it reading the whole queue - #118

Open
ergofobe wants to merge 4 commits into
mainfrom
issue-89-repairs-panel
Open

ergofobe wants to merge 4 commits into
mainfrom
issue-89-repairs-panel

Conversation

@ergofobe

Copy link
Copy Markdown
Owner

Closes #89. Closes #94.

The two defects in the Repairs panel, one on each side of the wire.

The web half — #89

The mechanism the issue describes is the one that was there. refetchInterval read data to
decide whether to keep asking; after a failed poll data is undefined, so it returned false,
and with retry: false nothing ever asked again. The render path then folded isError in with
"nothing to show" and returned null. One transient 500 took the Repairs controls off the page
for the life of the tab, with no error and no way back, while a pass might still have been
walking the library.

It now gets the treatment the queue panel directly above it already had — the failure reported
with its reason and a Try again button, and an interval that keeps asking — with three
corrections the review turned up:

  • The banner sits over the list, not instead of it. React Query keeps the last good answer
    through an error, and this only polls while a pass is walking, so replacing the list would have
    blanked the running row and the Start buttons on every blip in the fifteen-second cadence.
  • A 404 gets no exemption. It reads like "this server is too old to have the route", which is
    exactly the wrong inference here: the admin API refuses everything with a plain 404 on purpose,
    being meant to be undiscoverable rather than merely closed. An expired session arrives as the
    same status, and excusing it left a stale list with "Walking the library" still showing and
    nothing asking again — One failed poll hides the Repairs section with no error and no retry #89 by another road. Nothing is lost, because the bundle is served by the
    server it talks to.
  • <Repairs> stays mounted when the queue fails. It was rendered only inside the queue
    query's success branch, so one failed GET /api/v1/admin/queue unmounted it — and a banner
    cannot report anything from a component that is not there.

repairsPollInterval is exported because that function is the defect, and a rendering assertion
cannot see the half of #89 that is "never asks again".

The server half — #94

repairs() selected every queued and running job row to build a Set of two names, and the panel
polls it every fifteen seconds while a pass walks — exactly when a bulk import has tens of
thousands of rows queued. It is now bounded by the two names and grouped, the shape queueHealth
a few lines above already uses.

Measured on a jobs-shaped table of 120,000 rows with only the existing jobs_claim_idx:

rows returned time
before 60,001 12.8 ms
after 1 6.4 ms
with an index on (name, status) 1 0.025 ms

So this removes the part that was actually costly — 60,000 rows crossing the wire and being
materialised into JavaScript objects — and leaves the scan, which is a separate trade-off about
indexing a write-hot table. See Found, not fixed.

Tests

Written first, each watched to fail:

  • admin.test.ts — "asks about the two repair jobs rather than reading every queued row". Drives
    repairs() through a drizzle instance with a query logger and asserts the SQL's parameters carry
    both repair job names, alongside the answer still being right with 200 unrelated rows queued. A
    right answer bought by dragging the whole queue into memory is the defect, and no assertion about
    state can see it.
  • adminProcessing.dom.test.tsx — five: the section says so rather than vanishing; it offers a way
    to ask again and comes back; the interval keeps asking after a failure; a 404 is reported like any
    other failure; the list survives both a later failed poll and a failed poll of the queue above it.

The file's stubs now reset in an afterEach rather than on each test's last line — the line that
does not run when an assertion above it throws.

Verify

bun install && bun run verify && bun run --filter '@imogen/web' build — green. 970 pass, 0 fail.
No bun.lock diff; the SDK pin is untouched at v0.5.1.

Not clean — three review iterations spent

Per CLAUDE.md this is left open rather than self-merged. code-review at medium was run four
times and fixed after three; the fourth run is not clean. Outstanding:

  • significant — AdminProcessing.tsx, the remount. <Repairs> sits at child index 1 in the
    queue-error branch and index 2 in the success branch, so React reconciles positionally and
    unmounts and remounts it on every queue error↔success transition. An intermittently failing queue
    polls every three seconds, and each flip discards the start mutation's error message and
    triggers a fresh repairs refetch — two whole-table counts over assets, one on a JSON field no
    index serves, which is the load The Repairs panel reads every queued job row, every fifteen seconds, to check two names #94 was tightening. The suggested fix is to hoist the conditional
    queue section into one stable slot so Repairs never changes position. It needs a test of its
    own, which is what the budget ran out before writing.
  • nit — the error banner's disabled={isFetching}. While the query is polling in its error
    state the button spends most of its time as a disabled "Asking", for a request that may take the
    full 45s backstop. Noted rather than fixed because it is a pattern: the queue panel's button
    above is the same code, and changing one would put them out of step.
  • nit, pre-existing — the queue branch is if (isError || !data), so a failed queue poll
    replaces the whole panel even though the last good data is still held. Arguably deliberate
    (A wedged database pool stops every background job silently, and the admin Processing screen shows a skeleton for ever #71's "a skeleton here would be a lie"), which is why no issue was filed, but it is the thing
    that makes the remount above possible.

Found, not fixed

🤖 Generated with Claude Code

ergofobe and others added 4 commits September 15, 2026 23:11
… queue

Two defects in the same panel, one on each side of the wire.

The web half (#89): `retry: false` was right, but the refetch interval read
`data` to decide whether to keep asking. After a failed poll `data` is
undefined, so the interval was `false`, nothing ever asked again, and the
render path folded `isError` in with "nothing to show" and returned null. One
transient 500 took the Repairs controls off the page for the life of the tab,
with no error and no way back — while a pass might still have been walking the
library. It now gets the treatment the queue panel directly above it already
had: an error state with the reason and a Try again button, and an interval
that keeps asking. The one silence kept is a 404, which is a server that has
never heard of repairs rather than one that could not answer.

The server half (#94): `repairs()` selected every queued and running job row to
build a Set of two names, and the panel polls it every fifteen seconds while a
pass walks — exactly when a bulk import has tens of thousands of rows queued.
Bounded by the two names and grouped, the shape `queueHealth` a few lines above
already uses.

The repairs query test asserts what is asked of Postgres, not only the answer
that comes back: a right answer bought by dragging the whole queue into memory
is the defect, and no assertion about `state` can see it.

Closes #89
Closes #94

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous commit. React Query keeps the last good answer through
an error, and this query only polls while a pass is walking — so treating any
error as "nothing to show" swapped the live list, the "Walking the library" row
and the Start buttons for a red box on every blip in the fifteen-second
cadence, and swapped them back on the next. The banner now sits above whatever
was last read; the standalone box is what a first failure still gets.

The dom test's stubs go back to their defaults in an `afterEach` rather than on
each test's last line — the line that does not run when an assertion above it
throws, which turned one failure into a run of unrelated ones.

And the new query's comment no longer implies the read is bounded. It bounds
what comes back; nothing indexes `jobs.name`, so the scan remains. Measured at
120,000 rows: 12.8ms and 60,001 rows across the wire before, 6.4ms and one row
now, 0.025ms with an index on `(name, status)`. Indexing a write-hot table to
save a read made four times a minute is its own trade-off, filed as #114.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous commit. The 404 exemption read as "this server is too
old to have the route", which is exactly the wrong inference to draw *here*:
the admin API answers every refusal with a plain 404 on purpose, being meant to
be undiscoverable rather than merely closed. So a session that expires while
the tab is open arrives as the same status, and the exemption turned that into
a stale list with "Walking the library" still showing, no banner, and nothing
asking again — #89 reached by another road.

Dropping it also puts this back in step with the queue panel above, which has
never treated a 404 as anything but a failure. Nothing is lost: the bundle is
served by the server it talks to, so the older-server case the exemption was
written for cannot arise from a browser that loaded this code.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous commit. `<Repairs>` was rendered only inside the queue
query's success branch, so one failed `GET /api/v1/admin/queue` unmounted it —
#89 again through the other door, and a banner cannot report anything from a
component that is not there. Worse during a pass than at rest: a queued repair
job puts the queue's own interval down to three seconds, so any one of twenty
polls a minute took the repair controls off the screen until the next
succeeded. The queue's error section is now a sibling of the repairs rather
than a replacement for them.

Two smaller things from the same review. The poll-interval comment claimed the
interval heals the panel for an administrator who has walked away;
`refetchIntervalInBackground` is false by default, so the timer does not fire
while the tab is behind and it is `refetchOnWindowFocus` that catches them up
on return. And the dom test's `afterEach` now also clears `started`, which was
the one piece of fixture state that actually accumulated across tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant