Skip to content

stewards: publish the realtime signal on every sidebar-visible change (BB-90) - #120

Merged
trotterdylan merged 2 commits into
mainfrom
bb/bb-90-publish-sidebar-signal-on-every-visible-bi-thr_3dyxefwetf
Oct 27, 2026
Merged

trotterdylan merged 2 commits into
mainfrom
bb/bb-90-publish-sidebar-signal-on-every-visible-bi-thr_3dyxefwetf

Conversation

@trotterdylan

Copy link
Copy Markdown
Collaborator

Implements BB-90. The stewards realtime channel was only rung when the "needs you" set changed, so a steward moving stage, going on or off hold, writing a status report, changing its requeue time, a deployment watch changing state, or a worker starting or finishing reached the sidebar and board up to 30 s later, on the next poll. This widens the signal to every sidebar/board-visible binding change and subscribes the overview to it too.

Merging this moves the bb-plugins pin, builds a new bb-server image and stages a restart for every developer's server (at Restart now, or in the 03:00–05:00 ET quiet window).

Classification

no-review: a tweak that makes the sidebar do what it already meant to do, sooner. Nothing in the RPC shape or the channel string changes; no owner-reviewed trigger applies.

What changes

  • store.ts: patch compares the row before and after and rings the channel when one of the sidebar/board-visible columns changed (stage, phase, hold, hold_behind, status, pending, requeue_at, advanced_at, deployments, title, url, ending, ended_at, thread_id). Per-turn bookkeeping (lastJudgmentAt, lastSnapshot, wokeAt, awaitingReport, missedReports, archiveWhenIdle, manualDetach, model-check flags, …) and same-value writes stay silent. addWorker and setWorker ring too. The existing per-operation coalescing / notifyDepth is unchanged.
  • note() writes the queued-notes column without a signal: a note is a reason for the next judgment turn, not a page change, and its one visible effect (the "worker finished" phrase) is published by setWorker when the worker's status changes. Ringing here would double-signal a worker finish and break the per-operation coalescing. patch still compares pending for any other write.
  • channels.ts: NEEDS_YOU_CHANGED → PROJECT_CHANGED; the channel string stays needs-you-changed so a server and client built from different images keep talking across a restart.
  • store.ts / server.ts: onNeedsYouChange / needsYouChanged → onProjectChange / projectChanged.
  • app.tsx: useOverview (the Status settings section) subscribes to the channel and refetches, alongside the sidebar and board page. The 30 s poll stays the fallback everywhere.

Testing

  • npm run typecheck in plugins/stewards: pass.
  • npm test in plugins/stewards: pass (710 tests; the plugin is in the CI matrix). New cases cover a stage change, a hold set and lifted, a status report, a deployment state change, a worker added and finished (one signal each), a lastJudgmentAt-only patch and a same-value visible patch (nothing), and the overview refetching on the channel. The existing coalescing test (answer + resolve → one signal) stays green.
  • Not tested: the real bb host's realtime delivery; the app tests drive the SDK's fake realtime slot.

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Independent review running in bb thread @thread:thr_j7i5hemh3a.

— from bb thread thr_3dyxefwetf

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 1 (GLM 5.3, at 75fe69b)

Owner review required: no — none of the owner-reviewed triggers apply. The change is a behaviour tweak inside a live plugin's runtime code: no store migration is added (the decide list's bb-plugins/plugins/*/store.ts entry is about migrations), source-contract.ts, the channel string, the RPC shape and dependencies are untouched, and nothing here can stop a server starting. Strictly additive signal widening plus the constant rename.

Blockers

  1. plugins/stewards/server.test.ts — the ticket requires "new server.test.ts cases cover each change type", but only 7 of the 14 visible columns get a case: stage, hold, status (+advancedAt), deployments, endedAt (via the updated BB-46 end test) and the worker paths are covered; phase, holdBehind, requeueAt, title, url, ending, threadId and pending have no case. pending matters doubly: the criterion names it visible, yet the only write exercised is note()'s silent bypass — there is no test that a patch({ pending }) rings (server.ts:2190/2473 do this), and none pinning the note()-stays-silent decision the PR body argues for. The mechanism is one generic comparison, but these cases are exactly what would catch a future typo or deletion in VISIBLE_COLUMNS; a table-driven test over the missing columns plus one note()-silence case would close it in ~30 lines.
  2. plugins/stewards/README.md / DESIGN.md — both still document the old signal scope. README §"The needs-you items and the picker count (BB-46, BB-85)" says the channel publishes "whenever a question is opened or answered, an escalation is filed … a need is filed, updated or closed, or a binding ends or reopens"; DESIGN.md §"Realtime, not the poll" names onNeedsYouChange(projectId) and enumerates the old firing points. After this merge both misdocument the channel (wider scope, patch/addWorker/setWorker firing, the note() exception, the overview subscription). In the no-review lane nobody else reads this before merge, and this repo's agents treat the plugin README/DESIGN as the manual.

Suggestions

  • server.ts:4239-4252 — the quiet worker-finish note (whose text drives the visible "worker finished" phrase) is written after await closeRequesterNeed and await archiveConsumedReviewers, while setWorker's signal fires before them. A refetch triggered by that signal can read the store before the note lands, showing an intermediate phrase ("no report yet" / a stale nextAction) until the 30 s poll — the exact lag this ticket exists to remove. Moving the store.note(...) above the two awaits, or ringing from note() when the note adds/removes a quiet worker finish, closes the race; the "double-signal" cost is one idempotent refetch.
  • store.ts (setWorker) — it rings unconditionally, so a duplicate thread.idle for an already-finished worker publishes a signal with no visible change. A before/after compare like patch's would make it uniform. Harmless as is.
  • The note() comment says ringing there "would … break the per-operation coalescing" — in the idle path note() runs outside the operation that rang setWorker (awaits in between), so at worst it would add one extra signal, not break coalescing. Worth rewording if the silent-note decision is kept.

Checked

  • Ran: npm run typecheck (pass) and npm test (710/710 pass) in plugins/stewards. The plugin is in the CI matrix and its checks pass on this head; I ran the suite myself anyway.
  • Verified against ticket: VISIBLE_COLUMNS matches the ticket's visible list exactly (14 columns; lastJudgmentAt, lastSnapshot, wokeAt, awaitingReport, missedReports, archiveWhenIdle, manualDetach and the model-check flags excluded, and setWorkerModelCheckPending stays silent) ✓; addWorker/setWorker ring ✓; coalescing/notifyDepth untouched and the answer+resolve one-signal test stays green ✓; rename complete with no needsYouChanged/onNeedsYouChange stragglers in code, channel string still needs-you-changed ✓; useOverview subscribes with the 30 s poll as fallback, and app.test.tsx covers the overview refetch on the channel ✓; diff is plugins/stewards only, RPC shape and contract copies untouched ✓.
  • Also checked adoption still signals: create() itself does not ring, but the steward thread's thread_id patch (server.ts:1730) does, so a brand-new sidebar row arrives with a signal.

_— from bb thread thr_j7i5hemh3a

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Round 1 addressed in 4d13df0.

Blockers

  1. Missing per-column tests — fixed in 4d13df0. Added a table-driven case over the remaining visible columns (phase, holdBehind, requeueAt, title, url, ending, threadId, pending), each asserting exactly one signal; a note()-stays-silent case; and a duplicate setWorker same-status case asserting nothing. The patch({ pending }) case is now covered too, so a VISIBLE_COLUMNS typo or deletion fails.
  2. README.md / DESIGN.md — fixed in 4d13df0. Both now describe the widened scope, the patch/addWorker/setWorker firing, the note() exception, the renamed onProjectChange, the unchanged channel string, and the overview subscription.

Suggestions

  • Worker-finish note race — fixed in 4d13df0. The store.note(...) and its needsSteward computation now run before closeRequesterNeed/archiveConsumedReviewers, so the quiet-finish phrase is in the store before setWorker's refetch can read it.
  • Unconditional setWorker ring — fixed in 4d13df0. It now compares the worker's status before and after and stays silent on a same-status write; the test asserts a repeated setWorker("finished") publishes nothing.
  • Overstated note() comment — reworded in 4d13df0 to say it would add a second signal for the finish and a second event in the answer path, not break coalescing.

npm run typecheck and npm test in plugins/stewards: pass (712 tests).

— from bb thread thr_3dyxefwetf

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 2 (GLM 5.3, at 4d13df0)

Owner review required: no — none of the owner-reviewed triggers apply. Same shape as round 1 (behaviour tweak in a live plugin's runtime code plus docs and tests); no store migration, no contract files, no new dependency, nothing that can stop a server starting.

Resolved since round 1

  • Blocker 1 (missing per-column test coverage): resolved by 4d13df0 — a table-driven case now covers phase, holdBehind, requeueAt, title, url, ending, threadId and pending-via-patch (one signal each, named per column), and new cases pin note() staying silent and a duplicate setWorker staying silent. Every visible column in VISIBLE_COLUMNS now has a direct case.
  • Blocker 2 (README/DESIGN.md staleness): resolved by 4d13df0 — README §BB-46 and DESIGN.md §"Realtime, not the poll" now describe the widened scope, the rename to onProjectChange, the unchanged channel string, the visible-column set with the silent exceptions, and the overview subscription. I re-read both; they now match the code.
  • Suggestion 1 (worker-finish note race): resolved — the quiet-finish note is now written immediately after setWorker and before the closeRequesterNeed/archiveConsumedReviewers awaits, so the refetch the signal triggers reads the store with the note in place. I checked the refactor for behaviour drift: needsSteward uses the same inputs, the ended-binding guard still skips both the note and the archive path, and the only delta is that a binding that ends during the two awaits can now carry a note it would not have had — it is ended, so nothing renders it; harmless.
  • Suggestion 2 (unconditional setWorker ring): resolved — it now compares status before/after and stays silent on a repeat write (and staying silent on a same-status result change is right: the result is not sidebar-visible).
  • Suggestion 3 (overstated comment): resolved — the comment now says what ringing would actually cost.

Blockers

None.

Checked

  • Ran: npm run typecheck (pass) and npm test (712/712 pass) in plugins/stewards on 4d13df0; the stewards job in the CI matrix is green on this head too.
  • Verified against ticket (re-checked on the new head): visible-field set, addWorker/setWorker, coalescing, rename with the channel string unchanged, overview subscription with the 30 s poll fallback, plugins/stewards-only scope — all still met; the new tests are the ones the criteria asked for and would fail if patch's comparison were dropped.

_— from bb thread thr_j7i5hemh3a

@trotterdylan
trotterdylan merged commit 0e8244c into main Oct 27, 2026
20 checks passed
@trotterdylan
trotterdylan deleted the bb/bb-90-publish-sidebar-signal-on-every-visible-bi-thr_3dyxefwetf branch October 27, 2026 20:53
@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Merged: no-review lane, reviewer approved 4d13df0, checks green; merge commit 0e8244c.

This merge moves the bb-plugins pin, builds a new bb-server image and stages a restart for every developer's server (at Restart now, or in the 03:00–05:00 ET quiet window).

— from bb thread thr_3dyxefwetf

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