Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: backoffice run dist with environment variables #2643

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

pratapalakshmi
Copy link
Collaborator

@pratapalakshmi pratapalakshmi commented Aug 18, 2024

Summary by CodeRabbit

  • New Features

    • Introduced a global configuration object for environment variables, allowing easy access to application settings.
    • Added a new entrypoint script to initialize environment variables and generate a configuration file for the application.
    • Included a script tag in the HTML to load the configuration settings dynamically.
    • Enhanced the application with a new working directory and entry point for the production container.
  • Bug Fixes

    • Improved the method of accessing environment variables to ensure consistent behavior across different execution contexts.
  • Documentation

    • Updated global declarations to enhance accessibility of environment variables throughout the application.

Copy link

changeset-bot bot commented Aug 18, 2024

⚠️ No Changeset found

Latest commit: b8ec025

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Aug 18, 2024

Walkthrough

The recent changes enhance the production setup of the backoffice application by introducing a global env object for centralized access to environment variables. A new entrypoint.sh script is added to initialize these variables and generate a configuration file for the application. The Dockerfile is updated to incorporate this script, ensuring a seamless transition to production. Additionally, an external JavaScript configuration file is included in the HTML document to facilitate dynamic settings management.

Changes

Files Change Summary
apps/backoffice-v2/global.d.ts, apps/backoffice-v2/public/config.js, apps/backoffice-v2/src/common/env/env.ts, apps/backoffice-v2/src/main.tsx Introduced a global env object for centralized access to environment variables, shifting access from import.meta.env to globalThis.env. Enhanced modular configuration management.
apps/backoffice-v2/index.html Added a <script> tag to load an external configuration file, enhancing dynamic capabilities.
apps/backoffice-v2/Dockerfile, apps/backoffice-v2/entrypoint.sh Updated Dockerfile for improved production setup, added entrypoint.sh for dynamic configuration initialization based on environment variables.

Possibly related PRs

Suggested reviewers

  • Omri-Levy
  • alonp99
  • liorzam

Poem

In the garden where the code does bloom,
A global env dispels the gloom.
With scripts that dance and settings that sing,
Flexibility now is the wondrous thing!
A hop, a skip, through config's delight,
Our app shines brighter, oh what a sight! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a50225 and b8ec025.

📒 Files selected for processing (7)
  • apps/backoffice-v2/Dockerfile (1 hunks)
  • apps/backoffice-v2/entrypoint.sh (1 hunks)
  • apps/backoffice-v2/global.d.ts (1 hunks)
  • apps/backoffice-v2/index.html (1 hunks)
  • apps/backoffice-v2/public/config.js (1 hunks)
  • apps/backoffice-v2/src/common/env/env.ts (1 hunks)
  • apps/backoffice-v2/src/main.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • apps/backoffice-v2/global.d.ts
  • apps/backoffice-v2/src/main.tsx
  • apps/backoffice-v2/index.html
  • apps/backoffice-v2/Dockerfile
  • apps/backoffice-v2/public/config.js
  • apps/backoffice-v2/src/common/env/env.ts
🧰 Additional context used
🪛 Shellcheck (0.10.0)
apps/backoffice-v2/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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test_windows
  • GitHub Check: test_linux
  • GitHub Check: build (windows-latest)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: lint
  • GitHub Check: build (ubuntu-latest)
🔇 Additional comments (7)
apps/backoffice-v2/entrypoint.sh (7)

1-2: Update shebang to match script requirements.

The script uses bash-specific features but declares itself as a sh script. Either:

  1. Change shebang to #!/usr/bin/env bash to explicitly require bash, or
  2. Make the script POSIX-compliant by replacing [[ with [
-#!/usr/bin/env sh
+#!/usr/bin/env bash

8-11: Use a more secure method to set the API key.

Hardcoding the API key as secret is not recommended. Consider using a secure vault or secrets manager to store and retrieve the API key.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 8-8: In POSIX sh, [[ ]] is undefined.

(SC3010)


3-37: Add input validation for critical environment variables.

The script should validate the format and values of critical variables like VITE_DOMAIN and VITE_POLLING_INTERVAL.

🧰 Tools
🪛 Shellcheck (0.10.0)

[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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)


3-37: Use POSIX-compliant syntax for conditional checks.

Replace [[ ]] with [ ] for compatibility with POSIX sh.

🧰 Tools
🪛 Shellcheck (0.10.0)

[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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)


39-51: Add value sanitization for environment variables.

Environment variables are directly interpolated into the JavaScript file without sanitization. This could lead to script injection if the variables contain malicious content.


53-54: Validate and sanitize command input.

The exec "$@" command executes arbitrary commands without validation. Consider implementing command whitelisting for security.


1-54: File permissions need to be fixed for Docker entrypoint.

The script needs to be executable (755) for use as a Docker entrypoint. Please:

  • Run chmod +x apps/backoffice-v2/entrypoint.sh
  • Consider adding a .gitattributes file to maintain executable permissions
🧰 Tools
🪛 Shellcheck (0.10.0)

[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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
apps/backoffice-v2/global.d.ts (1)

1-3: Document the global env variable.

Consider adding comments to explain the purpose and usage of the global env variable. This will help other developers understand its role and how it should be used.

+// Global environment variable used for configuration settings.
declare global {
  export var env: { [key: string]: any };
}
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4df82a8 and 30bd6fa.

Files selected for processing (5)
  • apps/backoffice-v2/global.d.ts (1 hunks)
  • apps/backoffice-v2/index.html (1 hunks)
  • apps/backoffice-v2/public/config.js (1 hunks)
  • apps/backoffice-v2/src/common/env/env.ts (1 hunks)
  • apps/backoffice-v2/src/main.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
  • apps/backoffice-v2/public/config.js
Additional comments not posted (5)
apps/backoffice-v2/src/common/env/env.ts (2)

Line range hint 13-18:
LGTM! Ensure error handling is robust.

The code correctly handles invalid environment variables by logging an error and throwing an exception. Ensure that this approach aligns with the application's error handling strategy.


13-13: Verify the impact of using globalThis.env.

The switch from import.meta.env to globalThis.env may affect how environment variables are managed. Ensure that globalThis.env is correctly populated in all environments where this code runs.

apps/backoffice-v2/index.html (1)

11-11: Verify the loading and security of /config.js.

Ensure that /config.js is correctly loaded and does not introduce security vulnerabilities such as cross-site scripting (XSS). Consider using integrity checks or other security measures.

apps/backoffice-v2/src/main.tsx (2)

19-19: Verify the import statement with a query string.

The import statement includes a query string (?url). Ensure that this is intentional and supported by your build system or bundler.


55-58: Verify the necessity of //@ts-ignore and the impact of modifying globalThis.

Using //@ts-ignore suppresses TypeScript checks, which might hide potential issues. Ensure that this is necessary and verify the impact of modifying globalThis.env to avoid unintended side effects.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 30bd6fa and 1e3dbce.

Files selected for processing (1)
  • apps/backoffice-v2/public/config.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • apps/backoffice-v2/public/config.js

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1e3dbce and 878fcca.

Files selected for processing (1)
  • apps/backoffice-v2/public/config.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • apps/backoffice-v2/public/config.js

@pratapalakshmi pratapalakshmi changed the title chore: run dist with environment variables chore: backoffice run dist with environment variables Aug 18, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 878fcca and fa5242f.

Files selected for processing (2)
  • apps/backoffice-v2/Dockerfile (1 hunks)
  • apps/backoffice-v2/entrypoint.sh (1 hunks)
Additional context used
Shellcheck
apps/backoffice-v2/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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)

Additional comments not posted (6)
apps/backoffice-v2/Dockerfile (4)

25-26: Setting the working directory.

The WORKDIR /app directive is a good practice to ensure that all subsequent commands run within the /app directory.


29-30: Copying the entrypoint script.

The COPY command ensures that entrypoint.sh is available in the production image, which is essential for initializing the container.


33-34: Granting execution permissions.

The RUN chmod a+x /app/entrypoint.sh command is necessary to make the entrypoint script executable.


37-38: Setting the entrypoint.

The ENTRYPOINT [ "/app/entrypoint.sh" ] directive allows for customized container initialization, enhancing flexibility and control over the startup process.

apps/backoffice-v2/entrypoint.sh (2)

39-51: Dynamic configuration file creation.

The use of a heredoc to write environment variables to config.js is an effective way to generate dynamic configuration files.


53-54: Handling CMD command with exec.

Using exec "$@" ensures that the CMD command is executed properly, maintaining the intended behavior of the container.

apps/backoffice-v2/entrypoint.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between fa5242f and 6815de4.

Files selected for processing (1)
  • apps/backoffice-v2/entrypoint.sh (1 hunks)
Additional context used
Shellcheck
apps/backoffice-v2/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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)

Additional comments not posted (6)
apps/backoffice-v2/entrypoint.sh (6)

3-27: Also applies to: 29-37

Tools
Shellcheck

[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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


13-16: LGTM!

The logic for setting the default value of VITE_AUTH_ENABLED is correct.

Tools
Shellcheck

[warning] 13-13: In POSIX sh, [[ ]] is undefined.

(SC3010)


19-22: LGTM!

The logic for setting the default value of VITE_MOCK_SERVER is correct.

Tools
Shellcheck

[warning] 19-19: In POSIX sh, [[ ]] is undefined.

(SC3010)


24-27: LGTM!

The logic for setting the default value of VITE_POLLING_INTERVAL is correct.

Tools
Shellcheck

[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


39-51: LGTM!

The generated config.js file correctly sets up the global env object with the necessary environment variables.


53-54: LGTM!

Using exec "$@" provides flexibility to execute any command passed to the script.

apps/backoffice-v2/entrypoint.sh Show resolved Hide resolved
apps/backoffice-v2/public/config.js Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (8)
apps/backoffice-v2/global.d.ts (1)

1-3: Make the environment object readonly to prevent accidental modifications.

The environment configuration should be immutable to prevent runtime modifications that could lead to unexpected behavior.

declare global {
-  export var env: { [key: string]: any };
+  export var env: Readonly<EnvConfig>;
}
apps/backoffice-v2/public/config.js (1)

1-9: Consider adding environment variable validation

The current implementation silently falls back to default values. Consider adding validation to ensure required environment variables are properly set during initialization.

Add a validation function:

const validateEnv = (env) => {
  const required = ['VITE_API_URL', 'VITE_API_KEY'];
  const missing = required.filter(key => !env[key] || env[key].startsWith('<REQUIRED_'));
  
  if (missing.length > 0) {
    console.error(`Missing required environment variables: ${missing.join(', ')}`);
    throw new Error('Missing required environment variables');
  }
};

globalThis.env = {
  // ... your existing env config ...
};

validateEnv(globalThis.env);
apps/backoffice-v2/Dockerfile (2)

37-38: Consider using ENTRYPOINT and CMD in shell form for better signal handling.

The current exec form ([ "/app/entrypoint.sh" ]) might not properly handle signals. Consider using shell form to ensure proper signal propagation to the nginx process.

-ENTRYPOINT [ "/app/entrypoint.sh" ]
-CMD ["nginx", "-g", "daemon off;"]
+ENTRYPOINT /app/entrypoint.sh
+CMD nginx -g "daemon off;"

29-38: Consider security improvements for the entrypoint setup.

A few security-related suggestions:

  1. Consider using a non-root user for running nginx
  2. Add health checks to ensure the configuration is properly generated

Example improvements:

+# Add after line 33
+RUN adduser -D -H -s /sbin/nologin nginx-user
+USER nginx-user

+# Add before ENTRYPOINT
+HEALTHCHECK --interval=30s --timeout=3s \
+  CMD wget --quiet --tries=1 --spider http://localhost:80/ || exit 1
apps/backoffice-v2/src/common/env/env.ts (1)

Line range hint 13-24: Consider enhancing error handling for production environments.

The current error handling throws an error, which might not be ideal for production. Consider adding error reporting and graceful fallbacks.

 if (_env.success === false) {
   terminal.error('❌ Invalid environment variables:\n', ...formatErrors(_env.error.format()));
+  // Report error to monitoring service
+  if (process.env.NODE_ENV === 'production') {
+    // Log to external service
+    console.error('Environment initialization failed:', _env.error);
+    // Use fallback values where possible
+    return defaultProductionConfig;
+  }
   throw new Error('Invalid environment variables');
 }
apps/backoffice-v2/index.html (1)

11-11: Consider adding error handling for config.js loading

While the config.js import is necessary for rendering as confirmed in the previous discussion, we should ensure graceful handling of loading failures.

Consider adding error handling:

-    <script type="text/javascript" src="/config.js"></script>
+    <script type="text/javascript" src="/config.js" onerror="console.error('Failed to load configuration. Please refresh the page or contact support if the issue persists.');"></script>
apps/backoffice-v2/entrypoint.sh (1)

24-32: Add validation for polling interval values.

The polling intervals should be validated to ensure they're within reasonable bounds to prevent performance issues.

 if [[ -z "$VITE_POLLING_INTERVAL" ]]
 then
     VITE_POLLING_INTERVAL=10
+else
+    # Ensure minimum 5 seconds to prevent excessive polling
+    if [ "$VITE_POLLING_INTERVAL" -lt 5 ]; then
+        VITE_POLLING_INTERVAL=5
+    fi
 fi

 if [[ -z "$VITE_ASSIGNMENT_POLLING_INTERVAL" ]]
 then
     VITE_ASSIGNMENT_POLLING_INTERVAL=5
+else
+    # Ensure minimum 3 seconds to prevent excessive polling
+    if [ "$VITE_ASSIGNMENT_POLLING_INTERVAL" -lt 3 ]; then
+        VITE_ASSIGNMENT_POLLING_INTERVAL=3
+    fi
 fi
🧰 Tools
🪛 Shellcheck

[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)

apps/backoffice-v2/src/main.tsx (1)

Line range hint 1-61: Consider separating environment initialization

The current file handles multiple responsibilities including app initialization, environment setup, and React rendering. Consider extracting the environment initialization into a separate module (e.g., initializeEnvironment.ts) that runs before any other initialization.

This would:

  1. Improve separation of concerns
  2. Make environment validation more centralized
  3. Ensure environment is properly set up before any other code runs
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6815de4 and 39d369a.

📒 Files selected for processing (7)
  • apps/backoffice-v2/Dockerfile (1 hunks)
  • apps/backoffice-v2/entrypoint.sh (1 hunks)
  • apps/backoffice-v2/global.d.ts (1 hunks)
  • apps/backoffice-v2/index.html (1 hunks)
  • apps/backoffice-v2/public/config.js (1 hunks)
  • apps/backoffice-v2/src/common/env/env.ts (1 hunks)
  • apps/backoffice-v2/src/main.tsx (2 hunks)
🧰 Additional context used
🪛 Shellcheck
apps/backoffice-v2/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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)

🔇 Additional comments (6)
apps/backoffice-v2/Dockerfile (2)

25-26: LGTM: Working directory setup follows best practices.

The WORKDIR directive is appropriately placed before operations that need the working directory context.


29-30: Verify entrypoint.sh existence and content.

While the script copying and permission setting look correct, we should verify:

  1. The existence of entrypoint.sh in the source location
  2. The script's content for security considerations

Also applies to: 33-34

✅ Verification successful

The entrypoint.sh script has been verified and appears secure

The script exists and its content has been examined. It performs the following safe operations:

  • Sets default values for environment variables if not provided
  • Generates a runtime config.js with environment variables
  • Uses proper shell scripting practices with environment variable checks
  • Executes the passed command safely using exec "$@"

No security concerns were identified in the script's implementation. The COPY instruction in the Dockerfile is appropriate for this use case.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if entrypoint.sh exists and examine its content
fd -t f "entrypoint.sh" -x cat {}

Length of output: 1077

apps/backoffice-v2/src/common/env/env.ts (2)

13-13: Review security implications of global environment exposure.

Moving environment variables to globalThis.env makes them globally accessible. This could expose sensitive configuration data to client-side scripts and browser dev tools.

#!/bin/bash
# Check for potentially sensitive environment variables in schema
rg -A 5 "schema.*string.*" apps/backoffice-v2/src/common/env/schema.ts

# Look for any security-related comments or patterns
rg -i "secret|key|token|password|credential" apps/backoffice-v2/src/common/env/schema.ts

Consider:

  1. Splitting environment variables into public/private
  2. Using runtime API endpoints for sensitive configurations
  3. Implementing proper security headers (e.g., CSP) to restrict script access

13-13: ⚠️ Potential issue

Verify environment variable initialization timing.

The switch from import.meta.env to globalThis.env changes when environment variables become available - from build-time to runtime. This could lead to race conditions if the code executes before the config.js script loads.

Consider adding an initialization guard:

-const _env = EnvSchema.safeParse(globalThis.env);
+const getEnv = () => {
+  if (!globalThis.env) {
+    throw new Error('Environment variables not initialized. Ensure config.js is loaded.');
+  }
+  return EnvSchema.safeParse(globalThis.env);
+};
+const _env = getEnv();
apps/backoffice-v2/entrypoint.sh (1)

53-54: LGTM: Proper command execution handling.

The use of exec "$@" is the correct pattern for Docker entrypoint scripts, allowing proper signal handling and process management.

apps/backoffice-v2/src/main.tsx (1)

20-20: Verify config.js availability and loading mechanism

The import of config.js using Vite's URL import might cause issues in production if the file is not properly generated during the build process.

Let's verify the config file setup:

✅ Verification successful

Config.js import and setup is correctly implemented

The verification shows that:

  1. The config.js is properly included in index.html via a script tag before the main application code.
  2. The entrypoint.sh correctly generates the config.js file at runtime in /usr/share/nginx/html/config.js with all necessary environment variables.
  3. The import with ?url in main.tsx is correct as it ensures the file is included in the build and available at runtime.

The implementation follows a proper pattern where:

  • Environment variables are injected at container startup via entrypoint.sh
  • The config file is loaded before the application code
  • The Vite URL import ensures the file is included in the build
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify config.js existence and content in public directory
# Expected: Find config.js and check its content structure

# Check if config.js exists in public directory
fd -t f "config.js" -x cat {} \;

# Check if the HTML file properly includes config.js
fd -t f "index.html" -x grep -l "config.js" {} \;

Length of output: 11134


Script:

#!/bin/bash
# Let's check the actual content of the index.html and look for config.js references
cat ./apps/backoffice-v2/index.html

# Let's also check the actual config.js in the backoffice-v2 directory
fd -t f "config.js" apps/backoffice-v2 -x cat {} \;

# Let's check if there are any build scripts or Dockerfile that might generate config.js
rg -l "config.js" --type sh

Length of output: 2226


Script:

#!/bin/bash
# Let's check the entrypoint.sh to understand how config.js is generated
cat apps/backoffice-v2/entrypoint.sh

# Let's also check if there are any other files that might be involved in config.js generation
rg -l "config\.js" --type typescript --type javascript

Length of output: 1172

apps/backoffice-v2/global.d.ts Show resolved Hide resolved
apps/backoffice-v2/public/config.js Show resolved Hide resolved
apps/backoffice-v2/index.html Show resolved Hide resolved
apps/backoffice-v2/entrypoint.sh Show resolved Hide resolved
apps/backoffice-v2/entrypoint.sh Show resolved Hide resolved
apps/backoffice-v2/src/main.tsx Show resolved Hide resolved
@pratapalakshmi pratapalakshmi force-pushed the chore/run/time/dist/backoffice branch from 39d369a to f48d792 Compare December 8, 2024 09:23
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 39d369a and f48d792.

📒 Files selected for processing (7)
  • apps/backoffice-v2/Dockerfile (1 hunks)
  • apps/backoffice-v2/entrypoint.sh (1 hunks)
  • apps/backoffice-v2/global.d.ts (1 hunks)
  • apps/backoffice-v2/index.html (1 hunks)
  • apps/backoffice-v2/public/config.js (1 hunks)
  • apps/backoffice-v2/src/common/env/env.ts (1 hunks)
  • apps/backoffice-v2/src/main.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • apps/backoffice-v2/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • apps/backoffice-v2/src/main.tsx
  • apps/backoffice-v2/index.html
  • apps/backoffice-v2/src/common/env/env.ts
  • apps/backoffice-v2/public/config.js
  • apps/backoffice-v2/Dockerfile
🧰 Additional context used
🪛 Shellcheck (0.10.0)
apps/backoffice-v2/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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)

🔇 Additional comments (1)
apps/backoffice-v2/entrypoint.sh (1)

1-2: Update shebang to match script requirements.

The script uses bash-specific features but declares itself as a sh script.

apps/backoffice-v2/entrypoint.sh Show resolved Hide resolved
apps/backoffice-v2/entrypoint.sh Show resolved Hide resolved
@pratapalakshmi pratapalakshmi force-pushed the chore/run/time/dist/backoffice branch from f48d792 to 4a462db Compare January 20, 2025 11:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f48d792 and 4a462db.

📒 Files selected for processing (7)
  • apps/backoffice-v2/Dockerfile (1 hunks)
  • apps/backoffice-v2/entrypoint.sh (1 hunks)
  • apps/backoffice-v2/global.d.ts (1 hunks)
  • apps/backoffice-v2/index.html (1 hunks)
  • apps/backoffice-v2/public/config.js (1 hunks)
  • apps/backoffice-v2/src/common/env/env.ts (1 hunks)
  • apps/backoffice-v2/src/main.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • apps/backoffice-v2/global.d.ts
  • apps/backoffice-v2/index.html
  • apps/backoffice-v2/src/main.tsx
  • apps/backoffice-v2/Dockerfile
  • apps/backoffice-v2/src/common/env/env.ts
  • apps/backoffice-v2/public/config.js
🧰 Additional context used
🪛 Shellcheck (0.10.0)
apps/backoffice-v2/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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test_windows
  • GitHub Check: test_linux
  • GitHub Check: build (windows-latest)
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: lint
🔇 Additional comments (5)
apps/backoffice-v2/entrypoint.sh (5)

1-2: Update shebang to match script requirements.

The script uses bash-specific features but declares itself as a sh script. Either:

  1. Change shebang to #!/usr/bin/env bash to explicitly require bash, or
  2. Make the script POSIX-compliant by replacing [[ with [
-#!/usr/bin/env sh
+#!/usr/bin/env bash

8-11: Use a more secure method to set the API key.

Hardcoding the API key as secret is not recommended. Consider using a secure vault or secrets manager to store and retrieve the API key.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 8-8: In POSIX sh, [[ ]] is undefined.

(SC3010)


53-54: Validate and sanitize command input.

The exec "$@" command executes arbitrary commands without validation. Consider:

  1. Validating allowed commands
  2. Implementing command whitelisting

1-54: File permissions need to be fixed for Docker entrypoint.

The script integration has issues that need to be addressed:

  • The entrypoint script currently has permissions 644 (-rw-r--r--) but needs to be executable (755)
  • While the Dockerfile sets permissions with chmod a+x, the file in the repository should be executable to maintain consistency
🧰 Tools
🪛 Shellcheck (0.10.0)

[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)


[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 34-34: In POSIX sh, [[ ]] is undefined.

(SC3010)


24-32: 🛠️ Refactor suggestion

Add validation for polling intervals.

The polling intervals should have reasonable bounds to prevent performance issues:

  • VITE_POLLING_INTERVAL: Currently defaults to 10 seconds
  • VITE_ASSIGNMENT_POLLING_INTERVAL: Currently defaults to 5 seconds

Add validation to ensure these values are within acceptable ranges (e.g., between 5-3600 seconds).

 if [[ -z "$VITE_POLLING_INTERVAL" ]]
 then
     VITE_POLLING_INTERVAL=10
+else
+    if ! [[ "$VITE_POLLING_INTERVAL" =~ ^[0-9]+$ ]] || \
+       [ "$VITE_POLLING_INTERVAL" -lt 5 ] || \
+       [ "$VITE_POLLING_INTERVAL" -gt 3600 ]; then
+        echo "Error: VITE_POLLING_INTERVAL must be between 5 and 3600 seconds"
+        exit 1
+    fi
 fi

 if [[ -z "$VITE_ASSIGNMENT_POLLING_INTERVAL" ]]
 then
     VITE_ASSIGNMENT_POLLING_INTERVAL=5
+else
+    if ! [[ "$VITE_ASSIGNMENT_POLLING_INTERVAL" =~ ^[0-9]+$ ]] || \
+       [ "$VITE_ASSIGNMENT_POLLING_INTERVAL" -lt 5 ] || \
+       [ "$VITE_ASSIGNMENT_POLLING_INTERVAL" -gt 3600 ]; then
+        echo "Error: VITE_ASSIGNMENT_POLLING_INTERVAL must be between 5 and 3600 seconds"
+        exit 1
+    fi
 fi

Likely invalid or redundant comment.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 24-24: In POSIX sh, [[ ]] is undefined.

(SC3010)


[warning] 29-29: In POSIX sh, [[ ]] is undefined.

(SC3010)

apps/backoffice-v2/entrypoint.sh Show resolved Hide resolved
@pratapalakshmi pratapalakshmi enabled auto-merge (squash) January 28, 2025 13:05
@pratapalakshmi pratapalakshmi force-pushed the chore/run/time/dist/backoffice branch from 3a50225 to b8ec025 Compare January 28, 2025 13:18
@pratapalakshmi pratapalakshmi merged commit 67fe89a into dev Jan 28, 2025
18 checks passed
@pratapalakshmi pratapalakshmi deleted the chore/run/time/dist/backoffice branch January 28, 2025 13:27
alonp99 added a commit that referenced this pull request Jan 28, 2025
@coderabbitai coderabbitai bot mentioned this pull request Jan 28, 2025
alonp99 added a commit that referenced this pull request Feb 11, 2025
* chore: run dist with environment variables (#2643)

* fix: adding trimmer in end of env name

---------

Co-authored-by: ChiragSharma <[email protected]>
Co-authored-by: Alon Peretz <[email protected]>
chesterkmr added a commit that referenced this pull request Feb 14, 2025
* Dev 336/amplify webhook builds (#2906)

* feat: added new actions to build front-end apps manually

* fix: fixed a typo

* fix: changed the action names for the new front-end build actions

* feat: added on-failuew job to notify when the job fails

* fix: added changes code rabbit

* fix: removed redundant step for build action

* chore: removing on push trigger on the actions

* [bal-3442] - Backoffice - Display timestamp in local timezone for transaction monitoring (#3037)

* fix(transaction.mapper): remove unnecessary success log on validation (#3023)

- Eliminate console.log for successful validation
- Clean up the code for improved readability

(your logs are so verbose, they could be mistaken for a motivational speech)

* feat(docs): add development rules for backoffice-v2, kyb-app, workflows-dashboard (#3038)

* feat(docs): add development rules for backoffice-v2, kyb-app, workflows-dashboard

- Introduce best practices for React TypeScript development
- Include guidance on components, hooks, state management, and testing

(These rules are so thorough, they could double as a user manual for a space shuttle)

* docs(workflows): enhance workflow service rules and guidelines

- Expand code organization and structure guidelines
- Introduce TypeScript usage best practices
- Include detailed documentation on error handling and API design

(Your code organization is so messy, it makes a cluttered desk look like a minimalist art piece)

* chore: run dist with environment variables (#2643) (#3041)

* chore: run dist with environment variables (#2643)

* fix: adding trimmer in end of env name

---------

Co-authored-by: ChiragSharma <[email protected]>
Co-authored-by: Alon Peretz <[email protected]>

* refactor(EditableDetails): optimize form handling with useMemo (#3048)

- Replace direct defaultValues assignment with memoized formValues
- Remove unnecessary useEffect for form reset on data change
- Simplify conditional rendering logic in the return statement

Co-authored-by: Alon Peretz <[email protected]>

* refactor(components): streamline details component properties access (#3049)

- Remove optional chaining for value.id and value.title
- Improve readability by ensuring direct property access

refactor(hooks): add workflowId to bankAccount and credit check blocks

- Update hooks to receive workflowId for better context handling
- Simplify conditional checks for plugin outputs

chore(migrations): update subproject commit reference

- Sync submodule to latest commit in workflows-service

* Use `report_outcomes` for report display (#3035)

* fix: table page

* wip

* fix: formating

* wip

* fix: small fixes

* fix

* fix: ui bug

* build: new version

* fix: validate report metadata

* fix: pr comments

* fix: pnpm-lock.json

* fix: update package

* fix: update package

* fix: update package

* fix: update package

* fix: update package

* fix: update package

* feat(bal-3616): added demo account config option (#3046)

* feat(bal-3616): added demo account config option

* chore: pr review fixes

* Illiar/feat/common document schemas (#3051)

* feat: added pci-certification document

* feat: added financial document && bump

* fix: lock fix

* fix: fixed backoffice crash & ui bump (#3053)

* fix: fixed incorect reason of revision (#3054)

---------

Co-authored-by: Chirag <[email protected]>
Co-authored-by: liorzam <[email protected]>
Co-authored-by: Alon Peretz <[email protected]>
Co-authored-by: pratapalakshmi <[email protected]>
Co-authored-by: ChiragSharma <[email protected]>
Co-authored-by: Shane <[email protected]>
Co-authored-by: Tomer Shvadron <[email protected]>
Co-authored-by: Matan Yadaev <[email protected]>
Co-authored-by: Artem Lavrentii <[email protected]>
Co-authored-by: Illia Rudniev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants