feat: pnpm #7549
feat: pnpm #7549
Conversation
![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
♻️ Duplicate comments (1)
.devcontainer/post-create-command.sh (1)
22-22: Ensure psql connects to the db service, not the local socket.Repeat of a prior comment; add host and user to be consistent with other calls.
-psql -c "CREATE USER \"test-user\" WITH PASSWORD 'test-password' CREATEDB;" || echo "User test-user might already exist" +psql -h db -U postgres -c "CREATE USER \"test-user\" WITH PASSWORD 'test-password' CREATEDB;" || echo "User test-user might already exist"
🧹 Nitpick comments (3)
.devcontainer/post-create-command.sh (3)
24-28: Harden pnpm bootstrap: create PNPM_HOME and export PATH.Guarantees directory exists and PATH is propagated to subprocesses.
# install pnpm echo "Installing pnpm..." -PNPM_HOME="$HOME/.local/share/pnpm" -PATH="$PNPM_HOME:$PATH" -corepack enable && corepack prepare pnpm --activate +PNPM_HOME="${HOME}/.local/share/pnpm" +mkdir -p "${PNPM_HOME}" +export PATH="${PNPM_HOME}:${PATH}" +corepack enable && corepack prepare pnpm --activate
31-32: Prefer avoiding global CLI installs or pin them exactly.Global CLIs can drift between devs/containers. Consider using
pnpm dlxper-invocation or adding these as devDependencies and running viapnpm exec. If you keep globals, pin exact versions to improve reproducibility.Example alternatives:
- Use on demand:
pnpm dlx nx --version- Or pin:
pnpm add -g nx@<exact> @nestjs/cli@8.1.5 foreman@<exact> apollo@<exact> graphql@<exact>
36-36: Make dependency install deterministic.Use the lockfile strictly to prevent accidental upgrades across contributors.
-pnpm install +pnpm install --frozen-lockfile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.devcontainer/Dockerfile(1 hunks).devcontainer/devcontainer.json(2 hunks).devcontainer/post-create-command.sh(2 hunks).devcontainer/post-start-command.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .devcontainer/Dockerfile
- .devcontainer/devcontainer.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: apps/journeys-admin/project.json:54-59
Timestamp: 2025-09-04T04:02:44.210Z
Learning: In pnpm workspaces, dependencies (including devDependencies) installed at the root package.json are available to all workspace packages via `pnpm exec`, so individual packages don't need to declare these dependencies locally.
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: package.json:5-5
Timestamp: 2025-09-04T22:08:32.269Z
Learning: In the JesusFilm/core repository, when using corepack in Dockerfiles, use `corepack prepare pnpm --activate` without specifying a version, as corepack will automatically read the version from the packageManager field in package.json. Do not hardcode pnpm versions in Dockerfiles.
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: apps/arclight/Dockerfile:20-20
Timestamp: 2025-09-04T04:01:06.303Z
Learning: In the JesusFilm/core repository, the build process for apps includes package.json with the necessary scripts, so Docker containers can safely use `pnpm start` after copying the dist directory.
📚 Learning: 2025-09-04T04:01:06.303Z
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: apps/arclight/Dockerfile:20-20
Timestamp: 2025-09-04T04:01:06.303Z
Learning: In the JesusFilm/core repository, the build process for apps includes package.json with the necessary scripts, so Docker containers can safely use `pnpm start` after copying the dist directory.
Applied to files:
.devcontainer/post-start-command.sh
📚 Learning: 2025-09-04T22:08:32.269Z
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: package.json:5-5
Timestamp: 2025-09-04T22:08:32.269Z
Learning: In the JesusFilm/core repository, when using corepack in Dockerfiles, use `corepack prepare pnpm --activate` without specifying a version, as corepack will automatically read the version from the packageManager field in package.json. Do not hardcode pnpm versions in Dockerfiles.
Applied to files:
.devcontainer/post-start-command.sh
🪛 Shellcheck (0.10.0)
.devcontainer/post-start-command.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ 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). (8)
- GitHub Check: Deploy Preview (journeys-admin, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (short-links, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (journeys, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (watch, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (videos-admin, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (arclight, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (watch-modern, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (docs, 7549/merge, pull_request, 22)
🔇 Additional comments (1)
.devcontainer/post-start-command.sh (1)
3-3: Corepack usage LGTM.Using
corepack prepare pnpm --activatewithout pinning a version aligns with the repo’s packageManager-driven versioning.
… for improved environment setup
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
… simplify environment setup
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.devcontainer/post-create-command.sh (1)
12-18: Fix off-by-one in DB readiness check (can false-fail on 30th success).If Postgres becomes ready exactly on attempt 30, the script breaks but still exits as failure since i==30. Check readiness after the loop instead of relying on i.
-for i in {1..30}; do - pg_isready -h db -p 5432 -U postgres && break - echo "Database not ready (try $i/30)…" - sleep 2 -done -[ "$i" -eq 30 ] && { echo "Postgres failed to start"; exit 1; } +for i in {1..30}; do + if pg_isready -h db -p 5432 -U postgres; then + break + fi + echo "Database not ready (try $i/30)…" + sleep 2 +done +# Treat as failure only if still not ready after the loop +pg_isready -h db -p 5432 -U postgres || { echo "Postgres failed to start"; exit 1; }
♻️ Duplicate comments (2)
.devcontainer/post-start-command.sh (1)
1-1: Add shebang, safe shell options, and PATH export; fix ShellCheck SC2148.This script lacks a shebang and robust shell opts, and doesn’t ensure PNPM_HOME is on PATH. Also, good job not pinning the pnpm version—this aligns with our Corepack guidance.
Apply:
+#!/usr/bin/env bash +set -euo pipefail +PNPM_HOME="${HOME}/.local/share/pnpm" +mkdir -p "${PNPM_HOME}" +export PATH="${PNPM_HOME}:${PATH}" corepack enable && corepack prepare pnpm --activate.devcontainer/post-create-command.sh (1)
22-22: Use explicit host/user for psql to avoid local-socket misrouting.Match other calls and connect to the db service explicitly.
-psql -c "CREATE USER \"test-user\" WITH PASSWORD 'test-password' CREATEDB;" || echo "User test-user might already exist" +psql -h db -U postgres -c "CREATE USER \"test-user\" WITH PASSWORD 'test-password' CREATEDB;" || echo "User test-user might already exist"
🧹 Nitpick comments (4)
.devcontainer/post-create-command.sh (4)
1-3: Harden shell options.Prefer -euo pipefail to catch unset vars and pipeline errors.
-#!/bin/bash - -set -e +#!/bin/bash +set -euo pipefail
8-8: Avoid hardcoding workspace path.Use devcontainer-provided WORKSPACE_FOLDER fallback for portability.
-cd /workspaces/core +cd "${WORKSPACE_FOLDER:-/workspaces/core}"
28-30: Reconsider global CLI installs; prefer workspace tools via pnpm exec.Global nx/@nestjs/cli can drift from workspace versions. Use workspace-managed CLIs instead.
-# install global CLIs -echo "Installing global CLIs..." -npm i -g nx @nestjs/cli@^8.1.5 foreman apollo graphql +# prefer workspace CLIs (no globals needed) +# usage examples: +# pnpm exec nx --version +# pnpm exec nest --help +# pnpm exec foreman start +# If globals are required, pin to workspace versions to avoid drift.
32-35: Consider lockfile fidelity in CI/devcontainer bootstrap.To ensure reproducible installs, consider honoring the lockfile strictly.
-pnpm install +pnpm install --frozen-lockfile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.devcontainer/Dockerfile(1 hunks).devcontainer/post-create-command.sh(2 hunks).devcontainer/post-start-command.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .devcontainer/Dockerfile
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: package.json:5-5
Timestamp: 2025-09-04T22:08:32.269Z
Learning: In the JesusFilm/core repository, when using corepack in Dockerfiles, use `corepack prepare pnpm --activate` without specifying a version, as corepack will automatically read the version from the packageManager field in package.json. Do not hardcode pnpm versions in Dockerfiles.
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: apps/journeys-admin/project.json:54-59
Timestamp: 2025-09-04T04:02:44.210Z
Learning: In pnpm workspaces, dependencies (including devDependencies) installed at the root package.json are available to all workspace packages via `pnpm exec`, so individual packages don't need to declare these dependencies locally.
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: apps/arclight/Dockerfile:20-20
Timestamp: 2025-09-04T04:01:06.303Z
Learning: In the JesusFilm/core repository, the build process for apps includes package.json with the necessary scripts, so Docker containers can safely use `pnpm start` after copying the dist directory.
📚 Learning: 2025-09-04T04:01:06.303Z
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: apps/arclight/Dockerfile:20-20
Timestamp: 2025-09-04T04:01:06.303Z
Learning: In the JesusFilm/core repository, the build process for apps includes package.json with the necessary scripts, so Docker containers can safely use `pnpm start` after copying the dist directory.
Applied to files:
.devcontainer/post-start-command.sh.devcontainer/post-create-command.sh
📚 Learning: 2025-09-04T22:08:32.269Z
Learnt from: mikeallisonJS
PR: JesusFilm/core#7549
File: package.json:5-5
Timestamp: 2025-09-04T22:08:32.269Z
Learning: In the JesusFilm/core repository, when using corepack in Dockerfiles, use `corepack prepare pnpm --activate` without specifying a version, as corepack will automatically read the version from the packageManager field in package.json. Do not hardcode pnpm versions in Dockerfiles.
Applied to files:
.devcontainer/post-start-command.sh.devcontainer/post-create-command.sh
📚 Learning: 2025-07-22T18:38:06.139Z
Learnt from: CR
PR: JesusFilm/core#0
File: .cursor/rules/kubernetes.mdc:0-0
Timestamp: 2025-07-22T18:38:06.139Z
Learning: Applies to infrastructure/kube/**/*.sh : Use `shellcheck` to lint Bash scripts and improve quality
Applied to files:
.devcontainer/post-start-command.sh
🪛 Shellcheck (0.10.0)
.devcontainer/post-start-command.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ 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). (10)
- GitHub Check: Deploy Preview (watch, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (docs, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (journeys, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (short-links, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (journeys-admin, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (videos-admin, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (watch-modern, 7549/merge, pull_request, 22)
- GitHub Check: Deploy Preview (arclight, 7549/merge, pull_request, 22)
- GitHub Check: build (22)
- GitHub Check: lint (22)
🔇 Additional comments (1)
.devcontainer/post-create-command.sh (1)
24-26: Corepack usage looks correct; no version pin.Enabling Corepack and activating pnpm without a hardcoded version matches our guidance (packageManager field drives the version).
Summary by CodeRabbit
Chores
Devcontainer
CI / Apps / Deployment
Docker
Tests
Dependencies