Settings load writes UserDefaults when nothing changed, and pays for it twice over

[ejc3]

What happens

Loading settings writes UserDefaults even when nothing changed, and that write costs far more than it looks.

KeyboardShortcutSettingsFileStore's construction, and every later file-watcher reload while cmux.json is being edited, unconditionally removes one legacy key and rewrites two others. UserDefaults posts didChangeNotification for a write that changes nothing, and for removing a key that was never there. An observer registered with queue: .main runs synchronously on the posting thread rather than being deferred, and SystemWideHotkeyController observes that notification by re-registering the global-search hotkey — which walks the whole action table twice through KeyboardShortcutSettings, reading each entry out of UserDefaults and decoding it.

So each settings load pays for a no-op write: every UserDefaults observer in the app wakes, the managed-settings reapply runs again, and roughly two hundred actions are read and decoded twice.

It is also a latent re-entrancy hazard. Those lookups reach back through the static store that may still be inside its own initializer; on the app path the store is a lazy static, so only launch ordering keeps this from re-entering its own initialization.

The fix is to write only when the value actually changes, which is the rule restoreUserDefaultsBackup in this same file already follows.

Why no new test

The coverage already existed and was already failing. testLegacySettingsShortcutBindingsParseWithoutRuntimeConflictLookup asserts that parsing a legacy shortcuts file performs no runtime conflict lookup, and it has been red because construction triggers exactly that through the notification above.

Worth recording, because it is what made this hard to see: the same test also asserts that zero KeyboardShortcutSettings.didChangeNotification posts reach the default center during the window, and that assertion passed the whole time. The trigger is UserDefaults.didChangeNotification, a different notification, so the test's own instrumentation could not point at it. The recorded lookups start with a lone globalSearch and then run through the entire action table, which is the exact shape of one hotkey re-registration.

Two Foundation behaviours this depends on were checked by running them, not by reading documentation: an observer registered with queue: .main runs synchronously when the post happens on the main thread, and removeObject posts even for an absent key.

Verification

Run on a macOS builder, identical arms, only this change between them:

arm	KeyboardShortcutSettingsFileStoreMigrationTests
base eeb4866b17	failing
this branch	passing

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

---

Summary by cubic

Stop no‑op UserDefaults writes during settings load and file‑watcher reloads to prevent redundant notifications and hotkey re‑registration. Make persistence deterministic across relaunches and inject UserDefaults/LanguageSettingsStore end‑to‑end.

• Bug Fixes

  • Gate unchanged writes for cmux.settingsFile.importedManagedDefaults.v1 and cmux.settingsFile.backups.v1; encode with sorted‑key JSON to avoid rewriting equal content; remove legacy keys only if present.
  • Inject UserDefaults and LanguageSettingsStore across the store; pass injected defaults through socket‑policy resolution and fail‑closed mode; watchers compare with injected defaults; choosing “system” clears language overrides in the injected suite; apply app icon using injected defaults.
  • Add focused tests for no‑op persistence and language reset; fix NotificationsPage .onChange deprecations.

• CI

  • Run determinism guard self‑test before the strict scan; broaden detection (leading redirections, stored base URLs, Python shell‑string/keyword launchers, command substitutions, multiline/evaluated sources, block comments); scope live‑network checks to invocation targets; ignore URL‑like env/headers; allowlist Sparkle pretag probe.
  • Add a standalone, non‑tolerant app‑host step for the settings no‑op regression; split each worker into two serial app‑host batches with strict timeout/kill; reject empty/failed shard generation; don’t reuse prior “expected failures” summaries after crashes; keep cross‑machine parallelism; add CI tests to enforce these semantics.

^{Written for commit 73ad557. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes

  • Improved settings persistence by avoiding unnecessary updates when imported settings or backups are unchanged.
  • Prevented redundant notifications and side effects during repeated saves.
  • Fixed language preference cleanup when switching back to the system language.
  • Improved startup handling for socket policy settings.

• Improvements

  • Ensured consistent settings formatting for reliable comparisons across app launches.
  • Increased reliability when applying settings during startup and managed-settings updates.
  • Updated notification settings interactions for compatibility with current system behavior.

---

Note

Medium Risk
Changes sit on the hot settings-load and UserDefaults notification path at startup and during file-watcher reloads, but the behavioral change is narrowly scoped to skip writes when values are unchanged.

Overview
Settings file load and reload no longer touch UserDefaults when imported managed defaults and backup blobs are already equivalent, avoiding spurious UserDefaults.didChangeNotification posts that synchronously re-register global hotkeys and re-read the full shortcut table.

KeyboardShortcutSettingsFileStore now compares decoded values before persisting, uses sorted-key JSON when a write is needed so legacy non-canonical bytes aren’t rewritten on upgrade, and removes legacy sidebar keys only if they exist. The same injectable UserDefaults pattern is extended through language override application (clearing overrides when returning to system language), socket-policy resolution, and app-icon startup paths, with regressions for no-op persistence and language reset.

CI adds a dedicated app-host step for the no-op regression, tighter determinism-guard scanning, and safer parallel app-test sharding; NotificationsPage updates deprecated .onChange usage.

^{Reviewed by Cursor Bugbot for commit 73ad557. Bugbot is set up for automated code reviews on this repo. Configure here.}

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Settings persistence now supports injected stores and skips semantically redundant writes. CI runs eight logical test batches with selector coverage checks. Network determinism detection recognizes executable process launches. Notification handlers use updated closure syntax.

Changes

Settings persistence

Layer / File(s)	Summary
Inject settings stores and route resolution Sources/KeyboardShortcutSettingsFileStore.swift, Sources/CmuxSettingsFileStore+Live.swift	Settings loading, socket-policy resolution, backups, managed defaults, language overrides, and icons use injected stores.
Suppress redundant persistence writes Sources/KeyboardShortcutSettingsFileStore.swift, cmuxTests/KeyboardShortcutSettingsFileStoreNoOpPersistenceTests.swift, cmux.xcodeproj/project.pbxproj	Imported defaults and backups conditionally remove keys, compare decoded values, and use canonical sorted-key JSON encoding. Tests cover no-op writes and language override removal.

CI test sharding

Layer / File(s)	Summary
Run sequential logical test batches .github/workflows/ci.yml, tests/test_ci_change_areas.py	Each physical worker runs two logical batches with independent output, timeout, and failure handling.
Validate logical shard coverage tests/test_ci_cmux_unit_test_shard.py, scripts/ci/cmux_unit_test_shard.py, tests/test_ci_change_areas.py	Shard checks validate unique assignment and complete selector coverage across eight logical shards. The focused persistence suite is excluded from discovery.

Network determinism detection

Layer / File(s)	Summary
Detect executable network launches scripts/check-test-determinism.py, .github/workflows/ci.yml, .github/test-determinism-allowlist.txt	The detector identifies executable network commands through process-launch APIs and ignores commands embedded in inert text. Self-tests run before strict validation, and one intentional network test is allowlisted.

Notification handler cleanup

Layer / File(s)	Summary
Simplify notification handlers Sources/NotificationsPage.swift	Three onChange closures omit the unused change-value parameter.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CIWorker as CI worker
  participant Workflow as run_unit_tests
  participant Xcodebuild as xcodebuild
  participant ShardValidator as shard validation
  CIWorker->>Workflow: start physical worker
  Workflow->>Xcodebuild: run logical batch 1
  Xcodebuild-->>Workflow: return test output and status
  Workflow->>Xcodebuild: run logical batch 2
  Xcodebuild-->>Workflow: return test output and status
  Workflow->>ShardValidator: validate logical selector assignments
  ShardValidator-->>CIWorker: report coverage and failures

Loading

Suggested reviewers: lawrencecchen

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors, 1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Cmux Algorithmic Complexity	❌ Error	Sources/Workspace+CmuxNavigationDescriptor.swift:22 sorts every panel's unbounded remote-tmux runtime IDs during each navigation snapshot, adding O(Σrᵢ log rᵢ) work without a bound or benchmark.	Deduplicate runtime IDs in linear time while preserving pane order, or cache the navigation descriptor and rebuild it only when topology changes.
Cmux Swift Package Boundaries	❌ Error	The diff keeps managed-settings persistence and canonical JSON logic in root Sources/CmuxSettingsFileStore.swift; injected defaults and isolated no-op tests confirm an independently testable domain...	Extract managed-settings values, backups, and canonical persistence into the existing CmuxSettings package target. Expose a public ManagedSettingsPersistence protocol; keep watchers and AppDelegate side effects in the app target.
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.88% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check	❓ Inconclusive	The supplied context does not define a linked-issue requirement or provide a linked issue for assessment.	Provide the repository's linked-issue policy and any required issue references.

✅ Passed checks (21 passed)

Check name	Status	Explanation
Cmux Swift Actor Isolation	✅ Passed	Production changes inject UserDefaults and LanguageSettingsStore and update pure helpers; they add no MainActor/Sendable declarations or new background UI access. Existing UI isolation remains unch...
Cmux Swift Blocking Runtime	✅ Passed	Production Swift additions contain no semaphore, wait, sleep, delayed dispatch, polling, main-queue sync, or lock APIs; the existing NSLock/main async remain unchanged, and async confirmation is te...
Cmux Browser Automation Off-Main	✅ Passed	The full PR diff changes neither TerminalController.swift nor ControlCommandExecutionPolicy.swift, and adds no browser socket command or routing changes.
Cmux Expensive Synchronous Load	✅ Passed	Changed Swift code adds settings persistence and in-memory navigation metadata only; no RestorableAgentSessionIndex, agent-store, transcript/JSONL, broad scan, or per-record syscall load was added...
Cmux Cache Substitution Correctness	✅ Passed	The Swift diff adds injected UserDefaults and semantic persistence guards; it does not replace an authoritative read with a cache, and NotificationsPage changes are syntax-only.
Cmux No Hacky Sleeps	✅ Passed	Changed TypeScript files add routing and analytics metadata only; sleep references are test fixtures or fake test scaffolding, and CI YAML waits are explicitly out of scope.
Cmux Swift Concurrency	✅ Passed	Changed Swift adds no new legacy async patterns; the existing stored MainActor watcher Task and main-queue hop remain, while NotificationsPage only updates onChange syntax.
Cmux Swift @Concurrent	✅ Passed	Changed Swift hunks add no @concurrent, nonisolated, or async constructs; the existing watcher task remains explicitly @MainActor, and no new heavy async helper call site appears.
Cmux Swiftpm Lockfiles	✅ Passed	cmux.xcodeproj/project.pbxproj only adds test-file references; no SwiftPM package-reference, Package.swift, .gitignore, or Package.resolved changes exist, and the root Xcode lockfile is unchanged.
Cmux Swift Logging	✅ Passed	The aggregate Swift diff adds no print, debugPrint, dump, NSLog, file/stdout diagnostics, or sensitive logging; the existing Logger is nonisolated private and unchanged.
Cmux User-Facing Error Privacy	✅ Passed	The production diff adds no user-facing error or alert copy; new navigation parse failures are debug-only, while CI and determinism messages are developer/test output.
Cmux Full Internationalization	✅ Passed	The production diff adds no user-facing Swift or web copy, localization keys, catalogs, or message entries; changes are IDs, settings, routing, analytics metadata, comments, tests, and CI.
Cmux Swiftui State Layout	✅ Passed	The only SwiftUI diff changes three existing NotificationsPage onChange closures to the modern syntax; no new state, GeometryReader, lazy-row store reference, or render-time mutation was added.
Cmux Architecture Rethink	✅ Passed	The Swift diff adds injected defaults/stores and semantic no-op persistence; it introduces no production sleeps, locks, delayed dispatch, or observers. Existing synchronization remains unchanged, a...
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	Swift changes cover settings persistence, injected stores, and NotificationsPage onChange; they add no standalone window or identifier assignment, and the auxiliary-window lint passes.
Cmux Source Artifacts	✅ Passed	All 23 changed paths are source, tests, scripts, configuration, or project metadata; no artifact-like directories, binary files, logs, screenshots, or build outputs appear in the diff.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	Changed production Swift adds no test/debug guards or seam-named members; added state is private and used by normal reload logic, while regression scaffolding remains in cmuxTests.
Cmux No Ambient Global State	✅ Passed	Changed Swift uses injected instance state and a private instance helper; diffs add no top-level API, mutable global, static-only namespace, or new singleton.
Out of Scope Changes check	✅ Passed	The settings, test, SwiftUI, and CI changes are explicitly included in the stated pull request objectives.
Title check	✅ Passed	The title clearly states the primary settings-load bug and its performance impact.
Description check	✅ Passed	The description clearly explains the change, rationale, affected behavior, and verification, but omits the template's Demo Video and Checklist sections.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot 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.

[greptile-apps]

Greptile Summary

This PR eliminates spurious UserDefaults writes that happened on every settings load — even when nothing changed — by guarding removals with an existence check and comparing encoded bytes before writing. Because UserDefaults.didChangeNotification observers with queue: .main run synchronously on the posting thread, each no-op write triggered hotkey re-registration and a full double-read of the action table, and created a re-entrancy risk during the store's own initialization.

• saveImportedManagedDefaults: checks key existence before removeObject for the legacy sidebar key and the managed-defaults key, and uses JSONEncoder().outputFormatting = .sortedKeys with a byte comparison against stored data before writing the encoded blob.
• saveBackups: applies the same existence-before-remove and sorted-keys byte-comparison pattern.
• The .sortedKeys option is load-bearing, not cosmetic: Swift's dictionary encoding order depends on the per-process hash seed, so without it the first reload after a relaunch could encode identical content to different bytes and trigger a spurious write.

Confidence Score: 5/5

Safe to merge. The change is a targeted guard around pre-existing write sites with no new code paths introduced.

Both modified functions follow the pattern already established in restoreUserDefaultsBackup in the same file. The existence check before removeObject is a direct Foundation correctness fix, and the sorted-keys byte comparison correctly handles the per-process hash seed instability of Swift dictionary encoding. There are no new write paths, no threading model changes, and the previously-failing test now passes.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
Sources/KeyboardShortcutSettingsFileStore.swift	Adds existence-guarded removals and sorted-keys byte-comparison writes to saveImportedManagedDefaults and saveBackups, eliminating spurious UserDefaults notifications on settings load with no logic regressions.

Sequence Diagram

sequenceDiagram
    participant FS as KeyboardShortcutSettingsFileStore
    participant UD as UserDefaults
    participant NC as NotificationCenter
    participant HC as SystemWideHotkeyController

    Note over FS: BEFORE — every settings load / file-watcher reload
    FS->>UD: removeObject(legacyKey) [key absent]
    UD->>NC: didChangeNotification (spurious)
    NC->>HC: observer fires synchronously on main thread
    HC->>UD: read ~200 keyboard actions
    FS->>UD: set(importedData) [value unchanged]
    UD->>NC: didChangeNotification (spurious)
    NC->>HC: observer fires again
    HC->>UD: read ~200 keyboard actions (second time)

    Note over FS: AFTER — this PR
    FS->>UD: object(forKey: legacyKey)?
    UD-->>FS: nil — skip removeObject
    FS->>UD: data(forKey: importedKey)?
    UD-->>FS: existing bytes
    FS->>FS: "encode(imported, sortedKeys) == existing bytes?"
    FS->>FS: yes — skip set()
    Note over NC,HC: No notification fired, no hotkey re-registration

Loading

_{Reviews (2): Last reviewed commit: "Compare encoded defaults with sorted key..." | Re-trigger Greptile}

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot 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.

[austinywang]

Regarding CodeRabbit’s Cmux Swift Package Boundaries check: I’m intentionally keeping this change in the existing CmuxSettingsFileStore owner. The PR tightens the store’s current persistence path; moving that store into Packages/macOS/CmuxSettings would also require extracting its file-watcher lifecycle, managed-settings models, socket password handling, app-side effects, and composition wiring. That is a broad, behavior-affecting package migration unrelated to the no-op write bug. The scoped fix keeps one persistence owner and does not add a parallel settings path; package extraction should be handled as a dedicated architectural change with its own validation.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot 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.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot 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.