CMG-1057 | Restart Locale Mapper Add Language dropdown#1117
Conversation
…es remain, cap rows at source count The Add Language button on delta-restart iteration was misbehaving in two ways: 1. It stayed disabled after a single addition when the picked CS locale's code happened to match a source locale code (e.g. CS `fr` mapped to source `fr`, or `ja` → `ja`). `filledMappingCount` had a stale `sourceLocaleLabels.has(k)` filter meant to defend against a fallback-key path in `handleSelectedSourceLocale` that no longer exists (source-first pick now no-ops until CS is picked). The filter was dropping legitimate CS keys, freezing the count at 1, and locking the button after the first Add. 2. The `hasIncompleteRow` gate forced users to fully fill each row before Add Language re-enabled. Removed — users can now spawn multiple empty rows and fill them in any order. Added a hard row cap at `totalSources` so N sources → at most N rows on screen (whether they're mapped yet or not), preventing a user from hammering Add and stranding empty rows that no source can back.
🔒 Security Scan Results
⏱️ SLA Breach Summary
🔴 Critical Severity - SLA Breached Issues (with fixes)Showing 1 issue(s) that have exceeded the 15-day SLA threshold:
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
Unblocks the security-sca gate on the CMG-1057/1058 PR — its critical SLA breach was SNYK-JS-ESBUILD-17750822 (esbuild@0.27.3, "Resources downloaded over insecure protocol"), which reached its 15-day SLA threshold while dev's scan hadn't re-run. - upload-api: vitest ^4.0.18 → ^4.1.10 (also @vitest/coverage-v8), pulls in vitest ≥ 4.1.0 which patches CVE-2026-47429 (Vitest UI arbitrary file read/exec) and drops the vulnerable transitive esbuild - ui: refresh package-lock.json so esbuild 0.27.3 → 0.27.7 across all @esbuild/* platform packages (via vite@^7.3.5, no manifest change) No source-code changes; scope-limited to lockfile / dev-dep bumps.
🔒 Security Scan Results
⏱️ SLA Breach Summary
🔴 Critical Severity - SLA Breached Issues (with fixes)Showing 1 issue(s) that have exceeded the 15-day SLA threshold:
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
Prior bump (dcbf00b) moved ui's transitive esbuild from 0.27.3 → 0.27.7 by refreshing the lockfile, but SNYK-JS-ESBUILD-17750822 flags every 0.27.x. The Snyk fix lands in 0.28.x. ui's `vite@^7.3.5` pins esbuild in the 0.27 range, so use an npm override to force ^0.28.1 across every consumer of the package (vite@7.3.5 → esbuild override; storybook's transitive esbuild dedupes to the same). No manifest bump for vite — keeps the update surgical and avoids the vite 7 → 8 major (vite 8 dropped esbuild entirely for rolldown, but that's a bigger migration than a security CI unblock warrants).
🔒 Security Scan Results
⏱️ SLA Breach Summary
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
✅ BUILD PASSED - All security checks passed |
umesh-more-cstk
left a comment
umesh-more-cstk
left a comment
There was a problem hiding this comment.
Review — CMG-1057 Add Language dropdown
The isolated disabled-predicate change is a clean, correct fix. Retiring the sourceLocaleLabels/filledMappingCount filter is the right call — it genuinely mis-fired on legitimate CS keys whose code matched a source code (fr↔fr, ja↔ja). Cap-consistency holds: rowCapReached and allSourcesMapped both derive from sourceLocales, and the delete path clears selectedMappings→Redux so the button re-enables after a row delete.
Verdict: Comment (the code change alone I'd approve; my one blocking-class reservation is scope — see the dependency comments).
Blocking
- Unrelated dependency/lockfile churn vs. stated scope — the PR says "single file / ui only" but also bumps
esbuild(ui overrides) andvitest/@vitest/coverage-v8in upload-api (not even checked in Affected Areas), with large lockfile churn in both packages. See inline comments onui/package.jsonandupload-api/package.json.
Non-blocking
- rowCap/allSourcesMapped drift edge case (inline,
LoadLanguageMapper.tsx) - redundant optional chaining (inline)
- Stale comment (nit):
LoadLanguageMapper.tsx:419still referencesfilledMappingCount, which this PR deleted. TheselectedLocaleKeyguard it documents is still valid — just retarget the wording (e.g. "…would create an orphan mapping key and mis-count mapped sources"). Couldn't anchor inline since that line is outside the diff.
| "form-data": ">=4.0.6", | ||
| "ws": ">=8.21.0" | ||
| "ws": ">=8.21.0", | ||
| "esbuild": "^0.28.1" |
There was a problem hiding this comment.
blocker (scope): This esbuild override is unrelated to CMG-1057 (a locale-mapper UI gate). It's correctly placed in overrides (not runtime deps), but it silently moves the build toolchain 0.27.3 → 0.28.1 inside a bugfix PR and adds ~26 platform packages of lockfile churn.
The description says "Single file" / "Affected Areas: ui only".
Suggestion: split dependency bumps into a separate chore PR, or if a security pin must ride along, call it out in the description and check the "Dependency Update" box.
| "ts-node": "^10.9.2", | ||
| "typescript": "^5.3.3", | ||
| "vitest": "^4.0.18" | ||
| "vitest": "^4.1.10" |
There was a problem hiding this comment.
blocker (scope): vitest (and @vitest/coverage-v8 above) are bumped to ^4.1.10 here in upload-api — but upload-api isn't checked in the PR's Affected Areas, and this is unrelated to CMG-1057. Pulls in oxc/rolldown lockfile churn too.
Suggestion: move these into a separate dependency PR so the bugfix stays isolated and cleanly revertable.
| const hasIncompleteRow = (cmsLocaleOptions?.length ?? 0) > filledMappingCount; | ||
| const mappedSources = new Set( | ||
| Object.values(savedMapping).filter( | ||
| (v): v is string => typeof v === 'string' && v?.length > 0 |
There was a problem hiding this comment.
nit: v is already narrowed to string by typeof v === 'string', so v?.length can be v.length. Harmless, but the ?. reads as if v could still be nullish here.
| const rowCapReached = | ||
| totalSources > 0 && (cmsLocaleOptions?.length ?? 0) >= totalSources; | ||
| return sourcesNotReady || allSourcesMapped || rowCapReached; |
There was a problem hiding this comment.
question (medium): Both rowCapReached and allSourcesMapped assume every value in savedMapping corresponds to a source still present in sourceLocales. If a delta restart can carry a stale mapping for a source that no longer exists (source set shrank between iterations), this re-locks the button — the exact failure class this PR fixes, via a new vector:
Iter 1 mapped: { en-master_locale: en, de: de } // 'de' later removed from source export
Iter 2 sourceLocales: [en, fr, ja] → totalSources = 3
→ rebuilt = 2 rows (master, de); mappedSources = {en, de}
→ user Adds a row (cap 3 reached), maps it to fr
→ mappedSources = {en, de, fr} → size 3 ≥ 3 → allSourcesMapped = true → disabled
→ 'ja' is now unreachable
Is localeMapping guaranteed to be reconciled against the current sourceLocales on restart? If a removed source can linger, harden by intersecting with the live source set:
const sourceSet = new Set(sourceLocales?.map((l) => l.value) ?? []);
const mappedSources = new Set(
Object.values(savedMapping).filter(
(v): v is string => typeof v === 'string' && v.length > 0 && sourceSet.has(v)
)
);If reconciliation is guaranteed upstream, no change needed — just confirm it.
🔗 Jira Ticket
CMG-1057
📋 PR Type
📝 Description
What changed?
filledMappingCounthad a stalesourceLocaleLabels.has(k)filter meant to defend againsthandleSelectedSourceLocale's old fallback-key path (source-first pick writing undersource.label.toLowerCase()). That path has been gone for a while — source-first pick now no-ops inselectedMappingsuntil CS is also picked — but the filter kept firing for legitimate CS locale keys whose codes match a source locale code (e.g. CSfrmapped to sourcefr, orja→ja), dropping them from the count and freezing the button at 1.hasIncompleteRowgate forced a strict "fill the current row before adding another", which meant the button toggled off after every Add and only came back when the row was fully mapped. Users can now spawn multiple empty rows and fill them in any order.sourceLocales.length. Prevents hammering Add and spawning empty rows that can never be filled — for 3 sources you get at most 3 rows on screen, whether they're mapped yet or not. Deleting a row frees up a slot again.Why?
On delta-restart with sources like
['en', 'fr', 'ja'], a user could map the first additional locale, and Add Language would then stay disabled forever — trapping the user with 1 unmapped source they couldn't reach. Root cause was the stale filter above. While there, the "fully-fill-before-next-Add" behaviour was tightening the UX for no benefit; loosening it and adding a hard row cap gave a cleaner mapping flow.🧩 Affected Areas
api— Node.js backendui— React frontendupload-api— Upload API serverdocker/docker-composeSingle file: ui/src/components/DestinationStack/Actions/LoadLanguageMapper.tsx (Add Language
disabledpredicate only).🧪 How to Test
Prerequisite: a delta-migration project that finished iteration 1 with ≥ 3 source locales (e.g. Sitecore/Contentful project with source
['en', 'fr', 'ja']and onlyenmapped as master).en-us (master) ↔ en) renders with its saved source populated, and Add Language is enabled.fr→ sourcefr). Add Language still enabled.ja→ sourceja). All sources mapped → button remains disabled (correct — no more sources to add).Expected result:
sourceLocales.length.fr↔fr,ja↔ja) locks the button.📸 Screenshots / Recordings
sourceLocales.length; users can spawn multiple empty rows and fill in any order.🔗 Related PRs / Dependencies
sourceLocaleLabelsfilter as defence for a code path that has since been removed; this PR retires the filter and simplifies the gate.✅ Author Checklist
feature/,bugfix/, orhotfix/+ 5–30 lowercase chars.env/example.envupdated if new environment variables were added (n/a)npm test) (run before requesting review)README.md/ docs updated if behaviour changed (n/a — internal UI gating only)👀 Reviewer Notes
Button'sdisabled={...}prop — the non-restart branch above it is untouched.rowCapReachedgate: it usessourceLocales?.length(hydrated state), same source of truth as the existingallSourcesMappedcheck, so both bounds move together. If ReduxsourceLocalehasn't hydrated yet,sourcesNotReadyalready disables the button — the cap doesn't need its own guard.sourceLocaleLabelsfilter's only caller wasfilledMappingCount, andfilledMappingCountitself is gone withhasIncompleteRow, so no dead code is left behind.