Optimize modal navigation #4947
Conversation
WalkthroughThe changes in this pull request primarily involve significant refactoring within the routing and state management components of the application. Key modifications include the restructuring of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (5)
app/packages/app/src/useEvents/useSetGroupSlice.ts (2)
18-22: LGTM! Consider adding type safety for the event value.The addition of the
eventproperty to the router state object is a good improvement. It provides more context about the nature of the navigation, which can be useful for routing logic or analytics.To enhance type safety, consider defining a union type for possible event values:
type RouterEvent = 'slice' | /* other possible events */; // Then use it in the router.push call router.push(pathname, { ...router.location.state, event: 'slice' as RouterEvent, groupSlice: slice, });This will help prevent typos and ensure consistency across the application.
Line range hint
1-29: Summary: Good optimization for modal navigationThe changes in this file, particularly the addition of the
eventproperty in therouter.pushcall, align well with the PR objectives of optimizing modal navigation. This modification provides more context about the navigation event, which could be leveraged for more efficient routing or analytics.As you continue to optimize the navigation system:
- Ensure consistent usage of the
eventproperty across all relevant navigation calls.- Consider updating any routing logic that might benefit from this additional context.
- If not already planned, think about adding analytics to track these navigation events, which could provide valuable insights for further optimizations.
app/packages/app/src/useWriters/onSetGroupSlice.ts (1)
16-20: LGTM! Consider enhancing type safety.The addition of the
eventproperty to the router state aligns well with the broader refactoring efforts mentioned in the PR objectives. This change should improve the consistency of modal navigation handling.To further enhance type safety and maintainability, consider defining a constant for the "slice" event string:
const SLICE_EVENT = "slice" as const; // Then use it in the router.push call router.push(pathname, { ...router.location.state, event: SLICE_EVENT, groupSlice: slice, });This approach would make it easier to refactor in the future and prevent typos.
e2e-pw/src/oss/poms/grid/index.ts (1)
Line range hint
1-161: Summary: Optimized grid interactions for improved performance.The changes in this file contribute to the overall goal of optimizing modal navigation by simplifying the
openNthSampleandselectSlicemethods. The removal of thepageChangewrapper in both methods should reduce unnecessary overhead in the recoil data flow, as mentioned in the PR objectives. These modifications are minimal and focused, which reduces the risk of introducing new issues while potentially improving the performance of the FiftyOne application, especially when dealing with a large number of sidebar fields.Consider documenting these changes in the component's documentation or comments to explain the performance optimization strategy for future maintainers.
app/packages/state/src/recoil/pathData/counts.ts (1)
Line range hint
178-189: Excellent optimization incumulativeCountsselectorThe modification to use the new
gatheredPathsselector incumulativeCountsis a great improvement. This change enhances code reusability and maintainability while preserving the existing functionality. It aligns well with the PR's objective of optimizing modal navigation and improving performance, especially when dealing with a large number of sidebar fields.Consider adding a brief comment explaining the performance benefit of using
gatheredPathshere, to help future maintainers understand the rationale behind this change.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- app/packages/app/src/routing/RouterContext.ts (4 hunks)
- app/packages/app/src/routing/matchPath.ts (1 hunks)
- app/packages/app/src/useEvents/useSetGroupSlice.ts (1 hunks)
- app/packages/app/src/useWriters/onSetGroupSlice.ts (1 hunks)
- app/packages/core/src/components/Grid/useRefreshers.ts (1 hunks)
- app/packages/state/src/recoil/pathData/counts.ts (2 hunks)
- app/packages/state/src/recoil/sidebarExpanded.ts (0 hunks)
- e2e-pw/src/oss/poms/grid/index.ts (3 hunks)
- e2e-pw/src/oss/poms/modal/index.ts (3 hunks)
- e2e-pw/src/oss/specs/selection.spec.ts (1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/state/src/recoil/sidebarExpanded.ts
🧰 Additional context used
📓 Path-based instructions (9)
app/packages/app/src/routing/RouterContext.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/app/src/routing/matchPath.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/app/src/useEvents/useSetGroupSlice.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/app/src/useWriters/onSetGroupSlice.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/Grid/useRefreshers.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/pathData/counts.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.e2e-pw/src/oss/poms/grid/index.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.e2e-pw/src/oss/poms/modal/index.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.e2e-pw/src/oss/specs/selection.spec.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 (10)
app/packages/app/src/routing/matchPath.ts (1)
13-13: LGTM! Verify usage of updatedLocationStatetype.The addition of "slice" as a possible value for the
eventproperty inLocationStatetype is a good enhancement that aligns with the PR objectives. It provides more flexibility in handling different types of navigation events.To ensure consistency, let's verify the usage of this updated type across the codebase:
✅ Verification successful
Verification Successful! The
LocationStatetype update is consistently applied across the codebase.
The addition of "slice" to theeventproperty in theLocationStatetype is correctly utilized in all relevant parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of LocationState type and 'event' property # Search for LocationState type usage echo "LocationState usage:" rg -t ts -t tsx "LocationState" --glob "!app/packages/app/src/routing/matchPath.ts" # Search for event property access on LocationState echo "\nEvent property access:" rg -t ts -t tsx "LocationState.*\.event" --glob "!app/packages/app/src/routing/matchPath.ts" # Search for assignments to event property echo "\nEvent property assignments:" rg -t ts -t tsx "event:\s*['\"](?:modal|slice)['\"]" --glob "!app/packages/app/src/routing/matchPath.ts"Length of output: 552
Script:
#!/bin/bash # Description: Check usage of LocationState type and 'event' property in .ts and .tsx files # Search for LocationState type usage echo "LocationState usage:" rg "LocationState" --glob "!app/packages/app/src/routing/matchPath.ts" -g "*.ts" -g "*.tsx" # Search for event property access on LocationState echo "\nEvent property access:" rg "LocationState.*\.event" --glob "!app/packages/app/src/routing/matchPath.ts" -g "*.ts" -g "*.tsx" # Search for assignments to event property echo "\nEvent property assignments:" rg "event:\s*['\"](?:modal|slice)['\"]" --glob "!app/packages/app/src/routing/matchPath.ts" -g "*.ts" -g "*.tsx"Length of output: 2616
e2e-pw/src/oss/specs/selection.spec.ts (1)
98-100: Improved modal closure method enhances test clarity and performance.The change from
await grid.url.back();toawait modal.close();is a positive improvement. It directly uses the modal's close method, which is more semantically clear and likely more efficient. This change aligns well with the PR's objective of optimizing modal navigation and simplifying control flow.e2e-pw/src/oss/poms/grid/index.ts (2)
81-81: LGTM: Streamlined slice selection process.The removal of the
pageChangewrapper in theselectSlicemethod simplifies the implementation, which is in line with the PR objectives to optimize modal navigation and eliminate unnecessary overhead. This change should contribute to improved performance when selecting slices.To ensure this change doesn't introduce any regressions, please verify that the e2e tests related to slice selection still pass:
#!/bin/bash # Description: Run e2e tests related to slice selection # Note: Adjust the test command based on your project's setup npm run test:e2e -- --grep "select.*slice"
55-55: LGTM: Simplified method implementation.The removal of the
pageChangewrapper simplifies theopenNthSamplemethod, which aligns with the PR objectives to optimize modal navigation and remove unnecessary overhead. This change is consistent with similar modifications in other files and should contribute to improved performance.To ensure this change doesn't introduce any regressions, please verify that the e2e tests related to opening samples still pass:
app/packages/state/src/recoil/pathData/counts.ts (1)
149-162: LGTM: Well-implementedgatheredPathsselectorThe new
gatheredPathsselector is well-implemented using Recoil'sselectorFamily. It correctly uses TypeScript for type safety and employs theSetdata structure to ensure unique paths. The use of the existinggatherPathsutility function maintains consistency with the rest of the codebase.e2e-pw/src/oss/poms/modal/index.ts (3)
1-1: LGTM: Import statement reordering.The reordering of the import statement is a minor style improvement, alphabetically sorting the imports. This change doesn't affect functionality and improves code readability.
220-220: Simplified closing logic: Verify potential side effects.The removal of
this.url.pageChange(...)in favor of a direct click action on the body element simplifies the closing logic and potentially improves performance. This change aligns with the PR objective of optimizing modal navigation.Please verify that removing
pageChangedoesn't have any unintended side effects. Run the following script to check for other usages ofpageChangein the codebase:#!/bin/bash # Description: Check for other usages of pageChange method rg "pageChange\(" --type ts
121-123: Simplified navigation logic: Verify potential side effects.The removal of
this.url.pageChange(...)in favor of a direct click action simplifies the navigation logic and potentially improves performance. This change aligns with the PR objective of optimizing modal navigation.Please verify that removing
pageChangedoesn't have any unintended side effects. Run the following script to check for other usages ofpageChangein the codebase:✅ Verification successful
Simplified navigation logic: No unintended side effects detected.
The removal of
this.url.pageChange(...)does not impact other usages ofpageChangein the codebase.
e2e-pw/src/oss/poms/url.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of pageChange method rg "pageChange\(" --type tsLength of output: 192
app/packages/app/src/routing/RouterContext.ts (2)
72-72: Initialization ofgetEntryResourcewithmakeGetEntryResourceThe
getEntryResourcefunction is correctly initialized using the generic factory functionmakeGetEntryResource<T>(), which enhances code modularity and readability.
145-151: Consistent Use of UpdatedgetEntryResourceSignatureThe
getEntryResourcefunction is invoked with the updated parameter structure, ensuring alignment with the new function signature and improving maintainability.
| @@ -160,7 +175,7 @@ export const cumulativeCounts = selectorFamily< | |||
| get: | |||
| ({ extended, path: key, modal, ftype, embeddedDocType }) => | |||
| ({ get }) => { | |||
| return [...new Set(gatherPaths(get, ftype, embeddedDocType))].reduce( | |||
There was a problem hiding this comment.
This functions becomes expensive as the dataset schema grows. The new gatheredPaths selector provides caching
| handleError?: (error: unknown) => void; | ||
| }): Resource<Entry<T>> => { | ||
| if (isReusable(location)) { | ||
| throw currentResource; |
There was a problem hiding this comment.
Throw the current resource (page) if it can be used and skip the page change
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/packages/app/src/routing/RouterContext.ts (3)
186-201: LGTM! Consider simplifying theisReusablefunction.The introduction of
SKIP_EVENTSand themakeGetEntryResourcefunction improves the organization of the routing logic. The closure pattern allows for maintaining state between calls, which is a good approach.Consider simplifying the
isReusablefunction for better readability:const isReusable = (location: FiftyOneLocation) => currentLocation && (SKIP_EVENTS.has(location.state.event || "") || SKIP_EVENTS.has(currentLocation.state.event || ""));
203-216: Remove unused parameter from type definition.The refactored
getEntryResourcefunction signature improves readability and flexibility. However, there's an unused parameter in the type definition.Remove the unused
currentparameter from the type definition:}: { environment: Environment; routes: RouteDefinition<T>[]; location: FiftyOneLocation; hard: boolean; handleError?: (error: unknown) => void; }): Resource<Entry<T>> => {
85-100: LGTM! Consider adding error logging.The new error handling logic in the
updatefunction improves the robustness of the routing system. It correctly handles the case where aResourceis thrown to skip page changes.Consider adding error logging for unexpected exceptions:
} catch (e) { if (e instanceof Resource) { // skip the page change if a resource is thrown return; } console.error('Unexpected error during update:', e); throw e; }This will help with debugging in case of unexpected errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/packages/app/src/routing/RouterContext.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/app/src/routing/RouterContext.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 (1)
app/packages/app/src/routing/RouterContext.ts (1)
150-156: LGTM! Consistent use of newgetEntryResourcesignature.The
loadmethod has been correctly updated to use the new object parameter format ofgetEntryResource. This change maintains consistency throughout the codebase and improves readability.
| if (isReusable(location)) { | ||
| // throw the current resource (page) if it can be reused | ||
| throw currentResource; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider alternative approach for handling reusable resources.
The current implementation throws the current resource as an exception to skip unnecessary page changes. While this achieves the desired result, it's an unconventional use of exceptions that might lead to confusion.
Consider returning the current resource instead of throwing it:
if (isReusable(location)) {
return currentResource;
}This approach maintains the same functionality while following more conventional control flow patterns.
What changes are proposed in this pull request?
Optimizes modal navigation when a large number of sidebar fields are present. Sample routing introduced unnecessary overhead in recoil data flow, which this PR removes. Before and after can be tested with a dataset like the below
How is this patch tested? If it is not, please explain why.
Existing coverage
Release Notes
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
gatheredPaths, to enhance path processing.Bug Fixes
Refactor
Tests