Conversation
- SubscriptionsService: reduce billing API MaxTimeout from 200s to 5s and wrap call in try-catch so network/timeout errors fall back to freePlan instead of hanging the request past Twilio's 15s webhook deadline - TwilioController.IncomingMessage: run the four independent department lookups (GetDepartmentById, GetTextToCallSourceNumbers, CanDepartmentProvisionNumber, GetAllActiveCustomStates) concurrently with Task.WhenAll instead of sequentially - TwilioController.IncomingMessage: reuse the UserProfile already fetched in the department-resolution block instead of issuing a second DB call inside the !isDispatchSource branch Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughTwo service files modified: SubscriptionsService adds error handling and reduces timeout for Billing API calls, while TwilioController optimizes async operations by executing four concurrent department-scoped lookups instead of sequential calls and reuses cached profile data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Core/Resgrid.Services/SubscriptionsService.cs (1)
59-86: LGTM — sensible fail-fast + fallback for webhook path.The 5s cap plus try/catch fallback to
freePlankeeps the Twilio webhook within its 15s deadline, and existingNotFound/ null-data fallbacks are preserved. Minor, optional considerations (non-blocking):
catch (Exception ex)will also swallowOperationCanceledExceptionfrom a caller-supplied token. If you ever thread aCancellationTokenthrough this method, consider rethrowing cancellations:catch (Exception ex) when (ex is not OperationCanceledException)- The
RestClientis not disposed here (pre-existing pattern throughout the file). Not introduced by this PR, but worth tracking —RestClientin RestSharp v107+ owns anHttpClientand is intended to be long-lived/reused; per-call construction can exhaust sockets under load. Consider caching a singleRestClientper base URL as a follow-up across this file.- Only
GetCurrentPlanForDepartmentAsyncgets the 5s timeout; the other billing calls in this file still useMaxTimeout = 200000. That's fine if they're not on the Twilio hot path, but if any of them are reachable from the webhook transitively, they'd reintroduce the same problem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/SubscriptionsService.cs` around lines 59 - 86, The catch-all in the GetCurrentPlanForDepartmentAsync call swallows caller cancellations; update the exception handling around the RestClient/ExecuteAsync<GetCurrentPlanForDepartmentResult> block so OperationCanceledException is not caught—i.e., only log and return freePlan for non-cancellation exceptions and rethrow or let OperationCanceledException propagate; keep references to RestClient, ExecuteAsync<GetCurrentPlanForDepartmentResult>, freePlan, and Framework.Logging.LogException when locating the code to change.Web/Resgrid.Web.Services/Controllers/TwilioController.cs (1)
146-157: Minor cleanup for the parallelized lookups.The parallelization is correct — per the provided context snippets, all four calls are independent and safe to fan out. A few small polish items on the new block:
System.Threading.Tasksis already imported (line 7); the fully-qualifiedSystem.Threading.Tasks.Task.WhenAllcan just beTask.WhenAll.- After
await Task.WhenAll(...), preferawait taskovertask.Resultto unwrap values — it's more idiomatic C# and avoidsAggregateException-style unwrapping semantics if anything ever changes upstream.- The new local
authroizedcarries forward a misspelling (also present further down in this file around line 747). Worth renaming toauthorizedwhile you're touching this code.Also note that if any of these four tasks throws,
await Task.WhenAllrethrows only the first exception; the remaining faults are still observed (tasks are awaited), so you're fine w.r.t.UnobservedTaskException, and the outertry/catchat line 448 will log it and thefinallywill still persistmessageEvent. No functional concern — just flagging for awareness.♻️ Proposed refactor
- // Run all department-level lookups in parallel — they are independent of each other. - var departmentTask = _departmentsService.GetDepartmentByIdAsync(departmentId.Value); - var dispatchNumbersTask = _departmentSettingsService.GetTextToCallSourceNumbersForDepartmentAsync(departmentId.Value); - var authorizedTask = _limitsService.CanDepartmentProvisionNumberAsync(departmentId.Value); - var customStatesTask = _customStateService.GetAllActiveCustomStatesForDepartmentAsync(departmentId.Value); - - await System.Threading.Tasks.Task.WhenAll(departmentTask, dispatchNumbersTask, authorizedTask, customStatesTask); - - var department = departmentTask.Result; - var dispatchNumbers = dispatchNumbersTask.Result; - var authroized = authorizedTask.Result; - var customStates = customStatesTask.Result; + // Run all department-level lookups in parallel — they are independent of each other. + var departmentTask = _departmentsService.GetDepartmentByIdAsync(departmentId.Value); + var dispatchNumbersTask = _departmentSettingsService.GetTextToCallSourceNumbersForDepartmentAsync(departmentId.Value); + var authorizedTask = _limitsService.CanDepartmentProvisionNumberAsync(departmentId.Value); + var customStatesTask = _customStateService.GetAllActiveCustomStatesForDepartmentAsync(departmentId.Value); + + await Task.WhenAll(departmentTask, dispatchNumbersTask, authorizedTask, customStatesTask); + + var department = await departmentTask; + var dispatchNumbers = await dispatchNumbersTask; + var authorized = await authorizedTask; + var customStates = await customStatesTask;You'll want to follow through and rename
authroized→authorizedat line 161 as well.As per coding guidelines: "Use modern C# features appropriately" and "Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 146 - 157, Replace the fully-qualified System.Threading.Tasks.Task.WhenAll call with Task.WhenAll and, after awaiting it, unwrap each result by awaiting the individual tasks (await departmentTask, await dispatchNumbersTask, await authorizedTask, await customStatesTask) instead of using .Result; also correct the misspelled local variable authroized to authorized (and update any other occurrences of that misspelling in this file) so the locals are department, dispatchNumbers, authorized, and customStates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Core/Resgrid.Services/SubscriptionsService.cs`:
- Around line 59-86: The catch-all in the GetCurrentPlanForDepartmentAsync call
swallows caller cancellations; update the exception handling around the
RestClient/ExecuteAsync<GetCurrentPlanForDepartmentResult> block so
OperationCanceledException is not caught—i.e., only log and return freePlan for
non-cancellation exceptions and rethrow or let OperationCanceledException
propagate; keep references to RestClient,
ExecuteAsync<GetCurrentPlanForDepartmentResult>, freePlan, and
Framework.Logging.LogException when locating the code to change.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Around line 146-157: Replace the fully-qualified
System.Threading.Tasks.Task.WhenAll call with Task.WhenAll and, after awaiting
it, unwrap each result by awaiting the individual tasks (await departmentTask,
await dispatchNumbersTask, await authorizedTask, await customStatesTask) instead
of using .Result; also correct the misspelled local variable authroized to
authorized (and update any other occurrences of that misspelling in this file)
so the locals are department, dispatchNumbers, authorized, and customStates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d59eb31-b74b-4554-a9ff-8ba87a91af95
📒 Files selected for processing (2)
Core/Resgrid.Services/SubscriptionsService.csWeb/Resgrid.Web.Services/Controllers/TwilioController.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Performance