From a56f6ea0730ee06141e49e48938fca73bd62c211 Mon Sep 17 00:00:00 2001 From: Hariharanpugazh Date: Tue, 1 Sep 2026 18:17:51 +0800 Subject: [PATCH 1/3] fix(frontend): reset left panel metadata on notebook switch When switching between notebooks, the left panel kept rendering the previous notebook's experiment, pipeline name, and description while the async loading RPCs (getKfpUiHost, getNamespace, getExperiments) were still pending. The metadata state was only updated after those awaits resolved, so stale values were visible during the gap. Reset the panel to fresh defaults synchronously at the start of every notebook switch (before any await) and surface the existing experiment 'Loading...' state via gettingExperiments. Also guard the async load against completing after the user has switched again, so a late-resolving load cannot clobber the now-active notebook's metadata. Fixes #644 Signed-off-by: Hariharanpugazh --- .../src/widgets/hooks/useNotebookLoader.ts | 33 ++++++++++++++++++- .../src/widgets/hooks/useNotebookMetadata.ts | 30 +++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/labextension/src/widgets/hooks/useNotebookLoader.ts b/labextension/src/widgets/hooks/useNotebookLoader.ts index bba825a73..5c1f215c6 100644 --- a/labextension/src/widgets/hooks/useNotebookLoader.ts +++ b/labextension/src/widgets/hooks/useNotebookLoader.ts @@ -87,6 +87,7 @@ export function useNotebookLoader({ experimentsRef, setIsEnabled, resetForNoNotebook, + resetForNotebookSwitch, } = setters; const loadNotebookPanel = useCallback( @@ -95,13 +96,24 @@ export function useNotebookLoader({ return; } + // The load sequence is async and the user may switch notebooks again + // before it resolves. If that happens, a late-completing load must not + // clobber the metadata of the notebook that is now active. + const isStale = () => tracker.currentWidget !== notebook; + const commands = new Commands(notebook, kernel); await notebook.sessionContext.ready; + if (isStale()) { + return; + } const [kfpUiHost, deployPanelCustomLinks] = await Promise.all([ commands.getKfpUiHost(), commands.getDeployPanelCustomLinks(), ]); + if (isStale()) { + return; + } const resolvedKfpUiHost = kfpUiHost || DEFAULT_UI_URL; setKfpUiHost(resolvedKfpUiHost); setDeployPanelCustomLinks(deployPanelCustomLinks); @@ -112,11 +124,18 @@ export function useNotebookLoader({ let fetchedExperiments: IExperiment[] = []; if (backend) { - setNamespace(await commands.getNamespace()); + const namespace = await commands.getNamespace(); + if (isStale()) { + return; + } + setNamespace(namespace); const nbFilePath = getNotebookPath(notebook); if (nbFilePath) { await commands.resumeStateIfExploreNotebook(nbFilePath); + if (isStale()) { + return; + } } setGettingExperiments(true); @@ -125,6 +144,9 @@ export function useNotebookLoader({ currentMeta.experiment, currentMeta.experiment_name, ); + if (isStale()) { + return; + } fetchedExperiments = expResult.experiments; setExperiments(expResult.experiments); @@ -215,6 +237,11 @@ export function useNotebookLoader({ base_image: DefaultState.metadata.base_image, })); } + + // Loading has finished for this notebook: clear the loading flag that + // resetForNotebookSwitch set at the start of the switch. Covers the + // non-backend path too, where nothing else resets it. + setGettingExperiments(false); }, [ tracker, @@ -238,6 +265,9 @@ export function useNotebookLoader({ notebook: NotebookPanel | null, ) => { if (notebook) { + // Clear the previous notebook's metadata synchronously so the panel + // does not display stale values while the async load below runs. + resetForNotebookSwitch(); await loadNotebookPanel(notebook); setIsEnabled(prev => enableKaleByDefault || prev); } else { @@ -260,5 +290,6 @@ export function useNotebookLoader({ enableKaleByDefault, setIsEnabled, resetForNoNotebook, + resetForNotebookSwitch, ]); } diff --git a/labextension/src/widgets/hooks/useNotebookMetadata.ts b/labextension/src/widgets/hooks/useNotebookMetadata.ts index 0838bb06d..7d22087a3 100644 --- a/labextension/src/widgets/hooks/useNotebookMetadata.ts +++ b/labextension/src/widgets/hooks/useNotebookMetadata.ts @@ -63,6 +63,7 @@ export interface ILoaderSetters { metadataRef: MutableRefObject; experimentsRef: MutableRefObject; resetForNoNotebook: () => void; + resetForNotebookSwitch: () => void; } interface IUseNotebookMetadataParams { @@ -128,15 +129,39 @@ export function useNotebookMetadata({ setMetadata(prev => ({ ...prev, volumes })); }, []); + // Build a fresh default metadata object so we never mutate (or hand out a + // reference to) the shared DefaultState.metadata. + const freshDefaultMetadata = useCallback( + (): IKaleNotebookMetadata => ({ + ...defaultMetadata, + experiment: { ...defaultMetadata.experiment }, + }), + [], + ); + const resetForNoNotebook = useCallback(() => { - setMetadata(defaultMetadata); + setMetadata(freshDefaultMetadata()); setExperiments([]); setGettingExperiments(false); setIsEnabled(false); setNamespace(''); setKfpUiHost(''); setDeployPanelCustomLinks({ upload: '', run: '' }); - }, []); + }, [freshDefaultMetadata]); + + // Synchronously clear the panel to defaults at the start of a notebook + // switch, before the async loading RPCs resolve. Without this, the panel + // keeps rendering the previous notebook's metadata during the async gap. + // isEnabled is intentionally left untouched here: it is resolved once the + // new notebook finishes loading. + const resetForNotebookSwitch = useCallback(() => { + setMetadata(freshDefaultMetadata()); + setExperiments([]); + setGettingExperiments(true); + setNamespace(''); + setKfpUiHost(''); + setDeployPanelCustomLinks({ upload: '', run: '' }); + }, [freshDefaultMetadata]); // --- composed hooks --- @@ -157,6 +182,7 @@ export function useNotebookMetadata({ metadataRef, experimentsRef, resetForNoNotebook, + resetForNotebookSwitch, }, }); From e417b3fb2ca5558dc7adb4339a12fe3229baa15d Mon Sep 17 00:00:00 2001 From: Hariharanpugazh Date: Wed, 2 Sep 2026 12:16:21 +0800 Subject: [PATCH 2/3] fix(frontend): persist pipeline metadata to its owning notebook The previous fix reset panel state on switch, but two deeper problems remained that caused metadata to leak between notebooks and the panel to hang when KFP was unreachable. Root cause of the leak: the persistence effect wrote metadata to tracker.currentWidget, which is resolved when the effect runs (after paint), not when the edit was made. Switching tabs before the effect flushed wrote one notebook's metadata into another. The effect now targets the notebook the metadata was loaded from (loadedNotebookRef), so edits always save to the notebook they belong to. Also: guard the metadata load so the loader's own setMetadata calls are not written back to the file (isLoadingRef), wrap the load in try/finally so the loading flag can never stay stuck, and stop backend RPC failures (unreachable KFP) from aborting the metadata load or freezing the panel. Fixes #644 Signed-off-by: Hariharanpugazh --- .../src/widgets/hooks/useNotebookLoader.ts | 307 +++++++++++------- .../src/widgets/hooks/useNotebookMetadata.ts | 38 ++- .../hooks/useNotebookMetadataPersistence.ts | 51 ++- 3 files changed, 239 insertions(+), 157 deletions(-) diff --git a/labextension/src/widgets/hooks/useNotebookLoader.ts b/labextension/src/widgets/hooks/useNotebookLoader.ts index 5c1f215c6..a048f462b 100644 --- a/labextension/src/widgets/hooks/useNotebookLoader.ts +++ b/labextension/src/widgets/hooks/useNotebookLoader.ts @@ -85,9 +85,10 @@ export function useNotebookLoader({ setExperiments, setMetadata, experimentsRef, + isLoadingRef, + loadedNotebookRef, setIsEnabled, resetForNoNotebook, - resetForNotebookSwitch, } = setters; const loadNotebookPanel = useCallback( @@ -101,147 +102,205 @@ export function useNotebookLoader({ // clobber the metadata of the notebook that is now active. const isStale = () => tracker.currentWidget !== notebook; - const commands = new Commands(notebook, kernel); - await notebook.sessionContext.ready; - if (isStale()) { - return; - } - - const [kfpUiHost, deployPanelCustomLinks] = await Promise.all([ - commands.getKfpUiHost(), - commands.getDeployPanelCustomLinks(), - ]); - if (isStale()) { - return; - } - const resolvedKfpUiHost = kfpUiHost || DEFAULT_UI_URL; - setKfpUiHost(resolvedKfpUiHost); - setDeployPanelCustomLinks(deployPanelCustomLinks); - DeployUtils.logLinksHint(resolvedKfpUiHost, deployPanelCustomLinks); - - const notebookMetadata = NotebookUtils.getMetaData(notebook, metadataKey); + // Guard persistence for the whole load: every setMetadata below reflects + // what we read from the notebook, not a user edit, so it must not be + // written back to the file. Cleared in the finally so it can never stay + // stuck (e.g. on an isStale early return). + isLoadingRef.current = true; - let fetchedExperiments: IExperiment[] = []; - - if (backend) { - const namespace = await commands.getNamespace(); + try { + const commands = new Commands(notebook, kernel); + await notebook.sessionContext.ready; if (isStale()) { return; } - setNamespace(namespace); - const nbFilePath = getNotebookPath(notebook); - if (nbFilePath) { - await commands.resumeStateIfExploreNotebook(nbFilePath); + // KFP UI host / custom links come from the backend and may be + // unreachable; a failure here must not abort the metadata load. + try { + const [kfpUiHost, deployPanelCustomLinks] = await Promise.all([ + commands.getKfpUiHost(), + commands.getDeployPanelCustomLinks(), + ]); if (isStale()) { return; } + const resolvedKfpUiHost = kfpUiHost || DEFAULT_UI_URL; + setKfpUiHost(resolvedKfpUiHost); + setDeployPanelCustomLinks(deployPanelCustomLinks); + DeployUtils.logLinksHint(resolvedKfpUiHost, deployPanelCustomLinks); + } catch (error) { + if (isStale()) { + return; + } + console.warn('Kale: failed to fetch KFP UI host / links', error); + setKfpUiHost(DEFAULT_UI_URL); } - setGettingExperiments(true); - const currentMeta = metadataRef.current; - const expResult = await commands.getExperiments( - currentMeta.experiment, - currentMeta.experiment_name, + const notebookMetadata = NotebookUtils.getMetaData( + notebook, + metadataKey, ); - if (isStale()) { - return; - } - fetchedExperiments = expResult.experiments; - setExperiments(expResult.experiments); - setGettingExperiments(false); - setMetadata(prev => ({ - ...prev, - experiment: expResult.experiment, - experiment_name: expResult.experiment_name, - })); - } + let fetchedExperiments: IExperiment[] = []; - if (notebookMetadata) { - const currentMeta = metadataRef.current; - const currentExperiments = experimentsRef.current; - let experiment: IExperiment = currentMeta.experiment; - let experiment_name: string = currentMeta.experiment_name; + if (backend) { + // Namespace/resume-state failures (e.g. running outside a cluster) + // must not abort the load, so the pipeline metadata still populates. + try { + const namespace = await commands.getNamespace(); + if (isStale()) { + return; + } + setNamespace(namespace); - if (notebookMetadata['experiment']) { - experiment = { - id: - notebookMetadata['experiment']['id'] || currentMeta.experiment.id, - name: - notebookMetadata['experiment']['name'] || - currentMeta.experiment.name, - }; - experiment_name = experiment.name; - const experimentsToUse = - fetchedExperiments.length > 0 - ? fetchedExperiments - : currentExperiments; - if ( - !experiment.id && - !experiment.name && - experimentsToUse.length > 0 - ) { - experiment = experimentsToUse[0]; - experiment_name = experimentsToUse[0].name; + const nbFilePath = getNotebookPath(notebook); + if (nbFilePath) { + await commands.resumeStateIfExploreNotebook(nbFilePath); + if (isStale()) { + return; + } + } + } catch (error) { + if (isStale()) { + return; + } + console.warn( + 'Kale: failed to resolve namespace / resume state', + error, + ); } - } else if (notebookMetadata['experiment_name']) { - const matching = currentExperiments.filter( - (e: IExperiment) => e.name === notebookMetadata['experiment_name'], - ); - if (matching.length > 0) { - experiment = matching[0]; - } else { + + const currentMeta = metadataRef.current; + setGettingExperiments(true); + // A failure fetching experiments (e.g. KFP unreachable) must not + // abort the rest of the load: the pipeline metadata below should + // still populate. Degrade to an empty experiment list instead. + try { + const expResult = await commands.getExperiments( + currentMeta.experiment, + currentMeta.experiment_name, + ); + if (isStale()) { + return; + } + fetchedExperiments = expResult.experiments; + + setExperiments(expResult.experiments); + setMetadata(prev => ({ + ...prev, + experiment: expResult.experiment, + experiment_name: expResult.experiment_name, + })); + } catch (error) { + if (isStale()) { + return; + } + console.warn('Kale: failed to fetch experiments', error); + setExperiments([]); + } + } + + if (notebookMetadata) { + const currentMeta = metadataRef.current; + const currentExperiments = experimentsRef.current; + let experiment: IExperiment = currentMeta.experiment; + let experiment_name: string = currentMeta.experiment_name; + + if (notebookMetadata['experiment']) { experiment = { - id: NEW_EXPERIMENT.id, - name: notebookMetadata['experiment_name'], + id: + notebookMetadata['experiment']['id'] || + currentMeta.experiment.id, + name: + notebookMetadata['experiment']['name'] || + currentMeta.experiment.name, }; - } - experiment_name = notebookMetadata['experiment_name']; - } else { - if (currentExperiments.length > 0) { - experiment = currentExperiments[0]; - experiment_name = currentExperiments[0].name; - } else if (currentMeta.experiment.id || currentMeta.experiment.name) { - experiment = currentMeta.experiment; - experiment_name = currentMeta.experiment_name || ''; + experiment_name = experiment.name; + const experimentsToUse = + fetchedExperiments.length > 0 + ? fetchedExperiments + : currentExperiments; + if ( + !experiment.id && + !experiment.name && + experimentsToUse.length > 0 + ) { + experiment = experimentsToUse[0]; + experiment_name = experimentsToUse[0].name; + } + } else if (notebookMetadata['experiment_name']) { + const matching = currentExperiments.filter( + (e: IExperiment) => + e.name === notebookMetadata['experiment_name'], + ); + if (matching.length > 0) { + experiment = matching[0]; + } else { + experiment = { + id: NEW_EXPERIMENT.id, + name: notebookMetadata['experiment_name'], + }; + } + experiment_name = notebookMetadata['experiment_name']; } else { - experiment = { id: '', name: '' }; - experiment_name = ''; + if (currentExperiments.length > 0) { + experiment = currentExperiments[0]; + experiment_name = currentExperiments[0].name; + } else if ( + currentMeta.experiment.id || + currentMeta.experiment.name + ) { + experiment = currentMeta.experiment; + experiment_name = currentMeta.experiment_name || ''; + } else { + experiment = { id: '', name: '' }; + experiment_name = ''; + } } - } - const defaultPipelineName = getNotebookFileName(notebook); - const sanitized = sanitizePipelineName(defaultPipelineName); - setMetadata({ - ...notebookMetadata, - experiment, - experiment_name, - pipeline_name: - notebookMetadata['pipeline_name'] && - notebookMetadata['pipeline_name'] !== '' - ? notebookMetadata['pipeline_name'] - : sanitized, - pipeline_description: notebookMetadata['pipeline_description'] || '', - base_image: '', - steps_defaults: DefaultState.metadata.steps_defaults, - }); - } else { - const defaultPipelineName = getNotebookFileName(notebook); - const sanitized = sanitizePipelineName(defaultPipelineName); - setMetadata(prev => ({ - ...DefaultState.metadata, - experiment: prev.experiment, - experiment_name: prev.experiment_name, - pipeline_name: sanitized, - base_image: DefaultState.metadata.base_image, - })); + const defaultPipelineName = getNotebookFileName(notebook); + const sanitized = sanitizePipelineName(defaultPipelineName); + setMetadata({ + ...notebookMetadata, + experiment, + experiment_name, + pipeline_name: + notebookMetadata['pipeline_name'] && + notebookMetadata['pipeline_name'] !== '' + ? notebookMetadata['pipeline_name'] + : sanitized, + pipeline_description: + notebookMetadata['pipeline_description'] || '', + base_image: '', + steps_defaults: DefaultState.metadata.steps_defaults, + }); + } else { + const defaultPipelineName = getNotebookFileName(notebook); + const sanitized = sanitizePipelineName(defaultPipelineName); + setMetadata({ + ...DefaultState.metadata, + experiment: { id: '', name: '' }, + experiment_name: '', + pipeline_name: sanitized, + base_image: DefaultState.metadata.base_image, + }); + } + } finally { + // The load is done (or was abandoned via an isStale early return). + // Always clear the loading state so the panel leaves "Loading..." and + // so genuine user edits are persisted again. Only clear for the + // notebook that is still active, to avoid a stale load re-enabling + // persistence mid-switch. + if (tracker.currentWidget === notebook) { + // Mark this notebook as the owner of the current metadata BEFORE + // re-enabling persistence, so the next user edit is written back to + // this notebook. + loadedNotebookRef.current = notebook; + setGettingExperiments(false); + isLoadingRef.current = false; + } } - - // Loading has finished for this notebook: clear the loading flag that - // resetForNotebookSwitch set at the start of the switch. Covers the - // non-backend path too, where nothing else resets it. - setGettingExperiments(false); }, [ tracker, @@ -256,6 +315,8 @@ export function useNotebookLoader({ setExperiments, setMetadata, experimentsRef, + isLoadingRef, + loadedNotebookRef, ], ); @@ -265,9 +326,6 @@ export function useNotebookLoader({ notebook: NotebookPanel | null, ) => { if (notebook) { - // Clear the previous notebook's metadata synchronously so the panel - // does not display stale values while the async load below runs. - resetForNotebookSwitch(); await loadNotebookPanel(notebook); setIsEnabled(prev => enableKaleByDefault || prev); } else { @@ -290,6 +348,5 @@ export function useNotebookLoader({ enableKaleByDefault, setIsEnabled, resetForNoNotebook, - resetForNotebookSwitch, ]); } diff --git a/labextension/src/widgets/hooks/useNotebookMetadata.ts b/labextension/src/widgets/hooks/useNotebookMetadata.ts index 7d22087a3..a3e0f9c10 100644 --- a/labextension/src/widgets/hooks/useNotebookMetadata.ts +++ b/labextension/src/widgets/hooks/useNotebookMetadata.ts @@ -20,7 +20,7 @@ import { useRef, useState, } from 'react'; -import { INotebookTracker } from '@jupyterlab/notebook'; +import { INotebookTracker, NotebookPanel } from '@jupyterlab/notebook'; import { Kernel } from '@jupyterlab/services'; import { DefaultState, @@ -62,8 +62,9 @@ export interface ILoaderSetters { setDeployPanelCustomLinks: Dispatch>; metadataRef: MutableRefObject; experimentsRef: MutableRefObject; + isLoadingRef: MutableRefObject; + loadedNotebookRef: MutableRefObject; resetForNoNotebook: () => void; - resetForNotebookSwitch: () => void; } interface IUseNotebookMetadataParams { @@ -103,6 +104,18 @@ export function useNotebookMetadata({ const experimentsRef = useRef(experiments); experimentsRef.current = experiments; + // True while a notebook is being loaded/switched. The metadata state changes + // the loader makes during a load are programmatic (they reflect what was read + // from the notebook), not user edits, so persistence must not write them back + // to the notebook file while this is set. + const isLoadingRef = useRef(false); + + // The notebook the current metadata state belongs to. Persistence writes to + // this notebook rather than to tracker.currentWidget, so a metadata edit is + // always saved to the notebook it was made against, even if the active tab + // has since changed. + const loadedNotebookRef = useRef(null); + // --- updaters (exposed to LeftPanel form inputs) --- const updateExperiment = useCallback((experiment: IExperiment) => { @@ -140,6 +153,7 @@ export function useNotebookMetadata({ ); const resetForNoNotebook = useCallback(() => { + loadedNotebookRef.current = null; setMetadata(freshDefaultMetadata()); setExperiments([]); setGettingExperiments(false); @@ -149,20 +163,6 @@ export function useNotebookMetadata({ setDeployPanelCustomLinks({ upload: '', run: '' }); }, [freshDefaultMetadata]); - // Synchronously clear the panel to defaults at the start of a notebook - // switch, before the async loading RPCs resolve. Without this, the panel - // keeps rendering the previous notebook's metadata during the async gap. - // isEnabled is intentionally left untouched here: it is resolved once the - // new notebook finishes loading. - const resetForNotebookSwitch = useCallback(() => { - setMetadata(freshDefaultMetadata()); - setExperiments([]); - setGettingExperiments(true); - setNamespace(''); - setKfpUiHost(''); - setDeployPanelCustomLinks({ upload: '', run: '' }); - }, [freshDefaultMetadata]); - // --- composed hooks --- useNotebookLoader({ @@ -181,8 +181,9 @@ export function useNotebookMetadata({ setDeployPanelCustomLinks, metadataRef, experimentsRef, + isLoadingRef, + loadedNotebookRef, resetForNoNotebook, - resetForNotebookSwitch, }, }); @@ -193,9 +194,10 @@ export function useNotebookMetadata({ }); useNotebookMetadataPersistence({ - tracker, metadata, metadataKey: KALE_NOTEBOOK_METADATA_KEY, + isLoadingRef, + loadedNotebookRef, }); return { diff --git a/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts b/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts index fc8c90ca6..d00279477 100644 --- a/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts +++ b/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts @@ -12,38 +12,61 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { useEffect, useRef } from 'react'; -import { INotebookTracker } from '@jupyterlab/notebook'; +import { MutableRefObject, useEffect, useRef } from 'react'; +import { NotebookPanel } from '@jupyterlab/notebook'; import NotebookUtils from '../../lib/NotebookUtils'; import { IKaleNotebookMetadata } from '../LeftPanelTypes'; interface IUseNotebookMetadataPersistenceParams { - tracker: INotebookTracker; metadata: IKaleNotebookMetadata; metadataKey: string; + // Set while the loader is populating state from a notebook. Metadata changes + // during that window are programmatic (a reflection of what was read), not + // user edits, so they must not be written back to the notebook file. + isLoadingRef: MutableRefObject; + // Identifies the notebook the current metadata state belongs to. The write + // targets this notebook, NOT tracker.currentWidget: the persistence effect + // runs asynchronously (after paint), so by the time it runs the user may + // have switched tabs and tracker.currentWidget may point at a different + // notebook. Writing to currentWidget would leak one notebook's metadata into + // another. Writing to the owning notebook keeps each notebook's data with it. + loadedNotebookRef: MutableRefObject; } /** - * Hook that writes the current metadata state back to the active notebook's + * Hook that writes the current metadata state back to its owning notebook's * .ipynb file whenever it changes, keeping the form and the file in sync. */ export function useNotebookMetadataPersistence({ - tracker, metadata, metadataKey, + isLoadingRef, + loadedNotebookRef, }: IUseNotebookMetadataPersistenceParams) { const prevMetadataJsonRef = useRef(JSON.stringify(metadata)); useEffect(() => { const json = JSON.stringify(metadata); - if (json !== prevMetadataJsonRef.current) { - prevMetadataJsonRef.current = json; - const notebook = tracker.currentWidget; - if (notebook) { - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { base_image: _baseImage, ...metadataToPersist } = metadata; - NotebookUtils.setMetaData(notebook, metadataKey, metadataToPersist); - } + if (json === prevMetadataJsonRef.current) { + return; } - }, [metadata, tracker, metadataKey]); + // Always track the latest metadata we have seen so that, once loading + // finishes, the next genuine user edit is detected as a change. + prevMetadataJsonRef.current = json; + // Skip write-back while the loader is populating state: those changes + // reflect what was just read from the notebook, and persisting them can + // clobber the freshly-activated notebook's saved metadata. + if (isLoadingRef.current) { + return; + } + // Persist to the notebook this metadata belongs to, not whatever tab is + // active now: this effect runs after a paint, and the user may have + // switched notebooks in the meantime. + const notebook = loadedNotebookRef.current; + if (notebook && !notebook.isDisposed) { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { base_image: _baseImage, ...metadataToPersist } = metadata; + NotebookUtils.setMetaData(notebook, metadataKey, metadataToPersist); + } + }, [metadata, metadataKey, isLoadingRef, loadedNotebookRef]); } From 58907854480343fc15ca35ed9f31996a257f2938 Mon Sep 17 00:00:00 2001 From: Hariharanpugazh Date: Wed, 2 Sep 2026 12:27:18 +0800 Subject: [PATCH 3/3] test(frontend): cover metadata persistence targeting the owning notebook Extract the persistence decision into a pure resolveMetadataPersistence() helper (no runtime JupyterLab imports, so it is unit-testable in isolation) and add regression tests for #644. The key assertion guards the leak fixed in the previous commit: a metadata change is written to the notebook it was loaded from, never to the currently active tab. Also covers skipping while the loader is populating state, when metadata is unchanged, and when the owning notebook is missing or disposed. Signed-off-by: Hariharanpugazh --- .../metadataPersistenceDecision.spec.ts | 102 ++++++++++++++++++ .../hooks/metadataPersistenceDecision.ts | 67 ++++++++++++ .../hooks/useNotebookMetadataPersistence.ts | 33 +++--- 3 files changed, 185 insertions(+), 17 deletions(-) create mode 100644 labextension/src/__tests__/metadataPersistenceDecision.spec.ts create mode 100644 labextension/src/widgets/hooks/metadataPersistenceDecision.ts diff --git a/labextension/src/__tests__/metadataPersistenceDecision.spec.ts b/labextension/src/__tests__/metadataPersistenceDecision.spec.ts new file mode 100644 index 000000000..680f1d414 --- /dev/null +++ b/labextension/src/__tests__/metadataPersistenceDecision.spec.ts @@ -0,0 +1,102 @@ +// Copyright 2026 The Kubeflow Authors. +// +// 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 type { NotebookPanel } from '@jupyterlab/notebook'; +import { resolveMetadataPersistence } from '../widgets/hooks/metadataPersistenceDecision'; + +// Minimal NotebookPanel stub: the decision only reads `isDisposed` and uses +// object identity to distinguish notebooks. +function makeNotebook(isDisposed = false): NotebookPanel { + return { isDisposed } as unknown as NotebookPanel; +} + +describe('resolveMetadataPersistence (#644 regression)', () => { + it('writes changed metadata to the owning notebook', () => { + const owner = makeNotebook(); + const decision = resolveMetadataPersistence({ + json: '{"pipeline_name":"a"}', + prevJson: '{"pipeline_name":""}', + isLoading: false, + loadedNotebook: owner, + }); + expect(decision.shouldWrite).toBe(true); + expect(decision.target).toBe(owner); + }); + + it('targets the owning notebook, not the currently active tab', () => { + // This is the core of the #644 leak: the effect runs after a paint, and by + // then the active tab may be a different notebook. The write must still go + // to the notebook the metadata was loaded from. + const owningNotebook = makeNotebook(); + const nowActiveNotebook = makeNotebook(); + + const decision = resolveMetadataPersistence({ + json: '{"pipeline_name":"belongs-to-owner"}', + prevJson: '{"pipeline_name":""}', + isLoading: false, + loadedNotebook: owningNotebook, + }); + + expect(decision.shouldWrite).toBe(true); + expect(decision.target).toBe(owningNotebook); + // Never the active tab. + expect(decision.target).not.toBe(nowActiveNotebook); + }); + + it('does not write while the loader is populating state', () => { + const owner = makeNotebook(); + const decision = resolveMetadataPersistence({ + json: '{"pipeline_name":"a"}', + prevJson: '{"pipeline_name":""}', + isLoading: true, + loadedNotebook: owner, + }); + expect(decision.shouldWrite).toBe(false); + expect(decision.target).toBeNull(); + }); + + it('does not write when metadata is unchanged', () => { + const owner = makeNotebook(); + const sameJson = '{"pipeline_name":"a"}'; + const decision = resolveMetadataPersistence({ + json: sameJson, + prevJson: sameJson, + isLoading: false, + loadedNotebook: owner, + }); + expect(decision.shouldWrite).toBe(false); + }); + + it('does not write when there is no owning notebook', () => { + const decision = resolveMetadataPersistence({ + json: '{"pipeline_name":"a"}', + prevJson: '{"pipeline_name":""}', + isLoading: false, + loadedNotebook: null, + }); + expect(decision.shouldWrite).toBe(false); + expect(decision.target).toBeNull(); + }); + + it('does not write when the owning notebook has been disposed', () => { + const disposed = makeNotebook(true); + const decision = resolveMetadataPersistence({ + json: '{"pipeline_name":"a"}', + prevJson: '{"pipeline_name":""}', + isLoading: false, + loadedNotebook: disposed, + }); + expect(decision.shouldWrite).toBe(false); + }); +}); diff --git a/labextension/src/widgets/hooks/metadataPersistenceDecision.ts b/labextension/src/widgets/hooks/metadataPersistenceDecision.ts new file mode 100644 index 000000000..2f74d1503 --- /dev/null +++ b/labextension/src/widgets/hooks/metadataPersistenceDecision.ts @@ -0,0 +1,67 @@ +// Copyright 2026 The Kubeflow Authors. +// +// 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 type { NotebookPanel } from '@jupyterlab/notebook'; + +export interface IPersistenceDecisionInput { + // JSON of the metadata about to be considered for persistence. + json: string; + // JSON of the metadata last seen by the effect (change detection). + prevJson: string; + // Whether the loader is currently populating metadata state. + isLoading: boolean; + // The notebook the current metadata belongs to (write target), or null. + loadedNotebook: NotebookPanel | null; +} + +export interface IPersistenceDecision { + // Whether to write the metadata back to the notebook file. + shouldWrite: boolean; + // The notebook to write to, if shouldWrite is true. + target: NotebookPanel | null; +} + +/** + * Pure decision for whether the current metadata should be written back to a + * notebook, and to which notebook. + * + * The critical rule (regression guard for #644): the write target is the + * notebook the metadata was loaded from (`loadedNotebook`), never the currently + * active tab. The persistence effect runs asynchronously after a paint, so by + * the time it runs the active tab may already be a different notebook; writing + * to the active tab would leak one notebook's metadata into another. + * + * Kept free of runtime imports so it can be unit-tested without pulling in the + * JupyterLab module graph. + */ +export function resolveMetadataPersistence({ + json, + prevJson, + isLoading, + loadedNotebook, +}: IPersistenceDecisionInput): IPersistenceDecision { + // Unchanged metadata: nothing to persist. + if (json === prevJson) { + return { shouldWrite: false, target: null }; + } + // Loader is populating state: these are reads, not user edits. + if (isLoading) { + return { shouldWrite: false, target: null }; + } + // No owning notebook, or it has been closed: nowhere to write. + if (!loadedNotebook || loadedNotebook.isDisposed) { + return { shouldWrite: false, target: null }; + } + return { shouldWrite: true, target: loadedNotebook }; +} diff --git a/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts b/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts index d00279477..c7021b909 100644 --- a/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts +++ b/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts @@ -16,6 +16,7 @@ import { MutableRefObject, useEffect, useRef } from 'react'; import { NotebookPanel } from '@jupyterlab/notebook'; import NotebookUtils from '../../lib/NotebookUtils'; import { IKaleNotebookMetadata } from '../LeftPanelTypes'; +import { resolveMetadataPersistence } from './metadataPersistenceDecision'; interface IUseNotebookMetadataPersistenceParams { metadata: IKaleNotebookMetadata; @@ -47,26 +48,24 @@ export function useNotebookMetadataPersistence({ useEffect(() => { const json = JSON.stringify(metadata); - if (json === prevMetadataJsonRef.current) { - return; - } - // Always track the latest metadata we have seen so that, once loading - // finishes, the next genuine user edit is detected as a change. + const decision = resolveMetadataPersistence({ + json, + prevJson: prevMetadataJsonRef.current, + isLoading: isLoadingRef.current, + loadedNotebook: loadedNotebookRef.current, + }); + // Always track the latest metadata we have seen (even when we skip the + // write) so that, once loading finishes, the next genuine user edit is + // detected as a change. prevMetadataJsonRef.current = json; - // Skip write-back while the loader is populating state: those changes - // reflect what was just read from the notebook, and persisting them can - // clobber the freshly-activated notebook's saved metadata. - if (isLoadingRef.current) { - return; - } - // Persist to the notebook this metadata belongs to, not whatever tab is - // active now: this effect runs after a paint, and the user may have - // switched notebooks in the meantime. - const notebook = loadedNotebookRef.current; - if (notebook && !notebook.isDisposed) { + if (decision.shouldWrite && decision.target) { // eslint-disable-next-line @typescript-eslint/no-unused-vars const { base_image: _baseImage, ...metadataToPersist } = metadata; - NotebookUtils.setMetaData(notebook, metadataKey, metadataToPersist); + NotebookUtils.setMetaData( + decision.target, + metadataKey, + metadataToPersist, + ); } }, [metadata, metadataKey, isLoadingRef, loadedNotebookRef]); }