-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Add pinned context support for Amazon Q chat #473
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
Conversation
plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/AmazonQLspClientImpl.java
Show resolved
Hide resolved
plugin/src/software/aws/toolkits/eclipse/amazonq/views/AmazonQChatViewActionHandler.java
Outdated
Show resolved
Hide resolved
QEclipseEditorUtils.getSelectionRange(editor).ifPresent(range -> { | ||
Map<String, Object> cursorState = new HashMap<>(); | ||
cursorState.put("range", range); | ||
params.put("cursorState", cursorState); |
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.
Should this put an explicit null if the selection range isn't present, like the else branch?
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.
Nope, we can skip it.
if (debounceTask != null) { | ||
debounceTask.cancel(true); | ||
} |
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.
Dumb question, but is there a case where actively cancelling debounces could end up running into whatever throttling problem the debounce tasks is supposed to prevent against?
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.
Not really, intention is to handle too many active editor switching user may perform. Cancel will only cancel the scheduled task which is supposed to execute 100ms after, which means the lsp notification would not have been sent to be throttled anyway.
var lspServer = Activator.getLspProvider().getAmazonQServer().get(); | ||
lspServer.activeEditorChanged(params); | ||
} catch (Exception e) { | ||
Activator.getLogger().error("Failed to send active editor changed notification", e); |
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.
Should the catch end the debounce task?
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.
No, that is not necessary. The task will end naturally as its already in progress and does not need cancelling.
.map(this::getRelativePath); | ||
} | ||
|
||
private String getRelativePath(final String absoluteUri) { |
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.
Is there a library that can handle this?
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.
Not one I could find at this time.
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.
maybe Path#relativize
private static ActiveEditorChangeListener instance; | ||
private static final long DEBOUNCE_DELAY_MS = 100L; | ||
private ScheduledFuture<?> debounceTask; | ||
private org.eclipse.ui.IWorkbenchWindow registeredWindow; |
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.
private org.eclipse.ui.IWorkbenchWindow registeredWindow; | |
private WorkbenchWindow registeredWindow; |
Description of changes:
This PR implements pinned context functionality for Amazon Q Eclipse plugin, allowing users to pin files
to their chat conversations for persistent context. This feature improves the relevance of Amazon Q
responses by maintaining important context throughout the conversation.
What does this PR do?
This change also remove the
isUriInWorkspace
check for when lsp sendsshowDocument
notification to client. This check would gate the ability to open documents not present in the workspace which is required for paths associated with prompts/rules are stored on disk and require opening it in the IDE for editingBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.