-
Notifications
You must be signed in to change notification settings - Fork 114
Refactor generic_chat to separate code destined for SDK from main etc. #15
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
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.
Code Review
This pull request successfully refactors the generic_chat example by introducing a WidgetTreeLlmAdapter to encapsulate conversation management and LLM interaction logic. This significantly cleans up main.dart, leaving it with just configuration and setup responsibilities. The introduction of an LlmConnection interface is a great step towards better abstraction and testability.
My review includes suggestions to improve code clarity, maintainability, and error handling:
- Standardizing indentation in the system prompt.
- Reducing code duplication in
ConversationWidget. - Improving error handling by using
debugPrintoverprintand providing user feedback for errors. - Removing dead code from a
StreamBuilder.
| final systemPrompt = | ||
| '''You are a helpful assistant who figures out what the user wants to do and then helps suggest options so they can develop a plan and find relevant information. | ||
| The user will ask questions, and you will respond by generating appropriate UI elements. Typically, you will first elicit more information to understand the user's needs, then you will start displaying information and the user's plans. | ||
| For example, the user may say "I want to plan a trip to Mexico". You will first ask some questions by displaying a combination of UI elements, such as a slider to choose budget, options showing activity preferences etc. Then you will walk the user through choosing a hotel, flight and accomodation. | ||
| Typically, you should not update existing surfaces and instead just continually "add" new ones. | ||
| '''; |
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.
The indentation within the systemPrompt multiline string is inconsistent. The first paragraph is not indented, while the subsequent paragraphs have significant leading whitespace. This could affect how the LLM interprets the prompt and also makes the code harder to read.
For consistency and predictability, it's best to have uniform indentation for all paragraphs.
final systemPrompt =
'''You are a helpful assistant who figures out what the user wants to do and then helps suggest options so they can develop a plan and find relevant information.
The user will ask questions, and you will respond by generating appropriate UI elements. Typically, you will first elicit more information to understand the user's needs, then you will start displaying information and the user's plans.
For example, the user may say \"I want to plan a trip to Mexico\". You will first ask some questions by displaying a combination of UI elements, such as a slider to choose budget, options showing activity preferences etc. Then you will walk the user through choosing a hotel, flight and accomodation.
Typically, you should not update existing surfaces and instead just continually \"add\" new ones.
''';
| return switch (message) { | ||
| SystemMessage() => Card( | ||
| elevation: 2.0, | ||
| margin: | ||
| const EdgeInsets.symmetric(horizontal: 8.0, vertical: 4.0), | ||
| child: ListTile( | ||
| title: Text(message.text), | ||
| leading: const Icon(Icons.smart_toy_outlined), | ||
| ), | ||
| ), | ||
| TextResponse() => Card( | ||
| elevation: 2.0, | ||
| margin: | ||
| const EdgeInsets.symmetric(horizontal: 8.0, vertical: 4.0), | ||
| child: ListTile( | ||
| title: Text(message.text), | ||
| leading: const Icon(Icons.smart_toy_outlined), | ||
| ), | ||
| ), | ||
| UserPrompt() => Card( | ||
| elevation: 2.0, | ||
| margin: | ||
| const EdgeInsets.symmetric(horizontal: 8.0, vertical: 4.0), | ||
| child: ListTile( | ||
| title: Text( | ||
| message.text, | ||
| textAlign: TextAlign.right, | ||
| ), | ||
| trailing: const Icon(Icons.person), | ||
| ), | ||
| ), | ||
| UiResponse() => Card( | ||
| elevation: 2.0, | ||
| margin: | ||
| const EdgeInsets.symmetric(horizontal: 8.0, vertical: 4.0), | ||
| child: Padding( | ||
| padding: const EdgeInsets.all(16.0), | ||
| child: DynamicUi( | ||
| key: message.uiKey, | ||
| catalog: catalog, | ||
| surfaceId: message.surfaceId, | ||
| definition: UiDefinition.fromMap(message.definition), | ||
| onEvent: onEvent, | ||
| ), | ||
| ), | ||
| ), | ||
| }; |
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.
There's repeated code for styling the Card widgets within the switch statement. Each case (SystemMessage, TextResponse, UserPrompt, UiResponse) defines a Card with the same elevation and margin.
To improve maintainability and reduce duplication, you could extract this into a shared private widget.
For example:
class _MessageCard extends StatelessWidget {
const _MessageCard({required this.child});
final Widget child;
@override
Widget build(BuildContext context) {
return Card(
elevation: 2.0,
margin: const EdgeInsets.symmetric(horizontal: 8.0, vertical: 4.0),
child: child,
);
}
}
// Then in your switch:
return switch (message) {
SystemMessage() => _MessageCard(
child: ListTile(
title: Text(message.text),
leading: const Icon(Icons.smart_toy_outlined),
),
),
// ... and so on for other cases
}| // TODO: Handle error - unknown surfaceId | ||
| print('Unknown surfaceId: $surfaceId'); | ||
| continue; |
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.
The TODO comment correctly identifies that this error handling can be improved. Using print() is not ideal for logging in Flutter apps; debugPrint() is preferred as it avoids being throttled on Android.
Additionally, this error should be surfaced to the user, for instance by adding a SystemMessage to the chat history, similar to how other errors are handled in _generateAndSendResponse.
// TODO: Handle error - unknown surfaceId
debugPrint('Unknown surfaceId: $surfaceId');
_chatHistory.add(SystemMessage(text: 'Error: Unknown surfaceId: $surfaceId'));
_uiDataStreamController.add(List.from(_chatHistory));
continue;
| _uiDataStreamController.add(List.from(_chatHistory)); | ||
| } catch (e) { | ||
| // TODO: better error handling | ||
| print('Error generating content: $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.
The TODO here indicates that error handling can be improved. While adding a SystemMessage to the chat is great for user feedback, using print() for logging is not ideal. debugPrint() is a better choice in Flutter as it's not throttled on Android. For more robust logging, consider integrating a logging package.
debugPrint('Error generating content: $e');
| return StreamBuilder( | ||
| stream: uiDataStream, | ||
| initialData: const <ChatMessage>[], | ||
| builder: (context, snapshot) { | ||
| if (snapshot.hasData) { | ||
| return ConversationWidget( | ||
| messages: snapshot.data!, | ||
| catalog: catalog, | ||
| onEvent: (event) { | ||
| _eventDebouncer.add(UiEvent.fromMap(event)); | ||
| }); | ||
| } else { | ||
| return const Center(child: CircularProgressIndicator()); | ||
| } | ||
| }); |
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.
The else block in this StreamBuilder is unreachable. Because you provide non-null initialData, snapshot.hasData will be true from the very first build, so the else branch containing the CircularProgressIndicator will never be executed. You can simplify this by removing the conditional logic.
// Since `initialData` is provided, `snapshot.data` will not be null,
// and `snapshot.hasData` will always be true.
return ConversationWidget(
messages: snapshot.data!,
catalog: catalog,
onEvent: (event) {
_eventDebouncer.add(UiEvent.fromMap(event));
});
Add WidgetTreeLlmAdapter which contains all the code to manage a conversation. This leaves main.dart to simply configure the widget library and define the system prompt.