Repository navigation
fix: AppKit workspace double-click rename no longer commits instantly - #9496
XueyanZhang wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughInline rename now lays out the field before focus, removes redundant selection, manages shared field-editor styling, and adds regression coverage for synchronous commit behavior. ChangesInline rename behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
![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
🤖 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/SidebarAppKitRowCellTests.swift`:
- Around line 873-906: The beginInlineRename regression test should also verify
that editing actually starts, not only that no commit occurred. After calling
cell.beginInlineRename(), assert that window.firstResponder is cell.renameField,
using the existing cell.renameField symbol.
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift`:
- Around line 1393-1402: Update controlTextDidBeginEditing(_:) to capture the
shared NSTextView editor’s prior drawsBackground and backgroundColor values
before changing them, then restore those values in the early-return path of
controlTextDidEndEditing(_:). Keep the existing editing behavior while ensuring
later controls do not inherit the transparent editor styling.
- Around line 1014-1017: Update the inline rename focus handling near the
sidebar row rename flow so the `tookFocus` binding exists only under `#if
DEBUG`; in non-debug builds, discard the
`window?.makeFirstResponder(renameField)` result while preserving the existing
debug log behavior.
🪄 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: 998397d7-3d17-40d8-a61b-d23234573ad8
📒 Files selected for processing (2)
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftcmuxTests/SidebarAppKitRowCellTests.swift
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/SidebarAppKitRowCellTests.swift (1)
873-910: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the regression test verify the commit callback.
The test only checks that
committedTitlesis empty before user input. IfmakeActionsorconfiguredCellstops forwardingcommitRename, the list remains empty and the test still passes even when editing commits. Add a positive Return/commit assertion that expects exactly one committed title. Keep the existing pre-commit assertion.🤖 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/SidebarAppKitRowCellTests.swift` around lines 873 - 910, Update beginInlineRenameLeavesFieldEditingWithoutCommittingPrefilledTitle to simulate the user completing the rename through the field editor’s Return/commit action, then assert committedTitles contains exactly one expected title. Preserve the existing assertion that committedTitles is empty immediately after cell.beginInlineRename, and use the existing configuredCell commitRename callback to verify forwarding.
🤖 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/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift`:
- Around line 1396-1400: Update controlTextDidEndEditing(_:) to extract the
ending NSFieldEditor from obj.userInfo and pass it to
restoreFieldEditorStyling(_:) before clearing saved styling, rather than
resolving window?.firstResponder. Also clear the saved field-editor styling in
the onCancel path of endInlineRename(commit:), since cancellation can return
without invoking the end-editing notification.
---
Outside diff comments:
In `@cmuxTests/SidebarAppKitRowCellTests.swift`:
- Around line 873-910: Update
beginInlineRenameLeavesFieldEditingWithoutCommittingPrefilledTitle to simulate
the user completing the rename through the field editor’s Return/commit action,
then assert committedTitles contains exactly one expected title. Preserve the
existing assertion that committedTitles is empty immediately after
cell.beginInlineRename, and use the existing configuredCell commitRename
callback to verify forwarding.
🪄 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: a8b4ebba-0dfa-4033-b1db-32a006a53cfc
📒 Files selected for processing (2)
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftcmuxTests/SidebarAppKitRowCellTests.swift
| // Restore the shared field editor's prior styling before committing, | ||
| // so the transparent background set in controlTextDidBeginEditing | ||
| // does not leak to the next control edited in this window. | ||
| restoreFieldEditorStyling() | ||
| guard !isHidden else { return } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'func endInlineRename|abortEditing|endEditing\(for:' \
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift || trueRepository: manaflow-ai/cmux
Length of output: 1405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map and inspect the relevant sections of the row cell view.
wc -l Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift
sed -n '960,1045p' Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift
sed -n '1280,1435p' Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift
# Locate the styling fields and delegate methods.
rg -n 'savedEditorDrawsBackground|savedEditorBackgroundColor|controlTextDidEndEditing|restoreFieldEditorStyling|controlTextDidBegin|textDidChange' Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftRepository: manaflow-ai/cmux
Length of output: 11430
🌐 Web query:
Apple Docs NSControlTextEditingDelegate controlTextDidEndEditing notification NSFieldEditor userInfo
💡 Result:
The controlTextDidEndEditing(:) method is part of the NSControlTextEditingDelegate protocol in AppKit [1]. When a control, such as an NSTextField, finishes an editing session, it posts an NSControl.textDidEndEditingNotification [2]. When a delegate implements the controlTextDidEndEditing(:) method, it receives this Notification object as its argument [2][3]. The Notification object provides access to the field editor through its userInfo dictionary [2][3]. To retrieve the field editor, you use the key "NSFieldEditor" [2][3]. For example, in Swift, you can access it as follows: if let fieldEditor = notification.userInfo?["NSFieldEditor"] as? NSTextView { // Access the field editor here } It is important to note that the notification's object is the NSControl that posted the notification [2][4]. Additionally, if you programmatically call abortEditing to discard pending changes, the control will not invoke controlTextDidEndEditing(_:) [3].
Citations:
- 1: https://developer.apple.com/documentation/AppKit/NSControlTextEditingDelegate
- 2: https://developer.apple.com/documentation/objectivec/nsobject-swift.class/controltextdidendediting:
- 3: https://sosumi.ai/documentation/appkit/nscontroltexteditingdelegate/controltextdidendediting(_:)
- 4: https://apple-docs.everest.mt/docs/appkit/nscontrol/textdidendeditingnotification/
Restore styling from the end-editing notification.
restoreFieldEditorStyling() currently reads window?.firstResponder, which may not be the field editor that ended editing. Use the NSFieldEditor value from obj.userInfo at controlTextDidEndEditing(_:) and pass that to the restore helper before clearing the saved styles.
endInlineRename(commit: false) returns from onCancel without calling the end-editing path, so clear the saved styling there too. abortEditing() can be called instead of controlTextDidEndEditing(_:), but this cancel handler just calls endInlineRename.
🤖 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/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift` around
lines 1396 - 1400, Update controlTextDidEndEditing(_:) to extract the ending
NSFieldEditor from obj.userInfo and pass it to restoreFieldEditorStyling(_:)
before clearing saved styling, rather than resolving window?.firstResponder.
Also clear the saved field-editor styling in the onCancel path of
endInlineRename(commit:), since cancellation can return without invoking the
end-editing notification.
Source: Path instructions
…ting instantly Adds beginInlineRenameLeavesFieldEditingWithoutCommittingPrefilledTitle to SidebarAppKitRowCellTests. Hosts the workspace row cell in a real NSWindow (so the shared field editor attaches), calls beginInlineRename(), and asserts commitRename is NOT invoked. On the buggy path the redundant selectText(nil) after makeFirstResponder re-enters the field editor and synchronously fires controlTextDidEndEditing, committing the pre-filled title, so this test fails.
SidebarWorkspaceRowTableCellView.beginInlineRename() called makeFirstResponder then selectText(nil); the extra selectText(nil) re-entered the field-editor machinery and synchronously fired controlTextDidEndEditing, which committed the untouched title before the user could type (field flashed and vanished). - Remove the redundant selectText(nil) (an editable NSTextField already enters editing with all text selected when made first responder). Also lay out before attaching the field editor so its frame is stable. - Make the shared field editor's background transparent in controlTextDidBeginEditing so the white selected-row text stays visible. Closes manaflow-ai#9495.
…scope debug log - Test: also assert the field editor attached (renameField.currentEditor() != nil), so the regression covers 'editing actually started', not only 'no spurious commit'. - controlTextDidBeginEditing now saves the shared field editor's prior drawsBackground/backgroundColor and restores them in controlTextDidEndEditing, so the transparent styling does not leak to later controls in the window. - Scope the tookFocus binding and its debug log to #if DEBUG; in release, discard the makeFirstResponder result (avoids an unused-variable warning).
9032625 to
b8fd6cd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto latest |
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 `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift`:
- Around line 1020-1021: Replace the cmuxDebugLog call in the inline-rename flow
of SidebarWorkspaceRowCellView with the canonical dlog logger, keeping it inside
the existing `#if` DEBUG block and preserving the current diagnostic message and
values.
🪄 Autofix
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: 9864f284-85ef-418e-bda2-c79f1c1396b3
📒 Files selected for processing (2)
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftcmuxTests/SidebarAppKitRowCellTests.swift
| let tookFocus = window?.makeFirstResponder(renameField) ?? false | ||
| cmuxDebugLog("sidebar.row.beginInlineRename tookFocus=\(tookFocus ? 1 : 0) window=\(window == nil ? 0 : 1)") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use dlog for this Sidebar diagnostic.
Line 1021 adds cmuxDebugLog in Sources/Sidebar. Replace it with dlog inside the existing #if DEBUG block.
Based on learnings: Sidebar Swift files that can access Bonsplit must use its canonical dlog logger.
🤖 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/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift` around
lines 1020 - 1021, Replace the cmuxDebugLog call in the inline-rename flow of
SidebarWorkspaceRowCellView with the canonical dlog logger, keeping it inside
the existing `#if` DEBUG block and preserving the current diagnostic message and
values.
Source: Learnings
teamleaderleo
added
area: sidebar
|
This is ready for a maintainer land once the CLA is recorded. Please comment: |
|
All contributors have signed the CLA ✍️ ✅ |
|
Main now ships the inline rename session and field-editor lifecycle fix in #14986, which supersedes this implementation. Closing this PR :) |
2 participants
Summary
Fixes the workspace double-click inline rename flashing and committing instantly on the AppKit sidebar (regression of #8270 / #8390). Double-clicking a workspace name created the field and it took first responder, but the field editor was torn down ~1 ms later and the untouched title was committed before the user could type.
Closes #9495.
Root cause
SidebarWorkspaceRowTableCellView.beginInlineRename()calledwindow.makeFirstResponder(renameField)(attaches the field editor, begins editing) and thenrenameField.selectText(nil). The extraselectText(nil)re-enters AppKit's field-editor machinery and synchronously firescontrolTextDidEndEditing, whichSidebarRowInlineRenameFieldhonors by committingstringValue— the pre-filled, unchanged title.How the root cause was isolated
A
firstResponderprobe incontrolTextDidEndEditingshowed the responder at blur time was still the shared field editor (NSTextView), not the terminal (GhosttyNSView) — so the editing session was being torn down and restarted synchronously insidebeginInlineRename, not stolen by async focus reconciliation. The teardown aligned to the single call that followedmakeFirstResponderin the same stack frame:selectText(nil). Removing it (an editableNSTextFieldalready selects all on becoming first responder) eliminated the spurious commit.Debug timeline before / after:
Changes
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftbeginInlineRename(): removed the redundantselectText(nil)aftermakeFirstResponder(the fix). Also lay out before attaching the field editor so the field's frame is stable.SidebarRowInlineRenameField.controlTextDidBeginEditing: make the shared field editor's background transparent so the white selected-row text is visible (was white-on-white once editing stuck).cmuxTests/SidebarAppKitRowCellTests.swift: regression testbeginInlineRenameLeavesFieldEditingWithoutCommittingPrefilledTitle— hosts the cell in a realNSWindow(so the field editor attaches), callsbeginInlineRename(), and assertscommitRenameis not invoked. Fails on the buggy path (selectText → spurious commit), passes with the fix.Test plan
cmux-unit-only-testing:cmuxTests/SidebarAppKitRowCellTests/beginInlineRenameLeavesFieldEditingWithoutCommittingPrefilledTitle(green with the fix)Notes
TextField), so no change there./cc @lawrencecchen
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the AppKit sidebar double-click rename so it no longer commits instantly or flashes; the field stays editable and commits only when expected. Closes #9495.
selectText(nil)aftermakeFirstResponderto prevent a synchronous end-edit and spurious commit.#if DEBUG.Written for commit b8fd6cd. Summary will update on new commits.
Summary by CodeRabbit