Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 40 additions & 6 deletions Sources/TextBoxInput.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3420,17 +3420,51 @@ struct TextBoxInputView: NSViewRepresentable {
let nextText = textView.plainText()
let nextAttachments = textView.inlineAttachments()
let nextHasPendingAttachmentUpload = textView.hasPendingAttachmentUploadPlaceholder()
let contentChanged = parent.text != nextText
|| parent.attachments.map(\.id) != nextAttachments.map(\.id)
|| parent.hasPendingAttachmentUpload != nextHasPendingAttachmentUpload
parent.text = nextText
parent.attachments = nextAttachments
parent.hasPendingAttachmentUpload = nextHasPendingAttachmentUpload
let didChangeText = parent.text != nextText
let didChangeAttachments = Self.attachmentsDiffer(parent.attachments, nextAttachments)
let didChangePendingUpload = parent.hasPendingAttachmentUpload != nextHasPendingAttachmentUpload
let contentChanged = didChangeText || didChangeAttachments || didChangePendingUpload
if didChangeText {
parent.text = nextText
}
if didChangeAttachments {
parent.attachments = nextAttachments
}
if didChangePendingUpload {
parent.hasPendingAttachmentUpload = nextHasPendingAttachmentUpload
}
if contentChanged {
parent.onContentChanged()
}
}

private static func attachmentsDiffer(
_ currentAttachments: [TextBoxAttachment],
_ nextAttachments: [TextBoxAttachment]
) -> Bool {
guard currentAttachments.count == nextAttachments.count else { return true }
return zip(currentAttachments, nextAttachments).contains { current, next in
current.id != next.id
|| current.displayName != next.displayName
|| current.submissionText != next.submissionText
|| current.submissionPath != next.submissionPath
|| current.localURL != next.localURL
|| current.cleanupLocalURLWhenDisposed != next.cleanupLocalURLWhenDisposed
|| !sameThumbnail(current.thumbnail, next.thumbnail)
}
}

private static func sameThumbnail(_ currentThumbnail: NSImage?, _ nextThumbnail: NSImage?) -> Bool {
switch (currentThumbnail, nextThumbnail) {
case (nil, nil):
true
case let (current?, next?):
current === next
default:
false
}
}

func recalculateHeight(_ textView: NSTextView) {
guard let layoutManager = textView.layoutManager,
let textContainer = textView.textContainer else { return }
Expand Down
4 changes: 4 additions & 0 deletions cmux.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@
C13519000000000000000001 /* TerminalStartupEnvironment.swift in Sources */ = {isa = PBXBuildFile; fileRef = C13519000000000000000002 /* TerminalStartupEnvironment.swift */; };
A5001532 /* TerminalWindowPortal.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5001531 /* TerminalWindowPortal.swift */; };
D0B10006A1B2C3D4E5F60001 /* TerminalWindowPortalDebug.swift in Sources */ = {isa = PBXBuildFile; fileRef = D0B10007A1B2C3D4E5F60001 /* TerminalWindowPortalDebug.swift */; };
C5345001A1B2C3D4E5F60719 /* TextBoxContentSyncTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C5345002A1B2C3D4E5F60719 /* TextBoxContentSyncTests.swift */; };
C0DE7B100000000000000001 /* TextBoxInput.swift in Sources */ = {isa = PBXBuildFile; fileRef = C0DE7B100000000000000002 /* TextBoxInput.swift */; };
C0DE7B310000000000000001 /* TextBoxMentionCachedIndex.swift in Sources */ = {isa = PBXBuildFile; fileRef = C0DE7B310000000000000002 /* TextBoxMentionCachedIndex.swift */; };
C0DE7B250000000000000001 /* TextBoxMentionCandidate.swift in Sources */ = {isa = PBXBuildFile; fileRef = C0DE7B250000000000000002 /* TextBoxMentionCandidate.swift */; };
Expand Down Expand Up @@ -1169,6 +1170,7 @@
C13519000000000000000002 /* TerminalStartupEnvironment.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TerminalStartupEnvironment.swift; sourceTree = "<group>"; };
A5001531 /* TerminalWindowPortal.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TerminalWindowPortal.swift; sourceTree = "<group>"; };
D0B10007A1B2C3D4E5F60001 /* TerminalWindowPortalDebug.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TerminalWindowPortalDebug.swift; sourceTree = "<group>"; };
C5345002A1B2C3D4E5F60719 /* TextBoxContentSyncTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TextBoxContentSyncTests.swift; sourceTree = "<group>"; };
C0DE7B100000000000000002 /* TextBoxInput.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TextBoxInput.swift; sourceTree = "<group>"; };
C0DE7B310000000000000002 /* TextBoxMentionCachedIndex.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TextBoxMentionCachedIndex.swift; sourceTree = "<group>"; };
C0DE7B250000000000000002 /* TextBoxMentionCandidate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TextBoxMentionCandidate.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1849,6 +1851,7 @@
F5310001A1B2C3D4E5F60718 /* RovoDevHookConfigTests.swift */,
FA100001A1B2C3D4E5F60718 /* BrowserImportMappingTests.swift */,
F6000001A1B2C3D4E5F60718 /* AppDelegateShortcutRoutingTests.swift */,
C5345002A1B2C3D4E5F60719 /* TextBoxContentSyncTests.swift */,
C0DE7B300000000000000002 /* TextBoxMentionCompletionTests.swift */,
C1713005C1713005C1713005 /* CommandPaletteShortcutCustomizationTests.swift */,
17FCD4CC61D54A2F8F2F463D /* AppDelegateBareSpaceShortcutRoutingTests.swift */,
Expand Down Expand Up @@ -2881,6 +2884,7 @@
A5A5A503A1B2C3D4E5F60718 /* TerminalNotificationQueueTests.swift in Sources */,
A5E380700000000000000001 /* TerminalNotificationSocketActionTests.swift in Sources */,
C0DE53360000000000000001 /* TerminalSearchOverlayMouseReleaseTests.swift in Sources */,
C5345001A1B2C3D4E5F60719 /* TextBoxContentSyncTests.swift in Sources */,
C0DE7B300000000000000001 /* TextBoxMentionCompletionTests.swift in Sources */,
F50030040000000000000001 /* TitlebarInteractiveControlTests.swift in Sources */,
D3284001A1B2C3D4E5F60718 /* TraditionalChineseIMENumpadRegressionTests.swift in Sources */,
Expand Down
79 changes: 79 additions & 0 deletions cmuxTests/TextBoxContentSyncTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import AppKit
import SwiftUI
import Testing

#if canImport(cmux_DEV)
@testable import cmux_DEV
#elseif canImport(cmux)
@testable import cmux
#endif

@MainActor
struct TextBoxContentSyncTests {
@Test func contentSyncSkipsUnchangedSwiftUIBindings() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 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 ""
done

Repository: 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.

var text = "same"
var attachments: [TextBoxAttachment] = []
var height: CGFloat = 24
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 }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 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.

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))

#expect(textWriteCount == 0)
#expect(attachmentWriteCount == 0)
#expect(pendingWriteCount == 0)
#expect(contentChangeCount == 0)
}
}
Loading