Skip to content

#9866 - Additional elements appear when inserting a file saved in mol format the expand monomers package#10138

Closed
Lianamirru wants to merge 5 commits into
masterfrom
9866-mol-format
Closed

#9866 - Additional elements appear when inserting a file saved in mol format the expand monomers package#10138
Lianamirru wants to merge 5 commits into
masterfrom
9866-mol-format

Conversation

@Lianamirru
Copy link
Copy Markdown
Collaborator

@Lianamirru Lianamirru commented May 29, 2026

How the feature works? / How did you fix the issue?

(Screenshots, videos, or GIFs, if applicable)

Check list

  • unit-tests written
  • e2e-tests written
  • documentation updated
  • PR name follows the pattern #1234 – issue name
  • branch name doesn't contain '#'
  • PR is linked with the issue
  • base branch (master or release/xx) is correct
  • task status changed to "Code review"
  • reviewers are notified about the pull request

@Lianamirru Lianamirru changed the title fix mol format copying #9866 - Additional elements appear when inserting a file saved in mol format the expand monomers package Jun 1, 2026
new Pile(selection.images),
new Pile(selection[MULTITAIL_ARROW_KEY]),
bondIdMap,
true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bare true as the 11th positional arg to Struct.clone(...) is opaque — a reader has to jump to struct.ts:192 to learn it means "clone attachment points." The sibling change in this same PR (ketSerializer.ts:967) introduces const needCloneAttachmentPoints = true; precisely to self-document the call. Mirroring that pattern here would keep the two new call sites consistent:

Suggested change
true,
bondIdMap,
true /* needCloneAttachmentPoints */,

Comment on lines +275 to +281
const isAbsFlag =
Array.from(this.molecule!.frags.values()).some((fr) =>
fr ? fr.enhancedStereoFlag === StereoFlag.Abs : false,
) ||
Array.from(this.molecule!.atoms.values()).some(
(atom) => atom.stereoLabel === StereoLabel.Abs,
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A one-liner explaining why a per-atom StereoLabel.Abs must also raise the chiral header flag would help — this is exactly the bug being fixed, and without context a future maintainer is likely to "simplify" it back. Something like // Atoms can carry an Abs stereo label even when no fragment is flagged as Abs (e.g., after copy/paste); both paths must raise the chiral flag. above the assignment would suffice.

Minor: Array.from(map.values()).some(...) materialises both maps before short-circuiting. For typical molecules this is negligible, but a small forEach/for…of helper would avoid the throwaway arrays if you're already touching this block.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 1, 2026

Targeted bug-fix PR — the two correctness changes (cloning attachment points so leaving-group hydrogens get dropped, and raising the chiral header flag for per-atom Abs labels) are sound and the fixture updates are consistent with the behaviour change. No security concerns; perf impact of the new Array.from(...).some(...) and the propagated needCloneAttachmentPoints flag is negligible (monomer-count bounded, not atom-count bounded).

Two small things noted inline. One broader follow-up worth flagging separately:

Struct.clone ergonomics (struct.ts:192-204). The signature now takes 11 optional positional params, and this PR adds a new call site that passes ten undefineds to reach the 11th (ketSerializer.ts:968-980). It also has no JSDoc despite needCloneAttachmentPoints materially changing production output. Converting to a single options object (clone({ needCloneAttachmentPoints: true })) — and documenting the flag — would eliminate the undefined-padding, the magic-boolean problem in Editor.ts:3047, and the docs gap in one pass. Out of scope here, but worth a follow-up issue.

@Lianamirru Lianamirru force-pushed the 9866-mol-format branch 2 times, most recently from 9b698cd to 8bb9181 Compare June 2, 2026 11:10
@Lianamirru Lianamirru closed this Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Additional elements appear when inserting a file saved in mol format the expand monomers package.

1 participant