Refactor: extract components from monolithic App.tsx, fix 60fps re-render bug#545
Refactor: extract components from monolithic App.tsx, fix 60fps re-render bug#545
Conversation
…ings - Extract ParticleExplosion, TargetSphere, BackgroundParticles, GameScene, HUD, GameControls, GameInstructions, PauseOverlay, GameOverOverlay into src/components/ with barrel export - Fix performance bug: replace setPulseScale useState in useFrame with ref (was causing 60 React re-renders/sec) - Use delta time for TargetSphere rotation animations (frame-rate independent) - Fix all lint warnings: add return types to useAudioManager callbacks - Clean up duplicated CSS declarations in App.css - Add JSDoc documentation to all extracted components Agent-Logs-Url: https://github.com/Hack23/game/sessions/f008753c-31f7-4146-bbda-e6b53fa43687 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
…Scene, BackgroundParticles) - HUD: 10 tests covering timer, score, level, accuracy, combo, high score - GameOverlay: 17 tests for GameControls, GameInstructions, PauseOverlay, GameOverOverlay - GameScene: 6 tests for scene rendering with mocked Three.js - BackgroundParticles: 1 test for sparkle configuration - Fix lint errors in test files - Total: 212 tests passing, 83.82% statement coverage Agent-Logs-Url: https://github.com/Hack23/game/sessions/f008753c-31f7-4146-bbda-e6b53fa43687 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Hack23/game/sessions/f008753c-31f7-4146-bbda-e6b53fa43687 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the previously monolithic App.tsx into a set of focused UI and scene components, while addressing a 60fps re-render performance issue in the interactive target animation and expanding component-level test coverage.
Changes:
- Extracted Three.js scene, HUD, overlays, and effects into dedicated
src/components/*modules and updatedApp.tsxto orchestrate them. - Refactored target pulse animation to avoid React state updates inside
useFrame(60fps re-render issue) and moved effects into component-local logic. - Updated Vitest configuration and added/expanded component tests for extracted UI/scene pieces.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Removes Pixi-related config and focuses dependency optimization on Three/Fiber/Drei. |
| src/hooks/useAudioManager.ts | Tightens callback typings (explicit param and return types). |
| src/components/TargetSphere.tsx | New extracted interactive target component with ref-based animation updates. |
| src/components/ParticleExplosion.tsx | New extracted particle explosion effect component driven by useFrame. |
| src/components/index.ts | Barrel export for extracted components/constants. |
| src/components/HUD.tsx | New extracted HUD overlay with derived accuracy. |
| src/components/HUD.test.tsx | Adds HUD rendering/behavior tests. |
| src/components/GameScene.tsx | New extracted scene graph (lighting, targets, shake, controls, effects). |
| src/components/GameScene.test.tsx | Adds mocked rendering tests for scene composition. |
| src/components/GameOverlay.tsx | New extracted controls/instructions/pause/game-over overlays. |
| src/components/GameOverlay.test.tsx | Adds tests for overlay rendering and control callbacks. |
| src/components/BackgroundParticles.tsx | New extracted ambient sparkles component. |
| src/components/BackgroundParticles.test.tsx | Adds configuration test for background particles. |
| src/App.tsx | Slimmed root component wiring together state, audio, overlays, and scene. |
| src/App.css | Deduplicates and normalizes fullscreen/app layout styling. |
| if (!isActive || !meshRef.current) return; | ||
|
|
||
| meshRef.current.rotation.y += delta * 1.2; | ||
| meshRef.current.rotation.x += delta * 0.6; | ||
|
|
||
| // Animate rings | ||
| if (outerRingRef.current) { | ||
| outerRingRef.current.rotation.z += delta * 0.9; | ||
| } | ||
| if (middleRingRef.current) { | ||
| middleRingRef.current.rotation.z -= delta * 1.2; | ||
| } | ||
|
|
||
| // Pulsing effect for hover state - use ref to avoid re-renders | ||
| if (hovered) { | ||
| pulseScaleRef.current = 1 + Math.sin(state.clock.elapsedTime * 5) * 0.1; |
There was a problem hiding this comment.
useFrame returns early when isActive is false, so the inactive scale branch (: 0.6) is never applied. If the game transitions from active → paused/game-over while the target remains mounted, the mesh can keep its last active scale/rotation state. Consider only gating the rotation/pulse logic on isActive, but always apply the target scale when meshRef.current exists (or set the inactive scale in an effect when isActive changes).
| if (!isActive || !meshRef.current) return; | |
| meshRef.current.rotation.y += delta * 1.2; | |
| meshRef.current.rotation.x += delta * 0.6; | |
| // Animate rings | |
| if (outerRingRef.current) { | |
| outerRingRef.current.rotation.z += delta * 0.9; | |
| } | |
| if (middleRingRef.current) { | |
| middleRingRef.current.rotation.z -= delta * 1.2; | |
| } | |
| // Pulsing effect for hover state - use ref to avoid re-renders | |
| if (hovered) { | |
| pulseScaleRef.current = 1 + Math.sin(state.clock.elapsedTime * 5) * 0.1; | |
| if (!meshRef.current) return; | |
| if (isActive) { | |
| meshRef.current.rotation.y += delta * 1.2; | |
| meshRef.current.rotation.x += delta * 0.6; | |
| // Animate rings | |
| if (outerRingRef.current) { | |
| outerRingRef.current.rotation.z += delta * 0.9; | |
| } | |
| if (middleRingRef.current) { | |
| middleRingRef.current.rotation.z -= delta * 1.2; | |
| } | |
| // Pulsing effect for hover state - use ref to avoid re-renders | |
| if (hovered) { | |
| pulseScaleRef.current = 1 + Math.sin(state.clock.elapsedTime * 5) * 0.1; | |
| } else { | |
| pulseScaleRef.current = 1; | |
| } |
| if (elapsed < duration) { | ||
| const scale = 1 + elapsed * EXPLOSION_SCALE_RATE; | ||
| particlesRef.current.scale.set(scale, scale, scale); | ||
|
|
||
| const geometry = particlesRef.current.geometry; | ||
| const positionAttr = geometry.attributes.position; | ||
|
|
||
| if (positionAttr?.array) { | ||
| const positions = positionAttr.array as Float32Array; | ||
|
|
||
| for (let i = 1; i < positions.length; i += 3) { | ||
| const current = positions[i]; | ||
| if (current !== undefined) { | ||
| positions[i] = current + PARTICLE_RISE_SPEED; | ||
| } | ||
| } | ||
| positionAttr.needsUpdate = true; | ||
| } | ||
|
|
||
| const opacity = 1 - elapsed / duration; | ||
| (particlesRef.current.material as THREE.PointsMaterial).opacity = opacity; | ||
| } else { | ||
| setVisible(false); | ||
| startTimeRef.current = 0; | ||
| } | ||
| }); | ||
|
|
||
| if (!visible && !active) return null; | ||
|
|
There was a problem hiding this comment.
When the explosion duration ends, the code sets visible to false and resets startTimeRef to 0, but the component still renders as long as active is true (if (!visible && !active) return null). Because active stays true until App's timeout fires, this can cause the explosion to restart/replay for a few frames (or indefinitely if active remains true). Consider rendering null whenever visible is false, and/or stop the useFrame update loop when visible is false, and avoid resetting the start time in a way that re-triggers the animation while active is still true.
| // Mock calculateAccuracy to control output | ||
| vi.mock("../utils/gameConfig", async () => { | ||
| const actual = await vi.importActual<typeof import("../utils/gameConfig")>("../utils/gameConfig"); | ||
| return { | ||
| ...actual, | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
The test comment says "Mock calculateAccuracy to control output", but the mock currently just re-exports the real module without overriding anything. This adds noise and can mask accidental reliance on module side effects. Either remove this mock entirely or explicitly stub calculateAccuracy to the desired value(s).
| // Mock calculateAccuracy to control output | |
| vi.mock("../utils/gameConfig", async () => { | |
| const actual = await vi.importActual<typeof import("../utils/gameConfig")>("../utils/gameConfig"); | |
| return { | |
| ...actual, | |
| }; | |
| }); |
| // Expose test API for E2E tests to trigger target clicks. | ||
| // This bypasses Three.js raycasting which doesn't work reliably in headless CI. | ||
| useEffect(() => { | ||
| const handleTestTargetClick = () => { | ||
| const handleTestTargetClick = (): void => { | ||
| if (gameState.isPlaying && gameState.timeLeft > 0 && gameState.targets.length > 0) { | ||
| handleTargetClick(gameState.targets[0]?.id ?? 0); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('test:targetClick', handleTestTargetClick); | ||
| return () => window.removeEventListener('test:targetClick', handleTestTargetClick); | ||
| window.addEventListener("test:targetClick", handleTestTargetClick); | ||
| return (): void => window.removeEventListener("test:targetClick", handleTestTargetClick); | ||
| }, [handleTargetClick, gameState.isPlaying, gameState.timeLeft, gameState.targets]); |
There was a problem hiding this comment.
This useEffect re-registers the test:targetClick window event listener whenever handleTargetClick / gameState.targets change. Since targets are updated via requestAnimationFrame in useGameState, this can cause add/removeEventListener to run every frame while playing. Consider registering the listener once (empty deps) and using refs to read the latest gameState / handler inside the event callback.
|
@copilot apply changes based on the comments in this thread |
… event listener 1. TargetSphere: always apply mesh scale even when inactive so the 0.6 scale branch is correctly reached on active→inactive transitions 2. ParticleExplosion: use ref instead of useState for visibility, render null immediately when animation completes to prevent replay while parent's active prop timeout hasn't fired yet 3. HUD.test.tsx: remove no-op vi.mock that re-exported the real module 4. App.tsx: register test:targetClick listener once with empty deps, use refs for latest gameState/handler to avoid re-registration on every frame when targets change Agent-Logs-Url: https://github.com/Hack23/game/sessions/c83b73ac-454e-42e5-a6db-a7b8192c55d2 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all 4 review comments in
|
setPulseScalestate update insideuseFrame(causes 60 re-renders/sec), use ref instead