[LUM-832] Fix cursor flickering in chat composer by making updateNSView efficient at the NSViewRepresentable boundary#25597
Conversation
Remove @State var cursorPosition from ComposerView — every keystroke and selection change wrote to this binding, forcing a full SwiftUI body re-evaluation cycle that caused the NSTextView insertion point to flicker and sometimes block text deletion/submission. Cursor position now flows directly from the NSTextView delegate to ComposerController via a closure callback, bypassing SwiftUI state entirely. ComposerController's observable property mutations are also guarded with equality checks to prevent no-op @observable change notifications from triggering unnecessary view updates. Closes LUM-832 Co-Authored-By: tkheyfets <timur@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…ndary Addresses the architectural root cause of cursor flickering: updateNSView doing heavyweight work on every keystroke. The previous fix only removed the cursorPosition binding; this overhaul makes the entire NSViewRepresentable boundary efficient by default. Changes: - Route all ComposerTextView callbacks through the Coordinator (set once in makeNSView, not reassigned per-update). Closures capture coordinator weakly and forward to coordinator.parent which is refreshed each updateNSView. - Guard every updateNSView property update behind change detection: isEditable, cmdEnterToSend, focus, and unregisterDraggedTypes only fire when their inputs actually changed. - Replace focus @binding with one-way let + onFocusChanged callback, matching the cursor-position callback pattern. Eliminates the binding round-trip that caused body re-evaluation on every NSTextView focus change. - Collapse 3 focus state variables (composerFocus, isComposerFocused, textViewIsFocused) into 1 (composerFocus). Removes the triangular onChange dependency that amplified re-evaluation cascades. - Remove redundant measureHeight call from updateNSView — frame/bounds notification observers already handle this. Only re-measure after external text replacement or font changes. - Seed Coordinator last-applied tracking in makeNSView so the first updateNSView skips properties that match their initial values. Part of LUM-832 Co-Authored-By: tkheyfets <timur@vellum.ai>
devin-ai-integration
Bot
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Re-strip drag types only when TextKit may have re-registered | ||
| // them (after font or attribute changes), not on every keystroke. | ||
| if fontChanged || colorChanged { | ||
| textView.unregisterDraggedTypes() | ||
| } |
There was a problem hiding this comment.
🚩 unregisterDraggedTypes guard may miss edge cases
The old code called textView.unregisterDraggedTypes() unconditionally on every updateNSView. The new code at ComposerTextEditor.swift:286-288 only calls it when fontChanged || colorChanged || textWasExternallyReplaced. This is based on the assumption that TextKit only re-registers drag types during attribute changes or text storage manipulation. If there are other AppKit code paths that silently re-register drag types (e.g., during scroll view layout, window activation), file paths could be inserted as text on drop. The previous unconditional call was defensive. This is likely fine in practice given the .onDrop modifier on the composer container handles file drops at the SwiftUI level.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid concern. Widened the guard in 25b21d0 to also call unregisterDraggedTypes() after external text replacement (textWasExternallyReplaced), since NSTextStorage manipulation can trigger drag type re-registration beyond just font/color changes. Normal per-keystroke typing (where text flows through the textDidChange delegate, not through updateNSView text sync) still skips the call.
Address two Devin Review findings: 1. Move lastFocused update inside the window-availability check so that if textView.window is nil during a view hierarchy transition, the focus request is retried on the next updateNSView call instead of being permanently lost. 2. Widen unregisterDraggedTypes guard to also fire after external text replacement (textWasExternallyReplaced), since NSTextStorage manipulation can trigger drag type re-registration beyond just font/color changes. Normal per-keystroke typing still skips it. Part of LUM-832 Co-Authored-By: tkheyfets <timur@vellum.ai>
Adopt the callback-based focus / cursor-position wiring and per-property\nchange guards introduced by #25597, and keep the measureHeight coalescer\n+ skip-guard from this branch. The guarded `if textWasExternallyReplaced\n|| fontChanged` re-measure in updateNSView now routes through\n`scheduleMeasureHeight` so it coalesces with frame/bounds notifications\nthat the same event will fire. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
1 participant
Fixes cursor flickering in the chat composer by making
updateNSViewessentially a no-op for text-only changes — the previous implementation reassigned ~10 closures, calledunregisterDraggedTypes(), scheduled focus updates, and forced TextKitensureLayouton every keystroke. The fix routes all callbacks through the Coordinator (wired once inmakeNSView), guards every property update behind change detection, replaces the focus@Bindingwith a one-way value + callback (eliminating the 3-variable focus dance), and removes redundantmeasureHeightcalls.