Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -72,8 +72,10 @@ describe('ReactionEntityForm — Paste Chunk filtering', () => {
await waitFor(() => expect(addUpdateReactionFieldMock).toHaveBeenCalled());

const { newValue } = addUpdateReactionFieldMock.mock.calls[0][0] as { newValue: Record<string, unknown> };
// 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');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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({
Expand All @@ -124,7 +126,7 @@ export function ReactionEntityForm({
});
}
},
[filterValues, form, onSubmit],
[filterValues, form, onSubmit, reactionPartWithNestedEntities],
);

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,48 @@
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;

Check warning on line 42 in ui/src/features/reactions/ReactionEntities/ReactionEntityForm/reactionEntityForm.utils.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This assertion is unnecessary since it does not change the type of the expression.

See more on https://sonarcloud.io/project/issues?id=open-reaction-database_ord-app&issues=AZ7DpjOWq26mY3X2Ma0X&open=AZ7DpjOWq26mY3X2Ma0X&pullRequest=725
// 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<string, unknown> = { ...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 {
Expand Down
Loading