fix(completion): buffer-scope the GPU popup mirror; clear status on optimistic edits
Two pre-merge review findings on PR #93: 1. (High) The BufferSnapshot arm switched current_buffer_id and dropped every other buffer-local mirror but left self.completion intact -- and the producer's first-sight-closed silence means no close message ever arrives for a viewport that no longer exists, so a stale popup rendered against the new buffer's rope and kept hijacking Esc/RET/TAB. Fixed three-deep: the snapshot arm clears the mirror; CompletionLocal now carries its buffer_id; and the shared completion_open_for_current_buffer() predicate gates both the key routing and the anchor mapping, so a foreign-buffer popup can neither paint nor steal keys even if a stale mirror survives by some other path. Regression: completion_popup_is_scoped_to_its_buffer. 2. (Medium) Optimistic typing (the CrdtOp path, the bulk of GPU keystrokes) never cleared core.status, so once v15 shipped the transient message over StatusFacts, '12 references' stayed wedged in the GPU band through ordinary typing -- only a round-tripped key's dispatch_key entry clear released it. handle_remote_crdt_op now clears the status when an edit applies, mirroring dispatch_key. Regression: handle_remote_crdt_op_clears_the_transient_status. Also checked: the a_closed_outbox_shuts_the_socket_down... hang seen once during review did not reproduce in 10 isolated runs -- a pre-existing timing flake in the F-008 shutdown test, untouched here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
05c6519649
commit
34188c0528
|
|
@ -749,6 +749,12 @@ struct MinibufferLocal {
|
|||
/// accept round-trip into the daemon's completion shadow.
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
struct CompletionLocal {
|
||||
/// Buffer the popup targets. Rendering and the key gates check
|
||||
/// this against `current_buffer_id` so a popup can never act
|
||||
/// against a buffer it wasn't opened in (buffer switches also
|
||||
/// clear the whole mirror at the `BufferSnapshot` arm; this is
|
||||
/// the belt to that suspender).
|
||||
buffer_id: BufferId,
|
||||
/// Byte offset of the prefix start.
|
||||
anchor: u64,
|
||||
/// Bytes of typed prefix at `anchor` (reserved for a bolded-
|
||||
|
|
@ -927,7 +933,7 @@ impl ApplicationHandler<AppEvent> for App {
|
|||
let completion_open = self
|
||||
.state
|
||||
.as_ref()
|
||||
.is_some_and(|state| state.completion.is_some());
|
||||
.is_some_and(State::completion_open_for_current_buffer);
|
||||
|
||||
// Escape cancels an active intercept (e.g. a running
|
||||
// search) or dismisses the completion popup; otherwise it
|
||||
|
|
@ -2428,6 +2434,14 @@ impl State {
|
|||
// next PresenceUpdate / CursorByte arrives.
|
||||
self.peer_presences.clear();
|
||||
self.own_cursor = None;
|
||||
// The completion popup too (Arc 1a): its anchor is a
|
||||
// byte in the prior buffer, and the producer never
|
||||
// ships a close for a viewport that no longer exists
|
||||
// (first-sight of the new buffer stays silent) — a
|
||||
// retained popup would render against the new rope AND
|
||||
// keep hijacking Esc/RET/TAB. The daemon-side session
|
||||
// was already invalidated by the switch.
|
||||
self.completion = None;
|
||||
self.cursor_fresh = false;
|
||||
self.optimistic_cursor_floor = None;
|
||||
self.optimistic_floor_set_at = None;
|
||||
|
|
@ -2864,6 +2878,7 @@ impl State {
|
|||
return None;
|
||||
}
|
||||
self.completion = Some(CompletionLocal {
|
||||
buffer_id,
|
||||
anchor,
|
||||
prefix_len,
|
||||
rows,
|
||||
|
|
@ -2877,6 +2892,16 @@ impl State {
|
|||
}
|
||||
}
|
||||
|
||||
/// True while the completion popup is open **for the buffer this
|
||||
/// window currently shows** — the predicate the key gates (Esc,
|
||||
/// RET/TAB) and the render path share, so a stale mirror can
|
||||
/// never act against a foreign buffer.
|
||||
fn completion_open_for_current_buffer(&self) -> bool {
|
||||
self.completion
|
||||
.as_ref()
|
||||
.is_some_and(|c| Some(c.buffer_id) == self.current_buffer_id)
|
||||
}
|
||||
|
||||
/// A `ViewportSend` for the current `view_range` if it differs from
|
||||
/// the last one declared, else `None` (Q#S5 coalescing). `generation`
|
||||
/// is 0 — the producer's full-resync triggers on the visible-range
|
||||
|
|
@ -3703,6 +3728,9 @@ impl State {
|
|||
/// scrolled out of the visible slice (the popup then simply
|
||||
/// doesn't draw this frame; scrolling back restores it).
|
||||
fn completion_anchor_px(&self) -> Option<(f32, f32, f32)> {
|
||||
if !self.completion_open_for_current_buffer() {
|
||||
return None; // never paint against a foreign buffer's rope
|
||||
}
|
||||
let comp = self.completion.as_ref()?;
|
||||
let (vstart, vend) = self.view_range;
|
||||
if vend <= vstart {
|
||||
|
|
@ -7822,6 +7850,50 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn completion_popup_is_scoped_to_its_buffer() {
|
||||
// Buffer-switch regression (PR #93 validation finding 1): a
|
||||
// retained popup mirror must be inert — no key gating, no
|
||||
// anchor mapping — the moment `current_buffer_id` differs
|
||||
// from the popup's buffer.
|
||||
let Some(mut state) = headless_or_skip(320, 240, "hello_world he") else {
|
||||
return;
|
||||
};
|
||||
let own = BufferId::next();
|
||||
let other = BufferId::next();
|
||||
state.current_buffer_id = Some(own);
|
||||
// Headless states never declare a viewport; anchor mapping
|
||||
// reads `view_range`, so pin it to the whole text.
|
||||
state.view_range = (0, state.current_text.len() as u64);
|
||||
state.completion = Some(CompletionLocal {
|
||||
buffer_id: own,
|
||||
anchor: 12,
|
||||
prefix_len: 2,
|
||||
rows: vec![CompletionPopupRow {
|
||||
label: "hello_world".into(),
|
||||
kind: 3,
|
||||
detail: None,
|
||||
}],
|
||||
selected: Some(0),
|
||||
total: 1,
|
||||
});
|
||||
assert!(state.completion_open_for_current_buffer());
|
||||
assert!(
|
||||
state.completion_anchor_px().is_some(),
|
||||
"the popup anchors in its own buffer"
|
||||
);
|
||||
// The window switches buffers; the mirror is stale.
|
||||
state.current_buffer_id = Some(other);
|
||||
assert!(
|
||||
!state.completion_open_for_current_buffer(),
|
||||
"a foreign-buffer popup must not gate keys"
|
||||
);
|
||||
assert!(
|
||||
state.completion_anchor_px().is_none(),
|
||||
"a foreign-buffer popup must not paint"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn headless_text_changes_the_rendered_frame() {
|
||||
let Some(mut empty) = headless_or_skip(320, 240, "") else {
|
||||
|
|
|
|||
|
|
@ -1877,6 +1877,13 @@ fn handle_remote_crdt_op(
|
|||
// notify but the op still needs broadcasting (F17).
|
||||
if let Some(edit) = edit_opt.as_ref() {
|
||||
let mut core = editor.core.borrow_mut();
|
||||
// Transient status messages clear on user input. The Key path
|
||||
// gets this from `dispatch_key`'s entry clear; the optimistic
|
||||
// path routes plain typing here instead, and since v15 ships
|
||||
// `core.status` over `StatusFacts`, a stale "12 references"
|
||||
// would otherwise stay wedged in a semantic frontend's band
|
||||
// through ordinary typing.
|
||||
core.status.clear();
|
||||
let post_edit_cursor = edit.range.start + edit.inserted_len;
|
||||
|
||||
// Identify source's active window id (so we can skip it
|
||||
|
|
@ -2342,6 +2349,63 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
/// v15 regression: an optimistic-path edit (the bulk of plain-char
|
||||
/// typing from a semantic frontend) must clear the transient
|
||||
/// status message, exactly as `dispatch_key`'s entry clear does
|
||||
/// for round-tripped keys — otherwise "12 references" stays
|
||||
/// wedged in the GPU band (which renders `StatusFacts.message`)
|
||||
/// through ordinary typing.
|
||||
#[cfg(feature = "crdt")]
|
||||
#[test]
|
||||
fn handle_remote_crdt_op_clears_the_transient_status() {
|
||||
use crate::editor::EditorState;
|
||||
use crate::protocol::FrontendId;
|
||||
|
||||
let mut editor = EditorState::new();
|
||||
let buffer_id = editor.core.borrow().active_window().buffer_id;
|
||||
{
|
||||
let core = editor.core.borrow();
|
||||
let mut reg = core.registry.borrow_mut();
|
||||
reg.get_mut(buffer_id)
|
||||
.expect("active buffer")
|
||||
.upgrade_to_crdt(2)
|
||||
.expect("upgrade to crdt");
|
||||
}
|
||||
let snapshot_bytes = {
|
||||
let core = editor.core.borrow();
|
||||
let reg = core.registry.borrow();
|
||||
reg.get(buffer_id)
|
||||
.expect("active buffer")
|
||||
.crdt_state()
|
||||
.expect("crdt-backed")
|
||||
.export_snapshot()
|
||||
.expect("export snapshot")
|
||||
};
|
||||
let peer = loro::LoroDoc::new();
|
||||
peer.set_peer_id(99).expect("set peer id");
|
||||
peer.import(&snapshot_bytes).expect("import snapshot");
|
||||
let v_before = peer.oplog_vv();
|
||||
peer.get_text("body").insert(0, "x").expect("peer insert");
|
||||
let op_bytes = peer
|
||||
.export(loro::ExportMode::updates(&v_before))
|
||||
.expect("export op");
|
||||
|
||||
editor.core.borrow_mut().status = "12 references".to_owned();
|
||||
super::handle_remote_crdt_op(
|
||||
&mut editor,
|
||||
FrontendId(99),
|
||||
buffer_id,
|
||||
crate::rope::CrdtOp {
|
||||
peer_id: 99,
|
||||
bytes: op_bytes,
|
||||
},
|
||||
);
|
||||
assert!(
|
||||
editor.core.borrow().status.is_empty(),
|
||||
"an optimistic-path edit must clear the transient status"
|
||||
);
|
||||
}
|
||||
|
||||
/// Session B1 regression: a `Key` event from a *semantic*
|
||||
/// (grid-less) frontend must reach the editor core. Before B1 the
|
||||
/// dispatcher's catch-all only called `apply_event` when the
|
||||
|
|
|
|||
Loading…
Reference in New Issue