fix: throttle and bound the settings API connection check#577
Draft
galbus wants to merge 1 commit into
Draft
Conversation
Utils::validate_api_connection() ran on every settings-page load of
the (default) Authentication tab. It deleted the
beyondwords_valid_api_connection flag *before* an uncached,
30-second-timeout API call and never short-circuited, so:
- a slow API blocked the admin page render for up to 30s on every
view — a VIP performance violation (uncached blocking remote
request on a render hot path);
- any transient failure (timeout, DNS error, 5xx, WP_Error) left the
flag deleted, collapsing Tabs::get_visible_tabs() to the
Authentication tab only and locking operators out of the
Integration and Preferences tabs during any API blip.
Fix:
- Short-circuit with a throttle transient
(beyondwords_api_connection_checked, 5-minute TTL) keyed to a
fingerprint of the current credentials. Repeat loads skip the
network call; changing the API key or project ID busts the
fingerprint so a credentials save re-validates immediately.
- Stop deleting the flag pre-emptively. It is now cleared only on a
definitive auth failure: 401 (already handled in
Client::call_api) or 403 (mirrored in validate_api_connection).
Transient failures preserve the last known-good flag.
- Use a bounded 3-second timeout for the validation GET.
call_api()/build_args() gained an optional $timeout (default 30);
because the timeout is now a variable rather than a literal > 3,
the VIP RemoteRequestTimeout sniff no longer fires, so the
previous phpcs:ignore was removed (no new ignores added).
- Remove the dead
delete_transient('beyondwords_validate_api_connection') call —
that transient was never set anywhere in the codebase.
Adds PHPUnit coverage: flag preserved on 500 and on WP_Error, flag
cleared on 403 and when credentials are removed, throttle skips the
repeat call, and a credentials change forces re-validation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ WordPress Plugin Check Report
📊 ReportAll checks passed! No errors or warnings found. 🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The BeyondWords settings page revalidated the REST API connection on every page load and could hide unrelated settings tabs during any API blip. This fixes both the performance and availability problems.
Settings::maybe_validate_api_creds()fires on theload-settings_page_beyondwordshook whenever the active tab is Authentication — which is the default tab used by both the admin-menu link and the plugin-row Settings link (neither passes?tab=). It calledUtils::validate_api_connection(), which:delete_option()'d thebeyondwords_valid_api_connectionflag before issuing an uncachedGET /projects/{id}with a 30-second timeout, and never short-circuited — despite the docblock claiming it did; andWP_Error) left it unset, soTabs::get_visible_tabs()collapsed the UI to the Authentication tab only.Impact
What changed
src/settings/class-utils.php—validate_api_connection()beyondwords_api_connection_checkedtransient (5-min TTL) keyed to an MD5 fingerprint of the current credentials. Repeat page loads skip the network call entirely; changing the API key or project ID busts the fingerprint, so saving new credentials revalidates immediately instead of waiting out the window.delete_option()is gone. The flag is now cleared only on a definitive auth failure — 401 (already handled inClient::call_api()) or 403 (mirrored here). Timeouts / 5xx /WP_Errorleave the last known-good flag intact, so an API blip can no longer hide the other tabs.delete_transient('beyondwords_validate_api_connection')line — that transient was never set anywhere (grep-confirmed).src/api/class-client.php—call_api()/build_args()$timeoutargument (defaultDEFAULT_TIMEOUT = 30, so all existing callers are unchanged). The validation GET passes3.> 3, the VIPWordPressVIPMinimum.Performance.RemoteRequestTimeoutsniff no longer fires — so the previous// phpcs:ignoreon that line was removed. No new ignores were added (per the VIP non-negotiable inAGENTS.md).call_api()'s 401-clears-the-flag behaviour is unchanged; 403 handling is scoped to the validation path only, so audio-generation calls are unaffected.Tests (
tests/phpunit/settings/)setUp/tearDownintest-utils.phpandtest-settings.php.validate_api_connectioncases: flag preserved on 500, flag preserved onWP_Error, flag cleared on 403, flag cleared when credentials removed, throttle skips the repeat call, and re-validate on credentials change.Behaviour matrix
WP_ErrorTesting / reviewer notes
vendor/bin/phpcsrun against the repo's.phpcs.xmland via the repo's grumphp pre-commit hook (phpcstask passed), including verifying that removing thephpcs:ignoredoes not reintroduce a timeout violation.php -l: clean on all four files.pre_http_request-stub patterns exactly. I was unable to execute them locally because the worktree'swp-envtest container could not start — the shared Docker daemon was saturated by ~20 parallelwp-envstacks (all base images were cached; the daemon itself was unresponsive). CI will run the full PHPUnit suite — please confirm it's green before merging.Draft until CI validates the PHPUnit run.
🤖 Generated with Claude Code