Transition DOI related works to REST API #548
Conversation
Bumps [next](https://github.com/vercel/next.js) from 15.4.8 to 15.4.9. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v15.4.8...v15.4.9) --- updated-dependencies: - dependency-name: next dependency-version: 15.4.9 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [next](https://github.com/vercel/next.js) from 15.4.9 to 15.4.10. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v15.4.9...v15.4.10) --- updated-dependencies: - dependency-name: next dependency-version: 15.4.10 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…ntifier because Crossref DOIs may have citation or other relationships not in Event Data, which confuses relationship counts compared to current method
WalkthroughMoved related-content rendering and related-DOI computation into the DOI Content component, introduced dual-query facet composition and expanded facet/query variable shapes, added tooltip and loading support for facets, and propagated new vars/props through WorksListing and WorkFacets. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Content as DOI Content
participant Compute as relatedDois computation
participant Related as RelatedContent
participant Q1 as useSearchDoiQuery
participant Q2 as useSearchDoiFacetsQuery
participant WL as WorksListing
participant WF as WorkFacets
Content->>Compute: derive relatedDois from work.relatedIdentifiers
Compute-->>Content: relatedDois (deduped, lowercased, ≤150)
Content->>Related: render RelatedContent(work, relatedDois)
Related->>Q1: call useSearchDoiQuery(vars)
Related->>Q2: call useSearchDoiFacetsQuery(vars)
Q1-->>Related: results (works, pagination)
Q2-->>Related: facets (clients, authors, counts)
Related->>Related: merge results + facets → combined QueryData
Related->>WL: render WorksListing(relatedWorks, vars, showMetrics)
WL->>WF: render WorkFacets(vars)
WF-->>WL: return facet lists (with tooltip/loading flags)
WL-->>Related: display works + facets + metrics
Related-->>Content: completed RelatedContent section
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
related-works
|
| Run status |
|
| Run duration | 01m 41s |
| Commit |
|
| Committer | codycooperross |
| 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 ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app/(main)/orcid.org/[orcid]/mapSearchParams.ts (1)
1-13: Add missing property to SearchParams interface.The
SearchParamsinterface is missing the'client-id'property that is accessed on line 27. This reduces type safety and could lead to runtime issues if callers don't provide the expected property.🔎 Proposed fix
export interface SearchParams { filterQuery?: string cursor?: string contributor?: string published?: string "resource-type"?: string language?: string license?: string "field-of-science"?: string "registration-agency"?: string + "client-id"?: string isBot: string }src/app/(main)/ror.org/[rorid]/mapSearchParams.ts (1)
1-14: Add missing properties to SearchParams interface.The
SearchParamsinterface is missing several properties that are accessed in the function body:'client-id','repository-type', and'organization-relation-type'. This reduces type safety.🔎 Proposed fix
export interface SearchParams { id: string filterQuery?: string cursor?: string contributor?: string published?: string "resource-type"?: string language?: string license?: string "field-of-science"?: string "registration-agency"?: string + "client-id"?: string + "repository-type"?: string + "organization-relation-type"?: string isBot: string }src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
1-15: Add missing 'client-id' property to SearchParams interface.The interface declares
'connection-type'on line 12 but is missing'client-id', which is accessed on line 29. This creates a type safety gap.🔎 Proposed fix
export interface SearchParams { id: string filterQuery?: string cursor?: string contributor?: string published?: string "resource-type"?: string language?: string license?: string "field-of-science"?: string "registration-agency"?: string "connection-type"?: string + "client-id"?: string isBot: string }src/data/queries/doiRelatedContentQuery.ts (1)
45-170: RemoveRELATED_CONTENT_QUERY– it is not used.The
RELATED_CONTENT_QUERYexport (lines 45-170) is dead code. TheuseDoiRelatedContentQueryfunction usesuseSearchDoiQueryanduseSearchDoiFacetsQueryinstead, and no other code in the repository imports or referencesRELATED_CONTENT_QUERYfrom this file.
🧹 Nitpick comments (7)
src/app/(main)/doi.org/[...doi]/Content.tsx (2)
32-41: DOI extraction regex may be too permissive.The regex
(?:https?:\/\/doi\.org\/)?(.+)will match any non-empty string after the optionalhttps://doi.org/prefix. Consider tightening the pattern to validate DOI format (typically10.prefix/suffix).🔎 Suggested improvement
const relatedDois: string[] = Array.from(new Set( relatedIdentifiers ?.filter((identifier) => identifier.relatedIdentifierType === 'DOI') ?.map((identifier) => { - const match = identifier.relatedIdentifier?.match(/(?:https?:\/\/doi\.org\/)?(.+)/) - const doi = match ? match[1] : identifier.relatedIdentifier + const match = identifier.relatedIdentifier?.match(/(?:https?:\/\/doi\.org\/)?(10\..+\/.+)/) + const doi = match ? match[1] : null return doi && doi.toLowerCase() - }) || [] + }) + .filter(Boolean) || [] )).slice(0, 150)
99-103: Suspense boundary lacks a fallback.The
<Suspense>wrapper aroundRelatedAggregateGraphhas no fallback prop, so users will see nothing while the async component loads. Consider adding a loading indicator for better UX, consistent with howRelatedContentshows a<Loading />component.🔎 Suggested fix
<Row> - <Suspense> + <Suspense fallback={<div>Loading connections...</div>}> <RelatedAggregateGraph doi={doi} /> </Suspense> </Row>src/components/InfoTooltip/InfoTooltip.tsx (2)
14-14: Static tooltip ID may cause duplicate ID warnings.Using a static
id="infoTooltip"will create duplicate IDs when multipleInfoTooltipcomponents render on the same page. Consider using React'suseId()hook or passing a unique identifier.🔎 Suggested fix
+import React, { useId } from 'react' -import React from 'react' ... -export const InfoTooltip = ({ text }: InfoTooltipProps) => ( +export const InfoTooltip = ({ text }: InfoTooltipProps) => { + const tooltipId = useId() + return ( <OverlayTrigger placement="top" - overlay={<Tooltip id="infoTooltip">{text}</Tooltip>} + overlay={<Tooltip id={tooltipId}>{text}</Tooltip>} > ... </OverlayTrigger> + ) +}
1-27: Consider consolidating with existing HelpIcon component.There's an existing
HelpIconcomponent atsrc/components/HelpIcon/HelpIcon.tsxwith similar functionality (OverlayTrigger, Tooltip, faQuestionCircle). Consider whether these could be consolidated or ifInfoTooltipcould extend/wrapHelpIconto reduce duplication.src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (1)
66-69: Simplify redundant conditional check forendCursor.The
pageInfoobject is always present inrelatedWorksdue to the fallback on lines 47-51, so the conditional check is unnecessary.🔎 Proposed fix
const hasNextPage = relatedWorks.pageInfo.hasNextPage - const endCursor = relatedWorks.pageInfo - ? relatedWorks.pageInfo.endCursor - : '' + const endCursor = relatedWorks.pageInfo.endCursorsrc/data/queries/searchDoiQuery.ts (1)
39-70: Overall structure is good, but there's a redundant null check.The
buildRelatedToDoiQueryfunction is well-structured. However, line 50'srelatedToDoi &&check is redundant since the function already returns early on line 40 if!relatedToDoi.🔎 Proposed fix
const outwardRelatedDois = relatedDois && relatedDois.length > 0 && '(doi:(' + relatedDois?.map(doi => '"' + doi + '"').join(' OR ') + '))' - const inwardRelatedDois = relatedToDoi && '(relatedIdentifiers.relatedIdentifier:("https://doi.org/' + relatedToDoi?.toLowerCase() + '" OR "' + relatedToDoi?.toLowerCase() + '") AND agency:"datacite")' + const inwardRelatedDois = '(relatedIdentifiers.relatedIdentifier:("https://doi.org/' + relatedToDoi.toLowerCase() + '" OR "' + relatedToDoi.toLowerCase() + '") AND agency:"datacite")'src/components/WorksListing/WorksListing.tsx (1)
20-37: Remove the unusedconnectionTypesCountsprop from the Props interface.The
connectionTypesCountsprop (line 27) is not destructured or used anywhere in the component, and none of the parent components passing props toWorksListinginclude it. Removing it will clean up the interface.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
cypress/e2e/searchWork.test.tspackage.jsonsrc/app/(main)/doi.org/[...doi]/Content.tsxsrc/app/(main)/doi.org/[...doi]/RelatedContent.tsxsrc/app/(main)/doi.org/[...doi]/mapSearchParams.tssrc/app/(main)/doi.org/[...doi]/page.tsxsrc/app/(main)/doi.org/page.tsxsrc/app/(main)/orcid.org/[orcid]/mapSearchParams.tssrc/app/(main)/ror.org/[rorid]/Content.tsxsrc/app/(main)/ror.org/[rorid]/RelatedContent.tsxsrc/app/(main)/ror.org/[rorid]/mapSearchParams.tssrc/app/(main)/ror.org/[rorid]/page.tsxsrc/components/FacetList/FacetList.tsxsrc/components/FacetList/FacetListItem.tsxsrc/components/InfoTooltip/InfoTooltip.tsxsrc/components/MetricsDisplay/MetricsDisplay.module.scsssrc/components/MetricsDisplay/MetricsDisplay.tsxsrc/components/SearchBox/SearchBox.tsxsrc/components/SummarySearchMetrics/SummarySearchMetrics.tsxsrc/components/WorkFacets/WorkFacets.tsxsrc/components/WorksDashboard/WorksDashboard.module.scsssrc/components/WorksDashboard/WorksDashboard.tsxsrc/components/WorksListing/WorksListing.tsxsrc/data/constants.tssrc/data/queries/doiRelatedContentQuery.tssrc/data/queries/searchDoiFacetsQuery.tssrc/data/queries/searchDoiQuery.tssrc/data/types.ts
💤 Files with no reviewable changes (2)
- src/app/(main)/doi.org/[...doi]/page.tsx
- src/components/WorksDashboard/WorksDashboard.module.scss
🧰 Additional context used
🧬 Code graph analysis (15)
package.json (1)
next.config.js (1)
config(56-108)
src/components/SearchBox/SearchBox.tsx (4)
src/components/Header/Search.tsx (2)
search(32-39)Search(11-76)src/app/(main)/ror.org/page.tsx (1)
SearchParams(11-13)src/app/(main)/page.tsx (1)
SearchParams(11-13)src/app/(main)/repositories/[...repoid]/mapSearchParams.ts (1)
mapSearchparams(16-33)
src/components/MetricsDisplay/MetricsDisplay.tsx (2)
src/components/MetricsCounter/MetricsCounter.tsx (1)
MetricsCounter(13-44)src/components/RepositoryDetail/RepositoryDetail.tsx (1)
Metrics(23-33)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (2)
src/app/(main)/repositories/[...repoid]/mapSearchParams.ts (2)
mapSearchparams(16-33)SearchParams(1-14)src/app/(main)/ror.org/page.tsx (1)
SearchParams(11-13)
src/components/SummarySearchMetrics/SummarySearchMetrics.tsx (1)
src/components/MetricsDisplay/MetricsDisplay.tsx (1)
MetricsDisplay(26-65)
src/data/constants.ts (1)
src/data/clients/ror-v2-client.ts (1)
RORFacet(85-89)
src/components/WorkFacets/WorkFacets.tsx (6)
src/data/queries/searchDoiQuery.ts (2)
QueryVar(262-283)useSearchDoiQuery(205-209)src/data/queries/repositoryQuery.ts (1)
QueryVar(137-139)src/data/queries/doiQuery.ts (1)
QueryVar(144-146)src/data/queries/organizationQuery.ts (1)
QueryVar(95-97)src/data/types.ts (1)
Facet(338-344)src/components/FacetList/FacetList.tsx (1)
FacetList(25-52)
src/app/(main)/orcid.org/[orcid]/mapSearchParams.ts (1)
src/app/(main)/repositories/[...repoid]/mapSearchParams.ts (2)
mapSearchparams(16-33)SearchParams(1-14)
src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (3)
src/data/types.ts (2)
Work(19-63)Works(65-93)src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
mapSearchparams(17-35)src/utils/helpers.ts (2)
isDMP(55-57)isProject(59-66)
src/data/queries/searchDoiFacetsQuery.ts (1)
src/data/queries/doiQuery.ts (3)
convertToQueryData(21-25)MetadataQueryData(136-138)QueryData(140-142)
src/components/WorksListing/WorksListing.tsx (3)
src/data/queries/searchDoiQuery.ts (1)
QueryVar(262-283)src/components/SummarySearchMetrics/SummarySearchMetrics.tsx (1)
SummarySearchMetrics(38-48)src/data/queries/searchSummaryQuery.ts (1)
useSearchSummaryCounts(22-30)
src/app/(main)/doi.org/[...doi]/Content.tsx (3)
src/app/(main)/doi.org/[...doi]/RelatedAggregateGraph.tsx (2)
RelatedAggregateGraph(37-84)Props(9-13)src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (1)
RelatedContent(25-95)src/app/(embed)/embed/related-aggregate/[...doi]/page.tsx (1)
EmbedRelatedAggregateGraphPage(8-19)
src/data/types.ts (1)
src/data/queries/repositoryRelatedContentQuery.ts (1)
QueryData(28-30)
src/app/(main)/ror.org/[rorid]/page.tsx (1)
src/app/(main)/ror.org/[rorid]/RelatedContent.tsx (1)
RelatedContent(22-77)
src/app/(main)/ror.org/[rorid]/Content.tsx (2)
src/components/DownloadReports/DownloadReports.tsx (1)
DownloadReports(28-45)src/components/SummarySearchMetrics/SummarySearchMetrics.tsx (1)
SummarySearchMetrics(38-48)
🪛 Biome (2.1.2)
src/components/WorkFacets/WorkFacets.tsx
[error] 54-54: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 79-79: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/app/(main)/doi.org/[...doi]/RelatedContent.tsx
[error] 14-14: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (5)
🔇 Additional comments (45)
src/components/WorksDashboard/WorksDashboard.tsx (1)
44-44: LGTM — Clean migration to Bootstrap utility classes.The replacement of the CSS module class with Bootstrap margin utilities (
mt-5 mb-5) aligns with the broader styling refactor across the codebase. This approach reduces stylesheet dependencies and promotes consistent spacing.src/app/(main)/orcid.org/[orcid]/mapSearchParams.ts (1)
27-27: Property access is type-safe with the interface fix above.Once the
SearchParamsinterface is updated to include'client-id', this line will have proper type checking.cypress/e2e/searchWork.test.ts (1)
32-33: Test correctly reflects new facet ordering.The updated assertions properly account for the addition of a new "Repository" facet at position 7, which shifts "Repository Type" to position 8. This aligns with the broader facet structure changes in the PR.
src/app/(main)/ror.org/[rorid]/mapSearchParams.ts (1)
28-30: Good use of default value for organizationRelationType.The fallback to
'allRelated'provides sensible default behavior when the parameter is not specified.src/components/SearchBox/SearchBox.tsx (2)
20-24: Good refactor to use URLSearchParams.Using
URLSearchParamsfor query string manipulation is more robust than manual string concatenation. This ensures existing parameters are preserved while updating onlyfilterQuery.
39-41: Consistent parameter handling in clear function.The clear operation correctly uses the same
URLSearchParamspattern, ensuring proper query string management.src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
29-30: Variables structure correctly maps both new parameters.The mapping of
clientIdandconnectionTypeinto thevariablesobject is consistent with the refactored data flow across DOI-related components.src/app/(main)/doi.org/page.tsx (1)
24-25: Variables construction aligns with refactored parameter structure.The addition of
clientIdto thevariablesobject is consistent with the broader migration to support client-based filtering across DOI-related queries.src/data/constants.ts (1)
77-77: Addition of 'clients' facet aligns with data model extensions.Adding
'clients'to the default facets is consistent with the broader PR changes that introduce client-based filtering and the corresponding UI components. This enables users to filter works by repository/client.src/components/MetricsDisplay/MetricsDisplay.module.scss (1)
14-14: LGTM!The padding adjustments enhance the visual spacing of the metrics display and align with the conditional rendering behavior introduced in MetricsDisplay.tsx.
Also applies to: 19-19
src/app/(main)/ror.org/[rorid]/page.tsx (2)
7-7: LGTM!The import statement correctly references the RelatedContent component from the same directory.
60-60: LGTM!The RelatedContent component is correctly integrated with the required
rorIdprop.src/data/types.ts (4)
44-44: LGTM!The
relatedIdentifiersfield is correctly added as an optional property, preventing breaking changes while enabling related DOI functionality.
79-79: LGTM!The
clientsfacet field is correctly added to the Works type, supporting the expanded facet functionality introduced in this PR.
328-336: LGTM!The
RelatedIdentifiertype is well-structured with appropriate required and optional fields, aligning with DataCite metadata schema patterns for related identifiers.
342-343: LGTM!The
tooltipTextandloadingfields extend the Facet type appropriately, enabling tooltip support and loading state indicators without breaking existing code.src/components/SummarySearchMetrics/SummarySearchMetrics.tsx (2)
45-45: LGTM!The logic correctly derives
displayWorksTotalbased on the absence oforganizationRelationType, hiding the works metric when viewing organization-related content.
47-47: LGTM!The
displayWorksTotalprop is correctly passed to MetricsDisplay, enabling conditional rendering of the works metric.src/app/(main)/ror.org/[rorid]/Content.tsx (1)
36-71: LGTM!The layout restructuring simplifies the component by removing the RelatedContent integration (now handled at the page level) and streamlining the props passed to child components.
src/data/queries/searchDoiFacetsQuery.ts (1)
38-38: LGTM!The
clientsfacet is correctly added following the established pattern, with appropriate optional chaining and the consistent 10-item limit.src/app/(main)/ror.org/[rorid]/RelatedContent.tsx (4)
23-23: LGTM!The props destructuring correctly matches the updated Props interface with
rorFundingIdsremoved.
31-31: LGTM!The
showMetricsflag is correctly derived from the presence of search parameters, enabling conditional metrics display when filters are applied.
64-66: WorksListing correctly acceptsvarsandshowMetricsprops.The WorksListing component's Props interface (lines 20-37) includes both
vars?: QueryVarandshowMetrics?: booleanas optional props. Both are properly destructured in the function signature and actively used within the component (vars at line 66, showMetrics at line 81). Props are compatible.
28-28: The code is correct as written. TheQueryVarinterface accepts optional parameters (rorId,filterQuery,cursor, etc.), and the vars construction{ rorId, ...variables }properly combines the required identifier with search parameters frommapSearchparams. There is nororFundingIdsfield in the query type definition, and this pattern matches the implementation across all related content components (DOI, ORCID, repositories).Likely an incorrect or invalid review comment.
src/components/MetricsDisplay/MetricsDisplay.tsx (3)
22-23: LGTM!The
displayWorksTotalprop is correctly added as an optional boolean to the Props interface.
26-26: LGTM!The function signature correctly includes
displayWorksTotalwith a default value oftrue, ensuring backward compatibility.
52-52: LGTM!The conditional rendering logic correctly filters out the Works metric when
displayWorksTotalis false, with React handling the null return gracefully.src/app/(main)/doi.org/[...doi]/Content.tsx (1)
57-107: Good structural refactor using Fragment and Suspense.The use of a Fragment to return multiple root elements and wrapping the async
RelatedAggregateGraphin Suspense follows React 19 patterns correctly. PlacingRelatedContentoutside the mainContaineraligns with the component's own container handling.src/components/FacetList/FacetList.tsx (2)
34-34: Good conditional tooltip rendering.The tooltip is properly rendered only when
tooltipTextis provided, avoiding empty tooltips.
46-46: Tooltip propagation to FacetListItem looks correct.Passing
facet.tooltipTextto child items enables per-facet tooltips, aligning with the updatedFacetListIteminterface.src/components/FacetList/FacetListItem.tsx (4)
27-38: Clean loading skeleton implementation.The
LoadingCountcomponent provides appropriate visual feedback with reasonable dimensions matching the count display area.
73-82: Good keyboard accessibility implementation.The
handleKeyDownhandler properly supports Enter and Space keys withpreventDefault()to avoid unwanted scrolling on Space. Combined withrole="button"andtabIndex={0}, this provides proper keyboard navigation.
84-86: Early return for zero-count facets is reasonable.Hiding facets with zero results when not loading improves UX by avoiding clutter. The condition correctly allows showing the facet during loading state.
90-103: Good migration from Link to router-based navigation.Using
router.pushwith{ scroll: false }maintains scroll position during facet selection, improving UX. The div wrapper with proper ARIA attributes provides equivalent accessibility to the previous Link-based approach.src/data/queries/doiRelatedContentQuery.ts (2)
19-37: Dual-query composition approach is sound.The pattern of combining
useSearchDoiQueryfor results anduseSearchDoiFacetsQueryfor metrics/facets allows separation of concerns. The combined loading/error state handling is correct.
29-32: Object spread order overwritestotalCountfrom search results with facets query value.The merge
{...results.data?.works || {}, ...facets.data?.works}causesfacets.data?.works.totalCountto overrideresults.data?.works.totalCount. WhilenodesandpageInfofrom the search query are preserved (the facets query omits them), the total count from the search results is replaced by the facets query's total. Ensure this behavior is intentional, as it means the returnedtotalCountreflects the faceted query results rather than the main search results.src/components/WorksListing/WorksListing.tsx (3)
17-18: LGTM!The new imports for
SummarySearchMetricsandQueryVarare correctly added to support the new functionality.
81-83: LGTM!The conditional rendering of
SummarySearchMetricswith the spread operator handling the optionalvarsprop is correct. The fallback to an empty object{}whenvarsis undefined ensures the component receives valid props.
60-69: LGTM!The
varsprop is correctly passed down toWorkFacets, enabling the new query-based facet functionality.src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (1)
47-53: LGTM!Good defensive coding with the fallback for
relatedWorks. The type assertion toWorksis appropriate given the structure matches the expected type.src/components/WorkFacets/WorkFacets.tsx (2)
121-131: LGTM!The conditional rendering of Connection Types and Organization Relation Types facets based on URL path is appropriate. The logic correctly shows connection types only on DOI pages and organization relation types only on ROR pages.
Also applies to: 133-143
199-219: LGTM!The repository and repository type facets are correctly gated to not appear on
/repositoriespages, avoiding redundant facets. The tooltip text provides helpful context for users.src/data/queries/searchDoiQuery.ts (3)
13-37: LGTM!The refactored
buildOrgQueryfunction with theorganizationRelationTypeswitch provides clean, readable query construction for different organization relationship types. The default case correctly combines all relationship types.
129-130: LGTM!The pageSize handling correctly prioritizes
variables.pageSizeover thecountparameter, and only appends the parameter when a value is defined. This avoids sending undefined page sizes to the API.
262-283: LGTM!The expanded
QueryVarinterface correctly adds the new fields (organizationRelationType,relatedToDoi,relatedDois,connectionType,pageSize) needed to support the REST API transition.
| "linkifyjs": "^4.2.0", | ||
| "maltipoo": "https://github.com/datacite/maltipoo#2.0.3", | ||
| "next": "15.4.8", | ||
| "next": "15.4.10", |
There was a problem hiding this comment.
Upgrade Next.js due to critical security vulnerabilities.
Next.js 15.4.10 is affected by vulnerabilities including CVE-2025-66478 (CVSS 10.0 RCE), CVE-2025-55184 (CVSS 7.5 DoS), and CVE-2025-55183 (CVSS 5.3 source code exposure). Upgrade to next@15.5.9 for the 15.5.x line or later patched version in your release line.
🤖 Prompt for AI Agents
In package.json around line 64, the pinned "next": "15.4.10" is vulnerable;
update it to at least "15.5.9" (or a later patched 15.x release) in
package.json, then run your package manager to update the lockfile (npm install
or yarn install/pnpm install), run the test suite and build to catch breaking
changes, and push the updated package.json and lockfile; also run a dependency
audit (npm audit or equivalent) and ensure CI passes before merging.
| import { Works } from 'src/data/types' | ||
| import { Work, Works } from 'src/data/types' | ||
|
|
||
| import Error from 'src/components/Error/Error' |
There was a problem hiding this comment.
Rename the imported Error component to avoid shadowing the global.
The import shadows the global Error constructor, which can cause confusion and unexpected behavior if the global Error is needed elsewhere in this file.
🔎 Proposed fix
-import Error from 'src/components/Error/Error'
+import ErrorDisplay from 'src/components/Error/Error'And update the usage at line 41:
- <Error title="An error occured loading related content." message={error.message} />
+ <ErrorDisplay title="An error occured loading related content." message={error.message} />Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 14-14: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In src/app/(main)/doi.org/[...doi]/RelatedContent.tsx around line 14, the
imported name "Error" shadows the global Error constructor; rename the import to
a non-conflicting identifier (for example ErrorComponent or DoiError) in the
import statement and update all usages (notably the component usage at line 41)
to the new name so the file no longer masks the global Error.
| </Row> | ||
|
|
||
| if (!data) return | ||
| if ((!data || data.works.totalCount === 0) && !searchParams) return |
There was a problem hiding this comment.
Bug: The !searchParams condition is always falsy.
useSearchParams() always returns a URLSearchParams object (never null or undefined), so !searchParams will always be false. This means the early return will never trigger based on the searchParams check—only when !data || data.works.totalCount === 0.
If the intent is to return early when there are no search parameters set, use searchParams.size === 0 instead.
🔎 Proposed fix
- if ((!data || data.works.totalCount === 0) && !searchParams) return
+ if ((!data || data.works.totalCount === 0) && searchParams.size === 0) returnOr if the searchParams check is not needed:
- if ((!data || data.works.totalCount === 0) && !searchParams) return
+ if (!data || data.works.totalCount === 0) return null📝 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.
| if ((!data || data.works.totalCount === 0) && !searchParams) return | |
| if ((!data || data.works.totalCount === 0) && searchParams.size === 0) return |
🤖 Prompt for AI Agents
In src/app/(main)/doi.org/[...doi]/RelatedContent.tsx around line 45, the
condition uses `!searchParams` which is always false because useSearchParams()
returns a URLSearchParams object; change the check to test for no params (e.g.
`searchParams.size === 0`) or remove the searchParams clause entirely if it
isn’t needed, so the early return accurately triggers when there are no query
parameters or no data.
| //convert camel case to title and make first letter uppercase | ||
| //convert connectionType to title, allRelated becomes All Related Wokrs, references becomes References, citations becomes Citations, parts becomes Parts, partOf becomes Part Of, and otherRelated becomes Other Works | ||
| const displayedConnectionTitle = | ||
| displayedConnectionType === 'allRelated' ? 'All Related Works' : | ||
| displayedConnectionType === 'otherRelated' ? 'Other Works' : | ||
| displayedConnectionType.replace(/([A-Z])/g, ' $1').replace(/^./, str => str.toUpperCase()) | ||
|
|
||
|
|
||
|
|
||
| connectionType === 'allRelated' ? 'All Related Works' : | ||
| connectionType === 'otherRelated' ? 'Other Works' : | ||
| connectionType.replace(/([A-Z])/g, ' $1').replace(/^./, str => str.toUpperCase()) |
There was a problem hiding this comment.
Typo in comment: "Wokrs" should be "Works".
🔎 Proposed fix
//convert camel case to title and make first letter uppercase
- //convert connectionType to title, allRelated becomes All Related Wokrs, references becomes References, citations becomes Citations, parts becomes Parts, partOf becomes Part Of, and otherRelated becomes Other Works
+ //convert connectionType to title, allRelated becomes All Related Works, references becomes References, citations becomes Citations, parts becomes Parts, partOf becomes Part Of, and otherRelated becomes Other Works📝 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.
| //convert camel case to title and make first letter uppercase | |
| //convert connectionType to title, allRelated becomes All Related Wokrs, references becomes References, citations becomes Citations, parts becomes Parts, partOf becomes Part Of, and otherRelated becomes Other Works | |
| const displayedConnectionTitle = | |
| displayedConnectionType === 'allRelated' ? 'All Related Works' : | |
| displayedConnectionType === 'otherRelated' ? 'Other Works' : | |
| displayedConnectionType.replace(/([A-Z])/g, ' $1').replace(/^./, str => str.toUpperCase()) | |
| connectionType === 'allRelated' ? 'All Related Works' : | |
| connectionType === 'otherRelated' ? 'Other Works' : | |
| connectionType.replace(/([A-Z])/g, ' $1').replace(/^./, str => str.toUpperCase()) | |
| //convert camel case to title and make first letter uppercase | |
| //convert connectionType to title, allRelated becomes All Related Works, references becomes References, citations becomes Citations, parts becomes Parts, partOf becomes Part Of, and otherRelated becomes Other Works | |
| const displayedConnectionTitle = | |
| connectionType === 'allRelated' ? 'All Related Works' : | |
| connectionType === 'otherRelated' ? 'Other Works' : | |
| connectionType.replace(/([A-Z])/g, ' $1').replace(/^./, str => str.toUpperCase()) |
🤖 Prompt for AI Agents
In src/app/(main)/doi.org/[...doi]/RelatedContent.tsx around lines 57 to 62,
there is a typo in the comment: change "Wokrs" to "Works" so the comment reads
"...allRelated becomes All Related Works, references becomes References,
citations becomes Citations, parts becomes Parts, partOf becomes Part Of, and
otherRelated becomes Other Works".
| export const InfoTooltip = ({ text }: InfoTooltipProps) => ( | ||
| <OverlayTrigger | ||
| placement="top" | ||
| overlay={<Tooltip id="infoTooltip">{text}</Tooltip>} | ||
| > | ||
| <span | ||
| onClick={(e) => e.stopPropagation()} | ||
| className="ms-2" | ||
| style={{ cursor: 'help' }} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label={text} | ||
| > | ||
| <FontAwesomeIcon icon={faQuestionCircle} aria-hidden="true" /> | ||
| </span> | ||
| </OverlayTrigger> | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Missing keyboard handler for interactive element.
The <span> has role="button" and tabIndex={0}, making it keyboard-focusable, but lacks onKeyDown handler for Enter/Space activation. This is an accessibility gap since keyboard users cannot trigger the tooltip.
🔎 Suggested fix
export const InfoTooltip = ({ text }: InfoTooltipProps) => (
<OverlayTrigger
placement="top"
overlay={<Tooltip id="infoTooltip">{text}</Tooltip>}
>
<span
onClick={(e) => e.stopPropagation()}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.stopPropagation()
+ }
+ }}
className="ms-2"
style={{ cursor: 'help' }}
role="button"
tabIndex={0}
aria-label={text}
>
<FontAwesomeIcon icon={faQuestionCircle} aria-hidden="true" />
</span>
</OverlayTrigger>
)📝 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.
| export const InfoTooltip = ({ text }: InfoTooltipProps) => ( | |
| <OverlayTrigger | |
| placement="top" | |
| overlay={<Tooltip id="infoTooltip">{text}</Tooltip>} | |
| > | |
| <span | |
| onClick={(e) => e.stopPropagation()} | |
| className="ms-2" | |
| style={{ cursor: 'help' }} | |
| role="button" | |
| tabIndex={0} | |
| aria-label={text} | |
| > | |
| <FontAwesomeIcon icon={faQuestionCircle} aria-hidden="true" /> | |
| </span> | |
| </OverlayTrigger> | |
| ) | |
| export const InfoTooltip = ({ text }: InfoTooltipProps) => ( | |
| <OverlayTrigger | |
| placement="top" | |
| overlay={<Tooltip id="infoTooltip">{text}</Tooltip>} | |
| > | |
| <span | |
| onClick={(e) => e.stopPropagation()} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault() | |
| e.currentTarget.click() | |
| } | |
| }} | |
| className="ms-2" | |
| style={{ cursor: 'help' }} | |
| role="button" | |
| tabIndex={0} | |
| aria-label={text} | |
| > | |
| <FontAwesomeIcon icon={faQuestionCircle} aria-hidden="true" /> | |
| </span> | |
| </OverlayTrigger> | |
| ) |
🤖 Prompt for AI Agents
In src/components/InfoTooltip/InfoTooltip.tsx around lines 11-27, the span has
role="button" and tabIndex but no keyboard handler; add an onKeyDown handler
that listens for Enter and Space, calls e.preventDefault() and
e.stopPropagation(), and then triggers the same activation as a click (e.g.,
call e.currentTarget.click() or invoke the click handler) so keyboard users can
open the tooltip with Enter/Space while preserving existing click behavior.
| const useConnectionQuery = (connectionType: string) => | ||
| vars?.relatedToDoi | ||
| ? useSearchDoiQuery({ ...vars, connectionType, pageSize: 0 }) | ||
| : { loading: false, data: undefined, error: undefined } | ||
|
|
||
| const allRelatedQuery = useConnectionQuery('allRelated') | ||
| const referencesQuery = useConnectionQuery('references') | ||
| const citationsQuery = useConnectionQuery('citations') | ||
| const partsQuery = useConnectionQuery('parts') | ||
| const partOfQuery = useConnectionQuery('partOf') | ||
| const versionOfQuery = useConnectionQuery('versionOf') | ||
| const versionsQuery = useConnectionQuery('versions') | ||
| const otherRelatedQuery = useConnectionQuery('otherRelated') | ||
|
|
||
| const connectionTypeList: Facet[] = [ | ||
| { id: 'allRelated', title: 'All', count: allRelatedQuery.data?.works?.totalCount ?? 0, loading: allRelatedQuery.loading }, | ||
| { id: 'references', title: 'References', count: referencesQuery.data?.works?.totalCount ?? 0, loading: referencesQuery.loading }, | ||
| { id: 'citations', title: 'Citations', count: citationsQuery.data?.works?.totalCount ?? 0, loading: citationsQuery.loading }, | ||
| { id: 'parts', title: 'Parts', count: partsQuery.data?.works?.totalCount ?? 0, loading: partsQuery.loading }, | ||
| { id: 'partOf', title: 'Is Part Of', count: partOfQuery.data?.works?.totalCount ?? 0, loading: partOfQuery.loading }, | ||
| { id: 'versionOf', title: 'Version Of', count: versionOfQuery.data?.works?.totalCount ?? 0, loading: versionOfQuery.loading }, | ||
| { id: 'versions', title: 'Versions', count: versionsQuery.data?.works?.totalCount ?? 0, loading: versionsQuery.loading }, | ||
| { id: 'otherRelated', title: 'Other', count: otherRelatedQuery.data?.works?.totalCount ?? 0, loading: otherRelatedQuery.loading } | ||
| ] |
There was a problem hiding this comment.
Critical: Conditional hook calls violate React's Rules of Hooks.
The useConnectionQuery helper conditionally calls useSearchDoiQuery (which internally uses useQuery). This violates React's rule that hooks must be called in the same order on every render. This will cause runtime errors and unpredictable behavior.
The hook should always be called, with the condition moved to the enabled option of the underlying query.
🔎 Proposed fix
Refactor to always call the hook but disable it when not needed. You'll need to modify useSearchDoiQuery in searchDoiQuery.ts to accept an enabled option, or create individual query calls:
- const useConnectionQuery = (connectionType: string) =>
- vars?.relatedToDoi
- ? useSearchDoiQuery({ ...vars, connectionType, pageSize: 0 })
- : { loading: false, data: undefined, error: undefined }
+ const connectionVars = vars?.relatedToDoi ? vars : undefined
+
+ const allRelatedQuery = useSearchDoiQuery({ ...connectionVars, connectionType: 'allRelated', pageSize: 0 }, { enabled: !!connectionVars })
+ const referencesQuery = useSearchDoiQuery({ ...connectionVars, connectionType: 'references', pageSize: 0 }, { enabled: !!connectionVars })
+ // ... etc for other queriesAlternatively, modify useSearchDoiQuery to accept an enabled option:
// In searchDoiQuery.ts
export function useSearchDoiQuery(variables: QueryVar, options?: { enabled?: boolean }) {
const { isPending, data, error } = useQuery({
queryKey: ['doiSearch', variables],
queryFn: async () => fetchDois(variables),
enabled: options?.enabled ?? true
})
return { loading: isPending, data: data?.data, error }
}Then in WorkFacets:
const useConnectionQuery = (connectionType: string) =>
useSearchDoiQuery(
{ ...vars, connectionType, pageSize: 0 },
{ enabled: !!vars?.relatedToDoi }
)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 54-54: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/components/WorkFacets/WorkFacets.tsx around lines 52 to 75, the helper
currently conditionally calls useSearchDoiQuery which violates Rules of Hooks;
always call the hook and move the condition into an enabled option: update
useSearchDoiQuery (in searchDoiQuery.ts) to accept an options param with
enabled?: boolean and pass that to the underlying useQuery (preserving the
returned shape: { loading, data, error }), then change useConnectionQuery here
to always call useSearchDoiQuery with the connectionType variables and set
enabled: !!vars?.relatedToDoi (or equivalent) so hooks run in stable order while
queries are disabled when not needed.
| const useOrganizationRelationQuery = (organizationRelationType: string) => | ||
| vars?.rorId | ||
| ? useSearchDoiQuery({ ...vars, organizationRelationType, pageSize: 0 }) | ||
| : { loading: false, data: undefined, error: undefined } | ||
|
|
||
| const organizationAllRelatedQuery = useOrganizationRelationQuery('allRelated') | ||
| const organizationCreatedOrContributedByAffiliatedResearcherQuery = useOrganizationRelationQuery('createdOrContributedByAffiliatedResearcher') | ||
| const organizationCreatedContributedOrPublishedByQuery = useOrganizationRelationQuery('createdContributedOrPublishedBy') | ||
| const organizationFundedByQuery = useOrganizationRelationQuery('fundedBy') | ||
|
|
||
| const organizationRelationTypeList: Facet[] = [ | ||
| { id: "allRelated", title: "All", count: organizationAllRelatedQuery.data?.works?.totalCount ?? 0, loading: organizationAllRelatedQuery.loading }, | ||
| { id: "createdOrContributedByAffiliatedResearcher", title: "By Affiliated Researchers", tooltipText: "Works created or contributed by researchers affiliated with the organization.", count: organizationCreatedOrContributedByAffiliatedResearcherQuery.data?.works?.totalCount ?? 0, loading: organizationCreatedOrContributedByAffiliatedResearcherQuery.loading }, | ||
| { id: "createdContributedOrPublishedBy", title: "Created By", tooltipText: "Works created, contributed, or published by the organization.", count: organizationCreatedContributedOrPublishedByQuery.data?.works?.totalCount ?? 0, loading: organizationCreatedContributedOrPublishedByQuery.loading }, | ||
| { id: "fundedBy", title: "Funded By", tooltipText: "Works funded by the organization and its child organizations.", count: organizationFundedByQuery.data?.works?.totalCount ?? 0, loading: organizationFundedByQuery.loading }, | ||
| // OMP relationships are included in allRelated, but we don't document or explain this functionality ATM. | ||
| // { id: "connectedToOrganizationOMPs", title: "Related to OMPs", tooltipText: "Works related to Output Management Plans associated with the organization.", count: 0 }, | ||
| ] |
There was a problem hiding this comment.
Critical: Same conditional hook issue for organization relation queries.
The useOrganizationRelationQuery has the same Rules of Hooks violation as useConnectionQuery. Apply the same fix pattern.
🔎 Proposed fix
- const useOrganizationRelationQuery = (organizationRelationType: string) =>
- vars?.rorId
- ? useSearchDoiQuery({ ...vars, organizationRelationType, pageSize: 0 })
- : { loading: false, data: undefined, error: undefined }
+ const useOrganizationRelationQuery = (organizationRelationType: string) =>
+ useSearchDoiQuery(
+ { ...vars, organizationRelationType, pageSize: 0 },
+ { enabled: !!vars?.rorId }
+ )This requires the enabled option to be added to useSearchDoiQuery as described above.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 79-79: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/components/WorkFacets/WorkFacets.tsx around lines 77 to 94, the
useOrganizationRelationQuery currently conditionally calls useSearchDoiQuery
which violates the Rules of Hooks; change it to call the hook unconditionally
and use the query's enabled option instead (e.g. call useSearchDoiQuery({ ...,
organizationRelationType, pageSize: 0, enabled: Boolean(vars?.rorId) }) always),
remove the ternary that returns a manual {loading,data,error} fallback, and keep
the rest of the code using the hook results as before so hooks are
deterministic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/WorkFacets/WorkFacets.tsx (2)
52-75: Critical: Conditional hook calls still violate Rules of Hooks.This issue was previously flagged but remains unresolved. The
useConnectionQueryhelper conditionally callsuseSearchDoiQuery, which violates React's rule that hooks must be called unconditionally in the same order on every render.You must fix this before merging. Refactor
useSearchDoiQueryinsearchDoiQuery.tsto accept anenabledoption and always call the hook:// In searchDoiQuery.ts, update useSearchDoiQuery: export function useSearchDoiQuery(variables: QueryVar, options?: { enabled?: boolean }) { const { isPending, data, error } = useQuery({ queryKey: ['doiSearch', variables], queryFn: async () => fetchDois(variables), enabled: options?.enabled ?? true }) return { loading: isPending, data: data?.data, error } }Then update this component:
- const useConnectionQuery = (connectionType: string) => - vars?.relatedToDoi - ? useSearchDoiQuery({ ...vars, connectionType, pageSize: 0 }) - : { loading: false, data: undefined, error: undefined } + const useConnectionQuery = (connectionType: string) => + useSearchDoiQuery( + { ...vars, connectionType, pageSize: 0 }, + { enabled: !!vars?.relatedToDoi } + )
77-94: Critical: Same conditional hook violation for organization queries.The
useOrganizationRelationQueryhelper has the identical Rules of Hooks violation. Apply the same fix pattern as foruseConnectionQuery:- const useOrganizationRelationQuery = (organizationRelationType: string) => - vars?.rorId - ? useSearchDoiQuery({ ...vars, organizationRelationType, pageSize: 0 }) - : { loading: false, data: undefined, error: undefined } + const useOrganizationRelationQuery = (organizationRelationType: string) => + useSearchDoiQuery( + { ...vars, organizationRelationType, pageSize: 0 }, + { enabled: !!vars?.rorId } + )src/data/queries/searchDoiQuery.ts (1)
13-37: Organization query refactor is sound, but fix the double space typo.The switch-based approach correctly routes different organization relation types. However, Line 20 still contains a double space that was previously flagged:
- const createdContributedOrPublishedBy = `(organization_id:${id} OR provider.ror_id:${urlId})` + const createdContributedOrPublishedBy = `(organization_id:${id} OR provider.ror_id:${urlId})`
🧹 Nitpick comments (1)
src/components/FacetList/FacetListItem.tsx (1)
89-95: Consider using a semantic button element.While the div with
role="button"andtabIndex={0}is accessible and functional, using an actual<button>element would be more semantic and provide better built-in keyboard and screen reader support. However, this may be constrained by styling requirements with the parentListGroup.Item.🔎 Suggested refactor
- <div + <button + type="button" onClick={handleClick} onKeyDown={handleKeyDown} className={styles.facetlink} - role="button" - tabIndex={0} > <FontAwesomeIcon icon={icon} /> <span className={styles.facetTitle}> {facet.title} {tooltipText && <InfoTooltip text={tooltipText} />} </span> {facet.loading ? <LoadingCount /> : <span>{facet.count.toLocaleString('en-US')}</span>} - </div> + </button>Note: You may need to add CSS to reset default button styles to match the current appearance.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/FacetList/FacetListItem.tsxsrc/components/WorkFacets/WorkFacets.tsxsrc/data/queries/searchDoiQuery.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/data/queries/searchDoiQuery.ts (3)
src/data/queries/doiQuery.ts (2)
QueryVar(144-146)buildDoiSearchParams(9-19)src/data/queries/searchPersonQuery.ts (1)
QueryVar(93-96)src/data/queries/searchDoiFacetsQuery.ts (1)
buildDoiSearchParams(7-21)
src/components/FacetList/FacetListItem.tsx (3)
src/data/types.ts (1)
Facet(338-344)src/components/InfoTooltip/InfoTooltip.tsx (1)
InfoTooltip(11-27)src/components/FacetList/FacetList.tsx (2)
FacetList(44-70)FacetListProps(14-27)
src/components/WorkFacets/WorkFacets.tsx (6)
src/data/queries/searchDoiQuery.ts (2)
QueryVar(262-283)useSearchDoiQuery(205-209)src/data/queries/repositoryQuery.ts (1)
QueryVar(137-139)src/data/queries/doiQuery.ts (1)
QueryVar(144-146)src/data/types.ts (1)
Facet(338-344)src/components/FacetList/FacetList.tsx (1)
FacetList(25-52)src/data/queries/searchDoiFacetsQuery.ts (1)
QueryData(90-92)
🪛 Biome (2.1.2)
src/components/WorkFacets/WorkFacets.tsx
[error] 54-54: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 79-79: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (5)
🔇 Additional comments (13)
src/components/FacetList/FacetListItem.tsx (6)
4-4: LGTM!The new imports for
useRouter,InfoTooltip, andContentLoaderare properly used throughout the component.Also applies to: 13-14
26-37: LGTM!The
LoadingCountskeleton component is well-implemented and provides good visual feedback during loading states.
51-51: LGTM!The
useRouterhook is correctly imported fromnext/navigationand used for client-side navigation.
72-81: LGTM!The navigation handlers are well-implemented with proper keyboard support (Enter and Space keys) and the
preventDefault()call correctly prevents unwanted scrolling behavior.
83-85: LGTM!The early return logic correctly hides facet items with zero count while preserving them during loading states.
101-101: LGTM!The conditional rendering between the loading skeleton and the actual count provides good UX feedback.
src/components/WorkFacets/WorkFacets.tsx (3)
10-10: LGTM: Props and imports updated correctly.The transition to
QueryVarand the addition of theclientsfacet align well with the new dynamic query approach.Also applies to: 16-16, 30-30, 43-43
120-142: LGTM: Connection type and organization relation facets rendered correctly.The change from
includestostartsWithfor URL matching is more precise, and the new organization relation type facets follow the established pattern with appropriate tooltips.
198-218: LGTM: Repository facets with helpful tooltips.The conditional rendering prevents redundant display on repository pages, and the tooltips provide clear explanations of what each facet represents.
src/data/queries/searchDoiQuery.ts (4)
39-70: LGTM: DOI relationship query builder is well-structured.The function correctly handles all connection types with appropriate early returns, defensive filtering of falsy values, and a comprehensive default case that combines all relation types.
73-89: LGTM: Query composition correctly integrates new helpers.The updated
buildQuerycleanly delegates to the specialized helper functions while maintaining the existing filter-and-join pattern.
119-141: LGTM: Dynamic pageSize handling is more flexible.The optional pageSize logic correctly falls back to
countwhenvariables.pageSizeis undefined and only appends the parameter when a value is present.
262-283: LGTM: QueryVar interface expanded appropriately.The new optional fields support the enhanced query capabilities while maintaining backward compatibility.
| param: string | ||
| url: string | ||
|
|
||
| tooltipText?: string |
There was a problem hiding this comment.
Use facet.tooltipText instead of a separate prop.
The tooltipText prop is destructured but never passed by the parent component (FacetList.tsx). The Facet type already includes a tooltipText property, so the component should use facet.tooltipText directly instead of accepting it as a separate prop.
🔎 Proposed fix
interface Props {
facet: Facet
param: string
url: string
- tooltipText?: string
checked?: boolean
radio?: boolean
value?: string
} export default function FacetListItem(props: Props) {
const {
facet,
param,
url,
- tooltipText,
checked = false,
radio = false,
value: customValue
} = props <span className={styles.facetTitle}>
{facet.title}
- {tooltipText && <InfoTooltip text={tooltipText} />}
+ {facet.tooltipText && <InfoTooltip text={facet.tooltipText} />}
</span>Also applies to: 44-44, 99-99
🤖 Prompt for AI Agents
In src/components/FacetList/FacetListItem.tsx around lines 20, 44 and 99, the
component destructures a tooltipText prop that is never passed by the parent;
instead use facet.tooltipText from the Facet object. Remove tooltipText from the
component props/type definition and from the destructuring, and replace any uses
of the standalone tooltipText (lines ~44 and ~99) with facet.tooltipText; ensure
no breaking references remain and run a quick type-check.
Purpose
closes: Add github issue that originated this PR
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.