Skip to content

Bug 2009828 - Avoid duplicate common alert summary requests #9243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
junngo wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −5
Open

Bug 2009828 - Avoid duplicate common alert summary requests #9243

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions ui/perfherder/graphs/GraphsView.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,11 +232,13 @@ class GraphsView extends React.Component {
this.getAlertSummaries(series.signature_id, series.repository_id),
),
);
const commonAlerts = await Promise.all(
seriesData.map((series) =>
this.getCommonAlerts(series.framework_id, timeRange.value),
),
);
const uniqueFrameworkIds = [
...new Set(seriesData.map((series) => series.framework_id)),
];
const commonAlertsFlat = (await Promise.all(
uniqueFrameworkIds.map(id => this.getCommonAlerts(id, timeRange.value))
)).flat();
const commonAlerts = [commonAlertsFlat];
Copy link
Collaborator

@beatrice-acasandrei beatrice-acasandrei Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to merge this, but I still have a quick question. You're now flattening the results into commonAlertsFlat and then wrapping it in an array [commonAlertsFlat]. The original code returned an array of results mapped 1:1 to seriesData. Does the downstream logic expect this single combined list, or do we still need to maintain the mapping for each series? Just want to make sure this matches the expected type.

Copy link
Contributor Author

@junngo junngo Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review :)
Sorry about that. I should have explained this earlier. Yes, the result format has changed.

Before:
[ [alertsA], [alertsB], [alertsC],]
After:
[ [alertA, alertB, alertC,] ]

The change is intentional. createGraphData only uses commonAlerts[0], so returning one combined list matches how the data is used. The previous code also effectively relied on the first entry.
If you’d prefer preserving the 1:1 mapping for clarity, I can keep the previous shape.

[0]

treeherder/ui/perfherder/perf-helpers/helpers.js

Lines 841 to 844 in 5b1819b

commonAlert: reduceDictToKeys(
commonAlerts[0].find((alert) => alert.push_id === dataPoint.push_id),
['id', 'status'],
),

const newColors = [...colors];
const newSymbols = [...symbols];

Expand Down