Bugfix/aem cmg 1110 - #1146
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
🔒 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:
Consider reviewing these vulnerabilities when fixes become available. |
umesh-more-cstk
left a comment
umesh-more-cstk
left a comment
There was a problem hiding this comment.
Review — head 7d36eb60
Scope: all 4 files (1 api, 3 ui). Not a draft, not docs/lockfile-only. The diff matches the declared Affected Areas — api for CMG-1112, ui for CMG-1110/1113 — no unrelated churn.
2 blockers · 0 questions · 3 nits. Both blockers are in the CMG-1112 change; the two CSS/layout fixes verified clean.
Blockers (CMG-1112, api/src/services/aem.service.ts)
- L1398 — collision winner is filesystem-order dependent.
entriesDiris walked withfs-readdir-recursive(unsortedreaddirSyncorder), so whennotitle.model.jsonandpage-content-full-width.template.jsoncollide, whichever the FS yields first wins. If that's the.template.json, the real page content is skipped and the migration keeps a schema node instead — and it reproduces differently per machine. Previously both were written, so nothing was lost; the skip makes the arbitrary choice consequential. - L1401 — the skip key is too coarse, and locale-blind. On the derived path (no
parseData.id, the normal case for XF andcontent-pageper the comment at 1380-1384) the key degrades totitle_templateTypeor baretemplateType, so N untitledcontent-pagefiles collapse to 1 entry with N−1 silently dropped. Separately, per-locale exports of one page collide on the same key, and the non-first locale is now skipped entirely rather than written to its own locale bucket — that case wants the same uid and both entries, not a skip. Suggest keying on${modelId}::${mappedLocale}(needs the locale computation hoisted above the check).
Nits
- L1399 — the only skip in this loop that logs nothing; every other one goes through
customLogger. Given the above, this is the drop most likely to need diagnosing. - L1397 — the comment cites
extractEntries/entry_mapper(anduid-mapperat L1375), none of which exist in this repo;upload-api/migration-aemhas no entries lib. Worth clarifying if these mean CLI-side mappers. legacyCms.scssL88 —stretchis right, but.error-containerstill lacks.validation-container'spadding: 10px 15pxand instead uses a childmargin-left: 10px, so the row is flush to the right border and the two states still differ in width.
Verified correct — no findings
- CMG-1110.
.asset-mapper-toolbaris a direct child of thetableWrapperRefelement (assetMapper.tsx:439-440), so the scopedwrapper.querySelectorresolves it; it's a sibling of the venus-rendered.TablePanel, so nothing is double-reserved againstpanelSelector; and its SCSS (index.scss:988) isflex: 0 0 autowith padding + border and no margin, sooffsetHeightcaptures the full flow height it occupies. MakingtoolbarSelectoroptional leaves entryMapper's call site untouched, and the dev-only warning matches the existing pattern for the other selectors. - CMG-1113.
align-items: stretchrestores the flex default and matches.validation-container, which sets noalign-itemsat all. Behaviour-free otherwise.
One note on the checklist rather than the code: "New tests written (or not applicable)" is ticked with no explanation, and the CMG-1112 change is a behavioural one. There's no existing suite for api/src/services/aem.service.ts (only upload-api/tests/** covers AEM), so a first test here is a bigger lift than usual — but the same-run-collision path is exactly the kind of thing worth locking down once the key/tie-break questions above are settled.
Generated by Claude Code
| // untracked random uid (another duplicate) on every subsequent delta iteration, | ||
| // since it can never match anything recorded in entry_mapper. This mirrors | ||
| // extractEntries's collision policy in upload-api's migration-aem. | ||
| if (modelId && usedEntryUids.has(modelId)) { |
There was a problem hiding this comment.
blocker: the winner of a collision is whatever the filesystem happens to yield first, so this can keep the template/structure node and drop the real page.
entriesDir is walked with fs-readdir-recursive (import at line 7), i.e. fs.readdirSync order — not sorted, not semantically meaningful. Before this change both colliding files were written (one under modelId, one under a random uid), so nothing was lost; now exactly one survives and which one is arbitrary.
Failure scenario, using this PR's own example: notitle.model.json (the actual page content) and page-content-full-width.template.json (the template's structure definition) share the same id. If the .template.json is yielded first it claims modelId, and the real page — the one carrying [':items'].root content consumed at line 1412 — is continued away. The migration then produces one entry whose body is a schema node and silently loses the page. Because it depends on directory order, the same source data can migrate correctly on one machine and wrongly on another, which makes it painful to reproduce. The Reviewer Notes already acknowledge that structure nodes probably shouldn't become entries at all, which is exactly the file this can pick.
Suggested fix: make the tie-break explicit rather than incidental — prefer the file that is actual content (e.g. the one with parseData?.[':items']?.root, or skip structure/template exports outright) and only fall back to first-wins when both look like content, with the file list sorted so that fallback is at least reproducible.
Generated by Claude Code
| if (modelId && usedEntryUids.has(modelId)) { | ||
| continue; | ||
| } | ||
| const uid = modelId || uuidv4?.()?.replace?.(/-/g, ''); |
There was a problem hiding this comment.
blocker: modelId isn't unique enough to be a skip key on the derived path, and it ignores locale — both turn "duplicate" into silent content loss.
Two ways distinct files collide on modelId without being duplicate exports of the same page:
-
Derived ids. When
parseData.idis absent, the key isuidCorrector(\${title}_${templateType}`), oruidCorrector(templateType)alone when there's no title (lines 1385-1389). Per the comment right above it, that's the *normal* path for experience fragments andcontent-pages, not an edge case. So two untitledcontent-pagefiles both key tocontent_pageand only the first is migrated: N such files → 1 entry, N−1 dropped.getTitle(line 1307) also falls back totemplateType`, so these files aren't distinguishable by title downstream either. Previously all N were migrated (N−1 with random uids) — bad for delta tracking, but not lost. -
Locale. A page exported per-locale is one file per locale (
getCurrentLocale, line 258), bucketed by locale at line 1435 and written to<ct>/<locale>/<locale>.json. Two locale variants of one page that share anid, or share an untranslated title + templateType, collide here — and the non-first locale is now skipped entirely, so that locale's entry never gets written at all. This case is the opposite of a duplicate: it should keep the entry and reuse the same uid, which is exactly how Contentstack localizes.
Suggested fix: key the set on ${modelId}::${mappedLocale} so locale variants survive and legitimately share one uid — that means hoisting the locale/mappedLocale computation (lines 1410-1411) above this check. Then treat a same-locale repeat as a duplicate only when the file really is a redundant export (see the tie-break note on line 1398) rather than whenever the coarse derived key happens to repeat.
Generated by Claude Code
| // since it can never match anything recorded in entry_mapper. This mirrors | ||
| // extractEntries's collision policy in upload-api's migration-aem. | ||
| if (modelId && usedEntryUids.has(modelId)) { | ||
| continue; |
There was a problem hiding this comment.
nit: this is the only skip in the loop that leaves no trace. The else at lines 1437-1448 logs Skipped entry from "<file>": <reason> through customLogger, and the DAM skip at line 1369 is structural/expected. This branch drops a file's content entirely, so an operator investigating a missing entry has nothing to go on — and given the two blockers above, that's the case most likely to need investigating.
await customLogger(
projectId,
destinationStackId,
'warn',
getLogMessage(srcFunc, `Skipped duplicate entry from "${fileName}": uid "${modelId}" already used in this run.`, {})
);
continue;Generated by Claude Code
| // uid here would create a second, permanent duplicate entry that mints yet another | ||
| // untracked random uid (another duplicate) on every subsequent delta iteration, | ||
| // since it can never match anything recorded in entry_mapper. This mirrors | ||
| // extractEntries's collision policy in upload-api's migration-aem. |
There was a problem hiding this comment.
nit: the symbols this comment leans on don't exist in this repo, so the reasoning can't be checked. extractEntries and entry_mapper return no hits anywhere in the tree, and upload-api/migration-aem has no entries lib at all (only contentType, locales, validate) — same for uid-mapper referenced by the older comment at line 1375. If these mean the Contentstack CLI's import mapper files rather than code in this repo, worth saying so explicitly; as written, "this mirrors extractEntries's collision policy in upload-api's migration-aem" points a future reader at something they can't find, and the PR description repeats it.
Generated by Claude Code
| align-items: flex-start; | ||
| // Stretch (not flex-start) so the path row keeps the container's full width — flex-start | ||
| // let it shrink-to-fit for short/invalid paths, visibly narrowing the input (CMG-1113). | ||
| align-items: stretch; |
There was a problem hiding this comment.
nit: this correctly removes the shrink-to-fit (stretch is the flex default), but it doesn't quite reach the parity with .validation-container that the change is aiming for.
.validation-container (line 65) has padding: 10px 15px. .error-container has no padding and instead relies on .error-container > * { margin-left: 10px } (lines 96-97). So now that children stretch, the path row sits 10px in from the left and flush against the right border — still a different width from the neutral state, and asymmetric within the error state itself. Adding padding: 10px 15px here (and dropping the child margin-left) would line the two states up exactly.
Also pre-existing while you're in this block: margin-left: 20px !important is declared twice, lines 89 and 94.
Generated by Claude Code
3 participants
🔗 Jira Ticket
CMG-1110
CMG-1112
CMG-1113
📋 PR Type
📝 Description
CMG-1110: Fixed the Save footer on the "Map Entry Assets" screen being visually clipped out of view. useMeasuredTableHeight now also reserves space for .asset-mapper-toolbar (the status-filter row unique to the asset mapper), which it previously didn't account for — causing the table body to render taller than available space and push the Save button below the overflow: hidden wrapper's visible bounds.
CMG-1112: createEntry (AEM connector) now skips a file when its derived modelId was already used earlier in the same migration run, instead of minting it a fresh random uid.
CMG-1113: .error-container's align-items: flex-start (shown when an invalid path is validated) is now align-items: stretch, matching the neutral .validation-container state.
What changed?
Why?
CMG-1110: The Save button (and its handleSaveAssets → updateAssetMapper flow, wired through to isUpdate and consumed at actual-migration time to decide asset replace-vs-reuse) was already implemented and functional — it just wasn't visible due to a layout height-measurement bug specific to the asset mapper's extra toolbar row.
CMG-1112: AEM can export a page template's structure/model definition into two separate files sharing the same source id (e.g. notitle.model.json and page-content-full-width.template.json). createEntry previously minted a new random uid for the second occurrence rather than recognizing it as a duplicate — creating an untracked entry that could never be matched by the delta "already migrated" check, so a new untracked duplicate was created on every iteration (2 entries after iteration 1, +1 every iteration after). This now matches the collision policy extractEntries (upload-api/migration-aem) already uses for entry_mapper tracking.
CMG-1113: flex-start let the file-path row shrink-to-fit around short/invalid path text instead of filling the container's fixed width, unlike the neutral validation state.
🧩 Affected Areas
api— Node.js backend (CMG-1112)ui— React frontend (CMG-1110, CMG-1113)🧪 How to Test
CMG-1110 - Start an AEM migration project, reach Step 4 "Map Entry", switch to the Assets tab (with at least one matched/updatable asset). Confirm the Save button is visible in the footer and clickable.
CMG-1112 - Run an AEM migration against source data containing a page template whose structure/model definition is exported into two files sharing the same id (e.g. sample templates data: notitle.model.json + page-content-full-width.template.json).Run iteration 1, then iteration 2 (delta) against the same or copied source data.
📸 Screenshots / Recordings
✅ Author Checklist
feature/,bugfix/, orhotfix/+ 5–30 lowercase chars.env/example.envupdated if new environment variables were addednpm test)README.md/ docs updated if behaviour changed👀 Reviewer Notes
CMG-1112's fix only activates on a same-run modelId repeat — every normal, non-colliding AEM page is unaffected.
CMG-1112 does not address whether AEM template-structure/schema nodes should be migrated as entries at all (they aren't real content) — flagged as a possible larger follow-up, out of scope here.
CMG-1110 and CMG-1113 are both CSS/layout-only fixes with no logic changes.