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

display feed icons on frontpage #3787

Closed
wants to merge 6 commits into from

Conversation

Niehztog
Copy link
Contributor

@Niehztog Niehztog commented Nov 1, 2023

This PR adds the display of the icon of each bridge on the front page of rss bridge. If the icon uri cannot be resolved, the icon will be hidden.
The code change in FrontpageAction.php is just a small refactoring and is optional.

Copy link

github-actions bot commented Nov 1, 2023

Pull request artifacts

Bridge Context Status
BlizzardNews 1 untitled (current) ⚠️ (shutdown) 8192: Function utf8_decode() is deprecated in lib/XPathAbstract.php line 637
BlizzardNews 1 untitled (pr) ⚠️ (shutdown) 8192: Function utf8_decode() is deprecated in lib/XPathAbstract.php line 637
Nius 1 untitled (current) ✔️
Nius 1 untitled (pr) ✔️

last change: Monday 2023-11-13 21:55:32

@Niehztog Niehztog force-pushed the frontpage-feed-icons branch from 6f6b623 to 832a541 Compare November 1, 2023 19:37
@Niehztog Niehztog force-pushed the frontpage-feed-icons branch from 832a541 to 2b61fc9 Compare November 1, 2023 19:50
@gsantner
Copy link
Contributor

gsantner commented Nov 10, 2023

This is a really bad change I would say.

It means that if the provider enabled lets say 100 bridges that has icon defined - then each visitor of frontpage makes additional 100 HTTP requests to bridge !!external!! URLs. Which can be abused for tracking users of rss-bridge on each of these 100 sites without visitors knowing it. Without using the bridge even.

If this has to be done really at dashboard, I would suggest to make it so rss-bridge fetches the icon on it's own and the image is served from rss-bridge cache only .

Just by example of this PR, it adds a quite unique URL which looks concerning.

b34a003c6f2f637ee8f4f7b406f3b9b120b918c04cabec7f03a760e708977ea9689a1c638f4396def8dce7b202cd007eae91946cc3c4a578aa8b5694226cfc6.ico

grafik

@dvikan
Copy link
Contributor

dvikan commented Nov 10, 2023

i agree. resource heave and privacy issue.

rssbridge could act as a proxy and serve icons but that would be a new feature.

@Niehztog
Copy link
Contributor Author

Hello @dvikan and @gsantner ,
thank you very much for your valuable feedback regarding security/privacy and ressource heave of my PR. I addressed all those issues in my latest commit. Please take a look.

@Niehztog
Copy link
Contributor Author

Niehztog commented Nov 12, 2023

The code has been further improved to decrease loading times and save performance. This has been accomplished by applying a timeout value of 10 seconds when loading images and storing information about broken images in cache to prevent continued attempts to load them.

The PR is now ready of being merged.

@dvikan
Copy link
Contributor

dvikan commented Jan 9, 2024

@Niehztog thanks for the effort but wont merge this

@dvikan dvikan closed this Jan 9, 2024
@Niehztog
Copy link
Contributor Author

Niehztog commented Jan 9, 2024

@dvikan could you please explain your decision? All issues are addressed, so whats the problem?

@dvikan
Copy link
Contributor

dvikan commented Jan 9, 2024

Main reason is visiting the front page produces 438 network calls which is too burdensome/slow/annoying for users.

Also low code quality. Sorry.

@Niehztog
Copy link
Contributor Author

Niehztog commented Jan 9, 2024

@dvikan Thank you for clarification. We could work together to address those issues you see in the code quality. You could just tell me which parts need to be improved.
The issue with the amount of network requests could be addressed by limiting display of feed icons only to activated bridges.

@dvikan
Copy link
Contributor

dvikan commented Jan 9, 2024

For example on https://rss-bridge.org/bridge01/ all 436 bridges are enabled/activated. We cant generate that many network calls for a single user. Not good ux.

@Niehztog
Copy link
Contributor Author

Niehztog commented Jan 9, 2024

I agree

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.

3 participants