fix: Fix error 500 for OWASP Virtual Chapter #2481
Conversation
Summary by CodeRabbitBug Fixes
WalkthroughFilters out chapters lacking numeric lat/lng before marker creation, replaces falsy fallbacks with nullish coalescing for lat/lng, and adds tests ensuring undefined/null coords are ignored while [0,0] is accepted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ChapterMap.tsx (1)
57-85: Marker creation logic is sound.The use of
validGeoLocDataensures only chapters with valid coordinates reach this point. The nullish coalescing operator is applied consistently.Consider extracting coordinates once during filtering to avoid duplication:
const validGeoLocData = geoLocData .map((chapter) => { const lat = chapter._geoloc?.lat ?? chapter.geoLocation?.lat const lng = chapter._geoloc?.lng ?? chapter.geoLocation?.lng return typeof lat === 'number' && typeof lng === 'number' ? { ...chapter, coordinates: { lat, lng } } : null }) .filter((chapter): chapter is Chapter & { coordinates: { lat: number; lng: number } } => chapter !== null ) // Then use: chapter.coordinates.lat, chapter.coordinates.lngThis eliminates repeated coordinate lookups and improves type safety.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx(2 hunks)frontend/src/components/ChapterMap.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
frontend/__tests__/unit/data/mockChapterData.ts (1)
mockChapterData(1-32)
🔇 Additional comments (5)
frontend/__tests__/unit/components/ChapterMap.test.tsx (3)
166-182: Excellent test coverage for undefined coordinates.This test correctly validates that virtual chapters without location data are filtered out, preventing the 500 error.
184-200: Good coverage for null coordinates.This test ensures null values are also filtered correctly, complementing the undefined test case.
238-252: Critical edge case test for zero coordinates.This test is particularly important because it validates that the nullish coalescing (
??) operator correctly preserves valid coordinates at[0, 0](Gulf of Guinea), which would be incorrectly filtered with a logical OR (||) operator.frontend/src/components/ChapterMap.tsx (2)
89-108: Local view logic correctly updated.The changes consistently use
validGeoLocDatafor bounds calculation and nearest chapter selection, ensuring the local view only considers chapters with valid coordinates. The length check at line 89 properly guards against empty filtered data.
51-55: Filter logic is correct; coordinate mixing doesn't occur in practice.The nullish coalescing approach properly handles the fallback between
_geolocandgeoLocation. Test data and the data model show that both fields contain the same coordinates when defined, so mixing doesn't happen in practice. The current implementation correctly prevents the 500 error for virtual chapters and is ready as-is.The suggested refactor to validate coordinates come from the same source is a nice-to-have for stricter validation if you prefer that pattern, but not necessary given your data model structure.
| const L = require('leaflet') | ||
| const virtualChapterData: Chapter[] = [ |
There was a problem hiding this comment.
We do not use require in this project. This is why you were getting the linting error - please do not just disable the error with the comment, they are there for a reason.
So please update these tests
There was a problem hiding this comment.
Sorry about this. I wrote the test this way because the previous tests were also using require. I have now replaced all these tests using require with import
There was a problem hiding this comment.
@kart-u yes, sorry! After I left this message I did a global search and found a few more spots where we had the require statement that clearly missed in some prev. PRs. I'll be cleaning those up. Thanks for updating your code!
There was a problem hiding this comment.
hello @kasya
just wanted to check if everything in current PR looks good or is there anything else you’d like me to update?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
156-186: Good coverage for virtual chapter filtering.The tests properly validate that chapters with undefined or null coordinates are filtered out, which directly addresses the 500 error for the OWASP Virtual Chapter. The distinction between null and undefined is appropriate for nullish coalescing behavior.
Consider adding tests for partial null coordinates (e.g.,
latvalid butlngnull) to ensure the implementation handles mixed scenarios correctly:it('filters out chapters with partial null coordinates', () => { const partialNullData: Chapter[] = [ mockChapterData[0], { ...mockChapterData[1], _geoloc: { lat: 51.5074, lng: null }, geoLocation: { lat: 51.5074, lng: null }, }, ] render(<ChapterMap {...defaultProps} geoLocData={partialNullData} />) expect(L.marker).toHaveBeenCalledTimes(1) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
frontend/__tests__/unit/data/mockChapterData.ts (1)
mockChapterData(1-32)
🔇 Additional comments (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx (2)
2-2: LGTM! Import statement follows project standards.The change from
requiretoimportaligns with the feedback from previous reviews and project conventions.
219-231: Excellent edge case coverage for zero coordinates.This test is crucial because it validates that legitimate [0, 0] coordinates (Gulf of Guinea intersection of Prime Meridian and Equator) are not filtered out. Since 0 is falsy in JavaScript, this confirms the implementation correctly uses nullish coalescing rather than falsy checks.
|
2 participants
Proposed change
Resolves #2467
Issue Overview - on visting https://nest.owasp.org/chapters/virtual virtual chapter was on not rendering and giving error 500
Solution - In
ChapterMap.tsxwe need a filter for case when latitude and longitude are null/none (case of virtual chapter)On side note regarding local test check
__tests__/unit/pages/Home.test.tsxthis is failing on original main branch even without any changes.Checklist
make check-testlocally; all checks and tests passed.