Keep titlebar stable at minimum sidebar width#5089
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExtracted titlebar leading padding into ChangesTitlebar Leading Padding Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR fixes titlebar icon/title jitter by extracting the leading-padding calculation into a pure
Confidence Score: 5/5Safe to merge — a narrowly scoped layout helper with no state mutations and full test coverage of the new code paths. The change extracts a pure computation into a static function, adds a minimumSidebarWidth floor to prevent jitter, and ships four targeted tests. No actor isolation, concurrency, or state-management concerns were introduced. The one untested combination (fullscreen + visible sidebar) is handled by the same guard chain already exercised by the other cases and matches the old formula's behavior. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[customTitlebarLeadingPadding] --> B{isFullScreen AND not isSidebarVisible}
B -- Yes --> C[return 8]
B -- No --> D[minimumSidebarTitleInset = max of titlebarLeadingInset and minimumSidebarWidth plus 12]
D --> E{isSidebarVisible}
E -- No --> F[return minimumSidebarTitleInset]
E -- Yes --> G{sidebarWidth greater than minimumSidebarWidth plus 0.5}
G -- No --> H[return minimumSidebarTitleInset]
G -- Yes --> I[return max of titlebarLeadingInset and sidebarWidth plus 12]
Reviews (2): Last reviewed commit: "fix: address titlebar padding review fee..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| let visibleSidebarTitleInset = sidebarWidth + 12 | ||
| guard sidebarWidth > minimumSidebarWidth + 0.5 else { |
There was a problem hiding this comment.
0.5 sub-pixel tolerance that separates "at minimum width" from "wider than minimum" is not explained. Without a comment, future readers may wonder whether this is pixels, points, a render artifact, or an arbitrary guard. A brief inline note stating that it absorbs floating-point drift around the minimum clamp would make the intent clear.
| guard sidebarWidth > minimumSidebarWidth + 0.5 else { | |
| // Use a 0.5-point tolerance to absorb floating-point drift at the minimum-width clamp. | |
| guard sidebarWidth > minimumSidebarWidth + 0.5 else { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Added an inline note explaining the 0.5-point tolerance around the minimum-width clamp.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@cmuxTests/WindowAndDragTests.swift`:
- Around line 1961-2026: The tests in CustomTitlebarLeadingPaddingTests should
be migrated from XCTest to the Swift Testing framework: replace the XCTestCase
class with a `@Suite` (e.g., make it a struct) and mark each test function
(testHiddenSidebarUsesMinimumSidebarTitleInset,
testMinimumWidthVisibleSidebarMatchesHiddenSidebarTitleInset,
testWiderSidebarPushesTitlebarContentRight,
testFullscreenHiddenSidebarKeepsCompactInset) with `@Test`; import Testing at the
top; convert assertions like XCTAssertEqual and XCTAssertGreaterThan to the
Swift Testing equivalents (`#expect`(...).to(equal(...)) or try `#require`(...) /
`#expect`(...).to(beGreaterThan(...)) as appropriate) while still calling
ContentView.customTitlebarLeadingPadding with the same parameters. Ensure test
names, calls to ContentView.customTitlebarLeadingPadding, and expected values
remain unchanged.
In `@Sources/ContentView.swift`:
- Line 2078: The return expression uses max(minimumSidebarTitleInset,
visibleSidebarTitleInset) but due to the guard that ensures sidebarWidth >
minimumSidebarWidth + 0.5 the minimumSidebarTitleInset (which includes
minimumSidebarWidth + 12) is always smaller than visibleSidebarTitleInset;
simplify the return in the function containing this logic by returning
max(titlebarLeadingInset, visibleSidebarTitleInset) (use
visibleSidebarTitleInset and titlebarLeadingInset names to locate the code) to
remove the redundant minimumSidebarTitleInset term while preserving behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ea7a9c8-6467-4bbc-9db1-c1e9392ca56c
📒 Files selected for processing (2)
Sources/ContentView.swiftcmuxTests/WindowAndDragTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary
Verification
Dogfood build: http://127.0.0.1:17320/tbmin
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Keep the titlebar icon/title position stable when the sidebar is hidden or at minimum width, and keep shifting it right when the sidebar is wider. Also handles the fullscreen edge case for a consistent look.
customTitlebarLeadingPadding(...)to clamp at min width, respecttitlebarLeadingInset, and absorb float drift near the clamp.customTitlebar; use an 8pt inset when fullscreen with a hidden sidebar.Written for commit 6f5db4c. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests