Refactor cloud mode v2 history menu.#10863
Conversation
|
I'm starting a first review of this pull 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 |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This refactors the cloud-mode V2 history menu to use InlineMenuView directly and adds compact/dismiss-on-click options to the shared inline menu view.
Concerns
- The cloud-mode history menu no longer closes/restores the composer when pressing Down on the last item or when there are no results.
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
| pub fn select_down(&self, ctx: &mut ViewContext<Self>) { | ||
| self.inner.update(ctx, |v, ctx| v.select_down(ctx)); | ||
| self.menu_view.update(ctx, |v, ctx| v.select_down(ctx)); | ||
| } |
There was a problem hiding this comment.
InlineHistoryMenuView::select_down did; Down on the last item (or with no results) should still emit Close.
| pub fn select_down(&self, ctx: &mut ViewContext<Self>) { | |
| self.inner.update(ctx, |v, ctx| v.select_down(ctx)); | |
| self.menu_view.update(ctx, |v, ctx| v.select_down(ctx)); | |
| } | |
| pub fn select_down(&self, ctx: &mut ViewContext<Self>) { | |
| let should_close = self.menu_view.read(ctx, |v, _| { | |
| let result_count = v.result_count(); | |
| let is_last_item_selected = | |
| result_count > 0 && v.selected_idx().is_some_and(|idx| idx == result_count - 1); | |
| is_last_item_selected || result_count == 0 | |
| }); | |
| if should_close { | |
| ctx.emit(InlineHistoryMenuEvent::Close); | |
| } else { | |
| self.menu_view.update(ctx, |v, ctx| v.select_down(ctx)); | |
| } | |
| } |
There was a problem hiding this comment.
+1 here, should we reuse the select_down from InlineHistoryMenuView?
There was a problem hiding this comment.
make select down great again
There was a problem hiding this comment.
it kinda gets mucky with re-using it because then the InlineHistoryMenuView needs to be aware of when to have select down close and when to have it wrap (it wraps currently). It was a very small change to have the cloud mode history view handle it so I went with that
liliwilson
left a comment
liliwilson
left a comment
There was a problem hiding this comment.
Love me a good refactor
| pub fn select_down(&self, ctx: &mut ViewContext<Self>) { | ||
| self.inner.update(ctx, |v, ctx| v.select_down(ctx)); | ||
| self.menu_view.update(ctx, |v, ctx| v.select_down(ctx)); | ||
| } |
There was a problem hiding this comment.
+1 here, should we reuse the select_down from InlineHistoryMenuView?
## Description <!-- Please remember to add your design buddy onto the PR for review, if it contains any UI changes! --> Fixes REMOTE-1669 and fixes APP-4450. Refactors the history menu to remove a layer of indirection that makes supporting new features easier and also eliminates a crash that Aloke experienced (only once ever). ## Linked Issue <!-- Link the GitHub issue this PR addresses. Before opening this PR, please confirm: --> - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). ## Testing <!-- How did you test this change? What automated tests did you add? If you didn't add any new tests, what's your justification for not adding any? Manual testing is required for changes that can be manually tested, and almost all changes can be manually tested. If your change can be manually tested, please include screenshots or a screen recording that show it working end to end. You can run the app locally using `./script/run` - see WARP.md for more details on how to get set up. --> - [x] I have manually tested my changes locally with `./script/run` ### Screenshots / Videos <!-- Attach screenshots or a short video demonstrating the change, where appropriate. Remove this section if it is not relevant to your PR. --> https://www.loom.com/share/7499f85761234f559cedbef62f5a89d9 ## Agent Mode - [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode
2 participants
Description
Fixes REMOTE-1669 and fixes APP-4450.
Refactors the history menu to remove a layer of indirection that makes supporting new features easier and also eliminates a crash that Aloke experienced (only once ever).
Linked Issue
ready-to-specorready-to-implement.Testing
./script/runScreenshots / Videos
https://www.loom.com/share/7499f85761234f559cedbef62f5a89d9
Agent Mode