feat: allow passing historical as a parameter #313
Conversation
WalkthroughThe changes in this pull request involve the modification of three functions— Changes
Poem
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: 2
🧹 Outside diff range and nitpick comments (2)
packages/state/src/recs/index.ts (2)
161-161: Consider enhancing the historical parameter documentation.While the documentation is accurate, it would be more helpful to explain what happens when
historicalis set tofalse. This would help developers understand the implications of this parameter choice.Consider expanding the documentation like this:
-@param historical - Whether to fetch historical events (default: true). +@param historical - Whether to fetch historical events (default: true). When set to false, only new events will be fetched, ignoring any events that occurred before the function call.
304-304: Consider enhancing the historical parameter documentation for subscription context.The documentation should clarify how this parameter affects event subscription behavior.
Consider expanding the documentation like this:
-@param historical - Whether to sync to historical events (default: true). +@param historical - Whether to sync to historical events (default: true). When true, the subscription will receive both historical events and new events as they occur. When false, only new events occurring after subscription creation will be received.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/state/src/recs/index.ts(6 hunks)
🔇 Additional comments (3)
packages/state/src/recs/index.ts (3)
66-66: LGTM: Documentation for historical parameter is clear and complete.
The added JSDoc documentation for the historical parameter clearly explains its purpose and default value.
93-104: LGTM: Implementation correctly propagates the historical parameter.
The changes maintain consistency by passing the historical parameter to both downstream functions.
168-169: LGTM: Implementation correctly uses the historical parameter.
The changes properly integrate with the client's API. However, let's verify the client's implementation supports this parameter.
Also applies to: 183-183
✅ Verification successful
Based on the search results from previous scripts, we can see that:
getEventMessagesis used throughout the codebase, particularly inpackages/sdk/src/getEventMessages.tswhere it's called with ahistoricalparameter- The client interface is imported from
@dojoengine/torii-clientpackage which is an external dependency - The method is consistently used with the
historicalparameter across different parts of the codebase
Since the ToriiClient interface is defined in an external package and we can see consistent usage of getEventMessages with the historical parameter throughout the codebase, including direct calls to client.getEventMessages(toriiQuery, true), this provides sufficient evidence that the parameter is supported.
Implementation verified: The historical parameter is supported by ToriiClient
The codebase shows consistent usage of the historical parameter with getEventMessages, and the parameter is properly passed through to the client's API call.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ToriiClient's getEventMessages method supports the historical parameter
# Test: Search for the getEventMessages method definition
ast-grep --pattern 'interface ToriiClient {
$$$
getEventMessages($_): $_
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search for ToriiClient and its method definitions
rg -A 10 "ToriiClient"
Length of output: 17270
Script:
#!/bin/bash
# Also search for getEventMessages implementation
rg -A 5 "getEventMessages"
Length of output: 2922
Script:
#!/bin/bash
# Search for any interface or class containing getEventMessages
ast-grep --pattern '(interface|class) $_ {
$$$
getEventMessages($_) {
$$$
}
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Let's search for the ToriiClient interface definition in the torii-client package
rg -A 10 "export (type|interface) ToriiClient"
# Also search for any TypeScript interface containing getEventMessages
ast-grep --pattern 'interface $_ {
$$$
getEventMessages($_): $_
$$$
}'
Length of output: 127
| logging: boolean = false, | ||
| historical: boolean = true |
There was a problem hiding this comment.
Tests needed for the new historical parameter functionality.
While the implementation looks good, tests should be added to verify the behavior of the historical parameter across all modified functions. This should include:
- Test cases with
historical=true(default behavior) - Test cases with
historical=false(new behavior) - Integration tests showing the interaction between these functions
Would you like me to help create test cases for these scenarios?
Also applies to: 168-169, 315-316
| logging: boolean = false, | ||
| historical: boolean = true |
There was a problem hiding this comment.
💡 Codebase verification
Verification needed: Historical parameter implementation mismatch
The implementation shows inconsistency in how the historical parameter is used:
- In
packages/state/src/recs/index.ts, it's properly passed as a parameter - In
packages/sdk/src/subscribeEventQuery.ts, it's hardcoded totrue
This suggests potential issues:
- The SDK's implementation doesn't expose the historical parameter to users
- The hardcoded value might conflict with the configurable parameter in the state package
🔗 Analysis chain
LGTM: Implementation correctly uses the historical parameter.
The changes properly integrate with the client's API. However, let's verify the client's implementation supports this parameter.
Also applies to: 321-321
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ToriiClient's onEventMessageUpdated method supports the historical parameter
# Test: Search for the onEventMessageUpdated method definition
ast-grep --pattern 'interface ToriiClient {
$$$
onEventMessageUpdated($_): $_
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search for ToriiClient and onEventMessageUpdated
# First, find relevant files
fd -t f -e ts -e tsx ToriiClient
# Then search for onEventMessageUpdated method/function definitions
rg -l "onEventMessageUpdated"
# Finally, let's look for actual implementations or interface definitions
ast-grep --pattern 'onEventMessageUpdated($_)'
Length of output: 186
Script:
#!/bin/bash
# Let's examine the actual implementation and usage in both files
rg -A 5 -B 5 "onEventMessageUpdated" packages/sdk/src/subscribeEventQuery.ts packages/state/src/recs/index.ts
Length of output: 1657
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
historicalparameter to key functions, allowing users to specify whether to include historical events in their queries.