Skip to content

refactor(desktop): extract Module Hub feature slice - #3649

Merged
Astro-Han merged 11 commits into
apache:mainfrom
chihumyum:refactor/module-hub-feature-slice
Aug 24, 2026
Merged

refactor(desktop): extract Module Hub feature slice#3649
Astro-Han merged 11 commits into
apache:mainfrom
chihumyum:refactor/module-hub-feature-slice

Conversation

@chihumyum

@chihumyum chihumyum commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Extract Module Hub from AppShell into a vertical feature slice with a public controller/host, service ports, a Desktop adapter, and testing fakes.
  • Move Skills, Scheduled Tasks, keep-awake, and the Shell-owned Daily Review actions/bridge into the slice. AppShell now supplies navigation and cross-feature intents, renders ModuleHubHost, and consumes only the Scheduled Tasks and Skills revision selectors needed elsewhere.
  • Preserve UI, IPC, storage, copy, shortcuts, navigation, page mount semantics, Runtime Host fencing, first-frame refresh, subscriptions, and mutation feedback behavior.
  • Keep MCP as an explicit page-owned direct-bridge exception; ModuleHubHost only composes that existing leaf.

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 — passed
  • npm run typecheck — passed
  • npm --workspace @maka/desktop test — 1409 passed, 0 failed/cancelled
  • npx knip --workspace apps/desktop — passed
  • npx knip --workspace packages/ui — passed
  • npm run astryx:surface-inventory:write — 209 files, 208 aligned, 0 blocker, 1 pre-existing upstream polish
  • npm run astryx:surface-inventory — passed
  • Desktop Storybook build plus render smoke — 194 stories passed
  • Related Playwright coverage — 4 passed: Module Hub routing/create-once, composer skill invocation, and skill draft lifecycle
  • The CLI clean-EOF backoff case that flaked on current main CI — passed in focused local rerun
  • git diff --check — passed
  • git merge-tree --write-tree upstream/main HEAD — clean against 1053c1926
  • GitHub CI — exact-head run passed in 10m27s, including ASF headers, affected workspace tests, Desktop E2E, Browser smoke, alignment audit, and Storybook smoke

Review 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 1053c1926 includes 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:

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

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: Codex trailers.

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

@chihumyum
chihumyum marked this pull request as ready for review August 23, 2026 18:37

@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.

独立完整复核当前 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。

@Astro-Han

Copy link
Copy Markdown
Contributor

Gate status note, so this is not mistaken for a logic problem: this PR had no checks at all on head 0764fd56 because its workflow run was parked at action_required, which is where fork PR runs sit until someone releases them. I released it, and test has now come back terminal failure — the failing step is Check ASF source headers.

That is mechanical: the new files this refactor adds are missing their Apache license headers. npm run write:asf-headers should fix it.

Note that the PR is also currently conflicting with main, so a rebase is needed regardless. A code review is being prepared separately and will be posted on its own.

@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.

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.

Comment thread apps/desktop/src/renderer/features/module-hub/controller/use-skills-controller.ts Outdated
@chihumyum
chihumyum force-pushed the refactor/module-hub-feature-slice branch from 0764fd5 to 2454556 Compare August 24, 2026 02:47
@chihumyum

Copy link
Copy Markdown
Contributor Author

Addressed the review blockers and pushed rebased exact head 24545560eaef5205bfb5091f8562e20ca4ae4165.

  • Restored the viewClientPath behavior gate for local Skill-source import and added false/true capability coverage.
  • Added the ASF header to the Module Hub README using npm run write:asf-headers; npm run check:asf-headers now passes.
  • Rebased onto current upstream/main (8e7fa68dc), including refactor: unify session catalog activity authority #3619 and the subsequent runtime fix. The only AppShell conflict preserved the new Session setting intent/queue authority while removing the migrated Scheduled Task nonce. Final merge-tree is clean.
  • Removed three unused production-entry type exports found by current Knip.

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 git diff --check all pass.

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.

@chihumyum
chihumyum force-pushed the refactor/module-hub-feature-slice branch from 2454556 to a9be3ce Compare August 24, 2026 11:38
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased again onto current upstream/main (8801edc32) and force-pushed exact head a9be3ce70 with an explicit lease. GitHub now reports the PR mergeable and the merge-tree is clean.

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 useSessionSettingIntent authority intact. The latest slash-picker stabilization in use-composer-mentions is preserved alongside Module Hub's skill-catalog revision invalidation.

Local exact-head evidence: lint, format, build, typecheck, Desktop 1409/1409, both Knip workspaces, Astryx inventory, Storybook 194/194, related Playwright 4/4, git diff --check, and clean merge-tree.

Hosted CI currently stops at ASF headers because base 8801edc32 already contains five unlicensed plugin-runtime files: base run and this exact-head run fail at the same step. The Module Hub diff does not touch those files, so I left that upstream repair out of this PR.

@chihumyum
chihumyum force-pushed the refactor/module-hub-feature-slice branch from a9be3ce to 27e5771 Compare August 24, 2026 12:09
@chihumyum

Copy link
Copy Markdown
Contributor Author

The failed exact-head CI has been cleared.

I rebased onto current upstream/main (1053c1926), which includes #3708's ASF header repair, and force-pushed exact head 27e57719a with an explicit lease. The 11 Module Hub commits are range-diff equivalent to the previous head and have no file overlap with the two new upstream commits.

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 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.

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 现在把 onOpenSkillonOpenSkillsFolderonImportManagedSkillSource 一起展开,条件是同一个 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。

@Astro-Han

Copy link
Copy Markdown
Contributor

LGTM. Merging at @astrohan's request — test is green on 27e57719a3b0d729e44f41a98ae08570fa2089ab.

Note for the two sibling slice extractions: this one has landed, so please rebase before the next one goes in.

简体中文

LGTM,按 @astrohan 的要求合并——27e57719a3b0d729e44f41a98ae08570fa2089abtest 为绿。

给另外两个同系列切片 PR 的提醒:这个已经合入,请在下一个合并前先 rebase。

@Astro-Han
Astro-Han merged commit 8fd33df into apache:main Aug 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants