Skip to content

Commit 5da0e31

Browse files
fiskusclaude
andcommitted
Key the transition boundary to the package
A Transition retains its children across a pending load, which is right for a refresh of the package on screen and wrong for a switch to another one: the retained subtree's toolbar and action bar close over the previous package's namespace and URI, so a click in that window would download, publish or pull against the package the user just left — worse than the spinner the retention removed. Reading namespace_key in view position rebuilds the boundary on a package switch, disposing those handlers and falling back to the spinner. A filter change deliberately does not rebuild: same package, handlers still correct. No navigation switches packages in place today, so this is a guard rather than a fix for a reachable bug — the comments now say that instead of asserting the switch happens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent b5e1b3d commit 5da0e31

2 files changed

Lines changed: 48 additions & 20 deletions

File tree

quilt-sync/ui/dist/.gitkeep

Whitespace-only changes.

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

Lines changed: 48 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -76,14 +76,24 @@ pub fn InstalledPackage() -> impl IntoView {
7676
// `last_fingerprint` below sits up here.
7777
let selection = RwSignal::new(RemoteSelection::default());
7878

79+
// Which package this page is showing. A `Memo` so a change to any *other*
80+
// query param — `filter` is the one that moves — does not read as a package
81+
// switch to the two consumers below.
82+
let namespace_key = Memo::new(move |_| query.read().get("namespace").unwrap_or_default());
83+
7984
// Moving between packages must not carry the previous package's picks over.
80-
// The router keeps this component **mounted** when only the `namespace`
81-
// query changes, so nothing unmounts the selection; and because it is keyed
82-
// by path, a carried-over set would tick same-named files in the package
83-
// just opened. Compares against the previous value rather than firing on
84-
// every read, so the first run (which has no previous namespace) is inert.
85-
Effect::new(move |previous: Option<Option<String>>| {
86-
let namespace = query.read().get("namespace");
85+
// A query-only change does not remount this component, so nothing unmounts
86+
// the selection; and because it is keyed by path, a carried-over set would
87+
// tick same-named files in the package just opened. Compares against the
88+
// previous value, so the first run (which has no previous namespace) is
89+
// inert.
90+
//
91+
// No navigation performs such a switch *today* — every route to this page
92+
// either comes from a different route or is a full webview navigation — so
93+
// this is a guard against a package switch being added in place, not a fix
94+
// for a reachable bug. Same for the keyed boundary below.
95+
Effect::new(move |previous: Option<String>| {
96+
let namespace = namespace_key.get();
8797
if previous.is_some_and(|prev| prev != namespace) {
8898
selection.set(RemoteSelection::default());
8999
}
@@ -190,19 +200,35 @@ pub fn InstalledPackage() -> impl IntoView {
190200
// is pending and falls back only on the initial one, so a genuine change
191201
// updates the page in place. The commit screen made the same switch for a
192202
// related reason (see the note on its boundary).
203+
//
204+
// The boundary is **keyed to the package**: keeping children mounted across a
205+
// load is right for a refresh of the package on screen, and wrong for a switch
206+
// to a different one, because the retained subtree's handlers close over the
207+
// *previous* package's namespace and URI — a click in that window would
208+
// download, publish, or pull against the package the user just left.
209+
// Re-reading `namespace_key` here rebuilds the boundary on a package switch,
210+
// which disposes those handlers and shows the spinner (the pre-`Transition`
211+
// behaviour, and the right one for a genuine navigation). A `filter` change
212+
// deliberately does not rebuild: same package, so the handlers stay correct.
193213
view! {
194-
<Transition fallback=move || {
214+
{move || {
215+
namespace_key.track();
216+
let mismatch_requested = mismatch_requested.clone();
217+
let mismatch_bucket = mismatch_bucket.clone();
218+
let mismatch_catalog = mismatch_catalog.clone();
195219
view! {
196-
<Layout breadcrumbs=vec![] notification=notification ui_locked=ui_locked>
197-
<Spinner />
198-
</Layout>
199-
}
200-
}>
201-
{move || {
202-
let mismatch_requested = mismatch_requested.clone();
203-
let mismatch_bucket = mismatch_bucket.clone();
204-
let mismatch_catalog = mismatch_catalog.clone();
205-
Suspend::new(async move {
220+
<Transition fallback=move || {
221+
view! {
222+
<Layout breadcrumbs=vec![] notification=notification ui_locked=ui_locked>
223+
<Spinner />
224+
</Layout>
225+
}
226+
}>
227+
{move || {
228+
let mismatch_requested = mismatch_requested.clone();
229+
let mismatch_bucket = mismatch_bucket.clone();
230+
let mismatch_catalog = mismatch_catalog.clone();
231+
Suspend::new(async move {
206232
match data.await {
207233
Ok(d) => {
208234
let ns = d.namespace.clone();
@@ -243,8 +269,10 @@ pub fn InstalledPackage() -> impl IntoView {
243269
}
244270
}
245271
})
246-
}}
247-
</Transition>
272+
}}
273+
</Transition>
274+
}
275+
}}
248276
}
249277
}
250278

0 commit comments

Comments
 (0)