-
Notifications
You must be signed in to change notification settings - Fork 0
fix: Enable image viewing in volunteer dashboard using presigned URLs #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add generateReadPresignedUrl() function to s3Client for reading images - Update pending reports API to generate presigned GET URLs for images - Update history API to generate presigned GET URLs for images - Change images query from take:1 to true for complete image data Previously, volunteers couldn't view report images because the stored S3 URLs required public bucket access. Now, we generate temporary presigned URLs (valid 1 hour) for all image requests, allowing secure access to images in private S3 buckets.
Summary of ChangesHello @varshapanda, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue where volunteers were unable to view report images due to direct access attempts to a private S3 bucket. By introducing presigned URLs, the system now securely generates temporary access links for images, allowing them to be displayed correctly in the volunteer dashboard. This enhancement significantly improves the user experience by providing complete visual information for reports while maintaining the security of the stored data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully addresses the issue of volunteers being unable to view images by implementing presigned URLs for S3 objects. The changes are well-structured, introducing a new utility function for generating read URLs and integrating it into the pending and history API routes for volunteers. My review focuses on improving robustness, data consistency, and efficiency. I've identified a couple of high-severity issues. In src/app/api/reports/create/route.ts, the report and image creation are not performed in a transaction, which could lead to data inconsistency. I've suggested using a Prisma nested write to make this operation atomic and more efficient. In src/lib/s3Client.ts, the use of a non-null assertion (!) for an environment variable could cause runtime crashes if the variable is not set. I've recommended adding explicit validation. Additionally, I've noted a medium-severity issue in the volunteer API routes where presigned URLs are being generated redundantly for the same image. Overall, this is a solid contribution that fixes a key bug. Addressing the feedback will make the implementation more resilient and correct.
| const command = new GetObjectCommand({ | ||
| Bucket: process.env.AWS_BUCKET_NAME!, | ||
| Key: key, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the non-null assertion operator (!) on environment variables like process.env.AWS_BUCKET_NAME! can introduce runtime errors if the variable is not defined. It's safer to validate the environment variable's existence and handle its absence gracefully by throwing an error within the try...catch block.
const bucketName = process.env.AWS_BUCKET_NAME;
if (!bucketName) {
throw new Error("AWS_BUCKET_NAME environment variable is not set.");
}
const command = new GetObjectCommand({
Bucket: bucketName,
Key: key,
});| const reportsWithSignedUrls = await Promise.all( | ||
| reports.map(async (report) => ({ | ||
| ...report, | ||
| imageUrl: await generateReadPresignedUrl(report.imageUrl), | ||
| images: await Promise.all( | ||
| report.images.map(async (img) => ({ | ||
| ...img, | ||
| url: await generateReadPresignedUrl(img.url), | ||
| })) | ||
| ), | ||
| })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block generates a presigned URL for report.imageUrl and then again for each image in report.images. Since report.imageUrl is also present in the report.images array (based on the creation logic), you are generating a presigned URL for the same image twice for every report. This is redundant. Consider refactoring to avoid the duplicate signing operation, for example by signing all images in the images array and then finding the corresponding new URL for imageUrl from that array.
| const reportsWithSignedUrls = await Promise.all( | ||
| reports.map(async (report) => ({ | ||
| ...report, | ||
| imageUrl: await generateReadPresignedUrl(report.imageUrl), | ||
| images: await Promise.all( | ||
| report.images.map(async (img) => ({ | ||
| ...img, | ||
| url: await generateReadPresignedUrl(img.url), | ||
| })) | ||
| ), | ||
| })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block generates a presigned URL for report.imageUrl and then again for each image in report.images. Since report.imageUrl is also present in the report.images array (based on the creation logic), you are generating a presigned URL for the same image twice for every report. This is redundant. Consider refactoring to avoid the duplicate signing operation, for example by signing all images in the images array and then finding the corresponding new URL for imageUrl from that array.
User description
Problem
Volunteers were unable to view report images in the verify and history pages. Images failed to load with "Failed to load image" errors because the stored S3 URLs pointed to a private bucket that blocked direct browser access.
Solution
Implemented presigned URL generation for image reading/viewing:
generateReadPresignedUrl()function to generate temporary signed URLs for GET requestsChanges Made
Modified Files
src/lib/s3Client.tsGetObjectCommandimport from AWS SDKgenerateReadPresignedUrl()function to create temporary signed URLs (valid 1 hour)src/app/api/volunteer/reports/pending/route.tsgenerateReadPresignedUrlfunctionimages: { take: 1 }toimages: trueimageUrlandimagesarraysrc/app/api/volunteer/history/route.tsHow to Test
Prerequisites
.envTest Steps
1. Test Pending Reports (Verify Page)
Expected Results:
To Verify in Console:
Full API Response:X-Amz-Algorithm,X-Amz-Credential,X-Amz-Signaturequery parameters (presigned URL markers)Image failed to loaderrors should appear2. Test History Page
# Navigate to volunteer history page http://localhost:3000/dashboard/volunteer/historyExpected Results:
Expected Results:
4. Browser Network Tab Verification
Expected Results:
200 OKSecurity Considerations
Related Issues
Fixes #43
PR Type
Bug fix, Enhancement
Description
Enable image viewing in volunteer dashboard using presigned URLs
Add
generateReadPresignedUrl()function for secure S3 image accessUpdate pending and history APIs to generate presigned URLs for all images
Fetch complete image data instead of limiting to first image
Improve report creation to explicitly create image records in database
Diagram Walkthrough
File Walkthrough
s3Client.ts
Add presigned URL generation for image readingsrc/lib/s3Client.ts
GetObjectCommandimport from AWS SDKgenerateReadPresignedUrl()function for generatingtemporary signed URLs
request
route.ts
Improve report creation with explicit image recordssrc/app/api/reports/create/route.ts
creation
route.ts
Generate presigned URLs for pending report imagessrc/app/api/volunteer/reports/pending/route.ts
generateReadPresignedUrlfunction from s3Clienttake: 1totrueto fetch all imagesimageUrlandimagesarrayreturning
route.ts
Generate presigned URLs for historical report imagessrc/app/api/volunteer/history/route.ts
generateReadPresignedUrlfunction from s3Clienttake: 1totrueto fetch all imagesimageUrlandimagesarrayreturning