Add organization relation type facets to ROR profile pages#561
Add organization relation type facets to ROR profile pages#561
Conversation
WalkthroughRemoves rorFundingIds and isBot usage; introduces OrganizationRelatedContentManager and OrganizationConnectionCountManager; adds organization-relation-type facets/counts to search and UI; wires a new RelatedContent component into the ror page; updates queries and types to use organizationRelationType and facetsLoading. Changes
Sequence DiagramsequenceDiagram
participant Page as Page (ror page)
participant RC as RelatedContent
participant MGR as OrganizationRelatedContentManager
participant Query as useOrganizationRelatedContentQuery
participant Conn as useOrganizationConnectionCounts
participant UI as WorksListing / WorkFacets
Page->>RC: render(rorId)
RC->>MGR: useOrganizationRelatedContentManager(vars)
par parallel fetches
MGR->>Query: fetch related content (vars)
MGR->>Conn: fetch per-relation counts (rorId)
end
Query-->>MGR: { data, loading, error, facetsLoading }
Conn-->>MGR: { counts, isLoading, isError }
MGR->>MGR: derive state (isLoading, facetsAreLoading, hasError, counts, selectedContent, pagination)
MGR-->>RC: manager (works, counts, loading flags, title, pagination)
alt hasError
RC->>UI: render CommonsError
else isLoading
RC->>UI: render Loading
else hasAnyRelatedWorks
RC->>UI: render WorksListing with works, counts, facet loading flags
UI->>UI: render WorkFacets (organization-relation-type) + listings
else no results
RC->>UI: render no-results
end
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
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/(main)/ror.org/[rorid]/Content.tsx (1)
26-26: 🛠️ Refactor suggestion | 🟠 MajorDead code:
rorFundingIdsis computed but never used.The
rorFundingIdsvariable is still being computed from organization identifiers, but it's no longer passed to any component or used in the render. This should be removed to avoid confusion.🧹 Proposed fix
export default function Content(props: Props) { const { rorid: rorId } = props const { data, error, loading } = useROROrganization(rorId) if (loading) return <Loading /> const organization = data?.organization || {} as OrganizationType - const rorFundingIds = organization.identifiers?.filter((id) => id.identifierType === 'fundref').map((id) => id.identifier) || [] if (error || !organization) return (src/components/WorkFacets/WorkFacets.tsx (1)
100-100:⚠️ Potential issue | 🟡 MinorPre-existing typo:
"conection-type-facets"should be"connection-type-facets".This appears to be a pre-existing typo (missing 'n' in "connection"). While not introduced by this PR, it may cause the connection-type facets accordion to not be expanded by default if the correct key is used elsewhere.
🤖 Fix all issues with AI agents
In `@src/app/`(main)/ror.org/[rorid]/mapSearchParams.ts:
- Line 31: The mapping uses searchParams['repository-type'] but the SearchParams
interface lacks that key; update the SearchParams interface (the type used by
mapSearchParams) to include "repository-type"?: string so TypeScript knows this
optional property exists, then ensure mapSearchParams still maps that value to
clientType (clientType: searchParams['repository-type']) and that any callers of
mapSearchParams pass/handle the optional string accordingly.
In `@src/app/`(main)/ror.org/[rorid]/RelatedContent.tsx:
- Around line 18-25: In RelatedContent, remove the unused rorId argument when
calling useOrganizationRelatedContentManager: keep computing vars = { rorId,
...variables } (so rorId is still part of vars) and call
useOrganizationRelatedContentManager(vars) instead of
useOrganizationRelatedContentManager(rorId, vars); update any references that
expect the two-argument call to use the single-argument form
(useOrganizationRelatedContentManager) so the component compiles and the hook
only receives the vars object.
In `@src/data/managers/OrganizationRelatedContentManager.ts`:
- Around line 120-136: The rorId parameter on
useOrganizationRelatedContentManager is unused; remove it from the function
signature and all places that call it (update any imports/usages) so the hook
becomes useOrganizationRelatedContentManager(vars: QueryVar) and continues to
return new OrganizationRelatedContentManager as before; ensure TypeScript types
and any exported signatures are updated accordingly to avoid type errors.
🧹 Nitpick comments (4)
src/data/queries/searchDoiQuery.ts (1)
21-30: Consider exportingVALID_CONNECTION_TYPESwithas constfor consistency.
VALID_ORGANIZATION_RELATION_TYPESis exported withas constfor type safety, butVALID_CONNECTION_TYPESis a local mutable array without const assertion. If this constant is ever needed elsewhere or for type inference, consider aligning patterns.♻️ Suggested alignment
-const VALID_CONNECTION_TYPES = [ +const VALID_CONNECTION_TYPES = [ 'references', 'citations', 'parts', 'partOf', 'versions', 'versionOf', 'allRelated', 'otherRelated', -]; +] as const;src/app/(main)/ror.org/[rorid]/RelatedContent.tsx (1)
64-78: Redundant loading state props passed to WorksListing.
loadingFacetsat Line 68 already includesmanager.organizationCountsLoading(via the OR condition), butorganizationRelationCountsLoadingat Line 70 passes the same value separately. This redundancy could cause confusion.Consider either:
- Not including
organizationCountsLoadinginloadingFacetsif it needs to be tracked separately- Removing
organizationRelationCountsLoadingprop ifloadingFacetsalready covers itLooking at
WorksListing.tsxLine 116, both are used in OR conditions, so the effect is the same but the intent is unclear.src/components/WorkFacets/WorkFacets.tsx (1)
38-44: Consider centralizing organization relation type definitions.The titles here (
'All related','Funded by', etc.) duplicate those informatOrganizationRelationTitle()inOrganizationRelatedContentManager.ts. If titles need to change, they must be updated in both places.Consider extracting these to a shared constant or utility to maintain a single source of truth.
src/data/managers/OrganizationRelatedContentManager.ts (1)
19-38: Redundant validation informatOrganizationRelationTitle.The check at Lines 21-23 returns
'All related'for invalid types, but thedefaultcase at Lines 35-36 does the same. Sincetypeis already defaulted to'allRelated'at Line 20, the explicit validation is unnecessary.♻️ Suggested simplification
function formatOrganizationRelationTitle(organizationRelationType: string): string { const type = organizationRelationType || 'allRelated' - if (!VALID_ORGANIZATION_RELATION_TYPES.includes(type as typeof VALID_ORGANIZATION_RELATION_TYPES[number])) { - return 'All related' - } switch (type) { case 'allRelated': return 'All related' case 'fundedBy': return 'Funded by' case 'createdBy': return 'Created by' case 'affiliatedResearcher': return 'Affiliated researcher' case 'dmp': return 'DMP' default: return 'All related' } }
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
funded-by-3
|
| Run status |
|
| Run duration | 01m 57s |
| Commit |
|
| Committer | Joseph Rhoads |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
2
|
Pending
|
3
|
Skipped
|
0
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Joseph Rhoads <jrhoads@users.noreply.github.com>
Pr 561 bot issues
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/data/queries/searchDoiQuery.ts (1)
32-92:⚠️ Potential issue | 🟡 MinorInvalid connectionType currently drops the related DOI filter.
If an unknown type slips in (e.g., URL tampering), the query becomes unscoped. Prefer defaulting to
allRelated, similar to buildOrgQuery.🐛 Proposed fix
- // if the connection type is not in the map, return an empty string - if (!VALID_CONNECTION_TYPES.includes(connectionType as typeof VALID_CONNECTION_TYPES[number])) return '' + const requestedType = connectionType ?? 'allRelated' + const relationType = VALID_CONNECTION_TYPES.includes( + requestedType as typeof VALID_CONNECTION_TYPES[number] + ) + ? requestedType + : 'allRelated' @@ - const selectedParts = queryPartsByType[connectionType as keyof typeof queryPartsByType] || []; + const selectedParts = queryPartsByType[relationType as keyof typeof queryPartsByType] || []; @@ - if (connectionType === "otherRelated") { + if (relationType === "otherRelated") { return `((${positivePart}) AND NOT (${negativePart}))`; }
🤖 Fix all issues with AI agents
In `@src/data/queries/searchDoiQuery.ts`:
- Around line 97-133: VALID_ORGANIZATION_RELATION_TYPES is missing 'dmp' while
queryPartsByType in buildOrgQuery declares a dmp key, causing a TS type error;
fix by adding 'dmp' back into the VALID_ORGANIZATION_RELATION_TYPES tuple (so it
becomes ['allRelated','affiliatedResearcher','createdBy','fundedBy','dmp'] as
const) so the Record<typeof VALID_ORGANIZATION_RELATION_TYPES[number], string[]>
typing matches the keys in queryPartsByType (no other changes to buildOrgQuery,
extractRORId, or quoteIdentifier required).
🧹 Nitpick comments (5)
src/app/(main)/ror.org/[rorid]/RelatedContent.tsx (3)
22-22: Consider improving type safety for search params.The
as anytype assertion bypasses TypeScript's type checking. Consider defining a proper type for the search params object or using a type guard to ensure type safety.
27-36: Inconsistent Container wrapping across states.The loading and error states don't use a
Containerwrapper, while the no-data (line 40) and success (line 58) states do. This may cause layout shifts or visual inconsistencies when transitioning between states.♻️ Proposed fix for consistent layout
- if (manager.isLoading) return <Row><Loading /></Row> + if (manager.isLoading) return ( + <Container fluid> + <Row><Loading /></Row> + </Container> + ) if (manager.hasError) return ( - <Row> - <Col md={{ offset: 3 }} className="panel panel-transparent"> - <CommonsError title="An error occurred loading related content." message={manager.errorMessage} /> - </Col> - </Row> + <Container fluid> + <Row> + <Col md={{ offset: 3 }} className="panel panel-transparent"> + <CommonsError title="An error occurred loading related content." message={manager.errorMessage} /> + </Col> + </Row> + </Container> )
65-77: Theloadingprop will always befalseat this point.Since line 27 returns early when
manager.isLoadingis true, theloading={manager.isLoading}prop on line 67 will always befalsewhen this render path is reached. This is technically correct but could be misleading. Consider removing it if WorksListing has a sensible default, or document that it's intentionally passed for consistency.src/data/managers/OrganizationRelatedContentManager.ts (1)
46-50: Consider usingisErroror removing it.The
isErrorproperty inExternalOrganizationCountsis passed into the manager (line 140) but doesn't appear to be used within the class. If error handling for organization counts is needed, consider exposing it via a getter; otherwise, it could be removed to reduce noise.src/data/queries/searchDoiQuery.ts (1)
327-346: Consider typing organizationRelationType to the allowed union.This improves call-site safety; if raw URL strings are needed, validate/cast at the boundary.
♻️ Suggested refactor
export interface QueryVar { query?: string filterQuery?: string rorId?: string - organizationRelationType?: string + organizationRelationType?: typeof VALID_ORGANIZATION_RELATION_TYPES[number] relatedDoi?: string connectionType?: string
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/data/managers/OrganizationRelatedContentManager.ts`:
- Around line 99-110: The hasAnyRelatedWorks getter can return false prematurely
while counts are still loading or errored; update the hasAnyRelatedWorks method
to short‑circuit when counts are not ready by checking the existing
organizationCountsLoading getter (and an error flag on organizationCounts if
available) and return a non‑empty/indeterminate truthy value (e.g., true) while
loading or errored, otherwise keep the current checks against works.totalCount
and Object.values(this.organizationRelationTypeCounts).some(...); modify the
hasAnyRelatedWorks getter (referencing works.totalCount,
organizationCountsLoading, and organizationRelationTypeCounts) to implement this
guard.
- Around line 19-30: Add the missing "dmp" relation type to the list and title
map so types align with OrganizationRelationTypeCounts: update
VALID_ORGANIZATION_RELATION_TYPES to include 'dmp' and add an entry in
ORGANIZATION_RELATION_TYPE_TITLES mapping (e.g., dmp: 'DMPs' or appropriate
label) so ORGANIZATION_RELATION_TYPE_FACETS (which maps
VALID_ORGANIZATION_RELATION_TYPES to titles) will produce a facet for dmp;
verify the keys match the OrganizationRelationType union and that the title
string is used in the existing facet generation code.
🧹 Nitpick comments (1)
src/data/queries/searchDoiQuery.ts (1)
329-329: Consider stricter typing fororganizationRelationType.Using
stringhere relies on runtime validation inbuildOrgQuery. For better type safety at the interface level, consider using the exported const type:♻️ Proposed stricter typing
export interface QueryVar { query?: string filterQuery?: string rorId?: string - organizationRelationType?: string + organizationRelationType?: typeof VALID_ORGANIZATION_RELATION_TYPES[number] relatedDoi?: string
| get hasAnyRelatedWorks(): boolean { | ||
| if (this.works.totalCount > 0) return true | ||
| return Object.values(this.organizationRelationTypeCounts).some(count => count > 0) | ||
| } | ||
|
|
||
| get organizationRelationTypeCounts(): OrganizationRelationTypeCounts { | ||
| return this.organizationCounts?.counts ?? EMPTY_ORGANIZATION_RELATION_TYPE_COUNTS | ||
| } | ||
|
|
||
| get organizationCountsLoading(): boolean { | ||
| return this.organizationCounts?.isLoading ?? false | ||
| } |
There was a problem hiding this comment.
Avoid a false “no related works” state while counts load or error.
If works.totalCount is 0 while counts are still loading (or failed), this returns false and can trigger the empty state prematurely. Consider short‑circuiting on loading/error or exposing a tri‑state.
🛠️ Suggested guard
get hasAnyRelatedWorks(): boolean {
if (this.works.totalCount > 0) return true
+ if (this.organizationCounts?.isLoading || this.organizationCounts?.isError) return true
return Object.values(this.organizationRelationTypeCounts).some(count => count > 0)
}📝 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.
| get hasAnyRelatedWorks(): boolean { | |
| if (this.works.totalCount > 0) return true | |
| return Object.values(this.organizationRelationTypeCounts).some(count => count > 0) | |
| } | |
| get organizationRelationTypeCounts(): OrganizationRelationTypeCounts { | |
| return this.organizationCounts?.counts ?? EMPTY_ORGANIZATION_RELATION_TYPE_COUNTS | |
| } | |
| get organizationCountsLoading(): boolean { | |
| return this.organizationCounts?.isLoading ?? false | |
| } | |
| get hasAnyRelatedWorks(): boolean { | |
| if (this.works.totalCount > 0) return true | |
| if (this.organizationCounts?.isLoading || this.organizationCounts?.isError) return true | |
| return Object.values(this.organizationRelationTypeCounts).some(count => count > 0) | |
| } | |
| get organizationRelationTypeCounts(): OrganizationRelationTypeCounts { | |
| return this.organizationCounts?.counts ?? EMPTY_ORGANIZATION_RELATION_TYPE_COUNTS | |
| } | |
| get organizationCountsLoading(): boolean { | |
| return this.organizationCounts?.isLoading ?? false | |
| } |
🤖 Prompt for AI Agents
In `@src/data/managers/OrganizationRelatedContentManager.ts` around lines 99 -
110, The hasAnyRelatedWorks getter can return false prematurely while counts are
still loading or errored; update the hasAnyRelatedWorks method to short‑circuit
when counts are not ready by checking the existing organizationCountsLoading
getter (and an error flag on organizationCounts if available) and return a
non‑empty/indeterminate truthy value (e.g., true) while loading or errored,
otherwise keep the current checks against works.totalCount and
Object.values(this.organizationRelationTypeCounts).some(...); modify the
hasAnyRelatedWorks getter (referencing works.totalCount,
organizationCountsLoading, and organizationRelationTypeCounts) to implement this
guard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
master
|
| Run status |
|
| Run duration | 01m 59s |
| Commit |
|
| Committer | Joseph Rhoads |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
3
|
|
Skipped
|
0
|
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
3 participants
closes: https://github.com/datacite/product-backlog/issues/515
Purpose
This PR updates the organization (ROR) profile pages to introduce "Organization Relation Types" facets. This allows users to filter related works by how they are connected to the organization (e.g., whether the organization funded the work, created it, or is the affiliation for the researchers).
Approach
The implementation introduces a new
OrganizationRelatedContentManagerandOrganizationConnectionCountManagerto handle the complexity of fetching and displaying different types of institutional relationships. The query logic for ROR identifiers was refactored to support specific fields (likefunder_rors,funder_parent_rors, andaffiliation_id) instead of a generic catch-all query.Key Modifications
buildOrgQueryinsearchDoiQuery.tsto support granular filtering based on relation types (fundedBy,createdBy,affiliatedResearcher,dmp).OrganizationConnectionCountManager: Fetches total counts for all relation types in parallel usinguseQueries.OrganizationRelatedContentManager: Coordinates state between work data, pagination, and relation facets.WorkFacetsto display a new "Organization Relation Types" facet group.WorksListingto handle loading states for these specific organization facets.RelatedContentin the ROR route to use these new managers and provided a cleaner layout.mapSearchParamsto includeorganization-relation-typeandsortparameters from the URL.Important Technical Details
funder_rorsandfunder_parent_rorsfor funding relations, replacing the previousrorFundingIdsapproach.page.tsx) for better component isolation.Types of changes
Reviewer, please remember our guidelines:
Note
Medium Risk
Changes the DOI search query construction for ROR filtering and adds parallel count-only API requests to drive new facets; mistakes here could alter search results or increase load/latency on the related-works view.
Overview
Adds an Organization Relation Type radio facet on ROR profile related-works listings, allowing filtering by how a work is connected to the organization (e.g.
fundedBy,createdBy,affiliatedResearcher).Refactors ROR related-works fetching to use a new
OrganizationRelatedContentManager+OrganizationConnectionCountManager, which fetches per-relation-type counts via parallel count-only DOI queries, improves loading/error/empty states, and updatessearchDoiQuery.buildOrgQueryto generate relation-specific OpenSearch/Lucene filters (removing the priorrorFundingIds-based approach).Written by Cursor Bugbot for commit 92222dd. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements