@claude can you review this?

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

Code Review

• Gather context and read changed files
• Review includes/compat/divi.php
• Review tests/phpunit/tests/diviCompat.php
• Review desktop-mode.php changes
• Review docs/plugin-compat-layer.md changes
• Post full review

The implementation approach is correct and well-reasoned. The diagnosis of both root causes (missing deps + cross-frame et_gb pointer) is accurate and matches the established plugin-compat-layer.md pattern. One bug in the test will cause a test failure in CI; the rest are minor notes.

Bug — wrong user-meta key in force_chromeless() (test will fail)

tests/phpunit/tests/diviCompat.php:39

update_user_meta( $user_id, 'wp_desktop_mode', '1' );
//                           ^^^^^^^^^^^^^^^^^^^
//                           wrong key

desktop_mode_is_chromeless_request() calls desktop_mode_is_enabled() first, which checks for the meta key desktop_mode_mode (see includes/helpers.php:41). Every other test in the suite that primes this state uses the correct key — e.g., desktopModeRender.php:42, desktopModePortal.php:241.

Because of the wrong key, desktop_mode_is_enabled() returns false → desktop_mode_is_chromeless_request() returns false → the wp_add_inline_script() branch is never taken → test_chromeless_request_appends_et_gb_window_override will fail with assertStringContainsString( 'window.et_gb = window;', … ).

As a side-effect, test_classic_request_does_not_append_et_gb_override passes but for the wrong reason (the chromeless guard never fires at all, not because it correctly identifies a non-chromeless request).

Fix this →

The fix is a one-liner:

// Before
update_user_meta( $user_id, 'wp_desktop_mode', '1' );

// After
update_user_meta( $user_id, 'desktop_mode_mode', '1' );

Minor notes

includes/compat/divi.php:110 — function_exists() guard is unnecessary

if ( function_exists( 'desktop_mode_is_chromeless_request' ) && desktop_mode_is_chromeless_request() ) {

routing.php is loaded unconditionally at line 33 of desktop-mode.php, and divi.php at line 85 — so desktop_mode_is_chromeless_request is always defined when this code runs. The guard is harmless, but it implies the function might be absent, which could confuse future readers. A plain call is sufficient.

tests/phpunit/tests/diviCompat.php — tear_down doesn't clean up desktop_mode_mode meta

Once the meta key is corrected, force_chromeless() will write desktop_mode_mode for a freshly created user. The WP unit test harness drops all created users and their meta automatically, so this isn't a real leak — but for clarity it's worth adding delete_user_meta alongside the unset( $_GET[…] ) in tear_down, consistent with how desktopModeRender.php:26 and desktopModePortal.php:38 clean up.

@since 0.18.x tag

Both docblocks use 0.18.x. This is presumably a placeholder to be updated before release. Fine to merge as-is and update on the release branch.

What looks good

• Correct hook priority. enqueue_block_editor_assets at priority 999 reliably runs after Divi's priority-4 registration and before the script loader prints tags.
• Object mutation is correct. $registration is a reference to the _WP_Dependency object; $registration->deps[] = $dep mutates the registered entry in-place.
• $existing snapshot prevents accidental duplication within a single call while still correctly detecting already-present deps on subsequent calls.
• Chromeless scope guard is right. Injecting window.et_gb = window only in chromeless requests preserves Divi's intended Cypress-iframe behavior in other contexts.
• Doc update is complete and accurate. The new Divi section in plugin-compat-layer.md correctly explains both root causes, references the fix file, and the new decision-tree step 5 gives future contributors a clear template.
• Tests cover all five meaningful cases (inject, no-op, idempotent, chromeless, non-chromeless) — just the wrong meta key.

Summary: One failing test due to wrong meta key (wp_desktop_mode → desktop_mode_mode). Fix that and this is ready to merge.

✅ 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

@claude can you review? we had to rework this, DIVI seems to not support being nested at all, and we couldn't add any kind of code injection to it.

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

Code Review

• Gather context on the reworked implementation
• Review includes/compat/divi.php
• Review tests/phpunit/tests/diviCompat.php
• Review wpd-confirm-dialog changes
• Review src/wpd-confirm.ts changes
• Post full review

The previous review's bugs (wrong meta key, missing tear_down cleanup, spurious function_exists guard) are all fixed. The rework itself is well-reasoned — abandoning Location.prototype patching in favour of click interception is the right call given how early Divi captures those references. Three things need attention before merge.

Bug — desktop_mode_compat_divi_eject_iframe_patch is double-registered

includes/compat/divi.php:398-399

add_action( 'admin_head',   'desktop_mode_compat_divi_eject_iframe_patch', 0 );
add_action( 'admin_footer', 'desktop_mode_compat_divi_eject_iframe_patch', 999 );

Both hooks fire on the same page request. The function has no emission guard, so the <script id="desktop-mode-compat-divi-vb-handoff"> block and its IIFE are injected twice.

At admin_head time document.readyState === 'loading', so the first IIFE adds a DOMContentLoaded listener. At admin_footer time document.readyState is interactive/complete, so the second IIFE calls bootstrap() directly. walkAndAttach(document) exits early on the second call (the __desktopModeDiviHandoffAttached guard), but bootstrap() then unconditionally creates a second MutationObserver on document.documentElement. That observer's callback also calls walkAndAttach, so it wastes work on every DOM mutation for the lifetime of the page.

If both hooks are intentional (head for early setup, footer to catch iframes Gutenberg may have added by then), a static flag prevents the double IIFE:

function desktop_mode_compat_divi_eject_iframe_patch() {
    static $emitted = false;
    if ( $emitted ) { return; }
    $emitted = true;
    // ... rest of function

Or alternatively, move the walkAndAttach guard into bootstrap() itself so a second call is cheap and no extra observer is created:

var bootstrapped = false;
function bootstrap() {
    if ( bootstrapped ) { return; }
    bootstrapped = true;
    // ...
}

Fix this →

Critical — plugin-compat-layer.md still describes the abandoned Location.prototype approach

The doc section heading says "Divi — eject to top-level for Visual Builder via Location.prototype patch", and the "Fix" block underneath it describes replacing Location.prototype.href's setter, Location.prototype.assign, and Location.prototype.replace. None of that exists in the code anymore. The actual implementation uses click interception keyed on visible button text content.

The "Why prototype patching instead of click-interception" subsection is now especially backwards — it lists three reasons the prototype approach was chosen over click-interception, when in fact click-interception is what shipped.

Also, the doc says the postMessage type is desktop-mode-divi-vb-eject; the code sends and listens for desktop-mode-divi-vb-handoff.

The doc update reflects the previous iteration of the implementation, not the rework. It needs to be rewritten to describe what the code actually does:

• Click-capture listener on document matching button text content (BUTTON, A, INPUT, SPAN elements whose trimmed lowercase text matches the BTN_TEXTS array)
• Walking same-origin nested iframes so the Gutenberg editor canvas is covered
• postMessage type desktop-mode-divi-vb-handoff to the parent shell
• The parent reshapes the URL (strips desktop_mode_chromeless, adds desktop_mode_classic) and confirms with the user before navigating window.top.location.href

Fix this →

Minor — window.confirm fallback in parent listener

includes/compat/divi.php:467

promptUser = Promise.resolve(
    window.confirm(
        'Divi cannot run inside Desktop Mode…'
    )
);

The parent listener is only emitted when desktop_mode_is_enabled() is true and the request is neither chromeless nor classic — i.e., exclusively on the shell page, where wp.desktop is always initialized before a user can interact with anything. The else branch is unreachable in practice, but it violates the spirit of CLAUDE.md's "never window.confirm" rule. Since the branch is dead code, removing it is the cleanest fix. If you want a belt-and-suspenders fallback, a no-op Promise.resolve(false) is better than a native dialog:

} else {
    promptUser = Promise.resolve( false );
}

What looks good

• Click-interception by text content is the right call. Divi captures Location.prototype early and the eject-on-navigate approach had unresolvable race conditions. The text-matching detection (BTN_TEXTS set) has been stable across Divi versions; using it as the trigger is pragmatic and low-fragility.
• User-facing UX — hideCancel: true + dismissable: true combination is honest: tells the user there's no workaround, lets them X out if they want to stay in Gutenberg, avoids a dead "Cancel" button that implies there's a choice.
• URL reshaping — stripping desktop_mode_chromeless and adding desktop_mode_classic is correct; without it the top-level nav would render headless again and confuse users.
• Same-origin guard on the message event — both origin check (ev.origin !== window.location.origin) and URL re-parse before navigating prevent foreign frames from triggering the handoff.
• desktop_mode_compat_divi_is_active() — clean, cheap, correctly guards the is_plugin_active call behind function_exists.
• Tests — all three of the previously reported issues (wrong meta key, tear_down cleanup, function_exists guard) are fixed. Coverage is thorough: 15 test methods covering the dep-injection, the et_gb override, the VB signal, the preloader bridge, and the eject/handoff pair.
• wpd-confirm-dialog additions (hide-cancel, dismissable, X close button) are well-implemented: correct shadow-DOM styling, correct event emission, correct test coverage for each new prop including the interaction between hideCancel + dismissable.

Summary: Two changes are needed before merge — the doc section describing the Location.prototype strategy must be rewritten to describe the click-interception approach that actually shipped, and the double-registration of desktop_mode_compat_divi_eject_iframe_patch needs a static emission guard. The window.confirm fallback is a minor cleanup. Everything else is solid.

@claude is it ok now?

Claude finished @AllTerrainDeveloper's task in 58s —— View job

Code Review

• Read includes/compat/divi.php — check double-registration fix
• Read docs/plugin-compat-layer.md — check doc rewrite
• Check window.confirm fallback
• Post findings

Yes — all three issues from the last review are fixed. Here's the rundown:

Double-registration (was: Bug)

The previous version registered the same function on both admin_head and admin_footer, creating a second MutationObserver. That's gone. The code is now correctly split into two separate functions with distinct responsibilities:

• desktop_mode_compat_divi_eject_iframe_patch → admin_head priority 0 (iframe-side click handler)
• desktop_mode_compat_divi_eject_parent_listener → admin_footer priority 1 (shell-side message listener)

Each has its own early-return guards so it only fires in the right context. Clean.

Doc section (was: Critical)

The plugin-compat-layer.md section is fully rewritten. It now correctly describes the click-interception approach with BTN_TEXTS, the capture-phase listener, the same-origin iframe walk, and postMessage type desktop-mode-divi-vb-handoff. The abandoned Location.prototype approach is gone. The new "Why click-by-text-content instead of patching navigation primitives" subsection gives future contributors the full reasoning.

window.confirm fallback (was: Minor)

Replaced with Promise.resolve( false ) with a comment explaining why (lines 464–472). The comment is clear, doesn't overexplain, and accurately describes the branch as unreachable in practice. Correct.

This is ready to merge.

@claude what's wrong?

Claude finished @AllTerrainDeveloper's task in 1m 30s —— View job

Checking current state

• Check recent commits and current branch state
• Analyze latest commit impact on tests
• Post findings

@claude you havent finished your review

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

What was wrong

CI was failing with 4 PHPUnit failures, all in Tests_DesktopMode_DiviCompat, all asserting that desktop_mode_compat_divi_vb_iframe_signal() emitted output (containing __Cypress__, desktop-mode-compat-divi-vb, etc.) when it actually emitted nothing.

Root cause: Commit 37edcd7 added a desktop_mode_compat_divi_is_active() guard near the top of desktop_mode_compat_divi_vb_iframe_signal() (after the is_admin() and desktop_mode_is_enabled() guards). Four tests that test the positive path of this function were never updated to call $this->activate_divi_theme() before capturing output. The function now bails early for all four, returning empty string.

The two tests that check for empty output (test_vb_iframe_signal_skips_admin_requests, test_vb_iframe_signal_skips_when_desktop_mode_disabled) were unaffected because their earlier guards (is_admin() and !desktop_mode_is_enabled()) fire before the Divi check.

Fix: Added $this->activate_divi_theme() to the four failing tests:

• test_vb_iframe_signal_emits_on_front_end_for_desktop_user
• test_vb_top_frame_emits_preloader_bridge
• test_inner_app_frame_skips_preloader_bridge
• test_inner_app_frame_bails_when_parent_equals_top

Pushed as 74f06e9. CI should be green now.
| Branch: fix-divi-init