Skip to content

Conversation

@weareoutman
Copy link
Member

@weareoutman weareoutman commented Nov 26, 2025

依赖检查

组件之间的依赖声明,是微服务组件架构下的重要信息,请确保其正确性。

请勾选以下两组选项其中之一:

  • 本次 MR 没有使用上游组件(例如框架、后台组件等)的较新版本提供的特性。

或者:

  • 本次 MR 使用了上游组件(例如框架、后台组件等)的较新版本提供的特性。
  • 在对应的文件中更新了该上游组件的依赖版本(或确认了当前声明的依赖版本已包含本次 MR 使用的新特性)。

提交信息检查

Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG,请检查工作内容与提交信息是否相符,并在以下每组选项中都依次确认。

破坏性变更是针对于下游使用者而言,可以通过本次改动对下游使用者的影响来识别变更类型:

  • 下游使用者不做任何改动,仍可以正常工作时,那么它属于普通变更。
  • 反之,下游使用者不做改动就无法正常工作时,那么它属于破坏性变更。

例如,构件修改了一个属性名,小产品 Storyboard 中需要使用新属性名才能工作,那么它就是破坏性变更。
又例如,构件还没有任何下游使用者,那么它的任何变更都是普通变更。

破坏性变更:

  • ⚠️ 本次 MR 包含破坏性变更的提交,请继续确认以下所有选项:
  • 没有更好的兼容方案,必须做破坏性变更。
  • 使用了 feat 作为提交类型。
  • 标注了 BREAKING CHANGE: 你的变更说明
  • 同时更新了本仓库中所有下游使用者的调用。
  • 同时更新了本仓库中所有下游使用者对该子包的依赖为即将发布的 major 版本。
  • 同时为其它仓库的 Migrating 做好了准备,例如文档或批量改动的方法。
  • 手动验证过破坏性变更在 Migrate 后可以正常工作。
  • 破坏性变更所在的提交没有意外携带其它子包的改动。

新特性:

  • 本次 MR 包含新特性的提交,且该提交不带有破坏性变更,并使用了 feat 作为提交类型。
  • 给新特性添加了单元测试。
  • 手动验证过新特性可以正常工作。

问题修复:

  • 本次 MR 包含问题修复的提交,且该提交不带有新特性或破坏性变更,并使用了 fix 作为提交类型。
  • 给问题修复添加了单元测试。
  • 手动验证过问题修复得到解决。

杂项工作:

即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动,例如修改注释、测试用例、开发文档等:

  • 本次 MR 包含杂项工作的提交,且该提交不带有问题修复、新特性或破坏性变更,并使用了 chore, docs, test 等作为提交类型。

Summary by CodeRabbit

  • 重构

    • 引入统一的可调用清理机制,改进挂载/卸载流程以确保渲染树、门户及运行时上下文被确定性释放,降低内存泄漏风险。
    • 事件绑定与数据变更监听改为返回可撤销的清理函数,简化注册与移除,自动在路由/卸载时清理相关监听器。
    • 提供可用于手动触发清理的返回项,提升资源管理透明度与可控性。
  • 新功能

    • 新增一个用于复现内存泄漏场景的示例故事板(UI 配置)。

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 26, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

在运行时多处引入统一的 Dispose 清理函数模式:mountTree、bindListeners、DataStore.onChange 等改为返回可调用的 disposer,相关调用方保存并在卸载或重定向时调用;接口和类型同步新增 Dispose,并移除元素上旧的监听器字段。

Changes

Cohort / File(s) 变更摘要
Dispose 类型与接口
packages/runtime/src/internal/interfaces.ts
新增导出类型别名 Dispose = () => void;将 RenderAbstract.disposesRuntimeBrick.disposes 类型改为 Dispose[];从 RuntimeBrickElement 中移除 $$eventListeners$$listeners,新增 $$disposes?: Dispose[] 字段。
挂载与卸载生命周期
packages/runtime/src/internal/mount.ts, packages/runtime/src/createRoot.ts, packages/runtime/src/internal/Router.ts, packages/runtime/src/internal/secret_internals.ts
mountTree 改为返回 Dispose 并收集/返回挂载期间创建的 disposables;在 createRoot/Router 中新增局部 disposeMount 变量,保存 mountTree 返回的 disposer 并在适当时机调用以清理挂载树;secret_internalsMountUseBrickResult 新增 dispose: Dispose,并在卸载路径调用该 disposer。
事件监听绑定
packages/runtime/src/internal/bindListeners.ts, packages/runtime/src/internal/bindListeners.spec.ts
bindListeners 签名改为返回 Dispose;不再依赖元素上的 $$listeners/$$eventListeners,改用本地 disposables 数组并把每个监听器的移除函数收集为 disposer;函数现在返回统一的卸载函数;测试文件添加 consoleError 的 mock/重置以配合测试。
数据监听与清理
packages/runtime/src/internal/data/DataStore.ts
onChange 现在返回 Dispose 并通过新建的 disposableMap: Map<string, Dispose[]> 跟踪每个数据项的 disposables;新增私有方法 addDisposable 与公开 dispose(),在路由或整体清理时遍历并调用这些 disposables。
渲染上下文与元素卸载
packages/runtime/src/internal/RendererContext.ts, packages/react-element/src/ReactNextElement.tsx
在卸载流程中,遍历并执行 brick.element.$$disposes,调用元素的 __secret_internal_dispose()(若存在);ReactNextElement 新增 __secret_internal_dispose() 用于卸载 React 根并清理内部引用;卸载顺序调整以纳入新 disposer 路径。
文档与示例
etc/runtime.api.md, mock-micro-apps/memory-leak/storyboard.yaml
API 文档更新以反映 MountUseBrickResult.dispose: Dispose 的新增;新增 memory-leak storyboard 配置(UI/样例文件,非运行时代码逻辑)。

Estimated code review effort

🎯 3 (中等) | ⏱️ ~20-30 分钟

需特别关注:

  • packages/runtime/src/internal/bindListeners.ts:确保每个事件监听的移除函数正确生成、收集与一次性调用,处理异常或重复绑定情形。
  • packages/runtime/src/internal/mount.ts:disposables 的收集顺序与执行顺序,确保无漏调用或重复调用。
  • packages/runtime/src/internal/data/DataStore.tsdisposableMap 生命周期与路由处置时机,确保不会引入内存泄露或提前释放监听。
  • packages/runtime/src/createRoot.tspackages/runtime/src/internal/Router.tsdisposeMount 的赋值、重置与调用时机,验证在重定向/错误/中止路径下行为一致。

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning PR标题与实际变更内容不符。标题声称"fix test error reporting"(修复测试错误报告),但实际变更是实现了一个完整的Dispose机制用于资源清理和内存泄漏防止。 更新PR标题以准确反映实际变更,例如:"feat: implement dispose mechanism for resource cleanup" 或 "feat: add disposal lifecycle for mounted components"。
Description check ⚠️ Warning PR描述仅包含模板的复选框,但所有复选框均未勾选,没有提供任何关于本次MR工作内容、变更影响或提交信息的说明。 完善PR描述:勾选适用的破坏性变更/新特性/问题修复/杂项工作复选框,并填写"简单描述"部分说明实现了哪些工作。
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch steve/v3-dispose

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 6d8ff39 and dd5c9f5.

📒 Files selected for processing (8)
  • etc/runtime.api.md (1 hunks)
  • mock-micro-apps/memory-leak/storyboard.yaml (1 hunks)
  • packages/react-element/src/ReactNextElement.tsx (2 hunks)
  • packages/runtime/src/internal/RendererContext.ts (1 hunks)
  • packages/runtime/src/internal/Router.ts (8 hunks)
  • packages/runtime/src/internal/bindListeners.ts (2 hunks)
  • packages/runtime/src/internal/data/DataStore.ts (8 hunks)
  • packages/runtime/src/internal/interfaces.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/runtime/src/internal/interfaces.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/runtime/src/internal/Router.ts (3)
packages/runtime/src/internal/interfaces.ts (2)
  • RuntimeContext (22-42)
  • Dispose (210-210)
packages/runtime/src/internal/Renderer.ts (1)
  • getDataStores (1174-1180)
packages/runtime/src/internal/mount.ts (1)
  • mountTree (17-141)
packages/runtime/src/internal/data/DataStore.ts (2)
packages/runtime/src/internal/interfaces.ts (1)
  • Dispose (210-210)
packages/types/src/manifest.ts (1)
  • RouteConf (270-273)
packages/runtime/src/internal/RendererContext.ts (1)
packages/runtime/src/internal/data/DataStore.ts (1)
  • dispose (623-634)
packages/runtime/src/internal/bindListeners.ts (2)
packages/runtime/src/internal/interfaces.ts (1)
  • Dispose (210-210)
packages/runtime/src/internal/RendererContext.ts (1)
  • dispose (531-549)
🪛 GitHub Actions: CI
packages/runtime/src/internal/Router.ts

[error] 1-1: Lifecycle script 'docs:build' failed. Command 'api-extractor run' exited with code 1. API Extractor completed with warnings (e.g., API signature changes).

packages/runtime/src/internal/data/DataStore.ts

[error] 1-1: Lifecycle script 'docs:build' failed. Command 'api-extractor run' exited with code 1. API Extractor completed with warnings (e.g., API signature changes).

packages/runtime/src/internal/RendererContext.ts

[error] 1-1: Lifecycle script 'docs:build' failed. Command 'api-extractor run' exited with code 1. API Extractor completed with warnings (e.g., API signature changes).

packages/runtime/src/internal/bindListeners.ts

[error] 1-1: Lifecycle script 'docs:build' failed. Command 'api-extractor run' exited with code 1. API Extractor completed with warnings (e.g., API signature changes).

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
mock-micro-apps/memory-leak/storyboard.yaml (2)

1-6: App 配置结构清晰,适合用作测试用例。

应用配置包含所有必要的字段(name、id、homepage、noAuthGuard、standaloneMode),结构规范,符合 storyboard.yaml 的标准格式。


8-43: 路由嵌套结构设计良好,适合测试 Dispose 清理模式。

该路由配置包含以下优点:

  1. 嵌套路由结构(第 28-43 行):在 div 的 routes slot 中定义子路由,支持多层级导航,有助于验证清理函数在嵌套 mount/unmount 时的正确行为。
  2. 导航链接(第 17-27 行):通过 eo-link 在 /memory-leak/1/memory-leak/2 之间切换,触发路由装载和卸载,有利于检测资源是否正确释放。
  3. incrementalSubRoutes: true(第 11 行):允许子路由按需加载,使测试场景更贴近实际使用。

该配置能有效地测试新的 Dispose 清理模式是否在嵌套路由切换时正确回收资源。

packages/runtime/src/internal/RendererContext.ts (1)

349-360: LGTM! 销毁逻辑实现正确

新增的清理流程合理:

  1. 先清理 $$disposes 中的监听器
  2. 然后调用内部的 __secret_internal_dispose() 方法

类型断言是必要的,因为 __secret_internal_dispose 是一个内部方法,不在公共接口中声明。

packages/react-element/src/ReactNextElement.tsx (2)

15-16: LGTM! 复用已存在的 shadowRoot

这个改动可以正确处理元素重新连接到 DOM 的情况,避免重复调用 attachShadow(对已有 shadowRoot 的元素调用会抛出异常)。


41-49: LGTM! 正确的 React 清理模式

disconnectedCallback 先渲染 null 再调用 unmount() 是正确的做法,确保 React 组件能够正常执行清理逻辑。将 #root 设置为 undefined 可以释放对 React root 的引用,防止内存泄漏。

packages/runtime/src/internal/Router.ts (2)

400-410: LGTM! 正确清理 RuntimeContext 中的 DataStores

通过 getDataStores 获取所有相关的数据存储(ctxStore、tplStateStoreMap、formStateStoreMap),然后逐一调用 dispose() 方法。这与 RendererContext 的清理模式保持一致。


383-387: The review comment's concern is based on incorrect code flow analysis and should be resolved.

The original comment worried that disposeMount might not be called if rendering is interrupted between the mountTree call and cleanUpPreviousRender call. However, examining the actual code flow reveals the order is reversed:

  1. cleanUpPreviousRender() is called first (line 560 or 611) — this cleans up the previous iteration's disposeMount
  2. disposeMount = mountTree(renderRoot) is assigned after (lines 571 or 630)

Each render iteration properly cleans up the previous iteration's disposeMount before assigning a new one. The render method is also protected by the #rendering flag in #queuedRender's try/finally block (lines 189-201), preventing concurrent renders. If any exception occurs after the mountTree assignment, the next queued render will still call cleanUpPreviousRender() and clean up the stale disposer.

No resource leak is possible with this pattern.

packages/runtime/src/internal/data/DataStore.ts (1)

623-634: LGTM! dispose() 方法实现正确

完整清理了所有状态:

  • 清空 data Map
  • 清空 pendingStack
  • 执行并清空所有 disposables
  • 重置 routeMaprouteStackMap
packages/runtime/src/internal/bindListeners.ts (2)

45-78: LGTM! 清晰的 Dispose 模式重构

bindListeners 现在返回一个 Dispose 函数,实现了以下改进:

  1. 每个事件监听器都有独立的 dispose 函数
  2. 返回的 dispose 函数会清理所有监听器并清空 brick.$$disposes
  3. 如果没有 eventsMap,返回空操作函数,避免调用方需要判空

这种模式与 DataStore.onChangemountTree 的返回值保持一致,使得清理逻辑更加统一和可预测。


67-68: 确认 $$disposes 的双重注册是否符合预期

这里将 dispose 同时添加到了本地 disposables 数组和 brick.$$disposes。返回的 disposer(第 71-77 行)会清理 disposables 并删除 brick.$$disposes

但在 RendererContext.ts#unmountBricks 中(第 349 行),也会遍历并调用 brick.element?.$$disposes

这意味着如果 bindListeners 返回的 disposer 被调用后,#unmountBricks 再次运行时,$$disposes 已被删除,不会重复执行。这个设计是正确的。

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/runtime/src/createRoot.ts (1)

22-26: 建议在二次 render 前也调用上一次的 disposeMount

当前只在 unmount() 里调用 disposeMount?.(),多次调用 render() 时,上一次 mountTree 的 disposer 会被新的覆盖而从未执行,虽然 DOM 已被 unmountTree(container) 清空,但某些监听可能仍持有跨 DOM 引用或外部资源。

可以考虑在每次新渲染前先释放上一次的 disposer,例如:

    async render( /* ... */ ) {
      if (unmounted) {
        throw new Error(
          "The root is unmounted and cannot be rendered any more"
        );
      }
+     // 先清理上一轮的 mount
+     if (disposeMount) {
+       disposeMount();
+       disposeMount = undefined;
+     }
      const bricks = ([] as BrickConf[]).concat(brick);
      // ...
-     disposeMount = mountTree(renderRoot);
+     disposeMount = mountTree(renderRoot);

这样行为与 Router 的清理逻辑更一致,避免长生命周期 root 在频繁 render 场景下潜在的资源残留。

Also applies to: 114-116, 261-262, 291-293

packages/runtime/src/internal/interfaces.ts (1)

208-209: Dispose 类型别名定义清晰,可考虑后续统一使用

  • 新增 export type Dispose = () => void; 让各处清理函数有了统一类型,便于复用(mountTree、bindListeners、DataStore 等已依赖该别名)。
  • 目前接口内仍有 disposes?: (() => void)[]; 这样的写法,后续可以视情况逐步替换为 Dispose[] 提升可读性和一致性,但不是现在必须要改。
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 22be1cb and 0a334bc.

📒 Files selected for processing (8)
  • packages/runtime/src/createRoot.ts (4 hunks)
  • packages/runtime/src/internal/Router.ts (4 hunks)
  • packages/runtime/src/internal/bindListeners.spec.ts (3 hunks)
  • packages/runtime/src/internal/bindListeners.ts (2 hunks)
  • packages/runtime/src/internal/data/DataStore.ts (7 hunks)
  • packages/runtime/src/internal/interfaces.ts (1 hunks)
  • packages/runtime/src/internal/mount.ts (4 hunks)
  • packages/runtime/src/internal/secret_internals.ts (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/runtime/src/createRoot.ts (2)
packages/runtime/src/internal/interfaces.ts (1)
  • Dispose (209-209)
packages/runtime/src/internal/mount.ts (1)
  • mountTree (17-141)
packages/runtime/src/internal/bindListeners.ts (1)
packages/runtime/src/internal/interfaces.ts (1)
  • Dispose (209-209)
packages/runtime/src/internal/mount.ts (2)
packages/runtime/src/internal/interfaces.ts (2)
  • Dispose (209-209)
  • RuntimeBrickElement (113-120)
packages/runtime/src/internal/bindListeners.ts (1)
  • bindListeners (45-74)
packages/runtime/src/internal/data/DataStore.ts (1)
packages/runtime/src/internal/interfaces.ts (1)
  • Dispose (209-209)
packages/runtime/src/internal/Router.ts (2)
packages/runtime/src/internal/interfaces.ts (1)
  • Dispose (209-209)
packages/runtime/src/internal/mount.ts (1)
  • mountTree (17-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/runtime/src/internal/secret_internals.ts (1)

39-40: useBrick 的 Dispose 接入整体合理

  • 引入 Dispose 类型并在 MountUseBrickResult 中强制要求 dispose 字段,使 mountUseBrick/unmountUseBrick 生命周期和 Router/createRoot 保持一致。
  • unmountUseBrick 先调用 mountResult.dispose() 再卸载 portal 与 rendererContext,清理顺序也符合预期。

Also applies to: 134-138, 153-167, 170-178

packages/runtime/src/internal/Router.ts (1)

42-46: Router 中新增的 Dispose 流程设计合理

  • 通过 disposeMount?: Dispose 配合 cleanUpPreviousRender 在每次新渲染前主动执行上一次 mountTree 的清理函数,之后再 unmountTree(main/portal),可以避免事件监听、订阅等资源在路由切换间累积。
  • 错误页分支同样用 disposeMount = mountTree(renderRoot);,确保后续再次渲染时也能统一释放。

Also applies to: 380-385, 554-556, 614-615

packages/runtime/src/internal/bindListeners.spec.ts (1)

676-689: 针对 event. 的 consoleError mock 调整是合理的*

  • event.preventDefault / event.stopPropagation 以及 “non-Event object” 三个用例中显式 mockReturnValue 并在断言后 mockReset,可以避免控制台噪声,也让这些用例对 console.error 的依赖范围更清晰可控。
  • 不影响原有断言语义,整体测试可读性和隔离性都有提升。

Also applies to: 691-723

packages/runtime/src/internal/mount.ts (1)

7-11: mountTree 返回 Dispose 的实现与监听绑定逻辑匹配良好

  • 通过 const disposables: Dispose[] = [] 收集每个 bindListeners 返回的清理函数,并在返回的总 disposer 中统一执行,保证所有事件监听在上层调用 dispose() 时被正确移除。
  • setTimeout 中恢复 window.DISABLE_REACT_FLUSH_SYNC 的逻辑保持不变,签名从 void 改为 Dispose 对现有调用者(Router/createRoot/useBrick)是向后兼容的增强。

Also applies to: 17-21, 25-26, 61-63, 135-140

packages/runtime/src/internal/bindListeners.ts (1)

28-32: bindListeners 返回 Dispose 的实现清晰且易于与 mountTree 集成

  • 当无事件配置时直接返回一个 noop Dispose,避免调用方做额外判空。
  • 为每个 addEventListener 创建对应的移除函数并收集到 disposables 中,最终由总的 disposer 统一执行,外部(如 mountTree)只需持有并调用一次即可完成全部解绑。
  • 这种“返回清理函数”的模式比之前依赖 element 内部状态(如 $$listeners)更可预测,也更便于测试。

Also applies to: 45-52, 54-55, 63-65, 68-73

Comment on lines +22 to 23
Dispose,
RouteNode,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

disposableMap 中重复存储同一 Dispose,可能导致长期悬挂引用

目前同一个 Dispose 会被存储两次:

  1. onChange(dataName, listener) 内部调用 this.addDisposable(dataName, disposable);(key 为依赖名 dataName)。
  2. dataConf.track 分支中再次调用 this.addDisposable(dataConf.name, disposable);(key 为当前 data 名)。

在路由销毁逻辑中,disposeDataInRoutes 只按将要删除的 data 名(来自 routeMap)去查询 disposableMap.get(name) 并执行其中的 disposables,然后删除该 key。这样会产生两个问题:

  • 对于作为“依赖”的 data(例如全局 CTX),其 key 下的 Dispose[] 永远不会被 disposeDataInRoutes 删除,即使这些 Dispose 已经在以 dataConf.name 为 key 的数组中被执行过,仍然会在 disposableMap 中保留,持有闭包引用,长期运行有内存上涨风险。
  • 同一个 disposable 被存入两个数组,遇到同时销毁依赖和被依赖 data 的场景时会被调用两次(虽然 DOM/EventTarget 层面是幂等的,但这是不必要的重复工作,也让状态变得更难推理)。

建议把“订阅登记”和“所有者”解耦,避免重复存储同一个 Dispose,比如采取更简单的策略:

  • onChange 仅负责注册监听并返回 Dispose,不在内部调用 addDisposable

    onChange(dataName: string, listener: EventListener): Dispose {
      const eventTarget = this.data.get(dataName)?.eventTarget;
      eventTarget?.addEventListener(this.changeEventType, listener);
  • const disposable = () => {

  • const disposable = () => {
    eventTarget?.removeEventListener(this.changeEventType, listener);
    };
  • this.addDisposable(dataName, disposable);
    return disposable;
    }

- 由真正需要 route 级自动清理的调用方(如 `dataConf.track` 那段)根据“拥有者”名字显式调用 `addDisposable(dataConf.name, disposable)`,避免再按依赖名重复登记。

如果确实有其它地方依赖“按依赖名自动登记”的行为,也可以考虑为 `onChange` 增加一个可选 ownerName 参数,仅以 ownerName 作为 `disposableMap` 的 key 来管理清理。





Also applies to: 75-76, 365-373, 545-563, 586-593, 603-612

<!-- fingerprinting:phantom:poseidon:olive -->

<!-- This is an auto-generated comment by CodeRabbit -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request refactors the runtime's resource cleanup mechanism by introducing a Dispose pattern for managing event listeners and other disposable resources. The changes improve memory management and lifecycle handling by ensuring proper cleanup of event listeners when components are unmounted or routes change.

Key changes:

  • Introduced Dispose type and refactored event listener binding to return cleanup functions
  • Modified mountTree to collect and return all disposables for later cleanup
  • Updated DataStore to track and dispose event listeners associated with data items

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/runtime/src/internal/interfaces.ts Added Dispose type definition and removed unused $$eventListeners and $$listeners properties
packages/runtime/src/internal/bindListeners.ts Refactored bindListeners to return a dispose function instead of storing listeners on elements
packages/runtime/src/internal/mount.ts Modified mountTree to collect disposables and return cleanup function
packages/runtime/src/internal/secret_internals.ts Updated mountUseBrick to propagate dispose function through mount result
packages/runtime/src/internal/data/DataStore.ts Added disposable tracking for data change listeners with cleanup in route disposal
packages/runtime/src/internal/Router.ts Added dispose function storage and invocation in cleanup flow
packages/runtime/src/createRoot.ts Added dispose function storage and invocation in unmount flow
packages/runtime/src/internal/bindListeners.spec.ts Fixed test error reporting by mocking console.error

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

!load && (newData.deps = [...deps]);
for (const dep of deps) {
this.onChange(
const disposable = this.onChange(
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The variable name disposable is inconsistent with the naming in the onChange method which also uses disposable. Consider renaming this to depDisposable or dependencyDisposable to clarify it's a disposable for a dependency listener, distinguishing it from the general disposable in the onChange method.

Copilot uses AI. Check for mistakes.
if (disposables) {
for (const disposable of disposables) {
disposable();
}
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disposables array cleanup is missing. After calling all dispose functions, the array should be cleared (e.g., disposables.length = 0) before deleting the map entry, similar to the pattern used in bindListeners.ts:72 and mount.ts:139. While the map entry is deleted, this could leave dangling references if the array is still referenced elsewhere.

Suggested change
}
}
disposables.length = 0;

Copilot uses AI. Check for mistakes.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 0a334bc and 6d8ff39.

📒 Files selected for processing (1)
  • etc/runtime.api.md (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build (20.x)
  • GitHub Check: Analyze (javascript-typescript)

Comment on lines +335 to +338
// Warning: (ae-forgotten-export) The symbol "Dispose" needs to be exported by the entry point index.d.ts
//
// (undocumented)
dispose: Dispose;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Script executed:

#!/bin/bash
# Find the entry point file for the runtime package
find . -path "*/packages/runtime/*" -type f \( -name "index.ts" -o -name "index.d.ts" \) | head -10

Repository: easyops-cn/next-core


<shell_result>
Repository: easyops-cn/next-core
Exit code: 0

stdout:

./packages/runtime/src/index.ts

</shell_result>


Script executed:

#!/bin/bash
# Read the entry point file to check current exports
cat -n ./packages/runtime/src/index.ts

Repository: easyops-cn/next-core


<shell_result>
Repository: easyops-cn/next-core
Exit code: 0

stdout:

     1	export * from "./auth.js";
     2	export * from "./CustomEditors.js";
     3	export * from "./CustomProcessors.js";
     4	export { customTemplates } from "./CustomTemplates.js";
     5	export * from "./fetchByProvider.js";
     6	export * from "./getBasePath.js";
     7	export * from "./getPageInfo.js";
     8	export * from "./handleHttpError.js";
     9	export * from "./history.js";
    10	export * from "./createRoot.js";
    11	export * from "./getRealValue.js";
    12	export {
    13	  createRuntime,
    14	  getRuntime,
    15	  type RuntimeOptions,
    16	  type RuntimeHooks,
    17	  type RuntimeHooksMenuHelpers,
    18	  type ImagesFactory,
    19	  type PageViewInfo,
    20	} from "./internal/Runtime.js";
    21	import * as __secret_internals from "./internal/secret_internals.js";
    22	export { __secret_internals };
    23	export { __test_only } from "./internal/test_only.js";
    24	export {
    25	  getCssPropertyValue,
    26	  getCurrentTheme,
    27	  getCurrentMode,
    28	  getThemeVariant,
    29	  batchSetAppsLocalTheme,
    30	  applyTheme,
    31	} from "./themeAndMode.js";
    32	export {
    33	  checkIfOfComputed,
    34	  checkIfByTransform,
    35	} from "./internal/compute/checkIf.js";
    36	export { registerWidgetFunctions } from "./internal/compute/WidgetFunctions.js";
    37	export { registerWidgetI18n } from "./internal/compute/WidgetI18n.js";
    38	export { StoryboardFunctionRegistryFactory } from "./StoryboardFunctionRegistry.js";
    39	export { matchPath, type MatchOptions } from "./internal/matchPath.js";
    40	export { Notification, type NotificationOptions } from "./Notification.js";
    41	export { Dialog, type DialogOptions } from "./Dialog.js";
    42	export * from "./getV2RuntimeFromDll.js";
    43	export { setUIVersion } from "./setUIVersion.js";
    44	export * from "./ModalStack.js";
    45	export * from "./isNetworkError.js";
    46	export * from "./shouldReloadForError.js";

</shell_result>


Script executed:

#!/bin/bash
# Search for MountUseBrickResult to see where it's defined and exported
rg -n "MountUseBrickResult" --type ts -A 2 -B 2 | head -40

Repository: easyops-cn/next-core


<shell_result>
Repository: easyops-cn/next-core
Exit code: 0

stdout:

packages/runtime/src/internal/secret_internals.ts-132-}
packages/runtime/src/internal/secret_internals.ts-133-
packages/runtime/src/internal/secret_internals.ts:134:export interface MountUseBrickResult {
packages/runtime/src/internal/secret_internals.ts-135-  portal?: HTMLElement;
packages/runtime/src/internal/secret_internals.ts-136-  dispose: Dispose;
--
packages/runtime/src/internal/secret_internals.ts-140-  { renderRoot, rendererContext, scopedStores }: RenderUseBrickResult,
packages/runtime/src/internal/secret_internals.ts-141-  element: HTMLElement
packages/runtime/src/internal/secret_internals.ts:142:): MountUseBrickResult {
packages/runtime/src/internal/secret_internals.ts-143-  let portal: HTMLElement | undefined;
packages/runtime/src/internal/secret_internals.ts-144-  renderRoot.createPortal = () => {
--
packages/runtime/src/internal/secret_internals.ts-170-export function unmountUseBrick(
packages/runtime/src/internal/secret_internals.ts-171-  { rendererContext }: RenderUseBrickResult,
packages/runtime/src/internal/secret_internals.ts:172:  mountResult: MountUseBrickResult
packages/runtime/src/internal/secret_internals.ts-173-): void {
packages/runtime/src/internal/secret_internals.ts-174-  // if (mountResult.mainBrick) {
--
packages/runtime/src/internal/Renderer.spec.ts-108-  "my-use-brick",
packages/runtime/src/internal/Renderer.spec.ts-109-  class MyUseBrick extends HTMLElement {
packages/runtime/src/internal/Renderer.spec.ts:110:    #mountResult: __secret_internals.MountUseBrickResult | undefined;
packages/runtime/src/internal/Renderer.spec.ts-111-    #renderResult: __secret_internals.RenderUseBrickResult | undefined;
packages/runtime/src/internal/Renderer.spec.ts-112-    async connectedCallback() {
--
packages/react-runtime/src/ReactUseBrick.tsx-35-  const [renderResult, setRenderResult] =
packages/react-runtime/src/ReactUseBrick.tsx-36-    useState<__secret_internals.RenderUseBrickResult | null>(null);
packages/react-runtime/src/ReactUseBrick.tsx:37:  const mountResult = useRef<__secret_internals.MountUseBrickResult>();
packages/react-runtime/src/ReactUseBrick.tsx-38-  const [renderKey, setRenderKey] = useState<number>();
packages/react-runtime/src/ReactUseBrick.tsx-39-  const IdCounterRef = useRef(0);

</shell_result>


Script executed:

#!/bin/bash
# Check the import of Dispose in secret_internals.ts
head -50 ./packages/runtime/src/internal/secret_internals.ts | grep -n "Dispose\|import"

Repository: easyops-cn/next-core


<shell_result>
Repository: easyops-cn/next-core
Exit code: 0

stdout:

1:import type {
12:import { pick } from "lodash";
13:import {
19:import {
24:import { RendererContext } from "./RendererContext.js";
25:import type { DataStore } from "./data/DataStore.js";
26:import { resolveData } from "./data/resolveData.js";
27:import { asyncComputeRealValue } from "./compute/computeRealValue.js";
28:import type {
39:  Dispose,
41:import { mountTree, unmountTree } from "./mount.js";
42:import { RenderTag } from "./enums.js";
43:import { isStrictMode } from "../isStrictMode.js";
44:import { customTemplates } from "../CustomTemplates.js";
45:import { registerAppI18n } from "./registerAppI18n.js";
46:import { getTplStateStore } from "./CustomTemplates/utils.js";
47:import {

</shell_result>


Script executed:

#!/bin/bash
# Verify the full import statement for Dispose
sed -n '1,45p' ./packages/runtime/src/internal/secret_internals.ts | grep -A 15 "^import type {"

Repository: easyops-cn/next-core


<shell_result>
Repository: easyops-cn/next-core
Exit code: 0

stdout:

import type {
  BrickConf,
  CustomTemplate,
  RouteConf,
  Storyboard,
  UseSingleBrickConf,
  RuntimeSnippet,
  ContextConf,
  BrickPackage,
  RouteConfOfBricks,
} from "@next-core/types";
import { pick } from "lodash";
import {
  _internalApiGetRenderId,
  _internalApiGetRuntimeContext,
  _internalApiGetStoryboardInBootstrapData,
--
import type { DataStore } from "./data/DataStore.js";
import { resolveData } from "./data/resolveData.js";
import { asyncComputeRealValue } from "./compute/computeRealValue.js";
import type {
  DataValueOption,
  PreviewOption,
  PreviewStoryboardPatch,
  RenderRoot,
  RuntimeContext,
  DebugDataValue,
  RuntimeDataVale,
  RuntimeDataValueOption,
  RenderBrick,
  RenderChildNode,
  Dispose,
} from "./interfaces.js";
import { mountTree, unmountTree } from "./mount.js";
import { RenderTag } from "./enums.js";
import { isStrictMode } from "../isStrictMode.js";

</shell_result>


Add Dispose type to entry point exports to resolve API Extractor warning

The warning is valid: Dispose is used in the public API (MountUseBrickResult interface exported via __secret_internals) but not exported from the main entry point. The type is imported from ./internal/interfaces.ts into secret_internals.ts but never re-exported from packages/runtime/src/index.ts. This prevents users from directly referencing the Dispose type despite encountering it in the public API.

To fix this, add Dispose to the exports in packages/runtime/src/index.ts:

export {
  createRuntime,
  getRuntime,
  type RuntimeOptions,
  type RuntimeHooks,
  type RuntimeHooksMenuHelpers,
  type ImagesFactory,
  type PageViewInfo,
  type Dispose,  // Add this line
} from "./internal/Runtime.js";

Or add a separate export from interfaces.ts if Dispose is not already exported from Runtime.js.

🤖 Prompt for AI Agents
In etc/runtime.api.md around lines 335 to 338, the API Extractor warning shows
the Dispose type is referenced in the public API but not exported from the
package entry point; fix by exporting Dispose from the runtime entry
(packages/runtime/src/index.ts) — add Dispose to the export list (or if Dispose
is only declared in interfaces.ts, re-export it from Runtime.js or directly from
interfaces.ts) so the public surface exposes the Dispose type alongside the
other exported runtime types.

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

❌ Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.20%. Comparing base (22be1cb) to head (6d8ff39).

Files with missing lines Patch % Lines
packages/runtime/src/internal/data/DataStore.ts 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##               v3    #4828      +/-   ##
==========================================
+ Coverage   95.16%   95.20%   +0.03%     
==========================================
  Files         212      212              
  Lines        9357     9376      +19     
  Branches     1802     1801       -1     
==========================================
+ Hits         8905     8926      +21     
+ Misses        332      329       -3     
- Partials      120      121       +1     
Files with missing lines Coverage Δ
packages/runtime/src/createRoot.ts 96.73% <100.00%> (+0.07%) ⬆️
packages/runtime/src/internal/Router.ts 95.41% <100.00%> (+0.03%) ⬆️
packages/runtime/src/internal/bindListeners.ts 92.39% <100.00%> (+1.64%) ⬆️
packages/runtime/src/internal/mount.ts 95.23% <100.00%> (+0.41%) ⬆️
packages/runtime/src/internal/secret_internals.ts 95.00% <100.00%> (+0.03%) ⬆️
packages/runtime/src/internal/data/DataStore.ts 96.95% <82.35%> (-1.20%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cypress
Copy link

cypress bot commented Nov 26, 2025

next-core    Run #11833

Run Properties:  status check passed Passed #11833  •  git commit 8774df3a06 ℹ️: Merge 6d8ff39ef1e1b7e47f16d84189371acfc001deff into 22be1cb42e9c501728041dde6713...
Project next-core
Branch Review steve/v3-dispose
Run status status check passed Passed #11833
Run duration 00m 24s
Commit git commit 8774df3a06 ℹ️: Merge 6d8ff39ef1e1b7e47f16d84189371acfc001deff into 22be1cb42e9c501728041dde6713...
Committer Shenwei Wang
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 17
View all changes introduced in this branch ↗︎

@weareoutman weareoutman marked this pull request as draft November 27, 2025 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants