-
Notifications
You must be signed in to change notification settings - Fork 993
SUPER-755: collapse chat composer model settings into one menu (v1+v2) #4866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| import { ModelSelectorLogo } from "@superset/ui/ai-elements/model-selector"; | ||
| import { PromptInputButton } from "@superset/ui/ai-elements/prompt-input"; | ||
| import type { ThinkingLevel } from "@superset/ui/ai-elements/thinking-toggle"; | ||
| import { | ||
| DropdownMenu, | ||
| DropdownMenuContent, | ||
| DropdownMenuItem, | ||
| DropdownMenuLabel, | ||
| DropdownMenuSeparator, | ||
| DropdownMenuTrigger, | ||
| } from "@superset/ui/dropdown-menu"; | ||
| import { claudeIcon } from "@superset/ui/icons/preset-icons"; | ||
| import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; | ||
| import { | ||
| BrainIcon, | ||
| CheckIcon, | ||
| ChevronRightIcon, | ||
| ShieldCheckIcon, | ||
| ShieldIcon, | ||
| ShieldOffIcon, | ||
| } from "lucide-react"; | ||
| import type React from "react"; | ||
| import { useRef, useState } from "react"; | ||
| import { PILL_BUTTON_CLASS } from "renderer/components/Chat/ChatInterface/styles"; | ||
| import type { | ||
| ModelOption, | ||
| PermissionMode, | ||
| } from "renderer/components/Chat/ChatInterface/types"; | ||
| import { | ||
| ANTHROPIC_LOGO_PROVIDER, | ||
| providerToLogo, | ||
| } from "../../../../ModelPicker/utils/providerToLogo"; | ||
|
|
||
| interface ComposerSettingsMenuProps { | ||
| selectedModel: ModelOption | null; | ||
| setModelSelectorOpen: React.Dispatch<React.SetStateAction<boolean>>; | ||
| permissionMode: PermissionMode; | ||
| setPermissionMode: React.Dispatch<React.SetStateAction<PermissionMode>>; | ||
| thinkingLevel: ThinkingLevel; | ||
| setThinkingLevel: (level: ThinkingLevel) => void; | ||
| } | ||
|
|
||
| interface PermissionModeOption { | ||
| value: PermissionMode; | ||
| label: string; | ||
| description: string; | ||
| icon: React.ComponentType<{ className?: string }>; | ||
| } | ||
|
|
||
| const PERMISSION_MODES: PermissionModeOption[] = [ | ||
| { | ||
| value: "bypassPermissions", | ||
| label: "Auto", | ||
| description: "Tools run without approval", | ||
| icon: ShieldOffIcon, | ||
| }, | ||
| { | ||
| value: "acceptEdits", | ||
| label: "Semi-auto", | ||
| description: "Edits auto-approved, others need approval", | ||
| icon: ShieldCheckIcon, | ||
| }, | ||
| { | ||
| value: "default", | ||
| label: "Manual", | ||
| description: "All tools require approval", | ||
| icon: ShieldIcon, | ||
| }, | ||
| ]; | ||
|
|
||
| interface ThinkingLevelOption { | ||
| value: ThinkingLevel; | ||
| label: string; | ||
| description: string; | ||
| } | ||
|
|
||
| const THINKING_LEVELS: ThinkingLevelOption[] = [ | ||
| { value: "off", label: "Off", description: "No extended thinking" }, | ||
| { value: "low", label: "Low", description: "Minimal reasoning effort" }, | ||
| { | ||
| value: "medium", | ||
| label: "Medium", | ||
| description: "Moderate reasoning effort", | ||
| }, | ||
| { value: "high", label: "High", description: "Thorough reasoning effort" }, | ||
| { | ||
| value: "xhigh", | ||
| label: "Max", | ||
| description: "Maximum reasoning effort", | ||
| }, | ||
| ]; | ||
|
|
||
| export function ComposerSettingsMenu({ | ||
| selectedModel, | ||
| setModelSelectorOpen, | ||
| permissionMode, | ||
| setPermissionMode, | ||
| thinkingLevel, | ||
| setThinkingLevel, | ||
| }: ComposerSettingsMenuProps) { | ||
| const [menuOpen, setMenuOpen] = useState(false); | ||
| const pendingDialogOpenRef = useRef(false); | ||
|
|
||
| const activePermission = | ||
| PERMISSION_MODES.find((m) => m.value === permissionMode) ?? | ||
| PERMISSION_MODES[0]; | ||
| const PermissionIcon = activePermission.icon; | ||
|
|
||
| const activeThinking = | ||
| THINKING_LEVELS.find((t) => t.value === thinkingLevel) ?? | ||
| THINKING_LEVELS[0]; | ||
|
|
||
| const brainIconColor = | ||
| thinkingLevel === "off" ? "text-muted-foreground" : "text-foreground"; | ||
|
|
||
| const selectedLogo = selectedModel | ||
| ? providerToLogo(selectedModel.provider) | ||
| : null; | ||
|
|
||
| const tooltipText = `Model: ${selectedModel?.name ?? "Model"} · Permission: ${activePermission.label} · Thinking: ${activeThinking.label}`; | ||
|
|
||
| const ariaLabel = `Chat settings: model ${selectedModel?.name ?? "Model"}, permission ${activePermission.label}, thinking ${activeThinking.label}`; | ||
|
|
||
| return ( | ||
| <DropdownMenu open={menuOpen} onOpenChange={setMenuOpen}> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <DropdownMenuTrigger asChild> | ||
| <PromptInputButton | ||
| className={`${PILL_BUTTON_CLASS} px-2 gap-1 text-xs text-foreground cursor-pointer`} | ||
| aria-label={ariaLabel} | ||
| > | ||
| <PermissionIcon className="size-3.5 text-foreground" /> | ||
| {selectedLogo === ANTHROPIC_LOGO_PROVIDER ? ( | ||
| <img alt="Claude" className="size-3" src={claudeIcon} /> | ||
| ) : selectedLogo ? ( | ||
| <ModelSelectorLogo provider={selectedLogo} /> | ||
| ) : null} | ||
| <span className="max-w-[180px] truncate"> | ||
| {selectedModel?.name ?? "Model"} | ||
| </span> | ||
| <BrainIcon className={`size-3.5 ${brainIconColor}`} /> | ||
| </PromptInputButton> | ||
| </DropdownMenuTrigger> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>{tooltipText}</p> | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| <DropdownMenuContent | ||
| align="start" | ||
| className="w-64" | ||
| onCloseAutoFocus={(event) => { | ||
| if (pendingDialogOpenRef.current) { | ||
| event.preventDefault(); | ||
| pendingDialogOpenRef.current = false; | ||
| setModelSelectorOpen(true); | ||
| } | ||
| }} | ||
| > | ||
| <DropdownMenuLabel>Permission</DropdownMenuLabel> | ||
| {PERMISSION_MODES.map((mode) => { | ||
| const Icon = mode.icon; | ||
| const isActive = mode.value === permissionMode; | ||
| return ( | ||
| <DropdownMenuItem | ||
| key={mode.value} | ||
| onSelect={() => setPermissionMode(mode.value)} | ||
| className="flex items-center gap-2" | ||
| > | ||
| <Icon className="size-4 shrink-0" /> | ||
| <div className="flex flex-1 flex-col gap-0.5"> | ||
| <span className="text-sm font-medium">{mode.label}</span> | ||
| <span className="text-xs text-muted-foreground"> | ||
| {mode.description} | ||
| </span> | ||
| </div> | ||
| {isActive && <CheckIcon className="size-4 shrink-0" />} | ||
| </DropdownMenuItem> | ||
| ); | ||
| })} | ||
|
|
||
| <DropdownMenuSeparator /> | ||
|
|
||
| <DropdownMenuLabel>Thinking</DropdownMenuLabel> | ||
| {THINKING_LEVELS.map((level) => { | ||
| const isActive = level.value === thinkingLevel; | ||
| return ( | ||
| <DropdownMenuItem | ||
| key={level.value} | ||
| onSelect={() => setThinkingLevel(level.value)} | ||
| className="flex items-center gap-2" | ||
| > | ||
| <div className="flex flex-1 flex-col gap-0.5"> | ||
| <span className="text-sm font-medium">{level.label}</span> | ||
| <span className="text-xs text-muted-foreground"> | ||
| {level.description} | ||
| </span> | ||
| </div> | ||
| {isActive && <CheckIcon className="size-4 shrink-0" />} | ||
| </DropdownMenuItem> | ||
| ); | ||
| })} | ||
|
|
||
| <DropdownMenuSeparator /> | ||
|
|
||
| <div className="p-1"> | ||
| <button | ||
| type="button" | ||
| className="flex w-full items-center justify-between gap-2 rounded-md border border-border bg-foreground/[0.04] px-2 py-1.5 text-xs text-foreground transition-colors hover:bg-foreground/[0.08] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" | ||
| aria-label={`Change model. Current model: ${selectedModel?.name ?? "Model"}`} | ||
| onClick={() => { | ||
| pendingDialogOpenRef.current = true; | ||
| setMenuOpen(false); | ||
| }} | ||
| > | ||
| <span className="flex items-center gap-2 min-w-0"> | ||
| {selectedLogo === ANTHROPIC_LOGO_PROVIDER ? ( | ||
| <img alt="Claude" className="size-3" src={claudeIcon} /> | ||
| ) : selectedLogo ? ( | ||
| <ModelSelectorLogo provider={selectedLogo} /> | ||
| ) : null} | ||
| <span className="truncate">{selectedModel?.name ?? "Model"}</span> | ||
| </span> | ||
| <span className="flex items-center gap-0.5 text-muted-foreground"> | ||
| Change | ||
| <ChevronRightIcon className="size-3" /> | ||
| </span> | ||
| </button> | ||
| </div> | ||
|
Comment on lines
+207
to
+230
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The footer model-change element is a raw Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/components/ChatComposerControls/ComposerSettingsMenu/ComposerSettingsMenu.tsx
Line: 207-230
Comment:
**"Change model" button unreachable via arrow-key navigation**
The footer model-change element is a raw `<button>` rather than a `DropdownMenuItem`. Radix `DropdownMenu` arrow-key traversal only visits `DropdownMenuItem` components; this button is skipped entirely and is only reachable via Tab. Users who open the dropdown with the keyboard and navigate by arrow keys will never land on "Change model". Consider wrapping the content in a `DropdownMenuItem` with `onSelect` instead of `onClick` so it participates in the same arrow-key flow as the other items. The same pattern applies to the byte-identical v2 file.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+207
to
+230
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
FILE="apps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/components/ChatComposerControls/ComposerSettingsMenu/ComposerSettingsMenu.tsx"
echo "== File exists =="
ls -l "$FILE" || { echo "missing"; exit 1; }
echo "== Lines 180-260 =="
sed -n '180,260p' "$FILE" | nl -ba
echo "== Lines 1-90 (imports/context) =="
sed -n '1,90p' "$FILE" | nl -ba
echo "== Grep for DropdownMenuItem usage in this file =="
rg -n "DropdownMenuItem|DropdownMenuContent|DropdownMenu" "$FILE"
echo "== Search for pendingDialogOpenRef/setMenuOpen usage =="
rg -n "pendingDialogOpenRef|setMenuOpen" "$FILE"Repository: superset-sh/superset Length of output: 374 🌐 Web query:
💡 Result: Summary answer: For a dropdown menu composed of a trigger button plus a menu content/panel, accessibility-focused implementations manage keyboard focus among menu items (often via “roving tabindex” / focus management) and define a clear keyboard interaction model (Arrow keys move focus, Enter/Space activates, Esc closes). In Radix UI’s DropdownMenu, focus management is fully managed, uses roving tabindex per the Menu Button WAI-ARIA design pattern, and ArrowDown/ArrowUp move focus through items while Esc returns focus to the trigger.[1][2] 1) What “roving focus” means for DropdownMenuItem - The roving tabindex (roving focus) pattern keeps only one item focusable at a time (typically tabindex="0" on the active item, tabindex="-1" on the rest) and updates which element receives DOM focus as the user presses arrow keys. This enables arrow-key navigation within a composite widget without forcing users to Tab through every item.[3][1] 2) Radix UI DropdownMenu: evidence of roving tabindex + keyboard behavior - Radix UI Primitives explicitly states that DropdownMenu uses roving tabindex to manage focus movement among menu items and adheres to the Menu Button WAI-ARIA design pattern.[1] - Radix UI Primitives also documents the keyboard interactions: when the dropdown is open, ArrowDown moves focus to the next item and ArrowUp moves focus to the previous item; Esc closes the menu and moves focus to the trigger; Enter opens/activates items; Space opens/activates items.[1] 3) Superset UI / Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
FILE="apps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/components/ChatComposerControls/ComposerSettingsMenu/ComposerSettingsMenu.tsx"
echo "== Imports (lines 1-80) =="
sed -n '1,80p' "$FILE" | awk '{printf "%d\t%s\n", NR, $0}'
echo "== Menu section (lines 180-260) =="
sed -n '180,260p' "$FILE" | awk '{printf "%d\t%s\n", NR, $0}'
echo "== Grep within file =="
rg -n "DropdownMenu(Content|Item)|DropdownMenuTrigger|`@superset/ui/dropdown-menu`|providerToLogo|Claude|pendingDialogOpenRef" "$FILE" || trueRepository: superset-sh/superset Length of output: 5419 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find tsconfig files
ls -la tsconfig*.json 2>/dev/null || true
find . -maxdepth 4 -name "tsconfig.json" -o -name "tsconfig.*.json" | sed -n '1,40p'
# Show compilerOptions.paths if present
for f in $(find . -maxdepth 4 -name "tsconfig*.json"); do
echo "== $f =="
rg -n "\"paths\"" "$f" >/dev/null 2>&1 && sed -n '1,200p' "$f" | rg -n "paths|baseUrl" || true
rg -n "\"paths\"" "$f" && sed -n '1,240p' "$f" | rg -n "\"paths\"|baseUrl|paths" -n || true
doneRepository: superset-sh/superset Length of output: 3011 🏁 Script executed: #!/bin/bash
set -euo pipefail
TS="apps/desktop/tsconfig.json"
FILE="apps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/components/ChatComposerControls/ComposerSettingsMenu/ComposerSettingsMenu.tsx"
echo "== apps/desktop/tsconfig.json: compilerOptions.paths (full) =="
sed -n '1,220p' "$TS" | awk '{printf "%d\t%s\n", NR, $0}' | sed -n '1,220p'
echo "== ComposerSettingsMenu.tsx: menu context (110-260) =="
sed -n '110,260p' "$FILE" | awk '{printf "%d\t%s\n", NR, $0}'
echo "== providerToLogo import usage across apps/desktop (rg) =="
rg -n "providerToLogo" apps/desktop/src/renderer | head -n 50Repository: superset-sh/superset Length of output: 11018 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find the dropdown-menu implementation in repo
fd -t f "dropdown-menu" . | head -n 50
# Search in packages/ui for exported dropdown components
rg -n "export (type )?(DropdownMenu|DropdownMenuItem|DropdownMenuContent|DropdownMenuTrigger)" packages/ui -S || true
rg -n "DropdownMenuItem" packages/ui -S | head -n 50
# Also search for '`@radix-ui/react-dropdown-menu`' usage
rg -n "`@radix-ui/react-dropdown-menu`" packages/ui -S || true
rg -n "roving|roving tabindex|menuitem" packages/ui -S || trueRepository: superset-sh/superset Length of output: 8448 🏁 Script executed: #!/bin/bash
set -euo pipefail
FILE="packages/ui/src/components/ui/dropdown-menu.tsx"
echo "== File: $FILE =="
wc -l "$FILE"
echo "== DropdownMenuItem component =="
rg -n "function DropdownMenuItem|const DropdownMenuItem|export.*DropdownMenuItem" "$FILE"
sed -n '1,140p' "$FILE" | awk '{print NR"\t"$0}'
echo "== DropdownMenuContent component =="
rg -n "function DropdownMenuContent|const DropdownMenuContent" "$FILE"
sed -n '140,260p' "$FILE" | awk '{print NR"\t"$0}'Repository: superset-sh/superset Length of output: 9191 Use a A plain Proposed fix- <div className="p-1">
- <button
- type="button"
- className="flex w-full items-center justify-between gap-2 rounded-md border border-border bg-foreground/[0.04] px-2 py-1.5 text-xs text-foreground transition-colors hover:bg-foreground/[0.08] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
- aria-label={`Change model. Current model: ${selectedModel?.name ?? "Model"}`}
- onClick={() => {
- pendingDialogOpenRef.current = true;
- setMenuOpen(false);
- }}
- >
+ <div className="p-1">
+ <DropdownMenuItem
+ className="flex w-full items-center justify-between gap-2 rounded-md border border-border bg-foreground/[0.04] px-2 py-1.5 text-xs text-foreground transition-colors hover:bg-foreground/[0.08] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
+ aria-label={`Change model. Current model: ${selectedModel?.name ?? "Model"}`}
+ onSelect={() => {
+ pendingDialogOpenRef.current = true;
+ setMenuOpen(false);
+ }}
+ >
<span className="flex items-center gap-2 min-w-0">
{selectedLogo === ANTHROPIC_LOGO_PROVIDER ? (
<img alt="Claude" className="size-3" src={claudeIcon} />
) : selectedLogo ? (
<ModelSelectorLogo provider={selectedLogo} />
) : null}
<span className="truncate">{selectedModel?.name ?? "Model"}</span>
</span>
<span className="flex items-center gap-0.5 text-muted-foreground">
Change
<ChevronRightIcon className="size-3" />
</span>
- </button>
+ </DropdownMenuItem>
</div>🤖 Prompt for AI Agents |
||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { ComposerSettingsMenu } from "./ComposerSettingsMenu"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModelSelectorLogofragment is copy-pasted verbatim in both the trigger (lines 134-138) and the footer "Change model" button (lines 218-222). If the rendering logic ever needs to change (e.g., ifModelSelectorLogogains anisAnthropicSafeprop), it will silently diverge. Extract to a localProviderLogohelper so the two render sites share one source of truth.Prompt To Fix With AI