-
Notifications
You must be signed in to change notification settings - Fork 57
Update tab selection logic after site deletion #2468
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
base: trunk
Are you sure you want to change the base?
Update tab selection logic after site deletion #2468
Conversation
- Update the logic to reset the selected tab to 'overview' only if the currently selected site no longer exists and a selection change has occurred. - Improve clarity in comments regarding selection changes and tab resets.
📊 Performance Test ResultsComparing 27d6e4d vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
sejas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it mostly worked, the only issue is that my selected site changed from one existing site to another automatically.
Steps to reproduce:
- Delete site 3
- Quickly switch to site 2
- Observe that Studio switches automatically to the first site instead of keeping you in the existing site 2.
Here is a recording:
test-deleting-sites-and-tabs.mp4
sejas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sejas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected! Great work!
src/components/site-content-tabs.tsx
Outdated
| // Track previous site ID in state so we can safely access it during render | ||
| const [ prevSiteId, setPrevSiteId ] = useState( selectedSite?.id ); | ||
|
|
||
| // Compute effective tab at render time | ||
| // This ensures TabPanel gets the correct initialTabName on the first render after deletion | ||
| const siteChanged = prevSiteId !== selectedSite?.id; | ||
| const prevSiteWasDeleted = prevSiteId && ! sites.some( ( site ) => site.id === prevSiteId ); | ||
| const effectiveTab = siteChanged && prevSiteWasDeleted ? 'overview' : selectedTab; | ||
|
|
||
| // Update prevSiteId selectedTab after render | ||
| useEffect( () => { | ||
| if ( siteChanged ) { | ||
| setPrevSiteId( selectedSite?.id ); | ||
| } | ||
|
|
||
| if ( effectiveTab !== selectedTab ) { | ||
| setSelectedTab( effectiveTab as TabName ); | ||
| } | ||
| }, [ siteChanged, selectedSite?.id, effectiveTab, selectedTab, setSelectedTab ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could isolate these new hooks into separate custom hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
epeicher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gcsecsey for solving this! I have tested it, and it works as expected. The site selected and the tab selected remains after the site deletion completes. I have also confirmed that if no other sites are selected, the first site in the Overview tab is opened. Changes also LGTM! ![]()
Related issues
Proposed Changes
When the deleted site was the selected site, the tab should reset to "Overview". However, calling
setSelectedTab('overview')from the site details hook didn't work reliably. TheContentTabsProviderwrapsSiteDetailsProvider, so when both states update, the site change renders the component before the tab update takes effect, causing the TabPanel to remount with the old tab value.This PR moves the state update call from the
onDeleteSitecallback toSiteContentTabs, and adds logic to compute aneffectiveTabvalue at render time. The component tracks the previous site ID in state and, during render, checks if the site changed AND the previous site was deleted. If both conditions are true, it uses'overview'as the effective tab forinitialTabName. A useEffect then syncs this back to the tab context.Testing Instructions
npm startCleanShot.2026-01-26.at.11.09.23.mp4
Pre-merge Checklist