Update translation workflows - #15112
Conversation
🔵 Review postedLast updated: 2026-07-31 16:15 UTC |
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15112 — the crowdin.yml dest restructure is coherent and the cleanup script's discovery/prune split is the right shape (tests use real dirs under tmp_path, and the fixture deliberately uses fictional plugin names so discovery is tested rather than a hardcoded inventory). Four things need fixing before merge.
- blocking — the Android
values-prunermtrees any non-language resource qualifier (values-night,values-v21, …); inline. - blocking — the new test file compares mixed-separator paths and will fail the
windowstox job; inline. - blocking —
uv sync --all-packagescannot make the Makefile guard pass, because that guard runs barepython, not the uv venv; inline. - suggestion — discovery globs recurse into gitignored build output (
build/,dist/,tar/); crowdindestmoves orphan existing Crowdin files; no test for thedestinvariants the comments call load-bearing. All inline.
pytest test/test_i18n_cleanup_unsupported_languages.py passes locally (9 tests) on Linux. CI has no failures but several platform builds are still pending, and both i18n workflows are workflow_dispatch, so neither is exercised by CI. No UI files changed — Phase 3 does not apply.
.github/workflows/i18n-download.yml:40 — suggestion: still plain uv sync, and make i18n-download hits the same guard at Makefile:316 before create-message-files --plugins. Not in this diff, but it is the sibling of the file you changed — if the guard is the reason for the upload-side change, the download side needs it too.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| android_supported = set(get_android_language_mapping().values()) | ||
| for directory in _android_res_roots(repo_root): | ||
| yield directory, android_supported, ANDROID_VALUES_PREFIX |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: values- is Android's prefix for every configuration qualifier, not just locales, so prune_locale_dirs will rmtree any values-* directory whose suffix isn't in the language mapping. Against the real mapping:
removed: ['values-fr-rFR-night', 'values-land', 'values-night', 'values-sw600dp', 'values-v21']
kept: ['drawable', 'values', 'values-b+es+419', 'values-fr-rFR']
platforms/android/app/src/main/res has only values/ today, but the tree already uses qualifier dirs elsewhere (mipmap-anydpi-v26, drawable-hdpi). The first values-night/ added gets silently deleted by the next make i18n-download and lands in the auto-generated translation PR as a deletion, reported as "removed unsupported language directory".
Gate on the qualifier looking like a language before testing membership — e.g. only consider names matching ^(b\+[a-z]{2,3}(\+[A-Za-z0-9]+)*|[a-z]{2,3}(-r[A-Z]{2})?)$ — and leave everything else alone. test_prune_with_prefix_only_touches_prefixed_dirs covers drawable/xml/bare values but no values--prefixed non-language qualifier, which is why this wasn't caught.
| targets = _target_dirs(repo_root) | ||
|
|
||
| for expected in ( | ||
| "kolibri/locale", |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: these comparisons mix separators and will fail on windows-latest (.github/workflows/tox.yml:277 runs pytest ... test). The module builds targets with native separators (os.path.join(repo_root, "kolibri", "locale") and glob.glob, which returns backslashes on Windows), but the tests join forward-slash literals: ntpath.join(r"C:\tmp\x", "kolibri", "locale") → C:\tmp\x\kolibri\locale vs ntpath.join(r"C:\tmp\x", "kolibri/locale") → C:\tmp\x\kolibri/locale.
On Windows this file fails here, and at lines 100 and 111 with StopIteration from the next(...) generators. Worse, test_vendored_locales_are_left_alone asserts not in, so it passes vacuously once separators diverge — it isn't testing what it claims on any platform where the join shape differs.
Split the literals (os.path.join(repo_root, *"kolibri/locale".split("/"))) or os.path.normpath both sides before comparing.
| # --all-packages installs the python_packages/* members; without them the | ||
| # Makefile's importability guard silently skips their message extraction and | ||
| # their CSVs never reach Crowdin. | ||
| run: uv sync --all-packages |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: --all-packages cannot make that guard pass, so the comment asserts a behaviour that doesn't occur. The guard runs a bare interpreter:
# Makefile:260 (i18n-extract-frontend) and Makefile:316 (i18n-download-translations)
if ! python -c "import $$module" >/dev/null 2>&1; then \Bare python, not uv run python — which every other Python recipe in this Makefile uses (205, 244, 248, 275, 285, 306). uv sync installs into .venv/ and nothing here puts it on PATH: setup-uv@v8.2.0 is invoked without activate-environment, and no step writes VIRTUAL_ENV/$GITHUB_PATH. So the guard's interpreter is the runner's system Python, which never sees the workspace members, and the plugin CSVs still don't reach Crowdin.
The fix is in the Makefile: change both guards to uv run python -c "import $$module" (keeping --all-packages here). Setting activate-environment: true on the setup-uv step would also work, but the Makefile change makes the recipes correct when run locally too.
| os.path.join(repo_root, top, "**", name), recursive=True | ||
| ): | ||
| # Vendored dependencies ship their own catalogs; not ours to prune. | ||
| if "node_modules" in path.split(os.sep): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: node_modules is the only exclusion, but the recursive globs also walk gitignored build output. platforms/android/.gitignore lists tar/, tar/extracted/, android_root/, build/, dist/, bin/ — none hidden, so glob(recursive=True) descends into all of them:
platforms/android/tar/extracted/kolibri-0.19/kolibri/locale -> removed ['tr_TR']
platforms/android/build/intermediates/res -> discovered as an android res root
So anyone who has built the APK, unpacked the Kolibri tar, or run PyInstaller under platforms/desktop-app/ gets shutil.rmtree applied inside a staged build by a routine make i18n-download. Nothing is lost from the repo, but the staged build is silently corrupted and the pruning report is noisy.
Either extend the existing exclusion to the artefact names (build, dist, tar, android_root, bin, *.egg-info), or restrict discovery to git-tracked paths (git ls-files) — which is exactly the set this script is meant to keep clean.
Separately: the docstring says these roots are "found by shape", but unlike _android_res_roots (which checks for a values/ sibling) this matches purely on the directory name. Worth either adding the shape check or rewording.
| # on a miss silently drops `scheme`/`first_line_contains_header`, so the file uploads | ||
| # clean but parses to nothing. `type: csv` does not help — isSpreadsheet ignores it. | ||
| # | ||
| "preserve_hierarchy": true |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: the comment says every entry dests to the project root, "pinning every file to the flat path its translations are already attached to". Four of the seven entries are actually moves or renames:
| entry | old flattened path | new dest |
|---|---|---|
| plugin frontend CSVs | /<plugin>-messages.csv |
/optional_plugins/<plugin>-messages.csv |
| oidc-client backend PO | /kolibri_oidc_client_plugin.django.po |
/optional_plugins/... |
| desktop-app wxapp PO | /wxapp.po |
/desktop_app.po |
| desktop installer PO | /messages.po |
/desktop_installer.messages.po |
crowdin upload sources keys on the project path, so a changed path creates a new file and the old one stays behind holding whatever translations it had. The desktop entries only landed on 2026-07-23 (b4f6f69cd0, 08554f1593) so they've plausibly never been uploaded, but the plugin CSV entry dates to 2026-07-10 (68f8e37c42). A --branch verification run wouldn't surface this, since everything is new on a Crowdin branch regardless.
Have you checked the live project for translations on the four pre-move paths? If there are any, move the files in the Crowdin UI (which preserves them) rather than letting the upload re-create them. Either way, correct the comment to describe the actual intent — flat by default, optional_plugins/ for de-prioritised catalogs — and say whether the orphaned root-level files get deleted.
| # downloads land back in the correct plugin locale/ regardless. A new frontend | ||
| # plugin needs NO change here. (Ignore node_modules so the recursive `**` can't | ||
| # pick up any vendored CSVs.) | ||
| {"source": "/python_packages/**/locale/en/LC_MESSAGES/*.csv", "dest": "/optional_plugins/%file_name%.csv", "ignore": ["/python_packages/**/node_modules/**"], "first_line_contains_header": true, "scheme": "identifier,source_phrase,context,translation", "update_option": "update_as_unapproved", "translation": "/python_packages/**/locale/%locale_with_underscore%/LC_MESSAGES/%original_file_name%", "languages_mapping": *language_mapping}, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: both invariants the new comments call load-bearing are untested, and both fail silently — a CSV dest that loses its literal .csv makes isSpreadsheet() drop scheme/first_line_contains_header (uploads clean, parses to nothing), and an entry without a dest quietly acquires a hierarchical path.
The repo already has this test shape: test/test_crowdin_android_config.py and test/test_i18n_desktop_crowdin.py both parse crowdin.yml under pytest.importorskip("yaml") and assert per-entry invariants. Three assertions over config["files"] would cover it: preserve_hierarchy is True; every entry has a dest whose extension matches its source's; dest values are unique. The comment at lines 49-57 explicitly anticipates someone adding a new plugin PO entry — without this, they reintroduce a collision or a schema-less CSV with no signal until a translator reports empty strings.
| # keys the locale definitions by hyphen-lowercase codes (es-es, zh-hans). Both | ||
| # maps derive from the same language_info.json via generate_mapping.py. | ||
| {"source": "/platforms/desktop-app/src/kolibri_app/locales/en/LC_MESSAGES/wxapp.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/src/kolibri_app/locales/%locale_with_underscore%/LC_MESSAGES/wxapp.po", "languages_mapping": *language_mapping}, {"source": "/platforms/desktop-app/installer/translations/locale/en/messages.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/installer/translations/locale/%locale%/messages.po", "languages_mapping": {"locale": {"ach": "ach-ug", "ar": "ar", "bg": "bg-bg", "bn": "bn-bd", "de": "de", "el": "el", "en": "en", "es-ES": "es-es", "fa": "fa", "fr": "fr-fr", "fv": "ff-cm", "gu-IN": "gu-in", "ha": "ha", "hi": "hi-in", "ht": "ht", "id": "id", "it": "it", "ka": "ka", "km": "km", "ko": "ko", "la": "es-419", "mr": "mr", "my": "my", "ny": "ny", "pa-IN": "pa", "pt-BR": "pt-br", "pt-mz": "pt-mz", "sw-TZ": "sw-tz", "te": "te", "uk": "uk", "ur-PK": "ur-pk", "vi": "vi", "yo": "yo", "zh-CN": "zh-hans"}}}, | ||
| {"source": "/platforms/desktop-app/src/kolibri_app/locales/en/LC_MESSAGES/wxapp.po", "dest": "/desktop_app.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/src/kolibri_app/locales/%locale_with_underscore%/LC_MESSAGES/wxapp.po", "languages_mapping": *language_mapping}, {"source": "/platforms/desktop-app/installer/translations/locale/en/messages.po", "dest": "/desktop_installer.messages.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/installer/translations/locale/%locale%/messages.po", "languages_mapping": {"locale": {"ach": "ach-ug", "ar": "ar", "bg": "bg-bg", "bn": "bn-bd", "de": "de", "el": "el", "en": "en", "es-ES": "es-es", "fa": "fa", "fr": "fr-fr", "fv": "ff-cm", "gu-IN": "gu-in", "ha": "ha", "hi": "hi-in", "ht": "ht", "id": "id", "it": "it", "ka": "ka", "km": "km", "ko": "ko", "la": "es-419", "mr": "mr", "my": "my", "ny": "ny", "pa-IN": "pa", "pt-BR": "pt-br", "pt-mz": "pt-mz", "sw-TZ": "sw-tz", "te": "te", "uk": "uk", "ur-PK": "ur-pk", "vi": "vi", "yo": "yo", "zh-CN": "zh-hans"}}}, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: /desktop_installer.messages.po and /desktop_app.po use different naming shapes. /desktop_installer.po would pair; the .messages infix only carries information for the django.po entries, where the source basename is genuinely ambiguous.
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15112 — no new commits since round 1 (HEAD still 99c2eb57, diff byte-identical), so 0 of 8 prior findings are resolved; all 8 remain open. CI has since gone red, and it fails on exactly the file this PR adds — round 1's predicted Windows separator bug is now a reproduced failure (Python unit tests on Windows Server (3.8), 3 failed in test/test_i18n_cleanup_unsupported_languages.py). One new nitpick this round on a comment at crowdin.yml:52.
One point that falls outside the branch diff, so it is a note rather than an inline finding: .github/workflows/i18n-download.yml:38 still runs plain uv sync. make i18n-download-translations depends on i18n-extract-frontend and runs a second identically-guarded create-message-files --plugins loop, so whatever fix lands for the upload workflow needs to land there too — otherwise plugin CSVs are never extracted, nothing matches the new crowdin.yml:46 entry on download, and the tracked *-messages.json are never regenerated.
Prior-finding status
UNADDRESSED — test/test_i18n_cleanup_unsupported_languages.py:81 — mixed-separator path comparisons fail on Windows (now reproduced by CI)
UNADDRESSED — build_tools/i18n/cleanup_unsupported_languages.py:94 — values- prune rmtrees non-language Android qualifiers
UNADDRESSED — .github/workflows/i18n-upload.yml:42 — --all-packages cannot make the bare-python Makefile guard pass
UNADDRESSED — build_tools/i18n/cleanup_unsupported_languages.py:73 — discovery globs recurse into gitignored build output
UNADDRESSED — crowdin.yml:25 — four dest values are moves/renames that can orphan existing Crowdin files
UNADDRESSED — crowdin.yml:46 — no test for the dest invariants the comments call load-bearing
UNADDRESSED — crowdin.yml:65 — /desktop_installer.messages.po vs /desktop_app.po naming shapes differ (nitpick)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| "platforms/handheld-app/installer/translations/locale", | ||
| "platforms/pocket-os/app/src/main/res", | ||
| ): | ||
| assert os.path.join(repo_root, expected) in targets |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: confirmed by CI now, not just predicted — Python unit tests on Windows Server (3.8) (run 30590595820) fails here:
E AssertionError: assert '...\\test_cleanup_targets_discovers0\\kolibri/locale' in {'...\\test_cleanup_targets_discovers0\\kolibri\\locale', ...}
FAILED test_cleanup_targets_discovers_trees_by_shape_not_by_name
FAILED test_plain_locale_dirs_accept_any_configured_convention (line 97)
FAILED test_android_res_prunes_values_qualifiers (line 108)
The module builds targets with os.path.join/glob.glob (native \ on Windows) while these tests join forward-slash literals. Split the literals — os.path.join(repo_root, *"kolibri/locale".split("/")) — or os.path.normpath both sides.
Note also that test_vendored_locales_are_left_alone (line 89) asserts not in, so under the same divergence it passes vacuously and is not testing what it claims. This is the sole reason the PR's checks are red.
|
|
||
| android_supported = set(get_android_language_mapping().values()) | ||
| for directory in _android_res_roots(repo_root): | ||
| yield directory, android_supported, ANDROID_VALUES_PREFIX |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: values- is Android's prefix for every configuration qualifier, not just locales, so prune_locale_dirs rmtrees any values-* dir whose suffix isn't a language code. Verified against the real function:
$ ls -> values values-night values-v21 values-sw600dp values-fr-rFR values-tr-rTR
removed: ['values-night', 'values-sw600dp', 'values-tr-rTR', 'values-v21']
left: ['values', 'values-fr-rFR']
platforms/android/app/src/main/res has only values/ today, but the tree already uses qualifier dirs elsewhere (mipmap-anydpi-v26, drawable-hdpi), so the first values-night/values-v21 added gets silently deleted by the next make i18n-download — which runs unattended in i18n-download.yml and auto-opens a PR whose diff already touches hundreds of translation files, so the deletion would be invisible in review.
Gate on the qualifier looking like a language before testing membership (e.g. ^(b\+[a-z]{2,3}(\+[A-Za-z0-9]+)*|[a-z]{2,3}(-r[A-Z]{2})?)$) and leave everything else alone. test_prune_with_prefix_only_touches_prefixed_dirs covers only drawable/xml/bare values, none of which start with values-, so it doesn't exercise the hazard — add values-night/values-v21 to it.
| # --all-packages installs the python_packages/* members; without them the | ||
| # Makefile's importability guard silently skips their message extraction and | ||
| # their CSVs never reach Crowdin. | ||
| run: uv sync --all-packages |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: the diagnosis in the comment is right, but --all-packages can't fix it, so the comment asserts a behaviour that doesn't occur. The guards at Makefile:260 and Makefile:316 run a bare interpreter:
if ! python -c "import $$module" >/dev/null 2>&1; then \not uv run python — unlike every other Python recipe in this Makefile (205, 244, 248, 275, 285, 306). uv sync installs into .venv/ and nothing here puts it on PATH: setup-uv@v8.2.0 is invoked without activate-environment, and no step writes VIRTUAL_ENV/$GITHUB_PATH. The guard's interpreter is the runner's system Python, which never sees the workspace members, so the plugin CSVs still don't reach Crowdin.
Fix in the Makefile: uv run python -c "import $$module" in both guards (keeping --all-packages here). That also makes the recipes correct when run locally.
| os.path.join(repo_root, top, "**", name), recursive=True | ||
| ): | ||
| # Vendored dependencies ship their own catalogs; not ours to prune. | ||
| if "node_modules" in path.split(os.sep): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: node_modules is the only exclusion, but the recursive globs also walk gitignored build output. Dot-prefixed dirs are safe (glob skips them, so .venv/ is fine — verified), but non-hidden build trees are matched, and platforms/android/.gitignore:24 shows android_root/ is exactly one: p4a unpacks a full CPython + site-packages there, and django/conf/locale/ is one dir per language. A developer who has built the APK and then runs make i18n-download loses Django's ar/, de/, tr/… out of the p4a python-install, plus any dists/*/src/main/res/values-* qualifiers. Same for python_packages/*/build/lib/**/locale after an sdist build.
Nothing tracked is lost, so not blocking — but the guard is the same shape and just as cheap to extend: exclude build, dist, android_root, site-packages alongside node_modules, and mirror test_vendored_locales_are_left_alone for one of them.
| # on a miss silently drops `scheme`/`first_line_contains_header`, so the file uploads | ||
| # clean but parses to nothing. `type: csv` does not help — isSpreadsheet ignores it. | ||
| # | ||
| "preserve_hierarchy": true |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: the comment says dest pins every file "to the flat path its translations are already attached to", but this diff changes four Crowdin paths:
| entry | before | after |
|---|---|---|
| plugin frontend CSVs | /<name>.csv |
/optional_plugins/<name>.csv |
| oidc backend PO | /kolibri_oidc_client_plugin.django.po |
/optional_plugins/kolibri_oidc_client_plugin.django.po |
| wxapp PO | /wxapp.po |
/desktop_app.po |
| installer PO | /messages.po |
/desktop_installer.messages.po |
The CSVs are arguably safe — per this PR's own diagnosis they uploaded but parsed to nothing, so likely nothing is attached. The three .po entries are not covered by that argument: PO uploads were never affected by isSpreadsheet(), and those entries have been on develop since 5e60356872 (2026-07-10) and b4f6f69cd0/08554f1593 (2026-07-23). If i18n-upload.yml (a workflow_dispatch) has run since, those files exist with translations attached and the rename orphans them.
I can't inspect the Crowdin project from here. Please confirm before merge whether any of those three PO files carries translations — if so, use the Crowdin UI's move/rename (preserves the file ID) rather than letting the CLI create new files — and reword the comment so it describes the moves it is performing.
| # downloads land back in the correct plugin locale/ regardless. A new frontend | ||
| # plugin needs NO change here. (Ignore node_modules so the recursive `**` can't | ||
| # pick up any vendored CSVs.) | ||
| {"source": "/python_packages/**/locale/en/LC_MESSAGES/*.csv", "dest": "/optional_plugins/%file_name%.csv", "ignore": ["/python_packages/**/node_modules/**"], "first_line_contains_header": true, "scheme": "identifier,source_phrase,context,translation", "update_option": "update_as_unapproved", "translation": "/python_packages/**/locale/%locale_with_underscore%/LC_MESSAGES/%original_file_name%", "languages_mapping": *language_mapping}, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: both invariants the new comments call load-bearing are untested, and both fail silently — a CSV dest not ending in a literal .csv uploads clean and parses to nothing; preserve_hierarchy: false strands every file. This repo already has the habit of testing this config: test/test_crowdin_android_config.py asserts the android entry's type/translation/languages_mapping, and test/test_i18n_desktop_crowdin.py asserts both desktop entries. A handful of assertions over the parsed config would pin the reasoning that is currently only a comment:
preserve_hierarchy is True;- every entry has a
dest; - every entry carrying a
schemehas adestending in.csv; destvalues are unique across entries.
| # Plugin backend PO — one entry PER plugin, unlike the frontend CSVs above, and | ||
| # likewise filed under optional_plugins/. Django makemessages emits no domain but | ||
| # `django`, so every plugin's catalog is named django.po and they would collide | ||
| # with each other inside the folder. Only a literal `dest` separates them, and a |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: "a literal dest takes no placeholder" is disproved six lines earlier — line 46 uses "dest": "/optional_plugins/%file_name%.csv". The real constraint is narrower: %file_name% resolves to the basename, which is django for every plugin, so it can't disambiguate them. Worth stating that way, since as written the rule reads as general.
Separately, a question rather than a claim (I haven't read the CLI): is %original_path% or %parent_directory_name% usable in dest? If so, "/optional_plugins/%original_path%/django.po" would collapse this to one wildcard entry and remove the "a plugin gaining its first backend string needs a new entry" maintenance burden.
| # keys the locale definitions by hyphen-lowercase codes (es-es, zh-hans). Both | ||
| # maps derive from the same language_info.json via generate_mapping.py. | ||
| {"source": "/platforms/desktop-app/src/kolibri_app/locales/en/LC_MESSAGES/wxapp.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/src/kolibri_app/locales/%locale_with_underscore%/LC_MESSAGES/wxapp.po", "languages_mapping": *language_mapping}, {"source": "/platforms/desktop-app/installer/translations/locale/en/messages.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/installer/translations/locale/%locale%/messages.po", "languages_mapping": {"locale": {"ach": "ach-ug", "ar": "ar", "bg": "bg-bg", "bn": "bn-bd", "de": "de", "el": "el", "en": "en", "es-ES": "es-es", "fa": "fa", "fr": "fr-fr", "fv": "ff-cm", "gu-IN": "gu-in", "ha": "ha", "hi": "hi-in", "ht": "ht", "id": "id", "it": "it", "ka": "ka", "km": "km", "ko": "ko", "la": "es-419", "mr": "mr", "my": "my", "ny": "ny", "pa-IN": "pa", "pt-BR": "pt-br", "pt-mz": "pt-mz", "sw-TZ": "sw-tz", "te": "te", "uk": "uk", "ur-PK": "ur-pk", "vi": "vi", "yo": "yo", "zh-CN": "zh-hans"}}}, | ||
| {"source": "/platforms/desktop-app/src/kolibri_app/locales/en/LC_MESSAGES/wxapp.po", "dest": "/desktop_app.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/src/kolibri_app/locales/%locale_with_underscore%/LC_MESSAGES/wxapp.po", "languages_mapping": *language_mapping}, {"source": "/platforms/desktop-app/installer/translations/locale/en/messages.po", "dest": "/desktop_installer.messages.po", "update_option": "update_as_unapproved", "translation": "/platforms/desktop-app/installer/translations/locale/%locale%/messages.po", "languages_mapping": {"locale": {"ach": "ach-ug", "ar": "ar", "bg": "bg-bg", "bn": "bn-bd", "de": "de", "el": "el", "en": "en", "es-ES": "es-es", "fa": "fa", "fr": "fr-fr", "fv": "ff-cm", "gu-IN": "gu-in", "ha": "ha", "hi": "hi-in", "ht": "ht", "id": "id", "it": "it", "ka": "ka", "km": "km", "ko": "ko", "la": "es-419", "mr": "mr", "my": "my", "ny": "ny", "pa-IN": "pa", "pt-BR": "pt-br", "pt-mz": "pt-mz", "sw-TZ": "sw-tz", "te": "te", "uk": "uk", "ur-PK": "ur-pk", "vi": "vi", "yo": "yo", "zh-CN": "zh-hans"}}}, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: /desktop_app.po and /desktop_installer.messages.po use different naming shapes — the first drops the source basename, the second keeps it. /desktop_installer.po would match its sibling.
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15112 — all 8 prior findings resolved; 4 new findings (2 suggestions, 2 nitpicks), none blocking.
I re-ran discovery against this checkout (nothing would be deleted) and confirmed empirically that uv run does not prune workspace members installed by uv sync --all-packages, so the Makefile guard fix holds. pytest test/test_i18n_cleanup_unsupported_languages.py test/test_crowdin_config.py → 17 passed locally. No CI failures, but Python unit tests on Windows Server (3.8) — the job that was red last round — is still pending, so the separator fix isn't yet confirmed by CI. No UI files changed; visual verification does not apply.
The sharpest new item is _is_excluded matching against the whole absolute path (inline on cleanup_unsupported_languages.py:94): a checkout under any directory segment named build, dist, tar, node_modules, site-packages or android_root silently drops every glob-discovered root — the exact failure mode this PR exists to prevent, with no signal.
Nice touch: _path() splitting on / before os.path.join is the right shape for the Windows fix, and test_untracked_build_output_is_left_alone pairs its not in assertion with a positive in assertion (line 141) so the negative can't pass by separator divergence — that was the sharper half of last round's finding.
Prior-finding status
RESOLVED — test/test_i18n_cleanup_unsupported_languages.py:81 — mixed-separator path comparisons fail on Windows
RESOLVED — build_tools/i18n/cleanup_unsupported_languages.py:94 — values- prune rmtrees non-language Android qualifiers
RESOLVED — .github/workflows/i18n-upload.yml:42 — --all-packages cannot make the bare-python Makefile guard pass
RESOLVED — build_tools/i18n/cleanup_unsupported_languages.py:73 — discovery globs recurse into gitignored build output
RESOLVED — crowdin.yml:46 — no test for the dest invariants the comments call load-bearing
RESOLVED — crowdin.yml:52 — "a literal dest takes no placeholder" is disproved by line 46
RESOLVED — crowdin.yml:65 — /desktop_installer.messages.po vs /desktop_app.po naming shapes differ
RESOLVED — .github/workflows/i18n-download.yml:38 — download workflow still on plain uv sync
ACKNOWLEDGED — crowdin.yml:25 — four dest values are moves/renames that can orphan existing Crowdin files; the comment now documents the hazard and the UI-move remedy, and whether the pre-move .po paths carry translations is only checkable in the live project
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
|
|
||
| def _is_excluded(path): | ||
| parts = path.split(os.sep) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: this splits the full path returned by glob, which starts at repo_root — so a checkout under any directory segment named build, dist, tar, node_modules, site-packages or android_root (~/build/kolibri, /srv/dist/kolibri) excludes every glob-discovered root. kolibri/locale is yielded unconditionally so it still gets pruned, but all the trees this PR exists to cover silently drop out, and the failure mode is the one being fixed: unsupported languages committed into python_packages/* and platforms/* by the auto-generated translation PR, with no signal.
Match only the portion below the root:
def _is_excluded(path, repo_root):
parts = os.path.relpath(path, repo_root).split(os.sep)| # correct plugin locale/. A new frontend plugin needs NO change here. (Ignore | ||
| # node_modules so the recursive `**` can't pick up any vendored CSVs.) | ||
| {"source": "/python_packages/**/locale/en/LC_MESSAGES/*.csv", "ignore": ["/python_packages/**/node_modules/**"], "first_line_contains_header": true, "scheme": "identifier,source_phrase,context,translation", "update_option": "update_as_unapproved", "translation": "/python_packages/**/locale/%locale_with_underscore%/LC_MESSAGES/%original_file_name%", "languages_mapping": *language_mapping}, | ||
| # `dest` files them under optional_plugins/ in the Crowdin project: they are not a |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: the old comment stated the invariant this wildcard entry rests on — plugin frontend CSVs are namespace-prefixed (kolibri_sentry_plugin-messages.csv), so their basenames are globally unique. That sentence is gone, but "dest": "/optional_plugins/%file_name%.csv" is still a flattening dest over a ** source; it just flattens into optional_plugins/ instead of the project root, so two plugins sharing a CSV basename would now collide there. The invariant didn't stop being load-bearing — only its scope moved.
Worth restoring because the PO comment below (lines 56–63) reasons at length about exactly this collision for django.po and concludes "a literal dest per plugin it is"; a reader comparing the two entries can't see why the CSV entry gets away with one wildcard. test_dests_are_unique can't cover it either — the CSV entry contributes a single template string, not the resolved per-plugin names.
There was a problem hiding this comment.
Understood — deferring to @marcellamaki's assessment that the crowdin.yml comments carry enough context for a future revisit. Withdrawing this suggestion.
| # shape of a locale one — `fr`, `fr-rFR` or BCP-47 `b+es+419` — and leave the rest | ||
| # (values-night, values-v21, values-sw600dp) alone. | ||
| ANDROID_LOCALE_QUALIFIER = re.compile( | ||
| r"^values-(b\+[a-z]{2,3}(?:\+[A-Za-z0-9]+)*|[a-z]{2,3}(?:-r[A-Z]{2})?)$" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: the regex fixes the reported cases (values-night, values-v21, values-sw600dp, values-land all correctly fall through), but [a-z]{2,3} also matches Android's UI-mode qualifier car:
>>> ANDROID_LOCALE_QUALIFIER.match("values-car").group(1)
'car'
car isn't in the android mapping, so values-car/ would be removed. Unlikely for this app to grow, and car genuinely is an ISO 639-3 code — a note rather than a request. If you want it airtight, an explicit deny-set of the handful of ≤3-char non-locale qualifiers is cheaper than tightening the shape rule further.
| assert {"es-rES", "b+es+419"} <= supported | ||
|
|
||
|
|
||
| def test_downloaded_extras_are_supported_nowhere(repo_root): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: this exercises no code path in the PR — it restates the current contents of language_info.json. The day Kolibri adds Turkish or Nepali it fails, and the failure points at this file rather than at the change that caused it. The prune_locale_dirs tests above already prove unsupported names get removed; consider dropping this one, or narrowing it to a single sentinel with a comment saying it is a canary for the mapping generator.
`dest` only works under preserve_hierarchy: true, so the previous flat-plus-dest config never validated and every upload failed on it. Turn hierarchy on and dest every entry back to the project root, leaving the Crowdin layout flat so existing files keep their paths and their file IDs. A CSV dest must end in a literal `.csv`: the CLI extension-tests the unresolved dest string, and on a miss drops the scheme silently, uploading a file that parses to nothing. File plugin catalogs under optional_plugins/ — they are not a translation priority. Install the workspace packages in CI, without which plugin message extraction is skipped and those CSVs never reach Crowdin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bd4b7a59DK4bhQpaQmRV1
cleanup_unsupported_languages only walked kolibri/locale, so the plugin, desktop-app and Android catalogs kept whatever extra languages Crowdin delivered — a languages_mapping renames codes, it does not filter. For Android that meant unsupported values-<code> directories shipping in the APK. Find the trees by shape rather than by name, so a new plugin or platform app is covered without an edit here. Plain locale directories prune against the union of the gettext and installer code sets, since a path does not reveal which convention its tree follows; res/ prunes values-<qualifier> and leaves the other resource directories alone. Invoke via -m, as the mapping helpers are now imported as a package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Bd4b7a59DK4bhQpaQmRV1
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15112 — 14 of 14 prior findings resolved or acknowledged; 1 new nitpick on a comment.
Re-ran discovery against this checkout: zero deletions across all seven discovered locale trees. pytest test/test_i18n_cleanup_unsupported_languages.py test/test_crowdin_config.py → 18 passed locally. CI at 2714837 is green on everything complete, but the Python unit tests workflow — including the Windows Server 3.8 job that surfaced the earlier separator failure — is still pending, so the platform matrix isn't confirmed on this commit yet. No UI files changed; visual verification and manual QA do not apply.
- nitpick — inline, on the Android qualifier comment.
Prior-finding status
RESOLVED — build_tools/i18n/cleanup_unsupported_languages.py:94 — _is_excluded matched against the whole absolute path
RESOLVED — build_tools/i18n/cleanup_unsupported_languages.py:28 — ANDROID_LOCALE_QUALIFIER also matched values-car / values-tv
RESOLVED — build_tools/i18n/cleanup_unsupported_languages.py — recursive globs walked gitignored build output
RESOLVED — test/test_i18n_cleanup_unsupported_languages.py:120 — path comparisons mixed separators, failed on windows-latest
RESOLVED — test/test_i18n_cleanup_unsupported_languages.py — test_downloaded_extras_are_supported_nowhere restated language_info.json
RESOLVED — .github/workflows/i18n-upload.yml:42 — comment asserted a behaviour --all-packages doesn't produce
RESOLVED — crowdin.yml:32 — dest comment overstated what the flat path pins
RESOLVED — crowdin.yml:56 — untested, silently-failing invariants named load-bearing in comments
RESOLVED — crowdin.yml — "a literal dest takes no placeholder" contradicted by line 46
RESOLVED — crowdin.yml — /desktop_app.po vs /desktop_installer.messages.po naming shapes
ACKNOWLEDGED — crowdin.yml:25 — four dest values are moves that can orphan existing Crowdin files
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| # `values-` prefixes every Android resource qualifier, not just locales, so match the | ||
| # shape of a locale one — `fr`, `fr-rFR` or BCP-47 `b+es+419` — and leave the rest | ||
| # (values-night, values-v21, values-sw600dp) alone. `car` and `tv` are the UI-mode | ||
| # qualifiers short enough to also be shaped like a language code (both are real ISO 639 |
There was a problem hiding this comment.
nitpick: tv is not an ISO 639 code — there is no tv in 639-1/2/3 (Tuvaluan is tvl; the 639-1 t* set is ta/te/tg/th/ti/tk/tl/tn/to/tr/ts/tt/tw/ty). car is real (639-3, Galibi Carib).
The denial is still correct and still necessary — tv matches [a-z]{2,3}, which is the actual reason it needs excluding. Only the parenthetical rationale is wrong; "short enough to also be shaped like a language code" already carries the argument, so it can just be dropped.
marcellamaki
left a comment
There was a problem hiding this comment.
I think the comments in the crowdin.yaml file make sense and explain the approach and situation/why it needed to change (mainly i am thinking just enough to inform things if we need to revisit when the crowdin api changes again)
i think the layout in crowdin also will work fine.
Summary
Fixes crowdin configuration to fix configuration linting due to incompatibility between
preserve_hierarchy: falseand use of thedestoption to avoid name conflicts between django.po files.Switches to
preserve_hierarchy: truebut usesdestextensively to retarget files for more translator friendly layout and naming.For the new layout see
final-verify-testhere: https://crowdin.com/project/kolibri/sources/filesAdds an explicit .csv extension to the uploaded CSV files, as without it, Crowdin fails to recognize the schema for the files and they have to be manually remediated for configuration in the crowdin UI
This finessed layout also sets us up in future to do a more 'progressive' translation pass, once we organize the strings in a way that allows for this.
Updates the unsupported language cleanup script to make sure package and platform strings for not yet supported languages are not committed.
References
Fixes crowdin configuration issues left over from monorepo work
Reviewer guidance
Confirm the crowdin.yaml makes sense
Verify the layout/file naming
AI usage
Used Claude Code to iterate on the crowdin.yaml configuration, until I found a version that mostly preserved our existing layout but gave the remapping required to avoid django.po naming conflicts. Also used it to update the cleanup script using TDD.