feat(add-data): internationalize the Add Data dialog (#427)#438
Conversation
Move all hardcoded English strings in the Add Data dialog to react-i18next under a new addData.* namespace (en.json is the source of truth): - Shell title/description (KIND_LABELS/KIND_DESCRIPTIONS -> addData.kind.*) - Shared chrome in shared.tsx (layer name, insert-before, footer buttons) - ServiceLibrarySection.tsx + Uncategorized grouping label - Every web-service and file source form: labels, placeholders, select options, buttons, status notices, and validation errors Reuses common.save/common.cancel where they exist. Other locales fall back to English until translated. Closes #427
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAll hardcoded English strings in the "Add Data" workflow are replaced with ChangesAdd Data workflow i18n migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code reviewThis is a large, mechanical i18n migration (662 additions, 281 deletions across 14 files). The approach is sound: BugsNone found. SecurityNone found. PerformanceNone found. Quality
CLAUDE.md
|
- DeckVizSource: lift the "·" separator and trailing period out of the JSX template into a translatable `addData.deckViz.loadedTabular` wrapper key, so translators control word order/punctuation and the row/column fragments stay correctly pluralized (loadedRows/loadedColumns now carry no "Loaded" prefix). - ServiceLibrarySection: rename the `importedDropped` interpolation var from `count` to `dropped` so i18next does not attempt plural resolution on a key with no plural variants. - DelimitedText/Gpx/Mbtiles sources: capture the default layer name once via a lazy `useState` so the "did the user rename it?" comparison stays stable even if the UI language changes mid-session. - Add tests/add-data-i18n.test.ts: asserts every KIND_I18N_KEY maps to a label+description in en.json, catching renamed/removed dialog kind subtrees at CI time (the dialog resolves these via a runtime-interpolated t() key that the type checker cannot validate).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/add-data-i18n.test.ts`:
- Around line 32-38: The assertions for entry.label and entry.description allow
whitespace-only strings to pass validation. Modify the assert.ok() calls to trim
the values before checking them for truthiness. For both entry.label and
entry.description, apply the trim() method before passing them to assert.ok() so
that strings containing only whitespace will properly fail the assertion and
enforce that translations contain meaningful content.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02ac5e30-f6cb-44e0-a7bf-6dfd3b181de2
📒 Files selected for processing (7)
apps/geolibre-desktop/src/components/layout/add-data/ServiceLibrarySection.tsxapps/geolibre-desktop/src/components/layout/add-data/sources/DeckVizSource.tsxapps/geolibre-desktop/src/components/layout/add-data/sources/DelimitedTextSource.tsxapps/geolibre-desktop/src/components/layout/add-data/sources/GpxSource.tsxapps/geolibre-desktop/src/components/layout/add-data/sources/MbtilesSource.tsxapps/geolibre-desktop/src/i18n/locales/en.jsontests/add-data-i18n.test.ts
Code reviewThis PR cleanly internationalizes the entire Add Data dialog as one consistent unit. The approach — a new Bugs / Logic errors
Quality / Maintainability
Security: Nothing found. Performance: Nothing found. CLAUDE.md: No violations. The PR uses |
- tests/add-data-i18n.test.ts (CodeRabbit): trim label/description before the
non-empty assertion so whitespace-only translations fail.
- ServiceLibrarySection + en.json (Claude): replace the fragile
imported + importedDropped suffix-concatenation with two self-contained plural
keys (imported / importedWithDropped), so each notice is a complete sentence a
translator can reorder freely.
- WfsSource (Claude): translate the "max features" numeric validation error
(addData.wfs.errorMaxFeaturesNumeric) via an inline check instead of the
English-only parseOptionalNumber helper label.
- VideoSource (Claude): parse the four corners with a local translated
parseCorner (addData.video.errorCorner* + corner-name keys) so the corner
validation errors are localized.
- DeckVizSource (Claude): document that loadedTabular interpolates pre-pluralized
{{rows}}/{{columns}} fragments; kept the nested-t() composition so each count
keeps its own i18next plural rules (a 4-key rowOne/colOne form would regress
to "=== 1" and break languages with >2 plural forms).
| if (!Number.isFinite(lng) || !Number.isFinite(lat)) { | ||
| throw new Error(t("addData.video.errorCornerNumber", { corner })); |
There was a problem hiding this comment.
Bug: combined lng/lat NaN check loses error precision
The original parseVideoCorner used parseRequiredNumber separately for longitude and latitude, so the error message pinpointed which coordinate was bad ("Enter a numeric top-left longitude" vs. "Enter a numeric top-left latitude"). The combined check here always emits "Enter a numeric {{corner}} longitude and latitude" even when only one of the two values is NaN.
| if (!Number.isFinite(lng) || !Number.isFinite(lat)) { | |
| throw new Error(t("addData.video.errorCornerNumber", { corner })); | |
| if (!Number.isFinite(lng)) { | |
| throw new Error(t("addData.video.errorCornerNumber", { corner, coord: t("addData.video.coordLng") })); | |
| } | |
| if (!Number.isFinite(lat)) { | |
| throw new Error(t("addData.video.errorCornerNumber", { corner, coord: t("addData.video.coordLat") })); | |
| } |
(Requires adjusting the errorCornerNumber key to "Enter a numeric {{corner}} {{coord}}." and adding "coordLng": "longitude" / "coordLat": "latitude" to en.json, or just splitting into two dedicated keys like errorCornerLngNaN / errorCornerLatNaN.)
| "loadPlaceholder": "Load a saved service…", | ||
| "builtin": "{{name}} (built-in)", | ||
| "deleteAria": "Delete saved service", | ||
| "empty": "No saved services yet — fill in the form and choose “Save current”.", |
There was a problem hiding this comment.
Quality: empty string hardcodes the saveCurrent button label
The phrase "Save current" (in curly quotes) is the button's label — but that label is separately translatable as addData.serviceLibrary.saveCurrent. If a locale renders the button as anything other than "Save current", this prose message will be out of sync with the actual UI.
| "empty": "No saved services yet — fill in the form and choose “Save current”.", | |
| "empty": "No saved services yet — fill in the form and save the current settings.", |
Alternatively, omit the specific button reference entirely ("No saved services yet.") so the string doesn't depend on knowing how another key is translated.
| DEFAULT_VIDEO_WEBM_URL, | ||
| } from "../constants"; | ||
| import { createBaseLayer, parseVideoCorner } from "../helpers"; | ||
| import { createBaseLayer } from "../helpers"; |
There was a problem hiding this comment.
Nit: parseVideoCorner is now a dead production export
Dropping parseVideoCorner from the import here means helpers.ts:parseVideoCorner is no longer called from any production file — it's only imported by tests/add-data-helpers.test.ts. The same applies to parseOptionalNumber after the WfsSource change. Neither is broken (the tests keep the contracts alive), but a quick JSDoc on those two helpers clarifying their test-only status would save a future reader from wondering why they exist.
| t("addData.deckViz.loadedTabular", { | ||
| rows: t("addData.deckViz.loadedRows", { count: parsed.rowCount }), | ||
| columns: t("addData.deckViz.loadedColumns", { | ||
| count: parsed.columns.length, |
There was a problem hiding this comment.
Quality: loadedTabular composition is an unchecked inter-key dependency
loadedTabular silently requires loadedRows and loadedColumns to be present and returning sensible strings — TypeScript's typed t() won't flag a missing or renamed loadedRows/loadedColumns key because the template is constructed at runtime. The inline comment explains the design well, but this dependency isn't covered by the new add-data-i18n.test.ts (which only guards addData.kind.*).
Consider adding a small guard in add-data-i18n.test.ts that confirms all three keys exist and produce non-empty output at count=1 and count=2, similar to the kind-label test already there.
Code reviewOverall this is a clean, well-scoped i18n migration. The kind-label indirection via Bugs1.
Quality2. The prose "…choose "Save current"" mirrors 3. After this PR, no production file imports either helper (they're only consumed by 4. The pattern Performance / SecurityNothing worth raising. CLAUDE.mdNo violations. Branch/PR convention followed, |
1 participant
Summary
Internationalizes the entire Add Data dialog as one consistent unit (per the maintainer decision in #427), moving every hardcoded English string to
react-i18nextunder a newaddData.*namespace.en.jsonis the source of truth; other locales fall back to English until translated.Follow-up to #423 / #417.
What changed
AddDataDialog.tsx+constants.ts): replacedKIND_LABELS/KIND_DESCRIPTIONSwith aKIND_I18N_KEYmap; the title/description now resolve viat("addData.kind.<key>.{label,description}").shared.tsx): layer name, insert-before (incl. optgroup labels and the default option), and footer (Add layer/Adding…/ reusedcommon.cancel).ServiceLibrarySection.tsx): title, buttons, aria/title labels, the saved/removed/imported notices (with plural + interpolation), and theUncategorizedgrouping label. TheUNCATEGORIZED_LABELconstant stays as an internal grouping sentinel and is translated only at render.Xyz,Wms,Wfs,Wmts,ArcGIS,Gpx,DelimitedText,Mbtiles,Postgres,Video,DeckViz): labels, placeholders, select options, button states, status notices, and the validation errors thrown in each submit handler. Shared strings (Service URL,Tile size,Source type,Choose file,Optional,Request failed with status {{status}}, …) live underaddData.common.*; reusescommon.save/common.cancel.Notes / scope
"WMS Layer") now resolve throught(), so newly added layers get a localized default name (still user-editable).helpers.ts(parseRequiredNumber,parseVideoCorner, …) are intentionally left as-is: they are technical, framework-agnostic, and covered by unit tests asserting their English messages. The web-service format identifiers (PNG/JPEG) and numeric coordinate example placeholders are also left verbatim.i18next.d.ts(keys checked againsten.json).Testing
npm run typecheck— no new type errors from this change. (There are 5 pre-existing, unrelated errors inpackages/plugins/src/plugins/maplibre-basemap-control.tsabout abasemapremoveevent not present inmaplibre-gl-layer-control@0.16.0's types; confirmed identical on a cleanmain, so not introduced here.)node --testoveradd-data-helpers,service-library,web-service-sync— 51/51 pass.Summary by CodeRabbit
labelanddescriptiontext.