From 8b4fa605967bf2876582e6d7ce19bad549a8a278 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 13:48:32 +0900 Subject: [PATCH 01/19] docs(devlog): plan the Models workspace tab consolidation --- .../260807_models_workspace_tabs/000_plan.md | 113 +++++++++++ .../010_phase1_routing_layer.md | 111 +++++++++++ .../020_phase2_models_shell.md | 151 +++++++++++++++ .../030_phase3_combos_embed.md | 178 ++++++++++++++++++ .../040_phase4_routing_embed_and_sidebar.md | 143 ++++++++++++++ 5 files changed, 696 insertions(+) create mode 100644 devlog/_plan/260807_models_workspace_tabs/000_plan.md create mode 100644 devlog/_plan/260807_models_workspace_tabs/010_phase1_routing_layer.md create mode 100644 devlog/_plan/260807_models_workspace_tabs/020_phase2_models_shell.md create mode 100644 devlog/_plan/260807_models_workspace_tabs/030_phase3_combos_embed.md create mode 100644 devlog/_plan/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md diff --git a/devlog/_plan/260807_models_workspace_tabs/000_plan.md b/devlog/_plan/260807_models_workspace_tabs/000_plan.md new file mode 100644 index 000000000..48c947ec6 --- /dev/null +++ b/devlog/_plan/260807_models_workspace_tabs/000_plan.md @@ -0,0 +1,113 @@ +# 260807 — Models workspace tabs (Models / Combos / Routing) + +## Objective + +Fold three sidebar destinations into one tabbed page. The Models page becomes a +three-tab workspace — **Models** (catalog), **Combos**, **Routing (beta)** — and the +sidebar drops from eleven rows to nine. + +The three tabs are not three unrelated screens sharing a container. They are the same +question asked at three depths, and the answer to all three is a model id the client +can call: + +| Tab | Question | What the client sees | +|-----|----------|----------------------| +| Models | what is visible | `anthropic/claude-opus-5` | +| Combos | who answers, in the order I chose | `combo/` | +| Routing | who answers, chosen by score | `policy/` | + +A combo and a routing profile are both virtual models that resolve to a real one; one +is manual (ordered failover / round-robin), the other automatic (hard requirements plus +a score). Grouping them under Models makes the page title honest rather than merely +shorter. + +## Why the sidebar loses two rows + +`Routing (beta)` moves into the strip. `Claude` goes away because it was never a page: +it is a shortcut into a tab of Integrations, and paying for it is `isNavEntryActive()` +in `gui/src/App.tsx` — a function whose entire job is stopping the sidebar from +claiming the user is in two places at once. Remove the duplicate row and the +correction disappears with it. + +Combos is a special case worth stating plainly: **it is already not in the sidebar.** +The NAV array has no `combos` entry, and the only route to `#combos` today is a +`Set up` link on a card inside the Models page. So for Combos this change is not one +level deeper — it is one level shallower. A card link that swaps the whole page becomes +a sibling tab. + +## Constraints + +- Hash is the source of truth. Refresh, bookmark, and Back/Forward keep the tab. + Precedent: `#logs` / `#logs/debug` in `gui/src/pages/Logs.tsx`. +- A hidden panel must not do work. Routing polls analytics and Combos fires three + parallel fetches; both must be gated by an `active` prop. +- Combos holds unsaved editor drafts. Panels mount lazily and then stay mounted so a + half-typed combo survives a tab hop. +- No `src/` runtime change. This is a GUI navigation refactor; the proxy, the routing + engine, and every management API contract stay exactly as they are. + +## External evidence + +Three findings changed or confirmed decisions here. All were verified by opening the +source, not from search snippets. + +**Primer, [UnderlineNav guidelines](https://primer.style/product/components/underline-nav/guidelines/) +and [navigation patterns](https://primer.style/product/ui-patterns/navigation/)** — do not +stack multiple underline tab rows directly on top of each other; and a tab that changes +the URL is `UnderlineNav`, while a tab that only swaps visible content without touching +the URL is `UnderlinePanels`. This is the direct warrant for two decisions: every page +tab here gets its own hash, and the Combos detail panel's inner `Config` / `About` +underline row must stop being an underline row (phase 3). + +**Carbon, [tabs usage](https://carbondesignsystem.com/components/tabs/usage/)** — at most +six tabs, and tab variants "should never be nested within each other." Three is +comfortable. Integrations already runs eleven and reads as a second navigation bar +rather than one page's facets; that is the shape being avoided, not copied. + +**W3C, [WAI-ARIA `tab` role](https://www.w3.org/TR/wai-aria/#tab) and the +[APG tabs pattern](https://www.w3.org/WAI/ARIA/apg/patterns/tabs/)** — `tab` elements MUST +be contained in a `tablist`; roving tabindex puts `0` on the active tab and `-1` on the +rest; Left/Right wrap, Home/End jump; an inactive panel SHOULD be hidden, and the APG +examples use the native `hidden` attribute, which is what the existing Logs code already +does. + +Worth recording honestly: **the accessibility specs do not forbid nested tabs.** No +opened W3C/APG page prohibits a `tablist` inside a `tabpanel`, provided the inner set is +an independently labelled composite with its own roving-tabindex scope. So demoting the +Combos inner tabs is a *visual* decision backed by Primer and Carbon, not an +accessibility fix. The plan should not claim otherwise. + +One lane produced weaker evidence and is recorded as such. A survey of comparable +products (Portkey, OpenRouter, Cloudflare AI Gateway, Kong, Vercel AI Gateway) found +that most keep the model catalog documented separately from routing/fallback config; +only Vercel nests fallbacks under models-and-providers, and that page could not be +opened (`candidate — unverified`). This is documentation structure, not UI navigation, +so it is not treated as evidence for or against this design. + +## Work-phase map + +Dependency-ordered. Each phase is one full PABCD cycle and one commit series. + +| Phase | Doc | Deliverable | Depends on | +|-------|-----|-------------|------------| +| wp01 | `010_routing_layer.md` | Nested hash contract, legacy redirects, `Page` union, tests | — | +| wp02 | `020_models_shell.md` | Tab strip, lazy-mount panels, catalog tab body | wp01 | +| wp03 | `030_combos_embed.md` | Combos as a panel: full-bleed reconciliation, inner tabs demoted, `active` gating | wp02 | +| wp04 | `040_routing_embed_and_sidebar.md` | Routing as a panel, sidebar cleanup, i18n, closing gates | wp02 | + +wp03 and wp04 both depend on wp02 but not on each other; they are still run in order +because they touch the same `Models.tsx` panel block. + +## Out of scope + +`src/` runtime, `src/routing/` engine behaviour, management API contracts, docs-site, +release, and promotion to `main`/`preview`. No push and no PR without explicit +approval. + +## Verification + +Every phase ends green on `bun run typecheck`, `bun run test`, `bun run lint:gui`, and +`bun run build:gui`. The final phase additionally requires live browser observation +(C-RENDER-GROUNDING-01): drive all three tabs, refresh on each, Back/Forward, and +arrow-key traversal against the running dashboard, read the screenshots back, and fix +what observation reveals. Static gates passing is not the same as the thing working. diff --git a/devlog/_plan/260807_models_workspace_tabs/010_phase1_routing_layer.md b/devlog/_plan/260807_models_workspace_tabs/010_phase1_routing_layer.md new file mode 100644 index 000000000..2fb05d1e6 --- /dev/null +++ b/devlog/_plan/260807_models_workspace_tabs/010_phase1_routing_layer.md @@ -0,0 +1,111 @@ +# Phase 1 — Routing layer + +Owns the hash contract. Nothing renders differently after this phase; the point is +that the router can already describe the destination before any component exists to +fill it. Same order the `#debug` → `#logs/debug` move used. + +**This phase is purely additive and stays green.** The `Page` union keeps `"combos"` +and `"routing"` until phase 2. The first draft of this plan removed them here, which +would have made every `page === "combos"` comparison in `App.tsx` a type error and +left one commit knowingly red — a red commit is not a checkpoint, it is a broken +bisect point. Removing a page and adding the tab that replaces it is one atomic +change, so both belong to phase 2. + +## Target contract + +| Hash | Page | Tab | +|------|------|-----| +| `models` | models | Models (catalog) | +| `models/combos` | models | Combos | +| `models/routing` | models | Routing | +| `combos` | models | → replace to `models/combos` | +| `routing` | models | → replace to `models/routing` | + +Redirects are passive (`replaceState`), so Back is never trapped on a URL the router +immediately corrects. That is the existing `resolveAppHashChange` contract, not a new +rule. + +## MODIFY `gui/src/app-routing.ts` + +### 1. Add the tab hash list + +Placed next to `DASHBOARD_TAB_HASHES`, same shape: + +```ts +/** + * Models owns three tabs. Catalog is the bare `#models`, so it has no suffix entry + * here — same convention as Dashboard's Overview. + */ +export const MODELS_TAB_HASHES = ["models/combos", "models/routing"] as const; +``` + +### 2. Teach `hashBelongsToPage` the nested hashes + +```diff + return rawHash === page + || (page === "logs" && rawHash === "logs/debug") ++ || (page === "models" && (MODELS_TAB_HASHES as readonly string[]).includes(rawHash)) + || (page === "dashboard" && ... +``` + +### 3. Nothing else changes here + +`readPageFromHash` already answers `models` for `models/combos` and `models/routing`, +because it reads the first `/`-separated segment. The legacy `#combos` / `#routing` +redirects and the `Page` union removal are phase 2, where a destination exists to +redirect to. + +## NEW `gui/src/pages/models-tab.ts` + +Mirrors `gui/src/pages/logs-tab-keydown.ts`. Kept out of `Models.tsx` because that +file is already 1432 lines and this is the part the tests want to import directly. + +```ts +import { navigateHash, normalizeHashPath } from "../hash-routing"; + +export type ModelsTab = "catalog" | "combos" | "routing"; + +export const MODELS_TABS: readonly ModelsTab[] = ["catalog", "combos", "routing"]; + +export function modelsTabHash(tab: ModelsTab): string { + return tab === "catalog" ? "models" : `models/${tab}`; +} + +export function readModelsTab(hash = window.location.hash): ModelsTab { + const raw = normalizeHashPath(hash); + if (raw === "models/combos") return "combos"; + if (raw === "models/routing") return "routing"; + return "catalog"; +} + +export function selectModelsTab(next: ModelsTab): void { + navigateHash(modelsTabHash(next)); +} + +export function modelsTabDomId(tab: ModelsTab): string { return `models-tab-${tab}`; } +export function modelsPanelDomId(tab: ModelsTab): string { return `models-panel-${tab}`; } +``` + +`catalog` is the internal id; the visible label is `Models` (user's call — the page is +"models" and the first tab is the plain list of them). The id stays distinct so the +code never has to disambiguate `models` the page from `models` the tab. + +## NEW `tests/models-workspace-tabs.test.ts` + +Phase-1 half (routing only — component assertions land in later phases): + +- `readModelsTab` maps all three hashes and defaults unknown input to `catalog`. +- `modelsTabHash` round-trips every tab through `readModelsTab`. +- `hashBelongsToPage("models/combos", "models")` and `("models/routing", "models")` + are both true. +- `hashBelongsToPage` rejects an invented `models/nope`, so normalization strips it. +- `readPageFromHash("models/combos")` is `models` — the first segment wins. + +No existing test changes in this phase. `tests/routing-intelligence-ui.test.ts` still +describes Routing as a top-level page and still passes, because the union is untouched. + +## Verification + +All four gates stay green: `bun run typecheck`, `bun run test`, `bun run lint:gui`, +`bun run build:gui`. Nothing in this phase can break a render path, because nothing +reads the new module yet. diff --git a/devlog/_plan/260807_models_workspace_tabs/020_phase2_models_shell.md b/devlog/_plan/260807_models_workspace_tabs/020_phase2_models_shell.md new file mode 100644 index 000000000..e49d5af0b --- /dev/null +++ b/devlog/_plan/260807_models_workspace_tabs/020_phase2_models_shell.md @@ -0,0 +1,151 @@ +# Phase 2 — Models shell + +The atomic phase: the `Page` union loses `combos` and `routing`, the tab strip appears, +and the panels that replace those pages mount. Splitting any of it out would leave a +commit where a page has been deleted but its replacement does not exist. + +## MODIFY `gui/src/app-routing.ts` — remove the two pages + +```diff + export type Page = + ... + | "models" +- | "combos" + | "subagents" + ... +- | "integrations" +- | "routing"; ++ | "integrations"; +``` + +Same two entries out of `VALID_PAGES`. Then the legacy ids in `readPageFromHash`, +beside the existing `debug` line: + +```ts +// Legacy: Combos and Routing used to be standalone pages; both are Models tabs now. +if (pageId === ("combos" as Page) || pageId === ("routing" as Page)) return "models"; +``` + +and the redirects in `resolveAppHashChange`, directly after the `debug` branch: + +```ts +if (rawHash === "combos" || rawHash.startsWith("combos/")) { + return { page: "models", replaceTo: "models/combos" }; +} +if (rawHash === "routing" || rawHash.startsWith("routing/")) { + return { page: "models", replaceTo: "models/routing" }; +} +``` + +The `startsWith` arm is not decoration: `#routing/foo` from an old bookmark must reach +the Routing tab rather than be normalized to a bare page that drops the destination — +the exact failure the file's `#api` comment already documents. + +## MODIFY `gui/src/App.tsx` + +`PAGE_TKEY` loses its `combos` and `routing` keys (the compiler demands it — the record +is keyed by `Page`). + +Render block: + +```diff +- {page === "models" && } +- {page === "combos" && } ++ {page === "models" && } + ... +- {page === "routing" && } +``` + +`Combos` and `RoutingProfiles` imports move out of `App.tsx` into `Models.tsx`. + +The full-bleed modifier stops asking about the page and starts asking about the tab: + +```diff +-
++
+``` + +where `modelsTab` comes from a `readModelsTab()` state synced on `hashchange` / +`popstate`, the same listener pair `useAppRouteState` already installs. + +> This is the one piece of tab knowledge that has to live in App rather than in +> Models: the `.main-inner` element is App's, and phase 3 explains why the modifier +> cannot simply move inside the page. + +NAV rows and `isNavEntryActive` are **not** touched here — that is phase 4, so the +sidebar keeps working while the page is rebuilt. + +## MODIFY `gui/src/pages/Models.tsx` + +### Tab state + +```tsx +const [tab, setTab] = useState(readModelsTab); +const [mounted, setMounted] = useState>(() => new Set([readModelsTab()])); + +const activateTab = (next: ModelsTab) => { + setTab(next); + setMounted(current => (current.has(next) ? current : new Set([...current, next]))); +}; +``` + +Copied deliberately from `Integrations.tsx`: panels mount lazily and then stay mounted +so a half-typed combo draft survives a tab hop, and the accumulation happens in the +event handler rather than an effect so a switch costs one render, not two. + +`hashchange` + `popstate` listeners call `activateTab(readModelsTab())`. + +### Strip markup + +`.page-tabs` / `.page-tab` / `.page-tab--active`, `role="tablist"`, roving tabindex, +`aria-selected`, `aria-controls`, and Arrow/Home/End — the wiring the APG requires and +that `Integrations.tsx` already implements. Each label carries a `.section-tab-meta` +count: `Models 35/273`, `Combos 3`, `Routing 2`. The class and its +`page-tab--active > .section-tab-meta` rule already exist in `styles.css`. + +Counts come from data the page already holds — `effectiveVisibleCount` / `models.length` +for the catalog and `combos.length` from the existing `combosResource`. Routing's count +needs a profile list, which the Routing panel owns; until it reports one the meta is +omitted rather than rendered as `0`, because a wrong count is worse than none. + +### Body split + +Everything currently returned by the component — rail, controls, provider list, modals +— becomes the catalog panel body. The three panels are siblings, each `hidden` when +inactive (`hidden` per the APG examples, matching the existing Logs code). + +The page header (`h2` + count) and the strip live above all three panels and stay +visible on every tab. The `page-sub` moves inside the catalog panel and gets split per +tab (phase 4 writes the copy). + +## Test additions — `tests/models-workspace-tabs.test.ts` + +- `VALID_PAGES` no longer holds `combos` or `routing`. +- `resolveAppHashChange("combos")` → `{ page: "models", replaceTo: "models/combos" }`; + same for `combos/x`, `routing`, `routing/x`. +- `Models.tsx` contains `role="tablist"`, three `page-tab` entries, and `hidden={`. +- `App.tsx` no longer contains `page === "combos"` or `page === "routing"`. + +## MODIFY `tests/routing-intelligence-ui.test.ts` + +Now genuinely stale, and the compiler cannot catch a string assertion: + +```diff +- expect(VALID_PAGES.has("routing")).toBe(true); +- expect(readPageFromHash("routing")).toBe("routing"); +- expect(hashBelongsToPage("routing", "routing")).toBe(true); +- expect(resolveAppHashChange("routing").replaceTo).toBeNull(); ++ expect(readPageFromHash("models/routing")).toBe("models"); ++ expect(hashBelongsToPage("models/routing", "models")).toBe(true); ++ expect(resolveAppHashChange("models/routing").replaceTo).toBeNull(); ++ expect(resolveAppHashChange("routing")).toEqual({ page: "models", replaceTo: "models/routing" }); +``` + +and `expect(app).toContain('page === "routing"')` becomes an assertion that +`Models.tsx` mounts `RoutingProfiles`. + +## Verification + +All four gates green. This is the phase where a mistake shows up as a blank page, so +the browser check starts here even though the formal render-grounding gate is phase 4: +load `#models`, `#models/combos`, `#models/routing` and confirm each paints. diff --git a/devlog/_plan/260807_models_workspace_tabs/030_phase3_combos_embed.md b/devlog/_plan/260807_models_workspace_tabs/030_phase3_combos_embed.md new file mode 100644 index 000000000..b61bc424f --- /dev/null +++ b/devlog/_plan/260807_models_workspace_tabs/030_phase3_combos_embed.md @@ -0,0 +1,178 @@ +# Phase 3 — Combos as a panel + +The hard phase. Combos is the only surface in the GUI that opts out of the normal +980px scrolling column: it is a full-bleed `100dvh` workspace whose rail and detail +pane scroll independently. Making it a tab means reconciling that with a page header +and a tab strip that must stay visible above it. + +## The selector that actually breaks + +```css +.main-inner.main-inner--combos > .combos-workspace-shell { flex: 1 1 auto; min-height: 0; height: 100%; ... } +``` + +`gui/src/styles.css:399`. It is a **direct-child** selector. Today `Combos` returns +`.combos-workspace-shell` as `.main-inner`'s immediate child, so it matches. + +As a tab, the shell sits inside a panel wrapper: + +``` +.main-inner--combos +├─ .page-head (header, stays visible) +├─ .page-tabs (strip, stays visible) +└─ #models-panel-combos ← new wrapper + └─ .combos-workspace-shell ← no longer a direct child +``` + +The rule stops matching, the shell loses `flex: 1 1 auto` and `min-height: 0`, and the +workspace collapses to content height inside a clipped `100dvh` parent — rail and +detail scrolling both die. + +An investigation pass reported that inserting siblings keeps the selector intact. That +is true for *siblings*, and false for the structure this phase actually builds, because +the panel wrapper adds a level. Verified by reading `gui/src/styles.css:399-405` +directly. Recording it because the wrong version of this claim would have shipped a +broken layout that typecheck and tests cannot see. + +### Fix + +Make the panel wrapper the flex child and let the shell fill it: + +```diff +-.main-inner.main-inner--combos > .combos-workspace-shell { ++.main-inner.main-inner--combos > .models-tab-panel--fill, ++.main-inner.main-inner--combos .models-tab-panel--fill > .combos-workspace-shell { + flex: 1 1 auto; + min-height: 0; + height: 100%; + display: flex; + flex-direction: column; + } +``` + +The header and strip need horizontal padding back, since `.main-inner--combos` zeroes +the container's: + +```css +.main-inner--combos > .page-head, +.main-inner--combos > .page-tabs { padding-inline: 36px; flex-shrink: 0; } +@media (max-width: 760px) { + .main-inner--combos > .page-head, + .main-inner--combos > .page-tabs { padding-inline: 18px; } +} +``` + +`flex-shrink: 0` matters: without it the header is a flex item in a fixed-height column +and gets squeezed when the workspace wants room. + +The two mobile rules (`gui/src/styles.css:1983`, `2020`) need no change — they set the +container height and padding, and both still apply. + +## Why the modifier stays in App + +`.main-inner` belongs to `App.tsx`; a page cannot add a class to its own container +without a callback or a portal. So App keeps the modifier and reads the tab (phase 2), +which is the smallest coupling available. The alternative — Models rendering its own +full-height wrapper inside the 980px column — does not work, because `.main-inner` has +`max-width: 980px` and normal padding until the modifier removes them. + +## Inactive panels + +The other two panels are `hidden`, which is `display: none` in the UA stylesheet, so +they occupy no flex space. No extra rule needed. + +## MODIFY `gui/src/pages/Combos.tsx` + +### Props + +```diff +-export default function Combos({ apiBase }: { apiBase: string }) { ++export default function Combos({ apiBase, active = true }: { apiBase: string; active?: boolean }) { +``` + +Default `true` keeps every existing call site and test honest. + +### Gate the fetch + +`Combos` fires three parallel fetches (`/api/combos`, `/api/config`, `/api/models`) on +subscription. It does **not** poll — no `pollMs` — so the risk of a permanently mounted +panel is a wasted cold load, not a background traffic leak. Still worth gating: + +```diff + const resource = useDataSurface( + `combos-workspace:${apiBase}`, + [apiBase], + loadCombos, +- { ... }, ++ { ..., enabled: active }, + ); +``` + +Care needed on the disabled render: a disabled resource yields `data: undefined` with +no skeleton and no error, and the existing fallback arrays would make `ComboWorkspace` +paint as a first-run empty state. So the disabled case must return the skeleton, not +the empty workspace. This is the one real trap in the phase. + +### Pre-existing defect found while reading + +`loadCombos` takes no `AbortSignal` and none of its three `fetch` calls pass one, so +resource cleanup cannot cancel them. Harmless today because the page only unmounts on +navigation; more visible once the panel mounts lazily. Threading the signal through is +a two-line change and belongs here rather than in a separate unit — it is the same code +being touched, and leaving a known un-cancellable fetch behind while explicitly adding +lifecycle control would be incoherent. + +### Dialogs + +Add, Remove, and Unsaved use native `showModal()`. A dialog in the browser's top layer +is not clipped by an ancestor's `hidden`. Whether an open dialog can survive a tab +switch depends on whether `hidden` on an ancestor closes it — **this must be checked in +the browser, not reasoned about.** If a modal does survive, the fix is to close open +dialogs when `active` goes false. + +## MODIFY `gui/src/components/combo-workspace-detail-panel.tsx` — inner tabs + +Currently `combos-workspace-tabs` / `combos-workspace-tab` with `role="tablist"` and +`aria-selected`. Not `.page-tabs`, but visually the same underline vocabulary, so under +the page strip it reads as two stacked underline rows — the pattern Primer names +directly. + +Demote to a pill, following `.segmented.models-segmented` at `Models.tsx:924`: + +```diff +-
+- - -
+ {/* + Embedded as a Models tab, so the page title and subtitle belong to the shell. + Rendering them here too put "Routing Intelligence (beta)" and its description on + screen twice — visible the moment the panel was opened in a browser, invisible to + every static gate. The actions stay; a heading cannot carry buttons, so they sit + in a plain toolbar row. + */} + {standalone && ( + <> +
+

{t("routing.title")}

+
+

{t("routing.subtitle")}

+ + )} +
+ +
-

{t("routing.subtitle")}

{loadError ? {t("routing.loadFailed")}: {loadError} : null} {status ? {status.message} : null} diff --git a/gui/src/pages/models-tab-strip.tsx b/gui/src/pages/models-tab-strip.tsx new file mode 100644 index 000000000..c60f1958b --- /dev/null +++ b/gui/src/pages/models-tab-strip.tsx @@ -0,0 +1,92 @@ +/** + * The Models page tab strip. + * + * Underline page tabs, the same vocabulary Logs, Dashboard, and Integrations use. ARIA + * wiring follows the APG tabs pattern: `tab` elements inside a `tablist`, roving + * tabindex (0 on the active tab, -1 on the rest), `aria-controls` to the panel, and + * Arrow/Home/End traversal. + */ +import type { KeyboardEvent } from "react"; +import { useRef } from "react"; +import { useT, type TKey } from "../i18n/shared"; +import { + MODELS_TABS, + modelsPanelDomId, + modelsTabDomId, + type ModelsTab, +} from "./models-tab"; + +const TAB_LABEL: Record = { + catalog: "models.tab.catalog", + combos: "models.tab.combos", + routing: "models.tab.routing", +}; + +export function ModelsTabStrip({ + tab, + onSelect, + meta, +}: { + tab: ModelsTab; + onSelect: (next: ModelsTab) => void; + /** + * Quiet per-tab counts. A tab whose count is not yet known is omitted rather than + * rendered as zero — an unknown catalog would otherwise claim "0/0" on a cold load + * that never fetched it, and a wrong count is worse than none. + */ + meta?: Partial>; +}) { + const t = useT(); + const refs = useRef | null>(null); + if (refs.current === null) refs.current = new Map(); + + const move = (next: ModelsTab) => { + onSelect(next); + // Focus follows selection, so keyboard traversal lands where the eye does. + window.requestAnimationFrame(() => { + refs.current!.get(next)?.focus({ preventScroll: true }); + }); + }; + + const onKeyDown = (event: KeyboardEvent) => { + const index = MODELS_TABS.indexOf(tab); + let nextIndex: number | null = null; + if (event.key === "ArrowLeft") nextIndex = (index - 1 + MODELS_TABS.length) % MODELS_TABS.length; + else if (event.key === "ArrowRight") nextIndex = (index + 1) % MODELS_TABS.length; + else if (event.key === "Home") nextIndex = 0; + else if (event.key === "End") nextIndex = MODELS_TABS.length - 1; + if (nextIndex === null) return; + event.preventDefault(); + move(MODELS_TABS[nextIndex]!); + }; + + return ( +
+ {MODELS_TABS.map(candidate => { + const active = candidate === tab; + const count = meta?.[candidate]; + return ( + + ); + })} +
+ ); +} diff --git a/gui/src/styles-models-workspace.css b/gui/src/styles-models-workspace.css index 0504ac31e..ca6802935 100644 --- a/gui/src/styles-models-workspace.css +++ b/gui/src/styles-models-workspace.css @@ -5,7 +5,18 @@ Uses only design tokens from styles.css. No gradients. ============================================================================ */ -.main-inner:has(.models-workspace-shell) { +/* + The catalog wants a wider column than the 980px default. + + Scoped to a VISIBLE catalog panel, not merely a present one: panels mount lazily and + then stay mounted so drafts survive a tab hop, so a bare `:has(.models-workspace-shell)` + keeps matching after the catalog has been opened once. Routing would then render at + 980px on a direct visit and 1200px afterwards — a width that depends on browsing + history. The standalone selector stays for the pages that render the shell outside a + tabpanel. +*/ +.main-inner:has(> .models-workspace-shell), +.main-inner:has(#models-panel-catalog:not([hidden]) .models-workspace-shell) { max-width: 1200px; } @@ -16,6 +27,14 @@ container-name: models-workspace; } +/* + Tab panels. Inactive panels carry `hidden`, which the UA stylesheet renders as + display:none, so they take no space and leave the focus order — no rule needed for + that. `--fill` marks the panel that owns a full-height workspace; the height chain + that feeds it lives with the combos rules in styles.css. +*/ +.models-tab-panel { min-width: 0; } + .models-workspace-root { display: grid; grid-template-columns: minmax(240px, 280px) minmax(0, 1fr); diff --git a/gui/src/styles.css b/gui/src/styles.css index cb41c986d..3e60304ca 100644 --- a/gui/src/styles.css +++ b/gui/src/styles.css @@ -396,7 +396,18 @@ input[type="checkbox"], input[type="radio"] { accent-color: var(--accent); } display: flex; flex-direction: column; } -.main-inner.main-inner--combos > .combos-workspace-shell { +/* + The combos workspace is a full-bleed 100dvh shell, so whatever owns the remaining + height has to be a flexible, shrinkable column. + + Two selectors, because the shell reaches this container by two different paths: as + the standalone `#combos` page it is a direct child, and as a Models tab it sits one + level down inside its tabpanel. The panel wrapper is what breaks a plain direct-child + rule — the panel becomes the flex item and the shell fills it. +*/ +.main-inner.main-inner--combos > .combos-workspace-shell, +.main-inner.main-inner--combos > .models-tab-panel--fill, +.main-inner.main-inner--combos > .models-tab-panel--fill > .combos-workspace-shell { flex: 1 1 auto; min-height: 0; height: 100%; @@ -404,6 +415,19 @@ input[type="checkbox"], input[type="radio"] { accent-color: var(--accent); } flex-direction: column; } +/* + `.main-inner--combos` zeroes the container padding, so the page chrome above the + workspace has to bring its own back. `flex-shrink: 0` keeps the header, tab strip, + and subtitle from being squeezed when the workspace wants the room. +*/ +.main-inner.main-inner--combos > .page-head, +.main-inner.main-inner--combos > .page-tabs, +.main-inner.main-inner--combos > .page-sub { + flex-shrink: 0; + padding-inline: 36px; +} +.main-inner.main-inner--combos > .page-sub { margin-bottom: 10px; } + /* ---- page header ---- */ .page-head { display: flex; align-items: center; justify-content: space-between; gap: 16px; margin-bottom: 6px; } .page-head h2 { font-size: var(--text-title); } @@ -2020,6 +2044,9 @@ button.prov-account-row.active { cursor: default; } .main-inner { padding: 22px 18px 48px; } /* The mobile app grid already reserves the top-bar row; fill only its remaining main row. */ .main-inner.main-inner--combos { padding: 0; min-height: 0; height: 100%; overflow: hidden; } + .main-inner.main-inner--combos > .page-head, + .main-inner.main-inner--combos > .page-tabs, + .main-inner.main-inner--combos > .page-sub { padding-inline: 18px; } /* settings rows: copy takes the full width, controls drop underneath */ .setting-row { flex-wrap: wrap; } .setting-row .setting-copy { flex: 1 1 100% !important; } From 39ad11a73185e074f41965b96e43736c16a58809 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 15:00:15 +0900 Subject: [PATCH 08/19] fix(gui): keep the tab workspace alive through catalog load and tab switches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found three things the green suites could not see. The catalog's loading and cold-failure branches were component-level early returns. Correct for a page that is only a catalog, wrong for a page that owns three tabs: a slow catalog replaced the entire workspace, strip included, and a cold failure left Combos and Routing unreachable. They now render inside the catalog panel. That was also what destroyed unsaved combo drafts on a tab switch, since the whole tree went with it. Combos additionally retains its last coherent payload so a disabled resource reporting undefined cannot swap the editor for an empty state. Verified in a browser: type into a combo, switch tabs, come back, the value is still there. Gating suppressed results without cancelling work — fetchCatalog took a signal and passed it to none of its four requests, loadCombos took none at all. Both now thread it, and fetchSelectedModels accepts one. Adds six mounted tests. They were driven red against a reverted lazy-mount to prove they are not vacuous. --- gui/src/model-visibility.ts | 3 +- gui/src/pages/Combos.tsx | 35 ++- gui/src/pages/Models.tsx | 50 +++-- gui/tests/models-workspace-panels.test.tsx | 239 +++++++++++++++++++++ 4 files changed, 297 insertions(+), 30 deletions(-) create mode 100644 gui/tests/models-workspace-panels.test.tsx diff --git a/gui/src/model-visibility.ts b/gui/src/model-visibility.ts index e422edc40..5007d979d 100644 --- a/gui/src/model-visibility.ts +++ b/gui/src/model-visibility.ts @@ -27,8 +27,9 @@ export function parseSelectedModels(value: unknown): ProviderModelMap { export async function fetchSelectedModels( apiBase: string, fetchImpl: typeof fetch = fetch, + signal?: AbortSignal, ): Promise { - const response = await fetchImpl(`${apiBase}/api/selected-models`); + const response = await fetchImpl(`${apiBase}/api/selected-models`, signal ? { signal } : undefined); if (!response.ok) throw new Error(`selected models HTTP ${response.status}`); return parseSelectedModels(await response.json()); } diff --git a/gui/src/pages/Combos.tsx b/gui/src/pages/Combos.tsx index e24b346c7..a825276ac 100644 --- a/gui/src/pages/Combos.tsx +++ b/gui/src/pages/Combos.tsx @@ -70,6 +70,20 @@ export default function Combos({ const t = useT(); const cacheKey = `ocx.combos.workspace.v1:${apiBase}`; const cached = useMemo(() => seedCombos(cacheKey), [cacheKey]); + + /* + * The last coherent payload, kept so a hidden panel can keep rendering. + * + * While `active` is false the resource is disabled and reports `data: undefined` with + * no skeleton and no error. Falling back to empty arrays there swaps the whole + * ComboWorkspace for a first-run empty state and takes every unsaved draft with it — + * proven in a browser: type into a combo, switch tabs, come back, field blank. + * + * State rather than a ref: this repo avoids render-time ref reads under React + * Compiler, and a ref would not re-render when the retained payload changes. Written + * on the load success path, never during render and never from an effect. + */ + const [retainedData, setRetainedData] = useState(cached ?? null); const [status, setStatus] = useState(""); const [statusOk, setStatusOk] = useState(false); const [adding, setAdding] = useState(false); @@ -89,12 +103,13 @@ export default function Combos({ return () => window.clearTimeout(timer); }, [status, statusOk]); - const loadCombos = useCallback(async (): Promise => { + const loadCombos = useCallback(async (signal?: AbortSignal): Promise => { // Keep all three requests parallel: this workspace is only coherent once every input arrives. const [combosRes, configRes, modelsRes] = await Promise.all([ - fetch(`${apiBase}/api/combos`), - fetch(`${apiBase}/api/config`), - fetch(`${apiBase}/api/models`), + // Signals were missing entirely, so resource cleanup could not cancel these. + fetch(`${apiBase}/api/combos`, { signal }), + fetch(`${apiBase}/api/config`, { signal }), + fetch(`${apiBase}/api/models`, { signal }), ]); if (!combosRes.ok || !configRes.ok || !modelsRes.ok) { throw new Error("combo workspace load failed"); @@ -165,6 +180,9 @@ export default function Combos({ const next = { combos, providers, models, cataloguedComboIds: [...catalogued] } satisfies CachedCombosPage; writeSessionListCache(cacheKey, next); + // Retain the coherent payload here — one place, on the success path, never during + // render. See the `retainedData` note below. + setRetainedData(next); return next; }, [apiBase, cacheKey]); @@ -174,14 +192,15 @@ export default function Combos({ loadCombos, /* * Gate the network, never the tree. A hidden panel must not fetch, but the rendered - * workspace has to stay mounted so an unsaved editor draft survives a tab hop — - * that retention path lands in wp03, where the disabled resource stops reporting - * data. Until then the panel keeps its own last render. + * workspace has to stay mounted so an unsaved editor draft survives a tab hop. + * Disabling reports `data: undefined`, so `retainedData` below keeps the last good + * payload and the subtree never unmounts. */ { isEmpty: () => false, initialData: cached ?? undefined, enabled: active }, ); const { state } = resource; - const data = state.data; + + const data = state.data ?? retainedData ?? undefined; const combos = data?.combos ?? []; /* diff --git a/gui/src/pages/Models.tsx b/gui/src/pages/Models.tsx index 6d0df528f..208133946 100644 --- a/gui/src/pages/Models.tsx +++ b/gui/src/pages/Models.tsx @@ -268,10 +268,12 @@ export default function Models({ apiBase }: { apiBase: string }) { const fetchCatalog = useCallback(async (signal: AbortSignal): Promise => { const [modelsRes, capsRes, providersRes, selectionData] = await Promise.all([ - fetch(`${apiBase}/api/models`), - fetch(`${apiBase}/api/provider-context-caps`), - fetch(`${apiBase}/api/providers`), - fetchSelectedModels(apiBase), + // Every request carries the resource signal, so leaving the catalog tab cancels + // the work rather than only discarding its result. + fetch(`${apiBase}/api/models`, { signal }), + fetch(`${apiBase}/api/provider-context-caps`, { signal }), + fetch(`${apiBase}/api/providers`, { signal }), + fetchSelectedModels(apiBase, fetch, signal), ]); const [data, capsData, providerData] = await Promise.all([ readJsonOrThrow(modelsRes), @@ -741,22 +743,19 @@ export default function Models({ apiBase }: { apiBase: string }) { const catalog = catalogState.data ?? cached; - // A session seed keeps the workspace usable during the first shared-resource revalidation. - // Without a catalog, the skeleton owns the only live region for this transition. - if (catalogState.showSkeleton && !catalog) { - return ( - - ); - } - if (catalogState.kind === "failed-cold") { - const reason = catalogState.error instanceof Error ? catalogState.error.message : t("models.loadFail"); - return ( - <> - {reason} - - - ); - } + /* + * Catalog loading and cold failure belong to the CATALOG PANEL, not the page. + * + * These used to be component-level early returns, which is correct for a page that is + * only a catalog and wrong for a page that owns three tabs: a slow or failed catalog + * would unmount the whole workspace, tab strip included, taking every sibling panel + * and any unsaved combo draft with it — and on a cold failure the user could not even + * reach Combos or Routing. Rendered below inside the catalog panel instead. + */ + const catalogColdFailure = catalogState.kind === "failed-cold" + ? (catalogState.error instanceof Error ? catalogState.error.message : t("models.loadFail")) + : null; + const catalogCold = catalogState.showSkeleton && !catalog; const selectedModelMap = selectedModels ?? {}; @@ -1545,7 +1544,16 @@ export default function Models({ apiBase }: { apiBase: string }) { detailsLabel={t("errorBoundary.details")} reloadLabel={t("errorBoundary.reload")} > - {catalogPanel} + {catalogCold + ? + : catalogColdFailure !== null + ? ( + <> + {catalogColdFailure} + + + ) + : catalogPanel}
diff --git a/gui/tests/models-workspace-panels.test.tsx b/gui/tests/models-workspace-panels.test.tsx new file mode 100644 index 000000000..c4a363666 --- /dev/null +++ b/gui/tests/models-workspace-panels.test.tsx @@ -0,0 +1,239 @@ +/** + * Models tab workspace — mounted behaviour. + * + * The routing helpers are unit-tested at `tests/models-workspace-tabs.test.ts`. This file + * exists because those assertions cannot see the failures that actually happened here: + * a component-level early return that unmounted the whole tab tree while the catalog + * loaded, and a disabled resource that swapped the combo editor for an empty state and + * destroyed an unsaved draft. Both passed typecheck, lint, and every source-string + * assertion. Only mounting the thing catches them. + */ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { Window } from "happy-dom"; +import { act } from "react"; +import type { Root } from "react-dom/client"; +import Models from "../src/pages/Models"; +import { LanguageProvider } from "../src/i18n/provider"; +import { clearClientResourceStoresForTests } from "../src/client-resource"; + +const globals = ["document", "window", "navigator", "localStorage", "sessionStorage"] as const; +let previousGlobals: Record<(typeof globals)[number], unknown>; +let testWindow: Window; +const originalFetch = globalThis.fetch; + +const API_BASE = "http://localhost"; + +/** Every catalog/combos/routing endpoint the workspace can reach, with counted hits. */ +function installFetch(): { hits: Map } { + const hits = new Map(); + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = String(input); + const key = url.replace(API_BASE, "").split("?")[0]!; + hits.set(key, (hits.get(key) ?? 0) + 1); + if (url.includes("/api/models")) { + return Response.json([ + { provider: "openai", id: "gpt-5", namespaced: "openai/gpt-5", native: true }, + { provider: "anthropic", id: "claude", namespaced: "anthropic/claude" }, + ]); + } + if (url.includes("/api/provider-context-caps")) return Response.json({ providers: {} }); + if (url.includes("/api/providers")) return Response.json([{ name: "openai", disabled: false }]); + if (url.includes("/api/selected-models")) return Response.json({}); + if (url.includes("/api/combos")) return Response.json([]); + if (url.includes("/api/config")) return Response.json({ providers: { openai: { defaultModel: "gpt-5" } } }); + if (url.includes("/api/routing-profiles")) return Response.json([]); + if (url.includes("/api/routing-analytics")) return Response.json(null); + if (url.includes("/api/shadow-call-settings")) return Response.json({ enabled: false }); + if (url.includes("/api/v2")) return new Response(null, { status: 404 }); + return new Response(null, { status: 404 }); + }) as typeof fetch; + return { hits }; +} + +beforeEach(() => { + clearClientResourceStoresForTests(); + previousGlobals = Object.fromEntries(globals.map(k => [k, Reflect.get(globalThis, k)])) as typeof previousGlobals; + testWindow = new Window({ url: "http://localhost/#models" }); + Object.defineProperties(globalThis, { + document: { configurable: true, value: testWindow.document }, + window: { configurable: true, value: testWindow.window }, + navigator: { configurable: true, value: testWindow.navigator }, + localStorage: { configurable: true, value: testWindow.localStorage }, + sessionStorage: { configurable: true, value: testWindow.sessionStorage }, + }); + (globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + clearClientResourceStoresForTests(); + testWindow.close(); + for (const key of globals) { + Object.defineProperty(globalThis, key, { configurable: true, value: previousGlobals[key] }); + } +}); + +async function mountModels(): Promise<{ container: HTMLElement; root: Root }> { + const { createRoot } = await import("react-dom/client"); + const container = document.createElement("div"); + document.body.append(container); + let root!: Root; + await act(async () => { + root = createRoot(container); + root.render( + + + , + ); + }); + await act(async () => { await Promise.resolve(); }); + return { container, root }; +} + +const tabs = (container: HTMLElement) => [...container.querySelectorAll('[role="tab"]')] as HTMLButtonElement[]; +const panel = (container: HTMLElement, id: string) => container.querySelector(`#models-panel-${id}`); + +test("the strip renders all three tabs with the catalog selected on the bare hash", async () => { + installFetch(); + const { container, root } = await mountModels(); + try { + expect(tabs(container).map(t => t.id)).toEqual([ + "models-tab-catalog", "models-tab-combos", "models-tab-routing", + ]); + const selected = tabs(container).filter(t => t.getAttribute("aria-selected") === "true"); + expect(selected).toHaveLength(1); + expect(selected[0]!.id).toBe("models-tab-catalog"); + // Roving tabindex: exactly one tab is in the tab order. + expect(tabs(container).filter(t => t.tabIndex === 0)).toHaveLength(1); + } finally { + await act(async () => root.unmount()); + } +}); + +/* + * The regression that shipped and had to be fixed: catalog loading and cold failure were + * component-level early returns, so a slow catalog replaced the entire workspace — strip + * included — and a cold failure left Combos and Routing unreachable. + */ +test("a cold catalog never removes the tab strip", async () => { + let releaseCatalog!: () => void; + const gate = new Promise(resolve => { releaseCatalog = resolve; }); + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/api/models")) { await gate; return Response.json([]); } + if (url.includes("/api/provider-context-caps")) return Response.json({ providers: {} }); + if (url.includes("/api/providers")) return Response.json([]); + if (url.includes("/api/selected-models")) return Response.json({}); + return new Response(null, { status: 404 }); + }) as typeof fetch; + + const { container, root } = await mountModels(); + try { + // Still cold here: the catalog fetch is parked on the gate. + expect(tabs(container)).toHaveLength(3); + expect(panel(container, "catalog")).toBeTruthy(); + releaseCatalog(); + await act(async () => { await Promise.resolve(); }); + expect(tabs(container)).toHaveLength(3); + } finally { + await act(async () => root.unmount()); + } +}); + +test("a cold catalog failure still leaves the other tabs reachable", async () => { + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/api/models")) throw new Error("catalog down"); + if (url.includes("/api/provider-context-caps")) return Response.json({ providers: {} }); + if (url.includes("/api/providers")) return Response.json([]); + if (url.includes("/api/selected-models")) return Response.json({}); + return new Response(null, { status: 404 }); + }) as typeof fetch; + + const { container, root } = await mountModels(); + try { + await act(async () => { await Promise.resolve(); }); + expect(tabs(container)).toHaveLength(3); + const combosTab = container.querySelector("#models-tab-combos") as HTMLButtonElement; + expect(combosTab).toBeTruthy(); + expect(combosTab.disabled).toBe(false); + } finally { + await act(async () => root.unmount()); + } +}); + +test("panels mount lazily and then stay mounted, hidden", async () => { + installFetch(); + const { container, root } = await mountModels(); + try { + // Never visited: not in the tree at all. + expect(panel(container, "combos")).toBeNull(); + expect(panel(container, "routing")).toBeNull(); + + await act(async () => { + (container.querySelector("#models-tab-combos") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + expect(panel(container, "combos")).toBeTruthy(); + expect(panel(container, "catalog")?.hasAttribute("hidden")).toBe(true); + + await act(async () => { + (container.querySelector("#models-tab-catalog") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + // Still mounted, just hidden — this is what lets an unsaved draft survive. + expect(panel(container, "combos")).toBeTruthy(); + expect(panel(container, "combos")?.hasAttribute("hidden")).toBe(true); + expect(panel(container, "catalog")?.hasAttribute("hidden")).toBe(false); + } finally { + await act(async () => root.unmount()); + } +}); + +test("every rendered panel is wired to its tab and carries an error boundary", async () => { + installFetch(); + const { container, root } = await mountModels(); + try { + for (const id of ["combos", "routing"]) { + await act(async () => { + (container.querySelector(`#models-tab-${id}`) as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + } + for (const id of ["catalog", "combos", "routing"]) { + const p = panel(container, id)!; + expect(p.getAttribute("role")).toBe("tabpanel"); + expect(p.getAttribute("aria-labelledby")).toBe(`models-tab-${id}`); + const tab = container.querySelector(`#models-tab-${id}`)!; + expect(tab.getAttribute("aria-controls")).toBe(`models-panel-${id}`); + } + } finally { + await act(async () => root.unmount()); + } +}); + +/* + * The whole point of gating: a hidden catalog must stop polling. Counted rather than + * timed, because the assertion is "no NEW requests", not "requests within a window". + */ +test("leaving the catalog stops its requests", async () => { + const { hits } = installFetch(); + const { container, root } = await mountModels(); + try { + await act(async () => { await Promise.resolve(); }); + const before = hits.get("/api/models") ?? 0; + expect(before).toBeGreaterThan(0); + + await act(async () => { + (container.querySelector("#models-tab-routing") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + const afterSwitch = hits.get("/api/models") ?? 0; + + // Let any interval that survived the switch fire. + await act(async () => { await new Promise(r => setTimeout(r, 60)); }); + expect(hits.get("/api/models") ?? 0).toBe(afterSwitch); + } finally { + await act(async () => root.unmount()); + } +}); From af93e29ab4bdf7ac6628603547ba2ed1f0f92ef1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 15:13:15 +0900 Subject: [PATCH 09/19] fix(gui): stop a failed combos reload from discarding retained work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Disabling the only subscriber schedules store eviction, so a reactivation whose fetch fails is classified failed-cold even when the component still holds a coherent payload. Replacing the workspace there unmounted the editor and destroyed the draft that retention exists to protect. The cold-failure branch now requires that no data is retained. Also repairs three tests that were weaker than their names. The polling assertion waited 60ms against a 10-second interval, so it passed whether or not the poll was gated; it now waits a full period and counts a catalog-exclusive endpoint. The boundary assertion only checked ARIA ids and would have passed with every boundary deleted; a panel now actually fails. And nothing typed a draft, so the bug that shipped could not have been caught — that sequence is now a test. Each new assertion was driven red against its own reverted fix. --- gui/src/pages/Combos.tsx | 9 +- gui/tests/models-workspace-panels.test.tsx | 166 +++++++++++++++++++-- 2 files changed, 160 insertions(+), 15 deletions(-) diff --git a/gui/src/pages/Combos.tsx b/gui/src/pages/Combos.tsx index a825276ac..7b4fc86d6 100644 --- a/gui/src/pages/Combos.tsx +++ b/gui/src/pages/Combos.tsx @@ -273,7 +273,14 @@ export default function Combos({ return ; } - if (state.kind === "failed-cold") { + /* + * `!data` matters. Disabling the only subscriber schedules store eviction, so a + * reactivation whose fetch fails is classified `failed-cold` even when this component + * still holds a coherent retained payload — and replacing the workspace there would + * unmount the editor and destroy the very draft retention exists to protect. With + * retained data the workspace stays up and the failure shows in the stale banner below. + */ + if (state.kind === "failed-cold" && !data) { const reason = state.error instanceof Error ? state.error.message : t("cws.loadFailed"); return ( <> diff --git a/gui/tests/models-workspace-panels.test.tsx b/gui/tests/models-workspace-panels.test.tsx index c4a363666..feaf69302 100644 --- a/gui/tests/models-workspace-panels.test.tsx +++ b/gui/tests/models-workspace-panels.test.tsx @@ -140,13 +140,16 @@ test("a cold catalog never removes the tab strip", async () => { } }); -test("a cold catalog failure still leaves the other tabs reachable", async () => { +test("a cold catalog failure still lets the user reach another tab", async () => { globalThis.fetch = (async (input: RequestInfo | URL) => { const url = String(input); if (url.includes("/api/models")) throw new Error("catalog down"); if (url.includes("/api/provider-context-caps")) return Response.json({ providers: {} }); if (url.includes("/api/providers")) return Response.json([]); if (url.includes("/api/selected-models")) return Response.json({}); + if (url.includes("/api/routing-profiles")) return Response.json([]); + if (url.includes("/api/routing-analytics")) return Response.json(null); + if (url.includes("/api/config")) return Response.json({ providers: {} }); return new Response(null, { status: 404 }); }) as typeof fetch; @@ -154,9 +157,15 @@ test("a cold catalog failure still leaves the other tabs reachable", async () => try { await act(async () => { await Promise.resolve(); }); expect(tabs(container)).toHaveLength(3); - const combosTab = container.querySelector("#models-tab-combos") as HTMLButtonElement; - expect(combosTab).toBeTruthy(); - expect(combosTab.disabled).toBe(false); + + // "Reachable" has to mean the click works and the panel actually appears — asserting + // the button merely exists would pass with a dead tab. + await act(async () => { + (container.querySelector("#models-tab-routing") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + expect(panel(container, "routing")).toBeTruthy(); + expect(panel(container, "routing")?.hasAttribute("hidden")).toBe(false); } finally { await act(async () => root.unmount()); } @@ -190,7 +199,7 @@ test("panels mount lazily and then stay mounted, hidden", async () => { } }); -test("every rendered panel is wired to its tab and carries an error boundary", async () => { +test("every rendered panel is wired to its tab", async () => { installFetch(); const { container, root } = await mountModels(); try { @@ -213,26 +222,155 @@ test("every rendered panel is wired to its tab and carries an error boundary", a }); /* - * The whole point of gating: a hidden catalog must stop polling. Counted rather than - * timed, because the assertion is "no NEW requests", not "requests within a window". + * The ARIA test above pins wiring, not isolation — it would pass with every boundary + * deleted. This one makes a panel actually throw. App's boundary is keyed by page, and + * all three tabs are now one page, so without per-panel boundaries one broken tab would + * take the whole workspace down and stay broken across a switch. */ -test("leaving the catalog stops its requests", async () => { +test("a panel that throws does not take its siblings with it", async () => { + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/api/models")) return Response.json([]); + if (url.includes("/api/provider-context-caps")) return Response.json({ providers: {} }); + if (url.includes("/api/providers")) return Response.json([]); + if (url.includes("/api/selected-models")) return Response.json({}); + // A shape the combos loader cannot parse into a coherent page. + if (url.includes("/api/combos")) return Response.json({ combos: { not: "an array" } }); + if (url.includes("/api/config")) return Response.json(null); + if (url.includes("/api/routing-profiles")) return Response.json([]); + if (url.includes("/api/routing-analytics")) return Response.json(null); + return new Response(null, { status: 404 }); + }) as typeof fetch; + + const { container, root } = await mountModels(); + try { + await act(async () => { + (container.querySelector("#models-tab-combos") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + + // Whatever Combos did, the strip and the other tabs must still be usable. + expect(tabs(container)).toHaveLength(3); + await act(async () => { + (container.querySelector("#models-tab-routing") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + expect(panel(container, "routing")?.hasAttribute("hidden")).toBe(false); + } finally { + await act(async () => root.unmount()); + } +}); + +/* + * The whole point of gating: a hidden catalog must stop polling. + * + * The first version of this test waited 60ms against a 10-SECOND poll interval, so it + * passed whether or not the poll was gated — a green assertion proving nothing. Real + * time is what the interval reads, so this waits past one full period and counts a + * catalog-exclusive endpoint rather than `/api/models`, which several panels request. + */ +test("a hidden catalog stops polling across a full interval", async () => { const { hits } = installFetch(); const { container, root } = await mountModels(); try { await act(async () => { await Promise.resolve(); }); - const before = hits.get("/api/models") ?? 0; - expect(before).toBeGreaterThan(0); + expect(hits.get("/api/provider-context-caps") ?? 0).toBeGreaterThan(0); await act(async () => { (container.querySelector("#models-tab-routing") as HTMLButtonElement).click(); }); await act(async () => { await Promise.resolve(); }); - const afterSwitch = hits.get("/api/models") ?? 0; + const afterSwitch = hits.get("/api/provider-context-caps") ?? 0; + + // One full poll period plus slack. Slow, but a shorter wait cannot tell a gated + // poll from an ungated one. + await act(async () => { await new Promise(r => setTimeout(r, 11_000)); }); + expect(hits.get("/api/provider-context-caps") ?? 0).toBe(afterSwitch); + } finally { + await act(async () => root.unmount()); + } +}, 30_000); + +/* + * The failure that actually shipped: an unsaved draft vanished on a tab switch. The + * lazy-mount test above cannot catch it — the panel wrapper stayed mounted the whole + * time while the editor subtree underneath was replaced. + */ +test("an unsaved combo draft survives a tab switch", async () => { + installFetch(); + const { container, root } = await mountModels(); + try { + await act(async () => { + (container.querySelector("#models-tab-combos") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + + const field = () => panel(container, "combos")?.querySelector("input") as HTMLInputElement | null; + const input = field(); + expect(input).toBeTruthy(); + + await act(async () => { + const setter = Object.getOwnPropertyDescriptor( + testWindow.HTMLInputElement.prototype, "value", + )?.set; + setter?.call(input, "draft-probe"); + input!.dispatchEvent(new testWindow.Event("input", { bubbles: true })); + }); + await act(async () => { await Promise.resolve(); }); + expect(field()?.value).toBe("draft-probe"); + + await act(async () => { + (container.querySelector("#models-tab-catalog") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + await act(async () => { + (container.querySelector("#models-tab-combos") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + + expect(field()?.value).toBe("draft-probe"); + } finally { + await act(async () => root.unmount()); + } +}); + +/* + * A reactivation whose fetch fails is classified `failed-cold` once the store has been + * evicted, and replacing the workspace there would destroy the retained draft. + */ +test("a failed combos reload keeps the workspace instead of replacing it", async () => { + let failNext = false; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = String(input); + if (failNext && url.includes("/api/combos")) throw new Error("combos down"); + if (url.includes("/api/models")) return Response.json([]); + if (url.includes("/api/provider-context-caps")) return Response.json({ providers: {} }); + if (url.includes("/api/providers")) return Response.json([]); + if (url.includes("/api/selected-models")) return Response.json({}); + if (url.includes("/api/combos")) return Response.json([]); + if (url.includes("/api/config")) return Response.json({ providers: {} }); + return new Response(null, { status: 404 }); + }) as typeof fetch; + + const { container, root } = await mountModels(); + try { + await act(async () => { + (container.querySelector("#models-tab-combos") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + expect(panel(container, "combos")?.querySelector(".combos-workspace-root")).toBeTruthy(); + + failNext = true; + await act(async () => { + (container.querySelector("#models-tab-catalog") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); + await act(async () => { + (container.querySelector("#models-tab-combos") as HTMLButtonElement).click(); + }); + await act(async () => { await Promise.resolve(); }); - // Let any interval that survived the switch fire. - await act(async () => { await new Promise(r => setTimeout(r, 60)); }); - expect(hits.get("/api/models") ?? 0).toBe(afterSwitch); + expect(panel(container, "combos")?.querySelector(".combos-workspace-root")).toBeTruthy(); } finally { await act(async () => root.unmount()); } From d1ecbed120aac70b9669fce7881b28081786c9c7 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 15:18:52 +0900 Subject: [PATCH 10/19] test(gui): use fake timers for the poll gate and stop overclaiming a name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 11-second real wait cost every GUI run that time; fake timers advance past the interval without spending it, following logs-auto-refresh. Still verified red against an ungated catalog resource, so the speedup did not cost the assertion. The panel-failure test never exercised an ErrorBoundary — malformed responses reject in the loader, which the resource layer turns into failure state, and nothing throws during render. Renamed to what it actually proves: one panel's failed load stays contained. --- gui/tests/models-workspace-panels.test.tsx | 28 +++++++++++++--------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/gui/tests/models-workspace-panels.test.tsx b/gui/tests/models-workspace-panels.test.tsx index feaf69302..b114fd9b6 100644 --- a/gui/tests/models-workspace-panels.test.tsx +++ b/gui/tests/models-workspace-panels.test.tsx @@ -8,7 +8,7 @@ * destroyed an unsaved draft. Both passed typecheck, lint, and every source-string * assertion. Only mounting the thing catches them. */ -import { afterEach, beforeEach, expect, test } from "bun:test"; +import { afterEach, beforeEach, expect, jest, test } from "bun:test"; import { Window } from "happy-dom"; import { act } from "react"; import type { Root } from "react-dom/client"; @@ -227,7 +227,7 @@ test("every rendered panel is wired to its tab", async () => { * all three tabs are now one page, so without per-panel boundaries one broken tab would * take the whole workspace down and stay broken across a switch. */ -test("a panel that throws does not take its siblings with it", async () => { +test("a panel load failure does not take its siblings with it", async () => { globalThis.fetch = (async (input: RequestInfo | URL) => { const url = String(input); if (url.includes("/api/models")) return Response.json([]); @@ -249,7 +249,10 @@ test("a panel that throws does not take its siblings with it", async () => { }); await act(async () => { await Promise.resolve(); }); - // Whatever Combos did, the strip and the other tabs must still be usable. + // Whatever Combos did, the strip and the other tabs must still be usable. This is a + // failed LOAD, not a render throw — the boundary mechanism itself is covered by + // error-boundary.test.tsx; what matters here is that one panel's failure is + // contained. expect(tabs(container)).toHaveLength(3); await act(async () => { (container.querySelector("#models-tab-routing") as HTMLButtonElement).click(); @@ -264,13 +267,14 @@ test("a panel that throws does not take its siblings with it", async () => { /* * The whole point of gating: a hidden catalog must stop polling. * - * The first version of this test waited 60ms against a 10-SECOND poll interval, so it - * passed whether or not the poll was gated — a green assertion proving nothing. Real - * time is what the interval reads, so this waits past one full period and counts a - * catalog-exclusive endpoint rather than `/api/models`, which several panels request. + * The first version waited 60ms against a 10-SECOND poll interval, so it passed whether + * or not the poll was gated — a green assertion proving nothing. Fake timers advance + * past a full period without spending it, following `logs-auto-refresh.test.tsx`. The + * counted endpoint is catalog-exclusive; `/api/models` is requested by several panels. */ test("a hidden catalog stops polling across a full interval", async () => { const { hits } = installFetch(); + jest.useFakeTimers({ now: 1_700_000_000_000 }); const { container, root } = await mountModels(); try { await act(async () => { await Promise.resolve(); }); @@ -282,14 +286,16 @@ test("a hidden catalog stops polling across a full interval", async () => { await act(async () => { await Promise.resolve(); }); const afterSwitch = hits.get("/api/provider-context-caps") ?? 0; - // One full poll period plus slack. Slow, but a shorter wait cannot tell a gated - // poll from an ungated one. - await act(async () => { await new Promise(r => setTimeout(r, 11_000)); }); + // Past one full poll period. A shorter advance cannot tell a gated poll from an + // ungated one, which is exactly how the first version of this test lied. + await act(async () => { jest.advanceTimersByTime(11_000); }); + await act(async () => { await Promise.resolve(); }); expect(hits.get("/api/provider-context-caps") ?? 0).toBe(afterSwitch); } finally { await act(async () => root.unmount()); + jest.useRealTimers(); } -}, 30_000); +}); /* * The failure that actually shipped: an unsaved draft vanished on a tab switch. The From 035cce31d101e0aef0cf77619d12baaddb7ee74c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 15:33:47 +0900 Subject: [PATCH 11/19] feat(gui): retire the standalone Combos and Routing pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both drop out of the Page union, so the compiler finds every remaining reference. #combos and #routing keep working through passive redirects to their nested destinations, including the startsWith arm that stops #routing/anything from being normalised down to a bare page. The sidebar loses its Routing row — a NavEntry is typed Page, so this is forced rather than chosen. The Combos card in the catalog goes too: it existed to point at a page that was otherwise unreachable, and pointing at a sibling tab is just duplicate navigation. Its summary resource went with it, since the tab count comes from the Combos panel itself. Verified in a browser: #combos, #routing, #routing/anything, and #models/nope all land on the right tab with the right hash. --- gui/src/App.tsx | 21 ++-- gui/src/app-routing.ts | 22 ++++- gui/src/pages/Models.tsx | 136 +------------------------- tests/routing-intelligence-ui.test.ts | 31 ++++-- 4 files changed, 45 insertions(+), 165 deletions(-) diff --git a/gui/src/App.tsx b/gui/src/App.tsx index f645866a6..c1c2d9eff 100644 --- a/gui/src/App.tsx +++ b/gui/src/App.tsx @@ -3,18 +3,16 @@ import { useKeyedClientResource } from "./client-resource"; import Dashboard from "./pages/Dashboard"; import Providers from "./pages/Providers"; import Models from "./pages/Models"; -import Combos from "./pages/Combos"; import Subagents from "./pages/Subagents"; import Logs from "./pages/Logs"; import Usage from "./pages/Usage"; -import RoutingProfiles from "./pages/RoutingProfiles"; import Storage from "./pages/Storage"; import CodexAuth from "./pages/CodexAuth"; import Integrations from "./pages/Integrations"; import Startup from "./pages/Startup"; import ErrorBoundary from "./components/ErrorBoundary"; import { SidebarGithubRow } from "./components/sidebar-github-row"; -import { IconGrid, IconServer, IconBoxes, IconBot, IconList, IconActivity, IconHardDrive, IconKey, IconMenu, IconSun, IconMoon, IconMonitor, IconGlobe, IconPower, IconTerminal, IconX, IconRoute } from "./icons"; +import { IconGrid, IconServer, IconBoxes, IconBot, IconList, IconActivity, IconHardDrive, IconKey, IconMenu, IconSun, IconMoon, IconMonitor, IconGlobe, IconPower, IconTerminal, IconX } from "./icons"; import { useI18n, useT, LOCALES, type Locale, type TKey } from "./i18n/shared"; import { Select } from "./ui"; import { installApiAuthFetch } from "./api"; @@ -33,14 +31,12 @@ const PAGE_TKEY: Record = { startup: "nav.startup", providers: "nav.providers", models: "nav.models", - combos: "nav.combos", subagents: "nav.subagents", logs: "nav.logs", usage: "nav.usage", storage: "nav.storage", "codex-auth": "nav.codexAuth", integrations: "nav.integrations", - routing: "nav.routing", }; const API_BASE = import.meta.env.VITE_API_BASE || ""; @@ -69,7 +65,6 @@ const NAV: NavEntry[] = [ { id: "subagents", tkey: "nav.subagents", Icon: IconBot }, { id: "logs", tkey: "nav.logs", Icon: IconList }, { id: "usage", tkey: "nav.usage", Icon: IconActivity }, - { id: "routing", tkey: "nav.routing", Icon: IconRoute }, { id: "storage", tkey: "nav.storage", Icon: IconHardDrive }, /* * Claude sits directly above Integrations because it is a shortcut into that @@ -340,14 +335,12 @@ export default function App() {
{/* - The combos workspace is full-bleed, and it is reachable two ways during the - tab migration: as the standalone `#combos` page and as the Models Combos tab. - `.main-inner` is App's element, so App is the only place that can know. + Combos is full-bleed, unlike every other surface, and it is reachable only as + a Models tab. `.main-inner` is App's element, so App is the only place that + can know which tab is showing. */}
} {page === "startup" && } {page === "providers" && } - {page === "models" && } - {page === "combos" && } + {page === "models" && } {page === "subagents" && } {page === "logs" && } {page === "usage" && } - {page === "routing" && } {page === "storage" && } {page === "codex-auth" && } {page === "integrations" && } diff --git a/gui/src/app-routing.ts b/gui/src/app-routing.ts index 7eda18d3d..47a5362a0 100644 --- a/gui/src/app-routing.ts +++ b/gui/src/app-routing.ts @@ -7,28 +7,24 @@ export type Page = | "startup" | "providers" | "models" - | "combos" | "subagents" | "logs" | "usage" | "storage" | "codex-auth" - | "integrations" - | "routing"; + | "integrations"; export const VALID_PAGES = new Set([ "dashboard", "startup", "providers", "models", - "combos", "subagents", "logs", "usage", "storage", "codex-auth", "integrations", - "routing", ]); export function readPageFromHash(hash?: string): Page { @@ -39,6 +35,8 @@ export function readPageFromHash(hash?: string): Page { const pageId = raw.split("/")[0] as Page; // Legacy: Debug used to be a standalone page; it now lives as a tab on Logs. if (pageId === ("debug" as Page)) return "logs"; + // Legacy: Combos and Routing used to be standalone pages; both are Models tabs now. + if (pageId === ("combos" as Page) || pageId === ("routing" as Page)) return "models"; // Legacy integration pages now live below one Integrations route. Returning // the destination page here keeps the initial hook state aligned until the // resolver replaces the hash with the exact nested destination. @@ -120,6 +118,20 @@ export function resolveAppHashChange(rawHash: string): AppHashChangeAction { return { page: "logs", replaceTo: "logs/debug" }; } + /* + * Legacy: Combos and Routing used to be standalone pages, now Models tabs. + * + * The `startsWith` arm is not decoration. Without it the generic normalization below + * would rewrite `#routing/anything` to the bare page and drop the destination — the + * same bug the `#api` comment below documents. + */ + if (rawHash === "combos" || rawHash.startsWith("combos/")) { + return { page: "models", replaceTo: "models/combos" }; + } + if (rawHash === "routing" || rawHash.startsWith("routing/")) { + return { page: "models", replaceTo: "models/routing" }; + } + /* * Legacy top-level integration pages. * diff --git a/gui/src/pages/Models.tsx b/gui/src/pages/Models.tsx index 208133946..3b0f69309 100644 --- a/gui/src/pages/Models.tsx +++ b/gui/src/pages/Models.tsx @@ -1,11 +1,10 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { Switch, Notice, EmptyState, Select, Tooltip } from "../ui"; -import { IconChevron, IconBoxes, IconInfo, IconShuffle, IconCheck, IconAlert } from "../icons"; +import { IconChevron, IconBoxes, IconInfo, IconCheck, IconAlert } from "../icons"; import { useT } from "../i18n/shared"; import type { TFn, TKey } from "../i18n/shared"; import { modelLabel } from "../model-display"; import { formatNamespacedModelId, formatProviderDisplayName, providerDisplaySlug } from "../provider-icons"; -import { type ComboItem, parseComboList } from "../combo-workspace-data"; import { readJsonIfOk, readJsonOrThrow } from "../fetch-json"; import { readSessionListCache, writeSessionListCache } from "../session-list-cache"; import { setClientResourceData } from "../client-resource"; @@ -45,11 +44,9 @@ import { fmtK, PAGE, readCollapsedProviders, - readCombosOpen, THREAD_OPTION_SET, THREAD_OPTIONS, writeCollapsedProviders, - writeCombosOpen, discoveryFailureLabel, type ModelRow, type ProviderContextCapsResponse, @@ -69,12 +66,6 @@ type CachedModelsPage = { contextCapValue: number; }; -/** Session JSON is untrusted — only seed rows that survive parseComboList (targets always arrays). */ -function readCachedCombos(value: unknown): ComboItem[] | null { - if (!Array.isArray(value)) return null; - return parseComboList({ combos: value }); -} - /** One subtitle per tab: only one panel is visible, so only one description applies. */ const SUBTITLE_TKEY: Record = { catalog: "models.subtitle", @@ -187,42 +178,9 @@ export default function Models({ apiBase }: { apiBase: string }) { const hoverTimerRef = useRef | null>(null); const [shadowCall, setShadowCall] = useState(null); const [shadowCallSaving, setShadowCallSaving] = useState(false); - // Combo summary section. null = cold load with no seed (pending strut). Failed reads stay - // null + combosError so an API error never masquerades as "no combos configured". - const combosCacheKey = `ocx.models.combos.v1:${apiBase}`; - const seededCombos = useMemo(() => { - const own = readCachedCombos(readSessionListCache(combosCacheKey)); - if (own !== null) return own; - // Reuse the Combos workspace session snapshot when Models opens first in the session. - const workspace = readSessionListCache<{ combos?: unknown }>(`ocx.combos.workspace.v1:${apiBase}`); - return readCachedCombos(workspace?.combos); - }, [apiBase, combosCacheKey]); - const combosResource = useDataSurface( - `models-combos:${apiBase}`, - [apiBase], - async (signal) => { - const r = await fetch(`${apiBase}/api/combos`, { signal }); - const j = await readJsonOrThrow(r); - const next = parseComboList(j); - writeSessionListCache(combosCacheKey, next); - return next; - }, - { isEmpty: () => false, initialData: seededCombos ?? undefined, enabled: catalogActive }, - ); - const combosState = combosResource.state; - // Keep a previously painted card on a later failure so the catalog does not yank down. - const combos = combosState.data ?? seededCombos; - // Announce failures even when stale/seeded rows remain (layout kept; freshness not faked). - const combosError = combosState.showError; - const [combosOpen, setCombosOpen] = useState(readCombosOpen); // App owns the in-session view mode; fallback to persisted mode for isolated renders/tests. const [selectedProvider, setSelectedProvider] = useState(null); - const toggleCombosOpen = () => { - const next = !combosOpen; - writeCombosOpen(next); - setCombosOpen(next); - }; useEffect(() => () => { if (hoverTimerRef.current) clearTimeout(hoverTimerRef.current); @@ -1141,97 +1099,6 @@ export default function Models({ apiBase }: { apiBase: string }) { ); - const combosBlock = ( - <> - {/* Silent height strut: reserves the empty-card slot so a late /api/combos - cannot insert a row, without a bordered "Combos · Loading…" placeholder. */} - {combos === null && !combosError && ( -
- - {t("common.loading")} - -
- )} - {combos === null && combosError && ( -
-
-
-
- -
-
- )} - {combos !== null && combos.length === 0 && ( -
-
-
-
- {combosError ? ( - - ) : ( - {t("models.combosSetup")} - )} -
-
- )} - {combos !== null && combos.length > 0 && ( -
-
- - {combosError ? ( - - ) : ( - {t("models.combosSetup")} - )} -
- {combosOpen && ( -
- {combos.map(c => ( -
- {c.model} - {c.strategy} · {c.targets.length} -
- ))} - - + {t("models.combosAdd")} - -
- )} -
- )} - - ); - const collapseControls = (
-
diff --git a/gui/src/styles-combos-workspace.css b/gui/src/styles-combos-workspace.css index f73aa3b8a..18f9cabdd 100644 --- a/gui/src/styles-combos-workspace.css +++ b/gui/src/styles-combos-workspace.css @@ -217,34 +217,30 @@ flex-wrap: wrap; } -.combos-workspace-tabs { - display: flex; - gap: 4px; +/* + Pill group for the detail panel's Config/About switch. Mirrors `.models-segmented`; + `.segmented` has no standalone declaration in this codebase, so every use pairs it + with a concrete class. It replaced an underline row that would have stacked under the + Models page tab strip. +*/ +.combos-workspace-segmented { + display: inline-flex; + border: 1px solid var(--border); + border-radius: var(--radius-pill); + background: var(--surface); + padding: 2px; + gap: 2px; margin-bottom: 16px; - border-bottom: 1px solid var(--border-soft); } -.combos-workspace-tab { - appearance: none; +.combos-workspace-segmented .btn { + border-radius: var(--radius-pill); + min-width: 0; + min-height: 0; + padding: 4px 12px; border: none; - background: none; - font: inherit; - font-size: var(--text-control); - font-weight: 500; - color: var(--muted); - padding: 8px 12px; - cursor: pointer; - border-bottom: 2px solid transparent; - margin-bottom: -1px; -} - -.combos-workspace-tab:hover { - color: var(--text); -} - -.combos-workspace-tab.combos-workspace-tab--active { - color: var(--text); - border-bottom-color: var(--accent); + font-size: var(--text-label); + line-height: inherit; } .combos-workspace-tab-content { diff --git a/gui/tests/combos-detail-segmented.test.ts b/gui/tests/combos-detail-segmented.test.ts new file mode 100644 index 000000000..39be280fb --- /dev/null +++ b/gui/tests/combos-detail-segmented.test.ts @@ -0,0 +1,43 @@ +/** + * The Combos detail panel's Config/About switch. + * + * Combos is a tab of the Models page now, so an underline row here would sit directly + * beneath the page tab strip — two rows of the same visual language stacked, which + * reads as two levels of navigation rather than one page's facets. Primer names this + * directly in its UnderlineNav guidance. + * + * The roles stay tab semantics because they control a real tabpanel; only the styling + * changed. That distinction is what these assertions protect: a future "cleanup" that + * converts them to a radiogroup would misdescribe the widget. + */ +import { expect, test } from "bun:test"; + +const panel = await Bun.file( + new URL("../src/components/combo-workspace-detail-panel.tsx", import.meta.url), +).text(); +const css = await Bun.file( + new URL("../src/styles-combos-workspace.css", import.meta.url), +).text(); + +test("the detail switch renders as a segmented pill, not an underline row", () => { + expect(panel).toContain('className="segmented combos-workspace-segmented"'); + // The old underline classes are gone from both the markup and the stylesheet. + expect(panel).not.toContain("combos-workspace-tab--active"); + expect(css).not.toContain(".combos-workspace-tab {"); + expect(css).not.toContain(".combos-workspace-tabs {"); +}); + +test("it keeps tab semantics because it controls a real tabpanel", () => { + expect(panel).toContain('role="tablist"'); + expect(panel).toContain('role="tab"'); + expect(panel).toContain("aria-selected={tab ==="); + expect(panel).toContain('role="tabpanel"'); + // A filter shape would be wrong here: these switch a panel, they do not filter rows. + expect(panel).not.toContain('role="radiogroup"'); +}); + +test("the pill group has its own concrete styling, since .segmented alone has none", () => { + expect(css).toContain(".combos-workspace-segmented {"); + expect(css).toContain(".combos-workspace-segmented .btn {"); + expect(css).toContain("border-radius: var(--radius-pill)"); +}); From 3e2d467e8bdfaf24f4f5f763c16a0293b238fba6 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 16:11:33 +0900 Subject: [PATCH 14/19] feat(gui): drop the duplicate Claude row and finish the panel lifecycles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sidebar reaches nine rows. The Claude row was never a page — it was a shortcut into a tab of Integrations, and paying for it meant subPath, activeHashes, a navHash mirror, and an isNavEntryActive helper whose only job was stopping two rows from lighting at once. Removing the duplicate removed all four. #integrations/claude and its Desktop route are untouched. RoutingProfiles now owns its AbortController at component level, so all four entry points — mount, Retry, post-save, post-delete — are cancellable. Hiding the tab or leaving Models aborts the request and bumps the generation; suppressing a state write while the network keeps running was only half the job. The combo detail tablist gets what its role already promised: roving tabindex, Arrow/Home/End traversal, id/aria-controls/aria-labelledby wiring, and an accessible name across six locales. Review flagged this as pre-existing rather than introduced, but a tablist without arrow keys is a tablist in name only. sidebar-claude-entry.test.ts asserted the exact row being removed, so it is replaced by sidebar-rows.test.ts, which keeps its two surviving rules and pins the nine-row one-to-one mapping. --- gui/src/App.tsx | 84 ++++--------------- .../combo-workspace-detail-panel.tsx | 72 +++++++++++----- gui/src/i18n/de.ts | 1 + gui/src/i18n/en.ts | 1 + gui/src/i18n/ja.ts | 1 + gui/src/i18n/ko.ts | 1 + gui/src/i18n/ru.ts | 1 + gui/src/i18n/zh.ts | 1 + gui/src/pages/RoutingProfiles.tsx | 51 +++++++++-- gui/tests/sidebar-claude-entry.test.ts | 60 ------------- gui/tests/sidebar-rows.test.ts | 55 ++++++++++++ 11 files changed, 173 insertions(+), 155 deletions(-) delete mode 100644 gui/tests/sidebar-claude-entry.test.ts create mode 100644 gui/tests/sidebar-rows.test.ts diff --git a/gui/src/App.tsx b/gui/src/App.tsx index c1c2d9eff..853116d09 100644 --- a/gui/src/App.tsx +++ b/gui/src/App.tsx @@ -12,12 +12,11 @@ import Integrations from "./pages/Integrations"; import Startup from "./pages/Startup"; import ErrorBoundary from "./components/ErrorBoundary"; import { SidebarGithubRow } from "./components/sidebar-github-row"; -import { IconGrid, IconServer, IconBoxes, IconBot, IconList, IconActivity, IconHardDrive, IconKey, IconMenu, IconSun, IconMoon, IconMonitor, IconGlobe, IconPower, IconTerminal, IconX } from "./icons"; +import { IconGrid, IconServer, IconBoxes, IconBot, IconList, IconActivity, IconHardDrive, IconKey, IconMenu, IconSun, IconMoon, IconMonitor, IconGlobe, IconPower, IconX } from "./icons"; import { useI18n, useT, LOCALES, type Locale, type TKey } from "./i18n/shared"; import { Select } from "./ui"; import { installApiAuthFetch } from "./api"; import { type Page } from "./app-routing"; -import { normalizeHashPath } from "./hash-routing"; import { readModelsTab, type ModelsTab } from "./pages/models-tab"; import { useAppRouteState } from "./use-app-route-state"; import { requestProxyStop } from "./stop-proxy"; @@ -43,18 +42,17 @@ const API_BASE = import.meta.env.VITE_API_BASE || ""; const THEME_KEY = "ocx-theme"; /** - * A sidebar row usually maps one-to-one onto a page. Claude does not: it is a - * shortcut into a tab of the Integrations page, so it needs a destination that - * is not the bare page hash and a current-state rule that is not `page === id`. + * Every sidebar row maps one-to-one onto a page again. + * + * The Claude row was the exception: a second entry pointing at a tab of Integrations, + * which needed `subPath`, `activeHashes`, and an `isNavEntryActive` helper whose only + * job was stopping the sidebar from lighting two rows and claiming the user was in two + * places. Removing the duplicate removed all four. */ type NavEntry = { id: Page; tkey: TKey; Icon: typeof IconGrid; - /** Sub-path handed to navigateToPage; the row targets a tab of `id`. */ - subPath?: string; - /** Hash prefixes that keep this row current, instead of the page match. */ - activeHashes?: readonly string[]; }; const NAV: NavEntry[] = [ @@ -66,41 +64,9 @@ const NAV: NavEntry[] = [ { id: "logs", tkey: "nav.logs", Icon: IconList }, { id: "usage", tkey: "nav.usage", Icon: IconActivity }, { id: "storage", tkey: "nav.storage", Icon: IconHardDrive }, - /* - * Claude sits directly above Integrations because it is a shortcut into that - * page. It carries navigation ONLY — the connection switch that used to live - * on this row now belongs to ClaudeCode, which owns GET/PUT /api/claude-code. - * A nav row owning a mutation is exactly the trap that was removed. - * - * The prefix also covers `integrations/claude/desktop`, so Desktop keeps the - * row current without a second entry. - */ - { - id: "integrations", - tkey: "nav.claude", - Icon: IconTerminal, - subPath: "claude", - activeHashes: ["integrations/claude"], - }, { id: "integrations", tkey: "nav.integrations", Icon: IconGlobe }, ]; -/** - * Two rows resolve to the same page, so `page === id` would light both at once - * and the sidebar would claim the user is in two places. A row with - * `activeHashes` wins its own hash; a plain row keeps the page match only while - * no sibling has claimed the current hash. - */ -function isNavEntryActive(entry: NavEntry, page: Page, rawHash: string): boolean { - if (entry.activeHashes) { - return entry.activeHashes.some(prefix => rawHash === prefix || rawHash.startsWith(`${prefix}/`)); - } - if (entry.id !== page) return false; - return !NAV.some(sibling => sibling.activeHashes?.some( - prefix => rawHash === prefix || rawHash.startsWith(`${prefix}/`), - )); -} - const THEME_ICON = { light: IconSun, dark: IconMoon, system: IconMonitor } as const; const THEME_TKEY: Record = { light: "theme.light", dark: "theme.dark", system: "theme.system" }; @@ -137,24 +103,13 @@ export default function App() { // Narrow screens: the sidebar becomes an off-canvas drawer behind a hamburger toggle. const [navOpen, setNavOpen] = useState(false); - /* - * The sidebar's current row is a HASH question, not just a page question: - * Claude and Integrations are the same page and are told apart by the tab. - * `useAppRouteState` only surfaces the page, so track the raw hash here. - */ - const [navHash, setNavHash] = useState(() => normalizeHashPath( - typeof window === "undefined" ? "" : window.location.hash, - )); const menuBtnRef = useRef(null); const sidebarRef = useRef(null); const navWasOpen = useRef(false); useEffect(() => { // External navigation (hash edit, back/forward) also dismisses the mobile drawer. - const dismissNav = () => { - setNavOpen(false); - setNavHash(normalizeHashPath(window.location.hash)); - }; + const dismissNav = () => setNavOpen(false); window.addEventListener("hashchange", dismissNav); window.addEventListener("popstate", dismissNav); return () => { @@ -269,27 +224,20 @@ export default function App() { one layout, so that filter would have hidden the page permanently. */} {/* - The sidebar is navigation only. The Claude row used to carry the - connection switch, which made a nav entry the owner of a mutation - and left the control stranded once the three integration pages - collapsed into one. ClaudeCode owns GET/PUT /api/claude-code, and - the switch lives on its own surface. + The sidebar is navigation only — no row owns a mutation. That rule was + written when the Claude row carried the Claude Code connection switch; + ClaudeCode owns GET/PUT /api/claude-code now, and the row itself is gone. */} {NAV.map(entry => { - const { id, tkey, Icon, subPath } = entry; - const active = isNavEntryActive(entry, page, navHash); + const { id, tkey, Icon } = entry; + const active = id === page; return ( -
+
- +
+ {DETAIL_TABS.map((candidate, index) => ( + + ))}
-
+
{tab === "config" ? (
diff --git a/gui/src/i18n/de.ts b/gui/src/i18n/de.ts index 296f33ca2..9614cf187 100644 --- a/gui/src/i18n/de.ts +++ b/gui/src/i18n/de.ts @@ -1716,6 +1716,7 @@ export const de: Record = { "cws.allCombos": "Alle Combos", "cws.copyModel": "ID kopieren", "cws.copied": "Kopiert", + "cws.tabsLabel": "Combo-Detailbereiche", "cws.tab.config": "Konfiguration", "cws.tab.about": "Info", "cws.strategy": "Strategie", diff --git a/gui/src/i18n/en.ts b/gui/src/i18n/en.ts index 53594c8c5..474f7aca7 100644 --- a/gui/src/i18n/en.ts +++ b/gui/src/i18n/en.ts @@ -1750,6 +1750,7 @@ export const en = { "cws.allCombos": "All combos", "cws.copyModel": "Copy id", "cws.copied": "Copied", + "cws.tabsLabel": "Combo detail sections", "cws.tab.config": "Config", "cws.tab.about": "About", "cws.strategy": "Strategy", diff --git a/gui/src/i18n/ja.ts b/gui/src/i18n/ja.ts index 9407be00b..28fd3c6df 100644 --- a/gui/src/i18n/ja.ts +++ b/gui/src/i18n/ja.ts @@ -1784,6 +1784,7 @@ export const ja: Record = { "cws.copyModel": "ID をコピー", "cws.copied": "コピーしました", "cws.renamed": "{from} を {to} に変更しました。", + "cws.tabsLabel": "コンボ詳細セクション", "cws.tab.config": "設定", "cws.tab.about": "概要", "cws.strategy": "ストラテジー", diff --git a/gui/src/i18n/ko.ts b/gui/src/i18n/ko.ts index 727429276..595083836 100644 --- a/gui/src/i18n/ko.ts +++ b/gui/src/i18n/ko.ts @@ -1743,6 +1743,7 @@ export const ko: Record = { "cws.allCombos": "모든 콤보", "cws.copyModel": "ID 복사", "cws.copied": "복사됨", + "cws.tabsLabel": "콤보 상세 섹션", "cws.tab.config": "설정", "cws.tab.about": "정보", "cws.strategy": "전략", diff --git a/gui/src/i18n/ru.ts b/gui/src/i18n/ru.ts index fc31d96d0..0cae6e561 100644 --- a/gui/src/i18n/ru.ts +++ b/gui/src/i18n/ru.ts @@ -1826,6 +1826,7 @@ export const ru: Record = { "cws.allCombos": "Все комбо", "cws.copyModel": "Копировать id", "cws.copied": "Скопировано", + "cws.tabsLabel": "Разделы деталей комбо", "cws.tab.config": "Конфигурация", "cws.tab.about": "О комбо", "cws.strategy": "Стратегия", diff --git a/gui/src/i18n/zh.ts b/gui/src/i18n/zh.ts index c8cc79573..227816521 100644 --- a/gui/src/i18n/zh.ts +++ b/gui/src/i18n/zh.ts @@ -1736,6 +1736,7 @@ export const zh: Record = { "cws.allCombos": "全部组合", "cws.copyModel": "复制 ID", "cws.copied": "已复制", + "cws.tabsLabel": "组合详情分区", "cws.tab.config": "配置", "cws.tab.about": "关于", "cws.strategy": "策略", diff --git a/gui/src/pages/RoutingProfiles.tsx b/gui/src/pages/RoutingProfiles.tsx index 40d6ae90b..a2363c425 100644 --- a/gui/src/pages/RoutingProfiles.tsx +++ b/gui/src/pages/RoutingProfiles.tsx @@ -182,6 +182,8 @@ export default function RoutingProfiles({ const [running, setRunning] = useState(false); const selectedRef = useRef(null); const loadGenerationRef = useRef(0); + /** Owned by `load` so every entry point — mount, Retry, save, delete — is cancellable. */ + const loadAbortRef = useRef(null); const dryRunGenerationRef = useRef(0); const notify = useCallback((message: string, ok: boolean) => { @@ -210,14 +212,26 @@ export default function RoutingProfiles({ }, [clearDryRun]); const load = useCallback(async (preferredId?: string) => { + /* + * `load` owns the controller, not the effect that happens to call it. + * + * There are four entry points — the mount effect, Retry, post-save, and + * post-delete — so an effect-local controller would cancel only the first and let + * a Retry or a mutation reload keep running after the tab hides. Generation + * invalidation stops the state write but not the network work. + */ + loadAbortRef.current?.abort(); + const controller = new AbortController(); + loadAbortRef.current = controller; + const { signal } = controller; const generation = ++loadGenerationRef.current; setLoadError(""); try { const [profilesRes, analyticsRes, configRes, modelsRes] = await Promise.all([ - fetch(`${apiBase}/api/routing-profiles`), - fetch(`${apiBase}/api/routing-analytics`), - fetch(`${apiBase}/api/config`), - fetch(`${apiBase}/api/models`), + fetch(`${apiBase}/api/routing-profiles`, { signal }), + fetch(`${apiBase}/api/routing-analytics`, { signal }), + fetch(`${apiBase}/api/config`, { signal }), + fetch(`${apiBase}/api/models`, { signal }), ]); if (!profilesRes.ok) throw new Error(`load-${profilesRes.status}`); const [profilesJson, analyticsJson, configJson, modelsJson] = await Promise.all([ @@ -255,19 +269,42 @@ export default function RoutingProfiles({ } } catch (error) { if (generation !== loadGenerationRef.current) return; + // An aborted supersede or deactivate is not a failure worth showing. + if (signal.aborted) return; setLoadError(error instanceof Error ? error.message : String(error)); + } finally { + // Clear only if this request still owns the ref; a newer load may have replaced it. + if (loadAbortRef.current === controller) loadAbortRef.current = null; } }, [apiBase, clearDryRun]); useEffect(() => { - if (!active) return; + if (!active) { + // Hidden: cancel work in flight and invalidate its generation so a late resolve + // cannot write into a panel nobody is looking at. + loadAbortRef.current?.abort(); + loadGenerationRef.current++; + return; + } const timer = window.setTimeout(() => void load(), 0); - return () => window.clearTimeout(timer); + return () => { + window.clearTimeout(timer); + // Unmounting counts too — leaving Models entirely must not strand a request. + /* + * Reading the refs at cleanup time is the point, not a mistake: whatever load is + * in flight NOW is what has to be cancelled, and the generation counter has to + * move past whatever value that load captured. A snapshot taken when the effect + * ran would cancel a stale controller and leave the live one running. + */ + loadAbortRef.current?.abort(); + // eslint-disable-next-line react-hooks/exhaustive-deps -- see above: the latest generation is what must be invalidated + loadGenerationRef.current++; + }; }, [active, load]); /* * Report the count up to the tab strip from an effect keyed on the list length, not - * during render. Full cancellation ownership for `load` lands with wp04. + * during render. */ useEffect(() => { onCountChange?.(profiles.length); diff --git a/gui/tests/sidebar-claude-entry.test.ts b/gui/tests/sidebar-claude-entry.test.ts deleted file mode 100644 index 446add149..000000000 --- a/gui/tests/sidebar-claude-entry.test.ts +++ /dev/null @@ -1,60 +0,0 @@ -import { expect, test } from "bun:test"; - -/** - * The Claude row. - * - * It was removed from the sidebar together with the connection switch it used - * to carry (a56a4aea6). Removing the switch was right — a nav row owning a - * mutation is a trap — but the entry went with it, and Claude Code is the - * deepest surface in the app. - * - * Two things have to stay true: the row navigates and nothing else, and it does - * not light up at the same time as Integrations, since both resolve to the same - * page and only the hash tells them apart. - */ - -const src = await Bun.file(new URL("../src/App.tsx", import.meta.url)).text(); - -test("the Claude row targets the Claude tab and carries no mutation", () => { - expect(src).toContain('tkey: "nav.claude"'); - expect(src).toContain('subPath: "claude"'); - expect(src).toContain('activeHashes: ["integrations/claude"]'); - - /* - * The sidebar is navigation only. A Switch here is the exact regression the - * collapse removed. Strip comments before asserting: the block explains the - * removed mutation in prose, and matching that prose is not evidence about - * the code — an earlier version of this test failed on its own explanation. - */ - const navBlock = src.slice(src.indexOf("")); - const navCode = navBlock.replace(/\{?\/\*[\s\S]*?\*\/\}?/g, "").replace(/\/\/.*$/gm, ""); - expect(navCode).not.toContain("Switch"); - expect(navCode).not.toContain("/api/claude"); -}); - -test("the orphaned sidebar switch styles are gone", async () => { - const css = await Bun.file(new URL("../src/styles.css", import.meta.url)).text(); - expect(css).not.toContain(".nav-entry-claude .switch"); -}); - -/** - * Mirrors `isNavEntryActive`. Kept as a local re-implementation rather than an - * export because App does not otherwise expose its nav internals; the rule is - * small and the two hash cases below are what actually matter. - */ -function activeRow(rawHash: string): "claude" | "integrations" | null { - const claimed = rawHash === "integrations/claude" || rawHash.startsWith("integrations/claude/"); - if (claimed) return "claude"; - if (rawHash === "integrations" || rawHash.startsWith("integrations/")) return "integrations"; - return null; -} - -test("exactly one row is current for any integrations hash", () => { - expect(activeRow("integrations")).toBe("integrations"); - expect(activeRow("integrations/keys")).toBe("integrations"); - expect(activeRow("integrations/grok")).toBe("integrations"); - // Claude wins its own tab, and the nested Desktop route too — that is what - // the prefix match buys instead of a second nav entry. - expect(activeRow("integrations/claude")).toBe("claude"); - expect(activeRow("integrations/claude/desktop")).toBe("claude"); -}); diff --git a/gui/tests/sidebar-rows.test.ts b/gui/tests/sidebar-rows.test.ts new file mode 100644 index 000000000..0c379654c --- /dev/null +++ b/gui/tests/sidebar-rows.test.ts @@ -0,0 +1,55 @@ +/** + * The sidebar's row contract. + * + * Replaces `sidebar-claude-entry.test.ts`, which asserted the exact Claude shortcut row + * that has now been removed. Two of its rules outlived it and are kept here: the + * sidebar carries navigation and nothing else, and no orphaned switch styles are left + * behind. The third — that exactly one of two rows resolving to the same page lights up + * — cannot be violated any more, because every row maps one-to-one onto a page again. + */ +import { expect, test } from "bun:test"; + +const raw = await Bun.file(new URL("../src/App.tsx", import.meta.url)).text(); + +/* + * Comments explain the removed Claude row by name, and matching that prose is not + * evidence about the code — the predecessor of this file learned that the hard way, and + * so did this one on its first run. + */ +const src = raw.replace(/\/\*[\s\S]*?\*\//g, "").replace(/^\s*\/\/.*$/gm, ""); + +test("every row maps one-to-one onto a page", () => { + // The duplicate-row machinery is gone with the row that needed it. + expect(src).not.toContain("activeHashes"); + expect(src).not.toContain("isNavEntryActive"); + expect(src).not.toContain('tkey: "nav.claude"'); + + // Nine rows: dashboard, codex-auth, providers, models, subagents, logs, usage, + // storage, integrations. Routing folded into Models; Claude was a duplicate. + const navBlock = src.slice(src.indexOf("const NAV: NavEntry[] = ["), src.indexOf("];", src.indexOf("const NAV: NavEntry[] = ["))); + expect(navBlock.match(/\{ id: /g) ?? []).toHaveLength(9); + + // No two rows share a page id, which is what made the correction helper necessary. + const ids = [...navBlock.matchAll(/\{ id: "([^"]+)"/g)].map(m => m[1]); + expect(new Set(ids).size).toBe(ids.length); +}); + +test("the sidebar is navigation only", () => { + // A nav row owning a mutation is the exact regression that removed the Claude + // connection switch. + const navCode = src.slice(src.indexOf("")); + expect(navCode).not.toContain("Switch"); + expect(navCode).not.toContain("/api/claude"); +}); + +test("the orphaned sidebar switch styles are gone", async () => { + const css = await Bun.file(new URL("../src/styles.css", import.meta.url)).text(); + expect(css).not.toContain(".nav-entry-claude .switch"); +}); + +test("Claude Code is still reachable, just not as a duplicate row", async () => { + // Removing the shortcut must not remove the destination. + const routing = await Bun.file(new URL("../src/app-routing.ts", import.meta.url)).text(); + expect(routing).toContain('"integrations/claude"'); + expect(routing).toContain('"integrations/claude/desktop"'); +}); From 6b00d5e25154f5dce58f8c4acb424985e1a9a40b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 16:28:48 +0900 Subject: [PATCH 15/19] fix(gui): stop a late mutation from loading into a hidden Routing panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aborting what is already running does not stop what starts afterwards. A save or delete resolving after the panel is hidden called load(), which opened a fresh controller and four requests the deactivation effect had already run past — with a current generation, so the writes would have landed in a panel nobody was looking at. load() now returns early when the panel is inactive. Both tab panels stay in the tree, hidden, on the Models strip and in the combo detail. A conditional wrapper meant the unvisited tab's aria-controls pointed at an element that did not exist. Shells are always present; only their contents mount lazily. Removes the dead standalone branch from RoutingProfiles — the only production caller always passed false — and the nav.claude key its row took with it. New routing-panel-lifecycle tests, each driven red against the reverted guard. The first attempt passed with the fix removed, which meant it was not reproducing the path at all; it now drives the real one. --- .../combo-workspace-detail-panel.tsx | 35 +++- gui/src/i18n/de.ts | 1 - gui/src/i18n/en.ts | 1 - gui/src/i18n/ja.ts | 1 - gui/src/i18n/ko.ts | 1 - gui/src/i18n/ru.ts | 1 - gui/src/i18n/zh.ts | 1 - gui/src/pages/Models.tsx | 47 ++--- gui/src/pages/RoutingProfiles.tsx | 32 ++-- gui/tests/models-workspace-panels.test.tsx | 34 +++- gui/tests/routing-panel-lifecycle.test.tsx | 168 ++++++++++++++++++ gui/tests/routing-profiles.test.tsx | 8 +- gui/tests/sidebar-rows.test.ts | 11 +- 13 files changed, 278 insertions(+), 63 deletions(-) create mode 100644 gui/tests/routing-panel-lifecycle.test.tsx diff --git a/gui/src/components/combo-workspace-detail-panel.tsx b/gui/src/components/combo-workspace-detail-panel.tsx index 933043b45..4e2cf180a 100644 --- a/gui/src/components/combo-workspace-detail-panel.tsx +++ b/gui/src/components/combo-workspace-detail-panel.tsx @@ -220,13 +220,19 @@ export function DetailPanel({ ))}
+ {/* + Both panels stay in the tree, the inactive one `hidden`. A single panel whose id + followed the active tab left the OTHER tab's `aria-controls` pointing at an + element that did not exist — a broken IDREF on whichever tab was not selected. + */} + + {/* + `tabIndex={0}` because this panel holds no focusable descendants: without it, + Tab out of the tablist would skip the content the tab just revealed. + */} +
); } diff --git a/gui/src/i18n/de.ts b/gui/src/i18n/de.ts index 9614cf187..bc23cf5a2 100644 --- a/gui/src/i18n/de.ts +++ b/gui/src/i18n/de.ts @@ -1178,7 +1178,6 @@ export const de: Record = { "api.attribution.ambiguous": "Zwei Schlüssel teilen sich diese ID, daher lässt sich die Nutzung keinem davon zuordnen. Vergib in der Konfigurationsdatei je Schlüssel eine eindeutige ID.", "api.attribution.railAmbiguous": "doppelte ID", // Claude Code inbound - "nav.claude": "Claude", "claude.subtitle": "GPT, Gemini und andere Modelle in Claude Code verwenden.", "claude.enabledLabel": "Claude-Verbindung", "claude.enabledHint": "Wenn aus, kann Claude Code diesen Proxy nicht verwenden.", diff --git a/gui/src/i18n/en.ts b/gui/src/i18n/en.ts index 474f7aca7..c63a33495 100644 --- a/gui/src/i18n/en.ts +++ b/gui/src/i18n/en.ts @@ -1641,7 +1641,6 @@ export const en = { "api.attribution.ambiguous": "Two keys share this ID, so usage cannot be attributed to one of them. Give each key a unique ID in the config file.", "api.attribution.railAmbiguous": "duplicate ID", // Claude Code inbound - "nav.claude": "Claude", "claude.subtitle": "Use GPT, Gemini, and other models inside Claude Code.", "claude.pageTitle": "Claude Code", "claude.workspace.settings": "Settings", diff --git a/gui/src/i18n/ja.ts b/gui/src/i18n/ja.ts index 28fd3c6df..8bef10acc 100644 --- a/gui/src/i18n/ja.ts +++ b/gui/src/i18n/ja.ts @@ -1586,7 +1586,6 @@ export const ja: Record = { "api.attribution.ambiguous": "2 つのキーが同じ ID を共有しているため、どちらの使用状況か判別できません。設定ファイルでキーごとに一意の ID を指定してください。", "api.attribution.railAmbiguous": "ID 重複", // Claude Code inbound - "nav.claude": "Claude", "claude.subtitle": "Claude Code 内で GPT、Gemini などのモデルを使用します。", "claude.pageTitle": "Claude Code", "claude.workspace.settings": "設定", diff --git a/gui/src/i18n/ko.ts b/gui/src/i18n/ko.ts index 595083836..4112cbd79 100644 --- a/gui/src/i18n/ko.ts +++ b/gui/src/i18n/ko.ts @@ -1205,7 +1205,6 @@ export const ko: Record = { "api.attribution.ambiguous": "두 키가 같은 ID를 쓰고 있어 어느 쪽 사용량인지 가릴 수 없습니다. 설정 파일에서 키마다 다른 ID를 주세요.", "api.attribution.railAmbiguous": "ID 중복", // Claude Code inbound - "nav.claude": "Claude", "claude.subtitle": "Claude Code에서 GPT, Gemini 등 다른 모델도 쓸 수 있게 해줍니다.", "claude.enabledLabel": "Claude 연결", "claude.enabledHint": "끄면 Claude Code가 이 프록시를 사용할 수 없습니다.", diff --git a/gui/src/i18n/ru.ts b/gui/src/i18n/ru.ts index 0cae6e561..d38a236bc 100644 --- a/gui/src/i18n/ru.ts +++ b/gui/src/i18n/ru.ts @@ -1628,7 +1628,6 @@ export const ru: Record = { "api.attribution.ambiguous": "Два ключа используют один и тот же ID, поэтому нельзя определить, чьё это использование. Задайте каждому ключу уникальный ID в файле конфигурации.", "api.attribution.railAmbiguous": "дубль ID", // Claude Code inbound - "nav.claude": "Claude", "claude.subtitle": "Используйте GPT, Gemini и другие модели внутри Claude Code.", "claude.pageTitle": "Claude Code", "claude.workspace.settings": "Настройки", diff --git a/gui/src/i18n/zh.ts b/gui/src/i18n/zh.ts index 227816521..ba9ca825e 100644 --- a/gui/src/i18n/zh.ts +++ b/gui/src/i18n/zh.ts @@ -1198,7 +1198,6 @@ export const zh: Record = { "api.attribution.ambiguous": "两个密钥共用同一个 ID，无法判断用量属于哪一个。请在配置文件中为每个密钥设置唯一 ID。", "api.attribution.railAmbiguous": "ID 重复", // Claude Code inbound - "nav.claude": "Claude", "claude.subtitle": "在 Claude Code 中使用 GPT、Gemini 等其他模型。", "claude.enabledLabel": "Claude 连接", "claude.enabledHint": "关闭后 Claude Code 无法使用此代理。", diff --git a/gui/src/pages/Models.tsx b/gui/src/pages/Models.tsx index 3b0f69309..3c94b66a8 100644 --- a/gui/src/pages/Models.tsx +++ b/gui/src/pages/Models.tsx @@ -1423,14 +1423,19 @@ export default function Models({ apiBase }: { apiBase: string }) {
- {mounted.has("combos") && ( - - {mounted.has("routing") && ( - ); diff --git a/gui/src/pages/RoutingProfiles.tsx b/gui/src/pages/RoutingProfiles.tsx index a2363c425..6f5200dd1 100644 --- a/gui/src/pages/RoutingProfiles.tsx +++ b/gui/src/pages/RoutingProfiles.tsx @@ -144,20 +144,14 @@ function selectedAfterLoad( export default function RoutingProfiles({ apiBase, active = true, - standalone = true, onCountChange, }: { apiBase: string; /** * False while this panel is mounted but hidden behind another Models tab. Defaults - * true so the standalone page keeps its existing behaviour. + * true so a direct render (tests) behaves like a visible panel. */ active?: boolean; - /** - * True on the standalone `#routing` page, which owns its own title and subtitle. - * False inside the Models tab, where the shell already renders both. - */ - standalone?: boolean; /** Reports the profile count up to the tab strip. */ onCountChange?: (count: number) => void; }) { @@ -184,6 +178,14 @@ export default function RoutingProfiles({ const loadGenerationRef = useRef(0); /** Owned by `load` so every entry point — mount, Retry, save, delete — is cancellable. */ const loadAbortRef = useRef(null); + /* + * Cancelling in-flight work is not enough on its own. A save or delete can resolve + * AFTER the panel is hidden or unmounted and then call `load()`, which would open a + * fresh controller and four requests that the deactivation effect has already run + * past — and whose generation is current, so its writes would land in a panel nobody + * is looking at. `load` checks this before it starts anything. + */ + const loadEnabledRef = useRef(true); const dryRunGenerationRef = useRef(0); const notify = useCallback((message: string, ok: boolean) => { @@ -212,6 +214,7 @@ export default function RoutingProfiles({ }, [clearDryRun]); const load = useCallback(async (preferredId?: string) => { + if (!loadEnabledRef.current) return; /* * `load` owns the controller, not the effect that happens to call it. * @@ -280,12 +283,14 @@ export default function RoutingProfiles({ useEffect(() => { if (!active) { - // Hidden: cancel work in flight and invalidate its generation so a late resolve - // cannot write into a panel nobody is looking at. + // Hidden: stop new loads, cancel work in flight, and invalidate its generation so + // a late resolve cannot write into a panel nobody is looking at. + loadEnabledRef.current = false; loadAbortRef.current?.abort(); loadGenerationRef.current++; return; } + loadEnabledRef.current = true; const timer = window.setTimeout(() => void load(), 0); return () => { window.clearTimeout(timer); @@ -296,6 +301,7 @@ export default function RoutingProfiles({ * move past whatever value that load captured. A snapshot taken when the effect * ran would cancel a stale controller and leave the live one running. */ + loadEnabledRef.current = false; loadAbortRef.current?.abort(); // eslint-disable-next-line react-hooks/exhaustive-deps -- see above: the latest generation is what must be invalidated loadGenerationRef.current++; @@ -489,14 +495,6 @@ export default function RoutingProfiles({ every static gate. The actions stay; a heading cannot carry buttons, so they sit in a plain toolbar row. */} - {standalone && ( - <> -
-

{t("routing.title")}

-
-

{t("routing.subtitle")}

- - )}