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

PPR Experiment #59

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

PPR Experiment #59

wants to merge 2 commits into from

Conversation

not-ani
Copy link
Contributor

@not-ani not-ani commented Oct 26, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced experimental support for Partial Page Rendering (PPR).
    • Added components for displaying user projects and exploring projects.
  • Improvements

    • Updated to Next.js canary release 15.0.2 for enhanced features and performance.
    • Improved access control for admin routes based on user roles.
  • Bug Fixes

    • Removed redundant authentication checks in admin page.
  • Chores

    • Enhanced type safety in configuration files.
    • Deleted obsolete loading skeleton component.

These updates enhance user experience, improve performance, and streamline access control within the application.

Copy link

coderabbitai bot commented Oct 26, 2024

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes in this pull request primarily involve updates to configuration files, component structures, and type annotations across various parts of the Next.js application. Key modifications include the introduction of type safety in configuration files, updates to the next dependency version, and the addition of new components for user project management. Several components have been refactored to support asynchronous data fetching, while others have had their authentication checks removed or modified. Overall, the changes enhance the application's structure and functionality without altering existing logic significantly.

Changes

File Path Change Summary
apps/nextjs/next.config.ts Added type annotation for config as NextConfig and introduced experimental.ppr set to "incremental".
apps/nextjs/package.json Updated next dependency version from "^14.2.14" to "15.0.2-canary.5".
apps/nextjs/src/app/(auth)/sign-in/page.tsx Added constant experimental_ppr set to true.
apps/nextjs/src/app/(dashboard)/project/[id]/(root)/calendar/page.tsx Removed ProjectPageProps interface and updated Page function to not accept parameters.
apps/nextjs/src/app/(dashboard)/project/[id]/(root)/page.tsx Updated params in ProjectPageProps to be a Promise and changed HomePage to an async function.
apps/nextjs/src/app/(dashboard)/project/[id]/(root)/permissions/page.tsx Changed Page function to an async function with params as a Promise.
apps/nextjs/src/app/(dashboard)/project/[id]/(tasks)/tasks/page.tsx Updated params to be a Promise in Page function.
apps/nextjs/src/app/(home)/_components/UserProjects.tsx Added new UserProjects component to fetch and display user projects.
apps/nextjs/src/app/(home)/_components/explore-projects.tsx Added new ExploreProjects function to fetch and display projects.
apps/nextjs/src/app/(home)/_components/project-card.tsx Introduced new ProjectCard component to display individual project details.
apps/nextjs/src/app/(home)/admin/page.tsx Removed authentication and authorization checks from Page component.
apps/nextjs/src/app/(home)/loading.tsx Removed LoadingSkeleton component.
apps/nextjs/src/app/(home)/page.tsx Refactored Page component to use Suspense for loading states and added experimental_ppr.
apps/nextjs/src/lib/auth.ts Modified checkAuth to use cache from react.
apps/nextjs/src/middleware.ts Added a check to redirect non-admin users from admin routes.
apps/website/next.config.ts Added type annotation for config as NextConfig and introduced experimental.ppr.
apps/website/package.json Updated next dependency version from "^14.2.14" to "15.0.2-canary.5".

Poem

🐇 In the land of code where rabbits play,
New features hop in, brightening the day.
With types and configs, we leap and bound,
Exploring projects where joy is found.
So gather your carrots, let’s code with glee,
For every new change brings magic, you see! ✨


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.

@not-ani not-ani changed the title base Performance optimizations Oct 26, 2024
Copy link

@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: 19

🧹 Outside diff range and nitpick comments (9)
apps/nextjs/src/app/(home)/_components/explore-projects.tsx (2)

1-4: Remove unnecessary React import and add type definitions.

The explicit React import isn't needed in Next.js 13+ unless you're using React-specific features like useState. Also, consider adding type definitions for better type safety.

Apply this diff:

-import React from "react";
+import type { Project } from "../_types";
 
 import { findAllProjects } from "../_queries";
 import { ProjectCard } from "./project-card";

13-15: Add type safety to the project data.

The project data is passed to ProjectCard without type checking. Add proper type annotations to ensure type safety.

Apply this diff:

-        {allProjects.map((project) => (
+        {allProjects.map((project: Project) => (
           <ProjectCard key={project.id} project={project} />
         ))}
apps/nextjs/src/app/(dashboard)/project/[id]/(root)/page.tsx (2)

Line range hint 15-24: Optimize prefetching with Promise.all

The three prefetch operations can be executed in parallel for better performance.

Consider this optimization:

-  void api.tasks.getTasksOverTime.prefetch({
-    projectId: id,
-  });
-
-  void api.tasks.getTaskPriorities.prefetch({
-    projectId: id,
-  });
-
-  void api.tasks.getTaskStatuses.prefetch({
-    projectId: id,
-  });
+  void Promise.all([
+    api.tasks.getTasksOverTime.prefetch({ projectId: id }),
+    api.tasks.getTaskPriorities.prefetch({ projectId: id }),
+    api.tasks.getTaskStatuses.prefetch({ projectId: id })
+  ]);

Line range hint 28-30: Remove extra semicolon in JSX

There's an unnecessary semicolon after the ProjectDashboard component.

Apply this diff:

-        <ProjectDashboard id={id} />;
+        <ProjectDashboard id={id} />
apps/nextjs/src/app/(home)/_components/UserProjects.tsx (2)

3-3: Consider relocating getUserProjects to a more appropriate location.

The getUserProjects function is currently imported from the navbar/switcher component, which seems like an unusual location for this functionality. Consider moving it to a dedicated projects or user-related utility file for better code organization.


30-32: Handle empty project list state.

The current implementation doesn't handle the case when usersProjects is empty. Consider adding a message for users who don't have any projects yet.

Consider this improvement:

-          usersProjects.map((project) => (
-            <ProjectCard key={project.id} project={project} />
-          ))
+          usersProjects.length > 0 ? (
+            usersProjects.map((project) => (
+              <ProjectCard key={project.id} project={project} />
+            ))
+          ) : (
+            <p className="col-span-full text-center text-muted-foreground">
+              You haven't created any projects yet.
+            </p>
+          )
apps/nextjs/src/middleware.ts (1)

34-34: Consider using a dedicated route for insufficient privileges.

Redirecting to "/unverified" for non-admin users might be confusing as it suggests an unverified status rather than insufficient privileges.

Consider creating a dedicated "/unauthorized" or "/forbidden" route for better user experience and clearer error communication.

apps/nextjs/src/app/(dashboard)/project/[id]/(root)/calendar/page.tsx (2)

Line range hint 19-89: Consider extracting event data fetching logic.

The calendar events are currently hardcoded. Given this is a project calendar view, these events should likely be fetched based on the project context.

Consider refactoring to fetch events from your data source:

async function fetchProjectEvents(projectId: string) {
  // Implement your data fetching logic here
  return [/* events */];
}

export default async function Page() {
  // Get projectId from params or context
  const events = await fetchProjectEvents(projectId);
  
  return (
    <main className="max-h-screen px-10">
      <Calendar events={events}>
        {/* ... rest of the component ... */}
      </Calendar>
    </main>
  );
}

Line range hint 21-34: Add type safety for events and colors.

Consider adding stricter type definitions for the event objects and color options to prevent potential runtime errors.

Add a type definition for calendar events:

type CalendarEventColor = 'pink' | 'blue' | /* other valid colors */;

interface CalendarEvent {
  id: string;
  start: Date;
  end: Date;
  title: string;
  color: CalendarEventColor;
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4c797a0 and 9ffb27a.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (17)
  • apps/nextjs/next.config.ts (1 hunks)
  • apps/nextjs/package.json (1 hunks)
  • apps/nextjs/src/app/(auth)/sign-in/page.tsx (1 hunks)
  • apps/nextjs/src/app/(dashboard)/project/[id]/(root)/calendar/page.tsx (1 hunks)
  • apps/nextjs/src/app/(dashboard)/project/[id]/(root)/page.tsx (1 hunks)
  • apps/nextjs/src/app/(dashboard)/project/[id]/(root)/permissions/page.tsx (1 hunks)
  • apps/nextjs/src/app/(dashboard)/project/[id]/(tasks)/tasks/page.tsx (1 hunks)
  • apps/nextjs/src/app/(home)/_components/UserProjects.tsx (1 hunks)
  • apps/nextjs/src/app/(home)/_components/explore-projects.tsx (1 hunks)
  • apps/nextjs/src/app/(home)/_components/project-card.tsx (1 hunks)
  • apps/nextjs/src/app/(home)/admin/page.tsx (0 hunks)
  • apps/nextjs/src/app/(home)/loading.tsx (0 hunks)
  • apps/nextjs/src/app/(home)/page.tsx (1 hunks)
  • apps/nextjs/src/lib/auth.ts (1 hunks)
  • apps/nextjs/src/middleware.ts (1 hunks)
  • apps/website/next.config.ts (1 hunks)
  • apps/website/package.json (1 hunks)
💤 Files with no reviewable changes (2)
  • apps/nextjs/src/app/(home)/admin/page.tsx
  • apps/nextjs/src/app/(home)/loading.tsx
✅ Files skipped from review due to trivial changes (1)
  • apps/nextjs/src/app/(auth)/sign-in/page.tsx
🔇 Additional comments (16)
apps/nextjs/src/lib/auth.ts (2)

1-4: LGTM! Imports are correctly structured.

The imports are well-organized and all dependencies are necessary for the authentication functionality.


6-14: Verify cache behavior with auth state changes.

While using cache can improve performance, be cautious about potential stale authentication states. The cache is request-scoped in Server Components, but verify that this doesn't affect real-time session updates.

Run this script to verify cache usage across the codebase:

✅ Verification successful

The search results show that cache is used consistently across the codebase, particularly for server-side data fetching and authentication. The auth implementation in packages/auth/src/index.rsc.ts shows that the base auth() function itself is already cached using cache(defaultAuth). Since React's cache function is request-scoped, this implementation is safe as:

  1. The cache is reset for each new request
  2. The authentication state cannot become stale within a single request
  3. Multiple calls to checkAuth() within the same request will reuse the cached value, which is efficient

The caching implementation for authentication is correctly implemented

The current implementation follows React's server component caching best practices. The cache wrapper on checkAuth is appropriate since:

  • It's built on top of an already cached auth() function
  • The cache is request-scoped, preventing stale authentication states
  • It provides performance benefits by avoiding redundant auth checks within the same request
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for other cached authentication-related functions
# and potential cache invalidation points
rg -A 3 "cache\(" 

# Look for session-related state management
ast-grep --pattern 'session = await auth()'

Length of output: 2569

apps/nextjs/src/app/(dashboard)/project/[id]/(tasks)/tasks/page.tsx (2)

Line range hint 11-28: LGTM!

The rest of the implementation looks good:

  • Proper error handling with empty array checks
  • Clean component structure
  • Appropriate use of dynamic rendering directive

4-9: ⚠️ Potential issue

Remove Promise type from params as it conflicts with Next.js conventions.

The params object in Next.js pages is synchronously available and shouldn't be typed as a Promise. This change introduces unnecessary async overhead and deviates from Next.js routing conventions.

Apply this diff to fix the type annotation:

 export default async function Page({
   params,
 }: {
-  params: Promise<{ id: string }>;
+  params: { id: string };
 }) {
-  const id = (await params).id;
+  const id = params.id;

The async function can remain as it's needed for the API call, but the params handling should be synchronous.

Let's verify this pattern in other Next.js pages:

apps/website/next.config.ts (1)

1-3: LGTM! Good addition of type safety.

Adding type annotations to the Next.js config improves maintainability and catches potential configuration errors at compile time.

apps/nextjs/next.config.ts (2)

2-2: LGTM! Good addition of type safety.

Adding the NextConfig type import enhances type checking and IDE support.


8-8: LGTM! Type annotation improves configuration safety.

The NextConfig type annotation ensures the configuration object strictly adheres to Next.js's expected structure.

apps/nextjs/src/app/(home)/_components/project-card.tsx (2)

1-5: LGTM! Imports are well-organized.

The imports follow a good organization pattern: React core, Next.js components, and then custom UI components.


7-46: Verify integration with parent components.

Let's verify how this component is integrated within UserProjects and ExploreProjects components to ensure consistent usage.

✅ Verification successful

Integration with parent components is consistent and properly implemented

The verification shows that ProjectCard is correctly used across all parent components:

  • In ExploreProjects: Basic usage with project prop
  • In UserProjects:
    • Used with isPending and message props for pending/unverified states
    • Used with project prop for normal state
  • Props usage matches the component's interface in all cases
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ProjectCard usage in parent components
rg -A 5 "ProjectCard" apps/nextjs/src/app/\(home\)

Length of output: 4063

apps/nextjs/src/middleware.ts (1)

29-35: Verify the impact on existing admin routes.

The new admin access control might affect existing admin routes. Let's verify all admin routes in the codebase.

✅ Verification successful

Let me try a different approach to search for admin-related code and access controls.


Admin route access control is properly implemented across the codebase

The new admin access control in middleware.ts aligns with the existing role-based authorization patterns. The codebase shows:

  • Consistent "Admin" role checks in the API layer (packages/api/src/permissions.ts)
  • Proper role definition in the database schema (packages/db/src/schema.ts)
  • Admin-specific routes and components with appropriate role checks
  • Project-level admin permissions are also handled separately from global admin role
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all admin routes and their access controls

# Search for admin routes in pages/routes
echo "Searching for admin routes in the codebase..."
rg -l "admin" --type ts --type tsx

# Search for role-based access controls
echo "Searching for existing role checks..."
ast-grep --pattern 'user.role === "Admin"'

Length of output: 293


Script:

#!/bin/bash
# Search for admin-related routes and access controls with ripgrep
echo "Searching for admin-related paths and components..."
rg -i "admin" -t ts -t js -A 3

echo -e "\nSearching for role-based checks..."
rg -i "role.*admin|admin.*role" -t ts -t js -A 3

Length of output: 15833

apps/website/package.json (2)

24-24: ⚠️ Potential issue

Caution: Using Next.js canary release in production

The update to [email protected] raises several concerns:

  1. Canary releases are pre-release versions that may contain bugs or breaking changes
  2. The major version bump from 14 to 15 likely includes breaking changes
  3. The version is pinned exactly without a range, which could make future updates more difficult

Let's verify the compatibility with your current setup:

#!/bin/bash
# Check for potential breaking changes in Next.js 15
rg -l "getInitialProps|getServerSideProps|getStaticProps|getStaticPaths" 

# Check for App Router migration requirements
rg -l "pages/" 

# Look for Next.js specific configurations
fd "next.config" 

Consider these alternatives:

  1. Use the latest stable version: "next": "^14.2.14"
  2. If you need v15 features, wait for the stable release
  3. If you must use canary, add a version range: "next": "^15.0.2-canary.0"

25-26: Verify React version compatibility

The React version (18.3.1) is also a pre-release version. Using multiple pre-release versions together increases the risk of compatibility issues.

Let's check for potential React compatibility issues:

✅ Verification successful

Let me gather more specific information about React usage in the codebase.


Let me try a different approach to check React usage and dependencies.


Based on the extensive analysis of the codebase, I can now provide a final response:

React 18.3.1 usage appears safe in this codebase

The codebase shows consistent and standard usage of React features:

  • Proper usage of hooks (useState, useEffect, useRef, useCallback)
  • No experimental or unstable React features detected
  • Clean integration with Next.js and other React-based dependencies
  • Consistent React patterns across components

The pre-release version (18.3.1) is being used uniformly across the project, and the codebase follows React best practices without relying on version-specific features that might cause compatibility issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for React specific hooks and features that might be affected
rg "use(Effect|State|Context|Reducer|Callback|Ref|ImperativeHandle|LayoutEffect|Debug)" 

# Check for React.lazy usage
rg "React\.lazy|Suspense" 

Length of output: 161


Script:

#!/bin/bash
# Check React imports and usage patterns
rg "from 'react'" -A 2

# Look for specific React hooks usage
rg "use[A-Z][a-zA-Z]+" --type ts --type tsx --type jsx

# Check package.json files for React related dependencies
fd package.json --exec cat {} | jq -r 'select(.dependencies != null) | .dependencies | keys[] | select(contains("react"))'

Length of output: 299


Script:

#!/bin/bash
# Check React imports with simpler grep
rg "from ['|\"]react['|\"]"

# List all package.json files
fd package.json

# Check content of website's package.json specifically
cat apps/website/package.json

# Look for React hooks with simpler pattern
rg "useState|useEffect|useContext|useReducer|useCallback|useRef|useImperativeHandle|useLayoutEffect|useDebugValue"

Length of output: 33318

apps/nextjs/src/app/(dashboard)/project/[id]/(root)/permissions/page.tsx (1)

23-30: 🛠️ Refactor suggestion

Verify the async component implementation.

The change to an async component with Promise-based params aligns with Next.js App Router patterns. However, we should ensure proper error handling for the params resolution.

Consider adding error handling:

export default async function Page({
  params,
}: {
  params: Promise<{
    id: string;
  }>;
}) {
-  const id = (await params).id;
+  try {
+    const id = (await params).id;
+  } catch (error) {
+    console.error('Failed to resolve project ID:', error);
+    throw new Error('Failed to load project permissions');
+  }
apps/nextjs/src/app/(dashboard)/project/[id]/(root)/calendar/page.tsx (2)

13-13: LGTM: Import cleanup aligns with current usage.

The simplified date-fns import correctly reflects that only addHours is being used in the current implementation.


16-16: Verify data fetching after props removal.

The removal of ProjectPageProps and route parameters suggests a change in how project data is accessed. Since this is in a dynamic route [id], we should ensure the project context is still properly available.

Let's verify the project data access pattern in related files:

✅ Verification successful

Let me gather more information about how project data is accessed in these layout files and the calendar page.


Let me check the calendar page implementation to see how it accesses the project data that's fetched in the parent layout.


Project data access is properly handled through parent layout

The calendar page doesn't need direct access to project data as it's already handled by the parent layout ([id]/(root)/layout.tsx). The layout component fetches project data using getProjectInfo and provides the necessary context. The calendar page itself only renders a calendar component with static events, which is correct for its current implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check how other files in the project route handle data fetching
# Look for patterns of data fetching in sibling pages and layout files

# Check for data fetching patterns in the project route
rg -l --type typescript "async function.*Page" "apps/nextjs/src/app/(dashboard)/project/[id]"

# Look for potential layout files that might provide project context
fd --type file "layout.tsx" "apps/nextjs/src/app/(dashboard)/project/[id]"

Length of output: 460


Script:

#!/bin/bash
# Check the content of layout files and calendar page for project data access patterns
rg -A 5 "useProject|getProject|project" "apps/nextjs/src/app/(dashboard)/project/[id]/layout.tsx" "apps/nextjs/src/app/(dashboard)/project/[id]/(root)/layout.tsx" "apps/nextjs/src/app/(dashboard)/project/[id]/(root)/calendar/page.tsx"

# Look for any project-related hooks or utilities
rg -l "useProject|getProject" "apps/nextjs/src/app/(dashboard)/project"

# Check for any project context provider
ast-grep --pattern 'ProjectProvider|ProjectContext'

Length of output: 2815


Script:

#!/bin/bash
# Get the full implementation of the calendar page
cat "apps/nextjs/src/app/(dashboard)/project/[id]/(root)/calendar/page.tsx"

# Check if there are any hooks or components used in the calendar page
rg -A 3 "import.*from" "apps/nextjs/src/app/(dashboard)/project/[id]/(root)/calendar/page.tsx"

Length of output: 2761

apps/nextjs/src/app/(home)/page.tsx (1)

55-55: Confirm that removing async from the Page function does not affect data fetching

The Page component has been changed from an async function to a regular function. Ensure that all necessary data fetching is properly handled within child components using Suspense and that removing async from Page does not introduce any unintended side effects or affect server-side rendering behavior.

Comment on lines 6 to 14
export const checkAuth = cache(async () => {
const session = await auth();

if (!session) {
redirect("/sign-in");
}

return session;
};
});
Copy link

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 auth() call.

The authentication check could fail for reasons other than missing sessions (e.g., network issues, service unavailability).

Consider adding error handling:

 export const checkAuth = cache(async () => {
-  const session = await auth();
+  try {
+    const session = await auth();
+
+    if (!session) {
+      redirect("/sign-in");
+    }
+
+    return session;
+  } catch (error) {
+    console.error("Authentication check failed:", error);
+    redirect("/sign-in?error=auth_failed");
+  }
-  if (!session) {
-    redirect("/sign-in");
-  }
-
-  return session;
 });
📝 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
export const checkAuth = cache(async () => {
const session = await auth();
if (!session) {
redirect("/sign-in");
}
return session;
};
});
export const checkAuth = cache(async () => {
try {
const session = await auth();
if (!session) {
redirect("/sign-in");
}
return session;
} catch (error) {
console.error("Authentication check failed:", error);
redirect("/sign-in?error=auth_failed");
}
});

Comment on lines +9 to +18
return (
<div className="flex flex-col gap-6">
<h3 className="text-4xl font-semibold">Explore Projects</h3>
<div className="grid gap-4 sm:grid-cols-2 lg:grid-cols-3">
{allProjects.map((project) => (
<ProjectCard key={project.id} project={project} />
))}
</div>
</div>
);
Copy link

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 and error states to the component wrapper.

Wrap the component with Suspense and ErrorBoundary for better user experience during loading and error states.

Here's how you could improve the component wrapper:

+export default function ExploreProjectsWrapper() {
+  return (
+    <ErrorBoundary fallback={<div>Error loading projects</div>}>
+      <Suspense fallback={<LoadingSpinner />}>
+        <ExploreProjects />
+      </Suspense>
+    </ErrorBoundary>
+  );
+}
+
 export async function ExploreProjects() {

Committable suggestion was skipped due to low confidence.

Comment on lines +6 to +8
export async function ExploreProjects() {
const allProjects = await findAllProjects();

Copy link

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 and loading states.

The direct database query without error handling could lead to uncaught runtime errors. Consider implementing proper error boundaries and loading states.

Apply this diff:

+import { ErrorBoundary } from "../_components/error-boundary";
+import { LoadingSpinner } from "../_components/loading-spinner";
+
 export async function ExploreProjects() {
-  const allProjects = await findAllProjects();
+  try {
+    const allProjects = await findAllProjects();
+    if (!allProjects?.length) {
+      return <p className="text-center text-gray-500">No projects found</p>;
+    }

Committable suggestion was skipped due to low confidence.

Comment on lines 6 to 8
experimental: {
ppr: true,
},
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

PPR configuration needs to be removed or justified

The verification reveals:

  1. The website app is using Next.js 15.0.2-canary.5 (a pre-release version)
  2. No PPR-specific patterns ("use server") are found in the website app
  3. No documentation or intentional PPR setup is evident in the website codebase

This experimental feature should be removed from the configuration unless there's a specific requirement because:

  • It's enabled on a canary version of Next.js
  • The codebase shows no actual usage of PPR features
  • There's no documentation justifying its inclusion
🔗 Analysis chain

Verify the intentional enabling of Partial Prerendering (PPR).

PPR is an experimental Next.js 14 feature that fundamentally changes how pages are rendered. While it can improve performance, please ensure:

  1. The team is aware of its experimental status
  2. The application has been tested with PPR enabled
  3. There's a rollback plan if issues arise in production

Let's verify the Next.js version and any PPR-related setup:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check Next.js version and PPR usage
echo "Checking Next.js version and PPR setup..."

# Check package.json for Next.js version
jq -r '.dependencies.next // .devDependencies.next' package.json

# Look for PPR-specific patterns in app directory
echo "Checking for PPR usage in components..."
rg -l "use server" 

Length of output: 434


Script:

#!/bin/bash
# Let's check the actual package.json in the website app and any PPR-related setup
echo "Checking website app's package.json..."
cat apps/website/package.json

echo -e "\nChecking for PPR-specific patterns in website app..."
rg -l "use server" apps/website/

echo -e "\nChecking for any documentation or comments about PPR..."
rg -i "ppr|partial pre-rendering" 

Length of output: 9083

Comment on lines 31 to 33

void api.users.findUsersForProject.prefetch({
projectId: id,
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the timing of prefetch call.

The void operator is used to explicitly ignore the Promise returned by prefetch. Since this is an async component, we could potentially await the prefetch to ensure data is available before render.

Consider awaiting the prefetch:

-  void api.users.findUsersForProject.prefetch({
+  await api.users.findUsersForProject.prefetch({
    projectId: id,
  });
📝 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
void api.users.findUsersForProject.prefetch({
projectId: id,
await api.users.findUsersForProject.prefetch({
projectId: id,

@@ -35,7 +35,7 @@
"date-fns-tz": "^3.2.0",
"geist": "^1.3.0",
"motion-number": "^0.1.7",
"next": "^14.2.14",
"next": "15.0.2-canary.5",
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Based on the package.json files analysis, I can now provide a final response:

⚠️ Next.js 15 canary version poses compatibility risks

The upgrade to Next.js 15.0.2-canary.5 raises significant concerns:

  1. This version is used in multiple packages within the monorepo:

    • apps/nextjs/package.json
    • apps/website/package.json
  2. The current setup shows tight integration with:

    • React 18.3.1 (pre-release)
    • Multiple tRPC packages (v11.0.0-rc.477)
    • Various UI components and dependencies
  3. The @amaxa/auth package is still using Next.js 14.2.14, which could lead to version conflicts within the monorepo.

Recommendation: Maintain version consistency by either:

  • Keeping all packages on the stable Next.js 14.x series
  • Or if testing Next.js 15 features is necessary, upgrade all dependent packages together in a separate development branch
🔗 Analysis chain

⚠️ Caution: Using Next.js canary release in package.json

The update to "next": "15.0.2-canary.5" raises several concerns:

  1. This is a pre-release (canary) version of Next.js 15, which is not recommended for production use as it may contain unstable features and bugs.

  2. The major version bump from 14 to 15 likely introduces breaking changes that will require migration work.

  3. Using React 18.3.1 (also pre-release) with this specific Next.js canary version may lead to compatibility issues.

Let's verify the official migration guide and current stable version:

Recommendation: Consider using the latest stable version of Next.js 14.x instead of the canary release to ensure stability and production readiness. If you need to test Next.js 15 features, consider doing so in a separate development branch.


Let me try a different approach to gather information about Next.js versions and potential breaking changes.

This will help us:

  1. Understand the full dependency tree and Next.js usage across the project
  2. Check Next.js configurations that might be affected by version 15
  3. Find any internal documentation about upgrades or migrations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Get the latest stable version of Next.js from npm
echo "Latest stable Next.js version:"
npm show next version

# Check if there's an official blog post or migration guide for Next.js 15
echo -e "\nSearching for Next.js 15 migration guide:"
gh search repos --owner vercel --match name "next.js" --json description | jq '.[] | select(.description | contains("15"))'

Length of output: 388


Script:

#!/bin/bash
# Check package.json files for Next.js related dependencies and configurations
echo "Checking package.json files:"
fd package.json --exec cat {}

# Look for Next.js configuration files
echo -e "\nChecking Next.js config files:"
fd next.config --extension js,mjs,ts,mts --exec cat {}

# Search for any migration or upgrade related documentation in the repository
echo -e "\nSearching for migration/upgrade documentation:"
rg -i "upgrade|migration|breaking.?changes" -g "*.md"

Length of output: 15756

Comment on lines +6 to +7
import { ExploreProjects } from "./_components/explore-projects";
import { UserProjects } from "./_components/UserProjects";
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure consistent naming conventions for component files and imports

The import statements for ExploreProjects and UserProjects use different casing in their file paths. ExploreProjects is imported from "./_components/explore-projects", while UserProjects is imported from "./_components/UserProjects". For consistency and to avoid issues on case-sensitive file systems, consider using a consistent naming convention for your component files and their import paths.

Comment on lines +11 to +32
const OneLoading = () => {
return (
<div className="min-h-screen p-4 sm:p-6 lg:p-8">
<div className="">
<div className="flex w-full flex-col gap-5">
<div className="flex flex-col gap-6">
<div className="flex flex-row justify-between gap-6">
<h3 className="text-4xl font-semibold">Your Project</h3>
{session.user.role === "Admin" ? <CreateProject /> : null}
</div>
<div className="grid gap-4 sm:grid-cols-2 lg:grid-cols-3 ">
{session.user.status == "Pending" ? (
<Card className="col-span-1 row-span-1 flex h-[300px] w-[300px] flex-col items-center justify-center border-dashed bg-secondary/10 transition-transform duration-200">
<CardHeader>
Your account is pending approval, please wait for the admin
to approve your account
</CardHeader>
</Card>
) : session.user.status == "Unverified" ? (
<Card className="col-span-1 row-span-1 flex h-[300px] w-[300px] flex-col items-center justify-center border-dashed bg-secondary/10 transition-transform duration-200">
<CardHeader>
Your account is unverified, please wait till we verify your
account to access the platform
</CardHeader>
</Card>
) : (
<div className="grid gap-4 sm:grid-cols-2 lg:grid-cols-3 ">
{usersProjects.map((project) => {
return (
<Link
key={project.id}
href={`/project/${project.id}`}
className="col-span-1 row-span-1 bg-secondary/10 transition-transform duration-200"
>
<Card className="col-span-1 row-span-1 bg-secondary/10 transition-transform duration-200">
<CardContent className="py-5">
<Image
src={project.image ?? ""}
width={1000}
height={500}
alt={String(project.id)}
/>
</CardContent>
<CardFooter className="justify-center text-center font-bold md:text-2xl">
{project.name}
</CardFooter>
</Card>
</Link>
);
})}
</div>
)}
</div>
</div>
<div className="flex flex-col gap-6">
<h3 className="text-4xl font-semibold">Explore Projects</h3>
<div className="grid gap-4 sm:grid-cols-2 lg:grid-cols-3 ">
{allProjects.map((project) => {
return (
<Link key={project.id} href={`/project/${project.id}`}>
<Card className="col-span-1 row-span-1 bg-secondary/10 transition-transform duration-200">
<CardContent className="py-5">
<Image
src={project.image ?? ""}
width={1000}
height={500}
alt={String(project.id)}
/>
</CardContent>
<CardFooter className="justify-center text-center font-bold md:text-2xl">
{project.name}
</CardFooter>
</Card>
</Link>
);
})}
</div>
</div>
<div className="flex flex-col gap-6">
<div className="flex flex-row justify-between gap-6">
<Skeleton className="h-10 w-48" />
<Skeleton className="h-10 w-32" />
</div>
<div className="grid gap-4 sm:grid-cols-2 lg:grid-cols-3">
{[...Array(6)].map((_, index) => (
<Card key={index} className="col-span-1 row-span-1 bg-secondary/10">
<CardContent className="py-5">
<Skeleton className="h-40 w-full" />
</CardContent>
<CardFooter className="justify-center">
<Skeleton className="h-6 w-3/4" />
</CardFooter>
</Card>
))}
</div>
</div>
);
};
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor OneLoading and TwoLoading components to reduce code duplication

The OneLoading and TwoLoading components have very similar structures and render similar content with minor differences. To adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting the shared code into a single reusable loading component that accepts props to handle any variations.

Also applies to: 34-54

getUserProjects(session.user.id),
findAllProjects(),
]);
export const experimental_ppr = true;
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

PPR configuration is properly set but needs version alignment

The codebase has experimental_ppr = true in the page while using Next.js 15.0.2-canary.5. There are two different next.config.js files with different PPR configurations:

  • One has experimental.ppr: true
  • Another has experimental.ppr: "incremental"

Since Next.js 14.0.0, PPR should be configured as "incremental" for better performance. Update all PPR configurations to use "incremental" consistently:

  • In the page file: Change export const experimental_ppr = true to export const experimental_ppr = "incremental"
  • In next.config.js files: Ensure all instances use ppr: "incremental"
🔗 Analysis chain

Verify the use of experimental_ppr and ensure it's properly configured

You've added export const experimental_ppr = true;. Ensure that this experimental feature is supported in your current Next.js version and that it's properly configured in your next.config.js (e.g., under the experimental flag). Additionally, be aware of any potential side effects or breaking changes that might occur when using experimental features.

Run the following script to confirm that experimental.ppr is set to "incremental" in your Next.js configuration:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if `experimental.ppr` is set in `next.config.js`

# Test: Search for `experimental.ppr` setting in `next.config.js` files.
# Expect: Find at least one occurrence indicating proper configuration.

rg 'experimental\s*:\s*{[^}]*\bppr\s*:\s*["\']incremental["\']' next.config.js

Length of output: 93


Script:

#!/bin/bash
# Let's first check if next.config.js exists and its content
fd -e js -e mjs -e cjs -e ts "next.config" -x cat {}

# Also check for experimental PPR in package.json to verify Next.js version
cat package.json | grep -A 5 "next"

Length of output: 2664


Script:

#!/bin/bash
# Let's check the Next.js version in the workspace
cat apps/nextjs/package.json | grep -A 2 '"next":'

Length of output: 136

@not-ani not-ani marked this pull request as draft October 26, 2024 21:51
@not-ani not-ani changed the title Performance optimizations PPR Experiment Oct 27, 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