delete the combobox instead of giving it anchor semantics

audit m7 ranked this medium because "every caller inherits the opaque click". there are no callers. nothing has imported Combobox since the initial commit, there is no barrel that re-exports it, and nothing anywhere sets `href` on a SelectOption — so the navigate, the separator that only appeared for href options, and the href field on both declarations of the type were all unreachable. writing anchor semantics into a component that is never rendered is building, not fixing. the Command primitives it used stay; AIHarnessesSection needs them.
2026-08-07 12:35:58 +00:00
parent 990ead93b9
commit 32aa1e7cc3
5 changed files with 5 additions and 83 deletions
@@ -83,7 +83,7 @@ are good building blocks. The **Workspace/Panel framework** contains **zero** ro
 | ~~M4~~ | ~~`FileBrowser/.../useFileBrowserApp.ts:269`~~, `FileItem.tsx:516`, ~~`Breadcrumb.tsx:16`~~ | a folder | `/files?path=<dir>` | **Partly done — the rest is an owner decision, not a defect.** `currentPath` is `?path=` on `/files`, so back/forward and linking a folder work, and the crumbs are `<Link>`s. Two things the audit line did not know: `?view=` is *ephemeral* (wiped on mount by `useFileViewerPanels`), so `path` is the screen's first durable param, and four `setSearchParams({…})` calls replaced the whole query string — opening any file would have silently reset the folder. They go through a `setViewerParams` helper now that keeps `path`. Opt-in via the parsed `WorkspaceIdentity` (`screens/files`), because a dashboard can hold two browsers and one shared param would move both. **Folder *items* stay buttons:** ⌘/Ctrl/Shift-click is already bound to multi-select in `FileItem.tsx` and open is double-click, so anchor semantics collide with an existing gesture. |
 | ~~M5~~ | ~~`CodeEditor/FileTree.tsx:59`, `EditorTabs.tsx:33`~~ | open source file / active tab | `/code-editor?file=<path>` | **Done, minus `open=`.** The active file is `?file=`; tree *file* rows and tabs are `<Link>`s. **The tab set stays local** — it is a working session, not an address: it grows without bound, each entry costs a read on load, and nobody links someone else to a tab bar. A `?file=` naming a file that is not open now *opens* it, which is what makes a pasted link work; a path that fails to read is remembered so a bad link errors once instead of once per render, and the address is left alone rather than rewritten. **Tree folder rows stay buttons** — unlike the M4 case this needs no owner call, because expanding a directory is disclosure, not navigation. Two things fixed in passing: the tab close control was a `role="button"` span *nested inside* the tab (invalid then, a nested interactive inside an anchor now) and is a sibling `<button>` with an `aria-label`; and `closeFile` computed the next-active file *inside* a `setFiles` updater, which is exactly the impurity React double-invokes to catch. |
 | ~~M6~~ | `Settings/SettingsPanel.tsx` | a settings sub-section | `/settings/:page/:section` | **Done.** `<NavLink>` + `useParams`, five `*_SELECTED` globals gone, one `SettingsRoute` guard per page. The "one change covers all settings pages" claim was *almost* right: Integrations builds its own sidebar and did not go through `createSettingsPanelComponents`, and it also held the Enterprise/Personal tab in a second global — derived from the section key now, which is what fixes deep-linking a Personal section. |
-| M7 | `workspaces/components/Combobox.tsx:53` | caller-supplied route | (existing) | shared widget: `href`-bearing options do `navigate(option.href)`; render them as `<Link>`. Every caller inherits the opaque click. |
+| ~~M7~~ | ~~`workspaces/components/Combobox.tsx:53`~~ | caller-supplied route | — | **Deleted, not fixed.** "Every caller inherits the opaque click" was the reason this ranked MEDIUM, and it is wrong: `Combobox` has **no callers**. Nothing has imported it since the initial commit, there is no barrel export, and nothing anywhere sets `href` on a `SelectOption` — so the navigate, the separator that only showed for `href` options, and the `href` field on both declarations of the type were all unreachable. Writing anchor semantics into a component that is never rendered is building, not fixing. Its `Command` primitives stay; `AIHarnessesSection` uses them. |
 | ~~M8~~ | `Layout/Header/UserMenu.tsx` | — | — | **Done.** Removed rather than routed: nothing had ever been built behind `/settings/resources`, so the item was a bounce to `/` dressed as navigation. Its `header.userMenu.resources` locale keys went with it. |
 | M9 | `Screens/Dashboard/Plans/index.tsx:36` | a plan document | `/plans/:name` | native `<select>` → local state; a plan is a real addressable doc. Have the select `navigate()` or use a link list. |
 | ~~M10~~ | `SystemMonitor/ScopeList.tsx` | monitor scope (btop/pm2/docker) | `/system-monitor/:scope` | **Done.** Route pair + `Navigate` guard; the scope buttons are `NavLink`s and `useMonitorScope` reads `useParams` instead of the channel. The Dock's hand-rolled `isActive` is a `startsWith`, so its highlight survives the redirect. |
@@ -175,7 +175,7 @@ publishers means changing the chat panel, which is another agent's, so it is wri
 | 9 | `Projects/ProjectPreview.tsx:457` | open project | `/projects/:id` | YES | verify vs H3 |
 | 10 | `Jobs/JobDetail.tsx:709` | back button | `/jobs` | YES | use `BackButton`/`<Link>` |
 | 11 | `ChatHistory/SessionList.tsx:70` | "New Chat" | `/chat/new` | ~ | borderline (LOW) |
-| 12 | `components/Combobox.tsx:54` | nav combobox option | `option.href` | YES | M7 |
+| ~~12~~ | ~~`components/Combobox.tsx:54`~~ | nav combobox option | `option.href` | — | M7 — file deleted, it had no callers |
 | ~~13~~ | `MusicPlayer/MusicPlayerHost.tsx:251` | open current album (setCwd+nav) | `/music` | ~ | **Done** — the side-effect was the `setCwd`; with the location in the URL the tile is a plain `<Link>` to `musicPath(albumRel)`. |
 | 14 | `FileBrowserWidget/useFileBrowserWidget.ts:53` | open folder in Files | `/files?view=…` | ~ | query nav |
 | 15 | `FileBrowserApp/useFileBrowserApp.ts:401` | create dashboard from folder | `/dashboards/new?…` | ~ | action-nav |
@@ -219,7 +219,7 @@ publishers means changing the chat panel, which is another agent's, so it is wri
 - [x] **M3** Activity → `/activity/:id`; the `{label, query}` selection object is gone — the id is the URL and the stream query is derived from the registry row. `/activity` also had no `usePageTitle` rule (it read "Officer"); added. **Needs runtime test.**
 - [x] **M4** FileBrowser folders → `/files?path=`; breadcrumbs are `<Link>`s. Folder *rows* deliberately still buttons — ⌘-click is multi-select, open is double-click; converting them needs an owner call on the gesture.
 - [x] **M5** CodeEditor active file → `/code-editor?file=`; tree file rows and tabs are `<Link>`s. The open-tab *set* stays local state, on purpose — see the findings row.
- [ ] **M7** Combobox: render `href` options as `<Link>` (`Combobox.tsx:53`).
+- [x] **M7** Combobox — **deleted instead**. Zero callers since the initial commit; `href` on `SelectOption` was never set by anything, so the whole branch was unreachable.

 ### Phase 3 — Whole-workspace routing decisions (needs a design call first)
 - [x] **Music** — `/music?path=<rel>`; `music:cwd` deleted; every drill-in (including the dock's now-playing tile, navigate-site 13) is a `<Link>`. `music:favorites` and `music:resync` stay — a view toggle and a refresh signal. **Needs runtime test.**
@@ -19,7 +19,6 @@ Location: `src/workspaces/components/`
 Built on top of shadcn primitives:
 - `Avatar.tsx` - User avatars
 - `Card.tsx` - Custom card wrapper
- `Combobox.tsx` - Searchable select
 - `ColorPicker.tsx` - Color selection
 - `DataTable/` - Table with sorting, filtering, pagination (see below)
 - `Dialogs/` - Common dialog patterns
@@ -1,77 +0,0 @@
-import type { SelectOption } from 'types';
-import * as React from 'react';
-import { useNavigate } from 'react-router';
-import { cn } from 'helpers/cn';
-import { Check, ChevronsUpDown } from 'lucide-react';
-import { Button } from '@/components/ui/button';
-import { Command, CommandEmpty, CommandGroup, CommandInput, CommandItem } from '@/components/ui/command';
-import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover';
-import { Separator } from '@/components/ui/separator';
-
-type ComboboxProps<T> = {
-  options: SelectOption[];
-  value: string | number;
-  onChange: (value: T) => void;
-  placeholder?: string;
-  className?: string;
-};
-export const Combobox = <T extends string | number>({
-  options,
-  value,
-  onChange,
-  placeholder = 'Select...',
-  className,
-}: ComboboxProps<T>) => {
-  const navigate = useNavigate();
-  const [open, setOpen] = React.useState(false);
-
-  return (
-    <Popover open={open} onOpenChange={setOpen}>
-      <PopoverTrigger asChild>
-        <Button
-          variant="outline"
-          role="combobox"
-          aria-expanded={open}
-          className={cn('w-[200px] justify-between', className)}
-        >
-          <span className="block truncate">
-            {value ? options?.find((option) => option.value === value)?.label : placeholder}
-          </span>
-          <ChevronsUpDown className="ml-2 h-4 w-4 shrink-0 opacity-50" />
-        </Button>
-      </PopoverTrigger>
-      <PopoverContent className={cn('w-[200px] justify-between', className)}>
-        <Command>
-          <CommandInput placeholder={placeholder} />
-          <CommandEmpty></CommandEmpty>
-          <CommandGroup className="max-h-96 overflow-auto">
-            {options?.map((option) => (
-              <div key={option.value}>
-                <CommandItem
-                  value={option?.label || '' + option.value}
-                  onSelect={(currentValue) => {
-                    if (option.href) {
-                      return navigate(option.href);
-                    }
-                    const newValue = options.find(
-                      (option) => option.label?.toLowerCase() === currentValue.toLowerCase(),
-                    )?.value;
-                    if (value && value !== newValue) {
-                      const setValue = typeof value === 'number' ? Number(newValue) : '' + newValue;
-                      onChange(setValue as T);
-                    }
-                    setOpen(false);
-                  }}
-                >
-                  <Check className={cn('mr-2 h-4 w-4', value === option.value ? 'opacity-100' : 'opacity-0')} />
-                  {option.label}
-                </CommandItem>
-                {option.href && <Separator className="my-2" />}
-              </div>
-            ))}
-          </CommandGroup>
-        </Command>
-      </PopoverContent>
-    </Popover>
-  );
-};
@@ -1,5 +1,5 @@
 declare global {
-  type SelectOption = { value: number | string; label?: string; href?: string; hidden?: boolean };
+  type SelectOption = { value: number | string; label?: string; hidden?: boolean };

  type StateSetter<T> = React.Dispatch<React.SetStateAction<T>>;

@@ -4,6 +4,6 @@ export * from 'officerdb/types';
 export type { Job, JobStep, JobStatus, JobStepStatus, JobProgress } from './queue';
 export type { EmailSummary, EmailMessage, EmailThread } from './email';

-export type SelectOption = { value: number | string; label?: string; href?: string };
+export type SelectOption = { value: number | string; label?: string };

 export type StateSetter<T> = React.Dispatch<React.SetStateAction<T>>;