Skip to content

Conversation

@rebelchris
Copy link
Contributor

@rebelchris rebelchris commented Dec 23, 2025

Changes

Describe what this PR does

  • The service worker (sw.ts) now detects Safari (excluding other browsers that spoof the UA) and immediately unregisters itself to prevent the WebKitInternal:0 crash.
  • _app.tsx has been updated to proactively detect Safari, unregister any existing /sw.js service workers, and prevent Serwist from being registered for Safari users.
  • Trade-off: Safari users will temporarily lose service worker caching and offline capabilities, but the site will load reliably without crashing.

Events

No new tracking events.

Experiment

No new experiments.

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

On those affected packages:

  • Have you done sanity checks in the webapp?

Did you test on actual mobile devices?

  • iOS (Chrome and Safari) - Crucial to validate the fix on physical Safari devices, especially with existing service worker caches.

ENG-210


Linear Issue: ENG-210

Open in Cursor Open in Web

Preview domain

https://cursor-eng-210-safari-service-wo.preview.app.daily.dev

@cursor
Copy link

cursor bot commented Dec 23, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@vercel
Copy link

vercel bot commented Dec 23, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
daily-webapp Ready Ready Preview Jan 7, 2026 11:53am
1 Skipped Deployment
Project Deployment Review Updated (UTC)
storybook Ignored Ignored Jan 7, 2026 11:53am

Copy link
Member

@idoshamun idoshamun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you managed to reproduce and validate the fix?

@rebelchris
Copy link
Contributor Author

@idoshamun I tested it before it was indeed the safari crash loop on outdated sw, haven't validated though. it's just vibes that disable it all together for safari for now.

@rebelchris
Copy link
Contributor Author

@claude please review and let me know if anything here can break the app?

@claude
Copy link
Contributor

claude bot commented Jan 7, 2026

Claude finished @rebelchris's task in 1m 43s —— View job


Code Review Progress

  • Read modified files
  • Analyze Safari detection logic
  • Check for potential breaking changes
  • Review service worker lifecycle handling
  • Identify edge cases and race conditions
  • Provide comprehensive feedback

Code Review Findings

I've analyzed the changes and identified several potential issues that could break the app or cause unexpected behavior:

⚠️ Critical Issues

1. Safari Detection Logic Mismatch Between Files (_app.tsx:156 vs sw.ts:18-21)

The Safari detection logic differs between the two files:

  • _app.tsx uses getCurrentBrowserName() which checks if userAgent.includes('Safari') but doesn't filter out Chrome/Edge/etc properly
  • sw.ts uses a custom regex that explicitly excludes Chrome|Chromium|Android|CriOS|FxiOS|Edg|OPR

This inconsistency is dangerous because:

  • Chrome's UA contains "Safari" (e.g., Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36)
  • If getCurrentBrowserName() incorrectly identifies Chrome as Safari, you'll disable service workers for all Chrome users

Let me check the actual Safari detection logic:

Looking at func.ts:126-154, the getCurrentBrowserName() function checks Chrome/Brave/Firefox/Edge before Safari, so it should work correctly. However, the order matters, and I'm concerned about the simple includes('Safari') check in UserAgent.Safari.

Verify Safari detection doesn't false-positive on Chrome →

2. Race Condition in Service Worker Unregistration (_app.tsx:124-154)

The unregisterSerwist() function runs asynchronously but isn't awaited:

if (browserName === BrowserName.Safari) {
  didRegisterSwRef.current = true;
  unregisterSerwist(); // ❌ Not awaited!
  return;
}

Why this is dangerous:

  • The unregistration might still be in progress when the user navigates
  • If a Safari user has an existing service worker, it might intercept requests before being unregistered
  • This could lead to the exact WebKitInternal:0 crash you're trying to prevent

Fix: Either await the unregistration or move the logic outside the useEffect to ensure it completes before any navigation occurs.

Fix race condition by awaiting unregisterSerwist →

3. Service Worker clients.claim() Before Unregister (sw.ts:33-34)

await self.clients.claim();
await self.registration.unregister();

Problem: You call clients.claim() to take control of all clients, then immediately unregister. This creates a brief window where:

  • The SW takes control of pages
  • Then disappears
  • Pages might be left in a controlled state with no active SW

This could cause navigation failures or cached responses to never load.

Better approach: Just skipWaiting() + unregister() without claiming clients.

Remove clients.claim() before unregister →

⚠️ Moderate Issues

4. Silent Error Swallowing (_app.tsx:151-153)

} catch {
  // Ignore cleanup failures – Safari sometimes rejects during shutdown.
}

While this prevents crashes, it also hides legitimate errors. Consider:

  • Logging errors to analytics/monitoring (without crashing)
  • Different handling for different error types

5. Missing Feature Flag or Gradual Rollout

This is a major behavioral change affecting all Safari users. If something goes wrong, you can't quickly disable it without deploying new code.

Recommendation: Wrap the Safari SW disabling logic in a feature flag (GrowthBook) so you can:

  • Roll out gradually (e.g., 10% of Safari users first)
  • Quickly disable if issues arise
  • A/B test the impact

Add feature flag for Safari SW disabling →

6. No Cleanup of Existing Caches (_app.tsx:124-154)

You unregister the service worker, but don't clear the caches it created. This could leave:

  • Stale cached resources that never update
  • Orphaned cache storage taking up space

Consider adding cache cleanup:

if ('caches' in window) {
  const keys = await caches.keys();
  await Promise.all(keys.map(key => caches.delete(key)));
}

ℹ️ Minor Issues

7. Inconsistent didRegisterSwRef Usage (_app.tsx:86, 159, 168)

The ref is set to true for Safari (line 159) to prevent re-running, but this means if a user switches browsers (unlikely but possible in dev), the logic won't re-run. This is probably fine but worth noting.

8. Service Worker Detection Logic Incompleteness (_app.tsx:118-122)

The early returns don't account for:

  • navigator.serviceWorker being defined but getRegistrations throwing (some privacy modes)
  • The case where window.serwist is undefined but service workers are supported

These edge cases might leave some users without SW when they should have it (non-Safari browsers).


Summary

Will this break the app?

For non-Safari users: Probably not, assuming getCurrentBrowserName() correctly identifies Chrome/Edge/Firefox.

For Safari users: The race condition (#2) and clients.claim() issue (#3) could cause problems:

  • Users might still hit the crash if unregistration doesn't complete fast enough
  • Navigation might fail during the brief window where SW claims then unregisters

Biggest risks:

  1. 🔴 Race condition - unregisterSerwist() not awaited
  2. 🔴 Safari detection false positives - could disable SW for Chrome users
  3. 🟡 No feature flag - can't roll back without deployment

Recommendations:

  1. Fix the race condition by awaiting unregisterSerwist()
  2. Add comprehensive logging to track Safari detection accuracy
  3. Add a feature flag for gradual rollout
  4. Test thoroughly on actual Safari devices (iOS checkbox still unchecked!)
  5. Consider cache cleanup to prevent orphaned data

The core approach is sound, but the implementation has some rough edges that could cause issues. I'd recommend addressing issues #1-3 before merging.


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.

4 participants