feat(api-server): added elapsedMilliseconds value for precision#4591
feat(api-server): added elapsedMilliseconds value for precision#4591AARP41298 wants to merge 1 commit into
Conversation
…ternal synced lyrics
📝 WalkthroughWalkthroughThe song info model and API schema now support optional millisecond precision. Song info routes read the renderer’s live video position, populate elapsed timing fields, and fall back to existing elapsed seconds when necessary. ChangesSong info playback timing
Estimated code review effort: 3 (Moderate) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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/plugins/api-server/backend/routes/control.ts`:
- Around line 746-748: Handle rejections from the executeJavaScript call used to
assign currentTime in the endpoint by adding a catch that returns null, allowing
the existing elapsedSeconds fallback logic to run without propagating an API
error.
🪄 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: 6dacf97b-260b-4119-9a46-f9dbedb3e300
📒 Files selected for processing (3)
src/plugins/api-server/backend/routes/control.tssrc/plugins/api-server/backend/scheme/song-info.tssrc/providers/song-info.ts
| const currentTime = (await window.webContents.executeJavaScript( | ||
| 'document.querySelector("video")?.currentTime ?? null', | ||
| )) as number | null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle executeJavaScript rejections to prevent API 500 errors.
If the renderer process is navigating, reloading, or otherwise unavailable when this endpoint is polled, executeJavaScript will reject. This causes an unhandled promise rejection that crashes the request handler instead of safely executing the elapsedSeconds fallback logic.
Add a .catch() block to gracefully handle these rejections.
🛡️ Proposed fix
// Prefer live video.currentTime for ms precision
const currentTime = (await window.webContents.executeJavaScript(
'document.querySelector("video")?.currentTime ?? null',
- )) as number | null;
+ ).catch(() => null)) as number | null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currentTime = (await window.webContents.executeJavaScript( | |
| 'document.querySelector("video")?.currentTime ?? null', | |
| )) as number | null; | |
| const currentTime = (await window.webContents.executeJavaScript( | |
| 'document.querySelector("video")?.currentTime ?? null', | |
| ).catch(() => null)) as number | null; |
🤖 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/plugins/api-server/backend/routes/control.ts` around lines 746 - 748,
Handle rejections from the executeJavaScript call used to assign currentTime in
the endpoint by adding a catch that returns null, allowing the existing
elapsedSeconds fallback logic to run without propagating an API error.
Fix #4320
Summary by CodeRabbit