Feat/sandhi expansion

[GJRobert]

No description provided.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[claude]

Review: Sandhi Expansion (#233)

Nice piece of work — generalizing the Dapu-only tone sandhi engine (getDapuSandhiHtml) into a rule-driven, data-configured engine (getSandhiHtml + sandhi_rules.json + classifyTone) that now covers all five major dialects (大埔/海陸/四縣/詔安/饒平) is a solid architectural improvement. Moving from regex-per-tone-color matching to a declarative tone-transition table is much more maintainable and testable. The new tests/sandhi_fixture_test.js with dialect-specific fixtures (including a bracket-guard case for 饒平 and a no-sandhi nasal case for 詔安) is a good sign — several fixtures were traced by hand against classifyTone/sandhi_rules.json and they check out.

Code quality / maintainability

• Dialect-code mapping is duplicated ~5 times. The dialect name → dialectCode if/else chain ('大埔'→'da', '海陸'→'ha', …) is re-implemented independently in getSandhiPronunciation (main.js:1057), three inline call sites in main.js (~2940, ~4409, ~6145), and again in formatGamePinyinWithSandhi (js/game/game-ui.js:362), each with slightly different source fields (selectedDialect, dialectInfo.腔, gameActiveDataVarName/gameActiveDialect prefix matching). A shared helper (e.g. getDialectCodeFromName(str)) would remove this duplication and prevent the five copies from silently drifting apart the next time a dialect is added or renamed.
• Stale guard in classifyTone (main.js:871): it still requires window.reverseToneMappingData.vowel_map to exist, but the comment right above explains vowel_map character-matching was deliberately abandoned in favor of NFD tone-mark extraction. vowel_map is otherwise unused in this function now, so the guard is misleading dead-code-adjacent logic — worth dropping or updating the comment to explain why it's still checked.
• applySandhiToGenerated (renamed from applyDapuSandhiToGenerated) is hardcoded to 'da' with a comment explaining it's legacy-specific — reasonable as a deliberate scoping choice, but slightly odd next to a function whose new name no longer signals that limitation. Consider keeping Dapu in the name or passing the dialect in, since the generic name reads as if it now supports all dialects like its sibling getSandhiHtml.
• Minor: index.html bumps game-ui.js to ?v=4.6.11 even though (per the diff) only one functional edit landed in that file — not wrong, just flagging in case it was meant to track something between 4.6.9 and 4.6.11 that didn't make it into this PR.

Potential bugs / correctness

• classifyTone's diacritic normalization uses .replace('ˊ', '́') etc. with plain-string (non-global) String.replace, which only replaces the first match. This is fine as long as a syllable only ever carries a single tone mark (true for the sample data), but it's a latent trap if this function is ever reused on multi-syllable strings without pre-splitting — worth a comment noting the single-tone-mark assumption, or switch to a /g regex to be safe.
• The new bracket-skip guard (inBracket && dialectCode === 'rh', main.js:997) only suppresses sandhi inside 【...】 for the 饒平 dialect. If this bracket convention (e.g., marking a repeated/quoted phrase) applies to source data for other dialects too, they'd still get (possibly incorrect) sandhi applied inside brackets. If this is intentionally 饒平-specific per the source dictionary's formatting conventions, a one-line comment explaining why it's scoped that way would help — otherwise it reads like an oversight.

Search feature (looksLikePhoneticRegex)

• Building a RegExp directly from raw user input (new RegExp(keyword, 'i')) is wrapped in try/catch, which correctly avoids a hard error on invalid patterns — good defensive touch.
• Since this only runs against the user's own locally-loaded combinedData in their own browser (triggered on Enter/radio-change, not per-keystroke — confirmed there's no input-event wiring driving it), there's no cross-user security exposure. The only residual risk is a user pasting a pathological catastrophic-backtracking pattern and locking up their own tab; not worth guarding against given the trigger is explicit (Enter key), but flagging for awareness.

Test coverage

• tests/sandhi_fixture_test.js is a nice addition, and its manual Node harness (mocking document/window) is appropriately lightweight given there's no existing test runner/package.json in the repo. Consider wiring it into CI (even a minimal GitHub Actions step running node tests/sandhi_fixture_test.js and checking the exit code) so regressions in sandhi_rules.json or classifyTone get caught automatically instead of relying on manual runs.
• Fixture coverage is good breadth-wise (one per dialect plus a couple of edge cases), but there's no fixture for the newly-added looksLikePhoneticRegex search path or for the bracket-guard interacting with a non-饒平 dialect — small gaps, not blocking.

Data/versioning

• data_version.json, style.css, and the script query-string version bumps look consistent with the project's existing cache-busting convention.

Overall this is a well-structured refactor that meaningfully expands functionality while improving the underlying design (data-driven rules > hardcoded regex-per-dialect). The main actionable item is de-duplicating the dialect-code mapping logic; everything else here is a minor polish suggestion.

[cloudflare-workers-and-pages]

Deploying hakspring with    Cloudflare Pages

Latest commit:	7d47fad
Status:	✅  Deploy successful!
Preview URL:	https://c883852c.hakspring.pages.dev
Branch Preview URL:	https://feat-sandhi-expansion.hakspring.pages.dev

View logs

[claude]

Review: Feat/sandhi expansion (#233)

Nice piece of work — generalizing the Dapu-only tone-sandhi engine into a data-driven, multi-dialect (da/ha/si/zh/rh) system is a big improvement over the old hardcoded regex/if-chain, and shipping a CI-gated fixture test (tests/sandhi_fixture_test.js + .github/workflows/sandhi-test.yml) for a project that previously had almost no automated test coverage is a genuine quality upgrade. A few things worth a look before/after merge:

Correctness

• typeof window !== 'undefined' guard doesn't actually distinguish "browser" from "Node test run" (main.js:6532-6543). tests/sandhi_fixture_test.js does global.window = {...} before require('../main.js'), so inside main.js window is defined and initializeApp() still gets invoked during the test run. initializeApp() immediately touches history (undeclared in the test stub) and DOM APIs the stub doesn't implement, so it will throw/reject. It currently doesn't fail the test suite only because the throw happens inside an async function (so it becomes an unhandled rejection) and the test script runs entirely synchronously to process.exit() before Node gets a chance to report it — that's incidental, not by design, and could start failing CI on a future Node version that reports/exits on unhandled rejections more eagerly, or the moment someone adds an await earlier in the test file. Consider guarding on something Node-specific instead (e.g. typeof module === 'undefined' || !module.exports), or explicitly skip via an env flag rather than relying on window being absent.

• sandhi_rules.json "24v" tone code for 詔安 (zh) has no direct fixture coverage. reverse_tone_mapping.json adds "á": "24v" and sandhi_rules.json adds a "24v": { "always": "55" } rule, but every zh fixture that could exercise "24v" as the front syllable (e.g. a bare sá, no stop coda) only appears in the next-syllable position of an "always" rule, where the next tone is never actually consulted. So the fixture suite currently can't catch a regression that swaps or drops the 24 vs 24v distinction. Worth adding one case with sá (no coda) as the leading syllable under zh.

Code quality

• applySandhiToGenerated() (renamed from applyDapuSandhiToGenerated, main.js:1101) is still hardcoded to 'da' and, as far as I can tell from a repo-wide grep, has no call sites at all (before or after this PR). Since this PR already did the work to generalize every other call site via getDialectCode, it'd be worth either wiring this one up too or removing it if it's genuinely dead — carrying forward dead single-dialect code in a PR whose whole point is killing single-dialect hardcoding is a little ironic.
• classifyTone's existence check reads window.reverseToneMappingData.vowel_map purely as a "data loaded" sentinel even though the function no longer uses vowel_map for lookups — this is called out in a comment, which is good, but it's a slightly fragile invariant (if vowel_map is ever dropped from reverse_tone_mapping.json as truly dead data, this guard silently breaks). Consider checking for dialect_reverse_map presence only, since that's what's actually used.
• The new getDialectCode() consolidation (single lookup for dialect-string → engine code) is a solid fix for the exact kind of drift bug the comment describes (海陸 hardcoded to 'da', missing 饒平) — good catch and good fix.

Performance / Security

• looksLikePhoneticRegex compiles user search input directly via new RegExp(keyword, 'i') (main.js ~2765). This is client-side only against the user's own locally cached dictionary, so it's not a cross-user attack surface, but a pathological pattern (e.g. catastrophic-backtracking regex) could hang the user's own tab. Not a blocker, but might be worth a length cap or a simple complexity heuristic given this is exposed directly to free-text search input.
• No other security concerns — no new use of innerHTML with unsanitized external input beyond what already existed, and the new JSON data files are static/bundled, not user-controlled.

Style / conventions

• Cache-busting query strings in index.html were bumped correctly per AGENTS.md conventions (style.css, main.js, game-ui.js), and data/data_version.json was bumped to force an IndexedDB refresh so the new sandhi_rules.json/reverse_tone_mapping.json get (re)fetched — good, and correctly added to DATA_FILES_TO_CACHE in main.js too.
• Comments are thorough and explain non-obvious invariants well (e.g. the 【】 bracket-skip being 饒平-only, the single-syllable assumption in the NFD tone-extraction regex). This matches the "preserve/explain intent" convention in AGENTS.md.
• Minor: this is a user-facing feature (new dialects get sandhi coloring, plus a new phonetic regex search mode) but whatsnew.md wasn't updated, which previous entries in that file suggest is the project's normal practice for shipped features.

Test coverage

• Good addition overall: 15 fixtures spanning all 5 dialects, including an edge case for the bracket-skip behavior and a "no sandhi applies" case. As noted above, consider adding one more fixture to pin down the 24/24v distinction for 詔安 as a front tone.
• The new phonetic-regex search mode (looksLikePhoneticRegex) has no test coverage at all (there wasn't an existing test harness for search behavior, so this is consistent with the status quo, but flagging since it's new logic with edge cases — e.g. malformed regex, mixed Hakka-vowel + regex-metacharacter input).

Overall this is a well-structured, well-documented refactor with real test coverage added where there was none — nice work. The window-guard issue is the one thing I'd actually want addressed since it affects the reliability of the new CI job's failure signal; the rest are minor polish items.