Skip to content

Fix handler registration for old test framework ran with vitest - #960

Merged
DZakh merged 2 commits into
mainfrom
dz/fix-handler-register-for-old-tests-with-vitest
Feb 19, 2026
Merged

Fix handler registration for old test framework ran with vitest#960
DZakh merged 2 commits into
mainfrom
dz/fix-handler-register-for-old-tests-with-vitest

Conversation

@DZakh

@DZakh DZakh commented Feb 19, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Refactor

    • Event registration reworked to use centralized storage and explicit contract/event naming for registrations.
    • Generated templates and runtime paths updated to use the new registration model.
  • Chores

    • Removed archived architecture decision documents describing prior test/worker choices.

@DZakh
DZakh requested a review from JonoPrest February 19, 2026 09:24
@coderabbitai

coderabbitai Bot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉


📝 Walkthrough

Walkthrough

Refactors 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

Cohort / File(s) Summary
Documentation
adr/why-vitest.md, adr/why-workers-for-test-indexer.md
Removed ADRs documenting the choice of Vitest and the decision to use worker threads for test indexer isolation.
HandlerRegister Module
codegenerator/cli/npm/envio/src/HandlerRegister.res, codegenerator/cli/npm/envio/src/HandlerRegister.resi
Reworked registration storage from per-record mutable fields to a centralized map keyed by contractName/eventName; removed public t record and make; API functions now accept ~contractName and ~eventName parameters and expose new get/set helpers and eventRegistration type.
Call-site type updates
codegenerator/cli/npm/envio/src/Ctx.res, codegenerator/cli/npm/envio/src/ChainFetcher.res, codegenerator/cli/npm/envio/src/EventRegister.gen.ts
Switched registrations field type from EventRegister.registrations to HandlerRegister.registrations; deleted generated EventRegister.gen.ts.
Handler loading
codegenerator/cli/npm/envio/src/HandlerLoader.res
Replaced calls to EventRegister.startRegistration/finishRegistration with HandlerRegister.startRegistration/finishRegistration.
Templates & codegen
codegenerator/cli/src/hbs_templating/codegen_templates.rs, codegenerator/cli/templates/dynamic/codegen/src/Indexer.res.hbs, codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs
Updated generated templates and code strings to reference HandlerRegister instead of EventRegister; changed Event public type to use contractName and name fields instead of a nested handlerRegister; updated mock DB registration lifecycle checks to use HandlerRegister APIs.
Other small edits
codegenerator/cli/npm/envio/src/ChainFetcher.res, codegenerator/cli/npm/envio/src/Ctx.res
Adjusted function/type signatures to reference HandlerRegister.registrations where previously EventRegister.registrations was used.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • JonoPrest
  • moose-code

Poem

🐰 I hopped through code with nimble feet,
Replaced scattered registers with one neat sheet,
Contracts and names in a tidy row,
Handlers find homes where mappings grow.
Hooray—organized hops, a refactor treat! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: migrating handler registration from EventRegister to HandlerRegister to fix compatibility with the old test framework running under vitest.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dz/fix-handler-register-for-old-tests-with-vitest

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
codegenerator/cli/npm/envio/src/HandlerRegister.res (2)

15-15: Nit: getKey separator 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 eventRegistrations dict persists across registration cycles while onBlockByChainId (line 69) is recreated fresh on each startRegistration() 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), setHandler would throw DuplicateEventRegistration because 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.

@DZakh
DZakh enabled auto-merge (squash) February 19, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants