feat: performance improve#69
Conversation
SafeDep Report Summary
Package Details
This report is generated by SafeDep Github App |
|
Caution Review failedThe pull request is closed. WalkthroughThis PR refactors error handling and player state management across the codebase by extracting Changes
Sequence Diagram(s)sequenceDiagram
participant OldPattern as Old Pattern<br/>(useCallback)
participant NewPattern as New Pattern<br/>(Module-level +<br/>extraData)
participant Component
participant FlashList
rect rgb(240, 248, 255)
Note over OldPattern: Previous approach
OldPattern->>Component: Component renders
Component->>OldPattern: useCallback evaluates<br/>with deps array
OldPattern->>FlashList: Pass renderItem
FlashList->>FlashList: Re-render may trigger<br/>callback re-evaluation
end
rect rgb(245, 255, 250)
Note over NewPattern: New approach
NewPattern->>NewPattern: Define top-level<br/>renderItem (stable)
Component->>Component: useMemo creates<br/>extraData with deps
Component->>FlashList: Pass stable renderItem<br/>+ memoized extraData
FlashList->>FlashList: Item render uses<br/>stable function &<br/>extraData reference
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/leaderboard.tsx (1)
60-60: ReplaceuseCurrentTrack()withhaveTrackfromusePlayerStoreto align with codebase pattern.This file is the only one using
currentTrackfor the padding calculation. All other files (LocalTrackList, RemoteTrackList, download, settings, and others) use:const haveTrack = usePlayerStore((state) => !!state.currentTrackUniqueKey)Then reference it as:
paddingBottom: haveTrack ? 70 + insets.bottom : insets.bottomAt line 60, replace the
useCurrentTrack()import and hook usage with theusePlayerStorepattern used elsewhere.src/lib/player/playerLogic.ts (1)
16-24: Verify idempotence: risk of duplicate event listener registration.The
initPlayerfunction no longer guards against re-initialization. While the caller insrc/app/_layout.tsx(line 97) checksglobal.playerIsReady, removing the guard frominitPlayeritself makes it fragile:
- Line 19:
setupEventListeners()registers event handlers viaTrackPlayer.addEventListener- TrackPlayer does not automatically deduplicate listeners
- If
initPlayeris called multiple times (e.g., during hot reload, or from another call site without a guard), duplicate listeners will accumulateThis can cause memory leaks and duplicate event handling (e.g., multiple play records for a single track, multiple pause calls on sleep timer).
Consider restoring the idempotence guard:
const initPlayer = async () => { + if (global.playerIsReady) { + logger.debug('播放器已初始化，跳过') + return + } logger.info('开始初始化播放器') await PlayerLogic.preparePlayer() PlayerLogic.setupEventListeners()
🧹 Nitpick comments (1)
docs/principles.md (1)
1-5: Good practice documented.This principle aligns well with React performance best practices by preventing unnecessary function recreations and ensuring FlashList can properly detect changes.
Consider adding a code example to illustrate the pattern:
// ❌ 不好的做法 function MyList() { const data = useData() const currentUser = useCurrentUser() return ( <FlashList data={data} renderItem={({ item }) => <Item item={item} user={currentUser} />} /> ) } // ✅ 推荐的做法 const renderItem = ({ item, extraData }: { item: DataType; extraData?: ExtraDataType }) => { if (!extraData) return null return <Item item={item} user={extraData.user} /> } function MyList() { const data = useData() const currentUser = useCurrentUser() const extraData = useMemo(() => ({ user: currentUser }), [currentUser]) return ( <FlashList data={data} renderItem={renderItem} extraData={extraData} /> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
CHANGELOG.md(1 hunks)docs/principles.md(1 hunks)patches/react-native-paper.patch(0 hunks)pnpm-workspace.yaml(0 hunks)src/app/(tabs)/index.tsx(1 hunks)src/app/(tabs)/settings.tsx(3 hunks)src/app/_layout.tsx(1 hunks)src/app/download.tsx(4 hunks)src/app/leaderboard.tsx(2 hunks)src/app/player.tsx(1 hunks)src/app/playlist/local/[id].tsx(1 hunks)src/app/test.tsx(3 hunks)src/components/NowPlayingBar.tsx(1 hunks)src/components/common/FunctionalMenu.tsx(1 hunks)src/components/modals/PlayerQueueModal.tsx(1 hunks)src/components/modals/edit-metadata/editPlaylistMetadataModal.tsx(1 hunks)src/components/modals/login/CookieLoginModal.tsx(1 hunks)src/components/modals/lyrics/EditLyrics.tsx(1 hunks)src/components/modals/lyrics/ManualSearchLyrics.tsx(4 hunks)src/components/modals/playlist/BatchAddTracksToLocalPlaylist.tsx(3 hunks)src/components/modals/playlist/UpdateTrackLocalPlaylistsModal.tsx(3 hunks)src/features/library/collection/CollectionList.tsx(2 hunks)src/features/library/favorite/FavoriteFolderList.tsx(3 hunks)src/features/library/local/LocalPlaylistList.tsx(2 hunks)src/features/library/multipage/MultiPageVideosList.tsx(2 hunks)src/features/player/components/PlayerFunctionalMenu.tsx(1 hunks)src/features/player/components/PlayerHeader.tsx(1 hunks)src/features/player/components/PlayerLyrics.tsx(4 hunks)src/features/player/components/PlayerTrackInfo.tsx(1 hunks)src/features/playlist/local/components/LocalTrackList.tsx(4 hunks)src/features/playlist/local/hooks/useLocalPlaylistMenu.ts(1 hunks)src/features/playlist/local/hooks/useLocalPlaylistPlayer.ts(1 hunks)src/features/playlist/remote/components/RemoteTrackList.tsx(5 hunks)src/features/playlist/remote/hooks/useCheckLinkedToLocalPlaylist.ts(1 hunks)src/hooks/mutations/bilibili/favorite.ts(1 hunks)src/hooks/mutations/bilibili/video.ts(1 hunks)src/hooks/mutations/db/playlist.ts(1 hunks)src/hooks/stores/usePlayerStore.ts(1 hunks)src/lib/config/queryClient.ts(1 hunks)src/lib/player/playerLogic.ts(1 hunks)src/lib/services/lyricService.ts(1 hunks)src/types/flashlist.ts(1 hunks)src/utils/error-handling.ts(1 hunks)src/utils/log.ts(0 hunks)src/utils/lyrics.ts(1 hunks)src/utils/search.ts(1 hunks)
💤 Files with no reviewable changes (3)
- src/utils/log.ts
- pnpm-workspace.yaml
- patches/react-native-paper.patch
🧰 Additional context used
🧬 Code graph analysis (15)
src/app/(tabs)/settings.tsx (1)
src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/features/library/local/LocalPlaylistList.tsx (2)
src/types/core/media.ts (1)
Playlist(65-78)src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/app/test.tsx (1)
src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/app/leaderboard.tsx (1)
src/features/leaderboard/LeaderBoardItem.tsx (1)
LeaderBoardListItem(18-135)
src/components/modals/playlist/BatchAddTracksToLocalPlaylist.tsx (2)
src/types/flashlist.ts (1)
ListRenderItemInfoWithExtraData(3-8)src/types/core/media.ts (1)
Playlist(65-78)
src/features/playlist/local/components/LocalTrackList.tsx (4)
src/types/flashlist.ts (1)
ListRenderItemInfoWithExtraData(3-8)src/types/core/media.ts (2)
Track(63-63)Playlist(65-78)src/features/playlist/local/components/LocalPlaylistItem.tsx (1)
TrackListItem(35-236)src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/features/player/components/PlayerLyrics.tsx (2)
src/types/flashlist.ts (1)
ListRenderItemInfoWithExtraData(3-8)src/types/player/lyrics.ts (1)
LyricLine(3-16)
src/components/modals/playlist/UpdateTrackLocalPlaylistsModal.tsx (2)
src/types/flashlist.ts (1)
ListRenderItemInfoWithExtraData(3-8)src/types/core/media.ts (1)
Playlist(65-78)
src/components/modals/lyrics/ManualSearchLyrics.tsx (2)
src/types/flashlist.ts (1)
ListRenderItemInfoWithExtraData(3-8)src/types/player/lyrics.ts (1)
LyricSearchResult(26-32)
src/features/library/multipage/MultiPageVideosList.tsx (2)
src/types/apis/bilibili.ts (1)
BilibiliFavoriteListContent(363-363)src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/features/library/favorite/FavoriteFolderList.tsx (1)
src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/utils/error-handling.ts (2)
src/lib/errors/index.ts (1)
CustomError(1-13)src/utils/log.ts (1)
flatErrorMessage(111-129)
src/features/library/collection/CollectionList.tsx (1)
src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/app/download.tsx (2)
src/types/core/downloadManagerStore.ts (1)
DownloadTask(1-7)src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
src/features/playlist/remote/components/RemoteTrackList.tsx (2)
src/types/flashlist.ts (1)
ListRenderItemInfoWithExtraData(3-8)src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(45-728)
🔇 Additional comments (51)
src/app/player.tsx (1)
8-8: Import path update verified as complete.The migration from
@/hooks/stores/playerHooks/useCurrentTrackto@/hooks/player/useCurrentTrackis fully complete across the codebase. All 7 imports in the codebase have been updated to use the new path, the old hook file has been removed, and the new hook maintains API compatibility (returnsTrack | null). The change on line 8 is correct with no outstanding concerns.src/app/leaderboard.tsx (2)
43-54: Good performance optimization with module-scoped renderItem.Moving
renderItemto module scope is an excellent performance optimization. The function is now created once rather than on every component render. SinceLeaderBoardListItemuses Zustand store subscriptions internally (accessingusePlayerStoreforisCurrentTrack), it will automatically re-render when needed without requiringextraDataon the FlashList.
3-3: Import path and hook are valid; no issues found.The new import path
@/hooks/player/useCurrentTrackis correct. The hook exists atsrc/hooks/player/useCurrentTrack.ts, is properly exported as a default export, and returnsTrack | nullas expected. The hook's usage in the file (line 60 for the call, line 129 for the ternary check on padding) is semantically correct and handles the null case properly.src/features/player/components/PlayerTrackInfo.tsx (1)
2-2: LGTM - Clean refactor.The import path consolidation improves code organization by moving player hooks to a dedicated directory.
src/features/player/components/PlayerHeader.tsx (1)
1-1: LGTM - Consistent with hook consolidation.Import path update aligns with the broader refactor to consolidate player hooks.
CHANGELOG.md (1)
16-20: LGTM - Clear changelog entries.The changelog appropriately documents the performance optimizations and Menu component fix with a reference to the upstream issue.
src/components/modals/PlayerQueueModal.tsx (1)
1-2: LGTM - Hook consolidation refactor.Import paths updated consistently with the broader effort to organize player hooks under a dedicated directory.
src/features/library/multipage/MultiPageVideosList.tsx (3)
18-22: LGTM - Follows documented principles.The module-scope
renderPlaylistItemfunction correctly follows the principle documented indocs/principles.md. SinceMultiPageVideosItemonly depends on theitemprop, noextraDataobject is needed, which is the appropriate simplification of the pattern.
26-26: Good optimization - using haveTrack for boolean check.Switching from
useCurrentTrack()to derivinghaveTrackfromusePlayerStore((state) => !!state.currentTrackUniqueKey)is more efficient for the boolean check use case. This avoids subscribing to the entire currentTrack object when only track presence is needed.
112-112: Consistent with haveTrack refactor.The padding calculation correctly uses the new
haveTrackboolean flag, maintaining the same conditional logic (70px when track exists for NowPlayingBar, 10px otherwise).src/hooks/mutations/bilibili/video.ts (1)
4-4: LGTM - Error handling consolidation.Moving
toastAndLogErrorto a dedicated error-handling module improves code organization and separation of concerns.src/lib/config/queryClient.ts (1)
4-4: LGTM! Clean import path refactor.The import path change aligns with the centralized error-handling module structure.
src/utils/lyrics.ts (1)
2-3: LGTM! Clean separation of concerns.Splitting error handling and logging imports makes the module boundaries clearer.
src/components/modals/login/CookieLoginModal.tsx (1)
5-5: LGTM! Consistent with the error-handling refactor.src/features/playlist/remote/hooks/useCheckLinkedToLocalPlaylist.ts (1)
3-3: LGTM! Aligns with the centralized error-handling pattern.src/components/modals/lyrics/EditLyrics.tsx (1)
6-6: LGTM! Consistent with the error-handling module migration.src/hooks/mutations/db/playlist.ts (1)
10-10: LGTM! Clean migration to the centralized error-handling module.src/app/(tabs)/index.tsx (1)
7-7: LGTM! Follows the error-handling refactor pattern.src/features/player/components/PlayerFunctionalMenu.tsx (1)
2-2: Hook path migration verified as complete.Migration to the new player hooks structure is successful. Verification confirms:
- No stale
@/hooks/stores/playerHooksimports remain- New modules exist at consolidated locations (
src/utils/error-handling.ts,src/hooks/player/useCurrentTrack.ts)- Codebase is consistent with the refactored import paths
src/utils/search.ts (1)
4-5: LGTM! Clean separation of concerns.The refactoring correctly moves
toastAndLogErrorto a dedicated error-handling module while maintaining thelogimport from the logging module. This improves code organization and maintainability.src/components/NowPlayingBar.tsx (1)
2-2: LGTM! Import path consolidation.The updated import path for
useCurrentTrackaligns with the broader player hook consolidation mentioned in the PR objectives.src/features/playlist/local/hooks/useLocalPlaylistMenu.ts (1)
9-9: LGTM! Consistent error-handling module usage.The import update correctly references the new dedicated error-handling module.
src/app/playlist/local/[id].tsx (1)
26-26: LGTM! Aligned with error-handling refactor.The import correctly uses the new error-handling module.
src/components/modals/edit-metadata/editPlaylistMetadataModal.tsx (1)
5-6: LGTM! Proper import separation.The split imports correctly separate error handling from logging utilities while maintaining all necessary functionality.
src/lib/services/lyricService.ts (1)
7-8: LGTM! Consistent module separation.The import reorganization properly separates error handling from logging utilities.
src/features/playlist/local/hooks/useLocalPlaylistPlayer.ts (1)
4-4: LGTM! Error-handling module usage.The import correctly references the dedicated error-handling module.
src/hooks/mutations/bilibili/favorite.ts (1)
4-5: LGTM! Import refactoring complete.The import split correctly separates error-handling utilities from logging, completing the consistent refactoring pattern across the codebase.
src/app/(tabs)/settings.tsx (1)
4-6: LGTM! Clean refactor to centralized error handling and player state.The migration from
useCurrentTrackto derivinghaveTrackdirectly fromusePlayerStore(state => !!state.currentTrackUniqueKey)is more efficient and aligns with the PR's consolidation objectives. The error handling import path change to@/utils/error-handlingis consistent with the codebase-wide refactor.Also applies to: 27-27, 41-41
src/app/test.tsx (1)
7-8: LGTM! Consistent with codebase-wide refactor.The separation of error handling utilities to
@/utils/error-handlingwhile keeping logging functions in@/utils/logimproves module organization. ThehaveTrackpattern adoption aligns with changes across other files.Also applies to: 24-24, 152-152
src/hooks/stores/usePlayerStore.ts (1)
15-16: LGTM! Import path consolidation.The refactor moves
toastAndLogErrorto a dedicated error-handling module while preserving other log utilities in their original location. No runtime behavior changes.src/app/_layout.tsx (1)
11-12: LGTM! Import reorganization aligns with centralized error handling.src/types/flashlist.ts (1)
1-8: LGTM! Well-defined generic type for FlashList extraData pattern.The
ListRenderItemInfoWithExtraDatatype correctly extendsListRenderItemInfoto support typedextraData, enabling type-safe module-levelrenderItemfunctions used throughout the PR.src/features/library/local/LocalPlaylistList.tsx (1)
5-5: LGTM! Efficient refactor with module-level renderItem.Moving
renderPlaylistItemto module scope (lines 13-15) is appropriate since it has no component-specific dependencies, improving render stability. ThehaveTrackpattern (line 19) and padding calculation (line 80) are consistent with the codebase-wide refactor.Also applies to: 13-15, 19-19, 80-80
src/app/download.tsx (1)
5-5: LGTM! Clean refactor aligning with codebase patterns.The module-level
renderItem(lines 15-17) andhaveTrackpattern (line 30) are consistent with changes across the PR. The padding calculation correctly accounts for the NowPlayingBar height when a track is active.Also applies to: 15-17, 30-30, 53-53
src/features/library/collection/CollectionList.tsx (2)
15-17: LGTM! Module-level renderItem improves performance.Moving
renderCollectionItemto module scope prevents function recreation on every render, which is a good performance optimization. SinceCollectionListItemonly depends on theitemprop, noextraDatais needed here.
21-21: LGTM! Using haveTrack flag avoids unnecessary re-renders.Deriving
haveTrackfromcurrentTrackUniqueKeyis a good optimization. This boolean flag will only trigger re-renders when track presence changes, not when track content changes.src/features/library/favorite/FavoriteFolderList.tsx (2)
16-18: LGTM! Consistent with the module-level renderItem pattern.This follows the same performance optimization pattern as other list components in this PR.
23-23: LGTM! Consistent haveTrack usage.The
haveTrackflag is consistently derived fromusePlayerStoreacross all list components in this PR.src/components/modals/lyrics/ManualSearchLyrics.tsx (2)
23-41: LGTM! extraData pattern correctly implemented.The module-level
renderItemwithextraDatais properly implemented:
- Throws an error if
extraDatais missing (defensive programming)- Extracts required handlers from
extraData- Passes them to
SearchItemThis pattern ensures that
renderItemdoesn't get recreated on every render while still having access to the latest handlers.
94-97: LGTM! extraData properly memoized.The
extraDataobject is correctly memoized with all necessary dependencies (handlePressItem,isFetchingLyrics). This ensures FlashList only re-renders when these values actually change.src/components/modals/playlist/BatchAddTracksToLocalPlaylist.tsx (2)
14-35: LGTM! extraData pattern correctly implemented for playlist selection.The module-level
renderPlaylistItemproperly usesextraDatato accessselectedPlaylistIdandsetSelectedPlaylistId. The error guard ensures the function fails fast ifextraDatais missing.
97-103: LGTM! extraData properly memoized with stable dependencies.The
extraDataobject is memoized with the correct dependencies. Note thatsetSelectedPlaylistIdis a stable function reference fromuseState, so it won't cause unnecessary re-renders.src/components/modals/playlist/UpdateTrackLocalPlaylistsModal.tsx (2)
17-41: LGTM! extraData pattern for checkbox state management.The module-level
renderPlaylistItemcorrectly usesextraDatato passcheckedPlaylistIdsandhandleCheckboxPressto the rendered item. The implementation is consistent with other modal components in this PR.
166-172: LGTM! extraData memoization is correct.All dependencies are properly included:
checkedPlaylistIds(state),handleCheckboxPress(memoized callback).src/utils/error-handling.ts (1)
1-41: LGTM! Centralizing error handling improves maintainability.The
toastAndLogErrorfunction properly handles different error types and provides consistent error reporting. The implementation correctly:
- Distinguishes between
CustomError, standardError,undefined, and unknown types- Flattens nested error messages using
flatErrorMessage- Logs to the appropriate scope
- Displays user-friendly toasts
However, verify that
duration: Number.POSITIVE_INFINITYis intentional. This means error toasts will never auto-dismiss and require manual user action. This might be the desired behavior for errors, but consider whether some error types should auto-dismiss after a longer timeout.src/features/player/components/PlayerLyrics.tsx (3)
169-190: LGTM! extraData pattern for lyric synchronization.The module-level
renderItemcorrectly usesextraDatato passcurrentLyricIndexandhandleJumpToLyrictoLyricLineItem. This ensures:
- The
renderItemfunction is not recreated on every render (performance win)- Lyric highlighting updates correctly when
currentLyricIndexchanges- Jump-to-lyric functionality remains accessible
10-10: LGTM! Error handling import updated to centralized module.This aligns with the broader refactoring to consolidate error handling in a dedicated module, improving maintainability.
273-279: LGTM! extraData properly memoized for lyric rendering.The
extraDatais memoized with the correct dependencies (currentLyricIndex,handleJumpToLyric), ensuring FlashList re-renders only when necessary.src/features/playlist/local/components/LocalTrackList.tsx (3)
34-78: LGTM! Complex extraData correctly manages track list interactions.The module-level
renderItemproperly usesextraDatato pass all necessary handlers and state for track list functionality:
- Selection mode state (
selectMode,selected)- Event handlers (
handleTrackPress,handleMenuPress,toggle,enterSelectMode)- Context data (
playlist)While the
extraDataobject is large (7 properties), this is necessary for the rich interaction model of the track list. The implementation is correct.
121-140: LGTM! All dependencies properly included in extraData memoization.The
useMemodependency array correctly includes all properties that make up theextraDataobject:
- State:
selectMode,selected- Callbacks:
handleTrackPress,handleMenuPress,toggle,enterSelectMode- Data:
playlistThis ensures the
extraDataobject is only recreated when necessary, optimizing FlashList performance.
94-94: LGTM! Consistent haveTrack pattern for layout adjustments.The
haveTrackflag is used to adjust bottom padding (line 153), ensuring the track list doesn't get obscured by the mini player when a track is playing. This is consistent with other list components in this PR.
1 participant
Summary by CodeRabbit
Performance
Bug Fixes
Documentation