Merged
Conversation
Remove the firebase-auth-token fallback cookie that was never read by middleware or auth.ts, causing a false sense of resilience. Remove the AbortController timeout on the session POST that was aborting before the server could respond on cold starts. Login now awaits the session POST fully and surfaces clear error messages on failure. - Remove firebase-auth-token cookie from login, logout, sidebar, user-nav - Remove AbortController from login page session POST - Remove onIdTokenChanged listener from ProtectedRoute - Remove onIdTokenChange export from client auth module - Revert middleware to only check __session cookie Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix transparent mobile sidebar by adding --sidebar CSS variable and Tailwind color mapping so bg-sidebar resolves correctly - Add "← Back to Dashboard" link to athletes, analytics, settings, profile, and games pages (matching dream-gym style) - Change header text from "Dashboard" to "Hustle" - Move sidebar close button to right of HUSTLE logo Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughRemoves client-side firebase-auth-token handling and onIdTokenChange usage, centralizes session to a server-set Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Client)
participant LoginPage as Login Page (client)
participant Firebase as Firebase Auth
participant SetSessionAPI as /api/auth/set-session
participant Server as App Server
participant Middleware as Edge Middleware
participant Dashboard as Dashboard (protected)
User->>LoginPage: Submit credentials / start sign-in
LoginPage->>Firebase: Sign in / request ID token
Firebase-->>LoginPage: Return ID token
LoginPage->>SetSessionAPI: POST ID token
SetSessionAPI->>Server: Verify token, create __session cookie
Server-->>LoginPage: 200 OK (sets __session cookie)
LoginPage->>Dashboard: Navigate to /dashboard (browser sends __session)
Dashboard->>Middleware: Incoming request with __session cookie
Middleware->>Server: Verify __session
Server-->>Middleware: Verification result
Middleware-->>Dashboard: Allow or deny access
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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. Comment |
Review Summary by QodoRemove OTEL deadlock and improve authentication flow with UI enhancements
WalkthroughsDescription• Remove broken firebase-auth-token fallback cookie and AbortController timeout that caused deadlocks - Eliminates false sense of resilience from unread fallback cookie - Removes premature AbortController timeout on session POST - Login now awaits session POST fully with clear error messages • Add sidebar CSS variables and Tailwind color mapping for mobile styling • Add "Back to Dashboard" navigation links to sub-pages (athletes, analytics, settings, profile, games) • Update header text from "Dashboard" to "Hustle" and reorganize sidebar close button Diagramflowchart LR
A["Auth Flow"] -->|Remove| B["firebase-auth-token cookie"]
A -->|Remove| C["AbortController timeout"]
A -->|Simplify| D["Middleware checks __session only"]
E["UI Improvements"] -->|Add| F["Sidebar CSS variables"]
E -->|Add| G["Back to Dashboard links"]
E -->|Update| H["Header text to Hustle"]
E -->|Reorganize| I["Sidebar close button"]
File Changes1. src/app/api/auth/logout/route.ts
|
Code Review by Qodo
1.
|
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily refines the application's authentication flow by deprecating the client-side 'firebase-auth-token' fallback cookie, ensuring that session management relies exclusively on the secure, server-side '__session' cookie. This change simplifies the authentication logic and enhances its robustness, particularly during the login process. Concurrently, the pull request introduces several user interface improvements across various dashboard sub-pages, such as adding "Back to Dashboard" navigation links and minor layout adjustments, to improve overall user experience. Note: The title "Fix/remove otel deadlock" does not align with the changes observed in the provided patch, which focus on authentication and UI enhancements. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication flow by removing the firebase-auth-token fallback cookie and relying solely on the __session cookie. This simplification is a positive change that likely resolves the deadlock issue mentioned in the title by creating a single source of truth for authentication state. The changes are applied consistently across login, logout, and middleware logic. My review includes one suggestion to improve maintainability by abstracting the duplicated 'Back to Dashboard' link into a reusable component.
… component Address code review feedback: 1. Login session POST now has a 30s client-side timeout (AbortController) with one automatic retry on transient failures (504, 500, network error, timeout). Prevents indefinite loading state while tolerating cold starts. 2. Extract BackToDashboard into a reusable component used by athletes, analytics, games, profile, and settings pages — reduces duplication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/login/page.tsx (1)
78-83: Consider a more robust network error detection heuristic.The condition
err.message?.includes('fetch')is a weak heuristic that could match unrelated errors containing the word "fetch". Consider checking forTypeError(whichfetchthrows on network failures) or using a more specific pattern.💡 Suggested improvement
const isRetryable = attempt < 2 && ( err.name === 'AbortError' || err.status === 504 || err.status === 500 || - err.message?.includes('fetch') + err instanceof TypeError // fetch throws TypeError on network failures );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/login/page.tsx` around lines 78 - 83, The retry heuristic in the isRetryable calculation is too loose because err.message?.includes('fetch') can match unrelated messages; update the condition used inside the isRetryable expression (where attempt, err.name, and err.status are referenced) to detect network/fetch failures more robustly by checking for TypeError (e.g., err instanceof TypeError or err.name === 'TypeError') and optionally adding a focused regex like /NetworkError|Failed to fetch/ on err.message, replacing the current err.message?.includes('fetch') check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/login/page.tsx`:
- Around line 78-83: The retry heuristic in the isRetryable calculation is too
loose because err.message?.includes('fetch') can match unrelated messages;
update the condition used inside the isRetryable expression (where attempt,
err.name, and err.status are referenced) to detect network/fetch failures more
robustly by checking for TypeError (e.g., err instanceof TypeError or err.name
=== 'TypeError') and optionally adding a focused regex like /NetworkError|Failed
to fetch/ on err.message, replacing the current err.message?.includes('fetch')
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e668e84b-aa98-4a8f-85ce-a673660016e2
📒 Files selected for processing (7)
src/app/dashboard/analytics/page.tsxsrc/app/dashboard/athletes/page.tsxsrc/app/dashboard/games/page.tsxsrc/app/dashboard/profile/page.tsxsrc/app/dashboard/settings/page.tsxsrc/app/login/page.tsxsrc/components/ui/back-to-dashboard.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/dashboard/profile/page.tsx
- src/app/dashboard/analytics/page.tsx
- src/app/dashboard/athletes/page.tsx
- src/app/dashboard/games/page.tsx
__session is now the sole auth mechanism (no fallback cookie). Server-side verifyIdToken/createSessionCookie timeouts of 10s are too aggressive for Firebase Admin cold starts (10-20s documented). Increased to 25s to stay under the client's 30s AbortController timeout while accommodating cold starts. Addresses Qodo review finding on PR #39. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/auth/set-session/route.ts (1)
43-59:⚠️ Potential issue | 🟠 MajorUse a single end-to-end timeout budget here.
Lines 43 and 59 now allow roughly 50s total because the 25s limits are applied sequentially. That exceeds the client's 30s abort window from the PR context, so slow cold starts can still fail login before the
__sessionresponse makes it back.⏱️ Proposed fix
+const SESSION_SETUP_TIMEOUT_MS = 25_000; + export async function POST(request: NextRequest) { const t0 = Date.now(); + const remainingTimeoutMs = () => + Math.max(1, SESSION_SETUP_TIMEOUT_MS - (Date.now() - t0)); console.log(`[set-session] POST handler entered at ${new Date().toISOString()}`); try { @@ - decodedToken = await withTimeout(adminAuth.verifyIdToken(idToken, true), 25000, 'verifyIdToken'); + decodedToken = await withTimeout( + adminAuth.verifyIdToken(idToken, true), + remainingTimeoutMs(), + 'verifyIdToken' + ); @@ - sessionCookie = await withTimeout(adminAuth.createSessionCookie(idToken, { expiresIn: SESSION_EXPIRES_MS }), 25000, 'createSessionCookie'); + sessionCookie = await withTimeout( + adminAuth.createSessionCookie(idToken, { expiresIn: SESSION_EXPIRES_MS }), + remainingTimeoutMs(), + 'createSessionCookie' + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/set-session/route.ts` around lines 43 - 59, The two sequential withTimeout calls (adminAuth.verifyIdToken and adminAuth.createSessionCookie) each use 25s, allowing ~50s total; change to a single end-to-end timeout budget by recording t0 at request start and computing remainingMs before each Firebase call, then pass that remainingMs into withTimeout (or return a timeout error if remainingMs <= 0). Update the calls that reference adminAuth.verifyIdToken and adminAuth.createSessionCookie (and the surrounding t0/withTimeout logic) to use the computed remaining time (based on a single constant TOTAL_SESSION_BUDGET_MS) so both calls share one timeout budget and never exceed the client abort window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/api/auth/set-session/route.ts`:
- Around line 43-59: The two sequential withTimeout calls
(adminAuth.verifyIdToken and adminAuth.createSessionCookie) each use 25s,
allowing ~50s total; change to a single end-to-end timeout budget by recording
t0 at request start and computing remainingMs before each Firebase call, then
pass that remainingMs into withTimeout (or return a timeout error if remainingMs
<= 0). Update the calls that reference adminAuth.verifyIdToken and
adminAuth.createSessionCookie (and the surrounding t0/withTimeout logic) to use
the computed remaining time (based on a single constant TOTAL_SESSION_BUDGET_MS)
so both calls share one timeout budget and never exceed the client abort window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8060cd3c-9509-4f90-809d-b18f248a7aba
📒 Files selected for processing (1)
src/app/api/auth/set-session/route.ts
jeremylongshore
pushed a commit
that referenced
this pull request
Mar 9, 2026
* fix(auth): remove broken fallback cookie and AbortController timeout Remove the firebase-auth-token fallback cookie that was never read by middleware or auth.ts, causing a false sense of resilience. Remove the AbortController timeout on the session POST that was aborting before the server could respond on cold starts. Login now awaits the session POST fully and surfaces clear error messages on failure. - Remove firebase-auth-token cookie from login, logout, sidebar, user-nav - Remove AbortController from login page session POST - Remove onIdTokenChanged listener from ProtectedRoute - Remove onIdTokenChange export from client auth module - Revert middleware to only check __session cookie Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): mobile sidebar background, back buttons, header text, close btn - Fix transparent mobile sidebar by adding --sidebar CSS variable and Tailwind color mapping so bg-sidebar resolves correctly - Add "← Back to Dashboard" link to athletes, analytics, settings, profile, and games pages (matching dream-gym style) - Change header text from "Dashboard" to "Hustle" - Move sidebar close button to right of HUSTLE logo Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(auth,ui): add session POST timeout+retry, extract BackToDashboard component Address code review feedback: 1. Login session POST now has a 30s client-side timeout (AbortController) with one automatic retry on transient failures (504, 500, network error, timeout). Prevents indefinite loading state while tolerating cold starts. 2. Extract BackToDashboard into a reusable component used by athletes, analytics, games, profile, and settings pages — reduces duplication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(auth): increase set-session timeouts from 10s to 25s for cold starts __session is now the sole auth mechanism (no fallback cookie). Server-side verifyIdToken/createSessionCookie timeouts of 10s are too aggressive for Firebase Admin cold starts (10-20s documented). Increased to 25s to stay under the client's 30s AbortController timeout while accommodating cold starts. Addresses Qodo review finding on PR #39. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: jeremylongshore <noreply@localhost.local>
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brief description of changes.
Changes Made
Testing
Risk Assessment
Rollback Plan
How to revert if needed.
Summary by CodeRabbit
New Features
UI Improvements
Authentication