fix(chat): harden session view ownership#1979
Merged
Merged
Conversation
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 (9)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe change centralizes session-view ownership in the message store and adds request, revision, and stream identity fences across ChatPage, session hydration, pending inputs, caching, and IPC streaming. Tests and architecture documentation cover rapid navigation, stale async work, cache invalidation, stream ordering, and submit races. ChangesChat session race hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ChatPage
participant messageStore
participant chatClient
ChatPage->>messageStore: verify session and page generation
ChatPage->>chatClient: submit chat turn
chatClient-->>messageStore: stream updates with request identity
messageStore-->>ChatPage: render current-session stream
chatClient-->>messageStore: terminal event
messageStore->>messageStore: settle only the owning request
Possibly related PRs
Suggested reviewers: 🚥 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: 2
🧹 Nitpick comments (2)
src/renderer/src/stores/ui/pendingInput.ts (1)
50-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated mutate-then-conditionally-reload pattern across
queueInput,updateQueueInput,moveQueueInput,steerPendingInput,deleteInput.Each function repeats the same "clear error if current session → call client → reload if still current session → set error if still current session, rethrow" shape. Worth extracting into a shared helper to reduce duplication.
♻️ Suggested extraction
+ async function runSessionScopedMutation( + sessionId: string, + operation: () => Promise<void>, + errorPrefix: string + ): Promise<void> { + if (currentSessionId.value === sessionId) { + error.value = null + } + try { + await operation() + if (currentSessionId.value === sessionId) { + await loadPendingInputs(sessionId) + } + } catch (e) { + if (currentSessionId.value === sessionId) { + error.value = `${errorPrefix}: ${e}` + } + throw e + } + } + async function queueInput(sessionId: string, input: string | SendMessageInput): Promise<void> { - if (currentSessionId.value === sessionId) { - error.value = null - } - try { - await sessionClient.queuePendingInput(sessionId, input) - if (currentSessionId.value === sessionId) { - await loadPendingInputs(sessionId) - } - } catch (e) { - if (currentSessionId.value === sessionId) { - error.value = `Failed to queue message: ${e}` - } - throw e - } + return runSessionScopedMutation( + sessionId, + () => sessionClient.queuePendingInput(sessionId, input), + 'Failed to queue message' + ) }Apply the same pattern to
updateQueueInput,moveQueueInput,steerPendingInput, anddeleteInput.🤖 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/stores/ui/pendingInput.ts` around lines 50 - 137, Extract the repeated clear-error, client mutation, conditional reload, and session-scoped error handling from queueInput, updateQueueInput, moveQueueInput, steerPendingInput, and deleteInput into a shared helper. Have each operation delegate its client call and operation-specific failure message to that helper while preserving the existing current-session checks and rethrow behavior.src/renderer/src/stores/ui/message.ts (1)
74-83: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
messageMutationRevisions,recentViewInvalidationRevisions, anddirtyRecentSessionViewsgrow unbounded across the app lifetime.Unlike
recentSessionViews(a bounded LRU), these three plainMap/Setstructures are only cleared wholesale inclear(). Deleted sessions invalidate their cached view (invalidateRecentSessionView, e.g. called fromremoveSessionsin session.ts) but never purge their entries here, so every session ever created or switched away from during a long-running Electron session leaks a small amount of memory indefinitely.♻️ Suggested cleanup hook
+ function purgeSessionTracking(sessionId: string): void { + messageMutationRevisions.delete(sessionId) + recentViewInvalidationRevisions.delete(sessionId) + dirtyRecentSessionViews.delete(sessionId) + }Expose
purgeSessionTrackingand call it alongsideinvalidateRecentSessionViewwherever a session is permanently removed.🤖 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/stores/ui/message.ts` around lines 74 - 83, Expose a purgeSessionTracking helper that removes a session’s entries from messageMutationRevisions, recentViewInvalidationRevisions, and dirtyRecentSessionViews. Invoke it alongside invalidateRecentSessionView in the permanent session-removal flow, including removeSessions, while preserving existing invalidation behavior for non-deleted sessions.
🤖 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/pages/ChatPage.vue`:
- Around line 2542-2544: Update the restore flow around loadMessagesForSession
and applyRestoredSessionSummary to check whether restoredSession is null before
applying it. Return without updating the active summary when the load returns
null, while preserving the existing canWriteSessionView validation and applying
non-null results normally.
In `@src/renderer/src/stores/ui/messageIpc.ts`:
- Around line 39-53: Update the stream acceptance logic around streamKey,
markStreamSettled, and the affected lines 57–79 so a request ID that was
superseded or settled cannot reclaim the session stream, regardless of event
timestamp or settled-key eviction. Track a durable per-session
generation/tombstone state, reject late superseded events such as A@3 after B@2,
and add a regression test covering that sequence.
---
Nitpick comments:
In `@src/renderer/src/stores/ui/message.ts`:
- Around line 74-83: Expose a purgeSessionTracking helper that removes a
session’s entries from messageMutationRevisions,
recentViewInvalidationRevisions, and dirtyRecentSessionViews. Invoke it
alongside invalidateRecentSessionView in the permanent session-removal flow,
including removeSessions, while preserving existing invalidation behavior for
non-deleted sessions.
In `@src/renderer/src/stores/ui/pendingInput.ts`:
- Around line 50-137: Extract the repeated clear-error, client mutation,
conditional reload, and session-scoped error handling from queueInput,
updateQueueInput, moveQueueInput, steerPendingInput, and deleteInput into a
shared helper. Have each operation delegate its client call and
operation-specific failure message to that helper while preserving the existing
current-session checks and rethrow behavior.
🪄 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: 6ca27d02-0aeb-4790-9d6f-d2fee506b03e
📒 Files selected for processing (14)
docs/architecture/chat-scroll-ownership/spec.mddocs/architecture/chat-scroll-ownership/tasks.mddocs/issues/chat-session-view-races/spec.mdsrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/messageIpc.tssrc/renderer/src/stores/ui/pendingInput.tssrc/renderer/src/stores/ui/session.tssrc/renderer/src/stores/ui/stream.tstest/renderer/components/ChatPage.test.tstest/renderer/stores/messageStore.reactivity.test.tstest/renderer/stores/messageStore.test.tstest/renderer/stores/pendingInputStore.test.tstest/renderer/stores/sessionStore.test.ts
Collaborator
Author
|
Addressed the two review-summary nitpicks in |
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
Root cause
PR #1974 introduced atomic view commits and recent-session caching, but
ChatPageretained an independent readiness owner. A matching stream-end refresh could supersede the page restore, commit the correct view, and leave the composer guarded until the user switched conversations. Several adjacent paths also used session-ID-only checks, which could admit stale work after A-B-A navigation or overwrite newer in-memory mutations.Compatibility
No database, persisted-data, preload, or shared IPC contract changes.
Validation
pnpm test: 5,516 passed, 198 skipped; the only 3 failures were sandboxlisten EPERMerrors in Feishu loopback callback testspnpm run typecheckpnpm run formatpnpm run i18npnpm run lintpnpm run buildLocal validation used Node 26.0.0 while the repository declares Node >=24.14.1 <25. Provider and ACP registry refreshes fell back to existing snapshots because their remote endpoints were unavailable in the validation environment.
SDD:
docs/issues/chat-session-view-races/spec.mdSummary by CodeRabbit