Refactor Rain component to improve mobile responsiveness and dynamic … #9
Conversation
…settings management
WalkthroughRefactors the rain sketch to be fully runtime- and state-driven. Replaces global/mobile flags and static arrays with React state and refs, adds SSR-safe dimension handling, debounced resize/orientation listeners, dynamic settings, and refactored helpers. P5 setup/draw now use refs and current settings; canvas resizes and drops reinitialize on dimension changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant React as React Component
participant Window
participant P5 as P5 Instance
participant Helpers as Helpers/settings
rect rgba(200,230,255,0.2)
User->>React: Mount InteractiveRain
React->>Helpers: getCurrentDimensions()
React->>P5: setupFunction(dimensions)
P5-->>React: p5InstanceRef, canvas created
React->>Helpers: initializeDrops(width,height, currentSettings)
Helpers-->>React: dropsRef seeded
end
loop draw loop
P5->>React: drawFunction tick
React->>Helpers: processDrop(drop, currentSettings, windowDimensions)
Helpers-->>React: rendered geometry (line/curve)
React->>P5: render with currentSettings (color/alpha/radii)
end
rect rgba(255,240,200,0.25)
Window-->>React: resize/scroll/orientationchange/visibility
React->>React: debounced via rAF
React->>Helpers: getCurrentDimensions(), checkIfMobile()
React->>React: update currentSettings
React->>P5: resizeCanvas(new dimensions)
React->>Helpers: initializeDrops(new width/height, currentSettings)
Helpers-->>React: dropsRef replaced
end
note over React,P5: Refs (dropsRef, p5InstanceRef, canvasRef) persist across renders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
yellowumbrella-web | c4361c4 | Commit Preview URL Branch Preview URL |
Sep 05 2025, 08:11 AM |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Rain component to improve mobile responsiveness and dynamic behavior. The changes focus on making the rain animation more adaptive to different screen sizes and device orientations while adding proper cleanup and performance optimizations.
- Converts static mobile detection to dynamic state-based detection with proper event handling
- Implements comprehensive window resize, scroll, and orientation change handling
- Adds proper cleanup and performance optimizations with refs and debouncing
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| return { | ||
| width: window.innerWidth, | ||
| height: Math.max(window.innerHeight, document.documentElement.scrollHeight, document.body.scrollHeight) |
There was a problem hiding this comment.
This calculation on every resize/scroll event can be expensive. Consider caching these DOM measurements or using a more efficient approach like ResizeObserver for better performance.
| if (mobile) { | ||
| document.addEventListener('touchstart', handleTouchStart, { passive: false }); | ||
| } |
There was a problem hiding this comment.
The mobile variable is captured in the effect's closure and won't update if the device orientation changes. This could lead to inconsistent touch event handling. Use isMobile state instead.
| if (mobile) { | ||
| document.removeEventListener('touchstart', handleTouchStart); | ||
| } |
There was a problem hiding this comment.
Same issue as the addEventListener - using the stale mobile variable from closure instead of current state. This could prevent proper cleanup of touch event listeners.
| if (mobile) { | |
| document.removeEventListener('touchstart', handleTouchStart); | |
| } | |
| if (checkIfMobile()) { | |
| document.removeEventListener('touchstart', handleTouchStart); | |
| } |
|
|
||
| clearTimeout(initialTimeout); | ||
| }; | ||
| }, [resizeCanvas, checkIfMobile, updateSettings, windowDimensions.height, getCurrentDimensions]); |
There was a problem hiding this comment.
Including windowDimensions.height in the dependency array will cause the entire effect to re-run every time the height changes, potentially creating duplicate event listeners. Consider removing it since resize events already handle dimension changes.
| }, [resizeCanvas, checkIfMobile, updateSettings, windowDimensions.height, getCurrentDimensions]); | |
| }, [resizeCanvas, checkIfMobile, updateSettings, getCurrentDimensions]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Rain.jsx (1)
4-24: Define mouseInteractionRadius at init to avoid NaNs during first framescurrentSettings starts without mouseInteractionRadius; draw/processDrop may run before updateSettings executes, leading to NaN comparisons/sqrt and broken rendering. Seed a default in SETTINGS.
const SETTINGS = { dropNumber: 200, dropLength: 30, globalSpeedMultiplier: 5, interactionStepDivider: 10, useUmbrella: false, mouseUmbrellaShadow: 0.8, windSpeed: -10, + // Default to non-mobile; updateSettings will override at runtime + mouseInteractionRadius: 100, dropColor: { r: 220, g: 220, b: 255, baseAlpha: 32 } }Also applies to: 47-52, 256-276, 268-276, 300-301
🧹 Nitpick comments (6)
src/components/Rain.jsx (6)
39-45: Mobile detection: prefer matchMedia/pointer coarse; decouple from magic 800pxwindow.matchMedia('(pointer: coarse)') or CSS breakpoints via matchMedia are more robust than a fixed width.
-const checkIfMobile = useCallback(() => { - const mobile = typeof window !== 'undefined' && window.innerWidth <= 800; +const checkIfMobile = useCallback(() => { + const mobile = + typeof window !== 'undefined' && + (window.matchMedia?.('(pointer: coarse)').matches || window.innerWidth <= 800); setIsMobile(mobile); return mobile; }, []);
194-196: Avoid double clear per framep5.clear() already clears; background(0, 0) right after is redundant.
- p5.clear(); - p5.background(0, 0); + p5.clear();
65-77: Off-by-one on initial seeding (x/y can equal width/height)Math.round(Math.random() * width) can return width; use floor to keep coordinates within [0, width-1].
- x: Math.round(Math.random() * width), - y: Math.round(Math.random() * height), + x: Math.floor(Math.random() * width), + y: Math.floor(Math.random() * height),Also applies to: 69-71
287-291: Promote magic constant 20 to a setting for easier tuningExpose as settings.xSmoothingThreshold and use it here.
- if (xDiff > 20) { + if (xDiff > settings.xSmoothingThreshold) { // Handle large x difference calculatedX = lastX; }Add to SETTINGS:
windSpeed: -10, +xSmoothingThreshold: 20, dropColor: { ... }
367-369: Remove unused _frictionOffsetDead code adds maintenance noise.
-const _frictionOffset = (x, distanceFromOrigin, settings) => { - return Math.round(Math.cos(x + distanceFromOrigin) * settings.windSpeed); -}
31-33: Use isMobile state for binding lifecycle or drop itisMobile is set but not used to (un)bind listeners post-resize. Either bind/unbind based on isMobile (see earlier snippet) or remove the state to reduce complexity.
Also applies to: 146-149, 163-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/Rain.jsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: yellowumbrella-web
🔇 Additional comments (1)
src/components/Rain.jsx (1)
223-235: Verify pointer data reliability with z-index: -1With the canvas behind content, p5 may not receive mouse/touch events; umbrella interaction can be inert under overlays. Please validate on pages with overlaying elements.
| const getCurrentDimensions = useCallback(() => { | ||
| if (typeof window === 'undefined') return { width: 800, height: 600 }; | ||
|
|
||
| return { | ||
| width: window.innerWidth, | ||
| height: Math.max(window.innerHeight, document.documentElement.scrollHeight, document.body.scrollHeight) | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid sizing canvas to document scrollHeight (huge offscreen surfaces, jank)
Using Math.max(...scrollHeight) can make the canvas as tall as the whole page; every frame clears/strokes that area. Prefer viewport height and drop the scroll-based resize.
const getCurrentDimensions = useCallback(() => {
if (typeof window === 'undefined') return { width: 800, height: 600 };
- return {
- width: window.innerWidth,
- height: Math.max(window.innerHeight, document.documentElement.scrollHeight, document.body.scrollHeight)
- };
+ return { width: window.innerWidth, height: window.innerHeight };
}, []);
- const handleScroll = () => {
- // Actualizar dimensiones en scroll para manejar páginas largas
- if (animationRef.current) {
- cancelAnimationFrame(animationRef.current);
- }
- animationRef.current = requestAnimationFrame(() => {
- const newDimensions = getCurrentDimensions();
- if (newDimensions.height !== windowDimensions.height && p5InstanceRef.current) {
- p5InstanceRef.current.resizeCanvas(newDimensions.width, newDimensions.height);
- setWindowDimensions(newDimensions);
- }
- });
- };
-
// Agregar event listeners
window.addEventListener('resize', handleResize, { passive: true });
-window.addEventListener('scroll', handleScroll, { passive: true });
window.addEventListener('orientationchange', handleResize, { passive: true });
...
// Cleanup
window.removeEventListener('resize', handleResize);
-window.removeEventListener('scroll', handleScroll);
window.removeEventListener('orientationchange', handleResize);Also applies to: 113-126, 142-142, 159-159
🤖 Prompt for AI Agents
In src/components/Rain.jsx around lines 55-62 (and also apply same fix at
113-126, 142, 159), the canvas sizing uses Math.max(window.innerHeight,
document.documentElement.scrollHeight, document.body.scrollHeight) which can
expand the canvas to the full document scrollHeight causing huge offscreen
surfaces and jank; replace that logic to use only the viewport height
(window.innerHeight) with an SSR-safe fallback (e.g., return {width: 800,
height: 600} when window is undefined), remove any use of document.*scrollHeight
in resize/dimension calculations, and ensure all other occurrences referenced
(lines 113-126, 142, 159) are changed the same way so the canvas is sized to the
viewport only.
| const handleTouchStart = (e) => { | ||
| // Prevenir que los toques detengan la animación | ||
| e.preventDefault(); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document-level touchstart preventDefault blocks page scroll/gestures on mobile
Preventing default at the document level disables scrolling. Scope it to the canvas only.
-// Prevenir que los toques interfieran con la animación
-if (mobile) {
- document.addEventListener('touchstart', handleTouchStart, { passive: false });
-}
+// Limitar a solo el canvas para no bloquear el scroll de la página
+if (mobile && canvasRef.current?.elt) {
+ canvasRef.current.elt.addEventListener('touchstart', handleTouchStart, { passive: false });
+}
...
-if (mobile) {
- document.removeEventListener('touchstart', handleTouchStart);
-}
+if (canvasRef.current?.elt) {
+ canvasRef.current.elt.removeEventListener('touchstart', handleTouchStart);
+}Optionally, add a small effect to (un)bind when isMobile or the canvas becomes available:
useEffect(() => {
const el = canvasRef.current?.elt;
if (!el) return;
if (isMobile) {
el.addEventListener('touchstart', handleTouchStart, { passive: false });
return () => el.removeEventListener('touchstart', handleTouchStart);
}
}, [isMobile]);Also applies to: 146-149, 163-165
Pues diria que lo de la lluvia está ya arreglado, pero por si acaso se me pasa algo, revisenlo.
Summary by CodeRabbit
New Features
Bug Fixes
Chores