fix: type build #307
fix: type build #307
Conversation
WalkthroughThe changes in this pull request involve modifications to several files. The Changes
Possibly related PRs
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: 1
🧹 Outside diff range and nitpick comments (2)
examples/example-vite-react-sdk/src/App.tsx (1)
69-70: Consider removing or conditionally enabling debug logging.While the entity update logic is good, consider removing the console.log or making it conditional based on a debug flag.
-console.log("subscribed", response.data[0]); +if (process.env.NODE_ENV === 'development') { + console.log("subscribed", response.data[0]); +}packages/sdk/src/state/zustand.ts (1)
93-95: Avoid usinganyin type assertionsUsing
as anyat line 95 bypasses TypeScript's type checking and may introduce bugs. Consider defining a more specific type instead of usingany.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- examples/example-vite-react-sdk/src/App.tsx (3 hunks)
- packages/sdk/src/state/zustand.ts (1 hunks)
🔇 Additional comments (3)
examples/example-vite-react-sdk/src/App.tsx (2)
46-56: LGTM: Well-structured Position entity subscription.The Position entity subscription follows the established query pattern and properly handles address padding.
227-230: Verify server-side movement validation.The client-side boundary checks have been removed. Please verify that proper server-side validation exists to prevent invalid movements.
packages/sdk/src/state/zustand.ts (1)
77-78:⚠️ Potential issueHandle updates when
entity.modelsis not providedCurrently, the
updateEntityfunction only updates the entity ifentity.modelsis present. This means updates to other properties of the entity will be ignored ifentity.modelsis undefined. Consider adjusting the condition to allow updates even whenentity.modelsis not provided.Apply this diff to modify the condition:
- if (entity.entityId && state.entities[entity.entityId] && entity.models) { + if (entity.entityId && state.entities[entity.entityId]) {Likely invalid or redundant comment.
| const mergedModels: typeof existingEntity.models = | ||
| Object.assign({}, existingEntity.models); | ||
|
|
||
| // Iterate through each namespace in the new models | ||
| Object.entries(entity.models).forEach( | ||
| ([namespace, namespaceModels]) => { | ||
| const typedNamespace = | ||
| namespace as keyof ParsedEntity<T>["models"]; | ||
| if (!(typedNamespace in mergedModels)) { | ||
| mergedModels[ | ||
| typedNamespace as keyof typeof mergedModels | ||
| ] = {} as any; | ||
| } | ||
|
|
||
| mergedModels[ | ||
| typedNamespace as keyof typeof mergedModels | ||
| ] = Object.assign( | ||
| {}, | ||
| mergedModels[ | ||
| typedNamespace as keyof typeof mergedModels | ||
| ], | ||
| namespaceModels | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the models merging logic for readability
The current implementation for merging models is complex and may be difficult to maintain. Consider simplifying the code using a utility function or a recursive merge to enhance readability.
For example, you could use a deep merge utility:
- // Existing models merging code
- const mergedModels: typeof existingEntity.models = Object.assign({}, existingEntity.models);
- Object.entries(entity.models).forEach(([namespace, namespaceModels]) => {
- const typedNamespace = namespace as keyof ParsedEntity<T>["models"];
- if (!(typedNamespace in mergedModels)) {
- mergedModels[typedNamespace as keyof typeof mergedModels] = {} as any;
- }
- mergedModels[typedNamespace as keyof typeof mergedModels] = Object.assign(
- {},
- mergedModels[typedNamespace as keyof typeof mergedModels],
- namespaceModels
- );
- });
+ const mergedModels = { ...existingEntity.models, ...entity.models };If a deep merge is required to merge nested objects, you can implement a helper function like this:
function deepMerge(target: any, source: any): any {
for (const key of Object.keys(source)) {
if (
source[key] instanceof Object &&
key in target &&
target[key] instanceof Object
) {
Object.assign(source[key], deepMerge(target[key], source[key]));
}
}
return { ...target, ...source };
}And then use it in your code:
const mergedModels = deepMerge(existingEntity.models, entity.models);
Summary by CodeRabbit
New Features
BurnerProviderPropsinterface is now publicly accessible for use in other modules.Bug Fixes
Documentation