Add in Chat UI to Quickstart Playground #3913
Conversation
chenss3
changed the title
| /// <summary> | ||
| /// Converter to convert the PromptStatus bool value to a style that will be used by the GenerateButton. | ||
| /// </summary> | ||
| public class PromptStatusToGenerateButtonStyle : IValueConverter |
There was a problem hiding this comment.
This converter works on a bool and has domain knowledge and "PromptStyle" I suggest to rename the class to something generic BoolToAccentButtonStyle In case other parts of DevHome want to use the converter.
| namespace DevHome.Common.Environments.Converters; | ||
|
|
||
| /// <summary> | ||
| /// Converter to convert the PromptStatus bool value to a style that will be used by the GenerateButton. |
There was a problem hiding this comment.
Converts a PromptStatus... Devs know this is a converter.
There was a problem hiding this comment.
Same with the other converter.
| { | ||
| try | ||
| { | ||
| TelemetryFactory.Get<ITelemetry>().LogCritical("QuickstartPlaygroundCreateChatStyleGenerationOperation"); |
There was a problem hiding this comment.
Please use .Log as this allows you to add extra information.
| { | ||
| try | ||
| { | ||
| TelemetryFactory.Get<ITelemetry>().LogCritical("QuickstartPlaygroundCreateChatStyleGenerationOperation"); |
There was a problem hiding this comment.
Please follow the standard event naming convention. DevHome_[Area]_Event Maybe DevHome_GenerationOperation_Event
| catch (Exception ex) | ||
| { | ||
| TelemetryFactory.Get<ITelemetry>().LogException("QuickstartPlaygroundCreateChatStyleGenerationOperation", ex); | ||
| _log.Error(ex, $"CreateProjectGenerationOperation for: {this} failed due to exception"); |
There was a problem hiding this comment.
what will {this} print? The name of the class?
| @@ -33,7 +33,7 @@ public async Task<List<QuickStartProjectProvider>> GetQuickStartProjectProviders | |||
| { | |||
| try | |||
| { | |||
| var quickStartProjectProviders = (await extension.GetListOfProvidersAsync<IQuickStartProjectProvider>()).ToList(); | |||
| var quickStartProjectProviders = (await extension.GetListOfProvidersAsync<IQuickStartProjectProvider2>()).ToList(); | |||
There was a problem hiding this comment.
What if this releases but no extensions implement IQuickStartPRojectProvider2? GetListOfProvidersAsync will return 0 results. This is true even if some providers implement IQuickStartProjectProvider
| @@ -33,7 +33,7 @@ public async Task<List<QuickStartProjectProvider>> GetQuickStartProjectProviders | |||
| { | |||
| try | |||
| { | |||
| var quickStartProjectProviders = (await extension.GetListOfProvidersAsync<IQuickStartProjectProvider>()).ToList(); | |||
| var quickStartProjectProviders = (await extension.GetListOfProvidersAsync<IQuickStartProjectProvider2>()).ToList(); | |||
| foreach (var quickStartProjectProvider in quickStartProjectProviders) | |||
| { | |||
| if (quickStartProjectProvider != null) | |||
There was a problem hiding this comment.
Can GetListOfProvidersAsync return null values?`
| @@ -33,7 +33,7 @@ public async Task<List<QuickStartProjectProvider>> GetQuickStartProjectProviders | |||
| { | |||
| try | |||
| { | |||
| var quickStartProjectProviders = (await extension.GetListOfProvidersAsync<IQuickStartProjectProvider>()).ToList(); | |||
| var quickStartProjectProviders = (await extension.GetListOfProvidersAsync<IQuickStartProjectProvider2>()).ToList(); | |||
| foreach (var quickStartProjectProvider in quickStartProjectProviders) | |||
There was a problem hiding this comment.
quickStartPRojectProviders.Where(x => x != null);
| @@ -1919,6 +1919,14 @@ | |||
| <value>Generate project</value> | |||
| <comment>Tooltip for the generate button on the QuickstartPlayground page</comment> | |||
| </data> | |||
| <data name="QuickstartPlaygroundSubmitButton.Content" xml:space="preserve"> | |||
| <value>Submit</value> | |||
There was a problem hiding this comment.
Don't use submit Change the name to the action that will happen when the button is clicked.
| { | ||
| var message = CustomPrompt; | ||
|
|
||
| if (message != null && message != string.Empty) |
There was a problem hiding this comment.
!string.IsNullOrEmpty(message)
|
|
||
| TelemetryFactory.Get<ITelemetry>().Log("QuickstartPlaygroundGenerateButtonClicked", LogLevel.Critical, new GenerateButtonClicked(userPrompt), ActivityId); | ||
| // TODO: Replace this (and throughout) with proper diagnostics / logging |
There was a problem hiding this comment.
Will this TODO be addressed in a later PR?
| TelemetryFactory.Get<ITelemetry>().Log("QuickstartPlaygroundGenerateButtonClicked", LogLevel.Critical, new GenerateButtonClicked(userPrompt), ActivityId); | ||
| // TODO: Replace this (and throughout) with proper diagnostics / logging | ||
| ArgumentNullException.ThrowIfNullOrEmpty(userPrompt); | ||
| ArgumentNullException.ThrowIfNull(ActiveQuickstartSelection); |
There was a problem hiding this comment.
How can ActivateQuickStartSelection be null if the only way this code runs is when ActivateQuickStartSelection is not null?
|
|
||
| TelemetryFactory.Get<ITelemetry>().Log("QuickstartPlaygroundGenerateButtonClicked", LogLevel.Critical, new GenerateButtonClicked(userPrompt), ActivityId); | ||
| // TODO: Replace this (and throughout) with proper diagnostics / logging | ||
| ArgumentNullException.ThrowIfNullOrEmpty(userPrompt); |
There was a problem hiding this comment.
Same question. How can this be null or empty?
| // will contain any text from last-opened file in the previous project (which is | ||
| // confusing as this project may not have anything to do with the current one). This | ||
| // ensures that the file view is back to a known, clean state. | ||
| GeneratedFileContent = string.Empty; |
There was a problem hiding this comment.
The amount of comments for three statements is a lot. Can you reduce this comment, and the once above to one sentence each? An example would be Reset the file content so any previous content is not visible
| // TODO: Replace this (and throughout) with proper diagnostics / logging | ||
| ArgumentNullException.ThrowIfNullOrEmpty(userPrompt); | ||
| ArgumentNullException.ThrowIfNull(ActiveQuickstartSelection); | ||
| var userPrompt = CustomPrompt; |
There was a problem hiding this comment.
Why re-assign CustomPrompt?
| IProgress<QuickStartProjectProgress> progress = new Progress<QuickStartProjectProgress>(UpdateProgress); | ||
| // Temporarily turn off the provider combobox and ensure user cannot edit the prompt for the moment | ||
| EnableQuickstartProjectCombobox = false; | ||
| IsPromptTextBoxReadOnly = true; |
There was a problem hiding this comment.
Please stay consistent with what true means.
In this case false will disable two controls and true disables another. Consider renaming IsPromptTextBoxReadOnly in a way where false makes it readonly.
Considering all these controls need to be disabled. Can these three controls be controlled with 1 bool instead of three?
| SetUpFileView(); | ||
| SetupLaunchButton(); | ||
| EnableProjectButtons = true; | ||
| TelemetryFactory.Get<ITelemetry>().LogCritical("QuickstartPlaygroundGenerateSuccceded", relatedActivityId: ActivityId); |
There was a problem hiding this comment.
Please follow event naming conventions.
|
|
||
| _quickStartProjectGenerationOperation = ActiveQuickstartSelection.CreateProjectGenerationOperation(userPrompt, outputFolder, ActivityId); | ||
| _quickStartProject = await _quickStartProjectGenerationOperation.GenerateAsync().AsTask(progress); | ||
| _quickStartProjectGenerationOperation = null!; |
There was a problem hiding this comment.
null!? Why set this to null, then tell the compiler "Trust me. This isn't null."?
| else | ||
| { | ||
| IsErrorViewVisible = true; | ||
| ErrorMessage = StringResource.GetLocalized("QuickstartPlaygroundGenerationFailedDetails", _quickStartProject.Result.DisplayMessage); |
There was a problem hiding this comment.
Please follow event naming conventions.
| TelemetryFactory.Get<ITelemetry>().Log( | ||
| "QuickstartPlaygroundGenerateFailed", | ||
| LogLevel.Critical, | ||
| new ProjectGenerationErrorInfo(_quickStartProject.Result.DisplayMessage, _quickStartProject.Result.ExtendedError, _quickStartProject.Result.DiagnosticText), |
There was a problem hiding this comment.
NIT: pass in the exception and have the class break it apart into the data it needs.
| } | ||
| else | ||
| finally |
There was a problem hiding this comment.
Because the exception is not re-thrown the code after the try/catch will always execute. finally isn't needed.
| [RelayCommand] | ||
| public void CopyExamplePrompt(string selectedPrompt) | ||
| { | ||
| CustomPrompt = selectedPrompt; | ||
| } | ||
|
|
||
| public async Task GenerateChatStyleCompetions(string message) | ||
| { | ||
| if (ActiveQuickstartSelection != null) |
There was a problem hiding this comment.
NIT: Consider early returns over indentation.
| Type = ChatStyleMessage.ChatMessageItemType.Response, | ||
| }); | ||
|
|
||
| if (result.ChatResponse.ToString().Contains("great prompt")) |
There was a problem hiding this comment.
does great prompt need to be localized?
|
|
||
| if (result.ChatResponse.ToString().Contains("great prompt")) | ||
| { | ||
| IsPromptValid = true; |
There was a problem hiding this comment.
what about a great prompt makes the prompt valid? Additionally, I remember in the SDK you talking about what makes a valid prompt.
Who is responsible for determining if a prompt is valid? Extensions, or DevHome?
|
|
||
| ChatMessages.Add(new ChatStyleMessage | ||
| { | ||
| Name = "Hello! What kind of project would you like to create? Try a sample prompt or write one of your own.", |
|
|
||
| ChatMessages.Add(new ChatStyleMessage | ||
| { | ||
| Name = "If you would like to help us guide you to a good prompt, hit the Enter/Return key on your keyboard or press Submit after typing in your prompt. Otherwise, simply click on Generate to ignore prompt guidance.", |
There was a problem hiding this comment.
Localization.
I don't know if "simply" can be used. Some people might not find clicking on "generate" to be a simple task.
There was a problem hiding this comment.
NIT: "ignore" is a bit harsh. Otherwise, click on Generate seems good enough.
| @@ -699,3 +784,80 @@ public DataTemplate? FileTemplate | |||
| return explorerItem.Type == ExplorerItem.ExplorerItemType.Folder ? FolderTemplate : FileTemplate; | |||
| } | |||
| } | |||
|
|
|||
| public class ChatStyleMessage : INotifyPropertyChanged | |||
There was a problem hiding this comment.
ChatStyleMessage seems like a model. Not a view model.
There was a problem hiding this comment.
If this truly is a model. Please remove the observable collections.
| @@ -699,3 +784,80 @@ public DataTemplate? FileTemplate | |||
| return explorerItem.Type == ExplorerItem.ExplorerItemType.Folder ? FolderTemplate : FileTemplate; | |||
| } | |||
| } | |||
|
|
|||
| public class ChatStyleMessage : INotifyPropertyChanged | |||
There was a problem hiding this comment.
One class to one file please.
| <SolidColorBrush Color="WhiteSmoke" /> | ||
| </StackPanel.Background> | ||
| <Border Padding="8,4,8,4" | ||
| CornerRadius="4"> |
There was a problem hiding this comment.
A lot of spaces. Please format the XAML to the correct tab level.
| @@ -49,6 +50,82 @@ | |||
| <ResourceDictionary Source="ms-appx:///DevHome.SetupFlow/Styles/SetupFlowStyles.xaml" /> | |||
| <ResourceDictionary Source="ms-appx:///DevHome.SetupFlow/Styles/QuickstartStyles.xaml" /> | |||
| </ResourceDictionary.MergedDictionaries> | |||
| <DataTemplate x:Key="ChatResponseMessageTemplate" x:DataType="viewmodels:ChatStyleMessage"> | |||
| <Grid Height="Auto" | |||
There was a problem hiding this comment.
is the Height, Margin, and HorizontalAlignment a property of the template, or the parent controller? THese seems like the parent controller wants the margin of the template to be 4, and the height to be auto.
| @@ -49,6 +50,82 @@ | |||
| <ResourceDictionary Source="ms-appx:///DevHome.SetupFlow/Styles/SetupFlowStyles.xaml" /> | |||
| <ResourceDictionary Source="ms-appx:///DevHome.SetupFlow/Styles/QuickstartStyles.xaml" /> | |||
| </ResourceDictionary.MergedDictionaries> | |||
| <DataTemplate x:Key="ChatResponseMessageTemplate" x:DataType="viewmodels:ChatStyleMessage"> | |||
| <Grid Height="Auto" | |||
There was a problem hiding this comment.
Please use RowSpacing to control the space between items instead of using margin
| <RowDefinition Height="Auto" /> | ||
| <RowDefinition Height="Auto" /> | ||
| </Grid.RowDefinitions> | ||
| <StackPanel Grid.Row="1" |
There was a problem hiding this comment.
Grid.Row starts at 0. Why is the first item at 1?
| </Border.Background> | ||
| <StackPanel Orientation="Horizontal" | ||
| Spacing="8"> | ||
| <TextBlock MaxWidth="400" |
There was a problem hiding this comment.
Lemme get this straight.
A TextBlock inside of a StackPanel inside of a Border inside of a StackPanel inside of a grid. Why not a lone TextBlock?
| CornerRadius="4"> | ||
| <Border.Background> | ||
| <SolidColorBrush Opacity="0.2" | ||
| Color="WhiteSmoke" /> |
There was a problem hiding this comment.
Please us ThemeResource for opacity and Color so the UI can change to events. For example, switching from dark mode to light mode.
| </StackPanel> | ||
| </Grid> | ||
| </DataTemplate> | ||
| <DataTemplate x:Key="ChatRequestMessageTemplate" x:DataType="viewmodels:ChatStyleMessage"> |
There was a problem hiding this comment.
These look identical. What is the different between this, and ChatResponseMessageTemplate?
| @@ -100,6 +172,30 @@ | |||
| TabIndex="1"/> | |||
| </ctcontrols:WrapPanel> | |||
| </Grid> | |||
| <ScrollViewer | |||
| VerticalScrollBarVisibility="Auto" | |||
| Margin="10,0,10,10" | ||
| Padding="10" | ||
| Background="white" | ||
| MaxHeight="300"> |
There was a problem hiding this comment.
Why MaxHeigth? Something wrong with the control expanding with the UI?
| MinHeight="200" | ||
| ItemsSource="{x:Bind ViewModel.ChatMessages, Mode=OneWay}" | ||
| ItemTemplateSelector="{StaticResource ChatMessageTemplateSelector}" | ||
| IsItemClickEnabled="False" |
There was a problem hiding this comment.
I believe IsItemClickEnabled is false when SelectionMode is none, ya?
| @@ -129,8 +220,8 @@ | |||
| PlaceholderText="{x:Bind ViewModel.PromptTextBoxPlaceholder, Mode=OneWay}" | |||
| x:Name="CustomPrompt" | |||
| TextWrapping="Wrap" | |||
| AcceptsReturn="True" | |||
| MinWidth="800" | |||
| KeyDown="CustomPrompt_KeyDown" | |||
There was a problem hiding this comment.
Considering CustomPrompt_KeyDown calls a method on the ViewModel, consider using a `[RelayCommand] to bypass code-behind.
| @@ -168,10 +259,21 @@ | |||
| </StackPanel> | |||
| </StackPanel> | |||
| <StackPanel Grid.Column="1" Orientation="Horizontal" HorizontalAlignment="Right" Margin="0 0 10 0"> | |||
There was a problem hiding this comment.
Please use Spacing instead of margin to control the space between child elements.
|
|
||
| private async void CustomPrompt_KeyDown(object sender, KeyRoutedEventArgs e) | ||
| { | ||
| if (e.Key == VirtualKey.Enter) |
There was a problem hiding this comment.
Consider early return over indentation.
Summary of the pull request
#3908 with the SDK changes needs to be merged in first
This adds the UI changes required for the chat style portion of Quickstart Playground, where the user can chat with the UI to refine the prompt before they generate a codespace.
References and relevant issues
Detailed description of the pull request / Additional comments
Video demo:
## Validation steps performed
https://github.com/user-attachments/assets/a6753ad4-4cc6-4639-88d6-4dc00093deb8
PR checklist