Repository navigation
fix(sidebar): write a read finish back when another tab removed it unheard - #1797
Merged
Merged
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
Merge pass: a gate reviewer on a different model from the author's reviewed this fix and found no blocking issues.
CI: the first browser run failed All required checks are green and no review thread is open. |
Owner
Author
|
Merging without a CodeRabbit review: CodeRabbit was rate-limited, so no review ran. The merge pass reviewed it instead. None All required checks are green and no review thread is open. |
devswha
added a commit
that referenced
this pull request
Oct 28, 2026
…nish-index code (#1539) (#2183) ## Change Closes #1539, the follow-ups from the merge pass's review of #1536. I re-checked each item against main 97f957b: 1. **Code kept for no current use** (`src/lib/seenFinishes.ts`, `src/lib/sidebarActivity.tsx`), removed: - `hydrate()`'s `if (!refreshing) ... stored = false` loop. Every path that marks the index unhydrated without a refresh (first load, a different storage, a storage clear event, the last subscriber leaving) empties `records` first, so the loop never had anything to reset. - `cleanSeen()` with no arguments, which forced a fresh storage snapshot. Only a test called it, and `hydrate()` already removes old-format and over-limit keys. `retired` is now required, and that test reaches the same cleanup through `loadSeen()`. - The `epochs` map's LRU capped at `SEEN_LIMIT`. The map is keyed by machine ID (one entry per PC in the roster), so it is now a plain `set`. 2. **Past 2,000 records, an old read finish turns unread while you watch.** This is kept on purpose (agreed with Dori), and a comment on `trimRecords()` now says so. It takes 2,000 other panes read since that one. Keeping evicted records for the page's life would bring back the per-update merge that #1536 removed. After a reload the finish shows unread anyway, as it did before. 3. **Stale cache with no subscriber mounted:** already fixed by #1797. 4. **A failed share is not retried if the answer fingerprint changes before the retry:** already fixed by #1747, which made retries timer-driven. A same-ID entry keeps its pending retry, and a newly read answer replaces the entry with one that waits for the same timer. On current main a same-ID fingerprint can only go from "none" to an answer: Codex re-dating is accepted without changing the remembered answer. A new test pins that this retry happens once and carries the answer. 5. **A PC with no finishes is not cached:** `recordFor()` now caches the empty record as well, so each roster update no longer walks up to 2,000 records for such a PC. The empty record is invalidated wherever a non-empty one is: `cacheChange` on a new ID, `trimRecords`, `hydrate`, storage events, storage loss and the last unsubscribe. `cleanSeen()` also drops retired scopes from that cache. The audit doc no longer cites a `/tmp` path for its disposable harness. No visual change, no new setting, and no change to the HTTP/WebSocket contract or the stored record format. **Design precedent:** none in the UI; this is internal bookkeeping. ## Validation - New `seenFinishes.test.ts` case "walks the index once for a PC with no finishes, and sees the first finish written for it". It **failed on main** (`Map.prototype.values` called once on a repeated empty lookup) and passes now. It also checks that the first `saveSeen` and `receiveSeen` into that scope show up. - New hook case "retries a failed share once, with the answer fingerprint that arrived before the retry". It passes on main and after. It fails when the retry is dropped while a backoff timer is pending (mutation checked, then restored). - New hook case "does not cache a retired epoch's scope again while it retires" (the Codex review's one LOW). Without the second commit, `cleanSeen()`'s notification ran the subscriber's state update at once, and that update read the retired epoch and cached its scope again: one entry per epoch change until the last subscriber left. No dot was wrong. The fix records the new epoch before `cleanSeen()` and makes the subscriber skip an epoch that is no longer the PC's current one. This test **failed before that fix** and passes now. - `bun test src/lib/seenFinishes.test.ts src/lib/sidebarActivity.hook.test.ts src/lib/panePresentation.test.ts`: 78 pass, 0 fail. - `bun run check fast` on the head: passes (workflow syntax, generated types and themes, typecheck, build, 7929 unit tests with 0 failures). - `bun run check run --build bun scripts/sidebar-activity-demo-regression.ts` on the head: 44 scenarios pass, 0 fail. - Reviews: omo-native-code-reviewer PASS on the first commit (three LOW notes, none needing a change). Codex Sol 6.1 Fast xhigh found one LOW (the retired-scope entry above), and its re-review of the fix passed with no findings. The items above were re-checked against main 97f957b; the three commits main gained since touch none of these files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change
Part of #1539 (item 3; the other items stay open).
A read finish could turn unread again. Here is how:
storagelistener),receiveSeenstored a finish this browser was told about.ovis:finish:key.write()saw its cached record markedstoredand returned early, writing nothing.The acknowledgement was lost. The DONE dot came back after a reload and in other tabs.
write()(src/lib/seenFinishes.ts) now checks storage before that early return, but only while no subscriber listens (listeningWindow === null). If storage no longer holds a record at least as new as the cached one, it falls through to the normal merge, which writes the cached record back. That keeps the newer id when the cache is ahead.With a subscriber mounted, the early return is unchanged. Its storage listener keeps
storedtrue to what other tabs do, and "uses the warm snapshot for multiple PCs and presentation-only updates" pins zero storage reads for repeated acknowledgements. An attempt that always read storage broke that test, so the check is scoped.Not covered: a same-tab clear while a subscriber is mounted (the issue's last clause). The app never clears
localStorageor removesovis:finish:keys outside this module, so only devtools or the browser could. Handling it would cost the warm path a storage read per acknowledgement. With no subscriber mounted, a same-tab clear is covered by the same check.Design precedent: none in the UI; this restores the record the sidebar already shows as read.
Validation
New
src/lib/seenFinishes.test.ts(a Map-backedlocalStorageper test):Also:
seenFinishes,sidebarActivity.hookandpanePresentation.bun run check fastpasses: workflow syntax, generated types and themes, typecheck, build, and 6305 unit tests with 0 failures.