perf(push): move web-push out of the request path by nash87 · Pull Request #607 · nash87/parkhub-php

nash87 · 2026-08-19T23:14:42Z

The comment says non-blocking. It isn't.

static::created(function (Notification $notification) {
    // Fire-and-forget push notification (non-blocking)
    PushNotificationService::sendToUser(...);
});

PushNotificationService ends in foreach ($webPush->flush() as $report), and minishlink/web-push's flush() is:

yield $promise->wait();   // WebPush.php:152

A blocking round-trip to every registered endpoint, with a 30-second default timeout (WebPush.php:61) and no override passed.

Why it matters here

There are ten Notification::create call sites in request handlers — BookingSwapController ×3, AbsenceApprovalController ×2, RescheduleController, and others.

BookingController::destroy alone creates up to three in a loop via notifyWaitlist(). So a single cancellation could pin a PHP-FPM worker for 3 × subscriptions × 30s.

The project ships deploy-shared-hosting.sh. On a shared-hosting worker pool that is a self-inflicted denial of service — and it fires on the cancellation path, which is exactly when several users are likely to be waiting.

Fix

The hook dispatches SendPushNotificationJob, following the pattern already in the repo (NotifyLotClosureJob): ShouldQueue, three tries, backoff, and a failed() handler that logs rather than rethrows.

That last part deliberately preserves what the original swallowed catch (\Throwable) was reaching for — a push must never break the surrounding work — while moving it somewhere that doesn't hold a request open.

Also passes an explicit five-second per-endpoint timeout. Even from a queue worker, 30s is far longer than a push is worth waiting for: a browser push service that hasn't answered in five seconds isn't going to.

Verification

3 tests, including a burst case (several notifications in one request → one job each, no blocking calls) since that is the shape that pins a worker.

Full PHP suite 1892 passed, 0 failures; pint pass; phpstan no errors.

🤖 Generated with Claude Code

https://claude.ai/code/session_013pGb5Hp5WY9t8Tv7WbE81P

…sword fallback - DatabaseSeeder now delegates to ProductionSimulationSeeder when DEMO_MODE=true so `php artisan migrate:fresh --seed --force` yields the full demo fleet (10 lots, 200 users, 3.5k bookings) instead of a single factory user. Without this, every E2E suite that asserts "lots exist" or "admin can see data" failed after a fresh seed. - Admin seed now tolerates an empty PARKHUB_ADMIN_PASSWORD env var. env() returns the empty string (not the default) when the key is declared but blank in .env, so `env('PARKHUB_ADMIN_PASSWORD', 'demo')` was seeding an unusable bcrypt hash of "". Normalise to the documented 'demo' default.

…oken refresh bookkeeping The earlier networkidle→domcontentloaded migration (6a67609) made every spec race React's initial paint: the SPA shell body is "Loading ParkHub" for 200–800 ms before hydration commits the real DOM, and assertions that scan `body.textContent` or call axe-core against that placeholder either flake (context destroyed mid-navigation) or trip fake critical violations. Changes: - helpers.ts: loginViaUi now recovers from the react-hook-form "Required" race (register() not yet wired when the first submit fires) by re-filling both fields with blur-on-tab and clicking again. - a11y.spec.ts: wait for networkidle + URL stability + non-splash body text before running axe. Previously axe could fire during a client-side redirect from / → /login, destroying the frame context. - concurrent-users.spec.ts: same hydration wait for both contexts before comparing rendered text. - offline-reconnect.spec.ts: wait for body.textContent.length > 50 past the splash on reconnect. - security-flows.spec.ts: admin-routes-blocked test now polls for either a URL change to /login|/welcome or an access-denied body string, with a 10s budget — the previous `domcontentloaded` read happened before React's AuthContext redirect committed. - visual.spec.ts: hard-wait past the "Loading ParkHub" splash before the screenshot, so baselines capture the real surface and reruns don't diff against the placeholder. - full-workflow.spec.ts: * refresh-token test now captures the new bearer Sanctum returns and shares it back into the describe-scoped `token`, so subsequent tests don't hit 401 with the revoked PAT. * profile test reorders tryEndpoints() so PHP's working /api/v1/users/me is tried before the Rust-only /api/v1/auth/me (which 404s on this backend and previously also surfaced bookkeeping-stale 401s). * theme-switcher-FAB test uses loginViaUi so it inherits the "Required" recovery path. - visual snapshots regenerated against the fresh ProductionSimulationSeeder data.

Screenshot baselines re-captured locally after the seeder now produces a deterministic 10-lot / 200-user fleet via DEMO_MODE=true.

axe-core flagged two critical "Form elements must have labels" violations on the SetupWizard Company Info step: - input[type=file] data-testid=input-logo had no label association - select[data-testid=select-timezone] had no label association Add htmlFor↔id pairs plus belt-and-braces aria-label attributes on both so screen readers announce them correctly. No behavioural change.

Parity with parkhub-rust. 13 new surfaces: register, forgot-password, welcome, credits, favorites, absences, notifications, calendar, profile, admin-settings, admin-users, admin-lots, admin-analytics. 20 surfaces × 2 viewports × 2 themes = 80 baselines (from 28). Regenerated against PHP server + ProductionSimulationSeeder. The frontend render differs from parkhub-rust only in auth-flow cookie plumbing (Sanctum vs JWT) so Rust and PHP baselines are not byte- identical — each repo keeps its own set against its own backend.

`Notification::booted()` called `PushNotificationService::sendToUser` straight from the model's `created` hook, under a comment reading "fire-and-forget push notification (non-blocking)". It is not non-blocking. `PushNotificationService` ends in `foreach ($webPush->flush() as $report)`, and minishlink/web-push's `flush()` is `yield $promise->wait()` — a blocking round-trip to every registered endpoint, with a 30-second default timeout and no override passed. There are ten `Notification::create` call sites in request handlers (`BookingSwapController` x3, `AbsenceApprovalController` x2, `RescheduleController`, and others). `BookingController::destroy` alone creates up to three in a loop via `notifyWaitlist()`, so one cancellation could pin a PHP-FPM worker for 3 x subscriptions x 30s. The project ships `deploy-shared-hosting.sh`; on a shared-hosting worker pool that is a self-inflicted denial of service. The hook now dispatches `SendPushNotificationJob`, following the pattern already used by `NotifyLotClosureJob` — `ShouldQueue`, three tries, backoff, and a `failed()` handler that logs rather than rethrows. That last part preserves what the original swallowed `catch (\Throwable)` was reaching for — a push must never break the surrounding work — while moving it somewhere it does not hold a request open. Also passes an explicit five-second per-endpoint timeout. Even from a queue worker, 30 seconds is far longer than a push is worth waiting for: a browser push service that has not answered in five seconds is not going to.

Copilot

Pull request overview

This PR moves web-push delivery off the Notification model’s created hook execution path by dispatching a queued job, reducing the risk of request-time blocking on minishlink/web-push network flushes. It also hardens browser E2E stability by adding SPA-hydration waits and improves form accessibility in the setup wizard.

Changes:

Replace synchronous push sending in Notification::booted() with a queued SendPushNotificationJob, and cap per-endpoint push timeout in PushNotificationService.
Add PHP feature tests asserting notification creation queues push jobs (including burst behavior).
Improve Playwright E2E reliability (SPA hydration/redirect races) and expand visual regression surface coverage; add htmlFor/id wiring + labels in setup UI.

Reviewed changes

Copilot reviewed 14 out of 94 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`app/Models/Notification.php`	Dispatches push delivery via a queued job from the `created` hook.
`app/Jobs/SendPushNotificationJob.php`	New queue job that calls `PushNotificationService` and logs on final failure.
`app/Services/PushNotificationService.php`	Sets an explicit (shorter) per-endpoint timeout for web-push delivery.
`tests/Feature/PushNotificationDispatchTest.php`	Adds tests asserting push delivery is queued (including burst behavior).
`database/seeders/DatabaseSeeder.php`	Adds DEMO_MODE-driven seeding path for production simulation data.
`database/seeders/ProductionSimulationSeeder.php`	Normalizes empty admin password env var to a usable default during sim seeding.
`parkhub-web/src/views/SetupWizard.tsx`	Improves label/input associations and adds explicit accessible labeling.
`e2e/helpers.ts`	Hardens UI login helper against a react-hook-form “Required” race.
`e2e/full-workflow.spec.ts`	Fixes token refresh flow and reuses the shared login helper.
`e2e/security-flows.spec.ts`	Waits for SPA redirect/deny state before asserting access-control outcomes.
`e2e/offline-reconnect.spec.ts`	Waits for hydration past splash screen to reduce test flakiness.
`e2e/concurrent-users.spec.ts`	Adds SPA “ready” waits to avoid comparing placeholder DOMs.
`e2e/a11y.spec.ts`	Reduces axe flake by waiting for route stability/hydration.
`e2e/visual.spec.ts`	Expands the visual snapshot coverage to more public/user/admin pages.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+            // Queue the push rather than performing it here. This hook runs
+            // inside whatever request created the notification, and the send
+            // is a blocking round-trip to every registered endpoint
+            // (web-push's `flush()` is `yield $promise->wait()`). The comment
+            // this replaces called it "non-blocking"; it never was.
+            SendPushNotificationJob::dispatch(
+                $notification->user_id,
+                $notification->title ?? 'ParkHub',
+                $notification->message ?? '',
+            );


+ * Deliver a web-push notification outside the request.
+ *
+ * `PushNotificationService` ends in `foreach ($webPush->flush() as $report)`,
+ * and minishlink/web-push's `flush()` is `yield $promise->wait()` — a
+ * blocking round-trip to every registered endpoint. Called from a model


Florian and others added 6 commits April 18, 2026 13:35

test(visual): refresh baselines against fresh ProductionSimulationSeeder

6eceb18

Screenshot baselines re-captured locally after the seeder now produces a deterministic 10-lot / 200-user fleet via DEMO_MODE=true.

Copilot AI lite review requested due to automatic review settings August 19, 2026 23:14

Copilot started reviewing on behalf of nash87 August 19, 2026 23:15 View session

Copilot AI reviewed Aug 19, 2026

View reviewed changes

Navigation Menu

Repository navigation

perf(push): move web-push out of the request path - #607

perf(push): move web-push out of the request path#607
nash87 wants to merge 6 commits into
mainfrom
fix/push-notifications-queued

nash87 commented Aug 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

nash87 commented Aug 19, 2026

The comment says non-blocking. It isn't.

Why it matters here

Fix

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants