fix: docs, preferences, mobile nav#43
Conversation
|
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:
📝 WalkthroughWalkthroughFrontend: feed typing/pagination tightened, Profile tab state consolidated, MobileNav adds unread counts and an Annotations item, getFeed return shape changed; README scripts switched from npm to bun. Backend: Firehose ingester persists a new optional Preferences field DisableExternalLinkWarning. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/navigation/MobileNav.tsx (1)
36-42:⚠️ Potential issue | 🟠 MajorAdd cleanup guard to prevent state updates after unmount or auth change.
The effect can resolve after unmount or when
isAuthenticatedflips, causing a stalesetUnreadCount. Use a cleanup guard to check if the component is still active before updating state (established pattern in MasonryFeed.tsx):♻️ Suggested fix
useEffect(() => { + let active = true; if (isAuthenticated) { getUnreadNotificationCount() .then((count) => { + if (active) setUnreadCount(count || 0); - setUnreadCount(count || 0); }) .catch(() => {}); } + return () => { + active = false; + }; }, [isAuthenticated]);
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/views/profile/Profile.tsx (1)
266-309:⚠️ Potential issue | 🟠 MajorEarly return leaves
loadingMorestuck attrue.When
capturedTab === "collections", the function returns on line 278 aftersetLoadingMore(true)was called on line 275. Thefinallyblock that resetssetLoadingMore(false)is never executed, leaving the loading state permanently true for subsequent renders.Suggested fix: Move early return before setting loading state
const loadMore = useCallback(async () => { const isHandle = !did.startsWith("did:"); const resolvedDid = isHandle ? profile?.did : did; if (!resolvedDid) return; const tabPagination = pagination[activeTab]; if (!tabPagination) return; const capturedTab = activeTab; + + if (capturedTab === "collections") return; + setLoadingMore(true); setLoadMoreError(null); - - if (capturedTab === "collections") return; - try {
🧹 Nitpick comments (2)
web/src/components/feed/MasonryFeed.tsx (1)
1-1: Remove redundant import alias.
useStoreis imported twice—once asuseNanoStoreand once directly. Since both reference the same function, pick one and use it consistently throughout the component (lines 125-126 currently mix both).Suggested fix
-import { useStore as useNanoStore, useStore } from "@nanostores/react"; +import { useStore } from "@nanostores/react";Then update line 126:
- const layout = useNanoStore($feedLayout); + const layout = useStore($feedLayout);web/src/views/core/Feed.tsx (1)
73-96: Inconsistent null-safety handling inloadMore.Lines 83 and 85 use optional chaining (
data?.items,data?.hasMore) defensively, but line 90 directly accessesdata.fetchedCount. SincegetFeedguarantees a completeFeedResponsein all paths, the optional chaining is unnecessary. Consider aligning the approach for consistency.Option A: Trust the types (recommended)
const data = await getFeed({ type, motivation, tag, limit: LIMIT, offset, }); - const fetched = data?.items || []; + const fetched = data.items; setItems((prev) => [...prev, ...fetched]); - if (data?.hasMore !== undefined) { - setHasMore(data.hasMore); - } else { - setHasMore(fetched.length >= LIMIT); - } + setHasMore(data.hasMore); setOffset((prev) => prev + data.fetchedCount);Option B: Be consistently defensive
setOffset((prev) => prev + data.fetchedCount); + setOffset((prev) => prev + (data?.fetchedCount ?? fetched.length));
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/src/views/profile/Profile.tsx`:
- Around line 275-278: The early-return for the collections tab leaks state
because setLoadingMore(true) and setLoadMoreError(null) run before returning;
modify the loadMore handler (the function containing setLoadingMore and
setLoadMoreError and the capturedTab/activeTab check) so the capturedTab ===
"collections" check runs before any state updates, or alternatively ensure
loadingMore is reset in the finally path; update references to setLoadingMore,
setLoadMoreError, and the capturedTab/activeTab check accordingly to prevent
leaving loadingMore stuck true.
🧹 Nitpick comments (1)
web/src/views/profile/Profile.tsx (1)
434-435: Variable shadowing: localitemsshadows component state.The local variable
itemsshadows the component-levelitemsstate. While functionally correct due to different scopes, this can cause confusion during maintenance.♻️ Suggested rename
<MoreMenu items={(() => { - const items: MoreMenuItem[] = []; - items.push({ + const menuItems: MoreMenuItem[] = []; + menuItems.push({Apply the same rename to all subsequent
items.push(...)calls within this callback.
2 participants
bun(npmdoesn't seem to work)Summary by CodeRabbit
New Features
Improvements
Documentation