Add global starred models to the model picker - #279
Conversation
Co-authored-by: Goose <opensource@block.xyz>
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. The global starred-model flow has three blocking implementation issues: cross-agent selections can inherit the current session's unrelated model provider, persisted snapshots remain executable after their model or agent becomes unavailable, and the new star control bypasses Berd's closed Button system. Two non-blocking picker issues make search inaccurate and can expose a no-op View more action. This is a meaningful graphical UI change, but the supplied PR body and conversation contain no screenshots or recording; add screenshots or a short screen recording before approval. Supplied GitHub check evidence was inspected; required checks independently govern merge readiness.
Deterministic publication result: 3 blocking and 2 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 1 blocking screenshot-evidence requirement(s) in this review body.
🤖 Blocking · Screenshots needed
This PR changes Berd’s graphical interface. Please add screenshots or a short screen recording so the visual result can be reviewed. Screenshots are review evidence; they do not replace accessibility, responsive, theme, localization, or behavior validation.
| model.providerId ?? | ||
| session?.executionTarget?.modelProviderId ?? | ||
| (selectedAgentId === "goose" ? undefined : selectedAgentId); | ||
| (targetAgentId === "goose" ? undefined : targetAgentId); |
There was a problem hiding this comment.
🤖 P1 · Cross-agent star borrows provider (blocking)
When a starred row belongs to a different agent but has no providerId, nextModelProviderId falls back to the current session's modelProviderId. A Claude star selected from a Goose/OpenAI session can therefore produce a target that combines the Claude harness with the unrelated OpenAI provider. The added component test checks only the callback shape and does not exercise this target derivation.
User effect: Choosing a valid favorite from another agent can fail, roll back, or configure the chat for a provider that does not match the model they chose.
Recommended fix: Reuse the session provider only when the target agent is still the selected agent. Resolve cross-agent selections from that target agent's current inventory; derive non-Goose provider identity from the target agent and require a concrete provider for Goose.
Test: Add a hook-level regression test that selects a providerless claude-acp star from a Goose session using OpenAI and asserts the resulting target uses claude-acp rather than openai.
| model.displayName?.toLowerCase().includes(normalizedQuery) || | ||
| model.providerName?.toLowerCase().includes(normalizedQuery) || | ||
| model.providerId?.toLowerCase().includes(normalizedQuery); | ||
| const globalStarred = starredModels.map(({ agentId, model }) => ({ |
There was a problem hiding this comment.
🤖 P1 · Stale stars remain executable (blocking)
The picker renders full ModelOption snapshots directly from localStorage without reconciling them against current per-agent inventory or readiness. Those records survive model removal or renaming, provider disconnection, and agent removal, while still being presented as selectable rows that bypass the setup gating applied to live agent rows.
User effect: People can see an outdated favorite as available and select it, only to hit a failed session start or switch with no trustworthy indication that the model or agent is no longer usable.
Recommended fix: Persist stable identity only, then resolve each favorite against the target agent's live inventory and readiness before rendering or selecting it. Hide removed entries or show unresolved entries as unavailable with a clear recovery or removal action.
Test: Cover a persisted favorite whose model has disappeared and one whose agent is not ready; assert neither is dispatched as an available model selection.
| <IconCheck className="size-4 shrink-0 text-muted-foreground" /> | ||
| ) : null} | ||
| </PickerItem> | ||
| <button |
There was a problem hiding this comment.
🤖 P2 · Star action forks Button system (blocking)
The star action is a raw button with feature-local size, radius, color, hover, and focus styling. Berd's shared UI rules define Button as a closed semantic menu and prohibit feature code from hand-rolling color and interactive-state classes on clickable controls.
User effect: The favorite control can drift from the rest of Berd's picker controls and miss future shared interaction, theme, or accessibility improvements.
Recommended fix: Use the shared Button with the appropriate ghost icon size and pressed semantics. If the intended selected-star treatment is not expressible, extend the shared primitive first rather than styling it locally.
Test: Update the component test to assert the shared Button variant and pressed-state contract while preserving the accessible star and unstar labels.
| return [ | ||
| // The global starred section is always visible, even while browsing or | ||
| // filtering another agent's inventory. | ||
| ...globalStarred, |
There was a problem hiding this comment.
🤖 P2 · Search ignores starred rows (non-blocking)
The active query predicate is applied only to ordinary inventory rows; every global favorite is prepended unconditionally. With any favorite present, unrelated rows remain in the results and the no-results state cannot appear.
User effect: Searching for a model can keep unrelated favorites above the actual matches, making results feel inaccurate and increasingly noisy as the favorite list grows.
Recommended fix: Apply the same query predicate to starred rows while search is active, while retaining their grouping above other matching rows.
Test: Store several unrelated favorites, search by model or provider, and assert that only matching favorite and inventory rows remain and that no-results can still appear.
| modelStarKey(selectedAgentId, model.providerId, model.id), | ||
| ), | ||
| ); | ||
| const hasMore = models.some( |
There was a problem hiding this comment.
🤖 P2 · View more can reveal nothing (non-blocking)
hasMore compares the inventory only with the recommended shortlist and does not account for non-recommended rows already visible through the global starred section. When favorites plus recommendations already cover the inventory, the control is still shown even though expansion adds no unique model.
User effect: People can click View more and see no additional choices; the picker only changes browsing state and hides other affordances, so the action appears broken.
Recommended fix: Derive hasMore by comparing unique rows in the expanded view with the complete compact starred-plus-recommended view.
Test: Use one recommended model and favorite every remaining inventory model, then assert that View more is absent.
2 participants
Summary
Related issue
None found.
Testing
pnpm test— 7,506 passed, 1 skippedpnpm checkpnpm typecheckclippy,tauri-check, andfmt-check