Skip to content
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

[C-3958] Add StaticImage component and update track artwork rendering on web and mobile-web track pages for ssr #7847

Conversation

Kyle-Shanks
Copy link
Contributor

Description

Added very minimal StaticImage component to handle rendering an img tag based on a provided src to kick off the img fetch sooner in the start up, and then split the image logic on web and mobile-web track pages to conditionally render the static image based on if SSR is enabled

Drops LCP time drop from 26 - 29 sec to 6 - 8 sec which is pretty neat

How Has This Been Tested?

Lots of manual testing on kj.audius.co

@Kyle-Shanks Kyle-Shanks requested review from dylanjeffers, sliptype and a team March 14, 2024 22:42
@Kyle-Shanks Kyle-Shanks changed the title Add StaticImage component and update track artwork rendering on web and mobile-web track pages for ssr [C-3958] Add StaticImage component and update track artwork rendering on web and mobile-web track pages for ssr Mar 14, 2024
Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like where this is headed, especially the big win we get with LCP. I'd like us to think a bit more about how we build in ssr capability to the existing components we have, so we dont have too much branching logic everywhere. Ideally we have a "TrackImage" component that takes a trackId, gets it from redux, and then can compute the trackCid, generate the trackUrl, and on server side it sets that as src right away, and on client side it also updates the store's track_cid map. lmk if that flow makes sense. For short term if we want to go with this just to get the ssr improvement im okay with it

packages/common/src/store/cache/reducer.ts Outdated Show resolved Hide resolved
@@ -318,6 +325,9 @@ const TrackHeader = ({
goToRepostsPage(trackId)
}, [goToRepostsPage, trackId])

const InnerImageElement = isSsrEnabled ? StaticImage : DynamicImage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we will probably need this in a lot of instances, maybe we update dynamic image to handle a "src" prop and do something static with it?

@Kyle-Shanks Kyle-Shanks force-pushed the kj-Add-static-image-component-and-update-track-page-image-rendering-for-srr branch from 9f60024 to 38b6a29 Compare March 18, 2024 18:23
@audius-infra
Copy link
Collaborator

@Kyle-Shanks Kyle-Shanks force-pushed the kj-Add-static-image-component-and-update-track-page-image-rendering-for-srr branch 3 times, most recently from 3a2fc96 to 6ab9257 Compare March 18, 2024 19:41
@audius-infra
Copy link
Collaborator

Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking sweet, please feel free to merge as is and follow up with my comments where relevant.

// Set height and width of wrapper container to 100%
fullWidth?: boolean
// Classes to apply to the wrapper
wrapperClassName?: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total nit, but this could just be "className" the benefit of that is then when you render <StaticImage /> you can apply a css prop instead of css-module. that gets converted to "className" ultimately, if that makes sense. i get this is mimicking DynamicImage, but so ya know

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the classname for the wrapping div though. any css prop or class name added to the component would be applied to the img properly with this setup

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh gotcha, nice

} = props

let url = ''
if (cid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicee

packages/web/src/components/static-image/StaticImage.tsx Outdated Show resolved Hide resolved

const auth = new AppAuth('', '')

const storageNodeSelector = new StorageNodeSelector({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a world where we can pull StorageNodeSelector from the existing sdk? iirc it's in the ssr context/app context right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem was that client side, initializing sdk is async: https://github.com/AudiusProject/audius-protocol/blob/main/packages/mobile/src/services/sdk/audius-sdk.ts#L41 causing us not to have the image source on first render, causing hydration to fail.

I wonder if we could make the initialization synchronous, right now it's

await discoveryNodeSelectorService.getInstance(),

and

await getStorageNodeSelector(),

that force it to be async.

Copy link
Contributor Author

@Kyle-Shanks Kyle-Shanks Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good thought, yea tried to do that before, but it causes issues bc of the async init of the sdk on the client

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

word makes sense, maybe we can init the storageNodeSelector synchronous and then pass that into sdk so we have one instance? but not a big deal rn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the discoveryNodeSelector has to wait for remote config to get the blocklist, so that would block us from making this synchronous i think?

Copy link
Contributor

@sliptype sliptype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So cool!

I agree it is kinda weird to have to initialize a discoveryNodeSelector and storageNodeSelector in the image component. But it does seem like the best option right now unless we could make sdk init synchronous on client


const auth = new AppAuth('', '')

const storageNodeSelector = new StorageNodeSelector({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem was that client side, initializing sdk is async: https://github.com/AudiusProject/audius-protocol/blob/main/packages/mobile/src/services/sdk/audius-sdk.ts#L41 causing us not to have the image source on first render, causing hydration to fail.

I wonder if we could make the initialization synchronous, right now it's

await discoveryNodeSelectorService.getInstance(),

and

await getStorageNodeSelector(),

that force it to be async.

@Kyle-Shanks Kyle-Shanks force-pushed the kj-Add-static-image-component-and-update-track-page-image-rendering-for-srr branch from 6ab9257 to f40186c Compare March 19, 2024 16:34
@audius-infra
Copy link
Collaborator

@Kyle-Shanks Kyle-Shanks merged commit 062814a into main Mar 19, 2024
13 of 15 checks passed
@Kyle-Shanks Kyle-Shanks deleted the kj-Add-static-image-component-and-update-track-page-image-rendering-for-srr branch March 19, 2024 16:55
Kyle-Shanks added a commit that referenced this pull request Mar 19, 2024
audius-infra pushed a commit that referenced this pull request Mar 25, 2024
[d09c970] Change internal sdk deps back to wildcard version (#7928) Sebastian Klingler
[7271dbc] [C-4026, C-4059] Update profile page to render basic info and pictures on the server side (#7915) Kyle Shanks
[bfde29f] Log error when retry on new node fails (#7924) Marcus Pasell
[a5a0d96] [C-3961] Fix sign up modal toast triggers (#7916) JD Francis
[0183bf8] [PROTO-1693] Fix tag search triggering autocomplete (#7913) Isaac Solo
[eeb6e19] [C-4028] Harmony QA (#7919) Dylan Jeffers
[fc94c44] [PROTO-1718] Parse DDEX subgenre and handle multiple genre elements (#7921) Theo Ilie
[bf34d35] Bump version to 0.6.66 audius-infra
[8c98f63] [C-4063] Add Sign on routes and aliases to static routes (#7918) Kyle Shanks
[92ced07] [PROTO-1700] Make DDEX publisher support batched and local S3 (#7902) Theo Ilie
[1f61df3] ⚠️ [C-4064] Fix empty feed/trending pages (#7920) Reed
[6ea4bd7] boost relay retries and address in logging (#7914) alecsavvy
[0c8911a] Misc accessibility improvements + mobile drawer fix (#7912) JD Francis
[d62d210] [C-3962] Fix sign up referrals (#7880) JD Francis
[61adb86] Bump version to 0.6.65 audius-infra
[9d270a5] Fix remix uploads and AI attributed uploads (#7911) Marcus Pasell
[4bc9f03] [C-4055] Improve modal and popover performance (#7909) Dylan Jeffers
[befe0a5] [C-4027] Harmony migration fixes (#7907) Dylan Jeffers
[5b3e9fa] Fix search autocomplete race condition (#7908) Isaac Solo
[8f9d022] Fix ddex indexing (#7904) Michelle Brier
[f1472a7] Re-add download to track api response for old clients (#7906) Reed
[e2c7024] Improve search quality: genre, mood, tag, fuzziness, exact matches (#7883) Isaac Solo
[efcb74e] [C-4024] Update SSR to dynamically import Root based on HYDRATE_CLIENT (#7892) Kyle Shanks
[daf7c1e] Log retryTxId in TransactionHandler (#7895) Reed
[38442bd] Bump version to 0.6.64 audius-infra
[e0a0606] Pin docker to older version to fix regression (#7905) Danny
[5f8aecc] [PAY-2577] Fix nft issues (#7884) Saliou Diallo
[f27c8e1] ⚠️  Upload Saga Typed and Parallel Stems v2 (#7723) Marcus Pasell
[4b4fd9b] fix nullable new fields in collection schema (#7903) Michelle Brier
[d71ec07] [PAY-2568] Add access field to playlists (#7894) Andrew Mendelsohn
[63ca5d4] Fix EM parsing of ddex fields (#7900) Michelle Brier
[921a19e] Add undefined check to sentry navigator and remove profile page route (#7898) Kyle Shanks
[73994e0] Add markdown-to-jsx to web (#7899) Dylan Jeffers
[474a902] Remove is_downloadable from trending query (#7896) Reed
[3fb1624] Fix collection search ranking in common (#7855) Isaac Solo
[2a0a663] [C-3561] Remove playback delay on native (#7893) Dylan Jeffers
[28f1150] Fix DDEX e2e tests (#7871) Michelle Brier
[78add43] [Mediorum] [Discovery] Change `/services` protodash routes to `/nodes` (#7864) nicoback2
[8fb1fd9] [SDK] Enable User to User Grants [PAY-2544] (#7889) nicoback2
[184c576] [Discovery] Enable user to user grants ("delegated writes") aka manager mode [PAY-2544] (#7890) nicoback2
[a9b4c5e] Add missing containers for DDEX Docker build (#7891) Theo Ilie
[e7b08ab] [C-4040, C-4032] Fix play cls, Fix pause on track screen (#7888) Dylan Jeffers
[c876ae7] [C-4031, C-4039] Fix add to collection, Add fuzzy search (#7887) Dylan Jeffers
[05a0bb3] Bump version to 0.6.63 audius-infra
[93fed47] Fix discovery ddex fields (#7885) Michelle Brier
[2f5f717] [C-4009] Remove stems styles and package (#7876) Dylan Jeffers
[38e3a87] [C-4034] Fix feed tip tile (#7886) Dylan Jeffers
[e4982a3] Bump mobile version for full-app-release (#7879) Dylan Jeffers
[050a1b3] [C-4014] Fix red chat icons (#7882) Dylan Jeffers
[f1edefd] [INF-672] Improve create-audius-app speed (#7866) Sebastian Klingler
[c4ca5ca] [C-4025] Update profile page for ssr (#7881) Kyle Shanks
[fdbcafc] fix solana relay build (#7872) alecsavvy
[f84715f] Ignore sentry errors for probers (#7870) Marcus Pasell
[2f17f53] Change loglevel to error in TransactionHandler _locallyConfirmTransaction (#7878) Reed
[aeed7a3] [PROTO-1700] Update DDEX UI for batches and improved schema (#7877) Theo Ilie
[062814a] [C-3958] Add StaticImage component and update track artwork rendering on web and mobile-web track pages for ssr (#7847) Kyle Shanks
[a7e4c14] [C-4003] Fix sign up overflow issues on small devices (#7868) JD Francis
[cb27af1] Misc Sign Up QA fixes (#7869) JD Francis
[c259739] Bump version to 0.6.62 audius-infra
[1d4f278] [C-4006] Remove all stems components (#7861) Dylan Jeffers
[efb2b44] Fix audius-cmd test (#7874) Saliou Diallo
[e32d515] Revert "Persist unused DDEX fields in discovery (#7862)" (#7873) Marcus Pasell
[bd822ac] Add fixed-decimal to web deps and update version in libs (#7867) Reed
[02b67d9] Document local audius-compose DDEX steps (#7865) Theo Ilie
[f6bc750] Persist unused DDEX fields in discovery (#7862) Michelle Brier
[e892314] [Protocol Dashboard] Requested changes to new Protocol Dashboard [C-4013] (#7863) nicoback2
[ed08d93] Related artists in sql (#7857) Steve Perkins
[6cad61d] [C-3953] Perma-dismiss purple select artists banner on mobile (#7790) JD Francis
[4cafd23] Bump version to 0.6.61 audius-infra
[075f483] [PAY-2348][PAY-2317] Remove download json field from track metadata (#7784) Saliou Diallo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants