Alternative UI examples | Side Chat, No Chat & Inline Chat (default) #297
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that introduces alternative UI examples (Side Chat, No Chat, Inline Chat) for the travel app. The core logic has been cleanly extracted into a TravelPlannerCanvasController, making the UI widgets reusable and focused. The addition of new tests for each component is also a great practice. I've found a critical compilation error and a couple of high-severity bugs related to state management and message ordering. I've also included some medium-severity suggestions to improve code quality, layout robustness, and test reliability. Overall, this is a great step forward for the example app.
| // Always dispose controller since we always create it | ||
| _controller.dispose(); | ||
|
|
There was a problem hiding this comment.
The _controller is disposed unconditionally. If the controller is passed in via the constructor (e.g., for testing), this widget does not own it and should not dispose it, as it may be in use elsewhere. This can lead to runtime errors.
The widget should only dispose of the controller if it created it. You can check if widget.controller was null, which is the same condition used for creating the controller in initState.
This same issue exists in no_chat_travel_planner.dart and side_chat_travel_planner.dart.
| // Always dispose controller since we always create it | |
| _controller.dispose(); | |
| // Only dispose the controller if this widget created it. | |
| if (widget.controller == null) { | |
| _controller.dispose(); | |
| } |
There was a problem hiding this comment.
_controller will be created either way, if its passed through contructor otherwise a default Controller will be initalized:
_controller =
widget.controller ??
TravelPlannerCanvasController(
enableChatOutput: true,
aiClient: widget.aiClient,
);
examples/travel_app/lib/src/chats/inline_chat_travel_planner.dart
Outdated
Show resolved
Hide resolved
| : Padding( | ||
| padding: const EdgeInsets.only(bottom: 40), | ||
| child: Conversation( | ||
| messages: _surfaces, | ||
| manager: _controller.genUiManager, | ||
| scrollController: _uiScrollController, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Using Padding with a hardcoded value (40) to avoid the floating ChatInput from overlapping the content is fragile and uses a "magic number". This is especially true since the padding for the chat pane on line 306 is a different value (60), leading to inconsistency.
A more robust layout would be to use a Column for the screen's body, with the Row of panes as an Expanded child and the ChatInput as the final child. This would allow Flutter to handle the layout automatically without needing manual padding adjustments.
If the Stack layout is required, these padding values should be consolidated into a single constant to ensure consistency.
| // ignore: inference_failure_on_instance_creation | ||
| await Future.delayed(const Duration(milliseconds: 50)); |
There was a problem hiding this comment.
Using Future.delayed to wait for async operations in tests can lead to flakiness, as the fixed duration is arbitrary and might not be long enough on a slow machine, causing the test to fail intermittently.
A more reliable approach is to await a signal that the async work is complete. You can use the isThinking stream for this:
controller.sendUserTextMessage('Hello');
await controller.isThinking.firstWhere((thinking) => thinking); // Wait for thinking to start.
await controller.isThinking.firstWhere((thinking) => !thinking); // Wait for thinking to end.
expect(textMessageCount, 2);This ensures the test waits exactly as long as needed. This also applies to the test on line 83.
…ive-ui-examples
|
Hi Hamza! These demos look great! I'm actually also iterating on this issue based on some internal requirements, and I'm not sure the best way to merge our efforts yet. Your work here is more polished than mine in some ways, but mine handles some other requirements coming from within Google and I'm not sure the best way to merge them yet. In particular, I want to keep the behavior of the inline UI layout exactly the same as it has been rather than adopting the TravelPlannerCanvasController constraints, like max 4 surfaces, because that inline UI is our main demo right now. If it's okay with you, I think I'll keep iterating on my branch, but fold in a few of your changes here when I can. Again, thank you for your high quality work here and your engagement with this project! If you're eager to get involved, I could look for some other issues that we're not working on right now, to avoid duplicating efforts? |
Hi, no worries at all. I'm litterally here for my own learnings. Even I won't mind getting this PR closed. I'm happy for the feedbacks. As of keeping the inline chat UI separate, I was on the same point as yours here. But then I thought why I'm not using the TravelPlannerCanvasController. All in all, I'm 100% okay with the team's priorities. I'm just roaming around and jotting down little stuff that I can contribute too. |
Description
Pre-launch Checklist
///).Results
Side Chat
This works best in horizontal orientation of device in mobile view.
Screen.Recording.2025-09-11.at.4.20.58.PM.mov
No Chat
Screen.Recording.2025-09-11.at.4.22.00.PM.mov