Skip AgentTips with unresolved keybinding placeholders#9509
Conversation
MaggieShan
force-pushed
the
oz-agent/agent-tips-voice-keybinding-fix
branch
from
4347e79 to
d65e69e
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 |
![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 filters AgentTips whose descriptions include unresolved keybinding placeholders and adds singleton revalidation hooks for settings, team, and keybinding changes.
Concerns
- The new revalidation hooks update only the singleton tip model, but the visible agent status bar stores a cloned
current_tip; this can leave an already-rendered keybinding tip visible with a raw<keybinding>placeholder after the binding is cleared.
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
MaggieShan
force-pushed
the
oz-agent/agent-tips-voice-keybinding-fix
branch
6 times, most recently
from
72f9cb7 to
ee6ddfa
Compare
When a tip's description contains '<keybinding>' but no keybinding is configured (e.g. voice mode tip with voice_input_toggle_key == None), is_tip_applicable now returns false so the raw '<keybinding>' string is never shown to users. This specifically fixes the voice tip: it is added to the tips pool whenever voice input is enabled, but the keybinding is only set after the user first interacts with voice. Previously the tip could be selected and displayed with the literal text '<keybinding>'. Now it is filtered out at refresh time until a toggle key is configured. The same guard also protects all other keybinding-based tips in case a user has unbound the corresponding action. Co-Authored-By: Oz <oz-agent@warp.dev>
MaggieShan
force-pushed
the
oz-agent/agent-tips-voice-keybinding-fix
branch
from
ee6ddfa to
4eedb25
Compare
| // when the keybinding is actually configured, so we never display the raw | ||
| // "<keybinding>" string to users. | ||
| if self.description.contains("<keybinding>") && self.keystroke(app).is_none() { | ||
| return false; |
There was a problem hiding this comment.
nit: is this extra check necessary given the other fixes in this PR?
if it is, might be worthwhile to log like a warning or error if it's an unexpected path
There was a problem hiding this comment.
Yes this check is still necessary since checking if voice is enabled vs. if there is a keystroke available are two separate checks - this one is for the latter. This is also an expected path for when a user has explicitly removed a setting's keybinding so I also don't think we need a log
| if should_replace { | ||
| let new_tip = Self::pick_random_applicable_tip(&self.tips, None, ctx); | ||
| if new_tip.is_some() || self.current_tip.is_some() { | ||
| self.current_tip = new_tip; |
There was a problem hiding this comment.
would we want to do this if new_tip is none but current_tip is some?
There was a problem hiding this comment.
Yes we still do - as long as current tip is some() in this block, it means it's currently invalid and should either be replaced by a new tip or not be rendered at all
2 participants
Description
AgentTip::is_tip_applicablesuch that tips that expect a keybinding but don't have one are removed from the displayed listTesting
Verified locally with only the voice tip - when setting is disabled or when keybinding is not available, the tip is not applicable/not shown
https://www.loom.com/share/dcfcf6a0b8b24be8aea1ca24dd27ddc9
Agent Mode