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
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (17)
📝 WalkthroughWalkthroughThis PR migrates the default TTS voice from legacy F3 to Klatt6 variant across configuration and runtime code, refactors Twilio call flow to support dispatch-based outbound calling with new menu endpoints and prompts, and optimizes S3 storage uploads to buffer payloads with configurable SSL protocol selection. ChangesVoice Configuration Migration
Twilio Dispatch Voice Call Flow
S3 Storage Payload Buffering
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller/PSTN
participant Twilio as Twilio Gateway
participant TwilioCtrl as TwilioController
participant DispatchSvc as Dispatch Service
participant S3 as S3 Storage
participant TTS as TTS Service
Caller->>Twilio: Incoming call
Twilio->>TwilioCtrl: POST /api/Twilio/VoiceCall
TwilioCtrl->>DispatchSvc: Get call & dispatch details
alt Dispatch Audio Available
DispatchSvc->>S3: Fetch dispatch audio
S3-->>DispatchSvc: Audio URL
else Geo-based Dispatch
TwilioCtrl->>TTS: Synthesize dispatch prompt
TTS-->>TwilioCtrl: Audio URL
end
TwilioCtrl-->>Twilio: TwiML (dispatch audio + Gather menu)
Twilio-->>Caller: Play dispatch, collect digits
Caller->>Twilio: Enter digit (1 or 2)
Twilio->>TwilioCtrl: POST /api/Twilio/VoiceCallAction
alt Digit = 1
TwilioCtrl-->>Twilio: Redirect to VoiceCall
else Digit = 2
TwilioCtrl-->>Twilio: Redirect to VoiceCallResponseOptions
end
Twilio->>TwilioCtrl: POST /api/Twilio/VoiceCallResponseOptions
TwilioCtrl-->>Twilio: TwiML (response options + Gather, finishOnKey=#)
Twilio-->>Caller: Play options, collect digits with #
Caller->>Twilio: Enter digit sequence + #
Twilio->>TwilioCtrl: POST /api/Twilio/VoiceCallRespond
alt Digit = 0
TwilioCtrl-->>Twilio: Redirect to VoiceCall
else Digit = 1
TwilioCtrl->>DispatchSvc: Log RespondingToScene
TwilioCtrl-->>Twilio: TwiML (scene response, hangup)
else Digit > 1
TwilioCtrl->>DispatchSvc: Log RespondingToStation(index)
TwilioCtrl-->>Twilio: TwiML (station response, hangup)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This PR spans three independent subsystems with significant structural changes: the voice configuration migration requires careful validation of normalization logic and config propagation across layers; the Twilio controller refactoring introduces new endpoints and multi-layered call routing that demands careful sequence validation; and the S3 storage buffering refactor involves stream lifecycle and retry semantics. The changes are heterogeneous across configuration, service logic, controller endpoints, and tests, requiring separate reasoning for each area. Possibly related PRs
🚥 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 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. Review rate limit: 0/1 reviews remaining, refill in 11 minutes and 48 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs (1)
108-154: 💤 Low valueNew normalization tests: consider asserting
result.Speedfor full result coverage.Both new tests verify
result.Voicebut skipresult.Speed.Should().Be(175). The first cache-hit test (lines 58–63) asserts both. Since these tests go through the same result-construction code path, adding the speed assertion would close a small gap and keep the pattern consistent across all cache-hit tests.✏️ Suggested additions
// generate_async_should_apply_configured_klatt_variant_to_requested_language result.Cached.Should().BeTrue(); result.Voice.Should().Be("fr+klatt6"); + result.Speed.Should().Be(175); result.Url.Should().Be(cachedUri.ToString());// generate_async_should_replace_legacy_f3_voice_with_configured_klatt_variant result.Cached.Should().BeTrue(); result.Voice.Should().Be("en-us+klatt6"); + result.Speed.Should().Be(175); result.Url.Should().Be(cachedUri.ToString());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs` around lines 108 - 154, Add an assertion to both tests to check the returned speech speed: after calling _service.GenerateAsync in generate_async_should_apply_configured_klatt_variant_to_requested_language and generate_async_should_replace_legacy_f3_voice_with_configured_klatt_variant, assert result.Speed.Should().Be(175); this mirrors the existing cache-hit test pattern and ensures the TtsRequest/GenerateAsync result includes the expected Speed value alongside Cached, Voice, and Url validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Model/TwilioVoicePromptCatalog.cs`:
- Around line 30-31: The prompts InvalidStatusSelection and NoStatusSelection
currently say "Goodbye." but the handler InboundVoiceActionStatus still
redirects callers back to the main menu; update to make behavior & wording
consistent by either (A) editing the constants InvalidStatusSelection and
NoStatusSelection to say they are "returning to the main menu" (matching the
redirect flow) or (B) change InboundVoiceActionStatus to hang up after playing
those prompts instead of redirecting; pick one approach and update the
corresponding symbol(s) (the two constants or the InboundVoiceActionStatus
handler logic) so wording and flow match.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Around line 1100-1107: BuildDispatchPrompt currently returns a dispatch prompt
string without terminal punctuation, causing TTS to run together; update the
method (BuildDispatchPrompt) to append a period (or appropriate sentence-ending
punctuation) to both return branches so the prompt ends cleanly, and apply the
same change to the duplicate helper in TwilioProviderController (the
corresponding BuildDispatchPrompt there) so both code paths produce a
sentence-terminated prompt for TTS.
- Around line 510-512: The three newly added public Twilio callback actions
(including VoiceCallAction and the two other methods added at the ranges
referenced) are missing the [ValidateRequest] attribute; add [ValidateRequest]
above each of these controller action declarations (e.g., above VoiceCallAction
and the other two new Twilio callback methods) so Twilio request signature
validation is enforced before executing logic like SetUserActionAsync(). Ensure
the attribute sits on the same method declarations where [HttpGet(...)] and
[Produces(...)] are declared.
In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs`:
- Around line 431-434: GetPresignedUrlProtocol currently forces HTTPS/HTTP based
only on _options.UseSsl; instead detect if _options.Endpoint is an absolute URI
and, if so, use its scheme (https => Protocol.HTTPS, http => Protocol.HTTP);
otherwise fall back to _options.UseSsl. Update the GetPresignedUrlProtocol
method to parse _options.Endpoint (e.g., with Uri.TryCreate) and return the
protocol based on that parsed Uri.Scheme when absolute, referencing
GetPresignedUrlProtocol and _options.Endpoint/_options.UseSsl so presigned URLs
honor an explicit endpoint URI scheme.
---
Nitpick comments:
In `@Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs`:
- Around line 108-154: Add an assertion to both tests to check the returned
speech speed: after calling _service.GenerateAsync in
generate_async_should_apply_configured_klatt_variant_to_requested_language and
generate_async_should_replace_legacy_f3_voice_with_configured_klatt_variant,
assert result.Speed.Should().Be(175); this mirrors the existing cache-hit test
pattern and ensures the TtsRequest/GenerateAsync result includes the expected
Speed value alongside Cached, Voice, and Url validations.
🪄 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: faf2e734-cad8-4e02-aeef-ffebd0682129
📒 Files selected for processing (15)
Core/Resgrid.Config/TtsConfig.csCore/Resgrid.Model/EspeakVoiceCatalog.csCore/Resgrid.Model/TwilioVoicePromptCatalog.csTests/Resgrid.Tests/Services/DepartmentSettingsServiceTtsLanguageTests.csTests/Resgrid.Tests/Web/Services/TwilioControllerVoiceVerificationTests.csTests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.csTests/Resgrid.Tests/Web/Tts/TtsAdminControllerTests.csTests/Resgrid.Tests/Web/Tts/TtsServiceTests.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web.Services/Controllers/TwilioProviderController.csWeb/Resgrid.Web.Tts/Configuration/TtsOptions.csWeb/Resgrid.Web.Tts/DockerfileWeb/Resgrid.Web.Tts/Services/S3StorageService.csWeb/Resgrid.Web.Tts/Services/TtsService.csWeb/Resgrid.Web.Tts/k8s/deployment.yaml
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This was referenced May 2, 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
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
New Features
Improvements