Enh/walkthrough - Githubissues

User description

Soft onboarding process w/ quests
Add visual queues for quests (where to click)
Add buttons in menus to open hints at specified section
Refactored useQuests as a store

PR Type

Enhancement, Bug fix, Documentation

Description

Implemented a new useQuestStore using Zustand for state management.
Removed the old useQuests hook and replaced it with the new store-based approach.
Added quest-related visual cues and interactions across various components.
Added hint buttons in multiple components to open the hint modal.
Updated navigation button visibility and interactions based on quest state.
Refactored HintModal to include an initial active section prop and moved Resources component to a separate file.
Fixed small issues and performed linting.

Changes walkthrough 📝

Relevant files

Enhancement

19 files

useQuestStore.tsx `Implement Zustand-based Quest Store and Hook` client/src/hooks/store/useQuestStore.tsx Created a new `useQuestStore` using Zustand for state management. Defined quest-related types and interfaces. Implemented `useQuests` hook to manage quest logic and state.	+175/-0
useQuests.tsx `Remove Deprecated useQuests Hook` client/src/hooks/helpers/useQuests.tsx - Removed the old `useQuests` hook.	+0/-99
useUIStore.tsx `Extend UI Store for Navigation Views` client/src/hooks/store/useUIStore.tsx Added right navigation view state management. Updated left navigation view state management.	+10/-5
main.tsx `Enable TourProvider with Empty Steps` client/src/main.tsx - Enabled the `TourProvider` with empty steps.	+7/-7
HooksComponent.tsx `Integrate useQuests Hook in HooksComponent` client/src/ui/components/HooksComponent.tsx - Added `useQuests` hook to the component.	+2/-0
SelectPreviewBuilding.tsx `Add Quest Visual Cues and Hint Button` client/src/ui/components/construction/SelectPreviewBuilding.tsx Added quest-related visual cues and interactions. Added hint button to open the hint modal.	+67/-5
HintBox.tsx `Update HintBox to Use Quest Store and Display Steps` client/src/ui/components/hints/HintBox.tsx Updated to use the new `useQuestStore`. Added quest steps display.	+27/-38
HintModal.tsx `Refactor HintModal and Extract Resources Component` client/src/ui/components/hints/HintModal.tsx Refactored to include initial active section prop. Moved Resources component to a separate file.	+13/-83
Resources.tsx `Create Resources Component` client/src/ui/components/hints/Resources.tsx - Created a new Resources component.	+80/-0
EntityList.tsx `Add Questing Visual Cue to Entity List` client/src/ui/components/list/EntityList.tsx - Added questing visual cue to entity list items.	+9/-2
ArmyList.tsx `Add Quest Visual Cues to Army List` client/src/ui/components/military/ArmyList.tsx - Added quest-related visual cues to army creation and enlistment.	+13/-1
MarketModal.tsx `Add Hint Button to Market Modal` client/src/ui/components/trading/MarketModal.tsx - Added hint button to open the hint modal.	+15/-0
LeaderboardPanel.tsx `Add Hint Button to Leaderboard Panel` client/src/ui/components/worldmap/leaderboard/LeaderboardPanel.tsx - Added hint button to open the hint modal.	+10/-1
Military.tsx `Add Hint Button to Military Module` client/src/ui/modules/military/Military.tsx - Added hint button to open the hint modal.	+12/-1
BottomNavigation.tsx `Add Quest Visual Cues and Update Notifications` client/src/ui/modules/navigation/BottomNavigation.tsx Added quest-related visual cues and interactions. Updated quest notification logic.	+20/-17
LeftNavigationModule.tsx `Add Quest Visual Cues and Update Navigation Visibility` client/src/ui/modules/navigation/LeftNavigationModule.tsx Added quest-related visual cues and interactions. Updated navigation button visibility based on quest state.	+49/-6
RightNavigationModule.tsx `Add Quest Visual Cues and Update Navigation Visibility` client/src/ui/modules/navigation/RightNavigationModule.tsx Added quest-related visual cues and interactions. Updated navigation button visibility based on quest state.	+115/-59
TopMiddleNavigation.tsx `Disable Navigation Button When Quests Are Claimable` client/src/ui/modules/navigation/TopMiddleNavigation.tsx - Disabled navigation button when quests are claimable.	+4/-0
Steps.tsx `Remove Deprecated Quest Logic from Onboarding Steps` client/src/ui/modules/onboarding/Steps.tsx - Removed old quest-related logic.	+9/-12

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

Name	Status	Preview	Comments	Updated (UTC)
eternum	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 21, 2024 11:46am

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Ensure `entityId` is always defined before conversion to `BigInt` to prevent runtime errors ___ To avoid potential type coercion errors with the `BigInt` constructor, ensure that `entityId` is always defined before converting it to `BigInt`. This can be done by providing a fallback value directly in the `useMemo` where `entityId` is defined. [client/src/hooks/store/useQuestStore.tsx [73]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR73-R73) ```diff -const entityId = playerRealms()[0]?.entity_id; +const entityId = playerRealms()[0]?.entity_id \|\| "0"; ``` Suggestion importance[1-10]: 10 Why: Ensuring `entityId` is always defined before conversion to `BigInt` prevents potential runtime errors, which is a significant improvement for code robustness and reliability.	10
Possible bug	Improve the safety of accessing properties on potentially undefined objects ___ Consider checking if `selectedEntityId` is defined before attempting to access its `id` property to avoid potential runtime errors. You can use optional chaining (`?.`) for safer access. [client/src/ui/modules/navigation/BottomNavigation.tsx [46]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-e88ab2bf26cead1655ae59538674a03ae7a573c4c6ea347a8ad382f197b3cc4dR46-R46) ```diff -const army = getArmyByEntityId(selectedEntityId?.id \|\| BigInt("0")); +const army = getArmyByEntityId(selectedEntityId?.id ?? BigInt("0")); ``` Suggestion importance[1-10]: 8 Why: The suggestion improves the safety of accessing properties on potentially undefined objects, which can prevent runtime errors.	8
Performance	Add a `key` prop to the parent in the map function for better performance ___ Add a key to the parent element in the map function to ensure that React can efficiently update and render elements. This is particularly important in lists to prevent rendering issues and improve performance. [client/src/ui/components/hints/HintBox.tsx [70-71]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR70-R71) ```diff quest.steps.map(({ description, completed }, index) => ( - + ``` Suggestion importance[1-10]: 9 Why: Adding a `key` prop to the parent ` ` in the map function is important for React to efficiently update and render elements, preventing rendering issues and improving performance.	9
	Improve the definition of `isOffscreen` function to avoid unnecessary redefinitions ___ To ensure that the `isOffscreen` function is not redefined every time the component re-renders, it should be defined outside of the component or wrapped in a `useCallback` hook if it depends on props or state. [client/src/ui/modules/navigation/RightNavigationModule.tsx [252-254]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR252-R254) ```diff -const isOffscreen = (view: View) => { +const isOffscreen = useCallback((view: View) => { return view === View.None; -}; +}, []); ``` Suggestion importance[1-10]: 9 Why: Wrapping the `isOffscreen` function in a `useCallback` hook improves performance by preventing unnecessary redefinitions on each render, which is crucial for maintaining efficient rendering in React components.	9
	Move `armyHasTroops` outside of `useQuests` to prevent unnecessary re-creations ___ The function `armyHasTroops` could be moved outside of the component to prevent it from being recreated on every render. This would improve performance, especially if `useQuests` is used frequently across re-renders. [client/src/hooks/store/useQuestStore.tsx [167-175]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR167-R175) ```diff const armyHasTroops = (entityArmies: ArmyInfo[]) => {...}; +export const useQuests = () => {...}; ``` Suggestion importance[1-10]: 8 Why: Moving `armyHasTroops` outside of `useQuests` prevents unnecessary re-creations of the function, which can improve performance. This is a good practice for optimizing frequently used hooks.	8
	Improve the stability and efficiency of useMemo by using a stable hash of the account object ___ Consider using a more robust key extraction method for `useMemo` to avoid unnecessary recalculations. The current dependencies list includes `account.address` which might not be stable across re-renders. Using a stable hash of the `account` object or specific stable properties might be more efficient. [client/src/hooks/store/useQuestStore.tsx [150]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR150-R150) ```diff -useMemo(() => {...}, [farms, resource, orders, entityArmies, HasClaimedStartingResources, entityId, account.address]); +useMemo(() => {...}, [farms, resource, orders, entityArmies, HasClaimedStartingResources, entityId, JSON.stringify(account)]); ``` Suggestion importance[1-10]: 7 Why: This suggestion improves the stability and efficiency of the `useMemo` hook by using a stable hash of the account object, which can prevent unnecessary recalculations. However, the improvement is minor and does not address a critical issue.	7
	Optimize performance by avoiding repeated BigInt instantiation ___ Avoid using `BigInt("0")` as a fallback value directly inside the component render. Instead, define a constant outside of the component to prevent creating a new `BigInt` on every render. [client/src/ui/modules/navigation/BottomNavigation.tsx [46]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-e88ab2bf26cead1655ae59538674a03ae7a573c4c6ea347a8ad382f197b3cc4dR46-R46) ```diff -const army = getArmyByEntityId(selectedEntityId?.id \|\| BigInt("0")); +const DEFAULT_ENTITY_ID = BigInt("0"); +const army = getArmyByEntityId(selectedEntityId?.id \|\| DEFAULT_ENTITY_ID); ``` Suggestion importance[1-10]: 7 Why: The suggestion optimizes performance by avoiding repeated BigInt instantiation, which is a good practice for improving efficiency.	7
	Simplify the initialization of `activeSection` state for efficiency ___ Initialize the `activeSection` state directly with the `initialActiveSection` prop if it exists, otherwise default to the first section. This simplifies the code by removing the need for the `find` operation on the sections array, which can be inefficient if the array is large. [client/src/ui/components/hints/HintModal.tsx [65-67]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-1dd2daf32164d13d632c2df30c82cc13776cd68a555f9775678593b6abe75783R65-R67) ```diff -const [activeSection, setActiveSection] = useState( - sections.find((section) => section.name === initialActiveSection) \|\| sections[0], -); +const [activeSection, setActiveSection] = useState(initialActiveSection \|\| sections[0].name); ``` Suggestion importance[1-10]: 6 Why: Simplifying the initialization of `activeSection` state can improve code readability and slightly enhance performance. However, the performance gain is minimal and the current code is not incorrect.	6
Best practice	Add missing dependencies to useEffect to ensure it updates correctly ___ Refactor the `useEffect` hook at line 152 to include `quests` in its dependencies array. This ensures that the effect correctly responds to changes in `quests`, aligning with React's best practices for effects that depend on props or state. [client/src/hooks/store/useQuestStore.tsx [152-154]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR152-R154) ```diff useEffect(() => { setQuests(quests); -}, []); +}, [quests]); ``` Suggestion importance[1-10]: 9 Why: Adding `quests` to the dependencies array of the `useEffect` hook ensures that the effect updates correctly when `quests` change, aligning with React's best practices. This is crucial for the correct functioning of the component.	9
	Ensure useMemo dependencies are correctly specified for accurate updates ___ The `useMemo` hook for `isWorldView` is missing its dependencies. Include `location` in the dependency array to ensure it updates correctly when `location` changes. [client/src/ui/modules/navigation/BottomNavigation.tsx [42]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-e88ab2bf26cead1655ae59538674a03ae7a573c4c6ea347a8ad382f197b3cc4dR42-R42) ```diff +const isWorldView = useMemo(() => location === "/map", [location]); - ``` Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a missing dependency in the `useMemo` hook, ensuring that the memoized value updates accurately when `location` changes.	9
	Use type assertion instead of `Number` for type safety in `findResourceById` ___ Avoid using `Number` for type casting directly within the `findResourceById` function call. Instead, ensure that `resourceId` is the correct type earlier in the code or use TypeScript's type assertion more safely. [client/src/ui/components/hints/Resources.tsx [18]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-d19afd508f5ca24a4c6b7aa3350cf9d813e38ea9f6dcf8a76d2fda9e3ae08d8dR18-R18) ```diff -resource: findResourceById(Number(resourceId)), +resource: findResourceById(resourceId as number), ``` Suggestion importance[1-10]: 8 Why: Using type assertion instead of `Number` for type casting improves type safety and aligns with TypeScript best practices, reducing the risk of runtime errors.	8
	Replace array indexes with unique identifiers as keys in React list items ___ It is recommended to avoid using indexes as keys in React lists if the order of items may change, as it could lead to performance issues and bugs. Use a unique identifier if available. [client/src/ui/modules/navigation/RightNavigationModule.tsx [178-180]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR178-R180) ```diff -{navigation.map((a, index) => ( - {a.button} +{navigation.map((navItem) => ( + {navItem.button} ))} ``` Suggestion importance[1-10]: 8 Why: Using unique identifiers instead of array indexes as keys in React list items is a best practice that helps prevent potential performance issues and bugs related to reordering of items.	8
	Improve type safety by using a more specific type for the `entityId` prop ___ Consider using a more specific type for the `entityId` prop in the `HintBox` component to ensure type safety. Currently, it is typed as `bigint`, which is very generic. If `entityId` is expected to be a specific kind of identifier, using a more specific type or creating a custom type could help catch errors during development. [client/src/ui/components/hints/HintBox.tsx [9]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR9-R9) ```diff -export const HintBox = ({ quest, entityId }: { quest: Quest; entityId: bigint }) => { +export const HintBox = ({ quest, entityId }: { quest: Quest; entityId: EntityIdType }) => { ``` Suggestion importance[1-10]: 7 Why: Using a more specific type for `entityId` can improve type safety and catch errors during development. However, the suggestion is not crucial and does not address a major bug or issue.	7
Maintainability	Use more descriptive variable names to enhance code readability ___ Consider using a more descriptive variable name than `a` for the map function in the `navigation` array to improve code readability. [client/src/ui/modules/navigation/RightNavigationModule.tsx [178-180]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR178-R180) ```diff -{navigation.map((a, index) => ( - {a.button} +{navigation.map((navItem, index) => ( + {navItem.button} ))} ``` Suggestion importance[1-10]: 7 Why: Using a more descriptive variable name enhances code readability and maintainability, making it easier for other developers to understand the code.	7
Enhancement	Simplify class application by directly using conditional expressions instead of `clsx` when unnecessary ___ The `clsx` function is used to conditionally apply classes, but it's being used without conditional classes in some instances. You can directly pass the class names to the `className` prop when no conditions are applied. [client/src/ui/modules/navigation/RightNavigationModule.tsx [76-89]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR76-R89) ```diff { setLastView(View.ResourceTable); setView(View.ResourceTable); }} /> ``` Suggestion importance[1-10]: 6 Why: Simplifying the class application by directly using conditional expressions instead of `clsx` when unnecessary can make the code more straightforward and slightly improve performance.	6

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The removal of the `useQuests` hook and its replacement with `useQuestStore` might lead to issues if other parts of the application still depend on the old hook. It's crucial to ensure that all references to `useQuests` are updated.
	Refactoring Scope: The refactoring of the HintModal to include an initial active section prop could affect how modals are displayed across the application. Verify that this change does not disrupt the modal behavior in other components.
	Performance Concern: The new Zustand store implementation in `useQuestStore.tsx` introduces more complex state management which could impact performance. Review the usage of Zustand and ensure it's optimized for frequent updates and re-renders.

Category	Suggestion	Score
Possible bug	Ensure `entityId` is always defined before conversion to `BigInt` to prevent runtime errors ___ To avoid potential type coercion errors with the `BigInt` constructor, ensure that `entityId` is always defined before converting it to `BigInt`. This can be done by providing a fallback value directly in the `useMemo` where `entityId` is defined. [client/src/hooks/store/useQuestStore.tsx [73]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR73-R73) ```diff -const entityId = playerRealms()[0]?.entity_id; +const entityId = playerRealms()[0]?.entity_id \|\| "0"; ``` Suggestion importance[1-10]: 10 Why: Ensuring `entityId` is always defined before conversion to `BigInt` prevents potential runtime errors, which is a significant improvement for code robustness and reliability.	10
Possible bug	Improve the safety of accessing properties on potentially undefined objects ___ Consider checking if `selectedEntityId` is defined before attempting to access its `id` property to avoid potential runtime errors. You can use optional chaining (`?.`) for safer access. [client/src/ui/modules/navigation/BottomNavigation.tsx [46]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-e88ab2bf26cead1655ae59538674a03ae7a573c4c6ea347a8ad382f197b3cc4dR46-R46) ```diff -const army = getArmyByEntityId(selectedEntityId?.id \|\| BigInt("0")); +const army = getArmyByEntityId(selectedEntityId?.id ?? BigInt("0")); ``` Suggestion importance[1-10]: 8 Why: The suggestion improves the safety of accessing properties on potentially undefined objects, which can prevent runtime errors.	8
Performance	Add a `key` prop to the parent in the map function for better performance ___ Add a key to the parent element in the map function to ensure that React can efficiently update and render elements. This is particularly important in lists to prevent rendering issues and improve performance. [client/src/ui/components/hints/HintBox.tsx [70-71]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR70-R71) ```diff quest.steps.map(({ description, completed }, index) => ( - + ``` Suggestion importance[1-10]: 9 Why: Adding a `key` prop to the parent ` ` in the map function is important for React to efficiently update and render elements, preventing rendering issues and improving performance.	9
	Improve the definition of `isOffscreen` function to avoid unnecessary redefinitions ___ To ensure that the `isOffscreen` function is not redefined every time the component re-renders, it should be defined outside of the component or wrapped in a `useCallback` hook if it depends on props or state. [client/src/ui/modules/navigation/RightNavigationModule.tsx [252-254]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR252-R254) ```diff -const isOffscreen = (view: View) => { +const isOffscreen = useCallback((view: View) => { return view === View.None; -}; +}, []); ``` Suggestion importance[1-10]: 9 Why: Wrapping the `isOffscreen` function in a `useCallback` hook improves performance by preventing unnecessary redefinitions on each render, which is crucial for maintaining efficient rendering in React components.	9
	Move `armyHasTroops` outside of `useQuests` to prevent unnecessary re-creations ___ The function `armyHasTroops` could be moved outside of the component to prevent it from being recreated on every render. This would improve performance, especially if `useQuests` is used frequently across re-renders. [client/src/hooks/store/useQuestStore.tsx [167-175]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR167-R175) ```diff const armyHasTroops = (entityArmies: ArmyInfo[]) => {...}; +export const useQuests = () => {...}; ``` Suggestion importance[1-10]: 8 Why: Moving `armyHasTroops` outside of `useQuests` prevents unnecessary re-creations of the function, which can improve performance. This is a good practice for optimizing frequently used hooks.	8
	Improve the stability and efficiency of useMemo by using a stable hash of the account object ___ Consider using a more robust key extraction method for `useMemo` to avoid unnecessary recalculations. The current dependencies list includes `account.address` which might not be stable across re-renders. Using a stable hash of the `account` object or specific stable properties might be more efficient. [client/src/hooks/store/useQuestStore.tsx [150]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR150-R150) ```diff -useMemo(() => {...}, [farms, resource, orders, entityArmies, HasClaimedStartingResources, entityId, account.address]); +useMemo(() => {...}, [farms, resource, orders, entityArmies, HasClaimedStartingResources, entityId, JSON.stringify(account)]); ``` Suggestion importance[1-10]: 7 Why: This suggestion improves the stability and efficiency of the `useMemo` hook by using a stable hash of the account object, which can prevent unnecessary recalculations. However, the improvement is minor and does not address a critical issue.	7
	Optimize performance by avoiding repeated BigInt instantiation ___ Avoid using `BigInt("0")` as a fallback value directly inside the component render. Instead, define a constant outside of the component to prevent creating a new `BigInt` on every render. [client/src/ui/modules/navigation/BottomNavigation.tsx [46]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-e88ab2bf26cead1655ae59538674a03ae7a573c4c6ea347a8ad382f197b3cc4dR46-R46) ```diff -const army = getArmyByEntityId(selectedEntityId?.id \|\| BigInt("0")); +const DEFAULT_ENTITY_ID = BigInt("0"); +const army = getArmyByEntityId(selectedEntityId?.id \|\| DEFAULT_ENTITY_ID); ``` Suggestion importance[1-10]: 7 Why: The suggestion optimizes performance by avoiding repeated BigInt instantiation, which is a good practice for improving efficiency.	7
	Simplify the initialization of `activeSection` state for efficiency ___ Initialize the `activeSection` state directly with the `initialActiveSection` prop if it exists, otherwise default to the first section. This simplifies the code by removing the need for the `find` operation on the sections array, which can be inefficient if the array is large. [client/src/ui/components/hints/HintModal.tsx [65-67]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-1dd2daf32164d13d632c2df30c82cc13776cd68a555f9775678593b6abe75783R65-R67) ```diff -const [activeSection, setActiveSection] = useState( - sections.find((section) => section.name === initialActiveSection) \|\| sections[0], -); +const [activeSection, setActiveSection] = useState(initialActiveSection \|\| sections[0].name); ``` Suggestion importance[1-10]: 6 Why: Simplifying the initialization of `activeSection` state can improve code readability and slightly enhance performance. However, the performance gain is minimal and the current code is not incorrect.	6
Best practice	Add missing dependencies to useEffect to ensure it updates correctly ___ Refactor the `useEffect` hook at line 152 to include `quests` in its dependencies array. This ensures that the effect correctly responds to changes in `quests`, aligning with React's best practices for effects that depend on props or state. [client/src/hooks/store/useQuestStore.tsx [152-154]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-a7c36ca377844946d158e339e652e7588b89111d70276c01cf716ce0dea0c9daR152-R154) ```diff useEffect(() => { setQuests(quests); -}, []); +}, [quests]); ``` Suggestion importance[1-10]: 9 Why: Adding `quests` to the dependencies array of the `useEffect` hook ensures that the effect updates correctly when `quests` change, aligning with React's best practices. This is crucial for the correct functioning of the component.	9
	Ensure useMemo dependencies are correctly specified for accurate updates ___ The `useMemo` hook for `isWorldView` is missing its dependencies. Include `location` in the dependency array to ensure it updates correctly when `location` changes. [client/src/ui/modules/navigation/BottomNavigation.tsx [42]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-e88ab2bf26cead1655ae59538674a03ae7a573c4c6ea347a8ad382f197b3cc4dR42-R42) ```diff +const isWorldView = useMemo(() => location === "/map", [location]); - ``` Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a missing dependency in the `useMemo` hook, ensuring that the memoized value updates accurately when `location` changes.	9
	Use type assertion instead of `Number` for type safety in `findResourceById` ___ Avoid using `Number` for type casting directly within the `findResourceById` function call. Instead, ensure that `resourceId` is the correct type earlier in the code or use TypeScript's type assertion more safely. [client/src/ui/components/hints/Resources.tsx [18]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-d19afd508f5ca24a4c6b7aa3350cf9d813e38ea9f6dcf8a76d2fda9e3ae08d8dR18-R18) ```diff -resource: findResourceById(Number(resourceId)), +resource: findResourceById(resourceId as number), ``` Suggestion importance[1-10]: 8 Why: Using type assertion instead of `Number` for type casting improves type safety and aligns with TypeScript best practices, reducing the risk of runtime errors.	8
	Replace array indexes with unique identifiers as keys in React list items ___ It is recommended to avoid using indexes as keys in React lists if the order of items may change, as it could lead to performance issues and bugs. Use a unique identifier if available. [client/src/ui/modules/navigation/RightNavigationModule.tsx [178-180]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR178-R180) ```diff -{navigation.map((a, index) => ( - {a.button} +{navigation.map((navItem) => ( + {navItem.button} ))} ``` Suggestion importance[1-10]: 8 Why: Using unique identifiers instead of array indexes as keys in React list items is a best practice that helps prevent potential performance issues and bugs related to reordering of items.	8
	Improve type safety by using a more specific type for the `entityId` prop ___ Consider using a more specific type for the `entityId` prop in the `HintBox` component to ensure type safety. Currently, it is typed as `bigint`, which is very generic. If `entityId` is expected to be a specific kind of identifier, using a more specific type or creating a custom type could help catch errors during development. [client/src/ui/components/hints/HintBox.tsx [9]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR9-R9) ```diff -export const HintBox = ({ quest, entityId }: { quest: Quest; entityId: bigint }) => { +export const HintBox = ({ quest, entityId }: { quest: Quest; entityId: EntityIdType }) => { ``` Suggestion importance[1-10]: 7 Why: Using a more specific type for `entityId` can improve type safety and catch errors during development. However, the suggestion is not crucial and does not address a major bug or issue.	7
Maintainability	Use more descriptive variable names to enhance code readability ___ Consider using a more descriptive variable name than `a` for the map function in the `navigation` array to improve code readability. [client/src/ui/modules/navigation/RightNavigationModule.tsx [178-180]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR178-R180) ```diff -{navigation.map((a, index) => ( - {a.button} +{navigation.map((navItem, index) => ( + {navItem.button} ))} ``` Suggestion importance[1-10]: 7 Why: Using a more descriptive variable name enhances code readability and maintainability, making it easier for other developers to understand the code.	7
Enhancement	Simplify class application by directly using conditional expressions instead of `clsx` when unnecessary ___ The `clsx` function is used to conditionally apply classes, but it's being used without conditional classes in some instances. You can directly pass the class names to the `className` prop when no conditions are applied. [client/src/ui/modules/navigation/RightNavigationModule.tsx [76-89]](https://github.com/BibliothecaDAO/eternum/pull/975/files#diff-8b98807e05dc7525757f9d43f5a3b1a081305f54aa3be0d5be4b2a130160bd0cR76-R89) ```diff { setLastView(View.ResourceTable); setView(View.ResourceTable); }} /> ``` Suggestion importance[1-10]: 6 Why: Simplifying the class application by directly using conditional expressions instead of `clsx` when unnecessary can make the code more straightforward and slightly improve performance.	6

BibliothecaDAO / eternum

Enh/walkthrough #975

User description

PR Type

Description

Changes walkthrough 📝

PR Reviewer Guide 🔍

PR Code Suggestions ✨