feat(react): change effect formatter types #513
Conversation
WalkthroughAdds two "with updates" infinite-scroll atoms that merge live updates into paginated state and refactors an entities infinite-scroll API; also relaxes return-type generics for three formatter types and updates exports and a changeset entry. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
MartianGreed
force-pushed
the
fix/react-effect-type
branch
from
05b6a79 to
f00bca5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/src/effect/atoms/events.ts (1)
88-88: Replaceany[]withEvent[]for type safety.The
itemsproperty is typed asany[], which violates the coding guideline to avoid theanytype. Since these are events from the Torii client, they should be typed asEvent[].Apply this diff to fix the type:
export interface EventsInfiniteState { - items: any[]; + items: Event[]; cursor?: string; hasMore: boolean; isLoading: boolean; error?: ToriiGrpcClientError; }Based on learnings, avoid
anytype; lean on generics, discriminated unions, and utility types.
🧹 Nitpick comments (2)
packages/react/src/effect/atoms/events.ts (1)
263-321: Consider extracting the deduplication logic to eliminate code duplication.The deduplication logic (lines 291-312) is nearly identical to the logic in
createEventQueryWithUpdatesAtom(lines 232-253). This duplication increases maintenance burden and the risk of inconsistencies if one implementation is updated without the other.Consider extracting a shared helper function:
function deduplicateEventsByTransactionHash( baseItems: Event[], updates: Event[] ): Event[] { const seenHashes = new Set<string>(); const items = [...baseItems]; for (const event of items) { if (event.transaction_hash && event.transaction_hash.length > 0) { seenHashes.add( Buffer.from(event.transaction_hash).toString("hex") ); } } for (const update of updates) { if (update.transaction_hash && update.transaction_hash.length > 0) { const hashStr = Buffer.from(update.transaction_hash).toString( "hex" ); if (!seenHashes.has(hashStr)) { items.push(update); seenHashes.add(hashStr); } } } return items; }Then use it in both functions to reduce duplication.
packages/react/src/effect/atoms/entities.ts (1)
533-593: Consider extracting the entity merge logic to reduce duplication.The entity merging and deduplication logic (lines 566-584) is nearly identical to the logic in
createEntityQueryWithUpdatesAtom(lines 505-523). This duplication makes the code harder to maintain.Consider extracting a shared helper function:
function mergeEntityUpdates( baseItems: ParsedEntity[], updatesMap: Map<string, EntityUpdate>, formatters?: DataFormatters ): ParsedEntity[] { const entitiesMap = new Map<string, ParsedEntity>(); for (const entity of baseItems) { entitiesMap.set(entity.entityId, entity); } for (const [hashedKeys, update] of updatesMap) { const entityId = addAddressPadding(hashedKeys); const parsedUpdate = parseEntitySync(update, formatters); const existing = entitiesMap.get(entityId); if (existing) { entitiesMap.set(entityId, { entityId, models: mergeModels(existing.models, parsedUpdate.models), }); } else { entitiesMap.set(entityId, parsedUpdate); } } return Array.from(entitiesMap.values()); }Then use it in both
createEntityQueryWithUpdatesAtomandcreateEntitiesInfiniteScrollWithUpdatesAtom.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/wise-dogs-clean.md(1 hunks)packages/react/src/effect/atoms/entities.ts(2 hunks)packages/react/src/effect/atoms/events.ts(1 hunks)packages/react/src/effect/atoms/index.ts(2 hunks)packages/react/src/effect/formatters.ts(1 hunks)packages/react/src/effect/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/react/src/effect/atoms/index.tspackages/react/src/effect/atoms/entities.tspackages/react/src/effect/formatters.tspackages/react/src/effect/index.tspackages/react/src/effect/atoms/events.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/react/src/effect/atoms/index.tspackages/react/src/effect/atoms/entities.tspackages/react/src/effect/formatters.tspackages/react/src/effect/index.tspackages/react/src/effect/atoms/events.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/react/src/effect/atoms/index.tspackages/react/src/effect/atoms/entities.tspackages/react/src/effect/formatters.tspackages/react/src/effect/index.tspackages/react/src/effect/atoms/events.ts
🧠 Learnings (2)
📚 Learning: 2025-11-26T13:27:46.736Z
Learnt from: CR
Repo: dojoengine/dojo.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:27:46.736Z
Learning: Applies to **/*.{ts,tsx} : Avoid `any` type; lean on generics, discriminated unions, and utility types
Applied to files:
packages/react/src/effect/formatters.ts
📚 Learning: 2025-11-26T13:27:46.736Z
Learnt from: CR
Repo: dojoengine/dojo.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:27:46.736Z
Learning: Applies to **/*.{ts,tsx} : Maintain explicit return types for exported APIs when inference is unclear
Applied to files:
packages/react/src/effect/formatters.ts
🧬 Code graph analysis (1)
packages/react/src/effect/atoms/events.ts (4)
packages/react/src/effect/atoms/index.ts (4)
createEventsInfiniteScrollWithUpdatesAtom(33-33)Atom(1-1)createEventUpdatesAtom(30-30)Result(1-1)packages/react/src/effect/index.ts (3)
createEventsInfiniteScrollWithUpdatesAtom(35-35)ToriiGrpcClient(2-2)createEventUpdatesAtom(32-32)packages/grpc/src/torii-client.ts (1)
ToriiGrpcClient(222-1256)packages/grpc/src/index.ts (1)
KeysClause(20-20)
⏰ 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). (3)
- GitHub Check: build-examples
- GitHub Check: build-packages
- GitHub Check: build
🔇 Additional comments (6)
packages/react/src/effect/atoms/events.ts (1)
269-270: Good composition pattern for combining infinite scroll with live updates.The approach of composing the base infinite scroll atom with the updates atom is clean and maintains separation of concerns. The merge logic correctly handles initial, error, and success states from the updates stream.
packages/react/src/effect/formatters.ts (2)
35-38: Verify that loosening the ModelFormatter return type constraint is intentional.The return type constraint has been changed from
R extends Record<string, unknown>toR = unknown. This allows formatters to return any type, including primitives,null, orundefined, which may not align with the intended use of model formatters that typically transform objects.If formatters are expected to always return structured data, consider keeping the
Record<string, unknown>constraint to maintain type safety at compile time.Based on learnings, maintain explicit return types for exported APIs when inference is unclear, and avoid overly broad types like
unknownwhen more specific constraints are appropriate.
44-50: Verify that token formatter return types should be unconstrained.Both
TokenFormatterandTokenBalanceFormatternow return the genericR = unknowninstead of their respective concrete types (TokenandTokenBalance). This change allows formatters to transform tokens/balances into any type, which provides flexibility but removes type guarantees.Ensure this change aligns with actual usage patterns. If formatters commonly transform these into different shapes (e.g., UI-specific types or primitives), this is appropriate. Otherwise, consider whether the original concrete return types provided better type safety.
Based on learnings, avoid overly broad types; lean on generics with appropriate constraints when the return type varies, but maintain specificity when possible.
packages/react/src/effect/atoms/entities.ts (1)
540-541: Good composition pattern for entities infinite scroll with updates.The function correctly composes the base infinite scroll behavior with entity updates, properly handling state transitions and error propagation from the updates stream.
packages/react/src/effect/index.ts (1)
27-27: LGTM! Export additions are clean and consistent.The new exports for
createEntitiesInfiniteScrollWithUpdatesAtomandcreateEventsInfiniteScrollWithUpdatesAtomare properly placed alongside related exports, maintaining consistency with the existing API surface.Also applies to: 35-35
packages/react/src/effect/atoms/index.ts (1)
8-8: LGTM! Export additions maintain proper module organization.The new exports are correctly placed within their respective domain blocks (entities and events), maintaining the established organizational pattern.
Also applies to: 33-33
| "@dojoengine/react": patch | ||
| --- | ||
|
|
||
| feat(react): change effect formatter types + add initewithupdates atoms |
There was a problem hiding this comment.
Fix typo in changeset description.
The text "initewithupdates" appears to be a typo and should be "infiniteWithUpdates" or "infiniteScrollWithUpdates" to match the actual function names being introduced.
Apply this diff:
-feat(react): change effect formatter types + add initewithupdates atoms
+feat(react): change effect formatter types + add infiniteWithUpdates atoms📝 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.
| feat(react): change effect formatter types + add initewithupdates atoms | |
| feat(react): change effect formatter types + add infiniteWithUpdates atoms |
🤖 Prompt for AI Agents
In .changeset/wise-dogs-clean.md around line 5, fix the typo in the changeset
summary by replacing "initewithupdates" with the correct function name; update
it to "infiniteWithUpdates" (or, if the actual export is named differently, use
"infiniteScrollWithUpdates") so the changeset matches the introduced function
names.
MartianGreed
force-pushed
the
fix/react-effect-type
branch
from
f00bca5 to
f13d08e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/react/src/effect/atoms/tokens.ts (1)
234-240: Token[] cast in infinite-scroll state hides formatter return flexibility
items: [...currentState.items, ...result.items] as Token[]restores compatibility withTokensInfiniteState.items: Token[]after relaxingTokenFormatter<R>’s return type. This is fine for runtime but is type-unsafe if a formatter intentionally returns a non-Tokenshape.If the intent is to support transformed token shapes, consider either:
- Constraining
TokenFormatter<R>back to return aToken-compatible object for this path, or- Making
TokensInfiniteState(andapplyTokenFormatters) generic over the item type so the state reflects the actual formatter output instead of casting.packages/react/src/effect/atoms/token-balances.ts (1)
94-102: TokenBalance[] casts trade type-safety for formatter flexibilityBoth in the polling updates atom and in the infinite-scroll atom you now cast:
items: applyTokenBalanceFormatters(... ) as TokenBalance[]items: [...currentState.items, ...result.items] as TokenBalance[]Given
TokenBalanceFormatter<R>now allows arbitraryR, these casts assume formatters still returnTokenBalance-compatible objects. That’s fine for existing usage but weakens type guarantees.If callers are expected to keep
TokenBalancesemantics here, consider constrainingR(e.g.R extends TokenBalance) for the formatters used in these atoms, or make the state/item types generic so they accurately reflect formatter outputs instead of forcing a cast.Also applies to: 246-251
packages/react/src/effect/index.ts (1)
22-38: New with-updates atom exports are wired correctlyRe-exporting
createEntitiesInfiniteScrollWithUpdatesAtomandcreateEventsInfiniteScrollWithUpdatesAtomfrom the effect index matches the atoms index and underlying modules, so the public surface stays consistent. Consider adding docs/tests for these new public APIs if they don’t exist yet.packages/react/src/effect/atoms/events.ts (1)
263-321: createEventsInfiniteScrollWithUpdatesAtom merge logic looks consistent and correctThe new atom cleanly composes:
- base infinite-scroll state (
createEventsInfiniteScrollAtom) and- the updates stream (
createEventUpdatesAtom),with sensible behavior:
- Initial updates → fall back to base state.
- Failed updates → keep base state but surface updates error.
- Successful updates → build a
seenHashesset and append only truly new events based ontransaction_hash(mirrorscreateEventQueryWithUpdatesAtom).Two minor follow-ups to consider:
- The hash-deduplication loop is duplicated from
createEventQueryWithUpdatesAtom; factoring this into a small helper would reduce drift risk.EventsInfiniteState.itemsisany[], but this code assumes anEvent-like shape (transaction_hash). Tightening that type toEvent[](or a generic) would make these assumptions explicit and avoidanycreep.packages/react/src/effect/atoms/entities.ts (3)
275-356: QueryBuilder-based infinite scroll is reasonable; be mindful of withCursor semanticsSwitching
createEntitiesInfiniteScrollAtomto take aToriiQueryBuilder<SchemaType>and then callingwithCursor(currentState.cursor)beforebuild()makes pagination configuration more centralised.One subtle point:
queryBuilderis closed over and reassigned on eachloadMorecall. This is fine ifwithCursorreturns a new builder (functional style), but if it mutates the underlying builder in place and the same builder instance is shared across multiple infinite-scroll atoms, their cursors could interfere.If
ToriiQueryBuilder.withCursoris mutative, consider clarifying in its docs and/or constructing a fresh builder per atom instead of reusing a shared one. If it is purely functional, this pattern is good as-is.
445-450: Casting modelFormatter result back to a plain object weakens type guarantees
formattedModel = modelFormatter(formattedModel, { ... }) as { [x: string]: unknown };is now necessary becauseModelFormatter’sRis unconstrained. At runtime, the rest of this function still assumesformattedModelis a plain object whose keys can be re-assigned and stored undermodels.If the intended contract is “model formatters must return object-like records”, it would be safer to keep that in the type (
R extends Record<string, unknown>) and avoid the cast, or to introduce a second, less-constrained formatter type for truly arbitrary transformations. As written, callers can now return non-object values without any type error, which may surface as runtime failures here.
533-593: createEntitiesInfiniteScrollWithUpdatesAtom correctly composes base scroll with updatesThis new atom:
- Reuses
createEntitiesInfiniteScrollAtomfor base pagination andcreateEntityUpdatesAtomfor live updates.- Propagates initial/failed updates sensibly (base state on initial; base state + updates error on failure).
- Builds a
Mapkeyed byentityId, seeding it frombaseState.itemsand then merging/parsing updates viaparseEntitySyncandmergeModels, mirroring the logic already used increateEntityQueryWithUpdatesAtom.The behavior looks correct and consistent with the existing query-with-updates atom. To reduce duplication and future drift, you might consider extracting the “merge base entities + updatesMap into a new array” logic into a shared helper used by both with-updates variants.
packages/react/src/effect/formatters.ts (1)
35-50: Formatter generics are now very loose; consider reflecting contracts more preciselyThe updated signatures:
ModelFormatter<..., R = unknown>TokenFormatter<R = unknown>TokenBalanceFormatter<R = unknown>make formatter return types fully unconstrained, but the rest of the codebase still largely assumes:
- Model formatters return plain objects (
Record<string, unknown>), and- Token/TokenBalance formatters return
Token/TokenBalance-compatible shapes (see the casts inentities.ts,tokens.ts, andtoken-balances.ts).This mismatch forces unsafe casts and can hide genuine type errors.
Possible refinements:
- Keep
Rgeneric but bound it to the expected shape where needed, e.g.ModelFormatter<T, R extends Record<string, unknown> = T>for the parsing/formatting pipeline, and/or- Add type parameters to
DataFormattersso callers who truly want non-Token/non-record outputs can opt into different state types, instead of having core atoms assume the stricter shapes while the types say “anything goes”.That would preserve the flexibility you’re aiming for while keeping the main data paths type-safe and reducing the need for
ascasts downstream.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/wise-dogs-clean.md(1 hunks)packages/react/src/effect/atoms/entities.ts(3 hunks)packages/react/src/effect/atoms/events.ts(1 hunks)packages/react/src/effect/atoms/index.ts(2 hunks)packages/react/src/effect/atoms/token-balances.ts(3 hunks)packages/react/src/effect/atoms/tokens.ts(2 hunks)packages/react/src/effect/formatters.ts(1 hunks)packages/react/src/effect/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/wise-dogs-clean.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/react/src/effect/atoms/tokens.tspackages/react/src/effect/index.tspackages/react/src/effect/atoms/token-balances.tspackages/react/src/effect/atoms/entities.tspackages/react/src/effect/formatters.tspackages/react/src/effect/atoms/events.tspackages/react/src/effect/atoms/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/react/src/effect/atoms/tokens.tspackages/react/src/effect/index.tspackages/react/src/effect/atoms/token-balances.tspackages/react/src/effect/atoms/entities.tspackages/react/src/effect/formatters.tspackages/react/src/effect/atoms/events.tspackages/react/src/effect/atoms/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/react/src/effect/atoms/tokens.tspackages/react/src/effect/index.tspackages/react/src/effect/atoms/token-balances.tspackages/react/src/effect/atoms/entities.tspackages/react/src/effect/formatters.tspackages/react/src/effect/atoms/events.tspackages/react/src/effect/atoms/index.ts
🧠 Learnings (1)
📚 Learning: 2025-11-26T13:27:46.736Z
Learnt from: CR
Repo: dojoengine/dojo.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:27:46.736Z
Learning: Applies to **/*.{ts,tsx} : Avoid `any` type; lean on generics, discriminated unions, and utility types
Applied to files:
packages/react/src/effect/formatters.ts
🧬 Code graph analysis (4)
packages/react/src/effect/atoms/tokens.ts (1)
packages/grpc/src/index.ts (1)
Token(26-26)
packages/react/src/effect/atoms/token-balances.ts (1)
packages/grpc/src/index.ts (1)
TokenBalance(28-28)
packages/react/src/effect/atoms/entities.ts (2)
packages/react/src/effect/atoms/index.ts (4)
createEntitiesInfiniteScrollWithUpdatesAtom(8-8)Atom(1-1)DataFormatters(45-45)createEntitiesInfiniteScrollAtom(6-6)packages/react/src/effect/formatters.ts (1)
DataFormatters(55-79)
packages/react/src/effect/atoms/events.ts (3)
packages/react/src/effect/atoms/index.ts (5)
createEventsInfiniteScrollWithUpdatesAtom(33-33)Atom(1-1)createEventsInfiniteScrollAtom(31-31)createEventUpdatesAtom(30-30)Result(1-1)packages/react/src/effect/index.ts (4)
createEventsInfiniteScrollWithUpdatesAtom(35-35)ToriiGrpcClient(2-2)createEventsInfiniteScrollAtom(33-33)createEventUpdatesAtom(32-32)packages/grpc/src/index.ts (1)
KeysClause(20-20)
⏰ 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). (3)
- GitHub Check: build-packages
- GitHub Check: build-examples
- GitHub Check: build
🔇 Additional comments (3)
packages/react/src/effect/atoms/tokens.ts (1)
17-21: applyTokenFormatters signature change is formatting-onlyThis is just brace/line placement; no behavioral or typing impact.
packages/react/src/effect/atoms/token-balances.ts (1)
13-17: applyTokenBalanceFormatters signature change is formatting-onlyOnly line-wrapping/brace placement changed; no impact on behavior or types.
packages/react/src/effect/atoms/index.ts (1)
3-35: Atoms index now exposes new with-updates variants
createEntitiesInfiniteScrollWithUpdatesAtomandcreateEventsInfiniteScrollWithUpdatesAtomare correctly re-exported alongside their base/query/update counterparts, keeping the atoms surface coherent. No issues spotted.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.