Fix model selector in cloud mode.#11175
Conversation
liliwilson
force-pushed
the
lili/fix-broken-model-selector
branch
from
efb480d to
86a6801
Compare
|
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 |
| } else { | ||
| Some(ChildView::new(&self.model_selector).finish()) | ||
| } | ||
| show.then(|| ChildView::new(&self.model_selector).finish()) |
There was a problem hiding this comment.
This hunk reverts the bad change in https://github.com/warpdotdev/warp/pull/11101/changes#diff-a7d9ab77795051071de9b7b0aaf60bd396d3d2a6bca1c60ba7b59785e5139209 directly
|
|
||
| let model_name = { | ||
| let model_name = if self.is_third_party_harness(ctx) { | ||
| self.harness_model_display_name(ctx) |
There was a problem hiding this comment.
this is the crucial part of the fix for now, given that we don't actually let people change models for 3p harnesses.
the rest of the diff is adding in selector behavior (it doesn't use the inline menu for right now, it just uses the dropdown, which I know is not great)—this isn't accessible at all atm, I'm adding so that if we do let people change models for 3p harnesses, we don't forget to implement the inline menu view for it
![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 PR switches the cloud-mode footer back to ProfileModelSelector and adds harness-aware model display, menu items, and persistence for third-party harness model selections.
Concerns
- The inserted
SelectHarnessModelmatch arm inProfileModelSelector::handle_actionis missing its closing brace beforeManageProfiles, so the file will not parse as shown in the annotated diff. - The new shared persistence helper now discards
set_valueerrors, regressing the previous reporting path for harness model persistence failures.
Found: 0 critical, 1 important, 1 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
| ); | ||
| }); | ||
| } | ||
| self.set_model_menu_visibility(false, ctx); |
There was a problem hiding this comment.
ManageProfiles arm, so the file will not parse as shown; add the missing closing brace after this call.
| }, | ||
| ); | ||
| } | ||
| let _ = self.last_selected_harness_model.set_value(map, ctx); |
There was a problem hiding this comment.
💡 [SUGGESTION] Preserve error reporting here; this helper replaces a call site that used report_if_error!, so persistence failures for harness model selection are now silently swallowed.
liliwilson
force-pushed
the
lili/fix-broken-model-selector
branch
from
75b1b79 to
0bd6673
Compare
2 participants
Description
This fixes a regression to the cloud mode model selector introduced in #11101 -- after that change, the model selector looked weirdly disabled in cloud mode and also rendered the popup rather than inline menu for changing models.
The change was made in the prior PR to fix a different bug where the model selector rendered in the cloud mode footer showed the wrong model (was unaware of harness model types). This happened because we're currently maintaining two different model selectors, one for cloud mode v2 and one for normal model selection.
We should consolidate these two model selectors in the future, but in the interest of minimizing surface area of impact, I chose to just add harness-awareness to the existing one and leave the cloud mode input one alone for now.
Testing
./script/runAgent Mode