Skip to content

refactor: simplify nested map access and improve readability #5247

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lkk214
Copy link
Contributor

@lkk214 lkk214 commented Apr 19, 2025

Description

  • Eliminate fragile type conversions and simplify access to nested properties
  • Fixed the issue that inline cannot get the model list
    bug0419

@lkk214 lkk214 requested a review from a team as a code owner April 19, 2025 13:48
@lkk214 lkk214 requested review from tomasz-stefaniak and removed request for a team April 19, 2025 13:48
Copy link

netlify bot commented Apr 19, 2025

Deploy Preview for continuedev canceled.

Name Link
🔨 Latest commit 52b1704
🔍 Latest deploy log https://app.netlify.com/sites/continuedev/deploys/68150e9019de0500089cff2b

@sestinj
Copy link
Contributor

sestinj commented Apr 24, 2025

@lkk214 I really like this update! Unfortunately before I saw it, we merged another PR that made related changes. I do like the way that you did it more. Would you be willing to resolve the merge conflicts, and then I'll merge?

@lkk214
Copy link
Contributor Author

lkk214 commented Apr 25, 2025

@sestinj Before submitting this PR, I was confused and therefore didn't change any other relevant code except the drop-down menu. Actually inline doesn't work as expected, I'm raising the issue now.

  1. Does the drop-down menu option value in inline come from config.modelsByRole.edit, or from config.modelsByRole.chat, or config.modelsByRole.edit ?? config.modelsByRole.chat
    (Normally it should be config.modelsByRole.edit, and use config.modelsByRole.chat if config.modelsByRole.edit does not exist, but streamDiffLines only uses config.selectedModelByRole.chat,

    continue/core/core.ts

    Lines 77 to 82 in 8057b2a

    const llm = config.selectedModelByRole.chat;
    if (!llm) {
    throw new Error("No chat model selected");
    }

    and is the edit role necessarily also the chat role?)

  2. Selecting a model in the drop-down menu does not work, it does not change the value of selectedModelByRole because it does not send the config/updateSelectedModel message to the core.
    So does the IDE need to send a message when a model is selected, or use this code in streamDiffLines?
    const llm = msg.data.modelTitle ?? config.selectedModelByRole.chat

# Conflicts:
#	extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/continue/CoreMessengerManager.kt
#	extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/continue/IdeProtocolClient.kt
#	extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/editor/InlineEditAction.kt
sestinj
sestinj previously approved these changes Apr 29, 2025
@sestinj
Copy link
Contributor

sestinj commented Apr 29, 2025

@lkk214 you're right, it is supposed to be config.modelsByRole.edit ?? config.modelsByRole.chat.

Do you want me to merge now and let you make this update in another PR?

@lkk214
Copy link
Contributor Author

lkk214 commented Apr 30, 2025

Selecting a model in the drop-down menu does not work, it does not change the value of selectedModelByRole because it does not send the config/updateSelectedModel message to the core.
So does the IDE need to send a message when a model is selected, or use this code in streamDiffLines?
const llm = msg.data.modelTitle ?? config.selectedModelByRole.chat

Send config/updateSelectedModel, or msg.data.modelTitle ?? config.selectedModelByRole.chat?

I prefer the latter. I need it for my next PR, otherwise I don't know which selectedModelByRole applies to the selected model and don't want to update the selected model of chat through it, any thoughts on this?

demo

@RomneyDa
Copy link
Collaborator

RomneyDa commented May 2, 2025

Note using selectedModelByRole.chat is fixed here
#5465

@lkk214
Copy link
Contributor Author

lkk214 commented May 2, 2025

@RomneyDa Got it.

config.modelsByRole.edit ?? config.modelsByRole.chat.

I've pushed the changes, please merge this PR now. Thanks!

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.

3 participants