fix(electron): disable service worker in desktop runtime#1066
Conversation
Greptile SummaryThis PR prevents Angular's
Confidence Score: 4/5Safe to merge with awareness that users upgrading from a version that already registered a file-origin SW will not have it automatically cleaned up by this change alone. The runtime guards and test coverage are correct for all new registrations. The gap is that provideServiceWorker(enabled: false) does not unregister a service worker that was previously installed under a file: origin in an earlier Electron build — affected users would still have a stale SW in control after updating until it is explicitly removed. apps/web/src/app/services/runtime-config.ts — the new shouldEnableServiceWorker guards are correct, but the call site in app.config.ts has no companion unregistration logic for existing stale SW registrations. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App startup\napp.config.ts] --> B[shouldEnableServiceWorker]
B --> C{production?}
C -- No --> Z[SW disabled]
C -- Yes --> D{navigator has\nserviceWorker?}
D -- No --> Z
D -- Yes --> E{electronBridge\npresent?}
E -- Yes --> Z
E -- No --> F{protocol === 'file:'?}
F -- Yes --> Z
F -- No --> G[SW enabled\nngsw-worker.js registered]
subgraph getDefaultServiceWorkerRuntimeContext
H[globalThis.window?.electron] --> I[electronBridge]
J[window?.location?.protocol\n?? globalThis.location?.protocol] --> K[protocol]
end
Reviews (2): Last reviewed commit: "fix(electron): disable service worker in..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ed2a71c7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !runtimeContext.electronBridge && | ||
| runtimeContext.protocol !== 'file:' |
There was a problem hiding this comment.
When a user upgrades from a desktop version that already registered ngsw-worker.js, returning false here only prevents this version's Angular initializer from registering again; it does not unregister an active registration or clear NGSW caches. The old file-origin worker can control the first file:// navigation and serve the previous index.html/chunks before this new runtime check ever runs, so affected Electron installs can remain pinned to stale UI until userData is cleared. Add cleanup from the Electron main/session side, or an enabled renderer cleanup path, for existing registrations/caches.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('disables service worker for Electron runtime', () => { | ||
| expect( | ||
| shouldEnableServiceWorker( | ||
| true, | ||
| { serviceWorker: {} } as Navigator, | ||
| { | ||
| electronBridge: {}, | ||
| protocol: 'file:', | ||
| } | ||
| ) | ||
| ).toBe(false); | ||
| }); |
There was a problem hiding this comment.
electronBridge-only guard
The Electron test always supplies both electronBridge and protocol: 'file:', so neither condition is ever isolated. If an Electron build ever uses a non-file: scheme (e.g. app:// or a custom iptvnator:// protocol), the only guard that fires is !electronBridge, but no test exercises that path. Adding a case where electronBridge is set and protocol is 'https:' (or any non-file: value) would pin that contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/app/services/runtime-config.spec.ts
Line: 40-51
Comment:
**Missing coverage: `electronBridge`-only guard**
The Electron test always supplies both `electronBridge` and `protocol: 'file:'`, so neither condition is ever isolated. If an Electron build ever uses a non-`file:` scheme (e.g. `app://` or a custom `iptvnator://` protocol), the only guard that fires is `!electronBridge`, but no test exercises that path. Adding a case where `electronBridge` is set and `protocol` is `'https:'` (or any non-`file:` value) would pin that contract.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
bbd54c6 to
ce78669
Compare
1 participant
Summary
file:origin.Changes
file:protocol before enablingngsw-worker.js.serviceworkersandcachestoragebefore packagedloadFile.Testing
pnpm nx test electron-backend -- app.spec.ts --runInBandpnpm nx test web -- runtime-config.spec.ts --runInBandpnpm nx lint electron-backend --skipNxCachepnpm nx lint web --skipNxCachepnpm nx build electron-backend --configuration=production --skipNxCacheNX_TASKS_RUNNER_DYNAMIC_OUTPUT=false pnpm nx run electron-backend:make --prepackageOnly --platform=mac --arch=arm64 --skipNxCacheagent-browser --cdp 9222: dashboard rendered, cleanup trace ran before renderer load, no service worker registration/controller/cache keysgit diff --checkDiagnostic Note
This PR currently opens DevTools by default in packaged Electron builds. Remove
chore(electron): open devtools for packaged diagnosticsbefore merging a normal release build.