Repository navigation
Fix AppKit sidebar double-click inline rename committing instantly (#9495) - #9798
Merged
Merged
Conversation
Double-clicking a workspace name on the AppKit sidebar list creates the inline rename field but tears the editing session down ~1ms later, committing the untouched title. These tests drive the real AppKit editing path (cell in a window, shared field editor, commands dispatched through the field editor) and assert the intended behavior: begin keeps the session alive without a write, Enter commits the live editor text once, an unchanged title is a no-op, Escape cancels, and focus loss commits the typed draft. Test-only commit: CI is expected to go red until the fix lands. Issue: #9495 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Double-clicking a workspace name on the AppKit sidebar list committed the untouched title ~1ms after the rename field appeared: after makeFirstResponder began the editing session, the follow-up selectText(nil) re-entered the field-editor machinery, synchronously fired controlTextDidEndEditing, and the row's forked SidebarRowInlineRenameField honored it by committing stringValue. Delete the fork and route the AppKit list through the SwiftUI sidebar's engine, one rename session per edit (SidebarRowInlineRenameSession): - SidebarInlineRenameTextField focuses and selects once, when the field enters the window; the selectText restart is gone by construction. - SidebarInlineRenameCoordinator resolves Enter, double-Escape, and focus loss at most once, passes IME composition through, and commits the live field-editor text instead of a stale stringValue. - SidebarInlineRenameCommit gives the AppKit path the same commit policy as SwiftUI: empty drafts and unchanged auto-titles resolve to no write, so a stray commit can never freeze auto-naming. SidebarWorkspaceRowModel gains hasUserCustomTitle (plumbed from SidebarWorkspaceRowInput) as the policy baseline. Cell suspension resolves the session before teardown, so end-editing during teardown can no longer re-enter commit while the write itself stays deferred past the table mutation. isEditing is now derived from the session instead of a mutable flag. Existing suspension tests that poked the old field's internals now drive the real session through the field editor. Fixes #9495 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Two-commit regression proof (dispatched
|
📝 WalkthroughWalkthroughThe AppKit sidebar now uses ChangesAppKit inline workspace renaming
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SidebarWorkspaceRowCellView
participant SidebarRowInlineRenameSession
participant SidebarInlineRenameTextField
SidebarWorkspaceRowCellView->>SidebarRowInlineRenameSession: begin inline rename
SidebarRowInlineRenameSession->>SidebarInlineRenameTextField: configure and focus field
SidebarInlineRenameTextField->>SidebarRowInlineRenameSession: send commit or cancel
SidebarRowInlineRenameSession->>SidebarWorkspaceRowCellView: resolve title once
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
![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 `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift`:
- Around line 1018-1022: Replace the cmuxDebugLog call in the beginInlineRename
debug probe with dlog, preserving the existing `#if` DEBUG guard and message
content; alternatively remove the probe. Ensure this Sidebar code does not use
cmuxDebugLog.
🪄 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: f6735a1b-1b84-4ded-ba4c-d0a44f554368
📒 Files selected for processing (9)
Sources/ContentView.swiftSources/Sidebar/AppKitList/Cells/SidebarRowInlineRenameSession.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowModel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarAppKitRowCellTests.swiftcmuxTests/SidebarWorkspaceRowInlineRenameTests.swiftcmuxTests/SidebarWorkspaceRowSuspensionTests.swiftcmuxTests/SidebarWorkspaceTableSuspensionTests.swift
The attach-time focus grab sizes the shared field editor from the field's current frame, and a zero-frame grab mis-sizes the editor's dark box over the row — the same lifecycle SidebarRowChecklistItemLine already documents and handles. Run the row layout pass before adding the field so it enters the window with its title-slot frame, and clear the field-editor background after attach, reusing the checklist's helper. Codex review finding on #9798. Co-Authored-By: Claude Fable 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
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.
Fixes #9495.
What was broken
Double-clicking a workspace name on the AppKit sidebar list (
sidebar-appkit-list-experiment, default-on) created the inline rename field, gave it first responder, and then lost the field editor ~1 ms later, committing the pre-filled unchanged title. The field flashed and the user could never type.Root cause, confirmed in code:
beginInlineRename()calledwindow.makeFirstResponder(renameField)(which begins the editing session) and thenrenameField.selectText(nil). The extraselectText(nil)re-enters AppKit's field-editor machinery, synchronously tears down the just-begun session, and firescontrolTextDidEndEditing, which the row's forkedSidebarRowInlineRenameFieldhonored by committingstringValue— the untouched title.Why the fix is structural, not a one-line patch
The AppKit list had forked the inline-rename engine instead of reusing the SwiftUI sidebar's canonical one (
SidebarInlineRenameTextField+SidebarInlineRenameCoordinator+SidebarInlineRenameCommit). The fork had no once-only commit/cancel guarantee, no two-stage Escape, no IME (marked text) pass-through, committed stalestringValueinstead of the live field-editor text, and treated every end-editing as a commit with no policy — so a spurious commit also wrotecustomTitle, converting an automatic title into a user title and freezing auto-naming (theworkspace.customTitle.write source=userline in the issue's log).Per the shared-behavior policy (one action path per behavior across entrypoints), this PR deletes the fork and routes the AppKit list through the same engine as the SwiftUI sidebar:
SidebarRowInlineRenameSession(new focused file): one rename session per edit, owning the sharedSidebarInlineRenameTextField(focus + select-all happens once, when the field enters the window — theselectText(nil)restart is gone by construction), the sharedSidebarInlineRenameCoordinator(Enter/double-Escape/focus-loss resolve at most once; IME composition passes through; commits live editor text), and the sharedSidebarInlineRenameCommitpolicy (empty drafts and unchanged auto-titles resolve to no write).SidebarWorkspaceRowTableCellView:isEditingis now derived from the session (renameSession != nil) instead of a mutable flag; the persistent hiddenrenameFieldsubview is gone; suspension resolves the session before teardown so end-editing during teardown can never re-enter commit, while the write itself stays deferred past the table mutation (unchanged staging contract).SidebarWorkspaceRowModel: gainshasUserCustomTitle(plumbed from the existingSidebarWorkspaceRowInputfield) so the commit policy has the same baseline input as the SwiftUI path.Behavior parity gained with the SwiftUI sidebar: select-all on focus, Enter commits live text once, first Escape moves the caret to the start / second Escape cancels, focus loss commits, IME-safe, and unchanged-title commits no longer freeze auto-naming.
Two-commit regression proof
cmuxTests/SidebarWorkspaceRowInlineRenameTestsonly — five behavior tests driving the real AppKit editing path (cell hosted in a window, commands dispatched through the shared field editor). Dispatchedtest-e2e.ymlat the commit-1 SHA: red (run link in PR comments).Existing suspension tests that poked the old field's internals (
cell.isEditing = true,renameField.stringValue,field.onCommit?(...)) were updated in commit 2 to drive the real session (begin → type through the field editor → resolve), keeping their behavioral assertions (deferred commits through atomic reloads/detach) intact.Localization audit
No new user-facing strings. The AppKit rename field now carries the same localized placeholder (
commandPalette.rename.workspacePlaceholder) and accessibility label (sidebar.workspace.rename.field.accessibilityLabel) as the SwiftUI field — both keys already exist inResources/Localizable.xcstrings. Web catalogs unaffected.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Low Risk
Localized sidebar UI rename behavior with shared commit policy and regression tests; no auth, data, or persistence schema changes beyond existing custom-title writes.
Overview
Fixes #9495: double-click rename on the AppKit sidebar list no longer flashes and immediately commits the untouched title.
The AppKit row drops its forked
SidebarRowInlineRenameFieldand routes inline rename throughSidebarRowInlineRenameSession, which wires the sameSidebarInlineRenameTextField,SidebarInlineRenameCoordinator, andSidebarInlineRenameCommitpath as the SwiftUI sidebar. Focus and select-all happen when the field enters the window hierarchy—no post-focusselectText(_:)that restarted the field editor and triggered spurious commits.SidebarWorkspaceRowModelnow carrieshasUserCustomTitleso unchanged auto-titles resolve to nocustomTitlewrite (avoiding frozen auto-naming). Suspension/detach resolves the session before teardown while still deferring writes past table mutations.Adds
SidebarWorkspaceRowInlineRenameTestsand updates suspension/table tests to drive the real field-editor path.Reviewed by Cursor Bugbot for commit ffd5b85. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the AppKit sidebar bug where double‑click inline rename instantly committed the unchanged title, and fixes the editor box sizing glitch on focus. The AppKit list now reuses the SwiftUI inline‑rename engine for stable, IME‑safe editing with consistent behavior.
Bug Fixes
Refactors
SidebarRowInlineRenameSessionusing the sharedSidebarInlineRenameTextField,SidebarInlineRenameCoordinator, andSidebarInlineRenameCommit.isEditingnow derives from the active session; suspension resolves before teardown and defers writes past table mutations.SidebarWorkspaceRowModeladdshasUserCustomTitleand is plumbed from input for commit policy parity.Written for commit c3472d6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests