Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/extensions/playable_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ extension PlayableListExtension on List<Playable> {
}

List<Playable> $sort(PlayableSortConfig config) {
return this
// 'position' keeps the order the server returned (e.g. a playlist's own
// order defined on the web), reversible via the sort direction.
if (config.field == 'position') {
return config.order == SortOrder.asc ? [...this] : reversed.toList();
}

return [...this]
..sort((a, b) => config.order == SortOrder.asc
? a.valueToCompare(config).compareTo(b.valueToCompare(config))
: b.valueToCompare(config).compareTo(a.valueToCompare(config)));
Expand Down
4 changes: 2 additions & 2 deletions lib/ui/screens/playlist_details.dart
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class _PlaylistDetailsScreen extends State<PlaylistDetailsScreen> {
final playlist = ModalRoute.of(context)!.settings.arguments as Playlist;
var sortConfig = AppState.get(
'playlist.sort',
PlayableSortConfig(field: 'title', order: SortOrder.asc),
PlayableSortConfig(field: 'position', order: SortOrder.asc),
)!;

return Scaffold(
Expand Down Expand Up @@ -98,7 +98,7 @@ class _PlaylistDetailsScreen extends State<PlaylistDetailsScreen> {
backgroundImage: _buildBackgroundImage(playlist, playables),
actions: [
SortButton(
fields: ['title', 'artist_name', 'created_at'],
fields: ['position', 'title', 'artist_name', 'created_at'],
currentField: sortConfig.field,
currentOrder: sortConfig.order,
onMenuItemSelected: (_sortConfig) {
Expand Down
1 change: 1 addition & 0 deletions lib/ui/widgets/playable_list_sort_button.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class SortButton extends StatelessWidget {
SortOrder currentOrder;

static const sortFields = {
'position': 'Custom order',
'track': 'Track number',
'disc': 'Disc number',
'title': 'Title',
Expand Down
44 changes: 44 additions & 0 deletions test/extensions/playable_list_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import 'package:app/enums.dart';
import 'package:app/extensions/extensions.dart';
import 'package:app/models/models.dart';
import 'package:app/values/values.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
late Song a, b, c;

setUp(() {
a = Song.fake(title: 'A');
b = Song.fake(title: 'B');
c = Song.fake(title: 'C');
});

// Deliberately not alphabetical — mimics a playlist's custom order.
List<Playable> customOrder() => <Playable>[c, a, b];

PlayableSortConfig config(String field, SortOrder order) =>
PlayableSortConfig(field: field, order: order);

group('\$sort', () {
test('position keeps the server-provided order', () {
final sorted = customOrder().$sort(config('position', SortOrder.asc));
expect(sorted, [c, a, b]);
});

test('position descending reverses the server-provided order', () {
final sorted = customOrder().$sort(config('position', SortOrder.desc));
expect(sorted, [b, a, c]);
});

test('other fields still sort', () {
final sorted = customOrder().$sort(config('title', SortOrder.asc));
expect(sorted.map((playable) => (playable as Song).title), ['A', 'B', 'C']);
});

test('does not mutate the original list', () {
final original = customOrder();
original.$sort(config('title', SortOrder.asc));
expect(original, [c, a, b]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add an immutability assertion for the position sort path.

The immutability test only covers the title field. Since position is the primary new feature, explicitly verifying that it also doesn't mutate the original list would close the coverage gap.

🧪 Suggested additional test
     test('does not mutate the original list', () {
       final original = customOrder();
       original.$sort(config('title', SortOrder.asc));
+      original.$sort(config('position', SortOrder.asc));
+      original.$sort(config('position', SortOrder.desc));
       expect(original, [c, a, b]);
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test('does not mutate the original list', () {
final original = customOrder();
original.$sort(config('title', SortOrder.asc));
expect(original, [c, a, b]);
test('does not mutate the original list', () {
final original = customOrder();
original.$sort(config('title', SortOrder.asc));
original.$sort(config('position', SortOrder.asc));
original.$sort(config('position', SortOrder.desc));
expect(original, [c, a, b]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extensions/playable_list_test.dart` around lines 38 - 41, Extend the
existing “does not mutate the original list” test to also exercise sorting via
the position field, using the position sort configuration and asserting the
original list remains unchanged. Keep the current title immutability assertion
intact.

});
});
}
Loading