feat(catalog): enable logo file upload in theme editor#4781
feat(catalog): enable logo file upload in theme editor#4781
Conversation
Implement useUploadFile to upload the logo image to the service bucket at catalog/logo.<ext> and return an s3:// URL, then wire it up by removing the useThirdPartyDomainForLogo flag so the drag-and-drop InputFile component is shown instead of the URL text field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4781 +/- ##
===========================================
+ Coverage 45.62% 94.05% +48.42%
===========================================
Files 831 146 -685
Lines 33597 11766 -21831
Branches 5727 0 -5727
===========================================
- Hits 15328 11066 -4262
+ Misses 16264 700 -15564
+ Partials 2005 0 -2005
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Test results against quilt-staging (us-east-1)All 9/9 checks passed after running the E2E test suite with a headed Playwright browser against the live nightly stack. Results
Infrastructure change requiredThe {
"Action": "s3:PutObject",
"Resource": "arn:aws:s3:::${ServiceBucket}/catalog/logo.*",
"Effect": "Allow"
}Applied to staging manually for testing; needs to be added to the stack CDK/CFN definition before deploying to other stacks. |
Covers the full upload flow against a live stack: - Admin Settings page loads with Theme section - Theme dialog shows drop-zone instead of URL text field - File selection shows preview image - Save triggers S3 PUT to catalog/logo.<ext> - Navbar renders the uploaded logo - Cleanup removes the custom theme Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The lock file was missing yaml@2.8.3, causing `npm ci` to fail in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI uses node 20 (per engines field in package.json). The previous lock file was generated with node 25 which uses a different npm version and lock file format, causing `npm ci` to fail with "Missing: yaml@2.8.3 from lock file". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-existing formatting issues caught by CI lint check after lock file regeneration pulled updated prettier rules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The playwright dependency caused lock file churn and transitive dependency conflicts in CI. The logo upload feature was already manually verified against staging. Drop the e2e test and restore master's package.json and package-lock.json. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-existing formatting issues detected by CI's prettier 3.4.2. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The flag was a temporary guard for the URL-only logo input. Now that file upload is implemented, remove the flag, the unused URL-input branch, and the unused Form import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…itor Allow admins to paste a logo URL in addition to drag-and-drop file upload, improving flexibility for theme customization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…quilt into feat/logo-file-upload
Summary
useUploadFileinCatalogSettings.tsxto upload the logo image to the service bucket atcatalog/logo.<ext>and return ans3://URLuseThirdPartyDomainForLogoflag inThemeEditor.tsxso the drag-and-drop file input is shown instead of the URL text fieldLogocomponent already supportss3://URLs (signs them via AWS credentials before rendering), so no further changes are neededTest plan
s3://<service-bucket>/catalog/logo.<ext>and the logo appears in the navbarvitest run "Logo")🤖 Generated with Claude Code
Greptile Summary
This PR activates logo file upload in the theme editor by implementing
useUploadFileinCatalogSettings.tsx(uploading tocatalog/logo.<ext>in the service S3 bucket) and flippinguseThirdPartyDomainForLogotofalseinThemeEditor.tsxso the drag-and-dropInputFilecomponent is shown instead of the URL text field.Key points:
useUploadFileimplementation is clean and correct — reads the file buffer, infers the extension from the filename, uploads vias3.putObject, and returns ans3://URL that the existingLogocomponent can sign and render.InputFileexposes a pre-existingURL.createObjectURLmemory leak in that component: blob URLs are created on every file drop butURL.revokeObjectURLis never called — neither on value change nor on unmount. This code path was unreachable before this PR.acceptconstraint, allowing non-image files to be uploaded to S3 as a logo.useThirdPartyDomainForLogois now a dead constant (false); the ternary branch fortruein the render is unreachable and can be removed.Confidence Score: 4/5
InputFileis a real bug activated by this PR (P1) but it only affects the admin settings dialog — a low-traffic, admin-only surface — so it won't cause production reliability problems in normal usage. The missingacceptrestriction and the dead code are P2 cleanups.catalog/app/containers/Admin/Settings/ThemeEditor.tsx— theInputFilecomponent'sURL.createObjectURLcall needs a correspondingrevokeObjectURLcleanup.Important Files Changed
useUploadFile— uploads a file tocatalog/logo.<ext>in the service S3 bucket and returns ans3://URL. Implementation is straightforward; minor note that old logo files at different extensions (e.g.logo.pngvslogo.jpg) are never cleaned up on overwrite.useThirdPartyDomainForLogotofalse, activating theInputFiledropzone branch. This exposes a pre-existingURL.createObjectURLmemory leak (norevokeObjectURLcleanup) and a missingacceptconstraint on the dropzone. The flag variable itself becomes dead code.Sequence Diagram
sequenceDiagram actor User participant InputFile participant ThemeEditor participant useUploadFile participant S3 User->>InputFile: Drop image file InputFile->>ThemeEditor: onChange(File) User->>ThemeEditor: Click Save ThemeEditor->>useUploadFile: uploadFile(File) useUploadFile->>S3: putObject to catalog/logo.ext S3-->>useUploadFile: success useUploadFile-->>ThemeEditor: object URL returned ThemeEditor->>S3: writeSettings with updated logo S3-->>ThemeEditor: success ThemeEditor->>ThemeEditor: close dialogComments Outside Diff (2)
catalog/app/containers/Admin/Settings/ThemeEditor.tsx, line 132-135 (link)URL.createObjectURLcreates a persistent blob URL that is never released. Every time the user drops a new file, the old blob URL is abandoned in memory and never cleaned up. When the dialog closes and the component unmounts, the same happens. This branch was dead code before this PR (guarded by the now-removeduseThirdPartyDomainForLogo = trueflag), so this leak is effectively introduced here.A
useEffectwith cleanup should replace theuseMemo:catalog/app/containers/Admin/Settings/ThemeEditor.tsx, line 128-131 (link)The dropzone has no
acceptconstraint, so users can drop arbitrary files (PDFs, videos, executables, etc.) as a logo. These will be uploaded to S3 and saved as the logo URL. Adding anacceptlist restricts both the native file picker and the drop validation:Reviews (1): Last reviewed commit: "feat(catalog): enable logo file upload i..." | Re-trigger Greptile