ref(integrations): Refactor the useAddIntegration hook#112766
ref(integrations): Refactor the useAddIntegration hook#112766
Conversation
…are passed to the callbakc
github-actions
bot
added
the
Scope: Frontend
ryan953
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (message.source !== dialogRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| const {success, data} = message.data; | ||
| dialogRef.current = null; | ||
|
|
There was a problem hiding this comment.
Bug: Calling startFlow multiple times in succession causes a race condition where the second integration flow silently fails because its message callback is prematurely unsubscribed by the first flow.
Severity: HIGH
Suggested Fix
The useLegacyDialogStrategy should be updated to handle multiple concurrent flows. Instead of overwriting unsubscribeRef.current, it should clean up the previous subscription before creating a new one. A better approach would be to return the unsubscribe function from subscribe and call it in a cleanup effect or when the callback logic completes, ensuring each callback is responsible for its own unsubscription.
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/integrations/useAddIntegration.tsx#L120-L126
Potential issue: If the `startFlow` callback is invoked multiple times before the first
dialog completes, the reference to the first unsubscribe function in `unsubscribeRef` is
overwritten. When the first dialog sends its response message, its callback executes and
incorrectly calls the unsubscribe function for the *second* callback. This prematurely
removes the second callback. Consequently, when the second dialog sends its response,
its message is silently dropped, causing the second integration flow to fail without any
error notification. This is likely to occur if a user double-clicks the button or clicks
different integration provider buttons in quick succession.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Previous subscription not cleaned up before re-subscribing
- Added unsubscribeRef.current?.() call before assigning new subscription to prevent subscription leaks and accidental unsubscription of the second dialog.
- ✅ Fixed: Hook uses context org instead of selected org
- Modified useAddIntegration to accept an optional organizationOverride parameter and passed the selected organization from IntegrationOrganizationLink to ensure correct feature flag evaluation and analytics.
- ✅ Fixed: Popup window no longer closed on component unmount
- Added dialogRef to store the popup window reference and close it in the cleanup effect, preventing orphaned popup windows when the component unmounts.
Or push these changes by commenting:
@cursor push b36a5dc6f8
Preview (b36a5dc6f8)
diff --git a/static/app/utils/integrations/useAddIntegration.tsx b/static/app/utils/integrations/useAddIntegration.tsx
--- a/static/app/utils/integrations/useAddIntegration.tsx
+++ b/static/app/utils/integrations/useAddIntegration.tsx
@@ -72,10 +72,10 @@
// Legacy dialog strategy (uses PostMessageContext)
// ---------------------------------------------------------------------------
-function useLegacyDialogStrategy() {
- const organization = useOrganization();
+function useLegacyDialogStrategy(organization: Organization) {
const subscribe = usePostMessageCallback();
- const unsubscribeRef = useRef<() => void | null>(null);
+ const unsubscribeRef = useRef<(() => void) | null>(null);
+ const dialogRef = useRef<Window | null>(null);
const startFlow = useCallback(
({
@@ -110,13 +110,15 @@
const installUrl = `${url}?${qs.stringify(query)}`;
const opts = `scrollbars=yes,width=${width},height=${height},top=${top},left=${left}`;
- let dialog = window.open(installUrl, name, opts);
+ const dialog = window.open(installUrl, name, opts);
if (!dialog) {
// Popup was blocked?
return;
}
dialog?.focus();
+ dialogRef.current = dialog;
+ unsubscribeRef.current?.();
unsubscribeRef.current = subscribe((message: MessageEvent) => {
const validOrigins = [
ConfigStore.get('links').sentryUrl,
@@ -131,7 +133,7 @@
}
const {success, data} = message.data;
- dialog = null;
+ dialogRef.current = null;
unsubscribeRef.current?.();
unsubscribeRef.current = null;
@@ -158,7 +160,7 @@
useEffect(() => {
return () => {
- // Unsubscribe if we unmount after having started the flow
+ dialogRef.current?.close();
unsubscribeRef.current?.();
unsubscribeRef.current = null;
};
@@ -170,9 +172,10 @@
// Public hook: selects between pipeline modal and legacy dialog
// ---------------------------------------------------------------------------
-export function useAddIntegration() {
- const organization = useOrganization();
- const {startFlow: legacyStartFlow} = useLegacyDialogStrategy();
+export function useAddIntegration(organizationOverride?: Organization) {
+ const contextOrganization = useOrganization();
+ const organization = organizationOverride ?? contextOrganization;
+ const {startFlow: legacyStartFlow} = useLegacyDialogStrategy(organization);
const startFlow = useCallback(
(params: AddIntegrationParams) => {
diff --git a/static/app/views/integrationOrganizationLink/index.tsx b/static/app/views/integrationOrganizationLink/index.tsx
--- a/static/app/views/integrationOrganizationLink/index.tsx
+++ b/static/app/views/integrationOrganizationLink/index.tsx
@@ -241,6 +241,7 @@
disabled={disabled}
disabledReason={disabledReason}
finishInstallation={finishInstallation}
+ organization={organization}
/>
</PostMessageProvider>
)}
@@ -412,6 +413,7 @@
disabled,
disabledReason,
finishInstallation,
+ organization,
}: {
disabled: boolean;
disabledReason: React.ReactNode;
@@ -420,8 +422,9 @@
onInstall: (data: Integration) => void;
provider: IntegrationProvider;
installationId?: string;
+ organization?: Organization | null;
}) {
- const {startFlow} = useAddIntegration();
+ const {startFlow} = useAddIntegration(organization ?? undefined);
return (
<ButtonWrapper>This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2e8c68e. Configure here.
| } | ||
| dialog?.focus(); | ||
|
|
||
| unsubscribeRef.current = subscribe((message: MessageEvent) => { |
There was a problem hiding this comment.
Previous subscription not cleaned up before re-subscribing
Medium Severity
When startFlow is called a second time, unsubscribeRef.current is overwritten with the new unsubscribe function without first cleaning up the previous subscription. This leaks the old subscription. Worse, if the first dialog completes, its callback calls unsubscribeRef.current?.() on line 135 — which now points to the second subscription's unsubscribe — accidentally removing the listener for the second dialog. The second dialog's completion message would then be silently dropped.
The old code avoided this by using a single useEffect-based listener that checked dialogRef.current, so only the latest dialog's messages were processed. Adding unsubscribeRef.current?.() before the assignment on line 120 would restore correct behavior.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2e8c68e. Configure here.
| installationId?: string; | ||
| }) { | ||
| const {startFlow} = useAddIntegration({provider, organization, onInstall}); | ||
| const {startFlow} = useAddIntegration(); |
There was a problem hiding this comment.
Hook uses context org instead of selected org
Medium Severity
In IntegrationOrganizationLink, the user selects an organization from a dropdown, and the fetched org was previously passed explicitly to useAddIntegration. Now, useAddIntegration calls useOrganization() internally, which returns the org from OrganizationContext (based on lastOrganizationSlug), not the user-selected org. This can cause incorrect feature flag evaluation in getApiPipelineProvider and wrong org attribution in analytics events.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2e8c68e. Configure here.
| unsubscribeRef.current?.(); | ||
| unsubscribeRef.current = null; | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Popup window no longer closed on component unmount
Medium Severity
The old code stored the popup window in a dialogRef and called dialogRef.current?.close() on unmount, ensuring orphaned popups were cleaned up when the user navigated away. The refactored code stores dialog as a local let variable inside the startFlow closure, so the unmount cleanup effect has no way to reference or close the popup window. This leaves orphaned popup windows open whose completion results would be silently dropped since the message subscription was already removed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2e8c68e. Configure here.
| }; | ||
| }, [provider.key, provider.name, organization]); | ||
| function useLegacyDialogStrategy() { | ||
| const organization = useOrganization(); |
There was a problem hiding this comment.
I'm actually not 100% positive we can always use useOrganization here. Since this can be triggered outside of org context, specifically here
There was a problem hiding this comment.
I'm not against refactoring these out, but I wonder how many other things could use them?
We could immediately refactor this hook to use both of these, so here's at least one usage. But, I am planning to also remove all of this legacy stuff from useAddIntegration once everything is switched over to an API pipeline flow (tbf which might not happen right away)
2 participants
The problem is that previously the
useAddIntegration()hook had all it's props passed into the hook itself, and therefore we were only able to get things setup for one (or a hard-coded list) provider within a given component.ie:
function Foo() { const {startFlow} = useAddIntegration({provider}); return <a onClick={startFlow()} />;I want to be able to run that
startFlow()function from within a dropdown, but i can't do that from the dropdown. The list of things in the dropdown is dynamic, so i'm not going to hard-codestartFlow1 = useAddIntegration(); startFlow2 = useAddIntegration()either.See #112751
Therefore this PR refactors the hook making two big changes:
startFlow(), not as props to the hook itself. The hook now is just a helper that provides auseCallbackfunction which triggers the modal/window.open call, and listenes for window messages. All the refs inside that useCallback go away, there are closures instead.