feat(mwpw-203439): emit Schema.org JSON-LD ItemList for card collections - #567
feat(mwpw-203439): emit Schema.org JSON-LD ItemList for card collections#567sanrai wants to merge 12 commits into
Conversation
Cards render as divs that machine readers cannot classify, and card tags arrive hashed. This emits one application/ld+json script block per collection describing rendered cards (name, url, keywords) as a Schema.org ItemList. Tag ids resolve to labels via the authored filter config, which Container already hashes to match card tags. - Opt-in via collection.showJsonLd (default false) - Serializes at most 50 cards; numberOfItems reports the true total - Additive script tag injected after render; replaced on re-render - Zero impact on rendering, payload, or SEO when disabled
AI Code ReviewLast updated Aug 10, 2026 11:25 AM PDT · new commit pushed · commit 0 open · 1 resolved Open findingsNo open findings. ✅ ✅ Resolved
Review history (11 runs)
|
Agent QA review — interactive + visual diff (advisory, non-blocking)Last updated Aug 10, 2026, 11:25 AM PDT · new commit pushed · commit 0 open · 0 resolved · visual diff 0.00% · verdict PASS Open findings
What the agent checkedTested the PR (feat(mwpw-203439): emit Schema.org JSON-LD ItemList for card collections) against the live business.adobe.com/resources/main.html collection. What I checked:
Overall: no regressions found in visible/interactive behavior of the card collection (filters, results count, card rendering). The core new feature (JSON-LD ItemList emission) is correctly not present because it is opt-in and disabled by default on this collection instance, which matches the PR's documented safety design. No console errors, no layout/visual defects observed. PR / stable / diff screenshots + console + axe artifacts in the workflow run. Review history (9 runs)
|
Replace the fixed element id with a data-caas-jsonld attribute and scope lookup/replacement to each collection's own container. Pages with multiple card collections now emit one valid block per collection with no id duplication and no cross-collection clobbering.
🧪 Feature QA review — injected feature test (advisory, non-blocking)Last updated Aug 10, 2026, 11:54 AM PT · commit Overall: PASS Injected the PR build with a searched, source-grounded config + collection fixture. Source test:
Verdict: The first collection's section container has a script[data-caas-jsonld][type='application/ld+json'] (block n=3) whose parsed JSON has '@type' === 'ItemList', matching the expected assertion. Screenshot in the workflow run. Review history (9 runs)
|
The effect previously bailed out on an empty card list, leaving a previously injected block describing cards no longer shown. An empty list now removes the existing block, so the metadata always matches the rendered state.
Titles and descriptions are already crawlable in the visible DOM. The block's unique contribution is the resolved tag taxonomy and the link, so entries now carry only url and keywords.
The observation step extracted only card-level data (title, text, links), so any assertion about non-card DOM, such as an injected script[type=application/ld+json] block, was structurally unverifiable and produced an automatic FAIL even when the tag was present (verified present via e2e and manual DOM inspection on the same build). The observation now also captures up to four ld+json blocks with their parent element, attributes, and content, passes them to the validation judge, and reports the block count in the PR comment.
The class-based parent label is ambiguous: two different collection sections share the same class, so one correct block per collection was judged as a duplicate injection into one container. Blocks now carry a parentNode identity index (equal values mean same container element) and a collectionIndex (which .consonant-Wrapper, in document order, or -1 for head/page level), and the judge is instructed to evaluate per-container assertions per container.
…ment All PRs deploy to the same github.io site, so run-e2e-tests raced: whichever PR deployed last was the build every concurrent PR's e2e actually tested. This PR's e2e failed because a later branch's deployment overwrote ours before the tests ran. The e2e job now builds this PR's dist and serves it locally, and generateUrl prefers an explicit E2E_BASE_URL over the shared site. Every PR's e2e is now hermetic to its own code.
sanrai
changed the title
|
Take a look at these findings and let me know if these are known issues so that I can approve it. Code review(review · 7 findings) One more thing worth flagging: unit tests pass with coverage thresholds met, but Container.jsx:1550 (the injectCollectionJsonLd call site) isn't exercised by any existing Container test — not a build breaker, but worth mentioning to the PR author. ✻ Cooked for 36s ※ recap: You're reviewing PR #567 (JSON-LD for card collections); the background code review just finished with 7 findings, mainly stale/misaligned JSON-LD injection timing and content. Next: decide whether to fix these issues or comment them on the PR. (disable recaps in /config) |
|
A few things from review, roughly in order of importance:
#1 and #2 are the ones I'd want addressed before merge — the rest are good to track as follow-ups. |
sheridansunier
left a comment
sheridansunier
left a comment
There was a problem hiding this comment.
Reviewed via automated pass (line-by-line, removed-behavior, cross-file, reuse, simplification, efficiency, altitude, conventions angles + direct source verification). Feature is opt-in/low-blast-radius and well tested overall; flagging two correctness issues that undercut the PR's own stated goals, one staleness edge case, and two CI nits. Posting as comments for discussion, not a formal block.
| * Opt-in via collection.showJsonLd; serializes at most 50 cards | ||
| * while numberOfItems reports the true filtered total. | ||
| */ | ||
| useEffect(() => { |
There was a problem hiding this comment.
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(); }, [...]);
| collectionTitle: getConfig('collection', 'i18n.title'), | ||
| totalItems: filteredCards.length, | ||
| }); | ||
| }, [gridCards, showJsonLd]); |
There was a problem hiding this comment.
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.
|
|
||
| const url = getByPath(card, 'ctaLink', '') | ||
| || getByPath(card, 'overlayLink', '') | ||
| || getByPath(card, 'footer.0.right.0.href', ''); |
There was a problem hiding this comment.
URL fallback misses real card link slots — this only ever reads the right CTA slot (footer.0.right.0.href). Cards/Card.jsx's actual link resolution (getCtaLink, lines ~296-339) also serves the card's rendered link from center and alt footer slots depending on ctaButtonStyle.
Any card whose real, rendered link lives in a center or alt slot (a real, supported layout) will get no url in its JSON-LD entry at all — silently dropping the one piece of data this PR describes as its "unique contribution" for that subset of cards. Worth reusing getCtaLink's resolution (or an equivalent covering all three slots) instead of a partial ad-hoc path lookup.
| run: npm ci | ||
| - name: Build and serve this PR's own dist | ||
| run: | | ||
| npm run build |
There was a problem hiding this comment.
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.
| run: | | ||
| npm run build | ||
| npx serve -l 5000 & | ||
| sleep 3 |
There was a problem hiding this comment.
Fragile readiness check — sleep 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.
sheridansunier
left a comment
sheridansunier
left a comment
There was a problem hiding this comment.
Requesting changes on the two correctness issues flagged inline (missing cleanup in the useEffect leaves a stale JSON-LD script in the DOM, and the URL fallback silently drops the url for cards using center/alt CTA slots) — see review comments above for details. The dependency-array staleness and CI items are non-blocking suggestions.
3 participants
What
Collections can now emit one
<script type="application/ld+json">block describing the rendered cards as a Schema.org ItemList. Each entry is minimal on purpose:url+keywords.Why
Cards render as divs whose tag taxonomy is invisible to machine readers: tags arrive hashed from the backend. Titles and descriptions are already crawlable in the visible DOM, so this block adds only what is otherwise unreadable: the resolved tag classification (
keywords) attached to a citableurl.How tag resolution works
We never reverse the hash. The authored filter config pairs tag ids with human labels, and Container already hashes those ids in place to match hashed card tags (existing behavior the filter panel depends on). So the config maps (hashed) tag id -> label for free. Card tags without a filter entry are skipped, never emitted raw.
Safety
collection.showJsonLd, defaultfalse.useEffectafter paint; scoped per collection container, so multiple collections on one page each manage their own block. Removed when the rendered card list becomes empty, so the metadata never describes cards no longer shown.numberOfItemsreports the true filtered total.Testing
Known limitation
Client-rendered only: crawlers that do not execute JS will not see the block. Static-fetch coverage would require SSR, out of scope.
Rollout
Enable on one low-traffic collection surface first. Note: keyword coverage equals the page's filter panel coverage; tags outside the panel taxonomy are skipped by design. MWPW ticket to be linked.