fix(auth-logs): record login/logout synchronously and harden filters#28
Merged
Conversation
The Login/Logout listeners implemented ShouldQueue, but production runs QUEUE_CONNECTION=database with no always-on worker, so those audit rows were queued and never written -- only the synchronous failed-login listener persisted. The page therefore showed failed attempts only, and filtering by login/logout returned an empty (apparently blank) result. - Drop ShouldQueue from LogSuccessfulLogin/LogSuccessfulLogout so every event is persisted synchronously even without a queue worker. - Wire the From/To date-range inputs (params were camelCase and silently ignored), reset pagination on filter change, make end_date inclusive of the whole day, and show an explicit empty-state message. - Add Pest coverage forcing queue.default=database, plus admin-sees-all-users and a Vitest render/empty-state guard for the page. The Auth Logs link and route were already admin-only and admins already see all users' events; both are now covered by tests.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99be8441eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Fixes missing successful login/logout audit rows in production (where a database queue is configured but no worker runs) and improves the /auth-logs admin filtering UX so filtered empty results don’t look like a blank page.
Changes:
- Made successful login/logout listeners persist audit rows synchronously (removed queued listeners).
- Fixed auth-log filter request param casing, added date range inputs, reset pagination on filter apply, and added a clear empty state.
- Added regression tests (Pest for database-queue-without-worker behavior + admin visibility, Vitest for empty-state rendering).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Feature/AuthLogTest.php | Adds regression coverage for database queue without worker + admin visibility across users. |
| app/Listeners/LogSuccessfulLogin.php | Removes queuing so login audits are persisted synchronously. |
| app/Listeners/LogSuccessfulLogout.php | Removes queuing so logout audits are persisted synchronously. |
| app/Http/Controllers/Admin/AuthLogController.php | Makes end_date inclusive of the whole day for date-only filters. |
| resources/js/pages/admin/auth-logs/index.tsx | Sends snake_case params, resets pagination, adds date inputs, and shows an empty message. |
| resources/js/pages/admin/auth-logs/tests/index.test.tsx | Ensures the page renders an explicit empty state and still triggers a router visit on apply. |
| docs/LESSON.md | Documents the queued-listener-without-worker pitfall and the UX symptom. |
| CHANGELOG.md | Notes the fixes and added coverage in v1.0.7. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Reset button now submits the cleared values directly instead of reading the async-stale `search` state (Codex P2), so it actually clears date filters; applyFilters accepts an explicit values arg. - Update the hardening-polish e2e and rename the Pest test to match the new empty-state message and the events it actually asserts (Copilot). - Prettier-format the new auth-logs page test (quality job). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LV8PnhgE5NvEQwzvgBEXVS
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
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.
Why
On
/auth-logsthe admin only ever saw failed logins, and filtering bylogin/logoutshowed an empty (apparently blank) page.Root cause:
LogSuccessfulLoginandLogSuccessfulLogoutimplementedShouldQueue, but production runsQUEUE_CONNECTION=databasewith no always-on worker. Those audit rows were queued to thejobstable and never processed — onlyLogFailedLogin(not queued) ran synchronously and got written. It stayed green in CI becausephpunit.xmlsetsQUEUE_CONNECTION=sync, so the queued listeners ran inline.Reproduced with a Pest run forcing
QUEUE_CONNECTION=database: theloginrow was missing (table empty); after the fix all three events are written.Changes
ShouldQueuefromLogSuccessfulLogin/LogSuccessfulLogout. A single lightweight audit insert must survive even without a queue worker.end_datebound inclusive of the whole day, and added an explicit empty-state message instead of a bare table.queue.default=database(regression guard a sync-queue test can't catch), a test that an admin sees a logout event belonging to another user, and a Vitest render/empty-state guard for the page.Already correct (now covered by tests)
/auth-logsroute are admin-only (auth.isAdmin+role:adminmiddleware).user_idscope).Verification
Green locally: Pint, PHPStan (max), Pest (225 passing / 1 skipped), Vitest (16),
tsc, ESLint. The prod asset build and Playwright e2e rely on CI here (the local build fetches a network-blocked remote font and no browsers are installed in the dev sandbox).CHANGELOG updated to v1.0.7; lesson recorded in
docs/LESSON.md([audit/queued-listener-needs-a-worker]).https://claude.ai/code/session_01LV8PnhgE5NvEQwzvgBEXVS
Generated by Claude Code