fix(app): add timeline layout transactions - #832
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 (3)
📝 WalkthroughWalkthroughThis PR introduces timeline layout transaction coordination to safely handle dock and content resize events while preventing cumulative layout shift. A new transaction coordinator manages mutation, scroll anchor restoration via immediate or fallback frame scheduling, and diagnostic lifecycle events. The system integrates transaction context into scroll command recording, extends scroll dock to route resize operations through transactions, and threads transaction state through the timeline UI for dynamic overscan adjustment. Runtime CLS probe monitoring is extended to capture and report transaction metadata. ChangesTimeline Layout Transaction Coordination
Sequence Diagram(s)sequenceDiagram
participant Client
participant Coordinator
participant FrameScheduler
participant StateSync
Client->>Coordinator: run(mutation, restoreLatest)
Coordinator->>Coordinator: apply mutation
Coordinator->>Coordinator: attempt immediate restore
alt immediate restore succeeds
Coordinator->>StateSync: settled
else restore fails
Coordinator->>FrameScheduler: schedule fallback frame
FrameScheduler->>Coordinator: frame ready
Coordinator->>Coordinator: attempt restore
alt second restore succeeds
Coordinator->>StateSync: settled
else budget exceeded
Coordinator->>StateSync: violation
end
end
Coordinator->>Client: emit diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/session.tsx, packages/app/src/pages/session/message-timeline.tsx, packages/app/src/pages/session/session-main-view.tsx, packages/app/src/pages/session/timeline-layout-stable-band.test.ts, packages/app/src/pages/session/timeline-layout-stable-band.ts, packages/app/src/pages/session/timeline-layout-transaction.test.ts, packages/app/src/pages/session/timeline-layout-transaction.ts, packages/app/src/pages/session/timeline-row-renderer.tsx, packages/app/src/pages/session/timeline-scroll-command-sink.test.ts, packages/app/src/pages/session/timeline-scroll-command-sink.ts, packages/app/src/pages/session/use-session-scroll-dock.test.ts, packages/app/src/pages/session/use-session-scroll-dock.ts, packages/app/src/pages/session/use-session-timeline-interaction.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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a layout transaction system designed to stabilize the session timeline's scroll position during layout shifts, such as dock resizes and content updates. Key additions include a TimelineLayoutTransactionCoordinator for managing scroll anchor restoration and updates to the scroll command sink to track transaction metadata. Feedback from the review highlights critical logical errors in the asynchronous fallback loop of the coordinator, which would trigger premature violations. The reviewer also pointed out a race condition where reactive state is cleared before asynchronous operations finish and noted that the provided cancelFrame functionality is currently unused, potentially leading to conflicting scroll commands.
Perf delta summaryComparator: pass
|
github-actions
Bot
added
ci
github-actions
Bot
removed
ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app/e2e/perf/perf-probe.spec.ts (1)
232-240: ⚡ Quick winUse the shared timeline event constant instead of a string literal.
"opencode:e2e:timeline"is a cross-layer contract; hardcoding it here can drift from the app/testing driver constant.Proposed diff
import { composerEvent, type ComposerDriverState, type ComposerWindow } from "../../src/testing/session-composer" +import { timelineEvent } from "../../src/testing/timeline" @@ async function revealCachedSessionMessagesThroughDriver(page: Parameters<typeof snapshotPerfProbe>[0]) { await page.evaluate(() => { window.dispatchEvent( - new CustomEvent("opencode:e2e:timeline", { + new CustomEvent(timelineEvent, { detail: { action: "reveal-cached" }, }), ) }) }🤖 Prompt for 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. In `@packages/app/e2e/perf/perf-probe.spec.ts` around lines 232 - 240, The test function revealCachedSessionMessagesThroughDriver hardcodes the event name "opencode:e2e:timeline"; replace this string with the shared timeline event constant exported by the app/testing driver (import it into the file and use it in the window.dispatchEvent call) so the test uses the canonical cross-layer contract rather than a literal; update the import list at the top (alongside snapshotPerfProbe) and reference that constant in the CustomEvent constructor.
🤖 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 `@packages/app/src/testing/perf-metrics.ts`:
- Around line 423-437: The missing_base_scenario checks in comparePerfBaselines
currently run against all head entries even when input.scenarioKeys restricts
the comparison; update the post-iteration that scans input.head (or the Map
headByScenario) for head-only scenarios to skip any head scenario whose
scenarioKey is not in the requestedScenarioKeys Set (i.e., if
requestedScenarioKeys is defined and !requestedScenarioKeys.has(key) then
continue), so that missing_base_scenario failures are only reported for
scenarios the caller requested; keep existing behavior when scenarioKeys is
undefined (check all head entries).
---
Nitpick comments:
In `@packages/app/e2e/perf/perf-probe.spec.ts`:
- Around line 232-240: The test function
revealCachedSessionMessagesThroughDriver hardcodes the event name
"opencode:e2e:timeline"; replace this string with the shared timeline event
constant exported by the app/testing driver (import it into the file and use it
in the window.dispatchEvent call) so the test uses the canonical cross-layer
contract rather than a literal; update the import list at the top (alongside
snapshotPerfProbe) and reference that constant in the CustomEvent constructor.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e9f4342-024a-493e-9dce-785ce94e8eb5
📒 Files selected for processing (14)
packages/app/e2e/perf/perf-probe.spec.tspackages/app/e2e/perf/runtime-cls-gate.spec.tspackages/app/script/compare-perf.tspackages/app/src/pages/session/session-main-view.tsxpackages/app/src/pages/session/session-timeline-scroll-controller.tspackages/app/src/pages/session/timeline-layout-recovery-policy.tspackages/app/src/pages/session/use-session-scroll-dock.test.tspackages/app/src/pages/session/use-session-scroll-dock.tspackages/app/src/pages/session/use-session-timeline-interaction.test.tspackages/app/src/pages/session/use-session-timeline-interaction.tspackages/app/src/testing/perf-metrics.test.tspackages/app/src/testing/perf-metrics.tspackages/app/src/testing/timeline.test.tspackages/app/src/testing/timeline.ts
1 participant
Summary
Why
This PR lands #747 PR2's minimal layout transaction layer for controlled dock/content resize windows. It does not complete the later PR3+
TimelineLayoutEnginemigration and does not retire all legacy scroll owners.Related Issue
Refs #747
Human Review Status
Pending— waiting for a human reviewer to approve.Review Focus
TimelineScrollCommandSink.Risk Notes
This is #747 PR2 only. It does not complete the later TimelineLayoutEngine migration and does not retire every legacy scroll owner. The coordinator still delegates final scroll execution to TimelineScrollCommandSink.
Skipped checklist items:
project.open()lands on a 404 duringwaitSessionin this worktree.Content-resize E2E deferred because deterministic streaming resize seed is unavailable; coordinator unit tests cover fallback semantics and existing runtime gate covers dock resize windows.
How To Verify
Screenshots or Recordings
Not applicable: no intended visual or copy change. Failed E2E attempts produced 404 screenshots under
packages/app/e2e/test-results/, before reaching the changed timeline surface.Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Tests