feat(NcAvatar): add a version prop for longer avatar caching

[pringelmann]

Adds a version prop to NcAvatar. When set it goes into the avatar URL as ?v=, which earns a much longer cache lifetime server-side. Talk will pass it from the participant list.

Also fixes the existing current-user handling. The version was appended as '?v=' + version onto a URL that already ends in ?guestFallback=true, giving /avatar/me/64?guestFallback=true?v=3. PHP reads that as guestFallback=true?v=3 with no v parameter, so cache-busting your own avatar has never worked. Going through generateUrl gives a proper &v=.

Needs nextcloud/server#64295 to have any effect. Harmless without it.

☑️ Resolves

• Groundwork for perf(core): allow versioned avatar URLs to be cached for 30 days nextcloud/server#64295

🖼️ Screenshots

Nothing visual. The avatar renders the same, only the URL changes.

🏁 Checklist

• ⛑️ Tests are included or are not applicable
• 📘 Component documentation has been extended, updated or is not applicable
• 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.27%. Comparing base (dab2fb1) to head (f08cc2a).
⚠️ Report is 24 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8959      +/-   ##
==========================================
+ Coverage   57.39%   58.27%   +0.87%     
==========================================
  Files         123      123              
  Lines        4460     4443      -17     
  Branches     1331     1323       -8     
==========================================
+ Hits         2560     2589      +29     
+ Misses       1614     1575      -39     
+ Partials      286      279       -7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Antreesy]

Does it mean e.g. that for Talk any endpoint that potentially can contain a user information (room, message, attendee, e.t.c) should provide avatarVersion on top?

[pringelmann]

Does it mean e.g. that for Talk any endpoint that potentially can contain a user information (room, message, attendee, e.t.c) should provide avatarVersion on top?

No. If a payload has no version the avatar just renders without ?v= and keeps today's 1 day cache, so a missing one costs optimisation, not correctness. Nothing breaks but it just doesn't get faster. So it only needs to go where the high volume actually is, which for Talk is the participant list and the message list.

I'm doing the Talk PR too, and I plan not to add the version endpoint by endpoint there. The current toArray() already emits actorDisplayName, so the version will go right next to it with a bulk prefetch, so that every endpoint gets it for free.

[Antreesy]

So it only needs to go where the high volume actually is, which for Talk is the participant list and the message list.

Likely rooms list as well, as it's polled every 30 seconds, and always includes all 1-1 rooms - but avatar here could come from another endpoint, not sure if it's cached

The current toArray() already emits actorDisplayName, so the version will go right next to it with a bulk prefetch, so that every endpoint gets it for free.

That's my concern - if it's something like "avatarVersion": "<digit>" - that's roughly 20 bytes per user entry, right?
I pulled log from the busy call (60-mins window, 25 Aug for reference):

• 7K requests to participants list, 160 users each
• 21K requests to room list, let's say ~50 private rooms each
• 1200 chat messages, relayed to ~120 active users

This field overhead sent in payload would be ~50MB total

Same period - 10K requests to 'avatar/user/size?guestFallback=true' - we assume they cached once per day - I see roughly 2-11 Kb each for 64*64

Not sure, if less pinging of avatars endpoint won't translate to more data transfer as JSON - and whether it's better or worse.

[pringelmann]

So it only needs to go where the high volume actually is, which for Talk is the participant list and the message list.

Likely rooms list as well, as it's polled every 30 seconds, and always includes all 1-1 rooms - but avatar here could come from another endpoint, not sure if it's cached

The current toArray() already emits actorDisplayName, so the version will go right next to it with a bulk prefetch, so that every endpoint gets it for free.

That's my concern - if it's something like "avatarVersion": "<digit>" - that's roughly 20 bytes per user entry, right? I pulled log from the busy call (60-mins window, 25 Aug for reference):

* 7K requests to participants list, 160 users each

* 21K requests to room list, let's say ~50 private rooms each

* 1200 chat messages, relayed to ~120 active users

This field overhead sent in payload would be ~50MB total

Same period - 10K requests to 'avatar/user/size?guestFallback=true' - we assume they cached once per day - I see roughly 2-11 Kb each for 64*64

Not sure, if less pinging of avatars endpoint won't translate to more data transfer as JSON - and whether it's better or worse.

Fair on the rooms list, I hadn't thought about that 🤔

The 20 bytes is pre-compression though. I mocked a 150-user participants payload: 27 B/user raw, under 4 B/user gzipped, because the key repeats once per user and that's most of the bytes. So ~10MB rather than 50MB, against tens of MB of avatars that get refetched every call since the 1 day window always expires in between.

Agreed it shouldn't go everywhere though. I'll keep it to the participants list for now, that's where the huge volume is, and leave chat and rooms alone until there's a reason. wdyt?

[Antreesy]

Sounds fine, we can discuss it in Talk PR once again with backender POV.

One more thing I could think of: NcAvatar uses BrowserStorage to keep keys like _user-has-avatar.<userId> : true - maybe we can utilize this?

• e.g. NcAvatar always request image with &v=0
• Server sends X-header that tells actual avatarVersion (if avatar is stale or was updated, should be stale and get number > known?)
• We store it in BS instead of boolean flag
• And if there's a match on userId at later request, we add &v={avatarVersion} from now on (IIRC that would be +1 extra request for changed version)

[pringelmann]

Sounds fine, we can discuss it in Talk PR once again with backender POV.

One more thing I could think of: NcAvatar uses BrowserStorage to keep keys like _user-has-avatar.<userId> : true - maybe we can utilize this?

* e.g. NcAvatar always request image with `&v=0`

* Server sends X-header that tells actual `avatarVersion` (if avatar is stale or was updated, should be stale and get number > known?)

* We store it in BS instead of boolean flag

* And if there's a match on userId at later request, we add `&v={avatarVersion}` from now on (IIRC that would be +1 extra request for changed version)

Yeah good idea, but the catch is the client only sees that header on a request that actually reaches the server. Once ?v=0 is cached for 30 days there's no request, so no header, and if the avatar changed in the meantime we'd keep serving the old one until it expires.

You could keep ?v=0 on a short cache so it revalidates and picks up the new version, but that's a request per user per day, which is the cost we're trying to remove in the first place.

Happy to go through the idea again on the Talk PR, will open it this evening :)

[Antreesy]

@nextcloud/router provides generateAvatarUrl, which is an exact copy of this. Maybe we migrate to it instead?
I can open a PR, but it would be conflicting. Waiting for opinions. IMO should be beneficial, as function can be reused by apps, and not only locked by NcAvatar

[Antreesy]

Went ahead for nc/router repo: nextcloud-libraries/nextcloud-router#916

[pringelmann]

thank you!

[Antreesy]

Shall we bump @nextcloud/router and get this change in?

export function getAvatarUrl(user: string, options?: AvatarUrlOptions): string {
      return generateAvatarUrl(user, {
              isDarkTheme: options?.isDarkTheme ?? checkIfDarkTheme(document.body),
              isGuestUser: options?.isGuest,
              size: options?.size,
              guestFallback: true,
              version: options?.version,
      })
}

[skjnldsv]

So, is this pr still wanted then ?

[pringelmann]

So, is this pr still wanted then ?

Yes, but I need to update it based on router changes. Will ping when ready

[Antreesy]

Shall we bump @nextcloud/router and get this change in?

export function getAvatarUrl(user: string, options?: AvatarUrlOptions): string {
      return generateAvatarUrl(user, {
              isDarkTheme: options?.isDarkTheme ?? checkIfDarkTheme(document.body),
              isGuestUser: options?.isGuest,
              size: options?.size,
              guestFallback: true,
              version: options?.version,
      })
}

[Antreesy]

Tested (?v=1 is from top-right menu on old lib version, &v=1 is from app + bumped lib)