Cut hidden and redundant thread-detail query refetches on realtime events - #1286
Cut hidden and redundant thread-detail query refetches on realtime events#1286SawyerHood wants to merge 1 commit into
Conversation
Re-verification showed the view-level work-status and pull-request queries feed the composer context banner, which is visible on every viewport, so they must stay enabled. The real waste was elsewhere: the PR lookup refetched on every workspace content edit and on every window focus, and two panel-only queries (forks mirror, thread-storage file list) refetched while the panel or drawer was closed. - Invalidate the environment PR query on git-refs-changed instead of work-status-changed; content-only edits cannot change PR state. - Make the PR focus refetch stale-aware so the settled-PR stale tier stops a gh shell-out on every app switch. - Gate the forks mirror query and the thread-storage file list on secondary panel visibility. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am reviewing this pull request now. I am the SlopCop for this review. I will run security, code quality, architecture, and performance checks in parallel. I will also test the affected route when possible. |
| dirty: [ | ||
| dirtyEnvironmentRefDerivedWorkspaceStateQueries, // Only cached ref-derived workspace queries need refresh. | ||
| dirtyEnvironmentBranchListQueries, // Refs can add/remove/rename branch options. | ||
| dirtyEnvironmentPullRequestQueries, // A commit/push moves the PR head; content-only edits do not. |
There was a problem hiding this comment.
🚨 slopcop/review — This event does not cover a branch checkout.
A checkout between existing branches changes HEAD and the local workspace fingerprint. It does not change a shared ref. The daemon then emits only work-status-changed, which no longer invalidates this query. A settled PR can remain in the cache for one hour. The UI can show branch A PR data after the checkout moves to branch B.
This event also covers all shared refs. One sibling worktree commit can start a gh lookup for each active environment in the repository. Please use a current-checkout signal or include checkout identity in the query key. A new daemon signal also requires a protocol version increment.
| originKind: "fork", | ||
| archived: false, | ||
| }, | ||
| { enabled: isSecondaryPanelOpen }, |
There was a problem hiding this comment.
🚨 slopcop/review — This gate disables only the mirror forks query.
The closed wide panel keeps ThreadMetadataContent mounted. Its ForksRow calls the same useThreads query without an enabled option. I confirmed this in the live route. The initial closed panel sent the targeted originKind=fork request.
The new test mocks ThreadMetadataContent, so it cannot detect this observer. Please fetch forks once and pass the result into the metadata component. Another option is to pass the visibility flag through to ForksRow.
| // Timeline storage links still resolve while closed: the link handler | ||
| // falls back to an on-demand refetch when the storage root is unknown. | ||
| const shouldLoadThreadStorageFiles = | ||
| thread !== undefined && isSecondaryPanelOpen; |
There was a problem hiding this comment.
🚨 slopcop/review — The saved open state is not the actual split-pane visibility.
Each split pane keeps its ThreadDetailView mounted. An inactive pane can keep isSecondaryPanelOpen true after focus moves. Its storage observer then remains active, although the host shows only the focused pane panel.
Please also require isFocused here. Please use the same effective visibility value for the forks query.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: This change asks less often for data that nobody can see. Two hidden queries still run. A branch checkout can also show old PR data.
I found one correctness issue and three performance issues. I left three inline comments because two PR-query issues share one changed line.
- High: A checkout between existing branches does not emit
git-refs-changed. The old branch PR can remain in the environment query cache. - Medium: The shared-ref event reaches each watched environment in the repository. One ref update can start many
ghprocesses. - Medium: The new forks gate controls only the mirror query. The mounted
ForksRowstill owns an active query while the panel is closed. - Medium: An inactive split pane can retain an open state. Its hidden storage query then remains active after focus moves.
I found no new security issue. I checked authorization, cache isolation, command input, storage path limits, and cross-environment data reuse.
The forks data has two query owners. One owner should provide both hasForks and the rendered fork list. A checkout-specific PR signal would also avoid the broad shared-ref fan-out.
Validation passed for 326 app test files and 2,469 tests. The Turbo app type check also passed. git diff --check passed.
I also tested the thread-detail route in the local app. The closed panel still sent the forks request. The storage request started only after I opened the panel.
|
Closing for now — not handling this follow-up at the moment. SlopCop's findings (checkout-vs-refs PR staleness, shared-ref fan-out, ForksRow duplicate query, split-pane visibility) are captured in the review thread for whenever this is picked back up. |
Summary
Follow-up #2 from the issue #1269 diagnosis (the first fix is PR #1281). This change removes hidden-query refetch work in the thread detail view and removes redundant pull-request lookups on all viewports.
git-refs-changed, not on everywork-status-changedevent."always".Re-verification of the reported finding
The task premise said the view-level
useEnvironmentWorkStatusanduseEnvironmentPullRequestqueries feed only hidden surfaces on a compact viewport. That premise is not correct. The composer context banner (ThreadPromptContextBanner) consumes both queries, and the composer is visible on every viewport, including mobile:ThreadDetailView.tsx→workspaceChangedFilesSection,threadCheckoutDisplay).ThreadDetailPromptArea.tsx→pullRequestSection).useThreadGitActions) also read the work status on every viewport.The
@container promptboxrules inapp.csshide only labels at narrow widths, not the banner. A visibility gate on these two queries would blank visible mobile UI and would also break the desktop banner when the panel is closed. So these two queries keep their currentenabledcondition, and this PR attacks the actual waste instead: the invalidation and refetch cadence, plus the queries that truly have no visible consumer while hidden.Root cause and fixes
1. The PR lookup ran on every workspace content edit.
dirtyEnvironmentLiveWorkspaceStateQueriesinvalidated the PR query on everywork-status-changedevent. The host daemon emits that event for any workspace content change, and the client debounce is only 250 ms, so an active agent turn triggered aghshell-out on the server per edit batch. A content-only edit cannot change PR state. The PR query now invalidates ongit-refs-changed(commit or push moves the PR head). Remote-only changes stay covered by three existing paths: theturn/completedinvalidation from #1025, the 5 s pending-checks poll from #251, and stale-aware focus refetches. This also improves freshness for a mid-turn push, which previously did not invalidate the PR query at all (a push moves refs, not the work status). In-app merge/ready/draft actions invalidate the query directly throughinvalidateEnvironmentActionQueries, unchanged.2. Focus refetch ignored the stale tiers.
refetchOnWindowFocus: "always"(#248) defeated the deliberate settled-PR stale tier (1 h for merged/closed PRs, #251 era). Every app switch on a phone paid aghshell-out through the Connect tunnel, even for a merged PR. The refetch is now stale-aware (true): open PRs still refetch on focus after the 30 s tier; settled PRs stop paying. This keeps the #248 intent for the states that can still change.3. Two panel-only queries ran while hidden. Same class as the #1214 outline fix:
ThreadDetailSecondaryContent(useThreadswithoriginKind: "fork") refetched on every realtime thread-list invalidation while the panel or drawer was closed. It now enables only while the panel is open. Its only consumer is the metadata placeholder decision inside the panel.ThreadDetailViewrefetched on everythread-storage-changedandenvironment-changedevent while closed. All consumers live in the panel (metadata storage section, storage browser, storage-tab pruning). Timeline storage links still resolve while closed because the link handler already falls back to an on-demandrefetch()when the storage root is unknown, andrefetch()bypassesenabled.Both fixes follow the existing precedent in the same file:
useThreadTerminalswas already gated onisSecondaryPanelOpen.Audit (scope item 2)
I checked every query family the hot realtime paths invalidate (
events-appended,turn/completed,status-changed,work-status-changed) against its visibility on a compact viewport:ThreadDetailSecondaryContent)ThreadDetailView)isDiffPanelActive, tab-active)Follow-up candidates, not changed here:
useEnvironmentWorkStatuskeepsstaleTime: 0plus per-edit invalidation because the visible changed-files tally must track edits. If the mobile composer ever drops the tally, this query becomes gateable on compact viewports.Request-count effect (reasoned, not device-measured)
I did not run a phone A/B capture for this change. The arithmetic from the invalidation paths: during an active turn that edits files, the previous behavior issued one PR lookup (server-side
ghshell-out) per 250 ms-debouncedwork-status-changedbatch, plus one per window focus regardless of PR state. After this change, a turn issues PR lookups only on ref moves (commits/pushes) and once at turn completion. For a turn with N edit batches and C commits, that is C + 1 lookups instead of ~N + C + 1, and settled-PR focus refetches drop to zero. The forks and storage queries drop to zero requests while the drawer is closed.Validation
pnpm exec turbo run typecheck --filter=@bb/apppnpm exec turbo run test --filter=@bb/app --force— 326 files, 2469 tests passedpnpm exec turbo run lint --filter=@bb/app— 0 errors (141 pre-existing warnings; the one in a touched file reproduces on main)git diff --check— cleanNew regression coverage, in the #1214 compact-pane pattern:
realtime-cache-effects.test.ts: a content-onlywork-status-changedevent does not refetch or invalidate an active PR query; agit-refs-changedevent refetches it.ThreadDetailSecondaryContent.test.tsx: the forks mirror query is disabled while the compact drawer (and the wide panel) is closed, and enables when it opens.🤖 Generated with Claude Code