Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 18 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Weather Alerts subsystem: domain models, enums, repositories, services, provider implementations and factory, caching, migrations, API endpoints, MVC views, background worker tasks, claims/authorization, DI registrations, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Weather Alert Worker
participant Service as WeatherAlertService
participant ProviderFactory as ProviderFactory
participant Provider as NwsWeatherAlertProvider
participant Repo as WeatherAlertSourceRepo
participant DB as Database
Worker->>Service: ProcessAllActiveSourcesAsync()
Service->>Repo: GetActiveSourcesForPollingAsync()
Repo-->>Service: active sources
loop per source
Service->>Service: Check poll interval & populate ContactEmail
Service->>ProviderFactory: GetProvider(source.SourceType)
ProviderFactory-->>Service: IWeatherAlertProvider
Service->>Provider: FetchAlertsAsync(source)
alt cache hit
Provider-->>Service: cached alerts
else cache miss
Provider->>Provider: Build HTTP request (area/ETag, headers)
Provider->>Provider: Parse GeoJSON -> List<WeatherAlert>
Provider->>Provider: Store in cache
Provider-->>Service: fetched alerts
end
loop per alert
Service->>Service: GetByExternalIdAndSourceIdAsync()
alt exists
Service->>Service: Update alert fields
alt has ReferencesExternalId
Service->>Repo: Mark referenced alerts Cancelled
end
else
Service->>Repo: Insert new alert
end
end
Service->>Repo: Update source LastPoll/LastSuccess/IsFailure
end
sequenceDiagram
participant Scheduler as Worker Scheduler
participant Service as WeatherAlertService
participant Repo as WeatherAlertRepository
participant Settings as DepartmentSettingsService
participant Message as MessageService
participant CallNotes as CallNotesRepository
Scheduler->>Service: SendPendingNotificationsAsync()
Service->>Repo: GetUnnotifiedAlertsAsync()
Repo-->>Service: unnotified alerts
Service->>Service: Group alerts by DepartmentId
loop per department
Service->>Settings: GetSettingByTypeAsync(dept, WeatherAlertsEnabled)
alt enabled
Service->>Settings: GetSettingByTypeAsync(dept, AutoMessageSeverity)
Service->>Service: Filter alerts >= threshold
loop per alert to notify
Service->>Message: SendBroadcastMessageAsync(...)
Message-->>Service: SystemMessageId
Service->>Repo: Update alert.SystemMessageId
end
else disabled
Service->>Repo: Mark alerts as NotificationSent (no send)
end
Service->>Repo: Mark all processed alerts NotificationSent = true
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost("SaveSource")] | ||
| [Authorize(Policy = ResgridResources.WeatherAlert_Create)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<GetWeatherAlertSourcesResult>> SaveSource([FromBody] SaveWeatherAlertSourceInput input) |
| [HttpPost("SaveZone")] | ||
| [Authorize(Policy = ResgridResources.WeatherAlert_Create)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<GetWeatherAlertZonesResult>> SaveZone([FromBody] SaveWeatherAlertZoneInput input) |
| [HttpPost("SaveSettings")] | ||
| [Authorize(Policy = ResgridResources.WeatherAlert_Update)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<GetWeatherAlertSettingsResult>> SaveSettings([FromBody] SaveWeatherAlertSettingsInput input) |
![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: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (29)
Core/Resgrid.Model/WeatherAlertSource.cs-31-33 (1)
31-33:⚠️ Potential issue | 🟠 MajorProtect provider credentials at rest.
ApiKeyis persisted as plaintext, which increases blast radius for DB-read compromises and backups. Persist an encrypted value (or a secret reference) instead of raw keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/WeatherAlertSource.cs` around lines 31 - 33, The ApiKey property on the WeatherAlertSource class is storing provider credentials in plaintext; change the model to store an encrypted value or a secret reference instead of raw keys by replacing or augmenting ApiKey with a secure field (e.g., EncryptedApiKey or ApiKeySecretId) and update persistence and retrieval flows to encrypt on write and decrypt or resolve the secret on read using your app's data protection/key vault utilities; specifically modify WeatherAlertSource.ApiKey handling, add helper methods or services for encryption/decryption (or secret resolution), and update any places that set or read ApiKey to use the new secure field so plaintext keys are never written to the DB.Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSettingsInput.cs-6-7 (1)
6-7:⚠️ Potential issue | 🟠 MajorAdd validation for severity bounds in SaveWeatherAlertSettingsInput.
Lines 6–7 accept arbitrary integers. Valid severity values are 0–4 (WeatherAlertSeverity enum: Extreme=0, Severe=1, Moderate=2, Minor=3, Unknown=4). The SaveSettings endpoint (line 335) does not validate input and directly persists invalid values. Add Range validation attributes or validate in the controller before persistence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSettingsInput.cs` around lines 6 - 7, The MinimumSeverity and AutoMessageSeverity properties on SaveWeatherAlertSettingsInput accept arbitrary ints; add validation to ensure values are within 0–4 (WeatherAlertSeverity enum). Fix by adding a Range(0,4) attribute to both properties on the SaveWeatherAlertSettingsInput class or perform explicit validation inside the controller method handling SaveSettings (e.g., the SaveSettings action) and return BadRequest when out of range before persisting; reference the SaveWeatherAlertSettingsInput class and its MinimumSeverity/AutoMessageSeverity properties and the SaveSettings endpoint to locate where to add these checks.Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs-494-496 (1)
494-496:⚠️ Potential issue | 🟠 MajorWeather-alert enrichment should not break call create/update.
These awaited calls run inside critical request paths. If
AttachWeatherAlertsToCallAsyncthrows, the user flow fails after the call is already persisted, causing partial-success behavior and retry confusion.💡 Suggested fault-isolation pattern
-// Attach weather alerts as call notes if enabled -await _weatherAlertService.AttachWeatherAlertsToCallAsync(call, cancellationToken); +// Attach weather alerts as call notes if enabled (best-effort) +try +{ + await _weatherAlertService.AttachWeatherAlertsToCallAsync(call, cancellationToken); +} +catch (Exception ex) +{ + // Log warning; don't fail the core call workflow. + // _logger?.LogWarning(ex, "Failed to attach weather alerts to call {CallId}", call.CallId); +}Also applies to: 861-863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs` around lines 494 - 496, The awaited call to _weatherAlertService.AttachWeatherAlertsToCallAsync in DispatchController is running in the critical request path and must not throw to the caller; wrap calls to AttachWeatherAlertsToCallAsync in a try/catch (both where it’s invoked at the create/update site and the other location referenced) and swallow exceptions after logging a warning (include the exception and identifying context such as call.Id and current action) so failures in weather enrichment do not break call create/update; alternatively consider firing it off as a background task (Task.Run or queued background work) with equivalent exception handling to isolate faults from the main request.Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Details.cshtml-44-45 (1)
44-45:⚠️ Potential issue | 🟠 MajorGuard enum/int-to-array lookups to prevent runtime crashes.
These direct indexes can throw if any stored alert value falls outside expected bounds. Use safe lookup with fallback labels/classes.
🛡️ Suggested safe-lookup pattern
@{ + string Safe(string[] values, int index, string fallback) => + (index >= 0 && index < values.Length) ? values[index] : fallback; + + var severityClass = Safe(severityClasses, Model.Severity, "default"); + var severityName = Safe(severityNames, Model.Severity, localizer["SeverityUnknown"].Value); } ... -<span class="label label-@(severityClasses[Model.Severity]) pull-right">@(severityNames[Model.Severity])</span> +<span class="label label-@severityClass pull-right">@severityName</span> ... -<tr><th>@localizer["Status"]</th><td>@(statusNames[Model.Status])</td></tr> -<tr><th>@localizer["Severity"]</th><td>@(severityNames[Model.Severity])</td></tr> -<tr><th>@localizer["Urgency"]</th><td>@(urgencyNames[Model.Urgency])</td></tr> -<tr><th>@localizer["Certainty"]</th><td>@(certaintyNames[Model.Certainty])</td></tr> -<tr><th>@localizer["Category"]</th><td>@(categoryNames[Model.AlertCategory])</td></tr> +<tr><th>@localizer["Status"]</th><td>@Safe(statusNames, Model.Status, "N/A")</td></tr> +<tr><th>@localizer["Severity"]</th><td>@Safe(severityNames, Model.Severity, localizer["SeverityUnknown"].Value)</td></tr> +<tr><th>@localizer["Urgency"]</th><td>@Safe(urgencyNames, Model.Urgency, "N/A")</td></tr> +<tr><th>@localizer["Certainty"]</th><td>@Safe(certaintyNames, Model.Certainty, "N/A")</td></tr> +<tr><th>@localizer["Category"]</th><td>@Safe(categoryNames, Model.AlertCategory, "N/A")</td></tr>Also applies to: 57-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Details.cshtml` around lines 44 - 45, The view directly indexes severityClasses and severityNames with Model.Severity which can throw if the enum/int is out of range; update the lookup in the <span> and the other occurrences (severityClasses, severityNames + Model.Severity) to perform a safe lookup: check that Model.Severity is within the collections' valid index/keys and use a fallback value/class when it is not (e.g., default label text and default CSS class), so replace the direct index expressions with a guarded conditional that returns severityClasses.ContainsKey/ index-in-range ? severityClasses[Model.Severity] : "default-class" and similarly for severityNames.Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertZoneInput.cs-5-11 (1)
5-11:⚠️ Potential issue | 🟠 MajorAdd request validation for zone payload invariants.
This input model currently accepts invalid values (e.g., empty
Name/ZoneCode, non-positiveRadiusMiles, malformedCenterGeoLocation), which can propagate bad configuration.✅ Proposed validation annotations
+using System.ComponentModel.DataAnnotations; + namespace Resgrid.Web.Services.Models.v4.WeatherAlerts { public class SaveWeatherAlertZoneInput { public string WeatherAlertZoneId { get; set; } + [Required] + [StringLength(200)] public string Name { get; set; } + [Required] + [StringLength(100)] public string ZoneCode { get; set; } + [Required] public string CenterGeoLocation { get; set; } + [Range(0.1, 500.0)] public double RadiusMiles { get; set; } public bool IsActive { get; set; } public bool IsPrimary { get; set; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertZoneInput.cs` around lines 5 - 11, The SaveWeatherAlertZoneInput model currently allows invalid payloads; add validation attributes and/or model-level validation to enforce invariants: mark Name and ZoneCode with [Required] and [MinLength(1)] (or [StringLength]) to prevent empty strings, constrain RadiusMiles with [Range(typeof(double), "0.0001", "10000")] (or suitable positive range) to forbid non-positive values, and validate CenterGeoLocation either with a Regex attribute for "lat,lon" format or implement IValidatableObject.Validate on SaveWeatherAlertZoneInput to parse and verify a valid latitude/longitude pair; ensure WeatherAlertZoneId can be optional or use [Required] depending on semantics and that model validation errors propagate to the controller (ModelState).Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSourceInput.cs-6-12 (1)
6-12:⚠️ Potential issue | 🟠 MajorValidate source input to protect scheduler/provider behavior.
PollIntervalMinutes,Name, and source selection fields are currently unconstrained. Invalid values can break import cadence and provider processing.✅ Proposed validation annotations
+using System.ComponentModel.DataAnnotations; + namespace Resgrid.Web.Services.Models.v4.WeatherAlerts { public class SaveWeatherAlertSourceInput { public string WeatherAlertSourceId { get; set; } + [Required] + [StringLength(200)] public string Name { get; set; } + [Range(0, int.MaxValue)] public int SourceType { get; set; } public string AreaFilter { get; set; } public string ApiKey { get; set; } public string CustomEndpoint { get; set; } + [Range(1, 1440)] public int PollIntervalMinutes { get; set; } public bool Active { get; set; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSourceInput.cs` around lines 6 - 12, The SaveWeatherAlertSourceInput model allows invalid values that can break scheduler/provider behavior; update the SaveWeatherAlertSourceInput class to add validation attributes: mark Name as required and add a max length (e.g., [Required], [StringLength(...)]) to the Name property; make SourceType and related source-selection fields (SourceType) required and restrict to valid enums/values (e.g., [Range(min,max)] or a custom validation) so invalid source IDs are rejected; enforce PollIntervalMinutes to be within safe bounds (e.g., [Range(1, 1440)] or your acceptable min/max) and mark Active, ApiKey, CustomEndpoint with appropriate optional/required attributes if needed; ensure server-side model validation is triggered where SaveWeatherAlertSourceInput is consumed.Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs-784-786 (1)
784-786:⚠️ Potential issue | 🟠 MajorIsolate weather-attachment failures from core call save/update flow.
SaveCallAsyncsucceeds before weather attachment runs. IfAttachWeatherAlertsToCallAsyncthrows, the API can return failure after data is already persisted, which can trigger duplicate client retries and partial side effects.Proposed hardening
- // Attach weather alerts as call notes if enabled - await _weatherAlertService.AttachWeatherAlertsToCallAsync(savedCall, cancellationToken); + // Attach weather alerts as call notes if enabled (best effort) + try + { + await _weatherAlertService.AttachWeatherAlertsToCallAsync(savedCall, cancellationToken); + } + catch (Exception ex) + { + Logging.LogException(ex); + } ... - // Attach weather alerts as call notes if enabled (deduplication handled inside) - await _weatherAlertService.AttachWeatherAlertsToCallAsync(call, cancellationToken); + // Attach weather alerts as call notes if enabled (deduplication handled inside, best effort) + try + { + await _weatherAlertService.AttachWeatherAlertsToCallAsync(call, cancellationToken); + } + catch (Exception ex) + { + Logging.LogException(ex); + }Also applies to: 1139-1140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs` around lines 784 - 786, The call save flow in CallsController calls _weatherAlertService.AttachWeatherAlertsToCallAsync after SaveCallAsync and currently allows exceptions from AttachWeatherAlertsToCallAsync to bubble up and fail the API despite the call being persisted; wrap the AttachWeatherAlertsToCallAsync invocation in a try/catch inside the method handling SaveCallAsync (and the duplicate location around lines ~1139) so any exception is caught, logged with context (including call id and exception), and not rethrown — alternatively dispatch the attachment to run asynchronously/fire-and-forget (e.g., Task.Run or background queue) so failures cannot change the API response; ensure the catch does not swallow required telemetry and preserves cancellationToken where appropriate.Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs-22-27 (1)
22-27:⚠️ Potential issue | 🟠 MajorDo not cache placeholder empty results for an unimplemented provider.
Right now the first fetch stores an empty list in cache, which can mask real alerts and prevent subsequent fetch attempts for that source/area during process lifetime.
Proposed fix
// TODO: Implement MeteoAlarm API integration // Endpoint: https://feeds.meteoalarm.org/api/v1/ var alerts = new List<WeatherAlert>(); -// Store in shared cache -WeatherAlertResponseCache.Set(SourceType, source.AreaFilter, alerts); +// Do not cache placeholder data until real provider integration is implemented. return alerts;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs` around lines 22 - 27, The current placeholder in MeteoAlarmWeatherAlertProvider stores an empty alerts list into WeatherAlertResponseCache via WeatherAlertResponseCache.Set(SourceType, source.AreaFilter, alerts), which masks real alerts; change the behavior so you do not cache placeholder/empty results: either throw/raise NotImplementedException or return null/avoid calling WeatherAlertResponseCache.Set until real alerts are fetched, and only call WeatherAlertResponseCache.Set(SourceType, source.AreaFilter, alerts) when alerts is non-null and contains items (alerts.Count > 0) or after the provider implementation returns a definite valid result.Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectUnnotifiedWeatherAlertsQuery.cs-28-31 (1)
28-31:⚠️ Potential issue | 🟠 MajorGeneric query overload should not throw by default.
This method is part of the interface surface but currently crashes if called.
Proposed fix
public string GetQuery<TEntity>() where TEntity : class, IEntity { - throw new System.NotImplementedException(); + return GetQuery(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectUnnotifiedWeatherAlertsQuery.cs` around lines 28 - 31, The generic GetQuery<TEntity>() in SelectUnnotifiedWeatherAlertsQuery currently throws NotImplementedException; replace it with a non-throwing implementation by delegating to the existing non-generic GetQuery() or returning the same SQL used for untyped queries so callers using the interface won't crash. Locate SelectUnnotifiedWeatherAlertsQuery and implement GetQuery<TEntity>() to call the class's GetQuery() (or return the identical query string) while preserving the generic constraint (where TEntity : class, IEntity).Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertSourcesForPollingQuery.cs-28-31 (1)
28-31:⚠️ Potential issue | 🟠 MajorGeneric overload should return SQL, not throw.
Throwing in
GetQuery<TEntity>()leaves a runtime trap in the query contract.Proposed fix
public string GetQuery<TEntity>() where TEntity : class, IEntity { - throw new System.NotImplementedException(); + return GetQuery(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertSourcesForPollingQuery.cs` around lines 28 - 31, The generic GetQuery<TEntity>() currently throws; replace the throw with returning the SQL string so callers don't hit a runtime exception. Implement GetQuery<TEntity>() to delegate to the existing non-generic query provider (e.g., call the class's GetQuery() or a GetQuery(Type entityType) helper) and return that SQL string; reference the generic method name GetQuery<TEntity>() and the IEntity constraint when locating the method to change.Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertHistoryByDepartmentQuery.cs-28-31 (1)
28-31:⚠️ Potential issue | 🟠 MajorPrevent runtime failure in generic query path.
GetQuery<TEntity>()should return a valid SQL string instead of throwing.Proposed fix
public string GetQuery<TEntity>() where TEntity : class, IEntity { - throw new System.NotImplementedException(); + return GetQuery(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertHistoryByDepartmentQuery.cs` around lines 28 - 31, The generic GetQuery<TEntity>() in SelectWeatherAlertHistoryByDepartmentQuery currently throws NotImplementedException which causes runtime failures; implement it to return the same SQL as the non-generic path by calling the existing GetQuery(Type entityType) or by returning the concrete SQL string used for department weather alert history. Specifically, in GetQuery<TEntity>() (where TEntity : class, IEntity) replace the throw with a return that delegates to GetQuery(typeof(TEntity)) or returns the exact SQL literal used by the class so generic callers receive a valid query string.Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndCategoryQuery.cs-28-31 (1)
28-31:⚠️ Potential issue | 🟠 MajorAvoid shipping an interface method that always throws.
GetQuery<TEntity>()currently throws unconditionally. If this overload is used, it will fail at runtime.Proposed fix
public string GetQuery<TEntity>() where TEntity : class, IEntity { - throw new System.NotImplementedException(); + return GetQuery(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndCategoryQuery.cs` around lines 28 - 31, The GetQuery<TEntity>() method in SelectWeatherAlertsByDepartmentAndCategoryQuery currently always throws; replace it with a real implementation that returns the SQL string instead of throwing — implement GetQuery<TEntity>() to call and return the existing non-generic GetQuery() (or the same query-building logic) so callers using the generic overload succeed; update the method body of GetQuery<TEntity>() in SelectWeatherAlertsByDepartmentAndCategoryQuery to return the query string rather than throwing NotImplementedException.Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs-22-29 (1)
22-29:⚠️ Potential issue | 🟠 MajorDon't cache an "empty success" for an unimplemented provider.
On the first miss, this stores
[]in the shared cache, so subsequent polls for the sameAreaFilternever attempt a real fetch and the source silently looks healthy while dropping every Environment Canada alert. Until CAP parsing exists, either fail fast / disable this provider, or at minimum skip caching placeholder results.Suggested fix
- var alerts = new List<WeatherAlert>(); - - // Store in shared cache - WeatherAlertResponseCache.Set(SourceType, source.AreaFilter, alerts); - - return alerts; + throw new NotSupportedException("Environment Canada weather alert fetching is not implemented yet.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs` around lines 22 - 29, The provider currently unconditionally caches an empty List<WeatherAlert> via WeatherAlertResponseCache.Set(SourceType, source.AreaFilter, alerts) which causes silent drops; change EnvironmentCanadaWeatherAlertProvider so it does not cache placeholder/empty results while CAP parsing is unimplemented—either throw/disable (e.g., throw new NotImplementedException or mark the source as disabled) or simply return without calling WeatherAlertResponseCache.Set when alerts is an empty placeholder; specifically, remove or guard the WeatherAlertResponseCache.Set call around the alerts variable (and return an error/exception or skip caching) so future polls still attempt real fetches once parsing is implemented.Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs-19-20 (1)
19-20:⚠️ Potential issue | 🟠 MajorDo not expose mutable cached list references.
Line 19 returns the same list instance stored at Line 32. Any caller mutation changes shared cache state and can leak data across requests.
💡 Proposed fix
public static bool TryGet(WeatherAlertSourceType sourceType, string areaFilter, out List<WeatherAlert> alerts) { var key = BuildKey(sourceType, areaFilter); if (_cache.TryGetValue(key, out var entry) && entry.ExpiresUtc > DateTime.UtcNow) { - alerts = entry.Alerts; + alerts = entry.Alerts == null ? null : new List<WeatherAlert>(entry.Alerts); return true; } alerts = null; return false; } public static void Set(WeatherAlertSourceType sourceType, string areaFilter, List<WeatherAlert> alerts, int? cacheMinutes = null) { var key = BuildKey(sourceType, areaFilter); var ttl = cacheMinutes ?? DefaultCacheMinutes; _cache[key] = new CacheEntry { - Alerts = alerts, + Alerts = alerts == null ? new List<WeatherAlert>() : new List<WeatherAlert>(alerts), ExpiresUtc = DateTime.UtcNow.AddMinutes(ttl) }; }Also applies to: 26-33, 49-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs` around lines 19 - 20, The cache currently returns and stores the direct list instance (entry.Alerts), exposing mutable cached state; change getters to return a shallow copy (e.g., alerts = entry.Alerts.ToList() or new List<T>(entry.Alerts)) and when saving into the cache store a copy as well so callers never receive the original list; update all occurrences that assign or return entry.Alerts (including the spots shown around entry.Alerts and the other referenced ranges) to use copies to prevent shared-mutation bugs in WeatherAlertResponseCache.Providers/Resgrid.Providers.Migrations/Migrations/M0063_AddingWeatherAlerts.cs-18-18 (1)
18-18:⚠️ Potential issue | 🟠 MajorAvoid storing provider API keys in plaintext.
Line 18 introduces
ApiKeyas plain text. If DB access is compromised, all upstream weather-provider credentials are immediately exposed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Migrations/Migrations/M0063_AddingWeatherAlerts.cs` at line 18, The migration currently creates a plaintext ApiKey column (.WithColumn("ApiKey").AsString(500).Nullable()) — change the schema and code to store API keys encrypted instead of plaintext: update the migration to create a binary/varbinary (or appropriately sized encrypted) column (e.g., EncryptedApiKey varbinary) or add an EncryptedApiKey column and remove/mark ApiKey deprecated, and update the data layer that writes/reads weather provider credentials to encrypt before persisting and decrypt on read using your app-wide key management (IDataProtectionProvider, Azure Key Vault, AWS KMS, etc.); ensure all code paths that use ApiKey (weather provider config loading/WeatherAlert logic) are updated to call the encrypt/decrypt helpers and existing plaintext values are migrated securely.Workers/Resgrid.Workers.Console/Tasks/WeatherAlertImportTask.cs-44-48 (1)
44-48:⚠️ Potential issue | 🟠 MajorDo not swallow handler exceptions.
Lines 44-48 log and continue. That can mark failed imports as successful runs and suppress scheduler retry/alert behavior.
💡 Proposed fix
catch (Exception ex) { Resgrid.Framework.Logging.LogException(ex); - _logger.LogError(ex.ToString()); + _logger.LogError(ex, "WeatherAlertImport task failed"); + throw; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Tasks/WeatherAlertImportTask.cs` around lines 44 - 48, The catch block in WeatherAlertImportTask (the catch(Exception ex) that calls Resgrid.Framework.Logging.LogException(ex) and _logger.LogError(ex.ToString())) swallows exceptions so failures are treated as successful; after logging, rethrow the exception (use throw; to preserve stack) or propagate it out of the WeatherAlertImportTask method (e.g., remove the catch or rethrow) so the scheduler can observe the failure and trigger retries/alerts.Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndSeverityQuery.cs-28-31 (1)
28-31:⚠️ Potential issue | 🟠 MajorImplement
GetQuery<TEntity>()instead of throwing.Line 30 currently throws
NotImplementedException, which will crash any generic call path using this query type.💡 Proposed fix
public string GetQuery<TEntity>() where TEntity : class, IEntity { - throw new System.NotImplementedException(); + return GetQuery(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndSeverityQuery.cs` around lines 28 - 31, Replace the NotImplementedException in SelectWeatherAlertsByDepartmentAndSeverityQuery.GetQuery<TEntity>() with an actual query string builder that returns the SQL (or ORM query) to select weather alerts filtered by department id and severity; implement the generic method to ignore/validate TEntity (IEntity) as needed, reference the class SelectWeatherAlertsByDepartmentAndSeverityQuery and method GetQuery<TEntity>(), ensure the returned value is the complete query string used elsewhere (matching the existing parameter names for department and severity) and removes the throw so generic callers can execute the query.Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs-1665-1668 (1)
1665-1668:⚠️ Potential issue | 🟠 MajorFilter out expired alerts in “active” queries.
Lines 1665, 1667, and 1668 return
Status = 0only. Expired rows can still appear as active until the background expiration pass updates status.💡 Proposed fix
-SelectActiveWeatherAlertsByDepartmentIdQuery = "SELECT * FROM %SCHEMA%.%TABLENAME% WHERE DepartmentId = %DEPARTMENTID% AND Status = 0"; +SelectActiveWeatherAlertsByDepartmentIdQuery = "SELECT * FROM %SCHEMA%.%TABLENAME% WHERE DepartmentId = %DEPARTMENTID% AND Status = 0 AND (ExpiresUtc IS NULL OR ExpiresUtc > NOW() AT TIME ZONE 'UTC')"; -SelectWeatherAlertsByDepartmentAndSeverityQuery = "SELECT * FROM %SCHEMA%.%TABLENAME% WHERE DepartmentId = %DEPARTMENTID% AND Severity <= %MAXSEVERITY% AND Status = 0"; +SelectWeatherAlertsByDepartmentAndSeverityQuery = "SELECT * FROM %SCHEMA%.%TABLENAME% WHERE DepartmentId = %DEPARTMENTID% AND Severity <= %MAXSEVERITY% AND Status = 0 AND (ExpiresUtc IS NULL OR ExpiresUtc > NOW() AT TIME ZONE 'UTC')"; -SelectWeatherAlertsByDepartmentAndCategoryQuery = "SELECT * FROM %SCHEMA%.%TABLENAME% WHERE DepartmentId = %DEPARTMENTID% AND AlertCategory = %CATEGORY% AND Status = 0"; +SelectWeatherAlertsByDepartmentAndCategoryQuery = "SELECT * FROM %SCHEMA%.%TABLENAME% WHERE DepartmentId = %DEPARTMENTID% AND AlertCategory = %CATEGORY% AND Status = 0 AND (ExpiresUtc IS NULL OR ExpiresUtc > NOW() AT TIME ZONE 'UTC')";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs` around lines 1665 - 1668, The "active" weather alert queries (SelectActiveWeatherAlertsByDepartmentIdQuery, SelectWeatherAlertsByDepartmentAndSeverityQuery, SelectWeatherAlertsByDepartmentAndCategoryQuery) currently filter only on Status = 0 and can return expired alerts; update each SQL string to also exclude expired rows by adding an ExpirationDate check such as AND (ExpirationDate IS NULL OR ExpirationDate > NOW()) (or CURRENT_TIMESTAMP) so only non-expired alerts are returned.Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtml-111-116 (1)
111-116:⚠️ Potential issue | 🟠 MajorUse local-date formatting for
<input type="date">defaults.Lines 115-116 use
toISOString(), which is UTC-based and can shift the date by one day for local users.💡 Proposed fix
+function toLocalDateInputValue(date) { + var local = new Date(date.getTime() - (date.getTimezoneOffset() * 60000)); + return local.toISOString().split('T')[0]; +} + $('#endDate').val(today.toISOString().split('T')[0]); $('#startDate').val(thirtyDaysAgo.toISOString().split('T')[0]); +$('#endDate').val(toLocalDateInputValue(today)); +$('#startDate').val(toLocalDateInputValue(thirtyDaysAgo));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtml` around lines 111 - 116, The current code sets date inputs '#endDate' and '#startDate' using toISOString() which is UTC and can shift the displayed day for local users; update the assignment to format the local Date objects (today and thirtyDaysAgo) into a "YYYY-MM-DD" string using the local date components (getFullYear(), getMonth()+1, getDate()) or toLocaleDateString('en-CA') and then set those strings into $('#endDate').val(...) and $('#startDate').val(...) so the inputs show the local calendar date instead of the UTC-shifted date.Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs-17-23 (1)
17-23:⚠️ Potential issue | 🟠 MajorEvict expired entries during
TryGetto prevent stale-key buildup.Expired records are treated as misses but never removed, so
_cachecan grow indefinitely with high area/source churn.💡 Proposed fix
if (_cache.TryGetValue(key, out var entry) && entry.ExpiresUtc > DateTime.UtcNow) { alerts = entry.Alerts; return true; } +if (entry != null && entry.ExpiresUtc <= DateTime.UtcNow) +{ + _cache.TryRemove(key, out _); +} alerts = null; return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs` around lines 17 - 23, The current TryGet logic returns a miss for expired entries but leaves them in _cache, causing growth; update the WeatherAlertResponseCache.TryGet (the block using _cache.TryGetValue, key, entry, entry.ExpiresUtc and entry.Alerts) so that when an entry is found but entry.ExpiresUtc <= DateTime.UtcNow you remove it from the cache (call _cache.Remove(key) or _cache.TryRemove(key) depending on whether _cache is a Dictionary or ConcurrentDictionary), then set alerts = null and return false; leave the non-expired hit path unchanged.Workers/Resgrid.Workers.Console/Tasks/WeatherAlertImportTask.cs-31-31 (1)
31-31:⚠️ Potential issue | 🟠 MajorInject
IWeatherAlertServicevia constructor instead of runtime resolution; this hidden dependency violates testability design and makes unit testing problematic.Currently the service is resolved at line 31 via
Bootstrapper.GetKernel().Resolve<IWeatherAlertService>(), which hides the dependency and prevents isolated testing. AddIWeatherAlertServiceto the constructor alongside the existingILoggerparameter, store it as a field, and use it in the method body. This keeps dependencies explicit and maintains testability.Proposed refactor
public class WeatherAlertImportTask : IQuidjiboHandler<WeatherAlertImportCommand> { + private readonly IWeatherAlertService _weatherAlertService; public string Name => "Weather Alert Import"; public int Priority => 1; public ILogger _logger; - public WeatherAlertImportTask(ILogger logger) + public WeatherAlertImportTask(ILogger logger, IWeatherAlertService weatherAlertService) { _logger = logger; + _weatherAlertService = weatherAlertService; } public async Task ProcessAsync(WeatherAlertImportCommand command, IQuidjiboProgress progress, CancellationToken cancellationToken) { try { progress.Report(1, $"Starting the {Name} Task"); - - var weatherAlertService = Bootstrapper.GetKernel().Resolve<IWeatherAlertService>(); _logger.LogInformation("WeatherAlertImport::Processing all active sources"); - await weatherAlertService.ProcessAllActiveSourcesAsync(cancellationToken); + await _weatherAlertService.ProcessAllActiveSourcesAsync(cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Tasks/WeatherAlertImportTask.cs` at line 31, The WeatherAlertImportTask currently hides a dependency by resolving IWeatherAlertService at runtime; update the WeatherAlertImportTask class to accept IWeatherAlertService in its constructor alongside the existing ILogger parameter, assign it to a private readonly field (e.g., _weatherAlertService) and remove the Bootstrapper.GetKernel().Resolve<IWeatherAlertService>() call in the method (e.g., Execute/RunTask), using the injected _weatherAlertService instead; also update any DI registration or tests to provide the new constructor parameter.Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Zones.cshtml-77-87 (1)
77-87:⚠️ Potential issue | 🟠 MajorPotential XSS vulnerability in inline onclick handlers.
The string escaping only handles single quotes but misses backslashes, double quotes, and other special characters. If
zone.Name,zone.ZoneCode, orzone.CenterGeoLocationcontain malicious content, it could break out of the JavaScript string or execute arbitrary code.Consider using
data-*attributes with proper HTML encoding and reading values in JavaScript:🛡️ Suggested safer approach
-<a class="btn btn-xs btn-info" href="javascript:void(0)" onclick="editZone('@zone.WeatherAlertZoneId', '@zone.Name.Replace("'", "\\'")','@(zone.ZoneCode ?? "")','@(zone.CenterGeoLocation ?? "")',`@zone.RadiusMiles`,`@zone.IsPrimary.ToString`().ToLower(),`@zone.IsActive.ToString`().ToLower())"><i class="fa fa-pencil"></i> `@localizer`["Edit"]</a> +<a class="btn btn-xs btn-info" href="javascript:void(0)" + data-id="@zone.WeatherAlertZoneId" + data-name="@zone.Name" + data-code="@zone.ZoneCode" + data-center="@zone.CenterGeoLocation" + data-radius="@zone.RadiusMiles" + data-primary="@zone.IsPrimary.ToString().ToLower()" + data-active="@zone.IsActive.ToString().ToLower()" + onclick="editZoneFromElement(this)"><i class="fa fa-pencil"></i> `@localizer`["Edit"]</a>Then in JavaScript:
function editZoneFromElement(el) { var $el = $(el); editZone($el.data('id'), $el.data('name'), $el.data('code'), $el.data('center'), $el.data('radius'), $el.data('primary'), $el.data('active')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Zones.cshtml` around lines 77 - 87, The inline onclick handlers for editZone, toggleZone and deleteZone use interpolated values (zone.Name, zone.ZoneCode, zone.CenterGeoLocation) with inadequate escaping, creating an XSS risk; replace these inline JS calls with HTML-encoded data-* attributes on the action links (e.g., data-id="@zone.WeatherAlertZoneId", data-name="@Html.AttributeEncode(zone.Name)", data-code="@Html.AttributeEncode(zone.ZoneCode ?? "")", data-center="@Html.AttributeEncode(zone.CenterGeoLocation ?? "")", data-radius="@zone.RadiusMiles", data-primary="@zone.IsPrimary.ToString().ToLower()", data-active="@zone.IsActive.ToString().ToLower()") and remove the onclick arguments, then bind click handlers in your JavaScript to read values via dataset or jQuery data() and call editZone/toggleZone/deleteZone with those safe values.Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs-421-423 (1)
421-423:⚠️ Potential issue | 🟠 MajorDon't expose provider API keys in the v4 response.
GetSources/SaveSourcereturnApiKeyback to the client verbatim. These are secrets; once they are in the response body they end up in browser state, logs, and dev tools. Return a masked/has-value flag instead, and preserve the stored key on update when the client sends an empty value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 421 - 423, The v4 responses currently include provider secrets via the ApiKey field (see GetSources/SaveSource and the WeatherAlert source DTO construction), so change the response model to stop returning the raw ApiKey: replace ApiKey with a masked indicator (e.g., HasApiKey or MaskedApiKey) in the returned DTO and ensure the server never serializes the actual key back to clients; update SaveSource handling so that when the incoming request contains an empty/omitted ApiKey you preserve the existing stored key instead of overwriting it, and only update the stored key when a non-empty value is provided.Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs-65-80 (1)
65-80:⚠️ Potential issue | 🟠 MajorDon't treat
304 NotModifiedas “no alerts”.
FetchAlertsAsyncreturns a full snapshot, but this branch returns an empty list. Ifsource.LastETagsurvives a restart whileWeatherAlertResponseCachedoes not, unchanged alerts disappear from the snapshot and can be expired/processed incorrectly. Only sendIf-None-Matchwhen you also have a reusable cached payload, or return that cached snapshot on 304.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs` around lines 65 - 80, FetchAlertsAsync currently sends If-None-Match with source.LastETag and treats 304 NotModified as returning an empty list, which can drop alerts if the payload cache is missing; modify the logic to consult the WeatherAlertResponseCache (or a cached payload associated with the source) before adding the If-None-Match header and, if you do send the header, return the cached alert snapshot when response.StatusCode == HttpStatusCode.NotModified instead of an empty list. Concretely: in FetchAlertsAsync, only add the IfNoneMatch header when a cached response exists for the source (use the same cache key used elsewhere), and if _httpClient.SendAsync returns 304, load and return the cached alerts from WeatherAlertResponseCache; also ensure source.LastETag and the cache are updated when you fetch a new payload.Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs-160-174 (1)
160-174:⚠️ Potential issue | 🟠 MajorReject malformed source IDs instead of creating a new source.
If
WeatherAlertSourceIdis present but not a valid GUID, execution falls into the create branch. A broken update request should return400, not silently create a duplicate poller.🛠️ Suggested fix
- if (!string.IsNullOrWhiteSpace(input.WeatherAlertSourceId) && Guid.TryParse(input.WeatherAlertSourceId, out var sourceGuid)) - { - source = await _weatherAlertService.GetSourceByIdAsync(sourceGuid); - if (source == null || source.DepartmentId != DepartmentId) - return NotFound(); - } - else + if (!string.IsNullOrWhiteSpace(input.WeatherAlertSourceId)) + { + if (!Guid.TryParse(input.WeatherAlertSourceId, out var sourceGuid)) + return BadRequest(); + + source = await _weatherAlertService.GetSourceByIdAsync(sourceGuid); + if (source == null || source.DepartmentId != DepartmentId) + return NotFound(); + } + else { source = new WeatherAlertSource {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 160 - 174, The handler in WeatherAlertsController currently treats a present but non-GUID WeatherAlertSourceId as "no id" and creates a new WeatherAlertSource; change the flow so that if input.WeatherAlertSourceId is not null/whitespace and Guid.TryParse fails you immediately return a 400 BadRequest (e.g., with a short message like "Invalid WeatherAlertSourceId"), otherwise proceed to call _weatherAlertService.GetSourceByIdAsync(sourceGuid) and the existing ownership check, and only create a new WeatherAlertSource when WeatherAlertSourceId is not provided at all.Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs-254-267 (1)
254-267:⚠️ Potential issue | 🟠 MajorMalformed zone IDs should return
400.A non-empty but invalid
WeatherAlertZoneIdcurrently falls through to the create branch and inserts a new zone. That makes update bugs silently create duplicates.🛠️ Suggested fix
- if (!string.IsNullOrWhiteSpace(input.WeatherAlertZoneId) && Guid.TryParse(input.WeatherAlertZoneId, out var zoneGuid)) - { - zone = await _weatherAlertService.GetZoneByIdAsync(zoneGuid); - if (zone == null || zone.DepartmentId != DepartmentId) - return NotFound(); - } - else + if (!string.IsNullOrWhiteSpace(input.WeatherAlertZoneId)) + { + if (!Guid.TryParse(input.WeatherAlertZoneId, out var zoneGuid)) + return BadRequest(); + + zone = await _weatherAlertService.GetZoneByIdAsync(zoneGuid); + if (zone == null || zone.DepartmentId != DepartmentId) + return NotFound(); + } + else { zone = new WeatherAlertZone {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 254 - 267, The code currently treats a non-empty but non-GUID WeatherAlertZoneId as "create new" which causes accidental inserts; update the logic in the controller to validate WeatherAlertZoneId: if input.WeatherAlertZoneId is not null/whitespace then attempt Guid.TryParse — if TryParse fails return BadRequest(); if parsed, call _weatherAlertService.GetZoneByIdAsync(zoneGuid) and keep the existing NotFound check (zone == null || zone.DepartmentId != DepartmentId) before proceeding; only fall back to creating a new WeatherAlertZone when WeatherAlertZoneId is null/whitespace.Tests/Resgrid.Tests/Providers/NwsWeatherAlertProviderTests.cs-16-27 (1)
16-27:⚠️ Potential issue | 🟠 MajorExtract mapping helpers into testable collaborators or inject HttpMessageHandler for full end-to-end testing.
The tests locally duplicate the provider's private mapping methods (
MapSeverity,MapUrgency,MapCertainty,MapCategory) and JSON property helpers (GetStringProp,GetDateProp), validating them only in isolation. This meansFetchAlertsAsyncis never exercised; regressions in the provider's integration logic or property extraction would not be caught. The staticHttpClientprevents dependency injection of mocks.Extract these mappers into a separate, injectable collaborator (or expose them as internal static utilities) so tests can verify the complete alert parsing flow, or refactor the provider to accept an
HttpMessageHandlerfor HTTP mocking in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Providers/NwsWeatherAlertProviderTests.cs` around lines 16 - 27, The tests duplicate private mapping and JSON helper logic and cannot exercise FetchAlertsAsync because the provider uses a static HttpClient; to fix this, refactor the provider to (a) extract MapSeverity, MapUrgency, MapCertainty, MapCategory and GetStringProp/GetDateProp into a new injectable collaborator or internal static utility class so tests can call the real mapping logic, and (b) replace the static HttpClient with an instance HttpClient constructed from an injected HttpMessageHandler (or accept an IHttpClientFactory) so unit tests can provide a mock handler and run FetchAlertsAsync end-to-end; update constructor and usages in the provider to accept the collaborator and handler/factory and adjust tests to inject mocks that exercise full parsing flow.Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs-51-88 (1)
51-88:⚠️ Potential issue | 🟠 MajorDispose the HTTP request and response objects.
Both
HttpRequestMessageandHttpResponseMessageimplementIDisposableand must be explicitly disposed. Currently, the response is not disposed on any code path (early return on 304 at line 80, exception fromEnsureSuccessStatusCode()at line 82, early return at line 118, or normal return at line 198). The request is also never disposed afterSendAsynccompletes. In a polling loop, this accumulates undisposed resources and exhausts the connection pool.Use
using var request = new HttpRequestMessage(...)andusing var response = await _httpClient.SendAsync(request, ct)to ensure cleanup on all paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/NwsWeatherAlertProvider.cs` around lines 51 - 88, The HttpRequestMessage and HttpResponseMessage in Get/Send flow are not disposed, leaking sockets; wrap creation of the request and the await _httpClient.SendAsync(request, ct) call in using (e.g. using var request = new HttpRequestMessage(...); using var response = await _httpClient.SendAsync(request, ct)) so both the request and response are disposed on all paths (including when response.StatusCode == NotModified, when EnsureSuccessStatusCode throws, or on normal return) while still preserving the existing logic that reads response.Headers.ETag into source.LastETag and response.Content.ReadAsStringAsync().Core/Resgrid.Services/WeatherAlertService.cs-369-375 (1)
369-375:⚠️ Potential issue | 🟠 Major
SystemMessageIdwill be set to 0 becausemessage.MessageIdis never populated.
SendMessageAsyncwithbroadcastSingle=truecreates newMessageobjects internally and saves those, never mutating the originalmessageparameter. The passed message object remains unsaved withMessageIdat its default value of 0. To capture the actual saved message ID, either callSaveMessageAsyncfirst to get the persisted message, or refactorSendMessageAsyncto return the saved message along with the success boolean.Current pattern
var message = new Message { /* ... */ }; var sent = await _messageService.SendMessageAsync( message, "Weather Alert System", departmentId, true, ct); if (sent) { alert.SystemMessageId = message.MessageId; // Still 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 369 - 375, The code in WeatherAlertService sets alert.SystemMessageId from message.MessageId after calling SendMessageAsync, but SendMessageAsync(broadcastSingle=true) creates and saves its own Message instances and does not populate the passed-in message, so message.MessageId stays 0; fix by ensuring you capture the persisted message ID — either call SaveMessageAsync (or whatever persistence method exists) to persist the Message first and use the returned MessageId before sending, or change SendMessageAsync to return the saved Message (or its id) along with the success flag so you can set alert.SystemMessageId from that returned persisted message; update references to SendMessageAsync, SaveMessageAsync, alert.SystemMessageId, and message.MessageId accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6232f9ab-8cc5-47c4-9a36-3b6e588d7211
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (97)
Core/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.csCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/Events/EventTypes.csCore/Resgrid.Model/Providers/IWeatherAlertProvider.csCore/Resgrid.Model/Providers/IWeatherAlertProviderFactory.csCore/Resgrid.Model/Repositories/IWeatherAlertRepository.csCore/Resgrid.Model/Repositories/IWeatherAlertSourceRepository.csCore/Resgrid.Model/Repositories/IWeatherAlertZoneRepository.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Model/Services/IWeatherAlertService.csCore/Resgrid.Model/WeatherAlert.csCore/Resgrid.Model/WeatherAlertCategory.csCore/Resgrid.Model/WeatherAlertCertainty.csCore/Resgrid.Model/WeatherAlertSeverity.csCore/Resgrid.Model/WeatherAlertSource.csCore/Resgrid.Model/WeatherAlertSourceType.csCore/Resgrid.Model/WeatherAlertStatus.csCore/Resgrid.Model/WeatherAlertUrgency.csCore/Resgrid.Model/WeatherAlertZone.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/WeatherAlertService.csProviders/Resgrid.Providers.Claims/ClaimsLogic.csProviders/Resgrid.Providers.Claims/ClaimsPrincipalFactory.csProviders/Resgrid.Providers.Claims/JwtTokenProvider.csProviders/Resgrid.Providers.Claims/ResgridClaimTypes.csProviders/Resgrid.Providers.Claims/ResgridIdentity.csProviders/Resgrid.Providers.Claims/ResgridResources.csProviders/Resgrid.Providers.Migrations/Migrations/M0063_AddingWeatherAlerts.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0063_AddingWeatherAlertsPg.csProviders/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.csProviders/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.csProviders/Resgrid.Providers.Weather/NwsWeatherAlertProvider.csProviders/Resgrid.Providers.Weather/Resgrid.Providers.Weather.csprojProviders/Resgrid.Providers.Weather/WeatherAlertProviderFactory.csProviders/Resgrid.Providers.Weather/WeatherAlertResponseCache.csProviders/Resgrid.Providers.Weather/WeatherProviderModule.csRepositories/Resgrid.Repositories.DataRepository/Configs/SqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Modules/ApiDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/NonWebDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertSourcesForPollingQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertZonesByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertsByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectExpiredUnprocessedWeatherAlertsQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectUnnotifiedWeatherAlertsQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertByExternalIdAndSourceIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertHistoryByDepartmentQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertSourcesByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertZonesByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndCategoryQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndSeverityQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csRepositories/Resgrid.Repositories.DataRepository/WeatherAlertRepository.csRepositories/Resgrid.Repositories.DataRepository/WeatherAlertSourceRepository.csRepositories/Resgrid.Repositories.DataRepository/WeatherAlertZoneRepository.csResgrid.slnTests/Resgrid.Tests/Providers/NwsWeatherAlertProviderTests.csTests/Resgrid.Tests/Resgrid.Tests.csprojTests/Resgrid.Tests/Services/WeatherAlertServiceTests.csWeb/Resgrid.Web.Eventing/Hubs/EventingHub.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/GetActiveWeatherAlertsResult.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/GetWeatherAlertResult.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/GetWeatherAlertSettingsResult.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/GetWeatherAlertSourcesResult.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/GetWeatherAlertZonesResult.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSettingsInput.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSourceInput.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertZoneInput.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertResultData.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSettingsData.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSourceResultData.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertZoneResultData.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.csprojWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Controllers/WeatherAlertsController.csWeb/Resgrid.Web/Areas/User/Views/Notifications/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Details.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Zones.cshtmlWeb/Resgrid.Web/Resgrid.Web.csprojWeb/Resgrid.Web/Startup.csWorkers/Resgrid.Workers.Console/Commands/WeatherAlertImportCommand.csWorkers/Resgrid.Workers.Console/Program.csWorkers/Resgrid.Workers.Console/Tasks/WeatherAlertImportTask.csWorkers/Resgrid.Workers.Framework/Bootstrapper.csWorkers/Resgrid.Workers.Framework/Resgrid.Workers.Framework.csproj
Comment on lines
+151
to
+173
| public async Task WeatherAlertReceived(int departmentId, string alertId) | ||
| { | ||
| var group = Clients.Group(departmentId.ToString()); | ||
|
|
||
| if (group != null) | ||
| await group.SendAsync("WeatherAlertReceived", alertId); | ||
| } | ||
|
|
||
| public async Task WeatherAlertExpired(int departmentId, string alertId) | ||
| { | ||
| var group = Clients.Group(departmentId.ToString()); | ||
|
|
||
| if (group != null) | ||
| await group.SendAsync("WeatherAlertExpired", alertId); | ||
| } | ||
|
|
||
| public async Task WeatherAlertUpdated(int departmentId, string alertId) | ||
| { | ||
| var group = Clients.Group(departmentId.ToString()); | ||
|
|
||
| if (group != null) | ||
| await group.SendAsync("WeatherAlertUpdated", alertId); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ASP.NET Core SignalR, can connected clients invoke any public Hub method, and does applying [AllowAnonymous] to the Hub allow unauthenticated clients to call those methods?
💡 Result:
Yes. In ASP.NET Core SignalR, clients invoke hub methods by name, and public hub methods are callable by connected clients (that’s effectively what “exposes” a hub method to clients). [1][2]
Authorization is separate:
- To require authentication/authorization, you apply
[Authorize](or authorization policies) to the hub (or specific hub methods). [3] - Applying
[AllowAnonymous]doesn’t “open up” methods beyond that—it only bypasses authorization checks (including default/fallback policies) for the endpoint. So it matters only if you’ve otherwise required authorization; if the hub isn’t protected in the first place, it’s basically a no-op. [4]
Bottom line: clients can call any public hub method they know the name of unless you restrict it (e.g., [Authorize]/policies). [AllowAnonymous] merely allows unauthenticated access where authorization would otherwise block it; it doesn’t add any extra capability beyond “skip auth.” [1][2][4]
Sources: [1] (learn.microsoft.com) [2] (learn.microsoft.com) [3] (learn.microsoft.com) [4] (learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/aspnet/core/signalr/javascript-client?view=aspnetcore-9.0&utm_source=openai
- 2: https://learn.microsoft.com/en-us/aspnet/signalr/overview/security/introduction-to-security?utm_source=openai
- 3: https://learn.microsoft.com/en-us/aspnet/core/signalr/authn-and-authz?view=aspnetcore-9.0&utm_source=openai
- 4: https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.authorizationendpointconventionbuilderextensions.allowanonymous?view=aspnetcore-10.0&utm_source=openai
🏁 Script executed:
rg -A 2 "class EventingHub" Web/Resgrid.Web.Eventing/Hubs/EventingHub.csRepository: Resgrid/Core
Length of output: 158
🏁 Script executed:
head -20 Web/Resgrid.Web.Eventing/Hubs/EventingHub.csRepository: Resgrid/Core
Length of output: 544
🏁 Script executed:
rg -B 5 "public class EventingHub" Web/Resgrid.Web.Eventing/Hubs/EventingHub.csRepository: Resgrid/Core
Length of output: 172
🏁 Script executed:
sed -n '151,173p' Web/Resgrid.Web.Eventing/Hubs/EventingHub.csRepository: Resgrid/Core
Length of output: 716
Don't expose broadcast methods on an anonymous hub.
Because EventingHub is [AllowAnonymous], any connected client can invoke these public methods and send fake WeatherAlert* events to any department group by choosing the departmentId. These notifications should be emitted from server code via IHubContext<EventingHub> or another internal notifier, not exposed as client-callable hub methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Eventing/Hubs/EventingHub.cs` around lines 151 - 173, The
three public hub methods (WeatherAlertReceived, WeatherAlertExpired,
WeatherAlertUpdated) on EventingHub must not be client-invokable because
EventingHub is [AllowAnonymous]; remove or make these methods non-public and
instead emit those notifications from server-side code using
IHubContext<EventingHub>. Replace the public methods by creating a server-only
notifier (e.g., WeatherAlertNotifier) that injects IHubContext<EventingHub> and
implements methods like NotifyWeatherAlertReceived/Expired/Updated which call
hubContext.Clients.Group(departmentId.ToString()).SendAsync(...); update callers
to use that notifier and keep EventingHub as a connection-only hub without
public broadcast methods.
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
1709-1787: Add parameter-level XML docs for Weather Alerts endpoints.The new summaries are helpful, but methods with multiple primitive args should include
<param>(and where useful<returns>) so generated API docs are unambiguous for clients.Suggested doc additions
<member name="M:Resgrid.Web.Services.Controllers.v4.WeatherAlertsController.GetAlertsNearLocation(System.Double,System.Double,System.Double)"> <summary> Gets active weather alerts near a geographic location </summary> + <param name="latitude">Latitude in decimal degrees.</param> + <param name="longitude">Longitude in decimal degrees.</param> + <param name="radiusKm">Search radius in kilometers.</param> </member> @@ <member name="M:Resgrid.Web.Services.Controllers.v4.WeatherAlertsController.ValidateCustomEndpoint(System.String,System.Int32)"> <summary> Validates a custom endpoint URL to prevent SSRF. Enforces HTTPS and restricts the host to known weather API domains per source type. Returns null if the URL is empty, invalid, or not on the allowlist. </summary> + <param name="url">Candidate endpoint URL.</param> + <param name="sourceType">Weather source/provider type identifier.</param> + <returns>Normalized URL if valid; otherwise null.</returns> </member>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 1709 - 1787, Add missing parameter-level XML docs and useful <returns> notes for the WeatherAlertsController methods: add <param> entries for GetWeatherAlert(id) describing the id string; GetAlertHistory(start, end) describing start and end DateTime and expected inclusive/exclusive behavior plus a <returns> describing returned collection; GetAlertsNearLocation(lat, lon, radiusKm) describing each numeric param and units; DeleteSource(id) and DeleteZone(id) describing the id param and add <returns> for success/failure; ValidateCustomEndpoint(url, sourceType) describing url and sourceType and document <returns> when null or allowed; NormalizeAreaFilter(areaFilter) and FormatAreaFilterForDisplay(areaFilter) describing the input string and the output format; also ensure SaveSource(SaveWeatherAlertSourceInput), SaveZone(SaveWeatherAlertZoneInput) and SaveSettings(SaveWeatherAlertSettingsInput) have <param> describing the input model and a <returns> describing created/updated behavior. Use the exact method names shown (e.g., GetWeatherAlert(System.String), GetAlertHistory(System.DateTime,System.DateTime), GetAlertsNearLocation(System.Double,System.Double,System.Double), ValidateCustomEndpoint(System.String,System.Int32)) so generated API docs are unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs`:
- Around line 39-63: The test fixture currently instantiates mocks in the
fixture constructor method with_the_weather_alert_service(), causing shared mock
state across tests; move the mock initializations for _weatherAlertRepoMock,
_weatherAlertSourceRepoMock, _weatherAlertZoneRepoMock, _providerFactoryMock,
_departmentSettingsRepoMock, _departmentsServiceMock, _messageServiceMock,
_callNotesRepoMock, _cacheProviderMock, and _eventAggregatorMock and the
WeatherAlertService construction into a method annotated with [SetUp] (or
alternatively annotate the test class with
[FixtureLifeCycle(LifeCycle.InstancePerTestCase)]) so each test gets fresh mocks
and the WeatherAlertService is recreated per test to avoid invocation-count
leakage that breaks Times.Once()/Times.Never() assertions.
In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs`:
- Around line 131-148: GetSources (and other read endpoints like the SaveSource
response) are currently copying source.ApiKey into response DTOs via
MapSourceToResultData, which leaks stored provider secrets; update
MapSourceToResultData to omit the ApiKey value for read responses (e.g., set
ApiKey to null or a masked value) and instead expose a non-sensitive indicator
like HasApiKey (boolean) on the GetWeatherAlertSourcesResult item; ensure the
same change is applied to all usages noted (lines around 189-200 and 419-436) so
no stored API keys are serialized back to clients.
- Around line 459-483: The NormalizeAreaFilter helper currently treats inputs
beginning with '[' that fail JsonSerializer.Deserialize<string[]>() as CSV and
splits them, which causes malformed JSON (e.g. ["WAZ021",]) to be silently
converted into bogus area codes; change NormalizeAreaFilter so that when
trimmed.StartsWith("[") and JsonSerializer.Deserialize<string[]>(trimmed)
throws, you do not fall back to comma-splitting — instead catch the
deserialization failure and return null (or otherwise clear/reject the value) so
malformed JSON is not reinterpreted as CSV; update the logic around
JsonSerializer.Deserialize and the trimmed.StartsWith("[") branch (within
NormalizeAreaFilter) to implement this behavior.
- Around line 160-177: The current logic in WeatherAlertsController (the block
checking input.WeatherAlertSourceId and using Guid.TryParse) accidentally
creates a new WeatherAlertSource when a non-empty but malformed ID is provided;
change the flow so that if input.WeatherAlertSourceId is not null/whitespace and
Guid.TryParse(...) returns false you immediately return BadRequest() instead of
falling into the "create new source" branch—keep the existing permission check
(User.HasClaim(...)) and the subsequent GetSourceByIdAsync(sourceGuid) lookup
and NotFound/Forbid behavior when parsing succeeds.
- Around line 257-273: In SaveZone in WeatherAlertsController, when
input.WeatherAlertZoneId is non-empty but Guid.TryParse fails, do not fall
through to creating a new WeatherAlertZone; instead return BadRequest().
Specifically, change the branch that currently tests "if
(!string.IsNullOrWhiteSpace(input.WeatherAlertZoneId) && Guid.TryParse(..., out
var zoneGuid)) { ... } else { create new zone }" so that if WeatherAlertZoneId
is non-empty and Guid.TryParse returns false you return BadRequest(), leaving
the existing update flow that calls
_weatherAlertService.GetZoneByIdAsync(zoneGuid) and the authorization check
intact for valid GUIDs.
In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtml`:
- Around line 70-97: The code uses loc.severityUnknown as the fallback when
mapping alert status, which mislabels unmapped statuses as severities; update
the status mapping to use a status-specific fallback (e.g., loc.statusUnknown)
instead of loc.severityUnknown, and add loc.statusUnknown to the loc object
alongside statusActive/statusUpdated/statusExpired/statusCancelled; apply the
same change to the other occurrence(s) of the status mapping so all unmapped
statuses render the correct "unknown status" label.
- Around line 111-116: The code uses toISOString().split('T')[0] on Date objects
(variables today and thirtyDaysAgo) which yields UTC dates and causes off-by-one
errors for non-UTC users; replace those uses by formatting the local calendar
date from each Date via getFullYear(), getMonth()+1 and getDate() with
zero-padding to produce a "YYYY-MM-DD" string and assign that to
$('#endDate').val(...) and $('#startDate').val(...), keeping the existing
variables today and thirtyDaysAgo and the same DOM targets.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 1709-1787: Add missing parameter-level XML docs and useful
<returns> notes for the WeatherAlertsController methods: add <param> entries for
GetWeatherAlert(id) describing the id string; GetAlertHistory(start, end)
describing start and end DateTime and expected inclusive/exclusive behavior plus
a <returns> describing returned collection; GetAlertsNearLocation(lat, lon,
radiusKm) describing each numeric param and units; DeleteSource(id) and
DeleteZone(id) describing the id param and add <returns> for success/failure;
ValidateCustomEndpoint(url, sourceType) describing url and sourceType and
document <returns> when null or allowed; NormalizeAreaFilter(areaFilter) and
FormatAreaFilterForDisplay(areaFilter) describing the input string and the
output format; also ensure SaveSource(SaveWeatherAlertSourceInput),
SaveZone(SaveWeatherAlertZoneInput) and
SaveSettings(SaveWeatherAlertSettingsInput) have <param> describing the input
model and a <returns> describing created/updated behavior. Use the exact method
names shown (e.g., GetWeatherAlert(System.String),
GetAlertHistory(System.DateTime,System.DateTime),
GetAlertsNearLocation(System.Double,System.Double,System.Double),
ValidateCustomEndpoint(System.String,System.Int32)) so generated API docs are
unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d1992ce-5f4b-4bdd-99b1-b8686cf61f5c
📒 Files selected for processing (6)
Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.csWeb/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml
✅ Files skipped from review due to trivial changes (1)
- Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml
🚧 Files skipped from review as they are similar to previous changes (1)
- Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Index.cshtml
Comment on lines
+39
to
+63
| protected with_the_weather_alert_service() | ||
| { | ||
| _weatherAlertRepoMock = new Mock<IWeatherAlertRepository>(); | ||
| _weatherAlertSourceRepoMock = new Mock<IWeatherAlertSourceRepository>(); | ||
| _weatherAlertZoneRepoMock = new Mock<IWeatherAlertZoneRepository>(); | ||
| _providerFactoryMock = new Mock<IWeatherAlertProviderFactory>(); | ||
| _departmentSettingsRepoMock = new Mock<IDepartmentSettingsRepository>(); | ||
| _departmentsServiceMock = new Mock<IDepartmentsService>(); | ||
| _messageServiceMock = new Mock<IMessageService>(); | ||
| _callNotesRepoMock = new Mock<ICallNotesRepository>(); | ||
| _cacheProviderMock = new Mock<ICacheProvider>(); | ||
| _eventAggregatorMock = new Mock<IEventAggregator>(); | ||
|
|
||
| _weatherAlertService = new WeatherAlertService( | ||
| _weatherAlertRepoMock.Object, | ||
| _weatherAlertSourceRepoMock.Object, | ||
| _weatherAlertZoneRepoMock.Object, | ||
| _providerFactoryMock.Object, | ||
| _departmentSettingsRepoMock.Object, | ||
| _departmentsServiceMock.Object, | ||
| _messageServiceMock.Object, | ||
| _callNotesRepoMock.Object, | ||
| _cacheProviderMock.Object, | ||
| _eventAggregatorMock.Object); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In NUnit 4.x, is a single test fixture instance reused across multiple tests by default unless FixtureLifeCycle(LifeCycle.InstancePerTestCase) is applied?
💡 Result:
Yes. In NUnit 4.x, the default fixture lifecycle is LifeCycle.SingleInstance, meaning one test fixture instance is created and shared across all test cases in that fixture. Only if you apply [FixtureLifeCycle(LifeCycle.InstancePerTestCase)] will NUnit construct a new fixture instance per test case. [1]
Sources:
[1] NUnit docs: FixtureLifeCycle (shows LifeCycle.SingleInstance is the default and shared for all test cases) (docs.nunit.org)
Citations:
🏁 Script executed:
cat Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs | head -100Repository: Resgrid/Core
Length of output: 3842
🏁 Script executed:
rg -n "Times\.(Once|Never|Exactly|AtLeast|AtMost)" Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs -A 2 -B 2Repository: Resgrid/Core
Length of output: 1996
🏁 Script executed:
rg -n "\[FixtureLifeCycle" Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.csRepository: Resgrid/Core
Length of output: 38
🏁 Script executed:
rg -n "public class|protected.*with_the_weather_alert_service|public void|private Mock" Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs | head -50Repository: Resgrid/Core
Length of output: 1458
Move mock initialization from constructor to [SetUp] or apply [FixtureLifeCycle(LifeCycle.InstancePerTestCase)].
Mocks are created in the fixture constructor and reused across all tests in each [TestFixture] class. With NUnit's default LifeCycle.SingleInstance, a single fixture instance is shared for all test cases, so invocation counts and setups can accumulate between tests. Multiple assertions use Times.Once() and Times.Never(), which will fail if mock state bleeds between tests or if execution order changes. Initialize mocks in a [SetUp] method instead, or apply [FixtureLifeCycle(LifeCycle.InstancePerTestCase)] to ensure test isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs` around lines 39 -
63, The test fixture currently instantiates mocks in the fixture constructor
method with_the_weather_alert_service(), causing shared mock state across tests;
move the mock initializations for _weatherAlertRepoMock,
_weatherAlertSourceRepoMock, _weatherAlertZoneRepoMock, _providerFactoryMock,
_departmentSettingsRepoMock, _departmentsServiceMock, _messageServiceMock,
_callNotesRepoMock, _cacheProviderMock, and _eventAggregatorMock and the
WeatherAlertService construction into a method annotated with [SetUp] (or
alternatively annotate the test class with
[FixtureLifeCycle(LifeCycle.InstancePerTestCase)]) so each test gets fresh mocks
and the WeatherAlertService is recreated per test to avoid invocation-count
leakage that breaks Times.Once()/Times.Never() assertions.
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+70
to
+97
| var loc = { | ||
| event: '@localizer["Event"]', | ||
| severity: '@localizer["Severity"]', | ||
| category: '@localizer["Category"]', | ||
| area: '@localizer["Area"]', | ||
| effectiveDate: '@localizer["EffectiveDate"]', | ||
| expires: '@localizer["Expires"]', | ||
| status: '@localizer["Status"]', | ||
| details: '@localizer["Details"]', | ||
| selectDates: '@localizer["SelectDates"]', | ||
| loadingHistory: '@localizer["LoadingHistory"]', | ||
| noHistoryFound: '@localizer["NoHistoryFound"]', | ||
| failedToLoadHistory: '@localizer["FailedToLoadHistory"]', | ||
| severityExtreme: '@localizer["SeverityExtreme"]', | ||
| severitySevere: '@localizer["SeveritySevere"]', | ||
| severityModerate: '@localizer["SeverityModerate"]', | ||
| severityMinor: '@localizer["SeverityMinor"]', | ||
| severityUnknown: '@localizer["SeverityUnknown"]', | ||
| categoryMet: '@localizer["CategoryMet"]', | ||
| categoryFire: '@localizer["CategoryFire"]', | ||
| categoryHealth: '@localizer["CategoryHealth"]', | ||
| categoryEnv: '@localizer["CategoryEnv"]', | ||
| categoryOther: '@localizer["CategoryOther"]', | ||
| statusActive: '@localizer["StatusActive"]', | ||
| statusUpdated: '@localizer["StatusUpdated"]', | ||
| statusExpired: '@localizer["StatusExpired"]', | ||
| statusCancelled: '@localizer["StatusCancelled"]' | ||
| }; |
Contributor
There was a problem hiding this comment.
Use a status fallback, not the severity fallback string.
Line 163 falls back to loc.severityUnknown, so an unmapped alert status renders as an unknown severity label. Add a status-specific fallback here to avoid misleading history rows.
Also applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtml` around lines
70 - 97, The code uses loc.severityUnknown as the fallback when mapping alert
status, which mislabels unmapped statuses as severities; update the status
mapping to use a status-specific fallback (e.g., loc.statusUnknown) instead of
loc.severityUnknown, and add loc.statusUnknown to the loc object alongside
statusActive/statusUpdated/statusExpired/statusCancelled; apply the same change
to the other occurrence(s) of the status mapping so all unmapped statuses render
the correct "unknown status" label.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs (5)
281-286: Consider validating required zone input fields.Similar to
SaveSource, fields likeNameandZoneCodeare assigned without null/empty validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 281 - 286, Add null/empty validation for required zone fields before assigning them: in WeatherAlertsController (the method where zone.Name, zone.ZoneCode, zone.CenterGeoLocation, zone.RadiusMiles, zone.IsActive, zone.IsPrimary are set) validate input.Name and input.ZoneCode (similar to SaveSource) and return a BadRequest/validation response if either is null or empty; ensure the validation runs before any assignments and include clear error messages indicating which field is missing.
349-352: Consider parallelizing independent save operations.These four
SaveOrUpdateSettingAsynccalls are independent and could be executed concurrently usingTask.WhenAllfor better performance.♻️ Proposed fix
- await _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.WeatherAlertsEnabled.ToString(), DepartmentSettingTypes.WeatherAlertsEnabled); - await _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.MinimumSeverity.ToString(), DepartmentSettingTypes.WeatherAlertMinimumSeverity); - await _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.AutoMessageSeverity.ToString(), DepartmentSettingTypes.WeatherAlertAutoMessageSeverity); - await _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.CallIntegrationEnabled.ToString(), DepartmentSettingTypes.WeatherAlertCallIntegration); + await Task.WhenAll( + _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.WeatherAlertsEnabled.ToString(), DepartmentSettingTypes.WeatherAlertsEnabled), + _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.MinimumSeverity.ToString(), DepartmentSettingTypes.WeatherAlertMinimumSeverity), + _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.AutoMessageSeverity.ToString(), DepartmentSettingTypes.WeatherAlertAutoMessageSeverity), + _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId, input.CallIntegrationEnabled.ToString(), DepartmentSettingTypes.WeatherAlertCallIntegration) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 349 - 352, The four independent calls to _departmentSettingsService.SaveOrUpdateSettingAsync (using DepartmentId and input.WeatherAlertsEnabled, input.MinimumSeverity, input.AutoMessageSeverity, input.CallIntegrationEnabled) should be executed concurrently to improve performance: create Task variables for each SaveOrUpdateSettingAsync call (e.g., t1..t4) and then await Task.WhenAll(t1, t2, t3, t4) instead of awaiting each call sequentially; ensure you still pass the same DepartmentSettingTypes for each call and propagate exceptions by awaiting the Task.WhenAll result.
506-514: Empty catch block silently swallows deserialization failures.While falling back to the raw value for display may be intentional, an empty catch block can hide unexpected errors. Consider logging or adding a comment to clarify the intent.
♻️ Proposed fix
try { var codes = JsonSerializer.Deserialize<string[]>(trimmed); return codes != null ? string.Join(", ", codes) : trimmed; } - catch { } + catch + { + // Malformed JSON — fall back to raw display + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 506 - 514, The empty catch in WeatherAlertsController around the JsonSerializer.Deserialize call swallows deserialization errors; change it to catch the specific exceptions (e.g., JsonException) and log the failure (including the exception and the input variable trimmed) using the controller's logger (for example _logger or Logger) before falling back to returning trimmed, or if no logger exists add a short comment explaining the swallow is intentional; update the try/catch around JsonSerializer.Deserialize<string[]> and include the logging call to make failures visible.
182-188: Consider validating required input fields.
input.Nameis assigned directly without null/empty validation. IfNameis a required field, validate it before saving.♻️ Proposed fix
+ if (string.IsNullOrWhiteSpace(input.Name)) + return BadRequest("Name is required"); + source.Name = input.Name; source.SourceType = input.SourceType;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 182 - 188, The code assigns input.Name directly to source.Name without validating it; update the controller action (the method that sets source.Name alongside NormalizeAreaFilter and ValidateCustomEndpoint) to validate that input.Name is not null or whitespace (e.g., string.IsNullOrWhiteSpace(input.Name)) before assigning, and if invalid return a proper 400 BadRequest or add a ModelState error so the request is rejected instead of saving an empty name; only assign source.Name = input.Name after the check passes.
533-539: Replace magic numbers with named enum values.The code uses integers
0,1,2for source types, making it harder to understand and maintain. Replace them with the correspondingWeatherAlertSourceTypeenum values, which are already defined in the codebase.As per coding guidelines: "Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations."
♻️ Proposed fix
var allowedHosts = sourceType switch { - 0 => new[] { "api.weather.gov" }, // NWS - 1 => new[] { "dd.weather.gc.ca", "dd.meteo.gc.ca" }, // Environment Canada - 2 => new[] { "feeds.meteoalarm.org", "meteoalarm.org" }, // MeteoAlarm + (int)WeatherAlertSourceType.NationalWeatherService => new[] { "api.weather.gov" }, + (int)WeatherAlertSourceType.EnvironmentCanada => new[] { "dd.weather.gc.ca", "dd.meteo.gc.ca" }, + (int)WeatherAlertSourceType.MeteoAlarm => new[] { "feeds.meteoalarm.org", "meteoalarm.org" }, _ => Array.Empty<string>() };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 533 - 539, The switch on sourceType uses magic integers (0,1,2); change the cases to use the WeatherAlertSourceType enum members (e.g., WeatherAlertSourceType.Nws, WeatherAlertSourceType.EnvironmentCanada, WeatherAlertSourceType.MeteoAlarm) so the intent is clear; update the switch in the block that assigns allowedHosts (variable allowedHosts and the switch over sourceType) to use those enum values and leave the host arrays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs`:
- Around line 112-115: GetAlertsNearLocation is missing validation for its
geographic inputs; validate that lat is between -90 and 90, lng between -180 and
180, and radiusMiles is non‑negative (optionally enforce a sane max) before
calling _weatherAlertService.GetActiveAlertsNearLocationAsync; if validation
fails return a BadRequest (e.g., BadRequestObjectResult or ModelState error)
with a clear message referencing the invalid parameter(s) so downstream
geospatial queries are never invoked with invalid values.
- Around line 90-93: Validate the incoming date range in GetAlertHistory by
checking that the startDate is not after endDate; if startDate > endDate return
a BadRequest result (e.g., BadRequest("startDate must be on or before endDate"))
before calling _departmentsService.GetDepartmentByIdAsync or
_weatherAlertService.GetAlertHistoryAsync so you avoid querying with an invalid
range.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs`:
- Around line 281-286: Add null/empty validation for required zone fields before
assigning them: in WeatherAlertsController (the method where zone.Name,
zone.ZoneCode, zone.CenterGeoLocation, zone.RadiusMiles, zone.IsActive,
zone.IsPrimary are set) validate input.Name and input.ZoneCode (similar to
SaveSource) and return a BadRequest/validation response if either is null or
empty; ensure the validation runs before any assignments and include clear error
messages indicating which field is missing.
- Around line 349-352: The four independent calls to
_departmentSettingsService.SaveOrUpdateSettingAsync (using DepartmentId and
input.WeatherAlertsEnabled, input.MinimumSeverity, input.AutoMessageSeverity,
input.CallIntegrationEnabled) should be executed concurrently to improve
performance: create Task variables for each SaveOrUpdateSettingAsync call (e.g.,
t1..t4) and then await Task.WhenAll(t1, t2, t3, t4) instead of awaiting each
call sequentially; ensure you still pass the same DepartmentSettingTypes for
each call and propagate exceptions by awaiting the Task.WhenAll result.
- Around line 506-514: The empty catch in WeatherAlertsController around the
JsonSerializer.Deserialize call swallows deserialization errors; change it to
catch the specific exceptions (e.g., JsonException) and log the failure
(including the exception and the input variable trimmed) using the controller's
logger (for example _logger or Logger) before falling back to returning trimmed,
or if no logger exists add a short comment explaining the swallow is
intentional; update the try/catch around JsonSerializer.Deserialize<string[]>
and include the logging call to make failures visible.
- Around line 182-188: The code assigns input.Name directly to source.Name
without validating it; update the controller action (the method that sets
source.Name alongside NormalizeAreaFilter and ValidateCustomEndpoint) to
validate that input.Name is not null or whitespace (e.g.,
string.IsNullOrWhiteSpace(input.Name)) before assigning, and if invalid return a
proper 400 BadRequest or add a ModelState error so the request is rejected
instead of saving an empty name; only assign source.Name = input.Name after the
check passes.
- Around line 533-539: The switch on sourceType uses magic integers (0,1,2);
change the cases to use the WeatherAlertSourceType enum members (e.g.,
WeatherAlertSourceType.Nws, WeatherAlertSourceType.EnvironmentCanada,
WeatherAlertSourceType.MeteoAlarm) so the intent is clear; update the switch in
the block that assigns allowedHosts (variable allowedHosts and the switch over
sourceType) to use those enum values and leave the host arrays unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef1e985b-48bb-4664-8a3f-79169d238e36
📒 Files selected for processing (3)
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSourceResultData.csWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtml
✅ Files skipped from review due to trivial changes (1)
- Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/History.cshtml
🚧 Files skipped from review as they are similar to previous changes (1)
- Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSourceResultData.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This was referenced Apr 10, 2026
Merged
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit