Skip to content

Crop avatars with Imgproxy #2074

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

Merged
merged 8 commits into from
Apr 15, 2025
Merged

Conversation

Soxasora
Copy link
Member

@Soxasora Soxasora commented Apr 6, 2025

Description

Fixes #1778
Client-side cropping wasn't giving the best of results, this PR introduces cropping via Imgproxy's Crop feature giving better cropping results.

Screenshots

This keyboard pic zoomed at 250% from 200x200 makes a huge case for server-side processing

space-1.jpg
left: Imgproxy - right: React Avatar Editor

Additional Context

It required minimal changes, now React Avatar Editor is only used to get cropping values. With them we can now upload the untouched image to S3 safely and create an Imgproxy path to crop it.
We then upload the cropped image back to S3. This is a standard approach that imo could be better than serving imgproxy urls on avatars since if imgproxy goes down, every avatar would go down. It also fits nicely with already existing avatars and db schema.

Checklist

Are your changes backwards compatible? Please answer below:
Yes, it uploads the Imgproxy cropped photo back to S3 to also retain the existing setPhoto

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
7, consistent good results

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
n/a

Did you introduce any new environment variables? If so, call them out explicitly here:
n/a

@Soxasora Soxasora force-pushed the crop_avatar_imgproxy branch from b9096a6 to 71dccb1 Compare April 6, 2025 22:00
@Soxasora Soxasora added enhancement improvements to existing features multimedia labels Apr 7, 2025
@Soxasora Soxasora marked this pull request as ready for review April 10, 2025 19:04
@huumn
Copy link
Member

huumn commented Apr 15, 2025

I'd guess this works fine but I'm having trouble QAing in the normal dev setup. It returns a url with imgproxy as the hostname.

Do I need to modify my env or something?

@Soxasora
Copy link
Member Author

Soxasora commented Apr 15, 2025

const IMGPROXY_URL = process.env.IMGPROXY_URL_DOCKER || process.env.NEXT_PUBLIC_IMGPROXY_URL

IMGPROXY_URL is based on IMGPROXY_URL_DOCKER in dev, in prod is instead based on NEXT_PUBLIC_IMGPROXY_URL

Since I tested it with HTTPS I just blanked the docker env var and used the public Imgproxy url. Basically docker containers can't use localhost with each other, they have to use hostnames instead.

But this is actually a thinking error because the upload happens client-side so it might be appropriate to return a NEXT_PUBLIC_IMGPROXY_URL based URL. Pushing it now, sorry and thanks!

@huumn
Copy link
Member

huumn commented Apr 15, 2025

I get a CORs issue now with the default sndev setup.

Access to fetch at 'http://localhost:3001/S_ASkXittsLJXhFYXpH__fGComLBm43_JOp2mHGVNyg/crop:512:512/gravity:fp:1:0.8333333333333333/rs:fill:200:200/aHR0cDovL3MzOjQ1NjYvdXBsb2Fkcy8xOTk1NA' from origin 'http://localhost:3000' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

@huumn
Copy link
Member

huumn commented Apr 15, 2025

Looks like we need to set IMGPROXY_ALLOW_ORIGIN. It'll also need to be set in prod. I'm making these changes now.

@huumn
Copy link
Member

huumn commented Apr 15, 2025

Looks like we've had a bug with avatars in sndev for awhile - avatars were using MEDIA_URL from lib/constants which defaulted to the docker hostname. That's not your fault.

For future stuff, it's important our changes work in our common development environment.

@huumn huumn merged commit b6f6cc8 into stackernews:master Apr 15, 2025
6 checks passed
@Soxasora
Copy link
Member Author

I honestly disregarded any CORS issue in dev because by running it on HTTPS I kinda expected to receive all kinds of errors. My bad for not looking into it.

For future stuff, it's important our changes work in our common development environment.

Agree! Will be more careful towards dev env experience

@huumn
Copy link
Member

huumn commented Apr 15, 2025

No worries! It's hard to appreciate these things - and just easy for me to which is I mentioned it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features multimedia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Profile photo pixelated
2 participants