UoE: prevent duplicate values in repeatable submission dropdowns (authority-aware + live re-check) - #32
Merged
milanmajchrak merged 4 commits intoJul 24, 2026
Conversation
…dropdowns - key duplicate detection on authority (fallback value) so authority-controlled vocabularies (e.g. Funder) no longer accept the same entry twice - re-check live sibling values at commit time (selectEntry) to close the stale-set gap when a value is chosen in another row after opening - block mousedown on disabled options (dsBtnDisabled); options commit on mousedown - guard keyboard Enter against selecting while options are still loading - expose aria-selected on options - add reproduction/regression spec and a self-contained before/after demo
There was a problem hiding this comment.
Pull request overview
This PR closes remaining edge cases where repeatable, authority-controlled submission dropdowns (e.g., Funder/Type) could still persist duplicate selections, by making duplicate detection authority-aware and re-checking siblings at commit time. It also hardens interaction blocking (mousedown) for disabled options and adds targeted regression coverage plus a standalone demo harness.
Changes:
- Canonicalize duplicate identity as
authority ?? value, and refresh sibling-used values insideselectEntry()to prevent stale-state commits. - Extend
dsBtnDisabledto blockmousedown(not just click/keyboard), matching the dropdown’s commit path. - Add reproduction/regression specs and a Playwright-based demo/recording harness for before/after verification.
Reviewed changes
Copilot reviewed 11 out of 17 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 | Adds reproduction/regression specs covering deletion/reindex, authority identity, stale used-set, blur behavior, and mousedown guard paths. |
| src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts | Implements authority-aware canonical keys and a live sibling-value refresh at commit time; tightens Enter-selection behavior while loading. |
| src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html | Exposes aria-selected on options (with a noted semantic issue in review comments). |
| src/app/shared/btn-disabled.directive.ts | Blocks mousedown for disabled elements to prevent interaction via non-click commit paths. |
| Dockerfile.karma | Adds a Node+Chromium image to reproduce/run headless Karma specs in a consistent environment. |
| demo/screenshot.mjs | Captures before/after stills via Playwright. |
| demo/record.mjs | Records before/after .webm clips via Playwright. |
| demo/gif.mjs | Generates embeddable before/after GIFs without ffmpeg. |
| demo/index.html | Self-contained UI harness mirroring the dropdown’s deduplication logic for visual verification. |
| demo/package.json | Demo-only dependencies/scripts for Playwright recording. |
| demo/.gitignore | Ignores demo node_modules/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The before/after clips stay embedded in the PR description; Dockerfile.karma and the demo/ harness were only used locally to reproduce and record, and do not belong in the codebase.
milanmajchrak
merged commit Jul 24, 2026
c202cd6
into
datashare-UoEMainLibrary-dspace-8_x
7 checks passed
milanmajchrak
added a commit
that referenced
this pull request
Jul 27, 2026
…alues Follow-up to #26 and #32. Both reported problems still reproduced on datashare-UoEMainLibrary-dspace-8_x, because they are not caused by the dropdown widget alone. 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 and resolved that row's FormGroup as control.get([startingIndex]) from then on. Nothing re-synced it, while DynamicFormArrayModel re-indexes its groups on every insert/remove/move and the template binds formGroupName to that live index. After deleting a non-last row the two disagreed: 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 a value the user never touched be overwritten, and what made the duplicate check read stale sibling values. 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 (moveGroupAndControl). 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. A click aimed at dismissing the menu erased the value ----------------------------------------------------- The menu is a full-width overlay drawn over the field's own bottom edge and 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. 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 therefore 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 event could reach it. This is now documented in the template. Duplicate detection ------------------- usedSiblingValues was a snapshot refreshed only in openDropdown()/selectEntry(), so it went stale whenever a row was added, removed or edited - and the toggle caret opens the menu through NgbDropdown without calling openDropdown() at all, leaving the set empty and nothing greyed out. It is now derived from the live models, and every path that opens the menu refreshes the options via (openChange). #32's 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. The keys were then incomparable and the duplicate went through. Identity is now the authority *and* the normalised value, and a match on either is a duplicate. Testing ------- 14 specs written first against the unfixed code and confirmed failing, covering the row/control binding across remove/insert/reorder and every duplicate and clearing path. Full suite: 5478 passing. Verified end to end on a dockerised DSpace 8 backend with the datashare theme: the three Type rows survive the dismissing click, the still-used values are greyed out after delete+add (input and caret alike), and what the server stores matches the form with no duplicate and nothing destroyed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Follow-up to #26. While testing the merged change I could still reproduce duplicate values in repeatable, authority-controlled submission dropdowns (Type / Funder). This PR closes the remaining gaps and adds reproduction/regression tests plus a before/after demo.
Reproduced gaps
valueonly (usedSiblingValues.has(entry.value)). For authority vocabularies the stable identity is theauthority, and the stored value string can differ from the option's display — so the same entry could still be selected in two rows.usedSiblingValues. The set was computed only inopenDropdown(), so a value chosen in another row after the dropdown was opened was not reflected and could be committed as a duplicate.Changes
DsDynamicScrollableDropdownComponent+dsBtnDisabled:authority ?? valuefor both the used-set and the option check.selectEntry()re-checks the live sibling values at commit time (defends against the stale set).dsBtnDisablednow also blocksmousedown— options commit onmousedown, which the directive previously let through.Enterno longer selects while options are still (re)loading.aria-selected.Testing
demo/contains a self-contained harness that replicates the component's exact deduplication logic and the Playwright recorder used for the clips below (the full submission form needs the backend). Full-resolution.webmclips are indemo/videos/.