Migrate from Js.* to modern ReScript APIs - #1104
Merged
Merged
Conversation
Continues the Js.* → ReScript stdlib migration from #1103 for the places where the -3 warning suppression was still left: - scenarios/test_codegen and scenarios/fuel_test: ran rescript-tools migrate-all, plus manual fixes for Array.sort (now returns unit — switched to Array.toSorted) with Int.compare comparators, and string_of_int/float_of_int → Int.toString/Int.toFloat. - packages/cli/templates/static/{codegen,blank_template}/rescript.json: dropped "warnings": -3. Templates regenerate indexer code, so also updated the Rust codegen: - type_schema.rs: Js.Json.t → JSON.t, Js.Date.t → Date.t, Js.Date.fromFloat → Date.fromTime. - codegen_templates.rs: SingleOrMultiple now uses Array.isArray instead of Js.Json.decodeArray, with JSON.t aliases. - Regenerated insta snapshots. https://claude.ai/code/session_01YCbJaSa8Vpi5tSgU8WRWcS
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMigrates ReScript code and tests from JS interop to ReScript stdlib: Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 |
…-flag-SVTy5 # Conflicts: # scenarios/test_codegen/test/rollback/Rollback_test.res
![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: 7
🧹 Nitpick comments (3)
scenarios/test_codegen/test/lib_tests/Persistence_test.res (1)
17-19: Fix typo in assertion message text.
Intialshould beInitialto keep failure output clear.Proposed patch
- t.expect(persistence.storageStatus, ~message=`Intial storage status should be unknown`).toEqual( + t.expect(persistence.storageStatus, ~message=`Initial storage status should be unknown`).toEqual( Persistence.Unknown, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/lib_tests/Persistence_test.res` around lines 17 - 19, Update the assertion message string for the test using t.expect(persistence.storageStatus, ~message=`Intial storage status should be unknown`) to correct the typo: replace "Intial" with "Initial" so the message reads "Initial storage status should be unknown" in the Persistence_test.res assertion.scenarios/test_codegen/test/lib_tests/PgStorage_test.res (1)
118-129: Consolidate this into one assertion for the whole outcome.At Line 118-Line 129, these checks are tightly related and can be expressed as a single assertion to keep failures clearer and align with test style conventions.
♻️ Proposed refactor
- t.expect( - queryAsc, - ~message="Same fields with different directions must produce different index names", - ).not.toBe(queryDesc) - t.expect( - queryAsc->String.includes("\"t_a_b\""), - ~message="ASC index name has no suffix", - ).toBeTruthy() - t.expect( - queryDesc->String.includes("\"t_a_desc_b_desc\""), - ~message="DESC index name encodes direction", - ).toBeTruthy() + t.expect( + { + differentQueries: queryAsc !== queryDesc, + ascNameOk: queryAsc->String.includes("\"t_a_b\""), + descNameOk: queryDesc->String.includes("\"t_a_desc_b_desc\""), + }, + ~message="Index names must differ and encode direction correctly", + ).toEqual({ + differentQueries: true, + ascNameOk: true, + descNameOk: true, + })Based on learnings: Applies to **/*.{test.res,test.js,test.ts,test.tsx,spec.res,spec.js,spec.ts,spec.tsx} : Always use single assert to check the whole value instead of multiple asserts for every field.
🤖 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 118 - 129, The three related assertions around queryAsc and queryDesc should be consolidated into a single assertion that verifies the entire outcome at once: build an expected combined check (e.g., a single string/regex or composed object) that ensures queryAsc !== queryDesc, that queryAsc contains "\"t_a_b\"", and that queryDesc contains "\"t_a_desc_b_desc\"", then replace the three t.expect calls with one t.expect(...).toBeTruthy() (or equivalent) that evaluates that combined condition; refer to the existing symbols queryAsc, queryDesc and their String.includes checks to locate and combine the checks into one assertion.packages/cli/src/hbs_templating/codegen_templates.rs (1)
1658-1681: PreferJSON.Decode.arrayhere instead ofArray.isArray+Utils.magic.
JSON.talready exposes arrays directly in ReScript 12, andJSON.Decode.arrayreturnsoption<array<t>>. Switching to that would remove the unchecked cast from the generated helper and make future refactors safer. (rescript-lang.org)Based on learnings: Always use ReScript 12 documentation. Never suggest ReasonML syntax♻️ Proposed fix
- let rec isMultiple = (t: t<'a>, ~nestedArrayDepth): bool => - if !Array.isArray(t) {{ - false - }} else {{ - let arr = t->(Utils.magic: t<'a> => array<t<'a>>) - if nestedArrayDepth == 0 {{ - true - }} else if arr->Array.length == 0 {{ - AmbiguousEmptyNestedArray->ErrorHandling.mkLogAndRaise( - ~msg="The given empty array could be interperated as a flat array (value) or nested array. Since it's ambiguous, - please pass in a nested empty array if the intention is to provide an empty array as a value", - ) - }} else {{ - arr->Utils.Array.firstUnsafe->isMultiple(~nestedArrayDepth=nestedArrayDepth - 1) - }} - }} + let rec isMultiple = (t: t<'a>, ~nestedArrayDepth): bool => + switch t->JSON.Decode.array {{ + | None => false + | Some(arr) => + if nestedArrayDepth == 0 {{ + true + }} else if arr->Array.length == 0 {{ + AmbiguousEmptyNestedArray->ErrorHandling.mkLogAndRaise( + ~msg="The given empty array could be interperated as a flat array (value) or nested array. Since it's ambiguous, + please pass in a nested empty array if the intention is to provide an empty array as a value", + ) + }} else {{ + arr->Utils.Array.firstUnsafe->isMultiple(~nestedArrayDepth=nestedArrayDepth - 1) + }} + }}🤖 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 1658 - 1681, The isMultiple helper currently checks arrays using Array.isArray plus an unchecked cast via Utils.magic and should be changed to use JSON.Decode.array to safely decode t as an option<array<t>>; replace the Array.isArray branch with a JSON.Decode.array(t) match (None -> false | Some(arr) -> ...), keep the existing nestedArrayDepth logic and error raise via AmbiguousEmptyNestedArray->ErrorHandling.mkLogAndRaise when arr is empty at deeper depth, and remove the Utils.magic cast to eliminate unsafe casting and rely on JSON.Decode.array for safe extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/hbs_templating/codegen_templates.rs`:
- Around line 1675-1677: Update the error message string in
AmbiguousEmptyNestedArray->ErrorHandling.mkLogAndRaise to correct the typo:
replace "interperated" with "interpreted" in the ~msg text so the raised message
reads "...could be interpreted as a flat array (value) or nested array..."
ensuring the rest of the message stays unchanged.
In `@scenarios/test_codegen/test/ChainManager_test.res`:
- Line 61: The event string uses an un-annotated magic cast; change the
expression `...->Utils.magic` to include an explicit ReScript cast signature
like `...->(Utils.magic: string => eventType)` (replace eventType with the
appropriate target type), referencing the `Utils.magic` usage on the `event`
field so the cast is explicit (i.e., annotate the input as string and the
desired output type).
In `@scenarios/test_codegen/test/Config_test.res`:
- Line 189: Update the unannotated uses of Utils.magic by adding explicit cast
signatures of the form (Utils.magic: inputType => outputType) for each modified
test param; specifically replace {"from": "0xabc", "to": "0xdef", "value":
100n}->Utils.magic with {"from": "0xabc", "to": "0xdef", "value":
100n}->(Utils.magic: {from:string,to:string,value:int} => <expectedOutputType>),
{"id": 1n, "details": ("hello","world")}->Utils.magic with ->(Utils.magic:
{id:int,details:(string,string)} => <expectedOutputType>), {"data":
(1n,(2n,"hello"))}->Utils.magic with ->(Utils.magic: {data:(int,(int,string))}
=> <expectedOutputType>), and {"ids":[1n,2n,3n]}->Utils.magic with
->(Utils.magic: {ids: list<int>} => <expectedOutputType>); also check and
annotate the empty-tuple case `()->Utils.magic` similarly. Replace
<expectedOutputType> with the actual expected return types for each test.
In `@scenarios/test_codegen/test/HyperSync_test.res`:
- Line 39: The test currently uses Console.log(page) which doesn't verify
behavior; replace that log with ReScript assertions using the Assert module to
validate the expected shape or error for the `page` value (e.g., check specific
fields like `page.id`, `page.status`, or that `page` is an error/empty shape).
Locate the `Console.log(page)` call in the test (in HyperSync_test.res) and swap
it for one or more Assert.* checks (e.g., Assert.equal, Assert.assert, or
Assert.exceptions) that express the contract for broken-transaction handling so
the test fails when the actual `page` value deviates from expectations.
In `@scenarios/test_codegen/test/HyperSyncSource_test.res`:
- Line 188: The topic1 entry uses Utils.magic without an explicit cast; update
the topic1 value (the mockAddress0->Utils.magic usage) to use the explicit
typed-cast form required by our ReScript guideline: wrap the Utils.magic use
with an explicit type annotation showing the input and output types (i.e.,
convert mockAddress0->Utils.magic into mockAddress0->(Utils.magic: inputType =>
outputType)), so the conversion boundary on topic1 is explicit and typed.
In `@scenarios/test_codegen/test/lib_tests/PgStorage_test.res`:
- Around line 693-695: The assertion message incorrectly says "Should return
empty string when no chain configs provided" while the test asserts that query
is None; update the expectation message for the t.expect(...).toBe(None)
assertion (the one referencing variable `query` in PgStorage_test.res) to
accurately describe the asserted value (e.g., "Should return None when no chain
configs provided") so test failures report the correct expectation.
In `@scenarios/test_codegen/test/LoadLayer_test.res`:
- Line 362: Replace all untyped Utils.magic casts of the form "fieldValue":
"123"->Utils.magic (and the other similar occurrences) with explicit annotated
casts using the ReScript pattern value->(Utils.magic: inputType => outputType);
update each instance to use the correct inputType and outputType for that field
(for example "123"->(Utils.magic: string => int) if converting string to int) so
each "fieldValue": ... entry uses the explicit cast form; search for the same
untyped ->Utils.magic token in the file and apply the same transformation to
each occurrence.
---
Nitpick comments:
In `@packages/cli/src/hbs_templating/codegen_templates.rs`:
- Around line 1658-1681: The isMultiple helper currently checks arrays using
Array.isArray plus an unchecked cast via Utils.magic and should be changed to
use JSON.Decode.array to safely decode t as an option<array<t>>; replace the
Array.isArray branch with a JSON.Decode.array(t) match (None -> false |
Some(arr) -> ...), keep the existing nestedArrayDepth logic and error raise via
AmbiguousEmptyNestedArray->ErrorHandling.mkLogAndRaise when arr is empty at
deeper depth, and remove the Utils.magic cast to eliminate unsafe casting and
rely on JSON.Decode.array for safe extraction.
In `@scenarios/test_codegen/test/lib_tests/Persistence_test.res`:
- Around line 17-19: Update the assertion message string for the test using
t.expect(persistence.storageStatus, ~message=`Intial storage status should be
unknown`) to correct the typo: replace "Intial" with "Initial" so the message
reads "Initial storage status should be unknown" in the Persistence_test.res
assertion.
In `@scenarios/test_codegen/test/lib_tests/PgStorage_test.res`:
- Around line 118-129: The three related assertions around queryAsc and
queryDesc should be consolidated into a single assertion that verifies the
entire outcome at once: build an expected combined check (e.g., a single
string/regex or composed object) that ensures queryAsc !== queryDesc, that
queryAsc contains "\"t_a_b\"", and that queryDesc contains
"\"t_a_desc_b_desc\"", then replace the three t.expect calls with one
t.expect(...).toBeTruthy() (or equivalent) that evaluates that combined
condition; refer to the existing symbols queryAsc, queryDesc and their
String.includes checks to locate and combine the checks into one assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2266f01-28c5-4c5d-b192-5f7bf9c89379
⛔ Files ignored due to path filters (2)
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!**/*.snap
📒 Files selected for processing (46)
packages/cli/src/hbs_templating/codegen_templates.rspackages/cli/src/type_schema.rspackages/cli/templates/static/blank_template/rescript/rescript.jsonpackages/cli/templates/static/codegen/rescript.jsonscenarios/fuel_test/rescript.jsonscenarios/fuel_test/test/HyperFuelSource_test.resscenarios/test_codegen/rescript.jsonscenarios/test_codegen/src/handlers/EventHandlers.resscenarios/test_codegen/test/BlockLag_test.resscenarios/test_codegen/test/ChainManager_test.resscenarios/test_codegen/test/Config_test.resscenarios/test_codegen/test/E2E_test.resscenarios/test_codegen/test/EntityColumnTypes_test.resscenarios/test_codegen/test/EventFilters_test.resscenarios/test_codegen/test/EventOrigin_test.resscenarios/test_codegen/test/HandlerTypes_test.resscenarios/test_codegen/test/HyperSyncSource_test.resscenarios/test_codegen/test/HyperSync_test.resscenarios/test_codegen/test/Indexer_test.resscenarios/test_codegen/test/LoadLayer_test.resscenarios/test_codegen/test/OptionalBlockParams_test.resscenarios/test_codegen/test/RawEventsTableMigration_test.resscenarios/test_codegen/test/ReorgDetection_test.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/Utils_test.resscenarios/test_codegen/test/Viem_test.resscenarios/test_codegen/test/WriteRead_test.resscenarios/test_codegen/test/__mocks__/MockConfig.resscenarios/test_codegen/test/__mocks__/MockEvents.resscenarios/test_codegen/test/fixtures/LogTesting.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/lib_tests/ClickHouse_test.resscenarios/test_codegen/test/lib_tests/EventRouter_test.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.resscenarios/test_codegen/test/lib_tests/Persistence_test.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/lib_tests/Rpc_Test.resscenarios/test_codegen/test/lib_tests/SingleOrMultiple_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/lib_tests/Throttler_test.resscenarios/test_codegen/test/rollback/ChainDataHelpers.resscenarios/test_codegen/test/rollback/MockChainData_test.resscenarios/test_codegen/test/rollback/Rollback_test.resscenarios/test_codegen/test/schema_types/BigDecimal_test.resscenarios/test_codegen/test/schema_types/Timestamp_test.res
💤 Files with no reviewable changes (1)
- packages/cli/templates/static/codegen/rescript.json
| logIndex, | ||
| eventConfig: Utils.magic("Mock eventConfig in ChainManager test"), | ||
| event: `mock event (chainId)${id->Int.toString} - (blockNumber)${currentBlockNumber.contents->string_of_int} - (logIndex)${logIndex->string_of_int} - (timestamp)${currentTime.contents->string_of_int}`->Utils.magic, | ||
| event: `mock event (chainId)${id->Int.toString} - (blockNumber)${currentBlockNumber.contents->Int.toString} - (logIndex)${logIndex->Int.toString} - (timestamp)${currentTime.contents->Int.toString}`->Utils.magic, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add an explicit type annotation to this Utils.magic cast.
Line 61 uses ->Utils.magic without a cast signature. Please annotate it inline as value->(Utils.magic: inputType => outputType).
As per coding guidelines **/*.res: “When using Utils.magic for type casting in ReScript, always add explicit type annotations: value->(Utils.magic: inputType => outputType)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scenarios/test_codegen/test/ChainManager_test.res` at line 61, The event
string uses an un-annotated magic cast; change the expression `...->Utils.magic`
to include an explicit ReScript cast signature like `...->(Utils.magic: string
=> eventType)` (replace eventType with the appropriate target type), referencing
the `Utils.magic` usage on the `event` field so the cast is explicit (i.e.,
annotate the input as string and the desired output type).
| // Serialize a params object to JSON via the schema | ||
| let testParams: Internal.eventParams = {"from": "0xabc", "to": "0xdef", "value": 100n}->Utils.magic | ||
| let testParams: Internal.eventParams = | ||
| {"from": "0xabc", "to": "0xdef", "value": 100n}->Utils.magic |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Unannotated Utils.magic calls:"
rg -nP '\->\s*Utils\.magic\b' scenarios/test_codegen/test/Config_test.res || true
echo
echo "Annotated Utils.magic calls:"
rg -nP '\->\s*\(Utils\.magic\s*:' scenarios/test_codegen/test/Config_test.res || trueRepository: enviodev/hyperindex
Length of output: 859
🏁 Script executed:
sed -n '185,235p' scenarios/test_codegen/test/Config_test.resRepository: enviodev/hyperindex
Length of output: 2457
🏁 Script executed:
git diff HEAD -- scenarios/test_codegen/test/Config_test.res | head -100Repository: enviodev/hyperindex
Length of output: 45
🏁 Script executed:
git statusRepository: enviodev/hyperindex
Length of output: 262
🏁 Script executed:
sed -n '240,270p' scenarios/test_codegen/test/Config_test.resRepository: enviodev/hyperindex
Length of output: 1311
🏁 Script executed:
rg -nP '->Utils\.magic' scenarios/test_codegen/test/Config_test.res | head -20Repository: enviodev/hyperindex
Length of output: 467
🏁 Script executed:
rg -n '\->Utils\.magic' scenarios/test_codegen/test/Config_test.res | head -20Repository: enviodev/hyperindex
Length of output: 798
🏁 Script executed:
sed -n '194,204p' scenarios/test_codegen/test/Config_test.resRepository: enviodev/hyperindex
Length of output: 581
Add explicit type annotations for Utils.magic on modified test params.
The following testParams assignments lack explicit cast signatures, which weakens type safety:
- Line 189:
{"from": "0xabc", "to": "0xdef", "value": 100n}->Utils.magic - Line 207:
{"id": 1n, "details": ("hello", "world")}->Utils.magic - Line 217:
{"data": (1n, (2n, "hello"))}->Utils.magic - Line 227:
{"ids": [1n, 2n, 3n]}->Utils.magic
Proposed fix
- let testParams: Internal.eventParams =
- {"from": "0xabc", "to": "0xdef", "value": 100n}->Utils.magic
+ let testParams: Internal.eventParams =
+ ({"from": "0xabc", "to": "0xdef", "value": 100n}: {"from": string, "to": string, "value": bigint})->(Utils.magic: {"from": string, "to": string, "value": bigint} => Internal.eventParams)
- let testParams: Internal.eventParams = {"id": 1n, "details": ("hello", "world")}->Utils.magic
+ let testParams: Internal.eventParams = ({"id": 1n, "details": ("hello", "world")}: {"id": bigint, "details": (string, string)})->(Utils.magic: {"id": bigint, "details": (string, string)} => Internal.eventParams)
- let testParams: Internal.eventParams = {"data": (1n, (2n, "hello"))}->Utils.magic
+ let testParams: Internal.eventParams = ({"data": (1n, (2n, "hello"))}: {"data": (bigint, (bigint, string))})->(Utils.magic: {"data": (bigint, (bigint, string))} => Internal.eventParams)
- let testParams: Internal.eventParams = {"ids": [1n, 2n, 3n]}->Utils.magic
+ let testParams: Internal.eventParams = ({"ids": [1n, 2n, 3n]}: {"ids": array<bigint>})->(Utils.magic: {"ids": array<bigint>} => Internal.eventParams)Per the guideline: **/*.res requires explicit type annotations when using Utils.magic: value->(Utils.magic: inputType => outputType).
Note: Line 196 also contains an unannotated Utils.magic testParams assignment with an empty tuple ()->Utils.magic and may need the same treatment if it is within the scope of changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scenarios/test_codegen/test/Config_test.res` at line 189, Update the
unannotated uses of Utils.magic by adding explicit cast signatures of the form
(Utils.magic: inputType => outputType) for each modified test param;
specifically replace {"from": "0xabc", "to": "0xdef", "value":
100n}->Utils.magic with {"from": "0xabc", "to": "0xdef", "value":
100n}->(Utils.magic: {from:string,to:string,value:int} => <expectedOutputType>),
{"id": 1n, "details": ("hello","world")}->Utils.magic with ->(Utils.magic:
{id:int,details:(string,string)} => <expectedOutputType>), {"data":
(1n,(2n,"hello"))}->Utils.magic with ->(Utils.magic: {data:(int,(int,string))}
=> <expectedOutputType>), and {"ids":[1n,2n,3n]}->Utils.magic with
->(Utils.magic: {ids: list<int>} => <expectedOutputType>); also check and
annotate the empty-tuple case `()->Utils.magic` similarly. Replace
<expectedOutputType> with the actual expected return types for each test.
| ) | ||
|
|
||
| Js.log(page) | ||
| Console.log(page) |
Contributor
There was a problem hiding this comment.
Replace logging with assertions in this test.
Console.log(page) does not verify behavior, so this test can pass without checking the broken-transaction handling contract. Please assert on expected page fields (or expected error/empty shape) using Assert instead of logging.
Suggested change
- Console.log(page)
+ Assert.isTrue(Array.length(page.data.logs) >= 0)
+ Assert.equal(page.nextBlock, Some(12403140))Based on learnings: “In ReScript tests, never log — use Assert module for all verifications.”
📝 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.
Suggested change
| Console.log(page) | |
| Assert.isTrue(Array.length(page.data.logs) >= 0) | |
| Assert.equal(page.nextBlock, Some(12403140)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scenarios/test_codegen/test/HyperSync_test.res` at line 39, The test
currently uses Console.log(page) which doesn't verify behavior; replace that log
with ReScript assertions using the Assert module to validate the expected shape
or error for the `page` value (e.g., check specific fields like `page.id`,
`page.status`, or that `page` is an error/empty shape). Locate the
`Console.log(page)` call in the test (in HyperSync_test.res) and swap it for one
or more Assert.* checks (e.g., Assert.equal, Assert.assert, or
Assert.exceptions) that express the contract for broken-transaction handling so
the test fails when the actual `page` value deviates from expectations.
| topicSelections: [ | ||
| { | ||
| topic0: ["event 2"->EvmTypes.Hex.fromStringUnsafe], | ||
| topic1: [mockAddress0->Utils.magic], |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make the Utils.magic cast explicit at this topic field.
Line 188 should use a typed cast form to keep the conversion boundary explicit.
Suggested change
- topic1: [mockAddress0->Utils.magic],
+ topic1: [mockAddress0->(Utils.magic: Address.t => EvmTypes.Hex.t)],As per coding guidelines **/*.res: “When using Utils.magic for type casting in ReScript, always add explicit type annotations: value->(Utils.magic: inputType => outputType)”.
📝 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.
Suggested change
| topic1: [mockAddress0->Utils.magic], | |
| topic1: [mockAddress0->(Utils.magic: Address.t => EvmTypes.Hex.t)], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scenarios/test_codegen/test/HyperSyncSource_test.res` at line 188, The topic1
entry uses Utils.magic without an explicit cast; update the topic1 value (the
mockAddress0->Utils.magic usage) to use the explicit typed-cast form required by
our ReScript guideline: wrap the Utils.magic use with an explicit type
annotation showing the input and output types (i.e., convert
mockAddress0->Utils.magic into mockAddress0->(Utils.magic: inputType =>
outputType)), so the conversion boundary on topic1 is explicit and typed.
Comment on lines
+693
to
+695
| t.expect(query, ~message="Should return empty string when no chain configs provided").toBe( | ||
| None, | ||
| ) |
Contributor
There was a problem hiding this comment.
Fix the assertion message to match the asserted value type.
At Line 693, the message says “empty string” but the assertion checks None. This can mislead test failure triage.
📝 Proposed fix
- t.expect(query, ~message="Should return empty string when no chain configs provided").toBe(
+ t.expect(query, ~message="Should return None when no chain configs are provided").toBe(
None,
)📝 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.
Suggested change
| t.expect(query, ~message="Should return empty string when no chain configs provided").toBe( | |
| None, | |
| ) | |
| t.expect(query, ~message="Should return None when no chain configs are provided").toBe( | |
| None, | |
| ) |
🤖 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 693 -
695, The assertion message incorrectly says "Should return empty string when no
chain configs provided" while the test asserts that query is None; update the
expectation message for the t.expect(...).toBe(None) assertion (the one
referencing variable `query` in PgStorage_test.res) to accurately describe the
asserted value (e.g., "Should return None when no chain configs provided") so
test failures report the correct expectation.
| t.expect(storageMock.loadByFieldOrThrowCalls).toEqual([ | ||
| { | ||
| "fieldName": "id", | ||
| "fieldValue": "123"->Utils.magic, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use explicit Utils.magic annotations for these field-value casts.
Lines 362, 368, 398, 467, and 473 currently use untyped ->Utils.magic. Please switch each to the explicit cast form value->(Utils.magic: inputType => outputType).
As per coding guidelines **/*.res: “When using Utils.magic for type casting in ReScript, always add explicit type annotations: value->(Utils.magic: inputType => outputType)”.
Also applies to: 368-368, 398-398, 467-467, 473-473
🤖 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 362, Replace all
untyped Utils.magic casts of the form "fieldValue": "123"->Utils.magic (and the
other similar occurrences) with explicit annotated casts using the ReScript
pattern value->(Utils.magic: inputType => outputType); update each instance to
use the correct inputType and outputType for that field (for example
"123"->(Utils.magic: string => int) if converting string to int) so each
"fieldValue": ... entry uses the explicit cast form; search for the same untyped
->Utils.magic token in the file and apply the same transformation to each
occurrence.
The migration from Array.sort with subtraction to Array.toSorted with Int.compare changed the runtime behaviour: the old a - b on Obj.magic strings relied on JS numeric coercion, so "137" - "1337" = -1200 sorted numerically. Int.compare uses the < primitive which compares strings lexicographically, so "1337" < "137" and the metric rows came out in the wrong order, breaking the multichain rollback test. Parse the chainId/value strings with Int.fromString before comparing so the sort keeps its original numeric semantics. https://claude.ai/code/session_01YCbJaSa8Vpi5tSgU8WRWcS
Typo was pre-existing in the template (from #1071) but my previous commit moved the string, so address the CodeRabbit review comment while it is still topical. https://claude.ai/code/session_01YCbJaSa8Vpi5tSgU8WRWcS
DZakh
pushed a commit
that referenced
this pull request
Apr 15, 2026
Conflicts resolved: - EventConfigBuilder.res (top): main's #1104 dropped `open Belt` and removed the empty-component-type stub. Kept the `eventParamComponent` / `eventParam.components` definitions from this PR (the feature) and dropped `open Belt` to follow main's modern-API migration. Migrated the component helpers (`componentsToSimulateSchema`, `componentsToDefaultValue`, `componentsToRemapper`, `paramsToRemap`) from `Js.String2.*`, `Js.Array2.*`, `Js.Dict.*` to the modern `String.*` / `Array.*` / `Dict.*` APIs to match main's style; flipped `Array.forEachWithIndex` argument order from `(i, c)` to `(c, i)` to match the modern signature. - EventConfigBuilder.res (buildSimulateParamsSchema): kept this PR's component-aware schema/default selection but used main's `Dict.set` (was `Js.Dict.set`). - EventHandlers.ts: both sides added independent registrations near the bottom of the file. Kept both — this PR's `#538` Solidity-struct regression handler and main's #1105 `indexer.onBlock` test registrations. - codegen_templates.rs (test): main's #1106 added a third `&CapitalizedOptions` contract_name argument to `EventTemplate::from_config_event`. Updated the new `event_template_named_struct_rescript_snapshot` test to pass a `SablierLockup` contract name. - snapshot regenerated for `event_template_named_struct_rescript_snapshot` to capture main's #1106 `onEventWhere` API refactor (chain-object callback rather than flat args). https://claude.ai/code/session_01QHeJe9qkDbozn6YP8uxdeg
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.
Summary
This PR modernizes the codebase by migrating from deprecated
Js.*APIs to their modern ReScript equivalents. This includes updates to exception handling, dictionary operations, array methods, date handling, and JSON types.Key Changes
Js.Exn.raiseError()withJsError.throwWithMessage()for more idiomatic error throwingJs.DicttoDictmodule (e.g.,Js.Dict.get()→Dict.get(),Js.Dict.empty()→Dict.make(),Js.Dict.fromArray()→Dict.fromArray())Js.Array2.map()toArray.map()for consistency with modern ReScript stdlibJs.Date.fromString()toDate.fromString()Js.Json.ttoJSON.tin type definitionsOption.getExnwithOption.getOrThrowfor better semanticsJs.log()toConsole.log()warningsconfiguration fromrescript.jsonfiles and cleaned upbs-dependenciesconfigurationNotable Implementation Details
https://claude.ai/code/session_01YCbJaSa8Vpi5tSgU8WRWcS
Summary by CodeRabbit