[Backport M76] fix(chat): Fix React warnings caused by relocating the model selector #7366
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…#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.  **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]> (cherry picked from commit c36d1e0)
|
As I said on chat, I don't think you should backport a change that is just fixing warnings. |
|
@julialeex Is there more to this than fixing warnings? Agree with @dominiccooney that we shouldn't need to backport is this is just fixing warnings.
Is this critical to backport? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
WHAT
popoverContentPropsprop on a DOM elementcmd+Nkeyboard shortcut as it is not compatible for other non vscode IDEsWHY
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 selectorpnpm -C vscode test:e2e chat-keyboard-shortcuts.test.tsand make sure the test passes2. Fix React warnings caused by the model selector
pnpm run test:unit --run -vand scroll through the output and check for warnings.Warning: flushSyncwas called from inside a lifecycle method., which is a pre-existing warning.popoverContentPropsand nested buttons warnings should not exist when you search for them.3. Remove
cmd+Nkeyboard shortcut as it is not compatible for other non vscode IDEspnpm install && pnpm build && pnpm -C vscode build && pnpm -C vscode run devcmd+N. This should create a new file.cmd+Mopens the model options, escape key closes the model optionsSee video before for my local testing
https://www.loom.com/share/0018805561774723a286350b57e3c9d1
Backport c36d1e0 from fix(chat): Fix React warnings caused by relocating the model selector #7351