feat(autofix): Better autoscroll on autofix drawer#114869
Conversation
Make sure to scroll to the bottom when each section is finished loading. Also make sure to cancel the auto scroll behaviour if the user scrolled upwards.
📊 Type Coverage Diff✅ No new type safety issues introduced. Coverage: 93.40% |
|
|
||
| return ( | ||
| <Flex direction="column" gap="lg"> | ||
| <Flex ref={containerRef} onScroll={onScrollHandler} direction="column" gap="lg"> |
There was a problem hiding this comment.
Bug: The onScroll handler is attached to a non-scrollable <Flex> element instead of the scrollable parent, so it will never be triggered.
Severity: MEDIUM
Suggested Fix
Move the onScroll={onScrollHandler} prop from the inner <Flex> element in content.tsx to the StyledDrawerBody component in body.tsx. This will attach the event listener to the element that actually scrolls, allowing the onScrollHandler to be correctly triggered.
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/autofix/v3/content.tsx#L70
Potential issue: The `onScroll` handler is attached to an inner `<Flex>` element which
is not scrollable. The actual scrollable container is the parent `StyledDrawerBody`.
Because `onScroll` events do not bubble in React 17+, the `onScrollHandler` will never
be called. This prevents the `canAutoScroll` state from ever being updated to `false`.
As a result, the feature that cancels auto-scrolling when a user scrolls up is
non-functional, and the user will always be force-scrolled to the bottom when the
autofix run completes.
Did we get this right? 👍 / 👎 to inform future reviews.
| import {defined} from 'sentry/utils'; | ||
|
|
||
| interface UseAutoScrollOptions { | ||
| key: any; |
There was a problem hiding this comment.
| key: any; | |
| key: unknown; |
ryan953
left a comment
There was a problem hiding this comment.
better with de-duplicated util 👍
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc3751b. Configure here.
| export function useAutoScroll({key}: UseAutoScrollOptions) { | ||
| const canAutoScroll = useRef(true); | ||
|
|
There was a problem hiding this comment.
Bug: The canAutoScroll ref is not reset on new autofix runs, permanently disabling auto-scrolling if the user scrolls up once.
Severity: MEDIUM
Suggested Fix
Reset the canAutoScroll ref to true when a new autofix run is initiated. This can be achieved by adding a dependency to the useEffect that triggers the scroll, or by exposing a reset function from the useAutoScroll hook that can be called when restarting the run.
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/utils/useAutoScroll.tsx#L9-L11
Potential issue: The `canAutoScroll` ref in the `useAutoScroll` hook is initialized to
`true` but is not reset when a new autofix run begins. Since the `SeerDrawer` component
remains mounted across runs, if a user scrolls up during one run,
`canAutoScroll.current` is set to `false`. When a new run is started, this `false` value
persists, causing the `useEffect` responsible for scrolling to exit early. This
permanently disables the auto-scroll functionality for all subsequent runs until the
component is fully unmounted, defeating the feature's purpose.

Make sure to scroll to the bottom when each section is finished loading. Also make sure to cancel the auto scroll behaviour if the user scrolled upwards.