-
Notifications
You must be signed in to change notification settings - Fork 23
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
Implement user-selected context files #367
Conversation
a6a4a20
to
e1a1f8e
Compare
pass context files on chat submit proper URI serialization
e1a1f8e
to
68ae69e
Compare
val POP_UPPER_MESSAGE_ACTION_SHORTCUT = CustomShortcutSet(UP) | ||
val POP_LOWER_MESSAGE_ACTION_SHORTCUT = CustomShortcutSet(DOWN) | ||
// split path on separator (OS agnostic) | ||
val pathComponents = path.split(File.separator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safer to avoid string operations on paths.
More idiomatic way to do it could be:
val pathLength = path.getNameCount()
val shortenPath = Paths.get("..").
resolve(path.subpath(pathLength - 3, pathLength))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get the recommendation here—converting uri.path
to a Path
object seems inadvisable, since the former uses posix style path separators, whereas Path
seems like it would be OS-specific. This does flag an issue where I was using File.separator
to split the path string, where I should just be using /
(and then replacing with File.separator
in the return value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I haven't noticed that we start with uri and not path, and so we have are quarantined to have well defined schema we want to preserve.
You can ignore my comment then.
if (listener != null) { | ||
listener.accept(params.getMessage()); | ||
} else { | ||
logger.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it makes sense to change it back to warn?
I recently switched if from warn to debug because I think this is not a real warning (we consciously do not handle other messages). I think we should setup it in the way that we on dev machines always see all debug logs but it does not leak to the users.
|
||
private final Map<String, Consumer<ExtensionMessage>> webviewMessageListeners = new HashMap<>(); | ||
|
||
public void onWebviewMessage(String panelID, Consumer<ExtensionMessage> callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like webviewMessageListeners
will be accessed from multiple threads.
We need to make it ConcurrentHashMap
.
@@ -46,6 +44,25 @@ private constructor( | |||
|
|||
init { | |||
cancellationToken.get().dispose() | |||
|
|||
CodyAgentService.applyAgentOnBackgroundThread(project) { agent -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need to add this code also in restoreAgentSession
.
The way it works right now is that we do not re relay on stable agent connection or even agent process existence in any given moment in time (as assumptions in that area were was major source of instability in past). applyAgentOnBackgroundThread
was added specifically to enforce that contract as it waits for agent to be operational before executing any operation on it (and that part of your code is fine).
But after successful startup, agent can be shut down and restarted later (e.g due to crash or account change), and in such case if we rely on any state in/out of agent we need to make sure to restore it. In restoreAgentSession
we are already doing this for current session restoring chat history in agent and obtain new sessionId.
|
||
fun getPanel(): ChatPanel = chatPanel | ||
// Register webview message listener on the agent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agent connection is fully managed in the CodyAgentService already.
But restoreAgentSession
is necessary to reload history of the every chat inside the agent (as agent is stateless, and if restarted looses any context).
The callback you added is different thing, and indded it would couple those object lifecycles a bit.
On the bright side I already resolved that issue by the way of the rewrite I did for multi-chat.
Specifically, look at this part:
https://github.com/sourcegraph/jetbrains/blob/main/src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentService.kt#L24
There is new service called AgentChatSessionService
which allow you to get chat session by sessionId/panelId. It's used to dispatch messages based on id to the correct AgentChatSession
instance. Currently it calls receiveMessage
on it, but you can add second callback (similar to agent.client.onNewMessage
) which will in the same way dispatch to receiveWebviewExtensionMessage
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The recent changes (#367) broke autocomplete. The uri was string before but not it is properly serialized 😅 Agent expect the param to be string (not an object). Let's keep the serialisation for now but just pass string as param. ## Test plan - autocomplete works
Adds the ability for the user to at-mention context files in the chat input.
jb.mov
Not implemented:
Questions
Test plan