Repository navigation
[DT-4062] Add study recommendation sections - #3933
otchet-broad merged 29 commits into
Conversation
Adds two carousels below the research output sections: studies often requested in the same data access request as this one, and studies recommended by shared data type or PI. Each card carries the study name, PI and description, and links through to that study's page. Both stay on the page when they come back empty, saying so, rather than disappearing - the same reasoning as the other sections, where an absent section reads as a failure to load. The response is typed as a new `StudyRecommendation` rather than the index-shaped `StudyAggregation`: the endpoints return only studyId, studyName, studyDescription, piName, datasetCount and datasetIds, so the richer type would let a reader of `study.dataTypes` compile and then fail at runtime. `datasetCount` therefore comes typed for whoever wants it on the card. Sixth of the DT-4062 stack. **Pairs with consent #3054** (otchet-dt-4061-study-recommendations), which adds /api/metrics/study-recommendations/{studyId}/similar and /frequently-requested-with. Until that deploys both requests fail and each carousel shows its error state. Worth knowing for review: the backend restricts candidates to publicly visible studies, so a recommendation never reveals a private study to someone who could not otherwise see it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tudy-recommendations
…tudy-recommendations
…tudy-recommendations # Conflicts: # src/types/model.ts
…tudy-recommendations # Conflicts: # src/components/study_details/StudyDetails.tsx
…tudy-recommendations
…tudy-recommendations
…tudy-recommendations
…tudy-recommendations
…tudy-recommendations
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved carousel interaction, refetch-error handling, and link-semantic issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds study recommendation sections to study detail pages with typed API clients, React Query hooks, recommendation cards, and scroll-reset behavior.
Changes:
- Adds recommendation models, API calls, and query hooks.
- Renders related-study recommendation sections and states.
- Adds navigation scroll behavior and supporting tests.
File summaries
| File | Summary |
|---|---|
test/components/study_details/StudyDetails.spec.tsx |
Tests navigation scroll behavior; recommendation rendering and error states remain uncovered. |
src/types/model.ts |
Defines the recommendation response type. |
src/libs/ajax/StudyRecommendations.ts |
Adds recommendation API calls; dedicated adapter tests are missing. |
src/hooks/useStudyDetailsData.ts |
Adds recommendation queries. |
src/components/study_details/StudyRecommendationCarousel.tsx |
Renders recommendation cards and states; needs carousel interaction, resilient refetch-error handling, router-link semantics, and broader coverage. |
src/components/study_details/StudyDetails.tsx |
Integrates recommendations and scroll reset; the similar-study heading should mention both recommendation signals. |
Review details
Suppressed comments (3)
src/components/study_details/StudyDetails.tsx:213
- The similar-study recommendations are described as using shared data type or PI, but this heading says they are based only on data type. That makes the user-facing explanation incomplete; mention both signals or use a neutral heading.
heading="Recommended Studies based on Data Type"
src/components/study_details/StudyRecommendationCarousel.tsx:25
- After a successful response, a background refetch can fail while
recommendationsstill contains the previous cards.StudyQueryResultcheckserrorbefore rendering its children, so this path hides the usable cards and shows the error instead, contrary to theisPendingcontract above. Preserve the previous cards when data exists, while optionally surfacing the refetch failure separately.
<StudyQueryResult
isPending={isPending}
error={error}
isEmpty={recommendations.length === 0}
src/libs/ajax/StudyRecommendations.ts:7
- This new AJAX adapter has no dedicated spec, unlike the existing
StudyandDatasetMetricsadapters. URL suffix construction, auth options, response extraction, and rejection propagation are therefore unverified; addStudyRecommendationstests before relying on this integration.
const get = async (studyId: number | string, path: string): Promise<StudyRecommendation[]> => {
const url = `${await Config.getApiUrl()}/api/metrics/study-recommendations/${studyId}/${path}`
return (await fetchGet<StudyRecommendation[]>(url, Config.authOpts())).data
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…tudy-recommendations
Each card's action area is a router link with a `to` value rather than a button that navigates imperatively, matching how study destinations work elsewhere. A button swallows the native link actions - open in a new tab, copy link address, middle click - which is most of how people follow a recommendation. Adds a spec for the component. The feature tests mock both recommendation calls to return empty arrays, so no card, link, empty state or error state was exercised at all. The link case fails against the previous button. Says plainly in the component that this is a responsive grid: every recommendation renders and wraps onto further rows, with no horizontal scrolling or next/previous affordance. The PR described carousels; building that interaction on a page already this long is worth doing deliberately rather than as a review fix, so it stays a follow-up and the description is corrected instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tudy-recommendations
…tudy-recommendations
otchet-broad
changed the title
…tudy-recommendations
…tudy-recommendations # Conflicts: # src/components/study_details/StudyDetails.tsx
…tudy-recommendations # Conflicts: # src/components/study_details/StudyDetails.tsx
…tudy-recommendations # Conflicts: # src/components/study_details/StudyDetails.tsx
…tudy-recommendations
…tudy-recommendations
…tudy-recommendations
…tudy-recommendations
kevinmarete
left a comment
kevinmarete
left a comment
There was a problem hiding this comment.
After an initial successful request, React Query can retain recommendations while setting error when a background refetch fails. Because error is passed unconditionally to StudyQueryResult, the component replaces valid cached cards with the error state. This also contradicts the comment that background refetches should keep cards on screen.
Please make the error blocking only when no recommendations are available—for example, error={recommendations.length === 0 ? error : undefined}—and add a regression test covering cached data plus a refetch error.
Review caught that the error went to StudyQueryResult unconditionally, so a failed background refetch replaced cards that had loaded fine. That also contradicted the note on isPending directly above it, which says a background refetch keeps the cards on screen - it does for the spinner and did not for the error. Reports a failure only when there is nothing else to show, as the asset and publication sections already do. The test pairs cached recommendations with an error and fails on the previous version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tudy-recommendations
The component test pairs cached recommendations with an error directly, which is the carousel's whole contract - it is presentational. The two sibling fixes got integration tests instead, driving a failed refetch through the query client, and this section should be held to the same standard: it is the wiring in StudyDetails that decides whether the component ever sees cached data alongside an error. Fails with the error passed unconditionally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fboulnois
left a comment
fboulnois
left a comment
There was a problem hiding this comment.
A few suggestions identified with Claude:
1. Unencoded route parameter in API path: src/libs/ajax/StudyRecommendations.ts:6
studyId from useParams is interpolated raw. useParams decodes the URL, so /studies/1%2F..%2Fdar-summaries%2F5 becomes /api/metrics/study-recommendations/1/../dar-summaries/5/similar, which the browser normalizes into an authenticated GET at a different endpoint. Sibling StudyComments.ts:5-9 wraps the same id in encodeURIComponent for exactly this reason.
2. Scroll reset fires on mount, defeating Back/Forward restoration: src/components/study_details/StudyDetails.tsx:269
The [studyId] effect runs on initial mount, not only when the route changes to a different study. Scroll deep into /studies/5, follow a dataset link, press Back: the browser restores the offset, then the effect snaps to top. A ref that skips the first run, or a pathname-driven reset in the router layer, matches the comment's stated intent.
3. Error guard keys on list length, not cached-ness: src/components/study_details/StudyRecommendationCarousel.tsx:28
A study with zero recommendations shows "No study recommendations yet."; if a background refetch then fails, React Query keeps data=[] and sets error, and recommendations.length === 0 ? error : undefined flips the section to the error banner. StudySecondaryResearchOutputs.tsx:50 uses data ? undefined : error; use recommendations === undefined ? error : undefined here.
4. Recommendation queries hoisted into the page component: src/components/study_details/StudyDetails.tsx:89
useSimilarStudies and useFrequentlyRequestedWithStudies live in StudyDetailsContent and are threaded through props, so every pending→success→refetch transition re-renders the whole page including the DataGrid. Siblings StudyDarHistory.tsx:49 and StudySecondaryResearchOutputs.tsx:41 take studyId and own their hook. A thin wrapper removes 4 imports, 2 hooks and 6 props.
5. Scroll-to-top is a router-level concern patched into one page: src/components/study_details/StudyDetails.tsx:266
The root cause the comment names — BrowserRouter preserves document offset on in-app navigation — applies app-wide: "Back to library" from a deep study page lands on /datalibrary at the same offset. A single pathname-keyed reset next to BrowserRouter in src/index.tsx/AppRoutes.tsx (or a data router with <ScrollRestoration>) fixes every route once.
6. Current study not filtered from its own recommendations: src/components/study_details/StudyRecommendationCarousel.tsx:34
If /similar or /frequently-requested-with returns the viewed study (natural for a data-type similarity query), a card links back to the current page; useParams is unchanged, so neither the key nor the scroll effect fires and the click appears to do nothing. Filter String(r.studyId) !== studyId in the hook, or pass the current id to the carousel.
Four items from fboulnois, and the scroll one taken at the altitude he argued for rather than patched here. Scroll: the reset moves to a component beside BrowserRouter, so every route gets it - 'Back to library' from deep inside a study no longer lands halfway down the library. It skips POP, which is the browser's own Back and Forward restoring an offset it recorded, and also the initial load, so a bookmark deep into a page is left where the browser put it. That was the reported defect: the page-level effect ran on mount and undid the restoration. The page effect is gone. The study id goes into the recommendations path encoded. It arrives from useParams already decoded, so a crafted id was path segments of its own and the browser would have normalized the request onto another endpoint. StudyComments was fixed for this; this module had the same hole and no spec at all, so it has one now. The carousel's error keys on whether anything has loaded rather than on the list being empty. A study with no recommendations has loaded fine, and testing length alone flipped it from 'none yet' to an error banner the moment a refetch failed. The prop loses its `= []` default, which would have erased the distinction before it arrived. A study is no longer recommended to itself. A similarity query matches it perfectly, so it came back in its own results, and the card linked to the open page: the route never changed and the click did nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex review caught that reacting to the location alone misbehaves where state lives in the query string. The library keeps filters, sort and paging there, so each of those is a navigation to the same page: the first one scrolled to the top, because navigationType flips from POP to PUSH once and re-runs the effect, and none of the later ones did. First filter jumps, second does not. Comparing pathnames instead leaves query-string navigation alone entirely, which is what filtering should do. It also covers the first render without relying on the initial load being reported as POP. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
bde6545
into
otchet-dt-4062-study-dar-history
Addresses
https://broadworkbench.atlassian.net/browse/DT-4062
Summary
Adds two recommendation sections below the research outputs: studies often requested in the same
data access request as this one, and studies recommended by shared data type or PI. Each card
carries the study name, PI and description, and links through to that study.
These are responsive grids rather than carousels — every recommendation renders, wrapping onto
further rows as needed, with no horizontal scrolling or next/previous affordance. The PR
previously called them carousels; a grid is what is built and what this describes. Building real
carousel interaction on a page already this long is worth doing deliberately rather than as a
review fix, so it is left as a follow-up.
Each card's action area is a router link with a
tovalue rather than a button that navigatesimperatively, matching how study destinations work elsewhere (
StudyCard). That restores thenative link actions a button swallows: open in a new tab, copy link address, middle-click.
Both sections stay on the page when they come back empty, saying so, for the same reason as the
other sections — an absent section on a study page reads as a failure to load.
The response is typed as a new
StudyRecommendationrather than the index-shapedStudyAggregation. The endpoints return only studyId, studyName, studyDescription, piName,datasetCount and datasetIds, so the richer type would let a reader of
study.dataTypescompileand then fail at runtime.
datasetCounttherefore comes typed for whoever wants it on the card.The sections sit near the bottom of a long page, and BrowserRouter preserves the document offset
during an in-app navigation, so the scroll position resets whenever the route points at a
different study — otherwise following a recommendation opens the next study at the same deep
scroll position.
Worth knowing for review: the backend restricts candidates to publicly visible studies, so a
recommendation never reveals a private study to someone who could not otherwise see it.
Have you read Terra's Contributing Guide lately? If not, do that first.