Skip to content

Bugfix/aem cmg 1110 #1146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shradha-nahar wants to merge 3 commits into
base: dev
Choose a base branch
from
+29 −5
Open

Bugfix/aem cmg 1110 #1146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
180317c
save button added for asset selection
ShradhaNahar Aug 20, 2026
516710f
fix: prevent uploaded file input from shrinking on invalid path valid…
ShradhaNahar Aug 20, 2026
7d36eb6
fix: skip duplicate AEM entries sharing the same source id
ShradhaNahar Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions api/src/services/aem.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,9 +1387,18 @@ const createEntry = async ({
? uidCorrector(`${parseData.title}_${parseData.templateType}`)
: uidCorrector(parseData.templateType);
}
const uid = modelId && !usedEntryUids.has(modelId)
? modelId
: uuidv4?.()?.replace?.(/-/g, '');
// A stable modelId already seen earlier in this same run means this file is a
// duplicate export of a page already processed (AEM can emit both a page's generic
// model and its template's structure/model definition as separate files sharing the
// same id — see CMG-1112). Skip it instead of minting a fresh random uid: a random
// 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.

@umesh-more-cstk umesh-more-cstk Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (modelId && usedEntryUids.has(modelId)) {

@umesh-more-cstk umesh-more-cstk Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

continue;

@umesh-more-cstk umesh-more-cstk Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
const uid = modelId || uuidv4?.()?.replace?.(/-/g, '');

@umesh-more-cstk umesh-more-cstk Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Derived ids. When parseData.id is absent, the key is uidCorrector(\${title}_${templateType}`), or uidCorrector(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 untitled content-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.

  2. 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 an id, 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

usedEntryUids.add(uid);
const title = getTitle(parseData);
const isEFragment = isExperienceFragment(parseData);
Expand Down
1 change: 1 addition & 0 deletions ui/src/components/ContentMapper/assetMapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ const AssetMapper = ({
const tableHeight = useMeasuredTableHeight(tableWrapperRef, [tableData?.length], {
panelSelector: '.TablePanel',
footerSelector: '.mapper-footer',
toolbarSelector: '.asset-mapper-toolbar',
});

// Single server-paginated fetch (same pattern as entryMapper's fetchEntries). The
Expand Down
14 changes: 13 additions & 1 deletion ui/src/components/ContentMapper/useMeasuredTableHeight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ export interface MeasuredTableHeightOptions {
panelSelector: string;
/** Selector for the Save footer, resolved within `wrapperRef`. */
footerSelector: string;
/**
* Selector for an extra chrome row above the table (e.g. the asset mapper's status-filter
* toolbar) that takes its own flex-flow height, resolved within `wrapperRef`. Omit when the
* mapper has no such row (e.g. the entry mapper, whose locale select is absolutely positioned
* and doesn't need reserving).
*/
toolbarSelector?: string;
}

// Fixed chrome fallbacks, used only until the real elements are mounted/measured.
Expand All @@ -42,7 +49,7 @@ const TOGGLE_SELECTOR = '.mapper-view-toggle';
export function useMeasuredTableHeight(
wrapperRef: RefObject<HTMLElement | null>,
deps: unknown[],
{ panelSelector, footerSelector }: MeasuredTableHeightOptions,
{ panelSelector, footerSelector, toolbarSelector }: MeasuredTableHeightOptions,
): number {
// Pre-measure guess: same model as measure() (box fallback − reserve), clamped to the floor
// so the one frame react-window renders before the effect runs never gets a negative height.
Expand All @@ -61,13 +68,17 @@ export function useMeasuredTableHeight(
const toggle = box?.querySelector(TOGGLE_SELECTOR) as HTMLElement | null;
const panel = wrapper.querySelector(panelSelector) as HTMLElement | null;
const footer = wrapper.querySelector(footerSelector) as HTMLElement | null;
const toolbar = toolbarSelector
? (wrapper.querySelector(toolbarSelector) as HTMLElement | null)
: null;

if (import.meta.env.DEV) {
// A rename/markup change in venus would drop us to the magic constants and quietly
// regress the layout — warn loudly in dev so it's caught rather than shipped.
if (!box) console.warn(`useMeasuredTableHeight: "${BOX_SELECTOR}" not found — falling back.`);
if (!panel) console.warn(`useMeasuredTableHeight: "${panelSelector}" not found — using ${PANEL_FALLBACK}px fallback.`);
if (!footer) console.warn(`useMeasuredTableHeight: "${footerSelector}" not found — using ${FOOTER_FALLBACK}px fallback.`);
if (toolbarSelector && !toolbar) console.warn(`useMeasuredTableHeight: "${toolbarSelector}" not found — not reserving space for it.`);
}

// `||` not `??`: a momentarily 0-height box (measured before layout settles) should
Expand All @@ -77,6 +88,7 @@ export function useMeasuredTableHeight(
(toggle?.offsetHeight ?? 0) +
(panel?.offsetHeight ?? PANEL_FALLBACK) +
(footer?.offsetHeight ?? FOOTER_FALLBACK) +
(toolbar?.offsetHeight ?? 0) +
PAGINATION_AND_BUFFER;
// Clamp rather than skip: at extreme zoom `avail` can dip low, but keeping the previous
// (possibly large) value would re-expose the overflow this hook exists to prevent.
Expand Down
4 changes: 3 additions & 1 deletion ui/src/components/LegacyCms/legacyCms.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@
background-color: $color-base-white-5;
flex-direction: column;
justify-content: center;
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;

@umesh-more-cstk umesh-more-cstk Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

margin-left: 20px !important;
border: 1px solid $color-brand-fail-base;
border-radius: var(--TermCount, 5px);
Expand Down
Loading