fix(chat): stabilize scroll ownership#1974
Merged
Merged
Conversation
Centralize viewport writes behind one request arbiter. Preserve user ownership across restore, history, search, and measurement flows. Commit session views atomically, cache recent sessions, and show a non-scrolling skeleton while uncached sessions prepare.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR centralizes chat viewport scrolling behind typed state, request arbitration, and session-aware controller logic. It updates message-window geometry, session restoration, logical-anchor pagination, search navigation, bounded caches, and related renderer, component, architecture, and Electron smoke tests. ChangesChat scroll ownership and layout
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatPage
participant ChatScrollController
participant MessageStore
participant Viewport
User->>ChatPage: gesture, search, submit, or session switch
ChatPage->>ChatScrollController: submit typed scroll request
ChatScrollController->>MessageStore: resolve session/message layout state
ChatScrollController->>Viewport: commit one expected scroll write
Viewport->>ChatScrollController: report scroll or resize
ChatScrollController->>ChatPage: update ownership and completion state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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: 7
🧹 Nitpick comments (4)
test/e2e/specs/31-chat-scroll-ownership.smoke.spec.ts (2)
71-76: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCheck the instance before traversing up the prototype chain.
The current loop skips checking the
elementinstance itself because it immediately reassigns the prototype reference before checking for the property descriptor. While DOM elements typically inheritscrollTopfromElement.prototype, standard prototype traversal should check the object itself first so it doesn't bypass potential instance-level overrides.💡 Proposed fix for prototype traversal
- let prototype: object | null = element + let current: object | null = element let descriptor: PropertyDescriptor | undefined - while (prototype && !descriptor) { - prototype = Object.getPrototypeOf(prototype) - descriptor = prototype ? Object.getOwnPropertyDescriptor(prototype, 'scrollTop') : undefined + while (current && !descriptor) { + descriptor = Object.getOwnPropertyDescriptor(current, 'scrollTop') + if (!descriptor) { + current = Object.getPrototypeOf(current) + } }🤖 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 `@test/e2e/specs/31-chat-scroll-ownership.smoke.spec.ts` around lines 71 - 76, Update the prototype traversal loop around prototype and descriptor so it checks the current element or prototype for scrollTop before advancing with Object.getPrototypeOf. Preserve traversal through successive prototypes until a descriptor is found or the chain ends, including support for instance-level overrides.
84-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering programmatic scroll methods.
This instrumentation successfully intercepts direct assignments to
scrollTop. However, if the application's scroll controller ever usesElement.prototype.scrollTo(),scrollBy(), orscroll(), those programmatic writes will bypass this proxy and remain unrecorded.If full coverage is desired against future refactors, you might also want to intercept and record calls to these scroll methods.
🤖 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 `@test/e2e/specs/31-chat-scroll-ownership.smoke.spec.ts` around lines 84 - 91, Extend the scroll instrumentation around the existing scrollTop proxy to also intercept Element.prototype.scrollTo(), scrollBy(), and scroll(), recording each programmatic scroll operation in __deepchatScrollWrites before delegating to the native implementation. Preserve the current direct scrollTop assignment tracking and original method behavior.test/renderer/composables/chatScrollArchitecture.test.ts (1)
10-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnforce exclusive scroll ownership across the renderer.
This only rejects writes in
ChatPage.vue; a direct write added toMessageList.vueor another composable would pass. Scan renderer sources while excluding the controller’s single authorized assignment.🤖 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 `@test/renderer/composables/chatScrollArchitecture.test.ts` around lines 10 - 28, Expand the scroll-write ownership test to scan all relevant renderer source files, not only ChatPage.vue, for direct viewport scrolling APIs. Exclude useChatScrollController.ts from the forbidden-write scan, while retaining its assertion that exactly one viewport.scrollTop assignment exists there.test/renderer/composables/chatScrollState.test.ts (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the new chat composable suites into the mirrored
chat/test directory.
test/renderer/composables/chatScrollState.test.ts#L1-L10: move totest/renderer/composables/chat/chatScrollState.test.ts.test/renderer/composables/useChatScrollController.test.ts#L1-L3: move totest/renderer/composables/chat/useChatScrollController.test.ts.As per coding guidelines, renderer Vitest suites must mirror the source layout under
test/renderer/. <coding_guidelines>🤖 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 `@test/renderer/composables/chatScrollState.test.ts` around lines 1 - 10, Move test/renderer/composables/chatScrollState.test.ts to test/renderer/composables/chat/chatScrollState.test.ts, preserving its ChatScrollState test contents. Move test/renderer/composables/useChatScrollController.test.ts to test/renderer/composables/chat/useChatScrollController.test.ts, preserving its useChatScrollController tests; no other changes are required.Source: Coding guidelines
🤖 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 `@src/renderer/src/composables/chat/chatScrollRequestQueue.ts`:
- Around line 6-16: Update ChatScrollRequestQueue.enqueue and take so requests
from older sessionEpoch values cannot replace or clear a pending request from a
newer epoch. Compare epoch ordering explicitly, retain the newer pending request
when a stale request arrives, and validate before clearing pendingRequest so
stale take attempts leave current-session work intact.
In `@src/renderer/src/composables/chat/useChatScrollController.ts`:
- Around line 253-256: Update the near-bottom transition in the scroll
controller so return-to-bottom is dispatched only when nearBottom is accompanied
by active user intent, such as state.value.activeGesture or the established
explicit intent marker. Do not trigger return-to-bottom from layout- or
measurement-induced native scrolls, and preserve user ownership through those
events while retaining the existing user/native return classification.
- Around line 153-161: Reset writeCommittedThisFrame after every physical scroll
write in the immediate-commit path, including the corresponding write near
scheduleCommit, by scheduling a boundary reset on the next animation frame after
viewport.scrollTop is assigned. Preserve normal commit-frame behavior and add a
regression test that issues a second immediate request after verification
completes, confirming it is not delayed.
In `@src/renderer/src/pages/ChatPage.vue`:
- Around line 385-398: The restoreSessionMessages flow must only expose a
session after messageStore has committed the requested session. After
loadMessagesForSession and the requestId check, verify
messageStore.committedSessionId equals id; if not, retain the safe loading/error
state and return without setting committedMessageSessionId or enabling the
target composer. Keep the existing commit, performance-marking, and
summary-application path for successfully committed sessions.
- Around line 744-752: Update endListGesture and the related list-scroll event
handling so touch ownership remains active during native inertial scrolling:
defer finishListScrollingAfterIdle until scrollend, retain the idle timer as a
fallback, and refresh that timer for user-sourced momentum scroll events.
Preserve the existing cleanup for inactive gestures and reset gesture state when
scrolling truly ends.
- Around line 754-769: Update the upward-intent handlers, including
onListTouchMove and the corresponding wheel, scrollbar, and keyboard handlers,
to set pendingHistoryLoadAtIdle immediately when upward intent begins and
scrollTop is at or below TOP_HISTORY_THRESHOLD. Preserve existing gesture-state
and intent-reset behavior, and ensure this works even when no scroll event is
emitted.
In `@test/e2e/specs/31-chat-scroll-ownership.smoke.spec.ts`:
- Around line 118-119: Replace the fixed waitForTimeout after
searchInput.fill(firstToken) with a reliable completion signal for the chat
search, such as expect.poll() or an awaited DOM state change indicating search
processing and scrolling have finished. Then retain the scrollTop assertion,
ensuring it runs only after the search result is settled.
---
Nitpick comments:
In `@test/e2e/specs/31-chat-scroll-ownership.smoke.spec.ts`:
- Around line 71-76: Update the prototype traversal loop around prototype and
descriptor so it checks the current element or prototype for scrollTop before
advancing with Object.getPrototypeOf. Preserve traversal through successive
prototypes until a descriptor is found or the chain ends, including support for
instance-level overrides.
- Around line 84-91: Extend the scroll instrumentation around the existing
scrollTop proxy to also intercept Element.prototype.scrollTo(), scrollBy(), and
scroll(), recording each programmatic scroll operation in __deepchatScrollWrites
before delegating to the native implementation. Preserve the current direct
scrollTop assignment tracking and original method behavior.
In `@test/renderer/composables/chatScrollArchitecture.test.ts`:
- Around line 10-28: Expand the scroll-write ownership test to scan all relevant
renderer source files, not only ChatPage.vue, for direct viewport scrolling
APIs. Exclude useChatScrollController.ts from the forbidden-write scan, while
retaining its assertion that exactly one viewport.scrollTop assignment exists
there.
In `@test/renderer/composables/chatScrollState.test.ts`:
- Around line 1-10: Move test/renderer/composables/chatScrollState.test.ts to
test/renderer/composables/chat/chatScrollState.test.ts, preserving its
ChatScrollState test contents. Move
test/renderer/composables/useChatScrollController.test.ts to
test/renderer/composables/chat/useChatScrollController.test.ts, preserving its
useChatScrollController tests; no other changes are required.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8642481-4758-4080-826c-ed2856faa315
📒 Files selected for processing (27)
docs/architecture/chat-scroll-ownership/plan.mddocs/architecture/chat-scroll-ownership/spec.mddocs/architecture/chat-scroll-ownership/tasks.mddocs/architecture/chat-scroll-windowing/spec.mddocs/issues/chat-history-search-scroll-coordinates/spec.mdsrc/renderer/src/components/chat/ChatSessionSkeleton.vuesrc/renderer/src/components/chat/MessageList.vuesrc/renderer/src/components/chat/MessageListRow.vuesrc/renderer/src/composables/chat/chatScrollOperationArbiter.tssrc/renderer/src/composables/chat/chatScrollRequestQueue.tssrc/renderer/src/composables/chat/chatScrollState.tssrc/renderer/src/composables/chat/chatSessionPerformance.tssrc/renderer/src/composables/chat/useChatScrollController.tssrc/renderer/src/composables/message/useMessageWindow.tssrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/recentMessageViewCache.tstest/e2e/specs/31-chat-scroll-ownership.smoke.spec.tstest/renderer/components/ChatPage.test.tstest/renderer/components/MessageList.test.tstest/renderer/components/MessageListRow.test.tstest/renderer/composables/chatScrollArchitecture.test.tstest/renderer/composables/chatScrollState.test.tstest/renderer/composables/useChatScrollController.test.tstest/renderer/composables/useMessageWindow.test.tstest/renderer/stores/messageStore.test.tstest/renderer/stores/recentMessageViewCache.test.ts
zhangmo8
commented
Jul 15, 2026
zhangmo8
left a comment
zhangmo8
left a comment
Collaborator
Author
There was a problem hiding this comment.
I found three additional production-path issues that should be addressed before merge. The combined #1973 + #1974 tree merges cleanly and passes typecheck plus the focused skill/message/scroll suites, so these findings are isolated to scroll eligibility, coalesced navigation state, and cache lifetime.
Preserve user and session boundaries across queued, resize, touch, navigation, and keyed remount flows. Replace timing-only smoke assertions and widen the architecture guard.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/composables/message/recentMessageMeasurementCache.ts (1)
22-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a strict
undefinedcheck foroldestSessionId.In JavaScript,
!oldestSessionIdis truthy for an empty string (""). While it's extremely unlikely for a session ID to be an empty string, using a strictundefinedcheck is more robust and clearly signals that you are checking for iterator exhaustion rather than a generic falsy string.💡 Proposed refactor
set(sessionId: string, snapshot: MessageMeasurementSnapshot): void { this.entries.delete(sessionId) this.entries.set(sessionId, Object.freeze({ ...snapshot })) while (this.entries.size > this.maxEntries) { const oldestSessionId = this.entries.keys().next().value - if (!oldestSessionId) return + if (oldestSessionId === undefined) return this.entries.delete(oldestSessionId) } }🤖 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 `@src/renderer/src/composables/message/recentMessageMeasurementCache.ts` around lines 22 - 38, Update the oldestSessionId guard in RecentMessageMeasurementCache.set to check specifically for undefined, preserving deletion of valid session IDs including an empty string while still returning when the iterator is exhausted.
🤖 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 `@test/renderer/composables/chat/chatScrollArchitecture.test.ts`:
- Around line 46-55: Normalize the paths added to files in the visit function by
converting Windows backslashes from relative(process.cwd(), path) to forward
slashes. Keep the existing extension filtering and recursive traversal unchanged
so comparisons with CONTROLLER_PATH and allowedDirectScrollWrites use consistent
POSIX separators.
---
Nitpick comments:
In `@src/renderer/src/composables/message/recentMessageMeasurementCache.ts`:
- Around line 22-38: Update the oldestSessionId guard in
RecentMessageMeasurementCache.set to check specifically for undefined,
preserving deletion of valid session IDs including an empty string while still
returning when the iterator is exhausted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ca83294-0f14-4030-84c5-662eb116af71
📒 Files selected for processing (14)
docs/architecture/chat-scroll-ownership/spec.mddocs/architecture/chat-scroll-ownership/tasks.mdsrc/renderer/src/composables/chat/chatScrollRequestQueue.tssrc/renderer/src/composables/chat/chatScrollState.tssrc/renderer/src/composables/chat/useChatScrollController.tssrc/renderer/src/composables/message/recentMessageMeasurementCache.tssrc/renderer/src/pages/ChatPage.vuetest/e2e/specs/31-chat-scroll-ownership.smoke.spec.tstest/renderer/components/ChatPage.test.tstest/renderer/components/ChatTabView.test.tstest/renderer/composables/chat/chatScrollArchitecture.test.tstest/renderer/composables/chat/chatScrollState.test.tstest/renderer/composables/chat/useChatScrollController.test.tstest/renderer/composables/message/recentMessageMeasurementCache.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/renderer/src/composables/chat/chatScrollRequestQueue.ts
- src/renderer/src/composables/chat/chatScrollState.ts
- test/e2e/specs/31-chat-scroll-ownership.smoke.spec.ts
- docs/architecture/chat-scroll-ownership/spec.md
- src/renderer/src/composables/chat/useChatScrollController.ts
- test/renderer/components/ChatPage.test.ts
- src/renderer/src/pages/ChatPage.vue
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
2 participants
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
UI Structure
Before
After
Behavior and Compatibility
Validation
Remaining Manual Validation
Summary by CodeRabbit