Sort items received from data source #694
Conversation
WalkthroughRenames reversed* arrays to forward-ordered names, switches contract-register and event processing loops to forward iteration, and changes queue maintenance from merge to concatenate-plus-sort using a new comparator. Public APIs updated to use newItems labels. Tests adjusted and a new sorting test added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DS as Data Source
participant GS as GlobalState
participant CF as ChainFetcher
participant FS as FetchState
DS->>GS: PartitionQueryResponse (raw events)
GS->>GS: Build itemsWithContractRegister, newItems (forward order)
GS->>CF: runContractRegistersOrThrow(~itemsWithContractRegister)
CF->>CF: Iterate forward and process registrations
GS->>CF: handleQueryResult(~newItems, ...)
CF->>FS: FetchState.handleQueryResult(~newItems, ...)
FS->>FS: queue := queue ++ newItems
FS->>FS: sort(queue) by (blockNumber desc, logIndex desc)
FS-->>CF: updated state
CF-->>GS: updated state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
1911-1949: Good addition: verifies sorting of unsorted inputsThe new test robustly asserts the queue is sorted DESC by (blockNumber, logIndex) regardless of input order. This covers the new concatenate+sort strategy. Consider adding one more case with equal (blockNumber, logIndex) duplicates to assert duplicates are preserved as-is (idempotency), but that’s optional.
codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs (1)
321-321: Rename and forward iteration alignment: OK
- Switched from
reversedWithContractRegister+unshifttoitemsWithContractRegister+pushand forward iteration. This removes reversal semantics and matches the new forward-processing approach in ChainFetcher.- Passing
~itemsWithContractRegistertoChainFetcher.runContractRegistersOrThrowis consistent with the updated signature.Optional: If test inputs can arrive in arbitrary order, you may sort
itemsWithContractRegisterby (blockNumber asc, logIndex asc) before invoking registers to guarantee early registrations are processed first, but currentregisterDynamicContractsalready chooses earliest per address, so this is non-essential.Also applies to: 358-359, 391-392
codegenerator/cli/npm/envio/src/FetchState.res (3)
549-560: Comparator correctness; minor nit in commentThe comparator orders by blockNumber desc, then logIndex desc — matches the “latest-to-earliest” invariant used throughout (earliest is queue last). Minor: “Comparitor” -> “Comparator” in the comment.
Apply this small fix:
-/* -Comparitor for two events from the same chain. No need for chain id or timestamp -*/ +/* +Comparator for two events from the same chain. No need for chain id or timestamp +*/
566-567: Outdated docstring vs new behaviorThe comment says “newItems are ordered earliest to latest”, but the implementation now accepts unsorted input and sorts internally. Update the doc to avoid confusion.
Apply this clarification:
- newItems are ordered earliest to latest (as they are returned from the worker) + newItems may be unsorted (as returned from the worker). The queue is maintained + sorted DESC by (blockNumber, logIndex) after concatenation.
629-635: Avoid unnecessary sort when newItems is empty; potential perf winWhen
newItemsis[], we still sort the existing queue every time. Skipping the sort in that case reduces work, especially in steady-state polling or merge-only responses.Apply this change:
- ~queue=fetchState.queue - ->Array.concat(newItems) - // Theoretically it could be faster to asume that - // the items are sorted, but there are cases - // when the data source returns them unsorted - ->Js.Array2.sortInPlaceWith(compareBufferItem), + ~queue= + if newItems->Utils.Array.isEmpty { + fetchState.queue + } else { + fetchState.queue + ->Array.concat(newItems) + // Theoretically it could be faster to assume that + // the items are sorted, but there are cases + // when the data source returns them unsorted + ->Js.Array2.sortInPlaceWith(compareBufferItem) + },Optional follow-up: if
newItemsis typically small, consider inserting each into the already-sorted queue via binary search to avoid full re-sorts. Keep as a future micro-optimization.codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (2)
551-570: Forward iteration and accumulation LGTM; consider avoiding getUnsafe for readabilityThe change to forward iteration plus push is consistent with the new ordering model. You can simplify the loop and avoid Array.getUnsafe with Array.forEach without cost here.
Apply within this range:
- for idx in 0 to parsedQueueItems->Array.length - 1 { - let item = parsedQueueItems->Array.getUnsafe(idx) - if ( - switch chainFetcher.processingFilters { - | None => true - | Some(processingFilters) => ChainFetcher.applyProcessingFilters(~item, ~processingFilters) - } - ) { - if item.eventConfig.contractRegister !== None { - itemsWithContractRegister->Array.push(item) - } - - // TODO: Don't really need to keep it in the queue - // when there's no handler (besides raw_events, processed counter, and dcsToStore consuming) - newItems->Array.push(item) - } - } + parsedQueueItems->Array.forEach(item => { + if ( + switch chainFetcher.processingFilters { + | None => true + | Some(processingFilters) => ChainFetcher.applyProcessingFilters(~item, ~processingFilters) + } + ) { + if item.eventConfig.contractRegister !== None { + itemsWithContractRegister->Array.push(item) + } + // TODO: Don't really need to keep it in the queue + // when there's no handler (besides raw_events, processed counter, and dcsToStore consuming) + newItems->Array.push(item) + } + })Also, the TODO suggests we might skip enqueuing when no handler exists; if we decide to keep pushing for raw_events/metrics, consider clarifying that in the comment to avoid future churn. I can help wire a guard once the desired behavior is decided.
572-581: Avoid Utils.magic on empty array branchThe empty-branch cast is unnecessary and unsafe. Prefer a typed binding to keep this branch allocation-free and type-safe.
Replace with:
-let dynamicContracts = switch itemsWithContractRegister { -| [] as empty => - // A small optimisation to not recreate an empty array - empty->(Utils.magic: array<Internal.eventItem> => array<FetchState.indexingContract>) -| _ => - await ChainFetcher.runContractRegistersOrThrow( - ~itemsWithContractRegister, - ~config=state.config, - ) -} +let dynamicContracts: array<FetchState.indexingContract> = + switch itemsWithContractRegister { + | [] => [] + | _ => + await ChainFetcher.runContractRegistersOrThrow( + ~itemsWithContractRegister, + ~config=state.config, + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
codegenerator/cli/npm/envio/src/FetchState.res(3 hunks)codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs(3 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res(4 hunks)codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(6 hunks)scenarios/test_codegen/test/ChainManager_test.res(1 hunks)scenarios/test_codegen/test/lib_tests/FetchState_test.res(24 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{res,resi}
📄 CodeRabbit Inference Engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
ReScript has record types which require a type definition before hand. You can access record fields by dot likefoo.myField.
It's also possible to define an inline object, it'll have quoted fields in this case.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
Never use %raw to access object fields if you know the type.
Files:
scenarios/test_codegen/test/ChainManager_test.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/npm/envio/src/FetchState.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.resscenarios/test_codegen/test/lib_tests/FetchState_test.res
codegenerator/cli/templates/{dynamic/**/*.hbs,static/**}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Templates live under codegenerator/cli/templates: dynamic/ for Handlebars (.hbs), static/ for raw Rescript files copied verbatim.
Files:
codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbscodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
{**/generated/src/**/*.res,codegenerator/cli/templates/static/codegen/src/**/*.res,codegenerator/cli/templates/dynamic/codegen/src/**/*.res}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Runtime code lives in each project’s generated/src, but template versions (good for editing) are under codegenerator/cli/templates/static/codegen/src or codegenerator/cli/templates/dynamic/codegen/src.
Files:
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
codegenerator/cli/npm/envio/**
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Library-fied runtime shared across indexers lives in codegenerator/cli/npm/envio.
Files:
codegenerator/cli/npm/envio/src/FetchState.res
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (9)
scenarios/test_codegen/test/ChainManager_test.res (1)
83-85: Renamed label to ~newItems: OKCallsite updated to
~newItems=[batchItem]aligns with the newFetchState.handleQueryResultsignature. Since each iteration passes a single item, ordering semantics are unaffected and the downstream queue sort remains correct.scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
1051-1053: Bulk migration to ~newItems: consistent and correctAll updated call sites now pass
~newItems(including empty arrays) toFetchState.handleQueryResult. This is consistent with the updated API and the sorting behavior introduced in FetchState. No logical issues spotted.Also applies to: 1186-1188, 1302-1303, 1421-1423, 1500-1502, 1548-1550, 1986-1987, 2000-2001, 2067-2072, 2136-2137, 2246-2247, 2263-2264, 2279-2280, 2296-2297, 2320-2321, 2337-2338, 2410-2411, 2446-2450, 2518-2519, 2686-2690, 2731-2735, 2773-2777
codegenerator/cli/npm/envio/src/FetchState.res (1)
569-575: No lingering reversed identifiers found*Verified via ripgrep that there are no remaining references to
reversedNewItemsorreversedWithContractRegister, and allhandleQueryResultcall sites correctly pass~newItems.codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (2)
356-357: Forward iteration over itemsWithContractRegister: OKRenamed arg and switched to forward iteration. Order shouldn’t matter for
registerDynamicContracts(it dedups and picks earliest per address), so this is safe. The explicit range loop withgetUnsafeis fine here.Also applies to: 399-401
453-460: Propagating ~newItems to FetchState: OKSignature and callsite changes are consistent with the new FetchState API; dynamic contract registration is applied prior to queue update as expected.
Also applies to: 469-476
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (4)
585-595: Dispatch payload: newItems and latestFetchedBlock shape look correctUsing newItems and packaging latestFetchedBlock as {blockNumber, blockTimestamp} aligns with FetchState.blockNumberAndTimestamp and PR intent.
654-669: Reducer updates for newItems are consistentDestructuring newItems and passing it through submitPartitionQueryResponse matches the upstream changes. Looks good.
126-133: Verification complete: no residualreversed*identifiers found
- Searched all
.resand.resifiles forreversed(NewItems),reversed(WithContractRegister), and anyreversed.*Item(s)- No occurrences detected
Switching
SubmitPartitionQueryResponsepayload tonewItemsaligns with the PR’s intent and improves clarity. Code changes approved.
482-500: Verified: ChainFetcher.handleQueryResult Accepts ~newItems
- Definition in codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (line 452) uses
~newItemsin its parameter list.- All downstream call-sites—including the call in GlobalState.res (line 493)—pass
~newItems.
No~reversedNewItemsremains. No further action required.
| // Theoretically it could be faster to asume that | ||
| // the items are sorted, but there are cases | ||
| // when the data source returns them unsorted |
There was a problem hiding this comment.
This is the issue I shared yesterday about Base internally.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests