Stop using getIntialProps #1305
Conversation
📝 WalkthroughWalkthroughThis refactoring converts the authentication system from server-side rendering with getInitialProps to a client-side hook-based pattern. A new useAuth hook centralizes authentication state, Higher-Order Components shift to functional implementations, and Emotion SSR integration is removed entirely. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1305 +/- ##
=======================================
Coverage 66.31% 66.31%
=======================================
Files 154 154
Lines 6460 6460
Branches 1591 1591
=======================================
Hits 4284 4284
Misses 2037 2037
Partials 139 139
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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)
frontend/lib/authentication.ts (1)
112-116: Unsafe type assertion on error path.Returning
{} as UserInfowhen the fetch fails will cause runtime errors when callers access properties likedetails.administrator(line 43). Consider throwing an error or returning a typed failure result.Suggested fix
if (res.ok) { return res.json() } - return {} as UserInfo + throw new Error(`Failed to fetch user details: ${res.status} ${res.statusText}`)Alternatively, handle the error in
signInto provide a better user experience.
🤖 Fix all issues with AI agents
In @frontend/pages/_app.tsx:
- Around line 22-24: The MyAppProps interface declares deviceType which is never
provided (getInitialProps was removed), so either remove deviceType from
MyAppProps and the MyApp component signature and stop passing it into
useThemeWithLocale, updating any call sites to useThemeWithLocale() accordingly,
or implement a device-detection source: e.g., detect on the client with a
useEffect + state (window.innerWidth / navigator.userAgent) or derive it
server-side (middleware/API) and provide that value via pageProps before calling
useThemeWithLocale(deviceType); update references to MyAppProps, the MyApp
function, and the useThemeWithLocale calls to match the chosen approach.
🧹 Nitpick comments (5)
frontend/pages/_old/email-templates/index.tsx (1)
39-43: Consider removing unused props from the interface.The
EmailTemplatesPropsinterface definessignedInandbaseUrl, but the component doesn't use these props. Line 47 computesbaseUrllocally using theuseIsOld()hook instead of consuming the prop. Consider removing unused props to keep the interface clean.♻️ Proposed cleanup
interface EmailTemplatesProps { admin?: boolean - signedIn?: boolean - baseUrl?: string }frontend/hooks/useAuth.ts (1)
28-55: Consider request cancellation and fetch policy implications.Two observations:
- Missing cleanup: If the component unmounts while the user query is in flight, the state update at line 42 or 45 will trigger a React warning. Consider adding cleanup logic:
useEffect(() => { let cancelled = false const checkAuth = async () => { const isUserSignedIn = isSignedIn() const isUserAdmin = isAdmin() setSignedIn(isUserSignedIn) setAdmin(isUserAdmin) if (isUserSignedIn) { try { const { data } = await apolloClient.query<CurrentUserDetailedQuery>({ query: CurrentUserDetailedDocument, fetchPolicy: "network-only", }) if (!cancelled) { setCurrentUser(data.currentUser ?? null) } } catch (error) { console.error("Failed to fetch current user:", error) if (!cancelled) { setCurrentUser(null) } } } else { setCurrentUser(null) } if (!cancelled) { setLoading(false) } } checkAuth() return () => { cancelled = true } }, [apolloClient])
- Fetch policy: The
"network-only"policy (line 40) bypasses the cache entirely, which could impact performance if this hook is used frequently. Consider"cache-first"or"cache-and-network"unless you have a specific reason to always fetch fresh data.frontend/lib/with-apollo-client/index.tsx (1)
28-28: Minor inconsistency in displayName fallback.Other HOCs in this PR use
Component.name ?? Component.displayName ?? "AnonymousComponent"pattern. Consider aligning for consistency.Suggested fix
- WithApollo.displayName = `withApollo(${App.displayName ?? "App"})` + WithApollo.displayName = `withApollo(${App.name ?? App.displayName ?? "App"})`frontend/lib/with-signed-in.tsx (1)
13-13: Consider more specific path matching for baseUrl.Using
includes("_old")could match unintended paths (e.g.,/pages/sold_old_items). A more precise check likestartsWith("/_old")would be safer.Suggested fix
- const baseUrl = router.pathname.includes("_old") ? "/_old" : "" + const baseUrl = router.pathname.startsWith("/_old") ? "/_old" : ""frontend/lib/authentication.ts (1)
77-88: Consider documenting or refactoring the nested setTimeout pattern.The nested
setTimeoutcalls create a 200ms total delay beforeapollo.resetStore(). While the comment explains the intent, this pattern is fragile and could lead to race conditions. Consider using a single delay or a more robust approach in a follow-up.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/hooks/useAuth.tsfrontend/lib/authentication.tsfrontend/lib/with-admin.tsxfrontend/lib/with-apollo-client/index.tsxfrontend/lib/with-signed-in.tsxfrontend/lib/with-signed-out.tsxfrontend/pages/_app.tsxfrontend/pages/_document.tsxfrontend/pages/_old/email-templates/index.tsxfrontend/src/createEmotionSsr.tsx
💤 Files with no reviewable changes (1)
- frontend/src/createEmotionSsr.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/lib/with-signed-out.tsx (2)
frontend/lib/redirect.ts (1)
redirect(12-42)frontend/hooks/useAuth.ts (1)
useAuth(20-58)
frontend/pages/_app.tsx (1)
frontend/hooks/useAuth.ts (1)
useAuth(20-58)
frontend/lib/with-signed-in.tsx (1)
frontend/hooks/useAuth.ts (1)
useAuth(20-58)
frontend/lib/with-apollo-client/index.tsx (2)
frontend/lib/authentication.ts (1)
getAccessToken(90-93)frontend/lib/with-apollo-client/get-apollo.ts (1)
getApollo(255-298)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build-deploy
🔇 Additional comments (13)
frontend/pages/_document.tsx (1)
12-33: LGTM: Simplified document structure.The change from custom props to standard
DocumentPropsappropriately removes SSR-related plumbing (apolloState, Emotion cache) while preserving theme selection and font class logic. This aligns well with the PR's shift to client-side authentication.frontend/pages/_old/email-templates/index.tsx (1)
45-109: LGTM: Props refactoring aligns with HOC pattern.The component correctly destructures
adminfrom props and maintains the admin check. While thewithAdminHOC should already ensure admin access, the defensive check at line 65-67 is acceptable.frontend/hooks/useAuth.ts (1)
1-18: LGTM: Clean interface and imports.The
UseAuthResultinterface clearly defines the authentication state contract. Imports are appropriate for the hook's functionality.frontend/lib/with-admin.tsx (4)
1-12: LGTM: Clean type-safe HOC signature.The function signature with generic constraints properly types the wrapped component's props, ensuring type safety for the injected authentication props.
13-22: LGTM: Redirect logic correctly handles authentication.The
useEffectproperly waits for loading to complete before redirecting unauthenticated users. The dependency array is correct.
24-34: LGTM: Proper loading and authorization checks.The sequential checks (loading → redirecting → admin validation) provide appropriate user feedback during authentication flows.
36-50: LGTM: Clean props injection and displayName handling.The component correctly passes through all props and injects authentication state. The
displayNamesetting improves debugging experience.frontend/pages/_app.tsx (2)
26-50: LGTM: Clean integration of useAuth hook.The component correctly sources authentication state from the
useAuthhook and constructs the login state context value. ThecurrentUser ?? undefinedconversion (line 47) appropriately handles the null-to-undefined mapping for the context.
92-92: LGTM: Simplified export aligns with SSR removal.The removal of the Emotion SSR wrapper (
withAppEmotionCache) and retention of onlywithApolloClientcorrectly implements the PR's goal of removing server-side rendering complexity.frontend/lib/with-signed-out.tsx (1)
1-37: LGTM! Clean refactoring to hook-based pattern.The implementation correctly handles the authentication flow:
- Loading state prevents premature redirects
- useEffect triggers redirect when signed in
- Fallback "Redirecting..." UI prevents flash of wrapped component
frontend/lib/with-apollo-client/index.tsx (1)
14-14: Potential stale closure issue with accessToken.
getAccessToken(undefined)is called during render, but the value is captured in useMemo dependencies. If the access token cookie changes (e.g., after sign-in/sign-out), the Apollo client will be recreated correctly. However, this relies on a re-render being triggered externally.Consider whether authentication state changes (from
useAuthin other components) will trigger re-renders here to pick up new tokens.frontend/lib/with-signed-in.tsx (1)
15-29: LGTM! Authentication guard logic is correct.The redirect flow properly handles:
- Loading state to prevent premature actions
- Redirect trigger via useEffect when not signed in
- Clean UI feedback during transitions
frontend/lib/authentication.ts (1)
13-17: LGTM! Clean refactoring to support optional context.The
nookies.get(ctx)correctly handles both server-side (with context) and client-side (undefined context) scenarios. The strict equality checktypeof accessToken === "string"is appropriate.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.