From 7cfca294ee4bd25cf40aecafdf3d7e7457ec4ee1 Mon Sep 17 00:00:00 2001 From: kimyenac Date: Sun, 30 Aug 2026 14:56:58 +0900 Subject: [PATCH 1/9] [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 f047b44b2e60..5caa33edc824 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 000000000000..01e441c3e5dc --- /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 58dd6a2af914..d456b6643421 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/9] [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 71c5e64e30b8..562cc8e5b64a 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 f981710bff1c..b5c4c403989d 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/9] [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 ce7edc883f1f..7c1bcb2e3be7 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 000000000000..06a24a95f8c3 --- /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 000000000000..8adf121d7cbc --- /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 d7de57b3d91d..e64df09278b4 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 b3d37908e4c2..f35dc30b4bfc 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 afe62b26f381..c0abe58bd01a 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 6d0d3897eb58..9559fbb9279e 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/9] [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 01e441c3e5dc..ab4f321052d2 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 000000000000..d641946b051e --- /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/9] [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 35a7da5d978e..a45bf08fa1f7 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 e8b51bf97f70..9536a85420a5 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/9] [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 d641946b051e..e812b69f09d1 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 8adf121d7cbc..ab33c4e8d900 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 b5c4c403989d..1295e8e12396 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'); From aa573b751576e26749c4357e15f9a6274195aba2 Mon Sep 17 00:00:00 2001 From: YONGJAE LEE Date: Fri, 11 Sep 2026 01:02:22 +0900 Subject: [PATCH 7/9] [ZEPPELIN-6631] Fix React repo-list save/cancel race condition --- .../src/pages/NotebookRepoList.spec.tsx | 267 +++++++++++++++++- .../src/pages/NotebookRepoList.tsx | 107 +++++-- .../notebook-repos.component.spec.ts | 175 ++++++++++++ .../notebook-repos.component.ts | 45 ++- 4 files changed, 564 insertions(+), 30 deletions(-) create mode 100644 zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.spec.ts 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 index 06a24a95f8c3..2fd93ae59cba 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.spec.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.spec.tsx @@ -105,8 +105,7 @@ describe('NotebookRepoList mount contract', () => { }); 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. + // The host owns the PUT, so it gets the whole repo, not a partial patch. expect(onRepoChange).toHaveBeenCalledTimes(1); expect(onRepoChange.mock.calls[0][0]).toEqual({ ...gitRepo(), @@ -114,6 +113,28 @@ describe('NotebookRepoList mount contract', () => { }); }); + it('reports the selected option to the host when a DROPDOWN setting is saved', () => { + const onRepoChange = vi.fn(); + mountList({ repositories: [dropdownRepo()], onRepoChange }); + + clickButton('S3NotebookRepo', /Edit/); + const combobox = within(card('S3NotebookRepo')).getByRole('combobox'); + act(() => { + fireEvent.mouseDown(combobox); + }); + // antd renders the option list in a portal, not inside the card. + act(() => { + fireEvent.click(within(document.body).getByTitle('eu-west-1')); + }); + clickButton('S3NotebookRepo', /Save/); + + expect(onRepoChange).toHaveBeenCalledTimes(1); + expect(onRepoChange.mock.calls[0][0]).toEqual({ + ...dropdownRepo(), + settings: [{ ...dropdownRepo().settings[0], selected: 'eu-west-1' }] + }); + }); + it('leaves the host alone and restores the value on cancel', () => { const onRepoChange = vi.fn(); mountList({ repositories: [gitRepo()], onRepoChange }); @@ -130,6 +151,22 @@ describe('NotebookRepoList mount contract', () => { expect(within(card('GitNotebookRepo')).getByRole('textbox')).toHaveProperty('value', '/opt/zeppelin/notebook'); }); + it('keeps showing the saved value if the card is cancelled before the host refetch lands', () => { + 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/); + clickButton('GitNotebookRepo', /Edit/); + clickButton('GitNotebookRepo', /Cancel/); + + expect(within(host!).getByText('/srv/notebook')).toBeTruthy(); + expect(within(host!).queryByText('/opt/zeppelin/notebook')).toBeNull(); + }); + it('refuses to save a blank setting', () => { const onRepoChange = vi.fn(); mountList({ repositories: [gitRepo()], onRepoChange }); @@ -139,12 +176,234 @@ describe('NotebookRepoList mount contract', () => { 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 saved value immediately, before the host refetch lands', () => { + 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/); + + expect(within(host!).getByText('/srv/notebook')).toBeTruthy(); + expect(within(host!).queryByText('/opt/zeppelin/notebook')).toBeNull(); + }); + + it('keeps in-progress edits when an unrelated refetch updates the list', () => { + mountList({ repositories: [gitRepo(), dropdownRepo()] }); + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: 'still typing' } }); + }); + + const h = handle!; + act(() => h.update({ repositories: [gitRepo(), dropdownRepo()] })); + + expect(within(card('GitNotebookRepo')).getByRole('textbox')).toHaveProperty('value', 'still typing'); + expect(within(card('GitNotebookRepo')).getByRole('button', { name: /Save/ })).toBeTruthy(); + }); + + it('resyncs the draft if the setting count changes mid-edit, instead of indexing past it', () => { + mountList({ repositories: [gitRepo()] }); + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: 'still typing' } }); + }); + + // The backing NotebookRepo now reports one more setting than it did when editing started - + // e.g. its settings schema isn't stable. `draft` was sized for the old, shorter + // `repo.settings`. + const grown = { + ...gitRepo(), + settings: [...gitRepo().settings, { type: 'INPUT', value: [], selected: '/srv/second', name: 'Second Path' }] + }; + const h = handle!; + act(() => h.update({ repositories: [grown] })); + + // Resynced to the host's latest values for both rows - not the stale in-progress edit, and + // not `draft[1]` read as undefined for the row that didn't exist when `draft` was last sized. + const textboxes = within(card('GitNotebookRepo')).getAllByRole('textbox'); + expect(textboxes).toHaveLength(2); + expect(textboxes[0]).toHaveProperty('value', '/opt/zeppelin/notebook'); + expect(textboxes[1]).toHaveProperty('value', '/srv/second'); + }); + + it('resyncs the draft if the setting count shrinks mid-edit, instead of leaving a stale blank key stuck', () => { + const twoSettings: NotebookRepo = { + ...gitRepo(), + settings: [...gitRepo().settings, { type: 'INPUT', value: [], selected: '/srv/second', name: 'Second Path' }] + }; + mountList({ repositories: [twoSettings] }); + + clickButton('GitNotebookRepo', /Edit/); + const textboxesBefore = within(card('GitNotebookRepo')).getAllByRole('textbox'); + act(() => { + // Blanking this row correctly disables Save. It's about to disappear from `repo.settings` + // below - its now-stale key would keep `invalid` true forever if the draft is never rebuilt + // to drop it. + fireEvent.change(textboxesBefore[1], { target: { value: '' } }); + }); + expect(within(card('GitNotebookRepo')).getByRole('button', { name: /Save/ })).toHaveProperty('disabled', true); + + // The backing NotebookRepo drops the now-blank setting entirely - the length half of + // `namesUnchanged` is what catches this; the `every(...)` half alone would still read as + // "unchanged", since every remaining name is still present in `draft`. + const h = handle!; + act(() => h.update({ repositories: [gitRepo()] })); + + // Resynced: the stale blank key is gone, so Save is enabled again for the one remaining, + // non-blank row. + expect(within(card('GitNotebookRepo')).getAllByRole('textbox')).toHaveLength(1); + expect(within(card('GitNotebookRepo')).getByRole('button', { name: /Save/ })).toHaveProperty('disabled', false); + }); + + it('matches saved values to the right setting by name, even if repo.settings is reordered mid-edit', () => { + const original: NotebookRepo = { + name: 'GitNotebookRepo', + className: 'org.apache.zeppelin.notebook.repo.GitNotebookRepo', + settings: [ + { type: 'INPUT', value: [], selected: '/opt/zeppelin/notebook', name: 'Notebook Path' }, + { type: 'INPUT', value: [], selected: 'true', name: 'One Way Sync' } + ] + }; + const onRepoChange = vi.fn(); + mountList({ repositories: [original], onRepoChange }); + + clickButton('GitNotebookRepo', /Edit/); + const textboxesBefore = within(card('GitNotebookRepo')).getAllByRole('textbox'); + act(() => { + fireEvent.change(textboxesBefore[0], { target: { value: '/srv/notebook' } }); + }); + + // Same setting names, different order - e.g. a refetch whose response order isn't guaranteed + // to match the last one. Same length and names, so the guard keeps this card in edit mode + // rather than discarding the in-progress edit above. + const reordered: NotebookRepo = { ...original, settings: [original.settings[1], original.settings[0]] }; + const h = handle!; + act(() => h.update({ repositories: [reordered], onRepoChange })); + + clickButton('GitNotebookRepo', /Save/); + + expect(onRepoChange).toHaveBeenCalledTimes(1); + const submitted = onRepoChange.mock.calls[0][0] as NotebookRepo; + const byName = Object.fromEntries(submitted.settings.map(setting => [setting.name, setting.selected])); + expect(byName['Notebook Path']).toBe('/srv/notebook'); + expect(byName['One Way Sync']).toBe('true'); + }); + + // KNOWN LIMITATION (ZEPPELIN-6707): unlike the editing case above, a card with pendingSave + // still true is not protected from an unrelated card's refetch, because the resync effect only + // checks `editingRef`. + // This test pins that current (self-correcting, not data-losing) behavior rather than asserting + // the fix - see the ticket for why a value/reference check alone can't distinguish "my own + // confirming refetch" from "someone else's". + // Its first half exercises the same guard as "accepts a refetch even when it differs from what + // was submitted" below (a single-card refetch reads the same at the component level as an + // unrelated card's list-wide one); the second-card list and the resync-after-flicker assertion + // at the end are what this test actually adds. + it('flickers back to the stale server value if an unrelated refetch lands before its own pendingSave confirms', () => { + mountList({ repositories: [gitRepo(), dropdownRepo()] }); + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: '/srv/notebook' } }); + }); + clickButton('GitNotebookRepo', /Save/); + expect(within(host!).getByText('/srv/notebook')).toBeTruthy(); + + // Another card's save resolves first and triggers the host's list-wide refetch; this card's + // own PUT hasn't landed yet, so the refetch still carries its pre-edit value. + const h = handle!; + act(() => h.update({ repositories: [gitRepo(), dropdownRepo()] })); + expect(within(host!).getByText('/opt/zeppelin/notebook')).toBeTruthy(); + expect(within(host!).queryByText('/srv/notebook')).toBeNull(); + + // This card's own refetch arrives moments later and self-corrects. + act(() => + h.update({ + repositories: [ + { ...gitRepo(), settings: [{ ...gitRepo().settings[0], selected: '/srv/notebook' }] }, + dropdownRepo() + ] + }) + ); + expect(within(host!).getByText('/srv/notebook')).toBeTruthy(); + }); + + it('accepts a refetch even when it differs from what was submitted', () => { + // Plugin repos' updateSettings is a no-op and VFS/Git normalize the path, + // so the host can legitimately refetch something else; that has to win. + mountList({ repositories: [gitRepo()], onRepoChange: vi.fn() }); + const h = handle!; + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: '/srv/notebook' } }); + }); + clickButton('GitNotebookRepo', /Save/); + expect(within(host!).getByText('/srv/notebook')).toBeTruthy(); + + act(() => h.update({ repositories: [gitRepo()] })); + + expect(within(host!).getByText('/opt/zeppelin/notebook')).toBeTruthy(); + expect(within(host!).queryByText('/srv/notebook')).toBeNull(); + }); + + it('restores the saved value, not a further in-progress edit, on cancel before the refetch lands', () => { + 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/); + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: 'garbage' } }); + }); + clickButton('GitNotebookRepo', /Cancel/); + + expect(within(host!).getByText('/srv/notebook')).toBeTruthy(); + expect(within(host!).queryByText('garbage')).toBeNull(); + expect(onRepoChange).toHaveBeenCalledTimes(1); + }); + + it('resyncs to a refetch that landed mid-edit once the edit is cancelled', () => { + const onRepoChange = vi.fn(); + mountList({ repositories: [gitRepo()], onRepoChange }); + const h = handle!; + + clickButton('GitNotebookRepo', /Edit/); + act(() => { + fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), { target: { value: '/srv/notebook' } }); + }); + clickButton('GitNotebookRepo', /Save/); + + // The [repo] effect skips this refetch since the card is already back in edit mode by the + // time it lands. + clickButton('GitNotebookRepo', /Edit/); + act(() => + h.update({ + repositories: [{ ...gitRepo(), settings: [{ ...gitRepo().settings[0], selected: '/opt/zeppelin/notebook' }] }] + }) + ); + clickButton('GitNotebookRepo', /Cancel/); + + expect(within(host!).getByText('/opt/zeppelin/notebook')).toBeTruthy(); + expect(within(host!).queryByText('/srv/notebook')).toBeNull(); + + clickButton('GitNotebookRepo', /Edit/); + expect(within(card('GitNotebookRepo')).getByRole('textbox')).toHaveProperty('value', '/opt/zeppelin/notebook'); + }); + it('shows the refetched values after the host updates the repositories', () => { mountList({ repositories: [gitRepo()] }); 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 ab33c4e8d900..af169f928d41 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx @@ -10,7 +10,7 @@ * limitations under the License. */ -import { useEffect, useState } from 'react'; +import { useEffect, useRef, 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'; @@ -44,6 +44,7 @@ const REPO_TOKENS = { fontWeightStrong: 500 }; // The Angular card spaces itself with @card-padding-base from the default theme. const CARD_GAP = 24; +// Stricter than the Angular form's Validators.required, which accepts whitespace-only input. const isBlank = (value: string): boolean => value.trim().length === 0; interface RepoCardProps { @@ -51,58 +52,122 @@ interface RepoCardProps { onRepoChange?: (repo: NotebookRepo) => void; } +// `setting.name` is the join key everywhere below (draft, save, cancel, render), never array +// position, since `repo.settings` order isn't guaranteed stable between refetches and a +// same-length reorder mid-edit would otherwise submit a value under the wrong name. +const draftFromSettings = (settings: NotebookRepoSetting[]): Record => + Object.fromEntries(settings.map(setting => [setting.name, setting.selected])); + 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. + const [draft, setDraft] = useState>(() => draftFromSettings(repo.settings)); + // Set on save, cleared by the next `repo` prop change (the host's refetch, success or failure, + // is the source of truth for what got persisted). View mode reads the draft while this is true, + // so the card shows the saved value instead of the pre-edit one during the round trip. Not + // gated on the refetch matching what was submitted, since several NotebookRepo implementations + // legitimately refetch something else (plugin repos' updateSettings is a no-op, VFS/Git + // normalize the path), and that has to win immediately. + const [pendingSave, setPendingSave] = useState(false); + const editingRef = useRef(editing); + editingRef.current = editing; + // The draft submitted by the most recent unconfirmed save, paired with the `repo` it was made + // against. Lets cancel() tell apart "no refetch yet" (repo unchanged, restore the saved draft) + // from "a refetch already landed mid-edit" (repo changed, that refetch wins). + const savedRef = useRef<{ repo: NotebookRepo; draft: Record } | null>(null); + + // Rebuilds the draft from a refetched repo. Skipped while editing, so an unrelated refetch + // (e.g. another card's save) can't discard in-progress keystrokes. Deps only on `repo`, so + // toggling `editing` doesn't re-run this against a repo prop the host hasn't updated yet. Still + // resyncs if the setting names changed underneath the edit, since view mode and save() look + // values up by name, so a missing name would render/submit `undefined`. + // + // KNOWN LIMITATION (ZEPPELIN-6707): this only guards an in-progress *edit*, not an in-progress + // *pendingSave* on a different card. If card B calls save() and, before B's own refetch lands, + // card A's independent save triggers the host's list-wide getRepos(), B isn't editing so this + // effect fires, resets B's pendingSave to B's still-stale server value, and B's display + // flickers backward before jumping forward again once B's own refetch arrives. A real fix needs + // the host to tell B's refetch apart from A's, since value or reference comparison alone can't + // (see the ticket). useEffect(() => { - setDraft(repo.settings.map(setting => setting.selected)); + // Object.hasOwn, not `in`, since `in` walks the prototype chain, so a setting literally named + // e.g. "toString" would read as already present. + const namesUnchanged = + repo.settings.length === Object.keys(draft).length && + repo.settings.every(setting => Object.hasOwn(draft, setting.name)); + if (editingRef.current && namesUnchanged) { + return; + } + savedRef.current = null; + setDraft(draftFromSettings(repo.settings)); + setPendingSave(false); + // Deliberately `[repo]` only, not `draft`, since the effect must run exactly when `repo` + // changes, not when `draft` does (typing would retrigger it). `draft` inside the guard is + // read from this render's closure regardless of what's in the deps array, so it's always + // current when the effect actually runs. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [repo]); - const invalid = draft.some(isBlank); + const invalid = Object.values(draft).some(isBlank); const save = () => { if (invalid) { return; } + const values = { ...draft }; onRepoChange?.({ ...repo, - settings: repo.settings.map((setting, index) => ({ ...setting, selected: draft[index] })) + settings: repo.settings.map(setting => ({ ...setting, selected: values[setting.name] })) }); + savedRef.current = { repo, draft: values }; + setPendingSave(true); setEditing(false); }; const cancel = () => { - setDraft(repo.settings.map(setting => setting.selected)); + // Pending and still against the same repo: restore the saved draft, not the stale pre-edit + // `repo` value or a further in-progress edit. Otherwise a refetch already moved `repo` on, so + // defer to it instead. + const saved = savedRef.current; + if (pendingSave && saved && saved.repo === repo) { + setDraft(saved.draft); + } else { + savedRef.current = null; + setPendingSave(false); + setDraft(draftFromSettings(repo.settings)); + } setEditing(false); }; - const setValue = (index: number, value: string) => - setDraft(current => current.map((entry, i) => (i === index ? value : entry))); + const setValue = (name: string, value: string) => setDraft(current => ({ ...current, [name]: value })); const columns = [ { title: 'Name', dataIndex: 'name', key: 'name', width: '30%' }, { title: 'Value', key: 'value', - render: (_: unknown, setting: NotebookRepoSetting, index: number) => { + render: (_: unknown, setting: NotebookRepoSetting) => { if (!editing) { - return setting.selected; + // While waiting on the host's refetch, show what was just saved rather than the pre-edit value still sitting in `repo`. + return pendingSave ? draft[setting.name] : setting.selected; } if (setting.type === 'DROPDOWN') { return ( setValue(index, event.target.value)} />; + return ( + setValue(setting.name, event.target.value)} + /> + ); } } ]; @@ -145,8 +210,14 @@ const RepoCard = ({ repo, onRepoChange }: RepoCardProps) => { export const NotebookRepoList = ({ repositories = [], onRepoChange }: NotebookRepoListProps) => (
- {repositories.map(repo => ( - + {repositories.map((repo, index) => ( + // Suffixed with the index, since zeppelin.notebook.storage isn't deduped server-side, so + // className alone would collide if listed twice. Trade-off: repo objects carry no stable + // id, so a same-length reorder (the host sorts by `name.charCodeAt(0)` only, which doesn't + // fully order same-first-letter names) changes this key too, remounting the card and losing + // any in-progress edit - the index can't tell "this repo moved" from "a different repo is + // now here". + ))}
); diff --git a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.spec.ts b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.spec.ts new file mode 100644 index 000000000000..e2286b0534c9 --- /dev/null +++ b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/notebook-repos.component.spec.ts @@ -0,0 +1,175 @@ +/* + * 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 { ChangeDetectorRef } from '@angular/core'; +import { ActivatedRoute } from '@angular/router'; +import { NzMessageService } from 'ng-zorro-antd/message'; +import { of, Subject, throwError } from 'rxjs'; +import { describe, expect, it, vi } from 'vitest'; + +import { NotebookRepo } from '@zeppelin/interfaces'; +import { NotebookRepoService, ReactFeatureService } from '@zeppelin/services'; + +import { NotebookReposComponent } from './notebook-repos.component'; + +const repo = (overrides: Partial = {}): NotebookRepo => ({ + name: 'GitNotebookRepo', + className: 'org.apache.zeppelin.notebook.repo.GitNotebookRepo', + settings: [{ type: 'INPUT', value: [], selected: '/opt/zeppelin/notebook', name: 'Notebook Path' }], + ...overrides +}); + +const createComponent = ( + notebookRepoService: Partial, + overrides: { activatedRoute?: ActivatedRoute; reactFeature?: Partial } = {} +) => { + const cdr = { markForCheck: vi.fn() } as unknown as ChangeDetectorRef; + const nzMessageService = { error: vi.fn() } as unknown as NzMessageService; + const activatedRoute = + overrides.activatedRoute ?? ({ queryParamMap: of({ get: () => null }) } as unknown as ActivatedRoute); + const reactFeature = (overrides.reactFeature ?? { isEnabled: () => false }) as unknown as ReactFeatureService; + const component = new NotebookReposComponent( + notebookRepoService as NotebookRepoService, + activatedRoute, + reactFeature, + cdr, + nzMessageService + ); + return { component, cdr, nzMessageService }; +}; + +describe('NotebookReposComponent', () => { + it('sorts and stores the repositories on a successful fetch', () => { + const data = [repo({ name: 'ZNotebookRepo', className: 'Z' }), repo({ name: 'ANotebookRepo', className: 'A' })]; + const { component, cdr } = createComponent({ getRepos: () => of(data) }); + + component.getRepos(); + + expect(component.repositories.map(r => r.name)).toEqual(['ANotebookRepo', 'ZNotebookRepo']); + expect(cdr.markForCheck).toHaveBeenCalled(); + }); + + it('surfaces a message and leaves `repositories` untouched when the fetch fails', () => { + const { component, nzMessageService } = createComponent({ getRepos: () => throwError(() => new Error('boom')) }); + const before = component.repositories; + + component.getRepos(); + + expect(nzMessageService.error).toHaveBeenCalledTimes(1); + // Same reference: reactListProps memoizes on this identity, so a silent failure here would + // otherwise leave the React card's pendingSave stuck. + expect(component.repositories).toBe(before); + }); + + it('refetches after a successful save', () => { + const getRepos = vi.fn(() => of([])); + const { component } = createComponent({ getRepos, updateRepo: () => of({}) }); + + component.updateRepoSetting(repo()); + + expect(getRepos).toHaveBeenCalledTimes(1); + }); + + it('surfaces a message and still refetches when a save fails', () => { + const getRepos = vi.fn(() => of([])); + const { component, nzMessageService } = createComponent({ + getRepos, + updateRepo: () => throwError(() => new Error('boom')) + }); + + component.updateRepoSetting(repo()); + + expect(nzMessageService.error).toHaveBeenCalledTimes(1); + expect(getRepos).toHaveBeenCalledTimes(1); + }); + + it('sends the edited repo settings as a flat name/value map', () => { + const updateRepo = vi.fn(() => of({})); + const { component } = createComponent({ getRepos: () => of([]), updateRepo }); + + component.updateRepoSetting( + repo({ + settings: [ + { type: 'INPUT', value: [], selected: '/srv/notebook', name: 'Notebook Path' }, + { type: 'INPUT', value: [], selected: 'true', name: 'One Way Sync' } + ] + }) + ); + + expect(updateRepo).toHaveBeenCalledWith({ + name: repo().className, + settings: { 'Notebook Path': '/srv/notebook', 'One Way Sync': 'true' } + }); + }); + + it('ignores an in-flight fetch that resolves after the component is destroyed', () => { + const response$ = new Subject(); + const { component } = createComponent({ getRepos: () => response$ }); + + component.getRepos(); + component.ngOnDestroy(); + response$.next([repo()]); + + // takeUntil(destroy$) tears down the subscription when the component is destroyed, so a + // response that arrives afterward is dropped. Without it, `repositories` would pick up the + // emit below. + expect(component.repositories).toEqual([]); + }); + + it('memoizes reactListProps by identity so ReactMountDirective only calls update() when repositories actually changes', () => { + const { component } = createComponent({ getRepos: () => of([repo()]) }); + component.getRepos(); + + const first = component.reactListProps; + const second = component.reactListProps; + expect(second).toBe(first); + + // A second fetch, even of equivalent data, is a new `repositories` reference. + component.getRepos(); + const third = component.reactListProps; + expect(third).not.toBe(first); + expect(third.repositories).toBe(component.repositories); + }); + + it('falls back to the Angular list once the React remote reports an error', () => { + const { component, cdr } = createComponent({ getRepos: () => of([]) }); + component.useReactList = true; + expect(component.shouldUseReactList).toBe(true); + + component.onReactListError(new Error('remote failed to load')); + + expect(component.reactListFailed).toBe(true); + expect(component.shouldUseReactList).toBe(false); + expect(cdr.markForCheck).toHaveBeenCalled(); + }); + + it('re-evaluates the React flag on every queryParamMap emission, not just the one seen at construction', () => { + const queryParamMap$ = new Subject<{ get: (key: string) => string | null }>(); + const isEnabled = vi.fn( + (_surface: string, params: { get: (key: string) => string | null }) => params.get('reactNotebookRepos') !== null + ); + const { component, cdr } = createComponent( + { getRepos: () => of([]) }, + { activatedRoute: { queryParamMap: queryParamMap$ } as unknown as ActivatedRoute, reactFeature: { isEnabled } } + ); + + component.ngOnInit(); + queryParamMap$.next({ get: () => null }); + expect(component.useReactList).toBe(false); + + // A snapshot read at construction (or inside ngOnInit) would never see this second + // navigation; only a live subscription picks up the flag turning on. + queryParamMap$.next({ get: key => (key === 'reactNotebookRepos' ? 'true' : null) }); + expect(component.useReactList).toBe(true); + expect(cdr.markForCheck).toHaveBeenCalled(); + }); +}); 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 c0abe58bd01a..e9996245bc2a 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 @@ -11,6 +11,7 @@ */ import { ChangeDetectionStrategy, ChangeDetectorRef, Component, OnDestroy, OnInit } from '@angular/core'; import { ActivatedRoute } from '@angular/router'; +import { NzMessageService } from 'ng-zorro-antd/message'; import { Subject } from 'rxjs'; import { takeUntil } from 'rxjs/operators'; import { NotebookRepo, NotebookRepoPutData } from '@zeppelin/interfaces'; @@ -26,6 +27,10 @@ import { NotebookRepoService, ReactFeatureService } from '@zeppelin/services'; export class NotebookReposComponent implements OnInit, OnDestroy { repositories: NotebookRepo[] = []; useReactList = false; + // Sticky for the component's lifetime: toggling the flag off and back on doesn't retry the + // remote, even though the live queryParamMap subscription below re-evaluates `useReactList` on + // every navigation. Matches the other React surfaces (configuration, published/paragraph, + // notebook/paragraph), which are sticky the same way. reactListFailed = false; private destroy$ = new Subject(); @@ -35,7 +40,8 @@ export class NotebookReposComponent implements OnInit, OnDestroy { private notebookRepoService: NotebookRepoService, private activatedRoute: ActivatedRoute, private reactFeature: ReactFeatureService, - private cdr: ChangeDetectorRef + private cdr: ChangeDetectorRef, + private nzMessageService: NzMessageService ) {} get shouldUseReactList(): boolean { @@ -83,10 +89,24 @@ export class NotebookReposComponent implements OnInit, OnDestroy { } getRepos() { - this.notebookRepoService.getRepos().subscribe(data => { - this.repositories = data.sort((a, b) => a.name.charCodeAt(0) - b.name.charCodeAt(0)); - this.cdr.markForCheck(); - }); + this.notebookRepoService + .getRepos() + .pipe(takeUntil(this.destroy$)) + .subscribe({ + next: data => { + this.repositories = data.sort((a, b) => a.name.charCodeAt(0) - b.name.charCodeAt(0)); + this.cdr.markForCheck(); + }, + // This also runs after a failed PUT (updateRepoSetting refetches on error too), so a + // silent failure here would leave the React card's pendingSave flag stuck forever, since + // `repositories` keeps its old reference, reactListProps never rebuilds, and the [repo] + // effect on the React side never re-fires. The message is the only recovery signal the + // user gets in that case. + error: error => { + console.error('Failed to fetch notebook repositories', error); + this.nzMessageService.error('Failed to load notebook repositories. Please try again.'); + } + }); } updateRepoSetting(repo: NotebookRepo) { @@ -98,8 +118,17 @@ export class NotebookReposComponent implements OnInit, OnDestroy { data.settings[name] = selected; }); - this.notebookRepoService.updateRepo(data).subscribe(() => { - this.getRepos(); - }); + this.notebookRepoService + .updateRepo(data) + .pipe(takeUntil(this.destroy$)) + .subscribe({ + next: () => this.getRepos(), + // Refetch on failure too, so the card isn't stuck showing an unpersisted value. + error: error => { + console.error('Failed to update notebook repository', error); + this.nzMessageService.error('Failed to save notebook repository settings. Please try again.'); + this.getRepos(); + } + }); } } From 50e838ad6cbed10c3e1502db8974a19e86a002fa Mon Sep 17 00:00:00 2001 From: YONGJAE LEE Date: Fri, 11 Sep 2026 01:03:24 +0900 Subject: [PATCH 8/9] [ZEPPELIN-6631] Fix e2e selectors for the React notebook-repo card --- .../e2e/models/notebook-repo-item.util.ts | 7 +- .../e2e/models/notebook-repos-page.ts | 19 +- .../notebook-repo-item-workflow.spec.ts | 211 ++++++++++-------- .../react-notebook-repo-list.spec.ts | 9 +- 4 files changed, 142 insertions(+), 104 deletions(-) diff --git a/zeppelin-web-angular/e2e/models/notebook-repo-item.util.ts b/zeppelin-web-angular/e2e/models/notebook-repo-item.util.ts index 333af7c4171f..e979a8e43904 100644 --- a/zeppelin-web-angular/e2e/models/notebook-repo-item.util.ts +++ b/zeppelin-web-angular/e2e/models/notebook-repo-item.util.ts @@ -23,13 +23,16 @@ export class NotebookRepoItemUtil extends BasePage { } async verifyDisplayMode(): Promise { + // Button visibility, not the `.edit` class, since that's an Angular-only + // `[class.edit]="editMode"` binding the React card never applies. await expect(this.repoItemPage.editButton).toBeVisible(); - await expect(this.repoItemPage.repositoryCard).not.toHaveClass(/\bedit\b/); + await expect(this.repoItemPage.saveButton).not.toBeVisible(); + await expect(this.repoItemPage.cancelButton).not.toBeVisible(); } async verifyEditMode(): Promise { await expect(this.repoItemPage.saveButton).toBeVisible(); await expect(this.repoItemPage.cancelButton).toBeVisible(); - await expect(this.repoItemPage.repositoryCard).toHaveClass(/\bedit\b/); + await expect(this.repoItemPage.editButton).not.toBeVisible(); } } diff --git a/zeppelin-web-angular/e2e/models/notebook-repos-page.ts b/zeppelin-web-angular/e2e/models/notebook-repos-page.ts index 5caa33edc824..d4c016d8d40c 100644 --- a/zeppelin-web-angular/e2e/models/notebook-repos-page.ts +++ b/zeppelin-web-angular/e2e/models/notebook-repos-page.ts @@ -26,13 +26,18 @@ export class NotebookReposPage extends BasePage { this.repositoryItems = page.locator('[data-testid="notebook-repo-item"]'); } - async navigate(): Promise { - await this.navigateToRoute('/notebook-repos', { timeout: 60000 }); - await this.page.waitForURL('**/#/notebook-repos', { timeout: 60000 }); + // `query` carries the ZEPPELIN-6631 React flag (e.g. '?reactNotebookRepos=true'), + // so callers can drive the same workflow through either branch. + async navigate(query = ''): Promise { + await this.navigateToRoute(`/notebook-repos${query}`, { timeout: 60000 }); + await this.page.waitForURL('**/#/notebook-repos*', { timeout: 60000 }); await waitForZeppelinReady(this.page); + // [data-testid="notebook-repo-item"], not the Angular-only zeppelin-notebook-repo-item element, + // since that tag never exists on the React branch, so the race would silently lose all its + // coverage there the moment the header arm ever got slow. await Promise.race([ this.zeppelinPageHeader.filter({ hasText: 'Notebook Repository' }).waitFor({ state: 'visible' }), - this.page.waitForSelector('zeppelin-notebook-repo-item', { state: 'visible' }) + this.repositoryItems.first().waitFor({ state: 'visible' }) ]); } } @@ -78,13 +83,15 @@ export class NotebookRepoItemPage extends BasePage { async fillSettingInput(settingName: string, value: string): Promise { const row = this.repositoryCard.locator('tbody tr').filter({ hasText: settingName }); - const input = row.locator('input[nz-input]'); + // .ant-input, not [nz-input], since ng-zorro's nz-input directive renders that class too, + // and it excludes a DROPDOWN row's Select search input. + const input = row.locator('input.ant-input'); await this.fillAndVerifyInput(input, value); } async getSettingInputValue(settingName: string): Promise { const row = this.repositoryCard.locator('tbody tr').filter({ hasText: settingName }); - const input = row.locator('input[nz-input]'); + const input = row.locator('input.ant-input'); return await input.inputValue(); } diff --git a/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts index f39f27730365..3b5c009cda4c 100644 --- a/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts +++ b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts @@ -15,120 +15,143 @@ import { NotebookReposPage, NotebookRepoItemPage } from '../../../models/noteboo import { NotebookRepoItemUtil } from '../../../models/notebook-repo-item.util'; import { addPageAnnotationBeforeEach, waitForZeppelinReady, PAGES } from '../../../utils'; -test.describe('Notebook Repository Item - Edit Workflow', () => { - addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_REPOS_ITEM); - - let notebookReposPage: NotebookReposPage; - let repoItemPage: NotebookRepoItemPage; - let repoItemUtil: NotebookRepoItemUtil; - let firstRepoName: string; - - test.beforeEach(async ({ page }) => { - await page.goto('/#/'); - await waitForZeppelinReady(page); - notebookReposPage = new NotebookReposPage(page); - await notebookReposPage.navigate(); - - // JUSTIFIED: .first() picks the first configured repo; tests require at least one repo to be present - const firstCard = notebookReposPage.repositoryItems.first(); - firstRepoName = (await firstCard.locator('.ant-card-head-title').textContent()) || ''; - repoItemPage = new NotebookRepoItemPage(page, firstRepoName); - repoItemUtil = new NotebookRepoItemUtil(page, firstRepoName); - }); - - test('should complete full edit workflow with save', async () => { - const settingRows = await repoItemPage.settingRows.count(); - - await repoItemUtil.verifyDisplayMode(); - - await repoItemPage.clickEdit(); - await repoItemUtil.verifyEditMode(); - - let savedSettingName = ''; - let savedValue = ''; - for (let i = 0; i < settingRows; i++) { - // JUSTIFIED: nth(i) iterates all rows deterministically to find the first INPUT-type row - const row = repoItemPage.settingRows.nth(i); - // JUSTIFIED: td.first() is the Name column in the fixed 2-column settings table - const settingName = (await row.locator('td').first().textContent()) || ''; - - const isInputVisible = await row.locator('input[nz-input]').isVisible(); - if (isInputVisible) { - savedValue = (await repoItemPage.getSettingInputValue(settingName)) || 'test-value'; - await repoItemPage.fillSettingInput(settingName, savedValue); - savedSettingName = settingName; - break; +// Run on both branches of the ZEPPELIN-6631 flag. verifyDisplayMode/verifyEditMode and +// fillSettingInput/getSettingInputValue are the only e2e paths that exercise the React card's +// markup (button visibility instead of the Angular-only `.edit` class, `.ant-input` instead of +// `[nz-input]`) - without this, those selectors are only ever proven against the Angular branch. +for (const branch of [ + { label: 'Angular list', query: '', mount: false }, + { label: 'React list', query: '?reactNotebookRepos=true', mount: true } +]) { + const { label } = branch; + + test.describe(`Notebook Repository Item - Edit Workflow (${label})`, () => { + addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_REPOS_ITEM); + + let notebookReposPage: NotebookReposPage; + let repoItemPage: NotebookRepoItemPage; + let repoItemUtil: NotebookRepoItemUtil; + let firstRepoName: string; + + test.beforeEach(async ({ page }) => { + await page.goto('/#/'); + await waitForZeppelinReady(page); + notebookReposPage = new NotebookReposPage(page); + // navigateBranch(), not navigate(branch.query): folds in the mount assertion so it can't be + // forgotten here (see the method's own doc comment). + await notebookReposPage.navigateBranch(branch); + + // JUSTIFIED: .first() picks the first configured repo; tests require at least one repo to be present + const firstCard = notebookReposPage.repositoryItems.first(); + firstRepoName = (await firstCard.locator('.ant-card-head-title').textContent()) || ''; + repoItemPage = new NotebookRepoItemPage(page, firstRepoName); + repoItemUtil = new NotebookRepoItemUtil(page, firstRepoName); + }); + + test('should complete full edit workflow with save', async () => { + const settingRows = await repoItemPage.settingRows.count(); + + await repoItemUtil.verifyDisplayMode(); + + await repoItemPage.clickEdit(); + await repoItemUtil.verifyEditMode(); + + let savedSettingName = ''; + let savedValue = ''; + for (let i = 0; i < settingRows; i++) { + // JUSTIFIED: nth(i) iterates all rows deterministically to find the first INPUT-type row + const row = repoItemPage.settingRows.nth(i); + // JUSTIFIED: td.first() is the Name column in the fixed 2-column settings table + const settingName = (await row.locator('td').first().textContent()) || ''; + + // JUSTIFIED: inline, not lifted to the Page Object - this locator only needs to + // distinguish an INPUT row from a DROPDOWN row within this row-scan loop. .ant-input, not + // [nz-input]: excludes a DROPDOWN row's Select search input. + const isInputVisible = await row.locator('input.ant-input').isVisible(); + if (isInputVisible) { + // Writes the value straight back rather than a distinct one: this repo config is shared + // across the whole suite, which runs this spec across both branches and every browser + // project in parallel, so a distinct value risks a lost update stomping another + // worker's read. The display-mode assertion below still exercises the save/refetch + // path; it just can't distinguish a successful round trip from a rejected one that the + // UI shows optimistically regardless (true on both branches), which is a weaker but + // safe guarantee. + savedValue = (await repoItemPage.getSettingInputValue(settingName)) || 'test-value'; + await repoItemPage.fillSettingInput(settingName, savedValue); + savedSettingName = settingName; + break; + } } - } - expect(savedSettingName, 'No INPUT-type setting found — cannot verify save result').not.toBe(''); - await expect(repoItemPage.saveButton).toBeEnabled(); + expect(savedSettingName, 'No INPUT-type setting found - cannot verify save result').not.toBe(''); + await expect(repoItemPage.saveButton).toBeEnabled(); - await repoItemPage.clickSave(); + await repoItemPage.clickSave(); - await repoItemUtil.verifyDisplayMode(); + await repoItemUtil.verifyDisplayMode(); - // Verify the saved value is shown in display mode — not just that mode switched - const displayValue = await repoItemPage.getSettingValue(savedSettingName); - expect(displayValue.trim()).toBe(savedValue.trim()); - }); + // Verify the saved value is shown in display mode - not just that mode switched + const displayValue = await repoItemPage.getSettingValue(savedSettingName); + expect(displayValue.trim()).toBe(savedValue.trim()); + }); - test('should complete full edit workflow with cancel', async () => { - await repoItemUtil.verifyDisplayMode(); + test('should complete full edit workflow with cancel', async () => { + await repoItemUtil.verifyDisplayMode(); - // JUSTIFIED: any row is representative — testing that cancel reverts all changes - const firstRow = repoItemPage.settingRows.first(); - // JUSTIFIED: td.first() is the Name column in the fixed 2-column settings table - const settingName = (await firstRow.locator('td').first().textContent()) || ''; - const originalValue = await repoItemPage.getSettingValue(settingName); + // JUSTIFIED: any row is representative - testing that cancel reverts all changes + const firstRow = repoItemPage.settingRows.first(); + // JUSTIFIED: td.first() is the Name column in the fixed 2-column settings table + const settingName = (await firstRow.locator('td').first().textContent()) || ''; + const originalValue = await repoItemPage.getSettingValue(settingName); - await repoItemPage.clickEdit(); - await repoItemUtil.verifyEditMode(); + await repoItemPage.clickEdit(); + await repoItemUtil.verifyEditMode(); - await repoItemPage.fillSettingInput(settingName, 'temp-modified-value'); + await repoItemPage.fillSettingInput(settingName, 'temp-modified-value'); - await repoItemPage.clickCancel(); - await repoItemUtil.verifyDisplayMode(); + await repoItemPage.clickCancel(); + await repoItemUtil.verifyDisplayMode(); - const currentValue = await repoItemPage.getSettingValue(settingName); - expect(currentValue.trim()).toBe(originalValue.trim()); - }); + const currentValue = await repoItemPage.getSettingValue(settingName); + expect(currentValue.trim()).toBe(originalValue.trim()); + }); - test('should toggle between display and edit modes multiple times', async () => { - await repoItemUtil.verifyDisplayMode(); + test('should toggle between display and edit modes multiple times', async () => { + await repoItemUtil.verifyDisplayMode(); - await repoItemPage.clickEdit(); - await repoItemUtil.verifyEditMode(); + await repoItemPage.clickEdit(); + await repoItemUtil.verifyEditMode(); - await repoItemPage.clickCancel(); - await repoItemUtil.verifyDisplayMode(); + await repoItemPage.clickCancel(); + await repoItemUtil.verifyDisplayMode(); - await repoItemPage.clickEdit(); - await repoItemUtil.verifyEditMode(); + await repoItemPage.clickEdit(); + await repoItemUtil.verifyEditMode(); - await repoItemPage.clickCancel(); - await repoItemUtil.verifyDisplayMode(); - }); + await repoItemPage.clickCancel(); + await repoItemUtil.verifyDisplayMode(); + }); - test('should preserve card visibility throughout edit workflow', async () => { - await expect(repoItemPage.repositoryCard).toBeVisible(); + test('should preserve card visibility throughout edit workflow', async () => { + await expect(repoItemPage.repositoryCard).toBeVisible(); - await repoItemPage.clickEdit(); - await expect(repoItemPage.repositoryCard).toBeVisible(); + await repoItemPage.clickEdit(); + await expect(repoItemPage.repositoryCard).toBeVisible(); - await repoItemPage.clickCancel(); - await expect(repoItemPage.repositoryCard).toBeVisible(); - }); + await repoItemPage.clickCancel(); + await expect(repoItemPage.repositoryCard).toBeVisible(); + }); - test('should maintain settings count during mode transitions', async () => { - const initialCount = await repoItemPage.getSettingCount(); + test('should maintain settings count during mode transitions', async () => { + const initialCount = await repoItemPage.getSettingCount(); - await repoItemPage.clickEdit(); - const editModeCount = await repoItemPage.getSettingCount(); - expect(editModeCount).toBe(initialCount); + await repoItemPage.clickEdit(); + const editModeCount = await repoItemPage.getSettingCount(); + expect(editModeCount).toBe(initialCount); - await repoItemPage.clickCancel(); - const finalCount = await repoItemPage.getSettingCount(); - expect(finalCount).toBe(initialCount); + await repoItemPage.clickCancel(); + const finalCount = await repoItemPage.getSettingCount(); + expect(finalCount).toBe(initialCount); + }); }); -}); +} 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 e812b69f09d1..412363e4b0fc 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 @@ -82,13 +82,18 @@ test.describe('Notebook Repository - React list behind a flag', () => { 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(); + // JUSTIFIED: inline rather than NotebookRepoItemPage - this spec locates the card via + // MOUNT/REPO_ITEM constants rather than that Page Object, so reusing its selector alone + // without its `repositoryCard` root would be inconsistent with the rest of the file. + // .ant-input, not a bare 'input': a DROPDOWN row's Select also renders an + // that this would otherwise match instead. + const input = card.locator('input.ant-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); + await expect(card.locator('input.ant-input')).toHaveCount(0); }); test('when the remote fails to load, the Angular list renders', async ({ page }) => { From 70bc83511d19cad328766a55c024ff2d4922e8ac Mon Sep 17 00:00:00 2001 From: YONGJAE LEE Date: Fri, 11 Sep 2026 01:18:31 +0900 Subject: [PATCH 9/9] [ZEPPELIN-6631] Document the {label, query} flag-toggle idiom in e2e/AGENTS.md --- zeppelin-web-angular/e2e/AGENTS.md | 5 +++- .../e2e/models/notebook-repos-page.ts | 28 ++++++++++++++++++- .../notebook-repo-item-workflow.spec.ts | 7 ++--- ...ebook-repos-save-reloads-note-tree.spec.ts | 10 +++---- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/zeppelin-web-angular/e2e/AGENTS.md b/zeppelin-web-angular/e2e/AGENTS.md index a45bf08fa1f7..907bb5df8475 100644 --- a/zeppelin-web-angular/e2e/AGENTS.md +++ b/zeppelin-web-angular/e2e/AGENTS.md @@ -133,7 +133,10 @@ Pages are moving from Angular to React fragments incrementally. Today this is na ### When a Route Gains a React Flag - The flag is a route query param read via `ActivatedRoute.queryParams`, so with the hash router it goes INSIDE the hash: `/#/notebook//paragraph/?react=true`, not before the `#`. Popups opened by app code (`window.open`) will not carry a flag added only to `page.goto`. -- To exercise both frameworks, follow the existing precedent and toggle the flag in-spec: navigate the same spec with and without the flag across tests, as `published-paragraph.spec.ts` does. A separate flag-appending Playwright project is an alternative, but scope it (its own `testMatch`) to routes that read the flag rather than running the whole suite twice. +- To exercise both frameworks, follow the existing precedent and toggle the flag in-spec: navigate the same spec with and without the flag across tests. + - Prefer looping `for (const { label, query } of [...])` — an array of `{ label, query }` pairs — and folding `label` into the surrounding `test.describe`/`test` name, as `notebook-repos-save-reloads-note-tree.spec.ts` and `notebook-repo-item-workflow.spec.ts` do. The query string is the data the test actually needs, so it travels with the label instead of being reassembled from a bare boolean at each call site (`published-paragraph.spec.ts` predates this and still loops a boolean; match the newer shape in new specs). + - Both existing specs import the shared `NOTEBOOK_REPOS_BRANCHES` from `e2e/models/notebook-repos-page.ts` rather than each inlining the pair — export a same-shaped constant next to the relevant Page Object when a second spec needs the same pair, rather than inlining it again. + - A separate flag-appending Playwright project is an alternative, but scope it (its own `testMatch`) to routes that read the flag rather than running the whole suite twice. ### Coverage diff --git a/zeppelin-web-angular/e2e/models/notebook-repos-page.ts b/zeppelin-web-angular/e2e/models/notebook-repos-page.ts index d4c016d8d40c..aaca7b2e8a76 100644 --- a/zeppelin-web-angular/e2e/models/notebook-repos-page.ts +++ b/zeppelin-web-angular/e2e/models/notebook-repos-page.ts @@ -10,13 +10,29 @@ * limitations under the License. */ -import { Locator, Page } from '@playwright/test'; +import { expect, Locator, Page } from '@playwright/test'; import { waitForZeppelinReady } from '../utils'; import { BasePage } from './base-page'; +// Shared by every spec that runs the same assertions against both branches of the ZEPPELIN-6631 flag, using the +// `{label, query}` loop documented in e2e/AGENTS.md, so the flag's query string has one place to change. `mount` lets +// a caller assert which branch actually rendered (see `reactMountedList` below) - without it, a spec whose assertions +// pass on both branches' markup would still report a "React list" pass if the remote failed to load and the host fell +// back to Angular. +export const NOTEBOOK_REPOS_BRANCHES = [ + { label: 'Angular list', query: '', mount: false }, + { label: 'React list', query: '?reactNotebookRepos=true', mount: true } +] as const; + export class NotebookReposPage extends BasePage { readonly pageDescription: Locator; readonly repositoryItems: Locator; + // The outer [data-testid="react-notebook-repo-list"] div is rendered as soon as the flag is on + // (notebook-repos.component.html's @if), before ReactMountDirective has even started loading the remote, so it + // alone can't tell "mounted" from "flag on, still loading, or loading failed and fell back". The nested + // [data-testid="notebook-repo-list"] only exists once NotebookRepoList.tsx itself has actually rendered inside that + // host, so this locator is scoped to it, matching react-notebook-repo-list.spec.ts's MOUNTED_LIST. + readonly reactMountedList: Locator; constructor(page: Page) { super(page); @@ -24,6 +40,7 @@ export class NotebookReposPage extends BasePage { // 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"]'); + this.reactMountedList = page.locator('[data-testid="react-notebook-repo-list"] [data-testid="notebook-repo-list"]'); } // `query` carries the ZEPPELIN-6631 React flag (e.g. '?reactNotebookRepos=true'), @@ -40,6 +57,15 @@ export class NotebookReposPage extends BasePage { this.repositoryItems.first().waitFor({ state: 'visible' }) ]); } + + // For specs looping NOTEBOOK_REPOS_BRANCHES: folds the mount assertion into the navigation itself, rather than + // leaving it to each call site. A spec that destructures only `{ label, query }` and calls `navigate(query)` + // directly would compile and run fine on a remote that failed to load - that's the exact gap a prior review round + // found and fixed; this method exists so a future branch-parametrized spec can't reopen it by omission. + async navigateBranch(branch: (typeof NOTEBOOK_REPOS_BRANCHES)[number]): Promise { + await this.navigate(branch.query); + await expect(this.reactMountedList).toHaveCount(branch.mount ? 1 : 0); + } } export class NotebookRepoItemPage extends BasePage { diff --git a/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts index 3b5c009cda4c..6c16ab8fe98d 100644 --- a/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts +++ b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repo-item-workflow.spec.ts @@ -11,7 +11,7 @@ */ import { expect, test } from '@playwright/test'; -import { NotebookReposPage, NotebookRepoItemPage } from '../../../models/notebook-repos-page'; +import { NOTEBOOK_REPOS_BRANCHES, NotebookReposPage, NotebookRepoItemPage } from '../../../models/notebook-repos-page'; import { NotebookRepoItemUtil } from '../../../models/notebook-repo-item.util'; import { addPageAnnotationBeforeEach, waitForZeppelinReady, PAGES } from '../../../utils'; @@ -19,10 +19,7 @@ import { addPageAnnotationBeforeEach, waitForZeppelinReady, PAGES } from '../../ // fillSettingInput/getSettingInputValue are the only e2e paths that exercise the React card's // markup (button visibility instead of the Angular-only `.edit` class, `.ant-input` instead of // `[nz-input]`) - without this, those selectors are only ever proven against the Angular branch. -for (const branch of [ - { label: 'Angular list', query: '', mount: false }, - { label: 'React list', query: '?reactNotebookRepos=true', mount: true } -]) { +for (const branch of NOTEBOOK_REPOS_BRANCHES) { const { label } = branch; test.describe(`Notebook Repository Item - Edit Workflow (${label})`, () => { 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 ab4f321052d2..a42589574d35 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 @@ -11,7 +11,7 @@ */ import { expect, test, Page } from '@playwright/test'; -import { NotebookReposPage, NotebookRepoItemPage } from '../../../models/notebook-repos-page'; +import { NOTEBOOK_REPOS_BRANCHES, NotebookReposPage, NotebookRepoItemPage } from '../../../models/notebook-repos-page'; import { NodeListPage } from '../../../models/node-list-page'; import { addPageAnnotationBeforeEach, PAGES, waitForZeppelinReady } from '../../../utils'; @@ -29,10 +29,7 @@ test.describe('Notebook Repository - save reloads the note tree', () => { // 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' } - ]) { + for (const { label, query, mount } of NOTEBOOK_REPOS_BRANCHES) { 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 @@ -53,6 +50,9 @@ test.describe('Notebook Repository - save reloads the note tree', () => { await waitForZeppelinReady(actor); const reposPage = new NotebookReposPage(actor); await expect(reposPage.repositoryItems.first()).toBeVisible({ timeout: 20000 }); + // Otherwise a remote that failed to load and fell back to Angular would still pass the "React list" run, + // against Angular markup. + await expect(reposPage.reactMountedList).toHaveCount(mount ? 1 : 0); // 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);