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: Allow copying an entire session #91

Merged
merged 4 commits into from
Jul 20, 2024

Conversation

web2ls
Copy link
Contributor

@web2ls web2ls commented Jul 14, 2024

Closes #87

@fmaclen
Copy link
Owner

fmaclen commented Jul 14, 2024

@web2ls I'm not sure if you are still working on this PR of it's ready for review but regardless this feature will need a test.
We run integration tests using Playwright. I think we can just add new assertions to the existing "Copy" tests.

After you are done feel free to ping me or "request" my review.
Thanks!

@fmaclen fmaclen changed the title Allow copying an entire session feat: Allow copying an entire session Jul 14, 2024
@web2ls
Copy link
Contributor Author

web2ls commented Jul 16, 2024

@fmaclen please check it

@@ -177,6 +190,7 @@

<svelte:fragment slot="nav">
{#if !isNewSession}
<CopyButton content={sessionContent} title="Copy session" />
Copy link
Owner

@fmaclen fmaclen Jul 17, 2024

Choose a reason for hiding this comment

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

I think we can simplify the implementation significantly by doing this:

Suggested change
<CopyButton content={sessionContent} title="Copy session" />
<CopyButton content={JSON.stringify(session.messages, null, 2)} />

Then we can get rid of the copySessionContent() function and the sessionContent variable.

The result will retain the structure of the session which is useful if you are pasting this session into new session as knowledge/context or another LLM entirely:

[
  {
    "role": "user",
    "content": "mic check, 1 2 3"
  },
  {
    "role": "ai",
    "content": "Checking the microphone input, 1...2...3."
  },
  {
    "role": "user",
    "content": "hello hello"
  },
  {
    "role": "ai",
    "content": "Hello! Is there anything I can assist you with today? If you have any questions or need assistance, please feel free to ask."
  }
]

Keeping the structure of the message will also be really useful for #54

Comment on lines 6 to 14
export let title: string;

function copyContent() {
navigator.clipboard.writeText(content);
}
</script>

<span class="copy-button">
<Button title="Copy" variant="icon" size="icon" on:click={copyContent}>
<Button title={title} variant="icon" size="icon" on:click={copyContent}>
Copy link
Owner

@fmaclen fmaclen Jul 17, 2024

Choose a reason for hiding this comment

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

Let's keep the default title="Copy" attribute for now and remove the export let title: string;.

await page.getByText('Sessions', { exact: true }).click();
await mockCompletionResponse(page, MOCK_SESSION_1_RESPONSE_1);
await page.getByTestId('new-session').click();
await expect(page.getByTitle('Copy session')).toHaveCount(0);
Copy link
Owner

Choose a reason for hiding this comment

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

After removing the custom title attribute you can target the "Copy session" button with:

await expect(page.locator(".header").getByTitle('Copy')).toHaveCount(0);

Copy link
Owner

Choose a reason for hiding this comment

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

Also, minor formatting issue, can you add an empty line after this assertion?
I'd like to group interactions + assertions in "blocks" separated by empty lines.

Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

Thanks for putting together this PR.
It's a great start 👍

Left you some notes on a few ways we can simplify the implementation.

@fmaclen
Copy link
Owner

fmaclen commented Jul 19, 2024

I just merged #90 and there are 2 minor conflicts in your branch.
Let me know if you need help settling them.

Also, this PR is almost done and I would like to merge it soon if you can, if you can't that's ok, just give me a heads up.

@web2ls
Copy link
Contributor Author

web2ls commented Jul 20, 2024

@fmaclen
sorry for delay
I've made changes based on your comments.

Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

@web2ls this looks great, thank you!

@fmaclen fmaclen merged commit 2c682ad into fmaclen:main Jul 20, 2024
1 check passed
@fmaclen
Copy link
Owner

fmaclen commented Jul 20, 2024

🎉 This PR is included in version 0.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow copying an entire session
2 participants