Skip to content

refactor(desktop): lock renderer root architecture - #4088

Open
chihumyum wants to merge 1 commit into
apache:mainfrom
chihumyum:refactor/app-shell-root-foundation
Open

refactor(desktop): lock renderer root architecture#4088
chihumyum wants to merge 1 commit into
apache:mainfrom
chihumyum:refactor/app-shell-root-foundation

Conversation

@chihumyum

@chihumyum chihumyum commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add enforced Desktop renderer ownership zones and a generated AppShell/root debt baseline.
  • Prevent AppShell and root-entry dependency growth while allowing debt to shrink during later slices.
  • Pin the production renderer entry chain from the main process through Vite to main.tsx.
  • Run the architecture check in CI.

Refs #3439

This is the R1 root foundation. It does not move feature ownership out of AppShell or close #3439.

Verification

  • npm run check:renderer-architecture -- --base upstream/main
  • TMPDIR=/private/tmp npm test
  • npm run build
  • npm run typecheck
  • npm run lint
  • npm run format:check

AI use

Select exactly one:

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

Tool(s) and scope: Codex — implementation and validation.

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 — CI and builds now reject renderer architecture regressions; runtime product behavior is unchanged.
  • No

@github-actions github-actions Bot added the effort/XL Over 1000 readable lines label Aug 28, 2026
@chihumyum
chihumyum force-pushed the refactor/app-shell-root-foundation branch from af9b950 to df2d6f9 Compare August 28, 2026 13:44

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

Thanks for putting serious work into making the renderer architecture enforceable. The problem is real: AppShell should stop being the default owner for feature state, subscriptions, environment access, and side effects. A monotonic architecture ratchet is a reasonable way to prevent that debt from continuing to grow.

Before approval, the current head needs to be rebased and the generated ledger refreshed. The ledger was generated against 590d37c, while current main has since changed the renderer file set, Hook calls, and dependency closure. In an independent current-main merge simulation, all 58 checker fixtures passed, but the actual architecture check failed with changes to the legacy AppShell file set, transitive closure, Hook calls, and dependency paths. CI is green for the PR’s old base, but that ledger is not valid for the code that would now be merged.

While refreshing it, I think two P2 gaps are worth addressing:

  1. React 19’s use() and React.use() are not included in the Hook recognizer. Both currently pass through strict zones without producing a Hook violation, even though Desktop uses React 19 and the documented contract says these zones reject React lifecycle/state ownership.
  2. The Hook ratchet compares exact Hook names. Replacing a broad legacy Hook with a narrower selector/read Hook is therefore rejected as new debt, even when the old Hook disappears and the ownership boundary improves. This can push future migrations toward retaining compatibility wrappers or hiding new reads inside an existing Hook. An explicit Hook transition rule, or a scope-based monotonic check, would better serve the architectural goal.

A related scanner case is namespace destructuring such as const { useHotkeys } = hooks, which is also currently missed. It can likely be covered alongside the React use() fixtures.

One design point should also be made explicit: ownership.targetZone is currently validated as metadata but is not enforced when a capability is migrated. If it is intended only as a human-readable roadmap, documenting that would be enough; if it is intended as an authority constraint, the checker needs a verifiable destination contract.

The 10k-line diff is not 10k lines of product logic: roughly 5.6k lines are the generated ledger, 2.2k are the checker, and 2.1k are focused checker tests. I did not find a sound reason to split it mechanically or delete those tests merely to reduce the diff. The important next step is making the ratchet valid against current main and ensuring that it supports correct migrations rather than only freezing today’s syntax.

Review analysis was assisted by Codex and seven independent @reviewer agents; Astro-Han evaluated the current-main merge simulation, severity and reachability of the findings, and the first-principles architecture direction and owns this comment.

中文对照

谢谢你为 renderer 架构的可执行约束投入了这么多工作。它解决的问题是真实的:AppShell 不应该继续成为 feature 状态、订阅、环境访问和副作用的默认 owner。用单调收敛的架构 ratchet 阻止债务继续增长,是一个合理方向。

在 Approve 之前,当前 head 需要先 rebase,并重新生成 ledger。现有 ledger 基于 590d37c,而当前 main 已经改变了 renderer 文件集合、Hook 调用和依赖闭包。在一次独立的 current-main 合并模拟中,58 个 checker fixture 全部通过,但实际架构检查会因为 legacy AppShell 文件集合、传递闭包、Hook 调用和依赖路径变化而失败。当前 CI 只证明它相对旧 base 是绿色的,并不能证明这份 ledger 对现在实际要合入的代码仍然有效。

重新生成时,我认为有两个 P2 值得一起处理:

  1. React 19 的 use()React.use() 没有进入 Hook 识别器。它们目前可以在 strict zone 中通过,并且不会产生 Hook violation;但 Desktop 已经使用 React 19,文档也声明这些区域不能持有 React lifecycle/state。
  2. Hook ratchet 按精确 Hook 名称比较。即使删除了一个宽泛的旧 Hook,并替换成职责更窄的 selector/read Hook,也会被判定为新增债务。这可能迫使后续迁移保留兼容 wrapper,或者把新的读取逻辑藏进旧 Hook。显式的 Hook transition 规则,或者基于调用作用域的单调检查,会更符合真正的架构目标。

另一个相关漏口是 const { useHotkeys } = hooks 这样的 namespace 解构,目前也不会被 scanner 识别。它可以和 React use() 的 fixture 一起补齐。

还需要明确一个设计语义:ownership.targetZone 现在只是经过格式校验的 metadata,并没有在 capability 迁移时真正执行。如果它只是给人阅读的迁移路线图,把这一点写清楚就够了;如果它代表 authority 约束,checker 就需要一个可验证的目标 owner contract。

这个一万行 diff 并不是一万行产品逻辑:约 5,600 行是生成 ledger,2,200 行是 checker,2,100 行是有针对性的 checker 测试。我没有找到仅为了缩小 diff 就机械拆分或者删除这些测试的合理依据。现在真正重要的是,让 ratchet 对当前 main 有效,并确保它能够支持正确迁移,而不只是冻结今天的代码写法。

本次审查分析由 Codex 和七个独立的 @reviewer 子代理协助;Astro-Han 评估了 current-main 合并模拟、问题的定级与可达性,以及基于第一性原理的架构方向,并对这条评论负责。

@chihumyum
chihumyum force-pushed the refactor/app-shell-root-foundation branch from df2d6f9 to 29fdcae Compare August 29, 2026 12:13
@chihumyum

Copy link
Copy Markdown
Contributor Author

Thanks — addressed in 29fdcae.

  • Rebased onto current main (a57d42c99) and regenerated/validated the ledger; stale ownership paths were removed, and features/*/stories remains test-only.
  • The scanner now recognizes React 19 use/React.use, namespace destructuring (including TypeScript wrappers/defaults), and conservatively tracks reassigned aliases.
  • Added one-time hookTransitions: each transition needs a same-path, same-section old-Hook decrease and new-Hook increase; aggregate Hook debt cannot grow, and transitions cannot be reused.
  • Documented targetZone as roadmap metadata rather than an enforced destination-authority contract.

Local validation passed: 62 checker fixtures, the checkout architecture check, AppShell Hook scope, 1,696 tests, build, Storybook, typecheck, lint, and format. CI is running on the exact head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Over 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(desktop): make AppShell a renderer composition root

2 participants