Skip to content

Conversation

@everettbu
Copy link
Contributor

Test for commit f32fe9a

…4220)

* add data provider when switching from non data panel

* handle adding and cleaning up data provider in panel editor on panel switch

* add data provider check sin panel  editor tests
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR adds proper data provider management when switching between panel types in the PanelEditor. When switching from a data-supporting panel to a non-data panel (skipDataQuery: true), it now properly cleans up the $data provider. When switching the other direction, it creates a new data provider using the last used datasource or falls back to the default datasource.

Key changes:

  • Added cleanup logic to remove $data provider when switching to panels that skip data queries
  • Added initialization logic to create $data provider when switching to panels that support data queries
  • Enhanced test coverage to verify $data state is properly managed for both panel types
  • Uses getLastUsedDatasourceFromStorage() and config.defaultDatasource for intelligent datasource selection

Confidence score: 4/5

  • This PR is safe to merge with minor risk
  • The implementation properly handles data provider lifecycle management when switching panel types. The logic is straightforward and well-tested. Only concern is potential null reference in dashboard UID access which should be addressed.
  • Pay attention to PanelEditor.tsx line 212 for potential null reference handling

2 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

Comment on lines +212 to +214
let ds = getLastUsedDatasourceFromStorage(getDashboardSceneFor(this).state.uid!)?.datasourceUid;
if (!ds) {
ds = config.defaultDatasource;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Consider using optional chaining here as getDashboardSceneFor(this).state.uid might be undefined:

Suggested change
let ds = getLastUsedDatasourceFromStorage(getDashboardSceneFor(this).state.uid!)?.datasourceUid;
if (!ds) {
ds = config.defaultDatasource;
let ds = getLastUsedDatasourceFromStorage(getDashboardSceneFor(this).state.uid)?.datasourceUid;

@everettbu
Copy link
Contributor Author

Closing PR as part of cleanup

@everettbu everettbu closed this Sep 18, 2025
@everettbu everettbu deleted the post-32e997d branch September 18, 2025 18:56
@everettbu everettbu restored the post-32e997d branch September 18, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants