fix(portal): address CodeRabbit review findings - #6
Conversation
- TranslateRoutes: move readiness gate after engine resolution so DeepL requests are not blocked by the local-model-not-ready check; use IsNullOrWhiteSpace instead of ?? to reject empty-string engine values - ModelSettingsStore: apply env-override + NonEmpty fallback to PreferredTranslationEngine in Merge(), matching all other fields - SettingsRoutes: add PATCH /api/settings/engine narrow endpoint so the dashboard can update only the engine without a read-modify-write cycle that would persist env-derived repo/dir/files values to disk - Dashboard: use PATCH /api/settings/engine; surface save failures via setErr() instead of silent console.error; clear err on retry - JobsListPage: restructure JobCard as sibling native <button> elements (no more div[role=button] inside <button>); add catch + deleteError signal to confirmDelete so failures are visible to the user - Rebuild client bundle Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds a narrow translation-engine settings endpoint, updates engine precedence and translation readiness checks, and refactors job-card deletion controls with failure feedback. Bundled frontend assets reflect the client interaction and styling changes. ChangesTranslation engine flow
Job deletion interactions
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard
participant SettingsRoutes
participant ModelSettingsStore
participant TranslateRoutes
User->>Dashboard: Select translation engine
Dashboard->>SettingsRoutes: PATCH /api/settings/engine
SettingsRoutes->>ModelSettingsStore: Persist preferred engine
ModelSettingsStore-->>SettingsRoutes: Updated settings
SettingsRoutes-->>Dashboard: Updated engine
User->>TranslateRoutes: Submit translation text and optional engine
TranslateRoutes->>TranslateRoutes: Validate text and select engine
TranslateRoutes-->>User: Continue or return readiness status
Possibly related PRs
🚥 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: 5
🤖 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 `@server/ClientApp/src/pages/JobsListPage.tsx`:
- Around line 194-202: Add role="alert" to the dynamically rendered
deletion-error div containing msg() in JobsListPage so assistive technology
announces deletion failures, preserving the existing dismissal behavior and
styling.
- Around line 62-69: Update the delete button in JobsListPage so it remains
visible by default on touch devices while preserving the current hover/focus
reveal behavior for capable devices. Adjust the opacity utility classes on the
button containing Trash2, using responsive or device-appropriate variants rather
than relying solely on group-hover.
In `@server/src/ModelSettingsStore.cs`:
- Around line 196-202: Update the PreferredTranslationEngine selection to ignore
whitespace-only PREFERRED_TRANSLATION_ENGINE values by validating with
!string.IsNullOrWhiteSpace. Trim the accepted environment override before
assigning it, while preserving the existing file-value and env-derived default
fallback order.
In `@server/src/Routes/SettingsRoutes.cs`:
- Around line 33-35: Replace the read-modify-write sequence in the settings
route with a store method that clones the current settings, applies the
requested engine update, and persists it while holding _writeLock. Update the
route to call this atomic method instead of modifying store.Current and passing
the full object to UpdateAsync, preserving unrelated concurrent settings.
In `@server/src/Routes/TranslateRoutes.cs`:
- Around line 20-27: Update the engine-selection flow in the translation route
to resolve “auto” to DeepL when configured, otherwise to the local backend,
before evaluating needsLocal. Apply the readiness check against this resolved
backend and enqueue the same resolved engine, preserving explicit engine
selections.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f111dad-353b-4725-88c4-064eb9880b12
📒 Files selected for processing (7)
server/ClientApp/src/components/Dashboard.tsxserver/ClientApp/src/pages/JobsListPage.tsxserver/src/ModelSettingsStore.csserver/src/Routes/SettingsRoutes.csserver/src/Routes/TranslateRoutes.csserver/wwwroot/js/app.cssserver/wwwroot/js/app.js
- JobsListPage: remove opacity-0/group-hover on delete button so it is always visible on touch devices (was undiscoverable without hover) - JobsListPage: add role="alert" to delete-error banner for screen reader announcement - ModelSettingsStore: add UpdateEngineAsync() that clones _current and persists atomically inside _writeLock, preventing a race between PATCH and concurrent PUT /api/settings - SettingsRoutes: use UpdateEngineAsync() in PATCH /api/settings/engine instead of the racy read-modify-write on store.Current - TranslateRoutes: resolve "auto" engine to deepl/local before needsLocal check so DeepL is used when configured and the local model is not ready - ModelSettingsStore: guard PREFERRED_TRANSLATION_ENGINE env var with IsNullOrWhiteSpace + Trim() to reject whitespace-only values Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@server/src/ModelSettingsStore.cs`:
- Around line 140-145: Update the persistence flow in the method containing
_current and _writeLock so the serialized updated settings are written
successfully, including directory creation, before assigning _current. Keep the
assignment inside the existing _writeLock scope, and only log and return the
committed value after persistence succeeds.
- Around line 126-138: Update UpdateEngineAsync to trim the engine value before
assigning PreferredTranslationEngine, then validate the normalized value at this
persistence boundary and reject it when it is empty or whitespace-only. Store
and propagate only the normalized, valid engine while preserving the existing
settings update flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67fc1730-2834-4348-9b0a-7cd397f75c2d
📒 Files selected for processing (5)
server/ClientApp/src/pages/JobsListPage.tsxserver/src/ModelSettingsStore.csserver/src/Routes/SettingsRoutes.csserver/src/Routes/TranslateRoutes.csserver/wwwroot/js/app.js
🚧 Files skipped from review as they are similar to previous changes (3)
- server/src/Routes/SettingsRoutes.cs
- server/src/Routes/TranslateRoutes.cs
- server/ClientApp/src/pages/JobsListPage.tsx
- Validate and trim engine before _writeLock in UpdateEngineAsync so
whitespace-padded values are rejected at the persistence boundary
- Move _current assignment to after File.WriteAllTextAsync so in-memory
state only reflects what was successfully persisted
- Add .coderabbit.yaml to exclude server/wwwroot/js/app.{js,css} from
CodeRabbit reviews (generated Vite artifacts, not hand-written code)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
73a1de0 to
09b7575
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
IsNullOrWhiteSpacefallback replaces??to reject empty-string engine valuesPreferredTranslationEngineinMerge()now uses env-override +IsNullOrWhiteSpacefallback, consistent with all other fieldsPATCH /api/settings/enginenarrow endpoint — updates only the engine field without persisting env-derived repo/dir/files values to diskPATCH /api/settings/engine; surface save failures viasetErr()instead of silentconsole.error; clear error on retryJobCardas two sibling native<button>elements inside a<div>— eliminates nested interactive controls (div[role=button]inside<button>); addcatch+deleteErrorsignal so failed deletes show a dismissable bannerThese are the 6 actionable findings from the CodeRabbit review of PR #5 that were fixed after the PR was merged.
Test plan
TRANSLATE_MODEL_ENABLED=falsePATCH /api/settings/enginereturns{ preferred_translation_engine }and persists across restart🤖 Generated with Claude Code
Summary by CodeRabbit