feat(opencode): group the model picker by connected provider - #8072

💭 Looking at the current diff, it seems the author addressed the earlier duplication concern by extracting a shared ModelPickerSectionRow, pluralizing the count, and adding hover/highlight styling to match ModelListRow. I should now check the current head for any new issues.

1. git_tools — ModelPickerSectionRow|ComboboxItem|ModelListRow

2. git_tools — hideIndicator|contentClassName|data-highlighted

💭 I want to look at the file around the new code, comparing ModelPickerSectionRow and ModelListRow to understand the pattern.

3. browse_code — apps/web/src/components/chat/ModelPickerContent.tsx

4. browse_code — apps/web/src/components/chat/ModelListRow.tsx

💭 The section rows seem fine since their values never match a selected model key, so the data-selected styling wouldn't conflict there. Missing the transition classes present on ModelListRow is minor. What's more interesting is that ModelListRow adds !min-w-0 max-w-full to override the combobox's default min-width behavior when side=none, but the section row only has w-full without that override — this was pre-existing for legacy section rows but now applies to new sub-provider rows too. I need to check if the picker popup actually renders with side=none, since that would determine whether this missing override causes real horizontal overflow issues. I also want to verify the key-parsing order in the onValueChange handler — whether parseModelPickerSubProviderSectionKey is checked before model key parsing, and whether there's any prefix collision between the new sub-provider section key and the legacy section key format. I should check other places in the web app that render sub-provider info for UI consistency, and confirm the virtualized list's keyboard navigation (flatModels, jumpLabels, visibleModelKeys) properly accounts for section rows. I'm also second-guessing whether aria-expanded is valid on a role="option" element per ARIA spec, since option semantics may not officially support that attribute. Actually, aria-expanded isn't a standard property for role=option in ARIA 1.2, but this was pre-existing behavior on the legacy row, not something introduced now. I'm confirming the section row's touch target height is fine given its padding and two-line text, and checking that the hover color-mix CSS variable is used consistently elsewhere like in ModelListRow. I'm also noting the leftover "group" class on ModelPickerSectionRow is harmless since it's not used for any group-* styling there, so no need to flag it, and now I'm shifting focus to mobile behavior. Mobile parity is out of scope since only web files are in scope here. I want to check lines 440-1000 of ModelPickerContent for real issues, particularly around how section headers render inside the virtualized list with estimated item sizes—since section rows likely have different heights than model rows, that could throw off the virtualization estimates.

5. browse_code — apps/web/src/components/chat/ModelPickerContent.tsx

💭 I want to look at the render section around lines 620-1050 to check list rendering, separators, jump labels, and item keys for any visual issues like duplicated provider labels. Specifically, within an expanded sub-provider section, each ModelListRow still shows the provider footer text, which duplicates the section header—I should check the showProvider logic to see if this is intentional or a consistency issue worth flagging.

6. browse_code — apps/web/src/components/chat/ModelPickerContent.tsx

💭 I notice models under sub-provider groups still show a "showProvider" footer repeating the vendor name, which duplicates the section header — a minor redundancy rather than a clear-cut violation. I'm also checking indentation for nested rows, label truncation, accessibility attributes on section rows, and whether combobox selection state could leak into section headers — all seem fine on inspection. I'm now weighing whether the legacy section row had different hover/highlight behavior that the new implementation might be missing. Looking more closely at the highlight styling, both ModelListRow and ModelPickerSectionRow override the background but rely on the base combobox's default text-accent-foreground on highlight, so they're consistent with each other. That said, I notice the color-mix treatment for the highlight background is duplicated verbatim across both components, which could be worth flagging as a candidate for a shared named primitive rather than repeated literal strings — though I'm not yet sure this rises to the level of a real issue.

7. git_tools — color-mix(in_srgb,var(--popover)_90%,var(--contrast-foreground))

8. browse_code — apps/web/src/components/ui/combobox.tsx

💭 The color-mix hover styling now appears at four call sites within the model picker, which per the guidelines is a legitimate case for extracting a named variant rather than repeating the treatment — though I did already nudge them toward this pattern in the prior review. Checking other consistency points: the section row label typography matches the model row's naming style, so that's fine. But the section row lacks the transition property that ModelListRow has, meaning hover on section rows will snap instantly while model rows fade smoothly — a minor visual inconsistency worth noting, though weak since it's an aesthetic nit. I'm also checking whether the min-width/max-width constraints that ModelListRow explicitly sets are missing from the new row. Since this container sits inside a virtualized list under a possibly inline combobox popup, if an ancestor carries a data-side="none" attribute, that CSS variant forces a minimum width based on anchor width. If the section rows don't defeat this like ModelListRow does, they could end up wider than the container, causing real horizontal overflow or a clipped chevron — this looks like a genuine layout regression worth flagging rather than a stylistic nit.

9. git_tools — data-side|ComboboxListVirtualized

💭 I'm checking that the hideIndicator prop is properly ignored, and confirming the keyboard navigation and aria-expanded pattern for section rows matches existing conventions, before considering whether there's a concrete filtering issue to address. Tracing through the sub-provider path, I verify that filteredItemKeys and visibleModels correctly account for ungrouped models, expanded sections, and legacy models without dropping any, and that jump-label ordering matches render order. Now I want to check the sub-provider section key encoding against the legacy prefix to rule out any collision, so I'm looking at modelPickerKeys.ts.

10. browse_code — apps/web/src/components/chat/modelPickerKeys.ts

💭 I'm noticing that the selector for stored expanded sections returns a fresh empty object literal whenever the value is undefined, which could cause unnecessary re-renders or even loops depending on how the store's equality check works — worth checking the useClientSettings implementation to confirm. I also want to flag whether rows nested under a vendor sub-header end up redundantly showing the same vendor label again in their footer text, since that could be a minor visual redundancy worth mentioning. Checking the chevron icon sizing and coloring in the section row against the base combobox styles confirms they inherit the muted-foreground color and shrink-0 behavior correctly, matching prior conventions. I'm not finding any new concrete violations beyond what's already been noted, though the check seems to default to failure unless there are explicit findings to report. I also want to verify the favorited-model exclusion logic doesn't create a count mismatch, and it checks out—counts stay accurate since favorited models are removed from their section entirely. I'll also glance at the separator placement between headers and models, and the typing of the expanded-sections state, but neither seems worth flagging as a real issue. That parsing looks correct since the instanceId matches the selected instance except when favorites are selected, which returns null appropriately. I'm satisfied the logic holds up after the earlier rounds of review, though I want to double check one lingering issue: the same color-mix highlight literal now appears four times across the sidebar and ModelListRow, and since I previously asked for consistency, the team just copied the literal rather than extracting a shared token — worth considering whether to flag that as a follow-up. Given the guidance to focus on concrete regressions from changed lines and pre-existing issues the patch worsens, this repetition is a minor worsening but feels more like a stylistic refactor than a real violation, so I'll leave it unflagged. The mobile session-memory claim in the docs is out of scope here too, so overall this looks clean.