fix: logging #360
fix: logging #360
Conversation
WalkthroughThe pull request introduces enhancements to data fetching and logging functionality across multiple files in the Dojo SDK. The primary changes include adding Changes
Sequence DiagramsequenceDiagram
participant Client
participant SDK
participant ToriiClient
Client->>SDK: getEventMessages(params, orderBy)
SDK->>ToriiClient: Query with ordering
ToriiClient-->>SDK: Return ordered results
SDK-->>Client: Filtered and ordered messages
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 3
🔭 Outside diff range comments (2)
packages/sdk/src/getEventMessages.ts (1)
Line range hint
60-61: Consider reducing logging verbosity.Given the PR's objective to reduce logging, consider:
- Removing or condensing the detailed entity logging at offset
- Limiting the error logging to essential information
- Removing the final log of all entities, as it duplicates information
- if (options?.logging) { - console.log(`Fetched entities at offset ${cursor}:`, entities); - } if (options?.logging) { - console.error("Error fetching entities:", error); + console.error("Error fetching entities"); } - if (options?.logging) { - console.log("All fetched entities:", allEntities); - }Also applies to: 77-79, 86-88
packages/sdk/src/index.ts (1)
Line range hint
153-155: Reduce error logging verbosity.In line with the PR's objective to reduce logging, consider simplifying the error message to exclude the full error object.
- console.error("Failed to send message:", error); + console.error("Failed to send message");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/sdk/src/getEventMessages.ts(1 hunks)packages/sdk/src/index.ts(2 hunks)packages/state/src/__tests__/utils.test.ts(1 hunks)packages/state/src/recs/index.ts(4 hunks)worlds/dojo-starter(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- worlds/dojo-starter
🔇 Additional comments (4)
packages/sdk/src/getEventMessages.ts (1)
53-53: Verify the purpose of empty order_by array.
The addition of an empty order_by array seems unrelated to the PR's objective of reducing logging. Please clarify the purpose of this change.
✅ Verification successful
The empty order_by array is consistent with the codebase pattern
The addition of order_by: [] in getEventMessages.ts aligns with the existing pattern in the codebase, where all getEntities and similar query calls include this parameter. This change appears to be a consistency improvement rather than being related to logging reduction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of order_by to understand the context
rg "order_by" --type ts
Length of output: 263
Script:
#!/bin/bash
# Get more context around the order_by usage in both files
rg "order_by" -B 5 -A 5 --type ts
Length of output: 2698
packages/state/src/__tests__/utils.test.ts (1)
211-213: LGTM! Removal of console.warn test aligns with logging reduction goal.
The commented-out test expectation for console.warn is consistent with the PR's objective to reduce logging in the codebase.
packages/state/src/recs/index.ts (2)
145-145: LGTM: Improved logging control
The change properly gates logging behind the logging flag, aligning with the PR objective to reduce logging.
142-142: Verify client implementation of order_by parameter
Let's verify if the Torii client properly implements the order_by functionality.
packages/sdk/src/index.ts
Outdated
| @@ -135,7 +135,7 @@ export async function init<T extends SchemaType>( | |||
| sendMessage: async ( | |||
| data: TypedData, | |||
| account: Account, | |||
| isSessionSignature: boolean = false | |||
| _isSessionSignature: boolean = false | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing unused parameter.
The underscore prefix in _isSessionSignature suggests this parameter is unused. If it's no longer needed, consider removing it entirely.
- _isSessionSignature: boolean = false
packages/state/src/recs/index.ts
Outdated
| @@ -139,9 +139,10 @@ export const getEntities = async <S extends Schema>( | |||
| offset, | |||
| clause, | |||
| dont_include_hashed_keys: false, | |||
| order_by: [], | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and documentation for the order_by parameter
The newly added order_by parameter has several concerns:
- Using
any[]reduces type safety. Consider defining a proper type for the ordering criteria. - The parameter is not documented in the function's JSDoc.
- An empty array default value might not provide value if not implemented in the underlying client.
Consider defining a proper type and updating the documentation:
// Define a type for the ordering criteria
type OrderBy = {
field: string;
direction: 'asc' | 'desc';
};
// Update the function signature and documentation
/**
* @param order_by - Optional array of ordering criteria for the results
*/
export const getEntities = async <S extends Schema>(
// ... other params ...
order_by: OrderBy[] = []
) => {Also applies to: 189-189, 261-261
| if (logging) | ||
| console.warn( | ||
| `Component ${componentName} not found in provided components.` | ||
| ); |
There was a problem hiding this comment.
Reconsider gating critical warning messages
While the change aligns with reducing logging, missing components might indicate a critical issue that developers should always be aware of. Consider keeping this warning unconditional or using a different logging level (error) that's always enabled.
Consider reverting this change or using error logging:
- if (logging)
- console.warn(
- `Component ${componentName} not found in provided components.`
- );
+ console.error(
+ `Component ${componentName} not found in provided components.`
+ );📝 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.
| if (logging) | |
| console.warn( | |
| `Component ${componentName} not found in provided components.` | |
| ); | |
| console.error( | |
| `Component ${componentName} not found in provided components.` | |
| ); |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
convertValuesfunction, particularly for large numbers and complex data structures.Chores