feat: User pfp + s3 file uploads#75
Merged
anish-sahoo merged 48 commits intomainfrom Jan 29, 2026
Merged
Conversation
…o s3_bucket_name to match doppler
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
anish-sahoo
commented
Jan 27, 2026
Contributor
📝 WalkthroughWalkthroughAdds end-to-end S3-backed image uploads: AWS SDK and presign clients, DB migrations and Image models, repository/service/controller/routes for upload/confirm/get, frontend compression/upload utilities and hooks, tests with autogenerated mocks, CI updates, test scripts, and S3 testing documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/App
participant API as API Server
participant DB as Database
participant S3 as S3 Service
Client->>API: POST /api/v1/files/upload (fileKey, sizes)
API->>DB: CreatePendingImages(imageID, fileKey, sizes)
DB-->>API: pending records
API->>S3: PresignPutObject (per size)
S3-->>API: presigned PUT URLs
API-->>Client: imageId, uploadUrls, expiresAt
Client->>S3: PUT to presigned URLs (parallel)
S3-->>Client: 200 OK
Client->>API: POST /api/v1/files/confirm (imageId, optional size)
API->>S3: HeadObject for relevant sizes
S3-->>API: object metadata / not found
API->>DB: Update images status -> confirmed/failed
DB-->>API: updated records
API-->>Client: confirm response (status, confirmed count)
Client->>API: GET /api/v1/files/{imageId}/{size}
API->>DB: FindByIDAndSize(imageID, size)
DB-->>API: confirmed record
API->>S3: PresignGetObject
S3-->>API: presigned GET URL
API-->>Client: GET URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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
Comment |
Contributor
in-mai-space
left a comment
in-mai-space
left a comment
There was a problem hiding this comment.
CRAZY WORK GUYS, THIS IS PROBABLY THE BIGGEST PR IN GENERATE HISTORY 🔥🔥🔥
very tiny nits otherwise the overall logic looks good to me
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/api/files/useGetImageAllSizes.ts`:
- Around line 40-42: The errors array filter currently only excludes nulls and
may let undefined through; update the filtering in useGetImageAllSizes to ensure
q.error values of both null and undefined are removed (e.g., change the
predicate on queries.map(q => q.error as GetImageError | null) to filter by e =>
e != null or explicitly e !== null && e !== undefined) so the resulting errors:
array is strictly GetImageError values; reference the queries array, q.error,
the errors variable and the GetImageError type when making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/dev.Dockerfile (1)
1-25: Optional: Consider adding HEALTHCHECK and non-root user for hardened dev images.Static analysis flags two low-severity items:
- No
HEALTHCHECKinstruction.- No non-root user created.
For a dev container with hot-reload, these are not critical. However, if you want consistency with production practices or run this in orchestrated environments, consider adding them.
Example additions
# Add a non-root user (before WORKDIR) RUN adduser -D appuser USER appuser # Add a healthcheck (after EXPOSE) HEALTHCHECK --interval=30s --timeout=3s --start-period=5s \ CMD curl -f http://localhost:8000/health || exit 1Note: Non-root user may require adjusting volume mount permissions for local development.
.github/workflows/frontend.yml (1)
46-47: Remove thebun add -D prettierstep from the workflow.Prettier is already listed in
frontend/package.json(line 70, version ^3.8.1). This installation step is redundant and mutates the workspace unnecessarily. Relying onbun installalone is sufficient.Proposed change
- - name: Install Prettier - run: cd frontend && bun add -D prettier
🤖 Fix all issues with AI agents
In `@backend/internal/repository/images.go`:
- Around line 106-114: The MarkFailed function should detect when the UPDATE
matched zero rows and return errs.ErrNotFound instead of nil; modify
imageRepository.MarkFailed to capture the result of Exec(ctx), check if err !=
nil and return it, then call res.RowsAffected() and if it returns 0 return
errs.ErrNotFound, otherwise return nil—follow the same pattern used in
DeleteByID and reference models.Image, models.UploadStatusFailed, and
errs.ErrNotFound when implementing this change.
In `@backend/internal/services/files.go`:
- Around line 168-201: The loop in ConfirmUpload (using f.findPendingImages,
f.s3Client.HeadObject, imageRepo.MarkFailed, imageRepo.ConfirmUpload) currently
swallows per-size failures and only returns confirmedCount; update it to collect
failed size details instead of silently skipping: maintain a slice of failed
sizes (and optional error reasons), increment it when HeadObject or
ConfirmUpload fails (and continue to MarkFailed), and return those failures in
the response (extend models.ConfirmUploadResponse with FailedSizes/FailedCount
and include those fields) or, if desired, return a consolidated error when any
sizes fail; ensure callers receive which image sizes failed and why rather than
only confirmedCount.
In `@backend/internal/utilities/validator.go`:
- Around line 30-39: The allowedImageSizes slice duplicates model constants and
can diverge; replace the hardcoded allowedImageSizes with references to the
model constants (e.g., models.ImageSizeSmall, models.ImageSizeMedium,
models.ImageSizeLarge) and update isAllowedImageSize to iterate over that
derived slice or check directly against those constants so the validator always
uses the canonical values from the models package (update any imports as needed
and keep function name isAllowedImageSize unchanged).
In `@docs/TESTING_S3_UPLOADS.md`:
- Around line 137-201: The fenced Go example contains hard tabs; replace all
tabs with spaces inside the code block to satisfy markdownlint MD010. Edit the
snippet for TestFileController_CreateUploadURLs (including the app setup,
mockFileService setup via mocks.NewMockFileService, controller :=
controllers.NewFileController, the mock On("CreateUploadURLs", ...), request
body creation using models.UploadURLRequest, and the call to
controller.CreateUploadURLs) and convert every tab character to spaces so the
code block uses consistent space indentation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/__tests__/utilities/images.test.ts`:
- Around line 28-31: Capture the original global fetch before overwriting it
(e.g., store it in a variable like originalFetch) and restore it in an afterAll
cleanup so the test suite doesn't leak globalThis.fetch; update the test module
where mockFetch is assigned (the mockFetch and globalThis.fetch assignment) to
save originalFetch first and add an afterAll block that sets globalThis.fetch
back to originalFetch (or undefined if it was undefined).
In `@frontend/app/`(app)/test-upload.tsx:
- Around line 26-34: If profileImageId or galleryImageId can be null, avoid
passing null into hooks: update the calls to useGetImage and useGetImageAllSizes
so they receive an empty array when the id is null (e.g.
useGetImage(profileImageId ? [profileImageId] : [], "small") and
useGetImageAllSizes(galleryImageId ? [galleryImageId] : [])), or alternatively
pass a disabled flag to the hook (e.g. an options { enabled: !!profileImageId })
so the hooks (useGetImage, useGetImageAllSizes) never run with a null ID; adjust
any downstream usages of profileImages[0] and galleryImagesAllSizes[0] to handle
undefined safely (profileImageData/galleryImageAllSizes).
In `@frontend/services/imageService.ts`:
- Around line 103-140: The uploadImage function should validate that the sizes
array is non-empty before doing work: check the UploadImageRequest.sizes (or
local sizes variable) and if sizes.length === 0 throw a clear client-side Error
(e.g., "sizes must contain at least one size") and return early; do this before
calling compressImage, getUploadURLs, or any uploads so compressImage,
getUploadURLs, and confirmUpload are not invoked with an empty sizes list.
In `@frontend/utilities/images.ts`:
- Around line 83-145: In compressWithIterativeQuality the scale loop compounds
because it uses result.width/result.height from previous iterations and
qualitySteps may exceed initialQuality; fix by capturing baselineWidth and
baselineHeight once before the scaleSteps loop and compute scaledWidth/Height
from those baselines (e.g., Math.round(baselineWidth * scale)), and filter
qualitySteps to only include values <= initialQuality before iterating; update
references in the loops that call ImageManipulator.manipulateAsync and
getFileSize and keep the final ImageCompressionError with sizeName unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/internal/repository/images.go`:
- Around line 48-58: The Returning("*") call on the update built with
r.db.NewUpdate().Model(image)...Exec(ctx) is misleading because Exec() does not
populate the model from RETURNING; remove the Returning("*") invocation from the
update chain (the code that sets status to models.UploadStatusConfirmed and
confirmed_at) and leave the existing SELECT that follows to fetch the updated
record so behavior and intent are clear; locate the update built with
r.db.NewUpdate() / Set("status = ?", models.UploadStatusConfirmed) /
Set("confirmed_at = ?", now) and delete the Returning("*") call.
In `@frontend/api/files/useGetImage.ts`:
- Around line 24-43: The aggregate status flags in useGetImage are computed
manually from the raw queries array and incorrectly include skipped queries
(created with skipToken), so refactor useGetImage to pass a combine callback to
useQueries that filters out skipped queries by testing fetchStatus !== 'idle'
(or similar) before computing aggregated values; inside the combine callback
compute data (map q.data and filter to GetImageURLResponse), isLoading
(active.some(q => q.isLoading)), isSettled (active.length > 0 && active.every(q
=> q.isSuccess || q.isError)), isSuccess (active.length > 0 && active.every(q =>
q.isSuccess)), and collect errors (map q.error and filter to GetImageError),
then return useQueries({ queries: imageIds.map(... queryKey/queryFn using
getImageURL and skipToken), combine }) so all callers of useGetImage get correct
aggregated state.
In `@frontend/api/files/useGetImageAllSizes.ts`:
- Around line 19-38: In useGetImageAllSizes, exclude skipped queries (those
where queryFn used skipToken / imageId falsy) before computing aggregate flags:
derive activeQueries = queries.filter(q => q.fetchStatus !== "idle" || q.queryFn
!== skipToken) or better filter by presence of queryKey or q.data !== undefined;
then compute isLoading, isSettled, and isSuccess from activeQueries (isLoading =
activeQueries.some(q => q.isLoading), isSettled = activeQueries.every(q =>
q.isSuccess || q.isError), isSuccess = activeQueries.every(q => q.isSuccess));
keep the existing data mapping from queries.map(q => q.data).filter(...)
unchanged and reference queries, getImageAllSizes, and skipToken when making the
change.
In `@frontend/api/files/useImageUpload.ts`:
- Around line 14-19: The JSDoc for the useImageUpload hook is misleading: update
the comment that currently states "Optional mutation options (cropping, etc.)"
to accurately describe the parameter as React Query mutation options
(UseMutationOptions) used to control the mutation behavior (onSuccess, onError,
retry, etc.); edit the JSDoc for the useImageUpload function/parameter to
reference UseMutationOptions and remove any mention of cropping so readers know
these are React Query mutation configuration options rather than image
processing settings.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/utilities/images.test.ts`:
- Around line 450-472: The test currently asserts parallelism by measuring
wall‑clock time for compressImage, which is flaky; instead modify the test in
frontend/__tests__/utilities/images.test.ts to use a deterministic barrier:
instrument the mocked ImageManipulator.manipulateAsync to push a Promise
resolver into an array and mark when each call starts (e.g., push to a
started[]), then await a “allStarted” Promise that resolves when started.length
=== expected (3) before allowing each manipulateAsync mock to resolve, and
finally assert that all manipulateAsync calls were initiated (started) before
any were resolved; keep references to compressImage and
ImageManipulator.manipulateAsync so the test logic replaces the timing check
with the barrier-based assertion.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@backend/internal/repository/images.go`:
- Around line 105-123: The MarkFailed method on imageRepository currently
updates any image by image_id and size; narrow the update to only pending images
by adding a status filter in the WHERE clause (e.g., add "AND status = ?" with
models.UploadStatusPending) so only images in pending state can be transitioned
to models.UploadStatusFailed; keep the existing rowsAffected==0 ->
errs.ErrNotFound behavior to surface when no pending row was updated.
In `@frontend/__tests__/utilities/images.test.ts`:
- Around line 165-169: The test uses a loose type annotation { size: any } when
mapping results; replace it by importing and using the actual CompressedVariant
type from the images module and annotate the map parameter as (r:
CompressedVariant) => r.size (or the correct exported type name if different) so
TypeScript can catch type errors; apply the same change to the other occurrences
at the map calls around the test (lines referenced use results.map and the other
two map usages).
- Line 471: The linter flags the implicit-return arrow in the call to
resolvers.forEach((r) => r()); — change it to use block syntax so the intent
(calling void resolvers) is explicit; update the forEach callback for the
resolvers array (the resolver variable r) to a block-bodied arrow (e.g., (r) =>
{ r(); }) to silence the warning.
In `@frontend/api/files/useGetImage.ts`:
- Around line 34-53: The aggregate query flags are computed from an `active` set
that currently filters with `r.fetchStatus !== "idle"`, which drops settled
queries; update the filter in useGetImage (the `active` variable derived from
`results`) to include settled queries by using `r.isSuccess || r.isError` (or
equivalently `r.isFetching || r.isSuccess || r.isError`) so that the computed
`isSettled` and `isSuccess` (which reference `active`) reflect completed query
states correctly.
In `@frontend/api/files/useGetImageAllSizes.ts`:
- Around line 29-48: The active query detection filters use fetchStatus and
excludes settled queries; change the filter to use the query's status so settled
results remain in active (e.g., replace results.filter((r) => r.fetchStatus !==
"idle") with results.filter((r) => r.status !== "idle")) so variables active,
isSettled, and isSuccess (in useGetImageAllSizes.ts) correctly reflect completed
queries.
In `@frontend/api/files/useImageUpload.ts`:
- Around line 67-71: The mutationFn wrapper is redundant in useImageUpload;
replace the arrow wrapper used in the useMutation call (currently mutationFn:
(data) => uploadImage(data)) with the uploadImage function reference directly so
mutationFn points to uploadImage; update the useMutation invocation in
useImageUpload accordingly to simplify and avoid the unnecessary lambda.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Jan 30, 2026
Merged
3 tasks
3 tasks
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
3 participants
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.
Description
backend/Makefileto support using doppler's backend > dev_personal keys.Tip
TESTING_S3_UPLOADS.mdinstructions to get setup.Upload flow (explained)
small,medium, orlargesize (there is also a function to upload all three sizes, which is the default except for the [planned] profile picture submissions, which will only uploadsmall.frontend/constants/images.tsHow has this been tested?
Backend - added
mockerymocking library and mocks for s3 sdk, and tested with a bunch of tests, tested file uploads end to end withscripts/test_uploads.shFrontend - added jest tests for both the services and utilities folder. We also added a button in the main login page that you can test the flow out yourself.
Checklist
Categorized change list
Author contribution summary (approx. lines)
Sequence (high-level) — file upload flow
Client -> compress variants -> POST /api/v0/files/upload (createUploadURLs) -> Backend creates pending DB rows + returns presigned PUT URLs -> Client PUT to S3 presigned URLs -> Client POST /api/v0/files/confirm -> Backend HeadObject(s) -> mark Confirmed (or Failed) in DB -> Client GET /api/v0/files/{imageId}/{size} (or /all) -> Backend returns presigned GET URL