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.
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.
appName could be undefined, but expected type is string
Eddy401
left a comment
There was a problem hiding this comment.
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.
| return generateSecureToken(email, token) === token | |
| return generateSecureToken(email) === token |
|
|
||
| export { | ||
| compareToken, | ||
| generateSecureToken, |
There was a problem hiding this comment.
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.
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.
Thought: tables is not explicitly imported. This reduces readability.
| name: 'nuxtHubLanding', | ||
| configKey: "landing", |
There was a problem hiding this comment.
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.
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.
Thought: All of those properties should be required.
No description provided.