gh-12740: remove deleted workspace tabs from switch-to-tab by Vedaant-Rajoo · Pull Request #12794 · zen-browser/desktop

Vedaant-Rajoo · 2026-03-17T03:35:46Z

Summary

Fixes #12740 by removing workspace-owned tabs through the normal tab close path when a
workspace is deleted.

Previously, deleting a workspace only removed the workspace metadata/UI. Tabs that
still belonged to that workspace could remain alive in the browser tab model, which
allowed stale Switch Tab results to continue appearing in the urlbar.

What changed

close workspace-owned tabs before removing workspace metadata
include both regular and pinned workspace tabs
exclude essentials and empty tabs from workspace deletion
clear Zen's stored tab cache after deletion
add/update workspace tests around deletion and unload behavior

Why this approach

The stale urlbar result was a symptom of the tabs not actually being closed. The safest
fix is to delete those tabs through the normal gBrowser.removeTabs(...) path so the
usual browser and Zen cleanup runs, instead of trying to manually refresh urlbar state
afterward.

Notes

essentials are intentionally not deleted because they are not workspace-owned
the delete regression test passes
while working on this, some older unload tests also needed cleanup because they were
creating unrealistic workspace state by mutating attributes directly

Short maintainer note:
The core reasoning here is that Switch Tab was not primarily a urlbar caching bug. The
deleted workspace tabs were surviving the workspace deletion flow, so the correct fix
point was the workspace delete path itself. Closing those tabs normally is lower risk
than adding special-case invalidation around urlbar providers.

mr-cheffy · 2026-03-17T07:15:14Z

src/zen/spaces/ZenSpaceManager.mjs

      workspace => workspace.uuid !== windowID
    );
    this.#propagateWorkspaceData(workspacesData);
+    this._allStoredTabs = null;


Not needed?

line 1271 is not redundant in all cases
the redundant one is more likely the clear inside #deleteWorkspaceOwnedTabs(), not the one in removeWorkspace()

These are some cases where i found it useful:

The workspace is deleted but it has no workspace-owned tabs left.
In that case #deleteWorkspaceOwnedTabs() may do nothing, so no tab-close-driven cache invalidation happens. But allStoredTabs is still stale because the workspace container set changed.

The workspace only had tabs that are excluded from deletion logic.
Example: no normal workspace-owned tabs, only essentials/empty-tab edge cases. The workspace still disappears from the workspace UI/container model, so the cached tab walk can still be stale.

You move tab deletion out of removeWorkspace() or it happens later/asynchronously.
Then line 1271 becomes the only immediate invalidation at the time workspace data is changed.

Why this matters:

allStoredTabs is built by walking workspace containers, not just gBrowser.tabs.

removeWorkspace() changes workspace data and leads to workspace container removal.

If no lower-level tab invalidation fires, the memoized cache can point at the old container-derived state.

So:

if real tab removal definitely happens first, line 1271 may be redundant

if workspace metadata/UI can change without guaranteed tab-removal invalidation, line 1271 is still useful

Please use delete then

Sorry, better to use gBrowser.tabContainer._invalidateCachedVisibleTabs();

I tracked down why this._allStoredTabs = null originally looked like the fix.

What made it confusing is that there are actually two delete paths with different behavior:

deleting the currently active workspace

deleting a different workspace while staying in the current one

Deleting the active workspace worked because the removal code was using gBrowser.tabs, and in Zen that effectively reflects the active workspace strip. So the tabs from the workspace being deleted were present in that enumeration and got removed.

Deleting a different workspace failed because its tabs were not part of that same gBrowser.tabs view, so #deleteWorkspaceOwnedTabs() could miss them entirely. That is why stale Switch to Tab entries remained.

I initially thought this._allStoredTabs = null was the right fix because clearing that cache made the stale results disappear in practice. After tracing it further, the real issue was not the open-tabs/urlbar unregister path, and not moz_openpages_temp either; those were being cleaned up correctly on workspace deletion. The actual problem was that the delete path was enumerating tabs from an active-workspace-scoped source.

So the fix here is to use allStoredTabs for workspace-owned tab deletion, since that walks tabs across workspace containers instead of only what gBrowser.tabs currently exposes for the active space. I also updated the test to cover deleting a workspace from another workspace, since that was the failing branch.

src/zen/spaces/ZenSpaceManager.mjs

…nager

mr-cheffy · 2026-03-19T10:39:53Z

src/zen/spaces/ZenSpaceManager.mjs

      workspace => workspace.uuid !== windowID
    );
    this.#propagateWorkspaceData(workspacesData);
+    this._allStoredTabs = null;


Please use delete then

mr-cheffy · 2026-03-19T10:42:19Z

src/zen/tests/workspaces/browser_workspace_unload.js

+  removeNonWorkspaceTabs();
+});
+
+add_task(async function test_DeleteWorkspace_RemovesWorkspaceOwnedTabs() {


This test is a bit useless, i'd just check if TabRemove event is being fired for every tab and TabGroupRemoved

Updated the test to cover the actual removal lifecycle instead of only checking the final tab state. It now waits for the relevant TabClose events and TabGroupRemoved, then asserts the workspace-owned tabs are gone. The updated coverage is in browser_workspace_unload.js.

mr-cheffy · 2026-03-19T10:43:03Z

src/zen/spaces/ZenSpaceManager.mjs

+      tab =>
+        tab.getAttribute("zen-workspace-id") === workspaceID &&
+        !tab.hasAttribute("zen-essential") &&
+        !tab.hasAttribute("zen-empty-tab")


!tab.hasAttribute("zen-empty-tab") not needed here AFAIK. At least i'd do (!tab.hasAttribute("zen-empty-tab") && !tab.group) so it also removes empty tabs of the folders and therefor, remove the folder its self

Fixed this by handling workspace folders explicitly during workspace deletion instead of trying to fold zen-empty-tab handling into the normal tab filter. removeWorkspace() now deletes workspace folders first, which also clears the folder empty-tab path, and then removes the remaining workspace-owned tabs. I also extended the test to cover a folder case in browser_workspace_unload.js. The deletion change is in ZenSpaceManager.mjs.

Important
I verified the workspace-deletion change in isolation and the updated deletion test passes.

I still couldn’t get a clean full-file run, but this does not look introduced by this PR: the same file already fails on a full origin/dev checkout, and it fails more broadly there. The main issue is unload tests not fully unloading tabs (pending stays false / linkedPanel still exists), which then leaves unexpected tabs behind at the end of the run.

Because of that, I used a temporary local .only() only to confirm the new deletion behavior independently, then removed it afterward.

src/zen/spaces/ZenSpaceManager.mjs

mr-cheffy · 2026-03-20T15:39:12Z

src/zen/spaces/ZenSpaceManager.mjs

      workspace => workspace.uuid !== windowID
    );
    this.#propagateWorkspaceData(workspacesData);
+    this._allStoredTabs = null;


Sorry, better to use gBrowser.tabContainer._invalidateCachedVisibleTabs();

…kspace folders and improve workspace switching tests

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Vedaant Rajoo <vedaant12345@gmail.com>

mr-cheffy · 2026-03-22T15:33:33Z

You haven't ran the tests you created, have you?

12740: remove deleted workspace tabs from switch-to-tab

b89535b

Vedaant-Rajoo requested a review from mr-cheffy as a code owner March 17, 2026 03:35

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Mar 17, 2026

Merge branch 'dev' into fix-issue-12740

6e133e1

dosubot bot added the Bug label Mar 17, 2026

mr-cheffy requested changes Mar 17, 2026

View reviewed changes

fix: Remove unnecessary null assignment for stored tabs in ZenSpaceMa…

5dca4f5

…nager

mr-cheffy requested changes Mar 19, 2026

View reviewed changes

zen-browsergh-12740: implement folder deletion and fix tests

3f908da

mr-cheffy requested changes Mar 20, 2026

View reviewed changes

src/zen/spaces/ZenSpaceManager.mjs Outdated Show resolved Hide resolved

mr-cheffy requested changes Mar 20, 2026

View reviewed changes

zen-browsergh-12740: Update ZenSpaceManager to use tab groups for wor…

832cd50

…kspace folders and improve workspace switching tests

mr-cheffy linked an issue Mar 21, 2026 that may be closed by this pull request

Tabs are lost then a workspace deletion is synced #10345

Closed

5 tasks

mr-cheffy changed the title ~~12740: remove deleted workspace tabs from switch-to-tab~~ gh-12740: remove deleted workspace tabs from switch-to-tab Mar 21, 2026

mr-cheffy requested review from Copilot and mr-cheffy March 21, 2026 17:00

Copilot started reviewing on behalf of mr-cheffy March 21, 2026 17:01 View session

This comment was marked as resolved.

Sign in to view

Vedaant-Rajoo and others added 2 commits March 21, 2026 15:36

Update src/zen/tests/workspaces/browser_workspace_unload.js

6c3ad53

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Vedaant Rajoo <vedaant12345@gmail.com>

Merge branch 'dev' into fix-issue-12740

edc3457

no-bug: Clean up

964dc08

mr-cheffy requested a review from Copilot March 22, 2026 15:59

Copilot started reviewing on behalf of mr-cheffy March 22, 2026 15:59 View session

mr-cheffy approved these changes Mar 22, 2026

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 22, 2026

This comment was marked as resolved.

Sign in to view

mr-cheffy merged commit e80c16a into zen-browser:dev Mar 22, 2026
5 checks passed

Uh oh!

Conversation

Vedaant-Rajoo commented Mar 17, 2026

Summary

What changed

Why this approach

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vedaant-Rajoo Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

mr-cheffy commented Mar 22, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Vedaant-Rajoo Mar 20, 2026 •

edited

Loading