Repository navigation
Enforce a 400pt main-window minimum height on every resize path - #11321
Conversation
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change enforces minimum dimensions for non-fullscreen windows, aligns startup and session persistence limits with those dimensions, and adds coverage for frame adjustment and edge preservation. ChangesWindow sizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change enforces a consistent 400pt minimum window height across interactive, restored, and programmatic resizing paths, preventing layout clipping and overlap. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
Full details: Cmux Swift Actor IsolationExplanation PASS. The production diff adds no actor-isolation mistake. Full details: Cmux Swift Blocking RuntimeExplanation The full PR diff adds only synchronous frame-sizing logic, a minimum-height constant, a derived window-size value, and deterministic test assertions. It adds no semaphore or blocking wait, sleep, delayed dispatch, timer, polling loop, main-queue sync, or manual lock in production Swift. Existing timing and dispatch code in AppDelegate is unchanged by this PR. The new synchronization-related test code is deterministic scaffolding, which the rule allows. Full details: Cmux Browser Automation Off-MainExplanation PASS: The PR changes only Full details: Cmux Expensive Synchronous LoadExplanation PASS: The production diff only changes window frame clamping, the new-window minimum height source, and Full details: Cmux Cache Substitution CorrectnessExplanation PASS: The PR changes window sizing only. The production diff adds frame clamping, changes the shared minimum-height constant, and derives cascade sizing from that constant. It does not replace a fresh authoritative read with a cached value in a persistence, history, undo, or snapshot path. No changed TypeScript or JavaScript files exist, and the added use of Full details: Cmux No Hacky SleepsExplanation PASS: The PR changes only Swift production files and a Swift test file. The merge-base diff contains no TypeScript, JavaScript, shell, or non-Swift build/runtime files. The changed lines introduce no fixed sleeps, timers, polling, delayed dispatch, or wall-clock waits. The check is therefore inapplicable. Full details: Cmux Algorithmic ComplexityExplanation PASS. The production diff adds only constant-time frame-size arithmetic, comparisons, and edge adjustments in Full details: Cmux Swift ConcurrencyExplanation PASS: The pull request changes only synchronous AppKit frame handling, a shared sizing constant, and XCTest coverage. The added Swift lines introduce no Dispatch queues/groups, Combine state, completion-handler APIs, or fire-and-forget Tasks. The Full details: Cmux Swift `@Concurrent`Explanation PASS: The PR adds no async function or async call site. The only new Full details: Cmux Swift Package BoundariesExplanation PASS. The production diff keeps the new behavior in ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/MainWindowSelfSizingTests.swift">
<violation number="1" location="cmuxTests/MainWindowSelfSizingTests.swift:199">
P3: These new tests in a non-UI cmuxTests file are added as XCTestCase methods, but the repo's Swift Testing policy prefers @Suite/@Test for new non-UI tests even inside existing XCTestCase files, reserving XCTest for UI-interaction suites. Express them as a new @Suite (kept separate from the existing XCTestCase class) instead.</violation>
<violation number="2" location="cmuxTests/MainWindowSelfSizingTests.swift:213">
P3: The regression test undersizes both width (220 < 300) and height (120 < 400), so both dims raise in one call; test 2 covers both-above passthrough. The PR's specific path — height-undersized, width-fitting frame where height is raised to the floor and the width passes through unchanged with the top edge anchored — is left untested. Add a case with width >= minimum (e.g. width 900, height 120) asserting only the y/origin and height change while width stays byte-for-byte.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| let minimum = CmuxMainWindow.minimumContentSize | ||
| let undersized = NSRect(x: 80, y: 500, width: 220, height: 120) |
There was a problem hiding this comment.
P3: The regression test undersizes both width (220 < 300) and height (120 < 400), so both dims raise in one call; test 2 covers both-above passthrough. The PR's specific path — height-undersized, width-fitting frame where height is raised to the floor and the width passes through unchanged with the top edge anchored — is left untested. Add a case with width >= minimum (e.g. width 900, height 120) asserting only the y/origin and height change while width stays byte-for-byte.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/MainWindowSelfSizingTests.swift, line 213:
<comment>The regression test undersizes both width (220 < 300) and height (120 < 400), so both dims raise in one call; test 2 covers both-above passthrough. The PR's specific path — height-undersized, width-fitting frame where height is raised to the floor and the width passes through unchanged with the top edge anchored — is left untested. Add a case with width >= minimum (e.g. width 900, height 120) asserting only the y/origin and height change while width stays byte-for-byte.</comment>
<file context>
@@ -188,4 +188,57 @@ final class MainWindowSelfSizingTests: XCTestCase {
+ }
+
+ let minimum = CmuxMainWindow.minimumContentSize
+ let undersized = NSRect(x: 80, y: 500, width: 220, height: 120)
+ window.setFrame(undersized, display: false)
+
</file context>
There was a problem hiding this comment.
The height-only-undersized path with a fitting width is covered by testFrameRaisePinsKeptBottomEdgeDuringTopEdgeDrag and testFrameRaisePinsKeptTopEdgeDuringBottomEdgeDrag (width 1000 ≥ 300, height raised alone), added in e4db0fd275.
| /// enforce the floor on every setFrame path, keeping the top edge (the | ||
| /// titlebar the user can grab) where the caller put it. | ||
| @MainActor | ||
| func testSetFrameRaisesUndersizedFrameToMinimumContentSize() { |
There was a problem hiding this comment.
P3: These new tests in a non-UI cmuxTests file are added as XCTestCase methods, but the repo's Swift Testing policy prefers @Suite/@test for new non-UI tests even inside existing XCTestCase files, reserving XCTest for UI-interaction suites. Express them as a new @suite (kept separate from the existing XCTestCase class) instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/MainWindowSelfSizingTests.swift, line 199:
<comment>These new tests in a non-UI cmuxTests file are added as XCTestCase methods, but the repo's Swift Testing policy prefers @Suite/@Test for new non-UI tests even inside existing XCTestCase files, reserving XCTest for UI-interaction suites. Express them as a new @Suite (kept separate from the existing XCTestCase class) instead.</comment>
<file context>
@@ -188,4 +188,57 @@ final class MainWindowSelfSizingTests: XCTestCase {
+ /// enforce the floor on every setFrame path, keeping the top edge (the
+ /// titlebar the user can grab) where the caller put it.
+ @MainActor
+ func testSetFrameRaisesUndersizedFrameToMinimumContentSize() {
+ let window = CmuxMainWindow(
+ contentRect: NSRect(x: 0, y: 0, width: 900, height: 600),
</file context>
There was a problem hiding this comment.
Kept as XCTest methods for idiom consistency with the surrounding MainWindowSelfSizingTests class; the pure-function cases sit beside the window-lifecycle behavior they pin.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
…t clamp CmuxMainWindow.minSize only bounds user resizes; programmatic setFrame can still shrink the window below the layout floor, where the sidebar footer, update pill, and tab bar overlap and clip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Below ~400pt the chrome cannot lay out without overlap: the sidebar footer (account/help/update pill) collides with workspace rows and the update pill clips at the window edge. Raise the policy floor from 200pt and make CmuxMainWindow.setFrame raise undersized programmatic frames to the floor (top edge anchored), mirroring the existing oversized cap; minSize already stops user drags there. The new-window cascade clamp now derives its height from the same policy instead of a stale 360pt literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Preflight on macOS 26 showed edge drags deliver below-minSize frames straight through setFrame, so the floor clamp is the only guard during a live drag. Anchoring the raise to the proposal's top edge made a top-edge drag slide the whole window down with the cursor once it hit the floor. Infer the kept edge by comparing the proposal against the current frame and pin that edge (bottom for top-edge drags, top for bottom-edge drags, right for left-edge drags); programmatic shrinks that move both edges keep the titlebar's top-left corner as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
96e9421 to
e4db0fd
Compare
A programmatic setFrame that happens to share the window's current bottom-left origin is not a top-edge drag; only infer the held edge while inLiveResize, and keep the titlebar's top-left corner for every programmatic raise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
717f357 Fix concurrent tmux-compatible set-buffer writes (manaflow-ai#11291) 71d7498 Fix dashboard tab loading flashes (manaflow-ai#11363) 3eb96d8 iOS onboarding: offer Keep Mac Awake once the Mac connects (manaflow-ai#11334) 9f14777 cloud-vm smoke: --paid marks the smoke user as a pro plan (manaflow-ai#11375) 983cccb fix(tests): cmuxTests compiles on main again (two missing lines from manaflow-ai#11059 and manaflow-ai#11345) (manaflow-ai#11346) 9bbf2fe Device-list authorization + DO-first control plane (listauth) (manaflow-ai#11220) d2a1ac9 vm: gate Cloud VM provisioning behind paid plans (manaflow-ai#11332) 61d0a51 Enforce a 400pt main-window minimum height on every resize path (manaflow-ai#11321) 4c5cb45 Glue terminal frames to live window resize ticks (manaflow-ai#11323) 28b71b2 Cloud tree agent parity: combined follow-ups (manaflow-ai#11300 + manaflow-ai#11301) + one-click workspace rows (manaflow-ai#11345) c15815b cloud images: minimal shell highlighting, restore devbox bashrc lockstep (manaflow-ai#11343) bcd3b7b feat: add supported iOS localizations (manaflow-ai#11315) 3523369 test: pin the plan gates for team-less accounts (user-scoped billing must not widen access) (manaflow-ai#11311) 006a4ee blaxel image: bump agent and cua-driver pins to latest (manaflow-ai#11293) 268ca9d web: billing recovery — email-claim redemption, portal for lapsed subscribers, dunning (manaflow-ai#11313)
This branch was successfully deployed
1 participant
Resizing the main window very short overflowed the chrome in an ugly way: the sidebar footer (account/help/update pill) collided with workspace rows, the update pill clipped at the window edge, and terminal content spilled over the titlebar during the drag.
Two mechanisms, one floor:
SessionPersistencePolicy.minimumWindowHeightgoes from 200pt to 400pt.minSize/contentMinSize, the SwiftUI root.frame(minHeight:), restored-frame validation, display-reconfiguration clamps, and the visible-frame fit rescue all already derive from this constant, so user drag resizes now stop at 400pt everywhere.CmuxMainWindow.setFramenow raises undersized programmatic frames tominimumContentSize(top edge anchored, mirroring the existing oversized cap), because AppKit'sminSizebounds only user resizes; session-restore math, display reconfiguration, and automation could still deliver a below-floor frame.The new-window cascade clamp in
positionNewMainWindowderives its height from the same policy instead of a stale 360pt literal.Commit 1 adds the regression test only (red), commit 2 the fix (green): a programmatic
setFramebelow the floor must clamp, keeping the top edge put.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Enforces a 400pt main-window minimum height on every resize path so the sidebar footer, update pill, and tab bar no longer overlap or clip when the window is short.
Bug Fixes
minSizeandcontentMinSizealready stop user drags, andCmuxMainWindow.setFramenow raises undersized programmatic frames to the floor (session restore, display reconfiguration, automation).Written for commit cddb0dc. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests