Repository navigation
fix(dashboard): let reading a session clear a stuck unread dot - #19779
unstablebrainiac wants to merge 1 commit into
Conversation
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the change fixes the stuck dot at its source (a saved watermark that names no row), only on the read gestures, and the spec now states when the tombstone and marker can be removed. Notes
[DESIGN-REVIEWED] 8ad3def Description read[DESCRIPTION-READ] 5e048725f5222b00b8a24bee11a121fe268d408d1117df2cedfc2e2aeadfdaa2 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
UX Review (Opus 5.5, fork) — ✅ PASSUX-level review of UX-Verdict: PASS Nothing to check. The diff adds no control, label or copy. It only changes store and relay logic, so an unread dot that used to come back now stays cleared when you open a session or mark it read. [UX-REVIEWED] 8ad3def Description read[DESCRIPTION-READ] 4c2e539411d90ebbb702041e868b518d74a79dc112b8c39465cdc076ca280214 That sha256 names everything this verdict read: the PR description, plus the 3 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks: the fix has a traced cause (#19688) and tests, and changes no default. The tombstone-and-marker machinery is the one thing worth questioning. Notes
What this change shipsInventory (8 items) — 7 justifiedIntent: let a user dismiss an unread dot that keeps coming back on a session they have opened or marked read. FIX.
[FIRST-PRINCIPLES-REVIEWED] 8ad3def Description read[DESCRIPTION-READ] 221b704030ab7f014d3e8618a483ce2d324b8d8037767ad5c88619d82e486d73 That sha256 names the PR description this verdict read. Recompute it with |
GPT 6.1 Review (fork) — ✅ no blocking findings (all downgraded on adjudication)Reviewed Opus 5.5 adjudication downgraded every blocking finding. Downgraded: the remedy each one needs is disproportionate to the harm it prevents. Adjudication downgraded all 1 blocking finding(s): the remedy each one requires is disproportionate to the harm it prevents. Review detailsBLOCKING -- website/src/store/dashboardSlice.ts:1657 -- A superseded read deletes another window’s completed retirement Adjudication (Opus 5.5) — is blocking on each finding proportionate?F1 is downgraded: blocking the merge on it costs more than it would prevent.
|
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
af1f572 to
aafcc71
Compare
aafcc71 to
e0a07c4
Compare
e0a07c4 to
8052589
Compare
|
This PR lets opening a session or Mark as read retire a pre-#15014 saved unread watermark that names no gateway row: the read captures the dot's watermark, asks the gateway for a page built after the click, and when no row is as new records an |
8052589 to
58081c6
Compare
|
Replies on head 58081c6:
|
fb9d863 to
0cc75b1
Compare
|
Rebased onto main at 0cc75b1. The only conflict was the Dispositions on 0cc75b1:
Migration question: still open for a maintainer. If the protocol stays, I'll open the removal issue and link it in the description. If the migration is preferred, I'll replace the protocol with it in this PR. Key leaks: opened #21498 for the last slot deleted during a reconnect and a slot deleted while no window is open. It is linked in the description. |
0cc75b1 to
9d9969a
Compare
9d9969a to
ada3651
Compare
mbriones98
left a comment
There was a problem hiding this comment.
Checks green and readiness passed at e74bdad; no new feature.
|
Holding at e74bdad for a maintainer decision. The code still looks sound, and our r99 remote-delete finding stays fixed. #21498 tracks the two leak cases. What is still open:
Checks on this head are green. — crew-pr-reviewer (r110) |
|
HOLD at e949e0b. The code still looks sound and our r99 remote-delete finding stays fixed. A retire checks the arrival marker on both sides of the tombstone write (dashboardSlice.ts L1556-L1570). A live list that drops a slot prunes all three keys for it (L498-L500). #21498 covers the two leak cases. CI on this head is green. What clears the hold:
I read the diff and the test descriptions. I did not run the tests. — crew-pr-reviewer (r111) |
|
Dispositions on e949e0b:
|
|
HOLD at e949e0b. Thanks for the dispositions on this head (comment). The second Reader list in Backwards compatibility covers the One thing clears the hold: a maintainer decision on the question. The choice is between keeping the I read the description and test names. I did not run the tests. — crew-pr-reviewer (r112) |
|
HOLD at ec9be94 for a maintainer decision. This head is a rebase of e949e0b. The only changed hunk is import and signature context from main in What clears the hold:
— crew-pr-reviewer (r116) |
|
Replies on ec9be94:
|
|
HOLD at ec9be94 for a maintainer decision. Thanks for the replies on this head. They answer the r116 items:
One thing still clears the hold: a maintainer decision on the open question, keep the CI is green and the branch merges cleanly with main. No PR code was run locally. — crew-pr-reviewer (r117) |
|
HOLD at 281e923 for a maintainer decision. This head is a rebase of ec9be94 plus a move of the unread persistence helpers into One thing still clears the hold: a maintainer decision on the open question. Either keep the No PR code was run locally. — crew-pr-reviewer (r118) |
|
HOLD at 46a71b1 for a maintainer decision. This head is a rebase of 281e923 onto main's split of the unread store. The tombstone helpers now live in One thing still clears the hold: a maintainer decision on the open question. Either keep the No PR code was run locally. — crew-pr-reviewer (r122) |
|
HOLD at 284f9ec for a maintainer decision. This head is a rebase of 46a71b1. The only change is that What clears the hold:
No PR code was run locally. — crew-pr-reviewer (r125) |
|
Replies on cde8bbc:
|
A shared unread watermark newer than every row the gateway still reports can never be covered by a read, so opening the session clears the dot and the window's own slot_read echo lights it again. Builds before kirodotdev#15014 left such watermarks from a turn's unsaved done row. A read now retires the watermark its dot carried when a page the gateway built after the read began has no row as new as it: the transcript switchSlot already fetches, or a one-row page for Mark as read. A page whose rows carry no time, such as a turn still streaming, proves nothing. Mark as read relays the watermark it captured at the click, so a row that arrives while its page is in flight stays unread. A read that may retire first writes a fresh per-slot marker, and retires nothing if it cannot or if the marker changed while it waited: every new message removes the marker before the tombstone, and the retire checks the marker before its write and again after, removing the tombstone if it changed. The read removes its own marker when it is done. Removing the session, or an authoritative slot list that drops a session this window listed, removes both per-slot keys. The retire never rewrites the shared record, which another window may be writing. It records the disproved value in a key of its own per slot, and every reader of the record treats a matching entry as absent. The slot's next message removes that key, so a value that comes back after it shows a dot. Another window already showing the dot keeps it until that window reloads or reads the session.
|
HOLD at 8ad3def for a maintainer decision. This head integrates main's queued shared-unread writes and What clears the hold: a maintainer rules on the open question, keep the No PR code was run locally. — crew-pr-reviewer (r126) |
Problem / Motivation
Goal: Opening, or marking as read, a session clears its unread dot for good, even when the dot was saved with a time newer than every row the gateway still reports.
An idle session's unread dot goes out when the session is opened, then comes back a moment later. Mark as read does the same, and every reload restores it. The dot was saved by a build before #15014 with the time of a turn's unsaved
donerow. #19688 traces why no read on main can clear it.Why it matters
#15014 stopped new dots like this. It left the ones already saved in a browser, to clear on the session's next saved row. A session that gets no new row keeps a dot the user cannot dismiss, short of editing
localStorageby hand.Not a goal
What changed (motivation → approach → change)
An unread dot that came back after every read now stays cleared once the user opens the session or marks it read.
A saved dot clears only with a read at least as new as it, and a read carries the slot's
last_ts, the newest row the gateway saved. A dot saved with the time of a row the gateway does not have is newer than anylast_ts, so no read cleared it, and each read's own echo lit it again.The fix asks the gateway, not this window. A read takes the watermark its dot carried at the click. If a page the gateway built after the click has no row as new as it, that watermark names a row the gateway does not have. A real one is always in that page: the gateway adds a row to the slot's window before broadcasting it, and the page includes unsaved rows. A page whose rows carry no time, such as a turn still streaming, proves nothing, so the dot keeps its guard.
A disproved watermark is not deleted from the shared record, which another window may be writing at the same moment. It goes in its own per-slot key,
mc-unread-orphaned:<slot>, and every reader of the record treats an entry equal to it as absent. A newer watermark is a different value, so it shows.Deleting the disproved value instead was rejected. The record's covered-read clear, given the disproved time, deletes any entry at or below it, so it erases a real older watermark another window wrote. An exact-value delete rewrites the whole record, so it can erase a newer watermark written in between. Nor can it tell an orphan from an arrival that left the value in place.
A one-time sweep at boot was rejected too. It would ask the gateway about every saved dot on every window start until a versioned record lands, and it races arrivals exactly as a read does, so it needs the same marker. Fixing a dot on read asks only about the session the user opened. A versioned record would drop every saved dot once, real ones included. The spec section names the removal condition.
slotSwitch.tslast_tscannot cover it, writes an arrival marker. Once the transcript loads, it retires the watermark if the page has no row as new as it and the marker is unchanged, removes the marker, then relays the read as before.dashboardSlice.ts,dashboard/readSlot.tsreadSlot(key)does the same for Mark as read. Only whenlast_tscannot cover the watermark, it first fetches one row from the gateway, bounded byUNREAD_PROOF_TIMEOUT_MS(10 s; a later answer is no proof), then relays the watermark the click captured, so a row that arrives meanwhile stays unread. A fresh marker inmc-unread-arrival:<slot>brackets the read, and every new message removes it, then the tombstone. A new message on a slot holding a tombstone writes the shared record at once instead of joining main's queued write, so the disproved value leaves before its tombstone. A read whose marker changed or could not be written retires nothing, and the retire re-checks the marker after writing its tombstone, so a row any window recorded meanwhile keeps its dot.retireSlotUnreadwrites the tombstone and, once that write lands, clears this window's dot while it carries that watermark.dashboard/unread.ts,dashboard/selectors.ts,dashboard/readSlot.ts,unreadArrivalMarker.tsdashboardSlice.tsunder the 2,000-line cap; the slice re-exports what callers import.dashboard/unread.tsgains the tombstone keys,forgetUnreadSlotsand the proof helpers.dashboard/selectors.tsis new and takes the slice's read-side selectors unchanged.dashboard/readSlot.tsis new and holdsreadSlot; callers import it from there, because it dispatches the slice's own actions and a re-export from the slice would make an import cycle.unreadArrivalMarker.tsis new and holds the marker.slotReadRelay.tsemitSlotReadsplits intoslotReadWatermark, the watermark a read made now carries, andrelaySlotReadWatermark, which sends a captured one without adding later rows. Existing callers send what they sent before.useSessionActions.tsreadSlot.docs/system-specs/modules/app-notifications.mdBackwards compatibility
Compatible: the shared record's format is unchanged, the retire never writes it, and every gesture relays the same
slot_readtime as before. A disproved watermark keeps onemc-unread-orphaned:<slot>key until the slot's next message, andmc-unread-arrival:<slot>exists only while a read of that slot waits for the gateway. Removing a session, or a live slot list dropping it, removes both. The diff hides tombstoned entries from the record's readers:restores a newer watermark written after the retire)reload restores neither watermark nor badge while the shared orphan remains)keeps the local dot cleared through its own older slot_read echo)a new older arrival replaces a tombstoned shared value and restores its dot) or from the queue (an arrival queued before another window retires the slot replaces the tombstoned value when it lands)retireSlotUnreadposts the lowered unread count to the hub (posts the lowered unread count to the embedding hub)The
unlistedpurge inreconcileSlotsdeletes the shared entry, tombstone and marker of every listed slot a live-stream list drops, retired ones included. It is gated like eviction, so a stale HTTP reply removes nothing, and held like main's drain, so a list the gateway labels partial keeps a session it has not restored yet. Aremovedframe clears them whatever the hold, as main's drain does. Readers of what it deletes:a retired session another window deletes leaves no saved dot, and a stale reply removes nothing)a removed frame for a retired session leaves no saved dot)mc-unread-slotsprojection, as main's departed-slot drain does (drains unread state for a slot that vanished from the authoritative list)Tests
website/src/test/unreadOrphanedWatermark.test.tsboots a window from the record an older build left, with a slot row whoselast_tsis below the saved dot:slot_readecho, restores nothing on reload, lowers the hub count without counting a held orphan, and relays the click-time watermark only after the gateway answers; a row that arrives while the gateway answers stays out of that relay and keeps its dot across a reload, in this window or another, and an unsaved row received before the click is still in it; an arrival leaves no marker, a read writes its marker before it clears the dot, an arrival while a read waits keeps its dot, including one that lands while the retire writes its tombstone, a read that cannot write its own marker retires nothing, and a finished or failed read leaves no marker behind;last_tsalready covers it, including alast_tsa streaming turn moved only in main's mid-turn overlay;mc-unread-shared; a newer watermark another window writes before or after it is restored on reload, and so is one this window copied from another window's record before the read;removedframe without it, or a first slot list without an unread session drops its tombstone and marker, and a retired session another window deletes leaves no saved dot while a stale HTTP reply removes nothing; a partial slot list keeps an absent session's dot, tombstone and marker, and aremovedframe still clears a retired one's, before the first snapshot too;website/src/test/slotReadRelay.test.tsx: opening the session over the dashboard socket keeps the dot out after the gateway echoes the read and restores none on reload, and keeps the saved dot when the fetched page holds its row or has no row time, or when another window records a row during the load, with or without a gap against this tab's cached rows. It writes no marker for a dotlast_tscovers, including alast_tsheld only in main's mid-turn overlay, retires nothing when it cannot write its own, and leaves none behind when the load fails. The first fails with main'sslotSwitch.ts.website/src/pages/members/MembersPage.openThread.test.tsx: viewing a crewmate thread never asks the gateway about the dot.Each of the 61 safeguards in the tombstone readers,
recordUnreadOrphan,retireSlotUnread,readSlot, the arrival write, the read relay, the arrival marker,watermarkDisprovedBy,orphanedWatermark,newestRowTs,switchSlot, the slot-list cleanup and the Members page view was removed in turn, and a test failed every time. Run locally on this commit: these files, the 61 removals,tsc -p tsconfig.app.json, eslint, the i18n, phantom-class and restyle checks, the production build and the website tests related to the diff (419 files). The whole website test suite (2884 files) ran on an earlier revision of this change.Manual verification
N/A: unit coverage sufficient. The tests boot a window from the exact record the report describes and drive the dashboard's real socket handler, unread store and read relay.
Screenshots / video
Why no screenshot: the dot looks and lights the same; only when a read clears it changes.
Related Issues
Closes #19688
Follow-up: #21498 (the two cases where the per-session keys still outlive a deleted session).
Pattern harvest
Rule candidate: review-prompt
Pattern: a guard that waits for a later value to cover a stored one never releases when the stored value names a row that no longer exists.
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)