feat: add metadata auto syncing for musics in library#140
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a background audio metadata synchronization feature, including a rate limiter (rateLimiter.ts) for Microsoft Graph API requests, a background sync hook (useMetadataSync.ts), and range-based metadata parsing (rangeMetadata.ts, rangeReader.ts) to extract ID3, FLAC, MP4, WAV, and Ogg tags directly from files. Feedback highlights potential race conditions and early termination issues in the background sync hook, a missing database index for metadataState in Dexie, performance overhead from dynamic regular expression creation and inefficient Uint8Array concatenation, and a lack of boundary checks in WAV parsing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| useEffect(() => { | ||
| if (!db || !account || fileSyncStatus !== 'success' || running.current) return | ||
|
|
||
| const fetchWithRetry = async (node: FileNode) => { | ||
| let lastError: unknown | ||
|
|
||
| for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt += 1) { | ||
| try { | ||
| return await getFileData(node.id, undefined, undefined, 'low', false) | ||
| } catch (error) { | ||
| lastError = error | ||
| if (attempt < MAX_ATTEMPTS - 1) { | ||
| await wait(RETRY_BASE_DELAY_MS * (2 ** attempt)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| throw lastError | ||
| } | ||
|
|
||
| const run = async () => { | ||
| running.current = true | ||
|
|
||
| try { | ||
| const nodes = await db.nodes | ||
| .where('metadataState') | ||
| .anyOf(['pending', 'failed']) | ||
| .and(node => node.type === 'audio') | ||
| .toArray() | ||
|
|
||
| if (nodes.length === 0) return | ||
|
|
||
| start() | ||
|
|
||
| await Promise.all(nodes.map(node => metadataTaskLimit(async () => { | ||
| try { | ||
| const remoteItem = await fetchWithRetry(node) | ||
| const graphTitle = typeof remoteItem.audio?.title === 'string' && remoteItem.audio.title.trim() | ||
| let metadata | ||
|
|
||
| if (graphTitle) { | ||
| metadata = graphAudioToMetadata(node, remoteItem.audio) | ||
| } else if (remoteItem['@microsoft.graph.downloadUrl']) { | ||
| try { | ||
| // Range requests must target the short-lived download URL. | ||
| // https://learn.microsoft.com/en-us/graph/api/driveitem-get-content?view=graph-rest-1.0#partial-range-downloads | ||
| metadata = await getRangeMetadata(node, remoteItem['@microsoft.graph.downloadUrl']) | ||
| } catch (error) { | ||
| if (error instanceof RangeRequestError) throw error | ||
| console.warn(`Falling back to filename metadata for ${node.name}:`, error) | ||
| metadata = graphAudioToMetadata(node) | ||
| } | ||
| } else { | ||
| throw new Error('No download URL returned for metadata extraction.') | ||
| } | ||
|
|
||
| await db.transaction('rw', db.metadata, db.nodes, async () => { | ||
| await db.metadata.put(metadata) | ||
| await db.nodes.update(node.id, { metadataState: 'completed' }) | ||
| }) | ||
| } catch (error) { | ||
| console.error(`Failed to get metadata for ${node.name}:`, error) | ||
| await db.nodes.update(node.id, { metadataState: 'failed' }) | ||
| } | ||
| }))) | ||
| } catch (error) { | ||
| console.error('Failed to start metadata sync:', error) | ||
| } finally { | ||
| running.current = false | ||
| finish() | ||
| } | ||
| } | ||
|
|
||
| void run() | ||
| // Graph helpers close over the current account and are recreated on render. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [account, db, fileSyncStatus, finish, start]) |
There was a problem hiding this comment.
在当前的 useEffect 实现中,存在以下几个潜在的并发与正确性问题:
- 竞态条件与资源浪费:当
account或db发生变化时,旧的同步任务(run)并不会被取消,它会继续在后台运行并尝试请求/写入数据库,这可能导致数据混乱或不必要的网络请求。 Promise.all的过早终止问题:Promise.all会在任意一个 Promise 拒绝(reject)时立即拒绝。如果在catch块中的db.nodes.update抛出异常,Promise.all会立即失败并执行finally块(将running.current设为false并调用finish()),但此时后台其实还有其他并发任务在继续运行。这会导致 UI 状态不准确,且允许用户触发重复的同步任务。
建议:引入 AbortController 来支持任务取消,并使用 Promise.allSettled 代替 Promise.all,确保所有并发任务都执行完毕后再重置状态。同时,对 catch 块中的数据库操作进行二次捕获,防止未捕获的异常导致整个链路崩溃。
useEffect(() => {
if (!db || !account || fileSyncStatus !== 'success') return
const controller = new AbortController()
const { signal } = controller
let active = true
const fetchWithRetry = async (node: FileNode) => {
let lastError: unknown
for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt += 1) {
if (signal.aborted) throw new DOMException('Aborted', 'AbortError')
try {
return await getFileData(node.id, undefined, signal, 'low', false)
} catch (error) {
lastError = error
if (attempt < MAX_ATTEMPTS - 1) {
await wait(RETRY_BASE_DELAY_MS * (2 ** attempt))
}
}
}
throw lastError
}
const run = async () => {
if (running.current) return
running.current = true
try {
const nodes = await db.nodes
.where('metadataState')
.anyOf(['pending', 'failed'])
.and(node => node.type === 'audio')
.toArray()
if (nodes.length === 0 || !active) return
start()
await Promise.allSettled(nodes.map(node => metadataTaskLimit(async () => {
if (signal.aborted || !active) return
try {
const remoteItem = await fetchWithRetry(node)
if (signal.aborted || !active) return
const graphTitle = typeof remoteItem.audio?.title === 'string' && remoteItem.audio.title.trim()
let metadata
if (graphTitle) {
metadata = graphAudioToMetadata(node, remoteItem.audio)
} else if (remoteItem['@microsoft.graph.downloadUrl']) {
try {
metadata = await getRangeMetadata(node, remoteItem['@microsoft.graph.downloadUrl'])
} catch (error) {
if (error instanceof RangeRequestError) throw error
console.warn(`Falling back to filename metadata for ${node.name}:`, error)
metadata = graphAudioToMetadata(node)
}
} else {
throw new Error('No download URL returned for metadata extraction.')
}
if (signal.aborted || !active) return
await db.transaction('rw', db.metadata, db.nodes, async () => {
await db.metadata.put(metadata)
await db.nodes.update(node.id, { metadataState: 'completed' })
})
} catch (error) {
if (signal.aborted || !active) return
console.error(`Failed to get metadata for ${node.name}:`, error)
try {
await db.nodes.update(node.id, { metadataState: 'failed' })
} catch (dbError) {
console.error(`Failed to update metadataState to failed for ${node.name}:`, dbError)
}
}
})))
} catch (error) {
if (active) {
console.error('Failed to start metadata sync:', error)
}
} finally {
if (active) {
running.current = false
finish()
}
}
}
void run()
return () => {
active = false
controller.abort()
running.current = false
}
// Graph helpers close over the current account and are recreated on render.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [account, db, fileSyncStatus, finish, start])There was a problem hiding this comment.
理论上应该被 2a240fb 修复,但并发问题看得我头晕,我只能说看上去至少没有引入额外危害,至于修没修彻底看不出来。
|
找了5首不同格式的音乐(mp3, ogg, flac, wav, m4a)弄了个小文件夹测试了一下,能够正常扫描全部元数据。happy path应该是没有问题的。 |
|
感谢你的贡献,这个pr我得评估一下风险,我OneDrive内目前有一万五千多首歌曲,如果全部扫描一遍,触发速率限制的风险非常大,正常播放功能也可能会受到影响。 |
为音乐库添加元数据自动获取功能。
目前并发2下元数据获取还是很慢,但比起一个个播放还是好得多,而且使用http range选择性下载文字元数据,流量消耗也可以接受。
Edit:
使用GPT 5.6sol辅助编写。简单review过一遍。尝试跑了一晚上,虽然元数据没获取完,但没遇到报错。