Skip to content

Fix tab not being inserted when opening in new tab #6178

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

Merged
merged 2 commits into from
Jun 6, 2025

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Jun 4, 2025

Task/Issue URL: https://app.asana.com/1/137249556945/project/1203249713006009/task/1210059188286306?focus=true

Description

Fix Duck Player video not being shown because of:

  1. In TabsRepository we were using postValue instead of setValue, and therefore in some cases the new value was not emitted in time
  2. This X post generates a temporary intermediate tab when clicking the link, that then launches the Duck Player video on a new tab. This intermediate tab has no URL, and therefore when the Duck Player tab is inserted via addAndSelectTab (which removes tabs with null URL as the first step), insertion fails because of a foreign key violation (sourceTabId doesn't exist anymore)

Steps to test this PR

Feature 1

  • Go to Feature flags inventory and make sure both tabInsertionFixes and swipingTabs are enabled
  • Restart the app
  • Set Duck Player to "Always"
  • Open this X post: https://x.com/tdinh_me/status/1909223312053657730
  • Click on the YouTube link shared in the X post
  • Check the video is opened in a new tab

Feature 2

  • Go to Feature flags inventory and make sure both tabInsertionFixes is enabled and swipingTabs is disabled
  • Restart the app
  • Set Duck Player to "Always"
  • Open this X post: https://x.com/tdinh_me/status/1909223312053657730
  • Click on the YouTube link shared in the X post
  • Check the video is opened in a new tab

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@CrisBarreiro CrisBarreiro requested a review from 0nko June 4, 2025 12:04
@CrisBarreiro CrisBarreiro marked this pull request as ready for review June 4, 2025 14:27
@0nko 0nko self-assigned this Jun 4, 2025
Copy link
Member

@0nko 0nko left a comment

Choose a reason for hiding this comment

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

I've tested both flows and both create new tabs, however, I've noticed a couple of things:

  1. I know this is not directly the result of this PR, but it's now a consequence of opening the video in a new tab -- when pasted the "x.com" link and then clicked on the video, this did not trigger the normal webview preview update of the previous tab (see screenshot at the top left tab).

image

  1. When I tested the 2nd flow without tab swiping, I noticed that clicking on the video actually opened 2 new tabs, one with the video and another empty tab with "t.co" URL (bottom left in the 2nd screenshot). Do you know why that might be the case?

image

image

Anyway, I'm pre-approving this and you can decide what to do. Nice work! 🥇

@CrisBarreiro CrisBarreiro force-pushed the fix/cris/tab-insertion-errors branch from 8755954 to f550f20 Compare June 5, 2025 13:21
@CrisBarreiro
Copy link
Contributor Author

@0nko, first issue is fixed

@CrisBarreiro CrisBarreiro merged commit 44f8f44 into develop Jun 6, 2025
6 checks passed
@CrisBarreiro CrisBarreiro deleted the fix/cris/tab-insertion-errors branch June 6, 2025 12:07
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