Skip to content

fix(edit-content): preserve category values across lock and save#35388

Merged
oidacra merged 3 commits intomainfrom
issue-33931-categories-not-saved-or-displayed-when-using-new-edit-contentlet-mode
Apr 21, 2026
Merged

fix(edit-content): preserve category values across lock and save#35388
oidacra merged 3 commits intomainfrom
issue-33931-categories-not-saved-or-displayed-when-using-new-edit-contentlet-mode

Conversation

@oidacra
Copy link
Copy Markdown
Member

@oidacra oidacra commented Apr 20, 2026

Summary

Fixes two races in the new edit-content mode that could silently blank category fields when a contentlet has more than one Category field.

Closes #33931

Root causes

  1. Lock/unlock replaced the whole contentlet. withLock() patched the store with the full server response, which changed the contentlet reference and forced DotEditContentFormComponent to rebuild the form. During the rebuild, the async category hierarchy fetch would briefly hold selected = [] and the effect would write that empty value into the form control. Saving during that window blanked the field.

  2. Category sync effect fired before the hierarchy fetch completed. In dot-category-field.component.ts, the effect that mirrors store.selected() → form control ran immediately on mount with the initial empty state, overwriting the form control's resolved inodes. Intermittent because the race depends on how quickly the user interacts after mount (or after a form rebuild).

Changes

  • store/features/lock/lock.feature.ts — on lock/unlock, patch only locked, lockedBy, lockedByName, lockedOn, preserving the rest of the contentlet.
  • components/dot-edit-content-form/dot-edit-content-form.component.ts — track a revision key (identifier|inode|modDate) and only rebuild the form when the contentlet's identity actually changes, not on every state-only update.
  • fields/dot-edit-content-category-field/components/dot-category-field/dot-category-field.component.ts — the sync effect now waits until ComponentStatus.LOADED before emitting to the form control, so the initial empty selected no longer wipes the value.
  • Spec updated to use a different inode for the historical-version mock, matching real backend responses.

Test plan

  • Open a contentlet with two Category fields, both populated.
  • Lock → add one more item to the first category → save.
  • Confirm the second category still holds its original values on reload.
  • Repeat several times (was intermittent before the fix).
  • Historical version view still reinitializes the form when switching versions.
  • Existing edit-content unit tests pass (yarn nx test edit-content).

This PR fixes: #33931

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 20, 2026

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


Code Review

  • Read changed files
  • Analyze lock feature change
  • Analyze form component change
  • Analyze category field component change
  • Analyze spec changes
  • Post review

The fix is well-reasoned. Two genuine issues and a few smaller notes follow.


lock.feature.ts — surgical patch instead of full replacement

The change is correct. Spreading current and overlaying only the four lock fields prevents downstream signal comparisons from seeing identity changes on fields that didn't move.

One observation: the onInit effect still calls checkCanLock() whenever store.contentlet() changes. Because patchState with a spread always creates a new object reference, checkCanLock() fires on every lock/unlock. That's one extra API round-trip per toggle. Pre-existing behaviour, no regression, but it's worth noting if you ever want to tighten it.


dot-edit-content-form.component.ts — revision key

modDate is redundant and potentially fragile (line 256):

const revisionKey = `${contentlet.identifier}|${contentlet.inode}|${contentlet.modDate}`;

inode already changes on every save. modDate is a tag-along that does nothing in the normal flow but can cause a spurious rebuild if the same version is re-fetched and the backend serialises the date differently (e.g. 2024-01-01T00:00:00.000Z vs 2024-01-01T00:00:00Z). Using identifier|inode is sufficient and unambiguous.

Fix this →


dot-category-field.component.ts — ERROR state creates a silent UI/data split

The comment at line 112–115 intentionally skips ERROR:

"we'd rather keep whatever the form control holds from writeValue (stale but truthful) than overwrite it with an empty array"

The tradeoff is: if the hierarchy fetch fails, the form control retains the inodes so they're submitted, but the chip display is empty because store.selected() is []. The user sees no categories selected yet the save will include them invisibly. The old "blank the field" behaviour was at least UI-consistent.

This is probably acceptable in practice (fetch errors should be rare), but the comment should acknowledge the UI→data divergence explicitly so future readers don't "fix" it the wrong way.


Spec — unlock path not tested

The new test should not reinitialize the form when only lock state changes (line 684) only exercises { checked: true } (lock). The unlockContent path is symmetric and equally important — the dotContentletService.unlockContent mock is already configured in that beforeEach, so adding an it('should not reinitialize the form when only unlock state changes', ...) variant with { checked: false } would be a one-liner.

Spec — LOADED guard has no test

Race condition #2 (effect fires before hierarchy fetch completes) is described in the PR but there's no new spec for dot-category-field.component.ts verifying that the effect skips when state !== LOADED. The fix itself looks correct; coverage for it just isn't there.


Overall: the lock-patch and form-key approaches are sound. The modDate issue is the only thing I'd want changed before merge; the rest are follow-ups or documentation nits.

oidacra added 2 commits April 20, 2026 16:34
Two races in the new edit-content mode could blank category fields when
a contentlet has more than one category field:

- The lock/unlock action replaced the whole contentlet in the store with
  the server response, which triggered a full form rebuild. During the
  rebuild, async category hierarchy fetches would reset form controls.
  Patch only the lock-related properties so unrelated field state is
  kept.

- The category field's sync effect emitted the store's initial empty
  selected into the form control before the async hierarchy fetch
  completed. If the user saved during that window, the field was sent
  empty. Wait for ComponentStatus.LOADED before propagating values.

Also guard the form-rebuild effect so it only reinitializes when the
contentlet identity (identifier|inode|modDate) actually changes, not on
every state-only update.

Closes #33931
- Guard lock/unlock handlers against a null contentlet (edge case when
  the user navigates away while the request is in-flight).
- Keep the form revision key in sync when isCopyingLocale rebuilds the
  form, so the main reinit effect doesn't rebuild a second time.
- Document why the category field's LOADED guard also skips the ERROR
  state (better stale than blank).
- Use a complete DotCMSContentlet (with lock fields) in the lock/unlock
  mocks so post-lock assertions would be meaningful.
- Add a regression test asserting the form is not reinitialized when
  only the lock state changes.
@oidacra oidacra force-pushed the issue-33931-categories-not-saved-or-displayed-when-using-new-edit-contentlet-mode branch from 1934cd0 to 137b634 Compare April 20, 2026 20:39
@oidacra oidacra marked this pull request as ready for review April 21, 2026 15:48
@oidacra oidacra enabled auto-merge April 21, 2026 15:55
@oidacra oidacra added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit f442394 Apr 21, 2026
28 checks passed
@oidacra oidacra deleted the issue-33931-categories-not-saved-or-displayed-when-using-new-edit-contentlet-mode branch April 21, 2026 17:38
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.

[DEFECT] Categories not saved or displayed when using New Edit Contentlet Mode

2 participants