Feature/group locations#498
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds Leaflet MarkerCluster integration: new cluster assets and CSS, JS interop for clustered group markers and map state, C# interop and Blazor APIs to update/clear/fit group markers and persist map view, and extends GroupSlim and result cache with geolocation and map-state fields. Changes
Sequence DiagramsequenceDiagram
participant Search as GroupSearch.razor
participant Form as GroupSearchForm
participant Filter as MapSearchFilter
participant LeafletMap as LeafletMap
participant Interop as LeafletMapInterop
participant JS as leaflet-interop.js
Search->>Form: Bind _searchResult.Groups
Form->>Form: Convert GroupSlim → GroupMarkerData[]
Form->>Filter: Pass Groups parameter
Filter->>LeafletMap: Call UpdateGroupMarkersAsync(groups)
LeafletMap->>Interop: Call UpdateGroupMarkersAsync(mapId, groups)
Interop->>JS: Call updateGroupMarkers(mapId, groups)
JS->>JS: createGroupIcon & createGroupPopupContent for each group
JS->>JS: Create/replace markerClusterGroup and add markers
Filter->>LeafletMap: Call FitBoundsToMarkersAsync()
LeafletMap->>Interop: Call FitBoundsToMarkersAsync(mapId, padding)
Interop->>JS: Call fitBoundsToMarkers(mapId, padding)
JS->>JS: Fit map bounds to cluster layer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
Review Summary by QodoImplement group location markers with clustering on map
WalkthroughsDescription• Add group marker clustering and visualization on map • Implement custom marker icons with profile pictures • Create interactive popups displaying group information • Add marker cluster styling with size-based color coding Diagramflowchart LR
A["GroupSearchForm"] -->|Groups parameter| B["MapSearchFilter"]
B -->|UpdateGroupMarkersAsync| C["LeafletMap"]
C -->|JS interop| D["leaflet-interop.js"]
D -->|Creates markers| E["Leaflet.markercluster"]
E -->|Renders with styling| F["marker-cluster.css"]
D -->|Displays popups| G["Group Popups"]
File Changes1. src/web/Jordnaer/Features/Map/LeafletMapInterop.cs
|
![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
🤖 Fix all issues with AI agents
In `@src/web/Jordnaer/wwwroot/css/marker-cluster.css`:
- Around line 80-87: The inner div for large clusters is offset because the JS
creates an iconSize of 44×44 while .marker-cluster-large div is 42×42 but uses
negative margins; update the .marker-cluster-large div CSS (selector
.marker-cluster-large div) to use margin-left: 1px and margin-top: 1px (i.e.,
(44−42)/2) so the 42×42 inner circle is centered within the 44×44 icon;
alternatively ensure the inner div size matches iconSize or adjust both
consistently if you prefer a different final visual.
🧹 Nitpick comments (7)
src/web/Jordnaer/wwwroot/js/leaflet-interop.js (3)
9-10:primaryColoris defined but never referenced; color is still hardcoded.
primaryColoron Line 10 is unused —updateSearchRadius(Lines 97-98) still hardcodes'#594F8D'. Either usethis.primaryColorinupdateSearchRadiusor remove the property.♻️ Suggested fix
mapInstance.circle = L.circle([lat, lng], { - color: '#594F8D', // Primary color from Jordnaer theme - fillColor: '#594F8D', + color: this.primaryColor, + fillColor: this.primaryColor, fillOpacity: 0.15, radius: radiusMeters }).addTo(mapInstance.map);Also applies to: 97-98
321-334:groupUrlis not escaped for HTML attribute context.
encodeURIComponentencodes"as%22so this is safe in practice, but for defense-in-depth consistency with the rest of the file, consider running it throughescapeAttribute:- const groupUrl = `/groups/${encodeURIComponent(group.name)}`; + const groupUrl = this.escapeAttribute(`/groups/${encodeURIComponent(group.name)}`);
386-450: Emptygroupsarray still allocates a cluster group and adds it to the map.When
groupsisnull/empty, the code still creates a newL.markerClusterGroup, adds zero layers, and attaches it to the map (Line 450). This is harmless but wasteful — consider returning early after clearing the old cluster.src/web/Jordnaer/wwwroot/css/marker-cluster.css (1)
6-28: CSS custom properties duplicate the primary color defined elsewhere.The
--jordnaer-primary:#594F8D`` and other theme colors here may already exist inapp.cssor other global stylesheets. If the primary color changes, multiple `:root` blocks must be updated.Not blocking — just a maintenance note to consider centralizing these variables if they aren't already shared.
src/web/Jordnaer/Features/Map/MapSearchFilter.razor (2)
319-343:Groups(IEnumerable) is enumerated multiple times:.Any()+UpdateGroupMarkersAsync.Line 325 calls
Groups.Any()(first enumeration), and Line 337 passesGroupstoUpdateGroupMarkersAsync(second enumeration for JSON serialization). This is safe today because the upstreamGroupSearchFormmaterializes it as aList<GroupMarkerData>, but theIEnumerable<GroupMarkerData>?parameter type doesn't enforce that.Consider accepting
IReadOnlyList<GroupMarkerData>?instead, or materializing locally, to make the contract explicit and prevent future callers from passing deferred queries.
296-317: PublicUpdateGroupMarkersAsyncandClearGroupMarkersAsync— note these bypass_previousGroupstracking.Callers using these public methods directly won't update
_previousGroups, so a subsequentOnParametersSetAsynccycle could re-push or clear markers unexpectedly if theGroupsparameter state drifts from what was imperatively set. This is fine if the public methods are only used for initial setup (as inOnAfterRenderAsync), but worth being aware of if they're called from parent components.src/web/Jordnaer/Features/Map/LeafletMapInterop.cs (1)
137-141: Null-guard is inconsistent with the non-nullable parameter type.The interface declares
groupsasIEnumerable<GroupMarkerData>(non-nullable), yet the implementation appliesgroups?.ToList() ?? [], silently converting a null into an empty list. This masks potential caller bugs. Consider either:
- Making the parameter nullable (
IEnumerable<GroupMarkerData>?) if null is a valid input, or- Dropping the null-conditional and just calling
groups.ToList()to let NRE surface misuse.Option: trust the non-nullable contract
public async Task<bool> UpdateGroupMarkersAsync(string mapId, IEnumerable<GroupMarkerData> groups) { - var materialized = groups?.ToList() ?? []; + var materialized = groups.ToList(); return await _jsRuntime.InvokeVoidAsyncWithErrorHandling( "leafletInterop.updateGroupMarkers", mapId, materialized); }
github-project-automation
Bot
moved this from Todo
to In Progress
in Jordnaer Community Website
Code Review by Qodo
✅
📘 Rule violation ⛯ Reliability |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…gaard/Jordnaer into feature/group-locations
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/web/Jordnaer/wwwroot/js/leaflet-interop.js`:
- Around line 316-327: The group.websiteUrl is currently inserted into
websiteHtml using this.escapeAttribute/escapeHtml but that doesn't block
javascript: or other dangerous schemes; update the popup-building logic that
sets websiteHtml to first validate/parsel the URL (using the URL constructor or
equivalent) and only render the anchor if the parsed protocol is "http:" or
"https:" (otherwise skip rendering the link or render as plain text). Locate the
block that reads group.websiteUrl and the use of
this.escapeAttribute/this.escapeHtml and add a guard that checks new
URL(group.websiteUrl).protocol for "http:" or "https:" before including the <a
href=...> element.
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/Map/MapSearchFilter.razor (1)
319-340: Potential multiple enumeration ofGroups(IEnumerable).Line 325 calls
Groups.Any(), and if the branch on line 337 is taken,Groupsis enumerated again insideUpdateGroupMarkersAsync(which calls.ToList()in the interop layer). Since the upstream caller (GroupSearchForm) materializes to aList<GroupMarkerData>, this is safe today, but the parameter typeIEnumerable<GroupMarkerData>?doesn't enforce that.A lightweight guard would be to check
_previousGroupsfirst to short-circuit more cheaply:Suggested reorder
if (_mapComponent?.IsInitialized == true) { - if (Groups is null || !Groups.Any()) + if (Groups is null or []) { if (_previousGroups is not null) { _previousGroups = null; await _mapComponent.ClearGroupMarkersAsync(); } } - else if (Groups != _previousGroups) + else if (!ReferenceEquals(Groups, _previousGroups)) { _previousGroups = Groups; await _mapComponent.UpdateGroupMarkersAsync(Groups); } }Using
ReferenceEqualsmakes the intent explicit (matchingGroupSearchForm.OnParametersSet), and list pattern[])avoids enumerating an arbitraryIEnumerablewhile still working onList<T>.src/web/Jordnaer/wwwroot/js/leaflet-interop.js (1)
258-276: Samejavascript:URI concern applies toprofilePictureUrlin<img src="...">.While
<img src="javascript:...">is not exploitable in modern browsers,data:URIs insrccan render unexpected content. Consider applying the samehttps?://scheme check toprofilePictureUrlfor defense in depth, or validate URLs server-side on save.
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)
src/web/Jordnaer/Features/Map/MapSearchFilter.razor (1)
98-128:⚠️ Potential issue | 🟠 MajorRemove the dead
_skipGeolocationfield and consolidate geolocation skip logic.The private field
_skipGeolocationis never set totrueanywhere in the file, making the check!_skipGeolocationon line 115 always evaluate totrueand thus dead code. The field is initialized tofalse(line 57), checked at line 115, and only reset tofalseat line 291. This overlaps with theSkipGeolocationparameter which already controls whether geolocation should be attempted. Remove the private field and rely solely on theSkipGeolocationparameter.
🤖 Fix all issues with AI agents
In `@src/web/Jordnaer/Pages/GroupSearch/GroupSearch.razor`:
- Around line 114-128: Replace the eval-based interop in
ClearScrollPositionAsync: instead of calling JsRuntime.InvokeVoidAsync("eval",
...) create a named JS function (e.g.
window.scrollHelper.clearGroupSearchScroll) that performs
sessionStorage.removeItem for 'groupSearch:scrollY' and
'groupSearch:visibleItemId' and window.scrollTo({ top: 0, behavior: 'instant'
}); then call it from ClearScrollPositionAsync using JsRuntime.InvokeVoidAsync
with the function identifier (e.g. "scrollHelper.clearGroupSearchScroll"); keep
the existing try/catch behavior for JS interop errors if desired.
🧹 Nitpick comments (4)
src/web/Jordnaer/Pages/GroupSearch/GroupSearch.razor (1)
57-63: Redundant else branch — fields already initialized at declaration.
_filter,_searchResult, and_hasSearchedare already initialized to the same default values on lines 42–46. Thiselseblock is a no-op.Proposed cleanup
if (Cache.SearchFilter is not null && Cache.SearchResult is not null) { _filter = Cache.SearchFilter; _searchResult = Cache.SearchResult; _hasSearched = true; } - else - { - // Ensure clean state if cache is empty - _filter = new GroupSearchFilter(); - _searchResult = new GroupSearchResult(); - _hasSearched = false; - }src/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor (1)
149-181:Task.Delay(100)is fragile for ensuring child component rendering.The hardcoded 100ms delay on line 154 is a race condition — it may be insufficient on slow devices/connections and wasteful on fast ones. Consider using the
OnMapInitializedcallback fromLeafletMapto signal readiness instead of guessing with a delay, or at minimum rely on the polling loop already present inMapSearchFilter.OnAfterRenderAsync.src/web/Jordnaer/Features/Map/MapSearchFilter.razor (2)
383-404: Potential multiple enumeration ofIEnumerable<GroupMarkerData>.Line 389 calls
Groups.Any()and line 401 passesGroupstoUpdateGroupMarkersAsync, which could enumerate the sequence twice if the caller passes a lazyIEnumerable. Currently safe becauseGroupSearchFormmaterializes to aList, but this is fragile if other callers are added.Defensive materialization
Consider accepting
IReadOnlyList<GroupMarkerData>?for theGroupsparameter, or materializing once inOnParametersSetAsync.
347-381: Map initialization polling loop is duplicated.The
while (_mapComponent?.IsInitialized != true && attempts < 20)pattern appears both inOnAfterRenderAsync(line 106) andRestoreMapStateAsync(line 362). Consider extracting aWaitForMapInitializationAsync()helper.
github-project-automation
Bot
moved this from In Progress
to Done
in Jordnaer Community Website
1 participant
No description provided.