Reduce fetching during ssr #1304
Conversation
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 #1304 +/- ##
==========================================
+ Coverage 61.98% 66.31% +4.33%
==========================================
Files 150 154 +4
Lines 6355 6460 +105
Branches 1577 1591 +14
==========================================
+ Hits 3939 4284 +345
+ Misses 2259 2037 -222
+ Partials 157 139 -18
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/with-apollo-client/get-apollo.ts (1)
300-302: AddskipSsrFetchparameter toinitNewApollo.The
initNewApollofunction should accept and propagate theskipSsrFetchparameter to maintain consistency with the underlyingcreate()function and thegetApollo()initialization function, which both support this parameter.Update the function signature and call:
export function initNewApollo(accessToken?: string, locale?: string, skipSsrFetch?: boolean) { apolloClient = create(undefined, accessToken, locale, skipSsrFetch) return apolloClient }
🤖 Fix all issues with AI agents
In @frontend/lib/with-apollo-client/index.tsx:
- Around line 72-78: The current logic sets skipSsrFetch = typeof window ===
"undefined" which forces skipping all SSR GraphQL fetches for unauthenticated
users; confirm intent and, if SEO/public content must be server-rendered, change
the condition or introduce a separate flag so skipSsrFetch is false for public
pages during server rendering. Locate the getApollo call (getApollo(apolloState,
accessToken, ctx.locale, skipSsrFetch)) and ensure apollo is initialized with
server fetch enabled when the route/page is public (e.g., check route or a new
isPublicPage parameter) so apolloState and apollo.toJSON() contain SSR data for
crawlers while still skipping user-private queries for unauthenticated users.
🧹 Nitpick comments (2)
frontend/lib/with-apollo-client/get-apollo.ts (1)
268-270: Consider passing skipSsrFetch consistently for future-proofing.Line 269 doesn't pass
skipSsrFetchwhen reusing or creating the client on the browser, while line 270 does for server-side or changed-state scenarios. AlthoughskipSsrFetchcurrently only affects server-side link selection and has no impact on browser clients, passing it consistently would improve maintainability and prevent potential issues if the browser path logic evolves.♻️ Proposed consistency improvement
const _apolloClient = !userChanged && !localeChanged && isBrowser - ? apolloClient ?? create(initialState, accessToken, locale) + ? apolloClient ?? create(initialState, accessToken, locale, skipSsrFetch) : create(undefined, accessToken, locale, skipSsrFetch)frontend/lib/with-apollo-client/index.tsx (1)
107-107: Consider supporting skipSsrFetch for authenticated users for consistency.The authenticated flow doesn't use or propagate
skipSsrFetch, meaning authenticated users always perform full SSR data fetching. While this may be intentional (the optimization targets unauthenticated users), it creates an asymmetry in the API where the flag is only meaningful for one flow.For consistency and flexibility, consider whether authenticated users should also have the option to skip SSR fetches in certain scenarios.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/lib/with-apollo-client/get-apollo.tsfrontend/lib/with-apollo-client/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/lib/with-apollo-client/index.tsx (1)
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). (3)
- GitHub Check: CodeQL
- GitHub Check: Analyze (javascript)
- GitHub Check: build-deploy
🔇 Additional comments (5)
frontend/lib/with-apollo-client/get-apollo.ts (3)
78-78: LGTM: Parameter addition is correct.The
skipSsrFetchparameter is properly typed and positioned.
226-228: LGTM: Server-side link selection is correct.The conditional logic correctly uses
skipSsrFetchLinkwhenskipSsrFetchis true on the server, completely bypassing the normal link chain. This is the expected behavior for skipping SSR fetches.
210-215: This pattern is an intentional optimization, not a problem.The
skipSsrFetchLinkis used only for unsigned users during server-side rendering to skip network calls. It returns null data so the page renders quickly. The client then receives an empty Apollo cache and uses real network links to fetch data after hydration. This is a standard SSR optimization pattern.Apollo hooks natively handle null/loading states, and the client automatically fetches data when needed. No changes required.
frontend/lib/with-apollo-client/index.tsx (2)
18-18: LGTM: Props interface updated correctly.The
skipSsrFetchproperty is properly added as an optional boolean.
31-42: LGTM: Component correctly propagates skipSsrFetch.The
skipSsrFetchprop is properly destructured and passed togetApolloduring client initialization.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frontend/pages/index.tsx:
- Line 8: The home page is currently wrapped with withNoSsr which disables
server-side rendering and can hurt SEO/UX/perf; verify this is intentional and
if not, remove the withNoSsr wrapper around the page component (or only apply it
to the specific client-only subcomponents) and instead use the Apollo client
skipSsrFetch option from frontend/lib/with-apollo-client to skip problematic
queries during SSR; if you decide to keep withNoSsr, add a code comment
explaining why SSR is disabled for the page and document which component or
query required it (reference withNoSsr and skipSsrFetch in your change).
In @frontend/pages/study-modules/index.tsx:
- Line 3: The page currently wraps the Study Modules page with withNoSsr which
disables SSR entirely; instead change the implementation to keep SSR but skip
only the data fetch by using the skipSsrFetch mechanism: remove/stop using
withNoSsr on the default export (or component wrapper) and update the data
loading code (the fetch call or getServerSideProps/getStaticProps equivalent
used by pages/study-modules/index.tsx) to pass or check skipSsrFetch so the
component renders during SSR while deferring the expensive data fetch to the
client; reference the withNoSsr wrapper to remove and the skipSsrFetch
flag/function used by your data fetching logic to gate server-side fetching.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/pages/index.tsxfrontend/pages/study-modules/index.tsx
⏰ 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
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.