-
Notifications
You must be signed in to change notification settings - Fork 7
feat(serverUtils): RN-1715: strip location data from photo uploads, enforce max. size #6514
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
base: dev
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @jaskfla, 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 PR introduces image processing capabilities to the 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
|
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 introduces image processing capabilities to strip metadata and resize images before uploading to S3, which is a great improvement for security and performance. The implementation using sharp is solid for standard raster images. However, I've identified a couple of significant side effects for specific image types that should be addressed.
7ab44e4 to
34bcefb
Compare
34bcefb to
524b7fa
Compare
524b7fa to
0827586
Compare
rohan-bes
left a comment
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.
Nice stuff! Couple of tiny nitpicks but approving 👍
| const destinationContentType = shouldConvert ? 'image/webp' : sourceContentType; | ||
|
|
||
| if (shouldConvert) { | ||
| buffer = await sharp(buffer) |
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.
| const destinationContentType = shouldConvert ? 'image/webp' : sourceContentType; | |
| if (shouldConvert) { | |
| buffer = await sharp(buffer) | |
| let destinationContentType = sourceContentType; | |
| if (shouldConvert) { | |
| destinationContentType = 'image/webp'; | |
| buffer = await sharp(buffer) |
Extremely minor but I think this reads a bit nicer to me.
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.
Would typically agree with you here, but ephemerally setting it to sourceContentType means that destinationContentType’s TS type has to be quite wide: "image/avif" | "image/gif" | "image/jpeg" | "image/png" | "image/svg+xml" | "image/tiff" | "image/webp"
Whereas immediately setting it to one or the other lets the LSP infer destinationContentType: 'image/webp' | 'image/svg+xml'
|
|
||
| if (shouldConvert) { | ||
| buffer = await sharp(buffer) | ||
| .autoOrient() |
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.
We might just wanna do a little bit of testing on this, if I recall we've had issues in the past of incorrectly oriented Facility photos. Possibly this will resolve that issue :)
Co-authored-by: Rohan Port <59544282+rohan-bes@users.noreply.github.com>
56b7cdd to
30c3615
Compare
# Conflicts: # packages/server-utils/src/s3/S3Client.ts # yarn.lock
5d5b1f1 to
d15ec8c
Compare
d15ec8c to
6ef0ec2
Compare
# Conflicts: # packages/server-utils/src/s3/S3Client.ts
RN-1715
Todo
Allow lossless PNGs if small?🥞 Stack
This is PR 1 of 2 in a stack: