fix: handle service worker registration edge cases#960
Draft
Conversation
belt and suspenders protection against edge cases that could leave users stuck on loading screen. these should not impact normal operation, but will save debugging time if we ever hit them thanks to clear error messages and timeout: 1. race condition: only listened for `updatefound`, but if a worker was already installing or waiting, we'd miss it and wait forever 2. no timeout: if worker failed to activate, the promise would never resolve and page would hang indefinitely 3. silent failures: if worker became redundant (replaced by newer version mid-install), we had no handling and would wait forever now we track existing installing/waiting workers, add a 30s timeout, and properly reject if worker becomes redundant.
lidel
force-pushed
the
fix/service-worker-registration
branch
from
46ef88f to
b847d05
Compare
refactor to use idempotent succeed/fail helpers that prevent double resolution. check worker state after adding listener to close race condition window where worker could activate before listener attached.
lidel
force-pushed
the
fix/service-worker-registration
branch
from
b847d05 to
2d86369
Compare
Member
|
Is it possible to add some tests here? I'm not sure I'd want to refactor this without having some assurance a refactor would not break something. |
achingbrain
reviewed
Jan 21, 2026
an attempt at E2E tests for SW registration, but these are closer to unit tests than true E2E - they test the registration mechanism in isolation rather than user-facing behavior. may not be worth keeping, but provides some regression coverage for: - fresh registration activation - re-registration after unregister - multiple register/unregister cycles note: timeout error page cannot be tested due to Playwright limitation (SW script fetches are not interceptable)
Member
Author
|
@achingbrain unsure how we can test this fully, i looked at playwright docs and it cant intercept the SW registration requests in a way that allows us to test it E2E. Added two other tests but they don't project as much confidence as proper e2e would. We probably can park/close this PR and revisit if we ever see actual bugs irl. I agree we dont want to change this unless we need too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this PR is belt and suspenders protection against edge cases that could leave users stuck on loading screen.
i've seen crazy bugs in Google Chrome shipping in production, and then they silently fixed them month later, wasting peoples time on debug
these should not impact normal operation, but will save debugging time if we ever hit them thanks to clear error messages and timeout