fix(macOS): defer lifecycle bottom-pin writes to avoid SwiftUI update-cycle mutation#24332
Conversation
…-cycle mutation onChange handlers for isSending and messages.count can fire during the same SwiftUI update pass that triggered them. Writing to ScrollPosition synchronously trips SwiftUI's "Modifying state during view update" runtime guard. Introduce scheduleDeferredBottomPin() which coalesces repeated requests via a generation counter and defers the actual ScrollPosition write to the next main-queue turn. Reset and cancelAll properly invalidate pending deferred pins.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ) { | ||
| deferredBottomPinGeneration &+= 1 | ||
| let generation = deferredBottomPinGeneration | ||
| DispatchQueue.main.async { [weak self] in | ||
| Task { @MainActor [weak self] in | ||
| guard let self, self.deferredBottomPinGeneration == generation else { return } | ||
| if forceFollowingBottom { | ||
| self.transition(to: .followingBottom) | ||
| } | ||
| if refreshRecoveryWindow { | ||
| self.bottomAnchorAppeared = false | ||
| self.recoveryDeadline = Date().addingTimeInterval(2.0) | ||
| } | ||
| _ = self.requestPinToBottom(animated: animated, userInitiated: userInitiated) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Generation-based coalescing drops forceFollowingBottom and refreshRecoveryWindow when handleSendingChanged fires before handleMessagesCountChanged
The scheduleDeferredBottomPin coalescing mechanism uses a generation counter where only the last call's parameters survive. In MessageListView.swift:152-153, onChange(of: isSending) is declared before onChange(of: messages.count), so when both fire in the same SwiftUI update cycle (the exact scenario the PR comment at line 128-131 describes), handleSendingChanged fires first with forceFollowingBottom: true, refreshRecoveryWindow: true (generation N), then handleMessagesCountChanged fires second with only animated: true (generation N+1). Only generation N+1 executes — silently dropping the mode transition and recovery window.
When the user is in .freeBrowsing (scrolled up) and sends a message, the deferred requestPinToBottom(animated: true, userInitiated: false) checks mode.allowsAutoScroll which is false for .freeBrowsing (MessageListScrollState.swift:52-56), so it returns false — the scroll never happens. The old synchronous code handled this correctly because handleSendingChanged immediately called transition(to: .followingBottom), so handleMessagesCountChanged's subsequent requestPinToBottom saw the updated mode.
The test masks this bug by using the wrong call order
In testDeferredBottomPinCoalescesRepeatedRequests (line 329-331), the forceFollowingBottom: true call is placed last, so it survives coalescing. In production, the declaration order at MessageListView.swift:152-153 causes it to fire first, where it gets superseded.
Prompt for agents
The generation-counter coalescing in scheduleDeferredBottomPin only executes the last call's parameters, but handleSendingChanged (which needs forceFollowingBottom and refreshRecoveryWindow) fires BEFORE handleMessagesCountChanged (which only needs animated: true) due to onChange declaration order at MessageListView.swift:152-153. The last call wins, dropping the critical flags.
Possible approaches:
1. Accumulate flags across coalesced calls: instead of capturing parameters per-call, maintain stored properties (pendingForceFollowingBottom, pendingRefreshRecoveryWindow, etc.) that are OR-merged on each scheduleDeferredBottomPin call and reset after execution. The generation counter still handles the defer-to-next-turn semantics.
2. Keep the mode transition synchronous (transition(to: .followingBottom) in handleSendingChanged) and only defer the ScrollPosition write (requestPinToBottom). This matches how the old code worked — the mode transition is safe during a SwiftUI update pass; only the ScrollPosition mutation triggers the runtime guard.
3. Reverse the onChange declaration order so messages.count fires before isSending — but this is fragile and could break other ordering assumptions.
Approach 1 is the most robust. The test testDeferredBottomPinCoalescesRepeatedRequests should also be updated to test the production call order (forceFollowingBottom call first, plain call second).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3716bc459b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| deferredBottomPinGeneration &+= 1 | ||
| let generation = deferredBottomPinGeneration |
There was a problem hiding this comment.
The coalescing strategy here is last-call-wins, which drops options from earlier requests in the same update cycle. In the current flow, handleSendingChanged() queues scheduleDeferredBottomPin(... forceFollowingBottom: true, refreshRecoveryWindow: true), then handleMessagesCountChanged() queues scheduleDeferredBottomPin(animated: true); the second call bumps the generation and suppresses the first closure. When the user sends while in .freeBrowsing, the surviving request no longer transitions to .followingBottom, so requestPinToBottom can no-op and the transcript may not scroll to latest after send. Coalescing should merge intent (e.g., OR these flags) instead of discarding earlier stronger requests.
Useful? React with 👍 / 👎.
…-cycle mutation (#24332) onChange handlers for isSending and messages.count can fire during the same SwiftUI update pass that triggered them. Writing to ScrollPosition synchronously trips SwiftUI's "Modifying state during view update" runtime guard. Introduce scheduleDeferredBottomPin() which coalesces repeated requests via a generation counter and defers the actual ScrollPosition write to the next main-queue turn. Reset and cancelAll properly invalidate pending deferred pins.
1 participant
Summary
scheduleDeferredBottomPin()onMessageListScrollStatethat coalesces repeated requests via a generation counter and defersScrollPositionwrites to the next main-queue turnrequestPinToBottomcalls inMessageListView+LifecycleonChange handlers with the deferred variant to avoid tripping SwiftUI's "Modifying state during view update" runtime guard