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(greader): set updateAt time correctly (and fix "Keep Archived Articles" setting) #702

Closed
wants to merge 1 commit into from

Conversation

mbestavros
Copy link
Collaborator

@mbestavros mbestavros commented May 22, 2024

This PR fixes an issue where articles fetched via the Google Reader API had their updateAt time set incorrectly.

Currently, this time is calculated in milliseconds, which converts into a Date object about 50,000 years into the future (since Date takes seconds by default). (See comments for further explanation.) This meant that the "Keep Archived Articles" setting would only ever see articles published in the future and would never delete any of them - causing the database to fill up over time and slow down the app.

Tagging @Ashinch and @JunkFood02 for review.

@JunkFood02
Copy link
Collaborator

image

I think the Date() here uses milliseconds?

@mbestavros
Copy link
Collaborator Author

mbestavros commented May 27, 2024

@JunkFood02 That's... strange. I will look into it when I'm back at my computer, but I did debug that date value and it was off by a factor of 1000 when I tested it. The bug does exist, and this PR does fix it.

@mbestavros
Copy link
Collaborator Author

mbestavros commented May 28, 2024

@JunkFood02 Aha, figured it out! This is way more annoying than I thought...

Turns out, the bug I encountered is specific to Miniflux. The crawlTimeMsec value that Read You uses is provided by the RSS backend, and apparently is not standardized (since Google Reader never had official documentation).

FreshRSS (which is what @Ashinch used to develop the Greader backend) sets crawlTimeMsec & timestampUsec differently, at millisecond- and microsecond-precision respectively.

Miniflux, on the other hand, sets both values at microsecond precision - see here. That's strange, since the different Msec and Usec postfixes would suggest they should be set like FreshRSS does.

In any case, the above mentioned bug does happen on Miniflux. I've opened #707 to track this.

Signed-off-by: Mark Bestavros <markbest@bu.edu>
@mbestavros
Copy link
Collaborator Author

mbestavros commented May 28, 2024

@JunkFood02 I've updated my PR to set the updateAt value using timestampUsec instead of crawlTimeMsec since that value is consistent across both Miniflux and FreshRSS. Both backends send the same value for crawlTimeMsec and timestampUsec (they just send different precisions for crawlTimeMsec, hence this bug), so there should be absolutely no functionality change on the FreshRSS side as a result of this. (I tested the fix against both FreshRSS and Miniflux.)

We could merge this fix now, see what Miniflux upstream says about fixing crawlTimeMsec, and revert back if it ever gets fixed.

EDIT I will also note that, according to this documentation, various other clients (including Reeder, which seems to be a gold standard for @Ashinch) uses timestampUsec for article ordering rather than crawlTimeMsec.

@mbestavros
Copy link
Collaborator Author

Looks like miniflux/v2#2673 fixed the related issue upstream!

I'm going to leave this PR open and let @Ashinch decide whether to merge or not, but either way the issue should be fixed.

@Ashinch
Copy link
Owner

Ashinch commented May 31, 2024

Thanks for your follow up. Since Miniflux has fixed their issues, I will likely keep things unchanged to prevent regression problems with other GReader providers. BTW, I will cut a release this weekend that includes all the new commits. 😉

@mbestavros
Copy link
Collaborator Author

@Ashinch I was about to suggest a new release too! Glad you're already on top of it 😁

@mbestavros mbestavros closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants