docs: Refine claude skills with respect to safe reentrancy

.claude/skills/gpui-async/SKILL.md

@@ -133,6 +133,43 @@ Tasks are automatically cancelled when dropped. Store in struct to keep alive.
 
 ## Common Pitfalls
 
+### ❌ Don't: Use `defer_in` and then update the same entity through its handle
+
+`cx.defer_in(window, callback)` schedules `callback` to run **on the current entity** — GPUI re-acquires that entity's lock to execute it. Calling `entity.update(cx, …)` on the *same* entity from within the deferred callback re-enters the lock and panics:
+
+```
+cannot update … while it is already being updated
+```
+
+```rust
+// ❌ Panic: list entity is locked for the defer_in; calling list.update re-enters
+fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context<ListState<Self>>) {
+    cx.defer_in(window, |list_state, window, cx| {
+        parent.update(cx, |this, cx| {
+            this.inner_list.update(cx, |_, _| {}); // PANIC if inner_list == the deferred entity
+        });
+    });
+}
+```
+
+```rust
+// ✅ Correct: use the direct &mut reference — no lock needed
+fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context<ListState<Self>>) {
+    cx.defer_in(window, |list_state, window, cx| {
+        // Access list data directly through the &mut reference
+        list_state.delegate_mut().some_method();
+
+        // Update a *different* entity — fine, different lock
+        parent.update(cx, |this, cx| { /* … */ });
+
+        // Sync list state directly after parent update — no lock needed
+        list_state.delegate_mut().update_snapshot(new_val);
+    });
+}
+```
+
+The rule: inside a `defer_in` callback, **never call `entity.update(cx, …)` or `entity.read(cx)` on the entity the `defer_in` was scheduled on**. Use the `&mut Entity` direct reference the callback provides instead.
+
 ### ❌ Don't: Update entities from background tasks
 
 ```rust

.claude/skills/gpui-entity/SKILL.md

@@ -120,17 +120,87 @@ entity.update(cx, |state, inner_cx| {
 });
 ```
 
-### Avoid Nested Updates
+### Avoid Nested Entity Updates
+
+Nested `entity.update(cx, …)` calls are dangerous. The default posture is: **do not nest them**. The sub-cases below clarify when a panic is guaranteed vs. merely possible.
+
+**Same entity → always panics.**  
+GPUI locks an entity for the entire duration of its update or render pass. Re-entering that same lock panics immediately:
+
+```
+cannot update … while it is already being updated
+```
 
 ```rust
-// ✅ Good: Sequential updates
-entity1.update(cx, |state, cx| { /* ... */ });
-entity2.update(cx, |state, cx| { /* ... */ });
+// ❌ Panic: updating entity_a from inside entity_a's own update
+entity_a.update(cx, |state, cx| {
+    entity_a.update(cx, |_, _| {}); // PANIC — same lock
+});
+```
 
-// ❌ Bad: Nested updates (may panic)
-entity1.update(cx, |_, cx| {
-    entity2.update(cx, |_, cx| { /* ... */ });
+**Different entity → generally safe, but indirect cycles still panic.**  
+Each entity has its own lock, so updating `entity_b` from within `entity_a`'s update normally succeeds. However, if `entity_b`'s callback reaches back into `entity_a` — directly or through a chain — GPUI will attempt to re-acquire `entity_a`'s lock and panic.
+
+```rust
+// ✅ Usually fine: different entities, no cycle
+entity_a.update(cx, |_, cx| {
+    entity_b.update(cx, |_, _| {}); // OK — different lock
 });
+
+// ❌ Panic: indirect cycle back to entity_a
+entity_a.update(cx, |_, cx| {
+    entity_b.update(cx, |_, cx| {
+        entity_a.update(cx, |_, _| {}); // PANIC — entity_a is still locked
+    });
+});
+```
+
+When in doubt, flatten the call sequence rather than nesting: finish the outer update, then update the second entity from outside.
+
+**`defer_in` does not bypass the lock.** `cx.defer_in(window, callback)` schedules `callback` to run on the current entity — meaning GPUI re-acquires the entity's lock to execute it. The re-entrancy rules apply equally inside the deferred callback:
+
+```rust
+// ❌ Panic: defer_in re-locks entity_a; calling entity_a.update inside re-enters
+impl SomeDelegate for MyAdapter {
+    fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context<ListState<Self>>) {
+        cx.defer_in(window, |list_state, window, cx| {
+            // list_state is locked for this callback!
+            parent.update(cx, |this, cx| {
+                this.list.update(cx, |_, _| {}); // PANIC — list is already locked above
+            });
+        });
+    }
+}
+
+// ✅ Fix: use the direct &mut reference the callback provides
+impl SomeDelegate for MyAdapter {
+    fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context<ListState<Self>>) {
+        cx.defer_in(window, |list_state, window, cx| {
+            // Access list data directly — no entity lock needed
+            list_state.delegate_mut().some_hook();
+
+            // Update the *parent* entity — different lock, safe
+            parent.update(cx, |this, cx| { /* … */ });
+
+            // Sync list state directly after parent update
+            list_state.delegate_mut().update_snapshot(new_val);
+        });
+    }
+}
+```
+
+**Snapshot pattern for render callbacks.** `render_item` (and any other rendering hook) runs inside the entity's render pass. It must never call `entity.read(cx)` or `entity.update(cx, …)` on any external entity. Instead, keep a plain `snapshot` field updated eagerly from *outside* render after every mutation:
+
+```rust
+// ❌ Panic in render_item — ListState is already locked
+fn render_item(&mut self, ix: IndexPath, window: &mut Window, cx: &mut Context<ListState<Self>>) -> … {
+    let checked = parent_entity.read(cx).selection.contains(&ix); // PANIC
+}
+
+// ✅ Read from a plain snapshot field — no entity access
+fn render_item(&mut self, ix: IndexPath, window: &mut Window, cx: &mut Context<ListState<Self>>) -> … {
+    let checked = self.selection_snapshot.iter().any(|(sel_ix, _)| sel_ix == &ix);
+}
 ```
 
 ## Common Use Cases

.claude/skills/gpui-entity/references/best-practices.md

@@ -4,34 +4,87 @@ Guidelines and best practices for effective entity management in GPUI.
 
 ## Avoiding Common Pitfalls
 
-### Avoid Entity Borrowing Conflicts
+### Avoid Re-entrant Entity Access (Same Entity)
 
-**Problem:** Nested updates can cause borrow checker panics.
+**Problem:** GPUI locks an entity for the entire duration of its render or update pass. Any attempt to `read` or `update` that *same* entity while the lock is held panics:
+
+```
+cannot update … while it is already being updated
+```
 
 ```rust
-// ❌ Bad: Nested updates can panic
-entity1.update(cx, |_, cx| {
-    entity2.update(cx, |_, cx| {
-        // This may panic if entities are related
-    });
+// ❌ Panic: entity_a updated from within entity_a's own update
+entity_a.update(cx, |_, cx| {
+    entity_a.update(cx, |_, _| {}); // PANIC
+});
+
+// ✅ Fine: updating a *different* entity from within an update
+entity_a.update(cx, |_, cx| {
+    entity_b.update(cx, |_, _| {}); // OK — different lock
 });
 ```
 
-**Solution:** Perform updates sequentially.
+**Note:** Updating two *different* entities in a nested fashion is safe. The restriction is strictly about re-entering the *same* entity's lock.
+
+### `defer_in` Re-locks the Entity — Same Rules Apply
+
+`cx.defer_in(window, callback)` schedules `callback` to run *on the entity the context refers to*. GPUI re-acquires that entity's lock to execute the deferred callback, so the re-entrancy rules apply equally inside it:
 
 ```rust
-// ✅ Good: Sequential updates
-entity1.update(cx, |state1, cx| {
-    // Update entity1
-    state1.value = 42;
-    cx.notify();
-});
+// ❌ Panic: defer_in runs with ListState locked; calling list.update re-enters
+fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context<ListState<Self>>) {
+    cx.defer_in(window, |list_state, window, cx| {
+        // list_state IS locked for this entire callback!
+        parent.update(cx, |this, cx| {
+            this.list.update(cx, |_, _| {}); // PANIC — re-enters ListState lock
+        });
+    });
+}
+```
 
-entity2.update(cx, |state2, cx| {
-    // Update entity2
-    state2.value = 100;
-    cx.notify();
-});
+**Fix:** Use the direct `&mut` reference the callback provides instead of going through the entity handle:
+
+```rust
+// ✅ Correct: direct mutable access — no lock needed
+fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context<ListState<Self>>) {
+    cx.defer_in(window, |list_state, window, cx| {
+        // Step 1: call hooks directly through list_state (no entity lock)
+        list_state.delegate_mut().on_will_change(&mut op, &snapshot);
+
+        // Step 2: update the parent entity — different lock, safe
+        let new_sel = parent.update(cx, |this, cx| {
+            this.state.apply_change(op);
+            cx.notify();
+            this.state.selection.clone() // return data needed for step 3
+        });
+
+        // Step 3: sync list state directly — no entity lock
+        if let Ok(sel) = new_sel {
+            list_state.delegate_mut().update_snapshot(sel.clone());
+            list_state.delegate_mut().on_confirm(&sel);
+        }
+    });
+}
+```
+
+### Render Callbacks Must Not Access External Entities
+
+`render_item` and any other rendering hook runs inside the entity's render pass. Calling `entity.read(cx)` or `entity.update(cx, …)` on an external entity (which may itself be in a render/update pass) panics with the same re-entrancy error.
+
+**Fix:** Keep a plain `snapshot` field updated eagerly from outside render after every mutation:
+
+```rust
+// ❌ Panic — called during ListState render; external entity access re-enters
+fn render_item(&mut self, ix: IndexPath, …) -> … {
+    let checked = parent.read(cx).selection.contains(&ix); // PANIC
+}
+
+// ✅ Read from snapshot field — no entity access at all
+fn render_item(&mut self, ix: IndexPath, …) -> … {
+    let checked = self.selection_snapshot.iter().any(|(sel_ix, _)| sel_ix == &ix);
+}
+// After every mutation from outside render:
+list.update(cx, |l, _| l.delegate_mut().update_snapshot(new_snapshot));
 ```
 
 ### Use Weak References in Closures

.claude/skills/gpui-test/SKILL.md

@@ -91,4 +91,5 @@ fn test_with_window(cx: &mut TestAppContext) {
 ## Additional Resources
 
 - For detailed testing patterns and examples, see [reference.md](reference.md)
+  - Includes **"Testing Crash-Free State Management (Re-entrancy)"** — patterns for catching entity re-entrancy panics (`cannot update … while it is already being updated`) before they reach users.
 - For best practices and running tests, see [examples.md](examples.md)

.claude/skills/gpui-test/reference.md

@@ -198,6 +198,125 @@ async fn test_external_io(cx: &mut TestAppContext) {
 }
 ```
 
+## Testing Crash-Free State Management (Re-entrancy)
+
+The most dangerous class of GPUI bugs is **entity re-entrancy**: code that tries to update or read an entity while it is already locked in a render/update pass. These bugs are invisible at compile time — they only panic at runtime, typically when the user clicks something in a list or dropdown.
+
+**Key properties of re-entrancy panics:**
+- Triggered by user interaction (click, key press), not during static rendering.
+- `#[should_panic]` only confirms the bug exists — tests must pass *without* panicking.
+- `cx.run_until_parked()` is required after the triggering action to let deferred callbacks (`defer_in`) execute.
+
+### Pattern: Drive `confirm` / `cancel` through the real delegate
+
+```rust
+#[gpui::test]
+fn test_confirm_does_not_panic(cx: &mut TestAppContext) {
+    // Build the component state the same way the real app does.
+    let state = cx.new(|cx| {
+        SelectState::new(MyDelegate::default(), None, &mut cx.window_handle(), cx)
+    });
+
+    // Simulate the user clicking the first item — this exercises on_confirm
+    // and its deferred callback, which is where re-entrancy bugs hide.
+    state.update(cx, |this, cx| {
+        this.state.list.update(cx, |list, cx| {
+            list.delegate_mut().set_selected_index(Some(IndexPath::new(0)), window, cx);
+            list.delegate_mut().confirm(false, window, cx);
+        });
+    });
+
+    // Let defer_in callbacks execute — required to trigger the crash.
+    cx.run_until_parked();
+
+    // If we reach here without panicking, the re-entrancy is fixed.
+    let selected = state.read_with(cx, |s, _| s.selected_value().cloned());
+    assert!(selected.is_some());
+}
+```
+
+### Pattern: Verify `on_will_change` and `on_confirm` hooks are called correctly
+
+Use a recording delegate to assert that hooks fire with the right arguments and in the right order:
+
+```rust
+#[derive(Default)]
+struct RecordingDelegate {
+    items: Vec<MyItem>,
+    will_change_calls: Vec<Vec<IndexPath>>,
+    confirm_calls: Vec<Vec<IndexPath>>,
+}
+
+impl SearchableListDelegate for RecordingDelegate {
+    // … required impls …
+
+    fn on_will_change(
+        &mut self,
+        change: &mut SearchableListChange<Self>,
+        _current: &[(IndexPath, Self::Item)],
+    ) {
+        self.will_change_calls.push(
+            change.select_queue.iter().map(|(ix, _)| *ix).collect()
+        );
+    }
+
+    fn on_confirm(&mut self, final_selection: &[(IndexPath, Self::Item)]) {
+        self.confirm_calls.push(
+            final_selection.iter().map(|(ix, _)| *ix).collect()
+        );
+    }
+}
+
+#[gpui::test]
+fn test_hooks_fire_in_correct_order(cx: &mut TestAppContext) {
+    let state = cx.new(|cx| SelectState::new(RecordingDelegate::with_items(3), None, window, cx));
+
+    // Simulate confirm on item 0
+    state.update(cx, |this, cx| {
+        // … trigger confirm …
+    });
+    cx.run_until_parked();
+
+    state.read_with(cx, |s, cx| {
+        let delegate = s.state.list.read(cx).delegate().delegate;
+        assert_eq!(delegate.will_change_calls.len(), 1);
+        assert_eq!(delegate.confirm_calls.len(), 1);
+        assert_eq!(delegate.confirm_calls[0], vec![IndexPath::new(0)]);
+    });
+}
+```
+
+### Pattern: Rapid multiple confirms (snapshot consistency)
+
+```rust
+#[gpui::test]
+fn test_rapid_confirms_keep_consistent_snapshot(cx: &mut TestAppContext) {
+    let state = cx.new(|cx| SelectState::new(MyDelegate::with_items(5), None, window, cx));
+
+    for i in 0..5 {
+        state.update(cx, |this, cx| {
+            // trigger confirm on item i
+        });
+        cx.run_until_parked();
+
+        state.read_with(cx, |s, cx| {
+            let snapshot = s.state.list.read(cx).delegate().selection_snapshot.clone();
+            let selection = s.state.selection.clone();
+            assert_eq!(snapshot, selection, "snapshot out of sync after confirm {i}");
+        });
+    }
+}
+```
+
+### Checklist: What to test for each component that uses `defer_in`
+
+- [ ] `confirm` path: no panic, correct final selection, snapshot matches selection
+- [ ] `cancel` path: no panic, selection unchanged, popover closed
+- [ ] `on_will_change` veto: selection not changed, `on_confirm` not called
+- [ ] `on_will_change` mutating the change: final selection reflects delegate's modification
+- [ ] Rapid consecutive confirms: each one leaves snapshot consistent with selection
+- [ ] `render_item` never panics even when called immediately after a mutation
+
 ## Property Testing
 
 Use random data to test edge cases: