Skip to content

fix(dashboards): Persist legend selection to URL for new chart widgets#112974

Open
gggritso wants to merge 1 commit intomasterfrom
georgegritsouk/browse-199-new-charts-do-not-update-url-with-legend-selection
Open

fix(dashboards): Persist legend selection to URL for new chart widgets#112974
gggritso wants to merge 1 commit intomasterfrom
georgegritsouk/browse-199-new-charts-do-not-update-url-with-legend-selection

Conversation

@gggritso
Copy link
Copy Markdown
Member

@gggritso gggritso commented Apr 14, 2026

Wire widgetLegendState through to VisualizationWidget so that toggling series visibility in the legend of new TimeSeriesWidgetVisualization charts persists to the unselectedSeries URL parameter. Previously, legend selections were lost on page refresh.

The new chart path uses plain series names internally, while WidgetLegendSelectionState uses seriesName|~|widgetId-encoded keys for per-widget disambiguation (needed because all dashboard charts share an ECharts chart group). This PR adds encode/decode at the VisualizationWidget boundary to bridge the two formats.

Also removes a now-unnecessary decode call from formatTimeSeriesLabel — the yAxis field is never encoded with |~|widgetId in the new path, so the decode was dead code. Corresponding test cases for encoded yAxis values are removed.

Closes BROWSE-199

New TimeSeriesWidgetVisualization charts in dashboards did not update
the URL with legend selection state, so toggling series visibility
was lost on page refresh. Wire widgetLegendState to VisualizationWidget
and disambiguate plottable names with widget ID for the shared ECharts
chart group.

Refs BROWSE-199
Co-Authored-By: Claude <noreply@anthropic.com>
@gggritso gggritso requested a review from a team as a code owner April 14, 2026 20:43
@linear-code
Copy link
Copy Markdown

linear-code bot commented Apr 14, 2026

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 14, 2026
@sentry
Copy link
Copy Markdown
Contributor

sentry bot commented Apr 14, 2026

Sentry Snapshot Testing

Name Added Removed Modified Renamed Unchanged Status
sentry-frontend
sentry-frontend
0 0 0 0 204 ✅ Unchanged

Comment on lines +537 to +541
plain: LegendSelection,
widgetId: string | undefined
): LegendSelection {
const encoded: LegendSelection = {};
for (const key in plain) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: encodeLegendSelection is called with an undefined widget.id for unsaved widgets, creating invalid legend keys. Downstream decoding logic using a non-null assertion on widget.id will then fail.
Severity: LOW

Suggested Fix

Add explicit validation in widgetCanUseTimeSeriesVisualization to ensure a widget has an ID before allowing legend state changes. Alternatively, defensively handle undefined IDs in the encoder and decoder functions to prevent creating invalid keys.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/views/dashboards/widgetCard/visualizationWidget.tsx#L537-L541

Potential issue: The `Widget` type allows `widget.id` to be optional. The function
`widgetCanUseTimeSeriesVisualization` determines if a widget can use the new time series
visualization but does not validate that `widget.id` is present. If a widget without an
ID (e.g., an unsaved widget) meets the criteria, `encodeLegendSelection` is called with
an `undefined` ID. This creates an invalid legend key containing the literal string
"undefined", such as `"seriesName|~|undefined"`. Downstream decoding logic in
`widgetLegendSelectionState.tsx` uses a non-null assertion (`widget.id!`), which will
fail or cause incorrect behavior when it encounters these malformed keys.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 243cd0f. Configure here.


let {yAxis: seriesName} = timeSeries;

// Decode from series name disambiguation
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removing decode also removes function name prettification

Low Severity

The removed decodeSeriesNameForLegend call did two things: strip the |~|widgetId suffix (correctly identified as dead code) and prettify aggregate function names via prettifyParsedFunction (e.g., count(span.duration)count(spans)). By removing the entire call, the prettification side-effect is also lost, causing a subtle label regression in the new chart path for specific aggregates like count(span.duration).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 243cd0f. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant