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 bugs #2360

Merged
merged 4 commits into from
Jul 18, 2023
Merged

Fix bugs #2360

merged 4 commits into from
Jul 18, 2023

Conversation

MattDHill
Copy link
Member

fix reset tor, delete http redirect, show message for tor http, update release notes

@MattDHill MattDHill requested a review from elvece July 18, 2023 14:59
@@ -39,14 +39,6 @@ export class AppComponent implements OnDestroy {
) {}

async ngOnInit() {
if (location.hostname !== 'localhost' && location.protocol === 'http:') {
Copy link
Member

@elvece elvece Jul 18, 2023

Choose a reason for hiding this comment

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

are we moving the redirect to the BE? or abandoning it all together?

Copy link
Member Author

Choose a reason for hiding this comment

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

Abandoning. It's unnecessary and the code was problematic

Copy link
Member

Choose a reason for hiding this comment

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

ok, could you relay why? just curious what changed @dr-bonez? for my own understanding

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. It just didn't work in my testing
  2. If it did work, the UX would be that after a period of time and with no indication why, the page would suddenly redirect/refresh
  3. There was no catch block, so if the fetch failed, it would be an unhandled promise rejection.

We don't need to force people away from http. We just need to tell them why they should be using https

Copy link
Member

Choose a reason for hiding this comment

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

Cool thanks for explaining. Those concerns could for sure be addressed in code, I definitely made a mistake not adding the catch block. In my testing, it only redirected once on initial load and worked great. Perhaps the lag over Tor would be abrupt. I think we had wanted to do this for a few reasons, rather than making the user manually change, but understood its not ideal to force the behavior.

@MattDHill MattDHill merged commit 11c21b5 into master Jul 18, 2023
@MattDHill MattDHill added this to the 0.3.4.4 milestone Jul 19, 2023
@dr-bonez dr-bonez deleted the fix/fe-bugs branch November 9, 2023 19:43
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