Feature/771 Add Magistrates Standard List style guide and PDF - #788
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the ChangesMagistrates Standard List
|Possibly related PRs|
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright E2E Test Results84 tests 52 ✅ 6m 13s ⏱️ Results for commit d8c348c. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
libs/list-types/magistrates-standard-list/src/index.ts (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReorder this barrel to match the repo export rule.
The type export sits above functional exports, and the locale constants are not first. Small change, but worth fixing before this barrel becomes the pattern for the package.
As per coding guidelines,
**/*.ts: Order module exports: top-level constants first, then exported functions, then other functions ordered by usage, with interfaces and types at the bottom.Suggested fix
-export * from "./email-summary/summary-builder.js"; export { cy as magistratesStandardListCy } from "./locales/cy.js"; export { en as magistratesStandardListEn } from "./locales/en.js"; -export * from "./models/types.js"; +export * from "./email-summary/summary-builder.js"; export * from "./pdf/pdf-generator.js"; export * from "./rendering/renderer.js"; export { validateMagistratesStandardList } from "./validation/json-validator.js"; +export * from "./models/types.js";Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44c72578-2ff4-4451-8284-5a12f03206ad
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
apps/web/package.jsonapps/web/src/app.tsapps/web/src/pages/(list-types)/magistrates-standard-list/index.test.tsapps/web/src/pages/(list-types)/magistrates-standard-list/index.tsapps/web/src/pages/(list-types)/magistrates-standard-list/magistrates-standard-list.njkdocs/tickets/771/plan.mddocs/tickets/771/tasks.mddocs/tickets/771/ticket.mdlibs/list-types/magistrates-standard-list/package.jsonlibs/list-types/magistrates-standard-list/src/config.tslibs/list-types/magistrates-standard-list/src/email-summary/summary-builder.test.tslibs/list-types/magistrates-standard-list/src/email-summary/summary-builder.tslibs/list-types/magistrates-standard-list/src/index.tslibs/list-types/magistrates-standard-list/src/locales/cy.tslibs/list-types/magistrates-standard-list/src/locales/en.tslibs/list-types/magistrates-standard-list/src/models/types.tslibs/list-types/magistrates-standard-list/src/pdf/pdf-generator.tslibs/list-types/magistrates-standard-list/src/pdf/pdf-template.njklibs/list-types/magistrates-standard-list/src/rendering/renderer.test.tslibs/list-types/magistrates-standard-list/src/rendering/renderer.tslibs/list-types/magistrates-standard-list/src/schemas/magistrates-standard-list.jsonlibs/list-types/magistrates-standard-list/src/validation/json-validator.tslibs/list-types/magistrates-standard-list/tsconfig.jsonlibs/location/src/list-type-data.tslibs/notifications/src/notification/notification-service.tslibs/publication/src/processing/service.tstsconfig.json
| <div class="govuk-warning-text"> | ||
| <span class="govuk-warning-text__icon" aria-hidden="true">!</span> | ||
| <strong class="govuk-warning-text__text"> | ||
| <span class="govuk-visually-hidden">Warning</span> | ||
| {{ t.restrictionInformationBoldText }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localise the warning text announced to assistive technology.
The visually hidden text is hardcoded to English, so Welsh pages will still announce “Warning” in English. Move this into the locale files and render it via t.
As per coding guidelines, "Implement Welsh language support on all user-facing text with separate en.ts and cy.ts content files."
Source: Coding guidelines
| <div class="govuk-form-group govuk-!-margin-top-4"> | ||
| <h2 class="govuk-heading-m">{{ t.searchCases }}</h2> | ||
| <input class="govuk-input govuk-!-width-one-half" id="case-search-input" name="search" type="text"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Give the search input an accessible name.
This input has no associated label, so screen-reader users get an unlabelled text box. Add a <label for="case-search-input"> or reference the existing heading with aria-labelledby.
| "dependencies": { | ||
| "@hmcts/location": "workspace:*" | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Declare @hmcts/publication here as well.
src/validation/json-validator.ts imports validateJson from @hmcts/publication, but this package does not declare that dependency. Right now resolution depends on workspace hoisting rather than this package’s own contract.
Suggested fix
"dependencies": {
- "`@hmcts/location`": "workspace:*"
+ "`@hmcts/location`": "workspace:*",
+ "`@hmcts/publication`": "workspace:*"
},📝 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.
| "dependencies": { | |
| "@hmcts/location": "workspace:*" | |
| }, | |
| "dependencies": { | |
| "`@hmcts/location`": "workspace:*", | |
| "`@hmcts/publication`": "workspace:*" | |
| }, |
| export const moduleRoot = __dirname; | ||
| export const schemaPath = path.join(__dirname, "schemas/magistrates-standard-list.json"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename these exported constants before the API spreads.
moduleRoot and schemaPath are new public constants, so this is the cheapest point to align them with the repo naming rule.
As per coding guidelines, **/*.{ts,tsx,js}: Use SCREAMING_SNAKE_CASE for constant declarations (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT).
Suggested fix
-export const moduleRoot = __dirname;
-export const schemaPath = path.join(__dirname, "schemas/magistrates-standard-list.json");
+export const MODULE_ROOT = __dirname;
+export const SCHEMA_PATH = path.join(__dirname, "schemas/magistrates-standard-list.json");📝 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.
| export const moduleRoot = __dirname; | |
| export const schemaPath = path.join(__dirname, "schemas/magistrates-standard-list.json"); | |
| export const MODULE_ROOT = __dirname; | |
| export const SCHEMA_PATH = path.join(__dirname, "schemas/magistrates-standard-list.json"); |
Source: Coding guidelines
| for (const caseItem of hearing.case ?? []) { | ||
| const defendant = caseItem.party?.find((p) => p.partyRole === "DEFENDANT"); | ||
| const prosecutor = caseItem.party?.find((p) => p.partyRole === "PROSECUTING_AUTHORITY"); | ||
| const fields: CaseSummary = []; | ||
|
|
||
| if (defendant) { | ||
| const name = extractPartyName(defendant); | ||
| if (name) fields.push({ label: "Name", value: name }); | ||
| } | ||
|
|
||
| if (prosecutor) { | ||
| const authority = extractPartyName(prosecutor); | ||
| if (authority) fields.push({ label: "Prosecuting authority", value: authority }); | ||
| } | ||
|
|
||
| fields.push({ label: "Reference", value: caseItem.caseUrn }); | ||
|
|
||
| if (hearing.hearingType) { | ||
| fields.push({ label: "Hearing type", value: hearing.hearingType }); | ||
| } | ||
|
|
||
| if (defendant) { | ||
| const offences = extractOffenceTitles(defendant); | ||
| if (offences) fields.push({ label: "Offence", value: offences }); | ||
| } | ||
|
|
||
| summaries.push(fields); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Emit one summary per matching party, not per case/application.
Both branches use .find(...), so only the first DEFENDANT or first subject === true party is ever emitted. The contract described for this builder is one CaseSummary per defendant and per application subject, so multi-party hearings will drop entries from notification emails. Please iterate the filtered parties and push a separate fields array for each one, then add a regression covering two defendants and two subject parties.
Suggested direction
- const defendant = caseItem.party?.find((p) => p.partyRole === "DEFENDANT");
- const prosecutor = caseItem.party?.find((p) => p.partyRole === "PROSECUTING_AUTHORITY");
- const fields: CaseSummary = [];
-
- if (defendant) {
- const name = extractPartyName(defendant);
- if (name) fields.push({ label: "Name", value: name });
- }
- ...
- summaries.push(fields);
+ const prosecutor = caseItem.party?.find((p) => p.partyRole === "PROSECUTING_AUTHORITY");
+ for (const defendant of caseItem.party?.filter((p) => p.partyRole === "DEFENDANT") ?? []) {
+ const fields: CaseSummary = [];
+ const name = extractPartyName(defendant);
+ if (name) fields.push({ label: "Name", value: name });
+ ...
+ summaries.push(fields);
+ }Apply the same pattern to application.party.
Also applies to: 66-86
| <p><span class="label">{{ t.asn }}</span>{{ hearing.partyInfo.asn }}</p> | ||
| <p><span class="label">{{ t.pncId }}</span>{{ hearing.partyInfo.pncId }}</p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hide empty ASN and PNC rows.
These fields are blank for organisation parties and many applications, so the PDF currently renders empty labels in the right-hand column. Guard them the same way as the other optional fields.
Suggested fix
- <p><span class="label">{{ t.asn }}</span>{{ hearing.partyInfo.asn }}</p>
- <p><span class="label">{{ t.pncId }}</span>{{ hearing.partyInfo.pncId }}</p>
+ {% if hearing.partyInfo.asn %}
+ <p><span class="label">{{ t.asn }}</span>{{ hearing.partyInfo.asn }}</p>
+ {% endif %}
+ {% if hearing.partyInfo.pncId %}
+ <p><span class="label">{{ t.pncId }}</span>{{ hearing.partyInfo.pncId }}</p>
+ {% endif %}📝 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.
| <p><span class="label">{{ t.asn }}</span>{{ hearing.partyInfo.asn }}</p> | |
| <p><span class="label">{{ t.pncId }}</span>{{ hearing.partyInfo.pncId }}</p> | |
| {% if hearing.partyInfo.asn %} | |
| <p><span class="label">{{ t.asn }}</span>{{ hearing.partyInfo.asn }}</p> | |
| {% endif %} | |
| {% if hearing.partyInfo.pncId %} | |
| <p><span class="label">{{ t.pncId }}</span>{{ hearing.partyInfo.pncId }}</p> | |
| {% endif %} |
| "offenceWording": { | ||
| "description": "Long description of offence", | ||
| "title": "Offence wording", | ||
| "type": "string", | ||
| "examples": ["driving whilst under the influence of alcohol"], | ||
| "pattern": "^(?!(?:.|\\r|\\n)*(?:<\\s*\\/[^>]*>|<[^>]*\\/>|<\\s*\\/[^&]*>|<[^&]*\\/>))(?:.|\\r|\\n)*$" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Tighten these anti-HTML patterns.
These regexes only reject closing or self-closing tags. Plain opening tags such as <script> or <img> still validate, which breaks the no-markup rule used elsewhere in this schema and leaves offenceWording able to flow into rendered output unchecked.
Suggested fix
- "pattern": "^(?!(?:.|\\r|\\n)*(?:<\\s*\\/[^>]*>|<[^>]*\\/>|<\\s*\\/[^&]*>|<[^&]*\\/>))(?:.|\\r|\\n)*$"
+ "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$"Apply the same replacement to both offenceWording and applicationParticulars.
Also applies to: 464-469
| "application": { | ||
| "title": "Application", | ||
| "description": "Application associated with the case", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "title": "Application", | ||
| "required": ["applicationReference"], | ||
| "properties": { | ||
| "party": { | ||
| "type": "array", | ||
| "description": "party for the case", | ||
| "title": "Party", | ||
| "items": { | ||
| "type": "object", | ||
| "$ref": "#/$defs/party" | ||
| } | ||
| }, | ||
| "applicationReference": { | ||
| "title": "Application Reference", | ||
| "description": "application reference", | ||
| "type": "string", | ||
| "examples": ["ABC1234567D"], | ||
| "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" | ||
| }, | ||
| "applicationType": { | ||
| "title": "Application Type", | ||
| "description": "application type", | ||
| "type": "string", | ||
| "examples": ["application type example"], | ||
| "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" | ||
| }, | ||
| "applicationParticulars": { | ||
| "title": "Application Particulars", | ||
| "description": "details of an application", | ||
| "type": "string", | ||
| "examples": ["application particulars example"], | ||
| "pattern": "^(?!(?:.|\\r|\\n)*(?:<\\s*/[^>]*>|<[^>]*\\/>|<\\s*\\/[^&]*>|<[^&]*\\/>))(?:.|\\r|\\n)*$" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the Application schema aligned with the exported type.
src/models/types.ts exposes reportingRestriction and reportingRestrictionDetails on Application, but this schema never validates them. That means validateMagistratesStandardList can accept malformed values for fields the rest of the package is allowed to read.
Suggested fix
"applicationParticulars": {
"title": "Application Particulars",
"description": "details of an application",
"type": "string",
"examples": ["application particulars example"],
"pattern": "^(?!(?:.|\\r|\\n)*(?:<\\s*/[^>]*>|<[^>]*\\/>|<\\s*\\/[^&]*>|<[^&]*\\/>))(?:.|\\r|\\n)*$"
+ },
+ "reportingRestriction": {
+ "title": "Reporting Restriction",
+ "type": "boolean"
+ },
+ "reportingRestrictionDetails": {
+ "title": "Reporting Restrictions Detail",
+ "type": "array",
+ "items": {
+ "type": "string",
+ "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$"
+ }
}📝 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.
| "application": { | |
| "title": "Application", | |
| "description": "Application associated with the case", | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "title": "Application", | |
| "required": ["applicationReference"], | |
| "properties": { | |
| "party": { | |
| "type": "array", | |
| "description": "party for the case", | |
| "title": "Party", | |
| "items": { | |
| "type": "object", | |
| "$ref": "#/$defs/party" | |
| } | |
| }, | |
| "applicationReference": { | |
| "title": "Application Reference", | |
| "description": "application reference", | |
| "type": "string", | |
| "examples": ["ABC1234567D"], | |
| "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" | |
| }, | |
| "applicationType": { | |
| "title": "Application Type", | |
| "description": "application type", | |
| "type": "string", | |
| "examples": ["application type example"], | |
| "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" | |
| }, | |
| "applicationParticulars": { | |
| "title": "Application Particulars", | |
| "description": "details of an application", | |
| "type": "string", | |
| "examples": ["application particulars example"], | |
| "pattern": "^(?!(?:.|\\r|\\n)*(?:<\\s*/[^>]*>|<[^>]*\\/>|<\\s*\\/[^&]*>|<[^&]*\\/>))(?:.|\\r|\\n)*$" | |
| } | |
| "applicationParticulars": { | |
| "title": "Application Particulars", | |
| "description": "details of an application", | |
| "type": "string", | |
| "examples": ["application particulars example"], | |
| "pattern": "^(?!(?:.|\\r|\\n)*(?:<\\s*/[^>]*>|<[^>]*\\/>|<\\s*\\/[^&]*>|<[^&]*\\/>))(?:.|\\r|\\n)*$" | |
| }, | |
| "reportingRestriction": { | |
| "title": "Reporting Restriction", | |
| "type": "boolean" | |
| }, | |
| "reportingRestrictionDetails": { | |
| "title": "Reporting Restrictions Detail", | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "pattern": "^(?!(.|\\r|\\n)*<[^>]+>)(.|\\r|\\n)*$" | |
| } | |
| } |
| import { | ||
| extractCaseSummary as extractMagistratesStandardSummary, | ||
| formatCaseSummaryForEmail as formatMagistratesStandardSummaryForEmail | ||
| } from "@hmcts/magistrates-standard-list"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== libs/notifications/package.json =="
jq '{dependencies, devDependencies, peerDependencies}' libs/notifications/package.json 2>/dev/null || cat libs/notifications/package.json
echo
echo "== matching dependency entries =="
rg -n '"`@hmcts/magistrates-standard-list`"' libs/notifications/package.jsonRepository: hmcts/cath-service
Length of output: 941
Add the new workspace dependency to libs/notifications/package.json @hmcts/magistrates-standard-list is imported here but not declared for this package, which can break isolated installs or package-level builds.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/pages/(list-types)/magistrates-standard-list/magistrates-standard-list.njk (1)
13-15: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
header.venueAddresson a single shape.This block assumes
header.venueAddressis an array, but the controller test forapps/web/src/pages/(list-types)/magistrates-standard-list/index.test.tspasses it as a string. If production data follows that contract, this loop will render one character per line. Please normalise the value before rendering, or update the template to handle the actual type consistently.
🧹 Nitpick comments (1)
libs/list-types/magistrates-standard-list/src/pdf/pdf-generator.test.ts (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
constdeclarations toSCREAMING_SNAKE_CASE.
mockSavePdfToStorage,mockRenderedData,mockJsonData,baseOptions, andmockNunjucksEnvall break the repo constant naming rule. Please normalise these names consistently across the file. As per coding guidelines,**/*.{ts,tsx,js}: Use SCREAMING_SNAKE_CASE for constant declarations.Also applies to: 37-69, 72-74
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e46fbe71-767f-4953-b253-433b01040836
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
apps/web/src/assets/css/list-types/magistrates-standard-list.scssapps/web/src/assets/css/web.scssapps/web/src/pages/(list-types)/magistrates-standard-list/magistrates-standard-list.njkdocs/tickets/771/review.mdlibs/list-types/magistrates-standard-list/package.jsonlibs/list-types/magistrates-standard-list/src/assets/css/magistrates-standard-list.scsslibs/list-types/magistrates-standard-list/src/config.test.tslibs/list-types/magistrates-standard-list/src/config.tslibs/list-types/magistrates-standard-list/src/locales/cy.tslibs/list-types/magistrates-standard-list/src/locales/en.tslibs/list-types/magistrates-standard-list/src/pdf/pdf-generator.test.tslibs/list-types/magistrates-standard-list/src/validation/json-validator.test.tslibs/list-types/magistrates-standard-list/tsconfig.jsonlibs/publication/package.json
✅ Files skipped from review due to trivial changes (3)
- libs/list-types/magistrates-standard-list/src/validation/json-validator.test.ts
- apps/web/src/assets/css/list-types/magistrates-standard-list.scss
- libs/list-types/magistrates-standard-list/src/locales/cy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/list-types/magistrates-standard-list/src/config.ts
- libs/list-types/magistrates-standard-list/package.json
- libs/list-types/magistrates-standard-list/tsconfig.json
- libs/list-types/magistrates-standard-list/src/locales/en.ts
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/pages/(list-types)/magistrates-standard-list/magistrates-standard-list.njk (2)
1-204: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftTemplate hand-rolls GOV.UK components instead of importing macros.
The whole page builds
govuk-warning-text,govuk-accordion,govuk-details, andgovuk-tablemarkup by hand with raw classes, with no{% from "govuk/components/.../macro.njk" import ... %}anywhere in the file. This bypasses the design system's built-in accessibility/JS wiring and diverges from the intended pattern.As per coding guidelines, "Use Nunjucks templates with
.njkextension inapps/web/src/pages/and import GOV.UK Design System macros."Source: Coding guidelines
47-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHeading hierarchy broken:
<h1>used repeatedly below an<h2>page title.The page title (line 7) is an
<h2>, but court room headings inside the loop (lines 53/56/58) are<h1>, and there can be multiple<h1>elements per page (one set per court room). This violates expected heading order and having a single top-level heading, which is a WCAG concern (1.3.1/2.4.6) for screen-reader navigation.♿ Suggested fix
- {% if room.courtHouseName %} - <h1 class="govuk-heading-l site-address">{{ room.courtHouseName }}</h1> - {% endif %} - {% if room.lja %} - <h1 class="govuk-heading-l site-address">{{ t.lja }}: {{ room.lja }}</h1> - {% endif %} - <h1 class="govuk-heading-l site-address">{{ room.courtRoomName }}</h1> + {% if room.courtHouseName %} + <h2 class="govuk-heading-l site-address">{{ room.courtHouseName }}</h2> + {% endif %} + {% if room.lja %} + <h2 class="govuk-heading-l site-address">{{ t.lja }}: {{ room.lja }}</h2> + {% endif %} + <h2 class="govuk-heading-l site-address">{{ room.courtRoomName }}</h2>
🧹 Nitpick comments (2)
libs/list-types/magistrates-standard-list/src/rendering/renderer.test.ts (1)
218-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a fixture builder to cut duplication.
Each of these tests hand-rolls a full
MagistratesStandardListobject (several also spread...FULL_JSONbut then fully replacecourtLists, making the spread a no-op). A small factory helper (e.g.buildCase(overrides)/buildHearing(overrides)) would shrink these blocks and make future schema changes easier to propagate across tests.Also applies to: 343-398, 400-458, 482-601, 603-647
libs/list-types/magistrates-standard-list/src/email-summary/summary-builder.test.ts (1)
185-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest title says "empty name" but asserts
undefined.Minor wording mismatch — the field is undefined, not an empty string, so the description could read "should not include Name field" for accuracy.
✏️ Suggested wording tweak
- it("should return empty name when party has neither individual nor organisation details", () => { + it("should omit Name field when party has neither individual nor organisation details", () => {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c19b5a66-a84d-4beb-9476-e452831084a5
📒 Files selected for processing (7)
apps/web/src/pages/(list-types)/magistrates-standard-list/magistrates-standard-list.njklibs/list-types/magistrates-standard-list/src/email-summary/summary-builder.test.tslibs/list-types/magistrates-standard-list/src/locales/cy.tslibs/list-types/magistrates-standard-list/src/locales/en.tslibs/list-types/magistrates-standard-list/src/pdf/pdf-template.njklibs/list-types/magistrates-standard-list/src/rendering/renderer.test.tslibs/list-types/magistrates-standard-list/src/rendering/renderer.ts
✅ Files skipped from review due to trivial changes (2)
- libs/list-types/magistrates-standard-list/src/locales/en.ts
- libs/list-types/magistrates-standard-list/src/locales/cy.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/list-types/magistrates-standard-list/src/pdf/pdf-template.njk
- libs/list-types/magistrates-standard-list/src/rendering/renderer.ts
| {% if offence.offenceWording %} | ||
| <tr class="govuk-table__row"> | ||
| <td class="govuk-table__cell">{{ offence.offenceWording }}</td> | ||
| </tr> | ||
| {% endif %} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Offence wording row lacks a label cell, unlike its siblings.
Every other row in this table has a label <td> plus a value <td>, but the offenceWording row only has a single cell. This is inconsistent for a table structure and gives no context to a screen reader user landing on that row.
💡 Suggested fix
{% if offence.offenceWording %}
<tr class="govuk-table__row">
+ <td class="govuk-table__cell"><span class="linked-cases-heading">{{ t.offenceWording }}</span></td>
<td class="govuk-table__cell">{{ offence.offenceWording }}</td>
</tr>
{% endif %}📝 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.
| {% if offence.offenceWording %} | |
| <tr class="govuk-table__row"> | |
| <td class="govuk-table__cell">{{ offence.offenceWording }}</td> | |
| </tr> | |
| {% endif %} | |
| {% if offence.offenceWording %} | |
| <tr class="govuk-table__row"> | |
| <td class="govuk-table__cell"><span class="linked-cases-heading">{{ t.offenceWording }}</span></td> | |
| <td class="govuk-table__cell">{{ offence.offenceWording }}</td> | |
| </tr> | |
| {% endif %} |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
… types Keeps magistrates-standard-list additions from this branch alongside crown daily/firm/warned list, SSCS daily hearing lists, SIAC/POAC/PAAC, FTT, GRC, WPAFCC, and UTIAC list types merged from master. Runs yarn install to link the newly added magistrates-standard-list workspace package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
2 participants
Jira link
#771
Change description
Add Magistrates Standard List style guide and PDF
Testing done
Security Vulnerability Assessment
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist
Summary by CodeRabbit