Conversation
…ature/hote-803/improve-lambdas
Lambdas Coverage Report
|
UI Coverage Report
|
There was a problem hiding this comment.
Pull request overview
This PR aims to migrate the repo’s developer/CI tooling to use Bun, refresh lockfiles, and improve WireMock auth stubbing reliability for Playwright tests.
Changes:
- Switched many scripts, docs, and CI steps from
npm/npxtobun/bunx, and added Bun tomise. - Updated WireMock auth mappings to generate unique
kidvalues per run and made the JWT issuer configurable. - Bumped Postgres images (local env + testcontainers) from 17.7 to 17.9.
Reviewed changes
Copilot reviewed 26 out of 31 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/README.md | Updates frontend dev/build/test instructions (now references pnpm). |
| ui/package.json | Changes serve:static script to use pnpm/pnpm exec. |
| tests/utils/users/wiremockAuthMappings.ts | Adds per-process kid nonce + issuer parameterization support. |
| tests/scripts/push-wiremock-mappings.ts | Adds WIREMOCK_JWT_ISSUER, refactors OS Places mapping import, updates usage to Bun. |
| tests/README.md | Updates Playwright setup/run commands to Bun. |
| tests/pnpm-lock.yaml | Adds a pnpm lockfile for tests. |
| tests/global-setup.ts | Passes configured WireMock JWT issuer into manifest + userinfo mappings. |
| tests/docs/guides/PullRequestGuidelines.md | Updates examples/commands to Bun and reformats TypeScript snippets. |
| tests/configuration/EnvironmentVariables.ts | Adds WIREMOCK_JWT_ISSUER env var. |
| tests/configuration/EnvironmentConfiguration.ts | Adds wiremockJwtIssuer to config + env override logic. |
| scripts/tests/unit.sh | Updates example command in comments to pnpm. |
| README.md | Updates root developer commands from npm to Bun. |
| pnpm-lock.yaml | Adds a pnpm lockfile at repo root. |
| package.json | Replaces many npm-based scripts with Bun equivalents and uses bun install --frozen-lockfile. |
| mise.lock | Adds Bun tool and updates various tool platform entries; adjusts Python version. |
| local-environment/wiremock/README.md | Updates local WireMock usage command to pnpm. |
| local-environment/scripts/localstack/ensure-localstack-running.sh | Updates guidance message to pnpm. |
| local-environment/scripts/database/Dockerfile | Bumps Postgres base image to 17.9. |
| local-environment/docker-compose.yml | Switches UI container command to pnpm install && pnpm run dev; bumps Postgres image to 17.9. |
| lambdas/src/lib/db/db-client.integration.test.ts | Bumps testcontainers Postgres image and minor formatting. |
| lambdas/README.md | Updates build/package/test commands to Bun. |
| lambdas/package.json | Switches build/package/test scripts to pnpm commands. |
| .prettierignore | Stops ignoring package-lock.json and ignores pnpm-lock.yaml instead. |
| .pre-commit-config.yaml | Updates hooks to call Bun commands; excludes bun.lock from yamllint. |
| .mise.toml | Adds Bun tool + settings changes; updates install task to Bun. |
| .github/workflows/playwright-e2e.yaml | Updates Playwright install/test invocation to Bun. |
| .github/copilot-instructions.md | Updates repo instructions to Bun. |
| .github/actions/run-npm-tests/action.yaml | Updates custom action to run tests via Bun. |
| .github/actions/init-mise/action.yaml | Changes cache to Bun cache path + uses bun lockfile hash for key. |
| "local:service:db:migrate": "npm run local:compose -- --profile backend up --build db-migrate", | ||
| "local:service:localstack:start": "npm run local:compose:up -- localstack", | ||
| "local:service:localstack:stop": "npm run local:compose:down -- localstack", | ||
| "postinstall": "cd ui && bun install --frozen-lockfile && cd ../lambdas && bun install --frozen-lockfile && cd ../tests && bun install --frozen-lockfile && cd ..", |
There was a problem hiding this comment.
bun install --frozen-lockfile will fail unless a Bun lockfile (e.g. bun.lock/bun.lockb) is present and committed; the repo currently still has package-lock.json/pnpm-lock.yaml but no Bun lockfile, so CI/local installs are likely to break. Commit the Bun lockfile(s) (and remove other lockfiles if migrating), or drop --frozen-lockfile until the lockfile strategy is consistent.
| "test:watch": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js --watch", | ||
| "serve:static": "npm run build && npx http-server build -p 8085 -c-1 --proxy http://localhost:8085?", | ||
| "serve:static": "bun run build && bunx http-server build -p 8085 -c-1 --proxy http://localhost:8085?", | ||
| "check-typescript": "next typegen && tsc --noEmit" |
There was a problem hiding this comment.
This script uses pnpm exec http-server, but http-server is not listed in dependencies/devDependencies, so serve:static will fail on a clean install. Either add http-server as a devDependency or switch to a runner that can download/execute it (e.g. pnpm dlx/bunx/npx).
| "test": "bun run test:unit && bun run test:integration", | ||
| "test:unit": "jest --testPathIgnorePatterns=\\.integration\\.test\\.ts$ --coverage", | ||
| "test:integration": "jest --testPathPatterns=\\.integration\\.test\\.ts$ --runInBand", | ||
| "test:watch": "jest --watch", | ||
| "test:coverage": "jest --coverage", | ||
| "build": "npx tsx scripts/build.ts", | ||
| "package": "npx tsx scripts/package.ts", | ||
| "local:deploy": "npm --prefix ../ run local:deploy", | ||
| "local:terraform:apply": "npm --prefix ../ run local:terraform:apply", | ||
| "build": "bunx tsx scripts/build.ts", | ||
| "package": "bunx tsx scripts/package.ts", | ||
| "local:deploy": "bun run --cwd ../ local:deploy", | ||
| "local:terraform:apply": "bun run --cwd ../ local:terraform:apply", |
There was a problem hiding this comment.
These scripts invoke pnpm, but the repo-level tooling and CI have been switched to Bun; unless pnpm is installed in the environment, test/build/package will fail. Align the lambdas scripts to use Bun (or plain tsx/node) consistently, or explicitly install/pin pnpm as part of the toolchain.
| env_file: | ||
| - ../ui/.env.local | ||
| command: sh -c "npm install && npm run dev" | ||
| command: sh -c "pnpm install && pnpm run dev" |
There was a problem hiding this comment.
The ui service runs pnpm install, but the node:24 image doesn’t include pnpm by default (and there’s no corepack enable here), so docker compose is likely to fail. Use npm/bun consistently, or enable corepack and pin pnpm before running pnpm install.
| command: sh -c "pnpm install && pnpm run dev" | |
| command: sh -c "npm install && npm run dev" |
|
|
||
| if ! docker ps --format '{{.Names}}' | grep -qx 'localstack-main'; then | ||
| echo "LocalStack is not running. Start the backend first with 'npm run local:backend:start', or run 'npm run local:start'." | ||
| echo "LocalStack is not running. Start the backend first with 'pnpm run local:backend:start', or run 'pnpm run local:start'." |
There was a problem hiding this comment.
This error message tells users to run pnpm ..., but the root scripts in this PR have been switched to Bun; this will mislead anyone following the guidance. Update the message to reference the actual supported command (bun run local:backend:start / bun run local:start).
| echo "LocalStack is not running. Start the backend first with 'pnpm run local:backend:start', or run 'pnpm run local:start'." | |
| echo "LocalStack is not running. Start the backend first with 'bun run local:backend:start', or run 'bun run local:start'." |
|
|
||
| ```shell | ||
| npm start | ||
| pnpm start |
There was a problem hiding this comment.
The README instructs pnpm start, but the root-level workflow/docs in this PR are moving to Bun; this is inconsistent and likely to confuse local setup. Update this to the correct root command (e.g. bun start) or explicitly document when pnpm is required.
| pnpm start | |
| bun start |
| @@ -33,7 +33,7 @@ The application uses client-side routing with React Router. You can start editin | |||
| Build the static export: | |||
|
|
|||
| ```bash | |||
| npm run build | |||
| pnpm run build | |||
| ``` | |||
|
|
|||
| This creates a `build/` directory with static HTML, CSS, and JavaScript files that can be deployed to any static hosting service. | |||
| @@ -43,7 +43,7 @@ This creates a `build/` directory with static HTML, CSS, and JavaScript files th | |||
| To test the production build locally: | |||
|
|
|||
| ```bash | |||
| npm run serve:static | |||
| pnpm run serve:static | |||
| ``` | |||
There was a problem hiding this comment.
This README now recommends pnpm commands, but the repo-level scripts/docs in this PR are switching to Bun, and pnpm may not be installed for contributors/CI. Update the instructions to match the chosen package manager (e.g. bun run dev/build/serve:static) or clearly document that the UI package requires pnpm.
| # Dependency directories | ||
| node_modules/ | ||
| package-lock.json | ||
| pnpm-lock.yaml | ||
|
|
There was a problem hiding this comment.
.prettierignore no longer ignores package-lock.json, but the repo still contains multiple package-lock.json files (root/ui/lambdas/tests), so Prettier hooks may waste time or create unwanted churn on lockfiles. Add package-lock.json (and Bun lockfiles like bun.lock* if applicable) back to the ignore list, or remove the lockfiles as part of the migration.
|
|
I think Bun is a complete runtime whereas PNPM which still uses node so it might be better to go with #342 for better compatibility. |
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.