tests/e2e/helpers/custom-db-simulator-mu-plugin.php (1)

21-21: Use a site-scoped prefix in the simulator to avoid multisite cross-site leakage.

Using a fixed slimext_ prefix can cause table collisions if these tests run in multisite contexts.

♻️ Suggested refactor

-    $custom->prefix = 'slimext_';
+    $custom->prefix = is_multisite() && get_current_blog_id() > 1
+        ? 'slimext_' . get_current_blog_id() . '_'
+        : 'slimext_';

Based on learnings: Test for multisite compatibility; ensure data is stored per-site with no cross-site leakage; test network activate/deactivate.

🤖 Prompt for AI Agents

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

In `@tests/e2e/helpers/custom-db-simulator-mu-plugin.php` at line 21, The
simulator currently sets a fixed table prefix via $custom->prefix = 'slimext_',
which can collide across sites in multisite tests; change it to use a
site-scoped prefix by deriving it from WordPress (e.g. use
$wpdb->get_blog_prefix(get_current_blog_id()) or $wpdb->prefix) and append the
existing 'slimext_' suffix so tables are namespaced per-site; update any related
setup/teardown in the helper so network activate/deactivate and per-site cleanup
use the same derived prefix.

tests/e2e/databuckets-custom-db-accuracy.spec.ts (1)

148-157: Harden sumV1 against string-valued datasets.

If v1 values ever come through as strings, a + b can concatenate instead of sum. Coercing values to numbers makes this helper resilient.

Proposed refactor

 function sumV1(json: any): number {
+  const sumNumeric = (arr: unknown[]) => arr.reduce((a, b) => a + Number(b ?? 0), 0);
+
   // AJAX response: { success: true, data: { data: { datasets: { v1: [...] } } } }
   const ajaxDatasets = json?.data?.data?.datasets?.v1;
   if (Array.isArray(ajaxDatasets)) {
-    return (ajaxDatasets as number[]).reduce((a, b) => a + b, 0);
+    return sumNumeric(ajaxDatasets);
   }
   // Server-rendered: { datasets: { v1: [...] } }
   const serverDatasets = json?.datasets?.v1;
   if (Array.isArray(serverDatasets)) {
-    return (serverDatasets as number[]).reduce((a, b) => a + b, 0);
+    return sumNumeric(serverDatasets);
   }
   return 0;
 }

🤖 Prompt for AI Agents

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

In `@tests/e2e/databuckets-custom-db-accuracy.spec.ts` around lines 148 - 157, The
sumV1 helper should coerce dataset entries to numbers to avoid string
concatenation: in function sumV1, when handling ajaxDatasets and serverDatasets,
map each entry to Number(entry) (or use unary +) and filter out NaN before
reducing, then use numeric reduce (e.g., (a,b)=>a+b,0) to return a correct
numeric sum; update references to ajaxDatasets and serverDatasets accordingly so
both paths handle string values robustly.

tests/e2e/weekly-bucketing-last-week-count.spec.ts (5)

281-284: Weak assertion may miss bucketing errors.

Using toBeGreaterThanOrEqual(3) is a loose check. If there's a bug that causes extra data to land in this bucket (e.g., duplication), this assertion would still pass. Consider calculating and asserting the exact expected value.

🎯 Consider precise assertion

       // Second-to-last should contain Mar 14 (Sat) = part of Mar 8 (Sun) week
       // Plus any other data in that week
       const secondToLast = v1[v1.length - 2];
-      expect(secondToLast, `Second-to-last bucket should include Mar 14=3`).toBeGreaterThanOrEqual(3);
+      // With sow=0: Mar 8 (Sun) week contains Mar 8-14; Mar 14=3 is the only seeded day in that week
+      expect(secondToLast, `Second-to-last bucket should be exactly 3 (Mar 14 only)`).toBe(3);

🤖 Prompt for AI Agents

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

In `@tests/e2e/weekly-bucketing-last-week-count.spec.ts` around lines 281 - 284,
The assertion for the second-to-last bucket (variable secondToLast from array
v1) is too loose; compute the exact expected count for that bucket based on the
test input (including Mar 14 and any other known events that should fall into
the Mar 8 week) and replace the toBeGreaterThanOrEqual(3) check with a strict
equality assertion (e.g., expect(secondToLast).toEqual(EXACT_COUNT)). Update the
test to calculate EXACT_COUNT deterministically from the test fixtures or
hardcode the known correct sum so the test fails on duplication/mis-bucketing.

---

143-146: Unused helper function dayName.

The dayName function is defined but never called anywhere in the test file. Consider removing it to reduce dead code, or add a comment if it's intended for debugging.

🧹 Remove unused function

-/** Get day of week name for a timestamp */
-function dayName(ts: number): string {
-  return new Date(ts * 1000).toUTCString().split(',')[0];
-}
-

🤖 Prompt for AI Agents

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

In `@tests/e2e/weekly-bucketing-last-week-count.spec.ts` around lines 143 - 146,
The helper function dayName is defined but never used; remove the dead code by
deleting the dayName(ts: number) function declaration (or, if you intend to keep
it for ad-hoc debugging, add a clear comment explaining its purpose and suppress
the unused-variable lint rule) so the file no longer contains the unused symbol
dayName.

---

210-212: Consider using Playwright's test.info() for structured test output.

Multiple console.log statements can clutter CI output. Playwright provides test.info().annotations or test.step() for structured logging that integrates better with reporters.

📊 Example using test.step for structured output

await test.step('Verify chart data', async () => {
  const v1 = getV1(json);
  const labels = getLabels(json);
  const chartSum = sumV1(json);
  
  test.info().annotations.push({ type: 'chart-data', description: `Labels: ${labels.join(', ')}` });
  test.info().annotations.push({ type: 'chart-data', description: `V1: ${v1.join(', ')}` });
  // assertions...
});

🤖 Prompt for AI Agents

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

In `@tests/e2e/weekly-bucketing-last-week-count.spec.ts` around lines 210 - 212,
Replace the three console.log calls that print labels, v1 and the chart sum with
structured Playwright reporting: wrap the verification in a test.step('Verify
chart data', ...) and use test.info().annotations.push(...) (or
test.info().annotations = [...]) to add annotations for Labels, V1 and Chart sum
(referencing the local vars labels, v1, chartSum and totalExpected) so the
output integrates with reporters and avoids cluttering CI logs; ensure
assertions remain inside the step.

---

108-109: Hardcoded wp_ table prefix may break in alternative test environments.

The table name wp_slim_stats and wp_options are hardcoded. If the test environment uses a custom table prefix (e.g., wp_test_), these queries will fail silently or target the wrong tables.

Consider using a helper that reads the prefix from WP_ROOT/wp-config.php or an environment variable.

Also applies to: 116-118

🤖 Prompt for AI Agents

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

In `@tests/e2e/weekly-bucketing-last-week-count.spec.ts` around lines 108 - 109,
The hardcoded table names ('INSERT INTO wp_slim_stats ...' and references to
'wp_options') will break when a WP table prefix differs; change these queries to
use a resolved table prefix by reading the prefix from wp-config.php or an
environment variable (e.g., process.env.WP_TABLE_PREFIX) and build table names
like `${prefix}slim_stats` and `${prefix}options` before executing them,
replacing the literal 'wp_slim_stats' and 'wp_options' strings in the test SQL
statements.

---

175-203: Test assumes default start_of_week=1 but doesn't explicitly set it.

Unlike Tests 2 and 4 which snapshot and explicitly set start_of_week, this test relies on the default being 1 (Monday). If a previous test run fails before restoring the option, or if the test environment has a different default, this test may produce unexpected results.

Consider adding explicit setup for consistency:

🔧 Explicitly set start_of_week for test isolation

   test('March 14 data lands in Mar 9 bucket, NOT in Mar 16 bucket (sow=1)', async () => {
+    // Ensure default Monday start
+    await snapshotOption('start_of_week');
+    await getPool().execute("UPDATE wp_options SET option_value = '1' WHERE option_name = 'start_of_week'");
+
+    try {
     // Seed: 3 hits on Mar 14 (Saturday), 2 hits on Mar 15 (Sunday),
     //        5 hits on Mar 16 (Monday), 4 hits on Mar 17 (Tuesday)
     // ... existing code ...
     console.log('Test 1 PASS — Mar 14 correctly in Mar 9 bucket, Mar 16-17 in last bucket');
+    } finally {
+      await restoreOption('start_of_week');
+    }
   });

🤖 Prompt for AI Agents

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

In `@tests/e2e/weekly-bucketing-last-week-count.spec.ts` around lines 175 - 203,
This test relies on the default start_of_week=1 but doesn't set it explicitly;
modify the test (the 'March 14 data lands...' case that calls utcMidnight,
insertRows and fetchChartData) to explicitly set start_of_week to 1 at test
setup and restore the original value at teardown using the same
snapshot/set/restore pattern used by Tests 2 and 4 so the test is isolated from
global defaults or prior failures.

src/Helpers/DataBuckets.php (1)

216-216: Match the repo's Yoda rule for the lower-bound check.

Line 216 is the only changed literal comparison here that still uses non-Yoda ordering.

♻️ Suggested tweak

-        if ($offset >= 0 && $offset < $this->points) {
+        if ( 0 <= $offset && $offset < $this->points ) {

As per coding guidelines "Use Yoda Conditions: `if ( 'value' === $var )`."

🤖 Prompt for AI Agents

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

In `@src/Helpers/DataBuckets.php` at line 216, Change the lower-bound comparison
in the conditional that checks $offset so it uses Yoda-style ordering: replace
the `$offset >= 0` part with `0 <= $offset` while keeping the upper-bound check
`$offset < $this->points` intact; update the condition around the `$offset`
variable in the DataBuckets class (the `if ($offset >= 0 && $offset <
$this->points)` statement) to `if (0 <= $offset && $offset < $this->points)`.

tests/e2e/chart-monthly-daily-granularity.spec.ts (2)

95-106: ⚠️ Potential issue | 🟡 Minor

D2 should wrap sow mutations in try/finally for guaranteed cleanup.

If any assertion fails in the loop, restoreOption('start_of_week') at line 106 won't execute, leaving the option dirty for subsequent tests.

🔧 Wrap in try/finally

     await snapshotOption('start_of_week');
-
     const results: Record<string, number> = {};
-
-    for (const sow of ['1', '0', '6']) {
-      await getPool().execute("UPDATE wp_options SET option_value = ? WHERE option_name = 'start_of_week'", [sow]);
-      const json = fetchChartData(rangeStart, rangeEnd, 'daily');
-      expect(json?.success).toBe(true);
-      results[`sow${sow}`] = sumArr(getV1(json));
-    }
-
-    await restoreOption('start_of_week');
+    try {
+      for (const sow of ['1', '0', '6']) {
+        await getPool().execute("UPDATE wp_options SET option_value = ? WHERE option_name = 'start_of_week'", [sow]);
+        const json = fetchChartData(rangeStart, rangeEnd, 'daily');
+        expect(json?.success).toBe(true);
+        results[`sow${sow}`] = sumArr(getV1(json));
+      }
+    } finally {
+      await restoreOption('start_of_week');
+    }

🤖 Prompt for AI Agents

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

In `@tests/e2e/chart-monthly-daily-granularity.spec.ts` around lines 95 - 106, The
test mutates the WP option via getPool().execute in the for-loop and calls
restoreOption('start_of_week') after the loop, which can be skipped if an
assertion fails; wrap the mutation+assert loop in a try/finally so
restoreOption('start_of_week') always runs: call snapshotOption('start_of_week')
before the try, perform the for-loop where you set the option
(getPool().execute), call fetchChartData and assertions (fetchChartData, getV1,
sumArr) inside the try, and place restoreOption('start_of_week') in the finally
block to guarantee cleanup even on failures.

---

219-230: ⚠️ Potential issue | 🟡 Minor

M2 should wrap sow mutations in try/finally for guaranteed cleanup.

Same issue as D2 — if assertions fail, restoreOption won't execute.

🔧 Wrap in try/finally

     await snapshotOption('start_of_week');
-
     const results: Record<string, number> = {};
-
-    for (const sow of ['1', '0', '6']) {
-      await getPool().execute("UPDATE wp_options SET option_value = ? WHERE option_name = 'start_of_week'", [sow]);
-      const json = fetchChartData(rangeStart, rangeEnd, 'monthly');
-      expect(json?.success).toBe(true);
-      results[`sow${sow}`] = sumArr(getV1(json));
-    }
-
-    await restoreOption('start_of_week');
+    try {
+      for (const sow of ['1', '0', '6']) {
+        await getPool().execute("UPDATE wp_options SET option_value = ? WHERE option_name = 'start_of_week'", [sow]);
+        const json = fetchChartData(rangeStart, rangeEnd, 'monthly');
+        expect(json?.success).toBe(true);
+        results[`sow${sow}`] = sumArr(getV1(json));
+      }
+    } finally {
+      await restoreOption('start_of_week');
+    }

🤖 Prompt for AI Agents

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

In `@tests/e2e/chart-monthly-daily-granularity.spec.ts` around lines 219 - 230,
The test mutates the WP option 'start_of_week' with getPool().execute inside a
loop but doesn't guarantee restoreOption runs if an assertion fails; wrap the
mutation+assertion loop in a try/finally: call await
snapshotOption('start_of_week') before the try, perform the for-loop (updates,
fetchChartData, expect, results accumulation) inside the try, and ensure await
restoreOption('start_of_week') is invoked in the finally block so cleanup always
occurs; reference functions: snapshotOption, getPool().execute, fetchChartData,
getV1, sumArr, restoreOption.

tests/e2e/weekly-bucketing-last-week-count.spec.ts (1)

43-92: ⚠️ Potential issue | 🟡 Minor

Test 1 should wrap mutations in try/finally to ensure cleanup.

Unlike Tests 2-5 which properly use try/finally, Test 1 calls restoreOption('start_of_week') only on the success path (line 91). If any assertion fails, start_of_week remains dirty for subsequent tests.

🔧 Wrap in try/finally

   test('March 14 data lands in Mar 9 bucket, NOT in Mar 16 bucket (sow=1)', async () => {
     await snapshotOption('start_of_week');
     await getPool().execute("UPDATE wp_options SET option_value = '1' WHERE option_name = 'start_of_week'");
 
+    try {
       const mar14 = utcMidnight('2026-03-14');
       // ... rest of test logic ...
       console.log('Test 1 PASS — Mar 14 correctly in Mar 9 bucket, Mar 16-17 in last bucket');
-
-    await restoreOption('start_of_week');
+    } finally {
+      await restoreOption('start_of_week');
+    }
   });

🤖 Prompt for AI Agents

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

In `@tests/e2e/weekly-bucketing-last-week-count.spec.ts` around lines 43 - 92, The
test "March 14 data lands in Mar 9 bucket, NOT in Mar 16 bucket (sow=1)" mutates
global state (calls snapshotOption and getPool().execute to set start_of_week)
but only calls restoreOption('start_of_week') on the success path; wrap the body
that seeds rows and assertions in a try/finally block so
restoreOption('start_of_week') runs in finally, ensuring cleanup even if
assertions fail, keeping snapshotOption(), getPool().execute(... 'start_of_week'
...), insertRows(...), fetchChartData(...), and related assertions inside the
try and calling restoreOption in the finally.

tests/e2e/chart-robustness.spec.ts (2)

44-51: Use parameterized queries for SQL to avoid injection risk.

The SET time_zone statement uses string interpolation which, while safe with the current hardcoded test values, establishes an unsafe pattern. Consider using parameterized execution or validating the input format.

♻️ Safer approach with validation

 async function setTimezone(mysqlTz: string, wpGmtOffset: string): Promise<void> {
+  // Validate timezone format (+HH:MM or -HH:MM)
+  if (!/^[+-]\d{2}:\d{2}$/.test(mysqlTz)) {
+    throw new Error(`Invalid timezone format: ${mysqlTz}`);
+  }
   const pool = getPool();
-  await pool.execute(`SET time_zone = '${mysqlTz}'`);
+  await pool.execute(`SET time_zone = ?`, [mysqlTz]);
   await pool.execute(
     "UPDATE wp_options SET option_value = ? WHERE option_name = 'gmt_offset'",
     [wpGmtOffset]
   );
 }

🤖 Prompt for AI Agents

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

In `@tests/e2e/chart-robustness.spec.ts` around lines 44 - 51, The setTimezone
function currently interpolates mysqlTz into the SQL string (in setTimezone and
pool.execute calls), which is unsafe; update setTimezone to avoid direct string
interpolation by using a parameterized query for the SET time_zone statement
(use pool.execute with a placeholder and pass mysqlTz) or, if the DB driver
disallows parameterizing SET, strictly validate/whitelist mysqlTz before
interpolating (e.g., allow only offsets like '+00:00' or known timezone names)
and keep the wp_options update using the existing parameterized placeholder for
wpGmtOffset; ensure references to setTimezone, getPool, and pool.execute are
updated accordingly.

---

294-297: Consider replacing fixed timeout with explicit wait condition.

page.waitForTimeout(3000) is brittle and slows tests unnecessarily. Consider waiting for a specific DOM change or network idle state after granularity selection.

♻️ More robust wait

     await granSelect.selectOption(targetGran);
 
-    // Wait for AJAX update
-    await page.waitForTimeout(3000);
+    // Wait for chart data to update after AJAX
+    await page.waitForFunction(
+      (el, oldData) => el?.getAttribute('data-data') !== oldData,
+      { timeout: 10_000 },
+      await chartDataEl.elementHandle(),
+      initialDataRaw
+    );

🤖 Prompt for AI Agents

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

In `@tests/e2e/chart-robustness.spec.ts` around lines 294 - 297, Replace the
brittle fixed delay call page.waitForTimeout(3000) with an explicit wait that
targets the post-granularity-change condition: for example, wait for the
specific DOM update (use page.waitForSelector or page.waitForFunction to detect
the chart element/text/attribute that changes after granularity selection) or
wait for the relevant network response using page.waitForResponse matching the
request URL/endpoint; update the test around the granularity selection step in
chart-robustness.spec.ts to use one of these explicit waits instead of
page.waitForTimeout(3000).

tests/e2e/chart-negative-regression.spec.ts (1)

61-147: runBucketTest function is defined but never called.

This function generates PHP scripts for comparing master vs current DataBuckets behavior, but neither NEG-1 nor NEG-2 actually invokes it. The tests instead use fetchChartData() from helpers and verify behavior through code content inspection.

Consider removing this dead code or documenting why it's retained for future use.

🤖 Prompt for AI Agents

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

In `@tests/e2e/chart-negative-regression.spec.ts` around lines 61 - 147,
runBucketTest is dead code (defined but never invoked by NEG-1/NEG-2) — either
remove the entire runBucketTest function from chart-negative-regression.spec.ts
to eliminate unused code, or explicitly document why it’s retained (add a
top-of-function comment referencing NEG-1 and NEG-2 and that the tests use
fetchChartData() instead) so future readers know it’s intentionally kept; locate
the runBucketTest declaration and delete it or add the explanatory comment
accordingly.

tests/e2e/helpers/chart.ts (1)

19-50: PHP code generation interpolates values directly into strings.

While startTs and endTs are numbers and granularity comes from test code (not user input), the pattern of interpolating values into PHP strings is fragile. The server-side validation in Chart.php provides defense-in-depth, but consider using a more structured approach.

🤖 Prompt for AI Agents

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

In `@tests/e2e/helpers/chart.ts` around lines 19 - 50, The test currently injects
startTs, endTs and granularity directly into the phpCode string inside
fetchChartData, which is fragile; change fetchChartData to serialize the
parameters into a single safe payload (e.g., JSON or base64) and embed only that
encoded string in phpCode, then decode/json_decode it in the PHP snippet and set
$_POST/$_REQUEST from the decoded structure before calling
\\SlimStat\\Modules\\Chart::ajaxFetchChartData(); specifically, stop
interpolating ${startTs}, ${endTs} and '${granularity}' directly and instead
write something like a JSON-encoded argsPayload in the tmpFile and have the PHP
use json_decode to populate $_POST['args'], $_POST['granularity'] and $_REQUEST
values so values are passed structurally and not string-interpolated.