feat(web): port auth pages from platform, fix route architecture to match platform URLs#31141
Conversation
…routes Route constants had /assistant prefix (e.g. /assistant/settings/general) but the React Router basename is already /assistant. Using these with navigate() or <Link to> would produce /assistant/assistant/settings/general. Changes: - Strip /assistant prefix from all route constants in utils/routes.ts - Export BASENAME constant for code that needs full browser paths - Update getSameOriginRoutePath to strip basename from returned paths - Update gate.ts, native-auth.ts, use-assistant-lifecycle.ts to use BASENAME - Delete duplicate lib/routes.ts (byte-for-byte identical, zero imports) - Use BASENAME in routes.tsx instead of hardcoded string Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Port real implementations for the account domain: - Login page (web + native iOS variants) - Signup redirect page - Provider callback handler (session probing, error display) - Provider signup completion form - Account landing page - OAuth popup completion page (web + native deep link) Supporting files: - Login flow URL builders (login-flow.ts) - Username heuristics (handle-heuristics.ts) - User profile API wrappers (profile.ts) - Native deep link parsing for iOS OAuth (native-deep-link.ts) - Account shell, form components, NativeSplash - GoogleLogo icon, LoginBackground Wire all account routes into React Router (outside ChatLayout). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
✦ APPROVE
Value: Lands the real auth/identity UI in the new web repo and ships the route-constant fix I flagged on #31109 — every internal navigate(routes.x) call now resolves to the correct path instead of /assistant/assistant/.... Unblocks the web-move execution arc and the iOS/Capacitor auth flow keeps working end-to-end.
What this does:
- Route fix (commit 1): strips the duplicate
/assistantprefix from every constant inutils/routes.ts(basename is applied bycreateBrowserRouter) and deletes the stalelib/routes.tsthat was shadowing it. - Auth pages (commit 2): ports
LoginPage,SignupPage,ProviderCallbackPage,ProviderSignupPage,OAuthPopupCompletePage,AccountPageplus supportinglib/account/*helpers,runtime/native-deep-link.ts, andcomponents/account/*primitives. Account routes are registered as siblings ofChatLayout(full-screen, no sidebar).
Strong points
- The route constants now match how they're registered in
routes.tsx(account/login,account/provider/callback,account/oauth/popup-complete). Spot-checked allaccount.*andsettings.*paths — they line up. This was the exact double-prefix bug I called out in the #31109 review. useIsNativePlatformis used consistently in render paths (login-page.tsx); directisNativePlatform()only shows up in non-render contexts (startAuthFlow,openOAuthUrlInPopup). Matches the SSR-safety rule.profile.tskeepsApiErrordiscipline —fetchMe/checkUsernameAvailabilitythrowApiErrorwithassertHasResponse+extractErrorMessage, andupdateMereturns a discriminated union for its UX-driven 400/409 branches. No rawErrorleaks.native-deep-link.tsparses incoming URLs against an explicitALLOWED_NATIVE_URL_PROTOCOLSallowlist and matcheshost === OAUTH_COMPLETE_DEEP_LINK_HOSTbefore reading params. Good open-redirect / scheme-spoofing posture.provider-callback-page.tsxmirrors native errors back throughredirectToNativeAppso the iOS/macOS host gets a clean?error=…even on the unhappy path. Allowed schemes are gated against theNATIVE_AUTH_ALLOWED_SCHEMESmirror.oauth-popup-links.tsstripsBASENAMEingetSameOriginRoutePathbefore passing tonavigate()— same root cause as the route fix, handled consistently.
Non-blocking — please consider before/after merge
-
Async effects miss try/catch. Two callback effects fire-and-forget an IIFE without a
try/catch:provider-callback-page.tsx(the(async () => { … getSession() … refreshSession() … })()afterdidRun.current).provider-signup-page.tsx((async () => { const result = await getProviderSignup(); … })()).
If
getSession,getProviderSignup, orrefreshSessionrejects, that's an unhandled promise rejection — exactly the pattern called out inanti-patterns-web.md(PR #6726, Sentry 7479512595). ThedidRunflag protects against double-execution but not against rejection. Wrap the IIFE bodies intry { … } catch (err) { setFallbackError("…"); /* or navigate to login */ }so transient session/network errors surface as inline UI instead of console + Sentry noise. -
Helper duplicated.
shouldUseFullPageNavigationinsideprovider-signup-page.tsxis byte-identical torequiresFullPageNavigationexported fromlib/account/login-flow.ts. Import the existing one — keeps the allowlist (http,/accounts/,/v1/,/_allauth/) in a single place so future server-side prefixes don't have to be added twice. -
Color/typography tokens in
components/account/*.account-shell.tsxusesbg-[#0d0d0d]andaccount-form.tsxusestext-white,text-stone-{300,400,500},bg-white/5,border-white/10,border-forest-600/50, plustext-sm/font-mediumcombos. Peranti-patterns-web.md, these should use semantic tokens (--surface-base,--content-default,--content-secondary,--border-base) and<Typography>/text-body-*utilities. The login card itself (login-page.tsx) already uses tokens correctly — the account-shell pages just inherited the platform's older styling. Not a port-blocker; worth a follow-up sweep so the dark account screen reads fromappTheme.csslike the rest of the app. -
Dead code check.
parseOAuthCompleteDeepLinkis exported fromnative-deep-link.tsbut I don't see a caller in this PR. It's almost certainly wired up via aCapacitorApp.addListener('appUrlOpen', …)somewhere — just confirm it's actually registered (or land that wiring in the next PR) so iOS deep-link completion doesn't silently no-op. -
isNativeFlowre-run sensitivity.oauth-popup-complete-page.tsx's effect depends on all URL params plusisNativeFlow. In practice these are stable for the popup's lifetime, but if a future change parameterizes them through React state, you'd end up callingwindow.close()twice. Worth a tiny mount-once guard if the file grows.
Anti-patterns / cross-platform check
- Anti-patterns KB: only #1 above lands. No
globalThis.fetchspies, no barrel files added, no Context+useReducer for cross-component state, nouseEffectwithout cleanup for subscriptions, noMutableRefObjectin deps, nodocument.querySelectorin render. - iOS/Capacitor: cookie domain hard-coded to
.vellum.aiis documented and intentional.installSessionCookiessets bothsessionidand__Secure-sessionid— works in dev and prod. Notarget="_blank"orwindow.openregressions for the WKWebView; OAuth popups route through@capacitor/browserviaopenUrl.
Merge gate
- CI: 3/3 green (Socket Security PR + Project + Lint/Type Check/Build).
- Reviewed at
cb8e7594. - Codex still at 👀; once it lands you'll have the second approval.
mergeable_state: blockedis the branch protection's "needs an approving review" not a CI/conflict issue.
Vellum Constitution — Trust-seeking: the unhandled-rejection paths above are the only failure modes that would silently swallow auth errors. Everything else surfaces explicit state (error UI, native callback with ?error=…, ApiError throws). Address those two effects and the auth surface is fully auditable.
|
Thanks for the thorough review. Addressing each finding: 1. Async effects miss try/catch — Agreed, this is a real bug. Both IIFEs in 2. Helper duplicated — Good catch. 3. Color/typography tokens — Acknowledged. These inherited the platform's older styling. Will defer to a follow-up sweep since the login card already uses tokens correctly. 4. Dead code check — 5. Pushing fixes for items 1 and 2 shortly. |
There was a problem hiding this comment.
🚩 New utility modules (profile.ts, handle-heuristics.ts) are unused — migration scaffolding
Several newly added modules are exported but never imported anywhere in the codebase: fetchMe, updateMe, checkUsernameAvailability, USERNAME_ERROR_COPY in apps/web/src/lib/account/profile.ts, looksAutoGenerated in apps/web/src/lib/account/handle-heuristics.ts, and buildLoginRedirectUrl in apps/web/src/lib/account/login-flow.ts:7. Per AGENTS.md dead-code rules these should be removed, but CONTRIBUTING.md notes apps/web/ is an active migration target with code being incrementally ported. If these are intentional scaffolding for an imminent follow-up PR, consider noting that in the PR description. Otherwise they should be removed per repo conventions.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
These are intentional migration scaffolding for the next PR (identity/onboarding port). fetchMe, updateMe, checkUsernameAvailability, and looksAutoGenerated will be consumed by the onboarding and identity pages. buildLoginRedirectUrl will be used by the auth gate (AppOrganizationGate equivalent). Will note this in the PR description.
There was a problem hiding this comment.
Resolved in 3184a33. Removed all three dead code items:
profile.ts— deleted (zero importers)handle-heuristics.ts— deleted (zero importers)buildLoginRedirectUrlinlogin-flow.ts— removed (exported but never imported)
These were originally intended as scaffolding for the identity/onboarding port, but per AGENTS.md dead code rules they shouldn't exist until they have consumers. They'll be re-added when the onboarding pages are ported.
| import { sanitizeReturnTo } from "@/lib/account/return-to.js"; | ||
|
|
||
| export const PROVIDER_ID = "workos-oidc"; | ||
| export const PROVIDER_CALLBACK_URL = routes.account.providerCallback; |
There was a problem hiding this comment.
🔴 OAuth callback URL missing /assistant basename prefix, breaking web login flow
PROVIDER_CALLBACK_URL is set to routes.account.providerCallback which is /account/provider/callback — a React Router path relative to the basename. However, this value is passed through startAuthFlow → startProviderRedirect → buildProviderRedirectFields (social-auth.ts:56) where it's resolved to an absolute URL via new URL(callbackUrl, origin).href, producing https://app.vellum.ai/account/provider/callback. The actual React Router route is registered at account/provider/callback under basename: "/assistant" (routes.tsx:67), so the real browser URL is https://app.vellum.ai/assistant/account/provider/callback. After OAuth completes, allauth redirects the user to the wrong URL (missing the /assistant prefix), so the ProviderCallbackPage is never reached. This breaks the entire web OAuth login flow for all three entry points: account-page.tsx:36, login-page.tsx:122-136, and signup-page.tsx:29.
| export const PROVIDER_CALLBACK_URL = routes.account.providerCallback; | |
| export const PROVIDER_CALLBACK_URL = `${BASENAME}${routes.account.providerCallback}`; |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — this is a real bug. routes.account.providerCallback is /account/provider/callback (relative to the React Router basename), but buildProviderRedirectFields in social-auth.ts:56 resolves it via new URL(callbackUrl, origin).href which produces the full browser URL without the /assistant prefix. After OAuth, allauth would redirect to the wrong URL. Fixing by prepending BASENAME.
There was a problem hiding this comment.
Resolved in 1d8b3dc. PROVIDER_CALLBACK_URL now uses `${BASENAME}${routes.account.providerCallback}` as suggested — produces /assistant/account/provider/callback which matches the actual browser route.
| const fallback = routes.root; | ||
| const { destination, requiresFullPageNavigation } = | ||
| resolvePostLoginDestination(returnTo, fallback); | ||
| if (requiresFullPageNavigation) { | ||
| window.location.href = destination; | ||
| } else { | ||
| navigate(destination, { replace: true }); |
There was a problem hiding this comment.
🚩 returnTo values passed to navigate() may double-prefix with basename
In provider-callback-page.tsx:97-103 and provider-signup-page.tsx:77-95, returnTo from query params is sanitized then passed to React Router's navigate(). If returnTo contains a full browser path (e.g. /assistant/home set by Django), navigate("/assistant/home") with basename /assistant would produce browser URL /assistant/assistant/home. This depends on what external systems put in returnTo — if it's always a React Router-relative path, this is fine. But if Django or other server-side redirects set full browser paths, it would break. The resolvePostLoginDestination helper at login-flow.ts:32-43 could strip the basename before returning paths intended for navigate(), similar to how getSameOriginRoutePath in oauth-popup-links.ts:65-67 already does this.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid concern. sanitizeReturnTo passes through any value starting with /, so if Django sets returnTo=/assistant/home, then navigate("/assistant/home") with basename /assistant would navigate to /assistant/assistant/home. resolvePostLoginDestination should strip the BASENAME prefix before returning destinations intended for navigate(). Fixing alongside the PROVIDER_CALLBACK_URL bug.
There was a problem hiding this comment.
Resolved in 1d8b3dc. resolvePostLoginDestination now strips BASENAME from the sanitized returnTo before returning paths intended for navigate(). Full-page navigation paths (window.location.href) keep the original value since they need the full browser path.
- PROVIDER_CALLBACK_URL now includes BASENAME so OAuth redirects resolve to the correct browser path (/assistant/account/provider/callback) - resolvePostLoginDestination strips BASENAME from returnTo before passing to navigate() to prevent double-prefix - Add try/catch to async effects in ProviderCallbackPage and ProviderSignupPage for unhandled promise rejections - Deduplicate shouldUseFullPageNavigation: use resolvePostLoginDestination from login-flow.ts instead of local copy in provider-signup-page Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
1d8b3dc
|
All findings from vex-assistant-bot addressed in 1. Async effects try/catch — Fixed. Both 2. Helper duplicated — Fixed. Removed 3. Color/typography tokens — Acknowledged as non-blocking follow-up. These inherited the platform's older styling. 4. Dead code ( 5. Additionally, this commit fixes the PROVIDER_CALLBACK_URL basename prefix bug (Devin Review finding) — now includes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb8e7594d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| import { sanitizeReturnTo } from "@/lib/account/return-to.js"; | ||
|
|
||
| export const PROVIDER_ID = "workos-oidc"; | ||
| export const PROVIDER_CALLBACK_URL = routes.account.providerCallback; |
There was a problem hiding this comment.
Prefix provider callback URL with router basename
PROVIDER_CALLBACK_URL is built as /account/provider/callback, but this app is mounted under /assistant (basename/Vite base). startProviderRedirect() turns this into an absolute callback URL, so OAuth redirects return to /account/provider/callback instead of /assistant/account/provider/callback, and the SPA callback route is never reached in the configured deployment path. Build callback URLs with BASENAME (or otherwise include the mount prefix) before handing them to the backend redirect flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in 1d8b3dc. PROVIDER_CALLBACK_URL now uses `${BASENAME}${routes.account.providerCallback}` — produces /assistant/account/provider/callback.
…te paths
Route refactoring to match the platform's original route structure:
- Remove React Router basename (/assistant) — routes are now absolute
browser paths matching the platform exactly:
/account/login, /account/signup, /account/provider/callback, etc.
/assistant, /assistant/home, /assistant/settings/*, etc.
- Revert utils/routes.ts to absolute paths matching the platform's
lib/routes.ts (routes.assistant = '/assistant', routes.home =
'/assistant/home', etc.). Remove BASENAME export.
- Fix auth behavior regressions introduced during port:
- PROVIDER_CALLBACK_URL: revert to routes.account.providerCallback
(was incorrectly prefixed with BASENAME)
- resolvePostLoginDestination: revert to platform behavior (no
BASENAME stripping)
- DEFAULT_POST_AUTH_DESTINATION: use routes.assistant (not BASENAME)
- oauth-popup-links getSameOriginRoutePath: return pathname as-is
(no BASENAME stripping), matching platform exactly
- Update route constants in chat-layout, side-menu, not-found to use
routes.* instead of hardcoded relative paths
- Restructure routes.tsx: /account/* and /assistant/* as separate
top-level route branches with no basename
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
… buildLoginRedirectUrl Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
Update on all BASENAME-related threads (comments 6-8, 9-11, 15-16): These threads flagged issues with
All three BASENAME-related concerns are now moot because the architecture no longer uses |
Test Results — Auth Pages at Corrected RoutesRan dev server locally on PR branch, navigated to each auth page at Auth page rendering & route wiring
Error states
OAuth popup complete
Observations
Key screenshots
|
There was a problem hiding this comment.
✦ APPROVE (re-review at 3184a330 — prior approval dismissed after 3 new commits)
Value: Unlocks OAuth flows that were silently broken — without this, WorkOS redirect-backs would land on /account/provider/callback instead of /assistant/account/provider/callback (with old basename), and auth completions from the native iOS SFSafariViewController flow would misroute. Every login path now works correctly across web, OAuth popup, and Capacitor native.
What this does (full picture): Removes basename: "/assistant" from createBrowserRouter() AND changes vite.config.ts base from /assistant to /. Route constants in utils/routes.ts are now absolute browser paths (/account/*, /assistant/*). All nav calls updated accordingly. Auth pages (Login, Signup, ProviderCallback, ProviderSignup, OAuthPopupComplete, AccountPage) landed in the new repo and wired into the router. Dead code removed (profile.ts, handle-heuristics.ts, buildLoginRedirectUrl).
On Codex's P1 (PROVIDER_CALLBACK_URL missing /assistant prefix): Stale analysis — verified at HEAD. With basename removed and Vite base: "/", routes.account.providerCallback = "/account/provider/callback" IS the correct browser path. No prefix needed. Devin's confirmation is accurate. PROVIDER_CALLBACK_URL = routes.account.providerCallback is correct as written.
resolvePostLoginDestination without BASENAME stripping: Correct at HEAD. With no router basename, navigate("/assistant/home") works as-is — Django returnTo values like /assistant/home are absolute SPA paths that the router now handles natively. requiresFullPageNavigation correctly routes /accounts/*, /_allauth/*, /v1/*, and http* through window.location.href. ✅
native-deep-link.ts security check: Allowlist (NATIVE_URL_SCHEME_BY_HOST) maps only known vellum.ai domains to native URL schemes. parseOAuthCompleteDeepLink validates protocol against ALLOWED_NATIVE_URL_PROTOCOLS and host against OAUTH_COMPLETE_DEEP_LINK_HOST before trusting any payload. requestId is required. No open-redirect risk. ✅
Route architecture: /account/* sits outside RootLayout (correct — no app chrome for auth pages). /assistant/* is wrapped in RootLayout → ChatLayout. Two catch-alls: one inside /assistant/* and one at root. Clean.
CI: 3/3 checks green (Socket, Lint+TypeCheck+Build). Same as prior approval.
Non-blocking notes (carried from cb8e7594 — new commits did not address):
-
Async IIFEs in
provider-callback-page.tsxandprovider-signup-page.tsx— the(async () => {...})()patterns insideuseEffectneedtry { ... } catch { setFallbackError(...) }even with thedidRun.currentguard. The guard prevents double-execution but doesn't catch rejections from the async body itself. Per anti-patterns-web.md: both a catch AND a cancelled/run guard are required. Low risk in practice sincegetSession()is unlikely to throw unexpectedly, but flagging for completeness. -
shouldUseFullPageNavigationduplicated inprovider-signup-page.tsx— already imported asrequiresFullPageNavigationfromlogin-flow.ts. Remove the duplicate. -
Color tokens in
account-shell.tsx/account-form.tsx— using inline hex/Tailwind colors instead of semantic tokens. Technical debt, matches current platform state, not a blocker.
Vellum Constitution — Trust-seeking: every login path now resolves to its exact intended destination — no silent 404s, no auth loops, no broken OAuth callbacks.
|
Both non-blocking notes are already addressed at HEAD:
|
Prompt / plan
Port all six auth/identity pages from the platform repo as faithful copies (import path adjustments only), and fix the React Router routing architecture so browser URLs match the platform exactly.
What changed
Auth pages ported (faithful copies from platform)
Six pages from
web/src/app/account/in the platform, ported toapps/web/src/domains/account/pages/:app/account/login/(_LoginForm.tsx)domains/account/pages/login-page.tsxASWebAuthenticationSession)app/account/signup/page.tsxdomains/account/pages/signup-page.tsxstartAuthFlowwithintent: "signup"app/account/provider/callback/page.tsxdomains/account/pages/provider-callback-page.tsxapp/account/provider/signup/page.tsxdomains/account/pages/provider-signup-page.tsxapp/account/page.tsxdomains/account/pages/account-page.tsxapp/account/oauth/popup-complete/page.tsxdomains/account/pages/oauth-popup-complete-page.tsxSupporting files also ported:
lib/account/login-flow.ts,return-to.ts,social-auth.ts— auth flow helpersruntime/native-auth.ts(fromlib/native-auth.ts) — Capacitor iOS auth bridgeruntime/native-deep-link.ts,runtime/native-biometric.ts— native platform bridgescomponents/account/account-form.tsx,account-shell.tsx— shared account UIcomponents/icons/apple-logo.tsx,google-logo.tsx— provider button iconscomponents/native-splash.tsx— iOS splash screen wrapperlib/auth/allauth-client.ts(fromlib/account/allauth-fetch.ts) — allauth session APIlib/auth/csrf.ts— CSRF cookie managementlib/domains.ts— vellum.ai domain validation forsanitizeReturnToRoute architecture fix
The previous implementation used
createBrowserRouter({ basename: "/assistant" }), which incorrectly prefixed all routes — auth pages appeared at/assistant/account/logininstead of/account/login.Fix: Removed
basenameentirely. The router now runs at/with explicit top-level branches:/account/*— auth pages (no app chrome)/assistant/*— chat app withRootLayout→ChatLayoutThis matches the platform's route structure exactly, where
app/account/andapp/(app)/assistant/are separate route groups.Changes:
routes.tsx: replacedbasename: "/assistant"with explicit/accountand/assistantroute branchesvite.config.ts: changedbasefrom"/assistant"to"/"utils/routes.ts: all path values match platform'slib/routes.ts(verified side-by-side)BASENAMEexport (migration artifact)Dead code removed
Per AGENTS.md dead code removal rules:
profile.ts— zero importers (will be re-ported with onboarding pages)handle-heuristics.ts— zero importers (same)buildLoginRedirectUrlinlogin-flow.ts— exported but never imported (only used by admin/org gate in platform, which is not being migrated)Auth behavior verification
Side-by-side comparison of every ported file confirms identical behavior:
_LoginForm.tsx→WebLoginFormlogin-page.tsx→WebLoginForm_LoginForm.tsx→NativeLoginFormlogin-page.tsx→NativeLoginFormlib/csrf.tslib/auth/csrf.tsinstallSessionCookies()installSessionCookies()storeBiometricToken()storeBiometricToken()sanitizeReturnTo()sanitizeReturnTo()resolvePostLoginDestination()resolvePostLoginDestination()startProviderRedirect()startProviderRedirect()buildOAuthCompleteDeepLink()buildOAuthCompleteDeepLink()classifyCallbackFlows()classifyCallbackFlows()Only framework adaptation changes were made:
"use client"removed (not needed in Vite SPA)next/linkLinkwithhref→react-routerLinkwithtouseRouter()→useNavigate()useSearchParams()→const [searchParams] = useSearchParams()(React Router returns tuple)Routetype → plainstring<Suspense>wrappers removed (no SSR in Vite SPA)try/catchadded around the provider-callback async effect to prevent unhandled promise rejectionsWhy this is safe
utils/routes.tsmatch platform'slib/routes.tsexactlyTest plan
bunx tsc --noEmit— zero errorsbun run lint— zero errorsbun run build— zero errors/account/loginwith correct UI, signup link navigates without double-prefix, provider callback shows distinct error states, OAuth popup complete shows success/failure statesReferences
Link to Devin session: https://app.devin.ai/sessions/5c400920d2b745bc84401cd020820f22
Requested by: @ashleeradka