diff --git a/.changeset/techdocs-fifty-cameras-listen.md b/.changeset/techdocs-fifty-cameras-listen.md new file mode 100644 index 0000000000..fb6be9d709 --- /dev/null +++ b/.changeset/techdocs-fifty-cameras-listen.md @@ -0,0 +1,7 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Only update the `path` when the content is updated. +If content and path are updated independently, the frontend rendering is triggered twice on each navigation: Once for the `path` change (with the old content) and once for the new content. +This might result in a flickering rendering that is caused by the async frontend preprocessing, and the fact that replacing the shadow dom content is expensive. diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index f861641708..ce72da4bf6 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -61,18 +61,18 @@ const useStyles = makeStyles(() => ({ export const Reader = ({ entityId, onReady }: Props) => { const { kind, namespace, name } = entityId; - const { '*': path } = useParams(); const theme = useTheme(); const classes = useStyles(); const { state, + path, contentReload, content: rawPage, contentErrorMessage, syncErrorMessage, buildLog, - } = useReaderState(kind, namespace, name, path); + } = useReaderState(kind, namespace, name, useParams()['*']); const techdocsStorageApi = useApi(techdocsStorageApiRef); const [sidebars, setSidebars] = useState(); @@ -322,6 +322,10 @@ export const Reader = ({ entityId, onReady }: Props) => { useEffect(() => { if (!rawPage || !shadowDomRef.current) { + // clear the shadow dom if no content is available + if (shadowDomRef.current?.shadowRoot) { + shadowDomRef.current.shadowRoot.innerHTML = ''; + } return () => {}; } if (onReady) { diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index 9d9a8e2b1a..b0d4e0326a 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -86,7 +86,7 @@ describe('useReaderState', () => { }; it('should return a copy of the state', () => { - expect(reducer(oldState, { type: 'navigate', path: '/' })).toEqual({ + expect(reducer(oldState, { type: 'content', path: '/' })).toEqual({ activeSyncState: 'CHECKING', contentLoading: false, path: '/', @@ -102,13 +102,13 @@ describe('useReaderState', () => { }); it.each` - type | oldActiveSyncState | newActiveSyncState - ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'navigate'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'navigate'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'sync'} | ${'BUILD_READY'} | ${undefined} - ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined} + type | oldActiveSyncState | newActiveSyncState + ${'contentLoading'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'contentLoading'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'sync'} | ${'BUILD_READY'} | ${undefined /* undefined, because we don't set an input */} + ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined /* undefined, because we don't set an input */} `( 'should, when type=$type and activeSyncState=$oldActiveSyncState, set activeSyncState=$newActiveSyncState', ({ type, oldActiveSyncState, newActiveSyncState }) => { @@ -124,18 +124,45 @@ describe('useReaderState', () => { }, ); - describe('"content" action', () => { + describe('"contentLoading" action', () => { it('should set loading', () => { + expect( + reducer(oldState, { + type: 'contentLoading', + }), + ).toEqual({ + ...oldState, + contentLoading: true, + }); + }); + + it('should keep content', () => { expect( reducer( { ...oldState, content: 'some-old-content', + }, + { + type: 'contentLoading', + }, + ), + ).toEqual({ + ...oldState, + contentLoading: true, + content: 'some-old-content', + }); + }); + + it('should reset errors', () => { + expect( + reducer( + { + ...oldState, contentError: new Error(), }, { - type: 'content', - contentLoading: true, + type: 'contentLoading', }, ), ).toEqual({ @@ -143,7 +170,9 @@ describe('useReaderState', () => { contentLoading: true, }); }); + }); + describe('"content" action', () => { it('should set content', () => { expect( reducer( @@ -164,6 +193,27 @@ describe('useReaderState', () => { }); }); + it('should set content and update path', () => { + expect( + reducer( + { + ...oldState, + contentLoading: true, + }, + { + type: 'content', + content: 'asdf', + path: '/new-path', + }, + ), + ).toEqual({ + ...oldState, + contentLoading: false, + content: 'asdf', + path: '/new-path', + }); + }); + it('should set error', () => { expect( reducer( @@ -185,20 +235,6 @@ describe('useReaderState', () => { }); }); - describe('"navigate" action', () => { - it('should work', () => { - expect( - reducer(oldState, { - type: 'navigate', - path: '/', - }), - ).toEqual({ - ...oldState, - path: '/', - }); - }); - }); - describe('"sync" action', () => { it('should update state', () => { expect( @@ -256,6 +292,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -267,6 +304,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -313,6 +351,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -324,6 +363,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'INITIAL_BUILD', + path: '/example', content: undefined, contentErrorMessage: 'NotFoundError: Page Not Found', syncErrorMessage: undefined, @@ -335,6 +375,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -346,6 +387,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -394,6 +436,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -405,6 +448,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -416,6 +460,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_STALE_REFRESHING', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -427,6 +472,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_STALE_READY', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -441,7 +487,8 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CHECKING', - content: undefined, + path: '/example', + content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, buildLog: [], @@ -452,6 +499,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my new content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -475,6 +523,103 @@ describe('useReaderState', () => { }); }); + it('should handle navigation', async () => { + techdocsStorageApi.getEntityDocs + .mockResolvedValueOnce('my content') + .mockImplementationOnce(async () => { + await new Promise(resolve => setTimeout(resolve, 1100)); + return 'my new content'; + }) + .mockRejectedValueOnce(new NotFoundError('Some error description')); + techdocsStorageApi.syncEntityDocs.mockResolvedValue('cached'); + + await act(async () => { + const { result, waitForValueToChange, rerender } = await renderHook( + ({ path }: { path: string }) => + useReaderState('Component', 'default', 'backstage', path), + { initialProps: { path: '/example' }, wrapper: Wrapper as any }, + ); + + expect(result.current).toEqual({ + state: 'CHECKING', + path: '/example', + content: undefined, + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // show the content + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + path: '/example', + content: 'my content', + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // navigate + rerender({ path: '/new' }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CHECKING', + path: '/example', + content: 'my content', + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + path: '/new', + content: 'my new content', + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // navigate + rerender({ path: '/missing' }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_NOT_FOUND', + path: '/missing', + content: undefined, + contentErrorMessage: 'NotFoundError: Some error description', + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/new', + ); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'backstage', + }, + expect.any(Function), + ); + }); + }); + it('should handle content error', async () => { techdocsStorageApi.getEntityDocs.mockRejectedValue( new NotFoundError('Some error description'), @@ -489,6 +634,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -500,6 +646,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_NOT_FOUND', + path: '/example', content: undefined, contentErrorMessage: 'NotFoundError: Some error description', syncErrorMessage: undefined, diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index 178c0746bf..07bd338de9 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -15,7 +15,7 @@ */ import { useApi } from '@backstage/core-plugin-api'; -import { useEffect, useMemo, useReducer, useRef } from 'react'; +import { useMemo, useReducer, useRef } from 'react'; import { useAsync, useAsyncRetry } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; @@ -131,13 +131,13 @@ type ReducerActions = state: SyncStates; syncError?: Error; } + | { type: 'contentLoading' } | { type: 'content'; + path?: string; content?: string; - contentLoading?: true; contentError?: Error; } - | { type: 'navigate'; path: string } | { type: 'buildLog'; log: string }; type ReducerState = { @@ -186,14 +186,22 @@ export function reducer( newState.syncError = action.syncError; break; - case 'content': - newState.content = action.content; - newState.contentLoading = action.contentLoading ?? false; - newState.contentError = action.contentError; + case 'contentLoading': + newState.contentLoading = true; + + // only reset errors but keep the old content until it is replaced by the 'content' action + newState.contentError = undefined; break; - case 'navigate': - newState.path = action.path; + case 'content': + // only override the path if it is part of the action + if (typeof action.path === 'string') { + newState.path = action.path; + } + + newState.contentLoading = false; + newState.content = action.content; + newState.contentError = action.contentError; break; case 'buildLog': @@ -204,10 +212,10 @@ export function reducer( throw new Error(); } - // a navigation or a content update loads fresh content so the build is updated to being up-to-date + // a content update loads fresh content so the build is updated to being up-to-date if ( ['BUILD_READY', 'BUILD_READY_RELOAD'].includes(newState.activeSyncState) && - ['content', 'navigate'].includes(action.type) + ['contentLoading', 'content'].includes(action.type) ) { newState.activeSyncState = 'UP_TO_DATE'; newState.buildLog = []; @@ -223,6 +231,7 @@ export function useReaderState( path: string, ): { state: ContentStateTypes; + path: string; contentReload: () => void; content?: string; contentErrorMessage?: string; @@ -238,14 +247,9 @@ export function useReaderState( const techdocsStorageApi = useApi(techdocsStorageApiRef); - // convert all path changes into actions - useEffect(() => { - dispatch({ type: 'navigate', path }); - }, [path]); - // try to load the content. the function will fire events and we don't care for the return values const { retry: contentReload } = useAsyncRetry(async () => { - dispatch({ type: 'content', contentLoading: true }); + dispatch({ type: 'contentLoading' }); try { const entityDocs = await techdocsStorageApi.getEntityDocs( @@ -253,11 +257,12 @@ export function useReaderState( path, ); - dispatch({ type: 'content', content: entityDocs }); + // update content and path at the same time + dispatch({ type: 'content', content: entityDocs, path }); return entityDocs; } catch (e) { - dispatch({ type: 'content', contentError: e }); + dispatch({ type: 'content', contentError: e, path }); } return undefined; @@ -335,6 +340,7 @@ export function useReaderState( return { state: displayState, contentReload, + path: state.path, content: state.content, contentErrorMessage: state.contentError?.toString(), syncErrorMessage: state.syncError?.toString(),