Skip to content

refactor(catalog): unify InputFile preview through Logo#4863

Merged
fiskus merged 2 commits into
masterfrom
refactor/logo-preview-single-component
May 7, 2026
Merged

refactor(catalog): unify InputFile preview through Logo#4863
fiskus merged 2 commits into
masterfrom
refactor/logo-preview-single-component

Conversation

@fiskus
Copy link
Copy Markdown
Member

@fiskus fiskus commented Apr 28, 2026

Summary

  • Collapse the two preview render branches in InputFile into a single one driven by previewSrc: string | null.
  • Route both the typed URL string and the File-derived blob: URL through Logo, which already handles s3://, http(s), and blob: uniformly.
  • Drop the unused preview style class and rename previewUrl state to blobUrl (it now only ever holds blob URLs).

Why

Before this change there were two preview JSX branches (<Logo> for URL strings, <img className={classes.preview}> for blob URLs) gated by overlapping conditions (isUrl, previewUrl, value). They were mutually exclusive in practice but expressed in a way that made it hard to see that at a glance. Single component, single nullable src, no ambiguity.

Visual change

The File preview now uses Logo's sizing (height-only, intrinsic width) instead of a fixed 50×50 box, so non-square logos preserve aspect ratio in the dropzone preview. Same as the URL-preview path, so the two cases now look consistent.

Out of scope

  • The meta/errors regression vs master (URL validation message no longer renders) — separate fix.
  • The keystroke-triggered network requests for partially-typed URLs — separate fix; the obvious place to add the gate is now the previewSrc memo.

Test plan

  • npm run test:only -- ThemeEditor.spec CatalogSettings.spec — 7/7 pass
  • npm run lint — clean
  • Manual: drop a non-square image, confirm preview shows aspect ratio
  • Manual: paste an s3:// URL, confirm preview signs and renders
  • Manual: paste an https:// URL to a real image, confirm preview renders

Greptile Summary

This PR simplifies the logo preview rendering in InputFile by collapsing two mutually-exclusive JSX branches (<Logo> for URL strings, <img> for blob URLs) into a single <Logo src={previewSrc}> path driven by a nullable previewSrc memo. The rename of previewUrlblobUrl sharpens intent, and the now-dead preview CSS class is removed.

  • Unified preview branch: previewSrc is derived from value (string) or blobUrl (File-derived blob URL); a single previewSrc ? <Logo> : <Placeholder> replaces three separate conditionals.
  • Aspect-ratio fix: Logo's custom CSS class only constrains height, so dropped non-square images now render with intrinsic width instead of being forced into the old 50×50 box.
  • Test update: the blob-preview assertion now queries the mocked Logo via data-testid="logo" / data-src, matching the new render path.

Confidence Score: 5/5

Safe to merge — the change is a purely structural cleanup of mutually-exclusive conditional branches with no new logic surface and correct blob-URL lifecycle handling.

The refactor replaces three overlapping boolean conditions with a single nullable previewSrc memo. The blob-URL creation and revocation lifecycle is preserved unchanged. The value || null coercion correctly maps the empty-string case to the placeholder. No new network requests, state shapes, or side-effects are introduced beyond what existed before.

No files require special attention.

Important Files Changed

Filename Overview
catalog/app/containers/Admin/Settings/ThemeEditor.tsx Collapses two conditional preview branches into a single Logo-driven path via a previewSrc memo; renames previewUrl state to blobUrl; removes unused preview CSS class.
catalog/app/containers/Admin/Settings/ThemeEditor.spec.tsx Updates the File-preview assertion to query the mocked Logo via data-testid instead of a raw img element; adds a Logo mock that exposes data-src for inspection.
catalog/CHANGELOG.md Appends PR #4863 reference to the existing drag-and-drop logo upload entry.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[InputFile renders] --> B{typeof value}
    B -->|string| C["previewSrc = value || null"]
    B -->|File| D[useEffect creates blob URL]
    D --> E["blobUrl state set"]
    E --> F["previewSrc = blobUrl"]
    C --> G{previewSrc truthy?}
    F --> G
    G -->|yes| H["Logo src={previewSrc}"]
    G -->|no| I["Placeholder (hide_image icon)"]
    H --> J{src is s3:// URL?}
    J -->|yes| K[Sign URL via AWS Signer]
    J -->|no| L[Use src as-is]
    K --> M["img rendered"]
    L --> M
Loading

Reviews (2): Last reviewed commit: "chore: add PR link for #4863 to theme ed..." | Re-trigger Greptile

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.51%. Comparing base (f9290bc) to head (abbcfde).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4863   +/-   ##
=======================================
  Coverage   46.51%   46.51%           
=======================================
  Files         832      832           
  Lines       34116    34116           
  Branches     5828     5826    -2     
=======================================
  Hits        15868    15868           
  Misses      16243    16243           
  Partials     2005     2005           
Flag Coverage Δ
api-python 93.14% <ø> (ø)
catalog 21.48% <100.00%> (ø)
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.

@fiskus fiskus marked this pull request as ready for review April 28, 2026 16:42
Base automatically changed from feat/logo-file-upload to master April 29, 2026 19:10
@drernie
Copy link
Copy Markdown
Member

drernie commented Apr 30, 2026

Is this still pending?

fiskus added 2 commits May 6, 2026 18:23
Collapse the two preview render branches in ThemeEditor's InputFile
into a single one driven by a nullable preview src. Both the typed
URL string and the File-derived blob URL now flow through Logo, which
already handles s3://, http(s), and blob: srcs uniformly. Drops the
unused preview style class and updates the spec to assert against
the (mocked) Logo for the File case.
@fiskus fiskus force-pushed the refactor/logo-preview-single-component branch from 231ce78 to abbcfde Compare May 6, 2026 16:35
@fiskus
Copy link
Copy Markdown
Member Author

fiskus commented May 6, 2026

@greptileai please re-review

@fiskus fiskus requested a review from drernie May 6, 2026 16:42
@fiskus fiskus added this pull request to the merge queue May 7, 2026
Merged via the queue into master with commit dbfbaa0 May 7, 2026
46 checks passed
@fiskus fiskus deleted the refactor/logo-preview-single-component branch May 7, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants