Conversation
📝 WalkthroughWalkthroughThis PR consolidates multiple enhancements: migrating in-memory caches to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/home/now-minting/NowMintingSection.tsx (1)
31-36: Bug:new Array(4).map()does not iterate over empty slots.
new Array(4)creates an array with 4 empty slots, but.map()skips empty slots entirely, resulting in no skeleton items being rendered. Additionally, the spread operator{...}here attempts to spread an array into JSX children incorrectly.🐛 Proposed fix
<div className="tw-grid tw-grid-cols-2 tw-gap-3"> - {...new Array(4).map((_, i) => ( + {Array.from({ length: 4 }).map((_, i) => ( <div key={i} className="tw-space-y-2"> <div className="tw-h-4 tw-w-16 tw-animate-pulse tw-rounded tw-bg-iron-800/50" /> <div className="tw-h-6 tw-w-24 tw-animate-pulse tw-rounded tw-bg-iron-800/50" /> </div> ))}hooks/useWaves.ts (1)
57-75: Use debouncedParams.limit to keep queryFn aligned with the queryKey.The
queryKeyincludesdebouncedParamswhich contains thelimitproperty, but thequeryFnuses the outerlimitvariable instead. Iflimitchanges before the debounce window closes, thequeryFnwill fetch with the new limit while thequeryKeyremains unchanged, causing React Query to return cached results from a different request. Always use values fromdebouncedParamsin thequeryFnto maintain cache coherency.🛠️ Suggested fix
- queryParams["limit"] = `${limit}`; + queryParams["limit"] = `${debouncedParams.limit}`;
🧹 Nitpick comments (2)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx (2)
50-62: Consider using partial typing instead ofas anyfor better type safety.Using
as anybypasses type checking, which could hide issues if the actual wave type changes. Consider usingPartial<T>or a test utility type to maintain some type safety while still allowing partial objects.💡 Suggested improvement
+import { ApiWave } from "@/generated/models/ApiWave"; // or appropriate type - const baseWave = { + const baseWave: Partial<ApiWave> & { newDropsCount: any; isPinned: boolean } = { id: "1", type: ApiWaveType.Chat, name: "Chat Wave", picture: "", contributors: [], newDropsCount: { count: 2, latestDropTimestamp: 123 }, isPinned: false, unreadDropsCount: 0, latestReadTimestamp: 0, firstUnreadDropSerialNo: null, isMuted: false, - } as any; + };
144-160: Icon detection usingdocument.querySelectorAllis pragmatic but fragile.Using
[data-icon="bell-slash"]relies on FontAwesome's internal attribute structure. This is a common workaround when testing icons, but consider adding adata-testidto the muted indicator wrapper in the component for more resilient tests.If the component can be modified, a more stable approach would be:
// In the component, wrap the muted indicator: <span data-testid="muted-indicator"> <FontAwesomeIcon icon={faBellSlash} /> </span>Then test with:
expect(screen.queryByTestId("muted-indicator")).toBeInTheDocument();
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/home/boosted/BoostedSection.tsx (1)
39-51: Layout inconsistency between loading and loaded states may cause visual shift.The loading state (line 41) includes
-tw-mx-8with innertw-px-8, while the loaded state (line 58) omits the negative margin. Additionally, padding differs:tw-py-16in loading vstw-py-10 md:tw-py-16in loaded. This mismatch can cause Cumulative Layout Shift (CLS) when transitioning from loading to loaded.Consider aligning the section wrapper classes for both states to prevent layout jumping.
Suggested fix to align loading state with loaded state
if (isLoading) { return ( - <section className="-tw-mx-8 tw-relative tw-bg-iron-950 tw-px-4 tw-py-16 md:tw-px-6 lg:tw-px-8"> + <section className="tw-relative tw-bg-iron-950 tw-px-4 tw-py-10 md:tw-px-6 md:tw-py-16 lg:tw-px-8"> <div className="tw-pointer-events-none tw-absolute tw-inset-x-0 tw-top-0 tw-h-px tw-bg-[linear-gradient(90deg,transparent_0%,rgba(255,255,255,0.08)_15%,rgba(255,255,255,0.08)_85%,transparent_100%)]" /> <div className="tw-pointer-events-none tw-absolute tw-inset-x-0 tw-bottom-0 tw-h-px tw-bg-[linear-gradient(90deg,transparent_0%,rgba(255,255,255,0.08)_15%,rgba(255,255,255,0.08)_85%,transparent_100%)]" /> - <div className="tw-relative tw-px-8"> + <div className="tw-relative"> <div className="tw-flex tw-h-64 tw-items-center tw-justify-center">Also applies to: 57-61
🧹 Nitpick comments (1)
components/home/HomePageTextSection.tsx (1)
4-4: Consider adding overflow containment to the section.The decorative gradient uses
180%width/height which extends beyond the section bounds. While thetw-pointer-events-noneprevents interaction issues, this could potentially cause horizontal scrollbars on some viewport sizes if ancestor elements don't clip overflow.💡 Optional: Add overflow-hidden to contain the gradient
- <section className="tw-relative tw-px-3 tw-pb-10 tw-pt-8 sm:tw-px-4 md:tw-px-6 md:tw-pb-16 md:tw-pt-10 lg:tw-px-8"> + <section className="tw-relative tw-overflow-hidden tw-px-3 tw-pb-10 tw-pt-8 sm:tw-px-4 md:tw-px-6 md:tw-pb-16 md:tw-pt-10 lg:tw-px-8">
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hooks/useWavesList.ts`:
- Line 49: allWavesRef is only refreshed when areWavesEqual deems arrays
different, but that comparator omits UI-relevant fields (e.g.,
metrics.latest_drop_timestamp) causing stale displayed data; update the equality
gate used when setting allWavesRef (or switch to returning combinedWaves
directly) to include a stable version/timestamp field (e.g.,
metrics.latest_drop_timestamp or an updated_at) or any UI-facing properties from
EnhancedWave so changes to those fields cause the ref/state to update and
consumers to re-render; adjust the comparison function (areWavesEqual) or
replace its usage around allWavesRef/combinedWaves to ensure those fields are
considered.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/useWavesList.ts (1)
215-231: Dependency array issues will cause unnecessary recomputation.Two problems with the dependency array:
prevMainWavesRef.current(line 227): Ref values don't trigger re-renders when mutated, making this dependency unreliable. Thereact-hooks/exhaustive-depsrule typically flags this pattern.
missingPinnedIds(line 228): This is a constant[]declared at line 87, creating a new array reference on every render. This defeats the memoization and causes recomputation on every render cycle.🐛 Proposed fix
+// Move outside component or memoize +const EMPTY_MISSING_IDS: string[] = []; const useWavesList = () => { // ... - const missingPinnedIds: string[] = []; + // Use the constant instead // ... return useMemo( () => ({ // ... - mainWaves: prevMainWavesRef.current, - missingPinnedIds, + mainWaves, // Use the actual mainWaves from useWavesOverview + missingPinnedIds: EMPTY_MISSING_IDS, // ... }), [ // ... - prevMainWavesRef.current, - missingPinnedIds, + mainWaves, // ... ] ); };
♻️ Duplicate comments (1)
hooks/useWavesList.ts (1)
74-79: Same side-effect pattern as noted above.The ref mutation at line 77 inside
useMemofollows the same pattern flagged forallPinnedWaves. Consider consolidating these ref updates outside of memoization callbacks if strict concurrent-mode safety is needed.
🧹 Nitpick comments (2)
hooks/useWavesList.ts (2)
103-119: Side effect inuseMemo- consider extracting.Mutating
prevPinnedWavesRef.currentinsideuseMemois a side effect. While benign here, React may invoke memoization callbacks multiple times in concurrent mode. Consider extracting the ref update or using a separate effect if this comparison becomes critical.
235-237: Unsafeanycast bypasses type checking.The
as anycast onw.chatdisables type safety for the nested property access. IfApiWavedoesn't define this structure, consider extending the type or adding a type guard. The optional chaining mitigates runtime errors but won't catch API schema changes at compile time.♻️ Type-safe alternative
+interface ChatWithDmScope { + scope?: { + group?: { + is_direct_message?: boolean; + }; + }; +} const waveIsDm = (w: ApiWave) => w.wave.type === ApiWaveType.Chat && - (w.chat as any)?.scope?.group?.is_direct_message === true; + (w.chat as ChatWithDmScope | undefined)?.scope?.group?.is_direct_message === true;
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/home/boosted/BoostedDropCardHome.tsx (3)
211-216: Fix unclosed<span>in “Time ago” pill.There are two opening
<span>tags but only one closing tag, which is a JSX parse error.🐛 Proposed fix
- <div className="tw-flex tw-items-center tw-rounded-full tw-border tw-border-solid tw-border-white/10 tw-bg-black/40 tw-px-2.5 tw-py-1 tw-shadow-[inset_0_1px_0_rgba(255,255,255,0.06)] tw-backdrop-blur-md tw-transition-colors group-hover:tw-bg-black/60"> - <span className="tw-text-[10px] tw-font-semibold tw-leading-4 tw-tracking-wide tw-text-zinc-300"> - <span className="tw-text-[10px] tw-font-semibold tw-leading-4 tw-tracking-wide tw-text-zinc-300"> + <div className="tw-flex tw-items-center tw-rounded-full tw-border tw-border-solid tw-border-white/10 tw-bg-black/40 tw-px-2.5 tw-py-1 tw-shadow-[inset_0_1px_0_rgba(255,255,255,0.06)] tw-backdrop-blur-md tw-transition-colors group-hover:tw-bg-black/60"> + <span className="tw-text-[10px] tw-font-semibold tw-leading-4 tw-tracking-wide tw-text-zinc-300"> {getTimeAgoShort(drop.created_at)} </span> </div>
225-230: Fix unclosed<span>in remaining boosts indicator.Two
<span>openings but only one closing tag → JSX parse error.🐛 Proposed fix
- {remainingBoosts > 0 && ( - <span className="tw-ml-1 tw-text-[10px] tw-font-bold tw-tabular-nums tw-leading-4 tw-text-iron-50"> - <span className="tw-ml-1 tw-text-[10px] tw-font-bold tw-tabular-nums tw-leading-4 tw-text-iron-50"> - +{remainingBoosts} - </span> - )} + {remainingBoosts > 0 && ( + <span className="tw-ml-1 tw-text-[10px] tw-font-bold tw-tabular-nums tw-leading-4 tw-text-iron-50"> + +{remainingBoosts} + </span> + )}
234-250: Close the extra media wrapper<div>.There are two consecutive wrapper
<div>openings, but only one is closed before the ternary ends. That’s a JSX parse error and will break the component.🐛 Proposed fix
- {media ? ( - <div className="tw-relative tw-aspect-[5/2] tw-w-full tw-overflow-hidden tw-rounded-xl sm:tw-aspect-[5/4] md:tw-aspect-[8/5] lg:tw-aspect-[5/4] xl:tw-aspect-[8/5]"> - <div className="tw-relative tw-aspect-[5/2] tw-w-full tw-overflow-hidden tw-rounded-xl sm:tw-aspect-[5/4] md:tw-aspect-[8/5] lg:tw-aspect-[5/4] xl:tw-aspect-[8/5]"> + {media ? ( + <div className="tw-relative tw-aspect-[5/2] tw-w-full tw-overflow-hidden tw-rounded-xl sm:tw-aspect-[5/4] md:tw-aspect-[8/5] lg:tw-aspect-[5/4] xl:tw-aspect-[8/5]"> + <div className="tw-relative tw-aspect-[5/2] tw-w-full tw-overflow-hidden tw-rounded-xl sm:tw-aspect-[5/4] md:tw-aspect-[8/5] lg:tw-aspect-[5/4] xl:tw-aspect-[8/5]"> <div className="tw-relative tw-h-full tw-w-full"> <div className="tw-relative tw-z-0 tw-flex tw-h-full tw-w-full tw-items-center tw-justify-center tw-rounded-xl tw-transition-transform tw-duration-700 group-hover:tw-scale-[1.02]"> <div className="tw-relative tw-h-full tw-w-full tw-overflow-hidden"> <DropListItemContentMedia media_mime_type={media.mime_type} media_url={media.url} imageScale={ImageScale.AUTOx450} imageObjectPosition="center" /> </div> </div> </div> - </div> + </div> + </div> ) : (
🧹 Nitpick comments (1)
__tests__/components/waves/drops/WaveDropsAll.test.tsx (1)
135-167: Consider reusing the shared drop factory to reduce mock drift.There’s already a
createMockDrophelper in__tests__/utils/editDropTestUtils.tsx. Reusing it (or importing and extending it here) keeps mock shapes aligned with evolvingApiDropfields and avoids divergence across suites.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/waves/list/WaveItemWide.tsx`:
- Line 152: Update the isDirectMessage expression to safely handle absent chat
by adding optional chaining on chat and scope: change the usage in WaveItemWide
where isDirectMessage is derived from wave to use
wave?.chat?.scope?.group?.is_direct_message ?? false (reference symbol:
isDirectMessage and wave in WaveItemWide.tsx) so runtime errors don't occur when
chat or scope is undefined.
🧹 Nitpick comments (3)
components/waves/list/WaveItemWide.tsx (3)
109-113:aria-labelon a plain<div>has no accessibility effect.Screen readers ignore
aria-labelon elements without an interactive role. Since the non-interactive case represents a wave that cannot be navigated to, consider removing thearia-labelfrom the div to avoid misleading the markup.Suggested fix
return ( - <div className={className} aria-label={ariaLabel}> + <div className={className}> {children} </div> );
274-278: Redundant Space key check.
event.key === " "is sufficient for detecting the Space key press;event.code === "Space"is redundant here.Suggested simplification
if ( event.key === "Enter" || - event.key === " " || - event.code === "Space" + event.key === " " ) {
341-347: Note: Zero counts display as "-" due tonumberWithCommasbehavior.The helper
numberWithCommasreturns"-"for zero values. Combined with the singular/plural logicdropsCount === 1 ? "Drop" : "Drops", a wave with 0 drops will display as- Drops. If displaying0 Dropsis preferred, consider adjusting the zero-handling logic.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.