Uh oh!
There was an error while loading. Please reload this page.
refactor(desktop): lock renderer root architecture - #4088
Conversation
af9b950 to
df2d6f9Compare
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 合并模拟、问题的定级与可达性,以及基于第一性原理的架构方向,并对这条评论负责。
df2d6f9 to
29fdcaeComparechihumyum
commented
Aug 29, 2026
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. |
Replacing an accidentally empty approval with the already confirmed formal review body.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for completing the rebase and addressing the earlier review in full.
I incrementally reviewed exact head 29fdcae. The React 19 use() coverage, namespace destructuring, Hook transition contract, and targetZone semantics are all resolved. The architecture direction remains sound: this establishes one enforceable renderer ownership boundary without introducing another product-state or lifecycle authority.
I also exercised the change against the current main: all 62 checker fixtures pass, and the AppShell render-scope gate passes with 42 Hooks across 81 call sites. I found no P0–P3 issues.
Approved.
One integration note: main advanced again after this rebase. The current merge tree only needs a mechanical ledger refresh (+25/-14, covering changes such as chat-recovery-notice.tsx and the ExploreAgent → Deep Research transition). That does not require another architecture review, but the ledger should be refreshed immediately before merge so the newly introduced gate is green on the resulting tree.
Review analysis was assisted by Codex and independent @reviewer agents. Astro-Han verified the exact head, the current-main merge simulation, the focused architecture gates, and the severity/reachability judgment, and owns this review.
中文对照
谢谢你完成 rebase,并完整处理了上一轮审查意见。
我对精确 head 29fdcae 做了增量审查。React 19 use() 覆盖、namespace destructuring、Hook transition 契约和 targetZone 语义都已经解决。架构方向仍然正确:它建立了一个可执行的 renderer ownership 边界,没有引入第二套产品状态或生命周期 authority。
我也把该变更合入当前 main 做了验证:62 个 checker fixture 全部通过,AppShell render-scope gate 也以 42 个 Hook、81 个调用点通过。没有发现 P0–P3。
Approve。
只有一个合并时的机械事项:本次 rebase 后 main 又继续前进,当前合并树需要刷新一次 ledger(+25/-14,主要记录 chat-recovery-notice.tsx 和 ExploreAgent → Deep Research 等变化)。这不需要再做一次架构重审,但应在合并前刷新,确保新引入的 gate 在最终合并树上保持绿色。
本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了精确 head、current-main 合并模拟、聚焦架构检查以及问题的分级与可达性,并对本次 Review 负责。
Generated-by: Codex
29fdcae to
f98c201Compare
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?