Skip to content

fix(desktop): save model selection as a local draft - #6162

Merged
me2seeks merged 4 commits into
mainfrom
codex/fix-model-selection-draft
Oct 19, 2026
Merged

me2seeks merged 4 commits into
mainfrom
codex/fix-model-selection-draft

Conversation

@me2seeks

@me2seeks me2seeks commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR makes two connected changes to provider model selection:

  1. Edit a local draft and save the final selection explicitly. Rapid model toggles currently save immediately and hold the editor lock through a catalog refresh, so later inputs are ignored. Switches now update a local draft; Save submits the final selection once. A bottom save bar remains visible while scrolling large catalogs, using the existing settings-pane scroll area. Failed writes retain edits. Background refreshes update the persisted baseline while preserving the draft, and a successful write remains acknowledged if its subsequent refresh fails. Connection tests use the saved selection.

  2. Protect unsaved model selections when leaving the editor. Explicit saving introduces a period with unsaved changes. The provider editor registers its leave check with shared settings navigation, so Back, settings-page changes, Host selection and Escape ask before discarding those edits. Escape dismissal stays available while the settings chunk is loading.

These changes belong together: users can make several edits without Host round trips, then save them or explicitly discard them when leaving. Adding a custom model still saves that model immediately without persisting an unrelated selection draft.

Refs #5549

Verification

  • Desktop typecheck, Biome lint and git diff --check pass. Desktop files are excluded from the repository formatter.
  • 37 connection-settings tests cover rapid edits, delayed save/refresh, failed writes, external-refresh baseline changes, connection testing, custom models and leave protection. The two external-refresh regressions fail before their fix.
  • 16 overlay tests and the renderer architecture check pass. The architecture policy suite also passes (168 tests).
  • Desktop Storybook builds successfully. All 30 Providers stories plus the parameter-save flow in Traditional Chinese and English pass (32 browser runs). Save/Cancel queries target the parameter dialog separately from the new save bar.
  • Browser checks cover the 444-model catalog in Chinese at 1280 × 900 and English at 480 × 800, save-bar visibility across scroll positions, ordinary provider detail, and Escape during suspended settings loading.

Screenshots show the production ProvidersPanel and settings frame in Desktop Storybook with a 444-model OpenRouter fixture and sample data.

Chinese, 1280 × 900:

Unsaved selection with a visible save bar in a 444-model catalog

English, 480 × 800:

Narrow provider editor keeps Save visible without horizontal overflow

Windows 11 native validation has not been run. This PR removes per-toggle writes; catalog-revision deduplication and the cause of the reported Windows process crash remain outside this change. Keep #5549 open for its remaining native validation and profiling criteria.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented the draft/save interaction, navigation protection and regression tests, performed local review and validation, and prepared this description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Keep model toggles local until Save, preserve failed drafts, and keep the save bar visible while scrolling. Protect explicit Settings navigation and Escape from discarding pending selections.

Refs #5549

Generated-by: Codex
@me2seeks
me2seeks requested review from Astro-Han and liugddx October 19, 2026 06:30
@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Oct 19, 2026
Compare connection-test results with persisted model choices while an unsaved selection draft is visible. Add a regression that fails when the successful saved model is incorrectly reported as a fallback.

Generated-by: Codex
@me2seeks

Copy link
Copy Markdown
Contributor Author

@sunrioa Thanks for reproducing this in #5549 and sharing the measurements! This PR keeps model toggles in a local draft and saves the selection explicitly, with Save always visible while scrolling.

If you have some time, I'd appreciate a quick look—especially whether it improves the rapid-toggle experience you observed on macOS. No pressure at all if you're busy or can't get to it. Thanks again!

The model selection save bar adds another Save and Cancel to the provider page. Scope the parameter story actions to its dialog so the test exercises the intended form. All 30 provider stories and the parameter flow in Traditional Chinese and English pass locally.

Generated-by: Codex

@liugddx liugddx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed ef4b789d6b04042beb747ccf7d90b07f1142fc92 — REQUEST_CHANGES,剩余一条 P2,建议合并前修复。

本地草稿、显式保存和共享离开确认的设计合理。后台刷新保留过期持久化基线的问题仍然存在,具体复现及修复方向见行内评论。

上一版的 Storybook Save/Cancel 查询歧义已由 ef4b789 修复,本次不再将其列为待修问题。最新 CI 仍在运行。

验证:本地 core 构建通过,并用真实 useConnectionDetail Hook(替换 UI 上下文与 Host bridge)复现了行内问题。本地 UI 构建受旧依赖阻塞,未完成完整套件;相关 controller 文件在新旧提交中的 Git blob 完全一致。

四个核心判断:

  • 最优解:方向合适,持久化基线同步尚未闭环。
  • 职责层次:草稿归 renderer、规范化保存结果归 Host,分层正确。
  • 奥卡姆剃刀:无需新增状态框架;嵌套导航守卫共享同一引用,未形成重复权威。
  • 测试质量:新增测试值得保留;补上“外部刷新后继续编辑并测试连接”的组合回归。

最小正确版本是保留当前设计,修复最新持久化基线的同步,并补一条上述组合回归测试。

Comment thread apps/desktop/src/renderer/settings/use-connection-detail.ts
Keep edited model choices while syncing their persisted baseline from background catalog snapshots. Cover reverting a draft to the old selection after an external update and testing the refreshed Host selection, while retaining acknowledgement after a failed refresh.

Generated-by: Codex
@me2seeks
me2seeks requested a review from liugddx October 19, 2026 07:01

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed 2289ea2b7f1378a0667bc96051a3c682603324cd. I found no remaining P0–P2 defect in the reviewed change.

Model toggles now update a local draft without writing to the Host; Save submits the final selection once. Failed writes retain edits, successful writes remain acknowledged when refresh fails, and adding a custom model does not save an unrelated selection draft. The shared navigation check protects Back, Settings navigation, Host selection and Escape while the selection is unsaved.

The previously reported stale-baseline P2 is fixed in the snapshot reconciliation effect: background refresh updates the saved baseline while preserving edited values. Both “external refresh, then edit back to the old selection” and subsequent connection testing pass. Replacing only this Hook with the implementation before 2289ea2b makes both regressions fail.

Validation passed:

  • Clean full build:test, all four Desktop TypeScript configurations, renderer and Storybook production builds.
  • 63 connection-settings/overlay tests, 168 architecture policy tests, the strict architecture check and lint on changed TypeScript files.
  • 38 provider browser runs, plus six viewport checks with a CJK font available. The 444-model catalog keeps Save visible at the top, middle and end at 1280×900 and 480×800 in Simplified Chinese, Traditional Chinese and English, with one scroll owner and no horizontal overflow. Removing sticky positioning puts the bar below the viewport in all six controls. The parameter dialog save flow also passes separately from the new footer buttons.

No Host protocol files changed; the epoch guard passes at 224. Current CI succeeds and a static merge with fetched main is clean. Native Windows crash behavior and the merged-tree build remain unverified; this review establishes the draft/save interaction, without establishing the cause of the separately reported process crash.

@sunrioa

sunrioa commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Thanks for the fix! I retested 2289ea2b in a local development build on macOS 26.5.2. Rapid changes to multiple model switches are now retained and saved together. This is noticeably better than the previous first-operation-only behavior.

There is still a brief hitch when clicking Save with the full 471-model catalog visible. I reproduced it during six saves, although it feels much less severe than before.

With Codex-assisted read-only profiling:

  • Renderer samples show CSS selector matching, style recalculation and layout work. The main process and Host spent most sampled time waiting.
  • Four standalone catalog reads took approximately 14–18 ms each. These are baseline read timings, not end-to-end Save timings.
  • Saving still changes the busy state of every model switch and parameter button, then restores them after the write and refresh. That full-list update looks worth investigating next, alongside the separate catalog-refresh subscriptions.

I haven’t isolated the exact CSS selector or JavaScript caller responsible for the hitch, so I wouldn’t call this a confirmed root cause yet. The rapid-toggle fix itself works in my testing; this is feedback about the remaining Save-time stutter.

简体中文

感谢修复!我在 macOS 26.5.2 上使用本地开发版重新测试了 2289ea2b。现在连续切换多个模型开关,选择都能保留并一起保存,相比之前只有第一个操作生效,已经有明显改善。

完整显示 471 个模型时,点击“保存”仍会短暂卡一下。我在六次保存中复现了这个现象,不过体感比之前轻得多。

借助 Codex 做了只读性能采样:

  • 渲染进程采样中出现了 CSS 选择器匹配、样式重算和布局开销;主进程与 Host 大部分采样时间在等待。
  • 四次独立目录读取各耗时约 14–18 毫秒。这是基线读取耗时,不是完整保存耗时。
  • 保存时仍会切换所有模型开关和参数按钮的忙碌状态,写入及刷新结束后再恢复。整表状态更新,以及分别订阅目录变化的刷新入口,值得作为下一步排查方向。

目前还没有定位到导致卡顿的具体 CSS 选择器或 JavaScript 调用点,因此暂不把这些线索当作已经确认的根因。快速切换的问题在我的测试中已经修好,这里反馈的是保存时剩余的轻微卡顿。

@liugddx liugddx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — OpenAI Codex, posted on behalf of @liugddx. This is not a substitute for independent human committer review.

APPROVE at 2289ea2b7f1378a0667bc96051a3c682603324cd.

The prior P2 is fixed: dirty local drafts retain their edited value while the saved baseline updates from the latest Host snapshot. The added regressions cover an external refresh, reverting the draft to the old selection while remaining dirty, explicit save, and testing against the current persisted selection.

I found no remaining concrete P1/P2/P3 issue in the current head. The hosted test check passes. The older CHANGES_REQUESTED state is from the superseded review; this approval is for the exact current head.

@me2seeks
me2seeks merged commit 6c09ba7 into main Oct 19, 2026
1 check passed
@me2seeks
me2seeks deleted the codex/fix-model-selection-draft branch October 19, 2026 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants