fix(ourlogs): Add drawer QP and fix scroll #103979
Conversation
This fixes draw scrollTo for log exceptions to not infinitely scroll, and fixes both metrics and logs to have a query param to have it open for linking.
github-actions
bot
added
the
Scope: Frontend
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| }, | ||
| onClose: () => { | ||
| navigate( | ||
| { | ||
| ...location, | ||
| query: { | ||
| ...location.query, | ||
| [LOGS_DRAWER_QUERY_PARAM]: undefined, | ||
| expandedLogId: undefined, | ||
| }, | ||
| }, | ||
| {replace: true} | ||
| ); | ||
| }, | ||
| } | ||
| ); | ||
| }, | ||
| [group, event, project, openDrawer, organization, traceId] |
There was a problem hiding this comment.
Bug: Race condition in onOpenLogsDrawer can cause expandedLogId to persist in URL due to stale location.query after drawer close.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When onOpenLogsDrawer is called after onClose has been triggered but before the location object has fully updated to reflect expandedLogId: undefined, the ...location.query spread will include the stale expandedLogId from the previous navigation. This occurs if a user closes the drawer and immediately clicks "View more" again. The drawer then opens with scrollToDisabled: true even though no specific log was selected, breaking the intended scroll behavior for the pseudo event.
💡 Suggested Fix
Modify onOpenLogsDrawer to explicitly set expandedLogId: undefined in the new query object when no expandedLogId is passed, instead of relying on ...location.query to implicitly remove it. This ensures the parameter is cleared regardless of location object staleness.
🤖 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/components/events/ourlogs/ourlogsSection.tsx#L104-L108
Potential issue: When `onOpenLogsDrawer` is called after `onClose` has been triggered
but before the `location` object has fully updated to reflect `expandedLogId:
undefined`, the `...location.query` spread will include the stale `expandedLogId` from
the previous navigation. This occurs if a user closes the drawer and immediately clicks
"View more" again. The drawer then opens with `scrollToDisabled: true` even though no
specific log was selected, breaking the intended scroll behavior for the pseudo event.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3557864
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| [onOpenLogsDrawer] | ||
| ); | ||
| } | ||
| }, [location.query, traceId, group, event, project, openDrawer, navigate, location]); |
There was a problem hiding this comment.
Bug: Drawer opens even when feature flag is disabled
The useEffect that opens the logs drawer checks shouldOpenDrawer && traceId but doesn't check the ourlogs-enabled feature flag. The feature flag check at line 158 happens after the effect is defined, but React hooks run regardless of what the component returns. This means if someone shares a URL with logsDrawer=true to an organization that doesn't have ourlogs-enabled, the drawer will still open, bypassing the feature gate. The effect's condition needs to include feature (e.g., shouldOpenDrawer && traceId && feature).
Summary
This fixes draw scrollTo for log exceptions to not infinitely scroll, and fixes both metrics and logs to have a query param to have it open for linking.