Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memo the market #2525

Merged
merged 4 commits into from
Dec 15, 2024
Merged

memo the market #2525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f2e1cc
memo the market
ponderingdemocritus Dec 15, 2024
bdc50a8
🐛 Fix black screen
bal7hazar Dec 15, 2024
a0ad4c6
memo combat
ponderingdemocritus Dec 15, 2024
8b0fbae
Merge branch 'market-memo' of https://github.com/BibliothecaDAO/etern…
ponderingdemocritus Dec 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion client/src/three/scenes/HexagonScene.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,11 @@ export abstract class HexagonScene {
this.updateLights();
this.updateHighlightPulse();
this.biomeModels.forEach((biome) => {
biome.updateAnimations(deltaTime);
try {
biome.updateAnimations(deltaTime);
} catch (error) {
console.error(`Error updating biome animations:`, error);
}
Comment on lines +417 to +421
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling with more context and recovery strategy.

While adding error handling is good, the current implementation could be improved to:

  1. Include more context about which biome failed
  2. Implement a recovery strategy for failed animations

Consider this enhanced implementation:

  this.biomeModels.forEach((biome, biomeType) => {
    try {
      biome.updateAnimations(deltaTime);
    } catch (error) {
-     console.error(`Error updating biome animations:`, error);
+     console.error(`Error updating animations for biome type ${biomeType}:`, error);
+     // Attempt to reset the animation state
+     try {
+       biome.resetAnimations?.();
+     } catch (resetError) {
+       console.error(`Failed to reset animations for biome type ${biomeType}:`, resetError);
+     }
    }
  });

Committable suggestion skipped: line range outside the PR's diff.

💡 Codebase verification

Based on the collected evidence, I can now provide the final response:

Add error handling for animation mixer updates

The current error handling for biome.updateAnimations() is good practice, but there's an unprotected animation update in Hexception.tsx where mixer.update(deltaTime) is called without error handling. To maintain consistency and prevent potential runtime errors, similar error handling should be added there as well.

  • client/src/three/scenes/Hexception.tsx: Add try-catch block around mixer.update(deltaTime) in the update method
🔗 Analysis chain

Verify error handling coverage across animation-related code.

Let's verify if similar error handling is implemented in other animation-related methods.

Let me analyze the findings and generate additional verification scripts to get a clearer picture.

The initial results show multiple animation-related code paths, but we need to specifically check the error handling in animation update methods and mixer updates. These additional scripts will help us verify the coverage of error handling in critical animation-related operations.

Let me try one more search pattern to ensure we have a complete view of animation-related error handling.

The previous results show only one mixer update without error handling, but we need more context to understand the full scope of animation-related updates in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other animation-related methods that might need error handling
echo "Searching for animation-related methods..."
rg -t ts "animation|animate" --no-filename

# Search for existing try-catch blocks in animation code
echo -e "\nChecking existing error handling in animation code..."
ast-grep --pattern 'try {
  $$$
  animation
  $$$
} catch'

Length of output: 16752

Script:

#!/bin/bash
# Search for methods that update animations to check their error handling
ast-grep --pattern 'updateAnimations($$$) {
  $$$
}'

# Search for animation mixers and their error handling
ast-grep --pattern 'mixer.update($$$)'

# Look for other try-catch blocks in animation-related code
rg -A 2 -B 2 "try.*catch.*animation"

Length of output: 208

Script:

#!/bin/bash
# Search for all animation update methods with context
rg -A 5 -B 5 "update.*animation|animation.*update"

# Look for mixer updates with more context
rg -A 5 -B 5 "mixer\.update"

# Search for animation-related class methods
ast-grep --pattern 'class $_ {
  $$$
  update($$$) {
    $$$
    animation
    $$$
  }
  $$$
}'

Length of output: 686

});
}

Expand Down
264 changes: 133 additions & 131 deletions client/src/ui/components/bank/ResourceBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,152 +5,154 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@
import TextInput from "@/ui/elements/TextInput";
import { divideByPrecision, formatNumber } from "@/ui/utils/utils";
import { ID, Resources, ResourcesIds, findResourceById, findResourceIdByTrait } from "@bibliothecadao/eternum";
import { useEffect, useRef, useState } from "react";
import { memo, useEffect, useRef, useState } from "react";
import { HintSection } from "../hints/HintModal";

export const ResourceBar = ({
entityId,
lordsFee,
resources,
resourceId,
setResourceId,
amount,
setAmount,
disableInput = false,
onFocus,
onBlur,
max = Infinity,
}: {
entityId: ID;
lordsFee: number;
resources: Resources[];
resourceId: ResourcesIds;
setResourceId: (resourceId: ResourcesIds) => void;
amount: number;
setAmount: (amount: number) => void;
disableInput?: boolean;
onFocus?: () => void; // New prop
onBlur?: () => void; // New prop
max?: number;
}) => {
const { getBalance } = useResourceBalance();
export const ResourceBar = memo(
({
entityId,
lordsFee,
resources,
resourceId,
setResourceId,
amount,
setAmount,
disableInput = false,
onFocus,
onBlur,
max = Infinity,
}: {
entityId: ID;
lordsFee: number;
resources: Resources[];
resourceId: ResourcesIds;
setResourceId: (resourceId: ResourcesIds) => void;
amount: number;
setAmount: (amount: number) => void;
disableInput?: boolean;
onFocus?: () => void; // New prop
onBlur?: () => void; // New prop
max?: number;
}) => {
Comment on lines +11 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Verify memoization effectiveness

While wrapping the component in memo is a good start for optimization, the component receives multiple callback props (setResourceId, setAmount, onFocus, onBlur) which could negate the benefits of memoization if their references change frequently in the parent component.

Consider wrapping the callback props with useCallback in the parent component to maintain stable references:

// In parent component:
const handleSetResourceId = useCallback((resourceId: ResourcesIds) => {
  setResourceId(resourceId);
}, []);

const handleSetAmount = useCallback((amount: number) => {
  setAmount(amount);
}, []);

const { getBalance } = useResourceBalance();

const [selectedResourceBalance, setSelectedResourceBalance] = useState(0);
const [searchInput, setSearchInput] = useState("");
const [open, setOpen] = useState(false);
const [selectedResourceBalance, setSelectedResourceBalance] = useState(0);
const [searchInput, setSearchInput] = useState("");
const [open, setOpen] = useState(false);

const inputRef = useRef<HTMLInputElement>(null);
const inputRef = useRef<HTMLInputElement>(null);

useEffect(() => {
setSelectedResourceBalance(divideByPrecision(getBalance(entityId, Number(resourceId)).balance));
}, [resourceId, getBalance, entityId]);
useEffect(() => {
setSelectedResourceBalance(divideByPrecision(getBalance(entityId, Number(resourceId)).balance));
}, [resourceId, getBalance, entityId]);

const handleResourceChange = (trait: string) => {
const newResourceId = findResourceIdByTrait(trait);
setResourceId(newResourceId);
};
const handleResourceChange = (trait: string) => {
const newResourceId = findResourceIdByTrait(trait);
setResourceId(newResourceId);
};

const handleAmountChange = (amount: number) => {
setAmount(amount);
};
const handleAmountChange = (amount: number) => {
setAmount(amount);
};

const hasLordsFees = lordsFee > 0 && resourceId === ResourcesIds.Lords;
const finalResourceBalance = hasLordsFees ? selectedResourceBalance - lordsFee : selectedResourceBalance;
const hasLordsFees = lordsFee > 0 && resourceId === ResourcesIds.Lords;
const finalResourceBalance = hasLordsFees ? selectedResourceBalance - lordsFee : selectedResourceBalance;

const filteredResources = resources.filter(
(resource) => resource.trait.toLowerCase().startsWith(searchInput.toLowerCase()) || resource.id === resourceId,
);
const filteredResources = resources.filter(
(resource) => resource.trait.toLowerCase().startsWith(searchInput.toLowerCase()) || resource.id === resourceId,
);

const handleOpenChange = (newOpen: boolean) => {
setOpen(newOpen);
if (newOpen && inputRef.current) {
setResourceId(ResourcesIds.Wood);
setSearchInput("");
setTimeout(() => {
inputRef.current?.focus();
}, 0);
}
};
const handleOpenChange = (newOpen: boolean) => {
setOpen(newOpen);
if (newOpen && inputRef.current) {
setResourceId(ResourcesIds.Wood);
setSearchInput("");
setTimeout(() => {
inputRef.current?.focus();
}, 0);
}
};

const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => {
if (e.key === "Enter") {
if (filteredResources.length > 0) {
const selectedResource = filteredResources.find((resource) => resource.id !== resourceId);
if (selectedResource) {
setResourceId(selectedResource.id);
setOpen(false);
const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => {
if (e.key === "Enter") {
if (filteredResources.length > 0) {
const selectedResource = filteredResources.find((resource) => resource.id !== resourceId);
if (selectedResource) {
setResourceId(selectedResource.id);
setOpen(false);
}
}
setSearchInput("");
} else {
e.stopPropagation();
}
setSearchInput("");
} else {
e.stopPropagation();
}
};
};

return (
<div className="resource-bar-selector w-full bg-gold/10 rounded-xl p-3 flex justify-between h-28 flex-wrap">
<div className="self-center">
<NumberInput
className="text-2xl border-transparent"
value={amount}
onChange={handleAmountChange}
max={max}
arrows={false}
allowDecimals
onFocus={onFocus}
onBlur={onBlur}
/>

return (
<div className="resource-bar-selector w-full bg-gold/10 rounded-xl p-3 flex justify-between h-28 flex-wrap">
<div className="self-center">
<NumberInput
className="text-2xl border-transparent"
value={amount}
onChange={handleAmountChange}
max={max}
arrows={false}
allowDecimals
onFocus={onFocus}
onBlur={onBlur}
/>
{!disableInput && (
<div
className="flex text-xs text-gold/70 mt-1 justify-center items-center relative text-center self-center mx-auto w-full cursor-pointer"
onClick={() => handleAmountChange(finalResourceBalance)}
>
Max: {isNaN(selectedResourceBalance) ? "0" : selectedResourceBalance.toLocaleString()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Replace unsafe isNaN with Number.isNaN

The global isNaN() function performs type coercion which can lead to unexpected results. Use Number.isNaN() instead for more predictable behavior.

Apply this fix:

-              Max: {isNaN(selectedResourceBalance) ? "0" : selectedResourceBalance.toLocaleString()}
+              Max: {Number.isNaN(selectedResourceBalance) ? "0" : selectedResourceBalance.toLocaleString()}
               {hasLordsFees && (
                 <div className="text-danger ml-2">
-                  <div>{`[+${isNaN(lordsFee) ? "0" : formatNumber(lordsFee, 2)}]`}</div>
+                  <div>{`[+${Number.isNaN(lordsFee) ? "0" : formatNumber(lordsFee, 2)}]`}</div>

Also applies to: 113-113

🧰 Tools
🪛 Biome (1.9.4)

[error] 110-110: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

{hasLordsFees && (
<div className="text-danger ml-2">
<div>{`[+${isNaN(lordsFee) ? "0" : formatNumber(lordsFee, 2)}]`}</div>
</div>
)}
</div>
)}
</div>

{!disableInput && (
<div
className="flex text-xs text-gold/70 mt-1 justify-center items-center relative text-center self-center mx-auto w-full cursor-pointer"
onClick={() => handleAmountChange(finalResourceBalance)}
>
Max: {isNaN(selectedResourceBalance) ? "0" : selectedResourceBalance.toLocaleString()}
{hasLordsFees && (
<div className="text-danger ml-2">
<div>{`[+${isNaN(lordsFee) ? "0" : formatNumber(lordsFee, 2)}]`}</div>
<Select
open={open}
onOpenChange={handleOpenChange}
value={findResourceById(Number(resourceId))?.trait || ""}
onValueChange={(trait) => {
handleResourceChange(trait);
setOpen(false);
setSearchInput("");
}}
>
<SelectTrigger className="w-[140px]">
<SelectValue placeholder={HintSection.Resources} />
</SelectTrigger>
<SelectContent>
{resources.length > 1 && (
<div className="p-2">
<TextInput
ref={inputRef}
onChange={setSearchInput}
placeholder="Filter resources..."
onKeyDown={handleKeyDown}
/>
</div>
)}
</div>
)}
{filteredResources.map((resource) => (
<SelectItem key={resource.id} value={resource.trait} disabled={resource.id === resourceId}>
<ResourceCost
resourceId={resource.id}
amount={divideByPrecision(getBalance(entityId, resource.id).balance)}
className="border-0 bg-transparent"
/>
</SelectItem>
))}
</SelectContent>
</Select>
</div>

<Select
open={open}
onOpenChange={handleOpenChange}
value={findResourceById(Number(resourceId))?.trait || ""}
onValueChange={(trait) => {
handleResourceChange(trait);
setOpen(false);
setSearchInput("");
}}
>
<SelectTrigger className="w-[140px]">
<SelectValue placeholder={HintSection.Resources} />
</SelectTrigger>
<SelectContent>
{resources.length > 1 && (
<div className="p-2">
<TextInput
ref={inputRef}
onChange={setSearchInput}
placeholder="Filter resources..."
onKeyDown={handleKeyDown}
/>
</div>
)}
{filteredResources.map((resource) => (
<SelectItem key={resource.id} value={resource.trait} disabled={resource.id === resourceId}>
<ResourceCost
resourceId={resource.id}
amount={divideByPrecision(getBalance(entityId, resource.id).balance)}
className="border-0 bg-transparent"
/>
</SelectItem>
))}
</SelectContent>
</Select>
</div>
);
};
);
},
);
Loading
Loading