feat: remove animation canvas entirely | move to animation package #1457
Conversation
shanimal08
force-pushed
the
test/ts_ani
branch
from
9e402cd to
83f7242
Compare
shanimal08
changed the title
shanimal08
requested review from
mmrrnn,
stringhandler,
brianp,
peps,
MCozhusheck,
Misieq01 and
leet4tari
nestedblocks.movWhen i turned visual mode off, then on this happened. It doesn't correct itself. On the next floor the glitched level moves up with everything else. When I turned it off and on again, it broke entirely and the tower never returned. Universe log shows nothing noteworthy. |
how did you even manage this @brianp 😭 turned it off during mining right? did you switch back super quick after, or after some time? |
|
I didn't switch it back super quick, as I was watching resources after I turned it off. First on/off. Broken floor. I have since won blocks. 5th on/off the tower returned, but it's in an absolutely abysmal state. I won't take a video. It's shocking and scary. |
|
There's also a memory leak in here somewhere. I can very quickly go from the normal 800mb on the front end to 2GB by flicking visual mode on and off. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/store/useAppConfigStore.ts (2)
323-340:⚠️ Potential issueImprove timeout handling in
setVisualMode.The current implementation has several issues:
- Fixed timeout of 3500ms might not be sufficient for all network conditions.
- No cleanup on component unmount.
- No retry mechanism for failed toggles.
The previous review already suggested adding a timeout and retry mechanism. Please refer to that comment for the implementation details.
1-1: 🛠️ Refactor suggestionMove timeout variable into component scope.
The global
loadingTimeoutvariable could lead to race conditions if multiple visual mode toggles occur simultaneously. Consider moving this into the component's scope or using a ref.-let loadingTimeout: NodeJS.Timeout | undefined;
🧹 Nitpick comments (1)
index.html (1)
192-195: Consider adding z-index to canvas element.The absolute positioning without a z-index might cause layering issues with other UI elements.
#tower-canvas { position: absolute; pointer-events: auto; + z-index: 1; }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
index.html(1 hunks)package.json(1 hunks)src/App/App.tsx(1 hunks)src/store/useAppConfigStore.ts(4 hunks)src/store/useUIStore.ts(2 hunks)src/theme/GlobalStyle.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/App/App.tsx
- src/store/useUIStore.ts
- src/theme/GlobalStyle.ts
🧰 Additional context used
🧠 Learnings (1)
index.html (1)
Learnt from: shanimal08
PR: tari-project/universe#1457
File: index.html:184-187
Timestamp: 2025-02-12T07:12:44.355Z
Learning: The canvas cleanup and DOM manipulation for the tower animation is handled by the `@tari-project/tari-tower` package, which completely removes the canvas element when visual mode is disabled.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tauri-build
- GitHub Check: cargo checks (fmt, clippy, check)
🔇 Additional comments (1)
index.html (1)
199-200: LGTM!The simplified DOM structure and delegation of canvas management to
@tari-project/tari-towerpackage aligns with the PR objectives.
| if (appConfig.visual_mode && !canvasElement) { | ||
| try { | ||
| await loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset }); | ||
| } catch (e) { | ||
| console.error('Error at loadTowerAnimation:', e); | ||
| useAppConfigStore.setState({ visual_mode: false }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in fetchAppConfig.
The error handling in the visual mode initialization could be more robust:
- No retry mechanism for failed tower animation loading.
- Silent failure might confuse users.
if (appConfig.visual_mode && !canvasElement) {
+ let retryCount = 0;
+ const maxRetries = 3;
+ const retryDelay = 1000;
+
+ const attemptLoad = async () => {
try {
await loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset });
} catch (e) {
console.error('Error at loadTowerAnimation:', e);
- useAppConfigStore.setState({ visual_mode: false });
+ if (retryCount < maxRetries) {
+ retryCount++;
+ console.warn(`Retrying tower animation load (attempt ${retryCount}/${maxRetries})...`);
+ setTimeout(attemptLoad, retryDelay);
+ } else {
+ console.error('Failed to load tower animation after max retries');
+ useAppConfigStore.setState({ visual_mode: false });
+ const appStateStore = useAppStateStore.getState();
+ appStateStore.setError('Failed to initialize visual mode');
+ }
}
}
+ attemptLoad();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (appConfig.visual_mode && !canvasElement) { | |
| try { | |
| await loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset }); | |
| } catch (e) { | |
| console.error('Error at loadTowerAnimation:', e); | |
| useAppConfigStore.setState({ visual_mode: false }); | |
| } | |
| } | |
| if (appConfig.visual_mode && !canvasElement) { | |
| let retryCount = 0; | |
| const maxRetries = 3; | |
| const retryDelay = 1000; | |
| const attemptLoad = async () => { | |
| try { | |
| await loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset }); | |
| } catch (e) { | |
| console.error('Error at loadTowerAnimation:', e); | |
| if (retryCount < maxRetries) { | |
| retryCount++; | |
| console.warn(`Retrying tower animation load (attempt ${retryCount}/${maxRetries})...`); | |
| setTimeout(attemptLoad, retryDelay); | |
| } else { | |
| console.error('Failed to load tower animation after max retries'); | |
| useAppConfigStore.setState({ visual_mode: false }); | |
| const appStateStore = useAppStateStore.getState(); | |
| appStateStore.setError('Failed to initialize visual mode'); | |
| } | |
| } | |
| }; | |
| attemptLoad(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/containers/main/Dashboard/components/VisualMode.tsx (1)
5-17: 💡 Verification agent🧩 Analysis chain
Verify the import paths for required components.
Check that all imported functions from the new
@tari-project/tari-towerpackage are available and properly exported.
🏁 Script executed:
#!/bin/bash # Check the tari-tower package exports echo "Checking the tari-tower package exports:" npm list @tari-project/tari-tower echo "Looking for exported functions in node_modules:" grep -r "export.*loadTowerAnimation\|export.*removeTowerAnimation\|export.*setAnimationState" ./node_modules/@tari-project/tari-tower --include="*.js" --include="*.ts" | head -10Length of output: 539
Actionable: Verify the '@tari-project/tari-tower' dependency installation and import paths
The executed script indicates that the
@tari-project/tari-towerpackage is not found in your dependencies—none of the expected exports (loadTowerAnimation,removeTowerAnimation,setAnimationState) are available. Please ensure that:
- The package is correctly installed (or added to your dependencies) if it is intended to be used.
- The import path is accurate based on the package’s actual location and exported members.
- If the package naming or structure has changed, update the imports accordingly.
♻️ Duplicate comments (1)
src/containers/main/Dashboard/components/VisualMode.tsx (1)
54-60: 🛠️ Refactor suggestionAdd debounce to prevent rapid toggling.
Rapid toggling could exacerbate memory issues and visual glitches reported in PR comments.
+import { debounce } from 'lodash'; + -const handleSwitch = useCallback(() => { +const handleSwitch = useCallback( + debounce(() => { if (visualMode) { handleDisable(); } else { handleEnable(); } - }, [handleDisable, handleEnable, visualMode]); + }, 1000), + [handleDisable, handleEnable, visualMode], +);
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/containers/main/Dashboard/components/VisualMode.tsx(3 hunks)src/store/useUIStore.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/store/useUIStore.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tauri-build
🔇 Additional comments (2)
src/containers/main/Dashboard/components/VisualMode.tsx (2)
28-41: Consider adding a timeout to prevent hangs during disable operation.The disable operation could potentially hang if the animation cleanup takes too long. Adding a timeout would ensure the UI remains responsive.
const handleDisable = useCallback(() => { + const timeoutId = setTimeout(() => { + console.error('Tower animation cleanup timed out'); + setVisualMode(false); + }, 5000); + setVisualMode(false); removeTowerAnimation({ canvasId: TOWER_CANVAS_ID }) .then(() => { // Force garbage collection to clean up WebGL context if (window.gc) { window.gc(); } }) .catch((e) => { console.error('Could not disable visual mode. Error at loadTowerAnimation:', e); setVisualMode(true); }) + .finally(() => { + clearTimeout(timeoutId); + }); }, []);
42-52: Add memory usage monitoring to help debug reported memory leaks.The PR comments mention significant memory usage increases (800MB to 2GB). Adding monitoring could help diagnose this issue.
const handleEnable = useCallback(() => { + const initialMemory = window.performance.memory?.usedJSHeapSize; + loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset }) .then(() => { setVisualMode(true); setAnimationState('showVisual'); + + // Monitor memory usage + const currentMemory = window.performance.memory?.usedJSHeapSize; + if (currentMemory && initialMemory && currentMemory - initialMemory > 500 * 1024 * 1024) { + console.warn('High memory usage detected after enabling visual mode'); + } }) .catch((e) => { console.error('Could not enable visual mode. Error at loadTowerAnimation:', e); setVisualMode(false); }); }, []);
| const handleDisable = useCallback(() => { | ||
| setVisualMode(false); | ||
| removeTowerAnimation({ canvasId: TOWER_CANVAS_ID }) | ||
| .then(() => { | ||
| // Force garbage collection to clean up WebGL context | ||
| if (window.gc) { | ||
| window.gc(); | ||
| } | ||
| }) | ||
| .catch((e) => { | ||
| console.error('Could not disable visual mode. Error at loadTowerAnimation:', e); | ||
| setVisualMode(true); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implement a reset mechanism to address reported visual glitches.
PR comments mention persistent visual glitches when toggling visual mode. A complete reset mechanism may help resolve this.
Also applies to: 42-52
🏁 Script executed:
#!/bin/bash
# Check for any existing error handling or reset mechanisms in the codebase
echo "Checking for existing reset mechanisms in animation handling:"
rg -A 3 "reset.*animation|animation.*reset" --type tsLength of output: 188
Implement a complete reset mechanism for the tower animation cleanup
After verifying the codebase, it appears that there’s no dedicated reset functionality for the tower animation. The current implementation in src/containers/main/Dashboard/components/VisualMode.tsx (lines 28–41 and the corresponding block at 42–52) disables visual mode by removing the tower animation and forcing garbage collection, but it lacks a reset step that fully clears any residual animation state. This omission could allow persistent visual glitches when toggling visual mode.
- Issue: No explicit reset mechanism is implemented in the animation cleanup process.
- Suggestion: In addition to calling
removeTowerAnimationand performing GC, implement a reset routine that clears all animation-related data and state, ensuring any stuck visual elements are fully removed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/store/useAppConfigStore.ts (3)
1-1: 🛠️ Refactor suggestionRemove unused global variable.
The global variable
loadingTimeoutmakes the code less maintainable and could lead to issues ifsetVisualModeis called multiple times. This variable should be localized within the function.-let loadingTimeout: NodeJS.Timeout | undefined;
315-322:⚠️ Potential issueAdd retry mechanism for tower animation loading.
The current implementation lacks a retry mechanism for loading the tower animation, which could be causing the reported issues with visual mode toggling. Users reported white screens and glitches when toggling back to visual mode.
if (appConfig.visual_mode && !canvasElement) { + let retryCount = 0; + const maxRetries = 3; + const retryDelay = 1000; + + const attemptLoad = async () => { try { await loadTowerAnimation({ canvasId: TOWER_CANVAS_ID, offset: sidebarTowerOffset }); } catch (e) { console.error('Error at loadTowerAnimation:', e); - useAppConfigStore.setState({ visual_mode: false }); + if (retryCount < maxRetries) { + retryCount++; + console.warn(`Retrying tower animation load (attempt ${retryCount}/${maxRetries})...`); + setTimeout(attemptLoad, retryDelay); + } else { + console.error('Failed to load tower animation after max retries'); + useAppConfigStore.setState({ visual_mode: false }); + const appStateStore = useAppStateStore.getState(); + appStateStore.setError('Failed to initialize visual mode'); + } } } + attemptLoad(); }
328-345:⚠️ Potential issueAdd timeout and resource cleanup for visual mode toggle.
The current implementation has several issues:
- Uses a global variable for timeout management
- Has a hardcoded delay that may not be sufficient
- Lacks a retry mechanism for visual mode toggle
- Doesn't properly clean up animation resources
These issues likely contribute to the reported memory leak (800MB to 2GB) and visual glitches when toggling modes.
+const VISUAL_MODE_TOGGLE_TIMEOUT_MS = 5000; +const MAX_RETRY_ATTEMPTS = 3; + export const setVisualMode = (enabled: boolean) => { + let timeoutId: NodeJS.Timeout; + let retryCount = 0; + + const cleanup = () => { + clearTimeout(timeoutId); + useAppConfigStore.setState({ visualModeToggleLoading: false }); + }; + + const attemptToggle = () => { + timeoutId = setTimeout(() => { + if (retryCount < MAX_RETRY_ATTEMPTS) { + console.warn(`Visual mode toggle attempt ${retryCount + 1} timed out, retrying...`); + retryCount++; + attemptToggle(); + } else { + console.error('Visual mode toggle failed after max retries'); + useAppConfigStore.setState({ visual_mode: !enabled }); + cleanup(); + } + }, VISUAL_MODE_TOGGLE_TIMEOUT_MS); + + invoke('set_visual_mode', { enabled }) + .then(() => { + clearTimeout(timeoutId); + cleanup(); + }) + .catch((e) => { + const appStateStore = useAppStateStore.getState(); + console.error('Could not set visual mode', e); + appStateStore.setError('Could not change visual mode'); + useAppConfigStore.setState({ visual_mode: !enabled }); + cleanup(); + }); + }; + useAppConfigStore.setState({ visual_mode: enabled, visualModeToggleLoading: true }); - invoke('set_visual_mode', { enabled }) - .catch((e) => { - const appStateStore = useAppStateStore.getState(); - console.error('Could not set visual mode', e); - appStateStore.setError('Could not change visual mode'); - useAppConfigStore.setState({ visual_mode: !enabled }); - }) - .finally(() => { - if (loadingTimeout) { - clearTimeout(loadingTimeout); - } - loadingTimeout = setTimeout(() => { - useAppConfigStore.setState({ visualModeToggleLoading: false }); - }, 3500); - }); + attemptToggle(); };
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/store/useAppConfigStore.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tauri-build
- GitHub Check: cargo checks (fmt, clippy, check)
🔇 Additional comments (1)
src/store/useAppConfigStore.ts (1)
16-18: Good addition of loading state tracking.Adding
visualModeToggleLoadingto the state is a good approach for handling the UI feedback during visual mode transitions.
| import { useMiningMetricsStore } from '@app/store/useMiningMetricsStore.ts'; | ||
| import { pauseMining, startMining, stopMining } from '@app/store/miningStoreActions.ts'; | ||
|
|
||
| type State = Partial<AppConfig>; | ||
| import { loadTowerAnimation } from '@tari-project/tari-tower'; |
There was a problem hiding this comment.
❓ Verification inconclusive
Ensure proper cleanup of tower animation resources.
To prevent the reported memory leaks (increasing from 800MB to 2GB), ensure loadTowerAnimation returns a cleanup function that is properly called when visual mode is toggled off.
Run this script to check if the loadTowerAnimation function returns a cleanup function:
🏁 Script executed:
#!/bin/bash
# Check if loadTowerAnimation returns a cleanup function
# Check if there's any code that handles cleanup returned from loadTowerAnimation
rg -A 2 -B 2 "loadTowerAnimation.*\).*cleanup|cleanup.*loadTowerAnimation"
# Look at the tower package to understand its API
rg "loadTowerAnimation|removeTowerAnimation" node_modules/@tari-project/tari-towerLength of output: 239
Manual Verification Required: Confirm Cleanup Implementation for loadTowerAnimation
The script results did not indicate that a cleanup function is returned or used when toggling visual mode off. Additionally, the error regarding the missing node_modules entry for @tari-project/tari-tower suggests that the package’s contents were not available for inspection. Please manually verify the following:
- Confirm that the
@tari-project/tari-towerdependency is correctly installed and its API (specifically, the behavior ofloadTowerAnimation) matches expectations. - Check that
loadTowerAnimationindeed returns a cleanup function and that this return value is utilized appropriately in your code—specifically when visual mode is toggled off. - Ensure that all necessary cleanup calls are in place to prevent memory leaks.
Description --- in #1457 i wanted the `isAppReady` & `isShutting` values for styling changes, so just called the hooks again, but this caused them to be mounted twice. the shut down hook includes the adds a `preventDefault` on `closeRequested`, _then_ calls our `exit_application` command so things froze up with it happening twice (including the state var having already been set to true) - just removed the second references to the hooks in `CurrentAppSection` (the original place they were used) and passed down the values as props Motivation and Context --- - resolves #1595 How Has This Been Tested? --- locally: https://github.com/user-attachments/assets/bf1d0193-ad0b-4716-b9f6-c1b60a5b10ad What process can a PR reviewer use to test or verify this change? --- - same steps as ticket but it actually shuts down <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated the application's state handling so that operational statuses (e.g., readiness and shutdown signals) are now provided by parent components instead of being determined internally. This change streamlines control flow, ensuring components receive accurate status updates. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
tari-towerrepo@tari-project/tari-towerreposetAnimationStateandsetAnimationProperties@tari-project/tari-towerpackageuseMiningUiStateMachine.ts)Motivation and Context
How Has This Been Tested?
GPU.mining.on.on.off.diff.mov
GPU.mining.off.on.off.diff.mov
animation.still.behaving.normally.mov
What process can a PR reviewer use to test or verify this change?
Breaking Changes
yes, after this is merged you will need to install@tari-labs/tari-towerfor local developmentnpm ciafter it's merged :3Summary by CodeRabbit
New Features
Refactor
Style
Bug Fixes
Chores