-
Notifications
You must be signed in to change notification settings - Fork 62
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
Avoid throwing not found error when not login in nextjs pages #494
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,8 @@ import { | |
} from '@latitude-data/web-ui' | ||
import { AppTabs } from '$/app/(private)/AppTabs' | ||
import { getCurrentUser } from '$/services/auth/getCurrentUser' | ||
import { getSession } from '$/services/auth/getSession' | ||
import { ROUTES } from '$/services/routes' | ||
import Link from 'next/link' | ||
import { redirect } from 'next/navigation' | ||
|
||
import { getActiveProjectsCached } from '../_data-access' | ||
import { ProjectsTable } from './_components/ProjectsTable' | ||
|
@@ -20,12 +18,8 @@ export default async function DashboardLayout({ | |
}: Readonly<{ | ||
children: ReactNode | ||
}>) { | ||
const data = await getSession() | ||
if (!data.session) return redirect(ROUTES.auth.login) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is already happening in the main layout. Is confusing having it here |
||
|
||
const { workspace } = await getCurrentUser() | ||
const projects = await getActiveProjectsCached({ workspaceId: workspace.id }) | ||
|
||
return ( | ||
<Container> | ||
<AppTabs /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,8 @@ export default async function PrivateLayout({ | |
if (!data.session) return redirect(ROUTES.auth.login) | ||
|
||
const { workspace, user } = await getCurrentUser() | ||
if (!user) return redirect(ROUTES.auth.login) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without session there is no user. But doesn't hurt this check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is why I mentioned to do the check in a middleware as redirects in pages always throw. Nextjs docs actually recommend doing any pre-render redirects in middleware (for the future) |
||
|
||
const supportIdentity = createSupportUserIdentity(user) | ||
|
||
return ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ export default function WorkspaceApiKeys() { | |
</TableCell> | ||
<TableCell> | ||
<Tooltip | ||
asChild | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't that the default behaviour? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think. I had to add it. Not sure if default behaviour makes sense. I don't see recommended on radix docs |
||
trigger={ | ||
<Button | ||
variant='ghost' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to fetch again workspace when is already fetched by the users of this function. Passing it by argument