diff --git a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.paste.test.tsx b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.paste.test.tsx index 24c38e7c..948a8c6d 100644 --- a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.paste.test.tsx +++ b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.paste.test.tsx @@ -47,10 +47,10 @@ beforeEach(() => { vi.clearAllMocks(); }); -describe('ReactionEntityForm — Paste Chunk filtering', () => { - it('submits the filtered chunk, dropping fields the sidebar excludes (e.g. recordModified)', async () => { - // A clipboard chunk that includes recordModified — which the Provenance sidebar filters out - // (it is edited in its own sidebar) — plus an ordinary field that must survive the paste. +describe('ReactionEntityForm — Paste Chunk fill-empty (#589)', () => { + it("fills the destination's blanks from the chunk but never pastes separately-managed metadata", async () => { + // A clipboard chunk that includes recordModified — auto-managed metadata that must never be + // pasted (#704) — plus a doi the (blank) destination should be filled with. pasteReactionPartMock.mockResolvedValue([ { doi: '10.0000/paste-test', recordModified: { time: { value: 'leaked' } } }, 'clipboard-text', @@ -72,8 +72,10 @@ describe('ReactionEntityForm — Paste Chunk filtering', () => { await waitFor(() => expect(addUpdateReactionFieldMock).toHaveBeenCalled()); const { newValue } = addUpdateReactionFieldMock.mock.calls[0][0] as { newValue: Record }; - // The bug submitted the raw chunk, leaking recordModified into the merge; the fix submits filtered values. - expect(newValue).not.toHaveProperty('recordModified'); + // The blank doi is filled from the chunk... expect(newValue).toHaveProperty('doi', '10.0000/paste-test'); + // ...but the chunk's recordModified is not applied (it's separately-managed metadata, and the + // pasted "leaked" value never reaches the merge). + expect(JSON.stringify(newValue)).not.toContain('leaked'); }); }); diff --git a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.tsx b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.tsx index 932bced5..a738c6fb 100644 --- a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.tsx +++ b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/ReactionEntityForm.tsx @@ -25,7 +25,7 @@ import { useCallback, useContext, useEffect, useMemo, useState } from 'react'; import type { ReactionEntityContext } from 'features/reactions/ReactionEntities/reactionEntities.types.ts'; import type { ReactionSidebarInfo } from 'features/reactions/ReactionEntities/sidebarInfo/sidebarInfo.types.ts'; import { reactionContext } from '../../reactions.context.ts'; -import { copyReactionPart } from './reactionEntityForm.utils.ts'; +import { copyReactionPart, fillEmptyDeepMerge } from './reactionEntityForm.utils.ts'; import { ReactionEntityPaste } from './ReactionEntityPaste.tsx'; import { useDisclosure } from '@mantine/hooks'; import { showNotification } from 'common/utils/showNotification.tsx'; @@ -110,12 +110,14 @@ export function ReactionEntityForm({ const onPasteChunk = useCallback( (reactionPart: object) => { try { - // Submit the filtered values, not the raw clipboard chunk: fields the sidebar excludes - // (e.g. setup.automationCode, provenance.recordModified, product.measurements) are edited - // in their own sidebars and must not be merged in here, or they get duplicated/retained. - const formValues = filterValues(reactionPart); - form.setValues(formValues); - onSubmit(formValues); + // Fill the destination's blank fields from the pasted chunk without clobbering values the + // user already set, and without pulling in separately-managed metadata (automationCode, + // recordModified, measurements — see PASTE_PRESERVED_FIELDS). The form only renders its + // managed subset (filterValues), but we persist the full merged entity so pasted + // structure/identifiers the form doesn't itself show are saved too. (#589) + const merged = fillEmptyDeepMerge(reactionPartWithNestedEntities, reactionPart); + form.setValues(filterValues(merged)); + onSubmit(merged); setFormKey(crypto.randomUUID()); } catch (_e: unknown) { showNotification({ @@ -124,7 +126,7 @@ export function ReactionEntityForm({ }); } }, - [filterValues, form, onSubmit], + [filterValues, form, onSubmit, reactionPartWithNestedEntities], ); return ( diff --git a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.test.ts b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.test.ts index b7e1c186..9dc40eb1 100644 --- a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.test.ts +++ b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ import { describe, it, expect, vi, afterEach } from 'vitest'; -import { copyReactionPart, pasteReactionPart } from './reactionEntityForm.utils.ts'; +import { copyReactionPart, fillEmptyDeepMerge, pasteReactionPart } from './reactionEntityForm.utils.ts'; import { ReactionNodeEntity } from 'store/entities/reactions/reactions.types.ts'; import { ordNotesToReaction } from 'store/entities/reactions/reactionNotes/reactionNotes.converters.ts'; @@ -76,3 +76,43 @@ describe('pasteReactionPart', () => { expect(await pasteReactionPart(ReactionNodeEntity.Notes)).toEqual([null, '']); }); }); + +describe('fillEmptyDeepMerge (Paste Chunk fill-empty semantics, #589)', () => { + it("fills the destination's blank fields from the source", () => { + const dest = { reactionRole: 'REACTANT', molBlockIdentifiers: [], amount: {} }; + const source = { reactionRole: 'PRODUCT', molBlockIdentifiers: [{ value: 'MOL' }], amount: { value: '5' } }; + // Blank arrays/objects in the destination are filled; a value the destination already set is kept. + expect(fillEmptyDeepMerge(dest, source)).toEqual({ + reactionRole: 'REACTANT', + molBlockIdentifiers: [{ value: 'MOL' }], + amount: { value: '5' }, + }); + }); + + it('keeps a structure the destination already has rather than replacing it', () => { + const dest = { molBlockIdentifiers: [{ value: 'EXISTING' }] }; + const source = { molBlockIdentifiers: [{ value: 'PASTED' }] }; + expect(fillEmptyDeepMerge(dest, source)).toEqual({ molBlockIdentifiers: [{ value: 'EXISTING' }] }); + }); + + it('treats 0 and false as set (does not overwrite them)', () => { + expect(fillEmptyDeepMerge({ isLimiting: false, n: 0 }, { isLimiting: true, n: 9 })).toEqual({ + isLimiting: false, + n: 0, + }); + }); + + it('fills nested blanks without clobbering nested set values', () => { + const dest = { source: { vendor: 'Acme', catalogId: '' } }; + const source = { source: { vendor: 'Other', catalogId: 'C-1', lot: 'L-1' } }; + expect(fillEmptyDeepMerge(dest, source)).toEqual({ source: { vendor: 'Acme', catalogId: 'C-1', lot: 'L-1' } }); + }); + + it('never pastes separately-managed metadata even into a blank destination (#704)', () => { + const dest = { doi: '' }; + const source = { doi: '10.1/x', recordModified: { time: 'leaked' }, automationCode: { v: 'x' }, measurements: [1] }; + const result = fillEmptyDeepMerge(dest, source); + expect(result).toEqual({ doi: '10.1/x' }); + expect(JSON.stringify(result)).not.toContain('leaked'); + }); +}); diff --git a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.ts b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.ts index 0ce5d8db..39936396 100644 --- a/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.ts +++ b/ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.ts @@ -27,6 +27,48 @@ interface ClipboardMessage { value: object; } +/** + * Fields that are auto-generated or edited in their own dedicated sidebars and must never be + * carried over by Paste Chunk, even into an empty destination — pasting them sets wrong metadata + * or duplicates separately-managed data (see #704). Everything else is paste-eligible. + */ +const PASTE_PRESERVED_FIELDS = new Set(['recordModified', 'automationCode', 'measurements']); + +/** A value counts as "not set" (a blank the paste may fill) if it's nullish, an empty string, or an empty array/object. */ +function isUnset(value: unknown): boolean { + if (value === undefined || value === null) return true; + if (typeof value === 'string') return value.length === 0; + if (Array.isArray(value)) return value.length === 0; + if (typeof value === 'object') return Object.keys(value as object).length === 0; + // Meaningful primitives (0, false, non-empty strings) are "set". + return false; +} + +/** + * Deep "fill-empty" merge used by Paste Chunk (#589): keep every value the destination has + * already set, and only fill the destination's blanks from the pasted source. Non-empty arrays + * are kept wholesale (we don't splice source items into a list the user already populated), and + * {@link PASTE_PRESERVED_FIELDS} are never taken from the source. This lets a pasted component + * bring over its structure/identifiers into an empty target without clobbering anything the user + * already entered. + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function fillEmptyDeepMerge(destination: any, source: any): any { + if (isUnset(source)) return destination; + if (isUnset(destination)) return source; + // Both sides are set. Recurse into plain objects; for primitives and (non-empty) arrays the + // destination already has a value, so it wins. + if (Array.isArray(destination) || typeof destination !== 'object' || typeof source !== 'object') { + return destination; + } + const result: Record = { ...destination }; + for (const key of Object.keys(source)) { + if (PASTE_PRESERVED_FIELDS.has(key)) continue; + result[key] = fillEmptyDeepMerge(destination[key], source[key]); + } + return result; +} + // TODO parse\stringify via ord-schema // eslint-disable-next-line @typescript-eslint/no-explicit-any function replacer(_: unknown, value: any): any {