VIBE-323 Add Welsh translations for Care Standards Tribunal Weekly Hearing List - #205
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe PR refactors the Care Standards Tribunal weekly hearing list to enhance header display with improved date/time formatting, introduces Welsh language translations replacing placeholders, updates template styling, and expands the page renderer to support dynamic provenance label localization. Changes
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
268-281: Test navigates to hardcoded artefactId, ignoring uploaded artefact.
completeCSTUploadFlowreturns the actualartefactId, but line 275 navigates toartefactId=test. This test will not verify the uploaded content and may fail or produce false positives.Proposed fix
test("should display the published CST list with correct formatting", async ({ page }) => { // Upload and publish the list - await completeCSTUploadFlow(page); + const artefactId = await completeCSTUploadFlow(page); // Navigate to the published list - // Note: In a real scenario, you'd navigate via search or direct URL - // For now, we'll construct the URL pattern - await page.goto("/care-standards-tribunal-weekly-hearing-list?artefactId=test"); - await page.waitForTimeout(1000); + await navigateToPublishedList(page, artefactId); // Verify page loads (may need adjustment based on actual implementation) const heading = page.locator("h1"); await expect(heading).toBeVisible(); });libs/list-types/common/src/mock-list-types.ts (1)
85-91: Replace the Welsh placeholder with the authoritative translation.This list type is user-facing with dedicated Welsh localisation support. Replace
welshFriendlyNamewith"Rhestr Gwrandawiadau Wythnosol y Tribiwnlys Safonau Gofal"(from the verified translation inlibs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/cy.ts).libs/list-types/common/package.json (1)
14-26: Ensure assets are copied intodistfor production exports.The new export map points production consumers to
./dist/assets/**, but the build only copies views. Unless another pipeline step handles assets, these exports will resolve to missing files.Suggested fix
"scripts": { - "build": "tsc && yarn build:nunjucks", + "build": "tsc && yarn build:nunjucks && yarn build:assets", + "build:assets": "mkdir -p dist/assets && cp -R src/assets/* dist/assets/", "build:nunjucks": "mkdir -p dist/views && cd src/views && find . -name '*.njk' -exec sh -c 'mkdir -p ../../dist/views/$(dirname {}) && cp {} ../../dist/views/{}' \\; || true",
🟡 Minor comments (17)
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/conversion/cst-config.ts-17-17 (1)
17-17: Localise the validation hint message.
Line 17’s English-only hint string is user-facing; please route it through the Welsh translation mechanism (or provide a bilingual string) to meet the bilingual requirement. Based on learnings, Welsh translations are required for all user-facing text.libs/list-types/london-administrative-court-daily-cause-list/src/schemas/london-administrative-court-daily-cause-list.json-22-25 (1)
22-25: Tighten time validation to reject invalid hours/minutes and allow AM/PM case.Current pattern accepts values like
19pmor9:99amand rejectsAM/PM. Consider a stricter 12‑hour pattern with optional minutes.💡 Suggested regex update
- "pattern": "^\\d{1,2}([:.]\\d{2})?[ap]m\\s*$" + "pattern": "^(1[0-2]|0?[1-9])([:.][0-5]\\d)?\\s*[aApP][mM]\\s*$"Also applies to: 59-62
libs/list-types/rcj-standard-daily-cause-list/src/pages/kings-bench-masters-daily-cause-list.njk-10-12 (1)
10-12: Guard the fact-link block when data is optional.If
common.factLinkUrlorcommon.factLinkTextis missing, this renders an empty link. Consider rendering this block only when both values are present.💡 Proposed fix
- <p class="govuk-body"> - <a href="{{ common.factLinkUrl }}" class="govuk-link">{{ common.factLinkText }}</a> {{ common.factAdditionalText }} - </p> + {% if common.factLinkUrl and common.factLinkText %} + <p class="govuk-body"> + <a href="{{ common.factLinkUrl }}" class="govuk-link">{{ common.factLinkText }}</a> {{ common.factAdditionalText }} + </p> + {% endif %}libs/list-types/rcj-standard-daily-cause-list/src/pages/county-court-central-london-civil-daily-cause-list.njk-10-12 (1)
10-12: Guard the optional fact link output.If
common.factLinkUrlorcommon.factLinkTextis absent, this will render an empty/undefined link. Wrap the anchor in a conditional while keeping the additional text.Proposed fix
<p class="govuk-body"> - <a href="{{ common.factLinkUrl }}" class="govuk-link">{{ common.factLinkText }}</a> {{ common.factAdditionalText }} + {% if common.factLinkUrl and common.factLinkText %} + <a href="{{ common.factLinkUrl }}" class="govuk-link">{{ common.factLinkText }}</a> + {% endif %} + {{ common.factAdditionalText }} </p>libs/list-types/rcj-standard-daily-cause-list/README.md-20-20 (1)
20-20: Minor: "Court room" should be "Courtroom".Static analysis flagged this as a compound noun.
📝 Suggested fix
-- **Venue** (required): Court room or location +- **Venue** (required): Courtroom or locationlibs/list-types/rcj-standard-daily-cause-list/README.md-47-50 (1)
47-50: Minor: Add language specifier to fenced code block.Markdownlint flags code blocks without a language. Use
textfor plain URL patterns.📝 Suggested fix
-``` +```text /civil-courts-rcj-daily-cause-list?artefactId=<id> /court-of-appeal-criminal-division-daily-cause-list?artefactId=<id> ```libs/list-types/london-administrative-court-daily-cause-list/README.md-20-20 (1)
20-20: Minor typo: "Court room" → "Courtroom".The noun "Courtroom" is typically written as one word.
📝 Suggested fix
-- **Venue** (required): Court room or location +- **Venue** (required): Courtroom or locationlibs/list-types/rcj-standard-daily-cause-list/src/pages/en.ts-117-125 (1)
117-125: Minor capitalisation inconsistency in table headers.The table headers use sentence case (
Case number,Case details,Hearing type,Additional information), whilst other list type files use title case (Case Number,Case Details,Hearing Type,Additional Information). Consider aligning for UI consistency.Suggested fix
tableHeaders: { venue: "Venue", judge: "Judge", time: "Time", - caseNumber: "Case number", - caseDetails: "Case details", - hearingType: "Hearing type", - additionalInformation: "Additional information" + caseNumber: "Case Number", + caseDetails: "Case Details", + hearingType: "Hearing Type", + additionalInformation: "Additional Information" },docs/tickets/VIBE-317/specification.md-99-103 (1)
99-103: Replace bare URL with a Markdown link.This avoids the MD034 lint failure.
Suggested fix
-- **URL:** https://www.find-court-tribunal.service.gov.uk/ +- **URL:** [find-court-tribunal.service.gov.uk](https://www.find-court-tribunal.service.gov.uk/)docs/tickets/VIBE-317/implementation-summary.md-118-121 (1)
118-121: Replace bare URL with a Markdown link.This avoids MD034.
Suggested fix
-- FaCT link (https://www.find-court-tribunal.service.gov.uk/) +- FaCT link ([find-court-tribunal.service.gov.uk](https://www.find-court-tribunal.service.gov.uk/))docs/tickets/VIBE-317/implementation-progress.md-235-253 (1)
235-253: Add a language to the fenced code block.This resolves MD040 and improves readability.
Suggested fix
-``` +```text module-name/ ├── package.json ├── tsconfig.json ├── README.md └── src/ ├── config.ts (module exports) ├── index.ts (business logic exports) ├── models/types.ts ├── conversion/config.ts ├── schemas/*.json ├── validation/json-validator.ts + tests ├── rendering/renderer.ts + tests └── pages/ ├── index.ts (GET handler) ├── template.njk ├── en.ts └── cy.ts -``` +```libs/list-types/london-administrative-court-daily-cause-list/src/pages/cy.ts-6-8 (1)
6-8: Translate remaining English place name in the Welsh locale.
locationLine2uses “Strand, London”, leaving “London” in English. Please translate/confirm the Welsh form (e.g., “Strand, Llundain”) to keep all user-facing text in Welsh.Based on learnings, Welsh translations are required for all user-facing text.
libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts-93-99 (1)
93-99: Make Excel detection case‑insensitive and avoid conversion when filename is missing.
Line 98 treats “.JSON” as Excel and also marks missing filenames as Excel. Consider normalising and guarding nulls to prevent unintended conversion.🛠️ Proposed fix
- const isExcelFile = !uploadData.fileName?.endsWith(".json"); + const fileName = uploadData.fileName?.toLowerCase() ?? ""; + const isExcelFile = fileName !== "" && !fileName.endsWith(".json");Also applies to: 121-121
libs/list-types/court-of-appeal-civil-daily-cause-list/README.md-49-50 (1)
49-50: Refresh date examples to the current year.The examples still use 2025; updating to 2026 avoids confusion for readers in the current year.
Proposed update
-- **Tab 2**: Date must be dd/MM/yyyy format (e.g., 15/01/2025), Time must be HH:MM format +- **Tab 2**: Date must be dd/MM/yyyy format (e.g., 15/01/2026), Time must be HH:MM format @@ -- Dates are formatted according to locale (English: "15 January 2025", Welsh: "15 Ionawr 2025") +- Dates are formatted according to locale (English: "15 January 2026", Welsh: "15 Ionawr 2026")Also applies to: 60-60
libs/list-types/court-of-appeal-civil-daily-cause-list/README.md-75-77 (1)
75-77: Specify a language on the fenced block.This fixes the markdownlint MD040 warning.
Proposed fix
-``` +```text /court-of-appeal-civil-division-daily-cause-list?artefactId=<id></details> </blockquote></details> <details> <summary>libs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/index.ts-77-88 (1)</summary><blockquote> `77-88`: **`JSON.parse` failure would be caught as 500, not 400.** If the file contains malformed JSON, `JSON.parse` throws a `SyntaxError` that will be caught by the outer `catch` block and return a 500 error. Consider wrapping the parse in its own try-catch to return 400 with a more descriptive message. <details> <summary>🐛 Proposed fix</summary> ```diff - const jsonData: CourtOfAppealCivilData = JSON.parse(jsonContent); + let jsonData: CourtOfAppealCivilData; + try { + jsonData = JSON.parse(jsonContent); + } catch { + return res.status(400).render("errors/common", { + en, + cy, + errorTitle: "Invalid Data", + errorMessage: "The list data could not be parsed" + }); + }libs/list-types/administrative-court-daily-cause-list/src/pages/index.ts-36-39 (1)
36-39: Normalise locale before indexing list configIf
res.locals.localeis something likeen-GB,listConfig[locale]will be undefined. Consider coercing to"en"/"cy"once.♻️ Suggested change
- const locale = res.locals.locale || "en"; - const t = locale === "cy" ? cy : en; + const locale = res.locals.locale === "cy" ? "cy" : "en"; + const t = locale === "cy" ? cy : en; @@ - const listTitle = listConfig[locale as "en" | "cy"]; + const listTitle = listConfig[locale];Also applies to: 105-107
🧹 Nitpick comments (52)
libs/simple-router/src/simple-router.ts (1)
108-113: Validation is good; consider explicit interface typing.The runtime validation is sound. However, the
RouteModuleinterface (lines 185-188) relies on the index signature[key: string]: unknownto permitROUTES. Adding an explicit optional property would improve type safety and discoverability.♻️ Suggested interface update
export interface RouteModule { [key: string]: unknown; onError?: ErrorRequestHandler; + ROUTES?: string[]; }libs/list-types/london-administrative-court-daily-cause-list/src/schemas/london-administrative-court-daily-cause-list.json (1)
8-80: Reduce duplication with a shared item definition.Both arrays repeat the same object schema; using
definitions+$refwill make updates safer and more consistent.♻️ Suggested refactor
{ "$schema": "http://json-schema.org/draft-07/schema#", "title": "London Administrative Court Daily Cause List", "description": "Schema for London Administrative Court with Main hearings and Planning Court tabs", "type": "object", "required": ["mainHearings", "planningCourt"], + "definitions": { + "hearingItem": { + "type": "object", + "required": ["venue", "judge", "time", "caseNumber", "caseDetails", "hearingType"], + "properties": { + "venue": { "type": "string", "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" }, + "judge": { "type": "string", "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" }, + "time": { "type": "string", "pattern": "^(1[0-2]|0?[1-9])([:.][0-5]\\d)?\\s*[aApP][mM]\\s*$" }, + "caseNumber": { "type": "string", "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" }, + "caseDetails": { "type": "string", "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" }, + "hearingType": { "type": "string", "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" }, + "additionalInformation": { "type": "string", "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" } + } + } + }, "properties": { "mainHearings": { "type": "array", - "items": { ... } + "items": { "$ref": "#/definitions/hearingItem" } }, "planningCourt": { "type": "array", - "items": { ... } + "items": { "$ref": "#/definitions/hearingItem" } } } }libs/list-types/rcj-standard-daily-cause-list/src/pages/family-division-high-court-daily-cause-list.njk (1)
21-24: Consider refactoring the long HTML concatenation for maintainability.The inline string concatenation is dense and brittle. A small refactor to build the content with standard Nunjucks markup (e.g., using
{% set %}blocks or partials) will improve readability and reduce future editing errors.e2e-tests/tests/care-standards-tribunal-upload.spec.ts (6)
14-18: Add guard for missing environment variables.Using non-null assertions without validation will produce cryptic errors if the environment variables are unset. Consider adding explicit checks.
Suggested improvement
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); }
166-198: AvoidwaitForTimeoutin favour of deterministic waits.
waitForTimeout(1000)on line 168 introduces flakiness. UsewaitForSelector,waitForLoadState, or similar Playwright APIs that wait for specific conditions.Additionally, per coding guidelines, prefer
getByLabel()orgetByRole()over attribute selectors for form inputs where possible.Example fix for waitForTimeout
await page.goto("/non-strategic-upload?locationId=9001"); - await page.waitForTimeout(1000); + await page.waitForLoadState("domcontentloaded");
208-224: Consider usingwaitForSelectorinstead ofwaitForTimeout.Line 210 uses a fixed delay. Replace with a condition-based wait to improve reliability.
Suggested fix
// Navigate to summary of publications to find the artefactId await page.goto("/summary-of-publications"); - await page.waitForTimeout(1000); + await page.waitForSelector('.govuk-list a[href*="care-standards-tribunal-weekly-hearing-list?artefactId="]');
226-230: ReplacewaitForTimeoutwith condition-based wait.Line 229 uses a fixed delay after navigation.
Suggested fix
async function navigateToPublishedList(page: Page, artefactId: string) { await page.goto(`/care-standards-tribunal-weekly-hearing-list?artefactId=${artefactId}`); - await page.waitForTimeout(1000); + await page.waitForLoadState("domcontentloaded"); }
364-374: Consider extracting repeated loop pattern to a helper.The loop pattern to find "Last updated" text is duplicated in the Welsh language test (lines 617-627). Extract to a reusable helper for maintainability.
Example helper
async function findParagraphContaining(page: Page, searchText: string): Promise<string | null> { const paragraphs = page.locator(".govuk-body"); for (let i = 0; i < await paragraphs.count(); i++) { const text = await paragraphs.nth(i).textContent(); if (text?.includes(searchText)) { return text; } } return null; }
501-507: Search highlighting assertion could be more specific.The comment states search "highlights, doesn't hide", but the test only verifies row count and text content. Consider asserting the presence of highlight markup (e.g.,
<mark>elements) if highlighting is implemented.libs/list-types/rcj-standard-daily-cause-list/src/pages/kings-bench-division-daily-cause-list.njk (1)
21-24: Avoid long HTML string concatenation; prefer a Nunjucks block with escaping.The current
html:value is built via concatenation, which is hard to read and bypasses escaping. If anylistContent.*ever comes from non‑static sources, this becomes an XSS risk. Consider constructing a block with explicit| escapeper paragraph and pass that ashtml. Please confirm alllistContent.*are static translations.♻️ Example refactor
+{% set importantInfoHtml %} + <h3 class="govuk-heading-s govuk-!-margin-bottom-2">{{ listContent.remoteHearingsTitle | escape }}</h3> + {% for p in listContent.remoteHearingsText.split('\n\n') %} + <p class="govuk-body">{{ p | escape }}</p> + {% endfor %} + <h3 class="govuk-heading-s govuk-!-margin-top-6 govuk-!-margin-bottom-2">{{ listContent.remoteJudgmentsTitle | escape }}</h3> + {% for p in listContent.remoteJudgmentsText.split('\n\n') %} + <p class="govuk-body">{{ p | escape }}</p> + {% endfor %} + <h3 class="govuk-heading-s govuk-!-margin-top-6 govuk-!-margin-bottom-2">{{ listContent.bundlesTitle | escape }}</h3> + <p class="govuk-body">{{ listContent.bundleFilingText | escape }}</p> +{% endset %} + {{ govukDetails({ summaryText: common.importantInfoTitle, - html: '<h3 class="govuk-heading-s govuk-!-margin-bottom-2">' + listContent.remoteHearingsTitle + '</h3><p class="govuk-body">' + listContent.remoteHearingsText.split('\n\n').join('</p><p class="govuk-body">') + '</p><h3 class="govuk-heading-s govuk-!-margin-top-6 govuk-!-margin-bottom-2">' + listContent.remoteJudgmentsTitle + '</h3><p class="govuk-body">' + listContent.remoteJudgmentsText.split('\n\n').join('</p><p class="govuk-body">') + '</p><h3 class="govuk-heading-s govuk-!-margin-top-6 govuk-!-margin-bottom-2">' + listContent.bundlesTitle + '</h3><p class="govuk-body">' + listContent.bundleFilingText + '</p>', + html: importantInfoHtml, open: true }) }}libs/list-types/rcj-standard-daily-cause-list/src/pages/county-court-central-london-civil-daily-cause-list.njk (1)
36-62: Use thegovukTablemacro for the hearings table.The table is hand-built, but coding guidelines require GOV.UK Frontend component macros throughout these templates. Switch to
govukTablefor consistency and to follow the established pattern used in similar list-type pages.libs/list-types/common/src/config.ts (1)
7-8: Add missing standard config exports.This config still omits pageRoutes/apiRoutes/prismaSchemas, which the standard config contract expects. Consider exporting empty placeholders if not applicable. As per coding guidelines, please align with the required config interface.
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/config.ts (1)
7-12: Add missing standard config exports.apiRoutes and prismaSchemas are still missing from this config. Please export them (even as empty stubs) to match the standard interface. As per coding guidelines, please align with the required config interface.
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/config.test.ts (1)
81-83: Usepath.relativeto avoid false-positive prefix matches.
startsWithcan pass for/foo/barvs/foo/barley. Prefer a relative-path check.♻️ Proposed update
- it("assets should be subdirectory of moduleRoot", () => { - expect(assets.startsWith(moduleRoot)).toBe(true); - }); + it("assets should be subdirectory of moduleRoot", () => { + const relativeAssets = path.relative(moduleRoot, assets); + expect(relativeAssets).toBe("assets"); + });libs/list-types/court-of-appeal-civil-daily-cause-list/src/config.test.ts (1)
20-24: Consider asserting the assets path exists.This keeps the assets check on par with moduleRoot/pageRoutes by validating the directory on disk.
Proposed change
it("should export assets as a valid directory path", () => { expect(assets).toBeDefined(); expect(typeof assets).toBe("string"); expect(assets).toContain("assets"); + expect(existsSync(assets)).toBe(true); });libs/list-types/rcj-standard-daily-cause-list/src/config.test.ts (1)
1-22: Align tests with standard config exports (assets).Once
assetsis exported, include a basic path check so this test matches the standard config contract.Proposed change
-import { moduleRoot, pageRoutes } from "./config.js"; +import { assets, moduleRoot, pageRoutes } from "./config.js";it("should have pageRoutes.path as a subdirectory of moduleRoot", () => { expect(pageRoutes.path.startsWith(moduleRoot)).toBe(true); }); + + it("should export assets as a valid directory path", () => { + expect(assets).toBeDefined(); + expect(typeof assets).toBe("string"); + expect(assets).toContain("assets"); + });Based on learnings, module
config.tsmust export standardised interfaces:pageRoutes,apiRoutes,prismaSchemas,assets.libs/list-types/rcj-standard-daily-cause-list/src/models/types.ts (1)
1-11: Consider extractingStandardHearingto@hmcts/list-types-common.This interface is duplicated verbatim in
london-administrative-court-daily-cause-listandadministrative-court-daily-cause-list. Centralising it would reduce maintenance overhead.♻️ Proposed approach
Export from
libs/list-types/common/src/models/types.ts:export interface StandardHearing { venue: string; judge: string; time: string; caseNumber: string; caseDetails: string; hearingType: string; additionalInformation: string; } export type StandardHearingList = StandardHearing[];Then import in each list-type module:
import { StandardHearing, StandardHearingList } from "@hmcts/list-types-common";libs/list-types/administrative-court-daily-cause-list/src/config.test.ts (1)
20-24: Consider addingexistsSynccheck for assets path.The tests for
moduleRootandpageRoutes.pathverify directory existence, but theassetstest only checks the string contains "assets". For consistency, consider verifying the directory exists.♻️ Suggested addition
it("should export assets as a valid directory path", () => { expect(assets).toBeDefined(); expect(typeof assets).toBe("string"); expect(assets).toContain("assets"); + expect(existsSync(assets)).toBe(true); });libs/list-types/london-administrative-court-daily-cause-list/src/config.test.ts (1)
20-24: Same suggestion: addexistsSynccheck for assets.As with the administrative-court module, consider adding existence verification for consistency.
♻️ Suggested addition
it("should export assets as a valid directory path", () => { expect(assets).toBeDefined(); expect(typeof assets).toBe("string"); expect(assets).toContain("assets"); + expect(existsSync(assets)).toBe(true); });libs/list-types/common/src/rendering/date-formatting.test.ts (1)
81-101: Good coverage for dd/MM/yyyy parsing.The tests cover English and Welsh locales, different months, and single-digit day/month handling. Consider adding an edge case for invalid input (e.g., malformed date string) to ensure graceful handling.
libs/list-types/rcj-standard-daily-cause-list/src/pages/civil-courts-rcj-daily-cause-list.njk (3)
21-25: HTML concatenation in govukDetails.The
htmlparameter uses string concatenation which could be fragile if any of the text variables contain special characters. Ensure the source data is properly escaped before reaching this template.
27-33: Redundant aria-label on input.The input has both a
<label>element (albeit visually hidden) and anaria-labelattribute. Having both is redundant; the<label>withforattribute already provides accessibility. Consider removing thearia-labelto avoid potential screen reader confusion.Suggested change
- <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text" aria-label="{{ common.searchCasesLabel }}"> + <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text">
48-60: Consider handling empty hearings state.If the
hearingsarray is empty, the table will render with headers but no body rows. Consider adding a message when there are no hearings to display, consistent with other templates that usenoHearingsMessage.Suggested change
<tbody class="govuk-table__body"> + {% if hearings.length == 0 %} + <tr class="govuk-table__row"> + <td class="govuk-table__cell" colspan="7">{{ common.noHearingsMessage }}</td> + </tr> + {% endif %} {% for hearing in hearings %} <tr class="govuk-table__row">libs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/court-of-appeal-civil-daily-cause-list.njk (2)
29-33: Redundantaria-labelon search input.The
<label for="case-search-input">already provides accessibility for screen readers. The additionalaria-labelattribute is redundant and may cause the label to be announced twice.Suggested fix
<label class="govuk-label govuk-visually-hidden" for="case-search-input"> {{ t.searchCasesLabel }} </label> - <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">
35-71: Consider adding a visible heading for the daily hearings section.The future judgments section has a visible
<h2>heading (line 76), but the daily hearings section relies solely onaria-labelon the table. A visible heading would improve consistency and navigation.Suggested addition after line 37
<div class="hearings-section" id="daily-hearings-section"> + <h2 class="govuk-heading-m">{{ t.dailyHearingsTitle }}</h2> <div id="daily-hearings-table-container">libs/list-types/rcj-standard-daily-cause-list/src/pages/senior-courts-costs-office-daily-cause-list.njk (1)
2-61: UsegovukTablemacro instead of raw table markup.This aligns with the requirement to use GOV.UK component macros and keeps table semantics/spacing consistent. Based on learnings, please refactor the table to
govukTable, ideally by building rows in the template or renderer.libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts (1)
12-32: ValidateartefactIdformat before querying.The endpoint currently checks presence only; please validate the expected format (e.g., UUID) to satisfy the input‑validation requirement and fail fast with 400.
♻️ Suggested update (format validation)
const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); // Navigate to monorepo root (from libs/list-types/civil-and-family-daily-cause-list/src/pages/) const MONOREPO_ROOT = path.join(__dirname, "..", "..", "..", "..", ".."); const TEMP_UPLOAD_DIR = path.join(MONOREPO_ROOT, "storage", "temp", "uploads"); +const ARTEFACT_ID_PATTERN = + /^[0-9a-fA-F-]{8}-[0-9a-fA-F-]{4}-[1-5][0-9a-fA-F-]{3}-[89abAB][0-9a-fA-F-]{3}-[0-9a-fA-F-]{12}$/; export const GET = async (req: Request, res: Response) => { const locale = res.locals.locale || "en"; const t = locale === "cy" ? cy : en; const artefactId = req.query.artefactId as string; - if (!artefactId) { + if (!artefactId || !ARTEFACT_ID_PATTERN.test(artefactId)) { return res.status(400).render("errors/common", { en, cy, errorTitle: t.errorTitle, errorMessage: t.errorMessage }); }As per coding guidelines, input validation must be performed on all endpoints.
libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/index.test.ts (2)
5-13: Rename the hoisted mock to SCREAMING_SNAKE_CASE.This is a module‑level constant and should follow the constant naming convention; please rename it and update references.
♻️ Suggested rename
-const mockValidate = vi.hoisted(() => vi.fn()); +const MOCK_VALIDATE = vi.hoisted(() => vi.fn()); vi.mock("@hmcts/list-types-common", () => ({ - createJsonValidator: () => mockValidate + createJsonValidator: () => MOCK_VALIDATE }));As per coding guidelines, constants should use SCREAMING_SNAKE_CASE.
244-292: Add a Welsh string assertion to cover translation wiring.The Welsh‑locale test only checks
locale: "cy"and doesn’t validate that Welsh strings (e.g., list title) are actually passed/rendered. Consider asserting the Welsh list title (sourced from the locale resource) in the render call or view model to catch regressions. Based on learnings, Welsh translations are required for all user‑facing text.e2e-tests/tests/court-of-appeal-civil-viewing.spec.ts (1)
56-72: Prefer role/label selectors and deterministic waits to reduce flakiness.The upload helpers rely on CSS selectors and
waitForTimeoutcalls. Where possible, usegetByRole/getByLabel/getByTextand wait on visible elements or URL changes instead of fixed delays to make tests more resilient. As per coding guidelines, prefer getByRole/getByLabel/getByText before CSS selectors in E2E tests.Also applies to: 206-221
libs/list-types/court-of-appeal-civil-daily-cause-list/src/schemas/court-of-appeal-civil-daily-cause-list.json (1)
1-86: Consider disallowing unknown fields in the schema.Without
additionalProperties: false, extra keys are silently accepted, which can mask payload issues. Consider adding it at the root and item levels.♻️ Suggested hardening
{ "$schema": "http://json-schema.org/draft-07/schema#", "title": "Court of Appeal (Civil Division) Daily Cause List", "description": "Schema for Court of Appeal Civil with Daily hearings and Future judgments tabs", "type": "object", + "additionalProperties": false, "required": ["dailyHearings", "futureJudgments"], "properties": { "dailyHearings": { "type": "array", "items": { "type": "object", + "additionalProperties": false, "required": ["venue", "judge", "time", "caseNumber", "caseDetails", "hearingType"], "properties": { ... } } }, "futureJudgments": { "type": "array", "items": { "type": "object", + "additionalProperties": false, "required": ["date", "venue", "judge", "time", "caseNumber", "caseDetails", "hearingType"], "properties": { ... } } } } }libs/list-types/common/src/validation/json-validator.ts (1)
4-5: Consider improving type safety.Multiple uses of
anyreduce type safety. TheAjv as anycast may be necessary due to ESM interop, but thecompiledValidatorsmap and error handling could be typed more strictly using AJV's built-in types.♻️ Suggested improvement
+import type { ValidateFunction } from "ajv"; + const ajv = new (Ajv as any)({ allErrors: true }); -const compiledValidators = new Map<string, any>(); +const compiledValidators = new Map<string, ValidateFunction>();libs/list-types/common/src/rendering/date-formatting.ts (1)
48-58: Consider defensive parsing for malformed date strings.
split("/")on an invalid input (e.g., empty string, missing parts) will produceNaNvalues, resulting in an Invalid Date. If upstream validation is guaranteed, this is acceptable; otherwise, consider early validation or a fallback.🛠️ Optional defensive implementation
export function formatDdMmYyyyDate(ddMMyyyyDate: string, locale: string): string { const [day, month, year] = ddMMyyyyDate.split("/"); + if (!day || !month || !year) { + throw new Error(`Invalid date format: ${ddMMyyyyDate}`); + } const date = new Date(Number.parseInt(year, 10), Number.parseInt(month, 10) - 1, Number.parseInt(day, 10)); + if (Number.isNaN(date.getTime())) { + throw new Error(`Invalid date: ${ddMMyyyyDate}`); + }libs/list-types/rcj-standard-daily-cause-list/src/rendering/renderer.test.ts (1)
6-75: Use SCREAMING_SNAKE_CASE for the constant test data.Rename
mockHearingsto align with the project constant naming rule.Proposed refactor
- const mockHearings: StandardHearingList = [ + const MOCK_HEARINGS: StandardHearingList = [ @@ - const result = renderStandardDailyCauseList(mockHearings, { + const result = renderStandardDailyCauseList(MOCK_HEARINGS, { @@ - const result = renderStandardDailyCauseList(mockHearings, { + const result = renderStandardDailyCauseList(MOCK_HEARINGS, { @@ - const result = renderStandardDailyCauseList(mockHearings, { + const result = renderStandardDailyCauseList(MOCK_HEARINGS, { @@ - const result = renderStandardDailyCauseList(mockHearings, { + const result = renderStandardDailyCauseList(MOCK_HEARINGS, {As per coding guidelines, ...
libs/list-types/london-administrative-court-daily-cause-list/src/conversion/london-administrative-court-daily-cause-list-config.ts (1)
14-17: Avoidas anycast without justification.The
as anycast bypasses type checking. Consider defining a proper type for the converter function or adding a comment explaining why the cast is necessary.💡 Suggested approach
If the type mismatch is due to
createMultiSheetConverterreturningPromise<Record<string, any[]>>whileregisterConverterexpects a different signature, consider:
- Adding a type assertion with a more specific type
- Creating a wrapper type that matches the expected interface
- At minimum, add a
//@ts-expect-error`` or comment explaining the type incompatibilitylibs/list-types/london-administrative-court-daily-cause-list/src/pages/london-administrative-court-daily-cause-list.njk (1)
28-34: Minor accessibility redundancy.The input has both a visually-hidden
<label>(line 30-32) and anaria-labelattribute (line 33). While not harmful, thearia-labelis redundant when a proper<label for="...">association exists.🔧 Suggested fix
<label class="govuk-label govuk-visually-hidden" for="case-search-input"> {{ t.searchCasesLabel }} </label> - <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-types/care-standards-tribunal-weekly-hearing-list/src/pages/index.ts (1)
54-55: Consider removing file path from error log.Logging the full file path (
jsonFilePath) may expose internal directory structure. Log a sanitised reference instead.- console.error(`Error reading JSON file at ${jsonFilePath}:`, error); + console.error(`Error reading JSON file for artefact ${artefactId}:`, error);libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/care-standards-tribunal-weekly-hearing-list.njk (1)
44-47: Redundant labelling on search input.The input has both a
<label>element (line 44-46) and anaria-labelattribute (line 47). The visually hidden label is sufficient; thearia-labelis redundant and may cause screen readers to announce different text if values diverge.- <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-types/court-of-appeal-civil-daily-cause-list/src/rendering/renderer.ts (2)
67-67: Consider extracting hardcoded Welsh title to translation files.The inline Welsh translation differs from other list-types that use separate
cy.ts/en.tslocale files. Centralising translations improves maintainability.- listTitle: options.locale === "cy" ? "Rhestr Achosion Dyddiol y Llys Apêl (Adran Sifil)" : "Court of Appeal (Civil Division) Daily Cause List", + listTitle: options.listTitle,Then pass
listTitleviaRenderOptionsfrom the page controller, sourced from the locale file.
4-34: Module ordering: consider moving interfaces to the bottom.Per coding guidelines, interfaces and types should be at the bottom of the module. Currently they precede the functions.
libs/list-types/administrative-court-daily-cause-list/src/pages/index.test.ts (1)
57-129: Prefer typed fixtures overas any.
as anyhides type regressions; consider importing the artefact type and usingsatisfiesor a typed fixture helper instead.libs/list-types/administrative-court-daily-cause-list/src/rendering/renderer.test.ts (1)
134-157: Test name and data don’t align.This case says “undefined” but uses an empty string and duplicates the earlier empty-info test. Consider removing it or making it truly undefined.
libs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/index.ts (1)
64-75: Consider distinguishing between file-not-found and other I/O errors.Currently, all
readFileerrors return 404. A permissions error or disk failure should likely return 500 instead, to aid debugging.libs/list-types/administrative-court-daily-cause-list/src/pages/administrative-court-daily-cause-list.njk (1)
29-35: Redundant labelling on search input.The input has both a
<label>(visually hidden) and anaria-label. Thearia-labeloverrides the label association for screen readers. Consider removing one to avoid duplication.♻️ Proposed fix
<div class="govuk-form-group search-container"> <h2 class="govuk-heading-s">{{ common.searchCasesTitle }}</h2> <label class="govuk-label govuk-visually-hidden" for="case-search-input"> {{ common.searchCasesLabel }} </label> - <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text" aria-label="{{ common.searchCasesLabel }}"> + <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text"> </div>libs/list-types/rcj-standard-daily-cause-list/src/pages/index.test.ts (1)
131-173: Consider usingit.eachfor parameterised list type tests.The loop-based approach works but
it.eachprovides better test naming and failure isolation.♻️ Suggested refactor
it.each([10, 11, 12, 13, 14, 15, 16, 17])( "should render the list successfully for listTypeId %i", async (listTypeId) => { // test body } );libs/list-types/court-of-appeal-civil-daily-cause-list/src/conversion/court-of-appeal-civil-daily-cause-list-config.ts (1)
70-81: Remove theas anycast by aligning converter typings.The cast erodes type safety and defeats strict TS. Please adjust the converter typing or add a typed adapter that matches
registerConverter’s expected signature. As per coding guidelines, avoidanywithout justification.libs/list-types/rcj-standard-daily-cause-list/src/pages/index.ts (1)
130-131: Removeanywhen accessing list content.Please type the translation map (e.g.,
Record<number, ListContent>) solistTypeIdindexing is type‑safe withoutas any. As per coding guidelines, avoidanywithout justification.libs/list-types/rcj-standard-daily-cause-list/src/rendering/renderer.ts (1)
4-31: Reorder declarations to match module ordering guidance.Interfaces/types should be at the bottom of the module, after exported functions. Please move
RenderOptions,RenderedHearing, andRenderedDatabeneathrenderStandardDailyCauseList. As per coding guidelines, keep interfaces and types at the bottom.libs/list-types/common/src/conversion/multi-sheet-converter.ts (2)
1-26: Replaceanywith ExcelJS types for stricter typing.
anyweakens safety and conflicts with strict TypeScript guidance. Please use ExcelJS types andunknown[](or a concrete row type) for the return/aggregate types. As per coding guidelines, avoidanywithout justification.♻️ Suggested typing update
-import ExcelJSPkg from "exceljs"; +import ExcelJSPkg, { type Worksheet, type Row, type Cell } from "exceljs"; import { convertExcelToJson, type ExcelConverterConfig } from "./excel-to-json.js"; export async function convertSheetToJson(worksheet: any, config: ExcelConverterConfig): Promise<any[]> { +export async function convertSheetToJson(worksheet: Worksheet, config: ExcelConverterConfig): Promise<unknown[]> { // Create a temporary buffer from the sheet const workbook = new Workbook(); const tempSheet = workbook.addWorksheet("temp"); // Copy all rows from source to temp worksheet - worksheet.eachRow((row: any, rowNumber: number) => { + worksheet.eachRow((row: Row, rowNumber: number) => { const newRow = tempSheet.getRow(rowNumber); - row.eachCell((cell: any, colNumber: number) => { + row.eachCell((cell: Cell, colNumber: number) => { newRow.getCell(colNumber).value = cell.value; }); newRow.commit(); }); -export async function createMultiSheetConverter(buffer: Buffer, sheets: SheetConfig[]): Promise<Record<string, any[]>> { +export async function createMultiSheetConverter(buffer: Buffer, sheets: SheetConfig[]): Promise<Record<string, unknown[]>> { const workbook = new Workbook(); // `@ts-expect-error` - ExcelJS types expect Node Buffer but accepts our Buffer type at runtime await workbook.xlsx.load(buffer); @@ - const result: Record<string, any[]> = {}; + const result: Record<string, unknown[]> = {};Also applies to: 54-65
28-37: MoveSheetConfigbelow exported functions.Keep interfaces/types at the bottom of the module for consistent ordering. As per coding guidelines, place interfaces and types after exported functions.
libs/list-types/administrative-court-daily-cause-list/src/pages/index.ts (2)
36-36: Add a POST handler to match page-controller conventionsPage controllers in this repo are expected to export both GET and POST handlers; consider adding a POST that returns 405 or delegates to GET as appropriate. Based on learnings, this aligns with established page-controller patterns.
122-124: Avoidanywhen indexing locale contentThe
anycast bypasses strict typing. Consider typing the locale dictionary and indexing via a typed key instead. As per coding guidelines, avoidanywithout justification.♻️ Suggested change
- const listContent = (t as any)[listTypeId] || {}; + type ListContent = typeof en[20]; + const listContent = (t as Record<number, ListContent>)[listTypeId] ?? {};
| test.describe("Administrative Court Daily Cause Lists - Viewing @nightly", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await authenticateSystemAdmin(page); | ||
| }); | ||
|
|
||
| test("should view Birmingham Administrative Court list with English and Welsh content", async ({ page }) => { | ||
| // Upload list | ||
| await uploadAdminCourtList(page, "20"); | ||
|
|
||
| // Navigate to summary of publications | ||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Find and click the publication link | ||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await expect(publicationLinks.first()).toBeVisible(); | ||
| const firstLinkHref = await publicationLinks.first().getAttribute("href"); | ||
| expect(firstLinkHref).toContain("/birmingham-administrative-court-daily-cause-list?artefactId="); | ||
|
|
||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Verify English content | ||
| await expect(page.locator("h1")).toContainText("Birmingham Administrative Court Daily Cause List"); | ||
| await expect(page.locator("body")).toContainText("List for 15 January 2026"); | ||
| await expect(page.locator("body")).toContainText("Last updated"); | ||
| await expect(page.locator("body")).toContainText("Court 1"); | ||
| await expect(page.locator("body")).toContainText("Mr Justice Williams"); | ||
| await expect(page.locator("body")).toContainText("R (Smith) v Secretary of State"); | ||
|
|
||
| // Verify time normalization (dot replaced with colon) | ||
| await expect(page.locator("tbody")).toContainText("10:00am"); | ||
| await expect(page.locator("tbody")).toContainText("2:30pm"); | ||
|
|
||
| // Test Welsh translation | ||
| await page.getByRole("link", { name: "Cymraeg" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
| await expect(page.locator("body")).toContainText("Rhestr ar gyfer 15 Ionawr 2026"); | ||
| await expect(page.locator("body")).toContainText("Diweddarwyd ddiwethaf"); | ||
| await expect(page.locator("body")).toContainText("Lleoliad"); | ||
|
|
||
| // Test accessibility | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .disableRules(["target-size", "link-name"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
|
|
||
| // Test table search functionality | ||
| const searchInput = page.locator('input[id="case-search-input"]'); | ||
| await expect(searchInput).toBeVisible(); | ||
| await searchInput.fill("Smith"); | ||
| await page.waitForTimeout(500); | ||
| await expect(page.locator("tbody tr:visible")).toHaveCount(1); | ||
| }); | ||
|
|
||
| test("should view Leeds Administrative Court list with proper formatting", async ({ page }) => { | ||
| await uploadAdminCourtList(page, "21"); | ||
|
|
||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Verify page loads correctly | ||
| await expect(page.locator("h1")).toContainText("Leeds Administrative Court Daily Cause List"); | ||
| await expect(page.locator("body")).toContainText("List for"); | ||
|
|
||
| // Verify table headers | ||
| await expect(page.locator("th")).toContainText("Venue"); | ||
| await expect(page.locator("th")).toContainText("Judge"); | ||
| await expect(page.locator("th")).toContainText("Time"); | ||
| await expect(page.locator("th")).toContainText("Case Number"); | ||
|
|
||
| // Test accessibility | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .disableRules(["target-size", "link-name"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
| }); | ||
|
|
||
| test("should view Manchester Administrative Court list and verify data source", async ({ page }) => { | ||
| await uploadAdminCourtList(page, "23"); | ||
|
|
||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Verify data source is shown | ||
| await expect(page.locator("body")).toContainText("Data source"); | ||
| await expect(page.locator("body")).toContainText("Manual Upload"); | ||
|
|
||
| // Test Welsh data source | ||
| await page.getByRole("link", { name: "Cymraeg" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
| await expect(page.locator("body")).toContainText("Ffynhonnell data"); | ||
| await expect(page.locator("body")).toContainText("Llwytho â Llaw"); | ||
| }); |
There was a problem hiding this comment.
Single‑journey E2E requirements are not met.
These tests don’t include validation checks or keyboard navigation within the same journey as Welsh and accessibility assertions. Please consolidate into one test flow (or add a dedicated single journey that covers all four). Based on learnings, include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey.
| test("should view Court of Appeal Civil list with daily hearings and future judgments sections", async ({ page }) => { | ||
| // Upload list | ||
| await uploadCourtOfAppealCivilList(page); | ||
|
|
||
| // Navigate to summary of publications | ||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Find and click the publication link | ||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await expect(publicationLinks.first()).toBeVisible(); | ||
| const firstLinkHref = await publicationLinks.first().getAttribute("href"); | ||
| expect(firstLinkHref).toContain("/court-of-appeal-civil-division-daily-cause-list?artefactId="); | ||
|
|
||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Verify English content | ||
| await expect(page.locator("h1")).toContainText("Court of Appeal (Civil Division) Daily Cause List"); | ||
| await expect(page.locator("body")).toContainText("List for 15 January 2026"); | ||
| await expect(page.locator("body")).toContainText("Last updated"); | ||
|
|
||
| // Verify daily hearings section | ||
| await expect(page.locator("body")).toContainText("Lord Justice Smith"); | ||
| await expect(page.locator("body")).toContainText("Appellant v Respondent"); | ||
| await expect(page.locator("body")).toContainText("CA-2025-000123"); | ||
|
|
||
| // Verify future judgments section | ||
| await expect(page.locator("h2")).toContainText("Future Judgments"); | ||
| await expect(page.locator("body")).toContainText("Lord Justice Williams"); | ||
| await expect(page.locator("body")).toContainText("Estate of Smith v Executor"); | ||
| await expect(page.locator("body")).toContainText("20 January 2026"); | ||
|
|
||
| // Verify future judgments table has Date column | ||
| const futureJudgmentsTable = page.locator("#future-judgments-table-container table"); | ||
| await expect(futureJudgmentsTable.locator("th").first()).toContainText("Date"); | ||
|
|
||
| // Verify time normalization (dot to colon) | ||
| await expect(page.locator("tbody")).toContainText("10:30am"); | ||
| await expect(page.locator("tbody")).toContainText("2:00pm"); | ||
|
|
||
| // Test Welsh translation | ||
| await page.getByRole("link", { name: "Cymraeg" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
| await expect(page.locator("h1")).toContainText("Rhestr Achosion Dyddiol y Llys Apêl (Adran Sifil)"); | ||
| await expect(page.locator("body")).toContainText("Rhestr ar gyfer 15 Ionawr 2026"); | ||
| await expect(page.locator("body")).toContainText("Diweddarwyd ddiwethaf"); | ||
|
|
||
| // Future Judgments section in Welsh | ||
| await expect(page.locator("h2")).toContainText("Dyfarniadau yn y Dyfodol"); | ||
|
|
||
| // Test accessibility | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .disableRules(["target-size", "link-name"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
|
|
||
| // Test table search across both sections | ||
| const searchInput = page.locator('input[id="case-search-input"]'); | ||
| await expect(searchInput).toBeVisible(); | ||
|
|
||
| // Switch back to English for search test | ||
| await page.getByRole("link", { name: "English" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| await searchInput.fill("Estate"); | ||
| await page.waitForTimeout(500); | ||
| // Should show only the future judgment row | ||
| await expect(page.locator("tbody tr:visible")).toHaveCount(1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Single test journey is missing required validation + keyboard checks.
Welsh and accessibility checks are in the main test, but keyboard navigation sits in a separate test and there’s no validation step. Please fold the keyboard/back‑to‑top assertions into the main viewing journey and add a small validation check (e.g., submit without a file and assert the error summary) within the same flow. Based on learnings, E2E journeys must include validation, Welsh translation, accessibility, and keyboard navigation checks in one test.
Also applies to: 249-274
| test.describe("London Administrative Court Daily Cause List - Viewing @nightly", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await authenticateSystemAdmin(page); | ||
| }); | ||
|
|
||
| test("should view London Administrative Court list with main hearings and planning court sections", async ({ page }) => { | ||
| // Upload list | ||
| await uploadLondonAdminCourtList(page); | ||
|
|
||
| // Navigate to summary of publications | ||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Find and click the publication link | ||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await expect(publicationLinks.first()).toBeVisible(); | ||
| const firstLinkHref = await publicationLinks.first().getAttribute("href"); | ||
| expect(firstLinkHref).toContain("/london-administrative-court-daily-cause-list?artefactId="); | ||
|
|
||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Verify English content | ||
| await expect(page.locator("h1")).toContainText("London Administrative Court Daily Cause List"); | ||
| await expect(page.locator("body")).toContainText("List for 15 January 2026"); | ||
| await expect(page.locator("body")).toContainText("Last updated"); | ||
|
|
||
| // Verify main hearings section | ||
| await expect(page.locator("body")).toContainText("R (Brown) v Home Secretary"); | ||
| await expect(page.locator("body")).toContainText("Mr Justice Roberts"); | ||
|
|
||
| // Verify planning court section | ||
| await expect(page.locator("h2")).toContainText("Planning Court"); | ||
| await expect(page.locator("body")).toContainText("R (Developer Ltd) v Council"); | ||
| await expect(page.locator("body")).toContainText("Mr Justice Black"); | ||
|
|
||
| // Verify time normalization | ||
| await expect(page.locator("tbody")).toContainText("10:00am"); | ||
| await expect(page.locator("tbody")).toContainText("2:30pm"); | ||
|
|
||
| // Test Welsh translation | ||
| await page.getByRole("link", { name: "Cymraeg" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
| await expect(page.locator("h1")).toContainText("Rhestr Achosion Dyddiol Llys Gweinyddol Llundain"); | ||
| await expect(page.locator("body")).toContainText("Rhestr ar gyfer 15 Ionawr 2026"); | ||
| await expect(page.locator("body")).toContainText("Diweddarwyd ddiwethaf"); | ||
| await expect(page.locator("h2")).toContainText("Llys Cynllunio"); | ||
|
|
||
| // Test accessibility | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .disableRules(["target-size", "link-name"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
|
|
||
| // Test table search across both sections | ||
| const searchInput = page.locator('input[id="case-search-input"]'); | ||
| await expect(searchInput).toBeVisible(); | ||
|
|
||
| // Switch back to English for search test | ||
| await page.getByRole("link", { name: "English" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| await searchInput.fill("Developer"); | ||
| await page.waitForTimeout(500); | ||
| // Should show only the planning court row | ||
| await expect(page.locator("tbody tr:visible")).toHaveCount(1); | ||
| }); | ||
|
|
||
| test("should display important information section correctly", async ({ page }) => { | ||
| await uploadLondonAdminCourtList(page); | ||
|
|
||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Check for important information section | ||
| const importantInfoDetails = page.locator(".govuk-details"); | ||
| await expect(importantInfoDetails).toBeVisible(); | ||
| await expect(importantInfoDetails.locator(".govuk-details__summary-text")).toContainText("Important information"); | ||
|
|
||
| // Expand the details section | ||
| await importantInfoDetails.locator(".govuk-details__summary").click(); | ||
| await expect(importantInfoDetails).toContainText("Hearings take place in public"); | ||
|
|
||
| // Check for judgments section | ||
| await expect(importantInfoDetails).toContainText("Judgments"); | ||
| }); | ||
|
|
||
| test("should verify keyboard navigation works correctly", async ({ page }) => { | ||
| await uploadLondonAdminCourtList(page); | ||
|
|
||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Test keyboard navigation to search input | ||
| await page.keyboard.press("Tab"); | ||
| await page.keyboard.press("Tab"); | ||
| const searchInput = page.locator('input[id="case-search-input"]'); | ||
| await expect(searchInput).toBeFocused(); | ||
|
|
||
| // Test keyboard navigation to table | ||
| await page.keyboard.press("Tab"); | ||
| const focusedElement = page.locator(":focus"); | ||
| await expect(focusedElement).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Single-journey E2E requirements are not met.
The suite splits Welsh/accessibility and keyboard navigation across tests and does not include any validation checks. Please consolidate these into one end‑to‑end journey (or add a single test that includes all four elements). Based on learnings, include validation checks, Welsh translation checks, accessibility checks, and keyboard navigation within a single E2E test journey.
| test("should view Civil Courts at RCJ list with English and Welsh content", async ({ page }) => { | ||
| // Upload list | ||
| await uploadRCJList(page, "10"); | ||
|
|
||
| // Navigate to summary of publications | ||
| await page.goto("/summary-of-publications?locationId=9001"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Find and click the publication link | ||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| await expect(publicationLinks.first()).toBeVisible(); | ||
| const firstLinkHref = await publicationLinks.first().getAttribute("href"); | ||
| expect(firstLinkHref).toContain("/civil-courts-rcj-daily-cause-list?artefactId="); | ||
|
|
||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Verify English content | ||
| await expect(page.locator("h1")).toContainText("Civil Courts at the Royal Courts of Justice Daily Cause List"); | ||
| await expect(page.locator("body")).toContainText("List for 15 January 2026"); | ||
| await expect(page.locator("body")).toContainText("Last updated"); | ||
| await expect(page.locator("body")).toContainText("Court 1"); | ||
| await expect(page.locator("body")).toContainText("Mr Justice Smith"); | ||
| await expect(page.locator("body")).toContainText("R v Jones"); | ||
|
|
||
| // Test Welsh translation | ||
| await page.getByRole("link", { name: "Cymraeg" }).click(); | ||
| await page.waitForLoadState("networkidle"); | ||
| await expect(page.locator("body")).toContainText("Rhestr ar gyfer 15 Ionawr 2026"); | ||
| await expect(page.locator("body")).toContainText("Diweddarwyd ddiwethaf"); | ||
| await expect(page.locator("body")).toContainText("Lleoliad"); | ||
|
|
||
| // Test accessibility | ||
| const accessibilityScanResults = await new AxeBuilder({ page }) | ||
| .disableRules(["target-size", "link-name"]) | ||
| .analyze(); | ||
| expect(accessibilityScanResults.violations).toEqual([]); | ||
|
|
||
| // Test table search functionality | ||
| const searchInput = page.locator('input[id="case-search-input"]'); | ||
| await expect(searchInput).toBeVisible(); | ||
| await searchInput.fill("Jones"); | ||
| await page.waitForTimeout(500); | ||
| await expect(page.locator("tbody tr:visible")).toHaveCount(1); | ||
|
|
||
| // Test back to top link | ||
| const backToTopLink = page.getByRole("link", { name: /Yn ôl i frig y dudalen/i }); | ||
| await expect(backToTopLink).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Add validation + keyboard navigation checks in the same journey.
This flow includes Welsh and accessibility checks but does not assert form validation or keyboard navigation within the same journey. Please add a lightweight validation check (e.g., submit with a required field missing) and a Tab/focus-order assertion in this test. Based on learnings, include validation and keyboard navigation in the same E2E journey.
| const jsonFilePath = path.join(TEMP_UPLOAD_DIR, `${artefactId}.json`); | ||
|
|
||
| let jsonContent: string; | ||
| try { | ||
| jsonContent = await readFile(jsonFilePath, "utf-8"); | ||
| } catch (error) { | ||
| console.error(`Error reading JSON file at ${jsonFilePath}:`, error); | ||
| return res.status(404).render("errors/common", { | ||
| en, | ||
| cy, | ||
| errorTitle: "Not Found", | ||
| errorMessage: "The requested list could not be found" | ||
| }); | ||
| } |
There was a problem hiding this comment.
Prevent path traversal when building the JSON file path.
artefactId is interpolated into a filesystem path without sanitisation. A crafted value can escape TEMP_UPLOAD_DIR. Please validate the ID and/or enforce the resolved path is under the intended directory.
🔒 Suggested hardening
const TEMP_UPLOAD_DIR = path.join(MONOREPO_ROOT, "storage", "temp", "uploads");
+const ARTEFACT_ID_PATTERN = /^[0-9a-f-]+$/i;
const schemaPath = path.join(__dirname, "../schemas/rcj-standard-daily-cause-list.json");
const validate = createJsonValidator(schemaPath);
@@
if (!artefactId) {
return res.status(400).render("errors/common", {
@@
});
}
+
+ if (!ARTEFACT_ID_PATTERN.test(artefactId)) {
+ return res.status(400).render("errors/common", {
+ en,
+ cy,
+ errorTitle: "Bad Request",
+ errorMessage: "Invalid artefactId parameter"
+ });
+ }
@@
- const jsonFilePath = path.join(TEMP_UPLOAD_DIR, `${artefactId}.json`);
+ const jsonFilePath = path.join(TEMP_UPLOAD_DIR, `${artefactId}.json`);
+ const resolvedJsonFilePath = path.resolve(jsonFilePath);
+ if (!resolvedJsonFilePath.startsWith(`${TEMP_UPLOAD_DIR}${path.sep}`)) {
+ return res.status(400).render("errors/common", {
+ en,
+ cy,
+ errorTitle: "Bad Request",
+ errorMessage: "Invalid artefactId parameter"
+ });
+ }| const validationResult = validate(jsonData); | ||
| if (!validationResult.isValid) { | ||
| console.error("Validation errors:", validationResult.errors); | ||
| return res.status(400).render("errors/common", { | ||
| en, | ||
| cy, | ||
| errorTitle: "Invalid Data", | ||
| errorMessage: "The list data is invalid" | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid logging potentially sensitive validation details.
validationResult.errors may include user data; logging it risks PII exposure. Log only a summary/count instead. As per coding guidelines, avoid logging sensitive data.
🧹 Safer logging
- if (!validationResult.isValid) {
- console.error("Validation errors:", validationResult.errors);
+ if (!validationResult.isValid) {
+ console.error("Validation failed for RCJ Standard Daily Cause List", {
+ errorCount: validationResult.errors?.length ?? 0
+ });| @@ -0,0 +1,72 @@ | |||
| {% extends "layouts/base-template.njk" %} | |||
There was a problem hiding this comment.
Use the required base template name.
Line 1 extends layouts/base-template.njk, but the project requirement for templates under libs/*/src/pages/**/*.njk is layouts/base-templates.njk. If the singular template doesn’t exist, rendering will fail; even if it does, this breaks consistency.
💡 Suggested change
-{% extends "layouts/base-template.njk" %}
+{% extends "layouts/base-templates.njk" %}As per learnings, templates must extend layouts/base-templates.njk.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% extends "layouts/base-template.njk" %} | |
| {% extends "layouts/base-templates.njk" %} |
| @@ -0,0 +1,72 @@ | |||
| {% extends "layouts/base-template.njk" %} | |||
There was a problem hiding this comment.
Align base layout with required layouts/base-templates.njk.
This file extends layouts/base-template.njk, but templates in this path are required to extend layouts/base-templates.njk. That mismatch can break layout rendering or standard components.
💡 Proposed fix
-{% extends "layouts/base-template.njk" %}
+{% extends "layouts/base-templates.njk" %}Based on learnings, this template must extend layouts/base-templates.njk.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% extends "layouts/base-template.njk" %} | |
| {% extends "layouts/base-templates.njk" %} |
| @@ -0,0 +1,72 @@ | |||
| {% extends "layouts/base-template.njk" %} | |||
There was a problem hiding this comment.
Base template name does not match required layout.
The template should extend layouts/base-templates.njk per project guidance; the current name is different and may break rendering. Based on learnings, please align the base template reference.
Suggested fix
-{% extends "layouts/base-template.njk" %}
+{% extends "layouts/base-templates.njk" %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% extends "layouts/base-template.njk" %} | |
| {% extends "layouts/base-templates.njk" %} |
🎭 Playwright E2E Test Results207 tests 207 ✅ 20m 9s ⏱️ Results for commit 1c30ae5. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (2)
265-278: Bug: Hardcoded artefactId renders test ineffective.Line 267 calls
completeCSTUploadFlowbut discards the returnedartefactId. Line 272 then navigates to?artefactId=test, which won't find the actual published list.🐛 Proposed fix
test("should display the published CST list with correct formatting", async ({ page }) => { // Upload and publish the list - await completeCSTUploadFlow(page); + const artefactId = await completeCSTUploadFlow(page); // Navigate to the published list - // Note: In a real scenario, you'd navigate via search or direct URL - // For now, we'll construct the URL pattern - await page.goto("/care-standards-tribunal-weekly-hearing-list?artefactId=test"); - await page.waitForTimeout(1000); + await navigateToPublishedList(page, artefactId); // Verify page loads (may need adjustment based on actual implementation) const heading = page.locator("h1"); await expect(heading).toBeVisible(); });
545-550: Add explanatory comment for disabled accessibility rules.Disabling
target-sizeandlink-namerules is a widespread pattern across multiple E2E tests. Other test files document this reasoning with comments (e.g. "Known GOV.UK Design System footer issues"). Add a similar comment to this file to explain why these specific rules are disabled, so the decision is transparent and maintainable.
🧹 Nitpick comments (6)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (6)
165-197: AvoidwaitForTimeout- prefer explicit waits.
waitForTimeout(1000)on line 167 is flaky. UsewaitForLoadState('networkidle')or wait for a specific element that indicates page readiness.♻️ Suggested improvement
async function uploadCSTExcel(page: Page, excelBuffer: Buffer, expectSuccess = true) { await page.goto("/non-strategic-upload?locationId=9001"); - await page.waitForTimeout(1000); + await page.waitForLoadState("networkidle"); // Select Care Standards Tribunal (listTypeId === 9)
199-227: ReplacewaitForTimeoutcalls with explicit waits.Lines 209 and 226 use arbitrary timeouts. This pattern can cause flaky tests across different environments.
♻️ Suggested improvement
// Navigate to summary of publications to find the artefactId await page.goto("/summary-of-publications"); - await page.waitForTimeout(1000); + await page.waitForLoadState("networkidle"); // Find the first CST publication link const publicationLinks = page.locator('.govuk-list a[href*="care-standards-tribunal-weekly-hearing-list?artefactId="]');async function navigateToPublishedList(page: Page, artefactId: string) { await page.goto(`/care-standards-tribunal-weekly-hearing-list?artefactId=${artefactId}`); - await page.waitForTimeout(1000); + await page.waitForLoadState("networkidle"); }
356-371: Consider usinggetByTextfor cleaner assertions.The loop iterating through paragraphs to find "Last updated" text is verbose. Playwright's
getByTextwith a regex would be more idiomatic per coding guidelines.♻️ Suggested improvement
- // Verify "Last updated" date and time line - const bodyParagraphs = page.locator(".govuk-body"); - let foundLastUpdated = false; - for (let i = 0; i < await bodyParagraphs.count(); i++) { - const text = await bodyParagraphs.nth(i).textContent(); - if (text?.includes("Last updated")) { - foundLastUpdated = true; - expect(text).toMatch(/Last updated \d{1,2} \w+ \d{4} at \d{1,2}(:\d{2})?(am|pm)/); - break; - } - } - expect(foundLastUpdated).toBeTruthy(); + // Verify "Last updated" date and time line + const lastUpdated = page.getByText(/Last updated \d{1,2} \w+ \d{4} at \d{1,2}(:\d{2})?(am|pm)/); + await expect(lastUpdated).toBeVisible();
491-504: PrefergetByLabelorgetByRolefor the search input.Per coding guidelines, selector priority should be:
getByRole(),getByLabel(),getByText(),getByTestId(). Using#case-search-inputis less accessible-friendly.♻️ Suggested improvement
- const searchInput = page.locator("#case-search-input"); + const searchInput = page.getByRole("textbox", { name: /search/i }); await expect(searchInput).toBeVisible();If the input lacks an accessible name, consider adding a label to the component.
578-598: Keyboard navigation test could be more thorough.The test only tabs through 5 elements. Consider verifying that key interactive elements (form inputs, buttons, links) are reachable and that focus indicators are visible.
237-239: PreferwaitForLoadStateoverwaitForTimeout.Line 238 uses
waitForTimeout(1000). This pattern appears throughout the file and should be replaced with deterministic waits.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
🧰 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/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/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/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/care-standards-tribunal-upload.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
e2e-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/care-standards-tribunal-upload.spec.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
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.
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Tag nightly-only E2E tests with `nightly` in the test title
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
600-653: Comprehensive Welsh translation coverage - LGTM.The test verifies Welsh translations for page title, "week commencing", "last updated", "Important information", all table headers, data source, "Back to top", and search label. Based on learnings, Welsh translations are required for all user-facing text.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
262-275: Bug: Test ignores returned artefactId and uses hardcoded value.
completeCSTUploadFlowreturns the actualartefactId, but line 269 navigates to?artefactId=test. This test will not validate the published list content correctly.Proposed fix
test("should display the published CST list with correct formatting", async ({ page }) => { // Upload and publish the list - await completeCSTUploadFlow(page); + const artefactId = await completeCSTUploadFlow(page); // Navigate to the published list - // Note: In a real scenario, you'd navigate via search or direct URL - // For now, we'll construct the URL pattern - await page.goto("/care-standards-tribunal-weekly-hearing-list?artefactId=test"); - await page.waitForTimeout(1000); + await navigateToPublishedList(page, artefactId); // Verify page loads (may need adjustment based on actual implementation) const heading = page.locator("h1"); await expect(heading).toBeVisible(); });
🧹 Nitpick comments (6)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (6)
166-167: AvoidwaitForTimeout— prefer explicit wait conditions.Hard-coded timeouts are flaky. Use
waitForLoadState('networkidle'),waitForSelector, or Playwright's auto-waiting assertions instead.Example replacement
- await page.goto("/non-strategic-upload?locationId=9001"); - await page.waitForTimeout(1000); + await page.goto("/non-strategic-upload?locationId=9001"); + await page.waitForLoadState("networkidle");- await page.goto(`/care-standards-tribunal-weekly-hearing-list?artefactId=${artefactId}`); - await page.waitForTimeout(1000); + await page.goto(`/care-standards-tribunal-weekly-hearing-list?artefactId=${artefactId}`); + await page.waitForLoadState("networkidle");Also applies to: 223-224, 464-465, 475-476, 493-494, 510-511, 524-525, 528-529, 601-601
13-16: Non-null assertions on environment variables risk unclear runtime errors.If
SSO_TEST_SYSTEM_ADMIN_EMAILorSSO_TEST_SYSTEM_ADMIN_PASSWORDis undefined, the test will fail with an obscure error. Consider validating upfront or throwing a descriptive error.Proposed improvement
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); }
166-166: Extract magic numbers into named constants.Values like
"9001"(locationId) and"9"(listTypeId) appear multiple times. Per coding guidelines, useSCREAMING_SNAKE_CASEconstants.Proposed improvement
const CST_LOCATION_ID = "9001"; const CST_LIST_TYPE_ID = "9";Then reference these throughout:
- await page.goto("/non-strategic-upload?locationId=9001"); + await page.goto(`/non-strategic-upload?locationId=${CST_LOCATION_ID}`); ... - await page.selectOption('select[name="listType"]', "9"); + await page.selectOption('select[name="listType"]', CST_LIST_TYPE_ID);Also applies to: 169-170
353-356: Prefer semantic locators over escaped CSS class selectors.
".govuk-body.govuk-\\!-font-weight-bold"is brittle. Consider usinggetByText()or a more semantic approach aligned with selector priority guidelines.Alternative approach
// Instead of relying on class escaping: const weekCommencingText = await page.getByText(/List for week commencing/i).textContent();Also applies to: 364-365
358-368: Simplify "Last updated" verification with a targeted locator.Iterating over all
.govuk-bodyparagraphs is fragile. Use a more direct approach.Proposed simplification
const lastUpdatedText = page.getByText(/Last updated \d{1,2} \w+ \d{4} at \d{1,2}/); await expect(lastUpdatedText).toBeVisible();
542-545: Document whytarget-sizeandlink-namerules are disabled.Disabling axe-core rules without explanation may mask genuine accessibility issues. Add a comment explaining the rationale.
Also applies to: 554-557, 566-569
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
🧰 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/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/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/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/care-standards-tribunal-upload.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
e2e-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/care-standards-tribunal-upload.spec.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Tag nightly-only E2E tests with `nightly` in the test title
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
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
⏰ 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/care-standards-tribunal-upload.spec.ts (2)
597-650: Good coverage of Welsh translations.The Welsh language test comprehensively verifies page title, date labels, accordion summary, table headers, data source, back-to-top link, and search title. This aligns well with the requirement that Welsh translations are required for all user-facing text. Based on learnings, this coverage is essential.
536-572: Accessibility checks using axe-core are properly implemented.WCAG 2.2 AA compliance checks are included for upload form, error page, and list display page as required by coding guidelines.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
349-374: Prefer role/text selectors over CSS locators.To align with the selector priority and improve resilience, use
getByRole/getByText(orlocator.filter({ hasText })) instead of CSS selectors and manual loops for the heading and “Last updated” checks. As per coding guidelines, please prefer the selector priority order.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
🧰 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/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/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/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/care-standards-tribunal-upload.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
e2e-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/care-standards-tribunal-upload.spec.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Tag nightly-only E2E tests with `nightly` in the test title
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
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
⏰ 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 (10)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (10)
220-227: Navigation helper looks solid.Waiting for the main heading after navigation is a sensible sync point.
376-404: Accordion default-open checks look good.The open-attribute check plus content assertions give solid coverage.
407-434: Table content assertions are clear.Good coverage of headers and representative row data.
436-447: English data source assertion is fine.Nice, concise validation of source text.
449-461: Welsh data source assertion is fine.Clear check for the translated data source text.
464-485: Back‑to‑top behaviour is well covered.The scroll, link, and position assertions are appropriate.
489-507: Search highlight coverage looks good.Confirms highlighting without hiding rows, which matches the intended behaviour.
509-521: Multi‑column match check is OK.Good to see coverage outside the case name column.
523-538: Clear‑search behaviour is covered.The reset path is properly exercised.
603-657: Welsh content assertions are comprehensive.This is a strong set of checks for translated labels and content.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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([]); | ||
| const artefactId = await completeCSTUploadFlow(page); | ||
| await navigateToPublishedList(page, artefactId); | ||
|
|
||
| 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.
🧩 Analysis chain
🏁 Script executed:
wc -l e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 120
🏁 Script executed:
cat -n e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 31594
Consolidate validation, Welsh, accessibility, and keyboard navigation into a single E2E test journey.
The E2E guidance requires these checks within one journey, but they are currently split across separate describe blocks. Create a unified test that:
- Completes the CST upload flow (capturing validation)
- Runs Axe accessibility scan (WCAG 2.2 AA)
- Verifies keyboard navigation through the journey
- Confirms Welsh language support throughout
This consolidation ensures all compliance checks are exercised in a realistic user workflow rather than in isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
302-315: Use the real artefactId instead of the hard-coded “test”.This test discards the returned artefactId and navigates to a fixed URL, which will fail unless a matching artefact exists. Capture the ID and reuse the helper for navigation.
Suggested fix
- await completeCSTUploadFlow(page); - - // Navigate to the published list - // Note: In a real scenario, you'd navigate via search or direct URL - // For now, we'll construct the URL pattern - await page.goto("/care-standards-tribunal-weekly-hearing-list?artefactId=test"); - await page.waitForTimeout(1000); + const artefactId = await completeCSTUploadFlow(page); + await navigateToPublishedList(page, artefactId);
♻️ Duplicate comments (2)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (2)
318-695: E2E compliance checks are split across multiple journeys.Guidance requires validation, Welsh translation, accessibility, and keyboard navigation checks in a single end‑to‑end journey, but they are currently separated. Please consolidate into one journey to meet the requirement. Based on learnings, please unify these checks in one test flow.
199-223: ArtefactId derivation still looks incorrect.The helper returns
uploadIdasartefactId, but the confirm step typically creates a new artefact. This risks navigating to a non-existent list. Please extract the actual artefactId from the confirm response or created record instead of reusing uploadId.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
🧰 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/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/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/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/care-standards-tribunal-upload.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
e2e-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/care-standards-tribunal-upload.spec.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Tag nightly-only E2E tests with `nightly` in the test title
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
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
⏰ 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
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
267-331: Combine validation, Welsh, accessibility and keyboard checks into one journey.These checks are still split across separate describes. The E2E guidance requires them to run within a single user journey (e.g., invalid upload → corrected upload → list page → Axe scan → Welsh switch → keyboard navigation). Based on learnings, please consolidate these checks into one flow.
Also applies to: 521-579, 581-639
🧹 Nitpick comments (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts (1)
471-479: Prefer role/label-based selectors for the search input.Use Playwright’s role/label locators for the search input rather than
#case-search-inputto align with the selector priority guideline and improve resilience. As per coding guidelines, please prefer getByRole/getByLabel where possible.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
🧰 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/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/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/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/care-standards-tribunal-upload.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Input validation must be performed on all endpoints
Files:
e2e-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/care-standards-tribunal-upload.spec.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Applies to e2e-tests/**/*.spec.ts : Tag nightly-only E2E tests with `nightly` in the test title
Applied to files:
e2e-tests/tests/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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/care-standards-tribunal-upload.spec.ts
📚 Learning: 2025-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
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-12-15T13:38:57.497Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T13:38:57.497Z
Learning: Welsh translations are required for all user-facing text
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
⏰ 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
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Navigate to summary of publications to find and click the publication link | ||
| await page.goto("/summary-of-publications"); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Find the first (most recent) CST publication link | ||
| const publicationLinks = page.locator('.govuk-list a[href*="care-standards-tribunal-weekly-hearing-list?artefactId="]'); | ||
| await expect(publicationLinks.first()).toBeVisible(); | ||
|
|
||
| // Click the publication link to navigate to the list page | ||
| await publicationLinks.first().click(); | ||
| await page.waitForLoadState("networkidle"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 120
🏁 Script executed:
sed -n '190,230p' e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 1825
🏁 Script executed:
sed -n '1,50p' e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 1730
🏁 Script executed:
sed -n '100,190p' e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 3321
🏁 Script executed:
grep -n "artefactId" e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 188
🏁 Script executed:
grep -n "uploadCSTExcel\|completeCSTUploadFlowAndNavigate" e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 1290
🏁 Script executed:
sed -n '200,220p' e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 994
🏁 Script executed:
grep -n "summary-of-publications\|getByRole\|getByLabel\|getByText" e2e-tests/tests/care-standards-tribunal-upload.spec.ts | head -20Repository: hmcts/cath-service
Length of output: 829
🏁 Script executed:
grep -B5 -A10 "await page.waitForURL.*non-strategic-upload-success" e2e-tests/tests/care-standards-tribunal-upload.spec.tsRepository: hmcts/cath-service
Length of output: 1611
Use deterministic selector and follow accessibility guidelines for publication link.
The .first() selection assumes a clean test environment and stable ordering. If prior publications exist or the database is not cleaned between runs, the test selects the wrong publication. Instead, capture the artefactId from the upload response and use it to match the specific publication created in this run. Additionally, the current CSS selector violates E2E guidelines—use getByRole() or getByLabel() if the link has accessible text or ARIA attributes that identify it.
…BE-323 # Conflicts: # libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/index.ts
… feature/VIBE-323
|
2 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-323
Change description
Add Welsh translations for Care Standards Tribunal Weekly Hearing List
Checklist
Summary by CodeRabbit
New Features
Improvements
Chores