Skip to content

Allow configurable main window minimum height - #12991

Open
austinywang wants to merge 1 commit into
mainfrom
task-12944-configurable-window-height
Open

austinywang wants to merge 1 commit into
mainfrom
task-12944-configurable-window-height

Conversation

@austinywang

@austinywang austinywang commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Fixes #12944.

The main window minimum height is now resolved from the app.minimumWindowHeight setting (stored as minimumWindowHeight). The default remains 400 points, while values from 120 through 400 support compact tiling. All frame, restore, rescue, and content sizing paths use the same resolved policy.

Validation: added behavioral coverage for defaulting and lower/upper bounds; git diff --check passes.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes #12944 by replacing the hard-coded 400-point minimum window height with the configurable app.minimumWindowHeight setting (stored as minimumWindowHeight). The default remains 400, and values from 120 through 400 are accepted and clamped; all frame, restore, rescue, and content sizing paths now use the same resolved minimum.

Written for commit 1adbd08. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added a configurable minimum window height setting, defaulting to 400 points.
    • Window sizing now respects the saved setting and clamps values to a supported range of 120–400 points.
  • Bug Fixes

    • Improved window frame validation and screen-change handling to consistently apply the configured minimum height.
  • Tests

    • Added coverage for default, configured, and boundary minimum-height values.

@github-actions

Copy link
Copy Markdown

Note

Pull Request opener @austinywang is not an author or co-author of any commit in this PR (commit identities: lawrencecchen). The CLA check will still proceed and requires every listed identity plus @austinywang to have signed.

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1804642a-95f8-4b39-a3d6-409e42ad6700

📥 Commits

Reviewing files that changed from the base of the PR and between 9c2ba78 and 1adbd08.

📒 Files selected for processing (8)
  • Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/AppCatalogSection.swift
  • Sources/App/CmuxMainWindow.swift
  • Sources/App/MainWindowVisibleFrameFitRescue.swift
  • Sources/AppDelegate+WindowFramePolicy.swift
  • Sources/AppDelegate.swift
  • Sources/ContentView.swift
  • Sources/SessionPersistence.swift
  • cmuxTests/SessionPersistenceTests.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The PR adds a configurable, persisted minimum window height with a default of 400 points and a valid range of 120–400 points. Window sizing and frame validation now use the resolved value.

Changes

Configurable window height

Layer / File(s) Summary
Minimum height policy and setting
Packages/macOS/CmuxSettings/.../AppCatalogSection.swift, Sources/SessionPersistence.swift, cmuxTests/SessionPersistenceTests.swift
The settings catalog defines app.minimumWindowHeight. SessionPersistencePolicy resolves missing, invalid, and out-of-range values. Tests cover default, configured, and clamped values.
Window sizing integration
Sources/App/CmuxMainWindow.swift, Sources/App/MainWindowVisibleFrameFitRescue.swift, Sources/AppDelegate+WindowFramePolicy.swift, Sources/AppDelegate.swift, Sources/ContentView.swift
Window sizing, frame fitting, screen-change reconciliation, validation, and minimum-height constraints use the resolved policy value.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant UserDefaults
  participant SessionPersistencePolicy
  participant WindowSizing
  UserDefaults->>SessionPersistencePolicy: Store or provide minimumWindowHeight
  SessionPersistencePolicy->>SessionPersistencePolicy: Apply default or clamp value
  SessionPersistencePolicy-->>WindowSizing: Return resolved minimum height
  WindowSizing->>WindowSizing: Apply height to sizing and frame validation
Loading

Suggested reviewers: lawrencecchen


Important

Pre-merge checks failed

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

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Cmux Swift Package Boundaries ❌ Error The diff introduces pure, independently testable window-policy logic in the app target at Sources/SessionPersistence.swift. SessionPersistencePolicy.resolvedMinimumWindowHeight(defaults:) only use… Move the minimum-height policy (minimumWindowHeightKey, default, range, and resolution/clamping behavior) behind a small SwiftPM boundary. Use the existing CmuxWindowing target or a dedicated CmuxWindowPolicy target, with a public typ…
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (23 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: making the main window minimum height configurable.
Description check ✅ Passed The description clearly explains what changed, why it changed, and how it was tested. It omits the template headings, demo video, and checklist, but it remains mostly complete and relevant.
Linked Issues check ✅ Passed Issue #12944 requires a configurable main-window minimum height while preserving the 400-point default. The PR adds app.minimumWindowHeight, stores it as minimumWindowHeight, resolves values in th…
Out of Scope Changes check ✅ Passed The changed files add the setting, centralize its resolution, update all reported window-height enforcement paths, and add focused automated tests. These changes directly support issue #12944. No unre…
Cmux Cloud Persistent Session And Early Input ✅ Passed PASS. The review-scoped diff changes only the app minimum window-height setting, window-frame sizing paths, and related tests. It does not change Cloud terminal creation, cmux-tui clients, transports,…
Cmux Swift Actor Isolation ✅ Passed PASS: The production diff adds a DefaultsKey<Double> to the existing AppCatalogSection and a UserDefaults-backed resolver on SessionPersistencePolicy. AppCatalogSection already conforms to `…
Cmux Swift Blocking Runtime ✅ Passed PASS. The pull-request diff adds a UserDefaults resolver and replaces fixed minimum-height reads. It does not add or expand semaphores, blocking waits, sleeps, delayed dispatch, polling, main-queue sy…
Cmux Browser Automation Off-Main ✅ Passed PASS: The pull request changes only configurable window-height settings and related frame sizing. The scoped browser automation files, Sources/TerminalController.swift and `ControlCommandExecutionPo…
Cmux Expensive Synchronous Load ✅ Passed PASS. The pull request adds only a scalar UserDefaults lookup and numeric clamping in SessionPersistencePolicy.resolvedMinimumWindowHeight(). The changed UI, window, and frame-policy paths call th…
Cmux Cache Substitution Correctness ✅ Passed PASS — The diff does not substitute a cached value for a fresh persistence, history, undo, or snapshot read. It replaces the fixed 400-point constant with `SessionPersistencePolicy.resolvedMinimumWind…
Cmux No Hacky Sleeps ✅ Passed PASS. The authoritative diff changes only Swift source and Swift test files. The custom rule applies to TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. No covered file or fixed-del…
Cmux Algorithmic Complexity ✅ Passed PASS: The production diff adds one scalar UserDefaults lookup with finite-value validation and constant-time min/max clamping. The other production changes replace a fixed scalar height read wit…
Cmux Swift Concurrency ✅ Passed The authoritative diff adds a synchronous SessionPersistencePolicy.resolvedMinimumWindowHeight(defaults:) method, a DefaultsKey, synchronous minimum-height substitutions, and XCTest assertions. Ad…
Cmux Swift @Concurrent ✅ Passed PASS: The pull request introduces no async, @concurrent, await, or Task changes. resolvedMinimumWindowHeight is a synchronous static func that reads UserDefaults and clamps a value. The …
Cmux Swiftpm Lockfiles ✅ Passed PASS. The authoritative PR diff contains only Swift source and test changes. It does not modify any Package.swift, Package.resolved, .gitignore, workflow, or Xcode project/workspace file. The `P…
Cmux Swift Logging ✅ Passed PASS. The pull request adds a setting and minimum-height resolution logic, plus replaces static height references in window-sizing paths. The authoritative diff adds no print, debugPrint, dump, …
Cmux User-Facing Error Privacy ✅ Passed PASS — The PR adds a configurable window-height setting and updates frame-sizing policy. The production diff adds no user-facing errors, alerts, command output, API error bodies, or recovery copy. The…
Cmux Full Internationalization ✅ Passed PASS. The pull request adds a DefaultsKey<Double> and the literal minimumWindowHeight configuration key, plus runtime sizing logic and tests. It does not add or change user-facing Swift text, stri…
Cmux Swiftui State Layout ✅ Passed PASS: The PR adds no new SwiftUI state, ObservableObject, @Published, property-wrapper store references, GeometryReader, lazy/list row subtree, or render-time state mutation. The only SwiftUI ch…
Cmux Architecture Rethink ✅ Passed PASS. The diff adds a small policy-resolution method in SessionPersistencePolicy and routes the existing window sizing, restore, rescue, and frame paths through that shared source. It adds no sleeps…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR changes only the minimum-height policy for the existing CmuxMainWindow and its frame-fit, restore, rescue, and SwiftUI content paths. It adds no NSPanel, NSWindowController, standalone Sw…
Cmux Source Artifacts ✅ Passed All 8 changed paths are intentional Swift source or test files. The authoritative diff contains only modifications to existing .swift blobs, with no local logs, media, temp or scratch directories, c…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The PR adds no test/debug seam in production source. Added production members are the real app.minimumWindowHeight setting and SessionPersistencePolicy.resolvedMinimumWindowHeight(defaults:)
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. (2 skipped: 2 too large.)

Full details: Cmux Swift Package Boundaries

Explanation

The diff introduces pure, independently testable window-policy logic in the app target at Sources/SessionPersistence.swift. SessionPersistencePolicy.resolvedMinimumWindowHeight(defaults:) only uses Foundation.UserDefaults/NSNumber, defines a stable default and range, and receives injectable defaults. The new test exercises it without AppKit or app lifecycle. This matches the rule's failure condition for core logic in the root Sources/ path. The changed AppKit/UI call sites are glue and are not the issue. The setting declaration itself is correctly placed in the existing CmuxSettings SwiftPM target.

Resolution

Move the minimum-height policy (minimumWindowHeightKey, default, range, and resolution/clamping behavior) behind a small SwiftPM boundary. Use the existing CmuxWindowing target or a dedicated CmuxWindowPolicy target, with a public type such as MainWindowMinimumHeightPolicy exposing the resolved value API. Keep AppCatalogSection.minimumWindowHeight in CmuxSettings, have the package policy read that setting or accept its stored value, update the app call sites to use the package API, and move the default/clamping tests into the package test target. Remove the newly introduced policy implementation from Sources/SessionPersistence.swift.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

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.

Hard 400 pt minimum window height breaks compact tiling; allow a configurable minimum

2 participants