Skip to content

ref(ui): Replace EventWaiter class component with useEventWaiter hook#110376

Merged
billyvg merged 12 commits into
masterfrom
billy/ref-react-change-event-waiter-hook
Mar 12, 2026
Merged

ref(ui): Replace EventWaiter class component with useEventWaiter hook#110376
billyvg merged 12 commits into
masterfrom
billy/ref-react-change-event-waiter-hook

Conversation

@billyvg
Copy link
Copy Markdown
Member

@billyvg billyvg commented Mar 10, 2026

Replace the legacy EventWaiter render-prop class component with a modern useEventWaiter hook that uses useApiQuery with refetchInterval for polling.

What changed:

  • New useEventWaiter hook replaces the class component, using React Query's refetchInterval instead of manual setInterval
  • Eliminates the withApi HOC dependency — the hook uses useApiQuery internally
  • Migrates all 4 consumers (updatedEmptyState, performanceOnboarding/sidebar, profiling/onboarding, performance/onboarding) from the render-prop pattern to direct hook usage
  • Deletes the old eventWaiter.tsx class component and its tests, adds new hook-specific tests

Why:
The codebase has fully adopted React Query for API polling (see usePollReplayRecord, useAiSpanWaiter). EventWaiter was one of the last holdouts using manual setInterval polling, class components, and the withApi HOC — all patterns the codebase has moved away from. React Query's refetchInterval handles the polling lifecycle (start/stop/cleanup) automatically with proper cleanup on unmount.

Notable detail:
In performance/onboarding.tsx, the hook had to be placed above early returns to comply with React's Rules of Hooks. The disabled prop handles the conditional logic that was previously implicit (the <EventWaiter> JSX element was only rendered in the non-early-return path).

Comment thread static/app/utils/useEventWaiter.tsx Outdated
try {
captureError.cause = err;
} catch {
// some browsers don't let you set a `cause`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the browsers we support should https://caniuse.com/?search=Error.cause

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen exceptions thrown when trying to write to the property tho. But actually I forgot about the options.cause in Error constructor, will change to use that

Copy link
Copy Markdown
Member

@scttcper scttcper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

Comment on lines -128 to -139
{isLastStep && (
<EventWaiter
api={api}
organization={organization}
project={project}
eventType="profile"
>
{({firstIssue}) => (
<WaitingIndicator project={project} hasProfile={!!firstIssue} />
)}
</EventWaiter>
)}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit odd now, we may want to pull the hook up a component instead of calling it for every step (where only a single/last step needs it).

@billyvg billyvg changed the base branch from billy/feat-add-exception-to-eventwaiter to master March 11, 2026 17:34
@billyvg billyvg marked this pull request as ready for review March 11, 2026 17:41
@billyvg billyvg requested review from a team as code owners March 11, 2026 17:41
Comment thread static/app/utils/useEventWaiter.tsx
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment thread static/app/utils/useEventWaiter.tsx Outdated
Comment thread static/app/utils/useEventWaiter.tsx
billyvg added 12 commits March 11, 2026 16:59
Otherwise we do not have a very useful stacktrace since we are capturing and creating a new exception in the catch block
Replace the legacy EventWaiter render-prop class component with a modern
useEventWaiter hook that uses useApiQuery with refetchInterval for polling.
This eliminates manual setInterval management, the withApi HOC, and the
render-prop pattern in favor of React Query's declarative polling lifecycle.

Migrates all 4 consumers to use the hook directly.
Consumers can react to the hook's return value directly instead of
using a callback. This removes the onIssueReceived prop and replaces
`useState` + callback patterns with simple `!!firstIssue` derivation.
Replace the legacy EventWaiter render-prop class component with a modern
useEventWaiter hook that uses useApiQuery with refetchInterval for polling.
This eliminates manual setInterval management, the withApi HOC, and the
render-prop pattern in favor of React Query's declarative polling lifecycle.

Migrates all 4 consumers to use the hook directly.
…ter hook"

This reverts commit fd3fc4962cc703bc1cb324d6a83739e8f225e190.
Comment on lines +136 to +139
Sentry.captureException(
new Error(`Error polling for first ${eventType} event`, {cause: err})
);
}, [projectQuery.error, eventType]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we'll need a custom fingerprint here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of cause? I don't think cause should affect grouping

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making a new error in an effect doesn't always group super well, we might have fixed some of that though

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, because of the react stack trace? I'll keep an eye on it

@billyvg billyvg merged commit af4baa3 into master Mar 12, 2026
63 of 64 checks passed
@billyvg billyvg deleted the billy/ref-react-change-event-waiter-hook branch March 12, 2026 17:12
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants