Skip to content

test(mwpw-184989): repro getConfig pattern to validate AI review context [DO NOT MERGE]#540

Closed
sanrai wants to merge 1 commit into
mainfrom
ai-review-getconfig-repro
Closed

test(mwpw-184989): repro getConfig pattern to validate AI review context [DO NOT MERGE]#540
sanrai wants to merge 1 commit into
mainfrom
ai-review-getconfig-repro

Conversation

@sanrai

@sanrai sanrai commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Test-only PR (do not merge) to validate the AI code review depth-2 source-context change.

It reproduces the exact getConfig usage the AI reviewer repeatedly misread on #532 (claiming the empty-string second arg of getConfig('products', '') is a default value, and suggesting the broken getConfig('products', {})).

With the new source context (changed file + 2-hop relative imports), the bot should now pull hooks.js -> consonant.js and read makeConfigGetter/getByPath, so it understands the real (group, subKey) signature and does NOT repeat the misread.

Watching the AI review comment to confirm. Will close after.

… review context

Test-only change (do not merge). Reproduces the exact getConfig usage the AI
reviewer previously misread (claiming the empty-string 2nd arg is a default
value and suggesting getConfig('products', {})). Used to verify that the new
depth-2 source context lets the bot read makeConfigGetter/getByPath and get
the (group, subKey) signature right.
@github-actions

Copy link
Copy Markdown

🤖 Agent QA review — interactive + visual diff (advisory, non-blocking)

Drove the PR build on the live business.adobe.com collection (filtered, searched, paginated, inspected cards), guided by a PR-vs-stable visual diff (0.00% of pixels changed) and the PR code diff. Verdict: PASS.

QA Report: PR #540 — test(mwpw-184989): repro getConfig pattern

Summary

PR #540 is a test-only PR that adds useConfig() usage to FlexCard.jsx to validate AI code review behavior. The diff confirms 0.00% pixel change between the PR build and stable build.

What Was Tested

  1. Visual diff: Loaded the diff.png — 0.00% pixels changed, confirming no visual regressions.
  2. Page load: Navigated to https://business.adobe.com/resources/main.html — page loaded successfully, full card grid visible with 9 cards loaded, 3703 results shown.
  3. Card rendering: All [data-testid="consonant-Card"] elements render correctly with images, titles, descriptions, and CTA buttons. Cards are one-half type (standard half-width cards), not FlexCards (consonant-FlexCard — 0 found), so the changed component code is not exercised on this page.
  4. data-product attribute check: None of the 9 visible cards have data-product attributes set, consistent with the PR's logic: detailsTextOption !== 'productName' on this collection, so products = [] and productMatch = null, meaning the new spread {...(productMatch && { 'data-product': productMatch.tagID })} correctly produces no output.
  5. Filter panel: Products, Content Type, and Industry filters are present and visually intact. No filter-related errors.
  6. Console errors: Zero JavaScript console errors observed.
  7. No FlexCard instances found: The page uses one-half card layout, not FlexCard, so the component change has no direct visual impact here.

Code Change Assessment

The PR adds:

  • import { useConfig } from '../Helpers/hooks' to FlexCard.jsx
  • Calls getConfig('collection', 'detailsTextOption') and getConfig('products', '') to conditionally set data-product attribute on the <li> element.
  • The logic is guarded: only populates data-product when detailsTextOption === 'productName' AND a matching product is found.
  • The getConfig('products', '') pattern with empty-string second arg (not {}) is correct per the makeConfigGetter/getByPath signature ((group, subKey)).

Findings

  • No visual regressions — 0.00% pixel diff confirmed.
  • No runtime errors — zero console errors.
  • Functional behavior on this page is unchanged — FlexCard component not present on this collection page; even if it were, the guard condition ensures no attribute is added unless detailsTextOption === 'productName' is configured.
  • The code change is a test-only addition to validate AI review context — the PR description explicitly states "DO NOT MERGE" and this is purely for observing AI reviewer behavior.

Verdict

The PR introduces no visual, functional, or runtime regressions on the target page. The code logic is sound and the guard conditions are correct. The only concern is informational: this PR is explicitly marked "DO NOT MERGE" and should not be merged into production.

PR / stable / diff screenshots + console + axe artifacts in the workflow run.

@github-actions

Copy link
Copy Markdown

AI Code Review

Review

Issue 1: useConfig called with incorrect key path

const products = detailsTextOption === 'productName'
    ? Object.values(getConfig('products', '') || {})
    : [];

getConfig is makeConfigGetter(config), which resolves getConfig('products', '') as path products. (with a trailing dot, since key is '' not falsy enough — wait, let's check: key ? \${object}.${key}` : object— empty string''is falsy, so path is just'products'`). This is fine for path resolution.

However, DEFAULT_CONFIG has no products key, so defaultValue is undefined. If the actual config also lacks products, getByPath returns undefined, and isNullish(undefined) returns true, so getConfig returns undefined. The || {} guards Object.values(), so no crash — but silently produces an empty array, hiding misconfiguration. This is low severity.

Issue 2: Unstable key={cuid()} in footer map — pre-existing, not introduced here

Not introduced by this PR, skip.

Issue 3: getConfig('products', '') — empty string as key is ambiguous

When key is '' (empty string), the condition key ? ... : object evaluates to object ('products'), so the lookup is config.products. This works, but passing '' as the key to mean "no key" is fragile and undocumented. If the intent is to get the whole products object, getConfig('products') (one argument) would be clearer and more correct per the API contract. Low risk but worth noting.

Issue 4 (Critical): Object.values() on a non-plain-object config value

Object.values(getConfig('products', '') || {})

If config.products exists but is an array (which is plausible for a list of products authored as JSON), Object.values() on an array returns the array elements — which would work accidentally. But if it's a non-iterable primitive (e.g., a string was authored there), Object.values() on a string returns individual characters, causing product.tagID to be undefined for every "product", and productMatch would always be null. Silent data bug — no crash, just incorrect behavior.

Fix: Guard with isObject or use Array.isArray check:

const rawProducts = getConfig('products', '') || {};
const products = Array.isArray(rawProducts)
    ? rawProducts
    : (isObject(rawProducts) ? Object.values(rawProducts) : []);

Summary

Severity Issue
Medium Object.values() called on potentially non-object/array value — silently returns wrong data
Low getConfig('products', '') should be getConfig('products') per API contract

@sanrai

sanrai commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Test PR for validating AI review depth-2 context. Result: with depth-2 source context the bot reads getConfig's (group, subKey) signature correctly (no longer treats the 2nd arg as a default). Purpose served, closing.

@sanrai sanrai closed this Jun 25, 2026
@sanrai sanrai deleted the ai-review-getconfig-repro branch June 25, 2026 22:28
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.

1 participant