Funded by #539
Funded by #539
Conversation
WalkthroughRemoves rorFundingIds and RelatedContent embedding from Content, introduces organizationRelationType and clientId in mapped search params, threads vars and showMetrics into WorksListing → SummarySearchMetrics → MetricsDisplay, adds InfoTooltip and organization-relation-type facet, updates facets/types/styles, and bumps Next.js. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant mapSP as mapSearchParams
participant Page as ROR Page
participant Related as RelatedContent
participant Works as WorksListing
participant Query as searchDoiQuery
participant API as Data/API
participant Metrics as SummarySearchMetrics/MetricsDisplay
Browser->>mapSP: request URL + search params (organization-relation-type, client-id)
mapSP-->>Page: return variables (organizationRelationType, clientId, ...)
Page->>Related: render RelatedContent with rorId
Related->>Related: build vars (exclude rorFundingIds) and compute showMetrics
Related->>Works: pass vars + showMetrics
Works->>Query: buildQuery using rorId + variables.organizationRelationType
alt relation-specific fragment
Query->>Query: construct chosen relation fragment (fundedBy / createdBy / affiliatedResearchers / default union)
end
Query->>API: execute constructed query
API-->>Works: return works/results (+ meta.clients)
Works->>Metrics: if showMetrics then render SummarySearchMetrics with vars
Metrics->>Metrics: set displayWorksTotal based on variables and render MetricsDisplay
Works->>Works: render works/results UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/components/WorkFacets/WorkFacets.tsx (4)
⏰ 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)
🔇 Additional comments (5)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/SummarySearchMetrics/SummarySearchMetrics.tsx (1)
38-48: Potential logic mismatch:displayWorksTotalwill be false whenorganizationRelationTypedefaults to'allRelated'.If
'allRelated'is meant to behave like “no special filter”, consider aligning semantics, e.g. treat only specific relation filters as the signal:- const displayWorksTotal = !variables.organizationRelationType + const displayWorksTotal = + !variables.organizationRelationType || variables.organizationRelationType === 'allRelated'src/app/(main)/ror.org/[rorid]/mapSearchParams.ts (1)
1-30: FixSearchParamstyping: missing"repository-type"and"organization-relation-type"keys.This file types
searchParams, so indexing with undeclared keys should fail TS checks. Add the keys: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 + "repository-type"?: string + "organization-relation-type"?: string isBot: string }src/app/(main)/ror.org/[rorid]/page.tsx (1)
1-62: Pass theisBotprop toRelatedContentto ensure bot traffic doesn't load related works data. Currently,isBotdefaults tofalseacross all page renders, making the bot-detection check inRelatedContentnon-functional. Without this prop, search engines and crawlers will fetch and render the full related works list. TheuseSearchParams()usage and metrics logic are correct.src/components/MetricsDisplay/MetricsDisplay.tsx (1)
16-27:linksis optional but dereferenced—can crash at runtime / violates TS intent.
links?: { ... }butmetricsDatauseslinks.worksetc. This should belinks?.works(same for others), or makelinksrequired.const metricsData = [ { "label": "Works", "count": counts.works, - "link": links.works + "link": links?.works }, { "label": "Citations", "count": counts.citations, - "link": links.citations + "link": links?.citations }, { "label": "Views", "count": counts.views, - "link": links.views + "link": links?.views }, { "label": "Downloads", "count": counts.downloads, - "link": links.downloads + "link": links?.downloads } ];Also applies to: 28-57
🧹 Nitpick comments (3)
src/components/WorksDashboard/WorksDashboard.tsx (1)
43-45: Prefer plain string + verify layout parity after removing the CSS-module container styles.
className={'mt-5 mb-5'}can be simplified toclassName="mt-5 mb-5". More importantly, since this replaces the oldgraphsContainerCSS-module styling, please verify there’s no regression in chart spacing/wrapping across breakpoints (xs/sm) and in the OrganizationDashboard embedding case.- <div className={'mt-5 mb-5'}> + <div className="mt-5 mb-5">src/data/queries/searchDoiQuery.ts (2)
13-35: TightenorganizationRelationTypeto a union type (avoid “stringly-typed” switch).Right now any string will fall into
default, which can mask invalid values coming from routing/UI. Consider exporting a shared union type and using it in bothQueryVarandbuildOrgQuery.-function buildOrgQuery(rorId: string | undefined, organizationRelationType: string | undefined): string { +export type OrganizationRelationType = + | 'allRelated' + | 'fundedBy' + | 'createdContributedOrPublishedBy' + | 'createdOrContributedByAffiliatedResearcher' + | 'connectedToOrganizationOMPs' + +function buildOrgQuery( + rorId: string | undefined, + organizationRelationType: OrganizationRelationType | undefined +): string {
226-243: UpdateQueryVar.organizationRelationTypeto the shared union type.export interface QueryVar { query?: string filterQuery?: string rorId?: string, - organizationRelationType?: string, + organizationRelationType?: OrganizationRelationType, userId?: string clientId?: string cursor?: string contributor?: string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/app/(main)/ror.org/[rorid]/Content.tsx(1 hunks)src/app/(main)/ror.org/[rorid]/RelatedContent.tsx(2 hunks)src/app/(main)/ror.org/[rorid]/mapSearchParams.ts(1 hunks)src/app/(main)/ror.org/[rorid]/page.tsx(2 hunks)src/components/FacetList/FacetList.tsx(2 hunks)src/components/FacetList/FacetListItem.tsx(3 hunks)src/components/InfoTooltip/InfoTooltip.tsx(1 hunks)src/components/MetricsDisplay/MetricsDisplay.module.scss(2 hunks)src/components/MetricsDisplay/MetricsDisplay.tsx(2 hunks)src/components/SummarySearchMetrics/SummarySearchMetrics.tsx(1 hunks)src/components/WorkFacets/WorkFacets.tsx(4 hunks)src/components/WorksDashboard/WorksDashboard.module.scss(0 hunks)src/components/WorksDashboard/WorksDashboard.tsx(1 hunks)src/components/WorksListing/WorksListing.tsx(3 hunks)src/data/queries/searchDoiQuery.ts(2 hunks)src/data/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/WorksDashboard/WorksDashboard.module.scss
🧰 Additional context used
🧬 Code graph analysis (10)
src/components/SummarySearchMetrics/SummarySearchMetrics.tsx (1)
src/components/MetricsDisplay/MetricsDisplay.tsx (1)
MetricsDisplay(26-65)
src/data/types.ts (3)
src/components/SankeyGraph/SankeyGraph.tsx (1)
facet(25-42)src/components/OrganizationDashboard/OrganizationDashboard.tsx (1)
ContentFacet(12-16)src/data/clients/ror-v2-client.ts (1)
RORFacet(85-89)
src/app/(main)/ror.org/[rorid]/RelatedContent.tsx (1)
src/app/(main)/ror.org/[rorid]/mapSearchParams.ts (1)
mapSearchparams(16-34)
src/components/WorkFacets/WorkFacets.tsx (3)
src/data/types.ts (1)
Facet(325-330)src/components/FacetList/FacetList.tsx (1)
FacetList(25-52)src/data/queries/searchOrganizationQuery.ts (2)
convertRORTypeFacet(205-211)convertRORCountriesFacet(197-203)
src/app/(main)/ror.org/[rorid]/mapSearchParams.ts (3)
src/app/(main)/repositories/[...repoid]/mapSearchParams.ts (2)
mapSearchparams(16-33)SearchParams(1-14)src/app/(main)/orcid.org/[orcid]/mapSearchParams.ts (2)
mapSearchparams(15-31)SearchParams(1-13)src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (2)
mapSearchparams(17-34)SearchParams(1-15)
src/app/(main)/ror.org/[rorid]/page.tsx (1)
src/app/(main)/ror.org/[rorid]/RelatedContent.tsx (1)
RelatedContent(22-77)
src/components/InfoTooltip/InfoTooltip.tsx (1)
src/components/HelpIcon/HelpIcon.tsx (1)
Icon(23-36)
src/components/FacetList/FacetListItem.tsx (2)
src/data/types.ts (1)
Facet(325-330)src/components/InfoTooltip/InfoTooltip.tsx (1)
InfoTooltip(11-24)
src/data/queries/searchDoiQuery.ts (1)
src/data/queries/searchOrganizationQuery.ts (1)
QueryVar(278-283)
src/components/MetricsDisplay/MetricsDisplay.tsx (2)
src/components/MetricsCounter/MetricsCounter.tsx (1)
MetricsCounter(13-44)src/components/RepositoryDetail/RepositoryDetail.tsx (1)
Metrics(23-33)
🪛 GitHub Check: CodeQL
src/components/WorkFacets/WorkFacets.tsx
[failure] 105-105: Incomplete URL substring sanitization
'ror.org/' can be anywhere in the URL, and arbitrary hosts may come before or after it.
⏰ 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). (2)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (4)
🔇 Additional comments (5)
src/components/MetricsDisplay/MetricsDisplay.module.scss (1)
1-25: SCSS changes look safe for layout (padding/centering only).src/components/FacetList/FacetList.tsx (1)
8-47: Good centralization of tooltip UX; just ensure InfoTooltip a11y fix lands.src/data/types.ts (1)
325-334: [rewritten comment]
[classification tag]src/data/queries/searchDoiQuery.ts (1)
39-54: Inconsistent quoting on line 20 may cause unintended query behavior. Theorganization_idfield uses an unquoted ROR format (ror.org/02k3smh20) while theprovider.ror_idfield uses a quoted URL ("https://ror.org/02k3smh20"). This inconsistency within the same clause could produce incorrect Elasticsearch results if the backend requires consistent formatting. Additionally, these field names (funder_rors,provider.ror_id,organization_id,affiliation_id,related_dmp_organization_id) do not appear in DataCite's public API documentation; verify with backend Elasticsearch mappings that these fields are correct and accept the mixed quoting patterns used here.src/components/WorkFacets/WorkFacets.tsx (1)
59-70: Brittleurl?.includes('ror.org/')gate (CodeQL) + duplicate facet titles.
- Line 105: The substring check can be accidentally true for unintended URLs; prefer a stricter predicate like
url?.startsWith('/ror.org/')or pass an explicitisRorPageboolean from the route layer.- Lines 96 and 108: Both
FacetListcomponents use the title"Connection Types", creating duplicate headers. The second should be"Organization Relationships"or similar to clarify the distinction.Example tightening for line 105-108:
- {url?.includes('ror.org/') && ( + {url?.startsWith('/ror.org/') && ( <FacetList data={organizationRelationTypeList} - title="Connection Types" + title="Organization Relationships" id="organization-relation-type-facets" param="organization-relation-type" url={url} checked={(i) => !isOrganizationRelationTypeSet && i == 0} radio /> )}Also:
defaultActiveKeys(line 72–84) contains a typo on line 81:"conection-type-facets"(missing 'n') does not match any rendered FacetListidand makes accordion defaults harder to reason about. Remove or correct to"connection-type-facets".Likely an incorrect or invalid review comment.
| const { isBot = false, rorId } = props | ||
|
|
||
| const searchParams = useSearchParams() | ||
| const { variables } = mapSearchparams(Object.fromEntries(searchParams.entries()) as any) | ||
|
|
||
| const vars = { rorId, rorFundingIds, ...variables } | ||
| const vars = { rorId, ...variables } | ||
| const { loading, data, error } = useOrganizationRelatedContentQuery(vars) | ||
|
|
||
| const showMetrics = Boolean(vars.organizationRelationType && vars.organizationRelationType !== 'allRelated') | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(tsx|ts)$" | grep -i "ror" | head -20Repository: datacite/akita
Length of output: 517
🏁 Script executed:
fd . -type f -name "*.tsx" | head -30Repository: datacite/akita
Length of output: 228
🏁 Script executed:
find . -type f -name "*.tsx" 2>/dev/null | wc -lRepository: datacite/akita
Length of output: 61
🏁 Script executed:
find . -path "*ror.org*" -type f 2>/dev/null | head -30Repository: datacite/akita
Length of output: 282
🏁 Script executed:
find . -path "*RelatedContent*" -type f 2>/dev/nullRepository: datacite/akita
Length of output: 476
🏁 Script executed:
rg "const { isBot = false, rorId }" -t tsx -t ts -A 10Repository: datacite/akita
Length of output: 178
🏁 Script executed:
cat -n src/app/\(main\)/ror.org/\[rorid\]/RelatedContent.tsx | head -80Repository: datacite/akita
Length of output: 2873
🏁 Script executed:
wc -l src/app/\(main\)/ror.org/\[rorid\]/RelatedContent.tsxRepository: datacite/akita
Length of output: 110
🏁 Script executed:
sed -n '1,80p' src/app/\(main\)/ror.org/\[rorid\]/RelatedContent.tsx | cat -nRepository: datacite/akita
Length of output: 2873
🏁 Script executed:
head -80 "src/app/(main)/ror.org/[rorid]/RelatedContent.tsx" | cat -nRepository: datacite/akita
Length of output: 2873
🏁 Script executed:
find . -name "WorksListing*" -type f | head -10Repository: datacite/akita
Length of output: 104
🏁 Script executed:
cat -n src/app/\(main\)/ror.org/\[rorid\]/mapSearchParams.tsRepository: datacite/akita
Length of output: 1279
🏁 Script executed:
fd WorksListing --extension tsx --extension ts | head -5Repository: datacite/akita
Length of output: 102
🏁 Script executed:
head -100 "src/app/(main)/ror.org/[rorid]/mapSearchParams.ts"Repository: datacite/akita
Length of output: 1027
🏁 Script executed:
wc -l src/components/WorksListing/WorksListing.tsxRepository: datacite/akita
Length of output: 106
🏁 Script executed:
head -150 src/components/WorksListing/WorksListing.tsx | cat -nRepository: datacite/akita
Length of output: 4378
🏁 Script executed:
find . -name "SummarySearchMetrics*" -type fRepository: datacite/akita
Length of output: 120
🏁 Script executed:
head -80 src/components/SummarySearchMetrics/SummarySearchMetrics.tsx | cat -nRepository: datacite/akita
Length of output: 1956
🏁 Script executed:
grep -n "if (!data) return" src/app/\(main\)/*/\[*\]/RelatedContent.tsxRepository: datacite/akita
Length of output: 361
Fix if (!data) return to return null explicitly in RelatedContent components.
Line 44 returns undefined implicitly; React components should explicitly return null. This same pattern exists in the other three RelatedContent files (orcid.org, doi.org, and repositories).
The vars shape is correctly formed—mapSearchParams ensures organizationRelationType is present (defaulting to 'allRelated'), and it's properly spread into SummarySearchMetrics which expects it.
🤖 Prompt for AI Agents
In src/app/(main)/ror.org/[rorid]/RelatedContent.tsx around lines 23-32 (and
similarly update the other three RelatedContent files for orcid.org, doi.org,
and repositories), the component currently does "if (!data) return" which
returns undefined; change this to "if (!data) return null" so the React
component explicitly returns null when no data is available; ensure the same
explicit null return is applied at the early-return site (around line ~44) in
each RelatedContent file.
| import SummarySearchMetrics from '../SummarySearchMetrics/SummarySearchMetrics' | ||
| import { SearchParams } from 'next/dist/server/request/search-params' | ||
|
|
||
| interface Props { | ||
| works: Works | ||
| vars: SearchParams | ||
| showMetrics?: boolean | ||
| showAnalytics: boolean |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the WorksListing.tsx file
find . -type f -name "WorksListing.tsx" | head -5Repository: datacite/akita
Length of output: 104
🏁 Script executed:
# Also find SummarySearchMetrics component
find . -type f -name "*SummarySearchMetrics*" | head -10Repository: datacite/akita
Length of output: 120
🏁 Script executed:
cat -n ./src/components/WorksListing/WorksListing.tsxRepository: datacite/akita
Length of output: 4378
🏁 Script executed:
cat -n ./src/components/SummarySearchMetrics/SummarySearchMetrics.tsxRepository: datacite/akita
Length of output: 1956
🏁 Script executed:
# Search for QueryVar type definition
rg -A 10 "type QueryVar" --type ts --type tsxRepository: datacite/akita
Length of output: 85
🏁 Script executed:
# Also check what SearchParams type actually is
rg -B 5 -A 10 "type SearchParams\|interface SearchParams" --type ts --type tsxRepository: datacite/akita
Length of output: 85
🏁 Script executed:
# Search for QueryVar without type flag
rg "type QueryVar|export.*QueryVar" src/data/queries/Repository: datacite/akita
Length of output: 4163
🏁 Script executed:
# Find and read the searchDoiQuery file
find . -name "searchDoiQuery*" -type fRepository: datacite/akita
Length of output: 94
🏁 Script executed:
# Read the searchDoiQuery file to see QueryVar definition
head -100 ./src/data/queries/searchDoiQuery.tsRepository: datacite/akita
Length of output: 4344
🏁 Script executed:
# Search for the QueryVar interface definition specifically
rg -A 30 "export interface QueryVar" ./src/data/queries/searchDoiQuery.tsRepository: datacite/akita
Length of output: 853
🏁 Script executed:
# Search for all usages of SummarySearchMetrics in the codebase
rg "SummarySearchMetrics" --no-heading -B 2 -A 2Repository: datacite/akita
Length of output: 2975
🏁 Script executed:
# Check what SearchParams type is from Next.js
rg "SearchParams" --no-heading -B 2 -A 5Repository: datacite/akita
Length of output: 34209
🏁 Script executed:
grep -B 5 -A 5 "SummarySearchMetrics rorId" ./src/app/\(main\)/ror.org/\[rorid\]/Content.tsxRepository: datacite/akita
Length of output: 472
🏁 Script executed:
# Let me also check the exact imports and how SearchParams is being used
head -30 ./src/app/\(main\)/page.tsxRepository: datacite/akita
Length of output: 835
🏁 Script executed:
# Find where WorksListing is being called to see what type is passed for vars
rg -B 5 -A 5 "WorksListing" src/app --no-heading | head -80Repository: datacite/akita
Length of output: 6774
🏁 Script executed:
# Check if there are any TypeScript errors being reported
# Let's look at one of the calling sites more carefully
cat -n ./src/app/\(main\)/repositories/\[...\repoid\]/RelatedContent.tsx | head -60Repository: datacite/akita
Length of output: 2299
🏁 Script executed:
# Let's see the complete ROR RelatedContent.tsx to see how vars is passed to WorksListing
cat -n ./src/app/\(main\)/ror.org/\[rorid\]/RelatedContent.tsx | head -100Repository: datacite/akita
Length of output: 2873
🏁 Script executed:
# Also check the mapSearchParams to understand the vars type better
cat -n ./src/app/\(main\)/ror.org/\[rorid\]/mapSearchParams.tsRepository: datacite/akita
Length of output: 1279
Replace internal Next.js import with local type; pass vars as single prop to SummarySearchMetrics instead of spreading.
import { SearchParams } from 'next/dist/server/request/search-params' imports from Next.js internal API, which is unstable. The actual vars passed at call sites (e.g., line 64 in ror.org/[rorid]/RelatedContent.tsx) is a constructed object like { rorId, ...variables }, not Next.js's SearchParams type.
Additionally, <SummarySearchMetrics {...vars} /> spreads individual props, but SummarySearchMetrics expects a single variables parameter of type QueryVar. The spread pattern works at runtime but is misleading and inconsistent with other call sites (e.g., SummarySearchMetrics rorId={...}).
Replace with:
-import { SearchParams } from 'next/dist/server/request/search-params'
+type SearchParamsLike = Record<string, string | string[] | undefined>
interface Props {
works: Works
- vars: SearchParams
+ vars: SearchParamsLike
showMetrics?: boolean
...
}
...
- <SummarySearchMetrics {...vars}/>
+ <SummarySearchMetrics variables={vars} />Also applies to: 39-56, 82-85
🤖 Prompt for AI Agents
In src/components/WorksListing/WorksListing.tsx around lines 17-24 (and also
adjust usages in ranges 39-56 and 82-85), the component imports Next.js internal
SearchParams and spreads vars into SummarySearchMetrics; replace the internal
import with the local QueryVar type (or the correct local type used elsewhere),
change the Props signature to declare vars: QueryVar (or the appropriate local
type), and update the SummarySearchMetrics call to pass the vars object as a
single prop (e.g., variables={vars}) instead of spreading {...vars}; ensure any
other occurrences in the specified ranges follow the same pattern so types and
prop names match SummarySearchMetrics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/WorkFacets/WorkFacets.tsx (1)
105-105: Incomplete URL substring sanitization.The
url?.includes('ror.org/')check is vulnerable to URL manipulation. As noted by CodeQL, this substring can appear anywhere in a malicious URL (e.g.,https://evil.com/ror.org/orhttps://ror.org.evil.com/path).Consider using a more robust check:
- {url?.includes('ror.org/') && ( + {url?.startsWith('ror.org/') && (Or if the URL may include a protocol prefix:
const isRorOrgUrl = (url: string) => { try { const parsed = new URL(url.startsWith('http') ? url : `https://${url}`) return parsed.hostname === 'ror.org' || parsed.hostname.endsWith('.ror.org') } catch { return url.startsWith('ror.org/') } }
🧹 Nitpick comments (1)
src/components/WorkFacets/WorkFacets.tsx (1)
72-84: Pre-existing typo indefaultActiveKeys.The new additions look correct. However, note that line 81 has a pre-existing typo:
"conection-type-facets"is missing an 'n' (should match"connection-type-facets"from line 74). Consider fixing this while you're modifying the array."field-of-science-facets", "registration-agency-facets", - "conection-type-facets", + "connection-type-facets", "repository-type-facets", "organization-relation-type-facets"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/FacetList/FacetListItem.tsx(3 hunks)src/components/MetricsDisplay/MetricsDisplay.tsx(1 hunks)src/components/WorkFacets/WorkFacets.tsx(4 hunks)src/data/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/MetricsDisplay/MetricsDisplay.tsx
- src/components/FacetList/FacetListItem.tsx
- src/data/types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/WorkFacets/WorkFacets.tsx (3)
src/data/types.ts (1)
Facet(325-330)src/components/FacetList/FacetList.tsx (1)
FacetList(25-52)src/data/queries/searchOrganizationQuery.ts (2)
convertRORTypeFacet(205-211)convertRORCountriesFacet(197-203)
🪛 GitHub Check: CodeQL
src/components/WorkFacets/WorkFacets.tsx
[failure] 105-105: Incomplete URL substring sanitization
'ror.org/' can be anywhere in the URL, and arbitrary hosts may come before or after it.
⏰ 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 (4)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (1)
🔇 Additional comments (3)
src/components/WorkFacets/WorkFacets.tsx (3)
59-66: LGTM - Static facet options with tooltips.The static facet list for organization relation types is well-structured. The
count: 0values are appropriate since these are filter options rather than dynamically counted facets. The tooltips provide helpful context for users.
68-69: LGTM - Consistent pattern for param detection.The new
isOrganizationRelationTypeSetfollows the same pattern as the existingisConnectionTypeSet, maintaining consistency in how URL params are detected.
94-102: LGTM - Good defensive filtering.Filtering out facets with zero or undefined counts is a sensible UX improvement, ensuring users only see actionable filter options.
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
funded-by
|
| Run status |
|
| Run duration | 01m 44s |
| 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 ↗︎ | |
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>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/WorkFacets/WorkFacets.tsx (2)
106-114: Duplicate title issue remains unresolved from previous review.Both the organization relation type facet (line 108) and the existing connection type facet (line 96) use the title "Connection Types". As noted in the previous review, this creates user confusion when both facets could appear in the interface.
The previous review suggested changing this to a distinct title like "Organization Relation" or "Work Relationship". Please address this feedback:
<FacetList data={organizationRelationTypeList} - title="Connection Types" + title="Organization Relation" id="organization-relation-type-facets"
105-105: URL substring check is vulnerable to host confusion.The condition
url?.includes('ror.org/')will match any URL containing that substring, including malicious URLs likehttps://attacker.com/ror.org/orhttps://ror.org.evil.com/. This could enable unintended facet display or bypass security checks.Apply this fix to validate the URL properly:
- {url?.includes('ror.org/') && ( + {url?.startsWith('ror.org/') && ( <FacetListOr for more robust validation:
+ {(() => { + try { + const urlObj = new URL(url, 'https://example.com') + return urlObj.hostname === 'ror.org' || url?.startsWith('ror.org/') + } catch { + return url?.startsWith('ror.org/') + } + })() && ( <FacetList
🧹 Nitpick comments (1)
src/components/MetricsDisplay/MetricsDisplay.tsx (1)
51-57: Refactor to filter before mapping instead of returning null.The current implementation filters metrics by count, then returns
nullinside themapcallback when hiding the Works metric. While React handlesnullgracefully, this pattern leavesnullitems in the array and is less maintainable than filtering beforehand.Apply this diff to filter both conditions before mapping:
- const metricList = metricsData.filter(metric => metric.count && metric.count > 0).map((metric, index) => - metric.label === "Works" && !displayWorksTotal ? null : + const metricList = metricsData + .filter(metric => metric.count && metric.count > 0) + .filter(metric => !(metric.label === "Works" && !displayWorksTotal)) + .map((metric, index) => <React.Fragment key={"metric-" + index}> <dd>{compactNumbers(metric.count || 0)}</dd> <dt>{metric.label} <HelpIcon link={metric.link} size={20} position='inline' /></dt> </React.Fragment> )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/MetricsDisplay/MetricsDisplay.tsx(2 hunks)src/components/WorkFacets/WorkFacets.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/WorkFacets/WorkFacets.tsx (3)
src/data/types.ts (1)
Facet(325-330)src/components/FacetList/FacetList.tsx (1)
FacetList(25-52)src/components/OrganizationDashboard/OrganizationDashboard.tsx (1)
ContentFacet(12-16)
src/components/MetricsDisplay/MetricsDisplay.tsx (2)
src/components/SummarySearchMetrics/SummarySearchMetrics.tsx (1)
SummarySearchMetrics(38-46)src/components/RepositoryDetail/RepositoryDetail.tsx (1)
Metrics(23-33)
🪛 GitHub Check: CodeQL
src/components/WorkFacets/WorkFacets.tsx
[failure] 105-105: Incomplete URL substring sanitization
'ror.org/' can be anywhere in the URL, and arbitrary hosts may come before or after it.
⏰ 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 (1)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (3)
🔇 Additional comments (5)
src/components/WorkFacets/WorkFacets.tsx (3)
69-69: LGTM!The implementation correctly follows the existing pattern for
isConnectionTypeSeton line 68.
82-83: LGTM!The additions correctly register the facet IDs so they expand by default in the accordion.
59-66: This is not an incomplete implementation—organizationRelationTypeListserves a different purpose thanconnectionTypeList.The
organizationRelationTypeparameter is a static filter option (applied to the search query viabuildOrgQuery()), not a result facet showing distribution. UnlikeconnectionTypeListwhich displays dynamic counts of works in each relationship type,organizationRelationTypeListprovides a fixed set of filter choices with hardcoded zero counts by design. No filtering is needed because these are not result facets but UI options for users to narrow their search.That said, the design pattern is potentially confusing: both are typed as
Facet[]with count fields, yet used differently. Consider clarifying the distinction—either through naming (e.g.,organizationRelationTypeFilters), documentation, or a separate type to distinguish static filter menus from dynamic result facets.src/components/MetricsDisplay/MetricsDisplay.tsx (2)
22-24: LGTM!The new optional prop follows TypeScript best practices and maintains backward compatibility.
26-26: LGTM!The default value of
trueensures backward compatibility for existing usages.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/WorkFacets/WorkFacets.tsx (1)
108-108: Title change addresses previous review feedback.The title has been updated from "Connection Types" to "Organization Relation Types", which resolves the duplicate title issue flagged in the previous review.
🧹 Nitpick comments (2)
src/components/FacetList/FacetListItem.tsx (1)
84-84: Consider conditionally rendering the entire span element.When
facet.countis 0, an empty<span>element is rendered. While harmless, it's cleaner to conditionally render the entire element.Apply this diff:
- <span>{facet.count > 0 && facet.count.toLocaleString('en-US')}</span> + {facet.count > 0 && <span>{facet.count.toLocaleString('en-US')}</span>}src/components/WorkFacets/WorkFacets.tsx (1)
81-81: Pre-existing typo in defaultActiveKeys.Line 81 contains a typo:
"conection-type-facets"should be"connection-type-facets"(missing an 'n'). While this is not introduced by the current changes, fixing it would ensure the connection type facet expands by default as intended.- "conection-type-facets", + "connection-type-facets",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/FacetList/FacetListItem.tsx(4 hunks)src/components/MetricsDisplay/MetricsDisplay.module.scss(1 hunks)src/components/WorkFacets/WorkFacets.tsx(3 hunks)src/components/WorksListing/WorksListing.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/WorksListing/WorksListing.tsx
- src/components/MetricsDisplay/MetricsDisplay.module.scss
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/FacetList/FacetListItem.tsx (3)
src/data/types.ts (1)
Facet(325-330)src/components/InfoTooltip/InfoTooltip.tsx (1)
InfoTooltip(11-24)src/components/FacetList/FacetList.tsx (2)
FacetList(44-70)FacetListProps(14-27)
src/components/WorkFacets/WorkFacets.tsx (4)
src/data/types.ts (1)
Facet(325-330)src/components/FacetList/FacetList.tsx (1)
FacetList(25-52)src/data/queries/searchOrganizationQuery.ts (2)
convertRORTypeFacet(205-211)convertRORCountriesFacet(197-203)src/data/clients/ror-v2-client.ts (1)
RORV2Organization(7-24)
🪛 GitHub Check: CodeQL
src/components/WorkFacets/WorkFacets.tsx
[failure] 105-105: Incomplete URL substring sanitization
'ror.org/' can be anywhere in the URL, and arbitrary hosts may come before or after it.
⏰ 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 (2)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (1)
🔇 Additional comments (4)
src/components/FacetList/FacetListItem.tsx (4)
4-4: LGTM: Imports correctly support the new navigation and tooltip features.The addition of
useRouterfor programmatic navigation andInfoTooltipfor conditional tooltip rendering are appropriate for the refactored implementation.Also applies to: 13-13
38-38: LGTM: Router initialization is correct for Next.js 15.The
useRouterhook fromnext/navigationis properly initialized and used for programmatic navigation in thehandleClickhandler.
59-68: LGTM: Event handlers correctly implement accessible navigation.The
handleClickusesrouter.pushwithscroll: false, which is appropriate for facet selection (prevents page scroll). ThehandleKeyDownhandler properly supports Enter and Space keys withpreventDefault(), following accessibility best practices for button-role elements.
19-19: No action required. TheFacetList.tsxcomponent properly passestooltipText={facet.tooltipText}toFacetListItem, so the tooltip will render when the data is available.Likely an incorrect or invalid review comment.
| <div | ||
| onClick={handleClick} | ||
| onKeyDown={handleKeyDown} | ||
| className={styles.facetlink} | ||
| role="button" | ||
| tabIndex={0} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Link removal resolves previous navigation issue, but add aria-pressed for toggle state.
Replacing the Link with a clickable div addresses the previous review concern about stopPropagation() not preventing Link navigation. The accessibility attributes (role="button", tabIndex={0}) are good, but the element represents a toggle/checkbox-like control. Add aria-pressed or aria-checked to indicate the checked state for screen readers.
Apply this diff to add the aria attribute:
<div
onClick={handleClick}
onKeyDown={handleKeyDown}
className={styles.facetlink}
role="button"
tabIndex={0}
+ aria-pressed={params.get(param) == value}
>Based on past review comments, the Link removal resolves the stopPropagation navigation issue.
📝 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.
| <div | |
| onClick={handleClick} | |
| onKeyDown={handleKeyDown} | |
| className={styles.facetlink} | |
| role="button" | |
| tabIndex={0} | |
| > | |
| <div | |
| onClick={handleClick} | |
| onKeyDown={handleKeyDown} | |
| className={styles.facetlink} | |
| role="button" | |
| tabIndex={0} | |
| aria-pressed={params.get(param) == value} | |
| > |
🤖 Prompt for AI Agents
In src/components/FacetList/FacetListItem.tsx around lines 72 to 78, the
clickable div has role="button" and tabIndex but lacks an accessible state
indicator; add aria-pressed tied to the item's selected state (e.g.
aria-pressed={isSelected}) so screen readers know it’s a toggle. Ensure the prop
or local variable used (isSelected) is a boolean and add
aria-pressed={isSelected} to the div element.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
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 creates a type safety gap.Apply this diff to add the missing property:
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 properties that are accessed on lines 28-30:"client-id","repository-type", and"organization-relation-type". This creates type safety gaps.Apply this diff to add the missing properties:
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 property to SearchParams interface.The
SearchParamsinterface is missing the"client-id"property that is accessed on line 29. This creates a type safety gap.Apply this diff to add the missing property:
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 }
♻️ Duplicate comments (1)
src/components/WorkFacets/WorkFacets.tsx (1)
83-85: Note: Typo in line 82 affects defaultActiveKeys.The typo
"conection-type-facets"on line 82 (missing an 'n') means this facet key won't match the actualid="connection-type-facets"used on line 99, preventing the connection type facet from being expanded by default.This was flagged in a previous review but bears repeating since it affects functionality. Apply this fix:
"registration-agency-facets", - "conection-type-facets", + "connection-type-facets", "repository-type-facets",
🧹 Nitpick comments (1)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
29-29: Consider naming consistency: repositoryId vs clientId.This file uses
repositoryIdwhile other mapSearchParams files (ORCID, ROR) useclientIdfor the samesearchParams['client-id']source. While domain-specific naming can be appropriate, ensure this inconsistency is intentional and doesn't cause confusion in downstream query handling.Verify the naming convention used in the query layer:
#!/bin/bash # Description: Check naming conventions for client/repository ID across queries # Check usage of repositoryId vs clientId in query files rg -n "\b(repositoryId|clientId)\b" --type=ts -g "src/data/queries/*" -C2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts(1 hunks)src/app/(main)/doi.org/page.tsx(1 hunks)src/app/(main)/orcid.org/[orcid]/mapSearchParams.ts(1 hunks)src/app/(main)/ror.org/[rorid]/mapSearchParams.ts(1 hunks)src/components/WorkFacets/WorkFacets.tsx(5 hunks)src/data/constants.ts(1 hunks)src/data/queries/searchDoiFacetsQuery.ts(1 hunks)src/data/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/data/types.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/data/constants.ts (4)
src/data/clients/ror-v2-client.ts (1)
RORFacet(85-89)src/components/FacetList/FacetList.tsx (1)
FacetListProps(14-27)src/components/SankeyGraph/SankeyGraph.tsx (1)
facet(25-42)src/data/queries/searchDoiQuery.ts (1)
appendFacets(39-46)
src/app/(main)/doi.org/[...doi]/mapSearchParams.ts (1)
src/app/(main)/repositories/[...repoid]/mapSearchParams.ts (2)
mapSearchparams(16-33)SearchParams(1-14)
src/app/(main)/orcid.org/[orcid]/mapSearchParams.ts (2)
src/app/(main)/repositories/[...repoid]/mapSearchParams.ts (2)
mapSearchparams(16-33)SearchParams(1-14)src/data/queries/searchDoiQuery.ts (1)
appendFacets(39-46)
src/app/(main)/doi.org/page.tsx (1)
src/data/queries/searchDoiQuery.ts (2)
QueryVar(208-225)appendFacets(39-46)
src/app/(main)/ror.org/[rorid]/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/WorkFacets/WorkFacets.tsx (4)
src/data/types.ts (1)
Facet(326-331)src/components/FacetList/FacetList.tsx (1)
FacetList(25-52)src/data/clients/ror-v2-client.ts (1)
RORFacet(85-89)src/data/queries/searchOrganizationQuery.ts (2)
convertRORTypeFacet(205-211)convertRORCountriesFacet(197-203)
⏰ 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 (2)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (1)
🔇 Additional comments (8)
src/app/(main)/doi.org/page.tsx (1)
24-25: LGTM! Client-related parameters correctly mapped.The addition of
clientTypeandclientIdto the variables object is consistent with theQueryVarinterface and aligns with the broader PR changes to support client/repository filtering.src/app/(main)/ror.org/[rorid]/mapSearchParams.ts (1)
28-30: LGTM! Organization relation type parameters correctly mapped.The addition of
clientId,clientType, andorganizationRelationType(with sensible default 'allRelated') aligns with the new organization relationship filtering feature. The default ensures the facet shows all related works when no specific filter is selected.src/data/constants.ts (1)
77-77: LGTM! Clients facet added to default facets.The addition of the
'clients'facet to the default facets list is consistent with the broader changes to support client/repository filtering. The placement near'clientTypes'is logical and maintains related facets together.src/data/queries/searchDoiFacetsQuery.ts (1)
38-38: LGTM! Clients facet data correctly mapped from backend.The addition of
clients: meta.clients?.slice(0, 10)follows the established pattern used for other facets (resourceTypes, languages, etc.). The optional chaining and 10-item limit are consistent with the codebase conventions.src/components/WorkFacets/WorkFacets.tsx (3)
29-29: LGTM! Clients facet field added to interface.The addition of
clients?: Facet[]to theFacetsinterface properly supports the new repository facet data flow and aligns with the changes in the data layer.
70-70: LGTM! Organization relation type state check added.The
isOrganizationRelationTypeSetcheck follows the same pattern asisConnectionTypeSetand enables proper default selection behavior for the new facet.
172-192: Well-structured repository facets rendering.The conditional rendering of Repository and Repository Type facets when not on
/repositoriespages is logical and prevents redundant filtering. The addition oftooltipTextfor both facets ("The DataCite Repository where a DOI is stored" and "The type of DataCite Repository where a DOI is stored") provides helpful user guidance.The use of
data.clientsfor the repository facet properly connects to the new clients data flow introduced in this PR.src/app/(main)/orcid.org/[orcid]/mapSearchParams.ts (1)
27-27: clientId parameter is correctly used as optional DataCite API filter, not an ORCID API parameter.The
clientIdextracted fromsearchParams['client-id']is passed to DataCite API queries (viasearchDoiQuery.ts) as an optional repository/client filter for DOI results, not to ORCID API endpoints. This implementation is correct and aligns with DataCite API expectations for filtering works by DataCite client.
Purpose
This PR is intended to implement the following:
closes: https://github.com/datacite/product-backlog/issues/515
closes: https://github.com/datacite/product-backlog/issues/516
closes: https://github.com/datacite/product-backlog/issues/517
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
Refactor
Style
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.