Skip to content

fix: leaderboard columns going stale when context is shown for all measures #9807

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

Merged
nishantmonu51 merged 1 commit into from
Aug 18, 2026
Merged

fix: leaderboard columns going stale when context is shown for all measures #9807

Show file tree
Hide file tree
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
45 changes: 25 additions & 20 deletions web-common/src/features/dashboards/leaderboard/Leaderboard.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,28 +302,35 @@

$: isTimeComparisonActive = !!comparisonTimeRange;

// Measures that render context columns (percent of total, delta absolute and
// delta percent). This must be a reactive value rather than a function: a
// function called from the markup does not track the props it reads, so the
// columns would go stale when the context toggle changes.
$: measuresWithContext = new Set(
leaderboardShowContextForAllMeasures
? leaderboardMeasureNames
: [leaderboardSortByMeasureName],
);

$: columnCount =
1 + // Base column (dimension)
leaderboardMeasureNames.length + // Value column for each measure
(isTimeComparisonActive
? leaderboardMeasureNames.length * // For each measure
((isValidPercentOfTotal(leaderboardSortByMeasureName) ? 1 : 0) + // Percent of total column
(isTimeComparisonActive ? 2 : 0)) // Delta absolute and delta percent columns
: 0);
leaderboardMeasureNames.reduce(
(count, measureName) =>
count +
1 + // Value column
(measuresWithContext.has(measureName)
? (isValidPercentOfTotal(measureName) ? 1 : 0) + // Percent of total column
(isTimeComparisonActive ? 2 : 0) // Delta absolute and delta percent columns
: 0),
0,
);

// Calculate maximum values for relative magnitude bar sizing
// This includes both above-the-fold and below-the-fold data for accurate scaling
$: maxValues = getLeaderboardMaxValues(
[...aboveTheFold, ...belowTheFoldRows],
leaderboardMeasures,
);

function shouldShowContextColumns(measureName: string): boolean {
return (
leaderboardShowContextForAllMeasures ||
measureName === leaderboardSortByMeasureName
);
}
</script>

<div
Expand All @@ -343,13 +350,13 @@
<col data-dimension-column style:width="{dimensionColumnWidth}px" />
{#each leaderboardMeasureNames as measureName, index (index)}
<col data-measure-column style:width="{$valueColumn}px" />
{#if isValidPercentOfTotal(measureName) && shouldShowContextColumns(measureName)}
{#if isValidPercentOfTotal(measureName) && measuresWithContext.has(measureName)}
<col
data-percent-of-total-column
style:width="{COMPARISON_COLUMN_WIDTH}px"
/>
{/if}
{#if isTimeComparisonActive && shouldShowContextColumns(measureName)}
{#if isTimeComparisonActive && measuresWithContext.has(measureName)}
<col data-absolute-change-column style:width="{$deltaColumn}px" />
<col
data-percent-change-column
Expand All @@ -373,7 +380,7 @@
{isTimeComparisonActive}
{sortedAscending}
{leaderboardMeasureNames}
{leaderboardShowContextForAllMeasures}
{measuresWithContext}
{toggleSort}
{setPrimaryDimension}
{toggleComparisonDimension}
Expand All @@ -400,11 +407,10 @@
{dimensionName}
{itemData}
{isValidPercentOfTotal}
{leaderboardShowContextForAllMeasures}
{measuresWithContext}
{isTimeComparisonActive}
{leaderboardMeasureNames}
{toggleDimensionValueSelection}
{leaderboardSortByMeasureName}
{formatters}
{tooltipFormatters}
{dimensionColumnWidth}
Expand All @@ -422,13 +428,12 @@
{filterExcludeMode}
{atLeastOneActive}
{isValidPercentOfTotal}
{leaderboardShowContextForAllMeasures}
{measuresWithContext}
{isTimeComparisonActive}
{leaderboardMeasureNames}
borderTop={i === 0}
borderBottom={i === belowTheFoldRows.length - 1}
{toggleDimensionValueSelection}
{leaderboardSortByMeasureName}
{formatters}
{tooltipFormatters}
{dimensionColumnWidth}
Expand Down
27 changes: 17 additions & 10 deletions web-common/src/features/dashboards/leaderboard/LeaderboardDisplay.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,26 @@
MAX_DIMENSION_COLUMN_WIDTH,
);

$: showPercentOfTotal = $isMeasureValidPercentOfTotal(
$leaderboardSortByMeasureName,
$: measuresWithContext = new Set(
$leaderboardShowContextForAllMeasures
? $leaderboardMeasures.map((measure) => measure.name!)
: [$leaderboardSortByMeasureName],
);
$: showDeltaPercent = !!comparisonTimeRange;

$: tableWidth =
dimensionColumnWidth +
$valueColumn +
(comparisonTimeRange
? $deltaColumn + (showDeltaPercent ? COMPARISON_COLUMN_WIDTH : 0)
: showPercentOfTotal
// Mirrors the columns rendered in Leaderboard.svelte's colgroup.
$: tableWidth = $leaderboardMeasures.reduce((width, measure) => {
const showContext = measuresWithContext.has(measure.name!);
return (
width +
$valueColumn +
(showContext && $isMeasureValidPercentOfTotal(measure.name!)
? COMPARISON_COLUMN_WIDTH
: 0);
: 0) +
(showContext && comparisonTimeRange
? $deltaColumn + COMPARISON_COLUMN_WIDTH
: 0)
);
}, dimensionColumnWidth);
</script>

<div
Expand Down
55 changes: 55 additions & 0 deletions web-common/src/features/dashboards/leaderboard/LeaderboardHeader.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { render } from "@testing-library/svelte";
import { describe, expect, it } from "vitest";
import LeaderboardHeader from "./LeaderboardHeader.svelte";
import { SortType } from "../proto-state/derived-types";

const baseProps = {
dimensionName: "country",
displayName: "Country",
dimensionDescription: "",
isFetching: false,
isTimeComparisonActive: true,
isBeingCompared: false,
sortedAscending: false,
hovered: false,
// Any sort type other than the ones rendering a sort arrow, which needs the
// Web Animations API that jsdom does not implement.
sortType: SortType.DIMENSION,
allowDimensionComparison: true,
allowExpandTable: true,
leaderboardMeasureNames: ["impressions", "bids"],
leaderboardSortByMeasureName: "impressions",
isValidPercentOfTotal: () => false,
measureLabel: (measureName: string) => measureName,
toggleSort: () => {},
setPrimaryDimension: () => {},
toggleComparisonDimension: () => {},
dimensionColumnWidth: 164,
};

describe("LeaderboardHeader", () => {
it("adds context columns for every measure when the set of measures with context changes", async () => {
const { container, rerender } = render(LeaderboardHeader, {
props: { ...baseProps, measuresWithContext: new Set(["impressions"]) },
});

expect(
container.querySelectorAll("th[data-absolute-change-header]").length,
).toBe(1);
expect(
container.querySelectorAll("th[data-percent-change-header]").length,
).toBe(1);

await rerender({
...baseProps,
measuresWithContext: new Set(["impressions", "bids"]),
});

expect(
container.querySelectorAll("th[data-absolute-change-header]").length,
).toBe(2);
expect(
container.querySelectorAll("th[data-percent-change-header]").length,
).toBe(2);
});
});
16 changes: 5 additions & 11 deletions web-common/src/features/dashboards/leaderboard/LeaderboardHeader.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
export let allowExpandTable: boolean;
export let leaderboardMeasureNames: string[] = [];
export let leaderboardSortByMeasureName: string | null;
export let leaderboardShowContextForAllMeasures: boolean;
// Measures that render context columns; see Leaderboard.svelte.
export let measuresWithContext: Set<string>;
export let toggleSort: (sortType: SortType, measureName?: string) => void;
export let setPrimaryDimension: (dimensionName: string) => void;
export let toggleComparisonDimension: (
Expand All @@ -44,13 +45,6 @@
// Height of the whole leaderboard table, so the resize handle can span all
// rows instead of just the header cell.
export let tableHeight = 0;

function shouldShowContextColumns(measureName: string): boolean {
return (
leaderboardShowContextForAllMeasures ||
measureName === leaderboardSortByMeasureName
);
}
</script>

<thead>
Expand Down Expand Up @@ -161,7 +155,7 @@
</button>
</th>

{#if isValidPercentOfTotal(measureName) && shouldShowContextColumns(measureName)}
{#if isValidPercentOfTotal(measureName) && measuresWithContext.has(measureName)}
<th data-percent-of-total-header>
<button
aria-label={m.dashboard_sort_by_percent_total_aria()}
Expand Down Expand Up @@ -191,7 +185,7 @@
</th>
{/if}

{#if isTimeComparisonActive && shouldShowContextColumns(measureName)}
{#if isTimeComparisonActive && measuresWithContext.has(measureName)}
<th data-absolute-change-header>
<button
aria-label={m.dashboard_sort_by_absolute_change_aria()}
Expand Down Expand Up @@ -221,7 +215,7 @@
</th>
{/if}

{#if isTimeComparisonActive && shouldShowContextColumns(measureName)}
{#if isTimeComparisonActive && measuresWithContext.has(measureName)}
<th data-percent-change-header>
<button
aria-label={m.dashboard_sort_by_percent_change_aria()}
Expand Down
17 changes: 5 additions & 12 deletions web-common/src/features/dashboards/leaderboard/LeaderboardRow.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
export let atLeastOneActive: boolean;
export let isTimeComparisonActive: boolean;
export let leaderboardMeasureNames: string[] = [];
export let leaderboardShowContextForAllMeasures: boolean;
export let leaderboardSortByMeasureName: string | null;
// Measures that render context columns; see Leaderboard.svelte.
export let measuresWithContext: Set<string>;
export let isValidPercentOfTotal: (measureName: string) => boolean;
export let dimensionColumnWidth: number;
export let maxValues: Record<string, number> = {};
Expand All @@ -49,13 +49,6 @@
>;
export let lowerIsBetterMap: Record<string, boolean> = {};

function shouldShowContextColumns(measureName: string): boolean {
return (
leaderboardShowContextForAllMeasures ||
measureName === leaderboardSortByMeasureName
);
}

let hovered = false;
let valueRect = new DOMRect(0, 0, DEFAULT_COLUMN_WIDTH);
let deltaRect = new DOMRect(0, 0, COMPARISON_COLUMN_WIDTH);
Expand Down Expand Up @@ -269,7 +262,7 @@
{/if}
</LeaderboardCell>

{#if isValidPercentOfTotal(measureName) && shouldShowContextColumns(measureName)}
{#if isValidPercentOfTotal(measureName) && measuresWithContext.has(measureName)}
<LeaderboardCell
value={pctOfTotals[measureName]?.toString() || ""}
tooltipValue={pctOfTotals[measureName] != null
Expand All @@ -289,7 +282,7 @@
</LeaderboardCell>
{/if}

{#if isTimeComparisonActive && shouldShowContextColumns(measureName)}
{#if isTimeComparisonActive && measuresWithContext.has(measureName)}
<LeaderboardCell
value={deltaAbsMap[measureName]?.toString() || ""}
tooltipValue={deltaAbsMap[measureName] != null
Expand Down Expand Up @@ -323,7 +316,7 @@
</LeaderboardCell>
{/if}

{#if isTimeComparisonActive && shouldShowContextColumns(measureName)}
{#if isTimeComparisonActive && measuresWithContext.has(measureName)}
<LeaderboardCell
value={deltaRels[measureName]?.toString() || ""}
tooltipValue={deltaRels[measureName] != null
Expand Down
Loading