Skip to content

refactor: isolate composer appkit bridge behind explicit events #24401

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

refactor: isolate composer appkit bridge behind explicit events #24401

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
2 changes: 1 addition & 1 deletion clients/macos/vellum-assistant/Features/Chat/ComposerEmojiPicker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ extension ComposerView {
let length = cursorUtf16 - colonOffset
let nsRange = NSRange(location: colonOffset, length: length)

textReplacer.replaceText?(nsRange, entry.emoji)
bridgeCommands.pendingReplaceRange = (range: nsRange, replacement: entry.emoji)

@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.

🔴 Emoji insertion via pendingReplaceRange silently deferred because no @State change triggers updateNSView

In the old code, selectEmoji called textReplacer.replaceText?(nsRange, entry.emoji) which synchronously invoked textView.insertText(...) via a closure. The new code instead sets bridgeCommands.pendingReplaceRange, which is only consumed during updateNSView. However, bridgeCommands is a final class held by @State (ComposerView.swift:80), so mutating its properties does not trigger SwiftUI observation — SwiftUI tracks the reference identity, not mutations to the object's fields. Since selectEmoji performs no @State or @Binding writes, SwiftUI has no reason to re-evaluate the view tree and call updateNSView.

All three call sites are affected: the onTab keyboard handler (ComposerView.swift:249), the performSendAction Enter-key path (ComposerView.swift:367), and the EmojiPickerPopup.onSelect button action (ComposerView.swift:113). The emoji insertion is silently deferred until an unrelated event triggers a view update. Worse, if the user types another character before that happens, .onChange(of: inputText) sets pendingSetText, which is processed before pendingReplaceRange in updateNSView (ComposerTextEditor.swift:210-228), potentially making the stored NSRange stale and corrupting text.

By contrast, all other bridge commands (pendingSetText, pendingSetEditable, pendingRequestFocus) are always set alongside a @State mutation that guarantees updateNSView is called.

Prompt for agents 
The root cause is that setting a property on the reference-type ComposerBridgeCommands (held by @State) does not trigger SwiftUI to call updateNSView on ComposerTextEditor. Unlike all other bridge commands (pendingSetText, pendingSetEditable, pendingRequestFocus), pendingReplaceRange is the only command not accompanied by a @State or @Binding mutation.

Possible approaches:

1. Keep direct invocation for immediate commands: Restore a mechanism similar to the old TextReplacementProxy where selectEmoji can synchronously call textView.insertText. The coordinator could expose a replaceText method that selectEmoji calls directly. This could coexist with the bridge pattern for deferred commands.

2. Trigger a @State change alongside pendingReplaceRange: Add a dummy @State counter (e.g. @State private var bridgeCommandGeneration = 0) that gets incremented whenever a bridge command is set that needs immediate processing. This forces SwiftUI to re-evaluate the body and call updateNSView. You would need to do this in selectEmoji (ComposerEmojiPicker.swift:18) and pass the counter as a property to ComposerTextEditor so SwiftUI detects the change.

3. Make ComposerBridgeCommands @Observable: If ComposerBridgeCommands were @Observable, property mutations would be tracked by SwiftUI and trigger view updates. However, this would also cause updateNSView to fire on every property set, which the current design tries to avoid for pendingSetText.

Approach 1 is the most targeted fix since pendingReplaceRange is the only command that needs synchronous execution — it originates from user-initiated actions (keyboard/click) that expect immediate visual feedback.
Open in Devin Review

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

}
}

Expand Down
173 changes: 110 additions & 63 deletions clients/macos/vellum-assistant/Features/Chat/ComposerTextEditor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,42 @@ import AppKit
import SwiftUI
import VellumAssistantShared

/// Proxy object that allows callers to programmatically replace text
/// in the composer's underlying NSTextView. Uses `insertText(_:replacementRange:)`
/// so replacements participate in the undo stack.
final class TextReplacementProxy {
var replaceText: ((NSRange, String) -> Void)?
// MARK: - Bridge Event & Command Contracts

/// Events emitted by the AppKit text view coordinator to notify the SwiftUI layer.
/// All callbacks are owned by the coordinator — the view installs them once in
/// `makeNSView` and they remain stable for the lifetime of the representable.
struct ComposerBridgeEvents {
/// Fired when the text content changes. Carries the new text value.
var textChanged: ((String) -> Void)?
/// Fired when the selection/cursor position changes. Carries UTF-16 offset.
var selectionChanged: ((Int) -> Void)?
/// Fired when the text view gains or loses first-responder status.
var focusChanged: ((Bool) -> Void)?
/// Fired when the user presses Return in a "send" configuration.
var submitRequested: (() -> Void)?
}

/// One-way commands that the SwiftUI layer can push into the AppKit text view.
/// The coordinator processes these during `updateNSView` and clears consumed
/// values so they don't re-fire on subsequent SwiftUI render passes.
final class ComposerBridgeCommands {
/// When non-nil, the coordinator sets the text view's string to this value
/// and clears the field. Only applied when the value differs from the
/// current text view content to avoid TextKit re-layout churn.
var pendingSetText: String?

/// When non-nil, the coordinator updates the text view's `isEditable`.
var pendingSetEditable: Bool?

/// When non-nil, the coordinator requests or resigns first-responder status.
/// Queued to the next main-turn to avoid mutating window state during
/// `updateNSView`.
var pendingRequestFocus: Bool?

/// When non-nil, the coordinator performs an undoable text replacement at
/// the given range. Used for emoji insertion and similar programmatic edits.
var pendingReplaceRange: (range: NSRange, replacement: String)?
}

/// NSScrollView subclass that reports intrinsic content size based on
Expand All @@ -31,9 +62,13 @@ final class IntrinsicScrollView: NSScrollView {
}

/// NSViewRepresentable wrapper that hosts a ``ComposerTextView`` inside an
/// ``IntrinsicScrollView``. Manages two-way text and focus binding with
/// SwiftUI, height measurement via TextKit layout, and callback wiring
/// for key events, image paste, and submit actions.
/// ``IntrinsicScrollView``. Communicates with SwiftUI exclusively through
/// the ``ComposerBridgeEvents`` (AppKit -> SwiftUI) and
/// ``ComposerBridgeCommands`` (SwiftUI -> AppKit) contracts.
///
/// The coordinator never writes to SwiftUI `@Binding` properties directly
/// from AppKit callbacks. Instead, events flow through the bridge callbacks
/// and the view layer decides how to translate them into state updates.
///
/// Ref: https://developer.apple.com/documentation/swiftui/nsviewrepresentable
struct ComposerTextEditor: NSViewRepresentable {
Expand All @@ -43,26 +78,27 @@ struct ComposerTextEditor: NSViewRepresentable {
static let textInsetX: CGFloat = 5 // lineFragmentPadding
static let textInsetY: CGFloat = 6 // textContainerInset.height

@Binding var text: String
@Binding var isFocused: Bool

let font: NSFont
let lineSpacing: CGFloat
let insertionPointColor: NSColor
let minHeight: CGFloat
let maxHeight: CGFloat
let isEditable: Bool
let cmdEnterToSend: Bool
var textColorOverride: NSColor? = nil
var onSubmit: (() -> Void)? = nil
var onTab: (() -> Bool)? = nil
var onUpArrow: (() -> Bool)? = nil
var onDownArrow: (() -> Bool)? = nil
var onEscape: (() -> Bool)? = nil
var onPasteImage: (() -> Void)? = nil
var shouldOverrideReturn: (() -> Bool)? = nil
@Binding var cursorPosition: Int
var textReplacer: TextReplacementProxy? = nil

/// Explicit bridge events — the coordinator fires these instead of
/// writing to `@Binding` properties.
let bridgeEvents: ComposerBridgeEvents

/// Explicit bridge commands — the view layer writes pending commands
/// here and the coordinator consumes them in `updateNSView`.
let bridgeCommands: ComposerBridgeCommands

func makeCoordinator() -> Coordinator {
Coordinator(parent: self)
Expand All @@ -77,7 +113,7 @@ struct ComposerTextEditor: NSViewRepresentable {
scrollView.autohidesScrollers = true
scrollView.scrollerStyle = .overlay

// Build an explicit TextKit 1 stack to avoid the implicit TextKit 2→1
// Build an explicit TextKit 1 stack to avoid the implicit TextKit 2->1
// downgrade that occurs when accessing `layoutManager` on a default
// NSTextView (macOS 12+). The downgrade causes visual glitches where
// typed text is invisible even though the insertion point renders.
Expand Down Expand Up @@ -134,12 +170,6 @@ struct ComposerTextEditor: NSViewRepresentable {
scrollView.documentView = textView
textView.delegate = context.coordinator

if let proxy = textReplacer {
proxy.replaceText = { [weak textView] range, replacement in
textView?.insertText(replacement, replacementRange: range)
}
}

let coordinator = context.coordinator

coordinator.frameObserver = NotificationCenter.default.addObserver(
Expand Down Expand Up @@ -171,21 +201,39 @@ struct ComposerTextEditor: NSViewRepresentable {
context.coordinator.parent = self
guard let textView = scrollView.documentView as? ComposerTextView else { return }

if textView.string != text {
textView.string = text
textView.scrollRangeToVisible(textView.selectedRange())
let coordinator = context.coordinator
let commands = bridgeCommands

// --- Process one-way commands ---

// setText command
if let pendingText = commands.pendingSetText {
commands.pendingSetText = nil
if textView.string != pendingText {
textView.string = pendingText
textView.scrollRangeToVisible(textView.selectedRange())
}
}

textView.isEditable = isEditable
// setEditable command
if let pendingEditable = commands.pendingSetEditable {
commands.pendingSetEditable = nil
textView.isEditable = pendingEditable
}

// replaceRange command
if let pending = commands.pendingReplaceRange {
commands.pendingReplaceRange = nil
textView.insertText(pending.replacement, replacementRange: pending.range)
}

// Guard attribute updates behind change checks to avoid triggering
// redundant TextKit re-layouts during the SwiftUI render cycle.
// Each keystroke fires textDidChange → binding update → updateNSView;
// Each keystroke fires textDidChange -> bridge event -> updateNSView;
// unconditionally re-stamping font/color/typingAttributes here would
// cause a layout pass on every character, which can leave glyphs
// un-drawn until the *next* display cycle (appearing invisible).
// Ref: WWDC 2022 "Use SwiftUI with AppKit" — only update changed props.
let coordinator = context.coordinator
// Ref: WWDC 2022 "Use SwiftUI with AppKit" -- only update changed props.
let textColor = textColorOverride ?? NSColor(VColor.contentDefault)

let fontChanged = coordinator.lastAppliedFont != font
Expand All @@ -212,33 +260,32 @@ struct ComposerTextEditor: NSViewRepresentable {
}

textView.cmdEnterToSend = cmdEnterToSend
textView.onSubmit = onSubmit
textView.onSubmit = bridgeEvents.submitRequested
textView.onTab = onTab
textView.onUpArrow = onUpArrow
textView.onDownArrow = onDownArrow
textView.onEscape = onEscape
textView.onPasteImage = onPasteImage
textView.shouldOverrideReturn = shouldOverrideReturn
textView.onFocusChanged = { [weak coordinator = context.coordinator] focused in
coordinator?.scheduleFocusBindingUpdate(focused)
}

if let proxy = textReplacer {
proxy.replaceText = { [weak textView] range, replacement in
textView?.insertText(replacement, replacementRange: range)
}
textView.onFocusChanged = { [weak coordinator] focused in
coordinator?.scheduleFocusEvent(focused)
}

// Re-strip drag types in case TextKit re-registered them during
// font or attribute updates above.
textView.unregisterDraggedTypes()

if let window = textView.window {
coordinator.scheduleFirstResponderUpdate(
in: window,
textView: textView,
shouldFocus: isFocused
)
// requestFocus command — queued to next main turn as a single
// bridge policy instead of scattered DispatchQueue.main.async calls.
if let shouldFocus = commands.pendingRequestFocus {
commands.pendingRequestFocus = nil
if let window = textView.window {
Comment on lines +280 to +282

@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.

P2 Badge Keep focus command queued until text view has a window

pendingRequestFocus is cleared before verifying textView.window exists, so focus requests issued during mount/transition can be dropped when updateNSView runs before window attachment. In those cases there is no retry, which can leave the composer unfocused even though focusIntent is true.

Useful? React with 👍 / 👎.

coordinator.scheduleFirstResponderUpdate(
in: window,
textView: textView,
shouldFocus: shouldFocus
)
}
}

context.coordinator.measureHeight(textView)
Expand Down Expand Up @@ -267,7 +314,7 @@ struct ComposerTextEditor: NSViewRepresentable {
var lastAppliedFont: NSFont?
var lastAppliedLineSpacing: CGFloat?
var lastAppliedTextColor: NSColor?
var pendingFocusBindingValue: Bool?
var pendingFocusEventValue: Bool?
var pendingFirstResponderValue: Bool?
weak var textView: ComposerTextView?

Expand All @@ -278,13 +325,12 @@ struct ComposerTextEditor: NSViewRepresentable {
func textDidChange(_ notification: Notification) {
guard let textView = notification.object as? NSTextView else { return }
let newText = textView.string
if parent.text != newText {
parent.text = newText
}
// Fire bridge event instead of writing to @Binding directly
parent.bridgeEvents.textChanged?(newText)

let pos = textView.selectedRange().location
if parent.cursorPosition != pos {
parent.cursorPosition = pos
}
parent.bridgeEvents.selectionChanged?(pos)

measureHeight(textView)
}

Expand All @@ -293,19 +339,18 @@ struct ComposerTextEditor: NSViewRepresentable {
// becomeFirstResponder / resignFirstResponder callbacks.
// This delegate fires only once editing begins (on first
// keyDown), so it serves as a secondary sync only.
scheduleFocusBindingUpdate(true)
scheduleFocusEvent(true)
}

func textDidEndEditing(_ notification: Notification) {
scheduleFocusBindingUpdate(false)
scheduleFocusEvent(false)
}

func textViewDidChangeSelection(_ notification: Notification) {
guard let textView = notification.object as? NSTextView else { return }
let pos = textView.selectedRange().location
if parent.cursorPosition != pos {
parent.cursorPosition = pos
}
// Fire bridge event instead of writing to @Binding directly
parent.bridgeEvents.selectionChanged?(pos)
}

func measureHeight(_ textView: NSTextView) {
Expand All @@ -322,14 +367,16 @@ struct ComposerTextEditor: NSViewRepresentable {
}
}

func scheduleFocusBindingUpdate(_ focused: Bool) {
pendingFocusBindingValue = focused
/// Schedules a focus event to fire on the next main turn.
/// Coalesces rapid focus changes (e.g. becomeFirstResponder
/// followed immediately by resignFirstResponder) by checking
/// the intended value hasn't been superseded.
func scheduleFocusEvent(_ focused: Bool) {
pendingFocusEventValue = focused
DispatchQueue.main.async { [weak self] in
guard let self, self.pendingFocusBindingValue == focused else { return }
self.pendingFocusBindingValue = nil
if self.parent.isFocused != focused {
self.parent.isFocused = focused
}
guard let self, self.pendingFocusEventValue == focused else { return }
self.pendingFocusEventValue = nil
self.parent.bridgeEvents.focusChanged?(focused)
}
}

Expand Down
Loading