refactor(desktop): move Settings → Usage into a renderer feature slice - #4440
Conversation
6ecd161 to
8623f82
Compare
cfc12d9 to
32a0847
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I had a review ready for cfc12d9b and you force-pushed 32a08471 before I posted it, so this is against the new head. Build, the usage test file, all four typecheck projects, the architecture check, Biome, format and the surface-inventory regeneration are green locally.
The move itself is still faithful: the UI files match the old page line for line, class names and usage.css are untouched, the DOM is the same, so no before/after image is needed. The ratchet reason is real. But the new head undoes three things the body still claims, and the body was not updated, so right now a reviewer reading the description reviews a design that is no longer in the diff.
Behaviour vs main, two regressions.
- Leaving Usage and coming back no longer redisplays the snapshot. On
mainthe stats live at the top ofSettingsSurfaceContentand survive a section change; here they live inusage-settings-view.tsx:64, and the view is rendered only insidecase 'usage'atsettings-surface.tsx:1099, so a section switch unmounts it and the user comes back to four dashes until the reload lands. The body's "Review follow-up: fixed a section round-trip snapshot loss" describes the previous head, not this one. - The two synchronous fences from
mainare gone with no replacement.commitSelectedRuntimeHostProfileat:391no longer bumps the ticket and clears the snapshot, and the generation-change handler at:705no longer clears Usage alongside the other three Host-scoped resources, the casemain's comment there says must be fenced before the catalog refresh resolves. Clearing now waits forselectedRuntimeHostEpochto change, which waits for the asyncreloadRuntimeHostsin the same handler; in that window Usage shows the previous generation's numbers and refetches with the old host.providerKeyas a ReactkeyonUsageServicesProvider(usage-settings-page.tsx:56) covers the remount, not the window.selectedRuntimeHostKeyRefandselectedRuntimeHostEpochRefare now written and never read.
Range switching is still right: the tagged snapshot at view:109 is the same guard main had.
Tests. The body says seven tests covering round-trip retention, Host-generation invalidation, late-load fencing and the synchronous fence. The file has one (usage-settings-view.test.ts:95), and it covers range scoping, the one obligation that did not change. The three lifecycle obligations that did change have no test, and nothing runs through settings-surface, which is the seam this PR actually moves.
Still to shrink, unchanged from the previous head.
features/usage/copy.tsduplicateslocales/settings-usage-copy.ts. The README's reason (a shared import would be closure debt) is not what the checker does:isValidatedCopyCatalogexempts feature imports of a validated catalog,copyCatalogFailurereturns undefined for that file, and workbar, goals, task-entry and session-navigation already importlocales/*from feature code. Import it in the view and drop the file and the prop. With that,usage-settings-page.tsxcan go too andsettings-surfaceimports the feature directly;allowsMigrationDependencyaccepts a legacy edge swapped for a feature public path at equal count, and the closure loses one more file.controller/optimistic-settings-draft.tsandcontroller/action-guard.tsare copies ofsettings/optimistic-settings-draft-controller.tsandoauth-login-flow-guard.ts+use-action-guard.ts; only the originals have tests, and the originals keep nine consumers. Put the pure cores undersrc/shared/(the checker treats it as external; the view already importsshared/runtime-host-identity) with a React shell on each side, or say in the body that the copies sit outside the existing tests.- New on this head:
settings/settings-metric-card.tsxis back with zero consumers whilefeatures/usage/ui/metric-card.tsxis its copy; knip reports it as an unused file anduseUsageServicesas an unused export. The previous head had this right.ports.ts:28also cites aplatform/desktop/create-usage-services.tsthat does not exist and that the README says was deliberately not added.
So the ask: decide which design this PR ships. If it is this one, restore the two fences (a fenceTarget-style handle or the ticket bump at both Host sites), put the snapshot somewhere that survives a section change, rewrite the body to match, and add the tests the body already promises. If it is the previous one, the description and the deletions on cfc12d9b were closer. Either way the copy and controller duplication above should go in the same PR.
Evidence boundary: static read of 32a08471 against main 72eb982d; desktop main build, the usage test file, typecheck, architecture check, knip, Biome, format and inventory regeneration run locally; Storybook and Playwright not run.
AI-assisted review: drafted with Maka; I verified the snapshot location, the two removed fences against main, the test count and the copy-catalog exemption myself.
简体中文
评审写好时你 force-push 了新 head,这是对 32a08471 的。搬家本身仍忠实,DOM 不变。但新 head 撤销了正文仍在声称的三件事,正文没更新。相对 main 两处回归:离开 Usage 再回来快照不再重显(state 搬进了只在 case 'usage' 里渲染的 view);Host 切换和 generation 变更时的两处同步 fence 被删且无替代,那段窗口里显示旧一代的数字并用旧 host 重取。测试只剩一条,覆盖的恰是唯一没变的义务。仍要收的:copy.ts 重复权威(checker 有豁免,四个 feature 已直接 import locales);两个 controller 复制品无测试;新 head 又恢复了零消费者的 settings-metric-card.tsx,knip 报 unused。请先定这个 PR 发哪个设计,再改正文补测试;重复项同 PR 清掉。
cfc12d9 to
7d8d678
Compare
|
Thanks for the careful review, and sorry for the churn — your review landed on The three things the body claimed but
|
|
Hi, @chihumyum, thanks for the reminder! |
f021d84 to
bc17e1c
Compare
Extract the Usage settings surface out of the frozen legacy renderer zone into a feature slice `src/renderer/features/usage/` (ports + services-context + ui), so net-new Usage work (the apache#2015/apache#4164 pricing tab) is no longer blocked by the tooltip and the read-only pricing tab). - `features/usage/` owns the surface: narrow `UsageServices` ports (`loadUsageStats`, `updateUsageSettings` — `UsageSettings` in and out, never the whole `AppSettings`); a persistent `UsageFeatureScope` holding the single tagged `{ range, value }` snapshot + reload ticket + Host-generation invalidation + load-failure toast; and a disposable `UsageSettingsView`. - `settings/usage-settings-page.tsx` is a thin legacy shim (`UsageScopeMount` + `UsageSettingsPage`). `settings-surface.tsx` mounts the scope ABOVE its loading/error gate and passes a `host:epoch` `targetKey` as a prop; the scope clears and fences in place on a target change (no React `key`). The Host-change handler also calls the scope's imperative `fenceTarget()` synchronously, so an in-flight old-Host load cannot land in the window before the re-render. - Regenerate renderer-architecture.json and the astryx surface inventory. Extraction-only; the Desktop adapter/composition wiring and the editable pricing tab remain follow-ups. Refs apache#4425. Generated-by: Claude Code
bc17e1c to
972ba8c
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Approving 972ba8c9. Both regressions from last round are closed at the right places.
The snapshot now survives a section change: UsageScopeMount sits in LayoutContent above the loading gate, so switching sections unmounts only the view. Both synchronous fences are back, fenceTarget() at settings-surface.tsx:410 in commitSelectedRuntimeHostProfile and at :730 alongside the other three Host-scoped resources before reloadRuntimeHosts, and the two write-only refs are gone. copy.ts is deleted and the view imports the locale catalog directly, which the architecture check accepts. settings-metric-card.tsx moved rather than being copied, useUsageServices is no longer exported from the barrel, and ports.ts no longer cites a file that does not exist. knip is clean against main.
I compared the moved UI against the old page tag by tag and class by class. The only differences are the two wrappers that went to the shim and the type narrowing; no CSS changed and the DOM is identical, so this needs no before/after images.
Typecheck across all four projects, the architecture check against origin/main, 7/7 usage tests, format, lint and the surface-inventory regeneration all pass locally.
Two things before merge, then four small ones inline.
The body still describes copy.ts. "What changed" lists it and the first of the "two deviations" is a paragraph about it, while features/usage/README.md says one deviation and states that copy is not one. The squash message comes from the body, so this ends up in the permanent history.
Nothing tests the seam this PR moves. The seven tests are real and they pass, but usage-settings-view.test.ts:126 reconstructs the mount shape by hand and never loads settings-surface. The three lifecycle obligations are exactly what broke on 32a08471, and they broke with every test green. Move UsageScopeMount below the gate today, or delete the fenceTarget() call at :730, and all seven still pass. I am fine with deferring this, but say so in the README's Follow-up as well as the body; right now only the body mentions it.
The two controller copies stay deferred as we discussed. They are about 212 lines and the main source of the net addition, so keeping that note in the body matters.
Evidence boundary: static read of 972ba8c9 against main 8ea3c4f0; typecheck, architecture check, usage tests, format, lint, inventory and knip run locally; no Storybook or Playwright.
AI-assisted review: drafted with Maka; I verified the fence sites, the mount position, the tag-level UI comparison and the knip result myself.
简体中文
批准合并。上轮两条回归都在正确的位置关掉了。
快照现在能扛住断面切换:UsageScopeMount 挂在 LayoutContent 里、loading gate 之上,切断面只卸载 view。两道同步栅栏都回来了,settings-surface.tsx:410 在 commitSelectedRuntimeHostProfile 里、:730 与另外三个 Host 资源并列且在 reloadRuntimeHosts 之前,两个只写不读的 ref 也删了。copy.ts 删掉,view 直接 import locale 目录,架构检查放行。settings-metric-card.tsx 是 rename 不是复制,useUsageServices 不再从 barrel 导出,ports.ts 也不再提那个不存在的文件。knip 对着 main 干净。
我把搬过去的 UI 和原页面逐个标签、逐个 class 比对过,差异只有挪到 shim 的两个外层容器和类型收窄,CSS 一行没动,DOM 完全一致,所以不需要前后对比图。
本地四个 project 的 typecheck、对 origin/main 的架构检查、7/7 usage 测试、format、lint 和 inventory 重新生成全部通过。
合并前两件事,另有四条小的在行内。
正文还在描述 copy.ts。 "What changed" 列着它,"two deviations" 的第一条整段在讲它,而 features/usage/README.md 写的是 one deviation,并且明说 copy 不算 deviation。squash message 从正文取材,这段会留进永久历史。
没有任何测试穿过这次搬家真正动到的接缝。 七条测试是真的、也确实通过,但 usage-settings-view.test.ts:126 是手工复刻挂载形状,从不加载 settings-surface。那三条生命周期义务恰恰就是 32a08471 上坏掉的,而且坏的时候测试全绿。今天把 UsageScopeMount 挪到 gate 之下,或者删掉 :730 那行 fenceTarget(),七条依然全过。延后我没意见,但请在 README 的 Follow-up 里也写上,现在只有正文提到。
两个 controller 副本按我们说好的延后。它们大约 212 行,是净增行数的主要来源,正文里那段说明要留着。
…ey, README Follow-up to review on the Usage feature extraction (apache#4425): - `usage-settings-page.tsx`: `UsageScopeMount` was a pass-through `forwardRef` restating the scope's prop contract; replace it with a direct `export { UsageFeatureScope as UsageScopeMount }` re-export (the scope's props already are the mount contract). Drops the wrapper (~24 lines) and its `forwardRef`/`ReactNode`/`UsageServices` imports; the shim's total dependency count falls 7 -> 6. Corrects the stale doc comment (the page binds only `describeError`; copy is a direct validated-catalog import in the view). - `settings-surface.tsx`: build `usageTargetKey` from `selectedRuntimeHostKey` (`runtimeHostSettingsKey` = `profileId:hostId`) + epoch, so the key shape has one authority instead of re-deriving `profileId:hostId` inline. Behavior identical. - README `Follow-up`: record the deferred `SettingsSurface` integration test for the mount seam (was noted only in the PR body). - Regenerate `renderer-architecture.json`. check-renderer-architecture --base origin/main, astryx surface-inventory, typecheck (renderer + main), Biome, knip (apps/desktop) and 7/7 usage-settings-view.test.ts pass locally. Generated-by: Claude Code
|
Thanks for the careful pass — addressed on top of Both "before merge" items
Inline
Controller de-duplication stays deferred (noted in the body + README). Verified locally against 简体中文已在
controller 去重仍按约定延后(正文 + README 已注明)。本地对着 |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-approving on fb6f289f, since my previous approval was anchored to 972ba8c9. Both pre-merge items are closed and two of the inline notes with them.
The body now says one deviation and states that copy is not one, so it agrees with the README. The README's Follow-up carries the missing surface test, and it explains the gap better than my comment did. usageTargetKey builds on selectedRuntimeHostKey, so runtimeHostSettingsKey stays the only definition of that shape, and the shim collapsed to a re-export.
One note left over, not blocking: usageServices and describeError are still rebuilt every render, and the line suppressing the effect's dependency warning is an eslint-disable in a Biome-only repo, so it suppresses nothing. Whoever adds reload to those deps gets an infinite refetch. Worth a useMemo whenever you next touch the file.
Merging once CI is green on this head.
简体中文
在 fb6f289f 上重新批准,上一个绿勾锚在 972ba8c9。合并前那两件都关了,顺带带走两条行内。正文改成 one deviation 并说明 copy 不算,和 README 一致;README 的 Follow-up 把缺的 surface 测试写进去了,说得比我的评论还清楚;usageTargetKey 基于 selectedRuntimeHostKey,runtimeHostSettingsKey 仍是那个形状的唯一定义;shim 也塌成了 re-export。
剩一条不阻塞的:usageServices 和 describeError 每次 render 还是重建,而那行压制依赖告警的注释是 eslint-disable,这个仓库只有 Biome,等于没写。谁把 reload 加进依赖就会无限重取。下次动这个文件时顺手 useMemo 一下。
CI 在这个 head 上绿了就合。
apache#4440) The renderer-architecture ratchet froze the legacy AppShell closure: no new file may enter it and no legacy file's dependency count may grow. Settings → Usage lives inside that closure, so net-new Usage work (most immediately the editable pricing tab of apache#2015 / apache#4164, which has to live in the Usage tabs) had nowhere to go. Usage moves to `src/renderer/features/usage/`, following the apache#3439 reference boundary. `ports.ts` narrows the port to `UsageSettings` / `UsageStats` instead of the whole `AppSettings`. `services-context.tsx` owns the state: one tagged `{ range, value }` snapshot, the reload ticket, unmount isolation, Host-generation invalidation, the failure toast, and an imperative `fenceTarget()`. `ui/usage-settings-view.tsx` is a disposable view that reads the snapshot and reloads on `[range, targetKey]`. `settings-surface.tsx` drops the `usageStats` state, `reloadUsage` and the prop threading; it mounts the scope above the loading gate so the snapshot survives a section change, and calls `fenceTarget()` synchronously on a Host change alongside the other Host-scoped resources. Behavior is unchanged. The DOM was compared tag by tag against the old page; the only differences are the two wrappers that moved into the shim and a type narrowing. No CSS, IPC or copy changed. One deviation from the composition-feature pattern, forced by the ratchet: there is no `platform/desktop` adapter and no composition wiring, because a frozen closure file cannot import the feature, and usage stats are scoped to the settings-selected Runtime Host, which the app-global composition root does not know about. `settings-surface` assembles the services and mounts the feature through a thin shim. When apache#4425's composition step lands, only that seam moves; the scope stays feature-owned. Two follow-ups are recorded in the feature README rather than done here: a `SettingsSurface` integration test covering the three lifecycle obligations (the current tests reconstruct the mount shape by hand and never load `settings-surface`), and de-duplicating the feature-local `action-guard` and `optimistic-settings-draft` copies, which are about 212 lines and the main source of the net addition. Refs apache#4425, apache#4582, apache#4088 Generated-by: Claude Code Generated-by: GLM-5.3-Flash (ZCode)
Summary
Refactor: move
Settings → Usageout of the frozen legacy renderer zone into a feature slicesrc/renderer/features/usage/, following the #3439 reference boundary (ports+services-context+ui). No product, visual, IPC, or copy changes — the Usage tabs (Activity / Providers / Models / Tools), the request-target tooltip, and the current read-only Pricing tab behave exactly as onmain, including snapshot retention across section switches and Host-change fencing.Refs #4425, #4582 (extraction only — the editable pricing editor and the composition adapter stay follow-ups, so this does not close the full #4425 scope) · Foundation: #4088 (R1 of #3439, whose remaining work now continues in #4582) · Unblocks #2015 / #4164.
Why
Since the renderer-architecture ratchet landed (#4088, R1 of #3439), the legacy AppShell closure is frozen — no new file may enter it and no legacy file's dependency count may grow. This blocks net-new Usage functionality (most immediately the editable pricing tab from #2015 / #4164, which #2015 requires to live inside the Usage tabs). Moving the surface into a feature (exempt from closure debt) unblocks it and shrinks legacy debt.
What changed
features/usage/:ports.ts—UsageServices, narrow inward-facing:loadUsageStats(range)andupdateUsageSettings(patch)take/return onlyUsageSettings/UsageStats, never the wholeAppSettings.services-context.tsx—UsageFeatureScope, the persistent state owner: a single tagged{ range, value }snapshot, the reload ticket, unmount isolation, Host-generation invalidation, and the load-failure toast; plususeUsageServices()anduseUsageStats(range). It also exposes an imperativefenceTarget()handle.ui/usage-settings-view.tsx— the surface (overview + tabs + panels), now a disposable view: it reads the snapshot from the scope viauseUsageStatsand reloads on[range, targetKey].ui/{usage-stats-table,metric-card}.tsx,controller/{action-guard,optimistic-settings-draft}.ts,index.ts,README.md.settings/usage-settings-page.tsx→ thin legacy shim, split by mount level:UsageScopeMount(a re-export ofUsageFeatureScope) andUsageSettingsPage(the disposable view). It binds onlydescribeError(copy is a direct import of the validated locale catalog by the view) and touches nowindow.maka.settings/settings-surface.tsx: removed theusageStatsstate,reloadUsage, its reload effect, and theusageStats/onReloadUsage/runtimeHostEpochprop threading. It now builds a host-boundloadUsageStats(samewindow.maka.settings.usageStatsbridge path) plus anupdateUsageSettingsthat projects the result toUsageSettings, mountsUsageScopeMountabove the loading/error gate (so the snapshot survives a Skeleton/Banner state or a section change) with ahost:epochtargetKeyprop, renders the view in the section content slot, and calls the scope'sfenceTarget()synchronously on a Host change (alongside the other Host-scoped resources). Its hook/state debt decreased (useEffect/useRef/useState); its dependency set is unchanged — the dependency-count reduction lands on theusage-settings-page.tsxshim (7 → 6).renderer-architecture.json+docs/astryx-surface-file-inventory.{md,paths}.Behavior parity with main
The lifecycle matches
main's original surface:fenceTarget()), before the re-render — so an old-Host load can't land.targetKeyis a prop (not a Reactkey), so a Host change resets the scope in place without remounting the rest of Settings.One deviation from the composition-feature pattern (forced by the ratchet)
No
platform/desktopadapter / no composition wiring — a transitional seam.settings-surface(a frozen closure file) can't import the feature orplatform/, and usage stats are scoped to the settings-selected Runtime Host (a settings concept the app-global composition root does not have). So it assemblesUsageServicesand mounts the feature'sUsageScopeMount+ view via the thin shim; theports+services-contextstructure matches the pattern, only the mounting seam adapts to the frozen boundary. When #4425's composition step lands, only this seam moves tocomposition/desktop-feature-services.tsx+ a statelessplatform/desktopadapter — the scope stays feature-owned.(Copy is not a deviation: the view imports the validated locale catalog
locales/settings-usage-copy.tsdirectly, which the ratchet exempts viaisValidatedCopyCatalog— the same way workbar / goals / task-entry / session-navigation import theirlocales/*. Only the legacydescribeErrorhelper is injected by the shim.)Verification
Against latest
main:check-renderer-architecture --base origin/mainpassed;npm run typecheck(4 desktop projects + all@maka/*) 0 errors;check:asf-headers,astryx:surface-inventory,biome lint/formatclean; Knip (apps/desktop+packages/ui) clean; the feature'susage-settings-view.test.ts(7 tests) passes — covering round-trip snapshot retention, range-scoping, Host-generation invalidation, load-completes-while-unmounted, late-load fencing, loading-gate survival, and synchronous fence-at-Host-event. Recommend a quick in-app smoke of the Usage tabs + a Host switch during review.Review follow-up (addressed in this PR)
UsageFeatureScope).UsageServicesport toUsageSettings.fenceTarget()), matching main.Generated-by: Claude Codetrailer.SettingsSurfaceintegration test, and de-duplicating the feature-localaction-guard/optimistic-settings-draftcopies.AI use
Tool(s) and scope: Claude Code (Claude Opus 4.8) — the dependency analysis, the extraction, and verification. Reviewed and submitted by the human contributor of record.
Checklist
usage-settings-view.test.ts; the rest is a behavior-preserving refactor)Does this PR entail a change in behavior?
main