VIBE-162 Add non-strategic Excel upload pages #153
Conversation
WalkthroughAdds a non‑strategic XLSX upload flow and centralizes file‑upload middleware and route registration: new pages (upload, summary, success), validation and storage refactors to support multiple upload types, session-backed PRG flow, templates/translations, unit and e2e tests, and a new createFileUploadMiddleware plus route-list exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Browser as Browser
participant Server as Express App
participant Session as Session Store
participant UploadStore as Upload Store (Redis)
participant ArtefactSvc as Publication/Artefact Service
participant FileStore as Blob/File Storage
Admin->>Browser: GET /non-strategic-upload
Browser->>Server: request
Server->>Session: load session
Server->>Browser: render upload form (en/cy)
Admin->>Browser: submit form + XLSX
Browser->>Server: POST /non-strategic-upload (multipart)
Server->>Server: createFileUploadMiddleware -> multer -> set req.file / req.fileUploadError
Server->>Server: validate form and file
alt validation fails
Server->>Session: store form + errors
Server->>Browser: render form with errors
else validation passes
Server->>UploadStore: storeUpload(...) -> uploadId
Server->>Session: persist uploadId
Server->>Browser: redirect to /non-strategic-upload-summary?uploadId=...
end
Admin->>Browser: GET /non-strategic-upload-summary?uploadId=...
Browser->>Server: request
Server->>UploadStore: getUpload(uploadId)
Server->>Server: resolve metadata (location/list type)
Server->>Browser: render summary
Admin->>Browser: POST confirm
Browser->>Server: POST /non-strategic-upload-summary
Server->>UploadStore: getUpload(uploadId)
Server->>ArtefactSvc: create artefact (metadata) -> artefactId
Server->>FileStore: save uploaded file as artefactId
Server->>Session: set nonStrategicUploadConfirmed
Server->>Browser: redirect to /non-strategic-upload-success
Admin->>Browser: GET /non-strategic-upload-success
Browser->>Server: request
Server->>Session: verify nonStrategicUploadConfirmed
Server->>Browser: render success and clear flags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
libs/*/src/config.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{test,spec}.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (5)📚 Learning: 2025-12-01T11:31:12.342ZApplied to files:
📚 Learning: 2025-12-01T11:31:12.342ZApplied to files:
📚 Learning: 2025-12-01T11:31:12.342ZApplied to files:
📚 Learning: 2025-12-01T11:31:12.342ZApplied to files:
📚 Learning: 2025-12-01T11:31:12.342ZApplied to files:
🧬 Code graph analysis (2)apps/web/src/app.test.ts (2)
apps/web/src/app.ts (8)
⏰ 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)
🔇 Additional comments (7)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
apps/web/src/app.ts (1)
135-143: Non-strategic upload middleware mirrors existing upload pattern correctlyThis follows the same
upload.single("file")+req.fileUploadErrorpattern used for/manual-uploadand/reference-data-upload, so behaviour should be consistent across all admin uploads.If this pattern spreads further, consider a small helper like
registerUploadRoute(app, path, upload)to DRY up the three wrapper blocks, but it’s not urgent.libs/admin-pages/src/pages/non-strategic-upload-success/index.ts (1)
6-27: Session guard and auth wiring look good; consider clarifyingnonStrategicSuccessPageViewedusageThe GET handler correctly:
- Enforces admin roles via
requireRole(...).- Derives locale from
lngand selects the right translation bundle.- Redirects back to
/non-strategic-uploadwhennonStrategicUploadConfirmedis missing, which prevents direct access and success-page refresh issues.- Clears
nonStrategicUploadConfirmedbefore rendering.One small nit: this is the only place in the snippet that touches
nonStrategicSuccessPageViewed, and it’s only ever deleted. If that flag isn’t being set or read anywhere else, you could safely drop thedelete req.session.nonStrategicSuccessPageViewed;line to reduce noise; if it is part of a wider flow, a brief comment or test asserting its behaviour would help future readers.libs/admin-pages/src/pages/non-strategic-upload-success/index.test.ts (1)
5-26: Extract callHandler to a shared test utility.The
callHandlerhelper is duplicated across multiple test files (also in non-strategic-upload/index.test.ts). Extract this to a shared test utility module to follow DRY principles.libs/admin-pages/src/pages/non-strategic-upload/index.test.ts (1)
5-26: Extract callHandler to a shared test utility.The
callHandlerhelper is duplicated across multiple test files. Extract this to a shared test utility module to follow DRY principles and improve maintainability.libs/admin-pages/src/manual-upload/storage.test.ts (1)
164-308: Refactor to eliminate test duplication.The non-strategic-upload-storage test suite duplicates 145 lines of test logic from the manual-upload-storage suite, differing only in function names, Redis key prefixes, and file extensions. This violates DRY principles and doubles the maintenance burden.
Consider extracting shared test logic into a reusable helper function:
+function createStorageTests( + storeName: string, + storeFunction: typeof storeManualUpload, + getFunction: typeof getManualUpload, + keyPrefix: string, + fileExtension: string, + mimeType: string +) { + describe(`${storeName}-storage`, () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe(`store${storeName}`, () => { + it("should store upload data in Redis with 1 hour TTL", async () => { + const testData = { + file: Buffer.from("test file content"), + fileName: `test.${fileExtension}`, + fileType: mimeType, + locationId: "123", + listType: "CIVIL_DAILY_CAUSE_LIST", + hearingStartDate: { day: "15", month: "03", year: "2024" }, + sensitivity: "PUBLIC", + language: "ENGLISH", + displayFrom: { day: "15", month: "03", year: "2024" }, + displayTo: { day: "16", month: "03", year: "2024" } + }; + + mockRedisClient.setEx.mockResolvedValue("OK"); + const uploadId = await storeFunction(testData); + + expect(uploadId).toBe("test-uuid-123"); + expect(mockRedisClient.setEx).toHaveBeenCalledWith(`${keyPrefix}:test-uuid-123`, 3600, expect.any(String)); + // ... rest of assertions + }); + // ... other tests + }); + }); +} + +createStorageTests( + "manual-upload", + storeManualUpload, + getManualUpload, + "manual-upload", + "pdf", + "application/pdf" +); + +createStorageTests( + "non-strategic-upload", + storeNonStrategicUpload, + getNonStrategicUpload, + "non-strategic-upload", + "xlsx", + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" +);libs/admin-pages/src/pages/non-strategic-upload/index.njk (1)
31-39: Consider using the imported govukWarningText macro.The custom warning block implementation uses manual HTML and custom CSS classes (
.manual-upload-warning), but thegovukWarningTextmacro is already imported on line 8 and provides the same functionality with standard GOV.UK styling.Apply this diff to use the standard component:
-<div class="manual-upload-warning"> - <h2 class="govuk-heading-m">{{ warningTitle }}</h2> - <div class="manual-upload-warning__content"> - <span class="govuk-warning-text__icon manual-upload-warning__icon" aria-hidden="true">!</span> - <p class="govuk-body manual-upload-warning__text"> - <strong>{{ warningMessage }}</strong> - </p> - </div> -</div> +<h2 class="govuk-heading-m">{{ warningTitle }}</h2> +{{ govukWarningText({ + text: warningMessage, + iconFallbackText: "Warning" +}) }}libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts (1)
29-36: Guard against non-numeric IDs before usingparseIntresultsYou’re parsing
locationIdandlistTypein several places and then immediately using the numeric values:const location = await getLocationById(Number.parseInt(uploadData.locationId, 10)); const listTypeId = Number.parseInt(uploadData.listType, 10); // ... const listTypeId = uploadData.listType ? Number.parseInt(uploadData.listType, 10) : null;Validation should already ensure these are numeric, but adding a lightweight guard avoids surprising failures if upstream validation ever changes and prevents
NaNfrom reachinggetLocationByIdorcreateArtefact.For example:
- const location = await getLocationById(Number.parseInt(uploadData.locationId, 10)); + const locationId = Number.parseInt(uploadData.locationId, 10); + const location = Number.isNaN(locationId) ? null : await getLocationById(locationId); - const listTypeId = Number.parseInt(uploadData.listType, 10); + const listTypeId = Number.parseInt(uploadData.listType, 10); + if (Number.isNaN(listTypeId)) { + throw new Error("Invalid list type ID"); + }Similar checks could be applied in the catch path. This is defensive but keeps the behaviour predictable if invalid data ever sneaks in.
Also applies to: 89-90, 137-143
libs/admin-pages/src/manual-upload/storage.ts (1)
7-84: Shared upload storage abstraction looks solid; consider centralising prefixesThe
UploadDatashape and thestoreUpload/getUploadhelpers are straightforward, and the manual/non‑strategic wrappers via"manual-upload"and"non-strategic-upload"prefixes give you a nice, reusable pattern. Encoding the file as base64 and keeping a 1‑hour TTL mirror the existing behaviour, so no correctness issues stand out.If you expect to add more upload flavours later, you could optionally centralise the key prefixes in a small enum/const object to avoid typos:
const UPLOAD_PREFIX = { MANUAL: "manual-upload", NON_STRATEGIC: "non-strategic-upload", } as const;and then reuse those in the wrappers. Not essential, but it tightens things up slightly.
libs/admin-pages/src/manual-upload/validation.ts (1)
31-55: UseerrorMessagesfor JSON validation errors to preserve i18nInside the JSON validation block you currently emit hard‑coded English strings:
errors.push({ text: `Invalid JSON file format. ${firstError?.message || "Please check the JSON structure."}`, href: "#file" }); … errors.push({ text: "Invalid JSON file format. Please ensure the file contains valid JSON.", href: "#file" });Because these don’t go through
errorMessages, Welsh flows will always see English here, unlike the rest of the validation messages.To keep behaviour consistent:
- Add JSON‑specific message entries to the relevant
errorMessagesin the manual and non‑strategic translation files (e.g.invalidJsonWithDetail,invalidJsonGeneric).- Replace the string literals here with those properties, e.g.:
- const firstError = validationResult.errors[0] as { message?: string } | undefined; - errors.push({ - text: `Invalid JSON file format. ${firstError?.message || "Please check the JSON structure."}`, - href: "#file" - }); + const firstError = validationResult.errors[0] as { message?: string } | undefined; + const detail = firstError?.message ?? errorMessages.invalidJsonDetailFallback; + errors.push({ + text: `${errorMessages.invalidJsonWithDetail} ${detail}`, + href: "#file" + }); … - } catch { - errors.push({ - text: "Invalid JSON file format. Please ensure the file contains valid JSON.", - href: "#file" - }); + } catch { + errors.push({ + text: errorMessages.invalidJsonGeneric, + href: "#file" + });This keeps all validation messaging routed through the translation layer while retaining the helpful JSON schema feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/web/src/app.ts(1 hunks)docs/tickets/VIBE-162/plan.md(1 hunks)docs/tickets/VIBE-162/tasks.md(1 hunks)docs/tickets/VIBE-162/ticket.md(1 hunks)e2e-tests/tests/non-strategic-upload.spec.ts(1 hunks)libs/admin-pages/src/manual-upload/model.ts(2 hunks)libs/admin-pages/src/manual-upload/storage.test.ts(2 hunks)libs/admin-pages/src/manual-upload/storage.ts(4 hunks)libs/admin-pages/src/manual-upload/validation.test.ts(24 hunks)libs/admin-pages/src/manual-upload/validation.ts(2 hunks)libs/admin-pages/src/pages/manual-upload-summary/index.test.ts(2 hunks)libs/admin-pages/src/pages/manual-upload/index.ts(3 hunks)libs/admin-pages/src/pages/non-strategic-upload-success/cy.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-success/en.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-success/index.njk(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-success/index.test.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-success/index.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-summary/cy.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-summary/en.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-summary/index.njk(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-summary/index.test.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/cy.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/en.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/index.njk(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/index.test.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/index.ts(1 hunks)
🧰 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/non-strategic-upload-success/en.tslibs/admin-pages/src/pages/non-strategic-upload/index.test.tslibs/admin-pages/src/pages/non-strategic-upload/en.tslibs/admin-pages/src/pages/non-strategic-upload-summary/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.test.tslibs/admin-pages/src/manual-upload/storage.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.tsapps/web/src/app.tslibs/admin-pages/src/pages/non-strategic-upload/cy.tslibs/admin-pages/src/pages/non-strategic-upload/index.tslibs/admin-pages/src/pages/non-strategic-upload-success/cy.tslibs/admin-pages/src/pages/non-strategic-upload-summary/en.tse2e-tests/tests/non-strategic-upload.spec.tslibs/admin-pages/src/pages/manual-upload/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.tslibs/admin-pages/src/manual-upload/validation.tslibs/admin-pages/src/manual-upload/validation.test.tslibs/admin-pages/src/manual-upload/storage.tslibs/admin-pages/src/manual-upload/model.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/non-strategic-upload-success/en.tslibs/admin-pages/src/pages/non-strategic-upload/index.test.tslibs/admin-pages/src/pages/non-strategic-upload/en.tslibs/admin-pages/src/pages/non-strategic-upload-summary/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.test.tslibs/admin-pages/src/manual-upload/storage.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.tsapps/web/src/app.tslibs/admin-pages/src/pages/non-strategic-upload/cy.tslibs/admin-pages/src/pages/non-strategic-upload/index.tslibs/admin-pages/src/pages/non-strategic-upload-success/cy.tslibs/admin-pages/src/pages/non-strategic-upload-summary/en.tse2e-tests/tests/non-strategic-upload.spec.tslibs/admin-pages/src/pages/manual-upload/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.tslibs/admin-pages/src/manual-upload/validation.tslibs/admin-pages/src/manual-upload/validation.test.tslibs/admin-pages/src/manual-upload/storage.tslibs/admin-pages/src/manual-upload/model.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/non-strategic-upload-success/en.tslibs/admin-pages/src/pages/non-strategic-upload/index.test.tslibs/admin-pages/src/pages/non-strategic-upload/en.tslibs/admin-pages/src/pages/non-strategic-upload-summary/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.tslibs/admin-pages/src/pages/non-strategic-upload/cy.tslibs/admin-pages/src/pages/non-strategic-upload/index.tslibs/admin-pages/src/pages/non-strategic-upload-success/cy.tslibs/admin-pages/src/pages/non-strategic-upload-summary/en.tslibs/admin-pages/src/pages/manual-upload/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/admin-pages/src/pages/non-strategic-upload/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.test.tslibs/admin-pages/src/manual-upload/storage.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tse2e-tests/tests/non-strategic-upload.spec.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.test.tslibs/admin-pages/src/manual-upload/validation.test.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/non-strategic-upload-success/index.njklibs/admin-pages/src/pages/non-strategic-upload-summary/index.njklibs/admin-pages/src/pages/non-strategic-upload/index.njk
🧠 Learnings (9)
📚 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/non-strategic-upload-success/en.tslibs/admin-pages/src/pages/non-strategic-upload/en.tslibs/admin-pages/src/pages/non-strategic-upload-summary/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.tslibs/admin-pages/src/pages/non-strategic-upload/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/cy.tslibs/admin-pages/src/pages/non-strategic-upload-summary/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/non-strategic-upload-summary/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/index.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/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/non-strategic-upload-summary/cy.tslibs/admin-pages/src/pages/non-strategic-upload/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/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/non-strategic-upload-summary/cy.tslibs/admin-pages/src/pages/non-strategic-upload/cy.tslibs/admin-pages/src/pages/non-strategic-upload-success/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 : Page controllers must export named `GET` and/or `POST` functions with Express Request and Response types.
Applied to files:
libs/admin-pages/src/pages/non-strategic-upload-success/index.tslibs/admin-pages/src/pages/non-strategic-upload/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/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:
apps/web/src/app.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/non-strategic-upload-success/index.njklibs/admin-pages/src/pages/non-strategic-upload-summary/index.njklibs/admin-pages/src/pages/non-strategic-upload/index.njk
📚 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/admin-pages/src/manual-upload/validation.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/manual-upload/validation.test.ts
🧬 Code graph analysis (10)
libs/admin-pages/src/pages/non-strategic-upload-success/en.ts (1)
libs/admin-pages/src/pages/non-strategic-upload/en.ts (1)
en(1-60)
libs/admin-pages/src/pages/non-strategic-upload-success/index.test.ts (4)
libs/admin-pages/src/pages/manual-upload/index.ts (1)
GET(146-146)libs/admin-pages/src/pages/non-strategic-upload-success/index.ts (1)
GET(27-27)libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts (1)
GET(173-173)libs/admin-pages/src/pages/non-strategic-upload/index.ts (1)
GET(145-145)
libs/admin-pages/src/manual-upload/storage.test.ts (1)
libs/admin-pages/src/manual-upload/storage.ts (2)
storeNonStrategicUpload(78-80)getNonStrategicUpload(82-84)
libs/admin-pages/src/pages/non-strategic-upload/cy.ts (1)
libs/admin-pages/src/pages/non-strategic-upload-success/cy.ts (1)
cy(1-9)
libs/admin-pages/src/pages/non-strategic-upload/index.ts (6)
libs/admin-pages/src/manual-upload/model.ts (3)
SENSITIVITY_LABELS(20-24)LANGUAGE_LABELS(26-30)UploadFormData(4-13)libs/admin-pages/src/pages/non-strategic-upload/cy.ts (1)
cy(1-60)libs/admin-pages/src/pages/non-strategic-upload/en.ts (1)
en(1-60)libs/publication/src/index.ts (1)
Language(2-2)libs/admin-pages/src/manual-upload/validation.ts (1)
validateNonStrategicUploadForm(123-129)libs/admin-pages/src/manual-upload/storage.ts (1)
storeNonStrategicUpload(78-80)
libs/admin-pages/src/pages/non-strategic-upload-success/cy.ts (1)
libs/admin-pages/src/pages/non-strategic-upload/cy.ts (1)
cy(1-60)
e2e-tests/tests/non-strategic-upload.spec.ts (1)
e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
libs/admin-pages/src/pages/manual-upload/index.ts (2)
libs/admin-pages/src/manual-upload/model.ts (1)
UploadFormData(4-13)libs/admin-pages/src/manual-upload/validation.ts (1)
validateManualUploadForm(115-121)
libs/admin-pages/src/manual-upload/validation.test.ts (1)
libs/admin-pages/src/manual-upload/validation.ts (2)
validateManualUploadForm(115-121)validateNonStrategicUploadForm(123-129)
libs/admin-pages/src/manual-upload/storage.ts (1)
libs/redis/src/index.ts (1)
getRedisClient(6-16)
🪛 LanguageTool
docs/tickets/VIBE-162/tasks.md
[style] ~234-~234: Consider using a different verb for a more formal wording.
Context: ... checks - [ ] Run yarn lint:fix and fix any issues - [ ] Run yarn format to...
(FIX_RESOLVE)
docs/tickets/VIBE-162/ticket.md
[uncategorized] ~7-~7: Capitalize this word if you mean the computer program “Excel”.
Context: ...lishing route requires the upload of an excel file in CaTH which is then transformed ...
(ON_EXCEL)
[uncategorized] ~10-~10: Capitalize this word if you mean the computer program “Excel”.
Context: ...A** Local Admin I WANT to upload an excel file in CaTH SO THAT I can publish ...
(ON_EXCEL)
[style] ~15-~15: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...# Acceptance Criteria 1. A local admin is able to access a verified account by signing in...
(BE_ABLE_TO)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...d account by signing in through the sign in page with their approved log in detai...
(QB_NEW_EN_HYPHEN)
[style] ~16-~16: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...roved log in details 2. The local admin is able to see the 'Upload excel file' tab on the ...
(BE_ABLE_TO)
[uncategorized] ~16-~16: Capitalize this word if you mean the computer program “Excel”.
Context: ... local admin is able to see the 'Upload excel file' tab on the dashboard and is taken...
(ON_EXCEL)
[uncategorized] ~16-~16: Capitalize this word if you mean the computer program “Excel”.
Context: ...ab on the dashboard and is taken to the excel file upload form when the tab is clicke...
(ON_EXCEL)
[uncategorized] ~17-~17: Capitalize this word if you mean the computer program “Excel”.
Context: ... when the tab is clicked 3. The 'Upload excel file' tab displays the descriptive mess...
(ON_EXCEL)
[uncategorized] ~17-~17: Capitalize this word if you mean the computer program “Excel”.
Context: ...lays the descriptive message 'Upload an excel file to be converted and displayed on t...
(ON_EXCEL)
[grammar] ~17-~17: Use a hyphen to join words.
Context: ... converted and displayed on the external facing service on GOV.UK.' 4. The top of...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~18-~18: Capitalize this word if you mean the computer program “Excel”.
Context: ...g service on GOV.UK.' 4. The top of the excel file upload form displays a 'warning' w...
(ON_EXCEL)
[style] ~18-~18: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...on sign and the boldly written message 'Prior to upload you must ensure the file is suit...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[uncategorized] ~19-~19: Capitalize this word if you mean the computer program “Excel”.
Context: ...ng the production of this file.' 5. The excel file upload form displays various data ...
(ON_EXCEL)
[uncategorized] ~114-~114: Capitalize this word if you mean the computer program “Excel”.
Context: ...lsh Content Title/H1 - EN: "Upload excel file" - CY: "Welsh placeholder" **Intr...
(ON_EXCEL)
[style] ~118-~118: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...older" Introductory warning - EN: "Prior to upload you must ensure the file is suit...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
docs/tickets/VIBE-162/plan.md
[uncategorized] ~293-~293: Capitalize this word if you mean the computer program “Excel”.
Context: ...e already in dashboard tile: "Upload an excel file to be converted and displayed on t...
(ON_EXCEL)
[grammar] ~293-~293: Use a hyphen to join words.
Context: ... converted and displayed on the external facing service on GOV.UK." - Content in ...
(QB_NEW_EN_HYPHEN)
[style] ~303-~303: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...Bold text with warning icon - Content: "Prior to upload you must ensure the file is suit...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[uncategorized] ~368-~368: Capitalize this word if you mean the computer program “Excel”.
Context: ... - Dashboard description: "Upload an excel file" - Accepted formats in spec: cs...
(ON_EXCEL)
🪛 markdownlint-cli2 (0.18.1)
docs/tickets/VIBE-162/ticket.md
26-26: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
36-36: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
41-41: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
46-46: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
51-51: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
56-56: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
157-157: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
165-165: 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
🔇 Additional comments (12)
libs/admin-pages/src/pages/manual-upload-summary/index.test.ts (1)
349-379: Updated label expectations look consistent with shared constantsThe new expected labels for
PRIVATE(“Private – all verified users”) andBILINGUAL(“Bilingual English/Welsh”) match the updated copy and keep the mapping tests in sync with the shared label definitions. No issues from a test-behaviour standpoint.Also applies to: 381-411
libs/admin-pages/src/pages/non-strategic-upload-success/cy.ts (1)
1-9: Welsh strings are placeholders – ensure follow-up translation before releaseThe
cybundle has the right shape and keys, but all values are “[Welsh] …” placeholders. That’s fine for scaffolding, but please make sure there’s a follow-up step (ticket or checklist item) to replace these with real translations before exposing the non‑strategic upload success page to Welsh users.libs/admin-pages/src/pages/non-strategic-upload-success/index.njk (1)
1-33: Success template structure and GOV.UK component usage look goodExtending the base layout, using
govukPanelfor the confirmation message, and rendering the “next steps” links in a two‑thirds column all align with the existing pattern in this app. Nothing concerning here.libs/admin-pages/src/pages/non-strategic-upload-success/en.ts (1)
1-9: English success-page copy is consistent and clearThe
enbundle covers all fields used by the template with clear, concise copy and a page title that matches existing GOV.UK service naming conventions.libs/admin-pages/src/pages/non-strategic-upload-summary/cy.ts (1)
1-14: Replace placeholder Welsh translations with actual Welsh content.All Welsh strings are prefixed with "[Welsh]" placeholders. Based on learnings, Welsh translations are required for all user-facing text and should not be skipped.
Based on learnings, don't skip Welsh translations - required for all user-facing text.
⛔ Skipped due to learnings
Learnt from: CR Repo: hmcts/cath-service PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-01T11:31:12.342Z Learning: Don't skip Welsh translations - required for all user-facing text.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.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.Learnt from: junaidiqbalmoj Repo: hmcts/cath-service PR: 137 File: e2e-tests/tests/create-media-account.spec.ts:51-64 Timestamp: 2025-11-27T14:18:22.922Z Learning: For the create-media-account form in libs/public-pages, the English email validation error message (errorEmailInvalid) should be: "There is a problem - Enter a valid email address, e.g. nameexample.com" to match the Welsh translation and clearly indicate the format requirement rather than suggesting the field is empty.libs/admin-pages/src/pages/non-strategic-upload-summary/index.njk (1)
1-1: No action required. This template correctly extendslayouts/base-template.njk, which is the actual filename used consistently throughout the codebase. All other templates in libs/admin-pages use the same singular form.libs/admin-pages/src/pages/non-strategic-upload-summary/en.ts (1)
1-14: LGTM!The English translations are well-structured, complete, and follow the established pattern for locale files in the codebase.
libs/admin-pages/src/pages/non-strategic-upload-summary/index.test.ts (1)
1-371: LGTM!The test suite is comprehensive, well-structured, and covers all critical paths including authentication, data retrieval, validation, error handling, session management, and Welsh language support.
docs/tickets/VIBE-162/plan.md (1)
1-407: LGTM - File format clarification needed.The technical plan is comprehensive and well-structured. The file format discrepancy (title says "Excel" but spec lists csv, doc, docx, htm, html, json, pdf without .xls/.xlsx) is already documented in the "Open Questions" section (lines 366-371), demonstrating good awareness of the ambiguity.
The team should resolve the file format question before finalizing the implementation to ensure consistency between the feature name, accepted file types, and validation logic.
libs/admin-pages/src/pages/non-strategic-upload/cy.ts (1)
1-60: LGTM - Placeholder Welsh translations present.The Welsh locale file is properly structured with all required labels and error messages. The "[Welsh]" prefixes indicate placeholder content awaiting official translation, which is consistent with the approach documented in the technical plan.
Note: Line 7 mentions
.xlsxfile format, confirming this feature is for Excel files despite the plan.md listing other formats. This reinforces the need to resolve the file format clarification mentioned in the plan.libs/admin-pages/src/pages/non-strategic-upload/index.ts (2)
120-122: Preserve locale parameter in error redirect.When validation fails and the user is redirected back to the form, the Welsh language parameter (if present) is lost.
Apply this diff:
if (errors.length > 0) { req.session.nonStrategicUploadErrors = errors; req.session.nonStrategicUploadForm = formData; await saveSession(req.session); - return res.redirect("/non-strategic-upload"); + const langParam = locale === "cy" ? "?lng=cy" : ""; + return res.redirect(`/non-strategic-upload${langParam}`); }⛔ Skipped due to learnings
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.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.
60-62: Add Welsh language support.The locale is hardcoded to
"en"despite the existence of Welsh translations incy.ts. This violates the coding guideline that every page must support both English and Welsh.Apply this diff to add Welsh support:
const getHandler = async (req: Request, res: Response) => { - const locale = "en"; + const locale = (req.query.lng === "cy" ? "cy" : "en") as "en" | "cy"; const t = getTranslations(locale);Also update the POST handler:
const postHandler = async (req: Request, res: Response) => { - const locale = "en" as "en" | "cy"; + const locale = (req.query.lng === "cy" ? "cy" : "en") as "en" | "cy"; const t = getTranslations(locale);And preserve the language parameter in the redirect:
- res.redirect(`/non-strategic-upload-summary?uploadId=${uploadId}`); + const langParam = locale === "cy" ? "?lng=cy" : ""; + res.redirect(`/non-strategic-upload-summary?uploadId=${uploadId}${langParam ? `&lng=cy` : ""}`);Based on coding guidelines.
⛔ Skipped due to learnings
Learnt from: CR Repo: hmcts/cath-service PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-01T11:31:12.342Z Learning: Applies to **/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.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.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.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.
| return res.render("non-strategic-upload-summary/index", { | ||
| pageTitle: lang.pageTitle, | ||
| heading: lang.heading, | ||
| subHeading: lang.subHeading, | ||
| courtName: lang.courtName, | ||
| file: lang.file, | ||
| listType: lang.listType, | ||
| hearingStartDate: lang.hearingStartDate, | ||
| sensitivity: lang.sensitivity, | ||
| language: lang.language, | ||
| displayFileDates: lang.displayFileDates, | ||
| change: lang.change, | ||
| confirmButton: lang.confirmButton, | ||
| data: { | ||
| courtName, | ||
| file: uploadData.fileName, | ||
| listType: listTypeName, | ||
| hearingStartDate: formatDate(uploadData.hearingStartDate), | ||
| sensitivity: SENSITIVITY_LABELS[uploadData.sensitivity] || uploadData.sensitivity, | ||
| language: LANGUAGE_LABELS[uploadData.language] || uploadData.language, | ||
| displayFileDates: formatDateRange(uploadData.displayFrom, uploadData.displayTo) | ||
| }, | ||
| errors: [{ text: "We could not process your upload. Please try again.", href: "#" }], | ||
| hideLanguageToggle: true | ||
| }); |
There was a problem hiding this comment.
Localise generic upload error and consider extracting shared view-model builder
The error passed to errors is hard‑coded in English:
errors: [{ text: "We could not process your upload. Please try again.", href: "#" }],so Welsh (?lng=cy) users will still see an English message. It also duplicates most of the GET view‑model construction.
I’d recommend:
- Moving the error text into the
en/cytranslation files (e.g.lang.uploadError) and using that here for full i18n coverage. - Optionally extracting a small helper like
buildSummaryViewModel(uploadData, locale, lang)that both the GET handler and this catch block can call, to avoid duplicating thecourtName,listTypeName, anddataconstruction.
This keeps the page fully bilingual and reduces the chance of the two paths diverging over time.
🤖 Prompt for AI Agents
In libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts around lines
145 to 169, the catch block returns a hard‑coded English error message and
duplicates most of the GET view‑model construction; move the error text into the
locale files (e.g. lang.uploadError in both en and cy) and replace the
hard‑coded string with lang.uploadError, and refactor the shared view‑model
creation into a small helper (e.g. buildSummaryViewModel(uploadData, locale,
lang)) that constructs courtName, listTypeName and the data object so both the
GET handler and this catch block call that helper and pass its result plus
errors: [{ text: lang.uploadError, href: "#" }] to res.render.
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 (1)
libs/admin-pages/src/pages/manual-upload/index.test.ts(9 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/manual-upload/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/manual-upload/index.test.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/admin-pages/src/pages/manual-upload/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/manual-upload/index.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
🔇 Additional comments (1)
libs/admin-pages/src/pages/manual-upload/index.test.ts (1)
56-56: LGTM!The function rename from
validateFormtovalidateManualUploadFormis applied consistently across mock definition, import, and all test assertions. The synchronous mocking approach usingmockReturnValueis correct and aligns with the initial mock definition.Also applies to: 64-64, 548-548, 652-652, 656-656, 693-693, 739-739, 783-783, 823-823, 861-861
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
e2e-tests/tests/non-strategic-upload.spec.ts (4)
12-21: Fail fast when SSO env vars are missing inauthenticateSystemAdminRight now,
SSO_TEST_SYSTEM_ADMIN_EMAIL/SSO_TEST_SYSTEM_ADMIN_PASSWORDare asserted non‑null with!and only used when the URL includeslogin.microsoftonline.com. If those env vars are unset but SSO is actually enabled, you’ll passundefinedintologinWithSSO, leading to harder‑to‑diagnose failures later in the flow.Consider explicitly validating the env vars before calling
loginWithSSOso failures are clearer:async function authenticateSystemAdmin(page: Page) { await page.goto("/system-admin-dashboard"); - if (page.url().includes("login.microsoftonline.com")) { - const systemAdminEmail = process.env.SSO_TEST_SYSTEM_ADMIN_EMAIL!; - const systemAdminPassword = process.env.SSO_TEST_SYSTEM_ADMIN_PASSWORD!; - await loginWithSSO(page, systemAdminEmail, systemAdminPassword); - } + if (page.url().includes("login.microsoftonline.com")) { + const systemAdminEmail = process.env.SSO_TEST_SYSTEM_ADMIN_EMAIL; + const systemAdminPassword = process.env.SSO_TEST_SYSTEM_ADMIN_PASSWORD; + + if (!systemAdminEmail || !systemAdminPassword) { + throw new Error("SSO_TEST_SYSTEM_ADMIN_EMAIL and SSO_TEST_SYSTEM_ADMIN_PASSWORD must be set for SSO e2e tests."); + } + + await loginWithSSO(page, systemAdminEmail, systemAdminPassword); + } }
23-27: Replace fixedwaitForTimeout(1000)calls with explicit waits on page readinessMultiple tests rely on
await page.waitForTimeout(1000);afterpage.goto(...)to let the page/JS settle. This pattern can slow the suite down and is brittle in slower/faster environments.Examples:
- Line 27 (
navigateToSummaryPage)- Line 69 (keyboard accessibility test)
- Lines 143, 331, 459, 686, 722 in other flows
Prefer waiting on a specific, stable signal that the page is ready (e.g. a key control or heading becoming visible), which Playwright will auto‑wait for anyway:
- await page.goto("/non-strategic-upload?locationId=9001"); - await page.waitForTimeout(1000); + await page.goto("/non-strategic-upload?locationId=9001"); + await page.locator('input[name="file"]').waitFor({ state: "visible", timeout: 10000 });You can apply a similar pattern in the other places where a raw timeout is used.
Also applies to: 65-69, 140-144, 329-332, 457-460, 681-687, 717-723
23-41: Consider extracting a reusable helper for filling the non‑strategic upload formThe form‑filling steps (list type, hearing date, sensitivity, language, display dates, file upload) are repeated with only small variations across:
navigateToSummaryPage(lines 23‑41)- Keyboard accessibility + full flow tests (lines 65‑92, 140‑159)
- File validation, date‑range validation, sequential uploads, and mobile tests (lines 329‑345, 457‑479, 681‑699, 717‑740)
To reduce duplication and keep future changes to the form shape/field names in one place, consider extracting a helper like:
async function fillNonStrategicUploadForm(page: Page, overrides?: { listType?: string; hearingDate?: { day: string; month: string; year: string }; sensitivity?: string; language?: string; displayFrom?: { day: string; month: string; year: string }; displayTo?: { day: string; month: string; year: string }; }) { // apply sensible defaults then override as needed }Then each test can call this helper with only the fields it cares about. That should make the intent clearer and keep tests easier to update when the form evolves.
Also applies to: 65-92, 140-159, 329-345, 457-479, 681-699, 717-740
6-11: Align Axe rule disabling with the header comment, or clarify scopeThe header comment (lines 6‑11) says
target-sizeandlink-nameare disabled due to known footer issues affecting “ALL pages”. In practice:
- Form + error‑state tests disable those rules (lines 253‑256 and 321‑324).
- Summary and success WCAG tests (lines 591‑597 and 665‑671) do not disable them and assert
violationsis empty.This is a bit inconsistent and may either:
- Cause the summary/success Axe tests to fail on known global issues, or
- Drift from the documented behaviour if the footer problems really are site‑wide.
I’d suggest either:
Centralising Axe setup in a small helper that always applies the same tags and disabled rules, e.g.:
const AXE_TAGS = ["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"] as const; const AXE_DISABLED_RULES = ["target-size", "link-name"] as const; async function runAxe(page: Page) { return new AxeBuilder({ page }) .withTags(AXE_TAGS) .disableRules(AXE_DISABLED_RULES) .analyze(); }and use
runAxe(page)everywhere, orNarrowing the header comment to state exactly which pages are expected to have disabled rules and which are expected to be fully clean.
That will make the intent of the WCAG tests more obvious and avoid surprises when the global footer is fixed.
Also applies to: 253-270, 321-327, 591-597, 665-671
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e-tests/tests/non-strategic-upload.spec.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/cy.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/en.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload/index.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/admin-pages/src/pages/non-strategic-upload/en.ts
- libs/admin-pages/src/pages/non-strategic-upload/cy.ts
- libs/admin-pages/src/pages/non-strategic-upload/index.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/non-strategic-upload.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/non-strategic-upload.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/non-strategic-upload.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/non-strategic-upload.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 (2)
e2e-tests/tests/non-strategic-upload.spec.ts (2)
1-773: Strong overall coverage and structure for the non‑strategic upload flowEnd‑to‑end coverage (auth, form, summary, success, validations, keyboard and WCAG checks, responsive/mobile, Welsh) is thorough and matches the new feature surface well. Helpers for auth and journey navigation also keep the tests reasonably readable. The remaining comments are mostly about robustness and maintainability, not about changing the high‑level approach.
636-639: Success page behavior is correct and consistent across all test scenariosThe three tests verify different stages of the upload lifecycle and are not in conflict:
- Direct access without session → redirect to
/non-strategic-upload(correct)- After upload with session →
/non-strategic-upload-success?lng=cydisplays Welsh content (correct, since session persists)- After download (session cleared) → reload redirects (correct, session invalidation is intentional)
The page correctly supports both English and Welsh as required, and the single-use behavior is enforced through session clearing in the download endpoint. Language switching is available while the session is active; this is the intended design rather than a limitation.
| test("should validate date range and preserve all form data when validation fails", async ({ page }) => { | ||
| await page.goto("/non-strategic-upload?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| const fileInput = page.locator('input[name="file"]'); | ||
| await fileInput.setInputFiles({ | ||
| name: "test.xlsx", | ||
| mimeType: "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", | ||
| buffer: Buffer.from("PK\x03\x04") // Mock Excel file header | ||
| }); | ||
|
|
||
| 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.selectOption('select[name="sensitivity"]', "PUBLIC"); | ||
| await page.selectOption('select[name="language"]', "ENGLISH"); | ||
| await page.fill('input[name="displayFrom-day"]', "20"); | ||
| await page.fill('input[name="displayFrom-month"]', "06"); | ||
| await page.fill('input[name="displayFrom-year"]', "2025"); | ||
| await page.fill('input[name="displayTo-day"]', "10"); | ||
| await page.fill('input[name="displayTo-month"]', "06"); | ||
| await page.fill('input[name="displayTo-year"]', "2025"); | ||
|
|
||
| const continueButton = page.getByRole("button", { name: /continue/i }); | ||
| await continueButton.click(); | ||
|
|
||
| const errorSummary = page.locator(".govuk-error-summary"); | ||
| await expect(errorSummary).toBeVisible(); | ||
| const errorLink = errorSummary.getByRole("link", { name: /'display to' date must be the same as or later than 'display from' date/i }); | ||
| await expect(errorLink).toBeVisible(); | ||
|
|
||
| const inlineError = page.locator("#displayTo-error.govuk-error-message"); | ||
| await expect(inlineError).toBeVisible(); | ||
| await expect(inlineError).toContainText(/'display to' date must be the same as or later than 'display from' date/i); | ||
|
|
||
| await page.selectOption('select[name="sensitivity"]', "PRIVATE"); | ||
| await page.selectOption('select[name="language"]', "WELSH"); | ||
| await page.fill('input[name="displayFrom-day"]', "10"); | ||
| await page.fill('input[name="displayTo-day"]', "20"); | ||
|
|
||
| await continueButton.click(); | ||
|
|
||
| await expect(page.locator('select[name="listType"]')).toHaveValue("1"); | ||
| await expect(page.locator('input[name="hearingStartDate-day"]')).toHaveValue("15"); | ||
| await expect(page.locator('input[name="hearingStartDate-month"]')).toHaveValue("06"); | ||
| await expect(page.locator('input[name="hearingStartDate-year"]')).toHaveValue("2025"); | ||
| await expect(page.locator('select[name="sensitivity"]')).toHaveValue("PRIVATE"); | ||
| await expect(page.locator('select[name="language"]')).toHaveValue("WELSH"); | ||
| await expect(page.locator('input[name="displayFrom-day"]')).toHaveValue("10"); | ||
| await expect(page.locator('input[name="displayFrom-month"]')).toHaveValue("06"); | ||
| await expect(page.locator('input[name="displayFrom-year"]')).toHaveValue("2025"); | ||
| await expect(page.locator('input[name="displayTo-day"]')).toHaveValue("20"); | ||
| await expect(page.locator('input[name="displayTo-month"]')).toHaveValue("06"); | ||
| await expect(page.locator('input[name="displayTo-year"]')).toHaveValue("2025"); | ||
| }); |
There was a problem hiding this comment.
Clarify the intended behaviour in the date‑range validation preservation test
In should validate date range and preserve all form data when validation fails:
- First submission uses
displayFrom>displayTo, correctly triggering the error summary and inline error (lines 468‑491). - Then the test changes sensitivity, language, and makes the date range appear valid (
displayFrom= 10,displayTo= 20) and clicks Continue again (lines 493‑498). - After that second click, it asserts that all form fields still have their values (lines 500‑511).
If the second set of values is intended to be valid, the flow should normally move to the summary page, and these form field expectations would fail because the inputs are no longer present. If the second submission is still supposed to fail, it’d be good to assert the specific error that keeps you on the page, to make that expectation explicit.
It might be clearer to:
- Assert preservation of all field values immediately after the first failing submission (before changing any values), or
- Split this into two tests: one for “error when displayTo < displayFrom” and another for “all entered values are preserved when any validation fails”.
As written, the test’s name (“when validation fails”) and the second set of interactions appear slightly at odds.
🤖 Prompt for AI Agents
In e2e-tests/tests/non-strategic-upload.spec.ts around lines 457 to 512, the
test submits an invalid date range, then modifies fields and submits again but
still asserts the form fields remain on the same page — which conflicts with the
normal successful navigation flow; fix by either (A) move the assertions that
all form values are preserved to immediately after the first failing submit
(before changing sensitivity/language/dates), or (B) split into two tests: one
that verifies the date-range validation error and preservation after the failing
submit, and a separate test that submits a valid form and asserts navigation to
the summary page (or different post-submit expectations). If you choose to keep
the second submit in the same test, make the second submission still invalid and
assert the specific error to ensure the page does not navigate.
🎭 Playwright E2E Test Results412 tests 412 ✅ 34m 39s ⏱️ Results for commit 4f43c0c. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e-tests/tests/non-strategic-upload.spec.ts (1)
457-512: Test logic issue: Second submission should navigate to summary page.As flagged in the previous review, this test has a logic problem:
- First submission (lines 468-482): Invalid date range (
displayFromday=20,displayToday=10) correctly triggers validation error ✓- Second submission (lines 493-498): Date range is fixed (
displayFromday=10,displayToday=20), making all form data valid- Problem (lines 500-511): The test asserts form fields are still present, but with valid data the page should navigate to
/non-strategic-upload-summaryThe test name says "when validation fails" but the second submission has valid data, creating a mismatch between intent and implementation.
Recommendation: Either:
- Option A (if testing preservation): Move assertions to immediately after line 491 (before changing field values), or
- Option B (if testing valid flow): After line 498, assert navigation to summary page instead of asserting form field values
🧹 Nitpick comments (3)
libs/admin-pages/src/manual-upload/model.ts (2)
32-47: Consider a helper type to reduce session state duplication.The session fields for manual and non-strategic uploads follow an identical pattern with different prefixes. While this works, you could reduce duplication with a helper type:
interface UploadFlowSessionData { form?: UploadFormData; errors?: ValidationError[]; submitted?: boolean; confirmed?: boolean; successPageViewed?: boolean; viewedLanguage?: "en" | "cy"; } declare module "express-session" { interface SessionData { manualUpload?: UploadFlowSessionData & { // manual-specific overrides if needed confirmed?: boolean; // note: different field name (uploadConfirmed vs confirmed) }; nonStrategicUpload?: UploadFlowSessionData & { confirmed?: boolean; // nonStrategicUploadConfirmed }; // Keep flat structure for backward compatibility if needed: uploadConfirmed?: boolean; } }Note: The current flat structure works fine and may be preferred for simpler session access patterns. This refactor is optional.
1-47: Consider moving shared types to a more generic location.Since
UploadFormDataand related types are now shared between manual and non-strategic uploads, the file location atmanual-upload/model.tsmight be misleading. Consider moving shared types to a more generic location likeshared/upload-models.tsorcommon/upload-types.ts.However, keeping all upload-related models in one place may be intentional for easier maintenance, so this is purely a naming/organization suggestion.
e2e-tests/tests/non-strategic-upload.spec.ts (1)
27-27: Consider replacing hard-coded timeouts with specific wait conditions.The
await page.waitForTimeout(1000)calls after navigation could potentially be replaced with more deterministic wait conditions, such as:await page.waitForLoadState('networkidle'); // or wait for a specific element that indicates the page is ready await page.locator('input[name="file"]').waitFor({ state: 'visible' });This would make tests more reliable and potentially faster. However, if the timeout is required for specific JavaScript initialization (like autocomplete components), the current approach is acceptable.
Also applies to: 69-69, 143-143, 331-331, 459-459, 692-692, 728-728
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e-tests/tests/non-strategic-upload.spec.ts(1 hunks)libs/admin-pages/src/manual-upload/model.ts(2 hunks)libs/admin-pages/src/pages/non-strategic-upload-success/cy.ts(1 hunks)libs/admin-pages/src/pages/non-strategic-upload-success/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/admin-pages/src/pages/non-strategic-upload-success/cy.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/admin-pages/src/pages/non-strategic-upload-success/index.tslibs/admin-pages/src/manual-upload/model.tse2e-tests/tests/non-strategic-upload.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:
libs/admin-pages/src/pages/non-strategic-upload-success/index.tslibs/admin-pages/src/manual-upload/model.tse2e-tests/tests/non-strategic-upload.spec.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/non-strategic-upload-success/index.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/non-strategic-upload.spec.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 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/non-strategic-upload-success/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 **/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/non-strategic-upload-success/index.ts
📚 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:
e2e-tests/tests/non-strategic-upload.spec.ts
🧬 Code graph analysis (2)
libs/admin-pages/src/pages/non-strategic-upload-success/index.ts (5)
libs/admin-pages/src/pages/non-strategic-upload-success/cy.ts (1)
cy(1-9)libs/admin-pages/src/pages/non-strategic-upload-success/en.ts (1)
en(1-9)libs/admin-pages/src/pages/manual-upload/index.ts (1)
GET(146-146)libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts (1)
GET(173-173)libs/admin-pages/src/pages/non-strategic-upload/index.ts (1)
GET(145-145)
e2e-tests/tests/non-strategic-upload.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). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (8)
libs/admin-pages/src/manual-upload/model.ts (3)
20-24: LGTM! More descriptive sensitivity label.The updated label "Private – all verified users" provides better clarity about access control. Ensure that tests and snapshots have been updated to reflect the new label text.
26-30: LGTM! More explicit language label.The updated label "Bilingual English/Welsh" is more explicit and removes ambiguity about which languages are included.
4-13: All references toManualUploadFormDatahave been successfully updated toUploadFormData.The interface rename is complete across the codebase. Verification found the new interface name in use across all relevant files: manual-upload validation, both manual-upload and non-strategic-upload pages, and the model file itself. No old references remain.
libs/admin-pages/src/pages/non-strategic-upload-success/index.ts (2)
8-36: LGTM - Session-based one-time view pattern correctly implemented.The handler properly implements a session-based success page with language toggle support:
- Validates upload confirmation before rendering
- Allows viewing the page once in each language (English and Welsh)
- Cleans up session flags after the allowed views to prevent refresh/resubmission
- Correctly uses PRG (Post-Redirect-Get) pattern to prevent form resubmission
The logic on line 21 ensures users can switch languages once but prevents unlimited refreshing of the success page.
38-38: LGTM - Role requirements consistent with related upload pages.The authorization roles match those used in
/non-strategic-upload,/non-strategic-upload-summary, and/manual-upload(lines 144, 172, and 145 in their respective files), ensuring consistent access control across the upload workflow.e2e-tests/tests/non-strategic-upload.spec.ts (3)
12-58: LGTM - Well-structured test helpers.The helper functions are properly designed for reusability:
authenticateSystemAdminhandles SSO flow correctlynavigateToSummaryPageprovides a valid form submission path for testscompleteNonStrategicUploadFlowenables full journey testing- File uploads correctly use
.xlsxformat with proper MIME type and mock ZIP header
60-196: LGTM - Comprehensive end-to-end journey tests.The complete journey tests effectively verify:
- Keyboard accessibility throughout the entire upload workflow (tab navigation, focus states, enter key submission)
- Full happy path from form entry through summary to success
- Data integrity across page transitions
- Success page content and navigation links
These integration tests provide valuable coverage of the user journey.
515-778: LGTM - Comprehensive test coverage for summary, success, and responsive design.The test suites thoroughly verify:
Summary page:
- All summary list rows, keys, values, and change links
- Proper anchor references for navigation back to form fields
- Visually hidden text for accessibility
- WCAG 2.2 AA compliance
Success page:
- Success panel content and navigation links
- Session guard (redirect when accessed without upload confirmation) - correctly tests the logic from
libs/admin-pages/src/pages/non-strategic-upload-success/index.ts:13-15- Welsh language translations
- Refresh protection (one-time view pattern) - correctly tests the session cleanup logic from
index.ts:21-26- Multiple sequential upload workflows
- Accessibility compliance
Responsive design:
- Mobile viewport rendering across the entire flow
apps/web/src/app.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| // Register non-strategic upload with file upload middleware |
There was a problem hiding this comment.
should the upload stuff be here or in a lib?
There was a problem hiding this comment.
Refactored and moved the file upload stuffs to the file upload middleware in web-core
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/admin-pages/src/manual-upload/validation.ts (1)
61-74: Clarify the locationId vs locationName validation logic.The validation logic at line 66 returns
courtRequirederror whenlocationNameis valid (>= 3 characters), which appears counterintuitive. This might be intentional if:
locationIdis always required (e.g., from a dropdown selection)locationNameis only for autocomplete/search- A valid
locationNamewithoutlocationIdmeans the user typed but didn't select from the dropdownHowever, the current code lacks clarity. Consider adding a comment explaining why a valid
locationNamealone is insufficient.If the above interpretation is correct, consider adding a clarifying comment:
function validateLocation(body: UploadFormData, errorMessages: ErrorMessages): ValidationError | null { const hasLocationId = body.locationId && body.locationId.trim() !== ""; if (!hasLocationId) { + // locationName is for autocomplete only; user must select a court to populate locationId const hasValidLocationName = body.locationName && body.locationName.trim().length >= MIN_LOCATION_NAME_LENGTH; return hasValidLocationName ? { text: errorMessages.courtRequired, href: "#court" } : { text: errorMessages.courtTooShort, href: "#court" }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/src/app.test.ts(3 hunks)apps/web/src/app.ts(3 hunks)libs/admin-pages/src/config.ts(1 hunks)libs/admin-pages/src/manual-upload/validation.ts(1 hunks)libs/system-admin-pages/src/config.ts(1 hunks)libs/web-core/src/index.ts(1 hunks)libs/web-core/src/middleware/file-upload/file-upload-middleware.ts(2 hunks)
🧰 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/config.tslibs/web-core/src/middleware/file-upload/file-upload-middleware.tslibs/web-core/src/index.tsapps/web/src/app.tslibs/system-admin-pages/src/config.tsapps/web/src/app.test.tslibs/admin-pages/src/manual-upload/validation.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/config.tslibs/web-core/src/middleware/file-upload/file-upload-middleware.tslibs/web-core/src/index.tsapps/web/src/app.tslibs/system-admin-pages/src/config.tsapps/web/src/app.test.tslibs/admin-pages/src/manual-upload/validation.ts
libs/*/src/config.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate
config.tsfile to avoid circular dependencies during Prisma client generation. Apps import config using the/configpath.
Files:
libs/admin-pages/src/config.tslibs/system-admin-pages/src/config.ts
libs/**/*-middleware.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Reusable middleware must be placed in a dedicated
libs/[module]/src/[middleware-name]-middleware.tsfile and exported as a function.
Files:
libs/web-core/src/middleware/file-upload/file-upload-middleware.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
apps/web/src/app.test.ts
🧠 Learnings (6)
📚 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/config.tsapps/web/src/app.tslibs/system-admin-pages/src/config.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/**/*-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/middleware/file-upload/file-upload-middleware.tslibs/web-core/src/index.tsapps/web/src/app.tsapps/web/src/app.test.ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Use workspace aliases (`hmcts/*`) for imports instead of relative paths across packages.
Applied to files:
apps/web/src/app.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 put business logic in apps/* - use libs/ modules for all reusable logic.
Applied to files:
apps/web/src/app.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/system-admin-pages/src/config.ts
📚 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/admin-pages/src/manual-upload/validation.ts
🧬 Code graph analysis (6)
libs/admin-pages/src/config.ts (1)
libs/system-admin-pages/src/config.ts (1)
fileUploadRoutes(10-10)
libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)
libs/web-core/src/index.ts (3)
createFileUploadMiddleware(10-10)FileUploadOptions(9-9)createFileUpload(10-10)
apps/web/src/app.ts (9)
libs/web-core/src/index.ts (1)
createFileUploadMiddleware(10-10)libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)
createFileUploadMiddleware(21-32)libs/admin-pages/src/config.ts (1)
pageRoutes(7-7)libs/system-admin-pages/src/config.ts (1)
pageRoutes(7-7)libs/list-types/civil-and-family-daily-cause-list/src/config.ts (1)
pageRoutes(7-10)libs/auth/src/config.ts (1)
pageRoutes(7-7)libs/verified-pages/src/config.ts (1)
pageRoutes(7-7)libs/public-pages/src/config.ts (1)
pageRoutes(7-7)libs/web-core/src/config.ts (1)
pageRoutes(7-7)
libs/system-admin-pages/src/config.ts (1)
libs/admin-pages/src/config.ts (1)
fileUploadRoutes(10-10)
apps/web/src/app.test.ts (2)
libs/web-core/src/index.ts (1)
createFileUploadMiddleware(10-10)libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)
createFileUploadMiddleware(21-32)
libs/admin-pages/src/manual-upload/validation.ts (3)
libs/admin-pages/src/manual-upload/model.ts (2)
ValidationError(15-18)UploadFormData(4-13)libs/list-types/common/src/list-type-validator.ts (1)
validateListTypeJson(26-91)libs/web-core/src/utils/date-utils.ts (1)
parseDate(25-41)
⏰ 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 (13)
libs/system-admin-pages/src/config.ts (1)
10-10: LGTM!The new
fileUploadRoutesexport follows the established pattern used bypageRoutesand other config exports, and correctly places config exports in theconfig.tsfile per coding guidelines.libs/admin-pages/src/config.ts (1)
10-10: LGTM!The
fileUploadRoutesexport centralizes the admin file upload route definitions, following the established config pattern and enabling the loop-based route registration inapp.ts.libs/web-core/src/index.ts (1)
10-10: LGTM!The new
createFileUploadMiddlewareexport is correctly added alongsidecreateFileUpload, using the required.jsextension for ESM imports.apps/web/src/app.test.ts (3)
23-23: LGTM!The mock correctly simulates the new
createFileUploadMiddlewarebehavior, returning a middleware function that callsnext().
162-165: LGTM!Test correctly verifies that
createFileUploadMiddlewareis called during app initialization.
180-184: LGTM!The error simulation correctly demonstrates the middleware's error propagation pattern by setting
req.fileUploadErrorand callingnext(), matching the actual implementation.apps/web/src/app.ts (3)
4-4: LGTM!Correctly uses the workspace alias
@hmcts/admin-pages/configwith aliased import forfileUploadRoutes, following the coding guidelines for cross-package imports.
13-17: LGTM!Import follows workspace alias convention and properly aliases
fileUploadRoutestosystemAdminFileUploadRoutesto distinguish from admin routes.
113-124: Good centralization of file upload route registration.The loop-based approach with a single shared middleware instance is efficient and maintainable. New file upload routes can now be added by simply updating the
fileUploadRoutesarrays in the respective config files.The middleware is correctly registered on POST routes before the router, ensuring multer processes the upload before page handlers.
libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)
21-32: Type safety confirmed. The middleware correctly wraps the multer upload, attaching errors toreq.fileUploadError(typed asMulterError | undefinedin the Express type augmentation atlibs/web-core/src/types/express.tsline 6) for downstream handling. The implementation enables page controllers to render user-friendly error messages without aborting the request flow, as evidenced by usage in manual-upload, non-strategic-upload, and reference-data-upload handlers.libs/admin-pages/src/manual-upload/validation.ts (3)
168-182: Nice refactoring with clear separation of concerns.The two public validation functions provide a clean API for different upload types while sharing the common validation logic. The parameter-driven approach (allowedExtensions, validateJson) makes the code maintainable and testable.
184-200: Verify the strict day/month length requirement.The validation requires exactly 2 characters for day and month (lines 190-192), which will reject inputs like "1" for day or "9" for month. This strictness is unusual—most date inputs accept single-digit values. Confirm this is the intended UX behavior and that the frontend pads single digits appropriately.
Based on learnings, permissive date validation is expected in this codebase, so this strict length check may be inconsistent with that pattern.
9-9: No action needed — errorMessages structures are compatible.Both locale files have identical
errorMessageskeys, making the union type on line 9 safe. While the error message values differ between manual-upload and non-strategic-upload, the TypeScript type definition will correctly allow access to all shared keys.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/app.test.ts (2)
20-23: Consider removing the unusedcreateFileUploadmock.Since
app.tsnow usescreateFileUploadMiddlewaredirectly, thecreateFileUploadmock (lines 20-22) appears to be dead code. The middleware mock on line 23 correctly simulates the middleware behavior.vi.mock("@hmcts/web-core", () => ({ configureCookieManager: vi.fn().mockResolvedValue(undefined), configureGovuk: vi.fn().mockResolvedValue(undefined), configureHelmet: vi.fn(() => vi.fn()), configureNonce: vi.fn(() => vi.fn()), - createFileUpload: vi.fn(() => ({ - single: vi.fn(() => vi.fn((_req: any, _res: any, next: any) => next())) - })), createFileUploadMiddleware: vi.fn(() => vi.fn((_req: any, _res: any, next: any) => next())), errorHandler: vi.fn(() => vi.fn()), expressSessionRedis: vi.fn(() => vi.fn()), notFoundHandler: vi.fn(() => vi.fn()) }));
207-210: Minor: Test name doesn't match assertion scope.The test name mentions "with error handling" but only verifies that
createFileUploadMiddlewarewas called. The actual error handling is tested in the separate "File Upload Error Handling" describe block. Consider renaming for clarity.- it("should configure file upload middleware with error handling", async () => { + it("should configure file upload middleware", async () => { const { createFileUploadMiddleware } = await import("@hmcts/web-core"); expect(createFileUploadMiddleware).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
apps/web/src/app.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:
apps/web/src/app.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:
apps/web/src/app.test.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 libs/**/*-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:
apps/web/src/app.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 libs/*/src/config.ts : Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate `config.ts` file to avoid circular dependencies during Prisma client generation. Apps import config using the `/config` path.
Applied to files:
apps/web/src/app.test.ts
🧬 Code graph analysis (1)
apps/web/src/app.test.ts (2)
libs/web-core/src/index.ts (1)
createFileUploadMiddleware(10-10)libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)
createFileUploadMiddleware(21-32)
⏰ 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)
apps/web/src/app.test.ts (2)
55-98: LGTM!The config module mocks are well-structured, follow consistent naming patterns (camelCase for properties), and align with the codebase convention of separate config exports. The
fileUploadRoutesarrays for admin-pages and system-admin-pages correctly provide the routes needed for the middleware-based upload registration.
213-239: LGTM!The error handling test correctly simulates the middleware-based error flow by setting
req.fileUploadErrorand callingnext(), which aligns with the actualcreateFileUploadMiddlewareimplementation. This properly tests that the application handles file upload errors gracefully without crashing.
There was a problem hiding this comment.
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)
apps/web/src/app.test.ts (1)
210-237: Incomplete test: Error handling behavior not verified.The test correctly simulates middleware-based error propagation by setting
req.fileUploadError, but it only verifies that the app is created—not that the error is actually handled. Consider adding assertions to verify that:
- The error is propagated to downstream middleware
- The
errorHandlerreceives and processes file upload errors appropriatelyExample enhancement:
const { createApp } = await import("./app.js"); const app = await createApp(); expect(app).toBeDefined(); + +// Verify error handler was configured to handle file upload errors +const { errorHandler } = await import("@hmcts/web-core"); +expect(errorHandler).toHaveBeenCalled();
🧹 Nitpick comments (1)
apps/web/src/app.test.ts (1)
204-207: LGTM, but consider verifying middleware integration.The test correctly verifies that
createFileUploadMiddlewareis called. Given the pipeline failures, you may want to add assertions that verify the middleware is properly wired to the file upload routes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
apps/web/src/app.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:
apps/web/src/app.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:
apps/web/src/app.test.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 libs/**/*-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:
apps/web/src/app.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 libs/*/src/config.ts : Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate `config.ts` file to avoid circular dependencies during Prisma client generation. Apps import config using the `/config` path.
Applied to files:
apps/web/src/app.test.ts
🧬 Code graph analysis (1)
apps/web/src/app.test.ts (2)
libs/web-core/src/index.ts (1)
createFileUploadMiddleware(10-10)libs/web-core/src/middleware/file-upload/file-upload-middleware.ts (1)
createFileUploadMiddleware(21-32)
🪛 GitHub Actions: Test
apps/web/src/app.test.ts
[error] 1-1: 17 tests failed in Web Application createApp suite (e.g., 'should create an Express application', 'should configure properties volume', etc.).
⏰ 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)
apps/web/src/app.test.ts (2)
15-24: LGTM! Middleware mock correctly implements the pattern.The mock for
createFileUploadMiddlewarecorrectly returns a middleware function that matches the implementation pattern shown in the relevant code snippets.
52-96: Verify that mock exports exactly match actual config module exports.The mocks appear structurally correct per the learning that configs must be in separate
config.tsfiles with exports likepageRoutes,moduleRoot, andfileUploadRoutes. However, ensure each mock matches its actual module export:list-types-commonmock lackspageRoutes, andlocationmock only includesapiRoutes. These differences may be correct if the modules genuinely don't export those fields, but confirm against the actual config files to resolve the 17 pipeline test failures.
- Added createFileUpload mock alongside createFileUploadMiddleware - Supports both master branch code and feature branch code during CI merge tests - Resolves CI failure when testing PR merge with master
- Replaced old createFileUpload pattern with createFileUploadMiddleware - Uses idProof field name instead of default file field - Consistent with other file upload routes refactoring - Reduces code duplication and simplifies file upload handling
- Added fileUploadRoutes export to @hmcts/public-pages/config - Consistent with @hmcts/admin-pages and @hmcts/system-admin-pages pattern - Removed hardcoded /create-media-account route from app.ts - Now uses module config array for better maintainability - Updated test mocks to include public pages file upload routes
|
3 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-162
Change description
Add non-strategic Excel upload pages
Summary by CodeRabbit
New Features
Validation
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.