chore: Revise design of "bottom actions"#513
Merged
Merged
Conversation
- Based on how we layout the `layout={LinearTransition}`, the exiting animations might look a bit janky. This requires different positionings based on the desired layout.
- We have 3 cases: 1. Navbar w/ no miniplayer. 2. Navbar w/ miniplayer. 3. No navbar w/ miniplayer.
- On app launch, swipe gestures work fine on miniplayer. Once we navigate to the settings and go back, they randomly break.
📝 WalkthroughWalkthroughThe PR refactors the BottomActions navigation system: ChangesBottomActions Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mobile/src/navigation/layouts/NScrollLayout.tsx (1)
75-78: ⚡ Quick winExtract duplicated bottom-offset args into a shared helper.
The same offset expression appears twice; centralizing it avoids future drift between
NScrollLayoutandNScrollListLayout.Also applies to: 164-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mobile/src/navigation/layouts/NScrollLayout.tsx` around lines 75 - 78, The bottom-offset calculation in useBottomActionsOffset call is duplicated across two files. Extract the offset expression (16 + (showNavbar ? BottomActionsOffset : 0) and the second argument showNavbar ? -8 : 0) into a shared helper function that both NScrollLayout (lines 75-78) and NScrollListLayout (lines 164-167) can use. Create a utility function that takes showNavbar as a parameter and returns both offset values to eliminate the duplication and prevent future inconsistencies between the two layouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mobile/src/navigation/components/BottomActions/index.tsx`:
- Around line 60-71: The SearchButton and SettingsButton components in the
Animated.View are currently gated behind the rendered.navBar condition, which
causes them to disappear on non-home routes. This violates the no-navbar layout
contract where side buttons should remain visible independently of the
home-route flag. Remove the rendered.navBar checks from both the SearchButton
(line 64) and SettingsButton (line 70) conditionals, either by always rendering
these components or by replacing the navBar condition with an appropriate
condition specific to the no-navbar mode layout contract that is independent of
the onHomeScreen flag.
In `@mobile/src/navigation/components/BottomActions/NavActions.tsx`:
- Line 69: The update badge in the NavActions component uses the hardcoded
`right-3` class for positioning, which breaks the layout in RTL (right-to-left)
locales because the badge will still appear on the right side instead of
mirroring to the left. Replace the `right-3` class with `end-3` in the View
component's className, as the `end` utility in Tailwind CSS is RTL-aware and
automatically adjusts positioning based on text direction, ensuring the badge
appears in the correct corner for both LTR and RTL locales.
---
Nitpick comments:
In `@mobile/src/navigation/layouts/NScrollLayout.tsx`:
- Around line 75-78: The bottom-offset calculation in useBottomActionsOffset
call is duplicated across two files. Extract the offset expression (16 +
(showNavbar ? BottomActionsOffset : 0) and the second argument showNavbar ? -8 :
0) into a shared helper function that both NScrollLayout (lines 75-78) and
NScrollListLayout (lines 164-167) can use. Create a utility function that takes
showNavbar as a parameter and returns both offset values to eliminate the
duplication and prevent future inconsistencies between the two layouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15538a9f-72b0-4a71-868e-c98785d9dd8f
⛔ Files ignored due to path filters (1)
.github/assets/Promo - UI.pngis excluded by!**/*.png
📒 Files selected for processing (10)
mobile/src/navigation/components/BottomActions/MiniPlayer.tsxmobile/src/navigation/components/BottomActions/NavActions.tsxmobile/src/navigation/components/BottomActions/index.tsxmobile/src/navigation/components/BottomActions/useBottomActions.tsmobile/src/navigation/layouts/NScrollLayout.tsxmobile/src/navigation/screens/RecentlyPlayedView.tsxmobile/src/navigation/screens/albums/CurrentView.tsxmobile/src/navigation/screens/artists/CurrentView.tsxmobile/src/navigation/screens/genres/CurrentView.tsxmobile/src/navigation/screens/playlists/CurrentView.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mobile/src/navigation/layouts/NScrollLayout.tsx`:
- Around line 74-77: The miniplayVisible variable is using
useBottomActionsOffset() !== 0 as a visibility check, but this hook reflects
active-track offset logic rather than whether the mini-player is actually
rendered on the current route. This causes incorrect padding calculations even
when the mini-player is not displayed. Replace the miniplayVisible assignment at
line 76 (in the NScrollLayout.tsx file around the variable initialization) and
at line 164 (also in NScrollLayout.tsx) with a route-aware check that properly
determines if the mini-player is rendered, rather than relying on the bottom
offset hook value. Use a more accurate visibility flag that accounts for the
actual route-level rendering of the mini-player component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de08e632-c8d3-4763-b9c5-2a96a4723c73
📒 Files selected for processing (2)
mobile/src/navigation/components/BottomActions/useBottomActions.tsmobile/src/navigation/layouts/NScrollLayout.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Hopefully this will be the last time we change this in this major version of the app. This design basically reverts back to a design similar to what we had in v2, but offers a different design for when we want to hide the navbar.
Note
While working on this, I found out that the swipe gestures on the miniplayer randomly breaks only on dev mode when we return to the home screen after visiting the settings.
Checklist
pnpm sync:licenses.pnpm android:prod.Summary by CodeRabbit
New Features
Style