Update text chip to display label when no text is entered #286

jacobsimionato · 2025-09-08T03:58:35Z

E.g. if there is a "Destination" chip, it should say "Destination" and then if the user clicks on it and enters "New York", the chip should now say "New York" instead.

jacobsimionato · 2025-09-08T03:58:43Z

/gemini review

gemini-code-assist

Code Review

This pull request applies automated fixes and formatting. The main change refactors _TextInputChipState to remove local state in favor of a centralized values map, which is a good architectural improvement. However, I've identified a high-severity issue with an unsafe type cast that could lead to a runtime crash. My review includes a specific suggestion to address this. Additionally, as per the repository's style guide (lines 18-20), please remember to include the Pre-Review Checklist in your pull request descriptions.

gemini-code-assist · 2025-09-08T03:59:50Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

+    final currentValue = widget.values[widget.widgetId] as String?;
+    final text = currentValue ?? widget.label;


The direct cast as String? on line 88 is unsafe. Since widget.values is a Map<String, Object?>, the value for widget.widgetId could be a non-String type, which would cause a TypeError at runtime and crash the widget. It's safer to use a type check with is.

Suggested change

final currentValue = widget.values[widget.widgetId] as String?;

final text = currentValue ?? widget.label;

final value = widget.values[widget.widgetId];

final currentValue = value is String ? value : null;

final text = currentValue ?? widget.label;

gemini-code-assist

Code Review

This pull request refactors the _TextInputChip to remove local state and instead read its value from the widget.values map. This is a good step towards a single source of truth. However, the implementation has a couple of issues. First, there's an unsafe type cast that could lead to runtime errors. Second, the widget mutates the widget.values map directly, which is an anti-pattern in Flutter and can cause state inconsistencies. My review includes suggestions to address these points. Additionally, as per the repository's style guide, the pull request description should include the Pre-Review Checklist, which is currently missing.

gemini-code-assist · 2025-09-08T04:01:11Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

-                        setState(() {
-                          _currentValue = newValue;
-                        });
+                        setState(() {});


Calling setState here is necessary because widget.values was mutated directly on the previous line. While this works for this widget, directly mutating a widget property like values is an anti-pattern in Flutter and goes against common framework principles.¹ It can lead to inconsistent UI state if other widgets also depend on this values map but are not rebuilt.

A better approach is to use a callback to notify the parent widget of the change, and let the parent update the state. This is known as 'lifting state up' and leads to more predictable state management.

Style Guide References

Footnotes

↩

gemini-code-assist · 2025-09-08T04:01:11Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

    _textController.text = widget.initialValue ?? '';
  }

  @override
  Widget build(BuildContext context) {
+    final currentValue = widget.values[widget.widgetId] as String?;


The direct cast as String? can lead to a runtime exception if the value in widget.values for this widgetId is not a String or null. To make the code more robust, it's safer to perform a type check.

Suggested change

final currentValue = widget.values[widget.widgetId] as String?;

final rawValue = widget.values[widget.widgetId];

final currentValue = rawValue is String ? rawValue : null;

jacobsimionato · 2025-09-08T04:32:08Z

I have addressed the feedback. Please take another look.

jacobsimionato · 2025-09-08T04:32:14Z

/gemini review

gemini-code-assist

Code Review

This pull request updates the TextInputChip to display its label when no value has been entered, falling back to the entered value otherwise. The implementation correctly moves state management from a local state variable to the shared values map, which aligns better with the surrounding framework.

I've identified two high-severity issues:

The initialValue property is not handled correctly, causing it to be ignored.
Clearing the text input does not work as expected and leaves the user stuck in the modal.

Please see my detailed comments for suggestions on how to fix these issues.

gemini-code-assist · 2025-09-08T04:33:21Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

  void initState() {
    super.initState();
-    _currentValue = widget.initialValue ?? widget.label;
    _textController.text = widget.initialValue ?? '';
  }


The initialValue is not being handled correctly. Currently, it's set on _textController in initState, but this value is overwritten in onSelected before the text field is displayed. Furthermore, this doesn't make the initialValue appear on the chip label itself, as the build method only reads from widget.values.

To ensure initialValue is properly used as the default value, it should be used to populate widget.values when the widget is first initialized and no other value is present.

Suggested change

void initState() {

super.initState();

_currentValue = widget.initialValue ?? widget.label;

_textController.text = widget.initialValue ?? '';

}

void initState() {

super.initState();

if (widget.values[widget.widgetId] == null && widget.initialValue != null) {

widget.values[widget.widgetId] = widget.initialValue;

}

}

gemini-code-assist · 2025-09-08T04:33:21Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

                      if (newValue.isNotEmpty) {
                        widget.values[widget.widgetId] = newValue;
-                        setState(() {
-                          _currentValue = newValue;
-                        });
+                        setState(() {});
                        Navigator.pop(context);
                      }


The current logic prevents the user from clearing the input. If the user erases the text and presses 'Done', newValue.isNotEmpty is false, and the modal sheet doesn't close, which is a poor user experience.

To fix this, you should handle the case where the new value is empty by removing the corresponding entry from widget.values. The modal should always be closed when 'Done' is pressed.

if (newValue.isNotEmpty) { widget.values[widget.widgetId] = newValue; } else { widget.values.remove(widget.widgetId); } setState(() {}); Navigator.pop(context);

jacobsimionato · 2025-09-08T04:38:56Z

I have addressed the feedback. Please take another look.

jacobsimionato · 2025-09-08T04:39:02Z

/gemini review

gemini-code-assist

Code Review

This pull request successfully updates the TextInputChip to display its label when no text is entered, and the entered text otherwise. The refactoring to manage state in the parent values map is a good improvement. My review includes a critical fix for a memory leak and a suggestion to improve code style. Additionally, I've noted that adding tests for this updated component would be beneficial to align with the project's contribution guidelines (Style Guide, line 17: 'Code should be tested.').

gemini-code-assist · 2025-09-08T04:39:54Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

@@ -75,23 +75,27 @@ class _TextInputChip extends StatefulWidget {
 }

 class _TextInputChipState extends State<_TextInputChip> {
-  late String _currentValue;
  final TextEditingController _textController = TextEditingController();


The TextEditingController _textController is initialized here but never disposed of, which will cause a memory leak. You should override the dispose method in _TextInputChipState and call _textController.dispose() inside it.

For example:

@override void dispose() { _textController.dispose(); super.dispose(); }

gemini-code-assist · 2025-09-08T04:39:54Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

                      if (newValue.isNotEmpty) {
                        widget.values[widget.widgetId] = newValue;
-                        setState(() {
-                          _currentValue = newValue;
-                        });
-                        Navigator.pop(context);
+                      } else {
+                        widget.values.remove(widget.widgetId);
                      }
+                      setState(() {});


For better readability and to follow idiomatic Flutter code style, it's recommended to wrap state-mutating logic inside the setState call. This makes it clearer which changes are intended to trigger a UI rebuild.

Suggested change

if (newValue.isNotEmpty) {

widget.values[widget.widgetId] = newValue;

setState(() {

_currentValue = newValue;

});

Navigator.pop(context);

} else {

widget.values.remove(widget.widgetId);

}

setState(() {});

setState(() {

if (newValue.isNotEmpty) {

widget.values[widget.widgetId] = newValue;

} else {

widget.values.remove(widget.widgetId);

}

});

jacobsimionato · 2025-09-08T04:45:43Z

I have addressed the feedback. Please take another look.

jacobsimionato · 2025-09-08T04:45:49Z

/gemini review

gemini-code-assist

Code Review

This pull request updates the TextInputChip to display its label when no text is entered, and the entered text otherwise. The changes are well-implemented, improving state management by removing local state and relying on a shared values map. A potential memory leak is also fixed by correctly disposing of the TextEditingController. My review includes a suggestion to improve input handling and a recommendation to add tests to cover the new functionality, as per the repository's contribution guidelines.

gemini-code-assist · 2025-09-08T04:46:50Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

@@ -75,23 +75,33 @@ class _TextInputChip extends StatefulWidget {
 }

 class _TextInputChipState extends State<_TextInputChip> {


The changes look good and correctly implement the desired functionality. However, I noticed that tests for this new behavior are missing. The repository's style guide states that code should be tested.¹

Please consider adding a widget test for _TextInputChip to verify:

The chip displays the label when no value is provided.

The chip displays the initialValue when provided.

The chip displays the user-entered text after submission.

The chip reverts to displaying the label when the user clears the text.

Style Guide References

Footnotes

Code should be tested. (link) ↩

gemini-code-assist · 2025-09-08T04:46:50Z

examples/travel_app/lib/src/catalog/text_input_chip.dart

@@ -108,13 +118,14 @@ class _TextInputChipState extends State<_TextInputChip> {
                  ElevatedButton(
                    onPressed: () {
                      final newValue = _textController.text;


The new value from the text controller should be trimmed before checking if it's empty. This handles cases where a user might input only whitespace, which would currently result in an empty-looking chip instead of it reverting to the label text.

Suggested change

final newValue = _textController.text;

final newValue = _textController.text.trim();

Fix input chip

4553707

gemini-code-assist bot reviewed Sep 8, 2025

View reviewed changes

Remove .firebaserc

18b54fb

jacobsimionato changed the title ~~fix(chip): Apply automated fixes and formatting~~ Update text chip to display label when no text is entered Sep 8, 2025

fix: Address PR feedback

a16f421

gemini-code-assist bot reviewed Sep 8, 2025

View reviewed changes

fix: Address PR feedback

d534ebc

gemini-code-assist bot reviewed Sep 8, 2025

View reviewed changes

fix: Address PR feedback

d249f3e

gemini-code-assist bot reviewed Sep 8, 2025

View reviewed changes

		final currentValue = widget.values[widget.widgetId] as String?;
		final text = currentValue ?? widget.label;

	final currentValue = widget.values[widget.widgetId] as String?;
	final rawValue = widget.values[widget.widgetId];
	final currentValue = rawValue is String ? rawValue : null;

		@@ -75,23 +75,33 @@ class _TextInputChip extends StatefulWidget {
		}

		class _TextInputChipState extends State<_TextInputChip> {

	final newValue = _textController.text;
	final newValue = _textController.text.trim();

Update text chip to display label when no text is entered #286

Are you sure you want to change the base?

Update text chip to display label when no text is entered #286

Uh oh!

Conversation

jacobsimionato commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobsimionato commented Sep 8, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Style Guide References

Footnotes

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

jacobsimionato commented Sep 8, 2025

Uh oh!

jacobsimionato commented Sep 8, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

jacobsimionato commented Sep 8, 2025

Uh oh!

jacobsimionato commented Sep 8, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

jacobsimionato commented Sep 8, 2025

Uh oh!

jacobsimionato commented Sep 8, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Style Guide References

Footnotes

Uh oh!

gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacobsimionato commented Sep 8, 2025 •

edited

Loading