bito-code-review[bot] commented on code in PR #40000:
URL: https://github.com/apache/superset/pull/40000#discussion_r3484860883
##########
superset-frontend/src/filters/components/TimeGrain/types.ts:
##########
@@ -24,7 +24,7 @@ import { PluginFilterHooks, PluginFilterStylesProps } from
'../types';
interface PluginFilterTimeGrainCustomizeProps {
defaultValue?: string[] | null;
inputRef?: RefObject<HTMLInputElement>;
- time_grains?: string[];
+ timeGrains?: string[];
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Rename breaks dashboard-to-plugin data flow</b></div>
<div id="fix">
Renaming `time_grains` to `timeGrains` in
`PluginFilterTimeGrainCustomizeProps` (line 27) is a BREAKING CHANGE.
`getFormData` in `utils.ts` (line 89–90) outputs `time_grains` snake_case into
the formData object that flows into the plugin. Since the plugin's
`TimeGrainFilterPlugin.tsx` (line 115) reads `formData.timeGrains`, the
allowlist will always be `undefined` when the filter is rendered in a dashboard
— causing ALL time grain options to appear regardless of any saved
configuration. The snake_case name is intentionally consistent with the
dashboard config layer, API persistence, form state, and save transformer.
Revert this rename.
</div>
</div>
<small><i>Code Review Run #7c322b</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/filters/components/TimeGrain/TimeGrainPreFilter.integration.test.tsx:
##########
@@ -77,7 +81,9 @@ test('time grain pre-filter restricts dashboard filter
options', async () => {
};
// Step 2: Render the dashboard filter
- render(<PluginFilterTimegrain {...(dashboardConfig as any)} />);
+ render(<PluginFilterTimegrain {...dashboardConfig} />);
+
+ expect(screen.getByText('3 options')).toBeInTheDocument();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Hardcoded string without translation</b></div>
<div id="fix">
Import `tn` from '@apache-superset/core/translation' and replace the
hardcoded '3 options' assertion with `expect(screen.getByText(tn('%s option',
'%s options', 3, 3))).toBeInTheDocument()` so the test matches the component’s
translation logic.
</div>
</div>
<small><i>Code Review Run #7c322b</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]
