Skip to content

Add window notices feature with persistent dismissal and server sync#228

Merged
AllTerrainDeveloper merged 2 commits into
trunkfrom
new-notice-component
May 16, 2026
Merged

Add window notices feature with persistent dismissal and server sync#228
AllTerrainDeveloper merged 2 commits into
trunkfrom
new-notice-component

Conversation

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator

@AllTerrainDeveloper AllTerrainDeveloper commented May 16, 2026

  • Introduced desktop_mode_register_window_notice() for registering declarative window notices.
  • Created <wpd-notice> web component for rendering notices with customizable tones, messages, and dismissibility.
  • Implemented localStorage management for user-specific notice dismissal.
  • Added server-side synchronization for window notices to reflect plugin changes without page reloads.
  • Developed comprehensive tests for the <wpd-notice> component and its functionality.
Open WordPress Playground Preview
- Introduced `desktop_mode_register_window_notice()` for registering declarative window notices.
- Created `<wpd-notice>` web component for rendering notices with customizable tones, messages, and dismissibility.
- Implemented localStorage management for user-specific notice dismissal.
- Added server-side synchronization for window notices to reflect plugin changes without page reloads.
- Developed comprehensive tests for the `<wpd-notice>` component and its functionality.
@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

@claude can you review this please?

@claude
Copy link
Copy Markdown

claude Bot commented May 16, 2026

Claude finished @AllTerrainDeveloper's task in 3m 59s —— View job


PR Review: Add window notices feature with persistent dismissal and server sync

Todo list:

  • Gather context and read changed files
  • Review PHP backend (includes/window-notices.php, includes/core/payload.php)
  • Review TypeScript frontend (src/window-notices.ts, src/window-notices-server-sync.ts, src/types.ts)
  • Review <wpd-notice> web component (wpd-notice.ts, wpd-notice.styles.ts, storage.ts)
  • Review tests (PHP + TS)
  • Review documentation (docs/examples/window-notice.md, docs/hooks-reference.md)
  • Review integration points (src/desktop.ts, src/api/facade.ts, src/menu-refresh-apply.ts)
  • Post comprehensive feedback

Overall this is a well-structured feature — the layering mirrors the existing command/settings-tab/slot pattern, the component is clean, and the PHP side sanitizes correctly. A few things need addressing before merge.


Critical — documentation gap

javascript-reference.md was not updated. Per the project's own rules (CLAUDE.md: "UPDATE WHEN: adding… any wp.desktop.* method/property"), five new methods on wp.desktop must be documented there:

  • registerWindowNotice( entry )
  • unregisterWindowNotice( id )
  • listWindowNotices()
  • dismissWindowNotice( id )
  • undismissWindowNotice( id )

They're in facade.ts and the reserved-namespace allowlist, but javascript-reference.md has no entry for any of them. Plugin authors relying only on that doc won't know these exist. Fix this →


Major — match AND/OR semantics are undocumented

buildMatcher in src/window-notices-server-sync.ts:49-61 applies AND logic when multiple selector types are present: specifying both windows: ['edit-php'] and urlContains: 'wc-admin' means the window must match the id and the URL. The PHP docblock says "Combine any of:" which implies OR to most readers. The example doc (docs/examples/window-notice.md) never shows the two combined.

This should be spelled out explicitly in both the PHP docblock and the example doc — something like: "When both an id selector and urlContains are specified, both must match (AND)." Fix this →


Major — no TypeScript tests for the server-sync module

src/window-notices-server-sync.ts is the most complex piece of new code: buildMatcher has 5 distinct branches and applyServerWindowNotices has owner-based reconciliation logic. Neither is covered by tests. The wpd-notice.test.ts only tests the component.

Consider adding a window-notices-server-sync.test.ts covering at minimum:

  • buildMatcher with window, windows, urlContains, and their combination
  • applyServerWindowNotices adding, updating, and removing notices
  • that JS-registered notices (non-__server__ owner) survive a sync

Fix this →


Minor — icon field has no validation in PHP

includes/window-notices.php:139 stores icon as a raw cast string without any validation. It's emitted in the JSON payload and becomes a CSS class name inside shadow DOM, so there's no XSS risk, but any garbage string silently makes it through to the browser. The other string fields get validated (id via regex, tone via allowlist); icon should at minimum receive a dashicons-* pattern check, or the absence of validation should be explicitly documented. Fix this →


Minor — stability marker missing from hooks-reference.md section

Every other section in hooks-reference.md opens with a ### Foo — Stable / ### Foo — Experimental header. The window-notices section (line ~2335) has ### Window notices (since 0.22.0) — no stability label. Given that the component itself carries status: 'experimental' in its static help, the hooks section should say Experimental too, or decide on Stable and be consistent. Fix this →


Minor — dismissible="false" legacy path from day one

wpd-notice.ts:159 accepts dismissible="false" as a string attribute:

const dismissible = ! ( notDismissibleAttr || dismissibleAttr === 'false' );

The comment calls this a "legacy" path, but there's no legacy here — this is the first shipping version. Starting with a dual API (not-dismissible boolean attribute and dismissible="false" string) adds surface area for no benefit, and since the status is experimental it can still be cleaned up. Consider removing the dismissibleAttr === 'false' branch and only keeping not-dismissible. Fix this →


Minor — desktop_mode_flush_window_notice_registry lacks @internal

includes/window-notices.php:190 exposes desktop_mode_flush_window_notice_registry() as a public, unguarded function. The companion registry accessor desktop_mode_window_notice_registry() is marked @internal, but the flush helper is not. A plugin author could accidentally call it and wipe all registered notices. Add @internal (or prefix with _) and add a note that it's only for PHPUnit set_up().


Nit — PHP tests don't verify match field round-trip

test_menu_payload_includes_server_window_notices checks that the notice appears in the payload but doesn't assert that the match, icon, or order fields are preserved through desktop_mode_build_window_notices_payload(). Worth adding a quick assertion that those non-default fields survive the round-trip.


What's good ✓

  • Pattern fidelity — the module faithfully mirrors commands/server-sync.ts and settings/server-sync.ts in structure, owner-tag convention, and live-unregister behavior.
  • createSharedStore wiring is correct — cross-bundle state will work.
  • wp_kses_post() on PHP messages prevents <script> injection.
  • Per-user localStorage key (desktop-mode-notice-dismissed:<userId>) is the right design — shared-browser users won't see each other's dismissals.
  • storage.ts failure-mode handling is good — private-mode Safari and quota-exceeded paths fall back silently without breaking the render.
  • Test coverage for the <wpd-notice> component itself is thorough.
  • Documentation at docs/examples/window-notice.md is clear and practical.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 16, 2026

✅ 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 enabled auto-merge (squash) May 16, 2026 14:25
@AllTerrainDeveloper AllTerrainDeveloper merged commit e8399fa into trunk May 16, 2026
5 checks passed
@AllTerrainDeveloper AllTerrainDeveloper deleted the new-notice-component branch May 16, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant