feat: improve admin dashboard performance, widgets and customization - #4284
Conversation
Performance & UX: - Run dashboard init concurrently via Promise.allSettled - Cache remote news/version lookups with stale-on-error fallback - Add chart skeleton loaders and theme-aware bar palette New widgets: - Popular searches widget (most popular search terms, last 30 days) - Content health widget (orphaned and stale FAQ counts) - 7/30/90-day range switcher for the visitor chart Customization: - Per-admin widget layout (reorder + show/hide), persisted in the new faqadmindashboard table - Layout GET/POST/reset API endpoints with CSRF protection - Edit mode with per-widget move/hide controls Closes #3721
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds per-user dashboard layout persistence with an edit-mode UI and per-widget controls, new content-health and popular-searches endpoints and client renderers, chart range controls and theming, PSR-6 caching for remote data, DB schema/migration updates, services wiring, and comprehensive tests. ChangesDashboard Widget Customization and Persistence
Server-side persistence, API and data
Schema, migrations, services and tests
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
phpmyfaq/src/phpMyFAQ/Administration/Session.php (1)
133-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix off-by-one in visit window size.
Current bounds generate
days + 1daily buckets (inclusive loop + fulldayssubtraction).💡 Suggested fix
- $startDate = $endDate - ($days * 86_400); + $startDate = $endDate - (($days - 1) * 86_400);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/src/phpMyFAQ/Administration/Session.php` around lines 133 - 141, The daily-bucket loop currently produces days+1 entries because startDate is computed as $endDate - ($days * 86_400) while the loop uses for ($date = $startDate; $date <= $endDate; $date += 86_400), making the range inclusive; fix by adjusting the start boundary to $startDate = $endDate - (($days - 1) * 86_400) (respecting the min 1 constraint on $days) or alternatively change the loop condition to $date < $endDate + 86_400 to make the intent explicit; update the code around $days, $startDate and the for (...) loop and keep usage of $this->sessionRepository->getSessionTimestamps unchanged.phpmyfaq/admin/assets/src/dashboard.ts (1)
185-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale visits responses from overwriting the latest selected range.
Multiple rapid clicks (7/30/90) can resolve out of order and render stale data last.
Proposed fix (request versioning)
+ let latestVisitsRequest = 0; const getData = async (days: number): Promise<void> => { + const requestId = ++latestVisitsRequest; try { const response = await fetch(`./api/dashboard/visits?days=${days}`, { @@ if (response.status === 200) { const visits: { date: string; number: number }[] = await response.json(); + if (requestId !== latestVisitsRequest) { + return; + } visitorChart.data.labels = []; visitorChart.data.datasets[0].data = [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/admin/assets/src/dashboard.ts` around lines 185 - 223, The getData fetch can return out-of-order and overwrite the chart; implement request versioning by adding a module-level counter or AbortController and checking it inside getData: increment a requestId (or create/abort a stored AbortController) each time the range button click handler triggers before calling getData, capture the current id inside getData, and after awaiting the fetch/response verify the id still matches (or that the request wasn't aborted) before mutating visitorChart.data and calling visitorChart.update; update references to getData, the rangeGroup click handler, and visitorChart accordingly.
🧹 Nitpick comments (4)
phpmyfaq/admin/assets/src/dashboard-layout.test.ts (1)
51-73: ⚡ Quick winAdd a regression case for unordered
configpayloads.This test currently sends
configalready ordered by position, so it won’t catch ordering regressions. Please add a variant with shuffled entries and assert final DOM order byposition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/admin/assets/src/dashboard-layout.test.ts` around lines 51 - 73, The test currently passes an already-ordered config so add a regression case that supplies a shuffled config payload to the fetch mock and asserts DOM order is determined by each item’s position property; specifically, update or add a test that uses handleDashboardLayout() with a fetch mock returning config entries in random order (e.g., keys 'inactive-faqs', 'recent-users', 'content-health' shuffled) and then verify the order of elements selected by document.querySelectorAll('[data-pmf-widget]') matches the ascending position values and that visibility (e.g., the 'd-none' class on the element with data-pmf-widget="inactive-faqs") is still respected.tests/phpMyFAQ/Administration/DashboardLayoutTest.php (1)
24-31: ⚡ Quick winRestore global DB table prefix after each test for isolation.
This test mutates global static state and does not restore it, which can make other tests order-dependent.
💡 Suggested fix
class DashboardLayoutTest extends TestCase { + private string $previousPrefix; + protected function setUp(): void { parent::setUp(); - - Database::setTablePrefix(''); + $this->previousPrefix = Database::getTablePrefix(); + Database::setTablePrefix(''); ... } + + protected function tearDown(): void + { + Database::setTablePrefix($this->previousPrefix); + parent::tearDown(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpMyFAQ/Administration/DashboardLayoutTest.php` around lines 24 - 31, The test sets global static DB table prefix via Database::setTablePrefix('') and never restores it; to fix, save the original prefix at start of the test class (e.g., in setUp store Database::getTablePrefix() into a property like $this->originalPrefix) and restore it in tearDown by calling Database::setTablePrefix($this->originalPrefix); add or update the DashboardLayoutTest::tearDown() method to perform the restore so other tests remain isolated.phpmyfaq/admin/assets/src/dashboard.test.ts (1)
418-418: ⚡ Quick winAdd an interaction test for 7/90-day range switching.
Current coverage asserts only the initial
days=30fetch. Add a click-flow test forbutton[data-pmf-range]to verify endpoint/query updates and active-button state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/admin/assets/src/dashboard.test.ts` at line 418, Add an interaction test after the initial assertion that simulates clicking the range buttons (select elements with attribute button[data-pmf-range]) to verify the component updates fetch calls and active state; specifically, use the existing fetchMock to clear or reset calls, fire a click on the button with data-pmf-range="7", assert fetchMock was called with './api/dashboard/visits?days=7' and that the clicked button has the active class/aria state, then repeat for data-pmf-range="90" asserting './api/dashboard/visits?days=90' and the correct active-button state; ensure to use the same test helpers and DOM queries already present in dashboard.test.ts and reset mocks between interactions.phpmyfaq/admin/assets/src/index.ts (1)
109-119: ⚡ Quick winLog rejected dashboard tasks from
Promise.allSettled.Right now failures are fully swallowed; a small rejection log would preserve debuggability without reintroducing blocking behavior.
Proposed improvement
- await Promise.allSettled([ + const dashboardResults = await Promise.allSettled([ renderVisitorCharts(), renderTopTenCharts(), getLatestVersion(), handleVerificationModal(), fetchRecentNews(), fetchContentHealth(), fetchPopularSearches(), handleDashboardLayout(), ]); + dashboardResults.forEach((result) => { + if (result.status === 'rejected') { + console.error('Dashboard init task failed:', result.reason); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/admin/assets/src/index.ts` around lines 109 - 119, The Promise.allSettled call that runs renderVisitorCharts, renderTopTenCharts, getLatestVersion, handleVerificationModal, fetchRecentNews, fetchContentHealth, fetchPopularSearches, and handleDashboardLayout currently swallows failures; change it to capture the allSettled results into a variable and iterate over them to log any results with status === "rejected" (include the corresponding task name by mapping indexes to the function names above and log the rejection reason via console.error or the existing logger). Ensure you do not rethrow so behavior stays non‑blocking, but include clear context in each log entry (task name and reason).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phpmyfaq/admin/assets/src/dashboard-layout.ts`:
- Around line 66-74: The loop restores widgets using the config array order but
ignores entry.position, so sort the configuration entries by their numeric
position before iterating: call a stable sort on config (e.g.,
config.sort((a,b)=>a.position - b.position)) then iterate, look up widgets via
widgetsByKey, set widget.dataset.pmfHidden = entry.visible ? 'false' : 'true',
and call grid.appendChild(widget) in that sorted order so stored layouts restore
correctly; ensure you handle missing or undefined position values safely (treat
as Infinity or 0 as appropriate).
In `@phpmyfaq/admin/assets/src/dashboard.ts`:
- Around line 216-223: The callback dereferences rangeGroup without a guaranteed
non-null guard under TypeScript strict mode; fix by capturing rangeGroup into a
local constant before the listener (e.g., const rg = rangeGroup) or at the start
of the handler, then check if (!rg) return; and replace
rangeGroup.querySelectorAll(...) with rg.querySelectorAll(...). Update the
listener setup around rangeGroup, the forEach callback on buttons, and the inner
DOM manipulation so all references use the guarded local (rg) to satisfy strict
null-checking.
In `@phpmyfaq/admin/assets/src/index.test.ts`:
- Around line 12-13: Test currently registers mocks for fetchContentHealth and
fetchPopularSearches but never asserts they were invoked; update the test in
index.test.ts to assert the mocks are called (e.g.,
expect(fetchContentHealth).toHaveBeenCalled()/toHaveBeenCalledTimes(1) and same
for fetchPopularSearches) after the dashboard init runs, and add a mock/spy for
handleDashboardLayout (e.g., vi.fn or vi.spyOn) and assert it was invoked as
well so the concurrent init contract is fully covered; reference the mock
identifiers fetchContentHealth, fetchPopularSearches, and handleDashboardLayout
when adding these assertions.
In `@phpmyfaq/src/phpMyFAQ/Administration/DashboardLayout.php`:
- Around line 80-89: The current save() deletes the old row then inserts the new
one, risking data loss if the insert fails; make the replacement atomic by using
a database transaction around the delete+insert (call beginTransaction, perform
the DELETE and INSERT, then commit, rolling back on any failure) or replace the
two-step logic with a single atomic statement such as INSERT ... ON DUPLICATE
KEY UPDATE / REPLACE INTO targeting the faqadmindashboard table so that either
the new config is written or the operation fails without deleting the existing
row; update the code paths that call $database->query and the save() method in
DashboardLayout.php to use the chosen atomic approach and ensure proper error
handling/rollback.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/DashboardController.php`:
- Around line 376-379: The endpoint always returns success after calling
DashboardLayout->reset(...) even when persistence fails; update the controller
to check the outcome of the reset call (or catch exceptions thrown by
DashboardLayout::reset/resetLayout) and return a failure JSON and non-200 status
when it fails. Specifically, call
DashboardLayout->reset($this->currentUser->getUserId()), inspect its
boolean/return value or wrap it in try/catch, and on failure return
$this->json(['success'=>false, 'message' => 'Failed to reset dashboard layout' ,
'error' => $e->getMessage() ?? null], 500) (or appropriate message/status)
instead of always returning success.
In `@tests/phpMyFAQ/Controller/Administration/Api/DashboardControllerTest.php`:
- Around line 461-468: The test testSaveLayoutRejectsInvalidBody currently fails
true invalid-JSON assertion because it omits CSRF and can be rejected for auth;
update the test to include a valid CSRF token in the Request so it isolates body
parsing. Specifically, when creating the Request passed to
DashboardController::saveLayout in testSaveLayoutRejectsInvalidBody(), seed the
same CSRF token/value that createAuthenticatedContainer() expects (e.g., add the
CSRF token to the request attributes/post or headers as the app uses) so the
controller proceeds past CSRF validation and only the invalid JSON body is
tested.
---
Outside diff comments:
In `@phpmyfaq/admin/assets/src/dashboard.ts`:
- Around line 185-223: The getData fetch can return out-of-order and overwrite
the chart; implement request versioning by adding a module-level counter or
AbortController and checking it inside getData: increment a requestId (or
create/abort a stored AbortController) each time the range button click handler
triggers before calling getData, capture the current id inside getData, and
after awaiting the fetch/response verify the id still matches (or that the
request wasn't aborted) before mutating visitorChart.data and calling
visitorChart.update; update references to getData, the rangeGroup click handler,
and visitorChart accordingly.
In `@phpmyfaq/src/phpMyFAQ/Administration/Session.php`:
- Around line 133-141: The daily-bucket loop currently produces days+1 entries
because startDate is computed as $endDate - ($days * 86_400) while the loop uses
for ($date = $startDate; $date <= $endDate; $date += 86_400), making the range
inclusive; fix by adjusting the start boundary to $startDate = $endDate -
(($days - 1) * 86_400) (respecting the min 1 constraint on $days) or
alternatively change the loop condition to $date < $endDate + 86_400 to make the
intent explicit; update the code around $days, $startDate and the for (...) loop
and keep usage of $this->sessionRepository->getSessionTimestamps unchanged.
---
Nitpick comments:
In `@phpmyfaq/admin/assets/src/dashboard-layout.test.ts`:
- Around line 51-73: The test currently passes an already-ordered config so add
a regression case that supplies a shuffled config payload to the fetch mock and
asserts DOM order is determined by each item’s position property; specifically,
update or add a test that uses handleDashboardLayout() with a fetch mock
returning config entries in random order (e.g., keys 'inactive-faqs',
'recent-users', 'content-health' shuffled) and then verify the order of elements
selected by document.querySelectorAll('[data-pmf-widget]') matches the ascending
position values and that visibility (e.g., the 'd-none' class on the element
with data-pmf-widget="inactive-faqs") is still respected.
In `@phpmyfaq/admin/assets/src/dashboard.test.ts`:
- Line 418: Add an interaction test after the initial assertion that simulates
clicking the range buttons (select elements with attribute
button[data-pmf-range]) to verify the component updates fetch calls and active
state; specifically, use the existing fetchMock to clear or reset calls, fire a
click on the button with data-pmf-range="7", assert fetchMock was called with
'./api/dashboard/visits?days=7' and that the clicked button has the active
class/aria state, then repeat for data-pmf-range="90" asserting
'./api/dashboard/visits?days=90' and the correct active-button state; ensure to
use the same test helpers and DOM queries already present in dashboard.test.ts
and reset mocks between interactions.
In `@phpmyfaq/admin/assets/src/index.ts`:
- Around line 109-119: The Promise.allSettled call that runs
renderVisitorCharts, renderTopTenCharts, getLatestVersion,
handleVerificationModal, fetchRecentNews, fetchContentHealth,
fetchPopularSearches, and handleDashboardLayout currently swallows failures;
change it to capture the allSettled results into a variable and iterate over
them to log any results with status === "rejected" (include the corresponding
task name by mapping indexes to the function names above and log the rejection
reason via console.error or the existing logger). Ensure you do not rethrow so
behavior stays non‑blocking, but include clear context in each log entry (task
name and reason).
In `@tests/phpMyFAQ/Administration/DashboardLayoutTest.php`:
- Around line 24-31: The test sets global static DB table prefix via
Database::setTablePrefix('') and never restores it; to fix, save the original
prefix at start of the test class (e.g., in setUp store
Database::getTablePrefix() into a property like $this->originalPrefix) and
restore it in tearDown by calling
Database::setTablePrefix($this->originalPrefix); add or update the
DashboardLayoutTest::tearDown() method to perform the restore so other tests
remain isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b215e45-89d5-4efe-9e88-d4e036a95017
📒 Files selected for processing (30)
phpmyfaq/admin/assets/scss/layout/_dashboard.scssphpmyfaq/admin/assets/src/dashboard-layout.test.tsphpmyfaq/admin/assets/src/dashboard-layout.tsphpmyfaq/admin/assets/src/dashboard.test.tsphpmyfaq/admin/assets/src/dashboard.tsphpmyfaq/admin/assets/src/index.test.tsphpmyfaq/admin/assets/src/index.tsphpmyfaq/assets/templates/admin/dashboard.twigphpmyfaq/src/phpMyFAQ/Administration/DashboardLayout.phpphpmyfaq/src/phpMyFAQ/Administration/Faq.phpphpmyfaq/src/phpMyFAQ/Administration/Session.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/DashboardController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/DashboardController.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.phpphpmyfaq/src/services.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Administration/DashboardLayoutTest.phptests/phpMyFAQ/Cache/CacheFactoryTest.phptests/phpMyFAQ/Controller/Administration/Api/DashboardControllerTest.phptests/phpMyFAQ/Controller/Frontend/CategoryControllerTest.phptests/phpMyFAQ/EventListener/ApiRateLimiterListenerTest.phptests/phpMyFAQ/FaqTest.phptests/phpMyFAQ/Permission/GroupCategoryPermissionRepositoryTest.phptests/phpMyFAQ/Session/TokenTest.phptests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.phptests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.phptests/phpMyFAQ/Setup/Migration/MigrationRegistryTest.php
💤 Files with no reviewable changes (2)
- phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.php
- phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.php
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/admin/assets/src/dashboard.ts (1)
498-527:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear existing news content before appending new list.
fetchRecentNews()appends a new<ul>without clearing prior content, so repeated calls can duplicate items in the widget.Suggested fix
- container.appendChild(list); + container.replaceChildren(list);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/admin/assets/src/dashboard.ts` around lines 498 - 527, The fetchRecentNews() implementation appends a new list each call without clearing prior content, causing duplicate items; before creating/adding the new <ul> (i.e., before list is appended to container in the block handling response.ok), clear the container's existing content (use container.innerHTML = '' or remove all child nodes) so only the latest news list is shown; update the code around container, list, and container.appendChild to perform this clear operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/DashboardController.php`:
- Around line 91-100: The cache handling in DashboardController (the block that
reads $cached = $item->get() and returns $cached['payload']) doesn't verify that
payload is an array; update both occurrences (the block around the first snippet
and the similar block at lines 111-114) to check is_array($cached['payload'])
before returning and return null if it's not an array so the method's ?array
return type is preserved; keep the existing checks for fetchedAt and TTL, then
add a final guard like if (!is_array($cached['payload'])) { return null; }
before returning the payload.
- Around line 205-208: The code in DashboardController sets $endDate from
$request->server->get('REQUEST_TIME') which can be 0 when the server param is
missing; change it to use a safe fallback by computing $endDate = (int)
($request->server->get('REQUEST_TIME') ?? time()) (or check empty and use
time()) before calling $this->adminSession->getVisitsForDays($endDate, $days) so
getVisitsForDays always receives a valid timestamp.
---
Outside diff comments:
In `@phpmyfaq/admin/assets/src/dashboard.ts`:
- Around line 498-527: The fetchRecentNews() implementation appends a new list
each call without clearing prior content, causing duplicate items; before
creating/adding the new <ul> (i.e., before list is appended to container in the
block handling response.ok), clear the container's existing content (use
container.innerHTML = '' or remove all child nodes) so only the latest news list
is shown; update the code around container, list, and container.appendChild to
perform this clear operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00f69405-346e-4b38-9653-f543676236f8
📒 Files selected for processing (11)
phpmyfaq/admin/assets/src/dashboard-layout.test.tsphpmyfaq/admin/assets/src/dashboard-layout.tsphpmyfaq/admin/assets/src/dashboard.test.tsphpmyfaq/admin/assets/src/dashboard.tsphpmyfaq/admin/assets/src/index.test.tsphpmyfaq/admin/assets/src/index.tsphpmyfaq/src/phpMyFAQ/Administration/DashboardLayout.phpphpmyfaq/src/phpMyFAQ/Administration/Session.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/DashboardController.phptests/phpMyFAQ/Administration/DashboardLayoutTest.phptests/phpMyFAQ/Administration/SessionTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- phpmyfaq/admin/assets/src/index.test.ts
- phpmyfaq/src/phpMyFAQ/Administration/Session.php
- phpmyfaq/admin/assets/src/index.ts
Performance & UX:
New widgets:
Customization:
Closes #3721
Summary by CodeRabbit
New Features
Other