fix(fullAppDisplay): use GraphQL to fetch album date#3733
Open
cvius wants to merge 3 commits intospicetify:mainfrom
Open
fix(fullAppDisplay): use GraphQL to fetch album date#3733cvius wants to merge 3 commits intospicetify:mainfrom
cvius wants to merge 3 commits intospicetify:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughgetAlbumDate() in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Extensions/fullAppDisplay.js (1)
471-471: Redundantawaiton already resolved value.
responseis already the resolved value fromawait ...send()on line 469. The additionalawaithere is unnecessary.♻️ Suggested fix
- const albumInfo = await response; - if (!albumInfo?.ok) return null; + if (!response?.ok) return null; + + const albumDate = response.body?.date;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/fullAppDisplay.js` at line 471, Remove the redundant await when assigning albumInfo: the variable response is already resolved (from the prior await ...send()), so change the assignment in Extensions/fullAppDisplay.js that uses albumInfo and response to a direct assignment (albumInfo = response) rather than awaiting response; update references to the variable if needed and ensure no other code depends on response being a Promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Extensions/fullAppDisplay.js`:
- Around line 462-469: Wrap the Spicetify.Platform.RequestBuilder usage in a
try-catch and guard against the RequestBuilder being undefined: verify
Spicetify.Platform && Spicetify.Platform.RequestBuilder exist before calling
RequestBuilder.build(), then call
build().withHost(...).withPath(path).withEndpointIdentifier(path).send() inside
a try block and handle failures in catch by logging the error (or returning a
safe fallback) so the caller won’t crash; keep using Spicetify.URI.idToHex(id)
for path construction but ensure any exceptions from idToHex or the network call
are caught and handled consistently with other network helpers like
Spicetify.CosmosAsync.get().
- Around line 465-472: The network request using
Spicetify.Platform.RequestBuilder.build().withHost(...).withPath(...).withEndpointIdentifier(...).send()
can throw; wrap that call (and the subsequent await of response/albumInfo) in a
try-catch inside the fetchInfo function, catch any errors, log them via the
existing logging mechanism (or Spicetify.showNotification/processLogger) and
return null on failure so the UI update flow doesn't break; specifically modify
the block that defines response/albumInfo to perform the send() and await inside
try { ... } and move the existing if (!albumInfo?.ok) return null into that try,
with the catch handling errors and returning null.
---
Nitpick comments:
In `@Extensions/fullAppDisplay.js`:
- Line 471: Remove the redundant await when assigning albumInfo: the variable
response is already resolved (from the prior await ...send()), so change the
assignment in Extensions/fullAppDisplay.js that uses albumInfo and response to a
direct assignment (albumInfo = response) rather than awaiting response; update
references to the variable if needed and ensure no other code depends on
response being a Promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d90974f-9572-450d-87a9-507c703972cd
📒 Files selected for processing (1)
Extensions/fullAppDisplay.js
Extensions/fullAppDisplay.js
Outdated
Comment on lines
+462
to
+469
| const id = uri.split(":")[2]; | ||
| const path = `/album/${Spicetify.URI.idToHex(id)}`; | ||
|
|
||
| const albumInfo = await Spicetify.CosmosAsync.get(`https://api.spotify.com/v1/albums/${id}`); | ||
| const response = await Spicetify.Platform.RequestBuilder.build() | ||
| .withHost("https://spclient.wg.spotify.com/metadata/4") | ||
| .withPath(path) | ||
| .withEndpointIdentifier(path) | ||
| .send(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing usage of these Spicetify APIs in the codebase
echo "=== Searching for RequestBuilder usage ==="
rg -n "RequestBuilder" --type js -C3
echo ""
echo "=== Searching for idToHex usage ==="
rg -n "idToHex" --type js -C3
echo ""
echo "=== Searching for similar metadata endpoint patterns ==="
rg -n "spclient.wg.spotify.com" --type js -C2Repository: spicetify/cli
Length of output: 4137
🏁 Script executed:
#!/bin/bash
# Search for response handling patterns to verify response structure
echo "=== Searching for response.ok usage ==="
rg -n "\.ok" --type js -C2 | head -40
echo ""
echo "=== Searching for response.body usage ==="
rg -n "response\.body" --type js -C2 | head -40
echo ""
echo "=== Checking the context around the getAlbumDate function ==="
sed -n '460,475p' Extensions/fullAppDisplay.js
echo ""
echo "=== Searching for any Platform.RequestBuilder documentation or comments ==="
rg -n "Platform" --type js -C1 | grep -i "builder\|request" -B2 -A2Repository: spicetify/cli
Length of output: 2968
🏁 Script executed:
#!/bin/bash
# Search for context and error handling around getAlbumDate and RequestBuilder
echo "=== Full context of getAlbumDate function ==="
sed -n '455,490p' Extensions/fullAppDisplay.js
echo ""
echo "=== Search for any error handling or try-catch around this function ==="
rg -n "getAlbumDate" --type js -B5 -A15
echo ""
echo "=== Search for any Spicetify.Platform references with more context ==="
rg -n "Spicetify\.Platform" --type js -C3
echo ""
echo "=== Check if there are any imports or initialization of Platform.RequestBuilder ==="
rg -n "Platform\." --type js -C2 | head -60Repository: spicetify/cli
Length of output: 46929
Add error handling for RequestBuilder call.
Spicetify.URI.idToHex exists and is properly defined, but Spicetify.Platform.RequestBuilder is an undocumented internal API with no other usages in the codebase. Wrap the RequestBuilder.build().send() call in a try-catch block to handle potential failures. Additionally, verify this API is available and stable across target Spotify versions, as it deviates from the standard patterns used elsewhere (e.g., Spicetify.CosmosAsync.get() in jsHelper/spicetifyWrapper.js:1534-1539).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Extensions/fullAppDisplay.js` around lines 462 - 469, Wrap the
Spicetify.Platform.RequestBuilder usage in a try-catch and guard against the
RequestBuilder being undefined: verify Spicetify.Platform &&
Spicetify.Platform.RequestBuilder exist before calling RequestBuilder.build(),
then call
build().withHost(...).withPath(path).withEndpointIdentifier(path).send() inside
a try block and handle failures in catch by logging the error (or returning a
safe fallback) so the caller won’t crash; keep using Spicetify.URI.idToHex(id)
for path construction but ensure any exceptions from idToHex or the network call
are caught and handled consistently with other network helpers like
Spicetify.CosmosAsync.get().
Extensions/fullAppDisplay.js
Outdated
Comment on lines
+465
to
+472
| const response = await Spicetify.Platform.RequestBuilder.build() | ||
| .withHost("https://spclient.wg.spotify.com/metadata/4") | ||
| .withPath(path) | ||
| .withEndpointIdentifier(path) | ||
| .send(); | ||
|
|
||
| const albumDate = new Date(albumInfo.release_date); | ||
| return albumDate.toLocaleString("default", { | ||
| const albumInfo = await response; | ||
| if (!albumInfo?.ok) return null; |
There was a problem hiding this comment.
Missing error handling for network request.
The RequestBuilder.send() call can throw on network failures, timeouts, or other request errors. Without a try-catch, this will propagate as an uncaught exception and could break the UI update flow in fetchInfo().
🛡️ Suggested fix to add error handling
async getAlbumDate(uri) {
const id = uri.split(":")[2];
const path = `/album/${Spicetify.URI.idToHex(id)}`;
+ try {
const response = await Spicetify.Platform.RequestBuilder.build()
.withHost("https://spclient.wg.spotify.com/metadata/4")
.withPath(path)
.withEndpointIdentifier(path)
.send();
- const albumInfo = await response;
- if (!albumInfo?.ok) return null;
+ if (!response?.ok) return null;
- const albumDate = albumInfo.body?.date;
+ const albumDate = response.body?.date;
if (!albumDate?.year) return null;
if (!albumDate.month || !albumDate.day) {
return albumDate.year.toString();
}
const date = new Date(
albumDate.year,
albumDate.month - 1,
albumDate.day
);
return date.toLocaleString("default", {
year: "numeric",
month: "short",
day: "numeric",
});
+ } catch {
+ return null;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Extensions/fullAppDisplay.js` around lines 465 - 472, The network request
using
Spicetify.Platform.RequestBuilder.build().withHost(...).withPath(...).withEndpointIdentifier(...).send()
can throw; wrap that call (and the subsequent await of response/albumInfo) in a
try-catch inside the fetchInfo function, catch any errors, log them via the
existing logging mechanism (or Spicetify.showNotification/processLogger) and
return null on failure so the UI update flow doesn't break; specifically modify
the block that defines response/albumInfo to perform the send() and await inside
try { ... } and move the existing if (!albumInfo?.ok) return null into that try,
with the catch handling errors and returning null.
Member
|
Why not GraphQL endpoint? |
cvius
changed the title
Author
done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit