Speed up loading long conversations#13098
Conversation
|
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 targets long-conversation restore latency by adding an exchange-id index, reducing restored AI-block subscriptions, and bulk-inserting restored command blocks.
Concerns
- The new exchange-id index is not invalidated on all mutation paths, so lookups can miss or panic after exchange IDs change or tasks are removed.
- Restored AI blocks no longer subscribe to settings changes that their render path reads, so toggling thinking/orchestration display modes may leave historical blocks stale.
- Restored command blocks no longer disable reset-grid checks before parsing serialized command/output, which can reintroduce Windows ConPTY reset-grid assertions.
- This is a user-facing performance/restore behavior change, but the PR description does not include manual testing evidence or screenshots/screen recording demonstrating the long-conversation restore path end to end.
Verdict
Found: 0 critical, 4 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
| } | ||
|
|
||
| /// Returns the exchange with the given ID in O(1) time. | ||
| pub fn exchange_by_id(&self, exchange_id: AIAgentExchangeId) -> Option<&AIAgentExchange> { |
There was a problem hiding this comment.
modify_task (used by reassign_exchange_ids) can change exchange IDs without changing the exchange count, and remove does not rebuild it. Rebuild or invalidate this map for every task/exchange mutation before using it for exchange_by_id.
| } | ||
| ctx.notify(); | ||
| } | ||
| AISettingsChangedEvent::ThinkingDisplayMode { .. } |
There was a problem hiding this comment.
output::render reads AISettings::thinking_display_mode, and orchestration message collapse state also depends on AI settings. Keeping this subscription inside the live-only branch means restored historical blocks can stay stale after the setting changes.
| self.active_block_mut().start_background(None); | ||
| } else { | ||
| self.active_block_mut().start(); | ||
| self.active_block_mut().disable_reset_grid_checks(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
This isn't needed. Callers that pass BootstrapStage::RestoreBlocks already get PerformResetGridChecks::No from Block::new (because RestoreBlocks.is_done() == false), so there is no Windows ConPTY reset-grid assertion risk during parse_bytes.
acarl005
force-pushed
the
andy/fix-restore-long-convo-latency
branch
from
d685745 to
2cce665
Compare
acarl005
force-pushed
the
andy/fix-restore-long-convo-latency
branch
from
2cce665 to
3078048
Compare
1 participant
Description
Linked Issue
ready-to-specorready-to-implement.Testing
./script/runScreenshots / Videos
Agent Mode