diff --git a/apps/admin/src/layout/editor-sidebar.acceptance.test.tsx b/apps/admin/src/layout/editor-sidebar.acceptance.test.tsx index 13fc01ad132..3c70f55ab70 100644 --- a/apps/admin/src/layout/editor-sidebar.acceptance.test.tsx +++ b/apps/admin/src/layout/editor-sidebar.acceptance.test.tsx @@ -20,16 +20,18 @@ import { editorScreen } from '@/editor/editor.screen'; * on an already-active route, where `activate()` does not run again. The * sidebar came back from the second post onwards. * - * React decides it from the route instead, which does not care how many times - * you have been there. + * React decides it from the route whenever it owns either side of the + * navigation, which does not care how many times you have been there. When + * both screens are Ember-owned, the route yields to Ember's ui service so it + * can reveal the sidebar before React's URL match catches up with the + * transition. * * These tests pin the route's own decision. They cannot prove the *original* * bug is gone: there is no Ember in this harness, so `useSidebarVisibility` * returns its default and the Ember half of the handshake is never exercised. - * What they guarantee is that React hides the sidebar on the editor route - * regardless of what Ember reports — which is the property the fix relies on. - * The sequence that produced the bug (list -> editor -> list -> editor) was - * verified by hand against a real Ghost. + * What they guarantee is that React can keep the sidebar hidden on the editor + * route when it owns part of the navigation. The real cross-router transitions + * still need verification against a running Ghost. */ describe('Editor chrome', () => { const sidebar = () => page.getByTestId('admin-sidebar'); diff --git a/apps/admin/src/layout/sidebar-visibility.test.tsx b/apps/admin/src/layout/sidebar-visibility.test.tsx index f62f0649ad0..b1139fe57de 100644 --- a/apps/admin/src/layout/sidebar-visibility.test.tsx +++ b/apps/admin/src/layout/sidebar-visibility.test.tsx @@ -7,6 +7,7 @@ type RouteMatch = { const useMatchesMock = vi.fn<() => RouteMatch[]>(); const useEmberSidebarVisibilityMock = vi.fn<() => boolean>(); +const useFlagGatedRouteOwnerMock = vi.fn<(flag: string) => 'react' | 'ember' | 'pending'>(); vi.mock('@tryghost/admin-x-framework', () => ({ useMatches: () => useMatchesMock(), @@ -16,10 +17,15 @@ vi.mock('@/ember-bridge', () => ({ useSidebarVisibility: () => useEmberSidebarVisibilityMock(), })); +vi.mock('@/use-flag-gated-route-owner', () => ({ + useFlagGatedRouteOwner: (flag: string) => useFlagGatedRouteOwnerMock(flag), +})); + describe('useAdminSidebarVisibility', () => { beforeEach(() => { useMatchesMock.mockReturnValue([]); useEmberSidebarVisibilityMock.mockReturnValue(true); + useFlagGatedRouteOwnerMock.mockReturnValue('ember'); }); it('uses the Ember sidebar visibility by default', async () => { @@ -42,6 +48,23 @@ describe('useAdminSidebarVisibility', () => { expect(result.current).toBe(false); }); + it('lets Ember reveal the sidebar while leaving an Ember-owned editor for an Ember list', async () => { + const { useAdminSidebarVisibility } = await import('./sidebar-visibility'); + + useMatchesMock.mockReturnValue([ + { + handle: { + allowEmberSidebarControl: true, + hideAdminSidebar: true, + }, + }, + ]); + + const { result } = renderHook(() => useAdminSidebarVisibility()); + + expect(result.current).toBe(true); + }); + it('keeps the sidebar visible when no matched route opts out', async () => { const { useAdminSidebarVisibility } = await import('./sidebar-visibility'); diff --git a/apps/admin/src/layout/sidebar-visibility.ts b/apps/admin/src/layout/sidebar-visibility.ts index d69a7541b9e..9877fcca04a 100644 --- a/apps/admin/src/layout/sidebar-visibility.ts +++ b/apps/admin/src/layout/sidebar-visibility.ts @@ -1,5 +1,10 @@ import { type AdminRouteHandle, useMatches } from '@tryghost/admin-x-framework'; import { useSidebarVisibility as useEmberSidebarVisibility } from '@/ember-bridge'; +import { useFlagGatedRouteOwner } from '@/use-flag-gated-route-owner'; + +export type SidebarRouteHandle = AdminRouteHandle & { + allowEmberSidebarControl?: boolean; +}; function hidesAdminSidebar(handle: unknown): handle is AdminRouteHandle { return ( @@ -10,11 +15,33 @@ function hidesAdminSidebar(handle: unknown): handle is AdminRouteHandle { ); } +function allowsEmberSidebarControl(handle: unknown): handle is SidebarRouteHandle { + return ( + typeof handle === 'object' && + handle !== null && + 'allowEmberSidebarControl' in handle && + handle.allowEmberSidebarControl === true + ); +} + export function useAdminSidebarVisibility(): boolean { const emberSidebarVisible = useEmberSidebarVisibility(); + const editorOwner = useFlagGatedRouteOwner('editorReact'); + const postsListOwner = useFlagGatedRouteOwner('postsListReact'); const matches = useMatches(); - const routeHidesSidebar = matches.some((match) => hidesAdminSidebar(match.handle)); + // When both sides of the editor -> list transition are Ember-owned, Ember's + // ui service has the earliest knowledge of the destination. React still + // matches /editor/* while the list model is loading, so its route handle + // must yield to Ember or the sidebar appears only after the URL settles. + const emberControlsEditorNavigation = editorOwner === 'ember' && postsListOwner === 'ember'; + const routeHidesSidebar = matches.some((match) => { + if (!hidesAdminSidebar(match.handle)) { + return false; + } + + return !(allowsEmberSidebarControl(match.handle) && emberControlsEditorNavigation); + }); return emberSidebarVisible && !routeHidesSidebar; } diff --git a/apps/admin/src/routes.tsx b/apps/admin/src/routes.tsx index b683f07c076..6c59c5de95e 100644 --- a/apps/admin/src/routes.tsx +++ b/apps/admin/src/routes.tsx @@ -31,6 +31,7 @@ import { OnboardingRedirect, lazyOnboardingScreen } from './onboarding/api'; import { lazyPostAnalyticsRoot, postAnalyticsRouteChildren } from './posts/api'; import { canAccessSettingsRoute, lazySettingsScreen, settingsRouteChildren } from './settings/api'; import { lazyTagsScreen } from './tags/api'; +import { type SidebarRouteHandle } from './layout/sidebar-visibility'; import { canManageAutomations, canManageMembers, @@ -185,12 +186,17 @@ const appRoutes: RouteObject[] = [ // route aborts its transition, so the editor route never deactivates, // and a second visit is a model change on an already-active route where // `activate()` does not run again. The sidebar came back from the second - // post onwards. Deciding it from the route handle makes React the - // authority, removes the cross-implementation handshake, and applies to - // both sides of the flag. + // post onwards. The route handle therefore stays authoritative whenever + // React owns either side of the navigation. When both the editor and list + // are Ember-owned, it yields to Ember so the sidebar can return as soon as + // the list transition starts, before React's URL match leaves /editor/*. path: '/editor/*', Component: EditorGate, - handle: { ...emberFallbackHandle, hideAdminSidebar: true } satisfies AdminRouteHandle, + handle: { + ...emberFallbackHandle, + allowEmberSidebarControl: true, + hideAdminSidebar: true, + } satisfies SidebarRouteHandle, }, // Ember-handled routes ...emberFallbackRoutes, diff --git a/apps/ember-admin/app/routes/lexical-editor.js b/apps/ember-admin/app/routes/lexical-editor.js index 2e7a4311a52..e5ed39721c3 100644 --- a/apps/ember-admin/app/routes/lexical-editor.js +++ b/apps/ember-admin/app/routes/lexical-editor.js @@ -69,15 +69,18 @@ export default AuthenticatedRoute.extend({ classNames: ['editor'], - // React owns /editor/* when the flag is on. Aborting keeps the Ember - // editor subtree unrendered and skips `activate()`, so full-screen state - // is never set for a screen nobody sees. + // React owns /editor/* when the flag is on. When Ember owns it, claim + // full-screen mode before the editor model loads so the React shell never + // renders its sidebar during the loading state. Aborting the React-owned + // path keeps the Ember subtree unrendered and skips both this state change + // and `activate()`. beforeModel(transition) { this._super(...arguments); // Strictly boolean: a non-boolean labs value must not hand the route // to React. if (this.feature.editorReact !== true) { + this.ui.set('isFullScreen', true); return; } diff --git a/apps/ember-admin/app/routes/posts.js b/apps/ember-admin/app/routes/posts.js index 19052b2ea73..df13768f4b3 100644 --- a/apps/ember-admin/app/routes/posts.js +++ b/apps/ember-admin/app/routes/posts.js @@ -78,6 +78,13 @@ export default class PostsRoute extends AuthenticatedRoute { beforeModel(transition) { super.beforeModel(...arguments); + // The editor stays active while Ember resolves the list model, so its + // deactivate hook has not cleared full-screen mode when the posts/pages + // loading template first renders. Restore normal chrome at the start of + // the transition so the React shell keeps its sidebar visible throughout + // the loading state. PagesRoute inherits this hook. + this.ui.set('isFullScreen', false); + // Strictly boolean, matching the tag route: a non-boolean labs value // must not hand the route to React. if (this.feature.postsListReact !== true) { @@ -86,13 +93,6 @@ export default class PostsRoute extends AuthenticatedRoute { transition.abort(); - // Aborting means the route we came FROM never deactivates, so any UI - // state its teardown would have cleared stays set. The editor's - // `deactivate` clears full-screen mode, and the React shell reads that - // to decide whether to show the sidebar - so without this, returning - // from the editor leaves you looking at a sidebar-less screen. - this.ui.set('isFullScreen', false); - // Ember and React share window.location.hash, and an aborted // transition never reaches updateURL - so a navigation Ember itself // started would be a silent no-op without writing the URL ourselves. diff --git a/apps/ember-admin/tests/unit/routes/lexical-editor-test.js b/apps/ember-admin/tests/unit/routes/lexical-editor-test.js new file mode 100644 index 00000000000..013137aab47 --- /dev/null +++ b/apps/ember-admin/tests/unit/routes/lexical-editor-test.js @@ -0,0 +1,41 @@ +import Service from '@ember/service'; +import sinon from 'sinon'; +import {afterEach, describe, it} from 'mocha'; +import {expect} from 'chai'; +import {setupTest} from 'ember-mocha'; + +describe('Unit: Route: lexical-editor', function () { + setupTest(); + + afterEach(function () { + sinon.restore(); + }); + + function setupRoute(owner) { + class SessionStub extends Service { + isAuthenticated = true; + requireAuthentication = sinon.spy(); + } + class FeatureStub extends Service { + editorReact = false; + } + owner.register('service:session', SessionStub); + owner.register('service:feature', FeatureStub); + + const route = owner.lookup('route:lexical-editor'); + const ui = owner.lookup('service:ui'); + sinon.spy(ui, 'set'); + + return {route, ui}; + } + + it('hides the sidebar before loading the Ember editor', function () { + const {route, ui} = setupRoute(this.owner); + const transition = {intent: {url: '/editor/post/1'}}; + + route.beforeModel(transition); + + expect(ui.isFullScreen, 'full-screen mode enabled before model loading').to.be.true; + expect(ui.set.calledOnceWith('isFullScreen', true), 'full-screen mode enabled once').to.be.true; + }); +}); diff --git a/apps/ember-admin/tests/unit/routes/posts-test.js b/apps/ember-admin/tests/unit/routes/posts-test.js index c7d440dfcec..1188b332f34 100644 --- a/apps/ember-admin/tests/unit/routes/posts-test.js +++ b/apps/ember-admin/tests/unit/routes/posts-test.js @@ -71,4 +71,18 @@ describe('Unit: Route: posts', function () { expect(transition.abort.called, 'transition not aborted').to.be.false; expect(router.replaceWith.called, 'no parking').to.be.false; }); + + it('restores the sidebar before loading the Ember posts list', function () { + const {route, ui} = setupRoute(this.owner, {flagValue: false}); + const transition = {abort: sinon.spy(), intent: {url: '/posts'}}; + + ui.set('isFullScreen', true); + ui.set.resetHistory(); + + route.beforeModel(transition); + + expect(transition.abort.called, 'transition not aborted').to.be.false; + expect(ui.isFullScreen, 'full-screen mode cleared before model loading').to.be.false; + expect(ui.set.calledOnceWith('isFullScreen', false), 'full-screen reset once').to.be.true; + }); });