fix: fixed logo refetch on re-render #2978
Conversation
|
WalkthroughThe pull request introduces a modification to the Changes
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/kyb-app/src/components/layouts/AppShell/Logo.tsx (1)
28-28: Consider adding error handling for the image load.The implementation correctly uses
onLoadRef.currentto avoid stale closures. However, you might want to consider handling potential image load failures explicitly.- Promise.race([prefetchImage(logoSrc), fallback(3000)]).then(onLoadRef.current); + Promise.race([prefetchImage(logoSrc), fallback(3000)]) + .then(onLoadRef.current) + .catch(() => { + // Handle or log image load failures + onLoadRef.current?.(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/kyb-app/src/components/layouts/AppShell/Logo.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: lint
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
apps/kyb-app/src/components/layouts/AppShell/Logo.tsx (3)
1-1: LGTM! Good use ofuseRefValuehook.Using
useRefValueis a good practice here to prevent stale closure issues with theonLoadcallback.Also applies to: 20-21
29-29: LGTM! Correct dependency array update.The dependency array correctly includes
onLoadRefinstead ofonLoad, which is the right approach when using the ref value pattern.
Line range hint
1-32: Verify similar patterns in other components.Let's check if there are other components that might benefit from the same fix.
✅ Verification successful
No changes needed in other components
The timeout pattern in the Logo component is specific to preventing UI locks in stepper breadcrumbs. Other image loading implementations serve different purposes where a timeout would be inappropriate:
- Preload service requires actual load/error states for proper functionality
- SVG to PNG converter needs successful image load to perform canvas operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar image loading patterns that might need the same fix ast-grep --pattern 'new Image()' # Look for other Logo component usages rg -l "Logo.*onLoad"Length of output: 381
Script:
#!/bin/bash # Check implementation of other files using new Image() echo "=== preload-service/utils.ts ===" rg -A 10 -B 5 "new Image\(\)" "sdks/web-ui-sdk/src/lib/services/preload-service/utils.ts" echo -e "\n=== svg-to-png.ts ===" rg -A 10 -B 5 "new Image\(\)" "apps/backoffice-v2/src/common/utils/svg-to-png/svg-to-png.ts"Length of output: 1136
| Promise.race([prefetchImage(logoSrc), fallback(3000)]).then(onLoad); | ||
| }, [logoSrc, onLoad]); | ||
| Promise.race([prefetchImage(logoSrc), fallback(3000)]).then(onLoadRef.current); | ||
| }, [logoSrc, onLoadRef]); |
There was a problem hiding this comment.
Should this line be:
| }, [logoSrc, onLoadRef]); | |
| }, [logoSrc, onLoadRef.current]); |
There was a problem hiding this comment.
Nope, it will trigger useEffect each time prop updates, that was the case of bug. So only using link to callback and not callback itself as dependency.
| Promise.race([prefetchImage(logoSrc), fallback(3000)]).then(onLoadRef.current); | ||
| }, [logoSrc, onLoadRef]); |
There was a problem hiding this comment.
@tomer-shvadron Wouldn't that bring back the bug ? I think best way to handle would be something like
| Promise.race([prefetchImage(logoSrc), fallback(3000)]).then(onLoadRef.current); | |
| }, [logoSrc, onLoadRef]); | |
| Promise.race([prefetchImage(logoSrc), fallback(3000)]).then((value) => { if (onLoadRef.current) onLoadRef.current(value)) }); | |
| }, [logoSrc, onLoadRef]); |
(cleanup is needed, just an idea suggestion)
There was a problem hiding this comment.
Nope. It solves bug. Ref object is same each rerender whenever current is being updated. What you suggesting is pretty much safe check but its not needed in case withthen since it wont do anything if it will recieve undefined
There was a problem hiding this comment.
I was referring to proposed above changes in the first half :)
This isn't just a safe-check - onLoadRef will be updated with latest onLoad callback, and this will make sure that we always call most-recent version of the callback ( and thats something that I would expect from the callbacks ).
Summary by CodeRabbit