feat: add some sentry spans#83
Conversation
SafeDep Report Summary
Package Details
This report is generated by SafeDep Github App |
WalkthroughAdds Sentry instrumentation across the app: wraps React Lyrics component with Sentry.withProfiler and inlines its fetch/sync logic; and instruments many facade, service, and store async flows with Sentry.startSpan. Also lowers Sentry tracing sample rate and adds a couple of ArtistService methods and a trackService export. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Lyrics Component (React)
participant SentryUI as Sentry.withProfiler
participant Store as Player Store / Services
participant Sentry as Sentry.startSpan
participant DB as DB/IO/HTTP
UI->>SentryUI: render Lyrics (memoized)
SentryUI->>UI: execute component body
UI->>Store: call smartFetchLyrics / LyricSync
Store->>Sentry: startSpan("LyricService.smartFetchLyrics")
Sentry->>DB: read cache / HTTP fetch / parse
DB-->>Sentry: result
Sentry-->>Store: return lyrics
alt has lyrics
UI->>UI: render lyrics list / offset control
else pending/error
UI->>UI: render pending or error view
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (2)
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: 5
🧹 Nitpick comments (1)
src/lib/services/artistService.ts (1)
379-479: Excellent bulk operation implementation!This new method implements the bulk find-or-create pattern correctly using
onConflictDoNothing()followed by a query to retrieve all artists. The logic is sound and the instrumentation provides good visibility into batch sizes and found counts.Consider removing two redundant checks for cleaner code:
Line 410: The check
if (payloads.length > 0)is redundant since you've already returned early at line 395 when the array is empty.Lines 460-464: The check
fullArtists.length !== payloads.lengthwill never be true becausemap()always returns an array of the same length as the input (or throws during iteration, which happens at line 456).Apply this diff to remove the redundancies:
- if (payloads.length > 0) { - await Sentry.startSpan( - { name: 'db:insert:many:artists', op: 'db' }, - () => - this.db - .insert(schema.artists) - .values( - payloads.map( - (p) => - ({ - name: p.name, - source: p.source, - remoteId: p.remoteId, - avatarUrl: p.avatarUrl, - signature: p.signature, - }) satisfies CreateArtistPayload, - ), - ) - .onConflictDoNothing(), - ) - } + await Sentry.startSpan( + { name: 'db:insert:many:artists', op: 'db' }, + () => + this.db + .insert(schema.artists) + .values( + payloads.map( + (p) => + ({ + name: p.name, + source: p.source, + remoteId: p.remoteId, + avatarUrl: p.avatarUrl, + signature: p.signature, + }) satisfies CreateArtistPayload, + ), + ) + .onConflictDoNothing(), + ) const findConditions = payloads.map((p) => and( eq(schema.artists.source, p.source), eq(schema.artists.remoteId, p.remoteId!), ), ) const allArtists = await Sentry.startSpan( { name: 'db:query:many:artists', op: 'db' }, () => this.db.query.artists.findMany({ where: or(...findConditions), }), ) span?.setAttribute('found.count', allArtists.length) const fullArtists = payloads.map((p) => { const existing = allArtists.find( (a) => `${a.source}::${a.remoteId}` === `${p.source}::${p.remoteId}`, ) if (existing) { return existing } throw new DatabaseError( `批量查找或创建 artists 后数据不一致，未找到 artist: ${p.source}::${p.remoteId}`, ) }) - if (fullArtists.length !== payloads.length) { - throw new DatabaseError( - '创建或查找 artists 后数据不一致，部分 artist 未能成功写入或查询。', - ) - } const finalResultMap = new Map( fullArtists.map((artist) => [artist.remoteId!, artist]), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/features/player/components/PlayerLyrics.tsx(2 hunks)src/hooks/stores/usePlayerStore.ts(6 hunks)src/lib/config/sentry.ts(1 hunks)src/lib/facades/bilibili.ts(2 hunks)src/lib/facades/playlist.ts(4 hunks)src/lib/facades/sync.ts(6 hunks)src/lib/services/artistService.ts(9 hunks)src/lib/services/downloadService.ts(4 hunks)src/lib/services/lyricService.ts(3 hunks)src/lib/services/playlistService.ts(18 hunks)src/lib/services/trackService.ts(13 hunks)src/lib/services/updateService.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
src/lib/facades/sync.ts (5)
src/lib/errors/facade.ts (2)
createSyncTaskAlreadyRunningError(22-27)createFacadeError(29-39)src/lib/api/bilibili/utils.ts (2)
bv2av(10-25)av2bv(32-51)src/types/core/media.ts (1)
BilibiliTrack(38-54)src/utils/set.ts (1)
diffSets(7-32)src/lib/services/genKey.ts (1)
generateUniqueTrackKey(10-38)
src/lib/services/trackService.ts (6)
src/lib/errors/service.ts (3)
createValidationError(70-75)createTrackNotFound(27-32)createNotImplementedError(77-79)src/lib/services/genKey.ts (1)
generateUniqueTrackKey(10-38)src/types/services/track.ts (3)
BilibiliMetadataPayload(1-7)CreateTrackPayloadBase(13-18)CreateBilibiliTrackPayload(20-23)src/lib/errors/index.ts (1)
ServiceError(15-15)src/types/core/media.ts (2)
PlayRecord(12-16)Track(63-63)src/lib/db/schema.ts (1)
bilibiliMetadata(138-155)
src/lib/services/lyricService.ts (5)
src/lib/errors/index.ts (2)
FileSystemError(38-38)DataParsingError(37-37)src/types/player/lyrics.ts (2)
ParsedLrc(18-24)LyricSearchResult(26-32)src/utils/error-handling.ts (1)
toastAndLogError(11-41)src/types/core/media.ts (1)
BilibiliTrack(38-54)src/lib/api/bilibili/api.ts (1)
bilibiliApi(824-824)
src/hooks/stores/usePlayerStore.ts (4)
src/utils/log.ts (2)
flatErrorMessage(111-129)reportErrorToSentry(138-164)src/lib/config/queryClient.ts (1)
queryClient(9-45)src/hooks/queries/db/track.ts (1)
trackKeys(11-32)src/utils/error-handling.ts (1)
toastAndLogError(11-41)
src/lib/services/artistService.ts (5)
src/types/services/artist.ts (2)
CreateArtistPayload(1-7)UpdateArtistPayload(9-13)src/lib/errors/service.ts (2)
createValidationError(70-75)createArtistNotFound(34-42)src/lib/errors/index.ts (1)
ServiceError(15-15)src/types/core/media.ts (1)
Track(63-63)src/lib/db/schema.ts (1)
artists(13-49)
src/features/player/components/PlayerLyrics.tsx (7)
src/types/player/lyrics.ts (1)
LyricLine(3-16)src/hooks/stores/useAppStore.ts (1)
useAppStore(44-166)src/hooks/queries/lyrics/index.ts (2)
useSmartFetchLyrics(14-36)lyricsQueryKeys(6-12)src/features/player/hooks/useLyricSync.ts (1)
useLyricSync(9-146)src/lib/config/queryClient.ts (1)
queryClient(9-45)src/utils/error-handling.ts (1)
toastAndLogError(11-41)src/hooks/stores/useModalStore.ts (1)
useModalStore(29-72)
src/lib/services/downloadService.ts (4)
src/lib/errors/service.ts (2)
createNotImplementedError(77-79)createServiceError(15-25)src/hooks/stores/useAppStore.ts (2)
useAppStore(44-166)serializeCookieObject(36-42)src/utils/log.ts (1)
flatErrorMessage(111-129)src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(47-852)
src/lib/facades/playlist.ts (2)
src/lib/errors/service.ts (1)
createValidationError(70-75)src/lib/errors/facade.ts (1)
createFacadeError(29-39)
src/lib/services/playlistService.ts (5)
src/types/services/playlist.ts (2)
CreatePlaylistPayload(1-8)UpdatePlaylistPayload(10-14)src/lib/errors/service.ts (3)
createPlaylistNotFound(44-53)createTrackNotInPlaylist(55-68)createValidationError(70-75)src/lib/errors/index.ts (1)
ServiceError(15-15)src/lib/db/schema.ts (1)
playlists(86-115)src/types/core/media.ts (1)
Track(63-63)
src/lib/facades/bilibili.ts (2)
src/lib/errors/facade.ts (1)
createFacadeError(29-39)src/lib/api/bilibili/utils.ts (1)
av2bv(32-51)
🔇 Additional comments (8)
src/lib/services/artistService.ts (8)
1-1: Excellent instrumentation approach!The Sentry import and comprehensive tracing pattern throughout this service is well-structured. The consistent use of
Sentry.startSpanwith meaningful operation names (function,db) and span attributes will provide valuable performance insights.
45-76: Well-instrumented method with clear span hierarchy.The nested spans (function → db:insert:artist) provide excellent granularity for performance monitoring. The source attribute will be useful for filtering traces.
84-151: New method implementation looks solid!The find-or-create pattern is correctly implemented with appropriate validation. Excellent use of
span.setAttribute('artistAlreadyExists', ...)to track cache hits vs. misses—this will be valuable for performance analysis.
159-212: LGTM!The instrumentation correctly traces both the existence check and the update operation. The error handling properly distinguishes between
ArtistNotFoundand database errors.
219-263: LGTM!Consistent with
updateArtistpattern—validates existence before deletion with proper tracing throughout.
270-313: LGTM!The
track.countspan attribute will be useful for monitoring query result sizes. Proper error handling for formatting failures.
319-339: LGTM!Clean implementation with
andTeeto set theartist.countattribute—nice use of the neverthrow API.
346-371: LGTM!Straightforward implementation with appropriate tracing.
| Sentry.startSpan({ name: 'io:file:write', op: 'io' }, () => | ||
| lyricFile.write(JSON.stringify(lyrics)), | ||
| ) | ||
| return okAsync(lyrics) |
There was a problem hiding this comment.
Await the span-wrapped lyric writes
Line 132 and Line 141 start Sentry.startSpan calls that wrap lyricFile.write(...), but the returned promise is never awaited nor converted into a ResultAsync. As a result, smartFetchLyrics now reports success even if the write fails, and the rejection is lost outside of the pipeline. Please propagate the promise so IO failures surface again.
@@
- .andThen((lyrics) => {
- logger.info('自动搜索最佳匹配的歌词完成')
- Sentry.startSpan({ name: 'io:file:write', op: 'io' }, () =>
- lyricFile.write(JSON.stringify(lyrics)),
- )
- return okAsync(lyrics)
- })
+ .andThen((lyrics) => {
+ logger.info('自动搜索最佳匹配的歌词完成')
+ return ResultAsync.fromPromise(
+ Sentry.startSpan(
+ { name: 'io:file:write', op: 'io' },
+ () => lyricFile.write(JSON.stringify(lyrics)),
+ ),
+ (e) =>
+ new FileSystemError('写入歌词缓存失败', {
+ cause: e,
+ data: { filePath: lyricFile.uri },
+ }),
+ ).map(() => lyrics)
+ })
@@
- return this.getBestMatchedLyrics(track).andThen((lyrics) => {
- logger.info('自动搜索最佳匹配的歌词完成')
- Sentry.startSpan({ name: 'io:file:write', op: 'io' }, () =>
- lyricFile.write(JSON.stringify(lyrics)),
- )
- return okAsync(lyrics)
- })
+ return this.getBestMatchedLyrics(track).andThen((lyrics) => {
+ logger.info('自动搜索最佳匹配的歌词完成')
+ return ResultAsync.fromPromise(
+ Sentry.startSpan(
+ { name: 'io:file:write', op: 'io' },
+ () => lyricFile.write(JSON.stringify(lyrics)),
+ ),
+ (e) =>
+ new FileSystemError('写入歌词缓存失败', {
+ cause: e,
+ data: { filePath: lyricFile.uri },
+ }),
+ ).map(() => lyrics)
+ })Also applies to: 140-143
🤖 Prompt for AI Agents
In src/lib/services/lyricService.ts around lines 132-135 (and similarly
140-143), the Sentry.startSpan wrapping lyricFile.write returns a promise that
is not awaited, so write failures are swallowed; modify the code to await the
span-wrapped write (or wrap it in a ResultAsync) and propagate rejections—i.e.,
call await Sentry.startSpan(..., () => lyricFile.write(JSON.stringify(lyrics)))
(or return/chain a ResultAsync from the write) and ensure any thrown error is
returned as an Err/ rejected promise so smartFetchLyrics reflects IO failures.
1 participant
Summary by CodeRabbit