-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(dashboards): temporary filters broken when changing values #104482
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
fix(dashboards): temporary filters broken when changing values #104482
Conversation
Ahmed-Labs
left a comment
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.
I tested global filters in non-prebuilt dashboards and it seems that unsaved changes don't get picked up (changes get pushed to url params but no save button pops up)
👍 Thanks for testing! We should probs add some test cases here to prevent regressions |
| const prebuiltDashboardFilters: GlobalFilter[] = prebuiltDashboardId | ||
| ? (PREBUILT_DASHBOARDS[prebuiltDashboardId].filters.globalFilter ?? []) | ||
| : []; |
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.
Bug: Missing validation for prebuiltDashboardId as a key in PREBUILT_DASHBOARDS can lead to runtime crash when accessing .filters.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The code at static/app/views/dashboards/filtersBar.tsx:73-75 accesses PREBUILT_DASHBOARDS[prebuiltDashboardId] without validating if prebuiltDashboardId is a valid key within PREBUILT_DASHBOARDS. This can lead to PREBUILT_DASHBOARDS[prebuiltDashboardId] evaluating to undefined if prebuiltDashboardId exists but is not a recognized key. Subsequently, attempting to access .filters on undefined will cause a runtime crash. This can occur due to incomplete deployments, API mismatches, or server/client version discrepancies.
💡 Suggested Fix
Before accessing PREBUILT_DASHBOARDS[prebuiltDashboardId], add a check to ensure prebuiltDashboardId in PREBUILT_DASHBOARDS to prevent accessing properties on undefined.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/views/dashboards/filtersBar.tsx#L73-L75
Potential issue: The code at `static/app/views/dashboards/filtersBar.tsx:73-75` accesses
`PREBUILT_DASHBOARDS[prebuiltDashboardId]` without validating if `prebuiltDashboardId`
is a valid key within `PREBUILT_DASHBOARDS`. This can lead to
`PREBUILT_DASHBOARDS[prebuiltDashboardId]` evaluating to `undefined` if
`prebuiltDashboardId` exists but is not a recognized key. Subsequently, attempting to
access `.filters` on `undefined` will cause a runtime crash. This can occur due to
incomplete deployments, API mismatches, or server/client version discrepancies.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6235892
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.
typing wouldn't allow this
Ahmed-Labs
left a comment
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.
Lgtm!
temporaryFiltersquery param and addisTemporaryproperty toGlobalFilterswhich is easier to manage then having two separate query params