fix(admin): decouple AJAX calls and remove unnecessary return values

[faisalahammad]

What?

Closes #131

Decouple the AJAX call chain in the JS admin so each step takes its own arguments instead of relying on raw response data from the previous step. Remove the now-unnecessary return values from the PHP endpoint.

Why?

The previous JS chain passed the full response data object from one step to the next, forcing PHP endpoints to echo back values they did not own. For example, set_up_environment returned plugin and checks only because the next step needed them. This made the flow harder to understand and maintain.

How?

• Added a fetchAJAX(formData) wrapper to reduce repeated fetch boilerplate across 5 call sites.
• Added a getSelectedValues(list) helper to read checked checkboxes from a NodeList.
• Added a module-scoped currentChecks variable so check slugs are stored between getChecksToRun and runChecks.
• Refactored getChecksToRun, setUpEnvironment, runChecks, and runCheck to accept individual parameters instead of a data object.
• Removed plugin and checks from the set_up_environment response. configure_runner() now returns void.
• Updated exportResults to use the shared fetchAJAX wrapper.

Testing Instructions

1. Open Tools > Plugin Check, pick a plugin, click Check it!.
2. Verify results appear as expected. No console errors.
3. Open DevTools > Network, confirm all four AJAX calls return 200 and success: true.
4. Check the plugin_check_set_up_environment response - it should contain only data.message (no data.plugin or data.checks).
5. Click export buttons (CSV, JSON, CTRF, Markdown) - all should download files with the correct filenames.
6. Test failure paths: no plugin selected, invalid nonce via console, subscriber role.

AI Usage Disclosure

• This PR was created without the help of AI tools
• This PR includes AI-assisted code or content

If AI tools were used, please describe how they were used:
Used AI-assisted development tools for code suggestions and refactoring patterns.

Screenshots or screencast

No UI changes. The plugin behaves identically before and after.

Behavior fix

handleDataErrors now handles both error payload shapes returned by the server: WP_Error responses serialize to an array (data.data[0].message), while plain-object payloads (e.g. { message } from export_results()) pass through as-is. The export failure path no longer throws a TypeError and surfaces the real server message instead.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: faisalahammad <faisalahammad@git.wordpress.org>
Co-authored-by: davidperezgar <davidperez@git.wordpress.org>
Co-authored-by: ernilambar <nilambar@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[Copilot]

Pull request overview

This PR refactors the Plugin Check admin “Check it!” flow to decouple the chained AJAX calls by passing explicit parameters between steps (instead of passing raw response payloads), and simplifies the PHP AJAX response surface accordingly.

Changes:

• Refactored plugin-check-admin.js to pass explicit plugin, checks, types, and flags between getChecksToRun, setUpEnvironment, runChecks, and runCheck.
• Added shared JS helpers (fetchAJAX, getSelectedValues) and updated exports to use the shared AJAX wrapper.
• Simplified the PHP runtime setup endpoint by removing unnecessary returned plugin/checks values and making configure_runner() effectively void.

Reviewed changes

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

File	Description
includes/Admin/Admin_AJAX.php	Removes unused runner config return values and trims the runtime setup success payload to message only.
assets/js/plugin-check-admin.js	Introduces fetchAJAX/getSelectedValues helpers and refactors the admin AJAX flow to use explicit arguments (including export requests).

---

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

[faisalahammad]

Review fixes applied (commit b0d4609)

Addressed the Copilot review finding:

• handleDataErrors() now reads the error message from both payload shapes. WP_Error responses serialize to an array (data.data[0].message), while plain-object payloads pass through as-is (data.data.message), matching the export_results() error response. Export failure no longer throws a TypeError and shows the server message instead.

Quality gates passed: wp-scripts lint-js, no CodeRabbit findings, manual testing confirmed by the author.

[davidperezgar]

@faisalahammad Small documentation note: since getSelectedValues() and fetchAJAX() are newly introduced in this PR, could their @since tags be set to 2.1.0 instead of 1.0.0?

[faisalahammad]

Update pushed (commit 2a2c41d): getSelectedValues() and fetchAJAX() @SInCE tags corrected from 1.0.0 to 2.1.0. lint-js passes. Doc-only change, no runtime impact. All review threads now closed.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

assets/js/plugin-check-admin.js:75

• The fetchAJAX() docblock says it wraps fetch with "credentials, nonce in body", but fetchAJAX does not append the nonce itself (callers do). This is misleading—either update the doc text to avoid claiming nonce handling, or implement automatic nonce injection in fetchAJAX.

	 * Wraps fetch with the standard options (ajaxurl, POST, same-origin
	 * credentials, nonce in body) and parses the JSON response, surfacing
	 * server errors via handleDataErrors.

[ernilambar]

Review

Model used: Opus 4.8

A few things should be addressed before merge.

1. getChecksToRun AJAX endpoint still returns plugin, not just check slugs

The issue is explicit: "getChecksToRun( plugin ): its ajax endpoint should return just the check slugs, and then they should be put into a variable."

Admin_AJAX::get_checks_to_run() is untouched by this diff and still does:

$plugin_basename = $runner->get_plugin_basename();
$checks_to_run   = $runner->get_checks_to_run();
...
wp_send_json_success(
    array(
        'plugin' => $plugin_basename,
        'checks' => array_keys( $checks_to_run ),
    )
);

The JS side already stopped relying on data.plugin (it uses the plugin closure variable instead), so $plugin_basename is now dead weight in both the response and the PHP method — it can be dropped from the endpoint, and the now-unused $plugin_basename assignment removed. set_up_environment() got this treatment correctly (plugin/checks removed from its response); get_checks_to_run() didn't.

2. fetchAJAX docblock overpromises — nonce is not actually auto-included

/**
 * Wraps fetch with the standard options (ajaxurl, POST, same-origin
 * credentials, nonce in body) and parses the JSON response, ...
 *
 * @param {FormData} formData Form data to send. Must include 'action' and 'nonce'.
 */
function fetchAJAX( formData ) {
    return fetch( ajaxurl, {
        method: 'POST',
        credentials: 'same-origin',
        body: formData,
    } )
    ...

The comment says nonce handling is part of what the wrapper does, but the implementation just forwards whatever FormData it's given — the @param line even says the caller must include it. All five call sites (getChecksToRun, setUpEnvironment, cleanUpEnvironment, runCheck, fetchExportPayload) still do pluginCheckData.append( 'nonce', pluginCheck.nonce ) manually.

This was explicitly called out in the issue as a nice-to-have ("it could also automatically include the nonce in the body data since that needs to be present in every request"). Since it wasn't done, at minimum fix the docblock so it doesn't claim behavior that isn't there. If it's easy to do (append nonce inside fetchAJAX itself and drop it from all five call sites), that would fully deliver on the issue and remove five more duplicated lines — seems worth doing here rather than a follow-up.

3. Minor: dropped the cleanup success log

Old chain:

getChecksToRun().then(setUpEnvironment).then(runChecks).then(cleanUpEnvironment)
    .then( ( data ) => {
        console.log( data.message );
        resetForm();
    } )

New chain:

.then( () => cleanUpEnvironment() )
.then( () => {
    resetForm();
} )

console.log( data.message ) (the cleanup confirmation message) is silently gone. Probably fine, but flagging in case it wasn't intentional — it's not something the issue asked to remove.

4. Nit: currentChecks doesn't need module-level scope

let checksCompleted = false;
let currentChecks = [];

currentChecks is only read/written inside the checkItButton click handler's promise chain — nothing else in the file touches it. It can be a local variable inside that handler instead of living at the IIFE's top level next to actual shared state like aggregatedResults/checksCompleted. Not a bug (the button is disabled for the duration of a run, so there's no re-entrancy), just unnecessary shared state.

5. Nit: data.message checks lost their !data || guard

Before:

if ( ! responseData.data || ! responseData.data.message ) { throw ... }

After:

if ( ! data.message ) { throw ... }

repeated in setUpEnvironment, cleanUpEnvironment, getChecksToRun (as !data.checks), and runCheck. If data (i.e. responseData.data) is ever null/undefined on a "success" response, this throws a raw TypeError instead of the intended Error( 'Response contains no data' ). Not reachable today since every relevant wp_send_json_success() call passes a real array, but it's slightly less defensive than what was there before.

Good catch: handleDataErrors array-vs-object fix

const firstError = Array.isArray( data.data ) ? data.data[ 0 ] : data.data;
if ( ! firstError || ! firstError.message ) { throw ... }
throw new Error( firstError.message );

This is a genuine pre-existing bug fix, not just refactor churn: Admin_AJAX::export_results() calls wp_send_json_error( array( 'message' => $exception->getMessage() ), 400 ) — a plain object, not a WP_Error — so data.data there is { message }, not an array. The old handleDataErrors (data.data[0].message) would have thrown TypeError: Cannot read properties of undefined on that path instead of surfacing the real message. Routing fetchExportPayload through fetchAJAX/handleDataErrors now handles both shapes correctly. Worth calling out in the PR description since it's an actual behavior fix riding along with the refactor.

Summary

• Must fix: Point 1 (getChecksToRun response still leaks plugin, contradicts the issue's explicit ask).
• Should fix or at least explain: Point 2 (docblock claims nonce auto-injection that isn't implemented — either implement it or correct the comment).

[faisalahammad]

Thanks for the review. Pushed at b5f088e.

1. Dropped plugin from get_checks_to_run response and removed the unused $plugin_basename.
2. fetchAJAX now auto-injects pluginCheck.nonce into a clone of the caller's FormData; all 5 call sites stopped appending nonce. Docblock updated.
3. Cleanup success log restored after cleanUpEnvironment.
4. currentChecks moved to a local let inside the click handler.
5. Null guards restored on all 4 response data sites.

PR body has a one-line note on the array-vs-object error payload fix (b0d4609).

Re-requesting your review.