feat(discord): display artist name and avatar in rich presence with caching and optimization#4491
feat(discord): display artist name and avatar in rich presence with caching and optimization#4491Nynele wants to merge 2 commits into
Conversation
…aching and optimization
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an ChangesDiscord artist image fetch and rich presence toggle
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (peard:video-src-changed)
participant SongInfoProvider as song-info provider
participant fetchArtistImage
participant DiscordService
Renderer->>SongInfoProvider: video src changed event
SongInfoProvider->>SongInfoProvider: handleData → songInfo with empty artistImageSrc
SongInfoProvider->>SongInfoProvider: emit VideoSrcChanged
SongInfoProvider->>fetchArtistImage: fetch og:image for artistUrl
fetchArtistImage-->>SongInfoProvider: image URL or null
SongInfoProvider->>SongInfoProvider: update songInfo.artistImageSrc under mutex
SongInfoProvider->>Renderer: peard:update-song-info (updated songInfo)
SongInfoProvider->>DiscordService: ArtistImageLoaded callback
DiscordService->>DiscordService: detect artistImageChanged → setActivity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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 `@src/providers/song-info.ts`:
- Around line 188-216: The response body stream is being consumed twice, which
violates the Fetch API specification. The getReader() call locks the response
body stream, making subsequent calls to response.text() invalid. After the while
loop in the streaming section, instead of calling response.text() on line 215,
use the already-accumulated htmlBuffer variable to perform the final regex match
for the og:image meta tag. This ensures the response body is only consumed once
through the stream reading process.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7e4a0e2-38b6-425c-9356-153738f2b0ff
📒 Files selected for processing (6)
src/i18n/resources/en.jsonsrc/i18n/resources/es.jsonsrc/plugins/discord/discord-service.tssrc/plugins/discord/index.tssrc/plugins/discord/menu.tssrc/providers/song-info.ts
| if (response.body && typeof response.body.getReader === 'function') { | ||
| const reader = response.body.getReader(); | ||
| const decoder = new TextDecoder('utf-8'); | ||
| let htmlBuffer = ''; | ||
|
|
||
| try { | ||
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
| htmlBuffer += decoder.decode(value, { stream: true }); | ||
| const match = htmlBuffer.match(/<meta property="og:image" content="([^"]+)"/); | ||
| if (match && match[1]) { | ||
| const imgSrc = match[1]; | ||
| artistImageCache.set(artistUrl, imgSrc); | ||
| reader.cancel(); | ||
| return imgSrc; | ||
| } | ||
| if (htmlBuffer.includes('</head>')) { | ||
| reader.cancel(); | ||
| break; | ||
| } | ||
| } | ||
| } catch (err) { | ||
| // stream error, fallback to full page load | ||
| } | ||
| } | ||
|
|
||
| const html = await response.text(); | ||
| const match = html.match(/<meta property="og:image" content="([^"]+)"/); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, find and inspect the file structure
fd -type f -name "song-info.ts"Repository: pear-devs/pear-desktop
Length of output: 331
🏁 Script executed:
# Examine the specific function in question
sed -n '180,225p' src/providers/song-info.tsRepository: pear-devs/pear-desktop
Length of output: 1568
🌐 Web query:
Fetch API Response body consumption getReader() and text() same response disturbed
💡 Result:
The Fetch API's Response body is implemented as a ReadableStream [1][2], which can only be consumed once [1][3][4]. When you call a consumption method—such as.text,.json,.blob, or.getReader—the stream is marked as "read" or "locked" [5][2][4]. Any subsequent attempt to consume the body using these methods on the same Response object will result in an error, typically "body stream already read" or "body stream is locked" [1][3][6]. This design choice allows the browser to stream data directly from the network to the consumer (e.g., the JSON parser or an ArrayBuffer) without needing to buffer the entire response in memory [1]. To work around this limitation if you need to use the body data multiple times, use one of the following approaches: 1. Clone the Response: Use the response.clone method before consuming the body [5][3]. This creates a new Response object that is identical to the original and has its own independent, unconsumed stream [3]. const response = await fetch(url); const responseClone = response.clone; const text = await response.text; // First consumption const json = await responseClone.json; // Second consumption (via clone) 2. Buffer the Data: If you intend to use the data in multiple places within your application, consume the stream once (e.g., via.json or.text) and store the resulting data in a variable [7][3]. You can then reference that variable as many times as needed [7]. 3. Use Teeing (for streams): If you are working directly with the ReadableStream via getReader, you can use the stream.tee method to split the stream into two independent branches, allowing you to consume both separately [7].
Citations:
- 1: https://stackoverflow.com/questions/78660320/why-can-i-not-read-a-body-stream-multiple-times-in-javascript
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_readable_streams
- 3: https://stackoverflow.com/questions/53511974/javascript-fetch-failed-to-execute-json-on-response-body-stream-is-locked
- 4: https://javascript.info/fetch-progress
- 5: https://stackoverflow.com/questions/40497859/reread-a-response-body-from-javascripts-fetch
- 6: https://stackoverflow.com/questions/74346746/try-to-fetch-and-read-the-response-body-as-json-or-fallback-to-plain-text
- 7: Question — Why can I not read the stream again ? whatwg/fetch#196
response.text() fallback is invalid after stream consumption.
Line 215 calls response.text() after lines 188-209 already consume the same body via getReader(). Per the Fetch API specification, once a Response body stream is locked (by calling getReader() or any other consumption method), subsequent attempts to consume it will throw an error like "body stream already read" or "body stream is locked".
🐛 Proposed fix
export const fetchArtistImage = async (artistUrl: string): Promise<string | null> => {
if (artistImageCache.has(artistUrl)) {
return artistImageCache.get(artistUrl) ?? null;
}
try {
const response = await net.fetch(artistUrl);
+ const fallbackResponse = response.clone();
if (!response.ok) return null;
if (response.body && typeof response.body.getReader === 'function') {
const reader = response.body.getReader();
const decoder = new TextDecoder('utf-8');
@@
}
- const html = await response.text();
+ const html = await fallbackResponse.text();
const match = html.match(/<meta property="og:image" content="([^"]+)"/);
if (match && match[1]) {
const imgSrc = match[1];
artistImageCache.set(artistUrl, imgSrc);
return imgSrc;
}🤖 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 `@src/providers/song-info.ts` around lines 188 - 216, The response body stream
is being consumed twice, which violates the Fetch API specification. The
getReader() call locks the response body stream, making subsequent calls to
response.text() invalid. After the while loop in the streaming section, instead
of calling response.text() on line 215, use the already-accumulated htmlBuffer
variable to perform the final regex match for the og:image meta tag. This
ensures the response body is only consumed once through the stream reading
process.
… fetchArtistImage
1 participant
Description of Changes
This Pull Request improves the Discord Rich Presence plugin to display the artist name directly and show the artist's profile picture as a small circular avatar on the cover art.
Key improvements:
songInfo.artist) on the second line instead of falling back on song tags.smallImageKey/smallImageText) displaying the artist's profile photo.song-info.tsthat aborts the HTTP request as soon as it matches theog:imagemeta tag in the<head>of the artist's channel page. This avoids downloading the rest of the 500KB+ page, resolving the image in ~50-100ms.artistImageCache) in the backend to store resolved artist images and prevent duplicate network requests when switching between cached tracks.artistImageChangedcheck indiscord-service.tsto immediately push the RPC update when the avatar resolves, bypassing the standard 15-second update throttle.hideArtistImage) in the plugin menu to allow users to disable the artist's profile picture if desired, complete with i18n support in English and Spanish.Verification Results
Summary by CodeRabbit