Skip to content

MWPW-206375: fix cascading publish/revert for new cards (PR #1039 regression) - #1207

Open
seanchoi0 wants to merge 7 commits into
mainfrom
MWPW-206375
Open

MWPW-206375: fix cascading publish/revert for new cards (PR #1039 regression)#1207
seanchoi0 wants to merge 7 commits into
mainfrom
MWPW-206375

Conversation

@seanchoi0

@seanchoi0 seanchoi0 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Resolves https://jira.corp.adobe.com/browse/MWPW-206375

Problems

Two regressions introduced by PR #1039, plus one additional edge case uncovered during review:

1. New cards — "No snapshot found" on revert / "Publish failed" on save

`recordSnapshot()` returned a failure for any fragment with no version history (new cards that were never published). This caused:

  • `save-snapshot-action` to write `SAVE_SNAPSHOT: No non-translation version found` to `lastError` → "Publish failed" banner on save
  • Revert to fail with "No snapshot found" because snapshot entries were empty

2. Cascading publish broken — cards & variations not published or reverted

In the `hasValidPreRecordedSnapshot` branch added by PR #1039:

  • `createSnapshot` was called without `includeCards`/`includeVariations` → sub-collections, cards, and variations were not traversed
  • `expandedPaths` was not captured → `publishPaths` fell back to top-level only
  • `snapshotEntries` contained only top-level fragments → revert only worked for the top-level collection

3. Silent no-op revert for published fragments with no prior non-translation version

A fragment can be live (wasPublished: true) but have versionId: null if all its CF versions were created by the translation system. In this case `revertSnapshot()` would skip both `restoreVersion` and `unpublishFragment` and return `null` (success), silently doing nothing.

Fixes

Fix 1 — `snapshot.js`

`recordSnapshot()`: Record new cards with `versionId: null` instead of failing. Reverting will unpublish them (restoring pre-publish state) rather than trying to restore a non-existent version.

`revertSnapshot()`: Skip `restoreVersion` when `versionId` is `null`; still call `unpublishFragment` since `wasPublished: false`. For the edge case where `versionId: null` and `wasPublished: true` — return `{ skipped }` instead of silently succeeding.

`hasValidPreRecordedSnapshot()` (`bulk-publish-worker.js`): Accept `null` versionId by checking `'versionId' in parsed` instead of `parsed.versionId &&`.

Fix 2 — `bulk-publish-worker.js`

Both `hasValidPreRecordedSnapshot` and fallback branches:

  • Pass `includeCards`/`includeVariations` to `createSnapshot` so cascaded fragments are traversed
  • Capture `expandedPaths` for the correct cascade publish paths
  • `mergeCascadedEntries()`: merge pre-recorded (green/null) versionIds for top-level fragments with freshly created Pre-bulk-publish versionIds for cascaded fragments → revert now covers all published items

Behavior

Scenario Before After
New card saved to project ❌ "Publish failed" banner immediately ✅ No error
New card bulk publish → revert ❌ "No snapshot found" ✅ Unpublished
Cascading publish (includeCards + includeVariations) ❌ Only top-level published ✅ All cards & variations published
Cascading revert ❌ Only top-level reverted ✅ All published items reverted
Published card with only translation versions → revert ❌ Silent no-op (reported as success) ✅ Skipped (reported honestly)
Existing card (has versions), revert ✅ Restored to prior version ✅ Unchanged

@aem-code-sync

aem-code-sync Bot commented Sep 3, 2026

Copy link
Copy Markdown

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-sync branch
Commits

New cards (never published) had no CF version history, causing
recordSnapshot to fail and revert to show "No snapshot found".

- recordSnapshot: record versionId: null for cards with no prior version
- revertSnapshot: skip restoreVersion when versionId is null; unpublish instead
- hasValidPreRecordedSnapshot: accept null versionId ('versionId' in entry)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

SNOW Change Request Transaction ID [IO]: 0000019c1ef37582-20e0d556

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

SNOW Change Request Transaction ID [STUDIO]: 0000019c1eec85a3-20bbe383

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.71%. Comparing base (2f2e131) to head (7731361).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1207      +/-   ##
==========================================
- Coverage   90.71%   90.71%   -0.01%     
==========================================
  Files         318      318              
  Lines      101712   101754      +42     
==========================================
+ Hits        92271    92309      +38     
- Misses       9441     9445       +4     
Files with missing lines Coverage Δ
io/studio/src/bulk-publish/bulk-publish-worker.js 100.00% <100.00%> (ø)
io/studio/src/bulk-publish/snapshot.js 97.90% <100.00%> (+0.04%) ⬆️

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f2e131...7731361. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

PR #1039 introduced a regression in the hasValidPreRecordedSnapshot
branch: createSnapshot was called without includeCards/includeVariations
and expandedPaths was not captured, so only top-level fragments were
published and revert only covered top-level items.

- Pass includeCards/includeVariations to createSnapshot in both branches
- Capture expandedPaths for cascaded publish paths
- mergeCascadedEntries(): merge pre-recorded (green) entries with newly
  snapshotted cascaded fragments so revert covers all published items

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@seanchoi0 seanchoi0 changed the title MWPW-206375: fix revert for new cards with no prior version history MWPW-206375: fix cascading publish/revert for new cards (PR #1039 regression) Sep 3, 2026
…CascadedEntries

- formatSnapshotError(prefix, failures): eliminates duplicated CREATE_SNAPSHOT
  and SAVE_SNAPSHOT error string construction across both snapshot branches
- mergeCascadedEntries: add early exit when primaryEntries is empty

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@Axelcureno Axelcureno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One blocker: recordSnapshot can now emit versionId: null with wasPublished: true (your own test, snapshot.test.js:1025). Your comment says revert will "unpublish them instead", but wasPublished: true blocks that unpublish too, so revert does nothing, reports no failure, and Studio toasts success. Route it to the existing skipped channel?

Minor: worker.test.js:490 would pass pre-fix; it doesn't assert the merge.

Merge dedupe is load-bearing: deserializeEntries uses Object.fromEntries, so cascaded would otherwise win.

…ished

versionId: null + wasPublished: true means the fragment was live before
bulk publish but has no non-translation version to restore to. Silently
returning success was misleading; route to the skipped channel instead.

Also strengthens the pending-resume worker test to verify publishComplete
is stripped from final snapshots.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@Axelcureno Axelcureno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocker's addressed. Deleted the guard locally and the new test fails, so it's a genuine regression test; both merge tests fail pre-fix. 156 passing.

Two non-blocking follow-ups:

  • skipped never reaches the UI. revert-action.js drops it from snapshots and returns REVERTED, and bulk-publish-store.js:76 only reads status, so Studio says "Reverted" while the card stays live.
  • deserializeEntries takes createdAt from parsed[0], so merged snapshots use the earlier timestamp. Check Modifications may flag every cascaded card.

Approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants