feat(catalog): enable logo file upload in theme editor

[drernie]

What is changing

• useUploadFile in CatalogSettings.tsx — writes the dropped file to catalog/logo.<ext> in the service bucket and returns its s3:// URL.
• ThemeEditor.tsx — drops the useThirdPartyDomainForLogo gate so the drag-and-drop InputFile renders alongside the URL field.
• Logo render component is unchanged; it already signs s3:// URLs.

Why

The theme editor previously accepted only an externally hosted logo URL — every operator had to host their logo image themselves, adding onboarding friction. The display path already supported s3:// URLs; the missing piece was a write path from the browser into the service bucket.

Verification

• E2E on quilt-staging (us-east-1): 9/9 Playwright checks pass — drag-and-drop upload, S3 PUT capture, dialog close, navbar render.
• Live smoke on tf-dev-unstable: drag-and-drop and pasted-URL flows both succeed end-to-end.

Notes

• Old logo files at differing extensions (e.g. logo.png after upload of logo.jpg) are not cleaned up — orphaned files are inert once settings.json updates the URL.

Related PRs

• quiltdata/deployment#2350 — IAM grants required for this PR to work end-to-end.
• quiltdata/deployment#2356 — stack/unstable tracking PR carrying the smoke deploy.

Greptile Summary

This PR activates the logo file-upload path in the theme editor: useUploadFile now writes to catalog/logo.<ext> in the service bucket (keyed by MIME type, SVG intentionally excluded for XSS reasons), and InputFile gains a URL text field alongside the dropzone so operators can use either input method. Both the upload hook and the InputFile component are covered by new unit tests.

Confidence Score: 5/5

Safe to merge; no logic-breaking defects found, only minor P2 polish items.

All findings are P2 (missing alt attribute and unforwarded disabled prop). The core upload and URL-conversion logic is correct, MIME allowlist aligns with the IAM policy, and SVG is excluded with a clear security rationale. New tests cover the critical paths.

ThemeEditor.tsx — InputFile should accept and forward a disabled prop

Important Files Changed

Filename	Overview
catalog/app/utils/CatalogSettings.tsx	Implements useUploadFile: derives catalog/logo.<ext> key from MIME type, uploads via S3 putObject, returns S3ObjectLocation. SVG is intentionally excluded with a clear security comment. Allowlist is pinned to IAM constraints.
catalog/app/containers/Admin/Settings/ThemeEditor.tsx	Removes the useThirdPartyDomainForLogo flag and dead code; renders InputFile unconditionally. Refactors InputFile to show both a dropzone and a URL text field. disabled prop from RF.Field is silently ignored, leaving fields interactive during submission.
catalog/app/utils/CatalogSettings.spec.tsx	New test suite for useUploadFile: covers MIME-to-key mapping for all four accepted types, rejects SVG and empty MIME, and verifies filename is ignored in favour of MIME type.
catalog/app/containers/Admin/Settings/ThemeEditor.spec.tsx	New InputFile unit tests: verifies empty state, URL string rendering, object-URL lifecycle (create + revoke on unmount), and text-field onChange wiring.

Sequence Diagram

sequenceDiagram
    actor Operator
    participant InputFile
    participant RF.Field
    participant onSubmit
    participant useUploadFile
    participant S3
    participant useWriteSettings

    Operator->>InputFile: Drop image file
    InputFile->>RF.Field: onChange(File)
    Operator->>InputFile: (or) type URL
    InputFile->>RF.Field: onChange(string)

    Operator->>onSubmit: click Save
    alt value is File
        onSubmit->>useUploadFile: uploadFile(File)
        useUploadFile->>S3: putObject(catalog/logo.<ext>)
        S3-->>useUploadFile: VersionId
        useUploadFile-->>onSubmit: S3ObjectLocation
        onSubmit->>onSubmit: handleToS3Url
    else value is string URL
        onSubmit->>onSubmit: use string as-is
    end
    onSubmit->>useWriteSettings: writeSettings
    useWriteSettings->>S3: putObject(catalog/settings.json)
    S3-->>useWriteSettings: ok
    onSubmit-->>Operator: dialog closes

Loading

_{Reviews (2): Last reviewed commit: "fix(catalog): widen logoUrl type and gua..." | Re-trigger Greptile}

[claude]

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]

Codecov Report

❌ Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.83%. Comparing base (cea497e) to head (915893b).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...alog/app/containers/Admin/Settings/ThemeEditor.tsx	73.07%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4781      +/-   ##
==========================================
+ Coverage   45.69%   45.83%   +0.13%     
==========================================
  Files         829      829              
  Lines       33532    33551      +19     
  Branches     5698     5702       +4     
==========================================
+ Hits        15323    15377      +54     
+ Misses      16212    16178      -34     
+ Partials     1997     1996       -1     

Flag	Coverage Δ
api-python	93.14% <ø> (ø)
catalog	19.78% <82.50%> (+0.23%)	⬆️
lambda	96.63% <ø> (ø)
py-shared	98.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[drernie]

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

Check	Result
Catalog loads authenticated	✅
Admin Settings page loads with Theme section	✅
Drop-zone InputFile rendered (not URL text field)	✅
URL text field absent	✅
Logo preview visible after file selection	✅
Dialog closed after Save	✅
S3 PUT to catalog/logo.* captured	✅
Logo image visible in navbar	✅
settings.json persisted with s3://…/catalog/logo.png	✅

Infrastructure change required

The catalog-config inline policy on the ReadWriteQuiltV2 IAM role only allowed s3:PutObject on catalog/settings.json. The logo upload writes to catalog/logo.<ext>, which was blocked (403). The policy needs one additional statement in the stack template that provisions this role:

{
  "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.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[fiskus]

I don't understand, how does it work, if it works.
I see that ThemeEditor uploads the file, and it returns s3:// URL and writes it to CatalogSettings. But we use that URL in app/containers/NavBar/NavBar.tsx to render the image. Shouldn't that URL be https://bucket/proxy/something instead of s3://?
Ah, I see

[drernie]

Split workflow_dispatch change out into #4861 per review feedback. Rebased and force-pushed to drop commit 97c66e8 from this branch.

[fiskus]

<Logo/> is able to show blob URLs, so we can simplify the state and components render: #4863

[fiskus]

Decision to merge is up to you, but consider that after merging this PR, frontend will be in a slightly broken state: logo change would not work.

I disabled auto-merge for now.

[drernie]

Happy to wait on this merge until deployment is approved.
BUT: "frontend will be in a slightly broken state" -- this confuses me.

I agree that technically it is inconsistent, but no deployment will actually be "broken" until some stack actually pins the new image, right? Which is precisely what the deployment PR does.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

catalog/app/containers/Admin/Settings/ThemeEditor.tsx:136

• onDrop unconditionally calls onChange(files[0]). When a user drops a rejected file type (or no accepted files), files can be empty and this will set the form value to undefined (clearing a previously set URL/file). Guard on files.length (and optionally surface fileRejections for feedback) before calling onChange.

  const onDrop = React.useCallback(
    (files: FileWithPath[]) => {
      onChange(files[0])
    },

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[fiskus]

@greptileai please re-review

[sir-sigurd]

This review was generated with Claude. I don't have frontend expertise — take at your own risk.

LGTM on the narrow-scope delta: the diff between 92fa91e6 (fiskus's last review touchpoint) and 915893b5 (current head) — two commits, 68246c09 (pin the MIME allowlist) and 915893b5 (widen logoUrl type + guard empty drops).

What I checked:

• LOGO_MIME_TO_EXT keys + UnsupportedLogoTypeError align with the deployment side's CATALOG_LOGO_EXTENSIONS / CATALOG_LOGO_CONTENT_TYPES / s3:content-type IAM Condition. ContentType: file.type always set.
• Dropzone accept derived from ACCEPTED_LOGO_MIME_TYPES — single source of truth across upload, dropzone, and the IAM allowlist on the deployment side.
• onSubmit type-narrowing reads cleanly through string, '', FileWithPath, undefined.
• onDrop empty-guard prevents clearing a previously-set logo on rejected/empty drops.
• Tests cover MIME→key for the four allowed types, SVG rejection, empty-MIME rejection, filename-ignored.

Minor / take-or-leave:

• UnsupportedLogoTypeError thrown from useUploadFile falls through onSubmit's generic catch as "Couldn't save settings, see console for details." The dropzone's accept filter is keyed on the same MIME allowlist, so this exception shouldn't fire from the UI in practice — but if it ever does (test path, future caller, browser MIME-sniffing oddity), the user gets a misleading error. Could surface a specific message via instanceof UnsupportedLogoTypeError in the catch.

Not in scope: the broader PR diff and Greptile's P2 polish items.

[nl0]

@drernie no changelog entry -- intentional?