Eslint fixes, dotenv and growthbook #95
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve the addition of environment variables for two external APIs in the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Config
participant Sentry
App->>Config: Initialize configuration
Config->>App: Load environment variables
App->>Sentry: Initialize error tracking
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 as PR comments)
Additionally, you can add 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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (8)
- .env (1 hunks)
- .eslintrc.js (4 hunks)
- app/config/index.dev.js (1 hunks)
- app/config/index.js (1 hunks)
- app/config/index.production.js (1 hunks)
- app/utils/growthbook.js (1 hunks)
- babel.config.js (1 hunks)
- package.json (2 hunks)
Files skipped from review due to trivial changes (1)
- .env
Additional comments not posted (9)
app/config/index.js (1)
1-4: LGTM! Verify the environment variable usage.The change to use the
SIMPSONS_APIenvironment variable forAPI_URLenhances configuration flexibility.Ensure that the
SIMPSONS_APIvariable is correctly set in the environment and used throughout the codebase.Run the following script to verify the usage of the
SIMPSONS_APIvariable:Verification successful
Environment Variable Usage Verified
The
SIMPSONS_APIenvironment variable is correctly set and utilized in the codebase throughConfig.API_URL, specifically in theapp/utils/apiUtils.jsfile for API client creation.
app/utils/apiUtils.js: UtilizesConfig.API_URLincreateApiClientWithTransForm.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `SIMPSONS_API` environment variable. # Test: Search for the usage of `SIMPSONS_API`. Expect: Occurrences in configuration and usage files. rg --type js 'SIMPSONS_API'Length of output: 128
Script:
#!/bin/bash # Description: Verify the usage of the `Config.API_URL` to ensure `SIMPSONS_API` is utilized in the codebase. # Test: Search for the usage of `Config.API_URL`. Expect: Occurrences in files where API calls are made. rg --type js 'Config\.API_URL'Length of output: 199
app/config/index.dev.js (1)
1-4: LGTM! Verify the environment variable usage.The change to use the
JSON_PLACEHOLDER_APIenvironment variable forAPI_URLenhances configuration flexibility.Ensure that the
JSON_PLACEHOLDER_APIvariable is correctly set in the environment and used throughout the codebase.Run the following script to verify the usage of the
JSON_PLACEHOLDER_APIvariable:Verification successful
Environment Variable Usage Verified
The
JSON_PLACEHOLDER_APIenvironment variable is correctly set in the.envfile and is used in both development and production configuration files. No further issues are found.
.env:JSON_PLACEHOLDER_API="https://jsonplaceholder.typicode.com"app/config/index.dev.js: UtilizesJSON_PLACEHOLDER_API.app/config/index.production.js: UtilizesJSON_PLACEHOLDER_API.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `JSON_PLACEHOLDER_API` environment variable. # Test: Search for the usage of `JSON_PLACEHOLDER_API`. Expect: Occurrences in configuration and usage files. rg --type js 'JSON_PLACEHOLDER_API'Length of output: 323
Script:
#!/bin/bash # Description: Search for the definition or assignment of `JSON_PLACEHOLDER_API` in environment configuration files. # Test: Look for occurrences of `JSON_PLACEHOLDER_API` in common environment configuration files. rg 'JSON_PLACEHOLDER_API' --glob '*.env' --glob '*.sh' --glob '*.bash' --glob '*.zsh'Length of output: 150
app/config/index.production.js (1)
1-4: LGTM! Verify the environment variable usage.The change to use the
JSON_PLACEHOLDER_APIenvironment variable forAPI_URLenhances configuration flexibility.Ensure that the
JSON_PLACEHOLDER_APIvariable is correctly set in the environment and used throughout the codebase.Run the following script to verify the usage of the
JSON_PLACEHOLDER_APIvariable:babel.config.js (2)
36-45: Verify handling of undefined environment variables.The configuration allows undefined environment variables (
allowUndefined: true). Ensure that your codebase handles cases where these variables might be undefined to prevent potential runtime errors.
36-45: LGTM!The addition of the
react-native-dotenvplugin is correctly configured for managing environment variables.app/utils/growthbook.js (1)
1-64: LGTM!The utility functions for GrowthBook integration are well-structured and make good use of environment variables.
.eslintrc.js (1)
122-122: Stricter complexity rule applied.The complexity threshold has been reduced from 10 to 2, which will enforce simpler code structures. Be prepared to refactor existing code to comply with this stricter rule.
package.json (2)
41-41: Verify the integration of@growthbook/growthbook.The addition of
@growthbook/growthbookaligns with the PR objectives for feature flagging and experimentation.Ensure that the package is correctly configured and used within the project to leverage its capabilities effectively.
141-141: Verify the integration ofreact-native-dotenv.The addition of
react-native-dotenvaligns with the PR objectives for environment variable management.Ensure that the package is correctly configured and used within the project to manage environment variables effectively.
| export async function getGrowthBookFeaturesData(name, email) { | ||
| try { | ||
| const growthBook = getGrowthBookClient(email); | ||
| await growthBook.loadFeatures(); | ||
| return growthBook.getFeatureValue(name); | ||
| } catch (error) { | ||
| return new Error(error); | ||
| } |
There was a problem hiding this comment.
Improve error handling in getGrowthBookFeaturesData.
Instead of returning an Error object, consider throwing the error to maintain consistency in error handling practices.
Apply this diff to improve error handling:
- return new Error(error);
+ throw error;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getGrowthBookFeaturesData(name, email) { | |
| try { | |
| const growthBook = getGrowthBookClient(email); | |
| await growthBook.loadFeatures(); | |
| return growthBook.getFeatureValue(name); | |
| } catch (error) { | |
| return new Error(error); | |
| } | |
| export async function getGrowthBookFeaturesData(name, email) { | |
| try { | |
| const growthBook = getGrowthBookClient(email); | |
| await growthBook.loadFeatures(); | |
| return growthBook.getFeatureValue(name); | |
| } catch (error) { | |
| throw error; | |
| } |
There was a problem hiding this comment.
can you please check this once
There was a problem hiding this comment.
@shamoilattaar-wednesday, I've reviewed the suggestion to throw the error instead of returning a new Error object in the getGrowthBookFeaturesData function. This change aligns with standard error handling practices in JavaScript, allowing errors to propagate up the call stack for proper handling. It looks like a valid improvement. Let me know if you need further assistance!
| export async function getGrowthBookFeatureFlag(name, email) { | ||
| try { | ||
| const growthBook = getGrowthBookClient(email); | ||
| await growthBook.loadFeatures(); | ||
| return growthBook.isOn(name); | ||
| } catch (error) { | ||
| return new Error(error); | ||
| } |
There was a problem hiding this comment.
Improve error handling in getGrowthBookFeatureFlag.
Instead of returning an Error object, consider throwing the error to maintain consistency in error handling practices.
Apply this diff to improve error handling:
- return new Error(error);
+ throw error;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getGrowthBookFeatureFlag(name, email) { | |
| try { | |
| const growthBook = getGrowthBookClient(email); | |
| await growthBook.loadFeatures(); | |
| return growthBook.isOn(name); | |
| } catch (error) { | |
| return new Error(error); | |
| } | |
| export async function getGrowthBookFeatureFlag(name, email) { | |
| try { | |
| const growthBook = getGrowthBookClient(email); | |
| await growthBook.loadFeatures(); | |
| return growthBook.isOn(name); | |
| } catch (error) { | |
| throw error; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (1)
- package.json (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- .gitignore (1 hunks)
- App.js (1 hunks)
- android/app/build.gradle (2 hunks)
- android/sentry.properties (1 hunks)
- app.json (1 hunks)
- ios/reactnativetemplatews.xcodeproj/project.pbxproj (3 hunks)
- ios/sentry.properties (1 hunks)
- metro.config.js (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments not posted (8)
metro.config.js (2)
2-2: LGTM!The import statement correctly imports the required function from the specified package.
The code changes are approved.
5-5: LGTM!The export statement correctly uses the new function to configure the Metro bundler.
The code changes are approved.
App.js (2)
7-7: LGTM!The import statement for Sentry is correct.
The code changes are approved.
Line range hint
14-23: LGTM!The control flow is correct and ensures that the root component is registered correctly.
The code changes are approved.
app.json (1)
25-35: LGTM!The integration of the
@sentry/react-native/expoplugin is correct and aligns with the Sentry documentation for React Native with Expo.The code changes are approved.
android/app/build.gradle (1)
80-80: LGTM!The integration of the Sentry Gradle script is correct and aligns with the Sentry documentation for React Native with Android.
The code changes are approved.
ios/reactnativetemplatews.xcodeproj/project.pbxproj (2)
223-223: Verify the correctness of the script paths and integration.Ensure that the paths to the Sentry upload script and the React Native Xcode script are correct and that the integration does not introduce any issues.
Run the following script to verify the existence of the scripts:
310-323: Verify the correctness of the script path and integration.Ensure that the path to the Sentry upload script is correct and that the integration does not introduce any issues.
Run the following script to verify the existence of the Sentry upload script:
|
|
||
| auth.token=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 | ||
|
|
||
| defaults.org=wednesday-solutions-5p | ||
| defaults.project=react-native | ||
|
|
||
| defaults.url=https://sentry.io/ |
There was a problem hiding this comment.
Avoid hardcoding sensitive information.
The auth token is hardcoded, which may pose a security risk. Consider using environment variables to manage sensitive information securely.
Apply this diff to replace the hardcoded auth token with an environment variable:
-auth.token=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
+auth.token=${SENTRY_AUTH_TOKEN}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auth.token=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 | |
| defaults.org=wednesday-solutions-5p | |
| defaults.project=react-native | |
| defaults.url=https://sentry.io/ | |
| auth.token=${SENTRY_AUTH_TOKEN} | |
| defaults.org=wednesday-solutions-5p | |
| defaults.project=react-native | |
| defaults.url=https://sentry.io/ |
|
|
||
| auth.token=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 | ||
|
|
||
| defaults.org=wednesday-solutions-5p | ||
| defaults.project=react-native | ||
|
|
||
| defaults.url=https://sentry.io/ |
There was a problem hiding this comment.
Avoid hardcoding sensitive information.
The auth token is hardcoded, which may pose a security risk. Consider using environment variables to manage sensitive information securely.
Apply this diff to replace the hardcoded auth token with an environment variable:
-auth.token=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
+auth.token=${SENTRY_AUTH_TOKEN}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auth.token=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 | |
| defaults.org=wednesday-solutions-5p | |
| defaults.project=react-native | |
| defaults.url=https://sentry.io/ | |
| auth.token=${SENTRY_AUTH_TOKEN} | |
| defaults.org=wednesday-solutions-5p | |
| defaults.project=react-native | |
| defaults.url=https://sentry.io/ |
App.js
Outdated
| Sentry.init({ | ||
| dsn: | ||
| 'https://0fbf25b4bfb443b7ae58aa4baf34460e@o4505374929584128.ingest.us.sentry.io/4505374931812352' | ||
| }); |
There was a problem hiding this comment.
Use environment variables for DSN.
Hardcoding the DSN is not a best practice. It should be stored in environment variables for better security and maintainability.
Apply this diff to use environment variables for the DSN:
-Sentry.init({
- dsn:
- 'https://0fbf25b4bfb443b7ae58aa4baf34460e@o4505374929584128.ingest.us.sentry.io/4505374931812352'
-});
+import { SENTRY_DSN } from '@env';
+
+Sentry.init({
+ dsn: SENTRY_DSN
+});Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Sentry.init({ | |
| dsn: | |
| 'https://0fbf25b4bfb443b7ae58aa4baf34460e@o4505374929584128.ingest.us.sentry.io/4505374931812352' | |
| }); | |
| import { SENTRY_DSN } from '@env'; | |
| Sentry.init({ | |
| dsn: SENTRY_DSN | |
| }); |
Ticket Link
Related Links
Description
Steps to Reproduce / Test
GIF's
Summary by CodeRabbit