Skip to content

Cut hidden and redundant thread-detail query refetches on realtime events - #1286

Closed
SawyerHood wants to merge 1 commit into
mainfrom
bb/gate-hidden-on-mobile-environment-queries-thr_nysargddie
Closed

Cut hidden and redundant thread-detail query refetches on realtime events#1286
SawyerHood wants to merge 1 commit into
mainfrom
bb/gate-hidden-on-mobile-environment-queries-thr_nysargddie

Conversation

@SawyerHood

Copy link
Copy Markdown
Collaborator

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.

  • Invalidate the environment pull-request query on git-refs-changed, not on every work-status-changed event.
  • Make the pull-request focus refetch stale-aware instead of "always".
  • Gate the forks mirror query and the thread-storage file list on secondary panel visibility.

Re-verification of the reported finding

The task premise said the view-level useEnvironmentWorkStatus and useEnvironmentPullRequest queries 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:

  • The changed-files tally and the checkout label come from the work-status query (ThreadDetailView.tsxworkspaceChangedFilesSection, threadCheckoutDisplay).
  • The PR pill and the merge/ready actions come from the pull-request query (ThreadDetailPromptArea.tsxpullRequestSection).
  • The header git actions (useThreadGitActions) also read the work status on every viewport.

The @container promptbox rules in app.css hide 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 current enabled condition, 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. dirtyEnvironmentLiveWorkspaceStateQueries invalidated the PR query on every work-status-changed event. 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 a gh shell-out on the server per edit batch. A content-only edit cannot change PR state. The PR query now invalidates on git-refs-changed (commit or push moves the PR head). Remote-only changes stay covered by three existing paths: the turn/completed invalidation 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 through invalidateEnvironmentActionQueries, 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 a gh shell-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:

  • The forks mirror query in ThreadDetailSecondaryContent (useThreads with originKind: "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.
  • The thread-storage file list in ThreadDetailView refetched on every thread-storage-changed and environment-changed event 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-demand refetch() when the storage root is unknown, and refetch() bypasses enabled.

Both fixes follow the existing precedent in the same file: useThreadTerminals was already gated on isSecondaryPanelOpen.

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:

Query Hidden while invalidated? Result
Thread timeline windows No — main view OK
Sidebar thread lists No — the mobile sidebar drawer unmounts when closed (vaul portal) OK
Forks mirror (ThreadDetailSecondaryContent) Yes Fixed here
Thread-storage file list (ThreadDetailView) Yes Fixed here
Thread search No — enabled only with a query string OK
Prompt history No — composer is visible OK
Environment diff TOC / file previews No — already gated (isDiffPanelActive, tab-active) OK
Thread terminals No — already gated on panel open OK
Conversation outline (TOC) No — fixed by #1214 OK
Parent-thread subset No — enabled only after the picker opens OK
Plugin sidebar PR rows No — per-mounted-row by design OK

Follow-up candidates, not changed here:

  • useEnvironmentWorkStatus keeps staleTime: 0 plus 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.
  • The Mobile version hangs for several seconds for each tap #1269 memory lists timeline re-render amplifiers (un-virtualized timeline, per-message document listeners). Those are render-cost items, not query items, and stay tracked there.

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 gh shell-out) per 250 ms-debounced work-status-changed batch, 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/app
  • pnpm exec turbo run test --filter=@bb/app --force — 326 files, 2469 tests passed
  • pnpm 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 — clean

New regression coverage, in the #1214 compact-pane pattern:

  • realtime-cache-effects.test.ts: a content-only work-status-changed event does not refetch or invalidate an active PR query; a git-refs-changed event 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

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>
@SawyerHood

Copy link
Copy Markdown
Collaborator Author

🚨 SLOP COP 🚨 · review

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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 },

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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 SawyerHood left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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 gh processes.
  • Medium: The new forks gate controls only the mirror query. The mounted ForksRow still 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.

@SawyerHood

Copy link
Copy Markdown
Collaborator Author

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.

@SawyerHood SawyerHood closed this Aug 10, 2026
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.

1 participant