Conversation
✅ Deploy Preview for btcmap ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughA new Event Map feature is introduced with navigation UI entry, supporting type definitions, layout routing configuration, and an interactive Leaflet-based map page displaying geolocated events with popups, controls, and dark mode support. Changes
Sequence DiagramsequenceDiagram
participant User as User/Client
participant Page as +page.svelte
participant API as External API
participant Map as Leaflet Map
participant DOM as DOM/UI
User->>Page: Mount component
Page->>Page: onMount triggered
Page->>API: Fetch events data
API-->>Page: Event array returned
Page->>Map: Load Leaflet dependencies
Map-->>Page: Dependencies loaded
Page->>Map: Initialize map instance
Page->>Map: Configure base layers & controls
Page->>Map: Add attribution, scale, geolocation
Page->>Page: Check if mapLoaded && events exist
Page->>Map: Create markers for each event
Map->>DOM: Render markers with popups
Map->>DOM: Apply dark mode styling
DOM-->>User: Interactive map displayed
User->>Map: Interact with map (pan/zoom)
Page->>Page: Update URL hash with view state
User->>Page: Unmount component
Page->>Map: onDestroy - remove markers
Page->>Map: Destroy map instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 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/routes/events/map/`+page.svelte:
- Line 92: The reactive guard currently checks events?.length which prevents
initializeEvents() from running when events is an empty array or error result;
update the reactive statement so it triggers when events is defined (e.g.,
events != null or typeof events !== "undefined") along with mapLoaded &&
!eventsLoaded, so initializeEvents() always runs even for empty results and
allows mapLoading to finish; update the reactive line referencing events,
mapLoaded, eventsLoaded, and initializeEvents accordingly.
- Around line 44-71: The popup uses innerHTML with unescaped interpolations
(popupContainer.innerHTML) of untrusted fields event.name and event.website,
enabling DOM XSS; to fix, stop injecting raw HTML: build the popup DOM using
safe methods (createElement, textContent, setAttribute) or sanitize/validate
inputs before inserting, ensure links use safe hrefs (reject or strip
javascript: schemes) when creating the anchor for event.website, and keep
theme-related style insertion controlled (use a CSS class toggle using
currentMapTheme instead of injecting <style> with string interpolation).
Reference popupContainer, event.name, event.website and currentMapTheme to
locate and replace the innerHTML assignment with DOM-safe construction.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/layout/Header.sveltesrc/lib/i18n/locales/en.jsonsrc/lib/types.tssrc/routes/+layout.sveltesrc/routes/events/map/+page.svelte
Comment on lines
+44
to
+71
| popupContainer.innerHTML = ` | ||
| <div class='text-center space-y-2'> | ||
| <span class='text-primary dark:text-white font-semibold text-xl' title='Event name'>${event.name}</span> | ||
|
|
||
| ${ | ||
| event.website | ||
| ? `<a href="${event.website}" target="_blank" rel="noopener noreferrer" class='block mt-4 bg-link hover:bg-hover !text-white text-center font-semibold py-3 rounded-xl transition-colors' title='Event website'>Visit Website</a>` | ||
| : "" | ||
| } | ||
| </div> | ||
|
|
||
| ${ | ||
| currentMapTheme === "dark" | ||
| ? ` | ||
| <style> | ||
| .leaflet-popup-content-wrapper, .leaflet-popup-tip { | ||
| background-color: #06171C; | ||
| border: 1px solid #e5e7eb | ||
| } | ||
|
|
||
| .leaflet-popup-close-button { | ||
| font-size: 24px !important; | ||
| top: 4px !important; | ||
| right: 4px !important; | ||
| } | ||
| </style>` | ||
| : "" | ||
| }`; |
There was a problem hiding this comment.
Prevent DOM XSS in popup rendering.
Untrusted API fields (event.name, event.website) are interpolated into innerHTML. That enables script/attribute injection (including unsafe javascript: URLs).
🔐 Suggested fix
+const safeExternalUrl = (value: string): string | null => {
+ try {
+ const parsed = new URL(value);
+ return parsed.protocol === "https:" || parsed.protocol === "http:"
+ ? parsed.toString()
+ : null;
+ } catch {
+ return null;
+ }
+};
events.forEach((event: EventMapEvent) => {
- const popupContainer = leaflet.DomUtil.create("div");
- popupContainer.innerHTML = `
- <div class='text-center space-y-2'>
- <span class='text-primary dark:text-white font-semibold text-xl' title='Event name'>${event.name}</span>
- ${event.website ? `<a href="${event.website}" ...>Visit Website</a>` : ""}
- </div>
- ...`;
+ const popupContainer = leaflet.DomUtil.create("div", "text-center space-y-2");
+ const title = leaflet.DomUtil.create(
+ "span",
+ "text-primary dark:text-white font-semibold text-xl",
+ popupContainer,
+ );
+ title.title = "Event name";
+ title.textContent = event.name;
+
+ const website = event.website ? safeExternalUrl(event.website) : null;
+ if (website) {
+ const link = leaflet.DomUtil.create(
+ "a",
+ "block mt-4 bg-link hover:bg-hover !text-white text-center font-semibold py-3 rounded-xl transition-colors",
+ popupContainer,
+ ) as HTMLAnchorElement;
+ link.href = website;
+ link.target = "_blank";
+ link.rel = "noopener noreferrer";
+ link.title = "Event website";
+ link.textContent = "Visit Website";
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/events/map/`+page.svelte around lines 44 - 71, The popup uses
innerHTML with unescaped interpolations (popupContainer.innerHTML) of untrusted
fields event.name and event.website, enabling DOM XSS; to fix, stop injecting
raw HTML: build the popup DOM using safe methods (createElement, textContent,
setAttribute) or sanitize/validate inputs before inserting, ensure links use
safe hrefs (reject or strip javascript: schemes) when creating the anchor for
event.website, and keep theme-related style insertion controlled (use a CSS
class toggle using currentMapTheme instead of injecting <style> with string
interpolation). Reference popupContainer, event.name, event.website and
currentMapTheme to locate and replace the innerHTML assignment with DOM-safe
construction.
| eventsLoaded = true; | ||
| }; | ||
|
|
||
| $: events?.length && mapLoaded && !eventsLoaded && initializeEvents(); |
There was a problem hiding this comment.
Loading can get stuck at 40% when no events are returned.
The reactive guard requires events.length, so initializeEvents() never runs for empty/error results, and mapLoading never reaches completion.
✅ Suggested fix
-$: events?.length && mapLoaded && !eventsLoaded && initializeEvents();
+$: if (mapLoaded && !eventsLoaded) {
+ initializeEvents();
+}📝 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.
Suggested change
| $: events?.length && mapLoaded && !eventsLoaded && initializeEvents(); | |
| $: if (mapLoaded && !eventsLoaded) { | |
| initializeEvents(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/events/map/`+page.svelte at line 92, The reactive guard currently
checks events?.length which prevents initializeEvents() from running when events
is an empty array or error result; update the reactive statement so it triggers
when events is defined (e.g., events != null or typeof events !== "undefined")
along with mapLoaded && !eventsLoaded, so initializeEvents() always runs even
for empty results and allows mapLoading to finish; update the reactive line
referencing events, mapLoaded, eventsLoaded, and initializeEvents accordingly.
Collaborator
Author
|
This PR is deliberately non-invasive, so the chances to affect the main map are minimal. Merging with the main map can be considered in the future, but I think it makes sense to have a separate sandbox until we know what we want and until we see more public interest and a core of active maintainers. Any feedback for the first version? Clustering would be an overkill with the current number of events, and we don't have any extra fields to display at the moment, so I guess translations is the only thing missing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit