fix: gate onlineApi requests on auth state (Rollbar #91)#92
Merged
Conversation
…rash When a user opens the account deletion modal and their Firebase auth session expires before confirming, getIdToken() throws "No user is signed in." which was logged to Rollbar as an unhandled error (Rollbar #91). Adds an authRequestInterceptor that pre-checks getCurrentUser() before every onlineApi request. If no user is found, it syncs the Zustand store and throws NotSignedInError. The deletion and cancel-deletion handlers now catch NotSignedInError specifically, close the modal, and show a "no longer signed in" toast without logging to Rollbar.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an authentication-state check to the Axios request interceptor that throws a new Changes
Sequence Diagram(s)sequenceDiagram
participant Axios as Axios (request)
participant Interceptor as authRequestInterceptor
participant Firebase as FirebaseAuthentication
participant Store as useStatusStore
Axios->>Interceptor: request config
Interceptor->>Firebase: getCurrentUser()
alt user exists
Interceptor->>Firebase: getIdToken()
Firebase-->>Interceptor: idToken
Interceptor->>Axios: set Authorization: Bearer <token>
else no user
Interceptor->>Store: getState().loggedInUser
alt loggedInUser != null
Interceptor->>Store: setLoggedInUser(null)
end
Interceptor-->>Axios: throw NotSignedInError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (2)
src/__tests__/unit/routes/settings.online.test.tsx (1)
1074-1077: Make modal-close assertion async-safe to reduce flakiness.The absence check can race UI commit; wrap the
queryByTextassertion inwaitForafter the rejection flow.🧪 Suggested stability tweak
- expect( - screen.queryByText("online.deleteAccountConfirmTitle"), - ).not.toBeInTheDocument(); + await waitFor(() => { + expect( + screen.queryByText("online.deleteAccountConfirmTitle"), + ).not.toBeInTheDocument(); + });As per coding guidelines, "
**/*.test.tsx: Never use hardcoded delays - usewaitFororfindBy*for async operations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/unit/routes/settings.online.test.tsx` around lines 1074 - 1077, The assertion that the modal is closed is racing the UI commit; update the test to use async-safe waitFor checks instead of immediate query assertions: wrap the check for absence of "online.deleteAccountConfirmTitle" in a waitFor (e.g., waitFor(() => expect(screen.queryByText("online.deleteAccountConfirmTitle")).not.toBeInTheDocument())) and similarly assert logger.error is not called inside a waitFor (waitFor(() => expect(logger.error).not.toHaveBeenCalled())), referencing the existing use of screen.queryByText and logger.error in this test.src/__tests__/unit/lib/onlineApi.test.ts (1)
515-531: Add a test for the already-null store guard branch.The interceptor intentionally skips
setLoggedInUser(null)whenloggedInUseris alreadynull; adding that assertion will lock in the anti-churn behavior introduced insrc/lib/onlineApi.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/unit/lib/onlineApi.test.ts` around lines 515 - 531, The test should cover the branch where the store already has loggedInUser === null so the interceptor does not call setLoggedInUser(null); update the unit test for authRequestInterceptor to mock useStatusStore.getState to return { loggedInUser: null, setLoggedInUser: mockSetLoggedInUser } (keep FirebaseAuthentication.getCurrentUser mocked to return { user: null }), call authRequestInterceptor and assert that mockSetLoggedInUser was NOT called, ensuring the early-guard anti-churn behavior in authRequestInterceptor is locked in.
🤖 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/__tests__/unit/lib/onlineApi.test.ts`:
- Around line 485-512: The tests for authRequestInterceptor use unsafe `as any`
casts—update them to use proper types: replace the mocked store state cast with
a typed literal using `satisfies` matching your store state shape (targeting
useStatusStore.getState), type the Axios config with
`InternalAxiosRequestConfig` when calling authRequestInterceptor, and make
Firebase mocks return correctly typed shapes for
FirebaseAuthentication.getCurrentUser and FirebaseAuthentication.getIdToken
(e.g., typed promise results instead of `as any`); ensure the NotSignedInError
assertion remains the same and remove all `as any` usages.
---
Nitpick comments:
In `@src/__tests__/unit/lib/onlineApi.test.ts`:
- Around line 515-531: The test should cover the branch where the store already
has loggedInUser === null so the interceptor does not call
setLoggedInUser(null); update the unit test for authRequestInterceptor to mock
useStatusStore.getState to return { loggedInUser: null, setLoggedInUser:
mockSetLoggedInUser } (keep FirebaseAuthentication.getCurrentUser mocked to
return { user: null }), call authRequestInterceptor and assert that
mockSetLoggedInUser was NOT called, ensuring the early-guard anti-churn behavior
in authRequestInterceptor is locked in.
In `@src/__tests__/unit/routes/settings.online.test.tsx`:
- Around line 1074-1077: The assertion that the modal is closed is racing the UI
commit; update the test to use async-safe waitFor checks instead of immediate
query assertions: wrap the check for absence of
"online.deleteAccountConfirmTitle" in a waitFor (e.g., waitFor(() =>
expect(screen.queryByText("online.deleteAccountConfirmTitle")).not.toBeInTheDocument()))
and similarly assert logger.error is not called inside a waitFor (waitFor(() =>
expect(logger.error).not.toHaveBeenCalled())), referencing the existing use of
screen.queryByText and logger.error in this test.
🪄 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
Run ID: 688b6639-43a2-4884-a307-84121ea9fe8f
📒 Files selected for processing (5)
src/__tests__/unit/lib/onlineApi.test.tssrc/__tests__/unit/routes/settings.online.test.tsxsrc/lib/onlineApi.tssrc/routes/settings.online.tsxsrc/translations/en-US.json
- Replace as any casts with proper types: InternalAxiosRequestConfig for config args, satisfies GetCurrentUserResult/GetIdTokenResult for Firebase mocks, and satisfies PartialStoreState for store mocks - Add test covering the loggedInUser === null guard (interceptor must not call setLoggedInUser when store is already null) - Wrap post-waitFor assertions in their own waitFor to avoid UI commit races
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.
authRequestInterceptor(exported named function) toonlineApi.tsthat callsFirebaseAuthentication.getCurrentUser()before every request. If no user is found, it syncs the Zustand store to null and throwsNotSignedInError— preventing the Firebase "No user is signed in." error from surfacing as an unhandled Rollbar event.handleDeleteAccountandhandleCancelDeletioninsettings.online.tsxnow catchNotSignedInErrorexplicitly: the delete modal closes, a "no longer signed in" toast is shown, and nothing is logged to Rollbar (expected/recoverable condition).loggedInUser !== nullcheck.authRequestInterceptordirectly rather than capturing it through the axios mock, eliminating a brittle ESM-caching dependency.Summary by CodeRabbit
New Features
Bug Fixes
Tests