feat(gui): fold Combos and Routing into a Models tab workspace - #1200
Merged
Conversation
github-actions
Bot
added
enhancement
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
📝 WalkthroughWalkthroughThe PR consolidates Models, Combos, and Routing into a nested three-tab workspace. It adds hash routing, persistent panels, cancellable loading, accessible tab controls, updated sidebar and localization content, responsive styling, documentation changes, and GUI test coverage. ChangesModels workspace migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Additive only: #models/combos and #models/routing are recognised by the router and mapped to tabs, while #combos and #routing stay first-class pages. Nothing renders differently yet. readModelsTab deliberately also resolves the legacy top-level hashes. The redirect that will rewrite them uses replaceState, which emits no hashchange, so tab state is read from the original hash — recognising only the nested form would land a cold load at #combos on the catalog while the URL claimed Combos.
Delimiter-aware matching, deeper nested hashes, and the exact DOM ids were all unpinned: a later simplification to a bare startsWith, or a silent id rename, would have passed every assertion.
Models grows a three-tab strip (Models / Combos / Routing) with the panels mounted lazily and then kept mounted, hidden, so an unsaved combo draft survives a tab hop. #combos and #routing stay first-class pages in this phase, so both routes render and every existing test stays valid. Catalog work now stops when the catalog is hidden: the 10-second catalog poll, the combo summary, and the shadow/V2 pair are all gated on the tab. Each panel owns an error boundary, because App's is keyed by page and all three tabs are one page now. Three things the browser found that no static gate could: the catalog's five-line subtitle pushed the full-height Combos workspace off screen, so subtitles are now per tab; Routing rendered its title and description a second time under the shell's; and the header count duplicated the tab label.
…witches 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.
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.
…name 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.
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.
The guides told users to choose Combos or Routing in the sidebar, which no longer exists there — the most user-visible residue of the cutover. Fixed in English plus all four localized combos guides. Also clears what the deleted card left behind: its localStorage preference helpers, five now-unused i18n keys across six locales, a CSS arm and comment describing a standalone page that is gone, and two test headers still describing the pre-cutover world.
Combos is a Models tab now, so its Config/About underline row sat directly beneath the page tab strip — two rows of the same visual language stacked, which reads as two navigation levels rather than one page's facets. Primer names this pattern directly. The roles stay tablist/tab/aria-selected: they control a real tabpanel, so this is a tab set wearing pill styling, not a filter. Using the radiogroup shape that .models-segmented uses would misdescribe it. Verified in a browser: only the page strip carries an underline now, and the pills match the Failover/Round-robin group right below them.
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.
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.
Author CSS beats the UA's [hidden] { display: none }, so the plain
display:flex on the combo detail panel and on the full-bleed Models panel
left a hidden panel on screen — Config and About stacked together, one of
them marked hidden and rendering regardless. Both rules are now scoped
with :not([hidden]).
This was self-inflicted: mounting both panels to fix the broken
aria-controls IDREFs is what exposed it, and no source-string assertion
could see it. New mounted tests cover the IDREFs, exactly-one-exposed,
roving tabindex, the focusable About panel, and the CSS scoping itself —
the last one driven red by unscoping the rule.
Adds the count-after-reload coverage the plan asked for, drops the
now-unreachable direct-child arm of the 1200px selector, and corrects the
phase doc, which still claimed nav.claude and nav.routing had consumers
after both were removed.
Width contract verified at 1600px: catalog 1200, routing 980 whether or
not the catalog was visited first.
Six work-phases, three adversarial audit rounds on the roadmap and up to three review rounds per phase. The record keeps what the reviews caught that five green gates could not, including the two habits worth carrying forward: verify the verification, and a rule is not a mechanism.
Three 1280x800 captures — Models, Combos, Routing — for the PR description and the enforce-target screenshot gate. Taken against a neutral fixture rather than the live proxy. The first set was shot against real data and showed this machine's account namespaces, model-by-model request volumes, and 5000 requests of usage history. privacy:scan passed on those, because it looks for credential patterns and cannot judge whether usage telemetry belongs in a public repository. A committed PNG is not practical to retract.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6199489cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+315
to
+317
| useEffect(() => { | ||
| onCountChange?.(profiles.length); | ||
| }, [onCountChange, profiles.length]); |
There was a problem hiding this comment.
On a cold visit to the Routing tab, this effect immediately reports 0 from the initial empty profiles state before load() finishes. The tab therefore temporarily claims there are no profiles, and if the management request fails it displays 0 indefinitely even though the actual count is unknown. Track whether a successful profile response has landed and call onCountChange only after that point, as the Combos panel already does.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| { isEmpty: () => false, pollMs: 10_000, initialData: cached ?? undefined }, | ||
| // Gated on the catalog tab: a 10-second poll that keeps running while the user | ||
| // reads Combos or Routing is exactly the hidden work this workspace avoids. | ||
| { isEmpty: () => false, pollMs: 10_000, initialData: cached ?? undefined, enabled: catalogActive }, |
There was a problem hiding this comment.
After the catalog first loads without a preexisting session seed, switching to Combos or Routing unsubscribes this resource and its store is evicted; the memoized cached value remains the original null. Returning to Models therefore treats the catalog as cold and replaces the retained rows with a skeleton, and if revalidation fails it shows only the cold error even though applyCatalog still holds the last successful data and fetchCatalog wrote it to session storage. Preserve the last coherent catalog payload across the disabled interval, as the Combos panel does, so tab changes do not discard usable stale content.
Useful? React with 👍 / 👎.
| * Disabling reports `data: undefined`, so `retainedData` below keeps the last good | ||
| * payload and the subtree never unmounts. | ||
| */ | ||
| { isEmpty: () => false, initialData: cached ?? undefined, enabled: active }, |
There was a problem hiding this comment.
When a save, create, or delete starts on the Combos tab and the user switches tabs before the mutation response arrives, the async handler retains the resource.refresh closure from the render where active was true. Its post-mutation refresh therefore still starts the three workspace GETs while the panel is hidden, bypassing this enabled: active gate; the subsequent onSaved/onCreated callbacks can trigger another refresh as well. Guard post-mutation refreshes with the current active state, using a ref or equivalent lifecycle guard like RoutingProfiles, rather than the stale render closure.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_fin/260807_models_workspace_tabs/000_plan.md`:
- Around line 120-128: Use the five-command verification set consistently: keep
devlog/_fin/260807_models_workspace_tabs/000_plan.md lines 120-128 as the
canonical gate, and add `cd gui && bun test tests` to the verification lists in
devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md lines
113-117 and devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md
lines 248-252.
- Around line 114-116: Update the scope statement in the roadmap to remove the
stale docs-site exclusion, keeping documentation updates and locale
synchronization checks within the declared PR objectives; do not move them to a
separate change unless the roadmap explicitly declares that separate
documentation work.
- Around line 42-45: Align the cancellation contract across the plan and
outcome: in devlog/_fin/260807_models_workspace_tabs/000_plan.md lines 42-45,
either add abort handling for every in-flight Models, Shadow, and V2 request
when a panel becomes inactive or narrow the plan’s universal cancellation
requirement; in devlog/_fin/260807_models_workspace_tabs/900_outcome.md lines
67-74, document the same shipped guarantee and its verification coverage,
explicitly identifying any non-cancellable Shadow/V2 requests.
In `@devlog/_fin/260807_models_workspace_tabs/001_audit_round1.md`:
- Around line 102-103: Update the retained-key guidance in the audit note to
remove nav.routing and nav.claude from the list of keys with non-sidebar
consumers, reflecting their removal and replacement by models.tab.routing and
integrations.tab.claude; alternatively, explicitly mark the existing statement
as a superseded assumption.
In `@devlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.md`:
- Around line 176-179: Update the Combos-visible polling test around the initial
`/api/models` request to allow and record the legitimate mount-time fetch,
establishing the baseline after Combos initialization completes. Then assert
that no subsequent catalog-owned polling requests occur after the poll interval,
while preserving the existing checks for the other catalog endpoints and
cold-load count.
In `@devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md`:
- Around line 25-31: Label the fenced ASCII tree block in the documentation with
the text language identifier by adding text to its opening fence, while leaving
the tree contents unchanged.
- Around line 100-103: Update the inactive-panel guidance to require an explicit
author-level rule targeting `.models-tab-panel[hidden]` with `display: none`,
preventing fill-panel styles from overriding the hidden state. Add a mounted
regression test verifying hidden panels do not occupy layout space.
- Around line 76-78: Update the phase 3 combo layout description to use a single
active-tab .page-sub rendered as a direct sibling between the tab strip and
panels, rather than a subtitle inside each panel. Align the surrounding selector
and padding guidance with this DOM structure, preserving the design established
in 002_audit_round2.md and 020_phase2_models_shell.md.
In
`@devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md`:
- Around line 74-82: Update the effect containing the active-state load logic to
abort loadAbortRef and increment loadGenerationRef when the Models workspace
unmounts, not only when active becomes false. Preserve the existing timer
cleanup and ensure any in-flight load started by load cannot continue after
unmount.
In `@gui/src/pages/Models.tsx`:
- Around line 704-716: Update the catalogColdFailure condition near catalogState
and catalog so the failed-cold state is only set when no retained catalog
payload exists, matching the guard used by the Combos panel. Preserve the
existing error message selection and allow the catalog workspace to render with
its stale-load banner when catalog is available.
In `@gui/src/pages/RoutingProfiles.tsx`:
- Around line 311-317: Update the profile count reporting around the load state
and the useEffect that calls onCountChange: track whether a load has
successfully produced data, set that flag only on the success path in load, and
return without reporting while data is still unknown. Keep reporting
profiles.length after successful loads, including a legitimate zero count.
In `@gui/tests/routing-panel-lifecycle.test.tsx`:
- Around line 23-44: Update the global snapshot and cleanup in
beforeEach/afterEach to capture each property descriptor, including
IS_REACT_ACT_ENVIRONMENT, rather than only its value. Restore the original
descriptor for properties that existed and delete properties that were absent
before setup, ensuring no browser globals or React test flag remain after the
test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f7c8e54-7761-4390-8e16-03690dc13bad
⛔ Files ignored due to path filters (3)
devlog/_fin/260807_models_workspace_tabs/evidence/01-models-tab.pngis excluded by!**/*.pngdevlog/_fin/260807_models_workspace_tabs/evidence/02-combos-tab.pngis excluded by!**/*.pngdevlog/_fin/260807_models_workspace_tabs/evidence/03-routing-tab.pngis excluded by!**/*.png
📒 Files selected for processing (44)
devlog/_fin/260807_models_workspace_tabs/000_plan.mddevlog/_fin/260807_models_workspace_tabs/001_audit_round1.mddevlog/_fin/260807_models_workspace_tabs/002_audit_round2.mddevlog/_fin/260807_models_workspace_tabs/003_audit_round3.mddevlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.mddevlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.mddevlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.mddevlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.mddevlog/_fin/260807_models_workspace_tabs/900_outcome.mddocs-site/src/content/docs/guides/combos.mddocs-site/src/content/docs/guides/routing-profile-editor.mddocs-site/src/content/docs/ja/guides/combos.mddocs-site/src/content/docs/ko/guides/combos.mddocs-site/src/content/docs/ru/guides/combos.mddocs-site/src/content/docs/zh-cn/guides/combos.mdgui/src/App.tsxgui/src/app-routing.tsgui/src/components/combo-workspace-detail-panel.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/model-visibility.tsgui/src/pages/Combos.tsxgui/src/pages/Models.tsxgui/src/pages/RoutingProfiles.tsxgui/src/pages/models-shared.tsgui/src/pages/models-tab-strip.tsxgui/src/pages/models-tab.tsgui/src/styles-combos-workspace.cssgui/src/styles-models-workspace.cssgui/src/styles.cssgui/tests/combos-detail-segmented.test.tsgui/tests/combos-detail-tabs-dom.test.tsxgui/tests/models-workspace-panels.test.tsxgui/tests/page-loading-contract.test.tsxgui/tests/routing-panel-lifecycle.test.tsxgui/tests/routing-profiles.test.tsxgui/tests/sidebar-claude-entry.test.tsgui/tests/sidebar-rows.test.tstests/models-workspace-tabs.test.tstests/routing-intelligence-ui.test.ts
💤 Files with no reviewable changes (2)
- gui/tests/sidebar-claude-entry.test.ts
- gui/src/pages/models-shared.ts
Comment on lines
+114
to
+116
| `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. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the stale docs-site scope exclusion.
The PR objectives include documentation updates across multiple locales, but Lines 114-116 explicitly place docs-site out of scope. This makes the roadmap inconsistent with the work being reviewed and can omit the required locale synchronization checks. Update the scope statement or move the documentation changes to a separately declared change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260807_models_workspace_tabs/000_plan.md` around lines 114 - 116,
Update the scope statement in the roadmap to remove the stale docs-site
exclusion, keeping documentation updates and locale synchronization checks
within the declared PR objectives; do not move them to a separate change unless
the roadmap explicitly declares that separate documentation work.
Comment on lines
+120
to
+128
| 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 | ||
| ``` |
Contributor
There was a problem hiding this comment.
🎯 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.
devlog/_fin/260807_models_workspace_tabs/000_plan.md#L120-L128: keep the five-command list as the canonical gate.devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md#L113-L117: addcd gui && bun test tests.devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md#L248-L252: addcd gui && bun test tests.
📍 Affects 3 files
devlog/_fin/260807_models_workspace_tabs/000_plan.md#L120-L128(this comment)devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md#L113-L117devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md#L248-L252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260807_models_workspace_tabs/000_plan.md` around lines 120 - 128,
Use the five-command verification set consistently: keep
devlog/_fin/260807_models_workspace_tabs/000_plan.md lines 120-128 as the
canonical gate, and add `cd gui && bun test tests` to the verification lists in
devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md lines
113-117 and devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md
lines 248-252.
Comment on lines
+100
to
+103
| ## 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. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Specify an explicit hidden-panel display rule.
The native hidden attribute uses display: none, but an author rule such as .models-tab-panel--fill { display: flex; } can override the user-agent rule. devlog/_fin/260807_models_workspace_tabs/900_outcome.md records this exact failure. Require a selector such as .models-tab-panel[hidden] { display: none; } and a mounted regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md` around
lines 100 - 103, Update the inactive-panel guidance to require an explicit
author-level rule targeting `.models-tab-panel[hidden]` with `display: none`,
preventing fill-panel styles from overriding the hidden state. Add a mounted
regression test verifying hidden panels do not occupy layout space.
Comment on lines
+74
to
+82
| Deactivation aborts it and bumps the generation: | ||
|
|
||
| ```tsx | ||
| useEffect(() => { | ||
| if (!active) { loadAbortRef.current?.abort(); loadGenerationRef.current++; return; } | ||
| const timer = window.setTimeout(() => void load(), 0); | ||
| return () => window.clearTimeout(timer); | ||
| }, [active, load]); | ||
| ``` |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Abort the request on unmount as well as deactivation.
The cleanup in Lines 74-82 clears the timer but does not abort loadAbortRef when the Models workspace unmounts while active remains true. A route change can therefore leave load() running after the panel is gone. Add a separate unmount cleanup that aborts the controller and increments loadGenerationRef, or document the existing equivalent cleanup and test it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md`
around lines 74 - 82, Update the effect containing the active-state load logic
to abort loadAbortRef and increment loadGenerationRef when the Models workspace
unmounts, not only when active becomes false. Preserve the existing timer
cleanup and ensure any in-flight load started by load cannot continue after
unmount.
Comment on lines
+23
to
+44
| beforeEach(() => { | ||
| clearClientResourceStoresForTests(); | ||
| previousGlobals = Object.fromEntries(globals.map(k => [k, Reflect.get(globalThis, k)])) as typeof previousGlobals; | ||
| testWindow = new Window({ url: "http://localhost/#models/routing" }); | ||
| 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] }); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the original global property descriptors.
Lines 23-44 restore values only. If a global was absent before setup, cleanup creates an own property with an undefined value. Line 34 also sets IS_REACT_ACT_ENVIRONMENT without restoring it.
Later Bun tests run in the same process. They can observe stale browser globals or the React test flag. Snapshot each property descriptor, then restore the descriptor or delete the property when it did not exist.
Proposed fix
const globals = ["document", "window", "navigator", "localStorage", "sessionStorage"] as const;
-let previousGlobals: Record<(typeof globals)[number], unknown>;
+let previousGlobals: Record<(typeof globals)[number], PropertyDescriptor | undefined>;
+let previousActEnvironment: PropertyDescriptor | undefined;
beforeEach(() => {
clearClientResourceStoresForTests();
- previousGlobals = Object.fromEntries(globals.map(k => [k, Reflect.get(globalThis, k)])) as typeof previousGlobals;
+ previousGlobals = Object.fromEntries(
+ globals.map(k => [k, Object.getOwnPropertyDescriptor(globalThis, k)]),
+ ) as typeof previousGlobals;
+ previousActEnvironment = Object.getOwnPropertyDescriptor(globalThis, "IS_REACT_ACT_ENVIRONMENT");
testWindow = new Window({ url: "http://localhost/#models/routing" });
// ...
});
afterEach(() => {
globalThis.fetch = originalFetch;
clearClientResourceStoresForTests();
testWindow.close();
for (const key of globals) {
- Object.defineProperty(globalThis, key, { configurable: true, value: previousGlobals[key] });
+ const descriptor = previousGlobals[key];
+ if (descriptor) Object.defineProperty(globalThis, key, descriptor);
+ else Reflect.deleteProperty(globalThis, key);
}
+ if (previousActEnvironment) {
+ Object.defineProperty(globalThis, "IS_REACT_ACT_ENVIRONMENT", previousActEnvironment);
+ } else {
+ Reflect.deleteProperty(globalThis, "IS_REACT_ACT_ENVIRONMENT");
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| clearClientResourceStoresForTests(); | |
| previousGlobals = Object.fromEntries(globals.map(k => [k, Reflect.get(globalThis, k)])) as typeof previousGlobals; | |
| testWindow = new Window({ url: "http://localhost/#models/routing" }); | |
| 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] }); | |
| } | |
| }); | |
| const globals = ["document", "window", "navigator", "localStorage", "sessionStorage"] as const; | |
| let previousGlobals: Record<(typeof globals)[number], PropertyDescriptor | undefined>; | |
| let previousActEnvironment: PropertyDescriptor | undefined; | |
| beforeEach(() => { | |
| clearClientResourceStoresForTests(); | |
| previousGlobals = Object.fromEntries( | |
| globals.map(k => [k, Object.getOwnPropertyDescriptor(globalThis, k)]), | |
| ) as typeof previousGlobals; | |
| previousActEnvironment = Object.getOwnPropertyDescriptor(globalThis, "IS_REACT_ACT_ENVIRONMENT"); | |
| testWindow = new Window({ url: "http://localhost/#models/routing" }); | |
| 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) { | |
| const descriptor = previousGlobals[key]; | |
| if (descriptor) Object.defineProperty(globalThis, key, descriptor); | |
| else Reflect.deleteProperty(globalThis, key); | |
| } | |
| if (previousActEnvironment) { | |
| Object.defineProperty(globalThis, "IS_REACT_ACT_ENVIRONMENT", previousActEnvironment); | |
| } else { | |
| Reflect.deleteProperty(globalThis, "IS_REACT_ACT_ENVIRONMENT"); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/routing-panel-lifecycle.test.tsx` around lines 23 - 44, Update the
global snapshot and cleanup in beforeEach/afterEach to capture each property
descriptor, including IS_REACT_ACT_ENVIRONMENT, rather than only its value.
Restore the original descriptor for properties that existed and delete
properties that were absent before setup, ensuring no browser globals or React
test flag remain after the test.
lidge-jun
force-pushed
the
codex/260807-models-workspace-tabs
branch
from
August 7, 2026 08:51
b619948 to
4e861a6
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…inter PR hygiene flagged new_suppression, correctly. The eslint-disable was on a deliberate latest-value ref read in effect cleanup, but 'deliberate' was only asserted in a comment — inline, it reads as a stale-ref mistake to the linter and to the next person. cancelActiveLoad is a stable callback that stops loading, aborts what is running, and moves the generation past whatever the in-flight load captured. Deactivation and unmount both call it, and the suppression is gone rather than justified. Behavior unchanged: the lifecycle test still fails when the guard is reverted.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
devlog/_fin/260807_models_workspace_tabs/002_audit_round2.md (2)
153-156: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOmit the catalog count until catalog data is ready.
On a cold
#models/combosload,models.lengthis initially zero. RenderingeffectiveVisibleCountandmodels.lengthimmediately can produceModels 0/0. Add an explicit readiness or session-seed check and omit the catalog meta until data is known.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/002_audit_round2.md` around lines 153 - 156, Update the `#models/combos` rendering flow to gate the catalog metadata, including effectiveVisibleCount and models.length, on an explicit data-readiness or session-seed condition. Omit the catalog count during the initial cold load when models is empty, and render it once catalog data is known.
176-179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefine the
/api/modelsbaseline before testing inactive polling.Combos legitimately fetches
/api/modelsduring initialization. Wait for that request, record the baseline, and then assert that no additional catalog-owned poll occurs after the interval. Otherwise the test can reject valid Combos behavior or measure the wrong request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/002_audit_round2.md` around lines 176 - 179, Update the inactive-polling test to wait for Combos initialization to complete, capture the resulting /api/models request count as the baseline, then wait through the polling interval and assert that no additional catalog-owned request occurs beyond that baseline.
♻️ Duplicate comments (8)
devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md (1)
74-82: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort the request on unmount as well as deactivation.
The cleanup shown at Lines 74-82 clears only the timer. A route change can unmount the Models workspace while
activeremains true, leavingload()in flight. Add unmount cleanup that abortsloadAbortRefand invalidatesloadGenerationRef.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md` around lines 74 - 82, Update the useEffect managing load scheduling so its cleanup also aborts loadAbortRef and increments loadGenerationRef when the component unmounts, while preserving the existing deactivation behavior and timer cleanup.devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md (3)
25-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLabel the ASCII tree fence.
Add
textto the opening fence at Line 25.markdownlintreports MD040 for the unlabeled block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md` at line 25, Label the ASCII tree code block’s opening fence with the text language identifier to satisfy markdownlint MD040, leaving the block contents unchanged.Source: Linters/SAST tools
100-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd an author rule that preserves
hidden.A fill-panel rule such as
display: flexcan override the user-agent[hidden] { display: none }rule. Require.models-tab-panel[hidden] { display: none; }and a mounted regression test that confirms inactive panels occupy no layout space.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md` around lines 100 - 103, Add an author-level rule for `.models-tab-panel[hidden]` that explicitly sets `display: none`, ensuring fill-panel styles cannot override hidden behavior. Add a mounted regression test confirming inactive panels remain hidden and occupy no layout space.
76-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse one active-tab subtitle layout.
Lines 76-78 describe a subtitle inside each panel. Phase 2 defines one
.page-subas a direct sibling between the tab strip and the panels. Keeping the per-panel design makes the direct-child selectors at Lines 63-65 ineffective. Update this section to describe the single active-tab subtitle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md` around lines 76 - 78, Update the phase 3 layout description to use one `.page-sub` as a direct sibling positioned between the tab strip and panels, rather than describing a subtitle inside each panel. Align the surrounding padding and active-tab behavior with this single shared subtitle so the direct-child selectors remain effective.devlog/_fin/260807_models_workspace_tabs/000_plan.md (2)
114-116: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep
docs-sitein scope.The PR objectives include documentation updates across locales, but Lines 114-116 exclude
docs-site. This can omit the required documentation and locale-synchronization work. Removedocs-sitefrom the exclusion or define a separate documented phase with its own checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/000_plan.md` around lines 114 - 116, Update the scope statement in the plan to include docs-site work, either by removing docs-site from the exclusion list or by defining a separate documented phase with explicit documentation and locale-synchronization checks.Source: Path instructions
42-45: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse one cancellation contract across the plan and outcome.
The plan requires universal cancellation, while the outcome accepts non-aborted Shadow/V2 and catalog-mutation requests.
devlog/_fin/260807_models_workspace_tabs/000_plan.md#L42-L45: narrow the universal requirement or add the missing abort paths.devlog/_fin/260807_models_workspace_tabs/900_outcome.md#L67-L74: document the same shipped exceptions and verification coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/000_plan.md` around lines 42 - 45, Align the cancellation contract across devlog/_fin/260807_models_workspace_tabs/000_plan.md:42-45 and devlog/_fin/260807_models_workspace_tabs/900_outcome.md:67-74: either add abort handling for Shadow/V2 and catalog-mutation requests, or explicitly narrow the plan’s universal requirement to document those exceptions. Update the outcome and verification coverage to match the chosen behavior, including the catalog pollMs and V2 interval paths.devlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.md (1)
176-179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefine the
/api/modelsbaseline before testing inactive polling.Combos performs a legitimate mount-time
/api/modelsfetch. The test plan must wait for that fetch, establish a baseline, and then reject only later catalog-owned polling requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.md` around lines 176 - 179, Update the inactive-polling test around the Combos panel to wait for and record the legitimate mount-time /api/models request before starting the poll interval assertion, then reject only subsequent catalog-owned polling requests. Preserve the existing checks for /api/v2, /api/provider-context-caps, and /api/providers, and retain the cold-load 0/0 assertion.devlog/_fin/260807_models_workspace_tabs/001_audit_round1.md (1)
102-103: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSynchronize the retained navigation-key guidance.
The audit says
nav.routingandnav.clauderemain, while phase 4 says both are removed.
devlog/_fin/260807_models_workspace_tabs/001_audit_round1.md#L102-L103: keep onlynav.combos, or mark the statement as superseded.devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md#L164-L168: retain the corrected replacement keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260807_models_workspace_tabs/001_audit_round1.md` around lines 102 - 103, The retained navigation-key guidance is inconsistent across the audit documents. In devlog/_fin/260807_models_workspace_tabs/001_audit_round1.md lines 102-103, keep only nav.combos or explicitly mark the existing statement as superseded; in devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md lines 164-168, retain the corrected replacement keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_fin/260807_models_workspace_tabs/002_audit_round2.md`:
- Around line 3-5: Update the phase introduction and related summaries in the
audit document to reflect the wp02a/wp02b split: keep legacy Page union members
during wp02a, then remove them in wp02b. Ensure all referenced sections
consistently describe this order so the intermediate commit remains valid.
In `@devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md`:
- Around line 113-117: Update the verification gates to include the separate GUI
test command, `cd gui && bun test tests`, in
devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md lines
113-117. In devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md
lines 248-252, replace “four gates” with the complete five-command gate
including that GUI suite.
In `@devlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.md`:
- Around line 3-5: Update the opening phase contract and the referenced
descriptions so they match the wp02a/wp02b split: state that wp02a additively
introduces the tab strip and replacement panels while retaining the legacy Page
union entries, and that wp02b performs the cutover by removing them. Ensure all
affected sections consistently preserve the legacy pages through wp02a.
- Around line 153-156: Guard the catalog count rendering with the catalog’s
readiness state, not only models.length. In the catalog metadata flow using
effectiveVisibleCount/models.length, omit the count until a session seed or
successful catalog response establishes a known count, preventing direct Combos
loads from displaying Models 0/0.
In
`@devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md`:
- Around line 96-99: Align the routing heading and tab-label definitions so the
tab uses “Routing (beta)” while “Routing Intelligence (beta)” remains the
subtitle. Update the routing test description and assertions around the routing
label and page marker to match these user-facing labels.
- Around line 52-84: Guard the post-save and post-delete continuations before
they call load, using the current active state or load generation so they cannot
start a new reload after deactivation. Update the save/delete handlers that
invoke load; keep abort handling in load unchanged and preserve reload behavior
while the tab remains active.
In `@gui/src/app-routing.ts`:
- Around line 56-62: The Models-tab hash classification is duplicated across
app-routing.ts:56-62 and models-tab.ts:33-38. Consolidate both sites around one
shared isModelsTabHash (or equivalent) classifier, used by hashBelongsToPage and
readModelsTab; preserve readModelsTab’s legacy top-level aliases while ensuring
future tab or alias changes require only one definition.
In `@gui/tests/models-workspace-panels.test.tsx`:
- Around line 391-401: Update the test around the tab switches and failNext
handling to invalidate the retained Combo resource or invoke the supported
reload action, ensuring the next activation actually requests /api/combos. Track
the request and assert that the failing request occurs before verifying that
.combos-workspace-root remains mounted.
---
Outside diff comments:
In `@devlog/_fin/260807_models_workspace_tabs/002_audit_round2.md`:
- Around line 153-156: Update the `#models/combos` rendering flow to gate the
catalog metadata, including effectiveVisibleCount and models.length, on an
explicit data-readiness or session-seed condition. Omit the catalog count during
the initial cold load when models is empty, and render it once catalog data is
known.
- Around line 176-179: Update the inactive-polling test to wait for Combos
initialization to complete, capture the resulting /api/models request count as
the baseline, then wait through the polling interval and assert that no
additional catalog-owned request occurs beyond that baseline.
---
Duplicate comments:
In `@devlog/_fin/260807_models_workspace_tabs/000_plan.md`:
- Around line 114-116: Update the scope statement in the plan to include
docs-site work, either by removing docs-site from the exclusion list or by
defining a separate documented phase with explicit documentation and
locale-synchronization checks.
- Around line 42-45: Align the cancellation contract across
devlog/_fin/260807_models_workspace_tabs/000_plan.md:42-45 and
devlog/_fin/260807_models_workspace_tabs/900_outcome.md:67-74: either add abort
handling for Shadow/V2 and catalog-mutation requests, or explicitly narrow the
plan’s universal requirement to document those exceptions. Update the outcome
and verification coverage to match the chosen behavior, including the catalog
pollMs and V2 interval paths.
In `@devlog/_fin/260807_models_workspace_tabs/001_audit_round1.md`:
- Around line 102-103: The retained navigation-key guidance is inconsistent
across the audit documents. In
devlog/_fin/260807_models_workspace_tabs/001_audit_round1.md lines 102-103, keep
only nav.combos or explicitly mark the existing statement as superseded; in
devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md
lines 164-168, retain the corrected replacement keys.
In `@devlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.md`:
- Around line 176-179: Update the inactive-polling test around the Combos panel
to wait for and record the legitimate mount-time /api/models request before
starting the poll interval assertion, then reject only subsequent catalog-owned
polling requests. Preserve the existing checks for /api/v2,
/api/provider-context-caps, and /api/providers, and retain the cold-load 0/0
assertion.
In `@devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md`:
- Line 25: Label the ASCII tree code block’s opening fence with the text
language identifier to satisfy markdownlint MD040, leaving the block contents
unchanged.
- Around line 100-103: Add an author-level rule for `.models-tab-panel[hidden]`
that explicitly sets `display: none`, ensuring fill-panel styles cannot override
hidden behavior. Add a mounted regression test confirming inactive panels remain
hidden and occupy no layout space.
- Around line 76-78: Update the phase 3 layout description to use one
`.page-sub` as a direct sibling positioned between the tab strip and panels,
rather than describing a subtitle inside each panel. Align the surrounding
padding and active-tab behavior with this single shared subtitle so the
direct-child selectors remain effective.
In
`@devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md`:
- Around line 74-82: Update the useEffect managing load scheduling so its
cleanup also aborts loadAbortRef and increments loadGenerationRef when the
component unmounts, while preserving the existing deactivation behavior and
timer cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 23d5402f-7d2f-40cd-921f-361dd75eb53c
⛔ Files ignored due to path filters (3)
devlog/_fin/260807_models_workspace_tabs/evidence/01-models-tab.pngis excluded by!**/*.pngdevlog/_fin/260807_models_workspace_tabs/evidence/02-combos-tab.pngis excluded by!**/*.pngdevlog/_fin/260807_models_workspace_tabs/evidence/03-routing-tab.pngis excluded by!**/*.png
📒 Files selected for processing (44)
devlog/_fin/260807_models_workspace_tabs/000_plan.mddevlog/_fin/260807_models_workspace_tabs/001_audit_round1.mddevlog/_fin/260807_models_workspace_tabs/002_audit_round2.mddevlog/_fin/260807_models_workspace_tabs/003_audit_round3.mddevlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.mddevlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.mddevlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.mddevlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.mddevlog/_fin/260807_models_workspace_tabs/900_outcome.mddocs-site/src/content/docs/guides/combos.mddocs-site/src/content/docs/guides/routing-profile-editor.mddocs-site/src/content/docs/ja/guides/combos.mddocs-site/src/content/docs/ko/guides/combos.mddocs-site/src/content/docs/ru/guides/combos.mddocs-site/src/content/docs/zh-cn/guides/combos.mdgui/src/App.tsxgui/src/app-routing.tsgui/src/components/combo-workspace-detail-panel.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/model-visibility.tsgui/src/pages/Combos.tsxgui/src/pages/Models.tsxgui/src/pages/RoutingProfiles.tsxgui/src/pages/models-shared.tsgui/src/pages/models-tab-strip.tsxgui/src/pages/models-tab.tsgui/src/styles-combos-workspace.cssgui/src/styles-models-workspace.cssgui/src/styles.cssgui/tests/combos-detail-segmented.test.tsgui/tests/combos-detail-tabs-dom.test.tsxgui/tests/models-workspace-panels.test.tsxgui/tests/page-loading-contract.test.tsxgui/tests/routing-panel-lifecycle.test.tsxgui/tests/routing-profiles.test.tsxgui/tests/sidebar-claude-entry.test.tsgui/tests/sidebar-rows.test.tstests/models-workspace-tabs.test.tstests/routing-intelligence-ui.test.ts
💤 Files with no reviewable changes (2)
- gui/tests/sidebar-claude-entry.test.ts
- gui/src/pages/models-shared.ts
Comment on lines
+3
to
+5
| Round 1's eight blockers came back as three resolved, four partially resolved, and one | ||
| resolved-with-a-caveat, plus five new findings. Accepted in full again. The pattern is | ||
| consistent and worth naming: round 1 caught *missing* work, round 2 caught **rules |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the phase introduction match the wp02a/wp02b split.
Lines 3-5 say the phase removes the Page union members immediately. Later sections correctly keep the legacy pages in wp02a and remove them in wp02b. Update the introduction so the documented order cannot produce a broken intermediate commit.
Also applies to: 76-85, 181-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260807_models_workspace_tabs/002_audit_round2.md` around lines 3
- 5, Update the phase introduction and related summaries in the audit document
to reflect the wp02a/wp02b split: keep legacy Page union members during wp02a,
then remove them in wp02b. Ensure all referenced sections consistently describe
this order so the intermediate commit remains valid.
Comment on lines
+113
to
+117
| ## 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. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run the separate GUI suite in every phase gate.
The canonical gate includes cd gui && bun test tests, but these phase documents omit it.
devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md#L113-L117: add the GUI test command.devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md#L248-L252: replace “four gates” with the complete five-command gate.
📍 Affects 2 files
devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md#L113-L117(this comment)devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md#L248-L252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md` around
lines 113 - 117, Update the verification gates to include the separate GUI test
command, `cd gui && bun test tests`, in
devlog/_fin/260807_models_workspace_tabs/010_phase1_routing_layer.md lines
113-117. In devlog/_fin/260807_models_workspace_tabs/030_phase3_combos_embed.md
lines 248-252, replace “four gates” with the complete five-command gate
including that GUI suite.
Comment on lines
+3
to
+5
| 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. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the phase introduction match the wp02a/wp02b split.
Lines 3-5 describe immediate removal of the legacy pages, while wp02a explicitly keeps them until wp02b. Update the opening contract to state that wp02a is additive and wp02b performs the cutover.
Also applies to: 76-85, 181-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.md` around
lines 3 - 5, Update the opening phase contract and the referenced descriptions
so they match the wp02a/wp02b split: state that wp02a additively introduces the
tab strip and replacement panels while retaining the legacy Page union entries,
and that wp02b performs the cutover by removing them. Ensure all affected
sections consistently preserve the legacy pages through wp02a.
Comment on lines
+153
to
+156
| 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. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard the catalog count with readiness state.
models.length is zero before a session seed or successful catalog response. Without a readiness check, a direct Combos load can display Models 0/0. Omit the meta until the catalog count is known.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260807_models_workspace_tabs/020_phase2_models_shell.md` around
lines 153 - 156, Guard the catalog count rendering with the catalog’s readiness
state, not only models.length. In the catalog metadata flow using
effectiveVisibleCount/models.length, omit the count until a session seed or
successful catalog response establishes a known count, preventing direct Combos
loads from displaying Models 0/0.
Comment on lines
+52
to
+84
| Audit round 2 pushed further: saying "add a signal" does not say *whose*. `load` has four | ||
| entry points — the initial effect (`:243`), Retry (`:426`), post-save (`:291`), and | ||
| post-delete (`:321`). An effect-local controller cancels only the first, so a Retry or a | ||
| mutation reload keeps running in the background after the tab hides. Generation | ||
| invalidation blocks the state write but not the network work. | ||
|
|
||
| So the controller belongs to `load` itself, at component level: | ||
|
|
||
| ```tsx | ||
| const loadAbortRef = useRef<AbortController | null>(null); | ||
|
|
||
| const load = useCallback(async (preferredId?: string) => { | ||
| loadAbortRef.current?.abort(); // supersede whatever was in flight | ||
| const controller = new AbortController(); | ||
| loadAbortRef.current = controller; | ||
| const generation = ++loadGenerationRef.current; | ||
| // ...every fetch takes { signal: controller.signal } | ||
| // clear the ref only if this request still owns it: | ||
| if (loadAbortRef.current === controller) loadAbortRef.current = null; | ||
| }, [...]); | ||
| ``` | ||
|
|
||
| Deactivation aborts it and bumps the generation: | ||
|
|
||
| ```tsx | ||
| useEffect(() => { | ||
| if (!active) { loadAbortRef.current?.abort(); loadGenerationRef.current++; return; } | ||
| const timer = window.setTimeout(() => void load(), 0); | ||
| return () => window.clearTimeout(timer); | ||
| }, [active, load]); | ||
| ``` | ||
|
|
||
| Every entry point is covered because they all go through `load`. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard late save and delete continuations.
Aborting the current controller does not prevent a save or delete continuation from calling load() after the tab becomes inactive. That call creates a new controller and starts new requests. Guard every post-save and post-delete reload with the current active state or generation, and do not rely on abort alone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md`
around lines 52 - 84, Guard the post-save and post-delete continuations before
they call load, using the current active state or load generation so they cannot
start a new reload after deactivation. Update the save/delete handlers that
invoke load; keep abort handling in load unchanged and preserve reload behavior
while the tab remains active.
Comment on lines
+96
to
+99
| `gui/tests/routing-profiles.test.tsx:175` asserts the literal "Routing Intelligence | ||
| (beta)" and `[data-page="routing"]`; both change with this decision. The test follows the | ||
| design, not the reverse — but the string does not vanish from the product, it becomes the | ||
| tab label and the subtitle. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the routing heading text with the tab-label table.
Lines 96-99 say Routing Intelligence (beta) becomes the tab label. Lines 148-156 define the tab label as Routing (beta) and reserve a separate subtitle. State which string remains in the subtitle and update the test description to match the actual user-facing labels.
Also applies to: 148-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_fin/260807_models_workspace_tabs/040_phase4_routing_embed_and_sidebar.md`
around lines 96 - 99, Align the routing heading and tab-label definitions so the
tab uses “Routing (beta)” while “Routing Intelligence (beta)” remains the
subtitle. Update the routing test description and assertions around the routing
label and page marker to match these user-facing labels.
Comment on lines
+56
to
+62
| /** | ||
| * Models owns three tabs: the catalog, Combos, and Routing. The catalog is the bare | ||
| * `#models`, so it has no suffix entry here — same convention Dashboard uses for | ||
| * Overview and Logs uses for the log list. | ||
| */ | ||
| export const MODELS_TAB_HASHES = ["models/combos", "models/routing"] as const; | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Two independent definitions of "which hash is a Models tab." gui/src/app-routing.ts and gui/src/pages/models-tab.ts each maintain their own logic for classifying a hash as the Combos or Routing tab. Nothing keeps them in sync if either changes.
gui/src/app-routing.ts#L56-L62:MODELS_TAB_HASHES(["models/combos", "models/routing"]) andhashBelongsToPagedefine the canonical, exact-match set of nested Models-tab hashes used for page-routing decisions. Export a shared classifier (or reusereadModelsTab's logic) instead of maintaining a parallel list here.gui/src/pages/models-tab.ts#L33-L38:readModelsTabre-implements the same classification independently, with extra legacy top-level fallback handling (combos,combos/*,routing,routing/*). Consolidate this with theapp-routing.tsdefinition, for example by having one module export a singleisModelsTabHash(or equivalent) function that bothhashBelongsToPageandreadModelsTabcall, so a future added tab or legacy alias only needs one edit.
📍 Affects 2 files
gui/src/app-routing.ts#L56-L62(this comment)gui/src/pages/models-tab.ts#L33-L38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/app-routing.ts` around lines 56 - 62, The Models-tab hash
classification is duplicated across app-routing.ts:56-62 and
models-tab.ts:33-38. Consolidate both sites around one shared isModelsTabHash
(or equivalent) classifier, used by hashBelongsToPage and readModelsTab;
preserve readModelsTab’s legacy top-level aliases while ensuring future tab or
alias changes require only one definition.
Comment on lines
+391
to
+401
| 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(); }); | ||
|
|
||
| expect(panel(container, "combos")?.querySelector(".combos-workspace-root")).toBeTruthy(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the failed reload before asserting workspace retention.
Lines 391-401 only switch tabs after the initial Combo load. The test does not evict the loaded resource, trigger a supported reload, or verify that /api/combos runs after failNext becomes true.
If reactivation reuses the retained resource, the throwing fetch branch never runs. The test then passes even when the failed-cold path replaces the workspace.
Invalidate the Combo resource or trigger the supported reload action. Count and assert the failing /api/combos request before asserting that .combos-workspace-root remains mounted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/models-workspace-panels.test.tsx` around lines 391 - 401, Update
the test around the tab switches and failNext handling to invalidate the
retained Combo resource or invoke the supported reload action, ensuring the next
activation actually requests /api/combos. Track the request and assert that the
failing request occurs before verifying that .combos-workspace-root remains
mounted.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
gui/src/pages/RoutingProfiles.tsx (1)
315-321: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not report a profile count before a successful load.
profilesstarts as[], so Line 320 reports0before/api/routing-profilesreturns. If the request fails,Modelsreceives and retains a known-zero count while this panel showsloadError.Track successful profile loading. Set the flag only after the profile response is processed. Return from this effect while the count is unknown.
Proposed fix
const [profiles, setProfiles] = useState<RoutingProfileDto[]>([]); + const [profilesLoaded, setProfilesLoaded] = useState(false); const [analytics, setAnalytics] = useState<Analytics | null>(null); @@ selectedRef.current = refreshed; setProfiles(nextProfiles); + setProfilesLoaded(true); setSelected(refreshed); @@ useEffect(() => { + if (!profilesLoaded) return; onCountChange?.(profiles.length); - }, [onCountChange, profiles.length]); + }, [onCountChange, profiles.length, profilesLoaded]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/pages/RoutingProfiles.tsx` around lines 315 - 321, Update the profile-loading state in the RoutingProfiles component and set it only after the /api/routing-profiles response has been successfully processed. In the effect that calls onCountChange, return without reporting while successful loading has not occurred, so an initial empty profiles array or failed request does not publish a zero count.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@gui/src/pages/RoutingProfiles.tsx`:
- Around line 315-321: Update the profile-loading state in the RoutingProfiles
component and set it only after the /api/routing-profiles response has been
successfully processed. In the effect that calls onCountChange, return without
reporting while successful loading has not occurred, so an initial empty
profiles array or failed request does not publish a zero count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0845b0b5-625b-4415-8a36-9b4916e80a23
📒 Files selected for processing (1)
gui/src/pages/RoutingProfiles.tsx
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Models page becomes a three-tab workspace — Models / Combos / Routing — and the sidebar drops from 11 rows to 9.
These are not three unrelated screens sharing a container. They are the same question at three depths, and the answer to all three is a model id the client can call:
anthropic/claude-opus-5combo/<id>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.
Two sidebar rows go.
Routing (beta)moves into the strip.Claudewas never a page — it was a shortcut into a tab of Integrations, and paying for it meantsubPath,activeHashes, anavHashmirror, and anisNavEntryActivehelper 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.#integrations/claudeand its Desktop route are untouched.Combos was already hidden. It had no sidebar row; the only way in was a
Set uplink on a card inside Models. So for Combos this is one level shallower, not deeper — a card link that swapped the whole page becomes a sibling tab. That card is gone as duplicate navigation, and the tab carries a live count instead.Old links keep working:
#combos,#routing, and their/anythingsuffixes redirect passively to the nested destination, so Back is never trapped.Cost, stated plainly
Routing (beta) loses sidebar discoverability. It is a young feature and moving it one level in means fewer people stumble onto it. Mitigations are real but partial: the strip is visible the moment anyone opens Models, the tab carries a live profile count, and the subtitle names routing directly. This is a trade, not a free win. Claude loses nothing.
Design warrant
Primer's UnderlineNav guidance says not to stack underline tab rows, and that a tab changing the URL is
UnderlineNavwhile one swapping content without touching the URL isUnderlinePanels. That is why every tab here owns a hash, and why the Combos detail panel'sConfig/Aboutunderline row was demoted to segmented pills — it would otherwise sit directly beneath the page strip.Worth recording honestly: the accessibility specs do not forbid nested tabs. No W3C/APG page prohibits a
tablistinside atabpanel. The demotion is a visual decision backed by Primer and Carbon, not an accessibility fix.Verification
bun run testdoes not reachgui/tests/—scripts/test.tsdefaults to./tests/, so the 116-file GUI suite needs its own command. Both were run at every phase boundary.Live browser at 1280×720, every result read back:
#models/routingkeeps the tab; Back returns to#models/combos, Forward to#models/routing, selection following#combos,#routing,#routing/anythingland on their tabs;#models/nopenormalizes to#modelsactiveCount1display: noneScreenshots
Captured against a neutral fixture rather than the live proxy — the real dashboard shows account namespaces and usage history.
Models
Combos — full-bleed workspace under the header and strip;
Config/Aboutnow pillsRouting (beta)
What review caught that five green gates did not
Every phase ended green on all five commands. These were all present in a green tree:
display: flexbeats the UA's[hidden] { display: none }. Mounting both panels to fix brokenaria-controlsIDREFs left a hidden panel painting anyway.Each regression test added here was driven red against its own reverted fix before being trusted — two earlier attempts passed with the fix removed and were rewritten.
Accepted residuals
Pre-existing, none introduced here: Shadow/V2 requests already in flight are not aborted (only future scheduling stops); the catalog's mutation refresh owns an untracked controller; the Combos tab has no cross-tab error badge, though failures stay visible inside the panel.
Full record in
devlog/_fin/260807_models_workspace_tabs/, including three roadmap audit rounds and the per-phase reviews.Checklist
Docs: the five dashboard guides (en/ja/ko/ru/zh-cn) said to choose Combos or Routing in the sidebar and now say Models → Combos / Models → Routing.
No
src/runtime, routing-engine, or management-API contract changed.Summary by CodeRabbit
New Features
Documentation