Skip to content

Clamp Settings window away from display edge - #3436

Merged
lawrencecchen merged 2 commits into
mainfrom
feat-settings-traffic-lights-offset
May 2, 2026
Merged

lawrencecchen merged 2 commits into
mainfrom
feat-settings-traffic-lights-offset

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented May 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • clamp the Settings window to the visible display with an 18 px inset when it is configured or focused
  • disable Settings window restoration so stale edge placements do not reopen flush against the display
  • remove the dead traffic-light reframe path that was still caching and reapplying button frames with a zero offset

Verification

  • ./scripts/reload.sh --tag setwin
  • Reproduced on cmux-macmini by forcing Settings to X=0,Y=30, then verified the patched setmm build moves it to X=18,Y=48 when Settings is opened again

Summary by cubic

Clamp the Settings window to the visible screen with an 18px inset and disable restoration to prevent reopening flush against the display. Also remove the unused traffic-light reframe path that was reapplying zero-offset button frames.

  • Bug Fixes

    • Clamp on configure and when showing to keep the window inside the screen’s visible area.
    • Set isRestorable = false to avoid restoring stale edge positions.
  • Refactors

    • Remove traffic-light offset logic and cached base frames from WindowDecorationsController.

Written for commit 7c1da75. Summary will update on new commits.

Summary by CodeRabbit

Bug Fixes

  • Settings window now clamps to and remains within the visible screen area when opened or refocused, preventing it from appearing off-screen.
  • Window decoration (traffic-light) buttons no longer shift position unexpectedly; their visibility is updated without repositioning, keeping window controls stable.

@vercel

vercel Bot commented May 2, 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 May 2, 2026 10:20am
cmux-staging Building Building Preview, Comment May 2, 2026 10:20am

@coderabbitai

coderabbitai Bot commented May 2, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 554f041d-1f00-4715-b284-77f7f3c45cde

📥 Commits

Reviewing files that changed from the base of the PR and between 5319a72 and 7c1da75.

📒 Files selected for processing (1)
  • Sources/cmuxApp.swift

📝 Walkthrough

Walkthrough

Removes per-window traffic-light button repositioning and related state; adds clamping to keep the Settings window origin inside the active screen's visibleFrame with an 18pt inset, applied during configure and focus.

Changes

Traffic-Light Button Repositioning Cleanup

Layer / File(s) Summary
State Removal
Sources/WindowDecorationsController.swift
Removed trafficLightBaseFrames dictionary tracking per-window button base frames.
Implementation Cleanup
Sources/WindowDecorationsController.swift
Deleted applyTrafficLightOffset(...), applyTrafficLightOffsetNow(...), and trafficLightOffset(for:) stub.
Behavior Update
Sources/WindowDecorationsController.swift
apply(to:) no longer applies positional offsets; it only calls hideStandardButtons(...) to update visibility.

Settings Window Visible Area Clamping

Layer / File(s) Summary
Configuration
Sources/cmuxApp.swift
Added visibleAreaInset: CGFloat = 18 constant to define padding from screen visible bounds.
Window Setup
Sources/cmuxApp.swift
SettingsWindowPresenter.configure(window:) now calls clampToVisibleAreaIfNeeded(window) after setting minimum sizes.
Focus / Presentation
Sources/cmuxApp.swift
SettingsWindowPresenter.focus(_:) calls clampToVisibleAreaIfNeeded(window) after deminiaturizing and before activating/ordering the window.
Core Clamping Logic
Sources/cmuxApp.swift
Added clampToVisibleAreaIfNeeded(_:) which computes a clamped origin from the screen's visibleFrame inset by visibleAreaInset and updates the window frame only if the origin changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

A rabbit nudges buttons out of sight,
Erases offsets, sets the window right,
With eighteen points it tucks the pane in tight,
Now Settings stays where screens are bright. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Clamp Settings window away from display edge' accurately and specifically summarizes the main change—adding boundary clamping to the Settings window.
Description check ✅ Passed The description covers the summary, verification details, and testing approach. However, it lacks a demo video (noted as optional in template) and the checklist section is missing, though most critical content is present.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 feat-settings-traffic-lights-offset

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
Review rate limit: 6/8 reviews remaining, refill in 11 minutes and 18 seconds.

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

@greptile-apps

greptile-apps Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR clamps the Settings window inside the visible display area (18 px inset) on both initial configuration and every re-focus, disables window restoration to prevent stale off-screen positions from persisting across launches, and removes the dead traffic-light offset code path that was caching button frames only to reapply a zero offset.

The clamping logic correctly guards against nil screens, handles windows wider than the available area, and is a no-op when the window is already in-bounds.

Confidence Score: 5/5

Safe to merge — changes are well-scoped, logic is correct, and dead code removal has no behavioral impact.

No P0 or P1 issues found. The clamping arithmetic is sound, nil-screen fallback is handled, and the isRestorable = false placement is correct. The only edge case (clamping a window whose frame has not yet been placed on first configure) is benign because the guard on clampedOrigin != frame.origin makes it a no-op when the frame is already valid.

No files require special attention.

Important Files Changed

Filename Overview
Sources/WindowDecorationsController.swift Removes dead-code traffic-light offset machinery: the cached base-frame dictionary, the async apply helper, and the trafficLightOffset(for:) stub that always returned .zero. No behavior change.
Sources/cmuxApp.swift Adds clampToVisibleAreaIfNeeded to both configure(window:) and focus(_:), sets isRestorable = false to prevent stale positions. Clamping logic handles narrow windows and nil-screen fallback correctly; minor edge case around clamping before a window has been placed on first configure.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[show] --> B{existingWindow?}
    B -- No --> C[openWindow]
    C --> D[configure window]
    D --> E[isRestorable = false]
    E --> F[clampToVisibleAreaIfNeeded]
    B -- Yes --> N[focus window]
    N --> O{isMiniaturized?}
    O -- Yes --> P[deminiaturize]
    P --> F
    O -- No --> F
    F --> G{screen available?}
    G -- nil --> H[fallback NSScreen.main]
    G -- present --> I[use window.screen]
    H --> J[compute clamped origin]
    I --> J
    J --> K{origin changed?}
    K -- No --> L[no-op]
    K -- Yes --> M[setFrame display true]
    M --> Q[makeKeyAndOrderFront]
    L --> Q
Loading

Reviews (1): Last reviewed commit: "Clamp Settings window away from display ..." | Re-trigger Greptile

@lawrencecchen
lawrencecchen merged commit 32370f3 into main May 2, 2026
26 of 27 checks passed
@lawrencecchen
lawrencecchen deleted the feat-settings-traffic-lights-offset branch May 2, 2026 10:35

This branch was successfully deployed

1 active deployment
Preview – cmux — 7c1da75a Deployed May 2, 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