Repository navigation
Fix TextBox IME composition rendering - #8688
Merged
Merged
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesIME composition layout
SSH working-directory derivation
Test runtime stability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant IMETest
participant TextBoxInputTextView
participant TextBoxInputViewCoordinator
IMETest->>TextBoxInputTextView: setMarkedText(preedit)
TextBoxInputTextView->>TextBoxInputTextView: layout and measure fragments
TextBoxInputTextView->>TextBoxInputViewCoordinator: report measured layout
TextBoxInputViewCoordinator->>TextBoxInputTextView: recalculate height
🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
✨ 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 |
Contributor
Greptile SummaryThis PR fixes a long-standing AppKit gap where
Confidence Score: 5/5Safe to merge. The production changes are a focused, well-tested AppKit integration fix with no new global state, no blocking primitives, and no user-facing string changes. All three production changes (IME layout scheduling, trailing-slash path fix, spurious-await removal) are narrowly scoped and each comes with a clear mechanical explanation. The recalculateHeight refactor moves ensureLayout before recenterSingleLineTextContainer (correct ordering) and uses an optional lineFragmentCount parameter to avoid double measurement on the layout path without changing the direct-call behavior. The layout() override's guard against re-entrant callbacks is preserved. The new regression test exercises the real NSTextInputClient entrypoint through a live NSWindow, and red/green CI runs validate the before/after behavior. No correctness risks were identified. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant IME as IME Input
participant TV as TextBoxInputTextView
participant LM as NSLayoutManager
participant C as Coordinator
participant SW as SwiftUI Binding
Note over IME,SW: Old path — height not updated during composition
IME->>TV: setMarkedText(_:selectedRange:replacementRange:)
TV->>LM: (TextKit layout updates internally)
Note over TV: textDidChange NOT fired
Note over SW: textViewHeight stays at committed-text-only height
Note over IME,SW: New path — height updated before commit
IME->>TV: setMarkedText(_:selectedRange:replacementRange:)
TV->>TV: super.setMarkedText (TextKit updates glyphs)
TV->>TV: onMarkedTextStateChanged(true)
TV->>TV: "needsLayout = true, needsDisplay = true"
TV->>TV: layout() triggered by AppKit
TV->>LM: ensureLayout(for: textContainer)
TV->>TV: visualLineFragmentCount()
TV->>TV: recenterSingleLineTextContainer(lineFragmentCount:)
TV->>C: onLayoutCompleted(self, lineFragmentCount)
C->>C: recalculateHeight(_:lineFragmentCount:)
C->>LM: ensureLayout (idempotent)
C->>TV: "frame.size.height = preferredHeight"
C->>SW: "textViewHeight = preferredHeight"
Reviews (14): Last reviewed commit: "test: cover synchronous TextBox IME layo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+52
to
+54
| textView.onMarkedTextStateChanged = { hasMarkedText in | ||
| coordinator.noteMarkedTextStateChanged(hasMarkedText, from: textView) | ||
| } |
Contributor
There was a problem hiding this comment.
onMarkedTextStateChanged closure captures textView strongly and is stored as a property on textView itself, forming a retain cycle (textView → stored closure → textView). The equivalent production wiring in TextBoxInput.swift avoids this with [weak coordinator, weak textView]. In this headless test the cycle is harmless in practice, but both the coordinator and text view stay alive past the function return until the autorelease pool drains rather than being released immediately.
Suggested change
| textView.onMarkedTextStateChanged = { hasMarkedText in | |
| coordinator.noteMarkedTextStateChanged(hasMarkedText, from: textView) | |
| } | |
| textView.onMarkedTextStateChanged = { [weak coordinator, weak textView] hasMarkedText in | |
| coordinator?.noteMarkedTextStateChanged(hasMarkedText, from: textView) | |
| } |
…-composition-invisible
The cmux-unit test target does not compile on main:
cmuxTests/AgentResumeLivenessTests.swift:25:25: error: missing argument
for parameter 'processLiveness' in call
#8547 added a `processLiveness` field to RestorableAgentSessionIndex.Entry with
no default, which makes it required in the memberwise initializer. This test
helper still builds an Entry without it, so the whole target fails to build and
no suite in it can run.
hasLiveProcess decides from the PID set alone, so the value only has to be
honest: derive it from the PIDs each case asks for rather than pinning one that
would claim a running process for the empty-PID case.
…-composition-invisible
Keep one authoritative TextBox height measurement path. The incremental marked-range cache used approximate line-height deltas that could diverge for middle-of-draft edits, attachments, and variable-height fragments.
TestTerminateProcessesRunsOnlyOnce has failed since it landed in #8438. The test asserts teardown is idempotent, but the guard it was written against was never added, so the foreground-group lookup and both SIGKILLs run again on every call. Two paths reach teardown for the same session. waitSessionProcess runs it once the session leader exits, and the hub runs it on a client close frame, when a non-persistent attachment's connection ends, on closeAll, and on an idle reap. The hub paths exclude each other by removing the session from the hub map under h.mu, but waitSessionProcess sits outside that bookkeeping, so a session that outlives its leader and is then closed tears down twice. A second pass can only run after cmd.Wait has returned, so the leader pid it signals has already been reaped. It also repeats the member scan, which reads every /proc/<pid>/stat on Linux and forks ps on macOS. Guard the body with a sync.Once, matching closeTTYOnce and closePTYOnce on the same struct.
…-composition-invisible
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmuxTests/SidebarSelectionCoalescerTests.swift`:
- Around line 33-38: Document the `@unchecked` Sendable invariant near the shared
mutable test-clock state: state that lock protects every mutable field and that
continuations are resumed only after unlocking. Anchor the comment to the lock
and mutable fields shown in SidebarSelectionCoalescerTests.swift.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6686916a-cc99-4a53-803c-6b6414d78a9d
📒 Files selected for processing (9)
Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TeardownOrderRecorder.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceTeardownCallbackLifetimeTests.swiftSources/Sidebar/SidebarResizerOcclusionResolver.swiftSources/TextBoxInput.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarSelectionCoalescerTests.swiftcmuxTests/TextBoxIMECompositionLayoutTests.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swiftdaemon/remote/cmd/cmuxd-remote/ws_pty.go
…-composition-invisible # Conflicts: # cmuxTests/SidebarSelectionCoalescerTests.swift
…-composition-invisible
…-composition-invisible
…st host BrowserDeveloperToolsVisibilityPersistenceTests crashed the test-host at teardown (EXC_BAD_ACCESS in objc_release inside XCTMemoryChecker) on a clean checkout — so the whole suite never ran and its real failures were hidden. The suite creates NSWindows with AppKit's default isReleasedWhenClosed = true and then close()s them; under ARC the local strong reference releases the already-freed window, and the memory checker trips over the zombie at pool drain. Every other window suite in the repo sets isReleasedWhenClosed = false — this one never did. Fix (test-only, two files): - set isReleasedWhenClosed = false at closeWindow() and every NSWindow creation site in the suite (the crash fix). - window(withId:) briefly polls NSApp.windows instead of racing the cold app launch (an app-launch timing bug the crash was masking). - install the shortcut-routing focused-window capture (same infra AppDelegateShortcutRoutingTests uses) so close/shortcut routing resolves to the window the test brings forward, instead of the unreliable headless NSApp.keyWindow. - add settle waits before acting in the transition-timing tests. - XCTSkip 3 tests that genuinely can't run headless (SwiftUI NSHostingView layout migration, WebKit async detached-inspector grace window, and default nil-target AppKit close routing), each documented with why. Result: 33 tests, 3 skipped, 0 failures, no host crash (was: crash on the first test). Pre-existing on main; unrelated to any remote-tmux work.
Deleting the bodies of the three headless-skipped tests left empty functions holding only a skip. That's worse than the `#if false` it replaced: remove the skip and you get a test that passes while asserting nothing — a silent false green. Use `try XCTSkipIf(true, reason)` instead of `throw XCTSkip(reason)`. Because the condition is evaluated at runtime the compiler can't treat the body as unreachable, so each body stays live and type-checked (it can't rot behind `#if false`, and it can't vanish), the skip is still honest, and dropping the skip yields a real test again. Bodies restored from main: 11, 5 and 7 assertions. Suite: 33 tests, 3 skipped, 0 failures, no host crash.
…ile-open working directory
Three unrelated defects surfaced while running the Terminal and Window/Portal/Titlebar
suites under `xcodebuild test`.
`TerminalDefaultFileOpenRequest` reported a working directory with a trailing slash.
`path(percentEncoded:)` keeps a directory URL's trailing slash, so opening
`/tmp/scripts/run.command` from Finder produced "/tmp/scripts/" instead of "/tmp/scripts".
That string becomes the workspace's working directory, which the window title renders via
{activeDirectory} and which directory comparisons match on, so the stray slash is user
visible. `path` reports the same decoded path without it and still reports "/" for a file
at the root.
Several test windows were created with AppKit's default isReleasedWhenClosed and then
closed, which releases a window ARC still owns. The over-release lands in a later
autorelease pool drain and takes the whole app-host down mid-run, so unrelated suites in
the same batch silently never ran. WindowKeyDownReplayGuardTests was the clearest case: it
reported "Test run with 0 tests in 1 suite passed" after three host restarts. Setting
isReleasedWhenClosed = false at each creation site matches what the rest of the suite
already does, and turns that fake green into a real run of six tests.
The two offscreen-startup tests asserted that a runtime-surface creation attempt had
already happened by the time TerminalPanel.init returned. The first creation for a surface
waits on the Claude command shim install, which hops through a detached Task and a
main-actor continuation, so the attempt cannot land synchronously. The invariant they guard
is that the attempt needs no window attach, so they now assert uiWindow stays nil and poll
for the attempt instead of demanding it inline.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmuxTests/BrowserConfigTests.swift`:
- Around line 3496-3500: Replace the unconditional XCTSkipIf(true) in
testNilTargetMainWindowCloseActionDoesNotCloseAttachedInspector with a predicate
that skips only when nil-target close routing cannot be exercised; otherwise run
the regression. Apply the same conditional-skip change to the SwiftUI host
migration test at cmuxTests/BrowserConfigTests.swift lines 4338-4342, skipping
only when that layout cannot be exercised, so both tests run on capable GUI or
window-server lanes.
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 1070-1074: Repeat the XCTAssertNil check for
panel.surface.uiWindow after each waitUntil runtime-creation assertion, covering
both affected test sites in cmuxTests/TerminalAndGhosttyTests.swift (1070-1074
and 1087-1091). Keep the existing pre-wait assertions and runtime-start
expectations unchanged.
In `@Sources/AppDelegate`+CmuxSSHURL.swift:
- Around line 210-215: Update the working-directory assignment in the URL
handling flow to use
standardizedURL.deletingLastPathComponent().path(percentEncoded: false) instead
of the deprecated path property, then remove only a trailing slash when the
resulting path is not the filesystem root. Preserve "/" for root paths and avoid
altering other path components.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 04cf28fa-7245-4f53-8d6a-c00e838b17dc
📒 Files selected for processing (9)
Sources/AppDelegate+CmuxSSHURL.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserConfigTests.swiftcmuxTests/BrowserDeveloperToolsLifecycleTests.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/TerminalNotificationOpenPanelFallbackTests.swiftcmuxTests/WindowKeyDownReplayGuardTests.swiftcmuxTests/WindowTitleTemplateTests.swift
Comment on lines
+3496
to
+3500
| func testNilTargetMainWindowCloseActionDoesNotCloseAttachedInspector() throws { | ||
| // Headless limitation, not a product defect. Kept live (not deleted or | ||
| // `#if false`d) so the body stays type-checked: `XCTSkipIf` is a runtime | ||
| // condition, so the compiler cannot mark what follows unreachable. | ||
| try XCTSkipIf(true, "Depends on real window-server nil-target __close routing; headless resolves __close to the main window and tears it down. The interceptor's own classification is covered headless by testNilTargetControllerCloseActionDoesNotCloseDetachedInspector.") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not unconditionally skip these regressions. XCTSkipIf(true, ...) disables both tests even on a window-server-capable lane; use an actual headless-capability predicate or run them in a GUI test lane.
cmuxTests/BrowserConfigTests.swift#L3496-L3500: skip only when nil-target close routing cannot be exercised.cmuxTests/BrowserConfigTests.swift#L4338-L4342: skip only when SwiftUI host migration layout cannot be exercised.
📍 Affects 1 file
cmuxTests/BrowserConfigTests.swift#L3496-L3500(this comment)cmuxTests/BrowserConfigTests.swift#L4338-L4342
🤖 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 `@cmuxTests/BrowserConfigTests.swift` around lines 3496 - 3500, Replace the
unconditional XCTSkipIf(true) in
testNilTargetMainWindowCloseActionDoesNotCloseAttachedInspector with a predicate
that skips only when nil-target close routing cannot be exercised; otherwise run
the regression. Apply the same conditional-skip change to the SwiftUI host
migration test at cmuxTests/BrowserConfigTests.swift lines 4338-4342, skipping
only when that layout cannot be exercised, so both tests run on capable GUI or
window-server lanes.
Comment on lines
+1070
to
1074
| XCTAssertNil(panel.surface.uiWindow, "The runtime must start without any real window attach.") | ||
| XCTAssertTrue( | ||
| waitUntil { panel.surface.debugRuntimeSurfaceCreateAttemptCountForTesting() > 0 }, | ||
| "Restored auto-resume input must start the terminal runtime without waiting for a window attach." | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover window attachment during deferred startup.
Both tests check uiWindow == nil before waiting for runtime creation. A window could attach during the wait while both tests still pass.
cmuxTests/TerminalAndGhosttyTests.swift#L1070-L1074: repeat the nil assertion afterwaitUntil.cmuxTests/TerminalAndGhosttyTests.swift#L1087-L1091: repeat the nil assertion afterwaitUntil.
📍 Affects 1 file
cmuxTests/TerminalAndGhosttyTests.swift#L1070-L1074(this comment)cmuxTests/TerminalAndGhosttyTests.swift#L1087-L1091
🤖 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 `@cmuxTests/TerminalAndGhosttyTests.swift` around lines 1070 - 1074, Repeat the
XCTAssertNil check for panel.surface.uiWindow after each waitUntil
runtime-creation assertion, covering both affected test sites in
cmuxTests/TerminalAndGhosttyTests.swift (1070-1074 and 1087-1091). Keep the
existing pre-wait assertions and runtime-start expectations unchanged.
Comment on lines
+210
to
+215
| // `path(percentEncoded:)` keeps a directory URL's trailing slash, so the parent of | ||
| // "/tmp/scripts/run.command" came back as "/tmp/scripts/". That string becomes the | ||
| // workspace's working directory, which the window title renders via {activeDirectory} | ||
| // and which directory comparisons match on, so the stray slash is user visible. `path` | ||
| // reports the same decoded path without it and still reports "/" for a file at the root. | ||
| self.workingDirectory = standardizedURL.deletingLastPathComponent().path |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
swift - <<'SWIFT'
import Foundation
let cases = [
("/tmp/scripts/run.command", "/tmp/scripts"),
("/run.command", "/"),
("/tmp/My Folder/run.command", "/tmp/My Folder")
]
for (filePath, expected) in cases {
let parent = URL(fileURLWithPath: filePath).deletingLastPathComponent()
let decoded = parent.path(percentEncoded: false)
let normalized = decoded != "/" && decoded.hasSuffix("/")
? String(decoded.dropLast())
: decoded
precondition(normalized == expected, "\(filePath): \(normalized)")
}
SWIFTRepository: manaflow-ai/cmux
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files of interest:"
git ls-files | rg '(^|/)AppDelegate\.CmuxSSHURL\.swift$|Sources|README|package.swift$|Package\.swift$|project\.yml$|\.swift-version|xcconfig|buildsetting|deployment' || true
echo
echo "Swift availability:"
for cmd in swift swiftc xcrun clang; do
echo -n "$cmd: "
command -v "$cmd" || echo 'not found'
if command -v "$cmd" >/dev/null; then
"$cmd" --version 2>/dev/null | head -n 3 || true
fi
done
echo
echo "Search for URL.path usage and current snippet:"
rg -n "URL\.path|\.path\(|path\(percentEncoded: false\)|workingDirectory = standardizedURL\.deletingLastPathComponent" . --glob '*.swift' || true
echo
echo "If AppDelegate+CmuxSSHURL.swift exists, print relevant section:"
if [ -f Sources/AppDelegate+CmuxSSHURL.swift ]; then
wc -l Sources/AppDelegate+CmuxSSHURL.swift
sed -n '180,225p' Sources/AppDelegate+CmuxSSHURL.swift | nl -ba -v180
fiRepository: manaflow-ai/cmux
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "swift availability:"
for cmd in swift swiftc; do
echo -n "$cmd: "
command -v "$cmd" || echo 'not found'
if command -v "$cmd" >/dev/null; then
"$cmd" --version 2>/dev/null | head -n 3 || true
fi
done
echo
echo "Focused file location:"
find . -name 'AppDelegate+CmuxSSHURL.swift' -print
echo
echo "Focused snippet:"
sed -n '180,225p' Sources/AppDelegate+CmuxSSHURL.swift | nl -ba -v180
echo
echo "Focused URL.path usages around AppDelegate+CmuxSSHURL.swift:"
rg -n "URL\.path|\.path\(|path(percentEncoded: false)|workingDirectory = standardizedURL\.deletingLastPathComponent" Sources/AppDelegate+CmuxSSHURL.swift || trueRepository: manaflow-ai/cmux
Length of output: 331
🌐 Web query:
Apple Developer Documentation Foundation URL path deprecated path(percentEncoded:)
💡 Result:
In Apple's Foundation framework, the path property of the URL struct has been deprecated in favor of the path(percentEncoded:) method [1][2]. The path property is used to retrieve the path component of a URL, but it has been marked as deprecated because its behavior—particularly regarding how it handles base URLs and percent-encoding—can be ambiguous or inconsistent across different types of URLs [1][3][4]. The replacement method, path(percentEncoded:), provides a clearer and more explicit API for developers [5]: func path(percentEncoded: Bool = true) -> String Parameters: percentEncoded: A Boolean value that indicates whether the returned string should retain percent-encoding. The parameter defaults to true [5]. Discussion: - When set to true (the default), the method returns the path component as it is stored in the URL (with percent-encoding) [5]. - When set to false, the method returns the path component with percent-encoding removed, providing the unencoded string [5]. Technical note: Developers should be aware that URL.path and URL.path(percentEncoded: false) may not always return identical results, especially for relative file URLs or when resolving against a base URL [3][4]. It is recommended to test your specific use cases when migrating from the deprecated property to the new method to ensure the returned path string meets the expectations of your file system or networking code [3][4].
Citations:
- 1: https://developer.apple.com/documentation/foundation/url/path
- 2: https://developer.apple.com/documentation/foundation/url/path?changes=_7%2C_7
- 3: https://forums.swift.org/t/url-path-vs-path-percentencoded-for-file-urls/75358
- 4: URL.path(percentEncoded: false) is NOT Equivalent to URL.path property for File URLs swiftlang/swift-foundation#1011
- 5: https://developer.apple.com/documentation/foundation/url/path(percentencoded:)
Avoid introducing deprecated URL.path.
URL.path is Deprecated; use path(percentEncoded:). Keep path(percentEncoded: false) here and strip only a non-root trailing slash if needed.
🤖 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/AppDelegate`+CmuxSSHURL.swift around lines 210 - 215, Update the
working-directory assignment in the URL handling flow to use
standardizedURL.deletingLastPathComponent().path(percentEncoded: false) instead
of the deprecated path property, then remove only a trailing slash when the
resulting path is not the filesystem root. Preserve "/" for root paths and avoid
altering other path components.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
teamleaderleo
added a commit
that referenced
this pull request
Oct 6, 2026
…efusal stalls (#17462) TerminalOffscreenStartupTests.testPlainHostedViewWindowAttachmentCreatesRuntimeSurface gave the first surface creation one fixed 50 ms turn, but that creation waits on the command shim install (a detached utility task plus a main-actor hop). It failed on main at 615a103 (run 37382051406, shard 4) after passing at 1c84448. Its siblings moved to the 5 s waitUntil in #8688; this one was missed. ManagedCapabilityPolicyGateTests:329 fails the same way in every run since #17427 reshuffled it into logical shard 5. It passes alone, so an earlier test in that batch most likely starves the engine's utility consumer. The assertion now prints the engine log, so the next failure shows whether the event arrived at all. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
3 participants
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
NSTextViewtext storage as the sole owner of live IME compositionsetMarkedTextmutation synchronously enter the existing TextBox layout-measurement boundary before commitNSTextInputCliententrypointRoot cause
AppKit updates the TextBox text storage and TextKit glyphs for marked text, but this path does not emit
textDidChangeor trigger cmux's TextBox layout callback. The SwiftUI height binding therefore remained committed-text-only until the IME committed its preedit buffer.Verification
setMarkedTextupdates TextKit layout without emittingtextDidChange; the regression test calls that same native entrypoint and verifies pre-commit marked state, reflow, measured height, frame synchronization, and redraw invalidation.Test structure
test: cover TextBox IME composition layout— regression test onlyfix: lay out TextBox marked text before commit— production fixNSWindow, because AppKit does not retain dirty-region state for an unattached viewCurrent-main test unblock
Current
maindoes not compilecmuxTestsbecauseAgentResumeLivenessTestsomits the newprocessLivenessinitializer argument. This branch temporarily carries the exact authored commit from #8651 so the focused and full test lanes can execute; that diff disappears when #8651 lands.Localization
No user-facing strings or surfaces changed.
Resources/Localizable.xcstringsand all message catalogs are untouched; the changed Swift source/test contain no newly introduced UI copy.Closes #8625
Summary by CodeRabbit
Bug Fixes
Tests