fix: a user cannot click outside of the org selection modal#5031
Open
fix: a user cannot click outside of the org selection modal#5031
Conversation
mcstepp
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughMake the org-selection modal always open, remove external close handlers, centralize pending-session cleanup with a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Browser Client
participant Router as Next.js Router
participant Server as App Server
participant Cookie as Cookie Store
User->>Client: Open sign-in page with pending session
Client->>Client: Render OrgSelector (always open)
User->>Client: Selects organization
Client->>Server: POST completeOrgSelection(orgId)
alt Success
Server-->>Client: success
Client->>Cookie: delete PENDING_SESSION_COOKIE, set/delete last-org
Client->>Router: router.push(dashboard)
Router-->>User: redirect to dashboard
else "does not belong to" error -> mapped to PENDING_SESSION_EXPIRED
Server-->>Client: mapped PENDING_SESSION_EXPIRED error
Client->>Cookie: clearPendingAuth (delete PENDING_SESSION_COOKIE & last-org)
Client->>Router: router.push('/auth/sign-in' + error)
Router-->>User: redirect to sign-in with error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
mcstepp
commented
Feb 13, 2026
mcstepp
commented
Feb 13, 2026
mcstepp
commented
Feb 13, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/apps/dashboard/lib/auth/workos.ts`:
- Around line 862-871: Remove the unsafe type assertion "error as WorkOSError"
and change the catch parameter to be explicitly typed as "unknown"; then perform
structural narrowing before accessing message (e.g., check typeof error ===
'object' && error !== null && 'message' in error && typeof (error as
any).message === 'string') and use that string to test for "does not belong to"
so you avoid asserting WorkOSError. Apply the same change to the other
occurrence (the similar pattern around the symbol at line ~635) so all
WorkOSError handling uses explicit unknown catch typing and runtime checks
instead of type assertions.
perkinsjr
requested changes
Feb 13, 2026
Member
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
No modal hijacking.
If I want to escape because I clicked the wrong user, I am now stuck till it timesout.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/apps/dashboard/app/auth/sign-in/org-selector.tsx`:
- Around line 36-40: The handleClose callback currently awaits
clearPendingAuth() without handling rejections; wrap the await in a try/catch
inside handleClose (the function defined as handleClose) so any error from
clearPendingAuth() is caught and logged (e.g., console.error or a logger) and
then always call router.push("/auth/sign-in") in a finally or after the catch so
the redirect occurs even if cookie/cleanup fails.
🧹 Nitpick comments (1)
web/apps/dashboard/app/auth/sign-in/org-selector.tsx (1)
57-57:isOpenis alwaystrue; consider simplifying.Since
isOpenis initialized totrueand never modified, you could use a constant instead ofuseState. This makes the intent clearer that the dialog cannot be closed programmatically.♻️ Proposed simplification
-const [isOpen] = useState(true); +const isOpen = true;
Contributor
✅ Actions performedReview triggered.
|
perkinsjr
approved these changes
Feb 17, 2026
chronark
reviewed
Feb 18, 2026
|
|
||
| // If session expired, the pending auth was already cleared | ||
| // Just show the error and let user see org selector | ||
| if (result.code === AuthErrorCode.PENDING_SESSION_EXPIRED) { |
Collaborator
There was a problem hiding this comment.
this seems pointless, why are we doing an if check if we don't run any conditional code?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #4935
Fix: Prevent org selector modal from closing when clicking outside the modal and improve auth error handling
Problem: Users could close the org selector modal during authentication, leading to:
Changes
Org Selector Modal
Error Handling
Session Management
Side Quests
_(biome linter accepts this, and the build works, so this was more cosmetic annoyance for false positive than a hard failure)Type of change
How should this be tested?
Prerequisites: A user with multiple workspaces
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated