Fix army

[aymericdelab]

User description

-> avoid unnecessary rerenders of armies on world map -> prevent from selecting army if moving

---

PR Type

Bug fix, Enhancement

---

Description

• Added isAnimating prop to WarriorModel and ArmyHitBox components to prevent interactions during animations.
• Refactored Army component to use isAnimating state and moved selection and combat label logic to a new ArmySelectionOverlay component.
• Updated useArmyAnimation hook to track animation state using isAnimatingRef.

---

Changes walkthrough 📝

Relevant files

Enhancement

WarriorModel.tsx Add `isAnimating` prop to WarriorModel component.                client/src/ui/components/models/armies/WarriorModel.tsx Added isAnimating prop to WarriorModel component. Passed isAnimating prop to ArmyHitBox component. +3/-2      Army.tsx Refactor Army component and add ArmySelectionOverlay.        client/src/ui/components/worldmap/armies/Army.tsx Refactored Army component to use isAnimating state. Moved selection and combat label logic to new ArmySelectionOverlay component. Passed isAnimating prop to WarriorModel component. +42/-28  ArmyHitBox.tsx Add `isAnimating` prop to ArmyHitBox component.                    client/src/ui/components/worldmap/armies/ArmyHitBox.tsx Added isAnimating prop to ArmyHitBox component. Updated hover and click handlers to respect isAnimating state. +20/-13  useArmyAnimation.tsx Track animation state with isAnimatingRef in useArmyAnimation. client/src/ui/components/worldmap/armies/useArmyAnimation.tsx Added isAnimatingRef to track animation state. Updated animation logic to set isAnimatingRef appropriately. +11/-9

---

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

[vercel[bot]]

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 21, 2024 6:10pm

[github-actions[bot]]

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 useArmyAnimation uses position.x and position.y in its dependency array, but it also accesses position directly inside the hook. This could lead to inconsistencies if position is updated in a way that does not change x or y. It would be safer to destructure position at the start of the hook and use only the destructured values throughout.
	Code Duplication: The ArmySelectionOverlay component seems to duplicate some logic related to entity selection and combat label visibility that might be better centralized or abstracted to reduce redundancy and improve maintainability.

[github-actions[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Add a cleanup function in useEffect to reset isAnimatingRef when the component unmounts ___ **It's recommended to use a cleanup function in the useEffect to reset the isAnimatingRef.current to false when the component unmounts. This ensures that the state is correctly reset and prevents potential bugs if the component is re-mounted.** [client/src/ui/components/worldmap/armies/useArmyAnimation.tsx [42]](https://github.com/BibliothecaDAO/eternum/pull/981/files#diff-ce247ef03061209e44162c6a4f2a687d3ad45f5b3843f3f16253cd738cf64335R42-R42) ```diff isAnimatingRef.current = true; +return () => { + isAnimatingRef.current = false; +}; ``` Suggestion importance[1-10]: 9 Why: Adding a cleanup function in the `useEffect` hook is a best practice that ensures the `isAnimatingRef` state is properly reset when the component unmounts, preventing potential bugs. This is a valuable improvement.	9
	Use a more specific event type in callbacks to enhance type safety ___ **Consider using a more specific type than any for the event parameter in the onPointerEnter and onPointerOut callbacks to improve type safety and take advantage of TypeScript's static type checking.** [client/src/ui/components/worldmap/armies/ArmyHitBox.tsx [40-53]](https://github.com/BibliothecaDAO/eternum/pull/981/files#diff-2dd1c84f2d9bb67d297d72c8c18345fa06b591e9f2ab0eae41c55f09407d7a35R40-R53) ```diff const onPointerEnter = useCallback( - (e: any) => { + (e: React.MouseEvent) => { e.stopPropagation(); if (!isAnimating) setHovered(true); }, [isAnimating], ); const onPointerOut = useCallback( - (e: any) => { + (e: React.MouseEvent) => { e.stopPropagation(); if (!isAnimating) setHovered(false); }, [isAnimating], ); ``` Suggestion importance[1-10]: 6 Why: Using a more specific event type improves type safety and leverages TypeScript's static type checking. This is a minor improvement but contributes to better code quality.	6
Possible issue	Add a check for isAnimating before setting the selected entity to prevent selection during animations ___ **Consider adding a check for isAnimating before setting the selected entity in the onRightClick function to ensure that the entity is not selected while it is animating. This is to prevent potential conflicts or UI issues where the entity might be in a transient state.** [client/src/ui/components/worldmap/armies/ArmyHitBox.tsx [34-37]](https://github.com/BibliothecaDAO/eternum/pull/981/files#diff-2dd1c84f2d9bb67d297d72c8c18345fa06b591e9f2ab0eae41c55f09407d7a35R34-R37) ```diff -if ((selectedEntity?.id || 0n) !== BigInt(army.entity_id) && army.isMine && !isAnimating) { +if (!isAnimating && (selectedEntity?.id || 0n) !== BigInt(army.entity_id) && army.isMine) { setSelectedEntity({ id: BigInt(army.entity_id), position: { x: army.x, y: army.y } }); playSelectedArmy(); } ``` Suggestion importance[1-10]: 8 Why: This suggestion correctly identifies a potential issue where an entity could be selected while it is animating, which could lead to UI inconsistencies. The proposed change is valid and improves the robustness of the code.	8
Maintainability	Refactor hover event handlers into a single function to improve code maintainability ___ **Refactor the onPointerEnter and onPointerOut callbacks to a single function to toggle the hovered state based on the event type. This reduces redundancy and improves maintainability.** [client/src/ui/components/worldmap/armies/ArmyHitBox.tsx [40-53]](https://github.com/BibliothecaDAO/eternum/pull/981/files#diff-2dd1c84f2d9bb67d297d72c8c18345fa06b591e9f2ab0eae41c55f09407d7a35R40-R53) ```diff -const onPointerEnter = useCallback( - (e: any) => { +const toggleHover = useCallback( + (e: any, entering: boolean) => { e.stopPropagation(); - if (!isAnimating) setHovered(true); - }, - [isAnimating], -); -const onPointerOut = useCallback( - (e: any) => { - e.stopPropagation(); - if (!isAnimating) setHovered(false); + if (!isAnimating) setHovered(entering); }, [isAnimating], ); ``` Suggestion importance[1-10]: 7 Why: The suggestion to refactor the hover event handlers into a single function reduces redundancy and improves maintainability. While not critical, it is a good practice for cleaner and more maintainable code.	7