Repository navigation
⏳ fix: Restore the Sign-In Card Width and Keep SSO Buttons Busy Until the Provider Loads - #196
Merged
Merged
Conversation
… and Keep Deep Links Through SSO
The sign-in page offers one button per SSO provider LibreChat has for admins,
from a registry in src/constants (SSO_PROVIDERS: OpenID, Google, SAML).
ssoProvidersFn replaces checkOpenIdFn and returns { providers, ssoOnly }.
OpenID still needs its /api/admin/oauth/openid/check probe. Google and SAML
come from one GET /api/config sent with the request's tenant, and count only
with socialLoginEnabled, since LibreChat registers their admin strategies under
ALLOW_SOCIAL_LOGIN alone. A provider left out of socialLogins is not offered,
as on LibreChat's own sign-in page. ADMIN_SSO_ENABLED=false still offers none.
OpenID and SAML buttons take the deployer's label from /api/config.
One POST ssoStartFn ({ provider, redirectTo }) builds the PKCE challenge and
keeps the verifier and the sanitized in-app path (toSafeRedirect) in the admin
session, instead of the redirectTo query LibreChat strips. oauthExchangeFn takes
{ code, provider }, stores the provider as tokenProvider, and returns and then
forgets the path. One /auth/$provider/callback route replaces the OpenID-only
one: an unknown provider is not found, a failure shows SsoError, and a sign-in
returns to the page it started from. Both callback paths are known metrics
routes.
Google sessions refresh through POST /api/admin/oauth/refresh with
provider: 'google' and keep the rotated refresh token. SAML sessions, which
LibreChat cannot renew, are never sent to the cookie-based /api/auth/refresh.
ADMIN_SSO_ONLY auto-redirects only when exactly one provider is offered, hides
the password form while any is, and brings the form back when a provider cannot
start or none resolves.
Supersedes #36 by @dustinhealy, reusing its provider registry, its
/api/config-based availability with the social-login and socialLogins gates,
and its idea of carrying the deep link through the admin session.
Closes #30
Closes #65
…w the socialLogins Order
… the Provider Loads
click-ui 0.9.1's unlayered `.panel { width: var(--panel-width, auto) }`
outranks Tailwind's layered `w-full`, so the sign-in card and the SSO-only
redirect card took their content's width: 304px and 311px at 1440 instead of
max-w-md's 448px. Both Panels now take Panel's `fillWidth` (an inline
`--panel-width: 100%`) and keep max-w-md.
startSso reset its busy state in a `finally` that ran right after
`window.location.href = authUrl`, so the button went back to its idle label,
enabled, while the browser was still loading the provider, and could be
clicked again. Only a start that fails gives the button back now: an error
result, a request that throws, or a result with an empty URL (assigning it
would reload /login). The button takes click-ui's `loading` (shimmer,
disabled, aria-busy); the other providers stay disabled.
On an SSO-only page with one provider, that state starts as the provider, so
the redirect card shows from the first render and ends the same way. A
`pageshow` with `persisted` (the browser restoring the page from
its back-forward cache, Back from the provider) clears it: the button is idle
again, and an SSO-only page offers its provider's button instead of a spinner
that never ends.
Supersedes #123 by @dustinhealy, whose fillWidth, loading and pageshow
approach this follows. Depends on #188.
Contributor
Author
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
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.
Pull Request Template
Summary
This PR supersedes #123 by @dustinhealy and redoes it on top of #188. It follows #123's approach: Panel's
fillWidthfor the card width, click-ui'sloadingon the SSO button, a busy state that ends only when a start fails, and apageshowreset for back-forward cache restores. #123 no longer applies as it stands.mainalready has its click-ui 0.9.1 bump and Vitest config, and #188 replaces the single OpenID button it edits with one button per provider, all started bystartSso.Depends on #188. It edits the
AuthCardthat #188 rewrites, so its branch is based on #188's. Merge #188 first. This PR's own change is the single commit on top.The sign-in page had two problems:
.panel { width: var(--panel-width, auto) }outranks Tailwind's layeredw-full. Both cards took their content's width. The sign-in card measured 304px at 1440 and 264px at 375, against max-w-md's 448px. The SSO-only redirect card measured 311px and 271px.startSsoreset its busy state in afinallythat runs right afterwindow.location.href = authUrl. Assigninghrefdoesn't wait for the navigation. With LibreChat's start route held for 3 s, the button went back to "Continue with Google", enabled, 1 ms afterbeforeunload, and stayed that way while the provider loaded, so a second click was possible. It never had a spinner oraria-busyeither: it useddisabled, notloading.Changes (
src/components/AuthCard.tsx)fillWidth, which click-ui applies as an inline--panel-width: 100%, in place of thew-fullthat lost, and keepmax-w-md. That alone stops the collapse to the content's width: the card spans the page's centered column, up to 448px. A short JSDoc on the class constant says whyw-fulldoesn't work there.finallyis gone. A newfailSsoclears the busy state, brings back the password form and shows the error. It runs on an error result, when the request throws, and when the result's URL is empty.ssoStartFnalways returns a URL today; the check keeps an empty one from reloading/login. On success, the clicked provider's button stays busy until the provider's page replaces the panel.aria-busy: the clicked button takesloading={ssoStarting === id}. click-ui then adds its shimmer,disabledandaria-busy="true", and keeps the "Redirecting…" label. The other providers stay disabled, as before.ADMIN_SSO_ONLYand one provider, the busy state starts as that provider, so the redirect card shows from the first render. It shows while that provider is starting and hasn't failed, so the auto-redirect uses the same state and the same error-only reset.pageshowlistener clears the busy state whenevent.persistedis true, meaning the browser restored the page from its back-forward cache. The button is idle again. On an SSO-only page, the card offers its provider's button instead of the redirect spinner, which would otherwise spin forever because the auto-redirect has already run.Not carried over from #123
server.deps.inline/.claude/**config.mainalready has both.vi.mock('@/hooks')with a key-echoinguseLocalize, and the stubbedwindow.location. The new tests use the existingAuthCard.test.tsxsetup: realuseLocalizewithlocalizeEn, real click-ui, andauthUrls that are hashes, so jsdom follows them.min-w-70. WithfillWidththe card already spans the page's column, so a min-width only takes effect when it is wider than that column. Its fixed 280px would then push the card plus the page's 16px gutters past a 280px-wide screen.Unchanged
/loginin its back-forward cache. The page is servedCache-Control: no-store, and the start sets a session cookie. Back from the provider reloads the page, so the button is idle. On an SSO-only page the reload redirects to the provider again, as it does onmain. Thepageshowreset covers browsers that do restore the page.Change Type
Testing
Unit:
src/components/__tests__/AuthCard.test.tsxrenders the realAuthCardwith real click-ui.ssoStartFnis mocked.aria-busy="true"and disabled, and the other provider is disabled withoutaria-busy.aria-busy, enabled). On an SSO-only page with two providers, the empty URL also brings back the hidden password form.pageshowwithpersisted: falseleaves the button busy, and one withpersisted: truemakes it idle with no error.pageshowreplaces it with an idle provider button, with no password form and no second start.--panel-width: 100%andmax-w-md.Against #188's
AuthCard, 7 of the 11 new tests fail. The three failure tests pass on both, because #188 also resets on failure; they fail iffailSsostops clearing the busy state. The first-render test also passes on #188, whose redirect card doesn't depend on the busy state; it fails here if the busy state starts empty. I also removed each fix in turn to check which tests catch it. Without thepageshowlistener, the two back-forward cache tests fail. Withoutloading, the pending, sent and back-forward cache tests fail. Without the empty-URL check, its test fails. WithoutfillWidth, both width tests fail. If the redirect card ignores the busy state, the SSO-only back-forward cache test fails.E2E (
e2e/login.spec.ts, against the mock backend):beforeunload. Playwright locators andevaluatewait for a pending navigation to finish, so the page logs to the console instead. The test expects "Redirecting…" busy and disabled, OpenID disabled and Sign in untouched, in any order, so it holds whichever order the mock lists its providers in. It then releases the route and checks that the provider's page loads.All three tests fail against #188's
AuthCard: the widths are 304 and 264, and the Google button is "Continue with Google", enabled, withoutaria-busy. With this change the wholelogin.spec.tspasses against the mock backend (36 tests).Live check: against a LibreChat backend with Google admin sign-in (placeholder credentials, the provider page stubbed), and the production build, in Chromium.
aria-busy, then "Continue with Google" enabled 1 ms afterbeforeunloadaria-busyand disabled from the click until the provider page loadspageshowmakes it idleLight and dark at 1440 look the same apart from the theme.
Test Configuration:
devbackend withALLOW_SOCIAL_LOGIN=trueand Google client credentials, MongoDB 8.2 replica set. For the SSO-only rows,ADMIN_SSO_ONLY=trueon the panel.bun run build+bun server.ts), Chromium via Playwright.npx playwright test e2e/login.spec.tsagainst the mock backend.npx tsc --noEmit,npx eslint src/ e2e/ --max-warnings 0,npx prettier --checkon the changed files,bun run test:lint-rules,npx vitest run(154 files, 3402 tests),bun run build.Checklist