[NEW] tab_config windows maximum height set to window height and scro…#13224
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @fabiSwDevBeg on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
Every PR must be linked to a same-repo issue before Oz can review it. This PR is linked to #13223, but no linked issue is marked See the contribution guidelines for the full readiness model. Powered by Oz |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #13223, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
|
Hi @fabiSwDevBeg — a reviewer requested changes on this PR and it hasn't had activity from you in 7 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
|
Hi @fabiSwDevBeg — final reminder: a reviewer requested changes on this PR and it has been inactive for 10 days. It will be automatically closed in about 4 day(s) unless you push updates or reply. Maintainers can apply the |
|
@cla-bot check |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #13223, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
|
The cla-bot has been summoned, and re-checked this pull request! |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR makes the new-session/tab configs menu scrollable and caps its height based on the available window height.
Concerns
- The new height calculation can still overestimate available space for the vertical-tabs AddTabButton path because rendering anchors from the saved vertical-tabs button position while the keyboard path passes a zero anchor.
- This is a user-facing UI behavior change, but the PR description does not include screenshots or a screen recording demonstrating the scrollable capped menu end to end.
- No security findings identified in the changed code. No approved spec context was available for spec-alignment review.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR makes the new session / tab configs menu scrollable and caps its height based on the available window space, with a focused unit test for the height calculation.
Concerns
- This is a user-facing menu/layout change, but the PR description does not include screenshots or a screen recording demonstrating the menu with many tab configs staying within the window and scrolling end to end. Warp's repo review guidance requires visual evidence for UI-impacting changes, so please attach a screenshot or short recording from a local desktop run or another environment with desktop/computer-use support.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR makes the new-session / tab configs menu scrollable and caps its height based on the available window space below the menu anchor. It also adds test-only accessors and a regression test for the scrollable, height-constrained menu behavior.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ctx: &ViewContext<Self>, | ||
| ) -> f32 { | ||
| let Some(window) = ctx.windows().platform_window(ctx.window_id()) else { | ||
| return 480.; |
There was a problem hiding this comment.
let's store these magic values as constants. same with 120 at line 6758
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub fn height(&self) -> f32 { |
There was a problem hiding this comment.
i think we don't need to add these just for the sake of testing, we can just directly check in the test itself
Extract menu height magic values into named constants and update the height test to assert the Workspace calculation directly instead of adding test-only Menu getters.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR makes the new session / tab configs menu scrollable and tries to cap its height to the remaining window space when opened.
Concerns
- The computed max height is applied to the menu's scrollable content, but the menu renderer adds vertical padding outside that constrained area, so a full menu can still extend past the bottom of the window.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| return NEW_SESSION_MENU_FALLBACK_MAX_HEIGHT; | ||
| }; | ||
| let anchor_y = self.new_session_menu_height_anchor_y(anchor, ctx); | ||
| let available_height = window.size().y() - anchor_y - NEW_SESSION_MENU_WINDOW_MARGIN; |
There was a problem hiding this comment.
Menu::set_height caps only the scrollable item area; Menu::render adds MENU_VERTICAL_PADDING above and below that constrained content, so using the full remaining window height here still lets a long menu overflow below the window by the padding/border. Subtract the menu chrome from this available height, or rely on WindowBySize sizing for the positioned menu, so the final rendered menu is actually capped to the window.
…ide the scrollable area, including vertical menu padding and border, so the final rendered dropdown fits within the remaining window height. The test now accounts for that chrome in the expected capped height.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR makes the new-session/tab-configs menu scrollable and caps its height based on the available window space across the existing horizontal, vertical, and pointer-opened menu anchor paths.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…13224) With enough tab configs the new-session menu overflowed past the bottom of the window with no way to reach the hidden entries. Compute a max height from the window and anchor position and switch the menu to MenuVariant::scrollable(). Ported from warpdotdev/warp#13224. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR makes the new session/tab configs menu scrollable and caps its height based on the available window space, with a focused unit test covering the height calculation. The PR description includes video evidence for the user-facing behavior.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Description
Makes the new session / tab configs menu height-constrained and scrollable. When users have many tab configs, the menu now caps itself to the available window height instead of extending below the terminal window, and the remaining items can be reached by scrolling.
Linked Issue
Fixes #13223
Testing
Additional automated testing:
./script/formatcargo test -p warp test_new_session_menu_is_scrollable_and_capped_to_window_heightcargo test -p warp tab_configs_menucargo runChangelog Entries for Stable
CHANGELOG-BUG-FIX: Fixed the tab configs menu overflowing past the terminal window when many tab configs are available.
Demo
Attached is a short video demonstrating the proposed behavior and the user experience after this change.
warpdotdev-13224.mp4