fix(explore): stop metric edits from bleeding across metrics with duplicate optionNames#41208
fix(explore): stop metric edits from bleeding across metrics with duplicate optionNames#41208jesperct wants to merge 4 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41208 +/- ##
=======================================
Coverage 64.36% 64.36%
=======================================
Files 2653 2653
Lines 144868 144881 +13
Branches 33424 33428 +4
=======================================
+ Hits 93241 93255 +14
+ Misses 49954 49953 -1
Partials 1673 1673
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jesperct
force-pushed
the
fix/metrics-shared-optionname-edit
branch
2 times, most recently
from
dfaeced to
80ff8d1
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #168057
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.tsx - 1
- Incomplete test assertion · Line 159-166
Review Details
-
Files reviewed - 4 · Commit Range:
d39ebbf..80ff8d1- superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
- superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx
- superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.tsx
- superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| expect(onChange).toHaveBeenCalledWith( | ||
| expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| aggregate: AGGREGATES.AVG, | ||
| label: 'AVG(value)', | ||
| }), | ||
| ]), | ||
| ); |
There was a problem hiding this comment.
The assertion only verifies the untouched AVG(value) metric is preserved. It does not verify the edited SUM(value)→MAX(value) metric was actually saved. If the save operation fails to propagate the edit, this test passes incorrectly. Per BITO.md rule [6262], tests must validate actual behavior, not just partial outcomes.
Code Review Run #168057
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Strengthened the assertion to also check the edited metric saved as MAX(value), so a dropped edit can't pass on the surviving AVG alone. dfc4266.
There was a problem hiding this comment.
The reviewer's suggestion is appropriate. The test currently only verifies that the untouched metric is preserved, but it fails to confirm that the intended edit (SUM(value) to MAX(value)) was successfully applied. Strengthening the assertion to check for the presence of the updated metric ensures the test validates the full behavior of the save operation.
Code Review Agent Run #246b45Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…ss metrics Metrics are matched by optionName when an edit is saved, but a saved chart can carry two adhoc metrics sharing the same optionName (e.g. born from a duplicated metric). Editing one then overwrote every metric with that optionName. coerceAdhocMetrics now regenerates colliding optionNames on load so each metric keeps a unique identity.
The drag-and-drop metric control (used by current Explore) matches edits by optionName the same way the legacy control does, so it shares the same bug: editing one of two metrics with a duplicate optionName rewrote both. Apply the same uniqueness fix in coerceMetrics, verified live.
… other survives Strengthen the shared-optionName regression test to also assert the targeted metric saved as MAX(value), so a silently-dropped edit can't make the test pass on the untouched metric alone.
jesperct
force-pushed
the
fix/metrics-shared-optionname-edit
branch
from
f1bc1da to
f1071aa
Compare
Code Review Agent Run #098e65Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
Editing one custom (adhoc) metric could silently rewrite other metrics on the same chart. When a chart's saved metrics include two adhoc metrics that share an
optionName(which can happen, for example after duplicating a metric), editing one of them overwrote every metric with thatoptionName— both the label and the definition snapped to the edited one.Root cause: the metric controls match an edit back to its metric by
optionName, but nothing guaranteedoptionNamewas unique. This fixes it where metrics are loaded into the control (coerceMetrics/coerceAdhocMetrics): a metric whoseoptionNamecollides with an earlier one is given a freshoptionName, so each metric keeps a unique identity. That both heals already-affected saved charts and prevents the behavior going forward. The fix is applied to the drag-and-drop metric control used by Explore and to the legacy metrics control, which share the same matching logic.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
editing one metric's aggregate (SUM → MAX) also changes the second metric, leaving two identical
MAX(num)metrics.AFTER:
only the edited metric changes; the second metric stays
AVG(num).TESTING INSTRUCTIONS
form_dataholds two adhoc metrics on the same column that share anoptionName(this is the state a duplicated metric can leave behind).DndMetricSelect.test.tsxandMetricsControl.test.tsxassert that loading two metrics with a duplicateoptionNameyields unique identities so an edit touches only one.ADDITIONAL INFORMATION