fix(core): invoke structured Signal listeners with runtime args before bound args

[cptbtptpbcptdtptp]

Summary

• Flip structured-binding listener invocation order from method(...boundArgs, ...signalArgs) to method(...signalArgs, ...boundArgs), so the event object always sits at index 0.
• Aligns with DOM (event, customData) convention.

Background

Cherry-picked from b16d8b5d7 on fix/shaderlab.

Test plan

• 3 new Signal.test.ts cases: arg ordering, event index 0 invariant, once() parity
• All 29 Signal tests pass
• CI green

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary by CodeRabbit

• Bug Fixes

  • Fixed Signal structured binding invocation order so runtime signal arguments are passed before pre-resolved bound arguments. The event object continues to be the first argument, restoring consistent handler behavior.

• Tests

  • Expanded Signal tests to verify runtime vs bound argument ordering for both persistent and single-use listeners.

• Chores

  • Updated related test handler signatures and formatting to align with structured binding argument expectations.

[coderabbitai]

Walkthrough

Signal's structured binding listener invocation now passes runtime signal arguments before pre-resolved bound arguments. The implementation is updated in the listener wrapper, documented in JSDoc for both on and once overloads, validated with new test cases that verify argument order preservation, and test handler signatures across multiple test files are realigned to accept the new parameter order.

Changes

Signal Argument Ordering for Structured Binding

Layer / File(s)	Summary
Core Implementation packages/core/src/Signal.ts	Listener wrapper for structured binding now calls target method as target[methodName](...signalArgs, ...args) instead of target[methodName](...args, ...signalArgs).
Documentation packages/core/src/Signal.ts	JSDoc for both on and once structured binding overloads now document that runtime signal arguments are passed first and bound arguments are appended.
Test Infrastructure tests/src/core/Signal.test.ts	TestHandler gains lastArgs field to record received arguments and handleWithArgs(...args) method to support argument ordering validation.
Test Validation tests/src/core/Signal.test.ts	New test cases verify runtime args precede bound args, event object stays at index 0 regardless of bound-arg count, and once preserves argument order for both invocation count and captured arguments.
Handler Signature Alignment tests/src/core/CloneUtils.test.ts, tests/src/ui/UIInteractive.test.ts	Test handlers are updated to accept leading runtime argument parameters (numeric or PointerEventData) before existing parameters, matching the new Signal structured binding invocation order. Import statements are also reformatted and minor punctuation is added.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Arguments waltz in perfect line,
Runtime first, then bound ones shine!
Signal speaks with ordered care,
Handlers listen everywhere! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the primary change: updating Signal listener invocation to pass runtime arguments before bound arguments, which is the core semantic change across all modified files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

tests/src/ui/UIInteractive.test.ts (1)

16-28: ⚠️ Potential issue | 🔴 Critical

Update handleClick() to accept PointerEventData parameter.

The handleClick() method is missing the PointerEventData parameter that was added to handleClickWithPrefix(). Both methods are used as Signal handlers and should follow the same argument ordering. Update handleClick() to:

handleClick(event: PointerEventData) {
  this.callCount++;
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/ui/UIInteractive.test.ts` around lines 16 - 28, The handleClick()
method in the ClickHandler class is missing the PointerEventData parameter that
is required for Signal handlers. Update the handleClick() method signature to
include the event parameter of type PointerEventData as the first parameter,
matching the signature pattern used in handleClickWithPrefix(). This ensures
both methods can be properly used as Signal handlers with consistent argument
ordering.

tests/src/core/CloneUtils.test.ts (1)

95-107: ⚠️ Potential issue | 🔴 Critical

Update handleClick() to accept the Signal data parameter.

The handleClick() method should receive the number argument emitted by the Signal, matching the structured binding convention where method(...signalArgs, ...args) places runtime signal arguments first. Change:

handleClick(): void {
  this.callCount++;
}

to:

handleClick(arg: number): void {
  this.callCount++;
}

The handleClickWithPrefix() method is already correct. No other handler methods exist in this test file.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/core/CloneUtils.test.ts` around lines 95 - 107, The handleClick()
method in the ClickHandler class does not accept the Signal data parameter that
is being emitted, while handleClickWithPrefix() correctly accepts parameters.
Update the handleClick() method signature to accept the arg: number parameter as
the first parameter, matching the signal binding convention where runtime signal
arguments are passed before custom arguments.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 95-107: The handleClick() method in the ClickHandler class does
not accept the Signal data parameter that is being emitted, while
handleClickWithPrefix() correctly accepts parameters. Update the handleClick()
method signature to accept the arg: number parameter as the first parameter,
matching the signal binding convention where runtime signal arguments are passed
before custom arguments.

In `@tests/src/ui/UIInteractive.test.ts`:
- Around line 16-28: The handleClick() method in the ClickHandler class is
missing the PointerEventData parameter that is required for Signal handlers.
Update the handleClick() method signature to include the event parameter of type
PointerEventData as the first parameter, matching the signature pattern used in
handleClickWithPrefix(). This ensures both methods can be properly used as
Signal handlers with consistent argument ordering.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: fb3b2753-084b-4402-a3a6-c19939f1c0b2

📥 Commits

Reviewing files that changed from the base of the PR and between eb8ac58 and 74800f2.

📒 Files selected for processing (2)

• tests/src/core/CloneUtils.test.ts
• tests/src/ui/UIInteractive.test.ts

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.89%. Comparing base (1bc2b10) to head (74800f2).
⚠️ Report is 28 commits behind head on dev/2.0.

Additional details and impacted files

@@             Coverage Diff             @@
##           dev/2.0    #2987      +/-   ##
===========================================
+ Coverage    78.25%   79.89%   +1.64%     
===========================================
  Files          900      919      +19     
  Lines        99234   102586    +3352     
  Branches     10172    11348    +1176     
===========================================
+ Hits         77657    81966    +4309     
+ Misses       21406    20438     -968     
- Partials       171      182      +11     

Flag	Coverage Δ
unittests	79.89% <100.00%> (+1.64%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

[GuoLei1990]

已关闭问题清单

历史 24+ 轮 review 的问题均已关闭：

• 参数顺序正确性（method(...boundArgs, ...signalArgs) → method(...signalArgs, ...boundArgs)）— 多轮验证正确，关闭。
• call-site 影响分析（closure vs structured binding） — 已确认引擎内无 listener 依赖旧顺序，关闭（详见下方复核）。
• breaking change 需记入 CHANGELOG — 历轮反复提出，最终结论为非阻塞 P2，按既定结论不再重提。
• fixture 对齐（commit 74800f28） — 已审，根因正确，关闭。

总结

本轮 HEAD 仍为 74800f28，与上轮审查的 commit 完全一致，无新 delta——稳态复审。核心修复（method(...signalArgs, ...args)，让 event 对象固定在 index 0，对齐 DOM addEventListener / Cocos (event, customData) 约定）方向正确，实现最小化。

第一性原理复核（独立于历轮共识，基于本次读源码）

• 方向正确性 — 确认。git log -S 追溯显示旧顺序 (...args, ...signalArgs) 是 PR #2921（新建 Signal.ts）引入的初始设计选择，本 PR 不是回退某个 fix: commit，而是修正一个从未正确过的顺序。event/signal 系统里回调第一参恒为事件本身，是本质正确的约定；JSDoc 已显式写明 method(...signalArgs, ...args)，契约自洽。
• 向后兼容 — 4 个生产 Signal 实例（Button.onClick / XRSessionManager.onStateChanged / XRInputManager.onTrackedDeviceChanged / XRTrackableFeature.onChanged）全部走 closure 形式（传函数，不传 component+methodName），参数顺序翻转对它们零影响。唯一的 structured-binding 生产调用点是 ReflectionParser.ts:181（反序列化 scene 数据时透传 signal.on(targetComponent, methodName, ...resolvedArgs)），本身不含 handler 逻辑，不会"坏"。
• 回归测试反向证伪 — 有效。新测试 "runtime args precede bound args" 期望 ["event", 42, "boundA", "boundB"]，把 fix revert 回 (...args, ...signalArgs) → 变成 ["boundA", "boundB", "event", 42] fail；once 测试期望 [99, "bound"] → 变成 ["bound", 99] fail。测试通过 invoke() 触发 + 断言 lastArgs 走公开链路，不戳私有字段，符合规范。
• fixture 修复落点 — commit 74800f28 把 CloneUtils.test.ts / UIInteractive.test.ts 的 handleClickWithPrefix 从单参 (prefix) 补成 (arg, prefix) / (event, prefix)，是补全首个 fix commit 漏改的部分，prefix 重新解析到绑定值；Signal.test.ts 同名 fixture 因两处调用均零 runtime arg invoke 而无需改，改与不改的边界划得准。
• 注释合规 — 新增 JSDoc 多行块结尾带句号、@param 用破折号不带类型不带句号，均合规。

问题

无新问题。

关于 CodeRabbit 历轮的 outside-diff 🔴 Critical（要求给 handleClick() 补 event/arg 形参）——误报，不需要改：handleClick() 方法体不消费 runtime 参数（只 callCount++），JS 对多余实参直接丢弃、TS 不要求声明未使用的形参，相关用例只断言 callCount，行为正确。补形参纯属把误报落成噪声改动，不建议执行。

LGTM，可合入。