codeant-ai-for-open-source[bot] commented on code in PR #42849:
URL: https://github.com/apache/superset/pull/42849#discussion_r3730200980
##########
superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx:
##########
@@ -50,6 +51,11 @@ export default function PluginFilterTimegrain(
} = props;
const { defaultValue } = formData;
+ const reduxContext = useContext(ReactReduxContext);
+ const dashboardTimeGrainAllowlist: string[] | undefined =
+ reduxContext?.store?.getState?.()?.dashboardInfo?.metadata
+ ?.time_grain_allowlist;
Review Comment:
**Suggestion:** The component reads Redux state imperatively through
`getState()` but never subscribes to store updates. If the filter mounts before
dashboard hydration, or the metadata changes while the dashboard is open, this
component will not rerender for the new allowlist and will continue displaying
the fallback options. Read this value through a Redux subscription such as
`useSelector`, or otherwise trigger a rerender when dashboard metadata changes.
[stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard metadata changes do not update mounted time-grain filters.
- ⚠️ Users may see options inconsistent with saved dashboard configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9d2bc3b5d38041288a024d7533407e2b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9d2bc3b5d38041288a024d7533407e2b&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/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx
**Line:** 54:57
**Comment:**
*Stale Reference: The component reads Redux state imperatively through
`getState()` but never subscribes to store updates. If the filter mounts before
dashboard hydration, or the metadata changes while the dashboard is open, this
component will not rerender for the new allowlist and will continue displaying
the fallback options. Read this value through a Redux subscription such as
`useSelector`, or otherwise trigger a rerender when dashboard metadata changes.
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%2F42849&comment_hash=b1873f2ccd2dfc1892947d27da243eae78cd8a1e399f03584888d9a00b45456f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42849&comment_hash=b1873f2ccd2dfc1892947d27da243eae78cd8a1e399f03584888d9a00b45456f&reaction=dislike'>👎</a>
##########
superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx:
##########
@@ -101,22 +104,51 @@ export default function PluginFilterTimegrain(
);
}
- const options = (data || [])
- .map((row: { name: string; duration: string }) => {
- const { name, duration } = row;
- return {
- label: name,
- value: duration,
- };
- })
- // Apply allowlist filter if timeGrains is configured, but keep current
selection visible
- .filter(option => {
- const allowlist = formData.timeGrains;
- if (!allowlist || allowlist.length === 0) {
- return true;
- }
- return allowlist.includes(option.value) || value.includes(option.value);
- });
+ const options = useMemo(() => {
+ const allOptions = (data || [])
+ .map((row: { name: string; duration: string }) => {
+ const { name, duration } = row;
+ return {
+ label: name,
+ value: duration,
+ };
+ });
+
+ const allowlist =
+ dashboardTimeGrainAllowlist?.length > 0
+ ? dashboardTimeGrainAllowlist
+ : formData.timeGrains;
+
+ if (!allowlist || allowlist.length === 0) {
+ return allOptions;
+ }
+
+ const allowedSet = new Set(allowlist);
+ return allOptions.filter(option => allowedSet.has(option.value));
+ }, [data, dashboardTimeGrainAllowlist, formData.timeGrains]);
+
+ const validValue = useMemo(() => {
+ if (options.length === 0) return [];
+ const optionValues = new Set(options.map(o => o.value));
+ return value.filter(v => optionValues.has(v));
+ }, [value, options]);
Review Comment:
**Suggestion:** Filtering the controlled `Select` value removes any
currently active grain that is outside the new allowlist without clearing the
corresponding `filterState` or data mask. The backend can therefore continue
querying with a grain that is invisible in the control, leaving the user unable
to see or clear the active selection. Preserve the active value in the
displayed options, or explicitly clear and publish the invalid selection when
the allowlist changes. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Native filter can query using an invisible time grain.
- ⚠️ Users cannot clear excluded active selections through the control.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2e05bacbd2774a8da42df8d106e3d188&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2e05bacbd2774a8da42df8d106e3d188&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/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx
**Line:** 130:134
**Comment:**
*Api Mismatch: Filtering the controlled `Select` value removes any
currently active grain that is outside the new allowlist without clearing the
corresponding `filterState` or data mask. The backend can therefore continue
querying with a grain that is invisible in the control, leaving the user unable
to see or clear the active selection. Preserve the active value in the
displayed options, or explicitly clear and publish the invalid selection when
the allowlist changes.
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%2F42849&comment_hash=f42fd66077e140fe19a2cdad232545677b8c369bae4c5702fe4583402b19ffae&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42849&comment_hash=f42fd66077e140fe19a2cdad232545677b8c369bae4c5702fe4583402b19ffae&reaction=dislike'>👎</a>
##########
superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx:
##########
@@ -101,22 +104,51 @@ export default function PluginFilterTimegrain(
);
}
- const options = (data || [])
- .map((row: { name: string; duration: string }) => {
- const { name, duration } = row;
- return {
- label: name,
- value: duration,
- };
- })
- // Apply allowlist filter if timeGrains is configured, but keep current
selection visible
- .filter(option => {
- const allowlist = formData.timeGrains;
- if (!allowlist || allowlist.length === 0) {
- return true;
- }
- return allowlist.includes(option.value) || value.includes(option.value);
- });
+ const options = useMemo(() => {
+ const allOptions = (data || [])
+ .map((row: { name: string; duration: string }) => {
+ const { name, duration } = row;
+ return {
+ label: name,
+ value: duration,
+ };
+ });
+
+ const allowlist =
+ dashboardTimeGrainAllowlist?.length > 0
+ ? dashboardTimeGrainAllowlist
+ : formData.timeGrains;
+
+ if (!allowlist || allowlist.length === 0) {
+ return allOptions;
+ }
+
+ const allowedSet = new Set(allowlist);
+ return allOptions.filter(option => allowedSet.has(option.value));
+ }, [data, dashboardTimeGrainAllowlist, formData.timeGrains]);
+
+ const validValue = useMemo(() => {
+ if (options.length === 0) return [];
+ const optionValues = new Set(options.map(o => o.value));
+ return value.filter(v => optionValues.has(v));
+ }, [value, options]);
+
+ const hasInitRef = useRef(false);
+ useEffect(() => {
+ if (hasInitRef.current) return;
+ if (options.length === 0) return;
+
+ const optionValues = new Set(options.map(o => o.value));
+ const target = ensureIsArray<string>(defaultValue).filter(v =>
+ optionValues.has(v),
+ );
+
+ hasInitRef.current = true;
+
+ if (target.length > 0) {
+ handleChange(target);
Review Comment:
**Suggestion:** The one-shot initialization runs after the effect that
applies `filterState.value`, so when options become available it
unconditionally replaces an already-restored or user-selected value with
`defaultValue`. This can overwrite the active dashboard filter during
asynchronous data or dashboard hydration. Only apply the default when there is
no current filter-state value, or give the restored filter state precedence.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Restored native-filter selections can be replaced on data arrival.
- ⚠️ Dashboard filter state and visible selection become incorrect.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c9d99cd65ac640f89789b7fc7b890f1c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c9d99cd65ac640f89789b7fc7b890f1c&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/filters/components/TimeGrain/TimeGrainFilterPlugin.tsx
**Line:** 141:149
**Comment:**
*Logic Error: The one-shot initialization runs after the effect that
applies `filterState.value`, so when options become available it
unconditionally replaces an already-restored or user-selected value with
`defaultValue`. This can overwrite the active dashboard filter during
asynchronous data or dashboard hydration. Only apply the default when there is
no current filter-state value, or give the restored filter state precedence.
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%2F42849&comment_hash=eb58458d3ba3f1834ddea7389cc869422c667c7c8e725d57e85dbca9e208f551&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42849&comment_hash=eb58458d3ba3f1834ddea7389cc869422c667c7c8e725d57e85dbca9e208f551&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]
