feat(gui): show only usable vision describers in the delegation card shape - #1328
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change defines vision-model eligibility and management API validation, then integrates server-provided options into dashboard state, persistence, rendering, and reasoning controls. Supporting documents record capability research, audits, stacked publication, landing, and verification. ChangesVision sidecar model filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SidecarSettingsAPI
participant useDashboardData
participant DashboardSidecarPanels
SidecarSettingsAPI-->>useDashboardData: visionModels
useDashboardData->>useDashboardData: derive vision model options
useDashboardData-->>DashboardSidecarPanels: options and sidecar state
DashboardSidecarPanels->>SidecarSettingsAPI: save model and reasoning settings
SidecarSettingsAPI-->>useDashboardData: updated settings and visionModels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 373e5ad653
ℹ️ 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".
| onChange={model => { | ||
| const ladder = visionReasoningLadder(models, model); | ||
| const reasoning = clampVisionReasoningToLadder(ladder, visionReasoning); | ||
| void saveSidecar({ vision: { model, backend: sidecarBackendForModel(models, model), reasoning } }); |
There was a problem hiding this comment.
Save the backend supplied with each vision option
When an offered model comes from an Anthropic-adapter provider whose configured provider name is not literally anthropic, this call infers openai from /api/models even though /api/sidecar-settings supplied backend: "anthropic" for the option; the same misclassification occurs for an Anthropic baseline absent from /api/models. Selecting that model therefore persists the wrong explicit backend, causing the runtime to invoke the OpenAI describer with a Claude model or skip vision entirely. Preserve each server option's backend in visionModelOptions and use it here instead of sidecarBackendForModel.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes for a functional persistence bug. The server option already carries its backend, but the dashboard drops that identity and later re-infers the backend from /api/models. For an Anthropic-compatible provider not literally named anthropic, or for an Anthropic baseline absent from the catalog, selecting the option can persist backend: openai and route a Claude model through the wrong describer.\n\nPreserve the backend with each vision option and submit that exact backend when the user selects it. Add a regression that selects an Anthropic option whose provider name is custom and verifies the saved backend remains anthropic. I am not making the final UI/UX approval decision; @lidge-jun should do that after the functional stack is corrected.
b640fe4 to
6196fc5
Compare
… shape Two review comments on the running dashboard drove this: make the Vision sidecar card match the subagent delegation panel above it, and follow that panel's pattern for the reasoning control -- raw wire values, compact. The picker now consumes the server's eligible-describer list instead of filtering by provider name, so it stops offering models that cannot see. The field is optional: an older server omits it and the client falls back to the previous list rather than rendering an empty picker. saveSidecar rebuilds SidecarData field by field at three separate sites, so a new field is dropped unless all three are updated. Missing the success writeback alone would have reverted the picker to the legacy list after every save -- on exactly the interaction the user performs. The effort select renders the wire value (low..max) rather than a localized label, matching the delegation panel. The card's own prose stays translated; only the enum values are raw. Layout is the panel itself, not an inner row: .dash-sidecar-card sets flex-direction: column and .dash-delegation-summary never resets it, so combining the two classes would have produced a column. The control group holds its intrinsic width and the copy block is the one that yields, because letting the controls shrink slides them left underneath the hint text instead of wrapping. Below 22rem the card stacks rather than overflowing. Verified by rendering the built dashboard at 1440/1280/768/420/330 and reading the result back; screenshots are in the plan unit's evidence directory. Plan: devlog/_plan/260809_vision_sidecar_model_filter/030
The vision card moved to the delegation panel shape; the web search card beside it kept the old title-row-plus-hint-underneath shell, so the pair read as two different components in one grid. No behavior change. The web search card keeps its own model list and its single control; only the shell moves. Three follow-ons fall out of sharing the shell: - dash-vision-card becomes dash-sidecar-row-card, since it never described anything vision-specific -- it carries the grid cell's min-width while .dash-delegation-summary supplies the row layout. - the compact effort rule now matches :nth-child(2) rather than :last-child. The web search card has a single select that is both first and last child, and :last-child would have shrunk its model picker to the effort width. - .dash-sidecar-card and .dash-sidecar-card__row lose their last user, so their rules are removed rather than left to rot. .dash-sidecar-grid switches from align-items: start to stretch: the two cards are a matched pair, and a hint that wraps to a different line count in one should not leave the other visibly short. Verified by rendering the built dashboard at 1440/1280/768/420/330: both cards report flex-direction row with equal heights while the grid is two-up, no control overflow, and a positive hint-to-control gap throughout.
The server now says which executor would run each describer, and the picker discarded it and re-derived one from /api/models, where only a provider named literally "anthropic" reads as Anthropic. A Claude model offered by a differently-named adapter, or a baseline missing from a stale catalog, was saved as openai — the runtime then called the OpenAI describer with a Claude model, or planned no vision at all. Options keep their backend through the memo, and the save handler prefers it. Catalog inference remains for legacy and grandfathered entries, which carry no server backend.
…ption Audit follow-up. The server-backend fix covered listed options but not the compatibility path: against a server that sends no option list, the configured model is unshifted as a bare entry, so the next save re-inferred its backend from /api/models and rewrote a working Anthropic describer as openai. The persisted backend now travels with that entry. What is already stored is better evidence than an inference over a catalog that never knew the model.
51bfc78 to
c0e6512
Compare
Final cross-layer audit. The client collapsed "no visionModels key" and "visionModels: []" into one legacy branch, so a current server that had computed nothing eligible got its answer replaced by the unfiltered openai+anthropic catalog — putting back the text-only rows this feature removes. Only an absent key now means a server that predates the field. An empty list is a real answer and is shown as one, with the configured model still grandfathered so the picker cannot silently rewrite it.
Records the merge commits, what each user requirement is answered by, and the four audit rounds — including the last one, which caught a GUI path that would have quietly undone the filter after both halves had been reviewed separately.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/260809_vision_sidecar_model_filter/000_plan.md`:
- Line 55: Update the opening pseudocode or diagram fences to specify the text
language: use ```text at
devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md lines 55 and 125,
040_stack_publication.md line 8, 050_stack_landing.md line 17, and
002_audit_synthesis.md line 19. No other fence content needs changing.
In
`@devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md`:
- Around line 131-155: Synchronize the documented visionEligibleModelOptions
contract with its four-argument caller: in
devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md
lines 131-155, document the active Anthropic provider-name parameter and its
routing behavior; in
devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md
lines 54-80 and 158-160, pass and document that provider name in both helper
examples so they use the same contract.
In `@devlog/_fin/260809_vision_sidecar_model_filter/060_outcome.md`:
- Around line 5-9: Replace the `(this unit's close)` placeholder in the layer-3
dashboard card row with the actual layer-3 merge commit SHA, preserving the
existing table structure and ensuring the shipped outcome records an auditable
commit.
In `@gui/src/styles-dashboard-workspace.css`:
- Around line 115-125: Update the narrow-breakpoint styles for
.dash-sidecar-row-card .dash-delegation-controls so the model and effort selects
can wrap or stack instead of remaining in a single non-wrapping row. Preserve
their minimum widths and ensure the responsive layout prevents horizontal
overflow.
🪄 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: 32c1e885-0394-4cfa-b659-a0727bcb9a0f
⛔ Files ignored due to path filters (3)
devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_sidecar_cards_matched.pngis excluded by!**/*.pngdevlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_delegation_form.pngis excluded by!**/*.pngdevlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_filtered_dropdown_ko.pngis excluded by!**/*.png
📒 Files selected for processing (14)
devlog/_fin/260809_vision_sidecar_model_filter/000_plan.mddevlog/_fin/260809_vision_sidecar_model_filter/001_capability_signal_inventory.mddevlog/_fin/260809_vision_sidecar_model_filter/002_audit_synthesis.mddevlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.mddevlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.mddevlog/_fin/260809_vision_sidecar_model_filter/030_dashboard_vision_card.mddevlog/_fin/260809_vision_sidecar_model_filter/040_stack_publication.mddevlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.mddevlog/_fin/260809_vision_sidecar_model_filter/060_outcome.mdgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/src/pages/use-dashboard-data.tsgui/src/styles-dashboard-workspace.cssgui/tests/vision-model-options.test.ts
| | Layer | PR | Merge commit | What landed | | ||
| |---|---|---|---| | ||
| | 1 — eligibility predicate | #1326 | `eebd9d48f` | `src/vision/eligibility.ts`, the devlog unit | | ||
| | 2 — management API + write gate | #1327 | `d4758bc94` | options module, both routes, shared model resolver | | ||
| | 3 — dashboard card | #1328 | (this unit's close) | filtered picker, backend provenance, card shell | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record the layer-3 merge commit.
This document states that the stack shipped, but the dashboard-card row uses (this unit's close) instead of the actual merge commit. 050_stack_landing.md requires each layer's merge commit to be recorded, so the final outcome is not auditable.
Replace the placeholder with the layer-3 merge SHA before publishing the shipped outcome.
Proposed documentation fix
- | 3 — dashboard card | `#1328` | (this unit's close) | filtered picker, backend provenance, card shell |
+ | 3 — dashboard card | `#1328` | `<layer-3 merge SHA>` | filtered picker, backend provenance, card shell |🤖 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/260809_vision_sidecar_model_filter/060_outcome.md` around lines 5
- 9, Replace the `(this unit's close)` placeholder in the layer-3 dashboard card
row with the actual layer-3 merge commit SHA, preserving the existing table
structure and ensuring the shipped outcome records an auditable commit.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 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/260809_vision_sidecar_model_filter/000_plan.md`:
- Line 55: Update the opening pseudocode or diagram fences to specify the text
language: use ```text at
devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md lines 55 and 125,
040_stack_publication.md line 8, 050_stack_landing.md line 17, and
002_audit_synthesis.md line 19. No other fence content needs changing.
In
`@devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md`:
- Around line 131-155: Synchronize the documented visionEligibleModelOptions
contract with its four-argument caller: in
devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md
lines 131-155, document the active Anthropic provider-name parameter and its
routing behavior; in
devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md
lines 54-80 and 158-160, pass and document that provider name in both helper
examples so they use the same contract.
In `@devlog/_fin/260809_vision_sidecar_model_filter/060_outcome.md`:
- Around line 5-9: Replace the `(this unit's close)` placeholder in the layer-3
dashboard card row with the actual layer-3 merge commit SHA, preserving the
existing table structure and ensuring the shipped outcome records an auditable
commit.
In `@gui/src/styles-dashboard-workspace.css`:
- Around line 115-125: Update the narrow-breakpoint styles for
.dash-sidecar-row-card .dash-delegation-controls so the model and effort selects
can wrap or stack instead of remaining in a single non-wrapping row. Preserve
their minimum widths and ensure the responsive layout prevents horizontal
overflow.
🪄 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: 32c1e885-0394-4cfa-b659-a0727bcb9a0f
⛔ Files ignored due to path filters (3)
devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_sidecar_cards_matched.pngis excluded by!**/*.pngdevlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_delegation_form.pngis excluded by!**/*.pngdevlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_filtered_dropdown_ko.pngis excluded by!**/*.png
📒 Files selected for processing (14)
devlog/_fin/260809_vision_sidecar_model_filter/000_plan.mddevlog/_fin/260809_vision_sidecar_model_filter/001_capability_signal_inventory.mddevlog/_fin/260809_vision_sidecar_model_filter/002_audit_synthesis.mddevlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.mddevlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.mddevlog/_fin/260809_vision_sidecar_model_filter/030_dashboard_vision_card.mddevlog/_fin/260809_vision_sidecar_model_filter/040_stack_publication.mddevlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.mddevlog/_fin/260809_vision_sidecar_model_filter/060_outcome.mdgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/src/pages/use-dashboard-data.tsgui/src/styles-dashboard-workspace.cssgui/tests/vision-model-options.test.ts
🛑 Comments failed to post (2)
devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add language identifiers to the documentation fences.
markdownlint-cli2reports MD040 for these opening fences. Addtextto each pseudocode or diagram fence to keep the documentation lint-clean.
devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md#L55-L55: use```textfor the eligibility formula.devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md#L125-L125: use```textfor the picker/write-gate rule.devlog/_fin/260809_vision_sidecar_model_filter/040_stack_publication.md#L8-L8: use```textfor the branch diagram.devlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.md#L17-L17: use```textfor the merge diagram.devlog/_fin/260809_vision_sidecar_model_filter/002_audit_synthesis.md#L19-L19: use```textfor the tri-state rule.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 4 files
devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md#L55-L55(this comment)devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md#L125-L125devlog/_fin/260809_vision_sidecar_model_filter/040_stack_publication.md#L8-L8devlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.md#L17-L17devlog/_fin/260809_vision_sidecar_model_filter/002_audit_synthesis.md#L19-L19🤖 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/260809_vision_sidecar_model_filter/000_plan.md` at line 55, Update the opening pseudocode or diagram fences to specify the text language: use ```text at devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md lines 55 and 125, 040_stack_publication.md line 8, 050_stack_landing.md line 17, and 002_audit_synthesis.md line 19. No other fence content needs changing.Source: Linters/SAST tools
devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md (1)
131-155: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the documented eligibility API with its caller.
The supplied
src/server/management/vision-sidecar-options.tscallsvisionEligibleModelOptionswithanthropicSidecar?.providerNameas a fourth argument. These snippets define or call a three-argument API. A developer who follows this documentation can create a TypeScript call-signature mismatch or omit the active Anthropic-provider selection behavior.
devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md#L131-L155: Document the fourth parameter in the function contract and its Anthropic routing behavior.devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md#L54-L80: Pass and document the active Anthropic provider name in the helper example.devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md#L158-L160: Update the replacement helper example to match the same contract.📍 Affects 2 files
devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md#L131-L155(this comment)devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md#L54-L80devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md#L158-L160🤖 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/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md` around lines 131 - 155, Synchronize the documented visionEligibleModelOptions contract with its four-argument caller: in devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md lines 131-155, document the active Anthropic provider-name parameter and its routing behavior; in devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md lines 54-80 and 158-160, pass and document that provider name in both helper examples so they use the same contract.
Review finding. Below the stacking breakpoint the control group still held nowrap, so the two selects' minimums (10.5rem + 6.5rem) exceeded the viewport the query targets. The reason for nowrap was children sliding under the hint in the row layout, which no longer applies once the controls own their own line.
Summary
Builds on #1327. Two inline review comments on the running dashboard drove this: make the Vision sidecar card match the subagent delegation panel above it, and follow that panel's pattern for the reasoning control — raw wire values, compact.
The picker now consumes the server's eligible-describer list from #1327 instead of filtering by provider name, so it stops offering models that cannot see. The field is optional: an older server omits it and the client falls back to the previous list rather than rendering an empty picker.
Three things a reviewer should look at:
saveSidecarrebuildsSidecarDatafield by field at three separate sites (optimistic update, success writeback, session cache), so a new field is silently dropped unless all three are updated. Missing the success writeback alone would have reverted the picker to the legacy list after every save — on exactly the interaction the user performs..dash-sidecar-cardsetsflex-direction: columnand.dash-delegation-summarynever resets that property, so combining the two classes would have produced a column regardless of import order..custom-selectkeeps its own min-width, so the children overflow their shrunken container and slide left underneath the hint text. Below 22rem the card stacks instead of overflowing.51bfc78a5), so the pair stops reading as two different components in one grid. No behavior change there — it keeps its own model list and its single control. Two consequences worth noting: the compact effort rule matches:nth-child(2)rather than:last-child, because the web search card's single select is simultaneously first and last child and would otherwise be shrunk to the effort width; and.dash-sidecar-card/.dash-sidecar-card__rowlose their last user, so those rules are removed rather than left to rot. The grid also moves fromalign-items: starttostretchso a hint wrapping to a different line count cannot leave one card visibly short.The effort select renders the wire value (
low…max) rather than a localized label. The card's own prose stays translated; only the enum values are raw.visionReasoningLabelis kept becausegui/tests/vision-reasoning-contract.test.tsstill covers it.Verification
bun run typecheck— exit 0bun run lint:gui— cleanbun run build:gui— exit 0bun test ./gui/tests— 699 pass, 0 failbun run test(full suite) — 10146 pass, 0 fail, 632 filesOPENCODEX_HOME, headless Chrome over CDP, cache-busting reloads):GET /api/sidecar-settingsreturned 7 eligible models,gpt-5.6-lunafirst and flagged baseline; the blind rows are absentlow, not낮음flex-direction: rowwith equal heights while the grid is two-up, no control overflow, positive gaps throughoutbun test ./gui/testsafter the shell change — 703 pass, 0 failBoth sidecar cards sharing the shell, equal height while the grid is two-up:
Checklist
Stack (merge bottom-up):
Depends on #1327. Review this PR's diff only.
Summary by CodeRabbit
New Features
Bug Fixes
Tests