codeant-ai-for-open-source[bot] commented on code in PR #40697:
URL: https://github.com/apache/superset/pull/40697#discussion_r3345886630
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:
##########
@@ -236,11 +236,107 @@ export const ExploreChartHeader:
FC<ExploreChartHeaderProps> = ({
[formData, sliceName],
);
+ const shouldShowDynamicTitle =
+ formData?.show_dynamic_title === true ||
+ formData?.showDynamicTitle === true;
+
const formDiffs = useMemo(
() => getChartFormDiffs(originalFormData, currentFormData),
[originalFormData, currentFormData],
);
+ const getFilterName = useCallback((filter: unknown): string | undefined => {
+ if (!filter || typeof filter !== 'object') {
+ return undefined;
+ }
+
+ const filterMeta = filter as Record<string, unknown>;
+ if (
+ typeof filterMeta.titleLabel === 'string' &&
+ filterMeta.titleLabel.length > 0
+ ) {
+ return filterMeta.titleLabel;
+ }
+ const subject = filterMeta.subject;
+ const subjectName =
+ typeof subject === 'string'
+ ? subject
+ : subject && typeof subject === 'object'
+ ? [
+ (subject as Record<string, unknown>).label,
+ (subject as Record<string, unknown>).column_name,
+ (subject as Record<string, unknown>).name,
+ (subject as Record<string, unknown>).column,
+ ].find(
+ candidate =>
+ typeof candidate === 'string' && candidate.length > 0,
+ )
+ : undefined;
+
+ if (typeof subjectName === 'string' && subjectName.length > 0) {
+ return subjectName;
+ }
+
+ const candidates = [filterMeta.column, filterMeta.label, filterMeta.name];
+ return candidates.find(
+ candidate => typeof candidate === 'string' && candidate.length > 0,
+ ) as string | undefined;
+ }, []);
+
+ const getBaseTitleWithoutDynamicSuffix = useCallback((title: string) => {
+ return title.replace(/\s+by\s+.+$/i, '').trimEnd();
+ }, []);
Review Comment:
**Suggestion:** The title-stripping regex removes everything after the first
`" by "` whenever dynamic titles are off, which will silently truncate
legitimate chart names (for example, normal titles containing "by"). Restrict
stripping to only the exact dynamic suffix pattern that was previously
appended, or only strip when the suffix matches the currently derived
active-filter list. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Charts with "by" in names show truncated titles.
- ⚠️ Editing properties can permanently drop trailing name text.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an existing chart whose saved name (`slice.slice_name`) contains the
word "by",
e.g. "Sales by Region", so that `sliceName` passed from
`ExploreViewContainer` into
`ExploreChartHeader` at
`src/explore/components/ExploreViewContainer/index.tsx:20-39` is
"Sales by Region".
2. Ensure dynamic titles are disabled for this chart so
`formData.show_dynamic_title` and
`formData.showDynamicTitle` are false/undefined (the Treemap control that
toggles this
flag is defined at
`plugins/plugin-chart-echarts/src/Treemap/controlPanel.tsx:123-136`).
3. Render Explore for this chart: `ConnectedExploreChartHeader` passes
`sliceName` and
`formData` into `ExploreChartHeader`, which computes `displayTitle` using
`getBaseTitleWithoutDynamicSuffix` at
`ExploreChartHeader/index.tsx:286-288`; the regex
`/\s+by\s+.+$/i` strips the " by Region" suffix from `"Sales by Region"`,
leaving
`"Sales"`.
4. Observe that both the chart header input in `PageHeaderWithActions` (fed
by
`editableTitleProps.title = displayTitle` at
`ExploreChartHeader/index.tsx:371-383`) and
the Properties modal name field (initialized from
`initialName={displayTitle}` at
`ExploreChartHeader/index.tsx:444-448`) show the truncated title "Sales"
instead of the
original saved name "Sales by Region".
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cb5feeacb3e54ff0b4623b8089401160&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cb5feeacb3e54ff0b4623b8089401160&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
**Line:** 286:288
**Comment:**
*Logic Error: The title-stripping regex removes everything after the
first `" by "` whenever dynamic titles are off, which will silently truncate
legitimate chart names (for example, normal titles containing "by"). Restrict
stripping to only the exact dynamic suffix pattern that was previously
appended, or only strip when the suffix matches the currently derived
active-filter list.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40697&comment_hash=2eb22277af3fd39171aaac39add74c228da09063fb5635b802be4c93c05ec2ca&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40697&comment_hash=2eb22277af3fd39171aaac39add74c228da09063fb5635b802be4c93c05ec2ca&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:
##########
@@ -236,11 +236,107 @@ export const ExploreChartHeader:
FC<ExploreChartHeaderProps> = ({
[formData, sliceName],
);
+ const shouldShowDynamicTitle =
+ formData?.show_dynamic_title === true ||
+ formData?.showDynamicTitle === true;
+
const formDiffs = useMemo(
() => getChartFormDiffs(originalFormData, currentFormData),
[originalFormData, currentFormData],
);
+ const getFilterName = useCallback((filter: unknown): string | undefined => {
+ if (!filter || typeof filter !== 'object') {
+ return undefined;
+ }
+
+ const filterMeta = filter as Record<string, unknown>;
+ if (
+ typeof filterMeta.titleLabel === 'string' &&
+ filterMeta.titleLabel.length > 0
+ ) {
+ return filterMeta.titleLabel;
+ }
+ const subject = filterMeta.subject;
+ const subjectName =
+ typeof subject === 'string'
+ ? subject
+ : subject && typeof subject === 'object'
+ ? [
+ (subject as Record<string, unknown>).label,
+ (subject as Record<string, unknown>).column_name,
+ (subject as Record<string, unknown>).name,
+ (subject as Record<string, unknown>).column,
+ ].find(
+ candidate =>
+ typeof candidate === 'string' && candidate.length > 0,
+ )
+ : undefined;
+
+ if (typeof subjectName === 'string' && subjectName.length > 0) {
+ return subjectName;
+ }
+
+ const candidates = [filterMeta.column, filterMeta.label, filterMeta.name];
+ return candidates.find(
+ candidate => typeof candidate === 'string' && candidate.length > 0,
+ ) as string | undefined;
+ }, []);
+
+ const getBaseTitleWithoutDynamicSuffix = useCallback((title: string) => {
+ return title.replace(/\s+by\s+.+$/i, '').trimEnd();
+ }, []);
+
+ const displayTitle = useMemo(() => {
+ if (!shouldShowDynamicTitle) {
+ return getBaseTitleWithoutDynamicSuffix(sliceName ?? '');
+ }
+
+ const appliedFilters = chart.queriesResponse?.[0]?.applied_filters;
+ const adhocFilters = formData?.adhoc_filters;
+ const extraFormData = formData?.extra_form_data as
+ | Record<string, unknown>
+ | undefined;
+ const nativeFilters = extraFormData?.filters;
+
+ const hasFormDataFilterSource =
+ Array.isArray(adhocFilters) || Array.isArray(nativeFilters);
+ const activeFilters = hasFormDataFilterSource
+ ? [
+ ...(Array.isArray(adhocFilters) ? adhocFilters : []),
+ ...(Array.isArray(nativeFilters) ? nativeFilters : []),
+ ]
+ : Array.isArray(appliedFilters)
+ ? appliedFilters
+ : [];
Review Comment:
**Suggestion:** The fallback selection logic ignores `applied_filters`
whenever `adhoc_filters` or native filter arrays merely exist, even if those
arrays are empty. In common states where `adhoc_filters` is an empty array,
this drops active query filters from the dynamic title. Use `applied_filters`
when the merged form-data filter list is empty, not only when those arrays are
absent. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dynamic titles omit filters present in applied_filters.
- ⚠️ Users may misinterpret which filters are active.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct chart state where `chart.queriesResponse?.[0].applied_filters`
contains an
active filter (e.g. `{ column: 'region' }`, as in the fixtures at
`spec/fixtures/mockStore.js:19-36`), while the form-data arrays exist but
are empty:
`formData.adhoc_filters = []` and `formData.extra_form_data = { filters: []
}`, with
`showDynamicTitle` or `show_dynamic_title` true.
2. Render Explore so `ExploreViewContainer` passes this `chart` and
`formData` into
`ConnectedExploreChartHeader` at
`src/explore/components/ExploreViewContainer/index.tsx:20-39`, which in turn
passes them
to `ExploreChartHeader`.
3. In `displayTitle` at
`src/explore/components/ExploreChartHeader/index.tsx:290-331`,
`hasFormDataFilterSource` (lines 302-303) becomes `true` because both
`adhocFilters` and
`nativeFilters` are arrays, and `activeFilters` (lines 304-311) is computed
as the
concatenation of those empty arrays rather than falling back to
`applied_filters`.
4. Since `activeFilters` is empty, `activeFilterNames` (313-321) is also
empty and no
dynamic suffix is added, so `PageHeaderWithActions` at `index.tsx:371-383`
renders only
the base title even though `queriesResponse.applied_filters` indicates an
active filter
that should be reflected in the dynamic chart title.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=df6546acf5e243b197902f6f855b42a8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=df6546acf5e243b197902f6f855b42a8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
**Line:** 302:311
**Comment:**
*Incorrect Condition Logic: The fallback selection logic ignores
`applied_filters` whenever `adhoc_filters` or native filter arrays merely
exist, even if those arrays are empty. In common states where `adhoc_filters`
is an empty array, this drops active query filters from the dynamic title. Use
`applied_filters` when the merged form-data filter list is empty, not only when
those arrays are absent.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40697&comment_hash=6b817c320d24f766e6ba8b1810091b28a811ee225ab6db18821d825769c259f8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40697&comment_hash=6b817c320d24f766e6ba8b1810091b28a811ee225ab6db18821d825769c259f8&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:
##########
@@ -345,6 +442,7 @@ export const ExploreChartHeader:
FC<ExploreChartHeaderProps> = ({
{isPropertiesModalOpen && (
<PropertiesModal
show={isPropertiesModalOpen}
+ initialName={displayTitle}
onHide={closePropertiesModal}
onSave={updateSlice}
slice={slice}
Review Comment:
**Suggestion:** Prefilling the properties modal with the computed display
title causes persistent chart names to be overwritten by transient dynamic text
(or stripped text) on save, even when the user is editing unrelated properties.
Pass the persisted chart name instead of the computed display title so saving
properties does not mutate the canonical chart name unexpectedly. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Saving properties overwrites name with dynamic filter text.
- ⚠️ Chart lists and dashboards show unstable, filter-specific names.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a chart with dynamic titles enabled via the
`show_dynamic_title` control for
Treemap (see
`plugins/plugin-chart-echarts/src/Treemap/controlPanel.tsx:123-136`) and an
adhoc filter on `country`, as in the test setup at
`src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx:135-151`.
2. Render Explore: `ExploreChartHeader` computes `displayTitle` in
`displayTitle` at
`src/explore/components/ExploreChartHeader/index.tsx:290-331`, yielding e.g.
"Age
distribution of respondents by country", and passes this value into both the
header
(`editableTitleProps.title`) and the properties modal via
`initialName={displayTitle}` at
`index.tsx:444-448`.
3. Open "Edit chart properties" from the actions menu (menu wiring is passed
into
`useExploreAdditionalActionsMenu` at
`ExploreChartHeader/index.tsx:200-211`); inside
`PropertiesModal`, the local `name` state is initialized as `(initialName ??
slice.slice_name) || ''` at
`src/explore/components/PropertiesModal/index.tsx:77`, so the
Name textbox now contains the computed dynamic title string including
current filters.
4. Without intending to rename the chart, change another property (e.g.
description or
owners) and click Save; `onSubmit` in `PropertiesModal` at
`src/explore/components/PropertiesModal/index.tsx:244-258` builds a payload
with
`slice_name: name`, and `ExploreChartHeader` dispatches
`sliceUpdated(updatedSlice)` at
`index.tsx:178-181`, causing the canonical chart name stored on the backend
to be
overwritten with the transient dynamic title based on the current filter
state.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=340e3c1891b54fb7b612c37a47ef78bd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=340e3c1891b54fb7b612c37a47ef78bd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
**Line:** 444:448
**Comment:**
*Api Mismatch: Prefilling the properties modal with the computed
display title causes persistent chart names to be overwritten by transient
dynamic text (or stripped text) on save, even when the user is editing
unrelated properties. Pass the persisted chart name instead of the computed
display title so saving properties does not mutate the canonical chart name
unexpectedly.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40697&comment_hash=a8a4aa46b890776b4a15ea87c5c1d02a5507532c702d081af1ba2c2c9b548090&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40697&comment_hash=a8a4aa46b890776b4a15ea87c5c1d02a5507532c702d081af1ba2c2c9b548090&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
