Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚡️ Prefetch queries with trpc #1454

Merged
merged 2 commits into from
Dec 2, 2024
Merged

⚡️ Prefetch queries with trpc #1454

merged 2 commits into from
Dec 2, 2024

Conversation

lukevella
Copy link
Owner

@lukevella lukevella commented Dec 1, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced server-side rendering capabilities across multiple components, improving data fetching efficiency.
    • Introduced prefetching mechanisms for user subscriptions, polls, participants, and comments, optimizing the loading experience.
    • Added a new PermissionProvider for managing user permissions based on user ID.
  • Bug Fixes

    • Improved error handling for non-existent polls and rate limiting logic.
  • Documentation

    • Updated context structure to include client IP address for better user tracking and analytics.

Copy link

vercel bot commented Dec 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 2, 2024 0:45am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
landing ⬜️ Skipped (Inspect) Dec 2, 2024 0:45am

Copy link
Contributor

coderabbitai bot commented Dec 1, 2024

Walkthrough

The changes in this pull request enhance the server-side rendering capabilities of various components in a Next.js application by integrating React Query and tRPC for improved data fetching and state management. Key modifications include the addition of createSSRHelper, which facilitates server-side data prefetching, and the incorporation of Hydrate to manage query client state. Additionally, the context structure is updated to include the client's IP address, streamlining data retrieval processes and enhancing the overall architecture of the application.

Changes

File Path Change Summary
apps/web/src/app/[locale]/(admin)/layout.tsx Added imports for dehydrate, Hydrate, and createSSRHelper. Updated Layout function for server-side rendering with data prefetching and wrapped layout in Hydrate component.
apps/web/src/app/[locale]/(admin)/page.tsx Added imports for dehydrate, Hydrate, and createSSRHelper. Modified Page function to prefetch dashboard info and wrap content in Hydrate component.
apps/web/src/app/[locale]/invite/[urlId]/page.tsx Updated Page function signature to accept params and searchParams. Added createSSRHelper import and prefetching for user permissions based on token.
apps/web/src/app/[locale]/poll/[urlId]/layout.tsx Added createSSRHelper, dehydrate, and Hydrate imports. Updated Layout function to prefetch multiple queries and wrap PollLayout in Hydrate component.
apps/web/src/pages/api/trpc/[trpc].ts Added request-ip import to retrieve client IP. Updated createContext to include IP in the context returned to the TRPC handler.
apps/web/src/trpc/context.ts Modified TRPCContext type to remove req and res properties; added optional ip property.
apps/web/src/trpc/server/create-ssr-helper.ts Introduced createSSRHelper function for server-side helpers and added createContext function to manage user session and context creation.
apps/web/src/trpc/trpc.ts Removed requestIp import and updated IP retrieval logic to use ctx.ip. Streamlined rate limiting logic to check ctx.ip for client IP.
apps/web/src/app/[locale]/invite/[urlId]/layout.tsx Updated Layout function signature to accept params and utilize createSSRHelper for data prefetching, replacing the previous Prefetch component.
apps/web/src/app/[locale]/invite/[urlId]/providers.tsx Introduced Providers component to wrap children with LegacyPollContextProvider and VisibilityProvider.
apps/web/src/components/participants-provider.tsx Simplified invocation of trpc.polls.participants.list.useQuery by removing additional caching options.
apps/web/src/contexts/permissions.tsx Introduced PermissionProvider component to provide user permissions based on userId and modified PermissionsContext declaration.
apps/web/src/contexts/poll.tsx Updated usePoll hook to directly pass params.urlId to trpc.polls.get.useQuery, removing local state management for urlId.

Possibly related PRs

Poem

In the realm of code, a rabbit hops,
With helpers and hydration, it never stops.
Prefetching data, oh what a delight,
With every new change, the future looks bright.
So here's to the updates, both swift and clever,
In the world of React, we'll code forever! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (8)
apps/web/src/app/[locale]/poll/[urlId]/layout.tsx (1)

15-20: Add error handling for prefetching queries

Using Promise.all without error handling means that if any of the prefetch queries fail, the entire operation will fail. Consider adding error handling to each prefetch to ensure robustness.

Proposed change:

-  await Promise.all([
-    trpc.polls.get.prefetch({ urlId: params.urlId }),
-    trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
-    trpc.polls.getWatchers.prefetch({ pollId: params.urlId }),
-    trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
-  ]);
+  await Promise.allSettled([
+    trpc.polls.get.prefetch({ urlId: params.urlId }),
+    trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
+    trpc.polls.getWatchers.prefetch({ pollId: params.urlId }),
+    trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
+  ]);

This way, all prefetches attempt to complete, and you can handle any that fail individually if needed.

apps/web/src/trpc/context.ts (1)

11-11: Consider making ip a required property if always present

If ip is consistently available and required by tRPC procedures, consider making it a non-optional property in TRPCContext for type safety and clarity.

apps/web/src/trpc/server/create-ssr-helper.ts (1)

14-19: Handle unauthorized access appropriately in SSR context

Throwing a TRPCError with code UNAUTHORIZED may not be the best approach in server-side rendering. Consider redirecting to a login page or rendering an appropriate error message instead.

Proposed change:

       if (!session) {
-        throw new TRPCError({
-          code: "UNAUTHORIZED",
-          message: "Unauthorized",
-        });
+        // Handle unauthorized access in a way suitable for SSR
+        redirect('/login');
       }

Ensure that the method used aligns with your application's routing and error handling practices.

apps/web/src/app/[locale]/(admin)/page.tsx (1)

19-20: Add error handling for SSR helper creation and prefetching

createSSRHelper() and helpers.dashboard.info.prefetch() may throw errors if there are issues like network failures or unauthorized access. Consider adding try-catch blocks to handle these potential errors gracefully.

Proposed change:

-  const helpers = await createSSRHelper();
-  await helpers.dashboard.info.prefetch();
+  let helpers;
+  try {
+    helpers = await createSSRHelper();
+    await helpers.dashboard.info.prefetch();
+  } catch (error) {
+    // Handle errors appropriately, e.g., log the error, redirect, or render an error message
+    console.error("Error during SSR helper creation or prefetching:", error);
+    // Return an appropriate response or fallback UI
+    return notFound();
+  }

This addition will enhance the robustness of your server-side rendering logic.

apps/web/src/trpc/trpc.ts (2)

87-91: Consider improving error handling for missing IP

The error message "Failed to get client IP" might not be helpful for debugging. Consider including more context about why the IP might be missing and potential remediation steps.

   if (!ctx.ip) {
     throw new TRPCError({
       code: "INTERNAL_SERVER_ERROR",
-      message: "Failed to get client IP",
+      message: "Failed to get client IP. This might happen if the request is not properly forwarded through the proxy server.",
     });
   }

Line range hint 94-102: Review rate limiting configuration

The current configuration of 5 requests per minute might be too restrictive for legitimate users, especially considering server-side rendering and prefetching scenarios.

Consider:

  1. Increasing the limit or using a sliding window of 30 requests per minute
  2. Adding different limits for authenticated vs unauthenticated users
  3. Implementing graceful degradation if the KV store is unavailable
apps/web/src/app/[locale]/(admin)/layout.tsx (1)

21-43: Consider performance optimization for hydration state

The current implementation wraps the entire layout with Hydrate. Consider moving the hydration boundary closer to where the data is actually used to optimize the initial render.

Consider splitting the layout into smaller components and only wrapping the components that actually use the subscription data with Hydrate.

apps/web/src/app/[locale]/invite/[urlId]/page.tsx (1)

9-17: Consider adding type safety for prefetch operations

The prefetch operations could benefit from stronger type safety to prevent potential runtime errors.

Consider creating a type-safe helper function for managing related prefetch operations:

type PollPrefetchParams = {
  pollId: string;
  urlId: string;
};

async function prefetchPollData({ pollId, urlId }: PollPrefetchParams) {
  return Promise.allSettled([
    trpc.polls.get.prefetch({ urlId }),
    trpc.polls.participants.list.prefetch({ pollId }),
    trpc.polls.comments.list.prefetch({ pollId }),
  ]);
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 40df1ff and 92aa4bc.

📒 Files selected for processing (8)
  • apps/web/src/app/[locale]/(admin)/layout.tsx (1 hunks)
  • apps/web/src/app/[locale]/(admin)/page.tsx (2 hunks)
  • apps/web/src/app/[locale]/invite/[urlId]/page.tsx (1 hunks)
  • apps/web/src/app/[locale]/poll/[urlId]/layout.tsx (1 hunks)
  • apps/web/src/pages/api/trpc/[trpc].ts (2 hunks)
  • apps/web/src/trpc/context.ts (1 hunks)
  • apps/web/src/trpc/server/create-ssr-helper.ts (1 hunks)
  • apps/web/src/trpc/trpc.ts (1 hunks)
🔇 Additional comments (3)
apps/web/src/app/[locale]/poll/[urlId]/layout.tsx (1)

28-31: Verify that dehydrate includes all necessary data

Ensure that dehydrate(trpc.queryClient) captures all the prefetched data needed by the client. Missing data might lead to additional client-side fetches or errors.

Run the following script to check the contents of the query client:

✅ Verification successful

Let me gather more information about the data prefetching in this component.


Let me try a different approach to find the relevant files and prefetching logic.


Based on the search results, I can now provide a final response as we have sufficient information about the data prefetching in the layout component.

All necessary data is being properly prefetched

The layout component correctly prefetches all the required data for the poll page:

  • polls.get: Fetches the poll details
  • polls.participants.list: Fetches the list of participants
  • polls.getWatchers: Fetches the poll watchers
  • polls.comments.list: Fetches the poll comments

These prefetched queries match exactly with the data requirements shown in the components:

  • PollLayout component uses all these queries
  • The data is properly dehydrated using dehydrate(trpc.queryClient)
  • Components like Discussion, ParticipantsProvider, and NotificationsToggle consume this prefetched data
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all expected queries are present in the query client state.

# Expected query keys
EXPECTED_KEYS=(
  'polls.get'
  'polls.participants.list'
  'polls.getWatchers'
  'polls.comments.list'
)

# Iterate over expected keys and check if they exist in the query client
for key in "${EXPECTED_KEYS[@]}"; do
  # This is a placeholder command; actual implementation requires runtime access.
  echo "Check if '$key' exists in trpc.queryClient."
done

Length of output: 753


Script:

#!/bin/bash
# Search for prefetch calls in the layout file and its parent components
rg -A 5 "prefetch|prefetchQuery" "apps/web/src/app/\[locale\]/poll/\[urlId\]/"

# Look for any trpc query definitions related to polls
ast-grep --pattern 'router.query($$$)'

Length of output: 205


Script:

#!/bin/bash
# Find all files in the poll directory
fd . "apps/web/src/app" --type f

# Search for trpc prefetch calls in the entire codebase
rg -A 5 "prefetch|prefetchQuery" "apps/web/src"

# Look for trpc router definitions for polls
rg -A 5 "polls\." "apps/web/src"

Length of output: 27341

apps/web/src/trpc/context.ts (1)

11-11: Verify removal of req and res from TRPCContext

Removing req and res from TRPCContext may impact procedures that rely on these properties. Ensure that no existing tRPC procedures depend on ctx.req or ctx.res.

Run the following script to search for any usage of ctx.req or ctx.res:

✅ Verification successful

Let me verify if these usages are in tRPC procedures or not.


Let me try a different pattern to search for tRPC procedures.


Let me check the actual tRPC procedure implementations.


Let me try one final search pattern to find tRPC procedure implementations.


Based on the search results, I can now provide a final response:

Removal of req and res from TRPCContext is safe

The only usages of ctx.req and ctx.res are in the Next.js page component disable-notifications.tsx, not in any tRPC procedures. The tRPC procedures only use ctx.user for authentication checks, as seen in the middleware implementations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for usages of `ctx.req` or `ctx.res` in the codebase.

rg 'ctx\.req|ctx\.res'

Length of output: 276


Script:

#!/bin/bash
# Search for tRPC procedure definitions in the codebase
ast-grep --pattern 'router.$_($$$, async ({ ctx }) => { $$$ })'

Length of output: 65


Script:

#!/bin/bash
# Search for files that might contain tRPC router definitions
fd "router.ts"

# Then let's examine the content of these files
rg "procedure|router\." --type ts

Length of output: 1917


Script:

#!/bin/bash
# Find all router implementation files
fd "router\." --type ts --exec cat {}

Length of output: 211


Script:

#!/bin/bash
# Search for files that might contain tRPC routers and their content
fd -e ts -e tsx | xargs rg "createTRPCRouter|procedure\." -A 5

Length of output: 1222

apps/web/src/pages/api/trpc/[trpc].ts (1)

40-41: Verify correct retrieval of client IP address

Using requestIp.getClientIp(opts.req) may return unexpected results depending on the deployment environment (e.g., behind proxies or load balancers). Ensure that the IP address is accurately retrieved, considering the potential need to handle X-Forwarded-For headers.

Run the following script to check for proper IP retrieval:


export default async function Layout({
children,
params,
}: React.PropsWithChildren<{ params: { urlId: string } }>) {
const trpc = await createSSRHelper();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle potential errors from createSSRHelper()

createSSRHelper() may throw an error if the user session is not available or if there's an issue creating the context. Consider adding error handling to manage this scenario and improve user experience.

Proposed change:

-  const trpc = await createSSRHelper();
+  let trpc;
+  try {
+    trpc = await createSSRHelper();
+  } catch (error) {
+    // Handle the error appropriately, e.g., redirect to login or show an error page
+    notFound();
+  }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +11 to +32
async function createContext(): Promise<TRPCContext> {
const session = await getServerSession();

if (!session) {
throw new TRPCError({
code: "UNAUTHORIZED",
message: "Unauthorized",
});
}

const res = {
user: {
id: session.user.id,
isGuest: session.user.email === null,
locale: session.user.locale ?? undefined,
image: session.user.image ?? undefined,
getEmailClient: () => getEmailClient(session.user.locale ?? undefined),
},
};

return res;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Include ip in the context for consistency

The TRPCContext includes an ip property, but createContext here does not set it. For consistency and to prevent potential issues in procedures that expect ip, consider including the ip in the context.

Proposed change:

       const res = {
         user: {
           id: session.user.id,
           isGuest: session.user.email === null,
           locale: session.user.locale ?? undefined,
           image: session.user.image ?? undefined,
           getEmailClient: () =>
             getEmailClient(session.user.locale ?? undefined),
         },
+        ip: undefined, // Set the IP address here if available in this context
       };

If obtaining the IP address is feasible in this context, it would maintain consistency across your server-side context creation.

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +17 to +19
const helpers = await createSSRHelper();
await helpers.user.subscription.prefetch();
const dehydratedState = dehydrate(helpers.queryClient);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding error handling for prefetch operations

The prefetch operation could fail silently. Consider adding error handling to gracefully handle failed prefetches.

   const helpers = await createSSRHelper();
-  await helpers.user.subscription.prefetch();
+  try {
+    await helpers.user.subscription.prefetch();
+  } catch (error) {
+    console.error('Failed to prefetch subscription data:', error);
+    // Consider implementing fallback behavior
+  }
   const dehydratedState = dehydrate(helpers.queryClient);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const helpers = await createSSRHelper();
await helpers.user.subscription.prefetch();
const dehydratedState = dehydrate(helpers.queryClient);
const helpers = await createSSRHelper();
try {
await helpers.user.subscription.prefetch();
} catch (error) {
console.error('Failed to prefetch subscription data:', error);
// Consider implementing fallback behavior
}
const dehydratedState = dehydrate(helpers.queryClient);

Comment on lines 13 to 17
await Promise.all([
trpc.polls.get.prefetch({ urlId: params.urlId }),
trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling for parallel prefetches

While using Promise.all for concurrent fetching is efficient, a single failed prefetch will cause all prefetches to fail. Consider using Promise.allSettled for more resilient prefetching.

-  await Promise.all([
+  const results = await Promise.allSettled([
     trpc.polls.get.prefetch({ urlId: params.urlId }),
     trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
     trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
   ]);
+  
+  // Log any failed prefetches
+  results.forEach((result, index) => {
+    if (result.status === 'rejected') {
+      console.error(`Failed to prefetch query ${index}:`, result.reason);
+    }
+  });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await Promise.all([
trpc.polls.get.prefetch({ urlId: params.urlId }),
trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
]);
const results = await Promise.allSettled([
trpc.polls.get.prefetch({ urlId: params.urlId }),
trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
]);
// Log any failed prefetches
results.forEach((result, index) => {
if (result.status === 'rejected') {
console.error(`Failed to prefetch query ${index}:`, result.reason);
}
});

@vercel vercel bot temporarily deployed to Preview – landing December 2, 2024 00:41 Inactive
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (4)
apps/web/src/app/[locale]/invite/[urlId]/layout.tsx (1)

11-12: Validate params.urlId before using it

Ensure that params.urlId is validated to prevent potential issues with malformed or unexpected input. This helps enhance security and robustness by preventing invalid data from causing errors downstream.

You might add validation logic to confirm that urlId meets expected patterns or criteria.

apps/web/src/components/participants-provider.tsx (1)

24-26: Re-evaluate caching strategy after removing staleTime and cacheTime

By removing the staleTime and cacheTime options from the useQuery, the default caching behavior is now in effect. This may lead to more frequent data refetching, which could impact performance. Consider verifying that the default caching strategy aligns with the desired application behavior.

If the intention is to always fetch fresh data, ensuring users see the most up-to-date participants list, then this change is appropriate. Otherwise, you may want to reintroduce caching options:

 const { data: participants } = trpc.polls.participants.list.useQuery({
   pollId,
+}, {
+  staleTime: [desired_stale_time],
+  cacheTime: [desired_cache_time],
 });

Replace [desired_stale_time] and [desired_cache_time] with appropriate values based on your application's needs.

apps/web/src/contexts/permissions.tsx (2)

9-14: Consider adding type documentation

The permissions context would benefit from JSDoc documentation explaining its purpose and usage.

+/**
+ * Context for managing user permissions.
+ * Provides the current user ID for permission checks.
+ */
const PermissionsContext = React.createContext<{
  userId: string | null;
}>({
  userId: null,
});

15-24: Optimize context value with useMemo

The context value should be memoized to prevent unnecessary re-renders of child components.

export const PermissionProvider = ({
  children,
  userId,
}: React.PropsWithChildren<{ userId: string | null }>) => {
+  const value = React.useMemo(() => ({ userId }), [userId]);
  return (
-    <PermissionsContext.Provider value={{ userId }}>
+    <PermissionsContext.Provider value={value}>
      {children}
    </PermissionsContext.Provider>
  );
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 92aa4bc and 1ef6f8a.

📒 Files selected for processing (6)
  • apps/web/src/app/[locale]/invite/[urlId]/layout.tsx (1 hunks)
  • apps/web/src/app/[locale]/invite/[urlId]/page.tsx (1 hunks)
  • apps/web/src/app/[locale]/invite/[urlId]/providers.tsx (1 hunks)
  • apps/web/src/components/participants-provider.tsx (1 hunks)
  • apps/web/src/contexts/permissions.tsx (1 hunks)
  • apps/web/src/contexts/poll.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • apps/web/src/app/[locale]/invite/[urlId]/providers.tsx
🔇 Additional comments (2)
apps/web/src/contexts/poll.tsx (1)

Line range hint 9-11: Enhance error message for missing prefetch

The error message indicates that prefetching is expected, but there's no clear documentation on where/how prefetching should occur.

Let's verify the prefetch implementation:

✅ Verification successful

Error message is appropriate, prefetching is well-implemented

The error message is actually appropriate as the poll data is consistently prefetched in both layout components (invite/[urlId]/layout.tsx and poll/[urlId]/layout.tsx) using trpc.polls.get.prefetch(). The error serves as a runtime safeguard in case the prefetching mechanism fails, which is a good practice.

  • Poll data is prefetched in layouts:
    • /invite/[urlId]/layout.tsx
    • /poll/[urlId]/layout.tsx
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for prefetch calls to polls.get
rg "polls\.get\.prefetch" -A 3

Length of output: 888


Script:

#!/bin/bash
# Let's check where the poll context is being used to understand the data flow
rg -l "usePoll" 

# Also check for any existing documentation about prefetching
fd "README|docs" --extension md --exec cat {}

Length of output: 9191


Script:

#!/bin/bash
# Let's check the layout files where prefetching occurs to understand the context
cat apps/web/src/app/[locale]/invite/[urlId]/layout.tsx apps/web/src/app/[locale]/poll/[urlId]/layout.tsx

# Also check the poll context implementation
cat apps/web/src/contexts/poll.tsx

Length of output: 2459

apps/web/src/app/[locale]/invite/[urlId]/page.tsx (1)

14-19: Previous comment about Promise.all still applies

The previous suggestion to use Promise.allSettled for more resilient prefetching remains relevant here, especially when fetching permissions.

Comment on lines +16 to +20
await Promise.all([
trpc.polls.get.prefetch({ urlId: params.urlId }),
trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for prefetch failures

The current implementation does not handle cases where the prefetch operations might fail or return null (e.g., if the poll does not exist or the user lacks permissions). This could lead to unexpected behaviors or runtime errors during rendering. Consider adding error handling to manage potential errors from trpc.polls.get.prefetch and other prefetch calls.

You can check the results after prefetching and handle errors appropriately. For example:

 await Promise.all([
   trpc.polls.get.prefetch({ urlId: params.urlId }),
   trpc.polls.participants.list.prefetch({ pollId: params.urlId }),
   trpc.polls.comments.list.prefetch({ pollId: params.urlId }),
 ]);

+const pollData = trpc.queryClient.getQueryData(['polls.get', { urlId: params.urlId }]);
+if (!pollData) {
+  notFound();
+}

Committable suggestion skipped: line range outside the PR's diff.

staleTime: Infinity,
},
);
const pollQuery = trpc.polls.get.useQuery({ urlId: params?.urlId as string });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using type-safe parameter handling

The type assertion as string could be unsafe. Consider adding a runtime check or using optional chaining with a fallback.

-  const pollQuery = trpc.polls.get.useQuery({ urlId: params?.urlId as string });
+  if (!params?.urlId) {
+    throw new Error("URL ID is required");
+  }
+  const pollQuery = trpc.polls.get.useQuery({ urlId: params.urlId });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const pollQuery = trpc.polls.get.useQuery({ urlId: params?.urlId as string });
if (!params?.urlId) {
throw new Error("URL ID is required");
}
const pollQuery = trpc.polls.get.useQuery({ urlId: params.urlId });

Comment on lines +27 to +37
export default async function Page({
searchParams,
}: {
params: { urlId: string };
searchParams: { token: string };
}) {
return (
<PermissionContext token={searchParams.token}>
<InvitePage />
</PermissionContext>
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding loading state handling

The Page component should handle loading states while permissions are being fetched.

export default async function Page({
  searchParams,
}: {
  params: { urlId: string };
  searchParams: { token: string };
}) {
+  // Consider adding Suspense boundary
+  return (
+    <Suspense fallback={<LoadingSpinner />}>
      <PermissionContext token={searchParams.token}>
        <InvitePage />
      </PermissionContext>
+    </Suspense>
  );
}

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +10 to +25
const PermissionContext = async ({
children,
token,
}: React.PropsWithChildren<{ token?: string }>) => {
const helpers = await createSSRHelper();
let impersonatedUserId: string | null = null;
if (token) {
const res = await helpers.auth.getUserPermission.fetch({ token });
impersonatedUserId = res?.userId ?? null;
}
return (
<PermissionProvider userId={impersonatedUserId}>
{children}
</PermissionProvider>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance error handling in PermissionContext

The token validation and user permission fetching lack proper error handling.

const PermissionContext = async ({
  children,
  token,
}: React.PropsWithChildren<{ token?: string }>) => {
  const helpers = await createSSRHelper();
  let impersonatedUserId: string | null = null;
  if (token) {
+    try {
      const res = await helpers.auth.getUserPermission.fetch({ token });
      impersonatedUserId = res?.userId ?? null;
+    } catch (error) {
+      console.error('Failed to fetch user permissions:', error);
+      // Consider redirecting to error page or showing error message
+    }
  }
  return (
    <PermissionProvider userId={impersonatedUserId}>
      {children}
    </PermissionProvider>
  );
};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const PermissionContext = async ({
children,
token,
}: React.PropsWithChildren<{ token?: string }>) => {
const helpers = await createSSRHelper();
let impersonatedUserId: string | null = null;
if (token) {
const res = await helpers.auth.getUserPermission.fetch({ token });
impersonatedUserId = res?.userId ?? null;
}
return (
<PermissionProvider userId={impersonatedUserId}>
{children}
</PermissionProvider>
);
};
const PermissionContext = async ({
children,
token,
}: React.PropsWithChildren<{ token?: string }>) => {
const helpers = await createSSRHelper();
let impersonatedUserId: string | null = null;
if (token) {
try {
const res = await helpers.auth.getUserPermission.fetch({ token });
impersonatedUserId = res?.userId ?? null;
} catch (error) {
console.error('Failed to fetch user permissions:', error);
// Consider redirecting to error page or showing error message
}
}
return (
<PermissionProvider userId={impersonatedUserId}>
{children}
</PermissionProvider>
);
};

@lukevella lukevella merged commit 82ebcd8 into main Dec 2, 2024
9 checks passed
@lukevella lukevella deleted the prefetch-with-trpc branch December 2, 2024 00:46
@coderabbitai coderabbitai bot mentioned this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant