[ZEPPELIN-6631] Render the notebook repository list through a React remote behind a flag - #5472
Open
kimyenac wants to merge 6 commits into
Open
[ZEPPELIN-6631] Render the notebook repository list through a React remote behind a flag#5472kimyenac wants to merge 6 commits into
kimyenac wants to merge 6 commits into
Conversation
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.
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.
…emote 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.
…wo 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.
…faces 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.
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.
kimyenac
force-pushed
the
ZEPPELIN-6631
branch
from
September 10, 2026 08:38
b07b350 to
4db1ced
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
Moves the
/notebook-reposrepository 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.contentchange 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 same shape.
The host follows the shape ZEPPELIN-6630 established for the configuration table rather than inventing a second one: a
shouldUseReactListgetter folding the flag together with a mount failure, props memoized on the only input that changes, andqueryParamMapsubscribed rather than read once, because navigating between/notebook-reposand/notebook-repos?reactNotebookReposreuses the component. AnonErrorfrom the remote falls back to the Angular list for the rest of the session.ReactMountDirectivenow re-enters the Angular zone for every function-valued prop, not onlyonErroras 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.
NotebookRepoand its setting type are declared in the remote because the SDK does not carry them and the shell'sNotebookRepoSettingsItemis an Angular interface the remote cannot import. On visual parity:REPO_TOKENSpassesfontWeightStrongthrough 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@card-padding-base, and the buttons carry the edit/save/close icons the Angular card draws throughnz-icon.What type of PR is it?
Improvement
Todos
None
What is the Jira issue?
How should this be tested?
npm run test:shell: 51 tests across 11 files.react-mount.directive.spec.tsgains the zone case for a non-onErrorcallback.projects/zeppelin-reactvitest: 76 tests across 12 files, including the 10 inNotebookRepoList.spec.tsx.npm run build:react(NotebookRepoListpresent inremoteEntry.js) andnpm run build:projects.PUT /api/notebook-repositoriesfollowed by the host's refetch, and the card returns to read mode showing the new value.remoteEntry.jsfalls back to the Angular list, with no mount host in the DOM.run-playwright-e2e-tests.DROPDOWNsettings are covered by unit tests only — the server used for the manual pass exposes one repository with a singleINPUTsetting.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: