Replace GraphQL with REST for related content management#550
Replace GraphQL with REST for related content management#550
Conversation
WalkthroughAdds a raw DOI attributes REST endpoint and a client-side related-works manager flow: new managers/hooks for connection types, counts, pagination, and related-content orchestration; removes GraphQL DOI fetchs and extends types and defensive guards across UI components. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as RelatedContent Component
participant ManagerFactory as useRelatedContentManager
participant Query as useDoiRelatedContentQuery
participant CountHook as useConnectionCounts
participant DOIAPI as REST DOI API
UI->>ManagerFactory: init with doi + vars
ManagerFactory->>Query: fetch related content + facets
Query->>DOIAPI: REST requests for content and facets
DOIAPI-->>Query: content + facets
Query-->>ManagerFactory: combined data, facetsLoading, loading, error
ManagerFactory->>CountHook: request per-type counts (uidList)
CountHook->>DOIAPI: count-only queries per connection type
DOIAPI-->>CountHook: counts per type
CountHook-->>ManagerFactory: counts, loading, errors
ManagerFactory-->>UI: manager instance (selectedContent, title, pagination, flags)
UI->>UI: render listing, sankey, error, or empty state based on manager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/WorkFacets/WorkFacets.tsx (1)
66-77: Duplicate/typo indefaultActiveKeys.Line 68 has
"connection-type-facets"and line 75 has"conection-type-facets"(missing 'n'). This appears to be a typo that creates a duplicate entry with a misspelling.Proposed fix
const defaultActiveKeys = [ "authors-facets", "connection-type-facets", "published-facets", "work-type-facets", "license-facets", "language-facets", "field-of-science-facets", "registration-agency-facets", - "conection-type-facets", "repository-type-facets" ]
🤖 Fix all issues with AI agents
In @api/app.py:
- Around line 30-41: The DOI validation and fetching in related_works_list
duplicates the logic in related_works; extract that shared behavior into a
helper (e.g., validate_and_get_full_doi_attributes) that takes the incoming doi
path, calls extract_doi, returns the same (400) for invalid format, calls
get_full_corpus_doi_attributes with RelatedWorkReports.parser and DOI_API and
returns the same (404) when not found; then replace the body of both
related_works_list and related_works to call this helper and return its result
(or the helper can return the full_doi_attributes for the route to jsonify),
preserving existing responses and references to extract_doi,
get_full_corpus_doi_attributes, RelatedWorkReports.parser, and DOI_API.
In @src/app/(main)/doi.org/[...doi]/RelatedContent.tsx:
- Line 69: Fix the typo in the error message string inside the RelatedContent
component: change "An error occured loading related content." to "An error
occurred loading related content." so the Error component receives the correctly
spelled message prop (reference: RelatedContent.tsx and the Error component
usage).
- Around line 10-11: The imported UI component named Error shadows the global
Error constructor; rename the import (for example to ErrorComponent or
RelatedContentError) in the import statement and update all usages of the JSX
tag (the <Error ... /> instance(s) in RelatedContent.tsx) to use the new
identifier so the global Error constructor is not shadowed.
In @src/components/WorksListing/WorksListing.tsx:
- Around line 23-32: Replace the inline prop type for connectionTypesCounts in
WorksListing.tsx with the centralized ConnectionTypeCounts type from
src/data/types.ts: remove the inline object shape, add an import for
ConnectionTypeCounts, and update the component prop/interface to use
ConnectionTypeCounts for the connectionTypesCounts prop so the file references
the shared type instead of duplicating it.
In @src/data/managers/RelatedContentManager.ts:
- Line 9: Replace the loose any types in RelatedContentManager with explicit
interfaces: define interfaces like RelatedContentVars (e.g., id: string, other
required query params), RelatedContentData (e.g., work?: Work, related fields
typed), and ConnectionCounts (map of string to number or specific shape), then
update the class property declarations (the private readonly data, private
readonly vars, private readonly connectionCounts) and the constructor signature
to use these interfaces instead of any; ensure usages inside methods match the
new shapes (adjust property accessors or types like Work, WorkEdge, etc.) and
import or reference existing domain types where applicable.
In @src/data/queries/doiRelatedContentQuery.ts:
- Around line 17-26: The work object currently hardcodes resourceTypeGeneral:
"TEXT", which can be inaccurate; replace this with the actual resource type from
the primary DOI data (e.g., use content.data?.works?.resourceTypeGeneral or
another authoritative field from the DOI response) or remove the property
entirely if no reliable type is available; ensure you preserve merging logic for
[variables.connectionType || "allRelated"] which spreads ...content.data?.works
and ...facets.data?.works so the correct resourceTypeGeneral comes from those
sources rather than a hardcoded string.
In @src/data/queries/searchDoiQuery.ts:
- Around line 72-76: In searchDoiQuery.ts update the typo in the inline comment
after the connectionType branch: change "retrun" to "return" so the comment
reads "By default only return positive part"; this is the comment adjacent to
the connectionType === "otherRelated" branch that returns `(${positivePart})`.
- Around line 218-219: The query key for useSearchDoiQuery omits the count
parameter so different counts can return stale cached results; update the
queryKey in useSearchDoiQuery to include count (e.g., ['doiSearch', variables,
count]) so React Query distinguishes queries, and ensure fetchDois(variables,
count) remains the queryFn to match the key.
In @src/data/types.ts:
- Around line 75-83: Optional facet properties (published, resourceTypes,
languages, licenses, fieldsOfScience, registrationAgencies, repositories,
affiliations, funders, personToWorkTypesMultilevel) are accessed directly in
several components causing runtime errors when not loaded; update all usages to
use optional chaining or null coalescing so code handles undefined values safely
— e.g., replace direct .map() calls on works.published / works.resourceTypes /
works.licenses / works.fieldsOfScience / works.languages and
data.publications.published with works.published?.map(...) or (works.published
?? []).map(...), and when calling multilevelToSankey pass
works.personToWorkTypesMultilevel ?? [] (or use optional chaining if function
accepts undefined) so components like WorksDashboard.tsx, Content.tsx
(statistics page), and WorksListing.tsx no longer assume facets are present.
In @src/utils/helpers.ts:
- Around line 189-191: Replace direct property access of json.relationships and
json.relationships.client.data with defensive optional chaining: change any uses
of json.relationships.client.data (e.g., where client data is read) to
json.relationships?.client?.data and set the returned object’s relationships
field to json.relationships ?? {} (instead of json.relationships) so missing
relationships won’t throw at runtime; update all occurrences in the helpers
where json.relationships is accessed.
🧹 Nitpick comments (9)
src/data/queries/repositoryRelatedContentQuery.ts (1)
11-14: Inconsistent loading behavior compared topersonRelatedContentQuery.ts.The loading logic here only considers
results.loading, while the similarpersonRelatedContentQuery.tsusesresults.loading || facets.loading. Additionally, the commented-out early return (line 14) should either be removed or restored with a comment explaining why it's disabled.If intentional (to allow partial rendering), consider documenting this difference or aligning
personRelatedContentQuery.tsto the same pattern for consistency.Suggested cleanup
- const loading = results.loading; + // Only block on results loading; facets load independently for partial rendering + const loading = results.loading; const error = results.error || facets.error; - // if (loading || error) return { loading, data: undefined, error }src/data/managers/PaginationManager.ts (1)
13-32: LGTM – clean encapsulation of pagination logic.The class correctly handles optional
pageInfowith safe defaults. The getters provide read-only access to pagination state.Consider adding a convenience getter that returns the full
Paginationobject for consumers that need all fields at once:Optional: Add toPagination() getter
get endCursor(): string { return this.works.pageInfo?.endCursor || '' } + + get toPagination(): Pagination { + return { + hasPagination: this.hasPagination, + hasNextPage: this.hasNextPage, + endCursor: this.endCursor + } + } }src/data/managers/ConnectionCountManager.ts (1)
82-102: DRY violation: error arrays are constructed twice.The same array of errors is built twice (lines 82-91 for
isErrorcheck and lines 93-102 forerrorslist). Extract to a single variable.Proposed refactor
+ const allErrors = [ + allRelatedQuery.error, + referencesQuery.error, + citationsQuery.error, + partsQuery.error, + partOfQuery.error, + versionsQuery.error, + versionOfQuery.error, + otherRelatedQuery.error + ] + - const isError = [ - allRelatedQuery.error, - referencesQuery.error, - citationsQuery.error, - partsQuery.error, - partOfQuery.error, - versionsQuery.error, - versionOfQuery.error, - otherRelatedQuery.error - ].some(Boolean) - - const errors = [ - allRelatedQuery.error, - referencesQuery.error, - citationsQuery.error, - partsQuery.error, - partOfQuery.error, - versionsQuery.error, - versionOfQuery.error, - otherRelatedQuery.error - ].filter(Boolean) + const isError = allErrors.some(Boolean) + const errors = allErrors.filter(Boolean)src/data/queries/searchDoiQuery.ts (1)
13-18: Potential DRY violation withVALID_CONNECTION_TYPES.This array duplicates
CONNECTION_TYPESfromConnectionTypeManager.ts. Consider importing and reusing the single source of truth to prevent drift.Suggested refactor
+import { CONNECTION_TYPES } from 'src/data/managers/ConnectionTypeManager' + -const VALID_CONNECTION_TYPES = ['references', 'citations', 'parts', 'partOf', 'versions', 'versionOf', 'allRelated', 'otherRelated']; - function buildRelatedDoiQuery(relatedDoi: string | undefined, uidList: string[] | undefined, connectionType="allRelated" ): string { if (!relatedDoi) return '' - if (!VALID_CONNECTION_TYPES.includes(connectionType)) return '' + if (!CONNECTION_TYPES.includes(connectionType as any)) return ''src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (2)
24-35: Type safety bypass withas anyforrelatedIdentifiers.The comment acknowledges
relatedIdentifiersmay not be in theWorktype, but casting toanyloses all type safety. Consider extending theWorktype to includerelatedIdentifiersor creating a proper type guard.Safer approach with type extension
// In types.ts or locally interface WorkWithRelatedIdentifiers extends Work { relatedIdentifiers?: Array<{ relatedIdentifierType: string relatedIdentifier: string }> } function extractRelatedDois(work: Work | undefined): string[] { if (!work) return [] const workWithRI = work as WorkWithRelatedIdentifiers if (!workWithRI.relatedIdentifiers) return [] return workWithRI.relatedIdentifiers .filter(identifier => identifier.relatedIdentifierType === 'DOI') .map(identifier => identifier.relatedIdentifier) .filter(Boolean) }
63-64: Consider combining loading states.Two sequential loading checks could be combined for cleaner code:
- if (doiQuery.isLoading) return <Row><Loading /></Row> - if (manager.isLoading) return <Row><Loading /></Row> + if (doiQuery.isLoading || manager.isLoading) return <Row><Loading /></Row>src/data/managers/RelatedContentManager.ts (1)
38-95: Add explicit return types to all getter methods.All getter methods lack explicit return types, which reduces code clarity and type safety. Adding explicit return types helps catch type mismatches early and serves as inline documentation.
📝 Example with explicit return types
- get isLoading() { + get isLoading(): boolean { return this.loading } - get facetsAreLoading() { + get facetsAreLoading(): boolean { return this.facetsLoading } - get hasError() { + get hasError(): boolean { return !!this.error } - get errorMessage() { + get errorMessage(): string | undefined { return this.error?.message } - get hasData() { + get hasData(): boolean { return !!this.data } - get hasAnyRelatedWorks() { + get hasAnyRelatedWorks(): boolean { return this.connectionManager?.hasAnyRelatedWorks() || false } - get showSankey() { + get showSankey(): boolean { return this.data?.work ? (isDMP(this.data.work) || isProject(this.data.work)) : false } - get selectedContent() { + get selectedContent(): { works: Works, title: string } { if (!this.connectionManager) return {works: EMPTY_WORKS, title: ''} return this.connectionManager.getWorksAndTitle(this.connectionType) } - get pagination() { + get pagination(): Pagination { if (!this.paginationManager) return EMPTY_PAGINATION return { hasPagination: this.paginationManager.hasPagination, hasNextPage: this.paginationManager.hasNextPage, endCursor: this.paginationManager.endCursor } } - get url() { + get url(): string { return '/doi.org/' + this.vars.id + '/?' }src/data/managers/ConnectionTypeManager.ts (2)
108-112: Add runtime validation before type assertion.Line 110 uses a type assertion (
as Works | undefined) to accessthis.work[connectionType]. SinceconnectionTypeis validated bygetValidConnectionType, the access should be safe, but the type assertion could hide issues if the Work type is incomplete or the connection type isn't present on the work object.Consider adding defensive runtime checks or using a type guard:
🛡️ Optional defensive approach
getWorks(connectionType: string): Works { connectionType = getValidConnectionType(connectionType) - const works = this.work[connectionType] as Works | undefined; + const works = (connectionType in this.work) + ? this.work[connectionType] as Works | undefined + : undefined; return works ?? EMPTY_WORKS }
81-119: Add explicit return types to public methods.Public methods lack explicit return types, which reduces type safety and code documentation. Adding explicit return types helps catch type mismatches and serves as inline documentation.
📝 Methods with explicit return types
- getCounts(): ConnectionTypeCounts { + getCounts(): ConnectionTypeCounts { return this.counts } - hasAnyRelatedWorks(): boolean { + hasAnyRelatedWorks(): boolean { return Object.values(this.counts).some(count => count > 0) } - getDefaultConnectionType(): string { + getDefaultConnectionType(): string { const { allRelated, references, citations, parts, partOf, versions, versionOf } = this.counts if (allRelated > 0) return 'allRelated' if (references > 0) return 'references' if (citations > 0) return 'citations' if (parts > 0) return 'parts' if (partOf > 0) return 'partOf' if (versions > 0) return 'versions' if (versionOf > 0) return 'versionOf' return 'otherRelated' } - formatTitle(connectionType: string): string { + formatTitle(connectionType: string): string { if (connectionType === 'allRelated') return 'All Related Works' if (connectionType === 'otherRelated') return 'Other Works' return connectionType.replace(/([A-Z])/g, ' $1').replace(/^./, str => str.toUpperCase()) } - getWorks(connectionType: string): Works { + getWorks(connectionType: string): Works { connectionType = getValidConnectionType(connectionType) const works = this.work[connectionType] as Works | undefined; return works ?? EMPTY_WORKS } - getWorksAndTitle(connectionType: string | undefined ): { works: Works, title: string } { + getWorksAndTitle(connectionType: string | undefined): { works: Works; title: string } { const validConnectionType = getValidConnectionType(connectionType) const works = this.getWorks(validConnectionType) const title = this.formatTitle(validConnectionType) return { works, title } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/app.pysrc/app/(main)/doi.org/[...doi]/RelatedContent.tsxsrc/app/(main)/doi.org/[...doi]/mapSearchParams.tssrc/components/SankeyGraph/SankeyGraph.tsxsrc/components/WorkFacets/WorkFacets.tsxsrc/components/WorksListing/WorksListing.tsxsrc/data/managers/ConnectionCountManager.tssrc/data/managers/ConnectionTypeManager.tssrc/data/managers/PaginationManager.tssrc/data/managers/RelatedContentManager.tssrc/data/queries/doiQuery.tssrc/data/queries/doiRelatedContentQuery.tssrc/data/queries/repositoryRelatedContentQuery.tssrc/data/queries/searchDoiQuery.tssrc/data/types.tssrc/utils/helpers.ts
💤 Files with no reviewable changes (1)
- src/data/queries/doiQuery.ts
🧰 Additional context used
🧬 Code graph analysis (11)
src/components/SankeyGraph/SankeyGraph.tsx (2)
src/data/types.ts (1)
MultilevelFacet(340-342)src/components/SankeyGraph/SankeySpec.ts (1)
SankeyGraphData(13-16)
src/data/managers/PaginationManager.ts (2)
src/data/types.ts (2)
PageInfo(349-352)Pagination(354-356)src/components/Pager/Pager.tsx (1)
Pager(15-36)
src/components/WorkFacets/WorkFacets.tsx (2)
src/data/types.ts (1)
Facet(334-338)src/components/FacetList/FacetList.tsx (1)
FacetListProps(14-27)
src/data/managers/ConnectionCountManager.ts (3)
src/data/types.ts (1)
ConnectionTypeCounts(363-372)src/data/managers/ConnectionTypeManager.ts (1)
CONNECTION_TYPES(30-30)src/data/queries/searchDoiQuery.ts (2)
fetchDois(170-190)useSearchDoiQuery(218-222)
src/data/queries/repositoryRelatedContentQuery.ts (4)
src/data/queries/searchDoiQuery.ts (1)
QueryData(271-273)src/data/queries/searchDoiFacetsQuery.ts (1)
QueryData(90-92)src/data/queries/repositoryQuery.ts (1)
QueryData(133-135)src/data/queries/personRelatedContentQuery.ts (1)
usePersonRelatedContentQuery(8-29)
src/utils/helpers.ts (1)
src/data/queries/doiQuery.ts (1)
convertToQueryData(21-25)
src/data/queries/doiRelatedContentQuery.ts (5)
src/data/queries/searchDoiQuery.ts (3)
QueryVar(275-295)useSearchDoiQuery(218-222)QueryData(271-273)src/data/queries/doiQuery.ts (2)
QueryVar(57-59)QueryData(53-55)src/data/queries/personRelatedContentQuery.ts (3)
QueryVar(81-81)QueryData(81-81)usePersonRelatedContentQuery(8-29)src/data/queries/searchDoiFacetsQuery.ts (1)
useSearchDoiFacetsQuery(78-87)src/data/queries/repositoryRelatedContentQuery.ts (1)
QueryData(31-33)
src/data/managers/RelatedContentManager.ts (4)
src/data/managers/ConnectionTypeManager.ts (4)
ConnectionTypeManager(51-120)getValidConnectionType(36-42)EMPTY_CONNECTION_TYPE_COUNTS(9-18)EMPTY_WORKS(20-27)src/data/managers/PaginationManager.ts (2)
PaginationManager(13-33)EMPTY_PAGINATION(7-11)src/utils/helpers.ts (2)
isDMP(55-57)isProject(59-66)src/data/types.ts (1)
ConnectionTypeCounts(363-372)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
src/data/queries/searchDoiQuery.ts (1)
SortOption(127-127)
src/data/managers/ConnectionTypeManager.ts (2)
src/data/types.ts (3)
ConnectionTypeCounts(363-372)Works(72-99)Work(19-70)src/data/queries/searchSummaryQuery.ts (1)
convertSummaryToCounts(12-20)
src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (3)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
mapSearchparams(19-37)src/data/queries/doiQuery.ts (1)
fetchDoi(24-47)src/data/managers/RelatedContentManager.ts (1)
useRelatedContentManager(102-107)
🪛 Biome (2.1.2)
src/app/(main)/doi.org/[...doi]/RelatedContent.tsx
[error] 10-10: 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). (3)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (3)
🔇 Additional comments (13)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
1-1: LGTM! Sort parameter properly integrated.The addition of the optional
sortparameter is well-typed and correctly propagated to the query variables. TypeScript's type system will ensure only validSortOptionvalues are passed.Also applies to: 15-15, 31-31
src/components/SankeyGraph/SankeyGraph.tsx (1)
21-22: LGTM! Proper undefined handling added.The early return guard prevents runtime errors when
facetsis undefined and aligns with the PR's goal of making facets optional. The component's existing empty-data handling at line 77 ensures a graceful fallback UI.src/data/queries/repositoryRelatedContentQuery.ts (1)
22-28: LGTM – return surface aligns with the new manager pattern.Exposing
loading,error, andfacetsLoadingseparately enables consumers to handle partial states (e.g., render results while facets are still loading). This aligns with the broader PR goal of flexible, independent data loading.src/data/queries/doiRelatedContentQuery.ts (1)
8-15: Consistent return shape on early exit.Good pattern: returning
facetsLoadingeven on early exit ensures consumers always have a consistent shape. This enables partial state handling.src/components/WorkFacets/WorkFacets.tsx (1)
62-64: LGTM – clean predicate for connection type visibility.The
hasAnyConnectionsfunction clearly expresses the intent and the logic is correct.src/data/managers/ConnectionCountManager.ts (1)
36-40: No type safety concern exists here.CONNECTION_TYPESis derived directly fromObject.keys(EMPTY_CONNECTION_TYPE_COUNTS), which is explicitly typed asConnectionTypeCounts. The keys maintain consistent order (ES2015+), and since the queries array mapsCONNECTION_TYPESin the same order, the index-based assignment is guaranteed to match correctly. The code follows the intended "single source of truth" pattern.src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (1)
44-60: Hook ordering verified – code complies with Rules of Hooks and handles empty data gracefully.The hooks are called unconditionally at the top level, and
useRelatedContentManagersafely receives an emptyuidListon first render (beforedoiQuerycompletes). The manager's constructor checksif (data?.work)and sets bothconnectionManagerandpaginationManagerto null when data is unavailable, with all getters providing proper fallbacks (EMPTY_WORKS,EMPTY_PAGINATION,EMPTY_CONNECTION_TYPE_COUNTS). Query building also handles emptyuidListwith a length check before inclusion.src/data/managers/RelatedContentManager.ts (1)
97-99: Verify the trailing/?in URL construction.The URL ends with
/?which appears incomplete. If query parameters are intended to be appended by consumers, this should be documented. Otherwise, consider removing the trailing/?or using a URL builder utility.src/data/types.ts (2)
56-68: LGTM! New Work fields support extended connection types.The new optional fields (
referenceCount,partCount,partOfCount,versionsCount,versionOfCount,allRelatedCount,versions,versionOf) follow the existing naming conventions and are consistently marked as optional, which aligns with the PR's goal of supporting additional connection types.
354-356: LGTM! New types support the manager architecture.The
Paginationtype appropriately extendsPageInfowith an additionalhasPaginationflag, andConnectionTypeCountsprovides type-safe structure for all connection type counts. Both types are well-defined and support the new manager-based architecture.Also applies to: 363-372
src/data/managers/ConnectionTypeManager.ts (3)
1-42: LGTM! Well-structured constants and helpers.The constants and helper functions establish a single source of truth for connection types. Deriving
CONNECTION_TYPESfromEMPTY_CONNECTION_TYPE_COUNTS(line 30) ensures consistency, and the helper functions (isConnectionType,getValidConnectionType) provide robust validation.
44-49: LGTM! Well-defined external counts type.The
ExternalConnectionCountstype appropriately encapsulates connection counts along with their loading and error states, supporting the dual-source count logic in the manager.
51-79: LGTM! Constructor and count calculation are well-designed.The constructor appropriately accepts both internal (
Work) and external (ExternalConnectionCounts) count sources. ThecalculateCountsmethod correctly prioritizes external counts (lines 64-66) and safely falls back to work data using optional chaining and nullish coalescing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/(main)/download-reports/ror/funders/route.ts (1)
5-8: Use proper types instead ofanyfor type safety.The defensive helper is a good addition for handling optional funders, but using
anybypasses TypeScript's type checking. Consider using theWorkstype fromsrc/data/types.tsand specifying a proper return type.♻️ Suggested refactor with proper types
Import the Works type at the top of the file:
import type { Works } from 'src/data/types'Then update the helper:
-function getFundersArray(works: any): any[] { - return works.funders ? works.funders : [] +function getFundersArray(works: Works): any[] { + return works.funders ?? [] }Note: Using the nullish coalescing operator (
??) is more idiomatic than a ternary for default values.src/components/WorksDashboard/WorksDashboard.tsx (1)
35-82: LGTM! Robust defensive data handling.The optional chaining and empty array fallbacks ensure charts render safely even when data fields are missing or still loading. This aligns perfectly with the PR's goal of allowing facets and results to render independently.
The pattern is applied consistently across all data transformations:
published,resourceTypes, andlicensesDataall gracefully handle undefined values- Chart components receive safe empty arrays rather than undefined
- Existing chart logic (
getTopFive,VerticalBarChart) already handles empty data appropriately💡 Optional: Consider nullish coalescing for clarity
For modern JavaScript clarity, you could use the nullish coalescing operator (
??) instead of logical OR (||), though functionally they're equivalent here since arrays are always truthy:- const published = works.published?.map((x) => ({ - title: x.title, - count: x.count - })) || [] + const published = works.published?.map((x) => ({ + title: x.title, + count: x.count + })) ?? []Same pattern could apply to lines 40, 41, 79, and 82. This is purely a style preference—the current implementation is correct.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/(main)/download-reports/ror/funders/route.tssrc/components/WorksDashboard/WorksDashboard.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/(main)/download-reports/ror/funders/route.ts (1)
src/data/queries/searchDoiFacetsQuery.ts (1)
fetchDoisFacets(57-76)
src/components/WorksDashboard/WorksDashboard.tsx (2)
src/utils/helpers.ts (2)
getTopFive(70-106)toBarRecord(108-110)src/components/VerticalBarChart/VerticalBarChart.tsx (1)
VerticalBarChart(29-143)
⏰ 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 (3)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (2)
🔇 Additional comments (1)
src/app/(main)/download-reports/ror/funders/route.ts (1)
16-18: LGTM! Defensive handling aligns with optional funders field.The CSV generation correctly uses the safely-extracted funders array, providing graceful handling when funders are missing. This aligns well with the broader data model changes in the PR.
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
search-related-rest
|
| Run status |
|
| Run duration | 01m 45s |
| 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 ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/app/(main)/doi.org/[...doi]/RelatedContent.tsx:
- Around line 63-71: The component currently checks doiQuery.isLoading but never
handles doiQuery errors; update RelatedContent.tsx to mirror the manager error
handling by checking for doiQuery.error (or doiQuery.hasError) after the loading
checks and returning the same error UI (use CommonsError with a title like "An
error occurred loading DOI data." and message from doiQuery.error.message or
doiQuery.errorMessage); keep the existing manager error branch as-is so both
data sources render the CommonsError when they fail.
In @src/data/queries/searchDoiQuery.ts:
- Around line 25-30: The DOI interpolations are inconsistently quoted which
risks breaking DataCite/OpenSearch queries; update searchDoiQuery.ts so every
place the DOI is injected (e.g., the relatedIdentifiers
array/relatedIdentifierPart and all occurrences like citation_ids:${doi},
reference_ids:${doi}, part_of_ids:${doi}, etc.) uses a single, consistent
strategy: either wrap the DOI in double quotes or escape Lucene/OpenSearch
special characters before interpolation; add or reuse a helper (e.g.,
escapeLuceneQuery or quoteIdentifier) and apply it to the DOI in all query
fragments to ensure every field interpolation is safely quoted/escaped.
🧹 Nitpick comments (9)
src/components/WorksListing/WorksListing.tsx (1)
23-23: Good refactor to use the shared type.Using
ConnectionTypeCountsfromsrc/data/typesimproves consistency across the codebase and supports the newversions/versionOfconnection types.Note:
WorkFacetsstill uses an inline type for this prop (seesrc/components/WorkFacets/WorkFacets.tsxlines 10-15). Consider updating it to also useConnectionTypeCountsfor consistency.src/data/queries/searchDoiQuery.ts (3)
13-13: Consider exportingVALID_CONNECTION_TYPESfor reuse.Based on the AI summary, this constant is intended for export to constrain connection types across the codebase. However, it's currently not exported, which limits its reusability in other modules like
ConnectionTypeManager.♻️ Suggested change
-const VALID_CONNECTION_TYPES = ['references', 'citations', 'parts', 'partOf', 'versions', 'versionOf', 'allRelated', 'otherRelated']; +export const VALID_CONNECTION_TYPES = ['references', 'citations', 'parts', 'partOf', 'versions', 'versionOf', 'allRelated', 'otherRelated'] as const; +export type ConnectionType = typeof VALID_CONNECTION_TYPES[number];
20-20: Remove redundant variable assignment.The assignment
const doi = relatedDoiis unnecessary sincerelatedDoiis already validated as non-empty on line 16.♻️ Suggested change
- const doi = relatedDoi;Then use
relatedDoidirectly throughout the function.
280-282: Consider stricter typing forconnectionType.Using a string literal union type derived from
VALID_CONNECTION_TYPESwould provide compile-time safety and better IDE autocompletion.♻️ Suggested change
If you export the constant as suggested above:
- connectionType?: string + connectionType?: ConnectionTypesrc/data/managers/RelatedContentManager.ts (4)
8-17: Consider replacinganytypes with proper type definitions.Multiple fields use
any(data,vars,connectionCounts), which undermines TypeScript's type safety. Since this is a new class, establishing proper types now will prevent future type-related bugs and improve maintainability.♻️ Suggested approach
+import { QueryVar } from 'src/data/queries/searchDoiQuery' +import { DoiRelatedContentData } from 'src/data/queries/doiRelatedContentQuery' + export class RelatedContentManager { - private readonly data: any + private readonly data: DoiRelatedContentData | undefined private readonly loading: boolean private readonly error: Error | undefined | null private readonly connectionManager: ConnectionTypeManager | null private readonly paginationManager: PaginationManager | null private readonly connectionType: string - private readonly vars: any + private readonly vars: QueryVar & { id: string } private readonly facetsLoading: boolean - private readonly connectionCounts: any + private readonly connectionCounts: ReturnType<typeof useConnectionCounts> | undefined
19-36: Constructor parameter types should match field types.The constructor parameters use
anytypes extensively. Consider defining an interface for the constructor parameters to improve code clarity and enable better IDE support.
83-86: Theworksreturn type isanywhich propagates type unsafety.Consider defining a proper return type for
selectedContentto maintain type safety through the component chain.♻️ Suggested approach
- get selectedContent() : {works: any, title: string} { + get selectedContent() : {works: Works, title: string} {This requires importing
Worksfromsrc/data/typesand ensuringConnectionTypeManager.getWorksAndTitlereturns the proper type.
97-99: URL construction could be more robust.The URL is built via string concatenation. Consider using
encodeURIComponentfor the DOI to handle special characters safely.♻️ Suggested change
get url() { - return '/doi.org/' + this.vars.id + '/?' + return `/doi.org/${encodeURIComponent(this.vars.id)}?` }src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (1)
24-35: AddrelatedIdentifiersproperty to theWorktype.The
relatedIdentifiersproperty is accessed in this function via a type assertion toany(line 28) because it's missing from theWorktype definition insrc/data/types.ts. Since the API returns this data (as evidenced by its usage in search queries), adding the property to theWorktype would improve type safety and eliminate the type assertion. Define it as an optional array of objects withrelatedIdentifierTypeandrelatedIdentifierfields.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/(main)/doi.org/[...doi]/RelatedContent.tsxsrc/components/WorksListing/WorksListing.tsxsrc/data/managers/RelatedContentManager.tssrc/data/queries/searchDoiQuery.tssrc/utils/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/helpers.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/data/queries/searchDoiQuery.ts (7)
src/data/queries/doiQuery.ts (3)
QueryVar(57-59)buildDoiSearchParams(9-19)QueryVar(144-146)src/data/queries/repositoryQuery.ts (1)
QueryVar(137-139)src/data/queries/searchPersonQuery.ts (1)
QueryVar(93-96)src/data/queries/organizationQuery.ts (1)
QueryVar(95-97)src/data/queries/gridQuery.ts (1)
QueryVar(24-26)src/data/queries/claimQuery.ts (1)
QueryVar(96-98)src/data/queries/searchDoiFacetsQuery.ts (1)
buildDoiSearchParams(7-21)
src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (3)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
mapSearchparams(19-37)src/data/queries/doiQuery.ts (1)
fetchDoi(24-47)src/data/managers/RelatedContentManager.ts (1)
useRelatedContentManager(102-107)
src/components/WorksListing/WorksListing.tsx (2)
src/data/types.ts (1)
ConnectionTypeCounts(363-372)src/components/WorkFacets/WorkFacets.tsx (2)
Props(11-16)WorkFacets(37-158)
⏰ 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 (2)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (5)
🔇 Additional comments (10)
src/components/WorksListing/WorksListing.tsx (1)
9-9: LGTM!Clean import of the shared
ConnectionTypeCountstype alongsideWorks.src/data/queries/searchDoiQuery.ts (4)
36-57: LGTM!The query mapping is well-structured. The
filter(Boolean)on line 69 correctly handles cases whereuidPartis empty.
59-76: LGTM!The
otherRelatedquery correctly excludes specific relationship types using theAND NOTclause, ensuring only non-categorized relationships are returned.
91-91: LGTM!The integration with
buildQuerycorrectly passes the new variables with sensible defaults.
218-219: LGTM!Including
countin thequeryKeyarray ensures proper cache separation for different page sizes, which is essential for TanStack Query's caching behavior.src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (4)
1-17: LGTM!Imports are well-organized and appropriate for the component's needs.
19-22: LGTM!The helper correctly transforms URL search params into query variables.
88-114: LGTM!The render logic cleanly extracts data from the manager and passes appropriate props to
WorksListing. The loading state combination on line 101 correctly reflects all pending data sources.
43-60: Manager hook initializes before DOI data is available, triggering refetch when data arrives.
useRelatedContentManageris called with potentially emptyuidListsince it executes beforedoiQuerycompletes. OnceprimaryWorkis loaded,varsWithRelatedDoischanges, triggering a refetch via TanStack Query'squeryKeydependency (line 219 insearchDoiQuery.ts). The component properly handles loading states with early returns (lines 63–64), so no incorrect content is rendered during the initial fetch, but this two-step fetch pattern is expected given the hook architecture.src/data/managers/RelatedContentManager.ts (1)
102-107: Memoization is not necessary here.The
managerinstance is only used within the component to access properties and is not passed to child components or used as a dependency in other hooks (likeuseEffect). SinceWorksListingreceives individual extracted properties rather than the manager object itself, creating a new instance on each render has no performance impact. The inner hooks (useDoiRelatedContentQuery,useConnectionCounts) already handle their own memoization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/WorkFacets/WorkFacets.tsx (1)
98-101: Inconsistent nullish handling between lines 99 and 100.Line 99 uses
||fordata.authorswhile line 100 uses??fordata.creatorsAndContributors. For consistency and correct null/undefined semantics (preserving empty arrays), consider using??on both lines.♻️ Suggested fix for consistency
{model == "person" - ? <AuthorsFacet authors={data.authors || []} title="Co-Authors" url={url} model={model} /> + ? <AuthorsFacet authors={data.authors ?? []} title="Co-Authors" url={url} model={model} /> : <AuthorsFacet authors={data.creatorsAndContributors ?? []} title="Creators & Contributors" url={url} model={model} /> }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Person/Person.tsxsrc/components/WorkFacets/WorkFacets.tsxsrc/components/WorkMetadata/WorkMetadata.tsxsrc/components/WorksDashboard/WorksDashboard.tsxsrc/components/WorksListing/WorksListing.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/WorksListing/WorksListing.tsx
- src/components/WorksDashboard/WorksDashboard.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/Person/Person.tsx (2)
src/data/queries/searchSummaryQuery.ts (1)
convertSummaryToCounts(12-20)src/data/types.ts (1)
PersonWorks(255-259)
src/components/WorkFacets/WorkFacets.tsx (3)
src/data/types.ts (1)
Facet(334-338)src/components/AuthorsFacet/AuthorsFacet.tsx (1)
AuthorsFacet(12-55)src/components/FacetList/FacetList.tsx (2)
FacetList(44-70)FacetListProps(14-27)
⏰ 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 (2)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (1)
🔇 Additional comments (10)
src/components/WorkMetadata/WorkMetadata.tsx (1)
254-266: LGTM — defensive optional chaining.The added
?.on line 256 is technically redundant since the&&guard on line 254 already ensuresfieldsOfScienceis truthy. However, it's a harmless defensive pattern that aligns with the PR's goal of improving robustness when data is missing.src/components/Person/Person.tsx (3)
24-38: LGTM!Good use of optional chaining to guard against undefined
openLicenseResourceTypes. This prevents a runtimeTypeErrorif the array is missing from the API response.
40-45: LGTM!Safe defaults for
totalOpenLicensesandtotalContentUrlensure these values are always defined for subsequent comparisons and calculations. The pattern is consistent with similar fallbacks used elsewhere in the codebase (e.g.,searchSummaryQuery.ts).
47-48: LGTM!Properly guards against division by zero using
|| 1for the denominator. WhentotalCountis 0 or undefined, this yields 0% rather thanNaNorInfinity, which is the expected behavior for display purposes.src/components/WorkFacets/WorkFacets.tsx (6)
15-15: LGTM!The type extension for
connectionTypesCountsto includeversionsandversionOfis consistent with the new connection type support.
18-31: LGTM!Making
publishedandresourceTypesoptional aligns with the flexible data-fetching approach described in the PR objectives and is properly handled with nullish coalescing in the rendering logic.
56-58: LGTM!New connection type entries for
versionsandversionOffollow the established pattern and are logically positioned in the list.
62-64: LGTM!The
hasAnyConnectionshelper withNonNullable<typeof connectionTypesCounts>is well-typed, and the logic correctly determines whether any connection counts exist before rendering the facet section.
86-96: LGTM!Using
hasConnectionTypesas the rendering condition provides clearer intent and correctly prevents rendering the Connection Types facet when no connections exist.
104-158: LGTM!The consistent use of nullish coalescing (
??) for all optional facet fields provides robust handling of undefined data while preserving empty arrays when present.
jrhoads
force-pushed
the
search-related-rest
branch
from
67eae7a to
b4e33ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/WorkFacets/WorkFacets.tsx`:
- Line 15: The prop definition in WorkFacets.tsx uses an inline type for
connectionTypesCounts that duplicates the existing ConnectionTypeCounts type;
replace the inline shape with the centralized ConnectionTypeCounts type by
importing ConnectionTypeCounts from src/data/types and using it for the
connectionTypesCounts prop (wherever connectionTypesCounts?: ... is declared),
and add the corresponding import statement at the top of the file so the
component uses the shared type.
♻️ Duplicate comments (3)
src/data/queries/searchDoiQuery.ts (1)
13-76: Normalize and escape DOI values before interpolating into query fragments.
doiis used raw incitation_ids,reference_ids, etc. IfrelatedDoiis a full URL (or contains Lucene special characters), those fields likely won’t match, andotherRelatedwon’t exclude non-target relations correctly. Normalize to the DOI name and consistently quote/escape all DOI interpolations.🔧 Suggested fix (normalize + quote/escape)
+function normalizeDoi(input: string): string { + return input.replace(/^https?:\/\/doi\.org\//i, '') +} + +function quoteLuceneTerm(term: string): string { + const escaped = term.replace(/([+\-!(){}\[\]^"~*?:\\/]|&&|\|\|)/g, '\\$1') + return `"${escaped}"` +} ... - const doi = relatedDoi; + const doi = normalizeDoi(relatedDoi); + const doiTerm = quoteLuceneTerm(doi); ... - `"${baseURI}${doi}"`, - `"${doi}"`, - `"${httpBaseURI}${doi}"` + quoteLuceneTerm(`${baseURI}${doi}`), + doiTerm, + quoteLuceneTerm(`${httpBaseURI}${doi}`) ... - references: [`citation_ids:${doi}`], + references: [`citation_ids:${doiTerm}`],src/data/queries/doiRelatedContentQuery.ts (1)
9-27: Avoid hardcodingresourceTypeGeneral: "TEXT".This can be incorrect for many DOIs. Prefer deriving it from authoritative work data or omit the field when unavailable.
🔧 Suggested fix (omit hardcoded type)
- const work = { - types: { - resourceTypeGeneral: "TEXT", - - }, - [variables.connectionType || "allRelated"]: { - ...content.data?.works || {}, - ...facets.data?.works - } - } + const work = { + [variables.connectionType || "allRelated"]: { + ...content.data?.works || {}, + ...facets.data?.works + } + }src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (1)
71-79:doiQuery.erroris not handled.The component checks
doiQuery.isLoadingbut doesn't handledoiQuery.error. If the initial DOI fetch fails,primaryWorkwill be undefined, and the component will proceed to show "No related works found" instead of an error message.🐛 Proposed fix
if (doiQuery.isLoading) return <Row><Loading /></Row> + if (doiQuery.error) return <Row> + <Col md={{ offset: 3 }} className="panel panel-transparent"> + <CommonsError title="An error occurred loading DOI data." message={(doiQuery.error as Error)?.message} /> + </Col> + </Row> if (manager.isLoading) return <Row><Loading /></Row>
🧹 Nitpick comments (5)
src/data/managers/ConnectionCountManager.ts (2)
50-53: Consider removing or deprecating the alternative implementation.The comment suggests this is a fallback "if useQueries causes issues." If
useQueriesis working correctly, consider removinguseConnectionCountsIndividualto avoid maintenance burden, or add a@deprecatedJSDoc tag with context on when to use it.
112-112: Fix inconsistent spacing.There's an extra space before
0in the nullish coalescing expression.- versionOf: versionOfQuery.data?.works?.totalCount ?? 0 + versionOf: versionOfQuery.data?.works?.totalCount ?? 0src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (2)
67-67: Typo in comment: "determin" → "determine".- // Need to determin showSankey based on primaryWork data + // Need to determine showSankey based on primaryWork data
25-36: Type assertion forrelatedIdentifiersis pragmatic but could be improved.The double
as anycast works for runtime checking, but consider defining a more specific type or extending theWorktype to includerelatedIdentifiersif this property is expected from the API.+interface WorkWithRelatedIdentifiers extends Work { + relatedIdentifiers?: Array<{ + relatedIdentifierType: string + relatedIdentifier: string + }> +} + function extractRelatedDois(work: Work | undefined): string[] { if (!work) return [] - // Check if relatedIdentifiers exists on the work object (even if not in the type) - const workWithRelatedIdentifiers = work as any - if (!workWithRelatedIdentifiers?.relatedIdentifiers) return [] + const workWithRelatedIdentifiers = work as WorkWithRelatedIdentifiers + if (!workWithRelatedIdentifiers.relatedIdentifiers) return [] return workWithRelatedIdentifiers.relatedIdentifiers - .filter((identifier: any) => identifier.relatedIdentifierType === 'DOI') - .map((identifier: any) => identifier.relatedIdentifier) + .filter(identifier => identifier.relatedIdentifierType === 'DOI') + .map(identifier => identifier.relatedIdentifier) .filter(Boolean) }src/data/managers/ConnectionTypeManager.ts (1)
62-79: Consider using nullish coalescing (??) instead of logical OR (||) for count fallbacks.Using
|| 0could mask valid0counts if the property exists but is explicitly0. While functionally equivalent here (since0 || 0=0),?? 0is semantically more precise for handling potentially undefined values.♻️ Suggested improvement
private calculateCounts(): ConnectionTypeCounts { if (this.externalCounts?.counts) { return this.externalCounts.counts } return { - allRelated: this.work.allRelated?.totalCount || 0, - references: this.work.references?.totalCount || 0, - citations: this.work.citations?.totalCount || 0, - parts: this.work.parts?.totalCount || 0, - partOf: this.work.partOf?.totalCount || 0, - versions: this.work.versions?.totalCount || 0, - versionOf: this.work.versionOf?.totalCount || 0, - otherRelated: this.work.otherRelated?.totalCount || 0 + allRelated: this.work.allRelated?.totalCount ?? 0, + references: this.work.references?.totalCount ?? 0, + citations: this.work.citations?.totalCount ?? 0, + parts: this.work.parts?.totalCount ?? 0, + partOf: this.work.partOf?.totalCount ?? 0, + versions: this.work.versions?.totalCount ?? 0, + versionOf: this.work.versionOf?.totalCount ?? 0, + otherRelated: this.work.otherRelated?.totalCount ?? 0 } }
…constructor parameters
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/data/queries/searchDoiQuery.ts`:
- Around line 17-19: The quoteIdentifier function currently wraps the identifier
in double quotes but doesn't escape embedded double quotes, producing malformed
queries for identifiers like 10.5555/test"value; update quoteIdentifier to
escape internal double-quote characters (replace " with \" per Lucene syntax)
before surrounding the string with quotes so that functions using
quoteIdentifier (e.g., searchDoiQuery generator) produce valid queries; ensure
the replacement targets all occurrences and preserves other characters.
🧹 Nitpick comments (3)
src/data/queries/searchDoiQuery.ts (1)
40-42: UID values should be quoted for query safety.The
uidListitems are joined without quoting, but they could contain special characters. For consistency with DOI handling and to prevent query injection, consider quoting each UID.♻️ Proposed fix
const uidPart = uidList && uidList.length > 0 - ? `uid:(${uidList.join(OR)})` + ? `uid:(${uidList.map(quoteIdentifier).join(OR)})` : '';src/app/(main)/doi.org/[...doi]/RelatedContent.tsx (1)
25-36: Type assertion toanybypasses type safety.The
work as anycast sidesteps TypeScript's type checking. IfrelatedIdentifiersexists on theWorktype but isn't currently declared, consider extending the type definition insrc/data/types.tsinstead.♻️ Proposed approach
If
relatedIdentifiersis a valid field returned by the API:// In src/data/types.ts, extend Work type: interface RelatedIdentifier { relatedIdentifier: string relatedIdentifierType: string // ... other fields } // Add to Work type: relatedIdentifiers?: RelatedIdentifier[]Then update the function:
function extractRelatedDois(work: Work | undefined): string[] { if (!work) return [] - - // Check if relatedIdentifiers exists on the work object (even if not in the type) - const workWithRelatedIdentifiers = work as any - if (!workWithRelatedIdentifiers?.relatedIdentifiers) return [] - - return workWithRelatedIdentifiers.relatedIdentifiers - .filter((identifier: any) => identifier.relatedIdentifierType === 'DOI') - .map((identifier: any) => identifier.relatedIdentifier) + if (!work.relatedIdentifiers) return [] + + return work.relatedIdentifiers + .filter(identifier => identifier.relatedIdentifierType === 'DOI') + .map(identifier => identifier.relatedIdentifier) .filter(Boolean) }src/data/managers/RelatedContentManager.ts (1)
104-108: Manager instance created on every render.The hook creates a new
RelatedContentManagerinstance on every render. While this is a lightweight class with no side effects, consider memoizing if performance becomes a concern with complex re-renders.💡 Optional optimization using useMemo
+import { useMemo } from 'react' + export function useRelatedContentManager(vars: QueryVar & { id: string }, connectionType: string | undefined) { const { loading, data, error, facetsLoading } = useDoiRelatedContentQuery(vars) const connectionCounts = useConnectionCounts(vars) - return new RelatedContentManager(vars, connectionType, data, loading, error, facetsLoading, connectionCounts) + return useMemo( + () => new RelatedContentManager(vars, connectionType, data, loading, error, facetsLoading, connectionCounts), + [vars, connectionType, data, loading, error, facetsLoading, connectionCounts] + ) }
| function quoteIdentifier(identifier: string): string { | ||
| return `"${identifier}"` | ||
| } |
There was a problem hiding this comment.
quoteIdentifier doesn't escape embedded double quotes.
If the identifier contains a double quote character (e.g., 10.5555/test"value), the resulting query "10.5555/test"value" will be malformed. Consider escaping internal double quotes with a backslash per Lucene syntax.
🐛 Proposed fix
function quoteIdentifier(identifier: string): string {
- return `"${identifier}"`
+ // Escape internal double quotes for Lucene query syntax
+ const escaped = identifier.replace(/"/g, '\\"')
+ return `"${escaped}"`
}🤖 Prompt for AI Agents
In `@src/data/queries/searchDoiQuery.ts` around lines 17 - 19, The quoteIdentifier
function currently wraps the identifier in double quotes but doesn't escape
embedded double quotes, producing malformed queries for identifiers like
10.5555/test"value; update quoteIdentifier to escape internal double-quote
characters (replace " with \" per Lucene syntax) before surrounding the string
with quotes so that functions using quoteIdentifier (e.g., searchDoiQuery
generator) produce valid queries; ensure the replacement targets all occurrences
and preserves other characters.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/`(main)/doi.org/[...doi]/RelatedContent.tsx:
- Around line 54-77: The pagination URL is built using the raw doi variable when
passing the url prop to WorksListing, which can produce invalid URLs for DOIs
with reserved characters; update RelatedContent (the component rendering
WorksListing) to construct the URL from encoded DOI path segments (use
encodeURIComponent on each segment of doi.split('/') or otherwise encode the
DOI) and pass the encoded path to the url prop (replace the current
url={'/doi.org/' + doi + '/?'} expression) so all pagination links are safe and
valid.
In `@src/data/managers/RelatedContentManager.ts`:
- Around line 128-132: The current build of varsWithRelatedDois overwrites any
incoming uidList by setting uidList: relatedDois; instead merge the
caller-provided vars.uidList with relatedDois and dedupe so existing filters
aren’t lost. In the RelatedContentManager code that creates varsWithRelatedDois,
read the existing vars.uidList (if any), concat with relatedDois, remove
duplicates (e.g., via a Set) and assign that merged array to uidList; keep all
other properties from vars unchanged. Ensure safe handling when vars.uidList or
relatedDois are undefined/null.
🧹 Nitpick comments (1)
src/data/managers/RelatedContentManager.ts (1)
101-112: Avoidanywhen readingrelatedIdentifiers.
This bypasses type-safety. Consider a narrow helper type or updateWorkto includerelatedIdentifiersif the API guarantees it. Please verify the response shape.[գն
Details
♻️ Suggested tightening
+type WorkWithRelatedIdentifiers = Work & { + relatedIdentifiers?: Array<{ relatedIdentifierType?: string; relatedIdentifier?: string }> +} + function extractRelatedDois(work: Work | undefined): string[] { if (!work) return [] // Check if relatedIdentifiers exists on the work object (even if not in the type) - const workWithRelatedIdentifiers = work as any + const workWithRelatedIdentifiers = work as WorkWithRelatedIdentifiers if (!workWithRelatedIdentifiers?.relatedIdentifiers) return [] return workWithRelatedIdentifiers.relatedIdentifiers - .filter((identifier: any) => identifier.relatedIdentifierType === 'DOI') - .map((identifier: any) => identifier.relatedIdentifier) + .filter( + (identifier): identifier is { relatedIdentifierType: string; relatedIdentifier: string } => + identifier?.relatedIdentifierType === 'DOI' && !!identifier.relatedIdentifier + ) + .map((identifier) => identifier.relatedIdentifier) .filter(Boolean) // Remove any undefined/null values }
digitaldogsbody
left a comment
digitaldogsbody
left a comment
There was a problem hiding this comment.
Great work! One question about the bot protection, but non-blocking.
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
This PR significantly refactors how related content and works are fetched and managed, moving from monolithic GraphQL queries to targeted REST-based queries supported by a new client-side manager architecture.
Purpose
The primary goal is to replace complex GraphQL queries with REST API calls to improve data retrieval performance and flexibility. This change introduces a more modular approach to handling "Related Works" for DOIs, including support for new connection types like "Versions"
and "Is Version Of".
Approach
RelatedContentManager,ConnectionTypeManager, andPaginationManagerto encapsulate business logic previously handled within components or large GraphQL schemas.@tanstack/react-queryfor better caching and loading state management of DOI data./api/doi/related-list/<path:doi>to facilitate fetching related work attributes via REST.Key Modifications
RelatedContent.tsxto useuseRelatedContentManager, simplifying its internal logic.WorkFacets.tsxandWorksListing.tsxto handle more connection types (versions, versionOf).SankeyGraph.tsxto handle undefined facets gracefully.ConnectionCountManager.tsto perform parallel count-only queries for different relationship types.searchDoiQuery.tsto include a robust query builder (buildRelatedDoiQuery) for various relationship types (citations, parts, versions, etc.).WorkandWorkstypes to include explicit counts and optional facet fields.ConnectionTypeCountsandPaginationtypes.Important Technical Details
relatedIdentifiers.buildRelatedDoiQueryutility now accounts for multiple URI formats (e.g.,https://doi.org/,http://doi.org/, or the raw DOI string) to ensure comprehensive results.useSearchDoiQueryanduseSearchDoiFacetsQuery), allowing the UI to render the list while facets continue to load.Types of changes
Reviewer, please remember our guidelines:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.