Add location data, Provenance to Lists and fix bug - #140
Conversation
WalkthroughAdded a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
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: 0
🧹 Nitpick comments (2)
libs/list-types/common/src/mock-list-types.ts (1)
1-75: Provenance addition looks good; consider tightening type / avoiding duplicated literalsMaking
provenancerequired onListTypeand populating it for all mock entries is consistent and will surface missing data at compile time. Two optional improvements to consider:
- If the set of provenances is small and known, introduce a
type Provenance = "CFT_IDAM" | ...;and useprovenance: Provenanceto prevent typos and make future additions explicit.- Alternatively (or additionally), extract
"CFT_IDAM"to aconst PROVENANCE_CFT_IDAM = "CFT_IDAM" as const;and reuse it across entries to avoid repeated string literals.Also, please double‑check other
ListTypeconstructions in the codebase still compile now thatprovenanceis mandatory.libs/location/src/seed-data.ts (1)
4-39: Seeding guard is sensible; confirm “all tables empty” invariant and consider module orderingThe
shouldSeed()guard is a good safety net: it prevents seeding in production/CI and avoids re-running against non-empty tables. A couple of points to double‑check:
- The logic only seeds when all of
region,jurisdiction, andlocationtables are empty. In any partially-populated state (e.g. regions/jurisdictions present but locations missing), seeding will be skipped entirely. If your migrations/flows guarantee “all-or-nothing” population, this is fine; otherwise you may want a strategy for repairing partially-seeded environments.- Skipping in CI via
process.env.CI === "true"assumes your CI sets that exact string; worth confirming for all CI providers you use.From a style perspective, project guidelines say exported functions should come before internal helpers. You could move
shouldSeedbelowexport async function seedLocationData()(function declarations are hoisted, so behavior won’t change) to match that convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/list-types/common/src/mock-list-types.ts(2 hunks)libs/location/src/seed-data.ts(1 hunks)libs/system-admin-pages/src/reference-data-upload/services/download-service.ts(1 hunks)package.json(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 (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/list-types/common/src/mock-list-types.tslibs/location/src/seed-data.tslibs/system-admin-pages/src/reference-data-upload/services/download-service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/list-types/common/src/mock-list-types.tslibs/location/src/seed-data.tslibs/system-admin-pages/src/reference-data-upload/services/download-service.ts
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: Package names must use @hmcts scope (e.g.,@hmcts/auth,@hmcts/case-management).
All package.json files must use"type": "module"to enforce ES modules. Never use CommonJSrequire()ormodule.exports. Useimport/exportonly.
Express version 5.x only must be used ("express": "5.1.0"). Pin all dependencies to specific versions except peer dependencies.
Build scripts must includebuild:nunjucksif the module contains Nunjucks templates in thepages/directory to copy .njk files to dist.
Files:
package.json
🧠 Learnings (1)
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Always run commands from the root directory (e.g., `yarn test`, `yarn dev`).
Applied to files:
package.json
🧬 Code graph analysis (1)
libs/location/src/seed-data.ts (1)
libs/location/src/index.ts (1)
seedLocationData(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (2)
libs/system-admin-pages/src/reference-data-upload/services/download-service.ts (1)
2-2: Confirm papaparse default import matches your TS/bundler configSwitching from a namespace import to
import Papa from "papaparse";is fine as long as your toolchain treats papaparse as having a default export (typically viaesModuleInterop/allowSyntheticDefaultImports). Please verify that builds/tests still pass across all environments (Node, bundler, tests), as misconfigured interop would surface as a runtimeundefinedforPapa.package.json (1)
33-37: Seeding script integration intodev:uplooks appropriate; verify behavior across envsHooking
db:seedintodev:upafterdb:migrate:devmakes local setup simpler, and the newshouldSeed()guard keeps this safe to run repeatedly and in CI/production contexts. This also respects the preference to run commands from the repo root (yarn db:seed). Based on learnings, this matches the intended workflow.Please just confirm:
yarn db:seedruns successfully viatsxon your local environment.- CI does not rely on
dev:upfor DB lifecycle, or, if it does, thatCIis set so seeding is skipped as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/location/src/seed-data.test.ts (1)
91-95: Addvi.resetModules()for reliable environment variable testing.Tests use dynamic imports with varying
process.envvalues, butvi.clearAllMocks()only resets mock call history, not the module cache. If the imported module caches any env-dependent state at load time, subsequent tests could receive stale behavior.beforeEach(() => { originalEnv = { ...process.env }; consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + vi.resetModules(); vi.clearAllMocks(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/location/src/seed-data.test.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 (useUserRepositorynotIUserRepository).
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: consts outside function scope at top, exported functions next, other functions ordered by usage, interfaces and types at bottom.
TypeScript must use strict mode enabled with noanywithout justification. Use workspace aliases (@hmcts/*) for imports.
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.
All input endpoints must validate inputs. Use parameterized database queries with Prisma. Never include sensitive data in logs.
Only export functions that are intended to be used outside the module. Don't export functions solely for testing purposes.
Only add comments when they are meaningful. Explain why something is done, not what is done.
Favor functional style with simple functional approaches. 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.
Use Express version 5.x with proper async/await error handling in middleware and route handlers.
Files:
libs/location/src/seed-data.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
API endpoints must use plural for collections (
/api/cases,/api/users), singular for specific resources (/api/case/:id), and singular for creation (POST /api/case).
Files:
libs/location/src/seed-data.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All packages must use
"test": "vitest run"as the test script. Unit and integration tests must be co-located with source code as*.test.tsfiles.
Files:
libs/location/src/seed-data.test.ts
🧠 Learnings (1)
📚 Learning: 2025-11-25T06:40:24.099Z
Learnt from: CR
Repo: hmcts/cath-service PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T06:40:24.099Z
Learning: Applies to **/*.test.ts : All packages must use `"test": "vitest run"` as the test script. Unit and integration tests must be co-located with source code as `*.test.ts` files.
Applied to files:
libs/location/src/seed-data.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (2)
libs/location/src/seed-data.test.ts (2)
102-190: Well-structured guard condition tests.The
shouldSeedtests comprehensively cover all early exit conditions: production environment, CI environment, and non-empty tables for each entity type. The assertions correctly verify both logging output and that database operations are skipped.
192-370: Thorough seeding workflow tests with good assertion patterns.The
seedLocationDatatests provide excellent coverage:
- Upsert payloads verified for each entity type
- Junction record delete-then-create pattern validated
- Empty array handling for location 3 tested explicitly
- Workflow ordering verified via log message sequence
The approach of deriving expected counts from mock data (e.g., line 308:
mockLocationData.locations.filter((l) => l.regions.length > 0)) makes tests resilient to mock data changes.
🎭 Playwright E2E Test Results361 tests 361 ✅ 30m 43s ⏱️ Results for commit 41e8bf8. ♻️ This comment has been updated with latest results. |
The build was failing because the Prisma client wasn't generated before running tests. This resulted in TypeScript errors where the location, jurisdiction, region, and other models didn't exist on the PrismaClient type. Added a step to run 'yarn db:generate' before running tests to ensure the Prisma client is generated from the collated schemas. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
2 participants
Change description
Add location data, Provenance to Lists and fix bug
Summary by CodeRabbit
Chores
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.