feat(ui): integrate ScrollToBottom components in SouthPane and Filter (#37804 issue fix)#40191
feat(ui): integrate ScrollToBottom components in SouthPane and Filter (#37804 issue fix)#40191YASHK-arch wants to merge 1 commit into
Conversation
…TitlePane for improved user experience
|
There was a problem hiding this comment.
Code Review Agent Run #bc7b67
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/components/ScrollToBottomButton/useScrollToBottom.ts - 1
- Remove ref.current from deps · Line 79-79
Review Details
-
Files reviewed - 7 · Commit Range:
cbd1376..cbd1376- superset-frontend/packages/superset-ui-core/src/components/ScrollToBottomButton/ScrollToBottomButton.test.tsx
- superset-frontend/packages/superset-ui-core/src/components/ScrollToBottomButton/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/ScrollToBottomButton/types.ts
- superset-frontend/packages/superset-ui-core/src/components/ScrollToBottomButton/useScrollToBottom.ts
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/src/SqlLab/components/SouthPane/index.tsx
- superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
| resizeObserver.disconnect(); | ||
| mutationObserver.disconnect(); | ||
| }; | ||
| }, [targetRef, updatePosition, targetRef.current]); |
There was a problem hiding this comment.
The targetRef.current value in the dependency array is unnecessary. ref.current is a mutable value that doesn't trigger re-renders when changed, so React cannot track changes to it. The targetRef object itself (already in deps) is stable across renders. Including ref.current triggers ESLint react-hooks/exhaustive-deps warnings.
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/packages/superset-ui-core/src/components/ScrollToBottomButton/useScrollToBottom.ts
+++ b/superset-frontend/packages/superset-ui-core/src/components/ScrollToBottomButton/useScrollToBottom.ts
@@ -76,6 +76,6 @@ export function useScrollToBottom(
resizeObserver.disconnect();
mutationObserver.disconnect();
};
- }, [targetRef, updatePosition, targetRef.current]);
+ }, [targetRef, updatePosition]);
Code Review Run #bc7b67
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Thank You for your suggestions Bito.
Applied the review fix: removed targetRef.current from the useEffect dependency array in useScrollToBottom.ts.
ref.current is mutable and does not participate in React’s dependency tracking, so listing it only triggers react-hooks/exhaustive-deps noise without reliable re-subscription. The stable targetRef object plus updatePosition (and the existing useLayoutEffect that calls updatePosition) are sufficient.
There was a problem hiding this comment.
The suggestion to remove targetRef.current from the useEffect dependency array is valid and appropriate. The ref.current is a mutable value that React cannot track, so including it in the dependency array only triggers unnecessary ESLint warnings without providing any benefit. Removing it aligns with best practices and ensures the dependency array contains only stable and relevant values (targetRef and updatePosition).
🚀 Summary
Long scrollable areas (e.g., SQL Lab results/history, native filter lists) require tedious manual scrolling—especially on mobile or when content updates in real time.
This PR introduces a reusable scroll-to-bottom control in
@superset-ui/coreand integrates it into key areas where users frequently need to jump to the latest content.fixes #37804
✨ New Components
Location:
packages/superset-ui-core/src/components/ScrollToBottomButton/🔹 ScrollToBottomButton
🔹 ScrollToBottomContainer
overflow-y: autoposition: relative🔹 useScrollToBottom (Hook)
scrollToBottomfunction🔌 Integrations
🧪 SQL Lab (South Pane)
Applied to:
Additional:
fullHeightenabled on south pane tabs for proper layout🎛️ Native Filters Config Modal
ScrollToBottomButtonremains unchanged
🎨 Design Notes
@superset-ui/core:ButtonTooltipIcons.VerticalAlignBottomOutlinedposition: relativecontainer→ does not affect document-level scrolling
aria-label💡 Impact