refactor(desktop): lock renderer root architecture - #4088
Conversation
af9b950 to
df2d6f9
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
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:
- React 19’s
use()andReact.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. - 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 值得一起处理:
- React 19 的
use()和React.use()没有进入 Hook 识别器。它们目前可以在 strict zone 中通过,并且不会产生 Hook violation;但 Desktop 已经使用 React 19,文档也声明这些区域不能持有 React lifecycle/state。 - 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 合并模拟、问题的定级与可达性,以及基于第一性原理的架构方向,并对这条评论负责。
Generated-by: Codex
df2d6f9 to
29fdcae
Compare
|
Thanks — addressed in 29fdcae.
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. |
Summary
main.tsx.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/mainTMPDIR=/private/tmp npm testnpm run buildnpm run typechecknpm run lintnpm run format:checkAI use
Select exactly one:
Tool(s) and scope: Codex — implementation and validation.
Checklist
Does this PR entail a change in behavior?