Feature/hjemlo#507
Merged
Merged
Conversation
Contributor
|
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. 📒 Files selected for processing (3)
WalkthroughAdds a HJEM groups feature: models, blob-backed admin service with geocoding, a provider that maps/caches markers, backoffice CRUD UI, GroupSearch integration to merge additional markers, a DataForsyningen zip-code search client method, DI registration, UI/CSS/JS tweaks, and unit/integration tests. Changes
Sequence DiagramssequenceDiagram
participant Page as GroupSearch.razor
participant Provider as IHjemGroupProvider
participant Blob as Azure Blob Storage
participant Form as GroupSearchForm.razor
participant Map as MapComponent
Page->>Provider: GetMarkersAsync()
Provider->>Blob: Read container "hjemlo-groups"/blob "groups.json"
Blob-->>Provider: JSON / NotFound / Error
Provider->>Provider: Deserialize -> List<HjemGroupEntry]
Provider->>Provider: Map entries -> GroupMarkerData (MD5->GUID)
Provider-->>Page: OneOf<IReadOnlyList<GroupMarkerData>, Error>
Page->>Form: Pass AdditionalMarkers
Form->>Form: Filter by Name, merge AdditionalMarkers + regular markers
Form->>Map: Provide combined markers
Map-->>Page: Render markers
sequenceDiagram
participant UI as HjemGroupManagementPage
participant Admin as HjemGroupAdminService
participant Blob as Azure Blob Storage
participant API as IDataForsyningenClient
participant EventBus as IPublishEndpoint
UI->>Admin: LoadAsync()
Admin->>Blob: Get blob "groups.json"
Blob-->>Admin: Content / NotFound / Error
Admin-->>UI: List<HjemGroupEntry> or HjemGroupLoadError
UI->>Admin: GeocodeAsync(locationText)
Admin->>API: SearchZipCodesAsync(query)
API-->>Admin: ZipCodeSearchResponse[]
Admin->>Admin: Extract city, zip, coordinates
Admin-->>UI: GeocodeResult or null
UI->>Admin: SaveAsync(entries)
Admin->>Blob: Create container if needed
Admin->>Blob: Upload "groups.json" (camelCase, overwrite)
Blob-->>Admin: Confirm upload
Admin->>EventBus: Publish cache invalidation
Admin-->>UI: Success or HjemGroupSaveError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ 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: 5
🧹 Nitpick comments (7)
tests/web/Jordnaer.Tests/UserSearch/DataForsyningenClientTests.cs (1)
79-79: Inconsistent assertion:IsSuccessStatusCodevsIsSuccessful.The new tests use
IsSuccessStatusCode(Lines 79, 99, 113) while the existing tests on Lines 26, 40, 54, 65 useIsSuccessful.IsSuccessfulonIApiResponseadditionally verifies deserialization succeeded, making it the stricter (and more useful) check. Consider aligning with the existing pattern for consistency.Suggested fix
- response.IsSuccessStatusCode.Should().BeTrue(); + response.IsSuccessful.Should().BeTrue();(Apply in all three new tests.)
Also applies to: 99-99, 113-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/UserSearch/DataForsyningenClientTests.cs` at line 79, The tests added in UserSearch/DataForsyningenClientTests.cs assert response.IsSuccessStatusCode but the project uses the stricter IApiResponse.IsSuccessful pattern; update the three new assertions (the ones currently checking IsSuccessStatusCode) to assert response.IsSuccessful instead so deserialization success is also validated, keeping behavior consistent with the existing tests that check IsSuccessful on IApiResponse.src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs (1)
28-68: Cache invalidation concern if lifetime changes to Singleton.If
HjemGroupProvideris promoted to Singleton (as suggested in the DI registration review), note that Lines 39 and 47 cache an empty list when the container/blob doesn't exist. Once cached, new data uploaded to blob storage will never be picked up for the lifetime of the application.Consider adding a TTL-based invalidation or exposing a cache-reset method for the admin service to call after saving new data.
Sketch: nullable timestamp-based TTL
+ private DateTime? _cachedAt; + private static readonly TimeSpan CacheDuration = TimeSpan.FromMinutes(15); + public async Task<IReadOnlyList<GroupMarkerData>> GetMarkersAsync(CancellationToken cancellationToken = default) { - if (_cached is not null) + if (_cached is not null && _cachedAt.HasValue && DateTime.UtcNow - _cachedAt.Value < CacheDuration) return _cached;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs` around lines 28 - 68, GetMarkersAsync currently stores an empty list into the provider-level field _cached when the blob container or blob is missing, which will permanently hide newly uploaded data if HjemGroupProvider becomes a singleton; change the caching strategy in HjemGroupProvider by either (A) adding a timestamp (e.g., _cacheTimestamp) and TTL check in GetMarkersAsync to refresh _cached when older than the configured TTL before returning, or (B) add a public InvalidateCache() / ResetCache() method that clears _cached (and _cacheTimestamp if present) so the admin service can call it after saving new data; keep the existing blob checks (ContainerName/BlobName) but ensure GetMarkersAsync re-fetches when the cache is expired or explicitly invalidated.tests/web/Jordnaer.Tests/HjemGroups/HjemGroupIntegrationTests.cs (1)
182-197:act()is invoked twice — once byNotThrow()and once explicitly.
act.Should().NotThrow()already invokes the delegate. The secondact()!invokes it again. This is harmless for string deserialization but is redundant work. Consider capturing the result from the first invocation.♻️ Proposed fix
- var act = () => JsonSerializer.Deserialize<List<HjemGroupEntry>>(json, CamelCase); - act.Should().NotThrow(); - act()!.Should().NotBeEmpty(); + var entries = JsonSerializer.Deserialize<List<HjemGroupEntry>>(json, CamelCase); + entries.Should().NotBeNull(); + entries!.Should().NotBeEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupIntegrationTests.cs` around lines 182 - 197, In FullPipeline_BlobContainsValidJson replace the double-invocation of the delegate 'act' by invoking JsonSerializer.Deserialize once: call the delegate (the expression that uses JsonSerializer.Deserialize with CamelCase) and store its result in a variable, assert that invoking the delegate does not throw (use act.Should().NotThrow()) if you must keep the delegate, or better, directly perform the deserialize into a local variable and assert the result is not null/empty (use the stored result for the subsequent Should().NotBeEmpty() check) so JsonSerializer.Deserialize is only executed a single time.tests/web/Jordnaer.Tests/HjemGroups/HjemGroupScraperServiceTests.cs (2)
253-277: Sameact()double-invocation pattern as in integration tests.
act.Should().NotThrow()invokes the delegate, thenact()!invokes it again. Consider capturing the result directly.♻️ Proposed fix
- var act = () => JsonSerializer.Deserialize<List<HjemGroupEntry>>( - uploadedJson, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); - act.Should().NotThrow(); - act()!.Should().NotBeEmpty(); + var entries = JsonSerializer.Deserialize<List<HjemGroupEntry>>( + uploadedJson, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); + entries.Should().NotBeNull(); + entries!.Should().NotBeEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupScraperServiceTests.cs` around lines 253 - 277, The test currently invokes the deserialization delegate twice (act.Should().NotThrow() then act()!), which duplicates work and can mask issues; replace the delegate pattern by performing the deserialization once into a local variable using JsonSerializer.Deserialize on uploadedJson (referencing JsonSerializer.Deserialize and uploadedJson from the test), then assert the deserialization succeeded (result is not null) and that the result collection is not empty; update the assertions in ScrapeAndSaveAsync_SavesValidJson_ToBlob to use the single captured variable instead of calling act() twice.
107-118:CaptureUploadedJson()is dead code with a latent bug.This helper is never called — all tests that capture uploaded JSON do their own inline
Arg.Dosetup. Additionally, if it were used, it would always returnstring.Emptybecausecapturedis reassigned inside theArg.Dolambda (after the method returns), but the caller holds a reference to the original immutable empty string.Consider removing it or, if you intend to use it, switching to a wrapper class so the reference is shared:
♻️ Option A: Remove dead code
- // Captures the uploaded JSON from the blob upload call - private string CaptureUploadedJson() - { - var captured = string.Empty; - _blobClient - .UploadAsync(Arg.Do<Stream>(s => - { - using var sr = new StreamReader(s, Encoding.UTF8); - captured = sr.ReadToEnd(); - }), Arg.Any<bool>(), Arg.Any<CancellationToken>()) - .Returns(Task.FromResult(Substitute.For<Response<BlobContentInfo>>())); - return captured; // note: populated after the act - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupScraperServiceTests.cs` around lines 107 - 118, The helper CaptureUploadedJson is dead/buggy: tests never call it and its captured variable is assigned inside the Arg.Do lambda after the method returns (so it always returns the initial empty string). Either delete CaptureUploadedJson entirely and consolidate UploadAsync Arg.Do setups inline, or convert it to return/use a mutable wrapper (e.g., a small class with a string property) and have CaptureUploadedJson register the Arg.Do on _blobClient.UploadAsync so the wrapper instance is mutated inside the lambda and can be inspected after the act; reference CaptureUploadedJson, _blobClient and UploadAsync when applying the change.tests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs (1)
85-120: Extract repeatedJsonSerializerOptionsto a shared static field.
new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }is duplicated in nearly every test. A private static field (like the integration tests do withCamelCase) would reduce noise.♻️ Proposed fix
Add a static field at the class level:
public class HjemGroupProviderTests { + private static readonly JsonSerializerOptions CamelCase = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + private readonly BlobServiceClient _blobServiceClient = Substitute.For<BlobServiceClient>();Then replace all inline usages, e.g.:
- SetupBlobWithContent(JsonSerializer.Serialize(entries, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase })); + SetupBlobWithContent(JsonSerializer.Serialize(entries, CamelCase));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs` around lines 85 - 120, Extract the repeated JsonSerializerOptions into a single shared static field in the HjemGroupProviderTests class and use it everywhere; specifically add a private static readonly JsonSerializerOptions CamelCase = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase } in the HjemGroupProviderTests class and replace inline constructions like new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase } (used in SetupBlobWithContent(...) and other test serialization calls inside GetMarkersAsync_ReturnsMappedMarkers_ForLokalafdeling and other tests) with the CamelCase field.src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs (1)
23-44:LoadAsyncsilently swallows all exceptions.The coding guidelines for
src/web/Jordnaer/Features/**/*.csrecommend usingOneOf<TSuccess, TError>for explicit error handling. Currently, callers cannot distinguish between "no data exists" and "load failed." Given the chill review mode, this is a recommended refactor that can be addressed later.As per coding guidelines:
src/web/Jordnaer/Features/**/*.cs: UseOneOf<TSuccess, TError>for explicit error handling in service methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs` around lines 23 - 44, Change LoadAsync to return Task<OneOf<List<HjemGroupEntry>, HjemGroupLoadError>> instead of silently swallowing failures: update the signature of LoadAsync and all callers to use OneOf; treat "no data exists" (container/blob not found) as a successful result returning an empty List<HjemGroupEntry>, but on exceptions return the HjemGroupLoadError variant (include the caught Exception and a descriptive message) instead of returning an empty list; keep logging via logger.LogWarning(ex, ...) but also populate the error object so callers can distinguish load failures from empty data; use the existing symbols (LoadAsync, blobServiceClient, ContainerName, BlobName, HjemGroupEntry) to find and update the implementation and call sites.
🤖 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/HjemGroups/HjemGroupAdminService.cs`:
- Around line 46-56: The container is being created with PublicAccessType.Blob
which grants unnecessary public read access; update the CreateIfNotExistsAsync
call in SaveAsync (the blobServiceClient -> containerClient creation flow using
ContainerName and BlobName) to use PublicAccessType.None so the container is not
publicly readable, retaining server-side-only access for HjemGroupProvider's
JSON blob handling.
In `@src/web/Jordnaer/Features/HjemGroups/WebApplicationBuilderExtensions.cs`:
- Line 7: The provider is registered with AddScoped which creates a new
HjemGroupProvider per request so its _cached field is always null; change the DI
registration from builder.Services.AddScoped<IHjemGroupProvider,
HjemGroupProvider>() to a singleton registration
(builder.Services.AddSingleton<IHjemGroupProvider, HjemGroupProvider>), or
alternatively refactor HjemGroupProvider to use IMemoryCache/FusionCache inside
GetMarkersAsync (inject IMemoryCache or FusionCache, store/retrieve the data
with a TTL) so the cache survives across requests; keep BlobServiceClient as
Singleton and ensure GetMarkersAsync uses the chosen shared cache rather than an
instance field.
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 236-273: CommitEdit currently saves entries without validating
coordinates, causing default (0,0) markers; add validation in CommitEdit to
ensure _form.Latitude and _form.Longitude are present and within valid ranges
(latitude between -90 and 90, longitude between -180 and 180) and reject or warn
when both are zero (or when either is null/invalid) by calling Snackbar.Add and
returning early; only construct and add/update the HjemGroupEntry and modify
_entries when coordinates are valid so markers won't be created at the Gulf of
Guinea.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupIntegrationTests.cs`:
- Around line 42-48: The tests reference a missing implementation class
HjemGroupScraperService which causes the build to fail; add the
HjemGroupScraperService class under the HjemGroups feature implementing the
behavior the tests expect (constructor signature:
HjemGroupScraperService(HttpClient, IDataForsyningenClient,
BlobServiceClient/BlobServiceClient-like _blobServiceClient,
ILogger<HjemGroupScraperService>) and the methods used by
HjemGroupIntegrationTests and HjemGroupScraperServiceTests), ensure its public
API matches how CreateScraper constructs it (accepts HttpClient and
IDataForsyningenClient and uses the provided logger and blob client), and
implement or stub the scraping and geocoding flows referenced by the tests so
the tests can compile and run.
In `@tests/web/Jordnaer.Tests/UserSearch/DataForsyningenClientTests.cs`:
- Around line 112-118: Add a null-guard assertion for the HTTP response content
before accessing it: in the test in DataForsyningenClientTests.cs that uses
response.Content!.First(), call response.Content.Should().NotBeNull() (like the
earlier test) immediately after asserting response.IsSuccessStatusCode to ensure
response.Content is non-null before using response.Content.First(),
response.Content![0], and response.Content![1].
---
Duplicate comments:
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupScraperServiceTests.cs`:
- Line 23: The tests reference a missing type HjemGroupScraperService causing
build failures; either update the tests to use the actual service type or add
the missing implementation/reference. Locate the usages of
HjemGroupScraperService in the test classes (e.g., HjemGroupScraperService field
in HjemGroupScraperServiceTests and similar references in
HjemGroupIntegrationTests) and fix by one of: 1) change the test to depend on
the correct existing class or interface name used by the production code, 2) add
the project/reference that contains HjemGroupScraperService to the test project,
or 3) create a minimal test-double (stub) class named HjemGroupScraperService in
the test assembly to satisfy compilation; ensure namespaces and using directives
match the tests.
---
Nitpick comments:
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs`:
- Around line 23-44: Change LoadAsync to return Task<OneOf<List<HjemGroupEntry>,
HjemGroupLoadError>> instead of silently swallowing failures: update the
signature of LoadAsync and all callers to use OneOf; treat "no data exists"
(container/blob not found) as a successful result returning an empty
List<HjemGroupEntry>, but on exceptions return the HjemGroupLoadError variant
(include the caught Exception and a descriptive message) instead of returning an
empty list; keep logging via logger.LogWarning(ex, ...) but also populate the
error object so callers can distinguish load failures from empty data; use the
existing symbols (LoadAsync, blobServiceClient, ContainerName, BlobName,
HjemGroupEntry) to find and update the implementation and call sites.
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs`:
- Around line 28-68: GetMarkersAsync currently stores an empty list into the
provider-level field _cached when the blob container or blob is missing, which
will permanently hide newly uploaded data if HjemGroupProvider becomes a
singleton; change the caching strategy in HjemGroupProvider by either (A) adding
a timestamp (e.g., _cacheTimestamp) and TTL check in GetMarkersAsync to refresh
_cached when older than the configured TTL before returning, or (B) add a public
InvalidateCache() / ResetCache() method that clears _cached (and _cacheTimestamp
if present) so the admin service can call it after saving new data; keep the
existing blob checks (ContainerName/BlobName) but ensure GetMarkersAsync
re-fetches when the cache is expired or explicitly invalidated.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupIntegrationTests.cs`:
- Around line 182-197: In FullPipeline_BlobContainsValidJson replace the
double-invocation of the delegate 'act' by invoking JsonSerializer.Deserialize
once: call the delegate (the expression that uses JsonSerializer.Deserialize
with CamelCase) and store its result in a variable, assert that invoking the
delegate does not throw (use act.Should().NotThrow()) if you must keep the
delegate, or better, directly perform the deserialize into a local variable and
assert the result is not null/empty (use the stored result for the subsequent
Should().NotBeEmpty() check) so JsonSerializer.Deserialize is only executed a
single time.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs`:
- Around line 85-120: Extract the repeated JsonSerializerOptions into a single
shared static field in the HjemGroupProviderTests class and use it everywhere;
specifically add a private static readonly JsonSerializerOptions CamelCase = new
JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase } in
the HjemGroupProviderTests class and replace inline constructions like new
JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }
(used in SetupBlobWithContent(...) and other test serialization calls inside
GetMarkersAsync_ReturnsMappedMarkers_ForLokalafdeling and other tests) with the
CamelCase field.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupScraperServiceTests.cs`:
- Around line 253-277: The test currently invokes the deserialization delegate
twice (act.Should().NotThrow() then act()!), which duplicates work and can mask
issues; replace the delegate pattern by performing the deserialization once into
a local variable using JsonSerializer.Deserialize on uploadedJson (referencing
JsonSerializer.Deserialize and uploadedJson from the test), then assert the
deserialization succeeded (result is not null) and that the result collection is
not empty; update the assertions in ScrapeAndSaveAsync_SavesValidJson_ToBlob to
use the single captured variable instead of calling act() twice.
- Around line 107-118: The helper CaptureUploadedJson is dead/buggy: tests never
call it and its captured variable is assigned inside the Arg.Do lambda after the
method returns (so it always returns the initial empty string). Either delete
CaptureUploadedJson entirely and consolidate UploadAsync Arg.Do setups inline,
or convert it to return/use a mutable wrapper (e.g., a small class with a string
property) and have CaptureUploadedJson register the Arg.Do on
_blobClient.UploadAsync so the wrapper instance is mutated inside the lambda and
can be inspected after the act; reference CaptureUploadedJson, _blobClient and
UploadAsync when applying the change.
In `@tests/web/Jordnaer.Tests/UserSearch/DataForsyningenClientTests.cs`:
- Line 79: The tests added in UserSearch/DataForsyningenClientTests.cs assert
response.IsSuccessStatusCode but the project uses the stricter
IApiResponse.IsSuccessful pattern; update the three new assertions (the ones
currently checking IsSuccessStatusCode) to assert response.IsSuccessful instead
so deserialization success is also validated, keeping behavior consistent with
the existing tests that check IsSuccessful on IApiResponse.
github-project-automation
Bot
moved this from Todo
to In Progress
in Jordnaer Community Website
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs (2)
155-160:StreamReaderdisposes the captured stream; preferleaveOpen: trueIn both
Arg.Docallbacks,new StreamReader(s, Encoding.UTF8)disposesswhen theusingblock exits. NSubstitute runs the callback synchronously before returning, so the stream the service passed in is already disposed. ForMemoryStreama double-dispose is harmless, but this is fragile if the production code ever switches to a different stream type or inspects the stream afterUploadAsyncreturns.♻️ Proposed fix — use `leaveOpen: true` in both callbacks
- using var sr = new StreamReader(s, Encoding.UTF8); + using var sr = new StreamReader(s, Encoding.UTF8, detectEncodingFromByteOrderMarks: false, bufferSize: -1, leaveOpen: true);Apply the same change at line 178.
Also applies to: 177-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs` around lines 155 - 160, The StreamReader in the Arg.Do<Stream> callback used to capture uploadedJson disposes the input stream; change the StreamReader construction to pass leaveOpen: true so the captured stream isn't closed (update the StreamReader in the UploadAsync Arg.Do<Stream> callback that reads into uploadedJson and the other matching callback around line 178), ensuring UploadAsync and the service-created stream are not disposed by the test helper.
51-115: Missing test:LoadAsyncwith malformed/non-JSON blob contentThe suite covers: container absent, blob absent, empty array
[], valid entries, andRequestFailedException. It does not test the case where the blob contains syntactically invalid JSON (e.g.,"not-json"). IfHjemGroupAdminService.LoadAsyncwrapsJsonSerializer.Deserializein a try/catch and returns empty, that path is currently untested.♻️ Suggested additional test
[Fact] public async Task LoadAsync_ReturnsEmpty_WhenBlobContainsInvalidJson() { SetupBlobWithContent("this is not valid json {{{"); var result = await CreateSut().LoadAsync(); result.Should().BeEmpty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs` around lines 51 - 115, Add a test that verifies HjemGroupAdminService.LoadAsync returns an empty collection when the blob contains malformed JSON: create a new Fact similar to existing tests (e.g., LoadAsync_ReturnsEmpty_WhenBlobContainsInvalidJson) that uses SetupBlobWithContent("this is not valid json {{{") then calls CreateSut().LoadAsync() and asserts the result is empty; this exercises the JSON-deserialization error path in LoadAsync and complements the existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs`:
- Around line 155-160: The StreamReader in the Arg.Do<Stream> callback used to
capture uploadedJson disposes the input stream; change the StreamReader
construction to pass leaveOpen: true so the captured stream isn't closed (update
the StreamReader in the UploadAsync Arg.Do<Stream> callback that reads into
uploadedJson and the other matching callback around line 178), ensuring
UploadAsync and the service-created stream are not disposed by the test helper.
- Around line 51-115: Add a test that verifies HjemGroupAdminService.LoadAsync
returns an empty collection when the blob contains malformed JSON: create a new
Fact similar to existing tests (e.g.,
LoadAsync_ReturnsEmpty_WhenBlobContainsInvalidJson) that uses
SetupBlobWithContent("this is not valid json {{{") then calls
CreateSut().LoadAsync() and asserts the result is empty; this exercises the
JSON-deserialization error path in LoadAsync and complements the existing tests.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor (1)
158-173: Thecatchblock inOnInitializedAsyncis dead code.
AdminService.LoadAsync()already has an internalcatch (Exception)that logs and returns[], so the outercatchblock (lines 164–168) can never be reached. This is misleading — it implies error-handling coverage that does not exist at this layer. Thefinallyblock is still needed to reset_isLoading.♻️ Proposed simplification
protected override async Task OnInitializedAsync() { - try - { - _entries = await AdminService.LoadAsync(); - } - catch (Exception ex) - { - Logger.LogError(ex, "Failed to load HJEM group entries."); - Snackbar.Add("Kunne ikke hente HJEM grupper.", Severity.Error); - } - finally - { - _isLoading = false; - } + _entries = await AdminService.LoadAsync(); + _isLoading = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` around lines 158 - 173, The catch block in OnInitializedAsync is dead because AdminService.LoadAsync already handles exceptions and returns an empty array, so remove the outer try/catch (the Logger.LogError and Snackbar.Add lines) and keep the finally to set _isLoading = false; locate the OnInitializedAsync method, remove the catch portion that references Logger.LogError and Snackbar.Add, and ensure _entries is still assigned from await AdminService.LoadAsync() and _isLoading is reset in the finally block.src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs (1)
14-21:ContainerName,BlobName, andJsonOptionsare duplicated inHjemGroupProvider.Both classes independently declare the same
ContainerName = "hjemlo-groups",BlobName = "groups.json", and a CamelCaseJsonOptionsinstance. A typo or future rename in one file would silently diverge the two readers/writers.Consider extracting to a shared static inner class or a separate
HjemGroupBlobConstantsfile:♻️ Proposed extraction
+// New file: HjemGroupBlobConstants.cs +namespace Jordnaer.Features.HjemGroups; + +internal static class HjemGroupBlobConstants +{ + internal const string ContainerName = "hjemlo-groups"; + internal const string BlobName = "groups.json"; + + internal static readonly JsonSerializerOptions ReadOptions = new() + { + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + }; + + internal static readonly JsonSerializerOptions WriteOptions = new() + { + WriteIndented = true, + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + }; +}Then reference
HjemGroupBlobConstants.*from bothHjemGroupAdminServiceandHjemGroupProviderinstead of local constants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs` around lines 14 - 21, ContainerName, BlobName, and JsonOptions are duplicated between HjemGroupAdminService and HjemGroupProvider; extract these shared values into a single static container (e.g. class HjemGroupBlobConstants with public const string ContainerName = "hjemlo-groups", public const string BlobName = "groups.json", and a public static readonly JsonSerializerOptions JsonOptions configured with WriteIndented=true and CamelCase) and update both HjemGroupAdminService and HjemGroupProvider to reference HjemGroupBlobConstants.ContainerName, HjemGroupBlobConstants.BlobName, and HjemGroupBlobConstants.JsonOptions to avoid divergence.src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs (1)
26-59: In-memory cache is never invalidated — admin saves won't be reflected until restart.
_cachedis populated once and never cleared. BecauseHjemGroupProvideris registered as a singleton, any groups added or updated viaHjemGroupAdminService.SaveAsyncwon't appear on the map until the server process restarts. Additionally, when the container or blob is missing (lines 38, 43),_cachedis deliberately not set, so every subsequent call makes two round-trips to blob storage to check existence.Consider either adding a cache-invalidation hook (
InvalidateCache()) thatHjemGroupAdminService.SaveAsynccan call after a successful write, or replacing the instance field with a short-TTL cache entry (e.g.IMemoryCache/ FusionCache — already registered inProgram.cs) so stale data ages out automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs` around lines 26 - 59, HjemGroupProvider currently caches marker data in the private field _cached inside GetMarkersAsync and never invalidates it, so updates via HjemGroupAdminService.SaveAsync aren’t reflected; modify the provider to either (A) expose a public InvalidateCache() method on HjemGroupProvider and call that from HjemGroupAdminService.SaveAsync after a successful blob write, or (B) replace the _cached field with a short-TTL cache entry using the registered IMemoryCache/FusionCache (inject IMemoryCache/FusionCache into HjemGroupProvider and use a cache key like "HjemGroupMarkers") so reads check the cache TTL instead of permanently storing results; also set the cache when container/blob is missing to avoid repeated ExistsAsync round-trips or ensure the cache stores an empty list result to short-circuit subsequent calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs`:
- Around line 14-21: ContainerName, BlobName, and JsonOptions are duplicated
between HjemGroupAdminService and HjemGroupProvider; extract these shared values
into a single static container (e.g. class HjemGroupBlobConstants with public
const string ContainerName = "hjemlo-groups", public const string BlobName =
"groups.json", and a public static readonly JsonSerializerOptions JsonOptions
configured with WriteIndented=true and CamelCase) and update both
HjemGroupAdminService and HjemGroupProvider to reference
HjemGroupBlobConstants.ContainerName, HjemGroupBlobConstants.BlobName, and
HjemGroupBlobConstants.JsonOptions to avoid divergence.
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs`:
- Around line 26-59: HjemGroupProvider currently caches marker data in the
private field _cached inside GetMarkersAsync and never invalidates it, so
updates via HjemGroupAdminService.SaveAsync aren’t reflected; modify the
provider to either (A) expose a public InvalidateCache() method on
HjemGroupProvider and call that from HjemGroupAdminService.SaveAsync after a
successful blob write, or (B) replace the _cached field with a short-TTL cache
entry using the registered IMemoryCache/FusionCache (inject
IMemoryCache/FusionCache into HjemGroupProvider and use a cache key like
"HjemGroupMarkers") so reads check the cache TTL instead of permanently storing
results; also set the cache when container/blob is missing to avoid repeated
ExistsAsync round-trips or ensure the cache stores an empty list result to
short-circuit subsequent calls.
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 158-173: The catch block in OnInitializedAsync is dead because
AdminService.LoadAsync already handles exceptions and returns an empty array, so
remove the outer try/catch (the Logger.LogError and Snackbar.Add lines) and keep
the finally to set _isLoading = false; locate the OnInitializedAsync method,
remove the catch portion that references Logger.LogError and Snackbar.Add, and
ensure _entries is still assigned from await AdminService.LoadAsync() and
_isLoading is reset in the finally block.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/web/Jordnaer/Components/NotificationItem.razor (1)
26-28: Correct fix —display: blockis required for CSS text-truncation to apply.
Typo.captionrenders as an inline<span>in MudBlazor, sowhite-space: nowrap; overflow: hidden; text-overflow: ellipsis(the usual content ofnotification-description-truncate) silently has no effect without a block formatting context. The inlineStyle="display: block;"fixes that correctly.As a minor clean-up, consider moving
display: blockinto thenotification-description-truncateCSS class so all truncation-related rules live in one place and the inlineStyleattribute can be removed:♻️ Proposed refactor — consolidate into CSS class
In the component's stylesheet (or global CSS), add the property to the existing rule:
.notification-description-truncate { + display: block; /* existing truncation rules, e.g.: */ overflow: hidden; text-overflow: ellipsis; white-space: nowrap; }Then remove the inline style:
-<MudText Typo="Typo.caption" Color="Color.Secondary" Class="notification-description-truncate" Style="display: block;"> +<MudText Typo="Typo.caption" Color="Color.Secondary" Class="notification-description-truncate">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Components/NotificationItem.razor` around lines 26 - 28, The MudText in NotificationItem.razor uses Typo.caption (renders as inline) so the truncation CSS doesn't work; move the inline Style="display: block;" into the notification-description-truncate CSS rule instead: update the notification-description-truncate class to include display: block (alongside white-space: nowrap; overflow: hidden; text-overflow: ellipsis) and remove the inline Style attribute from the MudText element that outputs `@Notification.Description` to keep truncation rules centralized and remove the inline override.tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs (2)
311-315: Optional: DisposeApiResponse/HttpResponseMessageinstances in the helper.
ApiResponse<T>implementsIDisposable(viaIApiResponse) and wraps a rawHttpResponseMessage. If the productionGeocodeAsyncdisposes the response (e.g.using var resp = await _geocoder.SearchZipCodesAsync(...)) theHttpResponseMessageis cleaned up through that path, but the inline instance inGeocodeAsync_ReturnsNull_WhenApiCallFails(line 221) is never returned to the substitute and leaks. Wrapping these withusingor makingMakeGeoResponsereturn a helper that manages lifetime keeps the intent explicit and prevents xUnit leak-detection warnings.♻️ Suggested approach
- private static IApiResponse<IEnumerable<ZipCodeSearchResponse>> MakeGeoResponse(params ZipCodeSearchResponse[] results) => - new ApiResponse<IEnumerable<ZipCodeSearchResponse>>( - new HttpResponseMessage(HttpStatusCode.OK), - results, - new RefitSettings()); + private static ApiResponse<IEnumerable<ZipCodeSearchResponse>> MakeGeoResponse( + params ZipCodeSearchResponse[] results) => + new(new HttpResponseMessage(HttpStatusCode.OK), results, new RefitSettings());And in
GeocodeAsync_ReturnsNull_WhenApiCallFails:- _geocoder - .SearchZipCodesAsync(Arg.Any<string>(), Arg.Any<CancellationToken>()) - .Returns(new ApiResponse<IEnumerable<ZipCodeSearchResponse>>( - new HttpResponseMessage(HttpStatusCode.InternalServerError), null, new RefitSettings())); + using var failResponse = new ApiResponse<IEnumerable<ZipCodeSearchResponse>>( + new HttpResponseMessage(HttpStatusCode.InternalServerError), null, new RefitSettings()); + _geocoder + .SearchZipCodesAsync(Arg.Any<string>(), Arg.Any<CancellationToken>()) + .Returns(failResponse);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs` around lines 311 - 315, The MakeGeoResponse helper currently constructs an ApiResponse wrapping a raw HttpResponseMessage but never disposes it; update MakeGeoResponse (used by GeocodeAsync_ReturnsNull_WhenApiCallFails) to manage disposal by either returning an IDisposable wrapper/IApiResponse that disposes its HttpResponseMessage or by creating/returning the ApiResponse inside a using/factory that ensures the HttpResponseMessage is disposed when the test/substitute is done; reference ApiResponse<IEnumerable<ZipCodeSearchResponse>>, HttpResponseMessage and MakeGeoResponse when changing the helper so no HttpResponseMessage leaks occur.
102-115: Optional: Verify that the logger receives an error call on blob failure.The AI summary notes that
HjemGroupAdminService.LoadAsynclogs errors when an exception is raised. The test currently only asserts the return value is empty, leaving the logging path untested. A small addition would catch any future regression where the log call is silently dropped:✅ Proposed addition
result.Should().BeEmpty(); + + _logger.Received(1).Log( + LogLevel.Error, + Arg.Any<EventId>(), + Arg.Any<object>(), + Arg.Is<Exception>(e => e is RequestFailedException), + Arg.Any<Func<object, Exception?, string>>());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs` around lines 102 - 115, The test currently asserts only that LoadAsync returns empty when the blob download throws; add an assertion that the injected logger receives an error call so the logging path is covered. After calling CreateSut().LoadAsync() in LoadAsync_ReturnsEmpty_WhenBlobThrows, verify the test's logger substitute (e.g. _logger or the ILogger<HjemGroupAdminService> mock used in the test fixture) received an error-level log invocation (use Received(1) with LogError or a ReceivedWithAnyArgs check for an error call) to ensure HjemGroupAdminService.LoadAsync logs the exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/web/Jordnaer/Components/NotificationItem.razor`:
- Around line 26-28: The MudText in NotificationItem.razor uses Typo.caption
(renders as inline) so the truncation CSS doesn't work; move the inline
Style="display: block;" into the notification-description-truncate CSS rule
instead: update the notification-description-truncate class to include display:
block (alongside white-space: nowrap; overflow: hidden; text-overflow: ellipsis)
and remove the inline Style attribute from the MudText element that outputs
`@Notification.Description` to keep truncation rules centralized and remove the
inline override.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs`:
- Around line 311-315: The MakeGeoResponse helper currently constructs an
ApiResponse wrapping a raw HttpResponseMessage but never disposes it; update
MakeGeoResponse (used by GeocodeAsync_ReturnsNull_WhenApiCallFails) to manage
disposal by either returning an IDisposable wrapper/IApiResponse that disposes
its HttpResponseMessage or by creating/returning the ApiResponse inside a
using/factory that ensures the HttpResponseMessage is disposed when the
test/substitute is done; reference
ApiResponse<IEnumerable<ZipCodeSearchResponse>>, HttpResponseMessage and
MakeGeoResponse when changing the helper so no HttpResponseMessage leaks occur.
- Around line 102-115: The test currently asserts only that LoadAsync returns
empty when the blob download throws; add an assertion that the injected logger
receives an error call so the logging path is covered. After calling
CreateSut().LoadAsync() in LoadAsync_ReturnsEmpty_WhenBlobThrows, verify the
test's logger substitute (e.g. _logger or the ILogger<HjemGroupAdminService>
mock used in the test fixture) received an error-level log invocation (use
Received(1) with LogError or a ReceivedWithAnyArgs check for an error call) to
ensure HjemGroupAdminService.LoadAsync logs the exception.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor (2)
301-301: No delete confirmation, but implicit safeguard exists.Deleting only modifies the in-memory list — changes are not persisted until "Gem alle" is clicked, so an accidental delete is recoverable by refreshing. If this is intentional, it's fine. A brief
MudMessageBoxconfirmation could improve UX down the road.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` at line 301, DeleteEntry currently removes an item from the in-memory _entries list without user confirmation; add a confirmation dialog before removal to improve UX. Update the DeleteEntry method to show a MudMessageBox (or equivalent confirmation component), and only call _entries.Remove(entry) if the user confirms; keep current behavior that changes are only persisted when "Gem alle" is clicked.
254-299: Good: coordinate validation addresses the (0,0) edge case.The range check and zero-coordinate guard look solid. One subtle note: since
HjemGroupEntryis arecord,IndexOfat line 293 uses value equality. If two entries happen to have identical field values,IndexOfcould match the wrong one. This is unlikely in practice but worth being aware of.Consider using an index-based approach in
OpenEditDialoginstead:♻️ Suggested alternative
- private HjemGroupEntry? _editingEntry; + private int _editingIndex = -1; private void OpenEditDialog(HjemGroupEntry entry) { _editingNew = false; - _editingEntry = entry; + _editingIndex = _entries.IndexOf(entry); ... } // In CommitEdit: - else if (_editingEntry is not null) + else if (_editingIndex >= 0 && _editingIndex < _entries.Count) { - var idx = _entries.IndexOf(_editingEntry); - if (idx >= 0) - _entries[idx] = updated; + _entries[_editingIndex] = updated; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` around lines 254 - 299, The CommitEdit uses _entries.IndexOf(_editingEntry) which relies on record value-equality and can misidentify the edited item; instead add and use an index tracked when opening the editor. In OpenEditDialog set a new private int _editingIndex (e.g. -1 default) to the index of the entry being edited (or -1 for new), then in CommitEdit replace the IndexOf branch with: if (_editingNew) _entries.Add(updated) else if (_editingIndex >= 0) _entries[_editingIndex] = updated; keep _editingEntry for convenience or clear it, and remove the IndexOf call to avoid value-equality collisions.tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs (1)
313-317:HttpResponseMessageinMakeGeoResponseis not disposed by callers.The
HttpResponseMessagecreated at line 315 is wrapped in anApiResponseand returned, but most callers (lines 210, 236, 252, 271, 284) don't dispose it. In test code this typically doesn't cause issues, but it's inconsistent with the explicitusingpattern at lines 220-221.Consider wrapping the helper to use
IDisposableor just accept the inconsistency since it's test code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs` around lines 313 - 317, MakeGeoResponse creates an HttpResponseMessage that never gets disposed by callers; change the helper so callers can dispose the underlying response: create a small wrapper type (e.g., DisposableApiResponse<T>) that implements IDisposable, holds the ApiResponse<IEnumerable<ZipCodeSearchResponse>> and the HttpResponseMessage, and disposes the HttpResponseMessage in Dispose; then change MakeGeoResponse to return DisposableApiResponse<ZipCodeSearchResponse> instead of ApiResponse<IEnumerable<ZipCodeSearchResponse>> so tests that currently call MakeGeoResponse can dispose the wrapper (or use a using statement) to properly release the HttpResponseMessage.
🤖 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/NotificationItem.razor`:
- Around line 26-28: Replace the raw div in NotificationItem.razor with the
MudBlazor component MudText to avoid hardcoding MudBlazor internals; use MudText
with the existing "notification-description-truncate" CSS via the Class
parameter and set equivalent public props (e.g., Typo="Typo.caption" and
Color="Color.Secondary") so the rendered markup and styling remain the same
while relying only on MudText's public API and keeping the content
`@Notification.Description`.
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs`:
- Around line 26-60: The provider permanently caches markers in the private
field _cached inside GetMarkersAsync, so admin edits via
HjemGroupAdminService.SaveAsync never propagate; add a cache invalidation method
to the provider interface (e.g., InvalidateCache or ClearMarkersCache) and
implement it to set _cached = null in HjemGroupProvider, then ensure
HjemGroupAdminService.SaveAsync calls that invalidation after a successful save
(or fire an event that the admin service triggers to clear the cache) so
subsequent GetMarkersAsync reloads fresh data.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs`:
- Around line 102-116: The logger assertion in the test
LoadAsync_ReturnsEmpty_WhenBlobThrows is not verifiable because ILogger.Log is
an extension method and cannot be intercepted by NSubstitute; remove the
unverifiable assertion `_logger.ReceivedWithAnyArgs(1).Log(...)` (or
alternatively replace it by using a logging test helper like
NSubstitute.Extensions.LoggingExtensions if you want to assert logs) and keep
the existing behavioral assertion that result.Should().BeEmpty(); this change
touches the test method LoadAsync_ReturnsEmpty_WhenBlobThrows and the `_logger`
verification line.
---
Nitpick comments:
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Line 301: DeleteEntry currently removes an item from the in-memory _entries
list without user confirmation; add a confirmation dialog before removal to
improve UX. Update the DeleteEntry method to show a MudMessageBox (or equivalent
confirmation component), and only call _entries.Remove(entry) if the user
confirms; keep current behavior that changes are only persisted when "Gem alle"
is clicked.
- Around line 254-299: The CommitEdit uses _entries.IndexOf(_editingEntry) which
relies on record value-equality and can misidentify the edited item; instead add
and use an index tracked when opening the editor. In OpenEditDialog set a new
private int _editingIndex (e.g. -1 default) to the index of the entry being
edited (or -1 for new), then in CommitEdit replace the IndexOf branch with: if
(_editingNew) _entries.Add(updated) else if (_editingIndex >= 0)
_entries[_editingIndex] = updated; keep _editingEntry for convenience or clear
it, and remove the IndexOf call to avoid value-equality collisions.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs`:
- Around line 313-317: MakeGeoResponse creates an HttpResponseMessage that never
gets disposed by callers; change the helper so callers can dispose the
underlying response: create a small wrapper type (e.g.,
DisposableApiResponse<T>) that implements IDisposable, holds the
ApiResponse<IEnumerable<ZipCodeSearchResponse>> and the HttpResponseMessage, and
disposes the HttpResponseMessage in Dispose; then change MakeGeoResponse to
return DisposableApiResponse<ZipCodeSearchResponse> instead of
ApiResponse<IEnumerable<ZipCodeSearchResponse>> so tests that currently call
MakeGeoResponse can dispose the wrapper (or use a using statement) to properly
release the HttpResponseMessage.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/HjemGroups/HjemGroupAdminService.cs`:
- Around line 25-45: LoadAsync currently swallows all exceptions and returns an
empty list; change its signature in HjemGroupAdminService to
Task<OneOf<List<HjemGroupEntry>, HjemGroupLoadError>> and stop returning [] on
failures so callers can distinguish "no entries" from "load error". Implement a
HjemGroupLoadError class/record that carries error details (exception
message/inner info) and return OneOf.Success when deserialization/network calls
succeed, and return OneOf.Error(HjemGroupLoadError) when blob container/blob
does not exist or any exception occurs (preserve/log exception via
logger.LogWarning but do not translate it into an empty list). Update references
to LoadAsync (e.g., HjemGroupManagementPage.razor) to handle the OneOf result
accordingly.
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs`:
- Around line 10-37: Change IHjemGroupProvider.GetMarkersAsync and
HjemGroupProvider.GetMarkersAsync to return
OneOf<IReadOnlyList<GroupMarkerData>, Error> instead of
IReadOnlyList<GroupMarkerData>, and update the fusionCache usage so the factory
returns OneOf by calling LoadFromBlobAsync which must also be changed to return
OneOf<IReadOnlyList<GroupMarkerData>, Error> rather than swallowing exceptions;
replace the current empty-list fallback (currently "?? []" and the catch in
LoadFromBlobAsync that returns []) with propagating/constructing an Error value
on failure (and return the successful list wrapped in OneOf on success), and
update any callers of GetMarkersAsync to handle the OneOf result; refer to
IHjemGroupProvider, HjemGroupProvider, GetMarkersAsync, LoadFromBlobAsync,
CacheKey and CacheTag when implementing these changes.
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 110-112: The label "URL (valgfri)" contradicts CommitEdit which
rejects empty _form.WebsiteUrl; update the UI to reflect the requirement by
changing the MudTextField Label from "URL (valgfri)" to a required label (e.g.,
"URL (påkrævet)" or simply "URL") for both occurrences (the MudTextField bound
to _form.WebsiteUrl at the shown block and the duplicate at lines ~256-259), and
ensure any validation messages shown next to the field match CommitEdit's
required validation so the UI and the CommitEdit method are consistent.
In `@src/web/Jordnaer/wwwroot/js/leaflet-interop.js`:
- Around line 356-369: The current logic sets isHjemUrl based only on hostname
and can allow non-HTTP schemes; update the check in the block that builds the
primary link (where parsedGroupUrl is created and isHjemUrl is set) to also
verify parsedGroupUrl.protocol is "http:" or "https:" before treating it as a
HJEM URL, and then reuse that safe isHjemUrl when computing groupUrl and
groupUrlTarget (the same change should be applied to the other occurrence around
groupUrl/groupUrlTarget assignment). Ensure you still wrap URL construction in
the try/catch around new URL(group.websiteUrl) and fall back to the encoded
local path when the protocol/hostname check fails.
---
Duplicate comments:
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs`:
- Around line 117-121: The test is asserting logs using NSubstitute's
_logger.ReceivedWithAnyArgs(...) which is not interceptable for ILogger<T>;
remove that unverifiable assertion or replace it with a proper logging test
helper (e.g., capture logs via a TestLogger/TestSink or use
Microsoft.Extensions.Logging.Testing) to assert a warning was written; update
the HjemGroupAdminServiceTests where CreateSut().LoadAsync() is called and
remove the line "_logger.ReceivedWithAnyArgs(1).Log(...)" or swap it out for a
logging helper that inspects emitted log entries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/wwwroot/css/marker-cluster.css (1)
91-111:⚠️ Potential issue | 🟡 MinorProvide a fallback for initials when
:has()is unsupported.Without a fallback, initial text will be invisible (white on white) in browsers lacking
:has()support. While:has()is now supported in all modern browsers (Chrome/Edge 105+, Safari 15.4+, Firefox 121+), adding a CSS-only fallback ensures robustness. Either apply the background color directly to the initial element or use a parent class.Recommended CSS fallback
.group-marker-icon .group-marker-initial { + width: 100%; + height: 100%; + display: flex; + align-items: center; + justify-content: center; + background-color: var(--map-primary); + border-radius: 50%; color: var(--neutral-white); font-size: 14px; font-weight: 600; font-family: var(--font-body-bold), 'Helvetica Neue', Arial, sans-serif; } @@ .group-popup-avatar .group-initial { + width: 100%; + height: 100%; + display: flex; + align-items: center; + justify-content: center; + background-color: var(--map-primary); + border-radius: 50%; color: var(--neutral-white); font-size: 20px; font-weight: 600; }Also applies to: 150–170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/wwwroot/css/marker-cluster.css` around lines 91 - 111, The .group-marker-icon relies on the :has(.group-marker-initial) selector which leaves initials invisible in browsers without :has() support; add a CSS-only fallback by giving the initial element itself a visible background and text color (e.g., set styles on .group-marker-initial: background-color: var(--map-primary); color: var(--neutral-white); border-radius: 50%; display:flex; align-items:center; justify-content:center) or by using a parent helper class (e.g., .group-marker-icon--has-initial) applied where initials exist and style .group-marker-icon--has-initial the same as the :has() rule; apply the same fallback to the other occurrence noted (the rule around lines 150–170) and ensure contrast and sizing match the existing .group-marker-icon styles.
🧹 Nitpick comments (5)
tests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs (1)
96-128: Consider extracting the repeatedJsonSerializerOptionsto a shared static field.The
new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }is duplicated across multiple tests (lines 109, 147, 173, 195, 207, 254, 275). Aprivate static readonlyfield (similar toHjemGroupAdminServiceTests.CamelCase) would reduce repetition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs` around lines 96 - 128, Extract the repeated JsonSerializerOptions instantiation into a shared private static readonly field (e.g. private static readonly JsonSerializerOptions CamelCaseOptions) in HjemGroupProviderTests and replace each occurrence of new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase } (used in SetupBlobWithContent calls around GetMarkersAsync test and others) with the new CamelCaseOptions field to eliminate duplication and centralize the camelCase configuration.src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor (2)
318-335:SaveAsyncdoesn't prevent double-clicks beyond the_isSavingflag.The
_isSavingflag disables the button, but since Blazor re-renders asynchronously, there's a small window where a rapid double-click could trigger two concurrent saves. In practice this is low risk given the admin-only audience, butStateHasChanged()isn't called after setting_isSaving = true— the UI won't update until theawaityields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` around lines 318 - 335, SaveAsync can still be started twice due to a small render race; guard the method and force a render: at the top of SaveAsync check if _isSaving is true and return immediately to prevent re-entry, then set _isSaving = true and call StateHasChanged() before awaiting AdminService.SaveAsync to ensure the UI (button disabled) updates before the async yield; keep the existing try/catch/finally but ensure the finally sets _isSaving = false and calls StateHasChanged() again so the UI re-enables reliably.
316-316: No delete confirmation — accidental deletions are possible.
DeleteEntryimmediately removes the entry from the in-memory list without a confirmation prompt. The "Gem alle" save pattern provides a safety net (unsaved changes are lost on navigation), but a simpleMudDialogconfirmation or a "Fortryd" (undo) Snackbar would be a nice UX improvement for an admin tool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` at line 316, DeleteEntry currently removes the HjemGroupEntry from the in-memory _entries list immediately, risking accidental deletions; update DeleteEntry (or replace calls to it) to open a MudDialog confirmation (or show a Snackbar with undo) before removing the item, and only call _entries.Remove(entry) after the dialog returns a positive confirmation (or if undo is not used, queue removal until the save action); reference the DeleteEntry method and the _entries collection when adding the confirmation flow so the UI blocks removal until the user confirms.src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs (1)
53-67:SaveAsyncdoesn't useOneOffor error signaling.Exceptions from blob upload or message publish propagate directly to the caller. The Razor page wraps calls in try/catch, but per the coding guideline for
src/web/Jordnaer/Features/**/*.cs, service methods should useOneOf<TSuccess, TError>for explicit error handling. This is a lower priority since the caller already handles exceptions, but worth noting for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs` around lines 53 - 67, SaveAsync currently throws exceptions from blob upload and publish instead of returning a OneOf result; change its signature from Task to Task<OneOf<Unit, TError>> (pick or create an appropriate error type, e.g., HjemGroupSaveError) and wrap the blobServiceClient/container/blobClient/upload and publishEndpoint.Publish( new InvalidateCacheTags { Tags = [HjemGroupProvider.CacheTag] } ) calls in try/catch blocks, returning OneOf.Success (Unit.Value) on success and OneOf.Error with the mapped error on failure; update callers to handle the OneOf result and keep HjemGroupEntry, publishEndpoint, InvalidateCacheTags and HjemGroupProvider.CacheTag references to locate the code.tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs (1)
332-336:MakeGeoResponsecreates unmanagedHttpResponseMessageinstances.Each call allocates a
HttpResponseMessagethat isn't disposed. While this is test code and the impact is negligible, the two explicitMakeGeoResponsecallers inGeocodeAsync_ReturnsNull_WhenApiCallFails(lines 239-240) correctly useusing. For consistency, consider making this helper return a disposable or noting that the caller should dispose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs` around lines 332 - 336, MakeGeoResponse currently allocates an HttpResponseMessage that is never disposed; change it to return a disposable wrapper so callers can use using and ensure the message is disposed. Implement a small class (e.g., DisposableApiResponse<T> : IDisposable) that holds the ApiResponse<IEnumerable<ZipCodeSearchResponse>> (constructed with the created HttpResponseMessage) and disposes the underlying HttpResponseMessage in Dispose; update MakeGeoResponse to return DisposableApiResponse<IEnumerable<ZipCodeSearchResponse>> and adjust callers (e.g., in GeocodeAsync_ReturnsNull_WhenApiCallFails) to use using on the wrapper.
🤖 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/HjemGroups/HjemGroupProvider.cs`:
- Around line 32-39: GetMarkersAsync currently caches the OneOf result returned
by LoadFromBlobAsync, so transient Error variants get stored and served; change
the factory passed to fusionCache.GetOrSetAsync (the lambda for key CacheKey
with tags CacheTag) so it does not return Error values to FusionCache: call
LoadFromBlobAsync, inspect the OneOf result and if it's an Error throw an
exception (or otherwise propagate a failure) instead of returning the Error,
ensuring only successful IReadOnlyList<GroupMarkerData> is cached; alternatively
implement a short-lived cache path for errors or use a predicate/overload that
prevents caching Error variants—apply this change around GetMarkersAsync, the
fusionCache.GetOrSetAsync call, and LoadFromBlobAsync.
In `@tasks/13-cheaper-infrastructure.md`:
- Around line 49-62: The document incorrectly labels Hetzner's CX22 as ARM and
misstates CX32 pricing; update the text to either (A) treat CX22 as an x86 plan:
remove all "Ampere ARM" references, delete ARM-specific guidance (the
`--platform linux/arm64` build flag note, ARM Docker image comment, and the ARM
compatibility risk row) and confirm the existing Dockerfile works for x86, or
(B) switch to a CAX plan if ARM is intended: replace CX22 with the appropriate
CAX model (e.g., CAX11/CAX21), update specs/pricing and ARM build guidance
accordingly; also correct the CX32 price to €6.80/month wherever CX32 is
mentioned.
- Line 168: Update the prose to use the correct trademark capitalization
"GitHub" instead of "Github" in the sentence that references the website_cd.yml
workflow and the Dokploy webhook replacement; locate the string "Github" in the
file (it appears near the sentence "In the current
[website_cd.yml](.github/workflows/website_cd.yml), replace the Azure deployment
steps with a Dokploy webhook call") and change it to "GitHub".
- Around line 184-188: The "Deploy via Dokploy webhook" CI step uses curl
without --fail so HTTP 4xx/5xx responses won't fail the job; update the curl
invocation in that step to include --fail (and optionally --show-error and
--silent if you want quieter output) so curl returns a non-zero exit code on
HTTP errors—i.e., change the curl command used in the "Deploy via Dokploy
webhook" step to curl --fail [--show-error --silent] -X POST "${{
secrets.DOKPLOY_WEBHOOK_URL }}" -H "Authorization: Bearer ${{
secrets.DOKPLOY_API_KEY }}" to ensure the CI step fails on rejected/failed
webhook calls.
- Line 5: Replace the duplicated phrase "storage storage" with the correct term
"azure blob storage" in the document: find the line containing "storage storage
(blob storage, close to free)" and change it to "azure blob storage (close to
free)" so the parenthetical isn't redundant and the service name is accurate.
- Around line 148-151: The current "Install Dokploy" step pipes the remote
installer into sh (the curl | sh command), which is a supply-chain risk; change
the runbook to first download the installer to a local file (e.g., save the
output of the curl command to dokploy-install.sh), verify its integrity (provide
steps to check a published SHA256 checksum or GPG signature), inspect the file
before running, and only then execute it (e.g., sh dokploy-install.sh); update
the "Install Dokploy" step to reference the saved installer file and include the
checksum/verification step and an explicit execution step instead of piping into
sh.
- Line 215: The migration plan is missing enabling Docker Swarm mode in Dokploy
so the "Rolling" mitigation will actually work; under Step 2 add a sub-step to
enable Swarm mode in Dokploy's Advanced Tab → Cluster Settings → Swarm Settings
and configure a health check that points at /alive plus an update_config for
rolling updates (so the Rolling strategy will use the /alive probe and perform
update_config-based draining rather than stopping the old container
immediately); mention that this is required for HostOptions.ShutdownTimeout,
boot.js retry behavior, and Blazor.reconnect() to function as described.
---
Outside diff comments:
In `@src/web/Jordnaer/wwwroot/css/marker-cluster.css`:
- Around line 91-111: The .group-marker-icon relies on the
:has(.group-marker-initial) selector which leaves initials invisible in browsers
without :has() support; add a CSS-only fallback by giving the initial element
itself a visible background and text color (e.g., set styles on
.group-marker-initial: background-color: var(--map-primary); color:
var(--neutral-white); border-radius: 50%; display:flex; align-items:center;
justify-content:center) or by using a parent helper class (e.g.,
.group-marker-icon--has-initial) applied where initials exist and style
.group-marker-icon--has-initial the same as the :has() rule; apply the same
fallback to the other occurrence noted (the rule around lines 150–170) and
ensure contrast and sizing match the existing .group-marker-icon styles.
---
Nitpick comments:
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs`:
- Around line 53-67: SaveAsync currently throws exceptions from blob upload and
publish instead of returning a OneOf result; change its signature from Task to
Task<OneOf<Unit, TError>> (pick or create an appropriate error type, e.g.,
HjemGroupSaveError) and wrap the blobServiceClient/container/blobClient/upload
and publishEndpoint.Publish( new InvalidateCacheTags { Tags =
[HjemGroupProvider.CacheTag] } ) calls in try/catch blocks, returning
OneOf.Success (Unit.Value) on success and OneOf.Error with the mapped error on
failure; update callers to handle the OneOf result and keep HjemGroupEntry,
publishEndpoint, InvalidateCacheTags and HjemGroupProvider.CacheTag references
to locate the code.
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 318-335: SaveAsync can still be started twice due to a small
render race; guard the method and force a render: at the top of SaveAsync check
if _isSaving is true and return immediately to prevent re-entry, then set
_isSaving = true and call StateHasChanged() before awaiting
AdminService.SaveAsync to ensure the UI (button disabled) updates before the
async yield; keep the existing try/catch/finally but ensure the finally sets
_isSaving = false and calls StateHasChanged() again so the UI re-enables
reliably.
- Line 316: DeleteEntry currently removes the HjemGroupEntry from the in-memory
_entries list immediately, risking accidental deletions; update DeleteEntry (or
replace calls to it) to open a MudDialog confirmation (or show a Snackbar with
undo) before removing the item, and only call _entries.Remove(entry) after the
dialog returns a positive confirmation (or if undo is not used, queue removal
until the save action); reference the DeleteEntry method and the _entries
collection when adding the confirmation flow so the UI blocks removal until the
user confirms.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs`:
- Around line 332-336: MakeGeoResponse currently allocates an
HttpResponseMessage that is never disposed; change it to return a disposable
wrapper so callers can use using and ensure the message is disposed. Implement a
small class (e.g., DisposableApiResponse<T> : IDisposable) that holds the
ApiResponse<IEnumerable<ZipCodeSearchResponse>> (constructed with the created
HttpResponseMessage) and disposes the underlying HttpResponseMessage in Dispose;
update MakeGeoResponse to return
DisposableApiResponse<IEnumerable<ZipCodeSearchResponse>> and adjust callers
(e.g., in GeocodeAsync_ReturnsNull_WhenApiCallFails) to use using on the
wrapper.
In `@tests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs`:
- Around line 96-128: Extract the repeated JsonSerializerOptions instantiation
into a shared private static readonly field (e.g. private static readonly
JsonSerializerOptions CamelCaseOptions) in HjemGroupProviderTests and replace
each occurrence of new JsonSerializerOptions { PropertyNamingPolicy =
JsonNamingPolicy.CamelCase } (used in SetupBlobWithContent calls around
GetMarkersAsync test and others) with the new CamelCaseOptions field to
eliminate duplication and centralize the camelCase configuration.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cssrc/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cssrc/web/Jordnaer/Features/Partners/PartnerService.cssrc/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razorsrc/web/Jordnaer/Pages/GroupSearch/GroupSearch.razorsrc/web/Jordnaer/Program.cssrc/web/Jordnaer/wwwroot/css/marker-cluster.csssrc/web/Jordnaer/wwwroot/js/leaflet-interop.jstasks/13-cheaper-infrastructure.mdtests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cstests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/web/Jordnaer/Program.cs
- src/web/Jordnaer/Pages/GroupSearch/GroupSearch.razor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs (1)
22-46: Verify there is a cache invalidation path after admin updates.If no invalidation exists, markers can remain stale until cache expiry/app restart. Please confirm that admin save flows clear this cache (e.g., via
fusionCache.RemoveByTag(CacheTag)), or add an explicit invalidation method.#!/bin/bash # Search for HJEM group cache invalidation usage (tags/keys). rg -n -C2 'hjem-groups|CacheTag|RemoveByTag|InvalidateCache|Clear.*Cache'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs` around lines 22 - 46, GetMarkersAsync is caching markers under CacheKey with tag CacheTag but there is no evidence here of a cache invalidation path; add or confirm a call to fusionCache.RemoveByTag(CacheTag) (or fusionCache.Remove(CacheKey)) from the admin save/update flow that modifies Hjem groups (e.g., in the admin controller/service method that handles group saves), or expose an explicit InvalidateHjemGroupCache() method that calls fusionCache.RemoveByTag(CacheTag) and invoke it after successful persistence so markers are refreshed immediately after admin updates.
🧹 Nitpick comments (1)
tasks/13-cheaper-infrastructure.md (1)
198-204:application.deployis asynchronous —curl --failonly validates job acceptance, not deployment successDeployments are asynchronous; many CRUD operations return immediately, but deployment progress must be polled until completion. The
POST /api/application.deploycall returns HTTP 200 as soon as the job is queued, so--failonly catches cases where Dokploy itself is unreachable or rejects the request — a deployment that builds, starts, and then fails health checks will still produce a green CI step.Consider one of:
- Poll the deployment status using
GET /api/deployment.allByType?id=...&type=applicationafter triggering until it transitions to a terminal state.- Use
dokploy/dokploy-action@v1— the official GitHub Action — which may abstract the polling. The Dokploy docs showuses: dokploy/dokploy-action@v1as the recommended way to trigger deployments from GitHub Actions workflows.- Rely on the health check + rollback already configured in the Swarm settings (line 232) as the safety net, and accept the async nature — document this explicitly so the team knows CI green ≠ deploy green.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/13-cheaper-infrastructure.md` around lines 198 - 204, The current curl POST to "application.deploy" (the block that uses curl with x-api-key: ${{ secrets.DOKPLOY_API_KEY }} and -d '{"applicationId":"${{ secrets.DOKPLOY_APPLICATION_ID }}"}') only validates job acceptance because application.deploy is asynchronous; update the workflow to either (A) poll the deployment status after the POST using the GET /api/deployment.allByType?id=...&type=application endpoint until the deployment reaches a terminal state and fail the step on terminal failure, or (B) replace the curl step with the official dokploy/dokploy-action@v1 to let the action handle polling, or (C) explicitly document and accept the async behavior and rely on the existing Swarm health check/rollback, making the CI step's semantics clear to the team; pick one approach and implement it for the application.deploy invocation.
🤖 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/HjemGroups/HjemGroupAdminService.cs`:
- Around line 53-104: Change SaveAsync and GeocodeAsync to return OneOf-based
results instead of void/null/exceptions: for SaveAsync (method SaveAsync) catch
blobServiceClient/container/blob UploadAsync and publishEndpoint.Publish errors
and return an appropriate OneOf (e.g., OneOf<Success, HjemGroupLoadError> or
OneOf<Success, Error<string>>) instead of letting exceptions bubble; update
callers accordingly. For GeocodeAsync (method GeocodeAsync) return
OneOf<GeocodeResult, NotFound> (or OneOf<GeocodeResult,
HjemGroupLoadError/NotFoundType>) when the Dataforsyningen response is
unsuccessful or missing fields instead of returning null; use the existing
dataForsyningenClient.SearchZipCodesAsync response checks and map parse failures
(int.TryParse) into the appropriate OneOf outcome. Ensure new result types are
used consistently where LoadAsync already uses OneOf and preserve existing
symbols like GeocodeResult, HjemGroupProvider.CacheTag, and HjemGroupEntry.
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs`:
- Around line 32-52: The catch-all in GetMarkersAsync swallows cancellations;
update the exception handling in GetMarkersAsync so OperationCanceledException
(and any cancellation originating from the provided CancellationToken or
innerToken used in fusionCache.GetOrSetAsync) is not converted into Error —
specifically, either check cancellationToken.IsCancellationRequested and rethrow
new OperationCanceledException(cancellationToken) or catch
OperationCanceledException and rethrow it before returning Error for other
exceptions; ensure this applies to both the outer method and any
OperationCanceledException thrown from the GetOrSetAsync callback (refer to
GetMarkersAsync, fusionCache.GetOrSetAsync call, and the innerToken parameter).
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 41-80: The MudLink in HjemGroupManagementPage.razor that renders
context.WebsiteUrl should include rel="noopener noreferrer" via the
UserAttributes dictionary to prevent reverse-tabnabbing; update the MudLink (the
instance with Href="@context.WebsiteUrl.ToString()") to supply UserAttributes =
new Dictionary<string, object>{{ "rel", "noopener noreferrer" }} (or reuse a
shared attribute helper) so the rendered anchor receives rel="noopener
noreferrer".
In `@tasks/13-cheaper-infrastructure.md`:
- Line 108: Update the remaining stray "CX22" mentions to "CAX11": replace the
table row text "Hetzner CX22 ARM (app only)" with "Hetzner CAX11 ARM (app
only)", change the instruction "Create Hetzner CX22 ARM server" to "Create
Hetzner CAX11 ARM server", and update the note "CX22 has 4 GB ..." to "CAX11 has
4 GB ...", ensuring all three referenced strings are consistently renamed
(search for "CX22" in the file and substitute "CAX11").
---
Duplicate comments:
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cs`:
- Around line 22-46: GetMarkersAsync is caching markers under CacheKey with tag
CacheTag but there is no evidence here of a cache invalidation path; add or
confirm a call to fusionCache.RemoveByTag(CacheTag) (or
fusionCache.Remove(CacheKey)) from the admin save/update flow that modifies Hjem
groups (e.g., in the admin controller/service method that handles group saves),
or expose an explicit InvalidateHjemGroupCache() method that calls
fusionCache.RemoveByTag(CacheTag) and invoke it after successful persistence so
markers are refreshed immediately after admin updates.
---
Nitpick comments:
In `@tasks/13-cheaper-infrastructure.md`:
- Around line 198-204: The current curl POST to "application.deploy" (the block
that uses curl with x-api-key: ${{ secrets.DOKPLOY_API_KEY }} and -d
'{"applicationId":"${{ secrets.DOKPLOY_APPLICATION_ID }}"}') only validates job
acceptance because application.deploy is asynchronous; update the workflow to
either (A) poll the deployment status after the POST using the GET
/api/deployment.allByType?id=...&type=application endpoint until the deployment
reaches a terminal state and fail the step on terminal failure, or (B) replace
the curl step with the official dokploy/dokploy-action@v1 to let the action
handle polling, or (C) explicitly document and accept the async behavior and
rely on the existing Swarm health check/rollback, making the CI step's semantics
clear to the team; pick one approach and implement it for the application.deploy
invocation.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cssrc/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cssrc/web/Jordnaer/Features/HjemGroups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Partners/PartnerService.cssrc/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razorsrc/web/Jordnaer/Pages/GroupSearch/GroupSearch.razorsrc/web/Jordnaer/Program.cssrc/web/Jordnaer/wwwroot/css/marker-cluster.csssrc/web/Jordnaer/wwwroot/js/leaflet-interop.jstasks/13-cheaper-infrastructure.mdtests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cstests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cstests/web/Jordnaer.Tests/UserSearch/DataForsyningenClientTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/web/Jordnaer/Features/Partners/PartnerService.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/web/Jordnaer.Tests/UserSearch/DataForsyningenClientTests.cs
- tests/web/Jordnaer.Tests/HjemGroups/HjemGroupAdminServiceTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor (1)
36-94: LGTM on the table structure overall, but theMudLinkis still missingrel="noopener noreferrer".The
Target="_blank"link at line 79 should includerel="noopener noreferrer"to prevent reverse-tabnabbing. Pass it viaUserAttributes:🔒 Proposed fix
- <MudLink Href="@context.WebsiteUrl.ToString()" Target="_blank"> + <MudLink Href="@context.WebsiteUrl.ToString()" Target="_blank" + UserAttributes="@(new Dictionary<string, object> { { "rel", "noopener noreferrer" } })">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` around lines 36 - 94, The MudLink with Target="_blank" in the RowTemplate needs rel="noopener noreferrer" to prevent reverse-tabnabbing; update the MudLink (component name MudLink) to pass UserAttributes including rel="noopener noreferrer" (e.g. add UserAttributes supplying the rel attribute) so external links opened in a new tab include the security attribute.src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs (1)
84-110:GeocodeAsyncstill returnsnullinstead ofOneOffor error/not-found cases.Per coding guidelines (
src/web/Jordnaer/Features/**/*.cs), service methods should useOneOf<TSuccess, TError>for explicit error handling.LoadAsyncandSaveAsyncalready follow this pattern, butGeocodeAsyncreturns a nullable, making callers handlenullchecks instead of pattern-matching on a discriminated union.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs` around lines 84 - 110, Change GeocodeAsync to return OneOf<GeocodeResult, TError> (matching the same error variant type used by LoadAsync/SaveAsync) instead of Task<GeocodeResult?>: update the method signature, replace each null return (failure to call dataForsyningenClient, missing content, invalid Visueltcenter, parse failure) with returning the appropriate OneOf error variant (e.g., OneOf.FromT1(new GeocodeNotFound() or the repository's standard error type), and return OneOf.FromT0(...) for the successful new GeocodeResult; also update all callers to pattern-match the OneOf result. Ensure you reference GeocodeAsync and GeocodeResult when making these changes so behavior matches other service methods.
🧹 Nitpick comments (2)
src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs (1)
18-19:ContainerNameandBlobNameare duplicated inHjemGroupProvider.Both this service and
HjemGroupProviderdefine identicalContainerName = "hjemlo-groups"andBlobName = "groups.json"constants. Consider extracting them to a shared location (e.g., a static class orHjemGroupEntry) to keep them in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs` around lines 18 - 19, The ContainerName and BlobName constants are duplicated between HjemGroupAdminService and HjemGroupProvider; extract these into a single shared constant (e.g., add public const strings ContainerName and BlobName to a shared type such as HjemGroupEntry or a new static class like HjemGroupStorage) and replace the private const usages in HjemGroupAdminService and HjemGroupProvider to reference the shared symbols (HjemGroupEntry.ContainerName / HjemGroupEntry.BlobName or HjemGroupStorage.ContainerName / BlobName) so both classes use the single source of truth.src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor (1)
253-315: Record value equality may causeIndexOfto match the wrong entry if duplicates exist.
HjemGroupEntryis arecord, soIndexOfuses value equality rather than reference equality. If two entries have identical property values, editing one could replace the other. This is an edge case (unlikely with distinct geocoded data) but worth being aware of.A safer approach would be to track the index directly when opening the dialog:
♻️ Sketch (optional refactor)
- private HjemGroupEntry? _editingEntry; + private int _editingIndex = -1; private void OpenEditDialog(HjemGroupEntry entry) { _editingNew = false; - _editingEntry = entry; + _editingIndex = _entries.IndexOf(entry); // ... } private void CommitEdit() { // ... - else if (_editingEntry is not null) + else if (_editingIndex >= 0 && _editingIndex < _entries.Count) { - var idx = _entries.IndexOf(_editingEntry); - if (idx >= 0) - _entries[idx] = updated; + _entries[_editingIndex] = updated; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` around lines 253 - 315, CommitEdit uses _entries.IndexOf(_editingEntry) which relies on HjemGroupEntry record value equality and can replace the wrong list item if duplicates exist; change the dialog-edit flow to record the index when opening the editor (e.g., add a private int? _editingIndex set in the method that assigns _editingEntry) and then in CommitEdit use that stored _editingIndex to update _entries[_editingIndex.Value] (fall back to adding when _editingNew is true and clear _editingIndex when closing); keep using _editingEntry only for displaying current values but use _editingIndex for the authoritative list position to avoid IndexOf/value-equality issues.
🤖 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/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 236-240: The Snackbar message currently always interpolates
result.ZipCode which shows empty parentheses when ZipCode is null; update the
Snackbar.Add call (the line invoking Snackbar.Add in
HjemGroupManagementPage.razor) to conditionally include the zip code from the
GeocodeResult (or omit the parentheses entirely) — e.g., build the message using
result.City plus (only if result.ZipCode is not null or empty) the `
({result.ZipCode})` suffix — leaving the assignments to
_form.City/_form.ZipCode/_form.Latitude/_form.Longitude unchanged.
---
Duplicate comments:
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs`:
- Around line 84-110: Change GeocodeAsync to return OneOf<GeocodeResult, TError>
(matching the same error variant type used by LoadAsync/SaveAsync) instead of
Task<GeocodeResult?>: update the method signature, replace each null return
(failure to call dataForsyningenClient, missing content, invalid Visueltcenter,
parse failure) with returning the appropriate OneOf error variant (e.g.,
OneOf.FromT1(new GeocodeNotFound() or the repository's standard error type), and
return OneOf.FromT0(...) for the successful new GeocodeResult; also update all
callers to pattern-match the OneOf result. Ensure you reference GeocodeAsync and
GeocodeResult when making these changes so behavior matches other service
methods.
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 36-94: The MudLink with Target="_blank" in the RowTemplate needs
rel="noopener noreferrer" to prevent reverse-tabnabbing; update the MudLink
(component name MudLink) to pass UserAttributes including rel="noopener
noreferrer" (e.g. add UserAttributes supplying the rel attribute) so external
links opened in a new tab include the security attribute.
---
Nitpick comments:
In `@src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cs`:
- Around line 18-19: The ContainerName and BlobName constants are duplicated
between HjemGroupAdminService and HjemGroupProvider; extract these into a single
shared constant (e.g., add public const strings ContainerName and BlobName to a
shared type such as HjemGroupEntry or a new static class like HjemGroupStorage)
and replace the private const usages in HjemGroupAdminService and
HjemGroupProvider to reference the shared symbols (HjemGroupEntry.ContainerName
/ HjemGroupEntry.BlobName or HjemGroupStorage.ContainerName / BlobName) so both
classes use the single source of truth.
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 253-315: CommitEdit uses _entries.IndexOf(_editingEntry) which
relies on HjemGroupEntry record value equality and can replace the wrong list
item if duplicates exist; change the dialog-edit flow to record the index when
opening the editor (e.g., add a private int? _editingIndex set in the method
that assigns _editingEntry) and then in CommitEdit use that stored _editingIndex
to update _entries[_editingIndex.Value] (fall back to adding when _editingNew is
true and clear _editingIndex when closing); keep using _editingEntry only for
displaying current values but use _editingIndex for the authoritative list
position to avoid IndexOf/value-equality issues.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/web/Jordnaer/Features/HjemGroups/HjemGroupAdminService.cssrc/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razortests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/web/Jordnaer.Tests/HjemGroups/HjemGroupProviderTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor (1)
236-241:⚠️ Potential issue | 🟡 MinorSnackbar still renders empty parentheses when
ZipCodeis null.Line 241 interpolates
result.ZipCodeunconditionally: when null the user sees"Fandt Randers ()". Conditionally include the zip suffix.✏️ Proposed fix
- Snackbar.Add($"Fandt {result.City} ({result.ZipCode})", Severity.Success); + var zipInfo = result.ZipCode.HasValue ? $" ({result.ZipCode})" : ""; + Snackbar.Add($"Fandt {result.City}{zipInfo}", Severity.Success);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor` around lines 236 - 241, The Snackbar message unconditionally interpolates result.ZipCode which yields empty parentheses when ZipCode is null; update the Snackbar.Add call (same area where _form.City/_form.ZipCode are set) to conditionally append the " (ZipCode)" suffix only when result.ZipCode is non-null/non-empty (e.g., use a conditional or ternary around result.ZipCode or string.IsNullOrWhiteSpace) so the message shows "Fandt City" when no zip and "Fandt City (ZipCode)" when present; keep using the existing Snackbar.Add call and Severity.Success severity.
🧹 Nitpick comments (1)
tasks/13-cheaper-infrastructure.md (1)
150-154: Make checksum verification executable, not just descriptive.Good improvement, but the current step still relies on manual comparison. Add an explicit compare command so the runbook is deterministic.
Proposed runbook tweak
# Download the installer first, verify it, then execute curl -sSL https://dokploy.com/install.sh -o dokploy-install.sh -# Verify SHA256 checksum matches the value published on https://dokploy.com/docs/get-started/installation -sha256sum dokploy-install.sh +# Fetch expected SHA256 from Dokploy docs/release page, then compare explicitly +EXPECTED_SHA256="<paste-published-sha256-here>" +echo "${EXPECTED_SHA256} dokploy-install.sh" | sha256sum -c - # Inspect the script before running less dokploy-install.sh sh dokploy-install.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/13-cheaper-infrastructure.md` around lines 150 - 154, Replace the manual sha256sum step with an explicit verification that compares the computed checksum of dokploy-install.sh to the published expected hash and fails the run if it does not match; specifically, after downloading with curl and before executing or inspecting, run a checksum-compare command that uses the expected SHA256 string for dokploy-install.sh and exits non-zero on mismatch so the installer is never executed when verification fails.
🤖 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/HjemGroups/HjemGroupProvider.cs`:
- Around line 59-95: The catch-all in LoadFromBlobAsync currently swallows
OperationCanceledException and returns Error; change the single catch(Exception
ex) into two catches: first catch (OperationCanceledException) { throw; } to
preserve cancellation semantics, then catch (Exception ex) { logger.LogError(ex,
"Failed to load HJEM group markers from blob storage."); return new Error(); }
so cancellations from blobServiceClient/blobClient calls propagate while other
exceptions are logged and wrapped as Error.
In `@tasks/13-cheaper-infrastructure.md`:
- Line 20: Replace the ambiguous claim "zero code changes required" with the
more precise phrase "zero application code changes required" wherever it appears
in this document (e.g., the summary sentence "Yes, it is viable. Estimated
monthly cost: ~$10-11 (saving ~$14/month, ~56% reduction) with zero code changes
required." and the other occurrences that reference CI/workflow edits), so
readers understand CI/workflow or infra configuration edits may still be
required even though application source code does not need changes.
---
Duplicate comments:
In `@src/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razor`:
- Around line 236-241: The Snackbar message unconditionally interpolates
result.ZipCode which yields empty parentheses when ZipCode is null; update the
Snackbar.Add call (same area where _form.City/_form.ZipCode are set) to
conditionally append the " (ZipCode)" suffix only when result.ZipCode is
non-null/non-empty (e.g., use a conditional or ternary around result.ZipCode or
string.IsNullOrWhiteSpace) so the message shows "Fandt City" when no zip and
"Fandt City (ZipCode)" when present; keep using the existing Snackbar.Add call
and Severity.Success severity.
---
Nitpick comments:
In `@tasks/13-cheaper-infrastructure.md`:
- Around line 150-154: Replace the manual sha256sum step with an explicit
verification that compares the computed checksum of dokploy-install.sh to the
published expected hash and fails the run if it does not match; specifically,
after downloading with curl and before executing or inspecting, run a
checksum-compare command that uses the expected SHA256 string for
dokploy-install.sh and exits non-zero on mismatch so the installer is never
executed when verification fails.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/web/Jordnaer/Features/HjemGroups/HjemGroupProvider.cssrc/web/Jordnaer/Pages/Backoffice/HjemGroupManagementPage.razortasks/13-cheaper-infrastructure.md
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.