Deepen conversation fold/feed/bus modules from architecture review - #141
Conversation
Scope click-target on ToolRow to its header instead of the whole row, so future interactive children (like DiffRow's open diff body) don't need to remember stopPropagation. Consolidate the seq/sub ordering comparator used by entries.ts and thread-feed.ts into one order-key module, and fix getTurnStartedAt to order by seq/sub instead of a skewed createdAt — the same bug class just fixed in fold.ts one file over. Consolidate turn-terminal tracking behind one shared predicate and settle pending waitForTurnEnd() waiters from the single cache commit chokepoint, instead of two independent trackers kept in sync by convention across three call sites. Wire watchThread's snapshotHighWaterMark into a cache-invalidation check so entries persisted just before a peer starts watching are no longer silently missed. Lock in ThreadEventBus's replay-log eviction behavior with a test (deltas already never enter the log, so eviction is delta-first by construction). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change centralizes conversation ordering and terminal-event handling, adds snapshot watermark recovery, updates work-log expansion behavior, and tests per-turn queue replay limits. ChangesConversation synchronization and ordering
Work-log expansion interaction
Queue replay retention coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/chat/work-log/tool-row.tsx`:
- Around line 71-83: Add aria-expanded={open} to the same header div props
guarded by canExpand, alongside role, tabIndex, onClick, and onKeyDown, so the
disclosure exposes its current expansion state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0175440-f3e7-47f5-b8bf-d31cf12eb28b
📒 Files selected for processing (16)
apps/cli/src/queue/bus.test.tsapps/web/src/components/chat/work-log/diff-row.tsxapps/web/src/components/chat/work-log/tool-row.test.tsxapps/web/src/components/chat/work-log/tool-row.tsxshared/hooks/src/conversation/conversation-cache.test.tsshared/hooks/src/conversation/conversation-cache.tsshared/hooks/src/conversation/use-thread-conversation.tsshared/hooks/src/conversation/use-thread-turns.tsshared/hooks/src/conversation/use-worker-conversation-sync.tsshared/utils/src/conversations/entries.test.tsshared/utils/src/conversations/entries.tsshared/utils/src/conversations/order-key.test.tsshared/utils/src/conversations/order-key.tsshared/utils/src/conversations/thread-feed.test.tsshared/utils/src/conversations/thread-feed.tsshared/utils/src/conversations/turn-waiters.ts
Addresses CodeRabbit review comment on PR #141: the expandable header div sets role="button" but never exposed its open/closed state to assistive tech.
|
Addressed in 93c4be5: added |
Summary
Follow-up to #140, from an
/improve-codebase-architecturereview of the conversation-folding / thread-feed / thread-event-bus layer.DiffRow's header button, but the open diff body (PatchDiff) had no such guard — any interactive element inside it still bubbled up and collapsed the tool row. Scoping the click target removes the need for any descendant to rememberstopPropagationat all (andDiffRow's is now removed as dead defensive code).entries.tsandthread-feed.tseach implemented their own seq/sub comparator; extracted into a sharedorder-key.ts. Also fixesgetTurnStartedAt, which still ordered a turn's first activity bycreatedAtstring comparison — the same bug class this PR's own follow-up fixed infold.ts'slatestTurnIdFromEntries, just one file over and unfixed.turn-waiters.ts,conversation-cache.ts,use-worker-conversation-sync.ts), each with its own copy of the same predicate, and settled via two independent trackers (turnWaitersMap,completedTurnKeysSet) kept in sync only by convention across multiple call sites. Now one sharedisTerminalEventpredicate, andcommitChunksettles any pendingwaitForTurnEnd()waiter automatically whenever a terminal chunk is committed, instead of relying on callers to remember a second manual step.snapshotHighWaterMarkinto a staleness check —watchThreadreturns a watermark for exactly this purpose (ADR-0008), but no client ever read it. Entries persisted just before a peer starts watching a thread could be silently missed. NowuseThreadConversationcompares the watermark against the cached snapshot on watch and invalidates to catch up if needed.ThreadEventBuseviction behavior with a test — investigated as a candidate "delta-first eviction isn't implemented" (ADR-0008), but found the property already holds by construction: ephemeral deltas never enter the bounded per-turn replay log in the first place, so there was nothing to fix. Added the missing eviction test and a comment documenting the invariant.Test plan
bun run test:unit— 220 tests pass (vitest + bun test), including new regression tests for each fixbun run check:types— clean across all 17 packagesultracite checkon all touched files — cleanSummary by CodeRabbit