Fix TextBox unchanged binding writes#5345
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors TextBoxInputView.Coordinator to detect changes separately for text, attachments, and pending-upload placeholder state and only update the corresponding SwiftUI bindings when that component changed. It adds attachment equality helpers and a regression test that ensures unchanged content does not trigger binding writes; the test is registered in the Xcode project. ChangesConditional TextBox binding updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e200cae796
ℹ️ 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".
| XCTAssertEqual(pendingWriteCount, 0) | ||
| } | ||
|
|
||
| func testTextBoxContentSyncSkipsUnchangedSwiftUIBindings() { |
There was a problem hiding this comment.
This commit adds testTextBoxContentSyncSkipsUnchangedSwiftUIBindings in the same commit as the production fix, but the repo's AGENTS.md regression-test policy requires bug-fix regressions to be added as a failing test-only commit first and the fix second so CI proves the test catches the bug. Because this test is specifically covering the TextBox binding-write regression fixed here, keeping it combined removes that red/green proof; please split the test addition into its own preceding commit.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR makes the
Confidence Score: 5/5The change is narrowly scoped to the AppKit to SwiftUI sync path and is conservative: it only skips writes when values provably have not changed, so the worst-case outcome of any undetected equality mismatch is an occasional missed no-op reduction rather than dropped real updates. All TextBoxAttachment fields are covered by attachmentsDiffer; thumbnail identity check is safe because the field is an immutable let on a value-type struct. The regression test correctly exercises the no-op path in a dedicated file. Both concerns from previous review threads are resolved in this iteration. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[textDidChange or markedText sync] --> B[publishTextViewContent]
B --> C{text changed?}
C -- yes --> D[write parent.text]
C -- no --> E[skip text write]
B --> F{attachmentsDiffer?}
F -- yes --> G[write parent.attachments]
F -- no --> H[skip attachments write]
B --> I{pendingUpload changed?}
I -- yes --> J[write parent.hasPendingAttachmentUpload]
I -- no --> K[skip pendingUpload write]
D --> L{any field changed?}
E --> L
G --> L
H --> L
J --> L
K --> L
L -- yes --> M[onContentChanged]
L -- no --> N[no-op, SwiftUI binding untouched]
Reviews (4): Last reviewed commit: "fix: skip unchanged TextBox binding writ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| parent.attachments = nextAttachments | ||
| parent.hasPendingAttachmentUpload = nextHasPendingAttachmentUpload | ||
| let didChangeText = parent.text != nextText | ||
| let didChangeAttachments = parent.attachments.map(\.id) != nextAttachments.map(\.id) |
There was a problem hiding this comment.
onContentChanged. The old code always wrote nextAttachments unconditionally, ensuring that any change to attachment objects (including non-id fields such as upload state, thumbnail, or submission path) was pushed to the SwiftUI binding. With this guard, if two successive AppKit syncs produce arrays whose .id sequences are identical but whose attachment objects differ in other properties, the binding update is silently skipped and SwiftUI retains stale attachment data.
| let didChangeAttachments = parent.attachments.map(\.id) != nextAttachments.map(\.id) | |
| let didChangeAttachments = parent.attachments != nextAttachments |
| func testTextBoxContentSyncSkipsUnchangedSwiftUIBindings() { | ||
| var text = "same" | ||
| var attachments: [TextBoxAttachment] = [] | ||
| var height: CGFloat = TextBoxLayout.minimumTextHeight | ||
| var hasPendingAttachmentUpload = false | ||
| var textWriteCount = 0 | ||
| var attachmentWriteCount = 0 | ||
| var pendingWriteCount = 0 | ||
| var contentChangeCount = 0 | ||
|
|
||
| let inputView = TextBoxInputView( | ||
| text: Binding( | ||
| get: { text }, | ||
| set: { newValue in | ||
| textWriteCount += 1 | ||
| text = newValue | ||
| } | ||
| ), | ||
| attachments: Binding( | ||
| get: { attachments }, | ||
| set: { newValue in | ||
| attachmentWriteCount += 1 | ||
| attachments = newValue | ||
| } | ||
| ), | ||
| textViewHeight: Binding(get: { height }, set: { height = $0 }), | ||
| hasPendingAttachmentUpload: Binding( | ||
| get: { hasPendingAttachmentUpload }, | ||
| set: { newValue in | ||
| pendingWriteCount += 1 | ||
| hasPendingAttachmentUpload = newValue | ||
| } | ||
| ), | ||
| font: NSFont.systemFont(ofSize: 14), | ||
| backgroundColor: .textBackgroundColor, | ||
| foregroundColor: .labelColor, | ||
| terminalTitle: "codex", | ||
| completionRootDirectory: nil, | ||
| onSubmit: {}, | ||
| onEscape: {}, | ||
| onFocusTextBox: {}, | ||
| onToggleFocus: {}, | ||
| onForwardText: { _, _ in }, | ||
| onForwardKey: { _ in }, | ||
| onForwardControl: { _ in }, | ||
| onPaste: { _, _ in false }, | ||
| onInsertFileURLs: { _, _ in false }, | ||
| onChooseFiles: {}, | ||
| onContentChanged: { contentChangeCount += 1 }, | ||
| onTextViewCreated: { _ in }, | ||
| onTextViewMovedToWindow: { _ in }, | ||
| onTextViewDismantled: { _ in } | ||
| ) | ||
| let coordinator = TextBoxInputView.Coordinator(parent: inputView) | ||
| let textView = TextBoxInputTextView(frame: NSRect(x: 0, y: 0, width: 320, height: 30)) | ||
| textView.font = NSFont.systemFont(ofSize: 14) | ||
| textView.textColor = .labelColor | ||
| textView.string = "same" | ||
|
|
||
| coordinator.textDidChange(Notification(name: NSText.didChangeNotification, object: textView)) | ||
|
|
||
| XCTAssertEqual(textWriteCount, 0) | ||
| XCTAssertEqual(attachmentWriteCount, 0) | ||
| XCTAssertEqual(pendingWriteCount, 0) | ||
| XCTAssertEqual(contentChangeCount, 0) | ||
| } |
There was a problem hiding this comment.
testTextBoxContentSyncSkipsUnchangedSwiftUIBindings tests TextBox binding write-skipping, which has no relation to shortcut routing. AppDelegateShortcutRoutingTests.swift already exceeds 10 000 lines. Appending another unrelated test further worsens the file-sprawl that the cmux-swift-file-package-boundaries rule targets — the existing test class is already well past the 400-line single-responsibility and 800-line hard-stop limits. A TextBoxInputTests class (or an extension to an existing TextBox-focused test file) would scope the coverage correctly.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/AppDelegateShortcutRoutingTests.swift">
<violation number="1" location="cmuxTests/AppDelegateShortcutRoutingTests.swift:10101">
P2: This PR adds another test to `AppDelegateShortcutRoutingTests`, but this suite is marked by team guidance as needing class-level quarantine on GitHub macOS CI due to hangs. Please quarantine/skip the full XCTestCase on GitHub CI before adding more coverage here.
(Based on your team's feedback about quarantining the entire AppDelegateShortcutRoutingTests suite on GitHub macOS CI.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| XCTAssertEqual(pendingWriteCount, 0) | ||
| } | ||
|
|
||
| func testTextBoxContentSyncSkipsUnchangedSwiftUIBindings() { |
There was a problem hiding this comment.
P2: This PR adds another test to AppDelegateShortcutRoutingTests, but this suite is marked by team guidance as needing class-level quarantine on GitHub macOS CI due to hangs. Please quarantine/skip the full XCTestCase on GitHub CI before adding more coverage here.
(Based on your team's feedback about quarantining the entire AppDelegateShortcutRoutingTests suite on GitHub macOS CI.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/AppDelegateShortcutRoutingTests.swift, line 10101:
<comment>This PR adds another test to `AppDelegateShortcutRoutingTests`, but this suite is marked by team guidance as needing class-level quarantine on GitHub macOS CI due to hangs. Please quarantine/skip the full XCTestCase on GitHub CI before adding more coverage here.
(Based on your team's feedback about quarantining the entire AppDelegateShortcutRoutingTests suite on GitHub macOS CI.) </comment>
<file context>
@@ -10098,6 +10098,73 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
XCTAssertEqual(pendingWriteCount, 0)
}
+ func testTextBoxContentSyncSkipsUnchangedSwiftUIBindings() {
+ var text = "same"
+ var attachments: [TextBoxAttachment] = []
</file context>
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/TextBoxContentSyncTests.swift`:
- Line 13: The test commit for TextBox unchanged-binding behavior
(cmuxTests/TextBoxContentSyncTests.swift containing the test method
contentSyncSkipsUnchangedSwiftUIBindings) was added after the production fix
commit, so recreate the intended commit ordering: introduce a failing-test-only
commit that adds the test (with the test method
contentSyncSkipsUnchangedSwiftUIBindings) first, verify CI fails, then apply the
production fix commit (the change that skips unchanged TextBox binding writes)
in a subsequent commit to make CI pass; alternatively, reorder the existing
commits so the test-adding commit precedes the fix commit to restore the CI
red→green workflow.
🪄 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
Run ID: c6143ac2-b30d-4e15-91e5-328e5d32d1d9
📒 Files selected for processing (2)
cmux.xcodeproj/project.pbxprojcmuxTests/TextBoxContentSyncTests.swift
|
|
||
| @MainActor | ||
| struct TextBoxContentSyncTests { | ||
| @Test func contentSyncSkipsUnchangedSwiftUIBindings() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check commit structure for this PR
echo "=== PR commits ==="
gh pr view 5345 --json commits --jq '.commits[] | "\(.oid[0:7]) \(.messageHeadline)"'
echo -e "\n=== Changed files per commit ==="
gh pr view 5345 --json commits --jq '.commits[].oid' | while read commit; do
echo "Commit: $commit"
git show --name-only --format="" "$commit" 2>/dev/null | grep -E '\.(swift|pbxproj)$' | head -20
echo ""
doneRepository: manaflow-ai/cmux
Length of output: 1295
Fix regression commit ordering for the TextBox unchanged-binding write bug.
cmuxTests/TextBoxContentSyncTests.swift (added in commit a64519f3) is introduced after the production change (fix: skip unchanged TextBox binding writes, commit e200cae7), so the required failing-test-only commit step (CI red) → fix commit step (CI green) isn’t present.
🤖 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/TextBoxContentSyncTests.swift` at line 13, The test commit for
TextBox unchanged-binding behavior (cmuxTests/TextBoxContentSyncTests.swift
containing the test method contentSyncSkipsUnchangedSwiftUIBindings) was added
after the production fix commit, so recreate the intended commit ordering:
introduce a failing-test-only commit that adds the test (with the test method
contentSyncSkipsUnchangedSwiftUIBindings) first, verify CI fails, then apply the
production fix commit (the change that skips unchanged TextBox binding writes)
in a subsequent commit to make CI pass; alternatively, reorder the existing
commits so the test-adding commit precedes the fix commit to restore the CI
red→green workflow.
lawrencecchen
force-pushed
the
task-sentry-terminalpanel-access-conflict
branch
from
a64519f to
2e58388
Compare
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/TextBoxContentSyncTests.swift`:
- Line 38: Add a write counter for the textViewHeight Binding in
TextBoxContentSyncTests to mirror the sibling test: declare a local
heightWriteCount variable, increment it inside the Binding's set closure used
for textViewHeight (alongside assigning height), and add an assertion that
heightWriteCount == 0 at the end of the test; alternatively, if writes are
intentionally ignored because height is layout-derived, add a brief comment near
the textViewHeight Binding explaining that decision instead of adding the
counter.
🪄 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
Run ID: e9236643-0aef-4bb9-86df-4d72bfaa5c07
📒 Files selected for processing (3)
Sources/TextBoxInput.swiftcmux.xcodeproj/project.pbxprojcmuxTests/TextBoxContentSyncTests.swift
| attachments = newValue | ||
| } | ||
| ), | ||
| textViewHeight: Binding(get: { height }, set: { height = $0 }), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider tracking textViewHeight binding writes for test completeness.
The sibling test (AppDelegateShortcutRoutingTests.testTextBoxRepresentableDismantleDoesNotWriteSwiftUIBindings) explicitly tracks heightWriteCount and asserts it remains 0. For consistency and completeness, consider adding a counter here:
var heightWriteCount = 0
// ...
textViewHeight: Binding(
get: { height },
set: { newValue in
heightWriteCount += 1
height = newValue
}
),
// ...
`#expect`(heightWriteCount == 0)If height is intentionally excluded because it's layout-derived rather than content-derived, a brief comment explaining that would clarify the test's scope.
🤖 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/TextBoxContentSyncTests.swift` at line 38, Add a write counter for
the textViewHeight Binding in TextBoxContentSyncTests to mirror the sibling
test: declare a local heightWriteCount variable, increment it inside the
Binding's set closure used for textViewHeight (alongside assigning height), and
add an assertion that heightWriteCount == 0 at the end of the test;
alternatively, if writes are intentionally ignored because height is
layout-derived, add a brief comment near the textViewHeight Binding explaining
that decision instead of adding the counter.
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
1 participant
Summary
Root cause
Sentry issue CMUXTERM-MACOS-MRP points at
TerminalPanel.textBoxContent.setterin productioncom.cmuxterm.app@0.64.12+92. The latest event maps toTextBoxInputView.dismantleNSView->TerminalPanel.preserveTextBoxContentForUnmount; inv0.64.12, that teardown path wrotetextBoxContent, which published synchronously while SwiftUI was destroying the representable. Current main already avoids the teardown write. This PR further reduces the same access-conflict risk by skipping unchanged TextBox binding writes from normal AppKit content sync.Testing
./scripts/lint-pbxproj-test-wiring.shpassed.cmux-unitfocused run attempted with-only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testTextBoxContentSyncSkipsUnchangedSwiftUIBindings; runner failed before executing tests because/tmpfilled during app target compile (errno=28,Macintosh HD is out of space).Issues
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized TextBox coordinator sync logic with a focused unit test; no auth, data, or broad architectural changes.
Overview
TextBox AppKit→SwiftUI sync now updates
text,attachments, andhasPendingAttachmentUploadonly when each field actually changes, and callsonContentChangedonly when something did change. Attachment equality no longer relies on ID lists alone: a newattachmentsDifferhelper compares display/submission metadata, URLs, cleanup flags, and thumbnail identity (===forNSImage).A
TextBoxContentSyncTestsregression test asserts that a no-optextDidChange(same plain text, no attachments) performs zero binding writes and does not fireonContentChanged, guarding against redundant SwiftUI updates during representable teardown/sync (related to Sentry CMUXTERM-MACOS-MRP).Reviewed by Cursor Bugbot for commit 2e58388. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Skip unchanged SwiftUI binding writes in TextBoxInputView during AppKit→SwiftUI sync and tighten attachment change detection to avoid missed updates. Reduces redundant updates and mitigates Sentry CMUXTERM-MACOS-MRP access-conflict risk.
text,attachments, andhasPendingAttachmentUploadonly when values differ; callonContentChangedonly on real changes.TextBoxContentSyncTests.contentSyncSkipsUnchangedSwiftUIBindingsto verify zero writes and noonContentChangedon same-value sync.Written for commit 2e58388. Summary will update on new commits.
Summary by CodeRabbit
Performance
Tests