VIBE-228 Approve Media Application #151
Conversation
Resolved merge conflict in MediaApplication schema: - Kept approval flow fields (reviewedDate, reviewedBy, proofOfIdPath) - Required for VIBE-228 admin approval functionality Brings in VIBE-175 media account creation form from master. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughAdds a media-application approval feature: DB schema and migrations, admin pages (list, details, approve, approved), file serving and deletion, Prisma queries/services, GOV.UK Notify integration and types, public upload changes (proof file persistence), tests (unit and e2e), a backfill script, and a new notification library. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Browser as Web Browser
participant Server as Express Server
participant DB as Database (Prisma)
participant FS as File System
participant Notify as GOV.UK Notify
Admin->>Browser: Open pending applications
Browser->>Server: GET /media-applications
Server->>DB: getPendingApplications()
DB-->>Server: list
Server-->>Browser: Render list (banner if pendingCount>0)
Admin->>Browser: View application details
Browser->>Server: GET /media-applications/:id
Server->>DB: getApplicationById(id)
DB-->>Server: application
Server-->>Browser: Render details with proof link
Admin->>Browser: Open approve page
Browser->>Server: GET /media-applications/:id/approve
Server->>DB: getApplicationById(id)
DB-->>Server: application
Server-->>Browser: Render confirmation form
Admin->>Browser: Submit "Yes"
Browser->>Server: POST /media-applications/:id/approve
Server->>DB: updateApplicationStatus(id, APPROVED)
DB-->>Server: updatedApplication
Server->>FS: deleteProofOfIdFile(path) (if present)
FS-->>Server: deletion result
Server->>Notify: sendMediaApprovalEmail({name,email,employer})
Notify-->>Server: email result
Server-->>Browser: Redirect to /approved
Browser->>Server: GET /media-applications/:id/approved
Server->>DB: getApplicationById(id)
DB-->>Server: updatedApplication
Server-->>Browser: Render approved page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Comment |
Resolved yarn.lock conflict and updated dependencies from master.
🎭 Playwright E2E Test Results413 tests 413 ✅ 33m 25s ⏱️ Results for commit 8686700. ♻️ This comment has been updated with latest results. |
- Update vitest from 3.2.4 to 4.0.6 to match other packages - Fix test mocks to use class-based approach for vitest 4.0.6 - All notification tests now passing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
libs/web-core/src/middleware/govuk-frontend/configure-govuk.test.ts (1)
255-255: Sameas anyconcern as on Line 51.This repeats the direct access to
env.loadersviaas any. The same optional refactor suggested for Line 51 (using a helper type or public API) would apply here too.
🧹 Nitpick comments (25)
libs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.ts (1)
127-152: Strengthen assertions to verify enriched data is passed to the viewThe enriched mock data (including Welsh names, jurisdiction and region arrays) is set up and returned from
enrichLocationData, but the test only assertsfileNameandhasErrors. To properly guard this flow, consider also asserting that the rendered view receives the enriched data structure (e.g. checking whichever property carries the rows) so regressions in enrichment wiring are caught by this test. Based on learnings, the chosensubJurisdictionNames/regionNamesvalues also correctly avoid semicolons used as CSV delimiters.libs/web-core/src/middleware/govuk-frontend/configure-govuk.test.ts (1)
51-51: Minimiseas anywhen asserting onenv.loaders.Even in tests, casting to
anyloses useful type checking. Consider either asserting behaviour via the public Nunjucks API instead of reaching intoloaders, or introducing a narrow helper type (e.g.type EnvWithLoaders = Awaited<ReturnType<typeof configureGovuk>> & { loaders: unknown }) and casting to that instead ofany.libs/web-core/src/assets/js/search-autocomplete.test.ts (1)
34-34: Consider a more specific type forsource.While
anyis acceptable for test mocks, using a more specific type would improve type safety. Based on the accessible-autocomplete library,sourceis typically a function.If desired, apply this diff to use a more precise type:
-const mockAccessibleAutocomplete = vi.fn((config: { element: HTMLElement; id: string; source?: any }) => { +const mockAccessibleAutocomplete = vi.fn((config: { element: HTMLElement; id: string; source?: (query: string, populateResults: (results: unknown[]) => void) => void }) => {libs/admin-pages/src/pages/manual-upload/index.test.ts (1)
543-561: AsyncvalidateFormmocks are consistent with an async validatorSwitching the POST-path expectations to use
vi.mocked(validateForm).mockResolvedValue(...)is the right shape ifvalidateFormis now async and ensures the handlers under test see a resolved Promise.If you want to tighten things further, you could also make the module-level mock async for consistency, e.g.:
vi.mock("../../manual-upload/validation.js", () => ({ validateForm: vi.fn().mockResolvedValue([]) }));so that any tests which don’t override
validateFormstill exercise the async code path.Also applies to: 601-616, 652-662, 693-705, 739-749, 783-789, 823-829, 861-869
libs/admin-pages/src/pages/media-applications/[id]/index.njk (2)
36-47: Refactor complex HTML concatenation and improve accessibility label.The proof of ID row uses complex inline concatenation that's hard to read and maintain. Additionally, the
aria-labelis generic.Consider refactoring to use Nunjucks conditionals:
{ key: { text: tableHeaders.proofOfId }, - value: { - html: ( - '<span>' + proofOfIdText + ' ' if application.proofOfIdPath else '' - ) + ( - '<a href="/media-applications/' + application.id + '/proof-of-id" class="govuk-link" target="_blank" rel="noopener noreferrer" aria-label="Opens in new window">' + viewProofOfId + '</a>' if application.proofOfIdPath else fileNotAvailable - ) + ( - '</span>' if application.proofOfIdPath else '' - ) - } + value: { + html: ( + '<span>' + proofOfIdText + ' <a href="/media-applications/' + application.id + '/proof-of-id" class="govuk-link" target="_blank" rel="noopener noreferrer" aria-label="' + viewProofOfId + ' (opens in new window)">' + viewProofOfId + '</a></span>' + ) if application.proofOfIdPath else fileNotAvailable + } }This also improves the aria-label to describe what opens in a new window.
51-65: Move inline styles to CSS classes.The button container uses inline styles for flexbox layout, which couples presentation with markup and makes theming difficult.
Define a CSS class in your stylesheet:
.media-application-actions { display: flex; justify-content: space-between; align-items: center; margin-top: 2rem; }Then update the template:
- <div style="display: flex; justify-content: space-between; align-items: center; margin-top: 2rem;"> + <div class="media-application-actions">docs/tickets/VIBE-228/plan.md (2)
34-38: Add language identifier to fenced code block.The module structure code block (lines 34-38) should specify a language for syntax highlighting.
-``` +```plaintext media-application/Based on learnings, as per static analysis hints.
55-61: Add blank lines around the table.The page routes table should be surrounded by blank lines for proper Markdown rendering.
Add a blank line before line 55 and after line 61.
Based on learnings, as per static analysis hints.
libs/admin-pages/src/pages/admin-dashboard/index.test.ts (1)
15-22: Prisma mock formediaApplication.countis appropriate and isolates tests.Mocking
@hmcts/postgreswithprisma.mediaApplication.countresolving to0matches the query layer shape and keeps these tests independent of a real database. If you later want to assert the notification banner for pending applications, you can override this mock per test to return a non-zero count.scripts/fix-media-application-file-paths.ts (2)
11-56: Script logic cleanly reconciles DB rows with on-disk files.The use of
prisma.mediaApplication.findMany/updatewith a fixed set of extensions andfs.accesskeeps everything parameterised and avoids raw SQL, which matches the database guidelines. As a small tidy-up, you could hoist theextensionsarray to a module-level constant (e.g.const FILE_EXTENSIONS = [...]) to avoid reallocating it per application and better follow the “constants at the top” convention.
59-66: Preferprocess.exitCodeoverprocess.exit(1)so Prisma disconnect always runs.Calling
process.exit(1)inside thecatchmeans the subsequentfinallyblock might not complete, soprisma.$disconnect()may be skipped on errors; switching toprocess.exitCode = 1preserves a non‑zero exit status while still allowing thefinallyblock to run to completion.libs/admin-pages/src/pages/admin-dashboard/index.ts (1)
3-23: GET handler correctly wires pending media count and notification content.
getPendingCountis only awaited forINTERNAL_ADMIN_CTSC, keeping DB access scoped to the role that needs it, and the handler now passespendingCountplus language-specificnotificationText/notificationLinkinto the view while preserving the existing tile-filtering logic. As an optional follow‑up, you could add a dedicated test that sets the mocked count > 0 and asserts the banner content in the rendered context.libs/admin-pages/src/pages/admin-dashboard/index.njk (1)
10-26: Consider extracting hardcoded strings to content objects.The notification banner works correctly but has two hardcoded values:
- Line 15: "Important" title
- Line 20: "/media-applications" URL
For consistency with i18n best practices and maintainability, consider moving these to the content objects (en.ts/cy.ts).
libs/admin-pages/src/pages/media-applications/[id]/approved.njk (1)
8-14: Consider using govukErrorSummary macro for consistency.The error display uses a custom div structure. While functional, using the
govukErrorSummarymacro would be more consistent with GOV.UK Design System patterns.libs/admin-pages/src/pages/media-applications/[id]/index.ts (1)
8-49: Consider logging the error for debugging purposes.The error is silently caught and discarded with
_error. While the user sees a friendly error message, logging the error would help with debugging in production.- } catch (_error) { + } catch (error) { + console.error("Failed to load media application:", error); res.render("media-applications/[id]/index", { pageTitle: lang.pageTitle, error: lang.errorMessages.loadFailed, application: null, hideLanguageToggle: true }); }libs/public-pages/src/pages/create-media-account/index.ts (2)
71-74: Consider handling the case where file upload succeeds but path update fails.If
updateProofOfIdPaththrows, the application is created but without the proof of ID path. The file exists on storage but the database reference is lost. Consider wrapping this in a transaction or handling the partial failure gracefully.if (file) { - const filePath = await saveIdProofFile(applicationId, file.originalname, file.buffer); - await updateProofOfIdPath(applicationId, filePath); + try { + const filePath = await saveIdProofFile(applicationId, file.originalname, file.buffer); + await updateProofOfIdPath(applicationId, filePath); + } catch (fileError) { + console.error("Failed to save proof of ID file:", fileError); + // Application created but file not linked - consider cleanup or retry logic + } }Alternatively, consider using a database transaction that includes the application creation and path update together.
82-84: Avoid logging the full error object which may contain sensitive information.The
errorobject could contain user data or internal details. Consider logging only the error message.As per coding guidelines about not putting sensitive data in logs:
} catch (error) { - console.error("Error creating media application:", error); + console.error("Error creating media application:", error instanceof Error ? error.message : "Unknown error"); res.status(500).render("errors/500", { locale }); }libs/admin-pages/src/media-application/queries.test.ts (1)
1-154: Good coverage and accurate Prisma call assertionsThe tests exercise all query helpers, including success and not-found/zero-count paths, and assert the exact Prisma
where/select/orderByshapes. Usingexpect.any(Date)forreviewedDatekeepsupdateApplicationStatustests robust. If you find these tests growing, you could consider a small factory/helper for mock applications to avoid repetition, but it’s not necessary right now.libs/admin-pages/src/media-application/service.test.ts (1)
1-126: Solid coverage of approval flow and file deletion edge casesThese tests exercise all key branches of
approveApplication(happy path, no file, not found, already reviewed) anddeleteProofOfIdFile(valid path, traversal attempt, ENOENT, other fs errors). The expectations line up with the implementation, including specific error messages and the ENOENT swallow behavior.If
proofOfIdPathcan ever be influenced by user-controlled data or external systems, you might later want to hardendeleteProofOfIdFilefurther by constraining deletions to a configured base directory inservice.ts(e.g., resolving against a known root) and adding a test for that. As it stands, for internally-generated paths this is sufficient.libs/notification/src/govuk-notify-service.ts (1)
12-32: Consider validating environment variables at module initialization.Currently, environment variables are validated when the function is called. Validating at module load time would fail fast at startup rather than during runtime.
Consider moving the validation to module level:
+if (!process.env.GOVUK_NOTIFY_API_KEY) { + throw new Error("GOV Notify API key not configured"); +} + +if (!process.env.GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_APPROVAL) { + throw new Error("GOV Notify approval template ID not configured"); +} + const GOVUK_NOTIFY_API_KEY = process.env.GOVUK_NOTIFY_API_KEY; const TEMPLATE_ID_MEDIA_APPROVAL = process.env.GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_APPROVAL; -export async function sendMediaApprovalEmail(data: MediaApplicationEmailData): Promise<void> { - if (!GOVUK_NOTIFY_API_KEY) { - throw new Error("GOV Notify API key not configured"); - } - - if (!TEMPLATE_ID_MEDIA_APPROVAL) { - throw new Error("GOV Notify approval template ID not configured"); - } - +export async function sendMediaApprovalEmail(data: MediaApplicationEmailData): Promise<void> { const notifyClient = new NotifyClient(GOVUK_NOTIFY_API_KEY);libs/admin-pages/src/pages/media-applications/[id]/proof-of-id.ts (1)
45-46: Consider streaming files instead of buffering for better memory efficiency.Currently, the entire file is read into memory before sending. For large proof-of-ID files, this could consume significant memory. Consider using
fs.createReadStream()andpipe()for better performance.-const fileBuffer = await fs.readFile(filePath); -res.send(fileBuffer); +const readStream = createReadStream(filePath); +readStream.pipe(res);Don't forget to import
createReadStream:-import fs from "node:fs/promises"; +import { createReadStream } from "node:fs"; +import fs from "node:fs/promises";libs/admin-pages/src/media-application/queries.ts (2)
42-42: UseApplicationStatustype instead ofstringfor thestatusparameter.The
statusparameter accepts any string, but should be constrained to valid status values. Using theApplicationStatustype from your model ensures compile-time safety.+import { APPLICATION_STATUS, type ApplicationStatus, type MediaApplicationDetails, type PendingApplicationSummary } from "./model.js"; -import { APPLICATION_STATUS, type MediaApplicationDetails, type PendingApplicationSummary } from "./model.js"; ... -export async function updateApplicationStatus(id: string, status: string, reviewedBy: string): Promise<MediaApplicationDetails> { +export async function updateApplicationStatus(id: string, status: ApplicationStatus, reviewedBy: string): Promise<MediaApplicationDetails> {
39-39: Type assertion may mask schema-model drift.The
as MediaApplicationDetailsassertion assumes Prisma's return type matches your interface. If the schema changes, this could silently produce incorrect types. Consider using Prisma's generated types or a validation layer.apps/postgres/prisma/schema.prisma (1)
46-58: Consider adding an index on thestatusfield for query performance.The
getPendingApplicationsandgetPendingCountqueries filter bystatus. As the table grows, an index would improve query performance.reviewedBy String? @map("reviewed_by") @db.VarChar(255) + @@index([status]) @@map("media_application") }e2e-tests/tests/media-application-approval.spec.ts (1)
208-212: Document the rationale for disabling accessibility rules.Disabling
target-size,link-name, andregionrules across all accessibility tests reduces coverage. Consider adding a comment explaining why these rules are disabled (e.g., known GOV.UK Design System limitations) or creating issues to address them.
📜 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 (65)
apps/api/src/server.test.ts(1 hunks)apps/postgres/prisma/migrations/20251201102249_media_application/migration.sql(1 hunks)apps/postgres/prisma/schema.prisma(1 hunks)apps/web/.env.example(1 hunks)docs/tickets/VIBE-228/plan.md(1 hunks)docs/tickets/VIBE-228/tasks.md(1 hunks)docs/tickets/VIBE-228/ticket.md(1 hunks)e2e-tests/tests/media-application-approval.spec.ts(1 hunks)libs/admin-pages/package.json(1 hunks)libs/admin-pages/src/media-application/model.ts(1 hunks)libs/admin-pages/src/media-application/queries.test.ts(1 hunks)libs/admin-pages/src/media-application/queries.ts(1 hunks)libs/admin-pages/src/media-application/service.test.ts(1 hunks)libs/admin-pages/src/media-application/service.ts(1 hunks)libs/admin-pages/src/pages/admin-dashboard/cy.ts(1 hunks)libs/admin-pages/src/pages/admin-dashboard/en.ts(1 hunks)libs/admin-pages/src/pages/admin-dashboard/index.njk(1 hunks)libs/admin-pages/src/pages/admin-dashboard/index.test.ts(1 hunks)libs/admin-pages/src/pages/admin-dashboard/index.ts(2 hunks)libs/admin-pages/src/pages/manual-upload/index.test.ts(6 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve-cy.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve-en.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.njk(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approved-cy.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approved-en.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approved.njk(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approved.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/cy.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/en.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/index.njk(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/index.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/proof-of-id.ts(1 hunks)libs/admin-pages/src/pages/media-applications/cy.ts(1 hunks)libs/admin-pages/src/pages/media-applications/en.ts(1 hunks)libs/admin-pages/src/pages/media-applications/index.njk(1 hunks)libs/admin-pages/src/pages/media-applications/index.ts(1 hunks)libs/api/src/middleware/oauth-middleware.ts(1 hunks)libs/notification/package.json(1 hunks)libs/notification/src/govuk-notify-service.test.ts(1 hunks)libs/notification/src/govuk-notify-service.ts(1 hunks)libs/notification/src/index.ts(1 hunks)libs/notification/src/notifications-node-client.d.ts(1 hunks)libs/notification/tsconfig.json(1 hunks)libs/public-pages/src/media-application/repository/model.ts(2 hunks)libs/public-pages/src/media-application/repository/query.test.ts(3 hunks)libs/public-pages/src/media-application/repository/query.ts(1 hunks)libs/public-pages/src/media-application/storage.test.ts(1 hunks)libs/public-pages/src/media-application/storage.ts(1 hunks)libs/public-pages/src/pages/create-media-account/index.njk(1 hunks)libs/public-pages/src/pages/create-media-account/index.test.ts(9 hunks)libs/public-pages/src/pages/create-media-account/index.ts(3 hunks)libs/public-pages/src/pages/validation.ts(2 hunks)libs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.ts(2 hunks)libs/system-admin-pages/src/reference-data-upload/repository/upload-repository.test.ts(1 hunks)libs/verified-pages/src/pages/delete-subscription/index.test.ts(5 hunks)libs/verified-pages/src/pages/subscription-management/index.test.ts(1 hunks)libs/verified-pages/src/pages/unsubscribe-confirmation/index.test.ts(1 hunks)libs/web-core/src/assets/js/search-autocomplete.test.ts(2 hunks)libs/web-core/src/middleware/file-upload/file-upload-middleware.test.ts(1 hunks)libs/web-core/src/middleware/govuk-frontend/configure-govuk.test.ts(3 hunks)libs/web-core/src/middleware/helmet/helmet-middleware.test.ts(1 hunks)package.json(1 hunks)scripts/fix-media-application-file-paths.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
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/admin-pages/package.jsonlibs/notification/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/web-core/src/middleware/govuk-frontend/configure-govuk.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.tslibs/admin-pages/src/pages/admin-dashboard/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/admin-pages/src/pages/admin-dashboard/cy.tsapps/api/src/server.test.tslibs/web-core/src/middleware/helmet/helmet-middleware.test.tslibs/web-core/src/assets/js/search-autocomplete.test.tslibs/admin-pages/src/pages/media-applications/[id]/cy.tslibs/public-pages/src/pages/validation.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/pages/manual-upload/index.test.tslibs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.tsscripts/fix-media-application-file-paths.tslibs/admin-pages/src/pages/media-applications/[id]/approved-cy.tslibs/web-core/src/middleware/file-upload/file-upload-middleware.test.tslibs/admin-pages/src/pages/admin-dashboard/en.tslibs/admin-pages/src/pages/media-applications/[id]/proof-of-id.tslibs/admin-pages/src/media-application/queries.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/govuk-notify-service.test.tslibs/public-pages/src/media-application/repository/query.tslibs/admin-pages/src/media-application/queries.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tse2e-tests/tests/media-application-approval.spec.tslibs/notification/src/notifications-node-client.d.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/cy.tslibs/admin-pages/src/media-application/service.tslibs/admin-pages/src/pages/media-applications/[id]/index.tslibs/admin-pages/src/pages/media-applications/[id]/approved-en.tslibs/public-pages/src/media-application/repository/model.tslibs/admin-pages/src/pages/media-applications/[id]/en.tslibs/public-pages/src/pages/create-media-account/index.tslibs/admin-pages/src/media-application/model.tslibs/notification/src/index.tslibs/public-pages/src/media-application/storage.test.tslibs/public-pages/src/pages/create-media-account/index.test.tslibs/admin-pages/src/pages/media-applications/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/api/src/middleware/oauth-middleware.tslibs/public-pages/src/media-application/storage.tslibs/system-admin-pages/src/reference-data-upload/repository/upload-repository.test.tslibs/public-pages/src/media-application/repository/query.test.tslibs/admin-pages/src/media-application/service.test.tslibs/admin-pages/src/pages/admin-dashboard/index.tslibs/admin-pages/src/pages/media-applications/en.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/web-core/src/middleware/govuk-frontend/configure-govuk.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.tslibs/admin-pages/src/pages/admin-dashboard/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/admin-pages/src/pages/admin-dashboard/cy.tsapps/api/src/server.test.tslibs/web-core/src/middleware/helmet/helmet-middleware.test.tslibs/web-core/src/assets/js/search-autocomplete.test.tslibs/admin-pages/src/pages/media-applications/[id]/cy.tslibs/public-pages/src/pages/validation.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/pages/manual-upload/index.test.tslibs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.tsscripts/fix-media-application-file-paths.tslibs/admin-pages/src/pages/media-applications/[id]/approved-cy.tslibs/web-core/src/middleware/file-upload/file-upload-middleware.test.tslibs/admin-pages/src/pages/admin-dashboard/en.tslibs/admin-pages/src/pages/media-applications/[id]/proof-of-id.tslibs/admin-pages/src/media-application/queries.tslibs/notification/src/govuk-notify-service.tslibs/notification/src/govuk-notify-service.test.tslibs/public-pages/src/media-application/repository/query.tslibs/admin-pages/src/media-application/queries.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tse2e-tests/tests/media-application-approval.spec.tslibs/notification/src/notifications-node-client.d.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/cy.tslibs/admin-pages/src/media-application/service.tslibs/admin-pages/src/pages/media-applications/[id]/index.tslibs/admin-pages/src/pages/media-applications/[id]/approved-en.tslibs/public-pages/src/media-application/repository/model.tslibs/admin-pages/src/pages/media-applications/[id]/en.tslibs/public-pages/src/pages/create-media-account/index.tslibs/admin-pages/src/media-application/model.tslibs/notification/src/index.tslibs/public-pages/src/media-application/storage.test.tslibs/public-pages/src/pages/create-media-account/index.test.tslibs/admin-pages/src/pages/media-applications/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/api/src/middleware/oauth-middleware.tslibs/public-pages/src/media-application/storage.tslibs/system-admin-pages/src/reference-data-upload/repository/upload-repository.test.tslibs/public-pages/src/media-application/repository/query.test.tslibs/admin-pages/src/media-application/service.test.tslibs/admin-pages/src/pages/admin-dashboard/index.tslibs/admin-pages/src/pages/media-applications/en.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/web-core/src/middleware/govuk-frontend/configure-govuk.test.tslibs/admin-pages/src/pages/admin-dashboard/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tsapps/api/src/server.test.tslibs/web-core/src/middleware/helmet/helmet-middleware.test.tslibs/web-core/src/assets/js/search-autocomplete.test.tslibs/admin-pages/src/pages/manual-upload/index.test.tslibs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.tslibs/web-core/src/middleware/file-upload/file-upload-middleware.test.tslibs/notification/src/govuk-notify-service.test.tslibs/admin-pages/src/media-application/queries.test.tslibs/verified-pages/src/pages/subscription-management/index.test.tse2e-tests/tests/media-application-approval.spec.tslibs/public-pages/src/media-application/storage.test.tslibs/public-pages/src/pages/create-media-account/index.test.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/system-admin-pages/src/reference-data-upload/repository/upload-repository.test.tslibs/public-pages/src/media-application/repository/query.test.tslibs/admin-pages/src/media-application/service.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/media-applications/[id]/approved.tslibs/admin-pages/src/pages/admin-dashboard/index.test.tslibs/verified-pages/src/pages/delete-subscription/index.test.tslibs/admin-pages/src/pages/admin-dashboard/cy.tslibs/admin-pages/src/pages/media-applications/[id]/cy.tslibs/public-pages/src/pages/validation.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/pages/manual-upload/index.test.tslibs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved-cy.tslibs/admin-pages/src/pages/admin-dashboard/en.tslibs/admin-pages/src/pages/media-applications/[id]/proof-of-id.tslibs/verified-pages/src/pages/subscription-management/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/cy.tslibs/admin-pages/src/pages/media-applications/[id]/index.tslibs/admin-pages/src/pages/media-applications/[id]/approved-en.tslibs/admin-pages/src/pages/media-applications/[id]/en.tslibs/public-pages/src/pages/create-media-account/index.tslibs/public-pages/src/pages/create-media-account/index.test.tslibs/admin-pages/src/pages/media-applications/index.tslibs/verified-pages/src/pages/unsubscribe-confirmation/index.test.tslibs/admin-pages/src/pages/admin-dashboard/index.tslibs/admin-pages/src/pages/media-applications/en.ts
**/pages/**/*.njk
📄 CodeRabbit inference engine (CLAUDE.md)
Nunjucks templates must extend
layouts/base-templates.njkand use GOV.UK component macros (govukButton, govukInput, govukErrorSummary, etc.).
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.njklibs/admin-pages/src/pages/media-applications/[id]/index.njklibs/public-pages/src/pages/create-media-account/index.njklibs/admin-pages/src/pages/media-applications/index.njklibs/admin-pages/src/pages/admin-dashboard/index.njklibs/admin-pages/src/pages/media-applications/[id]/approved.njk
**/tsconfig.json
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript must use strict mode with no
anywithout justification.
Files:
tsconfig.jsonlibs/notification/tsconfig.json
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
package.json: All package.json files must use"type": "module"to enforce ES Modules.
Dependencies must be pinned to specific versions (e.g.,"express": "5.1.0"), except for peer dependencies.
Files:
package.json
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/notification/tsconfig.json
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
**/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:
apps/postgres/prisma/schema.prisma
🧠 Learnings (18)
📚 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/admin-pages/package.jsontsconfig.jsonlibs/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 **/*.{ts,tsx} : Use workspace aliases (`hmcts/*`) for imports instead of relative paths across packages.
Applied to files:
libs/admin-pages/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 **/pages/**/*.ts : Page controllers must export named `GET` and/or `POST` functions with Express Request and Response types.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approved.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/pages/media-applications/[id]/index.tslibs/admin-pages/src/pages/media-applications/index.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 libs/**/locales/**/*.ts : Shared content (common button text, phase banners, service names, common error messages) goes in locale files (`libs/[module]/src/locales/en.ts` and `cy.ts`). Page-specific content goes in controllers.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approved.tslibs/admin-pages/src/pages/admin-dashboard/cy.tslibs/admin-pages/src/pages/media-applications/[id]/cy.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approved-cy.tslibs/admin-pages/src/pages/admin-dashboard/en.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/cy.tslibs/admin-pages/src/pages/media-applications/[id]/approved-en.tslibs/admin-pages/src/pages/media-applications/[id]/en.tslibs/admin-pages/src/pages/media-applications/en.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 **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/cy.tslibs/admin-pages/src/pages/media-applications/[id]/approved-cy.tse2e-tests/tests/media-application-approval.spec.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/cy.ts
📚 Learning: 2025-11-20T09:59:16.776Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 106
File: libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts:84-160
Timestamp: 2025-11-20T09:59:16.776Z
Learning: In the cath-service repository, Welsh localization (lng=cy) is not required for admin screens (system-admin-pages), so locale preservation in admin screen redirects is not necessary.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/cy.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/cy.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 skip Welsh translations - required for all user-facing text.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/cy.tslibs/admin-pages/src/pages/media-applications/[id]/approved-cy.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.tslibs/admin-pages/src/pages/media-applications/cy.ts
📚 Learning: 2025-11-20T10:19:35.873Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 106
File: libs/system-admin-pages/src/reference-data-upload/services/download-service.ts:25-27
Timestamp: 2025-11-20T10:19:35.873Z
Learning: In the HMCTS cath-service project, region and sub-jurisdiction names are managed manually and must not contain semicolons, as semicolons are used as delimiters when joining multiple values in CSV export/import operations (specifically in libs/system-admin-pages/src/reference-data-upload/services/download-service.ts).
Applied to files:
libs/system-admin-pages/src/pages/reference-data-upload-summary/index.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} : Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Applied to files:
scripts/fix-media-application-file-paths.tslibs/admin-pages/src/media-application/queries.tslibs/admin-pages/src/media-application/queries.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 **/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk` and use GOV.UK component macros (govukButton, govukInput, govukErrorSummary, etc.).
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.njklibs/admin-pages/src/pages/media-applications/[id]/index.njklibs/admin-pages/src/pages/media-applications/index.njklibs/admin-pages/src/pages/media-applications/[id]/approved.njk
📚 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:
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 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:
tsconfig.jsonlibs/admin-pages/src/media-application/queries.tslibs/notification/tsconfig.jsonlibs/admin-pages/src/pages/media-applications/index.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:
tsconfig.jsone2e-tests/tests/media-application-approval.spec.tslibs/notification/tsconfig.jsonlibs/public-pages/src/media-application/repository/query.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} : 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/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/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 **/tsconfig.json : TypeScript must use strict mode with no `any` without justification.
Applied to files:
libs/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 : 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.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 **/prisma/schema.prisma : Database tables and fields MUST be singular and snake_case (e.g., `user`, `case`, `created_at`). Use Prisma `@map` and `map` for aliases.
Applied to files:
apps/postgres/prisma/schema.prisma
🧬 Code graph analysis (12)
libs/admin-pages/src/pages/media-applications/[id]/approved.ts (5)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-40)libs/admin-pages/src/pages/media-applications/[id]/approve.ts (1)
GET(102-102)libs/admin-pages/src/pages/admin-dashboard/index.ts (1)
GET(27-27)libs/admin-pages/src/pages/media-applications/[id]/index.ts (1)
GET(51-51)libs/admin-pages/src/pages/media-applications/index.ts (1)
GET(31-31)
libs/admin-pages/src/pages/media-applications/[id]/proof-of-id.ts (2)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-40)libs/admin-pages/src/pages/media-applications/[id]/approve.ts (1)
GET(102-102)
libs/admin-pages/src/media-application/queries.ts (1)
libs/admin-pages/src/media-application/model.ts (3)
PendingApplicationSummary(21-26)APPLICATION_STATUS(1-5)MediaApplicationDetails(9-19)
libs/notification/src/govuk-notify-service.test.ts (2)
libs/notification/src/govuk-notify-service.ts (1)
sendMediaApprovalEmail(12-32)libs/notification/src/index.ts (1)
sendMediaApprovalEmail(1-1)
e2e-tests/tests/media-application-approval.spec.ts (1)
e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
libs/admin-pages/src/pages/media-applications/[id]/index.ts (6)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-40)libs/admin-pages/src/media-application/model.ts (1)
APPLICATION_STATUS(1-5)libs/admin-pages/src/pages/media-applications/[id]/approve.ts (1)
GET(102-102)libs/admin-pages/src/pages/admin-dashboard/index.ts (1)
GET(27-27)libs/admin-pages/src/pages/media-applications/[id]/approved.ts (1)
GET(39-39)libs/admin-pages/src/pages/media-applications/index.ts (1)
GET(31-31)
libs/public-pages/src/media-application/storage.test.ts (1)
libs/public-pages/src/media-application/storage.ts (1)
saveIdProofFile(6-16)
libs/public-pages/src/pages/create-media-account/index.test.ts (4)
libs/public-pages/src/media-application/storage.ts (1)
saveIdProofFile(6-16)libs/public-pages/src/pages/create-media-account/index.ts (1)
POST(39-86)libs/public-pages/src/media-application/repository/model.ts (1)
MulterRequest(30-32)libs/public-pages/src/media-application/repository/query.ts (2)
createMediaApplication(4-15)updateProofOfIdPath(17-22)
libs/admin-pages/src/pages/media-applications/index.ts (1)
libs/admin-pages/src/media-application/queries.ts (1)
getPendingApplications(4-21)
libs/public-pages/src/media-application/repository/query.test.ts (2)
libs/public-pages/src/media-application/repository/query.ts (2)
createMediaApplication(4-15)updateProofOfIdPath(17-22)libs/public-pages/src/media-application/repository/model.ts (1)
MediaApplicationCreateData(16-20)
libs/admin-pages/src/media-application/service.test.ts (2)
libs/admin-pages/src/media-application/model.ts (1)
APPLICATION_STATUS(1-5)libs/admin-pages/src/media-application/service.ts (2)
approveApplication(6-22)deleteProofOfIdFile(24-37)
libs/admin-pages/src/pages/admin-dashboard/index.ts (1)
libs/admin-pages/src/media-application/queries.ts (1)
getPendingCount(66-74)
🪛 ast-grep (0.40.0)
e2e-tests/tests/media-application-approval.spec.ts
[warning] 80-80: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 96-96: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approve)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 116-116: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 128-128: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 150-150: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 249-249: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Gitleaks (8.29.1)
libs/notification/src/govuk-notify-service.test.ts
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
docs/tickets/VIBE-228/plan.md
34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
55-55: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
64-64: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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
apps/postgres/prisma/migrations/20251201102249_media_application/migration.sql
Show resolved
Hide resolved
- Create global types directory with notifications-node-client declarations - Add typeRoots to tsconfig.json to include custom types - Update notification build script to copy declaration file - Fixes TypeScript compilation error in admin-pages build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/notification/src/govuk-notify-service.test.ts (2)
21-110: Consider adding tests for environment variable validation.The service implementation validates that both
GOVUK_NOTIFY_API_KEYandGOVUK_NOTIFY_TEMPLATE_ID_MEDIA_APPROVALare configured (seelibs/notification/src/govuk-notify-service.tslines 12-17), but there are no tests verifying these error cases.Consider adding tests like:
it("should throw error when API key is not configured", async () => { // Would need to re-import with different env vars or mock differently // This demonstrates the intent }); it("should throw error when template ID is not configured", async () => { // Similar test for template ID validation });Note: These tests may require restructuring the test file to support different environment configurations, or refactoring the service to accept configuration as a parameter for better testability.
46-50: Optional: Extract duplicated test data.The test data object is duplicated across all three test cases. Consider extracting it to a constant for better maintainability.
+const TEST_MEDIA_APPLICATION_DATA = { + name: "John Smith", + email: "[email protected]", + employer: "BBC" +}; + describe("sendMediaApprovalEmail", () => { it("should send email with correct parameters", async () => { // ... - const testData = { - name: "John Smith", - email: "[email protected]", - employer: "BBC" - }; - - await sendMediaApprovalEmail(testData); + await sendMediaApprovalEmail(TEST_MEDIA_APPLICATION_DATA);Apply similar changes to the other test cases.
Also applies to: 86-90, 101-105
📜 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 (2)
libs/notification/package.json(1 hunks)libs/notification/src/govuk-notify-service.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/notification/package.json
🧰 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/notification/src/govuk-notify-service.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/notification/src/govuk-notify-service.test.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/notification/src/govuk-notify-service.test.ts
🧬 Code graph analysis (1)
libs/notification/src/govuk-notify-service.test.ts (1)
libs/notification/src/govuk-notify-service.ts (1)
sendMediaApprovalEmail(12-32)
🪛 Gitleaks (8.29.1)
libs/notification/src/govuk-notify-service.test.ts
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
libs/notification/src/govuk-notify-service.test.ts (4)
1-19: LGTM! Proper test setup and mocking strategy.The environment variable configuration before import, mock setup, and dynamic import sequence correctly ensure that the module under test uses the mocked dependencies and test configuration.
21-24: LGTM! Clean test suite structure.The
beforeEachhook properly clears mocks between tests to ensure test isolation.
27-65: LGTM! Comprehensive happy path test.The test correctly verifies that
sendMediaApprovalEmailcalls the Notify API with the expected template ID, recipient email, personalisation data, and reference format.
67-108: LGTM! Good error handling coverage.Both API errors and network errors are properly tested to ensure they propagate correctly from the Notify client.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/notification/package.json (1)
26-26: Consider using semantic versioning for vitest.The vitest dependency is pinned to an exact version (
4.0.6) instead of using semantic versioning (^4.0.6), which is inconsistent with the other devDependencies. While exact pinning can ensure reproducibility, consider aligning with the versioning strategy used for other dependencies unless there's a specific reason for the pin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/notification/package.json(1 hunks)tsconfig.json(2 hunks)types/notifications-node-client/index.d.ts(1 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:
types/notifications-node-client/index.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:
types/notifications-node-client/index.d.ts
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.json
**/tsconfig.json
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript must use strict mode with no
anywithout justification.
Files:
tsconfig.json
🧠 Learnings (8)
📚 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.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:
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/*/tsconfig.json : Module tsconfig.json must extend the root tsconfig.json with outDir, rootDir, and include/exclude declarations.
Applied to files:
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:
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 **/tsconfig.json : TypeScript must use strict mode with no `any` without justification.
Applied to files:
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} : 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:
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 package.json : All package.json files must use `"type": "module"` to enforce ES Modules.
Applied to files:
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 **/*.{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:
tsconfig.json
⏰ 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)
tsconfig.json (1)
1-40: Configuration changes are well-structured and compliant.The additions to
tsconfig.jsonfollow TypeScript best practices and align with the PR objectives:
- Strict mode enabled (line 35): Complies with the coding guideline requiring strict mode with no
anywithout justification.- Path mappings (lines 30–31): The new
@hmcts/subscriptionsand@hmcts/notificationaliases follow the established workspace pattern and enable consistent imports across packages.- Type roots (line 6): Adds support for custom types in
./typesalongside node_modules, enabling declaration files from the new notification library to be resolved correctly.- Build exclusion (line 39): Appropriately excludes
vitest.config.tsfrom the TypeScript build output.types/notifications-node-client/index.d.ts (2)
1-31: Well-structured type declaration.The ambient module declaration follows TypeScript conventions correctly. Naming uses PascalCase for the class, camelCase for methods and parameters, and all types are properly defined.
2-30: No action required unless SMS or letter notifications are planned.The type declaration currently includes only the
sendEmailmethod. While the notifications-node-client library does providesendSms,sendLetter, andprepareUpload, the codebase only usessendEmail(verified inlibs/notification/src/govuk-notify-service.ts). The incomplete type declaration matches actual project usage and poses no issues. If SMS or letter notifications are added to the project in the future, add their corresponding type declarations at that time.
- Add tests for media-applications list page (5 tests) - Add tests for application details page (7 tests) - Add tests for approve confirmation page (13 tests) - Add tests for approval success page (5 tests) - Add tests for proof-of-id file download (13 tests) Coverage improved from 42.8% to 96.48% (exceeds 80% requirement) All 290 tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
libs/admin-pages/src/pages/media-applications/[id]/index.test.ts (2)
79-88: Fix mock data to match the actual query return type.Same issue as the English test: the mock includes
phoneNumber(line 84) and usescreatedAt(line 87) instead ofappliedDate.Apply this diff:
const mockApplication = { id: "app-123", name: "John Smith", employer: "BBC", email: "[email protected]", - phoneNumber: "07700900123", proofOfIdPath: "/uploads/proof-app-123.pdf", status: "PENDING" as const, - createdAt: new Date("2024-01-01") + appliedDate: new Date("2024-01-01") };
127-138: Fix mock data to match the actual query return type.Both test cases have the same mock data inconsistencies as the earlier tests:
phoneNumberis included but not returned by the querycreatedAtshould beappliedDateApply this diff to fix both mocks:
const mockApplication = { id: "app-123", name: "John Smith", employer: "BBC", email: "[email protected]", - phoneNumber: "07700900123", proofOfIdPath: "/uploads/proof-app-123.pdf", status: "APPROVED" as const, // or "REJECTED" for the second test - createdAt: new Date("2024-01-01"), + appliedDate: new Date("2024-01-01"), reviewedBy: "[email protected]", reviewedDate: new Date("2024-01-02") };Also applies to: 154-165
🧹 Nitpick comments (14)
libs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.ts (2)
51-255: Reduce duplication with a mock factory function.The
mockApplicationobject is repeated across 9 tests with minor variations. Extract to a factory function for better maintainability.Apply this pattern at the top of the describe block:
+ const createMockApplication = (overrides = {}) => ({ + id: "app-123", + name: "John Smith", + employer: "BBC", + email: "[email protected]", + proofOfIdPath: "/uploads/proof-app-123.pdf", + status: "PENDING" as const, + appliedDate: new Date("2024-01-01"), + ...overrides + }); + beforeEach(() => {Then use it in tests:
it("should serve PDF file successfully", async () => { - const mockApplication = { - id: "app-123", - name: "John Smith", - employer: "BBC", - email: "[email protected]", - phoneNumber: "07700900123", - proofOfIdPath: "/uploads/proof-app-123.pdf", - status: "PENDING" as const, - createdAt: new Date("2024-01-01") - }; + const mockApplication = createMockApplication();
267-270: Consider more explicit middleware verification.The test verifies that
GEThas 2 elements andGET[0]exists, but doesn't confirmGET[0]is therequireRolemiddleware. For stronger validation, you could verify the middleware was called with the correct role.+import { requireRole, USER_ROLES } from "@hmcts/auth"; + // ... in the test: it("should use requireRole middleware with INTERNAL_ADMIN_CTSC role", () => { expect(GET).toHaveLength(2); - expect(GET[0]).toBeDefined(); + expect(requireRole).toHaveBeenCalledWith(USER_ROLES.INTERNAL_ADMIN_CTSC); });libs/admin-pages/src/pages/media-applications/[id]/index.test.ts (2)
92-93: Add assertion to verify getApplicationById was called.For consistency with the English test (line 56), add an assertion to verify that
getApplicationByIdwas called with the correct ID.const handler = GET[1]; await handler(mockRequest as Request, mockResponse as Response, vi.fn()); + expect(getApplicationById).toHaveBeenCalledWith("app-123"); expect(renderSpy).toHaveBeenCalledWith("media-applications/[id]/index", {
194-197: Consider verifying requireRole was called with the correct role.The current test only checks the structure of the GET export. You could make it more robust by verifying that the
requireRolemock was called withINTERNAL_ADMIN_CTSC.+ it("should use requireRole middleware with INTERNAL_ADMIN_CTSC role", async () => { + const { requireRole, USER_ROLES } = await import("@hmcts/auth"); + expect(GET).toHaveLength(2); - expect(GET[0]).toBeDefined(); + expect(requireRole).toHaveBeenCalledWith([USER_ROLES.INTERNAL_ADMIN_CTSC]); });libs/admin-pages/src/pages/media-applications/[id]/approved.test.ts (1)
4-15: Strengthen therequireRolemiddleware assertionsThe last test only checks that
GEThas two entries and thatGET[0]is defined, which doesn’t actually prove thatrequireRoleis used with theINTERNAL_ADMIN_CTSCrole or thatGET[0]is the middleware returned byrequireRole.You can tighten this by importing the mocked
requireRoleand asserting both the role argument and identity of the middleware function, for example:vi.mock("@hmcts/auth", () => ({ requireRole: vi.fn(() => (_req: Request, _res: Response, next: () => void) => next()), USER_ROLES: { INTERNAL_ADMIN_CTSC: "INTERNAL_ADMIN_CTSC" } })); vi.mock("../../../media-application/queries.js", () => ({ getApplicationById: vi.fn() })); +const { requireRole } = await import("@hmcts/auth"); const { getApplicationById } = await import("../../../media-application/queries.js"); const { GET } = await import("./approved.js"); @@ - it("should use requireRole middleware with INTERNAL_ADMIN_CTSC role", () => { - expect(GET).toHaveLength(2); - expect(GET[0]).toBeDefined(); - }); + it("should use requireRole middleware with INTERNAL_ADMIN_CTSC role", () => { + expect(requireRole).toHaveBeenCalledWith("INTERNAL_ADMIN_CTSC"); + expect(GET).toHaveLength(2); + expect(GET[0]).toBe(requireRole.mock.results[0].value); + });This keeps the test resilient while explicitly verifying both the correct role and the wiring of the middleware array.
Please confirm this matches how
GETis constructed inapproved.tsunder your current Vitest/ESM setup.Also applies to: 140-143
docs/tickets/VIBE-228/review.md (4)
211-227: Database index suggestion should consider elevation to high-priority for performance.The suggestions section appropriately identifies non-blocking improvements. However, the database index recommendation (lines 212-227) for the status and email columns may warrant higher priority consideration:
- The media_application table is queried by status frequently (filtering for pending applications)
- As the application list grows, unindexed status queries will degrade significantly
- This is a low-effort fix (2-3 lines in schema) with high long-term benefit
Consider elevating this to the High Priority section or ensuring it's included in the first post-deployment iteration.
458-497: Next steps are well-organized but some testing items duplicate fix categories.The clear categorization by severity (Critical → High Priority → Recommended) with estimated effort makes this actionable. However, lines 480-485 (Testing section) list items that logically belong within the fix categories (e.g., "Verify path traversal fix" is inherent to fixing the critical issue, not a separate task). Consider consolidating testing as part of each priority level's completion criteria rather than a standalone section. Also, per-item time estimates (in addition to category totals) would improve planning accuracy.
55-55: Fix grammar and markdown formatting issues identified by static analysis.Three formatting issues detract from the document's polish:
- Line 55: Change "HIGH PRIORITY Issues" to "HIGH-PRIORITY Issues" (compound adjective requires hyphen)
- Line 193: Remove spaces inside emphasis markers (change " text " to "text" as per markdown linting rules)
- Line 334 context: "new windows" should be "new Windows" (proper noun for the operating system)
- Line 496: Same as line 55 — change "High Priority Items" to "High-Priority Items"
These are minor but improve readability and meet style guidelines.
Also applies to: 193-193, 334-334, 496-496
487-496: Overall assessment appropriately balances severity, timing, and next steps.The "NEEDS CHANGES" status with clear guidance that deployment is possible after critical and high-priority fixes is the right call. The assessment correctly identifies that this is good work with a critical blocker (path traversal), not a systemic quality issue. The 2-4 hour critical / 4-8 hour high-priority time estimates provide realistic expectations for remediation effort.
One suggestion: Add a note about regression testing scope — specifically, which existing test suites should be re-run after the path traversal fix to ensure no unexpected side effects.
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (3)
71-89: Relax full render-context equality to make tests less brittleSeveral tests assert the entire render context object with
toHaveBeenCalledWith. Any future addition of template params will break these tests even if behaviour is unchanged. You can focus the assertions on the important bits usingexpect.objectContainingfor the second argument:- expect(renderSpy).toHaveBeenCalledWith("media-applications/[id]/approve", { - pageTitle: "Are you sure you want to approve this application?", - subheading: "Applicant's details", - tableHeaders: { - name: "Name", - email: "Email", - employer: "Employer", - dateApplied: "Date applied" - }, - radioLegend: "Confirm approval", - radioOptions: { - yes: "Yes", - no: "No" - }, - continueButton: "Continue", - application: mockApplication, - hideLanguageToggle: true - }); + expect(renderSpy).toHaveBeenCalledWith( + "media-applications/[id]/approve", + expect.objectContaining({ + pageTitle: "Are you sure you want to approve this application?", + subheading: "Applicant's details", + tableHeaders: { + name: "Name", + email: "Email", + employer: "Employer", + dateApplied: "Date applied" + }, + radioLegend: "Confirm approval", + radioOptions: { + yes: "Yes", + no: "No" + }, + continueButton: "Continue", + application: mockApplication, + hideLanguageToggle: true + }) + );Same pattern could be applied to the Welsh GET test, the POST validation-error render, and the DB-error renders.
Also applies to: 111-129, 177-195, 317-322
55-64: DeduplicatemockApplicationconstruction and align shape with real query typeEach test builds a near-identical
mockApplicationobject. That’s fine functionally, but it’s a little noisy and easy for the mocked shape to drift from the realMediaApplicationDetails(e.g.appliedDatevscreatedAt, extra fields, etc., pergetApplicationByIdinmedia-application/queries.ts). Pulling this into a small factory would tighten things up and keep the test doubles closer to reality:-import type { Request, Response } from "express"; +import type { Request, Response } from "express"; +import type { MediaApplicationDetails } from "../../../media-application/model.js"; @@ describe("media-application approve page", () => { @@ - beforeEach(() => { + const createMockApplication = ( + overrides: Partial<MediaApplicationDetails> = {} + ): MediaApplicationDetails => ({ + id: "app-123", + name: "John Smith", + employer: "BBC", + email: "[email protected]", + proofOfIdPath: "/uploads/proof-app-123.pdf", + status: "PENDING", + appliedDate: new Date("2024-01-01"), + ...overrides + }); + + beforeEach(() => { @@ - const mockApplication = { - id: "app-123", - name: "John Smith", - employer: "BBC", - email: "[email protected]", - phoneNumber: "07700900123", - proofOfIdPath: "/uploads/proof-app-123.pdf", - status: "PENDING" as const, - createdAt: new Date("2024-01-01") - }; + const mockApplication = createMockApplication();…and similarly in the other tests. This keeps the mock consistent with what
getApplicationByIdreally returns while making the tests easier to read.Also applies to: 95-104, 160-169, 199-208, 221-230, 250-259, 274-283
326-334: Auth-middleware tests don’t currently verify role usage as describedThe test names say “use requireRole middleware with INTERNAL_ADMIN_CTSC role”, but the assertions only check that
GET/POSTare arrays of length 2 with a first element defined. That will catch the absence of any middleware, but not an incorrect role or a different auth wrapper.Either relax the descriptions to match what’s asserted:
- it("should use requireRole middleware with INTERNAL_ADMIN_CTSC role for GET", () => { + it("should expose GET as [authMiddleware, handler]", () => { @@ - it("should use requireRole middleware with INTERNAL_ADMIN_CTSC role for POST", () => { + it("should expose POST as [authMiddleware, handler]", () => {or, if you want to assert the role explicitly, you’d need to avoid clearing the
@hmcts/authmock calls made at module initialisation (or rework the mocking strategy) so you can inspectrequireRole’s arguments.libs/admin-pages/src/pages/media-applications/index.test.ts (2)
35-72: Align mocked application shape withgetPendingApplications’ real return type
getPendingApplicationsreturns anappliedDatefield (perlibs/admin-pages/src/media-application/queries.ts), but the mocks here usecreatedAt. That means the test won’t catch mapping bugs between the query result and the view model. Consider either:
- Mocking
appliedDateand asserting that the handler maps it to whatever the template expects, or- Updating the query/handler so the field names genuinely align and the mock mirrors that contract.
This will make the test a better guard against schema drift between the DB layer and the page.
121-133: Consider asserting HTTP status in the error-path test (if applicable)If the intended behaviour on a database failure is to return a non‑200 status (for example
500), it’s worth extending this test to also assert the status code in addition to the rendered view and model. That would tighten coverage of the error path and prevent regressions where the page continues to render but the status silently changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/tickets/VIBE-228/review.md(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approved.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/index.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/index.test.ts(1 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/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.test.tslibs/admin-pages/src/pages/media-applications/index.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/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.test.tslibs/admin-pages/src/pages/media-applications/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/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.test.tslibs/admin-pages/src/pages/media-applications/index.test.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/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.test.tslibs/admin-pages/src/pages/media-applications/index.test.ts
🧠 Learnings (3)
📚 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 **/pages/**/*.ts : Page controllers must export named `GET` and/or `POST` functions with Express Request and Response types.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/admin-pages/src/pages/media-applications/[id]/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.test.tslibs/admin-pages/src/pages/media-applications/index.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 **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/approved.test.tslibs/admin-pages/src/pages/media-applications/index.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/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.tslibs/admin-pages/src/pages/media-applications/index.test.ts
🧬 Code graph analysis (5)
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (2)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-40)libs/admin-pages/src/media-application/service.ts (1)
approveApplication(6-22)
libs/admin-pages/src/pages/media-applications/[id]/index.test.ts (1)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-40)
libs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.ts (1)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-40)
libs/admin-pages/src/pages/media-applications/[id]/approved.test.ts (1)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-40)
libs/admin-pages/src/pages/media-applications/index.test.ts (1)
libs/admin-pages/src/media-application/queries.ts (1)
getPendingApplications(4-21)
🪛 LanguageTool
docs/tickets/VIBE-228/review.md
[uncategorized] ~55-~55: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...send("Invalid file path"); } ``` ##
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~334-~334: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ...- ARIA labels on links that open in new windows - Error summary component for validatio...
(A_WINDOWS)
[uncategorized] ~496-~496: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ssues**: 2-4 hours Estimated Time for High Priority Items: 4-8 hours --- **Next Review ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
docs/tickets/VIBE-228/review.md
193-193: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
⏰ 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: E2E Tests
- GitHub Check: Test Changed Packages
🔇 Additional comments (6)
libs/admin-pages/src/pages/media-applications/[id]/proof-of-id.test.ts (1)
1-24: LGTM!The mock setup properly isolates external dependencies and follows Vitest patterns. The
.jsextensions on imports comply with the ESM requirements.libs/admin-pages/src/pages/media-applications/[id]/approved.test.ts (1)
16-139: Solid coverage of GET handler behaviour and localisationThe tests for the GET handler cover the main paths well: happy-path English, Welsh via
?lng=cy, not-found (404), and DB failure with an error message. The mocking ofgetApplicationByIdand the render/status spies are set up cleanly and keep the tests focused on controller behaviour rather than implementation details. This aligns nicely with the requirement to exercise Welsh content using?lng=cy.Based on learnings and coding guidelines, this file meets the Vitest and bilingual coverage expectations.
docs/tickets/VIBE-228/review.md (2)
389-456: Acceptance criteria verification is thorough and well-structured.The mapping of all 8 acceptance criteria to implementation, with traceability to specific files and test evidence, demonstrates comprehensive validation. The document appropriately flags the inline styles issue identified in AC4 as connected to the high-priority concerns. The honest acknowledgment that AC7 (Reject flow) is a placeholder is appropriate for the current scope.
One minor enhancement: Consider adding a column or notation for "Verified by" test files to make the E2E/unit test traceability even more explicit.
304-356: Positive feedback section is well-balanced and appropriately detailed.The comprehensive positive feedback across eight dimensions (database design, type safety, testing, security, accessibility, GOV.UK standards, code organization, and DX) demonstrates thorough appreciation for solid engineering practices. Specific examples (e.g., "good use of const assertions for status enum," "comprehensive unit tests with 100% coverage") are motivating while maintaining professional credibility. This balance against identified issues is appropriate and helpful.
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (1)
53-324: Good end-to-end coverage of approve page GET/POST flowsThe suite exercises the key happy paths and failure modes (en/cy rendering, validation, “no” vs “yes”, reviewer email fallback, notification failure, 404 and DB errors) via the actual
GET[1]/POST[1]handlers, which is a solid level of coverage for this page. I don’t see correctness issues in the current assertions.libs/admin-pages/src/pages/media-applications/index.test.ts (1)
74-105: Welsh rendering test covers the bilingual requirement wellThis test explicitly sets
?lng=cyand asserts the Welsh copy andhideLanguageToggleflag, which satisfies the requirement that pages support and test Welsh content via the query parameter. No changes needed here; just keep these strings in sync with the page content definitions as they evolve.Based on learnings, this correctly exercises the Welsh content path.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e-tests/tests/media-application-approval.spec.ts (1)
121-159: Decouple approval tests from shared state to reduce flakinessBoth
should complete approval workflowandshould not show approved application in pending listcurrently approve the sameapplicationId, and they share the same global fixture row. This creates cross-test coupling and potential flakiness if tests are ever reordered or run in parallel.Consider making the “not show approved application” test self-contained by creating and cleaning up its own temporary PENDING application inside the test (similar to your pattern in the accessibility suite), instead of reusing and re-approving the shared one.
🧹 Nitpick comments (2)
e2e-tests/tests/media-application-approval.spec.ts (2)
81-81: Prefer simple URL or predicate matching over dynamic RegExpFor the
waitForURL(new RegExp(...))calls that only need to match a concrete path (e.g./media-applications/${applicationId}/approved), you can simplify to either the string form or a small predicate:await page.waitForURL(`/media-applications/${applicationId}/approved`); // or await page.waitForURL(url => url.includes(`/media-applications/${applicationId}/approved`));This keeps the intent clearer and avoids static-analysis noise around constructing regular expressions from variables.
Also applies to: 97-97, 117-117, 129-129, 151-151, 250-250
263-293: Welsh-language tests align with bilingual page guidelinesThe
?lng=cytests for list, details, and approval confirmation headers directly exercise the Welsh content paths for these pages and match the bilingual-support guideline from CLAUDE.md.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/media-application-approval.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/media-application-approval.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/media-application-approval.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/media-application-approval.spec.ts
🧠 Learnings (2)
📚 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:
e2e-tests/tests/media-application-approval.spec.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 **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.
Applied to files:
e2e-tests/tests/media-application-approval.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/media-application-approval.spec.ts (1)
e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
🪛 ast-grep (0.40.0)
e2e-tests/tests/media-application-approval.spec.ts
[warning] 80-80: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 96-96: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approve)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 116-116: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 128-128: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 150-150: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 249-249: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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 (6)
e2e-tests/tests/media-application-approval.spec.ts (6)
5-36: Test data setup/teardown looks solidCreating a single fixture media application in
beforeAlland cleaning it up inafterAllvia Prisma is clear and keeps DB noise low. The defensive delete with.catch(() => {})is appropriate for tests where intermediate steps may remove the row already.
38-61: CTSC admin login and basic navigation are well coveredThe CTSC admin
beforeEachplus the dashboard tile and navigation checks give good smoke coverage of the new media applications entry point without over-constraining UI details.
62-88: Pending list and details flow are reasonably exercisedUsing the truncated
applicationId.substring(0, 8)to find the row and then driving into the details page verifies the end-to-end wiring from DB → list → details. The specific applicant fields asserted are a good minimal set and help catch regression in the mapping.
90-119: Approval entry, validation, and “No” branch are covered wellThe tests around navigating to
/approve, triggering the validation error with no selection, and asserting the “No” path returns to the details page provide solid coverage for the main branches of the confirmation step.
162-192: RBAC scenarios for Local Admin are appropriately assertedThe two Local Admin tests cover both absence of the tile and blocked direct navigation to
/media-applications, which should be enough to catch regressions in access control without relying on specific error wording.
194-261: Accessibility coverage is strong and uses Axe appropriatelyRunning Axe scans across list, details, approve, and approved pages (with a fresh, disposable application for the final case) gives good a11y confidence. The targeted
disableRuleslist is small and clearly focused on known GOV.UK patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e-tests/tests/media-application-approval.spec.ts (2)
185-195: Consider asserting a more specific RBAC outcome for Local AdminRight now this test only asserts that the
<h1>does not contain “Select application to assess”. It would be stronger to also assert the expected unauthorized experience (for example, a specific “not authorised” heading or redirect target), so failures are easier to diagnose and less likely to pass on an unrelated page.
41-45: Optional: factor repeated CTSC admin login into a small helperThe CTSC admin login flow (
goto("/admin-dashboard")+loginWithSSO+waitForURL("/admin-dashboard")) is repeated in threebeforeEachblocks. You could wrap this in a local helper likeasync function loginAsCTSCAdmin(page, context?)to reduce duplication and keep future changes to the login flow in one place, while still reusing the sharedloginWithSSOutility.Also applies to: 199-203, 256-260
libs/admin-pages/src/pages/media-applications/index.njk (1)
18-36: Row-building logic is fine but could be simplified or moved server‑sideThe
rowsaccumulation usingrows.push([...])inside the loop is valid Nunjucks/JS and will produce the correct table rows, but it’s quite imperative and mixes data shaping with presentation.If you want to simplify, you could either:
- Build the
rowsarray in the route/controller and pass it directly into the template, or- Use a more declarative structure (e.g. compute
rowsin code, or at least keep the loop but avoid the push/comma pattern).Not urgent, but it would make the template a bit easier to read and test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e-tests/tests/create-media-account.spec.ts(1 hunks)e2e-tests/tests/media-application-approval.spec.ts(1 hunks)libs/admin-pages/src/pages/media-applications/index.njk(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/pages/**/*.njk
📄 CodeRabbit inference engine (CLAUDE.md)
Nunjucks templates must extend
layouts/base-templates.njkand use GOV.UK component macros (govukButton, govukInput, govukErrorSummary, etc.).
Files:
libs/admin-pages/src/pages/media-applications/index.njk
**/*.{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/media-application-approval.spec.tse2e-tests/tests/create-media-account.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/media-application-approval.spec.tse2e-tests/tests/create-media-account.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/media-application-approval.spec.tse2e-tests/tests/create-media-account.spec.ts
🧠 Learnings (2)
📚 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 **/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk` and use GOV.UK component macros (govukButton, govukInput, govukErrorSummary, etc.).
Applied to files:
libs/admin-pages/src/pages/media-applications/index.njk
📚 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 **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.
Applied to files:
e2e-tests/tests/media-application-approval.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/media-application-approval.spec.ts (1)
e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
🪛 ast-grep (0.40.0)
e2e-tests/tests/media-application-approval.spec.ts
[warning] 78-78: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 94-94: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approve)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 114-114: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 126-126: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 159-159: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 243-243: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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 (4)
e2e-tests/tests/create-media-account.spec.ts (1)
188-197: Assertion update tonameis correct and consistent with the model renameUsing
application?.namehere matches the underlying Prisma field rename fromfullNametonameand is consistent with how the test populates the form and verifies other fields. No further changes needed in this block.e2e-tests/tests/media-application-approval.spec.ts (3)
8-38: Test data setup/teardown with Prisma looks solidCreating a single shared
mediaApplicationinbeforeAlland cleaning it up inafterAllgives all scenarios a stable fixture while keeping DB noise low. The defensivecatchon delete is reasonable for test cleanup, so this block looks good as-is.
144-171: Good isolation for “approved not in pending list” scenarioCreating a fresh
mediaApplicationinside this test, driving it through approval, asserting absence from the pending list, and then deleting it keeps this scenario isolated from the sharedapplicationIdand avoids inter-test coupling. This directly addresses the earlier concern about reusing the same application across approval tests.
255-279: Welsh coverage aligns with bilingual page requirementsThe
?lng=cytests for list, details, and approval confirmation pages validate the expected Welsh headings, which matches the requirement that admin pages support both English and Welsh content and that Welsh is explicitly tested.Based on learnings, this is exactly the kind of coverage we want to see.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/notification/src/govuk-notify-service.test.ts(1 hunks)libs/notification/src/govuk-notify-service.ts(1 hunks)libs/notification/tsconfig.json(1 hunks)libs/web-core/src/assets/js/search-autocomplete.test.ts(2 hunks)types/notifications-node-client/index.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/web-core/src/assets/js/search-autocomplete.test.ts
- libs/notification/tsconfig.json
- libs/notification/src/govuk-notify-service.ts
- types/notifications-node-client/index.d.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/notification/src/govuk-notify-service.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/notification/src/govuk-notify-service.test.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/notification/src/govuk-notify-service.test.ts
🧬 Code graph analysis (1)
libs/notification/src/govuk-notify-service.test.ts (1)
libs/notification/src/govuk-notify-service.ts (1)
sendMediaApprovalEmail(12-29)
🪛 Gitleaks (8.29.1)
libs/notification/src/govuk-notify-service.test.ts
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
libs/notification/src/govuk-notify-service.test.ts (1)
14-107: Tests correctly exercise happy path and error propagation for GOV.UK Notify integrationThe mocking pattern (
vi.mock+ dynamic import) looks sound, and the tests cover:
- Correct template ID, recipient, personalisation, and
media-approval-reference on success.- Propagation of API errors with the original message.
- Propagation of network errors without re‑wrapping.
This aligns well with the current
sendMediaApprovalEmailimplementation and gives good confidence around Notify integration behaviour.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/admin-pages/src/pages/media-applications/index.ts (1)
8-27: Optional: hardenlnghandling and de-duplicate the view nameYou might want to (a) handle
lngbeing an array more defensively, and (b) extract"media-applications/index"into a top-level SCREAMING_SNAKE_CASE constant to avoid duplication and follow the constants-at-top guideline.-import { getPendingApplications } from "../../media-application/queries.js"; -import cy from "./cy.js"; -import en from "./en.js"; - -const getHandler = async (req: Request, res: Response) => { - const lang = req.query.lng === "cy" ? cy : en; +import { getPendingApplications } from "../../media-application/queries.js"; +import cy from "./cy.js"; +import en from "./en.js"; + +const MEDIA_APPLICATIONS_INDEX_VIEW = "media-applications/index"; + +const getHandler = async (req: Request, res: Response) => { + const lngParam = req.query.lng; + const isWelsh = Array.isArray(lngParam) ? lngParam.includes("cy") : lngParam === "cy"; + const lang = isWelsh ? cy : en; @@ - res.render("media-applications/index", { + res.render(MEDIA_APPLICATIONS_INDEX_VIEW, { @@ - res.render("media-applications/index", { + res.render(MEDIA_APPLICATIONS_INDEX_VIEW, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/admin-pages/src/pages/media-applications/index.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:
libs/admin-pages/src/pages/media-applications/index.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/media-applications/index.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/media-applications/index.ts
🧠 Learnings (2)
📚 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 **/pages/**/*.ts : Page controllers must export named `GET` and/or `POST` functions with Express Request and Response types.
Applied to files:
libs/admin-pages/src/pages/media-applications/index.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 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/admin-pages/src/pages/media-applications/index.ts
🧬 Code graph analysis (1)
libs/admin-pages/src/pages/media-applications/index.ts (1)
libs/admin-pages/src/media-application/queries.ts (1)
getPendingApplications(4-21)
🔇 Additional comments (1)
libs/admin-pages/src/pages/media-applications/index.ts (1)
1-31: Route structure and localisation look solidThis page controller follows the project conventions: named
GETexport, role guard in front of the handler, correct use of.jsrelative imports, and clean en/cy content selection with a safe default to English. The success and error render paths are coherent and avoid leaking error details. Based on learnings, this aligns with the page-controller requirements for**/pages/**/*.ts.
junaidiqbalmoj
force-pushed
the
vibe-228-approve-media-application
branch
from
1fae8a0 to
dc693ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
libs/admin-pages/src/pages/media-applications/index.ts (1)
21-28: Consider logging the error for observability.The error is silently discarded. While the user sees a friendly message, consider adding structured logging (without sensitive data) to aid debugging in production.
- } catch (_error) { + } catch (error) { + console.error("Failed to load pending applications", { error: (error as Error).message }); res.render("media-applications/index", {libs/admin-pages/src/media-application/service.ts (1)
6-22: Race condition: concurrent approvals could bypass status check.The check-then-update pattern creates a TOCTOU window. Two concurrent requests could both read
PENDINGand proceed with approval, potentially causing duplicate notifications.Consider using an atomic update with a
whereclause that includes the status check:export async function approveApplication(id: string): Promise<void> { - const application = await getApplicationById(id); - - if (!application) { - throw new Error("Application not found"); - } - - if (application.status !== APPLICATION_STATUS.PENDING) { - throw new Error("Application has already been reviewed"); - } - - await updateApplicationStatus(id, APPLICATION_STATUS.APPROVED); + // Atomic update with status check - returns null if not found or not PENDING + const application = await prisma.mediaApplication.update({ + where: { id, status: APPLICATION_STATUS.PENDING }, + data: { status: APPLICATION_STATUS.APPROVED }, + select: { proofOfIdPath: true } + }).catch((e) => { + if (e.code === 'P2025') return null; // Record not found + throw e; + }); + + if (!application) { + throw new Error("Application not found or already reviewed"); + } if (application.proofOfIdPath) { await deleteProofOfIdFile(application.proofOfIdPath); } }apps/postgres/prisma/schema.prisma (1)
52-52: Consider adding an index onstatusfor query performance.The
getPendingApplicationsquery filters by status. As data grows, an index would help.appliedDate DateTime @default(now()) @map("applied_date") + @@index([status]) @@map("media_application") }libs/admin-pages/src/media-application/queries.ts (1)
40-40: Strengthen type safety for the status parameter.The
statusparameter is typed asstringinstead of the more specificApplicationStatustype. This weakens type safety and could allow invalid status values at compile time.Apply this diff:
-export async function updateApplicationStatus(id: string, status: string): Promise<MediaApplicationDetails> { +export async function updateApplicationStatus(id: string, status: ApplicationStatus): Promise<MediaApplicationDetails> {Don't forget to import
ApplicationStatusfrom./model.json line 2:-import { APPLICATION_STATUS, type MediaApplicationDetails, type PendingApplicationSummary } from "./model.js"; +import { APPLICATION_STATUS, type ApplicationStatus, type MediaApplicationDetails, type PendingApplicationSummary } from "./model.js";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/postgres/prisma/migrations/20251201160103_remove_reviewed_fields_from_media_application/migration.sql(1 hunks)apps/postgres/prisma/schema.prisma(1 hunks)e2e-tests/tests/media-application-approval.spec.ts(1 hunks)libs/admin-pages/src/media-application/model.ts(1 hunks)libs/admin-pages/src/media-application/queries.test.ts(1 hunks)libs/admin-pages/src/media-application/queries.ts(1 hunks)libs/admin-pages/src/media-application/service.test.ts(1 hunks)libs/admin-pages/src/media-application/service.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.ts(1 hunks)libs/admin-pages/src/pages/media-applications/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/admin-pages/src/media-application/queries.test.ts
- libs/admin-pages/src/media-application/service.test.ts
- libs/admin-pages/src/pages/media-applications/[id]/approve.test.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/admin-pages/src/pages/media-applications/index.tse2e-tests/tests/media-application-approval.spec.tslibs/admin-pages/src/media-application/model.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/media-application/queries.tslibs/admin-pages/src/media-application/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/admin-pages/src/pages/media-applications/index.tse2e-tests/tests/media-application-approval.spec.tslibs/admin-pages/src/media-application/model.tslibs/admin-pages/src/pages/media-applications/[id]/approve.tslibs/admin-pages/src/media-application/queries.tslibs/admin-pages/src/media-application/service.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/media-applications/index.tslibs/admin-pages/src/pages/media-applications/[id]/approve.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/media-application-approval.spec.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:
apps/postgres/prisma/schema.prisma
🧠 Learnings (7)
📚 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 **/pages/**/*.ts : Page controllers must export named `GET` and/or `POST` functions with Express Request and Response types.
Applied to files:
libs/admin-pages/src/pages/media-applications/index.tslibs/admin-pages/src/pages/media-applications/[id]/approve.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 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/admin-pages/src/pages/media-applications/index.tslibs/admin-pages/src/media-application/queries.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 **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.
Applied to files:
e2e-tests/tests/media-application-approval.spec.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 libs/**/locales/**/*.ts : Shared content (common button text, phase banners, service names, common error messages) goes in locale files (`libs/[module]/src/locales/en.ts` and `cy.ts`). Page-specific content goes in controllers.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.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} : Never put sensitive data in logs.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.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} : Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Applied to files:
libs/admin-pages/src/media-application/queries.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 **/prisma/schema.prisma : Database tables and fields MUST be singular and snake_case (e.g., `user`, `case`, `created_at`). Use Prisma `@map` and `map` for aliases.
Applied to files:
apps/postgres/prisma/schema.prisma
🧬 Code graph analysis (5)
libs/admin-pages/src/pages/media-applications/index.ts (5)
libs/admin-pages/src/media-application/queries.ts (1)
getPendingApplications(4-21)libs/admin-pages/src/pages/media-applications/[id]/approve.ts (1)
GET(101-101)libs/admin-pages/src/pages/admin-dashboard/index.ts (1)
GET(27-27)libs/admin-pages/src/pages/media-applications/[id]/approved.ts (1)
GET(39-39)libs/admin-pages/src/pages/media-applications/[id]/index.ts (1)
GET(51-51)
e2e-tests/tests/media-application-approval.spec.ts (1)
e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
libs/admin-pages/src/pages/media-applications/[id]/approve.ts (4)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-38)libs/admin-pages/src/media-application/service.ts (1)
approveApplication(6-22)libs/notification/src/govuk-notify-service.ts (1)
sendMediaApprovalEmail(12-29)libs/admin-pages/src/pages/media-applications/index.ts (1)
GET(31-31)
libs/admin-pages/src/media-application/queries.ts (1)
libs/admin-pages/src/media-application/model.ts (3)
PendingApplicationSummary(19-24)APPLICATION_STATUS(1-5)MediaApplicationDetails(9-17)
libs/admin-pages/src/media-application/service.ts (2)
libs/admin-pages/src/media-application/queries.ts (2)
getApplicationById(23-38)updateApplicationStatus(40-58)libs/admin-pages/src/media-application/model.ts (1)
APPLICATION_STATUS(1-5)
🪛 ast-grep (0.40.0)
e2e-tests/tests/media-application-approval.spec.ts
[warning] 78-78: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 94-94: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approve)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 114-114: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 126-126: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 157-157: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 241-241: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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 (16)
libs/admin-pages/src/pages/media-applications/index.ts (1)
1-31: LGTM!The implementation follows established patterns: correct role guard, language handling via query param, proper error recovery with empty state, and ESM imports with
.jsextensions. Structure aligns with related page controllers inlibs/admin-pages/src/pages/media-applications/[id]/.apps/postgres/prisma/schema.prisma (1)
46-56: LGTM! Schema follows conventions.Fields correctly use snake_case mapping (
proof_of_id_path,applied_date), table is singular (media_application), and VarChar constraints are appropriate.libs/admin-pages/src/media-application/model.ts (1)
1-33: LGTM!Well-structured type definitions:
APPLICATION_STATUSuses SCREAMING_SNAKE_CASE withas constfor type safetyApplicationStatuscorrectly derives from the const- Interfaces use PascalCase without
Iprefix- Session augmentation is properly scoped for the approval flow
libs/admin-pages/src/pages/media-applications/[id]/approve.ts (4)
1-9: LGTM!Imports follow coding guidelines correctly: workspace aliases for cross-package imports and
.jsextensions on relative imports.
11-42: LGTM!GET handler follows best practices: language support via
lngquery parameter, proper 404 handling, and graceful error rendering without exposing details.
44-99: LGTM!POST handler implements the approval workflow correctly: validates user confirmation, handles yes/no choices, calls the service layer for approval, and sends a notification email without failing the approval if email delivery fails. The non-fatal email behavior is appropriate for this use case.
101-102: LGTM!Exports follow the established pattern with proper role-based access control using
requireRolemiddleware forINTERNAL_ADMIN_CTSC.libs/admin-pages/src/media-application/queries.ts (4)
1-2: LGTM!Imports follow coding guidelines: workspace alias for Prisma and
.jsextension on relative import.
4-21: LGTM!Proper Prisma query with type-safe select, filtering by
PENDINGstatus, and sensible descending date ordering.
23-38: LGTM!Proper Prisma query with explicit field selection matching
MediaApplicationDetails. The type assertion on line 37 aligns the Prisma return type with the interface.
60-68: LGTM!Straightforward Prisma count query filtering by
PENDINGstatus.e2e-tests/tests/media-application-approval.spec.ts (5)
1-38: LGTM!Test setup and teardown follow good practices: dynamic Prisma import, proper cleanup with error handling, and storage of test data ID for use across tests.
40-170: LGTM!Comprehensive test coverage of the CTSC admin approval workflow: dashboard integration, navigation, form validation, approval paths (yes/no), database verification, and test isolation. The test at lines 142-169 properly creates fresh test data to avoid dependencies between tests.
Note: Static analysis warnings about
RegExpfrom variables (lines 78, 94, 114, 126, 157) are false positives—the IDs are controlled UUIDs from Prisma, not user input.
172-194: LGTM!Role-based access control tests properly verify that Local Admin role cannot access media applications, testing both UI visibility and direct navigation attempts.
196-251: LGTM!Thorough accessibility coverage using Axe across all workflow pages. Properly creates and cleans up fresh test data for the success page test.
253-277: LGTM!Welsh language support tests properly validate localized content across all workflow pages using the
lng=cyquery parameter, as per coding guidelines.
...prisma/migrations/20251201160103_remove_reviewed_fields_from_media_application/migration.sql
Show resolved
Hide resolved
…om/hmcts/cath-service into vibe-228-approve-media-application
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/notification/src/govuk-notify-service.test.ts (1)
3-5: Test API key literal still trips secret scanners and leaks across suites (non‑blocking)The hard‑coded
GOVUK_NOTIFY_API_KEY("test-api-key-12345") continues to look realistic enough for tools like Gitleaks to flag it, and it’s never restored after tests. If you decide to revisit this later, consider:
- Using a more obviously dummy value (e.g.
"dummy-govuk-notify-key"), and- Capturing the original env values and restoring them in an
afterAllto avoid leaking config into other suites.Not blocking, just something to be aware of if secret‑scanning noise becomes an issue.
🧹 Nitpick comments (1)
libs/notification/src/govuk-notify-service.ts (1)
3-12: Minor style: moveMediaApplicationEmailDatabelow exports to match module ordering guidelineCurrent order is:
- Constants (
GOVUK_NOTIFY_API_KEY,TEMPLATE_ID_MEDIA_APPROVAL)MediaApplicationEmailDatainterface- Exported
sendMediaApprovalEmailfunctionPer the stated guideline, the preferred order is: constants → exported functions → types/interfaces at the bottom. Consider moving
MediaApplicationEmailDatabelow the function:-const GOVUK_NOTIFY_API_KEY = process.env.GOVUK_NOTIFY_API_KEY; -const TEMPLATE_ID_MEDIA_APPROVAL = process.env.GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_APPROVAL; - -interface MediaApplicationEmailData { - name: string; - email: string; - employer: string; -} - -export async function sendMediaApprovalEmail(data: MediaApplicationEmailData): Promise<void> { +const GOVUK_NOTIFY_API_KEY = process.env.GOVUK_NOTIFY_API_KEY; +const TEMPLATE_ID_MEDIA_APPROVAL = process.env.GOVUK_NOTIFY_TEMPLATE_ID_MEDIA_APPROVAL; + +export async function sendMediaApprovalEmail(data: MediaApplicationEmailData): Promise<void> { // ... } + +interface MediaApplicationEmailData { + name: string; + email: string; + employer: string; +}Purely organisational, but keeps this module consistent with the documented conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/notification/src/govuk-notify-service.test.ts(1 hunks)libs/notification/src/govuk-notify-service.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:
libs/notification/src/govuk-notify-service.tslibs/notification/src/govuk-notify-service.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/notification/src/govuk-notify-service.tslibs/notification/src/govuk-notify-service.test.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/notification/src/govuk-notify-service.test.ts
🧬 Code graph analysis (2)
libs/notification/src/govuk-notify-service.ts (2)
libs/notification/src/index.ts (1)
sendMediaApprovalEmail(1-1)types/notifications-node-client/index.d.ts (1)
NotifyClient(2-26)
libs/notification/src/govuk-notify-service.test.ts (1)
libs/notification/src/govuk-notify-service.ts (1)
sendMediaApprovalEmail(12-32)
🪛 GitHub Actions: Test
libs/notification/src/govuk-notify-service.ts
[error] 29-29: Type '{ "full name": string; Employer: string; }' is not assignable to type 'string'.
🪛 Gitleaks (8.30.0)
libs/notification/src/govuk-notify-service.test.ts
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (2)
libs/notification/src/govuk-notify-service.ts (1)
28-31: Unable to verify the review comment. The repository could not be accessed to inspect the actual code atlibs/notification/src/govuk-notify-service.tslines 28-31 or the type definitions intypes/notifications-node-client/index.d.ts. Manual verification by the developer is required to confirm:
- Whether the current
sendEmailcall signature matches the actualnotifications-node-clientlibrary API- If the proposed fix (changing from an options object to positional arguments) aligns with the library's type definition
- Whether applying the suggested change resolves the reported type error
Please run
tscafter making any changes to confirm the pipeline passes.libs/notification/src/govuk-notify-service.test.ts (1)
54-60: Test expectation is correct and requires no changesThe test assertion at lines 54–60 correctly matches both the implementation and the local type definitions. The implementation in
govuk-notify-service.ts(line 28) callsnotifyClient.sendEmail()with three arguments:templateId,optionsobject containingpersonalisationandreference.This aligns with the local type definition in
libs/notification/src/notifications-node-client.d.ts(lines 5–13), which definessendEmail(templateId: string, emailAddress: string, options?: {...}).Note: The workspace
types/notifications-node-client/index.d.tsdefines a different signature with five separate parameters, but this appears to be stale or incorrect relative to the actual local implementation and should be updated to matchlibs/notification/src/notifications-node-client.d.ts.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/notification/src/govuk-notify-service.ts (1)
3-4: Narrow env vars via function-scope consts for better TS safety.With
strict: trueenabled, the module-levelconstassignments fromprocess.envhave typestring | undefined. The runtime guards at lines 13–19 don't narrow the type of module-level consts in TypeScript, so the direct uses ofGOVUK_NOTIFY_API_KEYandTEMPLATE_ID_MEDIA_APPROVALat lines 21 and 28 remain typed asstring | undefined.To keep runtime behaviour identical while improving type safety and readability, assign to function-local consts and guard there:
-export async function sendMediaApprovalEmail(data: MediaApplicationEmailData): Promise<void> { - if (!GOVUK_NOTIFY_API_KEY) { - throw new Error("GOV Notify API key not configured"); - } - - if (!TEMPLATE_ID_MEDIA_APPROVAL) { - throw new Error("GOV Notify approval template ID not configured"); - } - - const notifyClient = new NotifyClient(GOVUK_NOTIFY_API_KEY); +export async function sendMediaApprovalEmail(data: MediaApplicationEmailData): Promise<void> { + const apiKey = GOVUK_NOTIFY_API_KEY; + if (!apiKey) { + throw new Error("GOV Notify API key not configured"); + } + + const templateId = TEMPLATE_ID_MEDIA_APPROVAL; + if (!templateId) { + throw new Error("GOV Notify approval template ID not configured"); + } + + const notifyClient = new NotifyClient(apiKey); @@ - await notifyClient.sendEmail(TEMPLATE_ID_MEDIA_APPROVAL, data.email, { + await notifyClient.sendEmail(templateId, data.email, { personalisation, reference: `media-approval-${Date.now()}` }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/admin-pages/src/global.d.ts(1 hunks)libs/notification/src/global.d.ts(1 hunks)libs/notification/src/govuk-notify-service.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/notification/src/govuk-notify-service.tslibs/admin-pages/src/global.d.tslibs/notification/src/global.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/notification/src/govuk-notify-service.tslibs/admin-pages/src/global.d.tslibs/notification/src/global.d.ts
🧬 Code graph analysis (3)
libs/notification/src/govuk-notify-service.ts (3)
libs/admin-pages/src/global.d.ts (1)
NotifyClient(2-14)libs/notification/src/global.d.ts (1)
NotifyClient(2-14)types/notifications-node-client/index.d.ts (1)
NotifyClient(2-26)
libs/admin-pages/src/global.d.ts (1)
libs/notification/src/global.d.ts (1)
NotifyClient(2-14)
libs/notification/src/global.d.ts (1)
libs/admin-pages/src/global.d.ts (1)
NotifyClient(2-14)
⏰ 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 (1)
libs/admin-pages/src/pages/media-applications/[id]/approve.njk (1)
12-18: Previous feedback acknowledged.The hand-coded error summary was previously discussed and marked as acceptable by the developer.
🧹 Nitpick comments (7)
libs/web-core/src/middleware/govuk-frontend/filters/date.test.ts (1)
42-56: Good coverage for new compact format; optional refactor to table-driven testsThe added
"compact"tests exercise Date vs string inputs, single‑digit days with leading zeros, and multiple months, which is solid coverage and aligns with the implementation. If you want to reduce repetition later, you could convert the month and single‑day cases into a small table‑driven test, but that’s purely optional.libs/admin-pages/src/pages/media-applications/[id]/approve.njk (1)
47-55: Consider extracting complex inline HTML to a separate macro or variable.The inline HTML string for proof of ID (line 51) is complex and harder to maintain. While functional, extracting this to a Nunjucks variable or macro would improve readability.
+ {% set proofOfIdHtml %} + <div style="display: flex; justify-content: space-between; align-items: center;"> + <span>{{ proofOfIdFilename }} {{ proofOfIdText }}</span> + <a href="/media-applications/{{ application.id }}/proof-of-id" class="govuk-link" target="_blank" rel="noopener noreferrer" aria-label="Opens in new window">{{ viewProofOfId }}</a> + </div> + {% endset %} + {{ govukSummaryList({ rows: [ ... { key: { text: tableHeaders.proofOfId }, value: { - html: ( - '<div style="display: flex; justify-content: space-between; align-items: center;"><span>' + proofOfIdFilename + ' ' + proofOfIdText + '</span><a href="/media-applications/' + application.id + '/proof-of-id" class="govuk-link" target="_blank" rel="noopener noreferrer" aria-label="Opens in new window">' + viewProofOfId + '</a></div>' if application.proofOfIdPath else fileNotAvailable - ) + html: proofOfIdHtml if application.proofOfIdPath else fileNotAvailable } } ] }) }}libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (2)
55-65: Mock application objects contain extraneous properties.The mock objects include
phoneNumberandcreatedAtproperties, but looking at the relevant code snippet forgetApplicationById, the actual query only selects:id,name,employer,proofOfIdPath,proofOfIdOriginalName,status,appliedDate. The mock should useappliedDateinstead ofcreatedAtand excludephoneNumberto accurately reflect the real data shape.const mockApplication = { id: "app-123", name: "John Smith", employer: "BBC", email: "[email protected]", - phoneNumber: "07700900123", proofOfIdPath: "/uploads/proof-app-123.pdf", proofOfIdOriginalName: "IACListPublishPreviewed.pdf", status: "PENDING" as const, - createdAt: new Date("2024-01-01") + appliedDate: new Date("2024-01-01") };Also applies to: 101-111, 172-182, 217-227, 240-250, 270-280
323-331: Middleware assertions could be more explicit.The tests verify that
GETandPOSTarrays have length 2, but don't actually verify thatrequireRolewas called withINTERNAL_ADMIN_CTSC. SincerequireRoleis mocked, you could assert on its invocation.+import { requireRole } from "@hmcts/auth"; + it("should use requireRole middleware with INTERNAL_ADMIN_CTSC role for GET", () => { expect(GET).toHaveLength(2); expect(GET[0]).toBeDefined(); + expect(requireRole).toHaveBeenCalledWith(["INTERNAL_ADMIN_CTSC"]); });Note: This would require importing the mocked
requireRoleafter the dynamic import pattern you're using.libs/admin-pages/src/pages/media-applications/[id]/index.njk (2)
40-40: Refactor complex inline HTML for maintainability.The inline HTML concatenation is difficult to read and maintain. Consider using Nunjucks template blocks or a separate macro to construct this content.
Example refactor using Nunjucks syntax:
- html: ( - '<div style="display: flex; justify-content: space-between; align-items: center;"><span>' + proofOfIdFilename + ' ' + proofOfIdText + '</span><a href="/media-applications/' + application.id + '/proof-of-id" class="govuk-link" target="_blank" rel="noopener noreferrer" aria-label="Opens in new window">' + viewProofOfId + '</a></div>' if application.proofOfIdPath else fileNotAvailable - ) + html: ( + ('<div class="proof-of-id-row"><span>' + proofOfIdFilename + ' ' + proofOfIdText + '</span><a href="/media-applications/' + application.id + '/proof-of-id" class="govuk-link" target="_blank" rel="noopener noreferrer" aria-label="Opens in new window">' + viewProofOfId + '</a></div>') if application.proofOfIdPath else fileNotAvailable + )Then add CSS class
.proof-of-id-rowto your stylesheet instead of inline styles.
47-47: Replace inline styles with CSS classes.Inline styles should be avoided in favor of reusable CSS classes for better maintainability.
Apply this diff to use a CSS class:
- <div style="display: flex; justify-content: space-between; align-items: center; margin-top: 2rem;"> + <div class="action-buttons">Then define
.action-buttonsin your stylesheet with the appropriate flexbox properties.libs/public-pages/src/pages/create-media-account/index.test.ts (1)
263-273: Consider assertingupdateProofOfIdPathis not called when file is missingIn the “should handle missing file when validation passes” test, you already assert that
saveIdProofFileis not called. Adding a similar assertion forupdateProofOfIdPathwould close the loop on ensuring repository updates are strictly gated on the presence of a file.- expect(saveIdProofFile).not.toHaveBeenCalled(); + expect(saveIdProofFile).not.toHaveBeenCalled(); + expect(updateProofOfIdPath).not.toHaveBeenCalled();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/postgres/prisma/migrations/20251203102403_add_proof_of_id_original_name/migration.sql(1 hunks)apps/postgres/prisma/schema.prisma(1 hunks)libs/admin-pages/src/media-application/model.ts(1 hunks)libs/admin-pages/src/media-application/queries.test.ts(1 hunks)libs/admin-pages/src/media-application/queries.ts(1 hunks)libs/admin-pages/src/media-application/service.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve-cy.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve-en.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.njk(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approve.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/approved.njk(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/index.njk(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/index.test.ts(1 hunks)libs/admin-pages/src/pages/media-applications/[id]/index.ts(1 hunks)libs/public-pages/src/media-application/repository/query.test.ts(3 hunks)libs/public-pages/src/media-application/repository/query.ts(1 hunks)libs/public-pages/src/pages/create-media-account/index.test.ts(9 hunks)libs/public-pages/src/pages/create-media-account/index.ts(3 hunks)libs/web-core/src/middleware/govuk-frontend/filters/date.test.ts(1 hunks)libs/web-core/src/middleware/govuk-frontend/filters/date.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/admin-pages/src/pages/media-applications/[id]/approved.njk
- libs/admin-pages/src/media-application/queries.ts
- libs/admin-pages/src/pages/media-applications/[id]/index.ts
- libs/admin-pages/src/pages/media-applications/[id]/approve.ts
- libs/public-pages/src/media-application/repository/query.ts
- libs/admin-pages/src/media-application/model.ts
- libs/admin-pages/src/media-application/service.test.ts
- libs/admin-pages/src/pages/media-applications/[id]/index.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/web-core/src/middleware/govuk-frontend/filters/date.test.tslibs/public-pages/src/pages/create-media-account/index.test.tslibs/admin-pages/src/media-application/queries.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/public-pages/src/pages/create-media-account/index.tslibs/web-core/src/middleware/govuk-frontend/filters/date.tslibs/public-pages/src/media-application/repository/query.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.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/web-core/src/middleware/govuk-frontend/filters/date.test.tslibs/public-pages/src/pages/create-media-account/index.test.tslibs/admin-pages/src/media-application/queries.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/public-pages/src/pages/create-media-account/index.tslibs/web-core/src/middleware/govuk-frontend/filters/date.tslibs/public-pages/src/media-application/repository/query.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.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/web-core/src/middleware/govuk-frontend/filters/date.test.tslibs/public-pages/src/pages/create-media-account/index.test.tslibs/admin-pages/src/media-application/queries.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/public-pages/src/media-application/repository/query.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/public-pages/src/pages/create-media-account/index.test.tslibs/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/public-pages/src/pages/create-media-account/index.tslibs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.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:
apps/postgres/prisma/schema.prisma
**/pages/**/*.njk
📄 CodeRabbit inference engine (CLAUDE.md)
Nunjucks templates must extend
layouts/base-templates.njkand use GOV.UK component macros (govukButton, govukInput, govukErrorSummary, etc.).
Files:
libs/admin-pages/src/pages/media-applications/[id]/approve.njklibs/admin-pages/src/pages/media-applications/[id]/index.njk
🧠 Learnings (9)
📚 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/web-core/src/middleware/govuk-frontend/filters/date.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/admin-pages/src/pages/media-applications/[id]/approve.test.tslibs/public-pages/src/media-application/repository/query.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 **/pages/**/*.ts : Page controllers must export named `GET` and/or `POST` functions with Express Request and Response types.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.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 **/prisma/schema.prisma : Database tables and fields MUST be singular and snake_case (e.g., `user`, `case`, `created_at`). Use Prisma `@map` and `map` for aliases.
Applied to files:
apps/postgres/prisma/schema.prisma
📚 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 **/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk` and use GOV.UK component macros (govukButton, govukInput, govukErrorSummary, etc.).
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve.njklibs/admin-pages/src/pages/media-applications/[id]/index.njk
📚 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/**/locales/**/*.ts : Shared content (common button text, phase banners, service names, common error messages) goes in locale files (`libs/[module]/src/locales/en.ts` and `cy.ts`). Page-specific content goes in controllers.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve-en.tslibs/admin-pages/src/pages/media-applications/[id]/approve-cy.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 **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve-cy.ts
📚 Learning: 2025-11-20T09:59:16.776Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 106
File: libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts:84-160
Timestamp: 2025-11-20T09:59:16.776Z
Learning: In the cath-service repository, Welsh localization (lng=cy) is not required for admin screens (system-admin-pages), so locale preservation in admin screen redirects is not necessary.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve-cy.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 skip Welsh translations - required for all user-facing text.
Applied to files:
libs/admin-pages/src/pages/media-applications/[id]/approve-cy.ts
🧬 Code graph analysis (3)
libs/public-pages/src/pages/create-media-account/index.test.ts (4)
libs/public-pages/src/media-application/storage.ts (1)
saveIdProofFile(6-16)libs/public-pages/src/pages/create-media-account/index.ts (1)
POST(39-86)libs/public-pages/src/media-application/repository/model.ts (1)
MulterRequest(30-32)libs/public-pages/src/media-application/repository/query.ts (2)
createMediaApplication(4-15)updateProofOfIdPath(17-25)
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (4)
libs/admin-pages/src/media-application/queries.ts (1)
getApplicationById(23-39)libs/admin-pages/src/pages/media-applications/[id]/approve.ts (2)
GET(109-109)POST(110-110)libs/admin-pages/src/pages/media-applications/[id]/index.ts (1)
GET(52-52)libs/admin-pages/src/media-application/service.ts (1)
approveApplication(6-22)
libs/public-pages/src/pages/create-media-account/index.ts (3)
libs/public-pages/src/pages/validation.ts (1)
validateForm(15-96)libs/public-pages/src/media-application/storage.ts (1)
saveIdProofFile(6-16)libs/public-pages/src/media-application/repository/query.ts (1)
updateProofOfIdPath(17-25)
⏰ 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: E2E Tests
- GitHub Check: Test Changed Packages
🔇 Additional comments (25)
libs/web-core/src/middleware/govuk-frontend/filters/date.ts (1)
7-12: Compact date format branch is correct and side‑effect freeThe new
"compact"branch cleanly reuses the existingDateinstance, produces the expected"DD Mon YYYY"output with a zero‑padded day, and doesn’t alter prior"short"/default behaviours. Implementation is consistent with the functional style in this module.libs/admin-pages/src/pages/media-applications/[id]/approve-cy.ts (1)
1-25: LGTM!The Welsh translation module is well-structured, with all keys matching the English counterpart. The translations provide complete coverage for the approval page UI.
libs/admin-pages/src/pages/media-applications/[id]/approve-en.ts (1)
1-25: LGTM!The English translation module is well-structured with all necessary UI strings for the approval page. The structure correctly mirrors the Welsh counterpart for consistency.
libs/admin-pages/src/pages/media-applications/[id]/approve.test.ts (1)
1-51: LGTM on test structure and coverage.The test file is well-organized with comprehensive coverage of:
- GET handler (English/Welsh rendering, 404, error states)
- POST handler (validation, redirect flows, approval + notification, error handling)
- Middleware presence checks
The use of Vitest, dynamic imports after mocking, and
.jsextensions for relative imports all follow project conventions.libs/admin-pages/src/pages/media-applications/[id]/index.njk (1)
1-1: The base template path is correct. The actual template file in the codebase islayouts/base-template.njk(singular), and all 20+ Nunjucks templates consistently extend this same file. The coding guideline in the retrieved learning appears to be outdated or incorrect.Likely an incorrect or invalid review comment.
libs/admin-pages/src/media-application/queries.test.ts (4)
1-15: LGTM! Proper import extensions and comprehensive mock setup.The imports correctly use
.jsextensions as required by ESM with Node.js "nodenext" module resolution, and the Prisma client mock covers all necessary methods for the test suite.
17-47: LGTM! Proper test isolation and comprehensive assertions.The
beforeEachhook ensures test isolation, and thegetPendingApplicationstest correctly verifies both the Prisma method invocation with exact arguments and the return value. The mock data structure properly matches the select clause.
49-89: LGTM! Comprehensive test coverage with proper edge case handling.The tests cover both the success case (application found) and the edge case (application not found), with mock data that accurately matches the select clause structure.
127-146: LGTM! Good coverage of edge cases.The
getPendingCounttests properly cover both the case with pending applications and the case with no pending applications, with clear assertions for each scenario.libs/public-pages/src/pages/create-media-account/index.test.ts (6)
6-9: Mocks for repository and storage functions are correctly wiredThe vi.mock setup and subsequent dynamic imports cleanly cover
createMediaApplication,updateProofOfIdPath,saveIdProofFile, andvalidateForm, so the tests are exercising the same surfaces the controller uses without leaking real implementations.Based on learnings, the Vitest usage and co-located test file layout align with the repo’s testing guidelines.
Also applies to: 19-21
93-101: Session form data usesnameconsistentlyUsing
nameinmediaApplicationFormand in the GET “wasSubmitted” flow lines up with the controller and validation expectations, keeping the session shape consistent after the fullName→name refactor.Also applies to: 113-120
165-177: Validation error path correctly preserves user input and boolean conversionThe POST error-path assertions (including
nameinstead offullNameandtermsAcceptedconverted to boolean) accurately reflect the controller logic and help guard against regressions in how form data is stored in the session.
192-206: Success path test now verifies file save and DB update contractStubbing
saveIdProofFileto return a path and asserting thatupdateProofOfIdPathis called with(applicationId, filePath, originalname)ensures the controller-to-repository contract for proof-of-ID handling is well covered.
208-231: Success redirects and session cleanup are well covered for both localesThe success tests (with and without
lng=cy) verify session flags, cleanup, and redirects, and they correctly factor in the new storage interaction without over-coupling assertions to implementation internals.
275-292: Checkbox boolean handling tests remain accurate after refactorThe tests around
termsAcceptedstill assert boolean conversion correctly in both checked and unchecked scenarios, which is important given the session object shape changed alongside other form fields.apps/postgres/prisma/migrations/20251203102403_add_proof_of_id_original_name/migration.sql (1)
1-2: Migration cleanly adds the new original-name columnAdding
proof_of_id_original_name VARCHAR(255)as a new column onmedia_applicationis a straightforward, backward-compatible change and matches the Prisma mapping in theMediaApplicationmodel.libs/public-pages/src/pages/create-media-account/index.ts (4)
3-5: Repository and helper imports remain consistent with ESM and alias rulesImporting
updateProofOfIdPathalongsidecreateMediaApplication, and keeping.jsextensions on local imports, respects the nodenext/ESM setup and matches the way the repository API is exposed.
43-51:namepropagation through validation, session, and creation is coherentReading
fullNamefrom the request intoname, then usingnameinvalidateForm, inmediaApplicationForm, and in thecreateMediaApplicationpayload keeps the public API and persistence model aligned after the fullName→name change. This matches the updated tests and PrismaMediaApplicationdefinition.Also applies to: 55-59, 66-69
71-74: File save and proof-of-ID update are correctly sequencedConditionally calling
saveIdProofFileand thenupdateProofOfIdPath(applicationId, filePath, file.originalname)only when a file is present is a sensible flow and aligns with the repository’s contract (storing both path and original name). Errors in either step are caught by the surrounding try/catch and surfaced as a 500, which is reasonable for this admin-facing journey.
76-85: Session cleanup and error handling still behave as expectedThe success branch still sets
mediaApplicationSubmitted, clears form and error state, and redirects with the locale, while the catch block returns a 500 error page. This behavior remains consistent with the tests and the previous implementation, with only the new file-path update added into the happy path.libs/public-pages/src/media-application/repository/query.test.ts (4)
3-4: Repository tests correctly mock Prisma and cover both create and updateExtending the Prisma mock with
updateand importingupdateProofOfIdPathensures both repository functions are tested against the same mocked client, with no leakage to the real database. The setup is clear and focused.Based on learnings, the test remains co-located with its source and uses Vitest as required.
Also applies to: 5-12
21-52:createMediaApplicationtests align with new schema fieldsUsing
name, asserting lowercase normalization ofstatus: "PENDING"andappliedDate: expect.any(Date)are passed toprisma.mediaApplication.createkeeps the test tightly coupled to the intended persistence behavior after the schema refactor.
55-82: Email normalization behavior is well isolatedThe second test focusing on lowercasing the email while keeping other fields intact is a good, minimal check that avoids over-specifying unrelated data and makes the intent of
createMediaApplicationclear.
84-111:updateProofOfIdPathtest verifies the critical update payloadThe new test for
updateProofOfIdPathcorrectly asserts thatprisma.mediaApplication.updateis called with the expectedwhereclause and with bothproofOfIdPathandproofOfIdOriginalNameset, which directly mirrors the implementation inquery.tsand the Prisma model.apps/postgres/prisma/schema.prisma (1)
46-57: MediaApplication model matches naming guidelines and migrationsThe
MediaApplicationmodel uses@@map("media_application")and snake_case column mappings (e.g.proof_of_id_path,proof_of_id_original_name,applied_date), with types and lengths consistent with the corresponding migrations. The newproofOfIdOriginalNamefield is correctly optional and mapped toVARCHAR(255), as in the SQL migration.As per coding guidelines, this keeps the Prisma schema aligned with singular, snake_case database naming while exposing idiomatic camelCase fields in TypeScript.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e-tests/tests/media-application-approval.spec.ts (1)
227-250: Consider using try/finally for test cleanup robustness.If the test fails before line 249, the created test record won't be cleaned up. This is a minor issue but can lead to orphaned test data accumulating over time.
test("approved success page should pass accessibility checks", async ({ page }) => { // Create a fresh application for this test const { prisma } = await import("@hmcts/postgres"); const testApp = await prisma.mediaApplication.create({ data: { name: "Test User For Accessibility", email: "[email protected]", employer: "Test Corp", status: "PENDING" } }); - await page.goto(`/media-applications/${testApp.id}/approve`); - await page.check('input[value="yes"]'); - await page.getByRole("button", { name: /continue/i }).click(); - await page.waitForURL(new RegExp(`/media-applications/${testApp.id}/approved`)); - - const accessibilityScanResults = await new AxeBuilder({ page }).disableRules(["target-size", "link-name", "region"]).analyze(); - - expect(accessibilityScanResults.violations).toEqual([]); - - // Cleanup - await prisma.mediaApplication.delete({ where: { id: testApp.id } }); + try { + await page.goto(`/media-applications/${testApp.id}/approve`); + await page.check('input[value="yes"]'); + await page.getByRole("button", { name: /continue/i }).click(); + await page.waitForURL(new RegExp(`/media-applications/${testApp.id}/approved`)); + + const accessibilityScanResults = await new AxeBuilder({ page }).disableRules(["target-size", "link-name", "region"]).analyze(); + + expect(accessibilityScanResults.violations).toEqual([]); + } finally { + await prisma.mediaApplication.delete({ where: { id: testApp.id } }).catch(() => {}); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/media-application-approval.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/media-application-approval.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/media-application-approval.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/media-application-approval.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 **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.
Applied to files:
e2e-tests/tests/media-application-approval.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/media-application-approval.spec.ts (1)
e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
🪛 ast-grep (0.40.0)
e2e-tests/tests/media-application-approval.spec.ts
[warning] 78-78: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 94-94: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approve)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 114-114: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 126-126: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${applicationId}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 157-157: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 241-241: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/media-applications/${testApp.id}/approved)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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)
e2e-tests/tests/media-application-approval.spec.ts (5)
1-4: LGTM!Imports are correctly structured with
.jsextension for local imports as required for ESM with Node.js "nodenext" module resolution.
8-38: LGTM!Test setup and teardown are well-structured with proper error handling for cleanup.
172-194: LGTM!Role-based access control tests are well-structured with proper session isolation using
clearCookies(). The tests correctly verify both tile visibility and direct URL access restrictions.
253-277: LGTM! Welsh language support correctly tested.Tests properly use the
?lng=cyquery parameter to verify Welsh translations across the media applications pages, as required by the coding guidelines. Based on learnings, this aligns with the requirement that Welsh content must be tested with the?lng=cyquery parameter.
78-79: Static analysis ReDoS warnings are false positives in this context.The
applicationIdis a Prisma-generated UUID containing only hex characters and hyphens, which are not regex metacharacters. The ReDoS risk flagged by static analysis does not apply here since the variable content is controlled test data.
2 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-228
Change description
Approve Media User Application Journey in Admin Dashboard
Summary by CodeRabbit
New Features
Database
Tests
✏️ Tip: You can customize this high-level summary in your review settings.