Skip to content

Fix two main breakages from the CI-disabled window: reorderPanel overflow, double shortcut notification - #8153

Merged
lawrencecchen merged 2 commits into
mainfrom
fix-reorder-overflow
Jul 15, 2026
Merged

lawrencecchen merged 2 commits into
mainfrom
fix-reorder-overflow

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Main has been red since today's window where the repo's CI workflows were manually disabled — two PRs merged unchecked and each broke a different job. Both fixes here:

  1. CanvasModel.reorderPanel arithmetic overflow (from Add surface and workspace reorder shortcuts #8080): currentIndex + offset trapped on Int.max/Int.min before the clamp, crashing the whole CmuxCanvasUI suite mid-run (swift-package-tests red). Fixed with a saturating addingReportingOverflow before the existing clamp. swift test --package-path Packages/macOS/CmuxCanvasUI: 45/45 with the summary printed.

  2. Double didChangeNotification on settings-file change (from Fix Settings shortcut display for legacy overrides #8091): notifyShortcutSettingsDidChange() called reload() (which posts when contents changed) then posted again unconditionally, so every real change notified listeners twice; changedSettingsFilePostsOneShortcutNotification fails on exactly this (app-host unit tests (4/4) red, three runs in a row here and on main's own CI). reload() now reports whether it posted and the action posts only when it didn't, keeping the exactly-once contract both Fix Settings shortcut display for legacy overrides #8091 tests encode.

Related flaky-test hardening tracked separately: #8159

Summary by CodeRabbit

  • Bug Fixes
    • Improved panel reordering so very large movement values no longer risk failures; the target position is safely constrained while preserving the existing reorder/selection behavior.
    • Fixed shortcut settings change updates to avoid duplicate notifications, ensuring listeners receive a single, consistent update when the settings file is reloaded.

reorderPanel(_:by:) computed currentIndex + offset before clamping, so
offset = Int.max / Int.min (exactly what
reorderPanelClampsExtremeOffsetsWithoutOverflow drives) trapped with a
Swift runtime arithmetic-overflow crash and killed the whole
CmuxCanvasUI test process before it printed its summary. Saturate via
addingReportingOverflow, then clamp as before.

This landed broken in #8080,
which merged while the repo's CI workflows were manually disabled, and
has broken swift-package-tests on main since.
@vercel

vercel Bot commented Jul 15, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jul 15, 2026 5:47pm
cmux-staging Building Building Preview, Comment Jul 15, 2026 5:47pm

@coderabbitai

coderabbitai Bot commented Jul 15, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Panel reordering now handles integer-overflowing offsets safely. Settings reloads report whether they emitted a change notification, allowing the host callback to avoid duplicate notifications.

Changes

Panel reordering

Layer / File(s) Summary
Overflow-safe destination calculation
Packages/macOS/CmuxCanvasUI/Sources/CmuxCanvasUI/CanvasModel.swift
reorderPanel(_:by:) uses overflow-reporting arithmetic, saturates the target, and preserves existing reorder behavior.

Settings notification coordination

Layer / File(s) Summary
Reload notification contract
Sources/KeyboardShortcutSettingsFileStore.swift
reload() returns whether relevant settings changed and a notification was emitted.
Conditional host notification
Sources/HostSettingsActions.swift
The host callback posts a notification only when reload reports no change.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: austinywang


Important

Pre-merge checks failed

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

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Cmux Swift Package Boundaries ❓ Inconclusive placeholder need repo evidence
✅ Passed checks (23 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed PASS: Changed APIs are already @MainActor UI code or a lock-protected store; the Bool-returning reload and overflow fix don't alter actor isolation.
Cmux Swift Blocking Runtime ✅ Passed PASS: The diff only changes shortcut reload notification flow and a Bool return; it adds no semaphores, sleeps, main-queue sync, polling, or new locks.
Cmux Browser Automation Off-Main ✅ Passed Diff only changes settings files; the browser-automation rule applies to TerminalController and ControlCommandExecutionPolicy, which this PR doesn't touch.
Cmux Expensive Synchronous Load ✅ Passed Only canvas overflow handling and keyboard-shortcut settings reload changed; no agent-history loader or large corpus parse was added/moved onto an interactive main-actor path.
Cmux Cache Substitution Correctness ✅ Passed The diff keeps a fresh disk reload in reload(); the new Bool only avoids double-posting notifications, so no cached/stale value is substituted.
Cmux No Hacky Sleeps ✅ Passed Only Swift files changed; no TypeScript/JS/shell/build/runtime script diff exists for the no-hacky-sleeps rule to apply.
Cmux Algorithmic Complexity ✅ Passed The PR only adds a saturated index calculation and a Bool-returning reload check; it introduces no nested scans, repeated filters, or scalable-collection slowdowns.
Cmux Swift Concurrency ✅ Passed Diff only changes synchronous notification/return-value logic; no new Dispatch queues, Combine state, completion handlers, or fire-and-forget Tasks were added.
Cmux Swift @Concurrent ✅ Passed Touched Swift code stays on @MainActor or uses existing actor hops; no new @concurrent misuse or main-actor-heavy async boundary issue appears in the diff.
Cmux Swiftpm Lockfiles ✅ Passed PR diff only changes three Swift source files; no .gitignore, Package.swift, Xcode project, or Package.resolved files are touched, so the lockfile policy isn't violated.
Cmux Swift Logging ✅ Passed No added print/debugPrint/dump/NSLog or new nonisolated Logger violations; the diffs only change overflow handling and notification flow.
Cmux User-Facing Error Privacy ✅ Passed PASS: The diff only changes internal overflow handling and notification flow; no user-facing errors, alerts, or recovery copy were added or exposed.
Cmux Full Internationalization ✅ Passed The PR only changes logic/comments in Swift files; it adds no new user-facing text or locale assets, and existing localized UI copy remains unchanged.
Cmux Swiftui State Layout ✅ Passed PASS: The PR only changes model/settings plumbing; it adds no new SwiftUI state/layout patterns, and HostSettingsActions is an allowed AppKit bridge.
Cmux Architecture Rethink ✅ Passed Small local correctness fixes: overflow is saturated in CanvasModel, and shortcut notifications are centralized without new timing, polling, or split ownership.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR only changes CanvasModel tab reordering and shortcut-settings reload/notification logic; it adds no new NSWindow/WindowGroup code or cmuxAuxiliaryWindowIdentifiers changes.
Cmux Source Artifacts ✅ Passed Only hand-written source files changed; no logs, temp dirs, build output, caches, or other artifact paths appear in the diff.
Cmux No Test Or Debug Seam In Production Source ✅ Passed Touched production files only adjust overflow handling and notification flow; no new #if DEBUG/test-only accessor, debug* seam, or wrapper exposing private state was added.
Cmux No Ambient Global State ✅ Passed Diff only changes instance methods on existing types; no new file-scope funcs, mutable globals, static-namespace types, or singletons were added.
Title check ✅ Passed Title clearly summarizes the two fixes in the PR and matches the changeset.
Description check ✅ Passed Summary and testing are present, but the template's demo video, review trigger, and checklist sections are missing.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-reorder-overflow

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

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

@greptile-apps

greptile-apps Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes two unrelated production bugs: a Swift arithmetic overflow crash in CanvasModel.reorderPanel(_:by:) triggered by extreme offsets (Int.max/Int.min), and a double-notification defect in notifyShortcutSettingsDidChange() that fired didChangeNotification twice on every shortcut file change.

  • CanvasModel.swift: uses addingReportingOverflow to saturate before the existing clamp, safely handling Int.max/Int.min offsets that previously caused a Swift runtime trap and killed the CmuxCanvasUI test process.
  • KeyboardShortcutSettingsFileStore.swift: reload() now returns a Bool indicating whether it already posted didChangeNotification, and @discardableResult keeps all existing call sites warning-free.
  • HostSettingsActions.swift: notifyShortcutSettingsDidChange() only posts the notification when reload() did not, guaranteeing exactly one notification per call instead of two.

Confidence Score: 5/5

Both fixes are targeted, self-contained, and correct; no regressions introduced.

The overflow fix uses the standard Swift saturating-addition idiom and is provably safe for all index and offset values. The double-notification fix correctly inverts the boolean gate so exactly one notification fires per call in all branches (changed or unchanged), matching the original intent. No concurrency, actor-isolation, or architectural concerns were introduced.

No files require special attention.

Important Files Changed

Filename Overview
Packages/macOS/CmuxCanvasUI/Sources/CmuxCanvasUI/CanvasModel.swift Saturates currentIndex + offset with addingReportingOverflow before clamping, eliminating the Swift runtime arithmetic overflow crash on Int.max/Int.min offsets. Logic is correct for all index and offset values.
Sources/KeyboardShortcutSettingsFileStore.swift reload() now returns Bool indicating whether it posted didChangeNotification; @discardableResult preserves call-site backward compatibility. Change-detection gate (shortcuts / whenClauses / sourcePath) is unchanged.
Sources/HostSettingsActions.swift notifyShortcutSettingsDidChange() now conditionally posts the notification only when reload() did not, eliminating the double-notification on every shortcut settings change.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant HostSettingsActions
    participant CmuxSettingsFileStore
    participant NotificationCenter

    Note over Caller,NotificationCenter: After fix — exactly one notification per call
    Caller->>HostSettingsActions: notifyShortcutSettingsDidChange()
    HostSettingsActions->>CmuxSettingsFileStore: reload() → Bool
    alt shortcuts / path changed
        CmuxSettingsFileStore->>NotificationCenter: notifySettingsFileDidChange() [inside reload]
        CmuxSettingsFileStore-->>HostSettingsActions: true
        Note over HostSettingsActions: skips extra post
    else no shortcut change
        CmuxSettingsFileStore-->>HostSettingsActions: false
        HostSettingsActions->>NotificationCenter: notifySettingsFileDidChange()
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant HostSettingsActions
    participant CmuxSettingsFileStore
    participant NotificationCenter

    Note over Caller,NotificationCenter: After fix — exactly one notification per call
    Caller->>HostSettingsActions: notifyShortcutSettingsDidChange()
    HostSettingsActions->>CmuxSettingsFileStore: reload() → Bool
    alt shortcuts / path changed
        CmuxSettingsFileStore->>NotificationCenter: notifySettingsFileDidChange() [inside reload]
        CmuxSettingsFileStore-->>HostSettingsActions: true
        Note over HostSettingsActions: skips extra post
    else no shortcut change
        CmuxSettingsFileStore-->>HostSettingsActions: false
        HostSettingsActions->>NotificationCenter: notifySettingsFileDidChange()
    end
Loading

Reviews (2): Last reviewed commit: "Fix double didChangeNotification from no..." | Re-trigger Greptile

notifyShortcutSettingsDidChange() called settingsFileStore.reload(),
which already posts didChangeNotification when the file's contents
changed, then unconditionally posted the same notification again —
every real settings-file change notified listeners twice, and
changedSettingsFilePostsOneShortcutNotification (added with the same
PR, #8091) fails on it in CI
(app-host shard 4/4, red on main).

reload() now reports whether it posted; the action only posts when the
reload saw no change, preserving the exactly-once contract both tests
encode.
@lawrencecchen lawrencecchen changed the title Fix arithmetic overflow crash in CanvasModel.reorderPanel Fix two main breakages from the CI-disabled window: reorderPanel overflow, double shortcut notification Jul 15, 2026
@lawrencecchen
lawrencecchen merged commit db114aa into main Jul 15, 2026
27 of 29 checks passed
@lawrencecchen
lawrencecchen deleted the fix-reorder-overflow branch July 15, 2026 09:46
@lawrencecchen
lawrencecchen restored the fix-reorder-overflow branch July 18, 2026 10:19

This branch was successfully deployed

1 active deployment
Preview – cmux — bcfc5df6 Deployed Jul 15, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant