VIBE-221 Subscription Fulfilment Email #155
Conversation
Created specification and implementation plan for event-driven GOV.UK Notify email notification system with deduplication, retry logic, and audit logging. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Created specification document with requirements and acceptance criteria - Created tasks document with implementation checklist - Includes database schema, Gov.Notify integration, notification service, and testing tasks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Specification document with data models and Gov.Notify integration - Technical implementation plan with 10-phase approach - Detailed task breakdown with 70+ actionable tasks - Estimated effort: 4-6 days (27-43 hours) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- specification.md: Comprehensive system architecture and requirements - plan.md: 7-phase implementation plan (13 hours) - tasks.md: 30+ granular tasks with acceptance criteria - Downloaded email notification mockup images 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Created comprehensive technical implementation plan (plan.md) for subscription fulfilment: - Event-driven trigger system for publication notifications - Integration with VIBE-192 subscription infrastructure - Gov.Notify client integration with bilingual templates - Deduplication logic and error handling - Non-blocking batch processing architecture 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…s://github.com/hmcts/cath-service into feature/VIBE-221-subscription-fulfilment-email
The remote branch uses 'notifications' (plural) with better structure. Removed the local 'notification' directory to avoid confusion.
WalkthroughA notification system for hearing list publications is implemented via a new Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin/Upload
participant Ingestion as Blob Ingestion Service
participant NotifSvc as Notification Service
participant Subscriptions as Subscription Queries
participant GovNotify as Gov.Notify Client
participant AuditLog as Notification Audit Log
participant DB as Database
Admin->>Ingestion: Upload hearing list artefact
Ingestion->>DB: Save artefact
DB-->>Ingestion: artefact_id
Ingestion->>NotifSvc: sendPublicationNotifications(publicationId, locationId, ...)
activate NotifSvc
NotifSvc->>Subscriptions: findActiveSubscriptionsByLocation(locationId)
Subscriptions->>DB: Query subscriptions
DB-->>Subscriptions: [subscriptions with users]
Subscriptions-->>NotifSvc: subscriptions[]
par Process Each Subscription
rect rgb(200, 220, 255)
Note over NotifSvc,AuditLog: Per-user notification flow
NotifSvc->>AuditLog: createNotificationAuditLog(subscriptionId, userId, publicationId)
AuditLog->>DB: Insert notification_audit_log (Pending)
DB-->>AuditLog: notificationId
AuditLog-->>NotifSvc: notification record
alt Duplicate Check (user, publication already sent)
NotifSvc-->>NotifSvc: Mark as Skipped
else New User/Publication
NotifSvc->>GovNotify: sendEmail(templateId, email, templateParams)
alt Send Success
GovNotify-->>NotifSvc: { success, notificationId: govNotifyId }
NotifSvc->>AuditLog: updateNotificationStatus(notificationId, Sent, sentAt, govNotifyId)
AuditLog->>DB: Update status=Sent, gov_notify_id
else Send Failure
GovNotify-->>NotifSvc: { success: false, error }
NotifSvc->>AuditLog: updateNotificationStatus(notificationId, Failed, errorMessage)
AuditLog->>DB: Update status=Failed, error_message
end
end
end
end
deactivate NotifSvc
NotifSvc-->>Ingestion: { totalSubscriptions, sent, failed, skipped, errors }
Ingestion-->>Admin: success (notifications sent async)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
🎭 Playwright E2E Test Results151 tests 151 ✅ 10m 53s ⏱️ Results for commit ce6dbca. ♻️ This comment has been updated with latest results. |
Updated admin-pages manual-upload-summary to use the correct package name.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (24)
libs/notifications/tsconfig.json (1)
1-11: Consider adding typeRoots for consistency.While the root tsconfig.json includes
./libs/notifications/typesin the global typeRoots, the sibling modulelibs/notification/tsconfig.jsondeclares typeRoots locally. For consistency and explicit type resolution, consider adding:{ "extends": "../../tsconfig.json", "compilerOptions": { "outDir": "./dist", "rootDir": "./src", "declaration": true, - "declarationMap": true + "declarationMap": true, + "typeRoots": ["../../node_modules/@types", "./types"] }, "include": ["src/**/*", "types/**/*"], "exclude": ["**/*.test.ts", "**/*.spec.ts", "dist", "node_modules"] }apps/postgres/prisma/migrations/20251201115015_govnotifier_notification/migration.sql (1)
7-32: LGTM with minor observation.The
notification_logtable structure is well-designed with appropriate indexes for common query patterns. The indexes onuser_id,publication_id,subscription_id, andcreated_atwill support efficient lookups.Minor observation: Consider whether
location_idshould also have an index if queries will filter or join by location.docs/tickets/VIBE-221/tasks.md (1)
64-116: Markdown lint nits: bare URL + minor wordingThe content is clear; only minor markdown/grammar nits if you care about passing markdownlint/LanguageTool cleanly:
- Line 106: wrap the bare URL in
<...>or markdown link to satisfy MD034.- Line 37: “10 second timeout” → “10-second timeout” if you want to follow the hyphenation suggestion.
- Line 105: “populated correctly … correctly” – consider dropping one “correctly” to avoid repetition.
All optional and non-blocking.
libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
111-137: Notification trigger is awaited, and logging may expose too much detailA few points around this notification block:
Not truly fire-and-forget
- Because
sendPublicationNotificationsis awaited, the POST response is delayed until the whole notification run completes. That’s at odds with the “fire-and-forget” / non‑blocking requirement described in the VIBE-221 docs and could hurt UX if there are many subscribers or Notify is slow.- Consider kicking this off without awaiting the result, e.g.:
void sendPublicationNotifications(...).then(/* log summary */).catch(/* log error */);- This keeps the upload flow latency bound to artefact creation and file save, not email sending.
Log contents and PII
console.logof the fullnotificationResultobject andconsole.errorwith the rawnotificationErrorrisk leaking PII (email addresses, full Notify payloads, etc.) depending on how the notification module structures those objects.- Safer pattern is to log only high‑level, non‑PII fields (artefactId, locationId, counts, maybe a correlation/reference ID) and, for errors,
name/messageplus a sanitized error code instead of the full error object.- Suggest constraining logs here to a small, explicit shape and relying on the notification module itself for any deeper debug logging.
listTypeName field
- Here you use
listType?.name || \LIST_TYPE_${listTypeId}`whereas the rest of the file usesenglishFriendlyName/welshFriendlyName. Please double‑check thatmockListTypesactually exposes aname` field and that it’s the intended value for the email template; otherwise you may want to reuse the same friendly name logic for consistency.These are mostly design/privacy points rather than correctness, but they’re worth tightening before release.
docs/VIBE-221/tasks.md (1)
771-815: Markdown headings vs emphasis (MD036)In the “Progress Tracking” section the phase labels are bolded (
**Phase 1: Database Schema**, etc.). markdownlint flags this as “emphasis used instead of a heading” (MD036). If you want a clean lint run, consider turning these into proper headings, e.g.:### Phase 1: Database Schemaand similarly for the other phases. Content is otherwise fine.
libs/notifications/src/notification/notification-queries.test.ts (1)
1-97: Tests correctly assert Prisma interaction; consider one extra assertionThe mocks and expectations line up with the implementation:
createNotificationAuditLogreturns the mocked record and defaults status to "Pending".updateNotificationStatusandfindExistingNotificationassertions match the Prisma calls, including the compositeuserId_publicationIdkey anderrorMessage: undefined.If you want slightly stronger coverage, you could also assert on the arguments passed to
prisma.notificationAuditLog.create(status defaulting) in the first test, but as is this is already adequate.VIBE-221-plan.md (1)
238-262: Add language to fenced block for markdownlint (MD040)markdownlint flags the email‑template block for lacking a language (MD040). Since this is prose, using
text(ormd) is enough to satisfy it:```text Subject: New hearing list published: ((hearing_list_name)) ...Purely a documentation nit; behavior is unaffected. </blockquote></details> <details> <summary>libs/notification/src/notifications-node-client.d.ts (1)</summary><blockquote> `1-26`: **Duplicate NotifyClient typings and default export shape** Two small points on this declaration: 1. **Duplication** - There are already NotifyClient typings elsewhere (`types/notifications-node-client.d.ts` / `libs/notification/types/notifications-node-client.d.ts`). To avoid these drifting out of sync, consider having a single canonical definition and re‑using it (e.g. via `declare module "notifications-node-client"` that re‑exports that type), rather than maintaining multiple copies. 2. **Default export shape** - `export default { NotifyClient };` declares the default as an object with a `NotifyClient` property. If the real package exports a class as its default (or only uses a named export), this may not accurately match the runtime shape, and could mislead consumers using `import NotifyClient from "notifications-node-client";`. - If the project only ever uses `import { NotifyClient } ...`, you can probably drop the default export from this declaration to keep types closer to reality. Neither is breaking right now, but consolidating the typings and aligning the default export with actual usage will reduce confusion later. </blockquote></details> <details> <summary>apps/postgres/prisma/migrations/20251201095418_add_notification_audit_log/migration.sql (1)</summary><blockquote> `8-34`: **Notification audit table design looks consistent; consider status tightening** The `notification_audit_log` shape (UUID PK, timestamps, unique `(user_id, publication_id)` index, FK to `subscription`) looks aligned with a per-user-per-publication audit trail. If you want to prevent typos or drift in `status`, consider normalising it (enum / CHECK constraint), otherwise this is fine as-is. </blockquote></details> <details> <summary>libs/notification/prisma/schema.prisma (1)</summary><blockquote> `1-18`: **NotificationLog schema matches conventions; indexes look sensible** Model naming and `@map` / `@@map` usage align with the Prisma/DB naming guidelines, and the chosen indexes support common access patterns by user, subscription, publication, and time. If you anticipate frequent queries by status (e.g. failed/pending retries), adding an index on `status` later may help, but not required now. </blockquote></details> <details> <summary>docs/VIBE-221/plan.md (1)</summary><blockquote> `37-47`: **Add a language to the project-structure code fence** The file tree block is fenced with ``` but no language, which markdownlint (MD040) flags. Using something like: ```markdown ```text libs/notification/ ├── package.json …will keep the rendering the same while satisfying the linter. </blockquote></details> <details> <summary>docs/VIBE-221/specification.md (1)</summary><blockquote> `41-48`: **Tidy fenced blocks and bare URL to satisfy markdownlint** A few small tweaks will make this spec pass markdownlint: - Architecture diagram and GOV.UK Notify template snippet: add an explicit language, e.g. ```markdown ```text Publication Event → …and ```markdown ```text Your subscription has been triggered…- GOV.UK Notify docs link: wrap as a Markdown link instead of a bare URL, e.g. ```markdown - Documentation: [GOV.UK Notify Node client](https://docs.notifications.service.gov.uk/node.html)These keep the content unchanged but clear MD040/MD034.
Also applies to: 509-509, 648-657
libs/notifications/src/notification/notification-service.test.ts (1)
1-193: Good coverage of happy-path and edge cases; consider adding failure-path assertionsThe suite exercises the main flows (multi-subscriber send, duplicate suppression, invalid email skip, no-subscribers, invalid event) with a clean mocking pattern. To harden it further, you could:
- Add a test where
sendEmailrejects or returns a failure, assertingfailedanderrorsare populated andupdateNotificationStatusis called appropriately.- Optionally assert that
findActiveSubscriptionsByLocation/createNotificationAuditLogare invoked with the expected arguments.Not mandatory, but would make regressions in error handling more visible.
libs/notifications/src/notification/validation.ts (1)
1-50: Validation logic is sound; optional check for invalid Date valuesThe email validator and
validatePublicationEventcover the basic required fields and are consistent with how the service uses them. If you expectpublicationDateto sometimes be an invalid Date object (e.g.new Date("bad")), you could strengthen that check with:if (!event.publicationDate || Number.isNaN(event.publicationDate.getTime())) { errors.push("Publication date is required"); }Otherwise this is fine as-is.
libs/notifications/src/notification/subscription-queries.ts (1)
1-31: Subscription query is correct; consider future filters for email statusThe Prisma query and returned
SubscriptionWithUsershape look correct and comply with the parameterisation guidelines. As the user model matures, you may want to extend thewhereclause to filter on flags likeemailVerified/isActive(as outlined in the spec’s optimised SQL example) so invalid or inactive accounts are excluded at source.VIBE-221-specification.md (1)
1-342: LGTM! Comprehensive specification document.The specification is well-structured and provides clear technical requirements, event flow, error handling scenarios, and test cases. The document will serve as a valuable reference for implementation and testing.
Optional: Add language specifiers to fenced code blocks.
For improved markdown rendering and syntax highlighting, consider adding language specifiers to fenced code blocks (e.g.,
```sql,```typescript,```text). The bare URL on line 70 could also be wrapped in angle brackets or a markdown link for consistency.Apply static analysis suggestions if desired:
- Lines 49, 60, 66, 107, 157, 263: Add language specifiers
- Line 70: Wrap URL in markdown link format
libs/notification/src/notification-service.ts (1)
131-154: Consider extracting template parameter building logic.The inline date formatting and list type lookup logic makes the function harder to read. Consider extracting this into a dedicated helper function like
buildTemplateParameters.Extract template parameter construction into a separate helper function to improve readability and testability.
function buildTemplateParameters(user: User, hearingListName: string, publicationDate: string, location: Location, baseUrl: string) { const listType = mockListTypes.find((lt) => lt.name === hearingListName); const listTypeFriendlyName = listType?.englishFriendlyName || hearingListName; const date = new Date(publicationDate); const day = String(date.getDate()).padStart(2, "0"); const month = String(date.getMonth() + 1).padStart(2, "0"); const year = date.getFullYear(); const formattedDate = `${day}/${month}/${year}`; return { ListType: listTypeFriendlyName, content_date: formattedDate, locations: location.name, start_page_link: baseUrl, subscription_page_link: `${baseUrl}/account-home` }; }Then use it in the main function:
const baseUrl = process.env.BASE_URL || "https://localhost:8080"; - - // Convert list type name to friendly name - const listType = mockListTypes.find((lt) => lt.name === hearingListName); - const listTypeFriendlyName = listType?.englishFriendlyName || hearingListName; - - // Format date to dd/mm/yyyy - const date = new Date(publicationDate); - const day = String(date.getDate()).padStart(2, "0"); - const month = String(date.getMonth() + 1).padStart(2, "0"); - const year = date.getFullYear(); - const formattedDate = `${day}/${month}/${year}`; - + const personalisation = buildTemplateParameters(user, hearingListName, publicationDate, location, baseUrl); const client = await getNotifyClient(); const response = await client.sendEmail(GOVUK_NOTIFY_TEMPLATE_ID, user.email, { - personalisation: { - ListType: listTypeFriendlyName, - content_date: formattedDate, - locations: location.name, - start_page_link: baseUrl, - subscription_page_link: `${baseUrl}/account-home` - }, + personalisation, reference: `${publicationId}-${subscription.subscriptionId}` });docs/tickets/VIBE-221/plan.md (1)
1-544: LGTM! Comprehensive technical plan.The plan provides:
- Clear architecture overview with module structure
- Detailed database schema with proper snake_case naming
- Well-defined component responsibilities
- Comprehensive error handling scenarios
- Thorough testing strategy
- Security and compliance considerations
- Performance optimization guidelines
- Open questions that need clarification
The plan aligns well with the specification and provides solid guidance for implementation.
Optional: Address markdown linting issues.
Similar to the specification document, consider adding language specifiers to fenced code blocks and wrapping bare URLs in markdown links for better rendering.
libs/notifications/prisma/schema.prisma (1)
11-27: Consider using enum for status field.Line 16 uses
Stringfor the status field, but the specification defines specific status values ("Pending", "Sent", "Failed", "Skipped"). Using a Prisma enum would provide better type safety and prevent invalid status values.Define a Prisma enum for notification status to improve type safety:
+enum NotificationStatus { + PENDING + SENT + FAILED + SKIPPED +} + model NotificationAuditLog { notificationId String @id @default(uuid()) @map("notification_id") @db.Uuid subscriptionId String @map("subscription_id") @db.Uuid userId String @map("user_id") @db.Uuid publicationId String @map("publication_id") @db.Uuid - status String @default("Pending") + status NotificationStatus @default(PENDING) errorMessage String? @map("error_message") createdAt DateTime @default(now()) @map("created_at") sentAt DateTime? @map("sent_at")This aligns with the VIBE-221 specification which defines an enum for NotificationStatus.
libs/notifications/src/govnotify/govnotify-client.ts (1)
23-43: Consider caching the NotifyClient instance.A new
NotifyClientis instantiated on every email send. While acceptable for low-volume usage, consider extracting client creation if this becomes a performance concern at scale.libs/notifications/src/notification/notification-service.ts (1)
98-112: Redundant database operations when skipping notifications.The code creates an audit log with status "Skipped" then immediately updates the same record to "Skipped" with an error message. This results in two unnecessary database round-trips.
Consider passing the error message during creation, or modifying
createNotificationAuditLogto accept an optional error message:if (!subscription.user.email) { - const notification = await createNotificationAuditLog({ + await createNotificationAuditLog({ subscriptionId: subscription.subscriptionId, userId: subscription.userId, publicationId: event.publicationId, - status: "Skipped" + status: "Skipped", + errorMessage: "No email address" }); - - await updateNotificationStatus(notification.notificationId, "Skipped", undefined, "No email address"); return { status: "skipped", error: `User ${subscription.userId}: No email address` }; }This would require updating
CreateNotificationDatainterface to include an optionalerrorMessagefield.libs/notification/src/repository/queries.ts (3)
4-10: Export the interface for consumer type safety.
CreateNotificationLogParamsis not exported, which means callers cannot reference this type when constructing parameters.-interface CreateNotificationLogParams { +export interface CreateNotificationLogParams { subscriptionId: string; userId: string; publicationId: string; locationId: string; status: string; }
30-49: Inconsistent status string casing between modules.This file uses uppercase status values (
"SENT","FAILED") whilenotification-queries.tsuses title case ("Sent","Failed","Pending","Skipped"). If these feed into the same reporting or query system, this inconsistency could cause issues.Consider extracting status constants to a shared module:
// notification-status.ts export const NotificationStatus = { PENDING: "Pending", SENT: "Sent", FAILED: "Failed", SKIPPED: "Skipped" } as const;
51-63: Add explicit return types for public API functions.Both
findNotificationLogsByPublicationIdandfindNotificationLogsByUserIdrely on inferred return types. Adding explicit types improves API clarity and catches accidental changes.-export async function findNotificationLogsByPublicationId(publicationId: string) { +export async function findNotificationLogsByPublicationId(publicationId: string): Promise<NotificationLog[]> { return prisma.notificationLog.findMany({ where: { publicationId }, orderBy: { createdAt: "desc" } }); } -export async function findNotificationLogsByUserId(userId: string) { +export async function findNotificationLogsByUserId(userId: string): Promise<NotificationLog[]> { return prisma.notificationLog.findMany({ where: { userId }, orderBy: { createdAt: "desc" } }); }You'll need to define or import a
NotificationLoginterface matching the Prisma model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/VIBE-221/email notification mock up.pngis excluded by!**/*.pngdocs/VIBE-221/email notification template .pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (52)
.ai/plans/VIBE-221/plan.md(1 hunks).ai/plans/VIBE-221/specification.md(1 hunks).ai/plans/VIBE-221/tasks.md(1 hunks)VIBE-221-plan.md(1 hunks)VIBE-221-specification.md(1 hunks)apps/postgres/prisma/migrations/20251201095418_add_notification_audit_log/migration.sql(1 hunks)apps/postgres/prisma/migrations/20251201115015_govnotifier_notification/migration.sql(1 hunks)apps/postgres/src/schema-discovery.test.ts(1 hunks)apps/postgres/src/schema-discovery.ts(1 hunks)docs/VIBE-221/plan.md(1 hunks)docs/VIBE-221/specification.md(1 hunks)docs/VIBE-221/tasks.md(1 hunks)docs/tickets/VIBE-221/plan.md(1 hunks)docs/tickets/VIBE-221/specification.md(1 hunks)docs/tickets/VIBE-221/tasks.md(1 hunks)libs/account/src/repository/query.test.ts(2 hunks)libs/account/src/repository/query.ts(1 hunks)libs/admin-pages/src/pages/manual-upload-summary/index.ts(2 hunks)libs/api/src/blob-ingestion/repository/service.ts(3 hunks)libs/notification/package.json(1 hunks)libs/notification/prisma/schema.prisma(1 hunks)libs/notification/src/config.ts(1 hunks)libs/notification/src/index.ts(1 hunks)libs/notification/src/notification-service.test.ts(1 hunks)libs/notification/src/notification-service.ts(1 hunks)libs/notification/src/notifications-node-client.d.ts(1 hunks)libs/notification/src/repository/queries.test.ts(1 hunks)libs/notification/src/repository/queries.ts(1 hunks)libs/notification/tsconfig.json(1 hunks)libs/notification/types/notifications-node-client.d.ts(1 hunks)libs/notifications/package.json(1 hunks)libs/notifications/prisma/schema.prisma(1 hunks)libs/notifications/src/config.ts(1 hunks)libs/notifications/src/govnotify/govnotify-client.test.ts(1 hunks)libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)libs/notifications/src/govnotify/template-config.ts(1 hunks)libs/notifications/src/index.ts(1 hunks)libs/notifications/src/notification/notification-queries.test.ts(1 hunks)libs/notifications/src/notification/notification-queries.ts(1 hunks)libs/notifications/src/notification/notification-service.test.ts(1 hunks)libs/notifications/src/notification/notification-service.ts(1 hunks)libs/notifications/src/notification/subscription-queries.test.ts(1 hunks)libs/notifications/src/notification/subscription-queries.ts(1 hunks)libs/notifications/src/notification/validation.test.ts(1 hunks)libs/notifications/src/notification/validation.ts(1 hunks)libs/notifications/src/notifications-node-client.d.ts(1 hunks)libs/notifications/tsconfig.json(1 hunks)libs/notifications/types/notifications-node-client.d.ts(1 hunks)libs/subscriptions/prisma/schema.prisma(1 hunks)libs/subscriptions/src/repository/queries.ts(1 hunks)tsconfig.json(2 hunks)types/notifications-node-client.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
apps/postgres/src/schema-discovery.tslibs/account/src/repository/query.tslibs/notification/src/index.tslibs/notifications/src/notification/validation.test.tslibs/subscriptions/src/repository/queries.tslibs/notifications/src/notification/notification-service.test.tslibs/notification/src/config.tslibs/notifications/src/config.tslibs/account/src/repository/query.test.tslibs/notifications/src/notifications-node-client.d.tslibs/notifications/src/notification/validation.tslibs/notifications/src/notification/notification-queries.tslibs/notification/src/notification-service.test.tslibs/notifications/src/govnotify/template-config.tslibs/notification/src/repository/queries.test.tslibs/notification/src/notification-service.tslibs/notifications/src/govnotify/govnotify-client.tslibs/notifications/src/notification/subscription-queries.test.tslibs/notifications/types/notifications-node-client.d.tslibs/notification/src/notifications-node-client.d.tslibs/notifications/src/notification/notification-service.tslibs/notifications/src/notification/notification-queries.test.tslibs/notification/types/notifications-node-client.d.tslibs/notifications/src/govnotify/govnotify-client.test.tstypes/notifications-node-client.d.tslibs/notifications/src/notification/subscription-queries.tslibs/admin-pages/src/pages/manual-upload-summary/index.tslibs/notification/src/repository/queries.tsapps/postgres/src/schema-discovery.test.tslibs/notifications/src/index.tslibs/api/src/blob-ingestion/repository/service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
apps/postgres/src/schema-discovery.tslibs/account/src/repository/query.tslibs/notification/src/index.tslibs/notifications/src/notification/validation.test.tslibs/subscriptions/src/repository/queries.tslibs/notifications/src/notification/notification-service.test.tslibs/notification/src/config.tslibs/notifications/src/config.tslibs/account/src/repository/query.test.tslibs/notifications/src/notifications-node-client.d.tslibs/notifications/src/notification/validation.tslibs/notifications/src/notification/notification-queries.tslibs/notification/src/notification-service.test.tslibs/notifications/src/govnotify/template-config.tslibs/notification/src/repository/queries.test.tslibs/notification/src/notification-service.tslibs/notifications/src/govnotify/govnotify-client.tslibs/notifications/src/notification/subscription-queries.test.tslibs/notifications/types/notifications-node-client.d.tslibs/notification/src/notifications-node-client.d.tslibs/notifications/src/notification/notification-service.tslibs/notifications/src/notification/notification-queries.test.tslibs/notification/types/notifications-node-client.d.tslibs/notifications/src/govnotify/govnotify-client.test.tstypes/notifications-node-client.d.tslibs/notifications/src/notification/subscription-queries.tslibs/admin-pages/src/pages/manual-upload-summary/index.tslibs/notification/src/repository/queries.tsapps/postgres/src/schema-discovery.test.tslibs/notifications/src/index.tslibs/api/src/blob-ingestion/repository/service.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/notifications/src/notification/validation.test.tslibs/notifications/src/notification/notification-service.test.tslibs/account/src/repository/query.test.tslibs/notification/src/notification-service.test.tslibs/notification/src/repository/queries.test.tslibs/notifications/src/notification/subscription-queries.test.tslibs/notifications/src/notification/notification-queries.test.tslibs/notifications/src/govnotify/govnotify-client.test.tsapps/postgres/src/schema-discovery.test.ts
libs/*/src/config.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate
config.tsfile to avoid circular dependencies during Prisma client generation. Apps import config using the/configpath.
Files:
libs/notification/src/config.tslibs/notifications/src/config.ts
libs/*/tsconfig.json
📄 CodeRabbit inference engine (CLAUDE.md)
Module tsconfig.json must extend the root tsconfig.json with outDir, rootDir, and include/exclude declarations.
Files:
libs/notifications/tsconfig.jsonlibs/notification/tsconfig.json
**/tsconfig.json
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript must use strict mode with no
anywithout justification.
Files:
libs/notifications/tsconfig.jsonlibs/notification/tsconfig.jsontsconfig.json
libs/*/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
libs/*/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
Module package.json must include both main export (.) and config export (./config) in the exports field, with production and default conditions.
Module package.json build script must includebuild:nunjucksif the module contains Nunjucks templates in thepages/directory.
All test packages must use"test": "vitest run"script to run tests.
Files:
libs/notification/package.jsonlibs/notifications/package.json
**/prisma/schema.prisma
📄 CodeRabbit inference engine (CLAUDE.md)
Database tables and fields MUST be singular and snake_case (e.g.,
user,case,created_at). Use Prisma@@mapand@mapfor aliases.
Files:
libs/notifications/prisma/schema.prismalibs/notification/prisma/schema.prismalibs/subscriptions/prisma/schema.prisma
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/admin-pages/src/pages/manual-upload-summary/index.ts
🧠 Learnings (13)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/src/config.ts : Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate `config.ts` file to avoid circular dependencies during Prisma client generation. Apps import config using the `/config` path.
Applied to files:
apps/postgres/src/schema-discovery.tslibs/notification/src/config.tslibs/notifications/tsconfig.jsonlibs/notifications/src/config.tslibs/notifications/src/govnotify/template-config.tslibs/notification/tsconfig.jsontsconfig.json
📚 Learning: 2025-11-27T09:48:13.010Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 136
File: libs/api/src/blob-ingestion/validation.ts:156-163
Timestamp: 2025-11-27T09:48:13.010Z
Learning: In libs/api/src/blob-ingestion/validation.ts, the permissive date validation in isValidISODate and isValidISODateTime functions is expected behavior and should not be flagged for stricter validation.
Applied to files:
libs/notifications/src/notification/validation.test.ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{test,spec}.ts : Test files must be co-located with source files using the pattern `*.test.ts` or `*.spec.ts` and use Vitest.
Applied to files:
libs/notifications/src/notification/validation.test.tslibs/notifications/tsconfig.jsonlibs/account/src/repository/query.test.tslibs/notification/tsconfig.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/package.json : All test packages must use `"test": "vitest run"` script to run tests.
Applied to files:
libs/notifications/src/notification/validation.test.ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Use workspace aliases (`hmcts/*`) for imports instead of relative paths across packages.
Applied to files:
libs/notification/src/config.tstsconfig.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Always add `.js` extension to relative imports (e.g., `import { foo } from "./bar.js"`). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Applied to files:
libs/notification/src/config.tslibs/notifications/tsconfig.jsonlibs/notification/tsconfig.jsontsconfig.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/tsconfig.json : Module tsconfig.json must extend the root tsconfig.json with outDir, rootDir, and include/exclude declarations.
Applied to files:
libs/notifications/tsconfig.jsonlibs/notification/tsconfig.jsontsconfig.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/tsconfig.json : TypeScript must use strict mode with no `any` without justification.
Applied to files:
libs/notifications/tsconfig.jsonlibs/notification/tsconfig.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Applied to files:
libs/notifications/tsconfig.jsonlibs/notification/tsconfig.jsontsconfig.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/package.json : Module package.json must include both main export (`.`) and config export (`./config`) in the exports field, with production and default conditions.
Applied to files:
libs/notification/package.jsonlibs/notifications/package.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/package.json : Package names must use hmcts scope (e.g., `hmcts/auth`, `hmcts/case-management`).
Applied to files:
libs/notification/package.jsonlibs/notifications/package.jsontsconfig.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to package.json : All package.json files must use `"type": "module"` to enforce ES Modules.
Applied to files:
libs/notification/package.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/package.json : Module package.json build script must include `build:nunjucks` if the module contains Nunjucks templates in the `pages/` directory.
Applied to files:
libs/notification/package.jsonlibs/notifications/package.json
🧬 Code graph analysis (15)
libs/notifications/src/notification/validation.test.ts (1)
libs/notifications/src/notification/validation.ts (2)
isValidEmail(3-8)validatePublicationEvent(23-50)
libs/notifications/src/notification/notification-service.test.ts (4)
libs/notifications/src/notification/subscription-queries.ts (1)
findActiveSubscriptionsByLocation(14-31)libs/notifications/src/notification/notification-queries.ts (2)
findExistingNotification(45-56)createNotificationAuditLog(21-32)libs/notifications/src/index.ts (1)
sendPublicationNotifications(1-1)libs/notifications/src/notification/notification-service.ts (1)
sendPublicationNotifications(21-86)
libs/account/src/repository/query.test.ts (1)
libs/account/src/repository/query.ts (1)
findUserById(18-22)
libs/notifications/src/notifications-node-client.d.ts (2)
types/notifications-node-client.d.ts (1)
NotifyClient(2-24)libs/notifications/types/notifications-node-client.d.ts (1)
NotifyClient(2-24)
libs/notification/src/repository/queries.test.ts (1)
libs/notification/src/repository/queries.ts (3)
createNotificationLog(12-28)updateNotificationLogSent(30-38)updateNotificationLogFailed(40-49)
libs/notifications/src/govnotify/govnotify-client.ts (2)
libs/notifications/src/govnotify/template-config.ts (3)
TemplateParameters(32-39)getApiKey(12-17)getTemplateId(5-10)libs/notifications/src/notifications-node-client.d.ts (1)
NotifyClient(2-24)
libs/notifications/src/notification/subscription-queries.test.ts (1)
libs/notifications/src/notification/subscription-queries.ts (1)
findActiveSubscriptionsByLocation(14-31)
libs/notifications/types/notifications-node-client.d.ts (2)
types/notifications-node-client.d.ts (1)
NotifyClient(2-24)libs/notifications/src/notifications-node-client.d.ts (1)
NotifyClient(2-24)
libs/notification/src/notifications-node-client.d.ts (2)
libs/notification/types/notifications-node-client.d.ts (1)
NotifyClient(2-23)types/notifications-node-client.d.ts (1)
NotifyClient(2-24)
libs/notifications/src/notification/notification-service.ts (5)
libs/notifications/src/notification/validation.ts (3)
PublicationEvent(10-16)validatePublicationEvent(23-50)isValidEmail(3-8)libs/notifications/src/notification/subscription-queries.ts (1)
SubscriptionWithUser(3-12)libs/notifications/src/notification/notification-queries.ts (3)
findExistingNotification(45-56)createNotificationAuditLog(21-32)updateNotificationStatus(34-43)libs/notifications/src/govnotify/template-config.ts (1)
buildTemplateParameters(41-54)libs/notifications/src/govnotify/govnotify-client.ts (1)
sendEmail(19-21)
libs/notifications/src/notification/notification-queries.test.ts (1)
libs/notifications/src/notification/notification-queries.ts (3)
createNotificationAuditLog(21-32)updateNotificationStatus(34-43)findExistingNotification(45-56)
libs/notifications/src/govnotify/govnotify-client.test.ts (1)
libs/notifications/src/govnotify/govnotify-client.ts (1)
sendEmail(19-21)
types/notifications-node-client.d.ts (3)
libs/notification/src/notifications-node-client.d.ts (1)
NotifyClient(2-23)libs/notifications/src/notifications-node-client.d.ts (1)
NotifyClient(2-24)libs/notifications/types/notifications-node-client.d.ts (1)
NotifyClient(2-24)
libs/admin-pages/src/pages/manual-upload-summary/index.ts (3)
libs/publication/src/index.ts (1)
mockListTypes(1-1)libs/list-types/common/src/index.ts (1)
mockListTypes(2-2)libs/notification/src/notification-service.ts (1)
sendPublicationNotifications(38-236)
apps/postgres/src/schema-discovery.test.ts (1)
apps/postgres/src/schema-discovery.ts (1)
getPrismaSchemas(6-8)
🪛 LanguageTool
docs/VIBE-221/tasks.md
[grammar] ~158-~158: Ensure spelling is correct
Context: ...NotificationEmail()function 4. DefineEmailPersonalisationinterface 5. DefineSendEmailResult` i...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/tickets/VIBE-221/tasks.md
[grammar] ~37-~37: Use a hyphen to join words.
Context: ...] Add timeout handling for API calls (10 second timeout) - [x] Create template co...
(QB_NEW_EN_HYPHEN)
[style] ~105-~105: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...erify all template parameters populated correctly (user_name, hearing_list_name, publicat...
(ADVERB_REPETITION_PREMIUM)
docs/tickets/VIBE-221/plan.md
[style] ~484-~484: It’s more common nowadays to write this noun as one word.
Context: ... table with email field?) - Where is user name stored for template personalization? ...
(RECOMMENDED_COMPOUNDS)
[uncategorized] ~484-~484: Do not mix variants of the same word (‘personalization’ and ‘personalisation’) within a single text.
Context: ... Where is user name stored for template personalization? - Do we need to fetch user data fro...
(EN_WORD_COHERENCY)
VIBE-221-plan.md
[grammar] ~132-~132: Use a hyphen to join words.
Context: ...Retry Policy Decision: Retry once, 5 second delay Rationale: - Most trans...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~151-~151: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...for parallelism - GOV.UK Notify handles rate limiting - Better visibility into individual fai...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
docs/VIBE-221/specification.md
[uncategorized] ~354-~354: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...Errors**: - Count by error type - Rate limiting occurrences - Invalid email addresse...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~396-~396: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... Client**: - Mock API responses - Rate limiting handling - Error scenarios 3. **Aud...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~525-~525: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... Mitigations ### Risk 1: GOV.UK Notify Rate Limiting Impact: High volume of notification...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
docs/tickets/VIBE-221/specification.md
[style] ~7-~7: Try moving the adverb to make the sentence clearer.
Context: ...rigger-based mechanism in the CaTH back end to automatically send email notifications to subscribed users through Gov.Notify ...
(SPLIT_INFINITIVE)
.ai/plans/VIBE-221/plan.md
[style] ~475-~475: Consider using a different verb for a more formal wording.
Context: ...on flow continues normally 4. Debug and fix issues 5. Redeploy with fix ## Perform...
(FIX_RESOLVE)
[uncategorized] ~525-~525: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ses - Validate template parameters ### Rate Limiting - Respect Gov.Notify rate limits - Moni...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
docs/VIBE-221/tasks.md
771-771: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
778-778: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
784-784: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
790-790: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
796-796: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
802-802: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
807-807: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
812-812: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/VIBE-221/plan.md
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/tickets/VIBE-221/tasks.md
106-106: Bare URL used
(MD034, no-bare-urls)
docs/tickets/VIBE-221/plan.md
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Bare URL used
(MD034, no-bare-urls)
163-163: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
191-191: Bare URL used
(MD034, no-bare-urls)
541-541: Bare URL used
(MD034, no-bare-urls)
VIBE-221-plan.md
238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/VIBE-221/specification.md
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
509-509: Bare URL used
(MD034, no-bare-urls)
650-650: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
VIBE-221-specification.md
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Bare URL used
(MD034, no-bare-urls)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.ai/plans/VIBE-221/specification.md
38-38: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
60-60: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
110-110: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
apps/postgres/prisma/migrations/20251201115015_govnotifier_notification/migration.sql
Outdated
Show resolved
Hide resolved
- Set environment variables before module imports to ensure they're available - Fixed vi.fn() mock to use function declaration instead of arrow function - All 17 notifications tests now passing
- Pass Date object instead of ISO string for publicationDate - Include locationName parameter (fetch from database) - Use friendlyName instead of internal name for hearingListName - Properly handle missing location case - Fixed indentation and closing braces
- Added IF EXISTS to DROP CONSTRAINT statements - Prevents migration failures when constraint doesn't exist - Fixes GitHub Actions build error
- Removed 20251201115015_govnotifier_notification migration - This migration created 'notification_log' table (incorrect) - Keeping 20251201095418 which creates 'notification_audit_log' (correct) - Prevents table name mismatch with schema definition
- Added IF NOT EXISTS to CREATE TABLE statement - Added IF NOT EXISTS to all CREATE INDEX statements - Wrapped ADD CONSTRAINT in DO blocks with existence checks - Wrapped DROP DEFAULT in conditional check for existing default - Migration can now be safely re-run without errors
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/notifications/src/govnotify/govnotify-client.test.ts (1)
29-54: Expand test coverage to include error scenarios and retry logic.The test suite currently only covers the happy path. Consider adding test cases for:
- Email sending failures and error handling
- Retry logic with backoff (based on the
retryWithBackoffimplementation)- Missing or invalid environment variables
- Invalid email addresses or missing template parameters
This will help prevent regressions and ensure robust error handling.
libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
111-122: Remove redundant comments that describe "what" rather than "why".The comments on lines 111, 113, and 120 describe what the code does rather than why. Per the coding guidelines, comments should explain why something is done when the code itself is not self-explanatory. These comments can be safely removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/postgres/prisma/migrations/20251201095418_add_notification_audit_log/migration.sql(1 hunks)libs/admin-pages/src/pages/manual-upload-summary/index.ts(2 hunks)libs/notifications/src/govnotify/govnotify-client.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/postgres/prisma/migrations/20251201095418_add_notification_audit_log/migration.sql
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/admin-pages/src/pages/manual-upload-summary/index.tslibs/notifications/src/govnotify/govnotify-client.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/admin-pages/src/pages/manual-upload-summary/index.tslibs/notifications/src/govnotify/govnotify-client.test.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/admin-pages/src/pages/manual-upload-summary/index.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/notifications/src/govnotify/govnotify-client.test.ts
🧬 Code graph analysis (2)
libs/admin-pages/src/pages/manual-upload-summary/index.ts (3)
libs/location/src/repository/queries.ts (1)
getLocationById(37-65)libs/publication/src/index.ts (1)
mockListTypes(1-1)libs/list-types/common/src/index.ts (1)
mockListTypes(2-2)
libs/notifications/src/govnotify/govnotify-client.test.ts (4)
types/notifications-node-client.d.ts (1)
NotifyClient(2-24)libs/notifications/types/notifications-node-client.d.ts (1)
NotifyClient(2-24)libs/notifications/src/notifications-node-client.d.ts (1)
NotifyClient(2-24)libs/notifications/src/govnotify/govnotify-client.ts (1)
sendEmail(19-21)
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
libs/api/src/blob-ingestion/repository/service.test.ts (1)
301-517: Refactor tests to avoid setTimeout pattern for async operations.All notification-related tests use
await new Promise((resolve) => setTimeout(resolve, 50))to wait for fire-and-forget async operations. This pattern is problematic because:
- It's non-deterministic: the async operation might not complete within 50ms on slower environments
- It adds artificial delays that slow down test execution
- It can cause flaky test failures
Consider refactoring the production code to expose the notification promise for testing, either through:
- A test-mode flag that makes the operation awaitable
- Dependency injection that allows tests to intercept and await the async flow
- Using Vitest's
vi.waitFor()orvi.waitUntil()with proper async conditionsExample using
vi.waitFor():- // Wait for async notification processing - await new Promise((resolve) => setTimeout(resolve, 50)); + // Wait for the mock to be called + await vi.waitFor(() => { + expect(sendPublicationNotifications).toHaveBeenCalled(); + });Note: This still requires the production code to complete quickly, but provides better error messages and can have configurable timeouts.
e2e-tests/utils/notification-helpers.ts (1)
35-55: Consider extracting common Prisma type casting.Multiple functions use
(prisma as any)to access models not typed in the shared client. While this works, it reduces type safety. If this pattern is used elsewhere, consider creating typed wrappers or extending the shared Prisma client to include these models.e2e-tests/tests/manual-upload.spec.ts (2)
5-12: Unused import:getNotificationsByPublicationId.This helper is imported but never used in the tests. The tests only verify the success panel is visible, not that notification records were created. Consider adding assertions using
getNotificationsByPublicationIdto verify the notification audit trail, or remove the unused import.
839-839: Fixed delay may cause test flakiness.The
setTimeout(resolve, 2000)delays assume notifications complete within 2 seconds. Under load or with slower environments, this could cause intermittent failures. Consider polling for notification records with a timeout instead:// Example: Poll for notification completion const maxWait = 10000; const pollInterval = 500; let elapsed = 0; while (elapsed < maxWait) { const notifications = await getNotificationsByPublicationId(publicationId); if (notifications.length > 0) break; await new Promise(r => setTimeout(r, pollInterval)); elapsed += pollInterval; }Also applies to: 883-883
libs/notifications/src/govnotify/govnotify-client.test.ts (1)
146-182: Verify exponential backoff test assertion.The test checks that
delays[0]equals 1000ms (default delay), but it only verifies the first retry delay. For true exponential backoff verification, consider testing with more retry attempts to confirm the delay increases (e.g., 1000, 2000, 4000ms). Currently this only confirms a delay was applied, not that it's exponential.libs/notifications/src/notification/notification-service.test.ts (1)
300-332: Consider adding a test for partial audit log cleanup on exceptions.When
findExistingNotificationthrows, the test correctly expectsfailed: 1in the results. However, if the exception occurs aftercreateNotificationAuditLogsucceeds (e.g., during email sending), the audit log may remain in "Pending" status. Consider adding a test case that verifiesupdateNotificationStatusis called with "Failed" when an exception occurs after audit log creation.e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (1)
78-78: Hardcoded delays are fragile for async notification processing.Using fixed
setTimeoutdelays (2000ms) to wait for notification processing is brittle. Under load or slower environments, tests may fail spuriously. Consider implementing a polling mechanism:async function waitForNotifications(publicationId: string, expectedCount: number, timeoutMs = 10000) { const startTime = Date.now(); while (Date.now() - startTime < timeoutMs) { const notifications = await getNotificationsByPublicationId(publicationId); if (notifications.length >= expectedCount && notifications.every(n => n.status !== "Pending")) { return notifications; } await new Promise(r => setTimeout(r, 500)); } throw new Error(`Timed out waiting for ${expectedCount} notifications`); }This same issue applies to lines 105, 134, 158, 176, and 198.
libs/notifications/prisma/schema.prisma (1)
17-17: Consider using an enum for status field.Using
Stringfor status allows any value. Define a Prisma enum to restrict to valid statuses and improve type safety:enum NotificationStatus { Pending Sent Failed Skipped } model NotificationAuditLog { // ... status NotificationStatus @default(Pending) // ... }libs/notifications/src/notification/notification-queries.ts (4)
3-20: Avoid duplicating the Prisma model shape in local interfaces
NotificationAuditLog(and to a lesser extentCreateNotificationData) mirror the Prisma model fields. This will drift if the schema changes (new/renamed columns, nullability tweaks). If@hmcts/postgresexposes the generated Prisma types, consider aliasing those here and derivingCreateNotificationDatafrom them (e.g.Pick<...>), so the compiler catches schema changes.
22-33: Makestatusa well-defined domain type and avoid magic stringsRight now
statusis an untypedstringwith a hard‑coded"Pending"default. It would be safer to introduce a small status domain (string literal union or enum, plusSTATUS_PENDINGconstant) and use nullish coalescing for the default, e.g.status ?? STATUS_PENDING, so callers and future refactors can’t silently pass invalid values.
66-70: Clarify uniqueness / determinism forgovNotifyIdlookupsIf
govNotifyIdis guaranteed unique at the DB level, preferfindUniqueoverfindFirstto encode that invariant and fail fast if it is ever violated. If it is not unique by design, consider adding an explicitorderByhere so the chosen record is deterministic (e.g. most recent bycreatedAt), and document that behaviour in the caller.
72-75: Align publication lookups with deterministic ordering / reuse existing helper
getNotificationsByPublicationIdreturns results without anorderBy, while the e2e helper ate2e-tests/utils/notification-helpers.tsorders bycreatedAt: "asc". For consistency and to avoid tests or callers depending on implicit DB ordering, consider adding the sameorderByhere and, if practical, having the e2e helper call this function instead of duplicating the query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/postgres/prisma/migrations/20251201155125_add_gov_notify_id/migration.sql(1 hunks)e2e-tests/README-NOTIFICATIONS.md(1 hunks)e2e-tests/run-with-credentials.js(1 hunks)e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)e2e-tests/tests/manual-upload.spec.ts(2 hunks)e2e-tests/utils/api-auth-helpers.ts(1 hunks)e2e-tests/utils/notification-helpers.ts(1 hunks)libs/admin-pages/src/pages/manual-upload-summary/index.test.ts(2 hunks)libs/api/src/blob-ingestion/repository/service.test.ts(3 hunks)libs/notifications/prisma/schema.prisma(1 hunks)libs/notifications/src/govnotify/govnotify-client.test.ts(1 hunks)libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)libs/notifications/src/notification/notification-queries.ts(1 hunks)libs/notifications/src/notification/notification-service.test.ts(1 hunks)libs/notifications/src/notification/notification-service.ts(1 hunks)libs/notifications/src/notification/validation.ts(1 hunks)libs/subscriptions/src/repository/queries.test.ts(2 hunks)types/notifications-node-client.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e-tests/README-NOTIFICATIONS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/notifications/src/notification/validation.ts
- libs/notifications/src/govnotify/govnotify-client.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/subscriptions/src/repository/queries.test.tse2e-tests/tests/manual-upload.spec.tse2e-tests/utils/api-auth-helpers.tslibs/notifications/src/notification/notification-queries.tse2e-tests/tests/api/blob-ingestion-notifications.spec.tslibs/notifications/src/notification/notification-service.tslibs/notifications/src/notification/notification-service.test.tse2e-tests/utils/notification-helpers.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/notifications/src/govnotify/govnotify-client.test.tslibs/api/src/blob-ingestion/repository/service.test.tstypes/notifications-node-client.d.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/subscriptions/src/repository/queries.test.tse2e-tests/tests/manual-upload.spec.tse2e-tests/utils/api-auth-helpers.tslibs/notifications/src/notification/notification-queries.tse2e-tests/tests/api/blob-ingestion-notifications.spec.tslibs/notifications/src/notification/notification-service.tslibs/notifications/src/notification/notification-service.test.tse2e-tests/utils/notification-helpers.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tse2e-tests/run-with-credentials.jslibs/notifications/src/govnotify/govnotify-client.test.tslibs/api/src/blob-ingestion/repository/service.test.tstypes/notifications-node-client.d.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/subscriptions/src/repository/queries.test.tse2e-tests/tests/manual-upload.spec.tse2e-tests/tests/api/blob-ingestion-notifications.spec.tslibs/notifications/src/notification/notification-service.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/notifications/src/govnotify/govnotify-client.test.tslibs/api/src/blob-ingestion/repository/service.test.ts
**/prisma/schema.prisma
📄 CodeRabbit inference engine (CLAUDE.md)
Database tables and fields MUST be singular and snake_case (e.g.,
user,case,created_at). Use Prisma@@mapand@mapfor aliases.
Files:
libs/notifications/prisma/schema.prisma
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
🧠 Learnings (4)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/tsconfig.json : Module tsconfig.json must extend the root tsconfig.json with outDir, rootDir, and include/exclude declarations.
Applied to files:
types/notifications-node-client.d.ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Don't ignore TypeScript errors - fix or justify with comments.
Applied to files:
types/notifications-node-client.d.ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Don't create types.ts files - colocate types with the appropriate code.
Applied to files:
types/notifications-node-client.d.ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Don't duplicate dependencies - check root package.json first.
Applied to files:
types/notifications-node-client.d.ts
🧬 Code graph analysis (9)
libs/subscriptions/src/repository/queries.test.ts (1)
libs/subscriptions/src/repository/queries.ts (1)
findSubscriptionsByLocationId(14-23)
e2e-tests/tests/manual-upload.spec.ts (1)
e2e-tests/utils/notification-helpers.ts (4)
cleanupTestSubscriptions(84-92)cleanupTestUsers(94-102)createTestUser(19-33)createTestSubscription(35-55)
libs/notifications/src/notification/notification-queries.ts (1)
e2e-tests/utils/notification-helpers.ts (1)
getNotificationsByPublicationId(57-62)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (3)
e2e-tests/utils/notification-helpers.ts (7)
cleanupTestNotifications(74-82)cleanupTestSubscriptions(84-92)cleanupTestUsers(94-102)createTestUser(19-33)createTestSubscription(35-55)getNotificationsByPublicationId(57-62)getGovNotifyEmail(64-72)e2e-tests/utils/api-auth-helpers.ts (1)
getApiAuthToken(20-70)libs/notifications/src/notification/notification-queries.ts (1)
getNotificationsByPublicationId(72-76)
libs/notifications/src/notification/notification-service.ts (5)
libs/notifications/src/notification/validation.ts (3)
PublicationEvent(13-19)validatePublicationEvent(26-53)isValidEmail(6-11)libs/notifications/src/notification/subscription-queries.ts (2)
findActiveSubscriptionsByLocation(14-31)SubscriptionWithUser(3-12)libs/notifications/src/notification/notification-queries.ts (3)
findExistingNotification(53-64)createNotificationAuditLog(22-33)updateNotificationStatus(35-51)libs/notifications/src/govnotify/template-config.ts (1)
buildTemplateParameters(41-54)libs/notifications/src/govnotify/govnotify-client.ts (1)
sendEmail(18-29)
e2e-tests/utils/notification-helpers.ts (2)
libs/notifications/src/notification/notification-queries.ts (1)
getNotificationsByPublicationId(72-76)types/notifications-node-client.d.ts (1)
NotifyClient(2-30)
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (7)
libs/admin-pages/src/manual-upload/storage.ts (1)
getManualUpload(46-68)libs/admin-pages/src/manual-upload/file-storage.ts (1)
saveUploadedFile(12-23)libs/publication/src/repository/queries.ts (1)
createArtefact(4-52)libs/publication/src/index.ts (1)
createArtefact(6-6)libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
POST(212-212)libs/api/src/routes/v1/publication.ts (1)
POST(26-72)libs/location/src/repository/queries.ts (1)
getLocationById(37-65)
libs/notifications/src/govnotify/govnotify-client.test.ts (2)
types/notifications-node-client.d.ts (1)
NotifyClient(2-30)libs/notifications/src/govnotify/govnotify-client.ts (1)
sendEmail(18-29)
libs/api/src/blob-ingestion/repository/service.test.ts (3)
libs/api/src/blob-ingestion/validation.ts (1)
validateBlobRequest(9-158)libs/api/src/blob-ingestion/repository/service.ts (1)
processBlobIngestion(17-128)libs/api/src/blob-ingestion/repository/queries.ts (1)
createIngestionLog(4-16)
🔇 Additional comments (25)
libs/api/src/blob-ingestion/repository/service.test.ts (4)
6-40: LGTM!The mock additions follow the existing pattern and provide appropriate test doubles for the new dependencies.
43-48: LGTM!Import updates correctly reference the newly added mocks and mockListTypes.
220-299: LGTM!These tests provide thorough coverage of edge cases including missing fields, system errors, and non-Error exceptions with appropriate fallback behaviors.
301-336: LGTM!The notification test coverage is comprehensive, including:
- Successful notification flow with proper mock data
- Skipping notifications when location is not matched
- Graceful error handling that doesn't fail ingestion
- Edge cases for invalid/missing location and list type data
- Detailed logging verification for both success and error paths
The test logic and assertions are appropriate, with proper setup and teardown of console spies.
Also applies to: 338-354, 356-390, 392-414, 416-438, 440-465, 467-517
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (4)
566-602: Good test coverage for notification flow.The test properly verifies that
sendPublicationNotificationsis called with the expected payload after a successful upload. The assertions onpublicationId,locationId,locationName,hearingListName, andpublicationDateprovide comprehensive validation.
604-635: Resilience testing for missing location is well-implemented.The test correctly verifies that the upload succeeds even when
getLocationByIdreturnsnull, and that notifications are skipped with an appropriate warning log. The console spy cleanup withmockRestore()is properly handled.
637-675: Good fire-and-forget resilience test.This test validates that notification failures don't block the upload flow, which aligns with the async notification architecture described in the AI summary. The error logging verification ensures observability is maintained.
740-856: Comprehensive isFlatFile logic coverage.The tests cover JSON files (
isFlatFile: false), non-JSON files (isFlatFile: true), and the edge case of missingfileName(defaults totrue). This matches the expected behavior for flat file determination.e2e-tests/run-with-credentials.js (1)
23-25: LGTM - Azure credentials added for API E2E tests.The new secret mappings align with the
api-auth-helpers.tsrequirements and follow the existing naming conventions. Good that these credentials are not logged (unlike SSO/CFT emails), maintaining security best practices.libs/subscriptions/src/repository/queries.test.ts (1)
78-120: Well-structured tests for new location-based subscription query.The tests follow the established patterns in the file and properly verify that
findSubscriptionsByLocationIdpasses the correct parameters to Prisma (where.locationId,orderBy.dateAdded: "desc"). Coverage for both the happy path and empty result case is appropriate.apps/postgres/prisma/migrations/20251201155125_add_gov_notify_id/migration.sql (1)
1-15: Well-structured idempotent migration.The migration properly uses
IF NOT EXISTSchecks for both the column addition and index creation, ensuring it can be safely re-run. Thegov_notify_idcolumn and index support the notification audit functionality introduced in this PR.e2e-tests/tests/manual-upload.spec.ts (1)
795-801: Good cleanup order respects foreign key constraints.The cleanup correctly deletes subscriptions before users, which avoids FK constraint violations. The tracking pattern with
testDataarrays ensures reliable cleanup even if tests fail mid-execution.libs/notifications/src/govnotify/govnotify-client.test.ts (3)
29-54: Comprehensive test for successful email sending.The test properly verifies the template ID, email address, and personalisation parameters are passed correctly to the NotifyClient. The assertion structure clearly validates the expected behavior.
77-101: Good retry behavior coverage.Testing that the client retries on first failure and succeeds on the second attempt validates the retry logic works correctly with
NOTIFICATION_RETRY_ATTEMPTS=1(initial + 1 retry = 2 calls).
29-30: Dynamic imports may cause module caching issues.Using
await import("./govnotify-client.js")in each test could lead to the module being cached after the first import, meaning subsequent tests might not get fresh module instances. If tests start failing intermittently, consider usingvi.resetModules()inbeforeEachto ensure each test gets a fresh module instance.Also applies to: 56-57, 77-78, 103-104, 125-126, 146-147
e2e-tests/utils/api-auth-helpers.ts (2)
20-70: Well-implemented token caching with proper error handling.The implementation correctly:
- Caches tokens with a 5-minute safety buffer before expiry
- Provides clear error messages when credentials are missing
- Handles both HTTP errors and exceptions appropriately
- Avoids logging sensitive data (tokens/secrets)
The client credentials flow with
/.defaultscope is the correct pattern for Azure AD service-to-service authentication.
12-13: Module-level state is acceptable for E2E test helpers.While module-level mutable state (
cachedToken,tokenExpiry) is generally discouraged, it's appropriate here for E2E test utilities where token reuse across tests improves performance. TheclearCachedToken()function provides a way to reset state when needed.libs/notifications/src/notification/notification-service.test.ts (2)
1-79: Well-structured test setup and coverage.The mock setup and first test case correctly verify the notification flow for subscribed users. The use of
vi.mockat the module level followed byvi.mockedto configure return values is appropriate for Vitest.
182-204: LGTM - validation error tests are correctly structured.The
toThrow()assertions use substring matching, which will correctly match the actual error messages from the implementation (e.g.,"Invalid publication event: Publication ID is required, ..."will match"Invalid publication event").libs/notifications/src/notification/notification-service.ts (2)
21-86: Good use of Promise.allSettled for concurrent notification processing.The implementation correctly uses
Promise.allSettledto handle per-user notifications independently, preventing a single failure from affecting other users. The aggregation logic properly handles both fulfilled and rejected promises.
171-174: LGTM - buildUserName helper is concise and handles edge cases.Correctly filters out null/undefined values and provides a sensible fallback.
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (2)
39-46: Good use of test.skip for conditional test execution.The beforeAll hook appropriately skips tests when Azure AD OAuth tokens aren't available, with a clear explanation message directing users to alternative test suites.
48-56: Clean test data cleanup in afterEach.The cleanup order (notifications → subscriptions → users) respects foreign key constraints. Good pattern for test isolation.
libs/notifications/prisma/schema.prisma (2)
11-29: Schema follows naming conventions correctly.Good use of
@mapand@@mapto maintain snake_case in the database while using camelCase in the Prisma client. The indexes onpublicationId,status, andgovNotifyIdsupport the query patterns innotification-queries.ts.
22-22: No action needed. The Subscription model is properly defined inlibs/subscriptions/prisma/schema.prismaand the relation reference on line 22 is valid. This multi-file Prisma schema setup allows cross-module model references, which is correctly configured in this codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
e2e-tests/utils/notification-helpers.ts (1)
65-73: VerifygetNotificationByIdtype definition exists.The code calls
notifyClient.getNotificationById(notificationId)which may not be properly typed intypes/notifications-node-client.d.ts. If TypeScript compilation succeeds, the type definition exists. Otherwise, you'll need to extend the type definition.#!/bin/bash # Check if getNotificationById is defined in the type definition fd -t f "notifications-node-client.d.ts" | xargs rg "getNotificationById"libs/notifications/src/notification/notification-service.ts (3)
84-98: Redundant double-write persists: audit log created then immediately updated with same status.The code creates a notification audit log with status "Skipped" (line 89), then immediately calls
updateNotificationStatuswith the same "Skipped" status and error message (line 92). This is an unnecessary database round-trip.Per previous review feedback, pass the error message during creation:
if (!subscription.user.email) { - const notification = await createNotificationAuditLog({ + await createNotificationAuditLog({ subscriptionId: subscription.subscriptionId, userId: subscription.userId, publicationId: event.publicationId, - status: "Skipped" + status: "Skipped", + errorMessage: "No email address" }); - await updateNotificationStatus(notification.notificationId, "Skipped", undefined, "No email address"); - return { status: "skipped", error: `User ${subscription.userId}: No email address` }; }This requires adding an optional
errorMessagefield to theCreateNotificationDatainterface innotification-queries.ts.
100-114: Same double-write issue for invalid email case.Same pattern as the no-email case above - creates audit log then immediately updates it with the same status.
if (!isValidEmail(subscription.user.email)) { - const notification = await createNotificationAuditLog({ + await createNotificationAuditLog({ subscriptionId: subscription.subscriptionId, userId: subscription.userId, publicationId: event.publicationId, - status: "Skipped" + status: "Skipped", + errorMessage: "Invalid email format" }); - await updateNotificationStatus(notification.notificationId, "Skipped", undefined, "Invalid email format"); - return { status: "skipped", error: `User ${subscription.userId}: Invalid email format` }; }
148-155: Exception handling doesn't update audit log status on failure.If
createNotificationAuditLogsucceeds (line 116) but a subsequent exception occurs beforeupdateNotificationStatus, the audit log remains in "Pending" status indefinitely. This leaves orphaned records in the database.Refactor to ensure the audit log is updated even on exceptions:
+ let notification; try { - const notification = await createNotificationAuditLog({ + notification = await createNotificationAuditLog({ subscriptionId: subscription.subscriptionId, userId: subscription.userId, publicationId: event.publicationId, status: "Pending" }); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + return { + status: "failed", + error: `User ${subscription.userId}: ${errorMessage}` + }; + } + try { const userName = buildUserName(subscription.user.firstName, subscription.user.surname); const templateParameters = buildTemplateParameters({ userName, hearingListName: event.hearingListName, publicationDate: event.publicationDate, locationName: event.locationName }); const emailResult = await sendEmail({ emailAddress: subscription.user.email, templateParameters }); if (emailResult.success) { await updateNotificationStatus(notification.notificationId, "Sent", new Date(), undefined, emailResult.notificationId); return { status: "sent" }; } await updateNotificationStatus(notification.notificationId, "Failed", undefined, emailResult.error); return { status: "failed", error: `User ${subscription.userId}: ${emailResult.error}` }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); + await updateNotificationStatus(notification.notificationId, "Failed", undefined, errorMessage); return { status: "failed", error: `User ${subscription.userId}: ${errorMessage}` }; }
🧹 Nitpick comments (4)
libs/notifications/src/govnotify/notifications-node-client.d.ts (1)
4-4: BroadensendEmailoptions type to cover full GOV.UK Notify API.Right now the
optionsparameter only exposespersonalisation, but Notify also supportsreferenceandemailReplyToIdon this options object.(npmjs.com) This is not wrong for current usage but will block you from typing those fields later.Consider expanding the signature to more closely match the upstream API:
- sendEmail(templateId: string, emailAddress: string, options?: { personalisation?: Record<string, string> }): Promise<any>; + sendEmail( + templateId: string, + emailAddress: string, + options?: { + personalisation?: Record<string, string>; + reference?: string; + emailReplyToId?: string; + } + ): Promise<any>;Please double‑check against the exact
notifications-node-clientversion you depend on to ensure this matches its documented options shape.libs/notifications/src/govnotify/template-config.ts (1)
5-10: Variable name doesn't match the environment variable source.The constant
GOVUK_NOTIFY_TEMPLATE_IDis sourced fromGOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION(line 2), but the error message correctly references the full name. Consider renaming the constant to match the environment variable for clarity:-const GOVUK_NOTIFY_TEMPLATE_ID = process.env.GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION || ""; +const GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION = process.env.GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION || ""; export function getTemplateId(): string { - if (!GOVUK_NOTIFY_TEMPLATE_ID) { + if (!GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION) { throw new Error("GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION environment variable is not set"); } - return GOVUK_NOTIFY_TEMPLATE_ID; + return GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION; }libs/notifications/src/notification/notification-queries.test.ts (2)
18-41: Add assertion to verify the create method was called with correct arguments.The test validates the returned result but doesn't verify that
prisma.notificationAuditLog.createwas called with the expected data. This would catch regressions if the function incorrectly transforms the input.const result = await createNotificationAuditLog({ subscriptionId: "sub-1", userId: "user-1", publicationId: "pub-1" }); + expect(prisma.notificationAuditLog.create).toHaveBeenCalledWith({ + data: { + subscriptionId: "sub-1", + userId: "user-1", + publicationId: "pub-1", + status: "Pending" + } + }); expect(result.notificationId).toBe("notif-1"); expect(result.status).toBe("Pending");
43-57: Add test case with all optional parameters.The current test only exercises
sentAt. Add a test case that includeserrorMessageandgovNotifyIdto ensure all optional parameters are correctly passed through:it("should update notification status with all optional parameters", async () => { const { prisma } = await import("@hmcts/postgres"); const sentAt = new Date(); await updateNotificationStatus("notif-1", "Failed", sentAt, "Send failed", "gov-notify-123"); expect(prisma.notificationAuditLog.update).toHaveBeenCalledWith({ where: { notificationId: "notif-1" }, data: { status: "Failed", sentAt, errorMessage: "Send failed", govNotifyId: "gov-notify-123" } }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/postgres/prisma/migrations/20251202095746_remove_notification_unique_constraint/migration.sql(1 hunks)docs/tickets/VIBE-221/plan.md(1 hunks)e2e-tests/README-NOTIFICATIONS.md(1 hunks)e2e-tests/run-with-credentials.js(1 hunks)e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)e2e-tests/utils/notification-helpers.ts(1 hunks)libs/notifications/prisma/schema.prisma(1 hunks)libs/notifications/src/govnotify/govnotify-client.test.ts(1 hunks)libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)libs/notifications/src/govnotify/notifications-node-client.d.ts(1 hunks)libs/notifications/src/govnotify/template-config.ts(1 hunks)libs/notifications/src/notification/notification-queries.test.ts(1 hunks)libs/notifications/src/notification/notification-queries.ts(1 hunks)libs/notifications/src/notification/notification-service.test.ts(1 hunks)libs/notifications/src/notification/notification-service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/notifications/src/govnotify/govnotify-client.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
- e2e-tests/run-with-credentials.js
- libs/notifications/src/notification/notification-service.test.ts
- e2e-tests/README-NOTIFICATIONS.md
- libs/notifications/src/govnotify/govnotify-client.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/notifications/src/notification/notification-service.tslibs/notifications/src/notification/notification-queries.test.tslibs/notifications/src/notification/notification-queries.tslibs/notifications/src/govnotify/notifications-node-client.d.tse2e-tests/utils/notification-helpers.tslibs/notifications/src/govnotify/template-config.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/notifications/src/notification/notification-service.tslibs/notifications/src/notification/notification-queries.test.tslibs/notifications/src/notification/notification-queries.tslibs/notifications/src/govnotify/notifications-node-client.d.tse2e-tests/utils/notification-helpers.tslibs/notifications/src/govnotify/template-config.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/notifications/src/notification/notification-queries.test.ts
**/prisma/schema.prisma
📄 CodeRabbit inference engine (CLAUDE.md)
Database tables and fields MUST be singular and snake_case (e.g.,
user,case,created_at). Use Prisma@@mapand@mapfor aliases.
Files:
libs/notifications/prisma/schema.prisma
🧠 Learnings (1)
📚 Learning: 2025-11-27T09:48:13.010Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 136
File: libs/api/src/blob-ingestion/validation.ts:156-163
Timestamp: 2025-11-27T09:48:13.010Z
Learning: In libs/api/src/blob-ingestion/validation.ts, the permissive date validation in isValidISODate and isValidISODateTime functions is expected behavior and should not be flagged for stricter validation.
Applied to files:
libs/notifications/src/govnotify/template-config.ts
🧬 Code graph analysis (2)
libs/notifications/src/notification/notification-queries.test.ts (1)
libs/notifications/src/notification/notification-queries.ts (2)
createNotificationAuditLog(22-33)updateNotificationStatus(35-51)
libs/notifications/src/notification/notification-queries.ts (1)
e2e-tests/utils/notification-helpers.ts (1)
getNotificationsByPublicationId(58-63)
🪛 LanguageTool
docs/tickets/VIBE-221/plan.md
[style] ~484-~484: It’s more common nowadays to write this noun as one word.
Context: ... table with email field?) - Where is user name stored for template personalization? ...
(RECOMMENDED_COMPOUNDS)
[uncategorized] ~484-~484: Do not mix variants of the same word (‘personalization’ and ‘personalisation’) within a single text.
Context: ... Where is user name stored for template personalization? - Do we need to fetch user data fro...
(EN_WORD_COHERENCY)
🪛 markdownlint-cli2 (0.18.1)
docs/tickets/VIBE-221/plan.md
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Bare URL used
(MD034, no-bare-urls)
163-163: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
191-191: Bare URL used
(MD034, no-bare-urls)
541-541: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (24)
apps/postgres/prisma/migrations/20251202095746_remove_notification_unique_constraint/migration.sql (1)
1-2: Add documentation explaining the business rationale for removing the unique constraint.The removal of the unique index on
(user_id, publication_id)lacks context. While the current notification service creates one notification per subscription (and subscriptions enforce uniqueness on(userId, locationId)), the constraint removal suggests either a bug fix for existing duplicates or a future design change.Clarify:
- Why was the constraint needed initially, then removed one day later?
- Are there code paths outside the subscription model that create notifications?
- Does existing data contain duplicates that this migration needed to accommodate?
- Is the vague commit message "fix bug" referring to a specific issue?
Add a comment in the migration explaining the rationale to prevent future confusion.
libs/notifications/src/govnotify/notifications-node-client.d.ts (1)
1-7: Remove duplicate and incomplete ambient module declaration; update types/notifications-node-client.d.ts instead.Two conflicting
declare moduledeclarations exist for "notifications-node-client":
types/notifications-node-client.d.ts— comprehensive but missinggetNotificationById(used in e2e-tests)libs/notifications/src/govnotify/notifications-node-client.d.ts— incomplete and redundantDelete
libs/notifications/src/govnotify/notifications-node-client.d.tsand addgetNotificationByIdto the declaration intypes/notifications-node-client.d.tsto resolve the type inconsistency. Thetypes/location is the proper place for ambient module declarations (it's intypeRoots), and having both creates conflicting type information across the codebase.⛔ Skipped due to learnings
Learnt from: CR Repo: hmcts/cath-service PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-01T11:31:12.342Z Learning: Applies to libs/*/tsconfig.json : Module tsconfig.json must extend the root tsconfig.json with outDir, rootDir, and include/exclude declarations.Learnt from: CR Repo: hmcts/cath-service PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-01T11:31:12.342Z Learning: Don't create types.ts files - colocate types with the appropriate code.libs/notifications/src/govnotify/template-config.ts (4)
1-3: LGTM!The constants use appropriate defaults, with empty strings for required credentials that are validated by
getTemplateId()andgetApiKey(), and a production URL fallback forCATH_SERVICE_URL.
12-17: LGTM!Proper validation with clear error message.
19-21: LGTM!Returns the service URL with a sensible production default.
32-39: Confirm if template parameter keys are constrained by Gov.UK Notify template requirements.The interface uses mixed naming conventions (
ListTypein PascalCase,content_dateandstart_page_linkin snake_case,locationsin camelCase). These keys are passed directly aspersonalisationto the Gov.UK Notify API'ssendEmail()method, suggesting they may be required to match exact template placeholder names.If these exact names are dictated by the Gov.UK Notify template, the mixed casing is acceptable and should be documented. Otherwise, standardize to camelCase per coding guidelines.
libs/notifications/src/notification/notification-queries.test.ts (1)
4-11: LGTM!The mock properly isolates the Prisma client for unit testing.
libs/notifications/src/notification/notification-service.ts (3)
20-80: LGTM!The orchestration logic properly validates input, handles partial failures with
Promise.allSettled, and aggregates results correctly. The error handling for both fulfilled and rejected promises is comprehensive.
116-147: LGTM!The email sending logic correctly creates an audit log with "Pending" status, attempts the send, and updates to the final status ("Sent" or "Failed") based on the result. The status transitions are appropriate.
157-160: LGTM!The helper correctly handles null values and provides a sensible fallback.
libs/notifications/prisma/schema.prisma (2)
1-9: LGTM!Standard Prisma configuration with shared client output.
11-27: Unique constraint was intentionally removed—do not re-add without clarification.The schema currently lacks the
@@unique([userId, publicationId])constraint, but this is not an oversight. The constraint was created in migration20251201095418_add_notification_audit_logand then explicitly dropped in migration20251202095746_remove_notification_unique_constraint.The plan (docs/tickets/VIBE-221/plan.md lines 92-93) requires this constraint for deduplication, but the codebase has since removed it. Before re-adding the constraint to the schema, clarify why it was removed and whether that decision should be reversed or the plan should be updated.
docs/tickets/VIBE-221/plan.md (1)
1-552: Planning document - no code review required.This is a technical planning document. The static analysis hints are markdown formatting suggestions (e.g., adding language identifiers to code blocks, handling bare URLs) that don't impact functionality. The document provides valuable context for the implementation.
libs/notifications/src/notification/notification-queries.ts (4)
22-33: LGTM with enhancement suggested above.The function correctly creates an audit log entry with a sensible default status. See the previous comment about adding
errorMessagesupport.
35-51: LGTM!The function correctly updates the notification status with all optional fields.
53-57: LGTM!Appropriate query with null handling for not-found cases.
59-63: LGTM!Appropriate query for retrieving all notifications for a publication.
e2e-tests/utils/notification-helpers.ts (7)
5-17: LGTM!Interfaces follow camelCase naming convention as per coding guidelines.
19-35: LGTM!Test user creation logic is correct with proper typing and sensible defaults.
58-63: LGTM!Query logic is correct and consistent with the main implementation in
notification-queries.ts.
75-84: LGTM!Defensive filtering and early return prevent errors with empty arrays. Good practice.
86-94: LGTM!Consistent cleanup pattern with appropriate guards.
96-104: LGTM!Proper typing with no cast needed since
useris in the main Prisma client.
37-56: ThedateAddedfield has a database default and does not require an explicit value.The Subscription model in
libs/subscriptions/prisma/schema.prismadefinesdateAddedwith@default(now()), so the field will be automatically populated by the database when the record is created. The current implementation at lines 40-46 is correct. The(prisma as any)cast is acceptable for test helpers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/api/src/blob-ingestion/repository/service.ts (1)
95-116: Fire-and-forget trigger looks good; consider reusing parsed content dateThe fire-and-forget pattern with an explicit
.catchkeeps ingestion latency isolated from notification failures, which is a good trade-off here. To avoid parsingrequest.content_datemultiple times and keep publication date usage consistent, consider parsing it once earlier inprocessBlobIngestionand reusing it when triggering notifications:// earlier near artefact creation const contentDate = new Date(request.content_date); // use contentDate both for createArtefact and notifications- triggerPublicationNotifications(artefactId, request.court_id, validation.listTypeId, new Date(request.content_date)).catch((error) => { + triggerPublicationNotifications(artefactId, request.court_id, validation.listTypeId, contentDate).catch((error) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/e2e.yml(1 hunks)docs/GITHUB_SECRETS_SETUP.md(1 hunks)e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)libs/api/src/blob-ingestion/repository/service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/api/src/blob-ingestion/repository/service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/api/src/blob-ingestion/repository/service.ts
🪛 LanguageTool
docs/GITHUB_SECRETS_SETUP.md
[uncategorized] ~62-~62: The official name of this software platform is spelled with a capital “H”.
Context: ... Configuration The E2E tests workflow (.github/workflows/e2e.yml) automatically uses ...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (5)
docs/GITHUB_SECRETS_SETUP.md (3)
1-104: Well-structured and comprehensive documentation.The documentation provides clear guidance on required secrets, workflow configuration, local development setup, and troubleshooting. The tables are well-organized, and the explanations are thorough. The inclusion of environment variable mappings and notification test behavior helps users understand the full picture.
68-79: Local development credentials flow is correctly implemented.The
run-with-credentials.jsscript exists and properly implements the documented behavior. It detects CI mode viaprocess.env.CI === 'true'and conditionally loads credentials from either Azure Key Vault (local development) or relies on GitHub Secrets (CI environment). The script uses@azure/identitywithDefaultAzureCredentialfor Key Vault authentication and correctly passes loaded secrets to Playwright tests as environment variables. No discrepancies between documentation and implementation found.
97-103: Documentation accurately reflects test implementation.The test implementation in
blob-ingestion-notifications.spec.ts(line 68) correctly implements the documented behavior: the "should verify GOV.UK Notify email content" test usestest.skip(!process.env.GOVUK_NOTIFY_API_KEY, "Skipping: GOVUK_NOTIFY_API_KEY not set")to conditionally skip when the API key is not set. Other notification tests (email validation, skipped notifications for invalid emails) proceed without this API key dependency. The documentation at lines 97-103 is accurate.libs/api/src/blob-ingestion/repository/service.ts (2)
2-4: Imports align with alias/ESM conventionsUsing
@hmcts/*aliases and non-relative imports here is consistent with the workspace and ESM guidelines; no changes needed.
141-181: Verify notification result shape and avoid logging any sensitive dataThe defensive checks around
courtId,location, andlistTypeare solid, and the summary log of counts is useful. Two things to verify with the@hmcts/notificationsAPI and data contract:
- Ensure
result.errorsis always defined as an array soresult.errors.lengthcannot throw; if it can beundefined, guard with optional chaining or a default.- Confirm that
result.errorsdoes not contain PII (e.g. email addresses or subscriber identifiers); if it does, consider only logging aggregate information or sanitized error codes instead of the raw array:if (result.errors?.length) { console.error("Notification errors:", result.errors.map((e) => e.code ?? e.message)); }
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/auth/package.json (1)
32-32: Pin the @hmcts/account dependency to a specific version.The
@hmcts/accountdependency is unpinned ("*"), which violates the coding guideline that dependencies must be pinned to specific versions (except for peer dependencies). This can lead to unexpected breaking changes or inconsistent behavior across environments.Replace the wildcard with a specific version, for example:
- "@hmcts/account": "*", + "@hmcts/account": "1.0.0",Please ensure the version number matches an available release on your package registry.
libs/simple-router/package.json (1)
6-11: Add missing./configexport to align with package.json guidelines.The
exportsfield is missing the required./configexport. Per the coding guidelines, all @hmcts modules must include both main (.) and config (./config) exports with production and default conditions.Apply this diff to add the missing config export:
"exports": { ".": { "production": "./dist/index.js", "default": "./src/index.ts" } + }, + "./config": { + "production": "./dist/config.js", + "default": "./src/config.ts" }libs/cloud-native-platform/package.json (1)
6-11: Add missing./configexport to comply with module requirements.Per coding guidelines, module package.json files must include both main export (
.) and config export (./config) in the exports field with production and default conditions. The current exports field is missing the./configexport.Apply this diff to add the config export:
"exports": { ".": { "production": "./dist/index.js", "default": "./src/index.ts" } + }, + "./config": { + "production": "./dist/config.js", + "default": "./src/config.ts" + } }Note: Adjust the config paths as needed to match your actual config file locations.
🧹 Nitpick comments (3)
libs/notifications/src/govnotify/govnotify-client.ts (1)
13-22: Align boolean naming and interface placement with guidelines (optional)Two minor style nits against the stated guidelines:
SendEmailResult.successis a boolean but doesn’t use anis/has/canprefix.- Interfaces are defined near the top rather than at the bottom after functions.
If it’s still easy to change within this PR, consider:
- Renaming
successto something likeisSuccess(and updating call‑sites).- Moving
EmailResponse,SendEmailParams, andSendEmailResultbelow the function implementations.libs/api/src/blob-ingestion/repository/service.test.ts (2)
12-19: KeepmockListTypesshape aligned with production moduleThe added
mockListTypesonly exposeid,englishFriendlyName, andwelshFriendlyName. InvalidateBlobRequestthe implementation looks up bylt.name, which suggests the realmockListTypesobjects may also include anamefield. To avoid tests drifting from production behaviour (and to keep any future tests that rely onnameconsistent), consider mirroring the full structure exported by@hmcts/publicationhere, includingnameif it exists.Also applies to: 43-46
34-40: Notification tests are strong, but async contract ofgetLocationByIdand timers should be verifiedThe new notification-path tests cover success, no‑match, invalid location ID, missing location, and missing list type thoroughly, and the mocks for
@hmcts/location/@hmcts/notificationslook correct. Two things to sanity‑check:
- In
validation.tsgetLocationByIdis used synchronously, but here it’s mocked withmockResolvedValueand used as if it were async. Please confirm the real@hmcts/location.getLocationByIdis actually async; if it’s sync, switch tomockReturnValuehere to keep the contract consistent.- The
await new Promise((resolve) => setTimeout(resolve, 50));waits make the tests rely on wall‑clock timing for the fire‑and‑forget notifications. For long‑term stability it would be safer to either (a) expose a Promise from the notification helper that tests canawait, or (b) use fake timers / awaitFor‑style assertion instead of fixed sleeps.Functionally these tests look good; this is mainly about making them more robust and aligned with the real APIs.
Also applies to: 47-48, 301-391, 393-466
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
apps/api/package.json(1 hunks)apps/web/package.json(1 hunks)libs/admin-pages/package.json(1 hunks)libs/api/package.json(1 hunks)libs/api/src/blob-ingestion/repository/service.test.ts(3 hunks)libs/auth/package.json(1 hunks)libs/cloud-native-platform/package.json(1 hunks)libs/list-types/civil-and-family-daily-cause-list/package.json(1 hunks)libs/notifications/package.json(1 hunks)libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)libs/notifications/types/notifications-node-client/index.d.ts(1 hunks)libs/public-pages/package.json(1 hunks)libs/simple-router/package.json(1 hunks)libs/subscriptions/package.json(1 hunks)libs/system-admin-pages/package.json(1 hunks)libs/verified-pages/package.json(1 hunks)libs/web-core/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/notifications/package.json
🧰 Additional context used
📓 Path-based instructions (4)
libs/*/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
libs/*/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
Module package.json must include both main export (.) and config export (./config) in the exports field, with production and default conditions.
Module package.json build script must includebuild:nunjucksif the module contains Nunjucks templates in thepages/directory.
All test packages must use"test": "vitest run"script to run tests.
Files:
libs/web-core/package.jsonlibs/api/package.jsonlibs/verified-pages/package.jsonlibs/simple-router/package.jsonlibs/admin-pages/package.jsonlibs/system-admin-pages/package.jsonlibs/public-pages/package.jsonlibs/auth/package.jsonlibs/cloud-native-platform/package.jsonlibs/subscriptions/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/notifications/types/notifications-node-client/index.d.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/notifications/src/govnotify/govnotify-client.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/notifications/types/notifications-node-client/index.d.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/notifications/src/govnotify/govnotify-client.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/api/src/blob-ingestion/repository/service.test.ts
🧠 Learnings (4)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to package.json : Dependencies must be pinned to specific versions (e.g., `"express": "5.1.0"`), except for peer dependencies.
Applied to files:
libs/web-core/package.jsonlibs/api/package.jsonlibs/verified-pages/package.jsonapps/web/package.jsonapps/api/package.jsonlibs/simple-router/package.jsonlibs/admin-pages/package.jsonlibs/system-admin-pages/package.jsonlibs/public-pages/package.jsonlibs/auth/package.jsonlibs/list-types/civil-and-family-daily-cause-list/package.jsonlibs/cloud-native-platform/package.jsonlibs/subscriptions/package.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/package.json : Package names must use hmcts scope (e.g., `hmcts/auth`, `hmcts/case-management`).
Applied to files:
libs/api/package.jsonlibs/verified-pages/package.jsonlibs/system-admin-pages/package.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Use workspace aliases (`hmcts/*`) for imports instead of relative paths across packages.
Applied to files:
libs/api/package.jsonlibs/subscriptions/package.json
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/*/package.json : All test packages must use `"test": "vitest run"` script to run tests.
Applied to files:
libs/list-types/civil-and-family-daily-cause-list/package.json
🧬 Code graph analysis (1)
libs/api/src/blob-ingestion/repository/service.test.ts (3)
libs/api/src/blob-ingestion/validation.ts (1)
validateBlobRequest(9-158)libs/api/src/blob-ingestion/repository/service.ts (1)
processBlobIngestion(17-140)libs/api/src/blob-ingestion/repository/queries.ts (1)
createIngestionLog(4-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (18)
libs/verified-pages/package.json (1)
32-32: Express peer dependency alignment looks good.The update to
^5.2.0properly aligns this library with the workspace-wide Express upgrade, ensuring compatibility with the newly integrated notifications library while maintaining flexible peer dependency versioning.libs/auth/package.json (1)
27-27: Express peer dependency bump aligns with monorepo strategy.The peer dependency upgrade from
^5.1.0to^5.2.0is consistent with the repository-wide express upgrade across multiple packages (apps/api, apps/web, libs/notifications, etc.). Peer dependencies can use semantic versioning ranges, so this follows best practices.libs/list-types/civil-and-family-daily-cause-list/package.json (1)
39-39: Peer dependency version bump is appropriate.The Express peer dependency update from
^5.1.0to^5.2.0follows the learnings that peer dependencies can use caret ranges for flexibility, and is consistent with the coordinated Express upgrades across the monorepo mentioned in the PR. The caret range allows dependent packages to consume compatible minor/patch versions.Verify that Express 5.2.0 introduces no breaking changes affecting this package's Express integration (e.g., middleware patterns, routing, configuration).
libs/simple-router/package.json (1)
22-22: Express peer dependency version bump is appropriate.The upgrade from
^5.1.0to^5.2.0aligns with the broader express version bump across the monorepo (part of the notifications integration changes) and is acceptable for peer dependencies. No concerns with this change.libs/cloud-native-platform/package.json (1)
23-23: Express peerDependency version bump looks good.The minor version update from
^5.1.0to^5.2.0is appropriate for a peerDependency, and version ranges are acceptable per the learnings.libs/web-core/package.json (1)
56-56: Express 5.2.0 peer dependency update compliant.The peer dependency version constraint is correctly expressed as a caret range. Ensure that consuming packages using this peer dependency are compatible with Express 5.2.0.
libs/system-admin-pages/package.json (1)
28-28: Package structure and express peer dependency update compliant.All coding guidelines are satisfied: package name uses @hmcts scope, exports include both
.and./configwith production/default conditions, build:nunjucks script is present, test script uses vitest run, and express peer dependency is properly versioned.apps/api/package.json (1)
24-24: Express dependency properly pinned to specific version.The dependency is correctly pinned to 5.2.0 following the guideline for regular (non-peer) dependencies.
libs/public-pages/package.json (1)
26-26: Package structure and express peer dependency update compliant.All coding guidelines are satisfied: package name uses @hmcts scope, exports include both
.and./configwith production/default conditions, build:nunjucks script is present, test script uses vitest run, and express peer dependency is properly versioned.libs/admin-pages/package.json (1)
30-30: Package structure and express peer dependency update compliant.All coding guidelines are satisfied: package name uses @hmcts scope, exports include both
.and./configwith production/default conditions, build:nunjucks script is present, test script uses vitest run, and express peer dependency is properly versioned.libs/subscriptions/package.json (1)
30-30: Package structure and express peer dependency update compliant.All coding guidelines are satisfied: package name uses @hmcts scope, exports include both
.and./configwith production/default conditions, test script uses vitest run, and express peer dependency is properly versioned.libs/api/package.json (1)
2-2: No action required. The package.json file atlibs/api/package.jsonfully complies with coding guidelines: the package name correctly uses the@hmctsscope, exports include both main and config exports with proper production and default conditions, and the test script is configured correctly. The directory name (api) differs from the package name (blob-ingestion), which is a standard monorepo pattern and not a concern.libs/notifications/types/notifications-node-client/index.d.ts (3)
30-34: LGTM! NotifyClient class structure is well-defined.The class declaration follows coding guidelines with appropriate naming conventions and method signatures that return promises.
24-28: No issues found. The interface correctly uses the British spelling "personalisation" to match the Gov.UK Notify API and the official notifications-node-client library specifications.
2-16: No action required. Thefrom_emailproperty correctly reflects the actual notifications-node-client library API response structure and is appropriately named in snake_case to match the JSON payload from Gov.Notify. Ambient type declarations for third-party libraries must mirror the external API exactly, making this an acceptable exception to camelCase naming conventions.libs/notifications/src/govnotify/govnotify-client.ts (1)
24-35: sendEmail wrapper structure looks goodThe public
sendEmailAPI cleanly wrapssendEmailInternalwithretryWithBackoffand normalises failures into aSendEmailResultinstead of throwing, which is a nice, predictable contract for callers.apps/web/package.json (1)
38-38: Remove or correct invalid express version 5.2.0.Express version 5.2.0 does not exist in the npm registry; the latest express 5.x release is 5.1.0. This invalid version will cause
npm installto fail. Change to"5.1.0"or the intended version.⛔ Skipped due to learnings
Learnt from: CR Repo: hmcts/cath-service PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-01T11:31:12.342Z Learning: Applies to package.json : Dependencies must be pinned to specific versions (e.g., `"express": "5.1.0"`), except for peer dependencies.libs/api/src/blob-ingestion/repository/service.test.ts (1)
220-299: Good coverage of validation logging and system‑error edge casesThe new tests for missing
provenance/court_id, missinglistTypeIdafter a “valid” validation result, and non‑Errorrejections fromcreateArtefactaccurately mirror the behaviour inprocessBlobIngestionand its error logging. They ensure we don’t regress logging ofUNKNOWNprovenance/court IDs and that odd failure modes are captured asSYSTEM_ERRORwith the expected messages.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/api/src/blob-ingestion/repository/service.test.ts (1)
508-511: Update test expectation to align with sanitized error logging.This test expects email addresses to be logged in the errors array, which validates PII logging behavior. When the production code is updated to sanitize error logs (per the suggestion in
service.ts), update this expectation accordingly.After applying the fix in
service.ts, update this expectation:- expect(consoleErrorSpy).toHaveBeenCalledWith("Notification errors:", [ - { email: "[email protected]", error: "Invalid email" }, - { email: "[email protected]", error: "Service unavailable" } - ]); + expect(consoleErrorSpy).toHaveBeenCalledWith("Notification errors:", { + count: 2, + errorMessages: ["Invalid email", "Service unavailable"] + });
🧹 Nitpick comments (2)
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (1)
707-736: Verify if double mock is necessary.Line 713-714 chains two
mockResolvedValueOncecalls, but the test only calls the POST handler once. Other error tests in this file (e.g., lines 504-534, 536-564) only mockgetManualUploadonce.Consider simplifying to a single mock unless the POST handler genuinely calls
getManualUploadtwice during error rendering:- vi.mocked(getManualUpload).mockResolvedValueOnce(invalidDateUploadData).mockResolvedValueOnce(invalidDateUploadData); + vi.mocked(getManualUpload).mockResolvedValue(invalidDateUploadData);libs/api/src/blob-ingestion/repository/service.test.ts (1)
325-326: Consider usingvi.waitForfor more robust async assertions.Using
setTimeoutfor waiting on fire-and-forget operations can lead to flaky tests. Vitest'svi.waitFororwaitForfrom testing utilities provides a more reliable approach.Example pattern:
await vi.waitFor(() => { expect(sendPublicationNotifications).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts(2 hunks)libs/api/src/blob-ingestion/repository/service.test.ts(3 hunks)libs/api/src/blob-ingestion/repository/service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/api/src/blob-ingestion/repository/service.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/api/src/blob-ingestion/repository/service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/api/src/blob-ingestion/repository/service.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/api/src/blob-ingestion/repository/service.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/api/src/blob-ingestion/repository/service.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
🧠 Learnings (1)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Never put sensitive data in logs.
Applied to files:
libs/api/src/blob-ingestion/repository/service.test.ts
🧬 Code graph analysis (2)
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (5)
libs/api/src/blob-ingestion/file-storage.ts (1)
saveUploadedFile(12-23)libs/admin-pages/src/manual-upload/file-storage.ts (1)
saveUploadedFile(12-23)libs/publication/src/index.ts (1)
createArtefact(6-6)libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
POST(212-212)libs/location/src/index.ts (1)
getLocationById(17-17)
libs/api/src/blob-ingestion/repository/service.ts (2)
libs/notifications/src/notification/notification-service.ts (1)
sendPublicationNotifications(20-80)libs/notifications/src/index.ts (1)
sendPublicationNotifications(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (10)
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (3)
75-80: LGTM: Notification mock and imports properly configured.The mock setup and imports follow the established pattern in the test file and properly support the new notification integration tests.
566-674: Excellent test coverage for fire-and-forget notification pattern.These tests properly verify that:
- Notifications are sent with correct parameters on successful upload
- Upload succeeds even when location lookup fails for notifications
- Upload succeeds even when notification sending fails
- Appropriate warnings/errors are logged for troubleshooting
The resilience testing ensures the main upload flow is not blocked by notification failures, which is the correct behavior for a fire-and-forget pattern.
676-705: LGTM: Comprehensive edge case and i18n coverage.These tests properly verify:
- Welsh locale preservation in redirects (coding guideline compliance for i18n)
- isFlatFile determination for JSON vs non-JSON files
- Graceful handling of missing fileName (defensive programming)
The tests follow established patterns and provide good coverage of edge cases.
Also applies to: 738-851
libs/api/src/blob-ingestion/repository/service.ts (3)
1-8: LGTM!Imports are well-organized with workspace aliases (
@hmcts/*) for cross-package imports and.jsextensions for relative imports, following the coding guidelines.
95-116: LGTM!The fire-and-forget pattern is correctly implemented. The notification trigger runs asynchronously without blocking the ingestion response, and errors are caught and logged without affecting the main flow. The conditional skip for
noMatch=trueis appropriate.
155-159: The identifiermockListTypesis production reference data, not mock/test data. It's a shared catalog of list types exported from@hmcts/list-types-commonand used consistently across multiple production modules (validation, page handlers, notifications) to map list type IDs to bilingual display names. The usage in this file is appropriate.Likely an incorrect or invalid review comment.
libs/api/src/blob-ingestion/repository/service.test.ts (4)
6-20: LGTM!Mock setup is comprehensive and correctly structured. The
mockListTypesarray provides test data that aligns with the production mock data used in the service.
34-40: LGTM!New mock registrations for
@hmcts/locationand@hmcts/notificationsfollow the established pattern and are correctly placed.
392-465: LGTM!Excellent edge case coverage for the
triggerPublicationNotificationshelper. Tests properly verify handling of invalid location IDs, missing locations, and missing list types. The console spy cleanup pattern is consistently applied.
220-299: LGTM!Solid test coverage for edge cases including missing provenance/court_id fallbacks, missing list type ID after validation, and non-Error exception handling. These tests validate the defensive coding in the service implementation.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (1)
87-93: Add defensive checks before accessing notification data.The code assumes
notifications[0]exists and has a validgovNotifyId, but if notification processing hasn't completed or failed, this will throw an error rather than providing a clear test failure message.Add assertions before accessing nested properties:
const notifications = await getNotificationsByPublicationId(result.artefact_id); + expect(notifications).toHaveLength(1); + expect(notifications[0].govNotifyId).toBeTruthy(); + const govNotifyEmail = await getGovNotifyEmail(notifications[0].govNotifyId); + expect(govNotifyEmail).toBeTruthy(); expect(govNotifyEmail.email_address).toBe(process.env.CFT_VALID_TEST_ACCOUNT!);Based on learnings, this issue was previously flagged in past reviews.
🧹 Nitpick comments (2)
e2e-tests/tests/manual-upload.spec.ts (1)
839-839: Replace hard-coded delays with polling for notification processing.The fixed 2-second delays are arbitrary and don't verify that notification processing has completed. This can lead to flaky tests (too short) or unnecessarily slow tests (too long).
Once you add notification verification (as suggested in the previous comment), replace the fixed delay with retry/polling logic similar to the blob-ingestion tests (lines 114-118 in
blob-ingestion-notifications.spec.ts):- await new Promise((resolve) => setTimeout(resolve, 2000)); + // Wait for async notification processing with retry logic + let notifications = []; + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => setTimeout(resolve, 1000)); + notifications = await getNotificationsByPublicationId(publicationId); + if (notifications.length > 0) break; + }Better yet, extract this polling logic into a shared helper function in
notification-helpers.tsto avoid duplication across test files.Also applies to: 883-883
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (1)
85-85: Use consistent wait times or replace with polling logic.The first test uses a 2-second delay while the third test uses 1 second. These arbitrary fixed delays should either be consistent or replaced with the polling logic used in tests 2 and 4.
For consistency and reliability, apply the polling helper (suggested in the previous comment) to all tests:
- await new Promise((resolve) => setTimeout(resolve, 2000)); - const notifications = await getNotificationsByPublicationId(result.artefact_id); + // Or use the polling helper: + const notifications = await waitForNotifications(result.artefact_id);Also applies to: 138-138
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)e2e-tests/tests/manual-upload.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/manual-upload.spec.tse2e-tests/tests/api/blob-ingestion-notifications.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/manual-upload.spec.tse2e-tests/tests/api/blob-ingestion-notifications.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/manual-upload.spec.tse2e-tests/tests/api/blob-ingestion-notifications.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/manual-upload.spec.ts (1)
e2e-tests/utils/notification-helpers.ts (4)
cleanupTestSubscriptions(86-94)cleanupTestUsers(96-104)createTestUser(19-35)createTestSubscription(37-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (4)
68-70: Verify both required environment variables before running the test.The test only checks
GOVUK_NOTIFY_API_KEYbut also requiresCFT_VALID_TEST_ACCOUNTon line 70 with a non-null assertion that will throw if unset.
87-93: Add defensive checks before accessing notification properties.If
notifications[0]is undefined orgovNotifyIdis null, the subsequent calls will fail without clear error messages.
113-118: Extract duplicate polling logic into a shared helper.This retry/polling pattern is duplicated across multiple tests and violates DRY principles.
162-167: Extract duplicate polling logic into a shared helper.This is the same retry/polling pattern duplicated from lines 113-118.
🧹 Nitpick comments (1)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (1)
138-138: Use retry logic instead of fixed delay for reliability.Like the first test, this uses a fixed 1-second delay. If notification processing is slow or the system is under load, this could lead to false negatives where notifications exist but haven't been processed yet.
Consider using the retry pattern from tests 2 and 4:
- await new Promise((resolve) => setTimeout(resolve, 1000)); - - const notifications = await getNotificationsByPublicationId(result.artefact_id); + // Wait for async notification processing (with retry logic) + let notifications = []; + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => setTimeout(resolve, 1000)); + notifications = await getNotificationsByPublicationId(result.artefact_id); + // Break early if we're sure no notifications will be created + if (i > 5) break; + } + expect(notifications).toHaveLength(0);Or better yet, implement the shared helper function suggested for lines 113-118 and use it consistently across all tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.claude/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (1)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e-tests/utils/notification-helpers.ts (1)
40-46: Avoid(prisma as any)to retain Prisma type‑safety.Casting
prismatoanyforsubscriptionandnotificationAuditLogoperations removes compile‑time checks and can hide schema mismatches (as seen with the earliercreatedAtvsdateAddedissue). You can call the typed client directly and let Prisma enforce the correct field names and shapes.For example:
- await (prisma as any).subscription.create({ + await prisma.subscription.create({ data: { subscriptionId, userId, - locationId + locationId + // If `dateAdded` is a required field without a default, also include: + // dateAdded: new Date(), } });And similarly for the other calls:
- return await (prisma as any).notificationAuditLog.findMany({ + return await prisma.notificationAuditLog.findMany({ - await (prisma as any).notificationAuditLog.deleteMany({ + await prisma.notificationAuditLog.deleteMany({ - await (prisma as any).subscription.deleteMany({ + await prisma.subscription.deleteMany({This keeps the helpers aligned with the Prisma schema and lets TS catch future model changes early.
Also applies to: 59-62, 79-83, 90-101
🧹 Nitpick comments (2)
e2e-tests/utils/notification-helpers.ts (2)
5-17: Optional: move interfaces to bottom of the module for consistency.Coding guidelines say interfaces/types should live at the bottom after exported functions; consider moving
TestSubscriptionandTestUserbelow the helpers to match that pattern.
58-63: Consider reusing the shared notification query helper.There’s already a
getNotificationsByPublicationIdinlibs/notifications/src/notification/notification-queries.ts; reusing that (via the@hmcts/*alias) instead of an ad‑hoc Prisma call here would reduce duplication and keep behaviour (e.g., filters/order) in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)e2e-tests/utils/notification-helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/utils/notification-helpers.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/utils/notification-helpers.ts
🧬 Code graph analysis (1)
e2e-tests/utils/notification-helpers.ts (1)
libs/notifications/src/notification/notification-queries.ts (1)
getNotificationsByPublicationId(59-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (3)
e2e-tests/utils/notification-helpers.ts (3)
19-35: createTestUser helper looks sound for e2e usage.User creation logic (UUID id, provenance fields, fixed role) is simple and aligned with test helper intent; returning the basic shape is sufficient.
65-73: GovNotify helper is straightforward and fails fast on missing API key.The env check and direct
NotifyClientcall are appropriate for e2e tests; as long asGOVUK_NOTIFY_API_KEYis wired in the test environment, this looks fine.
104-112: User cleanup helper is clear and safe.Guarding on empty
userIdsand using a singledeleteManyonuserId in [...]is a clean approach for test teardown.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e-tests/utils/notification-helpers.ts (1)
37-56: Avoid(prisma as any)and tighten notification helper typingsThese helpers lean heavily on Prisma models, but the
(prisma as any)casts forsubscriptionandnotificationAuditLogand theany[]return types mean schema/field mistakes will only surface at runtime.You already use strongly‑typed Prisma access elsewhere (e.g.
prisma.notificationAuditLog.findMany(...)inlibs/notifications/src/notification/notification-queries.ts), so it would be safer and more consistent to:
- Drop the
as anycasts and call typed models directly:- await (prisma as any).subscription.create({
- await prisma.subscription.create({
data: {
subscriptionId,
userId,
locationId
}
});and similarly for all `notificationAuditLog` usages and `subscription.deleteMany`. - Optionally, return a concrete notification type instead of `any[]` from the `get*Notifications` / `waitForNotifications` helpers (e.g. reuse the same `NotificationAuditLog` type you use in your notifications library) so tests can rely on fields like `status`, `govNotifyId`, and `publicationId` with type safety. This keeps the test utilities aligned with your schema and makes refactors (like field renames) fail fast at compile time rather than in CI runs. Also applies to: 58-63, 79-84, 96-105, 107-123 </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (7)</summary><blockquote> <details> <summary>libs/notifications/src/notification/notification-queries.ts (1)</summary><blockquote> `61-65`: **Consider adding chronological ordering for predictability.** For audit logs, returning notifications in creation order is typically expected. The e2e test helper (`e2e-tests/utils/notification-helpers.ts` lines 57-62) includes `orderBy: { createdAt: "asc" }` for consistency. Apply this diff to add ordering: ```diff export async function getNotificationsByPublicationId(publicationId: string): Promise<NotificationAuditLog[]> { return await prisma.notificationAuditLog.findMany({ - where: { publicationId } + where: { publicationId }, + orderBy: { createdAt: "asc" } }); }libs/api/src/blob-ingestion/repository/service.ts (1)
155-159: Consider renamingmockListTypesto clarify it contains production reference data, not test data.The
mockListTypesconstant in@hmcts/list-types-commoncontains actual production reference data for court list types (Civil Daily Cause List, Family Daily Cause List, Crime Daily List, Magistrates Public List, Crown Warned List), not mock data. The "mock" prefix is misleading—rename tolistTypesDataorlistTypesReferenceto better reflect its purpose. This would require updates across multiple packages (blob-ingestion, admin-pages, public-pages) where it's currently imported and used.libs/api/src/middleware/oauth-middleware.ts (1)
59-72: Config/env resolution for AZURE_API_CLIENT_ID looks sound; consider per-key fallback as an optional improvementThe switch to
AZURE_API_CLIENT_IDwithAZURE_API_CLIENT_ID || AZURE_CLIENT_IDenv fallback and the updated error message are consistent and keep local dev working while enforcing the new config key.One nuance: wrapping both
config.getcalls in a singletrymeans a missingAZURE_API_CLIENT_IDin Key Vault will also discard a validAZURE_TENANT_IDfrom config and force both to come from env. If you ever expect mixed configuration (e.g. tenant in config, client ID via env), you might optionally split the lookups and fall back per-key.libs/api/src/middleware/oauth-middleware.test.ts (1)
94-200: Update remaining config mocks fromAZURE_CLIENT_IDtoAZURE_API_CLIENT_IDto exercise intended test branchesThe tests in this section correctly mock
AZURE_API_CLIENT_ID, ensuring the happy-path and "missing role" scenarios reach their intended token validation logic. However, five additional tests still mock:if (key === "AZURE_CLIENT_ID") return "test-client-id";Since the middleware now calls
config.get("AZURE_API_CLIENT_ID")(line 61 of oauth-middleware.ts), these tests never reach their intended branches. TheclientIdbecomes undefined,validateTokenthrows a config error, and the middleware's catch-all error handler returns 401 with "Invalid or expired token"—masking the fact that tests for token format validation, missingkid, JWKS failures, and JWT verification errors are no longer exercising those specific code paths.Update the mocks at lines 269, 297, 329, 370, and 418 to use
"AZURE_API_CLIENT_ID"so each test covers its intended failure scenario.e2e-tests/tests/manual-upload.spec.ts (2)
783-806: SharedtestData+ nestedbeforeEachmay bite if tests go parallelThe shared mutable
testDataobject and innerbeforeEachwork as long as tests in this file run serially. IffullyParallelis ever enabled for the file, concurrent tests would race ontestDataand cleanup calls, and the doubleauthenticateSystemAdmin(outerbeforeEachat Line 69 and inner one here) will run twice per test.Consider:
- Marking this describe as serial (
test.describe.serial(...)) or switching to per‑test fixtures instead of sharedtestData.- Dropping the inner
beforeEachand relying on the outer one for authentication to avoid redundant navigation.
808-860: Use polling instead of fixed 2‑second sleeps for notification assertionsBoth notification tests rely on a hardcoded
setTimeout(…, 2000)before querying for notifications. If notification processing is slower under load, this will cause intermittent flakes even though the system is behaving correctly.Since you already have DB helpers in
notification-helpers.ts, consider introducing a small polling helper (e.g.waitForSubscriptionNotifications(subscriptionId, maxRetries, delayMs)) that repeatedly callsgetNotificationsBySubscriptionIduntil records appear or a timeout is hit, and then use it in both tests instead of the fixed sleep.For example inside these tests:
- await new Promise((resolve) => setTimeout(resolve, 2000)); - - // Verify notifications were created - const myNotifications = await getNotificationsBySubscriptionId(subscription.subscriptionId); + const myNotifications = await waitForSubscriptionNotifications(subscription.subscriptionId);(with
waitForSubscriptionNotificationsimplemented alongside your other helpers). This keeps the tests fast when notifications are quick and more resilient when they are slow.Also applies to: 900-923
types/notifications-node-client.d.ts (1)
4-29:sendEmailsignature and response typing look consistent and practicalThe options and response body types for
sendEmailare coherent and give good type-safety for common fields while still allowing the caller to pass structured personalisation. This looks fine as-is; if these shapes start being reused elsewhere, you could later extract sharedTemplate/Contentinterfaces to reduce duplication, but that’s not necessary right now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)e2e-tests/tests/manual-upload.spec.ts(2 hunks)e2e-tests/utils/notification-helpers.ts(1 hunks)libs/api/src/blob-ingestion/repository/service.ts(3 hunks)libs/api/src/middleware/oauth-middleware.test.ts(4 hunks)libs/api/src/middleware/oauth-middleware.ts(1 hunks)libs/notifications/src/notification/notification-queries.ts(1 hunks)types/notifications-node-client.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/api/src/middleware/oauth-middleware.tslibs/api/src/middleware/oauth-middleware.test.tstypes/notifications-node-client.d.tse2e-tests/tests/manual-upload.spec.tslibs/api/src/blob-ingestion/repository/service.tslibs/notifications/src/notification/notification-queries.tse2e-tests/utils/notification-helpers.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/api/src/middleware/oauth-middleware.tslibs/api/src/middleware/oauth-middleware.test.tstypes/notifications-node-client.d.tse2e-tests/tests/manual-upload.spec.tslibs/api/src/blob-ingestion/repository/service.tslibs/notifications/src/notification/notification-queries.tse2e-tests/utils/notification-helpers.ts
libs/**/*-middleware.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Reusable middleware must be placed in a dedicated
libs/[module]/src/[middleware-name]-middleware.tsfile and exported as a function.
Files:
libs/api/src/middleware/oauth-middleware.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/api/src/middleware/oauth-middleware.test.tse2e-tests/tests/manual-upload.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Never put sensitive data in logs.
Applied to files:
libs/api/src/blob-ingestion/repository/service.ts
🧬 Code graph analysis (4)
e2e-tests/tests/manual-upload.spec.ts (1)
e2e-tests/utils/notification-helpers.ts (6)
cleanupTestNotifications(96-105)cleanupTestSubscriptions(107-123)cleanupTestUsers(125-133)createTestUser(19-35)createTestSubscription(37-56)getNotificationsBySubscriptionId(79-84)
libs/api/src/blob-ingestion/repository/service.ts (2)
libs/notifications/src/notification/notification-service.ts (1)
sendPublicationNotifications(20-80)libs/notifications/src/index.ts (1)
sendPublicationNotifications(1-1)
libs/notifications/src/notification/notification-queries.ts (1)
e2e-tests/utils/notification-helpers.ts (1)
getNotificationsByPublicationId(58-63)
e2e-tests/utils/notification-helpers.ts (2)
libs/notifications/src/notification/notification-queries.ts (1)
getNotificationsByPublicationId(61-65)types/notifications-node-client.d.ts (1)
NotifyClient(2-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (12)
libs/notifications/src/notification/notification-queries.ts (5)
3-9: LGTM! Past feedback addressed.The addition of the optional
errorMessagefield enables efficient single-write audit log creation, eliminating the create-then-update pattern.
11-21: LGTM!The interface correctly represents the Prisma model with appropriate nullable types for optional fields.
23-35: LGTM!The create function correctly defaults status to "Pending" and persists the optional errorMessage, enabling efficient single-write audit log creation.
37-53: LGTM!The update function correctly handles optional parameters. Prisma ignores undefined values in updates, making this pattern safe and idiomatic.
55-59: LGTM!The query correctly retrieves a notification by Gov.UK Notify ID using a parameterized Prisma query.
libs/api/src/blob-ingestion/repository/service.ts (3)
2-4: LGTM - imports follow guidelines.The workspace aliases are correctly used per coding guidelines, and the imports are properly structured.
95-116: Fire-and-forget pattern appropriately implemented.The notification trigger correctly uses a fire-and-forget pattern with
.catch(), ensuring that notification failures don't block the ingestion flow. The logging contains only non-PII identifiers.
177-181: Good improvement - PII sanitization now in place.The email sanitization regex addresses the previous review concern about logging PII. The pattern redacts email addresses while preserving error counts and non-PII diagnostics for debugging.
Based on learnings: "Never put sensitive data in logs."
libs/api/src/middleware/oauth-middleware.test.ts (1)
202-259: Env‑fallback test correctly aligned with AZURE_API_CLIENT_IDThe env‑fallback scenario now matches the implementation:
config.getthrows, driving the catch‑block path.process.env.AZURE_TENANT_IDandprocess.env.AZURE_API_CLIENT_IDare set and then cleaned up.- This exercises the
AZURE_API_CLIENT_ID || AZURE_CLIENT_IDlogic while keeping test isolation intact.Looks good.
e2e-tests/tests/manual-upload.spec.ts (2)
5-13: Notification helpers import is well‑factoredReusing the shared notification helpers here keeps user/subscription/notification setup and cleanup consistent across E2E tests, and the
.jsextension on the relative import matches the nodenext module resolution guidelines.
849-859: Notification assertions and cleanup look robustThe tests now assert on real notification records (status
"Sent"and non‑emptygovNotifyId) and verify that multiple subscribers share the samepublicationId, while also tracking thatpublicationIdfor cleanup viacleanupTestNotifications. This closes the previous gap where only the UI success panel was checked.Also applies to: 905-923
types/notifications-node-client.d.ts (1)
30-52: No action needed. The type declaration correctly reflects how this CJS package is used in your ESM codebase. WithesModuleInterop: trueandmodule: nodenextin your tsconfig, TypeScript synthesizes a named export from the package's default export, makingimport { NotifyClient }both type-safe and runtime-safe. The dual export in the.d.ts(named class + default object) appropriately documents both the intended ESM import pattern and the underlying CJS structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
libs/api/src/blob-ingestion/repository/service.test.ts (1)
301-465: Notification-flow tests thoroughly cover success/skip/error pathsThe notification tests exercise: successful send with location match, no-op when
noMatchis true, failure of the notification service while keeping ingestion successful, invalidcourt_id, missing location, and unknownlistTypeId. This gives good confidence that the fire‑and‑forget path won’t break ingestion and that diagnostics are emitted in the right scenarios. If you find the repeatedawait new Promise((resolve) => setTimeout(resolve, 50));noisy over time, you could factor it into a small helper (e.g.await waitForAsyncNotifications()) or move to fake timers to make the intent clearer, but that’s optional.libs/api/src/blob-ingestion/repository/service.ts (1)
142-185: Defensive notification helper and PII-safe logging align with guidelines
triggerPublicationNotificationsdefensively bails out (with clear logs) on invalidcourtId, missing location, or unknownlistTypeId, and then logs a concise summary of notification outcomes. The error logging path builds sanitized strings and uses a regex to redact email addresses before logging, so subscriber emails never hit logs while still preserving useful diagnostics like counts and messages; this is in line with the “never put sensitive data in logs” guidance. If you want to further tidy things up, you could extract the email-redaction regex into a named constant or utility for reuse across the codebase, but that’s purely a nicety.e2e-tests/utils/notification-helpers.ts (3)
59-64: Add explicit return type annotation.The function returns an implicit
anytype. For better type safety and developer experience, add an explicit return type (e.g.,Promise<NotificationAuditLog[]>or at minimum document the return shape).
66-78: Add explicit return type annotation.Similar to
getNotificationsByPublicationId, this function returnsany[]implicitly. Consider adding an explicit return type for better type safety.
108-124: Remove unnecessary cast for subscription operations.Line 119 uses
(prisma as any).subscription.deleteMany, but line 40 usesprisma.subscription.createwithout a cast. Sincesubscriptionis accessible on the typed Prisma client for create operations, it should also work for delete operations. The cast bypasses type safety unnecessarily.Apply this diff to remove the cast:
- await (prisma as any).subscription.deleteMany({ + await prisma.subscription.deleteMany({ where: { subscriptionId: { in: subscriptionIds } } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e-tests/utils/notification-helpers.ts(1 hunks)libs/api/src/blob-ingestion/repository/service.test.ts(3 hunks)libs/api/src/blob-ingestion/repository/service.ts(3 hunks)libs/api/src/middleware/oauth-middleware.test.ts(9 hunks)libs/notifications/src/govnotify/govnotify-client.test.ts(1 hunks)libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)libs/notifications/types/notifications-node-client/index.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/notifications/src/govnotify/govnotify-client.test.ts
- libs/api/src/middleware/oauth-middleware.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/api/src/blob-ingestion/repository/service.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/notifications/types/notifications-node-client/index.d.tslibs/notifications/src/govnotify/govnotify-client.tse2e-tests/utils/notification-helpers.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/api/src/blob-ingestion/repository/service.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/notifications/types/notifications-node-client/index.d.tslibs/notifications/src/govnotify/govnotify-client.tse2e-tests/utils/notification-helpers.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/api/src/blob-ingestion/repository/service.test.ts
🧠 Learnings (1)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Never put sensitive data in logs.
Applied to files:
libs/api/src/blob-ingestion/repository/service.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/notifications/src/govnotify/govnotify-client.ts
🧬 Code graph analysis (3)
libs/api/src/blob-ingestion/repository/service.ts (1)
libs/notifications/src/notification/notification-service.ts (1)
sendPublicationNotifications(20-80)
libs/api/src/blob-ingestion/repository/service.test.ts (3)
libs/api/src/blob-ingestion/validation.ts (1)
validateBlobRequest(9-158)libs/api/src/blob-ingestion/repository/service.ts (1)
processBlobIngestion(17-140)libs/api/src/blob-ingestion/repository/queries.ts (1)
createIngestionLog(4-16)
e2e-tests/utils/notification-helpers.ts (3)
libs/notifications/src/notification/notification-queries.ts (1)
getNotificationsByPublicationId(61-65)libs/notifications/types/notifications-node-client/index.d.ts (1)
NotifyClient(45-49)types/notifications-node-client.d.ts (1)
NotifyClient(2-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (13)
libs/api/src/blob-ingestion/repository/service.test.ts (4)
12-19: Publication Provenance and list-type mocks look consistentExtending the Provenance mock with SJP/SNL/COMMON_PLATFORM and adding a minimal
mockListTypesfixture gives the tests just enough surface to exercise the new notification logic and keep expectations readable; this looks consistent with how the service uses these values.
34-40: Location and notifications mocks are wired correctly for dynamic importsMocking
@hmcts/locationand@hmcts/notificationsat the module level and then destructuring them viaawait import(...)inside thedescribekeeps the tests aligned with the production import pattern and makesvi.mocked(...)usage type-safe and predictable.Also applies to: 42-48
220-299: Good coverage for validation and system-error edge casesThe new tests for missing
provenance, missingcourt_id, absentlistTypeIdafter a “successful” validation, and non-Errorexceptions during artefact creation align tightly with the service behavior and give solid regression protection for these failure paths.
467-515: PII-safe notification error logging is verified correctlyThe final test asserts that the notification summary log omits per-user details and that error logs contain redacted email addresses via the
[REDACTED_EMAIL]placeholder, matching the sanitization behavior in the service. This directly enforces the “no sensitive data in logs” guideline at test level, which is a nice improvement over earlier behavior. Based on learnings, this now aligns with the privacy logging rule.libs/api/src/blob-ingestion/repository/service.ts (1)
2-4: Non-blocking notification trigger is correctly isolated from ingestion successImporting
getLocationById/sendPublicationNotificationsand then invokingtriggerPublicationNotificationsonly whennoMatchis false, without awaiting it, keeps the main ingestion response path independent of notification latency or failures. The attached.catchensures any rejection from the helper is logged with artefact and court context but doesn’t leak raw error objects or affect the ingestion result.Also applies to: 95-117
libs/notifications/src/govnotify/govnotify-client.ts (1)
19-30: LGTM! Retry logic is now correct.The refactored implementation properly enables retries:
sendEmailInternalnow throws on failure (line 73), allowingretryWithBackoffto retry the operation. The outer try-catch converts the final thrown error into a{ success: false, error }result for the caller.libs/notifications/types/notifications-node-client/index.d.ts (3)
2-16: LGTM!The
NotificationBodyinterface correctly models the GOV.UK Notify email response structure.
18-20: LGTM! EmailResponse interface is now correct.The interface correctly defines the GOV.UK Notify response structure with a single required
bodyproperty, addressing the previous issue with redundant and incorrect properties.
45-49: LGTM!The
NotifyClientclass definition correctly includes all necessary methods includinggetNotificationById, which addresses the previous concern.e2e-tests/utils/notification-helpers.ts (4)
1-17: LGTM!Imports follow the coding guidelines, using workspace aliases (
@hmcts/*) and proper module resolution. Interface definitions use correct PascalCase naming.
19-35: LGTM!The function correctly creates a test user with proper type safety using the typed Prisma client.
37-57: LGTM! Field name corrected.The function now correctly uses
dateAddedfield and maintains type safety without requiring a cast, addressing the previous review concern.
87-95: LGTM!The function correctly uses the
getNotificationByIdmethod (now properly defined in the type definitions) and includes appropriate validation for the required API key.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libs/notifications/src/govnotify/govnotify-client.ts (2)
4-5: Guard against invalid retry configuration to avoid unbounded retriesIf
NOTIFICATION_RETRY_ATTEMPTSorNOTIFICATION_RETRY_DELAY_MSare set to non‑numeric or negative values,retries/delaycan becomeNaNor stay negative. With the currentif (retries === 0)check, this means the base case is never hit and you can end up with effectively unbounded retries / recursion with 0ms delay.It’s safer to sanitise the env values and tighten the stop condition, e.g.:
-const NOTIFICATION_RETRY_ATTEMPTS = Number.parseInt(process.env.NOTIFICATION_RETRY_ATTEMPTS || "1", 10); -const NOTIFICATION_RETRY_DELAY_MS = Number.parseInt(process.env.NOTIFICATION_RETRY_DELAY_MS || "1000", 10); +const NOTIFICATION_RETRY_ATTEMPTS = (() => { + const parsed = Number.parseInt(process.env.NOTIFICATION_RETRY_ATTEMPTS ?? "", 10); + return Number.isNaN(parsed) || parsed < 0 ? 1 : parsed; +})(); + +const NOTIFICATION_RETRY_DELAY_MS = (() => { + const parsed = Number.parseInt(process.env.NOTIFICATION_RETRY_DELAY_MS ?? "", 10); + return Number.isNaN(parsed) || parsed < 0 ? 1000 : parsed; +})(); @@ - } catch (error) { - if (retries === 0) { + } catch (error) { + if (retries <= 0) { throw error; }Also applies to: 96-106
55-60: Strip PII and raw payloads from logs to comply with logging guidelinesCurrent logging includes
emailAddress, fulltemplateParameters, anderror.response?.data/response.body, which can contain PII and other sensitive data. This conflicts with the “never put sensitive data in logs” guideline for TypeScript files. Consider logging only non‑identifying metadata and avoiding raw Notify payloads, e.g.:- console.log("[govnotify-client] Sending email:", { - templateId, - emailAddress: params.emailAddress, - templateParameters: params.templateParameters - }); + console.log("[govnotify-client] Sending GOV.UK Notify email", { + templateId + }); @@ - console.log("[govnotify-client] Response keys:", Object.keys(response || {})); - console.log("[govnotify-client] Response.body:", response?.body); + console.log("[govnotify-client] GOV.UK Notify sendEmail call succeeded"); @@ - console.error("[govnotify-client] Error details:", { - message: error.message, - response: error.response?.data, - status: error.response?.status - }); + console.error("[govnotify-client] Error when sending GOV.UK Notify email", { + message: error?.message, + status: error?.response?.status + });You can then optionally log
notificationIdin a separate success log if that’s acceptable. As per coding guidelines / learnings, this keeps logs free of email addresses and template personalisation.Also applies to: 66-68, 72-75, 77-88, 90-92
🧹 Nitpick comments (1)
libs/notifications/src/govnotify/govnotify-client.ts (1)
7-25: Clean up unused local types and align interface placement with module-order guideline
NotificationBodyandEmailResponseare declared but not referenced in this file, and their shapes duplicate what’s already defined in thenotifications-node-clienttypings. They can be safely removed to avoid a second source of truth.Also, the coding guidelines state that interfaces/types should live at the bottom of the module. You may want to move
SendEmailParamsandSendEmailResultbelow the functions to match that convention.This is purely a tidiness/consistency change, but it will make the file easier to maintain.
Also applies to: 27-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/notifications/src/govnotify/govnotify-client.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/notifications/src/govnotify/govnotify-client.ts
🧠 Learnings (1)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Never put sensitive data in logs.
Applied to files:
libs/notifications/src/govnotify/govnotify-client.ts
🧬 Code graph analysis (1)
libs/notifications/src/govnotify/govnotify-client.ts (3)
libs/notifications/types/notifications-node-client/index.d.ts (3)
NotificationBody(2-16)EmailResponse(18-20)NotifyClient(45-49)libs/notifications/src/govnotify/template-config.ts (3)
TemplateParameters(32-39)getApiKey(12-17)getTemplateId(5-10)types/notifications-node-client.d.ts (1)
NotifyClient(2-48)
end to end tests working on local
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/notifications/src/govnotify/govnotify-client.test.ts (1)
176-180: Reduce use ofanytypes.The setTimeout mock uses
anyfor the callback parameter and return type. While this is a test mock, you can be more specific about types.- vi.spyOn(global, "setTimeout").mockImplementation((callback: any, delay?: number) => { + vi.spyOn(global, "setTimeout").mockImplementation((callback: () => void, delay?: number) => { if (delay !== undefined) { delays.push(delay); } - return originalSetTimeout(callback, 0) as any; + return originalSetTimeout(callback, 0) as NodeJS.Timeout; });e2e-tests/utils/notification-helpers.ts (1)
59-88: Tighten typings and avoidany/(prisma as any)in notification helpers.The helpers currently bypass type safety:
getNotificationsByPublicationId/getNotificationsBySubscriptionIduse(prisma as any).waitForNotificationsreturnsPromise<any[]>.getGovNotifyEmailreturns an untypedany.As per coding guidelines, avoiding
anywhere possible will help catch schema or API changes early. Consider:
- Returning a concrete type (e.g. Prisma’s
NotificationAuditLog[]or a local interface) from thegetNotifications*andwaitForNotificationsfunctions.- Using typed
prisma.notificationAuditLog/prisma.subscriptioninstead of(prisma as any).- Giving
getGovNotifyEmaila proper return type that reflects the Notify payload you assert on in tests.This keeps the test utilities aligned with strict TypeScript and makes refactors in the notifications schema safer.
Also applies to: 90-123, 125-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
e2e-tests/package.json(1 hunks)e2e-tests/playwright.config.ts(1 hunks)e2e-tests/run-with-credentials.js(2 hunks)e2e-tests/tests/api/blob-ingestion-notifications.spec.ts(1 hunks)e2e-tests/utils/notification-helpers.ts(1 hunks)libs/notifications/src/govnotify/govnotify-client.test.ts(1 hunks)libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/notifications/src/govnotify/govnotify-client.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
Package.json must include"type": "module"and exports field with proper ESM paths.
Pin all dependency versions to specific versions (e.g.,"express": "5.1.0"), except for peer dependencies.
All test packages must use"test": "vitest run"script. Tests should achieve >80% coverage on business logic.
Files:
e2e-tests/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/playwright.config.tslibs/notifications/src/govnotify/govnotify-client.test.tse2e-tests/tests/api/blob-ingestion-notifications.spec.tse2e-tests/utils/notification-helpers.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/playwright.config.tslibs/notifications/src/govnotify/govnotify-client.test.tse2e-tests/tests/api/blob-ingestion-notifications.spec.tse2e-tests/run-with-credentials.jse2e-tests/utils/notification-helpers.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/playwright.config.tslibs/notifications/src/govnotify/govnotify-client.test.tse2e-tests/tests/api/blob-ingestion-notifications.spec.tse2e-tests/utils/notification-helpers.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/notifications/src/govnotify/govnotify-client.test.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
🧠 Learnings (3)
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/notifications/src/govnotify/govnotify-client.test.ts
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
libs/notifications/src/govnotify/govnotify-client.test.tse2e-tests/tests/api/blob-ingestion-notifications.spec.ts
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/*.{ts,tsx} : Constants must use SCREAMING_SNAKE_CASE (e.g., `MAX_FILE_SIZE`, `DEFAULT_TIMEOUT`).
Applied to files:
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
🧬 Code graph analysis (3)
libs/notifications/src/govnotify/govnotify-client.test.ts (3)
libs/notifications/types/notifications-node-client/index.d.ts (1)
NotifyClient(45-49)types/notifications-node-client.d.ts (1)
NotifyClient(2-48)libs/notifications/src/govnotify/govnotify-client.ts (1)
sendEmail(43-54)
e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (3)
e2e-tests/utils/notification-helpers.ts (8)
cleanupTestNotifications(125-134)cleanupTestSubscriptions(136-152)cleanupTestUsers(154-162)createTestUser(19-35)createTestSubscription(37-57)waitForNotifications(66-88)getGovNotifyEmail(97-123)getNotificationsByPublicationId(59-64)e2e-tests/utils/api-auth-helpers.ts (1)
getApiAuthToken(20-70)libs/notifications/src/notification/notification-queries.ts (1)
getNotificationsByPublicationId(61-65)
e2e-tests/utils/notification-helpers.ts (3)
libs/notifications/src/notification/notification-queries.ts (1)
getNotificationsByPublicationId(61-65)libs/notifications/types/notifications-node-client/index.d.ts (1)
NotifyClient(45-49)types/notifications-node-client.d.ts (1)
NotifyClient(2-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (8)
libs/notifications/src/govnotify/govnotify-client.test.ts (4)
41-66: LGTM!The test correctly validates the happy path: dynamic import, proper parameter passing, and assertion of success result with notification ID.
68-87: LGTM!The error handling test correctly validates failure scenarios and error message formatting.
89-147: LGTM!The retry tests correctly validate both successful retry after failure and exhausted retry attempts, including proper call count verification.
149-168: LGTM!The test correctly validates handling of non-Error exceptions, ensuring robust error handling.
e2e-tests/playwright.config.ts (1)
37-42: GOV.UK Notify env wiring into the dev server looks good.Injecting
GOVUK_NOTIFY_API_KEYandGOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTIONinto thewebServer.commandfor both CI and local aligns with the new notification tests and keeps the pattern consistent with the existing SSO/CFT flags. No further changes needed here.e2e-tests/run-with-credentials.js (1)
6-15: Dotenv + Azure credential mappings integrate cleanly with existing flow.Loading
../.envviadotenvin ESM and extendingSECRET_MAPPINGSto populateAZURE_TENANT_ID,AZURE_API_CLIENT_ID, andAZURE_API_CLIENT_SECRETfits well withgetApiAuthTokenand the non‑CI workflow. This should make local notification E2Es much easier to run without extra manual setup.Also applies to: 32-34
e2e-tests/package.json (1)
21-22: Pin dependency versions in e2e-tests/package.json.Using
"*"for@hmcts/postgresand"^17.2.3"fordotenvviolates the requirement to pin dependency versions and can make e2e runs non-reproducible as upstream packages change. Replace both with concrete versions (no range specifiers):- "@hmcts/postgres": "*", - "dotenv": "^17.2.3" + "@hmcts/postgres": "X.Y.Z", + "dotenv": "17.2.3"e2e-tests/tests/api/blob-ingestion-notifications.spec.ts (1)
47-66: Run this notification suite in serial mode to avoid shared‑state flakiness.With
fullyParallel: truein the Playwright config, tests in the same file can execute in parallel. This suite shares mutabletestDataand performs DB cleanup intest.afterEach, so one test can delete another test's users/subscriptions/notifications while it is still running. Switch this block to serial mode:-test.describe("Blob Ingestion - Notification E2E Tests", () => { +test.describe.serial("Blob Ingestion - Notification E2E Tests", () => {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/notifications/src/govnotify/govnotify-client.ts (1)
67-69: Replace custom type assertion with properAxiosResponse<T>from the axios library.The
notifications-node-clientv8.2.1 uses axios under the hood and returns standard Axios responses. Rather than a customAxiosEmailResponseinterface withas unknown as AxiosEmailResponseassertion, import and useAxiosResponse<T>directly:import { AxiosResponse } from 'axios'; const response: AxiosResponse<{ id: string }> = await notifyClient.sendEmail( templateId, params.emailAddress, { personalisation: params.templateParameters } ); const notificationId = response.data.id;This eliminates the double assertion, removes unnecessary
anytypes (config,request), and provides proper type safety for the response structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/notifications/src/govnotify/govnotify-client.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/notifications/src/govnotify/govnotify-client.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/notifications/src/govnotify/govnotify-client.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/notifications/src/govnotify/govnotify-client.ts
🧠 Learnings (1)
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/src/routes/**/*.ts : Validate all user inputs on endpoints. Use parameterized database queries (Prisma). Never include sensitive data in logs.
Applied to files:
libs/notifications/src/govnotify/govnotify-client.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
No need of this file in Github. commit by mistake
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/notifications/src/config.test.ts (1)
25-27: Make the “ends with prisma” assertion OS‑agnostic
expect(prismaSchemas).toMatch(/\/prisma$/);assumes POSIX-style/separators and will fail on Windows, where paths end with\\prisma.You can assert the folder name instead of raw string shape to keep the test portable:
- it("should resolve to a path ending with /prisma", () => { - expect(prismaSchemas).toMatch(/\/prisma$/); - }); + it("should resolve to a path ending with prisma", () => { + expect(path.basename(prismaSchemas)).toBe("prisma"); + });This still validates the intent without depending on platform-specific separators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/notifications/src/config.test.ts(1 hunks)libs/notifications/types/email/index.d.ts(1 hunks)tsconfig.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/notifications/src/config.test.tslibs/notifications/types/email/index.d.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/notifications/src/config.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/notifications/src/config.test.tslibs/notifications/types/email/index.d.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/notifications/src/config.test.tslibs/notifications/types/email/index.d.ts
🧠 Learnings (9)
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/libs/*/src/config.ts : Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate `config.ts` file to avoid circular dependencies during Prisma client generation. Apps import config using the `/config` path.
Applied to files:
libs/notifications/src/config.test.tstsconfig.json
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/notifications/src/config.test.ts
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
libs/notifications/src/config.test.ts
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/package.json : All test packages must use `"test": "vitest run"` script. Tests should achieve >80% coverage on business logic.
Applied to files:
libs/notifications/src/config.test.ts
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/*.{ts,tsx} : Use workspace aliases (`hmcts/*`) for imports between packages instead of relative paths.
Applied to files:
tsconfig.json
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/*.ts : Do not create generic `types.ts` files. Colocate types with the appropriate code file where they are used.
Applied to files:
tsconfig.json
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/*.{ts,tsx} : TypeScript strict mode must be enabled. No `any` type without justification. Use explicit types for all variables and function parameters.
Applied to files:
tsconfig.json
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/*.{ts,tsx} : Always add `.js` extension to relative imports (e.g., `import { foo } from "./bar.js"`), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Applied to files:
tsconfig.json
📚 Learning: 2025-12-03T13:55:34.675Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.675Z
Learning: Applies to **/package.json : Package.json must include `"type": "module"` and exports field with proper ESM paths.
Applied to files:
tsconfig.json
🧬 Code graph analysis (1)
libs/notifications/src/config.test.ts (1)
e2e-tests/run-with-credentials.js (2)
__filename(10-10)__dirname(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (5)
libs/notifications/src/config.test.ts (1)
1-24: Overall test structure and config import look good
- Vitest usage with
describe/it/expectand co-location next toconfig.tsalign with the project testing guidelines.- Deriving
__filename/__dirnameviafileURLToPath(import.meta.url)matches the existing pattern ine2e-tests/run-with-credentials.jsand avoids relying on CJS globals.- Importing
prismaSchemasfrom"./config.js"with the.jsextension is consistent with the ESM +nodenextconvention.
As per coding guidelines and retrieved learnings, this setup is correct and consistent.tsconfig.json (1)
6-6: The typeRoots path is properly configured and the type declarations directory exists.The
./libs/notifications/typesdirectory is correctly set up with type declarations atlibs/notifications/types/email/index.d.ts. The path alias@hmcts/notifications→libs/notifications/srcis also properly configured in the paths section. No additional action required.libs/notifications/types/email/index.d.ts (3)
1-16: LGTM!The
NotificationBodyinterface is well-structured with explicit types, proper nullability handling, and follows naming conventions.
18-20: LGTM!The
EmailResponseinterface correctly wrapsNotificationBody.
45-49: LGTM!The
NotifyClientclass is well-typed with explicit parameter and return types. Method signatures correctly reference the defined interfaces.
1 participant
Jira link
https://tools.hmcts.net/jira/browse/VIBE-221
Change description
Add gov notifier notification email for both api and manual upload functionality
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.