Dev

[vikrantraut-cstk]

🔗 Jira Ticket

Replace with your ticket link — required before requesting review.

MIGRATION-XXXX

---

📋 PR Type

• ✨ Feature
• 🐛 Bug Fix
• 🔥 Hotfix
• ♻️ Refactor
• 🧹 Chore / Dependency Update
• 📝 Documentation

---

📝 Description

What changed?

Why?

---

🧩 Affected Areas

• api — Node.js backend
• ui — React frontend
• upload-api — Upload API server
• docker / docker-compose
• CI / GitHub Actions workflows
• Environment variables / config
• Other:

---

🧪 How to Test

Expected result:

---

📸 Screenshots / Recordings

Before	After

---

🔗 Related PRs / Dependencies

---

✅ Author Checklist

Complete this before moving the PR out of Draft.

• Branch follows naming convention: feature/, bugfix/, or hotfix/ + 5–30 lowercase chars
• Jira ticket linked above
• Self-reviewed the diff — no debug logs, commented-out code, or TODOs left in
• .env / example.env updated if new environment variables were added
• No sensitive credentials or secrets committed
• Existing tests pass locally (npm test)
• New tests written (or not applicable — explain why)
• README.md / docs updated if behaviour changed
• Talisman pre-push scan passes (no secrets flagged)

---

👀 Reviewer Notes

---

Migration v2 · Docs · Issues

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[github-actions]

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type	Count (with fixes)	Without fixes	Threshold	Result
🔴 Critical Severity	0	0	10	✅ Passed
🟠 High Severity	15	378	25	✅ Passed
🟡 Medium Severity	18	11	500	✅ Passed
🔵 Low Severity	2	0	1000	✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity	Breaches (with fixes)	Breaches (no fixes)	SLA Threshold (with/no fixes)	Status
🔴 Critical	0	0	15 / 30 days	✅ Passed
🟠 High	0	0	30 / 120 days	✅ Passed
🟡 Medium	0	0	90 / 365 days	✅ Passed
🔵 Low	0	0	180 / 365 days	✅ Passed

ℹ️ 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:

• Critical without fixes: 0
• High without fixes: 378
• Medium without fixes: 11
• Low without fixes: 0

✅ BUILD PASSED - All security checks passed

[umesh-more-cstk]

Review — dev → pre-stage (20 commits, 34 files, +2192/-241)

Reviewed at head 51c9cbd. Findings below were each traced through the real code at that commit, not inferred from the diff. 2 blockers, 3 questions, 1 nit inline.

Substance is good — the delta-migration fixes (multi-iteration content-type classification, reference-uid resolution through __entryMapping__, deferred migrated_locales recording, the cs_failed.json double-join, the Array.isArray(entryId?.[id]) typo, and the JSON RTE reference-node shapes) are well-targeted and carry real unit tests. Most of what follows is edges around them.

Blockers

1. upload-api/src/config/index.json — a developer's local machine path (/Users/chetan.narkhede/…) and a hard-coded cmsType: "contentful" were committed over the placeholders. These are the env-var fallbacks in hydrate-config.ts, so any deploy without CMS_LOCAL_PATH/CONTAINER_PATH silently picks them up. Unrelated to everything else in this PR.
2. getNearestPriorUidMapper's hasData probe is narrower than its consumers — checks entry/assets while readers also use entryUid and entryByLocale, so an iteration that does have uid data gets skipped and a stale older map is returned instead.

Notes that don't anchor to a diff line

• PR hygiene (blocking for a promotion PR IMO). Title is Dev; the body is the untouched template — no Jira link (still MIGRATION-XXXX), no PR Type checked, no Affected Areas checked, no test steps, author checklist all unticked. I can't check "does the diff match the stated Affected Areas" because none are stated, and this diff spans api, ui, and upload-api plus a .gitignore and a config file. Worth filling in before this promotes to pre-stage — the config-file blocker above is exactly the kind of thing a self-review pass catches.

• api/src/services/contentful/jsonRTE.ts:98 — same bug class this PR is fixing, one line missed. The PR correctly threads lang/destination_stack_id through parseBlockquote and parseHeading1..6, but the table parser still has:

  .concat(parsers.get('tbody')?.(obj))   // ← no lang, no destination_stack_id

  Every other parsers.get(...) call in the file forwards both. A hyperlink inside a table body will hit the same degradation the PR describes for blockquotes — entry-hyperlink falling back to plain text, asset-hyperlink returning null and being dropped by .filter(Boolean). Line 98 isn't in the diff, so it can't be commented inline. One-line fix: parsers.get('tbody')?.(obj, lang, destination_stack_id).

• Scope, minor. .gitignore adds .claude/ alongside allien/ and aem_data_structure/; bootstrap@^5.3.6 is a new runtime dependency in ui/package.json. The bootstrap addition is justified in App.tsx (replacing the dead ui.contentstack.com CDN, which now 402s) and only the CSS is imported, so its @popperjs/core peer dep going uninstalled is fine — just calling out that both land in a PR with no stated areas.

• ui/src/services/api/migration.service.ts:436 builds …/${encodedSearchText}?${statusQuery} where statusQuery already starts with &, producing ?&status=failed. Express parses it correctly, so this is cosmetic only — mentioning it because the stray & reads like a bug at a glance.

Verified as correct (things I checked and am not flagging)

So the human gate doesn't have to re-derive these:

• retryFailedAsset's success check works — saveAsset does delete failedJSON[assets.sys.id] on a successful download (contentful.service.ts:746).
• packagePath matches: retryAssetDownload applies the same file_path.replace(/\/$/, '') that createAssets receives at migration.service.ts:999.
• Passing metadata as [] in the retry call is harmless — the array is only consumed by the commented-out ASSETS_METADATA_FILE write.
• assertResolvedPathUnderBase is imported in migration.service.ts (line 46), so recordDeltaMigratedLocales compiles.
• The new unconditional 'Entry Update Process Completed' marker does reach the client: safeDeltaMigrationLogPath, loggerPath, and runCli's transformePath all resolve to logs/<projectId>/<stackId>.log, which is the file the chokidar watcher is pointed at.
• newMigrationData.iteration exists on the redux shape (app.interface.ts:219), so isDeltaIteration is live rather than always-false.
• The UI does send locale in the updateEntryStatus body (entryMapper.tsx:452), so the new source-locale scoping is actually exercised.
• entryMapper.tsx's two fetch effects don't double-fire — React batches setSelectedLocale and setLocalesResolved in the same continuation, and the safety-net effect is guarded on !selectedLocale?.value.

Comment-only review by design — leaving the merge decision to a human reviewer.

---

Generated by Claude Code

[umesh-more-cstk]

blocker: A developer's local machine path has been committed here, and cmsType (line 7) was flipped from the "cmsType" placeholder to a hard-coded "contentful". Both look like an accidental commit from local testing (came in with 35a4858, the asset-retry commit) — neither is related to any change in this PR.

This is not inert. upload-api/src/utils/hydrate-config.ts:82-85 uses pickEnvOrExisting(process.env.CMS_TYPE, config.cmsType) / process.env.CMS_LOCAL_PATH || process.env.CONTAINER_PATH — the committed JSON value is the fallback when the env var is unset, and the file is also imported directly by upload-api/src/services/aws/client.ts:2.

Failure scenario: deploy pre-stage without CMS_LOCAL_PATH/CONTAINER_PATH set. Previously the resolved value was the obvious-garbage placeholder your_local_cms_data_path, which fails loudly; now upload-api silently resolves its source path to /Users/chetan.narkhede/Desktop/migration-v2/asic-v1.json and every CMS type defaults to contentful regardless of what the user selected.

Suggested fix: revert this file to the placeholders.

  "cmsType": "cmsType",
  ...
  "localPath": "your_local_cms_data_path"

---

Generated by Claude Code

[umesh-more-cstk]

blocker: hasData only inspects entry and assets, but the consumers of the model this returns read three other shapes — so an iteration that does carry uid data gets classified as empty and skipped.

• getEntryUidMap (just below, ~line 2264-2280) resolves from d.entryUid or d.entry. A uid-mapper holding mappings only under entryUid passes hasData === false here.
• writePerLocaleEntryUidMapping (api/src/utils/uid-mapper.utils.ts:178) writes entryByLocale and its own docblock says it "Leaves the flat entry / assets maps untouched" — so an iteration with per-locale mappings but no flat map also reads as empty.

Failure scenario: iteration 3 on a project where iteration 2's uid-mapper.json has entryByLocale: { "en-in": {...} } and entry: {}. The loop skips iteration 2 and returns iteration 1's model instead. enrichEntriesWithUidMapper then resolves against a stale, older map — Map Entry shows -/wrong Contentstack UIDs for entries that iteration 2 actually migrated, and getAssetMapping's hasAnyUidData computes false, which disables the delta filter entirely (line ~2537).

Suggested fix — align the probe with what the readers actually consume:

const hasData =
  Object.keys(data?.entry ?? {}).length > 0 ||
  Object.keys(data?.entryUid ?? {}).length > 0 ||
  Object.keys(data?.entryByLocale ?? {}).length > 0 ||
  Object.keys(data?.assets ?? {}).length > 0;

---

Generated by Claude Code

[umesh-more-cstk]

question: The double-join fix is correct (writeFile at line 355-365 does path.join(dirPath, filename) itself), but is there an upgrade path for data directories written by the old code?

The pre-fix call was writeFile(path.join(assetsSave, ASSETS_FAILED_FILE), ASSETS_FAILED_FILE, ...), and writeFile starts with mkdir(dirPath, { recursive: true }) — so every project that ran asset creation before this commit has a directory named cs_failed.json sitting at <assetsSave>/cs_failed.json.

Traced against the new code, that stale directory silently disables the whole failed/retry feature on those projects:

1. Here: writeOneFile → fs.promises.writeFile('<assetsSave>/cs_failed.json', ...) on an existing directory throws EISDIR, which writeFile's catch swallows to a console.error (line 363). Nothing reaches customLogger, so it never appears in the user's migration log.
2. contentMapper.service.ts getAssetMapping (~line 2500): fs.existsSync(failedPath) returns true for the directory, readFileSync throws EISDIR, the bare catch {} sets failedAssets = {} — so every row reports status: 'ok' and failedCount: 0.

Net effect on an upgraded project: assets that failed to download are reported as Ready, the "N assets won't be migrated" banner never appears, and the Retry button is never rendered — with no error anywhere the user can see.

Suggested fix: remove the stale entry before writing, and surface the write failure rather than swallowing it:

const failedPath = path.join(assetsSave, ASSETS_FAILED_FILE);
if (fs.existsSync(failedPath) && (await fs.promises.stat(failedPath)).isDirectory()) {
  await fs.promises.rm(failedPath, { recursive: true, force: true });
}
await writeFile(assetsSave, ASSETS_FAILED_FILE, failedJSON);

(Also worth logging via customLogger in writeFile's catch so a future silent write failure is visible.) If the deployed data directories are recreated from scratch on every release, then only the swallowed-error half applies — happy to drop this if that's the case.

---

Generated by Claude Code

[umesh-more-cstk]

question: retryFailedAsset reaches filesystem sinks with unsanitized values, while the sibling function added in this same PR sanitizes the identical value.

contentMapper.service.ts getAssetMapping (~line 2495) carries an explicit comment for exactly this reason:

destination_stack_id is DB-stored, but Snyk's taint tracker still traces it back to the HTTP projectId param via the lowdb lookup above — sanitize it the same way the rest of this codebase does … before it reaches a readFileSync sink.

…and applies sanitizeStackId + assertResolvedPathUnderBase. But retryAssetDownload (contentMapper.service.ts ~2400-2430) passes projectData?.destination_stack_id and projectData?.legacy_cms?.file_path straight through, and here they land in path.join(DATA, destination_stack_id, ASSETS_DIR_NAME) (line 876) plus fs.promises.readFile(packagePath, 'utf8') (line 880) — the same lowdb-sourced values, reached from the same HTTP :projectId param, with no sanitizer in between.

Failure scenario: whether or not it's exploitable today (both values are DB-written, not taken from the request body), the asymmetry means Snyk SAST will flag this new sink the same way it flagged the one 60 lines above — so CI will likely reject it — and any future code path that lets destination_stack_id be set from user input escapes the data directory here with no guard.

Suggested fix: apply the same two-step guard in retryAssetDownload before delegating:

const safeStackId = sanitizeStackId(destinationStackId);
if (!safeStackId) {
  return { status: HTTP_CODES?.BAD_REQUEST, data: { message: 'Invalid destination stack id.' } };
}
// pass safeStackId to retryFailedAsset, and assertResolvedPathUnderBase(path.resolve(DATA), assetsSave) inside it

---

Generated by Claude Code

[umesh-more-cstk]

question: The comment above states "hasSource: false rows are always non-selectable", but I can't find anything enforcing that on the UI side.

Selectability is decided solely by ui/src/components/ContentMapper/assetMapper.utils.ts:25:

_canSelect: !!asset?.contentstackAssetUid,

hasSource isn't consulted anywhere in ui/src (only api/src/services/contentMapper.service.ts:2551 reads it, to derive status: 'missing').

Failure scenario: an asset migrated successfully in iteration 1 (so it has a contentstackAssetUid) is re-exported in iteration 2 with its file removed from the Contentful space, so extractAssets emits hasSource: false. The row renders the "No source" badge, but _canSelect is true, so the checkbox is live — the user can queue it for update and Save. getAssetMapping's new delta filter (contentMapper.service.ts ~2537) deliberately keeps missing rows visible even without a uid, which makes this combination easy to hit.

Suggested fix: fold hasSource/status into the selectability decision so the comment holds:

_canSelect: !!asset?.contentstackAssetUid && asset?.hasSource !== false && asset?.status !== 'missing',

If selecting a missing-source asset is in fact harmless (the update silently no-ops), then the comment here is the thing to correct instead.

---

Generated by Claude Code

[umesh-more-cstk]

nit: A successful retry flips the row to status: 'ok' but leaves failedCount/missingCount untouched, so the banner they drive goes stale.

Failure scenario: one failed asset, banner reads "1 asset won't be migrated because the source file is broken or missing." User clicks Retry, it succeeds, the badge turns to Ready — and the banner still says 1. It only corrects on the next fetch (page change, search, or status-filter change). If the user is currently filtered to Failed, the retried row also stays in the list showing "Ready", since the filter was applied server-side.

Suggested fix: decrement alongside the row update in the success branch:

setFailedCount((c) => Math.max(0, c - 1));

Or, if you'd rather keep one source of truth, re-run fetchAssets(searchText, { seedSelection: false }) after a successful retry — that also drops the row out of a failed-filtered view.

---

Generated by Claude Code