Migrate to ReScript 12 new build system#1155
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughReplaces legacy ReScript tooling and schema generation: switches CLI/template invocations from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/e2e-tests/src/dependency-tests/install.test.ts (1)
80-99:⚠️ Potential issue | 🟡 MinorMinor comment indentation issue at line 81.
The comment at line 81 has an extra leading space (
//should be//) compared to adjacent comment lines 80 and 82. This is a minor formatting inconsistency in a comment block.Note: The handler auto-load mechanism is sound—handlers are loaded via dynamic import after persistence initialization (Main.res:644), with proper error reporting that surfaces underlying causes. The test scenario validates this end-to-end.
🤖 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 80 - 99, Fix the minor comment indentation inconsistency in the codegen comment block: locate the comment before the codegenResult assertion (the line starting with the `// rescript config land in generated/` text near the `runCommand(config.envioCommand, [...config.envioArgs, "codegen"], { cwd: baseProjectDir, ... })` call) and align its leading spaces to match the surrounding comment lines (change the extra leading space so the line uses the same indentation as the adjacent `//` comment lines)..github/workflows/publish.yml (1)
96-96:⚠️ Potential issue | 🟠 MajorPlatform packages always publish with
--tag latest, even for prerelease versions.The base
enviopackage correctly derivesNPM_TAGfrom the version (nextfor semver/-alpha/-rc,devotherwise) and publishes with--tag "$NPM_TAG". However, each platform package (envio-linux-x64, etc.) unconditionally uses--tag latest. This means publishing av1.2.3-alpha.1or anydev-tagged release will advance thelatestdist-tag on the platform packages — and because the baseenviopackage references them viaoptionalDependenciespinned to the exact version, existinglatestinstalls ofenviowill start resolving platform deps against an unreleased/prerelease binary metadata chain. Consider using$NPM_TAGfor platform packages as well.🛡️ Proposed fix
env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} VERSION: ${{ needs.prepare.outputs.version }} + NPM_TAG: ${{ needs.prepare.outputs.npm_tag }} run: | if [ ! -f envio.node ]; then echo "ERROR: envio.node not found in platform package" exit 1 fi node -e " const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf-8')); pkg.version = process.env.VERSION; fs.writeFileSync('package.json', JSON.stringify(pkg, null, 2) + '\n'); " echo "Publishing $(node -p "require('./package.json').name")@${VERSION}" - npm publish --access public --tag latest + npm publish --access public --tag "$NPM_TAG"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml at line 96, Platform packages are being published with a hardcoded --tag latest; change their npm publish invocations to use the computed NPM_TAG so prerelease/dev versions don't overwrite latest. Locate the npm publish command(s) that currently call npm publish --access public --tag latest (the platform package publish step) and replace --tag latest with --tag "$NPM_TAG" (ensuring the NPM_TAG environment variable is already set earlier in the workflow). Keep the rest of the publish flags intact so all platform packages use the same tag logic as the base package.scenarios/test_codegen/test/lib_tests/PgStorage_test.res (1)
951-962:⚠️ Potential issue | 🟡 MinorTautological assertion.
t.expect(true, ...).toBe(true)can never fail and provides no coverage — the only signal is thatmakeStorageFromEnvdidn't throw. Assert on the returned storage value (or at least bind it and assert it's non-null) so a regression that starts throwing whenclickhouse=falseis actually caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/lib_tests/PgStorage_test.res` around lines 951 - 962, The test currently uses a tautological assertion; instead capture the result of PgStorage.makeStorageFromEnv(~config) and assert on it — e.g., bind let storage = PgStorage.makeStorageFromEnv(~config) and replace t.expect(true...).toBe(true) with an assertion that storage is not null/undefined or matches the expected type/shape (for example t.expect(storage, ~message="Expected storage to be created").toBeDefined() or an instance/type check). This ensures the test fails if makeStorageFromEnv throws or returns an invalid value.packages/cli/Cargo.toml (1)
43-47:⚠️ Potential issue | 🟡 MinorVerify that cargo check passes with sqlx 0.8 upgrade.
The codebase uses
#[derive(sqlx::FromRow, sqlx::Type)](found inpackages/cli/src/persisted_state/hash_string.rs). In sqlx 0.8,#[derive(Type)]now automatically generatesPgHasArrayTypeimplementations where it previously did not. This can cause duplicate impl errors only if manualPgHasArrayTypeimplementations exist for those types—none were found in the codebase, so the primary risk here is lower than initially flagged. However, confirm thatcargo check --all-features --releasecompiles cleanly with the 0.8 upgrade. If compile errors appear related toPgHasArrayType, apply#[sqlx(no_pg_array)]to the affected derives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/Cargo.toml` around lines 43 - 47, Run a full compile check with the new sqlx = "0.8" dependency (cargo check --all-features --release) to verify no duplicate PgHasArrayType impl errors occur; if you hit compile errors for types derived with sqlx::Type (e.g., the derives in packages/cli/src/persisted_state/hash_string.rs), add the attribute #[sqlx(no_pg_array)] to the affected #[derive(sqlx::FromRow, sqlx::Type)] to suppress automatic PgHasArrayType generation and then re-run the cargo check until it compiles cleanly.packages/build-envio/src/build-artifact.ts (1)
93-102:⚠️ Potential issue | 🟡 Minor
./node_modules/.bin/rescriptis Unix-only.On Windows, the bin shim is
rescript.cmd/rescript.ps1— executing./node_modules/.bin/rescriptdirectly viaexecSyncwon't work there. If this script is only ever run in CI/Linux build containers this is fine; otherwise consider resolving through Node (require.resolve("rescript/...")) or invoking via the appropriate shell, or reinstate a pnpm-based invocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build-envio/src/build-artifact.ts` around lines 93 - 102, The compileRescript function currently calls execSync("./node_modules/.bin/rescript", { cwd: envioDir, ... }) which fails on Windows because the bin shim is rescript.cmd/rescript.ps1; update compileRescript to resolve and invoke the platform-appropriate binary: compute the bin path from envioDir/node_modules/.bin and choose "rescript" on POSIX or "rescript.cmd" (or "rescript.ps1" if preferred) on Windows (use process.platform === "win32"), then call execSync with that full path and the same options (cwd: envioDir, stdio: "inherit"); alternatively you can resolve the package via require.resolve or use a cross-platform invocation like "npx rescript" — reference symbols: compileRescript, execSync, envioDir.packages/cli/src/hbs_templating/codegen_templates.rs (1)
1548-1568:⚠️ Potential issue | 🟠 MajorUse explicit type annotations and typed intermediate for
Utils.magiccasting instead of bracket notation.The current template emits
(constructor->Utils.magic)["event"]["_0"]which violates ReScript guidelines. Cast to an explicitly typed intermediate value once, then access fields safely. Example pattern:let typed = (constructor->(Utils.magic: simulateItemConstructor<'event, 'paramsConstructor, 'where> => {event: {...}, params: ..., ...})) event: typed.event,This ensures type safety and readability per the guideline: always use explicit type annotations
value->(Utils.magic: inputType => outputType)and avoid bracket-based field access when types are known.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/hbs_templating/codegen_templates.rs` around lines 1548 - 1568, The template currently emits unsafe bracket-access like (constructor->Utils.magic)["event"]["_0"]; update the makeSimulateItem body to cast constructor once to a properly typed intermediate via Utils.magic (using the explicit annotation pattern: constructor->(Utils.magic: simulateItemConstructor<'event,'paramsConstructor,'where> => {event: ..., params: ..., block: ..., transaction: ...})), store that in a local (e.g., typed), and then replace all bracket accesses with dot-access on that local (typed.event, typed.params, typed.block, typed.transaction) when constructing the returned {event, contract, params, block, transaction} in makeSimulateItem to satisfy type-safety and ReScript guidelines.packages/envio/index.d.ts (1)
718-731:⚠️ Potential issue | 🟠 MajorFuel option aliases expose the EVM
whereshape instead of Fuel-specific filters.
FuelOnEventOptionsandFuelContractRegisterOptionsincorrectly aliasEvmOnEventOptions, which usesEvmOnEventWhere<...>in itswhereproperty. This means Fuel users receive the EVM filter API withblock.numberinstead of the correct Fuel filter API withblock.height. The Fuel-specificFuelOnEventWheretype already exists with the correctblock.heightconstraint but is not being used.Suggested fix
-export type FuelOnEventOptions<Event extends EventLike, Params = {}> = EvmOnEventOptions< - Event, - Params ->; +export type FuelOnEventOptions<Event extends EventLike, Params = {}> = { + readonly contract: Event["contractName"]; + readonly event: Event["eventName"]; + readonly wildcard?: boolean; + readonly where?: FuelOnEventWhere<Params, Event["contractName"] & string>; +}; -export type FuelContractRegisterOptions<Event extends EventLike, Params = {}> = EvmOnEventOptions< - Event, - Params ->; +export type FuelContractRegisterOptions<Event extends EventLike, Params = {}> = + FuelOnEventOptions<Event, Params>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/index.d.ts` around lines 718 - 731, FuelOnEventOptions and FuelContractRegisterOptions currently alias EvmOnEventOptions, which brings in EvmOnEventWhere (with block.number) instead of FuelOnEventWhere (with block.height); update these aliases to use the Fuel-specific where type by redefining FuelOnEventOptions<Event, Params> and FuelContractRegisterOptions<Event, Params> to extend/compose EvmOnEventOptions<Event, Params> but override its where property to use FuelOnEventWhere<Event> (or replace the where field type directly), referencing the existing FuelOnEventWhere and EvmOnEventOptions types to locate the change.
🧹 Nitpick comments (19)
packages/envio/src/bindings/PromClient.res (1)
66-72:Summary.makeSummarynot converted to the idempotent pattern — inconsistent with Counter/Gauge.
Counter.makeCounterandGauge.makeGaugewere routed throughgetOrCreateto tolerate duplicate module loads, butSummary.makeSummarystill binds directly tonew Summary(...). If anySummarymetric is created at module-init time, it will still throw on the second load — defeating the purpose of the refactor for that metric kind.♻️ Suggested change
module Summary = { type summary - `@new` `@module`("prom-client") external makeSummary: customMetric<'a> => summary = "Summary" + `@new` `@module`("prom-client") external makeSummaryUnsafe: customMetric<'a> => summary = "Summary" + let makeSummary = (config: customMetric<'a>): summary => + getOrCreate(config["name"], () => makeSummaryUnsafe(config)) `@send` external observe: (summary, float) => unit = "observe" `@send` external startTimer: (summary, unit) => float = "startTimer" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/bindings/PromClient.res` around lines 66 - 72, Summary.makeSummary is still bound directly to new Summary(...) and can throw on duplicate loads; change it to use the same idempotent creation path as Counter.makeCounter and Gauge.makeGauge by routing through the getOrCreate helper instead of directly calling the JS constructor. Update the binding for Summary.makeSummary (and any related observe/startTimer usages if necessary) to call the existing getOrCreate with the customMetric<'a> config so repeated module loads return the existing metric rather than constructing a new Summary instance.packages/e2e-tests/src/utils/process.ts (1)
148-175: Bound the retained output and renameoutputLines.
outputLinesaccumulates every stdout/stderr chunk for the full life of the wait, which for multi-minute e2e waits against verbose indexers can retain many MB even though only the last 20–30 chunks are ever used. Also, each entry is a chunk (possibly multi-line or partial), so the name is misleading andslice(-20)/slice(-30)yields a tail whose size is unpredictable.Consider a small ring buffer and consistent tail size:
♻️ Proposed refactor
- const outputLines: string[] = []; - - const onData = (data: Buffer) => { - const text = data.toString(); - outputLines.push(text); - if (text.includes(pattern)) { + const TAIL_CHUNKS = 30; + const outputChunks: string[] = []; + const pushChunk = (text: string) => { + outputChunks.push(text); + if (outputChunks.length > TAIL_CHUNKS) outputChunks.shift(); + }; + + const onData = (data: Buffer) => { + const text = data.toString(); + pushChunk(text); + if (text.includes(pattern)) { cleanup(); resolve(); } }; const onClose = (code: number | null) => { cleanup(); if (code === 0) resolve(); else { - const tail = outputLines.slice(-20).join(""); + const tail = outputChunks.join(""); reject( new Error( `Process exited with code ${code}\n--- last output ---\n${tail}` ) ); } }; const timer = setTimeout(() => { cleanup(); - const tail = outputLines.slice(-30).join(""); + const tail = outputChunks.join(""); reject(new Error(`Timed out waiting for "${pattern}" after ${timeoutMs}ms\n--- last output ---\n${tail}`)); }, timeoutMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/e2e-tests/src/utils/process.ts` around lines 148 - 175, The buffer outputLines is unbounded and misnamed; replace it with a bounded ring/buffer (e.g., outputTail) that keeps only the last N bytes/chars (not unlimited chunks) and update onData, onClose and the timeout handler to append new text into this tail while trimming oldest data to maintain a fixed max size (so slice(-20)/slice(-30) is no longer used); ensure the same tail-size and formatting are used when building the "last output" string in onClose and the timer rejection, and keep references to pattern, timeoutMs, cleanup, onData/onClose logic intact so behavior (resolving on pattern match and rejecting on nonzero exit/timeout) remains the same.packages/e2e-tests/src/config.ts (1)
56-73: Preserve the underlying resolution error for diagnostics.
try { ... } catch {}discards useful signal whenenvio/package.jsonexists but is malformed, whenbinlookup yields a path that doesn't exist on disk, or whenrequire.resolvefails for a non-MODULE_NOT_FOUNDreason. In CI, this makes it hard to distinguish "package not installed" from "bad artifact" — both produce the same generic "envio not found" message.♻️ Include the caught error in the thrown message
- const req = createRequire(import.meta.url); - try { - const pkgJsonPath = req.resolve("envio/package.json"); - const pkg = JSON.parse(fs.readFileSync(pkgJsonPath, "utf-8")); - const binRel = typeof pkg.bin === "string" ? pkg.bin : pkg.bin?.envio; - if (binRel) { - const binAbs = path.resolve(path.dirname(pkgJsonPath), binRel); - if (fs.existsSync(binAbs)) { - return { command: "node", args: [binAbs] }; - } - } - } catch {} - - throw new Error( - "envio not found. Either:\n" + - " - Set ENVIO_BIN env var\n" + - " - Run `pnpm install` to install the envio package" - ); + const req = createRequire(import.meta.url); + let cause: unknown; + try { + const pkgJsonPath = req.resolve("envio/package.json"); + const pkg = JSON.parse(fs.readFileSync(pkgJsonPath, "utf-8")); + const binRel = typeof pkg.bin === "string" ? pkg.bin : pkg.bin?.envio; + if (binRel) { + const binAbs = path.resolve(path.dirname(pkgJsonPath), binRel); + if (fs.existsSync(binAbs)) { + return { command: "node", args: [binAbs] }; + } + cause = new Error(`bin "${binRel}" resolved to ${binAbs} which does not exist`); + } else { + cause = new Error("envio/package.json has no usable `bin` field"); + } + } catch (e) { + cause = e; + } + + throw new Error( + "envio not found. Either:\n" + + " - Set ENVIO_BIN env var\n" + + " - Run `pnpm install` to install the envio package\n" + + `Underlying error: ${cause instanceof Error ? cause.message : String(cause)}` + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/e2e-tests/src/config.ts` around lines 56 - 73, The empty catch around the envio resolution swallows useful diagnostics; change the try/catch in the block that uses createRequire(import.meta.url), pkgJsonPath, pkg, binRel and binAbs to catch the error into a variable (e), and include e.message (or the original error) in the thrown Error (or rethrow a new Error that wraps the original) so the final error clearly states the underlying resolution/parsing/fs failure along with the existing "envio not found" guidance.packages/e2e-tests/src/dependency-tests/install.test.ts (1)
80-82: Nit: stray leading space on line 81.Line 81 is indented one column further than the surrounding comment lines (80, 82), which breaks the block-comment alignment.
✏️ Fix indentation
// 2. Run envio codegen — generates code into generated/. No deps or - // rescript config land in generated/, so nothing to install there. + // rescript config land in generated/, so nothing to install there. // Root package.json's "file:../../packages/envio" resolves to tmpRoot/packages/envio.🤖 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 80 - 82, There's a stray extra leading space in the middle line of the three-line comment starting with "// 2. Run envio codegen" — adjust the indentation of the line that currently reads "// rescript config land in generated/, so nothing to install there." so it matches the surrounding lines (single space after //) to align the comment block; locate that comment in install.test.ts and remove the extra space.packages/cli/build.rs (1)
1-5: Remove redundantextern crate napi_build;declaration.With
edition = "2021"inCargo.toml, the explicitextern crateis unnecessary —napi-buildis automatically in scope from the[build-dependencies]entry. This is a style improvement with no functional impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/build.rs` around lines 1 - 5, Remove the redundant extern crate napi_build; declaration at the top of build.rs and rely on the build-dependencies entry so napi_build::setup() is called directly; simply delete the extern crate line and leave the main function invoking napi_build::setup().scenarios/test_codegen/test/LoadLayer_test.res (1)
6-6: Optional: cacheallEntitiesonce per file.Each of the three updated cases (lines 6, 34, 69) re-invokes
Config.loadWithoutRegistrations()just to read.allEntities. IfloadWithoutRegistrationsdoes any non-trivial work, hoisting it to a module-levelletkeeps tests fast and readable:♻️ Suggested refactor
open Vitest +let allEntities = Config.loadWithoutRegistrations().allEntities + describe("LoadLayer", () => { Async.it("Trys to load non existing entity from db", async t => { let storageMock = MockIndexer.Storage.make([`#loadByIdsOrThrow`]) - let inMemoryStore = InMemoryStore.make(~entities=(Config.loadWithoutRegistrations()).allEntities) + let inMemoryStore = InMemoryStore.make(~entities=allEntities)Safe to defer if
loadWithoutRegistrationsis cheap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/LoadLayer_test.res` at line 6, Tests repeatedly call Config.loadWithoutRegistrations() just to access .allEntities; hoist that call to a module-level binding (e.g., add a top-level let cachedConfig = Config.loadWithoutRegistrations() or let allEntities = Config.loadWithoutRegistrations().allEntities) and update the three sites that call InMemoryStore.make(~entities=(Config.loadWithoutRegistrations()).allEntities) to use the cached binding (e.g., InMemoryStore.make(~entities=allEntities)) to avoid re-running loadWithoutRegistrations.scenarios/test_codegen/test/EventFilters_test.res (1)
3-4: Nit: comment restates what the code shows.The comment describes the return shape of
registerAllHandlersand what the returned config "captures" — that's already evident from the destructuring and the function name. Consider dropping it or, if useful, re-scoping to a non-obvious invariant (e.g. why event filters require the registration pass to be observable here).Based on learnings: "Don't write a comment that restates what the code already says — module purpose, what a function does, which callers use a value, history of a refactor".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/EventFilters_test.res` around lines 3 - 4, The comment repeating the return shape of registerAllHandlers (the destructured configWithRegistrations) should be removed or replaced with a focused note explaining a non-obvious invariant: either delete the redundant comment, or rewrite it to state why the function returns the config (e.g., that event filters must be collected during the registration pass because handlers register filters as a side-effect and tests rely on observing that pass), referencing registerAllHandlers and configWithRegistrations so readers understand the motivation rather than the obvious return shape.scenarios/test_codegen/test/EventOrigin_test.res (1)
42-63: CallConfig.loadWithoutRegistrations()once and reuse.
Config.loadWithoutRegistrations()is invoked three times (lines 42, 54, 62). Hoist it into a singleletso the test doesn't re-run the loader (and soInMemoryStore.make, the persistence, and the handler context all share the same config instance).♻️ Proposed change
- let inMemoryStore = InMemoryStore.make(~entities=(Config.loadWithoutRegistrations()).allEntities) + let config = Config.loadWithoutRegistrations() + let inMemoryStore = InMemoryStore.make(~entities=config.allEntities) let loadManager = LoadManager.make() @@ - persistence: PgStorage.makePersistenceFromConfig( - ~config=Config.loadWithoutRegistrations(), - ), + persistence: PgStorage.makePersistenceFromConfig(~config), @@ - config: Config.loadWithoutRegistrations(), + config,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/EventOrigin_test.res` around lines 42 - 63, Config.loadWithoutRegistrations() is being called multiple times; hoist it to a single binding and reuse it so the test shares the same config instance. Create a let binding (e.g. let config = Config.loadWithoutRegistrations()) and pass that binding into InMemoryStore.make(~entities=config.allEntities), PgStorage.makePersistenceFromConfig(~config=config), and UserContext.getHandlerContext(..., config: config) instead of calling Config.loadWithoutRegistrations() inline in InMemoryStore.make, PgStorage.makePersistenceFromConfig, and the handler context construction.scenarios/test_codegen/test/lib_tests/PgStorage_test.res (1)
925-949: Simplify error capture and drop the wildcard catch.The nested
switch (try {...} catch {...})is needlessly convoluted — a try/catch expression returns the message directly. The| _ => Nonearm also silently swallows any non-JsExnexception (including real bugs) and maps it to"", masking regressions behind an assertion-message mismatch.♻️ Proposed simplification
- let message = switch try { - let _ = PgStorage.makeStorageFromEnv(~config) - None - } catch { - | JsExn(e) => Some(e->JsExn.message->Option.getOr("")) - | _ => None - } { - | Some(m) => m - | None => "" - } + let message = try { + let _ = PgStorage.makeStorageFromEnv(~config) + "" + } catch { + | JsExn(e) => e->JsExn.message->Option.getOr("") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/lib_tests/PgStorage_test.res` around lines 925 - 949, Replace the nested switch/try/catch with a single try expression that captures only JsExn and returns its message (using JsExn.message->Option.getOr("")), removing the wildcard catch arm that swallows other exceptions; locate the test inside Async.it and the call to PgStorage.makeStorageFromEnv, and change the message assignment so it directly uses the try { let _ = PgStorage.makeStorageFromEnv(~config); "" } catch | JsExn(e) => e->JsExn.message->Option.getOr("") to preserve non-JsExn errors..github/workflows/build-platforms.yml (1)
81-87: Consider unifying Linux target libc configurations for consistency.The x64 Linux build uses
x86_64-unknown-linux-muslwhile arm64 usesaarch64-unknown-linux-gnu. Although Rust's musl target statically links musl libc by default (making the resulting binary self-contained and portable), the inconsistency across architectures can complicate maintenance. For maximum clarity and simplicity, consider:
- Switching x64 to
x86_64-unknown-linux-gnu(to match arm64), or- Switching arm64 to
aarch64-unknown-linux-musl(to match x64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-platforms.yml around lines 81 - 87, The workflow uses inconsistent Linux libc targets across architectures (x86_64-unknown-linux-musl vs aarch64-unknown-linux-gnu) which can complicate builds; update the target selection so both architectures use the same libc flavor: either change the x64 target from x86_64-unknown-linux-musl to x86_64-unknown-linux-gnu or change the arm64 target from aarch64-unknown-linux-gnu to aarch64-unknown-linux-musl wherever TARGET is set, and keep the rest of the packaging logic (the node_os case and lib_name -> envio.node copy) unchanged so packaging continues to use the correct lib_name per node_os.packages/envio/src/EventConfigBuilder.res (1)
429-430: Optional: derivetopicCountfromindexedParamsto avoid two passes.
topicCountis justindexedParams->Array.length + 1, so theArray.reduceon line 429 and theArray.filteron line 430 both walkparamsto compute equivalent information. Trivial perf, but it also removes a place where the two can drift.♻️ Proposed refactor
- let topicCount = params->Array.reduce(1, (acc, p) => p.indexed ? acc + 1 : acc) - let indexedParams = params->Array.filter(p => p.indexed) + let indexedParams = params->Array.filter(p => p.indexed) + let topicCount = indexedParams->Array.length + 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/EventConfigBuilder.res` around lines 429 - 430, Compute indexedParams first and derive topicCount from its length instead of doing a separate Array.reduce over params; specifically, replace the dual-pass pattern where topicCount is set with params->Array.reduce and indexedParams with params->Array.filter by building indexedParams via params->Array.filter(p => p.indexed) and then set topicCount = indexedParams->Array.length + 1 so both values come from the same single-pass result (refer to topicCount and indexedParams in EventConfigBuilder.res).packages/envio/src/PgStorage.res (1)
1689-1710: Consolidated ClickHouse env validation is a nice UX win.Collecting all missing env vars and reporting them in one shot is clearly better than failing on whichever
requireEnvfires first. Optional polish: the message readsenv vars are not setunconditionally, so a single missing var currently says "vars are" — either accept the small awkwardness or branch the noun/verb onmissing->Array.length. Not blocking.💬 Optional wording tweak
- JsError.throwWithMessage( - `ClickHouse storage is enabled but required env vars are not set: ${missing->Array.joinUnsafe( - ", ", - )}. Please set them, disable clickhouse in the \`storage\` config, or run \`envio dev\` for a pre-configured local ClickHouse.`, - ) + let (noun, verb) = missing->Array.length === 1 ? ("env var", "is") : ("env vars", "are") + JsError.throwWithMessage( + `ClickHouse storage is enabled but required ${noun} ${verb} not set: ${missing->Array.joinUnsafe( + ", ", + )}. Please set them, disable clickhouse in the \`storage\` config, or run \`envio dev\` for a pre-configured local ClickHouse.`, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/PgStorage.res` around lines 1689 - 1710, The error message for missing ClickHouse env vars always uses the plural form; update the JsError.throwWithMessage call to choose singular/plural based on missing->Array.length (e.g., if length === 1 use "env var is not set" and "var" otherwise "env vars are not set" and "vars"), keeping the same joined list from missing->Array.joinUnsafe and the rest of the sentence unchanged; locate the logic around the missing array, checkEnv function, and the JsError.throwWithMessage invocation to implement this conditional wording..github/workflows/build_and_verify.yml (2)
82-95: Brittle assumption:libenvio.soname and single-target build.Two small points:
L95 hard-codes
target/release/libenvio.so, which assumes the crate is configured as acdylibwith package nameenvio. If the crate is ever renamed or an additional[lib] name = "..."is set inCargo.toml, the copy silently no-ops and downstream artifacts upload an empty addon. Consider failing loudly with an explicit check, e.g.:- name: Build linux-x64 platform npm package run: | node packages/build-envio/build-platform-package.ts \ --version "0.0.1-dev" --platform "linux" --arch "x64" --out "envio-linux-x64" test -f target/release/libenvio.so || { echo "libenvio.so not found"; ls -la target/release; exit 1; } cp target/release/libenvio.so envio-linux-x64/envio.nodeOnly
linux-x64is produced. Darwin/Windows consumers (local dev, PR validators on macOS runners) won't get a platform package from this CI run. Fine if intentional for PR CI, but worth documenting in the job comment if the release workflow is elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_and_verify.yml around lines 82 - 95, The build step hard-codes and silently assumes target/release/libenvio.so exists and only produces linux-x64; update the "Build linux-x64 platform npm package" step to explicitly check for the built artifact (test -f target/release/libenvio.so or equivalent) and fail loudly (echo a clear error and exit non-zero, optionally listing target/release for debugging) before copying to envio-linux-x64/envio.node, and add a short job comment noting that only linux-x64 is produced here (or expand the job to build other platforms) so maintainers know this is intentional.
226-234: Symlink approach is correct; add a guard so a missing target fails fast.
ln -sf ../../.pnpmfile.cjs .pnpmfile.cjswill happily create a broken symlink if the target ever moves. Since a broken.pnpmfile.cjswould silently fall through pnpm without error ( pnpm treats a missing pnpmfile as "no hook" ), a fork/rename would regress CI without a direct failure signal. Adding atest -f ../../.pnpmfile.cjsbefore thelnpins the contract.♻️ Proposed change
- # scenarios/test_codegen has its own pnpm-workspace.yaml, so the root - # .pnpmfile.cjs isn't picked up when codegen triggers nested `pnpm install`. - # Symlink (not copy) so __dirname still resolves to the repo root. - ln -sf ../../.pnpmfile.cjs .pnpmfile.cjs + # scenarios/test_codegen has its own pnpm-workspace.yaml, so the root + # .pnpmfile.cjs isn't picked up when codegen triggers nested `pnpm install`. + # Symlink (not copy) so __dirname still resolves to the repo root. + test -f ../../.pnpmfile.cjs || { echo "root .pnpmfile.cjs missing"; exit 1; } + ln -sf ../../.pnpmfile.cjs .pnpmfile.cjs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_and_verify.yml around lines 226 - 234, In the test_codegen step, guard the symlink creation so a missing target fails fast: before running the ln -sf ../../.pnpmfile.cjs .pnpmfile.cjs command in the test_codegen step, check for the existence of ../../.pnpmfile.cjs (e.g., with a test -f check) and if it doesn't exist emit an explanatory error and exit non‑zero so CI fails; update the test_codegen run block to perform this existence check prior to creating the symlink.scenarios/test_codegen/test/helpers/MockIndexer.res (1)
202-207: Reuse the module-levelconfiginstead of re-loading.
toPersistencecallsConfig.loadWithoutRegistrations()again even though the top-of-filelet config = Config.loadWithoutRegistrations()is in scope. The memoization inConfig.resmakes this equivalent at runtime, but the duplication is an unnecessary read-after-read that diverges from L25 and L277 patterns.♻️ Proposed change
let toPersistence = (storageMock: t) => { { ...PgStorage.makePersistenceFromConfig( - ~config=Config.loadWithoutRegistrations(), + ~config, ~storage=storageMock.storage, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/helpers/MockIndexer.res` around lines 202 - 207, The toPersistence function is re-calling Config.loadWithoutRegistrations() instead of reusing the module-level config; update the call to PgStorage.makePersistenceFromConfig inside toPersistence to pass the existing top-level config variable (config) for the ~config argument rather than calling Config.loadWithoutRegistrations() again, leaving PgStorage.makePersistenceFromConfig and ~storage=storageMock.storage unchanged.scenarios/test_codegen/test/ApplyRegistrations_test.res (1)
35-54: Tautological assertion ondependsOnAddresses.This test compares
eventConfig.dependsOnAddressesagainstInternal.dependsOnAddresses(~isWildcard=eventConfig.isWildcard, ~filterByAddresses=eventConfig.filterByAddresses). Since both sides are derived from the same event config, the test passes as long asHandlerLoadercallsInternal.dependsOnAddresseswith those same two arguments — it does not pin down the expected boolean for the wildcard+where case the comment describes (“filterByAddresses=falseanddependsOnAddresses=false”).Consider asserting the concrete expected values directly so a regression that always returns
true(or flips a sign inInternal.dependsOnAddresses) would still fail the test:♻️ Proposed change
- t.expect( - eventConfig.dependsOnAddresses, - ).toBe( - Internal.dependsOnAddresses( - ~isWildcard=eventConfig.isWildcard, - ~filterByAddresses=eventConfig.filterByAddresses, - ), - ) + t.expect(eventConfig.isWildcard).toBe(true) + t.expect(eventConfig.filterByAddresses).toBe(false) + t.expect(eventConfig.dependsOnAddresses).toBe(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/ApplyRegistrations_test.res` around lines 35 - 54, The test currently asserts eventConfig.dependsOnAddresses equals Internal.dependsOnAddresses(...) which is tautological; instead hard-code the expected boolean values for this wildcard+where case: call getEvmEventConfig(~contractName="EventFiltersTest", ~eventName="Transfer") to get eventConfig and then assert eventConfig.filterByAddresses is false and eventConfig.dependsOnAddresses is false (and if you still want to check the helper, assert Internal.dependsOnAddresses(~isWildcard=eventConfig.isWildcard, ~filterByAddresses=false) === false); update references to eventConfig and Internal.dependsOnAddresses in the test accordingly so it verifies concrete expected booleans rather than reusing the same derived inputs.packages/envio/src/Config.res (1)
854-879: Memoization works for production but can bite tests that re-prime.
prime(json)correctly invalidatescached, but two patterns to verify:
Test isolation: if any test mutates the primed JSON in place (e.g., patches a field on the parsed object) and re-calls
loadWithoutRegistrations()without re-priming, it gets the cachedtfrom a previous JSON — hard-to-debug cross-test leakage. Worth adding a test-only reset or documenting "always re-prime between tests."Partial failure: if
fromPublicthrows on the first call,cachedstaysNone— good. But if a later call does succeed with different primed JSON, the earlier error's absence of caching means the failing primed JSON is still inprimedJson. Onceprimeis called again with valid JSON, the ref is replaced andcachedis reset, so this is fine. Just noting the pair-wise invariant.Also: consider making
primeidempotency explicit — callingprime(sameJson)invalidates even when nothing changed. That may force a re-parse that's not semantically meaningful. AJSON.stringify-comparison guard could keep the cache warm, but the complexity may not be worth it givenprimeis called at most once per CLI command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/Config.res` around lines 854 - 879, Add a test-safe reset and avoid in-place mutations: make prime(json) store a deep-cloned copy of json (e.g., JSON.parse(JSON.stringify(json))) into primedJson and keep its existing cached := None behavior, and add a test-only function (e.g., resetPriming or clearPrime) that sets primedJson := None and cached := None so tests can fully isolate; also ensure loadWithoutRegistrations uses the cloned primedJson.contents value when calling fromPublic to prevent cross-test mutation via shared object; reference primedJson, cached, prime, and loadWithoutRegistrations in your change..github/workflows/build_and_verify_pr.yml (1)
50-51: Use--frozen-lockfilefor CI installs.
pnpm installwithout--frozen-lockfilelets CI silently drift frompnpm-lock.yaml(e.g. when a floating range resolves to a new version between PRs). For a verification workflow meant to prove the PR builds reproducibly,--frozen-lockfileis the safer default.- - name: Install dependencies - run: pnpm install + - name: Install dependencies + run: pnpm install --frozen-lockfile(Skipping the Checkov
CKV_SECRET_4hit on lines 134–135 — that's a test-only Postgres password inside a CI service definition, not a real secret.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_and_verify_pr.yml around lines 50 - 51, Update the "Install dependencies" workflow step to run a locked install by changing its run command from `pnpm install` to `pnpm install --frozen-lockfile`; locate the step named "Install dependencies" in the GitHub Actions workflow and replace the run line so CI fails if pnpm-lock.yaml is out of sync.packages/envio/src/Core.res (1)
13-34: Fragile:_keepFs/_keepCprely on undocumented compiler-generated module binding names.The
loadDevAddon%rawbody referencesNodechild_process,Nodepath, andNodefsas JavaScript globals. These names are not documented in ReScript 12 and depend on the compiler's internal implementation for naming module bindings created by@module("node:...")externals. If the ReScript compiler changes how it generates these names (or optimizes them away), this code silently breaks at runtime with aReferenceErrorbecause the type system cannot see references inside%raw.♻️ Prefer inlining the Node built-ins via
requireinside the%raw-// Keeps `node:fs` / `node:child_process` imports alive for the %raw -// loadDevAddon block, which references them as Nodefs / Nodechild_process. -let _keepFs = existsSync -let _keepCp = execSyncRaw - let callRequire: ({..}, string) => addon = %raw(`(req, id) => req(id)`) let envioPackageDir = pathDirname(pathDirname(fileURLToPath(importMetaUrl))) // Runs `cargo build` on every invocation (like `cargo run`). let loadDevAddon: ({..}, string) => addon = %raw(`function(req, envioDir) { - var cp = Nodechild_process; - var path = Nodepath; - var fs = Nodefs; + var cp = require("node:child_process"); + var path = require("node:path"); + var fs = require("node:fs");This keeps the
%rawself-contained and removes the dependency on the compiler's internal module binding names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/Core.res` around lines 13 - 34, The raw JS in loadDevAddon currently relies on compiler-generated globals (referenced indirectly via _keepFs/_keepCp), which is fragile; update loadDevAddon so the %raw body obtains Node built-ins itself (e.g., call require inside the %raw to get 'node:fs', 'node:child_process', 'node:path' and any other Node modules) instead of depending on _keepFs/_keepCp or module binding names, and remove or repurpose the _keepFs/_keepCp shims; ensure you still use callRequire or createRequire only to bootstrap a require inside the %raw and reference the module names exactly ('node:fs', 'node:child_process', 'node:path') so the raw code is self-contained and no compiler-internal binding names are assumed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a706b3fa-acbf-44c5-b98b-134ae5e0ce16
⛔ Files ignored due to path filters (11)
Cargo.lockis excluded by!**/*.lockpackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__indexer_code_generates_correct_types_and_values.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__indexer_code_multiple_chains.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_generated_for_evm.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_generated_for_fuel.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_with_all_options.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_with_lowercase_contract_name.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_with_multiple_contracts.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_with_no_contracts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yamlscenarios/test_codegen/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (138)
.github/actions/prepare-envio-artifacts/action.yml.github/workflows/build-platforms.yml.github/workflows/build_and_verify.yml.github/workflows/build_and_verify_pr.yml.github/workflows/publish.yml.gitignore.pnpmfile.cjsCLAUDE.mdCONTRIBUTING.mdCommandLineHelp.mdpackages/build-envio/build-platform-package.tspackages/build-envio/src/build-artifact.tspackages/build-envio/src/verify-artifact.tspackages/cli/Cargo.tomlpackages/cli/CommandLineHelp.mdpackages/cli/Makefilepackages/cli/build.rspackages/cli/examples/script.rspackages/cli/src/cli_args/clap_definitions.rspackages/cli/src/cli_args/init_config.rspackages/cli/src/cli_args/interactive_init/mod.rspackages/cli/src/commands.rspackages/cli/src/config_parsing/graph_migration/mod.rspackages/cli/src/config_parsing/system_config.rspackages/cli/src/docker_env.rspackages/cli/src/executor/dev.rspackages/cli/src/executor/init.rspackages/cli/src/executor/local.rspackages/cli/src/executor/mod.rspackages/cli/src/fuel/abi.rspackages/cli/src/hbs_templating/codegen_templates.rspackages/cli/src/lib.rspackages/cli/src/main.rspackages/cli/src/napi.rspackages/cli/src/persisted_state/db.rspackages/cli/src/persisted_state/hash_string.rspackages/cli/src/persisted_state/mod.rspackages/cli/src/project_paths/path_utils.rspackages/cli/templates/dynamic/codegen/index.d.ts.hbspackages/cli/templates/dynamic/codegen/index.js.hbspackages/cli/templates/dynamic/codegen/internal.config.json.hbspackages/cli/templates/dynamic/codegen/package.json.hbspackages/cli/templates/dynamic/codegen/src/Indexer.res.hbspackages/cli/templates/dynamic/codegen/src/Path.res.hbspackages/cli/templates/dynamic/codegen/src/Types.ts.hbspackages/cli/templates/dynamic/init_templates/shared/package.json.hbspackages/cli/templates/static/blank_template/rescript/rescript.jsonpackages/cli/templates/static/blank_template/shared/.gitignorepackages/cli/templates/static/codegen/.npmrcpackages/cli/templates/static/codegen/rescript.jsonpackages/cli/templates/static/codegen/src/Index.bs.jspackages/cli/templates/static/codegen/src/Index.respackages/cli/templates/static/codegen/src/Js.shim.tspackages/cli/templates/static/codegen/src/TestIndexerWorker.respackages/cli/templates/static/codegen/src/bindings/OpaqueTypes.tspackages/cli/templates/static/factory_template/shared/.gitignorepackages/e2e-tests/package.jsonpackages/e2e-tests/src/config.tspackages/e2e-tests/src/dependency-tests/install.test.tspackages/e2e-tests/src/e2e/e2e.test.tspackages/e2e-tests/src/template-tests/templates.test.tspackages/e2e-tests/src/utils/process.tspackages/envio/.gitignorepackages/envio/bin.mjspackages/envio/index.d.tspackages/envio/index.jspackages/envio/package.jsonpackages/envio/rescript.jsonpackages/envio/src/Address.gen.tspackages/envio/src/Address.respackages/envio/src/Api.respackages/envio/src/Bin.respackages/envio/src/Config.respackages/envio/src/Core.respackages/envio/src/EnvSafe.respackages/envio/src/EnvSafe.resipackages/envio/src/Envio.gen.tspackages/envio/src/Envio.respackages/envio/src/EventConfigBuilder.respackages/envio/src/EvmTypes.gen.tspackages/envio/src/EvmTypes.respackages/envio/src/FetchState.respackages/envio/src/HandlerLoader.respackages/envio/src/InMemoryStore.gen.tspackages/envio/src/InMemoryStore.respackages/envio/src/Internal.gen.tspackages/envio/src/Internal.respackages/envio/src/Main.respackages/envio/src/Migrations.respackages/envio/src/PgStorage.gen.tspackages/envio/src/PgStorage.respackages/envio/src/PgStorage.res.d.mtspackages/envio/src/TestIndexer.respackages/envio/src/TestIndexerWorker.respackages/envio/src/Types.tspackages/envio/src/Utils.gen.tspackages/envio/src/Utils.respackages/envio/src/Utils.res.d.mtspackages/envio/src/bindings/BigDecimal.gen.tspackages/envio/src/bindings/BigDecimal.respackages/envio/src/bindings/BigDecimal.res.d.mtspackages/envio/src/bindings/BigInt.res.d.mtspackages/envio/src/bindings/Ethers.res.d.mtspackages/envio/src/bindings/NodeJs.respackages/envio/src/bindings/Pino.gen.tspackages/envio/src/bindings/Pino.respackages/envio/src/bindings/Postgres.gen.tspackages/envio/src/bindings/Postgres.respackages/envio/src/bindings/Postgres.res.d.mtspackages/envio/src/bindings/PromClient.respackages/envio/src/db/InternalTable.gen.tspackages/envio/src/db/InternalTable.respackages/envio/src/sources/HyperSyncClient.gen.tspackages/envio/src/sources/HyperSyncClient.respackages/envio/src/tui/Tui.resplan.mdscenarios/e2e_test/vitest.config.tsscenarios/fuel_test/package.jsonscenarios/fuel_test/rescript.jsonscenarios/fuel_test/src/Indexer.resscenarios/helpers/package.jsonscenarios/svm_test/package.jsonscenarios/svm_test/rescript.jsonscenarios/test_codegen/package.jsonscenarios/test_codegen/rescript.jsonscenarios/test_codegen/src/Indexer.resscenarios/test_codegen/src/handlers/EventHandlers.resscenarios/test_codegen/test/ApplyRegistrations_test.resscenarios/test_codegen/test/ChainManager_test.resscenarios/test_codegen/test/EventFilters_test.resscenarios/test_codegen/test/EventOrigin_test.resscenarios/test_codegen/test/LoadLayer_test.resscenarios/test_codegen/test/__mocks__/MockConfig.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/lib_tests/FetchState_test.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/rollback/ChainDataHelpers.resscenarios/test_codegen/test/rollback/MockChainData_test.res
💤 Files with no reviewable changes (48)
- packages/cli/templates/dynamic/codegen/internal.config.json.hbs
- packages/cli/templates/static/codegen/.npmrc
- .github/actions/prepare-envio-artifacts/action.yml
- .gitignore
- packages/envio/src/InMemoryStore.res
- packages/cli/templates/dynamic/codegen/src/Indexer.res.hbs
- packages/cli/templates/static/blank_template/shared/.gitignore
- packages/cli/templates/static/codegen/rescript.json
- packages/cli/templates/static/factory_template/shared/.gitignore
- packages/envio/src/bindings/BigDecimal.res.d.mts
- packages/envio/src/Utils.res.d.mts
- packages/envio/src/bindings/BigInt.res.d.mts
- packages/envio/src/db/InternalTable.gen.ts
- packages/envio/src/bindings/Postgres.res
- packages/cli/templates/static/codegen/src/bindings/OpaqueTypes.ts
- packages/cli/templates/dynamic/codegen/package.json.hbs
- packages/cli/templates/static/codegen/src/TestIndexerWorker.res
- packages/envio/src/bindings/Postgres.gen.ts
- packages/envio/src/InMemoryStore.gen.ts
- packages/envio/src/db/InternalTable.res
- packages/cli/src/main.rs
- packages/envio/src/bindings/BigDecimal.res
- packages/envio/src/Utils.res
- packages/envio/src/bindings/BigDecimal.gen.ts
- packages/envio/src/PgStorage.gen.ts
- packages/envio/src/bindings/Ethers.res.d.mts
- packages/envio/src/sources/HyperSyncClient.res
- packages/envio/src/Address.gen.ts
- packages/cli/templates/static/codegen/src/Index.res
- packages/cli/templates/static/codegen/src/Js.shim.ts
- packages/envio/src/Address.res
- packages/envio/src/Utils.gen.ts
- plan.md
- packages/envio/src/bindings/Postgres.res.d.mts
- packages/cli/src/project_paths/path_utils.rs
- packages/envio/src/Internal.gen.ts
- packages/envio/src/bindings/Pino.res
- packages/envio/src/Envio.gen.ts
- packages/envio/src/EvmTypes.gen.ts
- packages/envio/src/Types.ts
- packages/envio/src/EvmTypes.res
- packages/envio/src/bindings/Pino.gen.ts
- packages/envio/src/sources/HyperSyncClient.gen.ts
- packages/envio/src/PgStorage.res.d.mts
- packages/envio/src/Migrations.res
- packages/cli/templates/static/codegen/src/Index.bs.js
- packages/cli/templates/dynamic/codegen/src/Types.ts.hbs
- packages/cli/templates/dynamic/codegen/src/Path.res.hbs
| // dropping new files like src/Migrations.res.mjs. | ||
| // dropping new files like src/Bin.res.mjs. | ||
| const ARTIFACT_DIR = path.join(__dirname, ".envio-artifacts", "envio"); | ||
| const PLATFORM_ARTIFACT_DIR = path.join(__dirname, ".envio-artifacts", "envio-linux-x64"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm PLATFORM_ARTIFACT_DIR isn't consumed elsewhere in the repo.
rg -nP '\bPLATFORM_ARTIFACT_DIR\b'Repository: enviodev/hyperindex
Length of output: 45
🏁 Script executed:
cat -n .pnpmfile.cjs | head -50Repository: enviodev/hyperindex
Length of output: 2262
Remove the unused constant PLATFORM_ARTIFACT_DIR.
The constant is defined at line 15 but never referenced. Line 35 intentionally uses a relative path ("file:../envio-linux-x64") relative to the envio package directory, whereas the suggested option of using PLATFORM_ARTIFACT_DIR would produce an absolute path that changes pnpm's resolution semantics. Since the constant isn't used, the cleanest fix is to remove it.
-const PLATFORM_ARTIFACT_DIR = path.join(__dirname, ".envio-artifacts", "envio-linux-x64");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const PLATFORM_ARTIFACT_DIR = path.join(__dirname, ".envio-artifacts", "envio-linux-x64"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pnpmfile.cjs at line 15, Remove the unused constant PLATFORM_ARTIFACT_DIR:
delete the declaration const PLATFORM_ARTIFACT_DIR = path.join(__dirname,
".envio-artifacts", "envio-linux-x64") since it is never referenced; leave the
existing resolution that uses the relative package specifier
"file:../envio-linux-x64" unchanged so pnpm's resolution semantics are
preserved.
| ###### **Options:** | ||
|
|
||
| * `-n`, `--name <NAME>` — The name of your project | ||
| * `-l`, `--language <LANGUAGE>` — The language used to write handlers | ||
|
|
||
| Possible values: `typescript`, `rescript` | ||
|
|
||
| * `--api-token <API_TOKEN>` — The hypersync API key to be initialized in your templates .env file | ||
|
|
||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the clap definitions actually declare --package-manager for `init`.
rg -nP -C3 '\bpackage[_-]manager\b' packages/cli/src/cli_args/
# And whether other scaffolding (init_config.rs / executor/init.rs) references it.
rg -nP -C3 '\bPackageManager\b|\bpackage_manager\b' packages/cli/src/Repository: enviodev/hyperindex
Length of output: 8831
Generated help documentation is stale: --package-manager flag missing from envio init options.
The --package-manager flag is declared in clap_definitions.rs (lines 145–148) with support for pnpm, npm, yarn, and bun, and is actively used throughout the codebase (interactive_init, executor/init, commands). However, it does not appear in the CommandLineHelp.md options list (lines 70–79), which only documents -n/--name, -l/--language, and --api-token. Re-run make update-help to regenerate the documentation from the updated clap definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CommandLineHelp.md` around lines 70 - 79, The generated CLI help is stale:
the --package-manager flag declared in clap_definitions.rs (and used by
interactive_init, executor/init and commands) is not listed in
CommandLineHelp.md; fix by regenerating the docs (re-run make update-help) so
CommandLineHelp.md includes the --package-manager option (with its allowed
values pnpm, npm, yarn, bun) or, if regenerating is not possible, add a matching
options entry for --package-manager to CommandLineHelp.md so the documentation
matches the clap_definitions.rs declaration.
| pub fn resolve_default() -> Self { | ||
| let pnpm_available = std::process::Command::new("pnpm") | ||
| .arg("--version") | ||
| .stdout(std::process::Stdio::null()) | ||
| .stderr(std::process::Stdio::null()) | ||
| .status() | ||
| .map(|s| s.success()) | ||
| .unwrap_or(false); | ||
| if pnpm_available { | ||
| PackageManager::Pnpm | ||
| } else { | ||
| PackageManager::Npm | ||
| } | ||
| } |
There was a problem hiding this comment.
pnpm detection will likely fail on Windows.
On Windows, pnpm is typically installed as pnpm.cmd / pnpm.ps1; std::process::Command::new("pnpm") goes through CreateProcessW and doesn't auto-resolve .cmd/.bat shims. Windows users with pnpm installed will silently default to npm.
Consider probing both on Windows (or use a helper like which) so the default matches what's actually on PATH.
🛠️ Sketch
pub fn resolve_default() -> Self {
- let pnpm_available = std::process::Command::new("pnpm")
- .arg("--version")
- .stdout(std::process::Stdio::null())
- .stderr(std::process::Stdio::null())
- .status()
- .map(|s| s.success())
- .unwrap_or(false);
- if pnpm_available {
+ let candidates: &[&str] = if cfg!(windows) {
+ &["pnpm.cmd", "pnpm"]
+ } else {
+ &["pnpm"]
+ };
+ let pnpm_available = candidates.iter().any(|bin| {
+ std::process::Command::new(bin)
+ .arg("--version")
+ .stdout(std::process::Stdio::null())
+ .stderr(std::process::Stdio::null())
+ .status()
+ .map(|s| s.success())
+ .unwrap_or(false)
+ });
+ if pnpm_available {
PackageManager::Pnpm
} else {
PackageManager::Npm
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn resolve_default() -> Self { | |
| let pnpm_available = std::process::Command::new("pnpm") | |
| .arg("--version") | |
| .stdout(std::process::Stdio::null()) | |
| .stderr(std::process::Stdio::null()) | |
| .status() | |
| .map(|s| s.success()) | |
| .unwrap_or(false); | |
| if pnpm_available { | |
| PackageManager::Pnpm | |
| } else { | |
| PackageManager::Npm | |
| } | |
| } | |
| pub fn resolve_default() -> Self { | |
| let candidates: &[&str] = if cfg!(windows) { | |
| &["pnpm.cmd", "pnpm"] | |
| } else { | |
| &["pnpm"] | |
| }; | |
| let pnpm_available = candidates.iter().any(|bin| { | |
| std::process::Command::new(bin) | |
| .arg("--version") | |
| .stdout(std::process::Stdio::null()) | |
| .stderr(std::process::Stdio::null()) | |
| .status() | |
| .map(|s| s.success()) | |
| .unwrap_or(false) | |
| }); | |
| if pnpm_available { | |
| PackageManager::Pnpm | |
| } else { | |
| PackageManager::Npm | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli_args/init_config.rs` around lines 416 - 429, The pnpm
detection in resolve_default currently only tries "pnpm" which fails on Windows
where executables are often "pnpm.cmd" or "pnpm.ps1"; update resolve_default to
probe Windows-specific names (try "pnpm", then "pnpm.cmd", then "pnpm.ps1") or
use a PATH lookup helper (like which) and mark PackageManager::Pnpm if any
succeed; ensure you still null stdout/stderr and fallback to PackageManager::Npm
if none are found.
| pub fn get_envio_version(envio_package_dir: Option<&str>) -> Result<String> { | ||
| if is_valid_release_version_number(VERSION) { | ||
| return Ok(VERSION.to_string()); | ||
| } | ||
|
|
||
| // Dev mode: walk up from the binary to find the local envio package. | ||
| // Using current_exe() instead of current_dir() so this works even when | ||
| // cwd is outside the repo (e.g. template tests that run in /tmp/). | ||
| let exe = env::current_exe() | ||
| .and_then(|p| p.canonicalize()) | ||
| .context("failed to resolve current executable path")?; | ||
| // When the binary lives inside .envio-artifacts/ (the pre-built CI artifact), | ||
| // prefer .envio-artifacts/envio — it has compiled .res.mjs files that the | ||
| // source directory lacks. Dev binaries in target/ should use packages/envio | ||
| // to avoid creating a duplicate envio instance alongside workspace packages | ||
| // that already reference packages/envio (duplicate instances cause Prometheus | ||
| // metric registration collisions). | ||
| let prefer_artifact = exe | ||
| .components() | ||
| .any(|c| c.as_os_str() == ".envio-artifacts"); | ||
| let mut dir = exe.parent(); | ||
| while let Some(d) = dir { | ||
| let artifact = d.join(".envio-artifacts/envio"); | ||
| let source = d.join("packages/envio"); | ||
| if prefer_artifact && artifact.is_dir() { | ||
| return Ok(format!("file:{}", artifact.to_string_lossy())); | ||
| } | ||
| if source.is_dir() { | ||
| return Ok(format!("file:{}", source.to_string_lossy())); | ||
| } | ||
| if artifact.is_dir() { | ||
| return Ok(format!("file:{}", artifact.to_string_lossy())); | ||
| } | ||
| dir = d.parent(); | ||
| } | ||
| Err(anyhow!( | ||
| "could not find packages/envio above executable: {}", | ||
| exe.display() | ||
| )) | ||
| let pkg_dir = envio_package_dir.ok_or_else(|| { | ||
| anyhow!( | ||
| "envio version is not a release ({VERSION}) and no envio_package_dir was supplied. \ | ||
| Run via the NAPI host (which resolves it from import.meta.url) or pass an explicit path." | ||
| ) | ||
| })?; | ||
|
|
||
| // Format as `file:{dir}` so the generated `package.json` resolves to | ||
| // the SAME envio instance as the parent, avoiding duplicate module | ||
| // instances that break shared registries (HandlerRegister, Prometheus | ||
| // metrics). | ||
| let pkg = PathBuf::from(pkg_dir); | ||
| if !pkg.is_dir() { | ||
| return Err(anyhow!( | ||
| "envio_package_dir does not exist or is not a directory: {}", | ||
| pkg.display() | ||
| )); | ||
| } | ||
| Ok(format!("file:{}", pkg.to_string_lossy())) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Sweep all call sites of get_envio_version to confirm they pass Option<&str>.
ast-grep --pattern 'get_envio_version($$$)'
rg -nP '\bget_envio_version\b' --type=rust -C2Repository: enviodev/hyperindex
Length of output: 2182
Windows path handling in file: dependency entry needs normalization.
The single call site of get_envio_version at packages/cli/src/executor/init.rs:249 correctly passes the Option<&str> parameter following the signature change, and the function's role is well-documented in packages/cli/src/executor/mod.rs (lines 53–57).
The remaining concern: pkg.to_string_lossy() at line 417 emits backslashes on Windows (e.g., file:C:\foo\bar), which is problematic for some package managers and lossy for non-UTF-8 paths. Normalize the path to forward slashes or a platform-independent URL form when constructing the file: dependency entry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/config_parsing/system_config.rs` around lines 394 - 418,
get_envio_version currently uses pkg.to_string_lossy() to build the "file:{dir}"
dependency which emits backslashes on Windows; update get_envio_version to
normalize the path when building the file: entry (e.g., convert PathBuf to a
platform-independent URL or replace backslashes with forward slashes) so the
returned string is safe for package managers and non-Windows consumers; locate
the code in get_envio_version (references: get_envio_version, pkg,
pkg.to_string_lossy) and ensure the final Ok(...) returns a normalized path like
"file:{normalized_dir}" (use a Path/URL conversion utility or explicitly
transform separators) instead of the raw to_string_lossy output.
| let getOrCreate = (name: string, create: unit => 'a): 'a => { | ||
| switch defaultRegister->getSingleMetric(name) { | ||
| | Some(existing) => existing->(Utils.magic: metricInstance => 'a) | ||
| | None => create() | ||
| } | ||
| } |
There was a problem hiding this comment.
getOrCreate silently masks metric-config drift and type mismatches.
Two subtle issues worth considering:
- If an existing metric with the same name was registered elsewhere with different
helptext or label configuration,getOrCreatesilently returns it — the newconfigis discarded without warning. Previously,prom-clientwould throw on a conflicting registration, surfacing the bug. getSingleMetricis keyed by name only. If aGaugewas already registered under a name and a caller then requests aCounterwith that name, theUtils.magic: metricInstance => 'acast yields a mis-typed object whoseinc/labels/etc. calls would behave unexpectedly at runtime. In practice names are likely unique per kind, but there's no enforcement here.
If the sole motivation is surviving "module loaded twice via different pnpm paths", a narrower check (e.g. only dedupe on the defaultRegister re-import scenario, or at least assert the returned instance's constructor matches) would preserve more of the original safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envio/src/bindings/PromClient.res` around lines 26 - 31, The
getOrCreate function currently returns any existing metric by name and uses
Utils.magic to cast it, which can silently mask config drift and type
mismatches; update getOrCreate to: call defaultRegister->getSingleMetric(name),
if Some(existing) then verify the existing metric's constructor/type (e.g.
instanceof or constructor name) matches the requested metric type and compare
key config fields (help text and label names) against the requested config, and
if they differ either throw or log a clear error instead of returning the
mismatched instance; only fall back to returning the existing metric without
error when the type and config match, otherwise create() as intended (or fail
fast) to preserve safety.
| let fromPublic = (publicConfigJson: JSON.t) => { | ||
| let maxAddrInPartition = Env.maxAddrInPartition | ||
| // Parse public config | ||
| let publicConfig = try publicConfigJson->S.parseOrThrow(publicConfigSchema) catch { | ||
| | S.Raised(exn) => | ||
| JsError.throwWithMessage( | ||
| `Invalid internal.config.ts: ${exn->Utils.prettifyExn->(Utils.magic: exn => string)}`, | ||
| `Invalid indexer config: ${exn->Utils.prettifyExn->(Utils.magic: exn => string)}`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how Env.maxAddrInPartition is defined and read
fd -t f 'Env.res' packages/envio/src --exec cat {}
rg -nP --type-add 'res:*.res' --type=res -C2 '\bmaxAddrInPartition\b'Repository: enviodev/hyperindex
Length of output: 41904
Env.maxAddrInPartition is eagerly evaluated at module load—CLI overrides via MAX_PARTITION_SIZE won't apply.
Env.maxAddrInPartition is a module-level let binding (not a function) that reads MAX_PARTITION_SIZE from process.env when Env.res initializes via EnvSafe.make(). This happens at package load time, before Bin.applyEnv can mutate process.env. Consequently, CLI-provided values for MAX_PARTITION_SIZE won't be reflected when Config.fromPublic calls Env.maxAddrInPartition on line 457.
Either convert Env.maxAddrInPartition to a getter function that reads process.env on each access, or ensure environment setup happens before any module that reads Env.maxAddrInPartition is loaded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envio/src/Config.res` around lines 456 - 464, Env.maxAddrInPartition
is being read eagerly at module load which ignores CLI overrides; change it to
be a function that reads process.env at call time (e.g., rename to
getMaxAddrInPartition or make it a thunk) and update callers — specifically the
fromPublic function in Config.res (where Env.maxAddrInPartition is referenced)
to call the new getter function instead of the old value; ensure any other
usages across the codebase are updated to invoke the function so
MAX_PARTITION_SIZE from the CLI is respected at runtime.
| let loadAddon = () => { | ||
| let req = createRequire(importMetaUrl) | ||
| let platformPkg = `envio-${processPlatform}-${processArch}` | ||
|
|
||
| // 1. Platform package (production + CI via .pnpmfile.cjs redirect) | ||
| try { | ||
| callRequire(req, platformPkg) | ||
| } catch { | ||
| | _ => | ||
| // 2. Dev build (cargo build on every run) | ||
| switch loadDevAddon(req, envioPackageDir)->(Utils.magic: addon => option<addon>) { | ||
| | Some(addon) => addon | ||
| | None => | ||
| JsError.throwWithMessage(`Couldn't load the envio native addon. Install the envio npm package.`) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ReScript 12, does option(where T is a record) match None when the runtime value isnull, or only when it's undefined?
💡 Result:
In ReScript 12, option<T> (where T is a record) matches None only when the runtime value is undefined, not when it is null. A null value will not match None and will instead attempt to match Some(null), which fails because null is not a record object. Use Nullable.t<T> for values that can be either null or undefined.
Citations:
- 1: https://rescript-lang.org/docs/manual/v12.0.0/null-undefined-option
- 2: https://rescript-lang.org/docs/manual/null-undefined-option/
- 3: https://rescript-lang.org/docs/manual/null-undefined-option
- 4: https://rescript-lang.org/docs/manual/v11.0.0/api/core/nullable
🏁 Script executed:
cd packages/envio && wc -l src/Core.resRepository: enviodev/hyperindex
Length of output: 80
🏁 Script executed:
cd packages/envio && sed -n '50,80p' src/Core.resRepository: enviodev/hyperindex
Length of output: 1065
🏁 Script executed:
cd packages/envio && sed -n '100,120p' src/Core.resRepository: enviodev/hyperindex
Length of output: 682
🏁 Script executed:
cd packages/envio && sed -n '30,95p' src/Core.resRepository: enviodev/hyperindex
Length of output: 2293
🏁 Script executed:
cd packages/envio && sed -n '115,146p' src/Core.resRepository: enviodev/hyperindex
Length of output: 684
null return from loadDevAddon is mis-classified as Some(null).
The %raw loadDevAddon function returns JS null in multiple failure paths (lines 57, 65, 71, 73, 75 — when pnpm list fails, JSON parsing fails, dependency lookup fails, version is non-file:, or the resolved Cargo.toml doesn't exist). You then cast it with Utils.magic: addon => option<addon> and pattern-match:
switch loadDevAddon(req, envioPackageDir)->(Utils.magic: addon => option<addon>) {
| Some(addon) => addon
| None => JsError.throwWithMessage(`Couldn't load the envio native addon...`)
}ReScript's option type matches None only when the runtime value is undefined, not null. When loadDevAddon returns null, the pattern match hits Some(addon) with addon = null, the cache is populated with null, and subsequent calls like addon.getConfigJson(...) crash with Cannot read properties of null (reading 'getConfigJson') instead of surfacing the intended error message.
🛠️ Proposed fix — cast to Null.t and convert explicitly
switch loadDevAddon(req, envioPackageDir)
->(Utils.magic: addon => Null.t<addon>)
->Null.toOption {
| Some(addon) => addon
| None =>
JsError.throwWithMessage(`Couldn't load the envio native addon. Install the envio npm package.`)
}Alternatively, change every return null; in the loadDevAddon %raw body to return undefined;.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envio/src/Core.res` around lines 100 - 116, The loadDevAddon %raw
implementation can return JS null which, when cast via (Utils.magic: addon =>
option<addon>), is treated as Some(null) and later causes runtime null derefs
(e.g., when calling addon.getConfigJson); change the cast to treat null as None
by first casting to Null.t and converting to an option: after calling
loadDevAddon(req, envioPackageDir) use (Utils.magic: addon => Null.t<addon>) and
then Null.toOption before the switch so None triggers JsError.throwWithMessage,
or alternatively modify the %raw loadDevAddon to return undefined instead of
null in all failure paths so the existing option cast works correctly.
| module Env = { | ||
| @val | ||
| external // FIXME: process might be missing | ||
| default: dict<string> = "process.env" | ||
| } | ||
|
|
||
| let mixinMissingIssue = (envSafe, issue) => { | ||
| switch envSafe.maybeMissingIssues { | ||
| | Some(missingIssues) => missingIssues->Array.push(issue)->ignore | ||
| | None => envSafe.maybeMissingIssues = Some([issue]) | ||
| } | ||
| } | ||
|
|
||
| let mixinInvalidIssue = (envSafe, issue: invalidIssue) => { | ||
| switch envSafe.maybeInvalidIssues { | ||
| | Some(invalidIssues) => invalidIssues->Array.push(issue)->ignore | ||
| | None => envSafe.maybeInvalidIssues = Some([issue]) | ||
| } | ||
| } | ||
|
|
||
| let make = (~env=Env.default) => { | ||
| {env, isLocked: false, maybeMissingIssues: None, maybeInvalidIssues: None} | ||
| } |
There was a problem hiding this comment.
Env.default crashes on load in non-Node runtimes — worth resolving the FIXME.
external default: dict<string> = "process.env" is evaluated lazily only when make(~env=Env.default) is called, but the moment any caller does EnvSafe.make() with no args, ReScript reads process.env at the call site. If this module is ever loaded from a browser bundle (you already have Stdlib.Window.alert handling for that case on line 28), process is undefined and the whole make() call throws a ReferenceError before any of the "missing env vars" UX runs.
🛡️ Suggested guard
module Env = {
`@val`
- external // FIXME: process might be missing
- default: dict<string> = "process.env"
+ external defaultRaw: Nullable.t<dict<string>> = "globalThis.process?.env"
+ let default = defaultRaw->Nullable.toOption->Option.getOr(Dict.make())
}(or inline the guard inside make to keep this module closer to the upstream source.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module Env = { | |
| @val | |
| external // FIXME: process might be missing | |
| default: dict<string> = "process.env" | |
| } | |
| let mixinMissingIssue = (envSafe, issue) => { | |
| switch envSafe.maybeMissingIssues { | |
| | Some(missingIssues) => missingIssues->Array.push(issue)->ignore | |
| | None => envSafe.maybeMissingIssues = Some([issue]) | |
| } | |
| } | |
| let mixinInvalidIssue = (envSafe, issue: invalidIssue) => { | |
| switch envSafe.maybeInvalidIssues { | |
| | Some(invalidIssues) => invalidIssues->Array.push(issue)->ignore | |
| | None => envSafe.maybeInvalidIssues = Some([issue]) | |
| } | |
| } | |
| let make = (~env=Env.default) => { | |
| {env, isLocked: false, maybeMissingIssues: None, maybeInvalidIssues: None} | |
| } | |
| module Env = { | |
| `@val` | |
| external defaultRaw: Nullable.t<dict<string>> = "globalThis.process?.env" | |
| let default = defaultRaw->Nullable.toOption->Option.getOr(Dict.make()) | |
| } | |
| let mixinMissingIssue = (envSafe, issue) => { | |
| switch envSafe.maybeMissingIssues { | |
| | Some(missingIssues) => missingIssues->Array.push(issue)->ignore | |
| | None => envSafe.maybeMissingIssues = Some([issue]) | |
| } | |
| } | |
| let mixinInvalidIssue = (envSafe, issue: invalidIssue) => { | |
| switch envSafe.maybeInvalidIssues { | |
| | Some(invalidIssues) => invalidIssues->Array.push(issue)->ignore | |
| | None => envSafe.maybeInvalidIssues = Some([issue]) | |
| } | |
| } | |
| let make = (~env=Env.default) => { | |
| {env, isLocked: false, maybeMissingIssues: None, maybeInvalidIssues: None} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envio/src/EnvSafe.res` around lines 62 - 84, Env.default currently
references process.env at module evaluation which throws in non-Node runtimes;
change to avoid reading process at load-time by making Env.default a safe
accessor or moving the process.env access into make (e.g., have Env.default be a
function or set default in make when ~env is not provided); update references to
Env.default and EnvSafe.make so make(~env=Env.default) no longer triggers a
ReferenceError in browsers — ensure the runtime check uses typeof process !==
"undefined" before accessing process.env and keep the exported symbol names
Env.default and make unchanged.
Replace rescript-legacy with the new rescript build system across all packages. Rename deprecated rescript.json fields to the new format: bs-dependencies -> dependencies, bsc-flags -> compiler-flags. Remove unused version field from rescript.json (now rejected by new build system) and prune unused @rescript/react dep from test_codegen. https://claude.ai/code/session_01L49r1QXcBnnkymvLrDEowh
The new ReScript build system requires packages declared in rescript.json dependencies to be directly installed in the project's node_modules. Without this, upward traversal fails to find rescript-schema when running the init template build in CI. https://claude.ai/code/session_01L49r1QXcBnnkymvLrDEowh
The Fuel codegen emitted typeXSchema definitions alongside type declarations, but nothing consumed them: fuel param schemas are resolved at runtime via Internal.fuelSupplyParamsSchema / fuelTransferParamsSchema or raw JSON passthrough for logData. Removing the dead schema generation lets us drop 'open RescriptSchema' from the generated Indexer.res entirely, which in turn removes rescript-schema from user project dependencies. https://claude.ai/code/session_01L49r1QXcBnnkymvLrDEowh
DZakh
force-pushed
the
claude/migrate-rescript-build-AcTU5
branch
from
96ee2c7 to
f78cce8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/hbs_templating/codegen_templates.rs (1)
197-197:⚠️ Potential issue | 🟡 MinorRemove the unused
schema_codefield and its generation.The
schema_codefield is generated viatype_expr.to_rescript_schema(&"t".to_string(), &SchemaMode::ForDb)at line 241 but is never consumed by any HBS template. Remove the field fromEntityRecordTypeTemplate(line 197), the schema generation call (line 241), the struct assignment (line 295), and theSchemaModeimport (line 22, which is now unused in this file).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/hbs_templating/codegen_templates.rs` at line 197, Remove the unused schema_code field from the EntityRecordTypeTemplate struct and delete the generation and assignment that produce it: remove the schema_code declaration in EntityRecordTypeTemplate, remove the type_expr.to_rescript_schema(&"t".to_string(), &SchemaMode::ForDb) call that generates it, and remove the schema_code: ... entry where the struct is constructed; also remove the now-unused SchemaMode import at the top of the file. Ensure no other references to schema_code remain in this module before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/src/hbs_templating/codegen_templates.rs`:
- Line 197: Remove the unused schema_code field from the
EntityRecordTypeTemplate struct and delete the generation and assignment that
produce it: remove the schema_code declaration in EntityRecordTypeTemplate,
remove the type_expr.to_rescript_schema(&"t".to_string(), &SchemaMode::ForDb)
call that generates it, and remove the schema_code: ... entry where the struct
is constructed; also remove the now-unused SchemaMode import at the top of the
file. Ensure no other references to schema_code remain in this module before
committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a09b86c-4085-4792-939e-785aa7b2057e
⛔ Files ignored due to path filters (3)
packages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__indexer_code_generates_correct_types_and_values.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__indexer_code_multiple_chains.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
CLAUDE.mdpackages/build-envio/src/build-artifact.tspackages/cli/src/hbs_templating/codegen_templates.rspackages/cli/templates/dynamic/init_templates/shared/package.json.hbspackages/cli/templates/static/blank_template/rescript/rescript.jsonpackages/envio/package.jsonpackages/envio/rescript.jsonscenarios/fuel_test/package.jsonscenarios/fuel_test/rescript.jsonscenarios/fuel_test/src/Indexer.resscenarios/helpers/package.jsonscenarios/svm_test/package.jsonscenarios/svm_test/rescript.jsonscenarios/test_codegen/package.jsonscenarios/test_codegen/rescript.jsonscenarios/test_codegen/src/Indexer.res
💤 Files with no reviewable changes (2)
- scenarios/fuel_test/src/Indexer.res
- scenarios/test_codegen/src/Indexer.res
✅ Files skipped from review due to trivial changes (11)
- scenarios/svm_test/rescript.json
- packages/envio/package.json
- packages/cli/templates/static/blank_template/rescript/rescript.json
- packages/cli/templates/dynamic/init_templates/shared/package.json.hbs
- scenarios/helpers/package.json
- packages/envio/rescript.json
- scenarios/svm_test/package.json
- scenarios/test_codegen/package.json
- CLAUDE.md
- scenarios/test_codegen/rescript.json
- scenarios/fuel_test/rescript.json
🚧 Files skipped from review as they are similar to previous changes (1)
- scenarios/fuel_test/package.json
After dropping rescript-schema generation from Indexer.res, the to_rescript_schema impls on TypeDeclMulti, TypeDecl, TypeExpr and TypeIdent had no remaining call sites. The schema_code field on EntityRecordTypeTemplate and res_schema_code on field_types::Field were also set but never read. Removes the entire to_rescript_schema family, the SchemaMode enum, the dependencies() helpers (only used for schema topo-sort), and the now-unused fields and tests. https://claude.ai/code/session_01L49r1QXcBnnkymvLrDEowh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/hbs_templating/codegen_templates.rs (1)
643-657: Add a Fuelindexer_codesnapshot for this branch.Lines 643-657 now own the Fuel
Indexer.resemission path, but this test module only snapshots Fuelenvio_dts_codewhile theindexer_code_*snapshots are EVM-only. That leaves removedRescriptSchemareferences or missing ABI type declarations undetected until scenario CI.Suggested test
+ #[test] + fn indexer_code_generated_for_fuel() { + let project_template = get_project_template_helper("fuel-config.yaml"); + insta::assert_snapshot!(project_template.indexer_code); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/hbs_templating/codegen_templates.rs` around lines 643 - 657, The Fuel branch emitting Indexer.res (Abi::Fuel handling in codegen_templates.rs producing the indexer code string) isn't covered by tests—add a snapshot assertion for the Fuel indexer output similar to the existing envio_dts_code snapshot so changes to RescriptSchema or ABI type declarations are caught; specifically, in the test module that currently snapshots envio_dts_code, invoke the same codepath that produces the indexer_code (the string built in the Abi::Fuel arm / the Indexer.res output) and store/assert a snapshot named like indexer_code_fuel (or extend existing indexer_code_* snapshots) to include the Fuel emission. Ensure the test references the Abi::Fuel scenario and captures the formatted string that contains all_abi_type_declarations so ABI type changes are validated.
🤖 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/cli/src/hbs_templating/codegen_templates.rs`:
- Around line 643-657: The Fuel branch emitting Indexer.res (Abi::Fuel handling
in codegen_templates.rs producing the indexer code string) isn't covered by
tests—add a snapshot assertion for the Fuel indexer output similar to the
existing envio_dts_code snapshot so changes to RescriptSchema or ABI type
declarations are caught; specifically, in the test module that currently
snapshots envio_dts_code, invoke the same codepath that produces the
indexer_code (the string built in the Abi::Fuel arm / the Indexer.res output)
and store/assert a snapshot named like indexer_code_fuel (or extend existing
indexer_code_* snapshots) to include the Fuel emission. Ensure the test
references the Abi::Fuel scenario and captures the formatted string that
contains all_abi_type_declarations so ABI type changes are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65a73945-14ad-47bc-ad90-d0e07256c8b9
📒 Files selected for processing (4)
packages/cli/src/config_parsing/entity_parsing.rspackages/cli/src/config_parsing/field_types.rspackages/cli/src/hbs_templating/codegen_templates.rspackages/cli/src/type_schema.rs
💤 Files with no reviewable changes (1)
- packages/cli/src/config_parsing/field_types.rs
SelectedFieldTemplate carried a default_value_rescript field that was populated but never read (no callers, no template reference). Utils.BigInt.nativeSchema only existed to back SchemaMode::ForFieldSelection in the Rust codegen, which is gone — drop it too. https://claude.ai/code/session_01L49r1QXcBnnkymvLrDEowh
2 participants
Summary
Switches from the legacy
rescript-legacyCLI to the new ReScript 12 build system across every package, scenario, and template, and dropsrescript-schemaas a user-facing dependency.Changes
ReScript build system migration
rescript-legacy→rescriptin allpackage.jsonscripts (envio, scenarios, init template)rescript.jsonfield renames:bs-dependencies→dependencies,bsc-flags→compiler-flagsversionfield fromrescript.json(new build system rejects it)packages/build-envio/src/build-artifact.tsnow execs./node_modules/.bin/rescriptpnpm rescriptDropping rescript-schema from generated indexer code
typeXSchemadefs that nothing consumed (params schemas come fromInternal.fuelSupplyParamsSchema/fuelTransferParamsSchemaat runtime, or raw-JSON passthrough for logData). Removed.open RescriptSchemafrom the generatedIndexer.resheader (codegen_templates.rs)rescript-schema-removed from blank templaterescript.jsondeps and init templatepackage.json.hbsdevDepstest_codegen,fuel_test,svm_test) to match new codegen output; droppedrescript-schemafromfuel_testandsvm_testdepsTest plan
cargo test --package envio --lib— 233 passedpnpm rescriptclean build succeeds forpackages/envio,scenarios/helpers,scenarios/test_codegen,scenarios/fuel_test,scenarios/svm_testenvio initoutput layout locally in/tmpand confirmedrescriptbuild succeeds withoutrescript-schemapnpm test:templates) green on PRhttps://claude.ai/code/session_01L49r1QXcBnnkymvLrDEowh
Generated by Claude Code
Summary by CodeRabbit
Chores
Refactor