feat(chat): add starred models to the agent model picker - #287
feat(chat): add starred models to the agent model picker#287NickTitle wants to merge 24 commits into
Conversation
Star/unstar any model from the agent model picker. Starred models surface in a Starred section pinned at the top of the picker lists. Persistence lives in a small lib + hook: localStorage key goose:starredModels:v1, keyed per (scopeId, modelId), with a window event to sync pickers across windows. Adds en/es strings for star/unstar actions and extends the picker test suite (45 tests). 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 exact PR comparison has four blocking findings and one non-blocking finding. The core starred-model flow can hide saved stars, the expanded picker exceeds Berd's supported minimum window width, the new idle star control fails non-text contrast and bypasses the shared Button system, and concurrent window updates can overwrite saved intent. A failed local-storage write also produces no user-facing outcome. Supplied GitHub evidence is structurally valid for the exact head; some checks remain in progress, so required checks still govern merge readiness.
Deterministic publication result: 4 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
|
I’m aligned with the feature and would like to keep it, but the current visual treatment feels too heavy. Please refine the interaction and styling:
|
Co-authored-by: Goose <opensource@block.xyz>
…h softer fill Co-authored-by: Goose <opensource@block.xyz>
e018355 to
8811486
Compare
…ected state Replace the hand-rolled star <button> with the shared Button primitive (variant=ghost, size=icon-xs) so size, radius, hover, and focus come from the design-system contract, and extend Button with a ghost-only `selected` flag for pressed/added-state toggles: it rests the icon at text-foreground/80 (≈8:1 light, ≈11:1 dark against the popover) while the idle star rests on the ghost icon contract's muted-foreground (≈5.7:1 light, ≈6.1:1 dark) — both above the 3:1 WCAG non-text bar. Coverage: globals.test.ts enforces the muted-foreground/popover pairing in both themes; the picker test asserts the star action uses the shared Button contract (data-slot=button) with the idle/selected treatments and aria-pressed state. 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
COMMENT: the fresh review of the exact three-dot comparison found one new non-blocking localization issue. Four still-present issues are suppressed because unresolved review threads already cover them, and the previously reported compact-list omission is fixed. Supplied GitHub evidence is structurally valid for the exact head and all captured checks are successful.
Deterministic publication result: 0 blocking and 1 non-blocking inline finding(s) publishable; 4 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
|
🤖 P1 #3 resolved —
Review-infra note: the Builderbot review loop was dispatched for this SHA, but the routine run failed at GitHub submission ( Still open (next commits):
|
toggleModelStar previously rewrote one aggregate key via read-modify-write, so two windows toggling different models at the same time could drop each other's stars (last write wins). Each star now lives under its own key (goose:starredModels:v1:entry:<encoded>), so a toggle touches exactly one key and cannot clobber others. The same-model instant-interleave race remains; per-model state stays consistent either way. The storage-event listener in useStarredModels now matches the entry prefix (and clears) instead of one key. A one-shot, idempotent migration folds the legacy aggregate array into per-key entries on first read or toggle, so existing dev-build stars survive. Co-authored-by: Goose <opensource@block.xyz>
persistStarEntry caught localStorage failures (quota exceeded, unavailable storage) silently, so a star toggle would appear to do nothing with no explanation. Failures on write and remove now surface a toast.error with the new chat:notifications.starredModelsPersistError string (en + es). The read path in toggleModelStar reports the same failure when storage is entirely unavailable. Tests stub Storage.prototype setItem/removeItem to throw and assert one toast fires, the entry is untouched, and the toggle state bounces back to the stored truth. Co-authored-by: Goose <opensource@block.xyz>
A starred model a provider stopped serving kept rendering as starred through the synthesized row for the current selection: rows otherwise come only from the available models, so dead stars were already invisible everywhere else. RecommendedModelList now takes the raw catalog (catalogModels) and honors starred state only for models present in it. The dropped current model stays visible and selectable but renders unstarred, without a star toggle (a dead model cannot be favorited). Stored entries are kept, so a star returns if the provider serves the model again; hard pruning was rejected because loading/partial-catalog states could wipe stars. Tests: a starred current model dropped by its provider renders unstarred with no toggle or divider (verified red without the fix), and a star for a model absent from the list renders no row while its entry survives. 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 fresh review of the exact three-dot comparison found three blocking issues. Star state can remain stale after a picker remount, the new legacy migration can delete saved preferences before replacement writes succeed, and the selected model treatment still excludes the trailing star action. Two still-present issues are suppressed by unresolved review threads. Supplied GitHub evidence is structurally valid for the exact head; several checks remain in progress, so required checks still govern merge readiness.
Deterministic publication result: 3 blocking and 0 non-blocking inline finding(s) publishable; 2 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
The star toggle used unnamed group-hover and group-focus-within variants. Outer picker elements also use the group class, so their hover or focus state could reveal several row stars after the pointer left a row. Name the model-row group and scope both reveal variants to it. This keeps each star visible only while its own row is hovered or focused, or while the star itself has keyboard focus. Co-authored-by: Goose <opensource@block.xyz>
Tauri WebKit can retain CSS :hover after the pointer leaves a row, so named Tailwind groups did not fully fix star buttons hanging around. Track the exact row under the pointer and the row containing keyboard focus in React state instead. Pointer leave from a row or the model column now clears the reveal deterministically. Also keep explicit ghost toggle colors stable on hover: selected=false stays muted-foreground and selected=true stays foreground/80. This removes the mixed light and dark star outlines seen across the list. Co-authored-by: Goose <opensource@block.xyz>
Explicit row state cleared correctly, but transition-opacity kept each star visible while it faded. Diagonal pointer movement across narrow rows could therefore leave several stars on screen at different shades. Remove the fade so reveal state changes are immediate, and remove the vertical gaps between model-row hit areas so adjacent rows have no dead strip between them. Co-authored-by: Goose <opensource@block.xyz>
Restore a 75 ms opacity transition now that explicit row pointer state and gap-free hit areas prevent stale stars. Keep the stable toggle colors and deterministic row enter/leave behavior unchanged. Co-authored-by: Goose <opensource@block.xyz>
This reverts commit cf4cda8. Co-authored-by: Goose <opensource@block.xyz>
Use a 75 ms entrance animation instead of transition-opacity. Stars fade in when a row becomes active, but leaving the row removes the animation and opacity class immediately. This keeps the soft reveal without stale stars trailing diagonal pointer movement. Co-authored-by: Goose <opensource@block.xyz>
Increase the entrance-only star animation from 75 ms to 150 ms. Exit remains immediate, so diagonal pointer movement cannot leave trailing stars. Co-authored-by: Goose <opensource@block.xyz>
Treat starred state as a reveal condition so favorited models always show their filled star. Unstarred outlines still fade in on row entry and hide immediately on exit. Co-authored-by: Goose <opensource@block.xyz>
Give model rows and the favorites divider stable motion layout items. When a model is starred or unstarred, its row, nearby rows, and the divider slide to their new positions instead of jumping. Use position-only spring animation so row contents do not stretch. Respect reduced-motion preferences by keeping layout changes instant. 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
COMMENT: the fresh review of the exact three-dot comparison found two new non-blocking architecture/design-system issues. The PR retains migration machinery for an aggregate format absent from the exact base, and the shared Button visual toggle state can drift from its separately supplied accessible pressed state. Five still-present issues are suppressed by unresolved review threads. Supplied GitHub evidence is structurally valid for the exact head and all captured checks are successful.
Deterministic publication result: 0 blocking and 2 non-blocking inline finding(s) publishable; 5 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
Animate star toggles as a three-phase sequence: pop and spin the current star out, move the model row, then spin a newly filled favorite star in. Use one 240 ms rotation per star phase and keep the existing 240 ms row layout spring. Unfavoriting unwinds the filled star before moving the row back. Saved favorites do not animate when the picker opens. Reduced-motion users get an immediate toggle. Delay persistence until the spin-out completes and surface write success so a failed write restores the original row and star. Keep tests aligned with the staged timing, including consecutive toggles and write failures. 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 fresh review of the exact three-dot comparison found one blocking issue. The new decorative star animation delays persistence into a cancellable component effect, so closing the picker can discard the click and concurrent state changes can invert the requested result. Seven still-present issues are suppressed by unresolved review threads. Supplied GitHub evidence is structurally valid for the exact head; some checks remain in progress, so required checks still govern merge readiness.
Deterministic publication result: 1 blocking and 0 non-blocking inline finding(s) publishable; 7 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| return; | ||
| } | ||
| if (starAnimation.state.phase === "out") { | ||
| const timer = window.setTimeout(() => { |
There was a problem hiding this comment.
🤖 P1 · Commit stars before animating (blocking)
The click only records animation state; the actual preference mutation waits 240ms inside this effect. If the picker closes, switches agents, or otherwise unmounts during that interval, effect cleanup cancels the timer and the action is never saved. The delayed call also toggles whatever storage state exists later rather than committing the captured target, so an intervening cross-window change can produce the opposite result. A single shared animation guard additionally drops attempts to star another row while any animation is active.
User effect: A person can click a star and immediately continue or close the picker, then return to find the model was never favorited. Quickly favoriting multiple models can silently lose later clicks.
Recommended fix: Persist the explicit requested target state synchronously in the click handler, then animate the already-committed result. Keep animation state row-local and non-blocking so presentation lifecycle cannot cancel or globally lock user intent.
Test: Add regressions that click Star and immediately close or unmount before 240ms, inject an external same-entry change during the visual transition, and click two different rows quickly; every requested final state must persist.
Build the favorites section from the combined cached model catalogs for all picker agents while keeping regular rows scoped to the selected agent. Favorites now stay constant when switching between Goose, Claude, Codex, and other ready agents. Track each favorite model's owning agent. Clicking a cross-agent favorite switches to that agent first, then selects the model after the controlled agent state updates. Continue hiding a favorite only when its owning catalog no longer contains the model. Co-authored-by: Goose <opensource@block.xyz>
Render the owning agent's icon for favorite rows outside Goose. Preserve Goose's existing behavior: show model-provider icons when providerId is present, and leave providerless Goose rows unchanged. Add a regression assertion that a Claude favorite shown while Goose is selected includes the Claude icon. The test fails before the render fix. 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 fresh review of the exact three-dot comparison found two blocking issues in the new cross-agent favorites flow. Selecting a foreign-agent favorite coordinates two separate session transitions through component-local state, and same-ID models from different agents can both render as selected. Eight still-present issues are suppressed by unresolved review threads. Supplied GitHub evidence is structurally valid for the exact head and all captured checks are successful. A product decision is also needed on whether cross-agent favorites should bypass the existing gated agent-switch flow.
Deterministic publication result: 2 blocking and 0 non-blocking inline finding(s) publishable; 8 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| } | null>(null); | ||
| const handleModelSelect = (model: ModelOption, agentId: string) => { | ||
| if (agentId !== selectedAgentId) { | ||
| pendingCrossAgentModelRef.current = { agentId, model }; |
There was a problem hiding this comment.
🤖 P1 · Apply one cross-agent target (blocking)
A foreign-agent favorite is split into two independently owned transitions: the picker stores the model in a ref and first calls onAgentChange, whose controller path can apply that agent’s preferred model or recreate and navigate the session; only after selectedAgentId changes does this effect call onModelChange for the clicked model. Queued work can observe the intermediate target, navigation can unmount and lose the ref, and a failed first transition can leave stale intent that applies during a later unrelated switch.
User effect: Choosing a favorite from another agent can move the chat to that agent’s default model instead of the model clicked, briefly run queued work on the wrong target, or unexpectedly apply the old choice during a later switch.
Recommended fix: Emit one agent-and-model selection intent from the picker and let the session/controller owner apply that final execution target atomically through its existing versioning, recreation, navigation, and rollback machinery. Do not bridge session transitions with component-local ref state.
Test: Cover a started session with queued work, a recreation/navigation path, a failed or rejected agent switch, and a later unrelated switch. The click must produce one final agent/model target, never expose an intermediate preferred model, and never leave stale pending intent.
Keep favorite hover state stable while rows move, transfer hover to the row that finishes under a stationary pointer, and collapse cross-agent favorites that have no destination in the selected agent catalog. Prevent duplicate Claude default rows and sort favorites alphabetically across agents. Gate the agent panel by default, keep Switch agent anchored in the footer, fix the picker width for each panel mode, and anchor smart placement to the trigger's leading edge so model label changes do not move the popover. Co-authored-by: Goose <opensource@block.xyz>
Use a single half rotation for favorite and unfavorite star phases while keeping the existing timing, pop, opacity, and row movement. End each visible star upright. 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
COMMENT: the fresh review of the exact three-dot comparison found one new non-blocking accessibility and clarity issue: cross-agent favorite rows do not reliably expose their owning agent. Nine still-present issues are suppressed by unresolved review threads. Supplied GitHub evidence is structurally valid for the exact head; some checks remain in progress, so required checks still govern merge readiness. Favoriting and cross-agent favorites are accepted product direction; no escalation is needed.
Deterministic publication result: 0 blocking and 1 non-blocking inline finding(s) publishable; 9 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
Co-authored-by: Goose <opensource@block.xyz>
Remove the unshipped aggregate migration, refresh the cached snapshot when the store remounts, and use idiomatic Spanish unstar copy. Make the shared ghost Button selected state own both its visual and aria-pressed state, with primitive coverage for true, false, omitted, and unsupported variants. Co-authored-by: Goose <opensource@block.xyz>
Scope selected state to the active agent and label foreign favorites with their owning agent in visible and accessible text. Preserve one continuous selected surface across each complete row. 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
No new publishable findings were identified in the fresh review of the exact three-dot comparison. Two still-present blocking issues are suppressed by their existing unresolved threads, which have no substantive human replies; approval therefore fails closed and no publication/retry is recommended. The supplied GitHub evidence is structurally valid for the exact head. The frontend check is failed and still governs merge readiness. Favoriting remains accepted product direction, with no escalation.
Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 2 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
2 participants
What
Star/unstar any model from the agent model picker. Starred models surface in a Starred section pinned at the top of the picker lists (collapsed and expanded views), with a divider after the starred block.
How
src/features/chat/lib/starredModels.ts: localStorage persistence (keygoose:starredModels:v1), keyed per(scopeId, modelId), with a window event to sync pickers across windows.useStarredModelshook wrapping the lib for React components.AgentModelPickerLists: starred models are grouped first in both the shortlist and the sorted/browse views; recents and recommended entries exclude starred models so nothing appears twice. Starred items integrate with the existing recents/shortlist logic.AgentModelPickerrows get a star toggle button with en/es strings ("Star {{model}}" / "Unstar {{model}}").Separatormoves into the design-system explorer used-components list (it is now rendered by the picker).Testing
AgentModelPicker.test.tsxextended for star/unstar, starred-section ordering, and persistence; 51 tests pass.pnpm typecheckanddesign-system:coverage --strictpass.Co-authored-by: Goose opensource@block.xyz
