Fix handler registration for old test framework ran with vitest - #960
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRefactors event registration by replacing EventRegister with HandlerRegister, centralizing registrations into a dictionary keyed by contractName/eventName, and updating call sites and generated templates; also removes two ADR documents (Vitest rationale and worker-thread test indexer rationale). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
codegenerator/cli/npm/envio/src/HandlerRegister.res (2)
15-15: Nit:getKeyseparator could theoretically collide.
"A.B" ++ "." ++ "C"and"A" ++ "." ++ "B.C"both produce"A.B.C". Contract and event names in practice don't contain dots, so this is a non-issue today, but a different separator (e.g.,"::"or"\x00") would be unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codegenerator/cli/npm/envio/src/HandlerRegister.res` at line 15, The getKey function currently concatenates contractName and eventName with "." which can collide (e.g., "A.B" + "." + "C" == "A" + "." + "B.C"); update getKey (~contractName, ~eventName) to use an unambiguous separator (for example "::" or "\x00") or a constant SEPARATOR to build the key so contractName ++ SEPARATOR ++ eventName cannot overlap; modify the getKey definition accordingly and ensure any code that parses or compares these keys uses the same separator constant.
13-26: Add a clarifying comment about event handler lifecycle if asymmetry with onBlock handlers is intentional.The global
eventRegistrationsdict persists across registration cycles whileonBlockByChainId(line 69) is recreated fresh on eachstartRegistration()call. This asymmetry works correctly with ESM module caching (handlers register once per process), but if handler modules were ever re-imported (e.g., test frameworks with--forceReimport),setHandlerwould throwDuplicateEventRegistrationbecause the dict entry persists. A brief comment near line 13 explaining whether this difference in lifecycle is intentional would help future readers understand the design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codegenerator/cli/npm/envio/src/HandlerRegister.res` around lines 13 - 26, Add a brief clarifying comment near the top where eventRegistrations is declared explaining that eventRegistrations is intentionally global and persists across registration cycles (unlike onBlockByChainId which is recreated on each startRegistration call), and note the implication that re-importing handler modules (e.g., in tests with forceReimport) can lead setHandler to throw DuplicateEventRegistration; reference eventRegistrations, onBlockByChainId, startRegistration, setHandler, and DuplicateEventRegistration in the comment so future maintainers understand the lifecycle asymmetry and its consequences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@codegenerator/cli/npm/envio/src/HandlerRegister.res`:
- Line 15: The getKey function currently concatenates contractName and eventName
with "." which can collide (e.g., "A.B" + "." + "C" == "A" + "." + "B.C");
update getKey (~contractName, ~eventName) to use an unambiguous separator (for
example "::" or "\x00") or a constant SEPARATOR to build the key so contractName
++ SEPARATOR ++ eventName cannot overlap; modify the getKey definition
accordingly and ensure any code that parses or compares these keys uses the same
separator constant.
- Around line 13-26: Add a brief clarifying comment near the top where
eventRegistrations is declared explaining that eventRegistrations is
intentionally global and persists across registration cycles (unlike
onBlockByChainId which is recreated on each startRegistration call), and note
the implication that re-importing handler modules (e.g., in tests with
forceReimport) can lead setHandler to throw DuplicateEventRegistration;
reference eventRegistrations, onBlockByChainId, startRegistration, setHandler,
and DuplicateEventRegistration in the comment so future maintainers understand
the lifecycle asymmetry and its consequences.
Summary by CodeRabbit
Refactor
Chores