Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe TTS service now uses updated cache and rate-limit defaults, downloads and selects Piper’s Ryan high English voice, adjusts audio generation parameters, and retries transient requests. Mapping flows add missing-department guards and an initial Leaflet map view, while AddPOI removes its widget initialization scripts. ChangesTTS updates
Mapping safeguards
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs`:
- Around line 208-217: The Piper command arguments in AudioProcessingService
must use underscore-form flags for v1.2.0. Update the argument names in the
affected startInfo configuration from --sentence-silence, --noise-scale, and
--noise-w to --sentence_silence, --noise_scale, and --noise_w while preserving
their existing values.
🪄 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: 6c316118-09a6-466b-9070-75bfb76a29f6
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (4)
Core/Resgrid.Config/TtsConfig.csWeb/Resgrid.Web.Tts/Configuration/TtsOptions.csWeb/Resgrid.Web.Tts/DockerfileWeb/Resgrid.Web.Tts/Services/AudioProcessingService.cs
| // 0.35s of silence between sentences — dispatch messages are strings of | ||
| // short sentences and need audible boundaries to stay intelligible. | ||
| startInfo.ArgumentList.Add("--sentence-silence"); | ||
| startInfo.ArgumentList.Add("0.0"); | ||
| startInfo.ArgumentList.Add("0.35"); | ||
| // Lower generation noise than the Piper defaults (0.667/0.8): reduces | ||
| // prosody jitter that makes digits and short words sound mumbled. | ||
| startInfo.ArgumentList.Add("--noise-scale"); | ||
| startInfo.ArgumentList.Add("0.333"); | ||
| startInfo.ArgumentList.Add("--noise-w"); | ||
| startInfo.ArgumentList.Add("0.4"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Download Piper v1.2.0 binary and check its help output for argument names.
curl -fsSL "https://github.com/rhasspy/piper/releases/download/v1.2.0/piper_amd64.tar.gz" -o piper.tar.gz
tar -xzf piper.tar.gz
./piper/piper --help | grep -E 'sentence|noise'Repository: Resgrid/Core
Length of output: 1266
Use Piper’s underscore flags here.
--sentence-silence, --noise-scale, and --noise-w are not recognized by Piper v1.2.0; use --sentence_silence, --noise_scale, and --noise_w instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs` around lines 208 -
217, The Piper command arguments in AudioProcessingService must use
underscore-form flags for v1.2.0. Update the argument names in the affected
startInfo configuration from --sentence-silence, --noise-scale, and --noise-w to
--sentence_silence, --noise_scale, and --noise_w while preserving their existing
values.
| public static string FfmpegExecutable = "ffmpeg"; | ||
| public static string TempDirectory = ""; | ||
| public static string CachePrefix = "tts"; | ||
| public static string CachePrefix = "tts2"; |
There was a problem hiding this comment.
WHAT: The k8s deployment.yaml (line 28) sets RESGRID__TtsConfig__CachePrefix: tts, which overrides the new code default tts2 via the env-var → TtsConfig → TtsOptions chain in ServiceCollectionExtensions.cs:60, defeating the cache-busting intent in production. WHY: Non-English voice model names (Spanish, German, French, etc.) did not change in this PR, so their SHA256 cache hashes are identical to before; the prefix change from tts → tts2 was the only mechanism to invalidate their stale cache entries, but the env var override keeps it at tts, causing non-English voices to serve old audio generated with sentence-silence 0.0, lowpass 3000, and no loudnorm/noise parameters indefinitely. HOW: Update the k8s deployment.yaml RESGRID__TtsConfig__CachePrefix value from tts to tts2 to match the code default, or remove the env var override so the code default takes effect.
public static string CachePrefix = "tts2";
// Also update Web/Resgrid.Web.Tts/k8s/deployment.yaml line 28:
// RESGRID__TtsConfig__CachePrefix: tts2Prompt for LLM
File Core/Resgrid.Config/TtsConfig.cs:
Line 35:
WHAT: The k8s deployment.yaml (line 28) sets `RESGRID__TtsConfig__CachePrefix: tts`, which overrides the new code default `tts2` via the env-var → TtsConfig → TtsOptions chain in ServiceCollectionExtensions.cs:60, defeating the cache-busting intent in production. WHY: Non-English voice model names (Spanish, German, French, etc.) did not change in this PR, so their SHA256 cache hashes are identical to before; the prefix change from `tts` → `tts2` was the only mechanism to invalidate their stale cache entries, but the env var override keeps it at `tts`, causing non-English voices to serve old audio generated with sentence-silence 0.0, lowpass 3000, and no loudnorm/noise parameters indefinitely. HOW: Update the k8s deployment.yaml `RESGRID__TtsConfig__CachePrefix` value from `tts` to `tts2` to match the code default, or remove the env var override so the code default takes effect.
Suggested Code:
public static string CachePrefix = "tts2";
// Also update Web/Resgrid.Web.Tts/k8s/deployment.yaml line 28:
// RESGRID__TtsConfig__CachePrefix: tts2
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // 0.35s of silence between sentences — dispatch messages are strings of | ||
| // short sentences and need audible boundaries to stay intelligible. | ||
| startInfo.ArgumentList.Add("--sentence-silence"); | ||
| startInfo.ArgumentList.Add("0.0"); | ||
| startInfo.ArgumentList.Add("0.35"); | ||
| // Lower generation noise than the Piper defaults (0.667/0.8): reduces | ||
| // prosody jitter that makes digits and short words sound mumbled. | ||
| startInfo.ArgumentList.Add("--noise-scale"); | ||
| startInfo.ArgumentList.Add("0.333"); | ||
| startInfo.ArgumentList.Add("--noise-w"); | ||
| startInfo.ArgumentList.Add("0.4"); |
There was a problem hiding this comment.
Violates rule 'Replace magic numbers with named constants': the new piper synthesis parameters 0.35 (sentence silence), 0.333 (noise scale), and 0.4 (noise width) on lines 211, 215, 217 are meaningful numeric literals that should be extracted into named constants. The same class already extracts similar tuning values (SpeedReferenceWpm=175f, MinLengthScale=0.25f, MaxLengthScale=3.0f), establishing the expected pattern.
// Named constants at the top of the class:
private const float SentenceSilence = 0.35f;
private const float NoiseScale = 0.333f;
private const float NoiseWidth = 0.4f;
// In CreatePiperStartInfo:
startInfo.ArgumentList.Add("--sentence-silence");
startInfo.ArgumentList.Add(SentenceSilence.ToString("0.00", CultureInfo.InvariantCulture));
startInfo.ArgumentList.Add("--noise-scale");
startInfo.ArgumentList.Add(NoiseScale.ToString("0.000", CultureInfo.InvariantCulture));
startInfo.ArgumentList.Add("--noise-w");
startInfo.ArgumentList.Add(NoiseWidth.ToString("0.0", CultureInfo.InvariantCulture));Prompt for LLM
File Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs:
Line 208 to 217:
Violates rule 'Replace magic numbers with named constants': the new piper synthesis parameters 0.35 (sentence silence), 0.333 (noise scale), and 0.4 (noise width) on lines 211, 215, 217 are meaningful numeric literals that should be extracted into named constants. The same class already extracts similar tuning values (SpeedReferenceWpm=175f, MinLengthScale=0.25f, MaxLengthScale=3.0f), establishing the expected pattern.
Suggested Code:
// Named constants at the top of the class:
private const float SentenceSilence = 0.35f;
private const float NoiseScale = 0.333f;
private const float NoiseWidth = 0.4f;
// In CreatePiperStartInfo:
startInfo.ArgumentList.Add("--sentence-silence");
startInfo.ArgumentList.Add(SentenceSilence.ToString("0.00", CultureInfo.InvariantCulture));
startInfo.ArgumentList.Add("--noise-scale");
startInfo.ArgumentList.Add(NoiseScale.ToString("0.000", CultureInfo.InvariantCulture));
startInfo.ArgumentList.Add("--noise-w");
startInfo.ArgumentList.Add(NoiseWidth.ToString("0.0", CultureInfo.InvariantCulture));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
| public static int RateLimitPermitLimit = 600; | ||
| public static int RateLimitQueueLimit = 60; |
There was a problem hiding this comment.
Environment variable overrides in Web/Resgrid.Web.Tts/k8s/deployment.yaml silently negate the rate-limit increase, as ConfigProcessor.LoadAndProcessEnvVariables (Program.cs:17) loads these values into static TtsConfig fields that ApplyRateLimitOptions (ServiceCollectionExtensions.cs:77-78) passes to FixedWindowRateLimiter at Program.cs:86-87, causing futile retry-on-429 attempts. Update the RESGRID__TtsConfig__RateLimitPermitLimit and RESGRID__TtsConfig__RateLimitQueueLimit overrides at lines 35-36 to match the intended 600/60 code defaults.
// Code defaults are correct; but deployment.yaml must be updated to match,
// otherwise the env override keeps production at 60/10:
// RESGRID__TtsConfig__RateLimitPermitLimit: "600"
// RESGRID__TtsConfig__RateLimitQueueLimit: "60"Prompt for LLM
File Core/Resgrid.Config/TtsConfig.cs:
Line 76 to 77:
WHAT: The rate-limit raise to 600 permits / 60 queue is silently overridden in the production k8s deployment, so it never takes effect. WHY: deployment.yaml still sets RESGRID__TtsConfig__RateLimitPermitLimit:"60" and RateLimitQueueLimit:"10"; ConfigProcessor.LoadAndProcessEnvVariables (Program.cs:17) loads those into the static TtsConfig fields, which ApplyRateLimitOptions (ServiceCollectionExtensions.cs:77-78) copies into RateLimitOptions, which Program.cs:86-87 feeds to the FixedWindowRateLimiter — env vars take precedence over the code defaults, so production still throttles at 60/10. Combined with the new retry-on-429 logic, retries within the same 60s fixed window are largely futile (permits don't free until window reset), so the dispatch fan-out failures the PR targets can still occur. HOW: update Web/Resgrid.Web.Tts/k8s/deployment.yaml lines 35-36 to RESGRID__TtsConfig__RateLimitPermitLimit:"600" and RESGRID__TtsConfig__RateLimitQueueLimit:"60" (or remove those overrides so the code defaults apply).
Suggested Code:
// Code defaults are correct; but deployment.yaml must be updated to match,
// otherwise the env override keeps production at 60/10:
// RESGRID__TtsConfig__RateLimitPermitLimit: "600"
// RESGRID__TtsConfig__RateLimitQueueLimit: "60"
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (department == null) | ||
| return new Coordinates() { Latitude = 39.14086268299356, Longitude = -119.7583809782715 }; |
There was a problem hiding this comment.
Magic number violation identified at line 308 and lines 371-372, where the default map center coordinates 39.14086268299356 and -119.7583809782715 are hardcoded as raw literals. Extract these fallback values into static readonly fields, following the existing cache key pattern at lines 17-25 to eliminate duplication across PersonnelController, UnitsController, MappingController v4, and LeafletMapView.tsx.
Kody rule violation: Replace magic numbers with named constants
private static readonly double DefaultLatitude = 39.14086268299356;
private static readonly double DefaultLongitude = -119.7583809782715;
// ...
if (department == null)
return new Coordinates() { Latitude = DefaultLatitude, Longitude = DefaultLongitude };Prompt for LLM
File Core/Resgrid.Services/DepartmentSettingsService.cs:
Line 307 to 308:
Violates rule 'Replace magic numbers with named constants': the default map center coordinates 39.14086268299356 and -119.7583809782715 are hardcoded as raw literals at line 308 and again at lines 371-372 within the same method. These are meaningful fallback values (default location when no department coordinates exist) that also appear identically in PersonnelController, UnitsController, and MappingController v4. They should be extracted to named static readonly constants, following the existing pattern in this class (e.g., lines 17-25 define static string constants for cache keys).
Suggested Code:
private static readonly double DefaultLatitude = 39.14086268299356;
private static readonly double DefaultLongitude = -119.7583809782715;
// ...
if (department == null)
return new Coordinates() { Latitude = DefaultLatitude, Longitude = DefaultLongitude };
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| model.Department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId); | ||
|
|
||
| if (model.Department == null) |
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
3 participants
Pull Request Description
This PR improves the quality and intelligibility of text-to-speech (TTS) audio generated for telephone dispatch calls.
Voice model change:
TTS generation tuning:
Telephone audio filter improvements:
Cache invalidation:
ttstotts2to force regeneration of all cached audio with the new voice and settings.All unit tests were updated to reflect the new model, generation parameters, and audio filter.
Summary by CodeRabbit
New Features
Bug Fixes