ref(discover): Remove usages of deprecatedRouteProps from Discover views
#104923
Conversation
github-actions
bot
added
the
Scope: Frontend
| } | ||
|
|
||
| function HomepageContainer(props: Props) { | ||
| export default function HomepageContainer() { |
There was a problem hiding this comment.
I noticed that our legacy redirect scheme to event details (from the event IDs in the Discover table) fails in dev-ui and devserver, so that's the one thing I wasn't able to test manually. On the preview deployment for the PR, the redirect "works", but redirects me to prod. Seems this happens for all of our preview deployments. Just wanted to make sure this is expected?
I think to fix this, we would want to migrate ProjectEventRedirect off of XHR and instead:
- Fetch the issue ID for the given event from the API
- Construct the URL for event details manually
I can make a follow-up for this if that sounds good.
| export default function ResultsContainer( | ||
| props: Omit<Props, 'api' | 'organization' | 'selection' | 'loading' | 'setSavedQuery'> | ||
| ) { | ||
| export default function ResultsContainer() { |
There was a problem hiding this comment.
Keeping wrapper around class component
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| return ( | ||
| <PageFiltersContainer | ||
| disablePersistence={ | ||
| organization.features.includes('discover-query') && | ||
| !!(props.savedQuery || props.location.query.id) | ||
| organization.features.includes('discover-query') && !!location.query.id | ||
| } | ||
| skipLoadLastUsed={!!props.savedQuery} | ||
| skipLoadLastUsed={false} | ||
| // The Discover Results component will manage URL params, including page filters state | ||
| // This avoids an unnecessary re-render when forcing a project filter for team plan users | ||
| skipInitializeUrlParams |
There was a problem hiding this comment.
Bug: Setting skipLoadLastUsed to false causes last-used filters from local storage to override a saved query's filters, as local storage is loaded before the query is fetched.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When a user opens a saved query, the PageFiltersContainer is initialized with skipLoadLastUsed={false}. This unconditionally loads the user's last-used page filters from local storage. This loading occurs before the SavedQueryAPI fetches the saved query's data. As a result, when the EventView is created, it uses the filters from local storage instead of the filters defined in the saved query, effectively overriding the saved query's intended project, environment, and date range settings.
💡 Suggested Fix
Revert the skipLoadLastUsed prop to its previous behavior. It should be set to true when a saved query is being loaded to prevent filters from local storage from being applied. The logic should be similar to the previous implementation: skipLoadLastUsed={!!props.savedQuery}.
🤖 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/discover/results.tsx#L1035-L1043
Potential issue: When a user opens a saved query, the `PageFiltersContainer` is
initialized with `skipLoadLastUsed={false}`. This unconditionally loads the user's
last-used page filters from local storage. This loading occurs before the
`SavedQueryAPI` fetches the saved query's data. As a result, when the `EventView` is
created, it uses the filters from local storage instead of the filters defined in the
saved query, effectively overriding the saved query's intended project, environment, and
date range settings.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7607005
Migrates Discover views off of
deprecatedRouteProps.https://www.notion.so/sentry/Frontend-TSC-Project-Remove-all-uses-of-deprecatedRouteProps-true-26a8b10e4b5d8015a6a2dd14f9d41dd7