Skip to content

fix(AuthProvider): split guardComponent into loadingComponent + guardComponent#269

Open
erickteowarang wants to merge 8 commits into
mainfrom
feat/auth/loading-component
Open

fix(AuthProvider): split guardComponent into loadingComponent + guardComponent#269
erickteowarang wants to merge 8 commits into
mainfrom
feat/auth/loading-component

Conversation

@erickteowarang
Copy link
Copy Markdown
Contributor

@erickteowarang erickteowarang commented May 18, 2026

Summary

  • AuthProvider's single guardComponent slot rendered for the union of !isReady and !isAuthenticated. Any sign-in screen wired into that slot — the obvious thing to do, and what the type name implies — flashes on every reload before the session is known. This contradicts the canonical pattern documented in useAuth's own JSDoc.
  • Add loadingComponent for the !isReady state. Narrow guardComponent to fire only when isReady && !isAuthenticated (sign-in screens no longer flash).
  • OAuth callback child-suppression now keys off loadingComponent — the slot that explicitly owns the !isReady state.
  • Default flash protection: when autoLogin is enabled and a slot is omitted, the protected tree is hidden during that state instead of briefly rendering children. When autoLogin is off, children render in those windows — preserving the useAuthSuspense pattern (where a <Suspense> boundary inside the tree owns the loading UI) and public-app cases.

Default behavior matrix (no slots passed)

autoLogin !isReady isReady && !isAuthenticated
true hidden hidden
false / unset children render children render

Migration

// Before — guardComponent doubled as loading + unauthenticated
<AuthProvider client={authClient} guardComponent={() => <LoadingScreen />}>

// After — use the slot that matches your intent
<AuthProvider
  client={authClient}
  loadingComponent={() => <LoadingScreen />}
  guardComponent={() => <SignInScreen />}
>

// Or, with autoLogin, omit both and rely on default flash protection
<AuthProvider client={authClient} autoLogin>

If you were passing a loading UI to guardComponent, rename to loadingComponent. If you were passing a sign-in screen, keep it on guardComponent — it will no longer flash before the auth check resolves.

Test plan

  • pnpm type-check clean
  • pnpm lint 0 warnings / 0 errors
  • pnpm test — 1015 tests pass, including:
    • New regression: guardComponent does NOT render during !isReady
    • Renamed: loadingComponent renders during !isReady
    • Reframed: callback-pending suppression triggers off loadingComponent
    • New: autoLogin hides children during !isReady when no loadingComponent is set
    • New: autoLogin hides children during !isAuthenticated when no guardComponent is set
    • New: without autoLogin, children render during !isReady (Suspense pattern preserved)
  • pnpm fmt applied
  • Changeset added (.changeset/auth-loading-component.md, minor)
  • Docs updated (docs/concepts/authentication.md)

🤖 Generated with Claude Code

…Component

Previously `guardComponent` rendered for the union of `!isReady` and
`!isAuthenticated`, so any sign-in screen wired into that slot would
flash on every reload before the session was known — contradicting the
canonical `useAuth` pattern documented in the SDK itself.

Add `loadingComponent` for the `!isReady` state and narrow
`guardComponent` to fire only when `isReady && !isAuthenticated`. OAuth
callback child-suppression now keys off `loadingComponent` (the slot
that owns the `!isReady` state).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@IzumiSy
Copy link
Copy Markdown
Contributor

IzumiSy commented May 18, 2026

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Generated by API Design Review for issue #269

Comment thread packages/core/src/contexts/auth-context.tsx Outdated
// so the unprotected tree does not flash during the callback exchange.
const resolvedChildren =
callbackStatus === "pending" && props.guardComponent == null ? null : props.children;
callbackStatus === "pending" && props.loadingComponent == null ? null : props.children;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[2/2 — Medium] guardComponent-only consumers get a blank screen during OAuth callback

The condition props.loadingComponent == null means: when a consumer provides only guardComponent (no loadingComponent), resolvedChildren is set to null during a pending OAuth callback.

Execution path with guardComponent only + callbackStatus === "pending":

  1. resolvedChildren = null (this line)
  2. hasGuard = true (since guardComponent is set)
  3. AuthGuard receives children = null
  4. !isReady branch in AuthGuard: loadingComponent ? loadingComponent() : childrenundefined ? ... : nullblank screen

The migration note says "if you were passing a sign-in screen, keep it on guardComponent", which is exactly the consumer who reaches this path. Previously with the old code (props.guardComponent == null), those consumers would have seen guardComponent() rendered during the OAuth callback exchange. Now they see nothing.

This is probably intentional (you don't want a sign-in screen flashing during an active OAuth flow), but:

  • The migration guide doesn't document this behavioral difference for the OAuth callback case
  • The test "should render guarded children while callback is pending" was removed and replaced with a loadingComponent variant — leaving no coverage for the guardComponent-only + callback-pending path

Suggested fix: add a sentence to the changeset migration note acknowledging this and cover the scenario in a test:

it("should render nothing during callback when only guardComponent is set", async () => {
  // guardComponent-only consumers see null (not their guard) during OAuth
  // callback, because loadingComponent owns the !isReady state.
  ...

Alternatively, if showing guardComponent during callback is preferable UX (avoiding a blank flash), the condition could be:

callbackStatus === "pending" && props.loadingComponent == null && props.guardComponent == null
  ? null
  : props.children;

though this would render the sign-in screen during callback, which is the original bug this PR fixes for the loading case.

…gin is on

`loadingComponent` and `guardComponent` are both optional. Previously,
omitting one meant children rendered through during that state — which,
combined with `autoLogin`, briefly showed protected UI before the
redirect fired.

Couple the "hide on omission" default to `autoLogin`: when it is on, an
unset slot renders nothing so protected UI never flashes; when it is
off, children continue to render so the `useAuthSuspense` and public-app
patterns still work without any new props.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 18, 2026

Open in StackBlitz

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

commit: 659ddd6

Erick Teowarang and others added 2 commits May 18, 2026 16:23
`pnpm fmt:check` failed CI on the props table. Also point the
`useAuthSuspense` cross-reference at the correct heading slug
(`#suspense-compatible-hook`, not `#suspense-integration`).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous callback suppression set `resolvedChildren` to `null` but
still rendered `AuthGuard`. If the auth client surfaced a stale
`isReady && !isAuthenticated` state while a callback exchange was in
flight, `AuthGuard` would render `guardComponent` (the sign-in screen) —
flashing the very UI the user just came back from.

Bypass `AuthGuard` entirely during the blackout window. Providing
`loadingComponent` still opts the consumer out of the blackout, so the
"trust me, I'll handle transitions" contract is preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines +303 to +307
if (loadingComponent) return loadingComponent();
return hideUnresolved ? null : children;
}
if (!isAuthenticated) {
if (guardComponent) return guardComponent();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

loadingComponent() and guardComponent() are invoked as plain function calls here, which means any hooks inside them become hooks of the AuthGuard component. Since these calls are conditional (loadingComponent() only when !isReady, guardComponent() only when !isAuthenticated), this violates React's rules of hooks — the hook call order inside AuthGuard changes depending on auth state.

In practice, passing a component that calls hooks (e.g. a sign-in screen using useAuth()) to guardComponent triggers: React has detected a change in the order of Hooks called by AuthGuard.

Using createElement gives each slot its own fiber with an isolated hook scope:

Suggested change
if (loadingComponent) return loadingComponent();
return hideUnresolved ? null : children;
}
if (!isAuthenticated) {
if (guardComponent) return guardComponent();
if (loadingComponent) return createElement(loadingComponent);
return hideUnresolved ? null : children;
}
if (!isAuthenticated) {
if (guardComponent) return createElement(guardComponent);

Erick Teowarang and others added 4 commits May 19, 2026 15:55
…ok scope

Invoking `loadingComponent()` / `guardComponent()` as plain function calls
inlined the slot's hooks into AuthGuard's own hook list. Because the
calls are gated on auth state, the hook order changed across renders
the moment a consumer passed a slot that used a hook itself (e.g. a
sign-in screen calling `useAuth`) — tripping React's rules of hooks.

Render each slot via `createElement` so it becomes its own fiber. The
existing tests missed this because their slot components were stateless;
add a regression test that asserts no hook-order warnings when the slot
calls `useAuth` and the auth state transitions out of it.

Reported by IzumiSy in #269.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend the auth Playwright suite with a regression test that walks the
full login flow and asserts:
- the auth guard is not in the DOM after the callback resolves
  (no sign-in flash during the OAuth callback exchange), and
- no React hook-order warnings are logged during the auth state
  transition (the E2E app's <AuthGuard> uses `useAuth`, the exact
  shape that previously inlined a hook into AuthGuard's hook list).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Companion to the existing "maintains session on page reload" test:
that one verifies the authenticated content is eventually visible
after reload, but does not catch a single-frame guardComponent flash
during the `!isReady` window.

Inject a MutationObserver via `addInitScript` so it is attached
before any post-reload render, and assert the auth-guard testid
never enters the DOM while the session is being re-checked.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants