fix(use-image): load images after props change #4523
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request addresses an issue with the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
🦋 Changeset detectedLatest commit: c72c1b2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/hooks/use-image/src/index.ts (2)
99-117: Review useCallback dependenciesThe
onLoadandonErrorcallbacks are included in the dependencies array but aren't used within theloadfunction. This could cause unnecessary re-renders.- }, [src, crossOrigin, srcSet, sizes, onLoad, onError, loading]); + }, [src, crossOrigin, srcSet, sizes, loading]);
112-114: Add naturalHeight check for complete image validationThe complete image validation should check both naturalWidth and naturalHeight to ensure the image is fully loaded.
- if (img.complete && img.naturalWidth) { + if (img.complete && img.naturalWidth && img.naturalHeight) { return "loaded"; }.changeset/light-peaches-reflect.md (1)
1-5: Enhance changeset descriptionConsider adding more details about the specific changes and their impact:
--- "@nextui-org/use-image": patch --- -fix loading image after props changes (#4518) +fix: handle image loading when src changes from undefined/null (#4518) + +- Added new load function to handle dynamic src changes +- Improved image cleanup to prevent memory leaks +- Added test coverage for dynamic src updates
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/light-peaches-reflect.md(1 hunks)packages/hooks/use-image/__tests__/use-image.test.tsx(1 hunks)packages/hooks/use-image/src/index.ts(3 hunks)
| it("can handle changing image", async () => { | ||
| const {result, rerender} = renderHook(() => useImage({src: undefined})); | ||
|
|
||
| expect(result.current).toEqual("pending"); | ||
|
|
||
| setTimeout(() => { | ||
| rerender({src: "/test.png"}); | ||
| }, 3000); | ||
|
|
||
| mockImage.simulate("loaded"); | ||
| await waitFor(() => expect(result.current).toBe("loaded")); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability by controlling async operations
The current test implementation might be flaky due to race conditions between the setTimeout and image load simulation. Consider using Jest's timer mocks to control the timing precisely.
Here's a more reliable implementation:
it("can handle changing image", async () => {
const {result, rerender} = renderHook(() => useImage({src: undefined}));
expect(result.current).toEqual("pending");
- setTimeout(() => {
- rerender({src: "/test.png"});
- }, 3000);
+ // Use Jest's fake timers
+ jest.useFakeTimers();
+
+ // Schedule the src change
+ setTimeout(() => rerender({src: "/test.png"}), 3000);
+
+ // Fast-forward timers
+ jest.runAllTimers();
mockImage.simulate("loaded");
await waitFor(() => expect(result.current).toBe("loaded"));
+
+ // Cleanup
+ jest.useRealTimers();
});Committable suggestion skipped: line range outside the PR's diff.
| const img = new Image(); | ||
|
|
||
| img.src = src; | ||
| if (crossOrigin) img.crossOrigin = crossOrigin; | ||
| if (srcSet) img.srcset = srcSet; | ||
| if (sizes) img.sizes = sizes; | ||
| if (loading) img.loading = loading; | ||
|
|
||
| imageRef.current = img; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory leak in image creation
The new Image instance should be cleaned up when the src changes to prevent memory leaks, especially in scenarios with rapid prop changes.
Consider adding cleanup logic:
const load = useCallback((): Status => {
if (!src) return "pending";
if (ignoreFallback) return "loaded";
+ // Cleanup previous image if exists
+ if (imageRef.current) {
+ imageRef.current.onload = null;
+ imageRef.current.onerror = null;
+ }
+
const img = new Image();
img.src = src;
if (crossOrigin) img.crossOrigin = crossOrigin;
if (srcSet) img.srcset = srcSet;
if (sizes) img.sizes = sizes;
if (loading) img.loading = loading;
imageRef.current = img;Committable suggestion skipped: line range outside the PR's diff.
Closes #4518
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
no source initially -> set source after seconds
pr4523-no-src.webm
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Bug Fixes
@nextui-org/use-imagepackageTests
The changes enhance the reliability of image loading functionality, addressing potential issues with prop updates and providing more robust image handling.