feat(sistent): refactor pagination to use dynamic GraphQL routing and remove content.js#7587
feat(sistent): refactor pagination to use dynamic GraphQL routing and remove content.js#7587rishiraj38 wants to merge 5 commits intolayer5io:masterfrom
Conversation
Signed-off-by: Rishi Raj <rishiraj438gt@gmail.com>
YASHMAHAKAL
left a comment
YASHMAHAKAL
left a comment
There was a problem hiding this comment.
Looks good @rishiraj38
|
Solid refactoring, @rishiraj38. Replacing the hardcoded One coordination item to flag: #7582 (Card component docs) added entries to Also worth confirming:
Good work eliminating that technical debt. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors Sistent pagination to derive component/tab routes dynamically from Sistent MDX nodes via Gatsby useStaticQuery, removing the large manually-maintained content.js list.
Changes:
- Replace static
content.jspagination data with a GraphQLallMdxquery + derived route list. - Filter pagination to only include pages for components whose overview page is
published: true, and order tabs (overview → guidance → code). - Remove
src/components/SistentNavigation/content.jsand inline remaining stable routes inpagination.js.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/components/SistentNavigation/pagination.js | Adds useStaticQuery and runtime route derivation/sorting for pagination, plus a small stable-route prefix list. |
| src/components/SistentNavigation/content.js | Deletes the hardcoded pagination array previously used by Sistent pagination. |
| const fullContentArray = [...STABLE_ROUTES, ...dynamicRoutes]; | ||
|
|
||
| useEffect(() => { | ||
| const path = window.location.pathname; | ||
| const index = content.findIndex((x) => x.link === path); | ||
| // Handle trajectory slashes | ||
| const cleanPath = path.endsWith("/") && path.length > 1 ? path.slice(0, -1) : path; | ||
| const index = fullContentArray.findIndex((x) => x.link === cleanPath); | ||
| setCurrentPage(index); | ||
| }, []); | ||
| }, [fullContentArray]); |
There was a problem hiding this comment.
fullContentArray is rebuilt on every render, so the effect will re-run on every render as well. Even if setCurrentPage bails out when the value is unchanged, this still adds unnecessary work and can become noticeable as the MDX node count grows. Suggestion (mandatory): memoize dynamicRoutes and fullContentArray with useMemo (based on data.allMdx.nodes) so the dependency is stable, or change the effect dependency to something stable (e.g., memoized array + pathname).
| ); | ||
| }); | ||
|
|
||
| const fullContentArray = [...STABLE_ROUTES, ...dynamicRoutes]; |
There was a problem hiding this comment.
Merging STABLE_ROUTES and dynamicRoutes without de-duplication can produce duplicate entries if any of the “stable” pages are also present in the MDX collection (same link). This can break findIndex (it will pick the first match) and can cause incorrect previous/next navigation due to duplicated items in the array. Suggestion (mandatory): de-duplicate by link when building fullContentArray (e.g., build a map/set keyed by link and preserve the desired precedence/order).
| { link: "/projects/sistent/getting-started/about", text: "About" }, | ||
| { link: "/projects/sistent/getting-started/installation", text: "Installation" }, |
There was a problem hiding this comment.
Items in fullContentArray have inconsistent shapes: stable routes use text, while dynamic routes use name (and include extra fields). This makes the merged array harder to reason about and increases the chance of future regressions if UI later starts using the label field. Suggestion (mandatory): normalize to a single label key (e.g., text) across both stable and dynamic routes before merging.
| .map((node) => ({ | ||
| componentSlug: node.frontmatter.component, | ||
| name: node.frontmatter.name || node.frontmatter.component, | ||
| link: node.fields.slug, | ||
| pageType: node.fields.pageType, | ||
| })) |
There was a problem hiding this comment.
Items in fullContentArray have inconsistent shapes: stable routes use text, while dynamic routes use name (and include extra fields). This makes the merged array harder to reason about and increases the chance of future regressions if UI later starts using the label field. Suggestion (mandatory): normalize to a single label key (e.g., text) across both stable and dynamic routes before merging.
| useEffect(() => { | ||
| const path = window.location.pathname; | ||
| const index = content.findIndex((x) => x.link === path); | ||
| // Handle trajectory slashes |
There was a problem hiding this comment.
The comment says 'trajectory slashes' but it looks like this is handling trailing slashes. Consider correcting the wording for clarity.
| {currentPage > 0 ? ( | ||
| <Button $secondary $url={content[currentPage - 1]?.link}> | ||
| <Button $secondary $url={fullContentArray[currentPage - 1]?.link}> | ||
| ← Previous | ||
| </Button> | ||
| ) : null} | ||
| ) : <div />} | ||
|
|
||
| {currentPage < content.length - 1 ? ( | ||
| <Button $primary $url={content[currentPage + 1]?.link}> | ||
| {currentPage !== -1 && currentPage < fullContentArray.length - 1 ? ( | ||
| <Button $primary $url={fullContentArray[currentPage + 1]?.link}> | ||
| Next → | ||
| </Button> | ||
| ) : null} | ||
| ) : <div />} |
There was a problem hiding this comment.
Rendering empty <div /> placeholders can add extra non-semantic nodes to the DOM; depending on styling and assistive tech, these can be confusing or at least unnecessary noise. Suggestion (optional): either render null and handle spacing with CSS in PaginationWrapper, or mark placeholders as presentational (e.g., add aria-hidden="true" and ensure they don’t get focus).
Description
This PR fixes #7585 by replacing the hardcoded
content.jsarray with a dynamic GraphQLuseStaticQueryto automatically handle Sistent pagination.This ensures that any newly added component or tab (overview, guidance, code) is instantly indexed and correctly ordered in the pagination workflow without requiring developers to manually update boilerplate arrays.
Changes Made
src/components/SistentNavigation/content.jsto remove technical debt.SistentPaginationto dynamically fetch all Sistent MDX nodes.published: false), while successfully pulling grouped tabs (overview->guidance->code) belonging to published components.componentSlugto guarantee proper alphabetical sequencing.Signed commits