Skip to content

Fix Canvas reorder offset overflow - #8141

Closed
austinywang wants to merge 14 commits into
mainfrom
issue-8041-reorder-shortcuts
Closed

austinywang wants to merge 14 commits into
mainfrom
issue-8041-reorder-shortcuts

Conversation

@austinywang

@austinywang austinywang commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #8080, which was merged while its test-only overflow regression commit was still waiting for a macOS package-test runner.

This clamps the relative offset before adding it to the current Canvas tab index, so public callers can pass Int.min or Int.max without trapping. The regression test merged with #8080 now passes.

Validation:

  • /usr/bin/arch -arm64 swift test in Packages/macOS/CmuxCanvasUI (45 tests, 6 suites)
  • git diff --check origin/main...HEAD

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Prevents overflow crashes when reordering Canvas tabs by bounding the requested move to the valid tab range. Supports issue 8041’s reorder shortcuts by making out-of-range moves safe no-ops.

  • Bug Fixes
    • In CmuxCanvasUI CanvasModel, compute allowable relative offset bounds from the current index and clamp the input offset before adding, so extreme values (e.g., Int.min/Int.max) don’t trap; the regression test now passes.

Written for commit 417e55d. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Improved tab reordering behavior by reliably constraining tabs to valid positions.
    • Prevented invalid movement requests from causing unintended layout changes.

@cursor

cursor Bot commented Jul 15, 2026

Copy link
Copy Markdown

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.

@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 9:12am
cmux-staging Building Building Preview, Comment Jul 15, 2026 9:12am

@coderabbitai

coderabbitai Bot commented Jul 15, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7fb603c4-2089-4b31-8d94-308242e0067c

📥 Commits

Reviewing files that changed from the base of the PR and between 610e1c0 and 417e55d.

📒 Files selected for processing (1)
  • Packages/macOS/CmuxCanvasUI/Sources/CmuxCanvasUI/CanvasModel.swift

📝 Walkthrough

Walkthrough

CanvasModel.reorderPanel(_:by:) now clamps relative reorder offsets using explicit bounds before calculating the destination index. Existing no-op handling and layout revision behavior remain unchanged.

Changes

Panel reordering

Layer / File(s) Summary
Offset clamping
Packages/macOS/CmuxCanvasUI/Sources/CmuxCanvasUI/CanvasModel.swift
reorderPanel(_:by:) computes minimum and maximum allowable offsets, clamps the requested offset, and derives the destination index from it.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Possibly related PRs

  • manaflow-ai/cmux#8080: Routes surface move shortcuts through canvasModel.reorderPanel, the behavior affected by this clamping change.

Suggested reviewers: azooz2003-bit

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: clamping Canvas reorder offsets to prevent overflow.
Description check ✅ Passed The description explains what changed, why, and how it was validated, though the demo video, review trigger, and checklist sections are missing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 CanvasModel is already @MainActor, and the diff only changes index-clamping math—no new isolation debt, shared mutable Sendable type, or background access was introduced.
Cmux Swift Blocking Runtime ✅ Passed Diff only rewrites reorderPanel index clamping arithmetic; no semaphores, sleeps, sync waits, timers, or locks were introduced.
Cmux Browser Automation Off-Main ✅ Passed Diff only changes CanvasModel.swift offset clamping; no browser automation routing, WebKit/AppKit wait, or policy-test files were touched.
Cmux Expensive Synchronous Load ✅ Passed CanvasModel.reorderPanel only changes offset clamping arithmetic; no agent-history loads, JSON parsing, or I/O were added to the @MainActor path.
Cmux Cache Substitution Correctness ✅ Passed Patch only rewrites reorderPanel offset clamping; it չի touch cache, snapshot, undo, or persistence substitution paths.
Cmux No Hacky Sleeps ✅ Passed Only a Swift file changed; the no-hacky-sleeps rule is out of scope, and the diff adds no sleeps/timers/waits.
Cmux Algorithmic Complexity ✅ Passed CanvasModel.reorderPanel only rewrites O(1) bound clamping; no new scans, sorts, filters, or hot-path collection growth.
Cmux Swift Concurrency ✅ Passed Diff only adjusts integer clamping in CanvasModel.reorderPanel; no Dispatch/Task/Combine/completion-handler patterns were added or expanded.
Cmux Swift @Concurrent ✅ Passed Only synchronous arithmetic changed inside existing @MainActor CanvasModel; no async, @concurrent, or actor-hop issues were introduced.
Cmux Swift Package Boundaries ✅ Passed PASS: The only change is CanvasModel.swift inside the CmuxCanvasUI SwiftPM package, which already has package tests; no app-target boundary violation.
Cmux Swiftpm Lockfiles ✅ Passed Only CanvasModel.swift changed; no SwiftPM/Xcode dependency, lockfile, or .gitignore edits are present to violate the lockfile rule.
Cmux Swift Logging ✅ Passed Diff only rewrites offset clamping in CanvasModel.reorderPanel; no print/debugPrint/dump/NSLog, Logger, or ad hoc logging was added or changed.
Cmux User-Facing Error Privacy ✅ Passed The patch only changes reorder offset clamping in CanvasModel.reorderPanel; it adds no user-facing errors, alerts, or copy.
Cmux Full Internationalization ✅ Passed Diff only changes CanvasModel index clamping; no user-facing Swift/UI text or locale catalogs/messages were added or modified.
Cmux Swiftui State Layout ✅ Passed Patch only clamps tab offsets in plain CanvasModel; no new ObservableObject/@published, GeometryReader, lazy-row store refs, or render-time state writes.
Cmux Architecture Rethink ✅ Passed Local CanvasModel correctness fix: it clamps the offset before index arithmetic, adds no new owners or timing paths, and preserves the model as single source of truth.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed Only CanvasModel.reorderPanel changed; no NSWindow/WindowGroup/NSPanel code or cmuxAuxiliaryWindowIdentifiers registration was introduced.
Cmux Source Artifacts ✅ Passed Only changed path is a hand-written Swift source file; no logs, caches, build output, or artifact directories are added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed CanvasModel.swift only changes offset clamping in reorderPanel; no #if DEBUG, test-only member, or debug/test seam was added.
Cmux No Ambient Global State ✅ Passed The PR only rewrites logic inside an existing instance method in CanvasModel; it adds no new top-level funcs, globals, namespaces, or singleton state.
✨ 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 issue-8041-reorder-shortcuts

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 an integer overflow in CanvasModel.reorderPanel(_:by:) that could trap when callers passed extreme offset values like Int.min or Int.max. The old code did currentIndex + offset before clamping, which overflows for large offsets; the fix clamps the offset to the valid range first ([startIndex − currentIndex, lastIndex − currentIndex]) and then adds it to currentIndex, keeping the destination provably in bounds.

  • Overflow fix: clamp the raw offset to [minimumOffset, maximumOffset] before arithmetic, so Int.min/Int.max offsets produce the first/last tab index instead of trapping.
  • Regression test: the test reorderPanelClampsExtremeOffsetsWithoutOverflow was merged with Add surface and workspace reorder shortcuts #8080 and now passes with this change.

Confidence Score: 5/5

Safe to merge — the change is a minimal arithmetic reorder in one function, the clamped bounds are provably valid given existing guards, and a regression test already covers Int.min/Int.max inputs.

The change replaces a single clamped-sum expression with a clamp-then-add idiom. The bounds (minimumOffset and maximumOffset) are derived from index arithmetic that is always safe given the surrounding guards (non-empty collection, currentIndex comes from firstIndex(of:)), so no new overflow or out-of-bounds path is introduced. The fix is narrow, the pre/post-removal insertion semantics are unchanged, and the existing regression test exercises the exact inputs the PR claims to fix.

No files require special attention.

Important Files Changed

Filename Overview
Packages/macOS/CmuxCanvasUI/Sources/CmuxCanvasUI/CanvasModel.swift Clamps the offset before adding to currentIndex in reorderPanel, preventing integer overflow for Int.min/Int.max inputs; logic is correct and no new issues introduced.

Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

This branch was successfully deployed

1 active deployment
Preview – cmux — 417e55df 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

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants