Skip to content

RE1-T115 TTS fixes#369

Merged
ucswift merged 2 commits intomasterfrom
develop
May 5, 2026
Merged

RE1-T115 TTS fixes#369
ucswift merged 2 commits intomasterfrom
develop

Conversation

@ucswift
Copy link
Copy Markdown
Member

@ucswift ucswift commented May 5, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Adjusted default text-to-speech playback speed for improved clarity.
    • Enhanced sentence silence handling in audio generation.
    • Improved text preprocessing with automatic trailing punctuation enforcement.
    • Added support for expanding long numbers into individual digits for English voices to ensure proper pronunciation.

@request-info
Copy link
Copy Markdown

request-info Bot commented May 5, 2026

Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 5, 2026

Warning

Rate limit exceeded

@ucswift has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 35 minutes and 3 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 360410d9-061e-4d62-b981-d066320ea06e

📥 Commits

Reviewing files that changed from the base of the PR and between 32d9429 and 136bd58.

📒 Files selected for processing (2)
  • Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs
  • Web/Resgrid.Web.Services/Resgrid.Web.Services.xml
📝 Walkthrough

Walkthrough

This PR adjusts TTS configuration defaults from 165 to 150 and enhances audio preprocessing: adding sentence-silence control to Piper arguments, enforcing trailing punctuation on all text, and implementing long-number expansion for English voices.

Changes

TTS Configuration & Preprocessing Enhancement

Layer / File(s) Summary
Configuration Defaults
Core/Resgrid.Config/TtsConfig.cs, Web/Resgrid.Web.Tts/Configuration/TtsOptions.cs
DefaultSpeed reduced from 165 to 150 in both config classes.
Text Preprocessing
Web/Resgrid.Web.Tts/Services/TextPreprocessor.cs
Adds LongNumberRegex and LongNumberExpandoRegex for detecting 4+ consecutive digits. Implements ExpandLongNumbers (space-separates long digit sequences for English voices only). Enforces trailing sentence-ending punctuation (., !, ?) for all languages. English-voice preprocessing now includes long-number expansion with debug logging.
Audio Processing
Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs
CreatePiperStartInfo adds --sentence-silence 0.0 argument to Piper CLI after --length-scale to control inter-sentence silence.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Resgrid/Core#358: Updates the same TTS configuration defaults (DefaultSpeed in TtsConfig and TtsOptions).
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'RE1-T115 TTS fixes' is vague and generic, using non-descriptive terms like 'fixes' without clearly conveying what specific changes are being made to the TTS system. Consider using a more descriptive title that specifies the main change, such as 'Adjust TTS default speed and add sentence silence handling' or similar, to help reviewers quickly understand the changeset.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch develop

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 166-167: Update the three unit tests
(create_piper_start_info_should_build_correct_arguments,
create_piper_start_info_should_fallback_to_default_model_for_unmapped_languages,
create_piper_start_info_should_adjust_length_scale_for_speed) to reflect the new
Piper argument list by adding the two expected entries "--sentence-silence" and
"0.0" to the expected sequence used in the ArgumentList.Should().Equal(...)
assertions; ensure the new entries are inserted in the same relative position as
produced by AudioProcessingService.BuildPiperStartInfo (so the expected list now
includes --model, modelPath, --output_file, outPath, --length-scale, value,
--sentence-silence, "0.0").
🪄 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: 3647835e-d7eb-4d28-935e-f270c96a50b7

📥 Commits

Reviewing files that changed from the base of the PR and between 878add7 and 32d9429.

📒 Files selected for processing (4)
  • Core/Resgrid.Config/TtsConfig.cs
  • Web/Resgrid.Web.Tts/Configuration/TtsOptions.cs
  • Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs
  • Web/Resgrid.Web.Tts/Services/TextPreprocessor.cs

Comment thread Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs
Comment on lines +220 to +226
if (!string.Equals(original, result, StringComparison.Ordinal))
{
_logger.LogDebug(
"TextPreprocessor normalised \"{OriginalText}\" to \"{NormalisedText}\"",
original,
result);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid logging raw dispatch text in normalization debug logs.

This logs full pre/post message content, which can leak sensitive incident/person/location data into logs. Log only metadata (e.g., changed=true, lengths, hash) and use the project logging standard.

Proposed safe logging change
-				if (!string.Equals(original, result, StringComparison.Ordinal))
-				{
-					_logger.LogDebug(
-						"TextPreprocessor normalised \"{OriginalText}\" to \"{NormalisedText}\"",
-						original,
-						result);
-				}
+				if (!string.Equals(original, result, StringComparison.Ordinal))
+				{
+					_logger.LogDebug(
+						"TextPreprocessor normalized text. OriginalLength={OriginalLength}, NormalizedLength={NormalizedLength}",
+						original.Length,
+						result.Length);
+				}

As per coding guidelines: "Use Resgrid.Framework.Logging.LogException(), LogError(), LogInfo(), or LogDebug() for all logging throughout the codebase".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!string.Equals(original, result, StringComparison.Ordinal))
{
_logger.LogDebug(
"TextPreprocessor normalised \"{OriginalText}\" to \"{NormalisedText}\"",
original,
result);
}
if (!string.Equals(original, result, StringComparison.Ordinal))
{
_logger.LogDebug(
"TextPreprocessor normalized text. OriginalLength={OriginalLength}, NormalizedLength={NormalizedLength}",
original.Length,
result.Length);
}

{
_logger.LogDebug(
"TextPreprocessor normalised \"{OriginalText}\" to \"{NormalisedText}\"",
original,
@ucswift
Copy link
Copy Markdown
Member Author

ucswift commented May 5, 2026

Approve

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is approved.

@ucswift ucswift merged commit b0a5c00 into master May 5, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants