[Alpha][SelectPanel] Fix single select remote loading bug (#2986)

Author: kendallgassner

.changeset/dull-knives-leave.md

@@ -0,0 +1,5 @@
+---
+'@primer/view-components': patch
+---
+
+Fixes Alpha::SelectPanel remote loading bug where items from the server were overriding the users selections. Additionally, update the #removeSelectedItem function to grab the data-value from the correct element.

app/components/primer/alpha/select_panel_element.ts

@@ -84,6 +84,7 @@ export class SelectPanelElement extends HTMLElement {
   #selectedItems: Map<string, SelectedItem> = new Map()
   #loadingDelayTimeoutId: number | null = null
   #loadingAnnouncementTimeoutId: number | null = null
+  #hasLoadedData = false
 
   get open(): boolean {
     return this.dialog.open
@@ -399,9 +400,11 @@ export class SelectPanelElement extends HTMLElement {
     }
   }
 
-  #removeSelectedItem(item: Element) {
-    const value = item.getAttribute('data-value')
+  #removeSelectedItem(item: SelectPanelItem) {
+    const itemContent = this.#getItemContent(item)
+    if (!itemContent) return
 
+    const value = itemContent.getAttribute('data-value')
     if (value) {
       this.#selectedItems.delete(value)
     }
@@ -700,8 +703,12 @@ export class SelectPanelElement extends HTMLElement {
       if (!itemContent) continue
 
       const value = itemContent.getAttribute('data-value')
-
-      if (value && !this.#selectedItems.has(value) && this.isItemChecked(item)) {
+      if (this.#hasLoadedData) {
+        if (value && !this.#selectedItems.has(value)) {
+          itemContent.setAttribute(this.ariaSelectionType, 'false')
+        }
+      } else if (value && !this.#selectedItems.has(value) && this.isItemChecked(item)) {
+        this.#hasLoadedData = true
         this.#addSelectedItem(item)
       }
     }
@@ -863,19 +870,18 @@ export class SelectPanelElement extends HTMLElement {
     const itemContent = this.#getItemContent(item)
 
     if (this.selectVariant === 'single') {
+      const element = this.selectedItems[0]?.element
+      if (element) {
+        this.#getItemContent(element)?.setAttribute(this.ariaSelectionType, 'false')
+      }
+      this.#selectedItems.clear()
+
       // Only check, never uncheck here. Single-select mode does not allow unchecking a checked item.
       if (checked) {
         this.#addSelectedItem(item)
         itemContent?.setAttribute(this.ariaSelectionType, 'true')
       }
 
-      for (const checkedItem of this.querySelectorAll(`[${this.ariaSelectionType}]`)) {
-        if (checkedItem !== itemContent) {
-          this.#removeSelectedItem(checkedItem)
-          checkedItem.setAttribute(this.ariaSelectionType, 'false')
-        }
-      }
-
       this.#setDynamicLabel()
     } else {
       // multi-select mode allows unchecking a checked item

demo/app/controllers/select_panel_items_controller.rb

@@ -42,6 +42,12 @@ def index
                 []
               end
 
+    if unselect_items?
+      results.map! do |result|
+        result.merge(selected: false)
+      end
+    end
+
     clean_up_old_uuids(uuid)
 
     respond_to do |format|
@@ -84,4 +90,12 @@ def clean_up_old_uuids(current_uuid)
   def key_for(uuid)
     "#{COOKIE_PREFIX}#{uuid}"
   end
+
+  def select_items?
+    params.fetch(:select_items, "true") == "true"
+  end
+
+  def unselect_items?
+    !select_items?
+  end
 end

previews/primer/alpha/select_panel_preview.rb

@@ -18,6 +18,7 @@ class SelectPanelPreview < ViewComponent::Preview
       # @param open_on_load toggle
       # @param anchor_align [Symbol] select [start, center, end]
       # @param anchor_side [Symbol] select [outside_bottom, outside_top, outside_left, outside_right]
+      # @param select_items toggle
       def playground(
         title: "Sci-fi equipment",
         subtitle: "Various tools from your favorite shows",
@@ -31,10 +32,12 @@ def playground(
         show_filter: true,
         open_on_load: false,
         anchor_align: :start,
-        anchor_side: :outside_bottom
+        anchor_side: :outside_bottom,
+        select_items: true
       )
         render_with_template(locals: {
           subtitle: subtitle,
+          select_items: select_items,
           system_arguments: {
             title: title,
             size: size,

previews/primer/alpha/select_panel_preview/playground.html.erb

@@ -8,7 +8,8 @@
   src: select_panel_items_path(
     select_variant: :single,
     show_results: !simulate_no_results,
-    fail: simulate_failure
+    fail: simulate_failure,
+    select_items: select_items
   ),
   select_variant: :single,
   fetch_strategy: :remote,

test/system/alpha/select_panel_test.rb

@@ -1,3 +1,4 @@
+
 # frozen_string_literal: true
 
 require "system/test_case"
@@ -182,11 +183,12 @@ def test_remembers_selections_on_filter
       end
 
       # Phaser should already be selected
-      assert_selector "[aria-checked=true]", count: 1
+      assert_selector "[aria-checked=true]", text: "Phaser"
 
       click_on "Photon torpedo"
 
-      assert_selector "[aria-checked=true]", count: 2
+      assert_selector "[aria-checked=true]", text: "Phaser"
+      assert_selector "[aria-checked=true]", text: "Photon torpedo"
 
       wait_for_items_to_load do
         filter_results(query: "ph")
@@ -382,6 +384,100 @@ def test_single_select_disabled_item_cannot_be_checked
       refute_selector "[aria-selected=true]"
     end
 
+    def test_single_select_does_not_allow_server_to_check_items_on_filter_if_selections_already_made
+      # playground is single-select
+      visit_preview(:playground)
+
+      wait_for_items_to_load do
+        click_on_invoker_button
+      end
+
+      # Phaser should already be selected
+      assert_selector "[aria-selected=true]", text: "Phaser"
+
+      click_on "Photon torpedo"
+      click_on_invoker_button
+
+      wait_for_items_to_load do
+        filter_results(query: "ph")
+      end
+
+      # server will render this item checked, but since the user has already made selections,
+      # the server-rendered selections should be ignored
+      refute_selector "[aria-selected=true]", text: "Phaser"
+      assert_selector "[aria-selected=true]", text: "Photon torpedo"
+    end
+
+    def test_single_select_remembers_only_one_checked_item_and_ignores_checked_items_from_server
+      # playground is single-select
+      visit_preview(:playground)
+
+      wait_for_items_to_load do
+        click_on_invoker_button
+      end
+
+      # Phaser should already be selected
+      assert_selector "[aria-selected=true]", text: "Phaser"
+
+      wait_for_items_to_load do
+        filter_results(query: "light")
+      end
+
+      click_on "Lightsaber"
+
+      click_on_invoker_button
+
+      wait_for_items_to_load do
+        filter_results(query: "")
+      end
+
+      refute_selector "[aria-selected=true]", text: "Phaser"
+      assert_selector "[aria-selected=true]", text: "Lightsaber"
+    end
+
+    def test_single_select_handles_all_options_unselected_by_default
+      # playground is single-select
+      visit_preview(:playground, select_items: false)
+
+      wait_for_items_to_load do
+        click_on_invoker_button
+      end
+
+      # Phaser should note already be selected
+      refute_selector "[aria-selected=true]", text: "Phaser"
+
+      wait_for_items_to_load do
+        filter_results(query: "light")
+      end
+
+      click_on "Lightsaber"
+
+      click_on_invoker_button
+
+      wait_for_items_to_load do
+        filter_results(query: "")
+      end
+
+      refute_selector "[aria-selected=true]", text: "Phaser"
+      assert_selector "[aria-selected=true]", text: "Lightsaber"
+
+
+      wait_for_items_to_load do
+        filter_results(query: "ph")
+      end
+
+      click_on "Phaser"
+
+      click_on_invoker_button
+
+      wait_for_items_to_load do
+        filter_results(query: "")
+      end
+
+      assert_selector "[aria-selected=true]", text: "Phaser"
+      refute_selector "[aria-selected=true]", text: "Lightsaber"
+    end
+
     ########## MULTISELECT TESTS ############
 
     def test_multi_select_items_checked
@@ -477,6 +573,31 @@ def test_multi_select_disabled_item_cannot_be_checked
       refute_selector "[aria-checked=true]"
     end
 
+    def test_multi_select_does_not_allow_server_to_check_items_on_filter_if_selections_already_made
+      # remote_fetch is multi-select
+      visit_preview(:remote_fetch)
+
+      wait_for_items_to_load do
+        click_on_invoker_button
+      end
+
+      # Phaser should already be selected
+      assert_selector "[aria-checked=true]", text: "Phaser"
+
+      # check torpedo, uncheck phaser
+      click_on "Photon torpedo"
+      click_on "Phaser"
+
+      wait_for_items_to_load do
+        filter_results(query: "ph")
+      end
+
+      # server will render phaser checked, but since the user has already made selections,
+      # the server-rendered selections should be ignored
+      refute_selector "[aria-checked=true]", text: "Phaser"
+      assert_selector "[aria-checked=true]", text: "Photon torpedo"
+    end
+
     ########## JAVASCRIPT API TESTS ############
 
     def test_disable_item_via_js_api