refactor: consume file types, add doc string and merge image upload handling#839
refactor: consume file types, add doc string and merge image upload handling#839fingertips18 wants to merge 5 commits intomainfrom
Conversation
fingertips18
added
the
refactor
📝 WalkthroughWalkthroughReplaces image-specific upload/preview flow with a generic file upload flow: frontend renames Image→File types/services and Project.preview→previews: File[]; backend removes image DTOs/handlers/repos and adds FileUpload DTOs, FileHandler.Upload, FileRepository.Upload and a new /file/upload endpoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI
participant FileSvc as Frontend FileService
participant API as Backend /file/upload
participant Repo as FileRepository
participant UploadThing as UploadThing API
Admin->>FileSvc: request upload (file metadata)
FileSvc->>API: POST /file/upload { files, metadata }
API->>Repo: map to domain.FileUploadRequest & validate
Repo->>UploadThing: POST upload request (X-Uploadthing-Api-Key)
UploadThing-->>Repo: 202 + uploaded file data
Repo-->>API: validated domain.FileUploaded
API-->>FileSvc: 202 + dto.FileUploadedDTO
FileSvc-->>Admin: mapped file URL/fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/admin/src/pages/project/_components/card.tsx (1)
19-24: Critical: Image source set to project ID instead of image URL.Setting
src={project.id}will result in a broken image sinceproject.idis a UUID, not a URL. With the migration topreviews: File[], you likely need to access the file URL from the first preview.🐛 Proposed fix
<img - src={project.id} + src={project.previews[0]?.url ?? ''} alt={`${project.title} preview`} sizes='(min-width: 1024px) 25vw, (min-width: 640px) 50vw, 100vw' onLoad={() => setImageLoaded(true)} className='absolute object-center object-cover size-full' />You may also want to handle the case where
previewsis empty to avoid rendering a broken image or show a placeholder.apps/admin/src/pages/project/add/_components/form.tsx (1)
122-176: Critical: Uploaded file URL not associated with project.The
imageURLobtained fromFileService.uploadis never used in the project creation payload. The project is created without any preview data, leaving the uploaded file orphaned.Based on the backend's
CreateProjectRequestschema which includes apreviewsfield, the file information should be included in the project creation call.🐛 Proposed fix
const projectId = await ProjectService.create({ project: { blurhash: values.blurhash, title: values.title, subTitle: values.subTitle, description: values.description, tags: values.tags, type: values.type, link: values.link, + previews: [ + { + name: values.preview[0].name, + size: values.preview[0].size, + type: values.preview[0].type, + url: imageURL, + }, + ], }, signal: abortRef.current?.signal, });Note: The exact payload structure for
previewsdepends on what the backend expects inCreateFileRequest. Verify the required fields align with the backend DTO.
🤖 Fix all issues with AI agents
In `@apps/backend/internal/handler/v1/file.go`:
- Around line 74-76: The switch case for path == "/file/upload" calls
h.Upload(w, r) but lacks a return, causing fall-through and potential duplicate
response writes; fix by adding an immediate return after the h.Upload(w, r) call
in the handler's switch so the function exits (i.e., ensure the branch that
invokes h.Upload returns immediately to prevent executing subsequent cases).
🧹 Nitpick comments (4)
apps/backend/internal/domain/file.go (1)
189-222: Consider usingisValidMimeTypefor file type validation.The validation at lines 202-204 only checks for empty type strings. Consider using the
isValidMimeTypehelper defined earlier to ensure the MIME type format is valid before sending to UploadThing.💡 Suggested enhancement
if f.Type == "" { return errors.New("file[" + strconv.Itoa(idx) + "]: type missing") } + if err := isValidMimeType(f.Type); err != nil { + return errors.New("file[" + strconv.Itoa(idx) + "]: " + err.Error()) + }apps/backend/internal/handler/v1/file.go (2)
492-492: Unnecessarydefer r.Body.Close().The HTTP server automatically closes the request body after the handler returns. This defer is redundant and can be safely removed.
♻️ Suggested cleanup
- defer r.Body.Close() - var req dto.FileUploadRequestDTO
516-516: Rename variable fromimagetofilefor consistency.The variable name
imageis a leftover from the image-specific implementation. Since this is now a generic file upload, the variable should be namedfileoruploadedfor clarity.♻️ Suggested rename
- image, err := h.fileRepo.Upload(r.Context(), &upload) + uploaded, err := h.fileRepo.Upload(r.Context(), &upload) if err != nil { // The error in the repo is comprehensive enough // Ensure that the first letter is capitalize @@ -526,15 +526,15 @@ } file := dto.FileUploadedDTO{ - Key: image.Key, - FileName: image.FileName, - FileType: image.FileType, - FileUrl: image.FileUrl, - ContentDisposition: image.ContentDisposition, - PollingJwt: image.PollingJwt, - PollingUrl: image.PollingUrl, - CustomId: image.CustomId, - URL: image.URL, - Fields: image.Fields, + Key: uploaded.Key, + FileName: uploaded.FileName, + FileType: uploaded.FileType, + FileUrl: uploaded.FileUrl, + ContentDisposition: uploaded.ContentDisposition, + PollingJwt: uploaded.PollingJwt, + PollingUrl: uploaded.PollingUrl, + CustomId: uploaded.CustomId, + URL: uploaded.URL, + Fields: uploaded.Fields, }apps/admin/src/types/file.ts (1)
119-161: Enforce UploadThing contract instead of silently defaulting.
ensureFieldscurrently returns{}whenfieldsis missing or invalid, andcustomIdis optional. If UploadThing always returns these values, the current behavior can hide upstream contract violations. Consider throwing whenfieldsis not an object and (if the contract is strict) makingcustomIdrequired or explicitly documenting why it can be missing.♻️ Suggested tightening for
ensureFieldsfunction ensureFields(value: unknown): { [k: string]: string } { - return value && typeof value === 'object' - ? Object.fromEntries( - Object.entries(value).map(([k, v]) => [ - k, - ensureString({ value: v, name: k }), - ]), - ) - : {}; + if (!value || typeof value !== 'object') { + throw new Error("Expected property 'fields' to be an object"); + } + + return Object.fromEntries( + Object.entries(value).map(([k, v]) => [ + k, + ensureString({ value: v, name: k }), + ]), + ); }Based on learnings, UploadThing responses include all fields (including
fieldsandcustomId) and should be treated as non-null.
- Fixed broken image rendering by using the preview file URL instead of the project ID as the image source. - Strengthened file validation by checking MIME types with `isValidMimeType` before uploading. - Removed redundant `r.Body.Close()` defer from HTTP handlers. - Renamed variables from `image` to `file` to reflect generic upload behavior. - Enforced UploadThing response contract to avoid silently masking invalid payloads. - Added missing return to prevent switch-case fall-through after handling uploads.
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 (2)
apps/admin/src/pages/project/_components/card.tsx (1)
19-25: Empty string fallback forsrcmay cause unintended requests.When
project.previewsis empty,src=""causes some browsers to request the current page URL. Consider using a placeholder image URL or conditionally rendering the<img>element.💡 Suggested alternative: conditionally render
<div className='relative aspect-video'> - <img - src={project.previews[0]?.url ?? ''} - alt={`${project.title} preview`} - sizes='(min-width: 1024px) 25vw, (min-width: 640px) 50vw, 100vw' - onLoad={() => setImageLoaded(true)} - className='absolute object-center object-cover size-full' - /> + {project.previews[0]?.url && ( + <img + src={project.previews[0].url} + alt={`${project.title} preview`} + sizes='(min-width: 1024px) 25vw, (min-width: 640px) 50vw, 100vw' + onLoad={() => setImageLoaded(true)} + className='absolute object-center object-cover size-full' + /> + )}apps/backend/internal/domain/file.go (1)
124-133: Remove UserTable from the enum or create the users table—it currently has no backing migration.The UUID validation is appropriate for
projectsandeducations(both useid UUID PRIMARY KEY), and the file table enforcesparent_id UUID NOT NULLat the database level. However,UserTableis registered as a valid parent table in the enum but nouserstable exists in the database. This allows code to acceptparent_table="users"with a valid UUID, creating orphaned records with no foreign key target.Either remove
UserTablefrom theParentTableenum if it's not needed, create a users table migration, or add aCHECK (parent_table IN ('projects', 'education'))constraint on the file table to prevent invalid values.
🤖 Fix all issues with AI agents
In `@apps/admin/src/types/file.ts`:
- Around line 112-130: The JSDoc for ensureFields is inconsistent: it says the
function "returns an empty object if value is not a valid object" but
ensureFields actually throws an Error when value is not an object; update the
docstring to reflect the real behavior by removing the "returns an empty object"
claim and documenting that the function throws an Error when the input is not an
object or when any field value is not a string (keep references to ensureFields
and ensureString in the description and `@throws` tags).
In `@apps/backend/internal/handler/v1/file.go`:
- Around line 487-526: Validate the mapped request before calling
h.fileRepo.Upload in fileServiceHandler.Upload: after constructing
domain.FileUploadRequest (from dto.FileUploadRequestDTO -> domain.FileUpload),
run input validation (e.g., ensure req.Files is not empty, each
domain.FileUpload has non-empty Name, positive Size, valid
Type/ContentDisposition if required, and ACL/Metadata shapes as expected) and if
any check fails return http.StatusBadRequest with a concise, capitalized message
(use the same capitalization logic used for repo errors). Do not call
h.fileRepo.Upload when validation fails; only call it when the constructed
domain.FileUploadRequest passes these checks.
🧹 Nitpick comments (2)
apps/admin/src/types/file.ts (1)
7-18: Type name shadows globalFileinterface.The name
Fileconflicts with the built-in DOMFileinterface, which could cause confusion or require explicit imports when both types are needed in the same module. Consider renaming toAppFile,DomainFile, or similar.apps/backend/internal/domain/file.go (1)
172-239: Use snake_case JSON tags in domain models.
These structs use camelCase tags; per repo conventions, domain JSON tags should be snake_case and external API shapes should be mapped in DTOs/repo code.Based on learnings.
- Avoided empty string image sources by conditionally rendering previews or using a safe placeholder to prevent unintended browser requests. - Removed unsupported `UserTable` from the parent table enum to prevent creating orphaned file records without a backing users table. - Updated JSDoc to match actual behavior where invalid payloads throw instead of returning empty objects. - Added request-level validation for upload payloads so client errors return 400 instead of surfacing as 500s.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/internal/domain/file.go`:
- Around line 226-238: The FileUploaded struct's Fields must match the
frontend's expected Record<string,string> and be required: change Fields from
map[string]any `json:"fields,omitempty"` to map[string]string `json:"fields"`
(remove omitempty) and update any server-side code that constructs FileUploaded
(where FileUploaded is created/serialized) to ensure every field value is a
string and that Fields is always present (even if empty). Also verify the
UploadThing response wiring that populates FileUploaded to convert non-string
values to strings or omit/replace them so the API never returns non-string Field
values.
♻️ Duplicate comments (3)
apps/admin/src/types/file.ts (1)
112-131: JSDoc is now consistent with implementation.The documentation correctly describes that the function throws an error when the value is not a valid object. This addresses the previous review feedback.
apps/backend/internal/handler/v1/file.go (2)
74-76: Fixed: Return statement now present.The missing
returnstatement from the previous review has been added, preventing fall-through to subsequent switch cases.
487-519: Fixed: Request validation before repository call.The handler now validates the upload request (lines 516-519) before calling
h.fileRepo.Upload, returning 400 for invalid payloads instead of surfacing as 500. This addresses the previous review feedback.
🧹 Nitpick comments (2)
apps/admin/src/types/file.ts (1)
27-38: Unsafe cast before validation.Line 31 casts the string to
FileRoletype before validating it in line 33, which defeats type safety. The cast should occur after validation.♻️ Proposed fix
function ensureRole(value: unknown): 'image' { - const roleValue = ensureString({ + const roleString = ensureString({ value, name: 'role', - }) as (typeof FileRole)[keyof typeof FileRole]; + }); - if (!Object.values(FileRole).includes(roleValue)) { - throw new Error(`Invalid file role: ${roleValue}`); + if (!Object.values(FileRole).includes(roleString as typeof FileRole[keyof typeof FileRole])) { + throw new Error(`Invalid file role: ${roleString}`); } - return roleValue; + return roleString as 'image'; }apps/backend/internal/handler/v1/file.go (1)
521-532: Consider distinguishing client vs server errors.All repository errors return 500. If the repository can return errors for invalid external API responses (e.g., UploadThing validation failures), those would be more appropriate as 502 Bad Gateway.
♻️ Example: Distinguish upstream errors
// In repository, wrap upstream errors with a sentinel or typed error // Then in handler: if errors.Is(err, ErrUpstreamValidation) { http.Error(w, msg, http.StatusBadGateway) return }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/admin/src/types/file.ts`:
- Around line 103-113: The FileUpload type and mapFileUpload mapper expect
snake_case and inconsistent names; update the FileUpload type to use camelCase
property names (fileName, fileType, fileUrl, pollingJwt, pollingUrl, url)
replacing fileURL, pollingJWT, pollingURL, and URL, and then update
mapFileUpload to read those camelCase response fields (fileName, fileType,
fileUrl, pollingJwt, pollingUrl, url) instead of snake_case keys; ensure fields:
Record<string,string> and customId remain unchanged and adjust any property
assignments inside mapFileUpload (references to FileUpload and mapFileUpload)
accordingly so the shapes align with the backend's FileUploaded JSON tags.
|
@coderabbitai with the suggestions you provided, we have test workflow issues. Please resolve. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 1736 Let me get the detailed logs from the failed workflow run. 🏁 Script executed: Length of output: 11103 Let me examine the failing test file to understand the test structure and provide the fix. 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 18705 I've identified the issue! The tests are failing because the new validation logic in Here's the fix for the two failing test cases: File: 1. Fix "empty files array" test (around line 204-221): "empty files array": {
given: Given{
method: http.MethodPost,
body: func() string {
req := dto.FileUploadRequestDTO{
Files: []dto.FileUploadDTO{},
}
b, _ := json.Marshal(req)
return string(b)
}(),
// Remove mockRepo function - validation fails before repo is called
},
expected: Expected{
code: http.StatusBadRequest,
body: "Files missing\n",
},
},2. Fix "zero size file" test (around line 425-447): "zero size file": {
given: Given{
method: http.MethodPost,
body: func() string {
req := dto.FileUploadRequestDTO{
Files: []dto.FileUploadDTO{
{
Name: "empty.jpg",
Size: 0,
Type: "image/jpeg",
},
},
}
b, _ := json.Marshal(req)
return string(b)
}(),
// Remove mockRepo function - validation fails before repo is called
},
expected: Expected{
code: http.StatusBadRequest,
body: "File[0]: size invalid\n",
},
},Summary of changes:
These changes align the tests with the correct behavior: invalid inputs should be rejected with 400 status codes, not accepted. |
|
@coderabbitai help for the code changes needed in order to consume the file in the form submission. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7652 🏁 Script executed: Length of output: 3492 🏁 Script executed: Length of output: 5099 🏁 Script executed: Length of output: 2276 🏁 Script executed: Length of output: 1598 🏁 Script executed: Length of output: 1930 🏁 Script executed: Length of output: 1511 🏁 Script executed: Length of output: 2127 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 142 🏁 Script executed: Length of output: 1283 🏁 Script executed: Length of output: 502 🏁 Script executed: Length of output: 1881
Code Changes NeededFile: Update the const onSubmit = async (values: Schema) => {
let imageURL: string | null;
try {
setImageLoading(true);
const preview = values.preview[0];
const url = await FileService.upload({
file: preview,
signal: abortRef.current?.signal,
});
if (!url) {
throw new Error('Image URL undefined');
}
imageURL = url;
toast({
level: 'success',
title: 'Image upload complete 🎉',
description: `${preview.name} uploaded successfully!`,
});
} catch {
imageURL = null;
toast({
level: 'error',
title: 'Upload failed',
description: 'We couldn't upload your image. Please try again.',
});
} finally {
setImageLoading(false);
}
if (!imageURL) {
return;
}
// Proceed with project creation, including file metadata
try {
setProjectLoading(true);
const preview = values.preview[0];
const projectId = await ProjectService.create({
project: {
previews: [
{
role: 'image',
name: preview.name,
url: imageURL,
type: preview.type,
size: preview.size,
// parentTable and parentID are set by backend
parentTable: '',
parentID: '',
},
],
blurhash: values.blurhash,
title: values.title,
subTitle: values.subTitle,
description: values.description,
tags: values.tags,
type: values.type,
link: values.link,
},
signal: abortRef.current?.signal,
});
if (!projectId) {
throw new Error('Project ID undefined');
}
toast({
level: 'success',
title: 'Project upload complete 🎉',
description: `${values.title} uploaded successfully!`,
});
setSubmitted(true);
form.reset();
} catch {
toast({
level: 'error',
title: 'Upload failed',
description: 'We couldn't upload your project. Please try again.',
});
} finally {
setProjectLoading(false);
}
};ExplanationThe key changes are:
The |
1 participant
Relates to #769
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.