-
Notifications
You must be signed in to change notification settings - Fork 116
fix: page icon auth if needed #192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
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
Reviewer's GuideImplements authenticated loading and proper cleanup of page icon images by using getImageUrl/revokeBlobUrl in PageIcon and documenting blob URL cleanup to avoid memory leaks. Sequence diagram for authenticated page icon image loading and cleanupsequenceDiagram
participant PageIcon
participant getImageUrl
participant revokeBlobUrl
participant BrowserMemory
PageIcon->>PageIcon: mount or icon changes
PageIcon->>getImageUrl: getImageUrl(iconUrl)
getImageUrl-->>PageIcon: blobUrl
PageIcon->>PageIcon: setImgSrc(blobUrl)
PageIcon->>BrowserMemory: render img src=blobUrl
PageIcon->>PageIcon: icon changes or unmount
PageIcon->>revokeBlobUrl: revokeBlobUrl(blobUrl)
revokeBlobUrl->>BrowserMemory: URL.revokeObjectURL(blobUrl)
BrowserMemory-->>BrowserMemory: release blob data
Updated class diagram for PageIcon and authenticated image utilitiesclassDiagram
class PageIcon {
+view View
+className string
+iconSize number
-iconContent string
-imgSrc string
+PageIcon(props) ReactElement
}
class authenticated_image {
+getImageUrl(url string) Promise~string~
+revokeBlobUrl(url string) void
}
PageIcon ..> authenticated_image : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The async
getImageUrlcall in theuseEffectcan race with prop changes/unmounts, so consider tracking anisMounted/requestIdflag to avoid callingsetImgSrcorrevokeBlobUrlfor a staleview.iconafter the component has updated or unmounted. - The
useEffectcurrently depends onview.icon; ifviewis frequently re-created, this will trigger more fetches than necessary—consider narrowing the dependency toview.icon?.value(and type) to avoid redundant calls togetImageUrl.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The async `getImageUrl` call in the `useEffect` can race with prop changes/unmounts, so consider tracking an `isMounted`/`requestId` flag to avoid calling `setImgSrc` or `revokeBlobUrl` for a stale `view.icon` after the component has updated or unmounted.
- The `useEffect` currently depends on `view.icon`; if `view` is frequently re-created, this will trigger more fetches than necessary—consider narrowing the dependency to `view.icon?.value` (and type) to avoid redundant calls to `getImageUrl`.
## Individual Comments
### Comment 1
<location> `src/components/_shared/view-icon/PageIcon.tsx:38-42` </location>
<code_context>
}, [view]);
- const img = useMemo(() => {
+ useEffect(() => {
+ let currentBlobUrl: string | undefined;
+
if (view.icon && view.icon.ty === ViewIconType.URL && view.icon.value) {
+ void getImageUrl(view.icon.value).then((url) => {
+ currentBlobUrl = url;
+ setImgSrc(url);
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against stale async responses to avoid showing/referencing the wrong image and leaking blob URLs.
Because `getImageUrl` is async and `view.icon` can change, an earlier request can resolve after a later one. That older `then` will still call `setImgSrc` with a stale URL, and its blob URL may never be revoked because the cleanup for the newer effect has already run.
To avoid this, track whether the effect is still current and bail out if it isn’t, revoking any blob URL created by an outdated request. For example, use an `isCurrent` flag in the effect, set it to `false` in the cleanup, and check it before calling `setImgSrc` (revoking the URL immediately when `!isCurrent`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
josue693
pushed a commit
to josue693/AppFlowy-Web
that referenced
this pull request
Dec 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Ensure page icons load via authenticated image URLs and clean up associated blob URLs to prevent memory leaks.
Enhancements: