refactor image update logic in uploadImg function #59
Conversation
WalkthroughuploadImage now always generates a UUID-based filename (preserving the original file extension) and, if a fileUrl is provided, deletes the existing image (via storage.remove) before uploading the new file; tests updated to expect UUID-named uploads and the removal call. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant uploadImage as uploadImage()
participant deleteImage as deleteImage()
participant Storage
User->>uploadImage: call with (file, fileUrl?)
alt fileUrl provided
rect `#E8F0FF`
uploadImage->>deleteImage: delete(fileUrl)
deleteImage->>Storage: remove(oldPath)
Storage-->>deleteImage: { error: null } / removed
deleteImage-->>uploadImage: deleted
end
end
rect `#EFFFE8`
uploadImage->>uploadImage: generate UUID + original extension -> filename
end
uploadImage->>Storage: upload(file, filename)
Storage-->>uploadImage: uploaded -> publicUrl
uploadImage-->>User: return publicUrl
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/utils/imageUtils.ts (1)
76-78: Update test mock forstorage.remove()in uploadImage test case.The pipeline failure occurs in the test "uses existing filename from fileUrl if provided" (line 48–65). When
uploadImageis called with afileUrl, it invokesdeleteImage, which expectsstorage.remove()to return{ error, data }. However, the mock forremoveis not configured in this test, causing it to returnundefinedand triggering a destructuring error.Add the following line before calling
uploadImagein that test:storageMock.remove.mockResolvedValue({ error: null });
🧹 Nitpick comments (2)
src/utils/imageUtils.ts (2)
48-53: Theupsert: trueoption is now redundant.Since the filename is now always a fresh UUID, there's no scenario where you'd overwrite an existing file. The
upsertoption can be removed or set tofalse..upload(filePath, file.buffer, { contentType: file.mimetype, - upsert: true, });
44-44: Remove redundant type annotation and fix spacing.TypeScript automatically infers the
stringtype from the template literal, making the explicit annotation unnecessary. Also, there's inconsistent spacing around the assignment operator.- const filename:string = `${uuidv4()}.${ext}`; + const filename = `${uuidv4()}.${ext}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/imageUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/imageUtils.ts (1)
src/app.ts (1)
supabase(12-15)
🪛 GitHub Actions: Run Tests
src/utils/imageUtils.ts
[error] 76-76: TypeError: Cannot destructure property 'error' of '(intermediate value)' as it is undefined during uploadImage test. Ensure the storage.remove call returns an object with an 'error' field.
…f-code/COC-API into member-Profile-update-fix
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/imageUtils.test.ts (1)
58-81: Update test name and verify deletion behavior.The test name and comment (lines 58-59, 74) are outdated. The function now always generates a UUID-based filename rather than reusing the existing one. Additionally, the test doesn't verify that the old image is actually deleted when
fileUrlis provided.Apply this diff to update the test:
- it("uses existing filename from fileUrl if provided", async () => { - // Simulate providing an existing URL so the helper extracts the filename + it("generates new UUID filename and deletes old image when fileUrl provided", async () => { const existingUrl = "https://xyz.supabase.co/storage/v1/object/public/images/folder/existing.png"; storageMock.upload.mockResolvedValue({ error: null }); storageMock.getPublicUrl.mockReturnValue({ data: { publicUrl: "https://public.url/existing.png" }, }); storageMock.remove.mockReturnValue({error:null}); const url = await uploadImage( mockSupabase as SupabaseClient, dummyFile, "folder", existingUrl, ); - // Should use filename 'existing.png' instead of generating with uuid + // Should generate new UUID filename expect(storageMock.upload).toHaveBeenCalledWith( "folder/test-uuid.png", dummyFile.buffer, { contentType: dummyFile.mimetype, upsert: true }, ); + // Should delete the old image + expect(storageMock.remove).toHaveBeenCalledWith(["folder/existing.png"]); expect(url).toBe("https://public.url/existing.png"); });
🧹 Nitpick comments (1)
src/utils/imageUtils.ts (1)
63-65: Race condition resolved; consider error handling for deletion.The race condition from the previous review is now fixed—deletion occurs after successful upload, preventing data loss. However, if the deletion fails, the error is currently unhandled and the old image remains in storage.
Consider wrapping the deletion in a try-catch to log failures without blocking the upload flow:
if (fileUrl) { - await deleteImage(supabase, fileUrl); + try { + await deleteImage(supabase, fileUrl); + } catch (error) { + // Log but don't fail - new image already uploaded + console.error("Failed to delete old image:", error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/imageUtils.ts(2 hunks)tests/imageUtils.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/imageUtils.ts (1)
src/app.ts (1)
supabase(12-15)
| } else { | ||
| filename = `${uuidv4()}.${ext}`; | ||
| } | ||
| const filename:string = `${uuidv4()}.${ext}`; |
There was a problem hiding this comment.
Fix type annotation spacing.
The type annotation is missing a space after the colon, and there's an extra space before the equals sign.
Apply this diff to fix the formatting:
- const filename:string = `${uuidv4()}.${ext}`;
+ const filename: string = `${uuidv4()}.${ext}`;📝 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.
| const filename:string = `${uuidv4()}.${ext}`; | |
| const filename: string = `${uuidv4()}.${ext}`; |
🤖 Prompt for AI Agents
In src/utils/imageUtils.ts around line 40, the const declaration's type
annotation has incorrect spacing; remove the extra space before the equals sign
and add a single space after the colon so the declaration uses standard spacing
for type annotations and assignment (i.e., make the colon followed by one space
and no space before =).
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.