Skip to content

fix(backend,clerk-js): treat undefined satelliteAutoSync as false#8001

Open
nikosdouvlis wants to merge 6 commits intomainfrom
nikos/fix-satellite-auto-sync-default
Open

fix(backend,clerk-js): treat undefined satelliteAutoSync as false#8001
nikosdouvlis wants to merge 6 commits intomainfrom
nikos/fix-satellite-auto-sync-default

Conversation

@nikosdouvlis
Copy link
Member

@nikosdouvlis nikosdouvlis commented Mar 5, 2026

Why

Users upgrading to Core 3 expect satelliteAutoSync to default to false as documented in the JSDoc (@default false) and as the Core 3 upgrade codemod implies (it adds satelliteAutoSync={true} to existing satellite configs to preserve Core 2 behavior).

However, the runtime check used === false (strict equality), so undefined (not passing the prop) behaved identically to true, preserving Core 2 auto-sync-on-every-page-load behavior. A customer reported that their satellite domain was redirecting on every page load despite not opting into auto-sync.

What changed

  • Changed the satellite auto-sync gate from === false to !== true in both @clerk/backend (SSR middleware path in request.ts) and @clerk/clerk-js (CSR initialization in clerk.ts)
  • undefined and false now both skip automatic satellite handshake, matching the documented Core 3 default
  • true must be explicitly passed to enable auto-sync (opt-in)
  • __clerk_synced=false (post sign-in redirect) still triggers handshake regardless of the setting

Test coverage

  • Updated existing unit tests that relied on implicit auto-sync to explicitly pass satelliteAutoSync: true
  • Added new unit tests for undefined default behavior (prod + dev, with and without __clerk_synced=false)
  • Updated integration test that expected 307 redirect with unset satelliteAutoSync to expect 200
  • Added integration test verifying __clerk_synced=false still triggers handshake when satelliteAutoSync is unset

Packages affected

  • @clerk/backend: satellite handshake decision logic in request.ts
  • @clerk/clerk-js: #shouldSyncWithPrimary in clerk.ts

Summary by CodeRabbit

  • Bug Fixes

    • satelliteAutoSync now defaults to false (opt-in); automatic cross-domain satellite sync runs only when explicitly enabled.
    • Sign-out detection corrected so signed-out state is reported accurately.
  • Tests

    • Updated and added tests to cover auto-sync opt-in behavior and handshake/redirect expectations.
  • Documentation

    • Added changelog entry documenting the satelliteAutoSync default fix.

…re 3 default)

The JSDoc on satelliteAutoSync says @default false, and the Core 3
upgrade codemod adds satelliteAutoSync={true} to existing satellite
configs, but the runtime check used === false (strict equality).
This meant undefined (not passing the prop) behaved like true,
preserving Core 2 auto-sync behavior instead of the intended Core 3
default of no auto-sync.

Change the check from === false to !== true so that undefined is
treated the same as false, matching the documented default.
@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2026

🦋 Changeset detected

Latest commit: e664eb1

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

This PR includes changesets to release 14 packages
Name Type
@clerk/backend Patch
@clerk/clerk-js Patch
@clerk/agent-toolkit Patch
@clerk/astro Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/hono Patch
@clerk/nextjs Patch
@clerk/nuxt Patch
@clerk/react-router Patch
@clerk/tanstack-react-start Patch
@clerk/testing Patch
@clerk/chrome-extension Patch
@clerk/expo 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

@vercel
Copy link

vercel bot commented Mar 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
clerk-js-sandbox Skipped Skipped Mar 6, 2026 7:45am

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 81066520-9770-48b6-b559-61ad633ea590

📥 Commits

Reviewing files that changed from the base of the PR and between 90fed2d and e664eb1.

📒 Files selected for processing (1)
  • packages/clerk-js/src/core/__tests__/clerk.test.ts

📝 Walkthrough

Walkthrough

The PR changes satellite auto-sync behavior: satelliteAutoSync now enables auto-sync only when explicitly true; undefined or false skip auto-sync. Backend and client checks were updated to use the stricter condition. Tests were added/updated and a changelog entry added to document the default change. AuthCookieService.isSignedOut() was corrected to return true when no user is present.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: treating undefined satelliteAutoSync as false, which is the primary fix across backend and clerk-js packages.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

The post-load branch of isSignedOut() returned !!this.clerk.user,
which is true when the user IS signed in. This is inverted from
what the method name implies. Currently dead code (the only caller
runs before clerk.loaded is true), but fixing it prevents a latent
bug where signed-in satellite users with satelliteAutoSync=true
would get bounced to primary unnecessarily if this method were
ever called post-load.
@nikosdouvlis nikosdouvlis changed the title fix(backend,clerk-js): treat undefined satelliteAutoSync as false (Core 3 default) fix(backend,clerk-js): treat undefined satelliteAutoSync as false Mar 5, 2026
@nikosdouvlis nikosdouvlis marked this pull request as draft March 5, 2026 23:24
@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 6, 2026

Open in StackBlitz

@clerk/agent-toolkit

npm i https://pkg.pr.new/@clerk/agent-toolkit@8001

@clerk/astro

npm i https://pkg.pr.new/@clerk/astro@8001

@clerk/backend

npm i https://pkg.pr.new/@clerk/backend@8001

@clerk/chrome-extension

npm i https://pkg.pr.new/@clerk/chrome-extension@8001

@clerk/clerk-js

npm i https://pkg.pr.new/@clerk/clerk-js@8001

@clerk/dev-cli

npm i https://pkg.pr.new/@clerk/dev-cli@8001

@clerk/expo

npm i https://pkg.pr.new/@clerk/expo@8001

@clerk/expo-passkeys

npm i https://pkg.pr.new/@clerk/expo-passkeys@8001

@clerk/express

npm i https://pkg.pr.new/@clerk/express@8001

@clerk/fastify

npm i https://pkg.pr.new/@clerk/fastify@8001

@clerk/hono

npm i https://pkg.pr.new/@clerk/hono@8001

@clerk/localizations

npm i https://pkg.pr.new/@clerk/localizations@8001

@clerk/nextjs

npm i https://pkg.pr.new/@clerk/nextjs@8001

@clerk/nuxt

npm i https://pkg.pr.new/@clerk/nuxt@8001

@clerk/react

npm i https://pkg.pr.new/@clerk/react@8001

@clerk/react-router

npm i https://pkg.pr.new/@clerk/react-router@8001

@clerk/shared

npm i https://pkg.pr.new/@clerk/shared@8001

@clerk/tanstack-react-start

npm i https://pkg.pr.new/@clerk/tanstack-react-start@8001

@clerk/testing

npm i https://pkg.pr.new/@clerk/testing@8001

@clerk/ui

npm i https://pkg.pr.new/@clerk/ui@8001

@clerk/upgrade

npm i https://pkg.pr.new/@clerk/upgrade@8001

@clerk/vue

npm i https://pkg.pr.new/@clerk/vue@8001

commit: e664eb1

These tests reset mockClientFetch in their beforeEach but didn't
set up a return value. Previously this was fine because
#shouldSyncWithPrimary() returned true and the code never reached
Client.fetch(). Now that satelliteAutoSync defaults to false, the
code falls through to the client fetch, so a mock is needed.
@nikosdouvlis nikosdouvlis marked this pull request as ready for review March 6, 2026 07:44
return this.clientUat.get() <= 0;
}
return !!this.clerk.user;
return !this.clerk.user;
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be outside of the scope of this PR. Should we split this out into its own change so it has a changelog entry?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the change is small enough to not warrant a new PR, I also dont think a changeset is needed as its internal behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants