Skip to content

Patch sidebar rows from status-changed pushes instead of refetching - #2169

Merged
SawyerHood merged 1 commit into
mainfrom
bb/fix-1302-sidebar-bootstrap
Aug 21, 2026
Merged

Patch sidebar rows from status-changed pushes instead of refetching#2169
SawyerHood merged 1 commit into
mainfrom
bb/fix-1302-sidebar-bootstrap

Conversation

@SawyerHood

@SawyerHood SawyerHood commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

What was wrong

Every thread lifecycle transition broadcast status-changed as a bare dirty flag (metadata carried at most projectId). The app's registry rule for it (dirtyActiveThreadListQueries, flush immediate) invalidated the single sidebarNavigation query plus every cached thread list for the project. The sidebar query is always active (AppLayout observes it with staleTime: Infinity), so each push re-downloaded the whole GET /api/v1/sidebar-bootstrap document: about 1 KB per unarchived thread, 134 KB on the seeded database, twice per turn (turn start, turn end), for every thread that runs a turn. Nothing in the push let the client patch the one row that changed. Issue: #1302. Report: https://get-bb.github.io/reports/issues/1302.html

What changed

Server-to-app realtime contract (no daemon change; the host daemon does not consume thread change notifications, so HOST_DAEMON_PROTOCOL_VERSION is unchanged):

  • packages/domain/src/change-kinds.ts: threadChangeMetadataSchema gains optional statusChange: { status, runtime, activity, latestAttentionAt, updatedAt } (the list-row fields a lifecycle transition rewrites). The lenient inbound twin parses it with .catch(undefined) so a client that does not know a future status or runtime value drops just that field and falls back to a refetch. threadRuntimeStateSchema and threadActivityStateSchema are now exported from thread.ts.
  • apps/server/src/services/threads/thread-runtime-display.ts: buildThreadStatusChangeMetadata(deps, thread) builds the metadata in one place. Runtime is resolved from host connectivity the same way list rows do. Activity (background task counts plus the plan-mode and goal counts) is built by the new buildThreadActivityStateByThreadId, which toThreadListEntryResponses now also uses, so a pushed row and a fetched row cannot disagree. The builder therefore takes the prompt-banner deps (db, hub, providerRegistry); every caller already had them through AppDeps/WorkSessionDeps (failThreadProvisioning and applyTurnCompletedEvent widen their Pick).
  • packages/db/src/data/threads.ts: applyThreadLifecycleEvent(db, args) no longer takes a notifier or notifies. The db package cannot resolve the runtime (host connectivity lives in the hub), so the server wrapper applyLoggedThreadLifecycleEvent (lifecycle-outcome.ts) now owns the status-changed push and attaches the metadata. This covers turn start (run.started from the daemon's turn/started), turn end (turn/completed), provisioning, reconciliation and failure paths.
  • thread-send.ts, queued-messages.ts, parent-system-messages.ts: the three post-commit producers that activate a thread now carry the activated row out of the transaction (activeThread: Thread | null replaces threadBecameActive: boolean) and attach the metadata. queued-messages.ts also drops a redundant status-changed notify that fired inside the transaction, before commit; the post-commit notify on the next line already sent the same kind.
  • packages/domain/src/plugin-sdk-version.ts + packages/plugin-sdk/package.json: no longer changed by this PR. The new statusChange field does change the SDK's bundled types and dist/provider-bridge.js, so the npm version guard (check-npm-version-guard.mjs) needs an unpublished version. main has since moved the SDK to 0.4.13, which npm has not published (npm latest is 0.4.12), so this PR adopts main's version and the guard passes without a further bump.
  • In-transaction writers whose hub is a NotificationBuffer (stop requested, command failure, thread-start success, finalize, host-wide interruption, environment cleanup, host reconnect fan-out) still send the bare kind. The client falls back to today's refetch for those; they are not on the per-turn hot path.
  • apps/app: realtime-cache-effects.ts merges statusChange into the dirty context. A status-changed message is last-writer-wins for it: a later message that carries no row snapshot replaces (drops) an earlier one merged while the document was hidden, so the resume flush refetches instead of patching the row to the earlier, now-stale status. realtime-cache-registry.ts replaces dirtyActiveThreadListQueries in the status-changed rule with patchThreadListStatusState: with metadata it writes the five fields into every cached thread list row and the sidebar bootstrap (updateCachedThreadListStatusState in query-cache.ts, same shape as the existing pending-interaction patch), invalidates only list/sidebar queries that have a fetch in flight (that fetch read the database before the transition and would overwrite the patch when it lands), and still dirties the search prefix. Without metadata it behaves exactly as before. Thread detail invalidation is unchanged (about 600 B when the thread is open).

Revision after review (two findings, both fixed here):

  1. The first draft's patch left ThreadListEntry.activity stale. The plan-mode and goal counts are server-computed, gated on status === "active", and were only synced by the list refetch the patch removed, so a finished plan turn kept its sidebar indicator lit. The push now carries the post-transition activity and the app patches it with the rest of the row.
  2. The hidden-document merge kept an earlier statusChange when a later bare status-changed arrived (stop, command failure, interruption), so on resume the row was patched to active and never refetched. statusChange is now last-writer-wins per status-changed message.

Deviation from the report's proposal: the report suggested status + runtime only. latestAttentionAt and updatedAt are included because the lifecycle writer rewrites them and the sidebar sorts inactive rows by latestAttentionAt; activity for the reason above. The report's parts 2 (trim the bootstrap wire shape) and 3 (per-project sidebar keys) are not in this PR; with no refetch per turn, the payload size only matters on initial load and on membership changes.

Known, pre-existing: the sidebar learns that a plan turn is active only from a list row fetched after the provider's turn/input/accepted lands. The turn-start push (and on main, the turn-start refetch) is built at send time, before that event exists, and events-appended does not refetch lists, so the plan-mode glyph at turn start was already a race on main. This PR keeps that behavior and fixes the indicator turning off at turn end. Pushing an activity patch on the accepted/goal events is a separate follow-up.

How you verified

Tests added:

  • apps/app/src/hooks/realtime-cache-effects.test.ts: "patches cached thread list status from notification metadata instead of refetching the sidebar bootstrap". Fails on origin/main app sources with AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times (the sidebar query fn was refetched); passes after. The pushed statusChange in this test carries activity.activePlanModeCount: 1 and the row assertion covers it. Also: "refetches thread lists for a status change that carries no row metadata" (guards the fallback), "restarts a sidebar fetch already in flight so its stale snapshot cannot overwrite the patched status", and (hidden document) "refetches when a bare status-changed follows one that carried the row". The last fails on the first draft's merge with AssertionError: expected { activity: { …(5) }, …(5) } to be { activity: { …(5) }, …(5) } // Object.is equality (the idle row had been replaced by the patched active row); passes after.
  • apps/server/test/services/threads/lifecycle-outcome.test.ts (new): applyLoggedThreadLifecycleEvent broadcasts status-changed with projectId and the full statusChange (runtime active with a registered daemon, waiting-for-host without), nothing when the event is not applied, and "carries the status-gated plan and goal activity of the post-transition row": with an open accepted /plan turn and an active goal on record, run.started pushes activePlanModeCount: 1, activeGoalCount: 1 and run.succeeded pushes status: idle with activePlanModeCount: 0, activeGoalCount: 1. On the first draft's server builder the broadcast is rejected by the strict schema (ZodError: Invalid input: expected object, received undefined for activity); on main the first assertion fails because the db notify carried only projectId.
  • packages/domain/test/change-kinds.test.ts: maximal fixture extended (the parity guard requires it) plus "drops a status change a stale client cannot parse but keeps the message".
  • packages/db/test/data/thread-lifecycle.test.ts: the notify assertion moved to the server test; call sites updated for the new signature (also tests/integration/fake/recovery/idle-error-reconciliation.test.ts, which polls the API and does not depend on the push; ran it, passes).

Commands (on the committed tree, rebased on current origin/main, git status --porcelain empty):

  • pnpm exec turbo run typecheck (whole repo): Tasks: 72 successful, 72 total.
  • pnpm exec turbo run test --filter=@bb/domain --filter=@bb/db --filter=@bb/server --filter=@bb/app --continue: Tasks: 9 successful, 10 total. domain 136 passed; db 405 passed; app 3160 passed (3 skipped); server 1825 passed, 2 failed: test/internal/internal-skill-trees.test.ts (mode: 420 vs 436, a local umask 0002 artifact unrelated to this change, passes with umask 022) and test/services/plugins/plugin-update.test.ts "waits one full interval" (Test timed out in 5000ms under the full parallel run; passes alone, 27/27).

Manual check on my dev instance (scratch project, one codex thread, a dedicated headless Chromium profile with a window.fetch logger installed after load, then POST /api/v1/threads/:id/send with a /plan Reply only with ok. command mention from the driving script, sampling the sidebar row's indicator labels every 100 ms):

  • Before (report, same experiment with plain tell): 19 requests, 2 × GET /api/v1/sidebar-bootstrap at 134,865 B and 134,861 B (96% of bytes), plus child/fork list refetches at turn start and end.
  • After: 10 requests, 0 sidebar-bootstrap calls, no thread list refetches; the turn traffic is the thread detail (588/584 B), timeline deltas, outline, prompt history, PR state and read receipt. The sidebar row showed Thread working 123 ms after the send and cleared it at turn end (2.3 s), from the pushed patch alone. At 1.5 s GET /threads?projectId= reported status: active, activePlanModeCount: 1; after the turn the row carried no stale plan indicator. Log saved at /tmp/bb-fix-batch/issues/1302/revise-plan-turn-api-log.json.

Fixes #1302

AGENT GENERATED: by Claude Opus 5

Independent verification

Verified round 2 at head 1e1e56ff7 (rebased on origin/main c942421a4; git merge-base --is-ancestor origin/main HEAD true, GitHub reports MERGEABLE) in a fresh worktree.

Commands:

  • git fetch origin main && git fetch origin bb/fix-1302-sidebar-bootstrap && git checkout -b verify-1302-r2 FETCH_HEAD; pnpm install --frozen-lockfile --prefer-offline; pnpm exec turbo run build.
  • Fail-before: git checkout origin/main -- <13 non-test source files> then pnpm exec vitest run src/hooks/realtime-cache-effects.test.ts (apps/app): 1 failed / 56 passed, AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times ("patches cached thread list status from notification metadata instead of refetching the sidebar bootstrap"). pnpm exec vitest run test/services/threads/lifecycle-outcome.test.ts (apps/server): 3 failed / 1 passed; the broadcast metadata was { projectId } only (- "statusChange": { … } in the assertion diff), expected undefined to be 'waiting-for-host', expected undefined to deeply equal { activeGoalCount: 1, activePlanModeCount: 1, … }.
  • Revision check: with the first draft's realtime-cache-effects.ts (fdfaec771) checked out, -t "bare status-changed follows" fails with AssertionError: expected { activity: { …(5) }, …(5) } to be { activity: { …(5) }, …(5) } // Object.is equality.
  • Pass-after (git checkout HEAD -- …, tree clean): app file 57/57, server file 4/4.
  • pnpm exec turbo run typecheck --filter=@bb/domain --filter=@bb/db --filter=@bb/server --filter=@bb/app --filter=@bb/integration-tests --filter=@bb/mobile --filter=@bb/sdk --filter=@bb/desktop: Tasks: 12 successful, 12 total.
  • pnpm exec turbo run test --filter=@bb/domain --filter=@bb/db --filter=@bb/server --filter=@bb/app --continue: domain 136, db 405, app 3160 (3 skipped) passed; server 1826 passed / 1 failed = test/internal/internal-skill-trees.test.ts (mode: 420 vs 436, the known local umask 0002 artifact; passes in CI).
  • CI on the PR: all checks pass (Checks, Package Smoke x2, Tests app-1/2/3, integration, packages, server).

Repro on the fixed branch (own dev instance :18681/:26681/:34681, scratch project, one codex thread, headless Chromium with a window.fetch logger installed after load, pnpm bb:dev thread tell <id> "Reply only with ok." from the shell, 100 ms DOM poll of the sidebar row): three sends, each 9-10 API requests and 0 GET /api/v1/sidebar-bootstrap (report on main: 19 requests, 2 bootstrap downloads = 96% of bytes). The sidebar row showed Thread working about 100 ms after the send, Unread thread succeeded at turn end and cleared after the read receipt, all from the pushed patch. No longer reproduces.

Review notes: server-to-app contract only; the host daemon does not consume thread changed messages, so no HOST_DAEMON_PROTOCOL_VERSION bump is needed; every inbound consumer (app, mobile, desktop, sdk) uses the lenient schema; thread lists are ordered by pin/createdAt and not filtered on status, so patching cannot change membership; all applyThreadLifecycleEvent callers updated. Residual (documented in the body): in-transaction producers (stop, command failure, thread-start success, interruption, env cleanup, host reconnect) still push the bare kind and refetch the whole bootstrap; the plan-mode glyph at turn start stays a pre-existing race; the 138 KB payload shape and single sidebar key (report parts 2 and 3) are untouched, so a reviewer may prefer to keep #1302 open for the payload trim. Nit: thread-runtime-display.ts L268-270 is not prettier-formatted (CI does not enforce it).

AGENT GENERATED: by Claude Opus 5

Rebase

Rebased onto origin/main 75d6fc4d4 (was 32 commits behind at c942421a4) and squashed the two commits into one (766f1928f); the commit message keeps the original subject and body and folds in the revision-round notes (activity on the push, last-writer-wins merge). git rebase applied cleanly with no conflicts: none of the 32 commits on main touched the 23 files in this diff. The commits on main in the neighbouring areas (apps/server/src/services/threads, packages/domain/src, apps/app/src/hooks) are the provider v3 contract stack (#2124, #2136, #2148, #2164, #2179), the late tool-call completion fix (#2176) and the acp fork capability change (#2150); they do not touch the lifecycle writer, the thread change-kind schema, or the realtime cache registry, so the fix maps onto the new base unchanged. origin/main still has no statusChange in change-kinds.ts or realtime-cache-registry.ts.

Re-proved on the new base (committed tree, git status --porcelain empty):

  • Fail-before: with the 13 non-test source files checked out from origin/main, apps/app realtime-cache-effects.test.ts: 1 failed / 56 passed, AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times; apps/server lifecycle-outcome.test.ts: 3 failed / 1 passed (expected undefined to be 'waiting-for-host', expected undefined to deeply equal { Object (activeBackgroundAgentCount, ...) }). Pass-after: 57/57 and 4/4.
  • pnpm exec turbo run typecheck --filter=@bb/domain --filter=@bb/db --filter=@bb/server --filter=@bb/app --filter=@bb/integration-tests --filter=@bb/mobile --filter=@bb/sdk --filter=@bb/desktop --filter=@bb/host-daemon --filter=@bb/cli: Tasks: 14 successful, 14 total.
  • pnpm exec turbo run test --filter=@bb/domain --filter=@bb/db --filter=@bb/server --filter=@bb/app --continue: domain 27/27 files, db 28/28 files; server 1899 passed / 2 failed; app 3190 passed / 3 failed (3 skipped). The machine was under a load average of 40-70 from parallel agents: every app failure and the plugin-update.test.ts server failure were Test timed out in files unrelated to this change (different files on each of two runs), and each passes when rerun alone (135/135, 27/27). The one remaining server failure is test/internal/internal-skill-trees.test.ts (mode: 420 vs 436), the known local umask 0002 artifact that passes in CI.

AGENT GENERATED: by Claude Opus 5

Independent verification (guards)

Verified head 17746125f (rebased onto origin/main 27d1017fe; git merge-base --is-ancestor origin/main HEAD true; GitHub reports MERGEABLE / CLEAN) in a fresh worktree. Scope: confirm the post-verification change is only the CI-guard fix, re-prove fail-before/pass-after on the new head, confirm CI.

  • Interdiff: git diff 75d6fc4d4 766f1928f (previously verified patch) vs git diff origin/main 17746125f differ by exactly two hunks: packages/domain/src/plugin-sdk-version.ts PLUGIN_SDK_VERSION = "0.4.11""0.4.12" and packages/plugin-sdk/package.json "version": "0.4.11""0.4.12". No other line of the PR changed. origin/main and npm view @get-bb/plugin-sdk version are both 0.4.11; @get-bb/plugin-sdk@0.4.12 is 404 on npm, so the patch bump targets the next unpublished version. The commit keeps the original subject, body, and Co-Authored-By trailer.
  • CI on 17746125f: all checks pass (Checks, Package Smoke x2, Tests app-1/2/3, integration, packages, server, Version Lockstep x2; Node Compatibility Smoke and iOS simulator flows skipped by design). The Check plugin SDK npm version guard step logs npm version guard: PASS — @get-bb/plugin-sdk@0.4.12 is not on npm yet. The publish job will ship this version.
  • Fail-before on the new head (git checkout origin/main -- <15 non-test source files>): packages/domain change-kinds.test.ts 2 failed / 6 passed (ZodError on the maximal strict thread fixture, expected [ 'backgroundActivityChanged', …(4) ] to deeply equal [ …(3) ]); apps/server lifecycle-outcome.test.ts 3 failed / 1 passed (assertion diff shows the broadcast metadata is { projectId } only, - "statusChange": { activity, latestAttentionAt, runtime, status, updatedAt }); apps/app realtime-cache-effects.test.ts + cache-owner-registry.test.ts 2 failed / 59 passed (AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times at realtime-cache-effects.test.ts:1905).
  • Pass-after (git checkout HEAD -- …, git status --porcelain empty): domain 8/8, server 4/4, app 61/61. pnpm exec turbo run typecheck --filter=@bb/domain --filter=@get-bb/plugin-sdk: Tasks: 5 successful, 5 total. pnpm exec turbo run build: Tasks: 18 successful, 18 total.
  • Repro on the fixed branch: not re-run this round; the fix code is byte-identical to the head whose browser repro (0 GET /api/v1/sidebar-bootstrap per send) is recorded above.

Residual risks unchanged from the sections above. This PR no longer carries an SDK version change; main's unpublished 0.4.13 covers it.

Rebase (2026-08-21)

Rebased onto main at d41d1abee. Only packages/domain/src/plugin-sdk-version.ts and packages/plugin-sdk/package.json conflicted, because main moved the SDK from 0.4.12 to 0.4.13. Both were resolved to main's values, so the version files have dropped out of this PR's diff entirely (25 changed files -> 23). No other line of the fix changed.

Re-verified on the new base: node packages/plugin-sdk/scripts/check-npm-version-guard.mjs -> PASS - @get-bb/plugin-sdk@0.4.13 is not on npm yet. pnpm exec turbo run typecheck --filter=@bb/app --filter=@bb/server --filter=@bb/db --filter=@bb/domain --filter=@bb/integration-tests: Tasks: 9 successful, 9 total. pnpm exec turbo run test for app/server/db/domain: domain 27/27 files, db 28/28, server pass; @bb/app reported one failure in PromptBoxInternal.test.tsx > selection reveal, which passes on its own re-run and touches no file in this PR (the app changes are confined to src/hooks/cache-owners/). Treated as load-dependent flake; CI is the arbiter.

AGENT GENERATED: by Claude Opus 5

@SawyerHood
SawyerHood force-pushed the bb/fix-1302-sidebar-bootstrap branch from 9beae8b to 1e1e56f Compare August 21, 2026 07:38
@SawyerHood
SawyerHood marked this pull request as ready for review August 21, 2026 07:51
@SawyerHood
SawyerHood force-pushed the bb/fix-1302-sidebar-bootstrap branch from 1e1e56f to 766f192 Compare August 21, 2026 16:48
@SawyerHood
SawyerHood marked this pull request as draft August 21, 2026 17:12
@SawyerHood
SawyerHood force-pushed the bb/fix-1302-sidebar-bootstrap branch from 766f192 to 1774612 Compare August 21, 2026 17:18
@SawyerHood
SawyerHood marked this pull request as ready for review August 21, 2026 17:48
Every thread lifecycle transition broadcast a bare `status-changed`
dirty flag, and the app's only possible response was to invalidate the
single sidebarNavigation query plus every cached thread list. That is a
full GET /api/v1/sidebar-bootstrap (about 1 KB per unarchived thread,
134 KB on the seeded database) twice per turn, for every thread that
runs a turn.

The notification now carries the post-transition row fields (status,
runtime, activity, latestAttentionAt, updatedAt) as
`metadata.statusChange`. The server builds it in one place
(`buildThreadStatusChangeMetadata`), which also resolves the runtime
from host connectivity and the status-gated plan-mode and goal activity
with the same helpers as the list endpoints, so the db writer no longer
notifies and `applyLoggedThreadLifecycleEvent` owns the push. The app
patches the cached list rows and the sidebar bootstrap in place,
restarts any list fetch already in flight so a stale snapshot cannot
overwrite the patch, and keeps the refetch path only for pushes without
the row (older servers, writers inside a transaction that buffer their
notifications).

While the document is hidden, a later `status-changed` that carries no
row snapshot replaces (drops) an earlier one, so the resume flush falls
back to the refetch instead of patching the row to a stale status.

Server-to-app contract only; the host daemon does not consume thread
change notifications, so HOST_DAEMON_PROTOCOL_VERSION is unchanged.
Old clients ignore the new field (lenient inbound parse) and keep
refetching; a client that does not know a future status value drops
just the field and refetches.

Fixes #1302

Co-Authored-By: Claude <noreply@anthropic.com>
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.

sidebar-bootstrap ships 138KB and refetches wholesale on thread status changes

1 participant