Repository navigation
fix(local): restore scanned audio playback and metadata writes - #551
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868b5a54b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val localFiles = LocalFilesPlaylist.firstOrNull(_playlists.value) ?: return false | ||
| return localFiles.songs.any { song -> | ||
| preferredLocalMediaReference(song.localFilePath, song.mediaUri) == reference | ||
| } |
There was a problem hiding this comment.
When a user chooses the scan preview's secondary “add to playlist” action, createPlaylistWithScannedSongs or addScannedSongsToPlaylist persists the selected file only in a normal playlist, but this lookup searches only the Local Files system playlist. If that file's sidecar restores a remote identity, playback classifies it as a managed download; with a different configured download root, the exact scanned SAF URI is then rejected rather than played (and a missing file may incorrectly recover the remote source). Include scanned entries in user playlists in the explicit-source evidence, or persist their manual-import provenance separately.
Useful? React with 👍 / 👎.
| ) | ||
| } | ||
| } | ||
| ?: grantedDocumentSource(song, context) |
There was a problem hiding this comment.
When a stored MediaStore URI or raw path is unreadable under scoped storage, this fallback returns a newly constructed document URI from the persisted folder grant. For a manually scanned song whose sidecar restored a remote identity, the download coordinator treats the song as managed and asks peekExplicitLocalFileReference about this new URI; that cache contains only the original preferred reference, so it returns false and the readable recovered URI is rejected whenever the configured download root differs. Carry verified alias provenance into the explicit-reference check (or update the stored source) so the new folder-grant recovery also works for the remote-identity songs this change targets.
Useful? React with 👍 / 👎.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.87% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 343 functions across 55 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check 标题准确概括了 PR 的主要变更：恢复扫描音频的播放和元数据写入。标题简洁、具体，且与变更范围相关。 Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.87% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 343 functions across 55 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: cwuom/NeriPlayer/.coderabbit.yaml
- Review profile: QUIET
- Plan: Advanced
- Run ID:
87e3472f-786a-474d-9ff3-eadd32ed3b25
⛔ Files ignored due to path filters: 1
modules/local/README.mdis excluded by!**/*.md
📒 Files selected for processing: 36
app/src/androidTest/java/moe/ouom/neriplayer/core/download/execution/LocalSongDownloadAdmissionTest.ktapp/src/androidTest/java/moe/ouom/neriplayer/data/local/audioimport/LocalAudioImportStandaloneFlacTest.ktapp/src/androidTest/java/moe/ouom/neriplayer/data/local/media/LocalMediaStorePlaybackReferenceTest.ktapp/src/androidTest/java/moe/ouom/neriplayer/ui/screen/playlist/LocalPlaylistDownloadSelectionTest.ktapp/src/main/java/moe/ouom/neriplayer/ui/screen/playlist/LocalArtistDetailScreen.ktapp/src/main/java/moe/ouom/neriplayer/ui/screen/playlist/LocalPlaylistDetailModernContent.ktapp/src/main/java/moe/ouom/neriplayer/ui/screen/playlist/PlaylistDownloadCandidates.ktapp/src/test/java/moe/ouom/neriplayer/ui/screen/playlist/PlaylistDownloadCandidatesTest.ktmodules/common/src/main/java/moe/ouom/neriplayer/common/logging/PlaybackLogReference.ktmodules/common/src/test/java/moe/ouom/neriplayer/common/logging/PlaybackLogReferenceTest.ktmodules/download/logic/src/main/java/moe/ouom/neriplayer/core/download/storage/reference/ManagedDownloadReferenceIo.ktmodules/download/logic/src/test/java/moe/ouom/neriplayer/core/download/storage/reference/ManagedDownloadReferenceIoTest.ktmodules/download/runtime/src/main/java/moe/ouom/neriplayer/core/download/manager/batch/GlobalDownloadManagerBatchPrepare.ktmodules/download/runtime/src/main/java/moe/ouom/neriplayer/core/download/manager/runtime/GlobalDownloadManagerRequest.ktmodules/download/runtime/src/main/java/moe/ouom/neriplayer/core/download/storage/facade/ManagedDownloadStorageFacadeCatalog.ktmodules/download/runtime/src/main/java/moe/ouom/neriplayer/core/player/download/playback/AudioDownloadPlaybackCoordinator.ktmodules/download/runtime/src/test/java/moe/ouom/neriplayer/core/download/ManagedDownloadClassificationDiagnosticsTest.ktmodules/download/runtime/src/test/java/moe/ouom/neriplayer/core/player/download/playback/AudioDownloadExternalScannedPlaybackTest.ktmodules/download/runtime/src/test/java/moe/ouom/neriplayer/core/player/download/playback/AudioDownloadPlaybackDiagnosticsTest.ktmodules/local/src/main/java/moe/ouom/neriplayer/data/local/audioimport/LocalAudioImportManagerSetup.ktmodules/local/src/main/java/moe/ouom/neriplayer/data/local/media/LocalMediaSupport.ktmodules/local/src/main/java/moe/ouom/neriplayer/data/local/media/LocalMediaSupportFacadeMetadata.ktmodules/local/src/main/java/moe/ouom/neriplayer/data/local/playlist/LocalPlaylistRepository.ktmodules/local/src/test/java/moe/ouom/neriplayer/data/local/audioimport/LocalAudioImportMediaStoreSafReferenceTest.ktmodules/local/src/test/java/moe/ouom/neriplayer/data/local/media/LocalMediaLyricTransactionTest.ktmodules/local/src/test/java/moe/ouom/neriplayer/data/local/playlist/LocalPlaylistExplicitLocalReferenceTest.ktmodules/playback/runtime/src/main/java/moe/ouom/neriplayer/core/player/media/LocalPlaybackMediaResolver.ktmodules/playback/runtime/src/main/java/moe/ouom/neriplayer/core/player/persistence/PlayerManagerPersistenceExtensions.ktmodules/playback/runtime/src/main/java/moe/ouom/neriplayer/core/player/playback/PlaybackFailureRecoveryOwner.ktmodules/playback/runtime/src/main/java/moe/ouom/neriplayer/core/player/url/PlayerLocalPlaybackDiagnostics.ktmodules/playback/runtime/src/main/java/moe/ouom/neriplayer/core/player/url/PlayerLocalPlaybackRecovery.ktmodules/playback/runtime/src/main/java/moe/ouom/neriplayer/core/player/url/PlayerManagerUrlExtensions.ktmodules/playback/runtime/src/test/java/moe/ouom/neriplayer/core/player/media/LocalPlaybackDiagnosticsTest.ktmodules/playback/runtime/src/test/java/moe/ouom/neriplayer/core/player/media/LocalPlaybackMediaResolverTest.ktmodules/playback/runtime/src/test/java/moe/ouom/neriplayer/core/player/persistence/PlayerManagerLocalMetadataEditRegressionTest.ktmodules/playback/runtime/src/test/java/moe/ouom/neriplayer/core/player/url/PlayerLocalPlaybackRecoveryTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbd48b3402
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (compatible && (verifiedAnchor == null || verifiedAnchor == reference)) { | ||
| return null |
There was a problem hiding this comment.
When the configured SAF root is absent, every reference is considered compatible; references inside the configured tree are compatible as well. This early return therefore skips explicitLocalFileReference even when the exact preferred reference was persisted from a scan. For a scanned song whose sidecar supplied a remote identity, the subsequent completed-reference bridge can replace the user-selected file with another download of that identity before the selected reference is probed, and a missing selected file can enter managed rebind logic. Check explicit provenance before applying this compatibility shortcut so exact scanned sources retain the same protection as recovered aliases.
Useful? React with 👍 / 👎.
| val replacesRemoteSource = allowScannedSourceReplacement && updatedExplicitReference != null && | ||
| !LocalSongSupport.isLocalSong(currentSong, null) && currentSong.sameIdentityAs(newSongInfo) |
There was a problem hiding this comment.
When an ordinary playlist already contains a legacy or managed local entry with the same remote identity but no valid explicit marker, adding a newly scanned file reaches this merge with allowScannedSourceReplacement = true. Because the current entry is already considered local, this condition remains false; shouldPreserveEntryPlaybackSource then sees two different local references and copies the old playback fields back, silently discarding both the newly selected file and its provenance. Permit replacement when the current local entry has no valid explicit provenance, while retaining the existing protection for an already selected explicit file.
Useful? React with 👍 / 👎.
| return listOfNotNull(preferred, song.localFilePath, song.mediaUri) | ||
| .distinct() | ||
| .any { isRestorable(it, context) } | ||
| .any { isRestorable(it, context) } || ResolvedLocalMediaSource.recoverFromFolderGrant(context, song) != null |
There was a problem hiding this comment.
When all stored references are revoked, this fallback calls recoverFromFolderGrant, which can synchronously query MediaStore and open a document descriptor. isRestorableSong is reached through transportSnapshot(includeRestore = true) during PlayerStartupBootstrapper's main-dispatcher LaunchedEffect, and the same bootstrap predicate is also invoked from the Compose service-start callback, so a slow or stalled DocumentsProvider can block startup UI or trigger an ANR. Perform the recovery probe on Dispatchers.IO or consume an asynchronously prepared/cache-only result from this synchronous predicate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ff6fd146a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| PlayerManager.scheduleRestoredPlaybackResume(serviceScope) { | ||
| NPLogger.w("NERI-APS", "Restored playback after process restart") | ||
| updateAll() | ||
| } |
There was a problem hiding this comment.
When the restored queue points to a deleted raw path or file: URI, the new lightweight bootstrap check admits it and ensureForegroundStarted() runs, but this job returns null after its IO probe and invokes no callback. Execution has already selected START_STICKY because the queue still has items, so the user is left with an unnecessary foreground playback service and notification until the default 60-minute idle timeout. Handle the unsuccessful completion by disabling restart and stopping the foreground service while preserving the pending restore for a later retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e07ea6b1db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val conflicting = readable != null && readable != preferred && | ||
| ResolvedLocalMediaSource.hasConflictingDeclaredFile(context, song) | ||
| val resolved = if (conflicting) null else { | ||
| readable?.let { ResolvedLocalMediaSource.direct(song, it) } |
There was a problem hiding this comment.
When the preferred content URI has become unreadable but localFilePath still opens the same file (for example on devices with legacy filesystem access), the preceding conflict check verifies that the two declarations resolve to the same canonical file, but this line creates a direct source whose anchor is the fallback path itself. unverifiedDeclaredReferenceFor consequently treats the preferred URI as unverified, and the coordinator rejects the otherwise readable fallback whenever that preferred URI is recorded as an explicit scanned source. Carry the verified preferred reference as the anchor in this non-conflicting-alias case so revoked provider access does not make the scanned song unplayable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b44dc94592
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -110,7 +111,7 @@ fun PlayedEntry.toSongItem(): SongItem { | |||
| album = album, | |||
| durationMs = durationMs, | |||
| coverUrl = coverUrl, | |||
There was a problem hiding this comment.
When a scanned song whose sidecar restores a remote identity is later opened from Recent after its last playlist membership was removed, toPlayedEntry has discarded explicitLocalSourceReference and this reconstruction restores only its URI/path. peekExplicitLocalFileReference consequently returns false, so the download coordinator can classify the history item as managed and reject or replace the readable user-selected file when it lies outside the configured download root. Persist and restore the explicit-source marker with PlayedEntry so history remains independently playable.
Useful? React with 👍 / 👎.
| private suspend fun PlayerManager.restoredSourceAvailable(song: SongItem): Boolean { | ||
| if (!isLocalSong(song)) return true | ||
| return try { | ||
| withContext(Dispatchers.IO) { isReadableLocalMediaUri(localMediaSource(song)) }.also { available -> |
There was a problem hiding this comment.
When the restored song is a managed download whose persisted reference still points to the pre-migration directory, this direct readability check returns false even if the download catalog already contains a valid replacement in the new root. Normal playback handles exactly this case through the coordinator's managed-reference rebind, but restore validation never reaches it, so automatic resume is rejected and the service may be stopped despite an immediately playable source. Resolve restored managed songs through the same permitted-local-playback path, or distinguish recoverable managed references before declaring the source unavailable.
Useful? React with 👍 / 👎.
带入上游： - fix(ui): preserve page state and back navigation across rotation (cwuom#570) - fix(download): prevent cancelled mobile batches from restoring (cwuom#566) - fix(nowplaying): sync matched favorites and refine song sheets (cwuom#562) - feat(lyrics): add ColorOS 16/17 lockscreen lyrics with Root and Shizuku (cwuom#563) - fix(lyrics): preserve Bluetooth lyrics in display metadata (cwuom#560) - fix(ui): stabilize cover morphs and improve Clean switch contrast (cwuom#561) - fix(netease): isolate rejected songs during playlist sync (cwuom#559) - feat(backup): protect configuration exports and warn on HTTP WebDAV (cwuom#557) - perf(ui): defer expensive UI work and complete Clean theme styling (cwuom#556) - fix(auth): persist accounts when Android Keystore is unavailable (cwuom#554) - fix(lyrics): apply translation mode before publishing songs (cwuom#553) - fix(local): restore scanned audio playback and metadata writes (cwuom#551) 与心动模式改动的重叠文件（NowPlayingScreen、NowPlayingQueueSheet、 MoreOptionsMainContent、PersistedPlayerState、PersistedPlayerStateMapping、 PlayerManager、PlayerManagerPersistenceExtensions、NeteaseClient）均为自动合并， 无冲突；heartbeatMode 持久化字段与恢复调用保持完整。
描述 / Description
Folder-scanned songs restored remote identities from
.npmeta.jsonand were treated as managed downloads. Playback failed when the chosen local file was outside the configured download tree or its old MediaStore/path reference needed recovery through a persisted folder grant.Persist explicit scan provenance for both the Local Files system playlist and ordinary user playlists. Match it against the exact preferred reference, preserve it for surviving members during same-device sync application, and honor remote member/playlist deletions. Sync payloads omit local paths and provenance. Playback history and both main/shuffle queues keep the exact explicit-source marker beside their selected reference through JSON, Room, metadata updates, and incremental position writes. Room schema 21 adds nullable marker columns for history and queue entries without inventing provenance for old records. Same-device sync retains path-only local history; sync and backup projections exclude local provenance.
Carry a source context bound to the complete song snapshot and actual playback reference through playback admission, retries, metadata editing, and remote recovery. A recovered folder alias must prove the same canonical file and granted volume/directory; MediaStore proof requires a specific audio item and exactly one data row. Explicit or unknown manual provenance prevents same-identity download bridges and automatic remote replacement, including exact references inside the current download root and references used when no root is configured. An explicit rescan can replace an unmarked legacy or managed source in an ordinary playlist, while a valid existing explicit source remains protected.
类型 / Type
修复或解决的问题 / Issues Fixed or Closed by This PR
Follow-up to #545 for manually scanned audio failing to resolve a playback URL, local songs exposing download actions, and inconsistent metadata/artwork sidecar writes.
清单 / Checklist
其他信息 / Additional Information
Validation after the latest review fixes:
6d1b8de11: JVM/CRAP/lint, Worker, Listen Together, all three Android test groups, Native USB verification, and the unsigned Release build passed. CI Android reports contain 1,400 cases with zero actual failures/errors and five prerequisite assumption skips; all 14 migration cases that failed in the preceding run now pass.Earlier physical-device validation, before these review follow-ups: API 36 PLK110 played the previously failing track and advanced from about 12.1 s to 17.6 s after a signed release upgrade. The latest follow-up was validated on the emulator and was not reinstalled on the physical device. Local download action visibility/admission were also covered by instrumented tests during the original implementation; screenshots are not attached.