Conversation
📝 WalkthroughWalkthroughThe pull request adds four new section components (Performance, Features, Highlights, Footer) to the App, integrating scroll-driven GSAP animations and a responsive masonry layout. Additionally, the Macbook 3D model now supports dynamic video textures and runtime color updates via the store, with scroll-triggered texture changes in the Features section. Changes
Sequence DiagramsequenceDiagram
participant Scroll as User Scroll
participant GSAP as GSAP ScrollTrigger
participant Timeline as Animation Timeline
participant Store as Macbook Store
participant Model as Macbook 3D Model
participant Mesh as Screen Mesh Material
Scroll->>GSAP: Trigger scroll event in Features section
GSAP->>Timeline: Execute timeline based on scroll position
Timeline->>Store: Call setTexture('/videos/feature-N.mp4')
Store->>Model: Update texture state
Model->>Mesh: Apply new video texture via useVideoTexture
Mesh->>Mesh: Render updated screen material on 3D model
Timeline->>DOM: Fade and translate .boxN elements to opacity 1, y: 0
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/components/models/Macbook.jsx (1)
1-6: Remove duplicate React import.
Reactis imported on line 1, anduseEffectis imported separately on line 6. Consolidate into a single import.Proposed fix
-import React from 'react' +import React, { useEffect } from 'react' import { useGLTF, useVideoTexture } from '@react-three/drei' import useMacbookStore from '../../store' import { noChangeParts } from '../../constants/index.js' import { Color } from 'three' -import { useEffect } from 'react'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/models/Macbook.jsx` around lines 1 - 6, The file imports React twice (default import React and a separate named useEffect); consolidate to a single import by combining useEffect into the React import (e.g., import React, { useEffect } from 'react') and remove the standalone useEffect import; update any references relying on React/useEffect as needed in the Macbook component to ensure they still work.src/components/Performance.jsx (1)
49-50: Consider adding a comment explaining whyp5is skipped.The exclusion of
id === 'p5'from the animation timeline isn't self-documenting. A brief comment would help future maintainers understand the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Performance.jsx` around lines 49 - 50, Add an inline comment inside the Performance.jsx loop where performanceImgPositions.forEach((item) => { if (item.id === 'p5') return }) explaining why the element with id 'p5' is intentionally skipped (e.g., reserved for a different animation, static element, layout constraint, or handled elsewhere), referencing the performanceImgPositions loop and the 'p5' id so future maintainers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Features.jsx`:
- Around line 18-30: Fix the typo and add proper cleanup for preloaded videos:
in the useEffect that iterates featureSequence, change the video attribute name
from crossOrigen to crossOrigin and keep references to created elements (the
const v video nodes) in an array; after calling v.load() push each v into that
array and return a cleanup function from useEffect that iterates the array to
pause(), removeAttribute('src'), call load() to clear, and remove each node from
the DOM to avoid leaks. Ensure you update the useEffect closure (featureSequence
and created videos) accordingly so cleanup runs on unmount/remount.
In `@src/components/Footer.jsx`:
- Line 6: In the Footer component fix the typo in the div's className (currently
"'info") by removing the stray single quote so the className is "info"; locate
the div element in Footer.jsx that renders the container (the line with
className="'info") and update it to the correct string to restore styling.
In `@src/components/Highlights.jsx`:
- Around line 8-20: The animation uses isMobile inside the useGSAP callback
(gsap.to([...], { scrollTrigger: { start: isMobile ? 'bottom bottom ' : 'top
top' } })) but useGSAP is invoked without dependencies so the stale isMobile
value persists; update the call so the effect re-runs when isMobile
changes—either pass isMobile in the dependency array to useGSAP (e.g.,
useGSAP(..., [isMobile])) or modify useGSAP to accept and forward a dependencies
array, ensuring the gsap.to/ScrollTrigger config is recalculated when isMobile
toggles.
In `@src/store/index.js`:
- Around line 13-14: The reset setter currently assigns texture
'./videos/feature-1.mp4' which is inconsistent with the initial texture
'/videos/feature-1.mp4' and will break video loading; update the reset()
implementation (the reset arrow function that calls set) to use the same
absolute path '/videos/feature-1.mp4' for the texture key so both the initial
state and the set/reset flow use the identical path.
---
Nitpick comments:
In `@src/components/models/Macbook.jsx`:
- Around line 1-6: The file imports React twice (default import React and a
separate named useEffect); consolidate to a single import by combining useEffect
into the React import (e.g., import React, { useEffect } from 'react') and
remove the standalone useEffect import; update any references relying on
React/useEffect as needed in the Macbook component to ensure they still work.
In `@src/components/Performance.jsx`:
- Around line 49-50: Add an inline comment inside the Performance.jsx loop where
performanceImgPositions.forEach((item) => { if (item.id === 'p5') return })
explaining why the element with id 'p5' is intentionally skipped (e.g., reserved
for a different animation, static element, layout constraint, or handled
elsewhere), referencing the performanceImgPositions loop and the 'p5' id so
future maintainers understand the intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2476b741-51b1-4433-9aed-b15cee43d3da
📒 Files selected for processing (7)
src/App.jsxsrc/components/Features.jsxsrc/components/Footer.jsxsrc/components/Highlights.jsxsrc/components/Performance.jsxsrc/components/models/Macbook.jsxsrc/store/index.js
| useEffect(() => { | ||
| featureSequence.forEach((feature) => { | ||
| const v = document.createElement('video') | ||
| Object.assign(v, { | ||
| src: feature.videoPath, | ||
| muted: true, | ||
| playsInline: true, | ||
| preload: 'auto', | ||
| crossOrigen: 'anonymous' | ||
| }) | ||
| v.load() | ||
| }) | ||
| }, []) |
There was a problem hiding this comment.
Typo crossOrigen and missing cleanup for preloaded videos.
crossOrigenshould becrossOrigin(HTML attribute spelling).- The created video elements are never cleaned up, which could cause memory leaks if the component remounts.
Proposed fix
useEffect(() => {
+ const videos = []
featureSequence.forEach((feature) => {
const v = document.createElement('video')
Object.assign(v, {
src: feature.videoPath,
muted: true,
playsInline: true,
preload: 'auto',
- crossOrigen: 'anonymous'
+ crossOrigin: 'anonymous'
})
v.load()
+ videos.push(v)
})
+ return () => {
+ videos.forEach((v) => {
+ v.src = ''
+ v.load()
+ })
+ }
}, [])📝 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.
| useEffect(() => { | |
| featureSequence.forEach((feature) => { | |
| const v = document.createElement('video') | |
| Object.assign(v, { | |
| src: feature.videoPath, | |
| muted: true, | |
| playsInline: true, | |
| preload: 'auto', | |
| crossOrigen: 'anonymous' | |
| }) | |
| v.load() | |
| }) | |
| }, []) | |
| useEffect(() => { | |
| const videos = [] | |
| featureSequence.forEach((feature) => { | |
| const v = document.createElement('video') | |
| Object.assign(v, { | |
| src: feature.videoPath, | |
| muted: true, | |
| playsInline: true, | |
| preload: 'auto', | |
| crossOrigin: 'anonymous' | |
| }) | |
| v.load() | |
| videos.push(v) | |
| }) | |
| return () => { | |
| videos.forEach((v) => { | |
| v.src = '' | |
| v.load() | |
| }) | |
| } | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Features.jsx` around lines 18 - 30, Fix the typo and add
proper cleanup for preloaded videos: in the useEffect that iterates
featureSequence, change the video attribute name from crossOrigen to crossOrigin
and keep references to created elements (the const v video nodes) in an array;
after calling v.load() push each v into that array and return a cleanup function
from useEffect that iterates the array to pause(), removeAttribute('src'), call
load() to clear, and remove each node from the DOM to avoid leaks. Ensure you
update the useEffect closure (featureSequence and created videos) accordingly so
cleanup runs on unmount/remount.
| const Footer = () => { | ||
| return ( | ||
| <footer> | ||
| <div className="'info"> |
There was a problem hiding this comment.
Typo in className breaks styling.
The className has an extra single quote: "'info" should be "info".
Proposed fix
- <div className="'info">
+ <div className="info">📝 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.
| <div className="'info"> | |
| <div className="info"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Footer.jsx` at line 6, In the Footer component fix the typo in
the div's className (currently "'info") by removing the stray single quote so
the className is "info"; locate the div element in Footer.jsx that renders the
container (the line with className="'info") and update it to the correct string
to restore styling.
| useGSAP(() => { | ||
| gsap.to(['.left-column', '.right-column'], { | ||
| scrollTrigger: { | ||
| trigger: '#highlights', | ||
| start: isMobile ? 'bottom bottom ' : 'top top' | ||
| }, | ||
| y: 0, | ||
| opacity: 1, | ||
| stagger: 0.5, | ||
| duration: 1, | ||
| ease: 'power1.inOut' | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Missing dependency array causes stale isMobile value.
The useGSAP hook uses isMobile in the ScrollTrigger config but doesn't include it in a dependency array. If the viewport changes, the animation won't update to reflect the new start position.
Proposed fix
useGSAP(() => {
gsap.to(['.left-column', '.right-column'], {
scrollTrigger: {
trigger: '#highlights',
- start: isMobile ? 'bottom bottom ' : 'top top'
+ start: isMobile ? 'bottom bottom' : 'top top'
},
y: 0,
opacity: 1,
stagger: 0.5,
duration: 1,
ease: 'power1.inOut'
})
- })
+ }, { dependencies: [isMobile] })📝 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.
| useGSAP(() => { | |
| gsap.to(['.left-column', '.right-column'], { | |
| scrollTrigger: { | |
| trigger: '#highlights', | |
| start: isMobile ? 'bottom bottom ' : 'top top' | |
| }, | |
| y: 0, | |
| opacity: 1, | |
| stagger: 0.5, | |
| duration: 1, | |
| ease: 'power1.inOut' | |
| }) | |
| }) | |
| useGSAP(() => { | |
| gsap.to(['.left-column', '.right-column'], { | |
| scrollTrigger: { | |
| trigger: '#highlights', | |
| start: isMobile ? 'bottom bottom' : 'top top' | |
| }, | |
| y: 0, | |
| opacity: 1, | |
| stagger: 0.5, | |
| duration: 1, | |
| ease: 'power1.inOut' | |
| }) | |
| }, { dependencies: [isMobile] }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Highlights.jsx` around lines 8 - 20, The animation uses
isMobile inside the useGSAP callback (gsap.to([...], { scrollTrigger: { start:
isMobile ? 'bottom bottom ' : 'top top' } })) but useGSAP is invoked without
dependencies so the stale isMobile value persists; update the call so the effect
re-runs when isMobile changes—either pass isMobile in the dependency array to
useGSAP (e.g., useGSAP(..., [isMobile])) or modify useGSAP to accept and forward
a dependencies array, ensuring the gsap.to/ScrollTrigger config is recalculated
when isMobile toggles.
| export default function MacbookModel(props) { | ||
| const { nodes, materials } = useGLTF('/models/macbook-transformed.glb') | ||
| return ( | ||
| <group {...props} dispose={null}> | ||
| <mesh geometry={nodes.Object_10.geometry} material={materials.PaletteMaterial001} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_16.geometry} material={materials.zhGRTuGrQoJflBD} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_20.geometry} material={materials.PaletteMaterial002} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_22.geometry} material={materials.lmWQsEjxpsebDlK} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_30.geometry} material={materials.LtEafgAVRolQqRw} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_32.geometry} material={materials.iyDJFXmHelnMTbD} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_34.geometry} material={materials.eJObPwhgFzvfaoZ} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_38.geometry} material={materials.nDsMUuDKliqGFdU} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_42.geometry} material={materials.CRQixVLpahJzhJc} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_48.geometry} material={materials.YYwBgwvcyZVOOAA} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_54.geometry} material={materials.SLGkCohDDelqXBu} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_58.geometry} material={materials.WnHKXHhScfUbJQi} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_66.geometry} material={materials.fNHiBfcxHUJCahl} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_74.geometry} material={materials.LpqXZqhaGCeSzdu} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_82.geometry} material={materials.gMtYExgrEUqPfln} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_96.geometry} material={materials.PaletteMaterial003} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_107.geometry} material={materials.JvMFZolVCdpPqjj} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_123.geometry} material={materials.sfCQkHOWyrsLmor} rotation={[Math.PI / 2, 0, 0]} /> | ||
| <mesh geometry={nodes.Object_127.geometry} material={materials.ZCDwChwkbBfITSW} rotation={[Math.PI / 2, 0, 0]} /> | ||
| </group> | ||
| ) | ||
| const { color, texture } = useMacbookStore() | ||
|
|
||
| const { nodes, materials, scene } = useGLTF('/models/macbook-transformed.glb') | ||
|
|
||
| const screen = useVideoTexture(texture) | ||
|
|
||
| useEffect(() => { | ||
| scene.traverse((child) => { | ||
| if (child.isMesh) { | ||
| if (!noChangeParts.includes(child.name)) { | ||
| child.material.color = new Color(color) | ||
| } | ||
| } | ||
| }) | ||
| }, [color, scene]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @react-three/drei useVideoTexture hook properly dispose of video elements when the src URL changes?
💡 Result:
No, the useVideoTexture hook from @react-three/drei does not properly dispose of video elements when the src URL changes. When the src changes, suspend-react (used internally for caching) treats it as a new key, creating a new video element and texture. The old video element remains in the suspend-react cache, continues playing, and is not paused, src-cleared, or removed. The cleanup useEffect only destroys HLS instances if present and does not handle the HTMLVideoElement or evict the cache. The VideoTexture component calls texture.dispose on unmount, but this only affects the THREE texture, not the video DOM element. This leads to memory leaks and multiple videos playing simultaneously. A related open GitHub issue (#2712, opened 2026-03-28) confirms this behavior with a reproduction repo, and a fix PR (#2714) is proposed but not yet merged as of 2026-04-06.
Citations:
- 1:
VideoTexture- dispose() pmndrs/drei#2123 - 2: https://github.com/pmndrs/drei/blob/7d901b5c/src/core/VideoTexture.tsx
- 3: useVideoTexture: cached video element not cleaned up on unmount pmndrs/drei#2712
- 4: correctly dispose useVideoTexture() pmndrs/drei#1586
Rapid texture changes cause memory leaks and simultaneous video playback.
The useVideoTexture hook from drei does not properly dispose of video elements when the src URL changes. When texture from the store changes rapidly (triggered by GSAP in Features.jsx), each new URL creates a new video element while old ones persist in the suspend-react cache, continue playing, and are never cleaned up. This causes memory leaks and multiple videos playing simultaneously. See pmndrs/drei#2712 and the proposed fix in pmndrs/drei#2714. Consider implementing a workaround or patching drei until the fix is merged.
| reset: () => | ||
| set({ color: '#2e2c2e', scale: 0.08, texture: './videos/feature-1.mp4' }) |
There was a problem hiding this comment.
Path inconsistency in reset() will break video texture loading.
The initial texture value uses an absolute path ('/videos/feature-1.mp4'), but reset() uses a relative path with a leading dot ('./videos/feature-1.mp4'). This inconsistency will cause the video texture to fail loading if reset() is ever called.
Proposed fix
reset: () =>
- set({ color: '#2e2c2e', scale: 0.08, texture: './videos/feature-1.mp4' })
+ set({ color: '#2e2c2e', scale: 0.08, texture: '/videos/feature-1.mp4' })📝 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.
| reset: () => | |
| set({ color: '#2e2c2e', scale: 0.08, texture: './videos/feature-1.mp4' }) | |
| reset: () => | |
| set({ color: '#2e2c2e', scale: 0.08, texture: '/videos/feature-1.mp4' }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/index.js` around lines 13 - 14, The reset setter currently assigns
texture './videos/feature-1.mp4' which is inconsistent with the initial texture
'/videos/feature-1.mp4' and will break video loading; update the reset()
implementation (the reset arrow function that calls set) to use the same
absolute path '/videos/feature-1.mp4' for the texture key so both the initial
state and the set/reset flow use the identical path.
1 participant
Summary by CodeRabbit