Skip to content

Commit ff45547

Browse files
committed
release(runway): cherry-pick fix: infinite auth requests in carousel hook cp-13.7.0 (#37334)
## **Description** This PR fixes a bug where the Authentication API was being called on every re-render of `useCarouselManagement` because of infinite `getUserProfileLineage` requests, creating scenarios where users were being rate limited and couldn't authenticate. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/37334?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: Fixed a bug where the Authentication API was called infinitely in useCarouselManagement ## **Related issues** Fixes: #35367 , [MUL-1220](https://consensyssoftware.atlassian.net/browse/MUL-1220), potentially [MUL-1190](https://consensyssoftware.atlassian.net/browse/MUL-1190) ## **Manual testing steps** 1. Complete onboarding 2. Open the network tab, filter it by typing "lineage" 3. Switch accounts and verify the calls are only happening here 4. Verify that there are no infinite calls to the Authentication API anymore ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Separates download eligibility into its own effect to avoid repeated lineage calls, gates Contentful fetch until ready, and adds a test ensuring lineage runs once per account. > > - **Hook (`ui/hooks/useCarouselManagement/useCarouselManagement.ts`)** > - Introduces `downloadEligible` and `downloadEligibilityReady` state and a dedicated effect to compute eligibility via `getUserProfileLineage`, with cancellation and error handling. > - Gates Contentful slide fetching on `downloadEligibilityReady`; uses `contentfulEnabled` derived flag and removes it from deps in favor of the derived value. > - Applies eligibility when including `downloadMobileApp` slides; preserves existing behavior for `fund` slide and slide ordering. > - Refines effect dependency arrays (e.g., replaces `remoteFeatureFlags` with `contentfulEnabled`). > - **Tests (`ui/hooks/useCarouselManagement/useCarouselManagement.test.ts`)** > - Adds test verifying lineage is called only once per account and not on slide changes. > - Updates mocks and helpers accordingly. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 0898e82. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> [MUL-1220]: https://consensyssoftware.atlassian.net/browse/MUL-1220?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
1 parent e4ca203 commit ff45547

File tree

2 files changed

+93
-19
lines changed

2 files changed

+93
-19
lines changed

ui/hooks/useCarouselManagement/useCarouselManagement.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,4 +127,40 @@ describe('useCarouselManagement (simple Contentful tests)', () => {
127127

128128
expect(getDispatchedSlides()).toEqual([]);
129129
});
130+
131+
test('lineage is called only once per account, not in infinite loop', async () => {
132+
const mockGetUserProfileLineage = jest.requireMock(
133+
'../../store/actions',
134+
).getUserProfileLineage;
135+
136+
mockGetUseExternalServices.mockReturnValue(true);
137+
mockGetRemoteFeatureFlags.mockReturnValue({
138+
contentfulCarouselEnabled: true,
139+
});
140+
141+
const { rerender } = renderHook(() => useCarouselManagement());
142+
143+
await waitFor(() => expect(mockUpdateSlides).toHaveBeenCalled());
144+
145+
mockGetUserProfileLineage.mockClear();
146+
mockUpdateSlides.mockClear();
147+
148+
mockGetSlides.mockReturnValue([slide('fund')]);
149+
150+
rerender();
151+
152+
// Wait a bit to ensure any potential loops would manifest
153+
await new Promise((resolve) => setTimeout(resolve, 100));
154+
155+
// Lineage should NOT be called again just because slides changed
156+
// (it should only be called when account/settings change)
157+
expect(mockGetUserProfileLineage).not.toHaveBeenCalled();
158+
159+
mockGetSelectedInternalAccount.mockReturnValue({ address: '0xnew' });
160+
rerender();
161+
162+
await waitFor(() => expect(mockGetUserProfileLineage).toHaveBeenCalled());
163+
164+
expect(mockGetUserProfileLineage).toHaveBeenCalledTimes(1);
165+
});
130166
});

ui/hooks/useCarouselManagement/useCarouselManagement.ts

Lines changed: 57 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { isEqual } from 'lodash';
2-
import { useEffect, useRef } from 'react';
2+
import { useEffect, useRef, useState } from 'react';
33
import { useDispatch, useSelector } from 'react-redux';
44
import log from 'loglevel';
55
import { BigNumber } from 'bignumber.js';
@@ -103,8 +103,62 @@ export const useCarouselManagement = ({
103103
const hasZeroBalance = new BigNumber(totalBalance ?? ZERO_BALANCE).eq(
104104
ZERO_BALANCE,
105105
);
106+
const contentfulEnabled =
107+
remoteFeatureFlags?.contentfulCarouselEnabled ?? false;
108+
109+
const [downloadEligible, setDownloadEligible] = useState<boolean>(false);
110+
const [downloadEligibilityReady, setDownloadEligibilityReady] =
111+
useState<boolean>(false);
112+
113+
useEffect(() => {
114+
const eligibilityNeeded =
115+
contentfulEnabled && useExternalServices && showDownloadMobileAppSlide;
116+
117+
if (!eligibilityNeeded) {
118+
setDownloadEligible(false);
119+
setDownloadEligibilityReady(true);
120+
return () => undefined;
121+
}
122+
123+
setDownloadEligibilityReady(false);
124+
125+
let cancelled = false;
126+
127+
(async () => {
128+
try {
129+
const lineage = await getUserProfileLineageAction();
130+
if (!cancelled) {
131+
const onMobile = Boolean(
132+
lineage?.lineage?.some((l) => l.agent === Platform.MOBILE),
133+
);
134+
setDownloadEligible(!onMobile);
135+
setDownloadEligibilityReady(true);
136+
}
137+
} catch (error) {
138+
if (!cancelled) {
139+
log.warn('Failed to fetch user profile lineage:', error);
140+
setDownloadEligible(false);
141+
setDownloadEligibilityReady(true);
142+
}
143+
}
144+
})();
145+
146+
return () => {
147+
cancelled = true;
148+
};
149+
}, [
150+
selectedAccount.address,
151+
useExternalServices,
152+
showDownloadMobileAppSlide,
153+
contentfulEnabled,
154+
]);
106155

107156
useEffect(() => {
157+
// Wait until eligibility is resolved (or not required) to avoid double fetch
158+
if (!downloadEligibilityReady) {
159+
return;
160+
}
161+
108162
// If carousel is disabled, clear the slides
109163
if (!enabled) {
110164
const empty: CarouselSlide[] = [];
@@ -115,9 +169,6 @@ export const useCarouselManagement = ({
115169
return;
116170
}
117171
const maybeFetchContentful = async () => {
118-
const contentfulEnabled =
119-
remoteFeatureFlags?.contentfulCarouselEnabled ?? false;
120-
121172
// Early Return if Contentful is disabled
122173
if (!contentfulEnabled) {
123174
const empty: CarouselSlide[] = [];
@@ -156,17 +207,6 @@ export const useCarouselManagement = ({
156207
return s;
157208
};
158209

159-
const downloadEligible = await (async () => {
160-
if (!useExternalServices || !showDownloadMobileAppSlide) {
161-
return false;
162-
}
163-
const lineage = await getUserProfileLineageAction();
164-
const onMobile = Boolean(
165-
lineage?.lineage?.some((l) => l.agent === Platform.MOBILE),
166-
);
167-
return !onMobile;
168-
})();
169-
170210
const isEligible = (s: CarouselSlide) => {
171211
// Show Download Mobile App (only if not already on mobile + flags)
172212
if (s.variableName === 'downloadMobileApp') {
@@ -211,13 +251,11 @@ export const useCarouselManagement = ({
211251
enabled,
212252
dispatch,
213253
hasZeroBalance,
214-
remoteFeatureFlags,
254+
contentfulEnabled,
215255
testDate,
216256
inTest,
217257
slides,
218-
selectedAccount.address,
219-
useExternalServices,
220-
showDownloadMobileAppSlide,
258+
downloadEligibilityReady,
221259
]);
222260

223261
return { slides };

0 commit comments

Comments
 (0)