Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 38 additions & 3 deletions .github/qa/feature-review.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,36 @@ or {"sourceTest":"...","skipReason":"source search could not prove how the test
const observed = await page.evaluate(() => {
const grid = document.querySelector('.consonant-CardsGrid');
const cards = grid ? [...grid.querySelectorAll('.consonant-Card')] : [...document.querySelectorAll('.consonant-Card')];
return cards.slice(0, 12).map((card, index) => {
// Non-visual DOM artifacts (JSON-LD structured data). Card extraction
// alone is blind to script/meta tags, which made any assertion about
// them unverifiable and an automatic FAIL. Capture them explicitly.
// parentNode is an IDENTITY index: two blocks live in the same container
// element if and only if their parentNode values match. The class-based
// parent label alone is ambiguous (two different sections can share a
// class), which previously made one-block-per-collection look like a
// duplicate injection.
const parentIdentity = [];
const wrappers = [...document.querySelectorAll('.consonant-Wrapper')];
const jsonLd = [...document.querySelectorAll('script[type="application/ld+json"]')].slice(0, 6)
.map((scriptEl, index) => {
const parentEl = scriptEl.parentElement;
let parentNode = -1;
if (parentEl) {
parentNode = parentIdentity.indexOf(parentEl);
if (parentNode === -1) { parentIdentity.push(parentEl); parentNode = parentIdentity.length - 1; }
}
return {
n: index + 1,
parent: parentEl
? `${parentEl.tagName.toLowerCase()}${parentEl.className ? `.${String(parentEl.className).trim().split(/\s+/)[0]}` : ''}`
: '',
parentNode,
collectionIndex: parentEl ? wrappers.indexOf(scriptEl.closest('.consonant-Wrapper')) : -1,
attrs: [...scriptEl.attributes].map((a) => a.name).join(' '),
text: (scriptEl.textContent || '').slice(0, 1500),
};
});
const cardData = cards.slice(0, 12).map((card, index) => {
const title = card.querySelector('[class*="-title"]');
const links = [...card.querySelectorAll('a,button')].slice(0, 6).map((element) => ({
tag: element.tagName.toLowerCase(),
Expand All @@ -322,6 +351,7 @@ or {"sourceTest":"...","skipReason":"source search could not prove how the test
links,
};
});
return { cards: cardData, jsonLd };
});
console.log('[observed] ' + JSON.stringify(observed));
await page.screenshot({ path: '/tmp/feature-render.png', fullPage: true }).catch(() => {});
Expand All @@ -337,7 +367,11 @@ Expected, copied from that test: ${plan.expected}
Source mapping evidence: ${JSON.stringify(plan.mappingEvidence)}

Rendered first-collection cards (id, title, text, links/buttons):
${JSON.stringify(observed).slice(0, 6000) || '(no cards rendered)'}
${JSON.stringify(observed.cards).slice(0, 6000) || '(no cards rendered)'}

Structured data blocks on the page (script[type="application/ld+json"]):
${JSON.stringify(observed.jsonLd).slice(0, 6000) || '(none present)'}
Reading the blocks: parentNode is an identity index; two blocks share a container element only if their parentNode values are equal. collectionIndex says which .consonant-Wrapper collection (in document order) a block belongs to; -1 means outside any collection (e.g. page head). Per-container assertions must be judged per container, not page-wide.

Does the rendered DOM satisfy ONLY the selected test assertion? Do not introduce new expectations. Respond with ONLY JSON: {"verdict":"PASS"|"FAIL","reason":"one or two sentences citing observed vs expected"}`, 1500);
const res = extractJson(check);
Expand All @@ -352,7 +386,8 @@ Does the rendered DOM satisfy ONLY the selected test assertion? Do not introduce
**Fixture cards:** ${plan.cards.length}
**Expected:** ${plan.expected}
**Rendered (first collection):**
${observed.map((item) => `- ${item.n}. ${item.title || item.text.slice(0, 50)}${item.links.length ? ` [${item.links.map((link) => `${link.testId || link.tag}${link.href ? ` ${link.href}` : ''}`).join(', ')}]` : ''}`).join('\n') || '_(no cards rendered)_'}
${observed.cards.map((item) => `- ${item.n}. ${item.title || item.text.slice(0, 50)}${item.links.length ? ` [${item.links.map((link) => `${link.testId || link.tag}${link.href ? ` ${link.href}` : ''}`).join(', ')}]` : ''}`).join('\n') || '_(no cards rendered)_'}
**Structured data blocks:** ${observed.jsonLd.length}${observed.jsonLd.length ? ` (first: parent \`${observed.jsonLd[0].parent}\`, ${observed.jsonLd[0].text.length} chars)` : ''}

**Verdict:** ${res.reason}`);
process.exit(0);
Expand Down
7 changes: 7 additions & 0 deletions .github/workflows/pull-request.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,16 @@ jobs:
node-version: 16.13.1
- name: Install
run: npm ci
- name: Build and serve this PR's own dist
run: |
npm run build

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Duplicate build — this job needs: deployment (which already runs npm run build and uploads the result as a Pages artifact a few jobs up). This step rebuilds from source instead of reusing that artifact (e.g. via actions/download-artifact), so every PR run now builds the same commit twice with no reuse. Not blocking, but worth a follow-up to save CI time.

npx serve -l 5000 &
sleep 3

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fragile readiness checksleep 3 before curling the backgrounded serve process is a fixed-timing guess rather than a retry loop. On a slower/loaded runner this could intermittently fail the E2E step for reasons unrelated to this feature. Suggest a short retry loop instead, e.g. for i in $(seq 1 30); do curl -sf ... && break; sleep 1; done.

curl -sf http://localhost:5000/html/e2e/index.html > /dev/null
- name: E2E
run: npm run test:e2e-prod
env:
E2E_BASE_URL: http://localhost:5000
# Pin the Chrome binary path to the version installed above.
# The ubuntu-latest runner ships a newer system Chrome at
# /opt/google/chrome/chrome that chromedriver@146 cannot drive.
Expand Down
6 changes: 6 additions & 0 deletions e2e-tests/helpers/generateUrl.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ const mergeDeep = (target, source) => {
const generateUrl = (configOverrides = {}) => {
const finalConfig = mergeDeep(config, configOverrides);
const state = Buffer.from(JSON.stringify(finalConfig)).toString('base64');
// Prefer an explicitly served build. The shared github.io deployment is a
// race: every PR deploys to the same site, so e2e could test whichever
// PR deployed last instead of its own build.
if (process.env.E2E_BASE_URL) {
return `${process.env.E2E_BASE_URL}/html/e2e/index.html?state=${state}`;
}
if (process.env.GITHUB_ACTIONS) {
// eslint-disable-next-line no-template-curly-in-string
return `https://adobecom.github.io/caas/html/e2e/index.html?state=${state}`;
Expand Down
38 changes: 38 additions & 0 deletions e2e-tests/specs/jsonld.e2e.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// e2e-tests/specs/jsonld.e2e.js
const generateUrl = require('../helpers/generateUrl');

describe('JSON-LD Collection Emission', () => {
it('emits a parseable Schema.org ItemList when showJsonLd is enabled', async () => {
const url = generateUrl({ collection: { showJsonLd: true } });
await browser.url(url);

await browser.waitUntil(
async () => $('script[data-caas-jsonld]').isExisting(),
{ timeout: 15000, timeoutMsg: 'JSON-LD script tag was not injected' },
);

/* eslint-disable-next-line */
const jsonText = await browser.execute(() => document.querySelector('script[data-caas-jsonld]').textContent);
const jsonLd = JSON.parse(jsonText);

expect(jsonLd['@context']).toEqual('https://schema.org');
expect(jsonLd['@type']).toEqual('ItemList');
expect(jsonLd.numberOfItems).toBeGreaterThan(0);
expect(jsonLd.itemListElement.length).toBeGreaterThan(0);
expect(jsonLd.itemListElement.length).toBeLessThanOrEqual(50);
expect(jsonLd.itemListElement[0].item['@type']).toEqual('CreativeWork');
});

it('does not emit the block when showJsonLd is disabled', async () => {
const url = generateUrl({});
await browser.url(url);

await browser.waitUntil(
async () => $('.consonant-Card').isExisting(),
{ timeout: 15000, timeoutMsg: 'Cards did not render' },
);

const exists = await $('script[data-caas-jsonld]').isExisting();
expect(exists).toBe(false);
});
});
22 changes: 22 additions & 0 deletions react/src/js/components/Consonant/Container/Container.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import Bookmarks from '../Bookmarks/Bookmarks';
import Paginator from '../Pagination/Paginator';
import Grid from '../Grid/Grid';
import CardFilterer from '../Helpers/CardFilterer';
import { injectCollectionJsonLd } from '../Helpers/jsonLd';
import FiltersPanelTop from '../Filters/Top/Panel';
import LeftFilterPanel from '../Filters/Left/Panel';
import JsonProcessor from '../Helpers/JsonProcessor';
Expand Down Expand Up @@ -113,6 +114,7 @@ const Container = (props) => {
const paginationType = getConfig('pagination', 'type');
const paginationIsEnabled = getConfig('pagination', 'enabled');
const resultsPerPage = getConfig('collection', 'resultsPerPage');
const showJsonLd = getConfig('collection', 'showJsonLd');
const onlyShowBookmarks = getConfig('bookmarks', 'leftFilterPanel.bookmarkOnlyCollection');
const authoredFilters = getConfig('filterPanel', 'filters');
const categoryMappings = getConfig('filterPanel', 'categoryMappings');
Expand Down Expand Up @@ -1534,6 +1536,26 @@ const Container = (props) => {
gridCardLen = cardCount;
}

/**
* Emits a Schema.org ItemList describing the rendered cards, so LLM
* crawlers and agents can classify collection content. Card tag ids
* (hashed or not) resolve to labels via the authored filter config,
* which Container has already hashed to match when isHashed is set.
* Additive script tag, replaced on re-render; no rendering impact.
* Opt-in via collection.showJsonLd; serializes at most 50 cards
* while numberOfItems reports the true filtered total.
*/
useEffect(() => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No cleanup on unmount / toggle-off — this useEffect never returns a cleanup function, and injectCollectionJsonLd's removal step only runs when it's actually invoked. Since the effect body early-returns on if (!showJsonLd) return;, that removal path is skipped whenever showJsonLd goes true→false, and there's no unmount cleanup at all.

Concretely: unmounting the Container (route change, re-render without remount) or flipping showJsonLd off in the same mounted instance leaves the injected <script data-caas-jsonld> block in the DOM permanently, describing cards that are no longer shown — which directly contradicts the PR description's safety claim ("Removed when the rendered card list becomes empty").

Suggest: useEffect(() => { if (!showJsonLd) return; const script = injectCollectionJsonLd({...}); return () => script?.remove(); }, [...]);

if (!showJsonLd) return;
injectCollectionJsonLd({
cards: gridCards,
filters: authoredFilters,
container: box.current,
collectionTitle: getConfig('collection', 'i18n.title'),
totalItems: filteredCards.length,
});
}, [gridCards, showJsonLd]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Dependency array omits variables the effect reads — deps are [gridCards, showJsonLd], but the body also reads authoredFilters, getConfig('collection', 'i18n.title'), and filteredCards.length.

gridCards is timedCollection.length ? timedCollection : filteredCards (line 1533) — so whenever a timed-event sort is active, gridCards tracks timedCollection, fully decoupled from filteredCards. In that case filteredCards.length (the totalItems passed in) can change on a re-render without gridCards changing, so the effect won't re-run and the injected numberOfItems goes stale relative to the real current filtered count.


/**
* Total pages (used by Paginator Component)
* @type {Number}
Expand Down
154 changes: 154 additions & 0 deletions react/src/js/components/Consonant/Helpers/__tests__/jsonLd.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
import {
buildTagLabelMap,
buildCardEntry,
buildCollectionJsonLd,
injectCollectionJsonLd,
} from '../jsonLd';

const filters = [{
id: 'caas:products',
group: 'Products',
items: [
{ id: 'caas:products/photoshop', label: 'Photoshop' },
{
id: 'caas:products/video',
label: 'Video',
isCategory: true,
items: [{ id: 'caas:products/video/premiere', label: 'Premiere Pro' }],
},
],
}];

const hashedFilters = [{
id: 'h4x2',
group: 'Products',
items: [{ id: '4x24/l1s1', label: 'Photoshop' }],
}];

const card = {
id: '1.0.0',
contentArea: { title: 'Getting started with Photoshop' },
ctaLink: 'https://adobe.com/resources/photoshop-guide',
tags: [{ id: 'caas:products/photoshop' }],
};

const hashedCard = {
...card,
tags: [{ id: '4x24/l1s1' }, { id: 'zz99/qq11' }],
};

describe('buildTagLabelMap', () => {
test('maps item ids to labels, including nested category items', () => {
const map = buildTagLabelMap(filters);
expect(map['caas:products/photoshop']).toBe('Photoshop');
expect(map['caas:products/video/premiere']).toBe('Premiere Pro');
});

test('works with hashed ids', () => {
expect(buildTagLabelMap(hashedFilters)['4x24/l1s1']).toBe('Photoshop');
});

test('returns empty object for empty input', () => {
expect(buildTagLabelMap()).toEqual({});
});
});

describe('buildCardEntry', () => {
test('emits url and resolved keywords only', () => {
const entry = buildCardEntry(card, buildTagLabelMap(filters));
expect(entry).toEqual({
'@type': 'CreativeWork',
url: 'https://adobe.com/resources/photoshop-guide',
keywords: 'Photoshop',
});
});

test('resolves hashed tags via the filter map and skips unresolvable hashes', () => {
const entry = buildCardEntry(hashedCard, buildTagLabelMap(hashedFilters));
expect(entry.keywords).toBe('Photoshop');
});

test('omits url and keywords when absent', () => {
const entry = buildCardEntry({ contentArea: { title: 'X' } }, {});
expect(entry).toEqual({ '@type': 'CreativeWork' });
});
});

describe('buildCollectionJsonLd', () => {
test('builds a valid ItemList with collection title', () => {
const jsonLd = buildCollectionJsonLd([card], filters, 'All resources');
expect(jsonLd['@context']).toBe('https://schema.org');
expect(jsonLd['@type']).toBe('ItemList');
expect(jsonLd.name).toBe('All resources');
expect(jsonLd.numberOfItems).toBe(1);
expect(jsonLd.itemListElement[0].position).toBe(1);
expect(jsonLd.itemListElement[0].item.url).toBe('https://adobe.com/resources/photoshop-guide');
});

test('caps serialized entries at 50 while reporting the true total', () => {
const manyCards = Array.from({ length: 200 }, (_, i) => ({ ...card, id: `card-${i}` }));
const jsonLd = buildCollectionJsonLd(manyCards, [], '', 4000);
expect(jsonLd.itemListElement).toHaveLength(50);
expect(jsonLd.numberOfItems).toBe(4000);
});

test('true total never underreports the rendered count', () => {
expect(buildCollectionJsonLd([card, hashedCard], [], '', 0).numberOfItems).toBe(2);
});

test('round-trips through JSON serialization', () => {
const parsed = JSON.parse(JSON.stringify(buildCollectionJsonLd([card], filters)));
expect(parsed.itemListElement[0].item.keywords).toBe('Photoshop');
});
});

describe('injectCollectionJsonLd', () => {
afterEach(() => {
document.body.innerHTML = '';
});

test('injects one parseable script tag into the container', () => {
const container = document.createElement('div');
document.body.appendChild(container);
injectCollectionJsonLd({ cards: [card], filters, container });
const script = container.querySelector('script[type="application/ld+json"]');
expect(script).not.toBeNull();
expect(JSON.parse(script.textContent)['@type']).toBe('ItemList');
});

test('replaces the previous block on re-injection', () => {
const container = document.createElement('div');
document.body.appendChild(container);
injectCollectionJsonLd({ cards: [card], filters: [], container });
injectCollectionJsonLd({ cards: [card, hashedCard], filters: [], container });
const scripts = container.querySelectorAll('script[type="application/ld+json"]');
expect(scripts).toHaveLength(1);
expect(JSON.parse(scripts[0].textContent).numberOfItems).toBe(2);
});

test('returns null with no cards', () => {
expect(injectCollectionJsonLd({ cards: [] })).toBeNull();
});

test('removes the stale block when the card list becomes empty', () => {
const container = document.createElement('div');
document.body.appendChild(container);
injectCollectionJsonLd({ cards: [card], filters: [], container });
expect(container.querySelector('script[data-caas-jsonld]')).not.toBeNull();
injectCollectionJsonLd({ cards: [], filters: [], container });
expect(container.querySelector('script[data-caas-jsonld]')).toBeNull();
});

test('supports multiple collections on one page independently', () => {
const containerA = document.createElement('div');
const containerB = document.createElement('div');
document.body.appendChild(containerA);
document.body.appendChild(containerB);
injectCollectionJsonLd({ cards: [card], filters: [], container: containerA });
injectCollectionJsonLd({ cards: [card, hashedCard], filters: [], container: containerB });
injectCollectionJsonLd({ cards: [card], filters: [], container: containerA });
expect(document.querySelectorAll('script[data-caas-jsonld]')).toHaveLength(2);
expect(JSON.parse(containerA.querySelector('script').textContent).numberOfItems).toBe(1);
expect(JSON.parse(containerB.querySelector('script').textContent).numberOfItems).toBe(2);
});
});
1 change: 1 addition & 0 deletions react/src/js/components/Consonant/Helpers/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ export const DEFAULT_CONFIG = {
transparent: false,
},
displayTotalResults: true,
showJsonLd: false,
totalResultsText: '{} results',
i18n: {
prettyDateIntervalFormat: '{LLL} {dd} | {timeRange} {timeZone}',
Expand Down
Loading
Loading