-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Unify dimensions cache between lightbox and feed #6047
Merged
Conversation
This file contains 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
arcalinea
temporarily deployed
to
aspect-1 - social-app PR #6047
November 1, 2024 02:50
— with
Render
Destroyed
gaearon
commented
Nov 1, 2024
import {Image} from 'react-native' | ||
|
||
import type {Dimensions} from '#/lib/media/types' | ||
|
||
const sizes: Map<string, Dimensions> = new Map() | ||
type CacheStorageItem<T> = {key: string; value: T} | ||
const createCache = <T>(cacheSize: number) => ({ |
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.
this is copypasta from deleted file
|
arcalinea
temporarily deployed
to
aspect-1 - social-app PR #6047
November 1, 2024 03:00
— with
Render
Destroyed
It's not very good that you can't distingiush when we haven't loaded vs when we're certain. This shifts the burden of dealing with missing values to the caller.
Code is copy paste from useImageDimensions.ts
arcalinea
temporarily deployed
to
aspect-1 - social-app PR #6047
November 1, 2024 22:53
— with
Render
Destroyed
This was referenced Nov 1, 2024
Closed
mozzius
approved these changes
Nov 4, 2024
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.
tested, looks good
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.
We have two pieces of code doing essentially the same thing but with separate caches. (The "thing" is measuring image sizes and caching them.) I'm unifying them into a single cache:
aspectRatio
)knownDimensions
which should catch cases where we know them beforehandsrc
while rendering are now handledundefined
when the dimensions aren't known yet (instead of returning1
for ratio)In the final commit, I'm changing the lightbox code to use the dimensions plumbed down from the record when available. This means that for the vast majority of images, we won't trigger the "download and measure" codepath anymore.
Test Plan
Add
setTimeout
inside the hook before resolving and addknownDimensions = undefined
to force loading.Verify the feed jumps a bit as image dimensions get resolved (this is the worst case, e.g. when the records are done by clients that don't fill in
aspectRatio
) but that it works. Verify you can open the lightbox still.Add some logs to cache hits, misses, and active request tracking. Verify they work as expected.
Check solo lightbox, gallery lightbox, and profile avi lightbox.
Verify hardcoding
width: 0
orheight: 0
in the resize callback doesn't crash the lightbox.Remove the
setTimeout
andknownDimensions = undefined
. Verify we mostly useknownDimensions
except for records with missingaspectRatio
like https://bsky.app/profile/unseenjapan.com/post/3l5mkbqrqq72y. Verify the same for lightbox.