Cors env fix #34
Cors env fix #34
Conversation
WalkthroughThe pull request introduces multiple changes across various files, primarily focusing on updating configurations for frontend development and backend services. Key modifications include updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (4)
hub/conf/application.conf (1)
16-25: Summary: CORS configuration added but needs refinementThe CORS configuration has been successfully added, which partially addresses the PR objectives. However, there are a few important points to address:
- The configuration is currently hardcoded, which doesn't align with the goal of injecting environment variables from the Docker container.
- The allowed origins are set for local development (ports 5173 and 3000), which may not be suitable for production environments.
- No environment variables are used in this configuration, contrary to the PR objectives.
To fully meet the PR objectives and enhance security:
- Refactor the CORS configuration to use environment variables.
- Update the Docker configuration to provide these environment variables.
- Ensure that the configuration can be easily adjusted for different environments (development, staging, production).
These changes will improve flexibility, security, and alignment with the stated PR objectives.
.gitignore (1)
Line range hint
73-73: Good addition of frontend test results to .gitignoreAdding
/frontend/test-resultsto the .gitignore file is a good practice. Test results should not be committed to version control as they can vary between environments and runs. This change aligns with the PR checklist mentioning that changes are covered by existing CI processes.However, for consistency with other entries in this file, consider using a more specific pattern:
-/frontend/test-results +/frontend/test-results/This trailing slash ensures that only the directory (and all its contents) is ignored, not potential files with the same name.
docker-init/compose.yaml (1)
75-76: Approve the addition of SERVER_API_URL environment variable.The addition of the
SERVER_API_URLenvironment variable is a good practice. It allows for dynamic configuration of the API endpoint, which aligns with the PR objective of enhancing environment variable handling.Consider using a placeholder for the port number to make it more flexible:
- SERVER_API_URL=http://app:9000 + SERVER_API_URL=http://app:${APP_PORT:-9000}This change would make the configuration more consistent with the
appservice's healthcheck and allow for easier port changes in the future.build.sbt (1)
Line range hint
196-214: Approved: Enhanced frontend build processThe changes to the frontend build process are well-aligned with the PR objectives. The new process allows for better error handling and ensures proper integration of the built frontend with the Play application.
However, consider adding a step to clean the
hub/publicdirectory before copying new assets to prevent any stale files from persisting.Consider adding a cleanup step before copying new assets:
// Clean the target directory if needed -IO.delete(publicDir) +IO.delete(publicDir) +IO.createDirectory(publicDir)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
- .gitignore (1 hunks)
- build.sbt (1 hunks)
- docker-init/compose.yaml (1 hunks)
- frontend/.env (0 hunks)
- frontend/.env.docker (0 hunks)
- frontend/package.json (0 hunks)
- frontend/src/lib/api/api.ts (1 hunks)
- hub/conf/application.conf (1 hunks)
💤 Files with no reviewable changes (3)
- frontend/.env
- frontend/.env.docker
- frontend/package.json
🔇 Additional comments (12)
frontend/src/lib/api/api.ts (3)
2-2: LGTM: Import statement for browser environment detection.The addition of the
browserimport from$app/environmentis appropriate for determining the runtime environment, which is crucial for the new API base URL logic.
8-8: LGTM: Base constant declaration.The construction of the
baseconstant using the newapiBaseUrlis correct and maintains the existing API endpoint structure.
Line range hint
10-26: Verify API functionality across different environments.While the changes to
apiBaseUrlandbasedon't directly modify thesendandgetfunctions, it's crucial to ensure that API calls work correctly in all environments (local development, staging, production) after these changes.Please thoroughly test the API functionality:
- In local development (browser and server-side)
- In the Docker container environment
- In your staging/production environments
This will help catch any potential issues with the new URL construction logic.
To assist with verification, you can use the following script to check API call implementations:
✅ Verification successful
API URL Construction Verified
The definitions of
apiBaseUrlandbasehave been reviewed and are correctly structured to handle different environments. No issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API call implementations rg "send\(\{|get\(" --type typescript --type javascriptLength of output: 92
Script:
#!/bin/bash # Search for API call implementations with corrected file types rg "send\(\{|get\(" --type ts --type jsLength of output: 837
Script:
#!/bin/bash # Search for definitions and assignments of 'apiBaseUrl' and 'base' rg "(const|let|var)\s+(apiBaseUrl|base)\s*=" --type ts --type jsLength of output: 187
Script:
#!/bin/bash # Retrieve the full definition of 'apiBaseUrl' and 'base' with context rg "(const|let|var)\s+(apiBaseUrl|base)\s*=" --context 2 --type ts --type js frontend/src/lib/api/api.tsLength of output: 377
hub/conf/application.conf (2)
18-18: LGTM: CORS filter enabled correctly.The addition of the CORS filter to the enabled filters list is correct and aligns with the PR objectives.
21-25: 🛠️ Refactor suggestion
⚠️ Potential issueRefactor CORS configuration to use environment variables.
While the CORS configuration is functional, it doesn't fully align with the PR objectives:
- The configuration is hardcoded, which doesn't match the goal of injecting environment variables from the Docker container.
- The allowed origins are set for local development, which might not be suitable for production.
- No environment variables are used in this configuration.
Consider refactoring the CORS configuration to use environment variables:
play.filters.cors { allowedOrigins = ${?ALLOWED_ORIGINS} allowedHttpMethods = ${?ALLOWED_HTTP_METHODS} allowedHttpHeaders = ${?ALLOWED_HTTP_HEADERS} }This approach allows for different configurations in development and production environments, enhancing flexibility and security.
To ensure this change doesn't break existing functionality, please verify the Docker setup:
Would you like assistance in updating the Docker configuration to include these environment variables?
.gitignore (3)
63-67: Excellent addition of frontend environment file handling!These new entries align well with the PR objective of enhancing environment variable handling. Here's why this change is beneficial:
- Ignoring
/frontend/.envand/frontend/.env.*prevents sensitive environment-specific configurations from being committed to the repository, enhancing security.- Keeping
!/frontend/.env.exampleand!/frontend/.env.testunignored allows for maintaining example configurations and test setups in the repository, which is helpful for onboarding and CI processes.This change supports the move towards injecting environment variables from the Docker container instead of hard-coding them during the build process.
69-70: Good addition of Vite configuration timestamp files to .gitignoreAdding
/frontend/vite.config.js.timestamp-*and/frontend/vite.config.ts.timestamp-*to the .gitignore file is a smart move. These files are generated during the build process and don't need to be tracked in version control. Ignoring them helps:
- Keep the repository clean
- Prevent unnecessary merge conflicts
- Ensure that only the actual configuration files (not their timestamps) are versioned
This change aligns with best practices for managing build artifacts in a frontend project.
Line range hint
63-73: Overall, these .gitignore changes enhance frontend development practicesThe additions to the .gitignore file are well-thought-out and align perfectly with the PR objectives. They improve the project's handling of environment variables, build artifacts, and test results. These changes:
- Enhance security by preventing sensitive environment data from being committed
- Keep the repository clean by ignoring build-time generated files
- Maintain useful example configurations for onboarding and testing
- Align with best practices for frontend development
These modifications support the transition to injecting environment variables from Docker containers and demonstrate good version control hygiene.
docker-init/compose.yaml (1)
Line range hint
1-83: Verify alignment between PR objectives and implemented changes.While the addition of the
SERVER_API_URLenvironment variable enhances environment variable handling as mentioned in the PR objectives, there are some discrepancies:
- The PR objectives mention CORS configuration updates, but there are no visible CORS-related changes in this file.
- The objectives also mention allowing requests from local development port (5173) and Docker Node.js server port (3000), but these are not reflected in the changes.
Please clarify if the CORS-related changes are implemented in other files not included in this review. If so, consider updating the PR description to accurately reflect the scope of changes in this file.
To verify the implementation of CORS changes, you can run the following script:
This script will help identify any CORS-related configurations and specific port settings that might be relevant to the PR objectives.
✅ Verification successful
CORS configurations and required ports are correctly implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for CORS-related configurations in the codebase # Test 1: Search for CORS configurations echo "Searching for CORS configurations:" rg --type-add 'config:*.{js,ts,py,java,scala}' --type config 'cors' -g '!node_modules' # Test 2: Search for specific port configurations (5173 and 3000) echo "Searching for port configurations:" rg --type-add 'config:*.{js,ts,py,java,scala,yaml,yml}' --type config '(5173|3000)' -g '!node_modules'Length of output: 2506
build.sbt (3)
Line range hint
24-24: Approved: Spark version updateThe update from Spark 3.5.0 to 3.5.1 is a good practice. Minor version updates typically include bug fixes and improvements, which can enhance the overall stability and performance of the project.
Line range hint
226-236: Approved: Hub project settings update with a queryThe update to Scala 2.13 and the explicit listing of dependencies are good improvements. The addition of circe libraries suggests enhanced JSON handling capabilities.
However, I noticed that the PR objectives mention updates to CORS configuration, but I don't see any CORS-related changes in this file. Could you clarify where the CORS configuration changes are implemented?
To verify the CORS configuration, please run the following script:
✅ Verification successful
CORS Configuration Verified in
application.confThe CORS settings are correctly implemented in
hub/conf/application.conf. There are no required changes inbuild.sbtregarding CORS.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for CORS configuration in the project echo "Searching for CORS configuration..." rg --type scala --type conf --type hocon 'cors|origin' -g '!target' echo "Searching for Play framework configuration files..." fd -e conf -e scala 'application.conf|routes' -E 'target'Length of output: 450
Script:
#!/bin/bash # Description: Corrected search for CORS configuration in the project echo "Searching for CORS configuration..." rg --type scala --type conf --type hocon 'cors|origin' -g '!target' echo "Searching for Play framework configuration files..." fd -e conf -e scala 'application.conf|routes' -E 'target'Length of output: 376
Script:
#!/bin/bash # Description: Corrected search for CORS configuration in application.conf echo "Searching for CORS configuration in hub/conf/application.conf..." rg 'cors|origin' hub/conf/application.confLength of output: 258
Line range hint
1-243: Summary of build.sbt changesThe changes to
build.sbtare generally positive:
- Spark version update improves stability.
- Enhanced frontend build process aligns with PR objectives.
- Hub project settings update modernizes the Scala version and improves dependency management.
However, the PR objectives mention CORS configuration updates, which are not visible in this file. It's crucial to ensure that these CORS changes are implemented correctly, possibly in a separate configuration file.
To locate the CORS configuration, please run the following command:
Please provide information on where the CORS configuration changes have been implemented.
There was a problem hiding this comment.
@ken-zlai - largely looks good. One q - don't we need to leave the package-lock.json file though? We'd like to tie the package versions to a fixed set right?
Yes, thats correct. In this case I ran an update script to get the latest versions of our packages. There was something going on with one of the package versions, so updating the packages fixed it. |
## Summary We now inject environment variables from the docker container instead of hard coding them with the build. I also updated the CORS config to allow requests from the local dev port (5173) and the docker node js server port (3000) ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added CORS configuration to enhance API accessibility. - Introduced new environment variables `SERVER_API_URL` and `API_BASE_URL` for frontend service connectivity. - **Bug Fixes** - Updated the base API URL logic for better environment handling. - **Chores** - Removed outdated environment files and scripts related to Docker builds. - Updated `.gitignore` for improved file management in frontend development. - Modified the frontend build command for consistency. - **Documentation** - Adjusted the healthcheck configuration for the frontend service in Docker. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary We now inject environment variables from the docker container instead of hard coding them with the build. I also updated the CORS config to allow requests from the local dev port (5173) and the docker node js server port (3000) ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added CORS configuration to enhance API accessibility. - Introduced new environment variables `SERVER_API_URL` and `API_BASE_URL` for frontend service connectivity. - **Bug Fixes** - Updated the base API URL logic for better environment handling. - **Chores** - Removed outdated environment files and scripts related to Docker builds. - Updated `.gitignore` for improved file management in frontend development. - Modified the frontend build command for consistency. - **Documentation** - Adjusted the healthcheck configuration for the frontend service in Docker. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary We now inject environment variables from the docker container instead of hard coding them with the build. I also updated the CORS config to allow requests from the local dev port (5173) and the docker node js server port (3000) ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added CORS configuration to enhance API accessibility. - Introduced new environment variables `SERVER_API_URL` and `API_BASE_URL` for frontend service connectivity. - **Bug Fixes** - Updated the base API URL logic for better environment handling. - **Chores** - Removed outdated environment files and scripts related to Docker builds. - Updated `.gitignore` for improved file management in frontend development. - Modified the frontend build command for consistency. - **Documentation** - Adjusted the healthcheck configuration for the frontend service in Docker. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary We now inject environment variables from the doour clientser container instead of hard coding them with the build. I also updated the CORS config to allow requests from the local dev port (5173) and the doour clientser node js server port (3000) ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added CORS configuration to enhance API accessibility. - Introduced new environment variables `SERVER_API_URL` and `API_BASE_URL` for frontend service connectivity. - **Bug Fixes** - Updated the base API URL logic for better environment handling. - **Chores** - Removed outdated environment files and scripts related to Doour clientser builds. - Updated `.gitignore` for improved file management in frontend development. - Modified the frontend build command for consistency. - **Documentation** - Adjusted the healthcheour clients configuration for the frontend service in Doour clientser. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
We now inject environment variables from the docker container instead of hard coding them with the build. I also updated the CORS config to allow requests from the local dev port (5173) and the docker node js server port (3000)
Checklist
Summary by CodeRabbit
Release Notes
New Features
SERVER_API_URLandAPI_BASE_URLfor frontend service connectivity.Bug Fixes
Chores
.gitignorefor improved file management in frontend development.Documentation