Skip to content

Introduce region expansion feature #424

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 34 commits into
base: main
Choose a base branch
from
Open

Conversation

taldekar
Copy link
Contributor

@taldekar taldekar commented Apr 21, 2025

Description of changes:

  • Add profile selection page
  • Add profile selection action in drop downs
  • Add current selected profile notification
  • Enable Q developer profiles

Justification of changes:

Authentication and Profile Management

  • Added QDeveloperProfileState event to force chat refresh every time the user switches the profile.
  • Introduced QDeveloperProfileUtil to centralize profile-related functionality.
  • Implemented delayed auth state change event publishing using a CompletableFuture in DefaultAuthStateManager.
  • Added profile caching support for re-authentication on restart.
  • Implemented profile clearing on logout.
  • Added SSO token hydration with start URL as required for IAM login.

Browser Handling

  • Consolidated browser management in AmazonQBrowserProvider, which now handles creating the browser as well as maintaining state.
  • AmazonQView is now the base class for stateful, browser-based views.
  • Optimized BrowserCompatibilityState publishing (now only published once unless changes occur).

View updates

  • Made views static in AmazonQViewContainer to introduce caching and prevent new views from being created everytime.
  • Converted ToolkitLoginWebView to be stateful.
  • Added "Change profile" option to drow-downs.

Miscellaneous

  • Updated GetConfigurationServerParams to require passing in the expected response type to minimize programmer errors when using this lsp call.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

private QDeveloperProfile savedDeveloperProfile;
private QDeveloperProfile selectedDeveloperProfile;
private CompletableFuture<Void> profileSelectionTask;
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
Copy link
Contributor

Choose a reason for hiding this comment

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

ObjectMapperFactory

try {
return OBJECT_MAPPER.writeValueAsString(developerProfile);
} catch (JsonProcessingException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would at least need to go to our log - but these methods should probably bubble their exceptions up so they can be handled in the higher level workflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Will make the change.

Comment on lines 185 to 188
String section = "aws.q";
Map<String, Object> settings = Map.of("profileArn", selectedDeveloperProfile.getArn());
Activator.getLspProvider().getAmazonQServer()
.thenCompose(server -> server.updateConfiguration(new UpdateConfigurationParams(section, settings)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this and other configuration update logic should live in a specific component. The CustomizationUtil originally served that purpose but we've outgrown it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see, I'll look into this.


public class QLspTypeAdapterFactory implements TypeAdapterFactory {

@Override
@SuppressWarnings("unchecked")
public final <T> TypeAdapter<T> create(final Gson gson, final TypeToken<T> type) {
if (type.getRawType() == LspServerConfigurations.class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a specialized type adapter for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was required because the getConfigurationFromServer call could return either a customization or profiles, and GSON wasn't able to handle the generics out of the box.

Copy link
Contributor

@breedloj breedloj Apr 23, 2025

Choose a reason for hiding this comment

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

I see..that's unfortunate.

@@ -32,7 +32,7 @@ private LspConstants() {

private static VersionRange createVersionRange() {
try {
return VersionRange.createFromVersionSpec("[3.1.2, 3.10.0)");
return VersionRange.createFromVersionSpec("[3.1.100, 3.4.0)");
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just for testing. Will narrow this down before merging, once I know the actual version spec.

@@ -222,8 +222,7 @@ private String generateJS(final String jsEntrypoint) {
quickActionCommands: %s,
disclaimerAcknowledged: %b
});
})
.catch(error => console.error('Error initializing chat:', error));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we dropped this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dang. This is a regression. Wasn't intentional. I'll revert this.

Comment on lines 1 to 4
activeProfiles=
eclipse.preferences.version=1
resolveWorkspaceProjects=true
version=1
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the impact of checking this in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a maven file. Wasn't sure if this was necessitated by other updates. I'll remove this.

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'll update the .gitignore as well.

Comment on lines 35 to 36
private static final String WINDOW_TITLE = "Amazon Q Developer Profile";
private static final String HEADER = "Change your Q Developer profile";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's keep the casing consistent whenever we reference "Developer Profile"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -4,8 +4,7 @@
<!-- This Vue File is the login webview of AWS Toolkit and Amazon Q.-->
<template>
<div v-bind:class="[disabled ? 'disabled-form' : '']" class="auth-container centered-with-max-width">
<button v-if="(stage !== 'START' || cancellable) && stage !== 'AUTHENTICATING'" class="back-button" @click="handleBackButtonClick" tabindex="-1">
<svg width="24" height="24" viewBox="0 -3 13 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<button v-if="!['START', 'AUTHENTICATING', 'PROFILE_SELECT'].includes(stage) || cancellable" class="back-button" @click="handleBackButtonClick" tabindex="-1"> <svg width="24" height="24" viewBox="0 -3 13 16" fill="none" xmlns="http://www.w3.org/2000/svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this was intentional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional, but fixed the formatting issue.

@@ -74,11 +91,11 @@ export default defineComponent({
if (this.cancellable) {
window.ideApi.postMessage({command: 'toggleBrowser'})
}
window.telemetryApi.postClickEvent("backButton")
window.telemetryApi.postClickEvent("backButton")
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing a lot of new tab characters in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is properly formatted on my machine. I believe Github is glitching with the webview files.

taldekar and others added 7 commits April 29, 2025 11:45
* Refactor to improve code quality

* Improve user experience when changing profiles

* Remove unnecessary getProfiles call

* Preempt potential bug

* Preempt potential bug

* Add down arrow when scrollable

* Add down arrow when scrollable

* Fix checkstyle and minor QDeveloperProfileUtil interface update

* Fix fetch profile loading indicator indent
Co-authored-by: Jonathan Breedlove <[email protected]>
Co-authored-by: Ishan Taldekar <[email protected]>
Merge main into feature/regionExpansion
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