fix(events-screenshot): Stabilize AttachmentComponent reference#120036
fix(events-screenshot): Stabilize AttachmentComponent reference#120036sentry[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 61841ab. Configure here.
| () => getImageAttachmentRenderer(screenshot) ?? ImageViewer, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [screenshot.mimetype] | ||
| ); |
There was a problem hiding this comment.
Conditional useMemo breaks hooks
High Severity
The new useMemo for AttachmentComponent runs only after the if (!hasRole) return null guard, so renders without role skip that hook while renders with role call it. If hasRole changes on the same mount, React’s hook order no longer matches and the tree can crash.
Reviewed by Cursor Bugbot for commit 61841ab. Configure here.
| const AttachmentComponent = useMemo( | ||
| () => getImageAttachmentRenderer(screenshot) ?? ImageViewer, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [screenshot.mimetype] | ||
| ); | ||
| const downloadUrl = `/api/0/projects/${organization.slug}/${projectSlug}/events/${eventId}/attachments/${screenshot.id}/`; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Bug: The useMemo hook is called conditionally after an early return. This violates the Rules of Hooks and can cause a crash if the hasRole condition changes between renders.
Severity: LOW
Suggested Fix
Move the useMemo hook to be called unconditionally before the early return at line 61. Alternatively, restructure the component to avoid placing hooks after conditional guards.
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/eventTagsAndScreenshot/screenshot/index.tsx#L75-L82
Potential issue: The `useMemo` hook at line 75 is called after a conditional early
return at line 61. The condition depends on the `hasRole` variable, which is derived
from the `useRole` hook. If the value of `hasRole` changes between renders for the same
component instance (e.g., from `true` to `false` due to an asynchronous permission
update), the number of hooks executed will differ. This violates the Rules of Hooks and
will cause React to throw an error, crashing the component. While the trigger condition
is rare in practice, the code structure is inherently unstable.
Did we get this right? 👍 / 👎 to inform future reviews.


The
AttachmentComponentwas being derived inside the render function of these components, leading to a new component identity on every render. This causes React to unmount and remount the component subtree, resetting its state and preventing React Compiler optimizations.To fix this, the assignment of
AttachmentComponentin both files has been wrapped withuseMemo, keyed by the attachment'smimetype. This ensures thatAttachmentComponentmaintains a stable reference across renders, resolving the static component definition issue.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Fixes CODING-CONVENTIONS-34K
Comment
@sentry <feedback>on this PR to have Autofix iterate on the changes.