feat: use run time variables in dist in wf-dashboard #2476
Conversation
|
WalkthroughThis update to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 (
|
pratapalakshmi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
apps/workflows-dashboard/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (6)
- apps/workflows-dashboard/global.d.ts (1 hunks)
- apps/workflows-dashboard/index.html (1 hunks)
- apps/workflows-dashboard/package.json (1 hunks)
- apps/workflows-dashboard/public/config.js (1 hunks)
- apps/workflows-dashboard/src/lib/request/request.ts (1 hunks)
- apps/workflows-dashboard/src/main.tsx (1 hunks)
Files skipped from review due to trivial changes (3)
- apps/workflows-dashboard/index.html
- apps/workflows-dashboard/package.json
- apps/workflows-dashboard/public/config.js
Additional comments not posted (1)
apps/workflows-dashboard/src/main.tsx (1)
6-6: Review the necessity of URL import syntax.The import statement uses
?urlwhich might be intended to ensure the file is treated as a URL dependency. Confirm if this is necessary and correctly implemented as intended.
| declare global { | ||
| export var env: {[key: string]: any}; | ||
| } |
There was a problem hiding this comment.
Consider using const for global declarations.
Using var for the global declaration of env can lead to hoisting issues and is generally less safe than const. Since env is unlikely to be reassigned directly (it's properties might change), using const would be more appropriate.
declare global {
export const env: {[key: string]: any};
}| //@ts-ignore | ||
| baseURL: globalThis.env.VITE_API_URL, |
There was a problem hiding this comment.
Avoid using @ts-ignore where possible.
The use of @ts-ignore can hide potential type issues. It's better to handle these explicitly, either by defining the types or handling potential undefined cases.
// Assuming `env` and `VITE_API_URL` are always defined
baseURL: globalThis.env.VITE_API_URL as string,| //@ts-ignore | ||
| globalThis.env = globalThis.env || { | ||
| API_URL: import.meta.env.VITE_API_URL | ||
| } |
There was a problem hiding this comment.
Refine TypeScript usage and handle potential null cases.
Using @ts-ignore to bypass TypeScript checks is not ideal. Also, consider handling the case where document.getElementById('root') might return null.
const rootElement = document.getElementById('root') as HTMLElement;
if (!rootElement) {
console.error('Root element not found');
throw new Error('Root element not found');
}
ReactDOM.createRoot(rootElement).render(
<React.StrictMode>
<RouterProvider router={router}></RouterProvider>
</React.StrictMode>,
);
// Remove @ts-ignore and ensure type safety
globalThis.env = globalThis.env || {
API_URL: import.meta.env.VITE_API_URL as string
}
pratapalakshmi
force-pushed
the
dist/use/runtime/env/variables
branch
from
2384ed6
to
6211689
Compare
pratapalakshmi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
apps/workflows-dashboard/public/config.js (1)
1-3: Consider the implications of using global variables.Defining environment variables in the global scope can simplify access but may lead to potential issues with maintainability and testing. Consider using a module or a configuration management library to encapsulate these variables, which can help avoid potential conflicts and improve testability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
apps/workflows-dashboard/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (6)
- apps/workflows-dashboard/global.d.ts (1 hunks)
- apps/workflows-dashboard/index.html (1 hunks)
- apps/workflows-dashboard/package.json (1 hunks)
- apps/workflows-dashboard/public/config.js (1 hunks)
- apps/workflows-dashboard/src/lib/request/request.ts (1 hunks)
- apps/workflows-dashboard/src/main.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- apps/workflows-dashboard/global.d.ts
Additional comments not posted (4)
apps/workflows-dashboard/src/lib/request/request.ts (1)
4-5: Avoid using@ts-ignorewhere possible.The use of
@ts-ignorecan hide potential type issues. It's better to handle these explicitly, either by defining the types or handling potential undefined cases. Consider ensuring thatglobalThis.envandVITE_API_URLare properly typed and defined.apps/workflows-dashboard/index.html (1)
9-9: Ensure correct script loading sequence.Make sure that
/config.jsis loaded before any other scripts that depend on the environment variables defined within it. This ensures that all configurations are available when needed.apps/workflows-dashboard/src/main.tsx (1)
7-7: Verify the existence and accessibility ofconfig.js?url.Ensure that the
config.js?urlfile is correctly configured and accessible at runtime. This import is crucial for managing external configuration settings.Verification successful
File
config.jsis correctly located and accessible.The
config.jsfile exists atapps/workflows-dashboard/public/config.js, confirming that the import statement should function as expected. Ensure that the file is correctly configured and accessible at runtime.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of `config.js?url`. # Test: Check if `config.js` exists in the public directory. Expect: File should exist. fd 'config.js' publicLength of output: 115
Script:
#!/bin/bash # Description: Search for `config.js` in the entire repository. # Test: Locate `config.js` file. Expect: File should exist somewhere in the repository. fd 'config.js'Length of output: 1308
apps/workflows-dashboard/package.json (1)
9-9: Good practice: Clean build environment.The addition of
rm -rf distin the build script ensures a clean build environment by removing previous build artifacts. This is a good practice to prevent issues with stale files.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/workflows-dashboard/Dockerfile (1 hunks)
- apps/workflows-dashboard/entrypoint.sh (1 hunks)
- apps/workflows-dashboard/public/config.js (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/workflows-dashboard/public/config.js
Additional context used
Shellcheck
apps/workflows-dashboard/entrypoint.sh
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 8-8: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 13-13: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
Additional comments not posted (6)
apps/workflows-dashboard/Dockerfile (4)
22-22: Set working directory correctly.The
WORKDIR /appcommand is correctly used to set the working directory for subsequent commands. This is a standard practice in Dockerfiles.
26-26: Ensure the entrypoint script is copied correctly.The
COPYcommand is used to copy theentrypoint.shscript from the development stage to the production image. This is crucial for the script to be available in the production environment.
30-30: Correct file permissions for the entrypoint script.The
RUN chmod a+x /app/entrypoint.sh;command correctly sets the executable permissions for theentrypoint.shscript, ensuring it can be executed at runtime.
34-34: Set the entrypoint for the Docker container.The
ENTRYPOINTcommand is correctly updated to use theentrypoint.shscript. This change is crucial for initializing the container with the necessary environment setup.apps/workflows-dashboard/entrypoint.sh (2)
25-32: Review the configuration file generation logic.The script dynamically generates a configuration file
config.jswith runtime environment variables. This approach allows for flexible configuration based on the environment.
34-35: Ensure proper command handling in the script.The
exec "$@"command is used to handle the CMD command passed to the Docker container. This ensures that any command specified in the Docker CMD is executed after the script runs.
pratapalakshmi
force-pushed
the
dist/use/runtime/env/variables
branch
from
dae7cc9
to
6aae2e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
apps/workflows-dashboard/public/config.js (1)
1-6: LGTM! The global configuration object is a good addition.The introduction of the
globalThis.envobject provides a centralized way to manage environment-specific configuration for the application. This approach facilitates the development process by making the necessary environment variables readily accessible.A few suggestions for further improvement:
Consider adding comments to describe the purpose of each property in the
envobject. This will make the code more self-explanatory and easier to maintain.If there are any sensitive configuration values (e.g., API keys, secrets), ensure that they are not hardcoded in this file. Instead, consider using environment variables or a separate configuration management system to securely store and retrieve such values.
If the
envobject is expected to grow with more properties, consider organizing the properties into logical groups or sections for better readability.Overall, this is a good step towards making the application more adaptable to different environments.
apps/workflows-dashboard/src/main.tsx (1)
7-7: Clarify the purpose of using a URL query parameter in the import statement.The import statement is using a URL query parameter
?url, which is not a common practice for importing JavaScript modules. Can you please clarify the purpose of using this query parameter?Also, consider using a TypeScript file (
.ts) instead of a JavaScript file (.js) for type safety and consistency with the rest of the codebase.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
apps/workflows-dashboard/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (8)
- apps/workflows-dashboard/Dockerfile (1 hunks)
- apps/workflows-dashboard/entrypoint.sh (1 hunks)
- apps/workflows-dashboard/global.d.ts (1 hunks)
- apps/workflows-dashboard/index.html (1 hunks)
- apps/workflows-dashboard/package.json (1 hunks)
- apps/workflows-dashboard/public/config.js (1 hunks)
- apps/workflows-dashboard/src/lib/request/request.ts (1 hunks)
- apps/workflows-dashboard/src/main.tsx (2 hunks)
Additional context used
Shellcheck
apps/workflows-dashboard/entrypoint.sh
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 8-8: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 13-13: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
Additional comments not posted (10)
apps/workflows-dashboard/global.d.ts (1)
1-3: ** Consider usingconstfor global declarations.**Using
varfor the global declaration ofenvcan lead to hoisting issues and is generally less safe thanconst. Sinceenvis unlikely to be reassigned directly (its properties might change), usingconstwould be more appropriate.declare global { export const env: {[key: string]: any}; }apps/workflows-dashboard/src/lib/request/request.ts (1)
4-5: Handle types explicitly instead of using@ts-ignore.The use of
@ts-ignorecan hide potential type issues. It's better to handle these explicitly, either by defining the types or handling potential undefined cases.Consider this alternative:
// Assuming `env` and `VITE_API_URL` are always defined baseURL: globalThis.env.VITE_API_URL as string,If
globalThis.env.VITE_API_URLcan be undefined, handle it like this:baseURL: globalThis.env.VITE_API_URL ?? 'default_url',apps/workflows-dashboard/index.html (1)
9-9: LGTM!The inclusion of the
config.jsfile in the<head>section is a good practice for defining global configuration settings. This aligns with the PR objective of using runtime variables in the workflow dashboard.Note: The
type="text/javascript"attribute is optional as it is the default script type.apps/workflows-dashboard/src/main.tsx (1)
17-20: ** Refine TypeScript usage and handle potential null cases.**The suggestions from the previous review comment are still valid and applicable:
- Using
@ts-ignoreto bypass TypeScript checks is not ideal.- Consider handling the case where
document.getElementById('root')might return null.Here's a code snippet incorporating the suggestions:
const rootElement = document.getElementById('root') as HTMLElement; if (!rootElement) { console.error('Root element not found'); throw new Error('Root element not found'); } ReactDOM.createRoot(rootElement).render( <React.StrictMode> <RouterProvider router={router}></RouterProvider> </React.StrictMode>, ); // Remove @ts-ignore and ensure type safety globalThis.env = globalThis.env || { API_URL: import.meta.env.VITE_API_URL as string }apps/workflows-dashboard/Dockerfile (1)
22-35: LGTM!The changes to the Dockerfile enhance the production setup by:
- Setting a working directory for better filesystem organization.
- Copying the
entrypoint.shscript from the development stage to the production stage.- Making the script executable.
- Specifying the script as the container's entrypoint to perform any necessary setup or configuration before the main application starts.
The existing
CMDinstruction for running Nginx is retained, ensuring that Nginx runs in the foreground after theentrypoint.shscript is executed.apps/workflows-dashboard/entrypoint.sh (4)
3-6: ** Replace[[ ]]with[ ]for POSIX compatibility.**The past review comment is still valid. The script uses
[[ ]]for condition checks, which is not defined in POSIX sh. This can lead to script failures in strict POSIX environments.Replace
[[ ]]with[ ]to ensure compatibility:- if [[ -z "$VITE_DOMAIN" ]] + if [ -z "$VITE_DOMAIN" ]Tools
Shellcheck
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
8-11: ** Replace[[ ]]with[ ]for POSIX compatibility.**The past review comment is still valid. The script uses
[[ ]]for condition checks, which is not defined in POSIX sh. This can lead to script failures in strict POSIX environments.Replace
[[ ]]with[ ]to ensure compatibility:- if [[ -z "$MODE" ]] + if [ -z "$MODE" ]Tools
Shellcheck
[warning] 8-8: In POSIX sh, [[ ]] is undefined.
(SC3010)
13-16: ** Replace[[ ]]with[ ]for POSIX compatibility.**The past review comment is still valid. The script uses
[[ ]]for condition checks, which is not defined in POSIX sh. This can lead to script failures in strict POSIX environments.Replace
[[ ]]with[ ]to ensure compatibility:- if [[ -z "$VITE_IMAGE_LOGO_URL" ]] + if [ -z "$VITE_IMAGE_LOGO_URL" ]Tools
Shellcheck
[warning] 13-13: In POSIX sh, [[ ]] is undefined.
(SC3010)
19-22: ** Replace[[ ]]with[ ]for POSIX compatibility.**The past review comment is still valid. The script uses
[[ ]]for condition checks, which is not defined in POSIX sh. This can lead to script failures in strict POSIX environments.Replace
[[ ]]with[ ]to ensure compatibility:- if [[ -z "$VITE_ENVIRONMENT_NAME" ]] + if [ -z "$VITE_ENVIRONMENT_NAME" ]Tools
Shellcheck
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
apps/workflows-dashboard/package.json (1)
9-9: LGTM!The change to the build script is a good practice. Removing the
distdirectory before each build ensures a clean build process and prevents potential issues related to stale files from previous builds.
| cat << EOF > /usr/share/nginx/html/config.js | ||
| globalThis.env = { | ||
| VITE_API_URL: "http://$VITE_DOMAIN/api/v1/", | ||
| VITE_ENVIRONMENT_NAME: "$VITE_ENVIRONMENT_NAME", | ||
| MODE: "$MODE", | ||
| VITE_IMAGE_LOGO_URL: "$VITE_IMAGE_LOGO_URL", | ||
| } |
There was a problem hiding this comment.
Why do you need the file config.js if you are adding the same file content inline in here?
There was a problem hiding this comment.
i just say that you can delete config.js file
| cat << EOF > /usr/share/nginx/html/config.js | ||
| globalThis.env = { | ||
| VITE_API_URL: "http://$VITE_DOMAIN/api/v1/", | ||
| VITE_ENVIRONMENT_NAME: "$VITE_ENVIRONMENT_NAME", | ||
| MODE: "$MODE", | ||
| VITE_IMAGE_LOGO_URL: "$VITE_IMAGE_LOGO_URL", | ||
| } |
There was a problem hiding this comment.
i just say that you can delete config.js file
| @@ -4,6 +4,7 @@ import * as ReactDOM from 'react-dom/client'; | |||
| import { RouterProvider } from 'react-router-dom'; | |||
| import './index.css'; | |||
| import { initializeMonitoring } from '@/initialize-monitoring/initialize-monitoring'; | |||
| import '../public/config.js?url'; | |||
There was a problem hiding this comment.
if you add it in here it would take in place just if you build the app again
| @@ -6,6 +6,7 @@ | |||
| <link rel="icon" type="image/png" sizes="16x16" href="/public/favicon-16x16.png" /> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |||
| <title>Workflow Dashboard</title> | |||
| <script type="text/javascript" src="/config.js"></script> | |||
There was a problem hiding this comment.
the path should be relative im not sure its actually working
Summary by CodeRabbit
New Features
envvariable for configuration.entrypoint.shscript for environment variable setup.Enhancements
Configuration
config.jsto manage global configuration settings, includingVITE_API_URLand defaults for environment variables.