refactor(desktop): extract Module Hub feature slice - #3649
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
独立完整复核当前 exact head 0764fd56(45 文件,+5178/-1714)。Module Hub 的 services adapter、controller/lifecycle ownership、AppShell intent boundary、default Runtime Host generation fences、Daily Review composer claim、Scheduled Tasks subscriptions/mutations、Skills projections,以及有意保留的 MCP leaf exception 均核对通过;未发现可复现 P0–P3 code finding。Module Hub emitted suites 30/30、changed-file Biome、git diff --check 通过。当前 hosted test 失败在 apps/desktop/src/renderer/features/module-hub/README.md 缺 ASF license header,未发现与运行时行为相关的失败;当前 PR 的 merge state 暂为 UNKNOWN,未在此状态下作 approval。
|
Gate status note, so this is not mistaken for a logic problem: this PR had no checks at all on head That is mechanical: the new files this refactor adds are missing their Apache license headers. Note that the PR is also currently conflicting with |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 0764fd56733c2c223996a52d9a8e919d04b24345. NO-GO. One [P2] inline, plus two gate blockers below.
Gate
Hosted test is completed/failure on this exact head (run 32658374788). It failed at the first step, Check ASF source headers, and the whole run lasted 21 seconds — apps/desktop/src/renderer/features/module-hub/README.md is missing the ASF licence header. The consequence matters more than the cause: the workflow exited at the header step, so not a single test actually executed on this head. A green-looking history elsewhere on this PR does not cover this commit.
mergeable is CONFLICTING. The branch needs a rebase onto current main before anything else here can be settled.
What was checked and cleared
This was an adversarial second pass over a large refactor, so the negative results are part of the report. Route mounting and the four-leaf pages were compared branch by branch against the old conditional chain (ModuleHubHost resolves identically; both return null for an unknown route), and the agents-view / workbench toolbar suppression set (skills, cron, daily-review) is unchanged. Keep-awake is a faithful port with a strengthened generation guard and an equivalent supported probe. The scheduled-tasks surface-active toast gate on refresh and mutation matches the old action, with a new generation guard against stale overwrites. Daily Review's copy/save in-page feedback gates are equivalent; the command-palette pasteToday keeps its toast after the claim-mechanism change (showIfMounted is not page-scoped); and neither version toasts on a cancelled save. Of the three packages/ui changes, the scheduled-task-panel resync effect gaining its pending dependency is a bug fix rather than a loosening, and the other two are readonly type changes only. use-composer-mentions switching from skills-array identity to skillCatalogRevision is trigger-equivalent, since the revision increments on every successful skills list.
Local build of apps/desktop main passed, and node --test across all __tests__ gave 1368 tests / 1360 passed / 0 failed / 8 cancelled, the cancellations all confined to mcp-oauth-controller.test.js and untouched by this PR. E2E and Storybook were not run locally for want of a display; those are the hosted job's responsibility, and the hosted job did not reach them.
To close this out
Add the ASF header to the new README, rebase to clear the conflict, and restore the capability gate flagged inline. Then the hosted run gets past its first step and can say something about the rest of the change.
0764fd5 to
2454556
Compare
|
Addressed the review blockers and pushed rebased exact head
Fresh exact-head verification: lint, format, ASF headers, full build, typecheck, Desktop 1363/1363, Desktop/UI Knip, Astryx 208/208, Storybook 178/178, related Playwright 4/4, and I also re-reviewed the final upstream-to-head diff after the fixes. The capability projection now matches the baseline, the #3619 wiring remains intact, Module Hub has no Desktop global or reverse AppShell dependency, and I found no additional actionable P0-P3 issue. This self-review is not a substitute for your independent re-review. |
2454556 to
a9be3ce
Compare
|
Rebased again onto current The only generated-file conflict was resolved by regenerating the Astryx inventory. During the overlap review I also removed two obsolete Session-setting queue refs carried by the previous AppShell conflict resolution, so this branch now leaves #3619's Local exact-head evidence: lint, format, build, typecheck, Desktop 1409/1409, both Knip workspaces, Astryx inventory, Storybook 194/194, related Playwright 4/4, Hosted CI currently stops at ASF headers because base |
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
a9be3ce to
27e5771
Compare
|
The failed exact-head CI has been cleared. I rebased onto current Exact-head CI run 32725579836 is now green. It passed ASF headers, lint/format, inventory, build/typecheck, both Knip gates, affected workspace tests, Desktop E2E, Browser smoke, alignment audit, and Storybook build/smoke. Local exact-head verification is also green, including Desktop 1409/1409 and related Playwright 4/4. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving on 27e57719a3b0d729e44f41a98ae08570fa2089ab, with test terminal green on that exact head and the branch now mergeable.
Both blockers from my earlier round are closed, and I re-derived each from the code rather than taking the fix note at its word.
The capability gate is restored, and better than I asked for. use-skills-controller.ts:793 now spreads onOpenSkill, onOpenSkillsFolder and onImportManagedSkillSource together, conditioned on the single input.openSkillsFolder token, which app-shell.tsx:1472 supplies only when projectCapabilities.viewClientPath is true. Rather than re-adding the one lost condition, the three handlers now share one capability token — the group can no longer drift apart the way it did here.
The gate that had never actually executed is now green. The earlier run died at the ASF header step in 21 seconds, so no test on that head had run at all; this head has a real result.
On the follow-up commits. The six fixes added after my review are all fencing of asynchronous work, so I applied one criterion throughout: for each new guard, which side of the last await does it sit on? Every one of them re-checks after the final await rather than only before it — shouldReportOperationFailure re-verifies mounted, then whether the failure is still current, then mounted again; the Scheduled Tasks and Skills paths re-verify generation, host and surface-active after their last await. The new tests construct the out-of-order case with deferred promises instead of asserting a single call, so they pin the behaviour rather than the implementation. The five removed exports from features/module-hub/index.ts have no remaining consumers, and the new boundary test pins entry through index/testing only.
On merge order, not a blocker: two sibling slice extractions are open against the same app-shell.tsx. Whichever lands first forces the others to redo path-level work, so they should go in one at a time with a rebase between.
简体中文
已在 27e57719a3b0d729e44f41a98ae08570fa2089ab 上 approve,该 exact head 的 test 为终态绿,分支现在也可合并。
我上一轮的两个阻塞项都已闭合,而且每一条我都是从代码重新推导的,没有直接采信修复说明。
能力门已恢复,而且比我要求的更好。 use-skills-controller.ts:793 现在把 onOpenSkill、onOpenSkillsFolder、onImportManagedSkillSource 一起展开,条件是同一个 input.openSkillsFolder 令牌;而 app-shell.tsx:1472 只在 projectCapabilities.viewClientPath 为真时才提供它。作者没有只把丢掉的那一行加回来,而是让三个 handler 共用一个能力令牌——这一组从此不会再像这次一样各自漂移。
那个从未真正执行过的门禁现在绿了。 此前那次运行在 21 秒时死在 ASF header 步骤,也就是说那个 head 上一个测试都没跑过;这个 head 有了真实结果。
关于后续提交。 我上轮 review 之后新增的六个修复全部是对异步工作的围栏,所以我用了同一条判据贯穿:每一处新守卫,落在最后一个 await 的哪一侧?全部都是在最后一个 await 之后重新检查,而不是只在之前查一次——shouldReportOperationFailure 先验 mounted、再验失败是否仍然当前、然后再验一次 mounted;Scheduled Tasks 与 Skills 路径也都在最后一个 await 之后重新校验 generation、host 与 surface-active。新增测试用 deferred promise 构造乱序,而不是只调用一次就断言,因此锁住的是行为而不是实现。features/module-hub/index.ts 净删的五个导出没有遗留消费方,新增的边界测试把"只能经 index/testing 进入"锁住。
关于合并顺序,不构成阻塞:另有两个同类切片 PR 正对着同一个 app-shell.tsx 开着。谁先合都会迫使其余重做路径级调整,因此应当串行合并、中间让作者 rebase。
|
LGTM. Merging at @astrohan's request — Note for the two sibling slice extractions: this one has landed, so please rebase before the next one goes in. 简体中文LGTM,按 @astrohan 的要求合并—— 给另外两个同系列切片 PR 的提醒:这个已经合入,请在下一个合并前先 rebase。 |
Summary
Refs #3439
Verification
npm run lint— passed (2700 files)npm run format:check— passed (1624 files)npm run check:asf-headers— passed (2835 covered, 138 reviewed exclusions)npm run build— passednpm run typecheck— passednpm --workspace @maka/desktop test— 1409 passed, 0 failed/cancellednpx knip --workspace apps/desktop— passednpx knip --workspace packages/ui— passednpm run astryx:surface-inventory:write— 209 files, 208 aligned, 0 blocker, 1 pre-existing upstream polishnpm run astryx:surface-inventory— passedgit diff --check— passedgit merge-tree --write-tree upstream/main HEAD— clean against1053c1926Review focus
The adversarial AI red/blue pass reproduced and fixed keep-awake UI resync, late Skills/Daily Review feedback, superseded projection errors, stale Runtime Host mutation feedback, and destructive confirmation after controller disposal. Human review then found a missing client-path capability gate plus the ASF header and rebase blockers; all Module Hub findings remain fixed on exact head
27e57719a.The latest rebase onto
1053c1926includes upstream's ASF header repair and storage entrypoint refactor. Its 11 commits are range-diff equivalent to the previous exact head and have no file overlap with those two upstream commits. I rechecked the final diff, regenerated Astryx inventory, and found no additional actionable P0-P3 issue. This is supporting evidence only and does not count as independent human approval; human re-review remains pending.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex implemented the feature slice, authored tests and documentation, ran local verification, and performed the disclosed red/blue adversarial review. Affected commits retain
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?