Conversation
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughAdds a hydration- and null-safety guard to the blurrable image component: during client hydration the code now verifies the element is an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
🧹 Nitpick comments (1)
app/components/blurrable-image.tsx (1)
32-38: Optional:setVisible(true)inside thecompleteguard is redundant.Since
useIsomorphicLayoutEffect(running asuseLayoutEffect) always fires beforeuseEffecton the client, anycompleteimage is already handled before this point —visibleis alreadytrue. Thereturnon line 37 is still necessary to skip attaching the listener, but thesetVisible(true)call on line 36 is a no-op.♻️ Suggested simplification
if (imageEl.complete) { - setVisible(true) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/blurrable-image.tsx` around lines 32 - 38, The guard in React.useEffect that checks if jsImgElRef.current.complete includes a redundant setVisible(true); remove that setVisible(true) call but keep the early return to avoid attaching the load listener, since useIsomorphicLayoutEffect already sets visible for complete images; update the block in the blurrable-image component (the React.useEffect containing jsImgElRef, imageEl.complete, setVisible) accordingly.
🤖 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/blurrable-image.tsx`:
- Around line 32-38: The guard in React.useEffect that checks if
jsImgElRef.current.complete includes a redundant setVisible(true); remove that
setVisible(true) call but keep the early return to avoid attaching the load
listener, since useIsomorphicLayoutEffect already sets visible for complete
images; update the block in the blurrable-image component (the React.useEffect
containing jsImgElRef, imageEl.complete, setVisible) accordingly.
cursor
bot
force-pushed
the
cursor/blurrableimage-state-initialization-46b1
branch
from
711c554 to
4181841
Compare
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/blurrableimage-state-initialization-46b1
branch
from
4181841 to
bb9bbeb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/blurrable-image.tsx (1)
18-29: Guard is correct;useLayoutEffectwould eliminate the residual paint flash for cached imagesThe
instanceof HTMLImageElementguard at line 21 correctly handles bothnull(element not yet in DOM) and accidental non-imgmatches in a single check, and the simplifiedreturn !el.classList.contains('opacity-0')at line 29 is now provably safe. Behavior is preserved for all three paths:
Scenario getElementByIdresultvisibleinitCorrected by Traditional SSR hydration HTMLImageElementopacity-based — (correct upfront) Streaming SSR / Suspense hydration nullfalseuseEffectCSR (SPA navigation) nullfalseuseEffectOne minor residual concern: for the bottom two rows, the correction happens inside
useEffect, which fires after paint. A cached/complete image will therefore flash the blurred state for one frame beforesetVisible(true)takes effect. A drop-inuseIsomorphicLayoutEffect(switching betweenuseLayoutEffecton the client anduseEffecton the server) at lines 33–51 would eliminate that flash without affecting SSR safety, but it's a minor UX edge case.✨ Optional: isomorphic layout effect to avoid the post-paint flash
Add a small hook (or import it from a shared util if you already have one):
const useIsomorphicLayoutEffect = isServer ? React.useEffect : React.useLayoutEffectThen swap the existing effect:
- React.useEffect(() => { + useIsomorphicLayoutEffect(() => { if (!jsImgElRef.current) return if (jsImgElRef.current.complete) { setVisible(true) return }This keeps SSR safe (layout effects are a no-op on the server) while ensuring visibility is corrected synchronously before the browser paints on the client.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/blurrable-image.tsx` around lines 18 - 29, The current client-side effect runs after paint causing a one-frame blurred flash for cached images; replace the existing useEffect that checks document.getElementById(id) / instanceof HTMLImageElement and uses el.classList.contains('opacity-0') to setVisible(...) with an isomorphic layout effect: add a small hook (e.g. const useIsomorphicLayoutEffect = typeof window === 'undefined' ? React.useEffect : React.useLayoutEffect) and call useIsomorphicLayoutEffect in place of useEffect so the DOM check (getElementById, HTMLImageElement guard, classList.contains) and setVisible run synchronously on the client while remaining SSR-safe.
🤖 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/blurrable-image.tsx`:
- Around line 18-29: The current client-side effect runs after paint causing a
one-frame blurred flash for cached images; replace the existing useEffect that
checks document.getElementById(id) / instanceof HTMLImageElement and uses
el.classList.contains('opacity-0') to setVisible(...) with an isomorphic layout
effect: add a small hook (e.g. const useIsomorphicLayoutEffect = typeof window
=== 'undefined' ? React.useEffect : React.useLayoutEffect) and call
useIsomorphicLayoutEffect in place of useEffect so the DOM check
(getElementById, HTMLImageElement guard, classList.contains) and setVisible run
synchronously on the client while remaining SSR-safe.
kentcdodds
deleted the
cursor/blurrableimage-state-initialization-46b1
branch
2 participants
Refactor
BlurrableImage's visibility state initialization to prevent potential DOM access errors before mount.The initial state for
BlurrableImage'svisiblestate was attempting to access the DOM element viadocument.getElementByIdduring the initial render, which could lead to errors if the element was not yet mounted. This change moves the DOM lookup to an isomorphic layout effect, ensuring the element is available when its properties are accessed, and correctly synchronizes visibility after image load.Fixes KCD-NODE-VB
Note
Low Risk
Small, localized UI behavior change that only adds defensive checks around client-side DOM access during hydration.
Overview
Prevents
BlurrableImagefrom crashing during hydration by guarding the initialdocument.getElementById(id)lookup and defaulting to the blurred/hidden state when the element isn’t yet in the DOM.The initial visibility computation now only checks
opacity-0after confirming the lookup returns anHTMLImageElement, leaving the existing post-mountload/completeeffect behavior intact.Written by Cursor Bugbot for commit bb9bbeb. This will update automatically on new commits. Configure here.
Summary by CodeRabbit