Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@wp-slimstat.php`:
- Around line 359-372: The resolve_geolocation_provider() function currently
treats any set value for self::$settings['geolocation_provider'] (including
empty or malformed strings) as authoritative; update it to sanitize and
allowlist the new setting before returning it and only use the legacy
enable_maxmind flag if the new setting is missing or invalid. Specifically, in
resolve_geolocation_provider() call sanitize_text_field() on
self::$settings['geolocation_provider'], check it against an explicit allowed
set (e.g., 'disable', 'maxmind', 'dbip') and return false for 'disable' or the
allowed provider name; if the sanitized value is not in the allowlist, fall back
to reading self::$settings['enable_maxmind'] (handle 'on' -> 'maxmind', 'no' ->
'dbip') and return false otherwise.

---

Nitpick comments:
In `@src/Services/GeoService.php`:
- Around line 76-79: The isJsDelivrEnabled() method still checks the legacy
enableMaxmind value; update it to use the new provider resolver like the other
methods (e.g., mirror the pattern used in isGeoIPEnabled() and
isMaxMindEnabled()) so it reads the geolocation/provider state from the resolver
instead of comparing $this->enableMaxmind to 'no' (or alternatively check
geolocation_provider !== 'disable' via the resolver); adjust isJsDelivrEnabled()
to call the resolver method that determines JsDelivr/mirror availability.
- Around line 17-21: The constructor sets $this->enableMaxmind but several
methods (isGeoIPEnabled, isMaxMindEnabled) now call the resolver directly,
leaving the property and its accessors partially orphaned; to clean this up,
remove the unused property and its setter/getter (setEnableMaxmind,
getEnableMaxmind) and update any methods that currently read the property
(isJsDelivrEnabled, getEnableMaxmind usages) to call the same resolver used by
isGeoIPEnabled()/isMaxMindEnabled() so all checks consistently use the resolver;
locate symbols __construct, $this->enableMaxmind, isGeoIPEnabled(),
isMaxMindEnabled(), isJsDelivrEnabled(), setEnableMaxmind(), and
getEnableMaxmind to make these changes.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@composer.json`:
- Around line 39-49: Add an integration test that reproduces regression `#164`:
create a new test file (e.g., tests/admin-ajax-geoip-regression-164-test.php)
that exercises the admin fallback → AJAX handler flow, the capability/AJAX
guards, and verifies slimstat_last_geoip_dl is updated (ensuring the
self-replicating loop is resolved); then add a composer script entry (e.g.,
"test:geoip-regression-164": "php
tests/admin-ajax-geoip-regression-164-test.php") and include that script in the
"test:all" array so the aggregate suite covers this regression path alongside
the existing scripts ("test:license-tags", "test:geoip-resolver",
"test:geoservice", "test:legacy-sync", "test:lazy-migration").

In `@tests/lazy-migration-test.php`:
- Around line 37-73: The test duplicates the lazy migration logic instead of
exercising the real path; remove the local implementations (the local
lazy_migrate() and wp_slimstat::resolve_geolocation_provider()) and instead load
or reference the canonical implementation from admin/config/index.php (or
extract that migration logic into a shared helper that both the production code
and tests require). Ensure the test bootstraps the real resolver so assertions
exercise the actual migration flow (call the real lazy_migrate() from the
shared/loaded code) and keep stubs/mocks only for external dependencies, not the
migration logic itself.

In `@tests/legacy-sync-mapping-test.php`:
- Around line 28-46: The test currently shadows production logic by implementing
sync_enable_maxmind() and a handwritten resolver copy; instead, remove this
duplicate mapping and have the test call the real production mapping/save logic
used by the app (i.e., replace sync_enable_maxmind() and the resolver copy with
a call to the actual save handler or shared mapping function used in
production), or move the mapping into a shared function in production code and
import it into the test; also eliminate the extra unknown-provider fallback in
the test so behavior matches the real save handler exactly.

In `@tests/resolve-geolocation-provider-test.php`:
- Around line 37-66: The test currently defines a local stub of
wp_slimstat::resolve_geolocation_provider() which masks regressions in the real
implementation; instead, remove the local method stub and update the test to
exercise the production resolver by either requiring or autoloading the real
implementation (the original resolve_geolocation_provider logic from
wp-slimstat.php) or by extracting that resolver into a shared module used by
both production code and this test; locate references to
wp_slimstat::resolve_geolocation_provider and replace the local class definition
with a call/load of the production module so the assertions validate the real
code path.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/helpers/setup.ts`:
- Around line 15-24: The tests currently use hardcoded filesystem and socket
paths (WP_ROOT, WP_CONFIG, WP_CONTENT, MU_PLUGINS, LOGGER_SRC, LOGGER_DEST,
AJAX_LOG, CRON_LINE, MYSQL_SOCKET) which breaks portability; update setup.ts to
read these values from environment variables or a shared config with sensible
defaults (e.g., process.env.WP_ROOT || fallback) and replace direct constants
with those variables so local dev machines and CI can override paths and the
MySQL socket via env vars or a config file.

In `@tests/perf/geoip-load.js`:
- Around line 119-135: The teardown currently fetches the AJAX log over HTTP
using http.get(`${BASE_URL}/wp-content/geoip-ajax-calls.log`) inside the
teardown function, which can expose .log files; change this to avoid HTTP
exposure by either (A) reading the log file directly with k6's file APIs (e.g.,
use open or other local file-read mechanism at runtime) when tests run locally,
replacing the http.get call and subsequent log parsing, or (B) add a guard that
requires an explicit opt-in env flag before attempting HTTP access and otherwise
log a warning to check server config; update code references in teardown, the
BASE_URL/http.get usage, and the lines.length checks accordingly.
- Around line 71-92: The setup() function currently authenticates once but does
not propagate cookies to VU iterations; capture the authentication cookies from
loginRes in setup() (e.g., return loginRes.cookies or a structured cookies
object) and then in default(data) use http.cookieJar() to set those cookies into
the VU's jar before hitting admin pages so each VU is authenticated;
alternatively move the login logic from setup() into default() so each VU
performs its own POST to `${BASE_URL}/wp-login.php` (refer to setup(), loginRes,
loginOk, default(), and http.cookieJar()).

In `@tests/run-qa.sh`:
- Around line 12-16: The script hardcodes machine-specific paths (WP_ROOT,
WP_CONFIG, MU_PLUGINS, AJAX_LOG, MYSQL_SOCKET) which breaks portability; update
each variable to read from an environment variable with a sensible fallback
default (e.g., WP_ROOT="${WP_ROOT:-./public}" style) and derive related paths
from WP_ROOT (so WP_CONFIG, MU_PLUGINS, AJAX_LOG are constructed from WP_ROOT)
and make MYSQL_SOCKET configurable via an env var
(MYSQL_SOCKET="${MYSQL_SOCKET:-/var/run/mysqld/mysqld.sock}") so other devs/CI
can override values without editing the script.
- Around line 98-100: The script uses a macOS-specific sed invocation (the
string starting with sed -i '' "/That's all, stop editing/i\\
define('DISABLE_WP_CRON', true);" "$WP_CONFIG") which breaks on Linux; replace
it with a portable approach such as using sed -i.bak to create a temporary
backup and then remove the .bak file (or detect OS and use -i '' on Darwin but
-i on others), so update the sed invocation in tests/run-qa.sh to use sed -i.bak
for the insert and then delete the backup to maintain cross-platform
compatibility.

---

Nitpick comments:
In `@tests/e2e/geoip-ajax-loop.spec.ts`:
- Around line 144-152: Remove the dead page.evaluate block that assigns `nonce`
but immediately returns null and is never used: delete the fetch call inside the
test (the anonymous async passed to page.evaluate that posts to
'/wp-admin/admin-ajax.php' with body
'action=slimstat_update_geoip_database&security=invalid_nonce') and the unused
`const nonce = await page.evaluate(...)` declaration so the test relies on the
subsequent working approach at lines 163-177; ensure no other references to
`nonce` remain.
- Around line 46-57: Replace brittle fixed delays in the test "Test 1: single
admin page load triggers at most 1 AJAX call" (and the similar waits around
lines 71-76) by waiting for the specific AJAX activity instead of using
page.waitForTimeout(4000); locate the test function and the helper readAjaxLog
and replace the timeout with an awaited waitForResponse (or a short polling
loop) that checks for responses whose URL contains "admin-ajax.php" and whose
request post data includes "slimstat_update_geoip_database", with a reasonable
timeout (e.g., 5s) and a catch that allows no response as valid; this ensures
the assertion on readAjaxLog().length remains stable without hard-coded sleeps.

In `@tests/e2e/global-setup.ts`:
- Around line 43-44: The globalSetup function currently hardcodes baseURL;
instead extract the base URL from the provided Playwright config: inspect
config.projects (e.g., find the first project or the project matching your test
environment) and read its use.baseURL (or use.playwright options) to set
baseURL, falling back to a sensible default if no baseURL is present; update the
code in globalSetup to replace the hardcoded baseURL with this derived value and
handle missing/undefined values gracefully.
- Around line 51-52: Replace the hardcoded credentials 'parhumm' and
'testpass123' with environment-driven defaults (e.g., use
process.env.E2E_TEST_USER ?? 'parhumm' and process.env.E2E_TEST_PASS ??
'testpass123') wherever those literals appear (also update the other occurrences
noted at the second pair) so tests read credentials from env vars with the same
fallback values; ensure any helper/initializer (the array or variables that
currently contain 'parhumm'/'testpass123') now references these env variables.

In `@tests/e2e/helpers/setup.ts`:
- Around line 72-77: The readAjaxLog function currently calls JSON.parse(line)
directly which will throw on malformed lines and crash tests; update readAjaxLog
to wrap the JSON.parse call in a try/catch (inside the map/filter pipeline) so
malformed JSON entries are caught, skipped (or collected as parse errors), and
do not abort the entire function—optionally emit a warning (e.g., console.warn)
with the offending line or index for debugging while continuing to return the
successfully parsed AjaxLogEntry[].
- Around line 135-140: Escape regex metacharacters in the `key` before building
the `keyPattern` RegExp to avoid pattern injection; create an escapedKey (e.g.,
replace /[.*+?^${}()|[\]\\]/g) and use that in the new RegExp for `keyPattern`
while keeping the original `key` (and `value`) when computing the `replacement`
string so lengths remain correct; update the code that creates `keyPattern` and
`replacement` (symbols: keyPattern, replacement) to use escapedKey for the regex
and key for the serialized length.

In `@tests/run-qa.sh`:
- Line 131: The phase heading "Phase 7: Post-test DB verification" is
misnumbered relative to the earlier "Phase 4" and should be corrected; locate
the string "Phase 7: Post-test DB verification" in tests/run-qa.sh and either
renumber it to "Phase 5: Post-test DB verification" (if phases 5–6 were never
intended) or insert/rename the missing intermediate phase headings consistent
with the rest of the script so the sequence is contiguous.
- Around line 111-112: Replace the hardcoded password in the mysql call `mysql
--socket="$MYSQL_SOCKET" -u root -proot local -e "DELETE FROM wp_options WHERE
option_name = 'slimstat_last_geoip_dl';"` so credentials are not on the command
line; for example, read user/password from environment variables
(MYSQL_USER/MYSQL_PASSWORD) and invoke without `-p` while exporting
MYSQL_PWD="$MYSQL_PASSWORD" before the call, or use a temporary
`--defaults-extra-file` or `--login-path` pointing to a .my.cnf file generated
from those env vars; update the invocation to use the variableized user (replace
`-u root` with `-u "$MYSQL_USER"`) and remove the `-proot` literal.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/TESTING.md`:
- Around line 87-96: The markdown in TESTING.md contains unlabeled code fences
for the directory trees (the blocks showing "tests/e2e/" and "wp-slimstat/");
update each opening triple-backtick to include the language label "text" (i.e.,
replace ``` with ```text) for the directory-tree fences referenced in the diff
and the additional similar block around lines 274–298 so markdownlint MD040 is
satisfied, keeping the rest of the fence content unchanged.
- Around line 27-30: The TESTING.md section currently hard-codes local test
credentials and a machine-specific MySQL socket (usernames 'parhumm' and
'dordane', password 'testpass123', and path under 'conf/mysql/mysqld.sock');
replace these with generic placeholders or references to environment variables
(e.g., MYSQL_USER, MYSQL_PASSWORD, MYSQL_SOCKET) and update the "Local by
Flywheel" entry to show how to populate those placeholders or read from env;
ensure you remove any absolute paths and instead instruct readers to locate
their socket (or set MYSQL_SOCKET) so no machine-specific data remains checked
in.

In `@tests/e2e/geoip-ajax-loop.spec.ts`:
- Around line 136-187: The test "Test 6: direct AJAX POST — no self-recursion"
is flaky because prior navigations to '/wp-admin/' and
'/wp-admin/admin.php?page=slimstat' can enqueue the same server-side fallback
after clearAjaxLog() runs; remove those priming navigations or ensure the
navigation-triggered handler has fully settled before clearing the log. To fix,
either (A) eliminate the page.goto calls and perform the authenticated POST
directly via page.request.post (keep action 'slimstat_update_geoip_database' and
security field) so clearAjaxLog() only clears pre-existing entries, or (B) if
you must navigate, after each page.goto call wait for background AJAX to finish
(e.g., poll readAjaxLog() until its length is stable for a short period) before
calling clearAjaxLog() and before performing the POST, then assert
expect(readAjaxLog().length).toBe(1). Ensure changes touch the test function and
use clearAjaxLog, clearGeoipTimestamp, page.request.post, readAjaxLog, and the
final expect call.
- Around line 46-57: The tests currently only assert an upper bound on
readAjaxLog() so a zero-invocation (fallback never firing) still passes; update
the e2e tests (the Test 1/2/3 cases that call readAjaxLog()) to first set the
GeoIP provider state to force the fallback path (e.g., disable the primary
provider or set the plugin/transient/config that triggers fallback) before
navigation, then assert a positive invocation count (use
expect(log.length).toBeGreaterThan(0) or an exact expected count) instead of
only toBeLessThanOrEqual(1), referencing the test blocks named "Test 1: single
admin page load ..." and the helper readAjaxLog() so the test guarantees the
fallback actually ran.