Remove `useEffect` in authentication mechanism by IzumiSy · Pull Request #59 · tailor-platform/app-shell

IzumiSy · 2026-03-09T07:33:37Z

Improve authentication initialization to run outside of React's lifecycle by moving it into a react-router loader.

Motivation

AuthProvider previously used useEffect to handle OAuth callbacks (handleCallback), check auth status (checkAuthStatus), and trigger auto-login. This caused issues under React strict mode, where useEffect is invoked twice in development, potentially calling handleCallback multiple times.

Changes

Removed useEffect from AuthProvider: All auth side effects (handleCallback, checkAuthStatus, autoLogin) have been removed from React's lifecycle.
Introduced useAuthLoader hook (auth-context.tsx): An internal hook that returns a loader function for handling OAuth callbacks and auth status checks. Returns null when AuthProvider is not present, making auth integration optional.
Introduced createRootLoader (router.tsx): A higher-order function that combines the auth loader with the existing navigation loader. When authLoader is available, it runs before navigation loading and can short-circuit with a redirect response.
OAuth redirections happen before rendering: Since react-router loaders run before component rendering, OAuth callback redirections (e.g., stripping ?code= params) now complete before any UI is mounted, eliminating unnecessary renders.

No Public API Changes

This is a purely internal refactoring. AuthProvider, useAuth, createAuthClient, and all other public exports remain unchanged.

pkg-pr-new · 2026-03-09T07:35:16Z

Open in StackBlitz

npm i https://pkg.pr.new/@tailor-platform/app-shell@59

npm i https://pkg.pr.new/@tailor-platform/app-shell-vite-plugin@59

commit: 9e6d3d2

IzumiSy · 2026-03-09T07:56:15Z

/review

github-actions

Generated by API Design Review for issue #59

packages/core/src/contexts/auth-context.tsx

packages/core/src/routing/router.tsx

IzumiSy · 2026-03-09T08:15:56Z

/review

github-actions

Generated by API Design Review for issue #59

packages/core/src/contexts/auth-context.tsx

IzumiSy · 2026-03-09T08:53:46Z

/review

github-actions

Generated by API Design Review for issue #59

packages/core/src/contexts/auth-context.tsx

IzumiSy · 2026-03-10T03:32:01Z

~~TODO: need checking the behaviour on the real app.~~

Confirmed changes are working fine

Move replace(buildCleanOAuthCallbackUrl()) outside of the try block so the ?code= parameter is always removed from the URL regardless of whether handleCallback() succeeds or fails. This prevents an unrecoverable auth error loop caused by re-attempting the callback with an expired code.

Replace inline function type with the exported AuthLoader type for consistency and single-site maintenance.

…ize checkAuthStatus calls - Add try/catch around checkAuthStatus and login in the loader to prevent error boundary rendering on transient failures - Add @internal annotation to AuthLoader type for consistency - Skip checkAuthStatus on subsequent navigations (only run when isReady is false) - Separate autoLogin evaluation from checkAuthStatus to handle post-callback flow - Remove buildCleanOAuthCallbackUrl (handleCallback handles URL cleanup internally) - Update tests to reflect new behavior

IzumiSy · 2026-03-10T14:35:42Z

/review

github-actions

Generated by API Design Review for issue #59

github-actions · 2026-03-10T15:49:53Z

packages/core/src/contexts/auth-context.tsx

+      // handleCallback() internally cleans up the OAuth-related query parameters
+      // from the URL, so no additional URL cleanup is needed here.
+      if (requestUrl.searchParams.has("code")) {
+        try {
+          await client.handleCallback();
+        } catch (error) {


[1/4 — High] Unverified assumption: handleCallback() cleans the URL

The old code explicitly removed OAuth params from the URL:

// Old code await client.handleCallback(); const cleanUrl = buildCleanOAuthCallbackUrl(new URL(window.location.href)); window.history.replaceState({}, "", cleanUrl);

The new code relies on the comment "handleCallback() internally cleans up the OAuth-related query parameters", but this is an unverified assumption about a third-party library (@tailor-platform/auth-public-client). If handleCallback doesn't update window.location, then after a successful OAuth login, refreshing the page would re-run this loader with ?code= still in the URL, calling handleCallback again with a stale one-time-use authorization code — an error most OAuth servers will reject.

The matching test (router.test.tsx "handles OAuth callback and redirects to clean URL") doesn't assert URL cleanup, so there's no regression protection.

Consider adding explicit URL cleanup after handleCallback, or adding a test assertion that verifies the URL no longer contains ?code= after the loader runs.

github-actions · 2026-03-10T15:49:53Z

packages/core/src/routing/router.test.tsx

+  it("handles OAuth callback and redirects to clean URL", async () => {
+    const mockHandleCallback = vi.fn().mockResolvedValue(undefined);
+    const mockCheckAuthStatus = vi.fn().mockResolvedValue({
+      isAuthenticated: true,
+      error: null,
+      isReady: true,
+    });
+    const authClient = createMockAuthClient(
+      { isAuthenticated: true, error: null, isReady: true },
+      {
+        handleCallback: mockHandleCallback,
+        checkAuthStatus: mockCheckAuthStatus,
+      },
+    );
+
+    renderWithConfig({
+      modules: [],
+      rootComponent: () => <div>Home</div>,
+      initialEntries: ["/?code=auth-code-123&state=abc"],
+      authClient,
+    });
+
+    await screen.findByText("Home");


[2/4 — Medium] Test name promises URL cleanup that isn't asserted

This test is titled "handles OAuth callback and redirects to clean URL" but only asserts mockHandleCallback was called — it does not verify the URL was actually cleaned (no assertion on the browser URL, no replaceState spy).

The previous test explicitly used replaceStateSpy to confirm URL cleanup. That assertion was removed here, leaving the "redirects to clean URL" claim unverified. If URL cleanup regresses (e.g., if handleCallback never cleaned up the URL), this test would still pass.

Either rename the test to accurately describe what's tested, or add an assertion that the URL no longer contains ?code= after the loader runs (e.g., inspecting the rendered URL or window.location.href on a MemoryRouter).

github-actions · 2026-03-10T15:49:53Z

packages/core/src/contexts/auth-context.tsx

      value={{
-        authState,
-        login: () => client.login(),
-        logout: () => client.logout(),
-        checkAuthStatus: () => client.checkAuthStatus(),
-        ready: () => client.ready(),
+        client,
+        autoLogin: props.autoLogin,
+        guardComponent: props.guardComponent,
      }}


[3/4 — Medium] AuthClientContext.Provider has an unstable value (missing useMemo)

// Current code — new object on every render (AuthClientContext.Provider value=\{\{ client, autoLogin: props.autoLogin, guardComponent: props.guardComponent, }} )

The value object is recreated on every AuthProvider render. AuthProvider re-renders on every auth state change (via useSyncExternalStore), which means every auth state transition produces a new context value. All AuthClientContext consumers (notably RouterContainer via useAuthLoader) will re-render in response.

This is inconsistent with how AppShell manages its context values (with useMemo). More critically: RouterContainer calls createBrowserRouter/createMemoryRouter unconditionally in its render body, so any re-render of RouterContainer recreates the entire router — resetting navigation state (current route, history, etc.) on each auth state change (e.g., the transition from isReady: false → isReady: true on initial load).

// Suggested fix const authClientContextValue = useMemo( () => ({ client, autoLogin: props.autoLogin, guardComponent: props.guardComponent }), [client, props.autoLogin, props.guardComponent], ); // ... (AuthClientContext.Provider value={authClientContextValue})

github-actions · 2026-03-10T15:49:53Z

packages/core/src/contexts/auth-context.test.tsx

  useAuth,
  useAuthSuspense,
-  buildCleanOAuthCallbackUrl,
+  useAuthLoader,


[4/4 — Low] buildCleanOAuthCallbackUrl is now dead code — exported but never called, tests removed

This PR removed the useEffect that called buildCleanOAuthCallbackUrl, and deleted its 10 well-specified tests ("removes code parameter", "preserves hash fragments", etc.). However, buildCleanOAuthCallbackUrl is still exported from auth-context.tsx and remains in the public-facing file — it's just no longer called anywhere.

If the function is no longer needed (because handleCallback handles URL cleanup internally), it should be removed along with its JSDoc. If it's intentionally kept as a utility for external callers, its tests should be restored.

Leaving it exported without tests and without usages is confusing and creates false surface area.

IzumiSy self-assigned this Mar 9, 2026

IzumiSy changed the title ~~Remove useEffect in authentication mechanism~~ Remove useEffect in authentication mechanism Mar 9, 2026

github-actions bot reviewed Mar 9, 2026

View reviewed changes

packages/core/src/contexts/auth-context.tsx Outdated Show resolved Hide resolved

packages/core/src/routing/router.tsx Outdated Show resolved Hide resolved

github-actions bot reviewed Mar 9, 2026

View reviewed changes

packages/core/src/contexts/auth-context.tsx Outdated Show resolved Hide resolved

packages/core/src/contexts/auth-context.tsx Show resolved Hide resolved

github-actions bot reviewed Mar 9, 2026

View reviewed changes

packages/core/src/contexts/auth-context.tsx Outdated Show resolved Hide resolved

packages/core/src/contexts/auth-context.tsx Outdated Show resolved Hide resolved

IzumiSy force-pushed the remove_use_effect_auth_redirection branch from 3a87876 to 4716d75 Compare March 10, 2026 03:31

IzumiSy force-pushed the remove_use_effect_auth_redirection branch from 4716d75 to 3a42209 Compare March 10, 2026 08:47

IzumiSy added 11 commits March 10, 2026 23:30

Remove useEffect in authentication mechanism

a0144e7

Format files

58e3e71

Revert change in navigation.tsx

6f55069

Revert unnecessary change again

e628af2

Add integration tests

c253eef

Add changeset

5589101

refactor: use AuthLoader type alias in createRootLoader

2dc101a

Replace inline function type with the exported AuthLoader type for consistency and single-site maintenance.

Format

59552ce

Fix guardComponent resolution failure

9e6d3d2

IzumiSy force-pushed the remove_use_effect_auth_redirection branch from 3fe2bb6 to 9e6d3d2 Compare March 10, 2026 14:30

github-actions bot reviewed Mar 10, 2026

View reviewed changes

Conversation

IzumiSy commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

No Public API Changes

Uh oh!

pkg-pr-new bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IzumiSy commented Mar 9, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IzumiSy commented Mar 9, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IzumiSy commented Mar 9, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IzumiSy commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IzumiSy commented Mar 10, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

IzumiSy commented Mar 9, 2026 •

edited

Loading

pkg-pr-new bot commented Mar 9, 2026 •

edited

Loading

IzumiSy commented Mar 10, 2026 •

edited

Loading