Frontend thats mostly using v2 APIs#2350
Merged
Merged
Conversation
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrate waves/drops/notifications to V2: add SidebarWave types and V2 services/mappers, introduce QueryKey.WAVES_V2 and V2 cache handling, refactor hooks to V2 shapes and gating, thread wave into virtualization, refactor reactions and follow UI, adjust components, and update tests and OpenAPI. ChangesEnd-to-end V2 API adoption
Sequence Diagram(s)(skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hooks/useVirtualizedWaveDrops.ts (1)
10-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't insert
waveahead of the existingpageSizeparameter.This changes the public contract from
(waveId, dropId, pageSize?)to(waveId, dropId, wave?, pageSize?). Any unchanged caller still passing a numeric third argument now breaks, and the docblock above still advertises the old ordering.♻️ Suggested change
export function useVirtualizedWaveDrops( waveId: string, dropId: string | null, - wave?: ApiWave, - pageSize: number = 50 + pageSize: number = 50, + wave?: ApiWave ) {🤖 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 `@hooks/useVirtualizedWaveDrops.ts` around lines 10 - 23, The hook signature changed parameter order and breaks existing callers; restore the original public contract by keeping pageSize as the third parameter and moving wave to be an optional fourth parameter (i.e., useVirtualizedWaveDrops(waveId: string, dropId: string | null, pageSize?: number, wave?: ApiWave)). Update the JSDoc to reflect the original parameter ordering and adjust any internal references in useVirtualizedWaveDrops to read pageSize from the third argument and wave from the fourth so callers passing a numeric third arg remain compatible.helpers/stream.helpers.ts (1)
288-305:⚠️ Potential issue | 🔴 CriticalCache key mismatch: prefetch uses
limit: "10"but hook useslimit: "30"by default.The prefetch in
stream.helpers.ts(line 291) hardcodeslimit: "10", butuseNotificationsQuerydefaults tolimit: "30"(line 85) and is called explicitly withlimit: "30"inuseNotificationsController.ts(line 162). Since React Query uses the full query key for cache matching, these will be separate cache entries—the prefetch will never be hit. Additionally, the prefetch hardcodescause: null, butuseNotificationsQuerymay be called with different causes fromactiveFilter. Either align the prefetch to uselimit: "30"and variablecause, or ensure all consumers uselimit: "10"andcause: null.🤖 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 `@helpers/stream.helpers.ts` around lines 288 - 305, The prefetch uses a different query key than consumers (hardcoded limit: "10" and cause: null) so it never hits the same cache as useNotificationsQuery/useNotificationsController (which use limit: "30" and a variable activeFilter cause); update the prefetch call in stream.helpers.ts (queryClient.prefetchInfiniteQuery / fetchNotificationsV2) to use the same parameters as consumers—pass the same limit value (use "30" or read the default from where useNotificationsQuery is invoked) and pass the actual cause (e.g., activeFilter or the same variable used by useNotificationsQuery/useNotificationsController) so the QueryKey (QueryKey.IDENTITY_NOTIFICATIONS + { identity, limit, cause, version }) matches across code paths.
🟡 Minor comments (8)
components/waves/winners/drops/header/WaveWinnersDropHeaderVoters.tsx-25-36 (1)
25-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd fallback for React key to ensure stability.
Similar to the issue in
MemesWaveWinnerDrop.tsx, line 32 usesvoter.profile.handleas the key without a fallback. Ifhandleisnullorundefined, React's reconciliation may behave unexpectedly when multiple voters lack handles.🔑 Proposed fix
<WaveWinnersDropHeaderVoter voter={voter} winner={winner} index={index} - key={voter.profile.handle} + key={voter.profile.handle ?? voter.profile.id} />🤖 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/winners/drops/header/WaveWinnersDropHeaderVoters.tsx` around lines 25 - 36, The key prop for the mapped voters uses voter.profile.handle which can be null/undefined; update the map in the WaveWinnersDropHeaderVoters component to provide a stable fallback (e.g., use voter.profile.handle || voter.id || voter.profile.id || `voter-${index}`) when setting the key on WaveWinnersDropHeaderVoter so React reconciliation remains stable; change the key expression where winner.drop.top_raters is mapped to include that fallback.components/waves/winners/drops/MemesWaveWinnerDrop.tsx-287-329 (1)
287-329:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd fallback for React key to ensure stability.
The
keyprop on line 290 usesvoter.profile.handle, which could benullorundefined. React requires stable, unique keys, and this could cause rendering issues if multiple voters lack handles.Lines 292, 296, and 316 already include fallbacks (
handle ?? idorhandle ?? primary_address). The key should follow the same pattern.🔑 Proposed fix
- <React.Fragment key={voter.profile.handle}> + <React.Fragment key={voter.profile.handle ?? voter.profile.id}>🤖 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/winners/drops/MemesWaveWinnerDrop.tsx` around lines 287 - 329, The React key for items in the topVoters.map currently uses voter.profile.handle which can be null/undefined; change the key to a stable fallback consistent with other uses (e.g., use voter.profile.handle ?? voter.profile.id ?? voter.profile.primary_address) so each <React.Fragment key=...> is unique and stable; update the key expression in the topVoters.map callback to use that fallback pattern and ensure any tooltip id generation still matches the chosen fallback to avoid mismatches.openapi.yaml-13866-13869 (1)
13866-13869:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
contributorsto the required fields list for stable API response shapeThe
contributorsfield is defined inApiWaveOverview(as an array type) but not marked as required. This forces consumers to handle bothundefinedand[]cases unnecessarily. Marking it as required and always returning an empty array (when no contributors exist) provides a more stable, predictable response shape.♻️ Suggested schema fix
ApiWaveOverview: type: object required: - id - name - last_drop_time - created_at - subscribers_count - has_competition - is_dm_wave - description_drop - total_drops_count - is_private + - contributors🤖 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 `@openapi.yaml` around lines 13866 - 13869, The ApiWaveOverview schema currently defines contributors but doesn't mark it required; update the ApiWaveOverview required array to include "contributors" and ensure the contributors property remains type array (e.g., items: { ... }) so consumers always receive an array; also update the implementation that builds ApiWaveOverview responses (the code path that constructs ApiWaveOverview objects) to return an empty array for contributors when there are no contributors instead of omitting the field.openapi.yaml-13920-13922 (1)
13920-13922:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
nullable: truetofirst_unread_drop_serial_noinApiWaveOverviewContextProfileContextThis field should be nullable to represent users with no unread drops. The identical field in the
ApiMarkDropUnreadResponseschema already hasnullable: true, and this field is optional (not in the required list), making null an appropriate value.♻️ Suggested schema fix
first_unread_drop_serial_no: type: integer format: int64 + nullable: true🤖 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 `@openapi.yaml` around lines 13920 - 13922, In the ApiWaveOverviewContextProfileContext schema add "nullable: true" to the first_unread_drop_serial_no property so it can represent users with no unread drops; locate the first_unread_drop_serial_no field in ApiWaveOverviewContextProfileContext and add the nullable: true attribute (matching ApiMarkDropUnreadResponse) while leaving it optional in the required list.components/waves/drops/reaction-utils.ts-148-159 (1)
148-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid double-increment when profile is already present in target reaction.
On Line 158, count is incremented even when
hasProfileis true. That can overcount in edge cases where the actor already exists intarget.profiles.Suggested patch
nextEntries[existingIndex] = withReactionCount( { ...target, profiles: hasProfile ? target.profiles : [...target.profiles, profileMin], }, - getReactionCount(target) + 1 + getReactionCount(target) + (hasProfile ? 0 : 1) );🤖 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/reaction-utils.ts` around lines 148 - 159, The current block always increments the reaction count even when the profile already exists, causing double-increments; update the logic in the block that writes to nextEntries (around existingIndex using target, profileMin, hasProfile, withReactionCount, getReactionCount) so the profiles array remains unchanged when hasProfile is true and compute the new count as conditional: if hasProfile then use getReactionCount(target) else use getReactionCount(target) + 1, then pass that computed count into withReactionCount.components/home/explore-waves/ExploreWaveCard.tsx-34-35 (1)
34-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden drop-presence check to ignore invalid zero timestamps.
On Line 35,
!== nulltreats0as a valid drop time. That can render an incorrect “time ago” instead of the empty state. Prefer> 0when considering a wave as having drops.Suggested patch
- const hasDrops = lastMessageTime !== null; + const hasDrops = lastMessageTime !== null && lastMessageTime > 0;🤖 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/home/explore-waves/ExploreWaveCard.tsx` around lines 34 - 35, The current drop-presence check uses lastMessageTime = wave.latestDropTimestamp and hasDrops = lastMessageTime !== null which treats 0 as a valid timestamp; change the check to consider only positive timestamps (e.g., set hasDrops = lastMessageTime > 0) so waves with a zero/invalid timestamp are treated as having no drops; update any related logic that relies on hasDrops (e.g., time-ago rendering) to use this new boolean.services/api/wave-drops-v2-api.ts-569-620 (1)
569-620:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPopulate missing optional fields or adjust return type to match what's actually being returned.
The function returns only
{ wave, drops }but declaresPromise<ApiWaveDropsFeed>, which also defines optional fieldsroot_dropandtrace. This requires an unsafeas unknown asdouble cast. Either maproot_dropandtracefrom the API response (data), or change the return type to reflect what's actually populated.🤖 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 569 - 620, fetchWaveDropsFeedV2 currently returns only { wave, drops } and then force-casts to ApiWaveDropsFeed, but ApiWaveDropsFeed optionally includes root_drop and trace from the API response (ApiWaveDropsFeedV2), so remove the unsafe cast by populating those fields from data or by changing the declared return type; specifically, update fetchWaveDropsFeedV2 to map data.root_drop and data.trace (transforming them as needed—e.g., via existing hydrate/map helpers like mapApiWaveOverviewToApiWaveMin or hydrateDropsV2) into the returned object, or if those fields are intentionally omitted, change the function signature to Promise<{ wave: ...; drops: ... }>, ensuring consistency between the returned value and the declared type.components/brain/notifications/wave-created/NotificationWaveCreated.tsx-17-24 (1)
17-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against missing wave data producing a broken invitation link.
If
related_waveis absent andadditional_context.wave_idis empty (the v2 mapper defaultswave_idto""when norelated_waveis set – seeservices/api/notifications-v2-api.tslines 256–261),waveIdbecomes"", which produces an empty/invalidinvitationHrefand an emptywaveNamerendered inside theLink. Consider bailing out or rendering a non-link fallback whenwaveIdis falsy.🛡️ Suggested guard
- const wave = notification.related_wave; - const waveId = wave?.id ?? notification.additional_context.wave_id; - const invitationHref = getWaveRoute({ - waveId, - isDirectMessage: wave?.is_dm_wave ?? false, - isApp, - }); - const waveName = wave?.name ?? waveId; + const wave = notification.related_wave; + const waveId = wave?.id ?? notification.additional_context.wave_id; + if (!waveId) { + return null; + } + const invitationHref = getWaveRoute({ + waveId, + isDirectMessage: wave?.is_dm_wave ?? false, + isApp, + }); + const waveName = wave?.name ?? waveId;🤖 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/brain/notifications/wave-created/NotificationWaveCreated.tsx` around lines 17 - 24, The current code computes waveId from notification.related_wave or notification.additional_context.wave_id which can be an empty string, producing an invalid invitationHref/waveName; update the render logic in NotificationWaveCreated to check if waveId is truthy after computing it (from notification.related_wave / notification.additional_context.wave_id) and if not, do not call getWaveRoute or render a Link — instead render a safe non-link fallback (e.g., plain text or a disabled element) showing a fallback name/message; ensure getWaveRoute, invitationHref and waveName are only used when waveId is truthy to avoid producing broken links.
🧹 Nitpick comments (15)
hooks/useWaveActivityLogs.ts (1)
87-87: 💤 Low valueSimplify the effect dependency array.
The dependency array includes
waveId,dropId, andserializedLogTypes, but these are already captured in the memoizedqueryKey(line 50). Including them again is redundant and makes the dependency list harder to maintain.♻️ Simplified dependency array
- }, [canFetch, queryKey, waveId, dropId, serializedLogTypes, queryClient]); + }, [canFetch, queryKey, queryClient]);🤖 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 `@hooks/useWaveActivityLogs.ts` at line 87, The effect's dependency array is over-specified; remove redundant dependencies and rely on the memoized queryKey instead. Update the useEffect call that currently depends on canFetch, queryKey, waveId, dropId, serializedLogTypes, and queryClient so it only depends on canFetch, queryKey, and queryClient (or just queryKey and canFetch if queryClient is stable), ensuring queryKey (memoized at line with queryKey) captures waveId/dropId/serializedLogTypes and keeps behavior identical while simplifying maintenance.components/brain/notifications/NotificationsWrapper.tsx (1)
31-32: 💤 Low valueConsider renaming
hasChatScopefor clarity.The type guard now returns
truewhen eitherchatoris_direct_messageis present, making the namehasChatScopeslightly misleading. Consider renaming to something likehasDirectMessageInfoorcanDetermineDirectMessageto better reflect its purpose.🤖 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/brain/notifications/NotificationsWrapper.tsx` around lines 31 - 32, The function name hasChatScope is misleading because the type guard checks for presence of either chat or is_direct_message; rename the guard to a clearer name like hasDirectMessageInfo or canDetermineDirectMessage and update all usages and exports accordingly (search for hasChatScope, the declaration const hasChatScope = (wave: ExtendedDrop["wave"]): wave is WaveWithChatScope => ... , and any imports/consumers) so the name reflects that it detects direct-message-related info rather than a generic "chat scope".services/api/wave-curation-drops-v2-api.ts (2)
22-31: 💤 Low valueConsider documenting the fallback rationale.
The fallback sets all ratings to 0, all flags to false, and reaction to null. While these defaults appear safe, consider adding a comment explaining why these specific values were chosen for the fallback case, particularly for fields like
curatableandcurated.📝 Example documentation
+// Fallback context when the API doesn't return profile context +// All permissions/states default to most restrictive values for safety const FALLBACK_CONTEXT_PROFILE_CONTEXT: ApiDropContextProfileContext = { rating: 0,🤖 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-curation-drops-v2-api.ts` around lines 22 - 31, Add a brief comment above FALLBACK_CONTEXT_PROFILE_CONTEXT explaining the rationale for each chosen default (e.g., rating/min_rating/max_rating = 0 to denote neutral/unknown score, reaction = null to indicate no reaction, and boolean flags like boosted/bookmarked/curatable/curated = false to avoid granting implicit privileges or state). Mention that these defaults are conservative and used when profile context is unavailable, and reference ApiDropContextProfileContext and the fields curatable and curated so reviewers can quickly understand why those particular safe defaults were selected.
47-69: ⚡ Quick winConsider validating and encoding URL parameters.
The
curationIdis directly interpolated into the URL path without validation or encoding. While the hook trims the curationId, the service function doesn't validate that it's non-empty or URL-safe.🛡️ Suggested validation
export async function fetchWaveCurationDropsV2({ wave, curationId, page, pageSize, signal, }: FetchWaveCurationDropsV2Props): Promise<ApiCurationDropsPage> { + if (!curationId?.trim()) { + throw new Error("curationId is required"); + } const waveMin = normalizeWaveMin(wave); const response = await commonApiFetch<ApiDropV2PageWithoutCount>({ - endpoint: `v2/waves/${waveMin.id}/curations/${curationId}/drops`, + endpoint: `v2/waves/${encodeURIComponent(waveMin.id)}/curations/${encodeURIComponent(curationId.trim())}/drops`,🤖 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-curation-drops-v2-api.ts` around lines 47 - 69, In fetchWaveCurationDropsV2 validate and URL-encode the curationId before building the endpoint: trim the incoming curationId, ensure it is non-empty (throw a clear error or return a rejected Promise if empty), and use encodeURIComponent(curatedId) when interpolating into the endpoint string passed to commonApiFetch; keep the existing normalizeWaveMin, mapCurationDropV2 and response mapping logic unchanged.__tests__/hooks/useWaveCurationDrops.test.ts (1)
39-81: ⚡ Quick winConsider adding tests for edge cases.
The current test covers the happy path well. Consider adding tests for:
- Hook behavior when
enabled: false- Error handling when
waveis null- Error handling when
curationIdis null or empty- WebSocket message handling and cache updates
🤖 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__/hooks/useWaveCurationDrops.test.ts` around lines 39 - 81, Add unit tests to cover the edge cases for useWaveCurationDrops: create tests that renderHook with enabled: false and assert useInfiniteQuery (useInfiniteQueryMock) is called with enabled: false or not invoked; add a test that passes wave = null and asserts the queryFn is not called and/or that the hook returns the expected empty/error state; add a test for curationId null/empty (e.g., " " or null) to assert trimming/validation and that fetchWaveCurationDropsV2Mock is not called or appropriate error state is produced; and add a test simulating a WebSocket message (or calling whatever onMessage handler the hook registers) to verify the cache update behavior (e.g., that the fetch or cache update function is invoked) — reference the existing test utilities: useWaveCurationDrops, useInfiniteQueryMock, fetchWaveCurationDropsV2Mock, and renderHook to implement each case.hooks/useWaveCurationDrops.ts (1)
73-86: ⚡ Quick winConsider passing the abort signal for proper cancellation.
The
queryFndoesn't extract or pass thesignalfrom the React Query context. This means in-flight requests won't be properly cancelled when the query is aborted.🔧 Add signal support
- queryFn: async ({ pageParam }: { pageParam: number }) => { + queryFn: async ({ pageParam, signal }: { pageParam: number; signal?: AbortSignal }) => { if (!wave || !normalizedCurationId) { throw new Error( "Wave and curation are required to load curation drops" ); } return await fetchWaveCurationDropsV2({ wave, curationId: normalizedCurationId, page: pageParam, pageSize, + 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 `@hooks/useWaveCurationDrops.ts` around lines 73 - 86, The queryFn in useWaveCurationDrops.ts doesn't accept the React Query abort signal so inflight fetches can't be cancelled; update the queryFn signature to extract the signal from its params (e.g. async ({ pageParam, signal }: { pageParam: number; signal?: AbortSignal })) and pass that signal into fetchWaveCurationDropsV2 call (e.g. include signal in the options object alongside wave, curationId: normalizedCurationId, page, pageSize); if fetchWaveCurationDropsV2 doesn't yet accept a signal, add an optional signal parameter to its signature and forward it to the underlying fetch/XHR so cancellations work correctly.components/memes/drops/MemesLeaderboardDrop.tsx (1)
44-52: ⚡ Quick winConsider extracting duplicate helper functions to a shared utility.
The
getNonEmptyTextandgetMetadataValuefunctions are duplicated inMemeWinnerDrop.tsx(lines 37-50). Extracting these to a shared utility file would improve maintainability and ensure consistent behavior across components.Additionally, there's a type inconsistency: this file returns
string | nullwhileMemeWinnerDrop.tsxreturnsstring | undefined. Standardizing on one return type would improve consistency.♻️ Suggested approach
Create a new utility file (e.g.,
helpers/waves/dropMetadata.helpers.ts):import type { ExtendedDrop } from "@/helpers/waves/drop.helpers"; export const getNonEmptyText = ( value: string | null | undefined ): string | null => { const text = value?.trim(); return text && text.length > 0 ? text : null; }; export const getMetadataValue = ( drop: ExtendedDrop, dataKey: string ): string | null => getNonEmptyText( drop.metadata.find((metadata) => metadata.data_key === dataKey)?.data_value );Then import and use in both component files.
🤖 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/memes/drops/MemesLeaderboardDrop.tsx` around lines 44 - 52, Extract the duplicated helpers getNonEmptyText and getMetadataValue into a shared utility (e.g., helpers/waves/dropMetadata.helpers.ts) and update both MemesLeaderboardDrop.tsx and MemeWinnerDrop.tsx to import and use them; ensure the helper signatures use a single standardized return type (pick string | null) and update the implementation/type annotations accordingly (these helpers reference ExtendedDrop and the metadata.data_value lookup).components/waves/drops/WaveDropReply.tsx (1)
31-31: ⚡ Quick winMagic number: Extract hardcoded part ID.
The hardcoded
1is a magic number. If replies are always on part 1, define a named constant for clarity.♻️ Suggested refactor
+const REPLY_PART_ID = 1; + export default function WaveDropReply({ dropId, dropPartId: _dropPartId, maybeDrop, onReplyClick, }: WaveDropReplyProps) { const fixedReplyHeightClasses = "tw-h-[24px] tw-min-h-[24px] tw-max-h-[24px]"; - const { drop, content, isLoading } = useDropContent(dropId, 1, maybeDrop); + const { drop, content, isLoading } = useDropContent(dropId, REPLY_PART_ID, maybeDrop);🤖 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/WaveDropReply.tsx` at line 31, The call to useDropContent(dropId, 1, maybeDrop) uses a magic number; extract that hardcoded part ID into a named constant (e.g., const REPLIES_PART = 1) near the top of WaveDropReply and replace the literal 1 with REPLIES_PART so intent is clear; update any related usages or exports if this part ID is reused elsewhere and keep the symbol useDropContent, dropId, and maybeDrop intact.components/react-query-wrapper/ReactQueryWrapper.tsx (1)
916-918: 💤 Low valueConsider removing
voidfor consistency.The
voidprefix here is inconsistent with the otherinvalidateQueriescalls in this function (lines 910-915, 919-933). Either all should usevoidor none should, for uniform handling of the returned Promise.♻️ Align with existing style
- void queryClient.invalidateQueries({ + queryClient.invalidateQueries({ queryKey: [QueryKey.WAVES_V2], });🤖 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/react-query-wrapper/ReactQueryWrapper.tsx` around lines 916 - 918, The call to queryClient.invalidateQueries({ queryKey: [QueryKey.WAVES_V2] }) is prefixed with an inconsistent void; update this call to match the other invalidateQueries usage in the same function by removing the leading void (so it consistently handles the returned Promise the same way as the surrounding calls) or, if you prefer the opposite style, add void to the other invalidateQueries calls—change the single inconsistent occurrence referencing QueryKey.WAVES_V2 to match the prevailing pattern.__tests__/hooks/useWaveDropsLeaderboard.extra.test.ts (1)
115-126: ⚡ Quick winMock the V2 adapter at the hook boundary.
These assertions still couple the hook test to
commonApiFetchinternals even though the hook now depends onfetchWaveLeaderboardV2. Mock/assert the adapter here instead; endpoint-shape checks belong in the service test so harmless adapter refactors do not break this hook test.🤖 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__/hooks/useWaveDropsLeaderboard.extra.test.ts` around lines 115 - 126, The test is tightly coupled to the commonApiFetch implementation; instead mock and assert the v2 adapter used by the hook: replace the expect(commonApiFetch)... assertion with an assertion that fetchWaveLeaderboardV2 was called with the correct parameters (e.g., expect(fetchWaveLeaderboardV2).toHaveBeenCalledWith(expect.objectContaining({ sort: WaveDropsLeaderboardSort.PRICE, curation_id: "curation-group-1", min_price: "0.5", max_price: "2.75", price_currency: "ETH" }))). Ensure the test sets up a jest mock for fetchWaveLeaderboardV2 before rendering the hook and remove any dependence on commonApiFetch internals so adapter refactors won't break this hook test.components/waves/quorum/QuorumParticipationDropLinkPreview.tsx (1)
81-90: ⚡ Quick winThread the query abort signal into the new V2 preview fetch.
The new helper already accepts
signal, but thisqueryFnnever forwards React Query'sAbortSignal. Fast prop changes or unmounts will keep the old request in flight and can race the next preview load.♻️ Suggested change
- queryFn: async () => { + queryFn: async ({ signal }) => { if (normalizedDropId !== null) { return await fetchDropByIdBatched(normalizedDropId); } if (parsedSerialNo !== null) { return await fetchQuorumParticipationDropPreviewBySerialNoV2({ waveId, serialNo: parsedSerialNo, + 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/quorum/QuorumParticipationDropLinkPreview.tsx` around lines 81 - 90, The queryFn is not forwarding React Query's AbortSignal, so long-running preview requests can race; update the queryFn to accept the query context (e.g., ({ signal })) and pass that signal into fetchQuorumParticipationDropPreviewBySerialNoV2 (and into fetchDropByIdBatched if it supports a signal) when calling them using normalizedDropId or parsedSerialNo so in-flight requests are aborted on prop changes/unmounts.components/waves/drops/WaveDropReactions.tsx (1)
116-133: ⚡ Quick winAvoid silently swallowing reaction-detail fetch errors.
The empty
catch {}discards the error and pinsdetailedReactionsStatetoreactions: []. BecausedetailedReactionsthen becomes a truthy empty array, the early-return inloadReactionDetails(Line 107-109) prevents any retry on subsequent hover/focus, and the failure is also invisible to operators. At minimum log the error; consider not caching the failed result so the user can retry on re-hover.♻️ Suggested fix
try { const reactions = await fetchDropReactionDetailsV2(requestDropId); setDetailedReactionsState({ dropId: requestDropId, reactions }); - } catch { - setDetailedReactionsState({ dropId: requestDropId, reactions: [] }); + } catch (error) { + console.error( + `[WaveDropReactions] Failed to load reaction details for drop ${requestDropId}:`, + error + ); + // Intentionally do not cache an empty result here so a subsequent + // hover/focus can retry the request. } 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 `@components/waves/drops/WaveDropReactions.tsx` around lines 116 - 133, The catch currently swallows errors and caches an empty reactions array, preventing retries; update the async request in WaveDropReactions (the IIFE that calls fetchDropReactionDetailsV2) to catch the error as e and log it (e.g., console.error or a provided logger) and do NOT setDetailedReactionsState to { dropId, reactions: [] } on failure — either leave detailedReactionsState unchanged or set it to a sentinel (e.g., null/undefined) so loadReactionDetails can retry; maintain clearing of detailsRequestRef.current and setDetailsLoadingDropId in finally as-is.helpers/stream.helpers.ts (1)
137-143: 💤 Low valueNon-null assertion is safe but brittle.
waveId!relies on the upstream guard inprefetchAuthenticatedFeedItems(line 187). If the function is ever called directly, the assertion would silently pass anundefined/nulltofetchWaveDropsFeedV2. Tightening the parameter type tostring(and validating before delegation) would make the contract explicit and remove the!.♻️ Suggested tightening
- waveId, -}: { - queryClient: QueryClient; - headers: Record<string, string>; - waveId: string | null; + waveId, +}: { + queryClient: QueryClient; + headers: Record<string, string>; + waveId: string; }) => { @@ - return await fetchWaveDropsFeedV2({ - waveId: waveId!, + return await fetchWaveDropsFeedV2({ + waveId,🤖 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 `@helpers/stream.helpers.ts` around lines 137 - 143, The code uses a brittle non-null assertion on waveId when calling fetchWaveDropsFeedV2; tighten the contract by changing the caller's parameter type to string (not string | undefined|null) and validate waveId at the top of prefetchAuthenticatedFeedItems (or the function containing this return) before delegation—remove the `waveId!` non-null assertion and either throw or return early if waveId is missing, then pass the validated string into fetchWaveDropsFeedV2 so the callee always receives a concrete string.services/api/notifications-v2-api.ts (1)
212-220: 💤 Low value
DropBoostedspreads the entire v2 additional context.
additional_context: { ...context }copies every field onApiNotificationAdditionalContextV2(reaction, vote, reactors, quote/reply IDs, etc.) into the typedDropBoostednotification, even though only boost-specific fields are relevant. This widens the payload kept in cache and may leak unrelated fields into UI logic that usesObject.keysor similar. Consider picking only the fieldsINotificationDropBoostedexpects, like the cause-specific branches above do.🤖 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/notifications-v2-api.ts` around lines 212 - 220, The DropBoosted case is copying the entire v2 context into additional_context (additional_context: { ...context }), which widens the payload and may leak unrelated fields; change the ApiNotificationCause.DropBoosted branch to construct additional_context by selecting only the boost-specific fields that INotificationDropBoosted expects (i.e., pick/assign only the boost-related properties from context instead of spreading everything) so the returned object for DropBoosted contains only the relevant boost fields.hooks/useNotificationsQuery.tsx (1)
67-79: 💤 Low valueOptional: drop the now-redundant
fetchNotificationswrapper.After the V2 migration this function does nothing beyond forwarding its arguments to
fetchNotificationsV2. You can inline the call in bothqueryFnsites for less indirection.🤖 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 `@hooks/useNotificationsQuery.tsx` around lines 67 - 79, The function fetchNotifications is now a redundant passthrough to fetchNotificationsV2; remove the fetchNotifications wrapper and update the queryFn sites that reference fetchNotifications to call fetchNotificationsV2 directly (preserving the same argument shape: { limit, cause, pageParam, signal }). Also remove any unused import/export of fetchNotifications and run a quick type/check to ensure fetchNotificationsV2 is imported where needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 769a06c5-d933-42a9-9ba8-46aa5fd15448
⛔ Files ignored due to path filters (6)
generated/models/ApiNotificationV2.tsis excluded by!**/generated/**generated/models/ApiWaveOverview.tsis excluded by!**/generated/**generated/models/ApiWaveOverviewContextProfileContext.tsis excluded by!**/generated/**generated/models/ApiWaveOverviewContributor.tsis excluded by!**/generated/**generated/models/ApiWaveOverviewDescriptionDrop.tsis excluded by!**/generated/**generated/models/ObjectSerializer.tsis excluded by!**/generated/**
📒 Files selected for processing (111)
__tests__/components/brain/notifications/NotificationWaveCreated.test.tsx__tests__/components/home/explore-waves/ExploreWaveCard.test.tsx__tests__/components/memes/MemeDropTraits.test.tsx__tests__/components/memes/drops/MemeWinnerDrop.test.tsx__tests__/components/memes/drops/MemesLeaderboardDrop.test.tsx__tests__/components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.test.ts__tests__/components/waves/drop/DefaultSingleWaveDrop.test.tsx__tests__/components/waves/drop/MemesSingleWaveDrop.test.tsx__tests__/components/waves/drop/SingleWaveDropLogs.test.tsx__tests__/components/waves/drop/SingleWaveDropVoters.test.tsx__tests__/components/waves/drop/useSingleWaveDropData.test.tsx__tests__/components/waves/leaderboard/content/WaveLeaderboardDropContent.test.tsx__tests__/components/waves/leaderboard/drops/DefaultWaveLeaderboardDrop.interaction.test.tsx__tests__/components/waves/leaderboard/drops/DefaultWaveLeaderboardDrop.test.tsx__tests__/components/waves/quorum/QuorumParticipationDropLinkPreview.test.tsx__tests__/components/waves/winners/WaveWinnersDropHeaderVoters.test.tsx__tests__/components/waves/winners/drops/MemesWaveWinnerDrop.test.tsx__tests__/hooks/useDmWavesList.test.tsx__tests__/hooks/useUnreadNotifications.test.ts__tests__/hooks/useWaveCurationDrops.test.ts__tests__/hooks/useWaveDrops.test.ts__tests__/hooks/useWaveDropsLeaderboard.extra.test.ts__tests__/hooks/useWaveDropsSearch.test.ts__tests__/hooks/useWavesList.test.tsx__tests__/hooks/useWavesOverview.test.tsx__tests__/hooks/waves/useWaveDecisions.test.ts__tests__/hooks/waves/useWaveSalesDecisions.test.ts__tests__/services/api/notifications-v2-api.test.ts__tests__/services/api/wave-drops-v2-api.test.tsapp/discover/page.tsxcomponents/brain/my-stream/MyStreamWaveChat.tsxcomponents/brain/my-stream/MyStreamWaveContent.tsxcomponents/brain/notifications/NotificationsWrapper.tsxcomponents/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsxcomponents/brain/notifications/utils/navigationUtils.tscomponents/brain/notifications/wave-created/NotificationWaveCreated.tsxcomponents/brain/notifications/wave-created/NotificationWaveFollowBtn.tsxcomponents/home/explore-waves/ExploreWaveCard.tsxcomponents/home/explore-waves/ExploreWavesSection.tsxcomponents/memes/drops/MemeDropTraits.tsxcomponents/memes/drops/MemeWinnerDrop.tsxcomponents/memes/drops/MemesLeaderboardDrop.tsxcomponents/react-query-wrapper/ReactQueryWrapper.tsxcomponents/react-query-wrapper/utils/increaseWavesOverviewDropsCount.tsxcomponents/user/brain/UserPageBrainSidebarMobileStrip.tsxcomponents/user/brain/UserPageBrainSidebarMostActive.tsxcomponents/user/brain/UserPageBrainSidebarWaveItem.tsxcomponents/user/brain/userPageBrainSidebarWave.helpers.tscomponents/waves/drop/SingleWaveDropChat.tsxcomponents/waves/drop/SingleWaveDropLogs.tsxcomponents/waves/drop/SingleWaveDropVoters.tsxcomponents/waves/drop/useSingleWaveDropData.tscomponents/waves/drops/WaveDropActionsMarkUnread.tsxcomponents/waves/drops/WaveDropQuoteWithSerialNo.tsxcomponents/waves/drops/WaveDropReactions.tsxcomponents/waves/drops/WaveDropReactionsDetailDialog.tsxcomponents/waves/drops/WaveDropReply.tsxcomponents/waves/drops/reaction-utils.tscomponents/waves/drops/useDropContent.tscomponents/waves/drops/wave-drops-all/index.tsxcomponents/waves/header/WaveHeaderFollow.tsxcomponents/waves/header/options/mute/WaveMute.tsxcomponents/waves/leaderboard/content/WaveLeaderboardDropContent.tsxcomponents/waves/leaderboard/drops/DefaultWaveLeaderboardDrop.tsxcomponents/waves/leaderboard/drops/footer/WaveLeaderboardDropFooter.tsxcomponents/waves/leaderboard/drops/header/WaveleaderboardDropRaters.tsxcomponents/waves/leaderboard/sidebar/WaveLeaderboardRightSidebarBoostedDrops.tsxcomponents/waves/quorum/QuorumParticipationDropLinkPreview.tsxcomponents/waves/specs/wave-notification-settings/useWaveMuteSettings.tscomponents/waves/winners/WaveWinners.tsxcomponents/waves/winners/WaveWinnersSmall.tsxcomponents/waves/winners/drops/MemesWaveWinnerDrop.tsxcomponents/waves/winners/drops/header/WaveWinnersDropHeaderVoters.tsxcontexts/wave/hooks/useEnhancedWavesListCore.tscontexts/wave/hooks/useNewDropCounter.tscontexts/wave/utils/wave-messages-utils.tshelpers/stream.helpers.tshooks/drops/useDropInteractionRules.tshooks/drops/useDropReaction.tshooks/useConnectedAccountsUnreadNotifications.tshooks/useDmWavesList.tshooks/useDropMessages.tshooks/useFavouriteWavesOfIdentity.tshooks/useNotificationsQuery.tsxhooks/usePinnedWavesServer.tshooks/useUnreadNotifications.tshooks/useVirtualizedWaveDrops.tshooks/useVirtualizedWaveMessages.tshooks/useWaveActivityLogs.tshooks/useWaveBoostedDrops.tshooks/useWaveCurationDrops.tshooks/useWaveDrops.tshooks/useWaveDropsLeaderboard.tshooks/useWaveDropsSearch.tshooks/useWaveTopVoters.tshooks/useWavesList.tshooks/useWavesOverview.tshooks/useWavesV2.tshooks/waves/useWaveDecisions.tshooks/waves/useWaveSalesDecisions.tsopenapi.yamlservices/api/drop-api.tsservices/api/notifications-v2-api.tsservices/api/pinned-waves-api.tsservices/api/quorum-participation-drop-preview-v2-api.tsservices/api/wave-curation-drops-v2-api.tsservices/api/wave-decisions-v2-api.tsservices/api/wave-drops-v2-api.tsservices/api/waves-v2-api.tstypes/feed.types.tstypes/waves.types.ts
💤 Files with no reviewable changes (7)
- components/waves/leaderboard/drops/footer/WaveLeaderboardDropFooter.tsx
- hooks/useWavesOverview.ts
- app/discover/page.tsx
- components/waves/leaderboard/drops/header/WaveleaderboardDropRaters.tsx
- components/waves/leaderboard/content/WaveLeaderboardDropContent.tsx
- tests/hooks/useWavesOverview.test.tsx
- components/waves/leaderboard/drops/DefaultWaveLeaderboardDrop.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
services/api/notifications-v2-api.ts (1)
160-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle unknown
ApiNotificationCausevalues defensively.
mapNotificationV2still falls through with no return for unrecognized causes. That breaks theTypedNotification[]contract atflatMap(mapNotificationV2)and leaves forward-compat entirely to backend/client lockstep. Add adefaultbranch that returns a safe empty array and keeps the exhaustiveness intent explicit.Suggested fix
switch (notification.cause) { case ApiNotificationCause.IdentitySubscribed: return [ { ...base, cause: ApiNotificationCause.IdentitySubscribed, }, ]; @@ case ApiNotificationCause.PriorityAlert: return [ { ...base, cause: ApiNotificationCause.PriorityAlert, related_drops: relatedDrops, additional_context: { ...context }, }, ]; + default: { + const _exhaustive: never = notification.cause; + void _exhaustive; + 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/notifications-v2-api.ts` around lines 160 - 285, mapNotificationV2 currently has no default case for unknown ApiNotificationCause values so it can fall through without returning a TypedNotification[]; update the switch in mapNotificationV2 (which handles ApiNotificationCause cases like IdentitySubscribed, DropVoted, DropReacted, etc.) to include a default branch that returns an empty array ([]) to satisfy the TypedNotification[] contract when called via flatMap(mapNotificationV2) and keep exhaustiveness explicit—optionally emit a debug/warning via your logger in that default branch for visibility.
🤖 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 `@services/api/notifications-v2-api.ts`:
- Around line 253-255: The WaveCreated branch currently spreads
notification.related_wave raw into the payload, causing a different shape and
losing the is_direct_message alias; replace the direct spread with the
normalized form by calling
mapWaveOverviewToNotificationWaveMin(notification.related_wave) (or undefined
when absent) so the WaveCreated case uses the same mapped shape as other
branches and preserves the is_direct_message alias.
In `@services/api/wave-decisions-v2-api.ts`:
- Around line 81-82: The mapper is truncating multi-part drops by always calling
createBasePart(drop) and hardcoding parts_count: 1; update the mapping that
constructs the ApiDrop so it hydrates all parts the same way the feed adapter
does (or make the mapper async) so parts becomes an array of hydrated parts
rather than a single createBasePart result and parts_count reflects the actual
drop.parts.length; specifically adjust the code that sets parts, parts_count,
and any use of createBasePart(drop) to iterate/await over drop.parts (or reuse
the feed adapter’s part-hydration function) so the returned ApiDrop contains the
full parts array and correct parts_count.
In `@services/api/wave-drops-v2-api.ts`:
- Around line 108-115: The catch blocks around calls to commonApiFetch (e.g., in
fetchDropPartV2 that calls commonApiFetch with getDropEndpointId) are swallowing
AbortSignal cancellations and returning null/empty results; change each catch to
capture the error (catch (err)), re-throw if it’s an abort/cancellation (e.g.,
err.name === 'AbortError' or err.code === 'ERR_CANCELED' or err instanceof
DOMException with name 'AbortError'), and only return the null/[] fallback for
real non-abort fetch errors; apply the same pattern to the other fetch helpers
mentioned (the other try/catch blocks at the referenced sections that return
null/[]).
---
Duplicate comments:
In `@services/api/notifications-v2-api.ts`:
- Around line 160-285: mapNotificationV2 currently has no default case for
unknown ApiNotificationCause values so it can fall through without returning a
TypedNotification[]; update the switch in mapNotificationV2 (which handles
ApiNotificationCause cases like IdentitySubscribed, DropVoted, DropReacted,
etc.) to include a default branch that returns an empty array ([]) to satisfy
the TypedNotification[] contract when called via flatMap(mapNotificationV2) and
keep exhaustiveness explicit—optionally emit a debug/warning via your logger in
that default branch for visibility.
🪄 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: 42ee1f3f-21ae-4901-b625-47298a5e0490
📒 Files selected for processing (6)
services/api/drop-v2-mappers.tsservices/api/notifications-v2-api.tsservices/api/quorum-participation-drop-preview-v2-api.tsservices/api/wave-curation-drops-v2-api.tsservices/api/wave-decisions-v2-api.tsservices/api/wave-drops-v2-api.ts
✅ Files skipped from review due to trivial changes (1)
- services/api/quorum-participation-drop-preview-v2-api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- services/api/wave-curation-drops-v2-api.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
__tests__/services/api/drop-api.test.ts (2)
11-18: 💤 Low valueRemove redundant
jest.useRealTimers()call.Calling
jest.useRealTimers()in bothbeforeEach(line 12) andafterEach(line 17) is redundant. ThebeforeEachcall is sufficient to reset timers before each test.♻️ Proposed cleanup
beforeEach(() => { jest.useRealTimers(); jest.clearAllMocks(); }); afterEach(() => { - jest.useRealTimers(); });🤖 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/drop-api.test.ts` around lines 11 - 18, The afterEach block contains a redundant jest.useRealTimers() call because beforeEach already resets timers; remove the jest.useRealTimers() line from the afterEach so only beforeEach calls jest.useRealTimers(), keeping jest.clearAllMocks() in beforeEach unchanged and leaving afterEach only for any remaining cleanup if needed (refer to the beforeEach and afterEach blocks and the jest.useRealTimers() calls in the test file).
56-78: ⚡ Quick winMove
fetchDropByIdBatchedtest to its own describe block.This test validates
fetchDropByIdBatchedbehavior but is nested insidedescribe("fetchDropsByIds", ...). Extract it into a separatedescribe("fetchDropByIdBatched", ...)block for better test organization and clarity.📁 Suggested test structure
}); +}); + +describe("fetchDropByIdBatched", () => { it("resolves valid batched requests when another batched id fails", async () => { jest.useFakeTimers();🤖 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/drop-api.test.ts` around lines 56 - 78, The test that exercises fetchDropByIdBatched is currently inside describe("fetchDropsByIds", ...) and should be moved to its own describe("fetchDropByIdBatched", ...) block; extract the it(...) block as-is into a new describe wrapper named "fetchDropByIdBatched", keep the fetchDropV2ByIdMock.mockImplementation, jest.useFakeTimers(), the two calls to fetchDropByIdBatched("valid-drop") and fetchDropByIdBatched("deleted-drop"), the Promise.all assertions, jest.runOnlyPendingTimers(), and the final expect(fetchDropV2ByIdMock).toHaveBeenCalledTimes(2); ensure any shared setup/teardown (mocks/reset, jest.useRealTimers()) required by the surrounding test file is applied to the new describe block so the test environment stays consistent.__tests__/services/api/notifications-v2-api.test.ts (1)
125-136: ⚡ Quick winSimplify type narrowing with standard Jest assertions.
The current pattern uses a type guard with a thrown error, which is less idiomatic for Jest tests and produces less descriptive failure messages.
♻️ Refactor to use standard Jest assertions
- const [firstNotification] = response.notifications; - if ( - firstNotification?.cause === ApiNotificationCause.DropReacted && - "related_drops" in firstNotification - ) { - const mappedDrop = firstNotification.related_drops[0]!; - expect(mappedDrop.wave.id).toBe("wave-1"); - expect(mappedDrop.wave).toMatchObject({ is_direct_message: true }); - expect(mappedDrop.parts[0]?.content).toBe("hello"); - return; - } - - throw new Error("Expected drop reacted notification"); + const [firstNotification] = response.notifications; + expect(firstNotification?.cause).toBe(ApiNotificationCause.DropReacted); + expect(firstNotification).toHaveProperty("related_drops"); + + if (firstNotification?.cause === ApiNotificationCause.DropReacted && "related_drops" in firstNotification) { + const mappedDrop = firstNotification.related_drops[0]!; + expect(mappedDrop.wave.id).toBe("wave-1"); + expect(mappedDrop.wave).toMatchObject({ is_direct_message: true }); + expect(mappedDrop.parts[0]?.content).toBe("hello"); + }🤖 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/notifications-v2-api.test.ts` around lines 125 - 136, Replace the manual type-narrowing + thrown error pattern with standard Jest assertions: assert that firstNotification is defined, assert firstNotification.cause equals ApiNotificationCause.DropReacted, and assert firstNotification has the related_drops property before accessing it; then continue the existing expectations on mappedDrop.wave.id, mappedDrop.wave.is_direct_message and mappedDrop.parts content. Use the symbols firstNotification, ApiNotificationCause.DropReacted, related_drops and mappedDrop to locate where to change the test.components/react-query-wrapper/ReactQueryWrapper.tsx (2)
990-999: ⚡ Quick winImproved error handling, but silent suppression may hide issues.
The change from
voidto.catch(() => undefined)prevents unhandled promise rejections, which is good. However, silently suppressing all errors could hide legitimate issues with notification invalidation (e.g., query client in a bad state, configuration problems).Additionally, this pattern is inconsistent with
invalidateAllWaves()(lines 909-936), where most invalidations don't suppress errors.Consider:
- Log suppressed errors for debugging visibility
- Document why notification invalidations need error suppression
- Apply consistently across all invalidation functions
🛡️ Alternative: Log errors while preventing rejection
const invalidateNotifications = () => { queryClient .invalidateQueries({ queryKey: [QueryKey.IDENTITY_NOTIFICATIONS], }) - .catch(() => undefined); + .catch((error) => { + console.warn('Failed to invalidate identity notifications:', error); + }); queryClient .invalidateQueries({ queryKey: [QueryKey.CONNECTED_ACCOUNT_UNREAD_NOTIFICATIONS], }) - .catch(() => undefined); + .catch((error) => { + console.warn('Failed to invalidate unread notifications:', error); + }); };🤖 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/react-query-wrapper/ReactQueryWrapper.tsx` around lines 990 - 999, The two invalidateQueries calls that currently use .catch(() => undefined) for QueryKey.IDENTITY_NOTIFICATIONS and QueryKey.CONNECTED_ACCOUNT_UNREAD_NOTIFICATIONS should preserve suppression of unhandled rejections but log the error for visibility; update those catches to call the project's logger (or console.error) with a descriptive message including the query key and the error, and make this behavior consistent with other invalidation helpers (e.g., invalidateAllWaves) or document why notifications are treated differently so all invalidation paths either log or explicitly suppress errors in the same way.
916-920: ⚡ Quick winInconsistent error handling pattern across query invalidations.
Only the
WAVES_V2invalidation uses.catch(() => undefined), while other invalidations in the same function (lines 911-915, 921-935) don't suppress errors. This inconsistency makes the code harder to maintain and suggests special handling without explanation.Consider one of these approaches:
- If this is temporary migration code: Add a comment explaining why
WAVES_V2needs error suppression during the transition period- If this is the desired pattern: Apply
.catch(() => undefined)consistently to allinvalidateQueriescalls in this function- If errors should be visible: Remove the
.catch()to let potential issues surface📝 Example: Document migration-specific handling
+ // During V1->V2 migration, WAVES_V2 queries may not exist yet. + // Suppress errors to prevent issues during transition. queryClient .invalidateQueries({ queryKey: [QueryKey.WAVES_V2], }) .catch(() => undefined);🤖 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/react-query-wrapper/ReactQueryWrapper.tsx` around lines 916 - 920, The invalidateQueries call for QueryKey.WAVES_V2 is the only one using .catch(() => undefined), creating an inconsistent error-handling pattern; decide and apply one consistent approach: either (A) if suppression is temporary for migration, add a clear comment above the queryClient.invalidateQueries({ queryKey: [QueryKey.WAVES_V2] }) call explaining the migration window and why errors are being ignored, or (B) if suppressed errors are desired globally, add .catch(() => undefined) to all other queryClient.invalidateQueries calls in this function, or (C) if errors should surface, remove the .catch(() => undefined) on the WAVES_V2 invalidation so all invalidateQueries calls behave the same; update only the invalidateQueries calls and the comment near QueryKey.WAVES_V2 accordingly.
🤖 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__/components/waves/drop/useSingleWaveDropData.test.tsx`:
- Around line 47-112: Tests in the describe("useSingleWaveDropData") suite leak
mock state between cases (e.g., fetchDropV2ByIdMock retains
calls/implementations), so add a beforeEach hook at the top of that describe
that calls jest.resetAllMocks() (or jest.resetAllMocks(); jest.clearAllMocks()
if you prefer) to reset fetchDropV2ByIdMock and other mocks before each it
block; place the beforeEach directly inside the
describe("useSingleWaveDropData") scope so it runs before tests that use
useSingleWaveDropData, renderHook, fetchDropV2ByIdMock, etc.
In `@components/brain/notifications/wave-created/NotificationWaveCreated.tsx`:
- Around line 17-24: The code uses waveId (derived from
notification.related_wave and notification.additional_context.wave_id) to build
invitationHref via getWaveRoute and render a link, but it lacks a null guard;
update NotificationWaveCreated to check if waveId is present before calling
getWaveRoute and before rendering an anchor: if waveId is missing, skip
generating invitationHref and render plain fallback text (e.g., waveName or
“Unknown wave”) as non-clickable content; ensure the logic around variables
wave, waveId, invitationHref, and getWaveRoute is adjusted so getWaveRoute is
only invoked when waveId is truthy and the rendered link is only an <a> when
waveId exists.
---
Nitpick comments:
In `@__tests__/services/api/drop-api.test.ts`:
- Around line 11-18: The afterEach block contains a redundant
jest.useRealTimers() call because beforeEach already resets timers; remove the
jest.useRealTimers() line from the afterEach so only beforeEach calls
jest.useRealTimers(), keeping jest.clearAllMocks() in beforeEach unchanged and
leaving afterEach only for any remaining cleanup if needed (refer to the
beforeEach and afterEach blocks and the jest.useRealTimers() calls in the test
file).
- Around line 56-78: The test that exercises fetchDropByIdBatched is currently
inside describe("fetchDropsByIds", ...) and should be moved to its own
describe("fetchDropByIdBatched", ...) block; extract the it(...) block as-is
into a new describe wrapper named "fetchDropByIdBatched", keep the
fetchDropV2ByIdMock.mockImplementation, jest.useFakeTimers(), the two calls to
fetchDropByIdBatched("valid-drop") and fetchDropByIdBatched("deleted-drop"), the
Promise.all assertions, jest.runOnlyPendingTimers(), and the final
expect(fetchDropV2ByIdMock).toHaveBeenCalledTimes(2); ensure any shared
setup/teardown (mocks/reset, jest.useRealTimers()) required by the surrounding
test file is applied to the new describe block so the test environment stays
consistent.
In `@__tests__/services/api/notifications-v2-api.test.ts`:
- Around line 125-136: Replace the manual type-narrowing + thrown error pattern
with standard Jest assertions: assert that firstNotification is defined, assert
firstNotification.cause equals ApiNotificationCause.DropReacted, and assert
firstNotification has the related_drops property before accessing it; then
continue the existing expectations on mappedDrop.wave.id,
mappedDrop.wave.is_direct_message and mappedDrop.parts content. Use the symbols
firstNotification, ApiNotificationCause.DropReacted, related_drops and
mappedDrop to locate where to change the test.
In `@components/react-query-wrapper/ReactQueryWrapper.tsx`:
- Around line 990-999: The two invalidateQueries calls that currently use
.catch(() => undefined) for QueryKey.IDENTITY_NOTIFICATIONS and
QueryKey.CONNECTED_ACCOUNT_UNREAD_NOTIFICATIONS should preserve suppression of
unhandled rejections but log the error for visibility; update those catches to
call the project's logger (or console.error) with a descriptive message
including the query key and the error, and make this behavior consistent with
other invalidation helpers (e.g., invalidateAllWaves) or document why
notifications are treated differently so all invalidation paths either log or
explicitly suppress errors in the same way.
- Around line 916-920: The invalidateQueries call for QueryKey.WAVES_V2 is the
only one using .catch(() => undefined), creating an inconsistent error-handling
pattern; decide and apply one consistent approach: either (A) if suppression is
temporary for migration, add a clear comment above the
queryClient.invalidateQueries({ queryKey: [QueryKey.WAVES_V2] }) call explaining
the migration window and why errors are being ignored, or (B) if suppressed
errors are desired globally, add .catch(() => undefined) to all other
queryClient.invalidateQueries calls in this function, or (C) if errors should
surface, remove the .catch(() => undefined) on the WAVES_V2 invalidation so all
invalidateQueries calls behave the same; update only the invalidateQueries calls
and the comment near QueryKey.WAVES_V2 accordingly.
🪄 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: 4609104b-2170-45e7-9fc6-4fb520acc787
📒 Files selected for processing (21)
__tests__/components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.test.ts__tests__/components/waves/drop/useSingleWaveDropData.test.tsx__tests__/services/api/drop-api.test.ts__tests__/services/api/notifications-v2-api.test.ts__tests__/services/api/wave-drops-v2-api.test.tscomponents/brain/notifications/wave-created/NotificationWaveCreated.tsxcomponents/home/explore-waves/ExploreWavesSection.tsxcomponents/react-query-wrapper/ReactQueryWrapper.tsxcomponents/react-query-wrapper/utils/increaseWavesOverviewDropsCount.tsxcomponents/waves/drop/useSingleWaveDropData.tscomponents/waves/drops/WaveDropActionsMarkUnread.tsxcomponents/waves/drops/WaveDropReactions.tsxcomponents/waves/header/options/mute/WaveMute.tsxcontexts/wave/utils/wave-messages-utils.tshooks/useWavesV2.tsservices/api/drop-api.tsservices/api/notifications-v2-api.tsservices/api/quorum-participation-drop-preview-v2-api.tsservices/api/wave-drops-v2-api.tsservices/api/waves-v2-api.tstypes/feed.types.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- types/feed.types.ts
- components/waves/drops/WaveDropActionsMarkUnread.tsx
- tests/components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.test.ts
- services/api/quorum-participation-drop-preview-v2-api.ts
- components/waves/header/options/mute/WaveMute.tsx
- tests/services/api/wave-drops-v2-api.test.ts
- hooks/useWavesV2.ts
- contexts/wave/utils/wave-messages-utils.ts
- services/api/notifications-v2-api.ts
- components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.tsx
- components/waves/drops/WaveDropReactions.tsx
- services/api/wave-drops-v2-api.ts
- services/api/waves-v2-api.ts
- components/waves/drop/useSingleWaveDropData.ts
- components/home/explore-waves/ExploreWavesSection.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/brain/notifications/wave-created/NotificationWaveCreated.tsx`:
- Around line 48-52: In NotificationWaveCreated (the JSX rendering that shows
the "invited you to a wave:" span and the conditional Link controlled by
invitationHref), add an explicit space before the conditional element so the
rendered text doesn't collapse (e.g., insert a {" "} between the closing span
and the {invitationHref ? ( ... )} block); update the markup around the
span/Link in NotificationWaveCreated.tsx to include that explicit space so
output becomes "wave: My Wave" instead of "wave:My Wave".
In `@components/waves/drops/DropAuthorBadges.tsx`:
- Around line 73-75: getProfileWaveId currently uses the nullish coalescing
operator which treats an empty string as present and prevents falling back to
badges.profile_wave_id; update getProfileWaveId to treat empty or
whitespace-only profile.profile_wave_id as missing by trimming/checking for a
non-empty string first and only then falling back to
profile.badges?.profile_wave_id (and finally null) so downstream logic like
is_wave_creator receives a valid id or null.
🪄 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: fe8d5d70-b95c-460c-b4c4-4a054e2ec8ca
📒 Files selected for processing (8)
__tests__/components/brain/notifications/NotificationWaveCreated.test.tsx__tests__/components/waves/drop/useSingleWaveDropData.test.tsx__tests__/components/waves/drops/DropAuthorBadges.test.tsx__tests__/services/api/wave-drops-v2-api.test.tscomponents/brain/notifications/wave-created/NotificationWaveCreated.tsxcomponents/waves/drops/DropAuthorBadges.tsxhelpers/artist-activity.helpers.tsservices/api/drop-v2-mappers.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/services/api/wave-drops-v2-api.test.ts
- tests/components/waves/drop/useSingleWaveDropData.test.tsx
- services/api/drop-v2-mappers.ts
- tests/components/brain/notifications/NotificationWaveCreated.test.tsx
simo6529
previously approved these changes
May 11, 2026
Signed-off-by: GelatoGenesis <tarmokalling@gmail.com>
|
simo6529
approved these changes
May 11, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openapi.yaml`:
- Around line 5991-5993: The migration hint for the deprecated /waves-overview
(operationId getWavesOverview) should explicitly mention the legacy
overview_type mapping so clients preserve previous behavior: update the
description line that says "Use GET /v2/waves?view=OVERVIEW instead." to include
that callers must map the old `type`/legacy values to the new `overview_type`
(or equivalent query param `view`) and enumerate or reference the mapping rules
(e.g., legacy type -> new overview_type values) so consumers migrating from
/waves-overview to GET /v2/waves?view=OVERVIEW retain the same semantics.
🪄 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: 3c2e0bfe-0c40-4c4c-b92e-f09793d823d1
📒 Files selected for processing (7)
__tests__/hooks/useCommunityCurationsDrops.test.tscomponents/waves/drops/CurationDropFooter.tsxcomponents/waves/leaderboard/content/WaveLeaderboardDropContent.tsxcomponents/waves/leaderboard/drops/DefaultWaveLeaderboardDrop.tsxhooks/useCommunityCurationsDrops.tsopenapi.yamlservices/api/drop-v2-mappers.ts
💤 Files with no reviewable changes (2)
- components/waves/leaderboard/drops/DefaultWaveLeaderboardDrop.tsx
- components/waves/leaderboard/content/WaveLeaderboardDropContent.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- services/api/drop-v2-mappers.ts
This was referenced 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
New Features
Improvements