feat: desktop lyrcis#122
Conversation
SafeDep Report Summary
Package Details
This report is generated by SafeDep Github App |
Walkthrough播放开始时通过 headless 任务触发并导出 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Headless as Headless 任务 (index.js)
participant Player as player 工具 (setDesktopLyrics)
participant Track as trackService
participant Lyric as lyricService
participant Orpheus as 原生 Orpheus
participant UI as 设置面板
Headless->>Player: onTrackStarted(trackId, reason)
Note over Player: 记录时间戳并防抖 1s；显示占位歌词
Player->>Track: fetch track by id
Track-->>Player: track data
Player->>Lyric: smartFetchLyrics(track)
Lyric-->>Player: lyrics
Player->>Orpheus: setDesktopLyrics(lyrics)
Orpheus-->>Player: success / error
alt 错误
Player->>UI: toastAndLogError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 分钟 Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
src/app/(tabs)/settings/playback.tsx (1)
90-123: 桌面歌词设置 UI 实现良好。异步操作先执行后更新状态的模式正确，错误处理与现有代码风格一致。
一个小建议：可以考虑在切换操作进行时禁用 Switch 以防止重复点击，但这不是必须的。
src/utils/player.ts (1)
248-249: 考虑使用ReturnType<typeof setTimeout>类型以提高兼容性。在不同 TypeScript 配置下，
setTimeout的返回类型可能是NodeJS.Timeout而非number。🔎 建议的类型改进
-let debouncedSetDesktopLyrics: number | null = null +let debouncedSetDesktopLyrics: ReturnType<typeof setTimeout> | null = null
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
index.jspackage.jsonsrc/app/(tabs)/settings/playback.tsxsrc/utils/player.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/player.ts (2)
src/lib/services/trackService.ts (1)
trackService(819-819)src/utils/error-handling.ts (1)
toastAndLogError(11-41)
🔇 Additional comments (5)
index.js (1)
3-6: 集成实现正确。在
onTrackStarted事件中调用setDesktopLyrics，符合 issue #116 中提到的"每首曲目开始时 JS 层发送歌词到 native"的设计方案。setDesktopLyrics内部已包含防抖和错误处理。Also applies to: 27-29
src/app/(tabs)/settings/playback.tsx (1)
21-26: 状态初始化实现正确。从
Orpheus.isDesktopLyricsShown和Orpheus.isDesktopLyricsLocked初始化状态，与现有的Orpheus.restorePlaybackPositionEnabled等初始化模式一致。src/utils/player.ts (2)
248-284: 防抖实现正确，有效避免快速切歌时的重复请求。时间戳检查机制 (lines 263, 270, 276) 正确处理了异步操作中的竞态条件，确保只有最新的调用会完成执行。错误处理覆盖了所有关键路径。
251-254: 未使用的_transitionReason参数。参数
_transitionReason使用下划线前缀表明有意未使用。如果未来计划根据转换原因（如用户主动切歌 vs 自动播放下一首）采取不同逻辑，保留此参数是合理的。否则可以移除。package.json (1)
38-38: 依赖升级符合预期。版本从 ^0.7.2 升级到 ^0.8.0 以支持新的桌面歌词 API。所有必需的 API 都已在代码中正确使用：
showDesktopLyrics、hideDesktopLyrics、setDesktopLyricsLocked、setDesktopLyrics、isDesktopLyricsShown和isDesktopLyricsLocked分别在playback.tsx和player.ts中被调用。
|
坏了，本以为一切顺利，结果遇到了很多问题：
人麻了，觉得这功能做不了了😇 |
|
@CodeRabbit /review |
|
@roitium 我理解您的挫败感。实现桌面歌词功能确实涉及很多复杂的场景,尤其是后台切歌和各种边界情况的处理。 我会立即开始审查这个 PR 的代码。如果您需要的话,我也可以帮助调查您提到的具体问题:
请让我知道是否需要我深入分析这些问题,或者您更倾向于先看看整体的代码审查结果。 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/app/(tabs)/settings/playback.tsx:
- Around line 45-49: Currently the code optimistically calls
setIsDesktopLyricsShown(true) and shows a success toast immediately after
Orpheus.requestOverlayPermission(), but requestOverlayPermission() only opens
system settings and cannot confirm permission; remove the optimistic update and
success toast from the onPress handler and only call
Orpheus.requestOverlayPermission(); rely on the existing AppState listener (the
effect that watches app state on return to foreground) to detect the real
overlay permission state, call setIsDesktopLyricsShown(true) there when
permission is actually granted, and show the success toast from that AppState
handler so UI and messages reflect the true permission state.
- Around line 140-152: The onValueChange handler double-updates state:
enableDesktopLyrics() already calls setIsDesktopLyricsShown(true), but the
handler always calls setIsDesktopLyricsShown(!isDesktopLyricsShown) afterwards,
reverting the change when enabling; fix by only updating local state when the
called function does not already update it — specifically, in onValueChange (the
anonymous async handler) call Orpheus.hideDesktopLyrics() then
setIsDesktopLyricsShown(false) on success, but when calling
enableDesktopLyrics() do NOT call setIsDesktopLyricsShown again (or rely on
enableDesktopLyrics to return success and only set state if it returns false);
keep the try/catch and toastAndLogError as-is and remove the unconditional
setIsDesktopLyricsShown(!isDesktopLyricsShown).
🧹 Nitpick comments (2)
src/utils/player.ts (1)
248-249:setTimeout返回类型在不同环境下不一致。在 React Native 环境中，
setTimeout可能返回NodeJS.Timeout而非number。建议使用ReturnType<typeof setTimeout>来确保类型安全。♻️ 建议修复
-let debouncedSetDesktopLyrics: number | null = null -let lastSetDesktopLyricsTimestamp: number | null = null +let debouncedSetDesktopLyrics: ReturnType<typeof setTimeout> | null = null +let lastSetDesktopLyricsTimestamp: number | null = nullsrc/app/(tabs)/settings/playback.tsx (1)
61-74: 状态同步机制良好，但useFocusEffect缺少依赖处理。AppState 监听器能在应用从后台返回时同步状态。但
useFocusEffect的回调建议包装在useCallback中以避免不必要的重复执行，并确保正确的清理行为。♻️ 建议改进
+import { useCallback } from 'react' -useFocusEffect(() => { - setIsDesktopLyricsShown(Orpheus.isDesktopLyricsShown) - setIsDesktopLyricsLocked(Orpheus.isDesktopLyricsLocked) -}) +useFocusEffect( + useCallback(() => { + setIsDesktopLyricsShown(Orpheus.isDesktopLyricsShown) + setIsDesktopLyricsLocked(Orpheus.isDesktopLyricsLocked) + }, []) +)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
CHANGELOG.mdindex.jspackage.jsonsrc/app/(tabs)/settings/playback.tsxsrc/utils/player.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/(tabs)/settings/playback.tsx (2)
src/components/modals/AlertModal.tsx (1)
alert(72-85)src/types/core/appStore.ts (1)
AppState(29-29)
src/utils/player.ts (2)
src/lib/services/trackService.ts (1)
trackService(819-819)src/utils/error-handling.ts (1)
toastAndLogError(11-41)
🔇 Additional comments (3)
src/utils/player.ts (1)
251-289: 桌面歌词设置逻辑实现良好，使用时间戳进行竞态保护。
- 防抖机制正确地在每次调用时清除旧定时器
- 时间戳检查可有效防止过期请求覆盖新数据
- 错误处理使用
toastAndLogError保持一致性建议：当前设置空歌词（line 265）后获取数据若失败，桌面歌词将保持为空。可考虑在错误场景下显示一条提示信息给用户，或保留之前的歌词。
index.js (1)
27-31: Headless task 实现正确。通过
registerOrpheusHeadlessTask注册后台任务来处理onTrackStarted事件是正确的做法，可以在应用后台运行时设置桌面歌词。src/app/(tabs)/settings/playback.tsx (1)
155-169: 桌面歌词锁定开关实现正确。锁定功能的状态同步逻辑清晰，与 Orpheus 原生层的交互正确。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/(tabs)/settings/playback.tsx (2)
38-52: 权限请求流程可优化用户点击"去设置"后，
requestOverlayPermission()会跳转到系统设置页面，此时无法确定用户是否真的授予了权限。当前在onPress中直接设置setIsDesktopLyricsShown(true)是乐观的假设。虽然
AppState监听器会在用户返回时重新同步状态，但更健壮的做法是在用户返回后再次检查权限并调用showDesktopLyrics()。♻️ 建议的优化
{ text: '去设置', onPress: async () => { await Orpheus.requestOverlayPermission() - setIsDesktopLyricsShown(true) + // 状态将由 AppState listener 在用户返回时同步 }, },
69-72: 建议使用useCallback包裹useFocusEffect回调根据 React Navigation 最佳实践，
useFocusEffect的回调应使用useCallback包裹，避免每次渲染时重复执行。♻️ 建议的修改
+import { useCallback, useEffect, useState } from 'react' -import { useEffect, useState } from 'react' - useFocusEffect(() => { - setIsDesktopLyricsShown(Orpheus.isDesktopLyricsShown) - setIsDesktopLyricsLocked(Orpheus.isDesktopLyricsLocked) - }) + useFocusEffect( + useCallback(() => { + setIsDesktopLyricsShown(Orpheus.isDesktopLyricsShown) + setIsDesktopLyricsLocked(Orpheus.isDesktopLyricsLocked) + }, []), + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(tabs)/settings/playback.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(tabs)/settings/playback.tsx (2)
src/components/modals/AlertModal.tsx (1)
alert(72-85)src/types/core/appStore.ts (1)
AppState(29-29)
🔇 Additional comments (5)
src/app/(tabs)/settings/playback.tsx (5)
1-9: LGTM!导入项与新增的桌面歌词功能一致，无冗余导入。
23-28: LGTM!状态变量初始化方式与其他播放设置保持一致。
59-67: LGTM!
AppState监听器正确同步桌面歌词状态，清理逻辑完整。
93-94: LGTM!属性赋值方式与
@roitium/expo-orpheus0.9.2 的 API 变更一致。
154-168: LGTM!锁定开关的实现与其他设置项保持一致，逻辑正确。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/(tabs)/settings/playback.tsx (3)
30-57: 权限授予后未自动启用桌面歌词用户在系统设置中授予悬浮窗权限后返回应用，桌面歌词不会自动启用，需要再次手动切换开关。结合下方的
AppState监听器，可以在用户返回时检测权限状态并自动启用。♻️ 建议的改进方案
useEffect(() => { - const listener = AppState.addEventListener('change', () => { + const listener = AppState.addEventListener('change', async (nextAppState) => { setIsDesktopLyricsShown(Orpheus.isDesktopLyricsShown) setIsDesktopLyricsLocked(Orpheus.isDesktopLyricsLocked) + // 当应用从后台返回前台时，检查是否已获得权限但歌词未显示 + if (nextAppState === 'active' && !Orpheus.isDesktopLyricsShown) { + const hasPermission = await Orpheus.checkOverlayPermission() + if (hasPermission) { + // 可选：自动启用或提示用户 + } + } }) return () => { listener.remove() } }, [])
69-72:useFocusEffect缺少useCallback包装根据 expo-router 的最佳实践，
useFocusEffect的回调应该用useCallback包装，否则会在每次组件渲染时重新执行。♻️ 建议修改
+import { useCallback, useEffect, useState } from 'react' -import { useEffect, useState } from 'react' - useFocusEffect(() => { - setIsDesktopLyricsShown(Orpheus.isDesktopLyricsShown) - setIsDesktopLyricsLocked(Orpheus.isDesktopLyricsLocked) - }) + useFocusEffect( + useCallback(() => { + setIsDesktopLyricsShown(Orpheus.isDesktopLyricsShown) + setIsDesktopLyricsLocked(Orpheus.isDesktopLyricsLocked) + }, []), + )
153-167: 考虑在桌面歌词未启用时禁用锁定开关当桌面歌词未显示时，锁定开关的操作没有实际意义，建议禁用该开关以提升用户体验。
♻️ 可选改进
<View style={styles.settingRow}> <Text>桌面歌词锁定</Text> <Switch value={isDesktopLyricsLocked} + disabled={!isDesktopLyricsShown} onValueChange={() => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(tabs)/settings/playback.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(tabs)/settings/playback.tsx (2)
src/components/modals/AlertModal.tsx (1)
alert(72-85)src/types/core/appStore.ts (1)
AppState(29-29)
🔇 Additional comments (1)
src/app/(tabs)/settings/playback.tsx (1)
93-94: LGTM!API 从方法调用改为属性赋值的变更一致且正确，错误处理也得到保留。
1 participant
Closes #116
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.