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

refactor(webview): relocate model selector (CODY-5194) #7322

Merged
merged 28 commits into from
Mar 5, 2025
Merged

Conversation

julialeex
Copy link
Contributor

@julialeex julialeex commented Mar 4, 2025

Context: https://sourcegraph.slack.com/archives/C08DP4R5CG0/p1740789400315269

part of https://linear.app/sourcegraph/issue/CODY-5194/move-model-selector-to-the-top-nav-bar

  • moves the model selector to the top left instead of below the chat box
  • introduces a keyboard shortcut (cmd+m) to open the model selection dropdown in the webview
  • introduces a keyboard shortcut (cmd+n) to open a new chat

Test plan

Loom: https://www.loom.com/share/e742c98c2e3e4dada5eee85c40164b60

Screenshot 2025-03-04 at 9 42 09 PM
Screenshot 2025-03-04 at 9 42 21 PM

abeatrix and others added 16 commits February 28, 2025 17:11
This commit introduces a new `ModeSelectorButton` component to replace the intent selection logic within the submit button.

The changes include:

- Removal of intent selection logic from `SubmitButton.tsx`.
- Introduction of `ModeSelectorButton.tsx` with a popover menu for selecting chat modes (Chat, Search, Edit, Insert).
- Integration of the new `ModeSelectorButton` into the `Toolbar.tsx`.
- Optimizations for intent options and keyboard shortcuts to avoid recreating React elements.
This commit moves the model selection dropdown from the chat editor toolbar to the tab bar for better visibility and accessibility. It also simplifies the logic for determining the "new chat" command.

The changes include:

- Moving the `ModelSelectField` component from `vscode/webviews/chat/cells/messageCell/human/editor/toolbar/Toolbar.tsx` to `vscode/webviews/tabs/TabsBar.tsx`.
- Adding the `models` prop to the `TabsBar` component to pass available models to the `ModelSelectField`.
- Removing the `ModelSelectFieldToolbarItem` component.
- Adjusting the layout of the `TabsBar` to accommodate the new model selection dropdown.
- Simplifying the `getCreateNewChatCommand` function in `vscode/webviews/tabs/utils.ts` to always use `cody.chat.newPanel` instead of `cody.chat.newEditorPanel`.
- Adding a tooltip to the "New Chat" button in the tab bar.
add keyboard shorcut
@alexromano
Copy link
Contributor

Loom: https://www.loom.com/share/59c757bedbd048f795a7876424183783?sid=c1a419d6-cc85-4782-ad65-66b7c5ea70ca
i noticed at around 0:19 here, the tooltip shows the file path, and then we you hover again, it shows the keyboard shortcut,. is that a bug?

julialeex and others added 2 commits March 4, 2025 10:45
…Field

This commit introduces a keyboard shortcut (Cmd/Ctrl+M) to open the model selection dropdown in the webview.

The ModelSelectField component has been refactored to:

- Use a ref to directly control the popover's open/close state.
- Add an Escape key listener to close the dropdown.
- Remove the forwardRef and useImperativeHandle, simplifying the component's structure.
- Remove the openDropdownRef prop.
- Add a modelSelectorRef prop to allow parent components to control the dropdown.

These changes improve the user experience by providing a quick way to access the model selection and ensure the dropdown can be closed with the Escape key.
@julialeex
Copy link
Contributor Author

Loom: https://www.loom.com/share/59c757bedbd048f795a7876424183783?sid=c1a419d6-cc85-4782-ad65-66b7c5ea70ca
i noticed at around 0:19 here, the tooltip shows the file path, and then we you hover again, it shows the keyboard shortcut,. is that a bug?

@alexromano I don't think so. Re-record the video with the latest changes. https://www.loom.com/share/d91731628d7042c680d6150de392d412?sid=231710c7-3829-4f43-b819-9ac2fc1132d3

@@ -12,7 +12,7 @@ test('allows multiple enterprise models when server-sent models is enabled', asy
await sidebarSignin(page, sidebar, { enableNotifications: true })
// Open chat.
const [chatFrame] = await createEmptyChatPanel(page)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abeatrix just realized that if we open a new empty chat panel we can't see the model selector. However do we plan to remove the empty chat panel option?

Copy link
Contributor

Choose a reason for hiding this comment

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

we could just add the tab bar to the editor chat panel to show the model dropdown?

@@ -12,7 +12,7 @@ test('allows multiple enterprise models when server-sent models is enabled', asy
await sidebarSignin(page, sidebar, { enableNotifications: true })
// Open chat.
const [chatFrame] = await createEmptyChatPanel(page)
Copy link
Contributor

Choose a reason for hiding this comment

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

we could just add the tab bar to the editor chat panel to show the model dropdown?

@alexromano
Copy link
Contributor

alexromano commented Mar 4, 2025

https://www.loom.com/share/d91731628d7042c680d6150de392d412?sid=231710c7-3829-4f43-b819-9ac2fc1132d3

noticed a couple bugs here:

  • there's a huge delay when selecting the new model. i think this was pointed out previously but seems worse in this video. i get no delay on my extension right now
  • the "new chat" icon changes when you switch to history. i'd suggest we unify on the "+" icon for that
  • we should use the same pattern for all of the keyboard shortcut hovers. just the keys, with capitalized letters, no "+". so here it should be "⌘M"

@julialeex
Copy link
Contributor Author

julialeex commented Mar 4, 2025

  • there's a huge delay when selecting the new model. i think this was pointed out previously but seems worse in this video. i get no delay on my extension right now

@alexromano You mean between selecting and getting the new models rendered properly?

  • the "new chat" icon changes when you switch to history. i'd suggest we unify on the "+" icon for that

This is expected. I was trying to unify the chat and new chat icons. Will change to icon to always be '+' then!

  • we should use the same pattern for all of the keyboard shortcut hovers. just the keys, with capitalized letters, no "+". so here it should be "⌘M"

Will do!

@julialeex julialeex requested a review from abeatrix March 4, 2025 23:07
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

couldn't get your changes to work but pushed mine instead, can you give it a try and let me know if it doesn't work for you?
If not, we can revert it and debug it tmr

title: 'Chat',
Icon: MessagesSquareIcon,
title: currentView === View.Chat ? 'New Chat' : 'Chat',
Icon: PlusIcon,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Icon: PlusIcon,
Icon: PlusIcon,

Probably a question for product / design team. I noticed most of the apps use + icon for adding attachment, and the paper pen icon for new chat, so maybe we should use this icon instead? Wdyt?

image image

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 don't have a strong opinion here. @alexromano Wdyt? If we want product/design team's input who would be the good person to take a look here? I think Tim is OOO this week.

Copy link
Contributor

Choose a reason for hiding this comment

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

(it's not a big deal and can be done in a follow up)

For references:

Meta AI

image

Grok

image

ChatGPT

image

@abeatrix
Copy link
Contributor

abeatrix commented Mar 5, 2025

Might need some padding around the model dropdown in editor view:

image

@julialeex
Copy link
Contributor Author

julialeex commented Mar 5, 2025

couldn't get your changes to work but pushed mine instead, can you give it a try and let me know if it doesn't work for you? If not, we can revert it and debug it tmr

Thanks Bee! Yeah your changes work for me. My change works for me locally (commit 00ed8f003c894a1cd15e428560e0fa62e3aac302). @alexromano Does it work for you?

@alexromano
Copy link
Contributor

alexromano commented Mar 5, 2025

@alexromano Does it work for you?

most recent commit (47a55e93a6d54956b31a0f7cac783ba09762dff7) works for me!

capital M please! 😄
Screenshot 2025-03-04 at 8 40 37 PM

@julialeex
Copy link
Contributor Author

@alexromano Does it work for you?

most recent commit (47a55e93a6d54956b31a0f7cac783ba09762dff7) works for me!

capital M please! 😄 Screenshot 2025-03-04 at 8 40 37 PM

@abeatrix Since it works for Alex I'll revert your changes?

@abeatrix
Copy link
Contributor

abeatrix commented Mar 5, 2025

@julialeex @alexromano was cmd + n and escape working for you on 47a55e9 ?

@julialeex
Copy link
Contributor Author

@julialeex @alexromano was cmd + n and escape working for you on 47a55e9 ?

yes

@julialeex julialeex requested a review from abeatrix March 5, 2025 05:16
@julialeex
Copy link
Contributor Author

julialeex commented Mar 5, 2025

@alexromano @abeatrix Thank you guys for the detailed comments! I just updated my code again. Loom: https://www.loom.com/share/e742c98c2e3e4dada5eee85c40164b60

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Left some comments in line. I think we should revert the changes on updating the cody.chat.new keybinding to "cmd+n", and make sure it would work for other clients since the event listener is registered in the webview, which would make it work / break the other clients 😄

@@ -92,36 +92,33 @@ export const ToolbarButton = forwardRef<HTMLButtonElement, ToolbarButtonProps>(
)
ToolbarButton.displayName = 'ToolbarButton'

export const ToolbarPopoverItem: FunctionComponent<
export const ToolbarPopoverItem = forwardRef<
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: why do we need to update this to forwardRef? It seems like it still work as FunctionComponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like it still work as FunctionComponent?

What do you mean

why do we need to update this to forwardRef

I need it for the open: () => void

@julialeex julialeex merged commit d1a5d47 into main Mar 5, 2025
20 of 22 checks passed
@julialeex julialeex deleted the jlxu/move-model branch March 5, 2025 16:21
julialeex added a commit that referenced this pull request Mar 6, 2025
julialeex added a commit that referenced this pull request Mar 7, 2025
…#7351)

## Description
**WHAT**

1. Fix React warnings caused by [relocating the model
selector](#7322)

- Warning: React does not recognize the `popoverContentProps` prop on a
DOM element
  - Warning: Nested Buttons
- There are also other pre-existing warnings, but this PR does not fix
them

2. Remove `cmd+N` keyboard shortcut as it is not compatible for other
non vscode IDEs

**WHY**
Good practice to avoid React warnings. This prevents us from having
bigger problems in the future

**CONTEXT**
Slack:
https://sourcegraph.slack.com/archives/C05AGQYD528/p1741247210218679
Model Selector PR: #7322

## Test plan
**1. New e2e test `keyboard shortcuts cmd+M opens model selector`**
- Run `pnpm -C vscode test:e2e chat-keyboard-shortcuts.test.ts` and make
sure the test passes
<!-- Required. See
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles.
-->

**2. Fix React warnings caused by the model selector**

- Run `pnpm run test:unit --run -v` and scroll through the output and
check for warnings.
- The top warning should be `Warning: flushSync` was called from inside
a lifecycle method., which is a pre-existing warning.
- `popoverContentProps` and nested buttons warnings should not exist
when you search for them.
![Screenshot 2025-03-06 at 1 18
31 AM](https://github.com/user-attachments/assets/3c0abd26-93a3-405e-b836-216c695e44a7)


**3. Remove `cmd+N` keyboard shortcut as it is not compatible for other
non vscode IDEs**
- Run `pnpm install && pnpm build && pnpm -C vscode build && pnpm -C
vscode run dev `
- Click the Cody chat box and type `cmd+N`. This should create a new
file.
- Hover open the model select to see the tooltip
- `cmd+M` opens the model options, escape key closes the model options
See video before for my local testing
https://www.loom.com/share/0018805561774723a286350b57e3c9d1

---------

Co-authored-by: Beatrix <[email protected]>
julialeex added a commit that referenced this pull request Mar 9, 2025
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.

4 participants