Repository navigation
Unbreak main CI: canvas reorder overflow + duplicate shortcut notification - #8146
Closed
azooz2003-bit wants to merge 3 commits into
Closed
azooz2003-bit wants to merge 3 commits into
azooz2003-bit wants to merge 3 commits into
Conversation
reorderPanel computed currentIndex + offset before clamping, which traps on Int.max/.min offsets. The existing regression test reorderPanelClampsExtremeOffsetsWithoutOverflow (landed with #8080 while main CI was paused) crashes the whole CmuxCanvasUI test process with SIGTRAP, failing swift-package-tests for every PR. Saturate the addition before clamping to the valid index range. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
ChangesBehavior safety updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
3 of 4 tasks
Contributor
Greptile SummaryThis PR fixes two independent bugs: a SIGTRAP crash in
Confidence Score: 5/5Both fixes are narrow, targeted, and well-scoped; the crashing regression test already on main validates the overflow fix, and the double-notification change preserves exactly-once semantics in all cases. The overflow saturation logic is correct for all input combinations (positive/negative overflow, empty-pane guard in place). The Bool-returning reload() change eliminates a double-post without dropping any notification that should fire. No rule violations found across all three changed files. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["reorderPanel(_:by:)"] --> B{guard: panel in pane,\npanelIds non-empty}
B -- false --> C[return false]
B -- true --> D["addingReportingOverflow(offset)"]
D --> E{overflowed?}
E -- yes, offset > 0 --> F[target = Int.max]
E -- yes, offset ≤ 0 --> G[target = Int.min]
E -- no --> H[target = sum]
F & G & H --> I["destinationIndex = clamp(target, startIndex…lastIndex)"]
I --> J{destinationIndex\n== currentIndex?}
J -- yes --> K[return true, no-op]
J -- no --> L[removePanel → addPanel at destinationIndex]
L --> M[revision &+= 1\nreturn true]
%%{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"}}}%%
flowchart TD
A["reorderPanel(_:by:)"] --> B{guard: panel in pane,\npanelIds non-empty}
B -- false --> C[return false]
B -- true --> D["addingReportingOverflow(offset)"]
D --> E{overflowed?}
E -- yes, offset > 0 --> F[target = Int.max]
E -- yes, offset ≤ 0 --> G[target = Int.min]
E -- no --> H[target = sum]
F & G & H --> I["destinationIndex = clamp(target, startIndex…lastIndex)"]
I --> J{destinationIndex\n== currentIndex?}
J -- yes --> K[return true, no-op]
J -- no --> L[removePanel → addPanel at destinationIndex]
L --> M[revision &+= 1\nreturn true]
Reviews (3): Last reviewed commit: "Fix duplicate shortcut change notificati..." | Re-trigger Greptile |
HostSettingsActions.notifyShortcutSettingsDidChange posted didChangeNotification unconditionally after reload() had already posted it for an observed change, so a changed cmux.json produced two posts. The HostSettingsShortcutNotificationTests added with #8091 expect exactly one post for both changed and unchanged files; the changed case fails app-host unit tests (4/4) on every PR merge ref. reload() now reports whether it notified and the action only posts when it did not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/KeyboardShortcutSettingsFileStore.swift (1)
187-193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused tests for the exactly-once notification contract.
Test that a changed reload returns
trueand emits one notification, while an unchanged reload returnsfalse; also coverHostSettingsActions.notifyShortcutSettingsDidChange()forwarding exactly one notification in both cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/KeyboardShortcutSettingsFileStore.swift` around lines 187 - 193, add focused tests around the reload comparison logic in KeyboardShortcutSettingsFileStore, verifying changed reloads return true and emit exactly one notification while unchanged reloads return false. Also test HostSettingsActions.notifyShortcutSettingsDidChange() to confirm it forwards exactly one notification for both changed and unchanged cases, using notification counts and return values to enforce the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 187-193: add focused tests around the reload comparison logic in
KeyboardShortcutSettingsFileStore, verifying changed reloads return true and
emit exactly one notification while unchanged reloads return false. Also test
HostSettingsActions.notifyShortcutSettingsDidChange() to confirm it forwards
exactly one notification for both changed and unchanged cases, using
notification counts and return values to enforce the contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e910ed6c-3b12-4ced-abcd-d6033d426d8e
📒 Files selected for processing (2)
Sources/HostSettingsActions.swiftSources/KeyboardShortcutSettingsFileStore.swift
azooz2003-bit
changed the title
Collaborator
Author
|
Superseded: both fixes landed on main independently — the reorder overflow via #8153 and the duplicate shortcut notification via bcfc5df, with functionally identical implementations (saturating add before clamp; reload() reporting whether it notified). This PR's CI did go fully green at b2725e1, independently confirming both fixes. Closing; branch left in place. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two regressions landed on main while its CI runs were cancelled during the CI/CD pause; both fail required checks on every PR via the
pull_requestmerge ref. This PR fixes both.1.
swift-package-tests: SIGTRAP in CmuxCanvasUI — #8080 computescurrentIndex + offsetinCanvasModel.reorderPanelbefore clamping, which traps on theInt.max/Int.minoffsets its own regression test passes. Fixed by saturating the addition (addingReportingOverflow) before clamping. Verified:swift test --package-path Packages/macOS/CmuxCanvasUIgoes from SIGTRAP to 45/45 exit 0 locally, andswift-package-testspassed on this PR's earlier run at e860e84.2.
app-host unit tests (4/4): duplicate shortcut notification —HostSettingsActions.notifyShortcutSettingsDidChangepostsdidChangeNotificationunconditionally afterreload()already posted for an observed change, so a changed cmux.json posts twice.HostSettingsShortcutNotificationTests(added with #8091) expects exactly one post for changed and unchanged files. Fixed by havingreload()report whether it notified; the action posts only when it did not.No new tests: both defects are pinned by regression tests already on main (
reorderPanelClampsExtremeOffsetsWithoutOverflow,HostSettingsShortcutNotificationTests) that go red→green with these fixes. Example victim of both: #7670.🤖 Generated with Claude Code