From d93129cc611e23ad8d6063136e20b585082b0d9e Mon Sep 17 00:00:00 2001 From: Manolescu Razvan Date: Wed, 1 Apr 2026 14:07:22 +0300 Subject: [PATCH 1/3] feat: store accessibility code patches in FixEntity instead of suggestion data Replace direct suggestion.data.patchContent writes with FixEntity creation in processCodeFixUpdates. Each code fix now creates a FixEntity with the patch in changeDetails and links matched suggestions via the junction table. This eliminates duplicate patch content across suggestions sharing an aggregationKey, resolving 413 errors from payload bloat. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../spec.md | 178 ++++++++++++++++++ src/common/codefix-handler.js | 148 ++++++++------- 2 files changed, 256 insertions(+), 70 deletions(-) create mode 100644 docs/specs/accessibility-fix-entity-migration/spec.md diff --git a/docs/specs/accessibility-fix-entity-migration/spec.md b/docs/specs/accessibility-fix-entity-migration/spec.md new file mode 100644 index 0000000000..4c7d42a15f --- /dev/null +++ b/docs/specs/accessibility-fix-entity-migration/spec.md @@ -0,0 +1,178 @@ +# Accessibility Fix Entity Migration — spacecat-audit-worker + +**Created:** 2026-04-01 +**Status:** Draft + +--- + +## Feature Overview + +Accessibility (non-forms) code patches are currently stored directly in `Suggestion.data` (`patchContent` / `isCodeChangeAvailable`). Multiple suggestions can share the same code fix via `aggregationKey`, but because each suggestion carries its own copy of the patch, this creates duplicates and causes 413 errors. This migration stores patches in FixEntity records and links them to suggestions via the existing junction table. + +### Why + +The 1-to-M relationship between a code fix and its suggestions is not modeled with distinct DB entities. The `aggregationKey` maps multiple suggestions to the same fix, but each suggestion duplicates the patch content. This causes payload bloat and HTTP 413 responses. + +### Success Criteria + +- [ ] FixEntity records are created with embedded patch content in `changeDetails` for new accessibility code fixes +- [ ] Suggestions are linked to their FixEntity via `setSuggestionsForFixEntity()` +- [ ] `patchContent` and `isCodeChangeAvailable` are no longer written to suggestion.data (immediate, no dual-write) +- [ ] `handleAccessibilityRemediationGuidance` continues to work unchanged (it handles guidance, not patches) +- [ ] Existing tests continue to pass + +--- + +## What This Repo Does + +This repo runs the code fix processing pipeline for accessibility audits. The key change is in `processCodeFixUpdates()` in `codefix-handler.js`, which currently: + +1. Reads code fix reports from S3 +2. Matches suggestions by `aggregationKey` +3. Writes `patchContent` and `isCodeChangeAvailable` into each matched suggestion's data + +After this change, step 3 becomes: +3. Creates a FixEntity with the patch in `changeDetails` +4. Links all matched suggestions to that FixEntity via the junction table + +The `handleAccessibilityRemediationGuidance` function is unaffected — it enriches suggestion `htmlWithIssues` with Mystique guidance (generalSuggestion, updateTo, userImpact), which is separate from code patches. + +--- + +## Requirements + +1. **Create FixEntity instead of writing to suggestion.data** + - When `processCodeFixUpdates()` processes a code fix report, create a FixEntity with type `CODE_CHANGE`, status `PENDING`, and `changeDetails` containing the diff + - Acceptance: FixEntity exists in DB with correct fields after code fix processing + +2. **Link suggestions to FixEntity** + - All suggestions matching the aggregationKey must be linked to the FixEntity + - Acceptance: `GET /fixes/:fixId/suggestions` returns the matched suggestions + +3. **Stop writing patchContent to suggestion.data** + - Do not set `patchContent` or `isCodeChangeAvailable` on suggestion data for new accessibility fixes + - Acceptance: New suggestions have no `patchContent` in their data object + +4. **Do not change handleAccessibilityRemediationGuidance** + - This function handles Mystique guidance enrichment, not code patches — it must remain unchanged + - Acceptance: Existing guidance tests pass without modification + +--- + +## Data Flow + +``` +S3 Code Fix Report + │ + ▼ +processCodeFixUpdates() + │ + ├─ Read report.json from S3 (contains diff) + ├─ Match suggestions by aggregationKey + │ + ▼ (BEFORE - current) + suggestion.setData({ patchContent: diff, isCodeChangeAvailable: false }) + + ▼ (AFTER - new) + opportunity.addFixEntities([{ + type: 'CODE_CHANGE', + status: 'PENDING', + changeDetails: { patchContent: diff, aggregationKey, description } + }]) + │ + ▼ + FixEntityDataAccess.setSuggestionsForFixEntity(opportunityId, fixEntity, matchedSuggestions) +``` + +--- + +## Implementation Tasks + +### Task 1: Create FixEntity in processCodeFixUpdates + +- **Description:** Replace the suggestion data update with FixEntity creation. After matching suggestions by aggregationKey, build a FixEntity payload and add it to the opportunity. Then link matched suggestions. +- **Files:** + - `src/common/codefix-handler.js` — `processCodeFixUpdates()` (lines ~164-372) + - Specifically the block around lines 164-168 where `patchContent` is set on suggestion data + - The aggregationKey matching block (lines ~265-315) +- **Dependencies:** FixEntity and FixEntitySuggestion models from `@adobe/spacecat-shared-data-access` (already imported in `src/utils/data-access.js`) +- **Testing:** Verify FixEntity created, suggestions linked, suggestion.data clean + +### Task 2: Structure changeDetails + +- **Description:** Build the `changeDetails` object for the FixEntity: + ```javascript + { + description: `Accessibility code fix for ${aggregationKey}`, + patchContent: reportData.diff, + aggregationKey, + } + ``` +- **Files:** `src/common/codefix-handler.js` +- **Dependencies:** Task 1 +- **Testing:** Verify changeDetails shape + +### Task 3: Verify handleAccessibilityRemediationGuidance unchanged + +- **Description:** Review `handleAccessibilityRemediationGuidance` to confirm it has no coupling to `patchContent`. It should only enrich `htmlWithIssues` with guidance objects. No code changes expected. +- **Files:** `src/accessibility/utils/generate-individual-opportunities.js` (lines ~1011-1191) — review only +- **Dependencies:** None +- **Testing:** Run existing tests + +### Task 4: Unit tests (after implementation stabilizes) + +- **Description:** Write/update unit tests for the modified codefix-handler. Wait for implementation to stabilize before writing tests. +- **Files:** `test/common/codefix-handler.test.js` (or equivalent) +- **Dependencies:** Tasks 1-3 +- **Testing:** `npm test` + +--- + +## Code Patterns + +**Existing FixEntity creation pattern** (from `src/utils/data-access.js`, lines ~450-535): + +```javascript +// This repo already uses FixEntity in reconcileDisappearedSuggestions: +const fixPayload = buildFixEntityPayload(suggestion); +await opportunity.addFixEntities([fixPayload]); +``` + +**Existing setSuggestionsForFixEntity usage** (from spacecat-shared): + +```javascript +await FixEntityDataAccess.setSuggestionsForFixEntity( + opportunityId, + createdFixEntity, + matchedSuggestions, // array of Suggestion models +); +``` + +Follow these existing patterns for consistency. + +--- + +## Testing Requirements + +- Wait for implementation to stabilize before writing unit tests +- Run ALL existing tests to verify backwards compatibility: `npm test` +- Key test scenarios: + - FixEntity created with correct `type`, `status`, `changeDetails` + - Suggestions linked via junction table + - Suggestion.data does NOT contain `patchContent` for new fixes + - `handleAccessibilityRemediationGuidance` unaffected + +--- + +## Dependencies on Other Repos + +- **experience-success-studio-ui** — must be updated AFTER this repo to consume FixEntity data. The UI has a backwards-compatible fallback to `suggestion.data.patchContent` for pre-migration suggestions. + +--- + +## Risks + +| Risk | Impact | Mitigation | +|------|--------|------------| +| UI briefly shows no patches for new suggestions | Low | UI deploys shortly after; backwards compat fallback handles transition | +| FixEntity creation fails silently | Medium | Add logging around `addFixEntities` call; monitor in Coralogix | diff --git a/src/common/codefix-handler.js b/src/common/codefix-handler.js index 2935a7e441..89cb5beb1c 100644 --- a/src/common/codefix-handler.js +++ b/src/common/codefix-handler.js @@ -106,18 +106,18 @@ async function readCodeChangeReport(s3Client, bucketName, reportKey, log) { } /** - * Updates suggestions with code change data + * Finds suggestions that match the given code change criteria. * @param {Array} suggestions - Array of suggestion objects * @param {string} url - The page URL to match * @param {string} source - The source to match (optional) * @param {string} matchKey - The key to match (aggregation_key or ruleId from type) - * @param {Object} reportData - The code change report data + * @param {Object} reportData - The code change report data (must have a diff) * @param {boolean} useAggregationKey - If true, build aggregation key from suggestion data; * if false, use issue type (ruleId) * @param {Object} log - Logger instance - * @returns {Promise} - Array of updated suggestions + * @returns {Array} - Array of matching suggestions */ -async function updateSuggestionsWithCodeChange( +function findMatchingSuggestions( suggestions, url, source, @@ -126,58 +126,40 @@ async function updateSuggestionsWithCodeChange( useAggregationKey, log, ) { - const updatedSuggestions = []; - - try { - for (const suggestion of suggestions) { - const suggestionData = suggestion.getData(); - - // Check if this suggestion matches the criteria - const suggestionUrl = suggestionData.url; - const suggestionSource = suggestionData.source; - - let suggestionMatchKey; - let suggestionsMatch = false; - - if (useAggregationKey) { - const issueType = suggestionData.issues?.[0]?.type || ''; - const targetSelector = suggestionData.issues?.[0]?.htmlWithIssues?.[0]?.target_selector || ''; - suggestionMatchKey = buildAggregationKey( - issueType, - suggestionUrl, - targetSelector, - suggestionSource, - ); - suggestionsMatch = suggestionMatchKey === matchKey && !!reportData.diff; - } else { - suggestionMatchKey = buildKey(suggestionUrl, suggestionSource); - const issueType = suggestionData.issues?.[0]?.type; - suggestionsMatch = suggestionMatchKey === buildKey(url, source) - && issueType === matchKey - && !!reportData.diff; - } - - if (suggestionsMatch) { - log.info(`Updating suggestion ${suggestion.getId()} with code change data`); - - // Update suggestion data with diff content and availability flag - const updatedData = { - ...suggestionData, - patchContent: reportData.diff, - isCodeChangeAvailable: false, - }; - - suggestion.setData(updatedData); - updatedSuggestions.push(suggestion); - } + const matched = []; + + for (const suggestion of suggestions) { + const suggestionData = suggestion.getData(); + const suggestionUrl = suggestionData.url; + const suggestionSource = suggestionData.source; + + let suggestionsMatch = false; + + if (useAggregationKey) { + const issueType = suggestionData.issues?.[0]?.type || ''; + const targetSelector = suggestionData.issues?.[0]?.htmlWithIssues?.[0]?.target_selector || ''; + const suggestionMatchKey = buildAggregationKey( + issueType, + suggestionUrl, + targetSelector, + suggestionSource, + ); + suggestionsMatch = suggestionMatchKey === matchKey && !!reportData.diff; + } else { + const suggestionMatchKey = buildKey(suggestionUrl, suggestionSource); + const issueType = suggestionData.issues?.[0]?.type; + suggestionsMatch = suggestionMatchKey === buildKey(url, source) + && issueType === matchKey + && !!reportData.diff; } - log.info(`Matched ${updatedSuggestions.length} suggestions with code change data`); - return updatedSuggestions; - } catch (error) { - log.error(`Error updating suggestions with code change: ${error.message}`, error); - throw error; + if (suggestionsMatch) { + matched.push(suggestion); + } } + + log.info(`Matched ${matched.length} suggestions for key: ${matchKey}`); + return matched; } /** @@ -202,7 +184,7 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont const { log, dataAccess, s3Client, env, } = context; - const { Opportunity, Suggestion } = dataAccess; + const { Opportunity } = dataAccess; // Validation if (!opportunityId) { @@ -243,9 +225,9 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont // Default bucket name from environment const defaultBucketName = env.S3_MYSTIQUE_BUCKET_NAME; - const allUpdatedSuggestions = []; + const fixEntitiesToCreate = []; - // Process each update (S3 reads run in parallel, saves are batched at the end) + // Process each update (S3 reads run in parallel, fix entities batched at the end) await Promise.all(updates.map(async (update) => { const { url, @@ -300,8 +282,8 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont return; } - // Update matching suggestions in-memory (no save yet) - const updatedSuggestions = await updateSuggestionsWithCodeChange( + // Find matching suggestions and create a FixEntity for this code fix + const matchedSuggestions = findMatchingSuggestions( suggestions, url, source, @@ -310,7 +292,19 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont true, // useAggregationKey = true for new format log, ); - allUpdatedSuggestions.push(...updatedSuggestions); + + if (matchedSuggestions.length > 0) { + fixEntitiesToCreate.push({ + type: 'CODE_CHANGE', + status: 'PENDING', + changeDetails: { + description: `Accessibility code fix for ${aggregationKey}`, + patchContent: reportData.diff, + aggregationKey, + }, + suggestions: matchedSuggestions.map((s) => s.getId()), + }); + } return; } @@ -344,8 +338,8 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont return; } - // Update matching suggestions in-memory (no save yet) - const updatedSuggestions = await updateSuggestionsWithCodeChange( + // Find matching suggestions and create a FixEntity for this code fix + const matchedSuggestions = findMatchingSuggestions( suggestions, url, source, @@ -354,19 +348,33 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont false, // useAggregationKey = false for old format (use ruleId) log, ); - allUpdatedSuggestions.push(...updatedSuggestions); + + if (matchedSuggestions.length > 0) { + fixEntitiesToCreate.push({ + type: 'CODE_CHANGE', + status: 'PENDING', + changeDetails: { + description: `Code fix for ${ruleId}`, + patchContent: reportData.diff, + ruleId, + }, + suggestions: matchedSuggestions.map((s) => s.getId()), + }); + } })); })); - // Deduplicate (a suggestion may match multiple updates) and batch-save - const uniqueSuggestions = [...new Map( - allUpdatedSuggestions.map((s) => [s.getId(), s]), - ).values()]; + // Create all FixEntities with their linked suggestions in a single batch + let totalFixEntities = 0; + if (fixEntitiesToCreate.length > 0) { + const { createdItems, errorItems } = await opportunity.addFixEntities(fixEntitiesToCreate); + totalFixEntities = createdItems.length; - if (uniqueSuggestions.length > 0) { - await Suggestion.saveMany(uniqueSuggestions); + if (errorItems.length > 0) { + log.warn(`[CodeFixProcessor] ${errorItems.length} fix entities failed to create: ${errorItems.map((e) => e.error.message).join(', ')}`); + } } - log.info(`[CodeFixProcessor] Successfully processed all updates. Total suggestions updated: ${uniqueSuggestions.length}`); - return uniqueSuggestions.length; + log.info(`[CodeFixProcessor] Successfully processed all updates. Total fix entities created: ${totalFixEntities}`); + return totalFixEntities; } From 628e2d2205986d699d45f7deb6d33e00c630d6c3 Mon Sep 17 00:00:00 2001 From: Manolescu Razvan Date: Wed, 1 Apr 2026 14:17:11 +0300 Subject: [PATCH 2/3] test: update codefix-handler tests for FixEntity creation Update all assertions from suggestion.setData/Suggestion.saveMany to opportunity.addFixEntities with correct FixEntity payload verification. Tests verify: FixEntity creation with changeDetails, suggestion linking, old format support, error handling, and edge cases. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../codefix-handler.test.js | 119 +++++++----------- 1 file changed, 44 insertions(+), 75 deletions(-) diff --git a/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js b/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js index 41021508e2..c63f2fea6f 100644 --- a/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js +++ b/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js @@ -49,15 +49,13 @@ describe('AccessibilityCodeFixHandler', () => { getId: sandbox.stub().returns('opportunity-123'), getSiteId: sandbox.stub().returns('site-123'), getSuggestions: sandbox.stub().resolves([mockSuggestion]), + addFixEntities: sandbox.stub().resolves({ createdItems: [], errorItems: [] }), }; mockDataAccess = { Opportunity: { findById: sandbox.stub().resolves(mockOpportunity), }, - Suggestion: { - saveMany: sandbox.stub().resolves(), - }, }; getObjectFromKeyStub = sandbox.stub(); @@ -137,19 +135,14 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(validMessage, context); expect(result.status).to.equal(200); - expect(mockSuggestion.setData).to.have.been.calledWith({ - url: 'https://example.com/contact', - source: 'form', - issues: [{ - type: 'button-name', - htmlWithIssues: [{ - target_selector: 'button.submit', - }], - }], - patchContent: 'mock diff content', - isCodeChangeAvailable: false, - }); - expect(mockDataAccess.Suggestion.saveMany).to.have.been.called; + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; + const fixEntities = mockOpportunity.addFixEntities.firstCall.args[0]; + expect(fixEntities).to.have.lengthOf(1); + expect(fixEntities[0].type).to.equal('CODE_CHANGE'); + expect(fixEntities[0].status).to.equal('PENDING'); + expect(fixEntities[0].changeDetails.patchContent).to.equal('mock diff content'); + expect(fixEntities[0].changeDetails.aggregationKey).to.equal('https://example.com/contact|button-name|form'); + expect(fixEntities[0].suggestions).to.deep.equal(['suggestion-123']); }); it('should return badRequest when no data provided', async () => { @@ -356,8 +349,7 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(validMessage, context); expect(result.status).to.equal(200); - expect(mockSuggestion.setData).not.to.have.been.called; - expect(mockSuggestion.save).not.to.have.been.called; + expect(mockOpportunity.addFixEntities).not.to.have.been.called; }); it('should not update suggestions without diff content', async () => { @@ -389,10 +381,10 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(validMessage, context); expect(result.status).to.equal(200); - expect(mockSuggestion.setData).not.to.have.been.called; + expect(mockOpportunity.addFixEntities).not.to.have.been.called; }); - it('should handle suggestion save errors', async () => { + it('should handle addFixEntities errors', async () => { const mockReportData = { diff: 'mock diff content', }; @@ -409,8 +401,8 @@ describe('AccessibilityCodeFixHandler', () => { }; mockSuggestion.getData.returns(suggestionData); - // Suggestion.saveMany rejects to simulate batch save failure - mockDataAccess.Suggestion.saveMany.rejects(new Error('Save failed')); + // addFixEntities rejects to simulate batch creation failure + mockOpportunity.addFixEntities.rejects(new Error('Fix entity creation failed')); getObjectFromKeyStub.resolves(mockReportData); const handler = await esmock('../../../../src/common/codefix-response-handler.js', { @@ -429,12 +421,12 @@ describe('AccessibilityCodeFixHandler', () => { ); }); - it('should handle errors in updateSuggestionsWithCodeChange when getData throws', async () => { + it('should handle errors in findMatchingSuggestions when getData throws', async () => { const mockReportData = { diff: 'mock diff content', }; - // Make getData throw to trigger the catch block in updateSuggestionsWithCodeChange + // Make getData throw to trigger an error during suggestion matching mockSuggestion.getData.throws(new Error('getData exploded')); getObjectFromKeyStub.resolves(mockReportData); @@ -449,9 +441,6 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(validMessage, context); expect(result.status).to.equal(500); - expect(context.log.error).to.have.been.calledWith( - sinon.match(/Error updating suggestions with code change/), - ); }); it('should handle processing errors gracefully', async () => { @@ -616,7 +605,7 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(messageWithoutSource, context); expect(result.status).to.equal(200); - expect(mockSuggestion.setData).to.have.been.called; + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; }); it('should use provided code_fix_path and code_fix_bucket when available', async () => { @@ -671,7 +660,7 @@ describe('AccessibilityCodeFixHandler', () => { 'custom/path/to/report.json', sinon.match.any, ); - expect(mockSuggestion.setData).to.have.been.called; + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; }); it('should support old format with type array (backwards compatible)', async () => { @@ -713,7 +702,11 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(messageOldFormat, context); expect(result.status).to.equal(200); - expect(mockSuggestion.setData).to.have.been.called; + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; + const fixEntities = mockOpportunity.addFixEntities.firstCall.args[0]; + expect(fixEntities[0].type).to.equal('CODE_CHANGE'); + expect(fixEntities[0].changeDetails.patchContent).to.equal('mock diff content'); + expect(fixEntities[0].changeDetails.ruleId).to.equal('color-contrast'); }); it('should work without source parameter for old format', async () => { @@ -754,8 +747,8 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(messageOldFormatNoSource, context); expect(result.status).to.equal(200); - expect(mockSuggestion.setData).to.have.been.called; - + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; + // Verify S3 call arguments const callArgs = getObjectFromKeyStub.firstCall.args; expect(callArgs[1]).to.equal('test-mystique-bucket'); @@ -819,8 +812,9 @@ describe('AccessibilityCodeFixHandler', () => { expect(result.status).to.equal(200); expect(getObjectFromKeyStub).to.have.been.calledTwice; - expect(mockSuggestion.setData).to.have.been.called; - expect(mockSuggestion2.setData).to.have.been.called; + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; + const fixEntities = mockOpportunity.addFixEntities.firstCall.args[0]; + expect(fixEntities).to.have.lengthOf(2); }); it('should prefer aggregation_key over type array when both present', async () => { @@ -868,7 +862,7 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(messageBothFormats, context); expect(result.status).to.equal(200); - expect(mockSuggestion.setData).to.have.been.called; + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; // Should only be called once for aggregation_key, not for type array expect(getObjectFromKeyStub).to.have.been.calledOnce; }); @@ -973,20 +967,11 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(message, context); expect(result.status).to.equal(200); - // JSON string should be parsed and suggestion should be updated - expect(mockSuggestion.setData).to.have.been.calledWith({ - url: 'https://example.com/contact', - source: 'form', - issues: [{ - type: 'button-name', - htmlWithIssues: [{ - target_selector: 'button.submit', - }], - }], - patchContent: mockDiffContent, - isCodeChangeAvailable: false, - }); - expect(mockDataAccess.Suggestion.saveMany).to.have.been.called; + // JSON string should be parsed and FixEntity should be created + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; + const fixEntities = mockOpportunity.addFixEntities.firstCall.args[0]; + expect(fixEntities[0].changeDetails.patchContent).to.equal(mockDiffContent); + expect(fixEntities[0].suggestions).to.deep.equal(['suggestion-123']); }); it('should update multiple suggestions with same aggregation_key', async () => { @@ -1062,32 +1047,16 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(message, context); expect(result.status).to.equal(200); - // Both suggestions should be updated - expect(mockSuggestion.setData).to.have.been.calledWith({ - url: 'https://example.com/page1', - source: 'form1', - issues: [{ - type: 'aria-prohibited-attr', - htmlWithIssues: [{ - target_selector: 'div[aria-hidden]', - }], - }], - patchContent: 'mock diff content for aria-prohibited-attr', - isCodeChangeAvailable: false, - }); - expect(mockSuggestion2.setData).to.have.been.calledWith({ - url: 'https://example.com/page2', - source: 'form2', - issues: [{ - type: 'aria-prohibited-attr', - htmlWithIssues: [{ - target_selector: 'span.label', - }], - }], - patchContent: 'mock diff content for aria-prohibited-attr', - isCodeChangeAvailable: false, - }); - expect(mockDataAccess.Suggestion.saveMany).to.have.been.called; + // Both suggestions should be linked to fix entities + expect(mockOpportunity.addFixEntities).to.have.been.calledOnce; + const fixEntities = mockOpportunity.addFixEntities.firstCall.args[0]; + // Two updates with same PER_TYPE aggregation_key each match both suggestions + expect(fixEntities).to.have.lengthOf(2); + expect(fixEntities[0].changeDetails.patchContent).to.equal('mock diff content for aria-prohibited-attr'); + expect(fixEntities[0].suggestions).to.include('suggestion-123'); + expect(fixEntities[0].suggestions).to.include('suggestion-456'); + expect(fixEntities[1].suggestions).to.include('suggestion-123'); + expect(fixEntities[1].suggestions).to.include('suggestion-456'); }); it('should throw CodeFixConfigurationError when S3_MYSTIQUE_BUCKET_NAME is not set (new format)', async () => { From f5978faaab36243646fe66d207584b75ad23c260 Mon Sep 17 00:00:00 2001 From: Manolescu Razvan Date: Wed, 1 Apr 2026 15:17:43 +0300 Subject: [PATCH 3/3] fix: restore error isolation in codefix-handler - Add per-suggestion try/catch in findMatchingSuggestions so one bad suggestion does not abort all matching - Use Promise.allSettled for update processing so partial results are preserved when individual updates fail - Early-exit bucket validation before the loop so CodeFixConfigurationError still surfaces as a fatal error Co-Authored-By: Claude Opus 4.6 (1M context) --- src/common/codefix-handler.js | 70 ++++++++++++------- .../codefix-handler.test.js | 11 ++- 2 files changed, 51 insertions(+), 30 deletions(-) diff --git a/src/common/codefix-handler.js b/src/common/codefix-handler.js index 89cb5beb1c..73777eb4cf 100644 --- a/src/common/codefix-handler.js +++ b/src/common/codefix-handler.js @@ -129,32 +129,36 @@ function findMatchingSuggestions( const matched = []; for (const suggestion of suggestions) { - const suggestionData = suggestion.getData(); - const suggestionUrl = suggestionData.url; - const suggestionSource = suggestionData.source; - - let suggestionsMatch = false; - - if (useAggregationKey) { - const issueType = suggestionData.issues?.[0]?.type || ''; - const targetSelector = suggestionData.issues?.[0]?.htmlWithIssues?.[0]?.target_selector || ''; - const suggestionMatchKey = buildAggregationKey( - issueType, - suggestionUrl, - targetSelector, - suggestionSource, - ); - suggestionsMatch = suggestionMatchKey === matchKey && !!reportData.diff; - } else { - const suggestionMatchKey = buildKey(suggestionUrl, suggestionSource); - const issueType = suggestionData.issues?.[0]?.type; - suggestionsMatch = suggestionMatchKey === buildKey(url, source) - && issueType === matchKey - && !!reportData.diff; - } + try { + const suggestionData = suggestion.getData(); + const suggestionUrl = suggestionData.url; + const suggestionSource = suggestionData.source; + + let suggestionsMatch = false; + + if (useAggregationKey) { + const issueType = suggestionData.issues?.[0]?.type || ''; + const targetSelector = suggestionData.issues?.[0]?.htmlWithIssues?.[0]?.target_selector || ''; + const suggestionMatchKey = buildAggregationKey( + issueType, + suggestionUrl, + targetSelector, + suggestionSource, + ); + suggestionsMatch = suggestionMatchKey === matchKey && !!reportData.diff; + } else { + const suggestionMatchKey = buildKey(suggestionUrl, suggestionSource); + const issueType = suggestionData.issues?.[0]?.type; + suggestionsMatch = suggestionMatchKey === buildKey(url, source) + && issueType === matchKey + && !!reportData.diff; + } - if (suggestionsMatch) { - matched.push(suggestion); + if (suggestionsMatch) { + matched.push(suggestion); + } + } catch (error) { + log.warn(`Error matching suggestion ${suggestion?.getId?.()}: ${error.message}`); } } @@ -225,10 +229,22 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont // Default bucket name from environment const defaultBucketName = env.S3_MYSTIQUE_BUCKET_NAME; + // Fail early if any update requires the default bucket but it's not configured + const needsDefaultBucket = updates.some((update) => { + if (update.aggregation_key && update.code_fix_path && update.code_fix_bucket) return false; + if (!update.aggregation_key && !update.types) return false; + return true; + }); + if (needsDefaultBucket && !defaultBucketName) { + log.error('[CodeFixProcessor] S3_MYSTIQUE_BUCKET_NAME environment variable not set'); + throw new CodeFixConfigurationError('S3 bucket name not configured'); + } + const fixEntitiesToCreate = []; // Process each update (S3 reads run in parallel, fix entities batched at the end) - await Promise.all(updates.map(async (update) => { + // Use allSettled so a single update failure doesn't discard all other results + await Promise.allSettled(updates.map(async (update) => { const { url, source, @@ -322,7 +338,7 @@ export async function processCodeFixUpdates(siteId, opportunityId, updates, cont log.info(`[CodeFixProcessor] Processing update (old format) for URL: ${url}, source: ${source || 'N/A'}, types: ${types.join(', ')}`); // For each type in the update, try to read the code change report - await Promise.all(types.map(async (ruleId) => { + await Promise.allSettled(types.map(async (ruleId) => { const urlSourceHash = generateUrlSourceHash(url, source || ''); const reportKey = `fixes/${siteId}/${urlSourceHash}/${ruleId}/report.json`; diff --git a/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js b/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js index c63f2fea6f..5e1c4737ff 100644 --- a/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js +++ b/test/audits/accessibility/auto-optimization-handlers/codefix-handler.test.js @@ -421,12 +421,12 @@ describe('AccessibilityCodeFixHandler', () => { ); }); - it('should handle errors in findMatchingSuggestions when getData throws', async () => { + it('should skip individual suggestions that throw errors during matching', async () => { const mockReportData = { diff: 'mock diff content', }; - // Make getData throw to trigger an error during suggestion matching + // Make getData throw to trigger the per-suggestion catch in findMatchingSuggestions mockSuggestion.getData.throws(new Error('getData exploded')); getObjectFromKeyStub.resolves(mockReportData); @@ -440,7 +440,12 @@ describe('AccessibilityCodeFixHandler', () => { const result = await handler.default(validMessage, context); - expect(result.status).to.equal(500); + // Should succeed — the bad suggestion is skipped, no fix entities created + expect(result.status).to.equal(200); + expect(context.log.warn).to.have.been.calledWith( + sinon.match(/Error matching suggestion/), + ); + expect(mockOpportunity.addFixEntities).not.to.have.been.called; }); it('should handle processing errors gracefully', async () => {