-
Notifications
You must be signed in to change notification settings - Fork 368
fix(clerk-js): Preserve active organization when attempting to switch to personal workspace #6103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: ffb8f5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
📝 Walkthrough""" WalkthroughThis change updates the internal logic of the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@@ -1071,10 +1071,6 @@ export class Clerk implements ClerkInterface { | |||
); | |||
} | |||
|
|||
if (organization === null && this.environment?.organizationSettings?.forceOrganizationSelection) { | |||
throw new Error('setActive requires an organization parameter when organization selection is forced.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's better to not throw an error and just handle it idempotently as we're doing on the backend, cause it might break the entire application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/dark-cougars-burn.md (1)
5-5
: Add punctuation for consistency.
Consider ending the summary line with a period to maintain consistency across changesets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/dark-cougars-burn.md
(1 hunks)packages/clerk-js/src/core/__tests__/clerk.test.ts
(1 hunks)packages/clerk-js/src/core/clerk.ts
(2 hunks)
🔇 Additional comments (3)
packages/clerk-js/src/core/clerk.ts (2)
29-29
: Import order change looks good.The reorganization groups related imports together, which improves code organization.
1106-1115
:❓ Verification inconclusive
Implementation correctly prevents switching to personal workspace when force organization selection is enabled.
The logic properly handles the case by returning early without updating the session, which aligns with the PR objectives and the idempotent approach mentioned in past review comments.
Since the PR objectives mention that FAPI endpoints
/tokens
and/touch
already handle this scenario idempotently, let's verify the behavior is consistent:
🏁 Script executed:
#!/bin/bash # Description: Search for similar force organization selection handling in the codebase to ensure consistency # Look for other references to forceOrganizationSelection to understand the pattern rg -A 5 -B 5 "forceOrganizationSelection" # Search for handling of organization switching in token/touch endpoints ast-grep --pattern 'forceOrganizationSelection && $$$null$$$'Length of output: 11923
Approve in-memory logic; verify backend idempotency
The client-side implementation in
packages/clerk-js/src/core/clerk.ts
(lines 1106–1115) prevents switching to the personal workspace whenenvironment.organizationSettings.forceOrganizationSelection
is true. Similar flags are used consistently in:
usePreloadTasks.ts
(hasTasks
check)OrganizationSwitcher.ts
andOrganizationList.ts
(hidePersonal
props)Please confirm that the FAPI endpoints (
/tokens
and/touch
) on the backend also enforce this scenario idempotently, matching the PR objectives..changeset/dark-cougars-burn.md (1)
1-3
: YAML frontmatter is well-formed.
The changeset correctly specifies a patch bump for the@clerk/clerk-js
package.
5a0ed3d
to
4dbe881
Compare
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
4dbe881
to
8e56f9f
Compare
8e56f9f
to
1c9a95a
Compare
1c9a95a
to
f42b0a5
Compare
f42b0a5
to
98d41f6
Compare
98d41f6
to
ffb8f5b
Compare
… to personal workspace (#6103)
Description
Related to #6073
Instead of throwing an error, this PR maintains the latest active organization ID and does not update the session to the personal workspace.
Important to note that this only applies to the
Clerk.session
singleton, as FAPI has already been updated on/tokens
and/touch
to also handle it in a idempotent way.Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change
Summary by CodeRabbit