refactor(app): extract timeline scroll intent helper#671
Conversation
📝 WalkthroughWalkthroughThis PR extracts reusable timeline scroll intent utilities (boundary detection, metric conversion, gesture factories) into a new module, then refactors message-timeline and session-timeline-staging to use the shared logic instead of local implementations. ChangesTimeline scroll intent extraction and consumer refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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.
Code Review
This pull request refactors scroll intent and metrics logic by extracting helper functions from the MessageTimeline component into a new dedicated module, session-timeline-scroll-intents.ts, and adds corresponding unit tests. The review feedback suggests widening the root parameter type from HTMLDivElement to HTMLElement for increased flexibility and recommends replacing hardcoded scroll thresholds with a configuration object to improve maintainability.
Perf delta summaryComparator: pass
|
e1948d2 to
6e302c1
Compare
14265e0 to
1692f00
Compare
6e302c1 to
510653c
Compare
1692f00 to
1b6320d
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-scroll-intents.test.ts, packages/app/src/pages/session/session-timeline-scroll-intents.ts, 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.
Goal: extract the per-message comment parsing and comment strip rendering from MessageTimeline into a focused session-message-comments helper without changing message-flow behavior.\n\nBoundary: scoped to packages/app/src/pages/session/message-timeline.tsx, session-message-comments.tsx, and focused comment/staging/scroll-intent tests. No scroll behavior, turn-change behavior, persistence, IPC, platform, dependency, or public export changes.\n\nRestack: rebased from codex/timeline-scroll-intent-contract onto latest dev after #671 was squash merged; verified the PR diff contains only this slice. Used --force-with-lease for codex/message-comment-strip-contract after confirming local HEAD matched origin before rebase and the worktree was clean.\n\nReview follow-up: Gemini bg token thread fixed and resolved by using bg-surface-base. Fresh-eyes/CI typography guard finding fixed by replacing old text-13 utilities with role typography. Final fresh-eyes review found no P0-P3 actionable findings. Review threads are resolved.\n\nVerification: bun test --preload ./happydom.ts src/pages/session/session-message-comments.test.ts src/pages/session/session-timeline-staging.test.ts src/pages/session/session-timeline-scroll-intents.test.ts -> 16 pass, 0 fail. bun test test/no-dead-tokens.test.ts from packages/ui -> 3 pass, 0 fail. bun run typecheck -> 8 successful. Targeted perf gate via bun script/e2e-local.ts -- e2e/perf/perf-probe.spec.ts --grep "long-session-input-lag|session-timeline-recompute" -> 1 passed, 1 skipped. git diff --check origin/dev...HEAD and git diff --check -> pass. GitHub required checks passed, including unit-ui-focused and perf-probe-baseline.\n\nResidual risk: low; Electron manual visual verification was not run for this behavior-preserving extraction.
Topology Update (2026-05-16)
Restored after #670 was squash-merged and its stacked base branch was deleted.
Final base:
devat #682 merge commit6c643a4b.Head after latest dev update:
d1f5af8doncodex/timeline-scroll-intent-contract.Dependency status: no longer targets the deleted #670 branch. This PR now carries only the #671 scroll-intent helper diff against
dev.Review order: review before #672, because #672 still stacks on
codex/timeline-scroll-intent-contract.Verification after latest dev update:
bun --cwd packages/app test --preload ./happydom.ts src/pages/session/session-timeline-scroll-intents.test.ts src/pages/session/session-timeline-scroll-controller.test.ts-> 1138 pass;git diff --check origin/dev...HEAD && git diff --check-> pass. GitHub typecheck/lint/unit/e2e/desktop smoke were rerunning on the updated head.CI after latest push: rerunning on the updated head. Fresh-eyes review P2 about staging scope was fixed by removing staging files from the current PR diff.
Summary
Extracted the
MessageTimelinescroll intent and metrics adapter intosession-timeline-scroll-intents.ts, then added focused unit coverage for the moved boundary.Why
This is the next small #595/#615 scroll-perf slice in the frontend architecture governance stack. It does not claim to solve scroll stutter or consolidate the full scroll owner yet; it isolates the adapter code so the later controller/dock ownership work has a smaller, testable boundary.
Stack order after #670 merge: #671 -> #672 -> #674 -> #675.
Related Issue
Refs #595
Refs #615
Depends on #667, #669, and #670, all now merged to
dev.Canonical manifest:
.github/frontend-architecture-manifest.mdHuman Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
devbase and verification result.Risk Notes
Low. This is a behavior-preserving extraction with focused tests. It does not change scroll strategy, controller transitions,
createAutoScroll, dock state, visual layout, public UI exports, persistence, or platform behavior.How To Verify
Screenshots or Recordings
Not applicable. This is a non-visible extraction and test hardening PR.
Checklist
dev; the previous stacked base was removed after refactor(app): extract timeline staging helper #670 merged