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

fix_: ensure generated identity-images have a valid clock value #6239

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

seanstrom
Copy link
Member

@seanstrom seanstrom commented Jan 7, 2025

status-mobile PR: status-im/status-mobile#21931
status-desktop PR: status-im/status-desktop#17054

Summary

  • This PR attempts to resolve an issue related to how identity images are generated with clock field defaulted to zero. The clock field is likely used for determining the creation time of the identity images, we can help with detecting changes to identity images after they've been updated.
  • For example, here's the current work-around being used inside status-mobile: https://github.com/status-im/status-mobile/pull/21795/files

@seanstrom seanstrom self-assigned this Jan 7, 2025
@seanstrom seanstrom force-pushed the seanstrom/fix-identity-image-clock branch from b67ad51 to 97b4d83 Compare January 7, 2025 18:16
@status-im-auto
Copy link
Member

status-im-auto commented Jan 7, 2025

Jenkins Builds

Click to see older builds (48)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ b67ad51 #1 2025-01-07 18:20:05 ~3 min macos 📦zip
✔️ b67ad51 #1 2025-01-07 18:20:37 ~4 min ios 📦zip
✔️ b67ad51 #1 2025-01-07 18:20:59 ~4 min linux 📦zip
✔️ b67ad51 #1 2025-01-07 18:21:36 ~5 min android 📦aar
✔️ b67ad51 #1 2025-01-07 18:21:46 ~5 min windows 📦zip
✔️ b67ad51 #1 2025-01-07 18:22:50 ~6 min tests-rpc 📄log
✔️ b67ad51 #1 2025-01-07 18:25:22 ~9 min macos 📦zip
✔️ b67ad51 #1 2025-01-07 18:47:03 ~30 min tests 📄log
✔️ 97b4d83 #2 2025-01-07 18:24:06 ~3 min macos 📦zip
✔️ 97b4d83 #2 2025-01-07 18:25:28 ~3 min windows 📦zip
✔️ 97b4d83 #2 2025-01-07 18:25:35 ~4 min linux 📦zip
✔️ 97b4d83 #2 2025-01-07 18:27:15 ~5 min android 📦aar
✔️ 97b4d83 #2 2025-01-07 18:28:28 ~7 min ios 📦zip
✔️ 97b4d83 #2 2025-01-07 18:29:20 ~6 min tests-rpc 📄log
✔️ 97b4d83 #2 2025-01-07 18:33:51 ~8 min macos 📦zip
✔️ 97b4d83 #2 2025-01-07 19:19:16 ~32 min tests 📄log
✔️ f89776c #3 2025-01-08 19:34:33 ~4 min windows 📦zip
✔️ f89776c #3 2025-01-08 19:35:07 ~4 min macos 📦zip
✔️ f89776c #3 2025-01-08 19:35:11 ~5 min ios 📦zip
✔️ f89776c #3 2025-01-08 19:35:21 ~5 min linux 📦zip
✔️ f89776c #3 2025-01-08 19:35:36 ~5 min android 📦aar
✖️ f89776c #3 2025-01-08 19:36:35 ~6 min tests-rpc 📄log
✔️ f89776c #3 2025-01-08 19:40:52 ~10 min macos 📦zip
✖️ f89776c #3 2025-01-08 20:02:17 ~32 min tests 📄log
✔️ 6fad0f4 #4 2025-01-08 19:51:06 ~3 min windows 📦zip
✔️ 6fad0f4 #4 2025-01-08 19:52:21 ~5 min macos 📦zip
✔️ 6fad0f4 #4 2025-01-08 19:52:22 ~5 min ios 📦zip
✔️ 6fad0f4 #4 2025-01-08 19:52:30 ~5 min linux 📦zip
✔️ 6fad0f4 #4 2025-01-08 19:52:50 ~5 min android 📦aar
✔️ 6fad0f4 #4 2025-01-08 19:53:22 ~5 min tests-rpc 📄log
✔️ 6fad0f4 #4 2025-01-08 19:56:13 ~8 min macos 📦zip
✔️ 2494be9 #5 2025-01-08 19:55:28 ~3 min windows 📦zip
✔️ 2494be9 #5 2025-01-08 19:57:47 ~5 min linux 📦zip
✔️ 2494be9 #5 2025-01-08 19:58:03 ~5 min ios 📦zip
✔️ 2494be9 #5 2025-01-08 19:58:07 ~5 min macos 📦zip
✔️ 2494be9 #5 2025-01-08 19:58:18 ~5 min android 📦aar
✔️ 2494be9 #5 2025-01-08 19:59:38 ~6 min tests-rpc 📄log
✔️ 2494be9 #5 2025-01-08 20:08:35 ~12 min macos 📦zip
✖️ 2494be9 #4 2025-01-08 20:33:20 ~30 min tests 📄log
✖️ 2494be9 #5 2025-01-08 23:26:18 ~27 min tests 📄log
✔️ f1ef32f #6 2025-01-08 23:19:01 ~3 min windows 📦zip
✔️ f1ef32f #6 2025-01-08 23:19:12 ~3 min macos 📦zip
✔️ f1ef32f #6 2025-01-08 23:19:47 ~4 min ios 📦zip
✔️ f1ef32f #6 2025-01-08 23:20:20 ~5 min linux 📦zip
✔️ f1ef32f #6 2025-01-08 23:20:37 ~5 min android 📦aar
✔️ f1ef32f #6 2025-01-08 23:21:30 ~6 min tests-rpc 📄log
✔️ f1ef32f #6 2025-01-08 23:24:36 ~9 min macos 📦zip
✔️ f1ef32f #6 2025-01-08 23:55:54 ~29 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4239583 #7 2025-01-14 07:12:55 ~4 min macos 📦zip
✔️ 4239583 #7 2025-01-14 07:13:16 ~5 min ios 📦zip
✔️ 4239583 #7 2025-01-14 07:13:35 ~5 min linux 📦zip
✔️ 4239583 #7 2025-01-14 07:13:45 ~5 min macos 📦zip
✔️ 4239583 #7 2025-01-14 07:13:46 ~5 min windows 📦zip
✔️ 4239583 #7 2025-01-14 07:13:52 ~5 min android 📦aar
✔️ 4239583 #7 2025-01-14 07:14:31 ~6 min tests-rpc 📄log
✔️ 4239583 #7 2025-01-14 07:40:40 ~32 min tests 📄log
✔️ 281fb22 #8 2025-01-17 12:10:19 ~3 min macos 📦zip
✔️ 281fb22 #8 2025-01-17 12:10:36 ~3 min ios 📦zip
✔️ 281fb22 #8 2025-01-17 12:10:58 ~4 min windows 📦zip
✔️ 281fb22 #8 2025-01-17 12:12:16 ~5 min macos 📦zip
✔️ 281fb22 #8 2025-01-17 12:12:24 ~5 min linux 📦zip
✔️ 281fb22 #8 2025-01-17 12:12:52 ~6 min android 📦aar
✔️ 281fb22 #8 2025-01-17 12:13:01 ~6 min tests-rpc 📄log
✖️ 281fb22 #8 2025-01-17 12:37:06 ~30 min tests 📄log
✔️ 281fb22 #9 2025-01-18 04:26:55 ~27 min tests 📄log

@seanstrom seanstrom changed the title fix: ensure generated identity-images have a valid clock value fix_: ensure generated identity-images have a valid clock value Jan 7, 2025
@jrainville
Copy link
Member

If I understood correctly and this PR is to fix the issue that images don't re-render properly when they get updated, we also have it.

The problem is that we use a local server and the URLs it uses to serve the images only contain a pubkey, which doesn't change when the image being served changes.

I fixed that issue for community images here: #6118

I opened a similar issue to do the same thing for profile images here: status-im/status-desktop#16814

So if I'm right with my assessment of the problem you're trying to fix, just applying the same type of fix I did for the community to the profile image should do the trick.

Let me know if I misunderstood, if you have questions or if you need help trying to do the same work. @seanstrom

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.56%. Comparing base (bb7b1f2) to head (281fb22).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
protocol/messenger_contacts.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #6239   +/-   ##
========================================
  Coverage    61.56%   61.56%           
========================================
  Files          845      845           
  Lines       110696   110697    +1     
========================================
+ Hits         68147    68154    +7     
+ Misses       34585    34582    -3     
+ Partials      7964     7961    -3     
Flag Coverage Δ
functional 21.63% <0.00%> (+0.10%) ⬆️
unit 60.09% <75.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
images/main.go 67.74% <100.00%> (+0.52%) ⬆️
server/server_media.go 66.66% <100.00%> (+2.77%) ⬆️
protocol/messenger_contacts.go 62.24% <0.00%> (ø)

... and 41 files with indirect coverage changes

@seanstrom
Copy link
Member Author

@jrainville Yup it seems like we're aiming to solve the same problem 🙌

Although I'm a little confused about the difference between clock and version here. I thought clock was meant to be used as an updatedAt timestamp, is that true?

I suppose we could create another version map, but maybe we can leverage the clock value. At least that's what we're sorta doing in mobile, but we need to override the clock field since it never gets updated and is always zero. That's partly why I made this PR change because I thought the clock value always being zero might be a bug. Thoughts?

Copy link
Contributor

@ilmotta ilmotta left a comment

Choose a reason for hiding this comment

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

Using the clock seems like a simple and effective solution, in a way, it's versioning too. But maybe @jrainville has another idea in mind. Approving in advance anyway.

@jrainville
Copy link
Member

Although I'm a little confused about the difference between clock and version here. I thought clock was meant to be used as an updatedAt timestamp, is that true?

@seanstrom I'm not sure either. I don't know how the clock works for the images, but I'm also not sure how it would help for the problem we have on Desktop. The clock isn't exposed to the mediaserver as far as I can see, so the URL would still stay the same no, on a Contact for example?

I suppose we could create another version map, but maybe we can leverage the clock value. At least that's what we're sorta doing in mobile, but we need to override the clock field since it never gets updated and is always zero. That's partly why I made this PR change because I thought the clock value always being zero might be a bug. Thoughts?

Yeah using the clock in the mediaserver would serve the same purpose as using a local version in the URL. As long as the URL changes from image version to another, it fixes the issue.
I used a version because I didn't think it was necessary to save a clock or have a persisted solution, but I might have missed that there was already a clock. Or maybe IdentityImage has a clock and not the community one.

Either way, as long as the images have a different URL, I'm fine 😄

@seanstrom seanstrom force-pushed the seanstrom/fix-identity-image-clock branch from 97b4d83 to f89776c Compare January 8, 2025 19:29
@seanstrom
Copy link
Member Author

@jrainville Ahh I see what you mean know about the community images not having a clock field available. I misunderstood which image type was being used for the community images, and it seems the clock field is only available to the IdentityImage type, which is used for contacts and profiles, but not used for the community image (I think).

After looking around a little more, I found a utility the function MakeContactImageURL, and I've made a new change to include the clock value in the formatted LocalUrl. Let me know what you think about that since that may help resolve contact images re-rendering on desktop.

Hopefully this could work for both mobile and desktop 🙏
What do you think?

@seanstrom seanstrom force-pushed the seanstrom/fix-identity-image-clock branch 4 times, most recently from 2494be9 to f1ef32f Compare January 8, 2025 23:14
@igor-sirotin
Copy link
Collaborator

Just to more ideas on top 😅

We could also use some <uuid>.png with no URL parameters at all. And make it a common solution common for all images. The media server would just need to have a map[uuid]imagePath and resolve all UUIDs to some data, would it be a community, contact or whatever image.
This should make the media server itself much simpler.

Any solution will work, and are used in web world. version and clock approaches are simpler to implement.
Would be nice to have a common solution for difference images, but it's probably not a requirement.

Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Awesome! I just tested it in desktop by removing our ugly patch of manually adding a timestamp to the URLs to force a refresh. It refreshes the images just fine!

Thanks for doing that. It saved me a bunch of time 🙌

@seanstrom seanstrom force-pushed the seanstrom/fix-identity-image-clock branch from 4239583 to 281fb22 Compare January 17, 2025 12:06
@seanstrom seanstrom merged commit e6738e5 into develop Jan 18, 2025
20 checks passed
@seanstrom seanstrom deleted the seanstrom/fix-identity-image-clock branch January 18, 2025 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants