-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat: Add session information #1343
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 42e2436 in 45 seconds
More details
- Looked at
55
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. js/src/sdk/models/actions.ts:155
- Draft comment:
Consider validating that ComposioSDKContext.sessionId is defined before fallback assignment. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. js/src/sdk/types/action.ts:45
- Draft comment:
The schema addition for sessionInfo appears consistent; ensure downstream code handles optional sessionInfo properly. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. js/src/sdk/utils/telemetry/index.ts:23
- Draft comment:
Ensure that adding sessionId to telemetry meta does not expose sensitive data. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that adding sessionId to telemetry meta does not expose sensitive data. This is a request for the author to double-check something, which violates the rule against asking the author to ensure behavior is intended or to double-check things.
4. js/src/sdk/models/actions.ts:155
- Draft comment:
Good fallback for sessionId in execute; consider extracting the sessionInfo merging logic into a helper if reused elsewhere. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
5. js/src/sdk/types/action.ts:45
- Draft comment:
Add a description for the new sessionInfo schema for clarity. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
6. js/src/sdk/utils/telemetry/index.ts:23
- Draft comment:
Ensure consistency: If sessionId is added in sdk_meta for wrapped functions, consider including it in manualTelemetry as well. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
Workflow ID: wflow_2uW5i1oPqRQARmqM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on f8c6ef1 in 59 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. js/src/sdk/types/action.ts:45
- Draft comment:
Unnecessary formatting change. The logic remains unchanged—consider reverting if not required. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
According to the rules, we should not comment on purely stylistic changes. The comment doesn't suggest any actual code improvements or point out bugs - it's just noting that the formatting changed. Even if the formatting change wasn't necessary, commenting on it doesn't add value.
Perhaps the formatting change could make the code harder to read or goes against team style guidelines?
The rules explicitly state not to comment on purely stylistic changes. Even if there are style guidelines, this kind of feedback should be handled through automated linting/formatting rather than PR comments.
This comment should be deleted as it only addresses a formatting change with no functional impact.
2. js/src/sdk/types/action.ts:45
- Draft comment:
The sessionInfo property has been reformatted to use chained calls. This is a purely stylistic change with no functional impact. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_DLtRbx5Rd3MS0NcM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Adds session information to action execution and telemetry in the Composio SDK.
sessionInfo
withsessionId
toexecute
method inActions
class inactions.ts
.ComposioSDKContext.sessionId
ifsessionId
is not provided inrequestBody
.ZExecuteParams
inaction.ts
to include optionalsessionInfo
withsessionId
.sessionId
to telemetry data inTELEMETRY_LOGGER
inindex.ts
.This description was created by
for f8c6ef1. It will automatically update as commits are pushed.