feat: Added props for controlling tracing #1356
Conversation
plxity
requested review from
himanshu-dixit and
utkarsh-dixit
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
js/src/sdk/models/actions.ts
Outdated
| ...(parsedData.requestBody.allowTracing | ||
| ? { | ||
| sessionId: | ||
| parsedData.requestBody?.sessionInfo?.sessionId || | ||
| ComposioSDKContext.sessionId, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
Potential runtime error if parsedData.requestBody.allowTracing is accessed when requestBody is undefined, since the optional chaining is missing on the first property access
📝 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.
| ...(parsedData.requestBody.allowTracing | |
| ? { | |
| sessionId: | |
| parsedData.requestBody?.sessionInfo?.sessionId || | |
| ComposioSDKContext.sessionId, | |
| } | |
| : {}), | |
| ...(parsedData.requestBody?.allowTracing | |
| ? { | |
| sessionId: | |
| parsedData.requestBody?.sessionInfo?.sessionId || | |
| ComposioSDKContext.sessionId, | |
| } | |
| : {}), | |
|
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: |
![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 8d57da6 in 1 minute and 6 seconds
More details
- Looked at
33lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/sdk/models/actions.ts:157
- Draft comment:
Conditional sessionInfo block: Add a comment explaining the rationale for including sessionId only if allowTracing is true. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. js/src/sdk/types/action.ts:45
- Draft comment:
Document allowTracing: Add a short description to clarify the behavior of allowTracing in ZExecuteParams. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. js/src/sdk/models/actions.ts:157
- Draft comment:
Ensure conditional spread with allowTracing is intended: now sessionId is added only if allowTracing is true, changing original always-included behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. js/src/sdk/types/action.ts:45
- Draft comment:
Added allowTracing prop with default false. Confirm documentation reflects that tracing (sessionId inclusion) now requires allowTracing to be true. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm that the documentation reflects a change in behavior due to a new prop. This is indirectly asking the author to ensure the change is documented, which is not allowed by the rules.
Workflow ID: wflow_K5eQruYAOJtkpo8q
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
js/src/sdk/models/actions.ts
Outdated
| sessionId: | ||
| parsedData.requestBody?.sessionInfo?.sessionId || | ||
| ComposioSDKContext.sessionId, | ||
| ...(parsedData.requestBody.allowTracing |
There was a problem hiding this comment.
Consider adding type safety check for allowTracing access. The current code directly accesses parsedData.requestBody.allowTracing which could be undefined. Suggested fix:
const shouldTrace = parsedData.requestBody?.allowTracing ?? false;
...(shouldTrace ? { ... } : {})
js/src/sdk/types/action.ts
Outdated
| @@ -42,6 +42,7 @@ export const ZExecuteParams = z.object({ | |||
| appName: z.string().optional(), | |||
| text: z.string().optional(), | |||
| authConfig: ZCustomAuthParams.optional(), | |||
| allowTracing: z.boolean().optional().default(false), | |||
There was a problem hiding this comment.
Consider adding JSDoc documentation for the new allowTracing parameter to explain its purpose and default behavior:
/**
* @property {boolean} allowTracing - Controls whether session tracking is enabled for this action execution.
* When false, no session information will be included in the request.
* @default false
*/
allowTracing: z.boolean().optional().default(false),
Code Review SummaryChanges Overview
Positive Aspects✅ Privacy-first approach Suggestions
AssessmentWell-structured changes with good privacy considerations. Implementation is clean but needs documentation and type safety improvements. Rating: 8/10 - Good implementation with minor improvements needed. |
| @@ -42,6 +42,7 @@ export const ZExecuteParams = z.object({ | |||
| appName: z.string().optional(), | |||
| text: z.string().optional(), | |||
| authConfig: ZCustomAuthParams.optional(), | |||
| allowTracing: z.boolean().optional(), | |||
There was a problem hiding this comment.
Removing .default(false) makes allowTracing undefined when not provided, which could break code expecting a boolean. Should keep default value for backwards compatibility.
📝 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.
| allowTracing: z.boolean().optional(), | |
| allowTracing: z.boolean().default(false).optional(), |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 41d345c in 1 minute and 40 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. js/src/sdk/models/actions.ts:159
- Draft comment:
Good use of optional chaining to safely access allowTracing property. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. js/src/sdk/types/action.ts:45
- Draft comment:
Removal of default value for allowTracing may affect clients expecting false by default. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
This is a tricky case. On one hand, changing schema defaults could affect behavior. However, this is a TypeScript type definition file using Zod, and the default was optional to begin with. Without seeing the actual runtime code that uses this schema or knowing how clients interact with it, we can't be certain this is problematic. The schema still allows undefined/optional values.
I might be underestimating the impact of removing defaults in Zod schemas - they do affect runtime validation. Also, existing code might rely on the false default.
While Zod defaults do matter, the field was already optional. If code required this value to be false, it should explicitly set it rather than relying on schema defaults.
Without strong evidence that this default value is critical, and given that the field remains optional, we should err on the side of removing the comment.
3. js/src/sdk/models/actions.ts:159
- Draft comment:
Optional chaining was added for 'allowTracing'. Verify if 'requestBody' can ever be undefined per the schema, since it is required. Consistency between schema and usage is key. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. js/src/sdk/types/action.ts:45
- Draft comment:
Removal of .default(false) on 'allowTracing' changes its default behavior. Ensure that treating undefined as false in the execute() logic is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
This is a type definition change that could affect runtime behavior. However, the comment is asking to "ensure" the behavior is intended, which violates our rules about not asking for confirmation. Additionally, without seeing the execute() logic implementation, we can't be certain this is actually problematic. The author may have intentionally changed this to handle undefined differently.
The change could cause runtime issues if the execute() logic expects allowTracing to always be a boolean. The comment identifies a real potential issue.
While the concern is valid, the comment violates our rules by asking for confirmation rather than stating a clear issue. We also lack context about the execute() implementation to know if this is actually problematic.
Delete the comment because it asks for confirmation rather than stating a clear issue, and we lack context to confirm there's actually a problem.
5. js/src/sdk/types/action.ts:12
- Draft comment:
Typo observation: The property 'usecaseLimit' uses a different casing compared to 'useCase' used elsewhere (e.g., line 11 and 56). Consider renaming 'usecaseLimit' to 'useCaseLimit' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_lbbLxtW3PbDILeOR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -1,20 +1,24 @@ | |||
| export const env = process.env.TEST_ENV || "staging" | |||
| export const env = "staging"; | |||
There was a problem hiding this comment.
Hardcoding env = "staging" removes environment flexibility. Should keep original process.env.TEST_ENV || "staging" to allow environment override.
📝 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.
| export const env = "staging"; | |
| export const env = process.env.TEST_ENV || "staging"; |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 87b6c55 in 1 minute and 18 seconds
More details
- Looked at
44lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. js/config/getTestConfig.ts:1
- Draft comment:
Using a fixed env variable ("staging") removes flexibility. Confirm if this is intended for tests. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This comment violates several rules: 1) It asks for confirmation of intention which is explicitly forbidden 2) It's speculative about flexibility without showing a concrete issue 3) This is a test config file, so hardcoding to staging may be intentional 4) Without more context about test requirements, we can't be sure this is even a problem.
The comment does point out a real change in behavior - removing environment variable configuration could impact other developers.
While the behavior change is real, asking for confirmation is not actionable. If there's a concrete reason why multiple environments are needed for tests, that should be stated directly.
Delete the comment. It asks for confirmation rather than making a concrete suggestion, and we don't have enough context to know if multiple test environments are actually needed.
2. js/config/getTestConfig.ts:15
- Draft comment:
Clarify the need to JSON.stringify followed by JSON.parse on require(path). It seems redundant. - Reason this comment was not posted:
Comment was on unchanged code.
3. js/config/getTestConfig.ts:1
- Draft comment:
Hardcoding env to 'staging' removes flexibility. Reconsider using process.env.TEST_ENV if external override is needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a test configuration file. The change appears deliberate - moving from configurable to fixed staging environment. Without broader context of why this change was made, we can't assume it's wrong. The author might have intentionally standardized test environment to staging for consistency or simplification.
The comment raises a valid point about flexibility. What if tests need to run against different environments?
While flexibility can be good, test configurations often benefit from being deterministic and consistent. The PR author likely had a reason for standardizing on staging.
Without strong evidence that this change is incorrect or understanding the broader context, we should trust the author's decision to standardize the test environment.
4. js/config/getTestConfig.ts:15
- Draft comment:
The JSON.stringify+JSON.parse round-trip is unnecessary. Use require(path) directly with a type assertion. - Reason this comment was not posted:
Comment was on unchanged code.
5. js/config/getTestConfig.ts:21
- Draft comment:
There is an extra space in the error message after 'You'. - Reason this comment was not posted:
Comment was on unchanged code.
6. js/config/getTestConfig.ts:21
- Draft comment:
Typo: There is an extra space between 'You' and 'can' in the error message on line 21. Please reduce it to a single space for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_csfqrKaLE8lvD5in
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Adds
allowTracingto control session tracing inexecute()and hardcodesenvto "staging" ingetTestConfig.ts.allowTracingboolean toZExecuteParamsinaction.tsto control session tracing.execute()inactions.tsto conditionally includesessionIdbased onallowTracing.envto "staging" ingetTestConfig.ts.This description was created by
for 87b6c55. It will automatically update as commits are pushed.