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

Show on App Launch: Open existing tab if Specific Page url matches #5152

Conversation

mikescamell
Copy link
Contributor

@mikescamell mikescamell commented Oct 18, 2024

Task/Issue URL: https://app.asana.com/0/1207908166761516/1208369545355291/f

Description

This has been a marathon 🏃

This PR does the following:

  • Adds a trailing slash to a user entered url if appropriate

  • Removes http/https schemes and www subdomain when comparing the users url

  • Tries to resolve the url entered by the user so that we can prevent duplicating tabs when opening the app

    • This is where most of the time was spent on this PR.
    • After many options were explored (HttpClient, headless browser) we opted to observe the url being loaded when loading the specific url tab, which minimises duplication but will not stop it completely. The main reason we went with this was encountering Javascript Redirects which stop us easily getting the fully resolved url.
    • A caveat to this approach is that if a user clicks on a link while the website has not finished loading then we cannot get the resolved url.

    When opening the app for the first time with "Specific Url" set we:

    • Check to see if we have an existing tab with that url and load it if we do
    • If not we add a tab and store the tabId in memory that we're loading so we can observe the url that is loaded. We cannot prevent this first possible tab duplication as we need to load the website for the fully resolved url.
    • As the website is loading we continuously update the resolved url in prefs e.g. here in the UK bbc.com eventually resolves to bbc.co.uk
    • When the user launches the app again we'll now have two urls to check, the one they set and the one that has been resolved
    • We check both against their existing tabs
    • If one exists we open it and don't duplicate tabs.

This PR is explicitly focused on the Specific Page Url option and not the Last Opened Tab or New Tab Page options, they remain unchanged.

Steps to test this PR

Setting Specific Tab Option

  • Ensure you have no tabs
  • Open Show on App Launch feature in General settings
  • Leave url as default
  • Close the app via recents
  • Open the app
  • The DuckDuckGo home page should load

Simple Duplicate Tab check

  • Continuing from before, close the app via recents
  • Open the app
  • Check there is only one tab and the DuckDuckGo home page loads

Setting Specific Tab Option to URL that resolves to something different

  • Ensure you have no tabs
  • Open Show on App Launch feature in General settings
  • Enter "wikipedia.com" as url
  • Press back
  • Check url underneath "Show on App Launch" feature is "http://wikipedia.com"
  • Close the app via recents
  • Open the app
  • wikpedia.com should start loading
  • Wait until wikipedia.org loads

More complex duplicate tab check

  • Continuing from before, close the app via recents
  • Open the app
  • Check there is only one tab and it's wikipedia.org

Tab duplication only first time

  • Continuing from before, open Show on App Launch feature in General settings
  • Delete and re-enter "wikipedia.com" as the url, this will reset the "resolved url" that we store
  • Press back
  • Check url underneath "Show on App Launch" feature is "http://wikipedia.com"
  • Close the app via recents
  • Open the app
  • Check there are two tabs and wikipedia.org loads in the current tab

Tab duplication with different paths

  • Ensure you have no tabs
  • Open Show on App Launch feature in General settings
  • Set url to "news.sky.com"
  • Press back
  • Close the app via recents
  • Open the app
  • A tab should open to "news.sky.com". Wait for it to finish loading.
  • Open an article in a background tab.
  • Open the tab and wait for it to finish loading
  • The second tab is starts with "news.sky.com/story/"
  • You should now have two tabs
  • Close the app via recents
  • Open the app
  • Check that first tab is opened and not the second
  • Check that you only have two tabs

Tab duplication with extra tab

  • Continuing from before
  • Add a new tab
  • Open any website that is not "news.sky.com"
  • Wait for it to load
  • Close the app via recents
  • Open the app
  • Check that first tab is opened and not the second or third
  • Check that you have three tabs

UI changes

N/A

Copy link
Contributor Author

mikescamell commented Oct 18, 2024

@mikescamell mikescamell changed the title Show on App Launch: Open existing tab if urls match Show on App Launch: Open existing tab if Specific Page url matches Oct 18, 2024
@mikescamell mikescamell marked this pull request as ready for review October 18, 2024 12:46
@anikiki anikiki self-assigned this Oct 18, 2024
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-specific-url-tab-if-exists branch from 190ba65 to f688c6f Compare October 21, 2024 09:52
This is going to make it easier for us to search for the url in the DB as we store the fully resolved url for each tab e.g. bbc.com entered in Omnibar ends up as "https://www.bbc.com/" in the DB

I also added missing tests for non http/https schemes
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/feature-toggle branch from 9786cb1 to b9f1710 Compare October 21, 2024 09:56
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-specific-url-tab-if-exists branch 2 times, most recently from ec5e402 to a757aef Compare October 21, 2024 10:25
This is extracting the existing behaviour, in a future commit we'll fetch the resolved url

Added a test to BrowserViewModelTest which isn't massively helpful but better than nothing
We'll also use the store to keep hold of the tabId that is in used when setting the SpecificPage so that we can more easily identify when we should be attempting to store this url. We'll see this in a future commit.
Now we're storing both the url and resolved urls, we need to check for duplicate tabs.

Using a DB query seemed too complex and I think we can assume that we will not have 1000s of tabs to check (maybe it not in my case if XD)
We add a function to our Handler (should it be manager? 🤷) and if we know that we're on the root of the tab, we have a matching tab id with what was stored when we launched the tab, and it's not null, then we can set the resolved url in the store and re-use it for future launches to avoid duplicate tabs

Next up, tabs!
I noticed we had nothing for selectTabByUrl so added some for that too
It wasn't working correctly when using queries or fragments previously so move back to using URI builder like we do with the ShowOnAppLaunchConverter

I added some more tests as well
These were part of an old approach and are no longer needed
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-specific-url-tab-if-exists branch from a757aef to fa96fac Compare October 21, 2024 12:03
Copy link
Contributor

@anikiki anikiki left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Task/Issue URL:
https://app.asana.com/0/1207908166761516/1208588631698784/f

### Description

When clearing data with the fire button the website set for Specific
Page would load if that option was enabled.

This stops that happening and instead shows the new tab page.

### Steps to test this PR

_Fire button_
- [x] Open Settings
- [x] Open General Settings
- [x] Open Show on App Launch feature
- [x] Enable Specific Page option
- [x] Leave default web address
- [x] Go back to Browser screen
- [x] Press the fire button
- [x] Wait until the app relaunches
- [x] The default webpage (duckduckgo.com) should not load
- [x] The new tab page should be visible

### UI changes

N/A

### Demo


https://github.com/user-attachments/assets/03298b73-9682-485d-8be7-50b5ad5d13a4
@mikescamell mikescamell merged commit 1399bd0 into feature/mike/show-on-app-launch/feature-toggle Oct 22, 2024
4 of 5 checks passed
@mikescamell mikescamell deleted the feature/mike/show-on-app-launch/open-specific-url-tab-if-exists branch October 22, 2024 16:53
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.

2 participants