feat: Update CWV schema and projection to support atomic suggestions

[tkotthakota-adobe]

https://jira.corp.adobe.com/browse/SITES-44229

Problem:
The ASO/Backoffice UI needs the metric field (lcp/cls/inp) to group and label per-metric suggestion rows under each URL. Without it in the minimal projection, the API doesn't return it — the UI would have to dig into data.issues[0].type to figure out which metric a suggestion represents. Also, per-metric suggestions only have one metric field (e.g., just lcp), so the transformer was padding the response with cls: undefined, inp: undefined — unnecessary noise.

Changes:

• suggestion.data-schemas.js — Added metric and isCodeChangeAvailable to CWV Joi schema. Added metric to minimal projection.
• suggestion.projection-utils.js — filterCwvMetrics now omits null/undefined metric fields, only returns fields with actual values. Zero preserved.
• Tests — 7 new tests covering per-metric validation, backward compat, invalid metric rejection, projection field, and transformer behavior for null/zero values.

Results from test script:

=== SITES-44229: CWV Joi Schema & Projection Tests ===
ENV: dev | BASE_URL: https://spacecat.experiencecloud.live/api/ci
SITE_ID: cfaa3c1b-58a7-4cc8-bec9-ba9d7e201894 | OPPORTUNITY_ID: 07675f97-187f-4061-b26f-2dd304a79403

--- Test 1: Backward compatibility — old bundled suggestions ---
  Found 126 suggestions (HTTP 200)
  PASS: GET suggestions returns 200 with existing data

--- Test 2: metric field in minimal projection ---
  Total: 126 | per-metric (data.metric set): 120 | bundled (data.metric absent): 6
  First suggestion data keys: jiraLink, metrics, organic, pageviews, type, url
  PASS: Standard projection fields present (url, metrics, type)
  Per-metric suggestion: id=e7a9b566-860b-4e34-9741-2c7fe213cb52 metric=inp type=url aggKey=cwv#https://www.hdfc.bank.in/debit-cards#inp
  PASS: Per-metric suggestion: data.metric is valid (inp ∈ {lcp,cls,inp})
  PASS: Per-metric suggestion: aggregationKey matches cwv#<key>#inp convention
  Bundled suggestion: id=cfe9e73a-e438-4d21-b069-792c77ddd534  data.metric present? false
  PASS: Bundled suggestion: data.metric absent (backward compat preserved)

--- Test 3: filterCwvMetrics — metrics array structure ---
  Inspecting per-metric suggestion: metrics array length=2
  PASS: metrics[0] has deviceType
  PASS: metrics[0] has at least one CWV value (lcp/cls/inp)
  PASS: Per-metric suggestion: filterCwvMetrics keeps only 'inp' per device (other CWV keys absent)
  PASS: Per-metric suggestion: no null 'inp' values

--- Test 4: PATCH with metric field validates (non-destructive) ---
  Target suggestion: e7a9b566-860b-4e34-9741-2c7fe213cb52 (original data.metric=inp)
  PASS: PATCH with metric field accepted (HTTP 200)
  PASS: metric field persisted and returned in GET response
  PASS: Other data fields preserved (url/pattern present, metrics non-empty)
  Cleanup: restored original metric=inp (HTTP 200)

=== Results: 12 passed, 0 failed ===

[solaris007]

Hey @tkotthakota-adobe,

Clean, well-scoped change. Schema additions are correctly optional, the transformer's != null guard is the right idiom for preserving zero, and the tests cover the three documented data shapes plus the invalid-metric rejection path. Approving with a few minor observations as nice-to-haves.

Strengths

• Schema doc comment evolved from "two implicit shapes" to "three" rather than going stale (suggestion.data-schemas.js:140-145).
• Backward compatibility preserved: all new fields optional; the explicit "passes without metric field (backward compat with bundled suggestions)" test pins the contract.
• Zero preservation correctly tested at suggestion.projection-utils.test.js:88-97. This is exactly the regression test you want when someone later tries to "simplify" with a truthy check.
• Invalid-metric rejection test confirms the enum is enforced rather than slipping through unknown(true).

Minor (Nice to Have)

Hardcoded ['lcp', 'cls', 'inp'] enum at suggestion.data-schemas.js:147

The enum is a literal here, and the producer side (audit-worker) carries its own knowledge of the same set. The inner metrics[] sub-schema at lines 153-164 already accepts ttfb as a numeric field, so the outer discriminator and inner shape can drift when TTFB/FCP per-metric suggestions land.

Suggested follow-up (this PR or next): extract a constant at the top of the file:

const CWV_PER_METRIC_VALUES = ['lcp', 'cls', 'inp'];

and reference it from Joi.string().valid(...CWV_PER_METRIC_VALUES). The transformer can use the same source so adding a new metric becomes a one-line change.

metric vs metrics naming at suggestion.data-schemas.js:147

The schema now has both metric: Joi.string() (the discriminator) and metrics: Joi.array() (the measurements). Since both are optional, a typo on the consumer side falls through validation silently. Now is the cheapest moment to rename - once UI ships against metric, renaming becomes a breaking change. metricType would mirror the existing type field. Preference, not a blocker.

Test improvement: tie projection config to transformer function

The minimal projection references the transformer by name string 'filterCwvMetrics' while the function lives in FIELD_TRANSFORMERS. The two are tested in isolation today. A short integration test that resolves the projection config and applies the named transformer would catch the case where someone renames either side without the other.

Out of scope, worth tracking

• isCodeChangeAvailable is now declared in CWV plus the existing COLOR_CONTRAST, A11Y_ASSISTIVE, and FORM_ACCESSIBILITY schemas. Cross-cutting "this suggestion has a deployable patch" concept that probably wants a shared sub-schema once a fifth occurrence shows up. Not a regression here, trend tracking only.
• The wire-shape change (minimal projection now omits null/undefined metric keys instead of writing lcp: undefined) is exactly what the PR description says the UI consumer wants. Worth a quick check that the ASO/Backoffice UI build reads the updated shape and not a cached {deviceType, lcp, inp, cls} always-4-keys assumption.

Assessment

Ready to merge? Yes. To your verification question: nothing must be modified on top of this PR. The minor observations above are nice-to-haves; the wire-shape check belongs in the consumer repo, not here.

[github-actions]

This PR will trigger no release when merged.

[tkotthakota-adobe]

Thanks for the review @solaris007 Addressed some of the feasible review comments.

• Extracted CWV_PER_METRIC_VALUES constant — shared between Joi schema and filterCwvMetrics transformer. Adding a new metric (e.g., ttfb) is a one-line change.
• metric vs metricType naming — noted but not changing. metric is already in the code and consistent with the audit-worker's data.metric. Renaming now would require coordinating across repos.
• Projection-to-transformer integration test — not adding. The reviewer marked it as nice-to-have and the existing tests cover both sides. Low risk of drift.
• isCodeChangeAvailable cross-cutting pattern — acknowledged as trend tracking, no action needed now.
• Wire-shape check for UI — out of scope for this PR, belongs in the UI consumer repo.

[tkotthakota-adobe]

@ramboz @solaris007 Re-requesting review as I updated the PR with main branch.

[ramboz]

If we start having strict requirements for the issues, we should probably expand the schema here to include those (type, value, patchContent, etc.)

[tkotthakota-adobe]

Include type, value, patchContent as optional values is possible as we need to maintain backward compatibility.
To handle independent issue tracking, approvals etc, we might want to also include id and status. This will make things bit complex.

[solaris007]

Hey @tkotthakota-adobe,

Re-reviewed after the "review comments" commit and main-merges. The prior approval still stands; the incremental change cleanly addresses prior Minor 1, and the other two prior Minors resolve to defensible pushback or out-of-scope follow-up.

Strengths

• Prior Minor 1 (hardcoded ['lcp', 'cls', 'inp'] enum) is addressed cleanly. CWV_PER_METRIC_VALUES is exported from packages/spacecat-shared-data-access/src/models/suggestion/suggestion.data-schemas.js near the other module constants, consumed by Joi.string().valid(...CWV_PER_METRIC_VALUES) at the CWV entry, and imported by suggestion.projection-utils.js for filterCwvMetrics. Schema and transformer can no longer drift; adding ttfb/fcp is the one-line change the doc comment advertises.
• Behavior preserved in the incremental change. Both the prior if (metric.lcp != null) ... chain and the new for (const key of CWV_PER_METRIC_VALUES) if (m[key] != null) loop use the same != null guard against the same three keys, so output is identical for all three documented shapes (bundled, per-metric, group).
• Tests remain valid. chai.deep.equal is key-order-insensitive, so the rotated emission order is benign. The constant's contents are implicitly pinned by the existing per-metric validation tests (metric: 'lcp' accepted, metric: 'ttfb' rejected) plus the projection-utils tests - changing the constant without updating tests would break at least one.
• Validation surface tightened. metric: Joi.string().valid(...).optional() is a stricter check than relying on the parent unknown(true) to silently accept the field. Net win for input validation.
• Schema docstring updated. Now documents three CWV shapes (bundled, per-metric, group) rather than going stale at two.

Prior findings re-review

• Minor 2 (metric vs metricType naming) - pushback holds, dropped. Your counter-argument is sound: data.metric is already the wire name in audit-worker and downstream; the Joi.valid(...) set makes it unambiguous that this field stores a metric name; and a rename inside data-access alone would introduce inconsistency, not remove it. Not worth re-raising.
• Minor 3 (integration test linking projection config name string to transformer function) - partially mitigated. The shared constant fixes the values coupling (schema enum vs transformer keys). The name-string coupling (transformers: { metrics: 'filterCwvMetrics' } -> FIELD_TRANSFORMERS['filterCwvMetrics']) is orthogonal and unchanged - a typo on either side would still fail silently. That risk exists across all transformer registrations in this module, not specifically because of this PR. Out of scope here.

Recommendations

• Promote CWV_PER_METRIC_VALUES to @adobe/spacecat-shared-utils/src/constants.js when the next CWV-touching change in audit-worker lands. The same enum is duplicated as METRICS = ['lcp', 'cls', 'inp'] in spacecat-audit-worker/src/cwv/kpi-metrics.js. Audit-worker is the producer; data-access enforces the same enum on validate. That makes it a wire-contract value, not a schema implementation detail. The natural home is alongside OPPORTUNITY_TYPES (which suggestion.data-schemas.js already imports). Best to do producer-side deletion in the same cross-repo PR rather than living in eventual-consistency limbo.
• isCodeChangeAvailable is now present on CWV plus COLOR_CONTRAST, A11Y_ASSISTIVE, and FORM_ACCESSIBILITY schemas. Four occurrences is the threshold to start tracking; a shared codeChangeFields sub-schema (isCodeChangeAvailable, patchContent, jiraLink) becomes worthwhile at five. Note for the next schema author.
• Optional belt-and-suspenders: a one-liner in suggestion.projection-utils.test.js asserting CWV_PER_METRIC_VALUES contains exactly ['lcp', 'cls', 'inp'] would lock the contract explicitly. Not blocking.
• Defense-in-depth (out of scope here): confirm audit-worker logs Joi validation failures with enough context (opportunity type, suggestion id) so a future metric typo surfaces in Coralogix rather than silently dropping or 500-ing.

Assessment

Ready to merge? Yes.

The incremental change is a textbook addressing of prior Minor 1 with no regressions. CI is green. Pushback on Minor 2 is reasonable. Minor 3 is a pre-existing pattern concern beyond this PR's scope.

Next Steps

ramboz's open inline comment on suggestion.data-schemas.js:175 (the issues shape - "If we start having strict requirements for the issues, we should probably expand the schema here to include those (type, value, patchContent, etc.)") is in the same neighborhood as the recommendations above. The peer schemas (COLOR_CONTRAST, A11Y_ASSISTIVE, FORM_ACCESSIBILITY) all model the inner issue object explicitly, so tightening makes sense - but only once the producer shape is considered stable. A confirming reply on that thread (tighten now vs defer) would close the loop.

[solaris007]

Hey @tkotthakota-adobe,

3rd review cycle. The single commit a6fb59f since cycle 2 ("remove metric") reverses material parts of the prior approval - it drops the metric field from the CWV schema, removes the CWV_PER_METRIC_VALUES constant I praised in cycle 2, deletes 53 lines of associated tests, and inlines ['lcp', 'cls', 'inp'] back into filterCwvMetrics. The code change itself is self-consistent and CI is green, but the artifacts the revert leaves behind (stale description, missing rationale, undocumented contract) should be addressed before merge.

The runtime behavior change remaining in the PR (filterCwvMetrics null/undefined stripping with zero preservation) is durable and worth shipping.

Strengths

• filterCwvMetrics rewrite (packages/spacecat-shared-data-access/src/models/suggestion/suggestion.projection-utils.js:46-65) is clean. Uses != null so 0 correctly passes through, has dedicated test coverage for null/undefined/zero. For per-metric suggestions this drops the previously-padded cls: undefined, inp: undefined keys; for bundled suggestions output is unchanged.
• CWV schema now declares jiraLink, aggregationKey, and isCodeChangeAvailable explicitly rather than relying on .unknown(true) (suggestion.data-schemas.js:167-169). Brings CWV into line with peer schemas (COLOR_CONTRAST, A11Y_ASSISTIVE, FORM_ACCESSIBILITY).
• Test deletion is self-consistent: the 4 deleted tests in the describe('CWV per-metric suggestion', ...) block all exercised the now-removed metric field. No orphaned assertions.
• Docstring (suggestion.data-schemas.js:140-143) accurately describes the schema at this commit (back to "two implicit shapes").

Issues

Important (Should Fix)

1. PR description, title, and the embedded test-script output are out of sync with what a6fb59f ships.

The body (last updated 2026-05-07) still claims "Added metric and isCodeChangeAvailable to CWV Joi schema. Added metric to minimal projection." and "Tests - 7 new tests covering per-metric validation, backward compat, invalid metric rejection, projection field, and transformer behavior for null/zero values." After a6fb59f, only isCodeChangeAvailable and the null/zero transformer test claim survive. The embedded "Results from test script" block also exercises data.metric validation and aggregationKey: cwv#<key>#inp shapes that are no longer schema-level guarantees. The title "support atomic suggestions" overstates what merges since the schema no longer carries the atomic-vs-bundled discriminator.

Why it matters: reviewers approve against the description, downstream automation reads PR bodies (release notes, audit history), and the body becomes part of the squash-merge commit message permanently.

Action: rewrite the body to describe the current state. Consider retitling to something narrower like "feat(cwv): omit null metric values in minimal projection". Add a one-line rationale for the metric revert.

2. The scope reversal lacks rationale and leaves the atomic-vs-bundled contract undocumented.

The commit message is the one-word string "remove metric". No PR comment, no thread response, no Jira update referenced. The original Jira ticket (SITES-44229) was explicit that the ASO/Backoffice UI needs metric to group and label per-metric suggestion rows.

With metric gone from both the schema and the minimal projection, downstream consumers must now infer atomic-vs-bundled by inspecting data.metrics[*] keys (one CWV key present implies atomic, all three implies bundled). That convention is not documented anywhere - not in the schema, not in the projection transformer docstring, not in any test. Two concrete consequences:

• The schema-level validation regressed: unknown(true) means audit-worker can now write data.metric = 'ttfb', data.metric = '', data.metric = null, or any other value with no validation signal. The PR description's own integration test output shows producers writing data.metric in 120 of 126 returned suggestions today. The PR makes the contract laxer than main is today for a field that real production data is using.
• A tier of consumers see different views: a client reading raw data still sees data.metric via unknown(true). A client reading the minimal projection does not. The same suggestion shape depends on which endpoint the client hits. If audit-worker ever writes a bundled suggestion with one CWV key naturally null (real-world measurement gaps happen), the inference flips silently to "atomic" and the UI mislabels the row.

Action: pick one of two paths and commit to it in this PR.

• (a) Restore metric as Joi.string().valid('lcp', 'cls', 'inp').optional() in the CWV schema and in the minimal projection. Preferred if the UI still groups by it.
• (b) Keep the revert. Document the atomic-vs-bundled inference contract in the [OPPORTUNITY_TYPES.CWV] docstring. Add a unit test exercising "bundled suggestion with one CWV key null" so the inference behavior is pinned. State (in the PR description or docstring) that data.metric is intentionally a producer-side concern with the authoritative enum in spacecat-audit-worker/src/cwv/kpi-metrics.js.

Either is defensible. The current state (revert with no documentation and no test) is not.

3. Open thread on issues schema is unresolved.

suggestion.data-schemas.js:166. ramboz on 2026-05-12 suggested expanding the CWV issues shape to include type, value, patchContent. Your reply on 2026-05-13 acknowledged the complexity but left no concrete next step. Thread is still open.

Why it matters: shipping with an open thread on a schema PR risks freezing whatever the producer writes today as the de facto contract. Peer accessibility schemas all model their issues[*] objects explicitly; CWV is the outlier.

Action: either expand the issues shape in this PR (small diff behind optional fields, low risk) or file a follow-up issue and link it in the thread before merge. Don't leave it dangling.

Minor (Nice to Have)

4. isCodeChangeAvailable on CWV has no schema-level test.
The field is declared in the CWV schema (suggestion.data-schemas.js:167) but the only test that exercised it was inside the deleted per-metric describe block. The three surviving CWV validateData tests do not pass this field. Peer schemas have dedicated coverage. Joi's unknown(true) masks the practical risk, but if the field is renamed/removed, no CWV test will catch it. Add one assertion to an existing CWV validateData case.

5. jiraLink and aggregationKey null-acceptance has no test.
suggestion.data-schemas.js:168-169 declare both with .allow(null).optional(). The .allow(null) branch (deliberate contract for backfill scenarios) has no coverage. Extend one existing CWV validateData case to pass these as null.

6. filterCwvMetrics reintroduces inline ['lcp', 'cls', 'inp'].
suggestion.projection-utils.js:57. With CWV_PER_METRIC_VALUES deleted, this is now duplicated with audit-worker/src/cwv/kpi-metrics.js's METRICS constant. Not a blocker - the producer enum is legitimately a superset of the consumer-projection enum so the duplication is defensible. But the deletion moves the situation back to where it was before this PR. Reintroduce as a file-local const CWV_METRICS = [...] so the next person editing the loop has a docstring anchor.

7. filterCwvMetrics wire shape narrows for per-metric suggestions.
Previously emitted {deviceType, lcp, cls: undefined, inp: undefined}. Now emits {deviceType, lcp}. Consumers using if ('cls' in metric) to test shape will observe a different result. Most reasonable consumers (reading metric.cls) are unaffected since they got undefined either way. Worth a note in the PR description so consumer teams can verify their reads.

Recommendations

• isCodeChangeAvailable cross-cutting pattern now on CWV + COLOR_CONTRAST + A11Y_ASSISTIVE + FORM_ACCESSIBILITY. On the fifth occurrence, extract a shared codeFixMetadata sub-schema. Not blocking this PR.
• Cross-repo enum duplication (audit-worker METRICS + data-access inline) is back where it was before this PR. Same recommendation as cycle 2: promote a shared constant to @adobe/spacecat-shared-utils/src/constants.js the next time a CWV-touching change lands in audit-worker.
• Audit-worker logging on Suggestion.validateData Joi rejections for CWV suggestions, so a misconfigured producer surfaces in a dashboard rather than silently in DDB. Out of scope here.
• Once the metric decision is settled (Important item 2), revisit the deleted describe('CWV per-metric suggestion', ...) block. The diff is preserved in a6fb59f^ if those tests should come back later.

Out of scope, worth tracking

• Producer-side METRICS = ['lcp', 'cls', 'inp'] in spacecat-audit-worker/src/cwv/kpi-metrics.js cannot be addressed by commits to this repo.
• ASO/Backoffice UI behavior when data.metric is missing or malformed depends on consumer code, not this PR. Worth confirming the downstream plan in a Slack thread before merge.
• Verify whether audit-worker is still writing data.metric to CWV suggestions. If yes, the field is a wasted write padding every record; if no, the PR description's integration test output (showing data.metric=inp) is stale.

Assessment

Ready to merge? With fixes.

Reasoning: the code change is small, self-consistent, and operationally safe to deploy (backward compatible, CI green, reversible). The open items are process and documentation, not the code itself: the PR description claims work that does not exist, the scope reversal has no recorded rationale, the atomic-vs-bundled contract is now an undocumented inference, and an open inline thread on the issues shape needs resolution.

Not asking for metric to be restored if the team has decided against it - asking for the decision to be made and recorded.

Next Steps

1. Update the PR description (and consider retitling) to reflect what a6fb59f actually ships. Add a one-paragraph rationale for the metric revert (Important item 1).
2. Decide on the atomic-vs-bundled contract: restore metric to schema+projection OR document the inference and add a defensive test (Important item 2).
3. Resolve the open issues schema thread with either an in-PR expansion or a linked follow-up issue (Important item 3).
4. Optional: address the two coverage gaps on isCodeChangeAvailable and jiraLink/aggregationKey null-acceptance in this PR while touching the test file (Minor items 4 and 5).