Repository navigation
Apply sidebar workspace close/create as row edits instead of reloadData - #14866
Conversation
Any change to the sidebar's row id list that wasn't a small pure reorder went through reloadData, which retires every visible cell. Closing or creating one workspace (in this window, from the CLI, or by an agent) committed rename and checklist drafts early and closed popovers on unrelated rows, and repainted every row from recycled cells. When the next ids are the previous ids with rows only dropped or only added (order preserved), apply them with removeRows/insertRows and reconfigure the visible rows for per-index state. Mixed edits, reorders with height changes, forced reloads, and duplicate ids keep the reload path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe sidebar table now applies eligible order-preserving row additions and removals as incremental table updates. Reorder updates remain separate. Tests cover edit classification and preservation of surviving row cells. ChangesSidebar row updates
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant SidebarWorkspaceTableController
participant SidebarWorkspaceTableRowEdit
participant NSTableView
SidebarWorkspaceTableController->>SidebarWorkspaceTableRowEdit: classify old and new row IDs
SidebarWorkspaceTableRowEdit-->>SidebarWorkspaceTableController: return insertion or removal indexes
SidebarWorkspaceTableController->>NSTableView: insert or remove affected rows
SidebarWorkspaceTableController->>NSTableView: reconfigure loaded rows
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The incremental sidebar updates refresh retained rows after each edit. No merge-blocking issue was established; the change is ready to merge subject to normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is confined to sidebar presentation and preserves existing cleanup for removed rows. No new security boundary or authority is apparent, but the effects of externally initiated workspace changes and downstream draft commits are not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 passed)
Full details: Description checkExplanation The description clearly explains the problem, implementation, scope, and added tests. However, it does not state which test commands ran or their results, provides no required demo video or screenshot, and leaves required checklist items unresolved. Resolution Add the executed test commands and outcomes. Include a demo video or screenshot for this UI behavior change. Complete the applicable checklist items and document localization, documentation, and review status, or explain why each item does not apply. Full details: Cmux Swift Package BoundariesExplanation The diff adds Resolution Extract
✨ Finishing Touches 💡 1📝 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 |
NSTableView keeps prepared row views outside the viewport and does not ask viewFor again when they scroll in, so reconfiguring only visible rows after removeRows/insertRows (and the existing moveRow path) could leave them with a stale shortcut digit, first-row flag, or content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge receipt for |
…idebar close fixes (#14885) Pulls manaflow-ai/bonsplit#253 (strip-owned tab hover, Close Tab accessibility action) and records the merged sidebar fixes #14826 and #14866 under Unreleased. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
8e27d37 Bump bonsplit for tab hover that follows the pointer; changelog for sidebar close fixes (manaflow-ai#14885) 5d24cf4 test: free hosted test terminals before the test returns (manaflow-ai#14886) e34ab0a Apply sidebar workspace close/create as row edits instead of reloadData (manaflow-ai#14866) ec763d5 Stop sidebar close buttons flashing on every row after a close (manaflow-ai#14826) 8f9c685 Restore legacy Subrouter Claude sessions through the proxy (manaflow-ai#14412) 7e2157e test: expect the Claude Teams restore preload to survive an unusable TMPDIR (manaflow-ai#14880) f16e4e7 Fix prediction echo misses on sgr0 and bound keys, keep pinned-group windows on restore (manaflow-ai#14860)
Summary
Any change to the AppKit sidebar's row id list that wasn't a small pure reorder went through
reloadData, which retires every visible cell throughtableView(_:didRemove:forRow:)→retirePresentation(commitEdits: true). Closing or creating a single workspace, whether from this window, the CLI, or an agent, therefore:Change
SidebarWorkspaceTableRowEditclassifies a structural change as a pure removal or a pure insertion: the surviving rows keep their relative order, and ids are unique. Those changes now apply withremoveRows/insertRows(no animation, inside the existing geometry-update serializer), then reconfigure the visible rows, since per-index state like shortcut digits and group counts shifts. Cells whose model is unchanged skip the repaint.Mixed edits, reorders, forced reloads, duplicate ids, and the first population keep the existing paths. Group collapse and expand take the new path too, since they are pure drops and adds.
Tests
SidebarWorkspaceRowRetirementTests:closingOneWorkspaceKeepsSurvivingRowCellsmounts three rows, closes the middle one, and asserts the surviving rows keep the same cell instances.rowEditClassifiesOnlyOrderPreservingDropsAndAddscovers removal, insertion, reorder, mixed edits, no-op, and duplicate ids.The existing retirement tests, which remove the last row, now go through
removeRowsand still expect retirement viadidRemove.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Applies sidebar workspace close/create (and group collapse/expand) as
removeRows/insertRowsedits instead ofreloadData. Previously any structural change retired every visible cell, which committed rename and checklist drafts on unrelated rows early, closed their popovers, and repainted every row from a recycled cell.Written for commit 2919ac8. Summary will update on new commits.
Summary by CodeRabbit