codeant-ai-for-open-source[bot] commented on code in PR #40697:
URL: https://github.com/apache/superset/pull/40697#discussion_r3345888637
##########
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 : []),
+ ]
Review Comment:
**🟠Architect Review — HIGH**
Dynamic title composition expects active filters to expose `titleLabel` on
the filter objects it inspects (from `formData.adhoc_filters` /
`formData.extra_form_data.filters`), but native filter pipelines only emit
query clauses like `{ col, op, val }`, so dashboard-level `titleLabel`
configured on native filters is never surfaced in dynamic chart titles.
**Suggestion:** When building Explore form data from dashboard context,
propagate the native filter `titleLabel` into the corresponding adhoc filter
objects and/or attach it to `extra_form_data.filters`, resolving display names
from native filter metadata or dataMask instead of assuming the query clause
payload already contains that label.
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a827677bc774c76a63447d32c78a856&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=1a827677bc774c76a63447d32c78a856&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 an **Architect / Logical Review** comment left during a code review.
These reviews are first-class, important findings — not optional suggestions.
Do NOT dismiss this as a 'big architectural change' just because the title says
architect review; most of these can be resolved with a small, localized fix
once the intent is understood.
**Path:**
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
**Line:** 248:308
**Comment:**
*HIGH: Dynamic title composition expects active filters to expose
`titleLabel` on the filter objects it inspects (from `formData.adhoc_filters` /
`formData.extra_form_data.filters`), but native filter pipelines only emit
query clauses like `{ col, op, val }`, so dashboard-level `titleLabel`
configured on native filters is never surfaced in dynamic chart titles.
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.
If a suggested approach is provided above, use it as the authoritative
instruction. If no explicit code suggestion is given, you MUST still draft and
apply your own minimal, localized fix — do not punt back with 'no suggestion
provided, review manually'. Keep the change as small as possible: add a guard
clause, gate on a loading state, reorder an await, wrap in a conditional, etc.
Do not refactor surrounding code or expand scope beyond the finding.
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>
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -281,6 +281,97 @@ function isAggregatedChartType(vizType: string |
undefined): boolean {
return vizType ? AGGREGATED_CHART_TYPES.includes(vizType) : false;
}
+function getDisplaySliceTitle(
+ sliceName: string | null,
+ formData: QueryFormData,
+ chart: ChartState,
+): string {
+ const getBaseTitleWithoutDynamicSuffix = (title: string) =>
+ title.replace(/\s+by\s+.+$/i, '').trimEnd();
+
+ const shouldShowDynamicTitle =
+ formData?.show_dynamic_title === true ||
+ formData?.showDynamicTitle === true;
+ if (!shouldShowDynamicTitle) {
+ return getBaseTitleWithoutDynamicSuffix(sliceName ?? '');
+ }
Review Comment:
**🟠Architect Review — HIGH**
Dynamic title base-title extraction unconditionally strips any " by …"
suffix, so a chart legitimately named "Sales by Region" is displayed and saved
as "Sales" whenever dynamic titles are off or not configured.
**Suggestion:** Track and persist a separate base title (or otherwise mark
dynamically generated suffixes) and only strip the "by …" suffix when it is
known to have been added by the dynamic-title logic, instead of regex-trimming
all " by …" patterns from user-authored titles.
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9373d6eb18b04a17aaeef7a3678ae711&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=9373d6eb18b04a17aaeef7a3678ae711&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 an **Architect / Logical Review** comment left during a code review.
These reviews are first-class, important findings — not optional suggestions.
Do NOT dismiss this as a 'big architectural change' just because the title says
architect review; most of these can be resolved with a small, localized fix
once the intent is understood.
**Path:**
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
**Line:** 289:297
**Comment:**
*HIGH: Dynamic title base-title extraction unconditionally strips any "
by …" suffix, so a chart legitimately named "Sales by Region" is displayed and
saved as "Sales" whenever dynamic titles are off or not configured.
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.
If a suggested approach is provided above, use it as the authoritative
instruction. If no explicit code suggestion is given, you MUST still draft and
apply your own minimal, localized fix — do not punt back with 'no suggestion
provided, review manually'. Keep the change as small as possible: add a guard
clause, gate on a loading state, reorder an await, wrap in a conditional, etc.
Do not refactor surrounding code or expand scope beyond the finding.
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>
--
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]
