codeant-ai-for-open-source[bot] commented on code in PR #37167:
URL: https://github.com/apache/superset/pull/37167#discussion_r2698527075
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -165,200 +171,147 @@ const SliceHeader = forwardRef<HTMLDivElement,
SliceHeaderProps>(
formData,
width,
height,
- exportPivotExcel = () => ({}),
},
ref,
) => {
- const SliceHeaderExtension = extensionsRegistry.get(
- 'dashboard.slice.header',
- );
- const uiConfig = useUiConfig();
- const shouldShowRowLimitWarning =
- !isEmbedded() || uiConfig.showRowLimitWarning;
- const dashboardPageId = useContext(DashboardPageIdContext);
- const [headerTooltip, setHeaderTooltip] = useState<ReactNode | null>(null);
- const headerRef = useRef<HTMLDivElement>(null);
- // TODO: change to indicator field after it will be implemented
- const crossFilterValue = useSelector<RootState, any>(
- state => state.dataMask[slice?.slice_id]?.filterState?.value,
- );
- const isCrossFiltersEnabled = useSelector<RootState, boolean>(
- ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
- );
-
- const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
- state => state.charts[slice.slice_id].queriesResponse?.[0],
- );
-
- const theme = useTheme();
-
- const rowLimit = Number(formData.row_limit || -1);
- const sqlRowCount = Number(firstQueryResponse?.sql_rowcount || 0);
+ const SliceHeaderExtension =
extensionsRegistry.get('dashboard.slice.header');
+ const uiConfig = useUiConfig();
+ const dashboardPageId = useContext(DashboardPageIdContext);
+ const [headerTooltip, setHeaderTooltip] = useState<ReactNode | null>(null);
+ const headerRef = useRef<HTMLDivElement>(null);
+ // TODO: change to indicator field after it will be implemented
+ const crossFilterValue = useSelector<RootState, any>(
+ state => state.dataMask[slice?.slice_id]?.filterState?.value,
+ );
+ const isCrossFiltersEnabled = useSelector<RootState, boolean>(
+ ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
+ );
- const canExplore = !editMode && supersetCanExplore;
+ const canExplore = !editMode && supersetCanExplore;
- useEffect(() => {
- const headerElement = headerRef.current;
- if (canExplore) {
- setHeaderTooltip(getSliceHeaderTooltip(sliceName));
- } else if (
- headerElement &&
- (headerElement.scrollWidth > headerElement.offsetWidth ||
- headerElement.scrollHeight > headerElement.offsetHeight)
- ) {
- setHeaderTooltip(sliceName ?? null);
- } else {
- setHeaderTooltip(null);
- }
- }, [sliceName, width, height, canExplore]);
-
- const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice.slice_id}`;
+ useEffect(() => {
+ const headerElement = headerRef.current;
+ if (canExplore) {
+ setHeaderTooltip(getSliceHeaderTooltip(sliceName));
+ } else if (
+ headerElement &&
+ (headerElement.scrollWidth > headerElement.offsetWidth ||
+ headerElement.scrollHeight > headerElement.offsetHeight)
+ ) {
+ setHeaderTooltip(sliceName ?? null);
+ } else {
+ setHeaderTooltip(null);
+ }
+ }, [sliceName, width, height, canExplore]);
- const renderExploreLink = (title: string) => (
- <Link
- to={exploreUrl}
- css={(theme: SupersetTheme) => css`
- color: ${theme.colorText};
- text-decoration: none;
- :hover {
- text-decoration: underline;
- }
- display: inline-block;
- `}
- >
- {title}
- </Link>
- );
+ const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice.slice_id}`;
Review Comment:
**Suggestion:** Dereferencing `slice.slice_id` directly will throw a
TypeError if `slice` is undefined; compute the explore URL using optional
chaining (and a safe fallback) to avoid runtime errors when `slice` is not
provided. [null pointer]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard slice header fails rendering.
- ❌ Explore link generation throws runtime error.
- ⚠️ Chart header tooltip logic may not run.
```
</details>
```suggestion
const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice?.slice_id ??
''}`;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the app and render the dashboard page which mounts the SliceHeader
component
defined in superset-frontend/src/dashboard/components/SliceHeader/index.tsx.
The
exploreUrl is computed at line 207.
2. If the parent renders <SliceHeader> before the slice prop is populated
(the component
itself uses optional chaining elsewhere: see selector at lines 183-185:
`state =>
state.dataMask[slice?.slice_id]?.filterState?.value`), slice can be
undefined during
initial render.
3. On that initial render the code at line 207 executes `slice.slice_id`,
causing a
TypeError ("Cannot read property 'slice_id' of undefined") and breaking the
SliceHeader
render.
4. Result: header fails to mount, chart controls and explore links are not
rendered.
Because the file already uses `slice?.slice_id` in selectors (lines
183-185), guarding
here matches existing defensive patterns.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeader/index.tsx
**Line:** 207:207
**Comment:**
*Null Pointer: Dereferencing `slice.slice_id` directly will throw a
TypeError if `slice` is undefined; compute the explore URL using optional
chaining (and a safe fallback) to avoid runtime errors when `slice` is not
provided.
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.
```
</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]
