Repository navigation
Hold Files tree reloads while its context menu is open - #14451
Conversation
Ctrl-click on a Files row crashed in -[NSTableRowData rowViewAtRow: createIfNeeded:] while AppKit drew the context-menu highlight. The highlight keeps the clicked row index until the menu closes, and the coordinator reloads the outline from store changes and from SwiftUI's updateNSView, which can run in the same Core Animation commit that paints the highlight. Track the menu with willOpenMenu/didCloseMenu on the outline, skip reloads while it is open, and run one catch-up reload after it closes. Fixes #12914 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
Merged per Leo's direction to land without waiting on full CI. Reviewed adversarially (AppKit ordering checked with a probe: menuNeedsUpdate, then willOpenMenu, then the popup; open/close always pair). Caveat: this head's macOS compile admission never started. It sat queued on blacksmith-6vcpu-macos-26 for about 2.5 hours over three attempts, so the first compile of this change is main's next build. If main goes red in FileExplorerView.swift, FileExplorerNSOutlineView.swift or FileExplorerContextMenuReloadTests.swift, that's this PR. |
|
Merge receipt for
Labeled |
c055747 ci: parse runner expressions in the fork guard, and gate LINUX_RUNNER on fork PRs (manaflow-ai#14192) fc112a8 docs(testing): describe how run-e2e.sh actually picks the runner (manaflow-ai#14620) 0f6edfa ci(owned): keep seeds until the disk is actually short, not at a fixed 6 (manaflow-ai#14621) 1b78097 CI: one owned-pool rescue sweeper instead of a rescue run per CI run (manaflow-ai#14602) c7e79f6 Hold Files tree reloads while its context menu is open (manaflow-ai#14451) dbb24cb ci: read the owned-pool rescue's Actions API through the route App (manaflow-ai#14499) 36b8063 ci: store each app-host product file once in the product archive (manaflow-ai#14601) 0dba677 CI: run CmuxWorkspaces package tests (manaflow-ai#14592) 2244e98 Fix color detection in native tmux mirrors (manaflow-ai#14175) 3d2478e build: keep every built file in a project group so Xcode reuses its build description (manaflow-ai#14486) a1a5ae9 ci(cmux-tui): cache cargo builds in the Rust jobs (manaflow-ai#14613) 560e640 ci(owned): keep 8 seeds per mini and pick seeds by cost, not a 2-commit cap (manaflow-ai#14607) 561d317 cmux-debug-cli: find a publish-hq build in the HQ tag app cache (manaflow-ai#14579) c0aaac7 fix(events): survive receive-timeout reconfiguration churn during replay (manaflow-ai#13888) 3bd994a fix: keep split zoom when the zoomed pane outlives a tab close (manaflow-ai#12853) ca984c7 Session snapshots can send binding actions to ghostty on a surface that is no longer live (manaflow-ai#12623) a47d65b settings: expose local tmux session persistence (manaflow-ai#13210) e3acb25 ci: give an owned Mac's full app rebuild 35 minutes to compile (manaflow-ai#14594) # Conflicts: # .github/workflows/ci-artifact-transport.yml # .github/workflows/ci-cache-receipts.yml # .github/workflows/ci-guards.yml # .github/workflows/ci-macos.yml # .github/workflows/ci-owned-pool-rescue.yml # .github/workflows/ci-web.yml # .github/workflows/ci.yml # .github/workflows/cloud-machine-tests.yml # .github/workflows/cloud-vm-guest-install.yml # .github/workflows/cloud-vm-image-contract.yml # .github/workflows/cloud-vm-image-reachability.yml # .github/workflows/cloudflare-relay.yml # .github/workflows/cmux-skill-contract.yml # .github/workflows/cmux-tui-sdks.yml # .github/workflows/cmux-tui-spec.yml # .github/workflows/cmux-tui.yml # .github/workflows/indexnow-tests.yml # .github/workflows/iroh-v2.yml # .github/workflows/localization-catalog.yml # .github/workflows/r2-upload-tests.yml # .github/workflows/remote-daemon.yml # .github/workflows/repair-nightly-appcast-content-types.yml # .github/workflows/required-checks-drift.yml # .github/workflows/resolve-dispatch-ref.yml # .github/workflows/seed-derived-data.yml # .github/workflows/terminal-hang-diagnostics.yml # .github/workflows/test-e2e.yml # .github/workflows/test-ios.yml # .github/workflows/testbox-broker-guard.yml # .github/workflows/web-validation.yml
Summary
Ctrl-clicking a row in the Files sidebar could crash cmux (#12914, 4 reports on 0.64.24 and 0.64.25). Every report has the same stack: AppKit throws from
-[NSTableRowData rowViewAtRow:createIfNeeded:]while drawing the context-menu highlight for the clicked row, inside the Core Animation commit thatNSContextMenuTrackingSession startMonitoringEventsruns as the menu appears.AppKit keeps the clicked row index for that highlight until the menu closes. Meanwhile the Files coordinator reloads the outline (
reloadData, andreloadItem(_:reloadChildren:)plus re-expansion for every loaded folder) whenever the store changes, and SwiftUI'supdateNSViewcalls the samereloadIfNeeded()synchronously. SwiftUI flushes those updates inside a Core Animation commit, so a reload can land between the highlight row being chosen and being drawn. Store changes are frequent here: hovering a folder prefetches its children, and git status and directory watchers publish too.The outline now tracks its own context menu with
willOpenMenu(_:with:)/didCloseMenu(_:with:). While the menu is open,reloadIfNeeded()records that it owes a reload and returns without touching rows. When the menu closes it runs one catch-up reload on the next main-queue turn, after AppKit has cleared the highlight. Menu actions keep working because each item already carries itsFileExplorerNodeasrepresentedObject.The Find results table has the same shape (a context menu over rows that stream in) but is not in the reports; I left it alone here.
Fixes #12914
Testing
FileExplorerContextMenuReloadTests: with three rows loaded, it opens the menu, shrinks the store to one row and reloads (rows stay at 3), then closes the menu and waits up to 2 s for the deferred reload (rows become 1). It runs in the cmux unit lane in CI; I did not build cmux locally.scripts/lint-pbxproj-test-wiring.shandscripts/check-pbxproj.shpass.Checklist
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the cmux crash on ctrl-clicking a Files sidebar row by holding outline reloads while a context menu is open.
AppKit keeps drawing the context-menu highlight for the clicked row until the menu closes and throws if a reload removes that row underneath it. The outline now tracks its own context menu and skips reloads while one is on screen; a single catch-up reload runs on the next main-queue turn after the menu closes. Menu actions are unaffected since each item already carries its node as
representedObject. The Find results table has the same shape but is not in the crash reports, so it is left unchanged.Testing
FileExplorerContextMenuReloadTestscovers the hold-then-catch-up behavior.Written for commit acee8e6. Summary will update on new commits.