Skip to content

Commit cf3e734

Browse files
committed
fix(drink-detail): harden inline note autosave and undo flows
Review fixes on top of the inline-capture implementation: - Skip tasting analytics when addTasting restores a pour via Undo, matching removeTasting's removal-conditioned logging - Flush a cleared note on dispose (pending-edit flag instead of a text cache that treated empty as nothing-to-save) - Render the optimistic note value in display mode so the just-typed text does not flash back while the save is in flight - Keep a failed note save pending for retry instead of reporting it as saved - Extract the shared Undo SnackBar builder in the detail screen - Add a semantic test for the inline field's text-field node Refs #487
1 parent 05438f3 commit cf3e734

5 files changed

Lines changed: 193 additions & 46 deletions

File tree

lib/providers/beer_provider.dart

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -807,7 +807,9 @@ class BeerProvider extends ChangeNotifier {
807807
/// Record a new tasting event for a drink, returning the timestamp of the
808808
/// pour that was logged. The provider owns the timestamp (rather than letting
809809
/// the store generate it) so callers can offer a precise Undo that removes
810-
/// exactly this pour, not merely the newest one.
810+
/// exactly this pour, not merely the newest one. Pass [at] to restore a
811+
/// previously removed pour with its original timestamp (an Undo), which is
812+
/// not counted as a new tasting in analytics.
811813
Future<DateTime> addTasting(Drink drink, {DateTime? at}) async {
812814
final event = at ?? DateTime.now();
813815
if (_drinkRepository == null) return event;
@@ -824,11 +826,17 @@ class BeerProvider extends ChangeNotifier {
824826

825827
notifyListeners();
826828

827-
// Log analytics event
828-
unawaited(_analyticsService.logFestivalLogMarkTasted(drink));
829-
final count = newState?.tastingCount ?? 0;
830-
if (count > 1) {
831-
unawaited(_analyticsService.logFestivalLogMultipleTasting(drink, count));
829+
// Only a genuinely new pour is an analytics event — restoring a deleted
830+
// one via Undo (at != null) would double-count tastings that already
831+
// happened.
832+
if (at == null) {
833+
unawaited(_analyticsService.logFestivalLogMarkTasted(drink));
834+
final count = newState?.tastingCount ?? 0;
835+
if (count > 1) {
836+
unawaited(
837+
_analyticsService.logFestivalLogMultipleTasting(drink, count),
838+
);
839+
}
832840
}
833841
return event;
834842
}

lib/screens/drink_detail_screen.dart

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,20 @@ class _DrinkDetailScreenState extends State<DrinkDetailScreen>
108108
? 'Logged your first tasting'
109109
: 'Logged — $count tastings';
110110

111+
_showUndoSnackBar(
112+
messenger,
113+
message: message,
114+
onUndo: () => unawaited(provider.removeTasting(updated, event)),
115+
);
116+
}
117+
118+
/// The shared confirmation shape for reversible tasting mutations: a
119+
/// floating SnackBar whose message dismisses on tap, with an Undo action.
120+
void _showUndoSnackBar(
121+
ScaffoldMessengerState messenger, {
122+
required String message,
123+
required VoidCallback onUndo,
124+
}) {
111125
messenger
112126
..hideCurrentSnackBar()
113127
..showSnackBar(
@@ -128,10 +142,7 @@ class _DrinkDetailScreenState extends State<DrinkDetailScreen>
128142
child: Text(message),
129143
),
130144
),
131-
action: SnackBarAction(
132-
label: 'Undo',
133-
onPressed: () => unawaited(provider.removeTasting(updated, event)),
134-
),
145+
action: SnackBarAction(label: 'Undo', onPressed: onUndo),
135146
),
136147
);
137148
}
@@ -332,30 +343,11 @@ class _DrinkDetailScreenState extends State<DrinkDetailScreen>
332343
await provider.removeTasting(drink, event);
333344
if (!mounted || messenger == null) return;
334345

335-
final message = 'Removed — ${_tastingRowFormat.format(event)}';
336-
337-
messenger
338-
..hideCurrentSnackBar()
339-
..showSnackBar(
340-
SnackBar(
341-
behavior: SnackBarBehavior.floating,
342-
margin: const EdgeInsets.only(bottom: 12, left: 16, right: 16),
343-
duration: const Duration(seconds: 3),
344-
content: Semantics(
345-
label: message,
346-
button: true,
347-
hint: 'Double tap to dismiss',
348-
child: GestureDetector(
349-
onTap: messenger.hideCurrentSnackBar,
350-
child: Text(message),
351-
),
352-
),
353-
action: SnackBarAction(
354-
label: 'Undo',
355-
onPressed: () => unawaited(provider.addTasting(drink, at: event)),
356-
),
357-
),
358-
);
346+
_showUndoSnackBar(
347+
messenger,
348+
message: 'Removed — ${_tastingRowFormat.format(event)}',
349+
onUndo: () => unawaited(provider.addTasting(drink, at: event)),
350+
);
359351
}
360352

361353
List<Widget> _buildSimilarDrinksSlivers(

lib/widgets/your_take_card.dart

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ class _YourTakeCardState extends State<YourTakeCard> {
5757
bool _isEditing = false;
5858
bool _showSaved = false;
5959
String? _lastSavedNotes;
60-
String? _pendingNotes;
60+
bool _hasPendingEdit = false;
6161

6262
@override
6363
void initState() {
@@ -82,10 +82,12 @@ class _YourTakeCardState extends State<YourTakeCard> {
8282
void dispose() {
8383
_debounceTimer?.cancel();
8484
_savedIndicatorTimer?.cancel();
85-
// Best-effort flush: if there's a debounced save that hasn't landed yet,
86-
// fire it now rather than silently dropping the user's last edit.
87-
if (_pendingNotes != null && _pendingNotes != _lastSavedNotes) {
88-
unawaited(widget.onNotesChanged(_pendingNotes));
85+
// Best-effort flush: if there's an edit whose save hasn't landed yet
86+
// (including clearing the note to empty), fire it now rather than
87+
// silently dropping the user's last edit.
88+
final normalized = _normalizedControllerText;
89+
if (_hasPendingEdit && normalized != _lastSavedNotes) {
90+
unawaited(widget.onNotesChanged(normalized));
8991
}
9092
_notesFocusNode
9193
..removeListener(_handleFocusChange)
@@ -94,6 +96,12 @@ class _YourTakeCardState extends State<YourTakeCard> {
9496
super.dispose();
9597
}
9698

99+
/// The controller text as it would be persisted: trimmed, empty → null.
100+
String? get _normalizedControllerText {
101+
final trimmed = _notesController.text.trim();
102+
return trimmed.isEmpty ? null : trimmed;
103+
}
104+
97105
void _handleFocusChange() {
98106
if (!_notesFocusNode.hasFocus && _isEditing) {
99107
unawaited(_flushSave());
@@ -107,8 +115,7 @@ class _YourTakeCardState extends State<YourTakeCard> {
107115
}
108116

109117
void _onFieldChanged(String value) {
110-
final trimmed = value.trim();
111-
_pendingNotes = trimmed.isEmpty ? null : trimmed;
118+
_hasPendingEdit = true;
112119
_debounceTimer?.cancel();
113120
_debounceTimer = Timer(
114121
YourTakeCard.notesDebounceDuration,
@@ -119,12 +126,20 @@ class _YourTakeCardState extends State<YourTakeCard> {
119126
Future<void> _flushSave() async {
120127
_debounceTimer?.cancel();
121128
_debounceTimer = null;
122-
final trimmed = _notesController.text.trim();
123-
final normalized = trimmed.isEmpty ? null : trimmed;
124-
_pendingNotes = null;
129+
final normalized = _normalizedControllerText;
130+
_hasPendingEdit = false;
125131
if (normalized == _lastSavedNotes) return;
132+
final previous = _lastSavedNotes;
126133
_lastSavedNotes = normalized;
127-
await widget.onNotesChanged(normalized);
134+
try {
135+
await widget.onNotesChanged(normalized);
136+
} catch (_) {
137+
// The write failed — keep the edit pending so the next flush (debounce,
138+
// blur, or dispose) retries it, and don't claim "Saved".
139+
_lastSavedNotes = previous;
140+
_hasPendingEdit = true;
141+
return;
142+
}
128143
if (!mounted) return;
129144
_savedIndicatorTimer?.cancel();
130145
setState(() => _showSaved = true);
@@ -296,7 +311,11 @@ class _YourTakeCardState extends State<YourTakeCard> {
296311
);
297312
}
298313

299-
final notes = widget.drink.userNotes;
314+
// Render the optimistic local value, not widget.drink.userNotes — on blur
315+
// the card returns to display mode before the async save (and the
316+
// provider rebuild it triggers) has landed, and the note the user just
317+
// typed must not flash back to its previous state in that window.
318+
final notes = _lastSavedNotes;
300319
final hasNotes = notes != null && notes.isNotEmpty;
301320

302321
return Semantics(

test/beer_provider_test.dart

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3018,6 +3018,12 @@ void main() {
30183018
verify(
30193019
mockDrinkRepository.addTasting(any, any, now: original),
30203020
).called(1);
3021+
// Restoring a removed pour is not a new tasting — it must not
3022+
// inflate the tasting analytics.
3023+
verifyNever(mockAnalyticsService.logFestivalLogMarkTasted(any));
3024+
verifyNever(
3025+
mockAnalyticsService.logFestivalLogMultipleTasting(any, any),
3026+
);
30213027
},
30223028
);
30233029

test/widgets/your_take_card_test.dart

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import 'dart:async';
2+
13
import 'package:flutter/material.dart';
24
import 'package:flutter/semantics.dart';
35
import 'package:flutter_test/flutter_test.dart';
@@ -170,6 +172,110 @@ void main() {
170172
expect(calls, ['Quick save']);
171173
});
172174

175+
testWidgets(
176+
'clearing an existing note is persisted even when the widget is '
177+
'disposed before the debounce fires',
178+
(tester) async {
179+
final calls = <String?>[];
180+
await tester.pumpWidget(
181+
buildWidget(
182+
drink: createSampleDrink(notes: 'Existing'),
183+
onNotesChanged: (notes) async => calls.add(notes),
184+
),
185+
);
186+
187+
await tester.tap(find.byKey(const ValueKey('user-notes-editor')));
188+
await tester.pump();
189+
await tester.enterText(
190+
find.byKey(const ValueKey('user-notes-field')),
191+
'',
192+
);
193+
194+
// Dispose the card before the debounce window elapses — the cleared
195+
// note must still be flushed exactly once, not silently dropped.
196+
await tester.pumpWidget(const MaterialApp(home: SizedBox.shrink()));
197+
198+
expect(calls, [null]);
199+
},
200+
);
201+
202+
testWidgets('a failed save is not reported as Saved and is retried', (
203+
tester,
204+
) async {
205+
var shouldFail = true;
206+
var attempts = 0;
207+
final saved = <String?>[];
208+
await tester.pumpWidget(
209+
buildWidget(
210+
onNotesChanged: (notes) async {
211+
attempts++;
212+
if (shouldFail) throw Exception('write failed');
213+
saved.add(notes);
214+
},
215+
),
216+
);
217+
218+
await tester.tap(find.byKey(const ValueKey('user-notes-editor')));
219+
await tester.pump();
220+
await tester.enterText(
221+
find.byKey(const ValueKey('user-notes-field')),
222+
'Nice IPA',
223+
);
224+
await tester.pump(
225+
YourTakeCard.notesDebounceDuration + const Duration(milliseconds: 50),
226+
);
227+
await tester.pump();
228+
229+
expect(attempts, 1);
230+
expect(find.text('Saved'), findsNothing);
231+
232+
// The edit stays pending: the next keystroke's flush retries it.
233+
shouldFail = false;
234+
await tester.enterText(
235+
find.byKey(const ValueKey('user-notes-field')),
236+
'Nice IPA indeed',
237+
);
238+
await tester.pump(
239+
YourTakeCard.notesDebounceDuration + const Duration(milliseconds: 50),
240+
);
241+
await tester.pump();
242+
243+
expect(saved, ['Nice IPA indeed']);
244+
expect(find.text('Saved'), findsOneWidget);
245+
246+
await tester.pump(YourTakeCard.savedIndicatorDuration);
247+
});
248+
249+
testWidgets(
250+
'the note just typed stays visible after blur while the save is '
251+
'still in flight',
252+
(tester) async {
253+
final completer = Completer<void>();
254+
await tester.pumpWidget(
255+
buildWidget(onNotesChanged: (_) => completer.future),
256+
);
257+
258+
await tester.tap(find.byKey(const ValueKey('user-notes-editor')));
259+
await tester.pump();
260+
await tester.enterText(
261+
find.byKey(const ValueKey('user-notes-field')),
262+
'Great beer',
263+
);
264+
FocusManager.instance.primaryFocus?.unfocus();
265+
await tester.pump();
266+
267+
// Back in display mode with the save unresolved — the new text must
268+
// not flash back to the placeholder while the write is in flight.
269+
expect(find.byKey(const ValueKey('user-notes-field')), findsNothing);
270+
expect(find.text('Great beer'), findsOneWidget);
271+
expect(find.text('Tap to add your notes'), findsNothing);
272+
273+
completer.complete();
274+
await tester.pump();
275+
await tester.pump(YourTakeCard.savedIndicatorDuration);
276+
},
277+
);
278+
173279
testWidgets('the Saved indicator appears after a save and clears itself', (
174280
tester,
175281
) async {
@@ -230,6 +336,22 @@ void main() {
230336
);
231337
});
232338

339+
testWidgets('the inline note field exposes a text-field semantics node', (
340+
tester,
341+
) async {
342+
final handle = tester.ensureSemantics();
343+
try {
344+
await tester.pumpWidget(buildWidget());
345+
346+
await tester.tap(find.byKey(const ValueKey('user-notes-editor')));
347+
await tester.pump();
348+
349+
expect(find.semantics.byFlag(SemanticsFlag.isTextField), findsOne);
350+
} finally {
351+
handle.dispose();
352+
}
353+
});
354+
233355
testWidgets('the Saved indicator is announced as a live region', (
234356
tester,
235357
) async {

0 commit comments

Comments
 (0)