fix: added historical option for event getters #352
Conversation
WalkthroughThe changes in this pull request introduce a new boolean parameter, Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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: 1
🧹 Outside diff range and nitpick comments (6)
packages/sdk/src/subscribeEventQuery.ts (1)
39-40: Update JSDoc to document the historical parameterThe
historicalparameter needs to be documented in the function's JSDoc comments to maintain API documentation completeness.Add the following to the JSDoc params section:
+ * @param {boolean} [historical=true] - Whether to include historical event messages in the subscription.packages/sdk/src/getEventMessages.ts (1)
39-40: Update JSDoc to document the historical parameterThe
historicalparameter needs to be documented in the function's JSDoc comments.Add the following to the JSDoc params section:
+ * @param {boolean} [historical=true] - Whether to include historical event messages in the query.packages/sdk/src/index.ts (2)
56-64: Update JSDoc for subscribeEventQuery paramsThe
historicalparameter needs to be documented in the method's JSDoc.Update the params documentation:
* @param {SubscribeParams<T>} params - Parameters object + * params.historical - Whether to include historical event messages in the subscription
87-103: Update JSDoc for getEventMessages paramsThe
historicalparameter needs to be documented in the method's JSDoc.Update the params documentation:
* @param {GetParams<T>} params - Parameters object + * params.historical - Whether to include historical event messages in the querypackages/sdk/src/types.ts (2)
359-360: Enhance documentation for thehistoricalparameter.The comment "historical events" could be more descriptive. Consider adding JSDoc documentation that explains:
- The parameter's purpose
- The default value
- The behavior when true/false
- // historical events + /** + * Whether to include historical events in the subscription. + * When true (default), includes all past events. + * When false, only includes new events from the subscription point. + */ historical?: boolean;
377-378: Enhance documentation for thehistoricalparameter (consistency).Similar to the SubscribeParams interface, improve the documentation here to maintain consistency across the codebase.
- // historical events + /** + * Whether to include historical events in the query results. + * When true (default), includes all past events. + * When false, only includes new events. + */ historical?: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/sdk/src/getEventMessages.ts(2 hunks)packages/sdk/src/index.ts(2 hunks)packages/sdk/src/subscribeEventQuery.ts(1 hunks)packages/sdk/src/types.ts(2 hunks)
🔇 Additional comments (3)
packages/sdk/src/subscribeEventQuery.ts (1)
44-44: LGTM: Historical parameter implementation
The historical parameter is correctly passed to the underlying client method while maintaining backward compatibility with the default true value.
packages/sdk/src/getEventMessages.ts (1)
57-60: LGTM: Historical parameter correctly integrated with pagination
The historical parameter is properly passed to the client method while maintaining the existing pagination logic.
packages/sdk/src/index.ts (1)
Line range hint 56-103: Verify types.ts updates
The AI summary mentions updates to SubscribeParams and GetParams interfaces in types.ts.
Let's verify these type definitions:
✅ Verification successful
Types are correctly defined in types.ts
The historical parameter is properly defined as an optional boolean parameter in the relevant interfaces in types.ts. The code changes are consistent with the type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that types.ts has been updated with the historical parameter
# Check for the historical parameter in interface definitions
rg -A 5 "interface (Subscribe|Get)Params" packages/sdk/src/types.ts
Length of output: 582
Script:
#!/bin/bash
# Let's check for any historical parameter in the entire types.ts file
rg -A 10 "historical[?:]" packages/sdk/src/types.ts
# Also check for any interface that might contain the historical parameter
ast-grep --pattern 'interface $_ {
$$$
historical$_
$$$
}'
Length of output: 467
| // historical events | ||
| historical?: boolean; |
There was a problem hiding this comment.
💡 Codebase verification
Default value of historical parameter needs to be aligned with the interface
The implementation in both getEventMessages.ts and subscribeEventQuery.ts sets a default value of true for the historical parameter, while the interface in types.ts marks it as optional without a default value. This misalignment should be addressed.
packages/sdk/src/getEventMessages.ts: Remove default valuetrueforhistoricalparameterpackages/sdk/src/subscribeEventQuery.ts: Remove default valuetrueforhistoricalparameter
🔗 Analysis chain
Verify implementation details across related files.
The interfaces have been updated correctly, but let's verify the implementation details:
Also applies to: 377-378
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of the historical parameter
# Check getEventMessages implementation
echo "Checking getEventMessages implementation..."
rg -A 5 "getEventMessages.*=.*async.*" packages/sdk/src/
# Check subscribeEventQuery implementation
echo "Checking subscribeEventQuery implementation..."
rg -A 5 "subscribeEventQuery.*=.*async.*" packages/sdk/src/
# Check for any hardcoded true values that might need to be replaced
echo "Checking for hardcoded historical values..."
rg "historical.*:.*true" packages/sdk/src/
Length of output: 601
rsodre
force-pushed
the
fetch-events-historical-flag
branch
from
12f46ae to
c3f7328
Compare
|
@MartianGreed wen merge? |
rsodre
force-pushed
the
fetch-events-historical-flag
branch
from
104f8ea to
ff2adef
Compare
Introduced changes
historicaloption tosdk.getEventMessages()andsdk.subscribeEventQuery()Checklist
Summary by CodeRabbit
New Features
historicalparameter to thegetEventMessagesandsubscribeEventQuerymethods, allowing users to specify whether to include historical data in event queries and subscriptions.historicalparameter for subscriptions and entity retrieval.Bug Fixes
Documentation
historicalparameter in relevant methods.