Conversation
|
Cursor Agent can help with this pull request. Just |
|
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:
📝 WalkthroughWalkthroughAdds a suspense-based progressive image loader and ErrorBoundary for episode artwork, a new img preloader utility with bounded caching, tooltip pointer/focus/blur refinements (tooltip now opens on click), and tests for tooltip behavior and suspense-driven opacity transitions. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component as EpisodeArtworkPreview
participant Suspense
participant Utility as imgSrc
participant Browser as ImageAPI
User->>Component: hover tooltip / click anonymize toggle
Component->>Component: update UI state (tooltip, anonymize)
Component->>Utility: request imgSrc(imageUrl)
Utility->>Utility: check cache
alt cached
Utility-->>Component: resolve cached Promise
else not cached
Utility->>Browser: create Image, set src
Browser-->>Utility: onload / onerror
Utility->>Utility: store Promise in cache
Utility-->>Component: resolve/reject Promise
end
Component->>Suspense: suspend while Promise pending
Suspense-->>User: show fallback/dimmed preview
Browser-->>Component: image loaded
Suspense-->>User: reveal full image (restore opacity)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/utils/suspense-image.ts (1)
5-18: Allow retries after preload failures by evicting rejected cache entries.If
preloadImagerejects due to a transient issue, the rejected promise stays cached and prevents retries. Consider deleting the cache entry on rejection so later calls can recover.♻️ Proposed change
- const promise = preloadImage(src) + const promise = preloadImage(src).catch(error => { + imgCache.delete(src) + throw error + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/suspense-image.ts` around lines 5 - 18, The imgSrc function currently caches the promise returned by preloadImage so a rejected promise blocks future retries; modify imgSrc to remove the cached entry when preloadImage rejects so subsequent calls can retry: after creating const promise = preloadImage(src) and storing imgCache.set(src, promise), attach a .catch handler on that promise which calls imgCache.delete(src) (and then rethrows the error) so failures are evicted; keep the existing MAX_CACHE_ENTRIES eviction logic intact and reference the existing symbols imgSrc, imgCache, preloadImage, and MAX_CACHE_ENTRIES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/utils/suspense-image.ts`:
- Around line 26-30: The Promise in the image preload helper (the new Promise in
suspense-image.ts that instantiates Image) currently assigns img.src before
registering handlers; move the img.onload and img.onerror assignments to before
setting img.src, then set img.src, and finally check img.complete and if true
resolve immediately (or reject if appropriate) to handle cached images firing
synchronously; ensure the handlers call resolve(src) and reject with the
error/event for proper resolution.
---
Nitpick comments:
In `@app/utils/suspense-image.ts`:
- Around line 5-18: The imgSrc function currently caches the promise returned by
preloadImage so a rejected promise blocks future retries; modify imgSrc to
remove the cached entry when preloadImage rejects so subsequent calls can retry:
after creating const promise = preloadImage(src) and storing imgCache.set(src,
promise), attach a .catch handler on that promise which calls
imgCache.delete(src) (and then rethrows the error) so failures are evicted; keep
the existing MAX_CACHE_ENTRIES eviction logic intact and reference the existing
symbols imgSrc, imgCache, preloadImage, and MAX_CACHE_ENTRIES.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/calls/__tests__/episode-artwork-preview.test.tsxapp/components/calls/episode-artwork-preview.tsxapp/utils/suspense-image.ts
| return new Promise<string>((resolve, reject) => { | ||
| const img = new Image() | ||
| img.src = src | ||
| img.onload = () => resolve(src) | ||
| img.onerror = reject |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "suspense-image.ts" --type fRepository: kentcdodds/kentcdodds.com
Length of output: 96
🏁 Script executed:
cat -n app/utils/suspense-image.tsRepository: kentcdodds/kentcdodds.com
Length of output: 1203
🌐 Web query:
cached images synchronous load event javascript img.src handlers
💡 Result:
When an image is already in cache, setting img.src can make it become “loaded” immediately, so if you attach onload/addEventListener('load', …) after setting src, you can miss the event (it already happened). The fix is: bind handlers first, then set src. If src might already be set (existing <img>), also guard with img.complete. (developer.mozilla.org)
Reliable pattern (works for cached + non-cached)
function loadImage(url) {
return new Promise((resolve, reject) => {
const img = new Image();
const onLoad = () => resolve(img);
const onError = (e) => reject(e);
img.addEventListener("load", onLoad, { once: true });
img.addEventListener("error", onError, { once: true });
img.src = url;
// If it was already complete (e.g., cache), resolve on next tick
if (img.complete) queueMicrotask(onLoad);
});
}HTMLImageElement.complete tells you whether the image is considered fully loaded/available. (developer.mozilla.org)
If you need “decoded and ready to paint”
Use await img.decode() (it resolves when the image is decoded and safe to insert/render). (developer.mozilla.org)
const img = new Image();
img.src = url;
await img.decode();Special case: setting the same src again (WebKit/Safari/Chrome quirks)
Some WebKit behaviors won’t reliably fire load when reassigning the exact same src; a common workaround is to create a new Image() instance (or change the URL, e.g., cache-bust query param). (stackoverflow.com)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/load_event?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/complete?utm_source=openai
- 3: https://developer.mozilla.org/docs/Web/API/HTMLImageElement/decode?utm_source=openai
- 4: https://stackoverflow.com/questions/5024111/javascript-image-onload-doesnt-fire-in-webkit-if-loading-same-image?utm_source=openai
Attach load/error handlers before setting src.
Line 28 sets img.src before registering handlers. For cached images the load event can fire synchronously, leaving the promise unresolved and suspense stuck.
The basic fix is to reorder—attach handlers before setting src. For complete robustness, also check img.complete after setting src, since the image may already be loaded from cache before the handler attaches:
🐛 Proposed fix
return new Promise<string>((resolve, reject) => {
const img = new Image()
- img.src = src
img.onload = () => resolve(src)
img.onerror = reject
+ img.src = src
+ if (img.complete) queueMicrotask(() => resolve(src))
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/utils/suspense-image.ts` around lines 26 - 30, The Promise in the image
preload helper (the new Promise in suspense-image.ts that instantiates Image)
currently assigns img.src before registering handlers; move the img.onload and
img.onerror assignments to before setting img.src, then set img.src, and finally
check img.complete and if true resolve immediately (or reject if appropriate) to
handle cached images firing synchronously; ensure the handlers call resolve(src)
and reject with the error/event for proper resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/calls/episode-artwork-preview.tsx`:
- Around line 42-49: The loading indicator misses title-change transitions
because it only checks the anonymous mismatch; update the transition usage to
capture isPending from React.useTransition and include it in the spin delay
condition so showPending becomes driven by (previewIsAnonymous !== isAnonymous
|| isPending); specifically, in the component capture const [, startTransition,
isPending] (or const [isPending, startTransition] depending on your hook usage)
from React.useTransition and modify the useSpinDelay call to pass the combined
predicate, keeping existing setEnableSuspenseImage and setDebouncedTitle logic
intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/calls/__tests__/episode-artwork-preview.test.tsxapp/components/calls/episode-artwork-preview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/calls/tests/episode-artwork-preview.test.tsx
| const [debouncedTitle, setDebouncedTitle] = React.useState(title) | ||
| const [, startTransition] = React.useTransition() | ||
| const [enableSuspenseImage, setEnableSuspenseImage] = React.useState(false) | ||
| const [previewIsAnonymous, setPreviewIsAnonymous] = React.useState(isAnonymous) | ||
| const showPending = useSpinDelay(previewIsAnonymous !== isAnonymous, { | ||
| delay: 150, | ||
| minDuration: 250, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/components/calls/episode-artwork-preview.tsx | head -100Repository: kentcdodds/kentcdodds.com
Length of output: 3904
🏁 Script executed:
rg -A 10 -B 5 "startTransition" app/components/calls/episode-artwork-preview.tsxRepository: kentcdodds/kentcdodds.com
Length of output: 1648
🏁 Script executed:
rg -A 5 "debouncedTitle|enableSuspenseImage|title" app/components/calls/episode-artwork-preview.tsx | head -60Repository: kentcdodds/kentcdodds.com
Length of output: 1877
🌐 Web query:
React 19 useTransition isPending Suspense behavior suspended content resolves
💡 Result:
In React 19, useTransition’s isPending tracks whether the transition update is still “in flight” (i.e., not yet committed)—including cases where the render for that update suspends.
- If a transition update would cause an already-revealed
<Suspense>boundary to show its fallback, React will keep showing the previously revealed UI and “wait” long enough to avoid that hide/show jump;isPendingstaystrueuntil React can commit the transition result. [1] - When the suspended content needed for that transition becomes ready, React retries rendering and commits the new UI, and then
isPendingflips tofalse. (Conceptually: pending until the transition can finish and commit.) [2] - React does not wait for brand-new nested
<Suspense>boundaries introduced by the transition. Those can show their fallback immediately, and the transition can finish (soisPendingbecomesfalse) even while that newly shown fallback is still loading. [1][2]
Sources: React <Suspense> docs (Transitions + “already revealed content” vs “new boundary”) [1], React useTransition docs (pending transitions + note about not waiting for nested Suspense) [2].
Include transition pending state in the loading indicator.
When the title changes, startTransition wraps setEnableSuspenseImage and setDebouncedTitle, triggering image reload with Suspense. However, showPending only checks the anonymous state mismatch and won't show the loading indicator during title-driven reloads. React's useTransition isPending correctly tracks Suspense suspension state, so capturing and including it would ensure the dimming covers all suspension scenarios.
🔧 Proposed fix
- const [, startTransition] = React.useTransition()
+ const [isTransitionPending, startTransition] = React.useTransition()
const [enableSuspenseImage, setEnableSuspenseImage] = React.useState(false)
const [previewIsAnonymous, setPreviewIsAnonymous] = React.useState(isAnonymous)
- const showPending = useSpinDelay(previewIsAnonymous !== isAnonymous, {
+ const showPending = useSpinDelay(
+ isTransitionPending || previewIsAnonymous !== isAnonymous,
+ {
delay: 150,
minDuration: 250,
- })
+ },
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [debouncedTitle, setDebouncedTitle] = React.useState(title) | |
| const [, startTransition] = React.useTransition() | |
| const [enableSuspenseImage, setEnableSuspenseImage] = React.useState(false) | |
| const [previewIsAnonymous, setPreviewIsAnonymous] = React.useState(isAnonymous) | |
| const showPending = useSpinDelay(previewIsAnonymous !== isAnonymous, { | |
| delay: 150, | |
| minDuration: 250, | |
| }) | |
| const [debouncedTitle, setDebouncedTitle] = React.useState(title) | |
| const [isTransitionPending, startTransition] = React.useTransition() | |
| const [enableSuspenseImage, setEnableSuspenseImage] = React.useState(false) | |
| const [previewIsAnonymous, setPreviewIsAnonymous] = React.useState(isAnonymous) | |
| const showPending = useSpinDelay( | |
| isTransitionPending || previewIsAnonymous !== isAnonymous, | |
| { | |
| delay: 150, | |
| minDuration: 250, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/calls/episode-artwork-preview.tsx` around lines 42 - 49, The
loading indicator misses title-change transitions because it only checks the
anonymous mismatch; update the transition usage to capture isPending from
React.useTransition and include it in the spin delay condition so showPending
becomes driven by (previewIsAnonymous !== isAnonymous || isPending);
specifically, in the component capture const [, startTransition, isPending] (or
const [isPending, startTransition] depending on your hook usage) from
React.useTransition and modify the useSpinDelay call to pass the combined
predicate, keeping existing setEnableSuspenseImage and setDebouncedTitle logic
intact.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/calls/episode-artwork-preview.tsx (1)
215-220: Consider updatinglatestSrcRefduring render instead of in an effect.The
useEffectintroduces a brief gap between render and commit during whichlatestSrcRef.currentmay be stale. While image resolution is always async (so this rarely matters in practice), the standard idiomatic pattern is to write directly to the ref during render:♻️ Suggested refactor
const latestSrcRef = React.useRef(safeSrc) - - React.useEffect(() => { - latestSrcRef.current = safeSrc - }, [safeSrc]) + latestSrcRef.current = safeSrc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/calls/episode-artwork-preview.tsx` around lines 215 - 220, The ref latestSrcRef is updated in a useEffect which leaves a brief window where latestSrcRef.current is stale; instead assign latestSrcRef.current = safeSrc directly during render (in the component body, alongside the initialization of latestSrcRef and fallbackSrc) and remove the React.useEffect block so that latestSrcRef always reflects the current safeSrc immediately; keep the existing useState for fallbackSrc unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/calls/episode-artwork-preview.tsx`:
- Around line 215-220: The ref latestSrcRef is updated in a useEffect which
leaves a brief window where latestSrcRef.current is stale; instead assign
latestSrcRef.current = safeSrc directly during render (in the component body,
alongside the initialization of latestSrcRef and fallbackSrc) and remove the
React.useEffect block so that latestSrcRef always reflects the current safeSrc
immediately; keep the existing useState for fallbackSrc unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/calls/__tests__/episode-artwork-preview.test.tsxapp/components/calls/episode-artwork-preview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/calls/tests/episode-artwork-preview.test.tsx
This comment has been minimized.
This comment has been minimized.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
cursor
bot
force-pushed
the
cursor/anonymous-image-pending-state-7d33
branch
from
77f3320 to
1b3587e
Compare
| imgCache.set(src, promise) | ||
| promise.catch(() => { | ||
| if (imgCache.get(src) === promise) imgCache.delete(src) | ||
| }) |
There was a problem hiding this comment.
Cache clearing causes infinite suspend loop on image failure
Medium Severity
The promise.catch() handler in imgSrc deletes the rejected promise from imgCache before React can re-render the suspended component. When React re-renders PreloadImage, imgSrc(src) encounters a cache miss, creates a new pending promise, and React.use() suspends on it instead of throwing the rejection. The ErrorBoundary never catches the error. If the image consistently fails (e.g. 404), this produces an infinite suspend→reject→new-promise→suspend loop, generating unbounded HTTP requests and leaving the artwork stuck at 60% opacity forever.
Additional Locations (1)
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/anonymous-image-pending-state-7d33
branch
from
62f215a to
10ef86d
Compare
2 participants
Add image loading pending state and change tooltip trigger to hover for anonymous publishing.
Note
Medium Risk
Touches interactive UI behavior and introduces Suspense-based image preloading/caching; main risk is regressions in tooltip accessibility/closing logic or unexpected image loading behavior across browsers/SSR.
Overview
Improves the call submission artwork preview by preloading updated artwork URLs via Suspense, keeping the previous image visible and dimming it until the next image finishes loading (with an SSR-safe, size-bounded preload cache in new
utils/suspense-image.ts).Updates the "Publish anonymously" tooltip to open on hover/focus and close on pointer leave/blur/outside click (instead of toggling on click), and adds new tests covering tooltip hover behavior and the preview’s pending/loaded opacity transition.
Written by Cursor Bugbot for commit 10ef86d. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Tests