Solving N+1 problems with batch requests#2390
Merged
Merged
Conversation
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
simo6529
previously approved these changes
May 14, 2026
📝 WalkthroughWalkthroughThis PR migrates drop lookups to batched V2 endpoints (ids and serial_nos), adds server schema/router wiring and client hydration, implements client-side serial-number micro-batching with chunking and abort handling, refactors drop-api to batch and chunk ID requests, and updates components/tests to the new query shapes. ChangesDrop Fetching V2 Batch Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (5)
services/api/wave-drops-v2-api.ts (2)
441-486: 💤 Low valueConsider logging failed hydration attempts.
The function silently filters out drops that fail hydration (lines 480-485), which could make it difficult to debug when drops don't appear as expected. While this defensive approach prevents partial failures from breaking the entire batch, it may hide legitimate errors (e.g., network issues, malformed data).
Consider adding error logging when hydration fails:
📊 Suggested enhancement
const results = await Promise.allSettled( dropsWithWaves.map(({ drop, wave }) => hydrateDropV2({ drop, wave, signal, includeFullMetadata, includeTopRaters, }) ) ); return results .filter( (result): result is PromiseFulfilledResult<ApiDrop> => - result.status === "fulfilled" + { + if (result.status === "rejected") { + console.error("Failed to hydrate drop:", result.reason); + return false; + } + return true; + } ) .map((result) => result.value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/api/wave-drops-v2-api.ts` around lines 441 - 486, The current hydrateDropsWithEmbeddedWavesV2 function swallows failed hydrateDropV2 results by filtering only fulfilled promises; update the post-Promise.allSettled handling to log rejected results so failures are visible for debugging: after calling hydrateDropV2 for each drop, iterate the Promise.allSettled results, for each result with status "rejected" log a contextual error (including the drop id or wave id from the corresponding dropsWithWaves entry and the rejection reason) using your service logger, then continue returning only the fulfilled values as before; reference hydrateDropsWithEmbeddedWavesV2, hydrateDropV2 and the results variable to locate where to add the logging.
626-651: ⚡ Quick winGuard against malformed individual drop IDs.
Line 639 calls
getNormalizedDropIdwhich throws if an ID is empty after trimming. IfdropIdscontains even one whitespace-only or empty string, the entire batch request will fail.Consider filtering out malformed IDs before the API call:
🛡️ Proposed defensive handling
export async function fetchDropsV2ByIds({ dropIds, signal, includeFullMetadata = false, includeTopRaters = false, }: FetchDropsV2ByIdsProps): Promise<ApiDrop[]> { - if (dropIds.length === 0) { + const normalizedIds = dropIds + .map((id) => id.trim()) + .filter((id) => id.length > 0); + + if (normalizedIds.length === 0) { return []; } const response = await commonApiFetch<ApiDropV2PageWithoutCount>({ endpoint: "v2/drops", params: { - ids: dropIds.map(getNormalizedDropId).join(","), - page_size: dropIds.length.toString(), + ids: normalizedIds.join(","), + page_size: normalizedIds.length.toString(), }, signal, }); return hydrateDropsWithEmbeddedWavesV2({ drops: response.data, signal, includeFullMetadata, includeTopRaters, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/api/wave-drops-v2-api.ts` around lines 626 - 651, In fetchDropsV2ByIds, guard against malformed IDs by filtering dropIds first (e.g., dropIds.filter(id => id && id.trim() !== "")) before calling getNormalizedDropId so a whitespace/empty string doesn't make getNormalizedDropId throw; after filtering, if the resulting list is empty return [] immediately, otherwise proceed to map with getNormalizedDropId, call commonApiFetch and then hydrateDropsWithEmbeddedWavesV2 as before.services/api/drop-api.ts (1)
71-126: 💤 Low valueRemove non-null assertion with safer array access.
Line 93 uses a non-null assertion (
chunks[index]!) which TypeScript requires because it can't verifyforEach's index bounds. While this is safe in practice, using array destructuring in theforEachcallback provides type safety without assertions:♻️ Cleaner approach
- chunkResults.forEach((result, index) => { - const chunk = chunks[index]!; + chunks.forEach((chunk, index) => { + const result = chunkResults[index]; + if (!result) return; + if (result.status === "rejected") {Or keep the current structure but add a runtime guard:
chunkResults.forEach((result, index) => { - const chunk = chunks[index]!; + const chunk = chunks[index]; + if (!chunk) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/api/drop-api.ts` around lines 71 - 126, The non-null assertion chunks[index]! in fetchDropResultsByIds should be removed; instead iterate chunks with its index so you have a typed chunk value and match results by index safely. Replace chunkResults.forEach((result, index) => { const chunk = chunks[index]!; ... }) with chunks.forEach((chunk, index) => { const result = chunkResults[index]; if (!result) { /* push rejected entries for chunk or continue */ } ... }), or if you keep the original loop add a runtime guard const chunk = chunks[index]; if (!chunk) { continue; } before using it to avoid the assertion while preserving behavior.components/waves/drops/WaveDropQuoteWithSerialNo.tsx (1)
43-47: 💤 Low valueConsider passing React Query's abort signal for future cancellation support.
React Query provides an
AbortSignalvia the query function context that you can forward to the fetch function. While the current implementation offetchQuorumParticipationDropPreviewBySerialNoV2ignores the signal parameter, adding it now would make the code cancellation-ready when the underlying API is fixed.♻️ Proposed change to enable future cancellation
- queryFn: async () => - fetchQuorumParticipationDropPreviewBySerialNoV2({ + queryFn: async ({ signal }) => + fetchQuorumParticipationDropPreviewBySerialNoV2({ waveId, serialNo, + signal, }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/waves/drops/WaveDropQuoteWithSerialNo.tsx` around lines 43 - 47, The query function passed to React Query (queryFn) doesn't forward the provided AbortSignal to fetchQuorumParticipationDropPreviewBySerialNoV2, so update queryFn to accept the React Query context and pass context.signal into fetchQuorumParticipationDropPreviewBySerialNoV2; also adjust fetchQuorumParticipationDropPreviewBySerialNoV2 signature to accept an optional signal param (and thread it into the underlying fetch) so the query can be cancelled when React Query provides an AbortSignal.__tests__/services/api/quorum-participation-drop-preview-v2-api.test.ts (1)
181-181: 💤 Low valueSimplify arrow function to direct
Numberreference.The arrow function
(serialNo) => Number(serialNo)is equivalent to passingNumberdirectly as the map callback.♻️ Proposed simplification
const serialNos = request.params?.serial_nos .split(",") - .map((serialNo) => Number(serialNo)); + .map(Number);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/services/api/quorum-participation-drop-preview-v2-api.test.ts` at line 181, Replace the explicit arrow callback "(serialNo) => Number(serialNo)" with a direct reference to the Number function in the map call to simplify the code; locate the mapping expression ".map((serialNo) => Number(serialNo))" in the test and change it to use ".map(Number)" so the array of serials is converted to numbers more concisely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/services/api/quorum-participation-drop-preview-v2-api.test.ts`:
- Around line 90-92: Tests leak module-level state because
pendingSerialPreviewRequests and isSerialPreviewBatchScheduled aren't reset; add
cleanup in the beforeEach to reset that state by either exporting a test-only
reset helper from the implementation (e.g., __resetBatchStateForTests that
clears pendingSerialPreviewRequests and sets isSerialPreviewBatchScheduled =
false) and calling it in beforeEach, or call jest.resetModules() and re-require
the module before each test to ensure pendingSerialPreviewRequests and
isSerialPreviewBatchScheduled are fresh; reference the module symbols
pendingSerialPreviewRequests, isSerialPreviewBatchScheduled and the proposed
__resetBatchStateForTests when making the change.
In `@services/api/quorum-participation-drop-preview-v2-api.ts`:
- Around line 22-26: The module-level mutable variables
pendingSerialPreviewRequests and isSerialPreviewBatchScheduled should be
encapsulated inside a class (e.g., SerialPreviewBatcher) instead of global
state: create a class that owns the Map and scheduled flag, provides methods to
enqueue requests, run scheduleSerialPreviewBatchFlush, and flush batches, and
export a singleton instance that caller code uses; add a resetForTests() method
on that class to clear state for test isolation and allow mocking/resetting
between tests; update any functions that reference pendingSerialPreviewRequests
or isSerialPreviewBatchScheduled to call the corresponding instance methods on
SerialPreviewBatcher (including scheduleSerialPreviewBatchFlush) so concurrent
contexts use the instance and state can be controlled in tests.
- Around line 91-125: flushSerialPreviewRequests currently does not support
cancellation so in-flight fetches cannot be aborted; change
flushSerialPreviewRequests to accept an AbortSignal (or create an
AbortController per batch if you want per-batch cancellation), thread that
signal into fetchSerialPreviewChunk, and ensure fetchSerialPreviewChunk forwards
the signal into commonApiFetch; also update the error handling in
flushSerialPreviewRequests to treat AbortError specially by rejecting queued
request promises with the abort error (or a consistent cancellation error) and
avoid retrying, and ensure callers that schedule batches pass their provided
signal through when enqueuing so component unmounts/caller cancellation actually
aborts the underlying network request (refer to symbols:
flushSerialPreviewRequests, fetchSerialPreviewChunk, commonApiFetch,
pendingSerialPreviewRequests, mapSerialPreviewDrop).
---
Nitpick comments:
In `@__tests__/services/api/quorum-participation-drop-preview-v2-api.test.ts`:
- Line 181: Replace the explicit arrow callback "(serialNo) => Number(serialNo)"
with a direct reference to the Number function in the map call to simplify the
code; locate the mapping expression ".map((serialNo) => Number(serialNo))" in
the test and change it to use ".map(Number)" so the array of serials is
converted to numbers more concisely.
In `@components/waves/drops/WaveDropQuoteWithSerialNo.tsx`:
- Around line 43-47: The query function passed to React Query (queryFn) doesn't
forward the provided AbortSignal to
fetchQuorumParticipationDropPreviewBySerialNoV2, so update queryFn to accept the
React Query context and pass context.signal into
fetchQuorumParticipationDropPreviewBySerialNoV2; also adjust
fetchQuorumParticipationDropPreviewBySerialNoV2 signature to accept an optional
signal param (and thread it into the underlying fetch) so the query can be
cancelled when React Query provides an AbortSignal.
In `@services/api/drop-api.ts`:
- Around line 71-126: The non-null assertion chunks[index]! in
fetchDropResultsByIds should be removed; instead iterate chunks with its index
so you have a typed chunk value and match results by index safely. Replace
chunkResults.forEach((result, index) => { const chunk = chunks[index]!; ... })
with chunks.forEach((chunk, index) => { const result = chunkResults[index]; if
(!result) { /* push rejected entries for chunk or continue */ } ... }), or if
you keep the original loop add a runtime guard const chunk = chunks[index]; if
(!chunk) { continue; } before using it to avoid the assertion while preserving
behavior.
In `@services/api/wave-drops-v2-api.ts`:
- Around line 441-486: The current hydrateDropsWithEmbeddedWavesV2 function
swallows failed hydrateDropV2 results by filtering only fulfilled promises;
update the post-Promise.allSettled handling to log rejected results so failures
are visible for debugging: after calling hydrateDropV2 for each drop, iterate
the Promise.allSettled results, for each result with status "rejected" log a
contextual error (including the drop id or wave id from the corresponding
dropsWithWaves entry and the rejection reason) using your service logger, then
continue returning only the fulfilled values as before; reference
hydrateDropsWithEmbeddedWavesV2, hydrateDropV2 and the results variable to
locate where to add the logging.
- Around line 626-651: In fetchDropsV2ByIds, guard against malformed IDs by
filtering dropIds first (e.g., dropIds.filter(id => id && id.trim() !== ""))
before calling getNormalizedDropId so a whitespace/empty string doesn't make
getNormalizedDropId throw; after filtering, if the resulting list is empty
return [] immediately, otherwise proceed to map with getNormalizedDropId, call
commonApiFetch and then hydrateDropsWithEmbeddedWavesV2 as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac71210a-af2d-457f-ad10-75d038f7a09a
⛔ Files ignored due to path filters (2)
generated/models/ApiDropV2.tsis excluded by!**/generated/**generated/models/ObjectSerializer.tsis excluded by!**/generated/**
📒 Files selected for processing (11)
__tests__/components/waves/WaveDropQuoteWithSerialNo.test.tsx__tests__/components/waves/drops/WaveDropLinkPreview.test.tsx__tests__/services/api/drop-api.test.ts__tests__/services/api/quorum-participation-drop-preview-v2-api.test.ts__tests__/services/api/wave-drops-v2-api.test.tscomponents/waves/drops/WaveDropQuoteWithSerialNo.tsxcontexts/wave/hooks/types.tsopenapi.yamlservices/api/drop-api.tsservices/api/quorum-participation-drop-preview-v2-api.tsservices/api/wave-drops-v2-api.ts
💤 Files with no reviewable changes (1)
- contexts/wave/hooks/types.ts
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/api/quorum-participation-drop-preview-v2-api.ts (1)
246-259: 💤 Low valueRedundant error handling branches can be simplified.
Both the
isAbortErrorbranch and the fallthrough case callrejectRemainingRequests(). The early return afterisAbortErrorcheck doesn't change behavior since both paths end up rejecting remaining requests. This could be simplified unless different behavior is planned for abort vs other errors.♻️ Suggested simplification
} catch (error) { - const rejectRemainingRequests = () => { - activeRequests.forEach((request) => { - request.reject(error); - }); - activeRequests.clear(); - }; - - if (isAbortError(error)) { - rejectRemainingRequests(); - return; - } - - rejectRemainingRequests(); + activeRequests.forEach((request) => { + request.reject(error); + }); + activeRequests.clear(); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/api/quorum-participation-drop-preview-v2-api.ts` around lines 246 - 259, The catch block duplicates logic: both the isAbortError(error) branch and the fallthrough call rejectRemainingRequests(), so remove the redundant branch/early return and call rejectRemainingRequests() once. Locate the catch block that defines rejectRemainingRequests (which iterates activeRequests and calls request.reject(error) then clears activeRequests) and replace the conditional that checks isAbortError(error) with a single unconditional call to rejectRemainingRequests(); if special abort handling is intended later, add it explicitly there instead of duplicating rejection logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@services/api/quorum-participation-drop-preview-v2-api.ts`:
- Around line 246-259: The catch block duplicates logic: both the
isAbortError(error) branch and the fallthrough call rejectRemainingRequests(),
so remove the redundant branch/early return and call rejectRemainingRequests()
once. Locate the catch block that defines rejectRemainingRequests (which
iterates activeRequests and calls request.reject(error) then clears
activeRequests) and replace the conditional that checks isAbortError(error) with
a single unconditional call to rejectRemainingRequests(); if special abort
handling is intended later, add it explicitly there instead of duplicating
rejection logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 193a65d0-604f-4a3c-8c96-71d5701a0145
📒 Files selected for processing (3)
__tests__/components/waves/drops/WaveDropLinkPreview.test.tsx__tests__/services/api/quorum-participation-drop-preview-v2-api.test.tsservices/api/quorum-participation-drop-preview-v2-api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/components/waves/drops/WaveDropLinkPreview.test.tsx
- tests/services/api/quorum-participation-drop-preview-v2-api.test.ts
simo6529
approved these changes
May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Improvements
New Features
Chores
Tests