Fix weekly chart not showing today's data#235
Merged
parhumm merged 5 commits intodevelopmentfrom Mar 17, 2026
Merged
Conversation
The weekly offset calculation was using ISO week numbers (date('W'))
which always start on Monday, regardless of WordPress start_of_week
setting. This caused data to be placed in the wrong bucket when
start_of_week was set to Sunday (0).
Now calculates the start-of-week date for both base and data point
timestamps using the WordPress start_of_week setting, ensuring
correct bucket placement.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWeekly bucketing now aligns week boundaries to WordPress's start_of_week: addRow snaps timestamps to week-starts and computes week offsets from whole-week timestamp differences; toArray uses the same week-start logic for determining the "today" label. A helper getWeekStartTimestamp(int) was added; E2E tests were added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Helpers/DataBuckets.php`:
- Around line 198-211: The current-week check using wp_date('YW', $this->end)
must be replaced with the same WordPress start_of_week logic used earlier (the
block that computes $startOfWeek, $baseDayOfWeek, $baseDiff, $baseWeekStart and
$dtWeekStart). Compute $startOfWeek = (int) get_option('start_of_week', 1) and
then derive the week-start timestamp for $this->end exactly like the earlier
code (determine day-of-week via date('w'), compute diff, subtract days and
normalize to Y-m-d then strtotime) and compare that week-start to the current
week-start (or to the other week-start you previously compare against) instead
of using wp_date('YW', ...); update the conditional that references
wp_date('YW', $this->end) to use the calculated week-start timestamp equality
check so week bucketing honors the site start_of_week setting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f580785e-2a0e-4e6b-861a-316f7c540bc7
📒 Files selected for processing (1)
src/Helpers/DataBuckets.php
Validates weekly chart DataBuckets offset calculation respects WordPress start_of_week setting (Sunday/Monday/Saturday), cross-granularity consistency, and week boundary behavior. 5/5 pass.
Contributor
title: "PR #235 — Weekly chart start_of_week bucketing"
|
| # | Scenario | Why |
|---|---|---|
| 1 | start_of_week=0 (Sunday) |
Direct bug reproduction — the reported failure case |
| 2 | start_of_week=1 (Monday) |
Default/ISO-aligned — no regression check |
| 3 | start_of_week=6 (Saturday) |
Edge case — least common week start |
| 4 | Cross-granularity sow=0 | Daily vs weekly totals must match regardless of week start |
| 5 | Week boundary split | Sunday records must land in different buckets for sow=0 vs sow=1 |
Environment
| Property | Value |
|---|---|
| SlimStat Core | 5.4.4 |
| SlimStat Pro | n/a |
| WordPress | 6.9.4 |
| PHP | 8.5.0 |
| Site Profile | publisher |
| Environment | local (Local by Flywheel) |
| Plugin Scope | core |
Results Summary
| Category | Pass | Fail | Skip |
|---|---|---|---|
| tracking (chart accuracy) | 5 | 0 | 0 |
Regression Check
38 existing chart-related tests also executed (via --grep "chart"):
| Suite | Pass | Fail | Skip |
|---|---|---|---|
| adminbar-chart-consistency | 4 | 0 | 1 |
| chart-sql-expression-validation | 4 | 0 | 0 |
| databuckets-custom-db-accuracy | 7 | 0 | 0 |
| databuckets-no-calendar-ext | 3 | 0 | 0 |
| outbound-report-display | 1 | 0 | 0 |
| overview-charts-accuracy | 14 | 0 | 0 |
| weekly-start-of-week-bucketing (new) | 5 | 0 | 0 |
| Total | 38 | 0 | 1 |
Test Details
Test 1: start_of_week=0 (Sunday) — Bug Case
- Setup: 5 records at now-60s ("today"), 8 records at now-7d
- Assert:
chartSum == 13, last bucket >= 5 - Result: PASS — DB=13, sum=13, lastBucket=5
Test 2: start_of_week=1 (Monday) — Default
- Setup: 4 records at now-60s, 6 records at now-7d
- Assert:
chartSum == 10, last bucket >= 4 - Result: PASS — DB=10, sum=10, lastBucket=4
Test 3: start_of_week=6 (Saturday) — Edge Case
- Setup: 3 records at now-60s, 5 records at now-14d
- Assert:
chartSum == 8, last bucket >= 3 - Result: PASS — DB=8, sum=8, lastBucket=3
Test 4: Cross-Granularity Consistency (sow=0)
- Setup: 3+5+4 = 12 records across 3 time points
- Assert: daily sum == weekly sum == DB count
- Result: PASS — DB=12, daily=12, weekly=12
Test 5: Week Boundary Split
- Setup: 4 records on Saturday, 3 records on Sunday
- Assert: Both sow=0 and sow=1 sum to 7; bucket distribution differs
- Result: PASS — sow=0:
[0,0,0,4,3](Sun starts new week), sow=1:[0,0,0,7,0](Sat+Sun same week)
Analytics Invariants
| Invariant | Threshold | Measured | Status |
|---|---|---|---|
| Event loss (server) | ≤ 0.05% | 0% | PASS |
| Duplicates | ≤ 0.1% | 0% | PASS |
| Cross-granularity consistency | 100% | 100% | PASS |
Failure Analysis
No failures.
Artifacts
- Spec file:
tests/e2e/weekly-start-of-week-bucketing.spec.ts - Execution time: 11.6s (new spec), 2.2m (full chart regression)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/weekly-start-of-week-bucketing.spec.ts`:
- Around line 132-137: clearTestData() currently uses TRUNCATE TABLE
wp_slim_stats which removes all rows and can break other specs; change it to
remove only rows created by this test run (or within the test time window) by
replacing the TRUNCATE with a targeted DELETE that includes a WHERE clause (e.g.
DELETE FROM wp_slim_stats WHERE test_run_id = ? OR created_at >= ?) and pass the
appropriate identifier/timestamp from the test setup, or alternatively mark
inserts with a unique test marker and DELETE WHERE marker = ?. Update the
clearTestData function (and calls that use getPool()) to use that targeted
DELETE so cleanup is scoped to this spec rather than truncating the entire
table.
- Around line 319-357: The test currently only checks totals; update the 'Sunday
records land in correct week bucket for sow=0 vs sow=1' test to assert the
actual weekly bucket placement by using the same binning used by fetchChartData:
call setStartOfWeek(0) and setStartOfWeek(1) as already done, then inspect the
returned v1 arrays from fetchChartData (variables jsonSow0/jsonSow1 and
v1Sow0/v1Sow1) and assert that the Saturday timestamp inserted via
insertRows(saturdayTs, 4, ...) and the Sunday timestamp inserted via
insertRows(sundayTs, 3, ...) land in different bucket indices for sow=0 but the
same bucket index for sow=1; determine the bucket index by computing the weekly
bin index relative to rangeStart using the same week-start logic (start_of_week
offset) used by the implementation (or reuse any existing helper that maps a
timestamp to a weekly bucket), then assert v1Sow0[indexSat] === 4 and
v1Sow0[indexSun] === 3 (or equivalent non-equality for different indices) and
v1Sow1[indexBoth] === 7 to ensure correct placement rather than just totals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 700b40f2-47a8-4a51-bc73-784a393aaf5b
📒 Files selected for processing (1)
tests/e2e/weekly-start-of-week-bucketing.spec.ts
Comment on lines
+132
to
+137
| async function clearTestData(): Promise<void> { | ||
| await getPool().execute("TRUNCATE TABLE wp_slim_stats"); | ||
| await getPool().execute( | ||
| "DELETE FROM wp_options WHERE option_name LIKE '_transient_wp_slimstat_%' OR option_name LIKE '_transient_timeout_wp_slimstat_%'" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid global table truncation in E2E cleanup.
clearTestData() currently does TRUNCATE TABLE wp_slim_stats, which can erase unrelated test data and introduce cross-test flakiness when other specs share the DB.
Suggested fix
async function clearTestData(): Promise<void> {
- await getPool().execute("TRUNCATE TABLE wp_slim_stats");
+ await getPool().execute(
+ "DELETE FROM wp_slim_stats WHERE user_agent = 'weekly-sow-e2e'"
+ );
await getPool().execute(
"DELETE FROM wp_options WHERE option_name LIKE '_transient_wp_slimstat_%' OR option_name LIKE '_transient_timeout_wp_slimstat_%'"
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/weekly-start-of-week-bucketing.spec.ts` around lines 132 - 137,
clearTestData() currently uses TRUNCATE TABLE wp_slim_stats which removes all
rows and can break other specs; change it to remove only rows created by this
test run (or within the test time window) by replacing the TRUNCATE with a
targeted DELETE that includes a WHERE clause (e.g. DELETE FROM wp_slim_stats
WHERE test_run_id = ? OR created_at >= ?) and pass the appropriate
identifier/timestamp from the test setup, or alternatively mark inserts with a
unique test marker and DELETE WHERE marker = ?. Update the clearTestData
function (and calls that use getPool()) to use that targeted DELETE so cleanup
is scoped to this spec rather than truncating the entire table.
Comment on lines
+319
to
+357
| test('Sunday records land in correct week bucket for sow=0 vs sow=1', async () => { | ||
| // Find the most recent Sunday timestamp | ||
| const sundayTs = mostRecentDayOfWeek(0, now); // 0 = Sunday | ||
| // And the Saturday before it | ||
| const saturdayTs = sundayTs - day; | ||
|
|
||
| // Insert records on Saturday and Sunday | ||
| await insertRows(saturdayTs, 4, 'boundary-sat'); | ||
| await insertRows(sundayTs, 3, 'boundary-sun'); | ||
|
|
||
| const dbCount = await countTestRows(); | ||
| expect(dbCount).toBe(7); | ||
|
|
||
| // With sow=0 (Sunday): Sunday starts a NEW week | ||
| setStartOfWeek(0); | ||
| const jsonSow0 = fetchChartData(rangeStart, rangeEnd, 'weekly'); | ||
| expect(jsonSow0?.success).toBe(true); | ||
| const v1Sow0 = getV1(jsonSow0); | ||
| const sumSow0 = sumV1(jsonSow0); | ||
| expect(sumSow0).toBe(dbCount); | ||
|
|
||
| // With sow=1 (Monday): Sunday is the LAST day of the week | ||
| setStartOfWeek(1); | ||
| const jsonSow1 = fetchChartData(rangeStart, rangeEnd, 'weekly'); | ||
| expect(jsonSow1?.success).toBe(true); | ||
| const v1Sow1 = getV1(jsonSow1); | ||
| const sumSow1 = sumV1(jsonSow1); | ||
| expect(sumSow1).toBe(dbCount); | ||
|
|
||
| // Both must account for all records | ||
| expect(sumSow0).toBe(sumSow1); | ||
|
|
||
| // The bucket distribution should differ: with sow=0, Sunday starts a new week | ||
| // so Saturday (4) and Sunday (3) are in different weeks. | ||
| // With sow=1, Saturday and Sunday are in the same week (both after Monday). | ||
| // We can't assert exact bucket indices without knowing the day-of-week of 'now', | ||
| // but we can verify the total is correct and no records are lost. | ||
| console.log(`Test 5 PASS — boundary: DB=${dbCount}, sow0=${JSON.stringify(v1Sow0)}, sow1=${JSON.stringify(v1Sow1)}`); | ||
| }); |
There was a problem hiding this comment.
Boundary test does not actually validate bucket placement.
The test name says Sunday/Saturday should land in different/same weekly buckets by start_of_week, but assertions only check totals. This can pass even if bucket assignment regresses.
Suggested assertion hardening
const v1Sow0 = getV1(jsonSow0);
@@
const v1Sow1 = getV1(jsonSow1);
@@
expect(sumSow0).toBe(sumSow1);
+
+ const nonZeroSow0 = v1Sow0.filter((v: number) => v > 0).sort((a: number, b: number) => a - b);
+ const nonZeroSow1 = v1Sow1.filter((v: number) => v > 0).sort((a: number, b: number) => a - b);
+
+ // sow=0: Saturday (4) and Sunday (3) should be split across weeks
+ expect(nonZeroSow0).toEqual([3, 4]);
+ // sow=1: Saturday and Sunday should be in the same week
+ expect(nonZeroSow1).toEqual([7]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('Sunday records land in correct week bucket for sow=0 vs sow=1', async () => { | |
| // Find the most recent Sunday timestamp | |
| const sundayTs = mostRecentDayOfWeek(0, now); // 0 = Sunday | |
| // And the Saturday before it | |
| const saturdayTs = sundayTs - day; | |
| // Insert records on Saturday and Sunday | |
| await insertRows(saturdayTs, 4, 'boundary-sat'); | |
| await insertRows(sundayTs, 3, 'boundary-sun'); | |
| const dbCount = await countTestRows(); | |
| expect(dbCount).toBe(7); | |
| // With sow=0 (Sunday): Sunday starts a NEW week | |
| setStartOfWeek(0); | |
| const jsonSow0 = fetchChartData(rangeStart, rangeEnd, 'weekly'); | |
| expect(jsonSow0?.success).toBe(true); | |
| const v1Sow0 = getV1(jsonSow0); | |
| const sumSow0 = sumV1(jsonSow0); | |
| expect(sumSow0).toBe(dbCount); | |
| // With sow=1 (Monday): Sunday is the LAST day of the week | |
| setStartOfWeek(1); | |
| const jsonSow1 = fetchChartData(rangeStart, rangeEnd, 'weekly'); | |
| expect(jsonSow1?.success).toBe(true); | |
| const v1Sow1 = getV1(jsonSow1); | |
| const sumSow1 = sumV1(jsonSow1); | |
| expect(sumSow1).toBe(dbCount); | |
| // Both must account for all records | |
| expect(sumSow0).toBe(sumSow1); | |
| // The bucket distribution should differ: with sow=0, Sunday starts a new week | |
| // so Saturday (4) and Sunday (3) are in different weeks. | |
| // With sow=1, Saturday and Sunday are in the same week (both after Monday). | |
| // We can't assert exact bucket indices without knowing the day-of-week of 'now', | |
| // but we can verify the total is correct and no records are lost. | |
| console.log(`Test 5 PASS — boundary: DB=${dbCount}, sow0=${JSON.stringify(v1Sow0)}, sow1=${JSON.stringify(v1Sow1)}`); | |
| }); | |
| test('Sunday records land in correct week bucket for sow=0 vs sow=1', async () => { | |
| // Find the most recent Sunday timestamp | |
| const sundayTs = mostRecentDayOfWeek(0, now); // 0 = Sunday | |
| // And the Saturday before it | |
| const saturdayTs = sundayTs - day; | |
| // Insert records on Saturday and Sunday | |
| await insertRows(saturdayTs, 4, 'boundary-sat'); | |
| await insertRows(sundayTs, 3, 'boundary-sun'); | |
| const dbCount = await countTestRows(); | |
| expect(dbCount).toBe(7); | |
| // With sow=0 (Sunday): Sunday starts a NEW week | |
| setStartOfWeek(0); | |
| const jsonSow0 = fetchChartData(rangeStart, rangeEnd, 'weekly'); | |
| expect(jsonSow0?.success).toBe(true); | |
| const v1Sow0 = getV1(jsonSow0); | |
| const sumSow0 = sumV1(jsonSow0); | |
| expect(sumSow0).toBe(dbCount); | |
| // With sow=1 (Monday): Sunday is the LAST day of the week | |
| setStartOfWeek(1); | |
| const jsonSow1 = fetchChartData(rangeStart, rangeEnd, 'weekly'); | |
| expect(jsonSow1?.success).toBe(true); | |
| const v1Sow1 = getV1(jsonSow1); | |
| const sumSow1 = sumV1(jsonSow1); | |
| expect(sumSow1).toBe(dbCount); | |
| // Both must account for all records | |
| expect(sumSow0).toBe(sumSow1); | |
| const nonZeroSow0 = v1Sow0.filter((v: number) => v > 0).sort((a: number, b: number) => a - b); | |
| const nonZeroSow1 = v1Sow1.filter((v: number) => v > 0).sort((a: number, b: number) => a - b); | |
| // sow=0: Saturday (4) and Sunday (3) should be split across weeks | |
| expect(nonZeroSow0).toEqual([3, 4]); | |
| // sow=1: Saturday and Sunday should be in the same week | |
| expect(nonZeroSow1).toEqual([7]); | |
| // The bucket distribution should differ: with sow=0, Sunday starts a new week | |
| // so Saturday (4) and Sunday (3) are in different weeks. | |
| // With sow=1, Saturday and Sunday are in the same week (both after Monday). | |
| // We can't assert exact bucket indices without knowing the day-of-week of 'now', | |
| // but we can verify the total is correct and no records are lost. | |
| console.log(`Test 5 PASS — boundary: DB=${dbCount}, sow0=${JSON.stringify(v1Sow0)}, sow1=${JSON.stringify(v1Sow1)}`); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/weekly-start-of-week-bucketing.spec.ts` around lines 319 - 357, The
test currently only checks totals; update the 'Sunday records land in correct
week bucket for sow=0 vs sow=1' test to assert the actual weekly bucket
placement by using the same binning used by fetchChartData: call
setStartOfWeek(0) and setStartOfWeek(1) as already done, then inspect the
returned v1 arrays from fetchChartData (variables jsonSow0/jsonSow1 and
v1Sow0/v1Sow1) and assert that the Saturday timestamp inserted via
insertRows(saturdayTs, 4, ...) and the Sunday timestamp inserted via
insertRows(sundayTs, 3, ...) land in different bucket indices for sow=0 but the
same bucket index for sow=1; determine the bucket index by computing the weekly
bin index relative to rangeStart using the same week-start logic (start_of_week
offset) used by the implementation (or reuse any existing helper that maps a
timestamp to a weekly bucket), then assert v1Sow0[indexSat] === 4 and
v1Sow0[indexSun] === 3 (or equivalent non-equality for different indices) and
v1Sow1[indexBoth] === 7 to ensure correct placement rather than just totals.
The toArray() method was using wp_date('YW', ...) to check if the
end date is in the current week, but 'YW' returns ISO week format
which always starts on Monday regardless of WordPress start_of_week.
Added getWeekStartTimestamp() helper method to calculate week start
respecting the WordPress setting, and refactored both addRow() and
toArray() to use this consistent logic.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Helpers/DataBuckets.php`:
- Around line 198-205: The modified PHP block in DataBuckets.php is indented
with spaces; update indentation to use tabs to match project standards: replace
leading spaces with tabs for the lines where getWeekStartTimestamp($base) and
getWeekStartTimestamp($dt) are called and the offset calculation uses
$baseWeekStart, $dtWeekStart and $offset, and also fix the same spacing-to-tab
indentation in the other changed regions mentioned (around the blocks that
include the week start calculations and the later blocks at the ranges
referenced). Ensure only indentation changes are made (no code logic changes)
and preserve existing variable/function names (getWeekStartTimestamp,
$baseWeekStart, $dtWeekStart, $offset).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0e4c57b-ccd4-4fba-8f20-9edf18b97f02
📒 Files selected for processing (1)
src/Helpers/DataBuckets.php
Comment on lines
+198
to
+205
| // Calculate the start-of-week date for both base and dt | ||
| // This respects WordPress start_of_week setting instead of using ISO weeks | ||
| $baseWeekStart = $this->getWeekStartTimestamp($base); | ||
| $dtWeekStart = $this->getWeekStartTimestamp($dt); | ||
|
|
||
| // Calculate weeks between the two week start dates | ||
| $offset = (int) round(($dtWeekStart - $baseWeekStart) / (7 * 86400)); | ||
|
|
There was a problem hiding this comment.
Use tabs for indentation in modified PHP lines.
The changed blocks are space-indented; please switch to tabs to match project PHP formatting standards.
As per coding guidelines, **/*.php: Use tabs (not spaces) for indentation.
Also applies to: 241-251, 292-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Helpers/DataBuckets.php` around lines 198 - 205, The modified PHP block
in DataBuckets.php is indented with spaces; update indentation to use tabs to
match project standards: replace leading spaces with tabs for the lines where
getWeekStartTimestamp($base) and getWeekStartTimestamp($dt) are called and the
offset calculation uses $baseWeekStart, $dtWeekStart and $offset, and also fix
the same spacing-to-tab indentation in the other changed regions mentioned
(around the blocks that include the week start calculations and the later blocks
at the ranges referenced). Ensure only indentation changes are made (no code
logic changes) and preserve existing variable/function names
(getWeekStartTimestamp, $baseWeekStart, $dtWeekStart, $offset).
Contributor
E2E QA Report: Weekly Bucketing — start_of_week Fix ValidationTest spec: Results: development (before fix) vs PR #235 (after fix)
Score: 3/5 → 5/5 · 2 FIXED · 0 REGRESSED Root Cause (confirmed by tests)DataBuckets::addRow() used date('W') (ISO week, always Monday-start) for bucket offsets. When start_of_week != 1:
Fix ValidationThe getWeekStartTimestamp() helper correctly:
Environment
|
Contributor
E2E QA: sow=6 (Saturday) — Mar 7-14 Bucket ValidationTest spec: weekly-bucketing-sow6-mar7-validation.spec.ts (4 tests) Side-by-Side Results
Score: 1/4 to 4/4. 3 FIXED. 0 REGRESSED Bucket-by-Bucket Comparison (Test 1)
On development, every bucket is shifted 1 position left due to ISO week misalignment, plus 5 hits from Feb 18 are silently dropped (negative offset). Critical Finding: Boundary Split (Test 4)Seeded: Mar 13 (Fri) = 100 hits, Mar 14 (Sat) = 200 hits
This directly explains the screenshots: development shows 302 in Mar 7-14 because it contains data from the next week (Mar 14-17). PR #235 shows the correct 159 because it only contains Mar 7-13 data. VerdictPR #235 is correct. The getWeekStartTimestamp() fix resolves all 3 failure modes:
|
…dev) Targeted test for the exact screenshot scenario: weekly chart with start_of_week=6 (Saturday), validating Mar 7-14 bucket contains only Sat-Fri data, boundary split between Mar 13 (Fri) and Mar 14 (Sat), and cross-granularity consistency.
Contributor
E2E QA Report: Monthly & Daily Chart Granularity — Cross-Branch ComparisonTest spec: Results
Score: 9/9 → 9/9 · 0 FIXED · 0 REGRESSED · 9 UNCHANGED_PASS AnalysisThe weekly
ConclusionPR #235 is a safe, scoped fix — it corrects weekly bucketing without introducing regressions in daily or monthly chart modes. Combined E2E Summary (all tests across 3 specs)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/weekly-bucketing-sow6-mar7-validation.spec.ts (1)
257-264: Assert the previous bucket index, not only the total.A one-bucket shift in
datasets_prevstill leavessumArr(v1Prev) === 20, so this can pass while the Mar 7, 2026 previous-period bucket is still misaligned. Please check the expected bucket position directly.Possible tightening
const prevSum = sumArr(v1Prev); expect(prevSum, 'Previous period must have data (not zero)').toBeGreaterThan(0); expect(prevSum, 'Previous period sum').toBe(20); + expect(v1Prev[3], 'Previous-period Mar 7-equivalent bucket').toBe(20); + expect(v1Prev[4] ?? 0, 'Adjacent previous-period bucket should stay empty').toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/weekly-bucketing-sow6-mar7-validation.spec.ts` around lines 257 - 264, The test only checks sumArr(v1Prev) but not that the nonzero value sits in the correct bucket, so add an assertion that the specific bucket index for the previous period in v1Prev holds the expected value (instead of or in addition to the total); locate the previous-period array v1Prev (and any dataset name like datasets_prev) and assert v1Prev[expectedIndex] equals the expected bucket value (or that v1Prev[expectedIndex] > 0) using the same index calculation the test uses for current period (or a helper like your bucket index helper), leaving the total-sum assertions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/weekly-bucketing-sow6-mar7-validation.spec.ts`:
- Around line 118-120: The utcMidnight() helper currently forces UTC by
appending 'Z', causing mismatch with DataBuckets::addRow() which uses the
WordPress site timezone for bucketing; either (A) modify test setup to snapshot
and restore the site timezone keys (timezone_string and gmt_offset) in
test.beforeAll() and test.afterAll() alongside start_of_week so the site runs in
UTC for the test, or (B) change utcMidnight() to produce a site-local midnight
(i.e., construct the Date using the local offset instead of appending 'Z') so
the seeded timestamps align with DataBuckets::addRow()'s timezone conversion;
update references to utcMidnight() and the test setup accordingly.
---
Nitpick comments:
In `@tests/e2e/weekly-bucketing-sow6-mar7-validation.spec.ts`:
- Around line 257-264: The test only checks sumArr(v1Prev) but not that the
nonzero value sits in the correct bucket, so add an assertion that the specific
bucket index for the previous period in v1Prev holds the expected value (instead
of or in addition to the total); locate the previous-period array v1Prev (and
any dataset name like datasets_prev) and assert v1Prev[expectedIndex] equals the
expected bucket value (or that v1Prev[expectedIndex] > 0) using the same index
calculation the test uses for current period (or a helper like your bucket index
helper), leaving the total-sum assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84042782-a330-4ed7-b2bd-2a4fb60a0a47
📒 Files selected for processing (1)
tests/e2e/weekly-bucketing-sow6-mar7-validation.spec.ts
Comment on lines
+118
to
+120
| function utcMidnight(dateStr: string): number { | ||
| return Math.floor(new Date(dateStr + 'T00:00:00Z').getTime() / 1000); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search E2E setup for explicit WordPress timezone overrides:"
rg -n -C2 "timezone_string|gmt_offset|UTC|snapshotOption\\('timezone_string'|restoreOption\\('timezone_string'" tests/e2e
echo
echo "Show the timestamp helper in this spec:"
sed -n '118,160p' tests/e2e/weekly-bucketing-sow6-mar7-validation.spec.ts
echo
echo "Show the WordPress-side bucketing normalization:"
fd 'DataBuckets.php' src -x sed -n '173,251p' {}Repository: wp-slimstat/wp-slimstat
Length of output: 10629
Pin the site timezone to UTC or adjust utcMidnight() to use site-local time.
utcMidnight() hard-codes UTC with the 'Z' suffix, but DataBuckets::addRow() converts timestamps to the WordPress site timezone before bucketing weeks. The test setup snapshots start_of_week but does not pin timezone_string or gmt_offset. If the WordPress installation is not running in UTC, the March 13–14, 2026 boundary seed will be interpreted as a different local date, invalidating the week boundary assertion.
Add an explicit timezone snapshot/restore in test.beforeAll() and test.afterAll() (similar to the start_of_week pattern), or adjust the helper to seed local midnights instead of UTC midnights.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/weekly-bucketing-sow6-mar7-validation.spec.ts` around lines 118 -
120, The utcMidnight() helper currently forces UTC by appending 'Z', causing
mismatch with DataBuckets::addRow() which uses the WordPress site timezone for
bucketing; either (A) modify test setup to snapshot and restore the site
timezone keys (timezone_string and gmt_offset) in test.beforeAll() and
test.afterAll() alongside start_of_week so the site runs in UTC for the test, or
(B) change utcMidnight() to produce a site-local midnight (i.e., construct the
Date using the local offset instead of appending 'Z') so the seeded timestamps
align with DataBuckets::addRow()'s timezone conversion; update references to
utcMidnight() and the test setup accordingly.
…ssion Tests daily buckets, monthly buckets, previous periods, month boundaries, and cross-granularity consistency (daily=weekly=monthly) for sow=0,1,6. Both branches pass 9/9 — confirms PR #235 weekly fix is scoped and safe.
Merged
parhumm
added a commit
that referenced
this pull request
Mar 17, 2026
This was referenced Mar 19, 2026
parhumm
added a commit
that referenced
this pull request
Mar 20, 2026
Revert the anonymous nonce bypass — consent is a state-changing operation. A cross-site POST without nonce verification could force-accept consent, enabling PII tracking without genuine user action (GDPR violation). On cached pages, anonymous consent REST calls return 403, but the JS cookie still records consent client-side, and tracking works via the /hit endpoint (PR #235). This is an acceptable trade-off for security. Changes: - ConsentChangeRestController: restore nonce verification for all users - GDPRBannerRestController: restore nonce required + verification - ConsentHandler: restore check_ajax_referer and wp_verify_nonce - wp-slimstat.js: skip X-WP-Nonce header when nonce is empty to avoid WordPress core rest_cookie_check_errors 403 before handler runs - Unit tests: replace inline pattern tests with actual handler invocations - E2E test 5: verify stale nonce causes 403 (correct security behavior) while client-side cookie is still set Refs #240, #241
6 tasks
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
2 participants
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.
The weekly offset calculation was using ISO week numbers (date('W')) which always start on Monday, regardless of WordPress start_of_week setting. This caused data to be placed in the wrong bucket when start_of_week was set to Sunday (0).
Now calculates the start-of-week date for both base and data point timestamps using the WordPress start_of_week setting, ensuring correct bucket placement.
Describe your changes
...
Submission Review Guidelines:
CHANGELOG.md.Type of change
Summary by CodeRabbit
Bug Fixes
Tests