fix(typeahead): rebuild dropdown list on populate to stop duplicate and stale rows

[matharoo]

What this fixes

The typeahead dropdown (used for Collection ID, Activity, etc.) had several bugs where it showed the wrong items.

1. Duplicate items on reopen / double-click

Opening the same dropdown again — or double-clicking it — kept adding copies of the list (4 items became 8, then 12…).

2. Stale items when the list changes

When one dropdown's options depend on another (e.g. Activity depends on Collection), changing the first dropdown left the old options behind. Picking a new collection showed the new activities plus the previous collection's activities.

3. List not cleared when there are no options

Switching to a collection with zero activities still showed the previous collection's activities instead of an empty list.

4. Only the selected item shown on reopen

After picking an item, reopening the dropdown showed just that one item instead of the full list.

Why it happened

The dropdown reused Angular's incremental list rendering inside a Bootstrap-managed menu, which didn't reliably remove old rows. New rows were added but old ones were never taken away.

The fix

• Rebuild the option list from scratch every time it's populated, so old rows can never linger.
• Show the full list when the field is focused (not just the selected item).
• Clear the list when there are no options.
• De-duplicate options so brief flashes of repeated items can't happen.

Scope

All changes are in the typeahead component (ngds-forms). No API or usage changes — existing consumers work the same, just without the duplicate/stale rows.

[marklise]

Overview

This replaces the *ngTemplateOutlet-based option list with markup inlined directly in the <ul>, gated behind a new showList flag that rebuildList() toggles off→on with two synchronous detectChanges() calls, forcing a full teardown/re-stamp of the list on every repopulate. It also de-duplicates the match list, clears the displayed list when the option set is empty, and shows the full list on focus.

Thanks for the unusually clear write-up of the symptoms — it made this much easier to trace.

What's genuinely good

• [attr.aria-current] — the old markup had aria-current="match?.value === control?.value", which set the literal string as the attribute. Real a11y fix.
• Deleting the old #defaultListTemplate removes a block that was never valid: it ended with a stray opening <ng-template> instead of </ng-template>.
• updateDisplayedSelectionListItems clearing on empty is correct and addresses bug #3 directly.

Issues

1. typeaheadMinLength is no longer enforced on focus (correctness — the big one)

onTypeaheadFocus() previously routed through typeaheadChange(this.currentDisplay), and that function is the only place the min-length gate lives (typeahead-input.component.ts:174-189): it sets preventOpening = true and calls dropdownBlur() when inputLength < typeaheadMinLength. The new non-multiselect branch calls showAllItems() instead, which skips that gate entirely.

Since the input's (focus) handler calls dropdownFocus() → openDropdown() unconditionally, a typeahead configured with typeaheadMinLength: 3 will now pop open the full option list the moment it's focused with an empty input — exactly what the input exists to prevent. Note the multiselect branch still calls typeaheadChange(null), so the two branches now disagree about whether min-length applies. showAllItems() needs the same length gate before populating.

2. The selectionListTemplate contract changed — the PR body says it didn't

selectionListTemplate is a public @Input() on NgdsInput (ngds-input.component.ts:90) of a published library. Previously a consumer's template received the whole list ({matches, items, query, control}) and rendered its own markup. Now it's invoked per item with { data: match }, nested inside a <button class="dropdown-item">. Any external consumer using let-matches="matches" will silently render one broken row per option, and a consumer whose template contains a <ul> now emits <ul> inside <button>.

To be fair to the change: {data: match} matches both the picklist convention (picklist-input.component.html:133) and the prose docs on the typeaheads demo page ("access the properties of the option in your template using the data accessor"), and the demo's let-matches example is currently commented out. So this is arguably a fix to an already-inconsistent contract — but it's still breaking for downstream users, and "No API or usage changes — existing consumers work the same" isn't accurate. Worth calling out explicitly, and un-commenting/updating the demo so the supported contract is actually exercised.

3. The root cause looks misdiagnosed, and the fix reverses a recent optimization

The comment claims "the bootstrap-managed <ul> does not reliably remove old rows via incremental *ngFor diffing." *ngFor removes rows reliably — what it does not tolerate is duplicate trackBy keys. The old trackByValue = (_index, item) => item?.value ?? item returns duplicate keys whenever two options share a value, which produces exactly the duplicated/stale rows described here.

The de-dup filter added in this same PR plausibly fixes the root cause on its own. If so, the teardown/rebuild machinery is treating a symptom, and it costs real performance: rebuildList() runs on every keystroke (called from typeaheadChange), destroying and re-creating every row plus two full detectChanges() passes, with trackBy now removed and optionsLimit still unapplied. On a large list (Collection IDs) that's O(n) DOM churn per character typed — a meaningful regression for a shared form library.

Could you test whether the de-dup alone (plus a trackBy keyed on something genuinely unique) resolves all four bugs without showList? That would make this a much smaller and faster patch.

4. De-duping by display silently drops selectable options

onSelectionListItemsChange() filters on matchItem.display. But detectDisplayDuplicates() treats duplicate displays as a warning ("may cause problems when selecting between them") — i.e. a supported-but-discouraged configuration. Options with the same display but distinct values now vanish and become permanently unselectable. That turns a console warning into silent data loss. De-duping on value would be safer, and the seenValues name suggests that was the intent.

5. showList = false in onTypeaheadBlur() is non-deterministic

It's set without a following detectChanges(), so the teardown the comment promises ("Tear the list down when the dropdown closes") only happens whenever the next zone CD tick lands. Given rebuildList() re-stamps on the next open anyway, this line is either redundant or should call detectChanges() to do what it says.

6. setFocusIndex(0) runs before the list is stamped

In both showAllItems() and typeaheadChange(), setFocusIndex(0) is called before rebuildList(). setFocusIndex reaches into dropdownMenu.nativeElement.children[this.focusIndex] when isOpen (ngds-dropdown.component.ts:198-207), so it measures the old DOM — or an empty one. I don't think it crashes today (on focus, isOpen is still false when it runs, so the guard skips), but the ordering is fragile: swap to rebuildList() then setFocusIndex(0).

7. Orphaned code

With the typeahead's #defaultListTemplate gone, getTemplate() (ngds-dropdown.component.ts:368) and the @ViewChild('defaultListTemplate') (line 35) no longer resolve for typeaheads — getTemplate() has no remaining callers I could find. Either remove it or confirm date-input still needs it.

8. No tests

typeaheads.component.spec.ts is a should create stub, so all four bugs remain unguarded against regression. Given the subtlety here, I'd want at least: reopen-doesn't-duplicate, dependent-list-swaps-cleanly, and empty-list-clears.

Recommendation

Issue 1 is a functional regression on a documented @Input, and issue 2 contradicts the stated scope — both worth resolving before merge. I'd also like to settle the approach question in issue 3 first: if the de-dup alone fixes the reported bugs, the showList teardown/rebuild and the loss of trackBy can both be dropped.

One caveat on my confidence: I reviewed this statically at head 5b1ea82 without building or driving the component. Issues 1 and 4 I traced with high confidence through the call paths; issue 3's performance claim is reasoned from the code, not measured.

[matharoo]

thanks for the thorough review Mark!, Fixed the duplicate and stale (cross-collection) options in the typeahead dropdown, and addressed all your review points below.

In short - tldr: kept the list teardown since it's actually needed (1), de-dup now by value not display (2), min-length enforced on focus too (3), optionsLimit applied to keep rebuilds cheap (4), fixed the setFocusIndex ordering (5), dropped the redundant blur reset (6), removed the orphaned getTemplate/ViewChild (7), and added unit tests (8). The selectionListTemplate change is breaking and now documented; demo updated to match.

1. Min-length not enforced on focus
Fixed. When you focus a typeahead, it now checks typeaheadMinLength before showing anything — exactly like
typing does. So a typeahead with typeaheadMinLength: 3 no longer pops the full list open on an empty focus.
Both the focus path and the typing path now behave the same.

2. The selectionListTemplate contract changed (and the PR said it didn't)
You're right, and "no API changes" was wrong — sorry. The template is now called once per option with a
{data} accessor, instead of receiving the whole list. We did this on purpose to match how picklist already
works and what the demo docs describe, but it is a breaking change for anyone using the old let-matches
style. We've un-commented and updated the demo so it actually shows the supported {data} usage, and we're
calling this out as breaking in the PR description.

3. Root cause + the teardown reversing an optimization
We tested your suggestion directly: de-dup + a unique trackBy, no teardown. It does not fix the bugs — the
duplicate flashing and the stale "activities from the old collection" both came straight back. The reason is
the dropdown

is controlled by bootstrap, and Angular's incremental *ngFor diffing over it leaves
stale/duplicate rows behind on a repopulate or a dependent-list swap. So the full teardown/rebuild (showList
• rebuildList) is a correctness requirement here, not just a symptom patch — we've kept it. To address the
  performance cost you (rightly) flagged, we now apply optionsLimit, so the rebuilt list is capped on large
  lists like Collection IDs. trackBy is kept as a harmless safety net.

4. De-duping by display silently drops options
Fixed. De-dup now keys on value, not display. Two options that share a display but have different values both
stay in the list and remain selectable — no more silent data loss.

5. showList = false in blur was non-deterministic
Removed it entirely. It was redundant — the list already gets torn down and rebuilt cleanly the next time the
dropdown opens, so there was nothing for that line to do.

6. setFocusIndex(0) running before the list is stamped
Fixed the ordering. We now rebuild the list first, then set the focus index, so focus is always measured
against the freshly rendered rows.

7. Orphaned code
Removed the now-unused getTemplate() and the dead @ViewChild('defaultListTemplate'). Picklist is unaffected —
it uses its own local template reference, not that one.

8. No tests
Added unit tests covering: de-dup by value, options with a shared display but distinct values being kept,
empty-list clearing, the min-length gate (both under and over the limit), optionsLimit capping, and the
dependent-list swap (activity list changing when the collection changes). Honest caveat: these cover the
logic paths. The bootstrap-

DOM behaviour itself is still verified manually, since there's no test
harness wired up for the bootstrap Dropdown yet — worth a follow-up.