#8176-An IDT alias for one of the positions (5', internal, 3') must be unique in the whole library (another case) #10049
#8176-An IDT alias for one of the positions (5', internal, 3') must be unique in the whole library (another case) #10049guliaisaeva wants to merge 5 commits into
Conversation
|
@copilot resolve the merge conflicts in this pull request |
Resolved in commit 6d06801 by merging |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (!aliasCollisionExists) { | ||
| aliasCollisionExists = getExistingMonomerGroupTemplates().some( | ||
| (template) => { | ||
| idtAliasCollision = findIdtAliasCollision( | ||
| newMonomer.props?.idtAliases, | ||
| template.definition.idtAliases, | ||
| ); | ||
|
|
||
| return Boolean(idtAliasCollision); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
getExistingMonomerGroupTemplates() rebuilds the mapped/filtered list of group templates on every iteration of the newMonomersLibraryChunk.forEach. The same call also appears at line 701 inside the preset loop. With N incoming items and T existing templates this is O(N×T) work that recomputes identical data.
Suggest hoisting the call out of the loops (compute once before the monomer forEach, and once before the preset forEach, or maintain an index that's incrementally updated as items are added).
Note also: this check looks up existing group templates, but the monomer loop runs before the preset loop — so a monomer in the same chunk will not collide against a preset added later in the same updateMonomersLibrary call. If that's intentional (presets always "win"), worth a comment; otherwise the order matters.
| const formatIdtAliasDetails = (idtAliases?: IKetIdtAliases) => | ||
| IDT_ALIAS_POSITIONS.map((position) => { | ||
| const alias = getIdtAliasesByPosition(idtAliases)[position]; | ||
|
|
||
| return alias | ||
| ? `IDT ${IDT_ALIAS_POSITION_LABEL[position]} alias "${alias}"` | ||
| : null; | ||
| }) | ||
| .filter((value): value is string => Boolean(value)) | ||
| .join(', '); |
There was a problem hiding this comment.
getIdtAliasesByPosition(idtAliases) is called once per position inside the .map, so the same object is recomputed four times per call. Lift it out:
| const formatIdtAliasDetails = (idtAliases?: IKetIdtAliases) => | |
| IDT_ALIAS_POSITIONS.map((position) => { | |
| const alias = getIdtAliasesByPosition(idtAliases)[position]; | |
| return alias | |
| ? `IDT ${IDT_ALIAS_POSITION_LABEL[position]} alias "${alias}"` | |
| : null; | |
| }) | |
| .filter((value): value is string => Boolean(value)) | |
| .join(', '); | |
| const formatIdtAliasDetails = (idtAliases?: IKetIdtAliases) => { | |
| const aliasesByPosition = getIdtAliasesByPosition(idtAliases); | |
| return IDT_ALIAS_POSITIONS.map((position) => { | |
| const alias = aliasesByPosition[position]; | |
| return alias | |
| ? `IDT ${IDT_ALIAS_POSITION_LABEL[position]} alias "${alias}"` | |
| : null; | |
| }) | |
| .filter((value): value is string => Boolean(value)) | |
| .join(', '); | |
| }; |
This helper is called per failed monomer/preset and inside the collision-error path, so the redundant recomputes compound.
| collision?: ReturnType<typeof findIdtAliasCollision>, | ||
| ) => | ||
| collision | ||
| ? ` Bad position IDT alias "${collision.value}" for ${ |
There was a problem hiding this comment.
"Bad position IDT alias" reads awkwardly — "position" modifying "alias" suggests a format issue rather than a uniqueness violation, which is confusing since this message is only appended when a collision was already detected and reported in the prefix.
Consider rephrasing for clarity, e.g.:
| ? ` Bad position IDT alias "${collision.value}" for ${ | |
| ? ` Conflicting IDT ${IDT_ALIAS_POSITION_LABEL[collision.position]} alias "${collision.value}".` |
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should validate shorthand IDT position aliases from converted SDF data', () => { |
There was a problem hiding this comment.
Test name says "shorthand IDT position aliases" (plural) "from converted SDF data", but the fixtures only exercise ep3. The PR adds shorthand support for ep3, ep5, and i (see getIdtAliasesByPosition mapping endpoint3/endpoint5/internal to their shorthand forms), so the other two are untested.
Either rename to clarify the scope (e.g. "...for ep3 shorthand alias") or extend the fixtures to cover ep5 and i as well — the latter is preferable since the shorthand mapping is the value being added here.
| let idtAliasCollision = this._monomersLibrary | ||
| .map((monomer) => | ||
| findIdtAliasCollision( | ||
| templateDefinition.idtAliases, | ||
| monomer.props?.idtAliases, | ||
| ), | ||
| ) | ||
| .find(Boolean); | ||
|
|
||
| if (!idtAliasCollision) { | ||
| idtAliasCollision = getExistingMonomerGroupTemplates() | ||
| .filter((template) => template.ref !== templateRef.$ref) | ||
| .map((template) => | ||
| findIdtAliasCollision( | ||
| templateDefinition.idtAliases, | ||
| template.definition.idtAliases, | ||
| ), | ||
| ) | ||
| .find(Boolean); | ||
| } |
There was a problem hiding this comment.
This block (and the analogous block at lines 514-546 for monomers) duplicates the two-stage collision search: against existing monomers, then against existing group templates. The two flows have diverged subtly already — e.g. the monomer loop uses .some() with mutation, while this one uses .map().find(Boolean) building an intermediate array of length M.
Consider extracting a single helper that takes idtAliases and (optionally) a ref to exclude from the template scan, returning the collision (or undefined). That removes the duplication, unifies the error-message structure, and lets you replace .map(...).find(Boolean) with a short-circuiting for...of to avoid the intermediate allocation.
…6-unique-idt-alias-validation # Conflicts: # packages/ketcher-core/__tests__/application/editor/Editor.test.ts # packages/ketcher-core/src/application/editor/Editor.ts
How the feature works? / How did you fix the issue?
(Screenshots, videos, or GIFs, if applicable)
monomerGroupTemplate).ep3,ep5,i), preset validation, and monomer/preset collisions.