Fix three production defects surfaced by the strict-mode rollout (iframe listener leak, dead drop guard, template payload)
Nobody has claimed this yet.
- 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:12—for (let i = 0; i < args.length - 1; i++)never substitutes the last argument; with a single argument it substitutes nothing. Flagged with aNOTEin place. Check the call sites before changing it: some may have been written around the off-by-one.getPersonasURLhas aperper_pagetypo 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-uino 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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