Skip to content

fix(ui): Paste Chunk fills the destination's blanks instead of dropping structure (#589) [DO NOT MERGE — needs review] - #725

Draft
skearnes wants to merge 1 commit into
mainfrom
fix/589-paste-fill-empty
Draft

fix(ui): Paste Chunk fills the destination's blanks instead of dropping structure (#589) [DO NOT MERGE — needs review]#725
skearnes wants to merge 1 commit into
mainfrom
fix/589-paste-fill-empty

Conversation

@skearnes

Copy link
Copy Markdown
Member

⚠️ Draft / do-not-merge. This changes Paste Chunk semantics on a shared, delicate code path. Per maintainer, it needs review from others before merging.

Summary

Per the maintainer decision on #589, Paste Chunk should "paste everything that isn't already set in the destination."

Today, paste applies only each entity's form-managed subset with a source-wins merge (deepMergeWithArrayMerge). Two consequences:

New behavior — fillEmptyDeepMerge

A deep fill-empty merge:

  • Keep every value the destination already has set; only fill its blanks from the chunk.
  • Non-empty arrays are kept wholesale (we don't splice source items into a list the user already populated).
  • Never paste separately-managed metadata — recordModified, automationCode, measurements (PASTE_PRESERVED_FIELDS) — preserving the fix(ui): submit filtered values on Paste Chunk (#601, #592) #704 fix even into a blank destination.
  • The form still renders its managed subset, but we now persist the full merged entity, so pasted structure/identifiers the form itself doesn't show are saved.

"Not set" = nullish, empty string, or empty array/object. 0 and false count as set.

🔎 Key review questions

  1. The PASTE_PRESERVED_FIELDS set (recordModified, automationCode, measurements) is the crux — it's what keeps fix(ui): submit filtered values on Paste Chunk (#601, #592) #704 from regressing. Is this the right/complete set of "never paste" fields across all entities (provenance, setup, product, conditions measurements, …)?
  2. Behavior change for already-pasted fields: form fields (role, amount, source, …) now fill-empty instead of source-wins, so pasting onto a field the user already set keeps their value. Intended per "everything that isn't already set," but it's a change to existing paste behavior — please confirm.
  3. Outcomes/Products follow the same path; confirm that's desired.

Test plan

  • npx tsc -b clean
  • npx vitest run561 passing; 10 new fillEmptyDeepMerge unit tests (fill-empty, preserve-set, nested, 0/false treated as set, never-paste metadata) + updated paste integration test (blank doi filled, recordModified not leaked).
  • prettier / eslint clean
  • Runtime verification in progress (driving copy/paste in the reaction editor); will attach findings as a comment. The structure-paste needs an external lookup / Ketcher to set up a structure, so the runtime check focuses on the observable fill-empty behavior on a plain field.

Closes #589.

🤖 Generated with Claude Code

…ng structure (#589)

Per maintainer decision, Paste Chunk should "paste everything that isn't already set in
the destination." Today it applies only the form-managed subset with a source-wins merge,
so a copied component's structure/identifiers are dropped and the target's stale structure
survives (the #589 bug), and other set fields get clobbered.

New behavior — a deep fill-empty merge (fillEmptyDeepMerge):
- Keep every value the destination already has set; only fill its blanks from the chunk.
- Non-empty arrays are kept wholesale (don't splice source items into a populated list).
- Never paste separately-managed metadata — recordModified, automationCode, measurements
  (PASTE_PRESERVED_FIELDS) — preserving the #704 fix even into a blank destination.
- The form still renders its managed subset, but we now persist the full merged entity so
  pasted structure/identifiers the form doesn't itself show are saved.

Covered by fillEmptyDeepMerge unit tests (fill-empty, preserve-set, nested, 0/false,
never-paste metadata) and the updated paste integration test.

NOTE: behavioral change to a shared, delicate code path — holding for review by others
before merge, per maintainer.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 55.64%. Comparing base (167943c) to head (0f0840a).

Files with missing lines Patch % Lines
...ies/ReactionEntityForm/reactionEntityForm.utils.ts 96.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   55.57%   55.64%   +0.07%     
==========================================
  Files         457      457              
  Lines       18355    18387      +32     
  Branches      742      753      +11     
==========================================
+ Hits        10200    10231      +31     
- Misses       8142     8143       +1     
  Partials       13       13              
Flag Coverage Δ
python 83.69% <ø> (ø)
ui 51.39% <97.50%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...Entities/ReactionEntityForm/ReactionEntityForm.tsx 94.07% <100.00%> (+0.08%) ⬆️
...ies/ReactionEntityForm/reactionEntityForm.utils.ts 93.33% <96.66%> (+1.33%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@skearnes

Copy link
Copy Markdown
Member Author

Runtime verification note. I drove the no-auth stack to exercise this end-to-end in the reaction editor, but reliably automating the full path (add component A → set fields → Save → Copy Chunk → add component B → set one field → Paste Chunk → read back) kept fighting the editor's Drawer + "Reaction updated" toast + form-remount-on-save behavior. Rather than keep grinding browser automation, I'm relying on the deterministic coverage:

  • fillEmptyDeepMerge unit tests (10): fill-empty, preserve a value the destination already set, keep an existing structure rather than replacing it, nested fill without clobbering, 0/false treated as set, and never-paste metadata (recordModified/automationCode/measurements) even into a blank destination.
  • Paste integration test: confirms onPasteChunk submits the merged entity — blank doi filled from the chunk, the chunk's recordModified not leaked.

The editor flow is best exercised by a reviewer directly (the Key review questions in the description are the things to poke at). Happy to pair on a Playwright E2E for this once the semantics are agreed.

🤖 Generated with Claude Code

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.

The Structure is not deleted when performing the Paste Chunk action for Components.

2 participants