fix(adminbar): auto-refresh all modal stats and chart every minute (#224)#225
fix(adminbar): auto-refresh all modal stats and chart every minute (#224)#225parhumm merged 8 commits intodevelopmentfrom
Conversation
, #224) Previously only Online Users refreshed. Now Sessions Today, Views Today, Referrals Today, and the 30-bar chart also update via a single AJAX call (slimstat_get_adminbar_stats) with 60s transient caching for today stats. Chart data reuses LiveAnalyticsReport::get_users_chart_data() to preserve #221 consistency.
- Extract query_online_count() and check_ajax_view_capability() helpers - Combine 6 today-stats queries into 2 using conditional aggregates - DRY up stat card update blocks with loop in adminbar-realtime.js - Expose slimstatAnimateElement globally, remove inline animation in admin.js - Cache i18n at function entry instead of checking 4 times
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReplaces per-element online polling with a unified admin-bar stats flow: new AJAX action Changes
Sequence DiagramsequenceDiagram
participant Client as Frontend (Client)
participant AJAX as WP Admin AJAX
participant Server as get_adminbar_stats Handler
participant DOM as Admin Bar DOM
Client->>Client: minute poll timer / slimstat:minute_pulse
Client->>AJAX: POST /admin-ajax.php action=slimstat_get_adminbar_stats, nonce
AJAX->>Server: invoke get_adminbar_stats()
Server->>Server: check_ajax_view_capability(), query_online_count(), gather stats & chart
Server-->>Client: { success:true, data:{ online, sessions, views, referrals, chart? } }
Client->>Client: window.slimstatUpdateAdminBar(data)
Client->>DOM: update counts, cards, chart bars, peaks, tooltips
Client->>Client: window.slimstatAnimateElement() for value transitions
DOM-->>Client: admin bar updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
E2E Test Results — All 6 PassedEnvironment: Local by Flywheel, WordPress 6.8, PHP 8.2, Chromium (Playwright), macOS Why These Tests Are TrustworthyEach test validates a different layer of the fix. If any one layer breaks, a specific test fails — there's no single point of failure that all 6 share. Test 1: AJAX endpoint returns valid dataWhat it proves: The new Why it's reliable: It authenticates as a real admin user (via Playwright Test 2: All required element IDs exist in DOMWhat it proves: The 8 HTML element IDs that Why it's reliable: This is the contract between PHP (producer) and JS (consumer). Before this fix, 6 of these IDs didn't exist — the JS had nothing to update. If someone removes an Test 3: Localized script data has i18n + is_proWhat it proves: Why it's reliable: The JS refresh function uses Test 4: Global update functions are exposedWhat it proves: Why it's reliable: Test 5: AJAX response matches server-rendered valuesWhat it proves: The AJAX endpoint returns data consistent with what PHP rendered into the HTML on page load. Same SQL, same caching layer — no drift between initial render and subsequent refreshes. Why it's reliable: Compares server-rendered DOM text against AJAX response within seconds. Also validates that Test 6: No PHP errors on the admin pageWhat it proves: The admin dashboard loads with HTTP 200 and no Why it's reliable: Adding new PHP code (the AJAX handler, extracted helpers, localized script data) can trigger autoload failures, undefined method calls, or syntax errors. This is a smoke test that catches any PHP-level breakage introduced by our changes. It runs against the real WordPress admin — not a stub. What these tests do NOT cover (manual verification needed)
k6 performance testA separate k6 load test ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/assets/js/adminbar-realtime.js`:
- Around line 152-160: Don't short-circuit the adminbar fallback: remove the
early "return" inside the typeof SlimStatAdmin check so the fallback poller
remains active until a real minute pulse is observed; specifically, in
admin/assets/js/adminbar-realtime.js replace the early return in the block that
checks "typeof SlimStatAdmin !== 'undefined'" with logic to start or keep the
fallback (call checkMinutePulse()) unless SlimStatAdmin explicitly exposes and
signals an active countdown/pulse, so the admin bar
(window.slimstatUpdateAdminBar()) still refreshes on pages that load
SlimStatAdmin without its shared countdown.
- Around line 80-83: The current calculation forces a minimum heightPct of 3
which turns count === 0 into a visible bar; update the logic around
chartData/count/maxVal so that when count === 0 you set bar.style.height = "0%"
and only apply Math.max(Math.round((count / maxVal) * 100), 3) for positive
counts (i.e., use count > 0 as the guard), referencing the variables count,
maxVal, heightPct and the assignment to bar.style.height to locate where to
change it.
In `@admin/index.php`:
- Around line 1772-1776: The code uses server-local mktime() to compute
$today_start/$yesterday_start/$yesterday_end which mismatches WP site timezone;
replace that with site-timezone based timestamps using current_time('timestamp')
as the base (e.g. $today_start = strtotime('today', current_time('timestamp'))),
compute yesterday using DAY_IN_SECONDS (e.g. $yesterday_start = $today_start -
DAY_IN_SECONDS; $yesterday_end = $today_start - 1), and keep the existing
$site_host/ $referer_like logic unchanged so both rendering and counts use the
same site timezone boundaries.
- Around line 1766-1816: Transient TTL is fixed at 60s which can leave stale
data until the next minute boundary; change the set_transient call for
$transient_key so TTL is aligned to the next minute boundary (compute seconds
until next minute using current time, e.g., 60 - (time() % 60)) instead of
hardcoding 60, mirroring the alignment logic used by
LiveAnalyticsReport::get_users_chart_data(), and ensure TTL is at least 1 second
before calling set_transient($transient_key, $today_stats, ...).
- Around line 1684-1686: The permission-denied branch uses wp_send_json_error
which returns HTTP 200; change it to return a translated 403 response by passing
a 403 status code and a localized message: replace the current
wp_send_json_error(['message' => 'Insufficient permissions']) call with
wp_send_json_error([...translated message using __() or esc_html__()], 403) and
ensure the message uses the correct text domain (wp-slimstat or
wp-slimstat-pro); keep the capability check using
current_user_can($minimum_capability) and ensure the plugin’s text domain is
loaded earlier so translations are available.
In `@tests/e2e/adminbar-realtime-refresh.spec.ts`:
- Around line 151-179: The test captures serverValues (via getText and IDs
'slimstat-adminbar-online-count' and 'slimstat-adminbar-sessions-count') but
never asserts them against the AJAX response, so DOM/AJAX consistency is
unverified; update the test to parse serverValues.sessions and/or
serverValues.online into numeric counts and assert they match (or are within an
acceptable delta) the AJAX-returned data.sessions.count and data.online.count
from the slimstat_get_adminbar_stats response (use the previously-read nonce via
(window as any).SlimStatAdminBar.security and the response json.data fields) to
enforce the intended consistency check.
In `@tests/perf/adminbar-refresh-perf.js`:
- Around line 66-73: The nonce extraction silently fails and sets loggedIn=true
without validating the value; update the admin page parsing to locate and parse
the SlimStatAdminBar object (e.g., from adminRes.body), extract the "security"
field explicitly, validate the nonce length/format (hex of expected length), and
if extraction/validation fails cause the VU iteration to fail (throw or mark
error) instead of setting loggedIn and skipping the slimstat_get_adminbar_stats
call; change references around nonce, SlimStatAdminBar, adminRes, loggedIn and
the code path that calls slimstat_get_adminbar_stats to rely on this validated
nonce.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a77f46f-86c6-4634-94a8-195f4f05d1aa
📒 Files selected for processing (5)
admin/assets/js/admin.jsadmin/assets/js/adminbar-realtime.jsadmin/index.phptests/e2e/adminbar-realtime-refresh.spec.tstests/perf/adminbar-refresh-perf.js
…d nonce format - Fix nonce extraction: use `var nonce = 'xxx'` pattern (not `nonce: 'xxx'`) - Skip data comparison for free users (admin bar shows decorative placeholder data) - Add Pro-specific test: verify chart uses LiveAnalyticsReport, not placeholder - Add Free-specific test: verify decorative chart has valid structure
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/e2e/adminbar-chart-consistency.spec.ts (3)
24-24: Prefer condition-based waits over hardcoded timeouts.
page.waitForTimeout(3000)is repeated in multiple tests (lines 24, 139, 170). This pattern can cause flakiness in CI and unnecessarily slows down test execution. Consider waiting for a specific condition instead.♻️ Suggested improvement
- await page.waitForTimeout(3000); + // Wait for admin bar chart to be populated + await page.waitForSelector('.slimstat-adminbar__chart-bar[data-count]', { state: 'attached' });Or if you need to ensure the
SlimStatAdminBarobject is loaded:await page.waitForFunction(() => typeof (window as any).SlimStatAdminBar !== 'undefined');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/adminbar-chart-consistency.spec.ts` at line 24, Replace the hardcoded sleep calls using page.waitForTimeout with a condition-based wait to avoid flakiness: locate the occurrences of page.waitForTimeout(3000) in the tests (e.g., in adminbar-chart-consistency.spec.ts) and change them to wait for a specific condition such as the SlimStatAdminBar object being defined by using page.waitForFunction to check typeof (window as any).SlimStatAdminBar !== 'undefined' (or wait for the specific DOM selector or chart element to be visible if more appropriate); ensure all repeated occurrences (the three places noted) are updated to use the condition-based wait.
27-38: Consider extracting Pro detection to a reusable helper.The Pro detection logic and conditional skip pattern is duplicated across 4 test cases. Extracting this to a helper would improve maintainability.
♻️ Example helper extraction
Add to
tests/e2e/helpers/:// helpers/pro-detection.ts import { Page, TestInfo } from '@playwright/test'; export async function checkProStatus(page: Page): Promise<boolean> { return page.evaluate(() => { const bar = (window as any).SlimStatAdminBar; return bar?.is_pro === true || bar?.is_pro === '1'; }); } export async function skipIfNotPro(page: Page, testInfo: TestInfo): Promise<boolean> { const isPro = await checkProStatus(page); if (!isPro) { testInfo.skip(true, 'Pro not active — test requires Pro features'); } return isPro; }Then in tests:
const isPro = await skipIfNotPro(page, testInfo); if (!isPro) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/adminbar-chart-consistency.spec.ts` around lines 27 - 38, Duplicate Pro-detection logic in the test should be extracted to a reusable helper: create functions like checkProStatus(page: Page) that evaluates window.SlimStatAdminBar?.is_pro and skipIfNotPro(page: Page, testInfo: TestInfo) that calls checkProStatus and calls testInfo.skip(...) when not pro; then replace the inline page.evaluate block and test.skip usage in adminbar-chart-consistency.spec.ts (and the three other tests) with a call to skipIfNotPro(page, testInfo) and return early if it reports not-pro. Ensure helper names match checkProStatus and skipIfNotPro so callers are easy to find and import.
159-163: Document the source of the placeholder array.This hardcoded array must match the placeholder data defined in the PHP backend. Consider adding a comment indicating where the source of truth is located to help maintainers keep them in sync.
+ // Source: admin/view/adminbar.php or wherever the placeholder is defined + // Keep in sync with the decorative chart data for non-Pro users const fakeData = [3, 5, 4, 7, 6, 8, 5, 9, 7, 6, 8, 10, 7, 5, 6, 8, 9, 7, 6, 5, 8, 10, 9, 7, 6, 8, 5, 7, 6, 8];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/adminbar-chart-consistency.spec.ts` around lines 159 - 163, The hardcoded fakeData array in the test (variable fakeData compared against adminBarData) is a mirror of the PHP backend placeholder; add a one-line comment immediately above the fakeData declaration that cites the exact source of truth in the backend (the PHP constant/function/class name that defines the placeholder array and the file or class where it lives) and note that the test must be updated if that backend placeholder changes so maintainers know to keep them in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/adminbar-chart-consistency.spec.ts`:
- Line 24: Replace the hardcoded sleep calls using page.waitForTimeout with a
condition-based wait to avoid flakiness: locate the occurrences of
page.waitForTimeout(3000) in the tests (e.g., in
adminbar-chart-consistency.spec.ts) and change them to wait for a specific
condition such as the SlimStatAdminBar object being defined by using
page.waitForFunction to check typeof (window as any).SlimStatAdminBar !==
'undefined' (or wait for the specific DOM selector or chart element to be
visible if more appropriate); ensure all repeated occurrences (the three places
noted) are updated to use the condition-based wait.
- Around line 27-38: Duplicate Pro-detection logic in the test should be
extracted to a reusable helper: create functions like checkProStatus(page: Page)
that evaluates window.SlimStatAdminBar?.is_pro and skipIfNotPro(page: Page,
testInfo: TestInfo) that calls checkProStatus and calls testInfo.skip(...) when
not pro; then replace the inline page.evaluate block and test.skip usage in
adminbar-chart-consistency.spec.ts (and the three other tests) with a call to
skipIfNotPro(page, testInfo) and return early if it reports not-pro. Ensure
helper names match checkProStatus and skipIfNotPro so callers are easy to find
and import.
- Around line 159-163: The hardcoded fakeData array in the test (variable
fakeData compared against adminBarData) is a mirror of the PHP backend
placeholder; add a one-line comment immediately above the fakeData declaration
that cites the exact source of truth in the backend (the PHP
constant/function/class name that defines the placeholder array and the file or
class where it lives) and note that the test must be updated if that backend
placeholder changes so maintainers know to keep them in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 499f5c7f-a226-43c1-b0cb-a6c506e6407d
📒 Files selected for processing (1)
tests/e2e/adminbar-chart-consistency.spec.ts
Issue #221 Verification — Admin Bar Chart ConsistencyRan the What was verified:
Previous issue (now fixed):The test was previously failing due to a nonce extraction bug in the test itself (regex expected |
…ght, HTTP status
- Remove early return in adminbar-realtime.js that suppressed polling on admin
pages without .refresh-timer; now self-polls everywhere, skips if admin.js
pulse already handled the minute
- Fix zero-count chart bars showing 3% height — use 0% for count=0 (JS + PHP)
- Fix mktime() using server timezone instead of WP site timezone for today/
yesterday boundaries — use current_time('timestamp') + DAY_IN_SECONDS
- Align transient TTL to next minute boundary instead of fixed 60s
- Return HTTP 403 (not 200) for permission-denied AJAX responses with
translated error message
- E2E test: actually compare server-rendered DOM values against AJAX response
(was capturing but not asserting)
- k6 test: fail properly when nonce extraction fails instead of silently
skipping all iterations
… fix TTL clock
- Remove dead checkMinutePulse() function — was duplicated inline
- Merge pulseHandledMinute into lastTriggerMinute (pulse listener sets it
directly, eliminating redundant state)
- Fix TTL alignment to use current_time('timestamp') consistently with
today_start computation
- E2E test: fail explicitly on empty DOM values instead of masking with || 0
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin/index.php (1)
1123-1125:⚠️ Potential issue | 🟡 MinorServer-rendered stats use server timezone while AJAX uses WordPress timezone.
The initial render at lines 1123-1125 uses
mktime(0, 0, 0)which relies on PHP's server timezone, while the AJAX handler at lines 1772-1774 correctly usescurrent_time('timestamp')with WordPress timezone. This mismatch can cause brief inconsistencies between the initial render and the first AJAX refresh around midnight in sites where WordPress timezone differs from server timezone.Consider aligning the initial render to use site-timezone boundaries:
🔧 Suggested fix
- $today_start = mktime(0, 0, 0); - $yesterday_start = $today_start - 86400; - $yesterday_end = $today_start - 1; + $today_start = strtotime('today', current_time('timestamp')); + $yesterday_start = $today_start - DAY_IN_SECONDS; + $yesterday_end = $today_start - 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/index.php` around lines 1123 - 1125, The initial render uses mktime(0,0,0) (server timezone) causing a mismatch with the AJAX handler that uses current_time('timestamp') (WP timezone); replace the mktime-based midnight with a site-timezone midnight computed from current_time — e.g. set $today_start = strtotime( date_i18n('Y-m-d', current_time('timestamp')) ); then recompute $yesterday_start = $today_start - 86400 and $yesterday_end = $today_start - 1 so $today_start/$yesterday_start/$yesterday_end use the same WP timezone as the AJAX handler.
🧹 Nitpick comments (1)
admin/assets/js/adminbar-realtime.js (1)
160-177: Potential duplicate AJAX requests at minute boundary despite guard.The guard mechanism sets
pulseHandledMinutewhenslimstat:minute_pulsefires (lines 161-163), but there's a timing edge case: if the 200ms interval tick at lines 166-177 fires just before the event listener runs in the same JavaScript task queue, bothfetchAdminBarStats()calls could be queued simultaneously.However, since JavaScript is single-threaded and event listeners fire synchronously when the event is dispatched, the
slimstat:minute_pulselistener will execute and setpulseHandledMinutebefore the next interval tick has a chance to check it. The current implementation should work correctly in practice.Consider adding a small debounce or request-in-flight flag if duplicate requests are observed in production, but this is not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/assets/js/adminbar-realtime.js` around lines 160 - 177, The minute-boundary race can still enqueue duplicate fetches; add a simple in-flight guard: introduce a boolean (e.g., adminBarStatsInFlight) and check it in the interval callback (and in any other caller) before calling fetchAdminBarStats, set it true immediately before starting the AJAX call inside fetchAdminBarStats (or just before invoking it) and clear it in the success/error/finally handlers so concurrent ticks or event listeners cannot start a second request while one is active; reference pulseHandledMinute, slimstat:minute_pulse, lastTriggerMinute, setInterval and fetchAdminBarStats to locate where to add the check and the flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@admin/index.php`:
- Around line 1123-1125: The initial render uses mktime(0,0,0) (server timezone)
causing a mismatch with the AJAX handler that uses current_time('timestamp') (WP
timezone); replace the mktime-based midnight with a site-timezone midnight
computed from current_time — e.g. set $today_start = strtotime(
date_i18n('Y-m-d', current_time('timestamp')) ); then recompute $yesterday_start
= $today_start - 86400 and $yesterday_end = $today_start - 1 so
$today_start/$yesterday_start/$yesterday_end use the same WP timezone as the
AJAX handler.
---
Nitpick comments:
In `@admin/assets/js/adminbar-realtime.js`:
- Around line 160-177: The minute-boundary race can still enqueue duplicate
fetches; add a simple in-flight guard: introduce a boolean (e.g.,
adminBarStatsInFlight) and check it in the interval callback (and in any other
caller) before calling fetchAdminBarStats, set it true immediately before
starting the AJAX call inside fetchAdminBarStats (or just before invoking it)
and clear it in the success/error/finally handlers so concurrent ticks or event
listeners cannot start a second request while one is active; reference
pulseHandledMinute, slimstat:minute_pulse, lastTriggerMinute, setInterval and
fetchAdminBarStats to locate where to add the check and the flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d940aeb-706b-41d8-bcb0-f64dc83ad27c
📒 Files selected for processing (4)
admin/assets/js/adminbar-realtime.jsadmin/index.phptests/e2e/adminbar-realtime-refresh.spec.tstests/perf/adminbar-refresh-perf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/perf/adminbar-refresh-perf.js
…d try/catch for LiveAnalyticsReport - Replace tooltip.innerHTML with textContent + createElement (XSS defense-in-depth) - Wrap LiveAnalyticsReport instantiation in try/catch for graceful degradation
1 participant
Summary
slimstat_get_adminbar_statsreturns all admin bar data (online, sessions, views, referrals, chart) in a single request with 60s transient cachingLiveAnalyticsReport::get_users_chart_data()transient cache — ~1 fresh DB query per minute in steady stateLiveAnalyticsReportmethod used by Live Analytics pageCloses #224
Duplicate of #223 (closed)
Changes
admin/index.phpget_adminbar_stats()AJAX handler, extractedquery_online_count()+check_ajax_view_capability()helpers, added element IDs to stat cards, extendedwp_localize_scriptwithis_pro+i18nadmin/assets/js/adminbar-realtime.jswindow.slimstatUpdateAdminBar()admin/assets/js/admin.jsslimstat:minute_pulsenow calls new endpoint, shares update function with adminbar-realtime.jstests/e2e/adminbar-realtime-refresh.spec.tstests/perf/adminbar-refresh-perf.jsTest plan
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests