Repository navigation
feat(w7): PR previews, test utilities + coverage gate, shared states, notification centre (#104 #105 #106 #107) - #198
Conversation
|
@EmeditWeb Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Malware scan: passed, with notes for the reviewerNothing malicious was detected in
|
… sweep The malware scan on PR DocForum#198 flagged `pull_request_target` in pr-preview-cleanup.yml as high-severity (it hands fork PRs a write token). Cleanup now runs as an hourly `schedule` sweep from main instead of on `pull_request_target: [closed]` — no fork code ever reaches a write token, and the only PR-derived value read is a number from a gh-pages directory name. Docs updated to match. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
@EmeditWeb Thanks, this is a big piece of work, and the test utilities, coverage gate and notification centre look good. I reviewed the preview workflows by hand, and I can't merge them as written, for security reasons:
Could you split this PR? The test utilities, coverage gate, shared states and notification centre (#105–#107) can merge on their own once CI is green. For previews (#104), a safe version would build the preview and upload it as a downloadable artifact only, with a PR comment linking to the artifact, and never publish it to Pages. Alternatively, it could publish only for PRs a maintainer has labelled, and to a separate origin that isn't |
|
@EmeditWeb This PR now has merge conflicts with git fetch upstream # upstream = https://github.com/DocForum/docforum-web.git
git rebase upstream/main
# resolve the conflicts, re-run the tests, then:
git push --force-with-leaseThe usual culprits are shared docs like Automated message. It won't repeat unless the PR conflicts again after your next push. |
… notification centre (DocForum#104 DocForum#105 DocForum#106 DocForum#107) - DocForum#104 fork-safe per-PR mock-API previews (pull_request build, workflow_run publish, cleanup on close) plus check:preview so a preview can never reach the real backend; main.tsx dynamic mock import moved into boot() because the top-level await broke the production-target build previews use. - DocForum#105 renderWithProviders, typed factories and one shared MSW server, with a v8 coverage gate on src/features/ + src/services/ enforced in CI. - DocForum#106 src/components/states/ (Skeleton, EmptyState, InlineError, PageError, QueryState) applied across the query-driven views. - DocForum#107 notification bell, /notifications inbox, per-kind deep links, mark-one/ mark-all read, SSE via single-use stream ticket and cross-tab sync (assumed API until docforum-core#93). 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
… sweep The malware scan on PR DocForum#198 flagged `pull_request_target` in pr-preview-cleanup.yml as high-severity (it hands fork PRs a write token). Cleanup now runs as an hourly `schedule` sweep from main instead of on `pull_request_target: [closed]` — no fork code ever reaches a write token, and the only PR-derived value read is a number from a gh-pages directory name. Docs updated to match. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
The malware scanner matches the literal string in prose, so the comment explaining why the trigger is avoided was itself flagged. Reword to describe the risk without naming the trigger. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
The preview build now uploads dist/ as a private artifact and a separate workflow_run job comments the artifact link. It never publishes PR-built code to GitHub Pages, resolves fork PRs from the run's head SHA/branch instead of workflow_run.pull_requests, and removes the gh-pages deploy and cleanup workflows. deploy-pages.yml is back to contents: read.
509e3c1 to
cfe756b
Compare
|
@EmeditWeb This PR now has merge conflicts with git fetch upstream # upstream = https://github.com/DocForum/docforum-web.git
git rebase upstream/main
# resolve the conflicts, re-run the tests, then:
git push --force-with-leaseThe usual culprits are shared docs like Automated message. It won't repeat unless the PR conflicts again after your next push. |
|
@EmeditWeb resolve conflicts |
Resolve the PR DocForum#198 conflicts with DocForum/docforum-web main after the W7 notification/preferences/SSE work (DocForum#108-DocForum#111), Playwright suites (DocForum#156), jspdf 4 and the other recently merged changes. Conflict resolutions keep both sides: - package.json: retain test:coverage plus test:e2e/test:e2e:update; lockfile rebuilt from upstream with the coverage-v8 dependency graph added. - main.tsx: keep the preview-mock build flag and async boot (no top-level await), render behind upstream's TimeZoneProvider and Sentry init. - App.tsx: register both the notification inbox and notification settings routes. - types/models.ts, mocks: union of the notification-centre contract (DocForum#107) and notification-preferences/flags models (DocForum#108/DocForum#111); notification handlers serve both inbox and preferences endpoints. - states components: keep Skeleton/EmptyState/InlineError while carrying upstream's time-zone-aware date formatting in DataExportSection, CareTimeline and DependentsList. - test renderer: upstream's TimeZoneProvider inside renderWithProviders. - mocks/handlers.test.ts: import notification inbox and preferences services under distinct names. Verified: typecheck, 145 test files / 1317 tests, coverage gate (81.49 / 73.13 / 80.99 / 83.15 vs 78/70/77/79), production build, check:dist and check:stellar-boundary.
|
@EmeditWeb This PR now has merge conflicts with git fetch upstream # upstream = https://github.com/DocForum/docforum-web.git
git rebase upstream/main
# resolve the conflicts, re-run the tests, then:
git push --force-with-leaseThe usual culprits are shared docs like Automated message. It won't repeat unless the PR conflicts again after your next push. |
Resolve the PR DocForum#198 conflicts with DocForum/docforum-web main after the W7 platform-quality merge (DocForum#151). Conflict resolutions keep both sides: - Shared state components (Skeleton/InlineError/EmptyState, DocForum#106) stay in every conflicted screen, now carrying upstream's translated copy and error-catalogue messages (DataExportSection, ConsultationNote, DependentsList, DoctorInbox, FacilityFinder, PaymentIntentPanel, FacilityTeamView, HealthSection, PracticeInsightsView). - CareTimeline keeps DualZoneTime step timestamps plus upstream's locale-aware formatDate. - api-client exports both apiUrl (event stream) and getApiBaseUrl. - test renderer keeps renderWithProviders/signInAs/resetTestStores and wraps them in upstream's IntlProvider; render() accepts initialEntries. - setup.ts keeps the single MSW server lifecycle and store reset from DocForum#105 and registers the jest-axe matcher. - vite.config keeps VITE_BASE_PATH alongside the PWA plugin and the optimizeDeps exclusion. - package-lock regenerated from upstream's lock against the merged package.json. Verified: typecheck, 163 test files / 1420 tests, check:i18n and the production build. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Playwright/Chromium headless was missing system dependencies (libatk-1.0.so.0 etc.), causing visual test pixel differences and test failures. Installed dependencies and regenerated all 16 baseline screenshots (desktop + mobile).
afeezorobsco-cyber
left a comment
There was a problem hiding this comment.
Sweep: malware scan clean, CI green.
[Phase W7] Platform quality: PR previews, test utilities + coverage gate, consistent states, notification centre
Closes #104, closes #105, closes #106, closes #107
Summary
One PR for the four Phase W7 platform-quality issues:
.github/workflows/pr-preview-{build,deploy,cleanup}.yml,scripts/check-preview-dist.mjssrc/test/{render.tsx,factories.ts,server.ts},vite.config.ts,.github/workflows/ci.ymlsrc/components/states/+ the query-driven views that consumed themsrc/features/notifications/,src/services/notifications-service.ts,src/pages/NotificationsPage.tsxROADMAP.md(W7 items ticked),ARCHITECTURE_ESSENTIALS.md(new sections + "Known assumptions (issue #107)") andCONTRIBUTING.md("Preview deployments") are updated, and the changelog entry is inchangelog.d/104-107-w7-platform-quality.mdper the repo's changelog.d rules.Issue #104 — Preview deployments for every pull request
Security model (the thing to review)
Fork PR code must never get a write token or a secret, so the pipeline is split in two:
pr-preview-build.yml— onpull_request: checkout of the PR,npm ci, typecheck, tests, andnpm run build:previewin mock-API mode, uploadingdist/as an artifact.permissions: contents: read, no secrets — for a fork PR apull_requesttoken is read-only anyway, and nothing here needs either.pr-preview-deploy.yml— onworkflow_run(after the build): has write permissions, but never checks out or executes the PR. It only downloads the artifact and copies its bytes intopr-preview/pr-<n>/ongh-pages, then upserts one PR comment with the preview URL. Both it anddeploy-pages.ymlpush togh-pages, so they share one concurrency group (pages-publish) — a main deploy can never delete a preview and vice versa.pr-preview-cleanup.yml— onpull_request_target: [closed](needed for a write token on a fork PR), deletespr-preview/pr-<n>/and the preview comment. Safe for the same reason: the PR is never checked out and no PR code or artifact is read; the only input is the PR number.Previews can never reach the real backend
VITE_API_BASE_URL=https://mock-api.invalid(a reserved, guaranteed-unroutable name), so even a request that escapes MSW has nowhere to go.connect-srcinvite.config.tsand theindex.htmlpreconnect now follow the configured API origin, so a preview's HTML/CSP never mentions the real Render host either.npm run check:preview(scripts/check-preview-dist.mjs) fails the build if the mock API markers (setupWorker, the dev panel, the fixtures' scenario labels) are missing fromdist/or if the real API host or the production Pages origin appears anywhere in it. CI-equivalent guard rails both ways:check:distproves production has no mocks,check:previewproves a preview is nothing but mocks.VITE_BASE_PATH=/docforum-web/pr-preview/pr-<n>/serves each PR under its own subpath;VITE_USE_HASH_ROUTER=truekeeps deep links working on a static host (see ADR 0001).deploy-pages.ymlpreserves thepr-preview/directory when it publishesmain.One-time setup for maintainers
Documented in
CONTRIBUTING.md→ "Preview deployments": set the Pages source to thegh-pagesbranch once (or point the deploy job at a Cloudflare/Netlify host). No secrets are required for previews at all.Bug found and fixed while verifying
src/main.tsxoriginally used a top-levelawait import('./mocks/setup')behind the mocks flag. That compiled in dev but failed the production-target build withTop-level await is not available in the configured target environment— so the preview workflow would have been red on day one. The dynamic import now lives inside an asyncboot()that still completes before the first render (the persona session exists when route guards evaluate; deep links still don't bounce to/loginmid-boot), and the statically-dead branch keeps the mocks chunk out of production builds —npm run check:distconfirms it.Issue #105 — Test utilities and coverage gate
src/test/render.tsx—renderWithProviders(ui, { route, role, session }): freshQueryClientper test (retries off, so a failing query fails the test instead of silently retrying),MemoryRouterat a chosen route, optional signed-in role/session.signInAs(role)andresetTestStores()for tests that set up state directly; plainrenderstays as the entry point existing suites already use.src/test/factories.ts— typed factories mirroringsrc/types/models.ts(makeUser/makePatientUser/…,makeLabOrder,makePaymentIntent,makeNotification, …). Ids come from a deterministic counter and timestamps from a fixed clock, so assertions on generated ids stay stable;resetFactorySeq()resets both.src/test/server.ts— a single MSW server oversrc/mocks/handlers.ts, started/reset/closed once per run fromsrc/test/setup.tswithonUnhandledRequest: 'bypass'.src/mocks/handlers.test.tsnow shares it instead of starting a second server (two servers in one worker double-intercept requests).src/features/**andsrc/services/**(tests, stories,.d.tsand feature barrels excluded) with thresholds invite.config.tsset at the floor measured when the gate landed: statements 78 / branches 70 / functions 77 / lines 79. They ratchet up as tests land, never down. CI'snpm teststep is nownpm run test:coverage, so a coverage regression fails the build.src/features/auth/components/SignupForm.test.tsx(hand-rolled render + per-test server →renderWithProviders+ the shared server) andsrc/features/orders/components/OrdersList.test.tsx(hand-made fixtures → factories +signInAs('patient')).Issue #106 — Consistent loading, empty and error states
New
src/components/states/module, using the existing design tokens:Skeleton— placeholder bars announced withrole="status"+ a visually hidden "Loading…", and aprefers-reduced-motionfallback (no shimmer for users who asked for less motion).EmptyState— icon slot, title, description, and a required next action (button or link) — an empty list always tells the user what to do next.InlineError— for a failed section inside a working page; optional retry button.PageError— route-level failure with a retry affordance.QueryState— the React Query glue: picks loading / error / empty / content from a query result, plusqueryErrorMessage(error)for consistent, non-leaky messages (timeout, abort, APImessage, unknown fallback).Applied across the query-driven views instead of hand-rolled spinners and blank screens: patient dashboard, orders list/detail/finder, payments (intent panel, payout ledger), care hub/timeline, consultation note, dependents, health profile, doctor today/inbox/message thread/composer, facility queue/team/insights, results inbox, calendar subscription, patient chart, queue panel, and the admin consoles (audit, closures, disputes, feature flags, jobs, readiness, referrals).
src/components/states/states.stories.tsxdocuments every state in CSF (picked up by Storybook once #97 lands) andstates.test.tsxasserts the accessibility contract (status role, reduced motion, focusable retry).Issue #107 — Notification centre
New
src/features/notifications/plussrc/services/notifications-service.ts:NotificationBell) — server-backed unread count in the header for every role, with a dropdown of the most recent notifications./notifications,NotificationList+NotificationsPage) — cursor-paginated list, per-kind deep links, mark-one and mark-all read, per-role "you have no notifications yet" empty state.notification-meta.ts) — every notification kind maps to a role-aware route and an icon; unknown kinds render generically (safe fallback route per role) instead of dead-linking.safeAppPath()rejects anything that isn't an in-app path, so a malicious server payload can never push the UI off-origin.useNotificationLiveUpdates) — a single SSE stream per session, opened with a single-use stream ticket fromPOST /notifications/stream-ticket— the access token never appears in a URL. Events fold into the React Query cache (notifications-cache.ts), so the list, the dropdown and the badge all update from one code path. A dropped stream falls back to the existing 60-second polling; the reconnect budget is capped so a downed backend isn't hammered.notifications-channel.ts) — aBroadcastChannelsyncs read-state and new-notification events between tabs of the same account (ids only; the pushed payload is the same PHI-light object the server would send anyway).src/mocks/handlers/notifications.ts) so the whole flow works undernpm run dev:mockand in PR previews.Assumed API contract (dependency not landed)
Depends on DocForum/docforum-core#93, which has not landed yet. The assumed shape is recorded in
ARCHITECTURE_ESSENTIALS.md→ "Known assumptions (issue #107)" and mirrored insrc/types/models.ts:GET/notifications?cursor=&limit=NotificationPage+unreadCountGET/notifications/stream-ticketGET/notifications/stream?ticket=text/event-streamof notification eventsPATCH/notifications/:id/readPATCH/notifications/read-allAll calls go through
docforum-core's API viaapiFetch— no direct Stellar/escrow calls (architecture hard rule 1). If the real endpoints differ, the only places to change aresrc/types/models.ts,src/services/notifications-service.tsand the MSW handlers.Acceptance checklist
pull_requestbuild with no secrets →workflow_runpublish that never runs PR code).check:preview).CONTRIBUTING.md.renderWithProviders(role/session/router/fresh QueryClient), typed factories, single MSW server.src/features/+src/services/, enforced in CI, thresholds at measured floor (ratchet up only).Skeleton/EmptyState/InlineError/PageError/QueryStatewith tests and stories.ROADMAP.mditems ticked; changelog entry inchangelog.d/;ARCHITECTURE_ESSENTIALS.mdupdated including "Known assumptions (issue [Platform] Notification centre: in-app inbox, unread badge and live updates #107)".Tests
New:
src/test/factories.ts,src/test/server.ts,src/components/states/states.test.tsx,src/features/notifications/{notification-meta,notifications-cache}.test.ts,src/features/notifications/components/{NotificationBell,NotificationList}.test.tsx,src/features/notifications/hooks/useNotifications.test.tsx.Results
No screens changed visually except the added notification bell and
/notificationsinbox, so no screenshots are attached for #104/#105/#106 (a preview URL is posted on the PR for #107's new screens).PRD flows
Notification centre implements the PRD §4 notification flow (FR-15 / FR-16: patients and doctors notified of bookings, referrals, orders and results — the in-app half; email/SMS remain the away-from-app channel). The other three issues are platform/infrastructure work with no new PRD surface. No
docforum-escrow/Stellar calls are added anywhere (hard rule 1 — enforced bycheck:stellar-boundary).How this will be reviewed
pr-preview-build.yml'spermissions/secretsand confirmpr-preview-deploy.ymlandpr-preview-cleanup.ymlnever contain acheckoutof the PR ref. The only untrusted input that reaches a privileged job is the PR number.npm run check:previewis the mechanical proof; the builtindex.htmlin a preview hasconnect-src 'self' https://mock-api.invalid.npm run test:coveragefail.Skeleton→EmptyState/InlineError→ content; grep a screen forisLoadingto spot one that still hand-rolls it.Follow-ups / notes for maintainers