fix: historical events ordering #384
Conversation
WalkthroughThis pull request introduces patch updates across multiple Dojo Engine packages, focusing on improvements to historical event handling. The primary changes involve modifying the Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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)
packages/sdk/src/parseHistoricalEvents.ts (3)
22-25: Consider restricting console logs during production builds.
You appear to be logging the raw historical events (line 23) conditionally viaoptions?.logging, yet line 25 unconditionally logsentitiesregardless ofoptions.logging. This can clutter production logs or risk unintended data exposure.if (options?.logging) { console.log("Raw historical events", entities); + console.log(entities); } -console.log(entities);
32-33: Avoid excessive debugging logs.
Theconsole.log(model);within the loop can fill the logs excessively. Consider either removing it or selectively restricting it to debug mode.
69-79: Return a new array to avoid side effects.
orderKeyssorts the array in-place. If you want to avoid mutating the input, clone the array first. Otherwise, specify in the docstring thatorderKeysmutates the original object reference.-export function orderKeys(keys: string[]) { - keys.sort((a, b) => { +export function orderKeys(inputKeys: string[]) { + const keys = [...inputKeys]; // existing logic return keys; }packages/sdk/src/experimental/index.ts (1)
71-77: Avoid repeated calls toawait client.getEventMessages().
Instead of callingawait client.getEventMessages(query, historical)twice in the ternary expression, assign the result to a local constant and parse accordingly. This improves clarity and reduces duplication.const rawEvents = await client.getEventMessages(query, historical); const events = historical - ? parseHistoricalEvents<T>(await client.getEventMessages(query, historical)) - : parseEntities<T>(await client.getEventMessages(query, historical)); + ? parseHistoricalEvents<T>(rawEvents) + : parseEntities<T>(rawEvents);packages/sdk/src/__tests__/parseHistoricalEvents.test.ts (3)
7-18: Consider adding more specific test assertions.While the test verifies that the results are equal, it would be more robust to also assert:
- The specific ordering of events
- The structure of the parsed results
Consider adding these assertions:
it("should preserve order", () => { const res1 = parseHistoricalEvents( toriiResultUnsorted as torii.Entities, { logging: false } ); const res2 = parseHistoricalEvents( toriiResultSorted as torii.Entities, { logging: false } ); expect(res1).toEqual(res2); + // Verify specific ordering + const events1 = Object.keys(res1); + expect(events1).toEqual([ + "dojo_starter-Moved", + "dojo_starter-Moved-1", + // ... other expected events in order + ]); + // Verify structure + expect(res1["dojo_starter-Moved"]).toHaveProperty("direction"); + expect(res1["dojo_starter-Moved"]).toHaveProperty("player"); });
19-35: Consider extracting the repeated entity key to a constant.The long hex string is repeated and could be made more maintainable.
Apply this refactor:
+const ENTITY_KEY = "0x681d68850e9c98f4383832aa206f00de22040754d6094f7b9a5a6802c25d2a3"; + it("should order keys", () => { const key1Unsorted = Object.keys( - toriiResultUnsorted[ - "0x681d68850e9c98f4383832aa206f00de22040754d6094f7b9a5a6802c25d2a3" - ] + toriiResultUnsorted[ENTITY_KEY] ); const key2Sorted = Object.keys( - toriiResultSorted[ - "0x681d68850e9c98f4383832aa206f00de22040754d6094f7b9a5a6802c25d2a3" - ] + toriiResultSorted[ENTITY_KEY] ); expect(key1Unsorted).not.toEqual(key2Sorted);
38-793: Consider reducing test fixture size and improving maintainability.The test fixtures are quite large and repetitive. Consider:
- Creating helper functions to generate test data
- Using a smaller subset of events for testing
- Extracting common structures into reusable objects
Here's a suggested approach to reduce duplication:
const createMoveEvent = (direction: "Up" | "Down" | "Right", index?: number) => ({ [`dojo_starter-Moved${index ? `-${index}` : ""}`]: { direction: { type: "enum", type_name: "Direction", value: { option: direction, value: { type: "tuple", type_name: "()", value: [], key: false, } }, key: false, }, player: { type: "primitive", type_name: "ContractAddress", value: "0x013d9ee239f33fea4f8785b9e3870ade909e20a9599ae7cd62c1c292b73af1b7", key: true, } } }); const toriiResultUnsorted = { [ENTITY_KEY]: { ...createMoveEvent("Up"), ...createMoveEvent("Up", 1), // ... other events } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/neat-moose-press.md(1 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(2 hunks)packages/sdk/src/__tests__/parseHistoricalEvents.test.ts(1 hunks)packages/sdk/src/experimental/index.ts(1 hunks)packages/sdk/src/parseHistoricalEvents.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/neat-moose-press.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (4)
packages/sdk/src/parseHistoricalEvents.ts (1)
27-46: Validate the array-based type definition.
The function’s docstring indicates returningStandardizedQueryResult<T>, but it is instantiated as an array[]and populated with spread pushes. Ensure the type definition or docstring alignment is correct (ifStandardizedQueryResult<T>is indeed an array type, clarifying that in documentation would help).examples/example-vite-react-sdk/src/historical-events.tsx (2)
12-12: Ensure the updated array shape is used consistently.
Changing from a 2D to a 1D array simplifies state management. Verify that other logic expecting a nested array of events has been updated accordingly.
85-86: Great simplification of the event rendering logic.
This change eliminates the need for nested indexing and improves readability.packages/sdk/src/__tests__/parseHistoricalEvents.test.ts (1)
1-5: LGTM! Clean and minimal imports.The imports are well-organized and include only the necessary dependencies.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
Patch Updates
Bug Fixes
Performance