Skip to content

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

Open
chihumyum wants to merge 11 commits into
apache:mainfrom
chihumyum:refactor/module-hub-feature-slice
Open

refactor(desktop): extract Module Hub feature slice#3649
chihumyum wants to merge 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.

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