fix(engines): show the under-provisioned-VRAM warning instead of discarding it - #1280
Merged
Conversation
…arding it Four of the open low-VRAM reports (#1240, #1246, #1248 on 4 GB cards; #1277 on 6 GB) share one shape: the user generates, waits out the entire 300s compute budget, and is then told the job "was too heavy for the available compute". The warning existed the whole time. Routing computes it (#1226's `_caveat`: "…has 4.0 GB VRAM; this engine wants about 6 GB. It will run, but expect slow generations that may time out"), and `/engines/select` echoes it in `routing_reason` — but notifyEngineSelected only surfaced a reason when `routing_status === 'cpu_fallback'`. The VRAM caveat rides on an ACCELERATED verdict, so it fell through to the green "switched" success toast and was thrown away. The user was told everything was fine, then waited five minutes to find out it wasn't. Now any caveat on the echo raises a warn-tone toast naming it, with a longer duration since it lists the ways around the limit. This covers the kernel-risk caveat on the same path. Deliberately still ADVISORY, not blocking — matching the routing layer's documented contract (the driver can page to system RAM, and short inputs fit where long ones don't). The engine is still selected; the user just finds out now instead of after the timeout. This is the first-run path too: the wizard's library step shares notifyEngineSelected. Fail-before verified: both new tests fail against the previous version. Known remaining gap: a user whose engine is already selected sees this only when they re-pick. A generate-time preflight would close that, but it needs a "once per session, not per generate" design — filed as follow-up rather than guessed at here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/utils/engineSelectToast.js | The notification predicate matches the backend routing-notice contract and preserves benign status handling. |
| frontend/src/test/engineSelectToast.test.js | Regression coverage exercises VRAM and kernel caveats plus the previously reported CPU-only and unavailable false-positive cases. |
| tests/test_locale_parity.py | The new check prevents placeholder-only values while retaining an explicit exception mechanism for legitimate cases. |
| frontend/src/i18n/locales/en.json | Adds the user-facing caveat message and replaces the placeholder-only engine-switch notification with prose. |
Reviews (4): Last reviewed commit: "style: oxfmt the new toast test cases" | Re-trigger Greptile
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe frontend adds localized engine-selection messages across locales, shows warning toasts for accelerated routing caveats, tests guarded routing outcomes, and strengthens locale parity validation. ChangesEngine caveat feedback
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 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/i18n/locales/ar.json`:
- Around line 581-582: Replace the bare selectWithCaveat value with a natural,
locale-specific warning phrase in frontend/src/i18n/locales/ar.json:581-582,
de.json:581-582, es.json:581-582, fr.json:581-582, hi.json:581-582,
id.json:581-582, it.json:581-582, ja.json:581-582, ko.json:581-582, and
nl.json:581-582. Preserve both {{engine}} and {{reason}} placeholders in every
translation; routingCaveatTitle requires no direct change.
In `@frontend/src/i18n/locales/pl.json`:
- Around line 581-582: Replace the untranslated selectWithCaveat value with
natural localized wording around the {{engine}} and {{reason}} placeholders,
preserving both placeholders and the warning meaning. Apply the corresponding
translations in frontend/src/i18n/locales/pl.json#L581-L582, pt.json#L581-L582,
ru.json#L581-L582, sv.json#L581-L582, th.json#L581-L582, tr.json#L581-L582,
uk.json#L581-L582, vi.json#L581-L582, zh-CN.json#L540-L541, and
zh-TW.json#L581-L582.
🪄 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: 6be911b3-d036-4d2b-b5da-edb79705abef
📒 Files selected for processing (23)
frontend/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/engineSelectToast.test.jsfrontend/src/utils/engineSelectToast.js
…eholder
CodeRabbit flagged engines.selectWithCaveat as untranslated in 20 locales.
It was worse than that: en.json said "{{engine}}: {{reason}}" too, so the
English string had never been written and every "translation" was a faithful
copy of a non-sentence. All 21 languages would have shown a bare
"omnivoice: <English backend text>".
Writes the en sentence, translates it into all 20, and translates
engines.selectCpuFallback alongside it — same function, same toast, and it
was English-only in every locale (missing-key ratchet tightened 518 -> 517,
zh-CN 511 -> 510).
Adds test_no_placeholder_only_values to pin the class. Parity tests cannot
catch this: the key is present everywhere and the placeholders match exactly.
Only the absence of prose gives it away, so the guard checks en.json too —
that is where this one started. A bot catching a mechanical rule twice means
the rule belongs in CI (CLAUDE.md, Token economy).
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/test_locale_parity.py`:
- Around line 42-50: Remove settings.engine_switched from
_PLACEHOLDER_ONLY_ALLOWLIST, then update that key in all 21 locale JSON files
with localized user-facing prose while preserving the placeholder values and
intended engine-switch success meaning.
🪄 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: 8a4670b8-54b8-41fd-837e-36a4232d5a50
📒 Files selected for processing (22)
frontend/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.jsontests/test_locale_parity.py
🚧 Files skipped from review as they are similar to previous changes (18)
- frontend/src/i18n/locales/hi.json
- frontend/src/i18n/locales/vi.json
- frontend/src/i18n/locales/uk.json
- frontend/src/i18n/locales/id.json
- frontend/src/i18n/locales/es.json
- frontend/src/i18n/locales/tr.json
- frontend/src/i18n/locales/zh-CN.json
- frontend/src/i18n/locales/ru.json
- frontend/src/i18n/locales/ja.json
- frontend/src/i18n/locales/zh-TW.json
- frontend/src/i18n/locales/it.json
- frontend/src/i18n/locales/nl.json
- frontend/src/i18n/locales/de.json
- frontend/src/i18n/locales/en.json
- frontend/src/i18n/locales/sv.json
- frontend/src/i18n/locales/fr.json
- frontend/src/i18n/locales/pt.json
- frontend/src/i18n/locales/ko.json
…ce()
Greptile P1: testing a bare `routing_reason` also fires on benign verdicts.
Routing rule 5 gives a Windows DirectML host cpu_only + an explanatory reason
on a perfectly normal pick, and rule 6 attaches one to `unavailable` — neither
is a hardware warning, but both drew a 10s amber toast. routing_notice() in
engine_routing.py is the canonical predicate (cpu_fallback always, accelerated
only with a reason); the frontend now matches it. Two tests, both failing
before.
Also translates settings.engine_switched, which shipped as the identical
"{{family}} → {{engine}}" in all 21 files — an untranslated success toast
everywhere (CodeRabbit). That was the sole _PLACEHOLDER_ONLY_ALLOWLIST entry,
so the allowlist is now empty and the guard has no exceptions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Four open low-VRAM reports share one shape:
The user generates, waits out the entire 300 s compute budget, and is then told the job "was too heavy for the available compute".
The warning already existed
#1226 added exactly the right message in
engine_routing._caveat:/engines/selectechoes it inrouting_reason. ButnotifyEngineSelectedsurfaced a reason only whenrouting_status === 'cpu_fallback'— and the VRAM caveat rides on anacceleratedverdict. So it fell through to the plain green "switched" success toast and was discarded. The user was told everything was fine, then waited five minutes to learn it wasn't.Change
Any caveat on the echo now raises a warn-tone toast naming it (longer duration, since it lists the ways around the limit). Also covers the kernel-risk caveat, which took the same path.
Still advisory, not blocking — matching the routing layer's documented contract: the driver can page to system RAM, and short inputs fit where long ones don't. The engine is still selected. This is the first-run path too, since the wizard's library step shares
notifyEngineSelected.Verification
engines.selectWithCaveatadded to all 21 locales; locale-parity + CJK suites green.Known remaining gap
A user whose engine is already selected sees this only when they re-pick. A generate-time preflight would close that, but it needs a "once per session, not once per generate" design — flagging rather than guessing at it here.
Refs #1226, #1240, #1246, #1248, #1277
Updated engine-selection toasts so accelerated routing caveats (including VRAM limits and kernel-risk) surface as longer, advisory warning toasts using the new
engines.selectWithCaveat/selectCpuFallbacki18n strings, and the selection remains unchanged. This extends the first-run wizard flow and adds/adjustsengine_switchedcopy across all 21 locales, along with frontend tests and stricter locale-parity checks to prevent placeholder-only translations. Main risk to review: users with an already-selected engine may only see the warning upon reselecting it, since generate-time preflight is deferred.