Skip to content

#10034 System doens't assing added bonds to defined Preset component#10153

Open
oguzhanduran wants to merge 1 commit into
masterfrom
10034-system-does-not-added-bonds-to-defined-preset-component
Open

#10034 System doens't assing added bonds to defined Preset component#10153
oguzhanduran wants to merge 1 commit into
masterfrom
10034-system-does-not-added-bonds-to-defined-preset-component

Conversation

@oguzhanduran
Copy link
Copy Markdown
Collaborator

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

@oguzhanduran oguzhanduran linked an issue Jun 2, 2026 that may be closed by this pull request
@AlexeyGirin AlexeyGirin changed the title 10034:fix system does not added bonds to defined preset component #10034 System doens't assing added bonds to defined Preset component Jun 2, 2026
const rnaComponentToAssign = beginAtomRnaComponent ?? endAtomRnaComponent;
const struct = this.struct();
const atomIdToStartFrom = beginAtomRnaComponent ? bond.begin : bond.end;
const atomIdToStartFrom = beginAtomRnaComponent ? bond.end : bond.begin;
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 fix is correct — the previous code seeded the traversal from the atom already assigned to the component, so it walked through component atoms first and could incorrectly trip hasConnectionToOriginalStructure for connections that have nothing to do with the new bond. Two suggestions while this line is being touched:

  1. Rename atomIdToStartFromunassignedAtomId (or similar). The intent — "start from the unassigned endpoint" — would then be self-documenting, and the ternary's direction obvious by inspection. This is exactly the kind of bug that name-driven clarity prevents from regressing.
  2. Consider adding a focused unit test for this function. The three branches (no component / one component / two components) plus the "unassigned side reaches the original structure" case are highly testable, and a regression test would lock in this silent semantic inversion.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 2, 2026

Reviewed the one-line fix in canAssignRnaComponent. The change is correct: traversal must start from the unassigned endpoint so it explores only the new fragment's external connections. The previous code started from the atom already in the component, which walked through component atoms first and could falsely flag hasConnectionToOriginalStructure for pre-existing connections unrelated to the new bond. As a side benefit, the new behavior is also strictly faster (O(fragment size) instead of O(component size + fragment size)).

Main concern: no test is being added for a silent semantic-inversion bug. Inline suggestions cover a rename to make the intent self-documenting, an outer-loop early-exit, and a small dead-code cleanup.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 2, 2026

A couple of pre-existing nits in the same function, optional but cheap to fix while touching it:

  • Outer loop doesn't early-exit on disqualifying flags (Editor.ts:2112): once hasConnectionToAnotherComponent or hasConnectionToOriginalStructure is set, the forEach short-circuits per-neighbor at lines 2125–2130, but the outer while keeps draining atomsToCheck. Tightening the loop condition to while (atomsToCheck.length > 0 && !hasConnectionToAnotherComponent && !hasConnectionToOriginalStructure) would early-exit the whole traversal in the disqualifying case.
  • Redundant visitedAtomIds.add (Editor.ts:2176): neighborAtomId is already added to the set at line 2145, so the second add at 2176 is dead.

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.

System doens't assing added bonds to defined Preset component

2 participants