Handler type-checking for mock indexer fixtures (review fixes) - #1475
Merged
DZakh merged 1 commit intoJul 23, 2026
Conversation
Collapse the `parse_config_yaml` + `generate_indexer_types` napi exports into a single `from_user_api` that parses the inline config once and, when asked, returns the generated `.envio/types.d.ts` alongside the public config JSON — removing the double-parse and the risk of the runtime config drifting from the types. Drop the unused `isRescript` option from the test helper and napi surface. Narrow the handler type-check to diagnostics inferred in the handler source (plus global config errors); the generated `.d.ts` is trusted, not re-checked. Assert the TypeChecker's TypeScript version matches the init-template pin so the two can't silently diverge, and cache the unchanged declaration graph + reuse the prior program across fixtures so each check no longer re-parses `envio` and `@types/node`. Assert the exact diagnostic in the negative handler test, and rename the config-validation test to UserApiValidation_test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018jWRbYWgh5DQ74GF6NDdu2
Contributor
📝 WalkthroughWalkthroughThe config parser now exposes a ChangesUser API configuration flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TestHelper
participant Core
participant NAPI
participant ConfigParser
participant TypeScriptChecker
TestHelper->>Core: fromUserApi(configYaml, schema, env, files)
Core->>NAPI: request config and indexer types
NAPI->>ConfigParser: parse inline YAML inputs
ConfigParser-->>NAPI: public config JSON
NAPI-->>Core: config JSON and indexer types
Core-->>TestHelper: parsed runtime config
TestHelper->>TypeScriptChecker: checkHandlerTypes(types, handlers)
TypeScriptChecker-->>TestHelper: diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/envio/src/Core.res (1)
215-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove behavior-narrating ReScript comments.
These comments restate code behavior rather than documenting a non-obvious constraint or workaround.
packages/envio/src/Core.res#L215-L217: remove the generated-types behavior description.packages/envio-tests/test/helpers/InternalTestIndexer.res#L1-L2: remove the module-purpose comments.packages/envio-tests/test/helpers/InternalTestIndexer.res#L11-L14: remove the function behavior comments.As per coding guidelines,
**/*.rescomments should only capture non-obvious constraints, subtle invariants, or specific workarounds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/Core.res` around lines 215 - 217, Remove the behavior-narrating comments from packages/envio/src/Core.res lines 215-217, packages/envio-tests/test/helpers/InternalTestIndexer.res lines 1-2, and packages/envio-tests/test/helpers/InternalTestIndexer.res lines 11-14. Leave the surrounding ReScript code unchanged and retain only comments documenting non-obvious constraints, invariants, or workarounds.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/envio-tests/test/MockIndexerHandlers_test.res`:
- Around line 24-56: Update the tests in the “InternalTestIndexer handler
type-checking” and nonexistent-event cases to use the Assert module instead of
t.expect, and combine the handler’s separate event field type checks into one
tuple or record equality assertion. Preserve the existing expected config name
and exact diagnostic message while ensuring each test performs a single
whole-value verification.
---
Nitpick comments:
In `@packages/envio/src/Core.res`:
- Around line 215-217: Remove the behavior-narrating comments from
packages/envio/src/Core.res lines 215-217,
packages/envio-tests/test/helpers/InternalTestIndexer.res lines 1-2, and
packages/envio-tests/test/helpers/InternalTestIndexer.res lines 11-14. Leave the
surrounding ReScript code unchanged and retain only comments documenting
non-obvious constraints, invariants, or workarounds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ed40853-623d-482c-b05f-1af8b5c08166
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/cli/src/hbs_templating/codegen_templates.rspackages/cli/src/napi.rspackages/envio-tests/package.jsonpackages/envio-tests/test/MockIndexerHandlers_test.respackages/envio-tests/test/UserApiValidation_test.respackages/envio-tests/test/helpers/InternalTestIndexer.respackages/envio-tests/test/helpers/MockIndexerConfig.respackages/envio-tests/test/helpers/TypeChecker.tspackages/envio-tests/test/lib_tests/ColumnNameFormat_test.respackages/envio/src/Core.resscenarios/test_codegen/test/YamlConfigIndexer_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/lib_tests/ClickHouse_test.res
💤 Files with no reviewable changes (2)
- scenarios/test_codegen/test/helpers/MockIndexer.res
- packages/envio-tests/test/helpers/MockIndexerConfig.res
Comment on lines
+24
to
+56
| describe("InternalTestIndexer handler type-checking", () => { | ||
| it("accepts handlers that match the generated indexer types", t => { | ||
| let {config} = InternalTestIndexer.fromUserApi( | ||
| ~schema, | ||
| ~handlers=` | ||
| import { indexer } from "envio"; | ||
| import type { Address } from "envio"; | ||
| import { expectType, type TypeEqual } from "ts-expect"; | ||
|
|
||
| indexer.onEvent({ contract: "Token", event: "Transfer" }, async ({ event, context }) => { | ||
| expectType<TypeEqual<typeof event.params.value, bigint>>(true); | ||
| expectType<TypeEqual<typeof event.params.from, Address>>(true); | ||
| context.Account.set({ id: event.params.to, balance: event.params.value }); | ||
| }); | ||
| `, | ||
| ~configYaml=yaml, | ||
| ) | ||
| t.expect(config.name).toBe("mock-handlers") | ||
| }) | ||
|
|
||
| it("throws the exact diagnostic on a nonexistent event", t => { | ||
| t.expect( | ||
| () => | ||
| InternalTestIndexer.fromUserApi( | ||
| ~schema, | ||
| ~handlers=` | ||
| import { indexer } from "envio"; | ||
| indexer.onEvent({ contract: "Token", event: "Nonexistent" }, async () => {}); | ||
| `, | ||
| ~configYaml=yaml, | ||
| )->ignore, | ||
| ).toThrowError(`Type '"Nonexistent"' is not assignable to type '"Transfer"'`) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use one Assert-based whole-value verification.
This new _test.res uses t.expect, and the handler checks event fields with separate assertions. Use the Assert module and combine the type checks into one tuple/record equality assertion.
As per coding guidelines, “Always use single assert to check the whole value instead of multiple asserts for every field” and “In tests, never log — use Assert module for all verifications.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/envio-tests/test/MockIndexerHandlers_test.res` around lines 24 - 56,
Update the tests in the “InternalTestIndexer handler type-checking” and
nonexistent-event cases to use the Assert module instead of t.expect, and
combine the handler’s separate event field type checks into one tuple or record
equality assertion. Preserve the existing expected config name and exact
diagnostic message while ensuring each test performs a single whole-value
verification.
Source: Coding guidelines
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 007f664186
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| type fromUserApiResult = { | ||
| config: string, | ||
| indexerTypes: Null.t<string>, |
There was a problem hiding this comment.
When withIndexerTypes is false, the Rust result has indexer_types: None, and napi-rs omits that object property rather than setting it to null. This binding models the field as Null.t<string>, so a ReScript caller that does indexerTypes->Null.toOption will treat the omitted undefined value as present and pass undefined where a string is expected; model this as the repo's null-or-undefined nullable type or return an explicit null from Rust.
Useful? React with 👍 / 👎.
DZakh
changed the base branch from
main
to
claude/mock-indexer-handlers-plan-shi26i
July 23, 2026 13:58
DZakh
merged commit Jul 23, 2026
4341017
into
claude/mock-indexer-handlers-plan-shi26i
8 checks passed
DZakh
added a commit
that referenced
this pull request
Jul 23, 2026
* Add optional handlers type-checking to MockIndexerConfig Lets a mock indexer config type-check TypeScript handler source against the `indexer` surface generated from its own config + schema. Each check runs in an isolated in-process TS program, so independent configs never collide on the global `envio` module augmentation. - napi: `generate_indexer_types` returns the generated `.envio/types.d.ts` for an inline config (reuses `ProjectTemplate::from_config`), mirroring `parse_config_yaml`'s inputs. - Core: `generateIndexerTypes` binding. - envio-tests: `TypeChecker.ts` type-checks handlers via the TS compiler API with the init-template tsconfig options; `MockIndexerConfig.parseYaml` gains `~handlers`, throwing on any type error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QM68d5PWeGDSiVGh5xDiM4 * Rename MockIndexerConfig.parseYaml to MockIndexerFixture.fromYaml The helper now builds a mock indexer fixture (config + optional handler type-check), so the `Config` suffix and `parseYaml` name no longer fit. Updates all call sites in envio-tests and test_codegen. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QM68d5PWeGDSiVGh5xDiM4 * Rename fixture to InternalTestIndexer.fromUserApi with labeled ~configYaml - MockIndexerFixture -> InternalTestIndexer, fromYaml -> fromUserApi (it builds a fixture through the user-facing public boundary). - Make the config YAML a labeled ~configYaml argument at all call sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QM68d5PWeGDSiVGh5xDiM4 * Anchor TypeChecker's compiler host to the fixture package `createCompilerHost`'s current directory defaults to `process.cwd()`, so `types: ["node"]` resolution depended on the launching package rather than envio-tests — a caller without `@types/node` reachable from its cwd got a spurious global TS2688. Point the host at helpersDir so type-root lookup is stable across packages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QM68d5PWeGDSiVGh5xDiM4 * Rename ConfigYaml_test to FromUserApi_test Matches the module it exercises (InternalTestIndexer.fromUserApi). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QM68d5PWeGDSiVGh5xDiM4 * Drop parser-origin comment on MockIndexer customConfig The comment only named which callers supply the value; the line below already documents its non-obvious default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QM68d5PWeGDSiVGh5xDiM4 * Address review feedback on mock-indexer handler type-checking (#1475) Collapse the `parse_config_yaml` + `generate_indexer_types` napi exports into a single `from_user_api` that parses the inline config once and, when asked, returns the generated `.envio/types.d.ts` alongside the public config JSON — removing the double-parse and the risk of the runtime config drifting from the types. Drop the unused `isRescript` option from the test helper and napi surface. Narrow the handler type-check to diagnostics inferred in the handler source (plus global config errors); the generated `.d.ts` is trusted, not re-checked. Assert the TypeChecker's TypeScript version matches the init-template pin so the two can't silently diverge, and cache the unchanged declaration graph + reuse the prior program across fixtures so each check no longer re-parses `envio` and `@types/node`. Assert the exact diagnostic in the negative handler test, and rename the config-validation test to UserApiValidation_test. Claude-Session: https://claude.ai/code/session_018jWRbYWgh5DQ74GF6NDdu2 Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Lets a mock-indexer test fixture type-check TypeScript handler source (
import { indexer } from "envio"; indexer.onEvent(...)) against theindexersurface generated from its own inlineconfig.yaml+schema.graphql. This is the type-test half of the mock-indexer tooling; actually running the handlers is intentionally out of scope for now.Any handler type error throws with the compiler diagnostics.
How
from_user_api(napi) +Core.fromUserApi— parses an inline config once and returns{ config, indexerTypes }. WithwithIndexerTypes,indexerTypesis the exact.envio/types.d.tsproduction codegen emits (viaProjectTemplate::from_config); otherwise it's absent. A single napi call, so the runtime config and the generated types can't drift from two separate parses. No filesystem access.TypeChecker.ts— type-checks the handlers in an isolated, in-processts.Programusing the same compiler options as the init-templatetsconfig.json, and asserts its TypeScript version matches the init-template pin so the two can't silently diverge. The generateddeclare module "envio"augmentation is injected as a virtual file, so each fixture bindsindexerto its own config for that program only — independent fixtures never collide on the global module augmentation. Only diagnostics inferred in the handler source (plus global config errors) are reported; the generated.d.tsis trusted, not re-checked. The unchangedenvio+@types/nodedeclaration graph is parsed once and reused across fixtures (source-file cache + prior-program reuse), so each check doesn't re-parse it.InternalTestIndexer.fromUserApi(renamed fromMockIndexerConfig.parseYaml) — gains an optional~handlers; throws on any type error.Tests
MockIndexerHandlers_test: a clean handler set type-checks, and a nonexistent event name throws the exact diagnosticType '"Nonexistent"' is not assignable to type '"Transfer"'.UserApiValidation_test(renamed fromConfigYaml_test) andColumnNameFormat_testmoved to the renamed helper.envio_types_dtssnapshots (evm/fuel/svm) unchanged.Notes
isRescript, handler-scoped diagnostics, TS-version pin assertion, cross-fixture program reuse, and an exact-diagnostic negative test.🤖 Generated with Claude Code
https://claude.ai/code/session_018jWRbYWgh5DQ74GF6NDdu2
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests