feat: strict schema for data.issues[] with per-issue lifecycle fields #1594
Conversation
tkotthakota-adobe
requested review from
naydav,
ramboz,
solaris007 and
tathagat2241
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
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
isCodeChangeAvailableis 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.
|
This PR will trigger no release when merged. |
|
Thanks for the review @solaris007 Addressed some of the feasible review comments.
|
|
@ramboz @solaris007 Re-requesting review as I updated the PR with main branch. |
| @@ -164,12 +173,13 @@ export const DATA_SCHEMAS = { | |||
| }).unknown(true), | |||
| ).required(), | |||
| issues: Joi.array().items(Joi.object()).optional().default([]), | |||
There was a problem hiding this comment.
If we start having strict requirements for the issues, we should probably expand the schema here to include those (type, value, patchContent, etc.)
There was a problem hiding this comment.
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
left a comment
solaris007
left a comment
There was a problem hiding this comment.
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_VALUESis exported frompackages/spacecat-shared-data-access/src/models/suggestion/suggestion.data-schemas.jsnear the other module constants, consumed byJoi.string().valid(...CWV_PER_METRIC_VALUES)at the CWV entry, and imported bysuggestion.projection-utils.jsforfilterCwvMetrics. Schema and transformer can no longer drift; addingttfb/fcpis the one-line change the doc comment advertises. - Behavior preserved in the incremental change. Both the prior
if (metric.lcp != null) ...chain and the newfor (const key of CWV_PER_METRIC_VALUES) if (m[key] != null)loop use the same!= nullguard against the same three keys, so output is identical for all three documented shapes (bundled, per-metric, group). - Tests remain valid.
chai.deep.equalis 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 parentunknown(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 (
metricvsmetricTypenaming) - pushback holds, dropped. Your counter-argument is sound:data.metricis already the wire name in audit-worker and downstream; theJoi.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_VALUESto@adobe/spacecat-shared-utils/src/constants.jswhen the next CWV-touching change in audit-worker lands. The same enum is duplicated asMETRICS = ['lcp', 'cls', 'inp']inspacecat-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 alongsideOPPORTUNITY_TYPES(whichsuggestion.data-schemas.jsalready imports). Best to do producer-side deletion in the same cross-repo PR rather than living in eventual-consistency limbo. isCodeChangeAvailableis now present on CWV plus COLOR_CONTRAST, A11Y_ASSISTIVE, and FORM_ACCESSIBILITY schemas. Four occurrences is the threshold to start tracking; a sharedcodeChangeFieldssub-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.jsassertingCWV_PER_METRIC_VALUEScontains 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
left a comment
solaris007
left a comment
There was a problem hiding this comment.
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
filterCwvMetricsrewrite (packages/spacecat-shared-data-access/src/models/suggestion/suggestion.projection-utils.js:46-65) is clean. Uses!= nullso0correctly passes through, has dedicated test coverage for null/undefined/zero. For per-metric suggestions this drops the previously-paddedcls: undefined, inp: undefinedkeys; for bundled suggestions output is unchanged.- CWV schema now declares
jiraLink,aggregationKey, andisCodeChangeAvailableexplicitly 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-removedmetricfield. 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 writedata.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 writingdata.metricin 120 of 126 returned suggestions today. The PR makes the contract laxer thanmainis today for a field that real production data is using. - A tier of consumers see different views: a client reading raw
datastill seesdata.metricviaunknown(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
metricasJoi.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) thatdata.metricis intentionally a producer-side concern with the authoritative enum inspacecat-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
isCodeChangeAvailablecross-cutting pattern now on CWV + COLOR_CONTRAST + A11Y_ASSISTIVE + FORM_ACCESSIBILITY. On the fifth occurrence, extract a sharedcodeFixMetadatasub-schema. Not blocking this PR.- Cross-repo enum duplication (
audit-workerMETRICS +data-accessinline) is back where it was before this PR. Same recommendation as cycle 2: promote a shared constant to@adobe/spacecat-shared-utils/src/constants.jsthe next time a CWV-touching change lands in audit-worker. - Audit-worker logging on
Suggestion.validateDataJoi rejections for CWV suggestions, so a misconfigured producer surfaces in a dashboard rather than silently in DDB. Out of scope here. - Once the
metricdecision is settled (Important item 2), revisit the deleteddescribe('CWV per-metric suggestion', ...)block. The diff is preserved ina6fb59f^if those tests should come back later.
Out of scope, worth tracking
- Producer-side
METRICS = ['lcp', 'cls', 'inp']inspacecat-audit-worker/src/cwv/kpi-metrics.jscannot be addressed by commits to this repo. - ASO/Backoffice UI behavior when
data.metricis 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.metricto CWV suggestions. If yes, the field is a wasted write padding every record; if no, the PR description's integration test output (showingdata.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
- Update the PR description (and consider retitling) to reflect what
a6fb59factually ships. Add a one-paragraph rationale for the metric revert (Important item 1). - Decide on the atomic-vs-bundled contract: restore
metricto schema+projection OR document the inference and add a defensive test (Important item 2). - Resolve the open
issuesschema thread with either an in-PR expansion or a linked follow-up issue (Important item 3). - Optional: address the two coverage gaps on
isCodeChangeAvailableandjiraLink/aggregationKeynull-acceptance in this PR while touching the test file (Minor items 4 and 5).
|
Hi @solaris007 Those hard codings of lcs/cls/inp are existing code. When metric is reverted changes made to use enum got reverted. I will rework on the PR as we want to add specific attributed to issue object as @ramboz commented. |
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @tkotthakota-adobe,
5th re-review cycle. The code state is unchanged from cycle 4, and you've signaled you'll rework before merge, so this pass focuses on direction for the rework rather than relitigating prior findings. Two of the three Important items are new architectural observations (cross-schema asymmetry, orphaned transformer rationale); the third is the stale PR description from cycle 4 that still applies.
Strengths
filterCwvMetricsrewrite atsuggestion.projection-utils.js:55-61uses!= nullcorrectly, preserving zero as a valid CWV measurement. The accompanying test atsuggestion.projection-utils.test.js:88-97pins this against future "simplify with truthy check" regressions.- Test rename at
suggestion.projection-utils.test.js:41("filters metrics to essential CWV fields (bundled suggestion)") makes the previously-implicit shape assumption explicit. isCodeChangeAvailable: Joi.boolean().optional()atsuggestion.data-schemas.js:167is a net tightening - the field was previously slipping through.unknown(true)with no type enforcement.- Substantive engagement with ramboz's open thread on
suggestion.data-schemas.js:166and explicit commitment to rework rather than push through. Constructive disposition for a contract several consumers depend on.
Issues
Important (Should Fix)
1. isCodeChangeAvailable added to CWV without patchContent breaks cross-schema symmetry at suggestion.data-schemas.js:167
Peer schemas in the same file pair the two fields together: COLOR_CONTRAST, A11Y_ASSISTIVE, and FORM_ACCESSIBILITY all declare patchContent: Joi.string().optional() alongside isCodeChangeAvailable: Joi.boolean().optional(). The semantic contract across the platform is "code change available implies patch content exists." CWV is now the only schema where the boolean is declared without the payload it advertises.
Either CWV legitimately does not produce patch content (in which case the boolean is misleading and a different signal is needed) or it does and patchContent is missing. Resolve before merge so downstream consumers (ASO UI, Backoffice) don't have to special-case CWV.
2. filterCwvMetrics rewrite has orphaned rationale at suggestion.projection-utils.js:48-50
The JSDoc reads "Handles both bundled (all metrics present) and per-metric (single metric) formats. Omits null/undefined metric values to keep the payload clean for per-metric suggestions." After a6fb59f reverted the metric field, the schema does not define a per-metric shape - "per-metric" is no longer a concept the schema acknowledges. Two consequences:
- The behavioral change affects bundled records too: any bundled record where a metric is explicitly
null(real measurement gap) previously surfaced as"lcp": nullon the wire and now surfaces as the key being absent. Consumers using'lcp' in metric(key-existence checks) see a contract change. - A future reader cannot trace what "per-metric" means in the JSDoc - no schema home, no documented producer.
Fix options: (a) revert the transformer rewrite alongside the metric revert and ship only isCodeChangeAvailable in this PR, or (b) keep the rewrite but rewrite the JSDoc to drop the per-metric framing and justify null-omission on its own merits (payload size, "unmeasured" semantics). Either is fine; the current orphaned-rationale state is not.
3. PR description, title, and embedded test-script output are stale vs current HEAD a6fb59f (re-raise from cycle 4)
The description still claims "Added metric and isCodeChangeAvailable to CWV Joi schema. Added metric to minimal projection" and the embedded test-script output validates data.metric=inp. After the revert, only isCodeChangeAvailable and the null/zero transformer behavior survive. Title "support atomic suggestions" is now misleading - there is no atomic-suggestion mechanism in the schema. Description text becomes part of the squash-merge commit message permanently, and downstream automation (release notes, audit history) reads PR bodies. Cheap and high-value: rewrite the description and title to match what merges.
Minor (Nice to Have)
4. isCodeChangeAvailable has no CWV-block validation test at suggestion.data-schemas.js:167
Cycle 4 minor, re-raise. The COLOR_CONTRAST and A11Y_ASSISTIVE test fixtures include isCodeChangeAvailable. The CWV fixtures do not. If the field is renamed or removed, no CWV test fails. One-line fixture addition closes the gap.
5. Wire-shape change in filterCwvMetrics lacks contract assertion at suggestion.projection-utils.js:54-63
Pre-change, every entry had all four keys {deviceType, lcp, inp, cls} present (some possibly undefined). Post-change, keys with null/undefined values are absent. Consumers using Object.keys(metric).length === 4 or 'cls' in metric see different behavior. Either add a JSDoc note documenting the contract, or tighten the "omits null/undefined" test with expect(result[0]).to.not.have.any.keys('cls', 'inp') so key-absence (not just value-absence) is pinned.
Recommendations
- For the rework: ramboz's open thread plus your 14:46 commitment to add
type/value/patchContent(and possiblyid/status) to theissuesshape converges with what the peer accessibility schemas already do - COLOR_CONTRAST, A11Y_ASSISTIVE, FORM_ACCESSIBILITY all have rich typedissues[*]objects withwcagLevel,severity,occurrences,htmlWithIssues,failureSummary, etc. Aligning CWV with that pattern brings four "issue-bearing" schemas onto a shared mental model and lets consumers write one rendering path. - Forward-looking (3-5 year): consider splitting CWV into
CWV_PAGEandCWV_GROUPopportunity types. The "two shapes one schema" pattern is unique to CWV inDATA_SCHEMASand the multi-shape contract is currently advisory (cannot be enforced by.unknown(true)plus optional fields). Distinct opportunity-type keys would let each shape have a clean, validated schema, distinct projections, and independent evolution. Not blocking this PR; worth discussing with audit-worker owners as the platform accumulates more CWV variants (TTFB, FCP, per-metric atomic suggestions). - Out of scope, worth tracking: the producer side in
spacecat-audit-workerwill need a coordinated change for any of (1)patchContentaddition, (2) typedissuesitems, or (3) opportunity-type split. Belongs in a follow-up issue/spec, not this PR.
Assessment
Ready to merge? No, with fixes.
Reasoning: You've signaled the rework already. The three Important items (cross-schema asymmetry, orphaned rationale, stale description) should land as part of that rework rather than as a separate cycle. CI is green and no security or functional regressions are flagged. The architecture recommendations frame direction; the Important items are blocking for what merges.
Next Steps
- Address the three Important items in the rework: declare
patchContentalongsideisCodeChangeAvailable(or remove the boolean if patch content does not apply to CWV), fix or remove the per-metric framing in thefilterCwvMetricsJSDoc, rewrite the PR description and title to match what ships. - Apply the
issuesshape expansion per ramboz's open thread. - Close the two minor coverage gaps (
isCodeChangeAvailableCWV fixture, wire-shape assertion) while touching the test file.
tkotthakota-adobe
changed the title
|
Hi @solaris007 @ramboz Updated the PR with new strict fields in issue. |
| * to avoid a circular import — the model already imports DATA_SCHEMAS from this file. | ||
| * Keep in sync if either set ever changes. | ||
| */ | ||
| export const ISSUE_STATUSES = [ |
There was a problem hiding this comment.
We need to clarify: are these statuses specifically CWV issue statuses, or fixEntity statuses that are linked to that issues?
There was a problem hiding this comment.
These are per issue statues.
There was a problem hiding this comment.
@tkotthakota-adobe
If these are issue statuses, what do you think about:
PATCH_FAILED_ERROR
PATCH_FAILED_NO_PATCH
They feel a bit different in meaning
There was a problem hiding this comment.
These are modeled based on suggestion statuses.
I think ERROR would suffice. IMHO those are more in technical nature so adding a log for dev team to understand might be enough. Unless we see the value to end user or ECE on spelling out what exactly happended.
tkotthakota-adobe
dismissed stale reviews from solaris007 and tathagat2241
via
35a6b45
## [@adobe/spacecat-shared-data-access-v3.64.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-data-access-v3.63.0...@adobe/spacecat-shared-data-access-v3.64.0) (2026-05-15) ### Features * strict schema for data.issues[] with per-issue lifecycle fields ([#1594](#1594)) ([d101db4](d101db4))
|
🎉 This PR is included in version @adobe/spacecat-shared-data-access-v3.64.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
https://jira.corp.adobe.com/browse/SITES-44229
Summary
Tightens the CWV
data.issues[]schema fromJoi.object()(anything goes) to a strict shape that supports per-issue lifecycle tracking. Foundational change for upcoming per-issue status, Jira linking, and FixEntity binding work; UI/Mystique/autofix-worker land later in follow-ups.What changed
packages/spacecat-shared-data-access/src/models/suggestion/suggestion.data-schemas.jsCWV_ISSUE_SCHEMA) with fields:idtype(lcp/cls/inp),title,value,cwvValuepatchContent,isCodeChangeAvailablestatus,skipReason,skipDetailjiraLink,fixEntityIdCWV_METRIC_TYPES = ['lcp', 'cls', 'inp']ISSUE_STATUSES(duplicate ofSuggestion.STATUSES; avoids circular import)ISSUE_SKIP_REASONS(duplicate ofSuggestion.SKIP_REASONS)issues: Joi.array().items(CWV_ISSUE_SCHEMA)packages/spacecat-shared-data-access/src/models/suggestion/index.jsCWV_METRIC_TYPES,ISSUE_STATUSES,ISSUE_SKIP_REASONSfrom the suggestion barrel so audit-worker / api-service / autofix-worker / Mystique can validate against the same source of truth.packages/spacecat-shared-data-access/test/unit/models/suggestion/suggestion.model.test.jsper-issue lifecycle fields:source_index,semantic_type) ride along viaunknown(true)jiraLinkallowedBackward compatibility
issues: [], legacy issues withoutid/status, or current Mystique output (withsource_indexetc.) all continue to validate.unknown(true)preserves forward-compat for Mystique-internal fields.Test plan
packages/spacecat-shared-data-access/src/models/suggestion/import { CWV_METRIC_TYPES, ISSUE_STATUSES, ISSUE_SKIP_REASONS } from '@adobe/spacecat-shared-data-access'Follow-ups (separate PRs, not in this one)
id(UUID v4) andstatus: 'NEW'per issuestatus/fixEntityIdafter PR creation