fix(engines): warn before a long CPU synth burns the whole budget - #1302
Merged
Conversation
#1288 closed the under-provisioned-GPU gap but left the CPU one open, and I missed it: a CPU-only host is a BENIGN routing verdict, so routingNotice() correctly stays silent — yet #1299 and #1260 are exactly that shape, CPU hosts that hit the 300s budget on long text with no warning at all. "Nothing is misconfigured" and "this will finish in time" are different claims. Threshold is the backend's own definition of past-short: generate_timeout_for() gives the first 1200 characters the flat budget before extending it, so ordinary sentences on a CPU laptop stay quiet and only the shape that actually times out is flagged. Hardware caveats still take precedence — one toast, and it names the real reason rather than generic advice. 5 tests; engines.cpuLongText translated in all 21 locales. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/utils/generatePreflight.js | The CPU-tuned engine IDs match the canonical backend registry and the alternate warning fully resolves the previous thread. |
| frontend/src/api/generate.ts | Supplies the FormData text to the existing non-blocking preflight without changing request behavior. |
| frontend/src/test/generatePreflight.test.js | Covers both canonical CPU-tuned engine IDs, generic-engine behavior, threshold handling, deduplication, acceleration, and warning precedence. |
| tests/conftest.py | Restores imported configuration path constants after tests that reload configuration modules. |
Reviews (3): Last reviewed commit: "fix(engines): don't tell CPU-tuned engin..." | Re-trigger Greptile
Contributor
📝 WalkthroughWalkthroughChangesCPU long-text warning
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
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 `@frontend/src/utils/generatePreflight.js`:
- Around line 75-80: Update the documentation associated with generatePreflight
to describe the 1200-character CPU/long-text warning threshold, that the warning
is emitted only once per engine, and the recommended alternatives. Keep the
documented behavior synchronized with LONG_TEXT_CHARS and the existing preflight
warning implementation.
- Line 94: Update the long-text guard in generatePreflight to count Unicode code
points with Array.from(text ?? '').length instead of String.length, matching the
backend threshold for astral characters. Preserve the existing onCpu and
LONG_TEXT_CHARS conditions, and add a boundary test covering astral text at the
configured threshold.
🪄 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 Plus
Run ID: 94489bb5-6d66-4094-b18a-ae7650d10109
📒 Files selected for processing (24)
frontend/src/api/generate.tsfrontend/src/i18n/locales/ar.jsonfrontend/src/i18n/locales/de.jsonfrontend/src/i18n/locales/en.jsonfrontend/src/i18n/locales/es.jsonfrontend/src/i18n/locales/fr.jsonfrontend/src/i18n/locales/hi.jsonfrontend/src/i18n/locales/id.jsonfrontend/src/i18n/locales/it.jsonfrontend/src/i18n/locales/ja.jsonfrontend/src/i18n/locales/ko.jsonfrontend/src/i18n/locales/nl.jsonfrontend/src/i18n/locales/pl.jsonfrontend/src/i18n/locales/pt.jsonfrontend/src/i18n/locales/ru.jsonfrontend/src/i18n/locales/sv.jsonfrontend/src/i18n/locales/th.jsonfrontend/src/i18n/locales/tr.jsonfrontend/src/i18n/locales/uk.jsonfrontend/src/i18n/locales/vi.jsonfrontend/src/i18n/locales/zh-CN.jsonfrontend/src/i18n/locales/zh-TW.jsonfrontend/src/test/generatePreflight.test.jsfrontend/src/utils/generatePreflight.js
Comment on lines
+75
to
+80
| // Mirrors the backend's own definition of "past short": generate_timeout_for() | ||
| // in services/model_manager.py gives the first 1200 characters the flat budget | ||
| // and only then starts extending it. Below this the job is inside the budget | ||
| // the backend considers generous; above it, on a CPU-class host, it is the | ||
| // shape that times out. Keep the two in sync. | ||
| const LONG_TEXT_CHARS = 1200; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the warning behavior. This PR adds a user-visible CPU/long-text threshold but includes no corresponding documentation; document the threshold, once-per-engine behavior, and suggested alternatives in this PR. As per coding guidelines, “Synchronize documentation in the same PR.”
🤖 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 `@frontend/src/utils/generatePreflight.js` around lines 75 - 80, Update the
documentation associated with generatePreflight to describe the 1200-character
CPU/long-text warning threshold, that the warning is emitted only once per
engine, and the recommended alternatives. Keep the documented behavior
synchronized with LONG_TEXT_CHARS and the existing preflight warning
implementation.
Source: Coding guidelines
| */ | ||
| function warnIfLongTextOnCpu(active, entry, text) { | ||
| const onCpu = entry?.routing_status === 'cpu_only' || entry?.effective_device === 'cpu'; | ||
| if (!onCpu || (text || '').length <= LONG_TEXT_CHARS) return; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "LONG_TEXT_CHARS|warnIfLongTextOnCpu|\\.length|repeat\\(" \
frontend/src/utils/generatePreflight.js \
frontend/src/test/generatePreflight.test.jsRepository: debpalash/OmniVoice-Studio
Length of output: 693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generatePreflight.js relevant section =="
sed -n '60,130p' frontend/src/utils/generatePreflight.js
echo
echo "== generator tests relevant section =="
sed -n '180,235p' frontend/src/test/generatePreflight.test.js
echo
echo "== search for LONG_TEXT_CHARS / preflight warning keys/docs =="
rg -n "LONG_TEXT_CHARS|long-text|Warning:|Text exceeds|max .characters|1200" -S .Repository: debpalash/OmniVoice-Studio
Length of output: 14576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re, subprocess
from pathlib import Path
js = Path("frontend/src/utils/generatePreflight.js").read_text()
# Extract warnIfLongTextOnCpu body and the call site(s) for context.
start = js.index("function warnIfLongTextOnCpu")
brace = js.index("{", start)
depth = 0
end = None
for i in range(brace, len(js)):
if js[i] == "{":
depth += 1
elif js[i] == "}":
depth -= 1
if depth == 0:
end = i + 1
break
func = js[start:end]
print("warnIfLongTextOnCpu:")
print(func)
print("uses .length literal:", ".length" in func)
print("uses Array.from literal:", "Array.from" in func)
PY
node - <<'JS'
const text = '😀'.repeat(601);
console.log("JavaScript text.length:", text.length);
console.log("Array.from(text).length:", Array.from(text).length);
console.log("'abc😀def'.length:", 'abc😀def'.length);
console.log("Array.from('abc😀def').length:", Array.from('abc😀def').length);
JSRepository: debpalash/OmniVoice-Studio
Length of output: 798
Match the backend’s character grapheme threshold at Line 94. String.length counts UTF-16 code units, so 601 astral characters trip at 1,202 while the Python backend uses 601, causing false-long-text warnings; use Array.from(text ?? '').length and add an astral-code-point boundary test.
🤖 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 `@frontend/src/utils/generatePreflight.js` at line 94, Update the long-text
guard in generatePreflight to count Unicode code points with Array.from(text ??
'').length instead of String.length, matching the backend threshold for astral
characters. Preserve the existing onCpu and LONG_TEXT_CHARS conditions, and add
a boundary test covering astral text at the configured threshold.
Greptile P1. The advice names OmniVoice GGUF and Supertonic-3 as the CPU-tuned alternatives — shown to someone already running one of them, it is advice to switch to what they are using. Those two now get the same warning without the self-referential clause; the engine set matches the backend's own timeout message so the two can't disagree about who is CPU-tuned. Also documents the preflight in docs/performance.md (docs-sync rule): both warning shapes, why the threshold is 1200 characters (it is the figure the budget itself uses), that they are advisory and once-per-engine-per-session, and the CPU-tuned exception. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
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 `@tests/conftest.py`:
- Around line 516-518: Update the fixture’s finally block around the core.config
lookup so it conditionally skips restoration when the module is absent instead
of returning from finally. Preserve restoration when cfg exists, and ensure any
original test exception propagates unchanged.
🪄 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 Plus
Run ID: dffdd7f2-0ef6-4b09-a0f1-9e3bdaa1a231
📒 Files selected for processing (25)
docs/performance.mdfrontend/src/i18n/locales/ar.jsonfrontend/src/i18n/locales/de.jsonfrontend/src/i18n/locales/en.jsonfrontend/src/i18n/locales/es.jsonfrontend/src/i18n/locales/fr.jsonfrontend/src/i18n/locales/hi.jsonfrontend/src/i18n/locales/id.jsonfrontend/src/i18n/locales/it.jsonfrontend/src/i18n/locales/ja.jsonfrontend/src/i18n/locales/ko.jsonfrontend/src/i18n/locales/nl.jsonfrontend/src/i18n/locales/pl.jsonfrontend/src/i18n/locales/pt.jsonfrontend/src/i18n/locales/ru.jsonfrontend/src/i18n/locales/sv.jsonfrontend/src/i18n/locales/th.jsonfrontend/src/i18n/locales/tr.jsonfrontend/src/i18n/locales/uk.jsonfrontend/src/i18n/locales/vi.jsonfrontend/src/i18n/locales/zh-CN.jsonfrontend/src/i18n/locales/zh-TW.jsonfrontend/src/test/generatePreflight.test.jsfrontend/src/utils/generatePreflight.jstests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (19)
- frontend/src/i18n/locales/tr.json
- frontend/src/i18n/locales/zh-CN.json
- frontend/src/i18n/locales/ja.json
- frontend/src/i18n/locales/nl.json
- frontend/src/i18n/locales/ru.json
- frontend/src/i18n/locales/pt.json
- frontend/src/i18n/locales/zh-TW.json
- frontend/src/i18n/locales/fr.json
- frontend/src/i18n/locales/es.json
- frontend/src/i18n/locales/it.json
- frontend/src/i18n/locales/vi.json
- frontend/src/i18n/locales/ko.json
- frontend/src/i18n/locales/pl.json
- frontend/src/i18n/locales/hi.json
- frontend/src/i18n/locales/ar.json
- frontend/src/i18n/locales/uk.json
- frontend/src/i18n/locales/de.json
- frontend/src/test/generatePreflight.test.js
- frontend/src/utils/generatePreflight.js
Comment on lines
+516
to
+518
| cfg = sys.modules.get("core.config") | ||
| if cfg is None: | ||
| return |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not return from the fixture’s finally block.
If a test removes core.config from sys.modules and fails, Line 518 suppresses that failure, creating a false-positive test result. Replace the return with a conditional that skips restoration while allowing the original exception to propagate.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 518-518: return inside finally blocks cause exceptions to be silenced
(B012)
🤖 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 `@tests/conftest.py` around lines 516 - 518, Update the fixture’s finally block
around the core.config lookup so it conditionally skips restoration when the
module is absent instead of returning from finally. Preserve restoration when
cfg exists, and ensure any original test exception propagates unchanged.
Source: Linters/SAST tools
This was referenced Jul 29, 2026
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.
Closes #1299, #1260.
The gap I left in #1288
#1288 warns when the GPU is under-provisioned. A CPU-only host is a benign routing verdict — nothing is misconfigured — so
routingNotice()correctly stays silent for it.But "nothing is misconfigured" and "this will finish in time" are different claims. #1299 and #1260 are both CPU hosts that hit the 300s budget on long text and got no warning at all, which is the exact failure #1288 was meant to prevent. I missed it.
Threshold
Not invented — the backend's own definition of past-short.
generate_timeout_for()inservices/model_manager.pygives the first 1200 characters the flat budget and only then starts extending it. Below that the job is inside a budget the backend already considers generous; above it, on CPU-class hardware, it is the shape that times out.Ordinary sentences on a CPU laptop stay quiet. The constant carries a comment pointing at its backend twin so the two don't drift.
Precedence
A real hardware caveat still wins — one toast, naming the routing reason, not generic "long text" advice. There's a test pinning that (
cpu_fallback+ long text → the caveat, and explicitly notcpuLongText).Tests
5 new cases: fires on long text on a CPU host, quiet on short text, quiet on long text on an accelerated host, once-only per engine, and the precedence case above.
engines.cpuLongTexttranslated in all 21 locales. Full frontend suite 1615 passing; locale parity + CJK guard green.Adds a CPU-only preflight toast when synthesis input text exceeds 1,200 characters, preserving existing hardware caveat precedence and ensuring the warning fires once per engine per session (with deduping and CPU-tuned suppression), translated across all 21 locales. This targets CPU generations that can exceed the backend compute budget and get abandoned while still consuming resources. Risk worth a human look: verify CPU/routing detection, the “once per engine” dedupe logic, and warning precedence across engine types and translations.