Apply Bswup improvements (#12408)#12418
Open
msynk wants to merge 30 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
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:
WalkthroughUpdates Bit.Bswup runtime and service-worker behavior, adds progress/error UI, refreshes build and demo wiring, introduces FullDemo app/server components, and expands the README for the new message and update flow. ChangesBit.Bswup Service Worker, UI, and Demo Updates
Estimated code review effort: 4 (Complex) | ~60 minutes 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()
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
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. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts (2)
126-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly use
CLAIM_CLIENTSfor first-install reloads.When an update install fails,
reg.waitingis gone butreg.activeis still present, so this branch postsCLAIM_CLIENTSto the already-active worker instead of forcing a navigation retry. That makes the Retry button effectively a no-op for update-error recovery.Suggested fix
reload = () => { if (reg.waiting) { reg.waiting.postMessage('SKIP_WAITING'); return new Promise<void>(() => { }); } - 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 retry (or no worker to coordinate with): force a navigation so + // the browser re-runs registration/install logic. 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 126 - 147, The reload logic in the `reload` function is treating any `reg.active` worker as a first-install case, which causes Retry after an update failure to post `CLAIM_CLIENTS` to an already-active worker instead of navigating again. Update the branching so `CLAIM_CLIENTS` is only sent for true first-install reloads, and make the `reg.active` path for update-error recovery fall back to a plain `window.location.reload()` (or equivalent navigation retry) in `bit-bswup.ts`.
268-295: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon't report failed Blazor startup as
BLAZOR_STARTED.Both the rejected
startPromisepath and theundefinedpath callonStarted(). That resolves the first-install reload flow and postsBLAZOR_STARTEDeven when Blazor never booted, so the service worker and progress UI continue down the success path while the page can stay blank/inert.🤖 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 268 - 295, The `CLIENTS_CLAIMED` handling in `bit-bswup.ts` currently treats failed or missing Blazor startup the same as success by routing both the rejected `startPromise` path and the `undefined` path through `onStarted()`. Split the success and failure flows in the `startBlazor(true)` / `blazorStartResolver` logic so `BLAZOR_STARTED` is only posted after a real successful startup, and on rejection or unavailable startup resolve the reload flow separately without signaling success.
🧹 Nitpick comments (1)
src/Bswup/FullDemo/Server/Program.cs (1)
11-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope CORS to the origins that actually need it.
Line 42 opens every controller mapped on Line 47 to any origin, header, and method. That is a very permissive default for a server app and makes future controller additions cross-origin callable unless someone remembers to tighten it later. Prefer a named policy for the demo origin(s), or remove CORS entirely if this stays same-origin.
Also applies to: 42-42, 47-47
🤖 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/FullDemo/Server/Program.cs` around lines 11 - 12, The CORS setup in Program should not leave all controllers broadly cross-origin callable by default. Update the CORS configuration around builder.Services.AddCors() and the policy usage in the controller pipeline to use a named policy that only allows the specific demo origin(s), headers, and methods needed, or remove CORS entirely if the app is same-origin. Use the existing setup points in Program and the controller mapping where the policy is applied to keep the change scoped.
🤖 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.Demo/Pages/HomePage.razor`:
- Line 6: Replace the placeholder `111` heading in `HomePage.razor` by either
using the real page title or removing the extra `<h1>` entirely, since this
duplicated heading conflicts with the existing main heading. Update the markup
around the `HomePage` page content so only one meaningful top-level heading
remains.
In `@src/Bswup/Bit.Bswup.slnx`:
- Around line 10-15: The solution-level InstallNodejsDependencies target is now
invalid because the newly added FullDemo app projects do not define that target.
Update the solution composition in Bit.Bswup.slnx or add a no-op/passthrough
InstallNodejsDependencies target to the affected FullDemo project files (for
example, the Client and Server csproj symbols) so the solution-wide target can
execute without MSB4057.
In `@src/Bswup/Bit.Bswup/BswupProgress.razor`:
- Around line 24-58: The BswupProgress.razor markup still uses inline style
attributes for initial visibility, which keeps it incompatible with strict CSP.
Update the BswupProgress component to remove the inline display styles from the
root container, assets list, and error blocks, and move those defaults into CSS
classes or data-attribute-driven styling in the BswupProgress/bit-bswup
stylesheet. Keep the existing element IDs like bit-bswup, bit-bswup-assets, and
bit-bswup-error so the external script can continue toggling state without
inline styles.
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.sw-cleanup.ts`:
- Around line 28-34: Move the cache deletion logic out of the install flow and
into the service worker’s activate handling after clients.claim() so controlled
tabs are not left using a worker whose caches were already purged. Update the
cleanup logic in the sw-cleanup script by keeping skipWaiting() in install, then
performing the cacheKeys/caches.delete cleanup from the activate path once the
new worker has claimed clients; use the existing cache key filters for bit-bswup
and blazor-resources when relocating the code.
In `@src/Bswup/FullDemo/Server/Components/App.razor`:
- Around line 40-41: Move the Bswup progress bootstrap so the
`DemoBswupProgressBar` component is rendered before `bit-bswup.progress.js`
loads. In `App.razor`, reorder the `<script
src="_content/Bit.Bswup/bit-bswup.progress.js"></script>` and
`<DemoBswupProgressBar />` elements so `BswupProgress.razor` can find the
emitted `data-bit-bswup-*` attributes when the script initializes.
---
Duplicate comments:
In `@src/Bswup/Bit.Bswup/Scripts/bit-bswup.ts`:
- Around line 126-147: The reload logic in the `reload` function is treating any
`reg.active` worker as a first-install case, which causes Retry after an update
failure to post `CLAIM_CLIENTS` to an already-active worker instead of
navigating again. Update the branching so `CLAIM_CLIENTS` is only sent for true
first-install reloads, and make the `reg.active` path for update-error recovery
fall back to a plain `window.location.reload()` (or equivalent navigation retry)
in `bit-bswup.ts`.
- Around line 268-295: The `CLIENTS_CLAIMED` handling in `bit-bswup.ts`
currently treats failed or missing Blazor startup the same as success by routing
both the rejected `startPromise` path and the `undefined` path through
`onStarted()`. Split the success and failure flows in the `startBlazor(true)` /
`blazorStartResolver` logic so `BLAZOR_STARTED` is only posted after a real
successful startup, and on rejection or unavailable startup resolve the reload
flow separately without signaling success.
---
Nitpick comments:
In `@src/Bswup/FullDemo/Server/Program.cs`:
- Around line 11-12: The CORS setup in Program should not leave all controllers
broadly cross-origin callable by default. Update the CORS configuration around
builder.Services.AddCors() and the policy usage in the controller pipeline to
use a named policy that only allows the specific demo origin(s), headers, and
methods needed, or remove CORS entirely if the app is same-origin. Use the
existing setup points in Program and the controller mapping where the policy is
applied to keep the change scoped.
🪄 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: 0084a955-d3d0-42b0-818d-144fe8d14435
⛔ Files ignored due to path filters (3)
src/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/bit-bw-64.pngis excluded by!**/*.pngsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/favicon.icois excluded by!**/*.icosrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/icon-512.pngis excluded by!**/*.png
📒 Files selected for processing (52)
src/Bswup/Bit.Bswup.Demo/Pages/HomePage.razorsrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup.slnxsrc/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/FullDemo/Client/App.razorsrc/Bswup/FullDemo/Client/Bit.Bswup.FullDemo.Client.csprojsrc/Bswup/FullDemo/Client/Pages/Home.razorsrc/Bswup/FullDemo/Client/Properties/launchSettings.jsonsrc/Bswup/FullDemo/Client/Routes.razorsrc/Bswup/FullDemo/Client/Shared/MainLayout.razorsrc/Bswup/FullDemo/Client/_Imports.razorsrc/Bswup/FullDemo/Server/Bit.Bswup.FullDemo.Server.csprojsrc/Bswup/FullDemo/Server/Components/App.razorsrc/Bswup/FullDemo/Server/Components/_Imports.razorsrc/Bswup/FullDemo/Server/Pages/_Host.cshtmlsrc/Bswup/FullDemo/Server/Pages/_Layout.cshtmlsrc/Bswup/FullDemo/Server/Program.cssrc/Bswup/FullDemo/Server/Properties/launchSettings.jsonsrc/Bswup/FullDemo/Server/Startup/Middlewares.cssrc/Bswup/FullDemo/Server/Startup/Services.cssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Bit.Bswup.NewDemo.Client.csprojsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/DemoBswupProgressBar.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/DemoBswupProgressBar.razor.csssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/MainLayout.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/MainLayout.razor.csssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Pages/Counter.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Pages/Home.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Program.cssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Routes.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/_Imports.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/app.csssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/manifest.jsonsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/service-worker.jssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/service-worker.published.jssrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Bit.Bswup.NewDemo.csprojsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/App.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/Pages/Error.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/_Imports.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Program.cssrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Properties/launchSettings.jsonsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/appsettings.Development.jsonsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/appsettings.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (8)
- src/Bswup/FullDemo/Client/App.razor
- src/Bswup/FullDemo/Server/Startup/Services.cs
- src/Bswup/FullDemo/Server/Pages/_Host.cshtml
- src/Bswup/FullDemo/Server/Startup/Middlewares.cs
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
- src/Bswup/FullDemo/Server/Pages/_Layout.cshtml
- src/Bswup/FullDemo/Client/Properties/launchSettings.json
- src/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/App.razor
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. ✅ Action performedFull review finished. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Bswup/FullDemo/Server/Components/App.razor (1)
40-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProgress script still loads before
DemoBswupProgressBaris rendered.This exact ordering was flagged in a prior review and remains unfixed here.
bit-bswup.progress.js'sautoStart()readsdata-bit-bswup-*attributes off#bit-bswupat script-load time and bails out if the element isn't present yet, so loading the script before the component renders its markup means auto-initialization silently fails on first parse.🐛 Proposed fix
- <script src="_content/Bit.Bswup/bit-bswup.progress.js"></script> - <DemoBswupProgressBar /> + <DemoBswupProgressBar /> + <script src="_content/Bit.Bswup/bit-bswup.progress.js"></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/FullDemo/Server/Components/App.razor` around lines 40 - 41, The progress script is still being loaded before DemoBswupProgressBar renders, so autoStart in bit-bswup.progress.js can run before `#bit-bswup` exists and silently fail. Move the script include to after the DemoBswupProgressBar component in App.razor, or otherwise ensure the component markup is rendered before the script executes, so the data-bit-bswup-* attributes are present when the script initializes.
🧹 Nitpick comments (2)
src/Bswup/FullDemo/Server/Components/App.razor (1)
43-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out dead markup.
This block is unreachable Razor markup left behind; consider deleting it now that
DemoBswupProgressBaris the active implementation.🤖 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/FullDemo/Server/Components/App.razor` around lines 43 - 48, Remove the dead commented-out Razor markup in App.razor, since DemoBswupProgressBar is now the active implementation. Delete the obsolete <link> and <BswupProgress> block rather than leaving it commented so the App component stays clean and easier to maintain.src/Bswup/FullDemo/Server/Program.cs (1)
42-42: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRemove the blanket CORS policy
The demo is served from the same server/origin, so
AllowAnyOrigin().AllowAnyHeader().AllowAnyMethod()is broader than needed here. Prefer a named policy with the specific client origin, or drop CORS entirely if no cross-origin calls are expected.🤖 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/FullDemo/Server/Program.cs` at line 42, The current CORS setup in Program is overly permissive because app.UseCors is allowing any origin, header, and method. Replace this blanket policy by configuring a named CORS policy with the specific client origin used by the demo, and apply that policy in the pipeline; if the app only serves same-origin requests, remove the CORS middleware entirely. Use the existing app.UseCors call in the server startup flow to locate and update the policy.
🤖 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/FullDemo/Server/Components/App.razor`:
- Around line 40-41: The progress script is still being loaded before
DemoBswupProgressBar renders, so autoStart in bit-bswup.progress.js can run
before `#bit-bswup` exists and silently fail. Move the script include to after the
DemoBswupProgressBar component in App.razor, or otherwise ensure the component
markup is rendered before the script executes, so the data-bit-bswup-*
attributes are present when the script initializes.
---
Nitpick comments:
In `@src/Bswup/FullDemo/Server/Components/App.razor`:
- Around line 43-48: Remove the dead commented-out Razor markup in App.razor,
since DemoBswupProgressBar is now the active implementation. Delete the obsolete
<link> and <BswupProgress> block rather than leaving it commented so the App
component stays clean and easier to maintain.
In `@src/Bswup/FullDemo/Server/Program.cs`:
- Line 42: The current CORS setup in Program is overly permissive because
app.UseCors is allowing any origin, header, and method. Replace this blanket
policy by configuring a named CORS policy with the specific client origin used
by the demo, and apply that policy in the pipeline; if the app only serves
same-origin requests, remove the CORS middleware entirely. Use the existing
app.UseCors call in the server startup flow to locate and update the policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 914d74d8-9dc1-41f5-92ed-ecd22c13b6eb
⛔ Files ignored due to path filters (3)
src/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/bit-bw-64.pngis excluded by!**/*.pngsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/favicon.icois excluded by!**/*.icosrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/icon-512.pngis excluded by!**/*.png
📒 Files selected for processing (52)
src/Bswup/Bit.Bswup.Demo/Pages/HomePage.razorsrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.jssrc/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.jssrc/Bswup/Bit.Bswup.slnxsrc/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/FullDemo/Client/App.razorsrc/Bswup/FullDemo/Client/Bit.Bswup.FullDemo.Client.csprojsrc/Bswup/FullDemo/Client/Pages/Home.razorsrc/Bswup/FullDemo/Client/Properties/launchSettings.jsonsrc/Bswup/FullDemo/Client/Routes.razorsrc/Bswup/FullDemo/Client/Shared/MainLayout.razorsrc/Bswup/FullDemo/Client/_Imports.razorsrc/Bswup/FullDemo/Server/Bit.Bswup.FullDemo.Server.csprojsrc/Bswup/FullDemo/Server/Components/App.razorsrc/Bswup/FullDemo/Server/Components/_Imports.razorsrc/Bswup/FullDemo/Server/Pages/_Host.cshtmlsrc/Bswup/FullDemo/Server/Pages/_Layout.cshtmlsrc/Bswup/FullDemo/Server/Program.cssrc/Bswup/FullDemo/Server/Properties/launchSettings.jsonsrc/Bswup/FullDemo/Server/Startup/Middlewares.cssrc/Bswup/FullDemo/Server/Startup/Services.cssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Bit.Bswup.NewDemo.Client.csprojsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/DemoBswupProgressBar.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/DemoBswupProgressBar.razor.csssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/MainLayout.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Layout/MainLayout.razor.csssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Pages/Counter.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Pages/Home.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Program.cssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/Routes.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/_Imports.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/app.csssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/manifest.jsonsrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/service-worker.jssrc/Bswup/NewDemo/Bit.Bswup.NewDemo.Client/wwwroot/service-worker.published.jssrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Bit.Bswup.NewDemo.csprojsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/App.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/Pages/Error.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/_Imports.razorsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Program.cssrc/Bswup/NewDemo/Bit.Bswup.NewDemo/Properties/launchSettings.jsonsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/appsettings.Development.jsonsrc/Bswup/NewDemo/Bit.Bswup.NewDemo/appsettings.jsonsrc/Bswup/README.md
💤 Files with no reviewable changes (8)
- src/Bswup/FullDemo/Client/Properties/launchSettings.json
- src/Bswup/Bit.Bswup.Demo/wwwroot/service-worker.published.js
- src/Bswup/FullDemo/Client/App.razor
- src/Bswup/FullDemo/Server/Startup/Middlewares.cs
- src/Bswup/FullDemo/Server/Startup/Services.cs
- src/Bswup/FullDemo/Server/Pages/_Host.cshtml
- src/Bswup/FullDemo/Server/Pages/_Layout.cshtml
- src/Bswup/NewDemo/Bit.Bswup.NewDemo/Components/App.razor
Member
Author
|
@coderabbitai re-review |
|
Note: CodeRabbit uses incremental reviews and won't re-review commits that have already been reviewed unless automatic reviews are paused first. If you've pushed new commits since the last review, this will pick those up; otherwise there may be nothing new to report. ✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview 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.updateNotFound,updateCheckFailed,error), plus enhancedforceRefreshwith cache filtering.