refactor(app): extract timeline staging helper#670
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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.
Code Review
This pull request extracts the createTimelineStaging logic from MessageTimeline into a standalone session helper and introduces a new test suite to verify its behavior. The frontend architecture manifest was also updated to document this staging PR. A bug was identified in the staging effect where an unconditional call to cancel() and the shouldStage check cause staging to prematurely abort when new messages arrive, leading to a visual pop; a code suggestion was provided to ensure staging continues correctly for the active session.
Perf delta summaryComparator: pass
|
1550833 to
f2519e5
Compare
e1948d2 to
6e302c1
Compare
f2519e5 to
ee885f6
Compare
6e302c1 to
510653c
Compare
510653c to
f690bf3
Compare
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/session/message-timeline.tsx, packages/app/src/pages/session/session-timeline-staging.test.ts, packages/app/src/pages/session/session-timeline-staging.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
Topology Update (2026-05-16)
Final base:
dev.Dependency status: root of the only retained stack. Child order: #671 -> #672 -> #674 -> #675.
Review order: review after #667/#669, then before #671. #667 and #669 are merged into
dev.Verification after rebase:
bun --cwd packages/app test --preload ./happydom.ts src/pages/session/session-timeline-staging.test.ts src/pages/session/use-session-history-window.test.ts src/pages/session/session-timeline-scroll-controller.test.ts src/pages/session/session-timeline-scroll-anchors.test.ts-> 1110 pass / 2674 expects;bun run typecheck-> 8 successful;git diff --check-> pass.Summary
createTimelineStagingfromMessageTimelineintosession-timeline-staging.ts.Why
This is the first #601 message-flow architecture slice after the governance and contract stack. The #601 audit identified
createTimelineStagingas a launch-path contract inside the oversizedMessageTimeline; this PR gives that owner a small file and locks the existing behavior before later message-flow refactors.Related Issue
Refs #601.
Depends on #667 and #669, both now merged. Base branch is
dev.Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
MessageTimelinestill passes the samesessionKey,turnStart,renderedUserMessages, and{ init: 10, batch: 3 }config.--conditions=browserso Solid effects andsolid-js/webrender semantics are actually exercised.Risk Notes
Behavior should be unchanged except for the reviewed bug fix that preserves active staging when the same session receives more messages mid-stage. The main risk is staging timing around historical windows, completed sessions, rapid session switches, or streaming growth; the browser-condition tests cover those contracts. No dependency, storage, permissions, updater, packaging, or platform behavior changes.
How To Verify
Screenshots or Recordings
Not applicable: no visible UI or copy changes. Electron manual verification was not run because this is a behavior-preserving extraction plus tests, not a rendered UI surface change.
Checklist