Align Chinese output script with locale selection (zh-CN/zh-TW)#265
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 416733ace7
ℹ️ 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".
| void queueSetSettings(next) | ||
| .then(() => { | ||
| setPrefs(current => { | ||
| if (!current || syncSeqRef.current !== seq) return current; | ||
| if (current.chineseScriptPreference !== previousScript) return current; |
There was a problem hiding this comment.
This effect enqueues setSettings(next) from a snapshot and then conditionally mutates local state, but it does not account for updatePrefs calls that happen while the queued write is in flight. In that case, the queue can persist a newer object (e.g., another setting change) and this callback can still flip only chineseScriptPreference in memory based on an older previousScript, leaving UI state and backend settings diverged until a full refresh. The race is reachable whenever startup/script auto-sync overlaps with another settings save.
Useful? React with 👍 / 👎.
This change binds locale selection to a persisted chineseScriptPreference so zh-CN and zh-TW drive output script consistently. It injects script preference into LLM context for polish/translate/QA, removes ASR language-hint submission, and applies script conversion only in pure-ASR fallback paths. It also hardens preference persistence sequencing in HotkeySettingsContext to reduce write-order races. Constraint: Must keep output script behavior coupled to locale choice without adding a new toggle Constraint: ASR requests should not send language parameters Rejected: Add an independent output-script option | Conflicts with required locale-driven behavior Confidence: medium Scope-risk: moderate Reversibility: clean Directive: Keep zh-CN=>simplified and zh-TW=>traditional mapping consistent in both Settings entry points and backend consumers Tested: npm run -s build (openless-all/app) Tested: cargo test -q (openless-all/app/src-tauri) Not-tested: Live external provider E2E verification across multiple real ASR/LLM vendors Related: Open-Less#242 Co-authored-by: OmX <omx@oh-my-codex.dev>
The review highlighted a race where script auto-sync could persist an older prefs snapshot while concurrent settings updates were queued. This updates HotkeySettingsContext persistence to resolve the object at queue execution time from latestPrefsRef, then merge chineseScriptPreference into that latest shape for script sync writes. Constraint: Keep fix minimal and limited to HotkeySettingsContext persistence flow Rejected: Full settings-store refactor with transactional backend API | Out of scope for this review round Confidence: medium Scope-risk: narrow Reversibility: clean Directive: Any future auto-sync writes must merge against latest prefs before calling setSettings Tested: npm run -s build (openless-all/app) Not-tested: Full cargo test rerun (no Rust code changes in this patch) Related: Open-Less#265 Co-authored-by: OmX <omx@oh-my-codex.dev>
PR Reviewer Guide 🔍(Review updated until commit 7985476)Here are some key observations to aid the review process:
|
H-Chris233
force-pushed
the
feat/zh-tw-script-sync
branch
from
416733a to
2970451
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2970451f01
ℹ️ 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".
| const chineseScriptPreference = | ||
| resolved === 'zh-CN' ? 'simplified' : resolved === 'zh-TW' ? 'traditional' : 'auto'; | ||
| if (prefs.chineseScriptPreference === chineseScriptPreference) return; | ||
| await updatePrefs({ ...prefs, chineseScriptPreference }); |
There was a problem hiding this comment.
This writes a full UserPreferences object captured before await setLocalePreference(next), so any settings changes that complete during that await (e.g., provider/hotkey updates) can be overwritten when updatePrefs({ ...prefs, chineseScriptPreference }) runs. Because updatePrefs replaces the entire prefs object, this race can silently revert unrelated options; re-read current prefs at write time (or update only the script field) to avoid clobbering concurrent edits. The same pattern is also present in SettingsModal.
Useful? React with 👍 / 👎.
| // 仅在“纯 ASR 直出”场景做强制简繁收敛，避免误伤翻译/常规 LLM 输出： | ||
| // - mode=Raw：本来就不走润色 | ||
| // - polish_error.is_some()：润色/翻译失败后回退到原始 ASR 文本 | ||
| let polished = if !translation_active && (mode == PolishMode::Raw || polish_error.is_some()) { |
There was a problem hiding this comment.
When translation mode is active and the LLM call fails, translate_or_passthrough returns raw ASR text with polish_error.is_some(), but this guard skips conversion because of !translation_active. That leaves fallback output in the original script even when the user selected simplified/traditional, which contradicts the intended fallback normalization path and produces inconsistent output specifically on translation failures.
Useful? React with 👍 / 👎.
|
Persistent review updated to latest commit 2970451 |
Latest PR review flagged that translation-mode failures were bypassing chinese script normalization because of a translation_active guard. This narrows the condition to normalize only ASR-direct outputs, including translation fallback failures, while still avoiding normalization on successful translation/LLM outputs. Constraint: Keep successful translation and polished LLM outputs untouched Rejected: Remove all guards and always normalize final text | Would risk mutating successful non-ASR outputs Confidence: high Scope-risk: narrow Reversibility: clean Directive: Script normalization should run for any fallback-to-raw path regardless of translation mode Tested: cargo test -q (openless-all/app/src-tauri) Not-tested: Manual runtime translation failure simulation in desktop UI Related: Open-Less#265 Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Persistent review updated to latest commit 1adde82 |
Latest review flagged that language handlers captured prefs before awaiting setLocalePreference, then replayed stale snapshots via object spread. This adds functional update support to updatePrefs in HotkeySettingsContext and switches both language pickers to merge chineseScriptPreference against current prefs at write time. Constraint: Keep the fix minimal and avoid refactoring unrelated settings flows Rejected: Read prefs from IPC again before each write | Adds extra roundtrip and still duplicates merge logic Confidence: high Scope-risk: narrow Reversibility: clean Directive: Async settings handlers should pass functional updaters whenever they await before persisting Tested: npm run -s build (openless-all/app) Not-tested: cargo test -q (no Rust changes in this patch) Related: Open-Less#265 Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Persistent review updated to latest commit 7985476 |
1 participant
User description
Summary
Validation
Notes
PR Type
Enhancement, Bug fix
Description
Bind Chinese output script to locale selection (zh-CN→simplified, zh-TW→traditional)
Fix async settings persistence race in HotkeySettingsContext
Update language pickers (modal + Settings page) to sync script preference
Diagram Walkthrough
File Walkthrough
9 files
Pass ChineseScriptPreference to LLM validation callIntegrate script preference in polish/translate/QA and add OpenCCInclude script preference in LLM context premiseDefine ChineseScriptPreference enum and fieldUpdate script pref on language picker changeReturn resolved locale from setLocalePreferenceAdd chineseScriptPreference to mock settingsAdd chineseScriptPreference to UserPreferences typeSync script pref in LanguageSection picker1 files
Serialize writes and auto-sync script preference1 files
Add ferrous-opencc dependency