Fix reader translation popover defaults - #27
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements Task 21, a comprehensive workflow to unify Codex translation popovers via a shared translucent ChangesTask 21: Popover and Translation UI Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 833222862f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
review @coderabbitai |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch PR details and diff
gh pr view 27 --repo hauntedfail/Trauma --json title,body,files,additions,deletions,changedFiles 2>/dev/null | jq .
echo "=== PR FILES ==="
gh pr diff 27 --repo hauntedfail/Trauma 2>/dev/null | head -2000Length of output: 98554 🏁 Script executed: #!/bin/bash
gh pr diff 27 --repo hauntedfail/Trauma 2>/dev/null | tail -n +2001 | head -2000Length of output: 11268
Fix reader translation popover defaults — ReviewThe overall approach is solid: migrating to the shared 🔴 Issue 1 — Translation submit does not persist defaults to the DB
// src/components/reader/MemoryReader.tsx
setTranslationDefaultLanguage(input.langCode);
setTranslationDefaultModel(input.model ?? "");
setTranslationDefaultEffort(input.reasoningEffort ?? "");
void revalidateSettingsState(); // re-fetches DB — still has OLD valuesBecause Suggested fix — call the settings PATCH before revalidating: // Before revalidateSettingsState()
if (input.model !== null || input.reasoningEffort !== null) {
await fetch("/api/settings/translation-codex-defaults", {
method: "PATCH",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
...(input.model !== null && { model: input.model }),
...(input.reasoningEffort !== null && { reasoning_effort: input.reasoningEffort }),
}),
});
}
void revalidateSettingsState();Or route through the existing 🟡 Issue 2 —
|
|
Handled the new review from CodeRabbit: #27 (comment) Classifications and outcomes:
Validation:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/references/design-system/components-and-surfaces.md (1)
125-126: 💤 Low valueConsider rephrasing for readability.
Three successive sentences begin with "Theme", which may reduce readability. Consider varying sentence structure.
✍️ Suggested rephrase
- Theme, Add memory, action-menu, taxonomy, and reader translation popovers use the shared translucent elevated panel with backdrop blur. -- Theme selected state is visible inside the popover in every theme. -- Theme popover buttons remain labelled and readable below desktop. +- Selected state is visible inside the theme popover in every theme. +- Popover buttons remain labelled and readable below desktop.🤖 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 `@docs/references/design-system/components-and-surfaces.md` around lines 125 - 126, The three consecutive sentences starting with the word "Theme" reduce readability; rephrase them to vary sentence openings and improve flow—locate the sentences that reference the placeholder `text-trauma-text-placeholder` and the minimum height `42px` in the "components-and-surfaces.md" section and rewrite so one or two sentences start with alternative constructions (e.g., "This theme...", "For this theme...", or by leading with the capability or constraint) while preserving the meaning and the exact technical tokens `text-trauma-text-placeholder` and `42px`.
🤖 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 `@e2e/reader.spec.ts`:
- Around line 130-141: Replace the race-prone assertion that checks the
route-side counter (translationStartCount) after dialog.getByRole(...).click
with an explicit network sync: after triggering the Translate button
(dialog.getByRole(...).click), wait for the matching POST request/response (the
/translate route used by the page.route handler) using Playwright's
waitForRequest or waitForResponse, then assert the request payload equals the
expected object (compare the parsed JSON to the expected
lang_code/model/reasoning_effort) and that the dialog is closed
(expect(dialog).toHaveCount(0)); remove reliance on translationStartCount and
keep lastTranslationBody assertions tied to the awaited request.
In `@src/components/reader/MemoryReader.tsx`:
- Around line 613-625: The submitTranslationDialog currently reads
translationFormLanguage() but relies on canStartTranslation(), which only
inspects props.translationTargetLanguage, allowing duplicate requests when the
user selects a different language; update the validation so
submitTranslationDialog (and the similar handler around startTranslation later)
checks the selected language/effort directly before calling startTranslation —
specifically, use translationFormLanguage() (and translationFormEffort()) to
verify no existing variant/duplicate exists instead of relying on
canStartTranslation(), and only call startTranslation({ close, langCode, model:
canonicalTranslationModel(), ... }) when that selected-language check passes.
- Around line 651-666: The bug is that after submitCodexTranslationDefaults()
succeeds but startReaderTranslation() throws, the UI state
(setTranslationDefaultLanguage/setTranslationDefaultModel/setTranslationDefaultEffort)
and revalidateSettingsState() never run, leaving stale defaults; fix by ensuring
those state updates and revalidation run immediately after
submitCodexTranslationDefaults() succeeds (using
persistedModel/persistedReasoningEffort/langCode) and before calling
startReaderTranslation(), or alternatively move them into a finally block so
they always execute even if startReaderTranslation() throws; update the block
around submitCodexTranslationDefaults, startReaderTranslation,
setTranslationDefaultLanguage, setTranslationDefaultModel,
setTranslationDefaultEffort, and revalidateSettingsState accordingly.
---
Nitpick comments:
In `@docs/references/design-system/components-and-surfaces.md`:
- Around line 125-126: The three consecutive sentences starting with the word
"Theme" reduce readability; rephrase them to vary sentence openings and improve
flow—locate the sentences that reference the placeholder
`text-trauma-text-placeholder` and the minimum height `42px` in the
"components-and-surfaces.md" section and rewrite so one or two sentences start
with alternative constructions (e.g., "This theme...", "For this theme...", or
by leading with the capability or constraint) while preserving the meaning and
the exact technical tokens `text-trauma-text-placeholder` and `42px`.
🪄 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
Run ID: beb80bc6-62c7-4926-a46f-b462d149cf6c
📒 Files selected for processing (30)
docs/architecture/data-and-storage.mddocs/references/configuration.mddocs/references/design-system/components-and-surfaces.mddocs/references/design-system/interaction-and-accessibility.mddocs/references/design-system/verification.mddocs/workflows/README.mddocs/workflows/task-21-popover-and-translation-ui-fixes/01-codex-default-persistence-contract.mddocs/workflows/task-21-popover-and-translation-ui-fixes/02-settings-api-and-route-state.mddocs/workflows/task-21-popover-and-translation-ui-fixes/03-shared-popover-visual-contract.mddocs/workflows/task-21-popover-and-translation-ui-fixes/04-reader-translation-popover-migration.mddocs/workflows/task-21-popover-and-translation-ui-fixes/05-popover-consumer-audit.mddocs/workflows/task-21-popover-and-translation-ui-fixes/06-translation-integration-regression-checks.mddocs/workflows/task-21-popover-and-translation-ui-fixes/07-browser-verification-and-handoff.mddocs/workflows/task-21-popover-and-translation-ui-fixes/README.mde2e/reader.spec.tssrc/components/memories/TaxonomyAddControl.tsxsrc/components/reader/MemoryReader.tsxsrc/components/settings/settings-loader.tssrc/components/ui/KebabActionMenu.tsxsrc/components/ui/Popup.tsxsrc/server/db/repositories.tstests/components/dismissable-layer.test.tstests/components/memory-reader-actions.test.tstests/components/popup.test.tsxtests/components/settings-loader.test.tstests/server/backup/backup-environment.test.tstests/server/db/schema.test.tstests/server/routes/api-settings.test.tstests/server/settings/settings.test.tstests/server/translation/translation-repositories.test.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9cbd54fb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model: persistedModel, | ||
| reasoningEffort: persistedReasoningEffort, |
There was a problem hiding this comment.
When the user leaves either select on the “Codex app-server default” / “Selected model default” option, the settings PATCH correctly returns null, but these changed arguments still send those null values as explicit fields in the translation POST. The translation runner treats any defined model or reasoningEffort as an explicit override and then writes the resolved concrete selection back to settings (src/server/translation/runner.ts lines 259-273), so a translation immediately overwrites the just-preserved null default with e.g. the current default model/effort and the popover no longer reopens on the default choices. Omit these fields when the persisted value is null/undefined so the runner uses settings without re-persisting a concrete selection.
Useful? React with 👍 / 👎.
| }); | ||
| const persistedModel = settings.codexTranslationModel; | ||
| const persistedReasoningEffort = settings.codexTranslationReasoningEffort; | ||
| setTranslationDefaultLanguage(input.langCode); |
There was a problem hiding this comment.
When the user changes the popover Language to anything other than the configured translation_target_language, the defaults PATCH above still succeeds because it only updates model/effort, but this line promotes that requested language to the in-memory default before the translation POST runs. The server then rejects the POST via its requestedLangCode !== settings.translationTargetLanguage check, yet closing and reopening the popover resets to the unpersisted failed language instead of the DB-backed target until a later settings revalidation overwrites it.
Useful? React with 👍 / 👎.
Summary
Verification
Notes: build emits the existing Node DEP0155 warning from defuddle -> temml package exports. Full Vitest output includes expected backup-failsafe fixture diagnostics.
Summary by CodeRabbit
New Features
Documentation