VIBE-341 Add PDF and email summary generation for Civil & Family Daily Cause List - #320
Conversation
📝 WalkthroughWalkthroughThis PR implements the VIBE-341 subscription fulfilment feature, adding PDF generation from Civil and Family Daily Cause List HTML, email summary extraction, GOV.UK Notify template selection (PDF+summary, summary-only, legacy), and centralized publication processing orchestration. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin/API
participant Upload as Upload Service
participant PDF as PDF Generator
participant Summary as Summary Builder
participant Notify as GOV.UK Notify
participant Email as Subscriber Email
Admin->>Upload: Upload Publication (jsonData)
Upload->>PDF: generatePublicationPdf(causeListData)
PDF->>PDF: Render HTML + Nunjucks Template
PDF->>PDF: Convert to PDF (Puppeteer)
PDF-->>Upload: Return {pdfPath, sizeBytes, exceedsMaxSize}
Upload->>Summary: extractCaseSummary(jsonData)
Summary-->>Upload: Return CaseSummaryItem[]
Upload->>Summary: formatCaseSummaryForEmail(items)
Summary-->>Upload: Return formatted summary string
Upload->>Notify: Select template based on PDF size & listTypeId
alt PDF present and < 2MB
Upload->>Notify: sendEmail(pdfBuffer, templateId_PDF_AND_SUMMARY)
else No PDF or >= 2MB
Upload->>Notify: sendEmail(templateId_SUMMARY_ONLY)
end
Notify->>Notify: Upload PDF via prepareUpload (if present)
Notify->>Email: Send email with personalisation & link_to_file
Email-->>Notify: Delivery status
Notify-->>Upload: Notification result {sent, failed, skipped}
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
# Conflicts: # yarn.lock
🎭 Playwright E2E Test Results238 tests 238 ✅ 21m 14s ⏱️ Results for commit de75f03. ♻️ This comment has been updated with latest results. |
# Conflicts: # yarn.lock
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/list-types/civil-and-family-daily-cause-list/src/rendering/renderer.ts (1)
192-241:⚠️ Potential issue | 🟡 Minor
renderCauseListDatamutates the inputjsonDataobject in-place.The function modifies the original object by adding properties like
formattedJudiciariesto session objects (line 217) and directly assigning values to case items. This violates the immutability principle outlined in the coding guidelines. While current usage patterns pass a single locale per invocation—preventing the specific multi-render scenario initially described—in-place mutations remain a code quality issue that could cause problems if the usage patterns change.libs/notifications/src/govnotify/govnotify-client.ts (1)
100-110:⚠️ Potential issue | 🟡 MinorRetry count semantics:
NOTIFICATION_RETRY_ATTEMPTS=1means 2 total attempts.
retryWithBackoffcallsfn()once, then on failure retriesretriesmore times. So ifNOTIFICATION_RETRY_ATTEMPTSis1, the email send is attempted twice total. Ensure this matches the intended behaviour — typically "retry attempts" means additional attempts beyond the first.
🧹 Nitpick comments (13)
package.json (1)
87-87: Consider alternatives for production deployments.Puppeteer v24.36.1 is a valid version, but bundling a full Chromium browser (~400MB+) as a runtime dependency will increase container image size and cold start times. Evaluate whether
puppeteer-corepaired with a pre-installed browser in your Docker image would be more efficient for production use.libs/pdf-generation/src/generator.ts (1)
1-6:PdfGenerationResultis not exported.Downstream consumers (e.g.,
pdf-generator.tsin the cause-list module) cannot explicitly reference this type. Consider exporting the interface so callers can type their variables without relying solely on inference.libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
112-120: Silent catch swallows all JSON parse errors — consider logging.The fallback to flat-file behaviour is intentional, but a malformed JSON file (e.g. truncated upload) will silently proceed with
jsonDataasundefined. A debug-level log here would aid troubleshooting without changing control flow.Suggested improvement
try { jsonData = JSON.parse(uploadData.file.toString("utf8")) as CauseListData; - } catch { - // Not valid JSON, treat as flat file + } catch (parseError) { + console.debug("[Manual Upload] File is not valid JSON, treating as flat file:", parseError); }libs/notifications/src/notification/notification-service.test.ts (1)
53-82: Verbose but necessary mock reset pattern.The async
beforeEachthat re-imports and re-mocks every dependency is repetitive. If the boilerplate grows further, extracting asetupDefaultMocks()helper would reduce noise. Not blocking.libs/list-types/civil-and-family-daily-cause-list/src/rendering/renderer.ts (1)
85-116: Heavy mutation viaas anybypasses type safety and violates immutability guidelines.
calculateDuration,formatHearingChannel,processParties, andformatReportingRestrictionsall mutate their input objects and cast toanyto write computed fields. This creates invisible side effects and disables type checking on these augmented properties.Consider extending the model types with optional computed fields (e.g.
time?: string,durationAsHours?: number) or returning new enriched objects fromrenderCauseListDatainstead of mutating the input. This would eliminate everyas anycast in the file.As per coding guidelines: "Ensure data immutability by default — use const and avoid mutations. Functions should have no side effects and should not modify external state." and "Avoid
anywithout justification."docs/tickets/VIBE-341/plan.md (1)
73-135: Plan diverges from implementation — consider updating.The plan describes
libs/subscription-fulfilmentandlibs/document-serviceas new modules, but the actual PR useslibs/publicationfor orchestration and doesn't introduce a separate document-service module. Keeping the plan aligned with reality avoids confusion for future contributors.Also, the file structure block on line 73 is missing a language specifier (flagged by markdownlint MD040). Use
```textor similar.libs/list-types/civil-and-family-daily-cause-list/src/email-summary/summary-builder.ts (2)
11-20:convertPartyRoleis a no-op — every input maps to itself.The
roleMapmaps each key to an identical value, and the fallback (|| role) returns the original string anyway. This function always returns its input unchanged.Either this is scaffolding for future mapping (in which case a comment explaining intent would help), or it can be removed entirely.
60-84: Six levels of nesting — consider aflatMapchain for readability.The deeply nested
forloops mirror the data model faithfully, but aflatMapchain would flatten the traversal and reduce indentation.♻️ Optional refactor
export function extractCaseSummary(jsonData: CauseListData): CaseSummaryItem[] { - const summaries: CaseSummaryItem[] = []; - - for (const courtList of jsonData.courtLists) { - for (const courtRoom of courtList.courtHouse.courtRoom) { - for (const session of courtRoom.session) { - for (const sitting of session.sittings) { - for (const hearing of sitting.hearing) { - for (const caseItem of hearing.case) { - summaries.push({ - applicant: extractApplicant(caseItem), - caseReferenceNumber: caseItem.caseNumber || "", - caseName: caseItem.caseName || "", - caseType: caseItem.caseType || "", - hearingType: hearing.hearingType || "" - }); - } - } - } - } - } - } - - return summaries; + return jsonData.courtLists + .flatMap((cl) => cl.courtHouse.courtRoom) + .flatMap((cr) => cr.session) + .flatMap((s) => s.sittings) + .flatMap((sit) => sit.hearing) + .flatMap((hearing) => + hearing.case.map((caseItem) => ({ + applicant: extractApplicant(caseItem), + caseReferenceNumber: caseItem.caseNumber || "", + caseName: caseItem.caseName || "", + caseType: caseItem.caseType || "", + hearingType: hearing.hearingType || "" + })) + ); }libs/notifications/src/govnotify/template-config.ts (1)
62-66: Index signaturestring | unknown | undefinedis effectively justunknown.Since
unknownalready encompasses bothstringandundefined, the union is redundant. If the intent is to allow arbitrary keys while keeping the named properties typed asstring, consider narrowing the index signature or using a mapped/intersection type instead — the current signature silently weakens the type of all named fields.libs/notifications/src/notification/notification-service.ts (1)
135-135: Unsafe castevent.jsonData as CauseListData— consider validation.If
jsonDatadoesn't conform to theCauseListDatashape,extractCaseSummarywill throw at runtime. The surroundingtry/catchdoes catch this, but a schema validation step (e.g., using the existingvalidateCivilFamilyCauseList) before casting would provide clearer error messages and prevent silent fallback on malformed data.libs/list-types/civil-and-family-daily-cause-list/src/pdf/pdf-generator.ts (2)
36-46: Nunjucks environment re-created on every call.
configureNunjucks()is called per PDF generation. SincenoCache: trueis set, this is functionally correct, but creating a newEnvironmenteach time is unnecessary overhead. Consider initialising it once at module level.
13-15: Monorepo root resolution via relative path traversal is fragile and used consistently across the codebase.This pattern (5 levels of
..traversal) is repeated in at least 12 files. While it works when all files are at the same relative depth, moving a package or changing directory structure breaks all uses simultaneously. Consider establishing a single, centralised mechanism—either via a shared utility,process.cwd(), or environment variable—rather than duplicating this logic across the codebase.libs/publication/src/processing/service.ts (1)
8-17: Interfaces are interleaved with their associated functions rather than grouped at the bottom.Per the coding guidelines, interfaces and types should be at the bottom of the module. The current placement alongside each function aids readability for this file, so it's a trade-off. Consider moving them to the bottom if consistency with the guideline is preferred.
As per coding guidelines: "Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom."
Also applies to: 19-23, 58-74, 127-145
| - [x] Create `libs/pdf-generation` module (package.json, tsconfig.json) | ||
| - [x] Register module in root tsconfig.json paths | ||
| - [x] Create `libs/pdf-generation/src/generator.ts`: | ||
| - `generatePdfFromHtml(html: string): Promise<Buffer>` - Puppeteer-based conversion |
There was a problem hiding this comment.
Documentation drift: return type doesn't match implementation.
The documented signature says Promise<Buffer>, but generatePdfFromHtml actually returns Promise<PdfGenerationResult>. Worth correcting to avoid misleading future readers.
| 8. **Email Format Requirements**: | ||
|
|
||
| **Opening Statement:** | ||
| > Note this email contains Special Category Data as defined by Data Protection Act 2018, formally known as Sensitive Personal Data, and should be handled appropriately. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n docs/tickets/VIBE-341/ticket.md | sed -n '50,55p'Repository: hmcts/cath-service
Length of output: 641
🏁 Script executed:
rg -in "formally known as"Repository: hmcts/cath-service
Length of output: 628
🏁 Script executed:
rg -in "formerly known as"Repository: hmcts/cath-service
Length of output: 300
Fix typo in legal/compliance notice: "formally" should be "formerly".
The text uses "formally known as" which means "in a formal manner", but should be "formerly known as" meaning "previously". This typo has propagated to production code in libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts:41 and must be corrected in both locations, as this is user-facing legal language.
🧰 Tools
🪛 LanguageTool
[misspelling] ~52-~52: Did you mean “formerly known as” (=known earlier under the name of)?
Context: ...as defined by Data Protection Act 2018, formally known as Sensitive Personal Data, and should be ...
(FORMALLY_KNOWN_AS)
| expect(govNotifyEmail.body).toContain("Unsubscribe"); | ||
|
|
||
| // Verify email delivery status | ||
| expect(govNotifyEmail.status).toMatch(/delivered|sending|pending|created|permanent-failure/); |
There was a problem hiding this comment.
Status assertion accepts permanent-failure — weakens the delivery check.
Including permanent-failure in the expected status regex means the test passes even when email delivery fails. If this is a CI workaround, a comment would clarify intent. Otherwise, restrict to /delivered|sending|pending|created/.
| it("should handle missing optional fields with N/A", () => { | ||
| const testData: CauseListData = { | ||
| document: { | ||
| publicationDate: "2025-01-28T10:00:00Z" | ||
| }, | ||
| venue: { | ||
| venueName: "Test Court", | ||
| venueAddress: { | ||
| line: ["123 Test Street"], | ||
| postCode: "TEST 123" | ||
| } | ||
| }, | ||
| courtLists: [ | ||
| { | ||
| courtHouse: { | ||
| courtHouseName: "Test Courthouse", | ||
| courtRoom: [ | ||
| { | ||
| courtRoomName: "Court 1", | ||
| session: [ | ||
| { | ||
| sittings: [ | ||
| { | ||
| hearing: [ | ||
| { | ||
| case: [ | ||
| { | ||
| party: [] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| const result = extractCaseSummary(testData); | ||
|
|
||
| expect(result[0]).toEqual({ | ||
| applicant: "", | ||
| caseReferenceNumber: "", | ||
| caseName: "", | ||
| caseType: "", | ||
| hearingType: "" | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Misleading test description: says "N/A" but asserts empty strings.
The test name reads "should handle missing optional fields with N/A", yet all assertions check for "". The description should match the actual behaviour.
✏️ Suggested fix
- it("should handle missing optional fields with N/A", () => {
+ it("should handle missing optional fields with empty strings", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should handle missing optional fields with N/A", () => { | |
| const testData: CauseListData = { | |
| document: { | |
| publicationDate: "2025-01-28T10:00:00Z" | |
| }, | |
| venue: { | |
| venueName: "Test Court", | |
| venueAddress: { | |
| line: ["123 Test Street"], | |
| postCode: "TEST 123" | |
| } | |
| }, | |
| courtLists: [ | |
| { | |
| courtHouse: { | |
| courtHouseName: "Test Courthouse", | |
| courtRoom: [ | |
| { | |
| courtRoomName: "Court 1", | |
| session: [ | |
| { | |
| sittings: [ | |
| { | |
| hearing: [ | |
| { | |
| case: [ | |
| { | |
| party: [] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| } | |
| ] | |
| }; | |
| const result = extractCaseSummary(testData); | |
| expect(result[0]).toEqual({ | |
| applicant: "", | |
| caseReferenceNumber: "", | |
| caseName: "", | |
| caseType: "", | |
| hearingType: "" | |
| }); | |
| }); | |
| it("should handle missing optional fields with empty strings", () => { | |
| const testData: CauseListData = { | |
| document: { | |
| publicationDate: "2025-01-28T10:00:00Z" | |
| }, | |
| venue: { | |
| venueName: "Test Court", | |
| venueAddress: { | |
| line: ["123 Test Street"], | |
| postCode: "TEST 123" | |
| } | |
| }, | |
| courtLists: [ | |
| { | |
| courtHouse: { | |
| courtHouseName: "Test Courthouse", | |
| courtRoom: [ | |
| { | |
| courtRoomName: "Court 1", | |
| session: [ | |
| { | |
| sittings: [ | |
| { | |
| hearing: [ | |
| { | |
| case: [ | |
| { | |
| party: [] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| ] | |
| } | |
| } | |
| ] | |
| }; | |
| const result = extractCaseSummary(testData); | |
| expect(result[0]).toEqual({ | |
| applicant: "", | |
| caseReferenceNumber: "", | |
| caseName: "", | |
| caseType: "", | |
| hearingType: "" | |
| }); | |
| }); |
| cautionNote: | ||
| "Note this document contains Special Category Data as defined by Data Protection Act 2018, formally known as Sensitive Personal Data, and should be handled appropriately.", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the typo in the en.ts file
find . -type f -name "en.ts" -path "*/civil-and-family-daily-cause-list/*" -exec head -50 {} + | tail -20Repository: hmcts/cath-service
Length of output: 1234
🏁 Script executed:
# Search for "formally known" pattern to identify all occurrences
rg -n "formally known" --type=tsRepository: hmcts/cath-service
Length of output: 307
🏁 Script executed:
# Check Welsh translation file for the same pattern
find . -type f -name "cy.ts" -path "*/civil-and-family-daily-cause-list/*" -exec cat {} +Repository: hmcts/cath-service
Length of output: 3291
Fix typo in caution note message: "formally" should be "formerly".
Line 41 contains "formally known as Sensitive Personal Data" which should read "formerly known as Sensitive Personal Data". The word "formerly" (previously) is correct here; "formally" (in a formal manner) is incorrect.
The Welsh translation (cy.ts) is already correct, using "a elwid gynt" which properly conveys the past tense.
Proposed fix
cautionNote:
- "Note this document contains Special Category Data as defined by Data Protection Act 2018, formally known as Sensitive Personal Data, and should be handled appropriately.",
+ "Note this document contains Special Category Data as defined by Data Protection Act 2018, formerly known as Sensitive Personal Data, and should be handled appropriately.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cautionNote: | |
| "Note this document contains Special Category Data as defined by Data Protection Act 2018, formally known as Sensitive Personal Data, and should be handled appropriately.", | |
| cautionNote: | |
| "Note this document contains Special Category Data as defined by Data Protection Act 2018, formerly known as Sensitive Personal Data, and should be handled appropriately.", |
| export function buildEnhancedTemplateParameters(params: { | ||
| userName: string; | ||
| hearingListName: string; | ||
| publicationDate: Date; | ||
| locationName: string; | ||
| caseSummary: string; | ||
| }): TemplateParameters { | ||
| const baseParams = buildTemplateParameters({ | ||
| userName: params.userName, | ||
| hearingListName: params.hearingListName, | ||
| publicationDate: params.publicationDate, | ||
| locationName: params.locationName | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libs/notifications/src/govnotify/template-config.tsRepository: hmcts/cath-service
Length of output: 4530
🏁 Script executed:
rg -n 'buildTemplateParameters|buildEnhancedTemplateParameters' libs/notifications/src/Repository: hmcts/cath-service
Length of output: 3736
🏁 Script executed:
sed -n '120,135p' libs/notifications/src/notification/notification-service.ts && echo "---" && sed -n '135,150p' libs/notifications/src/notification/notification-service.ts && echo "---" && sed -n '155,170p' libs/notifications/src/notification/notification-service.tsRepository: hmcts/cath-service
Length of output: 1663
🏁 Script executed:
sed -n '138,195p' libs/notifications/src/govnotify/template-config.test.tsRepository: hmcts/cath-service
Length of output: 2347
Remove unused userName parameter from both functions.
The userName parameter is accepted in both buildTemplateParameters and buildEnhancedTemplateParameters but is not included in the returned TemplateParameters object. Either add it to the template output if it's needed, or remove it from the function signatures to avoid confusion.
| expect(sendEmail).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| templateId: undefined | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -B5 -A5 'templateId' libs/notifications/src/notification/notification-service.ts | head -100Repository: hmcts/cath-service
Length of output: 2179
🏁 Script executed:
# Also check the fallback logic flow in the test and production
rg -n -B3 -A3 'buildTemplateParameters|buildEnhancedTemplateParameters' libs/notifications/src/notification/notification-service.ts | head -80Repository: hmcts/cath-service
Length of output: 1345
🏁 Script executed:
# Examine the actual sendEmail call in the production code
rg -n -B10 -A10 'sendEmail' libs/notifications/src/notification/notification-service.ts | head -120Repository: hmcts/cath-service
Length of output: 1656
Production code bug: fallback path omits templateId, causing undefined to be passed to sendEmail.
The test correctly captures broken production behaviour. When the enhanced template build fails (lines 154–161 of notification-service.ts), the catch block returns only templateParameters without setting templateId. This causes undefined to be passed to sendEmail at line 78, which will likely fail the notification.
The fallback mechanism must supply a default template ID. Ensure buildTemplateParameters returns proper template configuration or explicitly set templateId in the fallback return at line 156.
| async function buildCivilFamilyEmailData(event: PublicationEvent, userName: string): Promise<EmailTemplateData> { | ||
| try { | ||
| const caseSummaryItems = extractCaseSummary(event.jsonData as CauseListData); | ||
| const caseSummary = formatCaseSummaryForEmail(caseSummaryItems); | ||
|
|
||
| const templateParameters = buildEnhancedTemplateParameters({ | ||
| userName, | ||
| hearingListName: event.hearingListName, | ||
| publicationDate: event.publicationDate, | ||
| locationName: event.locationName, | ||
| caseSummary | ||
| }); | ||
|
|
||
| if (event.pdfFilePath) { | ||
| return buildCivilFamilyWithPdf(event.pdfFilePath, templateParameters); | ||
| } | ||
|
|
||
| return { | ||
| status: "failed", | ||
| error: `User ${subscription.userId}: ${emailResult.error}` | ||
| templateParameters, | ||
| templateId: getSubscriptionTemplateIdForListType(CIVIL_AND_FAMILY_DAILY_CAUSE_LIST_ID, false, false) | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| console.error("Failed to build enhanced template parameters, falling back to standard template:", error); | ||
| return { | ||
| status: "failed", | ||
| error: `User ${subscription.userId}: ${errorMessage}` | ||
| templateParameters: buildTemplateParameters({ | ||
| userName, | ||
| hearingListName: event.hearingListName, | ||
| publicationDate: event.publicationDate, | ||
| locationName: event.locationName | ||
| }) | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback path omits templateId — will use the default subscription template.
When buildCivilFamilyEmailData catches an error (line 154), it falls back to buildTemplateParameters without setting templateId. Downstream, sendEmailInternal will then use getTemplateId() (the generic subscription template). This may send a Civil & Family notification using a template that lacks summary/PDF placeholders.
If this is intentional graceful degradation, a log message noting which template is being used would aid debugging. If not, consider explicitly setting the summary-only template ID in the fallback.
| export async function generatePdfFromHtml(html: string): Promise<PdfGenerationResult> { | ||
| // Dynamically import puppeteer to avoid ESM/CJS issues in tests | ||
| const puppeteer = await import("puppeteer"); | ||
| let browser: Awaited<ReturnType<typeof puppeteer.default.launch>> | undefined; | ||
|
|
||
| try { | ||
| browser = await puppeteer.default.launch({ | ||
| headless: true, | ||
| args: ["--no-sandbox", "--disable-setuid-sandbox", "--disable-dev-shm-usage"] | ||
| }); | ||
|
|
||
| const page = await browser.newPage(); | ||
|
|
||
| await page.setContent(html, { | ||
| waitUntil: "networkidle0" | ||
| }); | ||
|
|
||
| const pdfBuffer = await page.pdf(PDF_OPTIONS); | ||
|
|
||
| const sizeBytes = pdfBuffer.length; | ||
|
|
||
| return { | ||
| success: true, | ||
| pdfBuffer: Buffer.from(pdfBuffer), | ||
| sizeBytes | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | ||
| return { | ||
| success: false, | ||
| error: `PDF generation failed: ${errorMessage}` | ||
| }; | ||
| } finally { | ||
| if (browser) { | ||
| await browser.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a timeout to Puppeteer operations to prevent indefinite hangs.
Neither puppeteer.launch() nor page.pdf() has a timeout configured. In production, a malformed or very large HTML payload could cause this function to block indefinitely.
Consider adding a timeout to the launch options and wrapping the critical section in an AbortSignal.timeout or similar guard:
Proposed fix
browser = await puppeteer.default.launch({
headless: true,
- args: ["--no-sandbox", "--disable-setuid-sandbox", "--disable-dev-shm-usage"]
+ args: ["--no-sandbox", "--disable-setuid-sandbox", "--disable-dev-shm-usage"],
+ timeout: 30_000
});
const page = await browser.newPage();
+ page.setDefaultTimeout(30_000);| import { type CauseListData, generateCauseListPdf } from "@hmcts/civil-and-family-daily-cause-list"; | ||
| import { getLocationById } from "@hmcts/location"; | ||
| import { sendPublicationNotifications } from "@hmcts/notifications"; | ||
| import { mockListTypes } from "../index.js"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n 'mockListTypes' --type=ts -C 3Repository: hmcts/cath-service
Length of output: 50374
🏁 Script executed:
# Find the index.js file being imported from
fd index.ts index.js -p '*publication*'Repository: hmcts/cath-service
Length of output: 217
🏁 Script executed:
# Check the index.js or index.ts file in the publication directory
fd -t f 'index\.(ts|js)$' 'libs/publication/src/'Repository: hmcts/cath-service
Length of output: 91
Rename mockListTypes to clarify it is the production list type registry.
The mockListTypes export used throughout the system contains actual production data (list type definitions with IDs and localised friendly names). The "mock" prefix is misleading—rename to LIST_TYPES or listTypeRegistry to accurately signal this is the authoritative list type registry, not test scaffolding.
3 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-341
Change description
Add PDF and email summary generation for Civil & Family Daily Cause List
Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation