Skip to content

Added animations and an official onboarding process for new users.#57

Closed
Darkest-Teddy wants to merge 4 commits into
mainfrom
onboarding
Closed

Added animations and an official onboarding process for new users.#57
Darkest-Teddy wants to merge 4 commits into
mainfrom
onboarding

Conversation

@Darkest-Teddy

@Darkest-Teddy Darkest-Teddy commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator

Description

Adds a guided onboarding experience for new users starting from the landing page and Google sign-in flow. This also updates auth routing so new users resume onboarding after OAuth, while existing users continue directly to the dashboard, and adds safer fallback behavior for calendar and graph data loading.

Changes Made

  • Added a new animated multi-step onboarding flow covering account creation, name, school/year, academics, courses, and learning style.
  • Updated the landing page to launch onboarding directly, including new intro/outro transitions and onboarding-specific visual effects.
  • Refined Google OAuth handling with frontend proxy routes, PKCE/state support, and new-user detection so onboarding can continue after authentication.
  • Added more defensive fallback responses in calendar and graph routes to avoid breaking the UI when backend data fetches fail.
  • Updated supporting UI/details including onboarding animations, Closed Beta branding in the navbar, and a new linkedin-banner.html asset.

Related Issues

Closes #

Testing

  • Tested locally
  • Added/updated tests

Notes for Reviewers

  • Google sign-in now accepts .edu emails instead of only @bu.edu.
  • New users are flagged through the auth callback and returned to onboarding via sessionStorage.
  • School lookup is broadly searchable, while major/minor suggestions are currently populated from BU-specific lists.

Summary by CodeRabbit

  • New Features

    • Full multi-step onboarding modal and integrated landing-page onboarding flow with animated knowledge-graph canvas.
    • Client-side onboarding component and onboarding-driven routing for new users (is_new flag).
    • Downloadable LinkedIn banner generator page.
  • Bug Fixes

    • Broader error handling across auth, calendar, graph, and recommendation endpoints with safer fallbacks and clearer auth error redirects.
    • Email domain validation expanded to accept .edu addresses.
  • Style

    • Branding label updated to “Closed Beta”; refined shimmer and onboarding animations.
  • Tests

    • New tests covering calendar and graph error handling.

@coderabbitai

coderabbitai Bot commented Apr 15, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Refactors backend Google OAuth callback and user upsert with broader try/except logging; relaxes email domain check to .edu; hardens calendar and graph endpoints to return normalized fallbacks on error; adds frontend static fallback routes for Google auth; implements a client onboarding flow and OnboardingFlow component with landing-page integration, CSS animations, and a static LinkedIn banner.

Changes

Cohort / File(s) Summary
Backend: Auth
backend/routes/auth.py
Reordered/helpers for PKCE/state, simplified encode/decode, removed action from state, wrapped /google/callback in try/except with traceback logging, changed invalid-google redirect target, relaxed email restriction to .edu, refactored user resolution/upsert (prefer google_id, then email, then computed_id), moved token upsert into try, added is_new query param on final redirect.
Backend: Robustness & Logging
backend/routes/calendar.py, backend/routes/graph.py
get_upcoming() now returns {"assignments": []} on exceptions and logs error; graph endpoints catch exceptions, log via module logger, and raise HTTP 500 with the exception message (normalized error responses).
Frontend: OAuth Static Fallbacks
frontend/src/app/api/auth/google/route.ts, frontend/src/app/api/auth/google/callback/route.ts
Added Next.js App Router API routes exporting dynamic='force-static' that return HTTP 410 plain-text messages indicating the callback/start should be handled by backend or frontend static flow.
Frontend: Onboarding UI & Landing
frontend/src/components/OnboardingFlow.tsx, frontend/src/app/page.tsx, frontend/src/app/signin/callback/page.tsx
Added OnboardingFlow component (multi-step wizard with validation, autocomplete, chips, and finish handler); landing page integrates onboarding state/animations (node clusters, zoom, timed spawns) and start/close flows; signin callback reads is_new to mark sessionStorage.sapling_onboarding_pending and route new users to onboarding.
Frontend: Styling & Small Edits
frontend/src/app/globals.css, frontend/src/components/Navbar.tsx, frontend/src/context/UserContext.tsx
Added onboarding keyframes (ob-pulse-outer, ob-pulse-inner, ob-card-in), tweaked liquid-glass::after shimmer gradient and transitions; changed Navbar label “Closed Alpha” → “Closed Beta”; switched UserProvider fetch from http://localhost:5000/api/users to relative /api/users.
Marketing Asset
linkedin-banner.html
Added a new static 1584×396 HTML canvas banner generator that deterministically renders nodes, connections, and layered gradients via a seeded PRNG in a single render pass.
Tests
backend/tests/test_calendar_routes.py, backend/tests/test_graph_routes.py
Added tests asserting calendar endpoint returns {"assignments":[]} on DB failure and graph endpoints return HTTP 500 with exception details when underlying graph functions raise.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Frontend
    participant NextAPI as Frontend API
    participant Backend
    participant DB

    User->>Frontend: Click "Get Started" / open Onboarding
    Frontend->>Frontend: startOnboarding() -> show OnboardingFlow
    User->>Frontend: Complete onboarding -> request Google sign-in
    Frontend->>NextAPI: GET /api/auth/google (static fallback route may 410)
    NextAPI->>Backend: Redirect/start OAuth (PKCE, state)
    User->>Backend: Google consent -> POST/GET callback
    Backend->>Backend: Exchange code, fetch userinfo
    Backend->>Backend: Validate email endswith .edu
    Backend->>DB: Lookup/Upsert user (google_id → email → computed_id) and upsert tokens
    Backend-->>NextAPI: Redirect to /signin/callback?is_new=...&user_id=...
    NextAPI-->>Frontend: Client receives redirect/params
    Frontend->>Frontend: signin callback processes is_new -> set sessionStorage or route to /dashboard
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐇 I hopped through nodes and code tonight,
Wove onboarding lights in gentle flight.
OAuth gates now ask for .edu,
New users bloom — session flags in view.
Canvas banners shimmer — hop, delight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: adding animations and an official onboarding process for new users, which aligns with the primary purpose of this PR.
Description check ✅ Passed The description covers all required template sections with detailed information about the onboarding feature, changes made, testing status, and reviewer notes, though Related Issues remains unfilled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch onboarding
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch onboarding

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread backend/routes/graph.py Fixed
Comment thread frontend/src/app/page.tsx
const API_URL = process.env.NEXT_PUBLIC_API_URL ?? 'http://localhost:5000';

// Cluster node base positions — orbit around center (0.285, 0.475), varied radii
const CLUSTER_REL = [
Comment thread frontend/src/app/page.tsx
];
const OB_SPREAD = 58;
const OB_COUNT = 15;
const easeOutBack = (x: number) => {
Comment thread frontend/src/components/OnboardingFlow.tsx Fixed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

4-6: ⚠️ Potential issue | 🟡 Minor

Docstring is outdated — now accepts any .edu domain.

The module docstring still says "Restricts sign-in to @bu.edu email accounts only" but the code now validates any .edu suffix (line 116).

📝 Proposed fix
 """
 backend/routes/auth.py

 Google OAuth sign-in with unified calendar access.
-Restricts sign-in to `@bu.edu` email accounts only.
+Restricts sign-in to .edu email accounts only.
 """
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/routes/auth.py` around lines 4 - 6, The module docstring is outdated:
it still states "Restricts sign-in to `@bu.edu` email accounts only" but the code
now validates any `.edu` domain (see the email validation logic around line 116
in backend/routes/auth.py). Update the top-of-file docstring to accurately
describe current behavior — e.g., "Google OAuth sign-in with unified calendar
access; restricts sign-in to any .edu email domain" — and remove the explicit
`@bu.edu` mention so the comment matches the email validation implemented in this
module.
🧹 Nitpick comments (6)
linkedin-banner.html (1)

37-46: Add a guard to validate palette weights.

A small invariant check will prevent accidental visual drift if weights are edited later.

Suggested fix
 const palette = [
   { c: '#8A63D2', w: 0.24 }, { c: '#3B82F6', w: 0.24 },
   { c: '#D97706', w: 0.2 },  { c: '#14B8A6', w: 0.15 },
   { c: '#9CA3AF', w: 0.1 },  { c: '#D1D5DB', w: 0.07 },
 ];
+const paletteWeightTotal = palette.reduce((sum, p) => sum + p.w, 0);
+if (Math.abs(paletteWeightTotal - 1) > 1e-6) {
+  throw new Error(`Palette weights must sum to 1.0 (got ${paletteWeightTotal})`);
+}
 function randColor() {
   let r = rand(), s = 0;
   for (const p of palette) { s += p.w; if (r <= s) return p.c; }
   return palette[0].c;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@linkedin-banner.html` around lines 37 - 46, The palette weight logic lacks
validation so editing weights can break randColor; add a guard that validates
palette entries (each p.w is a finite non-negative number) and that the total
weight is > 0 (or approximately 1) before using it in randColor. Implement this
check either at palette initialization or at the start of randColor (referencing
palette and randColor) and throw/log a clear error or normalize the weights if
the total is zero/invalid so the loop that accumulates s and compares r to s
cannot fail silently.
frontend/src/components/OnboardingFlow.tsx (2)

144-152: Silent error swallowing hides API failures.

The empty catch block silently discards any errors from the university search API. Consider logging or providing user feedback when the search fails.

♻️ Proposed improvement
       try {
         const res = await fetch(`http://universities.hipolabs.com/search?name=${encodeURIComponent(value)}&country=United+States`);
+        if (!res.ok) throw new Error('Search failed');
         const data: { name: string }[] = await res.json();
         setSchoolSuggestions(data.slice(0, 10).map(u => u.name));
       } catch {
         setSchoolSuggestions([]);
+        // Optionally: console.warn('[onboarding] University search failed');
       }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/OnboardingFlow.tsx` around lines 144 - 152, In
OnboardingFlow's debounce handler (the block assigning
schoolDebounceRef.current), don't swallow errors in the catch; capture the error
(e.g., catch (err)) and log it via console.error or process logger and still
call setSchoolSuggestions([]); additionally, surface user feedback by setting a
local error state or invoking the existing UI feedback mechanism (e.g., a
snackbar setter) so the user knows the university search failed—update the catch
in the schoolDebounceRef async callback to log the error and trigger
user-visible error state instead of leaving it empty.

102-110: Consider reusing FormData type in Props.

The onFinish callback signature duplicates the FormData interface fields. Using the type directly improves maintainability.

♻️ Proposed simplification
 interface Props {
   visible: boolean;
   onClose: () => void;
-  onFinish: (data: { firstName: string; lastName: string; school: string; year: string; majors: string[]; minors: string[]; courses: string[]; style: string }) => void;
+  onFinish: (data: FormData) => void;
   activeStep: number;
   completed: Set<number>;
   setActiveStep: (s: number) => void;
   setCompleted: (s: Set<number>) => void;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/OnboardingFlow.tsx` around lines 102 - 110, The Props
interface duplicates the FormData shape in the onFinish callback; change the
onFinish signature to use the existing FormData type (e.g., onFinish: (data:
FormData) => void) instead of an inline object, ensure FormData is imported or
defined in this module, and update any callers/implementations of
OnboardingFlow.onFinish (and the component prop usage) to pass a FormData
instance so types remain consistent with the FormData interface.
backend/routes/auth.py (1)

185-188: Broad exception handler obscures root cause.

Catching Exception (flagged by Ruff BLE001) masks specific failures. Consider catching more specific exceptions or at least logging the exception type for better debugging.

♻️ Proposed improvement
-    except Exception as e:
+    except (ValueError, KeyError) as e:
+        traceback.print_exc()
+        print(f"[auth] FAILED (validation): {type(e).__name__}: {e}")
+        return RedirectResponse(f"{FRONTEND_URL}/?error=auth_failed")
+    except Exception as e:
         traceback.print_exc()
-        print(f"[auth] FAILED at step above: {e}")
+        print(f"[auth] FAILED (unexpected {type(e).__name__}): {e}")
         return RedirectResponse(f"{FRONTEND_URL}/?error=auth_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 185 - 188, The current broad "except
Exception as e" in the auth error handler (the block using
traceback.print_exc(), print(f"[auth] FAILED at step above: {e}"), and returning
RedirectResponse) should be narrowed and better-logged: replace the broad except
with targeted handlers for expected failures (e.g., OAuthError / HTTPException /
ValueError or whatever auth-library exceptions are relevant in this module) and
add a fallback that logs the exception type and full traceback before re-raising
or returning the error redirect; if you cannot enumerate all expected exception
classes immediately, at minimum change the handler to log type(e).__name__ and
the full traceback via the project logger (instead of print) and then return the
RedirectResponse or re-raise unrecognized exceptions so root causes are not
silently masked.
frontend/src/app/page.tsx (1)

148-460: Canvas animation logic is substantial — consider extracting.

The canvas useEffect spans ~300 lines mixing background nodes, cluster nodes, and onboarding graph logic. While functional, extracting the OB graph spawning/drawing into a separate helper or custom hook would improve readability.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/app/page.tsx` around lines 148 - 460, The canvas useEffect is
too large and mixes background/cluster logic with the onboarding (OB) graph;
extract the OB graph spawn and render logic into a focused helper or custom
hook. Move the OB-specific state and functions (references to obNodesRef,
obInitStepsRef, obDoneStepsRef, OB_STEP_CENTERS, OB_SPREAD, OB_COUNT, stepRand,
easeInOutCubic, and the migration/arrival math) into a new module or hook (e.g.,
useObGraph or createObHelpers) that exposes spawnStepPreview(stepIndex),
finalizeStep(stepIndex), cleanupOnIdle(), and renderOb(ctx, params) (or
equivalent) so the main useEffect only calls those APIs inside draw; replace the
inlined spawning/drawing blocks in draw() with calls to those helpers and keep
the animation loop, resize, and non-OB projection logic in the current effect.
Ensure you preserve signatures that reference now, t, rotAngle, zoom, cx/cy,
parallaxYRef, and mouseRef so the extracted functions can compute positions
identically.
frontend/src/app/api/auth/google/callback/route.ts (1)

11-19: Consider handling fetch failures gracefully.

If the backend is unreachable, fetch() will throw and the route will return a 500 error. Wrapping in try/catch would provide a better user experience.

♻️ Proposed improvement
-  // Fetch without following redirects so we get the Location header
-  const response = await fetch(backendUrl.toString(), { redirect: 'manual' });
-
-  const location = response.headers.get('location');
-  if (location) {
-    return NextResponse.redirect(location);
-  }
-
-  return NextResponse.redirect(new URL('/?error=auth_failed', request.url));
+  try {
+    const response = await fetch(backendUrl.toString(), { redirect: 'manual' });
+    const location = response.headers.get('location');
+    if (location) {
+      return NextResponse.redirect(location);
+    }
+  } catch {
+    // Backend unreachable
+  }
+  return NextResponse.redirect(new URL('/?error=auth_failed', request.url));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/app/api/auth/google/callback/route.ts` around lines 11 - 19, The
fetch call to backendUrl (const response = await fetch(...)) can throw if the
backend is unreachable; wrap the fetch and subsequent header handling in a
try/catch around the block that calls fetch and reads
response.headers.get('location') so network errors are caught, and in the catch
redirect using NextResponse.redirect(new URL('/?error=auth_failed',
request.url)) (or a more specific error query like auth_unreachable) instead of
letting the route throw; ensure you still handle the normal location redirect
via NextResponse.redirect(location) when present.
🤖 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/routes/auth.py`:
- Line 108: The prints that log user_info.get('email') expose full PII; update
both print calls that reference user_info.get('email') to avoid logging the full
address—extract and log only the domain (text after '@') or a masked form (e.g.,
****@domain) instead. Locate the print statements that use the user_info
variable in the auth route (the two print calls around the OAuth/user-info
handling) and replace the direct email interpolation with logic that safely
derives domain = email.split('@')[-1] (or masks the local part) and then log
only that domain/masked value along with the existing context.

In `@backend/routes/calendar.py`:
- Around line 117-127: The endpoint in backend/routes/calendar.py swallows all
exceptions without logging; update the try/except around the
table("assignments").select call to import logging, use a module-level logger,
and call logger.exception(...) (including user_id and the query params like
today) inside the except before returning {"assignments": []}; then add a test
in backend/tests/test_calendar_routes.py (e.g.,
test_returns_empty_on_db_failure) that patches routes.calendar.table so select
raises Exception("DB connection error") and asserts the response is 200 with
{"assignments": []} to cover the exception path.

In `@backend/routes/graph.py`:
- Around line 16-22: The try/except around get_graph(user_id) currently swallows
all exceptions and returns a 200 empty graph; change it so you catch Exception
as e, log the exception (e.g., logger.exception or app.logger.exception with
context mentioning get_graph/user_id) and then raise an HTTPException
(HTTPException(status_code=500, detail=str(e) or a concise message) so failures
are observable and return 500 instead of a silent 200. Apply the same pattern to
the other identical block handling get_graph (lines referenced in the review).

In `@frontend/src/components/OnboardingFlow.tsx`:
- Line 146: The fetch call in OnboardingFlow.tsx using the expression
`fetch(\`http://universities.hipolabs.com/search?...\`)` is using an unencrypted
HTTP URL; change the URL to use HTTPS (i.e.,
`https://universities.hipolabs.com/...`) in the fetch invocation so the `res`
request is sent over TLS, ensuring encrypted transmission for the university
search request.

In `@linkedin-banner.html`:
- Line 2: The root HTML element is missing a lang attribute which hurts
accessibility; update the <html> tag to include an appropriate language code
(e.g., lang="en" or the document's primary language) so assistive technologies
and browsers can default correctly, and ensure the value matches the page
content.

---

Outside diff comments:
In `@backend/routes/auth.py`:
- Around line 4-6: The module docstring is outdated: it still states "Restricts
sign-in to `@bu.edu` email accounts only" but the code now validates any `.edu`
domain (see the email validation logic around line 116 in
backend/routes/auth.py). Update the top-of-file docstring to accurately describe
current behavior — e.g., "Google OAuth sign-in with unified calendar access;
restricts sign-in to any .edu email domain" — and remove the explicit `@bu.edu`
mention so the comment matches the email validation implemented in this module.

---

Nitpick comments:
In `@backend/routes/auth.py`:
- Around line 185-188: The current broad "except Exception as e" in the auth
error handler (the block using traceback.print_exc(), print(f"[auth] FAILED at
step above: {e}"), and returning RedirectResponse) should be narrowed and
better-logged: replace the broad except with targeted handlers for expected
failures (e.g., OAuthError / HTTPException / ValueError or whatever auth-library
exceptions are relevant in this module) and add a fallback that logs the
exception type and full traceback before re-raising or returning the error
redirect; if you cannot enumerate all expected exception classes immediately, at
minimum change the handler to log type(e).__name__ and the full traceback via
the project logger (instead of print) and then return the RedirectResponse or
re-raise unrecognized exceptions so root causes are not silently masked.

In `@frontend/src/app/api/auth/google/callback/route.ts`:
- Around line 11-19: The fetch call to backendUrl (const response = await
fetch(...)) can throw if the backend is unreachable; wrap the fetch and
subsequent header handling in a try/catch around the block that calls fetch and
reads response.headers.get('location') so network errors are caught, and in the
catch redirect using NextResponse.redirect(new URL('/?error=auth_failed',
request.url)) (or a more specific error query like auth_unreachable) instead of
letting the route throw; ensure you still handle the normal location redirect
via NextResponse.redirect(location) when present.

In `@frontend/src/app/page.tsx`:
- Around line 148-460: The canvas useEffect is too large and mixes
background/cluster logic with the onboarding (OB) graph; extract the OB graph
spawn and render logic into a focused helper or custom hook. Move the
OB-specific state and functions (references to obNodesRef, obInitStepsRef,
obDoneStepsRef, OB_STEP_CENTERS, OB_SPREAD, OB_COUNT, stepRand, easeInOutCubic,
and the migration/arrival math) into a new module or hook (e.g., useObGraph or
createObHelpers) that exposes spawnStepPreview(stepIndex),
finalizeStep(stepIndex), cleanupOnIdle(), and renderOb(ctx, params) (or
equivalent) so the main useEffect only calls those APIs inside draw; replace the
inlined spawning/drawing blocks in draw() with calls to those helpers and keep
the animation loop, resize, and non-OB projection logic in the current effect.
Ensure you preserve signatures that reference now, t, rotAngle, zoom, cx/cy,
parallaxYRef, and mouseRef so the extracted functions can compute positions
identically.

In `@frontend/src/components/OnboardingFlow.tsx`:
- Around line 144-152: In OnboardingFlow's debounce handler (the block assigning
schoolDebounceRef.current), don't swallow errors in the catch; capture the error
(e.g., catch (err)) and log it via console.error or process logger and still
call setSchoolSuggestions([]); additionally, surface user feedback by setting a
local error state or invoking the existing UI feedback mechanism (e.g., a
snackbar setter) so the user knows the university search failed—update the catch
in the schoolDebounceRef async callback to log the error and trigger
user-visible error state instead of leaving it empty.
- Around line 102-110: The Props interface duplicates the FormData shape in the
onFinish callback; change the onFinish signature to use the existing FormData
type (e.g., onFinish: (data: FormData) => void) instead of an inline object,
ensure FormData is imported or defined in this module, and update any
callers/implementations of OnboardingFlow.onFinish (and the component prop
usage) to pass a FormData instance so types remain consistent with the FormData
interface.

In `@linkedin-banner.html`:
- Around line 37-46: The palette weight logic lacks validation so editing
weights can break randColor; add a guard that validates palette entries (each
p.w is a finite non-negative number) and that the total weight is > 0 (or
approximately 1) before using it in randColor. Implement this check either at
palette initialization or at the start of randColor (referencing palette and
randColor) and throw/log a clear error or normalize the weights if the total is
zero/invalid so the loop that accumulates s and compares r to s cannot fail
silently.
🪄 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: 1530a8d0-26a4-4afe-bcd1-92a37cc74c59

📥 Commits

Reviewing files that changed from the base of the PR and between 50721f8 and 11781bc.

📒 Files selected for processing (12)
  • backend/routes/auth.py
  • backend/routes/calendar.py
  • backend/routes/graph.py
  • frontend/src/app/api/auth/google/callback/route.ts
  • frontend/src/app/api/auth/google/route.ts
  • frontend/src/app/globals.css
  • frontend/src/app/page.tsx
  • frontend/src/app/signin/callback/page.tsx
  • frontend/src/components/Navbar.tsx
  • frontend/src/components/OnboardingFlow.tsx
  • frontend/src/context/UserContext.tsx
  • linkedin-banner.html

Comment thread backend/routes/auth.py Outdated
Comment on lines +117 to +127
try:
today = datetime.utcnow().strftime("%Y-%m-%d")
rows = table("assignments").select(
"*",
filters={"user_id": f"eq.{user_id}", "due_date": f"gte.{today}"},
order="due_date.asc",
limit=20,
)
return {"assignments": rows or []}
except Exception:
return {"assignments": []}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify blind catches and missing telemetry in these route handlers.
rg -n -C2 'except Exception' backend/routes/calendar.py backend/routes/graph.py
rg -n 'logger\.(exception|error|warning)\(' backend/routes/calendar.py backend/routes/graph.py

Repository: SaplingLearn/Sapling

Length of output: 1279


🏁 Script executed:

# Find test files for calendar routes
find . -type f -name "*test*calendar*" -o -name "*calendar*test*" | head -20

Repository: SaplingLearn/Sapling

Length of output: 103


🏁 Script executed:

# Check if logging is configured in the backend
rg -n "import logging" backend/ | head -10
rg -n "^logger = " backend/ | head -10

Repository: SaplingLearn/Sapling

Length of output: 176


🏁 Script executed:

# Look for pytest/test configuration
fd -t f "(conftest|pytest)" backend/

Repository: SaplingLearn/Sapling

Length of output: 89


🏁 Script executed:

cat -n backend/tests/test_calendar_routes.py

Repository: SaplingLearn/Sapling

Length of output: 8933


🏁 Script executed:

# Check calendar.py to see the full context of the get_upcoming function
cat -n backend/routes/calendar.py | head -150

Repository: SaplingLearn/Sapling

Length of output: 5870


🏁 Script executed:

# Look at how documents.py handles exceptions (shows logging pattern)
rg -B5 -A5 'except Exception' backend/routes/documents.py

Repository: SaplingLearn/Sapling

Length of output: 922


Add logging and exception path test coverage to /upcoming/{user_id} endpoint.

Line 126–127 silently swallows all exceptions, hiding real database/service failures and making incident detection impossible. Tests also lack coverage for the exception path.

Specific issues
  1. Missing telemetry: Exceptions are not logged; only the empty fallback is returned, eliminating visibility into failures.
  2. Incomplete test coverage: TestGetUpcoming tests the success and empty-result cases but does not test the exception handler path (line 126–127 behavior).

The logging pattern is already established in backend/routes/documents.py (import logging, logger exception calls with context).

Suggested minimal fix
+import logging
+
+logger = logging.getLogger(__name__)
+
 `@router.get`("/upcoming/{user_id}")
 def get_upcoming(user_id: str):
     try:
         today = datetime.utcnow().strftime("%Y-%m-%d")
         rows = table("assignments").select(
             "*",
             filters={"user_id": f"eq.{user_id}", "due_date": f"gte.{today}"},
             order="due_date.asc",
             limit=20,
         )
         return {"assignments": rows or []}
-    except Exception:
-        return {"assignments": []}
+    except HTTPException:
+        raise
+    except Exception:
+        logger.exception("calendar.get_upcoming_failed", extra={"user_id": user_id})
+        return {"assignments": []}

Add test case in backend/tests/test_calendar_routes.py:

def test_returns_empty_on_db_failure(self):
    with patch("routes.calendar.table") as t:
        t.return_value.select.side_effect = Exception("DB connection error")
        r = client.get("/api/calendar/upcoming/user_andres")
    assert r.status_code == 200
    assert r.json()["assignments"] == []
🧰 Tools
🪛 Ruff (0.15.10)

[warning] 126-126: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/routes/calendar.py` around lines 117 - 127, The endpoint in
backend/routes/calendar.py swallows all exceptions without logging; update the
try/except around the table("assignments").select call to import logging, use a
module-level logger, and call logger.exception(...) (including user_id and the
query params like today) inside the except before returning {"assignments": []};
then add a test in backend/tests/test_calendar_routes.py (e.g.,
test_returns_empty_on_db_failure) that patches routes.calendar.table so select
raises Exception("DB connection error") and asserts the response is 200 with
{"assignments": []} to cover the exception path.

Comment thread backend/routes/graph.py Outdated
Comment thread frontend/src/components/OnboardingFlow.tsx Outdated
Comment thread linkedin-banner.html Outdated
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Apr 15, 2026

Copy link
Copy Markdown

Deploying web with  Cloudflare Pages  Cloudflare Pages

Latest commit: d450c25
Status: ✅  Deploy successful!
Preview URL: https://04030d0d.web-75h.pages.dev
Branch Preview URL: https://onboarding.web-75h.pages.dev

View logs

Comment thread frontend/src/components/OnboardingFlow.tsx Fixed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (2)
frontend/src/components/OnboardingFlow.tsx (2)

501-501: ⚠️ Potential issue | 🟡 Minor

Remove unused local setInput.

Line 501 declares setInput but never uses it.

🧼 Proposed fix
-                const setInput = isMinor ? setMinorInput : setMajorInput;

As per coding guidelines **/*.{ts,tsx,js}: Frontend frontend code should follow the ESLint rules configured in eslint.config.mjs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/OnboardingFlow.tsx` at line 501, The local variable
setInput is declared but never used; remove the unused declaration "const
setInput = isMinor ? setMinorInput : setMajorInput;" (or replace its usage if
intended) in the OnboardingFlow component so ESLint no-unused-vars is satisfied;
if the selector was intended for later use, instead reference the chosen setter
directly where needed (using isMinor ? setMinorInput : setMajorInput) or keep a
used variable name.

148-148: ⚠️ Potential issue | 🟠 Major

Use HTTPS for the university search request.

Line 148 calls Hipolabs over http://, which is unencrypted and can fail under mixed-content restrictions on secure pages.

🔒 Proposed fix
-        const res = await fetch(`http://universities.hipolabs.com/search?name=${encodeURIComponent(value)}&country=United+States`);
+        const res = await fetch(`https://universities.hipolabs.com/search?name=${encodeURIComponent(value)}&country=United+States`);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/OnboardingFlow.tsx` at line 148, Change the
university search request in OnboardingFlow.tsx to use HTTPS instead of HTTP:
update the fetch call that assigns to "res" (the line using
fetch(`http://universities.hipolabs.com/search?...`)) to call the secure
endpoint (`https://universities.hipolabs.com/...`) so the request is encrypted
and avoids mixed-content failures while preserving the existing query encoding
via encodeURIComponent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/src/app/api/auth/google/callback/route.ts`:
- Around line 5-6: The GET route currently calls NextResponse.redirect with a
relative path which fails; update the handler signature to accept the incoming
Request (export async function GET(request: Request)) and build an absolute URL
from request.url (e.g. new URL('/', request.url) or request.nextUrl) then append
the error query param and pass that absolute URL into NextResponse.redirect in
the GET function; reference the GET handler and NextResponse.redirect so you
modify those symbols accordingly.

In `@frontend/src/components/OnboardingFlow.tsx`:
- Around line 136-140: The Escape key listener is currently added regardless of
modal visibility and the debounce timeout isn't cleared on cleanup; update the
useEffect that registers handler to only add/remove the keydown listener when
the modal is visible (include the visible state/prop in the effect dependency
list) and ensure the handler uses onClose; additionally, track the debounce
timeout (e.g., timeoutId) used in the OnboardingFlow and call
clearTimeout(timeoutId) in the effect cleanup (and/or component unmount cleanup)
so any pending debounced actions are cancelled when visibility changes or the
component unmounts.
- Around line 413-433: The suggestion rows rendered from schoolSuggestions (and
the similar yearSuggestions/majorSuggestions lists) are non-focusable divs with
only mouse handlers; make each option keyboard-accessible by replacing the divs
(or augmenting them) so they are focusable and activatable via keyboard: give
each item tabIndex={0} or use a <button> element, add an onKeyDown handler that
calls the same selection logic on Enter/Space, and include appropriate ARIA
attributes (role="option" / aria-selected) so screen readers see selection;
ensure the selection code that calls setFormData(...) and clears suggestions is
reused for school, year, and major rendering logic to keep behavior consistent.

---

Duplicate comments:
In `@frontend/src/components/OnboardingFlow.tsx`:
- Line 501: The local variable setInput is declared but never used; remove the
unused declaration "const setInput = isMinor ? setMinorInput : setMajorInput;"
(or replace its usage if intended) in the OnboardingFlow component so ESLint
no-unused-vars is satisfied; if the selector was intended for later use, instead
reference the chosen setter directly where needed (using isMinor ? setMinorInput
: setMajorInput) or keep a used variable name.
- Line 148: Change the university search request in OnboardingFlow.tsx to use
HTTPS instead of HTTP: update the fetch call that assigns to "res" (the line
using fetch(`http://universities.hipolabs.com/search?...`)) to call the secure
endpoint (`https://universities.hipolabs.com/...`) so the request is encrypted
and avoids mixed-content failures while preserving the existing query encoding
via encodeURIComponent.
🪄 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: ef2ecf9c-faff-4a64-90eb-75136d5734e7

📥 Commits

Reviewing files that changed from the base of the PR and between 11781bc and 5f00b6a.

📒 Files selected for processing (3)
  • frontend/src/app/api/auth/google/callback/route.ts
  • frontend/src/app/api/auth/google/route.ts
  • frontend/src/components/OnboardingFlow.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/app/api/auth/google/route.ts

Comment on lines +5 to +6
export async function GET() {
return NextResponse.redirect('/?error=auth_callback_not_supported');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

In Next.js App Router route handlers, does NextResponse.redirect accept a relative path string like '/?error=x', or must it receive an absolute URL (for example, new URL('/?error=x', request.url))?

💡 Result:

In Next.js App Router route handlers, NextResponse.redirect requires an absolute URL, such as new URL('/?error=x', request.url). It does not accept relative path strings like '/?error=x' directly. Official Next.js documentation consistently shows NextResponse.redirect used with absolute URLs constructed via new URL(relativePath, request.url), primarily in middleware (formerly proxy) examples, but applicable to route handlers as both use Web-standard Response.redirect which expects a full URL string. Relative paths are handled internally by resolving against the request's origin. In contrast, the redirect function from 'next/navigation' (used in Server Components, Server Actions, and mentioned for Route Handlers) explicitly accepts relative paths like '/login' as per its API: path is "The URL to redirect to. Can be a relative or absolute path."

Citations:


Use an absolute URL for NextResponse.redirect in route handlers.

NextResponse.redirect() requires an absolute URL and does not accept relative path strings. Line 6 passes a relative string '/?error=auth_callback_not_supported', which will fail at runtime. Build the redirect from the request URL.

Proposed fix
-export async function GET() {
-  return NextResponse.redirect('/?error=auth_callback_not_supported');
+export async function GET(request: Request) {
+  const redirectUrl = new URL('/', request.url);
+  redirectUrl.searchParams.set('error', 'auth_callback_not_supported');
+  return NextResponse.redirect(redirectUrl);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export async function GET() {
return NextResponse.redirect('/?error=auth_callback_not_supported');
export async function GET(request: Request) {
const redirectUrl = new URL('/', request.url);
redirectUrl.searchParams.set('error', 'auth_callback_not_supported');
return NextResponse.redirect(redirectUrl);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/app/api/auth/google/callback/route.ts` around lines 5 - 6, The
GET route currently calls NextResponse.redirect with a relative path which
fails; update the handler signature to accept the incoming Request (export async
function GET(request: Request)) and build an absolute URL from request.url (e.g.
new URL('/', request.url) or request.nextUrl) then append the error query param
and pass that absolute URL into NextResponse.redirect in the GET function;
reference the GET handler and NextResponse.redirect so you modify those symbols
accordingly.

Comment thread frontend/src/components/OnboardingFlow.tsx Outdated
Comment thread frontend/src/components/OnboardingFlow.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

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)

4-5: ⚠️ Potential issue | 🟡 Minor

Stale docstring: says @bu.edu but code now accepts any .edu domain.

Line 5 claims sign-in is restricted to @bu.edu, but line 123 validates with .endswith(".edu"). Update the docstring to match or remove it per coding guidelines.

📝 Proposed fix
 """
 backend/routes/auth.py

 Google OAuth sign-in with unified calendar access.
-Restricts sign-in to `@bu.edu` email accounts only.
+Restricts sign-in to .edu email accounts.
 """
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/routes/auth.py` around lines 4 - 5, Update the stale module/function
docstring that currently claims sign-in is restricted to "@bu.edu" to match the
actual validation logic which accepts any ".edu" domain (the check using
.endswith(".edu") in this module); either revise the docstring text to state
"allows sign-in for any .edu email address" or remove the incorrect restriction
entirely so docs and the code (the Google OAuth sign-in logic) stay consistent.
♻️ Duplicate comments (1)
backend/routes/graph.py (1)

20-22: ⚠️ Potential issue | 🟠 Major

Don’t collapse upstream HTTP errors into 500, and chain the caught exception.

At Line 20 and Line 29, except Exception also catches HTTPException, which can overwrite intended status codes. Also, at Line 22 and Line 31, re-raising without from e drops explicit exception chaining context.

🔧 Minimal fix
 `@router.get`("/{user_id}")
 def get_user_graph(user_id: str):
     try:
         return get_graph(user_id)
+    except HTTPException:
+        raise
     except Exception as e:
         logger.exception("get_graph failed for user_id=%s", user_id)
-        raise HTTPException(status_code=500, detail=str(e) or "Failed to fetch graph")
+        raise HTTPException(status_code=500, detail="Failed to fetch graph") from e

 `@router.get`("/{user_id}/recommendations")
 def get_user_recommendations(user_id: str):
     try:
         return {"recommendations": get_recommendations(user_id)}
+    except HTTPException:
+        raise
     except Exception as e:
         logger.exception("get_recommendations failed for user_id=%s", user_id)
-        raise HTTPException(status_code=500, detail=str(e) or "Failed to fetch recommendations")
+        raise HTTPException(status_code=500, detail="Failed to fetch recommendations") from e

Run this read-only check to verify current exception patterns before/after patch:

#!/bin/bash
# Verify whether graph routes catch broad exceptions and whether HTTPException is preserved.
rg -n -C2 'def get_user_graph|def get_user_recommendations|except HTTPException|except Exception as e|raise HTTPException' backend/routes/graph.py

# Verify where HTTPException is raised in related graph service/route paths.
rg -n -C2 'raise\s+HTTPException' backend/services/graph_service.py backend/routes/graph.py backend/routes/social.py backend/routes/learn.py

Also applies to: 29-31

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/routes/graph.py` around lines 20 - 22, The route handlers (e.g.,
get_graph / get_user_recommendations) are currently catching all Exception
including HTTPException and re-raising new HTTPException without chaining;
change the exception handling to first catch HTTPException and re-raise it
unchanged, then catch broad exceptions for internal errors and raise a 500
HTTPException using "raise HTTPException(... ) from e" so the original traceback
is preserved; update the except blocks around logger.exception(...) and raise
HTTPException(...) to implement this two-step handling and include "from e" when
constructing the new HTTPException.
🧹 Nitpick comments (2)
backend/tests/test_graph_routes.py (1)

1-3: Remove the module docstring in this backend test module.

This file’s logic is straightforward, so the docstring adds noise under current backend conventions.

As per coding guidelines, "No docstrings or comments unless the logic is non-obvious".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/tests/test_graph_routes.py` around lines 1 - 3, Remove the
module-level triple-quoted docstring at the top of
backend/tests/test_graph_routes.py (the three-line string literal containing
"Unit tests for routes/graph.py") so the file starts immediately with
imports/tests; simply delete that docstring block to comply with the guideline
"No docstrings or comments unless the logic is non-obvious."
frontend/src/components/OnboardingFlow.tsx (1)

247-253: Consider preventing duplicate course entries.

The current implementation allows adding the same course multiple times. A simple duplicate check would improve UX.

💡 Proposed fix
 function addClass() {
   const v = classInput.trim();
-  if (v) {
+  if (v && !formData.courses.includes(v)) {
     setFormData(prev => ({ ...prev, courses: [...prev.courses, v] }));
     setClassInput('');
   }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/OnboardingFlow.tsx` around lines 247 - 253, addClass
currently pushes classInput into formData.courses without checking for
duplicates; update addClass to compute const v = classInput.trim(), then before
calling setFormData check whether the normalized course already exists (e.g.,
prev.courses.includes(v) or use a case-insensitive compare) and only call
setFormData(prev => ({ ...prev, courses: [...prev.courses, v] })) and clear
classInput when it is not a duplicate; if it is a duplicate, simply return or
optionally show a validation message.
🤖 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/routes/auth.py`:
- Around line 136-139: The update in the google_id branch uses
table("users").update({...}) but omits setting auth_provider, causing
inconsistency with other branches that set auth_provider="google"; modify the
update payload in that branch to include "auth_provider": "google" alongside
name, avatar_url, and email (the same fields used in the other update branches)
for the record identified by user_id so legacy users receive the auth_provider
value.

In `@backend/routes/graph.py`:
- Line 22: The route currently raises HTTPException with detail=str(e), which
can leak internal errors; change both occurrences of raise
HTTPException(status_code=500, detail=str(e) or "Failed to fetch graph") to
raise HTTPException(status_code=500, detail="Failed to fetch graph") (i.e.,
return a stable generic message) and keep full exception logging as-is; also
update the assertions in backend/tests/test_graph_routes.py (the assertions at
Line 20 and Line 27) to expect the new generic message.

In `@frontend/src/components/OnboardingFlow.tsx`:
- Around line 298-308: The close button in OnboardingFlow.tsx lacks an
accessible name; update the button element (the one with onClick={onClose} and
the X icon) to include an accessible label such as aria-label="Close" (or
aria-label={`Close onboarding`} / title and/or visually hidden text) so screen
readers announce its purpose; ensure the label is descriptive and unique to this
control and does not rely solely on the decorative <X /> icon.
- Around line 621-624: The remove buttons for chips (the button wrapping XCircle
in OnboardingFlow.tsx using setFormData and field) lack accessible names; update
both chip remove buttons (the ones that call setFormData(prev => ({ ...prev,
[field]: prev[field].filter((_, i) => i !== idx) })) and render <XCircle>) to
include an explicit accessible name (e.g., aria-label or title) that includes
the chip context—preferably the chip value if available or at least `Remove
${field}` and the index for uniqueness—and also set type="button" to avoid
accidental form submission; ensure both occurrences (the one shown and the
symmetric one later) are changed consistently.

---

Outside diff comments:
In `@backend/routes/auth.py`:
- Around line 4-5: Update the stale module/function docstring that currently
claims sign-in is restricted to "@bu.edu" to match the actual validation logic
which accepts any ".edu" domain (the check using .endswith(".edu") in this
module); either revise the docstring text to state "allows sign-in for any .edu
email address" or remove the incorrect restriction entirely so docs and the code
(the Google OAuth sign-in logic) stay consistent.

---

Duplicate comments:
In `@backend/routes/graph.py`:
- Around line 20-22: The route handlers (e.g., get_graph /
get_user_recommendations) are currently catching all Exception including
HTTPException and re-raising new HTTPException without chaining; change the
exception handling to first catch HTTPException and re-raise it unchanged, then
catch broad exceptions for internal errors and raise a 500 HTTPException using
"raise HTTPException(... ) from e" so the original traceback is preserved;
update the except blocks around logger.exception(...) and raise
HTTPException(...) to implement this two-step handling and include "from e" when
constructing the new HTTPException.

---

Nitpick comments:
In `@backend/tests/test_graph_routes.py`:
- Around line 1-3: Remove the module-level triple-quoted docstring at the top of
backend/tests/test_graph_routes.py (the three-line string literal containing
"Unit tests for routes/graph.py") so the file starts immediately with
imports/tests; simply delete that docstring block to comply with the guideline
"No docstrings or comments unless the logic is non-obvious."

In `@frontend/src/components/OnboardingFlow.tsx`:
- Around line 247-253: addClass currently pushes classInput into
formData.courses without checking for duplicates; update addClass to compute
const v = classInput.trim(), then before calling setFormData check whether the
normalized course already exists (e.g., prev.courses.includes(v) or use a
case-insensitive compare) and only call setFormData(prev => ({ ...prev, courses:
[...prev.courses, v] })) and clear classInput when it is not a duplicate; if it
is a duplicate, simply return or optionally show a validation message.
🪄 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: ff3d73e5-6fd8-4713-a49c-e07187541ee6

📥 Commits

Reviewing files that changed from the base of the PR and between a809e46 and d450c25.

📒 Files selected for processing (7)
  • backend/routes/auth.py
  • backend/routes/calendar.py
  • backend/routes/graph.py
  • backend/tests/test_calendar_routes.py
  • backend/tests/test_graph_routes.py
  • frontend/src/components/OnboardingFlow.tsx
  • linkedin-banner.html
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/routes/calendar.py

Comment thread backend/routes/auth.py
Comment on lines 136 to 139
table("users").update(
{
"google_id": google_id,
"name": name,
"avatar_url": avatar_url,
"auth_provider": "google",
},
{"name": name, "avatar_url": avatar_url, "email": email},
filters={"id": f"eq.{user_id}"},
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Inconsistent auth_provider update across branches.

When a user is found by google_id match, the update (lines 136-139) does not set auth_provider, while all other code paths explicitly set auth_provider="google" (lines 146, 156, 169). This could leave legacy users without the field populated.

🔧 Proposed fix for consistency
         table("users").update(
-            {"name": name, "avatar_url": avatar_url, "email": email},
+            {"name": name, "avatar_url": avatar_url, "email": email, "auth_provider": "google"},
             filters={"id": f"eq.{user_id}"},
         )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/routes/auth.py` around lines 136 - 139, The update in the google_id
branch uses table("users").update({...}) but omits setting auth_provider,
causing inconsistency with other branches that set auth_provider="google";
modify the update payload in that branch to include "auth_provider": "google"
alongside name, avatar_url, and email (the same fields used in the other update
branches) for the record identified by user_id so legacy users receive the
auth_provider value.

Comment thread backend/routes/graph.py
return get_graph(user_id)
except Exception as e:
logger.exception("get_graph failed for user_id=%s", user_id)
raise HTTPException(status_code=500, detail=str(e) or "Failed to fetch graph")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid exposing raw backend exception text in API responses.

Using detail=str(e) can leak internal error content to clients. Since you already log the full exception, return a stable generic message instead. (If changed, update assertions in backend/tests/test_graph_routes.py at Line 20 and Line 27 accordingly.)

Also applies to: 31-31

🧰 Tools
🪛 Ruff (0.15.10)

[warning] 22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/routes/graph.py` at line 22, The route currently raises HTTPException
with detail=str(e), which can leak internal errors; change both occurrences of
raise HTTPException(status_code=500, detail=str(e) or "Failed to fetch graph")
to raise HTTPException(status_code=500, detail="Failed to fetch graph") (i.e.,
return a stable generic message) and keep full exception logging as-is; also
update the assertions in backend/tests/test_graph_routes.py (the assertions at
Line 20 and Line 27) to expect the new generic message.

Comment on lines +298 to +308
<button
onClick={onClose}
style={{
position: 'absolute', top: '28px', right: '32px', zIndex: 10,
color: 'rgba(0,0,0,0.28)', background: 'none', border: 'none',
padding: '8px', display: 'flex', cursor: 'pointer',
transition: 'color 0.2s ease',
}}
>
<X style={{ width: '22px', height: '22px' }} strokeWidth={1.5} />
</button>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add accessible label to the close button.

The close button contains only an icon with no accessible name. Screen reader users won't know its purpose.

♿ Proposed fix
 <button
   onClick={onClose}
+  aria-label="Close onboarding"
   style={{
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<button
onClick={onClose}
style={{
position: 'absolute', top: '28px', right: '32px', zIndex: 10,
color: 'rgba(0,0,0,0.28)', background: 'none', border: 'none',
padding: '8px', display: 'flex', cursor: 'pointer',
transition: 'color 0.2s ease',
}}
>
<X style={{ width: '22px', height: '22px' }} strokeWidth={1.5} />
</button>
<button
onClick={onClose}
aria-label="Close onboarding"
style={{
position: 'absolute', top: '28px', right: '32px', zIndex: 10,
color: 'rgba(0,0,0,0.28)', background: 'none', border: 'none',
padding: '8px', display: 'flex', cursor: 'pointer',
transition: 'color 0.2s ease',
}}
>
<X style={{ width: '22px', height: '22px' }} strokeWidth={1.5} />
</button>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/OnboardingFlow.tsx` around lines 298 - 308, The close
button in OnboardingFlow.tsx lacks an accessible name; update the button element
(the one with onClick={onClose} and the X icon) to include an accessible label
such as aria-label="Close" (or aria-label={`Close onboarding`} / title and/or
visually hidden text) so screen readers announce its purpose; ensure the label
is descriptive and unique to this control and does not rely solely on the
decorative <X /> icon.

Comment on lines +621 to +624
<button onClick={() => setFormData(prev => ({ ...prev, [field]: prev[field].filter((_, i) => i !== idx) }))}
style={{ background: 'none', border: 'none', padding: 0, cursor: 'pointer', display: 'flex', color: 'rgba(0,0,0,0.35)' }}>
<XCircle style={{ width: '13px', height: '13px' }} />
</button>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add accessible labels to remove buttons.

The remove buttons for chips only contain icons. Screen reader users won't know what action they perform.

♿ Proposed fix (apply to both chip remove buttons at lines 621-624 and 665-668)
-<button onClick={() => setFormData(prev => ({ ...prev, [field]: prev[field].filter((_, i) => i !== idx) }))}
+<button
+  aria-label={`Remove ${item}`}
+  onClick={() => setFormData(prev => ({ ...prev, [field]: prev[field].filter((_, i) => i !== idx) }))}
   style={{ background: 'none', border: 'none', padding: 0, cursor: 'pointer', display: 'flex', color: 'rgba(0,0,0,0.35)' }}>

For the courses chip at line 665:

-<button onClick={() => setFormData(prev => ({ ...prev, courses: prev.courses.filter((_, i) => i !== idx) }))}
+<button
+  aria-label={`Remove ${chip}`}
+  onClick={() => setFormData(prev => ({ ...prev, courses: prev.courses.filter((_, i) => i !== idx) }))}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<button onClick={() => setFormData(prev => ({ ...prev, [field]: prev[field].filter((_, i) => i !== idx) }))}
style={{ background: 'none', border: 'none', padding: 0, cursor: 'pointer', display: 'flex', color: 'rgba(0,0,0,0.35)' }}>
<XCircle style={{ width: '13px', height: '13px' }} />
</button>
<button
aria-label={`Remove ${item}`}
onClick={() => setFormData(prev => ({ ...prev, [field]: prev[field].filter((_, i) => i !== idx) }))}
style={{ background: 'none', border: 'none', padding: 0, cursor: 'pointer', display: 'flex', color: 'rgba(0,0,0,0.35)' }}>
<XCircle style={{ width: '13px', height: '13px' }} />
</button>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/OnboardingFlow.tsx` around lines 621 - 624, The
remove buttons for chips (the button wrapping XCircle in OnboardingFlow.tsx
using setFormData and field) lack accessible names; update both chip remove
buttons (the ones that call setFormData(prev => ({ ...prev, [field]:
prev[field].filter((_, i) => i !== idx) })) and render <XCircle>) to include an
explicit accessible name (e.g., aria-label or title) that includes the chip
context—preferably the chip value if available or at least `Remove ${field}` and
the index for uniqueness—and also set type="button" to avoid accidental form
submission; ensure both occurrences (the one shown and the symmetric one later)
are changed consistently.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
Member

Superseded — the OnboardingFlow component and integration have been cherry-picked into main in commit 7ef2982. Zoom/cluster canvas effects deferred to a follow-up.

@AndresL230 AndresL230 deleted the onboarding branch April 19, 2026 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants