Skip to content

refactor: isolate composer appkit bridge behind explicit events#24401

Merged
siddseethepalli merged 1 commit into
mainfrom
chat-surface-stab/pr-9-composer-text-bridge
Apr 8, 2026
Merged

refactor: isolate composer appkit bridge behind explicit events#24401
siddseethepalli merged 1 commit into
mainfrom
chat-surface-stab/pr-9-composer-text-bridge

Conversation

@siddseethepalli

@siddseethepalli siddseethepalli commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace AppKit binding mutations with explicit event callbacks and one-way commands
  • Move focus synchronization out of updateNSView hot path through controller
  • Remove manual reconciliation of composerFocus/textViewIsFocused in ComposerView

Part of plan: macos-chat-surface-stabilization.md (PR 9 of 11)


Open with Devin

Replace direct SwiftUI binding mutations from AppKit callbacks with explicit event/command
bridge contract. Route focus synchronization through ComposerController instead of
scattered DispatchQueue.main.async calls in updateNSView.
@siddseethepalli siddseethepalli merged commit 5a4b566 into main Apr 8, 2026
@siddseethepalli siddseethepalli deleted the chat-surface-stab/pr-9-composer-text-bridge branch April 8, 2026 19:11

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ec0c26108

ℹ️ 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".

Comment on lines 334 to +338
.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

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 Push initial composer text through bridge on first render

The bridge now only writes into NSTextView when pendingSetText is populated, but this command is assigned only inside .onChange(of: inputText). Because onChange does not fire for the initial value, any prefilled draft/starter text present when the composer appears is not rendered in AppKit, and the first keystroke can overwrite that unseen draft via the bridge event path.

Useful? React with 👍 / 👎.

Comment on lines +280 to +282
if let shouldFocus = commands.pendingRequestFocus {
commands.pendingRequestFocus = nil
if let window = textView.window {

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

@devin-ai-integration devin-ai-integration Bot left a comment

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.

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

let nsRange = NSRange(location: colonOffset, length: length)

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

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.

TirmanSidhu added a commit that referenced this pull request Apr 8, 2026
This reverts commit 5a4b566.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TirmanSidhu added a commit that referenced this pull request Apr 8, 2026
#24417)

This reverts commit 5a4b566.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant