fix(titlebar): stop the #511 drag fillers collapsing to zero height - #527
Merged
Conversation
The fillers added for #511 reserved their width but rendered 0px tall, so the reserved 96px navigation slot and the two 12px inter-cluster spacers stayed as dead as the `gap-3` they replaced. `h-full` is `height: 100%`, and a percentage cannot resolve against a parent whose own height is `auto`, which is what both containers had. The bar still looked right, which is why this survived review: an invisible filler that works and one that does not are indistinguishable on screen. The containers now carry `h-full` down from the only definite height in the chain, the `h-9` on the bar itself, and the fillers use `self-stretch` rather than `h-full`, because a definite height beats the stretch and would put them back at zero. Measured in the running app through the WebKit inspector, hit-testing every 2px across the bar. Before: x=1106 to 1212, immediately left of the AeroVault shield, answered with no drag region, which is exactly where the report placed the dead zone. After: the same span answers with the filler, and so does the second spacer at x=1432 to 1442. Checked both on My Servers, where the reserved slot renders no button, and on a connected session, where Home and Disconnect fill it. The guard test reads the source instead of the DOM on purpose: vitest runs in the node environment here and jsdom has no layout engine, so a rendered assertion would report zero height for a correct filler and pass for a broken one. This was verified on Linux only. Confirmation on Windows 10 is still what closes the report. #511 Co-Authored-By: Ehud Kirsh <100953560+EhudKirsh@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe titlebar now propagates full height through right-side clusters and uses ChangesTitlebar drag-region sizing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/components/customTitlebarDragRegions.test.ts`:
- Around line 32-39: Strengthen the test in the rightHandFillers case to verify
each filler role rather than only the aggregate count: assert exactly one class
string contains both flex-1 and self-stretch, and exactly two contain w-3,
self-stretch, and shrink-0. Keep the existing no-h-full assertion for all
right-hand fillers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7715b20f-6e80-4090-964f-59c6bc8b7bdf
📒 Files selected for processing (2)
src/components/CustomTitlebar.tsxsrc/components/customTitlebarDragRegions.test.ts
CodeRabbit's point on #527: three stretching fillers satisfy the total even if the reserved-slot filler has lost its `flex-1` and stopped claiming the empty width, or a spacer has lost its `w-3` and become a zero-width region, which is exactly as ungrabbable as the zero-height one this test exists to catch. Verified by breaking each case separately: dropping `flex-1` from the slot filler turns it red, and dropping `w-3` from either spacer turns it red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
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.
The fix for #511 shipped in #516 did not work, and it looked like it did.
The three drag fillers it added, the reserved 96px navigation slot and the two 12px inter-cluster spacers that replaced a
gap-3, all usedh-full. That isheight: 100%, and a percentage height cannot resolve against a parent whose own height isauto, which is what both of their containers had. They rendered 0px tall: they still reserved their width, the bar still looked right, and the window manager still had nothing to grab. Only the pre-existing centre spacer worked, because its parent is the bar itself, the one element in the chain with a definite height (h-9).Measured, not reasoned
Driven headlessly in the running app through the WebKit inspector, hit-testing every 2px across the titlebar with
document.elementFromPoint(x, y).closest('[data-tauri-drag-region]'):That dead span of 108px sits immediately left of the AeroVault shield, which is exactly where the report placed it. Filler heights over the same DOM go from
[35, 35, 0, 0, 0]to[35, 35, 35, 35, 35].Checked in both states: on My Servers, where the reserved slot renders no button and the filler owns all 96px, and on a connected session, where Home and Disconnect fill the slot and the two spacers carry the affordance (
DRAG 1202-1212,DRAG 1432-1442).The fix
h-fullon the two containers, so the definite height reaches down from the bar unbroken, andself-stretchinstead ofh-fullon the fillers themselves, because a definite height beats the stretch and would put them straight back to zero.The guard
src/components/customTitlebarDragRegions.test.tsreads the source rather than the rendered DOM, on purpose: vitest runs in thenodeenvironment here and jsdom has no layout engine, so a rendered assertion would report zero height for a correct filler and pass for a broken one. Verified by breaking it: restoringh-fullon a filler, or droppingh-fullfrom the 3-cluster container, turns it red.Gates
tsc --noEmitclean,vitest595/595,vite buildclean,i18n:validate46/46 with 0 placeholders. No Rust touched.Verified on Linux only. Confirmation on Windows 10 is what closes the report, so this does not close it.
#511
Summary by CodeRabbit
Bug Fixes
Tests
data-tauri-drag-regionelements in the title bar.