-
Notifications
You must be signed in to change notification settings - Fork 15.4k
fix: Console errors from various sources #34178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
Thanks for tackling this! Console has become unruly, makes it hard to use without active logging/filtering. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request addresses console errors by migrating menu components to the newer Ant Design v5 API and refactoring shared menu logic into reusable hooks.
- Converted the
ShareMenuItems
component into auseShareMenuItems
hook that returns aMenuItem
object. - Rewrote
SliceHeaderControls
to build its dropdown items from aMenuItem[]
array and consume the new share and drill-detail hooks. - Updated table dropdowns in both
TableChart
andTimeComparisonVisibility
to useopen
/onOpenChange
and themenu
prop instead ofvisible
/overlay
.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx | Refactored from a sub-component into useShareMenuItems hook returning a MenuItem . |
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx | Replaced JSX-based menu with a dynamic newMenuItems: MenuItem[] and integrated useShareMenuItems . |
superset-frontend/src/components/Chart/DrillDetail/useDrillDetailMenuItems.tsx | Added useDrillDetailMenuItems hook for programmatic drill-detail menu generation. |
superset-frontend/src/components/Chart/DrillDetail/index.ts | Exported the new useDrillDetailMenuItems hook. |
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx | Migrated Dropdown from Antd v4 API to v5: replaced visible /overlay with open /menu . |
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/TimeComparisonVisibility.tsx | Updated Dropdown usage to Antd v5 open /onOpenChange and menu prop, removed legacy Menu import. |
Comments suppressed due to low confidence (2)
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:45
- Consider adding unit tests for the new
useShareMenuItems
hook to verify the generatedMenuItem
structure and that the click handlers are invoked correctly.
export const useShareMenuItems = (props: ShareMenuItemProps): MenuItem => {
superset-frontend/src/components/Chart/DrillDetail/useDrillDetailMenuItems.tsx:120
- Add unit tests for
useDrillDetailMenuItems
to validate menu item generation, including disabled states and submenu offsets under different data scenarios.
export const useDrillDetailMenuItems = ({
onClick: () => onCopyLink, | ||
}, | ||
{ | ||
key: MenuKeys.ShareByEmail, | ||
label: emailMenuItemTitle, | ||
onClick: () => onShareByEmail, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onClick handler returns the function reference instead of invoking it. It should be either onClick: onCopyLink
or onClick: () => onCopyLink()
to actually execute the copy logic.
onClick: () => onCopyLink, | |
}, | |
{ | |
key: MenuKeys.ShareByEmail, | |
label: emailMenuItemTitle, | |
onClick: () => onShareByEmail, | |
onClick: () => onCopyLink(), | |
}, | |
{ | |
key: MenuKeys.ShareByEmail, | |
label: emailMenuItemTitle, | |
onClick: () => onShareByEmail(), |
Copilot uses AI. Check for mistakes.
onClick: () => onCopyLink, | ||
}, | ||
{ | ||
key: MenuKeys.ShareByEmail, | ||
label: emailMenuItemTitle, | ||
onClick: () => onShareByEmail, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onClick handler returns the function reference instead of invoking it. It should be either onClick: onShareByEmail
or onClick: () => onShareByEmail()
to actually open the email share dialog.
onClick: () => onCopyLink, | |
}, | |
{ | |
key: MenuKeys.ShareByEmail, | |
label: emailMenuItemTitle, | |
onClick: () => onShareByEmail, | |
onClick: onCopyLink, | |
}, | |
{ | |
key: MenuKeys.ShareByEmail, | |
label: emailMenuItemTitle, | |
onClick: onShareByEmail, |
Copilot uses AI. Check for mistakes.
@@ -54,6 +54,7 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { | |||
dashboardComponentId, | |||
title, | |||
disabled, | |||
children, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The children
prop is destructured but not used within this hook. Removing it will clean up the props signature.
children, |
Copilot uses AI. Check for mistakes.
Great to see the menu refactor happening! Gave it a shot during the theming branch effort and ended up bailing as it was pretty tangled up. |
c80f57d
to
14195d7
Compare
@geido Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Inconsistent HTML sanitization ▹ view | 🧠 Incorrect |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/components/Chart/DrillDetail/index.ts | ✅ |
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx | ✅ |
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/TimeComparisonVisibility.tsx | ✅ |
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx | ✅ |
superset-frontend/src/components/Chart/DrillDetail/useDrillDetailMenuItems.tsx | ✅ |
superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.tsx | ✅ |
superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx | ✅ |
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx | ✅ |
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx | ✅ |
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
const Filter = ({ | ||
children, | ||
stripHTML = false, | ||
}: { | ||
children: ReactNode; | ||
stripHTML: boolean; | ||
}) => { | ||
const content = | ||
stripHTML && typeof children === 'string' | ||
? removeHTMLTags(children) | ||
: children; | ||
return <span>{content}</span>; | ||
}; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@geido Ephemeral environment spinning up at http://52.33.229.141:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
SUMMARY
After a few big migrations and theming. Dev console errors increased. I aim to fix whatever i can in this pr.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Check browser console
ADDITIONAL INFORMATION