Conversation
- scaffold `apps/mobile` with Expo Router, Clerk auth, tabs, feeds, bookmarks, and article reader - extract feed parsing/content/cache logic into new `packages/shared` - move Convex server code to workspace root and update web app to use shared feed + subscription APIs
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces a complete React Native mobile application (using Expo) with authentication, feed browsing, bookmarking, and article reading capabilities. Concurrently, it extracts common feed-related logic (parsing, content sanitization, caching, utilities) into a new shared package that both the web and mobile apps consume. The web application is refactored to delegate feed operations to shared modules and integrate with Convex for server-side feed subscription management. The backend schema is updated to support feed subscriptions and enhanced bookmark tracking. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![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 (1)
apps/web/src/components/feed-reader/source-sync-controller.tsx (1)
29-71:⚠️ Potential issue | 🟠 MajorPrevent cross-source dedupe in refresh guard.
Line 29 and Line 66 dedupe only on
checkedAt. If the component switches sources without remount and two sources share the same timestamp,onRefreshcan be skipped for the new source.Suggested fix
- const lastAppliedCheck = useRef(lastCheckedAt); + const lastAppliedCheck = useRef<{ + sourceId: string; + checkedAt?: string; + }>({ + sourceId: source.sourceId, + checkedAt: lastCheckedAt, + }); + useEffect(() => { + lastAppliedCheck.current = { + sourceId: source.sourceId, + checkedAt: lastCheckedAt, + }; + }, [source.sourceId, lastCheckedAt]); useEffect(() => { - if (!query.data || query.data.checkedAt === lastAppliedCheck.current) { + if ( + !query.data || + (lastAppliedCheck.current.sourceId === source.sourceId && + query.data.checkedAt === lastAppliedCheck.current.checkedAt) + ) { return; } - lastAppliedCheck.current = query.data.checkedAt; + lastAppliedCheck.current = { + sourceId: source.sourceId, + checkedAt: query.data.checkedAt, + }; onRefresh(query.data); - }, [onRefresh, query.data]); + }, [onRefresh, query.data, source.sourceId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/feed-reader/source-sync-controller.tsx` around lines 29 - 71, The dedupe currently only compares checkedAt (lastAppliedCheck) so if the component swaps to a different source with the same timestamp on mount, onRefresh may be skipped; update the guard to include source identity: change lastAppliedCheck (useRef) to store a compound key (e.g., `${source.sourceId}:${checkedAt}`) and update both the initialization (useRef should use `${source.sourceId}:${lastCheckedAt}`) and the comparison in the useEffect to compare query.data by building the same compound key from query.data.checkedAt and source.sourceId before early-return; when setting lastAppliedCheck.current and calling onRefresh, use the compound key to ensure dedupe is per-source.
🟡 Minor comments (8)
apps/mobile/.expo/README.md-10-13 (1)
10-13:⚠️ Potential issue | 🟡 MinorKeep
.expo/untracked to match the documented policy.Line 10–Line 13 says this folder is machine-specific and should not be shared, but this PR adds a tracked file under
apps/mobile/.expo/. Move this note to a tracked location (for example,apps/mobile/README.md) and keep.expo/fully ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/.expo/README.md` around lines 10 - 13, The PR mistakenly adds a tracked file inside the machine-specific ".expo/" folder; remove that tracked file from version control (unstage/remove it so ".expo/" stays untracked), copy or move the README note content into the tracked project README (e.g., add the same note into the mobile module's README.md), and ensure ".expo/" is listed in the repository .gitignore so no files under ".expo/" are committed in future.apps/mobile/app/(auth)/sign-in.tsx-10-18 (1)
10-18:⚠️ Potential issue | 🟡 MinorHandle Clerk's loading state before checking
isSignedIn.
useAuth()also returnsisLoadedwhich isfalsewhile Clerk is initializing. During this time,isSignedInisundefined, which means the redirect won't trigger but the sign-in UI will briefly flash before redirecting authenticated users.Proposed fix to handle loading state
export default function SignInScreen() { - const { isSignedIn } = useAuth(); + const { isSignedIn, isLoaded } = useAuth(); const { startOAuthFlow } = useOAuth({ strategy: "oauth_google" }); const [error, setError] = useState<string | null>(null); const [isPending, setIsPending] = useState(false); + if (!isLoaded) { + return null; // or a loading spinner + } + if (isSignedIn) { return <Redirect href="/(tabs)" />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/`(auth)/sign-in.tsx around lines 10 - 18, The SignInScreen component currently checks only isSignedIn and lets the sign-in UI flash while Clerk is initializing; update the component to also read isLoaded from useAuth and gate rendering/redirects on it (e.g., if !isLoaded return null or a loading placeholder), and only perform the Redirect when isLoaded && isSignedIn; adjust any state/logic that assumes isSignedIn is always defined so you handle the undefined/initializing case returned by useAuth.apps/mobile/src/components/ui/button.tsx-79-80 (1)
79-80:⚠️ Potential issue | 🟡 MinorMap the loading spinner color per variant.
The current ternary treats
undefined,secondary,ghost, andoutlinethe same, so the default primary button and the ink-colored variants render the wrong indicator while loading.Proposed fix
+const spinnerColors = { + primary: "#ffffff", + secondary: "#0c7a5a", + ghost: "#211d1a", + outline: "#211d1a", +} as const; + export function Button({ children, className, @@ {loading ? ( - <ActivityIndicator color={variant === "primary" ? "#ffffff" : "#0c7a5a"} /> + <ActivityIndicator color={spinnerColors[variant ?? "primary"]} /> ) : typeof children === "string" || label ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/src/components/ui/button.tsx` around lines 79 - 80, The loading spinner color is currently determined by a simple ternary that treats undefined and non-primary variants the same, causing primary buttons to render the wrong indicator; update the Button component (where loading, variant and <ActivityIndicator> are used) to compute a spinnerColor based on the variant value (e.g., switch or map for "primary", "secondary", "ghost", "outline", and default/undefined) and pass that spinnerColor into the ActivityIndicator color prop so each variant shows the correct spinner.apps/mobile/app/(tabs)/bookmarks.tsx-10-11 (1)
10-11:⚠️ Potential issue | 🟡 MinorKeep loading distinct from empty.
useQuery()isundefineduntil Convex responds. Converting that to[]makes the screen briefly render “No bookmarks yet.” during every load/auth transition before the real data arrives.Suggested fix
export default function BookmarksScreen() { - const bookmarks = (useQuery(api.bookmarks.queries.listForCurrentUser, {}) ?? - []) as Doc<"bookmarks">[]; + const bookmarks = useQuery(api.bookmarks.queries.listForCurrentUser, {}) as + | Doc<"bookmarks">[] + | undefined; return ( <ScrollView className="flex-1 bg-canvas" contentContainerStyle={{ padding: 16, paddingBottom: 120 }} > - {bookmarks.length === 0 ? ( + {bookmarks === undefined ? ( + <View className="items-center rounded-[34px] border border-line bg-card px-6 py-10"> + <Text className="text-base text-muted">Loading bookmarks…</Text> + </View> + ) : bookmarks.length === 0 ? ( <View className="items-center rounded-[34px] border border-dashed border-line bg-card px-6 py-10"> <BookmarkSimple size={32} color="#8a7b6a" weight="duotone" /> <Text className="mt-4 text-xl font-semibold">No bookmarks yet.</Text>Also applies to: 18-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/`(tabs)/bookmarks.tsx around lines 10 - 11, The current code coerces useQuery(...) return value to [] which hides loading (useQuery is undefined while fetching) and causes a transient "No bookmarks yet." UI; change to keep the raw query result separate (e.g., const bookmarksResult = useQuery(api.bookmarks.queries.listForCurrentUser, {})) and only map to an empty array after confirming the query is not undefined (e.g., when bookmarksResult !== undefined then const bookmarks = (bookmarksResult as Doc<"bookmarks">[]) ?? []); update all similar usages in this file (the other occurrences around lines 18-25) and render a loading state when bookmarksResult === undefined instead of treating it as an empty list.packages/shared/src/feed/utils.ts-64-76 (1)
64-76:⚠️ Potential issue | 🟡 Minor
createExcerptcan overshootmaxLength.The truncation keeps
maxLength - 1characters and then appends"...", so the returned string can be two characters longer than the requested limit.Suggested fix
export const createExcerpt = (value: string | undefined, maxLength = 180) => { const plainText = stripHtml(value); if (!plainText) { return undefined; } if (plainText.length <= maxLength) { return plainText; } - return `${plainText.slice(0, maxLength - 1).trimEnd()}...`; + const suffix = "..."; + const end = Math.max(0, maxLength - suffix.length); + return `${plainText.slice(0, end).trimEnd()}${suffix}`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/feed/utils.ts` around lines 64 - 76, createExcerpt currently takes plainText.slice(0, maxLength - 1) then appends "..." which can exceed maxLength; fix createExcerpt by ensuring the returned string length is at most maxLength: when plainText.length > maxLength, slice to maxLength - 3 and append "..." (and handle small maxLength values by returning plainText.slice(0, maxLength) or a truncated ellipsis-safe string when maxLength <= 3) so that createExcerpt and its use of stripHtml and plainText always produce strings <= maxLength.apps/mobile/app/(tabs)/index.tsx-1-15 (1)
1-15:⚠️ Potential issue | 🟡 MinorDrive pull-to-refresh from real pending state.
The
refreshAll()function returns a Promise but the handler ignores it withvoid, leavingrefreshing={false}hardcoded. This means the refresh indicator clears immediately and users can trigger multiple refreshes while one is in flight. Track the refresh state with a localuseStateand awaitrefreshAll()within the handler.Suggested fix
+import { useState } from "react"; import { RefreshControl, ScrollView, View } from "react-native"; @@ export default function HomeScreen() { const { sourceSummaries, refreshAll, refreshSource, removeFeed } = useFeedData(); + const [isRefreshing, setIsRefreshing] = useState(false); @@ - refreshControl={<RefreshControl refreshing={false} onRefresh={() => void refreshAll()} />} + refreshControl={ + <RefreshControl + refreshing={isRefreshing} + onRefresh={async () => { + if (isRefreshing) return; + setIsRefreshing(true); + try { + await refreshAll(); + } finally { + setIsRefreshing(false); + } + }} + /> + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/`(tabs)/index.tsx around lines 1 - 15, HomeScreen currently ignores the Promise from refreshAll() and hardcodes RefreshControl refreshing={false}; add a local state (e.g., const [isRefreshing, setIsRefreshing] = useState(false)) and update the RefreshControl to use refreshing={isRefreshing}; in the onRefresh handler, if isRefreshing return early, otherwise setIsRefreshing(true), await refreshAll(), and setIsRefreshing(false) in a finally block so the indicator stays visible while refreshAll() is in flight and duplicate refreshes are prevented.apps/web/src/components/feed-reader/feed-reader-app.tsx-469-474 (1)
469-474:⚠️ Potential issue | 🟡 MinorHonor
authRedirectwhen a session already exists.This branch always navigates to
/, so an auth-intent link with a non-root redirect loses its destination for users who are already signed in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/feed-reader/feed-reader-app.tsx` around lines 469 - 474, The early-return navigation always forces "/" when hasClerkSession is true, which drops any intended auth redirect; update the branch in feed-reader-app.tsx that calls navigate (the block using hasClerkSession and navigate) to check for an authRedirect value (e.g., a prop, route search param, or existing variable named authRedirect) and navigate to that destination if present, otherwise fall back to "/"; also preserve any existing search/query parameters instead of passing an empty search object and keep replace: true.packages/shared/src/feed/cache.ts-154-157 (1)
154-157:⚠️ Potential issue | 🟡 MinorAdd per-source fallback when migrated pagination entry is missing.
If
paginationBySourceexists but lackssource.sourceId, Line 154-157 setspaginationtoundefined. That can silently disable load-more for migrated sources.Suggested fix
const pagination = "paginationBySource" in state - ? state.paginationBySource[source.sourceId] + ? state.paginationBySource[source.sourceId] ?? createSourcePagination(source) : createSourcePagination(source);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/feed/cache.ts` around lines 154 - 157, When computing pagination, handle the case where state.paginationBySource exists but has no entry for source.sourceId by falling back to createSourcePagination(source); specifically update the logic that sets the pagination variable (which currently checks "paginationBySource" in state and indexes state.paginationBySource[source.sourceId]) so that if state.paginationBySource[source.sourceId] is undefined it uses createSourcePagination(source) instead, ensuring load-more isn't disabled for migrated sources.
🧹 Nitpick comments (8)
apps/web/.gitignore (1)
17-18:.env.localis redundant with existing*.local.You can keep only
.env.productionhere to reduce duplication.Suggested cleanup
-.env.local .env.production🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/.gitignore` around lines 17 - 18, Remove the redundant explicit entry ".env.local" from the .gitignore since the existing "*.local" glob already ignores it; keep ".env.production" and ensure the "*.local" pattern remains to cover all local env files.apps/mobile/package.json (2)
26-26: Preview dependency may introduce instability.
nativewindat version5.0.0-preview.3is a pre-release. Preview versions can have breaking changes between releases and may lack production stability guarantees. Consider pinning to the exact preview version (remove caret) to avoid unexpected breakage, or document the rationale for using a preview release.Suggested change to pin exact version
- "nativewind": "^5.0.0-preview.3", + "nativewind": "5.0.0-preview.3",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/package.json` at line 26, The package.json currently uses a caret range for the pre-release dependency "nativewind": "^5.0.0-preview.3", which can pull newer incompatible preview patches; change it to a pinned exact version by removing the caret (use "nativewind": "5.0.0-preview.3") or alternatively add a brief comment in package.json and the PR description documenting why a preview release is required and any testing/rollback plan; update the dependency entry and commit the change so installs remain deterministic.
13-39: Inconsistent version pinning strategy.Some dependencies use exact versions (e.g.,
react-native-reanimated,react-native-safe-area-context) while others use caret ranges. For a mobile app where native module compatibility is critical, consider standardizing on exact versions for all native dependencies to ensure reproducible builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/package.json` around lines 13 - 39, The dependencies list mixes exact pins and caret ranges which risks native module mismatches; standardize by pinning exact versions for all native modules (e.g., react-native-reanimated, react-native-safe-area-context, react-native-gesture-handler, react-native-screens, react-native-svg, react-native-webview, expo-secure-store, react-native-render-html, react-native-url-polyfill) — update their entries in package.json to exact versions (remove ^ ranges) to match the versions you expect, then reinstall (yarn/npm) and commit the updated lockfile to ensure reproducible native builds.apps/web/src/components/feed-reader/item-list.tsx (1)
36-36: Consider extracting hostname using URL API for robustness.The regex approach works for common cases, but
URLparsing handles edge cases (ports, auth, paths) more reliably.♻️ Suggested improvement
- const sourceHost = source?.siteUrl.replace(/^https?:\/\//, "").replace(/\/$/, ""); + const sourceHost = source?.siteUrl ? new URL(source.siteUrl).hostname : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/feed-reader/item-list.tsx` at line 36, Replace the ad-hoc regex hostname extraction with the URL API: parse source?.siteUrl inside a try/catch (or guard) to create new URL(source.siteUrl) and use url.host or url.hostname (to include port if desired) to set sourceHost, falling back to the existing .replace(...) logic or an empty string when parsing fails or source?.siteUrl is falsy; update the symbol sourceHost and the usage of source?.siteUrl so the code safely handles undefined and malformed URLs.apps/mobile/app/(tabs)/_layout.tsx (2)
47-61: Consider extracting hardcoded colors to a theme constant.Multiple hardcoded hex values (e.g.,
#0c7a5a,#8a7b6a,#fff9f1,#211d1a) are used throughout the component. Extracting these to a shared theme object would improve maintainability and consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/`(tabs)/_layout.tsx around lines 47 - 61, Replace the hardcoded hex colors used in the Tabs screenOptions with references to a shared theme/constants object: extract values for tabBarActiveTintColor, tabBarInactiveTintColor, tabBarStyle.backgroundColor, tabBarStyle.borderTopColor, headerStyle.backgroundColor and headerTintColor into a theme (e.g., theme.colors.primary, theme.colors.muted, theme.colors.background, theme.colors.border, theme.colors.headerText) and update the Tabs component to use those theme tokens (look for the Tabs declaration and its screenOptions, plus keys tabBarActiveTintColor, tabBarInactiveTintColor, tabBarStyle, headerStyle, headerTintColor and insets usage) so colors are centralized and reusable.
125-139: Consider simplifying preference updates.The current pattern requires passing all preference fields even when updating a single value. If
updatePreferencessupports partial updates, this can be simplified:♻️ Suggested simplification (if API supports partials)
onPress={() => - void updatePreferences({ - pollingIntervalMinutes: value, - defaultView: preferences.defaultView, - }) + void updatePreferences({ pollingIntervalMinutes: value }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/`(tabs)/_layout.tsx around lines 125 - 139, The update currently always sends the entire preferences object (pollingIntervalMinutes and defaultView) from the Button map; change the onPress handler in the component rendering Buttons so it calls updatePreferences with only the changed field (pollingIntervalMinutes) to simplify updates—i.e., pass a partial update like { pollingIntervalMinutes: value } to updatePreferences and remove passing preferences.defaultView; if updatePreferences' type/signature (the function named updatePreferences) does not accept partials, update its definition to accept Partial<Preferences> or an optional field update so the call with a single property compiles and behaves correctly.apps/mobile/src/lib/preferences.ts (1)
3-6: Consider usingsatisfiesfor better type safety.The
as ArticleViewModeassertion bypasses type checking. Usingsatisfiesensures the value is validated at compile time while preserving the literal type.♻️ Suggested improvement
+import type { ArticleViewMode } from "@repo/shared/feed/types"; + +type UserPreferences = { + pollingIntervalMinutes: number; + defaultView: ArticleViewMode; +}; + -export const defaultUserPreferences = { +export const defaultUserPreferences = { pollingIntervalMinutes: 15, - defaultView: "reader" as ArticleViewMode, -}; + defaultView: "reader", +} satisfies UserPreferences;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/src/lib/preferences.ts` around lines 3 - 6, The object defaultUserPreferences currently uses a type assertion ("as ArticleViewMode") which bypasses compile-time checking; replace the assertion with a satisfies clause to validate the object shape and the defaultView literal against the ArticleViewMode union (e.g., export const defaultUserPreferences = { pollingIntervalMinutes: 15, defaultView: "reader" } satisfies { pollingIntervalMinutes: number; defaultView: ArticleViewMode } or satisfies UserPreferences if that type exists) and remove the "as ArticleViewMode" cast so the compiler ensures the value is valid while keeping the literal type.apps/mobile/app/feed/[sourceId].tsx (1)
34-40: Use object-form navigation to improve rendering latency.While the current path interpolation is safe (item IDs are always numeric hashes), passing article metadata via
paramsallows ArticleScreen to render with actual data immediately, rather than showing a placeholder untilensureItem()resolves. This improves perceived performance.Suggested fix
onPress={() => { markRead(sourceId, item.id); - router.push(`/article/${sourceId}/${item.id}`); + router.push({ + pathname: "/article/[sourceId]/[itemId]", + params: { + sourceId, + itemId: item.id, + url: item.url, + title: item.title, + excerpt: item.excerpt, + publishedAt: item.publishedAt, + imageUrl: item.imageUrl, + sourceLabel: source?.label, + sourceSiteUrl: source?.siteUrl, + }, + }); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/feed/`[sourceId].tsx around lines 34 - 40, Replace the string-path navigation in the Pressable onPress with object-form navigation so ArticleScreen can receive item metadata immediately; instead of router.push(`/article/${sourceId}/${item.id}`) call router.push({ pathname: '/article/[sourceId]/[id]', params: { sourceId, id: item.id, item } }) (keep the existing markRead(sourceId, item.id) call) and ensure ArticleScreen/ensureItem reads from params for optimistic render when available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/app/_layout.tsx`:
- Around line 17-18: Guard construction of ConvexReactClient by checking
process.env.EXPO_PUBLIC_CONVEX_URL and only call new ConvexReactClient(...) when
that env var is non-empty; change the top-level convex declaration to be
optional (e.g., let convex: ConvexReactClient | undefined) and assign it only if
EXPO_PUBLIC_CONVEX_URL is truthy, and update any usage in the RootLayout render
path to handle convex === undefined (show fallback UI or skip providing the
client) so startup does not instantiate the client with an empty string;
reference ConvexReactClient, convex, EXPO_PUBLIC_CONVEX_URL and the RootLayout
component when applying the fix.
In `@apps/mobile/app/article/`[sourceId]/[itemId].tsx:
- Around line 35-37: The bookmark toggle calls the non-idempotent mutation
api.bookmarks.mutations.toggleForCurrentUser via toggleBookmark and can enqueue
duplicate mutations on rapid taps; add a local pending guard (e.g.,
isTogglingBookmark state) around the onPress/handler that triggers
toggleBookmark.mutate to early-return if true, set isTogglingBookmark = true
before calling mutate and reset it in onSuccess/onError/onSettled of the
mutation; alternatively replace toggleForCurrentUser with explicit add/remove
mutations and guard similarly; apply the same fix to the other handler(s)
referenced around lines 111-126 that call the same mutation.
- Around line 50-53: The current code falls back to source?.siteUrl or "" for
item.url before ensureItem() resolves, causing site-mode, bookmark toggle, and
"Open original" to act on the feed homepage; instead, stop substituting
source.siteUrl and keep the article URL undefined until item.url or params.url
exists. Create a derived articleUrl variable that reads item?.url ?? params.url
(no fallback to source.siteUrl or ""), use articleUrl for the WebView source,
for the bookmark payload and for Linking.openURL, and disable/guard the
site-mode toggle, bookmark actions, and "Open original" button when articleUrl
is falsy so those actions only operate when a real article URL is present
(update usages around ensureItem, the bookmark toggle logic, the Open original
handler, and the WebView props).
In `@apps/mobile/src/components/source-card.tsx`:
- Around line 39-43: The two icon-only Button instances rendering ArrowClockwise
and Trash lack accessibilityLabel/accessibilityHint, so add props to each Button
(the one with onPress={onRefresh} and the one with onPress={onRemove}) supplying
a clear accessibilityLabel ("Refresh" / "Remove") and brief accessibilityHint
("Refresh the source" / "Remove the source") so screen readers can differentiate
their actions; update the JSX for the Button components that wrap ArrowClockwise
and Trash to include these props (target the Button component in
source-card.tsx).
In `@apps/mobile/src/providers/feed-provider.tsx`:
- Around line 61-64: The subscriptions query result is being collapsed to []
with "?? []", hiding the loading state and causing the reconcile effect to treat
pending queries as an empty list and wipe the cache; change the usage of
useQuery(api.feedSubscriptions.queries.listForCurrentUser, ...) so it is not
coerced to [] (i.e., keep it possibly undefined rather than using "?? []"),
update the local variable (subscriptions) type to allow undefined, and add a
guard in the reconcile logic that references subscriptions (or check
query.isLoading/isFetching) so it only treats an empty array as "no
subscriptions" after the query has resolved; apply the same change pattern to
the other occurrence at lines 114-121.
- Around line 77-103: The async hydration in the useEffect can continue after
userId changes and leaves isCacheReady false on read errors; fix it by making
the load cancellable and ensuring setIsCacheReady(true) always runs: inside the
useEffect create a `cancelled` flag (or AbortController) and return a cleanup
that sets it, move the AsyncStorage.getItem call into a try/catch/finally block
so any thrown error still reaches the finally, check `if (cancelled) return`
before calling setCache or setIsCacheReady, and use the same
createStorageKey(userId), localFeedCacheSchema parsing, setCache, and
setIsCacheReady calls so state updates are skipped when cancelled but
isCacheReady is always settled.
In `@apps/web/src/components/feed-reader/source-form.tsx`:
- Around line 65-75: The refresh Button becomes icon-only at sm+ and needs an
explicit accessible name: add an aria-label (e.g., aria-label="Refresh all
feeds") to the Button component that wraps the ArrowClockwiseIcon (keep the
existing title prop if desired) so screen readers get a proper label when the
visible text (<span className="sm:hidden">Refresh all feeds</span>) is hidden;
update the Button element where onRefreshAll, isRefreshing, and
ArrowClockwiseIcon are used to include aria-label.
In `@apps/web/src/components/feed-reader/store.ts`:
- Around line 179-183: The code is closing the detail panel by checking only
feedSubscriptionsAtom (sources) against detailPanel.sourceId, which causes the
panel to be sanitized to { mode: "closed" } when subscriptions lag behind cache
updates after addSourceSuccessAtom; change the validation in the detail panel
sanitizer to also check the cached source list (or the atom updated by
addSourceSuccessAtom) before closing — i.e., when validating
detailPanel.sourceId in the sanitizer that references sources and detailPanel,
consult both the feedSubscriptionsAtom-derived sources and the cache/atom
populated by addSourceSuccessAtom (or skip closing and set a transient
"loading"/"syncing" state) so the panel isn’t prematurely closed while the
subscription list catches up.
In `@apps/web/src/components/feed-reader/use-feed-reader.ts`:
- Around line 341-347: handleRemoveSource currently removes the source locally
(removeFeedSource and queryClient.removeQueries) before the server call
removeSubscriptionMutation.mutateAsync completes, causing no rollback if the
server fails; change the flow so the server mutation is attempted first and only
on success update local state and clear the cache (or implement
mutateAsync/mutate with onMutate/onError/onSuccess callbacks to perform an
optimistic update with a stored rollback that restores the source and query
cache on error). Specifically, adjust handleRemoveSource to call
removeSubscriptionMutation.mutate or mutateAsync and move/removeFeedSource and
queryClient.removeQueries into the onSuccess handler (or implement onMutate to
snapshot state and onError to restore that snapshot), referencing
handleRemoveSource, removeFeedSource, removeSubscriptionMutation.mutateAsync (or
mutate with onMutate/onError/onSuccess), queryClient.removeQueries and
queryKeys.sourceItems to locate where to change.
In `@package.json`:
- Around line 12-17: The root-level "dev" npm script is causing a recursive
Turbo loop because turbo.json also exposes a root "dev" task; remove the root
"dev" task from turbo.json (keep only the //#convex root task) or rename the
turbo root task to "//#dev" so it no longer matches the npm script, and then
update the package.json scripts that call "turbo run dev" (e.g., the "dev",
"dev:web", "dev:mobile" scripts) to target the correct task name ("convex" or
the new renamed task) instead of invoking the root "dev" task to prevent
recursion.
In `@packages/shared/package.json`:
- Line 24: The dependency entry for "@typescript/native-preview" in package.json
is using a careted dev snapshot which can float and differs from the rest of the
workspace; update the "@typescript/native-preview" version specifier to the
exact pinned version "7.0.0-dev.20260311.1" (remove the leading ^) so it matches
the other packages and prevents inconsistent type-checking across the workspace.
In `@packages/shared/src/feed/parser.ts`:
- Around line 324-348: The RSS 1.0 (rdf:RDF) branch is pulling items from
channel.item and resolving relative item/media URLs against siteUrl, which
breaks RDF feeds; change the rdf branch to use the root rss (rdf:RDF) items (use
asArray(rss.item) instead of asArray(channel.item)) and keep resolving relative
item/media URLs against the fetched feed URL (baseUrl) when calling
normalizeRssItems — e.g., compute siteUrl the same way for labeling but pass
baseUrl (or a new feedBaseUrl variable) as the second argument to
normalizeRssItems for rdf feeds so items and media URLs are resolved relative to
baseUrl.
In `@packages/shared/src/feed/service.ts`:
- Around line 32-50: The fetchRemoteDocument helper currently follows redirects
and fully buffers responses, which allows SSRF to private IPs and unbounded body
sizes; update fetchRemoteDocument to (1) validate the final URL after redirects
using the response.url (or equivalent) against your URL
policy/allowlist/blocklist (reject private RFC1918/loopback/metadata ranges and
disallowed hosts) and throw if disallowed, and (2) cap the response size before
calling response.text() by streaming response.body (use the ReadableStream
reader) and aborting/throwing once a configured max bytes threshold is reached
(e.g. 1–2MB) using the existing createTimeoutSignal/AbortController so you never
buffer unlimited content; reference fetchRemoteDocument, response.url,
response.headers.get, feedRequestHeaders and createTimeoutSignal when making
these changes.
In `@packages/shared/src/feed/utils.ts`:
- Around line 13-40: normalizeInputUrl and resolveUrl currently accept any URL
scheme; update both to only allow http: and https: by validating url.protocol
after constructing the URL. In normalizeInputUrl (function normalizeInputUrl)
after creating the URL and stripping hash, throw a descriptive Error if
url.protocol is not "http:" or "https:". In resolveUrl (function resolveUrl)
after creating the URL and stripping hash, return undefined if url.protocol is
not "http:" or "https:". Ensure the protocol check uses the URL object's
protocol property so non-HTTP(S) schemes like file:, data:, javascript: are
rejected before returning.
---
Outside diff comments:
In `@apps/web/src/components/feed-reader/source-sync-controller.tsx`:
- Around line 29-71: The dedupe currently only compares checkedAt
(lastAppliedCheck) so if the component swaps to a different source with the same
timestamp on mount, onRefresh may be skipped; update the guard to include source
identity: change lastAppliedCheck (useRef) to store a compound key (e.g.,
`${source.sourceId}:${checkedAt}`) and update both the initialization (useRef
should use `${source.sourceId}:${lastCheckedAt}`) and the comparison in the
useEffect to compare query.data by building the same compound key from
query.data.checkedAt and source.sourceId before early-return; when setting
lastAppliedCheck.current and calling onRefresh, use the compound key to ensure
dedupe is per-source.
---
Minor comments:
In `@apps/mobile/.expo/README.md`:
- Around line 10-13: The PR mistakenly adds a tracked file inside the
machine-specific ".expo/" folder; remove that tracked file from version control
(unstage/remove it so ".expo/" stays untracked), copy or move the README note
content into the tracked project README (e.g., add the same note into the mobile
module's README.md), and ensure ".expo/" is listed in the repository .gitignore
so no files under ".expo/" are committed in future.
In `@apps/mobile/app/`(auth)/sign-in.tsx:
- Around line 10-18: The SignInScreen component currently checks only isSignedIn
and lets the sign-in UI flash while Clerk is initializing; update the component
to also read isLoaded from useAuth and gate rendering/redirects on it (e.g., if
!isLoaded return null or a loading placeholder), and only perform the Redirect
when isLoaded && isSignedIn; adjust any state/logic that assumes isSignedIn is
always defined so you handle the undefined/initializing case returned by
useAuth.
In `@apps/mobile/app/`(tabs)/bookmarks.tsx:
- Around line 10-11: The current code coerces useQuery(...) return value to []
which hides loading (useQuery is undefined while fetching) and causes a
transient "No bookmarks yet." UI; change to keep the raw query result separate
(e.g., const bookmarksResult =
useQuery(api.bookmarks.queries.listForCurrentUser, {})) and only map to an empty
array after confirming the query is not undefined (e.g., when bookmarksResult
!== undefined then const bookmarks = (bookmarksResult as Doc<"bookmarks">[]) ??
[]); update all similar usages in this file (the other occurrences around lines
18-25) and render a loading state when bookmarksResult === undefined instead of
treating it as an empty list.
In `@apps/mobile/app/`(tabs)/index.tsx:
- Around line 1-15: HomeScreen currently ignores the Promise from refreshAll()
and hardcodes RefreshControl refreshing={false}; add a local state (e.g., const
[isRefreshing, setIsRefreshing] = useState(false)) and update the RefreshControl
to use refreshing={isRefreshing}; in the onRefresh handler, if isRefreshing
return early, otherwise setIsRefreshing(true), await refreshAll(), and
setIsRefreshing(false) in a finally block so the indicator stays visible while
refreshAll() is in flight and duplicate refreshes are prevented.
In `@apps/mobile/src/components/ui/button.tsx`:
- Around line 79-80: The loading spinner color is currently determined by a
simple ternary that treats undefined and non-primary variants the same, causing
primary buttons to render the wrong indicator; update the Button component
(where loading, variant and <ActivityIndicator> are used) to compute a
spinnerColor based on the variant value (e.g., switch or map for "primary",
"secondary", "ghost", "outline", and default/undefined) and pass that
spinnerColor into the ActivityIndicator color prop so each variant shows the
correct spinner.
In `@apps/web/src/components/feed-reader/feed-reader-app.tsx`:
- Around line 469-474: The early-return navigation always forces "/" when
hasClerkSession is true, which drops any intended auth redirect; update the
branch in feed-reader-app.tsx that calls navigate (the block using
hasClerkSession and navigate) to check for an authRedirect value (e.g., a prop,
route search param, or existing variable named authRedirect) and navigate to
that destination if present, otherwise fall back to "/"; also preserve any
existing search/query parameters instead of passing an empty search object and
keep replace: true.
In `@packages/shared/src/feed/cache.ts`:
- Around line 154-157: When computing pagination, handle the case where
state.paginationBySource exists but has no entry for source.sourceId by falling
back to createSourcePagination(source); specifically update the logic that sets
the pagination variable (which currently checks "paginationBySource" in state
and indexes state.paginationBySource[source.sourceId]) so that if
state.paginationBySource[source.sourceId] is undefined it uses
createSourcePagination(source) instead, ensuring load-more isn't disabled for
migrated sources.
In `@packages/shared/src/feed/utils.ts`:
- Around line 64-76: createExcerpt currently takes plainText.slice(0, maxLength
- 1) then appends "..." which can exceed maxLength; fix createExcerpt by
ensuring the returned string length is at most maxLength: when plainText.length
> maxLength, slice to maxLength - 3 and append "..." (and handle small maxLength
values by returning plainText.slice(0, maxLength) or a truncated ellipsis-safe
string when maxLength <= 3) so that createExcerpt and its use of stripHtml and
plainText always produce strings <= maxLength.
---
Nitpick comments:
In `@apps/mobile/app/`(tabs)/_layout.tsx:
- Around line 47-61: Replace the hardcoded hex colors used in the Tabs
screenOptions with references to a shared theme/constants object: extract values
for tabBarActiveTintColor, tabBarInactiveTintColor, tabBarStyle.backgroundColor,
tabBarStyle.borderTopColor, headerStyle.backgroundColor and headerTintColor into
a theme (e.g., theme.colors.primary, theme.colors.muted,
theme.colors.background, theme.colors.border, theme.colors.headerText) and
update the Tabs component to use those theme tokens (look for the Tabs
declaration and its screenOptions, plus keys tabBarActiveTintColor,
tabBarInactiveTintColor, tabBarStyle, headerStyle, headerTintColor and insets
usage) so colors are centralized and reusable.
- Around line 125-139: The update currently always sends the entire preferences
object (pollingIntervalMinutes and defaultView) from the Button map; change the
onPress handler in the component rendering Buttons so it calls updatePreferences
with only the changed field (pollingIntervalMinutes) to simplify updates—i.e.,
pass a partial update like { pollingIntervalMinutes: value } to
updatePreferences and remove passing preferences.defaultView; if
updatePreferences' type/signature (the function named updatePreferences) does
not accept partials, update its definition to accept Partial<Preferences> or an
optional field update so the call with a single property compiles and behaves
correctly.
In `@apps/mobile/app/feed/`[sourceId].tsx:
- Around line 34-40: Replace the string-path navigation in the Pressable onPress
with object-form navigation so ArticleScreen can receive item metadata
immediately; instead of router.push(`/article/${sourceId}/${item.id}`) call
router.push({ pathname: '/article/[sourceId]/[id]', params: { sourceId, id:
item.id, item } }) (keep the existing markRead(sourceId, item.id) call) and
ensure ArticleScreen/ensureItem reads from params for optimistic render when
available.
In `@apps/mobile/package.json`:
- Line 26: The package.json currently uses a caret range for the pre-release
dependency "nativewind": "^5.0.0-preview.3", which can pull newer incompatible
preview patches; change it to a pinned exact version by removing the caret (use
"nativewind": "5.0.0-preview.3") or alternatively add a brief comment in
package.json and the PR description documenting why a preview release is
required and any testing/rollback plan; update the dependency entry and commit
the change so installs remain deterministic.
- Around line 13-39: The dependencies list mixes exact pins and caret ranges
which risks native module mismatches; standardize by pinning exact versions for
all native modules (e.g., react-native-reanimated,
react-native-safe-area-context, react-native-gesture-handler,
react-native-screens, react-native-svg, react-native-webview, expo-secure-store,
react-native-render-html, react-native-url-polyfill) — update their entries in
package.json to exact versions (remove ^ ranges) to match the versions you
expect, then reinstall (yarn/npm) and commit the updated lockfile to ensure
reproducible native builds.
In `@apps/mobile/src/lib/preferences.ts`:
- Around line 3-6: The object defaultUserPreferences currently uses a type
assertion ("as ArticleViewMode") which bypasses compile-time checking; replace
the assertion with a satisfies clause to validate the object shape and the
defaultView literal against the ArticleViewMode union (e.g., export const
defaultUserPreferences = { pollingIntervalMinutes: 15, defaultView: "reader" }
satisfies { pollingIntervalMinutes: number; defaultView: ArticleViewMode } or
satisfies UserPreferences if that type exists) and remove the "as
ArticleViewMode" cast so the compiler ensures the value is valid while keeping
the literal type.
In `@apps/web/.gitignore`:
- Around line 17-18: Remove the redundant explicit entry ".env.local" from the
.gitignore since the existing "*.local" glob already ignores it; keep
".env.production" and ensure the "*.local" pattern remains to cover all local
env files.
In `@apps/web/src/components/feed-reader/item-list.tsx`:
- Line 36: Replace the ad-hoc regex hostname extraction with the URL API: parse
source?.siteUrl inside a try/catch (or guard) to create new URL(source.siteUrl)
and use url.host or url.hostname (to include port if desired) to set sourceHost,
falling back to the existing .replace(...) logic or an empty string when parsing
fails or source?.siteUrl is falsy; update the symbol sourceHost and the usage of
source?.siteUrl so the code safely handles undefined and malformed URLs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71d71d63-e7e5-40fb-a3c4-5d36ef2ed361
⛔ Files ignored due to path filters (6)
bun.lockis excluded by!**/*.lockconvex/_generated/api.d.tsis excluded by!**/_generated/**convex/_generated/api.jsis excluded by!**/_generated/**convex/_generated/dataModel.d.tsis excluded by!**/_generated/**convex/_generated/server.d.tsis excluded by!**/_generated/**convex/_generated/server.jsis excluded by!**/_generated/**
📒 Files selected for processing (74)
apps/mobile/.expo/README.mdapps/mobile/.expo/types/router.d.tsapps/mobile/.gitignoreapps/mobile/app.jsonapps/mobile/app/(auth)/sign-in.tsxapps/mobile/app/(tabs)/_layout.tsxapps/mobile/app/(tabs)/bookmarks.tsxapps/mobile/app/(tabs)/index.tsxapps/mobile/app/_layout.tsxapps/mobile/app/article/[sourceId]/[itemId].tsxapps/mobile/app/feed/[sourceId].tsxapps/mobile/babel.config.jsapps/mobile/convex/README.mdapps/mobile/convex/_generatedapps/mobile/global.cssapps/mobile/global.d.tsapps/mobile/metro.config.jsapps/mobile/nativewind-env.d.tsapps/mobile/package.jsonapps/mobile/postcss.config.mjsapps/mobile/src/components/source-card.tsxapps/mobile/src/components/ui/button.tsxapps/mobile/src/components/ui/card.tsxapps/mobile/src/components/ui/input.tsxapps/mobile/src/components/ui/sheet.tsxapps/mobile/src/components/ui/text.tsxapps/mobile/src/lib/auth.tsapps/mobile/src/lib/convex.tsapps/mobile/src/lib/preferences.tsapps/mobile/src/lib/utils.tsapps/mobile/src/providers/feed-provider.tsxapps/mobile/tsconfig.jsonapps/web/.gitignoreapps/web/convex/README.mdapps/web/convex/_generatedapps/web/package.jsonapps/web/src/components/feed-reader/feed-card.tsxapps/web/src/components/feed-reader/feed-reader-app.tsxapps/web/src/components/feed-reader/item-list.tsxapps/web/src/components/feed-reader/reader-pane.tsxapps/web/src/components/feed-reader/source-form.tsxapps/web/src/components/feed-reader/source-grid.tsxapps/web/src/components/feed-reader/source-list.tsxapps/web/src/components/feed-reader/source-sync-controller.tsxapps/web/src/components/feed-reader/store.tsapps/web/src/components/feed-reader/use-feed-reader.tsapps/web/src/lib/convex.tsapps/web/src/lib/feed-reader-state.tsapps/web/src/lib/feed/content.tsapps/web/src/lib/feed/parser.tsapps/web/src/lib/feed/utils.tsapps/web/src/lib/server/feed.tsapps/web/src/lib/types.tsapps/web/src/routes/_authenticated/bookmarks.tsxconvex/auth.config.tsconvex/bookmarks/mutations.tsconvex/bookmarks/queries.tsconvex/feedSubscriptions/mutations.tsconvex/feedSubscriptions/queries.tsconvex/lib/auth.tsconvex/preferences/mutations.tsconvex/preferences/queries.tsconvex/schema.tsconvex/tsconfig.jsonpackage.jsonpackages/shared/package.jsonpackages/shared/src/feed/cache.tspackages/shared/src/feed/content.tspackages/shared/src/feed/parser.tspackages/shared/src/feed/service.tspackages/shared/src/feed/types.tspackages/shared/src/feed/utils.tspackages/shared/tsconfig.jsonturbo.json
💤 Files with no reviewable changes (1)
- convex/preferences/mutations.ts
- block disallowed feed URLs, private IP ranges, and oversized remote feed documents - fix mobile article actions for missing URLs and prevent duplicate bookmark toggles - add mobile config guards, Expo build properties/Hermes override, and scoped Turbo dev tasks
Owner
Author
|
maybe another time !! |
Resolve the feed reader merge conflicts and keep the mobile branch aligned with the shared Convex and cache changes. Made-with: Cursor
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
1 participant
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.
apps/mobilewith Expo Router, Clerk auth, tabs, feeds, bookmarks, and article readerpackages/shared