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: thumbnail should show last thumbnail in server in any condition #858

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Monirzadeh
Copy link
Collaborator

@Monirzadeh Monirzadeh commented Mar 15, 2024

thumbnail not update in browser sometimes.

bun.lockb update with make style so i just push that too.

Closes #857
Depends on #896

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.76%. Comparing base (595cb45) to head (ce2ce88).
Report is 31 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
+ Coverage   25.58%   30.76%   +5.18%     
==========================================
  Files          47       60      +13     
  Lines        5628     4456    -1172     
==========================================
- Hits         1440     1371      -69     
+ Misses       3989     2863    -1126     
- Partials      199      222      +23     

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

@Monirzadeh Monirzadeh changed the title fix: thumbnail should show last thumnail in server in any condition fix: thumbnail should show last thumbnail in server in any condition Mar 15, 2024
return {
backgroundImage: `url("${this.imageURL}")`,
Copy link
Member

Choose a reason for hiding this comment

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

Can we use some properties from the bookmark instead of a random string? This could break the cache unintentially since each time the query string is going to be different. Can we use ?mtime=<bookmark modified time> ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will modified time update if just thumbnail update in article? i am sure that i am not update that in #683 so if that will handle in other part of code automatically we can use modified time too.
i don't find when modified time update in code. do you know where is that or i can send request to update that while thumbnail update

if just thumbnail will update but modify time not update automatically than it will get in trouble in that situation.

Copy link
Member

Choose a reason for hiding this comment

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

I always assumed Bookmark.ModifiedTime was the datetime when the bookmark was last modified (attributes/archive/readable/ebook) modified, it seems that is not the case. Let me think and get back to you.

Copy link
Member

Choose a reason for hiding this comment

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

I ended up not updating you here.

My idea is:

  • Refactor the migration system (feat: new migration system #876)
  • Add a created column to the bookmarks with the time the bookmark was first created and prefill it with the current modified for existing bookmarks.
  • Update modified every time the bookmark is updated (either the data, content, archive, etc).

Then, we can use the modified as query parameter for the thumbnail.

Copy link
Collaborator Author

@Monirzadeh Monirzadeh Apr 7, 2024

Choose a reason for hiding this comment

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

it is better so i will wait until #876 finish than start working on that.
but an small point: i like the idea of keep first created time and modified time separately if exist instead of prefill (you can look at #848)
so created and modified be better to separated. having created time help me to track my activity in long term so i think having that can be helpful.
@fmartingr and @GreenBlast what do you think?

Copy link
Collaborator Author

@Monirzadeh Monirzadeh Apr 7, 2024

Choose a reason for hiding this comment

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

can you mention the function that currently update modified time? i can't find that (to use when just thumbnail update).

@Monirzadeh Monirzadeh self-assigned this May 17, 2024
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.

Issue with image display in Chrome v1.6.0-rc.7
2 participants