UoE: fix duplicate + cleared values in repeatable submission dropdowns

[milanmajchrak]

Follow-up to #26 and #32. Both reported problems still reproduce on datashare-UoEMainLibrary-dspace-8_x, because neither is caused by the dropdown widget alone.

Reproduced end to end on a dockerised DSpace 8 backend + this branch's UI with the datashare theme, then fixed test-first.

Before

BUG B — "I opened the options, clicked next to it, and all my selected values were cleared"

Three Types selected → open a row's options → click just below the field → the value is gone. Repeat per row and the whole field empties; the dc.type field is then deleted from the item server-side.

BUG A — the same Type can be added twice after deleting a row

Article / Book / Dataset → delete the middle row → add a row → Dataset is not greyed out and can be picked again. Deleting the row also throws TypeError: Cannot read properties of null (reading 'get') and NG01052 in the console.

It is worse than a duplicate. In one run the form showed [Article, Dataset, Image] while the server stored:

[{"op":"add","path":"/sections/traditionalpageone/dc.type","value":[
  {"value":"Article","place":0},{"value":"Image","place":1},{"value":"Image","place":2}]}]

Dataset — which the user never touched — was destroyed, and Image duplicated. A page reload confirms it.

After

Root causes

1. Deleting a row silently rebound the surviving rows to the wrong controls

getControlOfGroup() stamped a startingIndex on a group model the first time the row rendered, then resolved that row's FormGroup as control.get([startingIndex]) forever. Nothing re-synced it — while DynamicFormArrayModel re-indexes its groups on every insert/remove/move, and the template binds formGroupName to that live index. The two disagreed after deleting a non-last row: surviving rows resolved to another row's control or to null, and after delete+add two rows could alias onto the same control.

That is what let an untouched value be overwritten, and what made the duplicate check read stale sibling values. startingIndex is upstream DSpace code (it arrived with the 8.2 merge and exists in DSpace/dspace-angular), so this is worth reporting upstream too.

The live index is now the single authority. Drag and keyboard reordering moved the group models only — which is what the frozen index had been compensating for — so both now move the control alongside the model.

DsDynamicFormControlContainerComponent.ngOnChanges also dereferenced a null group in the tick after a row was removed, throwing inside change detection and aborting the pass for the rest of the field.

2. A click aimed at dismissing the menu erased the value

Measured live: the input sits at y=581 h=38 (bottom edge 619), the open menu at y=613 h=218 — it starts above the field's own bottom edge and covers the rows beneath it, and its first entry was the destructive "Clear selection". So the spot a user naturally clicks to dismiss the dropdown wiped their selection, row after row. Playwright confirms the overlay intercepts: <button title="Animation"> … subtree intercepts pointer events.

Clearing now sits at the end of the menu, visually separated, and the menu keeps a small gap below the input. The clear entry also carried both (click) and (mousedown) and so fired twice per interaction — it now has a single handler, like the options.

Options deliberately keep committing on (mousedown): blurring the input flips showErrorMessages on a required field and forceShowErrorDetection() destroys and re-creates the control, so the element is gone before a click could reach it. Switching them to (click) made selection impossible — that is now documented in the template so it is not "fixed" again.

3. Duplicate detection was stale, and bypassed entirely by the caret

usedSiblingValues was a snapshot refreshed only in openDropdown()/selectEntry(), so it went stale whenever a row was added, removed or edited — both hiding options that had become free and offering options in use. Worse, the toggle caret ▾ opens the menu through NgbDropdown without calling openDropdown() at all, leaving the set empty so nothing was greyed out and clicking an in-use option was a silent dead click.

It is now derived from the live models, and every path that opens the menu refreshes the options via (openChange).

4. #32's authority key made duplicates incomparable

canonicalKey() keyed on authority ?? value. But the same entry arrives as a VocabularyEntry when picked in-session and as a FormFieldMetadataValueObject when rebuilt from stored metadata, and only one side may carry an authority — so after a reload the keys no longer matched and the duplicate went through. (#26 keyed both sides on .value and did not have this; it was a regression.)

Identity is now the authority and the normalised value, and a match on either counts as a duplicate.

Testing

Test-first: the 14 new specs were written against the unfixed code and confirmed failing, including the exact live symptoms —

✖ rebinds the surviving rows after the MIDDLE row is removed
    TypeError: Cannot read properties of null (reading 'get')
      at DsDynamicFormControlContainerComponent.ngOnChanges
✖ never resolves two rows to the same control after remove + insert
    each row must own a distinct FormGroup: Expected 2 to be 3.
✖ keeps the surviving row's value intact after remove + insert
    Dataset must survive the delete+add: Expected null to be 'Dataset'.
✖ disables a sibling value WITHOUT the dropdown having been opened first
✖ stops disabling a value once the row that used it is removed
✖ detects a duplicate when the sibling lost its authority in a server round-trip

• dynamic-form-array.component.row-binding.spec.ts — row→control binding across remove / insert / reorder, driving the real DynamicFormService over a real FormArray.
• dynamic-scrollable-dropdown.duplicate.spec.ts — live sibling lookup, the caret path, the authority/value identity matrix, and the clearing behaviour.

Full suite: 5478 passing, 0 failing. Lint clean (0 errors).

Live verification against the running datashare instance — 12/12:

PASS  no value is cleared by clicking below the field
PASS  server still holds all three Types
PASS  the strip below the input is no longer the clear action
PASS  surviving value is not lost on click-away
PASS  server matches what the form shows
PASS  every still-used value is greyed out in the new row
PASS  caret-opened menu greys out used values too
PASS  what the server stores matches what the form showed
PASS  no duplicate on the server
PASS  no value silently destroyed
PASS  reload shows the same values
PASS  no JS exceptions during any of the above

Note on #26's description

#26 states it kept an "existing onChange guard in SubmissionSectionFormComponent" as a backstop that skips the JSON-patch and reverts the visible value. That guard does not exist on this branch — onChange dispatches operations unconditionally, and neither PR touched that file. Client-side graying was therefore the only defence, which is why every gap above was directly user-visible.

This PR does not add a patch-layer guard either; it fixes the causes. A server-side or handleArrayGroupPatch-level rejection would still be worth having as defence in depth, and is left as a follow-up.

Not addressed here

Found while investigating, out of scope, all pre-existing upstream:

• arrow-key navigation does not skip disabled options; Enter on one is a silent no-op
• ArrowDown before the options load throws on optionsList.length
• scrollToSelected() indexes the clear entry too, so keyboard scrolling is off by one
• acceptableKeys holds KeyboardEvent.code names but is compared against event.key, so spaces and punctuation never reach the type-ahead filter

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes two end-user issues in repeatable submission dropdowns (duplicate selections and accidental clearing) by correcting the underlying row↔FormArray control binding, ensuring dedupe state is always derived from live models, and making the destructive “Clear selection” action harder to trigger accidentally.

Changes:

• Keep repeatable row group models and their FormArray controls in sync on reorder, and resolve controls via the group’s live index.
• Make duplicate detection live (no stale snapshots) and authority/value resilient across server round-trips; ensure all open paths refresh options.
• Move “Clear selection” to the end of the menu, separate it visually, and add regression specs covering the previously failing scenarios.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.repro.spec.ts	Updates repro expectations to assert disabled-state via the public API (isOptionDisabled).
src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.duplicate.spec.ts	Adds focused regressions for duplicate prevention + clearing behavior (caret-open path, stale state, authority/value identity).
src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts	Refactors sibling-used detection to be live, adds identity-key normalization, and centralizes open initialization via openChange.
src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.spec.ts	Aligns unit tests with the new open/commit behavior and clear-entry ordering.
src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.scss	Adds a small menu gap and visually separates the clear action from options.
src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html	Wires (openChange), moves clear action to bottom, and removes double mouse handlers.
src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts	Introduces moveGroupAndControl and resolves row controls by live index to prevent misbinding after mutations.
src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts	Adds regression coverage for row→control binding across remove/insert scenarios.
src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts	Guards against transient null group during repeatable re-render to avoid aborting change detection.

---

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