VIBE-247 Add authentication for publication #156
Conversation
|
Warning Rate limit exceeded@KianKwa has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughAdds a centralized publication authorization service and Express middleware enforcing role- and provenance-based access to publication metadata and data; integrates checks into publication and list pages, updates provenance constants, adds 403/common error templates, seeds and tests (unit, middleware, E2E), and relevant docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant Server
participant AuthMW as Auth\nMiddleware
participant AuthSvc as Auth\nService
participant DB as Prisma/DB
Browser->>Server: GET /publication/:id
Server->>AuthMW: requirePublicationAccess(req,res,next)
AuthMW->>DB: artefact.findUnique(publicationId)
DB-->>AuthMW: artefact or null
alt artefact found
AuthMW->>AuthSvc: canAccessPublication(user, artefact, listType)
AuthSvc-->>AuthMW: allowed / denied
alt allowed
AuthMW->>Server: next() -> handler renders page
Server->>Browser: 200 OK (HTML)
else denied
AuthMW->>Server: render 403 (localized)
Server->>Browser: 403 Access Denied
end
else not found
AuthMW->>Server: render 404
Server->>Browser: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (6)
libs/web-core/src/views/errors/common.njk (1)
3-7: Consider aligning variable names with 403.njk for consistency.This template uses
errorTitle/errorMessagewhile403.njkusestitle/message. Consistent variable names across error templates would simplify usage and reduce cognitive load.libs/public-pages/src/pages/summary-of-publications/index.ts (1)
36-46: Access filtering implemented correctly.The two-step approach (fetch all, then filter) ensures correct authorization logic. The naming convention (
allArtefacts→artefacts) clearly distinguishes between pre- and post-filtering datasets.For large datasets, consider implementing database-level filtering using Prisma's
whereclause to reduce memory usage and improve performance. This would require extending the Prisma query to include sensitivity and user role/provenance checks, though the current approach is simpler to maintain and test.libs/publication/src/authorisation/middleware.ts (3)
12-42: Middleware implementation follows Express patterns correctly.The
requirePublicationAccess()middleware properly handles all HTTP error cases (400, 404, 403, 500) and follows the middleware signature pattern.Consider replacing
console.erroron line 38 with a structured logger to:
- Enable better production monitoring
- Avoid potential information disclosure
- Support consistent log aggregation
- console.error("Error checking publication access:", error); + logger.error('Error checking publication access', { + error: error instanceof Error ? error.message : String(error), + publicationId, + userId: req.user?.id + });
50-89: Data access middleware includes helpful bilingual messaging.The middleware correctly distinguishes between general access (metadata) and data access (actual list content), with clear English and Welsh error messages explaining the restriction.
Similar to
requirePublicationAccess(), consider using a structured logger instead ofconsole.erroron line 85 for better production observability.
20-27: Consider caching artefact to avoid duplicate database queries.Both middlewares fetch the artefact from the database, and the downstream handler may fetch it again. While this ensures data consistency, it adds unnecessary database load.
To optimize, consider augmenting the Express Request type to attach the fetched artefact:
// In a types file declare global { namespace Express { interface Request { artefact?: Artefact; } } } // In middleware, after fetching req.artefact = artefact; // In handler const artefact = req.artefact || await prisma.artefact.findUnique(...);This reduces database queries while maintaining the fail-safe pattern if
req.artefactis undefined.Also applies to: 58-65
libs/publication/src/authorisation/service.ts (1)
119-124: Optional: pre-index listTypes to avoid repeated linear scans
filterAccessiblePublicationsdoes alistTypes.findfor each artefact, which is fine for small collections but is O(n*m). If this ends up in a hot path with many artefacts/list types, consider pre-indexinglistTypesbyidinto a Map before filtering.For example:
const listTypeById = new Map(listTypes.map((lt) => [lt.id, lt])); return artefacts.filter((artefact) => canAccessPublication(user, artefact, listTypeById.get(artefact.listTypeId)) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/tickets/VIBE-247/plan.md(1 hunks)docs/tickets/VIBE-247/review.md(1 hunks)docs/tickets/VIBE-247/tasks.md(1 hunks)docs/tickets/VIBE-247/ticket.md(1 hunks)libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts(2 hunks)libs/list-types/common/src/mock-list-types.ts(1 hunks)libs/public-pages/src/pages/publication/[id].test.ts(12 hunks)libs/public-pages/src/pages/publication/[id].ts(2 hunks)libs/public-pages/src/pages/summary-of-publications/index.ts(3 hunks)libs/publication/src/authorisation/middleware.test.ts(1 hunks)libs/publication/src/authorisation/middleware.ts(1 hunks)libs/publication/src/authorisation/service.test.ts(1 hunks)libs/publication/src/authorisation/service.ts(1 hunks)libs/publication/src/index.ts(1 hunks)libs/web-core/src/views/errors/400.njk(1 hunks)libs/web-core/src/views/errors/403.njk(1 hunks)libs/web-core/src/views/errors/common.njk(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/publication/src/authorisation/middleware.test.tslibs/publication/src/authorisation/service.test.tslibs/publication/src/authorisation/service.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.tslibs/publication/src/index.tslibs/publication/src/authorisation/middleware.tslibs/list-types/common/src/mock-list-types.tslibs/public-pages/src/pages/publication/[id].test.tslibs/public-pages/src/pages/summary-of-publications/index.tslibs/public-pages/src/pages/publication/[id].ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/publication/src/authorisation/middleware.test.tslibs/publication/src/authorisation/service.test.tslibs/publication/src/authorisation/service.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.tslibs/publication/src/index.tslibs/publication/src/authorisation/middleware.tslibs/list-types/common/src/mock-list-types.tslibs/public-pages/src/pages/publication/[id].test.tslibs/public-pages/src/pages/summary-of-publications/index.tslibs/public-pages/src/pages/publication/[id].ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
libs/publication/src/authorisation/middleware.test.tslibs/publication/src/authorisation/service.test.tslibs/public-pages/src/pages/publication/[id].test.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/list-types/civil-and-family-daily-cause-list/src/pages/index.tslibs/public-pages/src/pages/publication/[id].test.tslibs/public-pages/src/pages/summary-of-publications/index.tslibs/public-pages/src/pages/publication/[id].ts
🧠 Learnings (4)
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk` and use GOV.UK component macros (govukButton, govukInput, govukErrorSummary, etc.).
Applied to files:
libs/web-core/src/views/errors/403.njklibs/web-core/src/views/errors/common.njk
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to libs/**/*-middleware.ts : Reusable middleware must be placed in a dedicated `libs/[module]/src/[middleware-name]-middleware.ts` file and exported as a function.
Applied to files:
libs/publication/src/authorisation/middleware.tslibs/public-pages/src/pages/publication/[id].ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/pages/**/*.ts : Page controllers must export named `GET` and/or `POST` functions with Express Request and Response types.
Applied to files:
libs/public-pages/src/pages/publication/[id].test.tslibs/public-pages/src/pages/publication/[id].ts
📚 Learning: 2025-12-01T11:31:12.342Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T11:31:12.342Z
Learning: Applies to **/*.{ts,tsx} : Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Applied to files:
libs/public-pages/src/pages/publication/[id].test.ts
🧬 Code graph analysis (7)
libs/publication/src/authorisation/middleware.test.ts (1)
libs/publication/src/authorisation/middleware.ts (2)
requirePublicationAccess(12-42)requirePublicationDataAccess(50-89)
libs/publication/src/authorisation/service.test.ts (1)
libs/publication/src/authorisation/service.ts (4)
canAccessPublication(16-56)canAccessPublicationData(66-80)canAccessPublicationMetadata(90-110)filterAccessiblePublications(119-124)
libs/publication/src/authorisation/service.ts (2)
libs/publication/src/index.ts (7)
canAccessPublication(3-3)Artefact(7-7)ListType(1-1)Sensitivity(9-9)canAccessPublicationData(3-3)canAccessPublicationMetadata(3-3)filterAccessiblePublications(3-3)libs/list-types/common/src/mock-list-types.ts (1)
ListType(1-8)
libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts (2)
libs/list-types/common/src/mock-list-types.ts (1)
mockListTypes(10-75)libs/publication/src/authorisation/service.ts (1)
canAccessPublicationData(66-80)
libs/public-pages/src/pages/publication/[id].test.ts (1)
libs/public-pages/src/pages/publication/[id].ts (1)
GET(34-34)
libs/public-pages/src/pages/summary-of-publications/index.ts (3)
libs/publication/src/authorisation/service.ts (1)
filterAccessiblePublications(119-124)libs/publication/src/index.ts (2)
filterAccessiblePublications(3-3)mockListTypes(1-1)libs/list-types/common/src/mock-list-types.ts (1)
mockListTypes(10-75)
libs/public-pages/src/pages/publication/[id].ts (3)
libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
GET(173-173)libs/publication/src/authorisation/middleware.ts (1)
requirePublicationAccess(12-42)libs/publication/src/index.ts (1)
requirePublicationAccess(2-2)
🪛 LanguageTool
docs/tickets/VIBE-247/plan.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...ty, provenance), but we need to add list type provenance lookups to validate clas...
(QB_NEW_EN_HYPHEN)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ...sation check (and similar for other list type pages) - `libs/admin-pages/src/page...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~49-~49: Do not mix variants of the same word (‘authorization’ and ‘authorisation’) within a single text.
Context: ...Endpoints No new API endpoints needed. Authorization will be added to existing endpoints: - ...
(EN_WORD_COHERENCY)
[uncategorized] ~74-~74: Do not mix variants of the same word (‘authorization’ and ‘authorisation’) within a single text.
Context: ...AC5: Validation using user provenance | Authorization service uses UserProfile.provenance fro...
(EN_WORD_COHERENCY)
docs/tickets/VIBE-247/review.md
[uncategorized] ~93-~93: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...m in UserProfile interface. --- ##
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~343-~343: Use a hyphen to join words.
Context: ...r roles - [ ] Implement caching for list type lookups - [ ] Add rate limiting for...
(QB_NEW_EN_HYPHEN)
docs/tickets/VIBE-247/tasks.md
[grammar] ~26-~26: Use a hyphen to join words.
Context: ...horisation check - [x] Update all list type page handlers to include authorisat...
(QB_NEW_EN_HYPHEN)
⏰ 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 (22)
docs/tickets/VIBE-247/ticket.md (1)
1-57: LGTM! Well-documented requirements.The ticket documentation clearly outlines the access control rules and permissions matrix for publication sensitivity levels. This provides excellent context for the implementation.
libs/public-pages/src/pages/publication/[id].test.ts (2)
14-16: Mock is appropriate for unit testing the handler, but consider adding middleware integration tests.The mock correctly bypasses authorization to test handler logic in isolation. However, since
requirePublicationAccessmiddleware is part of the route's behavior, consider adding integration tests that verify the middleware properly denies access when needed.
33-34: Handler extraction pattern is correct.The logic correctly extracts the final handler from the middleware array, aligning with the production code where
GET = [requirePublicationAccess(), handler].libs/publication/src/index.ts (1)
2-3: LGTM! Authorization exports are well-organized.The new exports follow proper naming conventions (camelCase for functions) and include the required
.jsextension for ESM compatibility. The public API surface for authorization is clearly defined.libs/web-core/src/views/errors/403.njk (1)
1-9: LGTM! Well-structured 403 error template.The template correctly extends the base layout, uses GOV.UK styling classes, and provides sensible defaults for title and message. The contact link offers a clear path for users who believe they should have access.
libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts (2)
49-49: Verify ifmockListTypesis intended for production use.Using
mockListTypes(named with "mock" prefix) in production code suggests this may be placeholder data. Ensure this is either intended as production-ready configuration or planned to be replaced with a database-driven source.
48-61: Template variable mismatch will cause default messages to display.The
403.njktemplate expectstitleandmessageat the root level, but this code passes nesteden/cyobjects. The template will fallback to defaults ("Access Denied", "You do not have permission...") ignoring the Welsh translations.To support i18n properly, either:
Option 1: Pass the correct locale's values based on current locale:
+ const t403 = locale === "cy" + ? { title: "Mynediad wedi'i Wrthod", message: "Nid oes gennych ganiatâd i weld y cyhoeddiad hwn." } + : { title: "Access Denied", message: "You do not have permission to view this publication." }; + if (!canAccessPublicationData(req.user, artefact, listType)) { return res.status(403).render("errors/403", { - en: { - title: "Access Denied", - message: "You do not have permission to view this publication." - }, - cy: { - title: "Mynediad wedi'i Wrthod", - message: "Nid oes gennych ganiatâd i weld y cyhoeddiad hwn." - } + title: t403.title, + message: t403.message }); }Option 2: Update
403.njkto support theen/cypattern used elsewhere in this codebase.⛔ Skipped due to learnings
Learnt from: CR Repo: hmcts/cath-service PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-01T11:31:12.342Z Learning: Applies to **/pages/**/*.ts : Every page must support both English and Welsh with separate `en` and `cy` content objects in the controller, and Welsh content must be tested with `?lng=cy` query parameter.libs/list-types/common/src/mock-list-types.ts (1)
16-16: LGTM! Provenance values updated to support authorization logic.The simplified provenance values ("CFT" and "CRIME") align with the new authorization service's provenance matching logic for CLASSIFIED publications.
Also applies to: 24-24, 32-32, 40-40, 48-48, 56-56, 64-64, 72-72
libs/public-pages/src/pages/summary-of-publications/index.ts (1)
3-3: LGTM! Correct use of workspace alias.The import uses the
@hmcts/publicationworkspace alias correctly, which doesn't require a.jsextension per the coding guidelines.libs/publication/src/authorisation/middleware.test.ts (1)
1-467: Excellent test coverage!The test suite comprehensively covers both middleware functions with clear, descriptive test names and proper mock setup. The tests validate all critical paths including error cases, access scenarios, and bilingual messaging.
libs/public-pages/src/pages/publication/[id].ts (2)
2-2: LGTM! Correct workspace alias import.Using
@hmcts/publicationworkspace alias is correct per the coding guidelines, which don't require.jsextensions for workspace aliases.
5-34: Middleware pattern implemented correctly.The approach of extracting the handler as a separate constant and wrapping it in a middleware array follows the established pattern and aligns with the coding guidelines for reusable middleware.
Based on learnings, reusable middleware must be placed in a dedicated
libs/[module]/src/[middleware-name]-middleware.tsfile and exported as a function, which has been correctly done withrequirePublicationAccess().libs/publication/src/authorisation/middleware.ts (1)
1-4: LGTM! Correct import patterns.The imports follow the coding guidelines correctly:
- Workspace aliases (
@hmcts/*) don't require.jsextensions- Relative import from
./service.jsincludes the required.jsextension for ESMlibs/publication/src/authorisation/service.test.ts (5)
8-39: Test helpers look solid but double‑check ListType shapeThe helper factories make tests readable and ensure consistent Artefact/User setup. One thing to confirm is that the
createListTypeliteral matches the actualListTypeinterface exported from@hmcts/list-types-common(field names likelistType/name,jsonSchema, etc.). A mismatch there would only show up at compile time.If you haven’t already, please confirm
createListTypesatisfies the realListTypedefinition from@hmcts/list-types-common.
41-193: canAccessPublication tests comprehensively exercise the role/provenance matrixThe PUBLIC/PRIVATE/CLASSIFIED suites and “missing sensitivity” cases align well with the service logic: PUBLIC open to all, PRIVATE restricted to verified provenances, CLASSIFIED restricted to verified + matching listType provenance, and unknown/missing sensitivity failing closed as CLASSIFIED. System admin behavior and local/CTSC “public only on public pages” constraints are all explicitly asserted.
196-271: canAccessPublicationData tests correctly assert metadata‑only behaviorThese tests verify that LOCAL/CTSC admins are blocked from PRIVATE/CLASSIFIED data but allowed for PUBLIC, and that system admins and verified users follow the base
canAccessPublicationrules. That matches the intended “metadata‑only roles” semantics in the service.
274-343: canAccessPublicationMetadata tests match the intended access modelMetadata tests confirm: PUBLIC metadata is visible to everyone (including unauthenticated), and for PRIVATE/CLASSIFIED, any authenticated user (including admins and verified users) can see metadata. This aligns with the implementation and gives good confidence in the behavior.
345-417: filterAccessiblePublications tests cover key edge cases and provenance filteringThe scenarios around unauthenticated users, verified users with and without matching provenance, admins, empty artefact arrays, and missing list types all line up with how
filterAccessiblePublicationsdelegates tocanAccessPublication. The “fail closed” behavior for unknown listType IDs is explicitly asserted, which is important from a security perspective.libs/publication/src/authorisation/service.ts (4)
1-8: Imports and constants follow project conventionsUsing workspace aliases,
.jsextensions on relatives, and SCREAMING_SNAKE_CASE constants is consistent with the coding guidelines. CentralizingMETADATA_ONLY_ROLESandVERIFIED_USER_PROVENANCEShere keeps the access rules readable.
16-56: canAccessPublication implements a clear, fail‑closed access matrixThe role/sensitivity logic is sound:
- SYSTEM_ADMIN short‑circuit for full access.
- PUBLIC always accessible.
- PRIVATE restricted to verified provenances.
- CLASSIFIED restricted to verified provenances + matching listType provenance, failing closed when listType is missing.
- Missing/empty sensitivity defaulting to CLASSIFIED is appropriate.
This matches the tests and provides sensible security defaults.
66-80: canAccessPublicationData correctly enforces metadata‑only rolesThe extra guard for
METADATA_ONLY_ROLESon PRIVATE/CLASSIFIED, combined with delegation tocanAccessPublicationfor everyone else, cleanly expresses “local/CTSC admins can see only metadata for non‑public lists.” This is straightforward and easy to maintain.
82-110: Align canAccessPublicationMetadata doc comment with implementationThe implementation allows PUBLIC metadata to be accessed by anyone (including unauthenticated users) and grants full metadata access to system + metadata‑only admins, while requiring authentication for PRIVATE/CLASSIFIED. The doc comment currently says “All authenticated users can view PUBLIC metadata,” which understates the PUBLIC case.
[ suggest_recommended_refactor ]
Consider updating the comment to more precisely match behavior, e.g.:- * Determines if a user can access publication metadata - * All authenticated users can view PUBLIC metadata - * Admins can view all metadata + * Determines if a user can access publication metadata + * PUBLIC metadata is accessible to everyone + * System + metadata-only admins can view all metadata + * PRIVATE and CLASSIFIED metadata require an authenticated user
🎭 Playwright E2E Test Results165 tests 165 ✅ 11m 35s ⏱️ Results for commit 5cd82bb. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
libs/auth/src/pages/cft-callback/index.ts (1)
49-63: UserProfile provenance now consistent with CFT_IDAM DB valueSetting
provenance: "CFT_IDAM"on theUserProfilekeeps the session user aligned with thecreateOrUpdateUsercall and the updated tests. This tightens consistency across the CFT IDAM flow.If you find more CFT_IDAM literals elsewhere, consider a shared constant to avoid future drift.
e2e-tests/tests/publication-authorisation.spec.ts (6)
22-32: Weak assertion:count >= 0is always true.The assertion
expect(count).toBeGreaterThanOrEqual(0)will always pass sincecount()returns a non-negative integer. Consider either:
- Asserting a specific expected count if the test data is deterministic
- Adding a comment explaining this is intentional smoke test behavior
- Removing the assertion if it provides no value
62-68: Simplify async flow usingawaitinstead of.then()chains.The repeated
.then()pattern is inconsistent with the rest of the codebase which usesasync/await.- const is403Page = await page.locator("h1").textContent().then(text => text?.includes("Access Denied") || text?.includes("Forbidden")); - const isSignInPage = currentUrl.includes("/sign-in"); - const is404Page = await page.locator("h1").textContent().then(text => text?.includes("not found")); + const headingText = await page.locator("h1").textContent(); + const is403Page = headingText?.includes("Access Denied") || headingText?.includes("Forbidden"); + const isSignInPage = currentUrl.includes("/sign-in"); + const is404Page = headingText?.includes("not found");
72-109: Consider extracting repeated login flow totest.beforeEachor a fixture.The CFT login sequence (lines 75-88) is duplicated across 7+ tests in this file. Extract to a
beforeEachhook or Playwright fixture to reduce duplication and improve maintainability.test.describe("CFT IDAM authenticated users", () => { test.beforeEach(async ({ page }) => { await page.goto("/sign-in"); const hmctsRadio = page.getByRole("radio", { name: /with a myhmcts account/i }); await hmctsRadio.check(); const continueButton = page.getByRole("button", { name: /continue/i }); await continueButton.click(); await loginWithCftIdam( page, process.env.CFT_VALID_TEST_ACCOUNT!, process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD! ); await assertAuthenticated(page); }); test("should see PUBLIC, PRIVATE, and CLASSIFIED CFT publications", async ({ page }) => { // Navigate directly - already authenticated await page.goto("/summary-of-publications?locationId=3"); // ... rest of test }); });
82-86: Add runtime validation for required environment variables.Non-null assertions (
!) on environment variables will cause cryptic runtime errors if the variables are not set. Consider adding validation at the start of the test file or using a helper.// At the top of the file or in a beforeAll hook: const CFT_TEST_ACCOUNT = process.env.CFT_VALID_TEST_ACCOUNT; const CFT_TEST_PASSWORD = process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD; if (!CFT_TEST_ACCOUNT || !CFT_TEST_PASSWORD) { throw new Error("CFT_VALID_TEST_ACCOUNT and CFT_VALID_TEST_ACCOUNT_PASSWORD environment variables are required"); }
330-338:waitForLoadState("networkidle")can be flaky.The
networkidlestrategy waits for no network activity for 500ms, which can be unreliable. Consider using a more deterministic wait condition like waiting for a specific element or URL pattern.- await page.waitForLoadState("networkidle"); + // Wait for specific content that indicates the page loaded successfully + await page.waitForSelector("h1");
389-422: Consider integrating axe-core for comprehensive accessibility testing.The current accessibility check only verifies links have text content. For more comprehensive accessibility compliance, consider using
@axe-core/playwrightwhich can detect WCAG violations automatically.docs/tickets/VIBE-247/e2e-tests-summary.md (2)
4-4: Spelling inconsistency: "authorization" vs "authorisation".The document mixes American spelling ("authorization") with British spelling ("authorisation" used in the filename and elsewhere). Consider using consistent British spelling throughout to match the HMCTS standard.
-Created comprehensive end-to-end tests to verify role-based and provenance-based authorization for publications based on sensitivity levels. +Created comprehensive end-to-end tests to verify role-based and provenance-based authorisation for publications based on sensitivity levels.
38-44: Add language specifiers to fenced code blocks.Per markdownlint, fenced code blocks should have a language specified. These appear to be plain text scenarios, so
textorplaintextwould be appropriate.-``` +```text 1. Navigate to /summary-of-publications?locationId=3Also applies to: 47-57, 60-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/tickets/VIBE-247/e2e-tests-summary.md(1 hunks)e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)libs/auth/src/middleware/authorise.test.ts(1 hunks)libs/auth/src/pages/cft-callback/index.test.ts(1 hunks)libs/auth/src/pages/cft-callback/index.ts(1 hunks)libs/auth/src/pages/logout/index.test.ts(1 hunks)libs/list-types/common/src/mock-list-types.ts(1 hunks)libs/publication/src/authorisation/middleware.test.ts(1 hunks)libs/publication/src/authorisation/service.test.ts(1 hunks)libs/publication/src/authorisation/service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/publication/src/authorisation/service.test.ts
- libs/publication/src/authorisation/service.ts
- libs/publication/src/authorisation/middleware.test.ts
- libs/list-types/common/src/mock-list-types.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.tslibs/auth/src/pages/logout/index.test.tslibs/auth/src/pages/cft-callback/index.tslibs/auth/src/pages/cft-callback/index.test.tslibs/auth/src/middleware/authorise.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/publication-authorisation.spec.tslibs/auth/src/pages/logout/index.test.tslibs/auth/src/pages/cft-callback/index.tslibs/auth/src/pages/cft-callback/index.test.tslibs/auth/src/middleware/authorise.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/publication-authorisation.spec.tslibs/auth/src/pages/logout/index.test.tslibs/auth/src/pages/cft-callback/index.test.tslibs/auth/src/middleware/authorise.test.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/auth/src/pages/logout/index.test.tslibs/auth/src/pages/cft-callback/index.tslibs/auth/src/pages/cft-callback/index.test.ts
🧬 Code graph analysis (1)
e2e-tests/tests/publication-authorisation.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
🪛 GitHub Actions: Test
libs/auth/src/pages/logout/index.test.ts
[error] 1-1: Test command exited with code 1.
libs/auth/src/pages/cft-callback/index.ts
[error] 1-1: Test command exited with code 1.
libs/auth/src/pages/cft-callback/index.test.ts
[error] 1-1: Test command exited with code 1.
libs/auth/src/middleware/authorise.test.ts
[error] 1-1: Test command exited with code 1.
🪛 LanguageTool
docs/tickets/VIBE-247/e2e-tests-summary.md
[uncategorized] ~4-~4: Do not mix variants of the same word (‘authorization’ and ‘authorisation’) within a single text.
Context: ... verify role-based and provenance-based authorization for publications based on sensitivity l...
(EN_WORD_COHERENCY)
[style] ~13-~13: Try moving the adverb to make the sentence clearer.
Context: ...ld redirect to sign-in or show 403 when trying to directly access CLASSIFIED publications ### 2. CFT IDAM Authenticated Users (V...
(SPLIT_INFINITIVE)
🪛 markdownlint-cli2 (0.18.1)
docs/tickets/VIBE-247/e2e-tests-summary.md
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (8)
libs/auth/src/middleware/authorise.test.ts (1)
248-272: CFT_IDAM provenance in blockUserAccess test is consistent with auth flowThe updated provenance
"CFT_IDAM"for the CFT IDAM user matches the handler and other tests, keeping the blockUserAccess behaviour aligned with the new provenance model.libs/auth/src/pages/cft-callback/index.test.ts (1)
80-92: Login expectation now correctly asserts CFT_IDAM provenanceExpecting
provenance: "CFT_IDAM"in theloginpayload is consistent with the handler implementation and the mocked DB user, so the test accurately reflects the CFT IDAM flow.libs/auth/src/pages/logout/index.test.ts (1)
13-41: Logout test fixture now matches CFT_IDAM provenance semanticsUsing
provenance: "CFT_IDAM"for the CFT IDAM logout scenario is consistent with the rest of the auth flow and keeps this test’s fixture aligned with the updated provenance model.e2e-tests/tests/publication-authorisation.spec.ts (2)
1-3: Imports look correct.File follows ESM with
.jsextension as required by coding guidelines, and uses Playwright test framework appropriately.
380-386: Assertion may be too permissive.The regex
/\/400|\/summary-of-publications/allows the test to pass if the page stays onsummary-of-publications, which doesn't necessarily indicate graceful error handling. If invalidlocationIdshould show a 400 error, consider making the assertion more specific.docs/tickets/VIBE-247/e2e-tests-summary.md (3)
81-83: Hardcoded artefact ID may cause test brittleness.The documentation references a specific artefact ID
a4f06ae6-399f-4207-b676-54f35ad908edwhich is also used in the test file. If this ID changes in the test database, both the test and documentation will need updating. Consider using a more generic description or noting this as a data dependency.
100-107: Environment variable documentation looks good.The password is appropriately shown as a placeholder
<password>rather than an actual credential, and the documentation clearly lists required environment variables for running the tests.
1-165: Documentation is comprehensive and well-structured.The E2E test summary provides excellent coverage of test scenarios, requirements, running instructions, and integration context. This will be valuable for future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/auth/src/pages/logout/index.ts (1)
19-29: Provenance check updated to"CFT_IDAM"– looks good, but confirm consistency and consider centralisingThe behavior (CFT IDAM users bypassing Azure AD logout and going straight to
/session-logged-out) is preserved; only the provenance value has been renamed, which aligns with the PR’s provenance normalization.Two follow-ups to consider:
- Ensure
"CFT_IDAM"matches the canonical provenance value used across the rest of the auth/authorization layer (e.g. callbacks, user model/type, tests), so you don’t end up with mixed"CFT"vs"CFT_IDAM"handling.- If you already have a shared provenance enum/constant, it would be safer to reference that here instead of a raw string literal to avoid future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/auth/src/pages/logout/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
libs/auth/src/pages/logout/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
libs/auth/src/pages/logout/index.ts
**/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/pages/**/*.ts: Page controllers must export namedGETand/orPOSTfunctions with Express Request and Response types.
Every page must support both English and Welsh with separateenandcycontent objects in the controller, and Welsh content must be tested with?lng=cyquery parameter.
Files:
libs/auth/src/pages/logout/index.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
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e-tests/tests/publication-authorisation.spec.ts (3)
72-262: Reduce duplication in CFT-authenticated flows and strengthen semantics of “PRIVATE/CLASSIFIED visible” testsIn the
CFT IDAM authenticated usersblock there are two main concerns:
- Heavy duplication of the CFT login flow
The full CFT login sequence (visit
/sign-in, choose MyHMCTS radio, click continue, callloginWithCftIdam,assertAuthenticated) is repeated in every CFT-authenticated test (Lines 75–88, 113–126, 153–166, 183–196, 226–239). This makes the file harder to maintain and easy to break if the sign‑in UI or flow changes.It would be cleaner to centralise this into a helper within the describe, or into a
test.beforeEachthat logs in a CFT user for all tests in this block, e.g.:test.describe("CFT IDAM authenticated users (VERIFIED role with CFT provenance)", () => { async function loginAsCftUser(page: Page): Promise<void> { await page.goto("/sign-in"); await page.getByRole("radio", { name: /with a myhmcts account/i }).check(); await page.getByRole("button", { name: /continue/i }).click(); await loginWithCftIdam( page, process.env.CFT_VALID_TEST_ACCOUNT!, process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD! ); await assertAuthenticated(page); } test("should see PUBLIC, PRIVATE, and CLASSIFIED CFT publications", async ({ page }) => { await loginAsCftUser(page); // ... }); // other tests... });(You can also move
loginAsCftUserto a shared helper if other specs need it.)
- Assertions don’t really prove PRIVATE/CLASSIFIED visibility
Tests named
should see PUBLIC, PRIVATE, and CLASSIFIED CFT publicationsandshould see PRIVATE publicationscurrently only assert that there is at least one link (count > 0). That doesn’t distinguish between PUBLIC-only vs PUBLIC+PRIVATE+CLASSIFIED scenarios, so regressions in the new access rules could easily slip through.Where possible, I’d suggest:
- Asserting on known PRIVATE/CLASSIFIED artefact labels, or
- Explicitly comparing counts between unauthenticated and CFT-authenticated users within the same test, to demonstrate broader access for the verified CFT user.
Together with the unauthenticated tests, that would give a much clearer signal that the new authorisation logic is working end to end.
264-345: Provenance tests are mostly smoke checks; consider asserting provenance-specific behaviourThe
Provenance-based filtering for CLASSIFIED publicationsdescribe block is a good start but currently quite loose:
CFT user should see CFT CLASSIFIED publications(Lines 265–297) only assertsexpect(cftCount).toBeGreaterThanOrEqual(0);, which is always true, and then checks that the first link (if present) contains “Civil”. That’s more of a smoke test than a strong provenance assertion.should verify CLASSIFIED publications match user provenance(Lines 299–343) ensures that clicking the first publication link succeeds (no 403 / sign-in redirect), but doesn’t check that the publication is actually a CFT‑provenance CLASSIFIED artefact rather than just any PUBLIC item.If you have stable seeded data for this environment, consider:
- Targeting a known CFT‑provenance CLASSIFIED artefact by title or data‑test attribute and asserting that it is visible and accessible to the CFT user; and
- (Optionally) ensuring that non‑CFT CLASSIFIED items for the same location are not present in the list for this user, to really exercise the provenance filter.
That would better exercise the VIBE‑247 provenance rules rather than just confirming that “some link works”.
347-387: Edge-case/error-handling tests look good; minor opportunity to assert user-facing messagingThe edge-case tests are sensible and lightweight:
- Missing sensitivity level scenario just ensures the summary page loads (Lines 348–356).
- Direct access to a restricted artefact asserts one of 403/404/sign‑in outcomes based on body text and URL (Lines 358–378).
- Invalid
locationIdscenario checks we either land on/400or stay on/summary-of-publications(Lines 380–386).If you want to make these a bit more robust without overcomplicating them, you could:
- For the invalid
locationIdcase, also assert on a specific error heading or message (ideally in both English and Welsh) once you know what the design system uses there.- For the restricted-artefact case, assert that we don’t see the normal list content (e.g. by checking that the main summary heading is absent when
isAccessDenied || isNotFound || isSignInis true).Those are optional polish items; the current tests are still useful for basic regression coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)libs/cloud-native-platform/tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/tests/publication-authorisation.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.ts : Do not create generic `types.ts` files. Colocate types with the appropriate code file where they are used.
Applied to files:
libs/cloud-native-platform/tsconfig.json
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.{ts,tsx} : TypeScript strict mode must be enabled. No `any` type without justification. Use explicit types for all variables and function parameters.
Applied to files:
libs/cloud-native-platform/tsconfig.json
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/publication-authorisation.spec.ts (1)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (1)
libs/cloud-native-platform/tsconfig.json (1)
6-8: No action required; strict mode is already enabled.Strict mode is inherited from the parent config at
tsconfig.json, which contains"strict": truein compilerOptions. The changes tolibs/cloud-native-platform/tsconfig.jsonproperly extend this parent configuration without overriding strict mode settings. The declaration generation settings (declaration: true,declarationMap: true,composite: true) are appropriate for publishing a TypeScript library in a monorepo structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/publication/src/authorisation/service.ts (1)
25-57: Remove redundant non-null assertion on line 52.At line 52,
user!.provenanceuses a non-null assertion, butuseris already guaranteed non-null at this point: the!usercheck at line 39 returns early, andisVerifiedUser(user)at line 50 also returns false for undefined users. The assertion is unnecessary.// CLASSIFIED publications require provenance matching if (sensitivity === Sensitivity.CLASSIFIED) { if (!isVerifiedUser(user)) return false; if (!listType) return false; // Fail closed if list type not found - return user!.provenance === listType.provenance; + return user.provenance === listType.provenance; }libs/publication/src/authorisation/middleware.ts (1)
22-51: Consider structured logging and simplify the ternary.Two minor suggestions:
- Line 47:
console.errorshould be replaced with structured logging for production observability.- Line 42: The ternary is redundant since both branches render the same template.
if (!checkAccess(req.user, artefact, listType)) { - return customErrorMessage ? res.status(403).render("errors/403", customErrorMessage) : res.status(403).render("errors/403"); + return res.status(403).render("errors/403", customErrorMessage); } next(); } catch (error) { - console.error("Error checking publication access:", error); + // TODO: Replace with structured logger (e.g., logger.error({ error, publicationId }, "Error checking publication access")) + console.error("Error checking publication access:", error); return res.status(500).render("errors/500"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/publication/src/authorisation/middleware.ts(1 hunks)libs/publication/src/authorisation/service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/publication/src/authorisation/middleware.tslibs/publication/src/authorisation/service.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/publication/src/authorisation/middleware.tslibs/publication/src/authorisation/service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/publication/src/authorisation/middleware.tslibs/publication/src/authorisation/service.ts
🧠 Learnings (1)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/libs/*/src/*-middleware.ts : Reusable middleware should be placed in `libs/[module]/src/[middleware-name]-middleware.ts` and exported as a function.
Applied to files:
libs/publication/src/authorisation/middleware.ts
🧬 Code graph analysis (2)
libs/publication/src/authorisation/middleware.ts (2)
libs/list-types/common/src/mock-list-types.ts (2)
ListType(1-8)mockListTypes(10-75)libs/publication/src/authorisation/service.ts (2)
canAccessPublication(25-57)canAccessPublicationData(67-81)
libs/publication/src/authorisation/service.ts (1)
libs/list-types/common/src/mock-list-types.ts (1)
ListType(1-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (8)
libs/publication/src/authorisation/service.ts (5)
1-7: LGTM! Clean module setup with proper imports and constants.Constants follow SCREAMING_SNAKE_CASE, imports use workspace aliases and
.jsextensions as required. Theas constassertions provide proper type narrowing for the role/provenance checks.
14-16: LGTM! Clean helper function.Good encapsulation of the provenance verification logic. Not exported, keeping it internal to this module.
67-81: LGTM! Clean role-based data access restriction.Good composition pattern - delegates to
canAccessPublicationafter applying the metadata-only role restriction.
91-111: LGTM! Metadata access logic is correct.Clear hierarchy: SYSTEM_ADMIN → METADATA_ONLY_ROLES → PUBLIC sensitivity → authenticated users. Appropriately allows broader metadata visibility while restricting actual data access.
120-125: LGTM! Functional filtering approach.Clean use of
filterwith inlinefindfor listType lookup. Immutable pattern as per coding guidelines.libs/publication/src/authorisation/middleware.ts (3)
1-14: LGTM! Clean imports and type definitions.Proper use of workspace aliases,
.jsextension for relative imports, and colocated types as per coding guidelines.
59-80: LGTM! Well-structured middleware exports with i18n support.Good factory pattern for middleware creation. The
requirePublicationDataAccessprovides appropriate Welsh translations for the 403 error message, supporting the bilingual requirement.
39-39: The naming is misleading but intentional.mockListTypesis the actual production source for list types, not a placeholder. There is no database or service alternative; this constant is used consistently across all production code in the system as the authoritative list type registry.Consider renaming to
LIST_TYPES(following SCREAMING_SNAKE_CASE for constants) to better reflect its role as the primary data source.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
e2e-tests/tests/publication-authorisation.spec.ts (3)
12-51: Tautological assertions do not validate PUBLIC-only visibility.The assertions
expect(count).toBeGreaterThanOrEqual(0)on lines 26 and 50 will always pass regardless of what content is shown. These tests don't actually verify that CLASSIFIED publications are hidden from unauthenticated users.Consider using stable test data to assert absence of known CLASSIFIED artefact titles, or comparing counts before/after authentication in a single test flow.
367-396: Add Axe-core scan, keyboard navigation, and Welsh language validation.Per coding guidelines, E2E tests must include accessibility checks using Axe-core, keyboard navigation verification, and Welsh translations. This test only verifies links have text content.
Add the missing checks:
import AxeBuilder from "@axe-core/playwright"; // After verifying links have text (line 395), add: // Axe-core accessibility scan const accessibilityScanResults = await new AxeBuilder({ page }).analyze(); expect(accessibilityScanResults.violations).toEqual([]); // Keyboard navigation - verify focus moves through interactive elements await page.keyboard.press("Tab"); const focusedElement = page.locator(":focus"); await expect(focusedElement).toBeVisible(); // Welsh language validation await page.goto("/summary-of-publications?locationId=9&lng=cy"); await page.waitForSelector("h1.govuk-heading-l"); const welshHeading = await page.locator("h1.govuk-heading-l").textContent(); expect(welshHeading).toBeTruthy(); // Verify Welsh-specific text appears const bodyText = await page.locator("body").textContent(); expect(bodyText).toMatch(/Crynodeb|Cyhoeddiad/); // Welsh termsBased on learnings, E2E tests require Axe-core accessibility testing and Welsh translations.
269-280: Tautological assertion does not verify provenance filtering.
expect(cftCount).toBeGreaterThanOrEqual(0)on line 274 always passes. To validate that CFT users see CFT CLASSIFIED publications, the test should assertcftCount > 0when test data guarantees such publications exist, or compare against a baseline.- // Should see CFT publications (if they exist for this location) - expect(cftCount).toBeGreaterThanOrEqual(0); + // Should see at least one CFT CLASSIFIED publication for this location + // Note: Requires stable test data at locationId=9 + expect(cftCount).toBeGreaterThan(0);
🧹 Nitpick comments (2)
e2e-tests/tests/publication-authorisation.spec.ts (2)
79-112: Extract repeated CFT login flow to reduce duplication.The CFT IDAM login sequence (navigate to sign-in → select radio → click continue → loginWithCftIdam → assertAuthenticated) is repeated verbatim in every test within this describe block. Consider using a
test.beforeEachhook or a dedicated helper function to reduce duplication.test.describe("CFT IDAM authenticated users (VERIFIED role with CFT provenance)", () => { test.beforeEach(async ({ page }) => { await page.goto("/sign-in"); const hmctsRadio = page.getByRole("radio", { name: /with a myhmcts account/i }); await hmctsRadio.check(); const continueButton = page.getByRole("button", { name: /continue/i }); await continueButton.click(); await loginWithCftIdam(page, process.env.CFT_VALID_TEST_ACCOUNT!, process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD!); await assertAuthenticated(page); }); test("should see PUBLIC, PRIVATE, and CLASSIFIED CFT publications", async ({ page }) => { // Navigate directly to summary page - already authenticated await page.goto("/summary-of-publications?locationId=9"); // ... rest of test }); });
466-515: Extract SSO login flow to reduce duplication across admin tests.The SSO login sequence is repeated in every test within the
@nightlydescribe blocks. Consider using atest.beforeEachhook for tests that share the same login credentials, or a parameterized helper.async function loginAsSsoUser( page: Page, email: string, password: string ): Promise<void> { await page.goto("/sign-in"); const ssoRadio = page.getByRole("radio", { name: /with a justice account/i }); await ssoRadio.check(); const continueButton = page.getByRole("button", { name: /continue/i }); await continueButton.click(); await loginWithSSO(page, email, password); await assertSsoAuthenticated(page); }This would reduce each test's login code to a single function call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/tests/publication-authorisation.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/publication-authorisation.spec.ts (2)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (1)
e2e-tests/tests/publication-authorisation.spec.ts (1)
1-9: LGTM!Imports correctly use ES modules with
.jsextensions for local imports, following the coding guidelines.
| // Verify can access CLASSIFIED publication | ||
| const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); | ||
| if (await classifiedLink.isVisible()) { | ||
| await classifiedLink.click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Should successfully access the publication | ||
| await expect(page).toHaveURL(/\/civil-and-family-daily-cause-list\?artefactId=/); | ||
| const bodyText = await page.locator("body").textContent(); | ||
| expect(bodyText).not.toContain("Access Denied"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Conditional check may silently pass without testing authorization.
The if (await classifiedLink.isVisible()) pattern means this test will pass without verifying anything if no CLASSIFIED publication exists. For a System Admin user who should have full access, consider asserting that the CLASSIFIED link IS visible before clicking it.
// Verify can access CLASSIFIED publication
const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first();
- if (await classifiedLink.isVisible()) {
- await classifiedLink.click();
+ // System admin should see CLASSIFIED publications - assert visibility
+ await expect(classifiedLink).toBeVisible({ timeout: 5000 });
+ await classifiedLink.click();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e-tests/tests/publication-authorisation.spec.ts around lines 424 to 435,
the current conditional if (await classifiedLink.isVisible()) allows the test to
silently pass when no CLASSIFIED publication exists; replace the conditional
with an explicit assertion that the CLASSIFIED link is visible (e.g.,
expect(classifiedLink).toBeVisible()) before clicking, then proceed to click,
wait for network idle and assert the URL and that the page body does not contain
"Access Denied" so the test fails if the link is absent and properly verifies
authorization for System Admin.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
e2e-tests/tests/publication-authorisation.spec.ts (5)
10-51: Tautological assertions do not validate PUBLIC-only access.The assertions
expect(count).toBeGreaterThanOrEqual(0)on lines 26 and 50 are always true. These tests will pass even if CLASSIFIED content is incorrectly exposed to unauthenticated users.
424-435: Conditional visibility check may skip authorization validation for System Admin.This was previously flagged. The
if (await classifiedLink.isVisible())pattern means the test passes without verifying System Admin access if no CLASSIFIED publication exists.
535-551: Conditional visibility check may skip authorization validation for CTSC Admin.This was previously flagged. The test passes without testing anything if the PRIVATE publication is not visible.
367-397: Missing Axe-core scan, keyboard navigation, and Welsh language testing.This was previously flagged. Per coding guidelines and retrieved learnings, E2E tests must include accessibility testing using Axe-core, keyboard navigation verification, and Welsh translation checks. The current test only verifies links have text content.
358-364: Overly permissive assertion for invalid locationId handling.This was previously flagged. The regex allows the test to pass if the user remains on the summary page, which would indicate the invalid
locationIdisn't being handled.
🧹 Nitpick comments (3)
e2e-tests/tests/publication-authorisation.spec.ts (3)
79-91: Extract repeated login sequences intobeforeEachhooks or shared helpers.The CFT IDAM login sequence (navigate to sign-in, select radio, click continue, call
loginWithCftIdam, assert authenticated) is repeated verbatim in nearly every test across this file (~15 occurrences). This violates DRY and makes the test suite fragile to login flow changes.Consider extracting this into a
beforeEachhook within each describe block, or creating a higher-level helper:// Example helper in e2e-tests/utils/cft-idam-helpers.ts export async function performCftIdamLogin(page: Page): Promise<void> { await page.goto("/sign-in"); const hmctsRadio = page.getByRole("radio", { name: /with a myhmcts account/i }); await hmctsRadio.check(); const continueButton = page.getByRole("button", { name: /continue/i }); await continueButton.click(); await loginWithCftIdam(page, process.env.CFT_VALID_TEST_ACCOUNT!, process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD!); await assertAuthenticated(page); }Then use
test.beforeEachin each describe block that requires authentication:test.describe("CFT IDAM authenticated users", () => { test.beforeEach(async ({ page }) => { await performCftIdamLogin(page); }); test("should see PUBLIC, PRIVATE, and CLASSIFIED CFT publications", async ({ page }) => { // Navigate directly to test page await page.goto("/summary-of-publications?locationId=9"); // ... rest of test }); });The same pattern applies to SSO login sequences for System Admin and Internal Admin tests.
5-9: Missing Welsh language journey testing.The coding guidelines and retrieved learnings require Welsh translation testing in E2E tests. While this file includes some Welsh text as fallback assertions (e.g., "Mynediad wedi'i Wrthod"), there's no dedicated test verifying the Welsh version of the publication summary page loads correctly with proper Welsh headings and content.
Consider adding a test that verifies Welsh language support:
test("should display summary page in Welsh", async ({ page }) => { // Login as CFT user await performCftIdamLogin(page); // Navigate to Welsh version of summary page await page.goto("/summary-of-publications?locationId=9&lng=cy"); await page.waitForSelector("h1.govuk-heading-l"); // Verify Welsh heading is displayed const heading = await page.locator("h1.govuk-heading-l").textContent(); expect(heading).toContain("Crynodeb o Gyhoeddiadau"); // Welsh translation // Verify publication links are accessible in Welsh context const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); await expect(publicationLinks.first()).toBeVisible(); });Based on retrieved learnings, every page must support both English and Welsh.
13-14: Document or seed test data requirements.Multiple tests rely on
locationId=9having specific publication types (PUBLIC, PRIVATE, CLASSIFIED with CFT provenance). The comments acknowledge uncertainty (line 24: "This assumes locationId=9 has at least some PUBLIC publications").Consider one of these approaches to improve test reliability:
- Document test data requirements - Add a comment at the top of the file listing required test data:
/** * Required test data for locationId=9: * - At least 1 PUBLIC publication (e.g., crown-daily-list) * - At least 1 PRIVATE publication (e.g., civil-daily-cause-list) * - At least 1 CLASSIFIED CFT publication (e.g., civil-and-family-daily-cause-list) */
Use test fixtures - Create publications as part of test setup using API calls, ensuring deterministic test data.
Skip with meaningful messages - Use
test.skip()with descriptive messages when required data is absent rather than silent conditionals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/tests/publication-authorisation.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/publication-authorisation.spec.ts (2)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (2)
e2e-tests/tests/publication-authorisation.spec.ts (2)
1-3: LGTM - Imports follow ESM conventions correctly.Proper use of
.jsextension for relative imports as required for ESM with Node.js.
215-248: Good test structure: before/after comparison for logout behavior.This test correctly captures the authenticated count, performs logout, and compares with the unauthenticated count. This pattern validates the session-based access control effectively.
| if (await publicLink.isVisible()) { | ||
| await publicLink.click(); | ||
| await page.waitForLoadState("networkidle"); | ||
|
|
||
| // Should successfully access PUBLIC publication data | ||
| const bodyText = await page.locator("body").textContent(); | ||
| expect(bodyText).not.toContain("Access Denied"); | ||
| expect(bodyText).not.toContain("You do not have permission to view the data"); | ||
| } |
There was a problem hiding this comment.
Conditional checks for PUBLIC publication access may skip verification.
The tests for CTSC Admin (lines 611-619) and Local Admin (lines 642-650) viewing PUBLIC publication data use if (await publicLink.isVisible()), which allows tests to pass without verifying anything. Since these tests validate that admin users can access PUBLIC data (positive case), assert the link is visible first.
// Look for PUBLIC publication (Crown Daily List or Crown Firm List)
const publicLink = page.locator('.govuk-list a[href*="crown-daily-list"], .govuk-list a[href*="crown-firm-list"]').first();
-
- if (await publicLink.isVisible()) {
+ await expect(publicLink).toBeVisible({ timeout: 5000 });
+
await publicLink.click();Also applies to: 642-650
🤖 Prompt for AI Agents
In e2e-tests/tests/publication-authorisation.spec.ts around lines 611-619 (and
similarly at 642-650), the test currently uses a conditional `if (await
publicLink.isVisible())` which allows the test to skip verification; change this
to assert the link is visible before proceeding (e.g., await the visibility
check and use an expect/assert to fail the test if not visible), then continue
with the click, waitForLoadState and the body content assertions so the positive
case is actually validated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e-tests/utils/seed-location-data.ts (1)
158-213: Make seeding of the fixed-ID artefact idempotentBecause the last artefact in
testArtefactssets an explicitartefactIdand the loop usesprisma.artefact.create, re-running this seeder against a DB that already contains that row will likely hit a unique-constraint error onartefactId. For seed scripts it’s usually safer to be idempotent.Consider switching this block to a create-or-update pattern (e.g.
upsertonartefactId) or usingcreateMany({ data: testArtefacts, skipDuplicates: true })so repeated runs don’t fail while still preserving a known ID for tests.Example change using
createMany:- console.log(`Creating ${testArtefacts.length} artefacts...`); - - for (const artefact of testArtefacts) { - const created = await prisma.artefact.create({ - data: artefact, - }); - console.log(` ✓ Created artefact ${created.artefactId}: listType=${artefact.listTypeId}, date=${artefact.contentDate.toISOString().split('T')[0]}`); - } + console.log(`Creating ${testArtefacts.length} artefacts...`); + + await prisma.artefact.createMany({ + data: testArtefacts, + skipDuplicates: true, + }); + for (const artefact of testArtefacts) { + console.log( + ` ✓ Seeded artefact for listType=${artefact.listTypeId}, date=${artefact.contentDate.toISOString().split("T")[0]}` + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)e2e-tests/utils/seed-location-data.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/tests/publication-authorisation.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/utils/seed-location-data.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/utils/seed-location-data.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/utils/seed-location-data.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/utils/seed-location-data.ts (1)
158-191: Provenance and list type coverage for test artefacts looks goodThe updated
listTypeIdcomments andprovenancevalues (CRIME_IDAM/CFT_IDAM) combined with differingsensitivityvalues give you a useful spread of scenarios for E2E access-control tests. No issues from a correctness or style perspective in this block.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
e2e-tests/tests/publication-authorisation.spec.ts (5)
356-386: Accessibility test is incomplete - missing Axe-core, keyboard navigation, and Welsh validation.This test only checks that links have text content, which is not a comprehensive accessibility test. Per coding guidelines and learnings, E2E tests must include:
- Axe-core accessibility scanning for WCAG 2.2 AA compliance
- Keyboard navigation verification
- Welsh translation validation
As flagged in previous review, add the following:
+import AxeBuilder from "@axe-core/playwright"; test("authenticated summary page should be accessible", async ({ page }) => { // ... existing login and navigation code ... await page.waitForSelector("h1.govuk-heading-l"); + // Axe-core accessibility scan + const accessibilityScanResults = await new AxeBuilder({ page }).analyze(); + expect(accessibilityScanResults.violations).toEqual([]); + // Keyboard navigation check + await page.keyboard.press("Tab"); + const focusedElement = page.locator(":focus"); + await expect(focusedElement).toBeVisible(); + // Welsh language validation + await page.goto("/summary-of-publications?locationId=9&lng=cy"); + await page.waitForSelector("h1.govuk-heading-l"); + const welshHeading = await page.locator("h1.govuk-heading-l").textContent(); + expect(welshHeading).toBeTruthy(); // ... existing link text checks ... });
407-418: Conditional visibility check may skip authorization validation for System Admin.The
if (await classifiedLink.isVisible())pattern allows this test to pass without verifying CLASSIFIED publication access. For a System Admin user who should have full access, assert the link IS visible before clicking.// Verify can access CLASSIFIED publication const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); - if (await classifiedLink.isVisible()) { - await classifiedLink.click(); + await expect(classifiedLink).toBeVisible({ timeout: 5000 }); + await classifiedLink.click();
494-510: Conditional visibility check may skip CTSC Admin authorization validation.The
if (await privateLink.isVisible())pattern allows this test to pass without verifying the expected behavior. If test data should include a PRIVATE publication at this location, assert visibility first to ensure the authorization check is actually exercised.
525-541: Conditional visibility check may skip Local Admin authorization validation.Same pattern as flagged for CTSC Admin and System Admin tests. The test passes without verification if the CLASSIFIED publication is not visible.
const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); - - if (await classifiedLink.isVisible()) { + await expect(classifiedLink).toBeVisible({ timeout: 5000 }); + await classifiedLink.click();
556-566: Conditional checks for PUBLIC publication access may skip positive verification.The tests for CTSC Admin (lines 556-566) and Local Admin (lines 581-591) viewing PUBLIC publication data use
if (await publicLink.isVisible()). Since these validate that admin users can access PUBLIC data (positive case), assert visibility first.const publicLink = page.locator('.govuk-list a[href*="crown-daily-list"], .govuk-list a[href*="crown-firm-list"]').first(); - - if (await publicLink.isVisible()) { + await expect(publicLink).toBeVisible({ timeout: 5000 }); + await publicLink.click();Also applies to lines 581-591.
🧹 Nitpick comments (3)
e2e-tests/tests/publication-authorisation.spec.ts (3)
71-83: Consider extracting common login setup to reduce duplication.The CFT IDAM login flow (lines 74-83) is repeated verbatim in every test within this describe block. Extract this to a
test.beforeEachhook or a shared fixture to improve maintainability and reduce the ~40 duplicated lines across this section alone.test.describe("CFT IDAM authenticated users (VERIFIED role with CFT provenance)", () => { test.beforeEach(async ({ page }) => { await page.goto("/sign-in"); const hmctsRadio = page.getByRole("radio", { name: /with a myhmcts account/i }); await hmctsRadio.check(); const continueButton = page.getByRole("button", { name: /continue/i }); await continueButton.click(); await loginWithCftIdam(page, process.env.CFT_VALID_TEST_ACCOUNT!, process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD!); await assertAuthenticated(page); }); test("should see PUBLIC, PRIVATE, and CLASSIFIED CFT publications", async ({ page }) => { // Navigate directly - already authenticated await page.goto("/summary-of-publications?locationId=9"); // ... rest of test }); // ... other tests });
81-81: Environment variables accessed without validation.Using non-null assertion (
!) onprocess.env.CFT_VALID_TEST_ACCOUNTandprocess.env.CFT_VALID_TEST_ACCOUNT_PASSWORDwill throw an unclear runtime error if these are undefined. Consider validating these at the top of the file or in a setup hook.// Add at file top or in a global setup const CFT_TEST_EMAIL = process.env.CFT_VALID_TEST_ACCOUNT; const CFT_TEST_PASSWORD = process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD; if (!CFT_TEST_EMAIL || !CFT_TEST_PASSWORD) { throw new Error("CFT_VALID_TEST_ACCOUNT and CFT_VALID_TEST_ACCOUNT_PASSWORD environment variables are required"); }
318-326: Consider adding explicit assertion for default CLASSIFIED behavior.The test description mentions "defaults to CLASSIFIED" but only verifies the page loads without errors. Consider adding an assertion that actually validates the expected filtering behavior when sensitivity is missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/tests/publication-authorisation.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/publication-authorisation.spec.ts (2)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (3)
e2e-tests/tests/publication-authorisation.spec.ts (3)
1-4: LGTM!Imports correctly use
.jsextensions for relative imports and follow ES module conventions as required by the coding guidelines.
53-68: Good authorization boundary test.This test properly validates the access denial behavior by attempting direct URL access to a CLASSIFIED publication and asserting the 403 response with bilingual support check.
274-314: Good provenance verification pattern.This test properly validates that authenticated CFT users can navigate to publications without encountering 403 errors, checking both URL patterns and body content for access denied messages.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
e2e-tests/tests/publication-authorisation.spec.ts (7)
12-51: Unauthenticated tests are effectively tautological and won’t catch PUBLIC‑only regressionsBoth unauthenticated tests only assert
count >= 0and (optionally) that a link has some text. They will still pass if PRIVATE/CLASSIFIED artefacts are exposed to unauthenticated users, so they don’t actually enforce “PUBLIC only” behaviour.Consider tightening them by, for example:
- Using stable seed data to assert specific CLASSIFIED artefacts are not present when unauthenticated (e.g. check the texts/URLs list does not contain known IDs/titles), and/or
- In one flow, measuring
unauthenticatedCountfirst, then logging in as a CFT user for the samelocationIdand assertingauthenticatedCount >= unauthenticatedCountand that expected CLASSIFIED entries appear only after login.This turns these into genuine regression tests for the unauthenticated surface, rather than simple page‑load checks.
106-140: CFT CLASSIFIED access test can silently pass without verifying anythingIn
should be able to access CLASSIFIED Civil and Family Daily Cause List, theif (count > 0)guard means the test does nothing (but still passes) when no matching publications exist. For a scenario that is meant to guarantee a CFT user can access CLASSIFIED Civil & Family artefacts, this weakens the check.Suggest either:
- Fail fast when no such publications exist (e.g. assert
count > 0), or- Explicitly skip the test with a clear reason when
count === 0, so the report reflects missing seed data instead of a false pass.
347-353: InvalidlocationIdtest uses overly permissive URL assertion
expect(currentUrl).toMatch(/\/400|\/summary-of-publications/);passes both when:
- The app correctly redirects to a 400 page, and
- When it stays on
/summary-of-publicationsand does nothing for invalid IDs.That makes it impossible for this test to catch regressions where invalid
locationIdis silently ignored.Prefer a stricter check, for example:
- Assert the URL actually ends with the 400 route (e.g.
/400), or- If staying on the summary page is the intended UX, assert the presence of a visible error banner/message in the body instead of allowing either URL without content checks.
356-385: Accessibility test missing Axe scan, keyboard nav, and explicit Welsh UX checksThe
"authenticated summary page should be accessible"test currently only verifies that a few links have non‑empty text. Per the repo guidelines and earlier review, this should also:
- Run an Axe-core scan for WCAG 2.2 AA violations (via
@axe-core/playwright).- Include at least a basic keyboard navigation check (e.g. tabbing through heading and first few publication links and asserting focus progression).
- Exercise the Welsh variant (or language toggle) and assert key headings/labels appear in Welsh, not just allow Welsh error strings.
A minimal change would be to:
- import { expect, test } from "@playwright/test"; + import { expect, test } from "@playwright/test"; + import AxeBuilder from "@axe-core/playwright"; @@ - // Basic accessibility check - all links should have text + // Basic accessibility check - all links should have text const count = await publicationLinks.count(); for (let i = 0; i < Math.min(count, 3); i++) { const linkText = await publicationLinks.nth(i).textContent(); expect(linkText).toBeTruthy(); expect(linkText?.trim().length).toBeGreaterThan(0); } + + // Axe-core scan for WCAG 2.2 AA issues + const axeResults = await new AxeBuilder({ page }).analyze(); + expect(axeResults.violations).toEqual([]); + + // Simple keyboard navigation check (e.g. Tab to first publication link) + await page.keyboard.press("Tab"); // focus main heading + await page.keyboard.press("Tab"); // focus first publication link + await expect(publicationLinks.first()).toBeFocused(); + + // Welsh variant – navigate or toggle and assert a known Welsh heading/text + // (Example selector/text – adjust to actual implementation) + // await page.goto("/cy/summary-of-publications?locationId=9"); + // await expect(page.getByRole("heading", { name: /Crynodeb o gyhoeddiadau/i })).toBeVisible();Please adapt selectors/text to the app’s actual Welsh content and existing Axe usage patterns in this repo.
396-419: SYSTEM_ADMIN CLASSIFIED access check can be skipped entirelyIn
"should have full access to all publications", the critical verification is wrapped in:const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); if (await classifiedLink.isVisible()) { // click & assert access }If no such link is visible, the test still passes without asserting anything about CLASSIFIED access, despite the test name promising “full access”.
Instead, either:
- Assert that the link is visible (or that there is at least one CLASSIFIED entry) and then click it, or
- Explicitly skip the test with a descriptive message when the expected seed data is missing.
That way, missing/renamed CLASSIFIED artefacts cause a clear failure/skip, not a silent pass.
460-502: CTSC Admin PRIVATE/PUBLIC access tests may pass without exercising the pathsBoth CTSC Admin tests use
if (await privateLink.isVisible())/if (await publicLink.isVisible()) { ... }. If the expected PRIVATE or PUBLIC link is missing, the tests do nothing and pass, so they don’t reliably validate:
- That CTSC can see PRIVATE/PUBLIC artefacts in the list, and
- That PRIVATE data access is blocked vs PUBLIC data allowed.
Recommend:
- Asserting visibility (or non‑zero count) of the expected links up front, failing or skipping if seed data isn’t present; and
- Keeping the access‑denied / allowed body‑text assertions unconditional once that precondition holds.
This makes these role‑based behaviours genuinely enforced by the tests.
526-568: Local Admin CLASSIFIED/PUBLIC checks use same conditional pattern and can silently skip behaviourThe Local Admin tests mirror the CTSC ones, with:
if (await classifiedLink.isVisible()) { ... } ... if (await publicLink.isVisible()) { ... }As above, if the specific CLASSIFIED or PUBLIC publications are absent or renamed, the tests pass without asserting anything about:
- Data‑access denial for CLASSIFIED, and
- Successful access for PUBLIC.
It would be more robust to:
- Assert that the relevant links exist (or explicitly skip when test data is missing), and
- Always run the access‑denied / success assertions once that’s true.
🧹 Nitpick comments (1)
e2e-tests/tests/publication-authorisation.spec.ts (1)
53-68: Two direct‑access tests duplicate the same CLASSIFIED 403 scenario
"should show access denied when trying to directly access a CLASSIFIED publication"and"should show appropriate error when accessing restricted publication directly"both:
- Navigate to the same
/civil-and-family-daily-cause-list?artefactId=000...001URL unauthenticated, and- Assert the same “Access Denied / Mynediad wedi'i Wrthod” heading.
This duplication doesn’t add coverage and costs runtime/maintenance.
You could consolidate into a single test (or a small helper) that exercises this flow once, and reuse it where needed.
Also applies to: 328-345
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/tests/publication-authorisation.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/publication-authorisation.spec.ts (2)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
| test.describe("Provenance-based filtering for CLASSIFIED publications", () => { | ||
| test("CFT user should see CFT CLASSIFIED publications", async ({ page }) => { | ||
| // Login as CFT user | ||
| await page.goto("/sign-in"); | ||
|
|
||
| const hmctsRadio = page.getByRole("radio", { name: /with a myhmcts account/i }); | ||
| await hmctsRadio.check(); | ||
| const continueButton = page.getByRole("button", { name: /continue/i }); | ||
| await continueButton.click(); | ||
|
|
||
| await loginWithCftIdam(page, process.env.CFT_VALID_TEST_ACCOUNT!, process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD!); | ||
|
|
||
| await assertAuthenticated(page); | ||
|
|
||
| // Navigate to location with CFT CLASSIFIED publications | ||
| await page.goto("/summary-of-publications?locationId=9"); | ||
| await page.waitForSelector("h1.govuk-heading-l"); | ||
|
|
||
| // Look specifically for CFT list types (Civil and Family) | ||
| const cftLinks = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]'); | ||
| const cftCount = await cftLinks.count(); | ||
|
|
||
| // Should see CFT publications (if they exist for this location) | ||
| expect(cftCount).toBeGreaterThanOrEqual(0); | ||
|
|
||
| if (cftCount > 0) { | ||
| const linkText = await cftLinks.first().textContent(); | ||
| expect(linkText).toContain("Civil"); | ||
| } | ||
| }); | ||
|
|
||
| test("should verify CLASSIFIED publications match user provenance", async ({ page }) => { | ||
| // Login as CFT user | ||
| await page.goto("/sign-in"); | ||
|
|
||
| const hmctsRadio = page.getByRole("radio", { name: /with a myhmcts account/i }); | ||
| await hmctsRadio.check(); | ||
| const continueButton = page.getByRole("button", { name: /continue/i }); | ||
| await continueButton.click(); | ||
|
|
||
| await loginWithCftIdam(page, process.env.CFT_VALID_TEST_ACCOUNT!, process.env.CFT_VALID_TEST_ACCOUNT_PASSWORD!); | ||
|
|
||
| await assertAuthenticated(page); | ||
|
|
||
| // Navigate to summary page | ||
| await page.goto("/summary-of-publications?locationId=9"); | ||
| await page.waitForSelector("h1.govuk-heading-l"); | ||
|
|
||
| const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); | ||
| const count = await publicationLinks.count(); | ||
|
|
||
| if (count > 0) { | ||
| // Verify we can access CFT publications | ||
| const firstLink = publicationLinks.first(); | ||
|
|
||
| // Click the link | ||
| await firstLink.click(); | ||
|
|
||
| // Should successfully navigate (not 403) | ||
| await page.waitForLoadState("networkidle"); | ||
| const currentUrl = page.url(); | ||
|
|
||
| // Should be on a list type page, not an error page | ||
| expect(currentUrl).toMatch(/artefactId=/); | ||
| expect(currentUrl).not.toContain("/403"); | ||
| expect(currentUrl).not.toContain("/sign-in"); | ||
|
|
||
| // Should not see access denied message | ||
| const bodyText = await page.locator("body").textContent(); | ||
| expect(bodyText).not.toContain("Access Denied"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Provenance tests don’t actually assert provenance‑based behaviour
The two tests under "Provenance-based filtering for CLASSIFIED publications" currently:
- Assert
cftCount >= 0(which is always true) and only check that the first CFT link’s text contains"Civil". - In
should verify CLASSIFIED publications match user provenance, click the first generic artefact link and only assert “not 403 / not sign‑in / no access denied”.
Neither test validates that the CLASSIFIED entries shown match the user’s provenance (e.g. CFT vs CRIME) or that other provenances are excluded.
Consider strengthening these by, for example:
- Targeting known CFT‑provenance artefacts (e.g. via predictable path/query or seed titles) and asserting they are present; and
- Asserting that artefacts with non‑CFT provenance (if present in data) are not visible/accessible for this user, or are handled with metadata‑only / 403 semantics as expected.
Right now the names over‑promise compared to what’s actually being validated.
🤖 Prompt for AI Agents
In e2e-tests/tests/publication-authorisation.spec.ts around lines 243 to 314,
the two provenance tests only perform weak checks (cftCount >= 0 and generic
“not 403” navigation) and do not verify provenance-based behaviour; update the
tests to (1) assert cftCount > 0 (or seed/assert presence of a known CFT
artefact title/artefactId) so the positive case actually verifies CFT entries
exist, (2) filter or locate artefact links by provenance (e.g. link text, known
artefactId, or a provenance data-attribute) and after clicking an artefact
assert the page metadata/provenance field equals "CFT", and (3) add a negative
assertion that non-CFT artefacts are not visible or, if present, result in the
expected restricted behaviour (e.g. absent from list or access yields 403 /
shows metadata-only notice); implement these checks rather than the current
generic navigation/no-error assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
e2e-tests/tests/publication-authorisation.spec.ts (5)
10-51: Unauthenticated tests are tautological and don’t prove “PUBLIC‑only” behaviourBoth unauthenticated specs effectively only assert that the page loads:
expect(count).toBeGreaterThanOrEqual(0);(Line 26 & 50) is always true.- The optional
if (count > 0)block (Lines 30–33) only checks that the first link has text, not that PRIVATE/CLASSIFIED artefacts are absent.These tests would still pass if CLASSIFIED/CFT artefacts leaked to unauthenticated users.
Consider tightening them by, for example:
- Explicitly asserting that no CLASSIFIED links are present for unauthenticated users, e.g. checking that
.govuk-list a[href*="civil-and-family-daily-cause-list"]has count0; and/or- In a single flow, recording
unauthenticatedCountforlocationId=9, then logging in as a CFT user, re‑fetching the same locator and asserting:
authenticatedCount >= unauthenticatedCount, and- the presence of at least one CFT‑only artefact link (e.g. the Civil and Family Daily Cause List) only after login.
This would turn these into real regression tests for PUBLIC‑only visibility instead of smoke tests.
106-140:if (count > 0)guards allow key CLASSIFIED/provenance tests to silently passIn:
"should be able to access CLASSIFIED Civil and Family Daily Cause List"(Lines 106–140), and"Provenance-based filtering for CLASSIFIED publications"tests (Lines 243–314),all meaningful checks are gated behind
if (count > 0)orif (cftCount > 0), and in one case you still assertcftCount >= 0(Line 266), which is tautological.If seed data is missing or mis‑configured, these tests will pass without verifying:
- that CFT users can actually access CLASSIFIED CFT artefacts, or
- that provenance‑based filtering is working as intended.
To make them robust:
- After computing
count/cftCount, either:
- Assert strictly:
expect(count).toBeGreaterThan(0);(or similar forcftCount), then proceed with navigation and access checks, or- Use
test.skip(count === 0, "Requires CLASSIFIED Civil and Family publications at locationId=9");so the runner reports missing data explicitly rather than silently passing.- For the provenance test, consider also asserting that the page you land on exposes the expected provenance (e.g. a provenance field or data attribute) and that non‑CFT provenances are not shown for a CFT user.
This will ensure these specs actually validate the provenance‑driven logic added in VIBE‑247.
Also applies to: 243-314
347-353: InvalidlocationIdassertion is too permissive to catch missing error handling
expect(currentUrl).toMatch(/\/400|\/summary-of-publications/);(Line 352) lets the test pass even if the app simply stays on the summary page and shows no error, which wouldn’t exercise the invalid‑locationIdpath at all.Consider tightening this to assert the actual expected outcome, for example:
- If invalid IDs must redirect to a 400 page:
expect(currentUrl).toMatch(/\/400$/);, or- If remaining on the same page is valid, assert a concrete error UI instead (e.g. locate an error banner/message and
expect(...).toBeVisible()/toContainText("Invalid location")or similar).That way, missing or broken error handling can’t slip through.
356-385: Accessibility test lacks Axe‑core scan, keyboard navigation, and explicit Welsh coverageThe
"authenticated summary page should be accessible"test currently only checks that a few links have non‑empty text. Per the E2E guidelines and learnings for this repo, these tests must:
- Run an Axe‑core scan for WCAG 2.2 AA,
- Exercise basic keyboard navigation, and
- Validate Welsh translations as well as English, ideally within the same scenario. (Based on learnings, …)
Concrete improvements:
Import Axe‑core (aligned with patterns in other e2e tests in this repo):
import AxeBuilder from "@axe-core/playwright";After confirming the summary page has loaded and links are visible, run an Axe scan and assert no violations (or at least no unexpected ones), e.g.:
const axeResults = await new AxeBuilder({ page }).analyze(); expect(axeResults.violations.length).toBe(0);(Match the exact
AxeBuilderusage already used elsewhere ine2e-tests.)Add a short keyboard‑navigation check: tab through the page and assert focus moves to the main heading and then to the first publication link (e.g. using
page.keyboard.press("Tab")andawait expect(headingOrLink).toBeFocused()).Explicitly exercise the Welsh variant of the page (e.g. navigate with
lng=cyor via the language toggle) and assert that the expected Welsh heading/labels are present, mirroring the English assertions.This will bring the spec in line with the project’s WCAG and Welsh/i18n E2E requirements.
Check the recommended usage pattern for `@axe-core/playwright` with Playwright test (ESM import style) so this test can follow best practices.
396-419:if (await link.isVisible())patterns can skip System/CTSC/Local admin authorisation checksIn several admin tests:
- System Admin
"should have full access to all publications"(Lines 396–419),- CTSC Admin
"can view PUBLIC publication data"(Lines 486–503), and- Local Admin
"can view PUBLIC publication data"(Lines 553–569),core assertions are wrapped in
if (await classifiedLink.isVisible())/if (await publicLink.isVisible()). If the expected link isn’t visible—because of data issues or a regression—these tests will quietly pass without verifying the intended access rules.For positive cases (admin users should be able to see/use these links), it’s better to assert presence:
const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); await expect(classifiedLink).toBeVisible({ timeout: 5000 }); await classifiedLink.click(); // ... existing URL and “no Access Denied” assertionsand similarly for the PUBLIC link locators.
For genuinely optional data, prefer an explicit
test.skipwith a clear reason instead of silently returning, so the runner reports that coverage was skipped rather than passing a non‑executed assertion block.This makes these high‑privilege role tests reliable indicators of authorisation regressions.
Also applies to: 486-503, 553-569
🧹 Nitpick comments (2)
e2e-tests/tests/publication-authorisation.spec.ts (2)
53-68: Duplicate direct‑access 403 tests – consider consolidating
"should show access denied when trying to directly access a CLASSIFIED publication"(Lines 53–68) and"should show appropriate error when accessing restricted publication directly"(Lines 328–345) exercise the same URL and assert the same “Access Denied” heading.You can reduce test runtime and maintenance by consolidating to a single direct‑access 403 scenario (or extracting a small helper that both suites call). Behavioural coverage is unchanged either way.
Also applies to: 328-345
71-241: CFT login flows and role journeys are very repetitive – consider a shared helper/beforeEachThe CFT‑user and provenance tests inline the same sign‑in flow (radio selection, continue,
loginWithCftIdam,assertAuthenticated) in multiple specs.Not a correctness issue, but extracting a shared
test.beforeEachwithin the relevantdescribeblocks—or a small helper likeasync function loginAsCftUser(page)—would:
- Reduce duplication,
- Make it easier to update the journey if the sign‑in UI changes, and
- Keep each test focused on its specific authorisation assertions.
Given how many scenarios depend on CFT login, this refactor would pay off quickly.
Also applies to: 243-315
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/tests/publication-authorisation.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
🧬 Code graph analysis (1)
e2e-tests/tests/publication-authorisation.spec.ts (2)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
e2e-tests/tests/publication-authorisation.spec.ts (7)
12-51: Unauthenticated summary tests don’t actually assert “PUBLIC only” behaviourBoth unauthenticated tests are effectively smoke tests:
expect(count).toBeGreaterThanOrEqual(0)is tautological and no assertions are made about PRIVATE/CLASSIFIED items being absent. This means the tests would still pass if restricted artefacts leaked to unauthenticated users.Please strengthen these by asserting on concrete selectors/artefacts, for example:
- In
should only see PUBLIC publications, assert there is at least one publication and that known PRIVATE/CLASSIFIED list routes are not present in the list (e.g.civil-daily-cause-list,civil-and-family-daily-cause-list), and/or- In
should not see CLASSIFIED Civil and Family publications, assert that the classified selector count is zero and, ideally, compare against an authenticated baseline in the CFT user tests.This will turn these into real regression tests for PUBLIC-only access rather than simple page‑load checks.
- const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); - const count = await publicationLinks.count(); - - // Verify that publications are visible (PUBLIC ones should be available) - // Note: This assumes locationId=9 has at least some PUBLIC publications - // If no PUBLIC publications exist, count would be 0, which is correct behavior - expect(count).toBeGreaterThanOrEqual(0); - - // If publications exist, verify they don't include sensitive data indicators - // (This is a smoke test - the real verification is that PRIVATE/CLASSIFIED don't appear) - if (count > 0) { - const firstLinkText = await publicationLinks.first().textContent(); - expect(firstLinkText).toBeTruthy(); - } + const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); + const count = await publicationLinks.count(); + + // Seeded data for locationId=9 should expose at least one PUBLIC publication + expect(count).toBeGreaterThan(0); + + // Unauthenticated users must not see PRIVATE or CLASSIFIED list types + const classifiedLinks = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]'); + const privateLinks = page.locator('.govuk-list a[href*="civil-daily-cause-list"]'); + expect(await classifiedLinks.count()).toBe(0); + expect(await privateLinks.count()).toBe(0); @@ - // Get all publication links - const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); - const count = await publicationLinks.count(); - - // Verify CLASSIFIED publications are filtered out - // We can't directly check for absence of specific publications, - // but we verify the count is less than what an authenticated CFT user would see - // This is validated in combination with the authenticated user tests below - expect(count).toBeGreaterThanOrEqual(0); + // Unauthenticated users must not see CLASSIFIED Civil and Family publications + const classifiedLinks = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]'); + const classifiedCount = await classifiedLinks.count(); + expect(classifiedCount).toBe(0);
106-140: CLASSIFIED access test can silently pass when no data exists
should be able to access CLASSIFIED Civil and Family Daily Cause Listonly runs its assertions insideif (count > 0). If no matching artefacts are found, the test passes without verifying anything.Make the expectation explicit so the test fails (or is skipped) when the seed data isn’t present.
- const cftPublicationLinks = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]'); - const count = await cftPublicationLinks.count(); - - // If CLASSIFIED Civil and Family publications exist for this location, they should be visible - if (count > 0) { - // Click on the first CFT publication - await cftPublicationLinks.first().click(); + const cftPublicationLinks = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]'); + const count = await cftPublicationLinks.count(); + + // Test assumes at least one CLASSIFIED Civil and Family publication at this location + expect(count).toBeGreaterThan(0); + + // Click on the first CFT publication + await cftPublicationLinks.first().click(); @@ - expect(accessDeniedText).not.toContain("Access Denied"); - expect(accessDeniedText).not.toContain("Mynediad wedi'i Wrthod"); - } + expect(accessDeniedText).not.toContain("Access Denied"); + expect(accessDeniedText).not.toContain("Mynediad wedi'i Wrthod");
243-314: Provenance tests don’t currently assert provenance‑based behaviourThe
"Provenance-based filtering for CLASSIFIED publications"tests still:
- Allow
cftCount >= 0with an optional text check, and- In
should verify CLASSIFIED publications match user provenance, just click the first artefact and assert “not 403 / not sign-in / no access denied”.They don’t actually confirm that:
- CFT‑provenance CLASSIFIED artefacts are present for this user, and
- Non‑matching provenances are excluded or handled differently.
Please tighten these tests, for example by:
- Asserting
cftCount > 0and always validating the first CFT link, and- Targeting a known CFT artefact (e.g.
civil-and-family-daily-cause-list) to assert successful access, while also asserting that non‑CFT artefacts (if seeded) are either not visible or result in the expected restricted behaviour.Right now the test names over‑promise compared to what is actually validated.
- const cftLinks = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]'); - const cftCount = await cftLinks.count(); - - // Should see CFT publications (if they exist for this location) - expect(cftCount).toBeGreaterThanOrEqual(0); - - if (cftCount > 0) { - const linkText = await cftLinks.first().textContent(); - expect(linkText).toContain("Civil"); - } + const cftLinks = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]'); + const cftCount = await cftLinks.count(); + + // Seeded data should expose at least one CFT CLASSIFIED publication + expect(cftCount).toBeGreaterThan(0); + + const linkText = await cftLinks.first().textContent(); + expect(linkText).toContain("Civil"); @@ - const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); - const count = await publicationLinks.count(); - - if (count > 0) { - // Verify we can access CFT publications - const firstLink = publicationLinks.first(); - - // Click the link - await firstLink.click(); + // Focus this test on a known CFT CLASSIFIED artefact for provenance checks + const cftLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); + await cftLink.click(); @@ - expect(currentUrl).toMatch(/artefactId=/); + expect(currentUrl).toMatch(/civil-and-family-daily-cause-list\?artefactId=/); expect(currentUrl).not.toContain("/403"); expect(currentUrl).not.toContain("/sign-in"); @@ - const bodyText = await page.locator("body").textContent(); - expect(bodyText).not.toContain("Access Denied"); - } + const bodyText = await page.locator("body").textContent(); + expect(bodyText).not.toContain("Access Denied");
347-353: Invalid locationId test is too permissive for current controller behaviour
GET /summary-of-publicationsnow redirects to/400for any missing/invalid/unknownlocationId. The assertion/\/400|\/summary-of-publications/would still pass if the app simply stayed on the summary page without showing an error.Given the controller always redirects to
/400in these cases, this test should assert that behaviour explicitly.- // Should redirect to 400 error page or show appropriate error - const currentUrl = page.url(); - expect(currentUrl).toMatch(/\/400|\/summary-of-publications/); + // Should redirect to 400 error page + const currentUrl = page.url(); + expect(currentUrl).toMatch(/\/400$/);
356-385: Accessibility test missing Axe-core scan, keyboard nav, and Welsh coverageThe
"authenticated summary page should be accessible"test only checks that a few links have non‑empty text. Per the CLAUDE/i18n/WCAG guidance, this E2E spec should:
- Run an Axe-core scan for WCAG 2.2 AA,
- Include a basic keyboard navigation check, and
- Validate key Welsh translations on the same journey.
Please extend this test accordingly, and import AxeBuilder, for example:
-import { expect, test } from "@playwright/test"; +import { expect, test } from "@playwright/test"; +import AxeBuilder from "@axe-core/playwright"; @@ test("authenticated summary page should be accessible", async ({ page }) => { @@ const publicationLinks = page.locator('.govuk-list a[href*="artefactId="]'); await expect(publicationLinks.first()).toBeVisible(); @@ const count = await publicationLinks.count(); for (let i = 0; i < Math.min(count, 3); i++) { const linkText = await publicationLinks.nth(i).textContent(); expect(linkText).toBeTruthy(); expect(linkText?.trim().length).toBeGreaterThan(0); } + + // Axe-core WCAG 2.2 AA scan + const axeResults = await new AxeBuilder({ page }).analyze(); + expect(axeResults.violations.length).toBe(0); + + // Basic keyboard navigation: tab through to the first publication link + await page.keyboard.press("Tab"); // skip link / header + await page.keyboard.press("Tab"); // into main content + await expect(publicationLinks.first()).toBeFocused(); + + // Welsh variant: load the same page in Welsh and check key text + await page.goto("/summary-of-publications?locationId=9&lng=cy"); + await page.waitForSelector("h1.govuk-heading-l"); + const welshHeading = await page.locator("h1.govuk-heading-l").textContent(); + expect(welshHeading).toBeTruthy(); + // Optionally assert specific Welsh strings from summary-of-publications/cy.tsPlease double‑check the exact AxeBuilder usage and Welsh strings against your versions of
@axe-core/playwrightandcy.ts.
396-419: System Admin CLASSIFIED access test can skip verification when link is missing
should have full access to all publicationswraps the CLASSIFIED access checks inif (await classifiedLink.isVisible()). If that link is ever missing, the test passes without validating System Admin access to CLASSIFIED artefacts.For a positive “full access” scenario, it’s better to fail fast when the expected CLASSIFIED artefact is missing.
- // Verify can access CLASSIFIED publication - const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); - if (await classifiedLink.isVisible()) { - await classifiedLink.click(); + // Verify can access CLASSIFIED publication + const classifiedLink = page.locator('.govuk-list a[href*="civil-and-family-daily-cause-list"]').first(); + await expect(classifiedLink).toBeVisible({ timeout: 5000 }); + await classifiedLink.click(); await page.waitForLoadState("networkidle"); @@ - const bodyText = await page.locator("body").textContent(); - expect(bodyText).not.toContain("Access Denied"); - } + const bodyText = await page.locator("body").textContent(); + expect(bodyText).not.toContain("Access Denied");
465-482: CTSC/Local Admin PUBLIC access tests may silently skip assertionsThe CTSC and Local Admin
"can view PUBLIC publication data"tests wrap all checks inif (await publicLink.isVisible()). If no PUBLIC Crown list is found, the tests pass without validating anything, even though the scenario name is a positive “can view” case.Make the presence of a PUBLIC artefact an explicit requirement so regressions in PUBLIC visibility for these roles are caught.
- // Look for PUBLIC publication (Crown Daily List or Crown Firm List) - const publicLink = page.locator('.govuk-list a[href*="crown-daily-list"], .govuk-list a[href*="crown-firm-list"]').first(); - - if (await publicLink.isVisible()) { - await publicLink.click(); + // Look for PUBLIC publication (Crown Daily List or Crown Firm List) + const publicLink = page.locator('.govuk-list a[href*="crown-daily-list"], .govuk-list a[href*="crown-firm-list"]').first(); + await expect(publicLink).toBeVisible({ timeout: 5000 }); + await publicLink.click(); @@ - expect(bodyText).not.toContain("Access Denied"); - expect(bodyText).not.toContain("You do not have permission to view the data"); - } + expect(bodyText).not.toContain("Access Denied"); + expect(bodyText).not.toContain("You do not have permission to view the data");Also applies to: 511-528
🧹 Nitpick comments (2)
libs/public-pages/src/pages/summary-of-publications/index.ts (1)
36-48: Clarify comment around metadata‑level access in summary filtering
filterPublicationsForSummarycorrectly delegates tocanAccessPublicationMetadata, which (per service.ts) allows:
- SYSTEM_ADMIN to see all metadata,
- INTERNAL_ADMIN_CTSC / INTERNAL_ADMIN_LOCAL to see only PUBLIC metadata, and
- Other users to follow standard sensitivity/provenance rules.
The current comment (“allows admins to see CLASSIFIED publications in the summary even if they can't view the data”) doesn’t quite reflect that behaviour for CTSC/Local admins and may confuse future readers.
Consider rephrasing to describe the actual metadata rules, e.g. that summary visibility is driven by
canAccessPublicationMetadataand metadata‑only roles still only see PUBLIC entries.- // Filter artefacts based on user metadata access rights - // This allows admins to see CLASSIFIED publications in the summary even if they can't view the data + // Filter artefacts based on user metadata access rights + // Summary visibility is driven by canAccessPublicationMetadata: + // - System admins can see all publications + // - CTSC/Local admins see PUBLIC metadata only + // - Other users follow standard sensitivity/provenance rules const artefacts = filterPublicationsForSummary(req.user, allArtefacts, mockListTypes);libs/publication/src/authorisation/service.ts (1)
6-16: Verify role/provenance constants stay in sync with @hmcts/auth UserProfileThe logic in
isVerifiedUser,canAccessPublication, andcanAccessPublicationData/Metadatahinges on hard‑coded values in:
METADATA_ONLY_ROLES = ["INTERNAL_ADMIN_CTSC", "INTERNAL_ADMIN_LOCAL"], andVERIFIED_USER_PROVENANCES = ["B2C_IDAM", "CFT_IDAM", "CRIME_IDAM"].This is fine today, but if new roles or provenances are added to
UserProfileand not reflected here, those users will silently fall back to “unverified” or “no metadata” behaviour.It’s worth double‑checking these arrays against the authoritative role/provenance list in
@hmcts/auth, and considering a shared enum or helper there to avoid divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e-tests/tests/publication-authorisation.spec.ts(1 hunks)e2e-tests/utils/seed-location-data.ts(2 hunks)libs/public-pages/src/pages/summary-of-publications/index.ts(3 hunks)libs/publication/src/authorisation/service.ts(1 hunks)libs/publication/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/publication-authorisation.spec.tse2e-tests/utils/seed-location-data.tslibs/publication/src/authorisation/service.tslibs/public-pages/src/pages/summary-of-publications/index.tslibs/publication/src/index.ts
e2e-tests/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
e2e-tests/**/*.spec.ts: E2E test files must be ine2e-tests/directory named*.spec.ts, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Files:
e2e-tests/tests/publication-authorisation.spec.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
e2e-tests/tests/publication-authorisation.spec.tse2e-tests/utils/seed-location-data.tslibs/publication/src/authorisation/service.tslibs/public-pages/src/pages/summary-of-publications/index.tslibs/publication/src/index.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
e2e-tests/tests/publication-authorisation.spec.tse2e-tests/utils/seed-location-data.tslibs/publication/src/authorisation/service.tslibs/public-pages/src/pages/summary-of-publications/index.tslibs/publication/src/index.ts
**/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.ts: Pages are registered through explicit imports inapps/web/src/app.ts. Routes are created based on file names within thepages/directory (e.g.,my-page.tsbecomes/my-page, nested routes via subdirectories).
Page controller files must exportGETand/orPOSTfunctions that accept Express Request and Response, render usingres.render(), and handle form submissions.
Files:
libs/public-pages/src/pages/summary-of-publications/index.ts
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/public-pages/src/pages/summary-of-publications/index.ts
🧠 Learnings (4)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : WCAG 2.2 AA accessibility compliance is mandatory. Include accessibility testing in E2E tests using Axe-core.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
e2e-tests/tests/publication-authorisation.spec.ts
🧬 Code graph analysis (2)
e2e-tests/tests/publication-authorisation.spec.ts (2)
e2e-tests/utils/cft-idam-helpers.ts (1)
loginWithCftIdam(10-41)e2e-tests/utils/sso-helpers.ts (1)
loginWithSSO(10-46)
libs/publication/src/authorisation/service.ts (1)
libs/list-types/common/src/mock-list-types.ts (1)
ListType(1-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (7)
e2e-tests/utils/seed-location-data.ts (3)
1-1: LGTM!Using the
node:prefix for Node.js built-in modules is the correct ESM-compatible approach.
193-204: Well-structured test data for authorization testing.The hardcoded
artefactIdwith a known value is appropriate for E2E tests that need to reference this specific artefact. The CLASSIFIED sensitivity combined with CFT_IDAM provenance aligns well with the authentication/authorization changes in this PR.
210-216: No action needed. Theprisma.artefact.upsertcall without type casting is correct and consistent with usage throughout the codebase. Both theArtefactmodel (inapps/postgres/prisma/schema.prisma) and other models likeLocationandLocationSubJurisdiction(inlibs/location/prisma/schema.prisma) are properly defined in their Prisma schemas. Theartefactmodel is used withoutas anycasts in production code (e.g.,libs/publication/src/repository/queries.ts,libs/public-pages/src/pages/publication/[id].ts,apps/postgres/prisma/seed.ts), so this implementation is intentional and correct.libs/publication/src/index.ts (1)
2-9: Authorisation exports look consistent and ESM‑safeThe new exports for middleware and service functions use
.jsextensions and align with the centralised authorisation design. This keeps the public API coherent for downstream consumers.libs/publication/src/authorisation/service.ts (3)
25-57: Access rules are sensible and fail‑closed for missing sensitivity/list type
canAccessPublication:
- Treats missing
sensitivityasCLASSIFIED(fail‑closed),- Gives SYSTEM_ADMIN full access,
- Allows PUBLIC to everyone,
- Requires a verified user for PRIVATE, and
- For CLASSIFIED, additionally requires a matching
listType.provenance, failing closed whenlistTypeis not found.This matches the intended semantics and looks robust from a security perspective.
67-81: Metadata‑only roles correctly restricted from PRIVATE/CLASSIFIED data
canAccessPublicationDatablocksINTERNAL_ADMIN_CTSCandINTERNAL_ADMIN_LOCALfrom seeing actual data for PRIVATE/CLASSIFIED artefacts, delegating tocanAccessPublicationfor everyone else. Combined withcanAccessPublicationMetadata, this cleanly models “metadata‑only” behaviour without over‑complicating the core check.
117-139: Filtering helpers correctly centralise access decisionsBoth
filterAccessiblePublicationsandfilterPublicationsForSummarydelegate to the core access functions and fail closed whenlistTypecan’t be found. This keeps list‑level filtering consistent with per‑artefact rules and is a good centralisation of the authorisation logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/publication/src/authorisation/service.test.ts (3)
14-45: VerifycreateListTypeshape against the actualListTypeinterfaceThe
createListTypehelper currently setslistTypeandjsonSchemaproperties and does not set anameproperty. TheListTypesnippet inlibs/list-types/common/src/mock-list-types.tsshows fields likeid,name,englishFriendlyName,welshFriendlyName,provenance, andurlPath, which may differ from what you’re constructing here. Under strict TypeScript with excess property checks, a mismatch could cause these tests not to compile ifListTypedoesn’t actually declarelistType/jsonSchemaor does requirename.Please double‑check the definitive
ListTypedefinition from@hmcts/list-types-commonand align the helper accordingly (for example, usingnameinstead oflistType, and only including properties that are actually on the interface).
280-355: Confirm intended metadata behaviour for LOCAL/CTSC admins vs comments inservice.tsThese
canAccessPublicationMetadatatests assert that LOCAL/CTSC admins can only see PUBLIC metadata and are denied for PRIVATE/CLASSIFIED, while system admins and verified users follow the broader rules. That’s consistent with the implementation ofcanAccessPublicationMetadata.However, the comment in
filterPublicationsForSummaryinlibs/publication/src/authorisation/service.ts(“This allows admins to see CLASSIFIED publications in lists even if they can't view the data”) could be read as including LOCAL/CTSC admins as well, which the tests explicitly do not allow.Please confirm whether:
- The current behaviour (only SYSTEM_ADMIN seeing CLASSIFIED in summaries; LOCAL/CTSC limited to PUBLIC) is the desired spec, in which case the comment in
service.tsshould probably be tightened; or- LOCAL/CTSC admins are also meant to see CLASSIFIED metadata in summaries, in which case the implementation and these tests would need to be updated.
357-503: Consider DRY-ing shared fixtures between the two filter test suitesBoth
filterAccessiblePublicationsandfilterPublicationsForSummarydescribe blocks construct effectively the samepublicArtefact/privateArtefact/classifiedArtefact,listTypes, andartefactsarrays. This duplication makes it slightly harder to maintain if you tweak the fixture shape or add new scenarios.You could extract a small helper to build the shared fixture and reuse it in both describes, e.g.:
+const buildPublicationFixture = () => { + const publicArtefact = createArtefact(Sensitivity.PUBLIC); + const privateArtefact = { ...createArtefact(Sensitivity.PRIVATE), artefactId: "private-id" }; + const classifiedArtefact = { ...createArtefact(Sensitivity.CLASSIFIED), artefactId: "classified-id" }; + + const listTypes = [createListType("CFT_IDAM"), { ...createListType("CRIME_IDAM"), id: 2 }]; + + const artefacts = [ + publicArtefact, + privateArtefact, + { ...classifiedArtefact, listTypeId: 1 }, + { ...classifiedArtefact, artefactId: "classified-id-2", listTypeId: 2 } + ]; + + return { publicArtefact, privateArtefact, classifiedArtefact, listTypes, artefacts }; +};Then destructure this in each describe. Not mandatory, but it would keep the filter tests in sync and reduce noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/publication/src/authorisation/service.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/publication/src/authorisation/service.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/publication/src/authorisation/service.test.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/publication/src/authorisation/service.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/publication/src/authorisation/service.test.ts
🧠 Learnings (2)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/publication/src/authorisation/service.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
libs/publication/src/authorisation/service.test.ts
🧬 Code graph analysis (1)
libs/publication/src/authorisation/service.test.ts (2)
libs/list-types/common/src/mock-list-types.ts (1)
ListType(1-8)libs/publication/src/authorisation/service.ts (5)
canAccessPublication(25-57)canAccessPublicationData(67-81)canAccessPublicationMetadata(93-108)filterAccessiblePublications(117-122)filterPublicationsForSummary(133-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (3)
libs/publication/src/authorisation/service.test.ts (3)
1-12: Imports, ESM usage, and Vitest setup look correctESM imports use
.jsextensions for local modules, types come from workspace aliases, and the file is co‑located withservice.tsusing Vitest’sdescribe/it/expect, which matches the project testing conventions.Based on learnings, this matches the expected test layout and framework.
47-200:canAccessPublicationtests comprehensively cover role/sensitivity and default behaviourThis block does a good job exercising PUBLIC/PRIVATE/CLASSIFIED access for unauthenticated users, verified users across provenances, system/local/CTSC admins, and the “missing sensitivity defaults to CLASSIFIED (fail closed)” scenario. The expectations line up with the
canAccessPublicationimplementation, including the provenance checks and default‑to‑CLASSIFIED behaviour.
202-278:canAccessPublicationDatatests correctly encode metadata‑only role restrictionsThe data‑access tests clearly distinguish PUBLIC vs PRIVATE/CLASSIFIED and verify that metadata‑only roles (local/CTSC admins) are blocked from PRIVATE/CLASSIFIED data while system admins and verified users follow the standard rules. This matches the
canAccessPublicationDatalogic and gives good regression coverage for future changes aroundMETADATA_ONLY_ROLES.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/postgres/prisma/seed.ts (1)
14-14: Verify list type mapping and avoid magic IDs in seed dataThe change to
listTypeId: 6with commentCrown Daily List (CRIME_IDAM)looks intentional, but please double‑check that:
- ID
6exists in yourlistTypeseed/table with the expected meaning (Crown Daily List for CRIME_IDAM), and- The other entry using
listTypeId: 2 // Crown Daily Listbelow is still semantically correct and not now conflicting/naming the same list type differently.To reduce future breakage when list types change, consider replacing these raw integers with shared constants or importing from a single source of truth for list type IDs in your Prisma seeds.
libs/publication/package.json (1)
13-13: Consider cross-platform cleanup command.The
rm -rf distcommand may not work on Windows without a bash-compatible shell. For better cross-platform compatibility, consider usingrimrafor a similar tool.Example with rimraf:
- "build": "rm -rf dist && tsc", + "build": "rimraf dist && tsc",Then add to devDependencies:
"rimraf": "^6.0.1"libs/publication/src/authorisation/service.ts (2)
25-57: Authorization logic is sound.The access control flow correctly handles all sensitivity levels with proper role/provenance checks. The non-null assertion on line 52 is safe due to earlier guards, though it could be avoided for clarity.
Optional: Remove the non-null assertion for clarity:
- return user!.provenance === listType.provenance; + return user.provenance === listType.provenance;TypeScript should infer
useris defined after the checks on lines 39 and 50.
26-26: Consider extracting sensitivity defaulting logic.The pattern
const sensitivity = artefact.sensitivity || Sensitivity.CLASSIFIED;is repeated in three functions (lines 26, 68, 94). Consider extracting to a helper function for consistency.Example:
function getSensitivity(artefact: Artefact): Sensitivity { return artefact.sensitivity || Sensitivity.CLASSIFIED; }Then use in each function:
- const sensitivity = artefact.sensitivity || Sensitivity.CLASSIFIED; + const sensitivity = getSensitivity(artefact);Also applies to: 68-68, 94-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/postgres/prisma/seed.ts(1 hunks)libs/publication/package.json(1 hunks)libs/publication/src/authorisation/service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
Package.json must include"type": "module"and exports field with proper ESM paths.
Pin all dependency versions to specific versions (e.g.,"express": "5.1.0"), except for peer dependencies.
All test packages must use"test": "vitest run"script. Tests should achieve >80% coverage on business logic.
Files:
libs/publication/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
apps/postgres/prisma/seed.tslibs/publication/src/authorisation/service.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
apps/postgres/prisma/seed.tslibs/publication/src/authorisation/service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
apps/postgres/prisma/seed.tslibs/publication/src/authorisation/service.ts
🧠 Learnings (2)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/package.json : All test packages must use `"test": "vitest run"` script. Tests should achieve >80% coverage on business logic.
Applied to files:
libs/publication/package.json
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/publication/package.json
🧬 Code graph analysis (1)
libs/publication/src/authorisation/service.ts (2)
libs/publication/src/index.ts (8)
canAccessPublication(4-4)Artefact(13-13)ListType(1-1)Sensitivity(15-15)canAccessPublicationData(5-5)canAccessPublicationMetadata(6-6)filterAccessiblePublications(7-7)filterPublicationsForSummary(8-8)libs/list-types/common/src/mock-list-types.ts (1)
ListType(1-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarQube Analysis
- GitHub Check: E2E Tests
🔇 Additional comments (5)
libs/publication/src/authorisation/service.ts (5)
1-7: LGTM!Imports follow ESM conventions with
.jsextensions and workspace aliases. Constants are properly typed withas constand follow naming conventions.
9-16: LGTM!The helper function correctly validates verified user provenance. Properly scoped as internal-only (not exported).
67-81: LGTM!Correctly restricts metadata-only roles from accessing sensitive publication data while delegating other cases to
canAccessPublication.
93-108: LGTM!Properly implements metadata-level access control with appropriate restrictions for admin roles. The optional
listTypeparameter is appropriate for the metadata context.
117-122: LGTM!Both filter functions follow a clear functional pattern and correctly delegate to their respective access check functions. The code is readable and maintainable.
Also applies to: 134-139
| {% block page_content %} | ||
| <h1 class="govuk-heading-l">{{ title }}</h1> | ||
| <p class="govuk-body">{{ message or "You do not have permission to access this page." }}</p> | ||
| <p class="govuk-body">If you think you should have access, <a href="/contact-us" class="govuk-link">contact us</a> if you need to speak to someone about this service.</p> |
There was a problem hiding this comment.
can you move these text to language specific file? i.e. en and cy
There was a problem hiding this comment.
Moved all error messages to language resource file
| {% block page_content %} | ||
| <h1 class="govuk-heading-l">{{ errorTitle or "Error" }}</h1> | ||
| <p class="govuk-body">{{ errorMessage or "An error has occurred." }}</p> | ||
| <p class="govuk-body">If you continue to have problems, <a href="/contact-us" class="govuk-link">contact us</a> if you need to speak to someone about this service.</p> |
There was a problem hiding this comment.
can you move these text to language specific file? i.e. en and cy
There was a problem hiding this comment.
Moved all error messages to language resource file
| const listType = mockListTypes.find((lt) => lt.id === artefact.listTypeId); | ||
| if (!canAccessPublicationData(req.user, artefact, listType)) { | ||
| return res.status(403).render("errors/403", { | ||
| en: { |
There was a problem hiding this comment.
can you move these text to language specific file? i.e. en and cy
| */ | ||
| export function requirePublicationDataAccess(): RequestHandler { | ||
| return createPublicationAccessMiddleware(canAccessPublicationData, { | ||
| en: { |
There was a problem hiding this comment.
I think this we cannot move to language specific files. Just double check.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
libs/publication/src/authorisation/middleware.ts (1)
71-71: Replace console.error with a proper logging mechanism.Using
console.erroris not suitable for production code. Consider using a structured logging library or service.libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts (1)
48-61: Good placement of access check; consider failing closed whenlistTypelookup failsThe access control is correctly applied immediately after loading the artefact and before any file I/O or rendering, and the 403 response uses the new localized
error403Title/error403Messagekeys for both languages, which is coherent with the i18n setup.One thing to tighten up:
mockListTypes.find((lt) => lt.id === artefact.listTypeId)can returnundefined, and thatundefinedis passed intocanAccessPublicationData. Even though the function acceptsListType | undefined, it would be safer and clearer to treat a missing list type as a hard error (data mismatch) or as “no access” and short‑circuit explicitly, rather than relying on downstream handling ofundefined.For example:
- const listType = mockListTypes.find((lt) => lt.id === artefact.listTypeId); - if (!canAccessPublicationData(req.user, artefact, listType)) { + const listType = mockListTypes.find((lt) => lt.id === artefact.listTypeId); + if (!listType || !canAccessPublicationData(req.user, artefact, listType)) { return res.status(403).render("errors/403", { en: { title: en.error403Title, message: en.error403Message }, cy: { title: cy.error403Title, message: cy.error403Message } }); }This makes the behaviour explicit and fail‑closed if the artefact’s
listTypeIddoes not correspond to a known configuration.libs/web-core/src/views/errors/en.ts (1)
1-50: Optional: introduce a shared type for error translations to catch key drift
en(andcy) share a stable shape (error400,error403,error404,error500,errorCommonwith consistent fields). If you want stronger compile‑time guarantees, consider defining aErrorTranslationstype/interface here and using it for bothenandcyso missing/renamed keys are surfaced by TypeScript rather than by tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
libs/list-types/civil-and-family-daily-cause-list/src/pages/cy.ts(1 hunks)libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts(1 hunks)libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts(2 hunks)libs/publication/src/authorisation/middleware.ts(1 hunks)libs/web-core/package.json(1 hunks)libs/web-core/src/middleware/govuk-frontend/error-handler.ts(3 hunks)libs/web-core/src/views/errors/400.njk(1 hunks)libs/web-core/src/views/errors/400.njk.test.ts(3 hunks)libs/web-core/src/views/errors/403.njk(1 hunks)libs/web-core/src/views/errors/404.njk(1 hunks)libs/web-core/src/views/errors/500.njk(1 hunks)libs/web-core/src/views/errors/common.njk(1 hunks)libs/web-core/src/views/errors/cy.ts(1 hunks)libs/web-core/src/views/errors/en.ts(1 hunks)libs/web-core/src/views/errors/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/web-core/src/views/errors/403.njk
- libs/web-core/src/views/errors/common.njk
🧰 Additional context used
📓 Path-based instructions (7)
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
Package.json must include"type": "module"and exports field with proper ESM paths.
Pin all dependency versions to specific versions (e.g.,"express": "5.1.0"), except for peer dependencies.
All test packages must use"test": "vitest run"script. Tests should achieve >80% coverage on business logic.
Files:
libs/web-core/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/web-core/src/views/errors/en.tslibs/web-core/src/middleware/govuk-frontend/error-handler.tslibs/publication/src/authorisation/middleware.tslibs/web-core/src/views/errors/index.tslibs/web-core/src/views/errors/cy.tslibs/web-core/src/views/errors/400.njk.test.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/en.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/web-core/src/views/errors/en.tslibs/web-core/src/middleware/govuk-frontend/error-handler.tslibs/publication/src/authorisation/middleware.tslibs/web-core/src/views/errors/index.tslibs/web-core/src/views/errors/cy.tslibs/web-core/src/views/errors/400.njk.test.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/en.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/web-core/src/views/errors/en.tslibs/web-core/src/middleware/govuk-frontend/error-handler.tslibs/publication/src/authorisation/middleware.tslibs/web-core/src/views/errors/index.tslibs/web-core/src/views/errors/cy.tslibs/web-core/src/views/errors/400.njk.test.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/en.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/web-core/src/views/errors/400.njk.test.ts
**/src/pages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.ts: Pages are registered through explicit imports inapps/web/src/app.ts. Routes are created based on file names within thepages/directory (e.g.,my-page.tsbecomes/my-page, nested routes via subdirectories).
Page controller files must exportGETand/orPOSTfunctions that accept Express Request and Response, render usingres.render(), and handle form submissions.
Files:
libs/list-types/civil-and-family-daily-cause-list/src/pages/en.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
**/src/pages/**/*.{ts,njk}
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/pages/**/*.{ts,njk}: Every page must support both English and Welsh. Controllers must provide bothenandcyobjects with page content.
Welsh translations are required for all user-facing text. Do not skip Welsh support.
Files:
libs/list-types/civil-and-family-daily-cause-list/src/pages/en.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
🧠 Learnings (12)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk`, use govuk macros for components, include error summaries, and support conditional rendering based on language variables.
Applied to files:
libs/web-core/src/views/errors/404.njklibs/web-core/src/views/errors/400.njk.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/package.json : Package.json must include `"type": "module"` and exports field with proper ESM paths.
Applied to files:
libs/web-core/package.json
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/libs/*/src/config.ts : Config exports (pageRoutes, apiRoutes, prismaSchemas, assets) must be in a separate `config.ts` file to avoid circular dependencies during Prisma client generation. Apps import config using the `/config` path.
Applied to files:
libs/web-core/package.json
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.{ts,tsx} : Always add `.js` extension to relative imports (e.g., `import { foo } from "./bar.js"`), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Applied to files:
libs/web-core/package.json
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.{ts,tsx,js,mjs} : DO NOT use CommonJS. Use `import`/`export`, never `require()`/`module.exports`. Only ES modules are allowed.
Applied to files:
libs/web-core/package.jsonlibs/web-core/src/views/errors/index.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
libs/web-core/src/views/errors/en.tslibs/web-core/src/middleware/govuk-frontend/error-handler.tslibs/web-core/src/views/errors/index.tslibs/web-core/src/views/errors/cy.tslibs/web-core/src/views/errors/400.njk.test.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
libs/web-core/src/middleware/govuk-frontend/error-handler.tslibs/web-core/src/views/errors/index.tslibs/web-core/src/views/errors/cy.tslibs/web-core/src/views/errors/400.njk.test.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/libs/*/src/*-middleware.ts : Reusable middleware should be placed in `libs/[module]/src/[middleware-name]-middleware.ts` and exported as a function.
Applied to files:
libs/publication/src/authorisation/middleware.ts
📚 Learning: 2025-11-20T09:59:16.776Z
Learnt from: junaidiqbalmoj
Repo: hmcts/cath-service PR: 106
File: libs/system-admin-pages/src/pages/reference-data-upload/index.test.ts:84-160
Timestamp: 2025-11-20T09:59:16.776Z
Learning: In the cath-service repository, Welsh localization (lng=cy) is not required for admin screens (system-admin-pages), so locale preservation in admin screen redirects is not necessary.
Applied to files:
libs/web-core/src/views/errors/cy.tslibs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/package.json : All test packages must use `"test": "vitest run"` script. Tests should achieve >80% coverage on business logic.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.ts
🧬 Code graph analysis (4)
libs/web-core/src/views/errors/en.ts (1)
libs/web-core/src/views/errors/index.ts (1)
en(1-1)
libs/web-core/src/middleware/govuk-frontend/error-handler.ts (2)
libs/web-core/src/views/errors/en.ts (1)
en(1-50)libs/web-core/src/views/errors/cy.ts (1)
cy(1-50)
libs/web-core/src/views/errors/400.njk.test.ts (2)
libs/web-core/src/views/errors/en.ts (1)
en(1-50)libs/web-core/src/views/errors/cy.ts (1)
cy(1-50)
libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts (6)
libs/list-types/common/src/mock-list-types.ts (1)
mockListTypes(10-75)libs/publication/src/authorisation/service.ts (1)
canAccessPublicationData(67-81)libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts (1)
en(1-40)libs/web-core/src/views/errors/en.ts (1)
en(1-50)libs/list-types/civil-and-family-daily-cause-list/src/pages/cy.ts (1)
cy(1-41)libs/web-core/src/views/errors/cy.ts (1)
cy(1-50)
🪛 GitHub Actions: Test
libs/web-core/src/middleware/govuk-frontend/error-handler.ts
[error] 22-22: Cannot read properties of undefined (reading 'locale')
libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts
[error] 37-38: Formatter would have printed content with different whitespace for errorMessage in en.ts. The formatter detected formatting differences. Command failed: yarn run lint (exit code 1). Run 'prettier --write src/pages/en.ts' or fix formatting to resolve.
🪛 GitHub Check: Test Changed Packages
libs/web-core/src/middleware/govuk-frontend/error-handler.ts
[failure] 22-22: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > notFoundHandler > should render 404 for HEAD requests
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:22:23
❯ src/middleware/govuk-frontend/error-handler.test.ts:39:7
[failure] 22-22: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > notFoundHandler > should render 404 for GET requests
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:22:23
❯ src/middleware/govuk-frontend/error-handler.test.ts:30:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should log error message if stack is not available
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:133:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should render 500 error page
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:122:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should use console as default logger
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:114:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should log error with provided logger
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:106:7
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (15)
libs/publication/src/authorisation/middleware.ts (4)
87-89: LGTM!Clean middleware wrapper that properly delegates to the factory function with the appropriate access check.
97-99: LGTM!Clean middleware wrapper that properly uses the custom 403 message flag for data access restrictions.
44-44: This is not mock data used in production; it's a reference data structure.
mockListTypesis a properly exported reference data set from@hmcts/list-types-commoncontaining all supported list types in the system. It's used throughout production code—not just middleware but also admin pages, public pages, and API validation—to look up list type metadata (display names, friendly names) by ID. The "mock" prefix indicates example/reference data, not test fixtures. Test files appropriately mock this export for their own scenarios.
20-20: Route parameter naming is already consistent.The middleware accesses
req.params.id, and the only route using this middleware (libs/public-pages/src/pages/publication/[id].ts) also accessesreq.params.iddirectly. Both use the same parameter name, so there are no consistency issues. TherequirePublicationDataAccessmiddleware is exported but not currently used in production routes.libs/list-types/civil-and-family-daily-cause-list/src/pages/cy.ts (1)
38-40: Welsh 403 translations align and look correct
error403Titleanderror403Messageare consistent with the English copy and existing 403 wording; structure and keys match the rest of the locale object.libs/list-types/civil-and-family-daily-cause-list/src/pages/index.ts (1)
5-5: Import extension from@hmcts/publicationlooks consistentImporting
canAccessPublicationDataandmockListTypesalongsidePROVENANCE_LABELSkeeps related publication concerns in one place and fits the existing ESM/alias pattern.libs/web-core/package.json (1)
15-18: New./errorsexport is consistent with existing ESM export mapThe production/default paths align with how other entries are wired and with the build step copying
src/viewstodist/views, so this should work cleanly with your ESM setup. As per coding guidelines, this keeps the package exports structured and explicit.libs/web-core/src/views/errors/404.njk (1)
3-9: 404 template correctly wired to translation objectTitle and all body copy now come from
t.*keys that are present in bothen.error404andcy.error404, so 404 pages will localise correctly without hard-coded strings.libs/web-core/src/views/errors/500.njk (1)
3-8: 500 template localisation and error-details usage look correctAll text now comes from
twith keys that are present in both language maps, and the error details section only renders whenerroris provided, matching the handler’s production vs development behaviour.Also applies to: 14-19
libs/web-core/src/views/errors/index.ts (1)
1-2: Centralised error locale exports follow ESM import rulesRe-exporting
enandcyfrom./en.jsand./cy.jsmatches the project pattern of using.jsextensions in TS for ESM/nodenext resolution and gives a clean, single entry point for error translations.libs/web-core/src/views/errors/cy.ts (1)
1-50: Welsh error translations align with English structureThe
cyobject mirrors theenstructure forerror400,error403,error404,error500, anderrorCommon, so all templates that consumet.*have Welsh equivalents and remain in sync with the English copy. Based on learnings, this satisfies the requirement for Welsh support on user-facing pages.libs/web-core/src/views/errors/400.njk (1)
3-8: 400 template correctly parameterised witht.error400translationsAll previously hard-coded strings now come from
t.error400keys present in bothenandcy, and the start-page/contact links are composed from those tokens, so the 400 page is properly localised and consistent with your translation data.Also applies to: 10-11, 13-13
libs/web-core/src/views/errors/400.njk.test.ts (3)
5-6: LGTM! Translation imports added correctly.The imports for English and Welsh translation modules follow the ESM guidelines with
.jsextensions and are appropriately co-located.
27-48: Well-structured localization support in template tests.The updated tests correctly verify that the template uses locale variables (
t.title,t.heading,t.description, etc.) rather than hard-coded English text. This separation of concerns—template structure tests vs. translation content tests—is a good practice.
58-75: Translation tests properly support bilingual requirements.The English and Welsh translation tests appropriately verify that both language variants have the required content, meeting the project's bilingual requirements. The use of partial matches (
toContain) for longer strings provides good balance between validation and flexibility.Based on learnings, Welsh translations are required for all user-facing text, and this is properly validated here.
libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts
Outdated
Show resolved
Hide resolved
| res.status(404).render("errors/404", { | ||
| en: en.error404, | ||
| cy: cy.error404, | ||
| t: res.locals.locale === "cy" ? cy.error404 : en.error404 | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "error-handler.ts" -o -name "*error-handler*" -type f | head -20Repository: hmcts/cath-service
Length of output: 192
🏁 Script executed:
cat -n libs/web-core/src/middleware/govuk-frontend/error-handler.tsRepository: hmcts/cath-service
Length of output: 2434
🏁 Script executed:
find . -path "*/test*" -name "*error-handler*" -o -path "*/spec*" -name "*error-handler*" | head -10Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
cat -n libs/web-core/src/middleware/govuk-frontend/error-handler.test.tsRepository: hmcts/cath-service
Length of output: 5143
🏁 Script executed:
cd libs/web-core && npm test -- error-handler.test.ts 2>&1 | head -100Repository: hmcts/cath-service
Length of output: 448
🏁 Script executed:
git log -1 --oneline --format="%H %s" 2>/dev/null || echo "Not a git repo"Repository: hmcts/cath-service
Length of output: 113
🏁 Script executed:
git diff HEAD -- libs/web-core/src/middleware/govuk-frontend/error-handler.ts 2>/dev/null | head -50 || echo "No git diff available"Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
cat -n libs/web-core/src/middleware/govuk-frontend/error-handler.ts | sed -n '1,5p'Repository: hmcts/cath-service
Length of output: 280
🏁 Script executed:
rg "res\.locals" libs/web-core/src --type ts -B 2 -A 2Repository: hmcts/cath-service
Length of output: 20885
🏁 Script executed:
find libs/web-core -name "*.ts" -type f | xargs grep -l "notFoundHandler\|errorHandler" | head -5Repository: hmcts/cath-service
Length of output: 247
🏁 Script executed:
rg "notFoundHandler|errorHandler" libs/web-core/src --type ts -B 2 -A 2Repository: hmcts/cath-service
Length of output: 9138
🏁 Script executed:
find libs/web-core -name "app.ts" -o -name "index.ts" -o -name "main.ts" -o -name "server.ts" | head -5Repository: hmcts/cath-service
Length of output: 298
🏁 Script executed:
head -50 libs/web-core/src/index.tsRepository: hmcts/cath-service
Length of output: 2146
🏁 Script executed:
grep -A 10 -B 5 "res.locals\s*=" libs/web-core/src/middleware --include="*.ts" -rRepository: hmcts/cath-service
Length of output: 44
Guard against undefined res.locals when deriving locale (fixes runtime TypeError in tests)
The error handlers at lines 22 and 39 access res.locals.locale directly without defensive checks:
- Line 22:
res.locals.locale === "cy" - Line 39:
res.locals.locale || "en"
Your unit tests create mock Response objects without a locals property, causing TypeError: Cannot read properties of undefined (reading 'locale') when these handlers execute. More importantly, as last-line error handlers, they must not crash on missing properties.
Fix both handlers by using optional chaining to safely access locale and normalize to "cy" or "en":
@@
if (req.method === "GET" || req.method === "HEAD") {
+ const locale = res.locals?.locale === "cy" ? "cy" : "en";
res.status(404).render("errors/404", {
en: en.error404,
cy: cy.error404,
- t: res.locals.locale === "cy" ? cy.error404 : en.error404
+ t: locale === "cy" ? cy.error404 : en.error404
});
@@
logger.error("Error:", err.stack || err);
-
- const locale = res.locals.locale || "en";
+
+ const locale = res.locals?.locale === "cy" ? "cy" : "en";
const t = locale === "cy" ? cy.error500 : en.error500;Apply the same defensive pattern in both the 404 handler (lines 19–27) and 500 handler (lines 34–59).
📝 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.
| res.status(404).render("errors/404", { | |
| en: en.error404, | |
| cy: cy.error404, | |
| t: res.locals.locale === "cy" ? cy.error404 : en.error404 | |
| }); | |
| const locale = res.locals?.locale === "cy" ? "cy" : "en"; | |
| res.status(404).render("errors/404", { | |
| en: en.error404, | |
| cy: cy.error404, | |
| t: locale === "cy" ? cy.error404 : en.error404 | |
| }); |
🧰 Tools
🪛 GitHub Actions: Test
[error] 22-22: Cannot read properties of undefined (reading 'locale')
🪛 GitHub Check: Test Changed Packages
[failure] 22-22: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > notFoundHandler > should render 404 for HEAD requests
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:22:23
❯ src/middleware/govuk-frontend/error-handler.test.ts:39:7
[failure] 22-22: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > notFoundHandler > should render 404 for GET requests
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:22:23
❯ src/middleware/govuk-frontend/error-handler.test.ts:30:7
🤖 Prompt for AI Agents
In libs/web-core/src/middleware/govuk-frontend/error-handler.ts around lines
19–27 and 34–59, the handlers access res.locals.locale directly which can be
undefined in tests and cause a TypeError; update both places to safely read the
locale using optional chaining and a default (e.g., const locale =
res.locals?.locale === "cy" ? "cy" : "en" or use res.locals?.locale || "en" as
appropriate) and then use that normalized locale for t (and for choosing en/cy)
so the handlers never dereference res.locals when it's undefined.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/web-core/src/middleware/govuk-frontend/error-handler.ts (1)
19-23: Guard againstres.localsbeing undefined when resolving locale in error handlersBoth handlers currently dereference
res.locals.localedirectly:
- Line 22:
res.locals.locale === "cy" …- Line 39:
const locale = res.locals.locale || "en";In tests (and potentially some runtime paths),
res.localsis not initialised on the mockResponse, causingTypeError: Cannot read properties of undefined (reading 'locale')and breaking the pipeline. As last‑line handlers, these must be defensive and always normalise the locale.Normalise the locale using optional chaining and limit it to
"cy"/"en"in both places, e.g.:@@ - if (req.method === "GET" || req.method === "HEAD") { - res.status(404).render("errors/404", { - en: en.error404, - cy: cy.error404, - t: res.locals.locale === "cy" ? cy.error404 : en.error404 - }); + if (req.method === "GET" || req.method === "HEAD") { + const locale = res.locals?.locale === "cy" ? "cy" : "en"; + res.status(404).render("errors/404", { + en: en.error404, + cy: cy.error404, + t: locale === "cy" ? cy.error404 : en.error404 + }); @@ - const locale = res.locals.locale || "en"; - const t = locale === "cy" ? cy.error500 : en.error500; + const locale = res.locals?.locale === "cy" ? "cy" : "en"; + const t = locale === "cy" ? cy.error500 : en.error500;This keeps the i18n behaviour while preventing the
TypeErrorwhenres.localsis unset in tests or edge cases.Also applies to: 39-40
libs/publication/src/authorisation/middleware.ts (1)
48-63: Simplify the redundant 403 error structure.The custom 403 error response includes redundant properties (
en,cy, spreadtwithdefaultMessage, plus separatetitleandmessage). This is inconsistent with the standard 403 handling on lines 65-69 and creates unnecessary complexity.Apply this diff to align with the standard error structure:
if (shouldUseCustom403Message) { return res.status(403).render("errors/403", { - en: { - title: errorEn.error403.title, - message: errorEn.error403.dataAccessDeniedMessage - }, - cy: { - title: errorCy.error403.title, - message: errorCy.error403.dataAccessDeniedMessage - }, - t: - locale === "cy" - ? { ...errorCy.error403, defaultMessage: errorCy.error403.dataAccessDeniedMessage } - : { ...errorEn.error403, defaultMessage: errorEn.error403.dataAccessDeniedMessage }, - title: locale === "cy" ? errorCy.error403.title : errorEn.error403.title, - message: locale === "cy" ? errorCy.error403.dataAccessDeniedMessage : errorEn.error403.dataAccessDeniedMessage + en: errorEn.error403, + cy: errorCy.error403, + t: locale === "cy" ? { + ...errorCy.error403, + message: errorCy.error403.dataAccessDeniedMessage + } : { + ...errorEn.error403, + message: errorEn.error403.dataAccessDeniedMessage + } }); }
🧹 Nitpick comments (4)
libs/web-core/src/views/errors/400.njk.test.ts (1)
5-7: 400 error template tests correctly exercise locale‑driven content for EN/CYImporting
en/cyfrom the locale modules and asserting againstt.*bindings in the template gives good coverage of both the Nunjucks structure and the bilingual copy; the new English/Welsh translation suites match the locale definitions and satisfy the requirement to support both languages. As a small optional enhancement, you could also assert additional Welsh fields (e.g.cy.error400.checkAddress) for closer parity with the English checks.Also applies to: 27-40, 42-48, 58-75
libs/publication/src/authorisation/middleware.ts (3)
31-42: Consider attaching the fetched artefact to res.locals.The middleware fetches the artefact but doesn't attach it to
res.locals. If downstream handlers need the artefact, they'll have to query the database again. Consider addingres.locals.artefact = artefactafter line 42 to avoid redundant database queries.if (!artefact) { return res.status(404).render("errors/404", { en: errorEn.error404, cy: errorCy.error404, t: locale === "cy" ? errorCy.error404 : errorEn.error404 }); } + + // Attach artefact for downstream use + res.locals.artefact = artefact; const listType = mockListTypes.find((lt) => lt.id === artefact.listTypeId);
73-80: Consider using a structured logging framework instead of console.error.For production middleware, consider using a proper logging framework (e.g.,
winston,pino) instead ofconsole.errorto enable structured logging, log levels, and better observability.} catch (error) { - console.error("Error checking publication access:", error); + // TODO: Replace with structured logging framework + console.error("Error checking publication access:", error); return res.status(500).render("errors/500", {
44-44: Consider renamingmockListTypesto clarify it is static reference data, not test data.The code at line 44 uses
mockListTypesto resolve list types. While the name suggests test data, this is actually a static, immutable array of production court list type definitions (Civil Daily Cause List, Crime Daily List, etc.). The usage is appropriate for production, but the "mock" prefix is misleading. Consider renaming it toLIST_TYPESorCOURT_LIST_TYPESto better reflect that this is authoritative reference data, not mock/test data. This naming clarification would improve code clarity across the entire codebase where this array is imported and used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts(1 hunks)libs/publication/src/authorisation/middleware.ts(1 hunks)libs/web-core/src/middleware/govuk-frontend/error-handler.ts(3 hunks)libs/web-core/src/views/errors/400.njk.test.ts(3 hunks)libs/web-core/src/views/errors/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans must useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants outside function scope at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
TypeScript strict mode must be enabled. Noanytype without justification. Use explicit types for all variables and function parameters.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"), even when importing TypeScript files. This is required for ESM with Node.js 'nodenext' module resolution.
Use workspace aliases (@hmcts/*) for imports between packages instead of relative paths.
Only export functions that are intended to be used outside the module. Do not export functions solely for testing purposes.
Only add comments when they provide meaningful explanation of why something is done, not what is done. Code should be self-documenting.
Favor functional style. Don't use classes unless you have shared state.
Data should be immutable by default. Use const and avoid mutations to ensure predictable state.
Functions should have no side effects. Avoid modifying external state or relying on mutable data.
Files:
libs/web-core/src/views/errors/index.tslibs/publication/src/authorisation/middleware.tslibs/web-core/src/views/errors/400.njk.test.tslibs/web-core/src/middleware/govuk-frontend/error-handler.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CLAUDE.md)
DO NOT use CommonJS. Use
import/export, neverrequire()/module.exports. Only ES modules are allowed.
Files:
libs/web-core/src/views/errors/index.tslibs/publication/src/authorisation/middleware.tslibs/web-core/src/views/errors/400.njk.test.tslibs/web-core/src/middleware/govuk-frontend/error-handler.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not create generictypes.tsfiles. Colocate types with the appropriate code file where they are used.
Do not create generic files likeutils.ts. Be specific with naming (e.g.,object-properties.ts,date-formatting.ts).
Files:
libs/web-core/src/views/errors/index.tslibs/publication/src/authorisation/middleware.tslibs/web-core/src/views/errors/400.njk.test.tslibs/web-core/src/middleware/govuk-frontend/error-handler.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit/integration test files must be co-located with source files as
*.test.tsand use Vitest withdescribe,it, andexpect.
Files:
libs/web-core/src/views/errors/400.njk.test.ts
🧠 Learnings (9)
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Every page must support both English and Welsh. Controllers must provide both `en` and `cy` objects with page content.
Applied to files:
libs/web-core/src/views/errors/index.tslibs/web-core/src/views/errors/400.njk.test.tslibs/web-core/src/middleware/govuk-frontend/error-handler.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.{ts,tsx,js,mjs} : DO NOT use CommonJS. Use `import`/`export`, never `require()`/`module.exports`. Only ES modules are allowed.
Applied to files:
libs/web-core/src/views/errors/index.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/libs/*/src/*-middleware.ts : Reusable middleware should be placed in `libs/[module]/src/[middleware-name]-middleware.ts` and exported as a function.
Applied to files:
libs/publication/src/authorisation/middleware.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.{ts,njk} : Welsh translations are required for all user-facing text. Do not skip Welsh support.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.tslibs/web-core/src/middleware/govuk-frontend/error-handler.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/*.test.ts : Unit/integration test files must be co-located with source files as `*.test.ts` and use Vitest with `describe`, `it`, and `expect`.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to e2e-tests/**/*.spec.ts : E2E test files must be in `e2e-tests/` directory named `*.spec.ts`, use Playwright, include complete user journeys with validations, Welsh translations, accessibility checks, and keyboard navigation all within a single test.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/package.json : All test packages must use `"test": "vitest run"` script. Tests should achieve >80% coverage on business logic.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.njk : Nunjucks templates must extend `layouts/base-templates.njk`, use govuk macros for components, include error summaries, and support conditional rendering based on language variables.
Applied to files:
libs/web-core/src/views/errors/400.njk.test.ts
📚 Learning: 2025-12-03T13:55:34.702Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T13:55:34.702Z
Learning: Applies to **/src/pages/**/*.ts : Page controller files must export `GET` and/or `POST` functions that accept Express Request and Response, render using `res.render()`, and handle form submissions.
Applied to files:
libs/web-core/src/middleware/govuk-frontend/error-handler.ts
🧬 Code graph analysis (3)
libs/publication/src/authorisation/middleware.ts (2)
libs/list-types/common/src/mock-list-types.ts (2)
ListType(1-8)mockListTypes(10-75)libs/publication/src/authorisation/service.ts (2)
canAccessPublication(25-57)canAccessPublicationData(67-81)
libs/web-core/src/views/errors/400.njk.test.ts (2)
libs/web-core/src/views/errors/en.ts (1)
en(1-50)libs/web-core/src/views/errors/cy.ts (1)
cy(1-50)
libs/web-core/src/middleware/govuk-frontend/error-handler.ts (4)
libs/web-core/src/views/errors/index.ts (2)
en(2-2)cy(1-1)libs/web-core/src/views/errors/en.ts (1)
en(1-50)libs/web-core/src/index.ts (2)
en(6-6)cy(5-5)libs/web-core/src/views/errors/cy.ts (1)
cy(1-50)
🪛 GitHub Actions: Test
libs/web-core/src/middleware/govuk-frontend/error-handler.ts
[error] 22-22: Cannot read properties of undefined (reading 'locale')
🪛 GitHub Check: Test Changed Packages
libs/web-core/src/middleware/govuk-frontend/error-handler.ts
[failure] 22-22: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > notFoundHandler > should render 404 for HEAD requests
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:22:23
❯ src/middleware/govuk-frontend/error-handler.test.ts:39:7
[failure] 22-22: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > notFoundHandler > should render 404 for GET requests
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:22:23
❯ src/middleware/govuk-frontend/error-handler.test.ts:30:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should log error message if stack is not available
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:133:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should render 500 error page
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:122:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should use console as default logger
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:114:7
[failure] 39-39: src/middleware/govuk-frontend/error-handler.test.ts > Error Handler Middleware > errorHandler > should log error with provided logger
TypeError: Cannot read properties of undefined (reading 'locale')
❯ src/middleware/govuk-frontend/error-handler.ts:39:31
❯ src/middleware/govuk-frontend/error-handler.test.ts:106:7
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (4)
libs/web-core/src/views/errors/index.ts (1)
1-2: Re‑exports for error locales are correct and ESM‑compliantUsing named re‑exports with
.jsextensions matches the ESM/nodenext and shareden/cylocale pattern used elsewhere; no changes needed.libs/publication/src/authorisation/middleware.ts (3)
1-8: LGTM: Imports follow conventions.All imports correctly use ES modules, workspace aliases, and include the
.jsextension for relative imports as required by the coding guidelines.
23-29: LGTM: Parameter validation is appropriate.The 400 response for missing
publicationIdis correct, and the bilingual error handling is consistent with the file's approach.
90-92: LGTM: Well-designed public middleware API.The two exported middleware functions provide a clean, purpose-specific API while keeping the internal factory function private. The separation between metadata access (
requirePublicationAccess) and data access (requirePublicationDataAccess) is clear and well-documented.Based on learnings, the middleware is correctly placed in
libs/publication/src/authorisation/middleware.tsand exported as functions.Also applies to: 100-102
| * @param useCustom403Message - Whether to use the custom publication data access denied message | ||
| * @returns Express middleware function | ||
| */ | ||
| function createPublicationAccessMiddleware(checkAccess: AccessCheck, useCustom403Message = false): RequestHandler { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename boolean parameter to follow naming conventions.
As per coding guidelines, boolean variables must use is/has/can/should prefixes. The parameter useCustom403Message should be renamed to shouldUseCustom403Message.
Apply this diff:
-function createPublicationAccessMiddleware(checkAccess: AccessCheck, useCustom403Message = false): RequestHandler {
+function createPublicationAccessMiddleware(checkAccess: AccessCheck, shouldUseCustom403Message = false): RequestHandler {
return async (req: Request, res: Response, next: NextFunction) => {And update the usage on line 47:
- if (useCustom403Message) {
+ if (shouldUseCustom403Message) {As per coding guidelines, booleans must use is/has/can/should prefix.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libs/publication/src/authorisation/middleware.ts around line 18 (and usage at
line 47), rename the boolean parameter useCustom403Message to
shouldUseCustom403Message to follow the is/has/can/should prefix guideline;
update the function signature and every call site (including the usage on line
47) to use the new name, and ensure any internal references inside the function
are updated accordingly to avoid linter/type errors.
|
There was a problem hiding this comment.
There's a sonar warning for this file to reduce complexity :)
3 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-247
Change description
Add authentication for publication
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Docs
Chores
✏️ Tip: You can customize this high-level summary in your review settings.