fix(web): add alert when pdf export popup is blocked (#664)

[PratikRai0101]

Summary

This PR resolves issue #664, where clicking Share > Export as PDF fails silently with no user feedback if the browser's aggressive popup blocker halts the window.open call.

In the previous implementation, when a browser blocked the export window, the app would proceed without notifying the user, leaving the interface completely unresponsive to their action. This fix introduces robust error handling that immediately detects a blocked popup, alerts the user with actionable feedback, and cleanly halts execution.

---

Root Cause Analysis

The PDF export workflow in apps/web/src/runtime/exports.ts utilizes the following sequence:

1. Compiles the target design document into a static HTML string.
2. Generates a local blob URL (blob:http...).
3. Executes window.open(url, '_blank', ...) to open the printable preview in a new tab.

Under default security configurations in modern browsers (Chrome, Safari, Firefox, Brave), synchronous or unprompted window.open calls are flagged as popups and blocked. When blocked:

• window.open returns null (or undefined in some legacy engines).
• The code had an empty check: if (!win) { /* empty block */ }.
• As a result, execution silently skipped ahead, keeping the transient blob URL in memory without ever notifying the physical user why their action didn't yield a file download or print dialog.

---

Detailed Changes

apps/web/src/runtime/exports.ts

• Added Null Safety Check: Replaced the empty if (!win) block with an explicit check immediately following the window.open call.
• User-Facing Alert: Implemented a clear browser alert instructing the user that their popup blocker prevented the export and telling them how to resolve it.
• Early Return Execution Halt: Added an early return statement to stop any subsequent operations when win is null, preventing unnecessary resource overhead and potential orphan processes.

const blob = new Blob([doc], { type: 'text/html;charset=utf-8' });
const url = URL.createObjectURL(blob);
const win = window.open(url, '_blank', sandboxedPreview ? 'noopener,noreferrer' : undefined);

// --- FIX IMPLEMENTED HERE ---
if (!win) {
  // Gracefully handle browser popup blocking and notify the user
  alert('Popup blocked! Please allow popups for this site to export as PDF.');
  return;
}
// -----------------------------

// Revoke later — the loaded document keeps a reference until the tab
// closes; revoking the URL string only removes the lookup name.
setTimeout(() => URL.revokeObjectURL(url), 60_000);

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a17485e1a2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lefarcen]

Hi @PratikRai0101! 🎉

Thanks for the contribution — the alert() approach gives immediate feedback and the early return prevents resource waste.

I will run a deep review and get back to you within 24h.

Thanks for making open-design better!
— open-design team

[lefarcen]

Code review

What's working

The fix correctly detects popup blocking and prevents silent failure. The early return stops execution cleanly when window.open returns null, which is exactly what issue #664 called for.

Minor suggestions (non-blocking)

See inline comments below. These are opportunities to align with modern UX patterns and strengthen the fix, but the current implementation already resolves the reported bug.

Overall: the core fix is sound. The suggestions below would polish the user experience and test coverage.

[PratikRai0101]

hey @lefarcen I have updated the code to explicitly trigger URL.revokeObjectURL(url) immediately before we return early from the block. The fix is pushed.

[lefarcen]

Perfect! The memory leak is now addressed — runs before the early return.

I also see you added the typeof alert !== 'undefined' guard and the !sandboxedPreview condition. Those are smart defensive checks that prevent potential runtime errors in headless/test environments and avoid redundant alerts in sandboxed flows.

The P3 concern is resolved. The core fix is solid and ready from my side. Deferring final approval to a maintainer.

[lefarcen]

Good news @PratikRai0101! The TS1117 duplicate object key error blocking your CI has been fixed in PR #680 (just merged).

The duplicate Ukrainian translation keys in uk.ts have been removed. Your CI should pass once it re-runs against the latest main.

If CI is still running, you might need to rebase/merge main into your branch to pick up the fix.

[lefarcen]

✅ Blocker resolved: PR #680 (merged) fixed the TS1117 duplicate Ukrainian keys in uk.ts.

Your CI should pass once it re-runs. If still running, try rebasing main.

[alchemistklk]

Thanks for the fix. One issue remains before merge: the new alert branch is currently unreachable for existing PDF exports.

All current call sites use exportAsPdf(..., { deck }), so sandboxedPreview defaults to true. The new code only alerts when !win && !sandboxedPreview, meaning the Share > Export as PDF flow still gives no feedback if the popup is blocked.

Please update exportAsPdf so blocked popup detection works under the default sandboxed export path, or use a central fallback such as opening a blank tab synchronously, checking window.open failure, then navigating the opened tab to the blob URL.

[lefarcen]

Good catch, @alchemistklk! You're absolutely right — the current condition if (!win && !sandboxedPreview) makes the alert unreachable since sandboxedPreview defaults to true (line 226).

@PratikRai0101, the fix needs to handle the blocked-popup case regardless of sandboxedPreview. The simplest correction is to remove the !sandboxedPreview condition entirely:

if (!win) {
  if (typeof alert !== 'undefined') {
    alert('Popup blocked! Please allow popups for this site to export as PDF.');
  }
  URL.revokeObjectURL(url);
  return;
}

This way, the user gets feedback whether the export is sandboxed or not. The typeof alert guard already handles headless/test environments gracefully.

[mrcfps]

@PratikRai0101 thanks for iterating on the PDF export popup handling. I found one remaining blocker: the new blocked-popup alert is still bypassed by the default sandboxed export path used by the current UI, so the primary Share > Export as PDF flow can still fail silently.

_{Generated by Looper 0.6.1 · runner=reviewer · agent=opencode}

[PratikRai0101]

@mrcfps @lefarcen @alchemistklk The PR has been updated with the empty-tab synchronous strategy!.

Here is how it behaves now:

1. We synchronously open a blank tab first (window.open('', '_blank')) without the noopener string to reliably detect if the browser's popup blocker intercepts it.
2. If the popup blocker triggers, we cleanly revoke the Blob URL, alert the user, and return early to prevent memory leaks.
3. If successful, we manually disassociate the opener reference (win.opener = null) to preserve the sandboxed/noopener security behavior, and then navigate the verified window (win.location.href = url).

I also successfully rebased against the latest main branch to pull in the Ukrainian locale typefix (#680). The typecheck and build pipelines now compile flawlessly.

[lefarcen]

Perfect! The synchronous empty-tab strategy directly addresses the blocker — now reliably detects popup blocking regardless of the default, and you manually preserve the sandboxing with win.opener = null.

The P1 correctness issue is resolved. The approach is clean and handles the edge case @mrcfps flagged.

One minor note (non-blocking): the blob URL is no longer revoked after use. This is a P3 memory hygiene nit at most — the OS will GC blob URLs when the page unloads, so it won't cause a practical leak. If you want to tidy it up, you could add a win.addEventListener('beforeunload', () => URL.revokeObjectURL(url)) or leave it as-is.

Core fix is solid now. Waiting for CI to confirm the rebase didn't introduce any regressions.

[lefarcen]

Re-reviewed the new head (f30e279) — the P1 blocker is resolved.

What's fixed

The synchronous empty-tab strategy directly addresses the correctness issue @alchemistklk and @mrcfps identified:

1. Opens empty tab first: window.open('', '_blank') without noopener → can reliably detect if win === null (blocked popup)
2. Alert on block: If blocked → revoke blob URL → alert user → return early
3. Manual sandboxing: If successful → win.opener = null (preserves security) → navigate to blob URL

This works regardless of the sandboxedPreview default. The fix now covers the default Share > Export as PDF flow.

Minor note (non-blocking)

The blob URL is no longer revoked after use (the setTimeout(() => URL.revokeObjectURL(url), 60_000) is gone). This is a P3 memory hygiene nit at most — the OS will GC blob URLs when the page unloads, so it won't cause a practical leak. If you want to tidy it up later, you could add win.addEventListener('beforeunload', () => URL.revokeObjectURL(url)) or leave it as-is.

Verdict

Core fix is solid. The P1 correctness blocker is resolved. Waiting for CI to confirm the rebase didn't introduce regressions.

[mrcfps]

@PratikRai0101 thanks for the update — the empty-tab strategy resolves the previous popup-blocker detection issue for the main PDF export flow. I found one blocking follow-up around the existing web test coverage that needs to be updated with the new window.open contract before this can merge. 👍

_{Generated by Looper 0.6.1 · runner=reviewer · agent=opencode}

[mrcfps]

This runtime change now opens a blank tab with window.open('', '_blank') and then navigates the returned handle, which is a reasonable fix for distinguishing a real popup block from noopener returning null. However, the existing web tests still encode the old contract: apps/web/tests/runtime/exports.test.ts stubs window.open to capture the third features argument and the default sandboxed PDF tests assert openedFeatures === 'noopener,noreferrer'. Because this PR only changes exports.ts, those assertions no longer match the changed line here and the test suite will fail when dependencies are installed and the web runtime tests run.

Please update the PDF export tests alongside this implementation: have the window.open stub return a small WindowProxy-like object with opener and location.href, assert the successful sandboxed path clears opener and navigates to the blob URL, and add/adjust the blocked-popup case where open returns null so it verifies alert and immediate URL.revokeObjectURL. That keeps CI aligned with the new behavior while preserving coverage for the bug this PR fixes.

[PratikRai0101]

hey @mrcfps rightly said the strict assertions on the old window.open contract were causing the test suite to fail locally.

I've refactored apps/web/tests/runtime/exports.test.ts to fully support the new synchronous empty-tab architecture:

• window.open is now stubbed to return a WindowProxy-like mock ({ opener: {}, location: { href: '' } }).
• Success paths now correctly assert that mockWin.opener is cleared to null (for sandboxed exports) and that mockWin.location.href handles the navigation.
• Added explicit blocked-popup coverage: stubbed window.open to return null, and asserted that global.alert and URL.revokeObjectURL are invoked synchronously.

All 310 tests are green locally. Thanks for the help!

[mrcfps]

@PratikRai0101 thanks for continuing to iterate on the popup-blocked PDF export flow — the main detection path and the updated tests now line up nicely. I found one small resource-lifecycle follow-up that should be safe to handle without blocking the primary fix.

Non-blocking follow-up in apps/web/src/runtime/exports.ts, exportAsPdf, around win.location.href = url: the successful PDF-export path now navigates the opened tab to the Blob URL, but the delayed URL.revokeObjectURL(url) that used to run after a successful open was removed in this same hunk. The blocked-popup branch correctly revokes on early exit, yet every successful export now leaves its Blob URL registered until the main app page unloads, which can accumulate memory in a long-running session when users export multiple large artifacts. Please restore a success-path cleanup after navigation, e.g. schedule setTimeout(() => URL.revokeObjectURL(url), 60_000) after win.location.href = url as the previous implementation did.

_{Generated by Looper 0.6.1 · runner=reviewer · agent=opencode}

[mrcfps]

Approved.