Refactor entity routes to use Next.js layouts and granular Suspense boundaries#566
Refactor entity routes to use Next.js layouts and granular Suspense boundaries#566
Conversation
…o handle related content
…Suspense boundary
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 13. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMoves route-specific fetching, validation, and redirect logic from page components into new async App Router layouts for DOI, ORCID, ROR, and Repository routes; pages now render only RelatedContent. Adds ROR ID validator, a DOI search params mapper, and threads facets-loading into repository related-content rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant DOI_Layout as "DOI Layout"
participant Crossref as "Crossref API"
participant DOIService as "DOI data service"
Browser->>DOI_Layout: request /doi/...
DOI_Layout->>DOI_Layout: decode DOI
alt DOI starts with 10.13039
DOI_Layout->>Crossref: fetchCrossrefFunder(doi)
Crossref-->>DOI_Layout: funder data / none
alt funder data exists
DOI_Layout->>Browser: redirect to derived ROR URL
else
DOI_Layout->>DOI_Layout: notFound()
end
end
DOI_Layout->>DOIService: fetchDoi(doi)
DOIService-->>DOI_Layout: doi data / none
alt doi data missing
DOI_Layout->>DOI_Layout: notFound()
else
DOI_Layout->>Browser: render JSON‑LD + Suspense(Content, RelatedAggregateGraph) + children
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/(main)/repositories/[...repoid]/RelatedContent.tsx (1)
54-76:⚠️ Potential issue | 🟡 MinorNested
<Container fluid>— this component will be wrapped by a second fluid container from layout.tsx.The new
layout.tsxwraps{props.children}in<Container fluid>(layout.tsx Line 26), and this component also renders its own<Container fluid>at Line 54. This produces nested fluid containers in the DOM, which may cause unintended padding/margin behavior with react-bootstrap's grid system.Consider removing the outer
<Container fluid>from either this component or the layout to avoid the nesting.src/app/(main)/repositories/[...repoid]/page.tsx (1)
50-61:⚠️ Potential issue | 🟡 MinorPotential null dereference on
data.repository.Line 60 accesses
data.repository.clientIdwithout verifying thatdata.repositoryis defined. IffetchRepositoryreturns{ data }wheredatais truthy butdata.repositoryisundefinedornull, this will throw at runtime.Proposed defensive check
- if (!data) return null + if (!data?.repository) return null
🤖 Fix all issues with AI agents
In `@src/app/`(main)/doi.org/[...doi]/layout.tsx:
- Line 47: Guard the <Script> rendering by checking data.work.schemaOrg before
outputting it: only render the Script element when data?.work?.schemaOrg is
defined/truthy (or convert objects to a JSON string via JSON.stringify before
passing) so you don't emit empty/invalid JSON-LD; update the JSX where the
Script tag is created (the element using data.work.schemaOrg) to conditionally
render it or provide a safe string fallback and keep the existing id and type
attributes.
- Around line 24-29: The code calls
redirect(`/ror.org${rorFromUrl(data.organization.id)}`) without guarding against
data.organization or data.organization.id being null; update the
doi.startsWith('10.13039') branch (around fetchCrossrefFunder and redirect) to
check that data && data.organization && data.organization.id are present before
calling rorFromUrl/redirect, and if not present call notFound() (or otherwise
bail) to avoid a TypeError; reference the fetchCrossrefFunder call,
data.organization.id, rorFromUrl, notFound, and redirect when making the change.
In `@src/app/`(main)/repositories/[...repoid]/layout.tsx:
- Around line 20-23: The code currently only destructures { data } from
fetchRepository(repoid) and treats any missing data as a 404 via notFound(),
which hides API errors; instead destructure both { data, error } from
fetchRepository(repoid), explicitly check if error is present and handle it
(e.g., throw the error or call Next.js error handling) so API failures
(500/network) surface correctly, and only call notFound() when error is absent
but data is falsy or when the API indicates a 404; update the layout.tsx block
that calls fetchRepository to perform this explicit error check and branch.
In `@src/utils/ror.ts`:
- Line 1: The regex ROR_ID_PATTERN currently contains a literal pipe inside the
character class which incorrectly allows '|' characters (const ROR_ID_PATTERN =
/^0[a-hj-km-np-tv-z|0-9]{6}[0-9]{2}$/); remove the '|' so the class is
[a-hj-km-np-tv-z0-9] and update ROR_ID_PATTERN to
/^0[a-hj-km-np-tv-z0-9]{6}[0-9]{2}$/ (also adjust any uses like isValidRORId to
rely on the corrected constant).
🧹 Nitpick comments (8)
src/app/(main)/ror.org/[rorid]/layout.tsx (1)
1-3: Nit: consolidate React imports into a single statement.Lines 1 and 3 both import from
'react'.♻️ Proposed fix
-import React, { Suspense } from 'react' -import { notFound } from 'next/navigation' -import { PropsWithChildren } from 'react' +import React, { Suspense, PropsWithChildren } from 'react' +import { notFound } from 'next/navigation'src/app/(main)/repositories/[...repoid]/page.tsx (1)
54-56: Duplicate fetch is safe but consider a clarifying comment.Both
layout.tsxandpage.tsxcallfetchRepository(repoid). This works because Next.js deduplicatesfetch()calls with identical URLs during the same server render pass, so no extra network request is made. The existing comment on Line 54-55 partially explains this, but you might want to mention the deduplication guarantee for future maintainers.src/app/(main)/orcid.org/[orcid]/layout.tsx (2)
14-31: Layout structure looks good overall. The redirect logic and Suspense boundary are well-placed for this refactor.One minor nit on Line 19: prefer strict equality (
!==) over loose equality (!=).- if (params.orcid != orcid) + if (params.orcid !== orcid)
22-31: Add an error boundary (error.tsx) for this route segment.The Suspense block handles async suspension, but errors thrown during
Contentrendering will propagate upward. Currently only a global error boundary exists (global-error.tsx). Addsrc/app/(main)/orcid.org/[orcid]/error.tsxto catch and gracefully handle rendering errors specific to this route segment.src/app/(main)/doi.org/[...doi]/layout.tsx (2)
38-44: Second<Suspense>has no fallback.Line 42 wraps
RelatedAggregateGraphin a<Suspense>without afallbackprop. While this is valid (React renders nothing during suspension), it may result in a jarring layout shift when the graph loads in. Consider whether a skeleton or<Loading />fallback would be appropriate here for consistency with line 38.
31-34: Pass fetched DOI data from layout toContentto avoid duplicate fetch.The layout fetches DOI data at line 32 but only passes the
doistring toContent(line 39).Contentindependently callsfetchDoi(doi)again at line 25, resulting in two fetches for the same DOI. SincefetchConditionalCacheuses separate fetch calls with different cache options, Next.js request deduplication may not apply, causing unnecessary overhead.Pass the fetched
dataas a prop toContentto reuse the result, or ensure the caching layer deduplicates at the request level.src/app/(main)/doi.org/[...doi]/page.tsx (2)
65-76: Dead code:paramsanddoiare decoded but unused.Since
RelatedContentreads the DOI fromuseParams()internally, the param decoding on lines 66-67 serves no purpose. Simplify the component.♻️ Proposed simplification
-export default async function Page(props: Props) { - const params = await props.params; - const doi = decodeURIComponent(params.doi.join('/')) - - // Redirect handling moved to layout.tsx - // Content rendering moved to layout.tsx - // RelatedAggregateGraph moved to layout.tsx - // Script tag moved to layout.tsx - +export default function Page() { return ( <RelatedContent /> ) }
1-7: Unused import:Reactis no longer needed.With the JSX transform and the simplified
Pagecomponent, the explicitReactimport on line 1 is unnecessary.
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
content-in-layout
|
| Run status |
|
| Run duration | 03m 28s |
| Commit |
|
| Committer | Joseph Rhoads |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
3
|
Skipped
|
0
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/`(main)/doi.org/[...doi]/layout.tsx:
- Around line 31-35: The layout currently only destructures { data } from
fetchDoi and treats missing data as a 404; instead destructure both { data,
error } from fetchDoi, and if error is present rethrow it (or throw a new Error
with the error message) so server errors surface as 500s, while still calling
notFound() only when data is absent or data.work is missing; update the code
around the fetchDoi call and the subsequent checks (references: fetchDoi, data,
error, notFound) to implement this behavior.
In `@src/app/`(main)/repositories/[...repoid]/page.tsx:
- Around line 58-59: The build is broken because `import` statements (`notFound`
and `Metadata`) were placed inside the `Page` function; move the `notFound`
import to the top-level imports with the other imports, remove the
duplicate/unused `Metadata` import entirely, and delete the inline import
statements from inside the `Page` function so the function body contains no
import declarations; update any references in `Page` to use the top-level
`notFound`.
🧹 Nitpick comments (3)
src/app/(main)/doi.org/[...doi]/layout.tsx (1)
39-57: Layout and Content both fetch the same DOI data independently — use React'scache()to deduplicate.The layout fetches DOI data for validation and
schemaOrg(line 32), and Content independently callsfetchDoi(doi)again. Additionally,page.tsxcallsfetchDoi(doi)ingenerateMetadata(). WhilefetchConditionalCacheuses Next.js HTTP-level caching, this doesn't prevent redundant function calls across components. WrapfetchDoiwith React'scache()to ensure it executes only once per request, or pass the fetched data to Content as a prop.src/app/(main)/repositories/[...repoid]/page.tsx (1)
50-63: DuplicatefetchRepositorycall — acceptable but worth a note.Both
layout.tsxandpage.tsxcallfetchRepository(repoid)for the same ID. Next.js deduplicatesfetch()calls within the same server render pass, so there's no extra network cost. However, a brief comment explaining this would help future maintainers understand the pattern.Also, the layout already guards with
notFound(), so the check on line 61 is redundant (though harmless as a defensive measure).src/app/(main)/repositories/[...repoid]/layout.tsx (1)
18-18: Inconsistentrepoiddecoding between layout and page.This file decodes each path segment individually (
params.repoid.map(decodeURIComponent).join('/')), whilepage.tsxline 52 joins first and then decodes (decodeURIComponent(params.repoid.join('/'))). They're functionally equivalent for typical inputs, but the inconsistency is a maintenance hazard. Consider aligning them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/`(main)/doi.org/page.tsx:
- Around line 11-13: The function getQueryVariables incorrectly annotates its
return as SearchParams while mapSearchParams returns QueryVar; change the return
type of getQueryVariables to QueryVar (or remove the explicit annotation to let
TypeScript infer it) and avoid the blanket "as any" cast by using a narrower
cast (e.g., cast Object.fromEntries(...) to the expected Record<string,string>
or the exact input type expected by mapSearchParams); if you choose to annotate
with QueryVar, import the QueryVar type and update the signature of
getQueryVariables accordingly so downstream variables have the correct camelCase
shape.
🧹 Nitpick comments (1)
src/app/(main)/doi.org/page.tsx (1)
22-24: Minor:query === ''is redundant.
!queryalready coversundefined,null, and''. The explicit=== ''check is unnecessary.Simplification
- if (!query || query === '') { + if (!query) {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/`(main)/doi.org/page.tsx:
- Around line 16-17: This page uses useSearchParams() inside the SearchDoiPage
component and must be rendered inside a React Suspense boundary; wrap the
component's rendered output in a <Suspense fallback={...}> by importing Suspense
from 'react' and returning the existing JSX inside that boundary (or
alternatively add a Suspense around the children in the parent layout component
that renders this page); ensure the fallback is a simple loading UI and keep the
useSearchParams() usage unchanged.
🧹 Nitpick comments (1)
src/app/(main)/doi.org/page.tsx (1)
24-26: Nit:query === ''is redundant after!query.
!queryalready evaluates totrueforundefined,null, and''.Suggested simplification
- if (!query || query === '') { + if (!query) {
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
master
|
| Run status |
|
| Run duration | 01m 49s |
| Commit |
|
| Committer | Joseph Rhoads |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
1
|
|
Pending
|
3
|
|
Skipped
|
0
|
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
2 participants
Purpose
This PR refactors the page structures for DOI, ORCID, ROR, and Repository routes by introducing Next.js layouts. The primary goal is to improve performance and consistency by moving static or shared components (like header content and aggregate graphs) into
layout.tsx, allowing the mainpage.tsxto focus on dynamic related content.Approach
The refactoring relocates data fetching for "header" content and redirect logic from the individual
page.tsxfiles to newlayout.tsxfiles. This leverages Next.js's layout system to keep persistent UI elements stable and allows for more granular use ofSuspensefor loading states.Key Modifications
/doi.org/[...doi]):layout.tsxto handle Crossref Funder redirects and main DOI data fetching.Content,RelatedAggregateGraph, and Schema.org JSON-LD script to the layout.page.tsxto only renderRelatedContent./orcid.org/[orcid]):layout.tsxto handle case-normalization (uppercase) redirects and render theContentcomponent.page.tsxto focus onRelatedContent./repositories/[...repoid]):layout.tsxto fetch repository metadata and wrap content in a commonContainer.RelatedContent.tsxto handlefacetsLoadingin theWorksListing./ror.org/[rorid]):layout.tsxto handle ROR ID validation and primary profile content.isValidRORIdlogic into a shared utility file (src/utils/ror.ts).src/utils/ror.tsfor centralized ROR ID validation logic.Important Technical Details
ContentandRelatedAggregateGraphare now wrapped inSuspensewithin the layouts, ensuring that slow data fetching for one part of the page doesn't block the entire rendering process.Types of changes
Reviewer, please remember our guidelines:
Summary by CodeRabbit
New Features
Refactor