Skip to content

Mwpw-199318 Automatic highlighting of unpublished content on the preview page (V1) - #6440

Open
skholkhojaev wants to merge 68 commits into
stagefrom
MWPW-199318-preflight-content-diff
Open

skholkhojaev wants to merge 68 commits into
stagefrom
MWPW-199318-preflight-content-diff

Conversation

@skholkhojaev

@skholkhojaev skholkhojaev commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

When you're looking at a preview page, this automatically highlights the content that hasn't been published yet, this allows the authors to see exactly what's new or changed compared to what's live right now.

  • New content gets a green frame with a "NEW" tag.
  • Changed content gets an orange frame with a "CHANGED" tag.
  • Highlights appear on their own when the preview page loads — no button to press, no panel to open.
  • There's an on-page "Hide" button to clear them when you want a clean view.
  • It runs only on preview (.aem.page) never on stage or live, so other people always see the page the way it'll actually look once published.
  • Unpublished changes inside same-origin fragments now highlight on the preview page too —

How to test

Two preview pages on this branch:

Load either page, wait a second, and the highlights appear on their own. Click Hide to clear them and click Show and the highlights should appear again.

Resolves: MWPW-199318

Test URLs:

skholkhojaev and others added 30 commits August 5, 2026 16:22
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wires the actual compare view into the Content Diff panel: the panel now
fetches its own preview/live .plain.html via fetchVersions, diffs it with
diffContent/diffMetadata, renders both panes through renderPane, and applies
highlights via applyHighlights. Adds Content/Metadata toggle tabs, a
jump-to-change list (badge + path, click scrolls both panes), a metadata
key/value table, and a "Live: X · Preview: Y" last-modified header.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a highlight on/off toggle (persisted to localStorage, re-read on
mount) and optional synchronized pane scrolling to the compare panel.
Splits genuine fetch/parse failures into a distinct error+retry state
from the existing "no changes" empty state.

Also folds in review minors from the change-list/tabs task: parses
each side's .plain.html once instead of twice, fixes invalid
aria-selected on role="tabpanel" (replaced with hidden), fixes
change-path ellipsis truncation and adds a title attribute, and swaps
literal padding/font-weight values for existing design tokens.
DiffPanel now defers its fetch + two loadArea decorations until the
Content Diff tab is actually selected (guarded to fire once), instead
of running on every preflight open regardless of which tab is viewed.
preflight.js threads the tab's selected state through for this one
panel only; the shared TabPanel mounting mechanism is untouched.

Also renders each compare pane as a div instead of <main> (avoids
duplicate landmarks) and strips any ids left by decoration (avoids
duplicate ids across the two rendered page copies), and adds a
trailing period to the Content Diff tab description for consistency
with its siblings.
The Content Diff tab rendered two full, decorated copies of the live page
via loadArea. On heavy commerce pages, the decorated panes' block JS
(merch, georouting, deep-links) triggered a hashchange that modal.js's
global listener treated as a close signal, self-closing the whole
preflight modal. Drop the loadArea-based pane rendering (and its
scroll-sync/highlight wiring) and keep only the cheap fetch + diff engine
calls, starting the pivot to in-place highlighting on the real page.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… load

The live .plain.html fetch previously fell back to an empty document on any
non-200/error/network-failure, so a 404'd live page (e.g. cc/photoshop) diffed
every preview node as "New" with "Live: Unknown". fetchVersions now classifies
the live fetch outcome (ok/missing/error) instead of collapsing it to null.

checks/diff.js and panels/diff.js use that classification plus the admin
publish status to pick between three outcomes: a normal diff when live loads,
a "new page" state when the page is confirmed never-published (safe to label
preview content as new), and an explicit "couldn't load the live version"
state (with retry) when live should exist but didn't load or its publish
status is unknown — never silently fabricating a diff in that case.

Also replaces the change list's raw xpath labels with human-readable ones
(Image, Heading, Link, Text, or the lowercased tag) derived from the node's
tag/text, keeping the raw path as the row's title/tooltip.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Outlines content-diff changes directly on the real, already-decorated
preview page instead of only listing them. Milo decoration reshapes the
DOM (img->picture, block wrappers) so a raw pre-decoration xpath can't be
looked up 1:1, so diff-onpage.js walks it tolerantly: match by tag +
same-tag occurrence index at each level (matching getXPath's own rule),
falling back to an any-depth descendant search when decoration inserted
a wrapper level. Content inside a decorated block always resolves to the
block container rather than a literal leaf, since block JS can rebuild
its own internal DOM; default section content stays leaf-precise. Only
returns null (skip, never guess) when not even the outermost segment
resolves.

Added changes get outlined green, modified amber; removed changes never
rendered on the preview page in the first place, so they stay list-only.
Each change row is now a button that jumps to its on-page element,
mirroring panels/assets.js's goToAsset: close the preflight modal, flash
the element, and show a "Back to Preflight" popover that reopens it via
the sidekick's custom:preflight event. The existing highlight toggle now
also drives the on-page outlines, not just the modal's dead change-list
styling.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
diffContent.js now collects Milo blocks (direct-child div[class] of a
section) as opaque units alongside the existing leaf content tags, so
an added/removed/changed block actually shows up in the diff. Adds a
Jaccard similarity gate before pairing a same-slot removed+added leaf
into "modified" (blocks match by blockName instead), which also fixes
a phantom-wrapper bug where an empty <p> around a new <img> was
misclassifying a real removed paragraph as "Changed". diff-onpage.js's
resolver now only climbs to the containing block for block-kind
changes, so a text edit outlines the paragraph/image itself instead of
the whole default-content-wrapper. diff.js labels block changes as
"New/Changed/Removed block: <TitleCased name>".

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an expandable before/after detail per content-diff change row,
rendering cloned live/preview snippets (text, image, or lightweight
block text+images) without loadArea or innerHTML from network content.
Also rework the on-page highlight from an in-flow outline/ribbon into
a dedicated overlay element so it always paints above a block's own
layered content (e.g. marquee background/video), fixing highlights
getting buried underneath real blocks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The overlay used a max-int z-index to beat a block's own layered content,
but that also let it paint above the preflight modal (--modal-z-index:
100000), since a plain number can't distinguish "above this block" from
"above everything on the page." Isolating the host (isolation: isolate)
confines the comparison to the host's own stacking context, so a modest
z-index (5, in line with .asset-meta's ~2-3) reliably wins locally while
staying strictly below the modal, which lives in a separate context on
body.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@zagi25

zagi25 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Highlight is applied even though sidekick is disabled/paused, is this expected ?

Screenshot 2026-08-11 at 15 52 15

@skholkhojaev

Copy link
Copy Markdown
Contributor Author

@zagi25 no that was not intended, it should work properly now thankyou!

@mokimo

mokimo commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

I'd recommend two things, outside of the code:

  • Take this to someone from GWP you can actually show this to, e.g. the person who raised the ticket to give it a test spin from a UX perspective
  • Potentially provide an opt-out via the preflight in a settings tab? Personally, I wouldn't want to see or have to click the hide-button each time and have it sticky on the page all the time unless I have control over the feature

@skholkhojaev
skholkhojaev requested a review from mokimo August 18, 2026 11:59
Comment thread libs/utils/preflight-notification.js Outdated
Comment thread libs/utils/preflight-notification.js Outdated
Comment thread libs/scripts/delayed.js
const target = inPicture ? el.parentElement : el;
if (target.tagName === 'PICTURE' || VOID_HOST_TAGS.has(target.tagName)) {
const wrapper = createTag('span', { class: `${WRAP_CLASS} ${ISOLATE_CLASS}` });
target.replaceWith(wrapper);

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.

This mutates the live DOM the author is viewing — wrapping <img>/<picture>/void elements in an inline-block span (+ position:relative/isolation:isolate on hosts). A full-width image wrapped in an inline-block span can shift layout. Please spot-check full-width media (hero-marquee, brick) on a real preview page before merge.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tested this on a real page with the actual CSS: a full-width image stays the exact same size and place when highlighted (measured 1440px → 1440px, no movement).

Comment thread libs/blocks/preflight/checks/diff/diffContent.js Outdated
return null;
});

const pMod = status?.preview?.lastModified ? Date.parse(status.preview.lastModified) : NaN;

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.

Date.parse(lastModified) assumes the admin status field is an RFC-1123 string (matches the API today and your test mocks). If it ever returns an epoch number, Date.parseNaN and the skip optimization silently disables (degrades safe, but silently). A small guard/comment would make that intentional.

Comment thread libs/blocks/preflight/checks/diff/fetchVersions.js Outdated
Comment thread libs/blocks/preflight/checks/adminStatus.js Outdated
@skholkhojaev
skholkhojaev requested a review from mokimo August 26, 2026 07:44

@narcis-radu narcis-radu left a comment

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.

@skholkhojaev - work is great. Given the size and complexity of the code, it would be great to confirm with GWP that the enhancements is exactly what they requested. My suggestion is to create a video recording to walk them through and then ask for feedback in the JIRA ticket.

Comment thread libs/blocks/preflight/checks/diff/diffContent.js
Comment thread libs/scripts/delayed.js Outdated
const run = async () => {
const { default: showPreflightNotification } = await import('../utils/preflight-notification.js');
const { default: showPreflightNotification, autoHighlightUnpublished } = await import('../utils/preflight-notification.js');
if (isLocal || hostname.endsWith('.aem.page')) autoHighlightUnpublished();

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.

Are there any options to toggle off the highlight? If there is a lot of content that's new or unpublished, seeing too many highlights might become problematic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes that is already taken care of!
If you open any test page you should see a Toggle on/off on the bottom left of the screen

@mokimo

mokimo commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

👍 from me — CI is green, and the earlier Blocker (style bleed) I flagged looks resolved: autoHighlightUnpublished now loads the scoped panels/diff-onpage.css instead of the whole preflight.css, so the unscoped .tooltip (preflight.css:453) and friends no longer leak onto the live preview page from this path. The cleanup/popstate wiring landed too. Didn't re-chase the smaller open threads (diffContent overlapping units, fetchVersions' Date.parse/homepage-path edge cases) since none of those block — they degrade safe.

Two things before merge:

1. Please get explicit stakeholder sign-off that this is the intended behavior, not just an engineering check. This now auto-runs on every preview load with zero author action — no button, no opt-in — and mutates the live DOM (wraps images/pictures in extra spans) to draw the overlays. That's a meaningfully more intrusive default than the old "open preflight to see issues" model. Worth a explicit "yes, always-on is what we want" from whoever owns the authoring experience (design/content team), especially since a heavy-diff page will overlay a lot of frames.

2. libs/utils/preflight-notification.js is doing two unrelated jobs and should split. The file's existing code (sidekick previewed/published listeners, notification banner create/dismiss, show()) is about the quality-check notification. This PR bolts on autoHighlightUnpublished — a ~40-line orchestration (resolve URL → fetch versions → compute diff → wait for pending fragments → collect fragment changes → merge → bail if empty → load styles/tokens → dynamic-import the renderer → wire popstate cleanup) for a completely different feature (content-diff auto-highlight). It already delegates the actual mechanics well to checks/diff/* and panels/diff-onpage.js — good instinct there — but the orchestration itself doesn't belong in a file named (and otherwise entirely about) the notification banner.

Suggest moving autoHighlightUnpublished (+ PENDING_FRAGMENT_WAIT_MS) into its own module under checks/diff/ (e.g. checks/diff/index.js), and have delayed.js import it from there directly. preflight-notification.js shouldn't need to know this feature exists at all — right now it's just a shared launch point because both happen to fire from delayed.js.

…t-content-diff

# Conflicts:
#	libs/blocks/preflight/panels/general.js
Comment thread libs/blocks/preflight/preflight.css
Comment thread libs/blocks/preflight/preflight.css
Comment thread libs/blocks/preflight/preflight.css Outdated
Comment thread libs/blocks/preflight/preflight.css Outdated
Comment thread libs/blocks/preflight/preflight.css
Comment thread libs/scripts/delayed.js Outdated
const { default: autoHighlightUnpublished } = await import('../blocks/preflight/checks/diff/autoHighlight.js');
autoHighlightUnpublished();
}
if (isLocal) return;

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.

the logic is a bit confusing; I know we want to return early, but maybe it would make more sense to separate as

if (isLocal || isPreview) {
  // do something
}

if (!isLocal) {
  // do something additional
}

Comment thread libs/scripts/delayed.js
Comment thread libs/blocks/preflight/checks/diff/fetchVersions.js Outdated
Comment thread libs/blocks/preflight/checks/diff/autoHighlight.js Outdated
Comment thread libs/blocks/preflight/checks/adminStatus.js Outdated
The -webkit- prefix removals and blank-line additions in preflight.css were
unrelated scope creep (stray autofix) and dropped Safari mask support the rest
of Milo keeps. Restore the file to stage. Content-diff styles live in
panels/diff-onpage.css.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
if (!root) return;
try {
const url = new URL(window.location.href);
const page = computeDiff(await fetchVersions(url)).content;

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.

what happens if fetchVersions fails? We should check the state before accessing content

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fetchVersions already handles normal fetch failures, and when there is no page diff content is just undefined, which the optional checks below handle.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

do you still want me to add an extra state check?

let bestScore = 0.3;
root.querySelectorAll(`.${CSS.escape(blockName)}`).forEach((block) => {
const score = textSimilarity(normalizeText(block.textContent), expectedText);
if (score > bestScore) { best = block; bestScore = score; }

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.

in diffContent we have >=, here we have >. Shouldn't that be consistent? Also, would it be possible to reuse the value instead of defining it in multiple places.

Comment thread libs/scripts/delayed.js Outdated
await showPreflightNotification();
if (isLocal || isPreview) {
const { default: autoHighlightUnpublished } = await import('../blocks/preflight/checks/diff/autoHighlight.js');
autoHighlightUnpublished();

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.

shouldn't we await autoHighlightUnpublished() ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

This branch was successfully deployed

1 active deployment
MWPW-199318-preflight-content-diff 9fd776ee Deployed Sep 23, 2026 by aem-code-sync[bot]
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.

4 participants