Skip to content

fix(content-types): reset field editor form state when creating a new field#35435

Merged
oidacra merged 3 commits intomainfrom
oidacra/35434-fix-contentype-creator
Apr 23, 2026
Merged

fix(content-types): reset field editor form state when creating a new field#35435
oidacra merged 3 commits intomainfrom
oidacra/35434-fix-contentype-creator

Conversation

@oidacra
Copy link
Copy Markdown
Member

@oidacra oidacra commented Apr 23, 2026

Summary

Fixes a bug in the content type editor where the field properties dialog leaked state (most visibly the Value textarea) from a previously saved field into a newly opened field dialog. Users would see the prior field's values prepopulated, the textarea would behave as uneditable, and Save would stay disabled until toggling an unrelated checkbox.

Closes #35434

Root cause

Two independent defects compounded to produce the visible behavior:

  1. DotTextareaContentComponent.writeValue had a truthy guard (if (value)) that skipped empty/null/undefined values. When Reactive Forms called writeValue('') to reset the control, the Monaco editor's internal model kept the stale content from the previous field.

  2. DynamicFieldPropertyDirective decided whether to recreate the dynamic inner component by comparing previousFieldId !== currentFieldId. Since every new field has id === null, switching between two new fields (e.g. Radio → Select) resulted in null !== null === false → the ValuesPropertyComponent (and its child Monaco editor) was reused rather than recreated, preserving its stale DOM.

Changes

  • dot-textarea-content.component.tswriteValue now writes the value directly (using ?? ''), so empty/null/undefined correctly reset the control.
  • dynamic-field-property.directive.ts — compare the previous vs. current field reference instead of ids, so the inner component is recreated whenever the field instance changes.
  • dot-textarea-content.component.spec.ts — new writeValue tests asserting the control resets for '', null, and undefined.

Acceptance criteria

  • Creating a new field after saving any field type with a Value textarea opens a dialog with an empty Value textarea.
  • Default Value, Hint, Name, and all boolean flags are reset for the new field.
  • The Value textarea in the new-field dialog is immediately editable.
  • The Save button follows its normal enable/disable rules (no sticky-disabled state from stale form state).
  • Editing an existing field still loads that field's stored values (no regression in edit mode).
  • Cancelling a new-field dialog and re-opening it does not resurrect stale values.
  • Unit test added asserting writeValue correctly resets for empty/null/undefined.

Test plan

  • Edit a content type.
  • Add a Radio field with values like value|1 / value.2|2, save.
  • Add a new Select field — the Value textarea must be empty and editable; Save must enable as soon as required fields are valid.
  • Repeat for Radio → Radio, Select → Select, Select → Multi Select, Checkbox → Radio.
  • Edit an existing saved field and confirm its stored values still load correctly.
  • Open a new field dialog, type some values, Cancel, reopen — the form must be empty.

… field

The field properties dialog leaked values (notably the Value textarea)
from a previously saved field into the next new field.

Two fixes:

- DotTextareaContentComponent.writeValue skipped empty/null/undefined
  via a truthy guard, so the Monaco editor kept stale content when the
  form control was reset.

- DynamicFieldPropertyDirective compared previous/current field by id,
  but new fields share a null id, so the inner property component was
  reused across different field dialogs. Compare by field reference
  instead so the component is recreated when the field changes.

Closes #35434
@oidacra oidacra requested a review from nicobytes April 23, 2026 16:13
@github-actions github-actions Bot added the Area : Frontend PR changes Angular/TypeScript frontend code label Apr 23, 2026
@oidacra oidacra requested a review from hmoreras April 23, 2026 16:13
@oidacra oidacra assigned oidacra and unassigned adrianjm-dotCMS Apr 23, 2026
@oidacra oidacra marked this pull request as ready for review April 23, 2026 16:13
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 23, 2026

Claude finished @oidacra's task in 2m 32s —— View job


PR Review

  • Read changed files
  • Analyze code for issues
  • Post review

The fix is correct and the root cause analysis is accurate. Two issues worth flagging:

1. writeValue type signature is a lie

dot-textarea-content.component.ts:170

writeValue(value: string): void {
    this.value = value ?? '';
}

The implementation now handles null/undefined, but the parameter is typed string. The tests confirm the mismatch — they have to cast: component.writeValue(null as unknown as string). The ControlValueAccessor contract accepts any, so the fix works, but the declared type misleads callers. Should be string | null | undefined.

2. The isEqual outer guard can still swallow a recreation that shouldRecreate would have caught

dynamic-field-property.directive.ts:40-47

The outer ngOnChanges gate short-circuits on !isEqual(previousValue, currentValue). If two distinct new-field objects happen to be deeply equal (e.g. Radio → Radio where both share the same defaults and no variable diff yet), isEqual returns true, the whole body is skipped, and shouldRecreate is never reached. The reference-based fix in shouldRecreate only acts when the outer gate already opens.

In practice this edge case is rare because field switches produce a different clazz, but it's a pre-existing landmine this PR didn't close. Worth a follow-up or a comment in the code noting the dependency.

Everything else is clean.

…sed recreation

Add three unit tests for the new shouldRecreate logic:
- Two new fields with id=null but different references recreate the inner component.
- The same field reference passed again does not recreate.
- Switching from a saved field to a new null-id field recreates.

Addresses review feedback on #35435.
@oidacra
Copy link
Copy Markdown
Member Author

oidacra commented Apr 23, 2026

Added three unit tests in 13adbe9 covering the new reference-based shouldRecreate logic in DynamicFieldPropertyDirective:

  1. Two new fields with id=null but different references → recreates the inner component.
  2. The same field reference passed again → does not recreate.
  3. Switching from a saved field to a new null-id field → recreates.

All 2109 tests pass.

@oidacra oidacra added this pull request to the merge queue Apr 23, 2026
Merged via the queue into main with commit 010009d Apr 23, 2026
28 checks passed
@oidacra oidacra deleted the oidacra/35434-fix-contentype-creator branch April 23, 2026 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Field editor retains previous field's values when creating a new field

3 participants