From 984204c93a65c2ff9a2b255e3717117b8b59e18f Mon Sep 17 00:00:00 2001 From: David Miculit Date: Mon, 3 Aug 2026 17:10:17 +0300 Subject: [PATCH 1/7] feat: optimize array mapping and sibling rerenders --- ui/perfherder/graphs/LegendCard.jsx | 78 ++++++++++++++++------------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/ui/perfherder/graphs/LegendCard.jsx b/ui/perfherder/graphs/LegendCard.jsx index 8ef6cdbfc74..077fd19aba0 100644 --- a/ui/perfherder/graphs/LegendCard.jsx +++ b/ui/perfherder/graphs/LegendCard.jsx @@ -1,4 +1,4 @@ - +import React from 'react'; import PropTypes from 'prop-types'; import { Badge, Button, Form, CloseButton } from 'react-bootstrap'; @@ -22,42 +22,47 @@ const LegendCard = ({ const newSymbols = [...symbols]; const errorMessages = []; let updates; - const newTestData = [...testData].map((item) => { - if (item.signature_id === series.signature_id) { - const isVisible = !item.visible; - - if (isVisible && newColors.length && newSymbols.length) { - item.color = newColors.pop(); - item.symbol = newSymbols.pop(); - item.visible = isVisible; - item.data = item.data.map((test) => ({ - ...test, - z: item.color[1], - _z: item.symbol, - })); - } else if (!isVisible) { - newColors.push(item.color); - newSymbols.push(item.symbol); - item.color = ['border-secondary', '']; - item.symbol = ['circle', 'outline']; - item.visible = isVisible; - item.data = item.data.map((test) => ({ - ...test, - z: item.color[1], - _z: item.symbol, - })); - } else { - errorMessages.push( - "The graph supports viewing 6 tests at a time. To select and view a test that isn't currently visible, first deselect a visible test", - ); - } - } - return item; - }); + const targetIndex = testData.findIndex((item) => item.signature_id === series.signature_id); + const item = testData[targetIndex]; + const isVisible = !item.visible; + let updatedItem = { ...item }; + + if (isVisible && newColors.length && newSymbols.length) { + updatedItem.color = newColors.pop(); + updatedItem.symbol = newSymbols.pop(); + updatedItem.visible = isVisible; + updatedItem.data = item.data.map((test) => ({ + ...test, + z: updatedItem.color[1], + _z: updatedItem.symbol, + })); + } else if (!isVisible) { + newColors.push(item.color); + newSymbols.push(item.symbol); + updatedItem.color = ['border-secondary', '']; + updatedItem.symbol = ['circle', 'outline']; + updatedItem.visible = isVisible; + updatedItem.data = item.data.map((test) => ({ + ...test, + z: updatedItem.color[1], + _z: updatedItem.symbol, + })); + } else { + errorMessages.push( + "The graph supports viewing 6 tests at a time. To select and view a test that isn't currently visible, first deselect a visible test", + ); + } if (errorMessages.length) { updates = { errorMessages, visibilityChanged: false }; } else { + // rebuild the array by slicing around the updated item + const newTestData = [ + ...testData.slice(0, targetIndex), + updatedItem, + ...testData.slice(targetIndex + 1), + ]; + updates = { testData: newTestData, colors: newColors, @@ -228,4 +233,9 @@ LegendCard.propTypes = { selectedDataPoint: PropTypes.shape({}), }; -export default LegendCard; +const areEqual = (prev, next) => + prev.series.signature_id === next.series.signature_id && + prev.series.visible === next.series.visible && + prev.colors === next.colors; + +export default React.memo(LegendCard, areEqual); \ No newline at end of file From f851a43ac4abfc99a34a612397403dab1a7fcf0e Mon Sep 17 00:00:00 2001 From: David Miculit Date: Fri, 7 Aug 2026 11:41:59 +0300 Subject: [PATCH 2/7] fix: areEqual logic --- ui/perfherder/graphs/LegendCard.jsx | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/ui/perfherder/graphs/LegendCard.jsx b/ui/perfherder/graphs/LegendCard.jsx index 077fd19aba0..e57b8e7127b 100644 --- a/ui/perfherder/graphs/LegendCard.jsx +++ b/ui/perfherder/graphs/LegendCard.jsx @@ -22,7 +22,9 @@ const LegendCard = ({ const newSymbols = [...symbols]; const errorMessages = []; let updates; - const targetIndex = testData.findIndex((item) => item.signature_id === series.signature_id); + const targetIndex = testData.findIndex( + (item) => item.signature_id === series.signature_id, + ); const item = testData[targetIndex]; const isVisible = !item.visible; let updatedItem = { ...item }; @@ -62,7 +64,7 @@ const LegendCard = ({ updatedItem, ...testData.slice(targetIndex + 1), ]; - + updates = { testData: newTestData, colors: newColors, @@ -234,8 +236,10 @@ LegendCard.propTypes = { }; const areEqual = (prev, next) => - prev.series.signature_id === next.series.signature_id && - prev.series.visible === next.series.visible && - prev.colors === next.colors; + prev.series === next.series && + prev.testData === next.testData && + prev.colors === next.colors && + prev.symbols === next.symbols && + prev.selectedDataPoint === next.selectedDataPoint; -export default React.memo(LegendCard, areEqual); \ No newline at end of file +export default React.memo(LegendCard, areEqual); From a5d4bb27fd09b99dbe873b40df1346c5721eb2ac Mon Sep 17 00:00:00 2001 From: David Miculit Date: Fri, 7 Aug 2026 11:46:11 +0300 Subject: [PATCH 3/7] fix: change updateItem from let to const as it is not modified --- ui/perfherder/graphs/LegendCard.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/perfherder/graphs/LegendCard.jsx b/ui/perfherder/graphs/LegendCard.jsx index e57b8e7127b..03ffaeb99e3 100644 --- a/ui/perfherder/graphs/LegendCard.jsx +++ b/ui/perfherder/graphs/LegendCard.jsx @@ -27,7 +27,7 @@ const LegendCard = ({ ); const item = testData[targetIndex]; const isVisible = !item.visible; - let updatedItem = { ...item }; + const updatedItem = { ...item }; if (isVisible && newColors.length && newSymbols.length) { updatedItem.color = newColors.pop(); From 31bdd1ba10deac7ab02919fd442161d3d0d445d3 Mon Sep 17 00:00:00 2001 From: David Miculit Date: Fri, 7 Aug 2026 17:45:25 +0300 Subject: [PATCH 4/7] fix: identity comparison correctly sees a change and minor visual bug with the card colors --- ui/perfherder/graphs/LegendCard.jsx | 48 +++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/ui/perfherder/graphs/LegendCard.jsx b/ui/perfherder/graphs/LegendCard.jsx index 03ffaeb99e3..f0743b091cd 100644 --- a/ui/perfherder/graphs/LegendCard.jsx +++ b/ui/perfherder/graphs/LegendCard.jsx @@ -113,24 +113,46 @@ const LegendCard = ({ // when removing a test, check to see if the next test in the queue had a color; // if it had secondary and was deselected, reset its color and visibility to // the removed test's color, otherwise push that color back into the colors list + + const promoteIndex = graphColors.length - 1; + if ( - newData[graphColors.length - 1] && - newData[graphColors.length - 1].color[0] === 'border-secondary' + newData[promoteIndex] && + newData[promoteIndex].color[0] === 'border-secondary' ) { - newData[graphColors.length - 1].color = series.color; - newData[graphColors.length - 1].visible = true; - newData[graphColors.length - 1].data = newData[ - graphColors.length - 1 - ].data.map((item) => ({ - ...item, - z: series.color[1], - })); - resetParams(newData); + const promoted = newData[promoteIndex]; + let nextColor; + let nextSymbol; + const newColors = [...colors]; + const newSymbols = [...symbols]; + + // if the removed test was disabled, its real color/symbol are in the pools. + if (series.color[0] === 'border-secondary') { + nextColor = newColors.pop(); + nextSymbol = newSymbols.pop(); + } else { + nextColor = series.color; + nextSymbol = series.symbol; + } + + newData[promoteIndex] = { + ...promoted, + color: nextColor, + symbol: nextSymbol, + visible: true, + data: promoted.data.map((item) => ({ + ...item, + z: nextColor[1], + _z: nextSymbol, + })), + }; + resetParams(newData, newColors, newSymbols); } else if (series.color[0] === 'border-secondary') { resetParams(newData); } else { - const newColors = [...colors, ...[series.color]]; - resetParams(newData, newColors); + const newColors = [...colors, series.color]; + const newSymbols = [...symbols, series.symbol]; + resetParams(newData, newColors, newSymbols); } }; From 2f56a1d52d514d6b711c744f63a7548b47a1fdd5 Mon Sep 17 00:00:00 2001 From: David Miculit Date: Tue, 11 Aug 2026 15:22:03 +0300 Subject: [PATCH 5/7] fix: redone broken removeTest logic --- ui/perfherder/graphs/LegendCard.jsx | 66 ++++++++++++----------------- 1 file changed, 27 insertions(+), 39 deletions(-) diff --git a/ui/perfherder/graphs/LegendCard.jsx b/ui/perfherder/graphs/LegendCard.jsx index f0743b091cd..b96cc10b0e2 100644 --- a/ui/perfherder/graphs/LegendCard.jsx +++ b/ui/perfherder/graphs/LegendCard.jsx @@ -3,7 +3,6 @@ import PropTypes from 'prop-types'; import { Badge, Button, Form, CloseButton } from 'react-bootstrap'; import { getFrameworkName } from '../perf-helpers/helpers'; -import { graphColors } from '../perf-helpers/constants'; import { Perfdocs } from '../perf-helpers/perfdocs'; import GraphIcon from '../../shared/GraphIcon'; @@ -110,50 +109,39 @@ const LegendCard = ({ newData.splice(index, 1); - // when removing a test, check to see if the next test in the queue had a color; - // if it had secondary and was deselected, reset its color and visibility to - // the removed test's color, otherwise push that color back into the colors list - - const promoteIndex = graphColors.length - 1; - - if ( - newData[promoteIndex] && - newData[promoteIndex].color[0] === 'border-secondary' - ) { - const promoted = newData[promoteIndex]; - let nextColor; - let nextSymbol; - const newColors = [...colors]; - const newSymbols = [...symbols]; + // removing a disabled test frees nothing, since it never held a + // color, just drop it. Removing a visible test frees its + // color: promote the first currently-disabled test to take its + // place, or return the color/symbol to the pool if none is waiting. + if (series.color[0] === 'border-secondary') { + resetParams(newData); + return; + } - // if the removed test was disabled, its real color/symbol are in the pools. - if (series.color[0] === 'border-secondary') { - nextColor = newColors.pop(); - nextSymbol = newSymbols.pop(); - } else { - nextColor = series.color; - nextSymbol = series.symbol; - } + const promoteIndex = newData.findIndex( + (item) => item.color[0] === 'border-secondary', + ); - newData[promoteIndex] = { - ...promoted, - color: nextColor, - symbol: nextSymbol, - visible: true, - data: promoted.data.map((item) => ({ - ...item, - z: nextColor[1], - _z: nextSymbol, - })), - }; - resetParams(newData, newColors, newSymbols); - } else if (series.color[0] === 'border-secondary') { - resetParams(newData); - } else { + if (promoteIndex === -1) { const newColors = [...colors, series.color]; const newSymbols = [...symbols, series.symbol]; resetParams(newData, newColors, newSymbols); + return; } + + const promoted = newData[promoteIndex]; + newData[promoteIndex] = { + ...promoted, + color: series.color, + symbol: series.symbol, + visible: true, + data: promoted.data.map((item) => ({ + ...item, + z: series.color[1], + _z: series.symbol, + })), + }; + resetParams(newData); }; const subtitleStyle = 'p-0 mb-0 border-0 text-secondary text-start'; From 96bfc9e2d26118fa954abd0bf3649836bc07b97f Mon Sep 17 00:00:00 2001 From: David Miculit Date: Thu, 20 Aug 2026 15:09:05 +0300 Subject: [PATCH 6/7] feat: change to using the refs and revert the removeTest logic to match the old one --- ui/perfherder/graphs/GraphsView.jsx | 6 +- ui/perfherder/graphs/LegendCard.jsx | 101 +++++++++++++++------------- 2 files changed, 58 insertions(+), 49 deletions(-) diff --git a/ui/perfherder/graphs/GraphsView.jsx b/ui/perfherder/graphs/GraphsView.jsx index 824f2dcfb60..9532798bf2c 100644 --- a/ui/perfherder/graphs/GraphsView.jsx +++ b/ui/perfherder/graphs/GraphsView.jsx @@ -560,7 +560,7 @@ function GraphsView({ projects, frameworks, user }) { > diff --git a/ui/perfherder/graphs/LegendCard.jsx b/ui/perfherder/graphs/LegendCard.jsx index b96cc10b0e2..725a4c0f1d3 100644 --- a/ui/perfherder/graphs/LegendCard.jsx +++ b/ui/perfherder/graphs/LegendCard.jsx @@ -3,27 +3,31 @@ import PropTypes from 'prop-types'; import { Badge, Button, Form, CloseButton } from 'react-bootstrap'; import { getFrameworkName } from '../perf-helpers/helpers'; +import { graphColors } from '../perf-helpers/constants'; import { Perfdocs } from '../perf-helpers/perfdocs'; import GraphIcon from '../../shared/GraphIcon'; const LegendCard = ({ series, - testData = [], + testDataRef, updateState, updateStateParams, selectedDataPoint = null, frameworks, - colors, - symbols, + colorsRef, + symbolsRef, }) => { const updateSelectedTest = () => { - const newColors = [...colors]; - const newSymbols = [...symbols]; + const testData = testDataRef.current; + const newColors = [...colorsRef.current]; + const newSymbols = [...symbolsRef.current]; + const errorMessages = []; let updates; const targetIndex = testData.findIndex( (item) => item.signature_id === series.signature_id, ); + if (targetIndex === -1) return; const item = testData[targetIndex]; const isVisible = !item.visible; const updatedItem = { ...item }; @@ -100,48 +104,51 @@ const LegendCard = ({ }; const removeTest = () => { - const index = testData.indexOf(series); - const newData = [...testData]; + const testData = testDataRef.current; + const colors = colorsRef.current; + const symbols = symbolsRef.current; + + const index = testData.findIndex( + (item) => item.signature_id === series.signature_id + ); if (index === -1) { return; } - newData.splice(index, 1); + const newData = [...testData]; - // removing a disabled test frees nothing, since it never held a - // color, just drop it. Removing a visible test frees its - // color: promote the first currently-disabled test to take its - // place, or return the color/symbol to the pool if none is waiting. - if (series.color[0] === 'border-secondary') { - resetParams(newData); - return; - } + newData.splice(index, 1); - const promoteIndex = newData.findIndex( - (item) => item.color[0] === 'border-secondary', - ); + // promote the test that just shifted into the maximum visibility slot. + // this ignores user-deselected tests earlier in the queue and + // strictly targets the next auto-queued test that was forced hidden. + const promoteTargetIndex = graphColors.length - 1; - if (promoteIndex === -1) { + if ( + newData[promoteTargetIndex] && + newData[promoteTargetIndex].color[0] === 'border-secondary' + ) { + const promoted = newData[promoteTargetIndex]; + newData[promoteTargetIndex] = { + ...promoted, + color: series.color, + symbol: series.symbol, + visible: true, + data: promoted.data.map((item) => ({ + ...item, + z: series.color[1], + _z: series.symbol, + })), + }; + resetParams(newData); + } else if (series.color[0] === 'border-secondary') { + resetParams(newData); + } else { const newColors = [...colors, series.color]; const newSymbols = [...symbols, series.symbol]; resetParams(newData, newColors, newSymbols); - return; } - - const promoted = newData[promoteIndex]; - newData[promoteIndex] = { - ...promoted, - color: series.color, - symbol: series.symbol, - visible: true, - data: promoted.data.map((item) => ({ - ...item, - z: series.color[1], - _z: series.symbol, - })), - }; - resetParams(newData); }; const subtitleStyle = 'p-0 mb-0 border-0 text-secondary text-start'; @@ -235,21 +242,23 @@ const LegendCard = ({ }; LegendCard.propTypes = { - series: PropTypes.PropTypes.shape({ + series: PropTypes.shape({ visible: PropTypes.bool, }).isRequired, updateState: PropTypes.func.isRequired, - testData: PropTypes.arrayOf(PropTypes.shape({})), updateStateParams: PropTypes.func.isRequired, - colors: PropTypes.arrayOf(PropTypes.arrayOf(PropTypes.string)).isRequired, - selectedDataPoint: PropTypes.shape({}), -}; + testDataRef: PropTypes.shape({ current: PropTypes.array }).isRequired, + colorsRef: PropTypes.shape({ current: PropTypes.array }).isRequired, + symbolsRef: PropTypes.shape({ current: PropTypes.array }).isRequired, + selectedDataPoint: PropTypes.shape({}),}; -const areEqual = (prev, next) => - prev.series === next.series && - prev.testData === next.testData && - prev.colors === next.colors && - prev.symbols === next.symbols && - prev.selectedDataPoint === next.selectedDataPoint; +const areEqual = (prev, next) => { + const seriesEqual = prev.series === next.series; + + const prevWasSelected = prev.selectedDataPoint?.signature_id === prev.series.signature_id; + const nextIsSelected = next.selectedDataPoint?.signature_id === next.series.signature_id; + + return seriesEqual && prevWasSelected === nextIsSelected && prev.frameworks === next.frameworks; +}; export default React.memo(LegendCard, areEqual); From bfabae6cf33a0c7603eb32b790048423c9e0223d Mon Sep 17 00:00:00 2001 From: David Miculit Date: Thu, 20 Aug 2026 15:15:39 +0300 Subject: [PATCH 7/7] test: match the new props --- tests/ui/perfherder/legend_card_test.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ui/perfherder/legend_card_test.jsx b/tests/ui/perfherder/legend_card_test.jsx index 403d963af4f..82961bc4f1f 100644 --- a/tests/ui/perfherder/legend_card_test.jsx +++ b/tests/ui/perfherder/legend_card_test.jsx @@ -65,12 +65,12 @@ const legendCard = ( render( , );