From d478fc5d1eb8c7bc5d9e03e098de658a24310de2 Mon Sep 17 00:00:00 2001 From: Isaac Banner Date: Mon, 20 Jul 2026 17:34:14 -0700 Subject: [PATCH 1/3] feat(collection): batch set/clear dance level in multi-select (#409) Adds a 'Set level' action to the Collection multi-select bar, letting a caller apply (or clear) a difficulty level across many selected dances at once, parallel to the existing batch tag add/remove flow. Core gains DanceRepository.setLevelForMany, a single-transaction batched write that reuses the existing upsert path, skips unknown ids and dances already at the target level (idempotent), and returns the changed count so an error leaves the collection untouched rather than half-updated. The app-side flow captures prior per-dance levels for a snackbar Undo, announces the result to assistive tech, and treats an empty selection or a no-op change accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- app/lib/src/screens/dance_list_screen.dart | 96 +++++++++ app/lib/src/widgets/batch_level_dialog.dart | 113 ++++++++++ app/test/screens/batch_level_test.dart | 200 ++++++++++++++++++ .../repositories/dance_repository.dart | 42 ++++ .../test/storage/dance_batch_level_test.dart | 123 +++++++++++ 5 files changed, 574 insertions(+) create mode 100644 app/lib/src/widgets/batch_level_dialog.dart create mode 100644 app/test/screens/batch_level_test.dart create mode 100644 packages/compendium_core/test/storage/dance_batch_level_test.dart diff --git a/app/lib/src/screens/dance_list_screen.dart b/app/lib/src/screens/dance_list_screen.dart index 2ff2675a..c3f5c12a 100644 --- a/app/lib/src/screens/dance_list_screen.dart +++ b/app/lib/src/screens/dance_list_screen.dart @@ -24,6 +24,7 @@ import '../theme/keyboard_dismiss.dart'; import '../utils/confirm_delete.dart'; import '../widgets/add_to_program_sheet.dart'; import '../widgets/advanced_query_builder.dart'; +import '../widgets/batch_level_dialog.dart'; import '../widgets/batch_tag_dialog.dart'; import '../widgets/brand_mark.dart'; import '../widgets/by_phrase_panel.dart'; @@ -822,6 +823,95 @@ class _DanceListScreenState extends State { if (mounted) await _boot(); } + /// Sets the difficulty level on the selected dances. Opens the level picker, + /// persists the choice via the batched, single-transaction + /// [DanceRepository.setLevelForMany], announces the result to AT, and offers + /// Undo. Unlike batch tagging (additive), setting a level replaces it, so the + /// picker is single-choice and includes an explicit "clear" option. + Future _batchSetLevel() async { + final data = _data; + if (data == null || _selectedIds.isEmpty) return; + + final selectedIds = Set.of(_selectedIds); + final choice = await showBatchLevelDialog(context); + if (choice == null || !mounted) return; + + // Capture prior levels so Undo can restore each dance individually (the + // batch write collapses them to a single target level). + final priorLevels = {}; + for (final id in selectedIds) { + final dance = await _repos.dances.getById(id); + if (dance == null) continue; + priorLevels[id] = dance.level; + } + + final count = await _repos.dances.setLevelForMany( + priorLevels.keys, + level: choice.level, + clearLevel: choice.clear, + now: DateTime.now().toUtc(), + ); + + // Narrow the captured priors to only the dances that actually changed, so + // Undo doesn't rewrite (and re-stamp) untouched dances. + final target = choice.clear ? null : choice.level; + priorLevels.removeWhere((_, prior) => prior == target); + + if (!mounted) return; + final label = choice.clear ? 'Cleared level on' : 'Set level on'; + final message = count == 0 + ? 'No changes' + : '$label $count ${count == 1 ? 'dance' : 'dances'}'; + SemanticsService.sendAnnouncement( + View.of(context), + message, + Directionality.of(context), + ); + + _exitSelectionMode(); + await _boot(); + if (!mounted) return; + + final messenger = ScaffoldMessenger.of(context); + messenger.clearSnackBars(); + if (count == 0) { + messenger.showSnackBar( + SnackBar( + key: const ValueKey('batch-level-snackbar'), + content: Text(message), + ), + ); + return; + } + messenger.showSnackBar( + SnackBar( + key: const ValueKey('batch-level-snackbar'), + content: Text(message), + action: SnackBarAction( + label: 'Undo', + onPressed: () => _undoBatchLevel(priorLevels), + ), + ), + ); + } + + /// Restores the captured [priorLevels] for each affected dance (app-side undo; + /// the repository has no batch-undo primitive). + Future _undoBatchLevel(Map priorLevels) async { + for (final entry in priorLevels.entries) { + final dance = await _repos.dances.getById(entry.key); + if (dance == null) continue; + await _repos.dances.update( + dance.copyWith( + level: entry.value, + clearLevel: entry.value == null, + updatedAt: DateTime.now().toUtc(), + ), + ); + } + if (mounted) await _boot(); + } + @override Widget build(BuildContext context) { return Scaffold( @@ -946,6 +1036,12 @@ class _DanceListScreenState extends State { icon: const Icon(Icons.label_off_outlined), onPressed: hasSelection ? () => _batchTag(BatchTagMode.remove) : null, ), + IconButton( + key: const ValueKey('batch-set-level'), + tooltip: 'Set level', + icon: const Icon(Icons.signal_cellular_alt), + onPressed: hasSelection ? _batchSetLevel : null, + ), ], ); } diff --git a/app/lib/src/widgets/batch_level_dialog.dart b/app/lib/src/widgets/batch_level_dialog.dart new file mode 100644 index 00000000..01181321 --- /dev/null +++ b/app/lib/src/widgets/batch_level_dialog.dart @@ -0,0 +1,113 @@ +import 'package:compendium_core/compendium_core.dart'; +import 'package:flutter/material.dart'; + +import '../search/facet_labels.dart'; + +/// The level chosen in the batch-level dialog: either a concrete [DanceLevel] +/// to set across the selection, or "clear" ([level] `null`, [clear] `true`) to +/// unset the level. Cancelling the dialog returns `null` instead of this. +class BatchLevelChoice { + const BatchLevelChoice({this.level, this.clear = false}); + + /// The level to set; `null` together with [clear] means "unset". + final DanceLevel? level; + + /// Whether the user picked the explicit "Unspecified (clear)" option. + final bool clear; +} + +/// Shows the batch **set level** picker for the Collection multi-select flow +/// (parallel to [showBatchTagDialog]). Unlike batch tagging, setting a level is +/// a *replace*, so the picker is a single-choice radio list — explicit and +/// reversible via the caller's Undo. Includes an explicit "Unspecified (clear)" +/// option so the selection's level can be removed. +/// +/// Returns the [BatchLevelChoice], or `null` if the user cancelled. Choices use +/// [RadioListTile] so state is exposed to assistive tech (radio role + selected +/// state) paired with a text label — never color alone. +Future showBatchLevelDialog(BuildContext context) { + return showDialog( + context: context, + builder: (_) => const _BatchLevelDialog(), + ); +} + +class _BatchLevelDialog extends StatefulWidget { + const _BatchLevelDialog(); + + @override + State<_BatchLevelDialog> createState() => _BatchLevelDialogState(); +} + +/// Sentinel for the "clear level" radio option, distinct from a concrete +/// [DanceLevel] and from "nothing selected yet". +enum _LevelSelection { unspecified } + +class _BatchLevelDialogState extends State<_BatchLevelDialog> { + // Holds either a `DanceLevel` or `_LevelSelection.unspecified`; `null` until + // the user picks something (keeps the confirm button disabled). + Object? _selected; + + void _select(Object value) => setState(() => _selected = value); + + @override + Widget build(BuildContext context) { + return AlertDialog( + key: const ValueKey('batch-level-dialog'), + title: const Text('Set level'), + content: SizedBox( + width: 360, + child: RadioGroup( + groupValue: _selected, + onChanged: (value) { + if (value != null) _select(value); + }, + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + for (final v in DanceLevel.values) + RadioListTile( + key: ValueKey('batch-level-option-${v.name}'), + dense: true, + contentPadding: EdgeInsets.zero, + controlAffinity: ListTileControlAffinity.leading, + value: v, + title: Text(danceLevelLabel(v)), + ), + RadioListTile( + key: const ValueKey('batch-level-option-unspecified'), + dense: true, + contentPadding: EdgeInsets.zero, + controlAffinity: ListTileControlAffinity.leading, + value: _LevelSelection.unspecified, + title: const Text('Unspecified (clear)'), + ), + ], + ), + ), + ), + actions: [ + TextButton( + key: const ValueKey('batch-level-cancel'), + onPressed: () => Navigator.of(context).pop(), + child: const Text('Cancel'), + ), + FilledButton( + key: const ValueKey('batch-level-confirm'), + onPressed: _selected == null + ? null + : () { + final selected = _selected; + Navigator.of(context).pop( + selected is DanceLevel + ? BatchLevelChoice(level: selected) + : const BatchLevelChoice(clear: true), + ); + }, + child: const Text('Set'), + ), + ], + ); + } +} diff --git a/app/test/screens/batch_level_test.dart b/app/test/screens/batch_level_test.dart new file mode 100644 index 00000000..95a2b358 --- /dev/null +++ b/app/test/screens/batch_level_test.dart @@ -0,0 +1,200 @@ +import 'package:compendium_core/compendium_core.dart'; +import 'package:drift/drift.dart' show driftRuntimeOptions; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import 'package:compendium_app/src/data/active_dialect_scope.dart'; +import 'package:compendium_app/src/data/app_theme_scope.dart'; +import 'package:compendium_app/src/data/custom_themes_controller.dart'; +import 'package:compendium_app/src/data/custom_themes_scope.dart'; +import 'package:compendium_app/src/data/repositories_scope.dart'; +import 'package:compendium_app/src/screens/dance_list_screen.dart'; + +import '../support/test_repositories.dart'; + +Dance _dance({required String id, required String title, DanceLevel? level}) => + Dance( + id: id, + title: title, + level: level, + form: DanceForm.contra, + formation: const Formation(FormationShape.dupleImproper), + status: DanceStatus.active, + figures: const [], + customFields: const [], + hook: '', + createdAt: DateTime.utc(2026, 1, 1), + updatedAt: DateTime.utc(2026, 1, 1), + ); + +Future _pumpScreen( + WidgetTester tester, + CompendiumRepositories repos, +) async { + await tester.binding.setSurfaceSize(const Size(1200, 3000)); + addTearDown(() => tester.binding.setSurfaceSize(null)); + final notifier = ValueNotifier(Dialect.larksRobins); + addTearDown(notifier.dispose); + final themeNotifier = ValueNotifier( + AppThemeSelection.system, + ); + addTearDown(themeNotifier.dispose); + final customThemes = CustomThemesController(repos.settings); + await customThemes.load(); + addTearDown(customThemes.dispose); + await tester.pumpWidget( + MaterialApp( + builder: (context, child) => RepositoriesScope( + repositories: repos, + child: AppThemeScope( + notifier: themeNotifier, + child: CustomThemesScope( + controller: customThemes, + child: ActiveDialectScope(notifier: notifier, child: child!), + ), + ), + ), + home: const DanceListScreen(), + ), + ); + await tester.pumpAndSettle(); +} + +Future _enterSelectionMode(WidgetTester tester) async { + await tester.tap(find.byKey(const ValueKey('batch-select'))); + await tester.pumpAndSettle(); +} + +Future _toggle(WidgetTester tester, String danceId) async { + await tester.tap(find.byKey(ValueKey('batch-checkbox-$danceId'))); + await tester.pumpAndSettle(); +} + +Future _setLevel(WidgetTester tester, String optionKey) async { + await tester.tap(find.byKey(const ValueKey('batch-set-level'))); + await tester.pumpAndSettle(); + await tester.tap(find.byKey(ValueKey('batch-level-option-$optionKey'))); + await tester.pumpAndSettle(); + await tester.tap(find.byKey(const ValueKey('batch-level-confirm'))); + await tester.pumpAndSettle(); +} + +void main() { + driftRuntimeOptions.dontWarnAboutMultipleDatabases = true; + + testWidgets('set-level applies the chosen level to every selected dance', ( + tester, + ) async { + final repos = openTestRepositories(); + await repos.dances.create(_dance(id: 'd1', title: 'Alpha')); + await repos.dances.create(_dance(id: 'd2', title: 'Bravo')); + await repos.dances.create(_dance(id: 'd3', title: 'Charlie')); + await _pumpScreen(tester, repos); + + await _enterSelectionMode(tester); + await _toggle(tester, 'd1'); + await _toggle(tester, 'd2'); + await _setLevel(tester, 'intermediate'); + + expect((await repos.dances.getById('d1'))!.level, DanceLevel.intermediate); + expect((await repos.dances.getById('d2'))!.level, DanceLevel.intermediate); + // The un-selected dance is untouched. + expect((await repos.dances.getById('d3'))!.level, isNull); + // Selection mode ends after applying. + expect(find.byKey(const ValueKey('batch-select')), findsOneWidget); + }); + + testWidgets('set-level is idempotent on dances already at that level', ( + tester, + ) async { + final repos = openTestRepositories(); + await repos.dances.create( + _dance(id: 'd1', title: 'Alpha', level: DanceLevel.beginner), + ); + await repos.dances.create(_dance(id: 'd2', title: 'Bravo')); + await _pumpScreen(tester, repos); + + await _enterSelectionMode(tester); + await _toggle(tester, 'd1'); + await _toggle(tester, 'd2'); + await _setLevel(tester, 'beginner'); + + expect((await repos.dances.getById('d1'))!.level, DanceLevel.beginner); + expect((await repos.dances.getById('d2'))!.level, DanceLevel.beginner); + // Only one dance actually changed. + expect(find.text('Set level on 1 dance'), findsOneWidget); + }); + + testWidgets('set-level with "Unspecified (clear)" clears the level', ( + tester, + ) async { + final repos = openTestRepositories(); + await repos.dances.create( + _dance(id: 'd1', title: 'Alpha', level: DanceLevel.advanced), + ); + await _pumpScreen(tester, repos); + + await _enterSelectionMode(tester); + await _toggle(tester, 'd1'); + await _setLevel(tester, 'unspecified'); + + expect((await repos.dances.getById('d1'))!.level, isNull); + expect(find.text('Cleared level on 1 dance'), findsOneWidget); + }); + + testWidgets('undo restores the prior per-dance levels', (tester) async { + final repos = openTestRepositories(); + await repos.dances.create( + _dance(id: 'd1', title: 'Alpha', level: DanceLevel.beginner), + ); + await repos.dances.create(_dance(id: 'd2', title: 'Bravo')); + await _pumpScreen(tester, repos); + + await _enterSelectionMode(tester); + await _toggle(tester, 'd1'); + await _toggle(tester, 'd2'); + await _setLevel(tester, 'advanced'); + + expect((await repos.dances.getById('d1'))!.level, DanceLevel.advanced); + expect((await repos.dances.getById('d2'))!.level, DanceLevel.advanced); + + await tester.tap(find.text('Undo')); + await tester.pumpAndSettle(); + + // Each dance is restored to its own prior level. + expect((await repos.dances.getById('d1'))!.level, DanceLevel.beginner); + expect((await repos.dances.getById('d2'))!.level, isNull); + }); + + testWidgets('set-level is a no-op when nothing actually changes', ( + tester, + ) async { + final repos = openTestRepositories(); + await repos.dances.create( + _dance(id: 'd1', title: 'Alpha', level: DanceLevel.intermediate), + ); + await _pumpScreen(tester, repos); + + await _enterSelectionMode(tester); + await _toggle(tester, 'd1'); + await _setLevel(tester, 'intermediate'); + + expect(find.text('No changes'), findsOneWidget); + expect((await repos.dances.getById('d1'))!.level, DanceLevel.intermediate); + }); + + testWidgets('set-level button is disabled with an empty selection', ( + tester, + ) async { + final repos = openTestRepositories(); + await repos.dances.create(_dance(id: 'd1', title: 'Alpha')); + await _pumpScreen(tester, repos); + + await _enterSelectionMode(tester); + + final button = tester.widget( + find.byKey(const ValueKey('batch-set-level')), + ); + expect(button.onPressed, isNull); + }); +} diff --git a/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart b/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart index 4e56a599..bb08dfc6 100644 --- a/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart +++ b/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart @@ -428,6 +428,48 @@ class DanceRepository { }); } + /// Sets the difficulty [level] on many dances at once, in a single + /// transaction, for the Collection multi-select "batch set level" flow. + /// + /// Pass [clearLevel] `true` to unset the level (`null`) across the selection; + /// a set [clearLevel] wins over any [level] value, mirroring + /// [Dance.copyWith]. Each affected dance is rewritten through the same upsert + /// path as [update], so the derived figure/FTS indexes stay consistent. + /// + /// Skips unknown ids and dances already at the target level (idempotent), and + /// stamps [now] as `updatedAt` only on dances that actually change. Returns + /// the number of dances changed. An empty [ids] is a no-op returning `0`. + /// Because the whole batch runs in one transaction, an error leaves the + /// collection untouched rather than half-updated. + Future setLevelForMany( + Iterable ids, { + DanceLevel? level, + bool clearLevel = false, + required DateTime now, + }) { + assertUtc(now, 'now'); + final target = clearLevel ? null : level; + final list = ids.toList(); + if (list.isEmpty) return Future.value(0); + return _db.transaction(() async { + var changed = 0; + for (final id in list) { + final dance = await getById(id); + if (dance == null) continue; + if (dance.level == target) continue; + await _upsert( + dance.copyWith( + level: target, + clearLevel: target == null, + updatedAt: now, + ), + ); + changed++; + } + return changed; + }); + } + /// Duplicates the dance identified by [id] under [newId] via /// [Dance.duplicate] (fresh identity, no provenance) and persists it. Future duplicate({ diff --git a/packages/compendium_core/test/storage/dance_batch_level_test.dart b/packages/compendium_core/test/storage/dance_batch_level_test.dart new file mode 100644 index 00000000..f0f26176 --- /dev/null +++ b/packages/compendium_core/test/storage/dance_batch_level_test.dart @@ -0,0 +1,123 @@ +import 'package:compendium_core/compendium_core.dart'; +import 'package:test/test.dart'; + +import 'test_database.dart'; +import 'fixtures.dart'; + +void main() { + late CompendiumDatabase db; + late DanceRepository dances; + + final now = DateTime.utc(2026, 6, 1); + + setUp(() { + db = openTestDatabase(); + dances = DanceRepository(db, contraTaxonomy); + }); + + tearDown(() => db.close()); + + test('setLevelForMany sets the level on all listed dances', () async { + await dances.create(sampleDance(id: 'a', title: 'Alpha')); + await dances.create(sampleDance(id: 'b', title: 'Bravo')); + await dances.create(sampleDance(id: 'c', title: 'Charlie')); + + final changed = await dances.setLevelForMany( + ['a', 'b'], + level: DanceLevel.intermediate, + now: now, + ); + + expect(changed, 2); + expect((await dances.getById('a'))!.level, DanceLevel.intermediate); + expect((await dances.getById('b'))!.level, DanceLevel.intermediate); + // The un-listed dance is untouched. + expect((await dances.getById('c'))!.level, isNull); + }); + + test('setLevelForMany stamps updatedAt only on changed dances', () async { + await dances.create(sampleDance(id: 'a', title: 'Alpha')); + final before = (await dances.getById('a'))!.updatedAt; + + await dances.setLevelForMany(['a'], level: DanceLevel.advanced, now: now); + + expect((await dances.getById('a'))!.updatedAt, now); + expect(now, isNot(before)); + }); + + test( + 'setLevelForMany is idempotent — skips dances already at target', + () async { + await dances.create( + sampleDance( + id: 'a', + title: 'Alpha', + ).copyWith(level: DanceLevel.beginner), + ); + await dances.create(sampleDance(id: 'b', title: 'Bravo')); + + final changed = await dances.setLevelForMany( + ['a', 'b'], + level: DanceLevel.beginner, + now: now, + ); + + // Only b changes; a is already beginner. + expect(changed, 1); + expect((await dances.getById('a'))!.level, DanceLevel.beginner); + expect((await dances.getById('b'))!.level, DanceLevel.beginner); + }, + ); + + test('setLevelForMany with clearLevel unsets the level', () async { + await dances.create( + sampleDance(id: 'a', title: 'Alpha').copyWith(level: DanceLevel.advanced), + ); + + final changed = await dances.setLevelForMany( + ['a'], + clearLevel: true, + now: now, + ); + + expect(changed, 1); + expect((await dances.getById('a'))!.level, isNull); + }); + + test('setLevelForMany clearLevel wins over a passed level value', () async { + await dances.create( + sampleDance(id: 'a', title: 'Alpha').copyWith(level: DanceLevel.advanced), + ); + + await dances.setLevelForMany( + ['a'], + level: DanceLevel.beginner, + clearLevel: true, + now: now, + ); + + expect((await dances.getById('a'))!.level, isNull); + }); + + test('setLevelForMany ignores unknown ids and an empty list', () async { + await dances.create(sampleDance(id: 'a', title: 'Alpha')); + + expect( + await dances.setLevelForMany( + const [], + level: DanceLevel.intermediate, + now: now, + ), + 0, + ); + expect( + await dances.setLevelForMany( + ['does-not-exist'], + level: DanceLevel.intermediate, + now: now, + ), + 0, + ); + expect((await dances.getById('a'))!.level, isNull); + }); +} From 198385f1503aa2033de4cd4c6c302df0007f14ad Mon Sep 17 00:00:00 2001 From: Isaac Banner Date: Mon, 20 Jul 2026 17:48:49 -0700 Subject: [PATCH 2/3] fix(collection): guard setLevelForMany against accidental level clears (#409) Require a non-null level unless clearLevel is true, so calling the batch setter with default args can no longer silently wipe every selected dance's level. Documents the mutually-exclusive set-vs-clear contract and adds a test asserting the guard fires. Addresses Copilot review feedback on #422. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/storage/repositories/dance_repository.dart | 12 ++++++++++-- .../test/storage/dance_batch_level_test.dart | 10 ++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart b/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart index bb08dfc6..86f01260 100644 --- a/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart +++ b/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart @@ -431,8 +431,12 @@ class DanceRepository { /// Sets the difficulty [level] on many dances at once, in a single /// transaction, for the Collection multi-select "batch set level" flow. /// - /// Pass [clearLevel] `true` to unset the level (`null`) across the selection; - /// a set [clearLevel] wins over any [level] value, mirroring + /// Contract: to *set* a level pass a non-null [level]; to *unset* it pass + /// [clearLevel] `true`. These are mutually exclusive — asserting + /// `clearLevel || level != null` prevents the footgun of accidentally + /// clearing every dance by omitting [level] (which would otherwise diverge + /// from [Dance.copyWith], where a null value without a clear flag keeps the + /// existing value). A set [clearLevel] wins over any [level] value, matching /// [Dance.copyWith]. Each affected dance is rewritten through the same upsert /// path as [update], so the derived figure/FTS indexes stay consistent. /// @@ -447,6 +451,10 @@ class DanceRepository { bool clearLevel = false, required DateTime now, }) { + assert( + clearLevel || level != null, + 'setLevelForMany requires a non-null level unless clearLevel is true', + ); assertUtc(now, 'now'); final target = clearLevel ? null : level; final list = ids.toList(); diff --git a/packages/compendium_core/test/storage/dance_batch_level_test.dart b/packages/compendium_core/test/storage/dance_batch_level_test.dart index f0f26176..19dd029c 100644 --- a/packages/compendium_core/test/storage/dance_batch_level_test.dart +++ b/packages/compendium_core/test/storage/dance_batch_level_test.dart @@ -120,4 +120,14 @@ void main() { ); expect((await dances.getById('a'))!.level, isNull); }); + + test('setLevelForMany asserts a level or clearLevel is provided', () async { + await dances.create(sampleDance(id: 'a', title: 'Alpha')); + + // Neither a level nor clearLevel — must not silently clear. + expect( + () => dances.setLevelForMany(['a'], now: now), + throwsA(isA()), + ); + }); } From ee8eb6a24e64673269088791b58c69d1f72005ac Mon Sep 17 00:00:00 2001 From: Isaac Banner Date: Mon, 20 Jul 2026 18:04:02 -0700 Subject: [PATCH 3/3] fix(collection): throw (not just assert) on ambiguous setLevelForMany call (#409) Asserts are stripped in release, so the debug-only guard alone left a latent footgun: a caller omitting level: (with clearLevel default false) would silently wipe every selected dance's level in production. Add a release-safe ArgumentError thrown before the assert so the contract holds in all build modes; clearLevel still wins when both are passed. Updates the core test to expect ArgumentError and verify the existing level survives. Addresses Copilot review feedback on #422. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../repositories/dance_repository.dart | 21 +++++++++++++------ .../test/storage/dance_batch_level_test.dart | 16 ++++++++++---- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart b/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart index 86f01260..7bf0e8fe 100644 --- a/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart +++ b/packages/compendium_core/lib/src/storage/repositories/dance_repository.dart @@ -432,11 +432,11 @@ class DanceRepository { /// transaction, for the Collection multi-select "batch set level" flow. /// /// Contract: to *set* a level pass a non-null [level]; to *unset* it pass - /// [clearLevel] `true`. These are mutually exclusive — asserting - /// `clearLevel || level != null` prevents the footgun of accidentally - /// clearing every dance by omitting [level] (which would otherwise diverge - /// from [Dance.copyWith], where a null value without a clear flag keeps the - /// existing value). A set [clearLevel] wins over any [level] value, matching + /// [clearLevel] `true`. These are mutually exclusive — calling with neither + /// throws an [ArgumentError] (and trips a debug assert) to prevent the + /// footgun of accidentally clearing every dance by omitting [level] (which + /// would otherwise diverge from [Dance.copyWith], where a null value without + /// a clear flag keeps the existing value). A set [clearLevel] wins over any [level] value, matching /// [Dance.copyWith]. Each affected dance is rewritten through the same upsert /// path as [update], so the derived figure/FTS indexes stay consistent. /// @@ -451,9 +451,18 @@ class DanceRepository { bool clearLevel = false, required DateTime now, }) { + // Release-safe guard (asserts are stripped in release): a caller must pass + // a concrete level, or opt in to clearing via clearLevel. This is checked + // before the debug-only assert so the thrown ArgumentError is deterministic + // across build modes. clearLevel still takes precedence when both are set. + if (!clearLevel && level == null) { + throw ArgumentError( + 'setLevelForMany requires a non-null level unless clearLevel is true', + ); + } assert( clearLevel || level != null, - 'setLevelForMany requires a non-null level unless clearLevel is true', + 'setLevelForMany: pass a non-null level, or clearLevel: true to unset', ); assertUtc(now, 'now'); final target = clearLevel ? null : level; diff --git a/packages/compendium_core/test/storage/dance_batch_level_test.dart b/packages/compendium_core/test/storage/dance_batch_level_test.dart index 19dd029c..397230fc 100644 --- a/packages/compendium_core/test/storage/dance_batch_level_test.dart +++ b/packages/compendium_core/test/storage/dance_batch_level_test.dart @@ -121,13 +121,21 @@ void main() { expect((await dances.getById('a'))!.level, isNull); }); - test('setLevelForMany asserts a level or clearLevel is provided', () async { - await dances.create(sampleDance(id: 'a', title: 'Alpha')); + test('setLevelForMany throws (and does not wipe) when given neither level ' + 'nor clearLevel', () async { + await dances.create( + sampleDance( + id: 'a', + title: 'Alpha', + ).copyWith(level: DanceLevel.intermediate), + ); - // Neither a level nor clearLevel — must not silently clear. + // Release-safe guard: must throw ArgumentError, not silently clear. expect( () => dances.setLevelForMany(['a'], now: now), - throwsA(isA()), + throwsA(isA()), ); + // The existing level survives the rejected call. + expect((await dances.getById('a'))!.level, DanceLevel.intermediate); }); }