[3/5] Reduce duplicate remote codebase index status pushes#11093
[3/5] Reduce duplicate remote codebase index status pushes#11093moirahuang wants to merge 2 commits into
Conversation
moirahuang
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 |
| /// state again, while all three events serialize to the same remote | ||
| /// `Stale` status. This cache keeps that internal event fan-out from | ||
| /// becoming duplicate remote pushes. | ||
| last_pushed_codebase_index_statuses: HashMap<String, CodebaseIndexStatusPushKey>, |
There was a problem hiding this comment.
here i thought about if we can avoid emitting multiple events that are triggering the logs but since each of the events are important, i landed on it being better to dedupe so we only log once per change
There was a problem hiding this comment.
The main thing I think that's worth thinking through here is whether this dedupe logic should actually live in the codebase index manager layer rather than the server model
Like should we instead check if the codebase index state has changed in the manager before emitting SyncStateUpdated? That will better solve this case but prevent other unnecessary re-rendering cases when the state actually hasn't changed too
![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 codebase indexing events so remote status pushes are per-repo and deduplicated by semantic status rather than timestamp. The supplemental security pass did not identify security concerns.
Concerns
- The Code settings page still only reacts to
SyncStateUpdated, so it misses newly created local codebase indexes now that creation emitsNewIndexCreated.
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
|
|
||
| ctx.subscribe_to_model(&index_manager, |me, index, event, ctx| { | ||
| if let CodebaseIndexManagerEvent::SyncStateUpdated = event { | ||
| if let CodebaseIndexManagerEvent::SyncStateUpdated { .. } = event { |
There was a problem hiding this comment.
index_directory now emits NewIndexCreated instead of SyncStateUpdated, so this page no longer resizes/notifies when a new local codebase appears. Include NewIndexCreated { .. } here as well.
moirahuang
changed the title
moirahuang
force-pushed
the
moira/reduce-duplicate-status-pushes
branch
from
a5a05f9 to
a103d4c
Compare
moirahuang
force-pushed
the
moira/remote-codebase-indexing-telemetry
branch
from
2d7fd80 to
382a3af
Compare
|
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. |
moirahuang
force-pushed
the
moira/remote-codebase-indexing-telemetry
branch
from
382a3af to
ad36bb2
Compare
moirahuang
force-pushed
the
moira/reduce-duplicate-status-pushes
branch
from
a103d4c to
3dce8a8
Compare
moirahuang
force-pushed
the
moira/reduce-duplicate-status-pushes
branch
from
3dce8a8 to
ddfaa78
Compare
moirahuang
force-pushed
the
moira/remote-codebase-indexing-telemetry
branch
from
ad36bb2 to
2add2fe
Compare
Co-Authored-By: Oz <oz-agent@warp.dev>
moirahuang
force-pushed
the
moira/remote-codebase-indexing-telemetry
branch
from
2add2fe to
7b397d9
Compare
moirahuang
force-pushed
the
moira/reduce-duplicate-status-pushes
branch
from
ddfaa78 to
8e1987d
Compare
|
|
||
| ctx.subscribe_to_model(&index_manager, |me, index, event, ctx| { | ||
| if let CodebaseIndexManagerEvent::SyncStateUpdated = event { | ||
| if let CodebaseIndexManagerEvent::SyncStateUpdated { .. } = event { |
| /// state again, while all three events serialize to the same remote | ||
| /// `Stale` status. This cache keeps that internal event fan-out from | ||
| /// becoming duplicate remote pushes. | ||
| last_pushed_codebase_index_statuses: HashMap<String, CodebaseIndexStatusPushKey>, |
There was a problem hiding this comment.
The main thing I think that's worth thinking through here is whether this dedupe logic should actually live in the codebase index manager layer rather than the server model
Like should we instead check if the codebase index state has changed in the manager before emitting SyncStateUpdated? That will better solve this case but prevent other unnecessary re-rendering cases when the state actually hasn't changed too
2 participants
Description
Reduces duplicate codebase indexing notifications by 1) updating
push_all_codebase_index_statusesto instead only push for 1 repo aspush_codebase_index_status_update2) dedupe on the status sentTesting
Added unit tests + locally tested
./script/runScreenshots / Videos
https://www.loom.com/share/d20fe6af4a2e40acbbf0fb8f2194aeaa
Agent Mode