refactor: migrate remaining remote-view pages to useRemoteView() and shared isFirstLoad/isUnreachable flags

[jbouder]

Closes #504.

Summary

• Adds a withRemoteFlags helper in useRemote.ts that wraps every remote list query and returns named isFirstLoad / isUnreachable flags, so pages consume intent instead of re-deriving gating from TanStack internals (errorUpdateCount). The issue-Bug: White screen flash every ~5s when configured remote server is unreachable #217 retry-flash rationale now lives once, in the hook.
• Migrates the straggler pages — Registries, admin/UserManagement, admin/AuditLogs, admin/RegistryManagement — to useRemoteView(), each now showing RemoteUnreachableBanner when the remote is down in remote view, with the same first-load spinner gating Workspaces/Jobs/AdminDashboard got in Fix: White screen flash every ~5s when configured remote server is unreachable #501. Their empty states are gated behind !remoteUnreachable so a down server doesn't read as an empty list.
• Updates Workspaces, Jobs, and AdminDashboard to consume the new flags instead of hand-rolled errorUpdateCount checks.
• Migrates Layout to useRemoteView() (which subscribes to the same remote/server query internally, preserving the never-unmounting self-heal observer) with a comment explaining that role.
• Upgrades AGENTS.md wording from "new or updated pages should use" to "pages gate remote data … with useRemoteView()".
• Adds hook tests for the isFirstLoad lifecycle and the errored → isUnreachable transition; updates the RegistryManagement test mock.

Out of scope (per the issue): the no-interval queries keep their current no-polling behavior; no backend changes to /remote/server status semantics.

Test plan

• npm run test — 201 passed
• npm run check — clean
• npm run build — passes

🤖 Generated with Claude Code

[netlify]

✅ Deploy Preview for nebi-docs canceled.

Name	Link
🔨 Latest commit	0a91686
🔍 Latest deploy log	https://app.netlify.com/projects/nebi-docs/deploys/6a837166bb351500087bad33

[tylerpotts]

Architecture Review

Inferred Patterns

• [documented] TanStack Query networkMode is set per app mode in src/lib/queryClient.ts + src/store/modeStore.ts; never pin 'online' in desktop-reachable code paths (AGENTS.md "Two frontend invariants")
• [documented] GET /remote/server status: 'connected' means credentials are stored, not that the remote is reachable; reachability surfaces as errors on the remote data queries; pages gate remote data and the unreachable banner with useRemoteView() and consume isFirstLoad / isUnreachable from the remote query hooks rather than re-deriving from TanStack internals (AGENTS.md "Two frontend invariants" - wording strengthened by this PR)
• [documented] Frontend layering: typed axios clients in src/api/*.ts (one per backend resource), TanStack Query for server data, Zustand for client state, pages in src/pages/, feature components in src/components/ (AGENTS.md "Frontend")
• [inferred] Remote data flows pages → hooks in src/hooks/useRemote.ts → src/api/remote.ts; pages do not call useQuery/remoteApi directly (followed everywhere except RemoteWorkspaceDetail.tsx, which predates this PR)
• [inferred] Every remote-capable list page follows one shape: useRemoteView() for gating, an enabled flag into the remote list hook, spinner on isFirstLoad, RemoteUnreachableBanner when isRemoteView && isUnreachable, empty state suppressed by !remoteUnreachable
• [inferred] Remote queries that feed an unreachable-banner condition poll via pollWithErrorBackoff so the banner self-heals after the server recovers (explicit rationale comment in useRemoteDashboardStats, frontend/src/hooks/useRemote.ts:221-227)
• [inferred] useConnectServer/useDisconnectServer resetQueries(['remote']) (not invalidate) so stale errored state never survives a reconnect

Blockers (design-gating)

[D] AGENTS.md now overclaims the flag pattern the codebase actually satisfies

• AGENTS.md:77 - The PR replaces the scoped sentence ("new or updated pages should gate … Registries and the remaining admin pages still hand-roll the derivation, migration tracked in Migrate remaining remote-view pages to useRemoteView() #504") with an unqualified invariant: "pages gate remote data and the unreachable banner with useRemoteView() … and consume the isFirstLoad / isUnreachable flags the remote query hooks return."
  This violates the documented invariant it itself creates: frontend/src/pages/RemoteWorkspaceDetail.tsx:37-59 is a remote-data page that uses neither useRemoteView() nor the flags nor the banner - it bypasses the hook layer entirely, calling useQuery + remoteApi inline even though useRemoteWorkspace/useRemoteVersions/useRemoteTags exist in useRemote.ts:114-136 (and now have zero page consumers). Those detail hooks are also not wrapped in withRemoteFlags, so "the remote query hooks return" the flags is only true for the list/aggregate hooks.
  Suggested fix: either keep a scoping caveat in AGENTS.md (e.g. "remote list pages …; the remote workspace detail page and per-workspace hooks are not yet migrated, tracked in issue N") or fold RemoteWorkspaceDetail + the detail hooks into the migration. A doc invariant that a grep immediately falsifies loses the straggler-tracking the old sentence carried.

Questions

[D] Should the migrated banner pages also get the self-heal polling half of the pattern?

• useRemoteDashboardStats carries an explicit design comment (frontend/src/hooks/useRemote.ts:221-227): a query in a banner condition must poll, because an errored query without an interval only refetches on remount, so the banner sticks after the server recovers. This PR adds RemoteUnreachableBanner to four pages whose queries have no refetchInterval (useRemoteRegistries, useRemoteUsers, useRemoteAdminRegistries, useRemoteAuditLogs; banners at Registries.tsx:91, UserManagement.tsx:130, AuditLogs.tsx:78, RegistryManagement.tsx:95), recreating exactly the stuck-banner condition that comment warns about. The PR body declares no-polling behavior out of scope per Migrate remaining remote-view pages to useRemoteView() #504 - but the banner half and the polling half were designed as one mechanism. Is there a follow-up issue, and should the withRemoteFlags doc comment at useRemote.ts:42-55 note this coupling so the next page author doesn't add a banner without an interval?

[D] Was leaving the enabled-flag split (isRemoteConnected vs isRemoteView) intentional?

• After the migration, Workspaces/Jobs/Registries enable their remote list query with isRemoteConnected (fetches even while the user views local data: Registries.tsx:43, Jobs.tsx:227, Workspaces.tsx:80), while the admin pages enable with isRemoteView (UserManagement.tsx:57, AuditLogs.tsx:37, RegistryManagement.tsx:24, AdminDashboard.tsx:82-84). Both conventions predate the PR, but a PR whose purpose is unifying remote-view handling preserved the fork without comment. Is prefetch-while-in-local-view a deliberate UX choice for the non-admin pages, or an accident worth unifying (perhaps inside the hooks, so pages can't choose wrong)?

[F] "refactor:" title vs. behavior change in the second commit?

• Commit f9a7a4f ("hold isUnreachable through retry refetches", useRemote.ts:59-61) changes runtime banner behavior - the banner now stays up during retry windows where it previously flashed off - and the empty-state gating on four pages is likewise new behavior. Both are disclosed in the PR body and are the point of the migration, so this is flagged only to confirm scope intent: were the banner-hold semantics wanted on the previously-migrated pages (Workspaces/Jobs/AdminDashboard) too, since they silently pick it up?

Stylistic

• frontend/src/hooks/useRemote.ts:37-55: the pre-existing "Shared view-state derivation…" comment (which documents useRemoteView) now runs directly into the new withRemoteFlags comment, and withRemoteFlags sits between the combined block and useRemoteView - the first paragraph reads as if it documents withRemoteFlags. Splitting the blocks so each sits on its own declaration would help.
• frontend/src/pages/admin/RegistryManagement.test.tsx:39-53 hand-mirrors the useRemoteView/flag return shapes; a shared test factory for these mocks would keep them from drifting as the hook grows fields.
• frontend/README.md is badly stale (React 18, "Environments" pages, no mention of the remote-view architecture) - untouched by this PR, but it is the only other frontend doc and currently contradicts AGENTS.md.

Suggested documentation follow-up

For AGENTS.md "Two frontend invariants" (extends the sentence this PR rewrote), capturing the polling/banner coupling that today lives only in a code comment:

Remote list queries that feed the unreachable banner must keep retrying on their own: an errored TanStack query without a refetchInterval only refetches on remount or window focus, so a banner gated on it would stick until the user navigated away even after the server recovered. Use pollWithErrorBackoff (see useRemoteJobs / useRemoteDashboardStats in src/hooks/useRemote.ts) on any query whose isUnreachable flag a page renders. The remote workspace detail page (RemoteWorkspaceDetail.tsx) and the per-workspace hooks predate this pattern and are not yet migrated.

[jbouder]

Thanks for the thorough review @tylerpotts — addressed in 5a4ada6:

Blocker (AGENTS.md overclaim): Restored a scoping caveat to the "Two frontend invariants" bullet — RemoteWorkspaceDetail.tsx and the per-workspace hooks predate the pattern and aren't migrated — and filed #507 to track folding them in (including wrapping the detail hooks in withRemoteFlags), so the straggler-tracking the old sentence carried isn't lost. Went with the caveat rather than expanding this PR, since #504's acceptance criteria didn't cover the detail page.

Stuck banner on the no-poll queries: Agreed this recreated the exact condition the useRemoteDashboardStats comment warns about, so it's fixed here rather than deferred: a new retryWhileUnreachable interval (retry on the error-backoff cadence while errored, false while healthy) is wired into useRemoteRegistries, useRemoteUsers, useRemoteAdminRegistries, and useRemoteAuditLogs. Healthy-state behavior is unchanged — still no polling — so the steady-state freshness question #504 scoped out as a product decision remains open. The withRemoteFlags doc comment now states the coupling ("any query whose isUnreachable a page renders must keep retrying — pollWithErrorBackoff or retryWhileUnreachable"), and your suggested sentence is in AGENTS.md.

isRemoteConnected vs isRemoteView enabled split: Preserving it was deliberate in the sense of not changing fetch behavior in a migration PR, not an endorsement — I can't speak to whether prefetch-while-local was originally intentional on Workspaces/Jobs/Registries. Unifying it (ideally inside the hooks so pages can't choose wrong) is an acceptance criterion on #507.

"refactor:" title vs behavior: Yes — the banner-hold semantics from f9a7a4f applying to the previously-migrated pages is intended; the flash-off-during-retry was the same defect everywhere, and centralizing the fix in withRemoteFlags is the point of the migration.

Stylistic: Split the comment blocks so useRemoteView and withRemoteFlags each carry their own. Left the test-mock factory and the stale frontend/README.md alone for now — the README is worth its own issue (noted in #507's out-of-scope).

[tylerpotts]

Architecture Review (round 2)

Scope: 5a4ada6 (the response to the round-1 review) plus the 2dd8e39 main merge. I re-verified the round-1 items against the code rather than taking the reply at face value.

No blockers. One substantive finding worth fixing here, two small notes.

Round-1 items: all three resolved

• [Blocker] AGENTS.md overclaim - fixed. The scoping caveat is back, naming RemoteWorkspaceDetail.tsx and the per-workspace hooks, and the suggested coupling sentence landed essentially verbatim.
• Stuck banner on the no-poll queries - fixed rather than deferred. retryWhileUnreachable is wired into exactly the four banner-feeding queries that don't poll (useRemote.ts:183, :205, :216, :230), and healthy-state behavior is unchanged as claimed.
• Straggler tracking - Migrate RemoteWorkspaceDetail and per-workspace hooks to useRemoteView()/withRemoteFlags #507 is open and its acceptance criteria genuinely cover what the reply promised: consuming the detail hooks, wrapping them in withRemoteFlags, unifying the isRemoteConnected vs isRemoteView split, and removing the AGENTS.md caveat.

The flag logic is correct (I checked, rather than assumed)

I went in suspecting the isPending && errorUpdateCount > 0 hold in withRemoteFlags (useRemote.ts:70-71) was dead code, on the assumption that a refetch after an error keeps status: 'error'. It is not dead code. In TanStack v5, fetchState() resets status to 'pending' and clears error when data === undefined, so a never-succeeded query does drop out of isError on every retry. The hold is load-bearing, and useRemote.test.ts:267 exercises it against real msw + TanStack rather than a mocked result object, which is the right way to test it.

retryWhileUnreachable also survives that same reset: the transient 'pending' window clears the interval mid-flight, then the next error re-arms it, so the banner still self-heals on the 30s cadence.

Checked specifically as well: every migrated page scopes the banner as isRemoteView && remoteIsUnreachable, so on the pages that prefetch while in local view (Registries, Jobs, Workspaces enable with isRemoteConnected) an errored remote query cannot suppress the local empty state. And the pre-PR remoteLoading && remoteErrorCount === 0 on Workspaces/Jobs is exactly isFirstLoad, so those are faithful refactors.

Finding: the {...query} spread defeats TanStack's tracked-props optimization

frontend/src/hooks/useRemote.ts:67-72. A real regression introduced by the wrapper, and invisible to the test suite.

The chain, verified in the installed library rather than from memory:

1. useBaseQuery.js:99 returns observer.trackResult(result) whenever notifyOnChangeProps is unset. It is unset app-wide: src/lib/queryClient.ts sets only refetchOnWindowFocus, retry, and networkMode, and nothing in src/ sets it.
2. trackResult (queryObserver.js:133) is a Proxy whose get trap calls trackProp(key).
3. Object spread performs [[Get]] on every own enumerable property, so {...query} marks all of them tracked, including isFetching, fetchStatus, and dataUpdatedAt.

Measured with a throwaway probe (since deleted), on a refetch returning referentially identical data:

no-op refetch -> narrow: +0, spread: +1, spread+notifyOnChangeProps: +0

narrow is the pre-PR shape, where pages destructured exactly data / isLoading / isError / errorUpdateCount. So before this PR a poll tick with unchanged data caused no re-render; now it causes one.

Who pays: Jobs and Workspaces (5s polls), and AdminDashboard, which holds three whole query objects (workspaces and jobs at 5s, stats at 30s). The four no-poll admin pages are unaffected while healthy, but re-render on the 30s retry cadence while unreachable.

This is wasted reconciliation rather than incorrect UI, so it is not a blocker. I would still fix it in this PR: centralizing this hook layer is the whole point of the change, so it is the cheapest possible moment, and the third column above shows the fix works.

notifyOnChangeProps: ['data', 'isLoading', 'isPending', 'isError', 'errorUpdateCount'],

Setting that on the wrapped queries makes useBaseQuery return the raw result instead of the proxy, so the spread costs nothing. Best applied once in a shared base-options object next to withRemoteFlags so all seven hooks stay consistent, with a comment that the list must grow if a page ever starts consuming isFetching or refetch. Returning a nested { query, ...flags } also solves it but throws away the flat ergonomics this PR is going for.

Worth pairing with a render-count assertion on one polling hook, since nothing in the current suite would catch a reintroduction.

Two small notes

• Comment accuracy. useRemote.ts:26-27 and :64-66, plus the new AGENTS.md sentence, all say an errored query without an interval "only refetches on remount or window focus". refetchOnWindowFocus: false is set globally in src/lib/queryClient.ts, so window focus is not a recovery path in this app and remount is the only one. The conclusion is unchanged (arguably stronger), but the parenthetical will mislead the next person deciding whether an interval is required.
• Type narrowing. withRemoteFlags takes UseQueryResult<T> and spreads it, which collapses the discriminated union of pending/success/error variants into a single object type, so if (q.isSuccess) q.data no longer narrows data to non-undefined. Nothing relies on it today. Worth knowing before Migrate RemoteWorkspaceDetail and per-workspace hooks to useRemoteView()/withRemoteFlags #507 wraps the detail hooks, where narrowing on a single object is more tempting.

[tylerpotts]

Some optional fixes that would be worth doing in this PR described above, but not blocking if you decide to leave them alone

[jbouder]

Thanks for round 2 @tylerpotts — the {...query} finding is real and is fixed in 0a91686.

notifyOnChangeProps (the substantive finding): I reproduced it before fixing it, since the act(async () => await refetch()) shape I reached for first hides it — that batches fetch-start and fetch-settle into one flush, so all three configurations show delta 0. Measuring across real poll ticks instead (refetchInterval: 50, 400ms window):

narrow: +0   spread: +8   spread + notifyOnChangeProps: +0

which matches your numbers. Fixed as you suggested: a shared remoteFlagNotifyProps sits next to withRemoteFlags and is pinned on all seven wrapped queries, with a comment that the list must grow if a page ever starts consuming another field. Kept the flat return shape.

Also added the render-count assertion you called for (useRemote.test.ts, on useRemoteWorkspaces): it fires refetch() outside act() so the two notifications land in separate flushes the way a poll tick does, and asserts renders stay flat. Verified it actually bites — with the pin removed it fails at expected 3 to be 2.

Comment accuracy: fixed in the same commit. Both useRemote.ts sites and the AGENTS.md sentence now say remount is the only recovery path, noting refetchOnWindowFocus: false is set app-wide rather than leaving "or window focus" to mislead. Added a clause to the AGENTS.md invariant that withRemoteFlags-wrapped queries must pin notifyOnChangeProps, since #507 will wrap more hooks.

Type narrowing: noted, no change here — nothing relies on it today. I've added it to #507 so whoever wraps the detail hooks knows the union collapses before they reach for if (q.isSuccess) q.data.

npm run test (205 passed), npm run check, and npm run build are all clean on the rebased branch.