Slash command menu working.#9358
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
I'm starting a first review of this pull request. You can follow along in the session on Warp. I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a Cloud Mode V2 slash-command menu, including a new floating view, zero-state grouping for commands/skills/prompts, compact slash-command item rendering, and routing of keyboard acceptance/navigation to the new view when composing in Cloud Mode V2.
Concerns
- The new click handler in
CloudModeV2SlashCommandView::rebuild_from_resultscaptures and moves an unused weak handle, which makes the callback fail the renderer'sFnrequirement and blocks compilation.
Verdict
Found: 1 critical, 0 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
| item, | ||
| cmd_or_ctrl_enter: false, | ||
| }); | ||
| let _ = weak_handle; |
There was a problem hiding this comment.
🚨 [CRITICAL] This let _ = weak_handle; moves the captured WeakViewHandle out of the click closure, so the closure is only FnOnce and cannot satisfy QueryResultRenderer::new's Fn bound or be reused for every renderer. Remove the unused weak-handle capture/field, or create a fresh clone per renderer if the callback needs it.
There was a problem hiding this comment.
Overview
This PR adds a Cloud Mode V2 slash-command menu, new sectioned zero-state results for commands/skills/prompts, compact slash-command item rendering, and routes keyboard acceptance/navigation to the new menu while composing.
Concerns
- The new cloud-mode view currently captures and moves
weak_handleinside its click callback, which makes the callback incompatible with theFncallback expected byQueryResultRenderer::newand reused for each renderer. - The shared slash-command data source is switched to compact/v2 behavior whenever
CloudModeInputV2is enabled, even though the regular inline slash menu still uses the same handle outside cloud-mode composing. - Security pass: no security-specific findings in the changed lines.
Verdict
Found: 1 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
| item, | ||
| cmd_or_ctrl_enter: false, | ||
| }); | ||
| let _ = weak_handle; |
There was a problem hiding this comment.
🚨 [CRITICAL] let _ = weak_handle; moves the captured handle out of the click closure, so the closure is FnOnce while QueryResultRenderer::new requires an Fn and this same callback is reused for every renderer; remove this capture or clone per renderer.
| terminal_view_id, | ||
| }; | ||
| if FeatureFlag::CloudModeInputV2.is_enabled() { | ||
| SlashCommandDataSource::for_cloud_mode_v2(args, ctx) |
There was a problem hiding this comment.
slash_command_data_source to compact/v2 rendering for every slash menu whenever the flag is enabled, but the regular inline slash menu still receives this handle outside is_cloud_mode_input_v2_composing; keep the default source for the inline view or create a separate v2 source for CloudModeV2SlashCommandView.
21bc4dc to
a9aebec
Compare
liliwilson
left a comment
There was a problem hiding this comment.
Sweet!
A few things I am noticing while playing around:
- Inserting a skill into the input + hitting enter doesn't kick off a cloud agent
- Inserting a saved prompt into the input renders what I think is the saved prompt card at the top of the screen
- Some of the slash commands don't work and then prevent you from opening the command menu again (e.g. `/prompts`) - might just be some missing plumbing
| pub terminal_view_id: EntityId, | ||
| } | ||
|
|
||
| pub struct SlashCommandDataSource { |
There was a problem hiding this comment.
(Non-blocking) This is somewhat of a preexisting bug, but could we add something like an allowed_commands filter here that filters out commands that don't make sense in this cloud input view?
Thinking of things like /create-environment, /cloud-agent, /compact, /cost, /export..., /fork, /queue, /remote-control, /rewind
There was a problem hiding this comment.
just did this here because there's a bunch of commands that don't apply and making the correct UI behavior happen is extra work!
We also should eventually not show /fork and a bunch of others at the beginning of a conversation too
There was a problem hiding this comment.
My list is slightly different than yours, but lmk if there's something that you didn't expect!
| } | ||
| } | ||
|
|
||
| fn next_selectable_browsing_idx( |
There was a problem hiding this comment.
nit: can we genericify this fn and the next one?
| #[allow(dead_code)] | ||
| fn _icon_size() -> f32 { | ||
| ICON_SIZE | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| fn _icon_to_text_gap() -> f32 { | ||
| ICON_TO_TEXT_GAP | ||
| } |
|
All of these are fixed now! Was a little sloppy with Q&A - I noted all of these yesterday but forgor when I logged on late. |
4066eb6 to
d8daf72
Compare
liliwilson
left a comment
There was a problem hiding this comment.
Will be good to get this in people's hands on dogfood!
| description: "Start a new conversation", | ||
| icon_path: "bundled/svg/oz.svg", | ||
| availability: Availability::AI_ENABLED, | ||
| availability: Availability::AI_ENABLED.union(Availability::NOT_CLOUD_AGENT), |
There was a problem hiding this comment.
Ooh, nice that we can reuse this!
| pub terminal_view_id: EntityId, | ||
| } | ||
|
|
||
| pub struct SlashCommandDataSource { |
| slash_command_data_source: ModelHandle<SlashCommandDataSource>, | ||
| include_skills: bool, | ||
| include_saved_prompts: bool, | ||
| compact_layout: bool, |
There was a problem hiding this comment.
nit: can we call this is_cloud_mode_v2 to match the other one and pass that into a single constructor?
For now we might not even need separate include_skills and include_saved_prompts bools since they all just boil down to is_cloud_mode_v2
There was a problem hiding this comment.
Done — consolidated into a single is_cloud_mode_v2 field with one new(..., is_cloud_mode_v2: bool) constructor.
<!-- Please remember to add your design buddy onto the PR for review, if it contains any UI changes! --> Adds the new slash command menu for cloud mode v2. https://www.loom.com/share/12c523ae66ca4e26b2d2ef46fdbcb035 <!-- 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? If you're not sure whether you should add a test, check our testing policy: https://www.notion.so/warpdev/How-We-Code-at-Warp-257fe43d556e4b3c8dfd42f70004cc72#1f97825450504baa9c5fd87a737daa09 --> See loom.

Description
Adds the new slash command menu for cloud mode v2.
https://www.loom.com/share/12c523ae66ca4e26b2d2ef46fdbcb035
Testing
See loom.