Fix/mpris reporting#4438
Conversation
📝 WalkthroughWalkthroughTwo files updated: the MPRIS shortcuts plugin replaces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/shortcuts/mpris.ts`:
- Around line 11-19: The current type assertion on MprisPlayer creates no
runtime properties and causes Mpris.PLAYBACK_STATUS_* and Mpris.LOOP_STATUS_* to
be undefined; replace the cast by directly importing MprisPlayer plus the named
constants PLAYBACK_STATUS_PLAYING, PLAYBACK_STATUS_PAUSED,
PLAYBACK_STATUS_STOPPED, LOOP_STATUS_NONE, LOOP_STATUS_TRACK, and
LOOP_STATUS_PLAYLIST from '@jellybrick/mpris-service', remove the type assertion
block that defines Mpris as typeof MprisPlayer & {...}, and update all uses of
Mpris.PLAYBACK_STATUS_* and Mpris.LOOP_STATUS_* to reference the imported
constants directly (keeping MprisPlayer usage for the class where needed).
In `@src/providers/song-info-front.ts`:
- Around line 30-35: The seeked/play handlers use getTime() which reads the
stale songInfo.elapsedSeconds cache; fix by having setupSeekedListener read the
actual media element time (e.g. document.querySelector('video')?.currentTime ??
0) instead of songInfo, and additionally clear or reset songInfo.elapsedSeconds
when a new track is loaded (handle the dataloaded or 'peard:update-song-info'
point to set songInfo.elapsedSeconds = undefined/0) so the cached value cannot
leak into the first seeked/play-or-paused event; update getTime() or inline the
media-read in setupSeekedListener and apply the same change at the other
occurrence referenced (lines ~229-234).
🪄 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: 8eb066a7-08f4-44bd-af9f-62045aefc538
📒 Files selected for processing (2)
src/plugins/shortcuts/mpris.tssrc/providers/song-info-front.ts
| const Mpris = MprisPlayer as typeof MprisPlayer & { | ||
| PLAYBACK_STATUS_PLAYING: PlayBackStatus; | ||
| PLAYBACK_STATUS_PAUSED: PlayBackStatus; | ||
| PLAYBACK_STATUS_STOPPED: PlayBackStatus; | ||
|
|
||
| LOOP_STATUS_NONE: LoopStatus; | ||
| LOOP_STATUS_TRACK: LoopStatus; | ||
| LOOP_STATUS_PLAYLIST: LoopStatus; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the exact version of @jellybrick/mpris-service used in this repository, are PLAYBACK_STATUS_PLAYING / PLAYBACK_STATUS_PAUSED / PLAYBACK_STATUS_STOPPED and LOOP_STATUS_NONE / LOOP_STATUS_TRACK / LOOP_STATUS_PLAYLIST exposed as static properties on the default export, or only as named module exports? Please check the package source or official docs for that version.
💡 Result:
The constants PLAYBACK_STATUS_PLAYING, PLAYBACK_STATUS_PAUSED, PLAYBACK_STATUS_STOPPED, LOOP_STATUS_NONE, LOOP_STATUS_TRACK, and LOOP_STATUS_PLAYLIST are exposed only as named module exports, not as static properties on the default export (Player class). Pear-desktop, a likely candidate repository using this package (given the matching scoped name and JellyBrick's contributions), defines these exact constants in its TypeScript declaration file for @jellybrick/mpris-service (mpris-service.d.ts). They are declared as top-level named exports: export const PLAYBACK_STATUS_PLAYING: 'Playing'; export const PLAYBACK_STATUS_PAUSED: 'Paused'; export const PLAYBACK_STATUS_STOPPED: 'Stopped'; export const LOOP_STATUS_NONE: 'None'; export const LOOP_STATUS_TRACK: 'Track'; export const LOOP_STATUS_PLAYLIST: 'Playlist'; Usage in pear-desktop's mpris.ts confirms named imports: import MprisPlayer, { LOOP_STATUS_NONE, LOOP_STATUS_PLAYLIST, LOOP_STATUS_TRACK, PLAYBACK_STATUS_PAUSED, PLAYBACK_STATUS_PLAYING, PLAYBACK_STATUS_STOPPED, ... } from '@jellybrick/mpris-service'; The package structure includes dist/constants.js (742B), indicating constants are in a separate module file, typical for named exports. The default export is the Player class (new Player({...})), with no evidence of static properties for these values on Player. Properties like player.playbackStatus use the string literals directly (e.g., 'Playing'), but constants are provided as named exports for convenience and type safety.
Citations:
- 1: https://registry.npmjs.org/@jellybrick/mpris-service
- 2: https://github.com/pear-devs/pear-desktop/blob/4f31d470/src/plugins/shortcuts/mpris-service.d.ts
- 3: https://github.com/pear-devs/pear-desktop/blob/4f31d470/src/plugins/shortcuts/mpris.ts
🏁 Script executed:
# First, check the package.json for `@jellybrick/mpris-service` version
cat package.json | grep -A2 -B2 "@jellybrick/mpris-service" || echo "Not found in root"
# Also check for yarn.lock or package-lock.json
if [ -f yarn.lock ]; then
echo "=== From yarn.lock ==="
grep -A5 "@jellybrick/mpris-service" yarn.lock | head -20
elif [ -f package-lock.json ]; then
echo "=== From package-lock.json ==="
grep -A5 "@jellybrick/mpris-service" package-lock.json | head -20
fiRepository: pear-devs/pear-desktop
Length of output: 267
🏁 Script executed:
# Now examine the actual usage in mpris.ts
echo "=== File: src/plugins/shortcuts/mpris.ts (lines 11-50) ==="
sed -n '11,50p' src/plugins/shortcuts/mpris.ts
echo ""
echo "=== Checking all usage of 'Mpris.' pattern ==="
grep -n "Mpris\." src/plugins/shortcuts/mpris.ts | head -30Repository: pear-devs/pear-desktop
Length of output: 2097
🏁 Script executed:
# Check the declaration file
echo "=== File: src/plugins/shortcuts/mpris-service.d.ts ==="
cat src/plugins/shortcuts/mpris-service.d.tsRepository: pear-devs/pear-desktop
Length of output: 5777
Replace the type assertion with direct named imports of constants.
The cast at lines 11–19 only satisfies TypeScript; it does not create runtime properties. Since PLAYBACK_STATUS_PLAYING, PLAYBACK_STATUS_PAUSED, PLAYBACK_STATUS_STOPPED, LOOP_STATUS_NONE, LOOP_STATUS_TRACK, and LOOP_STATUS_PLAYLIST are exported only as named constants (not static properties on the Player class) in @jellybrick/mpris-service@2.1.5, every usage of Mpris.PLAYBACK_STATUS_* and Mpris.LOOP_STATUS_* throughout this file will be undefined at runtime, breaking all status comparisons and assignments.
Import the constants directly:
import MprisPlayer, {
PLAYBACK_STATUS_PLAYING,
PLAYBACK_STATUS_PAUSED,
PLAYBACK_STATUS_STOPPED,
LOOP_STATUS_NONE,
LOOP_STATUS_TRACK,
LOOP_STATUS_PLAYLIST,
} from '@jellybrick/mpris-service';Then remove the type assertion and use the imported constants directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/shortcuts/mpris.ts` around lines 11 - 19, The current type
assertion on MprisPlayer creates no runtime properties and causes
Mpris.PLAYBACK_STATUS_* and Mpris.LOOP_STATUS_* to be undefined; replace the
cast by directly importing MprisPlayer plus the named constants
PLAYBACK_STATUS_PLAYING, PLAYBACK_STATUS_PAUSED, PLAYBACK_STATUS_STOPPED,
LOOP_STATUS_NONE, LOOP_STATUS_TRACK, and LOOP_STATUS_PLAYLIST from
'@jellybrick/mpris-service', remove the type assertion block that defines Mpris
as typeof MprisPlayer & {...}, and update all uses of Mpris.PLAYBACK_STATUS_*
and Mpris.LOOP_STATUS_* to reference the imported constants directly (keeping
MprisPlayer usage for the class where needed).
| const getTime = () => songInfo.elapsedSeconds ?? 0; | ||
|
|
||
| export const setupSeekedListener = singleton(() => { | ||
| document.querySelector('video')?.addEventListener('seeked', (v) => { | ||
| if (v.target instanceof HTMLVideoElement) { | ||
| window.ipcRenderer.send('peard:seeked', v.target.currentTime); | ||
| } | ||
| document.querySelector('video')?.addEventListener('seeked', () => { | ||
| window.ipcRenderer.send('peard:seeked', getTime()); | ||
| }); |
There was a problem hiding this comment.
Reset the front-end elapsed-time cache when a new track starts.
getTime() now reads songInfo.elapsedSeconds, but that cache is only refreshed by peard:update-song-info and the progress-bar observer. During dataloaded, it still holds the previous track's position, so the first seeked / play-or-paused event for a newly chained track can report the old elapsed time to MPRIS. Reset the local cache on track change, or read the progress bar's current value directly here instead of the cached field.
Also applies to: 229-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/providers/song-info-front.ts` around lines 30 - 35, The seeked/play
handlers use getTime() which reads the stale songInfo.elapsedSeconds cache; fix
by having setupSeekedListener read the actual media element time (e.g.
document.querySelector('video')?.currentTime ?? 0) instead of songInfo, and
additionally clear or reset songInfo.elapsedSeconds when a new track is loaded
(handle the dataloaded or 'peard:update-song-info' point to set
songInfo.elapsedSeconds = undefined/0) so the cached value cannot leak into the
first seeked/play-or-paused event; update getTime() or inline the media-read in
setupSeekedListener and apply the same change at the other occurrence referenced
(lines ~229-234).
1 participant
Static constants
After the migration to Vite v8, some of the static constants (loop and playback consts) were just undefined references.
Position Reporting
The <video> element that was being used as a reference to report position to MPRIS behaves in a way that does not coordinate well with the current system.
Sometimes, when chaining tracks (that is, playing one after the other), the <video> element's currentTime and duration will not reset to 0, but rather accumulate on top of the previous track's values. This caused Pear to report a wrong position value to MPRIS (usually larger than the song's length).
Computing the elapsedSeconds from these values would still be feasible if the duration attribute immediately increased by the new track's length on track change, but this is not the case. Instead, duration gradually increases for as long as the track is playing.
As an alternative, I propose relying on the already implemented progress bar observer, which reports a more discrete but correct value.
Summary by CodeRabbit