Skip to content

feat(pwa): implement desktop_mode_pwa_force_replace_sw filter to manage service worker conflicts#242

Merged
AllTerrainDeveloper merged 1 commit into
trunkfrom
fix-pwa-install-issues
May 18, 2026
Merged

feat(pwa): implement desktop_mode_pwa_force_replace_sw filter to manage service worker conflicts#242
AllTerrainDeveloper merged 1 commit into
trunkfrom
fix-pwa-install-issues

Conversation

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator

@AllTerrainDeveloper AllTerrainDeveloper commented May 18, 2026

Might close #239 ?

Open WordPress Playground Preview

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

AllTerrainDeveloper commented May 18, 2026

@claude would you mind reviewing the code and checking if the patch would solve issue referenced in the pull request? I have no clue

@claude
Copy link
Copy Markdown

claude Bot commented May 18, 2026

Claude finished @AllTerrainDeveloper's task in 2m 53s —— View job


PR Review: desktop_mode_pwa_force_replace_sw filter

Tasks

  • Gather context and understand the request
  • Read changed PHP files (includes/pwa.php, includes/render/assets.php)
  • Read changed TypeScript files (src/pwa/sw-register.ts, src/pwa/install.ts, src/pwa/index.ts, src/types.ts)
  • Read documentation changes (docs/hooks-reference.md, docs/pwa.md)
  • Read test files (tests/phpunit/..., tests/vitest/...)
  • Review existing PWA infrastructure for context
  • Post comprehensive review

Does this solve issue #239?

Yes. Issue #239 describes a site with another PWA plugin's service worker already at root scope — desktop-mode's registration bail was silent and the install tile fell back to the generic "not available" toast, leaving the user with no actionable path. This PR solves it end-to-end:

  1. Detection: registerServiceWorker now inspects getRegistrations() before registering and flips _status to 'foreign-sw' when a foreign SW is found.
  2. User-facing feedback: onTileClick checks the status and shows a focused toast naming desktop_mode_pwa_force_replace_sw as the operator-side fix — instead of the generic fallback.
  3. Operator escape hatch: The new PHP filter lets operators opt in to replacing the foreign SW with a single line of code.

The end-to-end flow (PHP filter → config array → JS config → bootstrapPwaregisterServiceWorker) is correctly wired.


Code quality

includes/pwa.phpdesktop_mode_pwa_force_replace_sw()

Clean implementation matching the codebase's existing filter-wrapper pattern. The (bool) cast is correct and the PHP test correctly verifies coercion of non-bool filter returns (e.g. '1', ''). The function and filter docblock are well-written.

includes/render/assets.php

'forceReplaceSw' => desktop_mode_pwa_force_replace_sw() is placed correctly inside the pwa config array (line 492). The inline comment explains the connection to sw-register.ts.

src/types.tsPwaConfig.forceReplaceSw?: boolean

Correctly typed as optional to avoid breaking older PHP builds that don't emit this field.

src/pwa/index.tsbootstrapPwa

forceReplace: !! config.pwa.forceReplaceSw — the double negation is a reasonable defensive guard even though PHP already sends a bool.

src/pwa/sw-register.ts

The foreign SW detection logic (lines 179–198) is solid:

  • getRegistrations() is used to snapshot all existing SWs before registering.
  • The detection correctly checks both active?.scriptURL and installing?.scriptURL, covering the case where a foreign SW is mid-install.
  • url !== config.swUrl correctly excludes our own SW when it's already registered (avoiding false positives on a page reload).
  • .catch(() => []) fails open — if getRegistrations throws, we proceed to register rather than silently bailing. This is the right default.

The SwRegistrationStatus type export is clean and getSwRegistrationStatus() gives a synchronous read needed by the install tile.

src/pwa/install.ts — foreign-SW toast⚠️ Minor concern

The toast at lines 253–259 is user-facing to all desktop-mode users who click the install tile. The message "A site admin can opt in by setting the desktop_mode_pwa_force_replace_sw filter to true" is useful for admins, but non-admin users clicking the tile will see a raw PHP filter name that they cannot act on. Since install.ts doesn't receive capability info, this can't be fixed within the module without plumbing isAdmin through from config. That said, desktop-mode itself requires manage_options or equivalent capability by default, so in practice the audience seeing this toast will typically be admins. Not a blocker, but worth flagging as a future refinement.


Documentation

docs/hooks-reference.md ✅ — Filter entry placed correctly, marked Stable (justified for a boolean toggle with a simple contract), and usage example is correct.

docs/pwa.md ✅ — The new section accurately describes the foreign-SW bail behavior, the actionable toast, and the filter usage.

docs/examples/pwa-install.md ⚠️ Missing update

Per CLAUDE.md:

pwa-install.md — UPDATE WHEN: wp.desktop.pwa.promptInstall(), install-pill UX, or desktop_mode_pwa_manifest filter contract changes.

The install-pill UX did change (new foreign-SW toast path). The current example in pwa-install.md lists three reasons promptInstall() returns 'unavailable' (no beforeinstallprompt yet, already installed, browser unsupported) — a foreign-SW conflict is now a fourth. Consider adding a note there, and optionally adding a note to the existing "Trigger the prompt only when truly available" section. Fix this →


Tests

PHP tests (desktopModePwaForceReplaceSw.php) ✅

Three well-focused tests covering default false, filter opt-in, and non-boolean coercion. tear_down cleans up the filter correctly.

Vitest tests (pwa-install-foreign-sw.test.ts) ✅ with one minor note

Coverage is good:

  • Foreign SW → 'foreign-sw' status ✅
  • Clean origin → 'registered' + correct registration call ✅
  • forceReplace: true bypasses the guard and registers ✅
  • Toast message for 'foreign-sw' status ✅
  • Toast message for non-foreign status uses the generic fallback ✅

Minor: the forceReplace test (line 141) verifies the end result (status 'registered', register called) but doesn't assert that getRegistrations was not called — which would directly prove the guard was bypassed. The existing assertions are sufficient, but the additional expect(handle.registrations).not.toHaveBeenCalled() equivalent would make the intent clearer.


Summary

The implementation is correct and complete for the stated goal. The PHP → config → JS pipeline is properly wired, the status tracking is clean, tests cover the critical paths, and the hook and architecture docs are updated. The one actionable omission before merge is updating docs/examples/pwa-install.md per the project's CLAUDE.md rules (install-pill UX changes must be reflected there). Everything else is either minor or a future improvement.

@github-actions
Copy link
Copy Markdown

✅ WordPress Plugin Check Report

✅ Status: Passed

📊 Report

All checks passed! No errors or warnings found.


🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@AllTerrainDeveloper AllTerrainDeveloper merged commit 45537f7 into trunk May 18, 2026
5 checks passed
@AllTerrainDeveloper AllTerrainDeveloper deleted the fix-pwa-install-issues branch May 18, 2026 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Button Install Instance as an App doesn´t work

1 participant