Apply Bswup improvements (#12408)#12418
Open
msynk wants to merge 17 commits into
Open
Conversation
|
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 an install-failure UI and retry flow; fail-soft and validate service-worker manifests; rework asset install with retries/backoff and strict/lax modes; change passive fetch to async cache writes and explicit 403 for prohibited URLs; add registration-aware update checks, polling, guarded reloads, expanded options/logging, demo/published tweaks, build configuration, and README updates. ChangesBit.Bswup Service Worker Error Handling & Update Polling
Sequence Diagram(s)sequenceDiagram
participant SW as ServiceWorker
participant Page as BswupProgress
participant UI as Error Panel
SW->>Page: postMessage({type: "error", reason, message, url, hash})
Page->>UI: show panel, populate message/details from data
UI->>Page: Retry clicked (if reason permits retry)
Page->>SW: invoke data.reload() or window.location.reload()
flowchart TD
A["importScripts service-worker-assets.js"] -->|try/catch| B["validateAssetsManifest"]
B -->|invalid| C["sendError + normalizeAssetsManifest"]
B -->|valid| D["set MANIFEST_VALID = true"]
C --> E["prevent module crashes"]
E --> D
D -->|false| F["skip asset enumeration"]
D -->|true| G["createAssetsCache: warm-start + reconcile + install"]
G -->|strict mode| H["await + throw on failures"]
G -->|lax mode| I["fire-and-forget + log failures"]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 `@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 |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts (2)
237-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
Blazor.start()failure inCLIENTS_CLAIMEDto avoid hanging first-install flow.On Line 238,
Blazor.start()is invoked without a rejection path. If it rejects,blazorStartResolveris never resolved and callers awaitingdata.reload()can stall indefinitely.Suggested fix
if (e.data === 'CLIENTS_CLAIMED') { Blazor.start().then(() => { blazorStartResolver?.(undefined); - e.source.postMessage('BLAZOR_STARTED'); - }); + e.source?.postMessage('BLAZOR_STARTED'); + }).catch((err) => { + error('Blazor start failed after CLIENTS_CLAIMED', err); + blazorStartResolver?.(undefined); + }); 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/Bit.Bswup/Scripts/bit-bswup.ts` around lines 237 - 241, Blazor.start() is called in the CLIENTS_CLAIMED handler without a rejection path, which can leave blazorStartResolver unresolved and stall callers (e.g., data.reload()); update the handler around Blazor.start() so that both success and failure paths always call blazorStartResolver (resolve or reject as appropriate) and always post back to e.source (e.g., send a success or failure message like 'BLAZOR_STARTED' vs 'BLAZOR_START_FAILED'), ensuring Blazor.start().then(...) has a corresponding .catch(...) that resolves/rejects blazorStartResolver and posts the failure message so no flow can hang.
169-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
updateReadywhen install transitions to waiting in the same session.On Line 173 onward, once
reg.waitingappears in thestatechangepath, the code logs completion but never callshandle(BswupMessage.updateReady, { reload }). That leaves listeners without the public update-ready signal for updates discovered while the tab stays open.Suggested fix
reg.installing.addEventListener('statechange', function (e) { debug('state changed', e, 'eventPhase:', e.eventPhase, 'currentTarget.state:', e.currentTarget.state); handle(BswupMessage.stateChanged, e); - if (!reg.waiting) return; + if (!reg.waiting) return; + if ((e.currentTarget as ServiceWorker).state !== 'installed') return; + handle(BswupMessage.updateReady, { reload }); if (hadActiveWorkerAtStartup) { info('update finished.'); // not first install } 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 169 - 180, When reg.installing's 'statechange' handler detects reg.waiting, besides logging, emit the public update-ready message by calling handle(BswupMessage.updateReady, { reload }) so listeners get notified for updates discovered in the same session; locate the reg.installing.addEventListener callback and add the handle(BswupMessage.updateReady, { reload }) call when reg.waiting is true, using the existing reload variable (or defaulting to false if reload isn't in scope).src/Bswup/Bit.Bswup/BswupProgress.razor (1)
43-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJS-encode
AppContainerandHandlerbefore embedding into the inline script.On Line 43 and Line 46, these values are injected as raw single-quoted JS literals. A quote/backslash/newline payload can break the string and inject script.
Suggested fix
+@using System.Text.Json + <script> if (window.BitBswupProgress !== undefined) { window.BitBswupProgress.start( @(AutoReload ? "true" : "false"), @(ShowLogs ? "true" : "false"), @(ShowAssets ? "true" : "false"), - '@(AppContainer)', + @((MarkupString)JsonSerializer.Serialize(AppContainer)), @(HideApp ? "true" : "false"), @(AutoHide ? "true" : "false"), - '@(Handler)'); + @((MarkupString)JsonSerializer.Serialize(Handler))); } else { console.error('BitBswupProgress not found'); } </script>🤖 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/BswupProgress.razor` around lines 43 - 46, The inline script is embedding AppContainer and Handler as raw single-quoted JS literals which allows quote/backslash/newline payloads to break out; change the embedding to produce safe JS string literals by serializing these C# values with a JSON serializer (e.g. System.Text.Json.JsonSerializer.Serialize) and inject the serialized result (remove the extra single quotes around them) so AppContainer and Handler become valid JS-encoded literals; ensure you add the required using/import for System.Text.Json if missing and update the occurrences where '@(AppContainer)' and '@(Handler)' are used in BswupProgress.razor.
🧹 Nitpick comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (1)
762-782: 💤 Low valueStatic analysis flagged potential ReDoS, but risk is low.
The
new RegExp(p)on line 772 constructs regex from developer-configured values (self.prohibitedUrls,self.serverHandledUrls, etc.), not untrusted user input. An attacker would need to modify the service-worker.js file itself to inject a malicious pattern.While the catch block handles syntax errors, it doesn't guard against pathological patterns. Consider documenting best practices for pattern authors (avoid nested quantifiers like
(a+)+) in the README, but adding a ReDoS validation library seems disproportionate for this threat model.🤖 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 762 - 782, Static analysis flagged potential ReDoS from constructing RegExp from developer-configured values; keep the existing try/catch in prepareRegExpArray but also add a short developer-facing note documenting safe pattern practices (e.g., avoid catastrophic backtracking / nested quantifiers like (a+)+) and recommend limiting pattern length; update the repository README or the service worker module comment near prepareRegExpArray and reference the configuration sources (self.prohibitedUrls, self.serverHandledUrls, etc.) so maintainers know the threat model and how to author safe patterns.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@src/Bswup/Bit.Bswup/BswupProgress.razor`:
- Around line 43-46: The inline script is embedding AppContainer and Handler as
raw single-quoted JS literals which allows quote/backslash/newline payloads to
break out; change the embedding to produce safe JS string literals by
serializing these C# values with a JSON serializer (e.g.
System.Text.Json.JsonSerializer.Serialize) and inject the serialized result
(remove the extra single quotes around them) so AppContainer and Handler become
valid JS-encoded literals; ensure you add the required using/import for
System.Text.Json if missing and update the occurrences where '@(AppContainer)'
and '@(Handler)' are used in BswupProgress.razor.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 237-241: Blazor.start() is called in the CLIENTS_CLAIMED handler
without a rejection path, which can leave blazorStartResolver unresolved and
stall callers (e.g., data.reload()); update the handler around Blazor.start() so
that both success and failure paths always call blazorStartResolver (resolve or
reject as appropriate) and always post back to e.source (e.g., send a success or
failure message like 'BLAZOR_STARTED' vs 'BLAZOR_START_FAILED'), ensuring
Blazor.start().then(...) has a corresponding .catch(...) that resolves/rejects
blazorStartResolver and posts the failure message so no flow can hang.
- Around line 169-180: When reg.installing's 'statechange' handler detects
reg.waiting, besides logging, emit the public update-ready message by calling
handle(BswupMessage.updateReady, { reload }) so listeners get notified for
updates discovered in the same session; locate the
reg.installing.addEventListener callback and add the
handle(BswupMessage.updateReady, { reload }) call when reg.waiting is true,
using the existing reload variable (or defaulting to false if reload isn't in
scope).
---
Nitpick comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 762-782: Static analysis flagged potential ReDoS from constructing
RegExp from developer-configured values; keep the existing try/catch in
prepareRegExpArray but also add a short developer-facing note documenting safe
pattern practices (e.g., avoid catastrophic backtracking / nested quantifiers
like (a+)+) and recommend limiting pattern length; update the repository README
or the service worker module comment near prepareRegExpArray and reference the
configuration sources (self.prohibitedUrls, self.serverHandledUrls, etc.) so
maintainers know the threat model and how to author safe patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d8ed2388-cbc2-4b55-9764-b9752f60e6d8
📒 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
♻️ Duplicate comments (2)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (1)
559-562:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow integrity classification to avoid suppressing retries on transient failures.
Line 561 still treats broad transport markers (notably
ERR_FAILED) as integrity failures, which can classify transient network errors as non-retriable and skip backoff.Suggested patch
- const isIntegrity = - hasIntegrity && - /integrity|digest|EPRPROTO|ERR_FAILED/i.test(String(fetchErr && (fetchErr as any).message || fetchErr)); + const isIntegrity = + hasIntegrity && + /integrity|digest|subresource integrity|valid digest/i.test( + String((fetchErr && (fetchErr as any).message) || fetchErr) + );🤖 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 559 - 562, The integrity classification currently treats broad transport errors like "ERR_FAILED" as integrity failures; update the isIntegrity computation in the bit-bswup.sw.ts code (the const isIntegrity assignment that tests fetchErr) to remove/omit "ERR_FAILED" from the regex so only actual integrity indicators (e.g., "integrity", "digest", "EPRPROTO") are matched; preserve the hasIntegrity guard and the existing String(fetchErr... ) usage and keep the /.../i flag so transient transport errors no longer suppress retries/backoff.src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts (1)
243-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
startBlazor(true)inCLIENTS_CLAIMEDto keep one startup contract.Line 244 calls
Blazor.start()directly and bypasses the validations instartBlazor()(Line 377+). This keeps first-install startup behavior inconsistent with the rest of the runtime and can surface avoidable startup failures.Proposed fix
- if (e.data === 'CLIENTS_CLAIMED') { - Blazor.start().then(() => { - blazorStartResolver?.(undefined); - e.source.postMessage('BLAZOR_STARTED'); - }).catch((err) => { - error('Blazor.start() failed after clients claimed', err); - // Always settle the pending reload() promise so callers can't hang, and - // notify the worker that startup failed (mirrors the success path). - blazorStartResolver?.(undefined); - e.source.postMessage('BLAZOR_START_FAILED'); - }); + if (e.data === 'CLIENTS_CLAIMED') { + Promise.resolve() + .then(() => startBlazor(true)) + .then(() => { + blazorStartResolver?.(undefined); + e.source?.postMessage('BLAZOR_STARTED'); + }) + .catch((err) => { + error('Blazor.start() failed after clients claimed', err); + blazorStartResolver?.(undefined); + e.source?.postMessage('BLAZOR_START_FAILED'); + }); return; }Also applies to: 377-397
🤖 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 243 - 253, The CLIENTS_CLAIMED handler is calling Blazor.start() directly and bypassing the unified startup flow; replace that call with startBlazor(true) so the same validation/retry/contract logic is used (keep existing success and failure handling: call blazorStartResolver?.(undefined) and post 'BLAZOR_STARTED' or 'BLAZOR_START_FAILED' to e.source). Also search for other direct Blazor.start() uses (e.g., the similar block covered by startBlazor() at the other location) and ensure they call startBlazor(true) instead to maintain a single startup contract.
🤖 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`:
- Line 1: The startup script in BswupProgress.razor currently uses
System.Text.Json.JsonSerializer.Serialize to embed plain string parameters into
the generated JS which triggers trimming/AOT IL2026 warnings; replace those
calls by encoding the string values with
System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode before inserting into
the script, add the using for System.Text.Encodings.Web if absent, and ensure
you pass the encoded string (not Json-serialized) into the startup args so the
embedded values are safe for JS and avoid reflection-based serialization.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 196-200: createAssetsCache() is invoked in lax-mode without
handling promise rejection, which risks unhandled rejections; wrap the call to
createAssetsCache (or append a .catch) so any thrown error is caught and handled
(e.g., log a structured error and preserve lax behavior), referencing the
errorTolerance: 'lax' flow and handleFetch lazy-fill path. Specifically, change
the invocation in the lax branch to either await createAssetsCache() inside a
try/catch or call createAssetsCache().catch(err =>
processLogger.error("createAssetsCache failed (lax):", err)) so failures are
recorded and do not surface as unhandled rejections while keeping lifecycle
progression.
---
Duplicate comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 559-562: The integrity classification currently treats broad
transport errors like "ERR_FAILED" as integrity failures; update the isIntegrity
computation in the bit-bswup.sw.ts code (the const isIntegrity assignment that
tests fetchErr) to remove/omit "ERR_FAILED" from the regex so only actual
integrity indicators (e.g., "integrity", "digest", "EPRPROTO") are matched;
preserve the hasIntegrity guard and the existing String(fetchErr... ) usage and
keep the /.../i flag so transient transport errors no longer suppress
retries/backoff.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 243-253: The CLIENTS_CLAIMED handler is calling Blazor.start()
directly and bypassing the unified startup flow; replace that call with
startBlazor(true) so the same validation/retry/contract logic is used (keep
existing success and failure handling: call blazorStartResolver?.(undefined) and
post 'BLAZOR_STARTED' or 'BLAZOR_START_FAILED' to e.source). Also search for
other direct Blazor.start() uses (e.g., the similar block covered by
startBlazor() at the other location) and ensure they call startBlazor(true)
instead to maintain a single startup contract.
🪄 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: ecdc4731-1284-46fc-958a-9cdf16f4e399
📒 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: 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-cleanup.ts (1)
17-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
waitUntilresolves too early; cleanup signaling can be dropped.
removeBswup()schedulesskipWaiting()/matchAll()/postMessagebut doesn’t await/return that chain, so install can finish before activation + client teardown messages run. This weakens the intended “unregister + reload every tab” guarantee.Suggested fix
async function removeBswup() { @@ - 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 }); + await Promise.all((clients || []).map(async client => { + client.postMessage('UNREGISTER'); + await new Promise(res => setTimeout(res, 1000)); + client.postMessage('WAITING_SKIPPED'); + })); }🤖 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 17 - 26, The removal flow in removeBswup currently calls self.skipWaiting().then(...matchAll...postMessage...) but does not return or await that promise, so event.waitUntil resolves before the skipWaiting/messaging chain completes and cleanup signaling can be lost; change removeBswup to return/await the full promise chain (await or return self.skipWaiting().then(...).then(...)) so that the service worker's event.waitUntil covers skipWaiting(), clients.matchAll({includeUncontrolled}), and the postMessage('UNREGISTER') / setTimeout(...) -> postMessage('WAITING_SKIPPED') fallback, ensuring activation and client notifications complete before install finishes.
♻️ Duplicate comments (2)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (1)
91-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
VERSIONinCACHE_NAMEto prevent cache collision between workers.When
self.cacheVersionis set to a stable string (e.g., pinned across deployments to avoid dev churn),CACHE_NAMEbecomes version-agnostic. A waiting worker withVERSION = "abc123"and an active worker withVERSION = "xyz789"share the same cache bucket whencacheVersion = "v1". During install, the waiting worker updates assets in the shared bucket while the active worker serves from it, risking torn reads (e.g., newindex.htmlpaired with old.dllfiles before SKIP_WAITING).Include
VERSIONin the cache name to isolate each deployment:🔧 Proposed fix
-const CACHE_NAME = `${CACHE_NAME_PREFIX} - ${CACHE_VERSION}`; +const CACHE_NAME = `${CACHE_NAME_PREFIX} - ${VERSION} - ${CACHE_VERSION}`;This ensures each
VERSIONgets its own isolated bucket.cacheVersioncan still force a refresh when bumped (e.g.,v1→v2), and when unset it falls back toVERSION, preserving the existing auto-rotation behavior.🤖 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 91 - 99, The current CACHE_NAME uses only CACHE_VERSION (derived from self.cacheVersion or VERSION) which allows different workers with different VERSIONs but identical cacheVersion to collide; update the CACHE_NAME construction to include the actual VERSION as well so each deployment gets its own bucket while preserving CACHE_VERSION overrides—i.e., modify the code that defines CACHE_NAME (which references CACHE_VERSION and CACHE_NAME_PREFIX) to append or incorporate VERSION (while keeping the existing fallback logic for CACHE_VERSION) so cache names become unique per VERSION and still honor cacheVersion for forced refreshes.src/Bswup/Bit.Bswup/BswupProgress.razor (1)
43-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnsafe JavaScript injection of
AppContainerandHandlerparameters.Lines 43 and 46 inject
@(AppContainer)and@(Handler)directly into JavaScript single-quoted string literals. If either parameter contains',\, newline, or</script>, the string breaks and arbitrary script can execute. Razor's@()uses HTML encoding, which does not secure JavaScript contexts.Use
System.Text.Encodings.Web.JavaScriptEncoder.Default.Encodeto safely embed these values (avoiding IL2026 trimming warnings fromJsonSerializer).🔒 Proposed fix
+@using System.Text.Encodings.Web + <script> if (window.BitBswupProgress !== undefined) { window.BitBswupProgress.start( @(AutoReload ? "true" : "false"), @(ShowLogs ? "true" : "false"), @(ShowAssets ? "true" : "false"), - '@(AppContainer)', + '`@JavaScriptEncoder.Default.Encode`(AppContainer)', @(HideApp ? "true" : "false"), @(AutoHide ? "true" : "false"), - '@(Handler)'); + '`@JavaScriptEncoder.Default.Encode`(Handler ?? string.Empty)'); } else { console.error('BitBswupProgress not found'); } </script>🤖 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/BswupProgress.razor` around lines 43 - 46, The Razor code injects AppContainer and Handler directly into JavaScript single-quoted strings causing JS injection; update the interpolation to use System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(...) for both AppContainer and Handler so their values are safely JS-encoded before being embedded (use the fully qualified name if no using is present), leaving the existing boolean interpolations (HideApp/AutoHide) unchanged; locate the usages in BswupProgress.razor where '@(AppContainer)' and '@(Handler)' are emitted and replace them with the JavaScriptEncoder-encoded versions.
🤖 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.ts`:
- Around line 243-255: In the CLIENTS_CLAIMED handler inside the event listener
in bit-bswup.ts, replace e.source.postMessage(...) with optional chaining
e.source?.postMessage(...) in both the success ('BLAZOR_STARTED') and failure
('BLAZOR_START_FAILED') branches to avoid null dereference; then update the
service worker (bit-bswup.sw.ts) message handler to add a case for
'BLAZOR_START_FAILED' mirroring the recovery behavior used for 'BLAZOR_STARTED'
(e.g., trigger the same reload/claim/reset logic used by the existing Blazor
start success path) so failures can be handled by the worker.
---
Outside diff comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 17-26: The removal flow in removeBswup currently calls
self.skipWaiting().then(...matchAll...postMessage...) but does not return or
await that promise, so event.waitUntil resolves before the skipWaiting/messaging
chain completes and cleanup signaling can be lost; change removeBswup to
return/await the full promise chain (await or return
self.skipWaiting().then(...).then(...)) so that the service worker's
event.waitUntil covers skipWaiting(), clients.matchAll({includeUncontrolled}),
and the postMessage('UNREGISTER') / setTimeout(...) ->
postMessage('WAITING_SKIPPED') fallback, ensuring activation and client
notifications complete before install finishes.
---
Duplicate comments:
In `@src/Bswup/Bit.Bswup/BswupProgress.razor`:
- Around line 43-46: The Razor code injects AppContainer and Handler directly
into JavaScript single-quoted strings causing JS injection; update the
interpolation to use
System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(...) for both
AppContainer and Handler so their values are safely JS-encoded before being
embedded (use the fully qualified name if no using is present), leaving the
existing boolean interpolations (HideApp/AutoHide) unchanged; locate the usages
in BswupProgress.razor where '@(AppContainer)' and '@(Handler)' are emitted and
replace them with the JavaScriptEncoder-encoded versions.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Around line 91-99: The current CACHE_NAME uses only CACHE_VERSION (derived
from self.cacheVersion or VERSION) which allows different workers with different
VERSIONs but identical cacheVersion to collide; update the CACHE_NAME
construction to include the actual VERSION as well so each deployment gets its
own bucket while preserving CACHE_VERSION overrides—i.e., modify the code that
defines CACHE_NAME (which references CACHE_VERSION and CACHE_NAME_PREFIX) to
append or incorporate VERSION (while keeping the existing fallback logic for
CACHE_VERSION) so cache names become unique per VERSION and still honor
cacheVersion for forced refreshes.
🪄 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: 36307d12-020e-4960-8ee1-c037c0c1dc14
📒 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
…itframework into 12408-bswup-improvements
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
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/Bit.Bswup.csproj (1)
37-45:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBuild blocker: pipeline still fails with
MSB4057forInstallNodejsDependencies.The CI log reports:
The target "InstallNodejsDependencies" does not exist in the project.Since Line 38 invokes this target, this target wiring is still broken in the evaluated build graph and should be fixed before merge.🤖 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/Bit.Bswup.csproj` around lines 37 - 45, The BeforeBuildTasks target is calling InstallNodejsDependencies via CallTarget on line 38, but the InstallNodejsDependencies target is defined after BeforeBuildTasks in the project file. MSBuild requires target definitions to appear before they are referenced. Move the InstallNodejsDependencies target definition (and the related BuildJavaScript and BuildCss targets) to appear before the BeforeBuildTasks target definition in the Bit.Bswup.csproj file so that when BeforeBuildTasks tries to call these targets, they already exist in the build graph.Source: Pipeline failures
🧹 Nitpick comments (1)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts (1)
106-106: 💤 Low valueRemove unused module-level
integrityFailureCount.This variable is shadowed by the local declaration at Line 551 inside
createAssetsCache()and is never read or written at module scope.-let integrityFailureCount = 0;🤖 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` at line 106, Remove the module-level `integrityFailureCount` variable declaration as it is shadowed by a local declaration of the same name inside the `createAssetsCache()` function and is never actually used at module scope. The local variable within `createAssetsCache()` should be preserved as it is the one that is actively used.
🤖 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 89-93: The innerHTML assignment in the asset list creation (where
data.asset.url and data.asset.hash are inserted into the li element) creates an
XSS vulnerability if the asset manifest is compromised. Replace the innerHTML
assignment with DOM APIs by using textContent for the plain text portions and
createElement to create the bold element separately. Build the li element's
content by creating text nodes and appending the bold element using appendChild
instead of string concatenation with innerHTML.
- Around line 93-101: The const declarations of percent and perStr are declared
at switch-clause scope, making them accessible to other clauses which violates
Biome linting rules. Wrap these two variable declarations in a block (curly
braces) to restrict their scope to just that clause. Additionally, replace the
innerHTML assignment on the percentEl element with textContent instead, since
the value being set is a numeric string and textContent is more explicit and
safer for this use case.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Line 15: Remove the `importScripts: any` redeclaration at line 15 in
bit-bswup.sw.ts since the WebWorker library (configured in tsconfig.sw.json)
already provides the correct type declaration for `importScripts` on
`WorkerGlobalScope`. Deleting this redundant declaration will resolve the TS2717
duplicate identifier build error.
- Around line 957-967: The function `applyUrlCaseSensitivity` is defined twice
in the file with identical implementations and comment blocks, causing dead
code. Remove one of the duplicate definitions of `applyUrlCaseSensitivity`
(keeping only one complete definition with its comment block that explains the
case-insensitivity logic and pattern handling), and verify the remaining
definition includes the full implementation that checks for the
caseInsensitiveUrl flag and the 'i' flag before creating a new RegExp with the
case-insensitive flag appended.
---
Outside diff comments:
In `@src/Bswup/Bit.Bswup/Bit.Bswup.csproj`:
- Around line 37-45: The BeforeBuildTasks target is calling
InstallNodejsDependencies via CallTarget on line 38, but the
InstallNodejsDependencies target is defined after BeforeBuildTasks in the
project file. MSBuild requires target definitions to appear before they are
referenced. Move the InstallNodejsDependencies target definition (and the
related BuildJavaScript and BuildCss targets) to appear before the
BeforeBuildTasks target definition in the Bit.Bswup.csproj file so that when
BeforeBuildTasks tries to call these targets, they already exist in the build
graph.
---
Nitpick comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw.ts`:
- Line 106: Remove the module-level `integrityFailureCount` variable declaration
as it is shadowed by a local declaration of the same name inside the
`createAssetsCache()` function and is never actually used at module scope. The
local variable within `createAssetsCache()` should be preserved as it is the one
that is actively used.
🪄 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: cdaae3c3-42fb-44f9-9882-ff02504c68e5
📒 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.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
Comment on lines
+47
to
+52
| <Target Name="BuildJavaScript" Inputs="Scripts/bit-bswup.ts;Scripts/bit-bswup.progress.ts;Scripts/bit-bswup.sw.ts;Scripts/bit-bswup.sw-cleanup.ts" Outputs="wwwroot/bit-bswup.js;wwwroot/bit-bswup.progress.js;wwwroot/bit-bswup.sw.js;wwwroot/bit-bswup.sw-cleanup.js"> | ||
| <!-- Page scripts (DOM lib) and service-worker scripts (WebWorker lib) are compiled by | ||
| separate tsconfigs because the DOM and WebWorker type libraries cannot coexist in | ||
| one compilation. Both emit into wwwroot. --> | ||
| <Exec Command="node_modules/.bin/tsc" StandardOutputImportance="high" StandardErrorImportance="high" /> | ||
| <Exec Command="node_modules/.bin/tsc -p tsconfig.sw.json" StandardOutputImportance="high" StandardErrorImportance="high" /> |
There was a problem hiding this comment.
Include TypeScript config files in incremental build inputs.
Line 47 currently tracks only source .ts files as inputs. If tsconfig.json or tsconfig.sw.json changes, BuildJavaScript can be skipped and stale JS artifacts remain in wwwroot.
Suggested patch
- <Target Name="BuildJavaScript" Inputs="Scripts/bit-bswup.ts;Scripts/bit-bswup.progress.ts;Scripts/bit-bswup.sw.ts;Scripts/bit-bswup.sw-cleanup.ts" Outputs="wwwroot/bit-bswup.js;wwwroot/bit-bswup.progress.js;wwwroot/bit-bswup.sw.js;wwwroot/bit-bswup.sw-cleanup.js">
+ <Target Name="BuildJavaScript" Inputs="Scripts/bit-bswup.ts;Scripts/bit-bswup.progress.ts;Scripts/bit-bswup.sw.ts;Scripts/bit-bswup.sw-cleanup.ts;tsconfig.json;tsconfig.sw.json" Outputs="wwwroot/bit-bswup.js;wwwroot/bit-bswup.progress.js;wwwroot/bit-bswup.sw.js;wwwroot/bit-bswup.sw-cleanup.js">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Target Name="BuildJavaScript" Inputs="Scripts/bit-bswup.ts;Scripts/bit-bswup.progress.ts;Scripts/bit-bswup.sw.ts;Scripts/bit-bswup.sw-cleanup.ts" Outputs="wwwroot/bit-bswup.js;wwwroot/bit-bswup.progress.js;wwwroot/bit-bswup.sw.js;wwwroot/bit-bswup.sw-cleanup.js"> | |
| <!-- Page scripts (DOM lib) and service-worker scripts (WebWorker lib) are compiled by | |
| separate tsconfigs because the DOM and WebWorker type libraries cannot coexist in | |
| one compilation. Both emit into wwwroot. --> | |
| <Exec Command="node_modules/.bin/tsc" StandardOutputImportance="high" StandardErrorImportance="high" /> | |
| <Exec Command="node_modules/.bin/tsc -p tsconfig.sw.json" StandardOutputImportance="high" StandardErrorImportance="high" /> | |
| <Target Name="BuildJavaScript" Inputs="Scripts/bit-bswup.ts;Scripts/bit-bswup.progress.ts;Scripts/bit-bswup.sw.ts;Scripts/bit-bswup.sw-cleanup.ts;tsconfig.json;tsconfig.sw.json" Outputs="wwwroot/bit-bswup.js;wwwroot/bit-bswup.progress.js;wwwroot/bit-bswup.sw.js;wwwroot/bit-bswup.sw-cleanup.js"> | |
| <!-- Page scripts (DOM lib) and service-worker scripts (WebWorker lib) are compiled by | |
| separate tsconfigs because the DOM and WebWorker type libraries cannot coexist in | |
| one compilation. Both emit into wwwroot. --> | |
| <Exec Command="node_modules/.bin/tsc" StandardOutputImportance="high" StandardErrorImportance="high" /> | |
| <Exec Command="node_modules/.bin/tsc -p tsconfig.sw.json" StandardOutputImportance="high" StandardErrorImportance="high" /> |
🤖 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/Bit.Bswup.csproj` around lines 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.
Comment on lines
17
to
+33
| async function removeBswup() { | ||
| const cacheKeys = await caches.keys(); | ||
| const cachePromises = cacheKeys.filter(key => key.startsWith('bit-bswup') || key.startsWith('blazor-resources')).map(key => caches.delete(key)); | ||
| await Promise.all(cachePromises); | ||
|
|
||
| self.skipWaiting().then(() => self.clients | ||
| .matchAll({ includeUncontrolled: true }) | ||
| .then(clients => (clients || []).forEach(client => { | ||
| client.postMessage('UNREGISTER'); | ||
| setTimeout(() => client.postMessage('WAITING_SKIPPED'), 1000); | ||
| }))); | ||
| // skipWaiting() so this cleanup worker activates without waiting for existing clients to | ||
| // close, then message every client (controlled or not) to unregister itself. The delayed | ||
| // 'WAITING_SKIPPED' nudge is a fallback reload signal for clients that don't act on | ||
| // 'UNREGISTER' fast enough, so no tab is left running against the now-deleted caches. | ||
| // Await the whole chain so the install event (which waits on removeBswup via waitUntil) | ||
| // doesn't resolve before the teardown signalling has actually been dispatched. | ||
| await self.skipWaiting(); | ||
| const clients = await self.clients.matchAll({ includeUncontrolled: true }); | ||
| (clients || []).forEach((client: any) => { | ||
| client.postMessage('UNREGISTER'); | ||
| setTimeout(() => client.postMessage('WAITING_SKIPPED'), 1000); | ||
| }); |
There was a problem hiding this comment.
The delayed WAITING_SKIPPED messages may not be sent before the worker is terminated.
The setTimeout callbacks on lines 32-32 schedule delayed messages, but these callbacks are not part of the promise chain returned by removeBswup(). Once this function returns, the install event's waitUntil() resolves, and the service worker may be terminated before the 1-second timeout fires—cancelling any pending setTimeout callbacks.
While service workers typically remain alive briefly after events complete, and the primary UNREGISTER mechanism should handle most cases, the fallback WAITING_SKIPPED messages may be lost if the worker is terminated early. This could leave tabs in a degraded state if the UNREGISTER handler encounters errors.
🔧 Suggested fix to ensure delayed messages are sent
Wrap the delayed messages in promises and await them:
async function removeBswup() {
const cacheKeys = await caches.keys();
const cachePromises = cacheKeys.filter(key => key.startsWith('bit-bswup') || key.startsWith('blazor-resources')).map(key => caches.delete(key));
await Promise.all(cachePromises);
await self.skipWaiting();
const clients = await self.clients.matchAll({ includeUncontrolled: true });
+ const messagePromises = (clients || []).map((client: any) => {
+ client.postMessage('UNREGISTER');
+ return new Promise<void>(resolve => {
+ setTimeout(() => {
+ client.postMessage('WAITING_SKIPPED');
+ resolve();
+ }, 1000);
+ });
+ });
+ await Promise.all(messagePromises);
- (clients || []).forEach((client: any) => {
- client.postMessage('UNREGISTER');
- setTimeout(() => client.postMessage('WAITING_SKIPPED'), 1000);
- });
}This ensures the install event keeps the worker alive until all delayed messages have been dispatched.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function removeBswup() { | |
| const cacheKeys = await caches.keys(); | |
| const cachePromises = cacheKeys.filter(key => key.startsWith('bit-bswup') || key.startsWith('blazor-resources')).map(key => caches.delete(key)); | |
| await Promise.all(cachePromises); | |
| self.skipWaiting().then(() => self.clients | |
| .matchAll({ includeUncontrolled: true }) | |
| .then(clients => (clients || []).forEach(client => { | |
| client.postMessage('UNREGISTER'); | |
| setTimeout(() => client.postMessage('WAITING_SKIPPED'), 1000); | |
| }))); | |
| // skipWaiting() so this cleanup worker activates without waiting for existing clients to | |
| // close, then message every client (controlled or not) to unregister itself. The delayed | |
| // 'WAITING_SKIPPED' nudge is a fallback reload signal for clients that don't act on | |
| // 'UNREGISTER' fast enough, so no tab is left running against the now-deleted caches. | |
| // Await the whole chain so the install event (which waits on removeBswup via waitUntil) | |
| // doesn't resolve before the teardown signalling has actually been dispatched. | |
| await self.skipWaiting(); | |
| const clients = await self.clients.matchAll({ includeUncontrolled: true }); | |
| (clients || []).forEach((client: any) => { | |
| client.postMessage('UNREGISTER'); | |
| setTimeout(() => client.postMessage('WAITING_SKIPPED'), 1000); | |
| }); | |
| async function removeBswup() { | |
| const cacheKeys = await caches.keys(); | |
| const cachePromises = cacheKeys.filter(key => key.startsWith('bit-bswup') || key.startsWith('blazor-resources')).map(key => caches.delete(key)); | |
| await Promise.all(cachePromises); | |
| // skipWaiting() so this cleanup worker activates without waiting for existing clients to | |
| // close, then message every client (controlled or not) to unregister itself. The delayed | |
| // 'WAITING_SKIPPED' nudge is a fallback reload signal for clients that don't act on | |
| // 'UNREGISTER' fast enough, so no tab is left running against the now-deleted caches. | |
| // Await the whole chain so the install event (which waits on removeBswup via waitUntil) | |
| // doesn't resolve before the teardown signalling has actually been dispatched. | |
| await self.skipWaiting(); | |
| const clients = await self.clients.matchAll({ includeUncontrolled: true }); | |
| const messagePromises = (clients || []).map((client: any) => { | |
| client.postMessage('UNREGISTER'); | |
| return new Promise<void>(resolve => { | |
| setTimeout(() => { | |
| client.postMessage('WAITING_SKIPPED'); | |
| resolve(); | |
| }, 1000); | |
| }); | |
| }); | |
| await Promise.all(messagePromises); | |
| } |
🤖 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 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.
Comment on lines
+129
to
+135
| // First install: a worker is active but not yet controlling this page. | ||
| // Ask it to claim clients; once 'CLIENTS_CLAIMED' arrives we start Blazor | ||
| // and resolve this promise so callers can finalize (e.g. hide the splash). | ||
| if (reg.active) { | ||
| reg.active.postMessage('CLAIM_CLIENTS'); | ||
| return new Promise<void>((res) => blazorStartResolver = res as (value: unknown) => void); | ||
| } |
There was a problem hiding this comment.
Retry flow can be a no-op when an update install fails
On Line 132-135, reload() always chooses CLAIM_CLIENTS whenever reg.active exists. In update-error retries (reg.waiting absent, reg.active present), this does not reload or re-attempt install, so the Retry button path can effectively do nothing. Gate CLAIM_CLIENTS to first-install only, and force a navigation reload for update/error retries.
Proposed fix
reload = () => {
@@
- if (reg.active) {
+ if (reg.active && !hadActiveWorkerAtStartup) {
reg.active.postMessage('CLAIM_CLIENTS');
return new Promise<void>((res) => blazorStartResolver = res as (value: unknown) => void);
}
- // No worker to coordinate with - fall back to a plain reload.
+ // Update/error retry path (or no worker to coordinate with):
+ // navigate so the browser re-checks the worker and retries install flow.
window.location.reload();
return new Promise<void>(() => { });
};🤖 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 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.
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
New Features
showAssets, stay visually correct, and include ARIA progress attributes.Bug Fixes
Documentation
Style