This is an automated email from the ASF dual-hosted git repository.
voidmatcha pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new b13ddf4a49 [ZEPPELIN-6631] Render the notebook repository list through
a React remote behind a flag
b13ddf4a49 is described below
commit b13ddf4a4968b2a5514ec976e52b4eabacb5944f
Author: 김예나 <[email protected]>
AuthorDate: Wed Sep 16 22:23:40 2026 +0900
[ZEPPELIN-6631] Render the notebook repository list through a React remote
behind a flag
### What is this PR for?
Moves the `/notebook-repos` repository list and its edit surface to the
React remote behind `?reactNotebookRepos`, while Angular keeps the route,
`NotebookRepoService`, the PUT, the refetch that follows it, the note-tree
refresh the server broadcasts, and the fallback. Only the cards inside
`.content` change hands; the default path stays Angular.
**ZEPPELIN-6682**, first commit, is the safety net this rests on: a
live-server E2E proving that a successful repository save reloads notebooks and
refreshes the shell note tree. It carries no React implementation and stands on
its own — its spec passes with none of the port applied, so the two split
cleanly if you would rather take them separately. Both are here because
ZEPPELIN-6631 cannot close until that scenario passes with the React flag
enabled, and ZEPPELIN-6630/6363 took the [...]
The host follows the shape ZEPPELIN-6630 established for the configuration
table rather than inventing a second one: a `shouldUseReactList` getter folding
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.
`ReactMountDirective` now re-enters the Angular zone for **every**
function-valued prop, not only `onError` as ZEPPELIN-6565 left it. The
repository list is the first surface to hand the remote a real callback: its
save calls back into the host, which issues the PUT and refetches. Outside the
zone that work is untracked and the refresh lands late or not at all.
The remote owns no state beyond the open editor and its draft. A save hands
the whole repo back with the edited values in place, not a partial patch, since
the host issues the PUT and feeds the refetched list back down; the draft is
rebuilt when that arrives, so a card cannot keep showing values the server
already replaced. Blank settings are refused 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 and the shell's `NotebookRepoSettingsItem` is an
Angular interface the remote cannot import. On visual parity: `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,
antd at 600), the card gap is pinned to 24px to match `<at>card-padding-base`,
and the buttons carry the edit [...]
### What type of PR is it?
Improvement
### Todos
None
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-6631
* https://issues.apache.org/jira/browse/ZEPPELIN-6682
### How should this be tested?
* `npm run test:shell`: 51 tests across 11 files.
`react-mount.directive.spec.ts` gains the zone case for a non-`onError`
callback.
* `projects/zeppelin-react` vitest: 76 tests across 12 files, including the
10 in `NotebookRepoList.spec.tsx`.
* Production builds: `npm run build:react` (`NotebookRepoList` present in
`remoteEntry.js`) and `npm run build:projects`.
* Manually against a live server in both branches of the flag, anonymous
mode:
* The save round trip issues `PUT /api/notebook-repositories` followed by
the host's refetch, and the card returns to read mode showing the new value.
* A blank setting disables Save; Cancel restores the previous value and
re-entering edit shows it.
* Reading the repository names and setting rows from each branch returns
byte-identical output.
* The note-tree scenario ZEPPELIN-6682 covers, with the React flag on: a
note created over REST is absent from a second client's home note tree, stays
absent while nothing is saved, and appears after the repository save. Skipping
the save leaves it absent, which is the control that makes the assertion mean
something.
* Aborting `remoteEntry.js` falls back to the Angular list, with no mount
host in the DOM.
* Playwright was not run locally; the two new specs and the ZEPPELIN-6682
spec's two flag legs run for the first time in `run-playwright-e2e-tests`.
* `DROPDOWN` settings are covered by unit tests only — the server used for
the manual pass exposes one repository with a single `INPUT` setting.
### Screenshots (if appropriate)
The two lists are hard to tell apart, which is the intent. Header weight,
card gap and button icons match; the remaining difference is antd's own row
height.
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No, the flag defaults to
off and the Angular list is unchanged when it is
* Does this needs documentation? The two places that enumerate the flagged
React surfaces are updated in the last commit
Closes #5472 from kimyenac/ZEPPELIN-6631.
Signed-off-by: YONGJAE LEE <[email protected]>
---
zeppelin-web-angular/e2e/AGENTS.md | 7 +-
.../e2e/models/notebook-repo-item.util.ts | 7 +-
.../e2e/models/notebook-repos-page.ts | 58 ++-
.../notebook-repo-item-workflow.spec.ts | 210 +++++-----
.../notebook-repos-save-reloads-note-tree.spec.ts | 84 ++++
.../react-notebook-repo-list.spec.ts | 117 ++++++
.../projects/zeppelin-react/README.md | 2 +-
.../projects/zeppelin-react/src/main.ts | 1 +
.../src/pages/NotebookRepoList.spec.tsx | 431 +++++++++++++++++++++
.../zeppelin-react/src/pages/NotebookRepoList.tsx | 253 ++++++++++++
.../projects/zeppelin-react/webpack.config.js | 3 +-
.../notebook-repos/item/item.component.html | 9 +-
.../notebook-repos/notebook-repos.component.html | 12 +-
.../notebook-repos.component.spec.ts | 175 +++++++++
.../notebook-repos/notebook-repos.component.ts | 101 ++++-
.../src/app/services/react-feature.service.ts | 6 +-
.../react-mount/react-mount.directive.spec.ts | 69 ++++
.../app/share/react-mount/react-mount.directive.ts | 29 +-
18 files changed, 1437 insertions(+), 137 deletions(-)
diff --git a/zeppelin-web-angular/e2e/AGENTS.md
b/zeppelin-web-angular/e2e/AGENTS.md
index bade7acd25..4fd63b1de7 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 [...]
+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` fla [...]
### Write Framework-Neutral Specs
@@ -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/<id>/paragraph/<id>?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; mat [...]
+ - 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-repo-item.util.ts
b/zeppelin-web-angular/e2e/models/notebook-repo-item.util.ts
index 333af7c417..e979a8e439 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<void> {
+ // 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<void> {
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 f047b44b2e..aaca7b2e8a 100644
--- a/zeppelin-web-angular/e2e/models/notebook-repos-page.ts
+++ b/zeppelin-web-angular/e2e/models/notebook-repos-page.ts
@@ -10,29 +10,62 @@
* 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);
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"]');
+ this.reactMountedList =
page.locator('[data-testid="react-notebook-repo-list"]
[data-testid="notebook-repo-list"]');
}
- async navigate(): Promise<void> {
- 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<void> {
+ 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' })
]);
}
+
+ // 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<void> {
+ await this.navigate(branch.query);
+ await expect(this.reactMountedList).toHaveCount(branch.mount ? 1 : 0);
+ }
}
export class NotebookRepoItemPage extends BasePage {
@@ -46,13 +79,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<void> {
@@ -75,13 +109,15 @@ export class NotebookRepoItemPage extends BasePage {
async fillSettingInput(settingName: string, value: string): Promise<void> {
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<string> {
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 f39f277303..6c16ab8fe9 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,124 +11,144 @@
*/
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';
-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 NOTEBOOK_REPOS_BRANCHES) {
+ 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/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 0000000000..a42589574d
--- /dev/null
+++
b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/notebook-repos-save-reloads-note-tree.spec.ts
@@ -0,0 +1,84 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { expect, test, Page } from '@playwright/test';
+import { NOTEBOOK_REPOS_BRANCHES, 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<string> => {
+ const response = await page.request.post('/api/notebook', {
+ data: { notePath: `/${name}`, addingEmptyParagraph: true },
+ failOnStatusCode: false
+ });
+ expect(response.ok(), `Create notebook failed:
${response.status()}`).toBe(true);
+ return JSON.parse(await response.text()).body as string;
+};
+
+test.describe('Notebook Repository - save reloads the note tree', () => {
+ addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_REPOS);
+
+ // Run on both branches of the ZEPPELIN-6631 flag. The reload is the host's
+ // job either way, so a React list that swallows the save would show up here.
+ for (const { label, query, 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
+ // tell a broadcast from its own refetch. The home route keeps a tree
+ // mounted, which leaves the broadcast as the only thing that can change
it.
+ const watcher = await context.newPage();
+ const actor = await context.newPage();
+ const noteName = `NotebookRepoReload_${Date.now()}`;
+ let noteId = '';
+
+ try {
+ await watcher.goto('/#/');
+ await waitForZeppelinReady(watcher);
+ const noteTree = new NodeListPage(watcher);
+ await expect(noteTree.nodeListContainer).toBeVisible();
+
+ await actor.goto(`/#/notebook-repos${query}`);
+ await waitForZeppelinReady(actor);
+ const reposPage = new NotebookReposPage(actor);
+ await expect(reposPage.repositoryItems.first()).toBeVisible({ timeout:
20000 });
+ // 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);
+
+ await test.step('Given a note created out of band, which no broadcast
has announced', async () => {
+ await expect(noteTree.noteLinkByName(noteName)).toHaveCount(0);
+ noteId = await createNoteAtRoot(actor, noteName);
+ // Creating a note over REST does not broadcast the list, so a tree
+ // that picked this up on its own would make the assertion after the
+ // save meaningless.
+ await expect(noteTree.noteLinkByName(noteName)).toHaveCount(0);
+ });
+
+ await test.step('When the repository settings are saved unchanged',
async () => {
+ await repoItem.clickEdit();
+ await repoItem.clickSave();
+ });
+
+ await test.step('Then the note tree of the other client picks the note
up', async () => {
+ await expect(noteTree.noteLinkByName(noteName)).toHaveCount(1, {
timeout: 20000 });
+ });
+ } finally {
+ if (noteId) {
+ await actor.request.delete(`/api/notebook/${noteId}`, {
failOnStatusCode: false });
+ }
+ }
+ });
+ }
+});
diff --git
a/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts
b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts
new file mode 100644
index 0000000000..412363e4b0
--- /dev/null
+++
b/zeppelin-web-angular/e2e/tests/workspace/notebook-repos/react-notebook-repo-list.spec.ts
@@ -0,0 +1,117 @@
+/*
+ * 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<void> => {
+ await page.goto(`/#/notebook-repos${query}`);
+ await waitForZeppelinReady(page);
+};
+
+const settingRows = (page: Page, root: string) => page.locator(`${root} tbody
tr:not(.ant-table-placeholder)`);
+
+test.describe('Notebook Repository - React list behind a flag', () => {
+ addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK_REPOS);
+
+ test('without the flag, the Angular list renders', async ({ page }) => {
+ await openRepos(page);
+
+ await expect(page.locator(REPO_ITEM).first()).toBeVisible();
+ await expect(page.locator(MOUNT)).toHaveCount(0);
+ await expect(page.locator(`${REPO_ITEM}
button:has-text("Edit")`).first()).toBeVisible();
+ });
+
+ test('with reactNotebookRepos=true, the React list renders instead', async
({ page }) => {
+ await openRepos(page, '?reactNotebookRepos=true');
+
+ await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 });
+ await expect(page.locator(REPO_ITEM).first()).toBeVisible();
+ });
+
+ test('with a bare reactNotebookRepos flag, the React list renders', async ({
page }) => {
+ await openRepos(page, '?reactNotebookRepos');
+
+ await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 });
+ });
+
+ test('both lists show the same repositories and settings', async ({ page })
=> {
+ await openRepos(page);
+ await expect(page.locator(REPO_ITEM).first()).toBeVisible();
+ const angularNames = await page
+ .locator(`${REPO_ITEM}`)
+ .evaluateAll(cards => cards.map(card =>
card.getAttribute('data-repo-name')));
+ const angularRows = await settingRows(page, REPO_ITEM).allInnerTexts();
+
+ await openRepos(page, '?reactNotebookRepos=true');
+ await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 });
+ const reactNames = await page
+ .locator(`${MOUNT} ${REPO_ITEM}`)
+ .evaluateAll(cards => cards.map(card =>
card.getAttribute('data-repo-name')));
+ // JUSTIFIED: prefer-web-first-assertions. Both sides must be read the same
+ // way to compare, and toHaveText() reads textContent, which loses the cell
+ // separator innerText adds.
+ const reactRows = await settingRows(page, `${MOUNT}
${REPO_ITEM}`).allInnerTexts();
+
+ // The host still owns the fetch and the sort, so the remote must not
+ // reshape what it is handed.
+ expect(reactNames).toEqual(angularNames);
+ expect(reactRows).toEqual(angularRows);
+ });
+
+ test('the React card switches to inputs on edit and back on cancel', async
({ page }) => {
+ await openRepos(page, '?reactNotebookRepos=true');
+ await expect(page.locator(MOUNTED_LIST)).toBeVisible({ timeout: 15000 });
+
+ const card = page.locator(`${MOUNT} ${REPO_ITEM}`).first();
+ const value = (await settingRows(page, `${MOUNT}
${REPO_ITEM}`).first().locator('td').nth(1).innerText()).trim();
+
+ await card.getByRole('button', { name: 'Edit' }).click();
+ // 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
+ // <input role="combobox"> 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.ant-input')).toHaveCount(0);
+ });
+
+ test('when the remote fails to load, the Angular list renders', async ({
page }) => {
+ await test.step('Given a dead remote whose entry never loads', async () =>
{
+ await page.route('**/remoteEntry.js', route => route.abort());
+ });
+
+ await test.step('When the page opens with the React list enabled', async
() => {
+ // Angular is the default branch, so the assertions below pass even if
the
+ // flag never took. Awaiting the request is what proves this is a
fallback.
+ const remoteRequested = page.waitForRequest('**/remoteEntry.js');
+ await openRepos(page, '?reactNotebookRepos=true');
+ await remoteRequested;
+ });
+
+ await test.step('Then the Angular list takes over', async () => {
+ await expect(page.locator(REPO_ITEM).first()).toBeVisible({ timeout:
15000 });
+ await expect(page.locator(MOUNT)).toHaveCount(0);
+ });
+ });
+});
diff --git a/zeppelin-web-angular/projects/zeppelin-react/README.md
b/zeppelin-web-angular/projects/zeppelin-react/README.md
index e8b51bf97f..9536a85420 100644
--- a/zeppelin-web-angular/projects/zeppelin-react/README.md
+++ b/zeppelin-web-angular/projects/zeppelin-react/README.md
@@ -77,7 +77,7 @@ Each React surface is behind a URL query flag, resolved by
`ReactFeatureService`
| `?react=false` | disabled |
| flag absent | disabled |
-Append `?react=true` to any published paragraph URL, `?reactFooter=true` to a
notebook URL, or `?reactConfiguration=true` to the configuration URL to
activate React mode.
+Append `?react=true` to any published paragraph URL, `?reactFooter=true` to a
notebook URL, `?reactConfiguration=true` to the configuration URL, or
`?reactNotebookRepos=true` to the notebook repository URL to activate React
mode.
## Setup
diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/main.ts
b/zeppelin-web-angular/projects/zeppelin-react/src/main.ts
index 66f7f95a0a..ba6a218ee0 100644
--- a/zeppelin-web-angular/projects/zeppelin-react/src/main.ts
+++ b/zeppelin-web-angular/projects/zeppelin-react/src/main.ts
@@ -11,6 +11,7 @@
*/
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';
export type { NotebookCoreRemoteProps } from './notebookCoreContract';
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 0000000000..2fd93ae59c
--- /dev/null
+++
b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.spec.tsx
@@ -0,0 +1,431 @@
+/*
+ * 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 it gets the whole repo, not a partial patch.
+ expect(onRepoChange).toHaveBeenCalledTimes(1);
+ expect(onRepoChange.mock.calls[0][0]).toEqual({
+ ...gitRepo(),
+ settings: [{ ...gitRepo().settings[0], selected: '/srv/notebook' }]
+ });
+ });
+
+ 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 });
+
+ 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('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 });
+
+ clickButton('GitNotebookRepo', /Edit/);
+ act(() => {
+ fireEvent.change(within(card('GitNotebookRepo')).getByRole('textbox'), {
target: { value: ' ' } });
+ });
+
+ 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()] });
+
+ 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 0000000000..af169f928d
--- /dev/null
+++
b/zeppelin-web-angular/projects/zeppelin-react/src/pages/NotebookRepoList.tsx
@@ -0,0 +1,253 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { useEffect, useRef, useState } from 'react';
+import { createRoot, Root } from 'react-dom/client';
+import { CloseOutlined, EditOutlined, SaveOutlined } from '@ant-design/icons';
+import { Button, Card, Input, Select, Space, Table } from 'antd';
+import { ReactErrorBoundary } from '@/components';
+import { ZeppelinThemeProvider } from '@/theme';
+
+/** Mirrors the shell's NotebookRepoSettingsItem; the SDK does not declare it.
*/
+export interface NotebookRepoSetting {
+ type: string;
+ value: string[];
+ selected: string;
+ name: string;
+}
+
+export interface NotebookRepo {
+ name: string;
+ className: string;
+ settings: NotebookRepoSetting[];
+}
+
+export interface NotebookRepoListProps {
+ repositories?: NotebookRepo[];
+ /** The host owns the PUT and the refetch; this only reports the edited
repo. */
+ onRepoChange?: (repo: NotebookRepo) => void;
+ onError?: (error: unknown) => void;
+}
+
+// ng-zorro draws card titles and table headers at 500 where antd uses 600.
+const REPO_TOKENS = { fontWeightStrong: 500 };
+
+// The Angular card spaces itself with @card-padding-base from the default
theme.
+const CARD_GAP = 24;
+
+// Stricter than the Angular form's Validators.required, which accepts
whitespace-only input.
+const isBlank = (value: string): boolean => value.trim().length === 0;
+
+interface RepoCardProps {
+ repo: NotebookRepo;
+ 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<string,
string> =>
+ Object.fromEntries(settings.map(setting => [setting.name,
setting.selected]));
+
+const RepoCard = ({ repo, onRepoChange }: RepoCardProps) => {
+ const [editing, setEditing] = useState(false);
+ const [draft, setDraft] = useState<Record<string, string>>(() =>
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<string, string>
} | 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(() => {
+ // 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 = Object.values(draft).some(isBlank);
+
+ const save = () => {
+ if (invalid) {
+ return;
+ }
+ const values = { ...draft };
+ onRepoChange?.({
+ ...repo,
+ settings: repo.settings.map(setting => ({ ...setting, selected:
values[setting.name] }))
+ });
+ savedRef.current = { repo, draft: values };
+ setPendingSave(true);
+ setEditing(false);
+ };
+
+ const cancel = () => {
+ // 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 = (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) => {
+ if (!editing) {
+ // 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 (
+ <Select
+ size="small"
+ value={draft[setting.name]}
+ onChange={value => setValue(setting.name, value)}
+ options={setting.value.map(option => ({ label: option, value:
option }))}
+ style={{ minWidth: 160 }}
+ />
+ );
+ }
+ return (
+ <Input
+ size="small"
+ value={draft[setting.name]}
+ onChange={event => setValue(setting.name, event.target.value)}
+ />
+ );
+ }
+ }
+ ];
+
+ // Icons mirror the Angular card's nz-icon edit/save/close.
+ const extra = editing ? (
+ <Space size={8}>
+ <Button type="primary" size="small" icon={<SaveOutlined />}
disabled={invalid} onClick={save}>
+ Save
+ </Button>
+ <Button size="small" icon={<CloseOutlined />} onClick={cancel}>
+ Cancel
+ </Button>
+ </Space>
+ ) : (
+ <Button size="small" icon={<EditOutlined />} onClick={() =>
setEditing(true)}>
+ Edit
+ </Button>
+ );
+
+ return (
+ <Card
+ title={repo.name}
+ extra={extra}
+ size="small"
+ style={{ marginBottom: CARD_GAP }}
+ data-testid="notebook-repo-item"
+ data-repo-name={repo.name}
+ >
+ <h3>Setting</h3>
+ <Table<NotebookRepoSetting>
+ columns={columns}
+ dataSource={repo.settings.map((setting, index) => ({ ...setting, key:
`${setting.name}-${index}` }))}
+ size="small"
+ pagination={false}
+ />
+ </Card>
+ );
+};
+
+export const NotebookRepoList = ({ repositories = [], onRepoChange }:
NotebookRepoListProps) => (
+ <div data-testid="notebook-repo-list">
+ {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".
+ <RepoCard key={`${repo.className}-${index}`} repo={repo}
onRepoChange={onRepoChange} />
+ ))}
+ </div>
+);
+
+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(
+ <ReactErrorBoundary onError={props.onError}>
+ <ZeppelinThemeProvider token={REPO_TOKENS}>
+ <NotebookRepoList {...props} />
+ </ZeppelinThemeProvider>
+ </ReactErrorBoundary>
+ );
+ };
+
+ 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 aa73dc1647..bb8274bc48 100644
--- a/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js
+++ b/zeppelin-web-angular/projects/zeppelin-react/webpack.config.js
@@ -74,7 +74,8 @@ module.exports = (_env, argv) => {
exposes: {
'./PublishedParagraph': './src/pages/PublishedParagraph',
'./ParagraphFooter': './src/components/paragraph/ParagraphFooter',
- './ConfigurationTable': './src/pages/ConfigurationTable'
+ './ConfigurationTable': './src/pages/ConfigurationTable',
+ './NotebookRepoList': './src/pages/NotebookRepoList'
}
}),
new HtmlWebpackPlugin({
diff --git
a/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/item/item.component.html
b/zeppelin-web-angular/src/app/pages/workspace/notebook-repos/item/item.component.html
index 58dd6a2af9..d456b66434 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.
-->
-<nz-card class="repo-item" [class.edit]="editMode" [nzTitle]="repo.name"
[nzExtra]="extraTemplate">
+<nz-card
+ class="repo-item"
+ data-testid="notebook-repo-item"
+ [attr.data-repo-name]="repo.name"
+ [class.edit]="editMode"
+ [nzTitle]="repo.name"
+ [nzExtra]="extraTemplate"
+>
<ng-template #extraTemplate>
@if (!editMode) {
<div class="extra-wrap">
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 b3d37908e4..f35dc30b4b 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 @@
<zeppelin-page-header title="Notebook Repository"
[description]="notebookReposDescription"></zeppelin-page-header>
<ng-template #notebookReposDescription>Manage your Notebook Repositories'
settings.</ng-template>
<div class="content">
- @for (repo of repositories; track repo) {
- <zeppelin-notebook-repo-item (repoChange)="updateRepoSetting($event)"
[repo]="repo"></zeppelin-notebook-repo-item>
+ @if (shouldUseReactList) {
+ <div
+ data-testid="react-notebook-repo-list"
+ zeppelin-react-mount="./NotebookRepoList"
+ [reactProps]="reactListProps"
+ ></div>
+ } @else {
+ @for (repo of repositories; track repo) {
+ <zeppelin-notebook-repo-item (repoChange)="updateRepoSetting($event)"
[repo]="repo"></zeppelin-notebook-repo-item>
+ }
}
</div>
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 0000000000..e2286b0534
--- /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> = {}): 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<NotebookRepoService>,
+ overrides: { activatedRoute?: ActivatedRoute; reactFeature?:
Partial<ReactFeatureService> } = {}
+) => {
+ 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<NotebookRepo[]>();
+ 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 afe62b26f3..e9996245bc 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,13 @@
* 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 { NzMessageService } from 'ng-zorro-antd/message';
+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,23 +24,89 @@ 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;
+ // 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<void>();
+ private lastReactListProps: Record<string, unknown> | null = null;
constructor(
private notebookRepoService: NotebookRepoService,
- private cdr: ChangeDetectorRef
+ private activatedRoute: ActivatedRoute,
+ private reactFeature: ReactFeatureService,
+ private cdr: ChangeDetectorRef,
+ private nzMessageService: NzMessageService
) {}
+ 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<string, unknown> {
+ 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));
- 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) {
@@ -48,8 +118,17 @@ export class NotebookReposComponent implements OnInit {
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();
+ }
+ });
}
}
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 6d0d3897eb..9559fbb927 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<ReactSurface, ReactSurfaceConfig> = {
configurationTable: {
queryParam: 'reactConfiguration',
defaultEnabled: false
+ },
+ notebookRepoList: {
+ queryParam: 'reactNotebookRepos',
+ defaultEnabled: false
}
};
diff --git
a/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.spec.ts
b/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.spec.ts
index 71c5e64e30..562cc8e5b6 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<HTMLElement>(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 <T>(): Promise<T> => remote as T);
+ const loader = { loadModule } as Pick<ReactRemoteLoaderService,
'loadModule'>;
+ 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<HTMLElement>(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 <T>(): Promise<T> => 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 f981710bff..1295e8e123 100644
--- a/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.ts
+++ b/zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.ts
@@ -125,22 +125,31 @@ export class ReactMountDirective implements OnChanges,
OnDestroy {
}
}
+ /**
+ * Every function the host passes down, not just the `onError` ZEPPELIN-6565
+ * covered. The remote runs outside the Angular zone, so a callback it
invokes
+ * would leave the host's state change and any async work untracked by
NgZone.
+ */
private withHostCallbacks(props: ReactProps & ReactHostCallbacks):
ReactProps & ReactHostCallbacks {
- const onError = props.onError;
- if (typeof onError !== 'function') {
+ const entries = Object.entries(props).filter(([, value]) => typeof value
=== 'function');
+ if (entries.length === 0) {
return props;
}
- return {
- ...props,
- onError: (error: unknown): void => {
+
+ const wrapped: ReactProps = { ...props };
+ for (const [name, callback] of entries) {
+ wrapped[name] = (...args: unknown[]): void => {
this.ngZone.run(() => {
try {
- onError(error);
- } catch {
- /* swallow callback errors; they shouldn't loop */
+ (callback as (...callbackArgs: unknown[]) => void)(...args);
+ } catch (error) {
+ // Swallowed rather than rethrown: the caller is React, which would
+ // turn it into a render error in a tree the host does not own.
+ console.error(`[ReactMountDirective] host callback "${name}"
threw`, error);
}
});
- }
- };
+ };
+ }
+ return wrapped;
}
}
