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/useNotebookLoader.ts b/labextension/src/widgets/hooks/useNotebookLoader.ts index bba825a73..a048f462b 100644 --- a/labextension/src/widgets/hooks/useNotebookLoader.ts +++ b/labextension/src/widgets/hooks/useNotebookLoader.ts @@ -85,6 +85,8 @@ export function useNotebookLoader({ setExperiments, setMetadata, experimentsRef, + isLoadingRef, + loadedNotebookRef, setIsEnabled, resetForNoNotebook, } = setters; @@ -95,125 +97,209 @@ export function useNotebookLoader({ return; } - const commands = new Commands(notebook, kernel); - await notebook.sessionContext.ready; + // 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 [kfpUiHost, deployPanelCustomLinks] = await Promise.all([ - commands.getKfpUiHost(), - commands.getDeployPanelCustomLinks(), - ]); - const resolvedKfpUiHost = kfpUiHost || DEFAULT_UI_URL; - setKfpUiHost(resolvedKfpUiHost); - setDeployPanelCustomLinks(deployPanelCustomLinks); - DeployUtils.logLinksHint(resolvedKfpUiHost, deployPanelCustomLinks); + // 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; - const notebookMetadata = NotebookUtils.getMetaData(notebook, metadataKey); - - let fetchedExperiments: IExperiment[] = []; - - if (backend) { - setNamespace(await commands.getNamespace()); + try { + const commands = new Commands(notebook, kernel); + await notebook.sessionContext.ready; + if (isStale()) { + return; + } - 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, ); - 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; + } } }, [ @@ -229,6 +315,8 @@ export function useNotebookLoader({ setExperiments, setMetadata, experimentsRef, + isLoadingRef, + loadedNotebookRef, ], ); diff --git a/labextension/src/widgets/hooks/useNotebookMetadata.ts b/labextension/src/widgets/hooks/useNotebookMetadata.ts index 0838bb06d..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,6 +62,8 @@ export interface ILoaderSetters { setDeployPanelCustomLinks: Dispatch>; metadataRef: MutableRefObject; experimentsRef: MutableRefObject; + isLoadingRef: MutableRefObject; + loadedNotebookRef: MutableRefObject; resetForNoNotebook: () => void; } @@ -102,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) => { @@ -128,15 +142,26 @@ 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); + loadedNotebookRef.current = null; + setMetadata(freshDefaultMetadata()); setExperiments([]); setGettingExperiments(false); setIsEnabled(false); setNamespace(''); setKfpUiHost(''); setDeployPanelCustomLinks({ upload: '', run: '' }); - }, []); + }, [freshDefaultMetadata]); // --- composed hooks --- @@ -156,6 +181,8 @@ export function useNotebookMetadata({ setDeployPanelCustomLinks, metadataRef, experimentsRef, + isLoadingRef, + loadedNotebookRef, resetForNoNotebook, }, }); @@ -167,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..c7021b909 100644 --- a/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts +++ b/labextension/src/widgets/hooks/useNotebookMetadataPersistence.ts @@ -12,38 +12,60 @@ // 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'; +import { resolveMetadataPersistence } from './metadataPersistenceDecision'; 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); - } + 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; + if (decision.shouldWrite && decision.target) { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { base_image: _baseImage, ...metadataToPersist } = metadata; + NotebookUtils.setMetaData( + decision.target, + metadataKey, + metadataToPersist, + ); } - }, [metadata, tracker, metadataKey]); + }, [metadata, metadataKey, isLoadingRef, loadedNotebookRef]); }