Fixed some pointless drops list N+1 queries#2384
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis PR refactors drop hydration to fetch only metadata where possible, adds a metadata-only API helper, updates the single-drop hook to merge metadata into initial drops, and makes the drop-quote component use waveId and cached/derived initialDrop to avoid unnecessary queries. ChangesDrop metadata-only hydration architecture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🧹 Nitpick comments (1)
__tests__/components/waves/drops/WaveDropQuoteWithDropId.test.tsx (1)
74-97: ⚡ Quick winAdd a test for mismatched
maybeDrop.idvsdropId.Current cases don’t cover the scenario where
maybeDropexists but refers to a different drop. Add one assertion that fetch stays enabled andinitialDatais not set in that case.Also applies to: 121-153
🤖 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__/components/waves/drops/WaveDropQuoteWithDropId.test.tsx` around lines 74 - 97, Add a new test case for WaveDropQuoteWithDropId where maybeDrop.id does not match the passed dropId (e.g., maybeDrop.id = "d1" and dropId = " d2 "), mocking useQuery the same way; assert that the useQuery call for QueryKey.DROP has enabled === true (fetch stays enabled), that initialData is not provided (call.initialData is undefined and no initialDataUpdatedAt property), and that fetchDropByIdBatchedMock is allowed to be invoked (or has been called) instead of being suppressed. Use the existing test pattern (useQuery.mockImplementation, capturedProps, and examining useQuery.mock.calls[0][0]) and reference maybeDrop, dropId, useQuery, QueryKey.DROP, initialData, and fetchDropByIdBatchedMock to locate the relevant code.
🤖 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 `@components/waves/drops/WaveDropQuoteWithDropId.tsx`:
- Around line 96-105: The bug is that maybeDrop is used unconditionally as
initialData and can mismatch normalizedDropId which causes skipping the fetch;
fix by only accepting maybeDrop when its id matches normalizedDropId (e.g.
compute initialDrop as (maybeDrop?.id === normalizedDropId ? maybeDrop :
cachedDrop ?? waveMessagesDrop ?? null) or similarly guard each cached source by
id) so that useQuery's enabled/initialData logic uses a drop that actually
matches normalizedDropId (affecting getDropQueryKey, enabled and initialData
passed to useQuery).
In `@services/api/wave-drops-v2-api.ts`:
- Around line 219-227: The catch block in fetchDropMetadataByIdV2 is currently
swallowing errors (including from getNormalizedDropId) by returning
priorityMetadata; update the catch so that after calling
rethrowAbortFetchError(error) it does not return a fallback but instead rethrows
the error (e.g., throw error) so invalid dropId or other unexpected errors
propagate to the caller; keep special handling for aborts via
rethrowAbortFetchError but do not return [...priorityMetadata] in the catch.
References: fetchDropMetadataByIdV2, getNormalizedDropId, getDropEndpointId,
commonApiFetch, mergeMetadata.
---
Nitpick comments:
In `@__tests__/components/waves/drops/WaveDropQuoteWithDropId.test.tsx`:
- Around line 74-97: Add a new test case for WaveDropQuoteWithDropId where
maybeDrop.id does not match the passed dropId (e.g., maybeDrop.id = "d1" and
dropId = " d2 "), mocking useQuery the same way; assert that the useQuery call
for QueryKey.DROP has enabled === true (fetch stays enabled), that initialData
is not provided (call.initialData is undefined and no initialDataUpdatedAt
property), and that fetchDropByIdBatchedMock is allowed to be invoked (or has
been called) instead of being suppressed. Use the existing test pattern
(useQuery.mockImplementation, capturedProps, and examining
useQuery.mock.calls[0][0]) and reference maybeDrop, dropId, useQuery,
QueryKey.DROP, initialData, and fetchDropByIdBatchedMock to locate the relevant
code.
🪄 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: 56e745ba-1b71-47a1-a103-4bf3c7110755
📒 Files selected for processing (10)
__tests__/components/waves/drop/useSingleWaveDropData.test.tsx__tests__/components/waves/drops/WaveDropQuoteWithDropId.test.tsx__tests__/services/api/drop-api.test.ts__tests__/services/api/wave-drops-v2-api.test.tscomponents/drops/view/part/dropPartMarkdown/renderers.tsxcomponents/waves/drop/useSingleWaveDropData.tscomponents/waves/drops/WaveDropPartContentMarkdown.tsxcomponents/waves/drops/WaveDropQuoteWithDropId.tsxservices/api/drop-api.tsservices/api/wave-drops-v2-api.ts
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
GelatoGenesis
force-pushed
the
b-17786737983
branch
from
6ea3cea to
ce039a9
Compare
simo6529
approved these changes
May 13, 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
Bug Fixes
Performance Improvements
UX