Search: Reliably sync experience picker changes when Instant Search is off by kangzj · Pull Request #48745 · Automattic/jetpack

kangzj · 2026-05-12T23:06:11Z

Fixes https://linear.app/a8c/issue/SEARCH-186/search-sync-module-not-registered-when-instant-search-is-disabled

Why

When Instant Search is off, flipping jetpack_search_experience or turning Instant Search back on never reached WPcom — those writes were silently dropped by Sync on the source site. As a result, the experience picker in the dashboard would appear to save successfully but the cache-site option stayed stale, breaking the round-trip wired up in #48540 + Automattic/wpcom#215433.

Two issues conspired:

Actions::initialize_search() skipped registering the Search sync module on requests where instant_search_enabled was false at plugins_loaded time — so the option whitelist was missing exactly when it was needed.
Module_Control::update_experience('inline') deleted jetpack_search_experience, but delete_option() is a no-op when the option doesn't exist (it does not fire deleted_option), so a fresh site toggling to inline produced no sync event at all.

Proposed changes

packages/sync — drop the is_instant_search_enabled() gate in Actions::initialize_search().

The Search sync module is what adds instant_search_enabled, jetpack_search_experience, and the rest of $options_to_sync to the local sync options whitelist. Gating its registration on the very flag it owns is a chicken-and-egg — the request that flips Instant Search from off → on never sees Search sync loaded, so the option write is filtered out of the queue.
Gating instead on the module-level is_active() has the same hole at the off → embedded / off → overlay boundary, so registration is now unconditional.
The existing class_exists( Module_Control::class ) guard is unchanged — it tracks package presence (autoloader concern), not module activation, so sites without the Search package still skip Search sync entirely.
Added regression test Actions_Test::test_initialize_search_registers_module_regardless_of_instant_search_flag covering both instant_search_enabled states.

packages/search — update_experience('inline') now writes '' to jetpack_search_experience instead of deleting it.

Writing the empty string guarantees added_option / updated_option fires on every transition into inline (including from the "option missing" state), so Sync always has something to queue.
get_experience() already resolves '' (and missing) to EXPERIENCE_INLINE via the existing fall-through to the instant_search_enabled legacy check — no reader migration needed.
Updated test_update_experience_inline_* to assert the new storage shape and added test_update_experience_inline_fires_action_when_option_missing to lock in the action-firing contract.

Related product discussion/links

https://linear.app/a8c/issue/SEARCH-186/search-sync-module-not-registered-when-instant-search-is-disabled
Companion changes: Search 3.0: backend support for experience field in settings API (RSM-2291) #48540 + 215433-ghe-Automattic/wpcom (RSM-2291 / RSM-2535)

Does this pull request change what data or activity we track or use?

No. The Search options were already whitelisted in this package and in the WPcom shadow replicastore. This PR only ensures the existing whitelist is in effect during requests where instant_search_enabled happens to be off when plugins_loaded fires, and that the inline transition always produces a sync-able write. No new keys, no schema changes.

Verification

Reproduced the diagnostic from the linked issue on a connected, online site (instant_search_enabled = false at request start). Trunk drops the search option keys from the whitelist; this branch keeps them.

Trunk (bug)

$ wp eval '
  update_option( "instant_search_enabled", false );
  \Automattic\Jetpack\Sync\Actions::initialize_search();
  \Automattic\Jetpack\Sync\Modules::get_modules();
  var_dump( in_array( "jetpack_search_experience", \Automattic\Jetpack\Sync\Defaults::get_options_whitelist(), true ) );
  var_dump( in_array( "instant_search_enabled",   \Automattic\Jetpack\Sync\Defaults::get_options_whitelist(), true ) );
'
bool(false)
bool(false)

This branch (fixed)

$ wp eval '<same script as above>'
bool(true)
bool(true)

Same result with instant_search_enabled = true (no regression on the previously-working path).

Unit tests for both packages pass locally:

$ composer phpunit -- --filter Actions_Test            # packages/sync
OK (13 tests, 24 assertions)

$ composer phpunit -- --filter Module_Control_Test     # packages/search
OK (25 tests, 54 assertions)

Testing instructions

On a Jetpack-connected, non-offline-mode site running this branch + Automattic/wpcom#215433:

With Instant Search off, change jetpack_search_experience via POST /wp-json/jetpack/v4/search/settings (or the dashboard's experience picker). Confirm wp option get jetpack_search_experience matches on the source and on the WPcom cache-site option.
With Instant Search off, flip instant_search_enabled to true via the same endpoint. Confirm both instant_search_enabled and the lockstep jetpack_search_experience write land on WPcom (the WPcom audit log should now show updated_option events for both keys).
Verify the three transitions from the issue table all sync now:
- embedded → overlay (starting with instant_search_enabled=false)
- inline → embedded (regression check)
- overlay → inline (delete — regression check)
Fresh-site inline check: on a site that has never saved an experience (run wp option delete jetpack_search_experience first), switch to inline via the dashboard. Confirm wp option get jetpack_search_experience returns the empty string locally, the WPcom audit log shows an added_option / updated_option event, and the cache-site option is updated.
On a site without the Search package installed, confirm Modules::get_module( 'search' ) still returns false (the class_exists guard is unchanged).

Sync + Search test suites pass:

jetpack test php packages/sync
jetpack test php packages/search
jetpack test php plugins/jetpack --testsuite=sync

Drops the `is_instant_search_enabled()` gate in initialize_search() so the Search options whitelist (instant_search_enabled, jetpack_search_experience, and the other Search settings) is in place during requests that toggle those options. Without this fix, the request that flipped instant_search_enabled from false to true never had Search sync registered, so the very option write that should have synced — plus the lockstep jetpack_search_experience write — were filtered out before they hit the local sync queue. The class_exists() guard on Module_Control still skips loading on sites that do not have the Search package installed.

github-actions · 2026-05-12T23:07:04Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the echo/search-186-sync-gate-removal branch.
To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack echo/search-186-sync-gate-removal

bin/jetpack-downloader test jetpack-mu-wpcom-plugin echo/search-186-sync-gate-removal

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

kangzj · 2026-05-12T23:07:07Z

@claude please review this PR.

github-actions · 2026-05-12T23:07:11Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
You can use GitHub's Reviewers functionality to request a review.
When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

WordPress.com Simple releases happen as soon as you deploy your changes after merging this PR (PCYsg-Jjm-p2).
WoA releases happen weekly.
Releases to self-hosted sites happen monthly:
- Scheduled release: June 2, 2026
- Code freeze: June 1, 2026

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

claude · 2026-05-12T23:07:27Z

Claude finished @kangzj's task in 1m 27s —— View job

PR Review

Gather context
Read changed files
Review code changes
Post review feedback

Overall: ✅ Looks good — correct fix for a real chicken-and-egg bug. Minor suggestion below.

Summary

The bug is real and the fix is correct. instant_search_enabled lives in the Search sync module's own $options_to_sync whitelist (class-search.php:1778). Gating the module's registration on is_instant_search_enabled() means a request where that option is false at plugins_loaded time never loads the sync module — so the very write that flips it from false → true is silently dropped from the queue.

Removing the is_instant_search_enabled() gate and keeping only the class_exists( Module_Control::class ) guard is the minimal correct fix and aligns initialize_search() with the established pattern used by initialize_woocommerce():

// initialize_woocommerce() — the established pattern
if ( false === class_exists( 'WooCommerce' ) ) {
    return;
}
add_filter( 'jetpack_sync_modules', array( __CLASS__, 'add_woocommerce_sync_module' ) );

Code review

projects/packages/sync/src/class-actions.php (lines 878–882)

The change is correct and the docblock accurately explains the why — durable inline context for future maintainers. No issues here.

Scope of behavior change

Before this fix, the entire $options_to_sync list was silently skipped for sites with instant_search_enabled = false. After the fix, all 12 Search options (including jetpack_search_color_theme, jetpack_search_experience, jetpack_search_ai_prompt_override, etc.) are always synced when the Search package is present. The PR description correctly notes these were already whitelisted; this just ensures the whitelist is active. No schema changes, no new keys.

Changelog

Significance: patch / Type: fixed is appropriate. Description is accurate.

One suggestion — add a regression test

The AGENTS.md guidance says "When adding new behaviour, always add a corresponding test." More importantly, this particular chicken-and-egg bug is the kind that could easily be re-introduced. A targeted test in tests/php/ (or the Jetpack plugin's sync suite) verifying that jetpack_sync_modules includes the Search module regardless of the instant_search_enabled value would lock in this fix:

public function test_search_sync_module_registered_when_instant_search_disabled() {
    // Simulate instant_search_enabled = false at request boot
    update_option( 'instant_search_enabled', false );

    // Re-run initialize_search (or test the filter directly)
    Actions::initialize_search();

    $modules = apply_filters( 'jetpack_sync_modules', array() );
    $this->assertContains(
        'Automattic\\Jetpack\\Sync\\Modules\\Search',
        $modules,
        'Search sync module must be registered even when instant_search_enabled is false'
    );
}

This is a suggestion, not a blocker — the manual verification described in the PR is solid and the fix itself is clean.

…notes

Locks in the SEARCH-186 fix — verifies the Search sync module is registered regardless of `instant_search_enabled`, so the option whitelist is in place during the request that flips that flag. Addressing review: claude[bot] suggested a targeted unit test.

kangzj · 2026-05-12T23:15:07Z

Addressing claude[bot]'s review (comment #IC_kwDOAOho7M8AAAABCGFBvg):

Thanks for the careful read. Added a targeted regression test in 12aadc2345966c0626191ebb06a6123719acf7ac — Actions_Test::test_initialize_search_registers_module_regardless_of_instant_search_flag covers both instant_search_enabled = false (the bug case) and true (regression check on the previously-working path).

Also expanded the docblock in b15e745d7ff to record that is_active() would reintroduce the same chicken-and-egg at the off → overlay boundary, and that the class_exists() guard tracks package presence (autoloader concern), not module activation.

kangzj · 2026-05-12T23:15:21Z

@claude please re-review.

claude · 2026-05-12T23:15:47Z

Claude finished @kangzj's task in 56s —— View job

PR Re-Review

Gather context
Read changed files
Review updated code changes
Post review feedback

Overall: ✅ All previous feedback addressed — this is ready to merge.

What changed since the last review

Regression test (tests/php/Actions_Test.php:138–156)

The new test_initialize_search_registers_module_regardless_of_instant_search_flag test is well-written:

Dual PHPUnit compatibility — both the @testWith doc annotation (PHPUnit 8/9) and #[TestWith(...)] attribute (PHPUnit 10+) are present. That's the correct pattern for this codebase.
Proper isolation — remove_all_filters( 'jetpack_sync_modules' ) before and after ensures no state bleeds between tests.
Correct assertion target — add_search_sync_module() appends the class name string 'Automattic\\Jetpack\\Sync\\Modules\\Search' to the array, so assertContains on that string is exactly right.
Both cases covered — false (the bug case) and true (regression guard on the previously-working path).
Graceful skip — markTestSkipped when the Search package isn't present is the right move for an optional dependency.

Expanded docblock (class-actions.php:867–882)

The updated docblock is precise and durable:

Explicitly names is_instant_search_enabled() and is_active() as future regression traps, with the concrete reason why (gate fires at plugins_loaded before the REST handler runs).
Clarifies that class_exists( Module_Control::class ) is an autoloader concern, not a module-activation check — removes ambiguity that could lead a future developer to add a is_active() guard "for symmetry."

No issues found anywhere in the diff.

…the option Switching the experience picker to 'inline' now calls update_option(..., '') instead of delete_option(). On a site that has never written jetpack_search_experience, delete_option is a no-op (it returns false and does not fire deleted_option), so Sync had no event to queue and the WPcom cache site could stay stuck on a stale 'overlay' / 'embedded' from a previous transition. get_experience() still resolves '' (and a missing option) as INLINE via the existing fall-through to the instant_search_enabled legacy check — no reader change is needed.

…ce writes Adds option-hook handlers (`add_option_jetpack_search_experience`, `update_option_jetpack_search_experience`) that mirror the new value onto the legacy `instant_search_enabled` boolean. update_experience() already maintained this lockstep explicitly, but a direct update_option() that bypasses update_experience() — wp-cli, another plugin, or sync-side replication on a third-party setup — would otherwise drift the two values. The hook is idempotent: when update_experience() has already set instant_search_enabled to the right value, the mirror's update_option call is a no-op (WP returns false without firing the action again).

…ty-string sentinel Drops the '' sentinel and the 'absence-means-inline' indirection in update_experience() — every active branch now writes both instant_search_enabled and jetpack_search_experience to their literal values for the chosen experience. get_experience() recognises 'inline' directly; the legacy is_instant_search_enabled() fallback is kept for sites that have never saved via the new UI. Sync still fires on inline writes because the literal value is just as visible to updated_option as '' was, without the sentinel cost.

…_experience() are sufficient Removes the `add_option_jetpack_search_experience` / `update_option_jetpack_search_experience` action mirror that wrote `instant_search_enabled` from the experience value, plus its initializer wiring, lockstep test, and changelog entry. `update_experience()` already calls disable_instant_search() / enable_instant_search() inline on every active branch, so both options are written to their correct values without a hook-based indirection. The hook only protected against direct `update_option('jetpack_search_experience', ...)` writes that bypass `update_experience()` — a hypothetical caller we don't have, and not a real concern at this layer.

kangzj · 2026-05-12T23:44:56Z

@claude please review this PR.

claude · 2026-05-12T23:45:20Z

Claude finished @kangzj's task in 1m 38s —— View job

PR Review

Gather context
Read changed files
Review code changes
Post review feedback

Overall: ✅ Correct, minimal, and well-tested — ready to merge.

What this PR does

Two independent fixes that together close a sync gap:

packages/sync — initialize_search() gate removed (class-actions.php:885–887): The is_instant_search_enabled() guard is dropped. The Search sync module now registers unconditionally when the Search package is present (autoloader check only). Previously, any request where instant_search_enabled was false at plugins_loaded time never loaded the sync module, so the write that flipped it to true was silently excluded from the sync queue.
packages/search — update_experience('inline') writes instead of deletes (class-module-control.php:229–254): All three non-off experiences now call update_option() with their literal value ('inline', 'embedded', 'overlay'). The old path for inline called delete_option(), which is a no-op when the option doesn't exist, producing no added_option / updated_option hook and therefore nothing for Sync to queue.

Code review

class-actions.php:867–891

The fix is a clean 3-line deletion. The updated docblock accurately explains:

Why is_instant_search_enabled() is wrong (owns the option it gates on)
Why is_active() is equally wrong (same chicken-and-egg at the off → overlay boundary)
Why class_exists() is the right gate (autoloader concern, not activation state)

No issues.

class-module-control.php:195–211 — get_experience()

The refactored reader is correct. in_array($saved, ['inline','embedded','overlay'], true) correctly handles:

Any of the three live values → returned directly
false (option missing) → falls through to legacy check ✓
'' (empty string, previously possible sentinel) → falls through to legacy check ✓

The consolidated legacy fallback (return $this->is_instant_search_enabled() ? OVERLAY : INLINE) is functionally identical to the two-branch if/else it replaces.

class-module-control.php:229–254 — update_experience()

The switch-to-if/else refactor preserves all the original error-return semantics:

off → deactivate, early return (no option write — preserves prior preference on re-enable) ✓
overlay → activate → enable_instant_search (errors short-circuit both) → update_option('overlay') ✓
embedded / inline → activate → disable_instant_search → update_option($experience) ✓

disable_instant_search() return value is unchecked in both old and new code (not a regression).

Actions_Test.php:138–157 — regression test

Well-structured:

Dual PHPUnit annotation (@testWith + #[TestWith(...)]) for PHPUnit 8/9 and 10+ compatibility
Proper isolation: remove_all_filters('jetpack_sync_modules') before and after
Tests both instant_search_enabled = false (the bug case) and true (regression guard on the previously-working path)
markTestSkipped when Search package is absent — correct handling for optional dependency

Module_Control_Test.php:257–302 — updated + new tests

test_update_experience_inline correctly updates the assertion from assertFalse(get_option(...)) (old delete-based storage) to assertSame(EXPERIENCE_INLINE, get_option(...)) and also round-trips through get_experience() — good
test_update_experience_inline_fires_action_when_option_missing directly tests the sync contract: a fresh site switching to inline must fire added_option or updated_option for jetpack_search_experience. The hook counter pattern is the right approach here. Filter cleanup is correct.

Changelogs

Both Significance: patch / Type: fixed. Appropriate.

No issues found

The fix is surgical, the docblocks make the why durable, and the test suite covers both the bug case and the regression guard. No concerns.

Restores the switch in update_experience() and tightens the instant_search_enabled writes to the two transitions that actually need them: 'off' disables it (so the legacy boolean doesn't drift true after the user turns Search off from overlay), and 'overlay' enables it via the existing enable_instant_search() guard. 'inline' and 'embedded' no longer touch the boolean — they only need the experience option. get_experience() picks up an explicit 'inline' branch so a value written by update_experience('inline') reads back correctly without falling through to the legacy is_instant_search_enabled() inference.

kangzj · 2026-05-12T23:51:43Z

Heads-up: pushed two follow-up commits since the prior ping (`14b39e944bbe17565ce180662214d349ca17fc59`). Latest design only writes `instant_search_enabled` on OFF/OVERLAY — please re-review against the new HEAD.

- Search REST `test_update_settings_experience_off` now asserts `instant_search_enabled === false` after `experience=off` (matches the new design where OFF disables the legacy boolean). - Plugin sync `test_sync_default_options` covers the twelve options the Search sync module whitelists; the meta-test fails fast on any whitelisted option that lacks a matching test entry.

jp-launch-control · 2026-05-13T00:08:46Z

Code Coverage Summary

Coverage changed in 2 files.

File	Coverage	Δ%	Δ Uncovered
projects/packages/search/src/class-module-control.php	77/84 (91.67%)	1.13%	0 💚
projects/packages/sync/src/class-actions.php	183/434 (42.17%)	1.11%	-6 💚

Full summary · PHP report · JS report

kangzj · 2026-05-13T00:18:42Z

@claude please re-review at HEAD `cd98390f438725b032c9e348d720252a00c3bddb` — the design has been simplified since your last pass:

update_experience() only writes instant_search_enabled on OFF and OVERLAY
the auto-sync option-mirror has been dropped
the empty-string sentinel for inline is gone (writes the literal value inline)

CI is green.

claude · 2026-05-13T00:19:00Z

Claude finished @kangzj's task in 2m 31s —— View job

PR Re-Review (HEAD `cd98390f`)

Gather context
Read changed files at HEAD
Review updated design
Post review feedback

Overall: ✅ Design is cleaner and the sync fix is solid. Two minor observations below — neither is a blocker.

What changed since the last reviewed pass

Branch	Inline	Embedded	Off
old	activate → disable_instant_search → delete_option	activate → disable_instant_search → update_option('embedded')	Modules::deactivate (no instant_search write)
HEAD	activate → update_option('inline')	activate → update_option('embedded')	disable_instant_search → Modules::deactivate

Three concrete changes:

inline and embedded no longer call disable_instant_search() — the lockstep with the legacy boolean is intentionally dropped for these two experiences.
off now explicitly calls disable_instant_search() before deactivating — previously it bypassed $this->deactivate() and left the boolean alone.
inline writes the literal 'inline' string instead of deleting the option (fixes the Sync no-op on fresh sites).

Code review

class-actions.php — unchanged from the last pass. Still the correct minimal fix.

class-module-control.php — update_experience()

The off branch change is correct: calling disable_instant_search() before Modules::deactivate() prevents the legacy boolean from "drifting true" when a site disables Search and then re-enables it on a non-overlay experience. REST_Controller_Test.php is updated to match (assertFalse($data['instant_search_enabled']) for the off case) and test_update_experience_off_disables_instant_search_and_preserves_experience covers it cleanly.

The inline branch fix (write 'inline' instead of delete) is the right call — update_option() always fires added_option / updated_option, so Sync has something to queue even on a fresh site. test_update_experience_inline_fires_action_when_option_missing locks in the contract.

Jetpack_Sync_Options_Test.php

Good regression coverage: the 12 Search options are now exercised in the integration whitelist test, confirming they land in the queue end-to-end after the gate removal.

Observation 1 — legacy `instant_search_enabled` readers (confirm-before-ship)

The old docblock noted:

Legacy module_active / instant_search_enabled are kept in lockstep so unmigrated readers (Initializer, Options, sidebar registration) continue to see the right state until they're migrated to consult get_experience().

That comment is now gone, and the lockstep is intentionally dropped for inline / embedded. get_experience() short-circuits to the literal stored value before it reaches the legacy fallback, so the reader in this package is correct regardless. However, if any code outside this package (Jetpack core, WPcom) still reads instant_search_enabled directly and uses it to decide whether to load the overlay frontend, a user switching from overlay → embedded / overlay → inline would leave instant_search_enabled = true and those paths would see a stale value.

Worth confirming: are the "Initializer, Options, sidebar registration" readers now migrated to get_experience(), or is leaving instant_search_enabled = true safe because they no longer gate on it? If they're migrated, no issue. If not, this is a latent regression for the overlay → embedded/inline transition.

Observation 2 — stale assertions in `test_update_experience_embedded` / `test_update_experience_inline`

Both tests assert assertFalse( static::$search_module->is_instant_search_enabled() ). In the new design neither branch calls disable_instant_search(), so this assertion passes vacuously — instant_search_enabled was never set to true in the test setup. The assertion doesn't test behavior anymore; a reader will assume the code still disables it. This is pre-existing for embedded (test wasn't touched), introduced by the rename for inline.

Not a blocker — no wrong behavior, just a mildly misleading assertion. Could be converted to a comment or the setup could pre-set instant_search_enabled = true to document the "we intentionally leave it alone" contract, but that's cosmetic.

…legacy boolean Address review feedback (claude obs 1): with the new write semantics that only flip `instant_search_enabled` on OFF / OVERLAY, `Initializer::init_search()` was still using the boolean to decide between the Instant Search and Classic Search frontends. An overlay → embedded / overlay → inline transition would have left the boolean true and the overlay JS would have kept loading. Switch the gate to `get_experience() === 'overlay'`, which now reads the canonical experience option directly. No reader downstream of `is_instant_search_enabled()` is touched here — other callers (debug-bar, ai-chat editor state, etc.) still consult the legacy boolean and remain candidates for a follow-up migration. Also strengthen the embedded/inline tests (claude obs 2) by pre-setting `instant_search_enabled = true` so the assertion proves those branches intentionally leave the boolean alone, instead of passing vacuously on the default false.

kangzj · 2026-05-13T00:34:44Z

Addressing claude[bot]'s re-review (HEAD cd98390f) — both observations addressed in d54ff2e14662eb9cc8efe4c980e3f309103e712f:

Obs 1 (Initializer drift): real regression — Initializer::init_search() was still using is_instant_search_enabled() to choose between the Instant Search and Classic Search frontends, so an overlay → embedded/inline transition would have left the overlay JS loaded. Migrated the gate to get_experience() === 'overlay' so the experience option is the source of truth at the load layer too. Other readers of the legacy boolean (debug-bar, ai-chat editor state, dashboard initial state, search_auto_config REST gate) still exist and remain candidates for a follow-up migration; none of them are on the front-end load path.

Obs 2 (vacuous test assertions): strengthened both test_update_experience_embedded and test_update_experience_inline by pre-setting instant_search_enabled = true (simulating a prior overlay state), then asserting the boolean is still true after the call. The assertions now actively prove the design contract — those branches deliberately leave the legacy boolean alone — instead of passing vacuously on the default false.

kangzj · 2026-05-13T00:45:27Z

@claude please re-review at HEAD `d54ff2e14662eb9cc8efe4c980e3f309103e712f` — addressed both observations from the prior pass:

Initializer migrated to get_experience() (closes the overlay→inline/embedded frontend regression).
Embedded/inline tests strengthened: pre-set instant_search_enabled = true so they prove the contract.

claude · 2026-05-13T00:45:49Z

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

PR Re-Review (HEAD `d54ff2e`)

Gather context
Read changed files at HEAD
Review Initializer migration to get_experience()
Review strengthened test assertions
Post review feedback

Overall: ✅ Both observations fully addressed — ready to merge.

Observation 1 — Initializer migration (`class-initializer.php:142–154`)

$is_overlay = Module_Control::EXPERIENCE_OVERLAY === ( new Module_Control() )->get_experience();
if ( $is_overlay ) {
    $success = static::init_instant_search( $blog_id );
}
if ( apply_filters( 'jetpack_search_classic_search_enabled', ! $is_overlay ) ) {
    $success = static::init_classic_search( $blog_id );
}

Correct. $is_overlay is now derived from get_experience(), not is_instant_search_enabled(). Both the init_instant_search branch and the classic_search_enabled filter default are consistently keyed off the same expression. A site transitioning overlay → embedded or overlay → inline will now load the classic/inline frontend immediately rather than leaving the Instant Search overlay JS active.

Observation 2 — Strengthened test assertions

test_update_experience_embedded (lines 248–264)

Pre-setting instant_search_enabled = true and then asserting assertTrue( is_instant_search_enabled() ) after the call actively proves the contract — embedded deliberately leaves the legacy boolean alone — rather than passing vacuously on the default false.

test_update_experience_inline (lines 271–287)

Same treatment: pre-seeds 'embedded' in the option (proves the switch overwrites, not appends), pre-sets the legacy boolean to true, then verifies it is still true after the call. The double assertion at lines 284–285 (get_option + round-trip through get_experience()) correctly confirms the stored literal 'inline' is returned by the reader even when instant_search_enabled = true — the early-return in get_experience() correctly short-circuits before hitting the legacy fallback.

No new issues

The full picture at HEAD is clean. The sync gate removal, the delete → update_option('inline') fix, the Initializer migration, the off branch acquiring disable_instant_search(), and the full regression test suite all cohere. No concerns.

…tion first The legacy boolean is no longer the canonical source of truth — the experience option is. The rewrite reads `jetpack_search_experience` first: `'overlay'` ⇒ on, `'inline'` / `'embedded'` ⇒ off (an explicit opt-out wins over the boolean), and falls back to the legacy `instant_search_enabled` option only when the experience option has never been written (pre-existing sites). Also gates on `is_active()` so the preserve-on-OFF behavior doesn't make a stale `'overlay'` read as on. Restores `disable_instant_search()` in `update_experience()` for `inline` / `embedded` so writes stay in lockstep too — belt-and- suspenders for any caller that still reads the boolean directly. Adds data-provided coverage for the new precedence rules.

kangzj · 2026-05-13T01:04:40Z

@claude please re-review at HEAD 411bcdc375b9340c03729447a92903e47aa7f2e2. Design refined per owner direction:

Read side: Module_Control::is_instant_search_enabled() rewritten to read jetpack_search_experience first — 'overlay' ⇒ on; 'inline'/'embedded' ⇒ explicitly off; legacy instant_search_enabled boolean is the fallback only when the experience option has never been written. Also gates on is_active() so the preserve-on-OFF behavior doesn't make a stale 'overlay' read as on.

Write side: restored disable_instant_search() on inline / embedded branches of update_experience() — write-side lockstep is back as belt-and-suspenders.

Net effect: every reader of the boolean (debug-bar, AI Chat editor state, dashboard, REST settings response) is now correct without per-call-site migration; the Initializer still uses the explicit get_experience() === 'overlay' form for clarity at the call site.

Coverage: data-provided test for the experience-precedence rule, separate test for the module-inactive case, and the embedded/inline tests now actively prove disable runs (pre-set boolean to true, then assert false).

claude · 2026-05-13T01:05:06Z

Claude finished @kangzj's task in 2m 14s —— View job

PR Re-Review (HEAD `411bcdc`)

Gather context
Read changed files at HEAD
Review is_instant_search_enabled() rewrite (read side)
Review restored disable_instant_search() on inline/embedded (write side)
Review new/updated tests
Post review feedback

Overall: ✅ Clean, complete, and correct — ready to merge.

What changed since the prior pass

The single commit 411bcdc3 rewrites is_instant_search_enabled() to read jetpack_search_experience first, restores disable_instant_search() to the inline/embedded branches, and adds the test coverage to match.

Read side — `is_instant_search_enabled()` (`class-module-control.php:105–120`)

The new implementation is correct in both ordering and semantics:

is_active() gate first: returns false immediately when the module is off. This prevents a stale 'overlay' preserved for re-enable from reading as "Instant Search is on" while the module isn't loaded — exactly the concern raised in the previous review.
Experience option takes precedence:
- 'overlay' → true
- 'inline' / 'embedded' → false (explicit opt-out even if the legacy boolean drifted true)
- missing (false) → falls through to the legacy boolean
Legacy fallback: reads instant_search_enabled directly — clean and unambiguous.

No circular dependency: get_experience() calls is_instant_search_enabled() only in its own legacy-fallback path (option missing). In that path, is_instant_search_enabled() also finds the option missing and reads the legacy boolean directly. The two methods never reach each other's "option present" branches. ✓

Write side — `update_experience()` (`class-module-control.php:263–307`)

disable_instant_search() is back for inline and embedded. The full picture:

Experience	disable_instant_search	enable_instant_search	update_option
`off`	✓	—	(preserved)
`inline`	✓	—	`'inline'`
`embedded`	✓	—	`'embedded'`
`overlay`	—	✓	`'overlay'`

Belt-and-suspenders is the right call: is_instant_search_enabled() would resolve correctly from the experience option alone, but the lockstep keeps every legacy reader correct without individual migration.

Tests

test_is_instant_search_enabled_experience_precedes_legacy_boolean (lines 176–189)

The key design decision in the test — setting the legacy boolean to !$expected — proves the experience option is what's consulted, not the boolean. All three live values are covered with dual PHPUnit annotation. ✓

test_is_instant_search_enabled_false_when_module_inactive (lines 149–158)

Pre-seeds 'overlay' in the experience option and the legacy boolean to true, with no active-modules filter. assertFalse proves the is_active() gate fires before either option is read. ✓

test_update_experience_embedded/inline (lines 304–343)

Both tests now pre-set instant_search_enabled = true and assert false after the call — proves disable_instant_search() actually ran rather than asserting on the default false. ✓

test_update_experience_overlay comment (lines 289–290)

The added note explaining why return_search_active_array must stay live across the is_instant_search_enabled() assertion is accurate and will prevent a future maintainer from "simplifying" the test into a false green. ✓

No issues found

The design is now self-contained: is_instant_search_enabled() is correct for all callers without requiring per-call-site migration, the write side is belt-and-suspenders, and every key contract has a test. The sync gate fix (initialize_search() unconditional registration) from earlier passes is unchanged and still correct.

…write covers it The prior migration of `init_search()` to `get_experience() === 'overlay'` was a per-call-site fix for the overlay→inline/embedded drift. The subsequent rewrite of `is_instant_search_enabled()` to read the experience option (with a legacy boolean fallback) makes the boolean itself correct, so the explicit migration is now redundant. Restore the original `is_instant_search_enabled()` call and drop its changelog entry.

kangzj · 2026-05-13T01:27:22Z

Final tweak: reverted the Initializer migration at HEAD `55fd66cbe519823089dc01d77f4d472cf721ee26` since the is_instant_search_enabled() rewrite makes the per-call-site change redundant. Net effect on the Initializer is no diff vs trunk now. Tests still green (439/439).

…e_instant_search() Per owner review: the empty-string sentinel and the literal 'inline' write were over-engineering — Sync hooks `deleted_option` so a true delete on an existing option still replicates, and a delete on a missing option correctly no-ops (nothing to sync because nothing was different). Restore the trunk switch verbatim — inline goes back to delete_option(), no docstring rewrite, no scope changes — except for adding $this->disable_instant_search() to the OFF case so the legacy boolean doesn't drift true after a non-overlay re-enable. Drops the search-186-experience-write-inline-value changelog and the test_update_experience_inline_fires_action_when_option_missing test; both described behavior that no longer happens. The remaining inline test renames back to test_update_experience_inline_deletes_option and asserts the option is deleted.

…hange Per owner: switching to inline now writes `update_option('')` instead of `delete_option()` so the action always fires — `deleted_option` hooks won't catch a delete-on-missing on a fresh site, but `added_option` / `updated_option` always do. Drops the now-dead `if ('inline' === $saved) return INLINE` branch in `get_experience()` (we never write the literal 'inline' anymore — '' falls through to the legacy fallback which already returns INLINE when the boolean is false). Test updates: rename the inline test back to writes_empty_string, restore the fires-action-when-option-missing regression test (the empty-string write is what makes that case fire), and switch the option assertion from assertFalse to assertSame('', ...).

kangzj requested a review from a team as a code owner May 12, 2026 23:06

kangzj added [Status] Needs Review This PR is ready for review. [Package] Sync labels May 12, 2026

kangzj added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels May 12, 2026

kangzj added 2 commits May 13, 2026 11:11

Sync: Expand initialize_search() docblock with gating + class_exists …

b15e745

…notes

github-actions Bot added the [Tests] Includes Tests label May 12, 2026

kangzj changed the title ~~Sync: Always register Search sync module when the Search package is present~~ Search: Reliably sync experience picker changes when Instant Search is off May 12, 2026

github-actions Bot added the [Package] Search Contains core Search functionality for Jetpack and Search plugins label May 12, 2026

kangzj self-assigned this May 12, 2026

kangzj requested a review from adamwoodnz May 12, 2026 23:39

kangzj added 2 commits May 13, 2026 11:42

github-actions Bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label May 13, 2026

kangzj added 2 commits May 13, 2026 13:39

kangzj merged commit a8b0c81 into trunk May 13, 2026
87 of 91 checks passed

kangzj deleted the echo/search-186-sync-gate-removal branch May 13, 2026 02:12

github-actions Bot added [Status] UI Changes Add this to PRs that change the UI so documentation can be updated. and removed [Status] In Progress labels May 13, 2026

github-actions Bot added this to the jetpack/15.9 milestone May 13, 2026

Conversation

kangzj commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Proposed changes

Related product discussion/links

Does this pull request change what data or activity we track or use?

Verification

Testing instructions

Uh oh!

github-actions Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kangzj commented May 12, 2026

Uh oh!

github-actions Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

Summary

Code review

One suggestion — add a regression test

Uh oh!

kangzj commented May 12, 2026

Uh oh!

kangzj commented May 12, 2026

Uh oh!

claude Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Re-Review

What changed since the last review

Uh oh!

kangzj commented May 12, 2026

Uh oh!

claude Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

What this PR does

Code review

No issues found

Uh oh!

kangzj commented May 12, 2026

Uh oh!

jp-launch-control Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Uh oh!

kangzj commented May 13, 2026

Uh oh!

claude Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Re-Review (HEAD cd98390f)

What changed since the last reviewed pass

Code review

Observation 1 — legacy instant_search_enabled readers (confirm-before-ship)

Observation 2 — stale assertions in test_update_experience_embedded / test_update_experience_inline

Uh oh!

kangzj commented May 13, 2026

Uh oh!

kangzj commented May 13, 2026

Uh oh!

claude Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Re-Review (HEAD d54ff2e)

Observation 1 — Initializer migration (class-initializer.php:142–154)

Observation 2 — Strengthened test assertions

No new issues

Uh oh!

kangzj commented May 13, 2026

Uh oh!

claude Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Re-Review (HEAD 411bcdc)

What changed since the prior pass

Read side — is_instant_search_enabled() (class-module-control.php:105–120)

Write side — update_experience() (class-module-control.php:263–307)

Tests

No issues found

kangzj commented May 12, 2026 •

edited

Loading

github-actions Bot commented May 12, 2026 •

edited

Loading

github-actions Bot commented May 12, 2026 •

edited

Loading

claude Bot commented May 12, 2026 •

edited

Loading

claude Bot commented May 12, 2026 •

edited

Loading

claude Bot commented May 12, 2026 •

edited

Loading

jp-launch-control Bot commented May 13, 2026 •

edited

Loading

claude Bot commented May 13, 2026 •

edited

Loading

PR Re-Review (HEAD `cd98390f`)

Observation 1 — legacy `instant_search_enabled` readers (confirm-before-ship)

Observation 2 — stale assertions in `test_update_experience_embedded` / `test_update_experience_inline`

claude Bot commented May 13, 2026 •

edited

Loading

PR Re-Review (HEAD `d54ff2e`)

Observation 1 — Initializer migration (`class-initializer.php:142–154`)

claude Bot commented May 13, 2026 •

edited

Loading

PR Re-Review (HEAD `411bcdc`)

Read side — `is_instant_search_enabled()` (`class-module-control.php:105–120`)

Write side — `update_experience()` (`class-module-control.php:263–307`)