feat(chat): add multi-model selector and chat hook#9854
Open
feat(chat): add multi-model selector and chat hook#9854
Conversation
2 tasks
Contributor
Greptile SummaryThis PR adds the All five issues flagged in prior review rounds have been resolved: the orphaned separator is now correctly gated on Remaining findings:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useMultiModelChat hook] -->|selectedModels, add/remove/replace| B[ModelSelector component]
B -->|open popover| C[ModelListContent component]
C -->|buildLlmOptions| D[LLMPopover.buildLlmOptions]
C -->|groupLlmOptions| E[LLMPopover.groupLlmOptions]
B -->|replacingIndex=null → onAdd| A
B -->|replacingIndex=N → onReplace| A
B -->|onRemove| A
A -->|buildLlmOverrides| F[LLMOverride array sent to backend]
G[LLMPopover] -->|isLoading, isSelected, footer| C
Prompt To Fix All With AIThis is a comment left during a code review.
Path: web/src/refresh-components/popovers/ModelSelector.tsx
Line: 1003-1015
Comment:
**Fragile DOM class sniffing for X-icon detection**
The click handler distinguishes an X-remove click from an open-popover click by querying the internal CSS class `.interactive-foreground-icon` from `SelectButton`. This couples the component to an undocumented implementation detail of `@opal/components`.
If `SelectButton` renames or restructures that class, `lastIcon` will be `undefined`, the `contains` guard will always be `false`, and **all** clicks on a model pill (including on the X icon) will silently open the replace-popover instead of removing the model — with no compile-time error.
Consider requesting an `onRightIconClick` callback prop from the design system, or restructuring so the X uses a separate focusable element that calls `onRemove` via its own `onClick` and `e.stopPropagation()`:
```tsx
<div className="relative">
<SelectButton
icon={ProviderIcon}
state="empty"
variant="select-tinted"
interaction="hover"
size="lg"
onClick={(e: React.MouseEvent) => handlePillClick(index, e.currentTarget as HTMLElement)}
>
{model.displayName}
</SelectButton>
<button
className="absolute right-1 top-1/2 -translate-y-1/2"
onClick={(e) => { e.stopPropagation(); onRemove(index); }}
aria-label="Remove model"
>
<SvgX className="h-4 w-4" />
</button>
</div>
```
If `.interactive-foreground-icon` is a documented, stable part of the opal design system API, feel free to dismiss this — just worth confirming.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/refresh-components/popovers/ModelSelector.tsx
Line: 885-895
Comment:
**`isSelected`/`isDisabled` not wrapped in `useCallback`**
Both functions are recreated on every render and passed as props to `ModelListContent`, which uses `isSelected` as a `useMemo` dependency (`[groupedOptions, isSelected]`). A new function reference on each render causes `defaultGroupKey` to recompute every time `ModelSelector` re-renders (e.g. when `open` or `replacingIndex` state changes), even when the logical selection hasn't changed.
Wrapping both in `useCallback` with their actual dependencies (`replacingIndex`, `replacingKey`, `selectedKeys`, `otherSelectedKeys`, `atMax`) would stabilise the references and make `ModelListContent`'s memoisation effective.
How can I resolve this? If you propose a fix, please make it concise.Reviews (9): Last reviewed commit: "refactor(LLMPopover): use ModelListConte..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 4/5
- This PR looks generally safe to merge, but there is a policy-compliance risk rather than a clear runtime regression risk.
- The most significant issue is in
web/src/refresh-components/popovers/ModelSelector.tsx: it imports accordion primitives from@/components/ui/accordion, which conflicts with frontend standards that forbid using components fromweb/src/componentsin this area. - Because the reported problem is a standards violation (severity 7/10, confidence 8/10) and not a confirmed user-facing bug, the merge risk stays moderate-low if addressed soon.
- Pay close attention to
web/src/refresh-components/popovers/ModelSelector.tsx- replace forbidden accordion imports with the approved Opal/allowed alternative to avoid architecture drift.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/src/refresh-components/popovers/ModelSelector.tsx">
<violation number="1" location="web/src/refresh-components/popovers/ModelSelector.tsx:28">
P1: Custom agent: **Frontend standards**
Frontend standards forbid using components from `web/src/components`, but this file imports the accordion primitives from `@/components/ui/accordion`. Replace them with an Opal or refresh-component equivalent instead of the legacy components.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Preview Deployment
|
ModelSelector component with add/remove/replace semantics for up to 3 concurrent LLMs. useMultiModelChat hook manages model selection state and streaming coordination.
Replace hand-rolled ModelPill with opal SelectButton, custom ModelItem with LineItem, raw Radix AccordionPrimitive with shared Accordion, and reuse buildLlmOptions/groupLlmOptions from LLMPopover.
…tion headers ModelSelector was reimplementing LLMPopover internals (search, accordion, item rendering). Extract shared ModelListContent component that uses the ResourcePopover pattern — flat section headers with provider icon + separator line instead of collapsible accordions. ModelSelector now just handles selection logic and pill rendering.
nmgarza5
force-pushed
the
multi-model-4a-selector
branch
from
f7c1f8b to
2d3c838
Compare
Popover flew to upper-left at 3 models because no Trigger existed. Wrap pills in Popover.Anchor so content positions correctly. Leading separator now only shown when + button is visible.
Match Figma mock: rightIcon={SvgX} puts the remove icon inside the
pill. Size md makes pills larger. Clicking the pill removes the model.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/src/refresh-components/popovers/ModelSelector.tsx">
<violation number="1" location="web/src/refresh-components/popovers/ModelSelector.tsx:169">
P2: Clicking a model pill in multi‑model mode now removes it immediately, so the replace flow (via the popover) is no longer reachable for multi‑model selections. Use the existing `handlePillClick` to open the popover for replacement, consistent with the single‑model path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
🖼️ Visual Regression Report
|
- Fix model_provider mapping: use m.provider instead of m.name - Pass isLoading to ModelListContent to show loading state - Cap restoreFromModelNames at MAX_MODELS - Remove redundant llmManager from useEffect deps - Use Separator component instead of raw divs - Add two-click-zone on SelectButton (main area→replace, X→remove)
- Replace raw div separators with Separator component - Change size from md to lg per design mock - Add two-click-zone: main area opens replace popover, X icon removes
Replace static Popover.Anchor with virtualRef so the model list
positions above whichever button was clicked (+ button or pill).
Also add avoidCollisions={false} to prevent Radix from flipping
the popover to the bottom.
Hook logic is simple enough to not warrant a standalone test file.
Switch from @/components/ui/accordion (legacy) to @/refresh-components/Collapsible (Radix wrapper). Matches the visual layout of the existing LLMPopover: collapsible groups with provider icons, chevron indicators, auto-expand selected group, force-expand all on search.
Replace raw <button> and wrapper <div>s with LineItem and Section from the component library.
nmgarza5
commented
Apr 2, 2026
Comment on lines
+13
to
+17
| import { | ||
| Collapsible, | ||
| CollapsibleContent, | ||
| CollapsibleTrigger, | ||
| } from "@/refresh-components/Collapsible"; |
Contributor
Author
There was a problem hiding this comment.
i just realized we have this web/src/refresh-components/popovers/LLMPopover.tsx
going to try and consolidate really quick
LLMPopover now delegates search, grouping, and model list rendering to ModelListContent, eliminating duplicated code. Keeps popover wrapper, trigger button, and temperature slider.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
ModelSelectorcomponent anduseMultiModelChathook for multi-model chat support.ModelSelector.tsx— popover-based model picker using opalSelectButton/OpenButton, reusesLLMPopoverpatterns (accordion grouping,LineItemrendering, search filtering)useMultiModelChat.ts— hook managing selected model state (add/remove/replace up to 3 models)useMultiModelChat.test.tsx— unit tests for the hookStacked on #9648 (PR3: frontend types).
How Has This Been Tested?
Additional Options