Skip to content

Conversation

@weareoutman
Copy link
Member

@weareoutman weareoutman commented Nov 17, 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

发布说明

此版本为内部改进,对用户无直接影响

  • 重构
    • 优化并集中了事件处理逻辑,提升内部可维护性与一致性。
  • 测试
    • 增强了事件相关的单元测试,改进异常情况的覆盖与行为验证。

@coderabbitai
Copy link

coderabbitai bot commented Nov 17, 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

重构 bindListeners.ts 中对事件方法(如 preventDefaultstopPropagation)的调用:新增 handleEventAction 辅助函数,统一通过 argsFactoryuseEventAsDefault: true)计算参数并在提取的 Event 对象上调用指定方法;相关单元测试在 bindListeners.spec.ts 中更新并新增非 Event 情况的错误分支测试。 (≤50 字)

Changes

内聚 / 文件(s) 更改摘要
事件处理重构
packages/runtime/src/internal/bindListeners.ts
提取并引入新的辅助函数 handleEventAction,将对 preventDefaultstopPropagation 的直接调用改为通过 argsFactoryuseEventAsDefault: true)计算参数、从结果中提取 Event 并在其上调用指定方法;在非 Event 结果时记录错误并回退至原始 event 方法。
测试更新与新增
packages/runtime/src/internal/bindListeners.spec.ts
调整 event.preventDefault 相关测试以使用 <% EVENT %>,新增 event.stopPropagation 的直接调用与非 Event 对象情形下应记录 console.error 且仍调用原始方法的测试;修正并重排测试序列以反映新的错误处理行为。

Estimated code review effort

🎯 3 (中等) | ⏱️ ~25 分钟

  • 关注点:
    • handleEventAction 中对 argsFactory(useEventAsDefault: true) 的调用与返回值解包逻辑,确保正确提取 Event 并处理非 Event 分支的降级策略与错误日志。
    • 验证调用点(原先的 preventDefault/stopPropagation 分支)已正确替换为对新辅助函数的调用,且控制流保持一致。
    • 检查新增/修改的单元测试是否覆盖正常路径与异常(非 Event)路径,及测试中对 console.error 的断言与重置是否恰当。

概览

bindListeners.ts 中的事件处理逻辑被重构。将直接处理 event.preventDefaultevent.stopPropagation 的代码提取到新的 handleEventAction 辅助函数中,通过参数工厂计算参数并调用相应的事件方法,保持现有控制流不变。测试文件 bindListeners.spec.ts 已更新并新增针对 stopPropagation 及非 Event 情形的测试。

更改

内聚 / 文件 更改摘要
事件处理重构
packages/runtime/src/internal/bindListeners.ts
引入新的内部辅助函数 handleEventAction,用于集中处理事件方法调用。重构 preventDefaultstopPropagation 操作的处理逻辑,改用新的辅助函数,通过 useEventAsDefault: true 计算参数并从结果中提取事件对象。
测试调整与补充
packages/runtime/src/internal/bindListeners.spec.ts
更新 event.preventDefault 测试以使用 <% EVENT %>,新增 event.stopPropagation 的调用测试以及对非 Event 对象情形下应产生日志并回退调用原始方法的测试。

预估代码审查工作量

🎯 3 (中等) | ⏱️ ~25 分钟

  • 重点关注 handleEventAction 函数的实现细节、参数计算与事件方法调用的正确性。
  • 验证重构后 preventDefaultstopPropagation 的行为与原有逻辑一致,及新增测试覆盖异常分支。

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description contains the complete template structure with all required sections (依赖检查, 提交信息检查, 破坏性变更, 新特性, 问题修复, 杂项工作), but no checkboxes are actually marked and no substantive content is provided beyond the template itself. Please mark the appropriate checkbox(es) for your MR type (breaking change/new feature/bug fix/miscellaneous), and ensure all related confirmations are checked according to the PR category.
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.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'fix(): allow setting target event for event actions' clearly summarizes the main change: enabling users to set a target event for event actions, which aligns with the code changes that refactor event method handling.
✨ 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-event-action-target

📜 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 9fad896 and 351d7dc.

📒 Files selected for processing (2)
  • packages/runtime/src/internal/bindListeners.spec.ts (2 hunks)
  • packages/runtime/src/internal/bindListeners.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/runtime/src/internal/bindListeners.ts
⏰ 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)
🔇 Additional comments (3)
packages/runtime/src/internal/bindListeners.spec.ts (3)

677-687: 测试覆盖完善,验证显式传递事件的场景。

这个测试用例现在显式地通过 args: ["<% EVENT %>"] 传递事件对象,验证了 event.preventDefault 可以从参数中正确提取 Event 对象并调用其方法。这与新的集中式事件处理机制一致。


689-698: 测试默认事件传递行为,符合重构目标。

这个测试用例移除了显式的 args 参数,验证了当未提供参数时,事件处理机制会默认使用触发事件(useEventAsDefault: true)。这与 event.preventDefault 测试形成对比,共同覆盖了显式传参和默认传参两种代码路径。


700-719: 验证通过,无需修改。

错误消息格式与实现代码一致。该测试用例正确验证了非 Event 对象的错误处理路径:实现在第 353 行使用了 call event.${method}() on non-Event object: 模板,与测试中预期的消息格式完全匹配。测试代码的 mock 管理、错误验证和优雅降级逻辑都正确无误。


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

📜 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 13468db and 9fad896.

📒 Files selected for processing (1)
  • packages/runtime/src/internal/bindListeners.ts (2 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). (4)
  • GitHub Check: CodeQL analysis (javascript-typescript)
  • GitHub Check: Agent
  • GitHub Check: build (20.x)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/runtime/src/internal/bindListeners.ts (1)

208-211: 代码重构合理,逻辑清晰。

event.preventDefaultevent.stopPropagation 的处理提取到独立的 handleEventAction 函数中,保持了与其他事件处理器的一致性。

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 PR modifies the event action handlers for event.preventDefault and event.stopPropagation to allow specifying a target event through arguments, rather than always acting on the triggering event.

Key Changes:

  • Refactored event action handling to use a new handleEventAction function instead of directly calling methods
  • The new function evaluates arguments to determine which event object to act upon
  • Falls back to the original event when no arguments are provided

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

useEventAsDefault: true,
});
const [e] = computedArgs as [Event];
if (e) {
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

The function lacks validation to ensure the computed argument is actually an Event object. If args evaluates to a non-Event value (e.g., a string or number), the code will fail silently when e is truthy but doesn't have the required method, or could throw a runtime error. Consider adding type validation: if (e && typeof e === 'object' && method in e) { e[method](); }

Suggested change
if (e) {
if (e && typeof e === "object" && typeof e[method] === "function") {

Copilot uses AI. Check for mistakes.
@weareoutman weareoutman force-pushed the steve/v3-event-action-target branch from 9fad896 to 351d7dc Compare November 17, 2025 03:19
@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.17%. Comparing base (13468db) to head (351d7dc).
⚠️ Report is 2 commits behind head on v3.

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

Impacted file tree graph

@@            Coverage Diff             @@
##               v3    #4824      +/-   ##
==========================================
- Coverage   95.19%   95.17%   -0.02%     
==========================================
  Files         212      212              
  Lines        9343     9348       +5     
  Branches     1799     1800       +1     
==========================================
+ Hits         8894     8897       +3     
- Misses        330      331       +1     
- Partials      119      120       +1     
Files with missing lines Coverage Δ
packages/runtime/src/internal/bindListeners.ts 90.74% <75.00%> (-0.56%) ⬇️
🚀 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 17, 2025

next-core    Run #11816

Run Properties:  status check passed Passed #11816  •  git commit 4d197b83f4 ℹ️: Merge 351d7dcdf9a4386fac571257aa29457ccd429ca0 into 13468dba4f60a1944c1c44cc275b...
Project next-core
Branch Review steve/v3-event-action-target
Run status status check passed Passed #11816
Run duration 00m 24s
Commit git commit 4d197b83f4 ℹ️: Merge 351d7dcdf9a4386fac571257aa29457ccd429ca0 into 13468dba4f60a1944c1c44cc275b...
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 merged commit 04b2958 into v3 Nov 17, 2025
6 of 8 checks passed
@weareoutman weareoutman deleted the steve/v3-event-action-target branch November 17, 2025 03:31
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