fix(desktop): guide model selection when changing the default connection - #5866
Conversation
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head f481fbade712dac3d77ff2a8460b7e99616b6cdb. The new chooser makes an explicit model selection when no single enabled chat model is available; the Host can enable that model and change the default in one catalog commit. I found one P2 CI blocker:
- P2, the exact-head
testcheck fails in Storybook smoke.ConnectionDefaultActionnow opens the chooser whenenabled.length !== 1(apps/desktop/src/renderer/features/connection-settings/connection-default-action.tsx:156-162), but the unchangedModelsDefaultBadgeTypographystory clicks “Set as default” and immediately asserts that the “Default” badge exists (apps/desktop/stories/settings/settings-pages.stories.tsx:2103-2110). Its OpenAI Review fixture does not supply one enabled chat-model choice, so the new dialog appears instead. The hosted run36671017858failed this story again when retried alone withUnable to find an element with the text: 默认; this is deterministic, not the unrelated concurrency-only story retry in the same job. Update the story's fixture or complete the chooser in its play function, then rerun the current-head check.
Node 24 clean npm ci, build:test, and focused Desktop/Host/Storage tests (178/178) pass. git diff --check and static merge-tree against current main 0aa2707b pass. I reviewed the UI selection path, IPC input forwarding, and atomic catalog operation; I did not run native Electron or manually exercise a real provider discovery endpoint. This head is not ready to merge while its CI is red.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
@hqhq1025 Addressed in I also merged main CI for the current head is green, including the complete Storybook smoke, Desktop e2e, Runtime Host tests, geometry checks, and CLI package validation. Ready for another review. Automated response posted by Codex on behalf of @liuxiaocs7. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The default action bypasses legacy model-selection normalization, so older connections may be prompted unnecessarily instead of switching directly to their usable model.
Review effort: Lite
Findings: None
What changed in this PR
This PR guides Desktop users through selecting and enabling a chat model when changing a connection’s default model, with atomic catalog updates and selection-preserving discovery.
Changes:
- Adds atomic enable-and-set-default catalog operations and Runtime Host protocol epoch 200.
- Extends Desktop bridge, IPC, and discovery flows.
- Adds guided dialogs, localization, manual model entry, and coverage.
| File | Summary |
|---|---|
packages/storage/src/runtime-policy/operations.ts |
Extends model-fetch operations. |
packages/storage/src/runtime-policy/coordinator.ts |
Propagates selection-preservation behavior. |
packages/storage/src/runtime-policy/connection-catalog-document.ts |
Adds atomic model enablement and default selection. |
packages/storage/src/runtime-policy-stores.ts |
Updates the operation facade. |
packages/storage/src/__tests__/runtime-policy-stores.test.ts |
Tests atomic updates and conflicts. |
packages/runtime-host/src/server/connection-effect-coordinator.ts |
Preserves selections during discovery. |
packages/runtime-host/src/protocol/runtime-policy.ts |
Documents default-target input. |
packages/runtime-host/src/protocol/index.ts |
Bumps compatibility epoch. |
packages/runtime-host/src/protocol/connection-effects.ts |
Adds discovery-selection options. |
packages/runtime-host/src/__tests__/runtime-policy-coordinator.test.ts |
Tests protocol input compatibility. |
packages/runtime-host/src/__tests__/connection-effects-protocol.test.ts |
Tests discovery protocol validation. |
packages/runtime-host/src/__tests__/connection-effect-coordinator.test.ts |
Tests preserved discovery selections. |
packages/core/src/runtime-policy/connection-catalog-codec.ts |
Decodes enable-and-default input. |
packages/core/src/runtime-policy.ts |
Adds optional enable consent. |
docs/astryx-surface-file-inventory.paths |
Registers the new UI surface. |
docs/astryx-surface-file-inventory.md |
Updates the UI inventory. |
apps/desktop/stories/settings/settings-pages.stories.tsx |
Updates connection stories and assertions. |
apps/desktop/src/renderer/settings/providers-panel.tsx |
Integrates the default action. |
apps/desktop/src/renderer/platform/desktop/create-connection-settings-services.ts |
Threads model-selection options. |
apps/desktop/src/renderer/features/connection-settings/settings-provider-copy.ts |
Adds localized selection copy. |
apps/desktop/src/renderer/features/connection-settings/ports.ts |
Extends renderer bridge contracts. |
apps/desktop/src/renderer/features/connection-settings/index.ts |
Exports the new action. |
apps/desktop/src/renderer/features/connection-settings/connection-default-action.tsx |
Implements guided selection and confirmation UI. |
apps/desktop/src/preload/preload.ts |
Extends preload bridge calls. |
apps/desktop/src/preload/bridge-contract.d.ts |
Updates bridge typings. |
apps/desktop/src/main/runtime-host-connections-ipc-main.ts |
Handles model-aware default changes and discovery. |
apps/desktop/src/main/runtime-host-client.ts |
Extends Runtime Host client operations. |
apps/desktop/src/main/__tests__/runtime-host-connections-ipc-main.test.ts |
Tests IPC propagation and validation. |
apps/desktop/src/main/__tests__/connection-settings-locale-render.test.ts |
Tests renderer flows and localization. |
apps/desktop/renderer-architecture.json |
Updates architecture dependencies. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 8223aeba72e597ac89d409e59107f515d3354019. This revision fixes the Storybook connection fixture by projecting its enabled model IDs before resolving catalog entries (apps/desktop/stories/settings/settings-pages.stories.tsx:162-163), so the connection-detail story can exercise the single-enabled-model default action. The PR adds a guided default-model choice in connection-default-action.tsx:54-58,146-165; the Host validates and commits enablement with the default target in one catalog write (connection-catalog-document.ts:442-489). The merge commit also incorporates main's Conversation Plan/state refactor and reconciles the surface inventory.
I found no new substantiated P0-P3 issue on this head. I checked the model-choice, IPC, catalog write, and discovery paths, including the prior Storybook failure and the existing Copilot overview. The IPC projects Host catalog enabledModelIds into Desktop snapshots (runtime-host-connections-ipc-main.ts:409-430); the overview's suggested legacy-selection bypass does not establish a failing path in this code.
Node 24 clean npm ci and build:test passed; focused Desktop/Host/Storage tests passed (153/153), Storybook TypeScript typecheck passed, and this head's hosted test passed. Diff check and static merge against current main ed38ccbb were clean. I did not run the visual Storybook smoke or a packaged Electron/native OS interaction, so those UI behaviors remain unverified locally. GitHub still requires its normal review gate; this comment is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 7555e004ba325d9c4ec952b3b5f2ee312e064996. This card asked me to check the previously raised issues and the conflict resolution, so I concentrated there, plus the P3 below.
1×P3, no other P0–P3.
P3 — two unrelated protocol ledgers were re-dated by this PR. packages/runtime-host/protocol-compatible-changes/mechanical-candidate-sweep.json and turn-snapshot-optional-fields.json read "epoch": 203 at this head, while both main and this PR's own base (afde01c2) read 200. Those files record the compatibility argument for a particular change, and their epoch is the protocol baseline that argument was made and validated against — it does not track the current epoch. This PR's epoch bump is itself correct (see below), but bumping the other arguments' ledgers to match it asserts a re-validation that did not happen. They should keep the baseline they were argued at.
The epoch bump itself is right. main is at 202, this head is at 203, and the PR does change the protocol (packages/runtime-host/src/protocol/connection-effects.ts), so one increment past main is the correct numbering. Only the two other ledgers are the problem.
The previously raised P2 is fixed, and the head's gate proves it. That finding was that the exact-head test failed in Storybook smoke because ConnectionDefaultAction opens the chooser when enabled.length !== 1 while the unchanged ModelsDefaultBadgeTypography story still clicked "Set as default" and asserted the badge. This head's test check is green, which is precisely the check that was failing, and the fixture fix that made it green is still in place (settings-pages.stories.tsx:164 projects enabledModelIds before resolving catalog entries).
The conflict resolution is faithful. The PR is mergeable now, and the merge that resolved the conflict did touch two of this PR's own files — providers-panel.tsx and the settings story — so I checked both survived: the panel still renders <ConnectionDefaultAction> with its render-prop (providers-panel.tsx:354-384), and the story fixture projection above is intact. The merge combined main's edits with this PR's rather than replacing either.
On the default-action path, since it is the new behaviour: it switches directly when exactly one model is enabled, otherwise opens the chooser, and falls back to the full model list when none are enabled (connection-default-action.tsx:56,157,193). A legacy connection with no enabled model therefore gets the chooser rather than a silent direct switch; that is a UX trade-off within a new affordance rather than a lost capability, so I am not grading it.
Gate on this head: test is green; mergeable is true and the state is blocked, i.e. review pending.
What I could not judge
- Scope, stated honestly: I read the feature's entry points, the Host/storage/protocol file list and the increment since the last review here, not all 32 files line by line.
- No desktop run, so the chooser's behaviour in the real settings surface is taken from code and tests.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
7555e00 to
0a1399f
Compare
|
@Astro-Han Thanks for catching this. The P3 is addressed after rebasing onto main
CI for the current head is green, including Runtime Host tests, Desktop e2e, the full Storybook smoke, geometry checks, and CLI package installation validation. The default-badge story fix remains intact. Local full build, workspace typechecks, lint/format, architecture checks, and 197 focused tests also passed. Ready for another look. Automated response posted by Codex on behalf of @liuxiaocs7. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 0a1399f42daecd7f243a8d877a1b36713c0312cc.
The P3 from my previous pass is resolved, and no new P0–P3 findings.
I asked for two protocol ledgers to be reverted, since they recorded the baseline a specific compatibility argument was made against and this PR had re-dated them (200 → 203) without re-validating anything. At this head packages/runtime-host/protocol-compatible-changes/mechanical-candidate-sweep.json and turn-snapshot-optional-fields.json read "epoch": 200 and are now byte-identical to main, while the epoch constant is 203 against main's 202 — i.e. the PR's own bump is still correctly one past main and only the unrelated ledgers were restored. That is exactly the split I described.
Nothing else changed that bears on the feature: the delta since 7555e004 is the ledger revert plus main's own newer content merged in, and my earlier verification of this PR (the guided default-model choice, the atomic enable-and-set-default catalog write, the restored Storybook fixture, and the faithful conflict resolution) is untouched by it. The earlier P2 from the pass before mine — the stale Storybook fixture that made test red — remains fixed.
Gate on this head: test is green; mergeable is true and the state is blocked, i.e. review pending.
What I could not judge
- The merged
maincontent in the delta (composer send policy, TUI copy catalog and similar) is other people's already-landed work, which I did not re-review line by line here. - No desktop run, so the chooser's behaviour in the real settings surface still rests on code and tests.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: no blocking findings in our automated reviews of this head and CI is green. It now conflicts with main, so please merge main or rebase; we will re-check the conflict resolution before merging.
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
0a1399f to
d31d65a
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review of exact head d31d65a0 (previous review: 0a1399f4).
Rebase delta. Comparing each head against its merge-base with main, both diffs touch the same 30 files (+755/-76) with the same content. There are only two real changes:
docs/astryx-surface-file-inventory.mdtotals rebased onto main's 328-file baseline (now 329 total, 325 aligned). This is consistent with the single newconnection-default-action.tsxrow and its.pathsentry.RUNTIME_HOST_COMPATIBILITY_EPOCHis now 204 -> 205 (previously 202 -> 203), and the comment was renumbered.
The PR does not touch AppShell, features/conversation/controller, or the composer-submission owners moved in the R2 refactors (#5934-#5937, #5952, #5954). All bridge/port/client signature changes add optional trailing parameters, so the callers main added or moved keep their prior behavior. No behavior is lost, no logic is duplicated, and nothing is placed in the wrong layer.
Findings (no P0-P2):
- P3
packages/runtime-host/src/protocol/index.ts:107-108: 205 passesscripts/protocol-epoch-check.mjsagainst current main. However, #5709, #5902 and #5548 also claim 205 (and #4138 claims 206), so whichever lands second will need to re-bump. This is cross-PR coordination only. - P3 (pre-existing, informational)
apps/desktop/src/renderer/platform/desktop/create-overlays-services.ts:65: the command-palette "Set as default" path still callssetDefault(slug, host)without model guidance. R2 only moved it behind the overlays port and its semantics are unchanged. It may be worth a follow-up, but it is not a regression here.
CI: test is still queued/pending on d31d65a0. GitHub reports the PR as mergeable (merge state BLOCKED on the pending required check). Re-approval stands, conditional on green CI.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: the rebase onto current main only changed the surface inventory totals and the protocol epoch (now 205); no blocking findings in our review of this head, and CI is green.
main absorbed apache#5866, which landed epoch 205; apache#4138 holds 206 and apache#3700 holds 207, so this branch moves past both to 208. The call-kind note moves to the 208 slot with its older-peer reference updated to 207, and the two compatible-change declarations re-pin to 208. Generated-by: GLM-5.3-Flash (ZCode)
Summary
Fixes #5865
Setting a custom relay with no enabled chat model as default currently produces a generic error. The action now guides the user through choosing a model:
Production components/styles rendered in Chromium with a synthetic connection and stubbed bridge:
Empty model inventory
Verification
8223aeba7passed, including full Storybook smoke, Desktop e2e, Runtime Host tests, geometry checks, and CLI package validation.d6876d708:npm run build,npm run typecheck, affected Desktop/Host/storage suites (178 passed), renderer architecture and protocol-epoch checks passed.npm testrun was not fully green: the remaining Host implementation-child-patch timeout also reproduced on the unmodified baseline. Python eval tests passed under Python 3.12 (94 run, 13 skipped).Compatibility
Bump Runtime Host compatibility epoch 199 → 200 for atomic enable/default selection and discovery that preserves the enabled-model selection. Desktop and Runtime Host must be upgraded together.
AI use
Tool(s) and scope: Codex assisted with diagnosis, implementation, tests, visual verification, and this submission on behalf of @liuxiaocs7. The implementation commit includes
Generated-by: Codex; retain it when squashing.Checklist
Does this PR entail a change in behavior?