Repository navigation
fix: read every sidebar mode's availability from injected defaults - #16353
lawrencecchen wants to merge 13 commits into
Conversation
The same change as #16232 (7a51764), carried so this PR can restore main's cmuxTests build in one piece. #15381 made LastSurfaceClosePreferenceTests and WorkspaceCloseTabsContextMenuTests call drainMainQueue(timeout:), but the shared helper takes no arguments. Refs #15488 Co-authored-by: Leo Li <cheerleaderleo@outlook.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The same change as #16242 (90851e5), carried so this PR can restore main's cmuxTests build in one piece. #15381 called CMUXCLI.vmReadyPollInterval from the app-hosted CLIVMTransferTests, where CMUXCLI names the app's routing type, not the CLI. The policy check moves to cmuxCLITests, which builds the CLI target. Refs #15488 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#15420 (d0dd226) resolved a merge in this test by dropping `let controller = workspace.bonsplitController` while the divider assertions below still use `controller`, so main's cmuxTests don't compile: cmuxTests/PaneResizeShortcutTests.swift:66:39: error: cannot find 'controller' in scope The binding comes back just before its first use. Refs #15488 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#16158's budget test passes budget.admit(...) straight to #expect. Xcode 26.3's macro expands the argument inside a closure where budget is immutable ("cannot use mutating member on immutable value"), so the macOS 15 lane fails at TEST BUILD. Bind each result first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
#16196 added ClaudeHookSessionStoreRecoveryTests with `@testable import cmux_cli`. cmux_cli is the cmux-cli executable, which cmuxCLITests does not link and cannot host, so the target stopped compiling ("Unable to find module dependency: CmuxControlSocketAtomicsC / CmuxSimulatorSystem"), and adding those packages would only move the failure to link time. The two tests now seed the hook state file, run a real `cmux hooks claude session-start` against a mock socket, and read what the CLI left on disk, like the rest of cmuxCLITests: - a malformed sibling record no longer discards a valid session mapping, and a salvageable file is not quarantined; - each of two unreadable state files is moved to its own quarantine backup with its original bytes, and the store keeps working afterwards. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
#16245 moved the vm poll-interval check into cmuxCLITests with `@testable import cmux_cli`, which cannot compile or link for the same reason as the hook store tests: cmux_cli is the CLI executable. The pure policy now lives in CLI/VMReadyPollInterval.swift, compiled into both the CLI and cmuxCLITests (the CMUXCLI+AutoNaming precedent), and CMUXCLI.vmReadyPollInterval delegates to it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Every hook save prunes records older than the state retention window, so the 1970 timestamps from the in-process test made the valid record vanish for a reason unrelated to decode recovery. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
RightSidebarMode.availableModes(defaults:) passed the injected suite only to the feed and machines flags. The custom-sidebar case went through the flag-only isAvailable overload, which reads UserDefaults.standard, so a FileExplorerState built on an isolated suite reported .customSidebar as its mode while availableModes(defaults:) omitted it. Filter allCases through isAvailable(defaults:), the one predicate that already reads every flag from the given store. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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. |
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (4)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 (10)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes VM-ready polling interval resolution, updates sidebar mode availability to use the supplied defaults store, and changes Claude hook recovery tests to exercise the bundled CLI. Several other tests receive assertion, reference, or timeout adjustments. ChangesVM Poll Interval
Sidebar Mode Availability
Claude Hook Recovery Tests
Other Test Updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change fixes sidebar availability so it reads from the injected defaults store, and it centralizes VM poll interval handling without changing its behavior. The remaining changes are test updates. No concrete merge-blocking risk was found. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 15.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 8 files. (1 skipped: 1 unsupported.) Full details: Cmux Swift Package BoundariesExplanation The new Resolution Create a small SwiftPM target such as
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxCLITests/ClaudeHookSessionStoreRecoveryTests.swift">
<violation number="1" location="cmuxCLITests/ClaudeHookSessionStoreRecoveryTests.swift:125">
P3: `runSessionStart` sets `HOME` for the spawned `cmux` CLI but omits `CFFIXED_USER_HOME`. The repo's established child-process isolation pattern (see `codexHookTestEnvironment` in cmuxCLITestSupport/CLICodexHookTimeoutRegressionTestSupport.swift, which always sets both) requires both so the subprocess never reads the real user's CFPreferences/`~/Library/Preferences` defaults. Add `CFFIXED_USER_HOME` pointing at the same per-test `root` directory.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| executablePath: cliPath, | ||
| arguments: ["hooks", "claude", "session-start"], | ||
| environment: [ | ||
| "HOME": root.path, |
There was a problem hiding this comment.
P3: runSessionStart sets HOME for the spawned cmux CLI but omits CFFIXED_USER_HOME. The repo's established child-process isolation pattern (see codexHookTestEnvironment in cmuxCLITestSupport/CLICodexHookTimeoutRegressionTestSupport.swift, which always sets both) requires both so the subprocess never reads the real user's CFPreferences/~/Library/Preferences defaults. Add CFFIXED_USER_HOME pointing at the same per-test root directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmuxCLITests/ClaudeHookSessionStoreRecoveryTests.swift, line 125:
<comment>`runSessionStart` sets `HOME` for the spawned `cmux` CLI but omits `CFFIXED_USER_HOME`. The repo's established child-process isolation pattern (see `codexHookTestEnvironment` in cmuxCLITestSupport/CLICodexHookTimeoutRegressionTestSupport.swift, which always sets both) requires both so the subprocess never reads the real user's CFPreferences/`~/Library/Preferences` defaults. Add `CFFIXED_USER_HOME` pointing at the same per-test `root` directory.</comment>
<file context>
@@ -1,58 +1,142 @@
+ executablePath: cliPath,
+ arguments: ["hooks", "claude", "session-start"],
+ environment: [
+ "HOME": root.path,
+ "PATH": "/usr/bin:/bin:/usr/sbin:/sbin",
+ "CMUX_SOCKET_PATH": socketPath,
</file context>
| "HOME": root.path, | |
| "HOME": root.path, | |
| "CFFIXED_USER_HOME": root.path, |
Dogfood tours of
|
CI failure attributionCI failed on
Matched log linesNot re-run automatically: Written by |
teamleaderleo
left a comment
teamleaderleo
left a comment
There was a problem hiding this comment.
Superseded on main: 91fca8c (#12809) already changed RightSidebarMode.availableModes(defaults:) to allCases.filter { $0.isAvailable(defaults: defaults) }, the same change as here. The rest of the diff is the #16306 stack, which conflicts in four test files and auto-merges a duplicate let controller into cmuxTests/PaneResizeShortcutTests.swift (see my note on #16306). Nothing is left to land, so this can close once testInjectedDefaultsOwnCustomSidebarAvailabilityAndPersistence passes on main.
One gap is left on main, not caused by this PR: the flag-only isAvailable(feedEnabled:machinesEnabled:) still reads .customSidebar from .standard. Any caller of availableModes(feedEnabled:machinesEnabled:) still has the original mixed-store bug.
3 participants
Summary
FileExplorerStateModePersistenceTests.testInjectedDefaultsOwnCustomSidebarAvailabilityAndPersistencefails on main (seen in run https://github.com/manaflow-ai/cmux/actions/runs/36796217267, shard 3/7). It was added by #15381 but never ran green on main because the test target stopped compiling right after that merge.The product bug:
RightSidebarMode.availableModes(defaults:)passed the injectedUserDefaultsonly to the feed and machines flags. The custom-sidebar case went through the flag-onlyisAvailable(feedEnabled:machinesEnabled:)overload, which readsUserDefaults.standard. AFileExplorerStatebuilt on an isolated suite therefore reported.customSidebaras its mode whileavailableModes(defaults:)left it out.availableModes(defaults:)now filtersallCasesthroughisAvailable(defaults:), the single predicate that already reads every flag (feed, machines, custom sidebar) from the given store. Production callers pass.standard, so their result is unchanged.Stacked on #16306, which restores test-target compilation. Retarget or rebase once it merges.
Testing
Red: the existing test fails at
FileExplorerStateModePersistenceTests.swift:131(XCTAssertTrue failed) in run 36796217267 attempt 2, job "app-host unit tests (3/7)". Green: full-suiteci.ymlrun on this branch, linked in a comment below. Swift syntax check passed locally. No app tests ran on this Mac.Changelog
none
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the right sidebar availability check so the custom-sidebar mode reads its flag from injected defaults, and restores the
cmuxCLITestsbuild on main that broke after #15381.RightSidebarMode.availableModes(defaults:)only passed the injected suite to the feed and machines flags; the custom-sidebar case readUserDefaults.standardinstead, so aFileExplorerStatebuilt on an isolated suite could report.customSidebarwhileavailableModes(defaults:)excluded it. The method now filtersallCasesthroughisAvailable(defaults:), the predicate that already reads every flag from the given store, so the reported mode and the available list always agree. Production callers pass.standard, so their result is unchanged.Test build
cmuxbinary against a mock socket instead of@testable import cmux_cli, which this target cannot link; seeded records use current timestamps so prune-on-save doesn't remove them for an unrelated reason.VMReadyPollInterval.swift, compiled into both the CLI andcmuxCLITests, so override boundaries are testable there and the app-hostedCLIVMTransferTestscheck is gone.PaneResizeShortcutTestsrestores thecontrollerbinding its divider assertions need; the reconcile-budget test binds results before#expect;drainMainQueueaccepts an optional timeout.Written for commit 935b08c. Summary will update on new commits.
Summary by CodeRabbit
vm waitnow usesCMUX_VM_WAIT_POLL_SECONDSonly when it is a valid interval from 0.01 to 3 seconds; otherwise, it uses the 3-second default.