Apply Bswup improvements (#12408)#12418
Open
msynk wants to merge 25 commits into
Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new progress/error UI path, hardens service-worker install and caching flows, adds update polling and reload coordination, updates build and demo worker configuration, and expands the README for the new runtime behavior. ChangesBit.Bswup Service Worker Error Handling & Update Polling
Sequence Diagram(s)sequenceDiagram
participant Browser
participant BitBswup
participant ServiceWorker
participant Blazor
Browser->>BitBswup: startup / DOMContentLoaded
BitBswup->>ServiceWorker: registration.update()
ServiceWorker-->>BitBswup: updateNotFound / updateCheckFailed / waiting
BitBswup->>Blazor: startBlazor() when allowed
ServiceWorker-->>BitBswup: controllerchange / WAITING_SKIPPED / error
BitBswup->>Browser: reloadOnce()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 47-57: The manifest validation detects errors (MANIFEST_ERRORS
from validateAssetsManifest) but the code continues to run and later
dereferences self.assetsManifest.assets, causing a crash; after calling
sendError you should stop further startup: return/throw to prevent asset
enumeration, and guard any access to self.assetsManifest.assets (e.g., use
Array.isArray(self.assetsManifest.assets) before iterating) and ensure VERSION
fallback is computed safely (use (self.assetsManifest && typeof
self.assetsManifest.version === 'string') ? self.assetsManifest.version :
'0.0.0-invalid-manifest') so the service worker does not proceed when the
manifest is invalid; update the block around MANIFEST_ERRORS, sendError,
VERSION, and any asset-enumeration code that uses self.assetsManifest.assets to
enforce this early exit and safe checks.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 321-324: The catch block in checkForUpdate currently calls
handle(BswupMessage.error, ...) which routes transient registration.update()
failures into the blocking install-error path; change this to emit a
non-blocking message instead (e.g., add a new enum/value like
BswupMessage.updateCheckFailed or use a non-fatal level such as
BswupMessage.warn/info) and call handle(...) with that new message name,
preserving the payload ({ reason: 'update', message: String((err && (err as
any).message) || err), reload }) so callers can still react but the default
progress handler will not hide the app or show the install-failed UI. Ensure to
update any switch/handler that consumes BswupMessage.* to treat the new
updateCheckFailed value as non-blocking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae99fd6f-8550-4920-b5b5-7982f9b3572d
📒 Files selected for processing (7)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/README.md
msynk
requested review from
Cyrus-Sushiant,
Copilot,
mhrastegari,
yasmoradi and
zahra-yousefi
Member
Author
|
@coderabbitai re-review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== bit-bswup.sw.ts ==="
fd "bit-bswup.sw.ts" --exec cat {}
echo ""
echo "=== bit-bswup.ts ==="
fd "bit-bswup.ts" -E "*.js" --exec cat {}
echo ""
echo "=== bit-bswup.progress.ts ==="
fd "bit-bswup.progress.ts" --exec cat {}
echo ""
echo "=== BswupProgress.razor ==="
fd "BswupProgress.razor" --exec cat {}
echo ""
echo "=== bit-bswup.progress.css ==="
fd "bit-bswup.progress.css" --exec cat {}
echo ""
echo "=== service-worker.js (demo) ==="
fd "service-worker.js" --search-path "Bit.Bswup.Demo" --exec cat {}
echo ""
echo "=== README.md (Bswup) ==="
cat src/Bswup/README.mdLength of output: 87174
🔴 Critical — Crash when
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Bit.Bswup’s update experience and robustness by adding user-visible install error reporting (with retry), introducing opt-in update polling (interval and/or tab visibility), improving multi-tab update consistency, and expanding Blazor script auto-detection to cover both Blazor Web App and standalone WebAssembly entry points.
Changes:
- Add update polling controls (
updateInterval,updateOnVisibility) and expose/extend JS API docs (BitBswup.checkForUpdate,skipWaiting,forceRefresh). - Improve service worker install reliability (manifest validation, retries/backoff, strict vs lax error tolerance, safer multi-tab activation/claim behavior).
- Enhance the progress UI with an error panel (message/details + retry) and corresponding styles.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Bswup/README.md | Documents new script attributes, error tolerance behavior, multi-tab behavior, security notes, and JS API. |
| src/Bswup/Bit.Bswup/Styles/bit-bswup.progress.css | Adds styling for the new install error panel and retry button behavior. |
| src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts | Adds idempotent initialization, update polling, better controllerchange handling, richer logging levels, and expanded Blazor script detection. |
| src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts | Adds manifest validation + error reporting, retry/backoff for asset fetches, strict/lax install semantics, safer cache update sequencing, and RegExp parsing improvements. |
| src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts | Adds UI handling for install errors (message/details/retry) and keeps assets list visibility in sync with runtime config changes. |
| src/Bswup/Bit.Bswup/BswupProgress.razor | Updates the progress component markup to include an error panel and changes initialization to run via a <script> block. |
| src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js | Removes demo precachedAssetsInclude config line. |
| src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.js | Adjusts demo config to use errorTolerance = 'lax' with explanatory comment. |
Member
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (1)
333-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
e.waitUntil()for async work in the service workermessagehandler.Returning a Promise from the
messagelistener doesn’t reliably keep the worker alive; useExtendableMessageEvent.waitUntil(...)forskipWaiting(),clients.claim(), cache deletion/filling, and the follow-uppostMessage.Suggested fix
-function handleMessage(e: MessageEvent<string>) { +function handleMessage(e: any) { diag('handleMessage:', e); if (e.data === 'SKIP_WAITING') { - return self.skipWaiting() - .then(() => self.clients.claim()) - .then(() => deleteOldCaches()) - .then(() => sendMessage('WAITING_SKIPPED')); + e.waitUntil( + self.skipWaiting() + .then(() => self.clients.claim()) + .then(() => deleteOldCaches()) + .then(() => sendMessage('WAITING_SKIPPED')) + ); + return; } if (e.data === 'CLAIM_CLIENTS') { - return self.clients.claim() - .then(() => deleteOldCaches()) - .then(() => e.source?.postMessage('CLIENTS_CLAIMED')); + e.waitUntil( + self.clients.claim() + .then(() => deleteOldCaches()) + .then(() => e.source?.postMessage('CLIENTS_CLAIMED')) + ); + return; } if (e.data === 'BLAZOR_STARTED') { - createAssetsCache(true); + e.waitUntil(createAssetsCache(true)); + return; } if (e.data === 'CLEAN_UP') { - deleteOldCaches(); // remove the old caches + e.waitUntil(deleteOldCaches()); // remove the old caches } }🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts` around lines 333 - 362, The message handler currently returns Promises but must use e.waitUntil(...) to keep the service worker alive; wrap each async branch so that SKIP_WAITING uses e.waitUntil(self.skipWaiting().then(() => self.clients.claim()).then(() => deleteOldCaches()).then(() => sendMessage('WAITING_SKIPPED'))), CLAIM_CLIENTS uses e.waitUntil(self.clients.claim().then(() => deleteOldCaches()).then(() => e.source?.postMessage('CLIENTS_CLAIMED'))), BLAZOR_STARTED uses e.waitUntil(createAssetsCache(true)), and CLEAN_UP uses e.waitUntil(deleteOldCaches()), ensuring all calls to self.skipWaiting(), self.clients.claim(), deleteOldCaches(), createAssetsCache(true), sendMessage and e.source?.postMessage are inside the waitUntil chains.
🤖 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 `@src/Bswup/Bit.Bswup/BswupProgress.razor`:
- Around line 37-39: The inline script in BswupProgress.razor calls
BitBswupProgress.start(...) unguarded which throws if bit-bswup.progress.js
hasn't loaded; update the init to check for the global before invoking (e.g.,
use window.BitBswupProgress?.start(...) or an explicit if
(window.BitBswupProgress) call) passing the same parameters (AutoReload,
ShowLogs, ShowAssets, AppContainer, HideApp, AutoHide, Handler), or
alternatively move the startup into the component lifecycle (OnAfterRenderAsync)
and call JS interop to invoke BitBswupProgress.start to guarantee ordering.
- Line 38: The call to BitBswupProgress.start injects AppContainer and Handler
as raw single-quoted JS literals which can be broken by
quotes/backslashes/newlines; change those embeddings to JSON-encoded JS-safe
strings instead (e.g., replace the single-quoted '@(AppContainer)' and
'@(Handler)' with the output of
System.Text.Json.JsonSerializer.Serialize(AppContainer) and
JsonSerializer.Serialize(Handler) so the values are properly escaped and already
quoted), update the BswupProgress.razor to use JsonSerializer.Serialize for both
AppContainer and Handler and add the required using/import for System.Text.Json
if missing.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 86-94: The cache bucket currently uses only CACHE_VERSION (derived
from self.cacheVersion or VERSION) which allows different manifests to share a
bucket; change the cache naming so the manifest VERSION is always part of the
bucket identity. Keep the existing CACHE_VERSION calculation (using
self.cacheVersion fallback to VERSION) but construct CACHE_NAME to include both
VERSION and CACHE_VERSION (e.g. `${CACHE_NAME_PREFIX} - ${VERSION} -
${CACHE_VERSION}`), so the manifest VERSION remains in the cache bucket while
cacheVersion can still be used to pin/force refresh.
- Around line 516-540: The current logic sets isIntegrity true whenever
hasIntegrity && fetchErr instanceof TypeError, which treats all TypeError fetch
rejections as SRI failures and prevents retries; change the detection in the
addCache flow so a bare TypeError does NOT automatically mark an integrity
failure: remove fetchErr instanceof TypeError from the isIntegrity predicate and
instead only classify as integrity when there is a clear integrity signal (e.g.,
the error message matches the integrity/digest regex) or after explicitly
recomputing and comparing the response bytes to asset.hash; keep transient
TypeError cases as retryable (use lastError/continue while attempt <
MAX_RETRIES), and only increment integrityFailureCount, set sendError.reason to
'integrity', and skip retries when you have a confirmed integrity mismatch (use
the integrity regex or the explicit digest comparison to decide).
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Line 35: Guard runBswup against late script injection by checking
document.readyState before adding the listener: if the document is already
"interactive" or "complete" call runBswup() immediately, otherwise attach
window.addEventListener('DOMContentLoaded', runBswup) (refer to the runBswup
symbol). Also fix BitBswup.forceRefresh where cacheFilter.test(key) is used:
avoid inconsistent results when cacheFilter has the global (g) or sticky (y)
flags by resetting cacheFilter.lastIndex = 0 before each test or by creating a
non-global equivalent RegExp from cacheFilter (use cacheFilter.source with flags
stripped of g/y) and testing against that; reference the BitBswup.forceRefresh
method and the cacheFilter variable so the change is made exactly where repeated
RegExp.test calls occur.
- Around line 497-500: forceRefresh builds the shouldDelete predicate from
cacheFilter but when cacheFilter is a RegExp it calls cacheFilter.test()
repeatedly, which can misbehave for /.../g or /.../y because RegExp.lastIndex is
mutated; fix by updating the RegExp branch in the shouldDelete construction
(used in forceRefresh) to reset cacheFilter.lastIndex = 0 before calling
cacheFilter.test(key) so each key is tested from the start; reference the
shouldDelete variable and cacheFilter RegExp usage in forceRefresh when making
the change.
---
Outside diff comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 333-362: The message handler currently returns Promises but must
use e.waitUntil(...) to keep the service worker alive; wrap each async branch so
that SKIP_WAITING uses e.waitUntil(self.skipWaiting().then(() =>
self.clients.claim()).then(() => deleteOldCaches()).then(() =>
sendMessage('WAITING_SKIPPED'))), CLAIM_CLIENTS uses
e.waitUntil(self.clients.claim().then(() => deleteOldCaches()).then(() =>
e.source?.postMessage('CLIENTS_CLAIMED'))), BLAZOR_STARTED uses
e.waitUntil(createAssetsCache(true)), and CLEAN_UP uses
e.waitUntil(deleteOldCaches()), ensuring all calls to self.skipWaiting(),
self.clients.claim(), deleteOldCaches(), createAssetsCache(true), sendMessage
and e.source?.postMessage are inside the waitUntil chains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3c76fdc-5fec-4833-85fc-89e300ee0dd0
📒 Files selected for processing (8)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/Bswup/README.md (1)
105-110: ⚡ Quick winConsider showing complete error details in the example.
The comment documents that
data.urlanddata.hashare available "when known", but the example only logsdata.reasonanddata.message. For a more complete reference example, consider showing all available fields.📝 Suggested enhancement
case BswupMessage.error: // Structured install failure. data.reason is one of 'manifest' | 'integrity' | // 'fetch' | 'cache' | 'request' | 'install-incomplete'; data.message is human // readable, and data.url / data.hash point at the offending asset when known. - console.error('Bswup install error:', data.reason, data.message); + console.error('Bswup install error:', data.reason, data.message, data.url, data.hash); return;🤖 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 `@src/Bswup/README.md` around lines 105 - 110, The example logging for the BswupMessage.error case only prints data.reason and data.message; update the handler for the BswupMessage.error branch (the switch case) to log all available error details (data.reason, data.message, and when present data.url and data.hash, plus the whole data object for unknown fields) so consumers see the complete failure context; use the existing BswupMessage.error case and data variable to append url and hash if defined and include a fallback of logging the entire data object.
🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts`:
- Around line 129-149: The retry suppression omitted the 'install-incomplete'
reason so deterministic failures can still show the retry button; update the
nonRetriableReasons used where errorRetryButton is handled (the
nonRetriableReasons array and the isRetriable check in the same block) to
include 'install-incomplete' alongside 'manifest' and 'integrity' so isRetriable
becomes false for that reason, keep the existing behavior for invoking
data.reload() or window.location.reload() unchanged.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 388-425: The extract() function currently assigns options =
(window[optionsName] || defaultoptions) which replaces defaults when a partial
window[optionsName] exists; change it to merge defaults with any global config
(e.g. create options by combining defaultoptions and window[optionsName] such as
Object.assign({}, defaultoptions, window[optionsName]) or using the spread
operator) so missing keys in window[optionsName] fall back to defaultoptions;
keep the subsequent attribute overrides (log, sw, scope, handlerName,
blazorScript, updateInterval, updateOnVisibility) applied on top of the merged
options.
- Around line 427-438: The current handle function permanently replaces
options.handler with a noop when window[options.handlerName] is missing,
preventing later attachment from being picked up; change it so you do not
overwrite options.handler on missing handler — instead read
window[options.handlerName] into a local variable (e.g., foundHandler) or only
assign to options.handler when a real function is present, call that local/found
handler if available, and leave options.handler untouched (so future
registrations on window[options.handlerName] can be discovered); update the
logic in the handle function that references options.handlerName and
options.handler to reflect this non-destructive check.
In `@src/Bswup/Bit.Bswup/Styles/bit-bswup.progress.css`:
- Around line 74-89: Replace the deprecated word-break: break-word in the
.bit-bswup-error-message and .bit-bswup-error-details selectors by removing
those declarations and adding overflow-wrap: anywhere (leave existing
white-space: pre-wrap intact) so the stylesheet no longer triggers the Stylelint
warning.
In `@src/Bswup/README.md`:
- Line 218: Update the README entry for the `FullOffline` mode to correct the
grammar by changing "from first time the app is loaded" to "from the first time
the app is loaded" so the description reads: "Enables the full offline mode
where all assets are cached and served from the cache from the first time the
app is loaded."
- Around line 228-234: Update the "Polling for updates" paragraph to fix the
numeric inconsistency: change "There are three ways to check more often:" to
"There are two ways to check more often:" (since only the built-in options using
updateInterval/updateOnVisibility and calling BitBswup.checkForUpdate() are
listed), ensuring the section header "Polling for updates" and references to
updateInterval, updateOnVisibility and BitBswup.checkForUpdate() remain
unchanged.
- Line 213: The README wording incorrectly states that per-asset ?v= and SRI
derive from the manifest version; update the text to accurately reflect that
CACHE_VERSION / cacheVersion only affect the cache bucket name (computed into
CACHE_NAME) while per-asset cache-buster and integrity are set inside
createNewAssetRequest() using each asset's asset.hash (with fallback to
self.assetsManifest.version) and requestInit.integrity = asset.hash; replace the
sentence to mention that ?v= primarily comes from asset.hash (fallback to
manifest.version) and SRI uses asset.hash, and remove any implication that those
values reference self.cacheVersion or CACHE_VERSION.
---
Nitpick comments:
In `@src/Bswup/README.md`:
- Around line 105-110: The example logging for the BswupMessage.error case only
prints data.reason and data.message; update the handler for the
BswupMessage.error branch (the switch case) to log all available error details
(data.reason, data.message, and when present data.url and data.hash, plus the
whole data object for unknown fields) so consumers see the complete failure
context; use the existing BswupMessage.error case and data variable to append
url and hash if defined and include a fallback of logging the entire data
object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 427f1025-c5ef-456e-b74d-6a2168e132d7
📒 Files selected for processing (9)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts (1)
21-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait the activation/message chain so
installactually waits for teardown signaling.
removeBswup()isawaited bywaitUntil, but theskipWaiting().then(...matchAll...)chain is fire-and-forget. On Line 21 this allows the install promise to resolve before client teardown signaling runs.Suggested fix
- self.skipWaiting().then(() => self.clients - .matchAll({ includeUncontrolled: true }) - .then(clients => (clients || []).forEach(client => { - client.postMessage('UNREGISTER'); - setTimeout(() => client.postMessage('WAITING_SKIPPED'), 1000); - }))); + await self.skipWaiting(); + const clients = await self.clients.matchAll({ includeUncontrolled: true }); + (clients || []).forEach(client => { + client.postMessage('UNREGISTER'); + setTimeout(() => client.postMessage('WAITING_SKIPPED'), 1000); + });🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts` around lines 21 - 26, The skipWaiting().then(...) promise chain in the install event handler is fire-and-forget and not being awaited, allowing the install event to resolve before client teardown signaling completes. Ensure the entire promise chain starting with skipWaiting() and continuing through the matchAll() and client postMessage calls is properly awaited or passed to waitUntil() so that the install event waits for all teardown signaling to finish before resolving.
♻️ Duplicate comments (3)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts (1)
85-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope
downloadProgressdeclarations inside a block to satisfy switch-clause correctness.Lines 102-103 declare
constat switch-clause scope; Biome flags this (noSwitchDeclarations). Wrap this case body in braces.Suggested fix
- case BswupMessage.downloadProgress: + case BswupMessage.downloadProgress: { hideApp_ && appEl && (appEl.style.display = 'none'); bswupEl && (bswupEl.style.display = 'block'); @@ progressEl && progressEl.setAttribute('aria-valuenow', String(percent)); percentEl && (percentEl.textContent = `${percent}%`); return showLogs_ ? console.log('asset downloaded:', data) : undefined; + }🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts` around lines 85 - 112, The downloadProgress case in the switch statement has const declarations at the switch-clause scope without being wrapped in braces, which violates the noSwitchDeclarations rule. Wrap the entire body of the downloadProgress case (starting after the case label and ending before the return statement) in curly braces to create a proper block scope for the const declarations percent and perStr, allowing them to be safely declared at the clause level.Source: Linters/SAST tools
src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts (1)
273-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle rejected
startBlazor(true)inCLIENTS_CLAIMEDto avoid hanging first-install reload.On Line 274, a rejected
startPromiseskipsonStarted(), soblazorStartResolveris never released and the first-installreload()path can stall indefinitely.Suggested fix
- if (startPromise) { - startPromise.then(onStarted); + if (startPromise) { + startPromise + .then(onStarted) + .catch((err) => { + error('Blazor.start() failed after clients claimed', err); + onStarted(); + }); } else {🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts` around lines 273 - 275, The code on line 274 only handles the successful resolution of startPromise via the then(onStarted) callback, but does not handle rejection cases. When startPromise rejects (e.g., if startBlazor(true) fails), onStarted() is never called, leaving blazorStartResolver unresolved and causing the first-install reload path to hang indefinitely. Add error handling to the startPromise chain in the CLIENTS_CLAIMED section by attaching a rejection handler (either as a second parameter to then() or via a separate catch() call) that ensures blazorStartResolver is properly released even when the promise fails, preventing the reload from stalling.src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (1)
12-15:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAugment
WorkerGlobalScope, notWindow.This file’s own comment says
selfis typed asWorkerGlobalScope; declaring the customself.*fields onWindowwon’t type the service-worker global correctly, andimportScriptsshould be left to the WebWorker lib.Proposed fix
-interface Window { - clients: any - skipWaiting: any - importScripts: any +interface WorkerGlobalScope { + clients: Clients + skipWaiting(): Promise<void> assetsManifest: any // injected by service-worker-assets.js (version + asset list)Please verify with the Bswup TypeScript build after this change; expected result is no
self.assetsManifest/custom-config property errors and no duplicateimportScriptsdeclaration error.🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts` around lines 12 - 15, The interface augmentation at lines 12-15 is incorrectly declaring custom service-worker properties on the `Window` interface instead of `WorkerGlobalScope`. Change the interface name from `Window` to `WorkerGlobalScope` to properly type the `self` global object in this service-worker context. Additionally, remove the `importScripts` property from this interface declaration since it should already be provided by the WebWorker library types and declaring it here causes duplicate declaration conflicts.
🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 929-963: The applyUrlCaseSensitivity function must strip the g
(global) and y (sticky) flags from RegExp patterns because these flags cause
test() to mutate lastIndex, creating intermittent security bypasses when
patterns are reused across requests. Modify applyUrlCaseSensitivity to filter
out g and y flags when reconstructing the RegExp, preserving only other safe
flags and conditionally adding the i flag when caseInsensitiveUrl is enabled.
Ensure the function strips these dangerous flags regardless of whether
case-insensitivity is enabled, as this is a security-critical operation for
stored patterns like PROHIBITED_URLS.
In `@src/Bswup/README.md`:
- Line 213: Fix the grammar in the `disableHashlessAssetsUpdate` option
description in the README.md file. The phrase "every time an update found for
the app" is missing the auxiliary verb and should be changed to "every time an
update is found for the app". Additionally, simplify "the update of the
hash-less assets" to "the update of hash-less assets" by removing the first
article "the" for better conciseness. Make these two edits to improve the
clarity and grammatical correctness of the documentation.
---
Outside diff comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 21-26: The skipWaiting().then(...) promise chain in the install
event handler is fire-and-forget and not being awaited, allowing the install
event to resolve before client teardown signaling completes. Ensure the entire
promise chain starting with skipWaiting() and continuing through the matchAll()
and client postMessage calls is properly awaited or passed to waitUntil() so
that the install event waits for all teardown signaling to finish before
resolving.
---
Duplicate comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts`:
- Around line 85-112: The downloadProgress case in the switch statement has
const declarations at the switch-clause scope without being wrapped in braces,
which violates the noSwitchDeclarations rule. Wrap the entire body of the
downloadProgress case (starting after the case label and ending before the
return statement) in curly braces to create a proper block scope for the const
declarations percent and perStr, allowing them to be safely declared at the
clause level.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 12-15: The interface augmentation at lines 12-15 is incorrectly
declaring custom service-worker properties on the `Window` interface instead of
`WorkerGlobalScope`. Change the interface name from `Window` to
`WorkerGlobalScope` to properly type the `self` global object in this
service-worker context. Additionally, remove the `importScripts` property from
this interface declaration since it should already be provided by the WebWorker
library types and declaring it here causes duplicate declaration conflicts.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 273-275: The code on line 274 only handles the successful
resolution of startPromise via the then(onStarted) callback, but does not handle
rejection cases. When startPromise rejects (e.g., if startBlazor(true) fails),
onStarted() is never called, leaving blazorStartResolver unresolved and causing
the first-install reload path to hang indefinitely. Add error handling to the
startPromise chain in the CLIENTS_CLAIMED section by attaching a rejection
handler (either as a second parameter to then() or via a separate catch() call)
that ensures blazorStartResolver is properly released even when the promise
fails, preventing the reload from stalling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20130ae1-212b-4519-add5-62e3a74807ff
📒 Files selected for processing (12)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/Bit.Bswup.csprojsrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/Bit.Bswup/tsconfig.jsonsrc/Bswup/Bit.Bswup/tsconfig.sw.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Bswup/README.md (1)
208-216: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor style improvement for line 213.
The phrase "all of the hash-less assets" is slightly redundant; consider simplifying to "all hash-less assets" for better conciseness.
Suggested rewording
-- `disableHashlessAssetsUpdate`: Disables the update of hash-less assets. By default, the Bswup tries to automatically update all of the hash-less assets (e.g. the external assets) every time an update is found for the app. +- `disableHashlessAssetsUpdate`: Disables the update of hash-less assets. By default, Bswup automatically updates all hash-less assets (e.g. the external assets) every time an update is found for the app.This also simplifies "the Bswup tries to" to "Bswup" for consistency with the rest of the documentation.
🤖 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 `@src/Bswup/README.md` around lines 208 - 216, The `disableHashlessAssetsUpdate` option description contains redundant phrasing that should be simplified for better conciseness. In the disableHashlessAssetsUpdate documentation line, change "the Bswup tries to automatically update all of the hash-less assets" to "Bswup automatically updates all hash-less assets". This removes the redundant article "the" before Bswup, the unnecessary phrase "tries to", and simplifies "all of the" to "all" to match the documentation style used elsewhere.src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts (1)
3-6: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winThe
Windowinterface name is misleading in a service worker context.Service workers run in a
ServiceWorkerGlobalScope, not aWindowcontext. While this typing workaround functions correctly, the interface name is semantically incorrect and could confuse future maintainers.♻️ Suggested improvement
Consider either properly extending the service worker global scope types or using a more accurate interface name:
-interface Window { +interface ServiceWorkerGlobalScopeExtension { clients: any skipWaiting: any }And optionally use
ExtendableEventfor better type safety on line 13:-self.addEventListener('install', (e: any) => e.waitUntil(removeBswup())); +self.addEventListener('install', (e: ExtendableEvent) => e.waitUntil(removeBswup()));Also applies to: 13-13
🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts` around lines 3 - 6, The interface named Window at the top of the service worker script is semantically incorrect because service workers execute in a ServiceWorkerGlobalScope context, not a Window context. Rename the Window interface to a more accurate name that reflects the service worker context (such as ServiceWorkerGlobalScope or ServiceWorkerGlobal), or properly extend the actual ServiceWorkerGlobalScope type. Additionally, consider using ExtendableEvent type for better type safety on the event handling to properly reflect service worker semantics.
🤖 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 `@src/Bswup/Bit.Bswup/Bit.Bswup.csproj`:
- Around line 47-52: The BuildJavaScript target's Inputs attribute only includes
TypeScript source files (.ts files) but omits the TypeScript configuration files
that control compilation. To fix this, add tsconfig.json and tsconfig.sw.json to
the Inputs attribute of the BuildJavaScript Target element so that changes to
either configuration file will trigger a rebuild and prevent stale JavaScript
artifacts from remaining in the wwwroot directory.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 17-33: The delayed setTimeout callbacks in the removeBswup()
function that send WAITING_SKIPPED messages are not awaited, which means the
service worker may be terminated before these callbacks execute. Wrap each
setTimeout call in a Promise that resolves after the delay completes, collect
all these promises into an array, and await Promise.all() on that array before
the function returns. This ensures the install event's waitUntil() keeps the
worker alive until all delayed messages have been scheduled for dispatch.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 129-135: The condition checking `reg.active` on line 133 sends the
CLAIM_CLIENTS message regardless of whether this is a first install or an
update-error retry. Modify the condition to check if `reg.waiting` exists
(indicating first install) before posting the CLAIM_CLIENTS message and
returning the promise. For the else branch when `reg.waiting` is absent
(update/error retry case), force a navigation reload using
window.location.reload() to re-attempt the installation rather than having the
retry do nothing.
---
Nitpick comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 3-6: The interface named Window at the top of the service worker
script is semantically incorrect because service workers execute in a
ServiceWorkerGlobalScope context, not a Window context. Rename the Window
interface to a more accurate name that reflects the service worker context (such
as ServiceWorkerGlobalScope or ServiceWorkerGlobal), or properly extend the
actual ServiceWorkerGlobalScope type. Additionally, consider using
ExtendableEvent type for better type safety on the event handling to properly
reflect service worker semantics.
In `@src/Bswup/README.md`:
- Around line 208-216: The `disableHashlessAssetsUpdate` option description
contains redundant phrasing that should be simplified for better conciseness. In
the disableHashlessAssetsUpdate documentation line, change "the Bswup tries to
automatically update all of the hash-less assets" to "Bswup automatically
updates all hash-less assets". This removes the redundant article "the" before
Bswup, the unnecessary phrase "tries to", and simplifies "all of the" to "all"
to match the documentation style used elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48ea459f-6cee-4ed2-a299-590fb11feea2
📒 Files selected for processing (12)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/Bit.Bswup.csprojsrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/Bit.Bswup/tsconfig.jsonsrc/Bswup/Bit.Bswup/tsconfig.sw.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts (1)
49-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-resolve the optional user handler until it actually exists.
Line 50 captures
window[handler]once duringstart(). If that global is registered later than this auto-start, every subsequent Bswup event skips the custom handler forever. Resolve it insidebitBswupHandler()(or only cache once a real function is found) so normal script-order races do not break the handler contract.🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts` around lines 49 - 60, The optional user handler is being captured too early in start(), so late-registered globals never get invoked. Update bitBswupHandler in bit-bswup.progress.ts to resolve window[handler] on each message (or cache it only after confirming it is a real function) so the custom handler still runs when script order is racey.
♻️ Duplicate comments (3)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (3)
297-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the documented prohibited-URL status code.
This still returns
403, while the prior Bswup contract called for405. If consumers/tests rely on “method not allowed,” this is a behavior break.Proposed fix
return new Response('This URL is prohibited!', { - status: 403, - statusText: 'Prohibited', + status: 405, + statusText: 'Method Not Allowed', headers: { 'Content-Type': 'text/plain; charset=utf-8' } });🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts` around lines 297 - 300, The prohibited-URL response in the Bswup script still uses the wrong HTTP status and breaks the existing contract. Update the response returned by the prohibited-URL branch in bit-bswup.sw.ts so it preserves the documented “method not allowed” behavior expected by consumers/tests, keeping the same Response construction but using the contractually required status in the status/statusText fields.
96-104: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
VERSIONin the cache bucket identity.
cacheVersionalone lets different asset manifests share one cache. A waiting worker can then delete/rewrite entries in the cache the active worker is still serving, breaking atomic installs. Include both values in the bucket name.Proposed fix
-const CACHE_NAME = `${CACHE_NAME_PREFIX} - ${CACHE_VERSION}`; +const CACHE_NAME = `${CACHE_NAME_PREFIX} - ${VERSION} - ${CACHE_VERSION}`;🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts` around lines 96 - 104, The cache bucket identity in bit-bswup.sw.ts currently uses only cacheVersion/CACHE_VERSION, which can let different asset manifests collide in the same cache and break atomic worker installs. Update the CACHE_NAME construction to incorporate both VERSION and any cacheVersion override, and keep the logic in the same area around CACHE_VERSION and CACHE_NAME so the cache bucket remains unique per manifest while still allowing an override.
205-207: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport lax background cache infrastructure failures.
This catch prevents an unhandled rejection, but
diagis silent unless diagnostics are enabled. Ifcaches.open()/storage setup fails, the progress UI never receives the structured error this PR is adding.Proposed fix
createAssetsCache().catch(err => { diag('*** lax install - background createAssetsCache failed (assets will lazy-fill):', err); + sendError({ + reason: 'cache', + message: 'Background cache build failed: ' + (err && (err as any).message || String(err)), + }); });🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts` around lines 205 - 207, The background createAssetsCache() failure is only being sent to diag, so the progress UI never sees a structured cache-initialization error. Update the catch on the createAssetsCache() promise in bit-bswup.sw.ts to route the failure through the same structured reporting path used by the new progress UI/error plumbing, while still preserving the existing fallback behavior for lax installs. Ensure the handling references createAssetsCache and the surrounding install flow so storage/caches.open() failures are surfaced even when diagnostics are disabled.
🤖 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 `@src/Bswup/Bit.Bswup/Bit.Bswup.csproj`:
- Around line 30-37: The restore invalidation for InstallNodejsDependencies only
tracks package.json, so lockfile-only changes can leave stale node_modules on
incremental builds. Update the dependency-check logic in the
BeforeBuildTasks/InstallNodejsDependencies flow to also consider
package-lock.json (and any other lockfiles already included by the project), so
a lockfile change triggers npm install before the tsc/esbuild steps run.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts`:
- Around line 220-250: The auto-start logic in autoStart only runs once and
returns permanently when `#bit-bswup` is missing, so late-rendered components
never initialize. Update autoStart in bit-bswup.progress.ts to retry
initialization when the element appears, such as by adding a MutationObserver or
another DOM-ready/lifecycle hook, and ensure start is eventually called so
window.bitBswupHandler gets set for the BitBswupProgress flow.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 33-42: The cleanup worker is sending WAITING_SKIPPED during
install, which blocks activation and makes the main script treat it like a
post-activation reload signal. Update bit-bswup.sw-cleanup’s delayed message
path and the corresponding handling in bit-bswup.ts so the fallback runs from an
activate-time flow or uses a separate message name that does not imply
activation. Keep the waitUntil/promise logic aligned with the new symbol and
ensure the reload signal cannot be processed against the stale registration.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 395-415: The message handler in bit-bswup.sw.ts is returning
promises instead of extending the service worker lifetime, so the SKIP_WAITING
and CLAIM_CLIENTS flows can be interrupted before deleteOldCaches() or the
completion message runs. Update the message listener to treat the event as an
ExtendableMessageEvent and wrap each async chain in e.waitUntil(...), keeping
the existing SKIP_WAITING, CLAIM_CLIENTS, deleteOldCaches(), and
sendMessage()/postMessage() logic intact.
- Around line 12-42: The ambient declarations are attached to Window, but this
script runs in a worker context, so the missing types on self are not being
fixed correctly. Move the custom members from the Window interface to
WorkerGlobalScope in bit-bswup.sw.ts, and remove the duplicate importScripts
declaration since it already exists on the worker global type. Keep the
asset/config fields grouped there so self.* is properly typed.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 632-634: The forceRefresh() cleanup currently unregisters every
same-origin service worker via navigator.serviceWorker.getRegistrations(), which
can affect unrelated apps. Update the logic in forceRefresh() to target only
Bswup’s own registration by using the current registration or filtering by this
app’s scope/script, and keep the unregister call limited to that matching
registration instead of iterating over all registrations.
- Around line 273-287: The Blazor startup handling in bit-bswup.ts is
incorrectly treating both a rejected startPromise and a missing startPromise as
success by calling onStarted(), which resolves the reload flow and posts
BLAZOR_STARTED. Update the startPromise rejection branch and the
undefined/misconfigured-script branch to follow a real failure path instead of
invoking onStarted(), while keeping the success path limited to the actual
Blazor.start()/onStarted sequence. Use the existing startPromise, onStarted,
error, and BLAZOR_STARTED handling in bit-bswup.ts to locate and split the
success vs failure behavior.
---
Outside diff comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts`:
- Around line 49-60: The optional user handler is being captured too early in
start(), so late-registered globals never get invoked. Update bitBswupHandler in
bit-bswup.progress.ts to resolve window[handler] on each message (or cache it
only after confirming it is a real function) so the custom handler still runs
when script order is racey.
---
Duplicate comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 297-300: The prohibited-URL response in the Bswup script still
uses the wrong HTTP status and breaks the existing contract. Update the response
returned by the prohibited-URL branch in bit-bswup.sw.ts so it preserves the
documented “method not allowed” behavior expected by consumers/tests, keeping
the same Response construction but using the contractually required status in
the status/statusText fields.
- Around line 96-104: The cache bucket identity in bit-bswup.sw.ts currently
uses only cacheVersion/CACHE_VERSION, which can let different asset manifests
collide in the same cache and break atomic worker installs. Update the
CACHE_NAME construction to incorporate both VERSION and any cacheVersion
override, and keep the logic in the same area around CACHE_VERSION and
CACHE_NAME so the cache bucket remains unique per manifest while still allowing
an override.
- Around line 205-207: The background createAssetsCache() failure is only being
sent to diag, so the progress UI never sees a structured cache-initialization
error. Update the catch on the createAssetsCache() promise in bit-bswup.sw.ts to
route the failure through the same structured reporting path used by the new
progress UI/error plumbing, while still preserving the existing fallback
behavior for lax installs. Ensure the handling references createAssetsCache and
the surrounding install flow so storage/caches.open() failures are surfaced even
when diagnostics are disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ed5fd88-a4c6-4cdb-8fe3-68bc74560a15
📒 Files selected for processing (12)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/Bit.Bswup.csprojsrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/Bit.Bswup/tsconfig.jsonsrc/Bswup/Bit.Bswup/tsconfig.sw.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts (1)
28-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
WAITING_SKIPPEDis still sent before this worker can activate.Because
removeBswup()is awaited from the install event,await Promise.all(reloadSignals)keeps installation pending until after the delayed fallback fires.src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts:246-253treatsWAITING_SKIPPEDas a post-activation reload signal, so tabs can still reload against the stale registration being torn down. This still needs to move to an activate-time path or use a different message.🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts` around lines 28 - 42, The cleanup worker is still emitting WAITING_SKIPPED during install, which means the reload signal can fire before activation completes. Update the sw-cleanup flow around self.skipWaiting(), clients.matchAll(), and the delayed postMessage calls so the signal is sent from an activate-time path or replaced with a different message that bit-bswup.ts handles after activation. Keep the identifiers WAITING_SKIPPED, removeBswup, and the reloadSignals Promise chain in mind when relocating this behavior.
🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts`:
- Around line 231-252: The initialization latch is set too early in autoStart(),
which can block retries after a failed start() and can also leave manual
BitBswupProgress.start(...) calls untracked. Move the data-bit-bswup-initialized
flag update into the successful path of start() (or a shared post-start success
path) so it is only marked after initialization completes, and ensure
autoStart() no longer sets the flag before calling start().
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 651-653: The retry classification in bit-bswup.sw.ts is treating a
generic network failure as an integrity error because the isIntegrity check in
the fetch error handling includes ERR_FAILED. Update the logic in the fetch
retry path to only match true integrity-related signals (for example integrity,
digest, or EPRPROTO) and remove ERR_FAILED from that pattern so transient fetch
failures still retry when integrity is enabled.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 291-295: The UNREGISTER handler in bit-bswup.ts is unregistering
every same-origin service worker via navigator.serviceWorker.getRegistrations(),
which can affect other apps. Update the e.data === 'UNREGISTER' branch in the
service worker message handling logic to match the reset path by using
navigator.serviceWorker.getRegistration() and only unregister the registration
for this app. Keep the window.location.reload() behavior after the targeted
unregister completes.
---
Duplicate comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 28-42: The cleanup worker is still emitting WAITING_SKIPPED during
install, which means the reload signal can fire before activation completes.
Update the sw-cleanup flow around self.skipWaiting(), clients.matchAll(), and
the delayed postMessage calls so the signal is sent from an activate-time path
or replaced with a different message that bit-bswup.ts handles after activation.
Keep the identifiers WAITING_SKIPPED, removeBswup, and the reloadSignals Promise
chain in mind when relocating this behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bca4e4fe-9a2f-4e0d-9432-aa20d5d6c30a
📒 Files selected for processing (12)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/Bit.Bswup.csprojsrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/Bit.Bswup/tsconfig.jsonsrc/Bswup/Bit.Bswup/tsconfig.sw.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (1)
430-435: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExtend the
messageevent lifetime for the async cache work. Insrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts:431-435,BLAZOR_STARTEDandCLEAN_UPfire detached promises; wrap them ine.waitUntil(...)so the worker stays alive untilcreateAssetsCache(true)/deleteOldCaches()settle, and keep the error handling on the cleanup/top-up paths.🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts` around lines 430 - 435, The `message` handler in `bit-bswup.sw.ts` is starting async cache work without extending the event lifetime. Update the `BLAZOR_STARTED` and `CLEAN_UP` branches inside the `message` event listener to wrap `createAssetsCache(true)` and `deleteOldCaches()` with `e.waitUntil(...)`, so the service worker stays alive until those promises settle. Keep the existing error handling/logging around the cleanup/top-up flows when making this change.
🤖 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 `@src/Bswup/Bit.Bswup/Bit.Bswup.csproj`:
- Around line 47-52: The BuildJavaScript target is only tracking TypeScript
sources and tsconfig files, so changes to the node toolchain can leave stale
wwwroot outputs in incremental builds. Update the Inputs on BuildJavaScript to
also include the dependency manifests used by InstallNodejsDependencies,
especially package.json and package-lock.json, so a toolchain upgrade forces
recompilation through the existing tsc and tsc -p tsconfig.sw.json steps.
---
Outside diff comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 430-435: The `message` handler in `bit-bswup.sw.ts` is starting
async cache work without extending the event lifetime. Update the
`BLAZOR_STARTED` and `CLEAN_UP` branches inside the `message` event listener to
wrap `createAssetsCache(true)` and `deleteOldCaches()` with `e.waitUntil(...)`,
so the service worker stays alive until those promises settle. Keep the existing
error handling/logging around the cleanup/top-up flows when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a00b14d1-7bb2-4de7-801d-d8ef62667c00
📒 Files selected for processing (12)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/Bit.Bswup.csprojsrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/Bit.Bswup/tsconfig.jsonsrc/Bswup/Bit.Bswup/tsconfig.sw.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts (1)
125-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle failed
data.reload()calls in the auto-reload path.Both auto-reload branches assume
data.reload()succeeds. If it rejects,downloadFinishedcan leave the app hidden with the splash stuck (defaultAutoHideis false), andupdateReadygives the user no manual fallback. Catch the failure and restore/show the manual reload path instead of only cleaning up in.then().Proposed fix
- if (autoReload_ || data.firstInstall) { - data.reload().then(() => { - hideApp_ && appEl && (appEl.style.display = appElOriginalDisplay); - bswupEl && (bswupEl.style.display = 'none'); - }); + if (autoReload_ || data.firstInstall) { + Promise.resolve(data.reload()) + .then(() => { + hideApp_ && appEl && (appEl.style.display = appElOriginalDisplay); + bswupEl && (bswupEl.style.display = 'none'); + }) + .catch((err) => { + console.error('BitBswup reload failed:', err); + hideApp_ && appEl && (appEl.style.display = appElOriginalDisplay); + reloadButton && (reloadButton.style.display = 'block'); + reloadButton && (reloadButton.onclick = data.reload); + }); } else { reloadButton && (reloadButton.style.display = 'block'); reloadButton && (reloadButton.onclick = data.reload); @@ - if (autoReload_) { - data.reload(); + if (autoReload_) { + Promise.resolve(data.reload()).catch((err) => { + console.error('BitBswup reload failed:', err); + reloadButton && (reloadButton.style.display = 'inline'); + reloadButton && (reloadButton.onclick = data.reload); + }); } else {🤖 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 `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts` around lines 125 - 149, Handle failed data.reload() in both the downloadFinished and updateReady branches of bit-bswup.progress.ts. The current auto-reload path only handles success, so if data.reload() rejects the UI can stay hidden and no manual retry is offered. Update the logic around data.reload() to catch rejections, restore the app/splash visibility cleanup in the failure path, and fall back to showing the reloadButton with its onclick wired to data.reload so users can retry manually.
🧹 Nitpick comments (2)
src/Bswup/README.md (2)
225-229: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCorrect
forceRefreshunregister scope.Line 229 says "unregisters all service workers" but the implementation only unregisters the service worker whose scope controls the current page (
navigator.serviceWorker.getRegistration()), not every same-origin registration. Update the wording to match the actual behavior.- `BitBswup.forceRefresh(cacheFilter?)`: Clears caches, unregisters all service workers, and reloads the page. + `BitBswup.forceRefresh(cacheFilter?)`: Clears caches, unregisters the app's service worker, and reloads the page.🤖 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 `@src/Bswup/README.md` around lines 225 - 229, The `BitBswup.forceRefresh` docs overstate unregister scope by saying it “unregisters all service workers,” but the implementation only unregisters the current page’s controlling registration via `navigator.serviceWorker.getRegistration()`. Update the `forceRefresh(cacheFilter?)` description in the README to match the actual behavior and make it clear it only removes the active registration for the current scope, not every same-origin service worker.
227-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
updateCheckFailedevent.When
BitBswup.checkForUpdate()encounters a transient failure (offline, server hiccup), the runtime emitsupdateCheckFailedrather thanerrorso the default progress UI doesn't hide the app. Add this case to the handler example and mention it in the API description so callers can show a "check failed" message.case BswupMessage.updateNotFound: return console.log('checked for an update, already on the latest version.'); + + case BswupMessage.updateCheckFailed: + return console.warn('update check failed:', data.message);🤖 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 `@src/Bswup/README.md` at line 227, The README entry for BitBswup.checkForUpdate() is missing the updateCheckFailed event handling. Update the API description and the handler example to mention that transient check failures emit updateCheckFailed instead of error, and show how callers should handle it in the same update-flow documentation alongside updateFound, stateChanged, updateReady, downloadFinished, and updateNotFound.
🤖 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 `@src/Bswup/Bit.Bswup/Bit.Bswup.csproj`:
- Around line 13-14: The BswupAssetsBuildTfm logic in Bit.Bswup.csproj is pinned
to the first TargetFrameworks entry, which breaks single-framework builds
targeting a non-first TFM. Update the MSBuild property so
ResolveStaticWebAssetsInputsDependsOn and related asset-generation hooks use
$(TargetFramework) whenever it is already set, and only fall back to the first
TargetFrameworks value for cross-targeting builds. Keep the fix aligned with the
existing BswupAssetsBuildTfm condition so JS/CSS generation still runs for
dotnet build/publish -f scenarios.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 42-46: The cleanup logic in the service worker is broadcasting
UNREGISTER too broadly because `clients.matchAll({ includeUncontrolled: true })`
returns every same-origin client. Update the cleanup flow in `sw-cleanup.ts` to
filter the matched clients to only window clients whose URL falls under
`self.registration.scope` before calling `client.postMessage('UNREGISTER')`, so
the `reloadSignals` loop only targets the current registration.
---
Outside diff comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.ts`:
- Around line 125-149: Handle failed data.reload() in both the downloadFinished
and updateReady branches of bit-bswup.progress.ts. The current auto-reload path
only handles success, so if data.reload() rejects the UI can stay hidden and no
manual retry is offered. Update the logic around data.reload() to catch
rejections, restore the app/splash visibility cleanup in the failure path, and
fall back to showing the reloadButton with its onclick wired to data.reload so
users can retry manually.
---
Nitpick comments:
In `@src/Bswup/README.md`:
- Around line 225-229: The `BitBswup.forceRefresh` docs overstate unregister
scope by saying it “unregisters all service workers,” but the implementation
only unregisters the current page’s controlling registration via
`navigator.serviceWorker.getRegistration()`. Update the
`forceRefresh(cacheFilter?)` description in the README to match the actual
behavior and make it clear it only removes the active registration for the
current scope, not every same-origin service worker.
- Line 227: The README entry for BitBswup.checkForUpdate() is missing the
updateCheckFailed event handling. Update the API description and the handler
example to mention that transient check failures emit updateCheckFailed instead
of error, and show how callers should handle it in the same update-flow
documentation alongside updateFound, stateChanged, updateReady,
downloadFinished, and updateNotFound.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0728f6d6-dbdc-4ad2-928d-fc0f9f8e24df
📒 Files selected for processing (12)
src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup/Bit.Bswup.csprojsrc/Bswup/Bit.Bswup/BswupProgress.razorsrc/Bswup/Bit.Bswup/Scripts/bit-bswup.progress.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.tssrc/Bswup/Bit.Bswup/Scripts/bit-bswup.tssrc/Bswup/Bit.Bswup/Styles/bit-bswup.progress.csssrc/Bswup/Bit.Bswup/tsconfig.jsonsrc/Bswup/Bit.Bswup/tsconfig.sw.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (1)
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
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
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.
closes #12408
Summary by CodeRabbit
showAssets.updateInterval,updateOnVisibility) plus new update/error events (updateNotFound,updateCheckFailed,error) and improvedforceRefreshfiltering.