dotCMS / dotCMS/core

Fix three production defects surfaced by the strict-mode rollout (iframe listener leak, dead drop guard, template payload)

Open
#37,123 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

dotCMS : Technical Debt Team : Falcon Type : Task
Dominant language
Java
Stars
970
Forks
486
Avg merge
3d 33m
Merged PRs (30d)
170

Description

Task

Fix three production defects found while enabling strict mode (#35932). All three are flagged in place with a comment and left behaving exactly as before — the strict-mode PRs preserved behaviour on purpose, so that a type rollout never became a silent behaviour change. Each needs its own fix and its own test.

They are unrelated to each other; split into separate PRs if that reads better.


1. Every iframe load leaks a listener pair

apps/dotcms-ui/src/app/view/components/_common/iframe/iframe-component/iframe.component.ts

The component registers keydown/load handlers with .bind(this) and later calls removeEventListener with .bind(this) again. bind returns a new function object each time, so the reference passed to removeEventListener never matches the one that was added. Neither listener is ever removed; every iframe load adds another pair.

Fix: hold the bound handlers in fields (or use arrow properties) and pass the same reference to both calls.

Test: count listeners, or assert the same function identity reaches addEventListener and removeEventListener, across two loads.


2. A dead branch in the field drag-and-drop service

apps/dotcms-ui/src/app/portlets/shared/dot-content-types-edit/components/fields/service/field-drag-drop.service.ts:77

const wasDrop = (target?: Element) => target === null;

The parameter is optional, so a drop with no target arrives as undefined, not null — and undefined === null is false. wasDrop therefore always returns false, and the clearCurrentFullRowEl branch it guards has never run.

Fix: decide what the intended condition is (!target almost certainly) and make the guard say it. Confirm what clearCurrentFullRowEl was supposed to do before enabling it — it has been dead long enough that its side effect is unverified.

Test: a drop outside any container should take the branch.


3. cleanTemplateItem sends a design template's containers on every update

apps/dotcms-ui/src/app/portlets/dot-templates/dot-template-create-edit/store/dot-template.store.ts (see the comment around line 469)

The original cleanTemplateItem deleted type and then tested template.type === 'design'. By that point the property was gone, so the test never passed and the branch that strips a design template's containers before an update never ran. Design containers have always been sent to the update endpoint.

The same function also deleted the key off the store's own object rather than a copy — that half was fixed in #36957 (the copy), because it was a mutation bug the type work exposed directly. The ordering bug was left alone, because fixing it changes what the request body contains.

Fix: test type before deleting it, then verify against the update endpoint what it does with the containers a design template sends today. This one is a payload change — worth checking with backend before landing.

Test: assert the request body for a design template and for an advanced one.


Also worth a look, lower value

  • apps/dotcms-ui/src/app/api/util/stringFormat.ts:12for (let i = 0; i < args.length - 1; i++) never substitutes the last argument; with a single argument it substitutes nothing. Flagged with a NOTE in place. Check the call sites before changing it: some may have been written around the off-by-one.
  • getPersonasURL has a perper_page typo in a query parameter. Harmless if the backend ignores unknown params, but it means the page size is not being sent.

Acceptance Criteria

  • Each defect fixed with a test that fails before the fix
  • The explanatory comments left by #36957 updated or removed as each is fixed
  • For #3, the payload change confirmed against the update endpoint
  • nx run-many -t test lint -p dotcms-ui no worse than baseline

Notes

Found during #35932 / #36957. Nothing here is a regression from that work — all three predate it and were surfaced by turning the compiler on.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the three named files: iframe.component.ts, field-drag-drop.service.ts, and dot-template.store.ts, then inspect their existing tests and the comments around each flagged defect. Add a separate failing test for listener identity across two loads, an untargeted drop, and design versus advanced template payloads. Run nx run-many -t test lint -p dotcms-ui and confirm the design-template payload with the update endpoint before landing.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api, frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.