Uh oh!
There was an error while loading. Please reload this page.
Restore Google sign-in popup + add tutor Fast/Smart model toggle - #73
Merged
AndresL230 merged 3 commits intoMay 4, 2026
Merged
Conversation
Restores the popup sign-in flow that was reverted in 5669639 (commit "use top-level redirect for Google OAuth instead of popup"). That earlier revert was correct for the bug at the time -- COOP severs window.opener the moment the popup hops to a cross-origin URL (Railway, then Google), so postMessage from /auth/callback never reached the opener and the modal hung on "Waiting for Google...". This rebuild sidesteps the opener handle entirely. The opener generates a per-attempt popup_id, subscribes to BroadcastChannel("sapling_signin:<id>") *before* opening the window, and threads the id through the OAuth state blob. /api/auth/google/callback echoes the id back on the redirect to /auth/callback, which detects popup mode by the id's presence (not by window.opener, which COOP nulls), broadcasts the result on the same channel, then self-closes. BroadcastChannel is same-origin and survives the cross-origin nav, so COOP is harmless. Failure paths (google_not_configured, invalid_domain, not_approved) now also route through /auth/callback when popup_id is set, so the popup can broadcast the error and self-close instead of stranding the opener on /auth or /pending. Falls back to today's same-tab redirect when window.open returns null (pop-up blocker) or BroadcastChannel/crypto.randomUUID is unavailable. A 3-minute watchdog plus a Cancel link reset the modal if the user abandons the popup -- popup.closed isn't observable under COOP, so we can't poll for it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tutor chat was hardcoded to gemini-2.5-pro (MODEL_SMART) in start_session,
chat, and action -- great for reasoning quality, sluggish enough that
users felt blocked. Adds a per-user UI toggle in the chat TopBar so the
student decides when speed matters.
Backend: StartSessionBody / ChatBody / ActionBody accept optional
model_pref ("fast" | "smart"). New _resolve_tutor_model maps "fast" ->
MODEL_DEFAULT (gemini-2.5-flash), "smart" -> MODEL_SMART (gemini-2.5-pro),
unknown / missing -> fast. The default is fast on both sides so first-time
chats are snappy by default; users opt in to Smart when they want depth.
Frontend: new ModelToggle component (mirrors SharedContextToggle's
look-and-feel and localStorage-persisted hook pattern, key
sapling_model_pref). Mounted in the chat header next to the Class intel
toggle. modelPref is threaded through startSession, sendChat, and
learnAction.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds an optional per-request model preference ("fast" | "smart") propagated from frontend toggle through API to backend model resolver, and replaces opener-based OAuth popup messaging with a popup + BroadcastChannel flow plus server-side popup-aware OAuth state cookie handling and routing. ChangesModel Preference Selection System
Popup-Based OAuth Flow
sequenceDiagram
actor User
participant SignInModal as SignInModal (Main Page)
participant Popup as Popup Window (auth/callback)
participant GoogleOAuth as Google OAuth
participant Backend as Backend (/auth)
participant BroadcastCh as BroadcastChannel
User->>SignInModal: Click "Sign in with Google"
SignInModal->>SignInModal: Generate popup_id (crypto.randomUUID)
SignInModal->>Popup: Open popup to /auth/google?popup_id=...
Popup->>Backend: GET /auth/google?popup_id=...
Backend->>Backend: Store code_verifier + popup_id in signed cookie (nonce-only state)
Backend->>GoogleOAuth: Redirect to Google consent screen
GoogleOAuth->>Backend: Redirect to /auth/google/callback?code=...&state=...
Backend->>Backend: Decode cookie, verify state nonce, exchange code
Backend->>Popup: Redirect to /auth/callback?popup_id=... (final client page)
Popup->>Popup: Fetch session / finalize auth
Popup->>BroadcastCh: Post { type: 'sapling_signin', success: true, ... } on popup_id channel
Popup->>Popup: window.close()
SignInModal->>BroadcastCh: Listening on popup_id channel
BroadcastCh->>SignInModal: Deliver signin payload
SignInModal->>SignInModal: Update UI/session and navigate appropriately
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend | 3f99981 | Commit Preview URL Branch Preview URL | May 04 2026, 08:24 PM |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/routes/auth.py (1)
160-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute Google callback failures through
_fail_redirect.If the user cancels on Google's consent screen, Google returns
error=access_deniedinstead ofcode. Withcoderequired here, that becomes a 422; similarly,fetch_token()/userinfo()exceptions will 500. In both cases the popup never getsoauth_deniedorsignin_failed, so the new popup flow strands the user on an error page instead of closing cleanly.Possible fix
-@router.get("/google/callback")-def google_callback(code: str = Query(...), state: str = Query(None)):+@router.get("/google/callback")+def google_callback(+ code: str | None = Query(None),+ state: str | None = Query(None),+ error: str | None = Query(None),+): """Exchange auth code for tokens, validate `@bu.edu`, upsert user.""" state_data = _decode_state(state) if state else {} code_verifier = state_data.get("cv") popup_id = state_data.get("popup_id") @@ + if error == "access_denied":+ return _fail_redirect("oauth_denied")+ if not code:+ return _fail_redirect("signin_failed")+ flow = Flow.from_client_config(_google_client_config(), scopes=AUTH_SCOPES) flow.redirect_uri = GOOGLE_AUTH_REDIRECT_URI - flow.fetch_token(code=code, code_verifier=code_verifier)- creds = flow.credentials-- # Fetch user info from Google- service = build("oauth2", "v2", credentials=creds)- user_info = service.userinfo().get().execute()+ try:+ flow.fetch_token(code=code, code_verifier=code_verifier)+ creds = flow.credentials+ service = build("oauth2", "v2", credentials=creds)+ user_info = service.userinfo().get().execute()+ except Exception:+ return _fail_redirect("signin_failed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.py` around lines 160 - 185, The google_callback route should route all auth failures through _fail_redirect: check for an incoming 'error' query param (e.g. access_denied) at the start of google_callback and call _fail_redirect("oauth_denied") when present; wrap flow.fetch_token(...) and the Google userinfo call (service.userinfo().get().execute()) in try/except and on any exception call _fail_redirect("signin_failed") (or a more specific error code) instead of letting exceptions propagate; ensure any error paths used by Flow.from_client_config / flow.fetch_token and userinfo use _fail_redirect so popup flows receive the proper redirect (references: google_callback, _fail_redirect, Flow.from_client_config, flow.fetch_token, service.userinfo().get().execute()).
🧹 Nitpick comments (1)
frontend/src/lib/api.ts (1)
82-82: ⚡ Quick win
ModelPrefis declared twice — import from a single source
ModelPref = 'smart' | 'fast'is defined identically here (line 82) and infrontend/src/components/ModelToggle.tsx(line 5). TypeScript structural typing hides this today, but a future addition to one definition (e.g., a third tier) won't automatically update the other, causing silent divergence.Since
ModelPrefbelongs to the API contract, keep the definition inapi.tsand import it inModelToggle.tsx:♻️ Proposed fix
In
api.ts, keep line 82 as-is.In
frontend/src/components/ModelToggle.tsx:-export type ModelPref = "smart" | "fast";+export type { ModelPref } from "@/lib/api";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/api.ts` at line 82, Remove the duplicate ModelPref type declaration and import the canonical type from the API module: keep the export type ModelPref = 'smart' | 'fast' in api.ts and in the ModelToggle component (ModelToggle.tsx) delete its local ModelPref declaration and add an import of ModelPref from the API module, then update any references in the ModelToggle component to use the imported ModelPref type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/models/__init__.py`:
- Line 13: Update the misleading comment on model_pref to reflect that the
actual default is the "fast" model (gemini-2.5-flash) not "smart"; modify the
comment near the model_pref variable to state that valid values are "smart"
(gemini-2.5-pro) or "fast" (gemini-2.5-flash) and that None/unknown resolves to
the MODEL_DEFAULT (gemini-2.5-flash) via _resolve_tutor_model. Ensure the
wording matches the PR documentation ("default Fast") and references
MODEL_DEFAULT/_resolve_tutor_model behavior so readers aren’t misled.
In `@backend/routes/auth.py`:
- Around line 147-149: The state construction (state_payload with keys "action",
"cv" (code_verifier), and optional "popup_id") must be changed to include only a
server-bound nonce rather than the PKCE verifier; generate a strong random nonce
when initiating OAuth, store it server-side (e.g., in a session store or
HttpOnly cookie keyed to the browser session), set state_payload to {"n": nonce}
(omit cv and popup_id), and store the code_verifier and popup_id server-side
tied to that nonce; then update the OAuth callback handler (the code that reads
state and exchanges the code) to first validate the returned nonce against the
server-side store/cookie and only after a successful match retrieve the stored
code_verifier and popup_id to perform the token exchange and complete signin,
rejecting any callback with a missing/invalid nonce.
In `@frontend/src/components/ModelToggle.tsx`:
- Around line 41-56: The switch button's tooltip text is inaccessible because
the tooltip div is conditionally rendered and aria-describedby may point to a
non-existent element; update the button (the element using isSmart and onChange)
to include a static aria-label that combines the visible state and the
explanatory text (e.g., "Smart: stronger reasoning" vs "Fast: quicker replies")
so screen readers always receive the description, while leaving the visual
tooltip rendering unchanged for sighted users.
In `@frontend/src/components/SignInModal.tsx`:
- Around line 117-118: The SignInModal currently always appends popup_id to the
Google auth URL (const popupId = crypto.randomUUID(); const url =
`${API_URL}/api/auth/google?popup_id=...`) which breaks the same-tab fallback
when window.open is blocked; change the logic so that you only include popup_id
when the auth flow is actually launched in a popup. Concretely, construct two
URLs (or build the query conditionally): one with popup_id for the popup path
used by window.open, and one without popup_id for the same-tab fallback
navigation; ensure the code paths in the SignInModal component that call
window.open use the popup URL, while the fallback window.location.href (or
equivalent) uses the URL without popup_id, and apply the same conditional change
to the other occurrence noted (the code block around lines 151-157).
- Around line 123-143: The handler on channel.onmessage (inside the SignInModal
component) incorrectly requires data.name for a successful signin; change the
success condition to check only data.success and data.userId (i.e. if
(data.success && data.userId) ), and call setActiveUser(data.userId, data.name
|| "") so missing/empty display names are accepted; keep
cleanupPopupListeners(), setWaiting(false), confirmApproved(), the
router.replace("/dashboard") vs
sessionStorage.setItem("sapling_onboarding_pending", "1") logic, and onClose()
unchanged, and only call setLocalError when the success condition fails.
---
Outside diff comments:
In `@backend/routes/auth.py`:
- Around line 160-185: The google_callback route should route all auth failures
through _fail_redirect: check for an incoming 'error' query param (e.g.
access_denied) at the start of google_callback and call
_fail_redirect("oauth_denied") when present; wrap flow.fetch_token(...) and the
Google userinfo call (service.userinfo().get().execute()) in try/except and on
any exception call _fail_redirect("signin_failed") (or a more specific error
code) instead of letting exceptions propagate; ensure any error paths used by
Flow.from_client_config / flow.fetch_token and userinfo use _fail_redirect so
popup flows receive the proper redirect (references: google_callback,
_fail_redirect, Flow.from_client_config, flow.fetch_token,
service.userinfo().get().execute()).
---
Nitpick comments:
In `@frontend/src/lib/api.ts`:
- Line 82: Remove the duplicate ModelPref type declaration and import the
canonical type from the API module: keep the export type ModelPref = 'smart' |
'fast' in api.ts and in the ModelToggle component (ModelToggle.tsx) delete its
local ModelPref declaration and add an import of ModelPref from the API module,
then update any references in the ModelToggle component to use the imported
ModelPref type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 577d888d-5173-4b24-94a1-8cff9ad25fe1
📒 Files selected for processing (8)
backend/models/__init__.pybackend/routes/auth.pybackend/routes/learn.pyfrontend/src/app/auth/callback/page.tsxfrontend/src/components/ModelToggle.tsxfrontend/src/components/SignInModal.tsxfrontend/src/components/screens/Learn.tsxfrontend/src/lib/api.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Address PR #73 review feedback: - auth: bind state nonce to HttpOnly cookie carrying {nonce, cv, popup_id} (HMAC-signed via SESSION_SECRET); reject mismatched/missing state with invalid_state. Closes login-CSRF where attacker-crafted state could log victim into attacker's account. - auth: wrap flow.fetch_token in try/except → oauth_exchange_failed so popup self-closes instead of returning 500 on replayed/expired codes. - auth: validate popup_id charset/length ([A-Za-z0-9_-]{1,128}); invalid input degrades to non-popup mode. - SignInModal: split popupUrl (with popup_id) from sameTabUrl (without) so popup-blocker fallback redirects locally instead of broadcasting into a dead channel. - SignInModal: don't require data.name on success — /auth/callback legitimately broadcasts empty name when /api/auth/me has no display. - models: tighten model_pref to Optional[Literal["fast","smart"]] so garbage input 422s; fix misleading default-comment. - ModelToggle: static aria-label so screen readers hear the description even when the visual tooltip is hidden. - tests: add 23 unit tests for auth state cookie/nonce/popup_id helpers and 6 for _resolve_tutor_model.
| import json | ||
| import base64 | ||
| import pytest |
Uh oh!
There was an error while loading. Please reload this page.
AndresL230
deleted the
fix/auth-popup-broadcast-channel-and-tutor-model-toggle
branch
May 4, 2026 20:25
Jose-Gael-Cruz-Lopez added a commit
that referenced
this pull request
May 5, 2026
Closes the asymmetry I flagged in the post-merge review: chat tutor got `model_pref: Literal["fast", "smart"]` per-request via PR #73, quiz had only env-var-driven model selection. Now both routes accept the same body field and route to the same model strings. Wiring - backend/models/__init__.py: GenerateQuizBody.model_pref accepts "fast" | "smart" | None. Default None falls through to the agent's task-default (model_for("quiz") = gemini-2.5-flash-lite per ADR 0008). - backend/routes/quiz.py: * New _resolve_model_pref(pref) builds an optional GoogleModel override from the body's preference, returning None for None/empty/unknown so the agent's default wins on degraded input. * _quiz_via_agent threads `model_pref` through and passes `model=...` to quiz_agent.run only when an override is present (no model kwarg = agent default). * _legacy_generate_quiz now picks MODEL_SMART when pref="smart", falls back to MODEL_LITE otherwise. So the fast/smart toggle works on BOTH paths, including when the agent path trips and we degrade. - _PREF_MODEL_NAMES table at module scope so the {fast→flash, smart→pro} mapping is one place. Mirrors the chat tutor's mapping in services/gemini_service.py (MODEL_DEFAULT/MODEL_SMART). Tests (TestQuizModelPref, 5 new) - model_pref="smart" → quiz_agent.run gets model=GoogleModel('gemini-2.5-pro') - model_pref="fast" → quiz_agent.run gets model=GoogleModel('gemini-2.5-flash') - model_pref=None → quiz_agent.run gets NO model kwarg (agent default wins) - model_pref="auto" → _resolve_model_pref returns None (graceful unknown) - legacy fallback with smart → call_gemini_json gets model=MODEL_SMART Documentation: ADR 0013 addendum captures the design decision (env var for ops defaults + body field for per-request overrides; they compose). Tests - tests/test_quiz_routes.py: 28/28 (was 23, +5). - Full backend suite: 531 passed, 3 pre-existing live-Supabase failures unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Two independent, complementary changes ship together on this branch.
1. Rebuild Google sign-in popup on
BroadcastChannelRestores the popup sign-in flow that was reverted in
5669639(fix(auth): use top-level redirect for Google OAuth instead of popup). That earlier revert was correct for the bug at the time — COOP severswindow.openerthe moment the popup hops to a cross-origin URL (Railway, then Google), sopostMessagefrom/auth/callbacknever reached the opener and the modal hung on "Waiting for Google…".This rebuild does not depend on
window.opener— so COOP severing the opener handle is harmless.How it works:
SignInModal.tsx) generates a per-attemptpopup_id(crypto.randomUUID) and subscribes toBroadcastChannel("sapling_signin:<id>")before opening the window.stateblob (backend/routes/auth.py::google_login) so the backend can echo it back./api/auth/google/callbackdecodespopup_idfromstateand includes it on the redirect to/auth/callback./auth/callback(frontend/src/app/auth/callback/page.tsx) detects popup mode by thepopup_idquery (notwindow.opener, which COOP nulls), broadcasts the result on the same channel, thenwindow.close()s.BroadcastChannelis same-origin, so the cross-origin nav through Google is irrelevant.Failure paths also broadcast.
google_not_configured,invalid_domain, andnot_approvednow route through/auth/callback?error=...&popup_id=...whenpopup_idis set, so the popup can broadcast the error and self-close instead of stranding the opener on/auth(which doesn't exist post-modal-refactor) or/pending. Same-tab behavior is unchanged.Fallbacks / safety:
window.openreturnsnull(pop-up blocker) orBroadcastChannel/crypto.randomUUIDare unavailable → falls back to today's full same-tab redirect (no regression).popup.closedisn't reliably observable under COOP, so we can't poll for it.2. Tutor chat Fast/Smart model toggle (default Fast)
Tutor chat was hardcoded to
gemini-2.5-pro(MODEL_SMART) instart_session,chat, andaction(commit6f431d6). Reasoning quality is great; latency is enough to feel blocked. Adds a per-user toggle in the chat header so the student decides when speed matters.Backend (
backend/routes/learn.py,backend/models/__init__.py):StartSessionBody/ChatBody/ActionBodyaccept an optionalmodel_pref: "fast" | "smart"._resolve_tutor_modelmaps"fast"→MODEL_DEFAULT(gemini-2.5-flash),"smart"→MODEL_SMART(gemini-2.5-pro). Unknown / missing → fast.start_session,chat,action) routed through the resolver.Frontend (
frontend/src/components/ModelToggle.tsx(new),screens/Learn.tsx,lib/api.ts):ModelTogglecomponent mirrorsSharedContextToggle's look-and-feel andlocalStorage-persisted hook pattern (sapling_model_prefkey).useModelPrefdefaults to"fast"on first load; persists user's choice across reloads.TopBarbetween the AI disclaimer chip and Class intel toggle.modelPrefthreaded throughstartSession,sendChat, andlearnAction.Defaults are airtight on both sides. A fresh user opening their first chat:
useState<ModelPref>("fast")is the initial render value, so the firststartSessioncall explicitly sendsmodel_pref: "fast". Even if that field were ever omitted, the backend resolver still returnsMODEL_DEFAULT.Test plan
Auth popup
/dashboard(or onboarding if not completed).@bu.eduaccount → popup self-closes, modal shows "Sign-in is limited to approved school accounts" (instead of stranding the popup on/auth).@bu.eduaccount → popup self-closes, modal shows "Your account is pending approval" (instead of leaving popup on/pending).popup_id) still works end-to-end if anyone hits/api/auth/googledirectly.Tutor model toggle
/learnas a fresh user (clearlocalStoragefirst) → toggle reads "Fast" (un-highlighted).POST /api/learn/start-sessionbody includesmodel_pref: "fast"(Network tab).gemini-2.5-flashwas used.POST /api/learn/chatbody includesmodel_pref: "smart", backend usesgemini-2.5-pro._resolve_tutor_model(None | "" | "fast" | "garbage")→MODEL_DEFAULT;_resolve_tutor_model("smart")→MODEL_SMART.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests