perf(ai): skip live-only subscriptions for restored AI blocks#13115
perf(ai): skip live-only subscriptions for restored AI blocks#13115acarl005 wants to merge 2 commits into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This comment was marked as resolved.
This comment was marked as resolved.
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
acarl005
force-pushed
the
andy/long-convo-latency-imported-comments
branch
from
e64e970 to
9512de5
Compare
acarl005
force-pushed
the
andy/long-convo-latency-reduce-subscriptions-restored
branch
from
9e7ca1a to
f683f59
Compare
acarl005
force-pushed
the
andy/long-convo-latency-imported-comments
branch
from
9512de5 to
a513085
Compare
acarl005
force-pushed
the
andy/long-convo-latency-reduce-subscriptions-restored
branch
from
f683f59 to
5acf131
Compare
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review 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 |
There was a problem hiding this comment.
Overview
This PR reduces restored AI block overhead by moving several subscriptions behind a !model.is_restored() guard while keeping display-mode subscriptions active for restored output. No approved spec context was provided, so the review focused on the annotated diff, correctness, performance, and security/privacy behavior.
Concerns
- Restored blocks still render secrets using the current safe-mode/secret-display settings; moving
SafeModeSettingsbehind the live-only guard can leave restored secrets visible until an incidental re-render after the user enables hiding/redaction. - Restored blocks can own imported review comments, but their CWD-dependent open buttons will no longer update on
ActiveSessionEvent::UpdatedPwdonce the active session changes directories. - This is a user-facing behavioral/rendering change, but the PR description does not include screenshots or a short screen recording demonstrating restored blocks reacting correctly to the affected settings. Please attach visual evidence for the restored-conversation path.
Verdict
Found: 0 critical, 3 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
| .agent_mode_execute_read_only_commands; | ||
| } else { | ||
| me.autonomy_setting_speedbump = AutonomySettingSpeedbump::None; | ||
| } |
There was a problem hiding this comment.
SafeModeSettings controls secret redaction at render time for restored blocks too; behind this guard, enabling hide/redact secrets can leave restored block contents visible until some unrelated re-render. Keep this subscription unconditional or otherwise notify restored blocks on safe-mode changes.
| .. | ||
| } => { | ||
| *selected_option = match *settings_model | ||
| .as_ref(ctx) |
There was a problem hiding this comment.
ActiveSession subscription prevents their open-comment buttons from enabling/disabling when the user changes directories. Keep the CWD update path active for restored blocks that render imported comments.
acarl005
force-pushed
the
andy/long-convo-latency-reduce-subscriptions-restored
branch
from
cba4aba to
cad1523
Compare
acarl005
force-pushed
the
andy/long-convo-latency-imported-comments
branch
from
a513085 to
62a1c1a
Compare
acarl005
force-pushed
the
andy/long-convo-latency-reduce-subscriptions-restored
branch
from
cad1523 to
8e9c602
Compare
acarl005
force-pushed
the
andy/long-convo-latency-imported-comments
branch
from
62a1c1a to
7cb4072
Compare
1 participant
Description
Part of a stack of performance fixes for #9799.
This PR: When restoring a large conversation, each AI block subscribed to several models (AISettings autonomy/permissions, SafeModeSettings, AIExecutionProfilesModel, ActiveSession, GetRelevantFilesController, AIRequestUsageModel, CLISubagentController, ModelEventDispatcher) that are only meaningful for live, actively-streaming blocks. For 3000+ restored blocks, these subscriptions added significant overhead and spurious re-renders.
Moves all live-only subscriptions inside an
if !model.is_restored()guard.ThinkingDisplayModeandOrchestrationMessageDisplayModeremain unconditionally subscribed (moved outside the guard) since they affect how AI output is rendered for all blocks including restored ones.FontSettingsandInputModeSettingswere already unconditional.Linked Issue
Fixes #9799
ready-to-specorready-to-implement.Testing
Manually loaded a conversation with 3014 AI exchanges and verified that restored blocks still re-render correctly when thinking/orchestration display mode settings change.
./script/runAgent Mode