Skip to content

#1449 - The Template Library sections are opening slowly#10316

Open
mb-mbe wants to merge 12 commits into
masterfrom
1449-performence-fix-for-accordions-in-the-dialog
Open

#1449 - The Template Library sections are opening slowly#10316
mb-mbe wants to merge 12 commits into
masterfrom
1449-performence-fix-for-accordions-in-the-dialog

Conversation

@mb-mbe

@mb-mbe mb-mbe commented Jun 25, 2026

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

Comment thread packages/ketcher-react/src/script/ui/dialog/template/TemplateDialog.tsx Outdated
@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Reviewed for quality, perf, docs, and security. Security found nothing. The unmountOnExit addition is the real win here — StructRender rebuilds SVG via direct DOM manipulation per molecule, and previously every collapsed group kept its full table mounted. The SDF serialization useMemo is also justified (SaveButton is always mounted and was re-serializing the whole library on every accordion toggle).

Three issues called out inline:

  1. useCallback(select, [..., props]) is a no-op because props reference changes every render — and the custom memo comparator then latches the first select it sees, creating a stale-closure risk.
  2. The comment justifying the omitted props in MemoizedTemplateTable's comparator overstates safety; the omitted callbacks aren't actually identity-stable upstream.
  3. templates={shouldGroupBeRended ? ... : []} allocates a new [] each render — currently masked by unmountOnExit, but a latent footgun if that ever flips.

Minor nits not worth blocking on: useMemo(() => Object.keys(filteredTemplateLib), ...) is roughly equal in cost to the call itself; keep it only for ref stability or drop it. Worth verifying upstream that store.editor?.render?.options is referentially stable across unrelated editor updates, otherwise the comparator's renderOptions === check fails on every store change.

Comment thread packages/ketcher-react/src/script/ui/dialog/template/TemplateDialog.tsx Outdated
Comment thread packages/ketcher-react/src/script/ui/dialog/template/TemplateDialog.tsx Outdated
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Nice direction — memoizing the SDF serialization and adding unmountOnExit to the Accordion transitions are real wins. A few things to address before this fully delivers on the stated goal:

  • The EMPTY_TEMPLATES constant is declared inside the component body, so it gets a fresh reference each render and defeats the MemoizedTemplateTable comparator for collapsed accordions. (See inline.)
  • onDelete / onAttach come from the mapDispatchToProps factory and get new references whenever ownProps change, which also breaks the memo comparator. (See inline.)
  • The custom memo comparator is effectively the same as default shallow memo() and can be removed.
  • With unmountOnExit: true, the shouldGroupBeRended ? ... : EMPTY_TEMPLATES ternary is dead — the collapsed accordion's children never render.

With those fixes, MemoizedTemplateTable should actually start hitting. It would also be good to attach a quick before/after profile (or note the user-visible improvement) to confirm the perf delta.

Comment thread packages/ketcher-react/src/script/ui/dialog/template/TemplateDialog.tsx Outdated
Comment thread packages/ketcher-react/src/script/ui/dialog/template/TemplateDialog.tsx Outdated
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Nice tightening of the dialog — the useMemo around SDF serialization, the functional update on setExpandedAccordions, and especially unmountOnExit: true on the Accordion are the real wins here. Two notes inline:

  • The custom memo comparator is a maintenance hazard (prop allow-list) and its comment misuses "stale closures" — default memo() would do.
  • EMPTY_TEMPLATES is declared inside the component, so its array identity churns every render; with unmountOnExit the branch that uses it is also unreachable in practice. Removing both simplifies the diff and avoids a latent regression if unmountOnExit ever changes.

Security: no issues — pure client-side render perf change.

@AlexeyGirin AlexeyGirin linked an issue Jun 29, 2026 that may be closed by this pull request
@AlexeyGirin AlexeyGirin changed the title 1449 performence fix for accordions in the dialog #1449 - The Template Library sections are opening slowly Jun 29, 2026
Comment thread packages/ketcher-react/src/script/ui/dialog/template/TemplateDialog.tsx Outdated
@AlexeyGirin

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

…x-for-accordions-in-the-dialog

# Conflicts:
#	packages/ketcher-react/src/script/ui/dialog/template/TemplateTable.tsx

Copilot AI commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Merged origin/master and resolved the conflict in TemplateTable.tsx. The resolution preserves the performance improvements (memoized TemplateItem, progressive rendering, MemoizedStructRender) while incorporating the accessibility improvement from master that uses a semantic <button> element instead of <div role="button">.

Copilot AI requested a review from AlexeyGirin July 2, 2026 18:31
mb-mbe added 3 commits July 7, 2026 19:19
…ble partial rendering and moved svg previews to "lazy render" batch by batch to allow accordion open smoothly
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.

The Template Library sections are opening slowly

3 participants