Skip to content

[ZEPPELIN-6675] Prove the notebook route can consume the Shared Notebook Core port - #5527

Open
kimyenac wants to merge 2 commits into
apache:masterfrom
kimyenac:ZEPPELIN-6675
Open

kimyenac wants to merge 2 commits into
apache:masterfrom
kimyenac:ZEPPELIN-6675

Conversation

@kimyenac

@kimyenac kimyenac commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

What is this PR for?

Add a browser proof that the current Angular notebook route can hand one host-owned NotebookCorePort to a separately built React consumer. This builds on the ZEPPELIN-6674 port identity proof and moves it onto the real notebook route.

  • The proof harness bootstraps the production WorkspaceModule and NotebookModule lazy routes and asserts that the activated component is the production NotebookComponent.
  • It reads the activated /notebook/:noteId and /notebook/:noteId/revision/:revisionId parameters into one host-owned test Core. The React consumer reads the snapshot and receives route-driven subscription updates (note → other note → revision), always through the same port object.
  • A browser-only MessageService double records the production component's getNote, noteRevision and listRevisionHistory requests, and the test asserts their exact order.

This is a seam-only proof. It does not switch the production renderer, move notebook state out of Angular, or implement re-subscription, recovery or mutation reducers (ZEPPELIN-6687).

The implementation is based on the ZEPPELIN-6675-notebook-route-boundary branch in the voidmatcha fork (823183338), which was written before ZEPPELIN-6674 merged. Porting it onto current master:

  • CI wiring: the route proof runs in the integration-test phase behind web.e2e.core.port.proof.disabled, the same as the merged port identity proof, so it runs on the anonymous leg of run-playwright-e2e-tests. The branch bound it to the test phase with ${skipTests}, which frontend.yml's -DskipTests build skips.
  • Shared static server: it now returns 404 for unknown paths, matching the merged ZEPPELIN-6674 review change, instead of serving index.html.
  • Message double: it gains receiveEnvelope, added to MessageService by ZEPPELIN-6683. Without it, the workspace route did not render.
  • Theme double: it gains getCurrentTheme and theme$. Without them, ThemeToggleComponent throws in ngOnInit. The original test did not catch this, because Angular's ErrorHandler reports it through console.error('ERROR', ...) and never as a pageerror. The test now fails on those errors too. Removing the theme fix makes it fail with this.themeService.getCurrentTheme is not a function.
  • Lifecycle boundary is now asserted, not only documented:
    • the port's own keys are exactly getSnapshot and subscribe, and the port is frozen;
    • route activation and port consumption make no bootstrap, connect or close call.
  • README: the section moves to the end of e2e/core-contract/README.md instead of splitting the replay subsections. It describes the actual CI wiring and notes that ZEPPELIN-6683's stale-reply rejection lives inside NotebookComponent and is not moved into the Core.
  • Kept as is: the route path constants (notebook-route-boundary.ts). Production code changes are limited to extracting the existing route paths into those constants, which both the routing modules and the proof use.
  • Dropped from the branch: the ./NotebookRouteBoundaryProbe webpack alias (it re-exposed the same module), and unused route-proof proofs state.

What type of PR is it?

Improvement

Todos

  • Run the route proof on the production notebook routes with one host-owned port
  • Assert route-driven snapshot and subscription updates through the same port
  • Assert the port shape and that the connection lifecycle is not reached
  • Run it through the normal browser CI path
  • Record host-side responsibilities and what remains outside this proof

What is the Jira issue?

ZEPPELIN-6675

How should this be tested?

cd zeppelin-web-angular
npm run build:projects
npm run build:notebook-core-port-proof
npm run test:notebook-core-port-identity
npm run test:notebook-route-boundary

What I ran locally (after rebasing onto 6a5ed4459):

  • Both browser proofs passed. Also passing:
    • test:notebook-core, typecheck:notebook-core
    • check:core-contract-fixtures, test:shell
    • prettier and eslint on the changed files
  • ./mvnw verify -pl zeppelin-web-angular -Pweb-e2e (before the rebase): the proof build, the port identity proof and the route proof passed in integration-test.
  • The main Playwright suite finished with 749 passed and 20 failed. Rerunning only the failing specs with one worker left 6 failures:
    • paragraph-functionality.spec.ts:174 and :335, on all three browsers;
    • both execute %python, and every failure context shows Fail to launch python process from my local Python setup;
    • the other 14 failures passed when run serially.
  • Not run locally: the classic UI e2e. The maven run stopped after the main suite because I ran it without CI=true, so the HTML reporter waited for input. This PR does not touch zeppelin-web.
  • I checked that each build setting kept from the branch is still required by building without it:
    • mathjax types: without them, the MathJax directive fails to compile;
    • the custom webpack builder: without it, Monaco CSS fails to parse;
    • the style include paths: without them, the components' .less imports fail to resolve.

Screenshots (if appropriate)

N/A.

Questions:

  • Does the license files need to update? No. New files carry the ASF header.
  • Is there breaking changes for older versions? No. Route shapes and behaviour are unchanged.
  • Does this needs documentation? e2e/core-contract/README.md is updated.

Unrelated, found while testing: production main.js builds the Monaco codicon.ttf URL from the build machine's absolute file:/// path, so the browser refuses to load that font. It also happens on master.

🤖 Generated with Claude Code

…ook Core port

Add a browser proof on the current Angular notebook route host. The
proof harness bootstraps the production WorkspaceModule and
NotebookModule lazy routes, reads the activated /notebook/:noteId and
/notebook/:noteId/revision/:revisionId parameters into one host-owned
test Core, and passes its NotebookCorePort to the separately built
React test consumer, which reads the snapshot and receives route-driven
subscription updates.

The proof asserts that the port exposes only getSnapshot and subscribe,
and that route activation and port consumption make no bootstrap,
connect or close call, so the physical WebSocket lifecycle stays with
the shell. It also fails on errors Angular's ErrorHandler logs, so a
production component that throws against an incomplete test double
cannot pass silently.

The route proof runs with the port identity proof in the
integration-test phase of -Pweb-e2e, on the anonymous leg only.

Co-authored-by: YONGJAE LEE (이용재) <dev.yongjaelee@gmail.com>
});
const address = server.address();
assert.ok(address && typeof address === 'object');
const browser = await chromium.launch();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Chromium fails to launch, the harness never returns its cleanup handle, leaving the HTTP server open. We close local test servers on failure in capture-fixtures.spec.ts. Could we cover this failure path here too?

Suggested change
const browser = await chromium.launch();
let browser;
try {
browser = await chromium.launch();
} catch (error) {
await new Promise(resolve => server.close(resolve));
throw error;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in f38120b. I named the callback resolveClose instead of resolve, because this file imports resolve from node:path. I checked the failure path by pointing PLAYWRIGHT_BROWSERS_PATH at an empty directory. Before the change the process never exited because the server stayed open. After it, the proof fails immediately with the launch error.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants