From 7cfca294ee4bd25cf40aecafdf3d7e7457ec4ee1 Mon Sep 17 00:00:00 2001 From: kimyenac Date: Sun, 30 Aug 2026 14:56:58 +0900 Subject: [PATCH 1/6] [ZEPPELIN-6682] Add e2e for the note tree reload after a repository save The resolved /notebook-repos set covers the form, its validation and the save round trip, but stops at the repository list. It never reaches what the server does next: NotebookRepoRestApi broadcasts a reloaded note list after a successful update, and the shell note tree is what consumes it. The spec drives two clients. 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 instead, which leaves the broadcast as the only thing that can change it, so one client watches the tree while the other saves. A note is created over REST between the two assertions. Creating one does not broadcast the list, so the tree has no way to know about it until the save arrives; without that step a tree that refreshed on its own would read as success. Dropping the updateRepo call fails the spec. The save leaves the settings as they are, the way the existing workflow spec does, so the repository configuration is unchanged. The note is removed again in a finally block rather than left to the folder cleanup, since it is created at the root to stay visible in a collapsed tree. The repository card carries a data-testid and the page models select on it rather than on zeppelin-notebook-repo-item and nz-table. This page is a migration seam, and a model pinned to the Angular element would take the whole set down the moment the React list takes over. --- .../e2e/models/notebook-repos-page.ts | 11 ++- ...ebook-repos-save-reloads-note-tree.spec.ts | 75 +++++++++++++++++++ .../notebook-repos/item/item.component.html | 9 ++- 3 files changed, 90 insertions(+), 5 deletions(-) create mode 100644 zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repos-save-reloads-note-tree.spec.ts 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..01e441c3e5d --- /dev/null +++ b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repos-save-reloads-note-tree.spec.ts @@ -0,0 +1,75 @@ +/* + * 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); + + test('a repository save reloads notebooks and refreshes the shell note tree', 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(); + + const reposPage = new NotebookReposPage(actor); + await reposPage.navigate(); + // JUSTIFIED: .first() picks the first configured repo; the page requires at least one. + const repoName = (await reposPage.repositoryItems.first().locator('.ant-card-head-title').textContent()) || ''; + 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/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) {
From e15e780a3785d2abc2a4e26cd340177c6c361e8a Mon Sep 17 00:00:00 2001 From: kimyenac Date: Wed, 2 Sep 2026 16:33:36 +0900 Subject: [PATCH 2/6] [ZEPPELIN-6631] Re-enter the Angular zone for every host callback ZEPPELIN-6565 wrapped onError so a remote-invoked error handler runs back inside NgZone. It stopped there, and every other function arriving through reactProps is still handed to the remote untouched. That was fine while onError was the only callback any surface passed. The notebook repository list is the first to hand the remote a real one: its save calls back into the host, which then issues the PUT and refetches the list. Outside the zone that work is untracked, so the refresh lands late or not at all, which is the failure ZEPPELIN-6631 asks to check for before building on top of it. withHostCallbacks now wraps every function-valued prop. A callback that throws is logged rather than rethrown, since the caller is React and an exception would surface as a render error in a tree the host does not own. Non-function props keep their identity so the remote can still memoize on them. --- .../react-mount/react-mount.directive.spec.ts | 69 +++++++++++++++++++ .../react-mount/react-mount.directive.ts | 31 ++++++--- 2 files changed, 90 insertions(+), 10 deletions(-) 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..b5c4c403989 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,33 @@ export class ReactMountDirective implements OnChanges, OnDestroy { } } + /** + * Every function the host passes down, not only `onError`. The remote runs + * outside the Angular zone, so a callback it invokes would otherwise leave + * the host's state change and any async work it starts untracked by NgZone. + * ZEPPELIN-6565 covered `onError`; a surface that hands the remote a real + * callback needs the same for all of them. + */ 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; } } From be94ef74648c9b1bf88973b9ae11a24991b8f65e Mon Sep 17 00:00:00 2001 From: kimyenac Date: Tue, 8 Sep 2026 17:42:28 +0900 Subject: [PATCH 3/6] [ZEPPELIN-6631] Render the notebook repository list through a React remote behind a flag The list and its edit surface move to the React remote behind ?reactNotebookRepos, while Angular keeps the route, NotebookRepoService, the PUT, the refetch that follows it, and the note-tree refresh the server broadcasts. Only the repository cards inside .content change hands. The host follows the shape ZEPPELIN-6630 established for the configuration table rather than inventing a second one: a shouldUseReactList getter that folds the flag together with a mount failure, props memoized on the only input that changes, and queryParamMap subscribed rather than read once, because navigating between /notebook-repos and /notebook-repos? reactNotebookRepos reuses the component. An onError from the remote falls back to the Angular list for the rest of the session. The remote owns no state beyond the open editor and its draft. A save hands the whole repo back to the host with the edited values in place, not a partial patch, since the host is what issues the PUT and then feeds the refetched list back down. The draft is rebuilt when that new list arrives, so a card cannot keep showing values the server has already replaced. Blank settings are refused in the remote the way the Angular form's required validator refuses them, rather than sending a PUT the server would reject. NotebookRepo and its setting type are declared in the remote because the SDK does not carry them; the shell's own NotebookRepoSettingsItem is an Angular interface the remote cannot import. REPO_TOKENS passes fontWeightStrong through the theme provider for the same reason the configuration table does: ng-zorro draws card titles and table headers at 500 where antd uses 600. --- .../projects/zeppelin-react/src/main.ts | 1 + .../src/pages/NotebookRepoList.spec.tsx | 172 +++++++++++++++++ .../src/pages/NotebookRepoList.tsx | 177 ++++++++++++++++++ .../projects/zeppelin-react/webpack.config.js | 3 +- .../notebook-repos.component.html | 12 +- .../notebook-repos.component.ts | 56 +++++- .../src/app/services/react-feature.service.ts | 6 +- 7 files changed, 420 insertions(+), 7 deletions(-) create mode 100644 zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.spec.tsx create mode 100644 zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx 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..8adf121d7cb --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx @@ -0,0 +1,177 @@ +/* + * 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 { 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 }; + +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)} />; + } + } + ]; + + 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/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 } }; From f69a94edafdefda621b72be497043b09c43dd76f Mon Sep 17 00:00:00 2001 From: kimyenac Date: Tue, 8 Sep 2026 17:42:39 +0900 Subject: [PATCH 4/6] [ZEPPELIN-6631] Add e2e coverage for the notebook repository list's two branches Two specs, for the two things a flagged surface has to prove. react-notebook-repo-list.spec.ts covers which branch is live and that they agree. Both branches render the notebook-repo-item id, so the question is about the mount host around it, not the card. The parity test reads the repository names and setting rows from each branch and compares them: the host still owns the fetch and the sort, so a remote that reshaped what it was handed would show up here. The fallback test aborts remoteEntry.js and awaits the request before asserting, because Angular is the default branch and the assertions would otherwise pass on a flag that never took. notebook-repos-save-reloads-note-tree.spec.ts, from ZEPPELIN-6682, now runs on both branches. This is what ZEPPELIN-6631 asks for before it can close: the reload is the host's job either way, so a React list that swallowed the save would surface as a note tree that never picks the note up. The body is unchanged apart from reading the repository name from data-repo-name, which both branches carry, rather than from the ng-zorro card title. --- ...ebook-repos-save-reloads-note-tree.spec.ts | 87 +++++++------ .../react-notebook-repo-list.spec.ts | 114 ++++++++++++++++++ 2 files changed, 162 insertions(+), 39 deletions(-) create mode 100644 zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts 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 index 01e441c3e5d..ab4f321052d 100644 --- 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 @@ -27,49 +27,58 @@ const createNoteAtRoot = async (page: Page, name: string): Promise => { test.describe('Notebook Repository - save reloads the note tree', () => { addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_REPOS); - test('a repository save reloads notebooks and refreshes the shell note tree', 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 = ''; + // 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(); + try { + await watcher.goto('/#/'); + await waitForZeppelinReady(watcher); + const noteTree = new NodeListPage(watcher); + await expect(noteTree.nodeListContainer).toBeVisible(); - const reposPage = new NotebookReposPage(actor); - await reposPage.navigate(); - // JUSTIFIED: .first() picks the first configured repo; the page requires at least one. - const repoName = (await reposPage.repositoryItems.first().locator('.ant-card-head-title').textContent()) || ''; - const repoItem = new NotebookRepoItemPage(actor, repoName); + 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('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('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 }); + 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..d641946b051 --- /dev/null +++ b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts @@ -0,0 +1,114 @@ +/* + * 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 have to be read the + // same way to be comparable, and the expected side was captured from a page + // load that is gone by now. toHaveText() reads textContent, which drops the + // cell separator innerText inserts, so mixing the two would compare + // "Notebook Path\t/opt/zeppelin" against "Notebook Path/opt/zeppelin". + 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); + }); + }); +}); From 054b378016f0b49a88578db76bc83a1c19018295 Mon Sep 17 00:00:00 2001 From: kimyenac Date: Tue, 8 Sep 2026 17:42:45 +0900 Subject: [PATCH 5/6] [ZEPPELIN-6631] List the notebook repository list among the React surfaces Both places that enumerate the flagged surfaces stopped at the configuration table. e2e/AGENTS.md also counted them, so the count moves with the list. --- zeppelin-web-angular/e2e/AGENTS.md | 2 +- zeppelin-web-angular/projects/zeppelin-react/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/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 From 4db1ced01aa51ec7838f7d71790fdf832c373b06 Mon Sep 17 00:00:00 2001 From: kimyenac Date: Tue, 8 Sep 2026 18:05:15 +0900 Subject: [PATCH 6/6] [ZEPPELIN-6631] Match the Angular card's spacing and button icons Review follow-ups on the React list, all parity rather than behaviour. The card gap was hardcoded at 16px where the Angular card spaces itself with @card-padding-base, 24px in the default theme, so the two lists did not line up side by side. The buttons were missing the edit, save and close icons the Angular card draws through nz-icon. Two comments that restated their own code are shorter: the withHostCallbacks docblock and the prefer-web-first-assertions justification. --- .../notebook-repos/react-notebook-repo-list.spec.ts | 8 +++----- .../zeppelin-react/src/pages/NotebookRepoList.tsx | 13 +++++++++---- .../app/share/react-mount/react-mount.directive.ts | 8 +++----- 3 files changed, 15 insertions(+), 14 deletions(-) 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 index d641946b051..e812b69f09d 100644 --- 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 @@ -63,11 +63,9 @@ test.describe('Notebook Repository - React list behind a flag', () => { 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 have to be read the - // same way to be comparable, and the expected side was captured from a page - // load that is gone by now. toHaveText() reads textContent, which drops the - // cell separator innerText inserts, so mixing the two would compare - // "Notebook Path\t/opt/zeppelin" against "Notebook Path/opt/zeppelin". + // 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 diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx index 8adf121d7cb..ab33c4e8d90 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx @@ -12,6 +12,7 @@ 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'; @@ -40,6 +41,9 @@ export interface NotebookRepoListProps { // 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 { @@ -103,17 +107,18 @@ const RepoCard = ({ repo, onRepoChange }: RepoCardProps) => { } ]; + // Icons mirror the Angular card's nz-icon edit/save/close. const extra = editing ? ( - - ) : ( - ); @@ -123,7 +128,7 @@ const RepoCard = ({ repo, onRepoChange }: RepoCardProps) => { title={repo.name} extra={extra} size="small" - style={{ marginBottom: 16 }} + style={{ marginBottom: CARD_GAP }} data-testid="notebook-repo-item" data-repo-name={repo.name} > 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 b5c4c403989..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 @@ -126,11 +126,9 @@ export class ReactMountDirective implements OnChanges, OnDestroy { } /** - * Every function the host passes down, not only `onError`. The remote runs - * outside the Angular zone, so a callback it invokes would otherwise leave - * the host's state change and any async work it starts untracked by NgZone. - * ZEPPELIN-6565 covered `onError`; a surface that hands the remote a real - * callback needs the same for all of them. + * 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 entries = Object.entries(props).filter(([, value]) => typeof value === 'function');