fix(ui): release RefCell borrows across GTK calls outside overlays/ (#643)

[vibechoom]

Refs #643

Sweeps the non-overlays/ half of #643, under the corrected definition from the issue's follow-up comment: any RefCell borrow held across a GTK call, not just a chained borrow_mut().

Scope split — modal.rs / overlays/ deliberately excluded

PR #644 is open and owns trollshell/src/modal.rs and every file under trollshell/src/overlays/. Nothing here touches those files, so the two PRs can land in either order without conflicting. The overlay half of #643 — including the four install-shaped insert sites and the notifications.rs named-binding cluster the follow-up comment added — stays with #644. That is why this is Refs #643, not Closes.

The hazard, and the four spellings

A RefCell borrow held live across a GTK call that can synchronously emit a signal. If any handler on that emission borrows the same cell, it panics — and a panic unwinding through a glib callback aborts the process. Reachability is not the bar: the whole #631/#644 cluster fixed sites where emission is unverified, and this matches that standard.

All four spellings appear below:

1. chained temporary — for x in cell.borrow_mut().drain(..) { gtk_call(x) }; the RefMut lives for the whole loop.
2. named binding — let mut m = cell.borrow_mut(); held live across GTK calls further down.
3. shared borrow() held across GTK work where a borrow_mut() exists on another path.
4. argument-position temporary — w.set_title(&f(&mut cell.borrow_mut())); the Ref/RefMut is a temporary of the whole statement, so it outlives the call.

The semicolon rule

insert-shaped calls are safe or unsafe depending on a semicolon.

// SAFE — closure tail expression. The returned `Option<V>` is moved out past
// the `RefMut` and dropped by the caller, after the borrow has ended.
TOAST_WINDOWS.with(|map| map.borrow_mut().insert(k, v));

// HAZARD — same call, trailing semicolon. The returned `Option<V>` is now a
// statement temporary. Temporaries drop in reverse creation order, and the
// `Option` is created last (it is the call's return value), so it drops
// *before* the `RefMut` — i.e. inside the borrow, running V's drop glue there.
TOAST_WINDOWS.with(|map| { map.borrow_mut().insert(k, v); });

Two corollaries this PR leans on:

• drop(cell.borrow_mut().insert(k, v)) does not fix the direct (non-closure) spelling — drop consumes the value while the statement's RefMut temporary is still alive. Only a closure boundary (as in fix(overlays): release RefCell borrows before the GTK call that can re-enter them (#631) #644's drop(PANELS.with(...))) or binding the result with let moves the drop past the borrow.
• RefCell::take() is not borrow_mut().take(). take()'s own borrow ends before it returns, so if let Some(x) = cell.take() { gtk_call(&x) } is already correct, while if let Some(x) = cell.borrow_mut().take() { … } keeps the RefMut alive for the whole then-block.

Sites fixed (22)

crates/hytte-ui/src/popup.rs

• :286 close_catchers — spelling (1). borrow_mut().drain(..) across win.destroy(). → take(). fix(hytte-ui): destroy() the three remaining close()-teardown sites (#638) #639 converted this function's close() → destroy() without touching the borrow.

trollshell/src/components/reactive_list.rs

• :88 reactive_list — spelling (1), across remove_row(). → take(). Highest blast radius in the sweep: this generic helper backs 8 consumers (panels/{appearance,vpn,clipboard,displays,stats}.rs, panels/network/{wired,wifi,connection}.rs).

trollshell/src/components/power_profile.rs

• :49 — spelling (1), across expander.remove(). → take().

trollshell/src/panels/stats.rs

• :632 build_top_apps_expander — spelling (1), across remove(). → take().
• :654 — spelling (4). sample_display_name(s, &mut meta_cache.borrow_mut()) inlined into row.set_title(…), so the RefMut lives across set_title. → bind the title first.
• :661 — spelling (4). The RefMut from resolve_app_meta(app_id, &mut meta_cache.borrow_mut()) lives across the unwrap_or_else that builds the fallback icon. Weakest site of the group (a ThemedIcon constructor on a fresh object has no handlers); fixed for uniformity of the statement shape rather than on its own evidence.
• :756 build_live_per_core_row — spelling (2). let mut bars = bars_for_bind.borrow_mut(); held live across cores_row.remove(), cores_row.append(), bar.set_fraction() and bar.set_tooltip_text(). → take() for the whole update, stored back at the end. Side benefit: bars.clear() no longer drops GtkProgressBars inside the borrow.

trollshell/src/widgets/calendar.rs

• :328 rebuild_upcoming_list — spelling (1), across group.remove(). → take().
• :331 — Form B, borrow_mut().take() with group.remove() in the then-block. → RefCell::take().
• :281 on_day_clicked — spelling (3). rows_track.borrow() held across flash_row_highlight(); rebuild_upcoming_list is the borrow_mut() counterparty. → clone the matched row out, then flash.
• :408 refresh_upcoming_list — spelling (3), and the longest borrow in the sweep: state.events.borrow() passed as a live Ref into rebuild_upcoming_list, i.e. held across the entire group.remove()/group.add() rebuild. wire_events_bind does state.events.borrow_mut() on the other path. → clone the snapshot (one Vec<CalendarEvent> clone per refresh, negligible next to rebuilding the rows).

trollshell/src/widgets/tasks.rs

• :139 rebuild_list — spelling (1), across group.remove(). → take().
• :142, :145 — Form B ×2 (placeholder_track, overflow_track). → RefCell::take().
• :415 build_create_popover's connect_show — spelling (4). sync_list_picker(&picker, &lists_for_show.borrow()) holds the Ref across that function's GTK work; wire_lists_bind (:79) is the borrow_mut() counterparty. → clone out first.

trollshell/src/panels/connections.rs

• :67 (owned) and :71 (others) — spelling (2) ×2. Named RefMut bindings held from the top of the bind closure past every group.remove(), group.add(), other.remove() and other.add_row(), through to the end of the row-building loop. → take() both, rebuild into owned Vecs, store back once at the end.
• :75 (overflow_for_bind) — Form B. → RefCell::take().

crates/trollshell-control-center/src/main.rs

• :427 apply_plugins's same_set branch — spelling (3), the strongest-evidence site in this PR. state.by_id.borrow() is held across update_plugin_row, which ends in prow.row.set_active(…). GObject property notification is synchronous, so the row's own connect_active_notify handler runs inside that call. It does not panic today only because that handler early-returns on the syncing guard — exactly the "no known reentrant borrower" standard the fix(overlays): release RefCell borrows before the GTK call that can re-enter them (#631) #644 sites were held to. by_id.borrow_mut() counterparties live at :441 and :452. → re-borrow per iteration and clone the handle out (PluginRow gains #[derive(Clone)]; both its fields are refcounted GObjects).
• :441 — the semicolon rule, direct (non-closure) spelling. state.by_id.borrow_mut().insert(id.clone(), prow); drops the displaced Option<PluginRow> — two GTK widgets — inside the borrow. → bind with let, then drop. Weakest site here: clear_rows runs immediately above, so the displaced value is None today. Kept for the same reason fix(overlays): release RefCell borrows before the GTK call that can re-enter them (#631) #644 kept its install counterpart.
• :449 clear_rows — spelling (1), across group.remove(). → take().
• :452 clear_rows — state.by_id.borrow_mut().clear() runs each PluginRow's drop glue (a GtkSwitchRow + a GtkImage) inside the borrow. → drop(state.by_id.take()), whose borrow is over before the map drops.

Beyond the issue's list

The issue named 12 sites across these eight files. Applying the corrected four-spelling definition turned up 10 more in the same files: stats.rs:654/:661/:756, calendar.rs:281/:408, tasks.rs:415, and control-center :427/:441/:452. All are fixed above and called out individually.

Examined and deliberately left alone

Write-backs of the form *cell.borrow_mut() = new; / .push(x); are not hazards here and were not churned:
reactive_list.rs:95,105, power_profile.rs:70, calendar.rs:342,390, tasks.rs:79,156,167,178, connections.rs:109, popup.rs:256, control-center :440,:468. push returns (); each assignment replaces a value the same function already take()d, so the value dropped inside the borrow is an empty Vec or a None, with no drop glue to run. The two write-backs this PR adds (connections.rs, stats.rs's bars) carry a one-line comment saying exactly that, so the next sweep doesn't have to re-derive it.

• stats.rs:143 — PENDING_SCROLL.with(|m| { m.borrow_mut().insert(monitor_key(monitor), section); }); has the trailing semicolon, so the displaced value really does drop inside the borrow — but StatsSection is a #[derive(Clone, Copy)] fieldless enum with no drop glue, so nothing runs there. The benign side of the semicolon rule; left as-is rather than wrapped.
• tasks.rs:361 — let has_list = !lists_for_changed.borrow().is_empty(); on its own line, set_sensitive on the next. The Ref dies at the end of the let. This is the counter-example to spelling (4): the same expression inlined into the set_sensitive argument would be a hazard.
• tasks.rs:387-393 — already the correct idiom, with an explicit drop(lists); before the GTK calls.
• stats.rs:1025-1030 — borrow confined to an explicit block that closes before the set_text calls. :1039's for &v in window.borrow().iter() holds a Ref across a loop whose body is pure arithmetic.
• stats.rs:380, stats.rs:641, calendar.rs:302,514, tasks.rs:644-760, control-center :423 — borrow confined to a statement or block containing no GTK call.
• popup.rs:401,408 — test assertions.

The issue's own ruled-out list is left untouched, and I agree with it: of the two that fall in my files, components/focus.rs:146 aborts a timer and control-center :114 aborts a SourceId — no GTK call in either body.

Test

One test added, and it is a real falsification rather than a shape assertion: close_catchers_tolerates_a_reentrant_teardown_from_destroy in crates/hytte-ui/src/popup.rs.

GtkWidget::destroy is emitted synchronously from dispose, so a connect_destroy handler runs inside win.destroy(). The test hangs such a handler on a catcher and has it re-enter close_catchers on the same cell — the exact shape a real re-entrant teardown takes.

Falsified against the pre-fix borrow_mut().drain(..) before restoring it, and the failure mode is the one #631/#643 describe rather than a clean assert:

test popup::tests::close_catchers_drains_and_destroys_idempotently ... ok
thread '<unnamed>' panicked at crates/hytte-ui/src/popup.rs:293:25:
RefCell already borrowed
   2: core::cell::panic_already_borrowed
   3: core::cell::RefCell<T>::borrow_mut
thread '<unnamed>' panicked at .../core/src/panicking.rs:225:5:
panic in a function that cannot unwind
thread caused non-unwinding panic. aborting.
error: test failed ... (signal: 6, SIGABRT: process abort signal)

So this also settles, in-tree and reproducibly, the question the rest of the cluster leaves open: the emission is synchronous, and the resulting BorrowMutError does abort the process rather than unwind. With take() the inner call finds an empty vec and returns; all 106 hytte-ui unit tests pass.

It sits in hytte-ui's existing #[cfg(all(test, feature = "system-tests"))] mod tests next to close_catchers_drains_and_destroys_idempotently, so it runs in nix flake check's system-tests bucket under xvfb-run, not in the default hermetic cargo test.

No Waker-based test applies to the other 21 sites. #644's std::task::Wake harness works because reset_drawer_open_states calls Mutable::set_neq while holding the borrow, and futures-signals invokes Waker::wake() synchronously from notify — the reentrancy trigger is pure std. None of the sites here touch a Mutable inside the borrow: every trigger is a GTK emission (remove, destroy, set_title, set_active). And neither trollshell nor trollshell-control-center defines the system-tests feature or has any display-test harness, so there is nowhere to hang the equivalent for them. "Not hermetically testable" here is a statement about the trigger, not a shrug — popup.rs is the one site whose crate already has the display harness its trigger needs, which is why it is the one that got a test.

Gates

• cargo clippy --workspace --all-targets --features system-tests -- -D warnings, with RUSTC_WRAPPER unset (sccache serves cached artifacts and skips lint emission on hits, so a wrapped run is a false green) — clean, no warnings.
• cargo test --workspace (hermetic) — all pass, including trollshell's 233 and hytte-services' 672.
• xvfb-run cargo test -p hytte-ui --features system-tests — 106 + 1 pass; falsified against the pre-fix shape first (SIGABRT above), then restored.
• nix fmt — 0 changed, and idempotent on a second run. (Used nix fmt, not cargo fmt, so the #[cfg(feature)]-gated blocks are covered.)
• nix build .#trollshell — pass (which also runs the workspace's doCheck suite).

Line numbers above are from the pre-change tree, matching the issue; function names are given for each so they stay findable after the diff.

[vibechoom]

Adversarial review

Verdict: the 22 conversions are correct. No take()-vs-drain() semantic drift and no lost write survives scrutiny. I attacked the six things most likely to be wrong in a transformation of this shape (below), and the code holds on all six.

What does not hold is part of the audit trail. Three of the claims the PR body records as settled facts are wrong on the source, and one of them — the "strongest-evidence site" — has already been propagated into #643's second comment as the thing that raises the cluster's evidence grade. Since the durable output of this cluster is the enumerated definition and the exclusion list, not the 22 take()s, those need correcting before this lands.

All line numbers below are on the branch (ebb1704), not the pre-change tree.

---

Finding 1 — the "strongest-evidence site" is not stronger than the rest; the syncing guard is not what saves it

crates/trollshell-control-center/src/main.rs:428-436 (comment) and the PR body's :427 entry, repeated verbatim in #643's second comment:

It does not panic today only because that handler early-returns on the syncing guard — exactly the "no known reentrant borrower" standard the #644 sites were held to.

The first half is right: update_plugin_row does end in set_active (main.rs:553-555), notify::active is emitted synchronously, and row.connect_active_notify (main.rs:518) does run inside that call.

The second half is not. Delete the guard and the site still cannot panic. The handler body past the guard is main.rs:522-529:

let want_on = sw.is_active();
let (state, id) = (state.clone(), id.clone());
spawn_on_runtime(set_plugin_state(id, want_on), move |res| { … refresh_plugins_soon(&state); });

and spawn_on_runtime (main.rs:916-931) is runtime::handle().spawn(…) plus glib::spawn_future_local(async { rx.await … }) over a oneshot. The only route from this handler to by_id is refresh_plugins_soon → refresh_plugins → apply_plugins/clear_rows, and every one of those hops sits behind a oneshot awaiting a D-Bus round trip on a tokio worker. It cannot execute before the outer set_active returns, guard or no guard. Nothing else on the row connects — set_subtitle (:550) and apply_badge (:551, which does notify::icon-name/visible + css) have no handlers either.

Refutation attempt: I looked for a second synchronous path — another connect_active_notify on the inner GtkSwitch, a bind_two_way on the row, a connect_notify elsewhere. There is none; main.rs:518 is the only connection on prow.row, and main.rs:181 / :200 (the Place tab) don't touch rows/by_id. The refutation failed to save the claim.

Consequence: :427 carries exactly the same evidence grade as the other 21 — a borrow across a call whose emission reaches no known re-entrant borrower. That's a fine reason to convert it, and I'd keep the change. It just isn't the empirical anchor the PR and the issue now say it is, and #643's comment currently tells whoever picks up the overlay half that the control-center proves something it doesn't.

Suggested fix: rewrite main.rs:428-436 to say the handler defers all work through spawn_on_runtime, so no synchronous path back into by_id exists; drop "strongest-evidence site" from the PR body; post a correction on #643 so the follow-up sweep doesn't inherit it.

Severity: maintenance hazard (false premise in a cluster whose value is its recorded reasoning; no code defect).

---

Finding 2 — tasks.rs is declared swept, but DuePicker::refresh_summary is a live spelling-(3) site

The PR body's exclusion list contains tasks.rs:644-760 — borrow confined to a statement or block containing no GTK call. That range (pre-change) covers refresh_summary, and the claim is false for it. trollshell/src/widgets/tasks.rs:688:

fn refresh_summary(&self) {
    match *self.mode.borrow() {
        DueMode::None => { self.summary_label.set_visible(false); }          // :690
        DueMode::Today => { self.summary_label.set_visible(true);            // :693
                            self.summary_label.set_text("Due today"); }      // :694
        …

A match scrutinee temporary is not an if-condition temporary: it lives for the whole match expression. So the Ref<DueMode> is held across four GTK calls (:690, :693-694, :697-698, :705-706) — spelling (3) as the PR defines it, with three borrow_mut() counterparties on the same cell at :667 (set_mode), :748 (set_value) and :769 (reset).

Failure scenario: any handler reachable from summary_label.set_visible/set_text that flips a due chip → connect_toggled (:622-644) → set_mode → *self.mode.borrow_mut() = mode (:667) → BorrowMutError inside a glib callback → SIGABRT, i.e. the exact outcome this PR's own test demonstrates.

Refutation attempt: summary_label is private to DuePicker (never handed out), set_visible/set_text emit only notify::visible/notify::label plus a deferred frame-clock resize, and nothing in the tree connects to those. So it is not reachable today. That is precisely the standard under which the other 22 sites were converted — the refutation establishes "unreachable", not "not a site", and the PR's stated rule is any borrow across a GTK call.

Suggested fix: let mode = *self.mode.borrow(); then match mode { … } — DueMode is #[derive(Clone, Copy)] (:558), so it's a one-line change. And correct the exclusion entry, which currently certifies a 116-line range on a claim that fails inside it.

Severity: maintenance hazard (unreachable today; a wrong "checked, clean" entry is worse than an unchecked one, because the next sweeper trusts it).

---

Finding 3 — the do_create exclusion's stated reason is wrong

PR body: tasks.rs:387-393 — already the correct idiom, with an explicit drop(lists); before the GTK calls. On the branch that's trollshell/src/widgets/tasks.rs:393-399:

let lists = lists_for_create.borrow();                         // :393
let idx = list_picker_for_create.selected() as usize;          // :394  GTK call, borrow live
let Some(list) = lists.get(idx) else { return; };              // :395
let _ = tasks::create_task(list.uid.clone(), summary, due_picker_for_create.value());  // :398
drop(lists);                                                   // :399

drop(lists) is after the only GTK call in the block, not before it. This is spelling (2) verbatim — a named Ref binding held across gtk::DropDown::selected() — with wire_lists_bind (:79) as the borrow_mut() counterparty.

Refutation attempt: selected() is a g_object_get property read, which emits nothing and cannot run Rust code; create_task (:398) is a channel send; due_picker.value() touches only DuePicker's own cells. So the exclusion is correct — but on getter-vs-setter grounds, which the PR's four-spelling definition never states, not on the "drop comes first" grounds it records. The refutation saves the code and kills the reason.

Suggested fix: hoist let idx = list_picker_for_create.selected() as usize; above :393 (which also makes drop(lists) unnecessary), and add the getter/setter distinction to the definition — several exclusions across this PR silently depend on it (stats.rs:143's monitor_key(monitor) → Monitor::connector() is the same case).

Severity: maintenance hazard, low.

---

Finding 4 — three comment inaccuracies, grouped

• crates/trollshell-control-center/src/main.rs:450-457: "it would drop two GTK widgets while by_id is borrowed". Dropping an adw::SwitchRow/gtk::Image handle is a g_object_unref; the row was handed to state.group.add (:448) and state.rows (:449) one and two lines earlier, so the refcount can't reach zero and no dispose/finalize runs. The comment then concedes the value is None anyway. The drop-order rule it states is correct (I checked it: the statement's result is produced after the receiver temporary and same-scope drops run in reverse creation order, so the Option does drop inside the RefMut); only the "two GTK widgets get torn down" consequence is overstated. Keep the change, soften the comment.
• trollshell/src/panels/connections.rs:73: "stored back once, at the end". The write-back is at :110-111; the overflow-hint block (:113-122) and three other_for_bind calls (:124-129) run after it. Nothing pushes to owned/others after :111, so it's correct — just not "at the end", which is what a future reader will check against.
• trollshell/src/panels/stats.rs:645 — sample_display_name(s, &mut meta_cache.borrow_mut()) inside the format! feeding set_subtitle at :650 — is the same spelling as the two sites converted 15 lines below it, and appears in neither the fixed list nor the exclusion list. It is safe (the RefMut is a temporary of the closure's tail expression, so it dies when the closure returns, before set_subtitle), but it's the one place a reader has to re-derive that.

Severity: style/accuracy.

---

On the new test — genuine falsification, one overstated sentence

crates/hytte-ui/src/popup.rs:439-454. I assessed whether it would still fail with the fix reverted, and it would: catchers.borrow_mut().drain(..) keeps the RefMut live for the loop, gtk_window_destroy → dispose emits destroy synchronously, the handler re-enters close_catchers, borrow_mut() panics, and the panic crosses an extern "C" trampoline → non-unwinding abort. The pasted SIGABRT is consistent with that path. It is a real test of a real mechanism and the two facts it establishes (destroy is synchronous; a BorrowMutError there aborts rather than fails) are the ones the cluster needed.

Two caveats worth writing down rather than fixing:

• The doc comment (:432-433) calls the handler "the exact shape a real re-entrant teardown would take". Nothing in the tree installs a connect_destroy on a catcher — build_popover_catcher (:324-364) attaches only a GestureClick and an EventControllerScroll, and the two production callers of close_catchers (:275, :278) are sequential closed/unmap emissions on the popover, not nested. It is a synthetic probe of the mechanism, which is the right thing to have built; it just isn't evidence that popup.rs had a reachable bug.
• The assertion itself (:450-453) is vacuous post-fix — take() guarantees an empty cell whether or not destroy() is called. The test's entire signal is "did not abort", which means a regression takes down the whole hytte-ui system-tests binary rather than reporting one failure. Acceptable given the bug's nature (the sibling test at :389 covers the destroy-actually-happened half), but the CI output on a regression will point at whatever test happened to be running.

---

Coverage — attacked and found clean

1. take()-vs-drain() drift, all 9 loop sites. For each I traced whether anything can observe the cell between the take and the end of the loop. bind (crates/hytte-reactive/src/bind.rs:38-46) drives apply from inside a glib::MainContext::spawn_local poll; glib will not re-dispatch a source that is in-call and a Mutable wake only schedules, so no bind closure can re-enter itself — which kills the "signal fires twice, nested" family outright. I then enumerated every connect_* on the removed rows and their containers: calendar rows connect only connect_activated (calendar.rs:761), task rows connect_toggled/connect_activate (tasks.rs:226, :307), catchers a click+scroll gesture, control-center rows connect_active_notify (deferred, above). None is emitted by parenting/unparenting. Clean.
2. Lost writes. Every converted site's write-back is a clobbering assignment, so I checked each cell's full user list and every early exit between take and write-back. stats.rs:770→796 (core_bars): one clone, straight-line body, no return/?. connections.rs:74/77/80→110/111/121: three cells, one closure each, and — the thing I most expected to be broken — the overflow hint at :121 writes a different cell (owned_overflow_track, :43) in both versions, so moving the owned/others write-back above the hint block loses nothing. calendar.rs:338→403 and tasks.rs:145→173: the only return (calendar.rs:356, tasks.rs:163) fires before any row is mounted and tracks its placeholder first, byte-identical to the drain version. reactive_list.rs:94→111: same, with the empty-branch push at :101. power_profile.rs:53→74: unconditional. Clean.
3. The four spellings, re-grepped from the branch rather than trusted from the body. Every remaining .borrow()/.borrow_mut() in the 8 files classified. Result: Findings 2 and 3 above; everything else is genuinely safe. Notable near-misses I checked and cleared: stats.rs:1042 (safe only because of the explicit { } at :1041-1047), stats.rs:1056 (for &v in window.borrow().iter() — chained scrutinee across a loop, pure arithmetic body), calendar.rs:531 (group_events_by_day(&state.events.borrow()) — argument-position Ref, but the callee is pure and the GTK work starts at :536), tasks.rs:714 (same match-scrutinee shape as Finding 2 but with GTK-free arms), main.rs:449/:458/:493 (argument-position, but the arguments are g_object_ref/upcast/String::clone), and main.rs:114 (plugins_poll.borrow_mut().take() across source.remove() — g_source_remove, no widget signal; the issue's ruling on it is right).
4. control-center:427 reading. Verified update_plugin_row is untouched by the diff, so syncing.set(true) still brackets set_active exactly as before (main.rs:553-555); the restructure cannot have moved the guard. Also confirmed #[derive(Clone)] PluginRow is safe: both fields (adw::SwitchRow, gtk::Image, main.rs:289-290) are refcounted GObjects, so the clone is the same object — and build_plugin_row (:511-518) already relied on that. The separate evidence claim is Finding 1.
5. reactive_list.rs rebuild path across a fast double-emission. futures-signals coalesces to the latest value and the apply loop is sequential, so two rapid emissions are two sequential rebuilds, each taking what the previous one assigned. No duplicated or dropped rows in the 8 consumers.
6. The semicolon rule as stated. Checked independently: the closure-tail form moves the Option out past the RefMut; the trailing-semicolon form drops it inside; and drop(cell.borrow_mut().insert(k, v)) does not fix it, because drop consumes the value while the statement's RefMut is still live. The RefCell::take() vs borrow_mut().take() corollary is right too, including under edition 2024 — if let rescoping only moves scrutinee temporaries out of the else arm, so the pre-change Form B sites really were holding the RefMut across remove().

Also confirmed: no file overlap with #644 (modal.rs + overlays/ are untouched here), so the two can land in either order as the body claims. CI (flake-check) was still pending when I looked, so the gate table in the body is unverified from my side.