diff --git a/clients/macos/vellum-assistant/Features/Chat/ComposerEmojiPicker.swift b/clients/macos/vellum-assistant/Features/Chat/ComposerEmojiPicker.swift index 1b7305a3c5d..c1691b8166e 100644 --- a/clients/macos/vellum-assistant/Features/Chat/ComposerEmojiPicker.swift +++ b/clients/macos/vellum-assistant/Features/Chat/ComposerEmojiPicker.swift @@ -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) } } diff --git a/clients/macos/vellum-assistant/Features/Chat/ComposerTextEditor.swift b/clients/macos/vellum-assistant/Features/Chat/ComposerTextEditor.swift index 66b0a41dd81..8b0c15f61c8 100644 --- a/clients/macos/vellum-assistant/Features/Chat/ComposerTextEditor.swift +++ b/clients/macos/vellum-assistant/Features/Chat/ComposerTextEditor.swift @@ -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 @@ -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 { @@ -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) @@ -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. @@ -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( @@ -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 @@ -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 { + coordinator.scheduleFirstResponderUpdate( + in: window, + textView: textView, + shouldFocus: shouldFocus + ) + } } context.coordinator.measureHeight(textView) @@ -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? @@ -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) } @@ -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) { @@ -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) } } diff --git a/clients/macos/vellum-assistant/Features/Chat/ComposerView.swift b/clients/macos/vellum-assistant/Features/Chat/ComposerView.swift index facd5fc62d3..4ea4e81557f 100644 --- a/clients/macos/vellum-assistant/Features/Chat/ComposerView.swift +++ b/clients/macos/vellum-assistant/Features/Chat/ComposerView.swift @@ -70,13 +70,14 @@ struct ComposerView: View { #if os(macOS) @Environment(\.dropActions) private var dropActions #endif - @State private var composerFocus: Bool = false @State private var isComposerFocused = false - @State private var textViewIsFocused: Bool = false @State var cursorPosition: Int = 0 - @State var textReplacer = TextReplacementProxy() @State var composerController = ComposerController() + /// Bridge commands object shared with ComposerTextEditor. The view layer + /// writes pending commands here; the coordinator consumes them during + /// updateNSView. + @State var bridgeCommands = ComposerBridgeCommands() /// Live amplitude from VoiceInputManager, bypassing ChatViewModel's 100ms coalescing. @State private var liveAmplitude: Float = 0 @@ -141,12 +142,13 @@ struct ComposerView: View { .task { // Delay focus slightly so the NSTextView is fully installed // in the view hierarchy before requesting first-responder - // status. Setting @FocusState synchronously during an animated + // status. Setting focus synchronously during an animated // layout pass (e.g. the empty-state fade-in) can give logical // focus without rendering the blinking caret. try? await Task.sleep(nanoseconds: 50_000_000) guard !Task.isCancelled else { return } - composerFocus = isInteractionEnabled + composerController.focusChanged(isInteractionEnabled) + pushFocusCommand() } .task(id: conversationId) { guard isInteractionEnabled, !hasPendingConfirmation else { return } @@ -154,7 +156,8 @@ struct ComposerView: View { // (new empty state) whose layout isn't settled yet. try? await Task.sleep(nanoseconds: 50_000_000) guard !Task.isCancelled else { return } - composerFocus = true + composerController.focusChanged(true) + pushFocusCommand() } .onChange(of: currentMode) { composerLog.debug("Composer mode: \(String(describing: currentMode))") @@ -164,11 +167,12 @@ struct ComposerView: View { } .onChange(of: isInteractionEnabled) { _, enabled in composerController.interactionEnabledChanged(enabled, hasPendingConfirmation: hasPendingConfirmation) - composerFocus = composerController.focusIntent + pushFocusCommand() } .onChange(of: hasPendingConfirmation) { _, pending in if !pending, isInteractionEnabled { - composerFocus = true + composerController.focusChanged(true) + pushFocusCommand() } } } @@ -225,18 +229,14 @@ struct ComposerView: View { .padding(.leading, ComposerTextEditor.textInsetX) .padding(.top, ComposerTextEditor.textInsetY) ComposerTextEditor( - text: $inputText, - isFocused: $textViewIsFocused, font: nsFont, lineSpacing: 4, insertionPointColor: NSColor(VColor.primaryBase), minHeight: composerActionButtonSize, maxHeight: composerMaxHeight, - isEditable: isInteractionEnabled, cmdEnterToSend: cmdEnterToSend, textColorOverride: hasSlashHighlight ? NSColor(VColor.contentDefault).withAlphaComponent(0) : nil, - onSubmit: { performSendAction() }, onTab: { if composerController.showSlashMenu { if let command = composerController.handleSlashNavigation(.tab) { @@ -272,8 +272,31 @@ struct ComposerView: View { shouldOverrideReturn: { composerController.isPopupVisible }, - cursorPosition: $cursorPosition, - textReplacer: textReplacer + bridgeEvents: ComposerBridgeEvents( + textChanged: { [self] newText in + if inputText != newText { inputText = newText } + composerController.textChanged(newText) + // Keep local cursor tracking in sync for emoji picker + // (cursorPosition is emitted separately via selectionChanged) + }, + selectionChanged: { [self] pos in + if cursorPosition != pos { cursorPosition = pos } + composerController.cursorMoved(to: pos) + }, + focusChanged: { [self] focused in + composerController.focusChanged(focused) + isComposerFocused = focused + if focused { + if let window = NSApp.keyWindow as? TitleBarZoomableWindow { + window.clearComposerDismissed() + } + } + }, + submitRequested: { [self] in + performSendAction() + } + ), + bridgeCommands: bridgeCommands ) .fixedSize(horizontal: false, vertical: true) // Prevent inherited .animation() modifiers from creating animation @@ -288,30 +311,15 @@ struct ComposerView: View { .frame(maxWidth: .infinity) .background( ComposerFocusBridge( - isFocused: composerFocus, + isFocused: composerController.focusIntent, isInteractionEnabled: isInteractionEnabled, onRedirectKeystroke: { chars in inputText += chars - composerFocus = true + composerController.focusChanged(true) + pushFocusCommand() } ) ) - .onChange(of: composerFocus) { - if textViewIsFocused != composerFocus { - textViewIsFocused = composerFocus - } - isComposerFocused = composerFocus - if composerFocus { - if let window = NSApp.keyWindow as? TitleBarZoomableWindow { - window.clearComposerDismissed() - } - } - } - .onChange(of: textViewIsFocused) { - if composerFocus != textViewIsFocused { - composerFocus = textViewIsFocused - } - } .onReceive(NotificationCenter.default.publisher(for: NSApplication.didBecomeActiveNotification)) { _ in guard !hasPendingConfirmation else { return } guard let window = NSApp.keyWindow as? TitleBarZoomableWindow else { return } @@ -320,16 +328,29 @@ struct ComposerView: View { window.composerContainerView.map({ !responder.isDescendant(of: $0) }) ?? false { return } - composerFocus = true + composerController.focusChanged(true) + pushFocusCommand() } .onChange(of: inputText) { + // Push text changes from external sources (e.g. dictation cancel + // restoring preDictationText) into the AppKit text view via the + // bridge command, and keep the controller in sync. + bridgeCommands.pendingSetText = inputText composerController.textChanged(inputText) } - .onChange(of: cursorPosition) { - composerController.cursorMoved(to: cursorPosition) + .onChange(of: isInteractionEnabled) { + bridgeCommands.pendingSetEditable = isInteractionEnabled } } + /// Pushes the controller's current focusIntent into the bridge commands + /// so the coordinator can request/resign first responder on the next + /// main turn. This is the single policy point for focus synchronization. + private func pushFocusCommand() { + bridgeCommands.pendingRequestFocus = composerController.focusIntent + isComposerFocused = composerController.focusIntent + } + /// Shared send logic invoked by the composer's submit callback. /// Handles slash-menu selection and pending-confirmation approval /// regardless of how "send" is triggered. @@ -459,7 +480,8 @@ struct ComposerView: View { isDisabled: !canSend, iconSize: composerActionButtonSize ) { - composerFocus = true + composerController.focusChanged(true) + pushFocusCommand() performSendAction() } .vTooltip("Type a message to send") @@ -483,7 +505,8 @@ struct ComposerView: View { isDisabled: !canSend, iconSize: composerActionButtonSize ) { - composerFocus = true + composerController.focusChanged(true) + pushFocusCommand() performSendAction() } .vTooltip(canSend ? "Send" : "Type a message to send") @@ -515,7 +538,8 @@ struct ComposerView: View { isDisabled: !canSend, iconSize: composerActionButtonSize ) { - composerFocus = true + composerController.focusChanged(true) + pushFocusCommand() performSendAction() } } diff --git a/clients/macos/vellum-assistantTests/ComposerReturnKeyRoutingTests.swift b/clients/macos/vellum-assistantTests/ComposerReturnKeyRoutingTests.swift index 1fcb49cd1ee..8ad7a1d595b 100644 --- a/clients/macos/vellum-assistantTests/ComposerReturnKeyRoutingTests.swift +++ b/clients/macos/vellum-assistantTests/ComposerReturnKeyRoutingTests.swift @@ -71,5 +71,119 @@ final class ComposerReturnKeyRoutingTests: XCTestCase { XCTAssertEqual(action, .send) } + // MARK: - Bridge contract: event-based communication + + /// Verifies that ComposerBridgeEvents provides the explicit callback + /// contract that replaced synchronous @Binding writes from AppKit callbacks. + /// The bridge fires events rather than mutating SwiftUI-owned state directly. + @MainActor + func testBridgeEvents_textChangedFiresCallback() { + var receivedText: String? + let events = ComposerBridgeEvents( + textChanged: { receivedText = $0 }, + selectionChanged: nil, + focusChanged: nil, + submitRequested: nil + ) + + events.textChanged?("hello") + XCTAssertEqual(receivedText, "hello") + } + + @MainActor + func testBridgeEvents_selectionChangedFiresCallback() { + var receivedPosition: Int? + let events = ComposerBridgeEvents( + textChanged: nil, + selectionChanged: { receivedPosition = $0 }, + focusChanged: nil, + submitRequested: nil + ) + + events.selectionChanged?(42) + XCTAssertEqual(receivedPosition, 42) + } + + @MainActor + func testBridgeEvents_focusChangedFiresCallback() { + var receivedFocus: Bool? + let events = ComposerBridgeEvents( + textChanged: nil, + selectionChanged: nil, + focusChanged: { receivedFocus = $0 }, + submitRequested: nil + ) + + events.focusChanged?(true) + XCTAssertEqual(receivedFocus, true) + + events.focusChanged?(false) + XCTAssertEqual(receivedFocus, false) + } + + @MainActor + func testBridgeEvents_submitRequestedFiresCallback() { + var submitCalled = false + let events = ComposerBridgeEvents( + textChanged: nil, + selectionChanged: nil, + focusChanged: nil, + submitRequested: { submitCalled = true } + ) + + events.submitRequested?() + XCTAssertTrue(submitCalled) + } + + // MARK: - Bridge commands: one-way commands consumed by coordinator + + @MainActor + func testBridgeCommands_pendingSetTextIsConsumedOnRead() { + let commands = ComposerBridgeCommands() + commands.pendingSetText = "new text" + XCTAssertEqual(commands.pendingSetText, "new text") + + // Simulate coordinator consuming the command + let consumed = commands.pendingSetText + commands.pendingSetText = nil + XCTAssertEqual(consumed, "new text") + XCTAssertNil(commands.pendingSetText) + } + + @MainActor + func testBridgeCommands_pendingRequestFocusIsConsumedOnRead() { + let commands = ComposerBridgeCommands() + commands.pendingRequestFocus = true + XCTAssertEqual(commands.pendingRequestFocus, true) + + // Simulate coordinator consuming the command + commands.pendingRequestFocus = nil + XCTAssertNil(commands.pendingRequestFocus) + } + + @MainActor + func testBridgeCommands_pendingReplaceRangeIsConsumedOnRead() { + let commands = ComposerBridgeCommands() + commands.pendingReplaceRange = (range: NSRange(location: 5, length: 3), replacement: "emoji") + + XCTAssertNotNil(commands.pendingReplaceRange) + XCTAssertEqual(commands.pendingReplaceRange?.range, NSRange(location: 5, length: 3)) + XCTAssertEqual(commands.pendingReplaceRange?.replacement, "emoji") + + // Simulate coordinator consuming the command + commands.pendingReplaceRange = nil + XCTAssertNil(commands.pendingReplaceRange) + } + + @MainActor + func testBridgeCommands_pendingSetEditableIsConsumedOnRead() { + let commands = ComposerBridgeCommands() + commands.pendingSetEditable = false + XCTAssertEqual(commands.pendingSetEditable, false) + + commands.pendingSetEditable = nil + XCTAssertNil(commands.pendingSetEditable) + } + } #endif