Add modal routing optimization #5014
Conversation
WalkthroughThe pull request introduces significant changes to several files, primarily focusing on the restructuring of routing and event handling within the application. Key modifications include the refactoring of the Changes
Possibly related PRs
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (4)
app/packages/app/src/useEvents/useSetGroupSlice.ts (1)
18-22: Consider enhancing type safety for the event identifier.The addition of the event identifier improves routing event tracking. However, consider using a const enum or union type for the event string to prevent typos and improve maintainability.
Consider defining an enum or union type for events:
export type RouterEvent = "slice" | "modal" | /* other events */; // Then use it in LocationState interface LocationState { event?: RouterEvent; groupSlice?: string; // ... other properties }e2e-pw/src/oss/poms/grid/index.ts (1)
Line range hint
1-143: Consider centralizing event handling.The class already has an EventUtils dependency and methods like
getWaitForGridRefreshPromise. Consider centralizing the page change and grid refresh event handling logic into a dedicated event manager to maintain consistency across the codebase.Consider refactoring to introduce a dedicated event manager:
class GridEventManager { constructor(private readonly eventUtils: EventUtils) {} async waitForPageChange<T>(action: () => Promise<T>): Promise<T> { const gridRefreshPromise = this.getWaitForGridRefreshPromise(); const result = await action(); await gridRefreshPromise; return result; } private getWaitForGridRefreshPromise() { const refreshStartPromise = this.eventUtils.getEventReceivedPromiseForPredicate("grid-unmount"); const refreshEndPromise = this.eventUtils.getEventReceivedPromiseForPredicate("grid-mount"); return Promise.all([refreshStartPromise, refreshEndPromise]); } }app/packages/state/src/recoil/pathData/counts.ts (2)
149-162: Add error handling and documentation to the new selector.While the implementation is functionally correct, consider these improvements:
- Add JSDoc comments explaining the selector's purpose and parameters
- Add input validation for
ftypeandembeddedDocType- Consider memoizing the Set conversion result
Here's the suggested implementation:
+/** + * Selector that returns a unique set of paths based on field type and optional embedded document type. + * @param ftype - The field type(s) to gather paths for + * @param embeddedDocType - Optional embedded document type(s) to filter paths + * @returns Array of unique paths + */ export const gatheredPaths = selectorFamily({ key: "gatheredPaths", get: ({ embeddedDocType, ftype, }: { embeddedDocType?: string | string[]; ftype: string | string[]; }) => ({ get }) => { + if (!ftype) { + throw new Error("ftype is required"); + } return [...new Set(gatherPaths(get, ftype, embeddedDocType))]; }, });
Line range hint
178-190: Consider optimizing the reducer for performance.The current implementation is correct, but could be optimized by:
- Early termination if no paths are found
- Using Object.entries for more efficient iteration
Here's the suggested optimization:
- return get(gatheredPaths({ ftype, embeddedDocType })).reduce( - (result, path) => { - const data = get(counts({ extended, modal, path: `${path}.${key}` })); - for (const value in data) { - if (!result[value]) { - result[value] = 0; - } - - result[value] += data[value]; - } - return result; - }, - {} - ); + const paths = get(gatheredPaths({ ftype, embeddedDocType })); + if (paths.length === 0) return {}; + + return paths.reduce((result, path) => { + const data = get(counts({ extended, modal, path: `${path}.${key}` })); + Object.entries(data).forEach(([value, count]) => { + result[value] = (result[value] || 0) + count; + }); + return result; + }, {});
📜 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: Type extension aligns with routing optimization goals.
The addition of "slice" as a valid event type is a clean, non-breaking change that supports the modal routing optimization objectives. The type remains properly exported and maintains type safety for downstream consumers.
Let's verify the usage of this new event type across the codebase:
✅ Verification successful
Type extension for "slice" event is properly integrated and used consistently
The verification shows that the "slice" event type is well-integrated into the codebase:
- Used in router state management (
app/packages/app/src/routing/RouterContext.ts) - Properly implemented in group slice management (
app/packages/app/src/useEvents/useSetGroupSlice.ts) - Consistently used with
router.pushcalls with the correct state shape - Handled appropriately in event skipping logic (
SKIP_EVENTSset in RouterContext)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usages of the new "slice" event type
# and ensure it's being used consistently with the routing system
# Search for direct usage of the "slice" event string in routing-related code
rg -t ts -t tsx '"slice".*event' app/
# Look for router.push calls that might use this event type
ast-grep --pattern 'router.push({ $$$, event: "slice" })'
Length of output: 127
Script:
#!/bin/bash
# Let's try a different approach to find usages
# Search for "slice" in TypeScript files
rg -t typescript '"slice"' app/
# Search for any router.push calls to understand usage patterns
ast-grep --pattern 'router.push('
# Look for event type usage in state management
rg -t typescript 'event:' app/
# Search for LocationState type references
rg -t typescript 'LocationState' app/
Length of output: 243
Script:
#!/bin/bash
# Correcting the file type flags for ripgrep and trying more specific searches
# Search for "slice" in .ts and .tsx files
rg '"slice"' -g '*.ts' -g '*.tsx' app/
# Look for event type usage in state management
rg 'event:' -g '*.ts' -g '*.tsx' app/
# Search for LocationState type references
rg 'LocationState' -g '*.ts' -g '*.tsx' app/
# Look for router usage patterns
rg 'router\.push' -g '*.ts' -g '*.tsx' app/
Length of output: 8885
app/packages/core/src/components/Grid/useRefreshers.ts (1)
66-67: Verify the removal of modal event handling.
The simplified event handling now only checks for "fieldVisibility" events, with all other events triggering a grid location reset. While this aligns with the modal routing optimization goal, please verify that removing the modal-specific checks won't cause unintended side effects.
Consider adding a comment explaining the event handling logic:
subscribe(({ event }, { reset }) => {
+ // Skip grid location reset for field visibility changes only
if (event === "fieldVisibility") return;
// if not a modal page change, reset the grid locationLet's verify the modal event handling changes:
e2e-pw/src/oss/poms/grid/index.ts (2)
55-55: Verify event handling after pageChange removal.
The removal of pageChange wrapper simplifies the control flow, but we should verify that page state transitions are still properly handled.
#!/bin/bash
# Search for other page change event handlers that might be affected
rg -A 3 "pageChange|grid-mount|grid-unmount" 81-81: Verify slice selection state management.
The removal of pageChange wrapper from selectSlice aligns with the modal routing optimization, but we should ensure slice selection state is properly managed.
✅ Verification successful
Slice selection state management is properly implemented
The verification shows that slice selection is correctly managed through a well-structured implementation:
GridSliceSelectorPomhandles the slice selection UI interactions- The selection state changes are properly tracked through test assertions
- Usage patterns in test specs demonstrate consistent state verification after slice selection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for slice selection related event handling
ast-grep --pattern 'selectSlice($$$)'
Length of output: 133
Script:
#!/bin/bash
# Search for slice selection related event handling with correct syntax
ast-grep --pattern $'selectSlice($$$)'
# Search for slice selector implementation
rg -A 5 "class.*SliceSelector"
# Search for slice selection state management
rg -A 3 "selectSlice"
Length of output: 3920
e2e-pw/src/oss/poms/modal/index.ts (1)
121-123: LGTM! Clean and efficient navigation implementation.
The direct click implementation with proper waiting mechanism is more maintainable and follows Playwright's best practices.
app/packages/app/src/routing/RouterContext.ts (5)
72-73: Initialization of getEntryResource is correct
The getEntryResource function is properly initialized using makeGetEntryResource<T>().
85-104: Error handling in update function is appropriate
The try-catch block correctly handles resource reuse by catching thrown Resource instances and returning early. This ensures that page changes are skipped when necessary.
150-150: Correct usage of getEntryResource
The getEntryResource function is called with appropriate parameters, ensuring that the current entry resource is updated correctly.
Also applies to: 153-156
186-187: Definition of SKIP_EVENTS is appropriate
The use of a Set for SKIP_EVENTS allows efficient lookup of events that should skip page changes, improving performance.
192-201: Verify logic in isReusable function
The isReusable function returns true if either the current or new location's event is in SKIP_EVENTS. Please verify that this is the intended behavior, as it might cause resources to be reused when only one of the events matches.
| router.push(pathname, { | ||
| ...router.location.state, | ||
| event: "slice", | ||
| groupSlice: slice, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a constant or enum for the event type.
While the implementation is correct, the hardcoded "slice" string could be moved to a shared constant or enum to improve maintainability and type safety across the codebase.
Consider creating a shared events enum:
// routerEvents.ts
export enum RouterEvent {
SLICE = "slice"
}Then update the router push:
router.push(pathname, {
...router.location.state,
- event: "slice",
+ event: RouterEvent.SLICE,
groupSlice: slice,
});| await modal.close(); | ||
| await modal.assert.isClosed(); | ||
| await grid.assert.isSelectionCountEqualTo(1); |
There was a problem hiding this comment.
💡 Codebase verification
Found one remaining URL-based navigation that needs updating
There's still one instance of url.back() in e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts that should be updated to use modal.close() for consistency with the new modal interaction pattern. All other modal-related tests are already using the new approach correctly.
e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts: Replaceawait grid.url.back();withawait modal.close();
🔗 Analysis chain
LGTM: Modal closure implementation simplified
The change from URL-based navigation to direct modal closure aligns with the simplified modal interaction model. The test properly verifies both the modal closure and persistence of selection state.
Let's verify other modal-related tests are consistent with this pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining URL-based modal navigation
# Expect: No occurrences of url.back() in modal-related tests
# Search for potentially inconsistent modal navigation patterns
rg -l "modal.*url\.back|url\.back.*modal" "e2e-pw/src/oss/specs/"
# Search for the new pattern to ensure consistency
rg -l "modal\.close" "e2e-pw/src/oss/specs/"
Length of output: 499
Script:
#!/bin/bash
# Let's check the actual implementation of modal.close() in these files
# to ensure they're using the new pattern consistently
# First, let's look for any remaining url.back() calls in these files
rg "url\.back\(\)" e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts e2e-pw/src/oss/specs/selection.spec.ts e2e-pw/src/oss/specs/regression-tests/group-video/default-video-slice-group.spec.ts e2e-pw/src/oss/specs/groups/sparse-groups.spec.ts e2e-pw/src/oss/specs/groups/sparse-groups-scenes.spec.ts e2e-pw/src/oss/specs/groups/sparse-dynamic-groups.spec.ts e2e-pw/src/oss/specs/groups/large-groups.spec.ts
# Now, let's see the context around modal.close() usage
rg -B2 -A2 "modal\.close\(\)" e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts e2e-pw/src/oss/specs/selection.spec.ts e2e-pw/src/oss/specs/regression-tests/group-video/default-video-slice-group.spec.ts e2e-pw/src/oss/specs/groups/sparse-groups.spec.ts e2e-pw/src/oss/specs/groups/sparse-groups-scenes.spec.ts e2e-pw/src/oss/specs/groups/sparse-dynamic-groups.spec.ts e2e-pw/src/oss/specs/groups/large-groups.spec.ts
Length of output: 5393
| await this.url.pageChange(() => | ||
| this.page.click("body", { position: { x: 0, y: 0 } }) | ||
| ); | ||
| await this.page.click("body", { position: { x: 0, y: 0 } }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the modal closing more robust.
While clicking at (0,0) works, it might be fragile if the viewport is scrolled or if there are fixed position elements at that coordinate. Consider using a more reliable approach.
- await this.page.click("body", { position: { x: 0, y: 0 } });
+ // Find a guaranteed empty space or use modal's close button if available
+ const modalBounds = await this.modalContainer.boundingBox();
+ await this.page.mouse.click(
+ modalBounds.x - 10, // Click just outside the modal
+ modalBounds.y + modalBounds.height / 2
+ );Committable suggestion was skipped due to low confidence.
| 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 refactoring control flow instead of throwing currentResource
Throwing currentResource to manage control flow can be confusing and may complicate error handling. Consider refactoring to return early or use a different pattern to indicate resource reuse.
Apply this diff to refactor the control flow:
if (isReusable(location)) {
- // throw the current resource (page) if it can be reused
- throw currentResource;
+ // return the current resource for reuse
+ return currentResource;
}Committable suggestion was skipped due to low confidence.
| let resolveEntry: (entry: Entry<T>) => void; | ||
| const promise = new Promise<Entry<T>>((resolve) => { | ||
| resolveEntry = resolve; | ||
| }); | ||
| const subscription = fetchQuery( | ||
| environment, | ||
| concreteRequest, | ||
| matchResult.variables || {}, | ||
| { fetchPolicy } | ||
| ).subscribe({ | ||
| next: (data) => { | ||
| const { state: _, ...rest } = location; | ||
| resolveEntry({ | ||
| state: matchResult.variables as LocationState<T>, | ||
| ...rest, | ||
| component, | ||
| data, | ||
| concreteRequest, | ||
| preloadedQuery, | ||
| cleanup: () => { | ||
| subscription?.unsubscribe(); | ||
| }, | ||
| }); | ||
| }, | ||
| error: (error) => handleError?.(error), | ||
| }); | ||
|
|
||
| return promise; |
There was a problem hiding this comment.
Handle promise rejection in error callback of fetchQuery
In the error callback of fetchQuery, the promise created for currentResource is neither resolved nor rejected. This may cause the promise to remain pending indefinitely, leading to potential memory leaks or unhandled promise rejections.
Apply this diff to reject the promise in the error callback:
let resolveEntry: (entry: Entry<T>) => void;
+ let rejectEntry: (error: unknown) => void;
const promise = new Promise<Entry<T>>((resolve, reject) => {
resolveEntry = resolve;
+ rejectEntry = reject;
});
const subscription = fetchQuery(
environment,
concreteRequest,
matchResult.variables || {},
{ fetchPolicy }
).subscribe({
next: (data) => {
const { state: _, ...rest } = location;
resolveEntry({
state: matchResult.variables as LocationState<T>,
...rest,
component,
data,
concreteRequest,
preloadedQuery,
cleanup: () => {
subscription?.unsubscribe();
},
});
},
error: (error) => {
+ rejectEntry(error);
handleError?.(error);
},
});📝 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.
| let resolveEntry: (entry: Entry<T>) => void; | |
| const promise = new Promise<Entry<T>>((resolve) => { | |
| resolveEntry = resolve; | |
| }); | |
| const subscription = fetchQuery( | |
| environment, | |
| concreteRequest, | |
| matchResult.variables || {}, | |
| { fetchPolicy } | |
| ).subscribe({ | |
| next: (data) => { | |
| const { state: _, ...rest } = location; | |
| resolveEntry({ | |
| state: matchResult.variables as LocationState<T>, | |
| ...rest, | |
| component, | |
| data, | |
| concreteRequest, | |
| preloadedQuery, | |
| cleanup: () => { | |
| subscription?.unsubscribe(); | |
| }, | |
| }); | |
| }, | |
| error: (error) => handleError?.(error), | |
| }); | |
| return promise; | |
| let resolveEntry: (entry: Entry<T>) => void; | |
| let rejectEntry: (error: unknown) => void; | |
| const promise = new Promise<Entry<T>>((resolve, reject) => { | |
| resolveEntry = resolve; | |
| rejectEntry = reject; | |
| }); | |
| const subscription = fetchQuery( | |
| environment, | |
| concreteRequest, | |
| matchResult.variables || {}, | |
| { fetchPolicy } | |
| ).subscribe({ | |
| next: (data) => { | |
| const { state: _, ...rest } = location; | |
| resolveEntry({ | |
| state: matchResult.variables as LocationState<T>, | |
| ...rest, | |
| component, | |
| data, | |
| concreteRequest, | |
| preloadedQuery, | |
| cleanup: () => { | |
| subscription?.unsubscribe(); | |
| }, | |
| }); | |
| }, | |
| error: (error) => { | |
| rejectEntry(error); | |
| handleError?.(error); | |
| }, | |
| }); | |
| return promise; |
Adding #4957 to release/v1.0.2
Summary by CodeRabbit
gatheredPaths, for enhanced path management.eventproperty for better event tracking.useRefreshersfunction.