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
…p, error state width) CMG-1112: exclude AEM template structure/schema definition files outright (detected via repo:path) instead of letting them race a real page for the same derived id — removes the directory-order-dependent collision winner. Key the same-run dedup on modelId + locale instead of bare modelId so distinct locale variants of one page are no longer wrongly skipped, and log the skip via customLogger like the other skip path. CMG-1113: give .error-container the same padding as .validation-container instead of a child left-margin, so both states render the same width; drop the duplicate margin-left declaration.
🔒 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. |
4 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.