Conversation
|
This update introduces separate GitHub workflows for backend and frontend installations, aimed at ensuring that dependencies for each are correctly installed and the services can start successfully. Workflow triggers are linked to changes in relevant files. Additionally, modifications were made to lint workflows to specify paths for triggering lints, including markdown files and structured JSON paths for frontend. Walkthrough
Model: gpt-4o | Prompt Tokens: 1686 | Completion Tokens: 137 |
There was a problem hiding this comment.
Here's a friendly code review powered by AI assistance. These insights offer suggestions and patterns we've noticed, though they're recommendations rather than absolute requirements. You know your project best, so please take what's useful and leave what isn't. Think of this as a collaborative partner in your development process—you're always in control of the final decisions.
Always critique what AI says. Do not let AI replace YOUR I.
Model: anthropic--claude-4-sonnet | Prompt Tokens: 3254 | Completion Tokens: 1113
| - name: Set up Python environment | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.11" | ||
| token: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
The token parameter is not needed for actions/setup-python@v6 when using public Python distributions. This parameter is typically used for private registries or when downloading from GitHub releases.
- name: Set up Python environment
uses: actions/setup-python@v6
with:
python-version: "3.11"| paths: | ||
| - '**.py' | ||
| - '**.txt' | ||
| - '**/pyproject.toml' | ||
| - '**/uv.lock' |
There was a problem hiding this comment.
Consider adding more comprehensive path filters to improve workflow efficiency. The current patterns might trigger on unrelated changes.
paths:
- 'backend-agent/**/*.py'
- 'backend-agent/**/requirements*.txt'
- 'backend-agent/**/pyproject.toml'
- 'backend-agent/**/uv.lock'| # Start the dev server in background | ||
| npm start & | ||
| DEV_SERVER_PID=$! | ||
|
|
||
| # Wait for server to be ready (max 60 seconds) | ||
| for i in {1..60}; do | ||
| sleep 1 | ||
| if curl -f -s http://localhost:4200 > /dev/null 2>&1; then | ||
| echo "Dev server started successfully" | ||
| kill $DEV_SERVER_PID | ||
| exit 0 | ||
| fi | ||
| done | ||
|
|
||
| echo "Dev server failed to start within 60 seconds" | ||
| kill $DEV_SERVER_PID | ||
| exit 1 No newline at end of file |
There was a problem hiding this comment.
The process cleanup logic could be improved to handle cases where the kill command fails. Consider using a more robust cleanup approach.
# Start the dev server in background
npm start &
DEV_SERVER_PID=$!
# Cleanup function
cleanup() {
if kill -0 $DEV_SERVER_PID 2>/dev/null; then
kill $DEV_SERVER_PID 2>/dev/null || true
wait $DEV_SERVER_PID 2>/dev/null || true
fi
}
trap cleanup EXIT
# Wait for server to be ready (max 60 seconds)
for i in {1..60}; do
sleep 1
if curl -f -s http://localhost:4200 > /dev/null 2>&1; then
echo "Dev server started successfully"
exit 0
fi
done
echo "Dev server failed to start within 60 seconds"
exit 1| kill $DEV_SERVER_PID | ||
| exit 1 No newline at end of file |
There was a problem hiding this comment.
The file is missing a newline at the end. This should be added for better POSIX compliance.
kill $DEV_SERVER_PID
exit 1| for i in {1..20}; do | ||
| sleep 1 | ||
| status=$(curl -s -o /dev/null -w "%{http_code}" http://localhost:8080/health || true) | ||
| if [ "$status" -eq 200 ]; then | ||
| echo "Health check succeeded" | ||
| cat server.log | ||
| exit 0 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Consider using a more explicit health check URL and add timeout handling for the curl command to prevent hanging.
for i in {1..20}; do
sleep 1
status=$(timeout 5 curl -s -o /dev/null -w "%{http_code}" http://localhost:8080/health 2>/dev/null || echo "000")
if [ "$status" -eq 200 ]; then
echo "Health check succeeded"
cat server.log
exit 0
fi
echo "Attempt $i: Health check returned status $status"
done| - '**.json' | ||
| - '**.ts' | ||
| - '**.js' | ||
| - '**/package.json' | ||
| - 'CHANGELOG.md' |
There was a problem hiding this comment.
The path pattern change from '**.json' to '**/package.json' is more specific and appropriate, but consider if other JSON files in the frontend directory should also trigger linting (e.g., tsconfig.json, angular.json).
paths:
- '**.ts'
- '**.js'
- '**/package.json'
- '**/tsconfig*.json'
- 'CHANGELOG.md'
Add installation test for frontend, and rename the installation test for backend for consistency.
Fix linter actions (previously interrupted as soon as changelog-ci committed the changelog)