feat: implement i18n translation catalogs with English fallback for r… - #76
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an LRU-cached JSON i18n loader with English fallback, four language catalogs (en/de/es/fr), integrates localized strings into PDF/HTML report rendering via get_strings(ctx.report_language), and adds unit tests for German and unsupported-language fallback. Changesi18n Localization Framework
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/medcheck/pipeline/report.py (1)
23-23: ⚡ Quick winRemove unused
get_stringscall or clarify intent.The returned i18n dictionary is not stored or used. The JSON report structure maintains English keys (likely intentional for API stability), so this call serves no purpose. Either remove it or add a comment explaining why it's present (e.g., language validation).
♻️ Proposed fix
- get_strings(ctx.report_language) + # JSON keys remain in English for API stability; language recorded in metadata🤖 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 `@src/medcheck/pipeline/report.py` at line 23, The standalone call get_strings(ctx.report_language) is unused; either remove it or make its intent explicit: either delete the call, or replace it by assigning to a variable (e.g., strings = get_strings(ctx.report_language)) and use it or perform an explicit validation (e.g., assert or try/except to ensure ctx.report_language is valid) and add a one-line comment explaining that this call validates/loads i18n for report_language while leaving report keys in English; reference the get_strings function and ctx.report_language when making the change.
🤖 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 `@src/medcheck/pipeline/report.py`:
- Around line 241-244: The i18n lookup for "field_key" in report.py (where
lang_check is computed using i18n.get("field_key", "Field") and then escaped
into th_field) always returns the default because "field_key" is missing from
all translation catalogs; either add "field_key" with appropriate translations
to every catalog (en.json, de.json, es.json, fr.json) so i18n.get(...) can
return a localized label, or simplify the code by removing the unnecessary
i18n.get and hardcoding "Field" for th_field (and remove the ctx.report_language
conditional) if the header is intentionally not localized; update the logic
around lang_check/th_field accordingly so it reflects the chosen approach.
In `@tests/unit/test_pipeline/test_report.py`:
- Around line 73-86: The test test_report_step_language_german incorrectly
checks for a non-existent German key; update the assertion to verify the report
language metadata instead: call ReportStep().run(ctx) as before, load the JSON
into data, and assert data["language"] == "de" (the generate_json_report
function sets the language metadata), removing the bogus "patienten_info" key
check.
---
Nitpick comments:
In `@src/medcheck/pipeline/report.py`:
- Line 23: The standalone call get_strings(ctx.report_language) is unused;
either remove it or make its intent explicit: either delete the call, or replace
it by assigning to a variable (e.g., strings = get_strings(ctx.report_language))
and use it or perform an explicit validation (e.g., assert or try/except to
ensure ctx.report_language is valid) and add a one-line comment explaining that
this call validates/loads i18n for report_language while leaving report keys in
English; reference the get_strings function and ctx.report_language when making
the change.
🪄 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: 179b01f6-56b8-4112-9e07-9b851319dba6
📒 Files selected for processing (8)
src/medcheck/i18n/__init__.pysrc/medcheck/i18n/de.jsonsrc/medcheck/i18n/en.jsonsrc/medcheck/i18n/es.jsonsrc/medcheck/i18n/fr.jsonsrc/medcheck/i18n/loader.pysrc/medcheck/pipeline/report.pytests/unit/test_pipeline/test_report.py
Liohtml
left a comment
There was a problem hiding this comment.
Thanks for this, @LawdeWayde — really nice work. 🙌 The architecture is exactly what #16 called for: a clean i18n/ package, per-language JSON catalogs, an lru_cache'd loader with English fallback, and the report renderers reading from ctx.report_language. I pulled the branch, ran it, and confirmed lang=de produces a German PDF/HTML report and an unsupported language falls back to English — and ruff, ruff format, and mypy all pass on the new code. Good, idiomatic stuff.
I did a thorough review (pulled the branch and executed it, not just read the diff). A few things should be addressed before merge — one is a real bug CodeRabbit didn't catch:
🔴 1. Dead get_strings() call in generate_json_report (must fix)
def generate_json_report(ctx: PipelineContext) -> str:
get_strings(ctx.report_language) # ← result discarded, no effectThis call does nothing — the JSON report is intentionally all-English keys (correct! machine-readable schema shouldn't be translated), so the line should just be removed. As-is it's dead code that misleads the reader into thinking the JSON is localized.
🔴 2. The two new tests don't actually test translation (must fix)
Both test_report_step_language_german and test_report_step_language_fallback assert against the JSON report — which has no translated strings. I verified that test_report_step_language_german passes even though zero German text appears in its output (it only checks data["language"] == "de", which was already true before this PR). So the feature could fully regress and these tests would stay green.
Please point them at the HTML or PDF renderer instead, e.g.:
def test_html_report_german(tmp_path):
ctx = _make_ctx(tmp_path); ctx.report_language = "de"; ctx.report_format = "html"
path = ReportStep().run(ctx).report_path
assert "Patienteninformationen" in Path(path).read_text(encoding="utf-8")
def test_html_report_unknown_language_falls_back_to_english(tmp_path):
ctx = _make_ctx(tmp_path); ctx.report_language = "xyz"; ctx.report_format = "html"
assert "Patient Information" in Path(ReportStep().run(ctx).report_path).read_text(encoding="utf-8")🟠 3. <html lang="..."> emits invalid values on fallback (should fix)
In generate_html_report, the content correctly falls back to English for an unknown language, but the document still emits <html lang="{ctx.report_language}"> — so an unsupported code produces <html lang="xyz"> (verified) with English text inside. That's invalid/misleading markup. Suggest deriving the lang attribute from what was actually resolved — e.g. have get_strings also return the resolved language, or set lang="en" whenever the requested catalog wasn't found.
🟡 4. Minor / nits
field_keyis now consistent — good, you added it to all four catalogs, which resolves CodeRabbit's earlier note. But thei18n.get("field_key", "Field")inreport.pyis now redundant; a plaini18n["field_key"]is clearer and consistent with how every other key is accessed.- No direct test for the loader itself.
loader.py(the actual new logic — merge/fallback/corrupt-file handling) is never unit-tested; it's only exercised indirectly. A smalltest_i18n.pycoveringget_strings("de"),get_strings("xyz")→ English, andget_strings(None)would lock in the behavior and protect the catalogs from silent breakage. - Missing newline at EOF on all five JSON files (
\ No newline at end of file). Trivial, but worth fixing for cleanliness. - The PR description is the unfilled template (empty Summary,
Fixes #). Please set it toFixes #16so the issue auto-closes on merge.
Note: rebase needed
main moved since you branched (PR #75 just merged), and it also touched report.py indirectly via the LLM-finding path. Please rebase on latest main and confirm CI is green — I don't see check runs on this PR yet.
None of this is structural — the design is right and most of it is small. Fix #1–#3 (and ideally add the loader test) and I think this is mergeable. Really solid first contribution; thank you for picking this up. 🚀
Generated by Claude Code
4a9de6f to
109e4b6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_pipeline/test_report.py (1)
83-84: ⚡ Quick winUpdate or remove outdated comments.
The comments reference checking "structural or dictionary labels" and adjusting key/value checks, but the actual assertion on line 86 simply verifies the language metadata field. These comments appear to be remnants from the previous implementation (before the fix noted in past reviews) and no longer match the current test logic.
♻️ Proposed fix
- # Check that structural or dictionary labels are translated to German - # Note: Adjust the exact key/value check below based on what your de.json looks like! # Verify that the report config correctly registers the target language metadata assert data["language"] == "de"🤖 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 `@tests/unit/test_pipeline/test_report.py` around lines 83 - 84, Update the outdated test comments in tests/unit/test_pipeline/test_report.py so they accurately describe the current assertion: remove references to "structural or dictionary labels" and the note about adjusting key/value checks, and replace them with a short comment stating the test verifies the language metadata (e.g., that the metadata 'language' field is set to 'de') or simply delete the stale comment lines; ensure the comment near the assertion that checks the language metadata field matches the actual check performed.
🤖 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.
Nitpick comments:
In `@tests/unit/test_pipeline/test_report.py`:
- Around line 83-84: Update the outdated test comments in
tests/unit/test_pipeline/test_report.py so they accurately describe the current
assertion: remove references to "structural or dictionary labels" and the note
about adjusting key/value checks, and replace them with a short comment stating
the test verifies the language metadata (e.g., that the metadata 'language'
field is set to 'de') or simply delete the stale comment lines; ensure the
comment near the assertion that checks the language metadata field matches the
actual check performed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a17a38a-5133-4c32-adde-d95c7419f754
📒 Files selected for processing (8)
src/medcheck/i18n/__init__.pysrc/medcheck/i18n/de.jsonsrc/medcheck/i18n/en.jsonsrc/medcheck/i18n/es.jsonsrc/medcheck/i18n/fr.jsonsrc/medcheck/i18n/loader.pysrc/medcheck/pipeline/report.pytests/unit/test_pipeline/test_report.py
✅ Files skipped from review due to trivial changes (2)
- src/medcheck/i18n/fr.json
- src/medcheck/i18n/es.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/medcheck/i18n/en.json
- src/medcheck/i18n/loader.py
- src/medcheck/i18n/init.py
- src/medcheck/pipeline/report.py
fixed them |
Liohtml
left a comment
There was a problem hiding this comment.
Re-reviewed the updated branch (1530c41) — pulled it and ran it, didn't just read the diff. You nailed all three blocking points. Thank you for the thorough turnaround. 🎯
Confirmed fixed:
- ✅ #1 dead code — the no-op
get_strings()is gone fromgenerate_json_report; it's now only called where it's actually used (PDF + HTML). - ✅ #2 weak tests — both report tests now force
report_format = "html"and assert on real localized text (Patienteninformationen/ English fallback). They'd now actually fail if translation regressed. 👍 - ✅ #3
<html lang>— verified empirically: unsupported language →lang="en"with English content;de→lang="de". Consistent now. - ✅ Bonus — you added
tests/unit/test_i18n.pycovering the loader directly (de, unknown→en,None→en), andfield_keyis now a cleani18n["field_key"].ruff,ruff format,mypy, and the full report+i18n test set all pass locally for me.
Two minor, non-blocking nits (your call — I'm happy to approve once CI is green regardless):
a) resolved_lang is case-sensitive but the loader isn't — small consistency bug. The loader lowercases (get_strings("DE") → German), but in generate_html_report:
resolved_lang = ctx.report_language if ctx.report_language in ["en", "de", "fr", "es"] else "en"doesn't. I verified: report_language="DE" renders German content but emits <html lang="en"> — a mismatch. One-liner:
_lang = (ctx.report_language or "en").lower().strip()
resolved_lang = _lang if _lang in {"en", "de", "fr", "es"} else "en"Also avoids duplicating the supported-language list — ideally that set lives next to the catalogs in i18n/, but that's polish.
b) Dead or branch in the German test. assert "Befundbericht" in html_content or "Patienteninformationen" in html_content — "Befundbericht" exists in no catalog, so only the second term ever matches. Drop the first term so the assertion says what it means.
Neither blocks merge. Once CI is green I'm good to approve — mergeable_state currently shows unstable (checks pending/not reported), so let's let the run finish. Genuinely solid work for a first contribution. 🙌
(Heads-up: main has moved since you opened this — a couple of doc/release PRs merged — so a quick rebase before merge will keep the history clean, though I don't see conflicts with your files.)
Generated by Claude Code
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_i18n.py (1)
16-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing newline at EOF.
The file appears to be missing a newline character at the end. Most style guides and linters expect files to end with a newline character.
🤖 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 `@tests/unit/test_i18n.py` at line 16, tests/unit/test_i18n.py is missing a trailing newline at EOF; open that file (test_i18n.py), add a single newline character at the end of the file and save so the file ends with '\n' to satisfy linters and style checks.
🤖 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.
Outside diff comments:
In `@tests/unit/test_i18n.py`:
- Line 16: tests/unit/test_i18n.py is missing a trailing newline at EOF; open
that file (test_i18n.py), add a single newline character at the end of the file
and save so the file ends with '\n' to satisfy linters and style checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 497e4e96-0cf6-4955-8711-e975eb2c7fdd
📒 Files selected for processing (3)
src/medcheck/pipeline/report.pytests/unit/test_i18n.pytests/unit/test_pipeline/test_report.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/medcheck/pipeline/report.py
|
@Liohtml i just fixed the issues |
Liohtml
left a comment
There was a problem hiding this comment.
Approving — this is ready. 🎉
I pulled 4d7ff83 and ran it end-to-end. Both follow-up nits are fixed, verified empirically:
- ✅ Case-insensitive language resolution.
resolved_langnow does(ctx.report_language or "en").lower().strip()against a deduplicated{"en","de","fr","es"}set. I confirmedreport_language="DE"now renders German content and emits<html lang="de">— the mismatch is gone. - ✅ Dead test branch removed. The German test now asserts cleanly on
"Patienteninformationen".
Full local check on your branch: ruff ✅, ruff format ✅, mypy ✅, and all 8 i18n + report tests pass. The architecture is exactly right — clean i18n/ package, per-language JSON catalogs, cached loader with English fallback, renderers reading from ctx.report_language, and a direct loader test.
Thank you, @LawdeWayde — this is genuinely excellent work, and you turned around two rounds of detailed review quickly and precisely. This closes #16 and gives MedCheck German/French/Spanish reports out of the box. 🙌
If you enjoyed this, I'd love to see you take on more — a couple that build naturally on what you just learned about the report renderer:
- #56 — patient-friendly plain-language report mode (readability + glossary). Right in your wheelhouse now.
- #54 — structured report export (FHIR / DICOM SR), another renderer-adjacent feature.
- Or anything from the epic #51. Happy to help scope whichever interests you.
Merging now. Welcome aboard as a contributor — hope to see you in the next PR. 🚀
Generated by Claude Code
Summary
Implements comprehensive internationalization (i18n) support for pipeline reports, adding language catalog routing for PDF and HTML generation with fallback handling.
Fixes #16
Changes
Testing
uv run pytest)uv run pytest --cov-fail-under=85)uv run ruff check .)uv run mypy src/medcheck --strict)pre-commit run --all-files)Additional Notes
Summary by CodeRabbit
New Features
Tests