Skip to content

fix(web): don't show the previous entity's data when the query changes (audit M18) - #450

Merged
passcod merged 5 commits into
mainfrom
claude/pr-370-fix-m18-useapi-identity
Aug 1, 2026
Merged

fix(web): don't show the previous entity's data when the query changes (audit M18)#450
passcod merged 5 commits into
mainfrom
claude/pr-370-fix-m18-useapi-identity

Conversation

@passcod

@passcod passcod commented Aug 1, 2026

Copy link
Copy Markdown
Member

Fixes M18 (medium) from the audit in #370.

The bug

useApi keeps prior data on screen instead of flipping to loading, so a background refetch doesn't collapse the page to a placeholder. But the check was just prev.status === "ok" — no regard for whether that data belongs to the query now being made.

Detail routes reuse the mounted component across /servers/A/servers/B, so A's name, health chip, checks and page title render under B's URL, with no loading indicator, for the whole fetch. On a slow connection that's seconds of confidently-wrong data about a different server — the failure mode gives the user no reason to distrust it.

The fix

Prior data is kept only when deps are unchanged — an actual refetch of the same query. A different identity goes to loading. deps arrays are new objects each render, so the comparison is element-wise Object.is.

VersionDetail's StatusControl also gets an explicit key={versionStr}. It seeds useState from detail.status once, so a reused instance carried the previous version's status into the new one with the Change button enabled — submitting would have written the stale status onto the new version. The parent's loading branch now unmounts it anyway, but the key makes that guarantee local rather than a side effect of how the parent happens to render.

Tests

private-web/src/api.test.tsx, using @testing-library/react's renderHook with a gated fetch stub so the window between "deps changed" and "new data arrived" is deterministic — no network throttling needed, which is what the audit rated hard.

  • goes to loading when deps change to a different entity — confirmed to fail against the unfixed hook, which stays ok with server-1's data while identity B is in flight.
  • keeps prior data across a reload of the same query — passes both ways; pins the behaviour the original code was there for.

Adds the vitest CI step and just test-web, since vitest was configured but never run in CI. That's the same hunk as the M17 branch (#443) — whichever lands first, the other resolves trivially.


Generated by Claude Code

claude added 2 commits August 1, 2026 17:13
`useApi` keeps prior data on screen instead of flipping to `loading`, so a
background refetch doesn't collapse the page to a placeholder. But the check
was just "do we have data?", with no regard for whether it belongs to the
query now being made.

Detail routes reuse the mounted component across /servers/A → /servers/B, so
A's name, health chip, checks and page title render under B's URL — with no
loading indicator to say otherwise — for the whole fetch. On a slow
connection that's seconds of confidently-wrong data about a different server.

Prior data is now kept only when `deps` are unchanged, i.e. an actual
refetch of the same query. A different identity goes to `loading` as it
should.

VersionDetail's StatusControl gets an explicit `key`: it seeds `useState`
from `detail.status` once, so a reused instance carried the previous
version's status into the new one with the Change button enabled — and
submitting would have written it there. The parent's loading branch now
unmounts it anyway, but the key makes that guarantee local instead of
incidental.

Adds the vitest CI step and `just test-web` (same hunk as the M17 branch —
whichever lands first, the other resolves trivially).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGfH1cdFKPnKpM7ytRThft
The first cut treated any `deps` change as "a different entity" and blanked
the page. But callers force a refetch by bumping a nonce inside `deps` —
DeviceDetail's `[id, tick]` — which is the *same* entity, so that collapsed
the subtree to a spinner on every manual refresh and unmounted anything
holding local state inside it.

Caught by the Playwright suite: provisioning a device credential unmounted
the open dialog mid-flow, losing the minted key, so "Download key file" never
appeared. Verified against main — that test passes there and failed 3/3 with
the deps comparison.

What actually decides "which entity is this" is the module, the function and
the params, so compare those by value. A nonce in `deps` no longer counts as
a new entity; /servers/A → /servers/B still does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGfH1cdFKPnKpM7ytRThft

passcod commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

CI caught a real regression in the first cut — pushed a fix in cccdf27.

provision-credential.spec.ts:44 failed because comparing the whole deps array was too strict. Callers force a refetch by bumping a nonce inside depsDeviceDetail uses [id, tick] — which is the same entity, so my check read it as a new one, collapsed the subtree to <LinearProgress />, and unmounted ProvisionCredentialDialog mid-flow. The dialog holds the minted key in local state, so "Download key file" never appeared.

Verified it was mine and not flake: the test passes on main's api.ts and failed 3/3 with the deps comparison.

The fix compares what actually identifies the query — module, function, and params by value — so a refetch nonce isn't a new entity, while /servers/A/servers/B still is. Added keeps prior data when only a refetch nonce in deps changes to pin the idiom.

Locally: 3/3 vitest, and the full Playwright suite green at 227 passed.


Generated by Claude Code

claude added 3 commits August 1, 2026 21:07
…-useapi-identity

# Conflicts:
#	private-web/src/api.ts
Both this branch and #443 add the same `just test-web`, with different
comments — so git saw two different changes and conflicted on a recipe that
is identical in substance. Same text on both sides merges as one change,
which is cheaper than ordering the two PRs against each other.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGfH1cdFKPnKpM7ytRThft
There's no biome.json, no biome dependency and no lint script — the
directives referenced a tool that never runs, and appeared nowhere else in
the codebase. Replaced with a plain note on why the casts are there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGfH1cdFKPnKpM7ytRThft
@passcod
passcod marked this pull request as ready for review August 1, 2026 23:05
@passcod
passcod added this pull request to the merge queue Aug 1, 2026
Merged via the queue into main with commit bbece9f Aug 1, 2026
7 checks passed
@passcod
passcod deleted the claude/pr-370-fix-m18-useapi-identity branch August 1, 2026 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants