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

fix(ChatDisplay/index.tsx): users are only able to download a protocol #17015

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

Elyorcv
Copy link
Contributor

@Elyorcv Elyorcv commented Dec 3, 2024

Overview

Previously, when users clicked the download button, it created a Python file even for an empty string.
Now, the system downloads only valid protocols that are not single-line and must include def run(.

Closes AUTH-1064

Test Plan and Hands on Testing

  • Go to OpentronsAI.
  • Generate a protocol and download it. You should see a Python file containing the protocol.
  • Simulate the protocol and get feedback.
  • Click Download again, but this time, you should not be able to download the file if the protocol is invalid.

Changelog

  • Modified the functionality of the Download button to ensure only valid protocols can be downloaded.

Review requests

Look at the section Test Plan and Hands on Testing

Risk assessment

Low

@Elyorcv Elyorcv force-pushed the AUTH-1064-saving-protocol-incorrectly branch 2 times, most recently from e45ac23 to 311d693 Compare December 3, 2024 22:47
@Elyorcv Elyorcv marked this pull request as ready for review December 3, 2024 22:59
@Elyorcv Elyorcv requested a review from a team as a code owner December 3, 2024 22:59
@Elyorcv Elyorcv force-pushed the AUTH-1064-saving-protocol-incorrectly branch 5 times, most recently from 6a319b2 to d66207d Compare December 4, 2024 00:19
current when user clicks the download button it create a python for even an empty string.
@Elyorcv Elyorcv force-pushed the AUTH-1064-saving-protocol-incorrectly branch from d66207d to eaeee29 Compare December 4, 2024 00:33
@Elyorcv Elyorcv requested review from y3rsh and jbleon95 December 4, 2024 00:44
Copy link
Member

@y3rsh y3rsh left a comment

Choose a reason for hiding this comment

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

1 Question but not a blocker.

@@ -106,7 +106,18 @@ export function ChatDisplay({ chat, chatId }: ChatDisplayProps): JSX.Element {

const handleFileDownload = (): void => {
const lastCodeBlock = document.querySelector(`#${chatId}`)
const code = lastCodeBlock?.textContent ?? ''
const code = lastCodeBlock?.textContent?.trim() ?? ''
Copy link
Member

Choose a reason for hiding this comment

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

The logic here makes sense and I am guessing that we instruct the LLM to place the protocol contents in the last code block of the response? Feels a bit brittle but we do not need to address in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this version, we assume that there is only one code block.

@Elyorcv Elyorcv merged commit c2816ce into edge Dec 5, 2024
8 checks passed
@Elyorcv Elyorcv deleted the AUTH-1064-saving-protocol-incorrectly branch December 5, 2024 20:17
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