Fix external auth buttons in React Strict Mode #6175

mirekm · 2025-12-04T14:09:49Z

No description provided.

changeset-bot · 2025-12-04T14:09:54Z

🦋 Changeset detected

Latest commit: 59b0b21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
saleor-dashboard	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copilot

Pull request overview

This PR addresses issues with external authentication buttons in React Strict Mode by converting from a lazy query to a regular query with skip logic and implementing a ref-based guard to prevent double-processing of authentication callbacks.

Key Changes:

Replaced useAvailableExternalAuthenticationsLazyQuery with useAvailableExternalAuthenticationsQuery using conditional skip logic
Added useRef to prevent double-processing of external auth callbacks in React Strict Mode
Removed problematic cleanup logic from useEffect that was clearing localStorage during simulated unmounts

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/auth/views/Login.tsx	Converted lazy query to regular query with skip parameter; added ref-based guard for callback processing; removed cleanup logic from useEffect
src/auth/components/LoginPage/LoginPage.test.tsx	Improved test structure with helper function; added test cases for edge cases (empty array, undefined); removed mock in favor of actual MemoryRouter

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/auth/views/Login.tsx

codecov · 2025-12-04T14:13:22Z

Codecov Report

❌ Patch coverage is 11.76471% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.56%. Comparing base (5373ccd) to head (59b0b21).

Files with missing lines	Patch %	Lines
src/auth/views/Login.tsx	11.76%	15 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6175    +/-   ##
========================================
  Coverage   39.56%   39.56%            
========================================
  Files        2415     2415            
  Lines       39920    39918     -2     
  Branches     8821     9149   +328     
========================================
+ Hits        15794    15795     +1     
+ Misses      24100    24097     -3     
  Partials       26       26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-04T14:34:24Z

src/auth/views/Login.tsx

  useEffect(() => {
-    const { code, state } = params;
-    const externalAuthParamsExist = code && state && isCallbackPath;
    const externalAuthNotPerformed = !authenticating && !errors.length;

-    if (externalAuthParamsExist && externalAuthNotPerformed) {
-      handleExternalAuthentication(code, state);
+    // Guard against double-processing in React Strict Mode
+    if (isExternalAuthCallback && externalAuthNotPerformed && !externalAuthProcessedRef.current) {
+      externalAuthProcessedRef.current = true;
+      handleExternalAuthentication(code!, state!);
    }
-
-    return () => {
-      setRequestedExternalPluginId(null);
-      setFallbackUri(null);
-    };
-  }, []);
+  }, [isExternalAuthCallback, authenticating, errors.length]);


The useEffect hook is missing handleExternalAuthentication, code, and state in its dependency array. According to React's exhaustive-deps rule, all values used inside the effect that come from the component scope should be included in the dependency array.

Since handleExternalAuthentication is defined in the component, it should either be:

Added to the dependency array along with code and state (which would require memoizing handleExternalAuthentication with useCallback)

Or moved inside the useEffect to avoid the dependency

The recommended fix is to move the function definition inside the useEffect:

useEffect(() => { const externalAuthNotPerformed = !authenticating && !errors.length; if (isExternalAuthCallback && externalAuthNotPerformed && !externalAuthProcessedRef.current) { externalAuthProcessedRef.current = true; const handleExternalAuthentication = async (code: string, state: string) => { await loginByExternalPlugin!(requestedExternalPluginId, { code, state, }); setRequestedExternalPluginId(null); navigate(fallbackUri); setFallbackUri(null); }; handleExternalAuthentication(code!, state!); } }, [isExternalAuthCallback, authenticating, errors.length, loginByExternalPlugin, requestedExternalPluginId, setRequestedExternalPluginId, navigate, fallbackUri, setFallbackUri, code, state]);

witoszekdev · 2025-12-04T15:10:30Z

src/auth/views/Login.tsx

+  // Track if external auth callback has been processed to avoid double-processing in Strict Mode
+  const externalAuthProcessedRef = useRef(false);


issue: I don't think we should create hacks to work around strict mode, if we have issue there it probably means we have something wrong with our implementation. It's worth checking ESLint warnings.

There's also a possibility we could introduce a bug here, as copilot wrote: https://github.com/saleor/saleor-dashboard/pull/6175/files#r2589250212

As far as I see it it's not necessarily a hack for React Strict Mode per se. What should we do when component remounts and Apollo has the lazy query in flight?

Assuming we can't have that race condition realistically outside of strict mode the ref is indeed unnecessary but what do we do then for the strict mode so it works as expected?

The comment was I think already addressed.

witoszekdev · 2025-12-04T15:11:30Z

src/auth/views/Login.tsx

+    // Guard against double-processing in React Strict Mode
+    if (isExternalAuthCallback && externalAuthNotPerformed && !externalAuthProcessedRef.current) {
+      externalAuthProcessedRef.current = true;
+      handleExternalAuthentication(code!, state!);
    }


issue: again we probably have issue somewhere else, we shouldn't write specific fixes to Strict mode

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Fix external auth buttons in React Strict Mode

5babaf2

Copilot AI review requested due to automatic review settings December 4, 2025 14:09

mirekm requested a review from a team as a code owner December 4, 2025 14:09

mirekm requested review from lkostrowski and witoszekdev December 4, 2025 14:09

Copilot started reviewing on behalf of mirekm December 4, 2025 14:10 View session

Copilot finished reviewing on behalf of mirekm December 4, 2025 14:12

Copilot AI reviewed Dec 4, 2025

View reviewed changes

src/auth/views/Login.tsx Outdated Show resolved Hide resolved

src/auth/views/Login.tsx Show resolved Hide resolved

mirekm added 2 commits December 4, 2025 15:16

Add changeset

aad1284

Reset the whole cycle after processing

31de9fe

Copilot AI review requested due to automatic review settings December 4, 2025 14:30

Copilot started reviewing on behalf of mirekm December 4, 2025 14:31 View session

Copilot finished reviewing on behalf of mirekm December 4, 2025 14:33

Copilot AI reviewed Dec 4, 2025

View reviewed changes

Use all values used inside the effect in dependency array

59b0b21

witoszekdev reviewed Dec 4, 2025

View reviewed changes

Copilot AI reviewed Dec 4, 2025

View reviewed changes

		// Track if external auth callback has been processed to avoid double-processing in Strict Mode
		const externalAuthProcessedRef = useRef(false);

Fix external auth buttons in React Strict Mode #6175

Are you sure you want to change the base?

Fix external auth buttons in React Strict Mode #6175

Uh oh!

Conversation

mirekm commented Dec 4, 2025

Uh oh!

changeset-bot bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

witoszekdev Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

mirekm Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

mirekm Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

mirekm Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

witoszekdev Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Dec 4, 2025 •

edited

Loading

codecov bot commented Dec 4, 2025 •

edited

Loading