fix: utils - change way to compute selector hash #250
Conversation
|
Warning Rate limit exceeded@MartianGreed has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 0 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes enhance the functionality and maintainability of the Dojo.js codebase, focusing on improved logging, error handling, and testing capabilities. Key modifications include refining entity management functions, explicitly defining types for better clarity, and migrating the testing framework to Vitest, which streamlines coverage reporting and strengthens overall reliability. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
MartianGreed
force-pushed
the
fix/selector-hash
branch
from
7bac4a7
to
84d936d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (6)
- packages/state/src/recs/index.ts (2 hunks)
- packages/state/src/utils/index.ts (1 hunks)
- packages/utils/coverage/.tmp/coverage-0.json (1 hunks)
- packages/utils/package.json (2 hunks)
- packages/utils/src/test/utils/index.test.ts (1 hunks)
- packages/utils/src/utils/index.ts (5 hunks)
Additional context used
Biome
packages/state/src/recs/index.ts
[error] 189-189: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 194-194: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
Additional comments not posted (22)
packages/utils/package.json (2)
11-12: Approved: Update testing framework to Vitest.Switching to Vitest for testing and adding a coverage script enhances the testing infrastructure.
23-25: Approved: Add Vitest and coverage dependencies.Adding
@vitest/coverage-v8andvitestis necessary for the new testing framework and coverage functionality.packages/utils/coverage/.tmp/coverage-0.json (1)
1-38: Approved: Coverage report generated by Vitest.The coverage report contains valid JSON and relevant coverage information.
packages/state/src/utils/index.ts (3)
1-1: Approved: Import necessary types.The import statements are correct and necessary for the function.
3-3: Approved: Add explicit return type.Adding an explicit return type
ComponentValueenhances type safety and clarity.
4-82: Approved: Improve readability and maintainability.The function logic remains consistent with the previous implementation, with improvements in readability and maintainability.
packages/utils/src/_test_/utils/index.test.ts (5)
11-33: Good coverage forgetComponentNameFromEvent.The tests cover multiple scenarios for
getComponentNameFromEvent. Ensure that edge cases, such as empty arrays or invalid data, are also tested.
53-71: Good exception handling test forgetComponentNameFromEvent.The test correctly verifies that an exception is thrown when the action is not found. Consider adding more edge cases to ensure robustness.
73-81: Good coverage forsplitEventTag.The tests cover multiple scenarios for
splitEventTag. Ensure that edge cases, such as empty strings or strings without separators, are also tested.
83-88: Good coverage forcomputeByteArrayHash.The test correctly verifies the hash computation. Ensure that edge cases, such as empty strings or very large strings, are also tested.
90-105: Good coverage forgetSelectorFromTag.The tests cover multiple scenarios for
getSelectorFromTag. Ensure that edge cases, such as empty strings or invalid formats, are also tested.packages/state/src/recs/index.ts (2)
74-74: Approve logging statement ingetEntities.The logging statement enhances traceability. Ensure that no sensitive information is logged.
202-215: Approve error handling insetEntities.The error handling around
setComponentis robust and provides useful feedback for developers.packages/utils/src/utils/index.ts (9)
Line range hint
13-27: ApprovegetEventsfunction.The filtering logic is straightforward and correct. Ensure that the criteria cover all relevant scenarios.
Line range hint
30-35: ApprovesetComponentsFromEventsfunction.The iteration and update logic is correct. Ensure that the function handles edge cases and errors gracefully.
52-92: ApprovesetComponentFromEventfunction.The update logic and error handling are robust. Ensure that the function handles edge cases and errors gracefully.
95-102: ApprovegetComponentNamesfunction.The extraction logic is correct. Ensure that the function handles edge cases and errors gracefully.
Line range hint
103-115: ApproveparseComponentValuefunction.The parsing logic is correct. Ensure that the function handles edge cases and errors gracefully.
Line range hint
116-122: ApprovedecodeComponentfunction.The decoding logic is correct. Ensure that the function handles edge cases and errors gracefully.
185-191: ApprovehexToAsciifunction.The conversion logic is correct. Ensure that the function handles edge cases and errors gracefully.
196-215: ApprovegetComponentNameFromEventfunction.The retrieval logic is correct. Ensure that the function handles edge cases and errors gracefully.
224-230: ApprovegetSelectorFromTagfunction.The computation logic is correct. Ensure that the function handles edge cases and errors gracefully.
Comments failed to post (2)
packages/state/src/recs/index.ts
194-195: Replace
hasOwnPropertywithObject.hasOwn().It's recommended to use
Object.hasOwn()instead ofhasOwnProperty()to avoid potential issues with prototype chain.- if (!entities[key].hasOwnProperty(componentName)) { + if (!Object.hasOwn(entities[key], componentName)) {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 (!Object.hasOwn(entities[key], componentName)) { continue;Tools
Biome
[error] 194-194: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
189-190: Replace
hasOwnPropertywithObject.hasOwn().It's recommended to use
Object.hasOwn()instead ofhasOwnProperty()to avoid potential issues with prototype chain.- if (!entities.hasOwnProperty(key)) { + if (!Object.hasOwn(entities, key)) {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 (!Object.hasOwn(entities, key)) { continue;Tools
Biome
[error] 189-189: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (6)
- packages/state/src/recs/index.ts (2 hunks)
- packages/state/src/utils/index.ts (1 hunks)
- packages/utils/coverage/.tmp/coverage-0.json (1 hunks)
- packages/utils/package.json (2 hunks)
- packages/utils/src/test/utils/index.test.ts (1 hunks)
- packages/utils/src/utils/index.ts (5 hunks)
Files skipped from review due to trivial changes (1)
- packages/utils/coverage/.tmp/coverage-0.json
Files skipped from review as they are similar to previous changes (3)
- packages/state/src/utils/index.ts
- packages/utils/package.json
- packages/utils/src/test/utils/index.test.ts
Additional context used
Biome
packages/state/src/recs/index.ts
[error] 189-189: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 194-194: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
Additional comments not posted (9)
packages/state/src/recs/index.ts (4)
74-74: Logging added for fetched entities and components.The added logging statement enhances traceability and aids in debugging. Ensure that the logged data does not contain sensitive information.
189-191: Refined logic for handling entity properties.The inverted check for property existence improves readability.
Tools
Biome
[error] 189-189: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
194-196: Refined logic for handling component properties.The inverted check for property existence improves readability.
Tools
Biome
[error] 194-194: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
201-215: Enhanced error handling aroundsetComponent.The added try-catch block improves robustness by preventing the entire function from failing due to a single error and provides useful feedback for developers.
packages/utils/src/utils/index.ts (5)
13-14: Use of constantSTORE_SET_RECORD_EVENT_NAME.The use of a constant improves maintainability by avoiding hardcoded strings.
Also applies to: 27-27
52-92: Refactored logic for better error handling and clarity.The refactored logic and the try-catch block around
getComponentNameFromEventimprove robustness and readability.
185-191: New functionhexToAsciiadded.The function converts a hexadecimal string to an ASCII string. Ensure that the function handles edge cases and invalid input gracefully.
196-216: New functiongetComponentNameFromEventadded.The function retrieves the component name from an event. The use of a map for action-felt pairs improves efficiency. Ensure that the function handles cases where the action is not found.
224-231: New functiongetSelectorFromTagadded.The function computes a selector from a namespace and an event name. The use of
poseidonHashManyfor hashing improves security. Ensure that the function handles invalid input gracefully.
packages/state/src/recs/index.ts
Outdated
| if (!entities.hasOwnProperty(key)) { | ||
| continue; | ||
| } | ||
|
|
||
| for (let componentName in entities[key]) { | ||
| if (!entities[key].hasOwnProperty(componentName)) { |
There was a problem hiding this comment.
Use Object.hasOwn() instead of Object.prototype.hasOwnProperty.
It's recommended to use Object.hasOwn() for checking property existence.
- if (!entities.hasOwnProperty(key)) {
+ if (!Object.hasOwn(entities, key)) {- if (!entities[key].hasOwnProperty(componentName)) {
+ if (!Object.hasOwn(entities[key], componentName)) {Tools
Biome
[error] 189-189: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 194-194: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
MartianGreed
force-pushed
the
fix/selector-hash
branch
from
84d936d
to
99c0c86
Compare
MartianGreed
changed the title
packages/state/src/recs/index.ts
Outdated
| @@ -71,6 +71,7 @@ export const getEntities = async <S extends Schema>( | |||
|
|
|||
| while (continueFetching) { | |||
| const entities = await client.getAllEntities(limit, cursor); | |||
| console.log(entities, components); | |||
There was a problem hiding this comment.
we should remove this
|
Looks good other than that, nice iteration |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (6)
- packages/state/src/recs/index.ts (1 hunks)
- packages/state/src/utils/index.ts (1 hunks)
- packages/utils/coverage/.tmp/coverage-0.json (1 hunks)
- packages/utils/package.json (2 hunks)
- packages/utils/src/test/utils/index.test.ts (1 hunks)
- packages/utils/src/utils/index.ts (5 hunks)
Files skipped from review due to trivial changes (1)
- packages/utils/coverage/.tmp/coverage-0.json
Files skipped from review as they are similar to previous changes (2)
- packages/state/src/utils/index.ts
- packages/utils/src/test/utils/index.test.ts
Additional comments not posted (17)
packages/utils/package.json (3)
11-12: Transition to Vitest for testing.The test script has been updated to use Vitest instead of Jest. This change is correct and aligns with the objective of improving testing capabilities.
12-12: Addition of coverage reporting script.A new script for coverage reporting using Vitest has been added. This enhances the project's testing capabilities.
24-25: Addition of Vitest dependencies.New dependencies for Vitest and coverage reporting have been added. This is necessary for the transition to the new testing framework.
packages/state/src/recs/index.ts (3)
188-189: UseObject.hasOwn()instead ofObject.prototype.hasOwnProperty.It's recommended to use
Object.hasOwn()for checking property existence. This change improves code readability and reduces potential errors.
193-194: UseObject.hasOwn()instead ofObject.prototype.hasOwnProperty.It's recommended to use
Object.hasOwn()for checking property existence. This change improves code readability and reduces potential errors.
200-214: Improved error handling insetComponentcall.The try-catch block around the
setComponentcall ensures that errors are caught and logged, preventing the entire function from failing due to a single issue. This change improves the robustness and maintainability of the function.packages/utils/src/utils/index.ts (11)
13-14: Addition ofSTORE_SET_RECORD_EVENT_NAMEconstant.The addition of this constant improves code maintainability by replacing a hardcoded string.
27-27: Use ofSTORE_SET_RECORD_EVENT_NAMEingetEventsfunction.The use of the constant
STORE_SET_RECORD_EVENT_NAMEin thegetEventsfunction enhances clarity and reduces potential errors from string literals.
52-93: RefactoredsetComponentFromEventfunction.The logic in the
setComponentFromEventfunction has been refactored for better error handling and clarity. The retrieval of the component name is now encapsulated in a new function,getComponentNameFromEvent, which modularizes the code and adds robustness by throwing an error if the action is not found.
95-102: Addition ofgetComponentNameshelper function.This helper function extracts component names from components, improving code readability and modularity.
185-186: Addition ofhexToAsciifunction.This utility function converts a hexadecimal string to an ASCII string, adding useful functionality for handling hex strings.
193-216: Addition ofgetComponentNameFromEventfunction.This function retrieves the component name from an event, improving code modularity and error handling by throwing an error if the action is not found.
218-221: Addition oftoHexStringhelper function.This helper function encodes a big number to a formatted hex string, adding useful functionality for handling big numbers.
223-231: Addition ofgetSelectorFromTagfunction.This function computes a dojo selector from a namespace and event name, adding useful functionality for generating selectors.
233-242: Addition ofserializeByteArrayfunction.This function serializes a
ByteArrayto abigintarray, adding useful functionality for handling byte arrays.
245-248: Addition ofcomputeByteArrayHashfunction.This function computes the Poseidon hash of a string represented as a
ByteArray, adding useful functionality for generating hashes.
250-253: Addition ofsplitEventTagfunction.This function splits a selector name into a namespace and event name, adding useful functionality for handling event tags.
MartianGreed
force-pushed
the
fix/selector-hash
branch
from
8f0d4be
to
ead2a2d
Compare
Closes #247
Introduced changes
Changes way to get component from event hash.
This will not fix the recs issue about Map receiver.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores