Feature/group search#505
Merged
Merged
Conversation
Contributor
|
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:
WalkthroughRefactors group search to a map-centric, debounced filter-change flow: adds GroupMapSearchFilter component and bulk marker deduplication/offsetting in JS, removes paging in GetGroupsAsync, simplifies MapState (removes marker/address fields), and switches search triggering from form submit to OnFiltersChanged with debounce and request-id guarding. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant GroupSearch as GroupSearch (Blazor)
participant GroupMapFilter as GroupMapSearchFilter (Blazor)
participant Interop as LeafletInterop (JS)
participant Server as GroupSearchService
User->>GroupSearch: adjust filters (type/select)
GroupSearch->>GroupSearch: debounce (CancellationTokenSource)
GroupSearch-->>Server: LoadGroupsAsync(filter) [request-id]
Server-->>GroupSearch: GroupSlim[] (zip-code-based marker coords)
GroupSearch->>GroupMapFilter: set Groups parameter
GroupMapFilter->>Interop: addMarkers(allMarkers) (batch)
Interop-->>GroupMapFilter: markers added / click handlers bound
GroupMapFilter->>GroupSearch: (optional) GetMapStateAsync / RestoreMapStateAsync on init
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/web/Jordnaer/wwwroot/js/leaflet-interop.js (1)
461-515: Marker dedup and batch-add logic looks solid overall.The approach of collecting markers, offsetting co-located ones in a circle, and bulk-adding via
addLayersis a good improvement over incrementaladdLayercalls.Two minor observations:
Variable shadowing: The parameter name
groupon Line 500 shadows the outergroupfrom Line 464'sforEach. They're in separate scopes so it's not a bug, but renaming to e.g.bucketordupeswould improve readability.All duplicates are offset, including the first: Every marker in a duplicate set is moved away from the original coordinate—none remains at the true position. This is a valid approach for clickability, but if the intent is to keep one marker pinned at the real location, you'd start the loop at
i = 1.Optional: rename shadowed variable for clarity
- Object.values(byCoord).forEach(group => { - if (group.length <= 1) return; + Object.values(byCoord).forEach(bucket => { + if (bucket.length <= 1) return; const offsetMeters = 25; - const angleStep = (2 * Math.PI) / group.length; - group.forEach((marker, i) => { + const angleStep = (2 * Math.PI) / bucket.length; + bucket.forEach((marker, i) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/wwwroot/js/leaflet-interop.js` around lines 461 - 515, The duplicate-marker handling uses a nested variable name "group" that shadows the outer "group" from the marker creation loop and also offsets every marker in a duplicate set (so no marker remains at the true coordinate); rename the inner grouping variable (e.g., to "bucket" or "dupes") to avoid shadowing in the block that builds byCoord/Object.values, and change the duplicate-offset loop in the Object.values(byCoord).forEach so that if you want one marker to stay at the true position you skip index 0 (start the per-marker offset at i = 1) while still computing angleStep from group.length and applying offsets to the remaining items (ensure references to marker.getLatLng/marker.setLatLng and coordKey/byCoord remain consistent).src/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor (2)
130-149:MarkerLatitude/MarkerLongitudeare set to the view center, not an actual marker position.
viewStatefromGetMapStateAsyncreturns the map's center coordinates. Setting bothMarkerLatitude/MarkerLongitudeandViewLatitude/ViewLongitudeto the same values is redundant and the "Marker" naming is misleading. SinceRestoreMapStateAsync(Line 116-120) only readsViewLatitude,ViewLongitude,Zoom, andAddressText, theMarker*fields appear unused. Consider removing them fromMapStateif they serve no purpose, or populating them with actual marker data if they do.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor` around lines 130 - 149, SaveMapStateAsync is assigning the map center (viewState.Latitude/Longitude) to both ViewLatitude/ViewLongitude and MarkerLatitude/MarkerLongitude, which is misleading because RestoreMapStateAsync only uses ViewLatitude, ViewLongitude, Zoom, and AddressText; either remove the unused MarkerLatitude/MarkerLongitude from the MapState model or populate them with actual marker coordinates returned by GetMapStateAsync. Update MapState (remove MarkerLatitude/MarkerLongitude) and all usages, or change GetMapStateAsync/SaveMapStateAsync to return and store a separate marker position so MarkerLatitude/MarkerLongitude are meaningful, and ensure RestoreMapStateAsync and related code reference the updated MapState shape or marker fields accordingly.
125-128: Fire-and-forgetInvokeAsyncdiscards exceptions.
_ = OnFiltersChanged.InvokeAsync()silently swallows any exception thrown by the parent's handler. If the parent ever stops catching its own exceptions, errors will be lost.Consider awaiting it, which also lets Blazor's error-handling pipeline catch unhandled exceptions:
Suggested change
- private void OnFilterChanged() + private async Task OnFilterChanged() { - _ = OnFiltersChanged.InvokeAsync(); + await OnFiltersChanged.InvokeAsync(); }The callers on Lines 21 and 28 use inline lambdas that call
OnFilterChanged(). With theasync Taskreturn type, Blazor'sEventCallbackmachinery will properly await them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor` around lines 125 - 128, The OnFilterChanged method currently fire-and-forgets OnFiltersChanged.InvokeAsync(), which discards exceptions; change OnFilterChanged to an async Task method and await OnFiltersChanged.InvokeAsync() so Blazor will propagate exceptions via its pipeline. Update any callers (the inline lambdas that call OnFilterChanged) to handle the returned Task (they can remain as inline async lambdas or call the method directly) so the EventCallback machinery properly awaits the handler.src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor (2)
55-77: UnnecessaryToList()on every parameter change.
Groups?.ToList()on Line 62 materializes the collection every timeOnParametersSetAsyncruns, even whenGroupshasn't changed (the reference check on Line 72 will bail out, but the allocation already happened). Since the parent (GroupSearchForm) already materializes to aList<>, this is redundant work.Consider deferring materialization until after the change check:
Suggested optimization
protected override async Task OnParametersSetAsync() { if (_mapComponent?.IsInitialized != true) { return; } - var groupsList = Groups?.ToList(); - - if (groupsList is null || groupsList.Count == 0) + if (Groups is null || !Groups.Any()) { if (_previousGroups is not null) { _previousGroups = null; await _mapComponent.ClearGroupMarkersAsync(); } } else if (Groups != _previousGroups) { _previousGroups = Groups; - await _mapComponent.UpdateGroupMarkersAsync(groupsList); + await _mapComponent.UpdateGroupMarkersAsync(Groups); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 55 - 77, The method OnParametersSetAsync is unnecessarily calling Groups?.ToList() on every parameter update; avoid allocating until we know Groups changed by first comparing Groups to _previousGroups, and only then materialize into a list and call UpdateGroupMarkersAsync. Specifically, in OnParametersSetAsync, keep the early _mapComponent.IsInitialized check, then if Groups is null or empty handle clearing when _previousGroups is non-null (calling ClearGroupMarkersAsync and setting _previousGroups = null), else if Groups != _previousGroups set _previousGroups = Groups, then create a local list (materialize Groups to a List) and call _mapComponent.UpdateGroupMarkersAsync with that list; ensure you do not call ToList() before the equality check to avoid unnecessary allocations.
150-158: Polling for map initialization is pragmatic but fragile.The 20 × 50 ms loop is a reasonable workaround for JS interop timing, but if initialization ever takes longer (slow network, large tile sets), it silently proceeds with an uninitialized map. Consider logging a warning when the loop exhausts:
Optional enhancement
private async Task WaitForMapInitializationAsync() { var attempts = 0; while (_mapComponent?.IsInitialized != true && attempts < 20) { await Task.Delay(50); attempts++; } + + if (_mapComponent?.IsInitialized != true) + { + Console.WriteLine($"Warning: Map '{MapId}' did not initialize within {attempts * 50}ms"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 150 - 158, The polling loop in WaitForMapInitializationAsync can silently finish with an uninitialized map; after the while loop add a check for _mapComponent?.IsInitialized and, if still false, emit a warning mentioning the method name (WaitForMapInitializationAsync), the attempts count/timeout (attempts and 20×50ms) and any available context (e.g., component id) using the component's ILogger (or Console as fallback) so callers can see that initialization timed out instead of failing silently.
🤖 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/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 129-148: OnAddressSelected currently returns early on
null/whitespace input but never clears the backing field, so _addressText
remains stale for GetMapStateAsync; modify OnAddressSelected to set _addressText
= string.Empty (or null) when address is null/whitespace before returning (and
call StateHasChanged() if needed) so clearing the MudAutocomplete (Clearable)
actually resets the cached address used by GetMapStateAsync.
In `@src/web/Jordnaer/Pages/GroupSearch/GroupSearch.razor`:
- Around line 54-75: LoadGroupsAsync can leave the UI stuck because _isSearching
is set to false in the finally block without triggering a re-render; update the
finally block so when requestId == _activeRequestId you set _isSearching = false
and call StateHasChanged() to ensure MudLoading updates (this covers
cancellation and superseded background completions). Also fix the caching
inconsistency by storing the method parameter (filter) into Cache.SearchFilter
instead of the field _filter; keep assigning Cache.SearchResult = _searchResult
as before. References: LoadGroupsAsync, _isSearching, _activeRequestId,
StateHasChanged(), Cache.SearchFilter, _filter, _searchResult,
GroupSearchService.GetGroupsAsync.
---
Nitpick comments:
In `@src/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor`:
- Around line 130-149: SaveMapStateAsync is assigning the map center
(viewState.Latitude/Longitude) to both ViewLatitude/ViewLongitude and
MarkerLatitude/MarkerLongitude, which is misleading because RestoreMapStateAsync
only uses ViewLatitude, ViewLongitude, Zoom, and AddressText; either remove the
unused MarkerLatitude/MarkerLongitude from the MapState model or populate them
with actual marker coordinates returned by GetMapStateAsync. Update MapState
(remove MarkerLatitude/MarkerLongitude) and all usages, or change
GetMapStateAsync/SaveMapStateAsync to return and store a separate marker
position so MarkerLatitude/MarkerLongitude are meaningful, and ensure
RestoreMapStateAsync and related code reference the updated MapState shape or
marker fields accordingly.
- Around line 125-128: The OnFilterChanged method currently fire-and-forgets
OnFiltersChanged.InvokeAsync(), which discards exceptions; change
OnFilterChanged to an async Task method and await OnFiltersChanged.InvokeAsync()
so Blazor will propagate exceptions via its pipeline. Update any callers (the
inline lambdas that call OnFilterChanged) to handle the returned Task (they can
remain as inline async lambdas or call the method directly) so the EventCallback
machinery properly awaits the handler.
In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 55-77: The method OnParametersSetAsync is unnecessarily calling
Groups?.ToList() on every parameter update; avoid allocating until we know
Groups changed by first comparing Groups to _previousGroups, and only then
materialize into a list and call UpdateGroupMarkersAsync. Specifically, in
OnParametersSetAsync, keep the early _mapComponent.IsInitialized check, then if
Groups is null or empty handle clearing when _previousGroups is non-null
(calling ClearGroupMarkersAsync and setting _previousGroups = null), else if
Groups != _previousGroups set _previousGroups = Groups, then create a local list
(materialize Groups to a List) and call _mapComponent.UpdateGroupMarkersAsync
with that list; ensure you do not call ToList() before the equality check to
avoid unnecessary allocations.
- Around line 150-158: The polling loop in WaitForMapInitializationAsync can
silently finish with an uninitialized map; after the while loop add a check for
_mapComponent?.IsInitialized and, if still false, emit a warning mentioning the
method name (WaitForMapInitializationAsync), the attempts count/timeout
(attempts and 20×50ms) and any available context (e.g., component id) using the
component's ILogger (or Console as fallback) so callers can see that
initialization timed out instead of failing silently.
In `@src/web/Jordnaer/wwwroot/js/leaflet-interop.js`:
- Around line 461-515: The duplicate-marker handling uses a nested variable name
"group" that shadows the outer "group" from the marker creation loop and also
offsets every marker in a duplicate set (so no marker remains at the true
coordinate); rename the inner grouping variable (e.g., to "bucket" or "dupes")
to avoid shadowing in the block that builds byCoord/Object.values, and change
the duplicate-offset loop in the Object.values(byCoord).forEach so that if you
want one marker to stay at the true position you skip index 0 (start the
per-marker offset at i = 1) while still computing angleStep from group.length
and applying offsets to the remaining items (ensure references to
marker.getLatLng/marker.setLatLng and coordKey/byCoord remain consistent).
github-project-automation
Bot
moved this from Todo
to In Progress
in Jordnaer Community Website
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor (1)
96-104:WaitForMapInitializationAsyncsilently times out with no observable signal.If the map never initializes (JS error, slow client), the loop exits after 1 second and callers (
OnAfterRenderAsync,RestoreMapStateAsync) silently skip their work — markers aren't placed and state isn't restored. There's no log entry, exception, or returned bool to let callers distinguish success from timeout.Consider at minimum logging the timeout at
Warninglevel, or returning aboolso callers can skip/log:♻️ Suggested refactor
-private async Task WaitForMapInitializationAsync() +private async Task<bool> WaitForMapInitializationAsync() { var attempts = 0; while (_mapComponent?.IsInitialized != true && attempts < 20) { await Task.Delay(50); attempts++; } + return _mapComponent?.IsInitialized == true; }Then in callers:
-await WaitForMapInitializationAsync(); -if (_mapComponent?.IsInitialized == true) +if (await WaitForMapInitializationAsync()) { await _mapComponent!.CenterMapAsync(viewLatitude, viewLongitude, zoom); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 96 - 104, WaitForMapInitializationAsync currently spins up to ~1s and then silently gives up; change it to return a Task<bool> that returns true when _mapComponent?.IsInitialized becomes true and false on timeout (keep the attempts/Delay logic), and log a Warning when timing out (use the existing logger). Update callers (OnAfterRenderAsync and RestoreMapStateAsync) to await the new WaitForMapInitializationAsync and bail out or log if it returns false so markers/state restoration don't silently skip; reference the _mapComponent.IsInitialized check and the attempts timeout behavior when implementing.
🤖 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/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor`:
- Around line 80-81: The jitter uses Random.Shared.NextDouble(), causing
non-deterministic offsets and marker "jumping"; replace this with a stable
deterministic offset derived from the group's Id (e.g., use g.Id.GetHashCode()
or HashCode.Combine(g.Id) to produce an integer, convert to a deterministic
double in [0,1] (hash % largeNumber / (double)largeNumber), subtract 0.5 and
multiply by the existing scale factors) and use that value when computing
Latitude and Longitude instead of Random.Shared.NextDouble(); update the
assignments in GroupSearchForm.razor (the lines setting Latitude/Longitude that
reference ZipCodeLatitude/ZipCodeLongitude and Random.Shared.NextDouble()) to
use the new deterministic jitter function so positions remain consistent across
updates.
- Around line 119-122: The OnFilterChanged method currently fire-and-forgets
OnFiltersChanged.InvokeAsync(), which can hide exceptions; change the signature
of OnFilterChanged from void to async Task and await
OnFiltersChanged.InvokeAsync() (i.e., replace "_ =
OnFiltersChanged.InvokeAsync();" with "await OnFiltersChanged.InvokeAsync();")
so any exceptions bubble correctly and callers can await the Task; update any
Razor event bindings that call OnFilterChanged to handle an async Task handler
if necessary.
- Around line 45-46: The FilterChanged EventCallback on GroupSearchForm.razor is
declared but never invoked, breaking `@bind-Filter` semantics; either remove the
FilterChanged parameter or, preferably, invoke it after any mutation of the
GroupSearchFilter instance (like in search, reset, or toggle handlers) so
two-way binding completes. Update the component to call await
FilterChanged.InvokeAsync(Filter) immediately after every place you change
Filter (follow the pattern used in UserSearchForm.razor/PostSearchForm.razor) so
the parent receives updates; ensure you await the call where the methods are
async or return the Task when appropriate.
In `@src/web/Jordnaer/Pages/Groups/GroupDetails.razor`:
- Around line 220-225: The informational alert is incorrectly gated by
_group.Latitude/_group.Longitude; inside the branch where
string.IsNullOrEmpty(_group.Address) is false and _isMember is false the message
"Den præcise adresse vises kun for medlemmer" should always display. Remove the
coordinate guard around the MudAlert so the alert is rendered unconditionally
whenever a full address exists but the viewer is not a member (references:
_group.Address, _isMember, _group.Latitude/_group.Longitude,
ZipCodeLatitude/ZipCodeLongitude and the map render that follows).
---
Nitpick comments:
In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 96-104: WaitForMapInitializationAsync currently spins up to ~1s
and then silently gives up; change it to return a Task<bool> that returns true
when _mapComponent?.IsInitialized becomes true and false on timeout (keep the
attempts/Delay logic), and log a Warning when timing out (use the existing
logger). Update callers (OnAfterRenderAsync and RestoreMapStateAsync) to await
the new WaitForMapInitializationAsync and bail out or log if it returns false so
markers/state restoration don't silently skip; reference the
_mapComponent.IsInitialized check and the attempts timeout behavior when
implementing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor (2)
32-47: Minor inconsistency:Groupspassed directly on first render (Line 44) but materialized with.ToList()on parameter changes (Line 67).On Line 44 inside
OnAfterRenderAsync,Groupsis passed directly toUpdateGroupMarkersAsync, but on Line 67 insideOnParametersSetAsync, it's materialized with.ToList()first. Since the caller always provides aList<GroupMarkerData>, this doesn't cause a real issue, but consider being consistent — either always materialize or never.Also applies to: 49-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 32 - 47, On first render you pass Groups directly into _mapComponent.UpdateGroupMarkersAsync in OnAfterRenderAsync while OnParametersSetAsync materializes Groups with .ToList(); make this consistent by materializing Groups into a new list before usage in both methods (OnAfterRenderAsync and OnParametersSetAsync), assign that materialized list to _previousGroups and pass it to _mapComponent.UpdateGroupMarkersAsync so both code paths use the same List<GroupMarkerData> instance; update references to Groups in these areas (OnAfterRenderAsync, OnParametersSetAsync, _previousGroups, and _mapComponent.UpdateGroupMarkersAsync calls) accordingly.
56-56:Groups.Any()could enumerate a lazy source.
Groups.Any()on anIEnumerable<T>can trigger enumeration. Again, since the caller always passes aList, this is safe in practice, but using a pattern likeGroups is ICollection<GroupMarkerData> { Count: 0 }or materializing once would be more defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` at line 56, Change the null/empty check so it doesn't force enumerate a potentially lazy IEnumerable; replace the current condition using Groups.Any() in GroupMapSearchFilter.razor with a check that uses the collection Count when available (e.g. Groups is ICollection<GroupMarkerData> { Count: 0 }) or materialize once (e.g. assign Groups = Groups?.ToList() at the start) and then check Count/Count == 0, referencing the Groups property and GroupMarkerData type to locate the logic.
🤖 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/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor`:
- Around line 80-81: The current Latitude/Longitude jitter uses
Math.Abs(HashCode.Combine(g.Id, ...)) which can throw OverflowException when
HashCode.Combine returns int.MinValue; replace the Math.Abs usage in the
Latitude and Longitude assignments in GroupSearchForm.razor and instead clear
the sign bit on the HashCode.Combine result (e.g., use (HashCode.Combine(g.Id,
1) & 0x7FFFFFFF) and (HashCode.Combine(g.Id, 2) & 0x7FFFFFFF)) so you get a
non-negative int without risk of overflow, then proceed with the existing
modulus/division and offset math.
---
Duplicate comments:
In `@src/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor`:
- Around line 119-123: The previous fire-and-forget issue is already fixed:
ensure OnFilterChanged remains an async Task that awaits both
FilterChanged.InvokeAsync(Filter) and OnFiltersChanged.InvokeAsync(); no further
code changes are needed to OnFilterChanged, FilterChanged.InvokeAsync,
OnFiltersChanged.InvokeAsync, or the Filter parameter.
In `@src/web/Jordnaer/Pages/GroupSearch/GroupSearch.razor`:
- Around line 61-83: The previous issues in LoadGroupsAsync have been resolved:
ensure the method uses the incoming filter parameter for Cache.SearchFilter (not
the _filter field) and keep the StateHasChanged() call inside the finally block
guarded by the requestId == _activeRequestId check so _isSearching is reset only
for the active request; no code changes necessary—confirm the current
implementation of LoadGroupsAsync, Cache.SearchFilter, _activeRequestId, and
StateHasChanged are retained as shown.
---
Nitpick comments:
In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 32-47: On first render you pass Groups directly into
_mapComponent.UpdateGroupMarkersAsync in OnAfterRenderAsync while
OnParametersSetAsync materializes Groups with .ToList(); make this consistent by
materializing Groups into a new list before usage in both methods
(OnAfterRenderAsync and OnParametersSetAsync), assign that materialized list to
_previousGroups and pass it to _mapComponent.UpdateGroupMarkersAsync so both
code paths use the same List<GroupMarkerData> instance; update references to
Groups in these areas (OnAfterRenderAsync, OnParametersSetAsync,
_previousGroups, and _mapComponent.UpdateGroupMarkersAsync calls) accordingly.
- Line 56: Change the null/empty check so it doesn't force enumerate a
potentially lazy IEnumerable; replace the current condition using Groups.Any()
in GroupMapSearchFilter.razor with a check that uses the collection Count when
available (e.g. Groups is ICollection<GroupMarkerData> { Count: 0 }) or
materialize once (e.g. assign Groups = Groups?.ToList() at the start) and then
check Count/Count == 0, referencing the Groups property and GroupMarkerData type
to locate the logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor (2)
89-102:StateHasChanged()is unnecessary inRestoreMapStateAsync.
CenterMapAsyncis a JS interop call that updates the Leaflet map — it doesn't mutate any Blazor-rendered state. The trailingStateHasChanged()triggers a redundant render cycle with no visible effect.♻️ Proposed change
if (_mapComponent?.IsInitialized == true) { await _mapComponent.CenterMapAsync(viewLatitude, viewLongitude, zoom); } - - StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 89 - 102, The trailing StateHasChanged() in RestoreMapStateAsync causes an unnecessary Blazor render because CenterMapAsync is a JS interop call that does not change any Blazor-bound state; remove the StateHasChanged() call from RestoreMapStateAsync (keep the WaitForMapInitializationAsync check and the _mapComponent?.IsInitialized branch that calls CenterMapAsync) so the method only awaits CenterMapAsync and returns without forcing a redundant render.
104-122:WaitForMapInitializationAsyncuses time-based polling — consider an event-driven approach.The 20 × 50 ms loop (1 s cap) is fragile: slow devices or connections can exceed the timeout silently, leaving markers/state un-applied. A
TaskCompletionSourcecompleted when the map signals readiness is more robust and eliminates the arbitrary ceiling.♻️ Sketch of a TaskCompletionSource-based approach
+ private TaskCompletionSource _mapInitialized = new(TaskCreationOptions.RunContinuationsAsynchronously); + // Called by LeafletMap (or its wrapper) once JS initialization is complete: + public void OnMapInitialized() + { + _mapInitialized.TrySetResult(); + } - private async Task<bool> WaitForMapInitializationAsync() + private async Task<bool> WaitForMapInitializationAsync(int timeoutMs = 5000) { - var attempts = 0; - while (_mapComponent?.IsInitialized != true && attempts < 20) - { - await Task.Delay(50); - attempts++; - } - - if (_mapComponent?.IsInitialized != true) - { - Logger.LogWarning( - "WaitForMapInitializationAsync timed out after {Attempts} attempts ({Ms}ms). Map markers/state may not be restored.", - attempts, attempts * 50); - return false; - } - - return true; + var completed = await Task.WhenAny(_mapInitialized.Task, Task.Delay(timeoutMs)); + if (completed != _mapInitialized.Task) + { + Logger.LogWarning( + "WaitForMapInitializationAsync timed out after {Ms}ms. Map markers/state may not be restored.", + timeoutMs); + return false; + } + return true; }This requires the
LeafletMap(or its JS interop callback) to callOnMapInitialized()once it is ready, replacing theIsInitializedpolling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 104 - 122, WaitForMapInitializationAsync currently polls _mapComponent?.IsInitialized with a fixed attempts loop which can silently time out; replace polling with an event-driven TaskCompletionSource: add a private TaskCompletionSource<bool> field (e.g. _mapInitializedTcs), have the map JS interop or LeafletMap call a new public method OnMapInitialized() (or similar) that calls _mapInitializedTcs.TrySetResult(true), and change WaitForMapInitializationAsync to await _mapInitializedTcs.Task (with an optional short CancellationToken/timeout fallback to preserve current behavior); also ensure you recreate/reset _mapInitializedTcs when starting map setup so repeated initializations work and guard against nulls by returning false if the await times out.
🤖 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/web/Jordnaer/Components/Account/Pages/ForgotPassword.razor`:
- Around line 50-52: Remove embedding Input.Email in the reset callback URL in
ForgotPassword.razor: stop adding ["email"] = Input.Email to
NavigationManager.GetUriWithQueryParameters and instead only include the reset
token (code). Update ResetPassword.razor to resolve the user/email server-side
from the provided reset token using the UserManager API (e.g., find user from
token/validate token and pre-populate the email) so the email is not passed as a
plaintext query parameter; if you must keep a query param, add a comment
justifying the PII exposure.
In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 41-47: The bug is that _previousGroups is set to a new List
created by Groups.ToList(), so the reference equality check (Groups !=
_previousGroups) always passes and UpdateGroupMarkersAsync runs on every render;
change the assignment to store the original parameter reference (assign
_previousGroups = Groups) inside OnParametersSetAsync when
_mapComponent?.IsInitialized is true and Groups is not null, leaving the
ToList() only for the local markers variable passed to UpdateGroupMarkersAsync;
ensure all checks and the update call still use
_mapComponent.UpdateGroupMarkersAsync(markers) and that _previousGroups holds
the original Groups reference for correct guarding on subsequent calls.
- Around line 14-15: The MapId property in GroupMapSearchFilter.razor is
initialized using Guid.NewGuid(), causing different IDs between server prerender
and client hydration; remove the Guid.NewGuid() initializer and use a stable
deterministic default (e.g., "group-map-search") or require the parent to pass
MapId explicitly; update the MapId declaration (public string MapId { get; set;
} = "group-map-search"; or remove the default and let the parent set it) so the
element id is stable across prerender/hydration and matches the parent's `@key`
usage.
---
Duplicate comments:
In `@src/web/Jordnaer/Features/GroupSearch/GroupSearchForm.razor`:
- Around line 60-86: No code changes required: the ReferenceEquals guard in
OnParametersSet and the deterministic jitter using HashCode.Combine with the &
0x7FFFFFFF mask are correct; keep the existing logic and the assignment
_previousGroups = Groups as-is.
- Around line 45-46: The two-way binding is already fixed by awaiting
FilterChanged.InvokeAsync(Filter) alongside OnFiltersChanged.InvokeAsync(), but
there’s a duplicate approval/comment block and duplicated invocation region;
remove the duplicate lines at the other location (the repeated 119-123 block)
and keep the single awaited calls: FilterChanged.InvokeAsync(Filter) and
OnFiltersChanged.InvokeAsync(), ensuring the containing method remains async
Task so both awaits are preserved.
---
Nitpick comments:
In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 89-102: The trailing StateHasChanged() in RestoreMapStateAsync
causes an unnecessary Blazor render because CenterMapAsync is a JS interop call
that does not change any Blazor-bound state; remove the StateHasChanged() call
from RestoreMapStateAsync (keep the WaitForMapInitializationAsync check and the
_mapComponent?.IsInitialized branch that calls CenterMapAsync) so the method
only awaits CenterMapAsync and returns without forcing a redundant render.
- Around line 104-122: WaitForMapInitializationAsync currently polls
_mapComponent?.IsInitialized with a fixed attempts loop which can silently time
out; replace polling with an event-driven TaskCompletionSource: add a private
TaskCompletionSource<bool> field (e.g. _mapInitializedTcs), have the map JS
interop or LeafletMap call a new public method OnMapInitialized() (or similar)
that calls _mapInitializedTcs.TrySetResult(true), and change
WaitForMapInitializationAsync to await _mapInitializedTcs.Task (with an optional
short CancellationToken/timeout fallback to preserve current behavior); also
ensure you recreate/reset _mapInitializedTcs when starting map setup so repeated
initializations work and guard against nulls by returning false if the await
times out.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor (2)
96-99: RedundantIsInitializedguard afterWaitForMapInitializationAsync.
WaitForMapInitializationAsync()only returnstruewhen_mapComponent.IsInitialized == true, so the innerifon line 96 is always true in this branch and can be unwrapped.♻️ Proposed simplification
if (!await WaitForMapInitializationAsync()) { return; } - if (_mapComponent?.IsInitialized == true) - { - await _mapComponent.CenterMapAsync(viewLatitude, viewLongitude, zoom); - } + await _mapComponent!.CenterMapAsync(viewLatitude, viewLongitude, zoom); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 96 - 99, The inner IsInitialized check is redundant because WaitForMapInitializationAsync() only returns true when _mapComponent.IsInitialized is true; remove the if (_mapComponent?.IsInitialized == true) guard and directly call await _mapComponent.CenterMapAsync(viewLatitude, viewLongitude, zoom) after WaitForMapInitializationAsync() returns true, keeping references to _mapComponent, CenterMapAsync, WaitForMapInitializationAsync and IsInitialized to locate and update the code.
57-70: MaterializeGroupsonce to avoid double enumeration.
Groups.Any()(line 57) andGroups.ToList()(line 67) traverse the enumerable twice. IfGroupsis ever a deferred LINQ query, this causes two executions. Materialize upfront:♻️ Proposed refactor
- if (Groups is null || !Groups.Any()) + var groups = Groups?.ToList(); + if (groups is null || groups.Count == 0) { if (_previousGroups is not null) { _previousGroups = null; await _mapComponent.ClearGroupMarkersAsync(); } } else if (Groups != _previousGroups) { - var markers = Groups.ToList(); _previousGroups = Groups; - await _mapComponent.UpdateGroupMarkersAsync(markers); + await _mapComponent.UpdateGroupMarkersAsync(groups); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor` around lines 57 - 70, The code double-enumerates Groups via Groups.Any() and Groups.ToList(); materialize Groups once into a local variable (e.g., var markers = Groups?.ToList()) and use that for the emptiness check and for UpdateGroupMarkersAsync to avoid re-executing deferred queries; when markers is null or empty, clear _previousGroups and call _mapComponent.ClearGroupMarkersAsync(), otherwise set _previousGroups = markers (or a materialized copy) and call _mapComponent.UpdateGroupMarkersAsync(markers). Ensure all references to Groups in this block are replaced by the single materialized local to prevent multiple enumerations.
🤖 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/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 102-120: WaitForMapInitializationAsync can continue polling after
the component/circuit is disposed and then call JS interop on a disconnected
component; add a CancellationTokenSource field (e.g. _cts) and implement
IAsyncDisposable on the component, canceling _cts in DisposeAsync, then change
WaitForMapInitializationAsync to accept/observe a CancellationToken (throw or
return false when cancelled) and replace Task.Delay(50) with Task.Delay(50,
token); update all call sites (OnAfterRenderAsync, RestoreMapStateAsync, and any
calls before UpdateGroupMarkersAsync or CenterMapAsync) to pass _cts.Token so
the loop stops and no JS interop is invoked after disposal.
---
Nitpick comments:
In `@src/web/Jordnaer/Features/Map/GroupMapSearchFilter.razor`:
- Around line 96-99: The inner IsInitialized check is redundant because
WaitForMapInitializationAsync() only returns true when
_mapComponent.IsInitialized is true; remove the if (_mapComponent?.IsInitialized
== true) guard and directly call await
_mapComponent.CenterMapAsync(viewLatitude, viewLongitude, zoom) after
WaitForMapInitializationAsync() returns true, keeping references to
_mapComponent, CenterMapAsync, WaitForMapInitializationAsync and IsInitialized
to locate and update the code.
- Around line 57-70: The code double-enumerates Groups via Groups.Any() and
Groups.ToList(); materialize Groups once into a local variable (e.g., var
markers = Groups?.ToList()) and use that for the emptiness check and for
UpdateGroupMarkersAsync to avoid re-executing deferred queries; when markers is
null or empty, clear _previousGroups and call
_mapComponent.ClearGroupMarkersAsync(), otherwise set _previousGroups = markers
(or a materialized copy) and call
_mapComponent.UpdateGroupMarkersAsync(markers). Ensure all references to Groups
in this block are replaced by the single materialized local to prevent multiple
enumerations.
github-project-automation
Bot
moved this from In Progress
to Done
in Jordnaer Community Website
Merged
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.
No description provided.