fix(app): stabilize timeline scroll recovery#916
Closed
Astro-Han wants to merge 11 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughAdds DOM reading anchors and richer reading-position data, preserves latest during layout settling by ignoring weak upward gestures, exposes a dock layout-change preservation callback, and validates behavior with unit tests and an E2E diagnostic-capturing test. ChangesTimeline Scroll Anchoring and Latest Protection
Sequence DiagramsequenceDiagram
participant User as User gesture
participant Timeline as Timeline viewport
participant Sampler as sampleTimelineSafePosition
participant Finder as bestVisibleTimelineAnchor
participant Controller as ScrollController
participant Dock as ScrollDock
User->>Timeline: weak upward wheel/touch
Timeline->>Controller: intent(weak_upward)
Controller->>Controller: check following_latest + latestProtected
alt latestProtected active
Controller-->>Timeline: ignore intent (latest_protected_weak_upward_ignored)
else not protected
Controller-->>Timeline: accept intent -> scroll
end
Dock->>Dock: layout change starts (dock/content resize)
Dock->>Dock: collect pre-layout metrics (scrollTop,distanceFromBottom,...)
Dock->>Controller: shouldPreserveLatestForLayoutChange(event, metrics)
Controller-->>Dock: decision -> apply stickToBottom
Note over Sampler,Finder: sampling & restore
Timeline->>Sampler: sampleTimelineSafePosition()
Sampler->>Finder: query data-timeline-anchor elements
Finder-->>Sampler: primary + fallback anchors
Timeline->>Controller: store reading position keys
Controller->>Timeline: restoreReading() -> locate anchors -> compute scrollTop -> restore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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 |
Astro-Han
added
bug
![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/session-timeline-scroll-anchors.test.ts, packages/app/src/pages/session/session-timeline-scroll-anchors.ts, packages/app/src/pages/session/session-timeline-scroll-controller.test.ts, packages/app/src/pages/session/session-timeline-scroll-controller.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 timeline scroll anchor sampling and restoration to preserve the user's reading position across layout changes, such as dock resizing. It also adds logic to ignore weak upward scroll intents to protect the 'latest' message state during layout settling. The feedback focuses on performance optimizations (avoiding layout thrashing by checking rect dimensions before calling getComputedStyle), standardizing DOM queries using closest and attribute selectors, and removing duplicated logic by exporting and reusing the isWeakUpwardTimelineIntent helper.
Perf delta summaryComparator: pass
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pages/session/session-timeline-scroll-anchors.ts`:
- Around line 262-278: The loop restoring using timelineAnchors may pick an
element that is hidden/collapsed (zero-size or inside a closed <details>),
producing invalid geometry; update the loop that uses timelineAnchors and
timelineAnchorByKey to: after resolving anchor, compute its geometry and skip it
if it's not visible (e.g., getClientRects().length === 0 or bounding rect has
zero width/height, or the anchor is inside a <details> whose
closest('details')?.open === false, or other standard visibility checks) so the
code falls through to the next anchor; only call setTimelineScrollTop when the
anchor passes these visibility checks. Ensure references to timelineAnchors,
timelineAnchorByKey, anchor.getBoundingClientRect(), and setTimelineScrollTop
are preserved so reviewers can find the change.
In `@packages/app/src/pages/session/session-timeline-scroll-controller.ts`:
- Around line 25-30: cloneState currently shallow-copies lastSafePosition and
pendingRecovery.anchor which lets callers mutate internal reading-anchor
objects; update cloneState to deep-clone the nested reading-anchor fields when
TimelineSafePosition.kind === "reading" (specifically clone primaryAnchor,
fallbackTrowAnchor, and fallbackMessage and its fields) and likewise deep-copy
pendingRecovery.anchor before returning state() so lastSafePosition and
pendingRecovery.anchor no longer share references with internal structures.
🪄 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: f5a5d6b9-9e70-458a-ad60-2e9c4fb91b8b
📒 Files selected for processing (8)
packages/app/e2e/session/session-scroll-position.spec.tspackages/app/src/pages/session/session-timeline-scroll-anchors.test.tspackages/app/src/pages/session/session-timeline-scroll-anchors.tspackages/app/src/pages/session/session-timeline-scroll-controller.test.tspackages/app/src/pages/session/session-timeline-scroll-controller.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.ts
github-actions
Bot
added
ui
Contributor
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 `@packages/app/src/pages/session/session-timeline-scroll-anchors.ts`:
- Around line 110-119: The code currently falls back to scanning the whole
message even when a selectedBlock exists, which can pick a sibling block's trow
and restore the wrong position; change the scopes logic so that when
selectedBlock (from input.selected.closest(...)) is an HTMLElement you only
search that selectedBlock (scopes = [selectedBlock]) and do not perform a second
pass over message; keep using findFirstVisibleTrowAnchor with the same params
(scope, selectedKey, viewportRect) and if no anchor is found return undefined so
fallbackMessage can handle recovery instead of searching sibling blocks.
🪄 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: fac9f6d9-7fc6-4ea5-a2ba-1a4f534db77f
📒 Files selected for processing (10)
packages/app/e2e/session/session-scroll-position.spec.tspackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/session-timeline-scroll-anchors.test.tspackages/app/src/pages/session/session-timeline-scroll-anchors.tspackages/app/src/pages/session/session-timeline-scroll-controller.test.tspackages/app/src/pages/session/session-timeline-scroll-controller.tspackages/app/src/pages/session/use-session-timeline-interaction.test.tspackages/app/src/pages/session/use-session-timeline-interaction.tspackages/desktop-electron/src/main/renderer-diagnostics-sanitize.test.tspackages/desktop-electron/src/main/renderer-diagnostics-sanitize.ts
Comment on lines
+110
to
+119
| const selectedBlock = input.selected.closest('[data-component="session-turn-trow-block"]') | ||
| const scopes = selectedBlock instanceof HTMLElement ? [selectedBlock, message] : [message] | ||
| for (const scope of scopes) { | ||
| const anchor = findFirstVisibleTrowAnchor({ | ||
| scope, | ||
| selectedKey: input.selectedKey, | ||
| viewportRect: input.viewportRect, | ||
| }) | ||
| if (anchor) return anchor | ||
| } |
Contributor
There was a problem hiding this comment.
Don't widen the trow fallback back to the whole message.
When selectedBlock exists, Lines 111-118 still do a second pass over message. If the chosen tool row has no visible trow:* anchor in its own block, this can capture a sibling block's trow and restore to the wrong intra-message position. In that case it's safer to return undefined and let fallbackMessage handle recovery.
Suggested fix
const selectedBlock = input.selected.closest('[data-component="session-turn-trow-block"]')
- const scopes = selectedBlock instanceof HTMLElement ? [selectedBlock, message] : [message]
- for (const scope of scopes) {
- const anchor = findFirstVisibleTrowAnchor({
- scope,
- selectedKey: input.selectedKey,
- viewportRect: input.viewportRect,
- })
- if (anchor) return anchor
- }
+ const scope = selectedBlock instanceof HTMLElement ? selectedBlock : message
+ return findFirstVisibleTrowAnchor({
+ scope,
+ selectedKey: input.selectedKey,
+ viewportRect: input.viewportRect,
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const selectedBlock = input.selected.closest('[data-component="session-turn-trow-block"]') | |
| const scopes = selectedBlock instanceof HTMLElement ? [selectedBlock, message] : [message] | |
| for (const scope of scopes) { | |
| const anchor = findFirstVisibleTrowAnchor({ | |
| scope, | |
| selectedKey: input.selectedKey, | |
| viewportRect: input.viewportRect, | |
| }) | |
| if (anchor) return anchor | |
| } | |
| const selectedBlock = input.selected.closest('[data-component="session-turn-trow-block"]') | |
| const scope = selectedBlock instanceof HTMLElement ? selectedBlock : message | |
| return findFirstVisibleTrowAnchor({ | |
| scope, | |
| selectedKey: input.selectedKey, | |
| viewportRect: input.viewportRect, | |
| }) |
🤖 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/src/pages/session/session-timeline-scroll-anchors.ts` around
lines 110 - 119, The code currently falls back to scanning the whole message
even when a selectedBlock exists, which can pick a sibling block's trow and
restore the wrong position; change the scopes logic so that when selectedBlock
(from input.selected.closest(...)) is an HTMLElement you only search that
selectedBlock (scopes = [selectedBlock]) and do not perform a second pass over
message; keep using findFirstVisibleTrowAnchor with the same params (scope,
selectedKey, viewportRect) and if no anchor is found return undefined so
fallbackMessage can handle recovery instead of searching sibling blocks.
Owner
Author
|
Closing this PR without merging. Follow-up manual testing on May 26 showed that the timeline still jitters, especially around tool row expand/collapse and content resize. The latest exported renderer diagnostics (
This PR accumulated 11 commits across several patches:
That history is a useful investigation trail, but it is also the reason not to merge it: the fix strategy is now too patch-oriented for a core timeline ownership problem. The next pass should start from a small design/architecture reset around timeline scroll ownership, with explicit boundaries for user intent, latest following, layout transactions, tool/trow anchor restoration, and who is allowed to issue scroll writes. |
Owner
Author
|
Closing as not planned. Follow-up diagnostics and manual testing show the scroll jitter remains, and this PR has become a patch stack rather than a mergeable architecture fix. |
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
1 participant
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.
Summary
data-timeline-anchorsampling and fallback recovery.Why
Issue #911 exposed timeline jumps from two related paths: tool/trow expansion while reading history used message-level anchors that were too coarse, and answer-completion dock/content resize could let weak upward input interrupt latest-following. This PR wires both paths into the existing scroll controller / layout transaction model without a broad rewrite.
Related Issue
Closes #911
Human Review Status
Pending
Review Focus
Please review the reading-anchor fallback chain and the latest-protection boundary: weak upward wheel/touch is ignored only while latest protection is active, while strong upward gestures, keyboard upward navigation, scrollbar drag, and target-message navigation still yield to user intent.
Risk Notes
session-turn-trow-blockbefore falling back to the broader message-level search.state()snapshots.How To Verify
Screenshots or Recordings
Session trow snap was regenerated and reviewed locally:
docs/design/preview/screenshots/session-trow.pngChecklist
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
Bug Fixes
Tests