middleware based auth for pages, robots.txt, sitemap.xml #164
Conversation
![kaizen-bot[bot]](https://avatars.githubusercontent.com/in/480502?s=60&v=4){kind=small}
| return [ | ||
| { | ||
| url: 'https://supermemory.ai/', | ||
| lastModified: new Date(), | ||
| changeFrequency: 'yearly', | ||
| priority: 1, | ||
| } | ||
| ] |
There was a problem hiding this comment.
Comment: The sitemap.xml generation logic is missing URLs for authenticated pages.
Solution: Add URLs for authenticated pages to the sitemap.xml generation logic.
Reason For Comment: The current implementation only includes the root URL, which is insufficient for SEO purposes. Authenticated pages should be included in the sitemap to improve discoverability by search engines.
| return [ | |
| { | |
| url: 'https://supermemory.ai/', | |
| lastModified: new Date(), | |
| changeFrequency: 'yearly', | |
| priority: 1, | |
| } | |
| ] | |
| import{type MetadataRoute}from 'next' | |
| export default function sitemap(): MetadataRoute.Sitemap{ | |
| return[ | |
| {url: 'https://supermemory.ai/', lastModified: new Date(), changeFrequency: 'yearly', priority: 1}, | |
| {url: 'https://supermemory.ai/app/home', lastModified: new Date(), changeFrequency: 'daily', priority: 0.8}, | |
| {url: 'https://supermemory.ai/memories', lastModified: new Date(), changeFrequency: 'daily', priority: 0.8}, | |
| {url: 'https://supermemory.ai/space', lastModified: new Date(), changeFrequency: 'daily', priority: 0.8}, | |
| {url: 'https://supermemory.ai/chat', lastModified: new Date(), changeFrequency: 'daily', priority: 0.8}, | |
| {url: 'https://supermemory.ai/note', lastModified: new Date(), changeFrequency: 'daily', priority: 0.8}, | |
| {url: 'https://supermemory.ai/canvas', lastModified: new Date(), changeFrequency: 'daily', priority: 0.8}, | |
| ] | |
| } | |
| ``` |
|
Hi! let's get this merged, can you resolve the conflicts please? |
Code Review❗ Attention Required: This PR has potential issues. 🚨 Authentication LogicRedirect logic in Signin function should handle more cases.Potential Solution: Add error handling to manage failed authentication attempts and provide user feedback.
Error HandlingThe redirect logic in the Page function does not handle errors properly.Potential Solution: Change the condition to explicitly check for both success and data.
Middleware Authentication LogicThe middleware function should handle both authenticated and unauthenticated routes correctly.Potential Solution: Ensure that the authentication check is comprehensive and covers all necessary routes.
Useful Commands
|
| if (user) { | ||
| redirect("/home"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Comment: Redirect logic in Signin function should handle more cases.
Solution: Add error handling to manage failed authentication attempts and provide user feedback.
Reason For Comment: The current implementation only checks if a user exists but does not handle cases where the authentication fails or if the user is not authorized.
| async function Signin(){const user = await auth(); if (!user){return <ErrorComponent message='Authentication failed' />;}redirect('/home');} |
|
|
||
| async function Page({ params: { spaceid } }: { params: { spaceid: number } }) { | ||
| const user = await auth(); | ||
|
|
||
| const { success, data } = await getMemoriesInsideSpace(spaceid); | ||
| if (!success ?? !data) return redirect("/home"); |
There was a problem hiding this comment.
Comment: The redirect logic in the Page function does not handle errors properly.
Solution: Change the condition to explicitly check for both success and data.
Reason For Comment: Using a nullish coalescing operator (??) in the condition might lead to unexpected behavior. It should explicitly check for both success and data.
| if (!success ?? !data) return redirect("/home"); | |
| if (!success || !data) return redirect('/home'); |
| const info = await auth(); | ||
| if (routeTypes.authed.some((route) => request.nextUrl.pathname.startsWith(route))) { | ||
| if (!info) { | ||
| return NextResponse.redirect(new URL("/signin", request.nextUrl)); | ||
| } | ||
| } else if (routeTypes.unAuthedOnly.some((route) => request.nextUrl.pathname.endsWith(route))) { | ||
| if (info) { | ||
| return NextResponse.redirect(new URL("/home", request.nextUrl)); | ||
| } | ||
| } | ||
| return NextResponse.next(); |
There was a problem hiding this comment.
Comment: The middleware function should handle both authenticated and unauthenticated routes correctly.
Solution: Ensure that the authentication check is comprehensive and covers all necessary routes.
Reason For Comment: If the authentication logic fails or is misconfigured, users may gain unauthorized access or be incorrectly redirected.
| const info = await auth(); | |
| if (routeTypes.authed.some((route) => request.nextUrl.pathname.startsWith(route))) { | |
| if (!info) { | |
| return NextResponse.redirect(new URL("/signin", request.nextUrl)); | |
| } | |
| } else if (routeTypes.unAuthedOnly.some((route) => request.nextUrl.pathname.endsWith(route))) { | |
| if (info) { | |
| return NextResponse.redirect(new URL("/home", request.nextUrl)); | |
| } | |
| } | |
| return NextResponse.next(); | |
| if (routeTypes.authed.some((route) => request.nextUrl.pathname.startsWith(route))){/* existing logic */}else if (routeTypes.unAuthedOnly.some((route) => request.nextUrl.pathname.endsWith(route))){/* existing logic */} |
Middleware-Based Authentication for Pages, Robots.txt, and Sitemap.xml
Overview
This pull request introduces middleware-based authentication for various pages within the application, including the onboarding flow, sign-in, and dashboard pages. It also includes updates to the robots.txt and sitemap.xml files, ensuring proper handling of user authentication states.
Changes
New Features: None
Refactoring:
/app/homeroute instead of the previous/homeroute.SignIncomponent to remove unusedsearchParamsparameter and simplify the logic.Navbarcomponent in the landing page to use the/app/homeroute for the "Home" link.Pagefunctions in the memory-related pages to handle cases where user data is not available, redirecting to the/app/homeroute.Other Changes:
Original Description
None