feat: add language-aware detail view#5
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: d6263b1709
ℹ️ 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".
|
|
||
| function zh(text: string, language: DisplayLanguage): string { | ||
| if (language !== "zh-Hans") return text; | ||
| return Array.from(text, (ch) => SIMPLIFIED_CHARS[ch] ?? ch).join(""); |
There was a problem hiding this comment.
In zh-Hans mode this fallback leaves any character missing from SIMPLIFIED_CHARS unchanged, but the source hexagram text contains common traditional characters not in the table, e.g. 萬/雲 in packages/core/src/data/gua.ts:12 and 潛 in line 17. As a result, hexagram 1's Simplified detail view still renders mixed-script text such as 萬物, 雲行, and 潛龙; use a complete converter/table rather than a partial per-character map.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved. The naive local SIMPLIFIED_CHARS partial map was replaced by the audited core converter toSimplified from @iching/core (packages/core/src/i18n/simplify.ts); detail-renderer.ts now calls it (line 50) rather than a per-character fallback. The exact characters cited convert correctly: 萬物→万物, 雲行→云行, 潛龍→潜龙. scripts/verify-language-surfaces.ts --simplified (AC-006) enforces "no naive partial map" via a full-corpus residue scan and passes.
Persist a display language preference with canonical values for simplified Chinese, traditional Chinese, and English. Expose the setting through the CLI config command and the terminal settings scene while keeping current text rendering unchanged.
Use the persisted display language when constructing hexagram detail scenes. Chinese modes now render Chinese commentary and line texts without English translations; English mode renders English sections and line translations without the Chinese commentary block. Wire the saved preference through interactive, journal, post-cast, and standalone dict navigation.
Make English the default display language and present language choices in EN, traditional Chinese, simplified Chinese order across config and terminal settings.
Full display-language pass over the app: a terminal message catalog, an audited Traditional→Simplified conversion in core, language threading through every scene, and a deterministic verifier oracle. Core: - packages/core/src/i18n/simplify.ts — audited 314-entry T→S table + toSimplified() (1:1 single-codepoint, idempotent, 乾 stays 乾 not 干); exported from @iching/core. Terminal: - i18n/messages.ts catalog + tr(language, key); MessageKey = keyof typeof MESSAGES, shape enforced via `satisfies Record<string, Message>`. - DisplayLanguage threaded via SceneContext + yarrow constructors; all scenes localized (home/cast/reveal/browse/detail/journal/intention/toss/settings/yarrow + footers); hexagram names converted for 简, pinyin invariant, no bilingual stacking. CLI: - dict command threads config.language into the browse router. Tooling / docs: - scripts/verify-language-surfaces.ts — 10-mode oracle (inventory/policy/glossary/ simplified/terminal/cli/core-data/consults/self-test/run-all), each proven red→green, incl. name-rendering, footer-leak, and map-integrity guards. - docs/language-glossary.md — source-of-truth term/exception policy. - gitignore parked corpus-acquisition residue (data-acquisition/, scripts/data-acquisition/). Verified in one repo state: typecheck + 513 tests + bundle + verifier run-all all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Independent accuracy pass (OpenCC t2s + per-hexagram ctext source diff + structural invariants) over the EN / 繁 / 简 corpus: - 中孚 九二: 鶴鳴 → 鳴鶴 (在陰) — transposition vs the received text quoted verbatim in the 繫辭 - 巽 九二/上九: 牀 → 床 — match the ctext source + hex 23 剝; resolves an internal orthographic inconsistency - simplify: drop 祐→佑 — 祐 (自天祐之, hex 14) has no official simplification; 示→亻 to 佑 is non-standard and degrades the divine-blessing sense - Ext-B retention: 纆/餗/繻 added to SIMPLIFIED_EXCEPTIONS — their only standard simplification is a tofu CJK Ext-B glyph (𬙊/𫗧/𦈡); kept Traditional so the mapped-or-excepted coverage invariant stays honest Structural invariants, UI-message simplification (every zhHans == OpenCC t2s), all verifier modes, typecheck, and 517 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restore the 祐→佑 simplification mapping that the prior commit removed. A cross-check (ctext.org + frontier second opinion) confirmed the 中孚 九二 鳴鶴 and 巽 牀→床 received-text fixes are correct, but found the 祐→佑 removal unwarranted: simplified 周易 editions conventionally print 自天佑之, and 祐/佑 are interchangeable for 上天保佑. Consistent with the table's other deliberate convention-over-OpenCC entries (咷→啕, 遯→遁). zh-Hans renders 自天佑之. Verifier modes, typecheck, and 517 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex — the item from your prior review (incomplete zh-Hans conversion in the detail renderer) is resolved on this branch, and the diff has changed substantially since that review (rebased onto main, plus a corpus-accuracy pass). Please re-review the updated diff (commit b36c603) for any remaining issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b36c603cb6
ℹ️ 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 VALID_KEYS = Object.keys(CONFIG_SCHEMA) as Array<keyof typeof CONFIG_SCHEMA>; | ||
|
|
||
| function isConfigKey(key: string): key is keyof typeof CONFIG_SCHEMA { | ||
| return key in CONFIG_SCHEMA; |
There was a problem hiding this comment.
For inputs such as iching config get toString or iching config set toString x, key in CONFIG_SCHEMA treats inherited Object.prototype names as valid config keys. That bypasses the intended unknown-key error; get can print a function and set will try to call a non-schema value and crash instead of reporting a validation error. Use an own-property check or the existing VALID_KEYS.includes pattern here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Implemented in commit 7935bf5. Replaced key in CONFIG_SCHEMA with Object.hasOwn(CONFIG_SCHEMA, key) so inherited Object.prototype names (toString/constructor/valueOf/proto) no longer pass the guard — config get toString and config set constructor x now report a clean "Unknown key" error instead of printing a function / crashing with "schema.set is not a function". Added regression tests for get/set on inherited names.
`isConfigKey` used `key in CONFIG_SCHEMA`, which walks the prototype chain —
so `config get toString` printed the inherited function and `config set
constructor x` crashed ("schema.set is not a function") instead of reporting
an unknown-key error. Switch the guard to Object.hasOwn (own-property check)
and add regression tests for get/set on inherited names.
Addresses Codex P2 review feedback on PR #5.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7935bf5a59
ℹ️ 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".
|
|
||
| // Title | ||
| const title = "Settings"; | ||
| const lang = this.values.language; |
There was a problem hiding this comment.
When a user moves the Language row with ←/→, getValues() reflects the new selection, but render() still localizes labels from this.values.language, which is only refreshed when Escape saves. In that settings-session context the title, labels, and footer remain in the old language until leaving and re-entering settings, so the user gets no immediate confirmation that the display language changed; derive lang from getValues().language or update this.values on option changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Implemented in commit 97ff85d. render() now localizes from getValues().language (the live selection) instead of this.values.language (the saved snapshot), so moving the Language row with ←/→ re-localizes the title/labels/footer immediately — matching how Theme already previews live via onOptionChanged. Added a live-switch regression test in scene-language.test.ts.
Moving the Language row with ←/→ left the settings scene's own title, labels, and footer in the previously-saved language until Escape + reopen, because render() localized from this.values.language (the saved snapshot, refreshed only on save) instead of the live selection. Theme already previews live via onOptionChanged; language now matches — render() reads getValues().language. Adds a live-switch regression test. Addresses Codex P3 review feedback on PR #5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97ff85d05d
ℹ️ 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 kwStr = String(kw).padStart(3, " "); | ||
| const chinese = hex.n; | ||
| // Convert the hexagram name for zh-Hans; drop the English name in Chinese modes. | ||
| const chinese = language === "zh-Hans" ? toSimplified(hex.n) : hex.n; |
There was a problem hiding this comment.
When the dictionary is rendered in zh-Hans, rows now show simplified names such as 兑, but the search path still goes through BrowseModel.setQuery() → searchHexagrams(), which only compares the query with the Traditional gua.n. In that mode, users can see and type the displayed simplified name but get zero matches (e.g. searchHexagrams("兑") returns none while "兌" works), so the localized dictionary search is broken for simplified Chinese names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Implemented in commit 97f6a87. searchHexagrams now also matches toSimplified(gua.n), so the simplified name shown in zh-Hans mode resolves — searchHexagrams('兑') and '观' now return 兌/觀, while Traditional queries still work. Regression tests added.
| const beats: Step[] = buildYarrowRoundBeats( | ||
| this.model, this.timing, this.detail, lineIdx, roundIdx, round, | ||
| { narrating: true, revealCut: false }, | ||
| { narrating: true, revealCut: false, language: this.language }, |
There was a problem hiding this comment.
This localizes the round-beat captions, but on the third round the immediately following buildYarrowFuseBeat(...) call still omits language, so it defaults to English. In zh-Hant/zh-Hans manual yarrow mode, the final fuse caption for each line still shows English text like Remaining ... old yin even though the rest of the manual ritual captions are localized.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Implemented in commit 97f6a87. The manual-mode line fuse beat now forwards language: this.language to buildYarrowFuseBeat (matching the round-beats call), so the final fuse caption localizes (餘策 … 老陰) instead of defaulting to English. Regression test added (drives line 0 in zh-Hant and asserts no English fuse-caption leak).
…use caption Two localization gaps from Codex review of PR #5: - search (P2): searchHexagrams matched the query only against the Traditional gua.n, so in zh-Hans mode — where the dictionary displays simplified names — typing the displayed name (e.g. 兑) returned zero matches. Now also matches toSimplified(gua.n); 兌/兑 and 觀/观 both resolve. - yarrow (P3): the manual-mode line fuse beat omitted `language`, so the final fuse caption defaulted to English ("Remaining … old yin") while the round beats were localized. Forward this.language, matching the round-beats call. Regression tests added for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97f6a87378
ℹ️ 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".
| } else { | ||
| lines = [gua.p, structLine]; |
There was a problem hiding this comment.
When language === "zh-Hans" and a glyph is active, this branch omits the text name because the glyph is treated as the name, but the reveal timeline still composes that glyph from the Traditional GUA[n].n. In the normal interactive flow glyphConfig is always passed, so casts such as KW58 show a large Traditional 兌 while the rest of the reveal is Simplified; either compose the glyph from the Simplified name or keep a Simplified text name visible in this path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Implemented in commit a4eb62d. The glyph atlas now includes Simplified name glyphs, and the cast reveal composes from the Simplified name in zh-Hans (via CastGlyphConfig.language → buildGlyphReveal), so KW58 shows a Simplified 兑 glyph matching the rest of the reveal. Regression test added.
| this.model.contentHeight = buildContentLines(this.model, ctx.cols, { | ||
| language: this.language, | ||
| }).length; |
There was a problem hiding this comment.
Passing this.language here only localizes the text content; the glyph rows created just above are still composed from this.model.detail.gua.n, so Simplified dictionary detail views reached from the interactive browser render a Traditional large glyph (for example 兌) above a Simplified header (兑 Duì). Because main.ts supplies glyphConfig for dictionary detail scenes, this affects the default zh-Hans TUI path for any name with a Simplified variant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Implemented in commit a4eb62d. DetailScene.enter now composes the glyph from toSimplified(gua.n) when language is zh-Hans, so the large glyph matches the Simplified header (兑 above 兑 Duì). The 25 needed Simplified glyphs were added to large-glyphs.ts. Regression test added.
The large braille glyph was composed from the Traditional name even in zh-Hans, so a Traditional 兌 rendered above Simplified 兑 text in cast reveal and dictionary detail (Codex P2 ×2). - generate-glyphs.ts now also rasterizes the Simplified form of each name; the atlas gains 25 chars (讼师谦随蛊临观贲剥复颐过离恒遁壮晋损渐归丰兑涣节济). Regen is deterministic — the 71 existing Traditional glyphs are byte-identical. - cast reveal composes from the Simplified name via CastGlyphConfig.language (threaded through CastScene opts); dictionary detail composes from this.language. Glyph sizing is unchanged (same char count). Regression tests assert the zh-Hans glyph differs from zh-Hant for KW58 (兑/兌) in both the detail and cast paths. Addresses Codex P2 review feedback on PR #5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
On first boot (no config file), detect the display language from the POSIX locale environment and persist it, so a non-English user is greeted in their own language. It is a one-time SEED, not a live binding: once the config exists the saved choice is frozen and the locale is never consulted again — a later move to an English shell won't flip it. - detectSystemLanguage(): LC_ALL > LC_MESSAGES > LANG > LANGUAGE; maps zh_CN/zh_SG/zh_MY + bare zh → 简, zh_TW/zh_HK/zh_MO → 繁, script subtags (zh-Hant-*) win over region; anything non-Chinese (incl. C/POSIX/unset) → en. - loadOrSeed(): seeds + persists on ENOENT; wired into interactive TUI startup. load() stays a pure read so config subcommands and test fixtures never write. Tests: detectSystemLanguage mapping + precedence table; loadOrSeed seeds, persists, and stays frozen across a locale change; load() stays pure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Self-review gate (5-lens, adversarially verified) on the first-boot language seed surfaced four issues; all fixed: - MUST: first-boot save() was unguarded, so a read-only / full data dir crashed the TUI at startup (regression — the old load() never wrote). loadOrSeed now best-effort-persists: a failed seed-write degrades to the detected language in memory for the session and freezes on a later writable launch. - MUST: a corrupt / empty config.json threw a SyntaxError that crashed the TUI AND the recovery CLI (config get/set couldn't self-heal). load()/loadOrSeed now share readExisting(), which tolerates an unparseable file → defaults (non-destructive: a corrupt file is not clobbered) while still rethrowing genuine I/O faults (EACCES/EIO) so they stay visible. - SHOULD: GNU LANGUAGE is a colon-separated priority list; the parser didn't split on ":", misrouting e.g. LANGUAGE=zh_TW:en (繁) to 简. Split on [.@:]. - SHOULD: the `set language` round-trip only exercised "en" (the default), so a no-op write would pass green — now round-trips zh-Hant. Tests added: corrupt-config degradation (load + loadOrSeed, non-clobber), seed-write-failure degradation (stubbed save), LANGUAGE colon-list mapping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0742a835a5
ℹ️ 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".
| export function detectSystemLanguage( | ||
| env: Record<string, string | undefined> = process.env, | ||
| ): UserConfig["language"] { | ||
| const raw = env.LC_ALL || env.LC_MESSAGES || env.LANG || env.LANGUAGE || ""; |
There was a problem hiding this comment.
For users with LANG=en_US.UTF-8 and LANGUAGE=zh_TW:en (the GNU gettext priority-list form for message language), this line selects LANG before ever looking at LANGUAGE, so first boot seeds and persists en even though the explicit display-language preference is Chinese. Because the seed is frozen into config, the user has to discover and change it manually; prefer honoring a present LANGUAGE priority list before falling back to LANG when localization is enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit f7fa23f. Detection now follows gettext display-language precedence: the effective locale is LC_ALL>LC_MESSAGES>LANG, but a present LANGUAGE (colon priority list) outranks it when localization is on, and is ignored in the C/POSIX locale. Verified end-to-end — the real TUI boots Traditional under LANG=en_US + LANGUAGE=zh_TW:en. Tests updated.
| } catch { | ||
| /* deferred: not writable this session */ | ||
| } |
There was a problem hiding this comment.
When the first-boot seed cannot be written because the config directory is read-only/full, this swallowed error leaves no config file on disk; if the user opens Settings in the same session, main.ts reloads with configStore.load() and initializes from defaults, so a detected Chinese session flips back to English in Settings. To keep the promised “run with the detected language in memory this session” behavior, the interactive caller needs to retain/use the seeded config after a deferred write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit f7fa23f. The store now retains an unpersisted first-boot seed in memory (cleared once a real config file exists), so a same-session configStore.load() — e.g. opening Settings — keeps the detected language instead of flipping to defaults. Regression test added.
…P2 ×2) - LANGUAGE precedence: GNU gettext treats LANGUAGE (a colon priority list) as the *display/message* language selector that outranks LANG/LC_MESSAGES/LC_ALL when a real locale is active (and is ignored in the C/POSIX locale). The seed previously read LANGUAGE last, so a bilingual user (LANG=en_US, LANGUAGE=zh_TW:en) was wrongly seeded English. Detection now derives the effective locale from LC_ALL>LC_MESSAGES>LANG, applies the C-locale guard, then lets a present LANGUAGE's first entry win. Verified end-to-end: the real TUI now boots Traditional under LANG=en_US LANGUAGE=zh_TW:en. - Deferred seed: when first-boot persist fails (read-only/full dir), the swallowed error left no file, so a later configStore.load() in the same session (opening Settings) flipped back to defaults. The store now retains the unpersisted seed in memory (shadowed once a real file exists) so the session stays in its detected language. Tests: corrected LANGUAGE-precedence + C-locale-guard cases; deferred-seed in-memory retention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7fa23f0e7
ℹ️ 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 raw = env.LANGUAGE || locale; | ||
| const parts = raw.split(/[.@:]/)[0].replace(/_/g, "-").toLowerCase().split("-"); | ||
| if (parts[0] !== "zh") return "en"; |
There was a problem hiding this comment.
When a localized shell has LANGUAGE set to a priority list whose first entry is unsupported by this app, e.g. LANG=en_US.UTF-8 LANGUAGE=ja:zh_CN, this only parses the first token and returns en, so first boot persists English even though the user provided a supported Chinese fallback. GNU LANGUAGE is a priority/fallback list, so this should scan entries until one maps to en/zh-Hant/zh-Hans before falling back to the locale/default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit 2a457d2. detectSystemLanguage now walks the LANGUAGE priority list in order (then the locale) and honors the first entry that maps to a supported display language, so LANG=en_US LANGUAGE=ja:zh_CN seeds zh-Hans. Verified end-to-end — the real TUI boots Simplified for that env. Fallback-scan tests added.
…dex P2) GNU LANGUAGE is a colon-separated fallback list tried in order; the seed only parsed the first entry, so LANG=en_US LANGUAGE=ja:zh_CN seeded English even though the user listed a supported Chinese fallback. Detection now scans LANGUAGE entries (then the locale) and honors the first that maps to a supported display language (en / zh-Hant / zh-Hans), via a mapLocaleToken helper. All prior precedence/C-locale behavior is preserved. Verified end-to-end: the real TUI boots Simplified under LANG=en_US LANGUAGE=ja:zh_CN. Tests added for the fallback-scan cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a457d2cd9
ℹ️ 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".
| // snapshot (this.values, refreshed only on Escape) — so moving the Language | ||
| // row with ←/→ re-localizes the scene immediately, matching how Theme already | ||
| // previews live via onOptionChanged. | ||
| const lang = this.getValues().language; |
There was a problem hiding this comment.
When Settings is localized and the user focuses Cast Method/Cast Mode with yarrow selected, the preview pane still renders yarrow captions in English: renderPreview() derives vals.language but calls renderYarrowFieldStrip(...) without it, and that helper defaults its language parameter to "en". This leaks labels like “stalks”/“set aside” inside the Chinese Settings screen even though the rest of the scene has switched language.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit 24de4aa. renderPreview now passes vals.language to renderYarrowFieldStrip in both yarrow preview branches, so the Settings preview captions localize (策/奇策) instead of defaulting to English. Regression test added.
| const i = argv.indexOf(name); | ||
| return i >= 0 ? argv[i + 1] : undefined; | ||
| }; | ||
| const INVENTORY_REL = optValue("--inventory") ?? ".loop/language/TEXT_SURFACES.md"; |
There was a problem hiding this comment.
The new verifier defaults to .loop/language/TEXT_SURFACES.md, but those .loop/language files are not present in this checkout (rg only finds references), so bun scripts/verify-language-surfaces.ts currently fails immediately with missing inventory/CONSULTS errors unless the caller has private local files or knows to pass an override. Since the script and glossary advertise this as the deterministic language-surface check, it needs committed default inputs or a default path that exists in the repo.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit 50bf731. The required inventory + consults inputs are now committed under tests/fixtures/language/ and the verifier's defaults point there, so it runs reproducibly from a fresh clone — verified: all modes pass with .loop/ removed. (No npm impact; the package ships only the bundled iching.js + README + LICENSE.)
renderPreview() called renderYarrowFieldStrip without the selected language, so the yarrow preview strip in a localized Settings screen rendered its captions (策 / 奇策 / 餘策) in English. Pass vals.language through in both the auto and manual yarrow preview branches. Regression test drives the Settings yarrow preview in zh-Hant and asserts no English caption leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(Codex P2) verify-language-surfaces.ts defaulted its inventory/consults inputs to .loop/language/*, which is gitignored — so the committed, glossary-advertised "deterministic check" hard-failed on a fresh clone / CI. Commit the two required inputs (TEXT_SURFACES.md, CONSULTS.md) under tests/fixtures/language/ and point the verifier's defaults there. (No npm impact — the package ships only the bundled iching.js + README + LICENSE.) Proven reproducible: every mode passes with .loop/ removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50bf7318cf
ℹ️ 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 paths = resolvePaths( | ||
| globalOpts.dataDir ? { dataDir: globalOpts.dataDir } : undefined, | ||
| ); | ||
| const config = await new JsonConfigStore(paths.config).load(); |
There was a problem hiding this comment.
When iching dict is the user's first interactive entry point (or it is run with a fresh --data-dir) under a non-English locale, this pure load() returns the default en and does not persist the detected locale, so the router on line 45 launches both browse and detail in English until the user separately starts the main TUI. The main interactive path uses loadOrSeed() for this first-boot locale freeze, so the standalone dictionary should use the same seeding path rather than the config-subcommand pure read path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit 672cffb. iching dict now uses loadOrSeed() (the main-TUI first-boot path) instead of a pure load(), so it seeds + freezes the detected language. dict is the only interactive standalone entry that loads config (cast/journal are one-shot prints). Verified end-to-end — iching dict boots Traditional under zh_TW and seeds zh-Hant; subprocess regression test added.
…ctionary `iching dict` is an interactive entry point that threads config.language into the Browse/Detail scenes, but it used a pure load() — so a first run via `iching dict` (or a fresh --data-dir) under a non-English locale launched in English and never seeded/froze the choice. Switch to loadOrSeed(), matching the main TUI's first-boot path. (config get/set/list stay pure load() — they are non-interactive and must not write.) dict is the only interactive standalone entry point that loads config; cast and journal are one-shot (outputJson/plain print). Verified end-to-end: `iching dict` boots Traditional under zh_TW and seeds zh-Hant. Subprocess regression added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
Addresses the multi-agent code review of PR #5 (2 P2 + 5 P3, all verified): - P2: alias/legacy lookups walked the prototype chain — `"language":"constructor"` loaded the Object function into the config. Guard LANGUAGE_ALIASES, THEME_ALIASES, and both LEGACY_CAST_MODE lookups with Object.hasOwn (mirrors isConfigKey). - P2: first-boot `config set <x>` persisted the defaulted language:"en", freezing the locale seed before the TUI ever ran. `set` now uses loadOrSeed() (get/list stay pure load()). - P3: language matching is now case-insensitive (BCP-47 / RFC 5646) — "zh-hans", "ZH-HANT", "En" resolve instead of falling to English. - P3: normalizeConfig carries through unknown own-keys (schema-keys: "schemas only expand") so a settings-save doesn't strip keys from a newer version. - P3: a corrupt config file is copied to config.json.corrupt (with a stderr warning) before falling back to defaults, so the next save can't silently clobber it. - P3: settings "save & back" no longer crashes on a read-only data dir — the save is best-effort and changes apply live for the session. - P3 (test hygiene): neutralize LANGUAGE/LC_MESSAGES in the locale-seed tests and the dict/config-set subprocess env so they don't fail on hosts where LANGUAGE is set. Regression tests added for each. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view) Resolves the ultrareview's gated design question + the layout finding (per the maintainer's decisions: seed upgraders; fix layout now). - Upgrade cohort: a config from a pre-language version (file exists but never had a `language` key) is now seeded from the locale on first interactive launch, preserving the user's other settings — so an upgrading zh user is greeted in Chinese instead of frozen to the "en" backfill. The read path is refactored (readRaw) to distinguish absent / no-language-key / has-a-choice / corrupt. - Settings layout: below ~h22 the 7th row (Cast Mode) was pushed under the bottom-anchored footer and edited blind. The scene now scrolls a window of settings so the focused row stays visible (with ↑/↓ hints); h>=22 still renders all 7 unchanged. - Classified the new read-only-save diagnostic in the committed language-surface inventory + policy matrix so the verifier stays green. Tests: upgrade-seed + no-reseed-when-language-present; settings scroll at h=20, no-scroll at h=24. Suite + verifier green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g the loader `config set language 繁` / `简` / `EN` / `zh-hant` was rejected even though the Settings UI displays exactly those labels — and the file-load path already accepts them (ultrareview P3#2), so the CLI and file paths diverged. Extract a shared `canonicalLanguage()` (used by both normalizeConfig and the CLI) so `config set language` accepts the same inputs and persists the canonical value (en/zh-Hant/zh-Hans). Genuinely invalid values are still rejected with the canonical list. Resolves ultrareview P3 #7 (the documented strict-CLI/lenient-file asymmetry is no longer worth keeping now the loader is lenient). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ate) /gate failure-mode lens: `config set`'s save() was unguarded, so a read-only / full data dir crashed it with a raw EACCES stack trace. A write command should still fail (it can't persist), but with a clear message — matching the TUI settings-save hardening. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex — Two further commits landed since your last review: 5131f4f (config set language now accepts the same UI labels/case-variants the loader accepts, via a shared canonicalLanguage helper) and edd87b8 (config set fails cleanly when the data dir is unwritable). Please review the updated diff for any remaining issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edd87b8251
ℹ️ 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".
| // language first — otherwise persisting the defaulted "en" would freeze | ||
| // the locale seed before the user ever launches the TUI. (get/list stay | ||
| // pure load() — they don't write.) | ||
| const cfg = await store.loadOrSeed(); |
There was a problem hiding this comment.
On first boot, config set now calls loadOrSeed() before checking whether the key/value is valid, and loadOrSeed() persists a new config file when none exists. This means a failing command such as iching --data-dir <empty> config set notARealKey x or config set language klingon can still create config.json and freeze the locale seed even though the user’s write was rejected. Move the key/value validation ahead of the first persistent load/seed, then call loadOrSeed() only for commands that will actually save.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit 5c0d07e. Key/value validation (including the canonicalLanguage normalization) now runs before any store access, and loadOrSeed() is only called once the set is known-valid — a rejected config set exits without creating or touching config.json. Regression test added: failed sets (invalid value + unknown key) on a fresh data dir leave no config file.
…thing Codex P2 on PR #5: `config set` ran loadOrSeed() before key/value validation, and loadOrSeed persists a seeded config when none exists — so a REJECTED command (`config set language klingon`, unknown keys) still created config.json and froze the locale seed on first boot. Validate first, then load/seed only for a set that will actually save. Regression test pins the no-side-effect contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c0d07ee15
ℹ️ 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".
| } catch { | ||
| // A write command must fail when it can't persist — but cleanly, not | ||
| // with a raw EACCES stack trace (cf. the TUI settings-save hardening). | ||
| console.error(`Couldn't write config to ${paths.config} (read-only or full data dir?).`); |
There was a problem hiding this comment.
When this write-failure message was added, it was not added to tests/fixtures/language/TEXT_SURFACES.md, so the committed language-surface verifier now fails: bun scripts/verify-language-surfaces.ts --inventory-only reports the fragments "Couldn't write config to" and "read-only or full data dir" as unclassified from this line. Any change that introduces new user-facing CLI text needs the inventory updated, otherwise the acceptance check for zero unclassified surfaces stays red.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[auto-triage] ✅ Resolved in commit a8773a8. Added cli-config-save-failed to the committed inventory (AC-001) and the policy matrix (AC-002) as a developer-only English diagnostic, mirroring cli-settings-save-failed. bun scripts/verify-language-surfaces.ts run-all and --inventory-only both PASS — zero unclassified surfaces.
…ex P2) The edd87b8 save guard added new user-facing CLI text without updating the committed surface inventory, so verify-language-surfaces failed AC-001 with two unclassified fragments. Add cli-config-save-failed to the inventory + policy matrix (developer-only English diagnostic, mirroring cli-settings-save-failed). Verifier run-all + --inventory-only both PASS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
* refactor(terminal): consolidate scroll math into one helper module Four views each rolled their own scroll logic (only journal used the ScrollableRegion widget). Extract widgets/scroll.ts with the shared primitives and route every view through it — behavior-preserving: - scroll.ts: clampOffset (free-scroll range), offsetToShow (cursor-into-view, stateful), windowFor (stateless visible window), pageIndicator. - browse-model: ensureCursorVisible → offsetToShow (was the same math inline). - settings-scene: visible-window calc → windowFor. - ScrollableRegion (journal): scroll/indicator now call clampOffset/pageIndicator; gains ensureVisible() for cursor lists. - detail-model / detail-renderer: clamp + page indicator → clampOffset/pageIndicator. detail keeps its own scroll fields rather than folding fully onto ScrollableRegion: its scroll state is intertwined with a rich model AND a pre-existing viewportHeight(20)-vs-renderer-visibleRows split, which a full fold would have to reconcile — a behavior change, not a pure refactor. Sharing the math is safe and captures the duplication; the full state-fold is a separate follow-up. Adds scroll.test.ts (the helper math). Suite + verifier green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(terminal): journal reuses ScrollableRegion.ensureVisible (refactor lens) /refactor sibling sweep on the scroll consolidation: journal-scene owned a ScrollableRegion but hand-rolled ensureCursorVisible() as a verbatim copy of offsetToShow — the exact math the region now exposes as ensureVisible(). Delegate to it. Behavior-identical (character-for-character same offset math); full suite green (572 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(terminal): delete dead commentary-scroll state in the cast scene commentaryScrollOffset was written at three sites (exploration-mode ↑/↓ handlers, setFocusedHex reset, constructor init) and read by nothing — no renderer ever consumed it, so the arrows mutated invisible state. Remove the field and all writers. Key routing is unchanged: in exploration mode ↑/↓ now fall through every remaining handler and do nothing, same as the user ever saw. Surfaced by the /refactor sibling sweep on the scroll consolidation; if commentary scrolling is wanted later, it should be built on widgets/scroll.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…log (#8) * docs(i18n): ratify option-chip display labels in glossary + policy matrix (U1) Token-keyed renderings for font/castMethod/castMode/taijitu/anim chips (zh-Hant + zh-Hans authored explicitly). Theme labels recorded as deferred pending brand-vs-literal ruling. Input-alias policy: none. New surface row term-settings-option-chips + matching policy-matrix entry. * feat(terminal): option-label resolver + identity-bound settings rows (U2) OPTION_LABELS Map catalog (ships empty — behavior-neutral) with total optionLabel() fallback to the canonical token; SettingRow now carries canonical values, chip labels derived per render via chipLabel(); getValues() maps field→row by key, killing the positional-index drift (reordering rows can no longer swap persisted values). Language row's endonym badges untouched. * feat(terminal): localize font chips — 楷體/隸變/黑體 (U3) First ratified wave of the option-label catalog. zh-Hant/zh-Hans authored per glossary; en keeps canonical pinyin tokens; persistence and live language flip locked by tests. * feat(terminal): localize cast/taijitu/anim chips — wave 2 (U4) 銅錢 (coin)/蓍草 (yarrow) carry the canonical token as an in-label hint (no transliteration bridge to the CLI value); 自動/手動, 點陣/密實, and 噪點/點陣/放射/沙化 are literal label-only renderings. Token-keyed per glossary (噪點 = noise, 點陣 = dots, ratified once for both usages). Width budget pinned under the en theme row. * test(verify): enforce option-label catalog coverage (U5) --terminal now asserts the full ratified (settingKey, token) → 繁/简 contract behaviorally via optionLabel(), pins en = canonical token, theme verbatim fallback (deferred), and prototype-chain misses; adds representative label sentinels wired to the inventory row. Deleting a shipped entry reds the verifier (proven by mutation). * refactor(terminal): rename pick() to selectedValue() (gate /naming) Name the role (find the row owning the key, narrow its selected token), not the category. * docs(plans): mark settings option-label plan completed
…e state Final-pass review (8 personas + cross-model codex) traced every finding to one seam: config error paths returned/persisted state diverging from what the app showed. Root fixes in the store (red-green: 7 failing tests written first): - save() keeps the config in memory on a failed write [codex X1], so a later load() (e.g. reopening Settings) returns the session's edits instead of silently reverting them; seededInMemory generalized to pendingInMemory. - loadOrSeed() treats a corrupt config as a re-seed [P1+P2e]: locale-seeded defaults are persisted (healing the file) once the original bytes are safely in .corrupt — never when the backup failed. The reset is announced once with recovery instructions, not silent at the next save. PIN FLIP: the old "no-clobber" test asserted the live file stayed corrupt; the recoverable copy is the backup, and the unhealed file was the root of P1/P2c/P2e. - normalizeConfig unknown-key passthrough uses Object.hasOwn + an explicit __proto__/constructor/prototype denylist [P2a, 3-reviewer convergence] — `in` silently dropped future keys shadowing prototype names, and the pollution defense was incidental. - Corrupt warning fires once per store instance and the .corrupt backup is written wx (first backup never clobbered) [P2c+P3a]. - atomicWriteJson unlinks the temp file on a failed write/rename [P2d]; parent-dir fsync gap documented [P3b]. Rejected P2f (re-seed on invalid stored language): persisting a locale re-seed over a value written by a future version destroys the stored choice on downgrade; coerce-in-memory-only is the conservative behavior. Documented at the gate. TEXT_SURFACES storage-errors row updated (it falsely claimed zero custom strings). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntry clamp (review #5/#6/#7) Three P3 findings from the independent review: - #5: `journal list` validated --limit unconditionally, but --all discards it — so `journal list --all --limit abc` exited 1 over a flag it ignores. Validate --limit only on the !--all path. - #6: the journal search input handled backspace / deleteWord / up-down but dropped the caret keys the note input honors — arrow left/right, forward delete, home, end. Mirror the note-input branches (left/right move the query caret; up/down still move the result cursor). - #7: enter() reseeded the list + patterns viewports but skipped the cursor re-clamp resize() runs, so popping back from a replayed reading into a shrunk terminal left the selection below the fold. Run ensureCursorVisible() + patternsScroll.scrollDown(0) in enter(), matching resize(). Tests: --all ignores a bad --limit (mutation-verified); the search input does caret-aware insert + home/forward-delete + end/backspace (mutation-verified); re-entering shrunk keeps the selection on screen (mutation-verified). Gates: 1341 tests pass, typecheck clean, language-surface PASS, smoke 5/5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Adds a persisted display language preference for English, traditional Chinese, and simplified Chinese, then applies that preference to the terminal hexagram detail view. This is intended to merge before the parked data-enrichment branch so future depth can be gated by language rather than stacked into one surface.
Key Changes
Configuration
DisplayLanguagewith canonical valuesen,zh-Hant, andzh-Hans.languageinUserConfig, defaulting toen.languagethroughiching config get/set/list, ordered asen,zh-Hant,zh-Hans.EN,繁, and简.Terminal Settings
EN,繁, and简.Detail View Policy
iching dict.Translation Scope
This PR is not a full translation/content pass. It establishes the preference, routing, and display policy; the existing English and Chinese source texts remain as-is except for conservative Simplified rendering of visible Chinese strings.
Testing
bun run typecheckbun test packages/storage/src/__tests__/config-store.test.ts apps/cli/src/__tests__/config-command.test.ts packages/terminal/src/__tests__/settings-scene.test.ts packages/terminal/src/__tests__/detail-renderer.test.ts packages/terminal/src/__tests__/detail-scene.test.ts->39 pass,0 fail,109 expect()callsbun test --path-ignore-patterns='**/dist/**'->489 pass,0 fail,221132 expect()callsgit diff --checkbun run bundle->publish/iching.jsat ~1.18 MBNotes
No Linear ticket detected. PR #4 (
feat/data-enrichment) has been converted back to draft and can rebase onto this once language merges.Closes #1