Skip to content
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: upgraded the methods for Copilot using pieces-copilot-sdk #138

Conversation

VishalPawar1010
Copy link
Contributor

Description

This PR fixes #123

@VishalPawar1010 VishalPawar1010 changed the title feat: upgraded the methods for pieces copilot to implement pieces-copilot-sdk feat: upgraded the methods for Copilot using pieces-copilot-sdk Jul 20, 2024
GlobalConversationID = output.iterable.at(0).id;
const getInitialChat = async () => {
try {
const allConversations = await piecesClient.getConversations();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @VishalPawar1010 great work with this implementation. I was just wondering if you would like to also implement an additional functionality actually to list all of the conversations using the getConversations endpoint. on the UI. right now the back and forward buttons are not functionality. We can have a conversations list. and let the user select which conversation they wish to view. And you can use the piecesClient to then return that conversation ID and display the existing conversation in the Copilot chat window
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can remove the back and forward buttons and have a UI button for: choose copilot conversation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts @Arindam-at-Pieces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will implement this functinality to list conversations and give option to select then display selected conversation.
Do we have any plan to use TailwindCss in future for UI feature additions (like dropdown etc) like this ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think we can definitely introduce Tailwind @VishalPawar1010
I think that is a larger discussion as part to restructure the UI / Revamp the entire project, so that can definitely be explored.

Copy link
Contributor Author

@VishalPawar1010 VishalPawar1010 Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok then, until it is finalised I will implement UI in Vanilla Js + Css.

Copy link
Contributor Author

@VishalPawar1010 VishalPawar1010 Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @VishalPawar1010 what I will recommend is if you can also display the messages of the specific chat that you have selected in the chat window

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sure. Will try to display message as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VishalPawar1010 VishalPawar1010 force-pushed the #123-Upgrade-the-method-for-Pieces-Copilot branch from 3e19de1 to c130f4a Compare July 25, 2024 08:15
@shivay-at-pieces
Copy link
Contributor

Hey @VishalPawar1010 I dont see a change in the chat box when changing the conversation, did you test it?

@VishalPawar1010
Copy link
Contributor Author

VishalPawar1010 commented Jul 25, 2024

Yes, @shivay-at-pieces , in previous chat I added the video link of test - https://www.awesomescreenshot.com/video/29878767?key=e661acd7694e4055f75144ae76d61c5d

Somehow in video it does not show the dropdown, but it is there and we can see recent response when conversation is selected.

@shivay-at-pieces
Copy link
Contributor

WHat I meant was when I select a previous chat, it doesn't show the actual chat content in the chat window

@VishalPawar1010
Copy link
Contributor Author

VishalPawar1010 commented Jul 25, 2024

What it shows? Some message or nothing?
Because we cant show entire chat history there. We can only share previous response for now.

@shivay-at-pieces
Copy link
Contributor

The chat window content doesn't change. I agree we can show the last response / message in the conversation


}
const PORT = 5323
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should be Dynamic. You can probably use the os package to check the os and change the port dynamically

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will try to test with "os" package and its implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested 3 approaches (libraries) for the dynamic os detection

  1. https://www.npmjs.com/package/os
  2. https://www.npmjs.com/package/react-device-detect
  3. https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform

Option 3 which uses JavaScript’s navigator object suites best for the case, for os observed issues in imports, for react-device-detect observed limited os types detection.

Could you please review and let me know for any.

src/platform.config.ts Outdated Show resolved Hide resolved
src/platform.config.ts Outdated Show resolved Hide resolved
src/platform.config.ts Outdated Show resolved Hide resolved
src/platform.config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Arindam200 Arindam200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shivay-at-pieces shivay-at-pieces merged commit ba258c1 into pieces-app:main Aug 27, 2024
1 check passed
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.

Upgrade the method for Pieces Copilot experience using the SDK wrapper for creating Copilots
4 participants