Move Next env to test images #2330
Conversation
Summary by CodeRabbit
WalkthroughRemoved inline NEXT_PUBLIC_ENVIRONMENT from Makefile docker builds, added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
github-actions
bot
added
frontend
docker
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/package.json(1 hunks)
⏰ 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). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
frontend/package.json (1)
18-18: Verify: Tests now run even when type-checking failsChanging
&&to;meansjestwill execute regardless of whethertscsucceeds, which could mask type errors during local development and testing.However, note that
noEmitis already configured intsconfig.json(line 7:"noEmit": true), so TypeScript will not emit compiled output regardless of command-line flags.- "test:unit": "tsc && NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest" + "test:unit": "tsc; NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest"Consider: Is this change intentional for the Docker/CI test image context mentioned in this PR? The CI workflow runs this command inside a Docker container, so verify whether allowing tests to run despite type errors is the desired behavior for your test pipeline.
| "start": "next start", | ||
| "test:e2e": "npx playwright test", | ||
| "test:unit": "tsc --noEmit && NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest" | ||
| "test:unit": "tsc; NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' jest" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify: Is this change related to the PR objective?
The PR is about moving NEXT_PUBLIC_ENVIRONMENT to test Docker images, but this change modifies TypeScript compilation and test execution behavior. Please clarify:
- Is this change intentional or a workaround for another issue?
- Are there type errors that need to be addressed?
- Does the environment variable change affect TypeScript compilation?
Run this script to check for TypeScript errors:
🏁 Script executed:
#!/bin/bash
# Check for TypeScript compilation errors in the frontend
cd frontend && npx tsc --noEmit 2>&1 | head -50Length of output: 626
I need to gather more information to verify this change. Let me check the TypeScript installation and the broader context of the PR changes.
🏁 Script executed:
#!/bin/bash
# Check if TypeScript is installed and what version
cd frontend
if [ -f package.json ]; then
echo "=== TypeScript in dependencies ==="
cat package.json | jq '.dependencies.typescript // .devDependencies.typescript // "not found"'
echo ""
fi
# Try running tsc from node_modules
echo "=== Running TypeScript compiler ==="
if [ -f node_modules/.bin/tsc ]; then
./node_modules/.bin/tsc --noEmit 2>&1 | head -100
else
echo "TypeScript not installed in node_modules"
fiLength of output: 121
🏁 Script executed:
#!/bin/bash
# Check the git diff for this specific file to see the full change
echo "=== Git diff for frontend/package.json ==="
git diff HEAD~1 HEAD -- frontend/package.json 2>/dev/null || git diff --cached -- frontend/package.json 2>/dev/null || echo "Could not retrieve git diff"Length of output: 597
🏁 Script executed:
#!/bin/bash
# Look for PR description or commit messages
echo "=== Recent commit messages ==="
git log --oneline -5 2>/dev/null || echo "Git log unavailable"
echo ""
echo "=== Check for environment variable usage ==="
cd frontend
rg -n "NEXT_PUBLIC_ENVIRONMENT" --type=json --type=js --type=ts -g '!node_modules' -C2 | head -50Length of output: 365
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/run-ci-cd.yaml (1)
208-208: Consider the implications of--forceExitand direct Jest invocation.The updated command introduces several changes:
--forceExitflag: Forces Jest to exit after all tests complete, which can mask issues with improper async cleanup (e.g., open handles, timers, or connections). This may hide underlying problems that should be fixed.Direct Jest invocation: Bypasses the
test:unitscript inpackage.json, which can lead to drift between local development (pnpm run test:unit) and CI. If the script inpackage.jsonchanges, CI won't reflect those changes unless this workflow is also updated.Command complexity: The inline command is more verbose and harder to maintain compared to delegating to a package.json script.
Recommendation: Consider one of the following approaches:
Option 1 (Preferred): Use
pnpm run test:unitand move the NODE_OPTIONS and Jest flags to thepackage.jsonscript or a Jest config file. This ensures consistency between local and CI environments.- name: Run frontend unit tests run: | - docker run --env-file frontend/.env.example owasp/nest:test-frontend-unit-latest sh -c "NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' npx jest --ci --no-cache --verbose --detectOpenHandles --forceExit" + docker run --env-file frontend/.env.example owasp/nest:test-frontend-unit-latest pnpm run test:unitThen update
frontend/package.json:"test:unit": "NODE_OPTIONS='--experimental-vm-modules --no-warnings=DEP0040' tsc && jest --ci --no-cache --verbose --detectOpenHandles"Option 2: If
--forceExitis necessary, document why it's needed (e.g., known issue with a specific library). However, prefer fixing the underlying async cleanup issues instead.Option 3: Remove
--detectOpenHandlesin CI if it's primarily a debugging tool, as it can slow down test execution.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/run-ci-cd.yaml(1 hunks)
⏰ 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). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
Proposed change
Test Next env in the test images
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.