bito-code-review[bot] commented on code in PR #40697:
URL: https://github.com/apache/superset/pull/40697#discussion_r3345914206
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/controlPanel.tsx:
##########
@@ -120,6 +120,19 @@ const config: ControlPanelConfig = {
},
},
],
+ [<ControlSubSectionHeader>{t('Chart
Title')}</ControlSubSectionHeader>],
+ [
+ {
+ name: 'show_dynamic_title',
+ config: {
+ type: 'CheckboxControl',
+ label: t('Show dynamic chart title'),
+ renderTrigger: true,
+ default: false,
+ description: t('Update the chart title dynamically based on the
data.'),
+ },
+ },
+ ],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new feature</b></div>
<div id="fix">
Rule 11730 requires unit tests for new features. The `show_dynamic_title`
checkbox is added to the control panel without corresponding unit tests. While
ExploreViewContainer.test.tsx tests the feature at the container level, the
Treemap-specific control panel configuration lacks dedicated unit tests to
validate the new option.
</div>
</div>
<small><i>Code Review Run #01fb83</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts:
##########
@@ -32,6 +32,7 @@ import { ReactNode } from 'react';
export interface NativeFiltersFormItem {
scope: NativeFilterScope;
name: string;
+ titleLabel?: string;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate type definition in same file</b></div>
<div id="fix">
Syntactic duplication detected in
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/types.ts.
The same 22-line type definition appears twice in this file: at lines 35-56
and 72-93. Consider creating a shared interface (e.g., NativeFilterConfig) and
referencing it in both locations to eliminate this duplication and improve code
maintainability.
</div>
</div>
<small><i>Code Review Run #01fb83</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.ts:
##########
@@ -44,6 +44,7 @@ interface AdhocFilterInput {
deck_slices?: unknown;
layerFilterScope?: unknown;
filterOptionName?: string;
+ titleLabel?: string;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing tests for titleLabel</b></div>
<div id="fix">
The new `titleLabel` property is added to the interface, class, constructor,
duplicateWith(), and equals() methods, but lacks unit test coverage. The
existing test file tests similar properties like `filterOptionName` and
`operator` in constructor, duplicateWith, and equals - add parallel tests for
`titleLabel`.
</div>
</div>
<small><i>Code Review Run #01fb83</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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 ?? '');
+ }
+
+ 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
+ : [];
+
+ const getFilterName = (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 activeFilterNames = Array.isArray(activeFilters)
+ ? [
+ ...new Set(
+ activeFilters
+ .map(filter => getFilterName(filter))
+ .filter((name): name is string => Boolean(name)),
+ ),
+ ]
+ : [];
+
+ const baseTitle = getBaseTitleWithoutDynamicSuffix(sliceName ?? '');
+
+ if (!activeFilterNames.length) {
+ return baseTitle;
+ }
+
+ const dynamicSuffix = `by ${activeFilterNames.join(', ')}`;
+ return baseTitle ? `${baseTitle} ${dynamicSuffix}` : dynamicSuffix;
+}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>SEMANTIC_DUPLICATION: Shared logic duplicated</b></div>
<div id="fix">
This new function duplicates ~85 lines of logic from
`ExploreChartHeader/index.tsx` displayTitle computation (lines 248-331). The
duplicate includes `getFilterName`, `getBaseTitleWithoutDynamicSuffix`, and
filter aggregation. This creates divergence risk if filter-handling logic
evolves independently in either location.
</div>
</div>
<small><i>Code Review Run #01fb83</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
