Feature/3580 add music next queue#161
Merged
rodrigonepomucenosm merged 5 commits intomasterfrom Mar 12, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Makefile Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Player as Player
participant Queue as Queue
participant Storage as Storage
participant Events as Events
Client->>Player: enqueue(item, enqueueAfterCurrent=true)
Player->>Queue: add(item, enqueueAfterCurrent=true)
alt queue non-empty
Queue->>Queue: compute insertIndex = currentIndex+1\ninsert items at insertIndex\n_fixPositions()
else queue empty
Queue->>Queue: append items
end
Queue->>Storage: _save(medias=updatedList)
Storage-->>Queue: save result
Queue-->>Player: updated positions/currentIndex
Player->>Events: emit PositionChangeEvent / DurationChangeEvent
Events-->>Client: subscribers receive updates
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)
📝 Coding Plan
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
| 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.
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.
atrope
requested changes
Mar 2, 2026
packages/player/lib/src/queue.dart
Outdated
|
|
||
| if (enqueueAfterCurrent && storage.isNotEmpty) { | ||
| final position = _index + 1; | ||
| storage.insert(position, QueueItem(pos, pos, media)); |
Member
There was a problem hiding this comment.
eu usaria o reorder. inseria no final e usaria o reorder(mais otimizado)
Contributor
Author
There was a problem hiding this comment.
Aplicado, assim fica mais performatico certo?
packages/player/lib/src/queue.dart
Outdated
| if (saveOnTop) { | ||
| if (enqueueAfterCurrent && storage.isNotEmpty) { | ||
| final insertAt = _index + 1; | ||
| storage.insertAll(insertAt, _toQueueItems(medias, i)); |
Member
There was a problem hiding this comment.
Acho que tentaria usar o reorder tb. mais modular.
Contributor
There was a problem hiding this comment.
Sim e tbm ja contempla o shuffle
Contributor
Author
There was a problem hiding this comment.
Ajustado também para o reorder.
packages/player/lib/src/queue.dart
Outdated
| } | ||
|
|
||
| await _save(medias: items, saveOnTop: saveOnTop); | ||
| await _save(medias: items, saveOnTop: enqueueAfterCurrent || saveOnTop); |
Member
There was a problem hiding this comment.
porque ao adicionar esta saveontop true?
Contributor
There was a problem hiding this comment.
saveOnTop é para o preload(Album pre carregado ao iniciar o app), quando queremos que as musicas desse album fique antes do onde parei
Contributor
Author
There was a problem hiding this comment.
Não precisa... acabei de rever aqui. Removi
| import 'package:flutter_test/flutter_test.dart'; | ||
|
|
||
| import 'package:snowplow_example/main.dart'; | ||
| import '../lib/main.dart'; |
Contributor
Author
There was a problem hiding this comment.
Ajustado
.gitignore
Outdated
| !/gradle/wrapper/gradle-wrapper.jar | ||
|
|
||
| # End of https://www.gitignore.io/api/java,swift,kotlin,android,flutter,intellij,objective-c,androidstudio | ||
| *.sh |
Contributor
Author
There was a problem hiding this comment.
Não deveria. Erro meu. Arrumado
lstonussi
requested changes
Mar 3, 2026
packages/player/lib/src/queue.dart
Outdated
| if (saveOnTop) { | ||
| if (enqueueAfterCurrent && storage.isNotEmpty) { | ||
| final insertAt = _index + 1; | ||
| storage.insertAll(insertAt, _toQueueItems(medias, i)); |
Contributor
There was a problem hiding this comment.
Sim e tbm ja contempla o shuffle
packages/player/lib/src/queue.dart
Outdated
| } | ||
|
|
||
| await _save(medias: items, saveOnTop: saveOnTop); | ||
| await _save(medias: items, saveOnTop: enqueueAfterCurrent || saveOnTop); |
Contributor
There was a problem hiding this comment.
saveOnTop é para o preload(Album pre carregado ao iniciar o app), quando queremos que as musicas desse album fique antes do onde parei
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/snowplow/example/pubspec.yaml`:
- Line 6: The pubspec SDK constraint ("sdk: \"...\"") and the cupertino_icons
dependency (^1.0.8) are incompatible; either raise the SDK lower bound to Dart
3.1+ (e.g., change the SDK constraint to ">=3.1.0 <4.0.0") to keep
cupertino_icons ^1.0.8, or downgrade cupertino_icons to a pre-Dart-3.1
compatible release (e.g., ^1.0.2) so the existing ">=2.12.0 <4.0.0" SDK range
remains valid; update the sdk line or the cupertino_icons entry in pubspec.yaml
accordingly.
In `@packages/snowplow/example/test/widget_test.dart`:
- Line 21: The predicate in the test uses a force-unwrap on Text.data (the
lambda "(Widget widget) => widget is Text && widget.data!.startsWith('Running
on:')"), which will throw for Text.rich() instances; update the predicate to
safely handle nullable data (e.g., check widget is Text and data != null before
calling startsWith, or use a null-aware check like data?.startsWith(...) == true
or coalesce to an empty string) so the test no longer throws for
Text.rich-created widgets.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignorepackages/player/lib/src/queue.dartpackages/snowplow/example/pubspec.yamlpackages/snowplow/example/test/widget_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- packages/player/lib/src/queue.dart
|
|
||
| environment: | ||
| sdk: ">=2.1.0 <3.0.0" | ||
| sdk: ">=2.12.0 <4.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/snowplow/example/pubspec.yamlRepository: SuaMusica/flutter_plugins
Length of output: 1993
🌐 Web query:
cupertino_icons 1.0.8 pub.dev dart sdk requirements
💡 Result:
cupertino_icons v1.0.8 requires Dart SDK >= 3.1 (i.e., Flutter that ships with Dart 3.1; minimum stated as Flutter 3.13 / Dart 3.1). [1] [2]
Typical pubspec constraint: environment: sdk: ">=3.1.0 <4.0.0" (Dart 3.1+). [2]
Sources:
[1] https://pub.dev/packages/cupertino_icons/changelog
[2] https://pub.dev/packages/cupertino_icons/versions
Align SDK constraints with cupertino_icons 1.0.8 requirements.
Line 6 declares Dart >=2.12.0, but Line 14 pins cupertino_icons ^1.0.8, which requires Dart 3.1+. This creates an invalid supported range that causes flutter pub get failures for Dart 2.12–3.0.
Suggested fix (option A: keep 1.0.8)
environment:
- sdk: ">=2.12.0 <4.0.0"
+ sdk: ">=3.1.0 <4.0.0"Alternatively, use an older cupertino_icons version compatible with Dart 2.12 support.
📝 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
| sdk: ">=2.12.0 <4.0.0" | |
| sdk: ">=3.1.0 <4.0.0" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/snowplow/example/pubspec.yaml` at line 6, The pubspec SDK constraint
("sdk: \"...\"") and the cupertino_icons dependency (^1.0.8) are incompatible;
either raise the SDK lower bound to Dart 3.1+ (e.g., change the SDK constraint
to ">=3.1.0 <4.0.0") to keep cupertino_icons ^1.0.8, or downgrade
cupertino_icons to a pre-Dart-3.1 compatible release (e.g., ^1.0.2) so the
existing ">=2.12.0 <4.0.0" SDK range remains valid; update the sdk line or the
cupertino_icons entry in pubspec.yaml accordingly.
| find.byWidgetPredicate( | ||
| (Widget widget) => widget is Text && | ||
| widget.data.startsWith('Running on:'), | ||
| (Widget widget) => widget is Text && widget.data!.startsWith('Running on:'), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file to understand the context
find . -name widget_test.dart -path "*/snowplow/example/test/*" -type fRepository: SuaMusica/flutter_plugins
Length of output: 118
🌐 Web query:
Flutter Text widget API documentation - is Text.data nullable and null for Text.rich constructor
💡 Result:
Yes.
- In the Flutter API,
Text.datais declared asfinal String? data(nullable). The docs explicitly say it “will be null if atextSpanis provided instead.” [1] - The
Text.rich(InlineSpan textSpan, ...)constructor’s implementation setsdata = null(and usesthis.textSpan). [2] - Conversely, the default
Text(String data, ...)constructor takes a non-nullableStringand usesdata(withtextSpanbeing null). [3]
Sources:
[1] (api.flutter.dev)
[2] (api.flutter.dev)
[3] (api.flutter.dev)
Citations:
- 1: https://api.flutter.dev/flutter/widgets/Text/data.html?utm_source=openai
- 2: https://api.flutter.dev/flutter/widgets/Text/Text.rich.html?utm_source=openai
- 3: https://api.flutter.dev/flutter/widgets/Text/Text.html?utm_source=openai
🏁 Script executed:
# Read the test file to understand the context around line 21
cat -n ./packages/snowplow/example/test/widget_test.dart | head -40Repository: SuaMusica/flutter_plugins
Length of output: 1132
Avoid force-unwrapping nullable Text.data in predicate.
On Line 21, widget.data! will throw for Text instances created with Text.rich() (which sets data to null), making this test fragile if the widget tree contains any such widgets.
Proposed 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 predicate in
the test uses a force-unwrap on Text.data (the lambda "(Widget widget) => widget
is Text && widget.data!.startsWith('Running on:')"), which will throw for
Text.rich() instances; update the predicate to safely handle nullable data
(e.g., check widget is Text and data != null before calling startsWith, or use a
null-aware check like data?.startsWith(...) == true or coalesce to an empty
string) so the test no longer throws for Text.rich-created widgets.
| import 'package:flutter_test/flutter_test.dart'; | ||
|
|
||
| import 'package:mediascanner_example/main.dart'; | ||
| import '../lib/main.dart'; |
Member
There was a problem hiding this comment.
full import - como o trope havia sugerido (é o padrão que usamos)
| import 'package:flutter_test/flutter_test.dart'; | ||
|
|
||
| import 'package:migration_example/main.dart'; | ||
| import '../lib/main.dart'; |
atrope
requested changes
Mar 12, 2026
Member
atrope
left a comment
atrope
left a comment
There was a problem hiding this comment.
Fazendo o review pensei em edge cases e nao gostei do que vi.
Método add:
storage.add(QueueItem(pos, pos, media));
if (enqueueAfterCurrent && storage.length > 1) {
reorder(storage.length - 1, _index + 1);
}Adiciona no final e depois chama reorder, que faz ordenação O(n) + ajuste de posições. Para um único item até tá ok!
Método addAll:
storage.addAll(_toQueueItems(medias, i));
for (var k = 0; k < medias.length; k++) {
reorder(oldLength + k, insertAt + k);
}Esse é o problemático... chama reorder uma vez por item, cada chamada disparando um sort O(n log n). Com 100 músicas, são 100 sorts.
Abordagem otimizada pra ambos:
Em vez de adicionar no final e reordenar, usa insertAll direto na posição alvo e corrige as posições em uma única passagem O(n):
// add
if (enqueueAfterCurrent && storage.length > 1) {
storage.insert(_index + 1, QueueItem(_index + 1, _index + 1, media));
} else {
storage.add(QueueItem(pos, pos, media));
}
// uma única passagem pra corrigir posições
for (var j = 0; j < storage.length; j++) {
storage[j].position = j;
storage[j].originalPosition = j;
}
// addAll
if (enqueueAfterCurrent && storage.isNotEmpty) {
storage.insertAll(_index + 1, _toQueueItems(medias, _index));
} else { ... }
// uma passagem O(n) em vez de n sorts
for (var j = 0; j < storage.length; j++) {
storage[j].position = j;
storage[j].originalPosition = j;
}Da até para o for do fix position ser uma outra funcao auxiliar pois usamos em dois lugares(pelo menos) Nao pensei muito quando adiciona Shufflado..
O ponto principal: o reorder foi feito pra reordenação por drag-and-drop de itens já existentes. Para inserção, usar insert/insertAll direto + uma passagem de correção de posições é O(n) contra O(n² log n) Muito pesado. do jeito atual.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/player/lib/src/queue.dart (3)
191-207:⚠️ Potential issue | 🔴 CriticalRe-sync
_indexand_currentafter deletions.If the deleted set contains the current item or the tail, this leaves both pointers stale. Removing the last current item makes
_index == storage.length, so the nextenqueueAfterCurrentcan compute an out-of-range insert; removing the current middle item leavescurrentpointing at a track that is no longer instorage.Suggested fix
for (var j = 0; j < storage.length; ++j) { storage[j].position = j; } + + if (storage.isEmpty) { + _index = 0; + _current = null; + } else { + setIndex = min(_index, storage.length - 1); + }🤖 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 191 - 207, The removeByPosition method currently mutates storage but doesn't re-sync the queue pointers; after deletions ensure the internal pointers (_index and _current) are valid: clamp _index to be within 0..storage.length-1 (or -1/nullable if storage is empty) and update _current to point to storage[_index] when a valid index exists, otherwise set _current to null; do this inside removeByPosition (after you reassign positions) so subsequent operations like enqueueAfterCurrent use a valid index/current; reference the removeByPosition method, the _index/_current fields, and enqueueAfterCurrent when making the change.
115-129:⚠️ Potential issue | 🟠 MajorPersist the post-mutation queue, not
previousItems + medias.Line 146 filters the runtime insertion set into
medias, but Line 160 still savesitems, soshouldRemoveFirstcan resurrect the skipped first track on restore. Separately, Line 167 rebuilds from disk and Lines 172-174 only model prepend/append, so the newenqueueAfterCurrentpath persists a different order thanstorage.Suggested fix
- await _save(medias: [media]); + await _saveCurrentQueue(); ... - await _save(medias: items, saveOnTop: saveOnTop); + await _saveCurrentQueue(); ... + Future<void> _saveCurrentQueue() async { + await IsarService.instance.addPreviousPlaylistMusics( + PreviousPlaylistMusics( + musics: storage.map((e) => e.item).toList().toListStringCompressed, + ), + ); + }Also applies to: 140-160, 163-188
🤖 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 115 - 129, The add(...) method currently persists the wrong set (previousItems + medias) causing restored queue order to differ from runtime storage when enqueueAfterCurrent is used; update the persistence to save the post-mutation queue state — e.g. use the current storage/items sequence (or rebuild a list from storage) when calling _save instead of the earlier items+medias set so that storage, _fixPositions(), and the new QueueItem entries are what get written; ensure the branch that inserts at insertAt and the else branch both end up passing the mutated storage (or its equivalent ordered list) into _save so shouldRemoveFirst/restore logic won't resurrect the skipped first track.
119-122:⚠️ Potential issue | 🟠 Major
_fixPositions()is wiping the queue's shuffle metadata.
shuffle()deliberately preservesoriginalPosition, but Line 264 rewrites it for every item. Calling this helper from the newenqueueAfterCurrentandsaveOnTopbranches erases the pre-shuffle order, so a laterunshuffle()cannot reliably restore the original sequence.Also applies to: 149-155, 261-265
🤖 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 119 - 122, _fixPositions() is currently resetting every QueueItem's originalPosition which destroys shuffle state; modify it so it updates only the runtime positional fields (like index/position/currentIndex) and does not overwrite originalPosition if it's already set (i.e., only set originalPosition when creating a brand new item if none exists). Update places that create QueueItem (notably enqueueAfterCurrent and saveOnTop where QueueItem(insertAt, insertAt, media) is used) to supply originalPosition explicitly when appropriate (or leave it null so _fixPositions won't overwrite it). Keep shuffle() and unshuffle() logic intact so unshuffle() can rely on preserved originalPosition. Ensure references: _fixPositions(), enqueueAfterCurrent, saveOnTop, shuffle(), unshuffle(), and QueueItem are updated accordingly.
🧹 Nitpick comments (1)
packages/migration/example/test/widget_test.dart (1)
20-21: Consider using null-safe operators instead of force-unwrap.Using
widget.data!will throw if anyTextwidget in the tree has nulldata(e.g., when usingText.rich()). A null-safe approach is more robust:♻️ Suggested safer alternative
expect( find.byWidgetPredicate( (Widget widget) => - widget is Text && widget.data!.startsWith('Running on:'), + widget is Text && (widget.data?.startsWith('Running on:') ?? false), ), findsOneWidget, );🤖 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` around lines 20 - 21, The predicate in the test uses a force-unwrap (widget.data!) which can throw for Text widgets with null data; update the predicate used in the matcher (the lambda that checks "(Widget widget) => widget is Text && widget.data!.startsWith('Running on:')") to use null-safe operators — e.g. use widget.data?.startsWith('Running on:') == true or (widget.data?.startsWith('Running on:') ?? false) — so it safely handles null Text.data values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/player/lib/src/queue.dart`:
- Around line 191-207: The removeByPosition method currently mutates storage but
doesn't re-sync the queue pointers; after deletions ensure the internal pointers
(_index and _current) are valid: clamp _index to be within 0..storage.length-1
(or -1/nullable if storage is empty) and update _current to point to
storage[_index] when a valid index exists, otherwise set _current to null; do
this inside removeByPosition (after you reassign positions) so subsequent
operations like enqueueAfterCurrent use a valid index/current; reference the
removeByPosition method, the _index/_current fields, and enqueueAfterCurrent
when making the change.
- Around line 115-129: The add(...) method currently persists the wrong set
(previousItems + medias) causing restored queue order to differ from runtime
storage when enqueueAfterCurrent is used; update the persistence to save the
post-mutation queue state — e.g. use the current storage/items sequence (or
rebuild a list from storage) when calling _save instead of the earlier
items+medias set so that storage, _fixPositions(), and the new QueueItem entries
are what get written; ensure the branch that inserts at insertAt and the else
branch both end up passing the mutated storage (or its equivalent ordered list)
into _save so shouldRemoveFirst/restore logic won't resurrect the skipped first
track.
- Around line 119-122: _fixPositions() is currently resetting every QueueItem's
originalPosition which destroys shuffle state; modify it so it updates only the
runtime positional fields (like index/position/currentIndex) and does not
overwrite originalPosition if it's already set (i.e., only set originalPosition
when creating a brand new item if none exists). Update places that create
QueueItem (notably enqueueAfterCurrent and saveOnTop where QueueItem(insertAt,
insertAt, media) is used) to supply originalPosition explicitly when appropriate
(or leave it null so _fixPositions won't overwrite it). Keep shuffle() and
unshuffle() logic intact so unshuffle() can rely on preserved originalPosition.
Ensure references: _fixPositions(), enqueueAfterCurrent, saveOnTop, shuffle(),
unshuffle(), and QueueItem are updated accordingly.
---
Nitpick comments:
In `@packages/migration/example/test/widget_test.dart`:
- Around line 20-21: The predicate in the test uses a force-unwrap
(widget.data!) which can throw for Text widgets with null data; update the
predicate used in the matcher (the lambda that checks "(Widget widget) => widget
is Text && widget.data!.startsWith('Running on:')") to use null-safe operators —
e.g. use widget.data?.startsWith('Running on:') == true or
(widget.data?.startsWith('Running on:') ?? false) — so it safely handles null
Text.data values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ab366e9-3e5d-4ddb-bc72-4ffaa36c8e96
📒 Files selected for processing (9)
packages/mediascanner/example/lib/main.dartpackages/mediascanner/example/pubspec.yamlpackages/mediascanner/example/test/widget_test.dartpackages/mediascanner/pubspec.yamlpackages/migration/example/pubspec.yamlpackages/migration/example/test/widget_test.dartpackages/migration/pubspec.yamlpackages/player/example/lib/sm_player.dartpackages/player/lib/src/queue.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mediascanner/example/test/widget_test.dart
felipecastrosales
approved these changes
Mar 12, 2026
atrope
approved these changes
Mar 12, 2026
rodrigonepomucenosm
force-pushed
the
feature/3580-add-music-next-queue
branch
from
fe371a7 to
2850098
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/player/lib/src/queue.dart (2)
130-130:⚠️ Potential issue | 🟠 MajorPersist the actual queue order, not a reconstructed append/prepend list.
After
enqueueAfterCurrent, the in-memory queue is updated in the middle, but_savestill serializespreviousItemsplusmediasas prepend/append only. That means restore order diverges fromstorage. Line 168 also passesitemsinstead ofmedias, soshouldRemoveFirstcan persist a song that was never enqueued.🔧 Proposed fix
- await _save(medias: [media]); + await _save(medias: storage.map((e) => e.item).toList());- await _save(medias: items, saveOnTop: 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)), + PreviousPlaylistMusics(musics: medias.toListStringCompressed), ); }Also applies to: 168-182
🤖 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 130, The current persistence logic in _save is serializing previousItems plus medias as simple prepend/append which breaks when enqueueAfterCurrent mutates the in-memory queue in the middle; update _save to persist the actual queue order (the live items list) rather than reconstructing from previousItems and medias, and ensure callers use the correct arguments (e.g., when enqueueAfterCurrent calls _save pass the full updated items list, not medias or previousItems). Specifically, modify calls around enqueueAfterCurrent and the call at line 168 to pass the current in-memory queue (items) into _save, remove reliance on shouldRemoveFirst persisting a media that was never enqueued, and adjust _save to accept/serialize the provided items list as the canonical persisted order.
120-123:⚠️ Potential issue | 🔴 CriticalClamp the enqueue-after-current insert index.
Line 121 and Line 151 still trust
_index + 1. If the current item was removed earlier,_indexcan be stale and theseinsertcalls will throwRangeError, breaking enqueue.🔧 Proposed fix
- final insertAt = _index + 1; + final insertAt = min(max(_index + 1, 0), storage.length); final originalPos = _maxOriginalPosition() + 1; storage.insert(insertAt, QueueItem(originalPos, insertAt, media));- final insertAt = _index + 1; + final insertAt = min(max(_index + 1, 0), storage.length); var originalPos = _maxOriginalPosition(); final newItems = medias.map((e) { originalPos++; return QueueItem(originalPos, 0, e); }).toList(); storage.insertAll(insertAt, newItems);Also applies to: 150-157
🤖 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 120 - 123, The insert index computed as `_index + 1` for enqueue-after-current can be stale and out of range; update the logic in the enqueue path(s) that call `storage.insert(insertAt, QueueItem(...))` (the block using `enqueueAfterCurrent`, `_index`, `_maxOriginalPosition()`, and the other similar block around the later `insert`) to clamp `insertAt` into the valid range [0, storage.length] before calling `storage.insert` (e.g. compute a bounded insertAt via clamp/min-max), then create the `QueueItem(originalPos, insertAt, media)` and insert; apply this same clamping fix to both occurrences that currently trust `_index + 1`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/player/lib/src/queue.dart`:
- Line 130: The current persistence logic in _save is serializing previousItems
plus medias as simple prepend/append which breaks when enqueueAfterCurrent
mutates the in-memory queue in the middle; update _save to persist the actual
queue order (the live items list) rather than reconstructing from previousItems
and medias, and ensure callers use the correct arguments (e.g., when
enqueueAfterCurrent calls _save pass the full updated items list, not medias or
previousItems). Specifically, modify calls around enqueueAfterCurrent and the
call at line 168 to pass the current in-memory queue (items) into _save, remove
reliance on shouldRemoveFirst persisting a media that was never enqueued, and
adjust _save to accept/serialize the provided items list as the canonical
persisted order.
- Around line 120-123: The insert index computed as `_index + 1` for
enqueue-after-current can be stale and out of range; update the logic in the
enqueue path(s) that call `storage.insert(insertAt, QueueItem(...))` (the block
using `enqueueAfterCurrent`, `_index`, `_maxOriginalPosition()`, and the other
similar block around the later `insert`) to clamp `insertAt` into the valid
range [0, storage.length] before calling `storage.insert` (e.g. compute a
bounded insertAt via clamp/min-max), then create the `QueueItem(originalPos,
insertAt, media)` and insert; apply this same clamping fix to both occurrences
that currently trust `_index + 1`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4a948c7-1dbf-4dfa-988c-ec45f46c7752
📒 Files selected for processing (10)
packages/mediascanner/example/lib/main.dartpackages/mediascanner/example/pubspec.yamlpackages/mediascanner/example/test/widget_test.dartpackages/mediascanner/pubspec.yamlpackages/migration/example/pubspec.yamlpackages/migration/example/test/widget_test.dartpackages/migration/pubspec.yamlpackages/player/example/lib/sm_player.dartpackages/player/lib/src/queue.dartpackages/player/test/queue_test.dart
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/player/example/lib/sm_player.dart
- packages/migration/pubspec.yaml
- packages/mediascanner/example/test/widget_test.dart
- packages/migration/example/pubspec.yaml
- packages/mediascanner/example/lib/main.dart
- packages/mediascanner/example/pubspec.yaml
- packages/migration/example/test/widget_test.dart
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
5 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
Tests