Skip to content

fix(ui): remove blocksHolder DOM listeners and detach holder on destroy#173

Closed
LocalhostTI wants to merge 1 commit into
editor-js:mainfrom
LocalhostTI:fix/ui-destroy-listeners
Closed

fix(ui): remove blocksHolder DOM listeners and detach holder on destroy#173
LocalhostTI wants to merge 1 commit into
editor-js:mainfrom
LocalhostTI:fix/ui-destroy-listeners

Conversation

@LocalhostTI

@LocalhostTI LocalhostTI commented Jul 20, 2026

Copy link
Copy Markdown

Fixes #170

What

  • BlocksUI now registers its blocksHolder DOM listeners (beforeinput, keydown) with an AbortController signal and aborts them in destroy(); the holder element itself is detached from the DOM as well.
  • EditorjsUI.destroy() (previously a no-op) now removes the editor wrapper from the holder element.
  • Adds the first test suite to @editorjs/ui (Blocks.spec.ts, index.spec.ts, 14 tests) with Jest + jsdom infra mirroring the core package setup. Written test-first (TDD): the destroy specs failed against the previous implementation.
  • Adds a UI check CI workflow (ui.yml, unit tests without mutations, modeled on model-types.yml).

CI enablers (why this PR touches files outside packages/ui)

  • .github/actions/base-coverage/action.yml: the guard now also skips base coverage when the base ref has no test:coverage script. Without this, the base-coverage step would run yarn workspace @editorjs/ui test:coverage against main (where the script doesn't exist yet) and fail. It flows through the same "missing base artifact" path that new packages already use (aggregate-report downloads artifacts with continue-on-error).
  • packages/ui build script: build now runs build:deps (topological workspace-deps build) before vite build. Every other package builds via tsc --build whose project references transitively build workspace deps into their dist/; ui is the first vite-built package through the unit-tests action, and vite externalizes @editorjs/*, so on a fresh runner packages/sdk/dist would never exist and jest could not resolve @editorjs/sdk (reproduced locally by wiping dep dists). This makes ui's build behave like the tsc --build packages — matching the build action's "Build the package with dependencies" step.
  • packages/ui/vite.config.ts: dts() now uses tsconfig.build.json (which excludes src/**/*.spec.ts); otherwise the new co-located specs leak empty *.spec.d.ts stubs into the published dist/.

Notes for reviewers

  • The issue lists a third copy listener — that one is introduced by feat(core): implement Clipboard Plugin #129, which is not merged yet, so this PR covers the two listeners that exist on main. Once feat(core): implement Clipboard Plugin #129 lands, its copy listener can simply join the same AbortSignal.
  • EventBus (non-DOM) subscriptions made in the BlocksUI/EditorjsUI constructors are intentionally untouched: the bus lives and dies with the editor instance. Happy to tackle that as a follow-up if you'd like.
  • OpenSpec: openspec/specs/ui/spec.md gets destroy scenarios and a refreshed test-suite note (openspec validate ui passes). No openspec/changes/ proposal folder — small behavior bug fix with the spec delta applied directly, same approach as feat(core): implement Clipboard Plugin #129. Can convert to a full proposal if preferred.
  • Expected CI caveat: this PR comes from a fork, so GITHUB_TOKEN is read-only and the Aggregate and post report job (which posts the coverage comment) will fail with 403. That's the repo-wide package-check.yml pattern, not something this PR changes — lint/tests/build jobs are unaffected. This will be the first fork PR to exercise the report job since it was introduced.

Verification

  • packages/ui: yarn lint:ci (0 warnings), yarn test (14/14), yarn build — all green locally
  • Fresh-runner simulation: after yarn workspaces foreach -A run clear (no dist/, no tsbuildinfo anywhere), the CI sequence yarn workspace @editorjs/ui run build && yarn workspace @editorjs/ui test:coverage passes
  • yarn constraints and root yarn build pass; npx openspec validate ui passes

- register BlocksUI's beforeinput/keydown listeners with an
  AbortController signal and abort them in destroy(); detach the
  blocks holder element as well
- implement EditorjsUI.destroy() (was a no-op) to remove the editor
  wrapper from the holder
- add the first test suite for @editorjs/ui (Jest + jsdom), written
  test-first per the TDD rule
- add UI check CI workflow; skip base-coverage when the base ref has
  no test:coverage script; build workspace deps before vite build so
  jest can resolve them on a fresh runner
- exclude *.spec.ts from published type declarations (vite-plugin-dts
  now uses tsconfig.build.json)
- sync openspec/specs/ui/spec.md destroy scenarios

Fixes #170

Copilot AI left a comment

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.

Pull request overview

This PR fixes UI teardown leaks in @editorjs/ui by ensuring the blocks holder’s DOM listeners are removed and the holder/wrapper elements are detached during destroy(). It also introduces a Jest + jsdom test setup for the UI package and adds CI coverage for the package.

Changes:

  • Add AbortController-scoped beforeinput/keydown listeners to BlocksUI and abort + detach the blocks holder in BlocksUI.destroy().
  • Implement real teardown in EditorjsUI.destroy() by removing the editor wrapper from the holder.
  • Introduce @editorjs/ui unit tests + Jest infrastructure, and add a dedicated UI check workflow; update build + typings config to support co-located specs.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
yarn.lock Adds UI testing/build tool dependencies (Jest, ts-jest, jsdom, babel-jest).
packages/ui/vite.config.ts Points vite-plugin-dts at tsconfig.build.json to avoid emitting spec d.ts stubs.
packages/ui/test/mocks/styleMock.cjs Adds CSS-module identity proxy for Jest runs.
packages/ui/src/index.ts Makes EditorjsUI.destroy() remove the editor wrapper element.
packages/ui/src/index.spec.ts Adds unit coverage for EditorjsUI wrapper attachment/removal.
packages/ui/src/Blocks/Blocks.ts Aborts holder DOM listeners on destroy and detaches the holder from the DOM.
packages/ui/src/Blocks/Blocks.spec.ts Adds unit coverage for block rendering, input remapping, undo/redo shortcuts, and destroy cleanup.
packages/ui/package.json Adds Jest scripts + deps; ensures build runs workspace dependency builds first.
packages/ui/jest.config.ts Introduces Jest+ts-jest+jsdom config (ESM-friendly) for UI package tests.
packages/ui/eslint.config.mjs Allows @jest/globals imports in spec files under published-import rules.
openspec/specs/ui/spec.md Updates UI spec to reflect destroy behavior and new test coverage.
.github/workflows/ui.yml Adds UI package CI workflow using the shared package-check pipeline.
.github/actions/base-coverage/action.yml Skips base coverage when the base ref lacks a test:coverage script (new test suites).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ui.yml
Comment on lines +19 to +20
secrets:
stryker_dashboard_api_key: ${{ secrets.STRYKER_DASHBOARD_API_KEY }}
@LocalhostTI LocalhostTI closed this by deleting the head repository Jul 21, 2026
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.

BlocksUI doesn't remove its blocksHolder DOM event listeners (or the holder itself) on destroy()

2 participants