Feature/3580 add music next queue#161
Open
rodrigonepomucenosm wants to merge 3 commits intomasterfrom
Open
Conversation
WalkthroughAdds a gitignore rule for shell scripts, a Makefile target to run Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
packages/mediascanner/example/test/widget_test.dart (2)
11-11: Prefer package import over relative import traversing intolib/.Same issue as in the snowplow test file. Use a package import to comply with
avoid_relative_lib_imports.♻️ Suggested fix
-import '../lib/main.dart'; +import 'package:mediascanner_example/main.dart';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mediascanner/example/test/widget_test.dart` at line 11, Replace the relative import in widget_test.dart that traverses into lib (import '../lib/main.dart';) with a package import (e.g., import 'package:mediascanner/main.dart';) to satisfy the avoid_relative_lib_imports rule; update any test references if needed to use the same public symbols from Main (or whatever top-level symbols are exposed) after switching to the package import.
21-21: Consider null-safe access instead of force-unwrap.Same pattern as the snowplow test. A null-aware operator avoids a potential runtime exception.
🛡️ Suggested fix
- (Widget widget) => widget is Text && widget.data!.startsWith('Running on:'), + (Widget widget) => widget is Text && (widget.data?.startsWith('Running on:') ?? false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mediascanner/example/test/widget_test.dart` at line 21, The predicate in the widget test uses a force-unwrap on Text.data which can throw; update the check in the closure used for finding the widget (the lambda that currently uses "widget is Text && widget.data!.startsWith('Running on:')") to use null-safe access (e.g., call startsWith via a null-aware operator and compare to true) so it only returns true when data is non-null and starts with the expected string.packages/snowplow/example/test/widget_test.dart (2)
11-11: Prefer package import over relative import traversing intolib/.Using
../lib/main.dartviolates Dart'savoid_relative_lib_importslint rule. The recommended approach is to use a package import when referencing files insidelib/from outside it.♻️ Suggested fix
-import '../lib/main.dart'; +import 'package:snowplow_example/main.dart';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/snowplow/example/test/widget_test.dart` at line 11, Replace the relative import that traverses into lib with a package import: update the import statement in widget_test.dart that currently references ../lib/main.dart to use the package import for the library (e.g., import 'package:<package_name>/main.dart';), ensuring you use the project package name used in pubspec.yaml so it conforms to the avoid_relative_lib_imports lint.
21-21: Consider null-safe access instead of force-unwrap.
Text.datais nullable (String?). Using!will throw at runtime ifdatais null (e.g., when aTextwidget was created viaText.rich()). A null-aware operator is safer:🛡️ Suggested fix
- (Widget widget) => widget is Text && widget.data!.startsWith('Running on:'), + (Widget widget) => widget is Text && (widget.data?.startsWith('Running on:') ?? false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/snowplow/example/test/widget_test.dart` at line 21, The test currently force-unwraps Text.data (in the lambda checking a Widget) which can throw if data is null; change the predicate in the widget matcher to use null-safe access on Text.data (e.g., use data?.startsWith('Running on:') and treat null as false) so the check uses Text.data's nullable type instead of `!`.packages/migration/example/test/widget_test.dart (2)
21-21: Consider null-safe access instead of force-unwrap.Same pattern as the other test files. A null-aware operator avoids a potential runtime exception.
🛡️ Suggested fix
- (Widget widget) => widget is Text && widget.data!.startsWith('Running on:'), + (Widget widget) => widget is Text && (widget.data?.startsWith('Running on:') ?? false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/migration/example/test/widget_test.dart` at line 21, The test currently force-unwraps Text.data in the predicate (the lambda checking "widget is Text && widget.data!.startsWith('Running on:')"), which can throw if data is null; update the predicate to use null-safe access and boolean fallback (e.g. replace the force-unwrap with a null-aware call and a default false) so the check becomes safe when Text.data is null while still ensuring the widget is a Text before calling startsWith.
11-11: Prefer package import over relative import traversing intolib/.Same issue as in the other test files. Use a package import to comply with
avoid_relative_lib_imports.♻️ Suggested fix
-import '../lib/main.dart'; +import 'package:migration_example/main.dart';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/migration/example/test/widget_test.dart` at line 11, Replace the relative import that traverses into lib ("import '../lib/main.dart';") with a package import to satisfy avoid_relative_lib_imports: change the import to "package:migration/main.dart" (or the actual package name if different) in widget_test.dart so the test imports main.dart via package imports rather than a relative path.Makefile (2)
3-6: Fail fast when no package pubspecs are found.Currently this can print “All packages updated!” even when nothing matched
packages/*/pubspec.yaml. Add a guard so CI fails loudly on misconfiguration.Suggested fix
pub-get: - `@failed`=""; \ + `@if` [ -z "$(strip $(PUBSPEC_DIRS))" ]; then \ + echo "No pubspec.yaml found under packages/*/"; \ + exit 1; \ + fi; \ + failed=""; \ for dir in $(PUBSPEC_DIRS); do \ echo "=> pub get: $$dir"; \ (cd $$dir && flutter pub get) || failed="$$failed $$dir"; \ done; \Also applies to: 17-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 3 - 6, The Makefile's pub-get target uses PUBSPEC_DIRS but doesn't fail when no pubspecs are found, so add a fail-fast guard at the start of the pub-get target: test whether PUBSPEC_DIRS (the variable defined at top) is empty (e.g. via a make conditional/strip check or shell test) and if so print a clear error and exit non-zero to fail CI; otherwise continue with the existing loop that runs flutter pub get for each dir. Reference the PUBSPEC_DIRS variable and the pub-get target when making the change.
1-1: Consider declaring standard phony targets to satisfy checkmake policy.If your lint gate enforces
minphony, addall,clean, andtestto avoid recurring warnings.Suggested fix
-.PHONY: pub-get +.PHONY: all clean test pub-get + +all: pub-get +clean: + @: +test: + @:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Add the standard phony targets required by the lint rule: update the .PHONY declaration (which currently contains pub-get) to also include all, clean, and test; if those targets do not exist yet, add simple target stubs (e.g., an "all" that depends on your default build, a "clean" that removes build artifacts, and a "test" target that runs your test command) so checkmake's minphony policy is satisfied and the Makefile's .PHONY entry references all four names: all, clean, test, pub-get.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 512: The .gitignore currently uses a global "*.sh" pattern which
unintentionally hides all shell scripts; replace this broad pattern (the "*.sh"
entry) with more specific ignores that target generated or tooling script
locations only (for example ignore a build or generated scripts directory or
specific filenames), or remove the entry entirely and list individual generated
script paths instead so repo automation and CI scripts aren’t accidentally
excluded; update the "*.sh" line accordingly to a scoped path or explicit
filenames.
In `@packages/player/lib/src/player.dart`:
- Around line 254-257: The load(Media media) method currently ignores its media
parameter and calls _doPlay(_queue.current!, shouldLoadOnly: true), which can
load the wrong track or throw if _queue.current is null; change the call in load
to pass the media argument to _doPlay (i.e. _doPlay(media, shouldLoadOnly:
true)) and ensure any callers or _doPlay signature support a Media argument
(update _doPlay parameter types or overload if needed) and add a null-safety
check if _doPlay can accept nulls.
- Line 109: The calls to the async persistence methods _queue.add and
_queue.addAll are currently invoked without awaiting, causing fire-and-forget
writes and silent failures; update the Player methods that call
_queue.add(media, enqueueAfterCurrent) and _queue.addAll(mediaList,
enqueueAfterCurrent) to await those calls (and mark the caller async if not
already) so that the internal _save (which uses IsarService) completes or
surfaces errors to callers instead of returning immediately.
In `@packages/player/lib/src/queue.dart`:
- Around line 121-124: The code uses `_index + 1` directly as an insert position
in the enqueue path (see enqueueAfterCurrent handling and calls to
`storage.insert`/`insertAll`), which can throw RangeError if `_index` is stale
after removals; clamp or validate the computed position before inserting by
computing `final position = (_index + 1).clamp(0, storage.length)` (or
equivalent bounds check) and use that bounded `position` when creating
`QueueItem` and calling `storage.insert`/`insertAll` so inserts never pass an
out-of-range index.
- Line 132: The code maps enqueueAfterCurrent to saveOnTop when calling _save
(see calls around enqueueAfterCurrent and _save), but _save only supports
prepend/append via organizeLists so middle insertions are not persisted and
storage order diverges from the runtime queue; update the persistence to record
the exact resulting queue order for middle inserts by either extending _save to
accept an explicit index/position (e.g., new parameter insertIndex) and
persisting via organizeLists or by constructing the full new ordered list (using
the runtime queue/state after inserting the media at the desired index) and
passing that list into _save so the stored order matches the runtime queue; make
the change where enqueueAfterCurrent is mapped (calls at lines around the
enqueue paths) and ensure organizeLists or the persistence layer receives the
explicit ordered list instead of relying solely on saveOnTop.
---
Nitpick comments:
In `@Makefile`:
- Around line 3-6: The Makefile's pub-get target uses PUBSPEC_DIRS but doesn't
fail when no pubspecs are found, so add a fail-fast guard at the start of the
pub-get target: test whether PUBSPEC_DIRS (the variable defined at top) is empty
(e.g. via a make conditional/strip check or shell test) and if so print a clear
error and exit non-zero to fail CI; otherwise continue with the existing loop
that runs flutter pub get for each dir. Reference the PUBSPEC_DIRS variable and
the pub-get target when making the change.
- Line 1: Add the standard phony targets required by the lint rule: update the
.PHONY declaration (which currently contains pub-get) to also include all,
clean, and test; if those targets do not exist yet, add simple target stubs
(e.g., an "all" that depends on your default build, a "clean" that removes build
artifacts, and a "test" target that runs your test command) so checkmake's
minphony policy is satisfied and the Makefile's .PHONY entry references all four
names: all, clean, test, pub-get.
In `@packages/mediascanner/example/test/widget_test.dart`:
- Line 11: Replace the relative import in widget_test.dart that traverses into
lib (import '../lib/main.dart';) with a package import (e.g., import
'package:mediascanner/main.dart';) to satisfy the avoid_relative_lib_imports
rule; update any test references if needed to use the same public symbols from
Main (or whatever top-level symbols are exposed) after switching to the package
import.
- Line 21: The predicate in the widget test uses a force-unwrap on Text.data
which can throw; update the check in the closure used for finding the widget
(the lambda that currently uses "widget is Text &&
widget.data!.startsWith('Running on:')") to use null-safe access (e.g., call
startsWith via a null-aware operator and compare to true) so it only returns
true when data is non-null and starts with the expected string.
In `@packages/migration/example/test/widget_test.dart`:
- Line 21: The test currently force-unwraps Text.data in the predicate (the
lambda checking "widget is Text && widget.data!.startsWith('Running on:')"),
which can throw if data is null; update the predicate to use null-safe access
and boolean fallback (e.g. replace the force-unwrap with a null-aware call and a
default false) so the check becomes safe when Text.data is null while still
ensuring the widget is a Text before calling startsWith.
- Line 11: Replace the relative import that traverses into lib ("import
'../lib/main.dart';") with a package import to satisfy
avoid_relative_lib_imports: change the import to "package:migration/main.dart"
(or the actual package name if different) in widget_test.dart so the test
imports main.dart via package imports rather than a relative path.
In `@packages/snowplow/example/test/widget_test.dart`:
- Line 11: Replace the relative import that traverses into lib with a package
import: update the import statement in widget_test.dart that currently
references ../lib/main.dart to use the package import for the library (e.g.,
import 'package:<package_name>/main.dart';), ensuring you use the project
package name used in pubspec.yaml so it conforms to the
avoid_relative_lib_imports lint.
- Line 21: The test currently force-unwraps Text.data (in the lambda checking a
Widget) which can throw if data is null; change the predicate in the widget
matcher to use null-safe access on Text.data (e.g., use
data?.startsWith('Running on:') and treat null as false) so the check uses
Text.data's nullable type instead of `!`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignoreMakefilepackages/mediascanner/example/test/widget_test.dartpackages/migration/example/test/widget_test.dartpackages/player/lib/src/player.dartpackages/player/lib/src/queue.dartpackages/snowplow/example/test/widget_test.dart
| !/gradle/wrapper/gradle-wrapper.jar | ||
|
|
||
| # End of https://www.gitignore.io/api/java,swift,kotlin,android,flutter,intellij,objective-c,androidstudio | ||
| *.sh |
There was a problem hiding this comment.
Avoid a global *.sh ignore pattern.
Line 512 will ignore all shell scripts, including valid repo automation scripts (CI, release, tooling), which can silently block commits. Prefer ignoring only generated/script-specific paths.
Suggested fix
-*.sh
+# Keep shell scripts versioned by default; ignore only generated ones explicitly.📝 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
| *.sh | |
| # Keep shell scripts versioned by default; ignore only generated ones explicitly. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 512, The .gitignore currently uses a global "*.sh"
pattern which unintentionally hides all shell scripts; replace this broad
pattern (the "*.sh" entry) with more specific ignores that target generated or
tooling script locations only (for example ignore a build or generated scripts
directory or specific filenames), or remove the entry entirely and list
individual generated script paths instead so repo automation and CI scripts
aren’t accidentally excluded; update the "*.sh" line accordingly to a scoped
path or explicit filenames.
| Media media, [ | ||
| bool enqueueAfterCurrent = false, | ||
| ]) async { | ||
| _queue.add(media, enqueueAfterCurrent); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the Queue.add and Queue.addAll signatures
echo "=== Checking Queue.add/addAll signatures ==="
rg -n --type=dart '^\s*(Future|async).*\s+(add|addAll)\s*\(' packages/player/lib/src/queue.dart -B1 -A5
echo -e "\n=== Checking Player._queue.add/addAll calls ==="
rg -n --type=dart '_queue\.(add|addAll)\s*\(' packages/player/lib/src/player.dart -B2 -A2Repository: SuaMusica/flutter_plugins
Length of output: 446
🏁 Script executed:
#!/bin/bash
# Get file sizes to determine if we should read them entirely
echo "=== File sizes ==="
wc -l packages/player/lib/src/queue.dart packages/player/lib/src/player.dart
echo -e "\n=== Check specific lines 109 and 119 in player.dart ==="
sed -n '105,125p' packages/player/lib/src/player.dartRepository: SuaMusica/flutter_plugins
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Search for Queue class and its add/addAll method definitions
echo "=== Full search for add/addAll methods in Queue ==="
ast-grep --pattern 'class Queue {
$$$
$_* add$_* $$$
$$$
}'
ast-grep --pattern 'class Queue {
$$$
$_* addAll$_* $$$
$$$
}'Repository: SuaMusica/flutter_plugins
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Read queue.dart to see the full implementation
cat -n packages/player/lib/src/queue.dartRepository: SuaMusica/flutter_plugins
Length of output: 14415
🏁 Script executed:
#!/bin/bash
# Search for add and addAll method definitions specifically
echo "=== Searching for add method definition ==="
rg -n 'add\s*\(' packages/player/lib/src/queue.dart -B1 -A3
echo -e "\n=== Searching for addAll method definition ==="
rg -n 'addAll\s*\(' packages/player/lib/src/queue.dart -B1 -A3Repository: SuaMusica/flutter_plugins
Length of output: 1023
🏁 Script executed:
#!/bin/bash
# Verify the _save method signature to confirm persistence is truly async
rg -n '_save\(' packages/player/lib/src/queue.dart -B2 -A8Repository: SuaMusica/flutter_plugins
Length of output: 1076
Add await to queue persistence calls to prevent silent failures.
Queue.add() and Queue.addAll() are async methods that internally await _save(), which performs I/O with IsarService. Calling these without await at lines 109 and 119 creates a fire-and-forget pattern where persistence errors are silently ignored and callers receive success before writes complete.
Suggested fix
- _queue.add(media, enqueueAfterCurrent);
+ await _queue.add(media, enqueueAfterCurrent);- _queue.addAll(
+ await _queue.addAll(
items,
shouldRemoveFirst: shouldRemoveFirst,
saveOnTop: saveOnTop,
enqueueAfterCurrent: enqueueAfterCurrent,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/player/lib/src/player.dart` at line 109, The calls to the async
persistence methods _queue.add and _queue.addAll are currently invoked without
awaiting, causing fire-and-forget writes and silent failures; update the Player
methods that call _queue.add(media, enqueueAfterCurrent) and
_queue.addAll(mediaList, enqueueAfterCurrent) to await those calls (and mark the
caller async if not already) so that the internal _save (which uses IsarService)
completes or surfaces errors to callers instead of returning immediately.
Comment on lines
254
to
+257
| Future<int> load(Media media) async => _doPlay( | ||
| _queue.current!, | ||
| shouldLoadOnly: true, | ||
| ); | ||
| _queue.current!, | ||
| shouldLoadOnly: true, | ||
| ); |
There was a problem hiding this comment.
load(Media media) ignores its media parameter.
Line 255 loads _queue.current! instead of the method argument, which can load the wrong track (or throw when current is null).
🔧 Suggested fix
- Future<int> load(Media media) async => _doPlay(
- _queue.current!,
+ Future<int> load(Media media) async => _doPlay(
+ media,
shouldLoadOnly: true,
);📝 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
| Future<int> load(Media media) async => _doPlay( | |
| _queue.current!, | |
| shouldLoadOnly: true, | |
| ); | |
| _queue.current!, | |
| shouldLoadOnly: true, | |
| ); | |
| Future<int> load(Media media) async => _doPlay( | |
| media, | |
| shouldLoadOnly: true, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/player/lib/src/player.dart` around lines 254 - 257, The load(Media
media) method currently ignores its media parameter and calls
_doPlay(_queue.current!, shouldLoadOnly: true), which can load the wrong track
or throw if _queue.current is null; change the call in load to pass the media
argument to _doPlay (i.e. _doPlay(media, shouldLoadOnly: true)) and ensure any
callers or _doPlay signature support a Media argument (update _doPlay parameter
types or overload if needed) and add a null-safety check if _doPlay can accept
nulls.
Comment on lines
+121
to
+124
| if (enqueueAfterCurrent && storage.isNotEmpty) { | ||
| final position = _index + 1; | ||
| storage.insert(position, QueueItem(pos, pos, media)); | ||
|
|
There was a problem hiding this comment.
Guard insert index before insert/insertAll.
Line 122 and Line 154 use _index + 1 directly. If _index is stale (e.g., after removals), this can throw RangeError and break enqueue flow.
🔧 Suggested fix
- final position = _index + 1;
- storage.insert(position, QueueItem(pos, pos, media));
+ final insertAt = min(max(_index + 1, 0), storage.length);
+ storage.insert(insertAt, QueueItem(pos, pos, media));
...
- final insertAt = _index + 1;
+ final insertAt = min(max(_index + 1, 0), storage.length);
storage.insertAll(insertAt, _toQueueItems(medias, i));Also applies to: 153-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/player/lib/src/queue.dart` around lines 121 - 124, The code uses
`_index + 1` directly as an insert position in the enqueue path (see
enqueueAfterCurrent handling and calls to `storage.insert`/`insertAll`), which
can throw RangeError if `_index` is stale after removals; clamp or validate the
computed position before inserting by computing `final position = (_index +
1).clamp(0, storage.length)` (or equivalent bounds check) and use that bounded
`position` when creating `QueueItem` and calling `storage.insert`/`insertAll` so
inserts never pass an out-of-range index.
packages/player/lib/src/queue.dart
Outdated
| storage.add(QueueItem(pos, pos, media)); | ||
| } | ||
|
|
||
| await _save(medias: [media], saveOnTop: enqueueAfterCurrent); |
There was a problem hiding this comment.
enqueueAfterCurrent persists a different order than runtime queue.
Line 132 and Line 166 map enqueueAfterCurrent to saveOnTop, but _save only supports prepend/append (organizeLists). For middle insertion, persisted order diverges from storage, so queue restore can be incorrect.
🔧 Suggested fix
- await _save(medias: [media], saveOnTop: enqueueAfterCurrent);
+ await _save(medias: storage.map((e) => e.item).toList());
...
- await _save(medias: items, saveOnTop: enqueueAfterCurrent || saveOnTop);
+ await _save(medias: storage.map((e) => e.item).toList());
...
- Future<void> _save({
- required List<Media> medias,
- bool saveOnTop = false,
- }) async {
- final items = await previousItems;
- debugPrint(
- '[TESTE] itemsFromStorage: ${items.length} - mediasToSave: ${medias.length}',
- );
-
- await IsarService.instance.addPreviousPlaylistMusics(
- PreviousPlaylistMusics(musics: organizeLists(saveOnTop, items, medias)),
- );
- }
+ Future<void> _save({
+ required List<Media> medias,
+ }) async {
+ await IsarService.instance.addPreviousPlaylistMusics(
+ PreviousPlaylistMusics(musics: medias.toListStringCompressed),
+ );
+ }Also applies to: 166-166, 169-180, 183-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/player/lib/src/queue.dart` at line 132, The code maps
enqueueAfterCurrent to saveOnTop when calling _save (see calls around
enqueueAfterCurrent and _save), but _save only supports prepend/append via
organizeLists so middle insertions are not persisted and storage order diverges
from the runtime queue; update the persistence to record the exact resulting
queue order for middle inserts by either extending _save to accept an explicit
index/position (e.g., new parameter insertIndex) and persisting via
organizeLists or by constructing the full new ordered list (using the runtime
queue/state after inserting the media at the desired index) and passing that
list into _save so the stored order matches the runtime queue; make the change
where enqueueAfterCurrent is mapped (calls at lines around the enqueue paths)
and ensure organizeLists or the persistence layer receives the explicit ordered
list instead of relying solely on saveOnTop.
rodrigonepomucenosm
force-pushed
the
feature/3580-add-music-next-queue
branch
from
4b826b5 to
f5c0655
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/player/lib/src/queue.dart (2)
121-124:⚠️ Potential issue | 🔴 CriticalClamp middle-insert index before
insert/insertAll.Line 122 and Line 154 use
_index + 1directly. If_indexis stale after removals, enqueue can throwRangeError.🔧 Proposed fix
- final position = _index + 1; - storage.insert(position, QueueItem(pos, pos, media)); + final position = min(max(_index + 1, 0), storage.length); + storage.insert(position, QueueItem(pos, pos, media)); ... - final insertAt = _index + 1; - storage.insertAll(insertAt, _toQueueItems(medias, i)); + final insertAt = min(max(_index + 1, 0), storage.length); + storage.insertAll(insertAt, _toQueueItems(medias, i));Also applies to: 153-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/player/lib/src/queue.dart` around lines 121 - 124, The insert index used when enqueuing after the current item is computed as _index + 1 but can be out of bounds if _index is stale; in the enqueue logic (where enqueueAfterCurrent is checked and where storage.insert and storage.insertAll are called) clamp the computed position to the valid range 0..storage.length (e.g., position = (_index + 1).clamp(0, storage.length)) before calling storage.insert or storage.insertAll and construct QueueItem(s) with the correct pos values; update both places that compute _index + 1 (the single-item insert and the multi-item insert) to use the clamped position to avoid RangeError.
132-132:⚠️ Potential issue | 🟠 MajorPersist the resulting queue order instead of prepend/append heuristics.
Line 132 and Line 166 persist via
_save+organizeLists, which only supports top/bottom composition. That diverges from runtime order for middle insertion, and Line 166 also ignoresmediaswhenshouldRemoveFirstis true.🔧 Proposed fix
- await _save(medias: [media]); + await _save(medias: storage.map((e) => e.item).toList()); ... - await _save(medias: items, saveOnTop: enqueueAfterCurrent || saveOnTop); + await _save(medias: storage.map((e) => e.item).toList()); - Future<void> _save({ - required List<Media> medias, - bool saveOnTop = false, - }) async { - final items = await previousItems; - debugPrint( - '[TESTE] itemsFromStorage: ${items.length} - mediasToSave: ${medias.length}', - ); - - await IsarService.instance.addPreviousPlaylistMusics( - PreviousPlaylistMusics(musics: organizeLists(saveOnTop, items, medias)), - ); - } + Future<void> _save({ + required List<Media> medias, + }) async { + await IsarService.instance.addPreviousPlaylistMusics( + PreviousPlaylistMusics(musics: medias.toListStringCompressed), + ); + }Also applies to: 166-166, 169-180, 183-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/player/lib/src/queue.dart` at line 132, The current persistence uses _save + organizeLists which only handles top/bottom composition and thus mis-saves middle insertions and ignores the medias payload when shouldRemoveFirst is true; change the code paths that call _save (and any caller that relies on organizeLists) so they compute the final ordered queue array (reflecting actual runtime order after insertion/removal) and pass that explicit ordered list to _save instead of relying on prepend/append heuristics, and ensure the branch that checks shouldRemoveFirst still includes the intended medias elements when constructing that final list; update any helper signatures as needed so _save persists the full ordered list rather than only using organizeLists.packages/player/lib/src/player.dart (2)
254-257:⚠️ Potential issue | 🟠 Major
load(Media media)should load the provided media.Line 255 ignores the
mediaargument and always uses_queue.current!, which can load the wrong track (or throw when current is null).🔧 Proposed fix
Future<int> load(Media media) async => _doPlay( - _queue.current!, + media, shouldLoadOnly: true, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/player/lib/src/player.dart` around lines 254 - 257, The load(Media media) method currently ignores its media parameter and calls _doPlay(_queue.current!), which can load the wrong item or crash; change the call to pass the provided media instead (e.g., return _doPlay(media, shouldLoadOnly: true)) or, if you must update queue state, assign _queue.current = media before calling _doPlay; ensure you remove the forced _queue.current! dereference and use the media argument when invoking _doPlay.
109-109:⚠️ Potential issue | 🟠 MajorAwait queue mutations in enqueue APIs.
Line 109 and Line 119 invoke async queue operations without
await, so callers getOkbefore persistence finishes and write failures can be missed.🔧 Proposed fix
- _queue.add(media, enqueueAfterCurrent); + await _queue.add(media, enqueueAfterCurrent); ... - _queue.addAll( + await _queue.addAll( items, shouldRemoveFirst: shouldRemoveFirst, saveOnTop: saveOnTop, enqueueAfterCurrent: enqueueAfterCurrent, );Also applies to: 119-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/player/lib/src/player.dart` at line 109, The enqueue APIs call asynchronous queue mutations without awaiting them (e.g., the call to _queue.add(media, enqueueAfterCurrent)), causing the method to return before persistence completes; change these calls to await the Future returned by _queue methods and mark the enclosing enqueue methods as async/returning Future if needed so failures surface to callers. Locate every queue mutation invocation in this file (notably the _queue.add(...) call and the other queue mutation calls around the 119–124 region) and prepend await, update the method signatures to async/return Future<Result> (or the existing return type wrapped in Future), and propagate errors/results after the awaited operation completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/player/lib/src/queue.dart`:
- Around line 125-127: When reindexing the in-memory queue after inserting in
the middle, the code currently only updates item.position (the loop in storage
where you set storage[i].position = i) but fails to update
storage[i].originalPosition, which breaks unshuffle()/reorder() later; modify
both reindexing loops (the one that sets position at the insertion spot and the
similar loop around lines 157-159) to also assign storage[i].originalPosition =
i so originalPosition stays consistent with the new ordering used by unshuffle()
and reorder().
---
Duplicate comments:
In `@packages/player/lib/src/player.dart`:
- Around line 254-257: The load(Media media) method currently ignores its media
parameter and calls _doPlay(_queue.current!), which can load the wrong item or
crash; change the call to pass the provided media instead (e.g., return
_doPlay(media, shouldLoadOnly: true)) or, if you must update queue state, assign
_queue.current = media before calling _doPlay; ensure you remove the forced
_queue.current! dereference and use the media argument when invoking _doPlay.
- Line 109: The enqueue APIs call asynchronous queue mutations without awaiting
them (e.g., the call to _queue.add(media, enqueueAfterCurrent)), causing the
method to return before persistence completes; change these calls to await the
Future returned by _queue methods and mark the enclosing enqueue methods as
async/returning Future if needed so failures surface to callers. Locate every
queue mutation invocation in this file (notably the _queue.add(...) call and the
other queue mutation calls around the 119–124 region) and prepend await, update
the method signatures to async/return Future<Result> (or the existing return
type wrapped in Future), and propagate errors/results after the awaited
operation completes.
In `@packages/player/lib/src/queue.dart`:
- Around line 121-124: The insert index used when enqueuing after the current
item is computed as _index + 1 but can be out of bounds if _index is stale; in
the enqueue logic (where enqueueAfterCurrent is checked and where storage.insert
and storage.insertAll are called) clamp the computed position to the valid range
0..storage.length (e.g., position = (_index + 1).clamp(0, storage.length))
before calling storage.insert or storage.insertAll and construct QueueItem(s)
with the correct pos values; update both places that compute _index + 1 (the
single-item insert and the multi-item insert) to use the clamped position to
avoid RangeError.
- Line 132: The current persistence uses _save + organizeLists which only
handles top/bottom composition and thus mis-saves middle insertions and ignores
the medias payload when shouldRemoveFirst is true; change the code paths that
call _save (and any caller that relies on organizeLists) so they compute the
final ordered queue array (reflecting actual runtime order after
insertion/removal) and pass that explicit ordered list to _save instead of
relying on prepend/append heuristics, and ensure the branch that checks
shouldRemoveFirst still includes the intended medias elements when constructing
that final list; update any helper signatures as needed so _save persists the
full ordered list rather than only using organizeLists.
Comment on lines
+125
to
+127
| for (var i = 0; i < storage.length; ++i) { | ||
| storage[i].position = i; | ||
| } |
There was a problem hiding this comment.
Reindex originalPosition when inserting in the middle.
Lines 125-127 and 157-159 only update position. unshuffle()/reorder() depend on originalPosition, so middle-enqueued items can jump to the wrong order later.
🔧 Proposed fix
for (var i = 0; i < storage.length; ++i) {
storage[i].position = i;
+ storage[i].originalPosition = i;
}
...
for (var j = 0; j < storage.length; j++) {
storage[j].position = j;
+ storage[j].originalPosition = j;
}Also applies to: 157-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/player/lib/src/queue.dart` around lines 125 - 127, When reindexing
the in-memory queue after inserting in the middle, the code currently only
updates item.position (the loop in storage where you set storage[i].position =
i) but fails to update storage[i].originalPosition, which breaks
unshuffle()/reorder() later; modify both reindexing loops (the one that sets
position at the insertion spot and the similar loop around lines 157-159) to
also assign storage[i].originalPosition = i so originalPosition stays consistent
with the new ordering used by unshuffle() and reorder().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Create rules to validate the inclusion of a song in the correct position in the queue.
This is changed through A/B testing. If there is no A/B test in the remote config, it follows the default value that includes it at the end of the playlist.
Summary by CodeRabbit
New Features
Bug Fixes
Chores