Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughTruncates multiple WeatherAlert fields and simplifies alert notification body; adds a process-wide RabbitMQ connection-reset event and exchange-declaration tracking; tightens parsing and bounds checks in several controllers; removes FullCalendar assets from a view; and makes client-side status form code more defensive. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| var detailTypeVal = $('#detailType').length ? $('#detailType').val() : '0'; | ||
| var noteTypeVal = $('#noteType').length ? $('#noteType').val() : '0'; | ||
| var requireGpsVal = $('#requireGps').length ? $('#requireGps').val() : 'false'; | ||
| $('#options tbody').first().append("<tr><td><input type='number' min='0' id='order_" + editstatus.optionsCount + "' name='order_" + editstatus.optionsCount + "' value='0' class='numberEntry'></td><td>" + $('#buttonText').val() + "<input type='hidden' id='buttonText_" + editstatus.optionsCount + "' name='buttonText_" + editstatus.optionsCount + "' value='" + $('#buttonText').val() + "'></input><input type='hidden' id='baseType_" + editstatus.optionsCount + "' name='baseType_" + editstatus.optionsCount + "' value='" + baseTypeVal + "'></input></td><td><a class='btn btn-default' role='button' style='color:" + $('#textColor').val() + ";background:" + $('#buttonColor').val() + ";'>" + $('#buttonText').val() + "</a><input type='hidden' id='buttonColor_" + editstatus.optionsCount + "' name='buttonColor_" + editstatus.optionsCount + "' value='" + $('#buttonColor').val() + "'><input type='hidden' id='textColor_" + editstatus.optionsCount + "' name='textColor_" + editstatus.optionsCount + "' value='" + $('#textColor').val() + "'><input type='hidden' id='detailType_" + editstatus.optionsCount + "' name='detailType_" + editstatus.optionsCount + "' value='" + detailTypeVal + "'></input><input type='hidden' id='noteType_" + editstatus.optionsCount + "' name='noteType_" + editstatus.optionsCount + "' value='" + noteTypeVal + "'></input><input type='hidden' id='requireGps_" + editstatus.optionsCount + "' name='requireGps_" + editstatus.optionsCount + "' value='" + requireGpsVal + "'></input></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='btn btn-xs btn-danger' data-original-title='Remove this option'>Remove</a></td></tr>"); |
| var detailTypeVal = $('#detailType').length ? $('#detailType').val() : '0'; | ||
| var noteTypeVal = $('#noteType').length ? $('#noteType').val() : '0'; | ||
| var requireGpsVal = $('#requireGps').length ? $('#requireGps').val() : 'false'; | ||
| $('#options tbody').first().append("<tr><td><input type='number' min='0' id='order_" + newstatus.optionsCount + "' name='order_" + newstatus.optionsCount + "' value='0' onkeypress='return resgrid.statuses.newstatus.isNumber(event)'></td><td>" + $('#buttonText').val() + "<input type='hidden' id='buttonText_" + newstatus.optionsCount + "' name='buttonText_" + newstatus.optionsCount + "' value='" + $('#buttonText').val() + "'></input><input type='hidden' id='baseType_" + newstatus.optionsCount + "' name='baseType_" + newstatus.optionsCount + "' value='" + baseTypeVal + "'></input></td><td><a class='btn btn-default' role='button' style='color:" + $('#textColor').val() + ";background:" + $('#buttonColor').val() + ";'>" + $('#buttonText').val() + "</a><input type='hidden' id='buttonColor_" + newstatus.optionsCount + "' name='buttonColor_" + newstatus.optionsCount + "' value='" + $('#buttonColor').val() + "'><input type='hidden' id='textColor_" + newstatus.optionsCount + "' name='textColor_" + newstatus.optionsCount + "' value='" + $('#textColor').val() + "'><input type='hidden' id='detailType_" + newstatus.optionsCount + "' name='detailType_" + newstatus.optionsCount + "' value='" + detailTypeVal + "'></input><input type='hidden' id='noteType_" + newstatus.optionsCount + "' name='noteType_" + newstatus.optionsCount + "' value='" + noteTypeVal + "'></input><input type='hidden' id='requireGps_" + newstatus.optionsCount + "' name='requireGps_" + newstatus.optionsCount + "' value='" + requireGpsVal + "'></input></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='btn btn-xs btn-danger' data-original-title='Remove this option'>Remove</a></td></tr>"); |
![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: 6
🧹 Nitpick comments (1)
Web/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.cs (1)
213-214: MoveCustomStateload to invalid-model return path.Line 214 triggers a service call even when
ModelStateis valid and the action redirects. This can be deferred to the invalid-model branch only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.cs` around lines 213 - 214, The code eagerly calls _customStateService.GetCustomSateByIdAsync and assigns model.Detail.CustomState regardless of ModelState, causing unnecessary work on successful POSTs; move the await _customStateService.GetCustomSateByIdAsync(model.Detail.CustomStateId) assignment so that model.Detail.CustomState is only populated in the branch that returns the view when ModelState is invalid (after setting model.BaseTypes = model.BaseType.ToSelectList()), leaving the successful/redirect path untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs`:
- Line 28: The ConnectionReset event is invoked inline
(ConnectionReset?.Invoke()) which allows a throwing subscriber to abort the
reconnect/reset flow; update the RabbitConnection class to protect those
invocations by iterating the ConnectionReset invocation list (or copy the
delegate) and invoking each subscriber inside its own try/catch so one
subscriber's exception is caught and logged but does not stop the overall
reconnect logic; apply this change to both places where ConnectionReset is
invoked so shared connection state remains consistent even if a handler throws.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs`:
- Around line 124-125: The early return based solely on _exchangeDeclared can
skip exchange re-declaration after a reconnect; in
RabbitTopicProvider.SendMessage move the _exchangeDeclared check until after
connection validation/creation (call CreateConnection or the existing
connection-check logic first) so the exchange is only considered declared when a
live connection/channel exists, and also ensure any reconnect/reset path clears
_exchangeDeclared (or re-declares the exchange) so publishing never uses a stale
flag; reference RabbitTopicProvider.SendMessage, CreateConnection, and the
_exchangeDeclared field when making the change.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Around line 599-602: In TwilioController (the webhook handler that reads
twilioRequest.Digits) replace the unsafe int.Parse(twilioRequest.Digits) with
int.TryParse to guard parsing errors; if TryParse fails return the same "invalid
selection" TwiML prompt instead of throwing, and when parsing succeeds compute
index = parsed - 2 and keep the existing bounds check using stations.Count
(index >= 0 && index < stations.Count) before selecting a station; ensure the
method returns the TwiML invalid-selection response on parse failure so the
public endpoint never throws FormatException.
In `@Web/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.cs`:
- Around line 105-106: The code in CustomStatusesController silently converts
invalid noteType/detailType parses to 0 by using int.TryParse without checking
the boolean result; update the parsing logic around the form processing (the
uses of int.TryParse for variables noteType and detailType) to validate the
TryParse return value and handle failures explicitly—either return a
BadRequest/validation error or skip the invalid item and log the problem—so an
invalid payload isn’t stored as enum value 0; apply the same change to the other
occurrence of noteType/detailType parsing later in the controller.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.editstatus.js`:
- Around line 88-89: The code reads the checkbox value for requireGps with
$('#requireGps').val(), which returns the input value rather than whether it's
checked, causing incorrect requireGps_* hidden inputs; update the requireGpsVal
assignment to check the checked state (e.g., use $('#requireGps').length ?
($('#requireGps').is(':checked') ? 'true' : 'false') : 'false') so the hidden
input id/name 'requireGps_' + editstatus.optionsCount stores a true/false string
based on $('#requireGps') being checked.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.newstatus.js`:
- Around line 96-97: The hidden requireGps value is using $('#requireGps').val()
which doesn't reflect the checkbox checked state; update the code that builds
the options row (the long append call that uses newstatus.optionsCount and
creates the hidden input id='requireGps_{index}') to compute requireGpsVal from
the checkbox checked state (e.g. use $('#requireGps').is(':checked') or
.prop('checked') and convert to 'true'/'false') and then set that requireGpsVal
into the hidden input for requireGps_{newstatus.optionsCount}; ensure you only
change how requireGpsVal is derived so the rest of the appended markup (ids like
buttonText_{index}, baseType_{index}, detailType_{index}, noteType_{index})
remains unchanged.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.cs`:
- Around line 213-214: The code eagerly calls
_customStateService.GetCustomSateByIdAsync and assigns model.Detail.CustomState
regardless of ModelState, causing unnecessary work on successful POSTs; move the
await _customStateService.GetCustomSateByIdAsync(model.Detail.CustomStateId)
assignment so that model.Detail.CustomState is only populated in the branch that
returns the view when ModelState is invalid (after setting model.BaseTypes =
model.BaseType.ToSelectList()), leaving the successful/redirect path untouched.
🪄 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: 90272eb4-88fd-4b0c-b5da-c4ded27b6bd7
📒 Files selected for processing (10)
Core/Resgrid.Services/WeatherAlertService.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.csWeb/Resgrid.Web/Areas/User/Controllers/GroupsController.csWeb/Resgrid.Web/Areas/User/Views/Shifts/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Templates/EditCallNote.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.editstatus.jsWeb/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.newstatus.js
💤 Files with no reviewable changes (1)
- Web/Resgrid.Web/Areas/User/Views/Shifts/Index.cshtml
Comment on lines
+105
to
+106
| int.TryParse(form["noteType_" + i], out var noteType); | ||
| int.TryParse(form["detailType_" + i], out var detailType); |
Contributor
There was a problem hiding this comment.
Don’t silently coerce invalid noteType/detailType to 0.
On Line 105/106 and Line 288/289, failed parses currently become 0, which can store unintended enum values instead of rejecting bad payloads. Add explicit validation on parse failure.
Proposed fix
- int.TryParse(form["noteType_" + i], out var noteType);
- int.TryParse(form["detailType_" + i], out var detailType);
+ if (!int.TryParse(form["noteType_" + i], out var noteType) ||
+ !int.TryParse(form["detailType_" + i], out var detailType))
+ {
+ ModelState.AddModelError("Detail", "Invalid note/detail type value.");
+ continue;
+ }Also applies to: 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.cs` around
lines 105 - 106, The code in CustomStatusesController silently converts invalid
noteType/detailType parses to 0 by using int.TryParse without checking the
boolean result; update the parsing logic around the form processing (the uses of
int.TryParse for variables noteType and detailType) to validate the TryParse
return value and handle failures explicitly—either return a
BadRequest/validation error or skip the invalid item and log the problem—so an
invalid payload isn’t stored as enum value 0; apply the same change to the other
occurrence of noteType/detailType parsing later in the controller.
Web/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.editstatus.js
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.newstatus.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs (1)
17-22: Avoid baking exchange state into process-wide static state.
_exchangeDeclaredplus the staticConnectionResetsubscription makes publish behavior depend on ambient global state, which is harder to isolate in tests and easier to leak across callers. Prefer moving this cache behindRabbitConnectionor an injected collaborator so the dependency is explicit.As per coding guidelines, "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs` around lines 17 - 22, The static process-wide flag _exchangeDeclared and the static RabbitTopicProvider constructor subscription to RabbitConnection.ConnectionReset should be removed and replaced by an explicit, testable cache owned by RabbitConnection or an injected collaborator; add an API on RabbitConnection (e.g., IsExchangeDeclared/MarkExchangeDeclared or a GetExchangeDeclarationState service) or inject an IExchangeDeclarationCache into RabbitTopicProvider, move the reset behavior into that collaborator so RabbitConnection triggers the reset on it, then update RabbitTopicProvider’s publish/ensure-exchange code to call the new collaborator methods instead of using the static _exchangeDeclared and remove the static constructor subscription; ensure unit tests can mock the injected collaborator to control exchange-declared state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs`:
- Around line 126-139: The exchange is being declared on a connection instance
returned by RabbitConnection.CreateConnection but SendMessage later re-fetches
the singleton connection, risking a race where the exchange flag
(_exchangeDeclared) is set for a different connection; change the flow so the
same validated IConnection instance is used for both declaration and publish:
obtain the connection once (e.g., via a VerifyAndCreateClients(_clientName) or
the local variable returned by RabbitConnection.CreateConnection), return false
if null, use that connection to CreateChannelAsync and call
ExchangeDeclareAsync(Topics.EventingTopic) and then pass that same
connection/channel into the publish path (SendMessage) instead of re-querying
the singleton, and ensure _exchangeDeclared is tied to the specific connection
instance rather than a global flag.
---
Nitpick comments:
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs`:
- Around line 17-22: The static process-wide flag _exchangeDeclared and the
static RabbitTopicProvider constructor subscription to
RabbitConnection.ConnectionReset should be removed and replaced by an explicit,
testable cache owned by RabbitConnection or an injected collaborator; add an API
on RabbitConnection (e.g., IsExchangeDeclared/MarkExchangeDeclared or a
GetExchangeDeclarationState service) or inject an IExchangeDeclarationCache into
RabbitTopicProvider, move the reset behavior into that collaborator so
RabbitConnection triggers the reset on it, then update RabbitTopicProvider’s
publish/ensure-exchange code to call the new collaborator methods instead of
using the static _exchangeDeclared and remove the static constructor
subscription; ensure unit tests can mock the injected collaborator to control
exchange-declared state.
🪄 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: d816efa6-c499-4d95-a396-9d7ecd13f0f5
📒 Files selected for processing (5)
Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.editstatus.jsWeb/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.newstatus.js
✅ Files skipped from review due to trivial changes (1)
- Web/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.newstatus.js
🚧 Files skipped from review as they are similar to previous changes (3)
- Web/Resgrid.Web.Services/Controllers/TwilioController.cs
- Web/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.editstatus.js
- Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs
Comment on lines
+126
to
+139
| // Validate/create connection first so a reconnect clears _exchangeDeclared | ||
| var connection = await RabbitConnection.CreateConnection(clientName); | ||
|
|
||
| if (_exchangeDeclared) | ||
| return true; | ||
|
|
||
| if (connection != null) | ||
| { | ||
| using (var channel = await connection.CreateChannelAsync()) | ||
| { | ||
| await channel.ExchangeDeclareAsync(RabbitConnection.SetQueueNameForEnv(Topics.EventingTopic), "fanout"); | ||
| } | ||
|
|
||
| _exchangeDeclared = true; |
Contributor
There was a problem hiding this comment.
Use the same validated connection for declaration and publish.
This method now validates/declares against one IConnection, but SendMessage fetches the singleton again afterward. If a reset happens in that gap, _exchangeDeclared is cleared and publish can still continue on the recreated connection without re-declaring the exchange.
Suggested shape
-private static async Task<bool> VerifyAndCreateClients(string clientName)
+private static async Task<IConnection> VerifyAndCreateClients(string clientName)
{
try
{
var connection = await RabbitConnection.CreateConnection(clientName);
+ if (connection == null)
+ return null;
if (_exchangeDeclared)
- return true;
-
- if (connection != null)
- {
- using (var channel = await connection.CreateChannelAsync())
- {
- await channel.ExchangeDeclareAsync(RabbitConnection.SetQueueNameForEnv(Topics.EventingTopic), "fanout");
- }
-
- _exchangeDeclared = true;
- }
+ return connection;
+
+ using (var channel = await connection.CreateChannelAsync())
+ {
+ await channel.ExchangeDeclareAsync(RabbitConnection.SetQueueNameForEnv(Topics.EventingTopic), "fanout");
+ }
+
+ _exchangeDeclared = true;
+ return connection;
}
catch (Exception ex)
{
Framework.Logging.LogException(ex);
- return false;
+ return null;
}
-
- return true;
}var connection = await VerifyAndCreateClients(_clientName);
if (connection == null)
return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.cs` around lines
126 - 139, The exchange is being declared on a connection instance returned by
RabbitConnection.CreateConnection but SendMessage later re-fetches the singleton
connection, risking a race where the exchange flag (_exchangeDeclared) is set
for a different connection; change the flow so the same validated IConnection
instance is used for both declaration and publish: obtain the connection once
(e.g., via a VerifyAndCreateClients(_clientName) or the local variable returned
by RabbitConnection.CreateConnection), return false if null, use that connection
to CreateChannelAsync and call ExchangeDeclareAsync(Topics.EventingTopic) and
then pass that same connection/channel into the publish path (SendMessage)
instead of re-querying the singleton, and ensure _exchangeDeclared is tied to
the specific connection instance rather than a global flag.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
Bug Fixes
Refactor