Fix crdt input buffer inconsistencies with agent prompts#13002
Conversation
seemeroland
requested review from
abhishekp106 and
harryalbert
and removed request for
abhishekp106
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR changes shared-session agent prompt input handling so unfreezing no longer reinitializes the CRDT buffer, adds a display-only ephemeral empty buffer for optimistic clearing, and updates the relevant acknowledgement/failure paths and tests.
Concerns
- The display-only ephemeral can become editable before the sharer's CRDT clear is applied, so the first user edit may operate on hidden stale prompt contents and emit CRDT operations for the wrong text.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // which would generate spurious CRDT delete ops for whatever the regular | ||
| // buffer currently contains (e.g. another viewer's concurrent edits). | ||
| self.buffer_and_display_map.deactivate_ephemeral_state(); | ||
| None |
There was a problem hiding this comment.
There was a problem hiding this comment.
I tested viewer typing immediately after sending a query. It's fine - there's a brief moment the inputs are appended to the prompt, but the correct prompt is sent, and the inputs end up in the new buffer (and are consistent across partaicipants)
| /// Exits the ephemeral loading state created by `set_buffer_text_ignoring_undo` | ||
| /// without touching the CRDT buffer or emitting any `UpdatePeers` operations. | ||
| /// The editor switches back to displaying the regular collaborative buffer. | ||
| pub fn exit_ephemeral_loading_state(&mut self, ctx: &mut ViewContext<Self>) { |
There was a problem hiding this comment.
IIUC, we don't really have an 'loading' state anymore right (or at least a visual loading state)? I guess the ephemeral buffer is the loading state?
There was a problem hiding this comment.
Yea we do still have the UI of making the input gray and adding a spinner between viewer submitting prompt and receiving the ack - that's the ephemeral buffer for loading state
| /// (exited by a non-ephemeral edit), its content is discarded rather than applied | ||
| /// to the regular buffer, preventing spurious CRDT operations. | ||
| fn activate_display_only_ephemeral_state(&mut self, ctx: &mut ModelContext<EditorModel>) { | ||
| self.activate_new_ephemeral_state(ctx); |
There was a problem hiding this comment.
what happens if a viewer submits a prompt while in the ephemeral state?
There was a problem hiding this comment.
I tried and wasn't able to
| /// to retry. | ||
| pub fn unfreeze_agent_input( | ||
| &mut self, | ||
| optimistically_show_empty: bool, |
There was a problem hiding this comment.
could you maybe add a comment on this field or rename it? I read the comment above, but I'm still not entirely grokking what this field is for
3 participants
Description
The input buffer ID is determined by the active block ID. When making a bunch of agent prompts, the active block stays the same, so we keep the same input buffer ID even though submitting agent prompts clears the input.
When a viewer submitted a prompt, they would see a frozen input UI (this created a temporary buffer for this frozen input) until the server sends back a "prompt in flight" ack. On this ack, the viewer would unfreeze and clear the input.
However, this clear was done by reinitializing the buffer, which replaces it completely and does not emit a CRDT operation. The sharer on receiving the prompt request would also call unfreeze and clear input, which does not emit a CRDT operation. As a result a new viewer joining would still see the input populated, because there was no CRDT input update to clear the buffer.
The takeaway is that changes to the input buffer must by synchronized through crdt operations. The viewer now only unfreezes the input on the "prompt in flight" ack. When the sharer executes the prompt, it will ultimately go through
system_clear_buffer, which will emit a crdt operation, and all viewers will apply that. This alone fixes the issue, but the viewer who sent the prompt would see the input become unfrozen and then get cleared a moment later. To smooth out this UI, the viewer sending the prompt optimistically clears the input by displaying an ephemeral empty buffer. When a crdt operation comes in (the clear), we replace the ephemeral buffer with the real one with the crdt operation applied, so that new operations can be applied.Alternative considered
Another way to fix the issue would have been to create a new buffer ID on every agent prompt submitted (similar to on every shell command submitted). But we don't have an existing mechanism to synchronize this buffer ID, which would be more complicated to add. Block IDs are automatically synchronized because they come from pty bytes that create the block.
Testing
./script/runScreenshots / Videos
https://www.loom.com/share/cf6461aacabb4075a4e804f11b96c876
Agent Mode