Skip to content

Add trainloop sdk #1735

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

Merged
merged 5 commits into from
Apr 5, 2025
Merged

Add trainloop sdk #1735

merged 5 commits into from
Apr 5, 2025

Conversation

Kitenite
Copy link
Contributor

@Kitenite Kitenite commented Apr 5, 2025

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Release
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes


Important

Adds TrainLoop SDK integration with new TrainLoopManager class and updates IPC channels for handling TrainLoop operations.

  • New Feature:
    • Introduces TrainLoopManager in trainloop.ts to manage TrainLoop SDK interactions.
    • Adds getClient() and saveApplyResult() methods to handle client creation and data submission.
    • Updates listenForChatMessages() in chat.ts to handle SAVE_APPLY_RESULT IPC channel.
  • Dependencies:
    • Adds @trainloop/sdk to package.json and bun.lock.
  • Miscellaneous:
    • Changes error message in index.ts to 'An unknown chat error occurred'.
    • Adjusts SCROLL_COOLDOWN in Carousel.tsx from 100 to 50.

This description was created by Ellipsis for 67c1d76. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 85193c7 in 1 minute and 21 seconds

More details
  • Looked at 134 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. apps/studio/electron/main/chat/index.ts:172
  • Draft comment:
    Updated error message now specifies 'chat error'. Ensure this change is consistent with the overall error handling strategy.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. apps/studio/package.json:46
  • Draft comment:
    Added dependency '@trainloop/sdk' version ^0.1.5. Confirm its compatibility with the current project and update tests if needed.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. apps/studio/src/routes/projects/ProjectsTab/Select/Carousel.tsx:18
  • Draft comment:
    Scroll cooldown reduced from 100ms to 50ms. Verify that this change provides the desired UX without side effects.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. packages/models/src/constants/api.ts:18
  • Draft comment:
    Added TRAINLOOP route under ProxyRoutes. Ensure that the backend supports this route and that access control is properly configured.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. apps/studio/electron/main/chat/index.ts:172
  • Draft comment:
    Updated error message to 'An unknown chat error occurred'. Verify that any logic relying on the old string is updated.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. apps/studio/package.json:46
  • Draft comment:
    Added '@trainloop/sdk' dependency. Confirm that its version and integration conform with project requirements.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. apps/studio/src/routes/projects/ProjectsTab/Select/Carousel.tsx:18
  • Draft comment:
    SCROLL_COOLDOWN reduced from 100ms to 50ms; ensure this change achieves the desired UI responsiveness without side effects.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
8. packages/models/src/constants/api.ts:18
  • Draft comment:
    Added TRAINLOOP route to ProxyRoutes. Confirm that this endpoint is correctly configured on the backend.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
9. apps/studio/package.json:30
  • Draft comment:
    The 'pree2e' script on line 30 may be a typographical error. Typically, a pre-script might be named with a colon (e.g., 'pre:e2e') or another consistent naming convention. Please confirm if 'pree2e' is intentional; if not, consider renaming it to avoid confusion.
  • 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_zV70w7cxIsa6EjIj


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 295eff8 in 33 seconds

More details
  • Looked at 18 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. apps/studio/src/lib/editor/engine/chat/message/assistant.ts:26
  • Draft comment:
    Removed getStringContent method. Verify that this method is no longer used elsewhere as its removal might break functionality.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify that the removal of a method does not break functionality elsewhere. This falls under the rule of not asking the author to double-check things or ensure behavior is intended. Therefore, this comment should be removed.
2. apps/studio/src/lib/editor/engine/chat/message/assistant.ts:1
  • Draft comment:
    The PR title and description mention adding trainloop sdk and improved onboarding docs, but the diff only shows method removal. Confirm that additional changes required by the issue are included elsewhere.
  • 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.
3. apps/studio/src/lib/editor/engine/chat/message/assistant.ts:26
  • Draft comment:
    The removed getStringContent() method may be relied upon by client code. Confirm that its functionality (converting content to a string) is no longer needed or has been replaced.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to confirm that the removal of a method is intentional and that its functionality is no longer needed or has been replaced. This falls under asking the author to confirm their intention, which is against the rules. Therefore, this comment should be removed.

Workflow ID: wflow_wa8reHORFl5PH2jM


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 9f2e838 in 1 minute and 54 seconds

More details
  • Looked at 165 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. apps/studio/electron/main/chat/trainloop.ts:15
  • Draft comment:
    Empty string passed as first argument to Client. Confirm if this is intentional and safe.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment is asking for confirmation/verification, which violates our rules. We don't have access to the Client class implementation, so we can't be certain this is actually a problem. The code may be intentionally designed to work with an empty string first parameter.
    The empty string could potentially cause runtime issues if the Client class requires a non-empty first parameter. We might be missing important security or configuration concerns.
    Without seeing the Client implementation, we can't be certain this is a problem. The rules explicitly state not to ask for confirmation or verification, and to only keep comments when we have strong evidence of an issue.
    Delete the comment because it's asking for verification without strong evidence that the empty string is actually problematic.
2. apps/studio/src/lib/editor/engine/chat/code.ts:83
  • Draft comment:
    Consider handling/awaiting the promise from saveApplyResult; errors might be swallowed.
  • Reason this comment was not posted:
    Based on historical feedback, this comment is too similar to comments previously marked by users as bad.
3. apps/studio/src/lib/editor/engine/chat/code.ts:172
  • Draft comment:
    dispose() sets observables to null; ensure this cleanup strategy doesn't lead to side-effects.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
4. apps/studio/electron/main/chat/trainloop.ts:15
  • Draft comment:
    The Client constructor is passed an empty string as the first parameter. Confirm if this is intentional or if this value should be configurable.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment is asking "Confirm if this is intentional" which directly violates the rule about not asking for confirmations. Additionally, without seeing the Client class implementation from @trainloop/sdk, we can't be certain that the empty string is actually problematic. The code may be working as intended.
    Maybe the empty string really is a bug that needs to be fixed? The first parameter position often expects a meaningful value.
    While an empty string parameter might be suspicious, the rules clearly state we need STRONG EVIDENCE that there's an issue. Without seeing the Client implementation, we can't be certain. Moreover, the comment's phrasing as a request for confirmation makes it inappropriate.
    Delete the comment because it violates the rule about asking for confirmations and we don't have strong evidence that the empty string is actually problematic.
5. apps/studio/electron/main/chat/trainloop.ts:27
  • Draft comment:
    Casting SampleFeedbackType to string for sendData might be error‐prone. Consider an explicit conversion or ensuring type safety.
  • 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%
    The type cast seems intentional since the sendData method likely expects a string parameter. Without seeing the Client class definition or understanding SampleFeedbackType, we can't be certain this is actually problematic. The code compiles and TypeScript would catch type mismatches. The comment is speculative about potential issues rather than pointing out a clear problem.
    I might be underestimating the risks of type casting. There could be valid reasons why this cast could cause runtime issues.
    While type casting can be risky, TypeScript's type system would catch incompatible casts at compile time. Without clear evidence of a problem, this is a speculative concern.
    The comment should be deleted as it raises a speculative concern without clear evidence of an actual problem, and we don't have enough context to confirm if this is truly an issue.
6. apps/studio/src/lib/editor/engine/chat/code.ts:61
  • Draft comment:
    If writing the file content fails, the applySuccess flag isn’t updated. This may lead to inaccurate feedback. Consider setting applySuccess = false on write failure.
  • 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.
7. apps/studio/src/lib/editor/engine/chat/code.ts:95
  • Draft comment:
    The invokeMainChannel call in saveApplyResult is not awaited. Consider awaiting its result or handling potential errors.
  • Reason this comment was not posted:
    Based on historical feedback, this comment is too similar to comments previously marked by users as bad.
8. PR_DESCRIPTION:1
  • Draft comment:
    The PR description mentions adding better onboarding docs (issue 123), but the changes only reflect trainloop SDK integration. Please update the PR description to accurately reflect the changes or include the missing documentation updates.
  • 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.
9. packages/models/src/constants/ipc.ts:54
  • Draft comment:
    Typographical issue: The constant CLEAN_CODE_KEYS is assigned the string value 'clean-move-keys'. This value seems inconsistent with the constant name. Please review whether the value should be 'clean-code-keys' to match the naming convention or if the constant name should be adjusted.
  • 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.
10. packages/models/src/constants/ipc.ts:64
  • Draft comment:
    Typographical issue: The constant GET_TEMPLATE_NODE_CLASS has a value 'get-template-node-classes'. There is an inconsistency between the singular form in the constant name and the plural form in the string. Please verify if this is intentional or if one should be corrected 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_fvC7Ujb5udXA0dBL


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 67c1d76 in 52 seconds

More details
  • Looked at 58 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. apps/studio/electron/main/chat/trainloop.ts:10
  • Draft comment:
    Consider validating that the environment variable VITE_SUPABASE_API_URL is defined to avoid forming an invalid proxyUrl.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. apps/studio/electron/main/chat/trainloop.ts:27
  • Draft comment:
    Review the casting 'type as string' when calling sendData; if possible, align the types to avoid unsafe casts.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. packages/models/src/constants/api.ts:16
  • Draft comment:
    Removal of TRAINLOOP_COLLECT_ROUTE: Ensure that all usages are updated to use the new route in TrainLoopManager.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. apps/studio/electron/main/chat/trainloop.ts:11
  • Draft comment:
    Verify that omitting TRAINLOOP_COLLECT_ROUTE in the proxy URL (line 11) is intentional and aligns with backend expectations for the 'onlook-apply-set' endpoint.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. apps/studio/electron/main/chat/trainloop.ts:28
  • Draft comment:
    Avoid forcing a type cast ('as string') on SampleFeedbackType in sendData; consider enforcing proper type safety in the SDK.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. apps/studio/electron/main/chat/trainloop.ts:10
  • Draft comment:
    Consider caching the Client instance if auth tokens remain valid to reduce overhead from repeated instantiation in getClient.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. packages/models/src/constants/api.ts:17
  • Draft comment:
    Ensure that the removal of TRAINLOOP_COLLECT_ROUTE is consistent with the new TrainLoop SDK usage and that no other parts of the codebase depend on it.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_SjtMIRgGwcZNDDCX


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@Kitenite Kitenite merged commit 0243ba2 into main Apr 5, 2025
@Kitenite Kitenite deleted the feat/trainloop branch April 10, 2025 23:53
ml-delaurier pushed a commit to ml-delaurier/nolook that referenced this pull request Apr 23, 2025
* Update scroll cooldown embla

* Add trainloop collection
t1c1 pushed a commit to t1c1/onlookbotcodes that referenced this pull request Jun 5, 2025
* Update scroll cooldown embla

* Add trainloop collection
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.

1 participant