KK-1539 Yarn classic to pnpm#403
Open
mikkojamG wants to merge 22 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMigrates the project to pnpm and Node 24, rewrites the Docker build/runtime flow, adds runtime environment file generation and browser loading, and switches app configuration, Apollo, and Sentry to shared env helpers. Changespnpm/Node 24 migration + repo updates
Sequence Diagram(s)sequenceDiagram
participant pnpm as pnpm update-runtime-env
participant script as scripts/update-runtime-env.ts
participant dotenv as dotenv.config
participant fs as fs.writeFile
participant html as index.html
participant browser as Browser
pnpm->>script: start()
script->>dotenv: load .env and .env.local
dotenv-->>script: merged env vars
script->>fs: write public/env-config.js
browser->>html: request document
html->>browser: load /env-config.js
html->>browser: load /src/index.tsx
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)CHANGELOG.mdmarkdownlint-cli2 wrapper config was not available before execution 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 |
KK-1539 - Pin pnpm@11.5.0 via packageManager field + engines.pnpm - Convert yarn.lock to pnpm-lock.yaml via pnpm import - Translate yarn resolutions to pnpm overrides in pnpm-workspace.yaml - Add supply-chain guardrail: minimumReleaseAge 10080 - Allow native build scripts: esbuild, full-icu, @ffmpeg-installer - Declare formerly-phantom deps that yarn's flat node_modules hoisted (lodash, react-query, react-hook-form, react-router, react-router-dom, ra-core, ra-i18n-polyglot, @mui/styled-engine), deduped to the versions react-admin already resolves - Drop stale v5 @types/react-router* (code uses v6, ships own types) - Remove CRA-leftover src/react-app-env.d.ts (imported react-scripts) - Update Dockerfile to install pnpm via corepack (ubi9/Node 20) - CI to City-of-Helsinki/.github ci-pnpm-node.yml reusable workflow - Replace yarn with pnpm in husky hooks, lint-staged, README, scripts - Add yarn.lock, .yarn/, .yarnrc.yml to .gitignore Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
KK-1539 Phase 2 of the pnpm migration: adopt the runtime environment configuration approach from kukkuu-ui (PR #757) and move onto the shared Helsinki base images on Node 24. - Add envUtils getEnvValue(key): window._env_ -> process.env -> import.meta.env fallback (src/common/utils/envUtils.ts) - Generate public/env-config.js via scripts/update-runtime-env.ts, load it in index.html; add src/types/env.d.ts and tsx devDep - Prefix start/build scripts with update-runtime-env - Migrate all import.meta.env.VITE_* reads to getEnvValue - Bump to Node 24 (.nvmrc, engines.node, @types/node, CI) - Dockerfile: ubi9/nodejs-24-pnpm-builder-base builder + ubi10/nginx-126-spa-standard production with runtime env.sh injection; COPY .env.example as .env; drop build-time VITE_ args (kept VITE_CSP_REPORT_URI for the index.html CSP) - Remove the custom .prod/nginx.conf.template (base image provides nginx and the /readiness endpoint) - Ignore generated env-config.js in eslint/prettier Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
KK-1539 env-config.js is served publicly, so anything in window._env_ is world-readable. Harden the runtime env injection so it cannot expose secrets, restoring the secure-by-default boundary that Vite's envPrefix gave the old import.meta.env model. - update-runtime-env.ts: emit only VITE_-prefixed keys (plus NODE_ENV) into env-config.js, mirroring Vite's envPrefix. Secrets such as BROWSER_TESTS_JWT_SIGN_SECRET can no longer leak into the browser bundle even if present in the build/runtime environment. - .env.example: drop BROWSER_TESTS_* keys. This file is also copied into the production image as the env-config.js key template; browser-test settings (incl. secrets) belong in .env.test.local (see .env.test.local.example). Recommended hardening for other City-of-Helsinki SPAs using the same runtime env-config.js pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address bot review findings on PR #394: - Add getEnvBoolean() helper. getEnvValue() returns env values as strings, so Boolean(value) treated "false"/"0" as true. This broke IS_TEST_ENVIRONMENT (".env.example"/".env.test" set it to 0, so it was always true) and made VITE_OIDC_AUTOMATIC_SILENT_RENEW_ENABLED impossible to disable. The helper recognizes true/1 and false/0 (case-insensitive). - Fix trailing-comma typo in the VITE_OIDC_SCOPE getEnvOrError label. - Add regression tests for IS_TEST_ENVIRONMENT parsing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback (mikkojamG) on the broad `COPY . ./` in the build stage. The final production image is already lean (only /build is copied into the nginx stage), but the shared appbase/staticbuilder layers and the build context pulled in everything. Exclude paths outside the production build graph (tsconfig "include" is ["src"]): tests, browser-tests, docs, pipelines, .github, and the codegen/lint/release tooling and quality reports. Also: - exclude *.local env files so local secrets (e.g. the browser-test JWT secret in .env.test.local) are never baked into an image layer. - fix a stale entry: LICENSE -> LICENSE.md (old pattern never matched). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the broad `COPY . ./` in the build stage with explicit copies, matching the pattern used in other City-of-Helsinki UI repos (e.g. kerrokantasi-ui). Per stage: - copy scripts/, public/ and .env* before running update-runtime-env; - copy the build configs (Vite, .babelrc for @vitejs/plugin-react, and the eslint/prettier configs run by @nabla/vite-plugin-eslint) plus src/ for the `tsc && vite build`. Editing src/ no longer invalidates the install/update-runtime-env layers, and only the production build graph enters the image. Verified locally: staticbuilder, production and development targets all build successfully (tsc + vite build pass, assets emitted). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The browser-test harness mints its login-bypass session for the
dev OIDC client (kukkuu-admin-ui-dev) and uses the dev API, but the
deployed review app read its client id from env-config.js, which
fell back to the test default (kukkuu-admin-ui-test) from
.env.example. oidc-client-ts keys the stored session as
oidc.user:{authority}:{client_id}, so the mismatch meant the app
never found the injected session and every test was stuck on the
login page.
Override the dev VITE_OIDC_* / VITE_API_URI values in the review
pipeline's configMap so the runtime env-config.js matches what the
harness expects. The kukkuu-pipelines template ref (pnpm_KK-1539)
currently omits these; this override can be dropped once that ref
is reverted to default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mikkojamG
force-pushed
the
KK1539-migrate-kukkuu-admin-yarn-to-pnpm
branch
from
57c4c4f to
f0d95fc
Compare
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is failed for https://kukkuu-admin-pr403.dev.hel.ninja 😿💢💥💥 |
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is failed for https://kukkuu-admin-pr403.dev.hel.ninja 😿💢💥💥 |
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is failed for https://kukkuu-admin-pr403.dev.hel.ninja 😿💢💥💥 |
1 similar comment
TestCafe result is failed for https://kukkuu-admin-pr403.dev.hel.ninja 😿💢💥💥 |
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is failed for https://kukkuu-admin-pr403.dev.hel.ninja 😿💢💥💥 |
mikkojamG
force-pushed
the
KK1539-migrate-kukkuu-admin-yarn-to-pnpm
branch
from
7623a18 to
556ee20
Compare
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-admin-pr403.dev.hel.ninja 😆🎉🎉🎉 |
mikkojamG
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
115
to
119
| */ | ||
| static get oidcKukkuuAPIScope() { | ||
| return getEnvOrError( | ||
| import.meta.env.VITE_KUKKUU_API_OIDC_SCOPE, | ||
| getEnvValue('VITE_KUKKUU_API_OIDC_SCOPE'), | ||
| 'VITE_KUKKUU_API_OIDC_SCOPE' |
There was a problem hiding this comment.
Bug: The required environment variable VITE_KUKKUU_API_OIDC_SCOPE is missing from all .env files, which can cause a runtime crash if other specific environment variables are not set.
Severity: HIGH
Suggested Fix
Add the VITE_KUKKUU_API_OIDC_SCOPE variable with a suitable value to all relevant .env files, including .env.local.example and .env.test.local.example, to ensure the application is correctly configured for all environments. Alternatively, provide a safe default value in the code if applicable.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/domain/application/AppConfig.ts#L115-L119
Potential issue: The environment variable `VITE_KUKKUU_API_OIDC_SCOPE` is read in
`AppConfig.ts` within the `oidcKukkuuAPIScope` getter, which will throw a runtime error
if the variable is not defined. This variable is missing from all `.env` files,
including example templates. While current execution paths may not trigger this getter
due to fallbacks for `AppConfig.oidcAudience` and `AppConfig.oidcKukkuuApiClientId`
being satisfied by other environment variables, the application is fragile. If
`VITE_OIDC_KUKKUU_API_CLIENT_ID` is not set in a specific deployment, the code will
attempt to access `oidcKukkuuAPIScope` as a fallback, causing the application to crash
on startup.
Also affects:
.env:1~18
Did we get this right? 👍 / 👎 to inform future reviews.
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/tsconfig.json (1)
10-10: 📐 Maintainability & Code Quality | 🔵 TrivialAdd Node typings to scripts tsconfig for proper type checking.
The scripts are executed with Node.js (
tsx --tsconfig ./scripts/tsconfig.json scripts/update-runtime-env.ts), yet the tsconfig only includesvite/clienttypes. The script uses Node.js APIs (fs,path,urlmodule,processglobal) that requirenodetype definitions. Replacevite/clientwithnodeto enable proper type checking.Suggested patch
- "types": ["vite/client"] + "types": ["node"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tsconfig.json` at line 10, The scripts/tsconfig.json file currently specifies only "vite/client" in the types array, but the scripts are executed with Node.js and use Node.js APIs like fs, path, url, and the process global. Replace "vite/client" with "node" in the types array to ensure proper type checking and IDE support for Node.js APIs used in scripts like update-runtime-env.ts.src/domain/__tests__/config.test.js (1)
24-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClone
process.envsnapshots to make cleanup effective.These tests mutate
process.env, but the outer hooks keep a reference (env = process.env), so restoration reuses the mutated object. Use a cloned snapshot for isolation (Line 7 and Line 11).🧪 Proposed fix
describe('Config', () => { let env = {}; beforeEach(() => { - env = process.env; + env = { ...process.env }; }); afterEach(() => { - process.env = env; + process.env = { ...env }; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/__tests__/config.test.js` around lines 24 - 43, The test suite is mutating process.env but not properly isolating changes because outer hooks keep a reference to the live process.env object. To fix this, create a cloned snapshot of process.env in a beforeAll hook (at the outer scope before the IS_TEST_ENVIRONMENT describe block) using Object.assign or spread operator to capture the current state, then restore the entire environment from that snapshot in an afterAll hook after all tests in that describe block complete. This ensures that mutations made during individual tests do not persist and the afterEach cleanup in the IS_TEST_ENVIRONMENT describe block operates on a properly isolated object rather than a reference to the mutated original.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 72-80: The COPY commands in the Dockerfile (lines 72, 77, and 80)
are copying files without explicitly setting ownership, which results in
root-owned files. Since the container runs as UID 1001 as inherited from the
base image, the env.sh script at runtime will not have write permissions to
update env-config.js. Add the --chown=1001:1001 flag to all three COPY commands
(the one copying from staticbuilder to /usr/share/nginx/html, the one copying
.env, and the one copying package.json) to ensure the runtime user owns these
files and can write to them when env.sh executes at container start.
In `@src/api/apolloClient/client.ts`:
- Around line 12-13: The createUploadLink function is being passed a potentially
undefined URI from getEnvValue('VITE_API_URI'), which violates the required
contract that AppConfig.apiUrl expects. Modify the code to fail fast by ensuring
VITE_API_URI is required rather than optional when creating the uploadLink. Use
a pattern that throws an error or enforces the environment variable is present
before passing it to createUploadLink, so configuration issues are caught
immediately instead of causing runtime failures during actual requests.
---
Nitpick comments:
In `@scripts/tsconfig.json`:
- Line 10: The scripts/tsconfig.json file currently specifies only "vite/client"
in the types array, but the scripts are executed with Node.js and use Node.js
APIs like fs, path, url, and the process global. Replace "vite/client" with
"node" in the types array to ensure proper type checking and IDE support for
Node.js APIs used in scripts like update-runtime-env.ts.
In `@src/domain/__tests__/config.test.js`:
- Around line 24-43: The test suite is mutating process.env but not properly
isolating changes because outer hooks keep a reference to the live process.env
object. To fix this, create a cloned snapshot of process.env in a beforeAll hook
(at the outer scope before the IS_TEST_ENVIRONMENT describe block) using
Object.assign or spread operator to capture the current state, then restore the
entire environment from that snapshot in an afterAll hook after all tests in
that describe block complete. This ensures that mutations made during individual
tests do not persist and the afterEach cleanup in the IS_TEST_ENVIRONMENT
describe block operates on a properly isolated object rather than a reference to
the mutated original.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fa99979-cbd5-4c08-864b-938279f4949e
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
.dockerignore.env.env.local.example.env.test.env.test.local.example.eslintignore.github/workflows/ci.yml.gitignore.husky/pre-commit.nvmrc.prettierignore.prod/nginx.conf.templateCHANGELOG.mdDockerfileREADME.mddocs/setup-keycloak.mddocs/setup-local-kukkuu-api.mddocs/setup-tunnistamo.mdindex.htmllint-staged.config.jspackage.jsonpipelines/kukkuu-admin-review.ymlpnpm-workspace.yamlscripts/tsconfig.jsonscripts/update-runtime-env.tssrc/api/apolloClient/client.tssrc/api/apolloClient/unauthenticatedClient.tssrc/common/utils/envUtils.tssrc/domain/__tests__/config.test.jssrc/domain/application/AppConfig.tssrc/domain/config.tssrc/index.tsxsrc/react-app-env.d.tssrc/types/env.d.ts
💤 Files with no reviewable changes (5)
- .env.test
- .env.local.example
- .prod/nginx.conf.template
- src/react-app-env.d.ts
- .env.test.local.example
Comment on lines
72
to
+80
| COPY --from=staticbuilder /app/build /usr/share/nginx/html | ||
| # Copy nginx config | ||
| COPY --from=staticbuilder /tmp/.prod/nginx.conf /etc/nginx/nginx.conf | ||
|
|
||
| USER 1001 | ||
| # 2. Setup Runtime Env Injection | ||
| # env.sh is provided by the base image | ||
| WORKDIR /usr/share/nginx/html | ||
| COPY .env . | ||
|
|
||
| # Run script uses standard ways to run the application | ||
| CMD ["/bin/bash", "-c", "nginx -g \"daemon off;\""] | ||
| # 3. Inject Versioning for the /readiness endpoint from package.json using base image | ||
| COPY package.json . |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ensure runtime env injection files are writable by the runtime user.
env.sh updates env-config.js at container start, and this stage documents inherited USER 1001. On Line 72, Line 77, and Line 80, COPY without --chown can leave runtime files root-owned and non-writable for UID 1001, which risks stale/missing runtime env config.
🔧 Proposed fix
FROM helsinki.azurecr.io/ubi10/nginx-126-spa-standard AS production
# 1. Copy the compiled assets.
-COPY --from=staticbuilder /app/build /usr/share/nginx/html
+COPY --from=staticbuilder --chown=1001:0 /app/build /usr/share/nginx/html
# 2. Setup Runtime Env Injection
# env.sh is provided by the base image
WORKDIR /usr/share/nginx/html
-COPY .env .
+COPY --chown=1001:0 .env .
# 3. Inject Versioning for the /readiness endpoint from package.json using base image
-COPY package.json .
+COPY --chown=1001:0 package.json .📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| COPY --from=staticbuilder /app/build /usr/share/nginx/html | |
| # Copy nginx config | |
| COPY --from=staticbuilder /tmp/.prod/nginx.conf /etc/nginx/nginx.conf | |
| USER 1001 | |
| # 2. Setup Runtime Env Injection | |
| # env.sh is provided by the base image | |
| WORKDIR /usr/share/nginx/html | |
| COPY .env . | |
| # Run script uses standard ways to run the application | |
| CMD ["/bin/bash", "-c", "nginx -g \"daemon off;\""] | |
| # 3. Inject Versioning for the /readiness endpoint from package.json using base image | |
| COPY package.json . | |
| COPY --from=staticbuilder --chown=1001:0 /app/build /usr/share/nginx/html | |
| # 2. Setup Runtime Env Injection | |
| # env.sh is provided by the base image | |
| WORKDIR /usr/share/nginx/html | |
| COPY --chown=1001:0 .env . | |
| # 3. Inject Versioning for the /readiness endpoint from package.json using base image | |
| COPY --chown=1001:0 package.json . |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 72 - 80, The COPY commands in the Dockerfile (lines
72, 77, and 80) are copying files without explicitly setting ownership, which
results in root-owned files. Since the container runs as UID 1001 as inherited
from the base image, the env.sh script at runtime will not have write
permissions to update env-config.js. Add the --chown=1001:1001 flag to all three
COPY commands (the one copying from staticbuilder to /usr/share/nginx/html, the
one copying .env, and the one copying package.json) to ensure the runtime user
owns these files and can write to them when env.sh executes at container start.
Comment on lines
12
to
+13
| const uploadLink = createUploadLink({ | ||
| uri: import.meta.env.VITE_API_URI, | ||
| uri: getEnvValue('VITE_API_URI'), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail fast when VITE_API_URI is missing.
Line 13 passes a possibly undefined URI, while AppConfig.apiUrl treats the same variable as required. This breaks the config contract and can hide misconfiguration until runtime requests fail.
💡 Proposed fix
+const apiUri = getEnvValue('VITE_API_URI');
+if (!apiUri) {
+ throw new Error('Missing required environment variable: VITE_API_URI');
+}
+
const uploadLink = createUploadLink({
- uri: getEnvValue('VITE_API_URI'),
+ uri: apiUri,
headers: {
'keep-alive': 'true',
},
});📝 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.
Suggested change
| const uploadLink = createUploadLink({ | |
| uri: import.meta.env.VITE_API_URI, | |
| uri: getEnvValue('VITE_API_URI'), | |
| const apiUri = getEnvValue('VITE_API_URI'); | |
| if (!apiUri) { | |
| throw new Error('Missing required environment variable: VITE_API_URI'); | |
| } | |
| const uploadLink = createUploadLink({ | |
| uri: apiUri, | |
| headers: { | |
| 'keep-alive': 'true', | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/apolloClient/client.ts` around lines 12 - 13, The createUploadLink
function is being passed a potentially undefined URI from
getEnvValue('VITE_API_URI'), which violates the required contract that
AppConfig.apiUrl expects. Modify the code to fail fast by ensuring VITE_API_URI
is required rather than optional when creating the uploadLink. Use a pattern
that throws an error or enforces the environment variable is present before
passing it to createUploadLink, so configuration issues are caught immediately
instead of causing runtime failures during actual requests.
TestCafe result is success for https://kukkuu-admin-pr403.dev.hel.ninja 😆🎉🎉🎉 |
mikkojamG
force-pushed
the
KK1539-migrate-kukkuu-admin-yarn-to-pnpm
branch
from
c5ec827 to
882d217
Compare
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-admin-pr403.dev.hel.ninja 😆🎉🎉🎉 |
| try { | ||
| fs.writeFile( | ||
| configurationFile, | ||
| 'globalThis.window._env_ = ' + util.inspect(publicEnv, false, 2, false), |
There was a problem hiding this comment.
Bug: The script uses util.inspect() to generate env-config.js. This method is not guaranteed to produce valid JavaScript and can fail if environment variables contain special characters.
Severity: MEDIUM
Suggested Fix
Replace util.inspect(publicEnv, false, 2, false) with JSON.stringify(publicEnv). This will ensure the generated object is always valid JavaScript syntax, regardless of the content of the environment variable strings.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: scripts/update-runtime-env.ts#L52
Potential issue: The script at `scripts/update-runtime-env.ts` uses `util.inspect()` to
serialize environment variables into the `public/env-config.js` file. The Node.js
documentation explicitly warns that the output of `util.inspect()` is not guaranteed to
be valid JavaScript and should not be depended upon programmatically. While current
`.env` values are simple and do not trigger this issue, future environment variables
containing special characters (e.g., backslashes, newlines) could generate a file with a
syntax error. This would cause `window._env_` to be undefined, making the application
silently fall back to using build-time variables, potentially connecting to incorrect
services in production.
|
|
||
| USER 1001 | ||
| # 2. Setup Runtime Env Injection | ||
| # env.sh is provided by the base image |
There was a problem hiding this comment.
Bug: The application has a critical, undocumented dependency on an env.sh script from the base Docker image. If it fails, the app silently uses test configuration in production.
Severity: HIGH
Suggested Fix
The dependency on the external env.sh script should be removed or made explicit. The application's own repository should contain the script responsible for generating the runtime configuration in the production container, ensuring it is version-controlled and validated alongside the application code. This eliminates the risk of the base image changing unexpectedly.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: Dockerfile#L75
Potential issue: The production Docker container relies on an external, undocumented
`env.sh` script from a base image to provide runtime environment variables. If this
script is missing, fails, or is incompatible with the application's expected
`window._env_` format, the application will silently fall back to using build-time
configuration from the test `.env` file. This would cause the production application to
connect to test APIs and services instead of production ones, as there is no validation
or error handling for this failure scenario.
|
|
KUKKUU-ADMIN branch is deployed to platta: https://kukkuu-admin-pr403.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-admin-pr403.dev.hel.ninja 😆🎉🎉🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reopening: #394
https://dev.azure.com/City-of-Helsinki/kukkuu/_git/kukkuu-pipelines/pullrequest/16282
Context
KK-1539
How Has This Been Tested?
Manual Testing Instructions for Reviewers
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Chores