-
Notifications
You must be signed in to change notification settings - Fork 6
Feat/add double opt in #6
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
base: main
Are you sure you want to change the base?
Conversation
Deploying nuxt-hub-landing with
|
Latest commit: |
d35e096
|
Status: | ✅ Deploy successful! |
Preview URL: | https://baeab745.nuxt-hub-landing.pages.dev |
Branch Preview URL: | https://feat-add-double-opt-in.nuxt-hub-landing.pages.dev |
PR Summary
|
const config = useRuntimeConfig() | ||
|
||
|
||
if (config.landing.verifyEmail) { |
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.
config.landing is possibly undefined. Would consider to use optional chaining
subject: `Confirm your email on ${appName}`, | ||
html: await render(VerifyTemplate, { | ||
email: entry.email, | ||
appName, |
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.
appName could be undefined, but expected type is string
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.
I have finished my first iteration of review. Due to time constraints I have to split the effort. I will continue providing as soon as possible. So far I could join the waitlist, but I did not receive a email from resend (probably configuration issue).
One thing I noticed when starting the app is that the database migration is run every time and I get error about already existing tables. Here is the stackstrace:
` ERROR Database migrations failed D1_ERROR: table waitlist already exists at offset 13: SQLITE_ERROR
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async SQLiteD1Session.batch (node_modules/src/d1/session.ts:82:24)
at async migrate (node_modules/src/d1/migrator.ts:47:3)
at async (server/plugins/migrations.ts:9:1)
at async Promise.all (index 0)
at async (node_modules/@nuxthub/core/dist/runtime/database/server/plugins/migrations.dev.js:15:5)
at async Promise.all (index 0)
at async (node_modules/@nuxthub/core/dist/runtime/ready.dev.js:5:3)`
|
||
|
||
function compareToken(email, token) { | ||
return generateSecureToken(email, token) === token |
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.
return generateSecureToken(email, token) === token | |
return generateSecureToken(email) === token |
|
||
export { | ||
compareToken, | ||
generateSecureToken, |
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.
Note: No need to export this function since its unused outside of this script.
generateSecureToken, |
|
||
export default defineEventHandler(async event => { | ||
consola.info("User trying to signup for waitlist...") | ||
const {emails} = useResend(); |
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.
Question: Is there no import required?
|
||
consola.info("User joining waitlist...") | ||
|
||
let entry = await useDrizzle().insert(tables.waitlist).values({ |
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.
Thought: tables
is not explicitly imported. This reduces readability.
name: 'nuxtHubLanding', | ||
configKey: "landing", |
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.
Thought: I think if this is supposed to be published as module a more descriptive name is required. How about evoize-waitlist
?
@@ -0,0 +1,76 @@ | |||
import {addServerHandler, createResolver, defineNuxtModule, logger, extendPages} from 'nuxt/kit' |
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.
Suggestion: Add a prettifier to format this import syntax, disallow semicolons and disallow implicit imports.
appName?: string | ||
verifyEmail?: boolean | ||
email?: string, | ||
resendApiKey?: string, |
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.
Thought: All of those properties should be required.
No description provided.