VIBE-309 - Configure list type and get list information from database #181
Conversation
WalkthroughAdds a new database-backed list-type configuration module (Prisma models, migration, queries, service, validation), a session-backed multi-step System Admin UI, replaces mock list-type usage across admin flows, updates file-upload middleware wiring, and introduces unit and E2E tests plus seed/migration scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as System Admin
participant Web as Web App
participant Session as Session Store
participant DB as Database/Prisma
participant Validator as Validation
Admin->>Web: GET /configure-list-type/enter-details
Web->>Session: load session.configureListType
alt edit flow (id present)
Web->>DB: findListTypeById(id)
DB-->>Web: existing ListType
end
Web-->>Admin: render enter-details
Admin->>Web: POST /configure-list-type/enter-details
Web->>Validator: validateListTypeDetails(data)
Validator-->>Web: errors | ok
alt errors
Web->>Session: store errors + form data
Web-->>Admin: redirect to enter-details (with errors)
else valid
Web->>DB: findListTypeByName(name)
DB-->>Web: existing|null
alt name conflict
Web->>Session: store duplicate error
Web-->>Admin: redirect to enter-details
else unique
Web->>Session: store configureListType
Web-->>Admin: redirect to select-sub-jurisdictions
end
end
Admin->>Web: GET /configure-list-type/select-sub-jurisdictions
Web->>Session: load configureListType
Web->>DB: findAllSubJurisdictions()
DB-->>Web: subJurisdictions[]
Web-->>Admin: render checkboxes
Admin->>Web: POST /configure-list-type/select-sub-jurisdictions
Web->>Validator: validateSubJurisdictions(selection)
Validator-->>Web: errors | ok
alt errors
Web->>Session: store errors
Web-->>Admin: redirect to select-sub-jurisdictions
else ok
Web->>Session: update subJurisdictionIds
Web-->>Admin: redirect to preview
end
Admin->>Web: GET /configure-list-type/preview
Web->>Session: load configureListType
Web->>DB: findAllSubJurisdictions()
Web-->>Admin: render preview
Admin->>Web: POST /configure-list-type/preview
Web->>DB: saveListType (create or update)
DB-->>Web: success | error
alt success
Web->>Session: clear configureListType
Web-->>Admin: redirect to success
else error
Web-->>Admin: render preview with error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright E2E Test Results181 tests 181 ✅ 15m 23s ⏱️ Results for commit 71194e0. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
apps/web/package.json (1)
38-38: Express version update duplicates concern from apps/api/package.json.This change is part of the same Express version bump. See the verification comment on
apps/api/package.jsonline 24.
🧹 Nitpick comments (54)
libs/api/src/middleware/oauth-middleware.ts (1)
44-50: Good practice to signal unused variable, but consider logging error details.The rename to
_errorcorrectly signals the variable is intentionally unused. However, discarding the actual error details reduces observability and makes debugging authentication failures more difficult.Consider logging the error while still returning a generic user-facing message:
- } catch (_error) { - console.error("API authentication error"); + } catch (error) { + console.error("API authentication error:", error); return res.status(401).json({ success: false, message: "Invalid or expired token" }); }This preserves security (generic error messages to clients) while improving observability for debugging.
libs/list-types/common/src/list-type-validator.ts (1)
7-11: Defensive handling for nullablefriendlyNamein user-facing messages
ListTypeInfo.friendlyNameis typed asstring | null, but the error messages interpolatelistType.friendlyNamedirectly, which could surface"null"to users if the DB value is missing. Consider introducing a localdisplayNamethat falls back toname:export async function validateListTypeJson(listTypeId: string, jsonData: unknown, listTypes: ListTypeInfo[]): Promise<ValidationResult> { // Find the list type by ID const listTypeIdNum = Number.parseInt(listTypeId, 10); const listType = listTypes.find((lt) => lt.id === listTypeIdNum); if (!listType) { return { isValid: false, errors: [{ message: "Invalid list type ID" }], schemaVersion: "unknown" }; } + const displayName = listType.friendlyName ?? listType.name; @@ if (!validationFunctionName) { return { isValid: false, errors: [ { - message: `No validation function found for ${listType.friendlyName}. JSON schemas are not available for this list type.` + message: `No validation function found for ${displayName}. JSON schemas are not available for this list type.` } ], schemaVersion: "unknown" }; @@ if (errorMessage.includes("Cannot find") || errorMessage.includes("Failed to resolve") || errorMessage.includes("Cannot find package")) { return { isValid: false, errors: [ { - message: `No JSON schema available for ${listType.friendlyName}. This list type does not support JSON uploads.` + message: `No JSON schema available for ${displayName}. This list type does not support JSON uploads.` } ], schemaVersion: "unknown" };This keeps existing behavior where
friendlyNameis present but avoids awkward messages when it’s null.Also applies to: 60-61, 81-82
docs/tickets/VIBE-166/tasks.md (1)
28-34: Doc still recommends a generictypes.ts, which conflicts with current guidelinesThe tasks mention creating
src/models/types.ts, but the coding guidelines now say to avoid generictypes.tsfiles and to colocate types or give them specific names. Consider updating this doc (and the module, if still usingtypes.ts) to something more specific likehearing-types.tsor colocated model files instead.libs/list-type-config/src/list-type/list-type-queries.test.ts (1)
1-242: Strong Prisma query test coverage; optional tightening forupdateListTypetransactionThese tests do a good job of:
- Mocking
@hmcts/postgres’sprismaclient in an ESM-friendly way.- Asserting the exact Prisma options for all read/create/query helpers.
- Exercising the transactional
updateListTypepath end-to-end.If you want to tighten things further, you could also assert the behavior inside the
$transactioncallback (e.g. thatdeleteManyis called withwhere: { listTypeId: 1 }andupdatewith the expecteddatapayload). One option is to place those expectations inside themockPrisma.$transaction.mockImplementationbody, after awaitingcallback(tx). Not required, but it would lock the transaction contract down a bit more.libs/list-types/care-standards-tribunal-weekly-hearing-list/src/config.test.ts (1)
1-87: Avoid POSIX-only path assumption when asserting trailing separator onassetsThe structural checks on
moduleRoot,pageRoutes, andassetsare solid and will catch misconfigured config early. One small portability concern:expect(assets).toMatch(/\/$/);assumes a
/path separator, which can make this test fail on Windows. To keep it cross‑platform, consider something like:const sep = path.sep; expect(assets.endsWith(sep)).toBe(true);or normalize
assetsfirst and assert on its relation tomoduleRootrather than the literal trailing character.docs/tickets/VIBE-309/review.md (1)
1-342: Review doc is thorough; markdown/style lint findings are optional polishThis review document is very detailed and aligned with the implemented changes. The static-analysis hints are mostly stylistic (hyphenation like “production-ready” and MD036 about using emphasis where a heading could be used, e.g.
**APPROVED**). If you want a completely clean markdownlint/language-tool run, you could:
- Turn emphasized “headings” such as
**APPROVED**into actual##/###headings.- Apply suggested hyphenation like “high‑quality standards” or “production‑ready”.
These are purely cosmetic and don’t affect correctness; feel free to treat them as low-priority.
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/care-standards-tribunal-weekly-hearing-list.njk (2)
2-2: Unused macro import.
govukTableis imported but never used—the table is built manually. Remove the unused import or refactor to use the macro for consistency.-{% from "govuk/components/table/macro.njk" import govukTable %}
40-43: Redundant accessibility attributes on search input.The input has both a
<label>element (line 40-42) and anaria-labelattribute (line 43). The associated<label>is sufficient for accessibility; the redundantaria-labelcan be removed.- <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text" aria-label="{{ t.searchCasesLabel }}"> + <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text">libs/list-type-config/src/scripts/migrate-mock-data.ts (2)
3-11: Remove unusedidfield from interface.The
idproperty inMockListTypeis never referenced in the migration logic. Remove it to avoid confusion.interface MockListType { - id: number; name: string; englishFriendlyName: string; welshFriendlyName: string; provenance: string; urlPath?: string; isNonStrategic: boolean; }Then remove
idfrom each object inmockListTypes.
107-140: Consider wrapping migration in a transaction.If the migration fails partway through (e.g., on item 5 of 9), the database will be left in an inconsistent state with some list types created and others missing. Wrapping the loop in
prisma.$transactionwould allow atomic rollback on failure.+ await prisma.$transaction(async (tx) => { for (const mockListType of mockListTypes) { - const existingListType = await prisma.listType.findUnique({ + const existingListType = await tx.listType.findUnique({ where: { name: mockListType.name } }); // ... rest of logic using tx instead of prisma } + });apps/postgres/prisma/migrations/20251209141600_add_list_types/migration.sql (1)
36-37: Consider adding index onsub_jurisdiction_idfor reverse lookups.If you query "which list types belong to a sub-jurisdiction?", the composite unique index won't efficiently support that lookup. An additional index on
sub_jurisdiction_idalone would improve performance for such queries.-- Optional: Add after the existing indexes CREATE INDEX "list_types_sub_jurisdictions_sub_jurisdiction_id_idx" ON "list_types_sub_jurisdictions"("sub_jurisdiction_id");docs/tickets/VIBE-166/ticket.md (1)
243-275: Add language identifiers to fenced code blocks.The wireframe code blocks lack language specifiers. While these are ASCII diagrams, adding
textorplaintextas the language identifier satisfies markdown linting rules.-``` +```text [ CaTH Header / Navigation ]Also applies to: 279-294, 300-335
libs/admin-pages/src/manual-upload/validation.test.ts (1)
634-646: DuplicatecreateMockFilehelper function.This helper is defined identically at lines 144-156 and again at lines 634-646, differing only in default
originalnameandmimetype. Extract to a shared helper at the top of the file to reduce duplication.+const createMockFile = (overrides?: Partial<Express.Multer.File>): Express.Multer.File => ({ + fieldname: "file", + originalname: "test.pdf", + encoding: "7bit", + mimetype: "application/pdf", + size: 1024, + buffer: Buffer.from("test"), + stream: {} as any, + destination: "", + filename: "", + path: "", + ...overrides +}); + describe("validateManualUploadForm", () => { - const createMockFile = (overrides?: Partial<Express.Multer.File>): Express.Multer.File => ({ - // ... - });Then use
createMockFile({ originalname: "test.xlsx", mimetype: "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" })in the non-strategic tests.docs/tickets/VIBE-309/plan.md (1)
46-46: Add language identifier to session structure code block.The TypeScript interface code block on line 74 has proper language, but line 46 lacks a language specifier in the module structure block.
Line 13 should specify a language:
-``` +```text libs/list-type-config/libs/list-types/common/src/conversion/excel-to-json.ts (1)
43-43: Unnecessaryasynckeyword.
convertExcelToJsonis declaredasyncbut contains noawaitexpressions. The function can be synchronous unless you anticipate adding async operations later.-export async function convertExcelToJson<T = Record<string, string>>(buffer: Buffer, config: ExcelConverterConfig): Promise<T[]> { +export function convertExcelToJson<T = Record<string, string>>(buffer: Buffer, config: ExcelConverterConfig): T[] {Note: This would require updating callers to remove
await.libs/list-type-config/src/pages/configure-list-type/success.ts (1)
6-17: Consider consistent locale detection pattern.Line 7 uses
req.query.lngfor language detection, which differs from other pages that useres.locals.locale. Consider using a consistent pattern across the application for locale detection.Additionally, the
asynckeyword on line 6 is unnecessary since the handler doesn't useawait.Apply this diff for consistency:
-const getHandler = async (req: Request, res: Response) => { - const language = req.query.lng === "cy" ? "cy" : "en"; +const getHandler = (req: Request, res: Response) => { + const language = res.locals?.locale === "cy" ? "cy" : "en"; const content = language === "cy" ? cy : en;libs/admin-pages/src/pages/non-strategic-upload-success/index.ts (1)
6-36: Session gating and locale handling look correct; only tiny optional tidy‑upsThe success page guard using
nonStrategicUploadConfirmedplus thenonStrategicSuccessPageViewed/nonStrategicViewedLanguageflags gives a clear, single‑view flow and still allows a one‑off language switch, which matches the comment on Line 20. The role protection and translation selection (en/cy) are also wired cleanly.If you want to tighten things further (optional):
- Narrow the
localetype andgetTranslationssignature to"en" | "cy"so TypeScript enforces valid values end‑to‑end.- You could derive
hasLangParamasObject.hasOwn(req.query, "lng")for slightly clearer intent, but current check is fine.libs/admin-pages/src/pages/non-strategic-upload/cy.ts (1)
1-60: Locale keys are complete, but text is still placeholderThe structure and key set look correct for the upload page, including
errorMessages, but the values are clearly placeholder ("[Welsh] ...") rather than real translations. Before exposing this to users, these should be replaced with proper Welsh copy (and ideally tracked in a separate translation ticket if not in scope here).libs/admin-pages/src/pages/non-strategic-upload-summary/cy.ts (1)
1-14: Locale keys align with template; ensure import style matches and replace placeholders laterThe default‑exported locale object has all the keys the summary template expects, so wiring is sound. Just make sure
index.tsimports this as a default export (e.g.import cy from "./cy.js";) to avoid ESM export mismatches, and plan to replace the[Welsh] ...placeholder strings with real Welsh copy before go‑live.libs/list-type-config/src/config.ts (1)
1-8: Config wiring is correct; consider usingmoduleRootinpageRoutes
moduleRootandpageRoutesare computed correctly for ESM, and this should plug into the app’s dynamic route loader without issues.You can avoid repeating
__dirnameby buildingpageRoutesfrommoduleRoot:-export const moduleRoot = __dirname; -export const pageRoutes = { path: path.join(__dirname, "pages") }; +export const moduleRoot = __dirname; +export const pageRoutes = { path: path.join(moduleRoot, "pages") };Purely cosmetic, but keeps the config single‑sourced.
libs/list-type-config/src/list-type/list-type-service.test.ts (1)
1-107: Good coverage ofsaveListTypebranches; optionally tighten one update testThe tests exercise all key paths of
saveListType(new, duplicate, update with same id, conflicting update, and non-conflicting update) and correctly assert whethercreateListTypevsupdateListTypeis called.If you want to make the “should allow updating when name matches own id” case a bit stricter, you could also assert interactions there (mirroring Line 67–70), e.g.:
expect(mockFindListTypeByName).toHaveBeenCalledWith("TEST_LIST"); expect(mockUpdateListType).toHaveBeenCalledWith(1, validData); expect(mockCreateListType).not.toHaveBeenCalled();Not required, but it would guard against regressions where the happy-path result shape stays the same but the wrong persistence function is called.
libs/api/src/blob-ingestion/validation.ts (1)
50-61: Consider deferring database fetch for better performance.The
findAllListTypes()call happens early in the validation flow, even before basic field validations (size, required fields). If the request fails early validations, the database query is unnecessary.Consider moving the database fetch after basic field validations:
export async function validateBlobRequest(request: BlobIngestionRequest, rawBodySize: number): Promise<BlobValidationResult> { const errors: ValidationError[] = []; // Size validation if (rawBodySize > MAX_BLOB_SIZE) { errors.push({ field: "body", message: `Payload too large. Maximum size is ${MAX_BLOB_SIZE / 1024 / 1024}MB` }); } // Required fields if (!request.court_id) { errors.push({ field: "court_id", message: "court_id is required" }); } if (!request.provenance) { errors.push({ field: "provenance", message: "provenance is required" }); } else if (!ALLOWED_PROVENANCES.includes(request.provenance)) { errors.push({ field: "provenance", message: `Invalid provenance. Allowed values: ${ALLOWED_PROVENANCES.join(", ")}` }); } // ... other basic validations + // Return early if basic validations failed + if (errors.length > 0) { + return { + isValid: false, + errors, + locationExists: false, + listTypeId: undefined + }; + } + + // Fetch list types only after basic validations pass - const listTypes = await findAllListTypes(); + const listTypes = await findAllListTypes();This optimization reduces unnecessary database calls when requests fail basic validation.
libs/admin-pages/src/pages/remove-list-search-results/index.ts (1)
66-86: Consider extracting shared artefact row building logic to reduce duplication.The logic for fetching list types (lines 67-68) and mapping artefacts to rows (lines 70-86) is duplicated in the POST handler's error path (lines 127-147). This violates the DRY principle.
Consider extracting a helper function:
+async function buildArtefactRows( + artefacts: Awaited<ReturnType<typeof getArtefactsByLocation>>, + courtName: string, + locale: string +): Promise<ArtefactRow[]> { + const listTypes = await findAllListTypes(); + const listTypeMap = new Map(listTypes.map((lt) => [lt.id, lt])); + + return artefacts.map((artefact) => { + const listType = listTypeMap.get(artefact.listTypeId); + const listTypeName = listType + ? (locale === "cy" ? listType.welshFriendlyName || listType.friendlyName : listType.friendlyName) || String(artefact.listTypeId) + : String(artefact.listTypeId); + + return { + artefactId: artefact.artefactId, + listType: listTypeName, + courtName, + contentDate: formatDateString(artefact.contentDate), + contentDateRaw: artefact.contentDate, + displayDates: formatDateRangeString(artefact.displayFrom, artefact.displayTo), + language: LANGUAGE_LABELS[artefact.language] || artefact.language, + sensitivity: SENSITIVITY_LABELS[artefact.sensitivity] || artefact.sensitivity + }; + }); +}Then use it in both handlers:
let artefactRows = await buildArtefactRows(artefacts, courtName, locale); artefactRows = sortArtefacts(artefactRows, sortBy, order);libs/list-type-config/src/list-type/list-type-service.ts (1)
37-47: Consider exportingSaveListTypeDataif consumers need it.The interface is well-structured. If external modules (e.g., controllers calling
saveListType) need to construct this data type, consider exporting it. Otherwise, keeping it internal is appropriate.libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts (2)
10-11: Remove duplicate side-effect import.Line 10 imports
model.jsas a side-effect, but line 11 already imports named exports from the same module. The side-effect import appears unnecessary.import { saveUploadedFile } from "../../manual-upload/file-storage.js"; -import "../../manual-upload/model.js"; import { LANGUAGE_LABELS, SENSITIVITY_LABELS } from "../../manual-upload/model.js";
141-143: Consider structured logging overconsole.error.For production observability, structured logging with appropriate log levels and context would be more maintainable than
console.error.libs/list-type-config/src/pages/configure-list-type/enter-details.njk (1)
83-88: Hardcoded sensitivity option texts should use localization.The sensitivity options have hardcoded English text ("Select an option", "Public", "Private", "Classified"). For Welsh support, these should reference localization keys like other labels in this form.
items: [ - { value: "", text: "Select an option", selected: not data.defaultSensitivity }, - { value: "Public", text: "Public", selected: data.defaultSensitivity == "Public" }, - { value: "Private", text: "Private", selected: data.defaultSensitivity == "Private" }, - { value: "Classified", text: "Classified", selected: data.defaultSensitivity == "Classified" } + { value: "", text: configureListType.enterDetails.selectOption, selected: not data.defaultSensitivity }, + { value: "Public", text: configureListType.enterDetails.sensitivityPublic, selected: data.defaultSensitivity == "Public" }, + { value: "Private", text: configureListType.enterDetails.sensitivityPrivate, selected: data.defaultSensitivity == "Private" }, + { value: "Classified", text: configureListType.enterDetails.sensitivityClassified, selected: data.defaultSensitivity == "Classified" } ],libs/list-type-config/src/pages/configure-list-type/enter-details.ts (1)
9-24: Move interfaces to the bottom of the file.Per coding guidelines, module ordering should be: constants at the top, exported functions next, other functions ordered by usage, and interfaces/types at the bottom.
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/index.ts (1)
16-17: Fragile path calculation using relative traversals.The monorepo root calculation using multiple
..segments is brittle and will break if the file is moved. Consider using a centralized configuration or environment variable for storage paths.-const MONOREPO_ROOT = path.join(__dirname, "..", "..", "..", "..", ".."); -const TEMP_UPLOAD_DIR = path.join(MONOREPO_ROOT, "storage", "temp", "uploads"); +const TEMP_UPLOAD_DIR = process.env.TEMP_UPLOAD_DIR || path.join(__dirname, "..", "..", "..", "..", "..", "storage", "temp", "uploads");libs/list-type-config/package.json (1)
25-27: Peer dependency should not be pinned to an exact version.Per coding guidelines, peer dependencies should use a version range rather than exact pinning to allow flexibility for the consuming application.
"peerDependencies": { - "express": "5.1.0" + "express": "^5.1.0" }docs/tickets/VIBE-162/plan.md (1)
66-66: Add language specifier to fenced code block.The code block should have a language identifier for syntax highlighting.
-``` +```text libs/admin-pages/src/libs/list-types/care-standards-tribunal-weekly-hearing-list/src/validation/json-validator.ts (1)
13-14: Avoidanytype cast - verify Ajv import and types.The type cast
(Ajv as any)bypasses TypeScript's type checking. This typically indicates a mismatch between the import and the expected type.Verify the correct import pattern for Ajv v8.17.1:
How to correctly import and instantiate Ajv version 8.17.1 in TypeScript ES modules?libs/list-types/care-standards-tribunal-weekly-hearing-list/package.json (1)
17-17: Consider cross-platform compatibility for build scripts.The
build:nunjucksscript uses Unix-specific shell commands (sh -c,find,mkdir -p,cp) that may not work on Windows without WSL or Git Bash. Consider using a cross-platform solution likecpy-cliorcpxfor copying files.Example using
cpy-cli:"build:nunjucks": "cpy 'src/pages/**/*.njk' dist/pages --parents"This would require adding
cpy-clias a dev dependency.libs/list-type-config/src/pages/configure-list-type/preview.ts (2)
34-35: Consider querying only selected sub-jurisdictions for better performance.The code fetches all sub-jurisdictions and then filters in memory. If the number of sub-jurisdictions is large, this could be inefficient. Consider adding a query function that fetches only the selected sub-jurisdictions by their IDs.
Example approach:
const selectedSubJurisdictions = await findSubJurisdictionsByIds( session.configureListType.subJurisdictionIds );This would require adding a new query function to
list-type-queries.ts.
73-79: Extract duplicate sub-jurisdiction query logic.Lines 73-79 duplicate the query and formatting logic from the GET handler (lines 34-37). Consider extracting this into a helper function to follow the DRY principle.
Example:
async function getSubJurisdictionsText( subJurisdictionIds: number[], language: "cy" | "en" ): Promise<string> { const allSubJurisdictions = await findAllSubJurisdictions(); const selected = allSubJurisdictions.filter((sj) => subJurisdictionIds.includes(sj.subJurisdictionId) ); return selected.map((sj) => (language === "cy" ? sj.welshName : sj.name)).join(", "); }e2e-tests/tests/care-standards-tribunal-upload.spec.ts (3)
117-117: AvoidwaitForTimeoutfor test reliability.Using
waitForTimeout(1000)is fragile and slows down tests. Prefer waiting for specific conditions like element visibility or network idle.- await page.waitForTimeout(1000); + await page.waitForLoadState("networkidle");
283-345: Placeholder tests should be implemented or tracked.Multiple tests in this block contain only commented-out code. Per coding guidelines, E2E tests should include complete user journeys with validations.
Consider either:
- Implementing these tests now if the list display page is available
- Removing placeholder tests and creating a tracking issue for future implementation
- Using
test.skip()with a JIRA reference to document pending workWould you like me to open an issue to track these placeholder tests?
11-14: Non-null assertions on environment variables may cause runtime errors.If the environment variables are not set, the
!assertion will pass type checking butloginWithSSOwill receiveundefined, potentially causing cryptic failures.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!; + 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"); + } await loginWithSSO(page, systemAdminEmail, systemAdminPassword); }libs/admin-pages/src/pages/non-strategic-upload/index.ts (2)
35-35: Avoidanytype – use explicit typing.Per coding guidelines,
anyshould not be used without justification. Consider usingunknownwith type guards or a more specific type.-const hasValue = (val: any) => val !== undefined && val !== null && val !== "" && val.toString().trim() !== ""; +const hasValue = (val: unknown): val is string | number => + val !== undefined && val !== null && val !== "" && String(val).trim() !== "";
37-56: Replaceanywith explicit types for form body parsing.These functions use
anyfor the request body. Consider defining a form body interface.+interface NonStrategicUploadBody { + locationId?: string; + "court-display"?: string; + listType?: string; + "hearingStartDate-day"?: string; + "hearingStartDate-month"?: string; + "hearingStartDate-year"?: string; + sensitivity?: string; + language?: string; + "displayFrom-day"?: string; + "displayFrom-month"?: string; + "displayFrom-year"?: string; + "displayTo-day"?: string; + "displayTo-month"?: string; + "displayTo-year"?: string; +} + -function parseDateInput(body: any, prefix: string) { +function parseDateInput(body: NonStrategicUploadBody, prefix: string) {docs/tickets/VIBE-166/plan.md (1)
46-69: Add language specifier to code block.The file structure diagram should have a language specifier for better rendering and linting compliance.
-``` +```text libs/list-types/care-standards-tribunal-weekly-hearing-list/ ├── package.json ...libs/admin-pages/src/pages/manual-upload/index.ts (1)
3-23: Tighten typing around form data and non-null assertions in POSTThe dynamic list-type loading and new
UploadFormDatausage look good and align with tests, but the POST handler now relies onvalidateManualUploadFormto guarantee presence of several fields:
formData.locationId!,listType!,hearingStartDate!,sensitivity!,language!,displayFrom!,displayTo!.If validation is ever relaxed or reused differently, these non-null assertions could turn into runtime errors.
Two small hardening options:
- Narrow the type of
UploadFormDatafor this route (e.g. aValidatedUploadFormDatawith required fields) and havevalidateManualUploadFormreturn that, or- Add explicit guards (e.g. early 400/error path) before calling
storeManualUpload, instead of relying solely on!.This keeps the handler safer against future validator changes while preserving current behaviour.
Also applies to: 44-54, 97-107, 120-146
e2e-tests/tests/configure-list-type.spec.ts (1)
1-125: Extend edit-flow test to mirror accessibility/i18n coverage of create-flowThe “admin can configure list type” test gives a solid, end-to-end journey including validation, Welsh toggle, Axe checks on each step, and a small keyboard navigation check. The edit-flow test, however, currently only exercises the edit path without any Welsh or accessibility assertions.
For better regression coverage and consistency with your own E2E guidelines, consider either:
- Adding at least one Axe run and a quick Welsh toggle assertion into the edit test, or
- Extracting small helpers for “run Axe on current page” and “verify language toggle works”, then reusing them in both tests.
This keeps both create and edit paths aligned on WCAG/i18n expectations.
libs/admin-pages/src/pages/manual-upload/index.test.ts (1)
55-84: Tests correctly exercise new dynamic list-type and validator wiringThe added
@hmcts/list-type-configmock and the rename tovalidateManualUploadFormare wired cleanly:
- GET tests now verify that
listTypesis populated and selection behaves as expected with ID-based values.- POST tests mock
validateManualUploadFormand assert both the happy path (upload stored, redirect withuploadId) and error paths (session errors and form data persistence).This provides good coverage for the new dynamic configuration; no issues from a behaviour perspective. If you end up adding more pages that rely on strategic list types, consider centralising the shared mock data to reduce duplication, but that’s optional.
Also applies to: 86-88, 95-96, 574-575, 579-580, 632-633, 683-684, 687-688, 724-725, 770-771, 814-815, 854-855, 892-893
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.ts (1)
9-47: Sub-jurisdiction selection flow is sound; consider extracting common item builderThe GET and POST handlers correctly:
- Guard on
session.configureListTypeand redirect if missing.- Respect
lng=cy/enfor label language.- Map
findAllSubJurisdictions()to checkbox items, preserving selected IDs.- On error, rebuild the items with user selections and surface
errorsanderrorListconsistent withvalidateSubJurisdictions.The only minor duplication is the
itemsconstruction logic (lines 37–41 and 70–74). Extracting a small helper likebuildSubJurisdictionItems(subJurisdictions, selectedIds, language)would reduce repetition and make future tweaks (e.g. extra fields or ordering changes) less error-prone.Also applies to: 49-92
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/rendering/renderer.ts (1)
4-28: Localise header strings for Welsh and consider moving types to bottomThe rendering logic (date parsing, Europe/London timezone, mapping hearings) looks correct, but when
locale === "cy"only the date portions are localised viatoLocaleDateString/ Luxon:
listTitleis always"Care Standards Tribunal Weekly Hearing List".durationalways starts with"List for week commencing".lastUpdatedalways starts with"Last updated"and uses"at"/am/pm.To align with the project’s bilingual approach, consider moving these user-facing phrases into per-locale resources (e.g. small
en/cymaps forlistTitle,durationPrefix,lastUpdatedPrefix) and building the strings from those, while keeping the date/time formatting logic here.Also, to match your module-ordering guideline, you might move the
RenderOptions/RenderedHearing/RenderedDatainterfaces to the bottom of the file, after the exported function and helpers.Also applies to: 30-65, 67-88
libs/list-types/common/src/conversion/non-strategic-list-registry.ts (1)
4-40: Converter registry is clean; consider adding generics for typed resultsThe registry design is simple and effective:
registerConverter/getConverterForListType/hasConverterForListTypewrap a module-localMap.convertExcelForListTypefails fast with a clear error when no converter is registered.createConverterneatly bindsExcelConverterConfigto the sharedconvertExcelToJson.Two possible improvements if you want stronger typing:
- Make
ListTypeConvertergeneric, e.g.ListTypeConverter<T = unknown>, and thread that throughconvertExcelForListType<T>so callers can getPromise<T[]>instead ofunknown[].- (Optional) decide whether re-registering the same
listTypeIdshould be allowed or should log/throw, to catch accidental double registration during startup.Neither is blocking, but they’d make the API safer and more self-documenting.
e2e-tests/tests/non-strategic-upload.spec.ts (2)
17-18: Non-null assertions on environment variables may cause runtime errors.Using
!(non-null assertion) on environment variables that might be undefined can cause cryptic runtime failures. Consider adding explicit validation or fallback handling.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!; + 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 environment variables must be set"); + } await loginWithSSO(page, systemAdminEmail, systemAdminPassword); } }
27-27: Replace fixed timeouts with explicit waits for more reliable tests.
waitForTimeout(1000)can cause flaky tests, especially in CI environments with variable load. Consider waiting for specific UI states or network idle instead.await authenticateSystemAdmin(page); await page.goto("/non-strategic-upload?locationId=9001"); - await page.waitForTimeout(1000); + await page.waitForLoadState("networkidle");This pattern appears in multiple places (lines 27, 69, 143, 331, 459, 692, 728). Consider replacing all occurrences for more reliable tests.
libs/location/prisma/schema.prisma (2)
52-53: Table mapping uses plural form instead of singular.As per coding guidelines, database tables MUST be singular. The table is mapped to
list_types_sub_jurisdictionsbut should belist_type_sub_jurisdiction.@@unique([listTypeId, subJurisdictionId]) - @@map("list_types_sub_jurisdictions") + @@map("list_type_sub_jurisdiction")Note: This change requires a corresponding migration update.
71-71: Table mapping uses plural form instead of singular.As per coding guidelines, database tables MUST be singular. The table is mapped to
list_typesbut should belist_type.- @@map("list_types") + @@map("list_type")Note: This change requires a corresponding migration update.
libs/list-type-config/src/list-type/list-type-queries.ts (1)
88-110: Duplicate interface definitions can be consolidated.
CreateListTypeDataandUpdateListTypeDataare identical. Consider using a single shared type or extending from a common base to reduce duplication.-interface CreateListTypeData { - name: string; - friendlyName: string; - welshFriendlyName: string; - shortenedFriendlyName: string; - url: string; - defaultSensitivity: string; - allowedProvenance: string[]; - isNonStrategic: boolean; - subJurisdictionIds: number[]; -} - -interface UpdateListTypeData { +interface ListTypeData { name: string; friendlyName: string; welshFriendlyName: string; shortenedFriendlyName: string; url: string; defaultSensitivity: string; allowedProvenance: string[]; isNonStrategic: boolean; subJurisdictionIds: number[]; } + +type CreateListTypeData = ListTypeData; +type UpdateListTypeData = ListTypeData;libs/admin-pages/src/manual-upload/validation.ts (1)
58-63: Consider logging the parsing error for debugging.The catch block returns a generic error message but discards the actual parsing error. Adding logging would help diagnose malformed JSON files during development and debugging.
- } catch { + } catch (error) { + console.error("JSON parsing error:", error); return { text: "Invalid JSON file format. Please ensure the file contains valid JSON.", href: "#file" }; }docs/tickets/VIBE-309/implementation-summary.md (1)
89-99: Database migration execution is blocked; no contingency or validation strategy documented.The critical migration command (
yarn db:migrate:dev) and mock data seeding script are marked "Ready but not executed" due to lack of running database. While the blocker is acknowledged, the summary lacks:
- Rollback/cleanup strategy if migration fails
- Validation checklist before mark as complete (e.g., schema applied, indices created, constraints enforced)
- Error handling details in
migrate-mock-data.tsfor duplicate prevention and transaction safetyEnsure these are documented or added before database is available for testing.
Would you like me to review the migration script and suggest error-handling and validation improvements?
| "config": "4.1.1", | ||
| "cors": "2.8.5", | ||
| "express": "5.1.0" | ||
| "express": "5.2.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Express 5.2.0 on npm registry
echo "=== Checking Express 5.2.0 on npm registry ==="
curl -s https://registry.npmjs.org/express/5.2.0 | jq -r '.version, .time.created, .time.modified' 2>/dev/null || echo "Failed to fetch from npm"
echo -e "\n=== Searching for all Express version references in package.json files ==="
find . -name "package.json" -type f | while read file; do
version=$(grep -o '"express":\s*"[^"]*"' "$file" 2>/dev/null)
if [ -n "$version" ]; then
echo "$file: $version"
fi
done
echo -e "\n=== Checking for Express in apps/api/package.json ==="
if [ -f "apps/api/package.json" ]; then
grep -A2 -B2 '"express"' apps/api/package.json || echo "Express not found"
fi
echo -e "\n=== Checking for Express in libs/list-type-config/package.json ==="
if [ -f "libs/list-type-config/package.json" ]; then
grep -A2 -B2 '"express"' libs/list-type-config/package.json || echo "Express not found"
else
echo "libs/list-type-config/package.json not found"
fiRepository: hmcts/cath-service
Length of output: 1340
Express 5.2.0 exists but version inconsistency detected across the monorepo.
Express 5.2.0 is confirmed to exist on the npm registry. However, this change introduces inconsistency: apps/api/package.json and apps/web/package.json are pinned to 5.2.0, while most packages in libs/ still reference 5.1.0 (many using the caret ^5.1.0, which violates the coding guideline requiring all dependency versions to be pinned to specific versions). Either update all packages to 5.2.0 consistently, or revert this change and coordinate a monorepo-wide version update. Additionally, ensure that unpinned versions (caret/tilde) are replaced with exact pinned versions as required by the guidelines.
🤖 Prompt for AI Agents
In apps/api/package.json around line 24, the PR introduces express "5.2.0" while
other packages in the monorepo still reference 5.1.0 and many use caret ranges;
fix by making dependency versions consistent and pinned across the repo: choose
to either revert apps/api to 5.1.0 OR update all package.json files (apps/* and
libs/*) to express "5.2.0" and remove any caret/tilde so each file uses the
exact version; after updating package.json files, regenerate the lockfile
(npm/yarn/pnpm install) and run the test suite/linters to ensure no regressions.
| The non-strategic publishing route requires the upload of an excel file in CaTH which is then transformed at the back end to a Json file before publishing. The upload process is completed over a number of steps. | ||
|
|
||
| **AS A** Local Admin | ||
| **I WANT** to upload an excel file in CaTH | ||
| **SO THAT** I can publish a hearing list in CaTH | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| 1. A local admin is able to access a verified account by signing in through the sign in page with their approved log in details | ||
| 2. The local admin is able to see the 'Upload excel file' tab on the dashboard and is taken to the excel file upload form when the tab is clicked | ||
| 3. The 'Upload excel file' tab displays the descriptive message 'Upload an excel file to be converted and displayed on the external facing service on GOV.UK.' |
There was a problem hiding this comment.
Minor: Capitalize "Excel" consistently throughout the document.
The static analysis correctly flags that "excel" should be capitalized to "Excel" when referring to the Microsoft Excel program. This appears multiple times (lines 7, 10, 16, 17, etc.).
🧰 Tools
🪛 LanguageTool
[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)
🤖 Prompt for AI Agents
In docs/tickets/VIBE-162/ticket.md around lines 7 to 17, the word "excel" is not
capitalized consistently; update every occurrence referring to Microsoft Excel
to "Excel" (e.g., lines 7, 10, 16, 17 and any other instances in the file)
ensuring consistent capitalization throughout the document and run a quick
search/replace to catch all occurrences.
| ### 9. Unit Tests ✅ | ||
| - Created comprehensive test suite: `list-type-validation.test.ts` | ||
| - **12 tests passing** covering: | ||
| - Valid input scenarios | ||
| - Empty field validation | ||
| - Length validation (1000 chars for name, 255 for Welsh names) | ||
| - Invalid sensitivity options | ||
| - Empty/invalid provenance | ||
| - Missing isNonStrategic selection | ||
| - Multiple validation errors | ||
| - Sub-jurisdiction validation | ||
|
|
There was a problem hiding this comment.
Significant testing gaps: Unit tests only; E2E and integration tests not started.
Only validation layer unit tests (12 tests) are passing. The multi-step form workflow—critical for user experience—lacks end-to-end coverage. Missing test scenarios include:
- Complete create/edit journeys with actual database persistence
- Session state management across pages
- Welsh language toggle with data preservation
- Keyboard navigation and accessibility compliance
Consider prioritizing E2E tests (listed at lines 130–136) before merging, particularly for the create/edit/preview flows to prevent regressions.
🤖 Prompt for AI Agents
docs/tickets/VIBE-309/implementation-summary.md lines 77-88: the test coverage
currently only contains 12 unit tests for validation but lacks E2E/integration
tests for critical multi-step form flows; add prioritized E2E/integration tests
that exercise full create/edit/preview journeys with real DB persistence (use
test DB or fixtures and teardown), verify session state is preserved across
pages, assert Welsh language toggle preserves data across steps, and include
keyboard-navigation and basic accessibility assertions; wire these E2E tests
into CI (use the existing E2E framework like Cypress/Playwright), seed necessary
test data, and block merging until these scenarios are green.
| editId: session.configureListType?.editId | ||
| }; | ||
|
|
||
| res.redirect("/configure-list-type/select-sub-jurisdictions"); |
There was a problem hiding this comment.
Redirect does not preserve language query parameter.
When a user is navigating in Welsh (?lng=cy), the redirect loses this preference. Consider preserving the language parameter.
-res.redirect("/configure-list-type/select-sub-jurisdictions");
+const langQuery = req.query.lng === "cy" ? "?lng=cy" : "";
+res.redirect(`/configure-list-type/select-sub-jurisdictions${langQuery}`);🤖 Prompt for AI Agents
In libs/list-type-config/src/pages/configure-list-type/enter-details.ts around
line 134, the redirect to "/configure-list-type/select-sub-jurisdictions" fails
to preserve the language query parameter (e.g. ?lng=cy); update the redirect to
include the current request's language query parameter by reading req.query.lng
(or req.locale/appropriate source) and appending it to the redirect URL
(ensuring proper encoding and handling if other query params exist), e.g. build
the target URL with the existing queries merged and then call
res.redirect(targetUrl).
| "@hmcts/web-core": "workspace:*", | ||
| "ajv": "8.17.1", | ||
| "luxon": "3.7.2", | ||
| "xlsx": "0.18.5" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version of xlsx npm package
💡 Result:
The current SheetJS "xlsx" release is 0.20.3 (distributed as a tarball via the SheetJS CDN — e.g. https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz). [1][2]
Notes: the npm registry's historical "xlsx" package has been effectively discontinued by SheetJS; installing the official CE release is done via the CDN tarball or trusted mirrors. [1][3]
Sources:
[1] SheetJS docs (NodeJS installation, shows xlsx-0.20.3).
[2] SheetJS CDN (release listing).
[3] Stack Overflow discussion / guidance about using the CDN tarball for xlsx.
🌐 Web query:
xlsx version 0.18.5 vulnerabilities security CVE
💡 Result:
Summary — [email protected] (SheetJS Community Edition)
- Known security issues affecting 0.18.5: prototype-pollution (CVE-2023-30533 / BDSA-2023-0967) and multiple ReDoS/DoS issues reported by security scanners. 0.18.5 is listed as vulnerable. [1][2][3][4]
Details & fixes
- CVE-2023-30533 (Prototype Pollution): fixed in SheetJS upstream code in v0.19.3 (and later), but the version published to the public npm registry remained at 0.18.5 for some time — many users of npm [email protected] remained vulnerable until patched versions were published by SheetJS. See the SheetJS issue threads. [3][5]
- Regular-expression DoS / ReDoS and other DoS findings: reported for versions including 0.18.5; fixes were merged in later SheetJS releases (references point to fixes in master and v0.20.x). [4][6]
What to do
- If you parse untrusted files with [email protected]: treat it as vulnerable — upgrade to a patched SheetJS release (>= v0.19.3 / preferably the latest v0.20.x+ published by the SheetJS authoritative source) or obtain the patched distribution from the SheetJS authoritative distribution as referenced in advisories. Verify the specific patched version you use addresses CVE-2023-30533 and the ReDoS fixes. [3][4][6]
- If you only generate/export spreadsheets and never read arbitrary files, the prototype-pollution risk is reduced but ReDoS/other issues should still be considered. [4]
Sources
- ReversingLabs / package vulnerability summary for [email protected]. [turn0search0]
- Snyk package advisory page for xlsx (general). [turn0search1]
- SheetJS Git issue: reporting CVE-2023-30533 and noting fix in v0.19.3. [turn0search2][turn0search3]
- Snyk per-version vulnerability page for [email protected] (Prototype Pollution, ReDoS/DoS). [turn0search4][turn0search5]
- SheetJS issue discussion about publishing patched versions to npm. [turn0search3]
- SheetJS issue threads mentioning ReDoS fixes and subsequent releases. [turn0search7]
If you want, I can:
- check the currently published xlsx version on npm and confirm whether patched versions are now available (I can do that now).
Upgrade xlsx to a patched version to fix critical security vulnerabilities.
Version 0.18.5 contains known vulnerabilities including CVE-2023-30533 (prototype pollution) and ReDoS/DoS issues. Upgrade to version 0.20.3 (the latest official SheetJS release) or at minimum 0.19.3.
🤖 Prompt for AI Agents
In libs/list-types/care-standards-tribunal-weekly-hearing-list/package.json
around line 33, the dependency "xlsx": "0.18.5" is vulnerable; update it to a
patched release (prefer "0.20.3", or at minimum "0.19.3") by changing the
version string in package.json, run your package manager to update the lockfile
(npm install or yarn install), run the test suite and any build steps to ensure
compatibility, and run npm audit/fix (or yarn audit) and commit the updated
package.json and lockfile.
| export const cy = { | ||
| pageTitle: "Welsh placeholder", | ||
| importantInformationTitle: "Welsh placeholder", | ||
| importantInformationText: "Welsh placeholder", | ||
| importantInformationLinkText: "Welsh placeholder", | ||
| importantInformationLinkUrl: "https://www.gov.uk/guidance/observe-a-court-or-tribunal-hearing", | ||
| searchCasesTitle: "Welsh placeholder", | ||
| searchCasesLabel: "Welsh placeholder", | ||
| tableHeaders: { | ||
| date: "Welsh placeholder", | ||
| caseName: "Welsh placeholder", | ||
| hearingLength: "Welsh placeholder", | ||
| hearingType: "Welsh placeholder", | ||
| venue: "Welsh placeholder", | ||
| additionalInformation: "Welsh placeholder" | ||
| }, | ||
| dataSource: "Welsh placeholder for 'Data source'", | ||
| backToTop: "Welsh placeholder" | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine its current content
find . -name "cy.ts" -path "*/care-standards-tribunal-weekly-hearing-list/*" | head -20Repository: hmcts/cath-service
Length of output: 139
🏁 Script executed:
# Look for the English counterpart
find . -name "en.ts" -path "*/care-standards-tribunal-weekly-hearing-list/*" | head -20Repository: hmcts/cath-service
Length of output: 139
🏁 Script executed:
# Search for the directory structure
fd "care-standards-tribunal-weekly-hearing-list" -type dRepository: hmcts/cath-service
Length of output: 232
🏁 Script executed:
# Read the English version to understand the expected content
cat ./libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/en.tsRepository: hmcts/cath-service
Length of output: 934
🏁 Script executed:
# Read the Welsh version as shown in review
cat ./libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/cy.tsRepository: hmcts/cath-service
Length of output: 782
🏁 Script executed:
# Check the directory structure
ls -la ./libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/Repository: hmcts/cath-service
Length of output: 501
Replace placeholder Welsh strings with real translations before release
The cy.ts file contains only "Welsh placeholder" values while the en.ts counterpart has complete English content. Per the localization requirements for /src/pages/** paths, this page must provide full Welsh support with actual translations—not placeholders. Shipping this to production would present broken Welsh UI to Welsh users.
Ensure all entries in the cy export match the structure and meaning of the English content (pageTitle, importantInformationTitle, searchCasesTitle, table headers, dataSource, backToTop, etc.) with proper Welsh translations before release.
🤖 Prompt for AI Agents
In libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/cy.ts
around lines 1 to 19, the exported cy object contains only "Welsh placeholder"
strings; replace each placeholder with proper Welsh translations that mirror the
en.ts structure and meaning (pageTitle, importantInformationTitle,
importantInformationText, importantInformationLinkText,
importantInformationLinkUrl, searchCasesTitle, searchCasesLabel, tableHeaders:
date/caseName/hearingLength/hearingType/venue/additionalInformation, dataSource,
backToTop). Ensure translations are accurate and context-appropriate (keep the
same keys and link URL), and run a quick UI spot-check to confirm text fits
layout before committing.
| const artefactId = req.query.artefactId as string; | ||
|
|
||
| if (!artefactId) { | ||
| return res.status(400).render("errors/common", { | ||
| en, | ||
| cy, | ||
| errorTitle: "Bad Request", | ||
| errorMessage: "Missing artefactId parameter" | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential path traversal vulnerability with unsanitized artefactId.
The artefactId is used directly in file path construction without validation. If an attacker provides a crafted artefactId containing ../, they could potentially read files outside the intended directory.
const artefactId = req.query.artefactId as string;
if (!artefactId) {
return res.status(400).render("errors/common", {
en,
cy,
errorTitle: "Bad Request",
errorMessage: "Missing artefactId parameter"
});
}
+// Validate artefactId format to prevent path traversal
+if (!/^[a-zA-Z0-9-]+$/.test(artefactId)) {
+ return res.status(400).render("errors/common", {
+ en,
+ cy,
+ errorTitle: "Bad Request",
+ errorMessage: "Invalid artefactId format"
+ });
+}Also applies to: 48-48
🤖 Prompt for AI Agents
In
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/index.ts
around lines 23-32 (and also at line 48), artefactId from req.query is used to
build file paths without validation which allows path traversal; validate and
sanitize artefactId before use by enforcing a strict allowlist pattern (e.g.
only alphanumeric, hyphen, underscore) or checking length, reject requests that
don't match, OR normalize the requested path with path.resolve and verify it is
inside the intended base directory (compare resolvedPath.startsWith(baseDir));
return 400 on invalid input and only use the sanitized/validated value for any
filesystem operations.
| return res.status(400).render("errors/common", { | ||
| en, | ||
| cy, | ||
| errorTitle: "Bad Request", | ||
| errorMessage: "Missing artefactId parameter" | ||
| }); |
There was a problem hiding this comment.
Error messages use hardcoded English instead of localized content.
All error rendering uses hardcoded English strings for errorTitle and errorMessage. These should use the localized t object for Welsh support, as required by coding guidelines.
Example fix for lines 26-31:
return res.status(400).render("errors/common", {
en,
cy,
- errorTitle: "Bad Request",
- errorMessage: "Missing artefactId parameter"
+ errorTitle: t.errors?.badRequest ?? "Bad Request",
+ errorMessage: t.errors?.missingArtefactId ?? "Missing artefactId parameter"
});Also applies to: 40-45, 55-60, 68-73, 98-103
junaidiqbalmoj
changed the base branch from
master
to
vibe-166-cst-excel-list
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/api/src/blob-ingestion/validation.ts (1)
50-50: Consider caching list types for improved performance.Fetching list types from the database on every blob validation request could become a bottleneck under load. Since list types are relatively static reference data, consider implementing an in-memory cache (e.g., with TTL or event-driven invalidation) to reduce database queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
libs/api/src/blob-ingestion/validation.test.ts(1 hunks)libs/api/src/blob-ingestion/validation.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/api/src/blob-ingestion/validation.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
libs/api/src/blob-ingestion/validation.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
libs/api/src/blob-ingestion/validation.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
libs/api/src/blob-ingestion/validation.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
libs/api/src/blob-ingestion/validation.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T09:48:13.010Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 136
File: libs/api/src/blob-ingestion/validation.ts:156-163
Timestamp: 2025-11-27T09:48:13.010Z
Learning: In libs/api/src/blob-ingestion/validation.ts, the permissive date validation in isValidISODate and isValidISODateTime functions is expected behavior and should not be flagged for stricter validation.
Applied to files:
libs/api/src/blob-ingestion/validation.ts
🧬 Code graph analysis (1)
libs/api/src/blob-ingestion/validation.ts (2)
libs/list-type-config/src/list-type/list-type-queries.ts (1)
findAllListTypes(3-14)libs/list-types/common/src/list-type-validator.ts (1)
validateListTypeJson(30-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (3)
libs/api/src/blob-ingestion/validation.ts (3)
1-2: LGTM: Imports follow coding guidelines.The workspace alias imports (
@hmcts/*) correctly replace mock data dependencies with database-backed implementations.
51-61: LGTM: List type validation correctly uses dynamic data.The validation logic properly checks against database-fetched list types and constructs informative error messages with actual available values.
136-141: LGTM: Data transformation and validation call are well-structured.The code correctly:
- Extracts only the required fields (
id,name,friendlyName) for validation, following the data isolation principle- Passes the transformed
listTypesInfotovalidateListTypeJsonwith the proper shape matching theListTypeInfointerface
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/list-type-config/src/pages/configure-list-type/success.test.ts (1)
58-107: Consider consolidating redundant cache header tests.Lines 58-92 test individual cache headers separately, and then lines 94-107 repeat the same assertions in a combined test. This creates maintenance overhead without adding meaningful coverage.
Consider keeping only the comprehensive test at lines 94-107 and removing the individual header tests, or document why separate tests are valuable for this particular scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/list-type-config/src/pages/configure-list-type/enter-details.test.ts(1 hunks)libs/list-type-config/src/pages/configure-list-type/preview.test.ts(1 hunks)libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.ts(1 hunks)libs/list-type-config/src/pages/configure-list-type/success.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.tslibs/list-type-config/src/pages/configure-list-type/enter-details.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.tslibs/list-type-config/src/pages/configure-list-type/enter-details.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source code using
*.test.tsor*.spec.tsnaming pattern
Files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.tslibs/list-type-config/src/pages/configure-list-type/enter-details.test.ts
libs/*/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/*/src/pages/**/*.ts: Create page controller files with GET and POST exports following the pattern:export const GET = async (req, res) => { ... }
Provide bothenandcylanguage objects in page controllers for English and Welsh support
Files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.tslibs/list-type-config/src/pages/configure-list-type/enter-details.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.tslibs/list-type-config/src/pages/configure-list-type/enter-details.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.tslibs/list-type-config/src/pages/configure-list-type/enter-details.test.ts
🧠 Learnings (3)
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : Include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey
Applied to files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.tslibs/list-type-config/src/pages/configure-list-type/enter-details.test.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to libs/*/src/pages/**/*.ts : Create page controller files with GET and POST exports following the pattern: `export const GET = async (req, res) => { ... }`
Applied to files:
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.tslibs/list-type-config/src/pages/configure-list-type/preview.test.tslibs/list-type-config/src/pages/configure-list-type/success.test.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to libs/*/src/pages/**/*.ts : Provide both `en` and `cy` language objects in page controllers for English and Welsh support
Applied to files:
libs/list-type-config/src/pages/configure-list-type/success.test.ts
🧬 Code graph analysis (1)
libs/list-type-config/src/pages/configure-list-type/enter-details.test.ts (2)
libs/list-type-config/src/list-type/list-type-queries.ts (2)
findListTypeById(16-27)findListTypeByName(29-33)libs/list-type-config/src/list-type/list-type-validation.ts (1)
validateListTypeDetails(4-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Tests
- GitHub Check: Test Changed Packages
🔇 Additional comments (5)
libs/list-type-config/src/pages/configure-list-type/select-sub-jurisdictions.test.ts (1)
38-192: Comprehensive test coverage for multi-step flow.The test suite thoroughly covers both GET and POST flows, including:
- English and Welsh content rendering
- Session data handling and redirects
- Validation error cases
- Preserving user selections on errors
The tests properly exercise the select-sub-jurisdictions step as part of the broader list-type configuration flow.
libs/list-type-config/src/pages/configure-list-type/enter-details.test.ts (1)
39-220: Well-structured tests for form entry and validation.The test suite provides thorough coverage of the enter-details page, including:
- Form population from session data
- Edit mode with existing list types
- Validation error handling
- Duplicate name detection with proper edit-mode exemption (lines 174-187)
- Session data preservation across submissions
The tests properly validate the initial step of the multi-step configuration flow.
libs/list-type-config/src/pages/configure-list-type/preview.test.ts (3)
131-155: Correct use of 303 redirect status for POST-Redirect-GET pattern.The test at line 154 verifies that a 303 status code is used for the redirect after successful save. This is the appropriate HTTP status code for the POST-Redirect-GET pattern, preventing form resubmission on browser refresh.
219-233: Good defensive error handling test.This test verifies that non-Error rejection values (e.g., string errors) are handled gracefully with a fallback error message. This defensive approach prevents crashes when dependencies reject with unexpected types.
38-234: Comprehensive preview and save flow coverage.The test suite thoroughly validates:
- Preview rendering with English and Welsh localization
- Sub-jurisdiction text formatting
- Save operation with proper session cleanup
- Edit mode via editId parameter
- Error handling with session preservation
The tests properly cover the final steps of the multi-step configuration flow before the success page.
| async function callHandler(handlers: RequestHandler | RequestHandler[], req: Request, res: Response) { | ||
| if (Array.isArray(handlers)) { | ||
| for (const handler of handlers) { | ||
| await new Promise<void>((resolve, reject) => { | ||
| const next = (err?: any) => (err ? reject(err) : resolve()); | ||
| const result = handler(req, res, next); | ||
| if (result instanceof Promise) { | ||
| result.then(() => resolve()).catch(reject); | ||
| } | ||
| }); | ||
| } | ||
| } else { | ||
| const result = handlers(req, res, () => {}); | ||
| if (result instanceof Promise) await result; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract shared test helper to reduce duplication.
The callHandler helper function is duplicated identically across all four configure-list-type test files (success.test.ts, select-sub-jurisdictions.test.ts, enter-details.test.ts, preview.test.ts). This violates the DRY principle.
Create a shared test utility file to eliminate duplication:
// libs/list-type-config/src/test-utils/call-handler.ts
import type { Request, RequestHandler, Response } from "express";
export async function callHandler(handlers: RequestHandler | RequestHandler[], req: Request, res: Response) {
if (Array.isArray(handlers)) {
for (const handler of handlers) {
await new Promise<void>((resolve, reject) => {
const next = (err?: any) => (err ? reject(err) : resolve());
const result = handler(req, res, next);
if (result instanceof Promise) {
result.then(() => resolve()).catch(reject);
}
});
}
} else {
const result = handlers(req, res, () => {});
if (result instanceof Promise) await result;
}
}Then import from each test file:
+import { callHandler } from "../../test-utils/call-handler.js";
-async function callHandler(handlers: RequestHandler | RequestHandler[], req: Request, res: Response) {
- if (Array.isArray(handlers)) {
- for (const handler of handlers) {
- await new Promise<void>((resolve, reject) => {
- const next = (err?: any) => (err ? reject(err) : resolve());
- const result = handler(req, res, next);
- if (result instanceof Promise) {
- result.then(() => resolve()).catch(reject);
- }
- });
- }
- } else {
- const result = handlers(req, res, () => {});
- if (result instanceof Promise) await result;
- }
-}🤖 Prompt for AI Agents
In libs/list-type-config/src/pages/configure-list-type/success.test.ts lines
4-19 the callHandler helper is duplicated across four test files; extract it
into a shared test utility (e.g.
libs/list-type-config/src/test-utils/call-handler.ts) exporting the same async
function signature, remove the duplicate implementations from success.test.ts
(and the other three test files), and update those tests to import callHandler
from the new test-utils path; ensure TypeScript types for Request,
RequestHandler, Response are imported in the new file and update any relative
import paths in the tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
520-530: Welsh language test still needs substantive assertions.As noted in the previous review, this test only verifies that the heading is visible but doesn't validate that Welsh content is actually displayed. Per coding guidelines, Welsh translations are required for all user-facing text and should be verified in tests.
The test should check for actual Welsh text content, for example:
const heading = page.locator("h1"); await expect(heading).toBeVisible(); // Verify actual Welsh text (update with correct Welsh translation) await expect(heading).toContainText("Llwytho");This remains an open item from the previous review.
Based on learnings: Welsh translations are required for all user-facing text.
🧹 Nitpick comments (5)
e2e-tests/tests/system-admin-dashboard.spec.ts (4)
24-27: Tile count update looks correct, but consider using recommended selectors.The numerical update from 8 to 9 tiles is consistent with the new "Configure List Type" tile addition.
However, per coding guidelines, E2E tests should prioritize
getByRole(),getByLabel(),getByText(), orgetByTestId()over CSS class selectors. Consider refactoring to usepage.getByRole('link', { name: /admin-tile/i })or similar semantic selectors for better accessibility alignment and test maintainability.As per coding guidelines, E2E tests should use selectors in priority order.
29-47: New tile data is correct; consider semantic selectors.The "Configure List Type" tile entry is properly defined with the correct href. However, the test uses CSS selectors (
a.admin-tile:has-text()) instead of the recommendedpage.getByRole('link', { name: title }), which would be more accessible and maintainable.Consider refactoring to:
for (const { title, href } of tileData) { - const link = page.locator(`a.admin-tile:has-text("${title}")`); + const link = page.getByRole('link', { name: title }); await expect(link).toBeVisible(); await expect(link).toHaveAttribute("href", href); }As per coding guidelines, E2E tests should use selectors in priority order.
49-62: Description update is accurate; refactor selectors for better semantics.The new description "Add and manage list type configurations" at line 61 is clear and consistent. However,
.admin-tile__descriptionCSS class selectors could be replaced withpage.getByRole()orpage.getByText()for better alignment with E2E best practices.As per coding guidelines, prioritize semantic selectors.
82-89: Accessible links count updated correctly; use semantic selectors.The update to 9 accessible links is correct. To fully align with accessibility testing best practices, consider using
page.getByRole('link')with appropriate filters instead of CSS class selectors.As per coding guidelines, E2E tests should use selectors in priority order.
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
300-303: Consider simplifying assertion logic.Lines 302 and 318 use a complex assertion pattern that, while functional, could be more readable:
const errorText = await errorSummary.textContent(); - expect(errorText && (errorText.includes("Invalid date format") || errorText.includes("dd/MM/yyyy"))).toBeTruthy(); + expect(errorText).toMatch(/Invalid date format|dd\/MM\/yyyy/);And similarly for line 318:
const errorText = await errorSummary.textContent(); - expect(errorText && (errorText.includes("HTML tags") || errorText.includes("Invalid content"))).toBeTruthy(); + expect(errorText).toMatch(/HTML tags|Invalid content/);This makes the intent clearer and handles null/undefined more gracefully.
Also applies to: 316-319
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts(2 hunks)e2e-tests/tests/system-admin-dashboard.spec.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source code using
*.test.tsor*.spec.tsnaming pattern
Files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E tests must be located ine2e-tests/directory with*.spec.tsnaming pattern
Tag nightly-only E2E tests with@nightlyin the test title
E2E tests should use selectors in priority order: getByRole(), getByLabel(), getByText(), getByTestId()
Accessibility testing (WCAG 2.2 AA) is mandatory - include axe-core checks in E2E tests
Include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey
Do not test visual styling (fonts, colors, margins, padding) in E2E tests
Files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
🧠 Learnings (13)
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : Include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey
Applied to files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : Accessibility testing (WCAG 2.2 AA) is mandatory - include axe-core checks in E2E tests
Applied to files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : Tag nightly-only E2E tests with `nightly` in the test title
Applied to files:
e2e-tests/tests/system-admin-dashboard.spec.tse2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E tests must be located in `e2e-tests/` directory with `*.spec.ts` naming pattern
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : Do not test visual styling (fonts, colors, margins, padding) in E2E tests
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to **/package.json : Use hmcts scope for package names: `hmcts/auth`, `hmcts/case-management`
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E tests should use selectors in priority order: getByRole(), getByLabel(), getByText(), getByTestId()
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Welsh translations are required for all user-facing text
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to libs/*/src/pages/**/*.ts : Provide both `en` and `cy` language objects in page controllers for English and Welsh support
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-11-27T14:18:22.932Z
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.932Z
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.
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.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:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to **/*.{ts,tsx} : Use parameterized database queries with Prisma (no raw SQL string concatenation)
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (2)
e2e-tests/tests/system-admin-dashboard.spec.ts (1)
64-67: All tile count updates are consistent and correct.The updates to 9 tiles are applied consistently across grid layout tests (line 66), keyboard navigation tests (lines 131-132), and responsive design tests (lines 145, 156, 167). The test coverage remains comprehensive.
Note: Welsh translation checks are not present in this dashboard test. If the system admin dashboard supports Welsh language, consider adding Welsh translation validation per E2E guidelines.
Based on learnings, E2E tests should include Welsh translation checks within the test journey.
Also applies to: 115-133, 137-169
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
6-6: LGTM: Database access properly configured.The prisma import follows the workspace alias pattern correctly and enables database verification in the E2E test flow.
| test("should meet WCAG 2.2 AA standards on list display page", async ({ page }) => { | ||
| // Placeholder - implement once list display is available | ||
| // await page.goto("/care-standards-tribunal-weekly-hearing-list?artefactId=test"); | ||
| // | ||
| // const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| // .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| // .disableRules(["target-size", "link-name"]) | ||
| // .analyze(); | ||
| // | ||
| // expect(accessibilityScanResults.violations).toEqual([]); | ||
| // Upload a valid CST file | ||
| await completeCSTUploadFlow(page); | ||
|
|
||
| // Query database to get the most recent CST artefact | ||
| const artefact = await prisma.artefact.findFirst({ | ||
| where: { locationId: "9001", listTypeId: 9 }, | ||
| orderBy: { lastReceivedDate: "desc" } | ||
| }); | ||
|
|
||
| expect(artefact).toBeTruthy(); | ||
|
|
||
| // Navigate to the list display page | ||
| await page.goto(`/care-standards-tribunal-weekly-hearing-list?artefactId=${artefact!.artefactId}`); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Verify the page loaded successfully (not an error page) | ||
| const heading = page.locator("h1"); | ||
| await expect(heading).toBeVisible(); | ||
|
|
||
| // Run accessibility scan | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .disableRules(["target-size", "link-name"]) | ||
| .analyze(); | ||
|
|
||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
Improve null handling and use semantic selectors.
The database-backed verification is a good addition, but there are several improvements needed:
- Line 481: The non-null assertion (
artefact!) is risky. If the artefact is not found, the test will throw a runtime error instead of providing a clear failure message. Consider restructuring:
- expect(artefact).toBeTruthy();
-
- // Navigate to the list display page
- await page.goto(`/care-standards-tribunal-weekly-hearing-list?artefactId=${artefact!.artefactId}`);
+ expect(artefact).toBeTruthy();
+ if (!artefact) {
+ throw new Error("No artefact found in database after upload");
+ }
+
+ // Navigate to the list display page
+ await page.goto(`/care-standards-tribunal-weekly-hearing-list?artefactId=${artefact.artefactId}`);-
Line 485: Use semantic selectors per coding guidelines. Replace
page.locator("h1")withpage.getByRole("heading", { level: 1 })for better accessibility and maintainability. -
Line 474: The database query filters only by
locationIdandlistTypeId. If tests run in parallel or cleanup is incomplete, this could match an artefact from a different test run. Consider filtering by a tighter time window or using theuploadIdfromcompleteCSTUploadFlowif available. -
Line 482: Replace the hard-coded
waitForTimeout(1000)withawait expect(heading).toBeVisible()or another condition-based wait.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
e2e-tests/utils/seed-list-types.ts (3)
100-112: Type casting withprisma as anybypasses Prisma's type safety.Using
(prisma as any)throughout the file circumvents TypeScript's type checking for Prisma operations. This could mask issues where the schema doesn't match the expected structure.Consider properly typing these operations by ensuring the Prisma client is generated with the correct models, or create a typed wrapper if the models are dynamically available.
17-18: Welsh translations appear to be placeholder values.Several
welshFriendlyNamevalues are identical to their English counterparts (e.g., "Civil Daily Cause List"). If Welsh translations are required for E2E testing of Welsh language support, these should be updated. If this is intentional placeholder data for initial testing, consider adding a TODO comment.
13-13: Consider using SCREAMING_SNAKE_CASE for the constant array.Per coding guidelines, constants should use SCREAMING_SNAKE_CASE. Consider renaming
listTypestoLIST_TYPES.-const listTypes: ListTypeData[] = [ +const LIST_TYPES: ListTypeData[] = [And update the reference on line 109:
- for (const listType of listTypes) { + for (const listType of LIST_TYPES) {e2e-tests/global-setup.ts (1)
70-72: Minor: Duplicate log message.The log "Seeding list types..." appears here and also inside
seedListTypes()function (line 98 of seed-list-types.ts). Consider removing one to avoid duplicate output.Either remove this log:
// Step 4: Seed list types (required for VIBE-309 Configure List Type feature) - console.log("Seeding list types..."); await seedListTypes();Or remove the log inside
seedListTypes().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/global-setup.ts(4 hunks)e2e-tests/utils/seed-list-types.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use camelCase for TypeScript variables:userId,caseDetails,documentId
Use PascalCase for classes and interfaces:UserService,CaseRepository. NOIprefix.
Use kebab-case for file and directory names:user-service.ts,case-management/
Use boolean variable names withis/has/canprefix:isActive,hasAccess,canEdit
Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom
Always add.jsextension to relative imports in ES modules (e.g.,import { foo } from "./bar.js")
Use workspace aliases for imports (@hmcts/*) instead of relative paths across packages
Use strict TypeScript mode enabled; noanywithout justification
Use parameterized database queries with Prisma (no raw SQL string concatenation)
Do not include sensitive data in logs
Files:
e2e-tests/utils/seed-list-types.tse2e-tests/global-setup.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use SCREAMING_SNAKE_CASE for constants:
MAX_FILE_SIZE,DEFAULT_TIMEOUT
Files:
e2e-tests/utils/seed-list-types.tse2e-tests/global-setup.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
e2e-tests/utils/seed-list-types.tse2e-tests/global-setup.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use CommonJS - ES modules only with import/export syntax
Files:
e2e-tests/utils/seed-list-types.tse2e-tests/global-setup.ts
🧠 Learnings (2)
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E tests must be located in `e2e-tests/` directory with `*.spec.ts` naming pattern
Applied to files:
e2e-tests/global-setup.ts
📚 Learning: 2025-12-15T13:38:57.483Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.483Z
Learning: Applies to e2e-tests/**/*.spec.ts : Include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey
Applied to files:
e2e-tests/global-setup.ts
🧬 Code graph analysis (1)
e2e-tests/global-setup.ts (1)
e2e-tests/utils/seed-list-types.ts (1)
seedListTypes(97-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (3)
e2e-tests/utils/seed-list-types.ts (1)
130-134: Verify: All sub-jurisdictions are linked to every list type.The seed logic associates every sub-jurisdiction with each list type. If this is intentional for test coverage, it's fine. However, if production data requires selective associations, consider making sub-jurisdiction mappings configurable per list type in the
ListTypeDatainterface.e2e-tests/global-setup.ts (2)
5-5: LGTM!Import correctly uses the
.jsextension as required for ES modules, following the coding guidelines.
66-76: LGTM! Seeding order is correct.The seeding order ensures dependencies are satisfied: reference data (including sub-jurisdictions) is seeded before list types, which require sub-jurisdictions to exist. The helpful Jira reference in the comment provides good context.
1 participant
Jira link
https://tools.hmcts.net/jira/browse/VIBE-309
Change description
Configure list type and get list from database
Summary by CodeRabbit
New Features
Tests
Documentation
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.