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

Improve / fix arrow keys for Cody history #51586

Merged
merged 8 commits into from
May 15, 2023
Merged

Conversation

SuperAuguste
Copy link
Contributor

@SuperAuguste SuperAuguste commented May 8, 2023

Closes #51575.

Some issues that were fixed:

  • Down arrow did not navigate history forward
  • You couldn't navigate the WIP prompt with the up arrow as it would navigate history always; now we only navigate to backwards when the up arrow is pressed at the start of the prompt and forwards then the down arrow is pressed at the end of the prompt
  • There was a typo that prevented the history index from being set properly in VSCode
  • Input history wasn't populated on the web

Test plan

Tested history navigation in VSCode and web.

@cla-bot cla-bot bot added the cla-signed label May 8, 2023
@SuperAuguste SuperAuguste requested a review from a team May 8, 2023 18:19
@sourcegraph-buildkite
Copy link
Collaborator

sourcegraph-buildkite commented May 8, 2023

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.33 kb) 0.00% (+0.33 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits d46f1ab and 0023755 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented May 8, 2023

📖 Storybook live preview

@SuperAuguste SuperAuguste force-pushed the auguste/history-arrow-keys branch 3 times, most recently from b395460 to fb1fa81 Compare May 8, 2023 19:17
Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Approving from VS Code side, but I think @thenamankumar you should look at Web.

client/cody/webviews/Chat.tsx Outdated Show resolved Hide resolved
@@ -74,6 +74,16 @@ export const CodyChat = ({ onClose }: CodyChatProps): JSX.Element => {
}
}, [transcript, shouldScrollToBottom, messageInProgress])

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong. transcriptHistory is transcriptJSON[]. It is the list of conversations not a list of chat messages belonging to a single transcript.

I guess what you want to do is:

transcript
  .filter(message => message.speaker === 'human' && !!message.displayText)
  .map(({displayText}) => displayText)

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 believe my code is the desired behavior as history navigation in VSCode goes through all user inputs for all past conversations, not just the current transcript. (But I'd be happy to switch to the behavior you mentioned if you believe that's the right course of action :))

Copy link
Member

Choose a reason for hiding this comment

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

I haven't experienced it in VS Code yet, will you be able to share a demo video regarding how this will behave on the web when you populate inputHistory with messages from all the previous chats and not just the active chat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for asking this Naman, it actually unearthed an odd behavior where only the current chat's messages are populated. I'll look into this.

EDIT: It's because transcriptHistory seems to contain messages like [A, B, C, A, B, C, D, E] where the first C is where I stopped the conversation before reloading the page. Figured it out: there was a incorrectly formatted conversation in there from a long time ago, everything's working great now - I'll send a demo video!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This looks good.

Copy link
Member

@thenamankumar thenamankumar left a comment

Choose a reason for hiding this comment

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

.

@mrnugget
Copy link
Contributor

Heads up: looks like you're getting a TypeScript compiler error

client/cody-web/src/Chat.tsx(71,9): error TS2322: Type '((event: KeyboardEvent<HTMLElement>, caretPosition: number \| null) => void) \| undefined' is not assignable to type 'KeyboardEventHandler<HTMLTextAreaElement> \| undefined'.
--
  | Type '(event: KeyboardEvent<HTMLElement>, caretPosition: number \| null) => void' is not assignable to type 'KeyboardEventHandler<HTMLTextAreaElement>'.

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Noice! This works very well (tested in VS Code). The old behavior was really buggy and confusing.

Don't forget to add a change log entry :)

client/web/src/cody/components/ChatUI/ChatUi.tsx Outdated Show resolved Hide resolved
client/cody-ui/src/Chat.tsx Outdated Show resolved Hide resolved
@SuperAuguste SuperAuguste merged commit 04ff23b into main May 15, 2023
25 of 26 checks passed
@SuperAuguste SuperAuguste deleted the auguste/history-arrow-keys branch May 15, 2023 16:59
@mrnugget
Copy link
Contributor

Thanks @SuperAuguste!

cesrjimenez pushed a commit that referenced this pull request May 17, 2023
Closes #51575.

Some issues that were fixed:
- Down arrow did not navigate history forward
- You couldn't navigate the WIP prompt with the up arrow as it would
navigate history always; now we only navigate to backwards when the up
arrow is pressed at the start of the prompt and forwards then the down
arrow is pressed at the end of the prompt
- There was a typo that prevented the history index from being set
properly in VSCode
- Input history wasn't populated on the web

## Test plan

Tested history navigation in VSCode and web.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cody: down arrow doesn't works to get the next prompt
6 participants