diff --git a/zeppelin-web-angular/e2e/AGENTS.md b/zeppelin-web-angular/e2e/AGENTS.md index 35a7da5d978..a45bf08fa1f 100644 --- a/zeppelin-web-angular/e2e/AGENTS.md +++ b/zeppelin-web-angular/e2e/AGENTS.md @@ -122,7 +122,7 @@ Use an existing key from the `PAGES` object in `e2e/utils.ts`; add a new one the ## Migration (Angular to React Microfrontend) -Pages are moving from Angular to React fragments incrementally. Today this is narrow: the published paragraph route reads a `?react=true` flag (`published/paragraph/paragraph.component`), the notebook footer swaps via a `?reactFooter=true` flag (read into the notebook component's `useReactFooter` input), and the configuration table swaps via a `?reactConfiguration=true` flag (`configuration/configuration.component`). All three are query params inside the hash. There is no app-wide "flip this route to React" flag and no separate cross-framework Playwright project in this config. The notebook parity registry records the Angular behavior baseline and links it to existing framework-neutral tests; add scenarios as migration work reaches them rather than duplicating the suite for both frameworks. +Pages are moving from Angular to React fragments incrementally. Today this is narrow: the published paragraph route reads a `?react=true` flag (`published/paragraph/paragraph.component`), the notebook footer swaps via a `?reactFooter=true` flag (read into the notebook component's `useReactFooter` input), the configuration table swaps via a `?reactConfiguration=true` flag (`configuration/configuration.component`), and the notebook repository list swaps via a `?reactNotebookRepos=true` flag (`notebook-repos/notebook-repos.component`). All four are query params inside the hash. There is no app-wide "flip this route to React" flag and no separate cross-framework Playwright project in this config. The notebook parity registry records the Angular behavior baseline and links it to existing framework-neutral tests; add scenarios as migration work reaches them rather than duplicating the suite for both frameworks. ### Write Framework-Neutral Specs diff --git a/zeppelin-web-angular/e2e/models/notebook-repos-page.ts b/zeppelin-web-angular/e2e/models/notebook-repos-page.ts index f047b44b2e6..5caa33edc82 100644 --- a/zeppelin-web-angular/e2e/models/notebook-repos-page.ts +++ b/zeppelin-web-angular/e2e/models/notebook-repos-page.ts @@ -21,7 +21,9 @@ export class NotebookReposPage extends BasePage { constructor(page: Page) { super(page); this.pageDescription = page.locator("text=Manage your Notebook Repositories' settings."); - this.repositoryItems = page.locator('zeppelin-notebook-repo-item'); + // Shared id, not the Angular element: /notebook-repos is a migration seam + // and these models have to survive the flip. + this.repositoryItems = page.locator('[data-testid="notebook-repo-item"]'); } async navigate(): Promise { @@ -46,13 +48,14 @@ export class NotebookRepoItemPage extends BasePage { constructor(page: Page, repoName: string) { super(page); - this.repositoryCard = page.locator('nz-card').filter({ hasText: repoName }); + this.repositoryCard = page.locator(`[data-testid="notebook-repo-item"][data-repo-name="${repoName}"]`); this.repositoryName = this.repositoryCard.locator('.ant-card-head-title'); this.editButton = this.repositoryCard.locator('button:has-text("Edit")'); this.saveButton = this.repositoryCard.locator('button:has-text("Save")'); this.cancelButton = this.repositoryCard.locator('button:has-text("Cancel")'); - this.settingTable = this.repositoryCard.locator('nz-table'); - this.settingRows = this.repositoryCard.locator('tbody tr'); + // .ant-table is what both ng-zorro and antd render. + this.settingTable = this.repositoryCard.locator('.ant-table'); + this.settingRows = this.repositoryCard.locator('tbody tr:not(.ant-table-placeholder)'); } async clickEdit(): Promise { diff --git a/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repos-save-reloads-note-tree.spec.ts b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repos-save-reloads-note-tree.spec.ts new file mode 100644 index 00000000000..ab4f321052d --- /dev/null +++ b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repos-save-reloads-note-tree.spec.ts @@ -0,0 +1,84 @@ +/* + * 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, Page } from '@playwright/test'; +import { NotebookReposPage, NotebookRepoItemPage } from '../../../models/notebook-repos-page'; +import { NodeListPage } from '../../../models/node-list-page'; +import { addPageAnnotationBeforeEach, PAGES, waitForZeppelinReady } from '../../../utils'; + +const createNoteAtRoot = async (page: Page, name: string): Promise => { + const response = await page.request.post('/api/notebook', { + data: { notePath: `/${name}`, addingEmptyParagraph: true }, + failOnStatusCode: false + }); + expect(response.ok(), `Create notebook failed: ${response.status()}`).toBe(true); + return JSON.parse(await response.text()).body as string; +}; + +test.describe('Notebook Repository - save reloads the note tree', () => { + addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_REPOS); + + // Run on both branches of the ZEPPELIN-6631 flag. The reload is the host's + // job either way, so a React list that swallows the save would show up here. + for (const { label, query } of [ + { label: 'Angular list', query: '' }, + { label: 'React list', query: '?reactNotebookRepos=true' } + ]) { + test(`a repository save reloads notebooks and refreshes the shell note tree (${label})`, async ({ context }) => { + // Two clients on purpose. The header's note tree is destroyed when the + // dropdown closes and calls listNodes() again on every open, so it cannot + // tell a broadcast from its own refetch. The home route keeps a tree + // mounted, which leaves the broadcast as the only thing that can change it. + const watcher = await context.newPage(); + const actor = await context.newPage(); + const noteName = `NotebookRepoReload_${Date.now()}`; + let noteId = ''; + + try { + await watcher.goto('/#/'); + await waitForZeppelinReady(watcher); + const noteTree = new NodeListPage(watcher); + await expect(noteTree.nodeListContainer).toBeVisible(); + + await actor.goto(`/#/notebook-repos${query}`); + await waitForZeppelinReady(actor); + const reposPage = new NotebookReposPage(actor); + await expect(reposPage.repositoryItems.first()).toBeVisible({ timeout: 20000 }); + // JUSTIFIED: .first() picks the first configured repo; the page requires at least one. + const repoName = (await reposPage.repositoryItems.first().getAttribute('data-repo-name')) || ''; + const repoItem = new NotebookRepoItemPage(actor, repoName); + + await test.step('Given a note created out of band, which no broadcast has announced', async () => { + await expect(noteTree.noteLinkByName(noteName)).toHaveCount(0); + noteId = await createNoteAtRoot(actor, noteName); + // Creating a note over REST does not broadcast the list, so a tree + // that picked this up on its own would make the assertion after the + // save meaningless. + await expect(noteTree.noteLinkByName(noteName)).toHaveCount(0); + }); + + await test.step('When the repository settings are saved unchanged', async () => { + await repoItem.clickEdit(); + await repoItem.clickSave(); + }); + + await test.step('Then the note tree of the other client picks the note up', async () => { + await expect(noteTree.noteLinkByName(noteName)).toHaveCount(1, { timeout: 20000 }); + }); + } finally { + if (noteId) { + await actor.request.delete(`/api/notebook/${noteId}`, { failOnStatusCode: false }); + } + } + }); + } +}); diff --git a/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts new file mode 100644 index 00000000000..e812b69f09d --- /dev/null +++ b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts @@ -0,0 +1,112 @@ +/* + * 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, Page } from '@playwright/test'; +import { addPageAnnotationBeforeEach, PAGES, waitForZeppelinReady } from '../../../utils'; + +// Both branches render REPO_ITEM; only the React branch has a mount host around +// it. Which branch is live is therefore a question about MOUNT. +const REPO_ITEM = '[data-testid="notebook-repo-item"]'; +const MOUNT = '[data-testid="react-notebook-repo-list"]'; +const MOUNTED_LIST = `${MOUNT} [data-testid="notebook-repo-list"]`; + +const openRepos = async (page: Page, query = ''): Promise => { + await page.goto(`/#/notebook-repos${query}`); + await waitForZeppelinReady(page); +}; + +const settingRows = (page: Page, root: string) => page.locator(`${root} tbody tr:not(.ant-table-placeholder)`); + +test.describe('Notebook Repository - React list behind a flag', () => { + addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_REPOS); + + test('without the flag, the Angular list renders', async ({ page }) => { + await openRepos(page); + + await expect(page.locator(REPO_ITEM).first()).toBeVisible(); + await expect(page.locator(MOUNT)).toHaveCount(0); + await expect(page.locator(`${REPO_ITEM} button:has-text("Edit")`).first()).toBeVisible(); + }); + + test('with reactNotebookRepos=true, the React list renders instead', async ({ page }) => { + await openRepos(page, '?reactNotebookRepos=true'); + + await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 }); + await expect(page.locator(REPO_ITEM).first()).toBeVisible(); + }); + + test('with a bare reactNotebookRepos flag, the React list renders', async ({ page }) => { + await openRepos(page, '?reactNotebookRepos'); + + await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 }); + }); + + test('both lists show the same repositories and settings', async ({ page }) => { + await openRepos(page); + await expect(page.locator(REPO_ITEM).first()).toBeVisible(); + const angularNames = await page + .locator(`${REPO_ITEM}`) + .evaluateAll(cards => cards.map(card => card.getAttribute('data-repo-name'))); + const angularRows = await settingRows(page, REPO_ITEM).allInnerTexts(); + + await openRepos(page, '?reactNotebookRepos=true'); + await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 }); + const reactNames = await page + .locator(`${MOUNT} ${REPO_ITEM}`) + .evaluateAll(cards => cards.map(card => card.getAttribute('data-repo-name'))); + // JUSTIFIED: prefer-web-first-assertions. Both sides must be read the same + // way to compare, and toHaveText() reads textContent, which loses the cell + // separator innerText adds. + const reactRows = await settingRows(page, `${MOUNT} ${REPO_ITEM}`).allInnerTexts(); + + // The host still owns the fetch and the sort, so the remote must not + // reshape what it is handed. + expect(reactNames).toEqual(angularNames); + expect(reactRows).toEqual(angularRows); + }); + + test('the React card switches to inputs on edit and back on cancel', async ({ page }) => { + await openRepos(page, '?reactNotebookRepos=true'); + await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 }); + + const card = page.locator(`${MOUNT} ${REPO_ITEM}`).first(); + const value = (await settingRows(page, `${MOUNT} ${REPO_ITEM}`).first().locator('td').nth(1).innerText()).trim(); + + await card.getByRole('button', { name: 'Edit' }).click(); + const input = card.locator('input').first(); + await expect(input).toBeVisible(); + await expect(input).toHaveValue(value); + + await card.getByRole('button', { name: 'Cancel' }).click(); + await expect(card.getByRole('button', { name: 'Edit' })).toBeVisible(); + await expect(card.locator('input')).toHaveCount(0); + }); + + test('when the remote fails to load, the Angular list renders', async ({ page }) => { + await test.step('Given a dead remote whose entry never loads', async () => { + await page.route('**/remoteEntry.js', route => route.abort()); + }); + + await test.step('When the page opens with the React list enabled', async () => { + // Angular is the default branch, so the assertions below pass even if the + // flag never took. Awaiting the request is what proves this is a fallback. + const remoteRequested = page.waitForRequest('**/remoteEntry.js'); + await openRepos(page, '?reactNotebookRepos=true'); + await remoteRequested; + }); + + await test.step('Then the Angular list takes over', async () => { + await expect(page.locator(REPO_ITEM).first()).toBeVisible({ timeout: 15000 }); + await expect(page.locator(MOUNT)).toHaveCount(0); + }); + }); +}); diff --git a/zeppelin-web-angular/projects/zeppelin-react/README.md b/zeppelin-web-angular/projects/zeppelin-react/README.md index e8b51bf97f7..9536a85420a 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/README.md +++ b/zeppelin-web-angular/projects/zeppelin-react/README.md @@ -77,7 +77,7 @@ Each React surface is behind a URL query flag, resolved by `ReactFeatureService` | `?react=false` | disabled | | flag absent | disabled | -Append `?react=true` to any published paragraph URL, `?reactFooter=true` to a notebook URL, or `?reactConfiguration=true` to the configuration URL to activate React mode. +Append `?react=true` to any published paragraph URL, `?reactFooter=true` to a notebook URL, `?reactConfiguration=true` to the configuration URL, or `?reactNotebookRepos=true` to the notebook repository URL to activate React mode. ## Setup diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/main.ts b/zeppelin-web-angular/projects/zeppelin-react/src/main.ts index ce7edc883f1..7c1bcb2e3be 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/main.ts +++ b/zeppelin-web-angular/projects/zeppelin-react/src/main.ts @@ -11,5 +11,6 @@ */ export { ConfigurationTable, mount as mountConfigurationTable } from './pages/ConfigurationTable'; +export { NotebookRepoList, mount as mountNotebookRepoList } from './pages/NotebookRepoList'; export { PublishedParagraph, mount } from './pages/PublishedParagraph'; export { ParagraphFooter, mount as mountParagraphFooter } from './components/paragraph/ParagraphFooter'; diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.spec.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.spec.tsx new file mode 100644 index 00000000000..06a24a95f8c --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.spec.tsx @@ -0,0 +1,172 @@ +/* + * 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 } from 'react'; +import { fireEvent, within } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { mount, NotebookRepo, NotebookRepoListMountHandle, NotebookRepoListProps } from './NotebookRepoList'; + +const gitRepo = (): NotebookRepo => ({ + name: 'GitNotebookRepo', + className: 'org.apache.zeppelin.notebook.repo.GitNotebookRepo', + settings: [{ type: 'INPUT', value: [], selected: '/opt/zeppelin/notebook', name: 'Notebook Path' }] +}); + +const dropdownRepo = (): NotebookRepo => ({ + name: 'S3NotebookRepo', + className: 'org.apache.zeppelin.notebook.repo.S3NotebookRepo', + settings: [{ type: 'DROPDOWN', value: ['us-east-1', 'eu-west-1'], selected: 'us-east-1', name: 'Region' }] +}); + +describe('NotebookRepoList mount contract', () => { + let host: HTMLElement | null = null; + let handle: NotebookRepoListMountHandle | null = null; + + const mountList = (props: NotebookRepoListProps): void => { + host = document.createElement('div'); + document.body.appendChild(host); + act(() => { + handle = mount(host as HTMLElement, props); + }); + }; + + const card = (repoName: string): HTMLElement => within(host!).getByText(repoName).closest('.ant-card') as HTMLElement; + + const clickButton = (repoName: string, name: RegExp): void => { + act(() => { + fireEvent.click(within(card(repoName)).getByRole('button', { name })); + }); + }; + + afterEach(() => { + if (handle) { + const h = handle; + act(() => h.unmount()); + handle = null; + } + host?.remove(); + host = null; + }); + + it('throws when no element is given', () => { + expect(() => mount(null as unknown as HTMLElement, {})).toThrow('Mount element is required'); + }); + + it('returns an update/unmount handle and renders one card per repository', () => { + mountList({ repositories: [gitRepo(), dropdownRepo()] }); + + expect(typeof handle!.update).toBe('function'); + expect(typeof handle!.unmount).toBe('function'); + expect(within(host!).getByText('GitNotebookRepo')).toBeTruthy(); + expect(within(host!).getByText('S3NotebookRepo')).toBeTruthy(); + }); + + it('shows each setting as name and value until the card is edited', () => { + mountList({ repositories: [gitRepo()] }); + + expect(within(host!).getByText('Notebook Path')).toBeTruthy(); + expect(within(host!).getByText('/opt/zeppelin/notebook')).toBeTruthy(); + expect(within(host!).queryByRole('textbox')).toBeNull(); + }); + + it('renders no cards when the host has no repositories yet', () => { + mountList({}); + + expect(host!.querySelector('[data-testid="notebook-repo-list"]')).not.toBeNull(); + expect(host!.querySelectorAll('.ant-card')).toHaveLength(0); + }); + + it('offers an input for INPUT settings and a dropdown for DROPDOWN settings', () => { + mountList({ repositories: [gitRepo(), dropdownRepo()] }); + + clickButton('GitNotebookRepo', /Edit/); + expect(within(card('GitNotebookRepo')).getByRole('textbox')).toBeTruthy(); + + clickButton('S3NotebookRepo', /Edit/); + expect(within(card('S3NotebookRepo')).getByRole('combobox')).toBeTruthy(); + }); + + it('reports the edited settings to the host on save', () => { + const onRepoChange = vi.fn(); + mountList({ repositories: [gitRepo()], onRepoChange }); + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: '/srv/notebook' } }); + }); + clickButton('GitNotebookRepo', /Save/); + + // The host owns the PUT, so what it receives is the whole repo with the + // edited value in place, not a partial patch. + expect(onRepoChange).toHaveBeenCalledTimes(1); + expect(onRepoChange.mock.calls[0][0]).toEqual({ + ...gitRepo(), + settings: [{ ...gitRepo().settings[0], selected: '/srv/notebook' }] + }); + }); + + it('leaves the host alone and restores the value on cancel', () => { + const onRepoChange = vi.fn(); + mountList({ repositories: [gitRepo()], onRepoChange }); + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: 'discard me' } }); + }); + clickButton('GitNotebookRepo', /Cancel/); + + expect(onRepoChange).not.toHaveBeenCalled(); + expect(within(host!).getByText('/opt/zeppelin/notebook')).toBeTruthy(); + clickButton('GitNotebookRepo', /Edit/); + expect(within(card('GitNotebookRepo')).getByRole('textbox')).toHaveProperty('value', '/opt/zeppelin/notebook'); + }); + + it('refuses to save a blank setting', () => { + const onRepoChange = vi.fn(); + mountList({ repositories: [gitRepo()], onRepoChange }); + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: ' ' } }); + }); + + // Matches the Angular form's required validator rather than sending a PUT + // the server would reject. + expect(within(card('GitNotebookRepo')).getByRole('button', { name: /Save/ })).toHaveProperty('disabled', true); + expect(onRepoChange).not.toHaveBeenCalled(); + }); + + it('shows the refetched values after the host updates the repositories', () => { + mountList({ repositories: [gitRepo()] }); + + const saved: NotebookRepo = { + ...gitRepo(), + settings: [{ ...gitRepo().settings[0], selected: '/srv/notebook' }] + }; + const h = handle!; + act(() => h.update({ repositories: [saved] })); + + expect(within(host!).getByText('/srv/notebook')).toBeTruthy(); + clickButton('GitNotebookRepo', /Edit/); + expect(within(card('GitNotebookRepo')).getByRole('textbox')).toHaveProperty('value', '/srv/notebook'); + }); + + it('unmount() empties the host element', () => { + mountList({ repositories: [gitRepo()] }); + const h = handle!; + handle = null; + + act(() => h.unmount()); + + expect(host!.innerHTML).toBe(''); + }); +}); diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx new file mode 100644 index 00000000000..ab33c4e8d90 --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx @@ -0,0 +1,182 @@ +/* + * 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, useState } from 'react'; +import { createRoot, Root } from 'react-dom/client'; +import { CloseOutlined, EditOutlined, SaveOutlined } from '@ant-design/icons'; +import { Button, Card, Input, Select, Space, Table } from 'antd'; +import { ReactErrorBoundary } from '@/components'; +import { ZeppelinThemeProvider } from '@/theme'; + +/** Mirrors the shell's NotebookRepoSettingsItem; the SDK does not declare it. */ +export interface NotebookRepoSetting { + type: string; + value: string[]; + selected: string; + name: string; +} + +export interface NotebookRepo { + name: string; + className: string; + settings: NotebookRepoSetting[]; +} + +export interface NotebookRepoListProps { + repositories?: NotebookRepo[]; + /** The host owns the PUT and the refetch; this only reports the edited repo. */ + onRepoChange?: (repo: NotebookRepo) => void; + onError?: (error: unknown) => void; +} + +// ng-zorro draws card titles and table headers at 500 where antd uses 600. +const REPO_TOKENS = { fontWeightStrong: 500 }; + +// The Angular card spaces itself with @card-padding-base from the default theme. +const CARD_GAP = 24; + +const isBlank = (value: string): boolean => value.trim().length === 0; + +interface RepoCardProps { + repo: NotebookRepo; + onRepoChange?: (repo: NotebookRepo) => void; +} + +const RepoCard = ({ repo, onRepoChange }: RepoCardProps) => { + const [editing, setEditing] = useState(false); + const [draft, setDraft] = useState(() => repo.settings.map(setting => setting.selected)); + + // A save reaches the host, which refetches and hands the repo back down. + // Rebuilding the draft on that keeps the form from showing stale values. + useEffect(() => { + setDraft(repo.settings.map(setting => setting.selected)); + }, [repo]); + + const invalid = draft.some(isBlank); + + const save = () => { + if (invalid) { + return; + } + onRepoChange?.({ + ...repo, + settings: repo.settings.map((setting, index) => ({ ...setting, selected: draft[index] })) + }); + setEditing(false); + }; + + const cancel = () => { + setDraft(repo.settings.map(setting => setting.selected)); + setEditing(false); + }; + + const setValue = (index: number, value: string) => + setDraft(current => current.map((entry, i) => (i === index ? value : entry))); + + const columns = [ + { title: 'Name', dataIndex: 'name', key: 'name', width: '30%' }, + { + title: 'Value', + key: 'value', + render: (_: unknown, setting: NotebookRepoSetting, index: number) => { + if (!editing) { + return setting.selected; + } + if (setting.type === 'DROPDOWN') { + return ( + setValue(index, event.target.value)} />; + } + } + ]; + + // Icons mirror the Angular card's nz-icon edit/save/close. + const extra = editing ? ( + + + + + ) : ( + + ); + + return ( + +

Setting

+ + columns={columns} + dataSource={repo.settings.map((setting, index) => ({ ...setting, key: `${setting.name}-${index}` }))} + size="small" + pagination={false} + /> +
+ ); +}; + +export const NotebookRepoList = ({ repositories = [], onRepoChange }: NotebookRepoListProps) => ( +
+ {repositories.map(repo => ( + + ))} +
+); + +export interface NotebookRepoListMountHandle { + update: (props: NotebookRepoListProps) => void; + unmount: () => void; +} + +export const mount = (element: HTMLElement, initialProps: NotebookRepoListProps): NotebookRepoListMountHandle => { + if (!element) { + throw new Error('Mount element is required'); + } + + const root: Root = createRoot(element); + + const renderWith = (props: NotebookRepoListProps) => { + root.render( + + + + + + ); + }; + + renderWith(initialProps); + + return { + update: (newProps: NotebookRepoListProps) => renderWith(newProps), + 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 d7de57b3d91..e64df09278b 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js +++ b/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js @@ -73,7 +73,8 @@ module.exports = (_env, argv) => { exposes: { './PublishedParagraph': './src/pages/PublishedParagraph', './ParagraphFooter': './src/components/paragraph/ParagraphFooter', - './ConfigurationTable': './src/pages/ConfigurationTable' + './ConfigurationTable': './src/pages/ConfigurationTable', + './NotebookRepoList': './src/pages/NotebookRepoList' } }), new HtmlWebpackPlugin({ diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/item/item.component.html b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/item/item.component.html index 58dd6a2af91..d456b664342 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/item/item.component.html +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/item/item.component.html @@ -10,7 +10,14 @@ ~ limitations under the License. --> - + @if (!editMode) {
diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.html b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.html index b3d37908e4c..f35dc30b4bf 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.html +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.html @@ -12,7 +12,15 @@ Manage your Notebook Repositories' settings.
- @for (repo of repositories; track repo) { - + @if (shouldUseReactList) { +
+ } @else { + @for (repo of repositories; track repo) { + + } }
diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.ts index afe62b26f38..c0abe58bd01 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.ts @@ -9,9 +9,12 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { ChangeDetectionStrategy, ChangeDetectorRef, Component, OnInit } from '@angular/core'; +import { ChangeDetectionStrategy, ChangeDetectorRef, Component, OnDestroy, OnInit } from '@angular/core'; +import { ActivatedRoute } from '@angular/router'; +import { Subject } from 'rxjs'; +import { takeUntil } from 'rxjs/operators'; import { NotebookRepo, NotebookRepoPutData } from '@zeppelin/interfaces'; -import { NotebookRepoService } from '@zeppelin/services'; +import { NotebookRepoService, ReactFeatureService } from '@zeppelin/services'; @Component({ selector: 'zeppelin-notebook-repos', @@ -20,18 +23,65 @@ import { NotebookRepoService } from '@zeppelin/services'; changeDetection: ChangeDetectionStrategy.OnPush, standalone: false }) -export class NotebookReposComponent implements OnInit { +export class NotebookReposComponent implements OnInit, OnDestroy { repositories: NotebookRepo[] = []; + useReactList = false; + reactListFailed = false; + + private destroy$ = new Subject(); + private lastReactListProps: Record | null = null; constructor( private notebookRepoService: NotebookRepoService, + private activatedRoute: ActivatedRoute, + private reactFeature: ReactFeatureService, private cdr: ChangeDetectorRef ) {} + get shouldUseReactList(): boolean { + return this.useReactList && !this.reactListFailed; + } + + // Memoized on repositories, the only input that changes. An object literal in + // the template would hand ReactMountDirective a new identity on every + // change-detection pass and make it call handle.update() each time. + get reactListProps(): Record { + if (this.lastReactListProps?.repositories !== this.repositories) { + this.lastReactListProps = { + repositories: this.repositories, + onRepoChange: this.onReactRepoChange, + onError: this.onReactListError + }; + } + return this.lastReactListProps; + } + + readonly onReactRepoChange = (repo: NotebookRepo): void => { + this.updateRepoSetting(repo); + }; + + readonly onReactListError = (error: unknown): void => { + console.error('React notebook repository list error', error); + this.reactListFailed = true; + this.cdr.markForCheck(); + }; + ngOnInit() { + // Subscribed rather than read once: navigating between /notebook-repos and + // /notebook-repos?reactNotebookRepos reuses this component, so a snapshot + // read would keep the flag it saw first. + this.activatedRoute.queryParamMap.pipe(takeUntil(this.destroy$)).subscribe(params => { + this.useReactList = this.reactFeature.isEnabled('notebookRepoList', params); + this.cdr.markForCheck(); + }); this.getRepos(); } + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } + getRepos() { this.notebookRepoService.getRepos().subscribe(data => { this.repositories = data.sort((a, b) => a.name.charCodeAt(0) - b.name.charCodeAt(0)); 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 6d0d3897eb5..9559fbb9279 100644 --- a/zeppelin-web-angular/src/app/services/react-feature.service.ts +++ b/zeppelin-web-angular/src/app/services/react-feature.service.ts @@ -13,7 +13,7 @@ import { Injectable } from '@angular/core'; import { parseBooleanFlag } from './query-flag.util'; -export type ReactSurface = 'publishedParagraph' | 'paragraphFooter' | 'configurationTable'; +export type ReactSurface = 'publishedParagraph' | 'paragraphFooter' | 'configurationTable' | 'notebookRepoList'; interface ReactSurfaceConfig { queryParam: string; @@ -32,6 +32,10 @@ const SURFACES: Record = { configurationTable: { queryParam: 'reactConfiguration', defaultEnabled: false + }, + notebookRepoList: { + queryParam: 'reactNotebookRepos', + defaultEnabled: false } }; diff --git a/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.spec.ts b/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.spec.ts index 71c5e64e30b..562cc8e5b64 100644 --- a/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.spec.ts +++ b/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.spec.ts @@ -118,4 +118,73 @@ describe('ReactMountDirective', () => { expect(update).toHaveBeenCalledOnce(); expect(zoneStates).toEqual([true, true]); }); + + it('re-enters the zone for host callbacks other than onError', async () => { + const host = new ElementRef(document.createElement('div')); + const ngZone = new NgZone({}); + let mountedProps: (ReactProps & ReactHostCallbacks) | undefined; + const remote: ReactExposedModule = { + mount: (_element: HTMLElement, props: ReactProps & ReactHostCallbacks) => { + mountedProps = props; + return { update: vi.fn(), unmount: vi.fn() }; + } + }; + const loadModule = vi.fn(async (): Promise => remote as T); + const loader = { loadModule } as Pick; + const zoneStates: boolean[] = []; + const received: unknown[] = []; + // A surface that hands the remote a real callback, the way the notebook + // repository list does with its save. Left unwrapped, the host's refetch + // and any HTTP it starts would run outside NgZone. + const onRepoChange = vi.fn((repo: unknown) => { + zoneStates.push(NgZone.isInAngularZone()); + received.push(repo); + }); + const directive = new ReactMountDirective(host, ngZone, loader as ReactRemoteLoaderService); + + directive.module = 'notebook-repos'; + directive.reactProps = { onRepoChange }; + directive.ngOnChanges({ + module: new SimpleChange(undefined, directive.module, true), + reactProps: new SimpleChange(undefined, directive.reactProps, true) + }); + await vi.waitFor(() => expect(mountedProps).toBeDefined()); + + ngZone.runOutsideAngular(() => { + (mountedProps!.onRepoChange as (repo: unknown) => void)({ name: 'GitNotebookRepo' }); + }); + + expect(zoneStates).toEqual([true]); + expect(received).toEqual([{ name: 'GitNotebookRepo' }]); + }); + + it('keeps non-function props as they are', async () => { + const host = new ElementRef(document.createElement('div')); + const ngZone = new NgZone({}); + let mountedProps: (ReactProps & ReactHostCallbacks) | undefined; + const remote: ReactExposedModule = { + mount: (_element: HTMLElement, props: ReactProps & ReactHostCallbacks) => { + mountedProps = props; + return { update: vi.fn(), unmount: vi.fn() }; + } + }; + const loader = { loadModule: vi.fn(async (): Promise => remote as T) } as Pick< + ReactRemoteLoaderService, + 'loadModule' + >; + const repositories = [{ name: 'GitNotebookRepo' }]; + const directive = new ReactMountDirective(host, ngZone, loader as ReactRemoteLoaderService); + + directive.module = 'notebook-repos'; + directive.reactProps = { repositories, readOnly: false }; + directive.ngOnChanges({ + module: new SimpleChange(undefined, directive.module, true), + reactProps: new SimpleChange(undefined, directive.reactProps, true) + }); + await vi.waitFor(() => expect(mountedProps).toBeDefined()); + + // Same references, so the remote can still memoize on them. + expect(mountedProps!.repositories).toBe(repositories); + expect(mountedProps!.readOnly).toBe(false); + }); }); diff --git a/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.ts b/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.ts index f981710bff1..1295e8e1239 100644 --- a/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.ts +++ b/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.ts @@ -125,22 +125,31 @@ export class ReactMountDirective implements OnChanges, OnDestroy { } } + /** + * Every function the host passes down, not just the `onError` ZEPPELIN-6565 + * covered. The remote runs outside the Angular zone, so a callback it invokes + * would leave the host's state change and any async work untracked by NgZone. + */ private withHostCallbacks(props: ReactProps & ReactHostCallbacks): ReactProps & ReactHostCallbacks { - const onError = props.onError; - if (typeof onError !== 'function') { + const entries = Object.entries(props).filter(([, value]) => typeof value === 'function'); + if (entries.length === 0) { return props; } - return { - ...props, - onError: (error: unknown): void => { + + const wrapped: ReactProps = { ...props }; + for (const [name, callback] of entries) { + wrapped[name] = (...args: unknown[]): void => { this.ngZone.run(() => { try { - onError(error); - } catch { - /* swallow callback errors; they shouldn't loop */ + (callback as (...callbackArgs: unknown[]) => void)(...args); + } catch (error) { + // Swallowed rather than rethrown: the caller is React, which would + // turn it into a render error in a tree the host does not own. + console.error(`[ReactMountDirective] host callback "${name}" threw`, error); } }); - } - }; + }; + } + return wrapped; } }