fix(dashboard): Resolve drill by wrong column header text #37412
+1,029
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Code Review Agent Run #586690Actionable Suggestions - 0Additional Suggestions - 8
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 |
dosubot
bot
added
change:frontend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+55
to
+71
| * | ||
| * Examples: | ||
| * - replaceLabelIfExists("testing_account 123", "testing_account", "Account") | ||
| * → "Account 123" | ||
| * - replaceLabelIfExists("testing_account 123", "account", "Account") | ||
| * → "testing_account 123" | ||
| * - replaceLabelIfExists("123", "12", "X") | ||
| * → "123" | ||
| */ | ||
|
|
||
| if (key === label) { | ||
| return replacement; | ||
| } | ||
|
|
||
| const escapedLabel = escapeRegex(label); | ||
| const regex = new RegExp(`(?<!\\w)${escapedLabel}(?!\\w)`, 'g'); | ||
| return regex.test(key) ? key.replace(regex, replacement) : key; |
Contributor
There was a problem hiding this comment.
Suggestion: Logic bug & portability: replaceLabelIfExists claims to ignore numeric-only labels and to match only tokens containing alphabetic characters, but it never checks for alphabetic characters in label. Also it uses a RegExp with a lookbehind (?<!\w), which can throw a SyntaxError in JS environments that don't support lookbehind. Add an explicit alphabetic check for label and replace lookbehind with a look-for-prefix capturing group so the function behaves as documented and works in older runtimes. [possible bug]
Severity Level: Critical 🚨
- ❌ Module load can throw SyntaxError in older JS engines.
- ❌ Column/header humanization may be incorrect for numeric labels.
- ⚠️ Breaks any UI code importing this util at runtime.
Suggested change
| * | |
| * Examples: | |
| * - replaceLabelIfExists("testing_account 123", "testing_account", "Account") | |
| * → "Account 123" | |
| * - replaceLabelIfExists("testing_account 123", "account", "Account") | |
| * → "testing_account 123" | |
| * - replaceLabelIfExists("123", "12", "X") | |
| * → "123" | |
| */ | |
| if (key === label) { | |
| return replacement; | |
| } | |
| const escapedLabel = escapeRegex(label); | |
| const regex = new RegExp(`(?<!\\w)${escapedLabel}(?!\\w)`, 'g'); | |
| return regex.test(key) ? key.replace(regex, replacement) : key; | |
| */ | |
| if (key === label) { | |
| return replacement; | |
| } | |
| // Ignore numeric-only labels (documented behavior) | |
| if (!/[A-Za-z]/.test(label)) { | |
| return key; | |
| } | |
| const escapedLabel = escapeRegex(label); | |
| // Avoid using lookbehind for broader runtime compatibility. | |
| // Use a capturing prefix group to ensure the label is not preceded by a word char. | |
| const regex = new RegExp(`(^|[^A-Za-z0-9_])${escapedLabel}(?!\\w)`, 'g'); | |
| return regex.test(key) | |
| ? key.replace(regex, (_match, prefix) => `${prefix}${replacement}`) | |
| : key; |
Steps of Reproduction ✅
1. In a runtime that loads the frontend bundle, the module at
superset-frontend/packages/superset-ui-core/src/utils/addLabelMapToVerboseMap.ts
is evaluated. The regex construction occurs at line 70 (`const regex = new
RegExp(...)`)
inside the function `replaceLabelIfExists` (lines 39-72).
2. If the environment's JS engine does not support ES2018 lookbehind (for example,
older Node/browsers without lookbehind support), the RegExp literal string
containing `(?<!\\w)` will cause a SyntaxError when `new RegExp(...)` is executed
(see line 70). This prevents the module from functioning or being required.
3. Independently, call the exported function `addLabelMapToVerboseMap` (line 81)
with inputs that include a numeric-only label:
- Example call (in a small repro script): import addLabelMapToVerboseMap from
'superset-frontend/packages/superset-ui-core/src/utils/addLabelMapToVerboseMap';
addLabelMapToVerboseMap({ '123 12': ['12'] }, { '12': 'Twelve' });
The call path reaches the loop that invokes `replaceLabelIfExists` at lines 138-143,
which will construct the lookbehind-based RegExp and attempt to replace "12" inside
"123 12".
4. Expected (per function documentation): numeric-only labels should be ignored.
Actual: because there is no alphabetic check, the current implementation will
attempt the replacement (or fail earlier with SyntaxError in lookbehind-unsupported
runtimes).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/utils/addLabelMapToVerboseMap.ts
**Line:** 55:71
**Comment:**
*Possible Bug: Logic bug & portability: `replaceLabelIfExists` claims to ignore numeric-only labels and to match only tokens containing alphabetic characters, but it never checks for alphabetic characters in `label`. Also it uses a RegExp with a lookbehind `(?<!\w)`, which can throw a SyntaxError in JS environments that don't support lookbehind. Add an explicit alphabetic check for `label` and replace lookbehind with a look-for-prefix capturing group so the function behaves as documented and works in older runtimes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+135
to
+152
| const newKey = key | ||
| .split(' ') | ||
| .map(word => { | ||
| for (const label of Object.keys(newLabelMap)) { | ||
| const newWord = replaceLabelIfExists( | ||
| word, | ||
| label, | ||
| newLabelMap[label], | ||
| ); | ||
|
|
||
| if (newWord !== word) { | ||
| return newWord; | ||
| } | ||
| } | ||
|
|
||
| return word; | ||
| }) | ||
| .join(' '); |
Contributor
There was a problem hiding this comment.
Suggestion: Logic bug: the code splits the compound key on spaces and attempts per-word replacements, which prevents matching multi-word labels (e.g., "1 week ago") and can produce incorrect results. Replace per-word mapping with applying replaceLabelIfExists across the entire key for each label to correctly handle multi-word labels and punctuation. [logic error]
Severity Level: Critical 🚨
- ❌ Multi-word timeframe labels not humanized.
- ❌ Dashboard column headers show raw tokens.
- ⚠️ Affects any UI that consumes verboseMap for display.
Suggested change
| const newKey = key | |
| .split(' ') | |
| .map(word => { | |
| for (const label of Object.keys(newLabelMap)) { | |
| const newWord = replaceLabelIfExists( | |
| word, | |
| label, | |
| newLabelMap[label], | |
| ); | |
| if (newWord !== word) { | |
| return newWord; | |
| } | |
| } | |
| return word; | |
| }) | |
| .join(' '); | |
| let newKey = key; | |
| for (const label of Object.keys(newLabelMap)) { | |
| newKey = replaceLabelIfExists(newKey, label, newLabelMap[label]); | |
| } |
Steps of Reproduction ✅
1. The code that builds `newKey` is at lines 135-152 inside addLabelMapToVerboseMap
(superset-frontend/packages/superset-ui-core/src/utils/addLabelMapToVerboseMap.ts).
It splits the compound key on spaces (line 136) and attempts per-word replacement.
2. Reproduce by calling addLabelMapToVerboseMap with:
- label_map = { "testing_count, 1 week ago": ["testing_count", "1 week ago"] }
- verboseMap = { testing_count: "Testing Count", "1 week ago": "1 week ago (label)" }
3. Execution splits the key "testing_count, 1 week ago" into words and passes each single
word
into replaceLabelIfExists (lines 136-151). The multi-word label "1 week ago" is never
matched because it's split across words, so it will not be replaced in the generated
entry.
4. Expected: "Testing Count, 1 week ago (label)". Actual: only single-word replacements
applied,
multi-word components remain unchanged. This reproduces incorrect header text
generation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/utils/addLabelMapToVerboseMap.ts
**Line:** 135:152
**Comment:**
*Logic Error: Logic bug: the code splits the compound `key` on spaces and attempts per-word replacements, which prevents matching multi-word labels (e.g., "1 week ago") and can produce incorrect results. Replace per-word mapping with applying `replaceLabelIfExists` across the entire `key` for each label to correctly handle multi-word labels and punctuation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| rawFormData: { | ||
| datasource: '1__table', | ||
| viz_type: 'echarts_timeseries', | ||
| x_axis: 'ds', |
Contributor
There was a problem hiding this comment.
Suggestion: The rawFormData.x_axis is set to 'ds' but the query column in this fixture is time_start; this mismatch will cause code that looks up the x-axis column (for drilldown, labeling, or mapping) to not find the intended column and show the wrong header or fail to map drill context correctly. Set x_axis to 'time_start' so it matches the actual column name in colnames/verboseMap. [logic error]
Severity Level: Critical 🚨
- ❌ Drill/context menu shows wrong column header.
- ⚠️ Timeseries story/tests produce incorrect snapshots.
Suggested change
| x_axis: 'ds', | |
| x_axis: 'time_start', |
Steps of Reproduction ✅
1. Open the fixture file at superset-frontend/spec/fixtures/mockTimeSeries.ts and inspect
the rawFormData block (lines 153-158). Note x_axis is set to 'ds' (line 156) while the
fixture's query column is named 'time_start' (see colnames at lines 62-72 and verboseMap
at lines 31-35).
2. Any test or story that imports mockedTimeSeriesProps and mounts the Echarts
Timeseries/MixedTimeseries component (the fixture is typed as EchartsTimeseriesChartProps
/ EchartsMixedTimeseriesProps via imports at lines 19-21) will pass this rawFormData into
the chart's props.
3. The chart code that maps x-axis values looks up the configured x_axis name
(rawFormData.x_axis) against queriesData[0].colnames to find the column index (colnames
present at lines 62-72). Because x_axis is 'ds' (not present in colnames), the lookup will
fail and the chart will either fall back to a wrong column or omit the x-axis mapping.
4. Observe the symptom: drill/context menu or column header displays the wrong header text
(or drill payload references an incorrect/missing column). This reproduces the issue
described in the PR title (drill by wrong column header text).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/spec/fixtures/mockTimeSeries.ts
**Line:** 156:156
**Comment:**
*Logic Error: The `rawFormData.x_axis` is set to `'ds'` but the query column in this fixture is `time_start`; this mismatch will cause code that looks up the x-axis column (for drilldown, labeling, or mapping) to not find the intended column and show the wrong header or fail to map drill context correctly. Set `x_axis` to `'time_start'` so it matches the actual column name in `colnames`/`verboseMap`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| setResultResp(transformedResponseArray); | ||
| setResponseError(''); | ||
| cache.set(queryFormData, json.result); | ||
| if (queryForce) { |
Contributor
There was a problem hiding this comment.
Suggestion: Inconsistent caching: the code stores the raw json.result into cache but sets the component state to the transformed result; on subsequent cache hits you will re-use the untransformed shape which likely doesn't match the component's expected shape and will cause rendering errors. Store the transformed array in the cache so read/write use the same data shape. [logic error]
Severity Level: Critical 🚨
- ❌ Explore DataTablesPane shows wrong table columns.
- ❌ Drill-by headers render incorrectly in Explore.
- ⚠️ Re-render with cached queries causes UI errors.
Suggested change
| if (queryForce) { | |
| cache.set(queryFormData, transformedResponseArray); |
Steps of Reproduction ✅
1. Open the Explore view that renders DataTablesPane which imports this hook:
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx.
2. Trigger a query such that isRequest === true and cache does NOT yet have the key:
the hook enters the code path at the getChartDataRequest call (useResultsPane.tsx lines
~88-96)
and executes the .then handler at lines 95-99 where cache.set(...) is called.
3. The .then handler (useResultsPane.tsx:95-99) transforms json.result via
transformTableData()
and calls setResultResp(transformedResponseArray) but stores the untransformed
json.result
in cache via cache.set(queryFormData, json.result).
4. Later (same session) when the same queryFormData is used, the early cache-read branch
runs
(useResultsPane.tsx — the useEffect branch that checks if (isRequest &&
cache.has(queryFormData))),
calling setResultResp(ensureIsArray(cache.get(queryFormData))). Because the cached
value is the
raw json.result (untransformed) the component receives a different data shape than it
expects,
producing mismatched colnames/coltypes or rendering errors in SingleQueryResultPane.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx
**Line:** 100:100
**Comment:**
*Logic Error: Inconsistent caching: the code stores the raw `json.result` into `cache` but sets the component state to the transformed result; on subsequent cache hits you will re-use the untransformed shape which likely doesn't match the component's expected shape and will cause rendering errors. Store the transformed array in the cache so read/write use the same data shape.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| import { ensureIsArray, addLabelMapToVerboseMap } from '@superset-ui/core'; |
Contributor
There was a problem hiding this comment.
Suggestion: The import of ensureIsArray uses a named import but the utility is exported as a default from its module; this can cause ensureIsArray to be undefined at runtime. Import the default and the named export correctly so both functions are available. [type error]
Severity Level: Critical 🚨
- ❌ DataTablesPane column labels throw TypeError.
- ⚠️ Explore UI table formatting fails to apply labels.
- ⚠️ Any view calling transformTableData affected.
Suggested change
| import { ensureIsArray, addLabelMapToVerboseMap } from '@superset-ui/core'; | |
| import ensureIsArray, { addLabelMapToVerboseMap } from '@superset-ui/core'; |
Steps of Reproduction ✅
1. Open the file superset-frontend/src/explore/components/DataTablesPane/utils.ts and note
the export of transformTableData at line 28. (file: utils.ts, line 28)
2. In a unit test or browser console, call transformTableData with a responseArray that
contains a label_map entry (any object with label_map present). The call site is the
exported function transformTableData in utils.ts:28.
3. During execution, transformTableData iterates labelMap at lines 41-46 and calls
ensureIsArray(value) at line 42 (Object.entries(labelMap).filter(...)). Because
ensureIsArray is imported as a named import (line 19) but the actual module
(packages/superset-ui-core/src/utils/ensureIsArray.ts) exports it as default,
ensureIsArray will be undefined and the call at utils.ts:42 throws TypeError:
ensureIsArray is not a function.
4. The TypeError surfaces while formatting column labels in DataTablesPane (utils.ts),
breaking table label processing and producing a stack trace pointing to
superset-frontend/src/explore/components/DataTablesPane/utils.ts lines ~41-46. This
reproduces reliably whenever transformTableData is invoked with a response that has a
label_map.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/DataTablesPane/utils.ts
**Line:** 19:19
**Comment:**
*Type Error: The import of `ensureIsArray` uses a named import but the utility is exported as a default from its module; this can cause `ensureIsArray` to be undefined at runtime. Import the default and the named export correctly so both functions are available.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This PR fixes an issue in the Dashboard Drill By feature where the table view ignored dataset labels and displayed raw column and metric names instead. The change ensures that dimensions and metrics in the Drill By table correctly use their defined labels, aligning the table view behavior with the chart view and improving overall consistency and usability.
Also moved
addLabelMapToVerboseMapto@superset-ui/coreBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
Screen.Recording.2026-01-24.at.4.22.17.AM.mov
AFTER
Screen.Recording.2026-01-24.at.4.04.45.AM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION