Skip to content

fix: batch featured publications queries for performance#1205

Open
hubsmoke wants to merge 3 commits into
developfrom
fix/featured-publications-performance
Open

fix: batch featured publications queries for performance#1205
hubsmoke wants to merge 3 commits into
developfrom
fix/featured-publications-performance

Conversation

@hubsmoke

@hubsmoke hubsmoke commented Jan 20, 2026

Copy link
Copy Markdown
Member

Replace N+1 query pattern with batched approach:

  • Single DB query for all submissions
  • Single batched getIndexedResearchObjects call
  • Parallel manifest fetches via Promise.all

Reduces cold cache response time from ~20-35s to ~2-4s.

Closes #<GH_issue_number>

Description of the Problem / Feature

Explanation of the solution

Instructions on making this work

UI changes for review

When major UI changes will happen with this PR, please include links to URLS to compare or screenshots demonstrating the difference and notify design

Summary by CodeRabbit

  • Performance Improvements
    • Featured publications load faster and more reliably by consolidating many individual lookups into batched retrievals, reducing latency and improving page responsiveness when viewing featured publication lists and details.
  • Stability
    • Batched fetching reduces per-item failures and simplifies caching behavior, improving overall reliability of featured publication displays.

✏️ Tip: You can customize this high-level summary in your review settings.

Replace N+1 query pattern with batched approach:
- Single DB query for all submissions
- Single batched getIndexedResearchObjects call
- Parallel manifest fetches via Promise.all

Reduces cold cache response time from ~20-35s to ~2-4s.
@coderabbitai

coderabbitai Bot commented Jan 20, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Replaces per-item async fetches in featured publication controllers with a single batched call getBatchedFeaturedPublicationDetails, consolidating DB and research-object retrieval while preserving parallel manifest fetches and result ordering.

Changes

Cohort / File(s) Summary
Featured publications controllers & service
desci-server/src/controllers/journals/featured.ts, desci-server/src/services/journals/JournalSubmissionService.ts
Controllers now call getBatchedFeaturedPublicationDetails (new export). Service adds getBatchedFeaturedPublicationDetails(submissionIds: number[]) which performs a single batched DB query, a batched getIndexedResearchObjects call, parallel manifest fetches, builds lookup maps, preserves input order, and filters out failed entries.

Sequence Diagram(s)

sequenceDiagram
    participant Controller as Featured Controller
    participant Service as Journal Submission Service
    participant DB as Database
    participant RO as Research Object Service
    participant Manifest as Manifest Service

    Controller->>Service: getBatchedFeaturedPublicationDetails([id1,id2,...])
    Service->>DB: Query submissions by IDs (batched)
    DB-->>Service: Submission rows (with UUIDs)
    Service->>RO: getIndexedResearchObjects(all UUIDs) (batched)
    RO-->>Service: Research objects map
    Service->>Manifest: Fetch manifests in parallel (hex ID + version per submission)
    Manifest-->>Service: Manifest data / errors
    Service->>Service: Build lookup map, preserve input order, filter failures
    Service-->>Controller: FeaturedSubmissionDetails[]
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

enhancement

Poem

🐰 I hopped through IDs in one quick bound,
Gathered manifests without running around,
Batched my carrots, neat and bright,
Saved some hops and sped the flight,
🥕✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: batching featured publications queries to improve performance, which aligns with the primary objective of replacing N+1 queries with a batched approach.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (1)
desci-server/src/controllers/journals/featured.ts (1)

203-206: Inconsistent count in meta response.

Line 205 uses publications.length for the count field, but data may contain fewer items if some fetches failed (the batched function filters out nulls). This is inconsistent with line 120 which correctly uses data?.length ?? 0.

Proposed fix
     logger.trace({ publications }, 'listFeaturedJournalPublicationsController');
     return sendSuccess(res, {
       data,
-      meta: { count: publications.length, totalCount, totalPages, currentPage, limit, offset },
+      meta: { count: data.length, totalCount, totalPages, currentPage, limit, offset },
     });
🧹 Nitpick comments (1)
desci-server/src/controllers/journals/featured.ts (1)

120-120: Unnecessary optional chaining.

data is typed as FeaturedSubmissionDetails[] and will always be an array (never null or undefined), so the optional chaining data?.length is unnecessary. Consider using data.length for consistency with line 197.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@desci-server/src/services/journals/JournalSubmissionService.ts`:
- Around line 1004-1009: Guard against researchObjects being undefined before
building roById: after calling getIndexedResearchObjects(uuids) check that
researchObjects is defined and is an array (or default it to an empty array)
before calling researchObjects.map; update the code around the const {
researchObjects } = await getIndexedResearchObjects(uuids) and the roById
creation so you either early-return/log when researchObjects is missing or
construct roById from (researchObjects || []) to avoid calling .map on
undefined, and ensure downstream logic (which uses roById) correctly handles
missing entries.
- Around line 1011-1042: The current manifestFetchTasks usage lets any thrown
error from getManifestByCid reject the whole Promise.all; change to use
Promise.allSettled(manifestFetchTasks) (or wrap each task body in a try/catch)
so individual manifest fetch failures don’t abort the batch: call
Promise.allSettled(manifestFetchTasks), filter results for status ===
'fulfilled' and non-null values, and then return only those as
FeaturedSubmissionDetails (keep references to manifestFetchTasks,
getManifestByCid, FeaturedSubmissionDetails and the existing logging for missing
researchObject/targetVersion).
- Around line 957-980: The findMany call using
prisma.journalSubmission.findMany({ where: { id: { in: submissionIds } } }) does
not preserve the input order, so sort the returned submissions to match
submissionIds before further processing: create an orderedSubmissions array by
mapping submissionIds to their corresponding entries from submissions (using id
or uuid) and then use orderedSubmissions (instead of submissions) for UUID
collection and the manifest tasks; ensure you handle missing ids gracefully
(e.g., filter out undefined).

Comment thread desci-server/src/services/journals/JournalSubmissionService.ts
Comment thread desci-server/src/services/journals/JournalSubmissionService.ts Outdated
Comment thread desci-server/src/services/journals/JournalSubmissionService.ts Outdated
- Reorder submissions to match input submissionIds order
- Guard against undefined researchObjects from getIndexedResearchObjects
- Use Promise.allSettled to prevent single manifest failure from aborting batch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant