Fixes quest claim rewards loading state

cwastche commented 3 months ago

User description

Fixes quest claim rewards loading state
Reverted close quest popup on claim, now the popup only closes when the last quest is claimed Closes #899

PR Type

Bug fix, Enhancement

Description

Fixed the quest claim rewards loading state by adding a useEffect to reset the loading state when the quest is not completed.
Removed the togglePopup call from the finally block in handleClaimResources to prevent premature popup closure.
Added logic to close the quest popup only when the last quest is claimed.
Updated the import alias for quests to QuestOSWindow for better clarity.

Changes walkthrough 📝

Relevant files

Bug fix

HintBox.tsx

Fix quest claim loading state and popup behavior

client/src/ui/components/hints/HintBox.tsx

Added useEffect to reset loading state when quest is not completed.

Removed togglePopup call from finally block in handleClaimResources.

Introduced logic to close popup only when the last quest is claimed.

Updated import alias for quests to QuestOSWindow.

+18/-6

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

vercel[bot] commented 3 months 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 13, 2024 11:41am

github-actions[bot] commented 3 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The `useEffect` hook in `HintBox` component checks if `quest.completed` is false to reset `isLoading`. However, there might be scenarios where the quest completion status changes to true, and `isLoading` should be reset as well. Consider handling the case when the quest gets completed.
⚡ Key issues to review	Code Clarity: The renaming of the import from `quests` to `QuestOSWindow` might confuse future maintainers if not documented properly outside of the PR. Ensure that the context of this change is clear in the codebase or documentation.

github-actions[bot] commented 3 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Ensure `setIsLoading(false)` is called even when exceptions occur by using a `finally` block ___ The removal of the `finally` block in the `catch` section of `handleClaimResources` function removes the guarantee that `setIsLoading(false)` will be called if an exception occurs. Consider adding error handling logic to ensure that the loading state is correctly managed even when an error occurs. [client/src/ui/components/hints/HintBox.tsx [73-74]](https://github.com/BibliothecaDAO/eternum/pull/922/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR73-R74) ```diff } catch (error) { console.error("Failed to claim resources:", error); +} finally { setIsLoading(false); } ``` Suggestion importance[1-10]: 9 Why: Ensuring `setIsLoading(false)` is called even when exceptions occur is crucial for maintaining the correct loading state, making this a high-priority suggestion.	9
Possible issue	Refine the logic inside useEffect to prevent unnecessary popup toggling ___ The `useEffect` hook used to toggle the popup based on `firstUnclaimedQuest` and `isPopupOpen` might cause unintended side effects if not handled carefully. Ensure that the conditions inside the `useEffect` are precise to avoid unnecessary toggling of the popup, which could lead to performance issues or user interface inconsistencies. [client/src/ui/components/hints/HintBox.tsx [110-114]](https://github.com/BibliothecaDAO/eternum/pull/922/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR110-R114) ```diff useEffect(() => { - if (firstUnclaimedQuest === undefined && isPopupOpen(QuestOSWindow)) { + const shouldClosePopup = firstUnclaimedQuest === undefined && isPopupOpen(QuestOSWindow); + if (shouldClosePopup) { togglePopup(QuestOSWindow); } -}, [firstUnclaimedQuest]); +}, [firstUnclaimedQuest, isPopupOpen, QuestOSWindow]); ``` Suggestion importance[1-10]: 8 Why: Refining the logic inside the `useEffect` hook is important to prevent unnecessary toggling, which can improve performance and user experience.	8
Maintainability	Improve the clarity and consistency of the import alias for better code readability ___ Consider renaming the import alias for `quests` from `QuestOSWindow` to something more descriptive and consistent with its usage. The current name `QuestOSWindow` might be misleading as it suggests a type of window or UI component rather than a configuration object. [client/src/ui/components/hints/HintBox.tsx [7]](https://github.com/BibliothecaDAO/eternum/pull/922/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR7-R7) ```diff -import { quests as QuestOSWindow } from "../../components/navigation/Config"; +import { quests as questsConfig } from "../../components/navigation/Config"; ``` Suggestion importance[1-10]: 7 Why: The suggestion improves code readability and maintainability by using a more descriptive alias, but it is not crucial for functionality.	7
Performance	Simplify or remove unnecessary useEffect to enhance performance and code cleanliness ___ The `useEffect` hook that sets `isLoading` to false when `quest.completed` is false could be simplified or removed if it only sets `isLoading` to a value it already holds. If the intention is to ensure `isLoading` is false when a quest is not completed, consider handling this logic where `quest.completed` changes, or ensure `isLoading` is not set to true unnecessarily elsewhere. [client/src/ui/components/hints/HintBox.tsx [37-41]](https://github.com/BibliothecaDAO/eternum/pull/922/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR37-R41) ```diff -useEffect(() => { - if (quest.completed === false) { - setIsLoading(false); - } -}, [quest]); +// Consider removing or refactoring this useEffect if redundant ``` Suggestion importance[1-10]: 6 Why: Simplifying or removing the `useEffect` hook can enhance performance and code cleanliness, but it requires careful consideration to avoid breaking existing functionality.	6

BibliothecaDAO / eternum