perf: cut composer typing latency#325
Merged
Merged
Conversation
Three layers of work were happening on every keystroke in the chat composer, compounding into noticeable input lag: - ChatView owned `messageText` as @State, so every keystroke invalidated ChatView's full body and walked the entire chat tree. Moved text into an @observable `ComposerTextStore` and introduced small hosting views (`ChatComposerBindingHost`, `ChatCollapsedComposerBarHost`, `ChatComposerTextChangeObserver`) that scope the observation to the composer subtree. - NSTextView's `textDidChange` synchronously wrote the SwiftUI binding, gating native character display on the SwiftUI update cycle. Deferred the write via `DispatchQueue.main.async` with a coalescing flag, and taught `updateNSView` to skip `syncExternalTextIfNeeded` while a flush is pending so a stale binding cannot overwrite in-flight typing. - Slash-command detection ran a regex with lookbehind on the full string every keystroke, the expanded composer recomputed word/char counts via `components(separatedBy:)` on every body call, and the height callback fired unconditionally. Replaced the regex with a manual tail-scan plus a cheap precheck, cached draft metrics in @State, and added a last-height guard in the Coordinator.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refactors ChatView's composer text state to prevent body invalidation on every keystroke. Composer text moves to an observable ChangesComposer Text State & Binding Decoupling
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Sources/UI/ChatView.swift (1)
83-88: ⚡ Quick winAdd the
private_swiftui_stateSwiftLint pragma for consistency.SwiftLint flags
composerTextStore(it must be accessible fromChatViewextensions, so it can't beprivate). Other@Statevars in this file that have the same constraint already carry// swiftlint:disable:next private_swiftui_state(see lines 91, 96, 117, 132, etc.). Add it here to silence the warning and match the convention.♻️ Proposed change
// Composer text lives in an `@Observable` store so per-keystroke writes do // not invalidate ChatView's body. Read `composerTextStore.text` only from // action handlers (or via the `messageText` accessor); body-context reads // must go through ChatComposerBindingHost / ChatComposerTextChangeObserver // so the dependency is scoped to those small wrapper views. + // swiftlint:disable:next private_swiftui_state `@State` var composerTextStore = ComposerTextStore()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/UI/ChatView.swift` around lines 83 - 88, Add the SwiftLint pragma to silence the private_swiftui_state warning for the ChatView `@State` property: above the declaration of composerTextStore (the `@State` var composerTextStore = ComposerTextStore() inside ChatView), insert the comment // swiftlint:disable:next private_swiftui_state so it matches the other `@State` properties in this file and remains accessible to ChatView extensions.Sources/UI/ComposerTextStore.swift (1)
9-12: 💤 Low valueConsider marking
ComposerTextStoreas@MainActor.This store holds UI-bound composer text mutated from SwiftUI views and from
Task {@mainactorin ... }action handlers (e.g.,toggleSpeechToTextinChatView+ComposerHelpers.swift). Annotating the type with@MainActordocuments and enforces main-thread access, which is consistent with the repo guideline to use@MainActorfor UI-bound state.♻️ Proposed change
`@Observable` +@MainActor final class ComposerTextStore { var text: String = "" }As per coding guidelines: "Prefer
actorfor shared mutable state, use@MainActorfor UI-bound state".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/UI/ComposerTextStore.swift` around lines 9 - 12, Mark the UI-bound store ComposerTextStore with `@MainActor` so its mutations are enforced on the main thread; update the declaration of final class ComposerTextStore to be `@MainActor` final class ComposerTextStore (affecting the class that contains var text: String) to ensure SwiftUI views and Task { `@MainActor` in ... } handlers like toggleSpeechToText access it on the main actor.Sources/UI/ChatComposerStateHosts.swift (1)
7-58: ⚡ Quick winAlign host view type/file naming with Swift UI conventions
This file introduces multiple primary view types and none follow the
*Viewsuffix rule. Please split these into separate files (or clearly define one primary type per file) and rename types to end withViewfor consistency and discoverability.As per coding guidelines, "
Sources/UI/**/*.swift: SwiftUI views should use the naming pattern*View" and "**/*.swift: Use one primary type per file, with the filename matching the type name".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/UI/ChatComposerStateHosts.swift` around lines 7 - 58, Rename the three SwiftUI host types to follow the *View suffix and place each primary type in its own file whose filename matches the type: rename ChatComposerBindingHost -> ChatComposerBindingView, ChatCollapsedComposerBarHost -> ChatCollapsedComposerBarView, and ChatComposerTextChangeObserver -> ChatComposerTextChangeObserverView; move each into a new file named exactly after the new type, update any call sites/imports to use the new type names, and keep the internal properties and body implementations unchanged so behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/UI/ChatComposerStateHosts.swift`:
- Line 35: The collapsed composer "hasContent" check currently uses
composerTextStore.text.isEmpty which treats whitespace-only input as content;
change it to check trimmed text instead (use composerTextStore.text trimmed via
trimmingCharacters(in: .whitespacesAndNewlines)) so hasContent is true only when
the trimmed text is non-empty or hasOtherContent is true; update the expression
that sets hasContent in ChatComposerStateHosts (the place constructing the
collapsed composer state) to use the trimmed check to match sendability logic.
In `@Sources/UI/DroppableTextEditor`+Coordinator.swift:
- Around line 15-16: The deferred binding flush can overwrite intentional
updates and leave hasPendingBindingFlush stuck; fix by (1) when scheduling the
async flush capture the current textBinding value (and/or previousBindingValue)
and store it with hasPendingBindingFlush, (2) in the async block first
clear/reset hasPendingBindingFlush and bail if textView is nil, and (3) before
assigning to textBinding compare the current textBinding to the captured
previous value and only assign if they still match (this preserves intentional
in-between updates like clear-on-submit) while ensuring hasPendingBindingFlush
is always reset even if textView is nil; update the code around uses of
hasPendingBindingFlush, textView, and textBinding to implement this
check-and-reset behavior.
In `@Sources/UI/ExpandedComposerOverlay.swift`:
- Line 47: draftMetrics is initialized empty causing a one-frame flash when
expanding with existing text; update the view to synchronously initialize
draftMetrics from the messageText binding (e.g. provide a custom init that sets
_draftMetrics = State(initialValue: ComposerDraftTextMetrics(messageText:
messageText)) or otherwise assign draftMetrics before the async focus dispatch)
and also apply the same fix to the other instance of ComposerDraftTextMetrics
initialization referenced in the file so ExpandedComposerFooter renders correct
metrics on first render.
---
Nitpick comments:
In `@Sources/UI/ChatComposerStateHosts.swift`:
- Around line 7-58: Rename the three SwiftUI host types to follow the *View
suffix and place each primary type in its own file whose filename matches the
type: rename ChatComposerBindingHost -> ChatComposerBindingView,
ChatCollapsedComposerBarHost -> ChatCollapsedComposerBarView, and
ChatComposerTextChangeObserver -> ChatComposerTextChangeObserverView; move each
into a new file named exactly after the new type, update any call sites/imports
to use the new type names, and keep the internal properties and body
implementations unchanged so behavior is preserved.
In `@Sources/UI/ChatView.swift`:
- Around line 83-88: Add the SwiftLint pragma to silence the
private_swiftui_state warning for the ChatView `@State` property: above the
declaration of composerTextStore (the `@State` var composerTextStore =
ComposerTextStore() inside ChatView), insert the comment //
swiftlint:disable:next private_swiftui_state so it matches the other `@State`
properties in this file and remains accessible to ChatView extensions.
In `@Sources/UI/ComposerTextStore.swift`:
- Around line 9-12: Mark the UI-bound store ComposerTextStore with `@MainActor` so
its mutations are enforced on the main thread; update the declaration of final
class ComposerTextStore to be `@MainActor` final class ComposerTextStore
(affecting the class that contains var text: String) to ensure SwiftUI views and
Task { `@MainActor` in ... } handlers like toggleSpeechToText access it on the
main actor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e803c7cc-0ae8-45e9-bdc1-4f7f4deec71c
📒 Files selected for processing (12)
Sources/UI/ChatComposerStateHosts.swiftSources/UI/ChatView+ComposerHelpers.swiftSources/UI/ChatView+ComposerViews.swiftSources/UI/ChatView+FloatingComposer.swiftSources/UI/ChatView+SlashCommands.swiftSources/UI/ChatView.swiftSources/UI/ComposerTextStore.swiftSources/UI/DroppableTextEditor+Coordinator.swiftSources/UI/DroppableTextEditor.swiftSources/UI/ExpandedComposerOverlay.swiftSources/UI/SlashCommandMCPSupport.swiftTests/JinTests/ChatComposerSupportTests.swift
- ChatCollapsedComposerBarHost: use trimmed text for `hasContent` so a whitespace-only draft doesn't paint the collapsed chip as having content (matches canSendDraft semantics). - DroppableTextEditor+Coordinator: capture the binding value at schedule time and skip the async flush when the binding has been mutated externally before it runs (e.g. clear-on-submit). Reset `hasPendingBindingFlush` via `defer` so the flag can never get stuck if the textView has been deallocated. - ExpandedComposerOverlay: seed `draftMetrics` from the messageText binding in a custom init so the first render of the footer reflects the existing draft instead of flashing "0 words · 0 characters" for one frame when expanding from compact. - ChatView: add the `private_swiftui_state` swiftlint pragma above `composerTextStore` to match the convention used by other @State vars in the file. - ComposerTextStore: mark `@MainActor` per repo guidelines for UI-bound state. Skipped CodeRabbit's nitpick to rename the helper hosts to `*View` and split them into one-type-per-file. The repo already groups small helper view types together (`MessageRowSupportViews.swift`, `ExpandedComposerOverlayChromeViews.swift`, etc.) and the `ChatComposerStateHosts.swift` filename follows that same pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The chat composer felt noticeably laggier than other native macOS apps. Three layers of per-keystroke work were stacking up:
messageTextwas@StateonChatView, so any text change invalidated the whole chat tree (composer + message timeline scaffolding). Moved the text into an@ObservableComposerTextStore. Introduced small wrapper views —ChatComposerBindingHost,ChatCollapsedComposerBarHost,ChatComposerTextChangeObserver— that observestore.textlocally.ChatView's body no longer reads the text in any body context; action handlers still access it via amessageTextaccessor (safe outside body).Coordinator.textDidChangeused to write the binding synchronously, which meant the keystroke handler couldn't return until SwiftUI scheduled its update. Now the binding write is deferred viaDispatchQueue.main.asyncwith a coalescing flag;updateNSViewskipssyncExternalTextIfNeededwhile a flush is pending so a stale binding cannot overwrite the in-flight string when an unrelated state change triggers a re-render between keystrokes.SlashCommandDetection.detectFilter's regex-with-lookbehind with a manual tail-scan and added amayContainActiveTokenprecheck. CachedComposerDraftTextMetricsin@State(instead of recomputingcomponents(separatedBy:)on every body call in the expanded composer). Added a last-reported-height guard inCoordinatorso the height callback no longer fires for typing that stays within a single line.Files of note
Sources/UI/ComposerTextStore.swift(new) —@Observabletext store.Sources/UI/ChatComposerStateHosts.swift(new) — three small wrappers that scope the observation.Sources/UI/ChatView.swift,ChatView+ComposerHelpers.swift,ChatView+ComposerViews.swift,ChatView+FloatingComposer.swift— switch to the store + wrappers; add the action-contextmessageTextaccessor.Sources/UI/DroppableTextEditor+Coordinator.swift,DroppableTextEditor.swift— deferred binding flush, height-cache guard.Sources/UI/SlashCommandMCPSupport.swift,ChatView+SlashCommands.swift— manual tail-scan + precheck.Sources/UI/ExpandedComposerOverlay.swift— cached draft metrics.Test plan
swift buildclean.swift test— full suite, 2260 tests, 0 failures.swift test --filter ChatComposerSupportTests— includes new edge-case coverage fordetectFilter(empty string, whitespace-only, no-slash, newline/tab boundary, trailing whitespace) plusmayContainActiveTokenshort-circuit behavior../Packaging/package.sh— release.appbuilds successfully./→ popover appears; type to filter → responsive; Escape → dismisses.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Tests