debounce modal navigation #5006
Conversation
WalkthroughThe changes introduced in this pull request enhance the modal navigation system by implementing a debounced navigation mechanism. This involves creating a new Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
app/packages/core/src/components/Modal/debouncedNavigator.ts (1)
8-16: Extract debounce time default as a named constantConsider extracting the magic number 100 as a named constant at the module level for better maintainability and documentation.
+const DEFAULT_DEBOUNCE_TIME_MS = 100; + export function createDebouncedNavigator({ isNavigationIllegalWhen, navigateFn, onNavigationStart, - debounceTime = 100, + debounceTime = DEFAULT_DEBOUNCE_TIME_MS, }: DebouncedNavigatorOptions) {app/packages/core/src/components/Modal/hooks.ts (1)
11-17: Remove resolved TODO commentThe implementation correctly uses refs to solve the referential stability issue mentioned in the TODO comment. Since the solution is now implemented, the TODO comment can be removed.
- // todo: jsonPanel and helpPanel are not referentially stable - // so use refs here const jsonPanelRef = useRef(jsonPanel); const helpPanelRef = useRef(helpPanel);app/packages/state/src/recoil/modal.ts (1)
122-123: LGTM! The type changes align well with the debouncing requirements.The addition of the optional
offsetparameter to both navigation methods provides the necessary flexibility for implementing debounced navigation while maintaining type safety. This change enables:
- Immediate execution of single navigation commands (when offset is undefined)
- Accumulation of multiple rapid navigation commands (using offset)
Consider documenting the following contract in the type definition:
- Positive offset values indicate forward navigation
- Negative offset values indicate backward navigation
- Undefined offset represents single-step navigation
app/packages/core/src/components/Modal/debouncedNavigator.test.ts (2)
1-27: Consider extracting the debounce time as a constant.The debounce time of 100ms is used in multiple test cases. Consider extracting it to a constant at the top of the file for better maintainability.
+const DEBOUNCE_TIME_MS = 100; describe("createDebouncedNavigator", () => { // ... debouncedNavigator = createDebouncedNavigator({ isNavigationIllegalWhen, navigateFn, onNavigationStart, - debounceTime: 100, + debounceTime: DEBOUNCE_TIME_MS, });
48-54: Consider using the debounce time constant in timing assertions.The timing values (50ms, 100ms) should use the same constant as the debounce time configuration for better maintainability and clarity.
-vi.advanceTimersByTime(50); +vi.advanceTimersByTime(DEBOUNCE_TIME_MS / 2); -vi.advanceTimersByTime(100); +vi.advanceTimersByTime(DEBOUNCE_TIME_MS);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/packages/core/src/components/Modal/ModalNavigation.tsx(5 hunks)app/packages/core/src/components/Modal/debouncedNavigator.test.ts(1 hunks)app/packages/core/src/components/Modal/debouncedNavigator.ts(1 hunks)app/packages/core/src/components/Modal/hooks.ts(1 hunks)app/packages/state/src/hooks/useExpandSample.ts(2 hunks)app/packages/state/src/recoil/modal.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/core/src/components/Modal/ModalNavigation.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/debouncedNavigator.test.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/debouncedNavigator.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Modal/hooks.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/hooks/useExpandSample.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/modal.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (14)
app/packages/core/src/components/Modal/debouncedNavigator.ts (1)
18-25: LGTM! Clean implementation of state cleanup
The cleanup function properly handles all state variables and timeout clearing.
app/packages/core/src/components/Modal/hooks.ts (2)
3-3: LGTM: Import addition is appropriate
The addition of useRef import is necessary for the new ref-based implementation.
19-22: Verify integration with debounced navigation
The stable panel references should work well with the debounced navigation mechanism, but we should verify that panel closures are properly debounced when rapid navigation occurs.
✅ Verification successful
Panel closures are properly integrated with debounced navigation
The implementation correctly handles panel closures during navigation:
- The
onNavigatecallback (which closes panels) is properly integrated into the debounced navigator asonNavigationStart - The debounced navigator calls
onNavigationStartat appropriate times:- Immediately on the first navigation
- After the debounce period (150ms) for subsequent rapid navigations
- Panel closures won't be triggered unnecessarily during rapid navigation due to the debouncing mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for debounced navigator implementation and its usage with panels
ast-grep --pattern 'createDebouncedNavigator($$$)'
rg -A 5 'onNavigate.*close'
Length of output: 1923
Script:
#!/bin/bash
# Let's examine the implementation of the debounced navigator and how it handles the onNavigationStart callback
cat app/packages/core/src/components/Modal/debouncedNavigator.ts
# Also check the implementation of onNavigate in the Modal component
cat app/packages/core/src/components/Modal/ModalNavigation.tsx
# And check for any panel-related tests
rg -l "panel.*close" --type ts
Length of output: 6187
app/packages/state/src/hooks/useExpandSample.ts (1)
68-83: Verify integration with debouncing mechanism.
The offset-based navigation support is crucial for the debouncing implementation. Let's verify that the debouncing logic correctly utilizes these offset parameters when accumulating navigation commands.
✅ Verification successful
Debouncing mechanism correctly integrates with offset-based navigation
The implementation is properly integrated:
- The
createDebouncedNavigatorcorrectly accumulates navigation offsets during the debounce period - The
ModalNavigationcomponent uses two debounced navigators (next/previous) that properly utilize the offset parameter - Tests verify that offsets are accumulated and passed correctly to navigation functions
- The debouncing mechanism is thoroughly tested for various scenarios including offset accumulation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the debouncing implementation that uses these navigation functions
rg -t typescript -l "createDebouncedNavigator|useExpandSample.*offset"
# Look for tests validating the debounced navigation
rg -t typescript "describe.*[Dd]ebounce.*[Nn]avig" -A 10
Length of output: 201
Script:
#!/bin/bash
# Search for the debouncing implementation that uses these navigation functions
rg "createDebouncedNavigator|useExpandSample.*offset"
# Look for tests validating the debounced navigation
rg "describe.*[Dd]ebounce.*[Nn]avig" -A 10
# Search for files containing both debounce and navigation related terms
rg -l "debounce.*\W+.*next|next.*\W+.*debounce"
# Look for the implementation context around useExpandSample
rg "useExpandSample" -A 10 -B 10
Length of output: 5726
Script:
#!/bin/bash
# Look at the debounced navigator implementation
cat app/packages/core/src/components/Modal/debouncedNavigator.ts
# Check the test file for navigation offset usage
cat app/packages/core/src/components/Modal/debouncedNavigator.test.ts
# Look at the modal navigation implementation
cat app/packages/core/src/components/Modal/ModalNavigation.tsx
Length of output: 11616
app/packages/state/src/recoil/modal.ts (1)
122-123: Verify the impact on existing modal navigation implementations.
The type signature change requires updates to all implementations of ModalNavigation.
✅ Verification successful
Let me gather more specific information about the ModalNavigation interface and its implementations.
Let me try one more search to find any implementations or usages with a different pattern.
The optional offset parameter is already being used correctly in the codebase
The search results show that the existing implementation in app/packages/core/src/components/Modal/ModalNavigation.tsx already uses the offset parameter when calling both next(offset) and previous(offset). Therefore, the type signature change to make the offset parameter optional is actually aligning with the current usage pattern and doesn't require any implementation updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of ModalNavigation interface
ast-grep --pattern 'implements ModalNavigation {
$$$
}'
# Search for direct usage of next/previous methods
rg -A 2 '\b(next|previous)\s*\(\s*\)' --type ts
Length of output: 1695
Script:
#!/bin/bash
# Search for ModalNavigation interface definition
ast-grep --pattern 'interface ModalNavigation {
$$$
}'
# Search for classes implementing ModalNavigation with next/previous methods
ast-grep --pattern 'class $_ implements ModalNavigation {
$$$
next($$$) {
$$$
}
$$$
}'
# Search for more specific usage of next/previous with offset parameter
rg '\b(next|previous)\s*\(\s*\d+\s*\)' --type ts
Length of output: 210
Script:
#!/bin/bash
# Search for files containing both ModalNavigation and next/previous methods
rg -l "ModalNavigation" --type ts | xargs rg -A 5 "\b(next|previous)\b"
# Search for any imports of ModalNavigation
rg "import.*ModalNavigation" --type ts
# Search for any type annotations using ModalNavigation
rg ": ModalNavigation" --type ts
Length of output: 3186
app/packages/core/src/components/Modal/debouncedNavigator.test.ts (1)
1-181: Well-structured test suite with comprehensive coverage.
The test implementation is solid, following testing best practices with:
- Clear test organization
- Proper setup and teardown
- Comprehensive coverage of main scenarios
- Good use of mock functions and fake timers
app/packages/core/src/components/Modal/ModalNavigation.tsx (8)
6-6: Imports are appropriate and necessary
The imported React hooks (useCallback, useEffect, useMemo, useRef) are correctly utilized in the component to manage state and performance optimizations.
9-9: Correctly importing createDebouncedNavigator
The createDebouncedNavigator is properly imported from ./debouncedNavigator, enabling the debounced navigation functionality.
71-82: Dependencies for nextNavigator are correctly specified
The dependencies in the useMemo hook for nextNavigator are appropriately set to navigation, onNavigate, and setModal. This ensures that the navigator remains referentially stable and only re-creates when necessary.
84-93: Dependencies for previousNavigator are correctly specified
Similarly, the useMemo hook for previousNavigator has the correct dependencies, ensuring stability and proper functioning of the debounced navigator.
95-100: Proper cleanup of debounced navigators
The useEffect hook effectively cleans up the nextNavigator and previousNavigator when the component unmounts or when the navigators change, preventing potential memory leaks.
116-118: Keyboard navigation handlers correctly updated
The keyboard event handlers now utilize previousNavigator.navigate() and nextNavigator.navigate(), integrating the debounced navigation functionality seamlessly.
121-121: Dependencies for keyboardHandler are accurate
The dependency array for keyboardHandler includes nextNavigator and previousNavigator, ensuring that the handler updates appropriately when these navigators change.
Line range hint 136-146: Arrow click handlers use debounced navigators
The onClick handlers for the navigation arrows are correctly updated to use previousNavigator.navigate and nextNavigator.navigate, providing debounced navigation on user clicks.
What changes are proposed in this pull request?
This PR allows modal navigation to be debounced. The first navigation command is immediately executed, whereas subsequent commands are accumulated and executed later. This should reduce the number of network requests incurred when users navigate really fast, and also improve perceived sample navigation speed.
How is this patch tested? If it is not, please explain why.
Unit tests. Local smoke tests. All existing E2E green.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation