search

powerpak / hn-sidebar

Hacker News Sidebar for Chrome
https://chrome.google.com/webstore/detail/hacker-news-sidebar/ngljhffenbmdjobakjplnlbfkeabbpma
MIT License
22 stars 5 forks source link

Fixes: Nicer UI, don't send url query params to thriftDB #3

Closed tkafka closed 10 years ago

tkafka commented 10 years ago

I like it much more like this. Plus I really don't like my query args (containing email ids etc.) sent to third party. What do you think?

2014-01-13_14-32-26 2014-01-13_14-33-05 2014-01-13_14-33-24

powerpak commented 10 years ago

Hey Tomáš,

Thanks for contributing! I really like the little stylistic changes you made, and agree that they help usability.

About the query parameters change, I cleaned it up a little because you declared a variable without using var which is a no-no (just a linting nitpick). Whether it's a good idea to drop the query string or not, I'm not sure yet, but I've merged it and I'm going to run with it for a little bit to see if it's noticeably better or worse. For example, it will definitely break HN comments on certain search results pages, but on second thought, I'm not sure I did want to be forwarding all of those to ThriftDB. The things it will definitely break that I will miss are URLS on old forum and blog software that change query parameters to change posts/pages, e.g. this link which is currently frontpaged on HN.

I will think about it, and if I come up with something (maybe a blacklist of sites I don't want query string params to be sent for) I'll add it in, then bump the version number and re-push the Chrome Extension.

Cheers Ted

tkafka commented 10 years ago

Hi Ted, thanks for merging!

If you look closely, it was't leaking var, just a badly formatted 2 line var block, but you saved a few lines anyway :).

Thanks for thinking about the query removal, you're right that I forgot about pretty big part of online forums, but I still prefer not seeing some discussions over sending all my queries to third party. Maybe a whitelist of a well-known query keys for PHPBB et al. would be good enough solution and could be easily improved over time?

(But it may pose a problem if someone posts something like ...?PHPSESSID=yyy&thread=xxx, we sanitize it to ...?thread=xxx and ThriftDB will no longer match it with original url, sigh. On the other side, HN posters should be smart enough to strip unnecessary or private query params :))

What do you think? Cheers, Tomas

powerpak commented 10 years ago

Oh whoops, you're right about the var, the indentation confused me.

Do you know if there is a list of such query keys? I suppose one way of doing this would be to scan the ThriftDB for all submitted URLs with query strings and then start picking out common param names that we know aren't sensitive.

tkafka commented 10 years ago

Hi, nope - sorting thriftDB's query args by frequency is a great idea :)