dev: backport refactor changes from One* by justlevine · Pull Request #74 · rtCamp/OneSearch

justlevine · 2025-12-04T21:13:55Z

Description

This provides the first round of OneDesign backports to this plugin.

Technical Details

Does not make any attempts to refactor legacy code or fix existing bugs. Specifically:

inc/classes/*
The internals of inc/Modules/Rest/*
inc/Utils

have been left untouched.

Checklist

Screenshots

To-do

Fixes/Covers issue

Fixes #

Copilot

Pull request overview

Copilot reviewed 58 out of 62 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

inc/Modules/Search/Settings.php

up1512001

Added few comments & hoping some of them are from legacy code.

up1512001 · 2025-12-08T12:04:37Z

assets/src/admin/settings/index.js


 	useEffect( () => {
-		const token = NONCE;
+		const token = ( NONCE );


why need to make const out of NONCE, i think its from old code base.

Yup legacy from OneDesign to keep the diff small. Since I want to rip this file out and replace it with tsx/apiFetch anyway, I'm leaving for now.

https://github.com/rtCamp/OneDesign/blob/77ce743610280a49021a0f3fcd858eed3e2e196e/assets/src/admin/settings/index.js#L33

assets/src/admin/settings/index.js

assets/src/components/SiteModal.js

up1512001 · 2025-12-08T12:10:41Z

inc/classes/algolia/class-algolia-index-by-post.php

-					'X-OneSearch-Plugins-Token' => $public_key,
+					'Accept'            => 'application/json',
+					'Content-Type'      => 'application/json',
+					'X-OneSearch-Token' => $public_key,


just as remainder here public_key should be api_key

You're referring to the name of PHP variable used inside the class, or something else?

(either way, the plan is to refactor all this)

up1512001 · 2025-12-08T12:11:39Z

inc/classes/algolia/class-algolia-index.php


-		$base_size       = strlen( wp_json_encode( $base_record ) ?: '' );
-		$available_space = 8000 - $base_size; // Per-chunk allowed size (left size).
+		$base_size       = strlen( wp_json_encode( $base_record, JSON_INVALID_UTF8_SUBSTITUTE ) ?: '' );


better to add check since wp_json_encode could return false as well.

Good eyes. Will make sure to include this in the followup PR.

up1512001 · 2025-12-08T12:17:58Z

inc/Modules/Rest/Basic_Options_Controller.php

+				[
+					'methods'             => WP_REST_Server::CREATABLE,
+					'callback'            => [ $this, 'set_shared_sites' ],
+					'permission_callback' => static fn () => current_user_can( 'manage_options' ),


can we create method into abstract class which needs to check for manage_options?

Why would we want to abstract a one-line function call with a single parameter?
It's just extra boilerplate we need to context-switch, review and maintain.

I could see the argument to abstract the capability itself into a constant or filterable static function, on a long-term release roadmap. But at the same time the reason we're using manage_options is because they're wp_option, so really we should be getting rid of these endpoints entirely and replacing them with wp/v2/settings

up1512001 · 2025-12-08T12:18:36Z

inc/Modules/Rest/Basic_Options_Controller.php

+					}
+
+					// Check X-OneSearch-Token header.
+					$token = sanitize_text_field( wp_unslash( $_SERVER['HTTP_X_ONESEARCH_TOKEN'] ) );


remainder as assuming cleanup is pending.

What specifically here needs cleanup?

usage of global vars instead of request.

up1512001 · 2025-12-08T12:20:29Z

inc/Modules/Rest/Basic_Options_Controller.php

+	 */
+	public function health_check(): WP_REST_Response|\WP_Error {
+		$requesting_origin = '';
+		if ( isset( $_SERVER['HTTP_X_ONESEARCH_REQUESTING_ORIGIN'] ) && ! empty( $_SERVER['HTTP_X_ONESEARCH_REQUESTING_ORIGIN'] ) ) {


remainder to cleanup using check_api_permissions permission callback.

Yup, rn the plugin has separate endpoints for local/cross-site. This will be handled in a followup PR when I dedupe.

up1512001 · 2025-12-08T12:23:46Z

inc/Modules/Search/Settings.php

+			$index->delete()->wait();
+		} catch ( \Throwable $e ) {
+			// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log -- We need visibility.
+			error_log( 'Algolia Exception: ' . $e->getMessage() );


log in dev mode only.

Imo, we need visibility that extends beyond dev-mode here.

Just for our own QA, we need these for our onepress-vip sites, and if we're using this on rtcamp-com then we definitely need this.

Possibly but unlikely that my Algolia refactor will get rid of these horrible try/catches that are hiding all bugs, and a logger class (shared or otherwise) is beyond the scope.

up1512001 · 2025-12-08T12:27:23Z

inc/Modules/Settings/Settings.php

 									'format' => 'uri',
 								],
-								'publicKey' => [
+								'logo_id' => [


if logo is not required then can we remove it?

Your call.

I want to keep the diffs as close to each other, so it feels like keeping it in code but hiding it in js-land is the safest approach (especially since most the codebases arent defining or enforcing array shapes).

up1512001

LGTM, adding one suggestion which I think we can address in next round.

up1512001 · 2025-12-08T16:58:44Z

inc/Modules/Rest/Governing_Data.php

-					'X-OneSearch-Plugins-Token' => $our_public_key,
+					'Accept'            => 'application/json',
+					'Content-Type'      => 'application/json',
+					'X-OneSearch-Token' => $our_public_key,


Just a suggestion can we stick to one terminology either api_key or public_key.

justlevine · 2025-12-08T17:15:56Z

Merging. Leaving the comments open so I can track against the next PR.

dev: refactor [first pass]

34b3b7e

justlevine requested a review from Copilot December 4, 2025 21:13

Copilot started reviewing on behalf of justlevine December 4, 2025 21:14 View session

Copilot finished reviewing on behalf of justlevine December 4, 2025 21:14

chore: fix composer scripts

eae9426

This comment was marked as outdated.

Sign in to view

justlevine requested a review from up1512001 December 4, 2025 21:32

dev: backport shared changes [round 2]

dad329e

justlevine requested a review from Copilot December 7, 2025 23:54

Copilot started reviewing on behalf of justlevine December 7, 2025 23:55 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

inc/Modules/Search/Settings.php Show resolved Hide resolved

justlevine changed the title ~~dev: refactor [first pass]~~ dev: backport refactor changes from One* Dec 8, 2025

up1512001 reviewed Dec 8, 2025

View reviewed changes

justlevine added 3 commits December 8, 2025 18:10

fix: cleanup health-check and X-OneSearch-Requesting-Origin

9ac6df8

fix: encrypt algolia credentials

d293481

chore: variable rename

bd022b5

justlevine requested a review from up1512001 December 8, 2025 16:31

up1512001 approved these changes Dec 8, 2025

View reviewed changes

justlevine merged commit 015b400 into develop Dec 8, 2025
6 of 8 checks passed

justlevine deleted the dev/refactor-first-pass branch December 8, 2025 17:16

justlevine mentioned this pull request Dec 8, 2025

fix: backport misc regressions #80

Merged

Conversation

justlevine commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Technical Details

Checklist

Screenshots

To-do

Fixes/Covers issue

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

up1512001 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justlevine Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

up1512001 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justlevine commented Dec 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

justlevine commented Dec 4, 2025 •

edited

Loading

justlevine Dec 8, 2025 •

edited

Loading