Fix mobile navigation tabs expanding/collapsing when scrolling in modals #3731
Conversation
actual-github-bot
bot
changed the title
joel-jeremy
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
WalkthroughThis pull request introduces structural changes across multiple components: Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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: 1
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/App.tsx (2)
176-184: Consider documenting the scrollbar visibility handlingThe implementation correctly handles scrollbar visibility changes, but the approach of using a key for re-rendering on scrollbar changes could benefit from documentation explaining the rationale.
Consider adding a comment explaining why the key prop is necessary:
<View + // Force re-render when scrollbar visibility changes to ensure proper layout calculations key={hiddenScrollbars ? 'hidden-scrollbars' : 'scrollbars'} style={{ flexGrow: 1, overflow: 'hidden', ...styles.lightScrollbar, }} >
168-195: Consider documenting the scroll handling architectureThe separation of scroll handling between App.tsx and FinancesApp.tsx represents a significant architectural change. Consider adding documentation (either in comments or README) explaining:
- Why scroll handling was moved from App to FinancesApp
- The relationship between these components regarding scroll behavior
- Guidelines for other developers on where to implement scroll-related features
This will help maintain the intended behavior as the codebase evolves.
packages/desktop-client/src/components/FinancesApp.tsx (1)
166-183: Consider extracting layout styles to a constantThe layout implementation looks solid, but the inline styles could be extracted to a constant for better maintainability.
Consider refactoring like this:
+const contentContainerStyles = { + flex: 1, + overflow: 'auto', + position: 'relative', +} as const; + +const titlebarStyles = { + WebkitAppRegion: 'drag', + position: 'absolute', + top: 0, + left: 0, + right: 0, + zIndex: 1000, +} as const; <View ref={scrollableRef} - style={{ - flex: 1, - overflow: 'auto', - position: 'relative', - }} + style={contentContainerStyles} > <Titlebar - style={{ - WebkitAppRegion: 'drag', - position: 'absolute', - top: 0, - left: 0, - right: 0, - zIndex: 1000, - }} + style={titlebarStyles} />packages/desktop-client/src/components/ScrollProvider.tsx (1)
56-60: Update type check to 'HTMLElement' for proper scroll property accessThe properties
scrollTop,scrollHeight, andclientHeightare available onHTMLElementbut not guaranteed on allElementtypes. Changing the type check totarget instanceof HTMLElementensures type safety and prevents potential runtime errors.Apply this diff:
- if (target instanceof Element) { + if (target instanceof HTMLElement) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3731.mdis excluded by!**/*.md
📒 Files selected for processing (3)
- packages/desktop-client/src/components/App.tsx (1 hunks)
- packages/desktop-client/src/components/FinancesApp.tsx (4 hunks)
- packages/desktop-client/src/components/ScrollProvider.tsx (3 hunks)
🔇 Additional comments (8)
packages/desktop-client/src/components/App.tsx (2)
168-175: LGTM: Proper layout container setupThe new View component correctly establishes the root layout container with appropriate theme handling and flex layout properties.
186-193: Verify scroll behavior in mobile navigationThe removal of ScrollProvider from App.tsx aligns with the PR objective. Let's verify that the mobile navigation tabs no longer trigger unexpectedly when scrolling through category/group notes.
✅ Verification successful
Let me gather more specific information about the scroll behavior and mobile navigation components.
Let me check the specific implementation of MobileNavTabs to understand how it uses the ScrollProvider.
Based on the gathered information, I can now provide the final response:
ScrollProvider removal from App.tsx is correct and safe
The ScrollProvider has been correctly moved from App.tsx to FinancesApp.tsx, where it's properly scoped to handle mobile navigation scroll behavior. The MobileNavTabs component still has access to the scroll context through the useScroll hook, and the provider is now closer to where it's actually needed. Key findings:
- ScrollProvider is now properly placed in FinancesApp.tsx with
isDisabled={!isNarrowWidth}, meaning it only activates for mobile views- MobileNavTabs.tsx correctly imports and uses the useScroll hook for scroll position tracking
- The scroll behavior implementation remains intact, just better organized
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining scroll event listeners in the mobile navigation components rg -l "scroll" --type ts --type tsx | grep -i "nav\|mobile" # Look for ScrollProvider usage to ensure it's only used in the intended location ast-grep --pattern 'ScrollProvider'Length of output: 517
Script:
#!/bin/bash # Search for scroll-related code in ts/tsx files with proper file type specification rg -l "scroll" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts | grep -i "nav\|mobile" # Look for mobile navigation component implementations rg -A 5 "MobileNavigation" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts # Check where ScrollProvider is being used and its context rg -A 5 "ScrollProvider" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type tsLength of output: 6587
Script:
#!/bin/bash # Check the implementation of MobileNavTabs and its scroll behavior rg -A 20 "function MobileNavTabs" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts # Look for useScroll hook usage in MobileNavTabs rg -A 10 "useScroll" packages/desktop-client/src/components/mobile/MobileNavTabs.tsx # Check ScrollProvider implementation to understand its purpose rg -A 20 "export function ScrollProvider" packages/desktop-client/src/components/ScrollProvider.tsxLength of output: 3177
packages/desktop-client/src/components/FinancesApp.tsx (3)
2-2: LGTM: Clean implementation of scroll ref setupThe addition of useRef and ScrollProvider imports, along with the properly typed scrollableRef declaration, provides a solid foundation for implementing contained scroll behavior.
Also applies to: 37-37, 137-138
187-273: LGTM: Well-structured routing implementationThe routing implementation is clean and properly organized, with mobile navigation tabs correctly positioned within the ScrollProvider context. This ensures that scroll events are properly contained and won't interfere with navigation tab behavior.
162-164: Verify scroll behavior on mobile and desktop viewsThe ScrollProvider implementation looks correct, with proper conditional enabling based on viewport width. This should resolve the unexpected mobile navigation behavior while maintaining normal scroll behavior on desktop.
Let's verify the ScrollProvider usage:
✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about the ScrollProvider implementation and its usage across the codebase.
ScrollProvider implementation is correctly used and isolated
The implementation is verified to be working as intended:
- ScrollProvider is properly implemented in a dedicated component with isDisabled prop
- It's correctly used in FinancesApp.tsx with the isNarrowWidth condition
- The scroll context is consumed only in mobile-specific components (MobileNavTabs and mobile/transactions/ListBox)
- No conflicting scroll event listeners were found in the codebase
- The other scroll provider (AvoidRefocusScrollProvider) serves a different purpose and doesn't interfere
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ScrollProvider implementation consistency # Test 1: Check if ScrollProvider is used consistently with isDisabled prop rg -A 2 "ScrollProvider" # Test 2: Check for any other scroll event listeners that might interfere ast-grep --pattern 'addEventListener("scroll", $$$)'Length of output: 3993
packages/desktop-client/src/components/ScrollProvider.tsx (3)
20-22: Enhancement: Use of generic type parameter improves flexibilityMaking
ScrollProviderPropsgeneric with<T extends Element>allows for greater type safety and flexibility when working with different element refs.
26-30: Appropriate update to 'ScrollProvider' function signatureUpdating
ScrollProviderto be a generic function ensures consistency with the props and enhances type inference throughout the component.
83-86: Good practice: Ensuring 'useScroll' is called within 'ScrollProvider'Throwing an error when
contextisundefinedhelps prevent misuse of theuseScrollhook outside of its provider, which is a reliable way to enforce correct usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/ScrollProvider.tsx (1)
20-24: Add JSDoc comments to document props.Consider adding JSDoc comments to document the purpose of props, especially
isDisabledwhich controls scroll tracking behavior.+/** + * Props for the ScrollProvider component + * @template T - The type of Element that can be scrolled + */ type ScrollProviderProps<T extends Element> = { + /** Reference to the scrollable container element */ scrollableRef: RefObject<T>; + /** When true, scroll tracking is disabled (e.g., on desktop) */ isDisabled: boolean; children?: ReactNode; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/ScrollProvider.tsx (3 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/ScrollProvider.tsx (2)
83-87: LGTM! Good error handling.The error message clearly indicates how to fix the issue when the hook is used incorrectly.
50-72: Verify scroll behavior on mobile navigation.The changes look good, but let's verify that limiting scroll events to the container fixes the mobile navigation issue.
✅ Verification successful
Let me gather more information about the mobile navigation and ScrollProvider usage.
Based on the gathered information, I can now generate the final response:
ScrollProvider implementation is correctly scoped to container-level events
The changes to ScrollProvider are verified to be correct and safe for mobile navigation:
- The scroll event listener is properly attached to the container element (
scrollableRef.current) instead of window- The implementation uses debouncing (10ms) to optimize performance
- Mobile navigation components (MobileNavTabs.tsx) use ScrollProvider appropriately
- No conflicting window-level scroll listeners were found that could interfere
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if ScrollProvider is properly configured in mobile navigation # Expected: ScrollProvider should only be enabled on mobile and attached to the correct container # Test 1: Verify ScrollProvider usage in mobile navigation rg -A 5 "ScrollProvider.*isNarrowWidth" # Test 2: Check for any window-level scroll listeners that might interfere ast-grep --pattern 'addEventListener("scroll"'Length of output: 6060
Script:
#!/bin/bash # Search for ScrollProvider usage in mobile navigation components rg -l "ScrollProvider|useScroll" packages/desktop-client/src/ # Check for mobile-specific components that might use ScrollProvider rg -l "isNarrowWidth|isMobile" packages/desktop-client/src/ # Look for any scroll-related event handlers in mobile components rg -A 5 "onScroll|scrollTo|scrollTop" packages/desktop-client/src/Length of output: 24449
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
improve-scroll-provider
branch
from
b0ae454 to
8426eb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/mobile/transactions/ListBox.jsx (1)
15-20: Implementation looks good, consider adding error handlingThe new scroll detection logic is more performant and cleaner. The 5px threshold for bottom detection is a good choice for triggering the load more action.
Consider enhancing the implementation with error handling and loading state:
useScrollListener(({ hasScrolledToEnd }) => { const scrolledToBottom = hasScrolledToEnd('down', 5); - if (scrolledToBottom) { + if (scrolledToBottom && !isLoading) { + try { loadMore?.(); + } catch (error) { + console.error('Failed to load more items:', error); } } });You'll need to track the loading state to prevent multiple simultaneous load requests.
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1)
2-2: Consider removing @ts-strict-ignore directive.The
@ts-strict-ignoredirective disables TypeScript's strict type checking for the entire file. Consider addressing any type-related issues directly to maintain type safety throughout the component.packages/desktop-client/src/components/App.tsx (1)
168-195: Good architectural improvementsThe removal of ScrollProvider from App.tsx and its relocation to FinancesApp.tsx follows the single responsibility principle. The use of refs instead of state for scroll handling should indeed improve performance by reducing unnecessary re-renders during scrolling.
Consider documenting these architectural decisions in the codebase to help future maintainers understand the performance considerations behind this structure.
packages/desktop-client/src/components/ScrollProvider.tsx (3)
13-22: Add JSDoc comments to document the types.The new type definitions are well-structured, but adding JSDoc comments would improve maintainability and developer experience.
+/** Represents the possible scroll directions */ type ScrollDirection = 'up' | 'down' | 'left' | 'right'; +/** Arguments passed to scroll listeners */ type ScrollListenerArgs = { scrollX: number; scrollY: number; + /** Checks if currently scrolling in the specified direction */ isScrolling: (direction: ScrollDirection) => boolean; + /** Checks if scrolled to the end in the specified direction within tolerance */ hasScrolledToEnd: (direction: ScrollDirection, tolerance?: number) => boolean; };
31-50: Document the purpose of isDisabled prop.The
isDisabledprop's purpose isn't immediately clear. Add JSDoc to explain when and why scroll tracking should be disabled.+/** + * Props for the ScrollProvider component + * @template T - The type of scrollable element + */ type ScrollProviderProps<T extends Element> = { scrollableRef: RefObject<T>; + /** When true, disables scroll tracking. Useful for optimizing performance on desktop. */ isDisabled: boolean; children?: ReactNode; };
193-205: Add validation for listener argument.Consider adding validation to ensure the listener is not undefined and remains stable.
export function useScrollListener(listener: ScrollListener) { const context = useContext(ScrollContext); if (!context) { throw new Error('useScrollListener must be used within a ScrollProvider'); } + if (typeof listener !== 'function') { + throw new Error('listener must be a function'); + } const { registerListener, unregisterListener } = context; useEffect(() => { const _listener = listener; registerListener(_listener); return () => unregisterListener(_listener); }, [listener, registerListener, unregisterListener]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3731.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/FinancesApp.tsx(4 hunks)packages/desktop-client/src/components/ScrollProvider.tsx(1 hunks)packages/desktop-client/src/components/mobile/MobileNavTabs.tsx(3 hunks)packages/desktop-client/src/components/mobile/transactions/ListBox.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/FinancesApp.tsx
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/ScrollProvider.tsx
[error] 56-61: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 64-76: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 79-84: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 87-100: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (7)
packages/desktop-client/src/components/mobile/transactions/ListBox.jsx (2)
5-5: LGTM: Import change aligns with new scroll handling approach
The switch to useScrollListener aligns with the PR's goal of using a more performant reference-based approach for scroll handling.
Line range hint 1-40: Verify complete migration from useScroll
Let's ensure all instances of useScroll have been migrated to useScrollListener for consistency.
✅ Verification successful
Migration from useScroll to useScrollListener is complete
The verification shows that all scroll-related functionality is using the new useScrollListener hook from ScrollProvider. There are no instances of the old useScroll hook in the codebase. The implementation in ListBox.jsx is consistent with other components like MobileNavTabs.tsx that also use the same pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining useScroll usage that needs migration
# Search for any remaining useScroll imports or usage
echo "Checking for remaining useScroll usage..."
rg "useScroll" --type js --type jsx --type tsx
Length of output: 167
Script:
#!/bin/bash
# Description: Check for any remaining useScroll usage with correct file type flags
# Search for any remaining useScroll usage
echo "Checking for remaining useScroll usage..."
rg "useScroll" -t js -t jsx -t ts -t tsx
# Also check for ScrollProvider imports as they might be related
echo -e "\nChecking ScrollProvider imports..."
rg "ScrollProvider" -t js -t jsx -t ts -t tsx
Length of output: 320
Script:
#!/bin/bash
# Description: Check for any remaining useScroll usage without file type flags
# Search for any remaining useScroll usage
echo "Checking for remaining useScroll usage..."
rg "useScroll"
# Also check for ScrollProvider imports
echo -e "\nChecking ScrollProvider imports..."
rg "ScrollProvider"
Length of output: 2456
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1)
126-132: LGTM! Improved scroll handling implementation.
The new scroll handling implementation using useScrollListener is more performant and stable:
- Uses a reference-based approach instead of state
- Simplifies logic to just direction checks
- Removes the need to track previous scroll position
Let's verify this is the only instance of scroll handling in this component:
✅ Verification successful
The search results show that useScrollListener is the only scroll-related code in the file, and there are no remnants of the previous implementation. However, let's verify the ScrollProvider implementation to ensure it's using refs as mentioned:
Implementation verified - using refs for scroll handling
The scroll handling implementation is correctly using refs through the ScrollProvider, which maintains scroll position and dimensions using useRef hooks. This confirms the performance benefits mentioned in the review, as it avoids state-based re-renders.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining scroll-related code that might conflict
# with the new implementation
# Test 1: Look for any other scroll-related hooks or event listeners
rg -i "scroll" "packages/desktop-client/src/components/mobile/MobileNavTabs.tsx"
# Test 2: Verify the removal of previous useScroll implementation
rg "useScroll" "packages/desktop-client/src/components/mobile/MobileNavTabs.tsx"
Length of output: 384
Script:
#!/bin/bash
# Check ScrollProvider implementation
rg -A 10 "export function ScrollProvider" ../ScrollProvider.tsx || rg -A 10 "export function ScrollProvider" "packages/desktop-client/src/components/ScrollProvider.tsx"
Length of output: 720
packages/desktop-client/src/components/App.tsx (3)
168-175: LGTM: Improved layout structure
The replacement of ScrollProvider with a simpler View component is a good architectural decision that aligns with the PR's performance objectives. The flex column layout with full height ensures proper containment of child components.
186-193: LGTM: Clean component hierarchy
The component structure is well-organized and maintains proper separation of concerns. The ErrorBoundary placement ensures proper error handling throughout the app.
177-183: Verify the impact of key-based re-renders
While using a key prop to handle scrollbar visibility changes is valid, it forces a complete re-render of the inner View and all its children. Let's verify this doesn't cause performance issues.
✅ Verification successful
Let me analyze the scrollbar visibility changes further by checking the implementation details and component hierarchy.
Based on the gathered information, I can now provide the final response:
Key-based re-renders for scrollbar visibility are optimized and safe
The implementation is well-designed and doesn't cause performance concerns because:
- Scrollbar visibility changes are rare, only triggered by system-level changes through
onScrollbarChange - The re-render scope is limited to styling updates, not affecting child component logic
- The pattern is consistently used across the app in key components (Modal, Popover, BudgetTable) showing it's a deliberate architectural choice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance impact of scrollbar visibility changes
# Test 1: Look for other components that might trigger scrollbar visibility changes
rg -g '*.{ts,tsx,js,jsx}' "hasHiddenScrollbars|setHiddenScrollbars"
# Test 2: Check for scroll-related event handlers that might affect performance
rg -g '*.{ts,tsx,js,jsx}' "onScroll|addEventListener.*scroll"
Length of output: 2528
Script:
#!/bin/bash
# Check the implementation of scrollbar visibility detection and its usage
ast-grep --pattern 'function hasHiddenScrollbars() {
$$$
}'
# Check App.tsx component's structure and re-render triggers
ast-grep --pattern 'const App = () => {
$$$
}'
# Look for components that might be affected by the re-render
rg -g '*.{ts,tsx,js,jsx}' -B 2 -A 2 "style.*lightScrollbar"
Length of output: 2261
packages/desktop-client/src/components/ScrollProvider.tsx (1)
1-205: Implementation successfully addresses the PR objectives.
The refactored ScrollProvider effectively solves the mobile navigation tabs issue by:
- Using refs instead of state to avoid unnecessary re-renders during scrolling
- Supporting conditional disabling for desktop optimization
- Providing precise scroll position tracking with proper cleanup
The implementation is type-safe, performant, and maintainable.
🧰 Tools
🪛 Biome
[error] 56-61: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 64-76: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 79-84: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 87-100: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
joel-jeremy
force-pushed
the
improve-scroll-provider
branch
from
9e5f17a to
fefe49b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/ScrollProvider.tsx (1)
145-163: Optimize scroll handler and improve type safety.
- The debounced scroll handler is recreated on every render
- Non-null assertions could be replaced with safer alternatives
Consider these improvements:
+const createScrollHandler = ( + refs: { + scrollX: RefObject<number | undefined>; + scrollY: RefObject<number | undefined>; + }, + callbacks: { + isScrolling: (direction: ScrollDirection) => boolean; + hasScrolledToEnd: (direction: ScrollDirection) => boolean; + }, + listeners: RefObject<ScrollListener[]> +) => debounce((e: Event) => { + const target = e.target; + if (target instanceof Element) { + // ... handler implementation + } +}, 10); useEffect(() => { if (isDisabled) { return; } - const listenToScroll = debounce((e: Event) => { - // ... current implementation - }, 10); + const listenToScroll = useMemo( + () => createScrollHandler( + { scrollX, scrollY }, + { isScrolling, hasScrolledToEnd }, + listeners + ), + [isScrolling, hasScrolledToEnd] + ); // ... rest of effect }, [hasScrolledToEnd, isDisabled, isScrolling]); - scrollX: scrollX.current!, - scrollY: scrollY.current!, + scrollX: scrollX.current ?? 0, + scrollY: scrollY.current ?? 0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3731.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/FinancesApp.tsx(4 hunks)packages/desktop-client/src/components/ScrollProvider.tsx(1 hunks)packages/desktop-client/src/components/mobile/MobileNavTabs.tsx(3 hunks)packages/desktop-client/src/components/mobile/transactions/ListBox.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/FinancesApp.tsx
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx
- packages/desktop-client/src/components/mobile/transactions/ListBox.jsx
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/ScrollProvider.tsx
[error] 56-61: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 64-76: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 79-84: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 87-100: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
packages/desktop-client/src/components/ScrollProvider.tsx (3)
13-26: Well-structured type definitions!
The new type definitions provide excellent type safety and the listener pattern is a good choice for performance optimization. The ScrollListenerArgs type provides comprehensive scroll information needed by consumers.
42-50: Good use of refs for performance optimization!
Using refs instead of state for scroll values is an excellent choice to prevent unnecessary re-renders during scroll events.
193-205: Well-implemented hook with proper cleanup!
The hook implementation includes:
- Proper error handling for context usage
- Clean effect cleanup
- Complete dependency array
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/ScrollProvider.tsx (2)
15-20: Consider making scroll position values optional in ScrollListenerArgs.The scroll position values can be undefined when the scroll event hasn't fired yet, but the type definition assumes they're always numbers.
type ScrollListenerArgs = { - scrollX: number; - scrollY: number; + scrollX: number | undefined; + scrollY: number | undefined; isScrolling: (direction: ScrollDirection) => boolean; hasScrolledToEnd: (direction: ScrollDirection, tolerance?: number) => boolean; };
179-184: Consider optimizing listener management.The current implementation is correct but could benefit from Set usage for better performance in add/remove operations.
- const listeners = useRef<ScrollListener[]>([]); + const listeners = useRef<Set<ScrollListener>>(new Set()); const registerScrollListener: RegisterScrollListener = useCallback(listener => { - listeners.current.push(listener); + listeners.current.add(listener); return () => { - listeners.current = listeners.current.filter(l => l !== listener); + listeners.current.delete(listener); }; }, []);🧰 Tools
🪛 GitHub Check: lint
[warning] 179-179:
Insert⏎····
[warning] 180-180:
Replace····with······
[warning] 182-182:
Insert··
[warning] 183-183:
Insert··
[warning] 184-184:
Replace····with······
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/ScrollProvider.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/ScrollProvider.tsx
[error] 57-62: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-77: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 80-85: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 88-101: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 GitHub Check: lint
packages/desktop-client/src/components/ScrollProvider.tsx
[warning] 24-24:
Replace listener:·ScrollListener with ⏎··listener:·ScrollListener,⏎
[warning] 179-179:
Insert ⏎····
[warning] 180-180:
Replace ···· with ······
[warning] 182-182:
Insert ··
[warning] 183-183:
Insert ··
[warning] 184-184:
Replace ···· with ······
[warning] 185-185:
Replace ··},·[] with ····},⏎····[],⏎··
🔇 Additional comments (2)
packages/desktop-client/src/components/ScrollProvider.tsx (2)
32-51: Well-structured component props and state management.
Good use of generics for type safety and refs for performance. The isDisabled prop provides good control over scroll behavior.
194-204: Well-implemented hook with proper error handling and cleanup.
The hook correctly validates context usage and ensures proper cleanup of listeners.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/ScrollProvider.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/ScrollProvider.tsx
[error] 59-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 67-79: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 82-87: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 90-103: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (2)
packages/desktop-client/src/components/ScrollProvider.tsx (2)
13-37: LGTM! Well-structured type definitions.
The new type definitions provide excellent type safety and clear interfaces for scroll handling. The generic type parameter for ScrollProviderProps ensures type-safe element references.
199-209: LGTM! Clean hook implementation.
The useScrollListener hook is well-implemented with proper error handling and cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/ScrollProvider.tsx (1)
100-129: Consider refactoring the isScrolling function to reduce code duplication.The
isScrollingfunction has repeated null checks across different cases. Consider extracting a helper function to reduce duplication.const isScrolling = useCallback((direction: ScrollDirection) => { + const hasDirectionalChange = ( + prev?: number, + curr?: number, + isForward: boolean + ) => + prev !== undefined && + curr !== undefined && + (isForward ? prev < curr : prev > curr); + switch (direction) { case 'up': - return ( - previousScrollY.current !== undefined && - scrollY.current !== undefined && - previousScrollY.current > scrollY.current - ); + return hasDirectionalChange(previousScrollY.current, scrollY.current, false); case 'down': - return ( - previousScrollY.current !== undefined && - scrollY.current !== undefined && - previousScrollY.current < scrollY.current - ); + return hasDirectionalChange(previousScrollY.current, scrollY.current, true); case 'left': - return ( - previousScrollX.current !== undefined && - scrollX.current !== undefined && - previousScrollX.current > scrollX.current - ); + return hasDirectionalChange(previousScrollX.current, scrollX.current, false); case 'right': - return ( - previousScrollX.current !== undefined && - scrollX.current !== undefined && - previousScrollX.current < scrollX.current - ); + return hasDirectionalChange(previousScrollX.current, scrollX.current, true); default: return false; } }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/ScrollProvider.tsx(1 hunks)
🔇 Additional comments (5)
packages/desktop-client/src/components/ScrollProvider.tsx (5)
13-38: LGTM! Well-structured type definitions.
The type definitions provide strong type safety and clear contracts for the scroll handling functionality. The generic type parameter T extends Element ensures type safety for the scrollableRef prop.
57-97: LGTM! Well-structured scroll end detection.
The hasScrolledToEnd function is well-implemented with helper functions that improve readability and maintainability. The switch statement correctly handles all scroll directions.
136-173: LGTM! Efficient scroll event handling.
The scroll event handling is well-implemented:
- Uses debouncing to optimize performance
- Correctly updates refs to track scroll positions
- Properly notifies listeners with current scroll state
- Includes proper cleanup in the useEffect hook
193-203: LGTM! Well-implemented scroll listener hook.
The useScrollListener hook follows React best practices:
- Proper error handling for context usage
- Clean listener registration and cleanup
- Correct dependency array in useEffect
41-184: Verify the implementation in mobile navigation tabs.
The implementation aligns well with the PR's objective to fix mobile navigation tabs expanding/collapsing when scrolling in modals:
- Uses refs instead of state to prevent unnecessary re-renders
- Implements debounced scroll handling to optimize performance
- Provides clean APIs for scroll position tracking
Let's verify the integration with mobile navigation tabs:
✅ Verification successful
Let me search for more specific information about the mobile navigation tabs implementation.
Based on the code review, I can now generate the final response:
Implementation in mobile navigation tabs is correctly using ScrollProvider
The MobileNavTabs component properly integrates with ScrollProvider through useScrollListener hook to handle scroll-based navigation behavior:
- Correctly uses the scroll direction detection to show/hide navigation
- Implements smooth animations using react-spring for transitions
- Properly handles both scroll and drag gestures for the expanding/collapsing behavior
The implementation successfully addresses the original issue by:
- Using the ScrollProvider's isScrolling API to detect scroll direction
- Applying proper show/hide animations based on scroll direction
- Avoiding unnecessary re-renders by leveraging ScrollProvider's ref-based implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how MobileNavTabs uses the ScrollProvider
rg -A 5 "useScrollListener.*MobileNavTabs"
# Verify there are no other scroll event listeners in MobileNavTabs
ast-grep --pattern 'addEventListener("scroll"'
Length of output: 6063
Script:
#!/bin/bash
# Search for MobileNavTabs component implementation
fd -e tsx -e ts -e js MobileNavTabs --exec cat {}
# Search for any scroll-related hooks or components used in mobile navigation
rg -l "useScroll|ScrollProvider" packages/desktop-client/src/components/mobile/
Length of output: 6392
…tainer and not the whole window
… state to avoid re-renders when scrolling
joel-jeremy
force-pushed
the
improve-scroll-provider
branch
from
ec53e21 to
8282842
Compare
This should fix the issue where the mobile navigation tabs are opening and closing when you are scrolling anywhere e.g. scrolling through long category/group notes in mobile budget page
Steps to test:
Previous result: The mobile navigation tabs collapses/expands when scrolling through the notes
Current result: The mobile navigation tabs stays put
Rewritten to be more performant by being ref based instead of state to avoid re-renders when scrolling.
I also disabled the scroll tracking in desktop so that could potentially help with the performance