Fix AI blocks not appearing due to conversation existing in more than one pane#11494
Conversation
seemeroland
changed the title
|
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 AI conversation restoration and agent-view entry paths so a conversation is transferred to a single owning pane, removes stale rendered blocks from previous owners, and adds tests for restored/duplicated conversation flows.
Concerns
- Restoring a conversation now removes it from the previous pane's live conversation list even though that pane may still own in-flight requested commands that resolve their conversation via that per-pane live mapping.
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
| self.view_id, | ||
| ctx, | ||
| ); | ||
| history_model.set_active_conversation_id(active_conversation_id, self.view_id, ctx); |
There was a problem hiding this comment.
action_id via conversation_id_for_action(action_id, previous_view_id), so those actions can lose their conversation after restore. Keep a non-rendering action mapping for the previous pane or defer removal until pending actions finish.
There was a problem hiding this comment.
It should not be possible to transfer the conversation to a new pane while it's still in progress - entrypoints should be navigating to where it lives.
There was a problem hiding this comment.
note that it's possible to open a conversation in a "new" pane while it's in progress but we're NOT actually "transferring" the conversation, we're just changing where/how we're rendering the terminal view i.e. it's always in the same pane but it could be same pane switch, new pane split or new tab split.
Tested this manually and confirmed it still works as expected!
advait-m
left a comment
advait-m
left a comment
There was a problem hiding this comment.
I tried this out myself manually with in-progress convos + finished convos and opening in new pane/tab + closing for orchestration. Seemed to all work as expected!
| self.restore_conversation_after_view_creation( | ||
| RestoredAIConversation::new(conversation), | ||
| false, | ||
| ctx, | ||
| ); | ||
| if let Err(e) = self.try_enter_agent_view( |
There was a problem hiding this comment.
nit: I think the restore call enters agent view as a side effect, so the follow-up entry sees "was already in agent view = true" when it shouldn't? For origins that auto-trigger only when already in agent view, the initial prompt could auto-submit.
potential fix: capture "was in agent view" before the restore call, or split the restore so it doesn't implicitly enter agent view and enter once after (matching the live branch)
trace:
restore_conversations_from_block_paramsai_context_model.set_pending_query_state_for_existing_conversationagent_view_controller.try_enter_agent_view
There was a problem hiding this comment.
This was the fix to restoring in the active pane, but agree this function should always restore and callers should do the liveness check before calling this.
Moving this fix within restore_conversation_in_active_pane
There was a problem hiding this comment.
Actually that doesn't address the trace you mentioned. I'm adding an arg passed down the restoration path to not enter the agent view if the caller is going to enter the agent view afterward with a different origin. Seems like this occurs in several other places
|
That issue happened when running two parallel conversations in split panes? I don't see why that should be an issue since they should be different conversation ids, unless it was a workaround around a deeper issue with the same conversation actually existing in both panes. |
4 participants
Description
Fix queries and output not showing up due to a conversation existing in more than one pane. A conversation and its blocks should only be in one pane at a time. This PR enforces this by notifying when a conversation becomes active in a new terminal view, so that old terminal views can delete the AI and command blocks for that conversation.
https://github.com/warpdotdev/warp/pull/10241/changes#diff-fe3dd831b48d66196f87f13cd7a57f0e3660444b8f4fd9f6a5718be5fa617e40 introduced
mark_active_conversation_idto not transfer ownership. #10327 uses it in the restoration path. The first PR mentions it's short term, and @advait-m mentioned these original fixes are probably no longer needed, but would like review from you and @szgupta to verify. My understanding is that a conversation must never be in more than one terminal view, even with orchestration, so it should be correct to enforce this when setting active conversation id.I'm also unsure what should be the source of truth for where the conversation lives. I'm using the ai history model for this, but it seems like the agent view controller also has this info.
Testing
./script/runScreenshots / Videos
https://www.loom.com/share/e0af242e4f2f42dcbe77b1753dca5177
Changelog Entries for Stable
CHANGELOG-BUG-FIX: Fixed an issue that could prevent an AI query and output from showing up