fix(error-boundary): stop showing fatal UI for benign window errors#305
fix(error-boundary): stop showing fatal UI for benign window errors#305spe1020 merged 9 commits intozapcooking:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts client error handling so benign window errors no longer trigger the fatal ErrorBoundary UI, while also bringing in several UX improvements stacked from #304.
Changes:
- Update ErrorBoundary to suppress fatal UI for native
windowerrors (log-only) and remove the redundant/incorrect<svelte:window on:error>binding. - Add recipe editor draft auto-save + URL stabilization, plus media add-by-URL and improved drag-reorder behavior.
- Ensure markdown preview links consistently open in a new tab and inline early logo visibility CSS to prevent dark-mode logo flash.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/ErrorBoundary.svelte | Stops showing fatal UI for native window errors; logs suppressed errors. |
| src/routes/create/+page.svelte | Adds debounced auto-save and avoids URL thrash while typing. |
| src/components/MediaUploader.svelte | Adds collapsible “add by URL” flow with validation. |
| src/components/StringComboBox.svelte | Fixes drag reorder to allow dropping after the last item and improves drop indicator UX. |
| src/lib/parser.ts | Forces markdown links to retain target/rel after sanitization. |
| src/app.html | Inlines early CSS for logo visibility to prevent theme flash. |
| src/components/Header.svelte | Adds logo-light / logo-dark marker classes for early CSS targeting. |
| src/components/DesktopSideNav.svelte | Adds logo-light / logo-dark marker classes for early CSS targeting. |
Comments suppressed due to low confidence (1)
src/components/ErrorBoundary.svelte:39
- After removing
<svelte:window on:error={handleError} />,handleErroris no longer wired to any event source, anderroris never set anywhere else. That makes the fallback UI effectively unreachable and also contradicts the comment about preserving a programmatic CustomEvent path. If you still want deliberate fallbacks, add an explicit listener for the intended CustomEvent source (e.g., a custom window event name, oron:error={handleError}on a wrapper/<slot on:error={...}>for bubbled CustomEvents) while keeping nativeErrorEvents log-only.
// Error handling function — only invoked for programmatically dispatched
// CustomEvents (e.g. from child components that rethrow). Native window
// `error` events are handled separately in onMount and are treated as
// non-fatal (logged only), matching how unhandled rejections are handled.
function handleError(event: Event) {
const customEvent = event as CustomEvent;
error = customEvent.detail?.error || new Error('Unknown error');
errorInfo = customEvent.detail?.errorInfo;
retryCount++;
if (logErrors && error) {
logError(error, 'ErrorBoundary', errorInfo);
}
// Dispatch error event for external monitoring
dispatch('error', {
error,
errorInfo,
retryCount,
timestamp: Date.now()
});
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // DOMPurify's default config can drop target/rel on <a> in some browsers; | ||
| // force both back on after sanitization so rendered markdown links always | ||
| // open in a new tab (important for the recipe editor preview — clicking a | ||
| // link in-place would otherwise destroy unsaved edits). | ||
| if (typeof window !== 'undefined') { | ||
| DOMPurify.addHook('afterSanitizeAttributes', (node) => { | ||
| if (node.tagName === 'A') { | ||
| node.setAttribute('target', '_blank'); | ||
| node.setAttribute('rel', 'noopener noreferrer'); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
DOMPurify.addHook(...) is registered on the global DOMPurify instance at module load, which will affect all other DOMPurify.sanitize(...) calls in the app once parser.ts is imported (e.g., the longform editor also imports DOMPurify directly). Consider scoping this behavior to markdown sanitization by creating a dedicated DOMPurify instance for this module or by applying the attribute fix only to the sanitized markdown output, so unrelated sanitization paths aren’t forced to target=_blank.
| <button | ||
| type="button" | ||
| class="flex items-center gap-1 text-xs text-caption hover:opacity-80 transition-opacity cursor-pointer" | ||
| on:click={() => (urlSectionOpen = !urlSectionOpen)} | ||
| > | ||
| <CaretDownIcon | ||
| size={12} | ||
| class="transition-transform duration-200 {urlSectionOpen ? 'rotate-180' : ''}" | ||
| /> | ||
| <span>Add images by URL</span> | ||
| </button> |
There was a problem hiding this comment.
The URL add-section toggle button should expose its expanded/collapsed state to assistive tech. Add aria-expanded={urlSectionOpen} (and ideally aria-controls pointing at the collapsible region) so screen readers can understand the disclosure behavior.
| // Serialized form of all draft fields — drives change detection for auto-save | ||
| $: draftSignature = JSON.stringify([ | ||
| title, | ||
| $images, | ||
| $selectedTags, | ||
| summary, | ||
| chefsnotes, | ||
| preptime, | ||
| cooktime, | ||
| servings, | ||
| $ingredientsArray, | ||
| $directionsArray, | ||
| additionalMarkdown | ||
| ]); |
There was a problem hiding this comment.
draftSignature is assigned in a reactive statement but never declared. In Svelte/TS this will fail compilation (and it’s referenced in onMount before any implicit declaration). Declare it explicitly (e.g., let draftSignature: string = '') before it’s used.
d89f69b to
25c43a4
Compare
Tailwind's dark:hidden / dark:block on the logo <img> tags only take effect once the main CSS bundle loads. In dev (and occasional edge cases in prod) there's a brief window where both logos are visible and the light one paints first. Add a tiny inline <style> in app.html keyed on html.dark so the correct logo is selected before any external CSS arrives; tag the <img> pairs with logo-light / logo-dark.
A subtle "Add images by URL" toggle under the media grid lets users paste a direct image/video URL instead of uploading. First URL added becomes the cover photo (satisfying the publish requirement). Works across all three recipe editors (create, fork, gated) via the shared MediaUploader component.
DOMPurify can strip target/rel on <a> in some configurations, which caused markdown-preview links in the recipe editor to navigate in-place and destroy unsaved edits. Add an afterSanitizeAttributes hook that reapplies target=_blank + rel=noopener noreferrer on every anchor post-sanitization.
Watch the 11 draft fields via a JSON signature; 2s after the last edit, save to localStorage (and queue the debounced relay sync) so users don't lose work. Skips while empty and while a manual save is in flight; updates the URL with ?draft=<id> so reloads resume the same draft.
Auto-save was toggling isSavingDraft and writing a transient status message, which swapped the Save Draft button to "Saving…" and shifted the button row every 2s while typing — the editor appeared to jump around. The URL was also rewritten on every save. Make auto-save fully silent: no button state toggle, no message flash, and only write ?draft=<id> the first time a new draft gets an id. Manual save keeps its full feedback, and the Save Draft button's cloud icon still reflects relay sync state once the background publisher settles.
Two issues made reordering the last items nearly impossible: - handleDrop always inserted before the target, so dropping on the last item never moved the dragged item past it. Detect whether the cursor is in the target's top or bottom half and insert before or after accordingly. A top/bottom box-shadow marker shows which side the drop will land on (no layout shift). - The 8px flex gap between items was dead space — the drop zone only covered each pill. Drop gap-2 and give each <li> py-1 padding (first:pt-0 last:pb-0) so the hit area extends into the gap; the visible pill moves to an inner <div>. Total vertical spacing between items is unchanged.
- parser.ts: use a dedicated DOMPurify instance for markdown so the afterSanitizeAttributes hook doesn't leak target=_blank behavior into every other sanitize() call in the app (e.g. the longform editor). - MediaUploader.svelte: normalize the pasted URL via `parsed.toString()` before the dedupe check and before storing, so equivalent URLs (`example.com` vs `example.com/`) collapse to one entry. - MediaUploader.svelte: add `aria-expanded` + `aria-controls` to the "Add images by URL" disclosure toggle for screen-reader support. - create/+page.svelte: allow auto-save to persist when all fields are cleared on an existing draft (previously the delete-everything state was never saved, so reloading brought the old content back). - create/+page.svelte: declare `draftSignature` explicitly and debounce its computation to 250ms so we don't re-JSON.stringify the entire draft on every keystroke.
<svelte:window on:error> was catching native ErrorEvents but treating them as CustomEvents, so the details were lost and every spurious window error — including the ones iOS Safari fires during share / backgrounding / service-worker transitions — nuked the entire app with an "Oops! Something went wrong" screen. Remove the svelte:window binding and align the window 'error' listener with the existing 'unhandledrejection' listener: log-only, no fatal UI. The programmatic CustomEvent path is preserved for child components that want to surface a fallback deliberately.
After the earlier patch removed `<svelte:window on:error={handleError} />`
to stop benign window errors from nuking the app, `handleError` had no
caller and the entire fallback UI / retry / error-state code was
effectively dead — RecipeErrorBoundary and FeedErrorBoundary would
never render their fallback.
Re-wire `handleError` to a dedicated `window` CustomEvent
(`zc:fatal-error`). Child components that hit a genuinely fatal error
can dispatch:
window.dispatchEvent(new CustomEvent('zc:fatal-error', {
detail: { error, errorInfo }
}));
to surface the boundary's fallback UI. Native ErrorEvents are still
logged-only, same as before.
25c43a4 to
5d20271
Compare
Summary
<svelte:window on:error={handleError} />was catching nativeErrorEvents but parsing them asCustomEvent.detail, so the real error was always lost and every spurious window error — including the ones iOS Safari fires during share / backgrounding / service-worker transitions — nuked the entire app with an "Oops! Something went wrong" screen.Fix:
svelte:windowbinding (it was redundant with thewindow.addEventListener('error', ...)inonMount, and itsCustomEventcoercion was wrong).errorlistener with the existingunhandledrejectionlistener: log to console, don't replace the page.CustomEvent-dispatch path for child components that want to surface a fallback deliberately.Notes
src/components/ErrorBoundary.svelte.Test plan
throw new Error('test')in asetTimeoutvia devtools → console shows[ErrorBoundary] Window error (suppressed): ...and the app remains rendered.