feat: change TEST_ENVIRONMENT -> TEST_ENV and log base url for CI #1198
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to afa6020 in 40 seconds
More details
- Looked at
60lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/run_js_test.yml:65
- Draft comment:
Use??instead of||for default values in GitHub Actions.
echo "Running test with environment: ${{ inputs.environment ?? 'prod' }}"
- Reason this comment was not posted:
Marked as duplicate.
2. .github/workflows/run_js_test.yml:68
- Draft comment:
Use??instead of||for default values in GitHub Actions.
run: cd js && TEST_ENV=${{ inputs.environment ?? 'prod' }} pnpm test:coverage --max-workers 16
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The??operator is the nullish coalescing operator that only falls back on null/undefined, while||falls back on any falsy value. In GitHub Actions, when an input is not provided, it evaluates to undefined, not an empty string. Since the input has a default value of 'prod' defined in the workflow, it should never actually be undefined. The distinction between??and||is not meaningful in this context.
The comment might be technically correct as??is generally preferred for default values in JavaScript. The change could still work with either operator.
While??might be marginally better, the input already has a default value defined in the workflow, making this suggestion unnecessary. The current code works correctly either way.
Delete the comment as it suggests a change that would have no practical impact, given that the input already has a default value defined in the workflow configuration.
Workflow ID: wflow_UOzyuQu1CQKStyHR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e9fb1dc in 26 seconds
More details
- Looked at
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/index.ts:21
- Draft comment:
Duplicate import ofIS_DEVELOPMENT_OR_CI. Remove the redundant import statement. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_H54Doe5gOm6Vrk2Q
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
js/src/sdk/index.ts
Outdated
|
|
||
|
|
||
| if (IS_DEVELOPMENT_OR_CI) { | ||
| logger.info( | ||
| `Initializing Composio w API Key: [REDACTED] and baseURL: ${baseURLParsed}` |
There was a problem hiding this comment.
The logger level should be consistent; using 'debug' instead of 'info' for development environments can help avoid unnecessary log clutter.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (IS_DEVELOPMENT_OR_CI) { | |
| logger.info( | |
| `Initializing Composio w API Key: [REDACTED] and baseURL: ${baseURLParsed}` | |
| if (IS_DEVELOPMENT_OR_CI) { | |
| logger.debug( | |
| `Initializing Composio w API Key: [REDACTED] and baseURL: ${baseURLParsed}` | |
| ); | |
| } |
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
| @@ -62,6 +63,11 @@ export class Composio { | |||
| config?.apiKey | |||
| ); | |||
|
|
|||
| if (IS_DEVELOPMENT_OR_CI) { | |||
| logger.info( | |||
| `Initializing Composio w API Key: [REDACTED] and baseURL: ${baseURLParsed}` | |||
There was a problem hiding this comment.
Consider using a more structured logging format for initialization. Instead of string interpolation, use an object structure:
logger.info('Composio SDK Initialization', {
baseURL: baseURLParsed,
apiKeyPresent: !!apiKeyParsed,
environment: process.env.NODE_ENV
});This would make log parsing and analysis easier in production environments.
| @@ -18,6 +18,7 @@ import { Triggers } from "./models/triggers"; | |||
| import { ZAuthMode } from "./types/integration"; | |||
| import ComposioSDKContext from "./utils/composioContext"; | |||
| import { getSDKConfig } from "./utils/config"; | |||
| import { IS_DEVELOPMENT_OR_CI } from "./utils/constants"; | |||
There was a problem hiding this comment.
The import of IS_DEVELOPMENT_OR_CI has been moved to a better location at the top with other imports, which improves code organization. However, consider grouping related imports together (e.g., all utility imports, all type imports, etc.) for better code maintainability.
Code Review SummaryOverall, the changes look good with some minor suggestions for improvement. Here's a breakdown: Positive Aspects:✅ Improved logging system with proper redaction of sensitive information Suggestions for Improvement:
Security & Best Practices:✅ Proper handling of sensitive data in logs The changes are well-structured and maintain good coding practices. The suggestions above are minor and the PR can be merged after addressing them. Rating: 8/10 - Good quality changes with minor improvements suggested. |
himanshu-dixit
changed the title
console.logwithlogger.debuginsendProcessReq()inexternal.tsfor better logging control.Composioconstructor inindex.tsto log API key and baseURL whenIS_DEVELOPMENT_OR_CIis true.inputs.environmentto 'prod' inrun_js_test.ymlif not provided.TEST_ENVIRONMENTtoTEST_ENVinrun_js_test.yml.This description was created by
<https://www.ellipsis.dev?ref=ComposioHQ%2Fcomposio&utm_source=github&utm_medium=referral%7Chttps://img.shields.io/badge/Ellipsis-blue?color=175173%7CEllipsis><sup> for e9fb1dc. It will automatically update as commits are pushed.