fix(tracemetrics): Metric panel title should use equation with label#113143
Conversation
github-actions
Bot
added
the
Scope: Frontend
3254404 to
5baa797
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dfdb148. Configure here.
| return unresolveExpression(visualize.expression.text, referenceMap); | ||
| } | ||
| return undefined; | ||
| }); |
There was a problem hiding this comment.
Stale chart title in non-refresh UI path
Medium Severity
The title state is initialized once via useState and only updated through onTitleChange (which is setTitle). In the non-refresh UI path, MetricToolbar is rendered in MetricsQueryBuilderSection without passing onTitleChange, while MetricPanel is rendered separately. When a user edits an equation through the external toolbar, title becomes stale but still takes precedence over metricLabel in the chart title via title ?? metricLabel. Before this PR, the chart title correctly showed the updated resolved expression from metricLabel; now it shows the original unresolved expression from initial mount.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dfdb148. Configure here.
There was a problem hiding this comment.
I suppose this is a concern and should be recalculated, but it works out because even if a reference map item changes, the label shouldn't because the equation itself wasn't affected. Going to accept this for now.
There was a problem hiding this comment.
Seems reasonable to accept 👍
| return unresolveExpression(visualize.expression.text, referenceMap); | ||
| } | ||
| return undefined; | ||
| }); |
There was a problem hiding this comment.
Seems reasonable to accept 👍
2 participants
Primarily, the equation builder was supposed to avoid leaking the internal representation out of itself, but because we want to display it in another component (i.e. the chart title), we need to expose a way to hook into the internal value when a change is reported.
In this case, call the
handleExpressionChangecallback with both the resolved version, and the internal version so we can read it and store it in local state for the metric panel to use for the title. This only applies to equations right now but I've made the terminology more general (i.e.title, etc)Ideally,
useMetricLabelwould have been able to provide this but I didn't really want to store more metadata higher than the metric panel, where it's relevant