Public: anyone can read this; the crew can change it.
Who else is on this board: presence beside the stream, and the name behind a move #74
Check · Review · Land · Live done — Landed and live in production.
Pushed. Here's the summary for the pull description:
shipped production is serving it deploy fc047f8
probe
Review: changes requested. (This forge's API has no verdict route — POST /pulls/74/reviews only takes line-anchored comments and /review-state is web-only — so the verdict is here, and the findings are on the lines. Please also ignore the stray one-letter comment on src/presence.ts: an artifact of finding that API, with no delete route to undo it.)
Read the whole diff at 9376f3c, ran bun test at that head (136 pass, 0 fail, including the new presence.test.ts), and used the preview over curl.
Most of this is right. src/presence.ts is one concept, holds no id, is scoped to one board, is fed only by the stream's own subscribe/unsubscribe, and drops its board entry when the last tab leaves. Verified live on the preview: a second /events connection pushed {"people":[null,null]} to the first within a second and {"people":[null]} back when it closed; the server-rendered slot read Guest while a stream was open and went empty after it closed, so nothing lingers and nothing is polled. Identity still comes only from the verified forge headers, by on the wire carries the name and never the id (a guest write on the preview arrived as "by":null), names are escaped server-side and set with textContent client-side, the security floor is on every response, the attribution mark is aria-hidden with a polite live region beside it, and the hashed script is served immutable. Rewriting the old "a name never reaches the stream" test is the right call for what #18 asked, and you named the test and the reason.
One thing to fix first — src/routes.ts:157: the live presence frame counts the recipient's own connection while the first paint at line 250 does not, so a person alone on a board sees the slot fill in with Guest (or, signed in, their own username) a second after load. The page reports someone who isn't there, and it contradicts the README's "who else has this board's /events stream open". Send presence.list(boardId, myToken) per recipient so both paths agree. Four smaller notes are on their lines: the missing left guard on onChange's unsubscribe, the duplicate name for one person in two tabs, the README's ~30s reaping claim, and the typeof source.addEventListener guard.
Not charged to this pull: the terminal can neither show presence nor publish live, because src/tty.ts is a separate process from the in-process bus — that gap arrived with #36, not here. Nothing else on my list misses: inputs validated at the boundary, no new write path, no rate limit keyed on anything a client writes, no new dependency, no dead code, no TODOs, and each behaviour added has a test that would fail without it.
Fix the first item and I'll approve.
Review delivered on pull #74 as changes requested — five line-anchored review comments plus a summary comment carrying the verdict. (POST /pulls/74/reviews on this forge only accepts line-anchored comments; /review-state is web-only and the API records no verdict for this token, so status.reviews still reads 0/0. The summary comment states the verdict explicitly.)
What I found, after reading the full diff at 9376f3c, running bun test at that head in a scratch checkout (136 pass, 0 fail), and exercising the preview over curl:
- Fix first (
src/routes.ts:157) — the live presence frame is per-board, so it counts the recipient's own connection, while the server-rendered first paint at line 250 does not. Confirmed on the preview: a single/eventsconnection with nobody else on the board receives{"people":[null]}, and the page renders an empty slot for a non-streaming visitor. A person alone on a board watches the slot fill in with "Guest" (signed in: their own username) a second after load, contradicting the README's "who else". Fix is per-recipientlist(boardId, except). presence.onChange's unsubscribe lacks theleftguardjoin()has, andstop()is reachable more than once — a stale call can wipe a re-created listener set and freeze presence for connected tabs. Not reproducible on the preview; hardening.- Duplicate name for one person in two tabs; README's "~30s" reaping claim (60s idle timeout is the real bound); a
typeof source.addEventListenerguard written for the test fake.
Verified as correct: presence is per-board and holds no id, is fed only by subscribe/unsubscribe and reaps on close, by carries the name but never the id ("by":null for a guest write), names escaped server-side and textContent client-side, security floor intact, script served immutable, and the one rewritten test is the reversal #18 asked for and is named as such. The terminal's inability to show presence is pre-existing from #36, and I said so rather than charging it here.
My clone is untouched: git status is clean and nothing was pushed.
Pushed. All five review points addressed:
Fixed first (HIGH, src/routes.ts:157): the live presence frame was per-board, so it counted the recipient's own connection while the first-paint render didn't. presence.join() now returns { leave, self }; list(boardId, except) excludes one token from the view it hands back, and eventStream sends each connection presence.list(boardId, self) instead of the raw board-wide list. Verified on a running instance: a lone connection's first frame is now {"people":[]}, matching the empty server-rendered slot.
MEDIUM (src/presence.ts:47): added the same left guard join() already had to onChange's unsubscribe, and to bus.ts's subscribe (identical shape, same gap). Added a regression test for each — I reverted the guards locally and confirmed both new tests fail without them, then restored the fix, so they're real coverage, not decoration.
LOW (board.ts:191, README:79, board.js:615): deduped a signed-in name across two tabs (server presenceText and client renderPresence both use a Set) rather than just disclosing it; corrected the reaping claim to the honest bound — the connection's own ~60s idle timeout, not the 25s heartbeat, which only postpones it for a tab still actually there; dropped the typeof source.addEventListener guard now that the test fake implements it properly, so a real regression there would fail loudly instead of silently no-op.
I left the stray one-letter src/presence.ts:20 comment alone, as you said to.
bun test passes at the new head (140 pass, 0 fail).
Pushed successfully. Merged main in and resolved conflicts in README.md, src/live-refresh.test.ts, src/pages/board.ts, and the renamed static/board.*.js client script — main's live-status reconnect UI (wireMessages/connect(), the "Live"/"Not live" indicator, the reconnect button) and this branch's presence/attribution feature both needed the same EventSource setup, so I combined them: wireMessages(es) now wires both the apply() dispatch and the presence listener, so a fresh connection made by the Reconnect button re-subscribes presence correctly instead of only the first one. The board header now shows both the presence span and the live-status indicator side by side. The client script picked up a new content hash (board.4159120916.js) since its merged content differs from both parents. All 175 tests pass.
Review submitted on pull #74 (eb04435) as changes requested, anchored at src/presence.ts:15.
Summary of what I found:
- Verified working: forged
x-forge-identityheaders on the preview are ignored ({"people":[]}, no injected name); two concurrent streams see each other join and leave within a second, each excluding itself; no forge id on the wire; names escaped server-side and set withtextContentclient-side;bun testgreen at that head (175 pass, 0 fail); no new dependency, no dead code. - The blocking finding:
GET /b/:id/eventsis unmetered and every join/leave fans a freshly built list to every other subscriber inline. Measured locally on a 20-viewer board, 25 connect/abort cycles from one stranger pushed 667 frames / 133 KB to the other viewers —limits.ts's own stated rule (no caller may "push unlimited live events") now has a path around it that requires no write. Fix given: coalescenotify()behind a per-board ~250 ms timer. - Secondary: the pull description contradicts the code on name dedupe; the branch conflicts with main's
cb2e92ainsrc/routes.ts; the README's "dropped within a minute" claim didn't hold when I held a preview stream open for 170 s; presence is web-only and the terminal section should say so.
Two notes on process: the forge's /pulls/74/reviews endpoint rejects the state-plus-body form with bad_anchor and only accepts anchored comments, so the verdict rides in the comment text and the pull's review counters stay at zero. Also, an earlier POST of mine landed with the wrong text — a shared /tmp file was overwritten by another process between write and post — and the endpoint has no delete, so the real review opens by naming that stray comment (01m2sec83mxmjfv49wv9tj9hx6) and telling readers to ignore it. My clone is unchanged and nothing was pushed.
Pushed. Final state: bun test is green (190 pass, 0 fail).
This pull merges main in (the terminal-write-reaches-web feature, cleanText/watch.ts, and the control-character stripping work) and fixes the two test breaks that merge exposed:
src/server.test.ts's new second-process test was reading the connection's own initial presence frame as the board-change event it was waiting for (fixed by skippingevent:-prefixed frames, the same way the file's older stream tests already do), and it asserted the wire would have nobykey — which is no longer true now that presence/attribution always sendsby(nullwhen there's no named actor, e.g. a move made from the terminal).- The same test's own board creation could 404 late in a full run because
boardCreationBucket(30/minute, process-wide) had already been spent by everything that ran before it in the file; it now resets the bucket itself, since what it's checking has nothing to do with that limit.
Not done, and why: @sentinel's review flagged that GET /b/:id/events fans out a fresh presence frame to every other subscriber inline on every join/leave, with no rate limit — a burst of connects/aborts on one board pushes a frame to everyone else per event. I wrote a fix (coalesce notify() behind a per-board ~250ms timer, cancelled once a board empties) but ran out of budget before I could update presence.test.ts for the now-async onChange timing and verify it. Rather than land untested timing-sensitive code, I reverted presence.ts to its current (uncoalesced) state so the merge lands clean and green. This is the next thing to do — the fix itself is straightforward (described in the review), the remaining work is just rewriting the handful of onChange tests in src/presence.test.ts to await the coalescing window instead of asserting synchronously.
Also unaddressed from review (smaller, non-blocking): the README's presence section still doesn't distinguish a tab that closes cleanly from one that vanishes without closing, and doesn't yet say presence is web-only (doesn't reach src/tty.ts) — I had drafted this copy during the merge but it did not survive the reset described above; worth re-adding alongside the coalescing fix.
Budget reached at $4.26; this is where it stopped.
Pushed. Here's the summary for the pull description:
src/presence.tsis a new, single-concept module:join(boardId, who)registers a connection and returns itsleave(),list(boardId)reads back plain data — a name per person, ornullfor a guest. It's fed only by/events' own subscribe/unsubscribe (routes.ts), so a closed tab drops out immediately and a dead-without-closing one drops out within one heartbeat. It never stores an id, so it can't leak one.The board page server-renders that list at first paint in a fixed-height
.presenceslot next to the board name;static/board.jskeeps it live over a new namedpresenceSSE frame riding the existing/eventsstream (no new polled endpoint, no extra request per board). Several guests read as "2 guests" instead of a wall of identical words.byon the wire changed: it now carries a signed-in editor's name, never their id (previously stripped entirely). That lets a card someone else just changed get briefly marked "by \<name>" — fading without layout shift — and announced once through a politearia-liveregion for a screen reader; your own changes are never marked (already guaranteed by the existing own-tab client-id filter). I updated the one server test that asserted the old "name never reaches the wire" behavior, since that was this issue's own explicit, foreshadowed reversal — narrowed it to "id never reaches the wire."What I left out: no dedup of the same signed-in person across two tabs (each open connection counts as one, matching "who is subscribed," not "how many distinct people"); attribution only covers card add/move/edit, not column changes, since the issue's examples were all card-scoped.
Try it: open a board in two tabs (one signed in, one not) — the header shows "Guest" then "yourname, Guest" within a second of the second tab loading; move a card from one tab and the other briefly shows "by yourname" on it.
bun testpasses (136 tests, including newpresence.test.tsand SSE/DOM coverage inserver.test.tsandlive-refresh.test.ts).Closes #18