Launch HN: Code-review #255
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -0,0 +1,47 @@ | |||
| import crypto from 'crypto'; | |||
There was a problem hiding this comment.
wouldn't we lose all images on restart? Why didn't we go with local file storage here?
| return parsed.data; | ||
| } | ||
|
|
||
| export async function createSharedWorkflowFromJson(json: string): Promise<{ id: string; ttlSeconds: number; }> |
There was a problem hiding this comment.
- there is no user + project auth happening here
- Ideally this createSharedId should be part of project.actions.ts where project auth check is also done
| return { id, ttlSeconds: DEFAULT_TTL_SECONDS }; | ||
| } | ||
|
|
||
| export async function loadSharedWorkflow(idOrUrl: string): Promise<z.infer<typeof Workflow>> { |
There was a problem hiding this comment.
- no auth checks in place
- this function expects an
idorurl. What is the url for? When is it used?
| @@ -0,0 +1,30 @@ | |||
| import { NextRequest, NextResponse } from 'next/server'; | |||
There was a problem hiding this comment.
Why is like toggling built as an API route? Shouldn't it have been a server-action? The problem here is that:
- we are not doing any user auth, instead some x-guest-id - which i'm not sure how thats getting populated
- this makes it a public endpoint which anyone can spam and fill up the likes collection!
| @@ -0,0 +1,57 @@ | |||
| import { NextRequest, NextResponse } from 'next/server'; | |||
There was a problem hiding this comment.
all of these should have been server actions
| @@ -0,0 +1,16 @@ | |||
| import { NextRequest, NextResponse } from 'next/server'; | |||
There was a problem hiding this comment.
why not use a server action here?
| @@ -0,0 +1,130 @@ | |||
| import { NextRequest, NextResponse } from 'next/server'; | |||
There was a problem hiding this comment.
why not use server actions here?
| // Try known extensions in order used by generator | ||
| const exts = ['.png', '.jpg', '.webp']; | ||
| let foundExt: string | null = null; | ||
| for (const ext of exts) { |
There was a problem hiding this comment.
not a blocker for launch, but this is suboptimal and costly. We should either
- know what extension we're dealing with beforehand (through a corresponding database entry), or
- remain blind and have s3 metadata return the type during the serve
in any case - we should avoid HEADing each possible type here
| authorName: "Rowboat", | ||
| authorEmail: undefined, | ||
| isAnonymous: false, | ||
| workflow: tpl as any, |
There was a problem hiding this comment.
A bad side-effect of using json files for templates is that we have now lost zod validation altogether. This is bad. instead, we could have used .ts files to keep validation.
in any case, avoid blindly ingesting any random json w/o ensuring that it is conforming to the workflow spec. We should first ensure that the json passes WorkflowSchema.parse() test. Otherwise this can lead to hard-to-debug issues downstream (when people create projects based on these broken templates)
DO NOT MERGE!
This PR is meant for code-review only