Skip to content

fix(macOS): defer lifecycle bottom-pin writes to avoid SwiftUI update-cycle mutation #24332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
siddseethepalli merged 1 commit into from
Apr 8, 2026
Merged

fix(macOS): defer lifecycle bottom-pin writes to avoid SwiftUI update-cycle mutation #24332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions clients/macos/vellum-assistant/Features/Chat/MessageListScrollState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,12 @@ final class MessageListScrollState {
/// Timeout task for expansion stabilization — auto-ends after 200ms.
@ObservationIgnored private var expansionTimeoutTask: Task<Void, Never>?

/// Generation counter for lifecycle-driven bottom pins that are deferred
/// onto the next main-queue turn. This keeps `onChange` handlers from
/// mutating `ScrollPosition` during the same SwiftUI update pass that
/// triggered them.
@ObservationIgnored private var deferredBottomPinGeneration: UInt64 = 0

/// Tracks overlapping stabilization windows. Stabilization only ends
/// when all active windows have completed, so concurrent reasons
/// (e.g. resize during pagination) don't prematurely restore the mode.
Expand Down Expand Up @@ -590,6 +596,40 @@ final class MessageListScrollState {
}
}

/// Schedules a bottom-pin for the next main-queue turn, coalescing
/// repeated requests from the same render cycle into a single execution.
///
/// This is primarily used by lifecycle `onChange` handlers that can fire
/// during a SwiftUI update pass (for example `isSending` and
/// `messages.count`). Deferring the actual `ScrollPosition` write avoids
/// tripping SwiftUI's "Modifying state during view update" runtime guard.
func scheduleDeferredBottomPin(
animated: Bool = false,
userInitiated: Bool = false,
forceFollowingBottom: Bool = false,
refreshRecoveryWindow: Bool = false
) {
deferredBottomPinGeneration &+= 1
let generation = deferredBottomPinGeneration
Comment on lines +612 to +613

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve forced-follow flags when coalescing deferred pins

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 👍 / 👎.

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)
}
}
}
Comment on lines +611 to +627

@devin-ai-integration devin-ai-integration Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


func cancelDeferredBottomPin() {
deferredBottomPinGeneration &+= 1
}

// MARK: - Scroll Execution

/// Executes a bottom-pin scroll if the current mode allows it.
Expand Down Expand Up @@ -810,6 +850,7 @@ final class MessageListScrollState {
func reset(for newConversationId: UUID?) {
cancelStabilizationTasks()
stabilizationGeneration &+= 1
cancelDeferredBottomPin()
paginationTask?.cancel()
paginationTask = nil
ScrollGeometryUpdateDispatcher.shared.cancel(for: self)
Expand Down Expand Up @@ -857,6 +898,7 @@ final class MessageListScrollState {
func cancelAll() {
cancelStabilizationTasks()
stabilizationGeneration &+= 1
cancelDeferredBottomPin()
uiSyncTask?.cancel()
uiSyncTask = nil
scrollRestoreTask?.cancel()
Expand Down
21 changes: 11 additions & 10 deletions clients/macos/vellum-assistant/Features/Chat/MessageListView+Lifecycle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,16 @@ extension MessageListView {
if isDaemonConfirmationResume && !scrollState.isFollowingBottom {
// Daemon resumed from confirmation while user was scrolled up.
} else {
scrollState.transition(to: .followingBottom)
// Start a fresh recovery window: the animated scroll can
// overshoot into blank LazyVStack estimated space. Without
// this, isAtBottom is falsely true at the estimated bottom
// and persistent recovery doesn't fire — the viewport
// stays blank until the user scrolls manually.
scrollState.bottomAnchorAppeared = false
scrollState.recoveryDeadline = Date().addingTimeInterval(2.0)
scrollState.requestPinToBottom(animated: true)
// Defer the actual bottom-pin to the next main-queue turn.
// Both `isSending` and `messages.count` can change in the
// same SwiftUI update cycle after the user sends a message;
// issuing immediate `ScrollPosition` writes here can trip
// SwiftUI's "Modifying state during view update" guard.
scrollState.scheduleDeferredBottomPin(
animated: true,
forceFollowingBottom: true,
refreshRecoveryWindow: true
)
os_signpost(.event, log: PerfSignposts.log, name: "scrollToRequested",
"target=bottom reason=sendFollowingBottom")
}
Expand Down Expand Up @@ -193,7 +194,7 @@ extension MessageListView {
scrollState.lastMessageId = lastId
}
if anchorMessageId == nil {
scrollState.requestPinToBottom(animated: true)
scrollState.scheduleDeferredBottomPin(animated: true)
}
// --- Confirmation focus handoff ---
#if os(macOS)
Expand Down
50 changes: 50 additions & 0 deletions clients/macos/vellum-assistantTests/MessageListScrollStateTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,56 @@ final class MessageListScrollStateTests: XCTestCase {
}
}

// MARK: - Deferred Bottom Pins

func testDeferredBottomPinExecutesOnNextRunLoop() async throws {
state.transition(to: .followingBottom)
let lastId = UUID()
state.lastMessageId = lastId

state.scheduleDeferredBottomPin(animated: true)

XCTAssertTrue(scrollToCalls.isEmpty,
"Deferred pin should not mutate scroll position synchronously")

try await Task.sleep(nanoseconds: 50_000_000)

XCTAssertEqual(scrollToCalls.count, 1)
XCTAssertEqual(scrollToCalls.first?.id, AnyHashable(lastId))
XCTAssertEqual(scrollToCalls.first?.anchor, .bottom)
}

func testDeferredBottomPinCoalescesRepeatedRequests() async throws {
state.transition(to: .followingBottom)
let lastId = UUID()
state.lastMessageId = lastId

state.scheduleDeferredBottomPin(animated: true)
state.scheduleDeferredBottomPin(animated: true)
state.scheduleDeferredBottomPin(animated: true, forceFollowingBottom: true, refreshRecoveryWindow: true)

try await Task.sleep(nanoseconds: 50_000_000)

XCTAssertEqual(scrollToCalls.count, 1,
"Repeated deferred pins from one update cycle should coalesce")
XCTAssertTrue(state.isFollowingBottom)
XCTAssertFalse(state.bottomAnchorAppeared)
XCTAssertNotNil(state.recoveryDeadline)
}

func testDeferredBottomPinCancelledByReset() async throws {
state.transition(to: .followingBottom)
state.lastMessageId = UUID()

state.scheduleDeferredBottomPin(animated: true)
state.reset(for: UUID())

try await Task.sleep(nanoseconds: 50_000_000)

XCTAssertTrue(scrollToCalls.isEmpty,
"Reset should cancel any deferred bottom pin from the old conversation")
}

// MARK: - Reset

func testResetRestoresDefaults() {
Expand Down