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(brave): not updating tab to the new redirect url in some cases. #1285

Merged
merged 3 commits into from Sep 21, 2023

Conversation

whizzzkid
Copy link
Contributor

Closes: #1283

RCA:

  • The timing in brave makes it hard to determine what's happening in the browser.
  • When redirecting from brave internal node, companion cannot find the tab containing that URL.
  • Checking repeatedly fixes the issue mostly.
  • Added tests.

Demo: the double refresh is slightly visible.

Purple-loop.mov

Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
@whizzzkid whizzzkid requested review from lidel and a team as code owners September 21, 2023 10:05
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Usually setTimeout smells fishy, but sometimes this is the only way to get UX we want, especially in Brave when there is additional IPFS code in play on the browser side that executes async (like the purple button scenario).

Retrying for half a second when there are no tabs sounds like a sane failsafe: In the past, I've experienced similar ordering discrepancies between Firefox and Chromium, and they appeared and disappeared between browser releases, so this may also help with smoothing the UX there.

originUrl: 'https://ipfs.io/ipns/en.wikipedia-on-ipfs.org',
redirectUrl: 'http://localhost:8080/ipns/en.wikipedia-on-ipfs.org'
})
await clock.tickAsync(400)
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we base this on MAX_RETRIES_TO_UPDATE_TAB somehow, so the reader can figure out why we sleep here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

Sgtm

Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
@whizzzkid whizzzkid merged commit 4097e2d into main Sep 21, 2023
5 checks passed
@whizzzkid whizzzkid deleted the fix/brave-quirks branch September 21, 2023 16:54
This was referenced Sep 21, 2023
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.

[MV3 Beta Bug] Redirect infinite loop with Brave when hitting "purple IPFS button"
3 participants