fix(catalog): resilient Logo render for malformed S3 URLs#4881
Conversation
`CustomLogo` called `parseS3Url` during render, so any string that starts with `s3://` but has no hostname (e.g. `s3://`, `s3:///foo`) threw `bucket not specified` out of the render path and unmounted the tree — crashing every page that mounts NavBar/footer when such a URL was persisted to catalog settings. Add a non-throwing `tryParseS3Url` helper and use it in `<CustomLogo>`. On parse failure, fall back to the default Quilt logo and report the bad src to Sentry once via `useEffect`. `parseS3Url` itself keeps its throwing contract for other callers.
Before the page-level crash was fixed, typing `s3://` into the ThemeEditor "Logo URL" field would unmount the editor. Now that `<Logo>` no longer throws, surface the validation feedback in the form itself: - Add `validators.s3Url` to the `composeOr` chain so `s3://bucket/key` is accepted alongside HTTP(S) URLs and uploaded files. - Show validation errors as soon as the field is modified (not only after a failed submit), so the helperText explains the problem while the user is still typing. - Render a `broken_image` icon in the preview area when the current string value fails validation, instead of forwarding it to `<Logo>`. This keeps the preview honest during typing and avoids spamming Sentry from `<Logo>`'s runtime fallback path on every keystroke.
…lformed-s3-url # Conflicts: # catalog/app/containers/Admin/Settings/ThemeEditor.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4881 +/- ##
==========================================
+ Coverage 46.51% 46.55% +0.04%
==========================================
Files 832 832
Lines 34116 34131 +15
Branches 5826 5833 +7
==========================================
+ Hits 15868 15890 +22
+ Misses 16243 16239 -4
+ Partials 2005 2002 -3
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:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…en_image fallback
… and s3Url ThemeEditor was the only consumer of validators.url, so fold the hostname check into url itself instead of carrying a stricter sibling. Drop s3Url too: any value it accepted was already accepted by the hostname-checked url, making it dead code in the composeOr chain.
…seS3Url Single caller, five-line wrapper — easier to read inline.
|
@greptileai please update confidence score |
|
Fair point. The defensive-fallback rationale is sound — keeping Updated assessment: Low confidence / P3 at most. The dead-key observation is technically correct today, but the developer's intent (avoid a later "where did that message go?" regression when Tip: You can customize Greptile's behavior for this repo with |
<Logo> already passes blob/http URLs through to a plain <img> (only s3:// gets signed), so routing the File-value preview through it keeps behavior consistent with the string-URL branch and lets the spec keep querying the mocked Logo by testid.
|
@greptileai please update confidence score |
|
That's a reasonable forward-compatibility argument. The Updated confidence: Low — the comment is technically accurate about the current runtime behavior, but the intent to keep |
|
@greptileai please re-review |
s3://bucket (no key) was passing the URL validator (hostname=bucket) and then parseS3Url returns key='', so the AWS signer was called with an empty key and the browser rendered a broken image with no fallback. Two layers: - validators.logoUrl: replaces use of validators.url in the theme editor; rejects URLs without a hostname AND s3:// URLs without a key. - <Logo>: treats parseS3Url results with empty key as malformed, falling back to QuiltLogo + Sentry. Protects NavBar / persisted settings from pre-existing bad data, since the validator only runs on form input.
|
@greptileai please update confidence score |
|
The PR has been updated since the initial review. The previously identified gap —
The original concern is fully resolved. Updated confidence score: Confidence Score: 5/5 All three layers — runtime fallback ( |
Replace the null sentinel for malformed S3 URLs with a tagged union
{ _tag: 'ok' } | { _tag: 'error' }. The error variant carries the
real caught Error and the src that produced it, so Sentry reports the
actual cause instead of a synthetic message and the useEffect can
depend on parsedSrc alone. Render branch uses switch + assertNever.
Also narrow CustomLogo's prop type to src: string (parent already gates
on truthy src) and drop the unreachable !src defensive check.
|
@greptileai please re-review |
2 participants
Summary
<Logo>falls back to the default Quilt logo (and reports to Sentry) when the configuredsrcis a malformeds3://URL, instead of crashing the render tree viaparseS3Urlthrowing.s3paths.tryParseS3Url— a null-returning, render-safe counterpart toparseS3Url.validators.urlWithHost— stricter sibling ofvalidators.urlthat rejects URLs with no hostname (e.g.s3://, whichnew URL()accepts because non-special schemes are allowed empty hosts). Used in the logo URL field so partial inputs likes3://are caught at validation time, not silently masked by<Logo>'s runtime fallback.validators.urlWithHostandvalidators.s3Urlto the logo URL field's compose chain.meta.modified || meta.submitFailed) instead of only after submit.Previewsub-component; renders abroken_imageplaceholder for invalid URLs and ahide_imageplaceholder for the empty state.errorflag).Test plan
npm run test:only -- validators Logo Admin/Settings/ThemeEditorpasses (14 + 5 + 4 tests).s3://(no bucket) — broken-image preview + "Enter a valid URL (https:// or s3://)" helper text.s3://bucket/key.png— signed URL renders.https://URL — renders unchanged.s3://value — catalog header gracefully falls back to the Quilt logo (no white screen, Sentry report fires); editor flags the value once the field is modified.Greptile Summary
This PR adds two complementary layers of resilience for malformed S3 logo URLs: a runtime fallback in
<Logo>that catches parse failures (including bucket-only URLs) and gracefully renders the default Quilt logo with a Sentry report, and a newvalidators.logoUrlthat rejects no-hostname and no-key S3 URLs at form-input time.CustomLogonow wrapsparseS3Urlin a try/catch and checks for an empty key, returning a discriminated-union result; auseEffectreports errors to Sentry exactly once per distinct bad src.validators.logoUrlreplaces the oldurl/s3Urlpair with a single validator that usesnew window.URL(v).hostnameto reject empty-host URLs (e.g.s3://) and additionally rejectss3://bucketands3://bucket/where no key is present.meta.modified || meta.submitFailedand introduces aPreviewsub-component that gates the broken-image icon through the sameerrorflag ashelperText, keeping both UI signals in sync.Confidence Score: 5/5
Safe to merge — all changed paths have defence-in-depth (validator + runtime fallback) and are covered by new tests.
The change is well-scoped: it adds a try/catch + key-guard to one render function, a single new validator, and tightens one form field's error-display logic. Both the runtime fallback and the form-side validation are exercised by new tests covering the specific edge cases that triggered the original crash. No existing behaviour is removed in a way that could introduce regressions.
No files require special attention.
Important Files Changed
Reviews (3): Last reviewed commit: "refactor(catalog): model <Logo> parse re..." | Re-trigger Greptile