fix optimistic travel/explore

aymericdelab commented 1 month ago

PR Type

Bug fix, Enhancement

Description

Added Stamina component to the list of overridable components.
Simplified the explore function by removing the finally block and adjusted error handling.
Introduced optimisticTravelHex for handling optimistic updates in travel and integrated it into travelToHex.
Improved error handling in travelToHex to revert position overrides and update animation paths correctly.

Changes walkthrough 📝

Relevant files

Enhancement

createClientComponents.ts `Add Stamina component to client components` client/src/dojo/createClientComponents.ts - Added `Stamina` component to the list of overridable components.	+1/-0
useTravel.tsx `Add optimistic travel and improve error handling` client/src/hooks/helpers/useTravel.tsx Added a new function `optimisticTravelHex` for handling optimistic updates. Integrated `optimisticTravelHex` into the `travelToHex` function. Improved error handling in `travelToHex` to revert position overrides.	+24/-1

Bug fix

useExplore.tsx

Simplify explore function and improve error handling

client/src/hooks/helpers/useExplore.tsx

Added a TODO comment for adding stamina.

Simplified the explore function by removing the finally block.

Adjusted error handling to log errors and update animation paths
correctly.

+9/-12

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

vercel[bot] commented 1 month ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
eternum	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 14, 2024 10:24am

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Error Handling: The removal of the `finally` block in `useExplore.tsx` might lead to scenarios where overrides are not removed if an exception occurs before the catch block. Consider adding finally to ensure cleanup.
	Logging: Direct use of `console.log` for error logging in `useExplore.tsx` is found. It's better to use a more robust logging approach or error handling strategy.
	Commented Code: There are `todo` comments related to stamina in both `useExplore.tsx` and `useTravel.tsx`. Ensure that these todos are addressed or tracked in your project management tool.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Error handling	Add error handling in the `optimisticTravelHex` function to manage exceptions ___ The `optimisticTravelHex` function currently does not handle exceptions that might occur during the execution of `getEntityIdFromKeys` or `addOverride`. It's recommended to add error handling within this function to manage such cases gracefully. [client/src/hooks/helpers/useTravel.tsx [33-47]](https://github.com/BibliothecaDAO/eternum/pull/931/files#diff-a3ae3bb7c20e0f95e121cc173b192dadd3a166fd8c2dc84cd37c28c8ed59126aR33-R47) ```diff const optimisticTravelHex = (entityId: bigint, col: number, row: number) => { - let overrideId = uuid(); - const entity = getEntityIdFromKeys([entityId]); - components.Position.addOverride(overrideId, { - entity, - value: { - entity_id: entityId, - x: col, - y: row, - }, - }); - return overrideId; + try { + let overrideId = uuid(); + const entity = getEntityIdFromKeys([entityId]); + components.Position.addOverride(overrideId, { + entity, + value: { + entity_id: entityId, + x: col, + y: row, + }, + }); + return overrideId; + } catch (error) { + logError("Failed to create optimistic travel hex", error); + throw error; // Rethrow or handle as needed + } }; ``` Suggestion importance[1-10]: 9 Why: Adding error handling within the `optimisticTravelHex` function is crucial for managing potential exceptions and ensuring the application remains stable. This suggestion addresses a significant issue and provides a clear implementation.	9
Error handling	Improve error handling in the explore function to include retries or user feedback ___ Consider handling the promise rejection from `explore` function more robustly. Currently, the catch block only logs the error and updates the UI, but it might be beneficial to also retry the operation or provide a user feedback mechanism. [client/src/hooks/helpers/useExplore.tsx [145-154]](https://github.com/BibliothecaDAO/eternum/pull/931/files#diff-45821874b656c15cf5d9d6a603afd6d4858df5008f6353317b5a2d75482aa9a9R145-R154) ```diff explore({ unit_id: explorerId, direction, signer: account, }).catch((e) => { - removeHex(path[1].x - FELT_CENTER, path[1].y - FELT_CENTER); - Position.removeOverride(overrideId); - setAnimationPaths([...prevPaths, { id: explorerId, path: [path[path.length - 1]], enemy: false }]); - console.log("error exploring", e); + handleExploreError(e, explorerId, path, overrideId); }); ``` Suggestion importance[1-10]: 7 Why: Enhancing error handling to include retries or user feedback can improve the user experience and make the application more resilient to transient errors. However, the suggestion is somewhat general and lacks specific implementation details.	7
Best practice	Replace console logging with a robust error handling mechanism ___ Replace the `console.log` with a more robust error handling mechanism. Using `console.log` for error reporting in production code is not recommended as it does not provide a way to centrally manage or track errors. Consider using a logging library that supports error level logging and possibly remote error reporting. [client/src/hooks/helpers/useExplore.tsx [153]](https://github.com/BibliothecaDAO/eternum/pull/931/files#diff-45821874b656c15cf5d9d6a603afd6d4858df5008f6353317b5a2d75482aa9a9R153-R153) ```diff -console.log("error exploring", e); +logError("error exploring", e); ``` Suggestion importance[1-10]: 8 Why: Replacing `console.log` with a more robust error handling mechanism is a best practice for production code. It improves error tracking and management, which is crucial for maintaining the application.	8
Possible issue	Ensure proper initialization of the `Stamina` component before use ___ Ensure that the `Stamina` component is properly initialized before adding it to the `contractComponents`. This might involve checking if `contractComponents.Stamina` is defined or ensuring that it meets certain conditions before its use. [client/src/dojo/createClientComponents.ts [12]](https://github.com/BibliothecaDAO/eternum/pull/931/files#diff-43af230b36a67fafb1de179ffe9bd30032b1ae8ed34934aa5acbea6b61b3cb69R12-R12) ```diff -Stamina: overridableComponent(contractComponents.Stamina), +Stamina: contractComponents.Stamina ? overridableComponent(contractComponents.Stamina) : undefined, ``` Suggestion importance[1-10]: 6 Why: Ensuring that the `Stamina` component is properly initialized before use can prevent potential runtime errors. However, the suggestion could be more robust by specifying what should happen if `contractComponents.Stamina` is undefined.	6

BibliothecaDAO / eternum