Re-implement Server Action reducer (#86367)#9
Open
MitchLewis930 wants to merge 1 commit intopr_039_beforefrom
Open
Re-implement Server Action reducer (#86367)#9MitchLewis930 wants to merge 1 commit intopr_039_beforefrom
MitchLewis930 wants to merge 1 commit intopr_039_beforefrom
Conversation
Rewrite of the Server Action reducer to use the PPR/Segment Cache navigation implementation, rather than the old lazy fetch implementation. Server Actions may trigger a revalidation, a redirect, or both. They may also invalidate the cache. The behavior could be naively implemented using router.refresh() and router.push(). Semantically, the routing behavior is equivalent. The main difference is that the server that invokes the action may also send back new data for the page within the same response. Compared to a separate request, this data is more likely to be consistent with any data that may have been mutated by the action, due to global data propagation races. (It's also faster since it avoids an extra server waterfall.) So, navigations initiated by a Server action must be able to "seed" the navigation with the data it just received from the server. I've added a new internal method, navigateToSeededRoute, that implements this behavior.
There was a problem hiding this comment.
Pull request overview
This PR re-implements the Server Action reducer to align with the new segment cache navigation architecture. The implementation unifies action handling with regular navigation flows, treating Server Actions as seeded navigations that optionally include rendered data from the server response.
Changes:
- Added
renderedSearchandcouldBeInterceptedfields to action and navigation flight responses - Refactored server action reducer to use the new
navigateToSeededRoutenavigation flow - Renamed
refreshtonavigateToSeededRouteto better reflect its general-purpose nature - Updated navigation functions to accept
shouldRefreshDynamicDataparameter
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/next/src/shared/lib/app-router-types.ts | Added q (renderedSearch) and i (couldBeIntercepted) fields to flight response types |
| packages/next/src/server/app-render/app-render.tsx | Included renderedSearch and couldBeIntercepted in RSC payload generation |
| packages/next/src/client/components/segment-cache/navigation.ts | Refactored refresh function to navigateToSeededRoute with broader applicability |
| packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts | Replaced custom action handling logic with unified navigation flows |
| packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts | Updated to use navigateToSeededRoute with explicit parameters |
| packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts | Added shouldRefreshDynamicData parameter to navigation call |
| packages/next/src/client/components/router-reducer/ppr-navigations.ts | Added seedData and seedHead parameters throughout navigation functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // since we keep them in sync anyway, but having two sources of truth can | ||
| // lead to subtle bugs and race conditions. | ||
| href === window.location.href | ||
| const isSamePageNavigation = href === currentUrl.href |
There was a problem hiding this comment.
The comparison uses href which is undefined at this point. The code should use url.href instead of href since the href variable hasn't been extracted from the url object yet.
Comment on lines
+74
to
+76
| // TODO: Refactor to be a discriminated union. Or just get rid of it; | ||
| // fetchServerAction only has one caller, no reason this intermediate type has | ||
| // to exist. |
There was a problem hiding this comment.
This TODO comment appears to be new code but suggests removing or refactoring the type immediately below it. Consider addressing this before merging, as the comment indicates the current design may be suboptimal.
Suggested change
| // TODO: Refactor to be a discriminated union. Or just get rid of it; | |
| // fetchServerAction only has one caller, no reason this intermediate type has | |
| // to exist. |
Comment on lines
+495
to
+505
| // Used to request all the dynamic data for a route, rather than just a subset, | ||
| // e.g. during a refresh or a revalidation. Typically this gets constructed | ||
| // during the normal flow when diffing the route tree, but for an unprefetched | ||
| // navigation, where we don't know the structure of the target route, we use | ||
| // this instead. | ||
| const DynamicRequestTreeForEntireRoute: FlightRouterState = [ | ||
| '', | ||
| {}, | ||
| null, | ||
| 'refetch', | ||
| ] |
There was a problem hiding this comment.
The constant name DynamicRequestTreeForEntireRoute is quite verbose. Consider shortening to FULL_DYNAMIC_REQUEST_TREE or REFETCH_TREE to improve readability while maintaining clarity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR_039