Fix service worker handling, #167
Conversation
📝 WalkthroughWalkthroughService worker fetch failures now return plain-text 503 responses; window bootstrap guards registration for unsupported/duplicate/crossOriginIsolated cases and derives script URL; after activation the page reloads once per session using ChangesService Worker Reliability and Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/coi-serviceworker.js`:
- Around line 89-97: The current service worker activation wait creates a
Promise whose executor uses await (and thus is improperly async) and never
resolves; replace that pattern with a single non-async Promise that (a)
immediately checks sw.state and resolves if already "activated", and (b)
otherwise adds a "statechange" listener on sw that calls resolve when
event.target.state === "activated" and removes the listener afterwards; ensure
the Promise executor is not declared async and do not nest another await inside
it so the outer await can actually complete.
- Around line 73-79: Capture the current script URL into a local variable (e.g.,
const scriptSrc = window.document.currentScript?.src) before calling await
navigator.serviceWorker.getRegistration(); then call
navigator.serviceWorker.getRegistration(), and only treat the worker as "already
registered" if existingRegistration exists and existingRegistration.scriptURL
=== scriptSrc; otherwise proceed to register using the captured scriptSrc. Also
handle the case where scriptSrc is null/undefined by failing early with an
explanatory log/error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@ANSHIKATYAGI30 please check the CI failure |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/coi-serviceworker.js`:
- Around line 103-109: The guard logic around RELOAD_KEY makes the inner branch
unreachable because RELOAD_KEY equals "coiReloaded" and you set that key
immediately before re-checking it; remove the redundant inner check and call
window.location.reload() right after setting the key (i.e., in the block where
!sessionStorage.getItem(RELOAD_KEY) is true), using the existing RELOAD_KEY
constant and sessionStorage.getItem/setItem to ensure a one-time reload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@sudip-mondal-2002 Kindly review whenever convenient. |
|
@ANSHIKATYAGI30 Please look at the test failure |
🌍 Cross-Platform Coverage ReportPlatforms: Linux + Windows ✅ Coverage meets threshold: 78.2% >= 60% Full Merged Coverage Summary |
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
There was a problem hiding this comment.
Check the CI failure
sudip-mondal-2002
added
type:bug
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/coi-serviceworker.js`:
- Around line 106-121: The Promise executor currently contains an invalid
async-await usage causing a SyntaxError; fix by ensuring the executor is a plain
synchronous function and only use a single top-level await with new Promise that
resolves when sw reaches "activated". Locate the activation-wait block using the
sw variable and the onStateChange handler, remove any inner await or async
keywords inside the Promise executor, and implement the logic so the executor
immediately resolves if sw.state === "activated" or otherwise adds a
"statechange" listener (onStateChange) that removes itself and calls resolve()
when e.target.state === "activated".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (sw) { | ||
| await new Promise((resolve) => { | ||
| // Resolve immediately if already activated | ||
| if (sw.state === "activated") { | ||
| resolve(); | ||
| return; | ||
| } | ||
| const onStateChange = (e) => { | ||
| if (e.target.state === "activated") { | ||
| sw.removeEventListener("statechange", onStateChange); | ||
| resolve(); | ||
| } | ||
| }; | ||
| sw.addEventListener("statechange", onStateChange); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the file has valid JavaScript syntax
# Use node to check syntax
node --check web/coi-serviceworker.js || echo "Syntax error detected"
# Show the problematic lines
echo "=== Lines 105-122 ==="
sed -n '105,122p' web/coi-serviceworker.js | cat -nRepository: sudip-mondal-2002/Amplitron
Length of output: 1201
Critical: Fix await inside non-async Promise executor (parse-time SyntaxError).
web/coi-serviceworker.js fails a syntax check: Node reports SyntaxError: await is only valid in async functions ... at the inner await new Promise((resolve) => { ... (around line 107 in the file). This means the service worker activation wait logic cannot run as written.
Remove the invalid await usage from inside the Promise executor and rewrite the activation-wait to use a single await new Promise(...) (with the executor being non-async), only waiting for the "activated" state.
🧰 Tools
🪛 Biome (2.4.15)
[error] 107-107: await is only allowed within async functions and at the top levels of modules.
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/coi-serviceworker.js` around lines 106 - 121, The Promise executor
currently contains an invalid async-await usage causing a SyntaxError; fix by
ensuring the executor is a plain synchronous function and only use a single
top-level await with new Promise that resolves when sw reaches "activated".
Locate the activation-wait block using the sw variable and the onStateChange
handler, remove any inner await or async keywords inside the Promise executor,
and implement the logic so the executor immediately resolves if sw.state ===
"activated" or otherwise adds a "statechange" listener (onStateChange) that
removes itself and calls resolve() when e.target.state === "activated".
|
@ANSHIKATYAGI30 are you still working on this? |
|
@sudip-mondal-2002 yes, I'm. |
…into fix-service-worker-handling
…IKATYAGI30/Amplitron into fix-service-worker-handling
…IKATYAGI30/Amplitron into fix-service-worker-handling
|
@ANSHIKATYAGI30 PTAL at the CI failures |
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
There was a problem hiding this comment.
@ANSHIKATYAGI30 Are you still working on this? the CI is failing, please take a look
|
@ANSHIKATYAGI30 CI checks on web demo are still failing, fix it |
|
@sudip-mondal-2002 |
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3 participants
What does this PR do?
This PR improves the reliability and stability of the COOP/COEP service worker used for enabling SharedArrayBuffer support in Amplitron.
Changes included:
These changes help improve audio engine stability and prevent runtime crashes or blank-screen issues caused by failed service worker responses.
Related Issue
Fixes #161
Type of Change
How Was This Tested?
./amplitron-testsChecklist
./amplitron-tests)std::mutex::lock()on the hot path)##Summary
Before
After
Summary by CodeRabbit
Bug Fixes
Compatibility
Performance