Keep the remote-file selection across a package refresh

The selection was created inside InstalledPackageContent, which takes
its data by value out of the resource's Suspend — so every re-resolution
re-ran the component and every RwSignal in it was a brand-new signal.
The picks died on any refresh, including refreshes with nothing to do
with them: edit one local file while choosing remote files to download
and the choices were gone.

It was also stored as indices into an entries list that is sorted by
filename, so one added or dropped entry shifts every later position.
Preserving that would have re-pointed the selection at other files, so
re-keying by path is a precondition, not a tidy-up.

The page now owns it, as All { chosen } | Subset. All pins no names, so
a file arriving in a later refresh is covered by it (today's behaviour);
Subset pins names, so an arrival is never ticked into a download the
user has not seen. Both fall out of one intersection against the entries
the package currently offers, which is also what drops a name that has
since been installed — no cleanup pass anywhere. The header checkbox and
every row read that one derivation and store nothing, so they cannot
disagree.

All-ticked always collapses to All, whatever path reached it: a subset of
every current name would draw identically yet diverge on the next
refresh. `chosen` records that the user asked for everything rather than
left the screen as it opened; nothing reads it yet — it is the state the
always-download-new-files opt-in needs.

The selection clears on a namespace change, since the router keeps this
component mounted between packages. Suspense becomes Transition so the
refreshes that remain update in place instead of blanking to a spinner.
A partial selection now draws indeterminate rather than empty, which
would claim nothing is selected — momentary before this change, standing
after it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: fiskus

quilt-sync/ui/Cargo.toml

@@ -23,7 +23,7 @@ send_wrapper = "0.6.0"
 
 [dev-dependencies]
 wasm-bindgen-test = "0.3.76"
-web-sys = { version = "0.3.103", features = ["DomTokenList", "Element", "HtmlButtonElement", "Node"] }
+web-sys = { version = "0.3.103", features = ["DomTokenList", "Element", "HtmlButtonElement", "HtmlInputElement", "Node"] }
 
 [lints]
 workspace = true

quilt-sync/ui/src/pages/installed_package.rs

@@ -1,12 +1,14 @@
 mod content;
 mod entries;
+mod selection;
 mod status_banner;
 mod toolbar;
 
 use leptos::prelude::*;
 use leptos_router::hooks::use_query_map;
 
 use content::InstalledPackageContent;
+use selection::RemoteSelection;
 use toolbar::build_toolbar_actions;
 
 use crate::commands::{
@@ -66,6 +68,28 @@ pub fn InstalledPackage() -> impl IntoView {
         async move { commands::get_installed_package_data(namespace, filter).await }
     });
 
+    // The remote-file selection is held *here*, not in the content component
+    // that renders the checkboxes. `InstalledPackageContent` receives its data
+    // by value out of the resource's `Suspend`, so every re-resolution re-runs
+    // it and every signal created inside it is a brand-new signal — a selection
+    // created down there is destroyed by any refresh. This is the same reason
+    // `last_fingerprint` below sits up here.
+    let selection = RwSignal::new(RemoteSelection::default());
+
+    // Moving between packages must not carry the previous package's picks over.
+    // The router keeps this component **mounted** when only the `namespace`
+    // query changes, so nothing unmounts the selection; and because it is keyed
+    // by path, a carried-over set would tick same-named files in the package
+    // just opened. Compares against the previous value rather than firing on
+    // every read, so the first run (which has no previous namespace) is inert.
+    Effect::new(move |previous: Option<Option<String>>| {
+        let namespace = query.read().get("namespace");
+        if previous.is_some_and(|prev| prev != namespace) {
+            selection.set(RemoteSelection::default());
+        }
+        namespace
+    });
+
     // Autosync watcher → page refresh: when the backend reports a
     // status change for the currently-open namespace, refetch the
     // detail data so the entries list and toolbar reflect the new
@@ -158,8 +182,16 @@ pub fn InstalledPackage() -> impl IntoView {
         refetch.notify();
     });
 
+    // A `Transition` (not `Suspense`) is deliberate: a plain `Suspense` shows its
+    // fallback whenever the resource re-enters a pending state, so every refresh
+    // that survives the fingerprint gate blanks the whole screen to a spinner and
+    // back — for a change that may have nothing to do with what is on it.
+    // `Transition` keeps the already-rendered children mounted while a later load
+    // is pending and falls back only on the initial one, so a genuine change
+    // updates the page in place. The commit screen made the same switch for a
+    // related reason (see the note on its boundary).
     view! {
-        <Suspense fallback=move || {
+        <Transition fallback=move || {
             view! {
                 <Layout breadcrumbs=vec![] notification=notification ui_locked=ui_locked>
                     <Spinner />
@@ -200,6 +232,7 @@ pub fn InstalledPackage() -> impl IntoView {
                                         local_only=local_only
                                         show_set_remote_popup=show_set_remote_popup
                                         paused_event=paused_event
+                                        selection=selection
                                     />
                                 </Layout>
                             }
@@ -211,7 +244,7 @@ pub fn InstalledPackage() -> impl IntoView {
                     }
                 })
             }}
-        </Suspense>
+        </Transition>
     }
 }
 

quilt-sync/ui/src/pages/installed_package/content.rs

@@ -1,6 +1,9 @@
+use std::collections::BTreeSet;
+
 use leptos::prelude::*;
 
 use super::entries::{EntriesToolbar, EntryRow};
+use super::selection::{RemoteSelection, all_selected, partially_selected, resolve, toggled_all};
 use super::status_banner::StatusBanner;
 use crate::commands::{self, InstalledPackageData, PausedEvent, PullCheck};
 use crate::components::buttons;
@@ -34,6 +37,10 @@ pub(super) fn InstalledPackageContent(
     local_only: bool,
     show_set_remote_popup: RwSignal<bool>,
     paused_event: RwSignal<Option<PausedEvent>>,
+    /// Which remote entries are ticked for download. Owned by the page component
+    /// above the resource boundary so it survives a refresh — see
+    /// [`super::selection`].
+    selection: RwSignal<RemoteSelection>,
 ) -> impl IntoView {
     let filter_unmodified = RwSignal::new(data.filter_unmodified);
     let filter_ignored = RwSignal::new(data.filter_ignored);
@@ -60,14 +67,26 @@ pub(super) fn InstalledPackageContent(
         .iter()
         .any(|e| matches!(e.status.as_str(), "added" | "modified" | "deleted"));
 
-    // Track which remote entries are checked (by index) — all selected by default
-    let initial_checked: Vec<usize> = entries
-        .iter()
-        .enumerate()
-        .filter(|(_, e)| e.status == "remote")
-        .map(|(i, _)| i)
-        .collect();
-    let checked_indices = RwSignal::new(initial_checked);
+    // The remote entries this package currently offers, by path. Every read of
+    // the selection is resolved against this set, which is what lets a preserved
+    // selection stay honest with no cleanup pass: a name that has since been
+    // installed or dropped is simply not in here any more. Held in a
+    // `StoredValue` so the rows share one copy rather than cloning it per row.
+    let remote_paths = StoredValue::new(
+        entries
+            .iter()
+            .filter(|e| e.status == "remote")
+            .map(|e| e.filename.clone())
+            .collect::<BTreeSet<String>>(),
+    );
+
+    // The one derived selection. The header checkbox and every row read *this*
+    // and store nothing of their own, so they cannot disagree with each other —
+    // before, they were two independent derivations over one index vector, and
+    // their agreement rested on both being rebuilt at once.
+    let selected = Memo::new(move |_| {
+        remote_paths.with_value(|remote| selection.with(|s| resolve(s, remote)))
+    });
 
     // Filtered entries
     let entries_for_view = entries.clone();
@@ -89,27 +108,22 @@ pub(super) fn InstalledPackageContent(
     });
 
     // Count checked remote entries
-    let checked_count = Memo::new(move |_| checked_indices.get().len());
+    let checked_count = Memo::new(move |_| selected.with(BTreeSet::len));
 
     let show_toolbar = has_remote_entries || ignored_count > 0 || unmodified_count > 0;
 
-    // Install selected paths
+    // Install selected paths. The resolved selection is already remote-only and
+    // already narrowed to what the package still offers, so it is the path list
+    // verbatim — no index lookup left to mis-resolve.
     let uri_for_install = uri.clone();
-    let entries_for_install = entries.clone();
     let on_install_paths = move |_| {
         let Some(uri) = uri_for_install
             .as_ref()
             .map(std::string::ToString::to_string)
         else {
             return;
         };
-        let indices = checked_indices.get_untracked();
-        let paths: Vec<String> = indices
-            .iter()
-            .filter_map(|&i| entries_for_install.get(i))
-            .filter(|e| e.status == "remote")
-            .map(|e| e.filename.clone())
-            .collect();
+        let paths: Vec<String> = selected.get_untracked().into_iter().collect();
         if paths.is_empty() {
             return;
         }
@@ -130,31 +144,20 @@ pub(super) fn InstalledPackageContent(
         });
     };
 
-    // Select all
-    let entries_for_select = entries.clone();
+    // Select all — clears a full selection, otherwise takes everything.
     let on_select_all = move |_: leptos::ev::Event| {
-        let current = checked_indices.get_untracked();
-        let remote_indices: Vec<usize> = entries_for_select
-            .iter()
-            .enumerate()
-            .filter(|(_, e)| e.status == "remote")
-            .map(|(i, _)| i)
-            .collect();
-        if current.len() == remote_indices.len() {
-            checked_indices.set(Vec::new());
-        } else {
-            checked_indices.set(remote_indices);
-        }
+        let current = selection.get_untracked();
+        selection.set(remote_paths.with_value(|remote| toggled_all(&current, remote)));
     };
 
-    let entries_for_all_check = entries.clone();
     let all_remote_selected = Memo::new(move |_| {
-        let checked = checked_indices.get();
-        let remote_count = entries_for_all_check
-            .iter()
-            .filter(|e| e.status == "remote")
-            .count();
-        remote_count > 0 && checked.len() == remote_count
+        remote_paths.with_value(|remote| selected.with(|s| all_selected(s, remote)))
+    });
+    // Drives the header checkbox's indeterminate state: a partial selection now
+    // outlives a refresh, so drawing it as an empty box would be a standing claim
+    // that nothing is selected.
+    let some_remote_selected = Memo::new(move |_| {
+        remote_paths.with_value(|remote| selected.with(|s| partially_selected(s, remote)))
     });
 
     // Commit button: primary when no remote entries are checked
@@ -318,8 +321,9 @@ pub(super) fn InstalledPackageContent(
                     <Show when=move || show_toolbar>
                         <EntriesToolbar
                             has_remote_entries=has_remote_entries
-                            on_select_all=on_select_all.clone()
+                            on_select_all=on_select_all
                             all_selected=all_remote_selected
+                            partially_selected=some_remote_selected
                             checked_count=checked_count
                             on_install_paths=on_install_paths.clone()
                             filter_unmodified=filter_unmodified
@@ -339,10 +343,11 @@ pub(super) fn InstalledPackageContent(
                             let:item
                         >
                             <EntryRow
-                                index=item.0
                                 entry=item.1
                                 pkg_uri=uri.clone()
-                                checked_indices=checked_indices
+                                selection=selection
+                                selected=selected
+                                remote_paths=remote_paths
                                 notification=notification
                                 show_ignore_popup=show_ignore_popup
                                 show_unignore_popup=show_unignore_popup

quilt-sync/ui/src/pages/installed_package/entries.rs

@@ -1,7 +1,10 @@
+use std::collections::BTreeSet;
+
 use leptos::prelude::*;
 
 use quilt_uri::S3PackageUri;
 
+use super::selection::{RemoteSelection, toggled_path};
 use crate::commands::{self, EntryData};
 use crate::components::buttons;
 use crate::components::{IgnorePopupData, Notification, UnignorePopupData};
@@ -15,6 +18,9 @@ pub(super) fn EntriesToolbar(
     has_remote_entries: bool,
     on_select_all: impl Fn(leptos::ev::Event) + 'static,
     all_selected: Memo<bool>,
+    /// Some remote entries are ticked but not all — draws the box indeterminate
+    /// rather than empty, which would read as "nothing is selected".
+    partially_selected: Memo<bool>,
     checked_count: Memo<usize>,
     on_install_paths: impl Fn(leptos::ev::MouseEvent) + 'static,
     filter_unmodified: RwSignal<bool>,
@@ -46,6 +52,7 @@ pub(super) fn EntriesToolbar(
                                 <input
                                     type="checkbox"
                                     prop:checked=move || all_selected.get()
+                                    prop:indeterminate=move || partially_selected.get()
                                     on:change=on_select_all
                                 />
                                 "Select all"
@@ -134,10 +141,15 @@ fn EntriesFilter(
     reason = "declarative Leptos view; length is markup, not logic complexity"
 )]
 pub(super) fn EntryRow(
-    index: usize,
     entry: EntryData,
     pkg_uri: Option<S3PackageUri>,
-    checked_indices: RwSignal<Vec<usize>>,
+    /// The held selection, written on a checkbox click.
+    selection: RwSignal<RemoteSelection>,
+    /// What is ticked right now — the one derivation the header shares, read
+    /// rather than mirrored, so a row cannot disagree with it.
+    selected: Memo<BTreeSet<String>>,
+    /// The remote entries the package currently offers, for resolving a toggle.
+    remote_paths: StoredValue<BTreeSet<String>>,
     notification: RwSignal<Option<Notification>>,
     show_ignore_popup: RwSignal<Option<IgnorePopupData>>,
     show_unignore_popup: RwSignal<Option<UnignorePopupData>>,
@@ -183,24 +195,23 @@ pub(super) fn EntryRow(
     let filename_title = filename.clone();
 
     // Checkbox state for remote entries
+    let name_for_check = filename.clone();
     let is_checked = Memo::new(move |_| {
         if !is_remote {
             return true; // non-remote always show as checked (disabled)
         }
-        checked_indices.get().contains(&index)
+        selected.with(|s| s.contains(&name_for_check))
     });
 
+    let name_for_toggle = filename.clone();
     let on_checkbox_change = move |_| {
         if !is_remote {
             return;
         }
-        let mut indices = checked_indices.get_untracked();
-        if let Some(pos) = indices.iter().position(|&i| i == index) {
-            indices.remove(pos);
-        } else {
-            indices.push(index);
-        }
-        checked_indices.set(indices);
+        let current = selection.get_untracked();
+        selection.set(
+            remote_paths.with_value(|remote| toggled_path(&current, remote, &name_for_toggle)),
+        );
     };
 
     // Action buttons
@@ -343,3 +354,72 @@ pub(super) fn EntryRow(
         </div>
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::EntriesToolbar;
+    use leptos::prelude::*;
+    use wasm_bindgen::JsCast;
+    use wasm_bindgen_test::wasm_bindgen_test;
+
+    fn mount<N: IntoView + 'static>(f: impl FnOnce() -> N + 'static) -> web_sys::Element {
+        let doc = web_sys::window().unwrap().document().unwrap();
+        let container: web_sys::HtmlElement =
+            doc.create_element("div").unwrap().dyn_into().unwrap();
+        doc.body().unwrap().append_child(&container).unwrap();
+        leptos::mount::mount_to(container.clone(), f).forget();
+        container.into()
+    }
+
+    /// The toolbar's header checkbox in one selection state. `indeterminate` is a
+    /// DOM *property* with no attribute form, so it can only be checked against a
+    /// real element — which is the whole reason these two tests are here rather
+    /// than beside the pure rules in `super::super::selection`.
+    fn header_checkbox(all: bool, partial: bool) -> web_sys::HtmlInputElement {
+        let el = mount(move || {
+            view! {
+                <EntriesToolbar
+                    has_remote_entries=true
+                    on_select_all=|_| {}
+                    all_selected=Memo::new(move |_| all)
+                    partially_selected=Memo::new(move |_| partial)
+                    checked_count=Memo::new(move |_| usize::from(all || partial))
+                    on_install_paths=|_| {}
+                    filter_unmodified=RwSignal::new(true)
+                    filter_ignored=RwSignal::new(true)
+                    ignored_count=0
+                    unmodified_count=0
+                    with_status=false
+                />
+            }
+        });
+        el.query_selector(".select-all input")
+            .unwrap()
+            .expect("the toolbar still renders a select-all checkbox")
+            .dyn_into()
+            .unwrap()
+    }
+
+    /// A partial selection draws **indeterminate**, not empty. An empty box says
+    /// "nothing is selected", which was a momentary lie while the selection died
+    /// on every refresh and is a standing one now that it survives.
+    #[wasm_bindgen_test]
+    fn a_partial_selection_draws_indeterminate() {
+        let header = header_checkbox(false, true);
+        assert!(header.indeterminate());
+        assert!(!header.checked());
+    }
+
+    /// The contrast, so the test above cannot pass by everything being drawn
+    /// indeterminate: full is plainly checked, empty is plainly empty.
+    #[wasm_bindgen_test]
+    fn full_and_empty_selections_draw_plainly() {
+        let full = header_checkbox(true, false);
+        assert!(full.checked());
+        assert!(!full.indeterminate());
+
+        let empty = header_checkbox(false, false);
+        assert!(!empty.checked());
+        assert!(!empty.indeterminate());
+    }
+}