#metabrainz

[listenbrainz-server] 14amCap1712 opened pull request #2648 (03master…fix-links): LB-1372: Cannot link a release from the stats page https://github.com/metabrainz/listenbrainz-serv...

[listenbrainz-server] 14amCap1712 opened pull request #2649 (03master…update-deps): Update dependencies https://github.com/metabrainz/listenbrainz-serv...

monkey: hi! lmk when you are around for a quick chat about LB-1371

LB-1371: "Playing now" does not show cover art on page load https://tickets.metabrainz.org/browse/LB-1371

i diagnosed the issue, have a quick fix and an intrusive proper fix in mind. not sure which one to go with.

O’Moin

yvanzo: hi! do you have the required GPG key mentioned here: https://github.com/metabrainz/mb-rngpy ?

need to update the models for https://github.com/metabrainz/mb-rngpy/pull/9

lucifer: yes it is the same as in MB server release process

ah makes sense. can you please do it whenever you get time?

Hi lucifer ! Give me 8 minutes to make some coffee and I'm ready for a chat

sounds good, thanks

lucifer: sure, doing it now

thanks

lucifer: Allrighty, what can I help with?

I read the ticket

i'll start with explaining what's going on and then we can discuss fixes.

1. ListenCard does not deepcopy the listen object it receives.

2. The playing now listen's field is directly modified in this case.

because the state of listen card and the object here share the object ref, the update is propagated without react knowing about it.

3. the check here returns false because the listen was updated already when the modification in 2 happened.

so we get an outdated/incorrect cover art thumbnail.

does this make sense?

OK, I'm following so far, looks like the classic react class props updating issues

(my bad)

right that props issues plus not cloning the object before storing it in state

OK, that seems easy to fix

the point fix is indeed easy.

we could deepcopy the playing now listen before passing it in the prop.

and update the check in componentDidUpdate to compare the existing state to the new prop

and merge the data together in case of a mismatch.

or we could start including the caa_id and caa_release_mbid in the react key for the listen cards.

the fix involving key is probably the recommended one.

Yep, I was wondering what we could use in the key to make it more robust

It's indeed the absolute simlest.

+p

however, i was wondering as we move LB towards a SPA. there will be more and more such cases of fields that need updating.

However it does require the same mechanism be applied everywhre ListenCards are in use, which will result in some issue eventually

how many fields do we manually list out in the key? should we instead look into hashing the entire object and using the hashcode as a key

Hm, not a bad idea at all

Our objects are not gigantic so the computational tradeoff is probably OK.

i couldn't find anything online about using hashcodes as keys which is weird.

i assumed others would have faced similar issues

right the computational tradeoff is something i am not sure about. i was also going to suggest to always deepcopy the listen inside the constructor of the ListenCard to avoid the other issue in this bug but again that comes at a computational tradeoff.

Keys are usually easier to generate, and I think the main issue is in componendDidUpdate which is being retired, so probably not a huge use case

The deep cloning shouldn't be an issue actually.

At least for our sizes of objects and number of listencards

i see makes sense.

i can see the same issues arising out of multiple useEffects interactions in ListenCard when we do the rewrite of the component but we can take a look into it when it actually happens i guess.

Looking at https://www.npmjs.com/package/object-hash which has some ways to optimize too, such as `excludeKeys` to avoid using keys that won't help in the hash

(re componentDidUdpate going away)

Yeah, that's solution 3. : rewriting the ListenCard to a functional component

Less likely to run into issues then

so i think i will just update the keys to use more fields now.

we can open a ticket for using hashcodes in keys, when we encounter the next such bug.

Sounds good. Please do check everywhere it is used

sure will do

also do you need any help with the frontend tasks?

for YIM

I was just going to ask you if we could have a sit down to plan the next couple of weeks :)

i don't see any backend tasks for it at the moment, but i am probably forgetting something so will ask mayhem again.

sure makes sense.

actually, there is one things we can do for monkey.

I've got the YIM page pretty much set up, apart from the data we're adding this year (I used last year's data to dev)

object-hash

Woops, copypasta fail

in periodic jams we missed the "don't allow more than two tracks by the same artist in the playlist" portion.

i'll update the spark code for that.

that should improve the playlists, but may end up not generating as many tracks.

lucifer: thanks!

I think it would be a good idea to generate this year's stats (complete with new datapoints) for a few users for some proper testing, including all the "you don't have enough data" stuff

sure will do

do we want to add any new stats/datapoints?

We also have the SVG images currently pending. I started setting them up

i can do the SVGs if you want.

Re: new data points: if you compare with the figma design, there are some items currently missing, in particular genre-related data

I don't know if we're doing the cover art color one

ahhh i was wondering what i was missing. yeah that would be it.

If we are not, then I have a good replacement: show the percentage difference between number of listens last year and this year

sure even simpler.

I'm still unsure what the level of mystery is supposed to be.

Actually, I understand the idea, but not the implementation. Also not sure how that will look with LB's sparse user data

i am not sure how the mystery thing would work.

Yeah.

We could possibly replace it with another maybe useful, but definitely easier datapoint: which percentile are you in regarding volume of listens

yup makes sense

i.e. you listen to more listens than 68% of LB users

In keeping with the spirit of comparing your stats to the mass of users

I think I'll manage the SVGs, if you want to focus on those datapoints that I sure won't be able to do myself :p

And finally this one would be really nice IMO: an SVG image that is an overview of stats: https://usercontent.irccloud-cdn.com/file/HZgMM...

We already have some of that data, and I guess you'll be working on the tags stats which can then fit in there

makes sense.

i'll get the stats done

I started some small refactoring for the YIM art endpoints, by the way, to allow for this new year while reusing some of the existing code

It's all on branch `yim-2023`

Ah, and I believe you took care of the email design as well last year; do you want to do that this year too, or would you like some help with it?

lucifer: done, added a comment to the pull request.

On my side, I'm going to continue implementing the stats art SVGs and prepare the "coming soon" page, the "not enough data" page and other small items well need to have ready

monkey: should be able to do the emails but would need help with testing on apple devices.

yvanzo: thanks!

yep, happy to help with testing.

[listenbrainz-server] 14amCap1712 merged pull request #2649 (03master…update-deps): Update dependencies https://github.com/metabrainz/listenbrainz-serv...

And maybe in a week or so we have another chat, see where we're at and add in the new datapoints in the page, maybe deploy to test and have a look at what we're forgetting ? :D

sounds great.

👍 sweet, we're on track :)

thanks!

Thank you !

mayhem: how did you handle the case of multiple artists on one track in troi? do you count it as one recording for each?

lemme see

yes.

I unroll the artist credit and then count each of the artists as one.

that is perhaps a bit to tight, but lets just keep that for now.

cool