fix(gui-app): stop the 15s host-directory poll from churning every consumer - #971
Conversation
…nsumer The remote-registry refresh poll emitted on every tick whether or not the merged directory had changed, and `useHostDirectoryList` fed a revision counter into the host-picker query key. Each emit therefore minted a FRESH cache entry - `data: undefined` plus a refetch - for all seventeen consumers of the list at once, every fifteen seconds. Most of them flashed a loading state for the refetch RTT (~150ms): the host picker, the workspace selector, the settings panels, the re-auth banner. The terminal and TUI tiles fared worse: `useHostReachability` maps `data === undefined` to "checking", and the tiles render a skeleton instead of `TerminalTileLive` in that state, so the xterm subtree was unmounted and remounted on a 15s cadence - each remount showing "Starting terminal session..." for the two-second grid-measure timeout and taking keyboard focus with it. The warm session registry kept the pty alive across the flap, so nothing closed and nothing was logged; only tile-level instrumentation showed it. Fix both halves: - `performRefresh()` emits only when the merged snapshot actually differs from the last one emitted. Every other emit site (local-host change, selection, re-enrollment) stays unconditional and refreshes the baseline, so a change landing between two polls cannot be swallowed. - The picker query key drops the revision. Directory changes now invalidate that stable key, and TanStack keeps the previous `data` through a same-key refetch, so no consumer regresses to a loading state once it has loaded. `staleTime: 0` stays, paired with the onChange invalidation: that pairing is what stops a boot-time empty fetch from being served fresh for the whole session, and it is load-bearing independently of the key change. Signed-off-by: Hardik Shingala <hardik@traycer.ai>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe change replaces revision-based host-picker queries with stable React Query keys and explicit invalidation. Directory polling suppresses duplicate snapshots. Reactive active-host state updates selection independently from directory data changes. ChangesHost picker refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35fbba39e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ription Dropping the revision from the list query key also dropped the picker's only render signal for the ACTIVE host. `hostClient.onChange` now invalidates the directory query, but a host swap leaves the directory contents unchanged, so structural sharing preserves `data`'s identity and none of the three fields this component reads - `isLoading`, `isError`, `data` - change during the refetch. Nothing re-renders, and the render-time `getActiveHostId()` read keeps the checked row on the previous host until something else happens to re-render the dialog. Read the active host through `useReactiveActiveHostId` instead, which is the existing `useSyncExternalStore` projection over `HostClient.onChange` built for exactly this - state that lives outside React and must be subscribed to rather than sampled at render time. Reported by Codex on #971. Signed-off-by: Hardik Shingala <hardik@traycer.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/gui-app/src/components/layout/header/__tests__/host-picker.test.tsx`:
- Around line 166-191: Update the host-picker test assertions around the
active-host change to query each option by its radio role and visible host
label, rather than using test IDs or data-selected attributes. Assert the
selection state through each radio’s aria-checked accessibility property,
preserving the expected local-then-remote selection transitions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 291ef472-c827-409f-9b0c-00fa18a0f7fb
📒 Files selected for processing (8)
clients/gui-app/src/components/layout/header/__tests__/host-picker.test.tsxclients/gui-app/src/components/layout/header/host-picker.tsxclients/gui-app/src/hooks/host/__tests__/use-host-directory-list-query.test.tsxclients/gui-app/src/hooks/host/use-host-directory-list-query.tsclients/gui-app/src/hooks/host/use-host-picker-list.tsclients/gui-app/src/lib/host/__tests__/host-directory-service.test.tsclients/gui-app/src/lib/host/host-directory-service.tsclients/gui-app/src/lib/query-keys/ui-query-keys.ts
The new active-host regression test read `data-testid` and `data-selected`. Query the options by their radio role and visible label instead, and assert `aria-checked` - the contract a screen reader actually consumes, and what `clients/gui-app/AGENTS.md` asks for. Re-probed after the rewrite: removing the host-client subscription still fails the test, so the assertions did not go slack. Signed-off-by: Hardik Shingala <hardik@traycer.ai>
…it-and-stable-picker-key Signed-off-by: Hardik Shingala <hardik@traycer.ai>
1 participant
The bug
Terminal and TUI tiles flashed "Starting terminal session…" every ~15 seconds, wall-aligned, losing keyboard focus each time. The same cadence blipped the host picker, the workspace selector, the settings panels and the re-auth banner into their loading states.
Nothing appeared in any log: the warm session registry keeps the pty stream alive across the flap, so no stream ever closed and no host event fired. It only surfaced under tile-level mount/unmount instrumentation.
The chain
HostDirectoryService's remote-registry refresh poll (15s) calledthis.emit()on every tick, changed content or not.useHostDirectoryListbumped arevisioncounter on each emit — and that revision was part of the query key, so every emit minted a fresh cache entry:data === undefined, refetching.data === undefinedat once.useHostReachabilitymaps that state to"checking", andTerminalTile/TuiAgentTilerender a loading skeleton instead ofTerminalTileLivewhile "checking" — so the xterm subtree unmounted. The refetch resolved ~150 ms later, the tile remounted, and the fresh bootstrap sat on the 2 s grid-measure timeout showing "Starting terminal session…".Most consumers only flashed for the refetch RTT; the tiles paid a full remount plus focus loss.
The fix
Producer —
performRefresh()now emits only when the merged snapshot actually differs from the last one emitted (field equality, reusing the existing entry comparison). Every other emit site — local-host change, selection, re-enrollment — stays unconditional and refreshes the baseline, so a change landing between two polls can't be swallowed.Consumers — the picker query key drops the revision. Directory changes invalidate that stable key instead, and TanStack keeps the previous
datathrough a same-key refetch, so no consumer regresses to a loading state once it has loaded. Defence in depth: even a legitimate future emit is now non-destructive.staleTime: 0is kept and now pairs with the onChange invalidation. That pairing is what stops a boot-time empty fetch from being served fresh for the whole session, and it is load-bearing independently of the key change.Tests
host-directory-service.test.ts— a field-identical poll does not notifyonChange; a changed field, an added host, and a re-delivered changed value (baseline tracking) all behave correctly.use-host-directory-list-query.test.tsx(new) — entries stay visible across a directory emit with noundefinedwindow, the emit still reaches consumers, and the cache holds exactly one host-picker entry after repeated emits.Both suites were mutation-probed: reverting the producer fix fails the first pair; restoring the revision-in-key fails the second pair with
expected [ undefined, … ] to not include undefinedand 3 accumulated cache keys.Verified in a local staging build — terminal tiles, host list and pickers all sit still across the 15s boundary.