-
Notifications
You must be signed in to change notification settings - Fork 638
feat(gui): fold Combos and Routing into a Models tab workspace #1200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8b4fa60
dcab0ae
a7aa3d1
8a922a1
b5b6f5d
086c222
7dcb8c2
39ad11a
af93e29
d1ecbed
035cce3
9659048
eeca089
3e2d467
6b00d5e
7ccf190
306c117
4e861a6
0ef8120
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| # 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/<id>` | | ||
| | Routing | who answers, chosen by score | `policy/<id>` | | ||
|
|
||
| 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. The poll is in **Models itself** — `pollMs: 10_000` | ||
| on the catalog resource plus a second 10-second V2 interval. Routing and Combos do not | ||
| poll; they fetch once on mount. Gating covers all three, and cancellation matters as | ||
| much as suppression: a load already in flight must be aborted, not merely ignored. | ||
| - Combos holds unsaved editor drafts. Panels mount lazily and then stay mounted so a | ||
| half-typed combo survives a tab hop. Gate the network, never the tree. | ||
| - 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` | Additive hash contract + `models-tab.ts`, tests | — | | ||
| | wp02a | `020_models_shell.md` | Nested workspace **alongside** the legacy pages: tab i18n, strip, panels, per-panel boundaries, active-aware CSS, catalog gating | wp01 | | ||
| | wp02b | `020_models_shell.md` | Route cutover: union removal, redirects, three links, Routing NAV row + `IconRoute` | wp02a | | ||
| | wp03 | `030_combos_embed.md` | Combos panel: `retainedData` state path, abort signal, inner tabs demoted, count callback | wp02b | | ||
| | wp04 | `040_routing_embed_and_sidebar.md` | Routing panel: shared abort controller, heading removal + its test, Claude row, subtitles, render grounding | wp02b | | ||
|
|
||
| wp03 and wp04 both depend on wp02b but not on each other; they run in order because they | ||
| touch the same panel block. | ||
|
|
||
| **Why wp02 is two halves.** The first draft spread this work across three phases and | ||
| produced commits that could not compile (audit round 1). The correction over-swung: one | ||
| atomic phase that was atomic in the sense of *unreviewable* (audit round 2). The split | ||
| the second audit proposed is better than either: wp02a builds the nested workspace while | ||
| `#combos` and `#routing` keep working, so both routes render and every existing Routing | ||
| test stays valid; wp02b then deletes the old form only once the new one is proven in the | ||
| same tree. | ||
|
|
||
| ## 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. | ||
|
Comment on lines
+114
to
+116
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Remove the stale The PR objectives include documentation updates across multiple locales, but Lines 114-116 explicitly place 🤖 Prompt for AI Agents |
||
|
|
||
| ## Verification | ||
|
|
||
| Every phase ends green on **five** commands: | ||
|
|
||
| ```bash | ||
| bun run typecheck | ||
| bun run test # root tests/ ONLY | ||
| cd gui && bun test tests # the 116-file GUI suite — a SEPARATE run | ||
| bun run lint:gui | ||
| bun run build:gui | ||
| ``` | ||
|
Comment on lines
+120
to
+128
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Use one verification command set in every phase document. The canonical plan requires five commands, but two phase documents list only four and omit the separate GUI suite. This allows a phase to pass without executing the GUI regression tests.
📍 Affects 3 files
🤖 Prompt for AI Agents |
||
|
|
||
| `bun run test` does **not** reach `gui/tests/`: `scripts/test.ts:122` defaults to | ||
| `["./tests/"]`. The first draft missed that directory entirely and concluded no test | ||
| covered the affected routes; the second draft knew it existed and still asserted the root | ||
| command ran it. Both were wrong, and the second kind of wrong is worse — an assumption | ||
| stated as fact inside the document that defines what "green" means. | ||
|
|
||
| `gui/tests/` holds the mounted happy-dom tests for page loading, the sidebar, and the | ||
| Routing page. Those are the oracle. `expect(src).toContain(...)` checks are supplements | ||
| that pass while the UI is broken. | ||
|
|
||
| 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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| # Audit round 1 — VERDICT: FAIL | ||
|
|
||
| An independent reviewer audited the roadmap against the actual tree and returned FAIL | ||
| with eight blockers. Every one was re-verified here before acceptance. All eight are | ||
| accepted; none is rebutted. The roadmap is amended in place and re-audited. | ||
|
|
||
| ## The root mistake | ||
|
|
||
| **There are two test directories.** `tests/` at the repository root, and `gui/tests/` | ||
| with 116 files. The roadmap looked only at the first and concluded "no existing test | ||
| covers the combos route." That is false: | ||
|
|
||
| - `gui/tests/page-loading-contract.test.tsx:136` boots a happy-dom window at | ||
| `#combos` and asserts against `.combos-workspace-shell-body`. | ||
| - `gui/tests/sidebar-claude-entry.test.ts:18` requires the exact Claude row and its | ||
| `activeHashes` — the row phase 4 deletes. | ||
| - `gui/tests/routing-profiles.test.tsx:175` requires the literal string | ||
| "Routing Intelligence (beta)" and `[data-page="routing"]`. | ||
|
|
||
| These are mounted behavioural tests, which is precisely the kind the roadmap proposed | ||
| to *invent* while the repository already had them. Worse, the plan's own test proposals | ||
| were mostly `expect(src).toContain(...)` string matches — assertions that pass while the | ||
| UI is broken. The reviewer's judgement stands: static source checks may supplement, but | ||
| they cannot be the oracle. | ||
|
|
||
| ## Blockers, verified | ||
|
|
||
| **B1 — phase 2 cannot typecheck.** `NavEntry.id` is typed `Page` | ||
| (`App.tsx:53`) and NAV holds `{ id: "routing" }` (`App.tsx:71`). Removing `"routing"` | ||
| from the union in phase 2 while deferring NAV cleanup to phase 4 is a type error. | ||
| Same class of problem for i18n: `TKey` derives from `en`, so the tab keys must exist in | ||
| the phase that renders the strip. | ||
| → Routing NAV row, `IconRoute`, its tests, and all tab-shell i18n keys move into | ||
| phase 2. Only the Claude row stays in phase 4. | ||
|
|
||
| **B2 — legacy hashes lose their destination on cold load.** `replaceHash` deliberately | ||
| emits no `hashchange` (`hash-routing.ts:8`) and the redirect runs in an effect | ||
| (`use-app-route-state.ts:87`). So a cold load at `#combos` rewrites the URL to | ||
| `#models/combos` while the tab state — initialized from the *original* hash — is already | ||
| `catalog`. The URL says Combos, the screen shows the catalog. The three | ||
| `href="#combos"` links (`Models.tsx:1104,1132,1143`) hit this on every click, and the | ||
| roadmap never scheduled changing them. | ||
| → `readModelsTab` must recognize `combos`, `combos/*`, `routing`, `routing/*` as well | ||
| as the nested forms, so the pre-redirect hash resolves to the right tab. All three links | ||
| point at `#models/combos`. Covered by a mounted cold-load test, not a resolver assertion. | ||
|
|
||
| **B3 — the `active` gating destroys the drafts it was meant to protect.** A disabled | ||
| `useDataSurface` yields `data: undefined` (`data-surface.ts:59`), and the roadmap's | ||
| answer was to render the skeleton. But the skeleton *replaces* `ComboWorkspace` | ||
| (`Combos.tsx:223`), unmounting the editor and its draft. Keeping the page mounted while | ||
| swapping its subtree preserves nothing. | ||
| → The disabled path must retain the last rendered data and keep the workspace subtree | ||
| alive. Gate the *network*, never the tree. Proven by a type → switch → switch back test. | ||
|
|
||
| **B4 — the hidden-work analysis gated the wrong component.** Routing and Combos do not | ||
| poll; that correction was right. But **Models does**: `pollMs: 10_000` on the catalog | ||
| resource (`Models.tsx:271`) and a second 10-second `setInterval` for V2 | ||
| (`Models.tsx:302`). So the catalog keeps hitting `/api/models` and `/api/v2` while the | ||
| user reads Combos — the exact leak the plan claimed to prevent, in the one panel it never | ||
| examined. Also, `if (!active) return` does not cancel a load already in flight: | ||
| `RoutingProfiles` fetches take no signal and hiding never bumps `loadGenerationRef`. | ||
| → Gate the catalog resource, the combo-summary resource, and the shadow/V2 effect and | ||
| interval on the catalog tab. Give Routing real cancellation, not just scheduling | ||
| suppression. | ||
|
|
||
| **B5 — the CSS fix is right but lands a phase late and is incomplete.** The direct-child | ||
| break at `styles.css:399` is real and the fill-panel chain is sound. But phase 2 inserts | ||
| the wrapper and phase 3 repairs it, so phase 2 knowingly ships a broken layout while | ||
| claiming all three tabs paint. Two omissions: the per-tab `.page-sub` also needs the | ||
| restored padding and `flex-shrink: 0`, and `.main-inner:has(.models-workspace-shell)` | ||
| (`styles-models-workspace.css:8`) still matches a *hidden* catalog panel — so Routing | ||
| renders at 980px on a direct visit and 1200px after the catalog has mounted once. A | ||
| history-dependent width is a bug, not a cosmetic detail. | ||
| → All wrapper CSS moves to phase 2. The 1200px selector becomes active-panel-aware. | ||
|
|
||
| **B6 — `ErrorBoundary key={page}` stops resetting.** The boundary is keyed on `page` | ||
| (`App.tsx:328`) and all three tabs are now one page, so an error in Combos persists | ||
| after switching to Routing. Keying on the tab instead is worse: it remounts the whole | ||
| workspace on every switch and destroys drafts — the same trap as B3. | ||
| → Per-panel boundaries, or a reset that clears an existing error without remounting. | ||
| Regression test: error, switch, expect a clean panel. | ||
|
|
||
| **B7 — the tab counts cannot work as specified.** Models' combo summary uses a different | ||
| resource key than the Combos workspace (`Models.tsx:143` vs `Combos.tsx:157`), and combo | ||
| mutations refresh only their own (`Combos.tsx:186`) — so the count goes stale right after | ||
| a create or delete. Routing has no channel at all to report `profiles.length`, which | ||
| makes the promised discoverability mitigation undeliverable as written. | ||
| → Child-to-shell count callbacks or one shared resource owner, tested after a mutation. | ||
| A count that lies is worse than no count. | ||
|
|
||
| **B8 — test adequacy.** Covered above. | ||
|
|
||
| ## Non-blocking, accepted | ||
|
|
||
| - The Routing header instruction was incoherent ("an `h3` carrying only the action | ||
| buttons" — a heading cannot carry buttons). Decision: `routing.title` is dropped from | ||
| the panel entirely and its actions move into a toolbar row; the Models page header is | ||
| the only title. `gui/tests/routing-profiles.test.tsx:175` asserts that string, so the | ||
| test moves with the decision rather than the decision bending to the test. | ||
| - The ≤939px stacked layout keeps a 220px rail minimum; adding a header and strip leaves | ||
| very little detail height on short landscape viewports. Added to browser coverage. | ||
| - `nav.combos`, `nav.routing`, and `nav.claude` all keep non-sidebar consumers. Do not | ||
| delete them. | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| ## Revised phase map | ||
|
|
||
| | Phase | Scope change | | ||
| |-------|--------------| | ||
| | wp01 | unchanged — additive, green | | ||
| | wp02 | **+** Routing NAV row + `IconRoute`, **+** all tab-shell i18n keys, **+** the complete wrapper CSS, **+** catalog poll gating, **+** per-panel error boundaries | | ||
| | wp03 | **−** CSS (moved up); **+** retained-data path for drafts; **+** count callback | | ||
| | wp04 | **−** Routing NAV (moved up); keeps Claude row, remaining i18n, render grounding | | ||
|
|
||
| wp02 becomes the largest phase. That is correct: "remove a page, add the tab that | ||
| replaces it, keep the tree compiling and the layout intact" is one atomic change, and | ||
| splitting it was what produced four of these eight blockers. | ||
Uh oh!
There was an error while loading. Please reload this page.