diff --git a/zeppelin-web-angular/e2e/core-contract/README.md b/zeppelin-web-angular/e2e/core-contract/README.md index dcdfb350b37..240fd29aaa8 100644 --- a/zeppelin-web-angular/e2e/core-contract/README.md +++ b/zeppelin-web-angular/e2e/core-contract/README.md @@ -359,6 +359,26 @@ reconnect recovery remains outside it until the lifecycle rules have an enforcea stale-reply mechanism. The harness does not implement those lifecycle rules, switch the production renderer, or move production notebook state out of Angular. +## Private React read view + +`#/notebook/?notebookCoreRead¬ebookCoreReact` mounts the React read view (`./NotebookReadSkeleton`) +on the private read route (ZEPPELIN-6727). `NotebookCoreEntryComponent` owns the route's read host and +passes the remote its `NotebookReadPort`, the `paragraph` query parameter, `onMounted` and `onError`. +The entry's `data-renderer` describes the mount, never whether the note is loaded: + +| `data-renderer` | Meaning | +| ------------------ | --------------------------------------------------------------------------------------------- | +| `angular` | No `notebookCoreReact`: the Angular read view. | +| `react-loading` | The remote is loading or has not committed its first render. | +| `react-mounted` | The React view committed its first render. | +| `angular-fallback` | The remote did not load (including the loader timeout) or its first render threw. The Angular read view takes over on the same host without reading the note again. | +| `runtime-error` | The React view failed after its first render. The failure is only reported; the view stays empty. | + +The renderer is chosen once on entry. Dropping `notebookCoreRead` leaves the private route and loads the +editable notebook afresh. The fallback keeps `paragraph` in the URL but does not scroll to it. +`notebookCoreReactFailure=mount|after-mount` makes the React view throw for tests; the entry ignores it +without `notebookCoreReact`. `e2e/tests/notebook/core-read/notebook-core-entry.spec.ts` covers these paths. + ## Private Core read view captures [notebook-read](../fixtures/notebook-read/) holds the server replies read by the private Core read diff --git a/zeppelin-web-angular/e2e/models/notebook-core-read-page.ts b/zeppelin-web-angular/e2e/models/notebook-core-read-page.ts index cb324084e5f..4f585e5c505 100644 --- a/zeppelin-web-angular/e2e/models/notebook-core-read-page.ts +++ b/zeppelin-web-angular/e2e/models/notebook-core-read-page.ts @@ -15,6 +15,12 @@ import { BasePage } from './base-page'; const NOTEBOOK_CORE_READ_QUERY = 'notebookCoreRead'; +/** The private read route once per renderer, with the `data-renderer` its entry settles on. */ +export const NOTEBOOK_CORE_READ_BRANCHES = [ + { label: 'Angular', query: '', renderer: 'angular' }, + { label: 'React', query: 'notebookCoreReact', renderer: 'react-mounted' } +] as const; + export class NotebookCoreReadPage extends BasePage { readonly view: Locator; readonly title: Locator; @@ -26,6 +32,9 @@ export class NotebookCoreReadPage extends BasePage { readonly error: Locator; readonly revisionBadge: Locator; readonly editableNotebook: Locator; + readonly editableParagraphs: Locator; + readonly entry: Locator; + readonly skeleton: Locator; constructor(page: Page) { super(page); @@ -39,6 +48,23 @@ export class NotebookCoreReadPage extends BasePage { this.error = page.getByTestId('notebook-core-read-error'); this.revisionBadge = page.getByTestId('notebook-core-read-revision'); this.editableNotebook = page.locator('zeppelin-notebook'); + this.editableParagraphs = page.locator('zeppelin-notebook-paragraph'); + this.entry = page.getByTestId('notebook-core-entry'); + this.skeleton = page.getByTestId('notebook-read-skeleton'); + } + + /** The read status as either renderer shows it. */ + readStatus(status: string): Locator { + return this.view + .and(this.page.locator(`[data-status="${status}"]`)) + .or(this.skeleton.locator(`[data-read-status="${status}"]`)); + } + + /** A note or revision id as either renderer shows it. */ + shows(id: string): Locator { + return this.view + .and(this.page.locator(`[data-note-id="${id}"], [data-revision-id="${id}"]`)) + .or(this.skeleton.getByText(id, { exact: true })); } /** The rendered chart of a saved TABLE result; G2 draws into a canvas. */ @@ -68,12 +94,12 @@ export class NotebookCoreReadPage extends BasePage { } /** Changes only the hash, so the Shell and its socket stay up. */ - async open(noteId: string, revisionId?: string): Promise { + async open(noteId: string, revisionId?: string, query = ''): Promise { const path = revisionId ? `/notebook/${noteId}/revision/${revisionId}` : `/notebook/${noteId}`; - await this.page.goto(`/#${path}?${NOTEBOOK_CORE_READ_QUERY}`); + await this.page.goto(`/#${path}?${[NOTEBOOK_CORE_READ_QUERY, query].filter(Boolean).join('&')}`); } - async openEditable(noteId: string): Promise { - await this.page.goto(`/#/notebook/${noteId}`); + async openEditable(noteId: string, query = ''): Promise { + await this.page.goto(`/#/notebook/${noteId}${query ? `?${query}` : ''}`); } } diff --git a/zeppelin-web-angular/e2e/tests/notebook/core-read/notebook-core-entry.spec.ts b/zeppelin-web-angular/e2e/tests/notebook/core-read/notebook-core-entry.spec.ts new file mode 100644 index 00000000000..d343cd08220 --- /dev/null +++ b/zeppelin-web-angular/e2e/tests/notebook/core-read/notebook-core-entry.spec.ts @@ -0,0 +1,272 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect, test, Route } from '@playwright/test'; + +import { NotebookCoreReadPage, NOTEBOOK_CORE_READ_BRANCHES } from '../../../models/notebook-core-read-page'; +import { + checkpoint, + deleteNote, + firstParagraphId, + importSavedResultsNote, + setNotePermissions, + CoreReadSocketProbe +} from '../../../models/notebook-core-read-page.util'; +import { + addPageAnnotationBeforeEach, + getTwoTestAccounts, + loginAs, + performLoginIfRequired, + waitForNotebookLinks, + waitForZeppelinReady, + PAGES +} from '../../../utils'; + +const REACT = 'notebookCoreReact'; +const REMOTE_ENTRY = '**/remoteEntry.js'; + +const uniqueName = (label: string) => `CoreEntry_${label}_${Date.now()}_${Math.random().toString(36).slice(2, 6)}`; + +test.describe('Notebook Core read entry', () => { + addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_CORE_ENTRY); + + let probe: CoreReadSocketProbe; + let readPage: NotebookCoreReadPage; + const notes: string[] = []; + + test.beforeEach(async ({ page }) => { + probe = await CoreReadSocketProbe.install(page); + readPage = new NotebookCoreReadPage(page); + await page.goto('/#/'); + await waitForZeppelinReady(page); + await performLoginIfRequired(page); + await waitForNotebookLinks(page); + }); + + test.afterEach(async ({ page }) => { + for (const noteId of notes.splice(0)) { + await deleteNote(page.request, noteId); + } + }); + + const importNote = async (request: Parameters[0], label: string) => { + const noteId = await importSavedResultsNote(request, uniqueName(label)); + notes.push(noteId); + return noteId; + }; + + const getNoteReads = (from: number) => probe.sentOps(from).filter(op => op === 'GET_NOTE'); + + for (const { label, query, renderer } of NOTEBOOK_CORE_READ_BRANCHES) { + test.describe(`with the ${label} renderer`, () => { + test('follows note changes on one entry, sending reads only', async ({ page }) => { + const a = await importNote(page.request, 'A'); + const b = await importNote(page.request, 'B'); + const from = probe.sent.length; + + await test.step('When the private route opens a note', async () => { + await readPage.open(a, undefined, query); + }); + + await test.step('Then the entry shows it with the chosen renderer', async () => { + await expect(readPage.entry).toHaveAttribute('data-renderer', renderer); + await expect(readPage.readStatus('ready')).toBeVisible(); + await expect(readPage.shows(a)).toBeVisible(); + }); + + await test.step('When the route moves to another note', async () => { + await readPage.open(b, undefined, query); + }); + + await test.step('Then the same entry reads and shows that note', async () => { + await expect(readPage.shows(b)).toBeVisible(); + await expect(readPage.readStatus('ready')).toBeVisible(); + await expect(readPage.entry).toHaveAttribute('data-renderer', renderer); + expect(probe.sentOps(from).filter(op => op !== 'PING')).toEqual(['GET_NOTE', 'GET_NOTE']); + expect(probe.forbiddenOps(from)).toEqual([]); + }); + }); + + test('enters and leaves a revision on one entry', async ({ page }) => { + const noteId = await importNote(page.request, 'revision'); + const revisionId = await checkpoint(page.request, noteId, 'entry revision'); + test.skip(revisionId === null, 'The configured notebook storage does not support revisions'); + const from = probe.sent.length; + + await test.step('When the private route opens a revision', async () => { + await readPage.open(noteId, revisionId!, query); + }); + + await test.step('Then the revision is shown', async () => { + await expect(readPage.shows(revisionId!)).toBeVisible(); + await expect(readPage.readStatus('ready')).toBeVisible(); + }); + + await test.step('When the route moves to the live note', async () => { + await readPage.open(noteId, undefined, query); + }); + + await test.step('Then the live note is read on the same entry', async () => { + await expect(readPage.shows(revisionId!)).toHaveCount(0); + await expect(readPage.readStatus('ready')).toBeVisible(); + await expect(readPage.entry).toHaveAttribute('data-renderer', renderer); + expect(probe.sentOps(from).filter(op => op !== 'PING')).toEqual(['NOTE_REVISION', 'GET_NOTE']); + }); + }); + + test('shows access denied to a user the note does not admit', async ({ page, browser }) => { + const accounts = await getTwoTestAccounts(); + test.skip(!accounts, 'Needs two shiro accounts; anonymous mode admits everyone'); + const [owner, other] = accounts!; + const noteId = await importNote(page.request, 'forbidden'); + await setNotePermissions(page.request, noteId, owner.username); + + // A fresh context would otherwise inherit the signed-in state of the default user. + const otherContext = await browser.newContext({ storageState: { cookies: [], origins: [] } }); + try { + const otherPage = await otherContext.newPage(); + await loginAs(otherPage, other); + const otherReadPage = new NotebookCoreReadPage(otherPage); + + await otherReadPage.open(noteId, undefined, query); + + await expect(otherReadPage.entry).toHaveAttribute('data-renderer', renderer); + await expect(otherReadPage.readStatus('access-denied')).toBeVisible(); + } finally { + await otherContext.close(); + } + }); + }); + } + + const deadRemotes: ReadonlyArray<{ label: string; handle: (route: Route) => Promise }> = [ + { label: 'the remote request fails', handle: route => route.abort() }, + { label: 'the remote answers 404', handle: route => route.fulfill({ status: 404, body: '' }) } + ]; + + for (const { label, handle } of deadRemotes) { + test(`falls back to the Angular read view on the same Core when ${label}`, async ({ page }) => { + const noteId = await importNote(page.request, 'deadRemote'); + const from = probe.sent.length; + + await test.step('Given a remote whose entry does not load', async () => { + await page.route(REMOTE_ENTRY, handle); + }); + + await test.step('When the private route opens with the React renderer', async () => { + // Angular also renders without the flag, so awaiting the request proves this is a fallback. + const remoteRequested = page.waitForRequest(REMOTE_ENTRY); + await readPage.open(noteId, undefined, REACT); + await remoteRequested; + }); + + await test.step('Then the Angular read view shows the note read once', async () => { + await expect(readPage.entry).toHaveAttribute('data-renderer', 'angular-fallback'); + await expect(readPage.view).toHaveAttribute('data-status', 'ready'); + await expect(readPage.skeleton).toHaveCount(0); + expect(getNoteReads(from)).toEqual(['GET_NOTE']); + }); + }); + } + + test('falls back to the Angular read view when the remote never answers', async ({ page }) => { + const noteId = await importNote(page.request, 'slowRemote'); + + await test.step('Given a remote that accepts the request and never answers', async () => { + // The handler settles nothing on purpose: the request is left open. + await page.route(REMOTE_ENTRY, () => {}); + }); + + await test.step('When the private route opens with the React renderer', async () => { + await readPage.open(noteId, undefined, REACT); + }); + + await test.step('Then the Angular read view takes over once the load budget expires', async () => { + await expect(readPage.entry).toHaveAttribute('data-renderer', 'angular-fallback', { timeout: 30000 }); + await expect(readPage.view).toHaveAttribute('data-status', 'ready'); + }); + }); + + test('falls back when the React view fails while mounting, keeping the paragraph anchor in the URL', async ({ + page + }) => { + const noteId = await importNote(page.request, 'mountFailure'); + const paragraphId = await firstParagraphId(page.request, noteId); + const from = probe.sent.length; + + await test.step('When the React view is made to fail before its first commit', async () => { + await readPage.open(noteId, undefined, `${REACT}¬ebookCoreReactFailure=mount¶graph=${paragraphId}`); + }); + + await test.step('Then the Angular read view shows the note and the URL keeps the anchor', async () => { + await expect(readPage.entry).toHaveAttribute('data-renderer', 'angular-fallback'); + await expect(readPage.view).toHaveAttribute('data-status', 'ready'); + await expect(readPage.skeleton).toHaveCount(0); + await expect(page).toHaveURL(new RegExp(`paragraph=${paragraphId}`)); + expect(getNoteReads(from)).toEqual(['GET_NOTE']); + }); + }); + + test('only reports a failure after the React view mounted, leaving the view empty', async ({ page }) => { + const noteId = await importNote(page.request, 'afterMountFailure'); + + await test.step('When the React view fails after its first commit', async () => { + await readPage.open(noteId, undefined, `${REACT}¬ebookCoreReactFailure=after-mount`); + }); + + await test.step('Then the entry records it without switching to Angular', async () => { + await expect(readPage.entry).toHaveAttribute('data-renderer', 'runtime-error'); + // Intended: the error boundary renders nothing, and a fallback is only taken before mounting. + await expect(readPage.skeleton).toHaveCount(0); + await expect(readPage.view).toHaveCount(0); + }); + }); + + test('passes the paragraph anchor to the React view', async ({ page }) => { + const noteId = await importNote(page.request, 'anchor'); + const paragraphId = await firstParagraphId(page.request, noteId); + + await readPage.open(noteId, undefined, `${REACT}¶graph=${paragraphId}`); + + await expect(readPage.entry).toHaveAttribute('data-renderer', 'react-mounted'); + await expect(readPage.skeleton.getByText(paragraphId, { exact: true })).toBeVisible(); + }); + + test('leaving private mode loads the editable notebook afresh at the paragraph anchor', async ({ page }) => { + const noteId = await importNote(page.request, 'leave'); + const lastParagraph = ( + (await (await page.request.get(`/api/notebook/${noteId}`)).json()).body.paragraphs as { + id: string; + }[] + ).at(-1)!.id; + // A short viewport keeps the last paragraph out of view unless the notebook scrolls to it. + await page.setViewportSize({ width: 1280, height: 400 }); + + await test.step('Given the React read view of the note', async () => { + await readPage.open(noteId, undefined, `${REACT}¶graph=${lastParagraph}`); + await expect(readPage.entry).toHaveAttribute('data-renderer', 'react-mounted'); + }); + + const from = probe.sent.length; + + await test.step('When the URL drops the private flags', async () => { + await readPage.openEditable(noteId, `paragraph=${lastParagraph}`); + }); + + await test.step('Then the editable notebook loads the note itself and scrolls to the anchor', async () => { + await expect(readPage.editableNotebook).toBeVisible(); + await expect(readPage.entry).toHaveCount(0); + await expect(readPage.editableParagraphs.last()).toBeInViewport(); + await expect.poll(() => getNoteReads(from)).toContain('GET_NOTE'); + }); + }); +}); diff --git a/zeppelin-web-angular/e2e/utils.ts b/zeppelin-web-angular/e2e/utils.ts index 3255297cc61..65f451657bd 100644 --- a/zeppelin-web-angular/e2e/utils.ts +++ b/zeppelin-web-angular/e2e/utils.ts @@ -53,6 +53,7 @@ export const PAGES = { NOTEBOOK_ACTION_BAR: 'src/app/pages/workspace/notebook/action-bar/action-bar.component', NOTEBOOK_ADD_PARAGRAPH: 'src/app/pages/workspace/notebook/add-paragraph/add-paragraph.component', NOTEBOOK_ASSISTANT: 'src/app/pages/workspace/notebook/assistant/assistant-host.component', + NOTEBOOK_CORE_ENTRY: 'src/app/pages/workspace/notebook/core-read/notebook-core-entry.component', NOTEBOOK_CORE_READ: 'src/app/pages/workspace/notebook/core-read/notebook-core-read.component', NOTEBOOK_INTERPRETER_BINDING: 'src/app/pages/workspace/notebook/interpreter-binding/interpreter-binding.component', NOTEBOOK_NOTE_FORM: 'src/app/pages/workspace/notebook/note-form-block/note-form-block.component', diff --git a/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-model.ts b/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-model.ts index 5ec53e9f961..edc1700ecb5 100644 --- a/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-model.ts +++ b/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-model.ts @@ -10,7 +10,11 @@ * limitations under the License. */ -import type { NotebookCoreSnapshot } from './host-remote-contract'; +import type { + NotebookCoreSnapshot, + NotebookCoreSnapshotListener, + NotebookCoreUnsubscribe +} from './host-remote-contract'; // Inputs: the read payload fields the Core consumes. The Core cannot import SDK types, so these // mirror the wire shapes structurally; the host converts NOTE, NOTE_REVISION and the permissions @@ -114,3 +118,9 @@ export type NotebookReadSnapshot = NotebookCoreSnapshot & note: NotebookReadNote | null; acl: NotebookReadAcl; }>; + +/** A NotebookPort whose snapshot is the read state, so a remote can read it without a cast. */ +export type NotebookReadPort = Readonly<{ + getSnapshot: () => NotebookReadSnapshot; + subscribe: (listener: NotebookCoreSnapshotListener) => NotebookCoreUnsubscribe; +}>; diff --git a/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.spec.ts b/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.spec.ts index 99055a4e61e..5f8a3fc8d02 100644 --- a/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.spec.ts +++ b/zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.spec.ts @@ -10,10 +10,10 @@ * limitations under the License. */ -import { assertType, describe, expect, it } from 'vitest'; +import { assertType, describe, expect, expectTypeOf, it } from 'vitest'; import type { NotebookCorePort } from './host-remote-contract'; -import type { NotebookNoteInput, NotebookParagraphInput, NotebookReadSnapshot } from './read-model'; +import type { NotebookNoteInput, NotebookParagraphInput, NotebookReadPort, NotebookReadSnapshot } from './read-model'; import { createNotebookReadStore } from './read-store'; const paragraph = (id: string, overrides: Partial = {}): NotebookParagraphInput => ({ @@ -58,6 +58,10 @@ assertType<(snapshot: NotebookReadSnapshot) => void>(snapshot => { // The store's snapshot extends the NotebookPort snapshot, so the store can serve as a port. assertType(createNotebookReadStore()); +assertType(createNotebookReadStore()); +// A read port can be passed wherever a NotebookPort is expected. +assertType<(port: NotebookReadPort) => NotebookCorePort>(port => port); +expectTypeOf>().toEqualTypeOf(); describe('notebook read store', () => { it('starts idle with no target and unknown permissions', () => { diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.spec.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.spec.tsx index d8f83b76379..aa09584700d 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.spec.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.spec.tsx @@ -12,11 +12,13 @@ import { act, StrictMode } from 'react'; import { render, renderHook, screen } from '@testing-library/react'; -import type { NotebookCorePort, NotebookCoreSnapshot } from '@zeppelin/notebook-core'; +import { createNotebookReadStore, type NotebookCorePort, type NotebookCoreSnapshot } from '@zeppelin/notebook-core'; import { describe, expect, it, vi } from 'vitest'; import { NotebookCoreProvider, + NotebookReadProvider, useNotebookCore, + useNotebookReadSnapshot, useNotebookSelector, useNotebookSnapshot } from './NotebookCoreProvider'; @@ -197,3 +199,33 @@ describe('useNotebookSelector', () => { expect(screen.getByTestId('revision').textContent).toBe('revision-1'); }); }); + +describe('NotebookReadProvider', () => { + it('keeps the exact read port as the tree port and reads its read state', () => { + const store = createNotebookReadStore(); + const core = Object.freeze({ getSnapshot: store.getSnapshot, subscribe: store.subscribe }); + const { result } = renderHook(() => ({ port: useNotebookCore(), snapshot: useNotebookReadSnapshot() }), { + wrapper: ({ children }) => {children} + }); + + expect(result.current.port).toBe(core); + expect(result.current.snapshot).toBe(store.getSnapshot()); + }); + + it('throws outside NotebookReadProvider, also inside a plain NotebookCoreProvider', () => { + const port = createHostPort(noteSnapshot); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const preventReport = (event: ErrorEvent) => event.preventDefault(); + window.addEventListener('error', preventReport); + try { + expect(() => + renderHook(() => useNotebookReadSnapshot(), { + wrapper: ({ children }) => {children} + }) + ).toThrow('useNotebookReadSnapshot must be used inside NotebookReadProvider'); + } finally { + window.removeEventListener('error', preventReport); + consoleError.mockRestore(); + } + }); +}); diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.tsx index 1ffb1ebec5c..78984f8a0b5 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/notebook/NotebookCoreProvider.tsx @@ -11,7 +11,13 @@ */ import { createContext, ReactNode, useContext, useSyncExternalStore } from 'react'; -import type { NotebookCorePort, NotebookCoreRemoteProps, NotebookCoreSnapshot } from '@zeppelin/notebook-core'; +import type { + NotebookCorePort, + NotebookCoreRemoteProps, + NotebookCoreSnapshot, + NotebookReadPort, + NotebookReadSnapshot +} from '@zeppelin/notebook-core'; const NotebookCoreContext = createContext(null); @@ -56,3 +62,21 @@ export const useNotebookSelector = (selector: (snapshot: NotebookCoreSnapsho const core = useNotebookCore(); return useSyncExternalStore(core.subscribe, () => selector(core.getSnapshot())); }; + +const NotebookReadContext = createContext(null); + +/** NotebookCoreProvider for a read port, so `useNotebookReadSnapshot` can read the read state. */ +export const NotebookReadProvider = ({ core, children }: Readonly<{ core: NotebookReadPort; children: ReactNode }>) => ( + + {children} + +); + +/** The read state of the host-provided read port. Throws outside NotebookReadProvider. */ +export const useNotebookReadSnapshot = (): NotebookReadSnapshot => { + const core = useContext(NotebookReadContext); + if (core === null) { + throw new Error('useNotebookReadSnapshot must be used inside NotebookReadProvider'); + } + return useSyncExternalStore(core.subscribe, core.getSnapshot); +}; diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookReadSkeleton.spec.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookReadSkeleton.spec.tsx new file mode 100644 index 00000000000..64f4808160b --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookReadSkeleton.spec.tsx @@ -0,0 +1,197 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { act, Profiler } from 'react'; +import { render, screen } from '@testing-library/react'; +import { createNotebookReadStore, type NotebookReadPort, type NotebookReadRequest } from '@zeppelin/notebook-core'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { mount, NotebookReadSkeleton, NotebookReadSkeletonMountHandle } from './NotebookReadSkeleton'; + +// The port as NotebookCoreReadHost builds it: a frozen view of the actual read store. +// `notifyUnchanged` calls its listeners without a change, which a port is allowed to do. +const createHostPort = () => { + const store = createNotebookReadStore(); + let listeners = 0; + const subscribed = new Set<() => void>(); + const core: NotebookReadPort = Object.freeze({ + getSnapshot: () => store.getSnapshot(), + subscribe: (listener: () => void) => { + listeners += 1; + subscribed.add(listener); + const unsubscribe = store.subscribe(listener); + return () => { + listeners -= 1; + subscribed.delete(listener); + unsubscribe(); + }; + } + }); + const notifyUnchanged = () => subscribed.forEach(listener => listener()); + return { store, core, listeners: () => listeners, notifyUnchanged }; +}; + +const note = (id: string) => ({ id, name: `${id} name`, path: `/${id}`, paragraphs: [] }); + +const status = () => screen.getByRole('status'); + +let element: HTMLElement; +let handle: NotebookReadSkeletonMountHandle | undefined; + +const mountSkeleton = (props: Parameters[1]) => { + element = document.createElement('div'); + document.body.appendChild(element); + act(() => { + handle = mount(element, props); + }); + return handle!; +}; + +// React reports a render error to console.error and, in development, as a window error event +// that jsdom would print as uncaught. Both are expected when a failure is injected. +const expectingRenderErrors = (run: () => void) => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const preventReport = (event: ErrorEvent) => event.preventDefault(); + window.addEventListener('error', preventReport); + try { + run(); + } finally { + window.removeEventListener('error', preventReport); + consoleError.mockRestore(); + } +}; + +afterEach(() => { + act(() => handle?.unmount()); + handle = undefined; + element?.remove(); +}); + +describe('NotebookReadSkeleton', () => { + it('shows every read status the actual read store publishes', () => { + const port = createHostPort(); + mountSkeleton({ core: port.core }); + const seen = [status().dataset.readStatus]; + + let request!: NotebookReadRequest; + act(() => { + request = port.store.activate({ kind: 'note', noteId: 'note-a' }); + }); + seen.push(status().dataset.readStatus); + act(() => request.acceptNote(note('note-a'))); + seen.push(status().dataset.readStatus); + expect(status().textContent).toBe('Note loaded'); + + for (const failure of ['not-found', 'access-denied', 'failed'] as const) { + act(() => port.store.activate({ kind: 'note', noteId: 'note-b' }).fail(failure)); + seen.push(status().dataset.readStatus); + } + act(() => port.store.dispose()); + seen.push(status().dataset.readStatus); + + expect(seen).toEqual(['idle', 'loading', 'ready', 'not-found', 'access-denied', 'failed', 'disposed']); + }); + + it('shows the note, the revision and the paragraph anchor', () => { + const port = createHostPort(); + port.store.activate({ kind: 'revision', noteId: 'note-a', revisionId: 'revision-1' }); + + mountSkeleton({ core: port.core, anchor: 'paragraph-2' }); + + expect(screen.getByText('note-a')).toBeTruthy(); + expect(screen.getByText('revision-1')).toBeTruthy(); + expect(screen.getByText('paragraph-2')).toBeTruthy(); + }); + + it('does not re-render while the read state is unchanged', () => { + const port = createHostPort(); + const request = port.store.activate({ kind: 'note', noteId: 'note-a' }); + request.acceptNote(note('note-a')); + let commits = 0; + render( + (commits += 1)}> + + + ); + const snapshot = port.core.getSnapshot(); + const commitsBefore = commits; + + act(() => { + request.acceptNote(note('note-a')); + request.fail('failed'); + port.notifyUnchanged(); + }); + + expect(port.core.getSnapshot()).toBe(snapshot); + expect(commits).toBe(commitsBefore); + }); + + it('releases its store subscription on unmount', () => { + const port = createHostPort(); + const mounted = mountSkeleton({ core: port.core }); + expect(port.listeners()).toBe(1); + + act(() => mounted.unmount()); + handle = undefined; + + expect(port.listeners()).toBe(0); + }); + + it('moves its subscription to a replacement port', () => { + const previous = createHostPort(); + const next = createHostPort(); + next.store.activate({ kind: 'note', noteId: 'note-b' }); + const mounted = mountSkeleton({ core: previous.core }); + + act(() => mounted.update({ core: next.core })); + + expect(previous.listeners()).toBe(0); + expect(next.listeners()).toBe(1); + expect(status().dataset.readStatus).toBe('loading'); + }); + + it('reports a completed mount once, also when updates pass a new callback', () => { + const port = createHostPort(); + const calls: string[] = []; + const mounted = mountSkeleton({ core: port.core, onMounted: () => calls.push('first') }); + + act(() => mounted.update({ core: port.core, onMounted: () => calls.push('second'), anchor: 'paragraph-1' })); + act(() => mounted.update({ core: port.core, onMounted: () => calls.push('third') })); + + expect(calls).toEqual(['first']); + }); + + it('reports an injected mount failure without completing the mount', () => { + const onMounted = vi.fn(); + const onError = vi.fn(); + + expectingRenderErrors(() => mountSkeleton({ core: createHostPort().core, failure: 'mount', onMounted, onError })); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onMounted).not.toHaveBeenCalled(); + expect(screen.queryByTestId('notebook-read-skeleton')).toBeNull(); + }); + + it('reports an injected failure after the mount completed', () => { + const calls: string[] = []; + + expectingRenderErrors(() => + mountSkeleton({ + core: createHostPort().core, + failure: 'after-mount', + onMounted: () => calls.push('mounted'), + onError: () => calls.push('error') + }) + ); + + expect(calls).toEqual(['mounted', 'error']); + }); +}); diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookReadSkeleton.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookReadSkeleton.tsx new file mode 100644 index 00000000000..0b8295a2748 --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookReadSkeleton.tsx @@ -0,0 +1,105 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { useEffect, useRef, useState } from 'react'; +import { createRoot, Root } from 'react-dom/client'; +import type { NotebookReadPort, NotebookReadSnapshot } from '@zeppelin/notebook-core'; +import { ReactErrorBoundary } from '@/components/paragraph/ReactErrorBoundary'; +import { NotebookReadProvider, useNotebookReadSnapshot } from '@/notebook/NotebookCoreProvider'; + +export interface NotebookReadSkeletonProps { + core: NotebookReadPort; + /** The `paragraph` query parameter of the notebook URL. */ + anchor?: string | null; + /** Test-only: throw while mounting, or on the first render after the mount completed. */ + failure?: 'mount' | 'after-mount' | null; + /** Called once the first render has committed. This is not notebook readiness. */ + onMounted?: () => void; + onError?: (error: unknown) => void; +} + +const STATUS_TEXT: Record = { + idle: 'No note selected', + loading: 'Loading the note', + ready: 'Note loaded', + 'not-found': 'Note not found', + 'access-denied': 'You do not have permission to read this note', + failed: 'The note could not be loaded', + disposed: 'Closed' +}; + +const Skeleton = ({ anchor, failure, onMounted }: Omit) => { + const snapshot = useNotebookReadSnapshot(); + const [mounted, setMounted] = useState(false); + // Read through a ref so a host passing a new callback on update() does not report the mount again. + const onMountedRef = useRef(onMounted); + onMountedRef.current = onMounted; + + useEffect(() => { + onMountedRef.current?.(); + setMounted(true); + }, []); + + if (failure === 'mount' || (failure === 'after-mount' && mounted)) { + throw new Error(`NotebookReadSkeleton: injected ${failure} failure`); + } + + return ( +
+

+ {STATUS_TEXT[snapshot.status]} +

+
+
Note
+
{snapshot.noteId}
+
Revision
+
{snapshot.revisionId ?? 'Live'}
+
Paragraph
+
{anchor ?? 'None'}
+
+
+ ); +}; + +export const NotebookReadSkeleton = ({ core, onError, ...props }: NotebookReadSkeletonProps) => ( + + + + + +); + +export interface NotebookReadSkeletonMountHandle { + update: (props: NotebookReadSkeletonProps) => void; + unmount: () => void; +} + +export const mount = ( + element: HTMLElement, + initialProps: NotebookReadSkeletonProps +): NotebookReadSkeletonMountHandle => { + if (!element) { + throw new Error('Mount element is required'); + } + + const root: Root = createRoot(element); + root.render(); + + return { + update: (props: NotebookReadSkeletonProps) => { + root.render(); + }, + unmount: () => { + root.unmount(); + } + }; +}; diff --git a/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js b/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js index 9c2c4dba609..5e3b9c7342a 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js +++ b/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js @@ -83,7 +83,8 @@ module.exports = (_env, argv) => { './ParagraphFooter': './src/components/paragraph/ParagraphFooter', './ConfigurationTable': './src/pages/ConfigurationTable', './NotebookRepoList': './src/pages/NotebookRepoList', - './AssistantWorkspace': './src/pages/AssistantWorkspace' + './AssistantWorkspace': './src/pages/AssistantWorkspace', + './NotebookReadSkeleton': './src/pages/NotebookReadSkeleton' } }), new HtmlWebpackPlugin({ diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-entry.component.testbed.spec.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-entry.component.testbed.spec.ts new file mode 100644 index 00000000000..af5d5636bd4 --- /dev/null +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-entry.component.testbed.spec.ts @@ -0,0 +1,205 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Component, Directive, Input, OnDestroy } from '@angular/core'; +import { TestBed } from '@angular/core/testing'; +import { convertToParamMap, ActivatedRoute, Params } from '@angular/router'; +import { BehaviorSubject, Subject } from 'rxjs'; +import { map } from 'rxjs/operators'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { OP } from '@zeppelin/sdk'; + +import { Permissions } from '@zeppelin/interfaces'; +import { BaseUrlService } from '@zeppelin/services/base-url.service'; +import { MessageService } from '@zeppelin/services/message.service'; +import { SecurityService } from '@zeppelin/services/security.service'; +import { TicketService } from '@zeppelin/services/ticket.service'; + +import { NotebookCoreEntryComponent } from './notebook-core-entry.component'; +import { NotebookCoreReadHost } from './notebook-core-read-host'; + +type SkeletonProps = Readonly<{ + core: unknown; + anchor: string | null; + failure: string | null; + onMounted: () => void; + onError: (error: unknown) => void; +}>; + +// Stands in for ReactMountDirective: records what the entry hands the remote and when it is torn down. +const mounts: Array<{ module: string; props: () => SkeletonProps; destroyed: boolean }> = []; + +@Directive({ selector: '[zeppelin-react-mount]', standalone: false }) +class ReactMountStubDirective implements OnDestroy { + @Input() reactProps: Record = {}; + private readonly mount = { module: '', props: () => this.reactProps as SkeletonProps, destroyed: false }; + + constructor() { + mounts.push(this.mount); + } + + @Input('zeppelin-react-mount') set module(module: string) { + this.mount.module = module; + } + + ngOnDestroy(): void { + this.mount.destroyed = true; + } +} + +@Component({ selector: 'zeppelin-notebook-core-read', standalone: false, template: '' }) +class ReadViewStubComponent { + @Input() host!: NotebookCoreReadHost; +} + +const setup = (query: Params = {}) => { + const messageService = new MessageService({} as BaseUrlService, {} as TicketService, null as never); + messageService.connectedStatus = true; + const sent: OP[] = []; + vi.spyOn(messageService, 'send').mockImplementation(((op: OP) => { + sent.push(op); + return `msg-${sent.length}`; + }) as MessageService['send']); + const params = new BehaviorSubject({ noteId: 'A' }); + const queryParams = new BehaviorSubject(query); + + TestBed.configureTestingModule({ + declarations: [NotebookCoreEntryComponent, ReactMountStubDirective, ReadViewStubComponent], + providers: [ + { provide: MessageService, useValue: messageService }, + { provide: SecurityService, useValue: { getPermissions: () => new Subject() } }, + { + provide: ActivatedRoute, + useValue: { + params, + queryParamMap: queryParams.pipe(map(convertToParamMap)), + snapshot: { queryParamMap: convertToParamMap(query) } + } + } + ] + }); + const fixture = TestBed.createComponent(NotebookCoreEntryComponent); + fixture.autoDetectChanges(); + + const { host } = fixture.componentInstance; + const root = () => fixture.nativeElement.querySelector('[data-testid="notebook-core-entry"]') as HTMLElement; + const readView = () => fixture.debugElement.query(el => el.componentInstance instanceof ReadViewStubComponent); + const mount = () => mounts[mounts.length - 1]; + return { fixture, host, sent, params, queryParams, root, readView, mount }; +}; + +const REACT = { notebookCoreRead: '', notebookCoreReact: '' }; + +afterEach(() => { + mounts.length = 0; + vi.restoreAllMocks(); +}); + +describe('NotebookCoreEntryComponent', () => { + it('shows the Angular read view on its host without the React flag, ignoring the failure parameter', () => { + const { host, sent, root, readView } = setup({ notebookCoreRead: '', notebookCoreReactFailure: 'mount' }); + + expect(root().dataset.renderer).toBe('angular'); + expect(readView().componentInstance.host).toBe(host); + expect(mounts).toHaveLength(0); + expect(sent).toEqual([OP.GET_NOTE]); + }); + + it('mounts the React read view on the host port with the paragraph anchor', () => { + const { fixture, host, queryParams, root, readView, mount } = setup({ ...REACT, paragraph: 'p2' }); + + expect(root().dataset.renderer).toBe('react-loading'); + expect(readView()).toBeNull(); + expect(mount().module).toBe('./NotebookReadSkeleton'); + expect(mount().props().core).toBe(host.port); + expect(mount().props().anchor).toBe('p2'); + expect(mount().props().failure).toBeNull(); + + queryParams.next({ ...REACT, paragraph: 'p3' }); + fixture.detectChanges(); + + expect(mount().props().core).toBe(host.port); + expect(mount().props().anchor).toBe('p3'); + expect(mounts).toHaveLength(1); + }); + + it('passes a known failure parameter to the React read view and drops an unknown one', () => { + expect( + setup({ ...REACT, notebookCoreReactFailure: 'after-mount' }) + .mount() + .props().failure + ).toBe('after-mount'); + TestBed.resetTestingModule(); + expect( + setup({ ...REACT, notebookCoreReactFailure: 'other' }) + .mount() + .props().failure + ).toBeNull(); + }); + + it('falls back to the Angular read view on the same host when React fails before its first commit', () => { + vi.spyOn(console, 'error').mockImplementation(() => undefined); + const { fixture, host, sent, root, readView, mount } = setup(REACT); + const reactMount = mount(); + + reactMount.props().onError(new Error('remote failed')); + fixture.detectChanges(); + + expect(root().dataset.renderer).toBe('angular-fallback'); + expect(reactMount.destroyed).toBe(true); + expect(readView().componentInstance.host).toBe(host); + expect(host.port.getSnapshot().status).toBe('loading'); + expect(sent).toEqual([OP.GET_NOTE]); + }); + + it('reports a failure after the first commit without switching to Angular', () => { + vi.spyOn(console, 'error').mockImplementation(() => undefined); + const { fixture, root, readView, mount } = setup(REACT); + const reactMount = mount(); + + reactMount.props().onMounted(); + fixture.detectChanges(); + expect(root().dataset.renderer).toBe('react-mounted'); + + reactMount.props().onError(new Error('render failed')); + fixture.detectChanges(); + + expect(root().dataset.renderer).toBe('runtime-error'); + expect(reactMount.destroyed).toBe(false); + expect(readView()).toBeNull(); + expect(console.error).toHaveBeenCalledTimes(1); + }); + + it('opens the note or revision from the route on the same host, sending reads only', () => { + const { host, sent, params } = setup(REACT); + const port = host.port; + + params.next({ noteId: 'A', revisionId: 'r1' }); + params.next({ noteId: 'B' }); + + expect(sent).toEqual([OP.GET_NOTE, OP.NOTE_REVISION, OP.GET_NOTE]); + expect(host.port).toBe(port); + expect(host.port.getSnapshot().noteId).toBe('B'); + }); + + it('disposes its host once on destroy and sends nothing while doing so', () => { + const { fixture, host, sent } = setup(REACT); + const dispose = vi.spyOn(host, 'dispose'); + + fixture.destroy(); + + expect(dispose).toHaveBeenCalledTimes(1); + expect(host.port.getSnapshot().status).toBe('disposed'); + expect(sent).toEqual([OP.GET_NOTE]); + }); +}); diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-entry.component.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-entry.component.ts new file mode 100644 index 00000000000..a5287c32632 --- /dev/null +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-entry.component.ts @@ -0,0 +1,121 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ChangeDetectionStrategy, ChangeDetectorRef, Component, OnDestroy, OnInit } from '@angular/core'; +import { ActivatedRoute, ParamMap } from '@angular/router'; +import { Subject } from 'rxjs'; +import { takeUntil } from 'rxjs/operators'; + +import { MessageService, ReactFeatureService, SecurityService } from '@zeppelin/services'; + +import { NotebookCoreReadHost } from './notebook-core-read-host'; + +/** + * Which view the private read route shows. The `react-*` values and `runtime-error` describe the + * React mount only, never whether the note is loaded. + */ +export type NotebookCoreRenderer = 'angular' | 'react-loading' | 'react-mounted' | 'runtime-error' | 'angular-fallback'; + +/** Test-only: makes the React read view throw, so the fallback can be exercised. */ +const NOTEBOOK_CORE_REACT_FAILURE_QUERY_PARAM = 'notebookCoreReactFailure'; +const NOTEBOOK_CORE_REACT_FAILURES = ['mount', 'after-mount'] as const; +type NotebookCoreReactFailure = (typeof NOTEBOOK_CORE_REACT_FAILURES)[number]; + +const toFailure = (value: string | null): NotebookCoreReactFailure | null => + NOTEBOOK_CORE_REACT_FAILURES.find(failure => failure === value) ?? null; + +/** + * The private read route (ZEPPELIN-6727). It owns one host for the route lifetime and, with + * `notebookCoreReact`, shows the React read view on that host's port. The renderer is chosen at + * entry: a React failure before the first commit falls back to the Angular read view on the same + * host, a failure after it is only reported. + */ +@Component({ + selector: 'zeppelin-notebook-core-entry', + standalone: false, + template: ` +
+ @if (renderer === 'angular' || renderer === 'angular-fallback') { + + } @else { +
+ } +
+ `, + changeDetection: ChangeDetectionStrategy.OnPush +}) +export class NotebookCoreEntryComponent implements OnInit, OnDestroy { + readonly host: NotebookCoreReadHost; + renderer: NotebookCoreRenderer; + reactProps: Record = {}; + private readonly failure: NotebookCoreReactFailure | null; + private readonly destroy$ = new Subject(); + + constructor( + private readonly activatedRoute: ActivatedRoute, + private readonly cdr: ChangeDetectorRef, + messageService: MessageService, + securityService: SecurityService, + reactFeature: ReactFeatureService + ) { + this.host = new NotebookCoreReadHost(messageService, messageService, securityService); + const query = activatedRoute.snapshot.queryParamMap; + this.renderer = reactFeature.isEnabled('notebookReadReact', query) ? 'react-loading' : 'angular'; + this.failure = toFailure(query.get(NOTEBOOK_CORE_REACT_FAILURE_QUERY_PARAM)); + } + + ngOnInit(): void { + this.activatedRoute.params.pipe(takeUntil(this.destroy$)).subscribe(({ noteId, revisionId }) => { + if (!noteId) { + throw new Error('Route parameter `noteId` is required.'); + } + this.host.open(revisionId ? { kind: 'revision', noteId, revisionId } : { kind: 'note', noteId }); + }); + if (this.renderer === 'react-loading') { + this.activatedRoute.queryParamMap.pipe(takeUntil(this.destroy$)).subscribe(query => this.setReactProps(query)); + } + } + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + this.host.dispose(); + } + + private readonly onMounted = (): void => { + if (this.renderer === 'react-loading') { + this.renderer = 'react-mounted'; + this.cdr.markForCheck(); + } + }; + + private readonly onError = (error: unknown): void => { + console.error('React notebook read view error', error); + if (this.renderer === 'react-loading') { + this.renderer = 'angular-fallback'; + } else if (this.renderer === 'react-mounted') { + this.renderer = 'runtime-error'; + } + this.cdr.markForCheck(); + }; + + private setReactProps(query: ParamMap): void { + this.reactProps = { + core: this.host.port, + anchor: query.get('paragraph'), + failure: this.failure, + onMounted: this.onMounted, + onError: this.onError + }; + this.cdr.markForCheck(); + } +} diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read-host.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read-host.ts index 27a6b1c8510..73a902c3dd6 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read-host.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read-host.ts @@ -14,7 +14,7 @@ import { BehaviorSubject, Observable, Subscription } from 'rxjs'; import { createNotebookReadStore, - NotebookCorePort, + NotebookReadPort, NotebookReadSnapshot, NotebookReadStore, NotebookReadTarget @@ -48,7 +48,7 @@ export type NotebookCoreReadState = Readonly<{ */ export class NotebookCoreReadHost { /** Stable for the whole lifetime, so a remote can keep it as its NotebookPort. */ - readonly port: NotebookCorePort; + readonly port: NotebookReadPort; readonly state$: Observable; private readonly store: NotebookReadStore = createNotebookReadStore(); diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.testbed.spec.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.testbed.spec.ts index 780d3afc375..8cd1905c6cf 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.testbed.spec.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.testbed.spec.ts @@ -12,8 +12,7 @@ import { Component, Input } from '@angular/core'; import { TestBed } from '@angular/core/testing'; -import { ActivatedRoute, Params } from '@angular/router'; -import { BehaviorSubject, Subject } from 'rxjs'; +import { Subject } from 'rxjs'; import { describe, expect, it, vi } from 'vitest'; import { MessageReceiveDataTypeMap, OP, WebSocketMessage } from '@zeppelin/sdk'; @@ -24,6 +23,7 @@ import { MessageService } from '@zeppelin/services/message.service'; import { SecurityService } from '@zeppelin/services/security.service'; import { TicketService } from '@zeppelin/services/ticket.service'; +import { NotebookCoreReadHost } from './notebook-core-read-host'; import { NotebookCoreReadComponent } from './notebook-core-read.component'; import template from './notebook-core-read.component.html?raw'; @@ -50,26 +50,25 @@ const setup = () => { return msgId; }) as MessageService['send']); const permissions = new Subject(); - const params = new BehaviorSubject({ noteId: 'A' }); + const securityService = { getPermissions: () => permissions } as unknown as SecurityService; + // The route entry owns the host; the view only receives it. + const host = new NotebookCoreReadHost(messageService, messageService, securityService); + host.open({ kind: 'note', noteId: 'A' }); TestBed.configureTestingModule({ declarations: [NotebookCoreReadComponent, ResultStubComponent], - providers: [ - { provide: MessageService, useValue: messageService }, - { provide: SecurityService, useValue: { getPermissions: () => permissions } }, - { provide: TicketService, useValue: { ticket: { principal: 'alice', roles: '[]' } } }, - { provide: ActivatedRoute, useValue: { params } } - ] + providers: [{ provide: TicketService, useValue: { ticket: { principal: 'alice', roles: '[]' } } }] }).overrideComponent(NotebookCoreReadComponent, { set: { template, templateUrl: undefined, styles: [], styleUrls: [] } }); const fixture = TestBed.createComponent(NotebookCoreReadComponent); + fixture.componentRef.setInput('host', host); fixture.autoDetectChanges(); const reply = (message: WebSocketMessage) => messageService.shortCircuit(message); const lastMsgId = () => sent[sent.length - 1].msgId; const root = () => fixture.nativeElement.querySelector('[data-testid="notebook-core-read"]') as HTMLElement; - return { fixture, messageService, sent, permissions, params, reply, lastMsgId, root }; + return { fixture, host, messageService, sent, permissions, reply, lastMsgId, root }; }; const noteMessage = (noteId: string, msgId: string) => @@ -89,11 +88,12 @@ const noteMessage = (noteId: string, msgId: string) => }) as WebSocketMessage; describe('NotebookCoreReadComponent', () => { - it('sends only the note read when it mounts', () => { + it('renders the state of the host it receives', () => { const { sent, root } = setup(); expect(sent.map(({ op }) => op)).toEqual([OP.GET_NOTE]); expect(root().dataset.status).toBe('loading'); + expect(root().dataset.noteId).toBe('A'); }); it('renders the loaded note when its reply arrives', async () => { @@ -128,28 +128,16 @@ describe('NotebookCoreReadComponent', () => { expect(root().querySelector('[data-testid="notebook-core-read-permissions"]')).toBeNull(); }); - it('reads a revision and then the live note when the route parameters change, sending reads only', () => { - const { sent, params } = setup(); - - params.next({ noteId: 'A', revisionId: 'r1' }); - params.next({ noteId: 'B' }); - - expect(sent.map(({ op }) => op)).toEqual([OP.GET_NOTE, OP.NOTE_REVISION, OP.GET_NOTE]); - }); - - it('disposes its Core once on unmount and sends nothing while doing so', () => { - const { fixture, sent, reply, lastMsgId, messageService } = setup(); + it('leaves the host open and sends nothing when it is destroyed', () => { + const { fixture, host, sent, reply, lastMsgId } = setup(); reply(noteMessage('A', lastMsgId())); - const { host } = fixture.componentInstance; const dispose = vi.spyOn(host, 'dispose'); const sentBefore = sent.length; fixture.destroy(); - messageService.connectedStatus$.next(false); - messageService.connectedStatus$.next(true); - expect(dispose).toHaveBeenCalledTimes(1); - expect(host.port.getSnapshot().status).toBe('disposed'); + expect(dispose).not.toHaveBeenCalled(); + expect(host.port.getSnapshot().status).toBe('ready'); expect(sent).toHaveLength(sentBefore); }); }); diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.ts index 9283a6216a4..244b2ec9253 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/core-read/notebook-core-read.component.ts @@ -10,13 +10,12 @@ * limitations under the License. */ -import { ChangeDetectionStrategy, Component, OnDestroy, OnInit } from '@angular/core'; -import { ActivatedRoute } from '@angular/router'; -import { Observable, Subject } from 'rxjs'; -import { map, takeUntil } from 'rxjs/operators'; +import { ChangeDetectionStrategy, Component, Input, OnInit } from '@angular/core'; +import { Observable } from 'rxjs'; +import { map } from 'rxjs/operators'; import { selectAclCapabilities, selectOrderedParagraphs, NotebookReadSnapshot } from '@zeppelin/notebook-core'; -import { MessageService, SecurityService, TicketService } from '@zeppelin/services'; +import { TicketService } from '@zeppelin/services'; import { NotebookCoreReadHost } from './notebook-core-read-host'; import { @@ -37,7 +36,7 @@ type ReadView = Readonly<{ /** * The private read-only notebook view. It renders loaded Core snapshots and saved results only: * nothing here sends run, save, patch, form or configuration messages, including on mount and - * unmount. Its host, and the port on it, live exactly as long as this route component. + * unmount. The route entry owns the host: this view neither opens nor disposes it. */ @Component({ selector: 'zeppelin-notebook-core-read', @@ -46,43 +45,23 @@ type ReadView = Readonly<{ styleUrls: ['./notebook-core-read.component.less'], changeDetection: ChangeDetectionStrategy.OnPush }) -export class NotebookCoreReadComponent implements OnInit, OnDestroy { - readonly host: NotebookCoreReadHost; - readonly view$: Observable; - private readonly destroy$ = new Subject(); +export class NotebookCoreReadComponent implements OnInit { + @Input({ required: true }) host!: NotebookCoreReadHost; + view$!: Observable; - constructor( - private readonly activatedRoute: ActivatedRoute, - messageService: MessageService, - securityService: SecurityService, - ticketService: TicketService - ) { - this.host = new NotebookCoreReadHost(messageService, messageService, securityService); + constructor(private readonly ticketService: TicketService) {} + + ngOnInit(): void { this.view$ = this.host.state$.pipe( map(({ snapshot, connected }) => ({ snapshot, connected, paragraphs: selectOrderedParagraphs(snapshot).map(toReadParagraphView), - access: toReadAccess(selectAclCapabilities(snapshot, toPrincipals(ticketService.ticket))) + access: toReadAccess(selectAclCapabilities(snapshot, toPrincipals(this.ticketService.ticket))) })) ); } - ngOnInit(): void { - this.activatedRoute.params.pipe(takeUntil(this.destroy$)).subscribe(({ noteId, revisionId }) => { - if (!noteId) { - throw new Error('Route parameter `noteId` is required.'); - } - this.host.open(revisionId ? { kind: 'revision', noteId, revisionId } : { kind: 'note', noteId }); - }); - } - - ngOnDestroy(): void { - this.destroy$.next(); - this.destroy$.complete(); - this.host.dispose(); - } - retry(): void { this.host.retry(); } diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-route-boundary.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-route-boundary.ts index a63dfd9b040..71a71ad33bd 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-route-boundary.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-route-boundary.ts @@ -24,7 +24,8 @@ export const createNotebookChildRoutes = (component: Type, canMatch?: C /** * Development and test opt-in for the private Core-backed read view (ZEPPELIN-6737), e.g. * `#/notebook/?notebookCoreRead`. Without it the editable notebook is unchanged. Navigating to - * a notebook URL without it leaves the read view and loads the editable notebook afresh. + * a notebook URL without it leaves the read view and loads the editable notebook afresh. Adding + * `notebookCoreReact` shows the React read view there instead (ZEPPELIN-6727). */ const NOTEBOOK_CORE_READ_QUERY_PARAM = 'notebookCoreRead'; diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-routing.module.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-routing.module.ts index ae454561bc3..f0cb0d052a6 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-routing.module.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook-routing.module.ts @@ -13,12 +13,12 @@ import { NgModule } from '@angular/core'; import { RouterModule, Routes } from '@angular/router'; -import { NotebookCoreReadComponent } from './core-read/notebook-core-read.component'; +import { NotebookCoreEntryComponent } from './core-read/notebook-core-entry.component'; import { NotebookComponent } from './notebook.component'; import { canMatchNotebookCoreRead, createNotebookChildRoutes } from './notebook-route-boundary'; const routes: Routes = [ - ...createNotebookChildRoutes(NotebookCoreReadComponent, [canMatchNotebookCoreRead]), + ...createNotebookChildRoutes(NotebookCoreEntryComponent, [canMatchNotebookCoreRead]), ...createNotebookChildRoutes(NotebookComponent) ]; diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.module.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.module.ts index 8dbfc6ea584..cec9751b2c0 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.module.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.module.ts @@ -52,6 +52,7 @@ import { NotebookPermissionsComponent } from './permissions/permissions.componen import { NotebookRevisionsComparatorComponent } from './revisions-comparator/revisions-comparator.component'; import { NotebookActionBarComponent } from './action-bar/action-bar.component'; +import { NotebookCoreEntryComponent } from './core-read/notebook-core-entry.component'; import { NotebookCoreReadComponent } from './core-read/notebook-core-read.component'; import { NoteFormBlockComponent } from './note-form-block/note-form-block.component'; import { NotebookRoutingModule } from './notebook-routing.module'; @@ -66,6 +67,7 @@ import { AssistantSlotDirective } from './assistant/assistant-slots'; AssistantHostComponent, AssistantSlotDirective, NotebookComponent, + NotebookCoreEntryComponent, NotebookCoreReadComponent, NotebookActionBarComponent, NotebookInterpreterBindingComponent, diff --git a/zeppelin-web-angular/src/app/services/react-feature.service.spec.ts b/zeppelin-web-angular/src/app/services/react-feature.service.spec.ts index 239ec2d4c5a..4fc868839b3 100644 --- a/zeppelin-web-angular/src/app/services/react-feature.service.spec.ts +++ b/zeppelin-web-angular/src/app/services/react-feature.service.spec.ts @@ -23,3 +23,14 @@ describe('Assistant React feature flag', () => { expect(service.isEnabled('assistantPanel', new Map([['reactAssistant', 'false']]))).toBe(false); }); }); + +describe('Notebook read React feature flag', () => { + const service = new ReactFeatureService(); + + it('is disabled by default and enabled only by its own query parameter, not by the read route flag', () => { + expect(service.isEnabled('notebookReadReact')).toBe(false); + expect(service.isEnabled('notebookReadReact', new Map([['notebookCoreRead', '']]))).toBe(false); + expect(service.isEnabled('notebookReadReact', new Map([['notebookCoreReact', '']]))).toBe(true); + expect(service.isEnabled('notebookReadReact', new Map([['notebookCoreReact', 'false']]))).toBe(false); + }); +}); diff --git a/zeppelin-web-angular/src/app/services/react-feature.service.ts b/zeppelin-web-angular/src/app/services/react-feature.service.ts index 239a49cee16..a29332805d0 100644 --- a/zeppelin-web-angular/src/app/services/react-feature.service.ts +++ b/zeppelin-web-angular/src/app/services/react-feature.service.ts @@ -14,7 +14,12 @@ import { Injectable } from '@angular/core'; import { parseBooleanFlag } from './query-flag.util'; export type ReactSurface = - 'publishedParagraph' | 'paragraphFooter' | 'configurationTable' | 'notebookRepoList' | 'assistantPanel'; + | 'publishedParagraph' + | 'paragraphFooter' + | 'configurationTable' + | 'notebookRepoList' + | 'assistantPanel' + | 'notebookReadReact'; interface ReactSurfaceConfig { queryParam: string; @@ -41,6 +46,11 @@ const SURFACES: Record = { assistantPanel: { queryParam: 'reactAssistant', defaultEnabled: false + }, + // Read only on the private read route, i.e. together with `notebookCoreRead`. + notebookReadReact: { + queryParam: 'notebookCoreReact', + defaultEnabled: false } }; diff --git a/zeppelin-web-angular/test/notebook-core/import-boundary.spec.ts b/zeppelin-web-angular/test/notebook-core/import-boundary.spec.ts index befd64f9aef..d336734d4c1 100644 --- a/zeppelin-web-angular/test/notebook-core/import-boundary.spec.ts +++ b/zeppelin-web-angular/test/notebook-core/import-boundary.spec.ts @@ -235,6 +235,17 @@ describe('notebook core import boundary', () => { ).toContain(`${path}: remote notebook core import NotebookCoreUnsubscribe`); }); + it('allows the read port and snapshot types but not the read store behind them', () => { + const path = reactNotebookCoreProofConsumer; + const read = `import type { NotebookReadPort, NotebookReadSnapshot } from '@zeppelin/notebook-core';`; + expect(findNotebookRemoteConsumerViolations(path, read)).toEqual([]); + + const store = `import type { NotebookReadStore } from '@zeppelin/notebook-core';`; + expect(findNotebookRemoteConsumerViolations(path, store)).toContain( + `${path}: remote notebook core import NotebookReadStore` + ); + }); + it('enforces the core boundary for built-package aliases and relative targets', () => { const path = reactNotebookCoreProofConsumer; const builtDeclaration = resolve( diff --git a/zeppelin-web-angular/test/notebook-core/import-boundary.ts b/zeppelin-web-angular/test/notebook-core/import-boundary.ts index 5c9ff6cb8e2..c74d962ddca 100644 --- a/zeppelin-web-angular/test/notebook-core/import-boundary.ts +++ b/zeppelin-web-angular/test/notebook-core/import-boundary.ts @@ -226,7 +226,13 @@ export const findReactNotebookConsumerViolations = ( return violations; }; -const notebookRemoteContractTypes = new Set(['NotebookCorePort', 'NotebookCoreRemoteProps', 'NotebookCoreSnapshot']); +const notebookRemoteContractTypes = new Set([ + 'NotebookCorePort', + 'NotebookCoreRemoteProps', + 'NotebookCoreSnapshot', + 'NotebookReadPort', + 'NotebookReadSnapshot' +]); const isTypeOnlyImportDeclaration = (node: ts.ImportDeclaration): boolean => { const clause = node.importClause;