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

pass chat history to webview using webviewAPI, not old postMessage protocol #5741

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sqs
Copy link
Member

@sqs sqs commented Sep 28, 2024

Previously, chat history was synced to the webview using the old postMessage protocol. Now, the ChatHistoryManager exposes an observable and that is read directly (using the standard webviewAPI) by the webview. This is a code refactor with no user-facing behavior change.

Test plan

In Cody Web and in VS Code, try downloading chat history. Ensure the JSON file is downloaded and is a JSON array of chat history.

Copy link
Contributor

@vovakulikov vovakulikov left a comment

Choose a reason for hiding this comment

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

Tested manually in Cody Web demo and linked in Sourcegraph Web
Found a few problems (nothing big but just UI/UX regressions)

  • It looks like there is no way to reach zero empty state that we've added here Add zero state for history tab #5390 anymore
  • Chat deleting doesn't work right if we try to delete the chat that we're currently in. Not sure this is a PR problem, the real issue here is how we work with active panel IDs (once we delete chat we delete the panel id but without the panel id we can't send anything through the postMessage channel, feel free to ignore it)

@sqs sqs force-pushed the sqs/history-api branch 3 times, most recently from 80099b0 to 10205fc Compare September 30, 2024 08:01
@sqs
Copy link
Member Author

sqs commented Sep 30, 2024

@vovakulikov Cool, I fixed the empty state problem and added a test to verify the fix (after confirming it manually).

@sqs
Copy link
Member Author

sqs commented Sep 30, 2024

Stamp because @vovakulikov reviewed it and I fixed the one outstanding issue

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.

2 participants