fix(apollo): migrate to Apollo Client v4 breaking changes#171
fix(apollo): migrate to Apollo Client v4 breaking changes#171
Conversation
- Move React hooks (useQuery, useMutation, etc.) from @apollo/client to @apollo/client/react - Replace removed ApolloError with ServerError/CombinedGraphQLErrors - Update onError link handler for v4's unified error object - Replace connectToDevTools with devtools option - Handle v4's QueryResult.data being TData | undefined with non-null assertions - Replace removed onCompleted callback with useMemo-derived state - Create apollo-compat barrel module for codegen compatibility - Update codegen config with apolloReactHooksImportFrom - Fix ApolloClient no longer being generic - Add @ts-nocheck to generated file (codegen plugin not yet v4-compatible) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 focuses on a significant upgrade of the application's Apollo Client dependency to version 4. The changes encompass adapting to breaking API changes, such as revised import paths for React hooks, updated error handling mechanisms, and new configuration options for developer tools. A dedicated compatibility layer was introduced to ensure seamless integration with code generation, and server-side query result handling was refined to align with the new type definitions. This comprehensive update ensures the application leverages the latest features and improvements of Apollo Client. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the project to Apollo Client v4, addressing several breaking changes as outlined in the summary. Key updates include adjusting React hook imports to @apollo/client/react, updating error handling to use ServerError and CombinedGraphQLErrors, and modifying doApolloServerQuery to consistently return { data: TData }. The introduction of apollo-compat.ts is a pragmatic solution for maintaining codegen compatibility during this transition. Dependency versions have also been updated across the board, which is a necessary step for such a migration. Overall, the changes reflect a thorough understanding of the Apollo Client v4 migration path.
| - add: | ||
| content: '// @ts-nocheck' |
There was a problem hiding this comment.
Adding // @ts-nocheck to generated files can sometimes mask underlying type issues that might be resolvable. While it's a common practice for generated code, consider if there's a more specific way to address the type incompatibility or if the codegen configuration can be adjusted to produce type-safe output without needing to suppress all checks. This would improve the overall type safety and maintainability of the codebase.
| }) | ||
| const renderList = React.useMemo(() => { | ||
| const initial = data?.clippingList.items ?? [] | ||
| return [...initial, ...extraItems].reduce( | ||
| (acc, cur) => { | ||
| if (!acc.find((x) => x.id === cur.id)) { | ||
| acc.push(cur) | ||
| } | ||
| return acc | ||
| }, | ||
| [] as FetchClippingsByUidQuery['clippingList']['items'] | ||
| ) | ||
| }, [data, extraItems]) |
There was a problem hiding this comment.
The refactoring of renderList to use React.useMemo and extraItems for managing the list, along with the deduplication logic, is a good improvement for handling infinite loading and preventing duplicate entries. This makes the component's state management more robust and predictable.
|
Apollo Client v4 Migration Review Good overall migration - the breaking changes are handled systematically and build/tests pass. A few issues worth addressing: Bug: ServerError catch misses GraphQL-level auth errors In Issue: Orphaned import in The last line of Issue: Simplified The custom Note: Adding Note: Silent error regression in comments pages Previously callers of Note: Inconsistent null-safety approach Some call sites use optional chaining ( Note: Reduced error detail in Error display simplified to just Minor: Large scope of dependency bumps Many unrelated packages updated alongside the Apollo migration (Sentry, OpenTelemetry, Tiptap, Tailwind, etc.). If a regression appears post-merge, bisecting will be harder. Summary: The migration is solid and well-structured. The Reviewed with Claude Code |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 useMultipleBook only receives book IDs from the initial query page, not from paginated extraItems
In clipping-list.tsx, the useMultipleBook hook at line 75-77 only receives book IDs from data?.clippingList.items (the initial query result), not from extraItems (items loaded via fetchMore). The refactored renderList now includes items from both data and extraItems, but the book lookup only covers the initial page's book IDs. When rendering clippings loaded via pagination, books.books.find(...) at line 118-120 will fail to find books for those items, resulting in missing book covers/info for paginated content. This is a pre-existing issue, but the refactor makes it more visible since renderList now clearly combines two sources while useMultipleBook only uses one.
(Refers to lines 75-77)
Was this helpful? React with 👍 or 👎 to provide feedback.
| squareResponse.data!.featuredClippings | ||
| .map((x) => x.bookID) | ||
| .filter((x) => x.length > 3) ?? [] |
There was a problem hiding this comment.
🟡 Operator precedence makes ?? [] fallback ineffective — .map().filter() always returns an array
Across multiple files, the pattern data.data?.public.books.map(...).filter(...) ?? [] has an operator precedence issue. The ?? operator binds to the result of .filter(), which always returns an array (never null/undefined). So when data.data is defined, ?? [] is dead code. And when data.data is undefined (the new v4 possibility the ?. was added to handle), the optional chain short-circuits the entire .map().filter() chain to undefined, and ?? [] correctly provides the fallback. So the fallback works for the undefined case — but this is misleading because a developer might think ?? [] also guards against data.data.public being missing. More critically, this exact same pattern at src/app/dash/[userid]/square/page.tsx:34 and src/app/report/yearly-legacy/page.tsx:40 uses data!. (non-null assertion) instead of ?., which means the ?? [] fallback is completely dead code there — if data is somehow undefined, the ! assertion will throw before ?? [] ever gets evaluated.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (e instanceof ServerError) { | ||
| const statusCode = e.statusCode |
There was a problem hiding this comment.
🟡 DashboardContainer catches ServerError but doApolloServerQuery may wrap/re-throw differently
In DashboardContainer, the catch block was changed from e instanceof ApolloError (which wraps network errors) to e instanceof ServerError. However, DashboardContainer calls doApolloServerQuery() which has its own catch block that already handles ServerError with statusCode 401 by redirecting. For non-401 ServerErrors (like 404), doApolloServerQuery re-throws them via throw e. This should work, but there's a subtle issue: doApolloServerQuery uses .catch((e: any) => { ... throw e }) in a promise chain. If Apollo Client v4's query() rejects with a ServerError, the throw e preserves it. However, if the error is wrapped (e.g., by the link chain or cache layer), it may not be a direct ServerError instance, causing the 404 redirect in DashboardContainer to silently fail — the error would be re-thrown as unhandled instead of redirecting to auth.
Was this helpful? React with 👍 or 👎 to provide feedback.
| getApolloServerClient() | ||
| .query<TData, TVariables>(options) | ||
| .then((result) => ({ data: result.data as TData })) |
There was a problem hiding this comment.
🟡 doApolloServerQuery casts potentially undefined result.data to TData, masking runtime undefined values
In doApolloServerQuery, the .then((result) => ({ data: result.data as TData })) line uses as TData to cast result.data, which in Apollo Client v4 can be TData | undefined. If the query succeeds but data is undefined (e.g., with certain errorPolicy settings or edge cases), the as TData cast silently hides the undefined, and downstream code like profileResponse.data.me or booksResponse.data.books will throw at runtime with a confusing "Cannot read properties of undefined" error. The return type Promise<{ data: TData }> makes callers believe data is always defined, but the as cast doesn't provide runtime safety.
| .then((result) => ({ data: result.data as TData })) | |
| .then((result) => { | |
| if (result.data === undefined) { | |
| throw new Error('Query returned no data') | |
| } | |
| return { data: result.data as TData } | |
| }) |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
@apollo/client/react,ApolloErrorreplaced byServerError/CombinedGraphQLErrors,connectToDevTools→devtools,onCompletedcallback removed,ApolloClientno longer genericapollo-compat.tsbarrel module for codegen compatibility and updatecodegen.ymlto use itQueryResult.databeingTData | undefinedwith non-null assertions on server queries, and updatedoApolloServerQueryto return{ data: TData }Test plan
pnpm codegensucceedspnpm buildsucceeds (0 errors)pnpm testpasses (7/7)pnpm lintpasses (0 errors, warnings only pre-existing)/renders correctly🤖 Generated with Claude Code