fix: handle _leaflet_pos TypeError on chapters map#4296
fix: handle _leaflet_pos TypeError on chapters map#4296taditharun wants to merge 7 commits intoOWASP:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds defensive guards in ChapterMap: MapZoomControl and its cleanup now early-return when Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ChapterMap.tsx`:
- Line 64: The TSX parse error is caused by a stray "M" after the closing brace
of the ChapterMap component; open the ChapterMap component (component/function
named ChapterMap in frontend/src/components/ChapterMap.tsx), remove the trailing
"M" character following the final "}" so the file ends with a valid closing
brace only, then save and run the TypeScript/TSX build to confirm the parse
error is resolved.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25bf5973-4f7b-4eab-a341-2300f34339f4
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx
anurag2787
left a comment
anurag2787
left a comment
There was a problem hiding this comment.
Hi @taditharun thanks for working on this
I think we should focus on fixing the root cause of the issue rather than handling it with a try catch
Also please run make check-test locally before pushing your code
|
hiii @anurag2787 , Thanks for the review! Will fix the root cause and run make check-test before pushing. |
taditharun
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
11dfff5
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ChapterMap.tsx (1)
74-79:⚠️ Potential issue | 🟡 MinorMissing null check for
map.getContainer()before accessing properties.
MapViewUpdaterretrieves the container at line 76 but immediately accessesclientWidthandclientHeightwithout verifying the container exists. IfgetContainer()returnsundefined(same race condition as the original bug), this will throw a TypeError.🛡️ Proposed fix to guard container access
useEffect(() => { if (!map) return const container = map.getContainer() + if (!container) return const width = container.clientWidth const height = container.clientHeight const aspectRatio = height > 0 ? width / height : 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ChapterMap.tsx` around lines 74 - 79, The useEffect block that reads map.getContainer() may receive undefined; add a null check for the container returned by map.getContainer() before accessing container.clientWidth or container.clientHeight (inside the same useEffect where map is checked). If container is falsy, either return early or fall back to safe defaults for width/height/aspectRatio so no TypeError is thrown; update the variables computed (width, height, aspectRatio) accordingly in the useEffect containing map.getContainer().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ChapterMap.tsx`:
- Around line 20-22: The cleanup function inside the useEffect that manipulates
the Leaflet map can run after the map container was removed and therefore should
mirror the defensive checks used on mount; update the cleanup block in the same
useEffect (the function returned from useEffect that references map and calls
map.getContainer(), invalidateSize, or manipulates _leaflet_pos) to first verify
map is defined and map.getContainer() is truthy before performing any map
operations, so the cleanup early-returns if the container is gone and avoids the
_leaflet_pos TypeError.
---
Outside diff comments:
In `@frontend/src/components/ChapterMap.tsx`:
- Around line 74-79: The useEffect block that reads map.getContainer() may
receive undefined; add a null check for the container returned by
map.getContainer() before accessing container.clientWidth or
container.clientHeight (inside the same useEffect where map is checked). If
container is falsy, either return early or fall back to safe defaults for
width/height/aspectRatio so no TypeError is thrown; update the variables
computed (width, height, aspectRatio) accordingly in the useEffect containing
map.getContainer().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25a4a499-d606-484e-924e-665f61cd7b1e
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx
|
Fixed!, Added null checks for map container in the cleanup effect and MapViewUpdater as well. Ran ChapterMap tests locally 40/40 passed. |
anurag2787
left a comment
anurag2787
left a comment
There was a problem hiding this comment.
Hi @taditharun i can still see the error
taditharun
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
7b62866
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ChapterMap.tsx">
<violation number="1" location="frontend/src/components/ChapterMap.tsx:127">
P2: Empty broad `catch` swallows all map update errors, masking real failures beyond the intended `_leaflet_pos` teardown case.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ChapterMap.tsx (1)
75-133:⚠️ Potential issue | 🔴 CriticalCritical: Missing closing brace for useEffect callback causes parse error.
The
useEffectcallback function is never closed before the dependency array. Line 131 has an orphaned dependency array[userLocation, showLocal, validGeoLocData, map])that is not syntactically connected to theuseEffectcall. This is a parse-breaking error that will prevent the application from building.Additional issues:
- Line 77: Missing space before
{intry{- Line 128: Typo "complite" → "complete"
- Inconsistent indentation throughout the block
🐛 Proposed fix for the parse error and formatting
useEffect(() => { if (!map) return - try{ + try { - const container = map.getContainer() - if (!container) return + const container = map.getContainer() + if (!container) return - const width = container.clientWidth - const height = container.clientHeight - const aspectRatio = height > 0 ? width / height : 1 + const width = container.clientWidth + const height = container.clientHeight + const aspectRatio = height > 0 ? width / height : 1 - const dynamicMinZoom = aspectRatio > 2 ? 1 : 2 - map.setMinZoom(dynamicMinZoom) + const dynamicMinZoom = aspectRatio > 2 ? 1 : 2 + map.setMinZoom(dynamicMinZoom) - if (userLocation && validGeoLocData.length > 0) { + if (userLocation && validGeoLocData.length > 0) { // ... existing code with proper indentation ... - } else if (showLocal && validGeoLocData.length > 0) { + } else if (showLocal && validGeoLocData.length > 0) { // ... existing code with proper indentation ... - } else { - map.setView([20, 0], Math.max(dynamicMinZoom, 2)) - } - } catch { - // the map container was removed before update complite - - } - [userLocation, showLocal, validGeoLocData, map]) - - return null + } else { + map.setView([20, 0], Math.max(dynamicMinZoom, 2)) + } + } catch { + // the map container was removed before update complete + } + }, [userLocation, showLocal, validGeoLocData, map]) + + return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ChapterMap.tsx` around lines 75 - 133, The useEffect callback is missing its closing brace before the dependency array which causes a parse error; close the try/catch/useEffect block properly by adding the required closing brace and parenthesis so the dependency array [userLocation, showLocal, validGeoLocData, map] is attached to useEffect, fix the "try{" spacing to "try {", correct the catch comment typo "complite" → "complete" (or add an error parameter to catch if needed), and normalize indentation inside the useEffect (refer to the useEffect callback that reads map.getContainer(), map.setMinZoom(), map.fitBounds(), map.setView() and the catch block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/components/ChapterMap.tsx`:
- Around line 75-133: The useEffect callback is missing its closing brace before
the dependency array which causes a parse error; close the try/catch/useEffect
block properly by adding the required closing brace and parenthesis so the
dependency array [userLocation, showLocal, validGeoLocData, map] is attached to
useEffect, fix the "try{" spacing to "try {", correct the catch comment typo
"complite" → "complete" (or add an error parameter to catch if needed), and
normalize indentation inside the useEffect (refer to the useEffect callback that
reads map.getContainer(), map.setMinZoom(), map.fitBounds(), map.setView() and
the catch block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 696b2cc0-3bd2-4587-9f0a-3969a74be32b
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx
taditharun
force-pushed
the
fix/leaflet-typeerror-chapters-map
branch
from
87e2c74 to
d2f5c8e
Compare
|
Hi @arkid15r and @anurag2787 Fixed the root cause and removed try,catch completely and kept only proper null checks for map container. All 40 ChapterMap tests passing locally with 0 errors. I don't have Algolia keys locally so unable to test the full search flow could you verify on your end? |
The error is still occurring on my end also for Algolia you can get a free API key from https://www.algolia.com |
|
|
Hi @anurag2787 !, Got Algolia keys set up and tested locally. The TypeError is fixed applying Date Created filter and selecting a country shows no errors in console. Screenshot attached. Could you please re-review? 
|
|
Hi @anurag2787 , just wanted to add I also tested on the live production site at nest.owasp.org/chapters and the bug is still reproducible there. Selecting Albania triggers the same Uncaught TypeError: Cannot read properties of undefined 
ded on production too!
|
2 participants
Proposed change
Resolves #4286
While browsing the chapters page I noticed a JavaScript error
in the browser console. The error occurs when the Date Created
filter is applied and a country is selected from the dropdown.
Error:
Uncaught TypeError: Cannot read properties of undefined
(reading '_leaflet_pos')
The issue is in the MapZoomControl component — leaflet tries
to read the map pane position during zoom transition before
the map container is fully initialized.
Wrapped the map control logic in a try/catch block to handle
this gracefully.
Type of change
Checklist