custom dashboard #193
custom dashboard #193
Conversation
WalkthroughThe changes in this pull request introduce several enhancements across multiple components and services in the application. Notable updates include the addition of new properties and methods to facilitate dashboard customization, improvements in state management, and refinements in component rendering logic. The Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning @grafana/eslint-config > [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (24)
public/app/fn-app/types.ts (1)
33-33: Consider adding JSDoc documentation.To improve maintainability, consider adding JSDoc comments to document the purpose and usage of the
isCustomDashboardproperty.+ /** Indicates whether this dashboard is customizable by the user */ isCustomDashboard?: boolean;public/app/features/dashboard/components/AddPanelButton/AddPanelButton.tsx (1)
13-13: Document the purpose ofisFNDashboardThe purpose and meaning of "FN" in
isFNDashboardis not immediately clear. Consider adding a JSDoc comment to explain its purpose and when it should be used.+ /** Flag indicating if this is a Function Navigator dashboard, which affects button styling */ isFNDashboard?: boolean;public/app/core/reducers/fn-slice.ts (1)
52-52: Document the purpose ofisCustomDashboardConsider adding a comment explaining the purpose and implications of this flag, similar to the existing comment for
FNDashboard.FNDashboard: false, + // NOTE: Controls whether the dashboard is customizable by the user isCustomDashboard: false,public/app/fn-app/fn-dashboard-page/render-fn-dashboard.tsx (3)
Line range hint
47-51: Consider memoizing the pathname valueThe pathname change effect might trigger unnecessary updates if window.location.pathname hasn't changed. Consider memoizing the pathname or adding it to the dependency array.
useEffect(() => { + const currentPathname = window.location.pathname; - locationService.fnPathnameChange(window.location.pathname, queryParams); + locationService.fnPathnameChange(currentPathname, queryParams); - }, [queryParams]); + }, [queryParams, window.location.pathname]);
Line range hint
53-69: Remove redundant spread of DEFAULT_DASHBOARD_PAGE_PROPSThe current implementation spreads DEFAULT_DASHBOARD_PAGE_PROPS twice, which is unnecessary and could impact performance.
const dashboardPageProps: DashboardPageProps = useMemo( () => - merge({}, DEFAULT_DASHBOARD_PAGE_PROPS, { - ...DEFAULT_DASHBOARD_PAGE_PROPS, + merge({}, DEFAULT_DASHBOARD_PAGE_PROPS, { match: { params: { ...props, }, }, location: locationService.getLocation(), queryParams, hiddenVariables, controlsContainer, isLoading, }), [controlsContainer, hiddenVariables, isLoading, props, queryParams] );
isCustomDashboardprop is already part of the interface but not used in the componentThe verification reveals that:
isCustomDashboardis properly defined in theFNDashboardPropsinterface inpublic/app/fn-app/types.ts- The component
RenderFNDashboardreceives the prop through its type definition but doesn't utilize it in its implementation- The property is actively used throughout the codebase, particularly in
DashboardPage.tsxandDashboardGrid.tsxfor controlling dashboard behaviorThe component should handle this prop as it's a crucial part of the dashboard functionality, affecting editability and rendering behavior.
🔗 Analysis chain
Line range hint
29-29: Verify custom dashboard props handlingThe component receives FNDashboardProps but should also handle the new
isCustomDashboardproperty mentioned in the summary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify custom dashboard property handling # Check FNDashboardProps interface definition rg -A 10 "interface FNDashboardProps" # Look for isCustomDashboard usage in related files rg "isCustomDashboard.*DashboardPage"Length of output: 697
Script:
#!/bin/bash # Let's check for any isCustomDashboard property usage in the codebase rg "isCustomDashboard" -A 3 # Also check the component implementation for any custom dashboard related logic rg -A 10 "RenderFNDashboard.*=.*props"Length of output: 7846
public/app/features/dashboard/dashgrid/DashboardPanel.tsx (1)
Line range hint
1-140: Consider modernizing to functional component in futureWhile the current implementation is stable and working well, consider gradually modernizing this component to use functional components with hooks in future iterations. This would align with current React best practices and potentially simplify the state management code.
packages/grafana-runtime/src/services/LocationService.tsx (1)
Line range hint
24-51: Consider architectural implications of location service changesWhile adding custom dashboard functionality, modifying core navigation services requires careful consideration:
- The LocationService is a fundamental service used throughout Grafana
- Changes here could affect all navigation flows, not just dashboard-related ones
- Direct manipulation of history state could interfere with Grafana's routing system
Consider these alternatives:
- Use existing methods like
partial()orpush()instead of adding a new method- If a new method is needed, implement it at the dashboard service level instead
- Document the specific use case this solves in the custom dashboard feature
public/app/features/dashboard/dashgrid/PanelStateWrapper.tsx (1)
621-621: Consider improving readability with a descriptive variable nameThe boolean expression could be more readable by extracting it into a descriptive variable name.
- isFnDashboard: state.fnGlobalState.FNDashboard && !state.fnGlobalState.isCustomDashboard, + const isStandardFunctionDashboard = state.fnGlobalState.FNDashboard && !state.fnGlobalState.isCustomDashboard; + isFnDashboard: isStandardFunctionDashboard,public/app/features/dashboard/dashgrid/DashboardGrid.tsx (2)
246-252: Simplify panel rendering logic when applying panel filtersThe conditional statements for adding panels to
panelElementscan be simplified to improve readability.Apply this diff to streamline the condition:
if (!panelFilter) { panelElements.push(p); } else { - if (panelFilter.test(panel.title)) { - panelElements.push(p); - } + if (panelFilter.test(panel.title)) { + panelElements.push(p); + } }Or even better, combine the conditions:
- if (!panelFilter) { - panelElements.push(p); - } else { - if (panelFilter.test(panel.title)) { - panelElements.push(p); - } - } + if (!panelFilter || panelFilter.test(panel.title)) { + panelElements.push(p); + }
340-341: Review conditional logic for draggable and resizable propertiesThe conditions for
isDraggableandisResizablemay be complex and could benefit from simplification for better readability.Consider simplifying the ternary operators:
<ReactGridLayout width={width} - isDraggable={isFnDashboard && !isCustomDashboard ? false : draggable} - isResizable={isFnDashboard && !isCustomDashboard ? false : isEditable} + isDraggable={!isFnDashboard || isCustomDashboard ? draggable : false} + isResizable={!isFnDashboard || isCustomDashboard ? isEditable : false} // ... other props />This may make the conditions easier to understand.
public/app/features/dashboard/containers/DashboardPage.tsx (13)
3-3: Remove unused import ofPureComponentThe
PureComponentimported from'react'is not used in the file and can be removed to clean up the code.Apply this diff to remove the unused import:
-import { PureComponent } from 'react';
16-16: Consider lazy loading internationalization stringsIf the
tfunction from'app/core/internationalization'is not heavily used, consider lazy loading it to improve performance.
22-22: Organize imports for better readabilityGroup related imports together and order them logically to enhance code readability.
80-80: Consistent use of TypeScript utility typesConsider using
PickorPartialconsistently when extracting types fromFnGlobalStateto maintain uniform code style.
101-102: Maintain alphabetical order inmapStateToPropsFor better readability and maintainability, consider arranging the properties in
mapStateToPropsin alphabetical order.
137-137: Ensure proper declaration ofscrollElementin stateThe optional property
scrollElementof typeScrollRefElementin the component state should be initialized appropriately to avoid potentialundefinedreferences.
161-161: DestructurepropsconsistentlyFor consistency, destructure
propsparameters in a consistent manner throughout the component.
201-201: Include all necessary props in destructuringEnsure that all used props are included when destructuring to avoid potential
undefinedvalues.
Line range hint
207-220: Handle potential infinite loops incomponentDidUpdateThe
componentDidUpdatemethod re-initializes the dashboard when certain conditions are met, which could lead to an infinite update loop if not carefully managed.Ensure that the
initDashboardcall and state updates do not cause repeated re-renders.
387-387: Destructure additional props for clarityConsider destructuring
initPhasefromthis.propsfor consistency and clarity.
400-400: Simplify the logic forisFNDashboardEditableThe assignment of
isFNDashboardEditablecan be simplified for better readability.Simplify the conditional logic:
-const isFNDashboardEditable = (isCustomDashboard && FNDashboard) || !FNDashboard; +const isFNDashboardEditable = isCustomDashboard && FNDashboard ? true : !FNDashboard;
452-453: Consider extracting inline styles to a CSS classFor better maintainability, move the inline styles into a dedicated CSS class instead of using inline styling.
Create a CSS class and apply it to the
<div>element..dashboard-toolbar { display: flex; justify-content: flex-end; gap: 4px; }Then update the code:
-<div - style={{ - display: 'flex', - justifyContent: 'flex-end', - gap: 4, - }} -> +<div className="dashboard-toolbar">
Line range hint
534-541: Avoid side effects ingetDerivedStateFromPropsUsing functions like
updateStatePageNavFromPropsthat may cause side effects withingetDerivedStateFromPropscan lead to unexpected behavior. Ensure that this method is pure and only computes and returns new state.Refactor
updateStatePageNavFromPropsto be a pure function and avoid mutating the state directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
packages/grafana-runtime/src/services/LocationService.tsx(1 hunks)public/app/core/components/Page/Page.tsx(1 hunks)public/app/core/reducers/fn-slice.ts(2 hunks)public/app/features/dashboard/components/AddPanelButton/AddPanelButton.tsx(2 hunks)public/app/features/dashboard/containers/DashboardPage.tsx(14 hunks)public/app/features/dashboard/dashgrid/DashboardGrid.tsx(11 hunks)public/app/features/dashboard/dashgrid/DashboardPanel.tsx(1 hunks)public/app/features/dashboard/dashgrid/PanelStateWrapper.tsx(1 hunks)public/app/fn-app/create-mfe.ts(1 hunks)public/app/fn-app/fn-app-provider.tsx(3 hunks)public/app/fn-app/fn-dashboard-page/render-fn-dashboard.tsx(1 hunks)public/app/fn-app/types.ts(1 hunks)
🔇 Additional comments (18)
public/app/fn-app/types.ts (1)
33-33: LGTM! The new property is well-implemented.
The isCustomDashboard optional boolean property is correctly typed and maintains backward compatibility.
public/app/features/dashboard/components/AddPanelButton/AddPanelButton.tsx (2)
16-16: LGTM!
The component signature is correctly updated to include the new prop while maintaining backward compatibility.
33-34: Verify design consistency for button styling
The button now has different sizes based on dashboard type (md for FN dashboards, sm for others) and uses the primary variant. This might create visual inconsistency across different dashboard views.
Let's check if this is consistent with other similar buttons in the codebase:
✅ Verification successful
Button styling is consistent with existing patterns
The verification shows that the button styling follows established patterns in the dashboard components:
- The ShareButton component in DashNav also uses
variant="primary"withsize="sm", matching our non-FN dashboard case - Primary variant is commonly used for important actions across dashboard components (share, apply, save, etc.)
- The size variation based on context is not unprecedented, as seen in PanelEditor where size is also conditionally set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Button components with size props to verify consistency
ast-grep --pattern 'Button $$$size={$_}$$$'
# Search for other primary variant buttons in dashboard components
rg -A 2 "variant=\"primary\"" "public/app/features/dashboard"
Length of output: 6143
public/app/core/reducers/fn-slice.ts (2)
9-9: Consider adding isCustomDashboard to mapped props types
The new property is not included in FnPropMappedFromState type. If components need to access this state via prop mapping, consider adding it to the mapped props.
Line range hint 65-75: Verify permission checks for custom dashboard state changes
The reducers allow unrestricted updates to isCustomDashboard. Since this property affects dashboard editing permissions (as mentioned in the PR summary), ensure that appropriate permission checks are implemented in the components that dispatch these actions.
public/app/fn-app/fn-dashboard-page/render-fn-dashboard.tsx (1)
20-20: Verify locationService initialization timing
The change from type assertion to locationService.getHistory() is more robust, but we should ensure that locationService is properly initialized when this code runs.
✅ Verification successful
Based on the gathered information, I can now provide a final response regarding the locationService initialization concern:
locationService is properly initialized and safe to use
The locationService is a singleton that's automatically initialized when imported from @grafana/runtime. It's implemented as a HistoryWrapper that's instantiated with browser history by default. The service is initialized before any components are rendered, making it safe to use getHistory() and other methods.
Key findings:
- locationService is created as a singleton with
new HistoryWrapper()on import - HistoryWrapper safely handles history initialization for both browser and test environments
- The service can only be overridden in test environments via setLocationService()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if locationService initialization is handled properly
# Look for locationService initialization and potential race conditions
# Search for locationService initialization
rg -A 5 "locationService.*init|init.*locationService"
# Check if there are any error handlers for locationService
rg "catch.*locationService|locationService.*error"
Length of output: 1205
Script:
#!/bin/bash
# Let's check how locationService is used and initialized in the codebase
# Check for locationService imports and its usage pattern
rg -A 3 "import.*locationService|locationService.*from"
# Look for any singleton or service initialization patterns
ast-grep --pattern 'const locationService = $_'
# Check the actual service definition
fd locationService --exec cat {}
Length of output: 100471
Script:
#!/bin/bash
# Let's check how locationService is initialized and used in the runtime package
# since it appears to be the source of the service
# Check the LocationService implementation
fd LocationService.ts --exec cat {}
# Look for any initialization patterns in the main app
rg -A 5 "initLocationService|setLocationService"
Length of output: 11981
public/app/core/components/Page/Page.tsx (1)
96-102: Clean implementation of wrapper styles
The refactored style implementation is cleaner and more maintainable. Good use of object syntax and proper labeling.
packages/grafana-runtime/src/services/LocationService.tsx (1)
Line range hint 48-51: Critical: Unsafe implementation of pathname change
The current implementation has several critical issues:
- Directly mutating
history.locationis unsafe and bypasses the History API's proper update mechanisms - No validation of the path parameter could lead to invalid URLs
- Changes won't trigger proper history events, potentially breaking React's state management
- Inconsistent with how other methods in this service handle location updates
Consider this safer implementation:
- fnPathnameChange(path: string, queryParams: any) {
- this.history.location.pathname = path;
- this.history.location.search = urlUtil.toUrlParams(queryParams);
+ updatePathname(path: string, queryParams: UrlQueryMap) {
+ const search = urlUtil.toUrlParams(queryParams);
+ this.history.push({
+ pathname: path,
+ search,
+ });
}Let's verify the usage of this method across the codebase to assess the impact:
public/app/fn-app/create-mfe.ts (1)
283-283: LGTM! Verify integration with custom dashboard feature.
The addition of isCustomDashboard to the state update follows the existing pattern and is consistent with the codebase.
Let's verify the integration with the custom dashboard feature:
✅ Verification successful
Integration with custom dashboard feature is properly implemented
The verification confirms that isCustomDashboard is:
- Correctly defined in
FNDashboardPropsinterface - Initialized as
falsein the initial state - Properly integrated across key components:
- Used in
DashboardPagefor conditional rendering and edit controls - Handled in
DashboardGridfor drag-and-drop functionality - Correctly propagated through the Redux state
- Used in
The addition of isCustomDashboard to the state update in create-mfe.ts is consistent with its usage throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the custom dashboard feature integration
# Check for related components and interfaces that should handle isCustomDashboard
# Check for the property definition in interfaces
ast-grep --pattern 'interface $_ {
$$$
isCustomDashboard?: boolean;
$$$
}'
# Check for usage in components
rg -A 5 'isCustomDashboard'
# Check for initialization in initial state
rg 'INITIAL_FN_STATE.*=.*{' -A 10
Length of output: 11752
public/app/features/dashboard/dashgrid/PanelStateWrapper.tsx (1)
621-621: Verify impact on existing function dashboards
The new condition !state.fnGlobalState.isCustomDashboard changes how function dashboards are identified. This could affect existing dashboards that were previously marked as function dashboards.
✅ Verification successful
Based on the search results, I can now provide a final response about the impact of the change:
Change to isFnDashboard condition is safe and intentional
The new condition !state.fnGlobalState.isCustomDashboard in PanelStateWrapper is consistent with the existing behavior in DashboardGrid where:
- Custom dashboards are meant to be editable even when they are function dashboards (
isFnDashboard && !isCustomDashboard ? false : draggable) - The same logic is applied to resizing (
isFnDashboard && !isCustomDashboard ? false : isEditable)
This change aligns the panel state behavior with the grid's handling of function dashboards vs custom dashboards, ensuring consistent editability rules across the dashboard.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impact on existing dashboards
# Look for any direct usage of FNDashboard state without the isCustomDashboard check
# Search for other direct uses of FNDashboard state
rg "FNDashboard" --type ts --type tsx
# Search for components consuming isFnDashboard prop
ast-grep --pattern 'interface Props {
$$$
isFnDashboard?: boolean;
$$$
}'
Length of output: 2222
Script:
#!/bin/bash
# Let's search for more context about FNDashboard and isCustomDashboard usage
# Note: Adding .tsx extension explicitly since previous rg command had issues
# Search for FNDashboard usage
rg "FNDashboard" --type-add 'tsx:*.tsx' --type tsx -A 2
# Search for isCustomDashboard usage
rg "isCustomDashboard" --type-add 'tsx:*.tsx' --type tsx -A 2
# Search for components that pass isFnDashboard prop
ast-grep --pattern '$_.$$$isFnDashboard$$$'
Length of output: 100291
Script:
#!/bin/bash
# Let's try a more focused search to understand the usage of FNDashboard and isCustomDashboard
# in the specific components that matter for this change
# Search for FNDashboard and isCustomDashboard in dashboard grid and panel components
rg "FNDashboard|isCustomDashboard" --type-add 'tsx:*.tsx' --type tsx -g "**/dashgrid/*" -g "**/dashboard/components/*" -A 2
# Look for components that consume isFnDashboard prop
ast-grep --pattern 'isFnDashboard={$_}'
Length of output: 1776
public/app/fn-app/fn-app-provider.tsx (2)
43-49: Function commandPaletteActionSelected correctly reports action selection
The newly added commandPaletteActionSelected function appropriately reports interactions when a command palette action is selected. This enhances analytics and user interaction tracking.
60-62: Ensure actions prop in KBarProvider is populated if required
Currently, the KBarProvider is initialized with an empty actions array. If the command palette is intended to have actionable items, consider populating this array to enable its functionality.
public/app/features/dashboard/dashgrid/DashboardGrid.tsx (1)
158-160: Clarify early return in onLayoutChange when panel filter is active
When a panelFilter is applied, the onLayoutChange method returns immediately, preventing layout updates. Ensure this behavior is intentional and consider adding a comment to explain why updates are skipped when filtering is active.
If this is intentional to prevent layout changes during filtering, no action is needed. If not, consider removing the early return.
public/app/features/dashboard/containers/DashboardPage.tsx (5)
11-11: Confirm the correct usage of ScrollRefElement
Ensure that the ScrollRefElement type imported from 'app/core/components/NativeScrollbar' is correctly used in the component's state and methods.
501-504: Ensure isEditable prop respects user permissions
Double-check that isEditable correctly reflects both the isFNDashboardEditable flag and the user's edit permissions (dashboard.meta.canEdit).
525-527: Confirm feature toggle and conditions for AddWidgetModal
Ensure that AddWidgetModal is rendered only when intended by verifying the conditions and feature toggles.
9-9: Verify necessity of imported components from @grafana/ui
Ensure that all imported components (Themeable2, withTheme2, ToolbarButtonRow, ToolbarButton, ModalsController) are used in the file. Remove any unused imports to improve code clarity.
Run the following script to identify unused imports:
✅ Verification successful
Let me generate additional verification to understand the actual usage context of these imports, as just counting occurrences might include the import statement itself.
This will help us see if these components are actually being used in the code beyond their import declarations.
All imported components from @grafana/ui are actively used in the code
Based on the verification results, all components are being used appropriately:
Themeable2is used in the Props type definitionwithTheme2is used to wrap the UnthemedDashboardPage componentToolbarButtonRowis used in multiple places for layoutToolbarButtonis used within the save dashboard functionalityModalsControlleris used to handle the save dashboard modal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for unused imports from @grafana/ui
imports=("Themeable2" "withTheme2" "ToolbarButtonRow" "ToolbarButton" "ModalsController")
file="public/app/features/dashboard/containers/DashboardPage.tsx"
for component in "${imports[@]}"; do
count=$(rg --glob "$file" -c "\b$component\b")
echo "$component: $count occurrences in the file"
done
Length of output: 2197
Script:
#!/bin/bash
# Get more context around the usage of these components
rg --glob "public/app/features/dashboard/containers/DashboardPage.tsx" -B 1 -A 1 "\b(Themeable2|withTheme2|ToolbarButtonRow|ToolbarButton|ModalsController)\b"
Length of output: 3120
Line range hint 415-421: Verify the condition for rendering EntityNotFound
Ensure that the condition dashboard.meta.dashboardNotFound && !FNDashboard correctly handles scenarios where a dashboard is not found, especially with the presence of FNDashboard.
Run the following script to check the usage of dashboard.meta.dashboardNotFound:
✅ Verification successful
Let me gather more context about the FNDashboard variable and its usage.
Let me search for more context about the FNDashboard variable declaration and its usage.
Let me search for the FNDashboard type definition and its usage in the app/fn-app/types file.
Let me check the FNDashboard type definition in the types file.
Based on the gathered context, I can now provide a final response about the dashboard not found condition.
The condition dashboard.meta.dashboardNotFound && !FNDashboard is correctly implemented
The condition appropriately handles the "not found" scenario by:
- Checking if the dashboard is not found via
dashboard.meta.dashboardNotFound - Only showing the "not found" page when it's not a Function Dashboard (
!FNDashboard)
This is correct because:
- For regular dashboards, it shows the EntityNotFound component when a dashboard isn't found
- For Function Dashboards (FNDashboard), which have special handling and different lifecycle, it skips showing the "not found" error as these are managed differently through the FNDashboard props interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of dashboard.meta.dashboardNotFound
rg "dashboard\.meta\.dashboardNotFound" public/app/
Length of output: 279
Script:
#!/bin/bash
# Search for FNDashboard declarations and usages
ast-grep --pattern 'const FNDashboard = $_'
# Also search for any other references to FNDashboard
rg "FNDashboard" -A 2 public/app/
Length of output: 132252
Script:
#!/bin/bash
# Search for FNDashboard in the specific file and its imports
rg -A 5 "FNDashboard" public/app/features/dashboard/containers/DashboardPage.tsx
Length of output: 5377
Script:
#!/bin/bash
# Search for FNDashboard type definition and related imports
rg -g "*.ts" -g "*.tsx" "FNDashboardProps" public/app/
Length of output: 2917
Script:
#!/bin/bash
# Get the content of the types file to understand FNDashboard interface
cat public/app/fn-app/types.ts
Length of output: 1025
| @@ -58,7 +58,7 @@ export class DashboardPanelUnconnected extends PureComponent<Props> { | |||
| } | |||
| } | |||
|
|
|||
| onInstanceStateChange = (value: any) => { | |||
| onInstanceStateChange = (value: unknown) => { | |||
There was a problem hiding this comment.
💡 Codebase verification
Type change from any to unknown needs additional updates
The type change in DashboardPanel.tsx is part of a chain of components that handle panel state, but there are inconsistencies in the type definitions across the codebase:
packages/grafana-ui/src/components/PanelChrome/PanelContext.tsstill usesanytypepublic/app/features/dashboard/dashgrid/PanelStateWrapper.tsxalready usesunknown- Several panel implementations (geomap, canvas, debug) are using this callback but rely on the context definition with
any
To maintain type safety consistently, the type in PanelContext.ts should also be updated from any to unknown.
🔗 Analysis chain
Good improvement in type safety!
The change from any to unknown is a positive step towards better type safety. This enforces explicit type checking before using the value, reducing potential runtime errors.
Let's verify the impact on consuming components:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components that pass values to onInstanceStateChange
# to ensure they handle the unknown type correctly
# Search for direct prop passing
rg -A 3 "onInstanceStateChange={"
# Search for component implementations that might be affected
ast-grep --pattern 'onInstanceStateChange($value) {
$$$
}'
Length of output: 208
Script:
#!/bin/bash
# Search for direct prop passing with escaped curly brace
rg -A 3 "onInstanceStateChange=\{"
# Search for usage of onInstanceStateChange using ast-grep with proper syntax
ast-grep --pattern 'onInstanceStateChange = ($value) => {
$$$
}'
# Search for any references to onInstanceStateChange
rg "onInstanceStateChange"
Length of output: 2433
| wrapper: css({ | ||
| label: 'page-wrapper', | ||
| display: 'flex', | ||
| flex: '1 1 0', | ||
| flexDirection: 'column', | ||
| position: 'relative', | ||
| }), |
There was a problem hiding this comment.
💡 Codebase verification
Based on the evidence gathered, I can now provide a final response:
Removal of bodyScrolling toggle requires careful consideration
The bodyScrolling feature toggle is extensively used across multiple components in the codebase for controlling scroll behavior and layout calculations. Removing it from the Page component's wrapper styles could lead to inconsistencies since:
- It's actively used in AppChrome, NativeScrollbar, and other core components
- It affects global styling through GlobalStyles/page.ts and elements.ts
- It's integrated with layout calculations in multiple UI components
Please:
- Keep the feature toggle condition in the wrapper styles to maintain consistency
- If the goal is to remove this toggle, it should be part of a broader refactoring effort across all affected components
🔗 Analysis chain
Verify removal of bodyScrolling feature toggle
The style implementation has been simplified by removing the conditional logic based on config.featureToggles.bodyScrolling. Please confirm if this standardization of scrolling behavior is intentional and doesn't impact any existing functionality.
The search results show that bodyScrolling feature toggle is used extensively throughout the codebase, affecting scrolling behavior, layout calculations, and styling in multiple components. Let's verify if there are any recent changes or migrations related to this feature toggle.
Let's check if there are any recent changes specifically related to the bodyScrolling feature toggle in the Page component or related files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of bodyScrolling feature toggle
rg "featureToggles.bodyScrolling" -A 3
Length of output: 7304
Script:
#!/bin/bash
# Check recent commits mentioning bodyScrolling
git log --oneline -n 10 | grep -i "bodyscroll"
# Check if there are any migration or cleanup files related to this
fd -e ts -e tsx -e md | rg -i "migration|cleanup|scroll"
Length of output: 4827
Script:
#!/bin/bash
# Check the git history of the Page component and related files
git log -p -- public/app/core/components/Page/Page.tsx | grep -A 5 "bodyScrolling"
# Check for any feature toggle documentation
fd -e md | xargs rg -i "bodyscrolling"
Length of output: 38
| @@ -21,6 +21,7 @@ export interface LocationService { | |||
| getHistory: () => H.History; | |||
| getSearch: () => URLSearchParams; | |||
| getSearchObject: () => UrlQueryMap; | |||
| fnPathnameChange: (path: string, queryParams: any) => void; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Method signature needs improvements
The new method signature has several issues that should be addressed:
- Use a more specific type than
anyforqueryParams(e.g.,UrlQueryMapwhich is already imported) - Consider following the existing method naming patterns (e.g.,
updatePathname) - Add a return type annotation (likely
void) - Add JSDoc documentation like other interface methods
- fnPathnameChange: (path: string, queryParams: any) => void;
+ /**
+ * Updates the current pathname and query parameters
+ * @param path - The new pathname
+ * @param queryParams - The new query parameters
+ */
+ updatePathname: (path: string, queryParams: UrlQueryMap) => void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fnPathnameChange: (path: string, queryParams: any) => void; | |
| /** | |
| * Updates the current pathname and query parameters | |
| * @param path - The new pathname | |
| * @param queryParams - The new query parameters | |
| */ | |
| updatePathname: (path: string, queryParams: UrlQueryMap) => void; |
| <Router history={locationService.getHistory()}> | ||
| <LocationServiceProvider service={locationService}> | ||
| <CompatRouter> | ||
| <ModalsContextProvider> | ||
| <GlobalStyles /> | ||
| {children} | ||
| </ModalsContextProvider> | ||
| </CompatRouter> | ||
| </LocationServiceProvider> | ||
| </Router> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the usage of nested routers to avoid potential conflicts
The component hierarchy includes both <BrowserRouter> and a nested <Router> using locationService.getHistory(). Ensure that nesting multiple routers does not introduce unexpected behavior or routing conflicts. If not necessary, consider simplifying the routing structure.
| panelPos.x = (count % 2) * GRID_COLUMN_COUNT; | ||
| panelPos.y = Math.floor(count / 2); | ||
| layout.push(panelPos); | ||
| count++; | ||
| } |
There was a problem hiding this comment.
Fix grid positioning error when applying panel filters
In the buildLayout method, the panelPos.x is set to either 0 or GRID_COLUMN_COUNT, which may exceed the grid boundaries since x should be between 0 and GRID_COLUMN_COUNT - 1. This could cause panels to render outside the visible grid area.
Adjust the positioning logic to divide the grid into two columns within the grid boundaries:
if (panelFilter.test(panel.title)) {
panelPos.isResizable = false;
panelPos.isDraggable = false;
- panelPos.x = (count % 2) * GRID_COLUMN_COUNT;
+ const columnWidth = Math.floor(GRID_COLUMN_COUNT / 2);
+ panelPos.w = columnWidth;
+ panelPos.x = (count % 2) * columnWidth;
panelPos.y = Math.floor(count / 2);
layout.push(panelPos);
count++;
}Committable suggestion skipped: line range outside the PR's diff.
| private resizeObserver?: ResizeObserver; | ||
| private rootEl: HTMLDivElement | null = null; | ||
| onMeasureRef = (rootEl: HTMLDivElement | null) => { | ||
| if (!rootEl) { | ||
| if (this.rootEl && this.resizeObserver) { | ||
| this.resizeObserver.unobserve(this.rootEl); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| this.rootEl = rootEl; | ||
| this.resizeObserver = new ResizeObserver((entries) => { | ||
| entries.forEach((entry) => { | ||
| this.setState({ width: entry.contentRect.width }); | ||
| }); | ||
| }); | ||
|
|
||
| this.resizeObserver.observe(rootEl); | ||
| }; |
There was a problem hiding this comment.
Properly disconnect ResizeObserver to prevent memory leaks
The ResizeObserver created in onMeasureRef is not disconnected when the component unmounts, which could lead to memory leaks. Ensure to disconnect the observer in componentWillUnmount.
Apply this diff to properly clean up the ResizeObserver:
componentWillUnmount() {
this.eventSubs.unsubscribe();
+ if (this.resizeObserver && this.rootEl) {
+ this.resizeObserver.unobserve(this.rootEl);
+ this.resizeObserver = undefined;
+ this.rootEl = null;
+ }
}
onMeasureRef = (rootEl: HTMLDivElement | null) => {
if (!rootEl) {
if (this.rootEl && this.resizeObserver) {
this.resizeObserver.unobserve(this.rootEl);
+ this.resizeObserver = undefined;
+ this.rootEl = null;
}
return;
}
// ... rest of the code ...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private resizeObserver?: ResizeObserver; | |
| private rootEl: HTMLDivElement | null = null; | |
| onMeasureRef = (rootEl: HTMLDivElement | null) => { | |
| if (!rootEl) { | |
| if (this.rootEl && this.resizeObserver) { | |
| this.resizeObserver.unobserve(this.rootEl); | |
| } | |
| return; | |
| } | |
| this.rootEl = rootEl; | |
| this.resizeObserver = new ResizeObserver((entries) => { | |
| entries.forEach((entry) => { | |
| this.setState({ width: entry.contentRect.width }); | |
| }); | |
| }); | |
| this.resizeObserver.observe(rootEl); | |
| }; | |
| private resizeObserver?: ResizeObserver; | |
| private rootEl: HTMLDivElement | null = null; | |
| onMeasureRef = (rootEl: HTMLDivElement | null) => { | |
| if (!rootEl) { | |
| if (this.rootEl && this.resizeObserver) { | |
| this.resizeObserver.unobserve(this.rootEl); | |
| this.resizeObserver = undefined; | |
| this.rootEl = null; | |
| } | |
| return; | |
| } | |
| this.rootEl = rootEl; | |
| this.resizeObserver = new ResizeObserver((entries) => { | |
| entries.forEach((entry) => { | |
| this.setState({ width: entry.contentRect.width }); | |
| }); | |
| }); | |
| this.resizeObserver.observe(rootEl); | |
| }; | |
| componentWillUnmount() { | |
| this.eventSubs.unsubscribe(); | |
| if (this.resizeObserver && this.rootEl) { | |
| this.resizeObserver.unobserve(this.rootEl); | |
| this.resizeObserver = undefined; | |
| this.rootEl = null; | |
| } | |
| } |
| if (!FNDashboard || isCustomDashboard) { | ||
| this.forceRouteReloadCounter = (this.props.history.location?.state as any)?.routeReloadCounter || 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any type assertions
Using (this.props.history.location?.state as any) bypasses TypeScript's type checking. Consider defining a proper type for location.state to enhance type safety.
Define an interface for the location state and update the code accordingly.
interface DashboardLocationState {
routeReloadCounter?: number;
}Then update the code:
-this.forceRouteReloadCounter = (this.props.history.location?.state as any)?.routeReloadCounter || 0;
+const locationState = this.props.history.location?.state as DashboardLocationState;
+this.forceRouteReloadCounter = locationState?.routeReloadCounter || 0;| setScrollRef = (scrollElement: ScrollRefElement): void => { | ||
| this.setState({ scrollElement }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for scrollElement in setScrollRef
Ensure that scrollElement is not null before updating the state to prevent potential runtime errors.
Apply this diff to add a null check:
setScrollRef = (scrollElement: ScrollRefElement): void => {
+ if (scrollElement) {
this.setState({ scrollElement });
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setScrollRef = (scrollElement: ScrollRefElement): void => { | |
| this.setState({ scrollElement }); | |
| }; | |
| setScrollRef = (scrollElement: ScrollRefElement): void => { | |
| if (scrollElement) { | |
| this.setState({ scrollElement }); | |
| } | |
| }; |
| console.log('Edit Panel: ', { editPanel, sectionNav, pageNav, isFNDashboardEditable }); | ||
| console.log('Dashboard settings: ', { editView: queryParams.editview, pageNav, sectionNav, isFNDashboardEditable }); | ||
| console.log('Add Widget: ', { | ||
| isFNDashboardEditable, | ||
| addWidget: queryParams.addWidget, | ||
| configToggle: config.featureToggles.vizAndWidgetSplit, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log statements before production
Debugging statements using console.log should be removed to prevent unwanted logs in production.
Apply this diff to remove the console logs:
- console.log('Edit Panel: ', { editPanel, sectionNav, pageNav, isFNDashboardEditable });
- console.log('Dashboard settings: ', { editView: queryParams.editview, pageNav, sectionNav, isFNDashboardEditable });
- console.log('Add Widget: ', {
- isFNDashboardEditable,
- addWidget: queryParams.addWidget,
- configToggle: config.featureToggles.vizAndWidgetSplit,
- });| </> | ||
| )} | ||
| {FNTimeRange} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check when accessing DOM elements
Using document.getElementById(controlsContainer)! assumes the element exists. Add a null check to prevent potential runtime errors.
Apply this diff to include a null check:
-<Portal container={document.getElementById(controlsContainer)!}>
+const containerElement = document.getElementById(controlsContainer);
+if (containerElement) {
+ <Portal container={containerElement}>Committable suggestion skipped: line range outside the PR's diff.
No description provided.