ffeat: add azure blob for store the file content - #16
Open
Mayank-saraswal wants to merge 3 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR integrates Azure Blob Storage for file content (blobPath), updates Convex file APIs to use ID-based fetches and blob references, adds blob upload/download endpoints and hooks, introduces design skill docs and landing UI, tightens security headers, and adjusts GitHub import/export and preview flows to the blob-backed model. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as Editor Component
participant API as /api/files/content
participant Blob as Azure Blob Storage
participant Convex as Convex Backend
User->>Editor: Edit file
Editor->>Editor: Debounce changes
Editor->>API: PUT (blobPath, content)
API->>Blob: uploadBlob(content)
Blob-->>API: blobPath
API-->>Editor: 200 OK (blobPath)
Editor->>Convex: updateFile(projectId, fileId, blobPath)
Convex-->>Editor: Success
sequenceDiagram
participant Preview as Preview Component
participant FileTree as File Tree Builder
participant API as /api/files/content
participant Blob as Azure Blob Storage
participant WebContainer as WebContainer
Preview->>FileTree: resolveFileContents(files)
FileTree->>API: GET (blobPath) for each file
API->>Blob: downloadBlob(blobPath)
Blob-->>API: content
API-->>FileTree: resolvedContent
FileTree-->>Preview: files with resolvedContent
Preview->>WebContainer: mount(filesWithContent)
WebContainer-->>Preview: ready
sequenceDiagram
participant UI as GitHub Dialog
participant API as /api/github/import
participant Clerk as Clerk
participant Convex as Convex Backend
participant Inngest as Inngest
UI->>API: POST (url)
API->>Clerk: fetch GitHub token for userId
Clerk-->>API: githubToken
API->>Convex: createProject(internalKey, ownerId, name)
Convex-->>API: projectId
API->>Inngest: emit import.repo (userId, projectId)
Inngest-->>API: eventId
API-->>UI: { success, projectId, eventId }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47ecd64906
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+23
to
+27
| if (!blobPath.startsWith("projects/")) { | ||
| return NextResponse.json( | ||
| { error: "Invalid blob path" }, | ||
| { status: 400 } | ||
| ); |
There was a problem hiding this comment.
This endpoint validates only that the path starts with projects/ and then serves blob data, but it never checks that the authenticated user owns the referenced project/file. The same pattern exists for PUT, so any signed-in user who learns a valid blob path can read or overwrite another tenant's file contents. Add a Convex ownership check (project owner or file owner) before calling blob storage operations.
Useful? React with 👍 / 👎.
Comment on lines
+285
to
288
| await ctx.db.patch(args.id, { | ||
| name: args.newName, | ||
| updatedAt: now, | ||
| }); |
There was a problem hiding this comment.
Renaming updates only name, leaving blobPath unchanged, while file creation now derives blob keys from folder/name (see file explorer create flows). If a user renames a.ts to b.ts and later creates a new a.ts in the same folder, both records can point to the same blob key and edits to one file overwrite the other. Rename should either move the blob or generate stable, name-independent blob keys.
Useful? React with 👍 / 👎.
Comment on lines
343
to
+346
| await ctx.storage.delete(item.storageId); | ||
| } | ||
|
|
||
| await ctx.db.delete("files", fileId); | ||
| await ctx.db.delete(fileId); |
There was a problem hiding this comment.
The recursive delete path removes binary storageId objects but immediately deletes the DB row without deleting blobPath content. After this migration, text files live in Azure Blob, so deleting files/folders now leaks orphaned blobs and retained file data even though Convex says the files are gone. Add blob deletion for items with blobPath before ctx.db.delete.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/app/api/messages/cancel/route.ts (1)
14-55:⚠️ Potential issue | 🟠 MajorMissing top-level
try/catch— unhandled exceptions escape the handlerThree failure paths are unguarded:
request.json()throws on a malformed body.requestSchema.parse(body)throws aZodErroron invalid input.- Any rejected Convex call inside
Promise.allpropagates uncaught.All of these result in Next.js returning a generic 500 without the JSON error shape callers expect.
quick-edit/route.tsin this same PR wraps its entire body intry/catch— apply the same pattern here.🛡️ Proposed fix
export async function POST(request: Request) { + try { const { userId } = await auth(); if (!userId) { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); } const body = await request.json(); const { projectId } = requestSchema.parse(body); const internalKey = process.env.MORIS_CONVEX_INTERNAL_KEY; if (!internalKey) { return NextResponse.json({ error: "internal key is missing" }, { status: 401 }); } // ... return NextResponse.json({ success: true, messageId: cancelledIds, cancelled: true }); + } catch (error) { + console.error("Failed to cancel messages:", error); + return NextResponse.json({ error: "Failed to cancel messages" }, { status: 500 }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/messages/cancel/route.ts` around lines 14 - 55, Wrap the entire POST handler body in a top-level try/catch so malformed JSON, Zod parse errors, and any rejected Convex/inngest calls are caught and returned as a JSON error response; specifically, enclose the logic that calls request.json(), requestSchema.parse(body), convex.query(api.system.getProcessingMessages,...), the Promise.all mapping which calls inngest.send and convex.mutation(api.system.updateMessageStatus,...), and the final NextResponse.json return inside try, and in catch return a consistent NextResponse.json({ error: err.message || "Internal Server Error" }, { status: err instanceof ZodError ? 400 : 500 }) (or equivalent) so callers always receive the expected JSON error shape.src/app/api/quick-edit/route.ts (1)
97-105:⚠️ Potential issue | 🔴 Critical
{ text }should be{ output }— response delivers raw JSON string instead of the extracted code objectWhen
generateTextis used withOutput.object(), the structured result is available on theoutputproperty, nottext. Per the Vercel AI SDK documentation, the correct syntax is:const { output } = await generateText({ model: ..., prompt: ..., output: Output.object({ schema: ... }), }); // output is now the validated object matching your schemaIn the current code, destructuring
{ text }retrieves the raw JSON string (e.g.,'{"editedCode":"…"}'), so the return statement sends{ editedCode: '{"editedCode":"…"}' }— the edited code gets double-wrapped and becomes unusable to callers.Fix
- const { text } = await generateText({ + const { output } = await generateText({ model: openrouter(model || "anthropic/claude-3.5-haiku"), prompt, output: Output.object({ schema: quickEditResponseSchema, }) }) - return NextResponse.json({ editedCode: text }, { status: 200 }); + return NextResponse.json({ editedCode: output.editedCode }, { status: 200 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/quick-edit/route.ts` around lines 97 - 105, The handler currently destructures { text } from generateText which returns structured results on the output property when using Output.object; change the destructuring to { output } = await generateText(...) and use that output (validated against quickEditResponseSchema) when calling NextResponse.json so you return the extracted object (e.g., output.editedCode) rather than the raw JSON string; update any references to text to use output and ensure generateText is still called with Output.object({ schema: quickEditResponseSchema }).src/app/api/suggestion/route.ts (1)
67-79:⚠️ Potential issue | 🟠 MajorReturn 400 for schema validation failures instead of 500.
Invalid client payloads from
suggestionRequestSchema.parse()currently fall into the generic catch block and return 500, misclassifying request validation errors as server errors.Catch
z.ZodErrorspecifically in the catch block and return 400 with validation details:Suggested fix
catch (error) { + if (error instanceof z.ZodError) { + return NextResponse.json( + { error: "Invalid request payload", details: error.flatten() }, + { status: 400 } + ); + } console.error("Error generating code suggestion:", error); return NextResponse.json({ error: "Failed to generate code suggestion" }, { status: 500 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/suggestion/route.ts` around lines 67 - 79, Update the request parsing error handling in the suggestion route so validation failures from suggestionRequestSchema.parse() are returned as HTTP 400 instead of a 500: specifically catch z.ZodError (or import ZodError) in the existing try/catch around suggestionRequestSchema.parse(body) and return a 400 response with the validation error details (message or issues) when a ZodError is caught; keep other exceptions falling through to the existing 500 handling. Reference suggestionRequestSchema.parse and the route handler's catch block when making this change.convex/files.ts (1)
360-381:⚠️ Potential issue | 🔴 CriticalDelete orphaned blobs when
blobPathchanges or files are deleted.The code deletes
storageIdblobs during file deletion but ignoresblobPathblobs entirely. BothupdateFilemutations (lines 357–389 inconvex/files.tsand lines 254–272 inconvex/system.ts) replaceblobPathwithout deleting the previous blob. Similarly,deleteFilemutations do not invokedeleteBlob()when removing files. ThedeleteBlob()function exists insrc/lib/azure-blob.tsbut is never called, causing orphaned storage and unnecessary cost accumulation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/files.ts` around lines 360 - 381, When updating or deleting files you must remove the old blob to avoid orphaned storage: in the updateFile handler in convex/files.ts (the handler that calls ctx.db.get(args.id) and patches blobPath) and the analogous updateFile in convex/system.ts, capture the existing file.blobPath before you call ctx.db.patch and if args.blobPath differs, call deleteBlob(existingBlobPath) (use the deleteBlob function in src/lib/azure-blob.ts), catching/logging errors but not failing the update; likewise, in the deleteFile mutation (the function that loads the file and currently deletes storageId) call deleteBlob(file.blobPath) before or after removing the DB row so both storageId and blobPath blobs are deleted. Ensure you reference deleteBlob(...) and the file.blobPath value when making these changes.src/features/conversations/inngest/tools/update-file.ts (1)
68-68:⚠️ Potential issue | 🟡 MinorGrammatical error in error message: "Error update file" → "Error updating file".
✏️ Proposed fix
- return `Error update file: ${error instanceof Error ? error.message : "Unknown error"}`; + return `Error updating file: ${error instanceof Error ? error.message : "Unknown error"}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/conversations/inngest/tools/update-file.ts` at line 68, The error message string in the return expression inside the update-file code should be corrected for grammar: change "Error update file" to "Error updating file" in the return that currently builds `Error update file: ${error instanceof Error ? error.message : "Unknown error"}` (look for the return in the update-file handler / function that constructs that error string).src/app/api/github/export/route.ts (1)
16-61:⚠️ Potential issue | 🟠 MajorMissing top-level
try/catch— unhandled errors return as uncaught exceptions.Unlike the import route, the export route has no outer
try/catch. Errors fromrequest.json(),requestSchema.parse(body)(ZodError),clerkClient(), orinngest.send()will propagate as unhandled exceptions and result in a 500 with an unformatted error rather than a structured JSON response.🐛 Proposed fix
export async function POST(request: Request) { + try { const { userId } = await auth(); if (!userId) { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); } - const body = await request.json(); - const { projectId, repoName, visibility, description } = requestSchema.parse(body); + const body = await request.json(); + const parsedBody = requestSchema.safeParse(body); + if (!parsedBody.success) { + return NextResponse.json( + { error: parsedBody.error.issues[0]?.message ?? "Invalid request body" }, + { status: 400 } + ); + } + const { projectId, repoName, visibility, description } = parsedBody.data; // ... rest of handler ... return NextResponse.json({ success: true, projectId, eventId: event.ids[0] }); + } catch (error) { + console.error("[github/export] Error:", error); + const message = error instanceof Error ? error.message : "Unknown error"; + return NextResponse.json({ error: `Failed to export repository: ${message}` }, { status: 500 }); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/github/export/route.ts` around lines 16 - 61, The POST handler currently lacks a top-level try/catch so exceptions from request.json(), requestSchema.parse(body), clerkClient(), or inngest.send() will bubble up; wrap the body of export POST (export async function POST) in a try/catch, catch ZodError from requestSchema.parse to return a 400 JSON error, and catch other errors to return a 500 JSON error (include a concise error message), ensuring any awaited calls like request.json(), clerkClient(), and inngest.send() are inside the try block and that the catch returns NextResponse.json({...}, { status: ... }).src/features/preview/utils/file-tree.tsx (1)
44-51:⚠️ Potential issue | 🟠 MajorFiles without
resolvedContentare silently dropped from theFileSystemTree.When
!file.storageId && file.resolvedContent === undefined, the file is not added to the WebContainer tree at all. If Azure Blob content pre-fetch fails for any text file, that file will be missing from the preview with no indication to the user. Consider either including an empty-string fallback or surfacing the missing files to the caller so the preview layer can display an appropriate warning.💡 Suggested fallback
- } else if (!file.storageId && file.resolvedContent !== undefined) { - current[part] = { file: { contents: file.resolvedContent } }; + } else if (!file.storageId) { + // Fall back to empty string if blob content failed to resolve + current[part] = { file: { contents: file.resolvedContent ?? "" } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/preview/utils/file-tree.tsx` around lines 44 - 51, The current logic in file-tree.tsx silently skips files when isLast && !file.storageId && file.resolvedContent === undefined, causing missing entries in the FileSystemTree/WebContainer; update the branch that handles files (the code manipulating current[part]) to create a file node even when resolvedContent is undefined by providing a fallback (e.g., set contents to an empty string) or mark the node with a missing-content sentinel so the caller/preview layer can surface a warning; ensure you reference and update the same conditional that checks file.storageId and file.resolvedContent and propagate the sentinel up through the tree-building code so the preview can detect and display missing files.src/features/conversations/inngest/tools/read-files.ts (1)
39-58:⚠️ Potential issue | 🟡 MinorSilent skip for files without
blobPathor with anullblob download produces a misleading "No files found" error.When a file record exists but has no
blobPath(e.g., a folder or a legacy file), ordownloadBlobreturnsnull, the file is silently dropped. The caller (AI agent) has no way to distinguish "ID doesn't exist" from "file exists but has no readable content". Consider returning per-file errors so the agent can reason about the failure:💡 Suggested improvement
for (const fileId of fileIds) { const file = await convex.query(api.system.getFileById, { internalKey, fileId: fileId as Id<"files">, }); - if (file && file.blobPath) { - const content = await downloadBlob(file.blobPath); - if (content !== null) { - results.push({ - id: file._id, - name: file.name, - content, - }); - } - }; + if (!file) { + results.push({ id: fileId, name: fileId, content: `[Error: file "${fileId}" not found]` }); + } else if (!file.blobPath) { + results.push({ id: file._id, name: file.name, content: `[Error: file "${file.name}" has no content]` }); + } else { + const content = await downloadBlob(file.blobPath); + if (content === null) { + results.push({ id: file._id, name: file.name, content: `[Error: blob for "${file.name}" not found]` }); + } else { + results.push({ id: file._id, name: file.name, content }); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/conversations/inngest/tools/read-files.ts` around lines 39 - 58, The current loop in read-files.ts silently skips file records that lack a blobPath or where downloadBlob(file.blobPath) returns null and then returns a generic "No files found" when results is empty; instead, modify the logic around the for (const fileId of fileIds) loop (the convex.query(api.system.getFileById, {...}) call and subsequent downloadBlob call) to collect per-file outcomes into a single response: for each fileId push either a success object ({id: file._id, name: file.name, content}) or an error object that includes the fileId and a short reason (e.g., "not found", "no blobPath", "download failed"), and return that array (or an object with successes and errors) rather than returning a blanket "Error: No files found..." message so callers can distinguish missing IDs from unreadable/empty blobs.
🟡 Minor comments (6)
src/components/landing/navbar.tsx-18-25 (1)
18-25:⚠️ Potential issue | 🟡 Minor"Dashboard" and "Get Started" both point to
/projects.Having two buttons with identical destinations on the same navbar is likely unintentional — "Get Started" should probably link to a sign-up, onboarding, or authentication route distinct from the dashboard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/landing/navbar.tsx` around lines 18 - 25, The two navbar buttons currently both link to /projects; update the second Button (the one rendering "Get Started" — Button with className "font-mono bg-white text-black hover:bg-neutral-200" and its child Link) to point to the correct onboarding/auth route (e.g., /signup or /auth) instead of /projects so the CTA leads to a sign-up/onboarding flow rather than the dashboard.src/components/landing/hero.tsx-21-21 (1)
21-21:⚠️ Potential issue | 🟡 Minor
frameBorderis a deprecated HTML attribute.React will emit a warning in development. Use
styleor theclassNameapproach instead.🛡️ Proposed fix
- frameBorder="0" + style={{ border: "none" }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/landing/hero.tsx` at line 21, Remove the deprecated JSX attribute "frameBorder" from the iframe in the Hero component and replace it with a style or className that sets the border to 0 (e.g., use style={{ border: 0 }} or a CSS class like "iframe--no-border"); locate the iframe JSX where frameBorder="0" is present and update it to use style/className so React no longer emits the deprecation warning.src/components/landing/hero.tsx-57-61 (1)
57-61:⚠️ Potential issue | 🟡 MinorRemove the dead link or add the matching anchor element.
The
#how-it-workslink has no matching anchor in the page. The Hero component (src/components/landing/hero.tsx, line 58) links to this fragment, but no element withid="how-it-works"exists anywhere in the codebase. Either add the anchor target or update thehrefto point to an existing section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/landing/hero.tsx` around lines 57 - 61, The Link in the Hero component (Button/Link in src/components/landing/hero.tsx) points to a non-existent fragment "#how-it-works"; either remove or change the href to an existing anchor, or add a matching element with id="how-it-works" in the landing page's target section (e.g., the docs/features section component) so the fragment resolves; update the Hero Link (the Button asChild/Link pair) to point to the correct fragment or ensure the target section has id="how-it-works"..agents/skills/web-design-guidelines/SKILL.md-25-27 (1)
25-27:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced block.
The URL fence is missing a language identifier (
MD040).📝 Suggested fix
-``` +```text https://raw.githubusercontent.com/vercel-labs/web-interface-guidelines/main/command.md</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/web-design-guidelines/SKILL.md around lines 25 - 27, The
fenced code block containing the URL string
"https://raw.githubusercontent.com/vercel-labs/web-interface-guidelines/main/command.md"
in SKILL.md lacks a language tag triggering MD040; update that fenced block to
include a language identifier (e.g., text) so the block begins withtext and ends with, ensuring the URL remains unchanged inside the block.</details> </blockquote></details> <details> <summary>.agents/skills/ui-ux-pro-max/SKILL.md-178-184 (1)</summary><blockquote> `178-184`: _⚠️ Potential issue_ | _🟡 Minor_ **Specify the fenced code block language.** This block is missing a language tag (`MD040`). <details> <summary>📝 Suggested fix</summary> ```diff -``` +```text I am building the [Page Name] page. Please read design-system/MASTER.md. Also check if design-system/pages/[page-name].md exists. If the page file exists, prioritize its rules. If not, use the Master rules exclusively. Now, generate the code... ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/ui-ux-pro-max/SKILL.md around lines 178 - 184, The fenced
code block in SKILL.md (the snippet starting with triple backticks under the
Page Name instructions) is missing a language tag (MD040); update that code
fence to include an explicit language identifier (for example changetotext or ```markdown) so the block is properly declared and linting passes.</details> </blockquote></details> <details> <summary>src/lib/azure-blob.ts-114-117 (1)</summary><blockquote> `114-117`: _⚠️ Potential issue_ | _🟡 Minor_ **`deleted` is incremented unconditionally, ignoring `deleteIfExists`'s return value.** `deleteIfExists()` returns `BlobDeleteIfExistsResponse` with a `succeeded: boolean` property. If a blob disappears between the `listBlobsFlat` iteration and the `deleteIfExists` call (a valid race in concurrent workloads), the counter is over-reported. <details> <summary>🔧 Proposed fix</summary> ```diff for await (const blob of client.listBlobsFlat({ prefix })) { - await client.getBlockBlobClient(blob.name).deleteIfExists(); - deleted++; + const result = await client.getBlockBlobClient(blob.name).deleteIfExists(); + if (result.succeeded) deleted++; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/lib/azure-blob.ts` around lines 114 - 117, In the blob deletion loop that iterates over client.listBlobsFlat (inside the function handling blob cleanup), change the unconditional increment of deleted to only increment when the call to client.getBlockBlobClient(blob.name).deleteIfExists() returns a response with succeeded === true; i.e., await the BlobDeleteIfExistsResponse, check response.succeeded and increment deleted only on success (and handle/ignore false cases) so races where the blob is already gone won't over-count. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (10)</summary><blockquote> <details> <summary>next.config.ts (1)</summary><blockquote> `10-35`: **Consider adding `Content-Security-Policy` and `Strict-Transport-Security` to complete the security headers baseline.** The four new headers are a solid addition. Two notable gaps remain: 1. **`Content-Security-Policy`** — absent entirely. This is the primary browser-enforced defence against XSS and data-injection. Even a report-only policy or a restrictive starter policy would significantly raise the security posture. 2. **`Strict-Transport-Security`** — absent from the config. Vercel injects this header automatically on its edge, so it may already be covered at the infrastructure layer; if this app can ever be served outside Vercel, it's worth adding explicitly. Additionally, `X-Frame-Options: DENY` (line 24) is a legacy mechanism superseded by `Content-Security-Policy: frame-ancestors 'none'` in CSP Level 2+ browsers. Keeping both is fine for backward compat, but once a CSP is introduced `frame-ancestors` should be the primary control. <details> <summary>♻️ Suggested additions</summary> ```diff { key: "Permissions-Policy", value: "camera=(), microphone=(), geolocation=()", }, + { + key: "Strict-Transport-Security", + value: "max-age=63072000; includeSubDomains; preload", + }, + { + key: "Content-Security-Policy", + value: "default-src 'self'; script-src 'self'; object-src 'none'; frame-ancestors 'none'; upgrade-insecure-requests;", + }, ``` > **Note:** The CSP `script-src` value above is intentionally strict. Next.js with inline scripts or third-party SDKs (Sentry tunnel, analytics, etc.) will likely require `'nonce-...'` or specific `https:` origins — tune `script-src`, `connect-src`, and `img-src` to match your actual resource origins before enabling. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@next.config.ts` around lines 10 - 35, The headers array in your Next.js config is missing Content-Security-Policy and Strict-Transport-Security; update the exported next config's headers property (the headers array shown) to include a Content-Security-Policy header (start with a restrictive or report-only policy that includes frame-ancestors 'none' and tuned script-src/connect-src/img-src values for your app) and a Strict-Transport-Security header (e.g., max-age=63072000; includeSubDomains; preload) while keeping or optionally retaining X-Frame-Options for legacy browsers; ensure CSP is tested for inline scripts/third-party SDKs (use nonce or specific origins as needed) before enabling. ``` </details> </blockquote></details> <details> <summary>src/components/landing/hero.tsx (1)</summary><blockquote> `3-3`: **Remove unused `useEffect` and `useRef` imports.** Neither hook is referenced anywhere in the component body. <details> <summary>♻️ Proposed fix</summary> ```diff -import { useEffect, useRef } from "react"; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/components/landing/hero.tsx` at line 3, The import line in the Hero component currently brings in unused React hooks (useEffect, useRef); remove these unused symbols from the import statement (keep only React or other used imports) so the component (e.g., the Hero/default-export component in src/components/landing/hero.tsx) no longer imports useEffect or useRef. ``` </details> </blockquote></details> <details> <summary>src/app/api/quick-edit/route.ts (2)</summary><blockquote> `91-95`: **Move `quickEditResponseSchema` to module scope** Defining the schema inside the handler body causes it to be re-instantiated on every request. Lift it to module level alongside `quickEditRequestSchema`. <details> <summary>♻️ Proposed refactor</summary> ```diff +const quickEditResponseSchema = z.object({ + editedCode: z + .string() + .describe("The edited version of the selected code based on the instructions"), +}); + export async function POST(req: Request) { // ... - const quickEditResponseSchema = z.object({ - editedCode: z - .string() - .describe("The edited version of the selected code based on the instructions"), - }); - const { output } = await generateText({ ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/app/api/quick-edit/route.ts` around lines 91 - 95, The quickEditResponseSchema is declared inside the handler and gets re-created on every request; move its declaration to module scope next to quickEditRequestSchema so it's instantiated once per module load. Locate quickEditResponseSchema in route.ts and lift the z.object(...) definition out of the handler function (same scope where quickEditRequestSchema is defined), then reference that module-level quickEditResponseSchema from the handler. ``` </details> --- `87-87`: **Redundant `|| ""`** — `fullCode` already defaults to `""` via `z.string().optional().default("")` in the schema (Line 12), so the fallback on Line 87 is dead code. <details> <summary>♻️ Proposed fix</summary> ```diff - .replace("{fullCode}", fullCode || "") + .replace("{fullCode}", fullCode) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/app/api/quick-edit/route.ts` at line 87, Remove the redundant fallback in the string replacement: since the schema defines fullCode with z.string().optional().default(""), drop the "|| \"\"" after fullCode in the .replace call (the symbol to change is the .replace("{fullCode}", fullCode || "") expression in route.ts) so it becomes .replace("{fullCode}", fullCode); leave everything else unchanged. ``` </details> </blockquote></details> <details> <summary>src/features/conversations/inngest/tools/create-files.ts (1)</summary><blockquote> `83-83`: **`blobPath` embeds the raw Convex `parentId` (an opaque ID) rather than the folder's human-readable path.** The resulting path looks like `projects/{projectId}/{convexFolderId}/{fileName}`, which is not meaningful in Azure Blob Storage Explorer and makes debugging difficult. If the folder is later renamed or reparented, the blob path doesn't reflect that change anyway (since paths are write-once), but at minimum consider using an empty string or "root" only for the root case, and using the folder _path_ (resolved via `getFilePath`) for nested files to keep blob paths meaningful. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/features/conversations/inngest/tools/create-files.ts` at line 83, blobPath currently uses the raw Convex parentId (buildBlobPath(projectId, `${parentId || "root"}/${file.name}`)), which yields opaque blob paths; instead resolve the human-readable folder path for non-root parents by calling getFilePath(parentId) and use that (fallback to "" or "root" only when parentId is null/undefined) when constructing the blob path so blobs show meaningful folder names; update the blobPath construction to use buildBlobPath(projectId, `${resolvedParentPath || "root"}/${file.name}`) where resolvedParentPath is obtained from getFilePath(parentId) for nested files. ``` </details> </blockquote></details> <details> <summary>src/features/conversations/inngest/tools/read-files.ts (1)</summary><blockquote> `39-55`: **Sequential blob downloads — consider `Promise.all` for parallel fetching.** The `for...of` loop fetches both Convex records and blob content one at a time. For batches of files this creates O(n) sequential round trips. Parallelizing with `Promise.all` would significantly reduce latency. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/features/conversations/inngest/tools/read-files.ts` around lines 39 - 55, The loop in read-files is doing sequential convex.query and downloadBlob calls; change it to run these IO-bound steps in parallel: map fileIds to an array of promises that call convex.query(api.system.getFileById, { internalKey, fileId }) and await Promise.all to get all file records, then filter those with file && file.blobPath and map to downloadBlob(...) promises and await Promise.all (or Promise.allSettled with error handling) to fetch blobs in parallel; finally assemble results (using file._id, file.name and the corresponding content) preserving correspondence between files and downloaded content before pushing into results. ``` </details> </blockquote></details> <details> <summary>src/features/projects/components/file-explorer/tree.tsx (1)</summary><blockquote> `54-66`: **Extract shared “create text file” helper with compensation** This path repeats the same upload-then-mutate sequence as `src/features/projects/components/file-explorer/index.tsx` and carries the same orphan-blob risk. Centralize this in one helper (upload + create + rollback on failure) to avoid drift. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/features/projects/components/file-explorer/tree.tsx` around lines 54 - 66, The handleCreate branch for creating a text file duplicates the upload-then-create flow (uploadFileContent + createFile) and risks orphan blobs; extract a shared helper (e.g., createTextFileWithRollback) used by both file-explorer/tree.tsx and file-explorer/index.tsx that performs: uploadFileContent(blobPath, ""), call the backend mutate (createFile), and on mutation failure deletes the uploaded blob to roll back (and rethrow/return the error). Update handleCreate to call this helper (pass projectId, item._id as parentId, name, and blobPath builder) and ensure the helper surfaces errors so callers can set UI state consistently. ``` </details> </blockquote></details> <details> <summary>src/lib/azure-blob.ts (3)</summary><blockquote> `49-64`: **`uploadBlob` redundantly calls `createIfNotExists()` on every upload.** `ensureContainer()` is the designated startup/lazy-init entry point. Calling `client.createIfNotExists()` on line 54 inside `uploadBlob` issues an extra HTTP request to Azure on every single upload, adding latency and an unnecessary API call in the hot path. <details> <summary>♻️ Proposed fix</summary> ```diff export async function uploadBlob( blobPath: string, content: string ): Promise<string> { const client = getContainerClient(); - await client.createIfNotExists(); - const blockBlobClient = client.getBlockBlobClient(blobPath); ``` </details> Callers that need the container to exist should invoke `ensureContainer()` once at startup (or lazily before first use). Alternatively, `getContainerClient()` itself could drive that one-time check with a module-level `boolean` flag. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/lib/azure-blob.ts` around lines 49 - 64, The uploadBlob function currently calls client.createIfNotExists() on every upload, causing redundant HTTP calls; remove that call from uploadBlob and ensure the container existence is handled once elsewhere by having callers invoke ensureContainer() at startup or by updating getContainerClient() to perform a one-time check (use a module-level boolean like containerInitialized inside getContainerClient/ensureContainer to avoid repeated createIfNotExists calls). Update uploadBlob to rely on getContainerClient()/ensureContainer() guaranteeing the container exists and keep uploadBlob focused only on uploading the blob. ``` </details> --- `135-139`: **`renameBlob` silently ignores a failed source-blob delete after a successful copy.** If `pollUntilDone()` succeeds but `sourceBlob.deleteIfExists()` throws (transient error, permission issue), the exception propagates to the caller who sees a failure, yet the destination blob was already created. The rename is effectively complete but the caller may retry, leading to a second copy of destination and another delete attempt. This is low-risk since data is never lost, but callers should be aware of the partial-success scenario. Consider wrapping the delete in a try/catch that logs the error without rethrowing, or document that the function is not atomic. <details> <summary>♻️ Proposed fix</summary> ```diff // Copy from source to destination const copyResult = await destBlob.beginCopyFromURL(sourceBlob.url); await copyResult.pollUntilDone(); - // Delete the source blob - await sourceBlob.deleteIfExists(); + // Best-effort delete; copy has already succeeded so data is safe + try { + await sourceBlob.deleteIfExists(); + } catch { + // Log or surface as a warning; the rename data is intact at newBlobPath + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/lib/azure-blob.ts` around lines 135 - 139, The renameBlob implementation currently performs destBlob.beginCopyFromURL(...) and await copyResult.pollUntilDone() then calls sourceBlob.deleteIfExists() which can throw and make callers think the operation failed even though the destination exists; update renameBlob to wrap the sourceBlob.deleteIfExists() call in a try/catch that logs the deletion error (include contextual info such as sourceBlob.name and destBlob.name and the caught error) but does not rethrow, so the function returns success after a completed copy; alternatively add a clear comment/docstring on renameBlob describing that delete failures are non-fatal—prefer the try/catch-and-log approach for robustness. ``` </details> --- `86-89`: **Use `RestError` for type-safe error handling instead of unsafe manual cast.** The current code casts `error` to `{ statusCode?: number }`, which silently accepts any object with that shape, including non-Azure errors. The `@azure/storage-blob` package (v12.31.0+) exports `RestError` from `@azure/core-rest-pipeline`, which is the actual error type thrown by Azure SDK operations with a properly typed `statusCode` field. <details> <summary>♻️ Proposed fix</summary> ```diff -import { +import { BlobServiceClient, ContainerClient, StorageSharedKeyCredential, + RestError, } from "@azure/storage-blob"; ``` ```diff - } catch (error: unknown) { - const blobError = error as { statusCode?: number }; - if (blobError.statusCode === 404) return null; + } catch (error: unknown) { + if (error instanceof RestError && error.statusCode === 404) return null; throw error; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/lib/azure-blob.ts` around lines 86 - 89, Replace the unsafe cast in the catch block with a type-safe check using RestError: import RestError from '@azure/core-rest-pipeline' (or the named export used by your SDK), then in the catch (error: unknown) test if error is an instance of RestError and if so check error.statusCode === 404 to return null; for any other error (non-RestError or RestError with different status) rethrow the original error. Update the catch in the function/method that currently casts to { statusCode?: number } (the block around the blob read/delete logic in src/lib/azure-blob.ts) accordingly. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ba8c81cd3bdcb840996956a8631063efae4afd8b and 47ecd64906f2cb57f17125e3af6d84e3e5a00bba. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (39)</summary> * `.agents/skills/frontend-design/LICENSE.txt` * `.agents/skills/frontend-design/SKILL.md` * `.agents/skills/ui-ux-pro-max/SKILL.md` * `.agents/skills/ui-ux-pro-max/data` * `.agents/skills/ui-ux-pro-max/scripts` * `.agents/skills/web-design-guidelines/SKILL.md` * `convex/files.ts` * `convex/schema.ts` * `convex/system.ts` * `next.config.ts` * `package.json` * `skills-lock.json` * `src/app/api/files/content/route.ts` * `src/app/api/github/export/route.ts` * `src/app/api/github/import/route.ts` * `src/app/api/messages/cancel/route.ts` * `src/app/api/quick-edit/route.ts` * `src/app/api/suggestion/route.ts` * `src/app/page.tsx` * `src/app/projects/page.tsx` * `src/components/landing/hero.tsx` * `src/components/landing/navbar.tsx` * `src/features/conversations/inngest/tools/create-files.ts` * `src/features/conversations/inngest/tools/read-files.ts` * `src/features/conversations/inngest/tools/update-file.ts` * `src/features/editor/components/editor-view.tsx` * `src/features/preview/hooks/use-webcontainer.ts` * `src/features/preview/utils/file-tree.tsx` * `src/features/projects/components/export-popover.tsx` * `src/features/projects/components/file-explorer/index.tsx` * `src/features/projects/components/file-explorer/tree.tsx` * `src/features/projects/components/import-github-dialog.tsx` * `src/features/projects/hooks/use-files.ts` * `src/features/projects/inngest/export-to-github.ts` * `src/features/projects/inngest/import-to-github-repo.ts` * `src/hooks/use-file-content.ts` * `src/inngest/client.ts` * `src/inngest/events.ts` * `src/lib/azure-blob.ts` </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * src/features/projects/components/export-popover.tsx </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+22
to
+28
| // Validate that the blobPath starts with "projects/" to prevent path traversal | ||
| if (!blobPath.startsWith("projects/")) { | ||
| return NextResponse.json( | ||
| { error: "Invalid blob path" }, | ||
| { status: 400 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Enforce project-level authorization, not only path-prefix validation
The projects/ prefix check is not sufficient authorization. An authenticated user can still target another project’s blob path. Parse projectId from blobPath and verify ownership/access before downloadBlob and uploadBlob.
Also applies to: 63-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/files/content/route.ts` around lines 22 - 28, The current check
only ensures blobPath starts with "projects/" but does not enforce project-level
authorization; parse the projectId from blobPath (e.g., extract the segment
after "projects/"), then call your auth/access check (or verify the
authenticated user's id/claims) to confirm the user has access to that project
before calling downloadBlob or uploadBlob; if the user lacks access, return
NextResponse.json({ error: "Unauthorized" }, { status: 403 }). Apply the same
projectId parsing and authorization guard in the other block that handles
uploads (the code around downloadBlob/uploadBlob).
Comment on lines
+60
to
+62
| const body = await request.json(); | ||
| const { blobPath, content } = uploadSchema.parse(body); | ||
|
|
There was a problem hiding this comment.
Handle invalid JSON/Zod input as 400 instead of uncaught errors
Line 61 uses uploadSchema.parse(body) directly. Invalid payloads currently throw and can bubble as 500. Use safeParse (and guard request.json() failures) to return deterministic client errors.
💡 Suggested validation flow
- const body = await request.json();
- const { blobPath, content } = uploadSchema.parse(body);
+ let body: unknown;
+ try {
+ body = await request.json();
+ } catch {
+ return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 });
+ }
+ const parsed = uploadSchema.safeParse(body);
+ if (!parsed.success) {
+ return NextResponse.json({ error: "Invalid request body" }, { status: 400 });
+ }
+ const { blobPath, content } = parsed.data;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const body = await request.json(); | |
| const { blobPath, content } = uploadSchema.parse(body); | |
| let body: unknown; | |
| try { | |
| body = await request.json(); | |
| } catch { | |
| return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 }); | |
| } | |
| const parsed = uploadSchema.safeParse(body); | |
| if (!parsed.success) { | |
| return NextResponse.json({ error: "Invalid request body" }, { status: 400 }); | |
| } | |
| const { blobPath, content } = parsed.data; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/files/content/route.ts` around lines 60 - 62, The handler
currently uses request.json() and uploadSchema.parse(body) which can throw and
surface as 500s; wrap request.json() in a try/catch to catch invalid JSON and
return a 400, then replace uploadSchema.parse(body) with
uploadSchema.safeParse(body), check the result.success flag and return a 400
with validation errors when false; update the code paths around the symbols
request.json(), uploadSchema.parse, and where the parsed { blobPath, content }
are used so only validated data proceeds.
Comment on lines
+71
to
+79
| const event = await inngest.send({ | ||
| name: "github/import.repo", | ||
| data: { | ||
| owner, | ||
| repo, | ||
| projectId, | ||
| githubToken, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
githubToken passed in the Inngest event payload is a security concern and inconsistent with the export route.
The export route (src/app/api/github/export/route.ts) was specifically refactored to pass userId instead of githubToken in the event payload, so the token is fetched server-side in the Inngest handler. This import route still embeds the raw token in the event data, which gets persisted in Inngest's event log.
🔒 Proposed fix — align with export route pattern
const event = await inngest.send({
name: "github/import.repo",
data: {
owner,
repo,
projectId,
- githubToken,
+ userId,
},
});The corresponding Inngest handler (import-to-github-repo.ts) should then fetch the token server-side using clerkClient with the provided userId, mirroring export-to-github.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const event = await inngest.send({ | |
| name: "github/import.repo", | |
| data: { | |
| owner, | |
| repo, | |
| projectId, | |
| githubToken, | |
| }, | |
| }); | |
| const event = await inngest.send({ | |
| name: "github/import.repo", | |
| data: { | |
| owner, | |
| repo, | |
| projectId, | |
| userId, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/github/import/route.ts` around lines 71 - 79, Replace the raw
githubToken in the Inngest event payload emitted by
src/app/api/github/import/route.ts (the call that sends "github/import.repo")
with the current user's userId so sensitive tokens are not persisted; update the
payload to include userId instead of githubToken and remove any propagation of
githubToken, and then update the corresponding handler import-to-github-repo.ts
to fetch the token server-side via clerkClient.getUser or the same helper used
by export-to-github.ts (mirroring the pattern in export-to-github.ts) so the
token is resolved by userId in the Inngest handler rather than sent in the
event.
Comment on lines
+174
to
+181
| if (file.blobPath) { | ||
| // Text file — download content from Azure Blob | ||
| const { downloadBlob } = await import("@/lib/azure-blob"); | ||
| const blobContent = await downloadBlob(file.blobPath); | ||
| if (blobContent === null) { | ||
| continue; // Skip files with no blob content | ||
| } | ||
| content = blobContent; |
There was a problem hiding this comment.
Do not silently skip missing blob content during export
Line 179 continue can produce partial exports while still marking status completed. Treat missing blob content as a hard failure (or collect and report missing files explicitly) so the export result is trustworthy.
Comment on lines
+169
to
180
| // Upload text content to Azure Blob Storage | ||
| const { uploadBlob, buildBlobPath } = await import("@/lib/azure-blob"); | ||
| const blobPath = buildBlobPath(projectId, file.path); | ||
| await uploadBlob(blobPath, content); | ||
|
|
||
| await convex.mutation(api.system.createFile, { | ||
| internalKey, | ||
| projectId, | ||
| name, | ||
| content, | ||
| blobPath, | ||
| parentId, | ||
| }); |
There was a problem hiding this comment.
Blob upload + metadata write should be failure-safe per file
Line 172 uploads content, then Line 174 writes DB metadata. If mutation fails, the blob is stranded. Also, dynamic-importing Azure helpers inside each iteration adds avoidable overhead.
💡 Suggested structure
- const { uploadBlob, buildBlobPath } = await import("@/lib/azure-blob");
+ const { uploadBlob, buildBlobPath, deleteBlob } = await import("@/lib/azure-blob");
const blobPath = buildBlobPath(projectId, file.path);
await uploadBlob(blobPath, content);
- await convex.mutation(api.system.createFile, {
- internalKey,
- projectId,
- name,
- blobPath,
- parentId,
- });
+ try {
+ await convex.mutation(api.system.createFile, {
+ internalKey,
+ projectId,
+ name,
+ blobPath,
+ parentId,
+ });
+ } catch (e) {
+ await deleteBlob(blobPath).catch(() => {});
+ throw e;
+ }
Comment on lines
+12
to
+40
| const fetchContent = useCallback(async () => { | ||
| if (!blobPath) { | ||
| setContent(null); | ||
| return; | ||
| } | ||
|
|
||
| setIsLoading(true); | ||
| setError(null); | ||
|
|
||
| try { | ||
| const res = await fetch( | ||
| `/api/files/content?blobPath=${encodeURIComponent(blobPath)}` | ||
| ); | ||
| if (!res.ok) { | ||
| throw new Error(`Failed to fetch file content: ${res.status}`); | ||
| } | ||
| const data = await res.json(); | ||
| setContent(data.content ?? null); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : "Failed to fetch content"); | ||
| setContent(null); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }, [blobPath]); | ||
|
|
||
| useEffect(() => { | ||
| fetchContent(); | ||
| }, [fetchContent]); |
There was a problem hiding this comment.
Guard against stale fetch responses when blobPath changes
Line 12-36 can race: a slower request for old blobPath may resolve after a newer one and overwrite state. Add a request token/abort guard, and reset loading/error on empty blobPath.
💡 Suggested guard pattern
-import { useState, useEffect, useCallback } from "react";
+import { useState, useEffect, useCallback, useRef } from "react";
...
+ const requestIdRef = useRef(0);
const fetchContent = useCallback(async () => {
+ const requestId = ++requestIdRef.current;
if (!blobPath) {
setContent(null);
+ setError(null);
+ setIsLoading(false);
return;
}
...
- const data = await res.json();
- setContent(data.content ?? null);
+ const data = await res.json();
+ if (requestId === requestIdRef.current) {
+ setContent(data.content ?? null);
+ }
} catch (err) {
- setError(err instanceof Error ? err.message : "Failed to fetch content");
- setContent(null);
+ if (requestId === requestIdRef.current) {
+ setError(err instanceof Error ? err.message : "Failed to fetch content");
+ setContent(null);
+ }
} finally {
- setIsLoading(false);
+ if (requestId === requestIdRef.current) {
+ setIsLoading(false);
+ }
}
}, [blobPath]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/use-file-content.ts` around lines 12 - 40, fetchContent can suffer
from race conditions when blobPath changes: a slower previous fetch may resolve
and overwrite state; also loading/error should be reset when blobPath is empty.
Fix by adding a request-scoped token or AbortController inside useEffect and
pass it into fetchContent (or create the fetch inside the effect) so only the
latest response updates state; check the token/aborted flag before calling
setContent/setError/setIsLoading, and call controller.abort() in the effect
cleanup. Update the blobPath-empty branch to explicitly reset
setIsLoading(false) and setError(null) immediately when blobPath is falsy so
state is consistent.
Comment on lines
+24
to
+30
| "github/import.repo": { | ||
| data: { | ||
| owner: string; | ||
| repo: string; | ||
| projectId: Id<"projects">; | ||
| githubToken: string; | ||
| }; |
There was a problem hiding this comment.
Do not put GitHub access tokens in event payloads.
Embedding githubToken in event data risks secret leakage via event persistence/logging/inspection paths. Send a token reference (or user/project key) and resolve the token server-side at execution time.
🔒 Safer payload shape
"github/import.repo": {
data: {
owner: string;
repo: string;
projectId: Id<"projects">;
- githubToken: string;
+ githubConnectionId: string;
};
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "github/import.repo": { | |
| data: { | |
| owner: string; | |
| repo: string; | |
| projectId: Id<"projects">; | |
| githubToken: string; | |
| }; | |
| "github/import.repo": { | |
| data: { | |
| owner: string; | |
| repo: string; | |
| projectId: Id<"projects">; | |
| githubConnectionId: string; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/inngest/events.ts` around lines 24 - 30, The "github/import.repo" event
schema currently includes githubToken in the data shape—remove githubToken from
the event payload and replace it with a non-secret reference (e.g., tokenRef,
credentialsId, or rely on existing projectId/userId) so secrets are not sent in
events; update the event type definition in src/inngest/events.ts (the
"github/import.repo" data interface) and change any producers to send the
reference instead, then resolve and fetch the actual token server-side inside
the import handler (the code that processes "github/import.repo") using secure
storage or the project credentials service and update validation/tests/consumers
accordingly.
Comment on lines
+100
to
+104
| try { | ||
| await blockBlobClient.deleteIfExists(); | ||
| } catch { | ||
| // Silently ignore if blob doesn't exist | ||
| } |
There was a problem hiding this comment.
deleteBlob silently swallows all errors, not just 404s.
deleteIfExists() already handles the non-existence case internally — it does not throw on a 404 and returns { succeeded: boolean }. The surrounding try/catch with an empty handler is therefore redundant for its stated purpose, but harmful in practice: auth failures, network errors, throttling, and any other real errors are silently discarded, making failures invisible to the caller.
🔧 Proposed fix
export async function deleteBlob(blobPath: string): Promise<void> {
const client = getContainerClient();
const blockBlobClient = client.getBlockBlobClient(blobPath);
-
- try {
- await blockBlobClient.deleteIfExists();
- } catch {
- // Silently ignore if blob doesn't exist
- }
+ await blockBlobClient.deleteIfExists();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| await blockBlobClient.deleteIfExists(); | |
| } catch { | |
| // Silently ignore if blob doesn't exist | |
| } | |
| export async function deleteBlob(blobPath: string): Promise<void> { | |
| const client = getContainerClient(); | |
| const blockBlobClient = client.getBlockBlobClient(blobPath); | |
| await blockBlobClient.deleteIfExists(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/azure-blob.ts` around lines 100 - 104, The current deleteBlob
implementation swallows all errors by wrapping blockBlobClient.deleteIfExists()
in an empty try/catch; since deleteIfExists already returns { succeeded: boolean
} for non-existent blobs, remove the empty try/catch and let real errors
propagate (or rethrow them) so auth/network/throttling failures are not hidden;
update the deleteBlob function (and its call sites if needed) to call
blockBlobClient.deleteIfExists() directly and handle the returned succeeded flag
or propagate exceptions from blockBlobClient.deleteIfExists().
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/api/github/import/route.ts (1)
14-20:⚠️ Potential issue | 🟠 MajorTighten GitHub URL parsing to avoid false positives.
The current regex is too permissive (
notgithub.comcan match, and repo can include query/hash). Parse withURLand validate hostname/path segments explicitly.🐛 Proposed fix
function parseGitHubUrl(url: string) { - const match = url.match(/github\.com\/([^/]+)\/([^/]+)/); - if (!match) { - throw new Error("Invalid GitHub URL"); - } - - return { owner: match[1], repo: match[2].replace(/\.git$/, "") }; + const parsed = new URL(url); + const hostname = parsed.hostname.toLowerCase(); + if (hostname !== "github.com" && hostname !== "www.github.com") { + throw new Error("Invalid GitHub URL"); + } + + const segments = parsed.pathname.split("/").filter(Boolean); + if (segments.length < 2) { + throw new Error("Invalid GitHub URL"); + } + + const [owner, repoSegment] = segments; + const repo = repoSegment.replace(/\.git$/, ""); + if (!owner || !repo) { + throw new Error("Invalid GitHub URL"); + } + + return { owner, repo }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/github/import/route.ts` around lines 14 - 20, Replace the permissive regex in parseGitHubUrl with robust URL parsing: construct a URL(...) inside parseGitHubUrl, catch invalid URLs and throw the existing error, then validate that url.hostname is "github.com" or "www.github.com" and that url.pathname split by "/" yields at least two non-empty segments; use the first two segments as owner and repo, strip a trailing ".git" from repo and return { owner, repo }. This eliminates false positives like "notgithub.com" and prevents query/hash from being included in the repo name.convex/system.ts (2)
625-629:⚠️ Potential issue | 🔴 CriticalCritical: Same incorrect
ctx.db.patchsignature issue.This has the same bug as
updateImportStatus- using three arguments instead of two.🐛 Proposed fix
- await ctx.db.patch("projects", args.projectId, { + await ctx.db.patch(args.projectId, { exportStatus: args.status, exportRepoUrl: args.repoUrl, updatedAt: Date.now(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/system.ts` around lines 625 - 629, The patch call uses the wrong three-argument signature; change ctx.db.patch("projects", args.projectId, { ... }) to the two-argument form expected by the DB client by passing a single object with the id plus fields to update (e.g. ctx.db.patch("projects", { id: args.projectId, exportStatus: args.status, exportRepoUrl: args.repoUrl, updatedAt: Date.now() })); mirror the same fix you applied in updateImportStatus to ensure the DB patch is invoked correctly.
601-605:⚠️ Potential issue | 🔴 CriticalCritical: Incorrect
ctx.db.patchsignature will cause runtime error.The
ctx.db.patchmethod in Convex takes two arguments(id, fields), not three arguments(tableName, id, fields). This call will fail at runtime.Compare with correct usage elsewhere in this file (e.g., line 56, 73, 219, 271, 451):
await ctx.db.patch(args.conversationId, { updatedAt: Date.now() });🐛 Proposed fix
- await ctx.db.patch("projects", args.projectId, { + await ctx.db.patch(args.projectId, { importStatus: args.status, updatedAt: Date.now(), });Note: The same issue exists at line 625 in the
updateExportStatusmutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/system.ts` around lines 601 - 605, The ctx.db.patch call is using the wrong Convex signature (tableName, id, fields); change the calls to use (id, fields) instead — for example update the patch in the import status mutation from ctx.db.patch("projects", args.projectId, { ... }) to ctx.db.patch(args.projectId, { importStatus: args.status, updatedAt: Date.now() }) and make the same fix in the updateExportStatus mutation where ctx.db.patch is currently called with three arguments so it becomes ctx.db.patch(args.projectId, { exportStatus: args.status, updatedAt: Date.now() }) ensuring you only pass the record id and the fields object.
♻️ Duplicate comments (2)
src/app/api/github/import/route.ts (1)
80-87:⚠️ Potential issue | 🟠 MajorDo not send raw
githubTokenin event payload.This repeats the previously raised concern and is still unresolved: sending OAuth tokens in event data increases secret exposure risk. Pass
userIdand resolve token inside the consumer.🔒 Proposed fix
const event = await inngest.send({ name: "github/import.repo", data: { owner, repo, projectId, - githubToken, + userId, }, });#!/bin/bash set -euo pipefail # Verify no raw github token is emitted in import event payloads rg -n -C3 'github/import\.repo|githubToken|userId' # Verify the import consumer resolves token server-side (by userId) fd 'import-to-github-repo\.(ts|js)$' -x sh -c ' echo "=== {} ===" rg -n -C3 "userId|githubToken|getUserOauthAccessToken|clerkClient" "{}" '🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/github/import/route.ts` around lines 80 - 87, The event currently includes a raw githubToken (see the inngest.send call for event "github/import.repo" where githubToken is in data); remove githubToken from the payload and include userId instead (keep owner, repo, projectId). Update the consumer/handler for the "github/import.repo" event so it resolves the OAuth token server-side (e.g., via your user/token lookup like getUserOauthAccessToken or Clerk client) using the passed userId before calling GitHub. Ensure all references to githubToken in the producer (inngest.send) are removed and the consumer fetches the token from secure storage by userId.src/app/api/files/content/route.ts (1)
91-92:⚠️ Potential issue | 🟠 MajorHandle invalid JSON and schema validation errors as 400 responses.
The current implementation uses
request.json()anduploadSchema.parse(body)directly. Invalid JSON or schema violations will throw uncaught exceptions, resulting in 500 errors instead of proper 400 client errors.🛡️ Proposed fix using safeParse pattern
- const body = await request.json(); - const { blobPath, content } = uploadSchema.parse(body); + let body: unknown; + try { + body = await request.json(); + } catch { + return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 }); + } + const parsed = uploadSchema.safeParse(body); + if (!parsed.success) { + return NextResponse.json({ error: "Invalid request body" }, { status: 400 }); + } + const { blobPath, content } = parsed.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/files/content/route.ts` around lines 91 - 92, Wrap the body parsing and validation so invalid JSON and schema errors produce 400 responses: catch JSON parse errors from request.json() and return a 400 with a clear message, and replace uploadSchema.parse(body) with uploadSchema.safeParse(body) (or try/catch around parse) in the route handler so when safeParse().success is false you return a 400 containing the validation issues instead of letting an exception bubble to a 500; reference the existing request.json() call and uploadSchema.parse/uploadSchema.safeParse to locate where to add the error handling.
🧹 Nitpick comments (2)
convex/system.ts (1)
496-502: Minor typos in comments.Small typos to clean up if convenient:
- Line 496: "sto rage" → "storage"
- Line 501: "fie/folder" → "file/folder"
📝 Suggested fix
- // Delete sto rage file if it exists + // Delete storage file if it exists if (item.storageId) { await ctx.storage.delete(item.storageId); } - // Delete the fie/folder itself + // Delete the file/folder itself await ctx.db.delete(fileId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/system.ts` around lines 496 - 502, Fix the minor typos in the comments around the file delete logic: change "sto rage" to "storage" and "fie/folder" to "file/folder" in the block that checks item.storageId and calls ctx.storage.delete(item.storageId) and then calls ctx.db.delete(fileId); keep the comments semantically the same but corrected to "Delete storage file if it exists" and "Delete the file/folder itself" near those statements.README.md (1)
155-155: Add language specifier to the fenced code block.The project structure code block should specify a language (e.g.,
textorplaintext) for better accessibility and consistent markdown rendering.📝 Suggested fix
-``` +```text moris/ ├── src/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 155, The fenced code block in README.md showing the project structure lacks a language specifier; update the opening triple-backtick for that block (the block containing "moris/ ├── src/") to include a language such as "text" or "plaintext" (e.g., change "```" to "```text") so markdown renderers and accessibility tools correctly recognize it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/github/import/route.ts`:
- Line 31: Wrap the calls to request.json() (the one assigning to body and the
similar call around lines 95-100) in a try/catch that detects JSON parse
failures (e.g., SyntaxError or any thrown by request.json()) and return an HTTP
400 response with a clear client-error message instead of letting it bubble to
the outer 500 handler; update the code around the body = await request.json()
site and the other request.json() call to catch the error and return a Response
with status 400 and an explanatory message so malformed JSON is treated as a
client error.
- Around line 96-100: The handler currently logs the full error but returns
error.message to clients; change the response to avoid exposing internal details
by returning a generic message (e.g., "Failed to import repository") instead of
using error.message, while keeping the detailed error in the server log via
console.error(error). Update the NextResponse.json call so its body does not
interpolate error.message and ensure any conditional using error instanceof
Error only affects logging, not the client response; reference the existing
console.error, error variable and NextResponse.json usage in this route.
---
Outside diff comments:
In `@convex/system.ts`:
- Around line 625-629: The patch call uses the wrong three-argument signature;
change ctx.db.patch("projects", args.projectId, { ... }) to the two-argument
form expected by the DB client by passing a single object with the id plus
fields to update (e.g. ctx.db.patch("projects", { id: args.projectId,
exportStatus: args.status, exportRepoUrl: args.repoUrl, updatedAt: Date.now()
})); mirror the same fix you applied in updateImportStatus to ensure the DB
patch is invoked correctly.
- Around line 601-605: The ctx.db.patch call is using the wrong Convex signature
(tableName, id, fields); change the calls to use (id, fields) instead — for
example update the patch in the import status mutation from
ctx.db.patch("projects", args.projectId, { ... }) to
ctx.db.patch(args.projectId, { importStatus: args.status, updatedAt: Date.now()
}) and make the same fix in the updateExportStatus mutation where ctx.db.patch
is currently called with three arguments so it becomes
ctx.db.patch(args.projectId, { exportStatus: args.status, updatedAt: Date.now()
}) ensuring you only pass the record id and the fields object.
In `@src/app/api/github/import/route.ts`:
- Around line 14-20: Replace the permissive regex in parseGitHubUrl with robust
URL parsing: construct a URL(...) inside parseGitHubUrl, catch invalid URLs and
throw the existing error, then validate that url.hostname is "github.com" or
"www.github.com" and that url.pathname split by "/" yields at least two
non-empty segments; use the first two segments as owner and repo, strip a
trailing ".git" from repo and return { owner, repo }. This eliminates false
positives like "notgithub.com" and prevents query/hash from being included in
the repo name.
---
Duplicate comments:
In `@src/app/api/files/content/route.ts`:
- Around line 91-92: Wrap the body parsing and validation so invalid JSON and
schema errors produce 400 responses: catch JSON parse errors from request.json()
and return a 400 with a clear message, and replace uploadSchema.parse(body) with
uploadSchema.safeParse(body) (or try/catch around parse) in the route handler so
when safeParse().success is false you return a 400 containing the validation
issues instead of letting an exception bubble to a 500; reference the existing
request.json() call and uploadSchema.parse/uploadSchema.safeParse to locate
where to add the error handling.
In `@src/app/api/github/import/route.ts`:
- Around line 80-87: The event currently includes a raw githubToken (see the
inngest.send call for event "github/import.repo" where githubToken is in data);
remove githubToken from the payload and include userId instead (keep owner,
repo, projectId). Update the consumer/handler for the "github/import.repo" event
so it resolves the OAuth token server-side (e.g., via your user/token lookup
like getUserOauthAccessToken or Clerk client) using the passed userId before
calling GitHub. Ensure all references to githubToken in the producer
(inngest.send) are removed and the consumer fetches the token from secure
storage by userId.
---
Nitpick comments:
In `@convex/system.ts`:
- Around line 496-502: Fix the minor typos in the comments around the file
delete logic: change "sto rage" to "storage" and "fie/folder" to "file/folder"
in the block that checks item.storageId and calls
ctx.storage.delete(item.storageId) and then calls ctx.db.delete(fileId); keep
the comments semantically the same but corrected to "Delete storage file if it
exists" and "Delete the file/folder itself" near those statements.
In `@README.md`:
- Line 155: The fenced code block in README.md showing the project structure
lacks a language specifier; update the opening triple-backtick for that block
(the block containing "moris/ ├── src/") to include a language such as "text" or
"plaintext" (e.g., change "```" to "```text") so markdown renderers and
accessibility tools correctly recognize it.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdconvex/system.tssrc/app/api/files/content/route.tssrc/app/api/github/import/route.ts
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
|
|
||
| const body = await request.json(); |
There was a problem hiding this comment.
Handle malformed JSON as 400, not 500.
request.json() can throw on invalid JSON and currently falls into the outer 500 path. Treat this as a client error.
🐛 Proposed fix
- const body = await request.json();
+ let body: unknown;
+ try {
+ body = await request.json();
+ } catch {
+ return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 });
+ }Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/github/import/route.ts` at line 31, Wrap the calls to
request.json() (the one assigning to body and the similar call around lines
95-100) in a try/catch that detects JSON parse failures (e.g., SyntaxError or
any thrown by request.json()) and return an HTTP 400 response with a clear
client-error message instead of letting it bubble to the outer 500 handler;
update the code around the body = await request.json() site and the other
request.json() call to catch the error and return a Response with status 400 and
an explanatory message so malformed JSON is treated as a client error.
Comment on lines
+96
to
100
| console.error("[github/import] Error:", error); | ||
| const message = error instanceof Error ? error.message : "Unknown error"; | ||
| return NextResponse.json( | ||
| { error: "Server configuration error" }, | ||
| { error: `Failed to import repository: ${message}` }, | ||
| { status: 500 } |
There was a problem hiding this comment.
Avoid returning raw internal error details to clients.
error.message can expose internal/system details. Keep detailed info in logs and return a generic error response body.
🛡️ Proposed fix
- const message = error instanceof Error ? error.message : "Unknown error";
return NextResponse.json(
- { error: `Failed to import repository: ${message}` },
+ { error: "Failed to import repository" },
{ status: 500 }
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.error("[github/import] Error:", error); | |
| const message = error instanceof Error ? error.message : "Unknown error"; | |
| return NextResponse.json( | |
| { error: "Server configuration error" }, | |
| { error: `Failed to import repository: ${message}` }, | |
| { status: 500 } | |
| console.error("[github/import] Error:", error); | |
| return NextResponse.json( | |
| { error: "Failed to import repository" }, | |
| { status: 500 } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/github/import/route.ts` around lines 96 - 100, The handler
currently logs the full error but returns error.message to clients; change the
response to avoid exposing internal details by returning a generic message
(e.g., "Failed to import repository") instead of using error.message, while
keeping the detailed error in the server log via console.error(error). Update
the NextResponse.json call so its body does not interpolate error.message and
ensure any conditional using error instanceof Error only affects logging, not
the client response; reference the existing console.error, error variable and
NextResponse.json usage in this route.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores