refactor(cards): simplify placeholder text + drive loading state from Realtime#951
Merged
navin-moorthy merged 8 commits intodevfrom Apr 20, 2026
Merged
Conversation
Replaces the four-way status text ("Fetching data...", "Taking
screenshot....", "Cannot fetch image for this bookmark") with a simple
binary: while the image is on its way (optimistic insert, server
pipeline running, or preload crossfade) show "Getting screenshot"; once
the pipeline is done with no image, render no text at all. The loading
gif stays to indicate the placeholder state visually.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request migrates bookmark loading state management from a store-based system ( Changes
Sequence DiagramsequenceDiagram
participant UI as UI Component
participant Mutation as Add Bookmark Mutation
participant Enrichment as Enrichment Subscription
participant Realtime as Supabase Realtime
participant Store as External Store
UI->>Mutation: mutate(bookmarkData)
Mutation->>Mutation: onSuccess (optimistic temp ID → real ID)
Mutation->>Enrichment: openBookmarkEnrichmentSubscription(realID)
Enrichment->>Store: notifyListeners()
Store->>UI: subscribeToBookmarkEnrichmentChanges
UI->>UI: useBookmarkEnrichmentActive(realID) → true
alt Screenshot Path
Mutation->>Mutation: trigger screenshot mutation
Realtime->>Enrichment: screenshot/enrichment complete
Enrichment->>Store: notifyListeners()
Enrichment->>Enrichment: teardownBookmarkEnrichmentSubscription
Store->>UI: subscribeToBookmarkEnrichmentChanges
UI->>UI: useBookmarkEnrichmentActive(realID) → false
else Non-Screenshot Path
Mutation->>Enrichment: teardownBookmarkEnrichmentSubscription(not_applicable)
Enrichment->>Store: notifyListeners()
Store->>UI: subscribeToBookmarkEnrichmentChanges
UI->>UI: useBookmarkEnrichmentActive(realID) → false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
Removes the Zustand loadingBookmarkIds set in favor of the Realtime subscription manager as the single source of truth for "is this bookmark still being enriched?". The store was a parallel signal that had to be manually kept in sync with the subscription lifecycle — now the card reads the same state the manager already tracks. Changes: - Manager exposes subscribeToBookmarkEnrichmentChanges + isBookmarkEnrichmentActive; notifyListeners fires on open, teardown, queue push. - useBookmarkEnrichmentActive(id) hook wraps useSyncExternalStore. - Add-bookmark mutation opens the subscription in onSuccess (right after the temp → real ID swap) instead of waiting for the media-type checks. Closes the ~200-1000ms gap where the card showed no loading state while checkIfUrlAnImage + getMediaType resolved. - IIFE tears down via new "not_applicable" reason on image/audio paths once the media check confirms no screenshot pipeline will run. - PDF path keeps subscription alive through handlePdfThumbnailAndUpload; terminal condition now covers PDF (ogImage set when meta_data.mediaType is application/pdf). Only tears down if both thumbnail retries fail. - Screenshot mutation drops Zustand writes; still tears down on error. - componentStore loses loadingBookmarkIds, addLoadingBookmarkId, removeLoadingBookmarkId; LoadersStoreState interface trimmed to match.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
src/async/mutationHooks/bookmarks/use-add-bookmark-min-data-optimistic-mutation.ts (1)
205-249:⚠️ Potential issue | 🟠 MajorHandle media-probe failures before leaving the subscription active.
If
checkIfUrlAnImage()orgetMediaType()rejects, thisvoidIIFE exits before calling the screenshot mutation or tearing down the Realtime subscription, leaving the card in"Getting screenshot"until the 90s timeout. Fall back to the screenshot path or explicitly teardown in an outercatch.🛠️ Proposed fix
void (async () => { - const isUrlOfMimeType = await checkIfUrlAnImage(url); - if (isUrlOfMimeType) { - void teardownBookmarkEnrichmentSubscription(data.id, "not_applicable"); - return; - } - - const mediaType = await getMediaType(url); - // Audio URLs already have ogImage fallback set in add-bookmark-min-data - if (mediaType?.includes("audio")) { - void teardownBookmarkEnrichmentSubscription(data.id, "not_applicable"); - return; - } - - if (mediaType === PDF_MIME_TYPE || URL_PDF_CHECK_PATTERN.test(url)) { - try { - successToast("Generating thumbnail"); - await handlePdfThumbnailAndUpload({ - fileId: data.id, - fileUrl: data.url, - sessionUserId: session?.user?.id, - }); - } catch { - try { - errorToast("retry thumbnail generation"); - await handlePdfThumbnailAndUpload({ - fileId: data.id, - fileUrl: data.url, - sessionUserId: session?.user?.id, - }); - } catch (retryError) { - console.error("PDF thumbnail upload failed after retry:", retryError); - errorToast("thumbnail generation failed"); - void teardownBookmarkEnrichmentSubscription(data.id, "screenshot_failed"); - } - } finally { - void queryClient.invalidateQueries({ - queryKey: [BOOKMARKS_KEY, session?.user?.id], - }); - } - return; - } - - addBookmarkScreenshotMutation.mutate({ id: data.id, url: data.url }); + try { + const isUrlOfMimeType = await checkIfUrlAnImage(url); + if (isUrlOfMimeType) { + void teardownBookmarkEnrichmentSubscription(data.id, "not_applicable"); + return; + } + + const mediaType = await getMediaType(url); + // Audio URLs already have ogImage fallback set in add-bookmark-min-data + if (mediaType?.includes("audio")) { + void teardownBookmarkEnrichmentSubscription(data.id, "not_applicable"); + return; + } + + if (mediaType === PDF_MIME_TYPE || URL_PDF_CHECK_PATTERN.test(url)) { + try { + successToast("Generating thumbnail"); + await handlePdfThumbnailAndUpload({ + fileId: data.id, + fileUrl: data.url, + sessionUserId: session?.user?.id, + }); + } catch { + try { + errorToast("retry thumbnail generation"); + await handlePdfThumbnailAndUpload({ + fileId: data.id, + fileUrl: data.url, + sessionUserId: session?.user?.id, + }); + } catch (retryError) { + console.error("PDF thumbnail upload failed after retry:", retryError); + errorToast("thumbnail generation failed"); + void teardownBookmarkEnrichmentSubscription(data.id, "screenshot_failed"); + } + } finally { + void queryClient.invalidateQueries({ + queryKey: [BOOKMARKS_KEY, session?.user?.id], + }); + } + return; + } + } catch (mediaProbeError) { + console.error("Bookmark media probing failed; falling back to screenshot:", mediaProbeError); + } + + addBookmarkScreenshotMutation.mutate({ id: data.id, url: data.url }); })();As per coding guidelines, "Always include proper error handling and logging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async/mutationHooks/bookmarks/use-add-bookmark-min-data-optimistic-mutation.ts` around lines 205 - 249, The IIFE can throw if checkIfUrlAnImage() or getMediaType() rejects, leaving the realtime subscription active; wrap the media-probing logic in a try/catch (or add an outer try/catch around the whole async IIFE) so any rejection either calls teardownBookmarkEnrichmentSubscription(data.id, "screenshot_failed") (or a sensible fallback like "not_applicable") and logs the error, or falls back to the screenshot path by calling addBookmarkScreenshotMutation.mutate({ id: data.id, url: data.url }); ensure you still run queryClient.invalidateQueries([BOOKMARKS_KEY, session?.user?.id]) where appropriate and surface errors (e.g., console.error) when handlePdfThumbnailAndUpload or probes fail so failures are not silently swallowed.src/lib/supabase/realtime/bookmark-enrichment-subscription.ts (1)
259-271:⚠️ Potential issue | 🟠 MajorNotify listeners when dequeuing a waiting bookmark.
isBookmarkEnrichmentActive()returnstruefor queued entries, but this teardown path removes a queued bookmark without notifying subscribers. In a burst overMAX_CONCURRENT_CHANNELS, an image/audio URL can be dequeued via"not_applicable"and the card will keep showing active loading until another unrelated subscription event fires.🛠️ Proposed fix
if (queuedIndex !== -1) { waiting.splice(queuedIndex, 1); logEvent("dequeued before subscribe", bookmarkId, { reason }); + notifyListeners(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/supabase/realtime/bookmark-enrichment-subscription.ts` around lines 259 - 271, When removing a queued entry in teardown(bookmarkId, reason) you must also notify subscribers so UI consumers relying on isBookmarkEnrichmentActive() stop showing loading; after waiting.splice(...) and logEvent("dequeued before subscribe", ...) invoke the same notification/event path used for active teardowns in this module (the function that broadcasts subscription state changes to listeners) with bookmarkId and a non-active status/reason (e.g., "not_applicable"), so listeners receive an update that the bookmark was dequeued and is no longer active/queued.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/async/mutationHooks/bookmarks/use-add-bookmark-min-data-optimistic-mutation.ts`:
- Around line 205-249: The IIFE can throw if checkIfUrlAnImage() or
getMediaType() rejects, leaving the realtime subscription active; wrap the
media-probing logic in a try/catch (or add an outer try/catch around the whole
async IIFE) so any rejection either calls
teardownBookmarkEnrichmentSubscription(data.id, "screenshot_failed") (or a
sensible fallback like "not_applicable") and logs the error, or falls back to
the screenshot path by calling addBookmarkScreenshotMutation.mutate({ id:
data.id, url: data.url }); ensure you still run
queryClient.invalidateQueries([BOOKMARKS_KEY, session?.user?.id]) where
appropriate and surface errors (e.g., console.error) when
handlePdfThumbnailAndUpload or probes fail so failures are not silently
swallowed.
In `@src/lib/supabase/realtime/bookmark-enrichment-subscription.ts`:
- Around line 259-271: When removing a queued entry in teardown(bookmarkId,
reason) you must also notify subscribers so UI consumers relying on
isBookmarkEnrichmentActive() stop showing loading; after waiting.splice(...) and
logEvent("dequeued before subscribe", ...) invoke the same notification/event
path used for active teardowns in this module (the function that broadcasts
subscription state changes to listeners) with bookmarkId and a non-active
status/reason (e.g., "not_applicable"), so listeners receive an update that the
bookmark was dequeued and is no longer active/queued.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32e81fb2-d3f6-45af-b4ec-e7db3ccdc601
📒 Files selected for processing (9)
src/async/mutationHooks/bookmarks/use-add-bookmark-min-data-optimistic-mutation.tssrc/async/mutationHooks/bookmarks/use-add-bookmark-screenshot-mutation.tssrc/hooks/use-bookmark-enrichment-active.tssrc/lib/supabase/realtime/bookmark-enrichment-subscription.tssrc/lib/supabase/realtime/bookmark-realtime-payload.tssrc/pageComponents/dashboard/cardSection/animatedBookmarkImage.tsxsrc/pageComponents/dashboard/cardSection/imageCard.tsxsrc/store/componentStore.tssrc/types/componentStoreTypes.ts
💤 Files with no reviewable changes (2)
- src/types/componentStoreTypes.ts
- src/store/componentStore.ts
2 tasks
rogerantony-dev
force-pushed
the
feat/simplify-card-loading-states
branch
from
0193253 to
e4d16ff
Compare
`useSyncExternalStore` consumers (card loading indicator) went stale when a bookmark id was dropped from `waiting` without passing through `teardown()`'s notify call: - Teardown of a queued-but-never-promoted id (rapid burst >5 adds, then non-enriching media type resolves) spliced the entry silently. Card stuck on "Getting screenshot" — queued ids don't hit the 90s timeout either, that only fires for active records. - `teardownAllBookmarkEnrichmentSubscriptions` cleared `waiting` in place on sign-out without notifying.
The onSettled IIFE opened the subscription in onSuccess then did the media-type probe + PDF path + screenshot mutation. Any unexpected throw (network error in checkIfUrlAnImage/getMediaType, toast lib failure, React Query internal throw) was swallowed by `void`, leaving the subscription dangling until the 90s timeout — card stuck on "Getting screenshot" with no actual pipeline running. Wrap the body in try/catch with a "screenshot_failed" teardown so the loading indicator always clears on unexpected failures.
Placing \`statusText &&\` outside \`AnimatePresence\` unmounted the entire presence tree when the text went null, so the \`exit\` transition on \`<motion.p>\` never ran — the text snapped out instantly while the surrounding placeholder faded over 0.15s, causing visible jank at the end of the loading state. Move the conditional inside \`AnimatePresence\` so the exit animation is driven by child removal, not parent unmount.
\`isRowTerminal\` branches on \`meta_data.mediaType\` to pick between the PDF and regular-URL terminal conditions, but the field was not listed in \`MetaDataRealtimeSchema\` — it passed through the catchall as \`unknown\`, forcing a \`typeof === "string"\` narrow at the use site. The field is as load-bearing as \`screenshot\` and \`ocr_status\`; declare it in the schema so the contract is explicit.
Contributor
Author
|
updated loading placeholder: Screen.Recording.2026-04-20.at.4.32.27.PM.movplaceholder when image is not there: 
|
Comment on lines
-42
to
-83
| onSettled: (response) => { | ||
| if (response?.[0]) { | ||
| const [updated] = response; | ||
|
|
||
| // Inject the screenshot response into the paginated cache | ||
| // synchronously, in the SAME callback as removeLoadingBookmarkId. | ||
| // React batches the two updates into a single render — the card | ||
| // never sees (img=null && isLoading=false) and so never falls | ||
| // through the statusText ladder to TERMINAL. | ||
| // | ||
| // The prior approach (`await invalidateQueries` then | ||
| // removeLoadingBookmarkId) looked correct on paper, but in | ||
| // practice the cache notify and the Zustand store update | ||
| // committed in two separate React renders — the first render | ||
| // landed (img=null, isLoading=false) and produced the exit-side | ||
| // "Cannot fetch image..." flash. Synchronous injection | ||
| // eliminates that race. The screenshot response is the source | ||
| // of truth for ogImage at this point: the route handler just | ||
| // backfilled it from the captured screenshot (when the scraped | ||
| // ogImage was missing or malformed) and returned the updated | ||
| // row. A background refetch (below) still picks up later | ||
| // addRemainingBookmarkData() enrichments (img_caption, ocr, | ||
| // blurhash, additional cover images). | ||
| queryClient.setQueryData<PaginatedBookmarks>( | ||
| [BOOKMARKS_KEY, session?.user?.id, CATEGORY_ID, sortBy], | ||
| (old) => { | ||
| if (!old) { | ||
| return old; | ||
| } | ||
| return { | ||
| ...old, | ||
| pages: old.pages.map((page) => | ||
| page.map((bookmark) => | ||
| bookmark.id === updated.id ? { ...bookmark, ...updated } : bookmark, | ||
| ), | ||
| ), | ||
| } as PaginatedBookmarks; | ||
| }, | ||
| ); | ||
|
|
||
| removeLoadingBookmarkId(updated.id); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The synchronous setQueryData + removeLoadingBookmarkId block you added in 047d1b7 existed to paper over the race between the cache notify and the Zustand loading flag — landing them in two separate renders meant the card briefly saw (img=null, isLoading=false) and flashed the Cannot fetch image... terminal of the 4-state statusText ladder.
Both legs of that race are gone in this PR:
- The statusText ladder collapsed to 2 states.
animatedBookmarkImage.tsxnow rendersGetting screenshotwhile loading andnullotherwise — there is noCannot fetch imageterminal branch to flash through, so the mid-render gap is visually inert even if it happened. - Loading state is no longer driven by Zustand. The card reads
useBookmarkEnrichmentActive(id)off the subscription manager (useSyncExternalStore). The subscription stays active until the Realtime terminal condition fires (meta_data.screenshot+ogImageboth set, or PDF thumbnail write for PDFs). The screenshot mutation returning is not what flipsisLoadingtofalseanymore — the Realtime UPDATE is, and it carries the image with it. - The cache splice already happens. When the screenshot route's first DB write lands, the Realtime UPDATE fires and
applyBookmarkUpdatesplices the row across every matching paginated / search / alt-category cache. ThesetQueryDatainonSettledwould re-do the same merge a few ms later.
So the synchronous injection became redundant once (1) the ladder only has loading / done, (2) loading comes from the subscription, not the mutation's return, and (3) the Realtime splice is the authoritative client-side cache write. The invalidateQueries is kept purely as a safety net for the downstream addRemainingBookmarkData enrichments (img_caption, OCR, blurhash, extra covers) that aren't on the Realtime publication.
Contributor
Author
There was a problem hiding this comment.
ji since we are removing zustand state for loading completely, this fix is no longer needed
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
Stacks on #950. Two changes:
Collapse bookmark-card placeholder text to two states. Replaces the four-way status ("Fetching data...", "Taking screenshot....", "Cannot fetch image for this bookmark") with a binary: while the image is on its way show "Getting screenshot"; once the pipeline is done with no image, render no text at all. Loader gif stays as the visual indicator.
Drive loading state from the Realtime subscription manager instead of Zustand. The
loadingBookmarkIdsset inuseLoadersStorewas a parallel signal that had to be manually kept in sync with the enrichment lifecycle. The subscription manager from feat(realtime): live screenshot + enrichment updates for in-session adds #950 already tracks "is this bookmark being enriched?" — this PR plumbs it out as the single source of truth and deletes the Zustand set.Why
Before #950, the only way to ask "is this bookmark loading?" was the manually-maintained Zustand set. After #950 we also had the subscription manager's internal
activemap tracking the same concept. Two sources of truth for one signal is a maintenance hazard. This PR collapses them into one.What changed
Manager (
bookmark-enrichment-subscription.ts)subscribeToBookmarkEnrichmentChanges(listener) → unsubscribeisBookmarkEnrichmentActive(id)—truewhile active or queuednotifyListeners()fires on open, teardown, queue push"not_applicable"for media paths that don't feed the pipelineNew hook (
src/hooks/use-bookmark-enrichment-active.ts)useSyncExternalStorewith manager's subscribe + snapshotuseLoadersStore((s) => s.loadingBookmarkIds.has(id))Terminal condition is now media-aware (
bookmark-realtime-payload.ts)meta_data.mediaType === "application/pdf"): terminal =ogImagepopulated (PDF flow writes thumbnail URL toogImage, nevermeta_data.screenshot)meta_data.screenshot+ogImagesetSubscription lifecycle moved earlier (
use-add-bookmark-min-data-optimistic-mutation.ts)onSuccessright after the temp → real ID swap (was: inside the IIFE screenshot branch)checkIfUrlAnImage+getMediaTypeprobed content type"not_applicable"on image/audio early-returnshandlePdfThumbnailAndUpload; the terminal condition catches the DB write fromuploadFileRemainingDataScreenshot mutation (
use-add-bookmark-screenshot-mutation.ts)Store cleanup
componentStore.ts— droppedloadingBookmarkIds,addLoadingBookmarkId,removeLoadingBookmarkId(−14 lines)componentStoreTypes.ts— matching interface trimCard placeholder (
animatedBookmarkImage.tsx)"Getting screenshot"while loading,nullwhen done<motion.p>skipped entirely when no text — no empty<p>on screenOut of scope
Testing
https://picsum.photos/300) — subscription briefly opens then tears down with"not_applicable"; card shows image from min-data response without flicker"screenshot_failed"; card drops to no-text statePost-Deploy Monitoring & Validation
category: realtime-bookmark) — watch for a rise in{ reason: "not_applicable" }(expected — new reason) and the overall ratioterminal : timeoutstaying >80:20v2-bookmark-add-url-screenshot— no change in success rate expected; the client-side change doesn't alter server behaviornot_applicableteardown events not appearing for image URLs → IIFE teardown branch is brokenSummary by CodeRabbit
Release Notes
Bug Fixes
New Features
Refactor