Repository navigation
fix(tabs): the tab context menu bulk closes and pin - #41
Merged
Merged
Conversation
…ork [tab-menu]
Two faults, both in what the menu's actions wrote rather than in the menu itself.
The hook refreshed its tab list only from a router location change, but every one of these actions
writes the list through URLTabSyncEngine.updateURL, which changes the URL with
history.replaceState - something React Router never sees. So closing the other tabs, or the tabs to
one side, changed the URL and left the tabs on screen: the action looked like it did nothing. Only
the actions that also navigated (close all but pinned, closing the active tab) appeared to work. The
hook now follows the engine's own notifications, which fixes the whole family at once - pin and
unpin were stale for the same reason. Close Other Tabs also navigates to the tab it kept, so the
page follows the tab that became active instead of showing one that is gone.
The pin item carried the icon's name as its label ('MapPinIcon Tab' - the same class of leak the tab
titles were fixed for) and, unlike every other item, left the menu open over the tab it had just
changed.
Tests: useTabsBulkClose.test.tsx drives the real hook over the real engine for the five closes; the
pin item's label and its menu closing are covered in TabLabelRendering. 264 tests pass across the
tab subsystem.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The tab context menu's bulk closes did nothing, and the pin item was labelled with an icon's name. Both are fixed here, each with the test that proves it.
Why the closes did nothing
useTabsrefreshed its tab list only when the router's location changed. But every one of these actions writes the list throughURLTabSyncEngine.updateURL, which changes the URL withhistory.replaceState— something React Router never sees. So:The hook now follows the engine's own notifications, which fixes the whole family at once. Pin and unpin were stale for exactly the same reason, which is why they are in this PR.
Close Other Tabs also navigates to the tab it kept, so the page follows the tab that became active instead of showing a tab that is gone — which is what you were doing by hand ("i cant close other tabs without navigating inside them").
The pin item
It carried the icon's name as its label —
MapPinIcon Tab— the same class of leak the tab titles were already fixed for, and unlike every other item it left the menu open over the tab it had just changed. It now reads Pin Tab / Unpin Tab and closes the menu.Verified
src/hooks/tabs/__tests__/useTabsBulkClose.test.tsx(new) drives the real hook over the real engine for the five closes, and mounts the three tabs the URL describes. Written first and watched fail: 4 of the 6 failed before the fix — the tab list simply did not change.TabLabelRendering.test.tsx, which already guarded against icon names leaking into labels.src/hooks/tabs,src/services/tabsandsrc/components/tabbedWorkspace.Not in this PR
Two things I noticed while working and deliberately did not chase here:
Duplicate TabandOpen in New Windowleave the menu open too. They act correctly; only the menu's dismissal is inconsistent. Same one-line shape as the pin fix if you want it.Index creation warning for app_settings: no such column: "[object Object]"on first run — a real defect in index creation, unrelated to tabs.