fix: new arch #631
fix: new arch #631
Conversation
fk.
wtf fb
resource exceed size limit etc
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request includes significant updates across various components of a React Native application. Key changes involve enhancing error handling and UI logic in Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (11)
src/components/landscape/AzusaPlayerLandscape.tsx (1)
37-39: Consider consolidating theme selection logic.While the separate navigation theme handling is good for separation of concerns, the theme selection logic is duplicated. Consider extracting the theme selection into a custom hook or utility function to maintain DRY principles.
+const useThemes = (isDark: boolean) => { + const combinedTheme = isDark ? CombinedDarkTheme : CombinedDefaultTheme; + const navTheme = isDark ? NavigationDarkTheme : NavigationDefaultTheme; + return { combinedTheme, navTheme }; +}; const AzusaPlayer = () => { const playerStyle = useNoxSetting(state => state.playerStyle); - const defaultTheme = playerStyle.metaData.darkTheme - ? CombinedDarkTheme - : CombinedDefaultTheme; - const defaultNavTheme = playerStyle.metaData.darkTheme - ? NavigationDarkTheme - : NavigationDefaultTheme; + const { combinedTheme: defaultTheme, navTheme: defaultNavTheme } = + useThemes(playerStyle.metaData.darkTheme);src/hooks/useSetupPlayer.ts (1)
60-62: Address the TODO comment regarding VIP-specific loading.The TODO comment indicates that VIP users should have different loading behavior, but this isn't implemented yet.
Would you like me to help implement the VIP-specific loading logic or create a GitHub issue to track this task?
Consider moving loadRN call after VIP check.
The
loadRN()call should ideally be placed after checking the VIP status to align with the TODO comment's intent.- NoxModule?.loadRN(); + // Check VIP status first + if (!isVIP) { + NoxModule?.loadRN(); + }src/components/playlists/View.tsx (5)
Line range hint
29-39: Add proper TypeScript types for components.The
BiliCardcomponent and the default export are usinganytype for props, which reduces type safety. Consider adding proper interfaces:interface BiliCardProps { backgroundURI?: string; children: React.ReactNode; } interface DrawerViewProps { // Add any props passed from the parent style?: StyleProp<ViewStyle>; children?: React.ReactNode; }Also applies to: 86-86
Line range hint
1-1: Remove global ESLint disable and fix dependencies.Instead of disabling
react-hooks/exhaustive-depsglobally, address the dependencies properly in each hook. This helps prevent stale closure bugs.-/* eslint-disable react-hooks/exhaustive-deps */Also applies to: 42-45
Line range hint
47-63: Extract deep linking logic to a custom hook.Consider moving the deep linking logic to a custom hook (e.g.,
useNotificationDeepLink) to improve reusability and separation of concerns:function useNotificationDeepLink(onNotificationClick: () => void) { useEffect(() => { function deepLinkHandler(data: { url: string }) { if (data.url === 'trackplayer://notification.click') { onNotificationClick(); } } const subscription = Linking.addEventListener('url', deepLinkHandler); return () => subscription.remove(); }, [onNotificationClick]); }
Line range hint
86-113: Consider component restructuring for better separation of concerns.The component handles multiple responsibilities (navigation, deep linking, playlist management). Consider:
- Renaming the file to something more specific (e.g.,
DrawerNavigationView.tsx)- Splitting into smaller, focused components
- Moving playlist-related logic to a separate component
This would improve maintainability and make the code easier to test.
I now have enough context to provide a final response about the HACK comment and hook update issue.
Consider moving Android Auto initialization logic to a dedicated effect
The current implementation has two issues:
- The hook update issue stems from mixing Android Auto initialization with intent handling in the same effect
- The component is using a workaround by calling
buildBrowseTreedirectly instead of properly handling updatesSuggested changes:
- Move the intent handling logic to a separate effect that only depends on
intentData- Add proper dependencies to the effect in
useAndroidAutoListenerto trigger updates when playlists change- Consider splitting Android Auto initialization and intent handling into separate hooks for better separation of concerns
🔗 Analysis chain
Line range hint
42-45: Address the HACK comment.The comment indicates an issue with hook updates. Consider investigating why the hook isn't updating in its own
useEffectand fix the root cause instead of working around it.Let's check for similar patterns in other files:
Let me gather more context about the HACK comment and the hook update issue.
Let me search for more context about the hook update issue in the specific file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usePlaybackAA hook usage patterns ast-grep --pattern 'const { $_ } = usePlaybackAA()'Length of output: 137
Script:
#!/bin/bash # Search for HACK comments near useEffect and hook usage rg "HACK" -B 2 -A 2 # Look for useEffect implementations in the same file ast-grep --pattern 'useEffect(() => { $$$ })'Length of output: 24758
Script:
#!/bin/bash # Look at the implementation of usePlaybackAA hook and its usage in View.tsx cat src/components/playlists/View.tsx # Look at the implementation of usePlaybackAA cat src/hooks/usePlaybackAA.tsLength of output: 6121
src/AzusaPlayer.tsx (2)
85-87: Good separation of navigation-specific theming!The introduction of a separate navigation theme variable improves the architecture by properly separating navigation-specific styling from the general app theme. This makes the theming system more maintainable and follows React Navigation's best practices.
127-131: Consider making drawer width more adaptive.While using percentage for drawer width is responsive, a hardcoded 70% might not be optimal for all device sizes. Consider:
- Using responsive width calculation based on screen size
- Making the width configurable through settings
Example implementation:
-drawerStyle={{ - width: '70%', - borderTopRightRadius: 0, - borderBottomRightRadius: 0, -}} +drawerStyle={{ + width: Math.min(deviceWidth * 0.7, MAX_DRAWER_WIDTH), + borderTopRightRadius: 0, + borderBottomRightRadius: 0, +}}android/app/src/main/java/com/noxplay/noxplayer/APMWidget.kt (1)
40-49: Consider enhancing error feedback to users.The error handling for service binding is good, but the TODO comment suggests a need for user feedback. Consider implementing a toast message or notification to inform users when the service binding fails.
try { binder = mBinder as MusicService.MusicBinder if (!binder.isBinderAlive) return false } catch (e: Exception) { // HACK: likely this is bound by another binder. for example // android.service.media.MediaBrowserService$ServiceBinder. - // TODO: display some error message here? Timber.tag("APM-widget").e(e) + context?.let { + Toast.makeText(it, "Unable to connect to music service", Toast.LENGTH_SHORT).show() + } return false }patches/react-native-screens+4.0.0.patch (1)
167-167: Fix Syntax Error in Commented-Out Code or Remove ItThe commented code on line 167 is missing '+' operators, making it syntactically incorrect. Please correct the syntax or remove the commented code if it's no longer necessary to maintain codebase cleanliness.
Apply this diff to fix the commented code:
-// return Rect(screen.x.toInt(), -screen.y.toInt(), screen.x.toInt() screen.width, screen.y.toInt() screen.height) +// return Rect(screen.x.toInt(), -screen.y.toInt(), screen.x.toInt() + screen.width, screen.y.toInt() + screen.height)Alternatively, consider deleting the commented-out code if it's obsolete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
android/app/src/main/java/com/noxplay/noxplayer/APMWidget.kt(3 hunks)android/settings.gradle(1 hunks)package.json(4 hunks)patches/@react-navigation+drawer+7.0.0.patch(1 hunks)patches/react-native-paper+5.12.5.patch(1 hunks)patches/react-native-screens+4.0.0.patch(3 hunks)patches/react-native-tab-view+3.5.2.patch(0 hunks)src/AzusaPlayer.tsx(3 hunks)src/components/landscape/AzusaPlayerLandscape.tsx(2 hunks)src/components/player/Lyric.tsx(1 hunks)src/components/playlists/View.tsx(2 hunks)src/hooks/useSetupPlayer.ts(3 hunks)src/stores/initializeStores.ts(0 hunks)
💤 Files with no reviewable changes (2)
- patches/react-native-tab-view+3.5.2.patch
- src/stores/initializeStores.ts
🧰 Additional context used
🪛 Biome
src/hooks/useSetupPlayer.ts
[error] 84-85: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
🔇 Additional comments (17)
patches/react-native-paper+5.12.5.patch (1)
9-10: LGTM! Good accessibility improvement.
Rounding the progress value for accessibility announcements is a good practice as it provides a cleaner, more user-friendly experience for screen reader users.
Let's verify if this accessibility improvement is consistently applied across other progress indicators:
✅ Verification successful
Let me try another search strategy to ensure we have comprehensive coverage of progress-related accessibility implementations.
Let me try one final search focusing specifically on the patch file content and any similar patterns:
Accessibility value rounding is correctly implemented
The search results show that there are only two components with accessibility values in the patch files:
- The ProgressBar component where the rounding is being added
- A Slider component that already uses direct values without multiplication (thus no rounding needed)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other progress indicators in the codebase need similar rounding
# for accessibility values
# Search for other progress indicators that might need similar treatment
rg -A 5 'accessibilityRole=["'\'']progressbar["'\'']'
# Look for other accessibility value calculations that might benefit from rounding
ast-grep --pattern 'accessibilityValue = { $$ }'
Length of output: 624
Script:
#!/bin/bash
# Search for progress-related components and their accessibility implementations
rg -g '!*.test.*' -g '!*.spec.*' -A 10 'progress.*accessibilityValue|accessibilityValue.*progress'
# Search specifically for ProgressBar components
rg -g '!*.test.*' -g '!*.spec.*' -A 10 'class.*ProgressBar|function.*ProgressBar'
# Look for other numeric accessibility values that might need rounding
rg -g '!*.test.*' -g '!*.spec.*' 'accessibilityValue.*\{.*:.*\*.*\}'
Length of output: 255
Script:
#!/bin/bash
# Check the complete patch file content
cat patches/react-native-paper+5.12.5.patch
# Look for any other accessibility value calculations in patch files
rg -g '*.patch' 'accessibilityValue'
# Search for progress-related components in patch files
rg -g '*.patch' 'ProgressBar|progress'
Length of output: 2185
patches/@react-navigation+drawer+7.0.0.patch (1)
5-11: Verify drawer styling behavior
The change moves drawerStyle from being internally destructured to being passed as a parameter. While the logic appears correct, please verify:
- Default styling still works when no drawerStyle is provided
- Custom styles are properly applied in all drawer states
Also applies to: 13-20
✅ Verification successful
Drawer style parameter change is safe and properly handled
The change simply moves the drawerStyle from being destructured within the function to being a direct parameter, which is a refactoring that maintains the same functionality. Evidence:
- The patch shows a clean move of
drawerStylefrom internal destructuring to parameter list - The codebase already uses
drawerStyleprop as expected (e.g., inAzusaPlayer.tsx) - No other modifications to style handling logic are present in the patch
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing drawerStyle usage to ensure compatibility
rg "drawerStyle" --type ts --type tsx --type js --type jsx -B 2 -A 2
Length of output: 96
Script:
#!/bin/bash
# Search for drawerStyle usage without file type restrictions since the patch is modifying React Navigation
rg "drawerStyle" -B 2 -A 2
Length of output: 1181
Script:
#!/bin/bash
# Look for the drawer component implementation to understand style handling
ast-grep --pattern 'function DrawerView$_($$$) {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Check the patch file content for complete context
cat patches/@react-navigation+drawer+7.0.0.patch
Length of output: 972
android/settings.gradle (1)
6-6: Verify the removal of react-native-code-push module.
The removal of the code-push module will affect the app's over-the-air update capabilities. Please confirm if this is intentional and if there's an alternative update strategy in place.
✅ Verification successful
The removal of react-native-code-push is intentional and part of the architectural changes
Based on the git history, we can see that code-push is being systematically removed from the project:
- The module was removed from
settings.gradle - Multiple imports of
codePushwere commented out across the codebase - The dependency was removed from the project configuration
This aligns with the PR's objective of implementing a "new arch" and the transition to using Expo modules, which provides its own update mechanisms through EAS Update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining code-push dependencies or usage
# Check if code-push is still referenced in package.json
echo "Checking package.json for code-push dependencies..."
rg -i "code-push" package.json
# Check for any remaining code-push imports or usage in the codebase
echo "Checking for code-push usage in source files..."
rg -i "code-push" --type js --type ts --type jsx --type tsx
Length of output: 336
Script:
#!/bin/bash
# Let's try a different approach to check for code-push related files and dependencies
# Check package.json with cat and grep instead
echo "Checking package.json for code-push dependencies..."
cat package.json | grep -i "code-push"
# Use find and grep to search for code-push in source files
echo "Checking source files for code-push usage..."
find . -type f \( -name "*.js" -o -name "*.ts" -o -name "*.tsx" \) -exec grep -l -i "code-push" {} \;
# Check for code-push related files in android directory
echo "Checking android directory for code-push related files..."
find ./android -type f -exec grep -l -i "code-push" {} \;
# Check git history for code-push removal
echo "Checking git history for code-push removal..."
git log -p | grep -i "code-push"
Length of output: 10888
src/components/landscape/AzusaPlayerLandscape.tsx (1)
62-62: Address technical debt in background color handling.
The HACK comment and background color override with undefined suggest a design issue that should be properly addressed. This could lead to unexpected behavior or styling inconsistencies.
Let's verify the background color handling across the codebase:
Consider:
- Document the current background handling approach in MainBackground
- Create a proper theming hierarchy instead of using overrides
- Remove the HACK comment by implementing a proper fix
src/hooks/useSetupPlayer.ts (2)
14-16: LGTM: Native module import and initialization.
The addition of NativeModules import and module initialization follows React Native best practices.
95-98: Consider a more robust solution for notification click handling.
The current implementation uses a HACK comment and might introduce race conditions. A more robust solution should:
- Handle the notification click state in a more predictable way
- Avoid relying on playerReady as a trigger for loadRN
- Consider implementing a proper state machine for player initialization
Let's verify if this notification click issue is documented or if there are similar workarounds:
src/components/playlists/View.tsx (1)
86-86: LGTM! Good style refactoring.
Moving the inline style to StyleSheet follows React Native best practices and improves maintainability.
Also applies to: 115-115
src/AzusaPlayer.tsx (1)
112-112: Consider addressing the background color technical debt.
While the font configuration is correctly implemented, the HACK comment above suggests there's technical debt in the background color handling. Consider refactoring the background color implementation to eliminate the need for the hack.
Let's check for similar background color workarounds:
package.json (4)
33-35: LGTM: CLI tools version update.
The React Native Community CLI packages have been consistently updated to version 15.1.2.
100-100: Review security implications of custom forks.
Using custom forks for critical packages like react-native-lyric and react-native-track-player could pose security risks. Consider:
- Regularly syncing with upstream repositories
- Documenting the reasons for using forks
- Setting up automated security scanning
#!/bin/bash
# Check if the custom forks have recent commits compared to upstream
echo "Checking react-native-lyric fork..."
gh repo view lovegaoshi/react-native-lyric --json updatedAt,pushedAt
echo "Checking react-native-track-player fork..."
gh repo view lovegaoshi/react-native-track-player --json updatedAt,pushedAtAlso applies to: 113-113
64-64: Verify compatibility between updated packages.
Several interdependent packages have been updated:
- expo: ^52.0.0-preview.23
- i18next: ^23.16.5
- react-native-screens: 4.0.0
- react-native-tab-view: ^4.0.1
Ensure these versions are compatible, especially:
- react-native-screens v4 with React Navigation v7
- react-native-tab-view v4 with React Navigation's material-top-tabs
#!/bin/bash
# Check for potential version conflicts in package.json
echo "Checking for version conflicts..."
rg -l "react-native-screens|react-native-tab-view|@react-navigation"
# Look for any error patterns in the codebase
echo "Checking for common migration issues..."
rg "ScreenStack|TabView|createMaterialTopTabNavigator"Also applies to: 76-76, 106-106, 111-111
43-46:
Verify React Navigation v7 migration changes.
The upgrade to React Navigation v7 introduces breaking changes that require code updates:
- Screen configuration syntax changes
- New TypeScript types
- Updated screen lifecycle behaviors
Please ensure the following migration steps are completed:
✅ Verification successful
React Navigation v7 upgrade appears compatible with existing codebase
The codebase shows proper implementation of React Navigation v7:
- Correct imports from
@react-navigationpackages - Proper usage of navigation containers and navigators
- Theme configuration follows v7 patterns with
adaptNavigationTheme - No deprecated v6 patterns found (like old ref handling or linking configurations)
The navigation setup in both AzusaPlayer.tsx and landscape mode follows current best practices, and no breaking changes from v7 appear to affect the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potentially affected navigation configurations
echo "Checking for affected navigation patterns..."
rg -l "createNativeStackNavigator|createDrawerNavigator|NavigationContainer"
# Look for common v6 patterns that need updates
echo "Checking for v6 patterns that need updates..."
rg "screenOptions={{[^}]*headerShown:\s*(true|false)[^}]*}}"
Length of output: 2808
Script:
#!/bin/bash
# Check for navigation configuration patterns
echo "Checking navigation configurations..."
rg -A 5 "NavigationContainer|createNativeStackNavigator|createDrawerNavigator" src/
# Check for screen options patterns
echo "Checking screen options..."
rg -A 3 "screenOptions\s*=\s*{" src/
# Check for navigation-related component imports
echo "Checking navigation imports..."
rg "^import.*@react-navigation" src/
Length of output: 8568
Script:
#!/bin/bash
# Check for screen options with fixed syntax
echo "Checking screen options patterns..."
rg -A 3 "screenOptions.*=.*{" src/
# Check for v6 specific navigation patterns that might need updates
echo "Checking for potential v6 patterns..."
rg -A 3 "(useNavigationContainerRef|navigationRef|linking:|useLinking)" src/
# Check for navigation theme usage
echo "Checking navigation theme usage..."
rg -A 3 "(NavigationDarkTheme|NavigationDefaultTheme|NavigationTheme)" src/
Length of output: 3291
src/components/player/Lyric.tsx (1)
131-132: Improved color system usage for karaoke text.
The switch to onSurface and onSurfaceVariant colors aligns better with Material Design principles for text display.
Let's verify the contrast ratios for accessibility:
✅ Verification successful
Based on the search results, I can now generate the final response:
Color contrast verified for karaoke text display
The implementation uses well-defined theme colors with good contrast ratios:
onSurfaceis set to#fcf75e(bright yellow)onSurfaceVariantis set torgb(202, 200, 165)(light gray)
These colors provide sufficient contrast for karaoke text display, with the bright yellow for active lyrics and a muted gray for inactive ones, ensuring good readability and visual hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom color overrides or theme definitions that might affect these colors
rg -A 5 "onSurface|onSurfaceVariant" --type ts --type tsx
# Look for any theme configuration files
fd -e ts -e tsx -e json "theme|style|color" --exec cat {} \; | grep -A 10 -B 10 "onSurface\|onSurfaceVariant"
Length of output: 2183
android/app/src/main/java/com/noxplay/noxplayer/APMWidget.kt (1)
57-57: Clarify spinner implementation details.
The TODO comment about showing a spinner needs more context. Consider:
- Where should the spinner be displayed in the widget layout?
- Should it replace the album art temporarily?
- How long should it be shown?
Let's check if the widget layout supports adding a spinner:
✅ Verification successful
Let me check the actual widget layout file to understand the current structure and potential places for the spinner.
Let me check the context where this TODO comment is used to understand when the spinner might be needed.
Consider using the existing album art ImageView for loading state
The TODO appears in bindOrStartService() when the service binding fails and needs to start. Since we have the widget layout context:
- The
albumArtImageView (80dp x match_parent) is the ideal place to show a loading indicator - You can overlay a ProgressBar on the album art or replace the album art source with an animated drawable during loading
- The spinner should be shown until the service successfully binds or fails
Example implementation:
private fun showLoading(show: Boolean) {
val views = RemoteViews(context.packageName, R.layout.a_p_m_widget)
views.setImageViewResource(R.id.albumArt,
if (show) R.drawable.loading_spinner
else R.mipmap.ic_launcher_foreground)
// Update widget
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check widget layout file for potential spinner placement
rg -A 10 "a_p_m_widget" --glob "*.xml"
Length of output: 1917
Script:
#!/bin/bash
# Check the widget layout file
rg -l "layout/a_p_m_widget" --glob "*.xml"
cat android/app/src/main/res/layout/a_p_m_widget.xml
Length of output: 5626
Script:
#!/bin/bash
# Check the APMWidget.kt file for context around the TODO comment
rg -B 5 -A 5 "TODO: show a spinner" android/app/src/main/java/com/noxplay/noxplayer/APMWidget.kt
Length of output: 539
patches/react-native-screens+4.0.0.patch (3)
19-22: Verify the Modified Condition in ScreenFragment
The condition in ScreenFragment.kt has been updated to include screen.container?.topScreen === screen. Please verify that this change behaves as intended and does not introduce any unintended side effects in the animation start event handling.
40-64: Ensure Standard Transitions Provide Desired Animation Effects
Custom animations have been replaced with standard fragment transitions using setTransition. Verify that the new transitions (TRANSIT_FRAGMENT_OPEN, TRANSIT_FRAGMENT_CLOSE, TRANSIT_FRAGMENT_FADE) deliver the intended animations and maintain a consistent user experience.
Also applies to: 71-91
109-162: onCreateAnimator Implementation Looks Correct
The new onCreateAnimator method in ScreenStackFragment.kt effectively handles animation events for system animations. The logic is clear and should improve animation handling.
| diff --git a/node_modules/@react-navigation/drawer/lib/commonjs/views/DrawerView.js b/node_modules/@react-navigation/drawer/lib/commonjs/views/DrawerView.js | ||
| index 0563a7c..6315acb 100644 | ||
| --- a/node_modules/@react-navigation/drawer/lib/commonjs/views/DrawerView.js | ||
| +++ b/node_modules/@react-navigation/drawer/lib/commonjs/views/DrawerView.js |
There was a problem hiding this comment.
Modifying files in node_modules is generally discouraged as:
- Changes will be lost on every
npm install - It makes the build non-reproducible
- It can cause conflicts with other packages
Consider these alternatives:
- Fork the package and use your fork
- Create a proper PR to the original package
- Use package.json's
patch-packageto manage patches systematically
| apply from: new File(["node", "--print", "require.resolve('expo/package.json')"].execute(null, rootDir).text.trim(), "../scripts/autolinking.gradle") | ||
| useExpoModules() |
There was a problem hiding this comment.
💡 Codebase verification
Expo configuration is not fully aligned with new architecture
The codebase appears to be using a hybrid setup where Expo modules are used alongside bare React Native. While useExpoModules() is added, there are potential issues:
- The new architecture flags are not found in
android/app/build.gradle - The project seems to be primarily using bare React Native modules (evidenced by numerous direct React Native imports) rather than Expo modules
- The Expo configuration is minimal, with only
expo-secure-storefound in dependencies
To properly support the new architecture with Expo:
- Remove
useExpoModules()since this project is not primarily Expo-based - Add proper new architecture flags in
android/app/build.gradle:
android {
defaultConfig {
// Add these configurations
newArchEnabled true
hermesEnabled true
}
}🔗 Analysis chain
Verify Expo configuration for new architecture.
The addition of useExpoModules() suggests a transition to using Expo modules. When moving to the new architecture:
- Ensure all native modules are compatible with Expo
- Verify that the app's
android/app/build.gradlehas the necessary new architecture configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Expo and new architecture setup
# Check for new architecture flags in android/app/build.gradle
echo "Checking new architecture configuration..."
rg -A 5 "newArchEnabled" android/app/build.gradle
# Check for Expo configuration
echo "Checking Expo configuration..."
rg "expo" app.json
rg "expo" app.config.js
# Check for potential compatibility issues with native modules
echo "Checking native module imports..."
rg "import.*react-native" --type js --type ts --type jsx --type tsx
Length of output: 511
Script:
#!/bin/bash
# Let's verify with more precise commands
# Check new architecture configuration with broader pattern
echo "Checking Android build configuration..."
cat android/app/build.gradle
# Check Expo configuration in package.json
echo "Checking Expo dependencies..."
cat package.json | grep -A 5 -B 5 "expo"
# Check native module imports with correct syntax
echo "Checking native module imports..."
rg "import.*react-native"
# Check for expo configuration files
echo "Checking for expo config files..."
fd -e json -e js -e ts "app.config"
Length of output: 34569
| case undefined: | ||
| await TrackPlayer.pause(); | ||
| default: | ||
| // await TrackPlayer.pause(); |
There was a problem hiding this comment.
Fix switch case fallthrough and clarify intent handling.
There are several issues in the intent handling:
- The
undefinedcase falls through todefault - The commented-out code in
defaultcreates ambiguity - Similar pause logic in both cases suggests redundancy
Apply this fix to handle the cases properly:
case undefined:
await TrackPlayer.pause();
+ break;
default:
- // await TrackPlayer.pause();
+ console.warn(`Unhandled intent: ${intentData}`);📝 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.
| case undefined: | |
| await TrackPlayer.pause(); | |
| default: | |
| // await TrackPlayer.pause(); | |
| case undefined: | |
| await TrackPlayer.pause(); | |
| break; | |
| default: | |
| console.warn(`Unhandled intent: ${intentData}`); |
🧰 Tools
🪛 Biome
[error] 84-85: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
| @@ -137,7 +146,7 @@ class APMWidget : AppWidgetProvider() { | |||
| val bitmap = binder.service.getCurrentBitmap()?.await() | |||
| val croppedBitmap = cropBitmap(bitmap) | |||
| val dr = RoundedBitmapDrawableFactory.create(context.resources, croppedBitmap) | |||
| dr.cornerRadius = 100f | |||
| dr.cornerRadius = 10f | |||
There was a problem hiding this comment.
Review bitmap handling for memory efficiency.
Several concerns with the current implementation:
- The commented-out
bitmap.recycle()could lead to memory leaks - Fixed scaling to 120x120 might not be optimal for all screen densities
Consider these improvements:
private fun cropBitmap(bitmap: Bitmap?): Bitmap? {
if (bitmap == null) return null
val dimension = bitmap.width.coerceAtMost(bitmap.height)
val cropped = ThumbnailUtils.extractThumbnail(bitmap, dimension, dimension)
- // bitmap.recycle()
+ bitmap.recycle()
- return Bitmap.createScaledBitmap(cropped, 120, 120, false)
+ val scaledSize = context.resources.getDimensionPixelSize(R.dimen.widget_artwork_size)
+ val scaled = Bitmap.createScaledBitmap(cropped, scaledSize, scaledSize, true)
+ cropped.recycle()
+ return scaled
}Also add a dimension resource:
<dimen name="widget_artwork_size">120dp</dimen>Also applies to: 163-165
| + fun isSystemAnimation(stackAnimation: StackAnimation): Boolean { | ||
| + return stackAnimation === StackAnimation.DEFAULT || stackAnimation === StackAnimation.FADE || stackAnimation === StackAnimation.NONE | ||
| + } |
There was a problem hiding this comment.
Use '==' Instead of '===' for Enum Comparison in Kotlin
In Kotlin, it's recommended to use '==' to compare enum values for structural equality. Using '===' checks for referential equality, which may not always be reliable for enums.
Apply this diff to fix the comparison operators:
fun isSystemAnimation(stackAnimation: StackAnimation): Boolean {
- return stackAnimation === StackAnimation.DEFAULT || stackAnimation === StackAnimation.FADE || stackAnimation === StackAnimation.NONE
+ return stackAnimation == StackAnimation.DEFAULT || stackAnimation == StackAnimation.FADE || stackAnimation == StackAnimation.NONE
}📝 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.
| + fun isSystemAnimation(stackAnimation: StackAnimation): Boolean { | |
| + return stackAnimation === StackAnimation.DEFAULT || stackAnimation === StackAnimation.FADE || stackAnimation === StackAnimation.NONE | |
| + } | |
| fun isSystemAnimation(stackAnimation: StackAnimation): Boolean { | |
| return stackAnimation == StackAnimation.DEFAULT || stackAnimation == StackAnimation.FADE || stackAnimation == StackAnimation.NONE | |
| } |
carplay delegate doesnt work with newarch until fixed
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (15)
src/components/playlists/ShuffleAllButton.tsx (2)
13-16: Consider adding error handling for the shuffle operation.The
shuffleAlloperation is awaited but lacks error handling. Consider wrapping it in a try-catch block to handle potential failures gracefully.const onPressed = async () => { - await shuffleAll(); - navigation.navigate({ route: NoxRoutes.PlayerHome }); + try { + await shuffleAll(); + navigation.navigate({ route: NoxRoutes.PlayerHome }); + } catch (error) { + console.error('Failed to shuffle playlist:', error); + // Handle error appropriately + } };
Line range hint
1-24: Consider adding TypeScript type definitions.The component could benefit from explicit TypeScript types for better maintainability and type safety.
+import { NavigationProp } from '@react-navigation/native'; + +interface Props { + navigation: NavigationProp<any>; +} -export default () => { +export default function ShuffleAllButton() { const navigation = useNavigation(); // ... rest of the componentsrc/enums/BottomTab.ts (1)
18-19: Consider maintaining distinct icons for different music-related routes.The current changes consolidate multiple routes (
Playlist,PlayerCover,Lyrics) to use the samemusicicon, which might impact user navigation and intuition. For example, using themusicicon for thePlaylistroute instead of the dedicatedplaylisticon could make it less immediately recognizable.Consider this alternative mapping to maintain better visual distinction:
- [NoxRoutes.Playlist]: BottomTabRouteIcons.music, - [NoxRoutes.PlayerCover]: BottomTabRouteIcons.music, - [NoxRoutes.Lyrics]: BottomTabRouteIcons.music, + [NoxRoutes.Playlist]: BottomTabRouteIcons.playlist, + [NoxRoutes.PlayerCover]: BottomTabRouteIcons.music, + [NoxRoutes.Lyrics]: BottomTabRouteIcons.music,This maintains the music icon for player-related routes while keeping the playlist icon for the playlist route, improving visual differentiation.
Also applies to: 22-22
src/hooks/useNavigation.ts (1)
7-11: Consider improving type safety for navigation options.The
optionsparameter is typed asany, which reduces TypeScript's type checking benefits. Consider using React Navigation's parameter types for better type safety.+import { NavigationState, ParamListBase } from '@react-navigation/native'; + interface NavigateProps { route: NoxRoutes; setIcon?: boolean; - options?: any; + options?: NavigationState['routes'][number]['params']; }src/components/player/PlayerTopInfo.tsx (1)
Line range hint
10-11: Consider consolidating navigation approaches.The component currently uses both the navigation prop and the
useNavigationhook. Consider standardizing on one approach to improve maintainability and reduce potential confusion.-export default ({ navigation }: NoxComponent.NavigationProps) => { - const navigationGlobal = useNavigation(); +export default () => { + const navigation = useNavigation();Then update the drawer call:
- onPress={() => navigation.openDrawer()} + onPress={() => navigation.navigate({ route: 'DrawerOpen' })}src/types/component.d.ts (1)
18-23: Consider documenting the navigation architecture.The codebase now supports both drawer and stack navigation patterns. Consider documenting:
- When to use
NavigationPropsvsStackNavigationProps- The overall navigation hierarchy
- Guidelines for choosing between navigation patterns in new components
src/components/player/controls/Progress.tsx (2)
40-44: Consider improving visual hierarchy of progress indicatorsThe current implementation overlays two progress bars with similar colors (secondary and secondaryContainer), which might make it difficult for users to distinguish between playback progress and fetch progress.
Consider these improvements:
<ProgressBar progress={fetchProgress / 100} - color={playerStyle.colors.secondary} + color={playerStyle.colors.primary} style={[ styles.progressBarDouble, - { backgroundColor: playerStyle.colors.secondaryContainer }, + { + backgroundColor: playerStyle.colors.secondaryContainer, + opacity: 0.5 + }, ]} />This would:
- Use a distinct color (primary) for the fetch progress
- Reduce opacity to better indicate it's a secondary indicator
Line range hint
15-21: Add error handling for edge casesThe component currently doesn't handle potential edge cases in the progress calculations.
Consider adding safety checks:
export const Progress: React.FC<{ live?: boolean }> = ({ live }) => { const { position, duration } = useProgress(); const playerStyle = useNoxSetting(state => state.playerStyle); - const fetchProgress = useStore(appStore, state => state.fetchProgress); + const fetchProgress = useStore(appStore, state => { + const progress = state.fetchProgress; + return Math.min(Math.max(0, progress), 100); + }); + + const safePosition = Math.max(0, position); + const safeDuration = Math.max(0, duration);This ensures:
- Fetch progress is always between 0-100
- Position and duration are non-negative
src/components/bottomtab/NoxBottomTab.tsx (1)
30-30: Consider keeping the more descriptive variable name.The change from
navigationGlobaltonavigationGmakes the code less self-documenting. While shorter names might seem convenient, they can make the code harder to understand for new contributors.- const navigationG = useNavigation(); + const navigationGlobal = useNavigation();src/components/explore/SongTab.tsx (1)
Line range hint
59-65: Navigation change looks good but there are potential improvements needed.The navigation update to use object format aligns with the codebase-wide pattern. However, there are a few concerns:
- The "HACK" comment suggests a workaround that needs proper documentation or fixing.
- Using
setTimeoutfor scroll could lead to race conditions or inconsistent behavior.- The sequence of navigation → playback → scroll might cause a jarring user experience.
Consider refactoring the handler to improve reliability and user experience:
onPress={() => { - navigationGlobal.navigate({ route: NoxRoutes.Playlist }); - playAsSearchList({ - songs: totalSongs ?? songs, - song: item, - // HACK: oh well. - }).then(() => setTimeout(scroll, 500)); + // Start playback first to avoid UI delays + playAsSearchList({ + songs: totalSongs ?? songs, + song: item, + }).then(() => { + // Navigate after playback starts + navigationGlobal.navigate({ route: NoxRoutes.Playlist }); + // Use requestAnimationFrame for smoother scrolling + requestAnimationFrame(scroll); + }); }}Would you like me to help document the current HACK or implement a proper fix? Please let me know if you need assistance understanding the root cause.
src/components/explore/SongRow.tsx (2)
Line range hint
45-50: Consider adding error handling for the playback operation.While the navigation change looks good, the Promise chain lacks error handling which could lead to silent failures.
Consider adding error handling:
navigationGlobal.navigate({ route: NoxRoutes.Playlist }); playAsSearchList({ songs: totalSongs ?? songs, song: item, -}).then(() => setTimeout(scroll, 500)); +}).then(() => setTimeout(scroll, 500)) + .catch((error) => { + console.error('Failed to play song:', error); + // Consider showing a user-friendly error message + });Also, consider explaining the purpose of the 500ms delay in a comment, or better yet, use a more deterministic approach for triggering the scroll.
Line range hint
95-95: Address the TODO comment about component abstraction.The
BiliSongRowandYTSongRowcomponents share significant code. Consider creating a base component to reduce duplication.Here's a suggested approach:
interface BaseSongRowProps { songs: Array<{ cover: string; name: string; singer?: string; }>; title?: string; onItemPress: (item: any) => Promise<void>; } const BaseSongRow: React.FC<BaseSongRowProps> = ({ songs, title, onItemPress }) => { // Common implementation here }; // Then use it like: export const BiliSongRow = (props: BiliSongCardProp) => ( <BaseSongRow {...props} onItemPress={async (item) => { // BiliSongRow specific logic }} /> );src/components/playlist/BiliSearch/BiliSearchbar.tsx (2)
Line range hint
115-121: Consider enhancing shared data comparison.While the early return for duplicate data is good, consider normalizing the shared data before comparison to handle equivalent URLs with different formats (e.g., with/without trailing slashes, different protocols).
const handleShare = useCallback((item?: SharedItem) => { if (!item) { return; } const { mimeType, data } = item; - if (data === sharedData) return; + const normalizeUrl = (url: string) => url.trim().toLowerCase().replace(/\/$/, ''); + if (normalizeUrl(data) === normalizeUrl(sharedData)) return; setSharedData(data); setSharedMimeType(mimeType); handleExternalSearch(data); setSearchVal(data); }, []);
Line range hint
1-1: Improve type safety by removing 'any' types.The code uses several
anytypes which reduce type safety. Consider defining proper interfaces for the search results and shared data.-/* eslint-disable @typescript-eslint/no-explicit-any */ +interface SearchResult { + // Define the structure of your search results + id: string; + title: string; + // ... other properties +} interface SharedItem { mimeType: string; data: string; - extraData: any; + extraData: Record<string, unknown>; } interface Props { - onSearched: (val: any) => void; + onSearched: (val: SearchResult[]) => void; }Also applies to: 24-26, 53-53
src/components/setting/DeveloperSettings.tsx (1)
Line range hint
67-72: Remove commented-out codeThe commented-out
chatGPTSongNamesetting should either be implemented or removed. Commented-out code can lead to confusion and maintenance issues.- /** - chatGPTSongName: { - settingName: 'chatGPTResolveSongName', - settingCategory: 'GeneralSettings', - }, - */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
android/settings.gradle(1 hunks)src/components/bottomtab/NoxBottomTab.tsx(2 hunks)src/components/explore/SongRow.tsx(2 hunks)src/components/explore/SongTab.tsx(1 hunks)src/components/player/PlayerTopInfo.tsx(1 hunks)src/components/player/controls/Progress.tsx(1 hunks)src/components/playlist/BiliSearch/BiliSearchbar.tsx(1 hunks)src/components/playlists/Playlists.tsx(2 hunks)src/components/playlists/ShuffleAllButton.tsx(1 hunks)src/components/playlists/View.tsx(4 hunks)src/components/setting/DeveloperSettings.tsx(2 hunks)src/components/setting/View.tsx(2 hunks)src/components/setting/appearances/View.tsx(2 hunks)src/enums/BottomTab.ts(1 hunks)src/hooks/useNavigation.ts(1 hunks)src/types/component.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- android/settings.gradle
- src/components/playlists/View.tsx
🔇 Additional comments (7)
src/components/playlists/ShuffleAllButton.tsx (1)
15-15: LGTM: Navigation format change aligns with new architecture.
The updated navigation format using object notation is consistent with the architectural changes being implemented across the codebase.
Let's verify the consistency of this navigation pattern across the codebase:
✅ Verification successful
The previous script had an issue with file type recognition. Let's try a more generic search to understand the navigation patterns in the codebase. This will help us verify if the object-style navigation is consistently used across the codebase.
Let's try a different approach by first finding the TypeScript files and then searching within them. This should help us verify the navigation patterns across the codebase.
Based on the search results, I can see that the object-style navigation format navigation.navigate({ route: ... }) is consistently used across the codebase, particularly in:
- src/components/playlists/Playlists.tsx:
navigation.navigate({ route: NoxRoutes.PlayerHome });
navigation.navigate({ route: NoxRoutes.Playlist });
navigation.navigate({ route: NoxRoutes.Settings });- src/components/playlists/ShuffleAllButton.tsx:
navigation.navigate({ route: NoxRoutes.PlayerHome });- src/components/playlists/View.tsx:
navigation.navigate({ route: view, setIcon: routeIcon !== undefined });
navigation.navigate({ route: NoxRoutes.PlayerHome });The navigation format change is consistent with the codebase architecture
The updated object-style navigation format is used consistently across all navigation calls in the codebase, confirming this is part of a standardized architectural pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for navigation.navigate calls to ensure consistent usage of the new format
rg -U "navigation\.navigate\([^{].*?\)" --type ts --type tsx
Length of output: 90
Script:
#!/bin/bash
# Search for navigation.navigate calls with proper file type patterns
rg -U "navigation\.navigate\(" "*.tsx" "*.ts"
# Alternative search to find all navigation patterns
rg -U "navigation\." "*.tsx" "*.ts"
Length of output: 267
Script:
#!/bin/bash
# First, find all TypeScript and TSX files
fd "\.(ts|tsx)$" | while read -r file; do
# Search for navigation.navigate patterns in each file
echo "=== $file ==="
rg -U "navigation\.navigate\(" "$file" || true
done
Length of output: 67854
src/components/player/PlayerTopInfo.tsx (1)
35-37: LGTM: Navigation pattern update is consistent with the new architecture.
The change to use an object-based navigation call aligns with the broader architectural updates across the application.
src/types/component.d.ts (1)
4-4: LGTM! Import statement is properly placed.
The import of NativeStackNavigationProp is correctly positioned alongside other navigation-related imports.
src/components/setting/appearances/View.tsx (1)
19-19: LGTM: Improved type safety with StackNavigationProps
The change from NavigationProps to StackNavigationProps is appropriate as this component is used within a native stack navigator, providing better type safety and access to stack-specific navigation methods.
src/components/bottomtab/NoxBottomTab.tsx (1)
71-73:
Fix navigation method calls to use correct React Navigation format.
The current implementation uses a non-standard format that might cause navigation issues. React Navigation's navigate method expects either a route name string or an object with the name property (not route).
Apply this fix to all navigation calls:
- onPress={() =>
- navigationG.navigate({ route: NoxRoutes.PlayerHome })
- }
+ onPress={() => navigationG.navigate(NoxRoutes.PlayerHome)}
- onPress={() => navigationG.navigate({ route: NoxRoutes.Explore })}
+ onPress={() => navigationG.navigate(NoxRoutes.Explore)}
- onPress={() => navigationG.navigate({ route: NoxRoutes.Settings })}
+ onPress={() => navigationG.navigate(NoxRoutes.Settings)}Let's verify if this non-standard navigation format is used elsewhere in the codebase:
Also applies to: 77-77, 81-81
src/components/explore/SongRow.tsx (1)
Line range hint 130-136: Improve async operation flow and error handling.
The current implementation has several potential issues:
- Navigation happens before async operations complete
- No loading state during playlist fetch
- Missing error handling for both async operations
Consider this improved implementation:
- onPress={async () => {
- navigationGlobal.navigate({ route: NoxRoutes.Playlist });
- const playlist = await item.getPlaylist();
- playAsSearchList({
- songs: playlist.songs,
- song: playlist.item,
- }).then(() => setTimeout(scroll, 500));
+ onPress={async () => {
+ try {
+ // Consider adding a loading indicator here
+ const playlist = await item.getPlaylist();
+ navigationGlobal.navigate({ route: NoxRoutes.Playlist });
+ await playAsSearchList({
+ songs: playlist.songs,
+ song: playlist.item,
+ });
+ setTimeout(scroll, 500);
+ } catch (error) {
+ console.error('Failed to load or play playlist:', error);
+ // Consider showing a user-friendly error message
+ }
+ }}This revision:
- Fetches playlist before navigation to ensure data availability
- Uses proper error handling
- Maintains consistent async/await pattern
Let's verify the usage of loading indicators in similar components:
src/components/setting/DeveloperSettings.tsx (1)
96-96: LGTM: Props type improvement
The change from NavigationProps to StackNavigationProps better reflects the actual navigation context and provides improved type safety for stack-specific navigation methods.
src/hooks/useNavigation.ts
Outdated
| const navigate = ({ route, setIcon, options }: NavigateProps) => { | ||
| // @ts-expect-error nav v7 hasnt fixed this type yet | ||
| navigationGlobal.navigate(route as never, options); | ||
| if (setIcon) { | ||
| setRoute(IconMap[route]); | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Navigation calls need to be updated to use consistent object syntax
Found inconsistent usage of the navigation API across the codebase:
- Most calls use the new object syntax:
navigate({ route: NoxRoutes.X }) - However, some calls in
LandscapeActions.tsxstill use the old direct style with type assertions:navigationGlobal.navigate(NoxRoutes.Lyrics as never) navigationGlobal.navigate(NoxRoutes.Explore as never) navigationGlobal.navigate(NoxRoutes.Settings as never)
These calls should be updated to use the new object-based signature for consistency:
navigationGlobal.navigate({ route: NoxRoutes.Lyrics })🔗 Analysis chain
LGTM! Verify navigation calls across the codebase.
The refactored navigation implementation looks good. The ts-expect-error is appropriately documented and necessary for React Navigation v7 compatibility.
Let's verify that all navigation calls have been updated to use the new object-based signature:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old-style navigation calls
# that haven't been updated to use the new object syntax
# Search for direct navigation calls that might need updating
rg -l "navigate\([^{]" --type ts --type tsx
# Search for potential navigation calls using the old format
ast-grep --pattern 'navigate($route, $setIcon)'
Length of output: 123
Script:
#!/bin/bash
# Let's try a different approach to find navigation calls
# Search for navigate calls without object syntax
rg "navigate\(" -A 2 --type typescript
# Search for potential direct navigation references
rg "navigationGlobal\.navigate\(" -A 2 --type typescript
# Look for any usage of navigate function
ast-grep --pattern 'navigate($$$)'
Length of output: 206
Script:
#!/bin/bash
# Let's try with correct file extensions and simpler patterns
# Search for navigate calls in ts files
rg "navigate\(" -A 2 --type ts
# Search for navigation usage in ts files
rg "navigationGlobal" -A 2 --type ts
# Find files that might contain navigation logic
fd -e ts -e tsx | xargs rg "navigate"
Length of output: 11981
| interface StackNavigationProps { | ||
| navigation: NativeStackNavigationProp<ParamListBase>; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the content of component.d.ts, I can now generate the final response:
Navigation props interfaces are well-structured but need consolidation
The codebase currently has three navigation-related interfaces in the NoxComponent namespace:
NavigationProps(using DrawerNavigationProp)NavigationProps2(using optional DrawerNavigationProp)StackNavigationProps(using NativeStackNavigationProp)
The new StackNavigationProps interface is well-defined, but having multiple navigation prop interfaces with similar purposes suggests a need to consolidate the navigation pattern. Consider:
- Merging
NavigationPropsandNavigationProps2into a single interface - Documenting when to use Stack vs Drawer navigation props
- Removing the numbered suffix from interface names (NavigationProps2)
🔗 Analysis chain
LGTM! New navigation props interface is well-structured.
The interface follows TypeScript best practices and maintains consistency with existing patterns.
Let's verify the navigation pattern usage across the codebase:
Let me try a different approach to verify the navigation patterns usage.
Let me try one more time with the correct file type flags.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of different navigation props to ensure consistent patterns
# Expected: Files should use either StackNavigationProps or NavigationProps consistently within their context
# Search for usage of both navigation types
echo "Checking navigation prop usage patterns..."
rg -l "NavigationProps|StackNavigationProps" --type ts --type tsx
# Check specific usage contexts
echo -e "\nChecking specific component usage..."
ast-grep --pattern 'interface $_ extends NoxComponent.NavigationProps'
ast-grep --pattern 'interface $_ extends NoxComponent.StackNavigationProps'
Length of output: 430
Script:
#!/bin/bash
# Search for NavigationProps usage with ripgrep, showing context
echo "Checking NavigationProps usage..."
rg "NavigationProps" -A 2 -B 2 --type typescript
# Search for specific navigation type imports and usage
echo -e "\nChecking navigation type imports..."
rg "@react-navigation/(native-stack|drawer)" --type typescript
# Look for component props definitions
echo -e "\nChecking component props definitions..."
rg "Props(\s*=|\s*extends|\s*interface)" -A 3 -B 1 --type typescript
Length of output: 548
Script:
#!/bin/bash
# Search for NavigationProps usage with ripgrep, showing context
echo "Checking NavigationProps usage..."
rg "NavigationProps" -A 2 -B 2 -t ts -t tsx
# Search for specific navigation type imports and usage
echo -e "\nChecking navigation type imports..."
rg "@react-navigation/(native-stack|drawer)" -t ts -t tsx
# Look for component props definitions
echo -e "\nChecking component props definitions..."
rg "Props(\s*=|\s*extends|\s*interface)" -A 3 -B 1 -t ts -t tsx
# Check the specific file where StackNavigationProps is defined
echo -e "\nChecking the types file content..."
cat src/types/component.d.ts
Length of output: 2470
| @@ -30,7 +30,7 @@ const MainView = ({ navigation }: NoxComponent.NavigationProps) => { | |||
| <SettingListItem | |||
| icon={'palette-swatch-variant'} | |||
| settingName="SkinSetting" | |||
| onPress={() => navigation.navigate(VIEW.SKIN)} | |||
| onPress={() => navigation.popTo(VIEW.SKIN)} | |||
There was a problem hiding this comment.
💡 Codebase verification
Custom navigation method popTo needs to be replaced with standard navigation
The codebase search reveals that:
popTois used extensively in settings components but has no implementation- The app uses
@react-navigation/native-stackfor navigation - No custom navigation class or extension implementing
popTowas found
Replace with standard React Navigation methods:
- onPress={() => navigation.popTo(VIEW.SKIN)}
+ onPress={() => navigation.navigate(VIEW.SKIN)}This change should be applied to all similar usages in:
- src/components/setting/View.tsx
- src/components/setting/DeveloperSettings.tsx
- src/components/setting/appearances/View.tsx
🔗 Analysis chain
Verify the custom navigation method
The popTo method is not a standard React Navigation method. This could lead to runtime errors if the method is not properly implemented.
Let's verify the implementation of this custom navigation method:
Consider using standard React Navigation methods if popTo is not well-defined:
-onPress={() => navigation.popTo(VIEW.SKIN)}
+onPress={() => navigation.navigate(VIEW.SKIN)}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation or type definition of popTo
echo "Searching for popTo implementation..."
rg -t typescript -t javascript "popTo.*=.*{" --no-heading
echo "Searching for popTo type definition..."
rg -t typescript "interface.*Navigation.*{" -A 10 --no-heading
echo "Searching for popTo usage across the codebase..."
rg -t typescript -t javascript "popTo\(" --no-heading
Length of output: 692
Script:
#!/bin/bash
# Search for popTo implementation with fixed regex
echo "Searching for popTo implementation..."
rg "popTo\s*=" -t ts -t tsx -t js -t jsx
echo "Searching for Navigation interface definitions..."
rg "interface.*Navigation" -t ts -t tsx -A 10
echo "Searching for popTo usage..."
rg "popTo" -t ts -t tsx -t js -t jsx
Length of output: 472
Script:
#!/bin/bash
# Search for popTo implementation and usage without file type restrictions
echo "Searching for popTo implementation..."
rg "popTo\s*="
echo "Searching for Navigation interface or type definitions..."
rg "(?:interface|type).*Navigation.*\{" -A 10
echo "Searching for popTo usage..."
rg "\bpopTo\b" -A 2
Length of output: 4947
Script:
#!/bin/bash
# Let's look for the navigation setup and any custom navigation implementations
echo "Searching for navigation setup..."
rg "createNativeStackNavigator|createStackNavigator" -A 5
echo "Searching for custom navigation implementations..."
fd -e ts -e tsx -e js -e jsx | xargs rg "class.*Navigation|extends.*Navigation"
echo "Checking the navigation type definitions..."
fd navigation -i | xargs cat
Length of output: 6063
| @@ -79,7 +79,7 @@ export default ({ | |||
| }; | |||
|
|
|||
| const handleExternalSearch = (data: string) => { | |||
| navigationGlobal.navigate(NoxRoutes.Playlist); | |||
| navigationGlobal.navigate({ route: NoxRoutes.Playlist }); | |||
There was a problem hiding this comment.
💡 Codebase verification
Navigation pattern inconsistency detected across components
The codebase shows mixed usage of navigation patterns:
-
Object-based pattern
{ route: NoxRoutes.Playlist }is used in:- src/components/playlist/BiliSearch/BiliSearchbar.tsx
- src/components/player/PlayerTopInfo.tsx
- src/components/explore/SongTab.tsx
- src/components/explore/SongRow.tsx
-
Direct route pattern
NoxRoutes.X as neveris used in:- src/components/landscape/LandscapeActions.tsx (multiple instances)
This inconsistency should be addressed by updating all navigation calls to use the object-based pattern for better type safety and maintainability.
🔗 Analysis chain
Verify consistent navigation pattern usage across the codebase.
The change to object-based navigation ({ route: NoxRoutes.Playlist }) aligns with modern navigation patterns, but we should ensure consistency across all navigation calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct route navigation patterns that might need updating
rg -t typescript "navigationGlobal\.navigate\([^{].*\)" --glob "src/**/*.tsx"
# Search for the new object-based navigation pattern for comparison
rg -t typescript "navigationGlobal\.navigate\(\{.*route:" --glob "src/**/*.tsx"
Length of output: 231
Script:
#!/bin/bash
# Search for direct route navigation patterns that might need updating
rg "navigationGlobal\.navigate\([^{].*\)" --glob "src/**/*.tsx"
# Search for the new object-based navigation pattern for comparison
rg "navigationGlobal\.navigate\(\{.*route:" --glob "src/**/*.tsx"
# Also search for any other navigation patterns to be thorough
rg "navigationGlobal\.navigate" --glob "src/**/*.tsx" -A 1
Length of output: 3082
src/components/setting/View.tsx
Outdated
| {isAndroid && isIOS && ( | ||
| <SettingListItem | ||
| icon={NoxView.DOWNLOAD} | ||
| settingName="DownloadOptions" | ||
| onPress={() => navigation.navigate(NoxView.DOWNLOAD)} | ||
| onPress={() => navigation.popTo(NoxView.DOWNLOAD)} | ||
| settingCategory="Settings" | ||
| /> |
There was a problem hiding this comment.
Fix platform condition for Download Settings.
The current condition isAndroid && isIOS will always evaluate to false since a device cannot be both Android and iOS simultaneously. This makes the Download Settings completely inaccessible.
Consider using this instead:
- {isAndroid && isIOS && (
+ {(isAndroid || isIOS) && (
<SettingListItem
icon={NoxView.DOWNLOAD}
settingName="DownloadOptions"
onPress={() => navigation.popTo(NoxView.DOWNLOAD)}
settingCategory="Settings"
/>
)}Or if you want to restrict it to a specific platform:
- {isAndroid && isIOS && (
+ {isAndroid && (📝 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.
| {isAndroid && isIOS && ( | |
| <SettingListItem | |
| icon={NoxView.DOWNLOAD} | |
| settingName="DownloadOptions" | |
| onPress={() => navigation.navigate(NoxView.DOWNLOAD)} | |
| onPress={() => navigation.popTo(NoxView.DOWNLOAD)} | |
| settingCategory="Settings" | |
| /> | |
| {(isAndroid || isIOS) && ( | |
| <SettingListItem | |
| icon={NoxView.DOWNLOAD} | |
| settingName="DownloadOptions" | |
| onPress={() => navigation.popTo(NoxView.DOWNLOAD)} | |
| settingCategory="Settings" | |
| /> |
| interface Props extends NoxComponent.StackNavigationProps { | ||
| headerBackVisible?: boolean; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Props interface type inconsistency detected across components
The codebase shows inconsistent navigation prop types:
src/AzusaPlayer.tsxstill usesNoxComponent.NavigationPropssrc/components/setting/View.tsxusesNoxComponent.StackNavigationProps
This inconsistency could lead to type-safety issues. The migration from NavigationProps to StackNavigationProps should be consistent across all components using stack navigation.
🔗 Analysis chain
LGTM! Props interface correctly updated to use StackNavigationProps.
The change to use StackNavigationProps better aligns with the native stack navigator implementation.
Let's verify the consistency of navigation prop types across related files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using NavigationProps to ensure consistent typing
rg "extends.*NavigationProps" --type tsx
Length of output: 229
Script:
#!/bin/bash
# Search for files with NavigationProps using ripgrep without file type filter
rg "extends.*NavigationProps"
# Also search for StackNavigationProps to understand the migration pattern
rg "extends.*StackNavigationProps"
# Search for .tsx files to verify ripgrep can find them
find . -name "*.tsx"
Length of output: 6023
src/components/setting/View.tsx
Outdated
| onPress={() => navigation.popTo(NoxView.GENERAL)} | ||
| settingCategory="Settings" | ||
| /> | ||
| <SettingListItem | ||
| icon={NoxView.SKIN} | ||
| settingName="AppearanceSetting" | ||
| onPress={() => navigation.navigate(NoxView.SKIN)} | ||
| onPress={() => navigation.popTo(NoxView.SKIN)} | ||
| settingCategory="Settings" | ||
| /> | ||
| <SettingListItem | ||
| icon={NoxView.LOGIN} | ||
| settingName="Login" | ||
| onPress={() => navigation.navigate(NoxView.LOGIN)} | ||
| onPress={() => navigation.popTo(NoxView.LOGIN)} | ||
| settingCategory="Settings" | ||
| /> | ||
| <SettingListItem | ||
| icon={NoxView.BACKUP} | ||
| settingName="BackupSetting" | ||
| onPress={() => navigation.navigate(NoxView.BACKUP)} | ||
| onPress={() => navigation.popTo(NoxView.BACKUP)} | ||
| settingCategory="Settings" | ||
| /> | ||
| <LanguageSettings icon={NoxView.LANGUAGE} /> | ||
| <SettingListItem | ||
| icon={NoxView.ALIST} | ||
| settingName="AListOptions" | ||
| onPress={() => navigation.navigate(NoxView.ALIST)} | ||
| onPress={() => navigation.popTo(NoxView.ALIST)} | ||
| settingCategory="Settings" | ||
| /> | ||
| {isAndroid && isIOS && ( | ||
| <SettingListItem | ||
| icon={NoxView.DOWNLOAD} | ||
| settingName="DownloadOptions" | ||
| onPress={() => navigation.navigate(NoxView.DOWNLOAD)} | ||
| onPress={() => navigation.popTo(NoxView.DOWNLOAD)} | ||
| settingCategory="Settings" | ||
| /> | ||
| )} | ||
| <SettingListItem | ||
| icon={NoxView.DEVELOPER} | ||
| settingName="DeveloperOptions" | ||
| onPress={() => navigation.navigate(NoxView.DEVELOPER)} | ||
| onPress={() => navigation.popTo(NoxView.DEVELOPER)} | ||
| settingCategory="Settings" | ||
| /> | ||
| <SettingListItem | ||
| icon={NoxView.SPLASH_GALLARY} | ||
| settingName="SplashSetting" | ||
| onPress={() => navigation.navigate(NoxView.SPLASH_GALLARY)} | ||
| onPress={() => navigation.popTo(NoxView.SPLASH_GALLARY)} | ||
| settingCategory="Settings" | ||
| /> | ||
| <SettingListItem | ||
| icon={NoxView.INFO} | ||
| settingName="InfoSetting" | ||
| onPress={() => navigation.navigate(NoxView.INFO)} | ||
| onPress={() => navigation.popTo(NoxView.INFO)} |
There was a problem hiding this comment.
💡 Codebase verification
Navigation method popTo appears to be undefined
The codebase search reveals that while popTo is being used in the settings components, there is no implementation of this method. The navigation hook (useNavigation.ts) only implements a navigate method, and the app uses React Navigation which doesn't have a built-in popTo method. This will likely cause runtime errors.
Key findings:
- The
useNavigationhook only implements anavigatemethod - The app uses standard React Navigation methods (
navigate,openDrawer,closeDrawer) - No custom
popToimplementation exists in the codebase
Recommendation: Revert back to using navigation.navigate() or implement the popTo method in the navigation hook if a specific navigation behavior is needed.
🔗 Analysis chain
Verify the navigation stack behavior with popTo method.
The change from navigation.navigate() to navigation.popTo() across all settings navigation could significantly alter the navigation stack behavior. This might affect how users navigate back through the settings screens.
Let's verify the implementation and usage of the popTo method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the popTo method implementation
rg "popTo.*=.*" --type tsx -A 5
# Search for other usages of popTo to ensure consistent behavior
rg "navigation\.popTo\(" --type tsx
Length of output: 229
Script:
#!/bin/bash
# Search for the popTo method implementation and usage with ripgrep
rg "popTo" -t ts -t tsx
# Search for navigation-related types and interfaces
rg "type.*Navigation|interface.*Navigation" -t ts -t tsx
# Look for navigation-related files
fd "navigation" -e ts -e tsx
Length of output: 194
Script:
#!/bin/bash
# Let's examine the navigation hook implementation
cat src/hooks/useNavigation.ts
# Search for popTo without file type restrictions since previous attempts failed
rg "popTo"
# Search for navigation-related code without file type restrictions
rg "navigation" -g "*.ts*" -A 3
Length of output: 25837
| @@ -130,7 +131,7 @@ export default () => { | |||
| {false && ( | |||
| <IconButton | |||
| icon={'cog'} | |||
| onPress={() => navigation.navigate(NoxRoutes.Settings)} | |||
| onPress={() => navigation.navigate({ route: NoxRoutes.Settings })} | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove the unreachable settings navigation code
The settings navigation is actively used across multiple components in the codebase with the same navigation pattern:
src/components/landscape/LandscapeActions.tsxsrc/components/bottomtab/NoxBottomTab.tsx- Other components reference it in their configurations
Since this specific button in Playlists.tsx is permanently disabled with false && and there are other accessible ways to reach the settings screen, the unreachable code should be removed to maintain codebase cleanliness.
🔗 Analysis chain
Remove or enable the settings navigation button.
While the navigation syntax update is good, this code is unreachable due to the false && condition. Either:
- Remove the unused code, or
- Enable the settings button if it's needed
Let's verify if this navigation pattern is used consistently:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check navigation patterns across the codebase
rg "navigation\.navigate\({ route: NoxRoutes\.Settings }\)" --type tsx
Length of output: 310
Script:
#!/bin/bash
# Check navigation patterns across the codebase with proper escaping
rg "navigation\.navigate\(\{.*?route:\s*NoxRoutes\.Settings.*?\}\)" --type tsx
# Also check for other navigation patterns to Settings
rg "navigation\.navigate.*Settings" --type tsx
# Let's also check the NoxRoutes enum definition
rg "enum.*NoxRoutes" -A 10
Length of output: 662
Script:
#!/bin/bash
# Search for navigation patterns without file type restriction
rg "navigation\.navigate.*Settings"
# Check the specific component for conditional rendering
rg -B 5 -A 5 "navigation\.navigate.*Settings" src/components/playlists/Playlists.tsx
# Look for other Settings navigation patterns
rg "NoxRoutes\.Settings"
Length of output: 1260
| navigation.navigate({ route: NoxRoutes.PlayerHome }); | ||
| navigation.navigate({ route: NoxRoutes.Playlist }); |
There was a problem hiding this comment.
💡 Codebase verification
Sequential navigation pattern needs to be consolidated
The search results confirm that this double navigation pattern (PlayerHome → Playlist) only exists in Playlists.tsx, while other components (View.tsx and ShuffleAllButton.tsx) use single navigation calls to PlayerHome. This inconsistency suggests the sequential navigation is unnecessary.
src/components/playlists/Playlists.tsx: Consolidate the two navigation calls into a single navigation toPlaylist- Other components (
View.tsx,ShuffleAllButton.tsx) demonstrate that direct navigation to a single route is the established pattern
🔗 Analysis chain
Review the navigation sequence implementation.
The double navigation sequence (PlayerHome → Playlist) might cause UI flicker and seems unnecessary. Consider:
- Why is navigation to
PlayerHomerequired beforePlaylist? - Could this be simplified to a single navigation call?
Let's check if this pattern is used consistently:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar navigation patterns in other components
rg -A 2 "navigation\.navigate.*PlayerHome.*Playlist" --type tsx
Length of output: 310
Script:
#!/bin/bash
# Search for navigation patterns with better regex
rg "navigation\.navigate.*route:.*PlayerHome" --type tsx -A 3
# Also search for Playlist navigation patterns
rg "navigation\.navigate.*route:.*Playlist" --type tsx -A 3
# Search for any sequential navigation patterns
rg "navigation\.navigate.*\n.*navigation\.navigate" --type tsx -A 3
Length of output: 443
Script:
#!/bin/bash
# Search for navigation patterns without file type restriction and with multiline
rg "navigation\.navigate.*route.*PlayerHome" -A 3
# Search for Playlist navigation patterns
rg "navigation\.navigate.*route.*Playlist" -A 3
# Search for sequential navigation with multiline
rg -U "navigation\.navigate.*\n.*navigation\.navigate" -A 3
Length of output: 1641
| @@ -223,7 +223,7 @@ const Home = ({ navigation }: NoxComponent.NavigationProps) => { | |||
| <SettingListItem | |||
| icon={Icons.plugins} | |||
| settingName="PluginsSetting" | |||
| onPress={() => navigation.navigate(VIEW.PLUGINS)} | |||
| onPress={() => navigation.popTo(VIEW.PLUGINS)} | |||
There was a problem hiding this comment.
💡 Codebase verification
Navigation method popTo needs to be replaced with standard navigation
The codebase analysis reveals that:
popTois used consistently across multiple components but is not defined in the custom navigation hook (useNavigation.ts)- There's no custom implementation of
popTomethod found in the codebase - The app uses React Navigation with standard navigation types (
DrawerNavigationProp,NativeStackNavigationProp)
Replace with standard React Navigation methods:
- In
src/components/setting/DeveloperSettings.tsx:
-onPress={() => navigation.popTo(VIEW.PLUGINS)}
+onPress={() => navigation.navigate(VIEW.PLUGINS)}🔗 Analysis chain
Verify navigation method compatibility
The popTo method is not a standard method in React Navigation's stack navigator. The common methods are navigate, push, pop, popToTop, etc.
Consider using standard navigation methods instead:
-onPress={() => navigation.popTo(VIEW.PLUGINS)}
+onPress={() => navigation.navigate(VIEW.PLUGINS)}The previous results show that popTo is consistently used across multiple components, suggesting it might be a custom navigation method. Let's verify if there's a custom navigation implementation or type definition that includes this method.
I notice there's a custom navigation hook at src/hooks/useNavigation.ts. Let's examine its implementation to understand if popTo is a custom navigation method defined there.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for popTo usage in navigation
rg "navigation\.popTo" -A 2 -B 2
# Search for potential navigation utility that might define popTo
rg "const.*popTo|function.*popTo" -A 5
Length of output: 3970
Script:
#!/bin/bash
# Search for navigation type definitions or custom navigation implementations
rg "type.*Navigation|interface.*Navigation" -A 5
# Search for potential navigation utility or wrapper
fd -e ts -e tsx -x rg "navigation.*=|createNavigation" -l
# Look for navigation setup or configuration
ast-grep --pattern 'createNavigationContainerRef()'
Length of output: 2237
Script:
#!/bin/bash
# Check the custom navigation hook implementation
cat src/hooks/useNavigation.ts
# Look for navigation utility implementations
rg "popTo.*=" -A 5 -B 5
# Search for navigation container setup
rg "navigationRef" -A 5 -B 5
Length of output: 776
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
patches/@react-navigation+drawer+7.0.1.patch (1)
8-10: Caution: Direct modification of node_modulesWhile the parameter addition is logically correct, directly modifying third-party packages in node_modules is discouraged as:
- Changes will be lost when dependencies are updated
- It makes the codebase harder to maintain
- It could cause conflicts with future package updates
Consider these alternatives:
- Fork the package and maintain your own version
- Create a wrapper component that extends the drawer functionality
- Submit a PR to the original package if this is a generally useful feature
src/components/landscape/LandscapeActions.tsx (1)
24-26: Consider strengthening type safety in navigation state handling.The current implementation uses optional chaining for navigation state access, which is defensive but might mask underlying type issues:
navigation.getState()?.routes?.at(-1)?.nameConsider adding proper type guards:
const state = navigation.getState(); const currentRoute = state.routes[state.routes.length - 1]; const targetRoute = currentRoute?.name === NoxRoutes.Playlist ? NoxRoutes.PlaylistsDrawer : NoxRoutes.Playlist; navigation.navigate({ route: targetRoute });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.json(4 hunks)patches/@react-navigation+drawer+7.0.1.patch(1 hunks)src/components/explore/SongRow.tsx(2 hunks)src/components/explore/SongTab.tsx(1 hunks)src/components/landscape/LandscapeActions.tsx(4 hunks)src/components/player/PlayerTopInfo.tsx(1 hunks)src/components/playlist/BiliSearch/BiliSearchbar.tsx(1 hunks)src/components/playlists/Playlists.tsx(2 hunks)src/components/setting/DeveloperSettings.tsx(1 hunks)src/components/setting/View.tsx(1 hunks)src/components/setting/appearances/View.tsx(1 hunks)src/hooks/useNavigation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- package.json
- src/components/explore/SongRow.tsx
- src/components/explore/SongTab.tsx
- src/components/player/PlayerTopInfo.tsx
- src/components/playlist/BiliSearch/BiliSearchbar.tsx
- src/components/playlists/Playlists.tsx
- src/components/setting/DeveloperSettings.tsx
- src/components/setting/View.tsx
- src/components/setting/appearances/View.tsx
- src/hooks/useNavigation.ts
🔇 Additional comments (2)
src/components/landscape/LandscapeActions.tsx (2)
18-18: LGTM! Clean variable naming.
The change to navigation is more concise and follows React hooks naming conventions.
7-7: Verify custom navigation hook implementation.
The switch to a custom navigation hook could introduce unexpected behavior if not properly implemented.
Let's verify the custom hook implementation:
✅ Verification successful
Custom navigation hook implementation is correct and safe to use
The custom hook is properly implemented as a wrapper around React Navigation's native useNavigation. It:
- Correctly imports and uses the official
useNavigationfrom '@react-navigation/native' - Adds app-specific functionality (icon management and route setting)
- Maintains core navigation capabilities through
navigateandgetState
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of the custom navigation hook
# Expected: The hook should properly wrap React Navigation's useNavigation
# Look for the custom hook implementation
ast-grep --pattern 'export default function useNavigation() {
$$$
}'
# Check for any other usages of this custom hook
rg -l "import.*useNavigation.*from.*@hooks/useNavigation"
Length of output: 502
Script:
#!/bin/bash
# Let's check the actual implementation of the custom hook by searching for the file
fd "useNavigation.ts" --type f
# Also search for any file containing useNavigation export
rg "export.*function.*useNavigation" -A 5
# Additionally, let's check if there are any TypeScript type definitions
fd "navigation.d.ts" --type f
Length of output: 131
Script:
#!/bin/bash
# Let's examine the content of the custom hook implementation
cat src/hooks/useNavigation.ts
# Also check for any usage of @react-navigation/native in the project
rg -l "from '@react-navigation/native'"
# And check the specific usage of useNavigation from react-navigation
rg "useNavigation.*from '@react-navigation/native'"
Length of output: 1261
| drawerHideStatusBarOnOpen, | ||
| drawerPosition = direction === 'rtl' ? 'right' : 'left', | ||
| drawerStatusBarAnimation, | ||
| - drawerStyle, | ||
| drawerType = _reactNative.Platform.select({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using composition instead of modification
Moving drawerStyle from an internal declaration to a parameter is a good change, but modifying the package directly is risky. Instead, consider creating a custom drawer component that wraps the original one:
// CustomDrawer.tsx
import { DrawerView } from '@react-navigation/drawer';
export const CustomDrawer = (props) => {
const { customDrawerStyle, ...rest } = props;
return (
<DrawerView
{...rest}
drawerStyle={customDrawerStyle}
/>
);
};This approach provides the same functionality while maintaining better separation of concerns and package integrity.
| navigation.navigate({ | ||
| route: | ||
| navigation.getState()?.routes?.at(-1)?.name === NoxRoutes.Playlist | ||
| ? NoxRoutes.PlaylistsDrawer | ||
| : NoxRoutes.Playlist, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize navigation method usage and improve type safety.
There are inconsistencies in how navigation is handled:
- Some calls use the new object format:
{ route: NoxRoutes.X } - Others use direct route names with type assertions:
NoxRoutes.X as never
Standardize all navigation calls to use the object format:
-onPress={() => navigation.navigate(NoxRoutes.Lyrics as never)}
+onPress={() => navigation.navigate({ route: NoxRoutes.Lyrics })}
-onPress={() => navigation.navigate(NoxRoutes.Explore as never)}
+onPress={() => navigation.navigate({ route: NoxRoutes.Explore })}
-onPress={() => navigation.navigate(NoxRoutes.Settings as never)}
+onPress={() => navigation.navigate({ route: NoxRoutes.Settings })}Also applies to: 34-34, 67-67, 77-77, 82-82
|
disabling newarch as drawer nested screen UX is very trash rn. |
|
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores