From dfdec8d62d56813c4353d8a68e5d84ba0a20d784 Mon Sep 17 00:00:00 2001 From: Cameron Dawson Date: Sun, 5 Jul 2026 10:08:22 -0700 Subject: [PATCH] Compute Failure Summary new-failure flags in render without mutating props FailureSummaryTab.render() mutated its own props on every render: it set selectedJob.newFailure and toggled suggestion.showNewButton on the suggestion objects while building the output. Deriving these during render makes the output a pure function of props/state. - Replace the mutation loop with an isNewFailureLine() predicate plus local newFailureCount and firstNewFailureIndex derivations. - Pass showNewButton to SuggestionsListItem as an explicit prop instead of mutating suggestion.showNewButton; the item reads it from props. - Add tests covering the new-failure banner count and single NEW flag. --- tests/ui/shared/FailureSummaryTab_test.jsx | 62 +++++++++++++++++++ .../tabs/failureSummary/FailureSummaryTab.jsx | 34 +++++----- .../failureSummary/SuggestionsListItem.jsx | 4 +- 3 files changed, 81 insertions(+), 19 deletions(-) diff --git a/tests/ui/shared/FailureSummaryTab_test.jsx b/tests/ui/shared/FailureSummaryTab_test.jsx index d5a22b88a6d..98bcdf35a2f 100644 --- a/tests/ui/shared/FailureSummaryTab_test.jsx +++ b/tests/ui/shared/FailureSummaryTab_test.jsx @@ -148,4 +148,66 @@ describe('FailureSummaryTab', () => { screen.getByTitle('Filter by test path: css/css-break/'), ).toBeInTheDocument(); }); + + describe('new failure lines', () => { + const newFailureSuggestion = (path, message) => ({ + search: `TEST-UNEXPECTED-FAIL | ${path} | ${message}`, + search_terms: [path], + path_end: path, + failure_new_in_rev: true, + bugs: { open_recent: [], all_others: [] }, + }); + + const mockSuggestions = (suggestions) => { + fetchMock.reset(); + fetchMock.get(getApiUrl('/jobs/?push_id=511138', repoName), selectedJob); + fetchMock.get( + getProjectUrl('/jobs/255514014/bug_suggestions/', repoName), + suggestions, + ); + }; + + test('banner counts every new failure line and flags only the first', async () => { + mockSuggestions([ + newFailureSuggestion('path/one.js', 'first new failure'), + newFailureSuggestion('path/two.js', 'second new failure'), + { + // Not a new failure: three-part search but not flagged new in rev. + ...newFailureSuggestion('path/three.js', 'not new'), + failure_new_in_rev: false, + }, + ]); + render(testFailureSummaryTab()); + + // Banner reflects the count of qualifying lines (2, not 3). + expect( + await screen.findByText(/2 new failure line\(s\)/), + ).toBeInTheDocument(); + + // Only the first qualifying line is flagged with the NEW button. + const newButtons = screen.getAllByTitle( + /number of times this error message has been seen/, + ); + expect(newButtons).toHaveLength(1); + }); + + test('renders no banner or NEW button when there are no new failure lines', async () => { + mockSuggestions([ + { + ...newFailureSuggestion('path/one.js', 'old failure'), + failure_new_in_rev: false, + }, + ]); + render(testFailureSummaryTab()); + + // Wait for the (non-new) failure line to render before asserting absence. + expect(await screen.findByText(/old failure/)).toBeInTheDocument(); + expect(screen.queryByText(/new failure line\(s\)/)).toBeNull(); + expect( + screen.queryByTitle( + /number of times this error message has been seen/, + ), + ).toBeNull(); + }); + }); }); diff --git a/ui/shared/tabs/failureSummary/FailureSummaryTab.jsx b/ui/shared/tabs/failureSummary/FailureSummaryTab.jsx index f6aeb2be243..085d5af4494 100644 --- a/ui/shared/tabs/failureSummary/FailureSummaryTab.jsx +++ b/ui/shared/tabs/failureSummary/FailureSummaryTab.jsx @@ -250,21 +250,18 @@ class FailureSummaryTab extends React.Component { const jobLogsAllParsed = logs.length > 0 && logs.every((jlu) => jlu.parse_status !== 'pending'); - selectedJob.newFailure = 0; - suggestions.forEach((suggestion) => { - suggestion.showNewButton = false; - // small hack here to use counter==0 and try for display only - if ( - suggestion.search.split(' | ').length === 3 && - (suggestion.failure_new_in_rev === true || - (suggestion.counter === 0 && currentRepo.name === 'try')) - ) { - if (selectedJob.newFailure === 0) { - suggestion.showNewButton = true; - } - selectedJob.newFailure++; - } - }); + // A "new failure" line has a three-part `search` and is either flagged new + // in the revision, or (on try) has never been seen before (counter === 0). + // Derived at render time rather than mutated onto the props so render stays + // a pure function of props/state. + const isNewFailureLine = (candidate) => + candidate.search.split(' | ').length === 3 && + (candidate.failure_new_in_rev === true || + (candidate.counter === 0 && currentRepo.name === 'try')); + + const newFailureCount = suggestions.filter(isNewFailureLine).length; + // Only the first new-failure line is flagged with the "NEW" button. + const firstNewFailureIndex = suggestions.findIndex(isNewFailureLine); return (
@@ -275,13 +272,13 @@ class FailureSummaryTab extends React.Component { ref={this.fsMount} id="failure-summary-scroll-area" > - {selectedJob.newFailure > 0 && ( + {newFailureCount > 0 && ( )} @@ -290,6 +287,7 @@ class FailureSummaryTab extends React.Component { key={`${selectedJob.id}-${index}`} // eslint-disable-line react/no-array-index-key index={index} suggestion={suggestion} + showNewButton={index === firstNewFailureIndex} toggleBugFiler={() => this.fileBug(suggestion)} toggleInternalIssueFiler={() => this.fileInternalIssue(suggestion) diff --git a/ui/shared/tabs/failureSummary/SuggestionsListItem.jsx b/ui/shared/tabs/failureSummary/SuggestionsListItem.jsx index 0ae19417cc1..3b0f0e95a1f 100644 --- a/ui/shared/tabs/failureSummary/SuggestionsListItem.jsx +++ b/ui/shared/tabs/failureSummary/SuggestionsListItem.jsx @@ -56,6 +56,7 @@ export default class SuggestionsListItem extends React.Component { addBug = null, currentRepo, developerMode, + showNewButton = false, } = this.props; const repoName = currentRepo.name; const { suggestionShowMore } = this.state; @@ -189,7 +190,7 @@ export default class SuggestionsListItem extends React.Component { - {suggestion.showNewButton && ( + {showNewButton && (