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

Avoid throwing not found error when not login in nextjs pages #494

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

@andresgutgon andresgutgon force-pushed the fix/do-not-report-not-found-web branch from 1657f8d to c719377 Compare October 22, 2024 18:23
if (!apiKey) {
const result = await getLatitudeApiKey()
const result = await getLatitudeApiKey(workspace)
Copy link
Contributor Author

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

@@ -20,12 +18,8 @@ export default async function DashboardLayout({
}: Readonly<{
children: ReactNode
}>) {
const data = await getSession()
if (!data.session) return redirect(ROUTES.auth.login)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

@@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without session there is no user. But doesn't hurt this check

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)

@andresgutgon andresgutgon force-pushed the fix/do-not-report-not-found-web branch 2 times, most recently from a7d17c9 to 2ad5785 Compare October 23, 2024 09:41
csansoon
csansoon previously approved these changes Oct 23, 2024
@@ -44,6 +44,7 @@ export default function WorkspaceApiKeys() {
</TableCell>
<TableCell>
<Tooltip
asChild
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't that the default behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
https://www.radix-ui.com/primitives/docs/guides/composition#composing-multiple-primitives

geclos
geclos previously approved these changes Oct 23, 2024
Throwing error end in Sentry with a NotFound. What we want to do is
redirect user to /login
@andresgutgon andresgutgon dismissed stale reviews from geclos and csansoon via 37156d7 October 23, 2024 11:10
@andresgutgon andresgutgon force-pushed the fix/do-not-report-not-found-web branch from 2ad5785 to 37156d7 Compare October 23, 2024 11:10
@andresgutgon andresgutgon merged commit c90550a into main Oct 23, 2024
3 checks passed
@andresgutgon andresgutgon deleted the fix/do-not-report-not-found-web branch October 23, 2024 11:15
@github-actions github-actions bot locked and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants