VIBE-175 Verified user- Account creation

[alexbottenberg]

Jira link

https://tools.hmcts.net/jira/browse/VIBE-175

Change description

Account creation for Verified user

Testing done

Security Vulnerability Assessment

CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?

• Yes
• No

Checklist

• commit messages are meaningful and follow good commit message guidelines
• README and other documentation has been updated / added (if needed)
• tests have been updated / new tests has been added (if needed)
• Does this PR introduce a breaking change

Summary by CodeRabbit

• New Features

  • Verified Media Account flow: bilingual application form, file upload and confirmation page; success page added.

• Improvements

  • Site-wide CSRF protection added and upload handling reordered for more reliable multipart submissions.
  • Centralized, safer file storage and improved upload/error handling.
  • Sign‑in copy clarified; session persistence ensured before redirects.

• Bug Fixes / Stability

  • CI now skips test reporting when JUnit results are missing.
  • Database migration to record media account requests.

• Tests & Docs

  • Extensive new unit and E2E tests plus planning and task documentation.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

apps/web/src/app.test.ts (3)

222-254: Consider strengthening route registration assertions.

The comments acknowledge that direct route inspection isn't reliable in this test environment, which is a valid concern. However, these tests currently only verify that createFileUpload was called and app.post is a function — they would still pass even if the routes were never registered.

A stronger approach would be to capture the app.post calls via spying on the Express app, similar to the pattern used in the handleMulterError tests (lines 263-272).

---

541-556: These middleware tests provide no meaningful verification.

The tests for compression, urlencoded, and cookie parser middleware only assert that app.use or app is defined — they would pass even if these middleware were never configured. Consider either:

1. Removing these tests if they're placeholders
2. Strengthening them by inspecting the middleware stack or mocking the imports

For example, to actually verify compression is configured:

-    it("should configure compression middleware", async () => {
-      // Compression is configured at line 41
-      // Verified by checking that app.use was called
-      expect(app.use).toBeDefined();
-    });
+    it("should configure compression middleware", async () => {
+      // Could mock compression and verify it was called
+      // Or inspect app._router.stack for compression middleware
+      // Current assertion is a placeholder
+    });

---

592-602: SSO/CFT callback route tests don't verify registration.

These tests only assert that ssoCallbackHandler and cftCallbackHandler are defined, which they always will be since they're imported from the mock. They don't verify the routes are actually registered with app.get.

Consider verifying the handlers were called during registration or spying on app.get:

     it("should register SSO callback route", async () => {
       const { ssoCallbackHandler } = await import("@hmcts/auth");
-      // Verify app.get was called for /sso/return route (line 140)
-      expect(ssoCallbackHandler).toBeDefined();
+      expect(ssoCallbackHandler).toHaveBeenCalled();
     });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 638905c and d27197f.

📒 Files selected for processing (1)

• apps/web/src/app.test.ts (4 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 use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• apps/web/src/app.test.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• apps/web/src/app.test.ts

**/*.test.ts

📄 CodeRabbit inference engine (CLAUDE.md)

All packages must use "test": "vitest run" as the test script. Unit and integration tests must be co-located with source code as *.test.ts files.

Files:

• apps/web/src/app.test.ts

🧠 Learnings (3)

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/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 must import config using the `/config` path (e.g., `hmcts/my-feature/config`).

Applied to files:

• apps/web/src/app.test.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Applied to files:

• apps/web/src/app.test.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.

Applied to files:

• apps/web/src/app.test.ts

🧬 Code graph analysis (1)

apps/web/src/app.test.ts (3)

apps/web/src/app.ts (1)

• createApp (50-170)

libs/auth/src/index.ts (2)

• configurePassport (3-3)
• authNavigationMiddleware (7-7)

libs/web-core/src/middleware/helmet/helmet-middleware.ts (1)

• configureHelmet (18-54)

⏰ 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)

apps/web/src/app.test.ts (4)

110-114: LGTM — CSRF protection test added as requested.

This test addresses the previous review feedback by explicitly verifying that the security-critical configureCsrf middleware is invoked during app initialization.

---

200-220: LGTM — Correct approach using invocationCallOrder for call-order verification.

This properly addresses the previous review feedback by avoiding the unavailable toHaveBeenCalledBefore matcher and using Vitest's native invocationCallOrder property instead.

---

257-316: Good test isolation pattern with vi.doMock.

The approach of using vi.resetModules(), vi.doMock(), and re-importing to capture middleware handlers is well-executed. This properly tests the error handling logic for different multer error codes.

---

667-680: LGTM — Good use of invocationCallOrder for handler ordering.

This correctly verifies that notFoundHandler is registered before errorHandler, which is critical for proper Express error handling behavior.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d27197f and 3bcd310.

📒 Files selected for processing (1)

• libs/admin-pages/src/pages/manual-upload/index.test.ts (9 hunks)

🧰 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 use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• libs/admin-pages/src/pages/manual-upload/index.test.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• libs/admin-pages/src/pages/manual-upload/index.test.ts

**/{pages,locales}/**/*.{ts,njk}

📄 CodeRabbit inference engine (CLAUDE.md)

Every page must support both English and Welsh by providing en and cy objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).

Files:

• libs/admin-pages/src/pages/manual-upload/index.test.ts

**/*.test.ts

📄 CodeRabbit inference engine (CLAUDE.md)

All packages must use "test": "vitest run" as the test script. Unit and integration tests must be co-located with source code as *.test.ts files.

Files:

• libs/admin-pages/src/pages/manual-upload/index.test.ts

**/pages/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Page routes are created based on file names within the pages/ directory. Nested routes are created using subdirectories (e.g., pages/admin/my-page.ts becomes /admin/my-page).

Files:

• libs/admin-pages/src/pages/manual-upload/index.test.ts

**/{locales,pages}/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.

Files:

• libs/admin-pages/src/pages/manual-upload/index.test.ts

🧬 Code graph analysis (1)

libs/admin-pages/src/pages/manual-upload/index.test.ts (3)

libs/admin-pages/src/pages/manual-upload/index.ts (1)

• POST (148-148)

libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)

• POST (172-172)

libs/admin-pages/src/manual-upload/storage.ts (1)

• storeManualUpload (20-44)

🔇 Additional comments (1)

libs/admin-pages/src/pages/manual-upload/index.test.ts (1)

541-541: LGTM: Async validation mock updated correctly.

The changes from mockReturnValue to mockResolvedValue consistently reflect that validateForm now returns a Promise. All test cases have been updated uniformly.

Also applies to: 599-599, 650-650, 691-691, 737-737, 781-781, 821-821, 859-859

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

apps/web/src/app.test.ts (1)

697-707: Mock CFT_IDAM_URL to avoid asserting undefined behavior.

process.env.CFT_IDAM_URL is not set in the test environment (see config mock at lines 36–46), so this test asserts that cftIdamUrl: undefined was passed, which is likely unintentional.

Add the value to the config mock:

 vi.mock("config", () => ({
   default: {
     get: vi.fn((key: string) => {
       if (key === "redis.url") return "redis://localhost:6379";
       if (key === "gtm") return { id: "GTM-TEST" };
       if (key === "dynatrace") return { enabled: false };
       if (key === "applicationInsights") return { connectionString: "test" };
+      if (key === "cftIdamUrl") return "https://test-idam.example.com";
       return {};
     })
   }
 }));

Then update the assertion:

       expect(configureHelmet).toHaveBeenCalledWith(
         expect.objectContaining({
-          cftIdamUrl: process.env.CFT_IDAM_URL
+          cftIdamUrl: "https://test-idam.example.com"
         })
       );

🧹 Nitpick comments (4)

e2e-tests/tests/remove-publication.spec.ts (2)

32-37: Pre-seeded upload flow and autocomplete init wait look good

Targeting locationId=1 and explicitly waiting for #court-autocomplete-wrapper to attach before proceeding should make the pre-seeded publication upload more reliable. The extra 500 ms pause is pragmatic for now; if this ever flakes, consider switching to a state-based wait (e.g. asserting the underlying location field has the expected value) instead of a fixed timeout.

---

133-135: Prefer a state-based wait over fixed 500 ms delays around autocomplete

Waiting for #locationId-autocomplete-wrapper to attach is a good synchronization point and should avoid racing autocomplete initialisation. The subsequent waitForTimeout(500) after selection works but is slightly brittle.

If feasible, consider waiting on the actual hidden field that backs the autocomplete (for example, asserting that input[name="locationId"] becomes non-empty or has the expected ID) instead of a hard-coded delay; this will both tighten the test and avoid unnecessary waiting on fast environments.

Also applies to: 146-147

e2e-tests/tests/admin-dashboard.spec.ts (1)

7-16: Consider extracting repeated authentication pattern.

The same authentication sequence (clearCookies → goto → loginWithSSO → waitForURL) is repeated across multiple test blocks. This could be extracted into a reusable helper to reduce duplication and improve maintainability.

Example helper in e2e-tests/utils/sso-helpers.ts:

export async function setupAuthenticatedSession(
  page: Page,
  context: BrowserContext,
  email: string,
  password: string,
  targetUrl: string = "/admin-dashboard"
): Promise<void> {
  await context.clearCookies();
  await page.goto(targetUrl);
  await loginWithSSO(page, email, password);
  await page.waitForURL(targetUrl);
}

Then simplify beforeEach hooks:

test.beforeEach(async ({ page, context }) => {
  await setupAuthenticatedSession(
    page,
    context,
    process.env.SSO_TEST_LOCAL_ADMIN_EMAIL!,
    process.env.SSO_TEST_LOCAL_ADMIN_PASSWORD!
  );
});

Also applies to: 67-76, 100-109, 158-163, 175-180, 192-197, 208-217, 291-302

apps/web/src/app.test.ts (1)

222-254: Consider strengthening route registration assertions.

While the comments acknowledge that direct route inspection is unreliable due to mocking, these tests would still pass if the routes were never registered. Consider:

1. Mock express() to track app.post calls and assert it was invoked with the expected paths (/create-media-account, /manual-upload)
2. Or inspect app._router.stack after creating a real Express instance (without extensive mocking) in a dedicated integration test

This would provide stronger guarantees that the routes are actually wired correctly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8774877 and 3cf2edb.

⛔ Files ignored due to path filters (1)

• yarn.lock is excluded by !**/yarn.lock, !**/*.lock

📒 Files selected for processing (17)

• .github/workflows/e2e.yml (2 hunks)
• apps/web/package.json (1 hunks)
• apps/web/src/app.test.ts (4 hunks)
• apps/web/src/app.ts (4 hunks)
• e2e-tests/tests/admin-dashboard.spec.ts (8 hunks)
• e2e-tests/tests/manual-upload.spec.ts (7 hunks)
• e2e-tests/tests/remove-publication.spec.ts (3 hunks)
• e2e-tests/tests/sign-in.spec.ts (4 hunks)
• libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (1 hunks)
• libs/admin-pages/src/pages/manual-upload-summary/index.ts (1 hunks)
• libs/admin-pages/src/pages/manual-upload/index.test.ts (9 hunks)
• libs/admin-pages/src/pages/manual-upload/index.ts (1 hunks)
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts (2 hunks)
• libs/public-pages/src/pages/sign-in/index.njk (2 hunks)
• libs/publication/src/repository/queries.ts (1 hunks)
• libs/web-core/src/index.ts (1 hunks)
• libs/web-core/src/middleware/session-stores/postgres-store.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (8)

• libs/admin-pages/src/pages/manual-upload-summary/index.ts
• libs/admin-pages/src/pages/manual-upload/index.test.ts
• apps/web/package.json
• e2e-tests/tests/manual-upload.spec.ts
• libs/public-pages/src/pages/sign-in/index.njk
• apps/web/src/app.ts
• libs/admin-pages/src/pages/manual-upload/index.ts
• libs/publication/src/repository/queries.ts

🧰 Additional context used

📓 Path-based instructions (7)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g., userId, caseDetails, documentId). Booleans should use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• e2e-tests/tests/sign-in.spec.ts
• libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
• apps/web/src/app.test.ts
• e2e-tests/tests/remove-publication.spec.ts
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts
• libs/web-core/src/index.ts
• libs/web-core/src/middleware/session-stores/postgres-store.ts
• e2e-tests/tests/admin-dashboard.spec.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• e2e-tests/tests/sign-in.spec.ts
• libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
• apps/web/src/app.test.ts
• e2e-tests/tests/remove-publication.spec.ts
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts
• libs/web-core/src/index.ts
• libs/web-core/src/middleware/session-stores/postgres-store.ts
• e2e-tests/tests/admin-dashboard.spec.ts

**/{pages,locales}/**/*.{ts,njk}

📄 CodeRabbit inference engine (CLAUDE.md)

Every page must support both English and Welsh by providing en and cy objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).

Files:

• libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

**/*.test.ts

📄 CodeRabbit inference engine (CLAUDE.md)

All packages must use "test": "vitest run" as the test script. Unit and integration tests must be co-located with source code as *.test.ts files.

Files:

• libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
• apps/web/src/app.test.ts
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

**/pages/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Page routes are created based on file names within the pages/ directory. Nested routes are created using subdirectories (e.g., pages/admin/my-page.ts becomes /admin/my-page).

Files:

• libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

**/{locales,pages}/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.

Files:

• libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

libs/*/src/index.ts

📄 CodeRabbit inference engine (CLAUDE.md)

All modules must have src/index.ts for business logic exports separate from src/config.ts.

Files:

• libs/web-core/src/index.ts

🧠 Learnings (7)

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{pages,locales}/**/*.{ts,njk} : Every page must support both English and Welsh by providing `en` and `cy` objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).

Applied to files:

• e2e-tests/tests/sign-in.spec.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/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 must import config using the `/config` path (e.g., `hmcts/my-feature/config`).

Applied to files:

• apps/web/src/app.test.ts
• libs/web-core/src/index.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Applied to files:

• apps/web/src/app.test.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.

Applied to files:

• apps/web/src/app.test.ts
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to libs/*/src/index.ts : All modules must have `src/index.ts` for business logic exports separate from `src/config.ts`.

Applied to files:

• libs/web-core/src/index.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*-middleware.ts : Reusable middleware must be placed in a dedicated `libs/[module]/src/[middleware-name]-middleware.ts` file and exported as a function.

Applied to files:

• libs/web-core/src/index.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/{locales,pages}/**/*.ts : Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.

Applied to files:

• libs/web-core/src/index.ts

🧬 Code graph analysis (2)

libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts (2)

libs/admin-pages/src/pages/remove-list-confirmation/index.ts (1)

• GET (133-133)

libs/public-pages/src/pages/view-option/index.ts (1)

• GET (10-15)

e2e-tests/tests/admin-dashboard.spec.ts (1)

e2e-tests/utils/sso-helpers.ts (1)

• loginWithSSO (10-46)

⏰ 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 (15)

e2e-tests/tests/sign-in.spec.ts (1)

33-37: LGTM! Text expectations updated consistently.

The test assertions have been properly updated to reflect the new account creation link text in both English and Welsh, including all accessibility checks. The changes maintain proper bilingual support and accessibility coverage.

Also applies to: 198-201, 267-269, 379-380

e2e-tests/tests/admin-dashboard.spec.ts (1)

7-16: Session isolation properly implemented.

The changes address the previous review concern about session leakage by clearing cookies and re-authenticating in each describe block. This ensures test isolation and prevents state from leaking between test contexts.

Also applies to: 67-76, 100-109, 208-217, 291-302

.github/workflows/e2e.yml (1)

192-201: Based on my analysis of the codebase, I can now provide a comprehensive verification:

Key findings:

1. Playwright junit reporter configuration (playwright.config.ts lines 11-18): The junit reporter is configured ONLY in CI mode (process.env.CI=true), outputting to junit-results.xml.

2. Run-with-credentials.js (lines 113-114): Properly propagates playwright's exit code to the workflow, so test failures are detected.

3. Workflow behavior:

   • The "Run E2E tests" step (line 185) can fail with a non-zero exit code
   • The "Check if junit results exist" step (line 192-201) runs regardless via if: always()
   • If junit is missing, reporting steps are skipped silently (lines 206, 236)

4. When junit-results.xml might NOT exist in CI:

   • Global setup fails (DB connection, migrations, seed data verification) - throws error before tests run
   • Playwright process crashes before generating any output
   • File I/O errors prevent file creation

5. Current design: Gracefully degrades when junit is missing - no workflow failure, just warning and skipped reporting. This prevents unrelated infrastructure failures from masking test results.

The concern about "masking test failures" is partially valid: if the e2e-tests step fails but junit-results.xml is missing, the workflow continues and skips PR check reporting. However, the workflow does capture exit codes for artifact uploads (lines 216, 227), so test failures are still detectable.

---

Verify that conditional reporting doesn't mask test failures.

The workflow tolerates missing junit-results.xml gracefully, which is appropriate for global setup failures but could hide scenarios where tests start but encounter critical errors. However, since the "Run E2E tests" step exit code is still propagated, actual test failures are captured—they just won't generate PR check annotations when junit is missing. Consider explicitly failing the workflow when junit is absent after successful setup steps, or add a pre-flight check ensuring test results are generated before conditional reporting steps.

libs/web-core/src/middleware/session-stores/postgres-store.ts (1)

10-11: LGTM!

The lint suppression is justified—ttl is indeed used in the getExpireDate method at line 89.

libs/web-core/src/index.ts (1)

9-9: LGTM!

The export follows coding guidelines (.js extension for ESM imports) and is positioned correctly among other middleware exports.

apps/web/src/app.test.ts (6)

110-114: LGTM!

This test properly verifies that CSRF middleware is configured during app initialization and called exactly once, addressing the previous review feedback.

---

200-220: LGTM!

Excellent solution using invocationCallOrder to verify middleware sequencing. This approach avoids the need for external matcher libraries and provides deterministic call-order verification.

---

257-498: LGTM!

Comprehensive test coverage for Multer error handling. The tests properly verify error storage, logging behavior, and success cases with appropriate module isolation.

---

500-538: LGTM!

The Redis connection tests properly verify error handling and connection flow with appropriate mocking.

---

540-589: LGTM!

The middleware configuration tests provide appropriate smoke test coverage, with particularly thorough validation of GOV.UK Frontend configuration.

---

591-681: LGTM!

The route registration tests effectively verify middleware sequencing using invocationCallOrder, ensuring that error handlers are properly ordered.

libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts (3)

4-4: Vitest imports and lifecycle helpers look correct

Importing beforeEach, describe, expect, it, and vi from vitest is consistent with the rest of the suite and supports the new global beforeEach below.

---

37-39: Global beforeEach with vi.clearAllMocks improves test isolation

Clearing all mocks before each test at the outer describe level is a good pattern here and keeps the expectations in each test independent of previous calls. The per-test configuration of mockGetLocationById, mockGetArtefactsByIds, and mockDeleteArtefacts makes this safe.

---

46-59: Redirect spying via mockRedirect is clear and explicit

Using a dedicated mockRedirect spy and wiring it into mockRes.redirect makes the redirect assertion explicit and easy to read, while still keeping the handler signature aligned with Express Response. No issues here.

libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (1)

67-73: Publication mocks for createArtefact and saveUploadedFile are wired correctly

Extending the @hmcts/publication mock to override createArtefact and saveUploadedFile with vi.fn() and then importing them for use with vi.mocked(...) in the POST tests cleanly isolates file saving and artefact creation from real infrastructure. This matches the refactor away from the old local file-storage module and looks good.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

apps/web/src/app.test.ts (1)

189-265: Tighten upload route tests and reduce duplication

• The “should configure multer file upload middleware” test overlaps heavily with “should configure file upload middleware with error handling” above; they both only assert that createFileUpload was called. Consider consolidating to a single test to keep the suite DRY.
• The “should configure upload.single middleware wrapper” and “should register file upload routes before CSRF middleware” tests add meaningful coverage (shape of single() and call order vs configureCsrf), which is good.
• The POST route tests for /create-media-account and /manual-upload still only verify that createFileUpload was called and that app.post is defined. They will continue to pass even if those specific routes are removed or mis‑wired. Since you already use an express mock later to capture route‑specific handlers, consider using the same approach here to assert that app.post is called with the expected paths and upload middleware rather than just checking app.post exists.

🧹 Nitpick comments (2)

apps/web/src/app.test.ts (2)

550-566: Strengthen or remove middleware “existence only” tests

The three tests for compression, urlencoded, and cookie‑parser middleware only check that app / app.use are defined, which will be true regardless of whether those middlewares are actually registered. They don’t fail on regressions and can create a false sense of coverage.

Consider either:

• Asserting concrete wiring (e.g. inspecting app._router.stack or using an express mock like in the multer tests to assert app.use was called with compression(), express.urlencoded({ … }), cookieParser(), etc.), or
• Dropping these tests if you don’t plan to assert anything more specific.

---

601-691: Route registration tests could assert more than existence

Within “Route Registration”:

• The SSO and CFT callback tests only assert that ssoCallbackHandler/cftCallbackHandler are defined, which they always will be due to the @hmcts/auth mock, even if the corresponding routes are never registered.
• The sequence tests for createSimpleRouter (civil/family, web core pages, auth, system admin, public, verified, admin) currently only check that calls.length is at least N and that calls[index] is defined. They don’t actually assert ordering or arguments, despite the test names implying “first/second/third/etc.” ordering.

If you care about sequence and presence of specific route groups, consider:

• Asserting the arguments of each createSimpleRouter call (e.g., using a base path or config identifier), or
• Explicitly checking the relative order using indices and a distinguishing argument, rather than only that the call exists.

Otherwise, it may be clearer to relax the test names so they don’t over‑promise on ordering guarantees.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3cf2edb and 579b47b.

📒 Files selected for processing (2)

• apps/web/src/app.test.ts (5 hunks)
• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts (2 hunks)

🧰 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 use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts
• apps/web/src/app.test.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts
• apps/web/src/app.test.ts

**/{pages,locales}/**/*.{ts,njk}

📄 CodeRabbit inference engine (CLAUDE.md)

Every page must support both English and Welsh by providing en and cy objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).

Files:

• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

**/*.test.ts

📄 CodeRabbit inference engine (CLAUDE.md)

All packages must use "test": "vitest run" as the test script. Unit and integration tests must be co-located with source code as *.test.ts files.

Files:

• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts
• apps/web/src/app.test.ts

**/pages/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Page routes are created based on file names within the pages/ directory. Nested routes are created using subdirectories (e.g., pages/admin/my-page.ts becomes /admin/my-page).

Files:

• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

**/{locales,pages}/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Shared/common content (button text, phase banner, service name, common errors) should be in locale files (libs/[module]/src/locales/en.ts and cy.ts). Page-specific content should be in controllers.

Files:

• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts

🧠 Learnings (3)

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.

Applied to files:

• libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts
• apps/web/src/app.test.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/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 must import config using the `/config` path (e.g., `hmcts/my-feature/config`).

Applied to files:

• apps/web/src/app.test.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Applied to files:

• apps/web/src/app.test.ts

🧬 Code graph analysis (1)

libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts (1)

libs/admin-pages/src/pages/remove-list-confirmation/index.ts (1)

• GET (133-133)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: SonarQube Analysis
• GitHub Check: E2E Tests

🔇 Additional comments (10)

libs/admin-pages/src/pages/remove-list-confirmation/index.test.ts (2)

4-4: Good use of beforeEach to keep mocks isolated between tests

Importing beforeEach and calling vi.clearAllMocks() before each test keeps mock call history clean while preserving the shared mock wiring set up at the top of the file. This reduces the risk of cross‑test leakage without adding extra boilerplate in each test block.

Please rerun the test suite for remove-list-confirmation to confirm everything still passes with the new global beforeEach behaviour in your Vitest configuration.

Also applies to: 28-30

---

37-49: Redirect mocking pattern is clearer and more explicit

Switching to a dedicated mockRedirect function and asserting on it directly makes the redirect expectation more explicit and keeps the Response stub minimal, which is easier to maintain and refactor alongside other tests following the same pattern.

After this refactor, verify that the GET handler test still exercises the real Express handler (no additional stubbing of GET[1]) and that the redirect path /remove-list-search matches the actual route wiring in libs/admin-pages/src/pages/remove-list-confirmation/index.ts.

apps/web/src/app.test.ts (8)

15-27: CSRF mock shape looks appropriate for createApp usage

Mocking configureCsrf to return an array of next()-calling middleware functions matches the expected “list of handlers” shape and keeps tests simple and side‑effect free. This should integrate cleanly with how createApp wires CSRF middleware.

---

55-75: Environment handling for CFT_IDAM_URL is robust

Capturing originalCftIdamUrl and restoring or deleting process.env.CFT_IDAM_URL in afterEach avoids cross‑test leakage while still giving a deterministic value (TEST_CFT_IDAM_URL) for the helmet configuration test. This is a solid pattern for env‑dependent behaviour.

---

120-124: New CSRF configuration test closes previous coverage gap

The “should configure CSRF protection middleware” test explicitly asserts that configureCsrf is called exactly once, which directly exercises the new CSRF wiring and addresses the earlier gap around CSRF initialization coverage.

---

267-508: Multer error‑handling tests are thorough and well‑scoped

The handleMulterError tests that mock express and @hmcts/web-core per test case give strong coverage:

• LIMIT_FILE_SIZE and LIMIT_FILE_COUNT errors are correctly propagated onto req.fileUploadError with the expected code and field, and next is still invoked.
• Unexpected errors are logged via console.error and still stored on the request, and the spy is restored at the end of the test.
• The “no error” path ensures req.fileUploadError remains undefined while next is called.

This isolated mocking pattern around createApp and app.post makes the behaviour of the upload wrapper and its error handling very explicit.

---

510-548: Redis connection tests cover both success and error paths

The Redis tests nicely split concerns:

• The error test verifies that an "error" event handler is registered on the client and that connection failures are logged via console.error.
• The success test asserts that connect() is actually called on the created client.

This combination gives good confidence around Redis wiring without touching real infrastructure.

---

568-599: Auth and GOV.UK Frontend wiring tests are useful

These tests meaningfully exercise integration points:

• configurePassport is asserted to have been called with app, which validates auth wiring.
• configureGovuk assertions check that the first call receives the actual app, an array of module paths of the expected length, and an options object with the expected nunjucksGlobals and assetOptions structure.
• authNavigationMiddleware is verified as invoked.

This gives good coverage of important middleware setup without over‑specifying implementation details.

---

693-705: Express Session Redis configuration assertion is precise

The “Express Session Redis Configuration” test does a nice job of asserting that expressSessionRedis was called and that its first argument includes a redisConnection property derived from the mocked Redis client. This is a concrete, behaviour‑focused check rather than a generic existence assertion.

---

707-717: Helmet CFT IDAM URL test correctly validates option propagation

The helmet configuration test now asserts that configureHelmet is called with an object containing cftIdamUrl: TEST_CFT_IDAM_URL, tying together the env setup at the top and the expected runtime configuration. This makes the CFT IDAM URL dependency explicit and avoids asserting on an undefined env value.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

apps/web/src/app.ts (1)

62-79: Type safety: err may not be a MulterError.

The function signature allows Error but line 66 unconditionally casts to MulterError. If multer passes a plain Error (e.g., during stream errors), accessing .code and .field will be undefined. Consider narrowing the type before casting:

 const handleMulterError = (err: MulterError | Error | undefined, req: Request, fieldName: string) => {
   if (!err) return;

-  // Store the error for the controller to handle
-  req.fileUploadError = err as MulterError;
+  // Store the error for the controller to handle
+  // Only treat as MulterError if it has the expected code property
+  if ("code" in err) {
+    req.fileUploadError = err as MulterError;
+  } else {
+    // For non-Multer errors, wrap in a generic structure
+    req.fileUploadError = {
+      ...err,
+      code: "UNKNOWN_ERROR",
+      field: fieldName
+    } as MulterError;
+  }

   // Log unexpected multer errors for debugging
   const knownCodes = ["LIMIT_FILE_SIZE", "LIMIT_FILE_COUNT", "LIMIT_FIELD_SIZE", "LIMIT_UNEXPECTED_FILE"];
-  const errorCode = (err as MulterError).code;
+  const errorCode = req.fileUploadError.code;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 462ed38 and 095740b.

📒 Files selected for processing (2)

• apps/web/src/app.ts (4 hunks)
• e2e-tests/tests/reference-data-upload.spec.ts (2 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 use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• apps/web/src/app.ts
• e2e-tests/tests/reference-data-upload.spec.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• apps/web/src/app.ts
• e2e-tests/tests/reference-data-upload.spec.ts

🧠 Learnings (10)

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/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 must import config using the `/config` path (e.g., `hmcts/my-feature/config`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : TypeScript must use strict mode enabled with no `any` without justification. Use workspace aliases (`hmcts/*`) for imports.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Constants must use SCREAMING_SNAKE_CASE (e.g., `MAX_FILE_SIZE`, `DEFAULT_TIMEOUT`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : Data should be immutable by default. Use const and avoid mutations to ensure predictable state.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.{ts,tsx} : TypeScript variables must use camelCase (e.g., `userId`, `caseDetails`, `documentId`). Booleans should use `is/has/can` prefix (e.g., `isActive`, `hasAccess`, `canEdit`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/pages/**/*.ts : Page routes are created based on file names within the `pages/` directory. Nested routes are created using subdirectories (e.g., `pages/admin/my-page.ts` becomes `/admin/my-page`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.

Applied to files:

• e2e-tests/tests/reference-data-upload.spec.ts

📚 Learning: 2025-11-25T06:40:24.082Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.082Z
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:

• e2e-tests/tests/reference-data-upload.spec.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: SonarQube Analysis
• GitHub Check: E2E Tests

🔇 Additional comments (4)

e2e-tests/tests/reference-data-upload.spec.ts (1)

3-10: ESM-compatible fixture path handling looks good

Using fileURLToPath(import.meta.url) to derive __filename/__dirname and then resolving fixturesPath relative to the spec file is a solid ESM-safe approach and more robust than relying on process.cwd(). No changes needed.
As per coding guidelines, ...

Also applies to: 47-47

apps/web/src/app.ts (3)

15-32: LGTM!

The imports are well-organized with proper type imports for Request and MulterError. Using configureCsrf from the @hmcts/web-core package maintains the correct abstraction boundary.

---

81-99: LGTM!

File upload middleware is correctly registered before CSRF protection, ensuring multipart form bodies are parsed before CSRF token validation. The consistent pattern across all three routes is clean and maintainable.

---

101-101: LGTM!

CSRF middleware is correctly placed after file upload middleware registration, ensuring proper request body parsing before CSRF validation.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

apps/web/src/app.ts (1)

164-170: Duplicate route registration for /reference-data-upload still present.

This POST handler duplicates the one already registered at lines 101-106. Both register upload.single("file") with identical error handling, causing the file upload middleware to potentially execute twice for this route.

Remove the duplicate registration:

-  // Register reference data upload with file upload middleware
-  app.post("/reference-data-upload", (req, res, next) => {
-    upload.single("file")(req, res, (err) => {
-      handleMulterError(err, req, "file");
-      next();
-    });
-  });
   app.use(await createSimpleRouter(systemAdminPageRoutes, pageRoutes));

🧹 Nitpick comments (1)

apps/web/src/app.ts (1)

72-73: Type assertion may incorrectly cast plain Error to MulterError.

When err is a generic Error (not a MulterError), casting it as MulterError will result in req.fileUploadError.code being undefined. While downstream handlers check .code before using it, storing the original error type would be more accurate.

Consider preserving the original error type:

     // Store the error for the controller to handle
-    req.fileUploadError = err as MulterError;
+    req.fileUploadError = "code" in err ? (err as MulterError) : err;

Alternatively, if the Express type augmentation in @hmcts/web-core already types fileUploadError as MulterError, consider broadening it to MulterError | Error for accuracy.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 095740b and 8480cf7.

📒 Files selected for processing (3)

• apps/web/src/app.ts (4 hunks)
• libs/system-admin-pages/src/pages/reference-data-upload-summary/index.njk (1 hunks)
• libs/web-core/src/middleware/file-upload/file-upload-middleware.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 use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• apps/web/src/app.ts
• libs/web-core/src/middleware/file-upload/file-upload-middleware.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• apps/web/src/app.ts
• libs/web-core/src/middleware/file-upload/file-upload-middleware.ts

**/{pages,locales}/**/*.{ts,njk}

📄 CodeRabbit inference engine (CLAUDE.md)

Every page must support both English and Welsh by providing en and cy objects in controllers and maintaining matching structure in locale files (libs/[module]/src/locales/en.ts and cy.ts).

Files:

• libs/system-admin-pages/src/pages/reference-data-upload-summary/index.njk

**/*-middleware.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Reusable middleware must be placed in a dedicated libs/[module]/src/[middleware-name]-middleware.ts file and exported as a function.

Files:

• libs/web-core/src/middleware/file-upload/file-upload-middleware.ts

🧠 Learnings (7)

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : TypeScript must use strict mode enabled with no `any` without justification. Use workspace aliases (`hmcts/*`) for imports.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Constants must use SCREAMING_SNAKE_CASE (e.g., `MAX_FILE_SIZE`, `DEFAULT_TIMEOUT`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Data should be immutable by default. Use const and avoid mutations to ensure predictable state.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : TypeScript variables must use camelCase (e.g., `userId`, `caseDetails`, `documentId`). Booleans should use `is/has/can` prefix (e.g., `isActive`, `hasAccess`, `canEdit`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/pages/**/*.ts : Page routes are created based on file names within the `pages/` directory. Nested routes are created using subdirectories (e.g., `pages/admin/my-page.ts` becomes `/admin/my-page`).

Applied to files:

• apps/web/src/app.ts

🧬 Code graph analysis (1)

apps/web/src/app.ts (3)

libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)

• createFileUpload (9-23)

libs/web-core/src/index.ts (2)

• createFileUpload (11-11)
• configureCsrf (9-9)

libs/web-core/src/middleware/csrf/csrf-middleware.ts (1)

• configureCsrf (25-40)

⏰ 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 (4)

libs/system-admin-pages/src/pages/reference-data-upload-summary/index.njk (1)

106-111: CSRF hidden field added correctly to the POST form

The _csrf hidden input is correctly named and positioned inside the POST form and bound to csrfToken, bringing this template in line with the CSRF-protection pattern used elsewhere; no further changes needed here.

libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)

16-21: LGTM! Reasonable multer limits to prevent abuse.

The limits configuration adds sensible defaults for multipart form handling: capping non-file fields, per-field size, and total parts helps prevent resource exhaustion attacks while being permissive enough for legitimate use cases.

apps/web/src/app.ts (2)

15-32: LGTM! Proper type imports for file upload error handling.

The imports correctly bring in MulterError type and Request type to support the typed error handling, and use workspace aliases as per coding guidelines.

---

107-108: LGTM! Correct CSRF middleware ordering.

Placing CSRF protection after file upload middleware registration ensures multipart form bodies are parsed before CSRF token validation, which is the correct pattern for handling file uploads with CSRF protection.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

apps/web/src/app.ts (1)

61-86: Well-structured error handling with proper typing.

The handleMulterError function properly uses union types (MulterError | Error | undefined) instead of any, addressing previous type safety concerns. The special handling for "Unexpected end of form" when a file is successfully received is a pragmatic workaround for test framework edge cases.

Minor consideration: The knownCodes array could be extracted as a module-level constant using SCREAMING_SNAKE_CASE (e.g., KNOWN_MULTER_ERROR_CODES) per coding guidelines, but keeping it local is also acceptable given its narrow scope.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8480cf7 and 8951ab7.

📒 Files selected for processing (1)

• apps/web/src/app.ts (3 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 use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• apps/web/src/app.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• apps/web/src/app.ts

🧠 Learnings (7)

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : TypeScript must use strict mode enabled with no `any` without justification. Use workspace aliases (`hmcts/*`) for imports.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Constants must use SCREAMING_SNAKE_CASE (e.g., `MAX_FILE_SIZE`, `DEFAULT_TIMEOUT`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : Data should be immutable by default. Use const and avoid mutations to ensure predictable state.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/pages/*.ts : Page controllers must export GET and/or POST functions with signature `(req: Request, res: Response) => Promise<void>`. Content (titles, descriptions) should be organized as `en` and `cy` objects.

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.{ts,tsx} : TypeScript variables must use camelCase (e.g., `userId`, `caseDetails`, `documentId`). Booleans should use `is/has/can` prefix (e.g., `isActive`, `hasAccess`, `canEdit`).

Applied to files:

• apps/web/src/app.ts

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/pages/**/*.ts : Page routes are created based on file names within the `pages/` directory. Nested routes are created using subdirectories (e.g., `pages/admin/my-page.ts` becomes `/admin/my-page`).

Applied to files:

• apps/web/src/app.ts

🧬 Code graph analysis (1)

apps/web/src/app.ts (3)

libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)

• createFileUpload (9-23)

libs/web-core/src/index.ts (2)

• createFileUpload (11-11)
• configureCsrf (9-9)

libs/web-core/src/middleware/csrf/csrf-middleware.ts (1)

• configureCsrf (25-40)

⏰ 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 (3)

apps/web/src/app.ts (3)

15-32: LGTM!

Imports are properly organized with workspace aliases (@hmcts/*) as per coding guidelines. Type imports correctly use import type syntax for Request and MulterError.

---

88-106: LGTM!

The file upload middleware is correctly registered before CSRF protection for the three routes that require multipart parsing. Each registration follows a consistent pattern and properly calls next() to delegate to the actual route handlers. The previous duplicate registration issue has been resolved.

---

107-108: LGTM!

CSRF middleware is correctly placed after session configuration (line 55) and after file upload middleware (lines 88-106). This ordering ensures:

1. Session is available for CSRF token storage
2. Multipart form bodies are parsed before CSRF validation

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

libs/web-core/src/middleware/file-upload/file-upload-middleware.test.ts (1)

29-34: Expanded Multer limits assertions look correct and aligned with middleware

The updated expectations for limits (fileSize, fields, fieldSize, parts) for both default and custom max file size cases match the described middleware behaviour and give good coverage of the configuration surface. As a minor optional tweak, you could extract a shared expectedLimits helper in the test to avoid repeating the same object shape if more scenarios are added later.

Also applies to: 48-53

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8951ab7 and 835b6d3.

📒 Files selected for processing (1)

• libs/web-core/src/middleware/file-upload/file-upload-middleware.test.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g., userId, caseDetails, documentId). Booleans should use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• libs/web-core/src/middleware/file-upload/file-upload-middleware.test.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• libs/web-core/src/middleware/file-upload/file-upload-middleware.test.ts

**/*.test.ts

📄 CodeRabbit inference engine (CLAUDE.md)

All packages must use "test": "vitest run" as the test script. Unit and integration tests must be co-located with source code as *.test.ts files.

Files:

• libs/web-core/src/middleware/file-upload/file-upload-middleware.test.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: SonarQube Analysis
• GitHub Check: E2E Tests

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (2)

e2e-tests/tests/remove-publication.spec.ts (2)

5-34: This is a duplicate of the getFutureDates() function in manual-upload.spec.ts.

See the review comment in manual-upload.spec.ts for the recommended refactoring to extract this into a shared utility module.

Note the key difference here: displayTo is set to 5 years in the future (line 15) vs. 13 days in manual-upload.spec.ts. This suggests the remove publication tests need far-future dates to ensure the publications remain visible long enough for the removal flow to find them. Consider adding a comment explaining this requirement.

---

66-68: Replace arbitrary waits with condition-based waits.

Similar to the issue in manual-upload.spec.ts, these waitForTimeout(500) calls are arbitrary and can cause flakiness.

• Line 68: After autocomplete initialization, wait for a specific condition (e.g., hidden locationId field populated) rather than an arbitrary 500ms.
• Line 179: After keyboard navigation, wait for the autocomplete value to be set or the hidden field to be updated.

Apply a similar fix as suggested in manual-upload.spec.ts:

  await page.waitForSelector('#court-autocomplete-wrapper', { state: 'attached', timeout: 5000 });
- await page.waitForTimeout(500);
+ await page.waitForFunction(() => {
+   const hiddenInput = document.querySelector('input[name="locationId"]');
+   return hiddenInput && hiddenInput.value !== '';
+ }, { timeout: 5000 });

For line 179, wait for the hidden field to update after keyboard selection:

  await page.keyboard.press('Enter');
- // Wait for the autocomplete to update the hidden locationId field
- await page.waitForTimeout(500);
+ // Wait for the autocomplete to update the hidden locationId field
+ await page.waitForFunction(() => {
+   const hiddenInput = document.querySelector('input[name="locationId"]');
+   return hiddenInput && hiddenInput.value !== '';
+ }, { timeout: 5000 });

Also applies to: 178-179

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 835b6d3 and 10f7133.

📒 Files selected for processing (2)

• e2e-tests/tests/manual-upload.spec.ts (12 hunks)
• e2e-tests/tests/remove-publication.spec.ts (4 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 use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• e2e-tests/tests/manual-upload.spec.ts
• e2e-tests/tests/remove-publication.spec.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• e2e-tests/tests/manual-upload.spec.ts
• e2e-tests/tests/remove-publication.spec.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: SonarQube Analysis
• GitHub Check: E2E Tests

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Extract getFutureDates() to a shared utility module.

This helper function is duplicated across both manual-upload.spec.ts and remove-publication.spec.ts with only minor differences in the displayTo calculation. This violates the DRY principle and makes maintenance harder.

Consider creating a shared utility file (e.g., e2e-tests/utils/date-helpers.ts) and parameterizing the date offsets:

// e2e-tests/utils/date-helpers.ts
export function getFutureDates(options?: {
  hearingDaysOffset?: number;
  displayFromDaysOffset?: number;
  displayToDaysOffset?: number;
  displayToYearsOffset?: number;
  includeFormatted?: boolean;
}) {
  const {
    hearingDaysOffset = 7,
    displayFromDaysOffset = 3,
    displayToDaysOffset = 13,
    displayToYearsOffset = 0,
    includeFormatted = false
  } = options ?? {};

  const today = new Date();
  const hearingDate = new Date(today);
  hearingDate.setDate(today.getDate() + hearingDaysOffset);

  const displayFrom = new Date(today);
  displayFrom.setDate(today.getDate() + displayFromDaysOffset);

  const displayTo = new Date(today);
  if (displayToYearsOffset > 0) {
    displayTo.setFullYear(today.getFullYear() + displayToYearsOffset);
  } else {
    displayTo.setDate(today.getDate() + displayToDaysOffset);
  }

  const result = {
    hearing: {
      day: hearingDate.getDate().toString().padStart(2, '0'),
      month: (hearingDate.getMonth() + 1).toString().padStart(2, '0'),
      year: hearingDate.getFullYear().toString(),
      ...(includeFormatted && {
        formatted: hearingDate.toLocaleDateString('en-GB', { day: 'numeric', month: 'long', year: 'numeric' })
      })
    },
    displayFrom: {
      day: displayFrom.getDate().toString().padStart(2, '0'),
      month: (displayFrom.getMonth() + 1).toString().padStart(2, '0'),
      year: displayFrom.getFullYear().toString(),
      ...(includeFormatted && {
        formatted: displayFrom.toLocaleDateString('en-GB', { day: 'numeric', month: 'long', year: 'numeric' })
      })
    },
    displayTo: {
      day: displayTo.getDate().toString().padStart(2, '0'),
      month: (displayTo.getMonth() + 1).toString().padStart(2, '0'),
      year: displayTo.getFullYear().toString(),
      ...(includeFormatted && {
        formatted: displayTo.toLocaleDateString('en-GB', { day: 'numeric', month: 'long', year: 'numeric' })
      })
    }
  };

  return result;
}

Then import and use it in both test files:

import { getFutureDates } from "../utils/date-helpers.js";

// In manual-upload.spec.ts:
const dates = getFutureDates({ includeFormatted: true });

// In remove-publication.spec.ts:
const dates = getFutureDates({ displayToYearsOffset: 5 });

🤖 Prompt for AI Agents

In e2e-tests/tests/manual-upload.spec.ts around lines 23-55, the
getFutureDates() helper is duplicated across tests; extract it to a shared
utility (e2e-tests/utils/date-helpers.ts) and parameterize the offsets and
whether to include formatted strings (hearingDaysOffset, displayFromDaysOffset,
displayToDaysOffset, displayToYearsOffset, includeFormatted). Replace the local
function with an import from the new utility in manual-upload.spec.ts and remove
duplication in remove-publication.spec.ts, calling getFutureDates(...) with the
appropriate options (e.g., includeFormatted: true or displayToYearsOffset: 5) so
both tests use the same centralized logic.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Replace arbitrary waitForTimeout with condition-based waits.

The waitForTimeout(500) calls after autocomplete initialization are arbitrary waits that can cause test flakiness. If 500ms is insufficient on slower environments, tests will fail intermittently. If it's too long, tests are unnecessarily slow.

Replace with a condition-based wait that checks for the actual state you need. For example, wait for the hidden locationId input to be populated or for a specific attribute to be set:

  // Wait for autocomplete to initialize (ensures JS has loaded and processed the locationId)
  await page.waitForSelector('#court-autocomplete-wrapper', { state: 'attached', timeout: 5000 });
- await page.waitForTimeout(500);
+ // Wait for the hidden locationId field to be populated
+ await page.waitForFunction(() => {
+   const hiddenInput = document.querySelector('input[name="locationId"]');
+   return hiddenInput && hiddenInput.value !== '';
+ }, { timeout: 5000 });

Alternatively, if you're waiting for the autocomplete to be fully interactive, wait for the combobox to have the expected ARIA attributes:

  await page.waitForSelector('#court-autocomplete-wrapper', { state: 'attached', timeout: 5000 });
- await page.waitForTimeout(500);
+ const courtInput = page.getByRole("combobox", { name: /court name or tribunal name/i });
+ await courtInput.waitFor({ state: "visible", timeout: 5000 });

Also applies to: 109-111, 187-189, 380-382, 511-513, 745-747, 784-786

🤖 Prompt for AI Agents

In e2e-tests/tests/manual-upload.spec.ts around lines 62-64, the test uses an
arbitrary await page.waitForTimeout(500) after waiting for the autocomplete
wrapper; replace this fragile fixed delay with a condition-based wait that
ensures the autocomplete is actually ready — for example, wait until the hidden
locationId input has a non-empty value or until the autocomplete combobox has
the expected ARIA attribute/state (aria-expanded, aria-activedescendant, or a
populated options list); remove the waitForTimeout and use page.waitForFunction
or page.waitForSelector with an attribute/value check instead; apply the same
replacement to the other occurrences listed (lines 109-111, 187-189, 380-382,
511-513, 745-747, 784-786).

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

e2e-tests/tests/manual-upload.spec.ts (1)

752-762: Replace hard-coded dates with dynamic dates from getFutureDates().

The hard-coded dates (24-26 November 2025) are at or near the current date and will cause test failures. Use the getFutureDates() helper for consistency and reliability.

Apply this diff:

+    const dates = getFutureDates();
     await page.selectOption('select[name="listType"]', "7");
-    await page.fill('input[name="hearingStartDate-day"]', "25");
-    await page.fill('input[name="hearingStartDate-month"]', "11");
-    await page.fill('input[name="hearingStartDate-year"]', "2025");
+    await page.fill('input[name="hearingStartDate-day"]', dates.hearing.day);
+    await page.fill('input[name="hearingStartDate-month"]', dates.hearing.month);
+    await page.fill('input[name="hearingStartDate-year"]', dates.hearing.year);
     await page.selectOption('select[name="sensitivity"]', "PRIVATE");
     await page.selectOption('select[name="language"]', "WELSH");
-    await page.fill('input[name="displayFrom-day"]', "24");
-    await page.fill('input[name="displayFrom-month"]', "11");
-    await page.fill('input[name="displayFrom-year"]', "2025");
-    await page.fill('input[name="displayTo-day"]', "26");
-    await page.fill('input[name="displayTo-month"]', "11");
-    await page.fill('input[name="displayTo-year"]', "2025");
+    await page.fill('input[name="displayFrom-day"]', dates.displayFrom.day);
+    await page.fill('input[name="displayFrom-month"]', dates.displayFrom.month);
+    await page.fill('input[name="displayFrom-year"]', dates.displayFrom.year);
+    await page.fill('input[name="displayTo-day"]', dates.displayTo.day);
+    await page.fill('input[name="displayTo-month"]', dates.displayTo.month);
+    await page.fill('input[name="displayTo-year"]', dates.displayTo.year);

♻️ Duplicate comments (2)

e2e-tests/tests/manual-upload.spec.ts (2)

23-57: Extract getFutureDates() to a shared utility module.

This helper function is duplicated across test files and should be extracted to a shared utility (e.g., e2e-tests/utils/date-helpers.ts) with parameterized offsets as previously suggested.

---

64-66: Replace arbitrary waitForTimeout with condition-based waits.

The waitForTimeout(500) calls after autocomplete initialization should be replaced with condition-based waits that check for the actual ready state, as previously suggested.

Also applies to: 109-111, 187-189, 380-382, 511-513, 745-747, 784-786

🧹 Nitpick comments (1)

e2e-tests/tests/manual-upload.spec.ts (1)

387-397: Use dynamic dates from getFutureDates() for consistency.

This test uses hard-coded dates (15/06/2025, 10/06/2025, 20/06/2025) while most other tests in this file use the getFutureDates() helper. For consistency and to prevent date-related test failures as dates age, use the helper here as well.

Apply this diff:

+    const dates = getFutureDates();
     await page.selectOption('select[name="listType"]', "1");
-    await page.fill('input[name="hearingStartDate-day"]', "15");
-    await page.fill('input[name="hearingStartDate-month"]', "06");
-    await page.fill('input[name="hearingStartDate-year"]', "2025");
+    await page.fill('input[name="hearingStartDate-day"]', dates.hearing.day);
+    await page.fill('input[name="hearingStartDate-month"]', dates.hearing.month);
+    await page.fill('input[name="hearingStartDate-year"]', dates.hearing.year);
     await page.selectOption('select[name="sensitivity"]', "PUBLIC");
     await page.selectOption('select[name="language"]', "ENGLISH");
-    await page.fill('input[name="displayFrom-day"]', "10");
-    await page.fill('input[name="displayFrom-month"]', "06");
-    await page.fill('input[name="displayFrom-year"]', "2025");
-    await page.fill('input[name="displayTo-day"]', "20");
-    await page.fill('input[name="displayTo-month"]', "06");
-    await page.fill('input[name="displayTo-year"]', "2025");
+    await page.fill('input[name="displayFrom-day"]', dates.displayFrom.day);
+    await page.fill('input[name="displayFrom-month"]', dates.displayFrom.month);
+    await page.fill('input[name="displayFrom-year"]', dates.displayFrom.year);
+    await page.fill('input[name="displayTo-day"]', dates.displayTo.day);
+    await page.fill('input[name="displayTo-month"]', dates.displayTo.month);
+    await page.fill('input[name="displayTo-year"]', dates.displayTo.year);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10f7133 and 9bab2ab.

📒 Files selected for processing (2)

• e2e-tests/tests/manual-upload.spec.ts (12 hunks)
• e2e-tests/tests/remove-publication.spec.ts (4 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• e2e-tests/tests/remove-publication.spec.ts

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g., userId, caseDetails, documentId). Booleans should use is/has/can prefix (e.g., isActive, hasAccess, canEdit).
Classes and Interfaces must use PascalCase (e.g., UserService, CaseRepository). Do NOT use I prefix for interfaces (use UserRepository not IUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with no any without justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.

Files:

• e2e-tests/tests/manual-upload.spec.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

API endpoints must use plural for collections (/api/cases, /api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).

Files:

• e2e-tests/tests/manual-upload.spec.ts

🧠 Learnings (1)

📚 Learning: 2025-11-25T06:40:24.099Z

Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/utils.ts : Don't create generic files like utils.ts. Be specific with filenames (e.g., object-properties.ts, date-formatting.ts).

Applied to files:

• e2e-tests/tests/manual-upload.spec.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: SonarQube Analysis
• GitHub Check: E2E Tests

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.5% Coverage on New Code
0.5% Duplication on New Code

See analysis details on SonarQube Cloud