Use project's installed envio binary in e2e tests - #1157
Conversation
After #1116 moved the CLI into in-process bin.mjs, running envio via the e2e-tests workspace's bin.mjs while the handler imports `generated` → `envio` resolved to the project's freshly installed envio produced two distinct envio module instances in one process. HandlerRegister state lives per-instance, so `indexer.onEvent` wrote to the project's dict while `applyRegistrations` read the e2e-tests dict, causing every event to surface as "no event handler" and chain 1 to fail with "Nothing to fetch". Invoke envio through `<projectDir>/node_modules/envio/bin.mjs` so bin.mjs and the handlers resolve to the same physical envio, matching the real `npx envio dev` flow. https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces module-local handler registration state with a shared process-wide registry on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
The NAPI migration (#1116) is merged to main, so the stopgap `build_and_verify_pr.yml` (which ran on `pull_request` so PR-modified build steps were exercised) is no longer needed. `build_and_verify.yml` already covers PRs via `pull_request_target`. https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/e2e-tests/src/dependency-tests/install.test.ts (2)
203-214: Minor: guardenvio stopagainst a pre-assignmentbeforeAllfailure.If
beforeAllthrows before line 170 (e.g.,pm installrejects, or the base-project copy fails),projectEnvioCommand/projectEnvioArgs/projectDirareundefinedwhenafterAllruns. The.catch(() => {})swallows the resulting rejection, but it’s cleaner — and less confusing in logs — to skip the stop call entirely when it was never started.🛡️ Proposed guard
- // docker compose down -v (removes containers + volumes for clean next run) - await runCommand(projectEnvioCommand, [...projectEnvioArgs, "stop"], { - cwd: projectDir, - timeout: 30_000, - }).catch(() => {}); + // docker compose down -v (removes containers + volumes for clean next run) + if (projectEnvioCommand && projectDir) { + await runCommand(projectEnvioCommand, [...projectEnvioArgs, "stop"], { + cwd: projectDir, + timeout: 30_000, + }).catch(() => {}); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/e2e-tests/src/dependency-tests/install.test.ts` around lines 203 - 214, The afterAll cleanup calls runCommand(projectEnvioCommand, [...projectEnvioArgs, "stop"], { cwd: projectDir }) even when beforeAll may have failed and those variables are undefined; update the afterAll block (the existing afterAll function and its use of projectEnvioCommand, projectEnvioArgs, projectDir) to skip the envio stop call when the env vars weren't initialized — either check that projectEnvioCommand, projectEnvioArgs, and projectDir are all truthy before calling runCommand, or use a boolean flag set in beforeAll (e.g., envioStarted) and only run the stop when that flag is true; keep the existing catch for safety.
170-171: Consider resolvingbin.mjsviapackage.json’sbinfield rather than hardcoding.
config.ts::resolveEnviodeliberately readspkg.binfromenvio/package.json(seepackages/e2e-tests/src/config.ts:36-74) so the entry point isn’t tied to a specific filename. Here the path is hardcoded tonode_modules/envio/bin.mjs, which will silently break if the package ever renames or relocates its bin. Not critical today (the file does exist), but mirroringresolveEnvio— scoped toprojectDir’snode_modules— keeps the two code paths consistent.♻️ Sketch
- projectEnvioCommand = "node"; - projectEnvioArgs = [path.join(projectDir, "node_modules", "envio", "bin.mjs")]; + const projectRequire = createRequire(path.join(projectDir, "package.json")); + const pkgJsonPath = projectRequire.resolve("envio/package.json"); + const pkg = JSON.parse(fs.readFileSync(pkgJsonPath, "utf-8")); + const binRel = typeof pkg.bin === "string" ? pkg.bin : pkg.bin?.envio; + projectEnvioCommand = "node"; + projectEnvioArgs = [path.resolve(path.dirname(pkgJsonPath), binRel)];(Requires
import { createRequire } from "node:module";.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/e2e-tests/src/dependency-tests/install.test.ts` around lines 170 - 171, The test hardcodes the envio CLI path in projectEnvioArgs to "node_modules/envio/bin.mjs", which can break if the package's bin changes; instead resolve the bin via the package.json like resolveEnvio in packages/e2e-tests/src/config.ts: use Node's module resolution (e.g., createRequire) scoped to projectDir to require("envio/package.json") and read pkg.bin to compute the actual entry point, then set projectEnvioArgs to that resolved path so the test mirrors resolveEnvio and avoids a brittle hardcoded filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/e2e-tests/src/dependency-tests/install.test.ts`:
- Around line 203-214: The afterAll cleanup calls
runCommand(projectEnvioCommand, [...projectEnvioArgs, "stop"], { cwd: projectDir
}) even when beforeAll may have failed and those variables are undefined; update
the afterAll block (the existing afterAll function and its use of
projectEnvioCommand, projectEnvioArgs, projectDir) to skip the envio stop call
when the env vars weren't initialized — either check that projectEnvioCommand,
projectEnvioArgs, and projectDir are all truthy before calling runCommand, or
use a boolean flag set in beforeAll (e.g., envioStarted) and only run the stop
when that flag is true; keep the existing catch for safety.
- Around line 170-171: The test hardcodes the envio CLI path in projectEnvioArgs
to "node_modules/envio/bin.mjs", which can break if the package's bin changes;
instead resolve the bin via the package.json like resolveEnvio in
packages/e2e-tests/src/config.ts: use Node's module resolution (e.g.,
createRequire) scoped to projectDir to require("envio/package.json") and read
pkg.bin to compute the actual entry point, then set projectEnvioArgs to that
resolved path so the test mirrors resolveEnvio and avoids a brittle hardcoded
filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7971eb5-513f-4784-87b8-664396792799
📒 Files selected for processing (2)
.github/workflows/build_and_verify_pr.ymlpackages/e2e-tests/src/dependency-tests/install.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/build_and_verify_pr.yml
When the CLI's bin.mjs resolves envio from one path (e.g. a globally installed or test-resolved copy) and the user's handlers resolve `import "envio"` from a different path (e.g. their project's node_modules), Node treats them as two separate ESM module instances. HandlerRegister's module-local state then lives per-instance: the handler's `indexer.onEvent` writes to one dict while `HandlerLoader.applyRegistrations` reads the other, so every event ends up "without an event handler" and chains with no other registrations fail with "Nothing to fetch". Stash `eventRegistrations`, `activeRegistration`, and `preRegistered` on `globalThis` so all envio instances in a single process share one registry. This is the same trick `react` and `@apollo/client` use to guard against the "two copies of the package" footgun. Revert the install.test.ts workaround that routed `envio dev` through the project-local bin.mjs — with the globalThis fix, the original `config.envioCommand` invocation works. https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/envio/src/HandlerRegister.res (1)
13-56:⚠️ Potential issue | 🟠 MajorVersion-skew risk: shared
globalThisslots assume a single registry schema.If two different envio versions end up in the same process (monorepo with divergent pinned versions, a transitive dep that ships its own copy, or an in-flight upgrade), both will hit the same unversioned keys (
__envioHandlerRegistrations,__envioActiveRegistration,__envioPreRegistered) and reuse whichever shape landed first. Any evolution ofeventRegistration,activeRegistration, orInternal.onBlockConfigbetween versions then produces silent cross-version reads/writes — this is exactly the failure mode React/Apollo guard against with internal version checks, and the PR description cites them as precedent but doesn't replicate the guard.Consider namespacing by package version (or stamping a
__envioRegistryVersionand bailing/warning on mismatch):🛡️ Suggested guard (sketch)
-let eventRegistrations: dict< - eventRegistration, -> = %raw(`globalThis.__envioHandlerRegistrations ??= {}`) +// Bump when any of the registry record shapes change. +let registryVersion = "1" +let eventRegistrations: dict<eventRegistration> = %raw(` + (globalThis.__envioRegistry ??= { version: "${registryVersion}" }), + (globalThis.__envioRegistry.version !== "${registryVersion}" + ? (() => { throw new Error("Incompatible envio HandlerRegister versions loaded in the same process (" + globalThis.__envioRegistry.version + " vs ${registryVersion}). Deduplicate the 'envio' dependency."); })() + : (globalThis.__envioRegistry.handlerRegistrations ??= {})) +`)(Apply the same pattern to
activeRegistrationandpreRegistered.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 13 - 56, The globalThis slots (__envioHandlerRegistrations, __envioActiveRegistration, __envioPreRegistered) are unversioned and can cause silent cross-version corruption; update the initialization to namespace or guard by the package version: compute a versioned key (e.g., `__envioRegistry_v${VERSION}_HandlerRegistrations`) or store a `__envioRegistryVersion` alongside the stored object and detect mismatches, and if versions differ either create a fresh registry or log/warn and bail; apply this change for eventRegistrations (symbol: eventRegistrations / getKey), activeRegistration (symbol: activeRegistration) and preRegistered (symbol: preRegistered) so reads/writes only use a matching schema and avoid silent cross-version reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 17-19: The global dict eventRegistrations and
activeRegistration.contents persist across module reloads causing stale handlers
and persistent "indexer finished initializing" errors; modify startRegistration
(or add a reset() called at start) to clear eventRegistrations (reset to an
empty dict) and set activeRegistration.contents = None before beginning a new
registration run so subsequent calls to
registerAllHandlers/finishRegistration/withRegistration/throwIfFinishedRegistration
operate on a fresh state. Ensure the reset happens early in startRegistration
and is also usable from tests if needed.
---
Outside diff comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 13-56: The globalThis slots (__envioHandlerRegistrations,
__envioActiveRegistration, __envioPreRegistered) are unversioned and can cause
silent cross-version corruption; update the initialization to namespace or guard
by the package version: compute a versioned key (e.g.,
`__envioRegistry_v${VERSION}_HandlerRegistrations`) or store a
`__envioRegistryVersion` alongside the stored object and detect mismatches, and
if versions differ either create a fresh registry or log/warn and bail; apply
this change for eventRegistrations (symbol: eventRegistrations / getKey),
activeRegistration (symbol: activeRegistration) and preRegistered (symbol:
preRegistered) so reads/writes only use a matching schema and avoid silent
cross-version reads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87d89d5f-71af-489f-8dca-c0c128b3dd58
📒 Files selected for processing (1)
packages/envio/src/HandlerRegister.res
Stashing the registry on unversioned globalThis keys would silently blend state if two envio majors ended up in one process (monorepo with divergent pins, a transitive dep shipping its own copy, an in-flight upgrade). The record shapes evolve between majors, so silent sharing would corrupt handlers/config reads later with opaque errors. Collapse the three globalThis slots into a single \`__envioRegistry\` object tagged with the envio package version. On load, if a registry already exists we accept it only when versions match; otherwise we throw with a deduplication hint pointing at the real problem (\`node_modules/envio\` duplication) rather than a random decode failure. https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/envio/src/HandlerRegister.res (1)
102-132:⚠️ Potential issue | 🟡 MinorStale state now survives across registration cycles in the same process.
With
eventRegistrations,activeRegistration, andpreRegisteredanchored onglobalThis, a secondstartRegistrationcall in the same process (tests spanning multiple indexer runs, watchers, REPL re-imports) inherits every prior entry ineventRegistrations, andfinishRegistrationnever resetsactiveRegistration.contentsso a subsequentwithRegistrationpath can still observe a finished registration. The primary CLI-spawned flow is unaffected, but multi-run-in-process scenarios will see stale handlers and spurious "indexer finished initializing" throws.♻️ Proposed reset at the start of a new run
let startRegistration = (~ecosystem, ~multichain) => { + // Clear any leftover state from a prior registration cycle in this process. + eventRegistrations + ->Dict.keysToArray + ->Array.forEach(k => eventRegistrations->Utils.Dict.deleteInPlace(k)) let r = { ecosystem, multichain, registrations: { onBlockByChainId: Dict.make(), }, finished: false, } activeRegistration.contents = Some(r)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 102 - 132, startRegistration currently leaves global state from prior runs intact and finishRegistration never clears activeRegistration, causing stale handlers to persist; fix by explicitly resetting the global registration state at the start of startRegistration (clear/recreate eventRegistrations, empty preRegistered array) before creating the new registration object, and update finishRegistration to set activeRegistration.contents = None after marking r.finished (so subsequent withRegistration/startRegistration won't see a finished registration); refer to startRegistration, finishRegistration, activeRegistration, eventRegistrations, and preRegistered when applying the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 102-132: startRegistration currently leaves global state from
prior runs intact and finishRegistration never clears activeRegistration,
causing stale handlers to persist; fix by explicitly resetting the global
registration state at the start of startRegistration (clear/recreate
eventRegistrations, empty preRegistered array) before creating the new
registration object, and update finishRegistration to set
activeRegistration.contents = None after marking r.finished (so subsequent
withRegistration/startRegistration won't see a finished registration); refer to
startRegistration, finishRegistration, activeRegistration, eventRegistrations,
and preRegistered when applying the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78077c10-9f96-41ed-81d0-e56cd27f122b
📒 Files selected for processing (1)
packages/envio/src/HandlerRegister.res
Drop the %raw IIFE that boxed the globalThis lookup + version check in a
JS string, and replace it with a typed ReScript match. `globalThis` is
now bound via `@val external` with an open `{..}` type so bracket access
(\`globalThis["__envioRegistry"]\`) stays syntactically light; the slot's
real shape is encoded as a typed record (\`registryShape\`). The version
check becomes a plain pattern match with a guard.
Behavior unchanged; generated JS is equivalent. Payoff is that the
string-embedded JS no longer shadows types, and future edits (e.g.
adding a field to the registry) are caught by the compiler.
https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/envio/src/HandlerRegister.res (1)
13-63: Typed, version-gated global registry looks correct.The replacement of the prior raw-JS IIFE with a typed record plus a pattern match with a version guard is a clean win:
registryShapemakes future schema bumps compiler-checked, and throwing on version mismatch (rather than silently reusing slots) is the safe choice given the duplicate-module scenario this PR targets. Aliases at lines 63/78/87 correctly preserve the existing mutation contract (same dict, same ref, same array).One optional refinement: the
@val external globalThis: {..}open-object type forces bracket access for both the read on line 43 and the write on line 57. You can tighten this a bit and drop the implicit trust in the field shape:🧹 Optional: narrower external for `__envioRegistry`
-@val external globalThis: {..} = "globalThis" +@val +external globalThis: {"__envioRegistry": Nullable.t<registryShape>} = "globalThis"With this,
globalThis["__envioRegistry"]is alreadyNullable.t<registryShape>without the local annotation on line 43, and the assignment on line 57 is still permitted via the object field.Not a blocker — purely a readability/type-precision nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 13 - 63, Tighten the external globalThis type so the __envioRegistry slot is typed instead of using an open object: change the declaration of globalThis (currently `@val` external globalThis: {..} = "globalThis") to include a "__envioRegistry": Nullable.t<registryShape> field (e.g. `@val` external globalThis: {. "__envioRegistry": Nullable.t<registryShape> } = "globalThis"), then remove the local type annotation on the existing lookup (the `existing: Nullable.t<registryShape>` local) and read/write the slot via the typed field (globalThis.__envioRegistry or globalThis["__envioRegistry"]) so the registry access in the registry initialization (the code that assigns to and reads from __envioRegistry, and the registryShape usage) is statically typed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 13-63: Tighten the external globalThis type so the __envioRegistry
slot is typed instead of using an open object: change the declaration of
globalThis (currently `@val` external globalThis: {..} = "globalThis") to include
a "__envioRegistry": Nullable.t<registryShape> field (e.g. `@val` external
globalThis: {. "__envioRegistry": Nullable.t<registryShape> } = "globalThis"),
then remove the local type annotation on the existing lookup (the `existing:
Nullable.t<registryShape>` local) and read/write the slot via the typed field
(globalThis.__envioRegistry or globalThis["__envioRegistry"]) so the registry
access in the registry initialization (the code that assigns to and reads from
__envioRegistry, and the registryShape usage) is statically typed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 167d2f95-50d8-4fee-b979-2da67539757e
📒 Files selected for processing (1)
packages/envio/src/HandlerRegister.res
Replace \`{..}\` with a \`mutable __envioRegistry\` record so both the
lookup and the assignment are statically typed, and drop the local
\`Nullable.t<registryShape>\` annotation. Typo in the slot name now
surfaces at compile time instead of silently creating a new globalThis
property.
https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/envio/src/HandlerRegister.res (1)
22-64: Global registry setup looks right; one small comment/behavior nit.The version-gated
globalThis.__envioRegistryapproach cleanly addresses the dual-instance problem described in the PR:eventRegistrations,activeRegistration, andpreRegisteredremain reference-shared across any duplicate envio copy of the same version, and incompatible copies fail loudly with an actionable deduplication hint rather than silently splitting state.One small nit: the rationale comment on lines 27–30 says the shapes "evolve between envio majors," but the guard on line 48 uses strict full-version equality (
existing.version === version), so any patch/pre-release mismatch also throws. The stricter behavior is arguably the safer default, but it would be worth reconciling the comment with the implementation (either soften the comment to "between envio versions" or relax the check to compare majors only) so future readers don't assume major-only matching is in effect.✏️ Optional wording tweak to match the implementation
// Version-gated: the record shapes below evolve between envio majors, so // sharing unversioned slots would corrupt state across incompatible // versions. On mismatch we throw with a deduplication hint instead of // silently mixing shapes. +// Version-gated: the record shapes below can evolve between envio versions, +// so sharing unversioned slots would risk corrupting state across +// incompatible builds. We compare the full version string and throw with a +// deduplication hint on any mismatch rather than silently mixing shapes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 22 - 64, The comment says "evolve between envio majors" but the code uses strict equality at the registry init (the equality check existing.version === version inside the private registry binding that reads globalThis.__envioRegistry using Utils.EnvioPackage.value.version), so reconcile them: either update the comment to say "between envio versions" to match the strict comparison, or change the guard to compare majors only (parse existing.version and Utils.EnvioPackage.value.version and allow match when major segments are equal) before creating/throwing during registry initialization; adjust the check around existing.version === version in the registry setup accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 22-64: The comment says "evolve between envio majors" but the code
uses strict equality at the registry init (the equality check existing.version
=== version inside the private registry binding that reads
globalThis.__envioRegistry using Utils.EnvioPackage.value.version), so reconcile
them: either update the comment to say "between envio versions" to match the
strict comparison, or change the guard to compare majors only (parse
existing.version and Utils.EnvioPackage.value.version and allow match when major
segments are equal) before creating/throwing during registry initialization;
adjust the check around existing.version === version in the registry setup
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f314e3d9-c2d1-4b72-a688-02aeb7f2fd1f
📒 Files selected for processing (1)
packages/envio/src/HandlerRegister.res
Comment said shapes "evolve between envio majors" but the guard is full-version equality, so a patch/pre-release mismatch also throws. Keep the stricter check (safer default) and reword the comment so a future reader doesn't assume major-only matching. https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
2 participants
Summary
Modified the isolated dependency e2e tests to invoke envio via the project's installed binary instead of the e2e-tests' envio command. This ensures that
bin.mjsand user handlers load the same envio module instance.Key Changes
projectEnvioCommandandprojectEnvioArgsvariables to reference the project's installed envio binary (node_modules/envio/bin.mjs)envio devcommand invocation to use the project's binary instead ofconfig.envioCommandenvio stopcommand invocation to use the project's binary instead ofconfig.envioCommandImplementation Details
The previous approach of using
config.envioCommand(the e2e-tests envio) would create two separate copies of the envio module in the same process. This caused handlers to appear unregistered becauseHandlerRegister.setHandlerwould write to one module instance's dictionary whileapplyRegistrationswould read from the other. By using the project's installed binary, both the handlers and the indexer load from the same envio module instance, ensuring proper handler registration.https://claude.ai/code/session_011rsp7McDiNshdkTLCnP6eT
Summary by CodeRabbit
Chores
Refactor