Fix sign-in hang and athlete positions#36
Merged
jeremylongshore merged 3 commits intomainfrom Mar 6, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAuthentication was made request-aware across many API routes; auth/provisioning gained timeouts and request-cookie handling. Storage validation/quota logic was moved to a new isomorphic module. Player update validation/schema and admin-focused storage/player service usage were introduced; frontend photo upload endpoints updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route
participant Auth
participant Firebase as UserDB
participant Provisioner
Client->>Route: HTTP request (with cookies)
Route->>Auth: auth(request)
Auth->>Auth: extract session cookie from request
Auth->>Auth: verifySessionCookie (with timeout)
Auth->>UserDB: fetch user profile by uid (with timeout)
alt user not found
Auth->>Provisioner: ensureUserProvisioned(uid) (with timeout)
Provisioner->>UserDB: create/ensure user doc
Auth->>UserDB: fetch user profile again
end
Auth-->>Route: Session / DashboardUser | null
Route-->>Client: API response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoFix sign-in hangs with request-based auth and stabilize athlete profile updates
WalkthroughsDescription• Pass NextRequest to auth() calls to read cookies directly from request instead of calling cookies() internally, preventing sign-in hangs • Add timeout wrappers around Firebase operations to prevent indefinite hangs during session verification and user provisioning • Implement comprehensive player profile validation using playerSchema with support for primary/secondary positions, gender, league code, and position notes • Fix photo upload endpoint to use correct FormData key (file instead of photo) and correct API route (/api/storage/upload-player-photo) • Prevent primary position from appearing in secondary positions list when primary position changes • Refactor storage utilities into isomorphic storage-utils.ts module for shared client/server usage • Migrate storage operations to use admin SDK functions for consistency and proper access control Diagramflowchart LR
A["API Routes"] -->|"Pass NextRequest"| B["auth function"]
B -->|"Read cookies directly"| C["Session verification"]
C -->|"Add timeouts"| D["Prevent hangs"]
E["Player Update"] -->|"Validate with schema"| F["Position handling"]
F -->|"Persist all fields"| G["Firestore"]
H["Photo Upload"] -->|"Use correct FormData key"| I["Storage API"]
I -->|"Admin SDK"| J["Firebase Storage"]
File Changes1. src/lib/auth.ts
|
Code Review by Qodo
1. authWithProfile not request-scoped
|
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 significantly enhances the application's stability and data integrity by addressing critical authentication issues and expanding athlete profile management capabilities. It introduces explicit request-based authentication, implements timeouts for key server-side operations to prevent sign-in hangs, and provides a more comprehensive and validated structure for athlete positions and personal details. Additionally, photo handling has been streamlined through dedicated storage APIs, and development tooling is improved with updated ESLint ignore patterns. Highlights
Changelog
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
|
|
✅ Synthetic QA Passed! Fake humans are happy. All critical user journeys working:
Safe to merge. |
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
![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 introduces several significant improvements to the application's stability and functionality. The server-side authentication has been stabilized by passing the NextRequest to auth calls, adding timeouts to prevent hangs, and implementing a fallback for user provisioning. The player update endpoint is now more robust, using Zod for validation and persisting additional profile fields like primary and secondary positions. Furthermore, the photo upload functionality has been corrected, and the user experience for managing positions has been enhanced. Overall, these are excellent changes that improve correctness, robustness, and maintainability. I've included a couple of minor suggestions to further refine the code.
| primaryPosition: validatedData.primaryPosition, | ||
| position: validatedData.primaryPosition, // Legacy field for backward compatibility | ||
| secondaryPositions: validatedData.secondaryPositions ?? [], | ||
| positionNote: validatedData.positionNote?.trim() ? validatedData.positionNote.trim() : null, |
Contributor
There was a problem hiding this comment.
This line calls trim() twice, which is slightly inefficient. You can simplify this to validatedData.positionNote?.trim() || null, which achieves the same result more concisely and avoids the redundant call.
Suggested change
| positionNote: validatedData.positionNote?.trim() ? validatedData.positionNote.trim() : null, | |
| positionNote: validatedData.positionNote?.trim() || null, |
Comment on lines
+149
to
+151
| leagueOtherName: validatedData.leagueCode === 'other' && validatedData.leagueOtherName?.trim() | ||
| ? validatedData.leagueOtherName.trim() | ||
| : null, |
Contributor
There was a problem hiding this comment.
The logic for leagueOtherName is a bit verbose and calls trim() twice. It can be simplified into a more concise and efficient single line while maintaining the same logic.
leagueOtherName: (validatedData.leagueCode === 'other' && validatedData.leagueOtherName?.trim()) || null,![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/storage/upload-player-photo/route.ts (1)
111-146:⚠️ Potential issue | 🟠 MajorDelete order can lose the existing photo on partial failure.
Line 111 deletes the current photo before Line 137 uploads/persists the replacement. If upload or player update fails, the user can end up with no valid photo, and storage accounting can drift.
🔧 Proposed sequencing fix
- // 7. Delete old photo if exists - let oldPhotoSizeMB = 0; - if (player.photoUrl) { - const oldPath = extractStoragePath(player.photoUrl); - if (oldPath) { - try { - oldPhotoSizeMB = await deletePlayerPhotoAdmin(oldPath); - logger.info('Replaced old player photo', { - userId, - playerId, - oldPath, - oldSize: oldPhotoSizeMB, - }); - } catch (error) { - // Log but don't fail - old photo might already be deleted - logger.warn('Failed to delete old player photo', { - userId, - playerId, - oldPath, - error, - }); - } - } - } - - // 8. Upload new photo - const uploadResult = await uploadPlayerPhotoAdmin({ file, userId, playerId }); - - // 9. Update player photoUrl in Firestore - await updatePlayerAdmin(userId, playerId, { - photoUrl: uploadResult.url, - }); + const oldPath = player.photoUrl ? extractStoragePath(player.photoUrl) : null; + + // 7. Upload new photo first + const uploadResult = await uploadPlayerPhotoAdmin({ file, userId, playerId }); + try { + // 8. Persist new URL + await updatePlayerAdmin(userId, playerId, { photoUrl: uploadResult.url }); + } catch (error) { + // Compensate orphaned upload if Firestore update fails + await deletePlayerPhotoAdmin(uploadResult.path).catch(() => undefined); + throw error; + } + + // 9. Delete old photo only after successful switch + let oldPhotoSizeMB = 0; + if (oldPath) { + try { + oldPhotoSizeMB = await deletePlayerPhotoAdmin(oldPath); + } catch (error) { + logger.warn('Failed to delete old player photo', { userId, playerId, oldPath, error }); + } + } // 10. Update workspace storage usage const netStorageChange = uploadResult.size - oldPhotoSizeMB; await updateWorkspaceStorageUsageAdmin(workspace.id, netStorageChange);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/storage/upload-player-photo/route.ts` around lines 111 - 146, The current flow deletes the old photo (deletePlayerPhotoAdmin) before uploading and persisting the replacement, risking loss if the upload/update fails; change the sequence to first upload the new file via uploadPlayerPhotoAdmin, then call updatePlayerAdmin to persist photoUrl, and only after successful persistence attempt to delete the old file via deletePlayerPhotoAdmin (using extractStoragePath to find oldPath). Compute and call updateWorkspaceStorageUsageAdmin with netStorageChange = uploadedSize - deletedOldSize, and if deletion fails log and avoid subtracting old size; additionally, if upload succeeds but updatePlayerAdmin fails, delete the newly uploaded file to avoid orphaned storage (wrap deletes in try/catch and log).
🧹 Nitpick comments (4)
src/app/api/players/create/route.ts (1)
84-95: Consider validating the session cookie before verification.The empty string fallback (
|| '') meansverifySessionCookie('')will be called if no cookie exists, which will throw an error caught by the try-catch. While functionally safe, an early return would be slightly cleaner and avoids an unnecessary Firebase call.♻️ Optional: Add early validation
// Get the decoded token claims from the current session const sessionCookie = request.cookies.get('__session')?.value || ''; + if (!sessionCookie) { + throw new Error('No session cookie for fallback provisioning'); + } const decodedToken = await adminAuth.verifySessionCookie(sessionCookie);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/players/create/route.ts` around lines 84 - 95, The code calls adminAuth.verifySessionCookie even when no cookie exists because sessionCookie is set to '' via the || fallback; change the flow to validate the cookie first (check request.cookies.get('__session')?.value or a local sessionCookie variable for truthiness) and if it's missing do an early return or skip the fallback provisioning block, otherwise call adminAuth.verifySessionCookie(sessionCookie), then proceed to ensureUserProvisioned and getUserProfileAdmin; update logging to reflect the early-return path and keep references to request.cookies, sessionCookie, adminAuth.verifySessionCookie, ensureUserProvisioned, getUserProfileAdmin, and logger to locate the affected code.src/lib/firebase/admin-services/players.ts (1)
172-194: Normalize position fields insideupdatePlayerAdminto enforce invariants.Right now the service trusts callers to keep
primaryPosition, legacyposition, andsecondaryPositionsconsistent. Centralizing normalization here avoids silent drift from any future caller.♻️ Suggested normalization in `updatePlayerAdmin`
export async function updatePlayerAdmin( userId: string, playerId: string, data: Partial<{ @@ }> ): Promise<void> { try { const playerRef = adminDb.collection(`users/${userId}/players`).doc(playerId); + const normalizedData = { ...data }; + + if (normalizedData.primaryPosition) { + normalizedData.position = normalizedData.primaryPosition; + if (Array.isArray(normalizedData.secondaryPositions)) { + normalizedData.secondaryPositions = normalizedData.secondaryPositions.filter( + (pos) => pos !== normalizedData.primaryPosition + ); + } + } + await playerRef.update({ - ...data, + ...normalizedData, updatedAt: new Date(), }); } catch (error: any) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/firebase/admin-services/players.ts` around lines 172 - 194, updatePlayerAdmin currently trusts callers to keep primaryPosition, legacy position, and secondaryPositions consistent; centralize normalization inside updatePlayerAdmin by computing/validating the position fields before calling playerRef.update: ensure primaryPosition and position are synchronized (if one is provided prefer primaryPosition or fallback deterministically to position), dedupe secondaryPositions and remove any entry equal to the resolved primary/position, normalize empty arrays/nulls to undefined when appropriate, and only spread the sanitized primaryPosition, position, and secondaryPositions into the update payload (alongside updatedAt) so invariants are enforced regardless of caller input.src/app/api/players/[id]/route.test.ts (1)
76-81: Add one regression case for primary/secondary overlap.Nice expansion of payload/assertions. Consider one extra test where
secondaryPositionscontains the newprimaryPositionto lock the position-consistency behavior this PR targets.Also applies to: 252-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/players/`[id]/route.test.ts around lines 76 - 81, Add a regression test in the existing players update tests to cover primary/secondary overlap: craft an update payload where primaryPosition is e.g. 'ST' and secondaryPositions includes that same value (e.g. ['ST','LW']), call the same update helper used in the file (the PATCH/update test block in route.test.ts), and assert that the response preserves the provided primaryPosition and does not include that primaryPosition inside secondaryPositions (i.e., response.primaryPosition === 'ST' and response.secondaryPositions does not contain 'ST'); add this case alongside the existing payload/assertion blocks (also apply the same addition around the other corresponding test block at the region mentioned).src/app/api/players/route.ts (1)
36-45: Consider normalizing all optional profile fields for a stable response shape.
secondaryPositions,positionNote, andleagueOtherNameare normalized, butgender,primaryPosition, andleagueCodemay still serialize as omitted when undefined. Consistent null defaults can reduce client-side branching.Optional response-shape normalization diff
return { id: player.id, name: player.name, birthday: player.birthday, - gender: player.gender, - primaryPosition: player.primaryPosition, + gender: player.gender ?? null, + primaryPosition: player.primaryPosition ?? null, secondaryPositions: player.secondaryPositions ?? [], positionNote: player.positionNote ?? null, // Legacy field (backward compatibility) position: player.primaryPosition ?? player.position, teamClub: player.teamClub, - leagueCode: player.leagueCode, + leagueCode: player.leagueCode ?? null, leagueOtherName: player.leagueOtherName ?? null, photoUrl: player.photoUrl, pendingGames: unverifiedGames.length, parentEmail: parentUser?.email ?? null };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/players/route.ts` around lines 36 - 45, Normalize optional profile fields in the response by ensuring gender, primaryPosition, and leagueCode are explicitly set to null when undefined; update the object construction that uses player.gender, player.primaryPosition, and player.leagueCode (alongside the already-normalized secondaryPositions, positionNote, and leagueOtherName) to use a null-default (e.g., player.gender ?? null) so the response shape remains stable for clients while preserving the existing legacy position logic (position: player.primaryPosition ?? player.position).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/auth.ts`:
- Around line 74-78: The withTimeout helper is leaving timers active when the
wrapped promise rejects (used around adminAuth.verifySessionCookie and other
auth calls like the ones at lines 102-106, 108-112, 116-120), so update
src/lib/utils/timeout.ts to always clear the timeout on both resolve and reject
(e.g., clear the timer in a finally or ensure both then and catch handlers call
clearTimeout) so rejected auth paths don't accumulate stale timers; keep the
same API so calls using withTimeout( adminAuth.verifySessionCookie(...),
VERIFY_SESSION_COOKIE_TIMEOUT_MS, 'verifySessionCookie' ) continue to work.
---
Outside diff comments:
In `@src/app/api/storage/upload-player-photo/route.ts`:
- Around line 111-146: The current flow deletes the old photo
(deletePlayerPhotoAdmin) before uploading and persisting the replacement,
risking loss if the upload/update fails; change the sequence to first upload the
new file via uploadPlayerPhotoAdmin, then call updatePlayerAdmin to persist
photoUrl, and only after successful persistence attempt to delete the old file
via deletePlayerPhotoAdmin (using extractStoragePath to find oldPath). Compute
and call updateWorkspaceStorageUsageAdmin with netStorageChange = uploadedSize -
deletedOldSize, and if deletion fails log and avoid subtracting old size;
additionally, if upload succeeds but updatePlayerAdmin fails, delete the newly
uploaded file to avoid orphaned storage (wrap deletes in try/catch and log).
---
Nitpick comments:
In `@src/app/api/players/`[id]/route.test.ts:
- Around line 76-81: Add a regression test in the existing players update tests
to cover primary/secondary overlap: craft an update payload where
primaryPosition is e.g. 'ST' and secondaryPositions includes that same value
(e.g. ['ST','LW']), call the same update helper used in the file (the
PATCH/update test block in route.test.ts), and assert that the response
preserves the provided primaryPosition and does not include that primaryPosition
inside secondaryPositions (i.e., response.primaryPosition === 'ST' and
response.secondaryPositions does not contain 'ST'); add this case alongside the
existing payload/assertion blocks (also apply the same addition around the other
corresponding test block at the region mentioned).
In `@src/app/api/players/create/route.ts`:
- Around line 84-95: The code calls adminAuth.verifySessionCookie even when no
cookie exists because sessionCookie is set to '' via the || fallback; change the
flow to validate the cookie first (check request.cookies.get('__session')?.value
or a local sessionCookie variable for truthiness) and if it's missing do an
early return or skip the fallback provisioning block, otherwise call
adminAuth.verifySessionCookie(sessionCookie), then proceed to
ensureUserProvisioned and getUserProfileAdmin; update logging to reflect the
early-return path and keep references to request.cookies, sessionCookie,
adminAuth.verifySessionCookie, ensureUserProvisioned, getUserProfileAdmin, and
logger to locate the affected code.
In `@src/app/api/players/route.ts`:
- Around line 36-45: Normalize optional profile fields in the response by
ensuring gender, primaryPosition, and leagueCode are explicitly set to null when
undefined; update the object construction that uses player.gender,
player.primaryPosition, and player.leagueCode (alongside the already-normalized
secondaryPositions, positionNote, and leagueOtherName) to use a null-default
(e.g., player.gender ?? null) so the response shape remains stable for clients
while preserving the existing legacy position logic (position:
player.primaryPosition ?? player.position).
In `@src/lib/firebase/admin-services/players.ts`:
- Around line 172-194: updatePlayerAdmin currently trusts callers to keep
primaryPosition, legacy position, and secondaryPositions consistent; centralize
normalization inside updatePlayerAdmin by computing/validating the position
fields before calling playerRef.update: ensure primaryPosition and position are
synchronized (if one is provided prefer primaryPosition or fallback
deterministically to position), dedupe secondaryPositions and remove any entry
equal to the resolved primary/position, normalize empty arrays/nulls to
undefined when appropriate, and only spread the sanitized primaryPosition,
position, and secondaryPositions into the update payload (alongside updatedAt)
so invariants are enforced regardless of caller input.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
eslint.config.mjssrc/app/api/account/pin/route.tssrc/app/api/billing/create-checkout-session/route.tssrc/app/api/debug/biometrics/[playerId]/route.tssrc/app/api/debug/workout-logs/[playerId]/route.tssrc/app/api/games/route.tssrc/app/api/players/[id]/assessments/[assessmentId]/route.tssrc/app/api/players/[id]/assessments/route.tssrc/app/api/players/[id]/biometrics/[logId]/route.tssrc/app/api/players/[id]/biometrics/route.tssrc/app/api/players/[id]/cardio-logs/[logId]/route.tssrc/app/api/players/[id]/cardio-logs/route.tssrc/app/api/players/[id]/dream-gym/ai-strategy/route.tssrc/app/api/players/[id]/dream-gym/check-in/route.tssrc/app/api/players/[id]/dream-gym/events/[eventId]/route.tssrc/app/api/players/[id]/dream-gym/events/route.tssrc/app/api/players/[id]/dream-gym/route.tssrc/app/api/players/[id]/dream-gym/workout-logs/[logId]/route.tssrc/app/api/players/[id]/dream-gym/workout-logs/route.tssrc/app/api/players/[id]/journal/[entryId]/route.tssrc/app/api/players/[id]/journal/route.tssrc/app/api/players/[id]/practice-logs/[logId]/route.tssrc/app/api/players/[id]/practice-logs/route.tssrc/app/api/players/[id]/route.test.tssrc/app/api/players/[id]/route.tssrc/app/api/players/create/route.tssrc/app/api/players/route.tssrc/app/api/storage/delete-player-photo/route.tssrc/app/api/storage/upload-player-photo/route.tssrc/app/api/verify/route.tssrc/app/dashboard/add-athlete/page.tsxsrc/app/dashboard/athletes/[id]/edit/page.tsxsrc/app/login/page.tsxsrc/lib/auth.tssrc/lib/firebase/admin-services/players.tssrc/lib/firebase/storage-utils.tssrc/lib/firebase/storage.ts
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+52
to
60
| async function getSessionCookieFromHeaders(): Promise<string | null> { | ||
| const cookieStore = await cookies(); | ||
| return cookieStore.get('__session')?.value || null; | ||
| return cookieStore.get(SESSION_COOKIE_NAME)?.value || null; | ||
| } | ||
|
|
||
| async function getSessionCookie(request?: NextRequest): Promise<string | null> { | ||
| if (request) return getSessionCookieFromRequest(request); | ||
| return getSessionCookieFromHeaders(); | ||
| } |
There was a problem hiding this comment.
1. Authwithprofile not request-scoped 🐞 Bug ⛯ Reliability
Multiple API routes still call authWithProfile() without passing the NextRequest, causing authWithProfile() to fall back to cookies() from next/headers. This likely reintroduces the same class of cookie/request-stream instability the PR is trying to fix, leaving some authenticated endpoints prone to hangs/timeouts.
Agent Prompt
## Issue description
Several API routes still call `authWithProfile()` without passing the `NextRequest`, which forces auth to fall back to `cookies()` from `next/headers`. This undermines the PR’s “read cookies from NextRequest” stabilization and may leave these routes vulnerable to the same hang/instability.
## Issue Context
`authWithProfile(request?: NextRequest)` is designed to use `request.cookies` when a request is provided and fall back to `cookies()` otherwise.
## Fix Focus Areas
- src/app/api/billing/change-plan/route.ts[32-42]
- src/app/api/billing/invoices/route.ts[27-50]
- src/app/api/billing/portal/route.ts[27-50]
- src/app/api/billing/create-portal-session/route.ts[24-35]
- src/app/api/admin/billing/replay-events/route.ts[106-116]
- src/app/api/workspace/current/route.ts[10-26]
- src/lib/auth.ts[52-60]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
| Filename | Overview |
|---|---|
| src/lib/auth.ts | Added timeout protection to Firebase auth operations, request-based cookie reading, and automatic user provisioning fallback to prevent sign-in hangs |
| src/app/api/players/[id]/route.ts | Switched to Zod validation and now properly persists all player position fields (primary, secondary, positionNote) plus league and gender data |
| src/app/dashboard/add-athlete/page.tsx | Fixed photo upload to use correct endpoint and FormData key ('file'), added logic to filter primary position from secondary positions on change |
| src/app/dashboard/athletes/[id]/edit/page.tsx | Fixed photo upload endpoint and FormData key, added position consistency logic matching add-athlete page |
| src/lib/firebase/storage-utils.ts | New isomorphic utility module for storage validation and limits, safely importable from both client and server |
| src/app/api/storage/upload-player-photo/route.ts | Migrated to admin-storage helpers and admin-services for server-side storage operations, maintains proper access control |
| src/app/login/page.tsx | Added timeout protection to getIdToken call to prevent indefinite hangs during authentication |
| src/app/api/players/[id]/route.test.ts | Updated tests to reflect new validation schema and position persistence fields, changed to resetAllMocks for cleaner test isolation |
Last reviewed commit: 69670b5
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
✅ Synthetic QA Passed! Fake humans are happy. All critical user journeys working:
Safe to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 64-68: The documentation currently implies
03-Tests/e2e/.auth/user.json is a tracked file; update the CLAUDE.md E2E details
to state clearly that the authenticated storage state
(03-Tests/e2e/.auth/user.json) is generated at runtime by the global setup
script (03-Tests/e2e/global-setup.ts) and that the .auth/ directory is
git-ignored (created locally, not source-controlled), so developers must run the
global setup or tests to produce the file; reference global-setup.ts and the
.auth/user.json path in the text to make this explicit.
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
|
✅ Synthetic QA Passed! Fake humans are happy. All critical user journeys working:
Safe to merge. |
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
jeremylongshore
force-pushed
the
fix/signin-hang-positions
branch
from
7f605b5 to
783046d
Compare
|
✅ Synthetic QA Passed! Fake humans are happy. All critical user journeys working:
Safe to merge. |
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
There was a problem hiding this comment.
Actionable comments posted: 1
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/storage/upload-player-photo/route.ts (1)
111-147:⚠️ Potential issue | 🟠 MajorDo not delete the existing photo before new upload + DB update succeeds.
Current ordering can leave the player pointing to a deleted image (or leave orphaned state) when downstream steps fail.
🔧 Suggested sequencing change
- // 7. Delete old photo if exists - let oldPhotoSizeMB = 0; - if (player.photoUrl) { - const oldPath = extractStoragePath(player.photoUrl); - if (oldPath) { - try { - oldPhotoSizeMB = await deletePlayerPhotoAdmin(oldPath); - logger.info('Replaced old player photo', { - userId, - playerId, - oldPath, - oldSize: oldPhotoSizeMB, - }); - } catch (error) { - logger.warn('Failed to delete old player photo', { - userId, - playerId, - oldPath, - error, - }); - } - } - } - - // 8. Upload new photo - const uploadResult = await uploadPlayerPhotoAdmin({ file, userId, playerId }); - - // 9. Update player photoUrl in Firestore - await updatePlayerAdmin(userId, playerId, { - photoUrl: uploadResult.url, - }); + // 7. Capture old photo path (delete only after successful replace) + const oldPath = player.photoUrl ? extractStoragePath(player.photoUrl) : null; + + // 8. Upload new photo + const uploadResult = await uploadPlayerPhotoAdmin({ file, userId, playerId }); + + // 9. Persist new URL first + await updatePlayerAdmin(userId, playerId, { photoUrl: uploadResult.url }); + + // 10. Best-effort old photo cleanup after successful update + let oldPhotoSizeMB = 0; + if (oldPath) { + try { + oldPhotoSizeMB = await deletePlayerPhotoAdmin(oldPath); + } catch (error) { + logger.warn('Failed to delete old player photo', { userId, playerId, oldPath, error }); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/storage/upload-player-photo/route.ts` around lines 111 - 147, The code currently deletes the old photo before the new upload and DB update, risking broken references; instead, first call uploadPlayerPhotoAdmin(...) and then updatePlayerAdmin(...) with uploadResult.url, only after those succeed perform deletion of the old photo (use extractStoragePath(player.photoUrl) to get oldPath) by calling deletePlayerPhotoAdmin(oldPath) and use the returned old size to compute netStorageChange = uploadResult.size - oldPhotoSizeMB, then call updateWorkspaceStorageUsageAdmin(workspace.id, netStorageChange); update logging around upload, DB update, and deletion (functions: uploadPlayerPhotoAdmin, updatePlayerAdmin, deletePlayerPhotoAdmin, updateWorkspaceStorageUsageAdmin, extractStoragePath).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/firebase/storage.ts`:
- Around line 21-30: Update the re-exports that currently reference
'./storage-utils' to use the repo path alias (replace './storage-utils' with the
'@/...' alias) so the exported symbols STORAGE_LIMITS, ALLOWED_IMAGE_TYPES,
validateFile, extractStoragePath, getRemainingStorage, getStorageUsagePercentage
and the ValidationResult type are re-exported from the aliased module; locate
the export block that lists those symbols and change the module specifier to the
equivalent '@/...' import path per coding guidelines.
---
Outside diff comments:
In `@src/app/api/storage/upload-player-photo/route.ts`:
- Around line 111-147: The code currently deletes the old photo before the new
upload and DB update, risking broken references; instead, first call
uploadPlayerPhotoAdmin(...) and then updatePlayerAdmin(...) with
uploadResult.url, only after those succeed perform deletion of the old photo
(use extractStoragePath(player.photoUrl) to get oldPath) by calling
deletePlayerPhotoAdmin(oldPath) and use the returned old size to compute
netStorageChange = uploadResult.size - oldPhotoSizeMB, then call
updateWorkspaceStorageUsageAdmin(workspace.id, netStorageChange); update logging
around upload, DB update, and deletion (functions: uploadPlayerPhotoAdmin,
updatePlayerAdmin, deletePlayerPhotoAdmin, updateWorkspaceStorageUsageAdmin,
extractStoragePath).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
CLAUDE.mdeslint.config.mjssrc/app/api/account/pin/route.tssrc/app/api/billing/create-checkout-session/route.tssrc/app/api/debug/biometrics/[playerId]/route.tssrc/app/api/debug/workout-logs/[playerId]/route.tssrc/app/api/games/route.tssrc/app/api/players/[id]/assessments/[assessmentId]/route.tssrc/app/api/players/[id]/assessments/route.tssrc/app/api/players/[id]/biometrics/[logId]/route.tssrc/app/api/players/[id]/biometrics/route.tssrc/app/api/players/[id]/cardio-logs/[logId]/route.tssrc/app/api/players/[id]/cardio-logs/route.tssrc/app/api/players/[id]/dream-gym/ai-strategy/route.tssrc/app/api/players/[id]/dream-gym/check-in/route.tssrc/app/api/players/[id]/dream-gym/events/[eventId]/route.tssrc/app/api/players/[id]/dream-gym/events/route.tssrc/app/api/players/[id]/dream-gym/route.tssrc/app/api/players/[id]/dream-gym/workout-logs/[logId]/route.tssrc/app/api/players/[id]/dream-gym/workout-logs/route.tssrc/app/api/players/[id]/journal/[entryId]/route.tssrc/app/api/players/[id]/journal/route.tssrc/app/api/players/[id]/practice-logs/[logId]/route.tssrc/app/api/players/[id]/practice-logs/route.tssrc/app/api/players/[id]/route.test.tssrc/app/api/players/[id]/route.tssrc/app/api/players/create/route.tssrc/app/api/players/route.tssrc/app/api/storage/delete-player-photo/route.tssrc/app/api/storage/upload-player-photo/route.tssrc/app/api/verify/route.tssrc/app/dashboard/add-athlete/page.tsxsrc/app/dashboard/athletes/[id]/edit/page.tsxsrc/lib/auth.tssrc/lib/firebase/admin-services/players.tssrc/lib/firebase/storage-utils.tssrc/lib/firebase/storage.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- src/app/api/players/[id]/dream-gym/ai-strategy/route.ts
- src/app/api/players/[id]/dream-gym/route.ts
- src/app/api/account/pin/route.ts
- src/app/api/players/[id]/dream-gym/workout-logs/[logId]/route.ts
- src/app/api/games/route.ts
- src/lib/firebase/storage-utils.ts
- src/app/dashboard/add-athlete/page.tsx
- eslint.config.mjs
- src/app/api/players/create/route.ts
- src/app/api/verify/route.ts
- src/app/api/billing/create-checkout-session/route.ts
- src/app/api/players/[id]/assessments/[assessmentId]/route.ts
- src/app/api/players/[id]/practice-logs/route.ts
- src/app/api/players/[id]/dream-gym/workout-logs/route.ts
- src/app/api/players/[id]/practice-logs/[logId]/route.ts
- src/app/api/players/[id]/cardio-logs/route.ts
- src/app/api/players/[id]/journal/[entryId]/route.ts
- src/app/api/debug/biometrics/[playerId]/route.ts
- src/app/api/players/[id]/cardio-logs/[logId]/route.ts
- src/app/api/players/[id]/biometrics/route.ts
- src/app/api/players/[id]/dream-gym/events/[eventId]/route.ts
Comment on lines
+21
to
30
| export { | ||
| STORAGE_LIMITS, | ||
| ALLOWED_IMAGE_TYPES, | ||
| validateFile, | ||
| extractStoragePath, | ||
| getRemainingStorage, | ||
| getStorageUsagePercentage, | ||
| } from './storage-utils'; | ||
| export type { ValidationResult } from './storage-utils'; | ||
|
|
There was a problem hiding this comment.
Use @/ alias for the new re-exports.
These re-exports currently use a relative path and should follow the repo import-path rule.
♻️ Suggested change
export {
STORAGE_LIMITS,
ALLOWED_IMAGE_TYPES,
validateFile,
extractStoragePath,
getRemainingStorage,
getStorageUsagePercentage,
-} from './storage-utils';
-export type { ValidationResult } from './storage-utils';
+} from '@/lib/firebase/storage-utils';
+export type { ValidationResult } from '@/lib/firebase/storage-utils';As per coding guidelines: **/*.{ts,tsx,js,jsx}: Use @/ path alias (maps to src/) for all imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/firebase/storage.ts` around lines 21 - 30, Update the re-exports that
currently reference './storage-utils' to use the repo path alias (replace
'./storage-utils' with the '@/...' alias) so the exported symbols
STORAGE_LIMITS, ALLOWED_IMAGE_TYPES, validateFile, extractStoragePath,
getRemainingStorage, getStorageUsagePercentage and the ValidationResult type are
re-exported from the aliased module; locate the export block that lists those
symbols and change the module specifier to the equivalent '@/...' import path
per coding guidelines.
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
jeremylongshore
force-pushed
the
fix/signin-hang-positions
branch
from
4d40b50 to
08a9549
Compare
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
…cations Document missing integration test commands and config, add mobile app stack details (Expo Router, NativeWind, React Query), fix unit test location accuracy, and add minor missing details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeremylongshore
force-pushed
the
fix/signin-hang-positions
branch
from
08a9549 to
2fb3d95
Compare
|
Deployed to staging: https://hustle-app-staging-d4f2hb75nq-uc.a.run.app |
|
Smoke tests ✅ Passed |
Merged
5 tasks
jeremylongshore
added a commit
that referenced
this pull request
Mar 9, 2026
* fix(firebase): prevent sign-in hangs and fix athlete positions * docs(CLAUDE.md): add integration tests, mobile stack, and fix test locations Document missing integration test commands and config, add mobile app stack details (Expo Router, NativeWind, React Query), fix unit test location accuracy, and add minor missing details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ci: disable all cron schedules to stop burning Actions minutes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: jeremylongshore <noreply@localhost.local> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
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.
Summary\n- Stabilize server auth by reading cookies from NextRequest and adding timeouts + provisioning fallback.\n- Fix player update endpoint to persist primary/secondary positions and related profile fields.\n- Fix athlete add/edit photo upload to use /api/storage/upload-player-photo (correct FormData key).\n- Keep secondary positions consistent when primary position changes.\n\n## Testing\n- npm run test:unit\n- npm run lint\n
Summary by CodeRabbit
New Features
Bug Fixes
Chores