Fix React error #310: Stabilize hooks and remove duplicate directives #398
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
WalkthroughThe PR addresses performance and stability across multiple map and calendar components by adding defensive guards, cleanup handlers, and conditional updates to prevent redundant re-renders and ensure proper state management during component lifecycle transitions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Suggested nitpicks:
Overall: These changes directly address the production crash and improve hook hygiene and component stability. The PR is a clear win for reliability—just ensure that functional dependencies are really stable, and perhaps add a quick explanatory note near the more complex effects. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
![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
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
components/calendar-notepad.tsxcomponents/map/geojson-layer.tsxcomponents/map/map-query-handler.tsxcomponents/map/mapbox-map.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
components/map/map-query-handler.tsx (1)
components/map/map-data-context.tsx (1)
MapData(7-17)
components/calendar-notepad.tsx (2)
lib/actions/calendar.ts (1)
getNotes(16-57)lib/auth/use-current-user.ts (1)
fetchUser(10-20)
components/map/mapbox-map.tsx (1)
components/map/map-data-context.tsx (1)
MapData(7-17)
🔇 Additional comments (6)
components/calendar-notepad.tsx (1)
23-39: LGTM! Well-implemented cleanup pattern for async effects.The
isMountedflag correctly guards against state updates after unmount, and the try/catch provides proper error handling. This pattern effectively addresses React error #310 for this component.components/map/geojson-layer.tsx (1)
82-100: LGTM! Robust cleanup pattern for map layers.The implementation correctly handles both the loaded and loading states. Wrapping cleanup in try/catch prevents unmount failures from breaking the application. The
map.off('load', onMapLoad)call at Line 90 is defensive—map.once()auto-removes after firing, so this is a safe no-op in most cases, which is fine for ensuring cleanup consistency.components/map/map-query-handler.tsx (1)
38-52: Good guarded update pattern to prevent infinite loops.The position comparison effectively prevents redundant
flyTocalls when the target hasn't changed. The implementation assumestargetPositionis always in array form[lng, lat], which is consistent with how this component sets it.components/map/mapbox-map.tsx (3)
164-169: Good guard to prevent infinite re-render loops.Using
JSON.stringifyfor deep comparison is effective here. For typical usage with a small number of drawn features, the performance impact is negligible. This correctly prevents the state update from triggering unnecessary re-renders that could cause React error #310.
519-531: Effective optimization to prevent redundantflyTocalls.The distance threshold check (0.0001 ≈ ~11 meters) is reasonable for determining if the map is already at the target position. This prevents animation stuttering and unnecessary map updates.
One consideration: the Euclidean distance calculation treats latitude and longitude uniformly, which introduces minor inaccuracy at high latitudes, but for this "already at position" threshold use case, it's perfectly acceptable.
558-566: Clean and correct timer cleanup.The simplified cleanup effect properly clears the long-press timer on unmount. The empty dependency array is correct since this only needs to run cleanup on unmount.
| setMapData(prevData => { | ||
| if (prevData.targetPosition === null && prevData.mapFeature === null) return prevData; | ||
| return { | ||
| ...prevData, | ||
| targetPosition: null, | ||
| mapFeature: null | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Guard condition may not match initial state.
Per map-data-context.tsx, targetPosition and mapFeature are typed as ... | null | undefined. The initial state may have these as undefined rather than null. The check on Line 55 only matches null, so if the initial state uses undefined, this guard won't prevent the first redundant update.
🔎 Suggested fix
setMapData(prevData => {
- if (prevData.targetPosition === null && prevData.mapFeature === null) return prevData;
+ if (!prevData.targetPosition && !prevData.mapFeature) return prevData;
return {
...prevData,
targetPosition: null,
mapFeature: null
};
});📝 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.
| setMapData(prevData => { | |
| if (prevData.targetPosition === null && prevData.mapFeature === null) return prevData; | |
| return { | |
| ...prevData, | |
| targetPosition: null, | |
| mapFeature: null | |
| }; | |
| }); | |
| setMapData(prevData => { | |
| if (!prevData.targetPosition && !prevData.mapFeature) return prevData; | |
| return { | |
| ...prevData, | |
| targetPosition: null, | |
| mapFeature: null | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In components/map/map-query-handler.tsx around lines 54 to 61, the guard only
checks for strict null which misses undefined (initial state allows null |
undefined); change the condition to check for both null and undefined (e.g., use
loose equality null check: if (prevData.targetPosition == null &&
prevData.mapFeature == null) return prevData;) so the redundant state update is
avoided for both null and undefined initial values.
User description
This PR fixes the minified React error #310 by:
These changes prevent hook misuse and unstable render cycles that were causing the application to crash in production builds.
PR Type
Bug fix
Description
Remove duplicate 'use client' directive in calendar-notepad.tsx
Add isMounted checks and cleanup logic to useEffect hooks
Stabilize dependency arrays to prevent infinite re-render loops
Optimize state updates to only occur when data actually changes
Add error handling and try-catch blocks in map components
Diagram Walkthrough
File Walkthrough
calendar-notepad.tsx
Remove duplicate directive and add cleanup logiccomponents/calendar-notepad.tsx
isMountedflag to prevent state updates after component unmountgetNotescall in try-catch for error handlingisMountedto false on unmountgeojson-layer.tsx
Stabilize map load listeners and add error handlingcomponents/map/geojson-layer.tsx
map.on('load')withmap.once('load')to prevent duplicatelisteners
map.off('load', onMapLoad)in cleanup functionisStyleLoadedin variable to improve code claritymap-query-handler.tsx
Optimize state updates with position comparisoncomponents/map/map-query-handler.tsx
mapbox-map.tsx
Optimize map updates and simplify cleanup logiccomponents/map/mapbox-map.tsx
drawnFeatureswhen unchangedupdateMapPositionto avoid redundantflyTo
array
Summary by CodeRabbit
Bug Fixes
Performance
✏️ Tip: You can customize this high-level summary in your review settings.