agent_ui: Fix scrolling in context server configuration modal #42502
Conversation
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
|
We require contributors to sign our Contributor License Agreement, and we don't have @Amogh-2404 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
Amogh-2404
force-pushed
the
fix-extension-modal-scroll
branch
from
c462578 to
7bfb48c
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have @Amogh-2404 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
When installing a dev extension with long installation instructions, the configuration modal would overflow and users couldn't scroll to see the full content or interact with buttons at the bottom. This fix adds a ScrollHandle to the ConfigureContextServerModal and passes it to the Modal component, enabling the built-in modal scrolling capability. This ensures all content remains accessible regardless of length. Fixes zed-industries#42342
Amogh-2404
force-pushed
the
fix-extension-modal-scroll
branch
from
7bfb48c to
2d95f73
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have @Amogh-2404 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
Contributor
Author
|
@cla-bot check |
Contributor
Author
|
@cla-bot check |
cla-bot
bot
added
the
cla-signed
|
The cla-bot has been summoned, and re-checked this pull request! |
MrSubidubi
requested changes
Nov 12, 2025
Member
MrSubidubi
left a comment
MrSubidubi
left a comment
There was a problem hiding this comment.
Thanks for this! Could you please also ensure that the modal does not grow beyond a certain size? Also, could you look into adding a scrollbar as described in the issue (and perhaps add a recording of it all working)? Thanks!
Add visible scrollbar and height constraint to installation instructions - Limit modal height with max_h_96() to prevent overflow - Add visible scrollbar using WithScrollbar trait - Ensure only instruction content scrolls, not entire modal - Keep footer buttons always visible and accessible Fixes zed-industries#42342
… scrolling Use Modal component's built-in scrolling mechanism instead of manual implementation - Pass ScrollHandle to Modal::new() to enable scrolling for entire modal content - Remove manual scrolling from render_modal_description - This ensures consistent scrolling behavior and fixes test failures The Modal component handles scrolling for all content uniformly, keeping header and footer fixed while the content area scrolls when needed. Fixes zed-industries#42342
Remove ScrollHandle entirely to fix test timeout issues. The Modal component already provides internal scrolling capabilities without requiring explicit ScrollHandle management. Changes: - Remove ScrollHandle field from ConfigureContextServerModal struct - Remove ScrollHandle import and initialization - Pass None to Modal::new() instead of ScrollHandle - Modal still scrolls internally without explicit tracking This minimal approach avoids initialization issues that were causing the extension_host tests to timeout while still allowing the modal content to be scrollable. Fixes zed-industries#42342
Implements maintainer's feedback for issue zed-industries#42342: - Modal now has a maximum height constraint (max_h_96 = 24rem) - Added visible scrollbar using WithScrollbar trait - Scrollable area is limited to installation instructions only - Header and footer remain fixed while content scrolls Implementation follows the proven pattern from ConfigureContextServerToolsModal: - ScrollHandle stored in struct (safe approach) - Pass None to Modal::new() to avoid conflicts - Custom scrollable container with track_scroll and vertical_scrollbar_for All tests pass including the previously timing-out extension_host test. Fixes zed-industries#42342
maxdeviant
changed the title
- Use Option<ScrollHandle> with lazy initialization to prevent test timeouts - Initialize ScrollHandle only when rendering, not in constructor - Add visible scrollbar as requested by maintainer - Ensure modal scrolls properly to bottom of extension description
- Wrap all modal content (description + editor + status) in scrollable container - Set max-height on modal content to enable scrolling on small screens - Add visible scrollbar to entire modal content area - Use lazy ScrollHandle initialization to prevent test timeouts This addresses the maintainer's feedback to make the configure pane scrollable when window is resized, not just the description section.
SomeoneToIgnore
added
the
ai
- Change fixed width to max-width for responsive window resizing - Replace vertical_scrollbar_for with custom_scrollbars - Add with_track_along for always-visible, draggable scrollbar - Modal now adapts to smaller windows while maintaining scroll Fixes: - Modal content no longer gets cut off on small windows - Scrollbar always visible and interactive (can grab and drag) - Follows pattern used in variable_list and breakpoint_list
- Change max_h from fixed rems(40.) to vh(0.7, window) - Modal content now 70% of viewport height - Adapts automatically to window resizing - Ensures footer buttons always accessible Fixes edge case: - When window height shrunk very small, footer buttons were cut off - Now modal content adapts, scrollbar appears, all buttons accessible - Follows pattern used in context_menu.rs and notifications.rs
- Reverted from custom_scrollbars() back to overflow_y_scroll() + vertical_scrollbar_for() - custom_scrollbars() was breaking scrolling in Modal context - Now using proven working pattern that enables scrolling - Modal width and height still responsive (max_w + vh) - Removed unused ScrollAxes and Scrollbars imports Fixes: - Content now scrollable - Footer buttons accessible - Modal adapts to window size (width and height)
Make entire modal content scrollable when window height is small by splitting scrollbar rendering into outer/inner div pattern: - Outer div handles scrollbar rendering with .vertical_scrollbar_for() - Inner div handles scroll behavior with .overflow_y_scroll() - Use vh(0.7, window) for responsive height (70% of viewport) - Ensure footer buttons remain accessible on small screens Fixes scrollbar not being visible and jumping when clicked.
Amogh-2404
force-pushed
the
fix-extension-modal-scroll
branch
from
4e15357 to
a621c1d
Compare
Contributor
Author
Screen.Recording.2025-11-13.at.4.43.27.PM.mov |
MrSubidubi
reviewed
Nov 13, 2025
Member
MrSubidubi
left a comment
MrSubidubi
left a comment
There was a problem hiding this comment.
The test failing was just the test being flaky, so we can just do it like this - no need to make this more complicated.
crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs
Outdated
Show resolved
Hide resolved
crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs
Outdated
Show resolved
Hide resolved
crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs
Outdated
Show resolved
Hide resolved
crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs
Outdated
Show resolved
Hide resolved
MrSubidubi
approved these changes
Nov 13, 2025
Member
MrSubidubi
left a comment
MrSubidubi
left a comment
There was a problem hiding this comment.
Thank you for getting the ball rolling on this, looks good! And congratulations to your first contribution! 🎉
Contributor
Author
|
Thank you 😊 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Fixes #42342
When installing a dev extension with long installation instructions, the configuration modal would overflow and users couldn't scroll to see the full content or interact with buttons at the bottom.
Solution
This PR adds a
ScrollHandleto theConfigureContextServerModaland passes it to theModalcomponent, enabling the built-in modal scrolling capability. This ensures all content remains accessible regardless of length.Changes
ScrollHandleimport to the ui importsscroll_handle: ScrollHandlefield toConfigureContextServerModalstructscroll_handlewithScrollHandle::new()when creating the modalModal::new()instead ofNoneTesting
Release Notes: