refactor: Abstract lyric parsing & make try-catch reusable#17
Merged
Conversation
- This broke things.
- It handles the error handling if we try to do our logic on a file that doesn't exist.
📝 WalkthroughWalkthroughThe PR consolidates metadata extraction logic by introducing two utilities— ChangesLyrics parsing and error handling consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
cyanChill
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/LyricsParser.kt`:
- Around line 42-52: The code in LyricsParser (inside getLyric) reads
byteArr[0..2] without checking length, which can throw
ArrayIndexOutOfBoundsException for truncated lyric frames; add a guard that
checks byteArr.size (or length) before accessing indices 0,1,2 and handle short
frames by returning null (or a safe default) so parsing continues without
throwing inside safeExecuteOnURI; update the encodingCharSet selection (the when
branch that references byteArr[0], byteArr[1], byteArr[2]) to first verify
byteArr.size >= 3 and fall back to ISO_8859_1 or null when the guard fails.
- Around line 38-60: In LyricsParser.handleBinaryFrame, BinaryFrame.data for
USLT/SYLT must not be decoded from offset 0; the first byte is the encoding
marker and is followed by a 3-byte language tag and a null-terminated descriptor
before the actual text. Change the logic that selects encoding and builds the
String so you: 1) check byteArr.size before accessing byteArr[1]/byteArr[2] to
avoid ArrayIndexOutOfBoundsException, 2) parse and skip the encoding marker +
3-byte language tag + the descriptor (scan for the descriptor's null terminator
using the detected single- or double-byte terminator depending on encoding) to
compute the start offset of the lyric payload, 3) apply the BOM/UTF-16LE vs
UTF-16BE heuristic only to the payload slice (byteArr.copyOfRange(payloadStart,
byteArr.size)) rather than the entire array, and 4) decode the payload slice
with the chosen charset and assign to lyrics; also handle malformed/truncated
frames by returning/setting empty lyrics when bounds/terminator are missing.
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/SafeUtils.kt`:
- Around line 13-19: The ENOENT detection is brittle because it inspects
ExecutionException.message; instead, traverse the exception cause chain
(e.cause, cause.cause, ...) and check each cause's runtime class name or
qualifiedName for
"androidx.media3.datasource.FileDataSource$FileDataSourceException" (or use
instanceof if you can import the class) to decide ENOENT; update the catch
blocks in SafeUtils.kt (the ExecutionException handler that currently calls
promise.reject("ENOENT", ...)) and the catch in MetadataRetrieverModule.kt's
getBulkMetadata to use this cause-chain check and only map to
promise.reject("ENOENT", ...) or the ENOENT error mapping when a matching cause
is found, otherwise fall back to promise.reject(errorKey, e.message, e).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5228abb-c355-4b74-864f-3d551f0bf304
📒 Files selected for processing (3)
android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/LyricsParser.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/SafeUtils.kt
Comment on lines
+42
to
+52
| val encodingCharSet = when (byteArr[0].toString()) { | ||
| "0" -> Charsets.ISO_8859_1 | ||
| "1" -> { | ||
| if (byteArr[1] == BYTE_0xFE && byteArr[2] == BYTE_0xFF) { | ||
| Charsets.UTF_16BE | ||
| } else if (byteArr[1] == BYTE_0xFF && byteArr[2] == BYTE_0xFE) { | ||
| Charsets.UTF_16LE | ||
| } else { | ||
| // Heuristic guess of byte order using new line character. | ||
| // - If the conversion contains a newline character, that charset should be used. | ||
| if (String(byteArr, Charsets.UTF_16LE).contains("\n")) Charsets.UTF_16LE |
There was a problem hiding this comment.
Guard short binary frames before reading bytes 0/1/2.
A malformed or truncated lyric frame will throw ArrayIndexOutOfBoundsException here. Since getLyric now runs inside safeExecuteOnURI, one bad embedded tag turns the whole call into a rejected Promise instead of falling back to other entries or returning null.
Suggested fix
val byteArr = frame.data
+ if (byteArr.isEmpty()) return
+
// The 1st byte in the array determines the encoding in ID3.
@@
- "1" -> {
- if (byteArr[1] == BYTE_0xFE && byteArr[2] == BYTE_0xFF) {
+ "1" -> {
+ if (byteArr.size >= 3 && byteArr[1] == BYTE_0xFE && byteArr[2] == BYTE_0xFF) {
Charsets.UTF_16BE
- } else if (byteArr[1] == BYTE_0xFF && byteArr[2] == BYTE_0xFE) {
+ } else if (byteArr.size >= 3 && byteArr[1] == BYTE_0xFF && byteArr[2] == BYTE_0xFE) {
Charsets.UTF_16LE
} else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/LyricsParser.kt`
around lines 42 - 52, The code in LyricsParser (inside getLyric) reads
byteArr[0..2] without checking length, which can throw
ArrayIndexOutOfBoundsException for truncated lyric frames; add a guard that
checks byteArr.size (or length) before accessing indices 0,1,2 and handle short
frames by returning null (or a safe default) so parsing continues without
throwing inside safeExecuteOnURI; update the encodingCharSet selection (the when
branch that references byteArr[0], byteArr[1], byteArr[2]) to first verify
byteArr.size >= 3 and fall back to ISO_8859_1 or null when the guard fails.
Comment on lines
+13
to
+19
| } catch (e: ExecutionException) { | ||
| val isWantedException = | ||
| e.message?.contains("androidx.media3.datasource.FileDataSource\$FileDataSourceException") | ||
| ?: false | ||
| when (isWantedException) { | ||
| true -> promise.reject("ENOENT", "ENOENT: No such file or directory (${uri})", e) | ||
| false -> promise.reject(errorKey, e.message, e) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the Kotlin file around the reported lines
FILE="android/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/SafeUtils.kt"
echo "=== File exists? ==="
ls -l "$FILE" || true
echo
echo "=== Kotlin snippet (lines 1-120) ==="
nl -ba "$FILE" | sed -n '1,120p'
# 2) Locate safeExecuteOnURI and related usage
echo
echo "=== Search for safeExecuteOnURI ==="
rg -n "safeExecuteOnURI" -S . || true
# 3) Find JS mapping where error.name is set/used
echo
echo "=== Search for error.name mapping ==="
rg -n "error\.name" -S src . || true
# 4) If src/index.ts exists, show relevant area around the cited lines
IDX="src/index.ts"
if [ -f "$IDX" ]; then
echo
echo "=== src/index.ts snippet (lines 1-120) ==="
nl -ba "$IDX" | sed -n '1,160p'
echo
echo "=== src/index.ts around 27-60 ==="
nl -ba "$IDX" | sed -n '20,80p'
else
echo
echo "=== No src/index.ts found at $IDX ==="
ls -la src || true
fiRepository: MissingCore/react-native-metadata-retriever
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="android/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/SafeUtils.kt"
echo "=== Tool availability ==="
for t in rg grep awk sed cat ls head tail find xargs; do
command -v "$t" >/dev/null 2>&1 && echo "$t: yes" || echo "$t: no"
done
echo
echo "=== File exists? ==="
ls -l "$FILE" || true
echo
echo "=== Kotlin snippet (lines 1-180) with line numbers (awk) ==="
awk 'NR>=1 && NR<=180 {printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,180p'
echo
echo "=== Search for safeExecuteOnURI ==="
if command -v rg >/dev/null 2>&1; then
rg -n "safeExecuteOnURI" -S . || true
else
grep -RIn "safeExecuteOnURI" . || true
fi
echo
echo "=== Search for error.name mapping ==="
if command -v rg >/dev/null 2>&1; then
rg -n "error\.name" -S src . || true
else
grep -RIn "error\.name" src . || true
fi
echo
IDX="src/index.ts"
if [ -f "$IDX" ]; then
echo "=== src/index.ts snippet (lines 1-220) with line numbers (awk) ==="
awk 'NR>=1 && NR<=220 {printf "%5d\t%s\n", NR, $0}' "$IDX" | sed -n '1,220p'
echo
echo "=== src/index.ts around 20-90 ==="
awk 'NR>=20 && NR<=90 {printf "%5d\t%s\n", NR, $0}' "$IDX"
else
echo "=== No src/index.ts found at $IDX ==="
ls -la src || true
fiRepository: MissingCore/react-native-metadata-retriever
Length of output: 8914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect MetadataRetrieverModule where safeExecuteOnURI is called and where promise.reject errors are handled
FILE1="android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.kt"
echo "=== Kotlin snippet: $FILE1 (lines 1-360) ==="
awk 'NR>=1 && NR<=360 {printf "%5d\t%s\n", NR, $0}' "$FILE1" | sed -n '1,360p'
# Search for promise.reject usage
echo
echo "=== Search for promise.reject in MetadataRetrieverModule.kt ==="
rg -n "promise\.reject" "$FILE1" || true
# Search for how errors are serialized back to JS (look for map/Bundle with keys like 'code'/'message'/'name'/'data'/'errors')
echo
echo "=== Search for errors array construction in TS ==="
# try find MetadataRetriever implementation
for f in src/MetadataRetriever.ts src/MetadataRetriever.* src/**/MetadataRetriever*.ts; do :; done
# find files containing "getBulkMetadata" and "result.errors"
rg -n "getBulkMetadata|result\.errors|errors\]|\berrors\b" -S src || true
echo
echo "=== Search for NativeError / error data shape in TS ==="
rg -n "Native|reject|code|message|data|error\.name|error\.code" -S src || trueRepository: MissingCore/react-native-metadata-retriever
Length of output: 21922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show src/types/GetMetadata.ts ==="
FILE="src/types/GetMetadata.ts"
ls -l "$FILE" || true
awk 'NR>=1 && NR<=120 {printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,120p'
echo
echo "=== Show src/NativeMetadataRetriever.ts (top) ==="
FILE="src/NativeMetadataRetriever.ts"
awk 'NR>=1 && NR<=140 {printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,140p'
echo
echo "=== Show src/MetadataRetriever.ts (top) ==="
FILE="src/MetadataRetriever.ts"
awk 'NR>=1 && NR<=160 {printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,160p'
echo
echo "=== Search for Android promise.reject serialization ==="
# If there is a shared ErrorObject or mapping on native side.
rg -n "promise\.reject|ErrorObject|ResultObject\(" android/src/main/java -S || true
echo
echo "=== Show NativeMetadataRetrieverSpec.kt (top if exists) ==="
SPEC="android/src/main/java/com/cyanchill/missingcore/metadataretriever/NativeMetadataRetrieverSpec.java"
SPEC2="android/src/main/java/com/cyanchill/missingcore/metadataretriever/NativeMetadataRetrieverSpec.kt"
if [ -f "$SPEC2" ]; then
awk 'NR>=1 && NR<=220 {printf "%5d\t%s\n", NR, $0}' "$SPEC2" | sed -n '1,220p'
elif [ -f "$SPEC" ]; then
awk 'NR>=1 && NR<=220 {printf "%5d\t%s\n", NR, $0}' "$SPEC" | sed -n '1,220p'
else
echo "Spec files not found at expected paths."
# try find by name
fd -t f "NativeMetadataRetrieverSpec.*" android/src/main/java -a || true
fiRepository: MissingCore/react-native-metadata-retriever
Length of output: 6012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="android/src/main/java/com/cyanchill/missingcore/metadataretriever/models/BridgeReturnables.kt"
echo "=== Show $FILE ==="
awk 'NR>=1 && NR<=200 {printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo
# Ensure MetadataRetrieverModule already has brittle check; show that region
FILE2="android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.kt"
echo "=== Show getBulkMetadata catch region (lines ~120-170) ==="
awk 'NR>=120 && NR<=180 {printf "%5d\t%s\n", NR, $0}' "$FILE2" | sed -n '120,180p'
echo
# Search for promise rejection handling on JS side (ErrorObject / error mapping for rejected promises)
echo "=== Search TS for 'ENOENT' and for handling rejected native errors ==="
rg -n "ENOENT|ERR_" src . || trueRepository: MissingCore/react-native-metadata-retriever
Length of output: 2353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List src directory files (top 200) ==="
find src -maxdepth 2 -type f | sed -n '1,200p'
echo
echo "=== Search for getArtwork/getLyric/getR128Gain error handling ==="
rg -n "getArtwork|getLyric|getR128Gain|ENOENT|ERR_" src --hidden || true
echo
echo "=== Show src/MetadataRetriever.ts if it exists ==="
FILE="src/MetadataRetriever.ts"
if [ -f "$FILE" ]; then
awk 'NR>=1 && NR<=200 {printf "%5d\t%s\n", NR, $0}' "$FILE"
else
echo "No $FILE"
ls -ლა src
fiRepository: MissingCore/react-native-metadata-retriever
Length of output: 2342
Map ENOENT via the exception cause chain instead of ExecutionException.message substring matching.
android/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/SafeUtils.ktdecides ENOENT by checkinge.message?.contains(...), which is brittle (and can benull), so missing files can be misclassified.- The same brittle substring check is also duplicated in
android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.kt(getBulkMetadata’scatch (e: ExecutionException)), which directly impactsgetMetadata()wheresrc/index.tssetserror.namefromresult.errors[0].data.name(so ENOENT can become"ERR_METADATA").
Suggested direction
} catch (e: ExecutionException) {
- val isWantedException =
- e.message?.contains("androidx.media3.datasource.FileDataSource\$FileDataSourceException")
- ?: false
+ val isWantedException =
+ generateSequence(e as Throwable?) { it.cause }
+ .any { it?.javaClass?.name == "androidx.media3.datasource.FileDataSource\$FileDataSourceException" }
when (isWantedException) {
true -> promise.reject("ENOENT", "ENOENT: No such file or directory (${uri})", e)
false -> promise.reject(errorKey, e.message, e)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/SafeUtils.kt`
around lines 13 - 19, The ENOENT detection is brittle because it inspects
ExecutionException.message; instead, traverse the exception cause chain
(e.cause, cause.cause, ...) and check each cause's runtime class name or
qualifiedName for
"androidx.media3.datasource.FileDataSource$FileDataSourceException" (or use
instanceof if you can import the class) to decide ENOENT; update the catch
blocks in SafeUtils.kt (the ExecutionException handler that currently calls
promise.reject("ENOENT", ...)) and the catch in MetadataRetrieverModule.kt's
getBulkMetadata to use this cause-chain check and only map to
promise.reject("ENOENT", ...) or the ENOENT error mapping when a matching cause
is found, otherwise fall back to promise.reject(errorKey, e.message, e).
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
1 participant
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.
This PR:
LyricParser.ktfile (basically how we handle replay gain parsing).safeExecuteOnURIfunction which handles the try-catch logic for us.Summary by CodeRabbit
New Features
Bug Fixes