Decouple DiffStateModel and CodeReviewView#10314
Conversation
kevinyang372
left a comment
kevinyang372
left a comment
There was a problem hiding this comment.
This looks good! I didn't review very closely given this is mostly a refactor. Lmk where there are major logic changes you want me to take a closer look at
| // After the view state is refreshed with fresh diffs, re-evaluate | ||
| // the git operations button (Commit / Push / Create PR) so that | ||
| // e.g. committing shows "Push" instead of staying on "Commit". | ||
| DiffStateModelEvent::MetadataRefreshed => { |
There was a problem hiding this comment.
Instead of having a separate MetadataRefreshed event (which goes back to look it up in DiffStateModel), can we bundle the aggregated diff stats update into DiffStateModelEvent::SingleFileUpdated and DiffStateModelEvent::NewDiffsComputed?
There was a problem hiding this comment.
I think there are still cases when MetadataRefreshed would fire outside of both DiffStateModelEvent::SingleFileUpdated and DiffStateModelEvent::NewDiffsComputed that we still need to cover - to avoid the roundtrip we can instead pass the updated metadata with the event itself?
| impl SyncQueueTaskTrait for FileInvalidationTask { | ||
| type Error = FileInvalidationError; | ||
| type Result = (PathBuf, Option<FileDiffAndContent>); | ||
| type Result = (PathBuf, Option<Arc<FileDiffAndContent>>); |
There was a problem hiding this comment.
Why do we need to wrap this in Arc now?
There was a problem hiding this comment.
The main change is in https://github.com/warpdotdev/warp/pull/10314/changes#diff-2fcff7c4f4f049e80ea75516a1e9b2a59974bc492260ebf01661419d372eb022R493-R503
On master, the Arc::try_unwrap failure triggered a full reload but I've changed it to instead borrow + clone the value and since FileDiffAndContent is an expensive clone, I wrapped it with an Arc
|
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 moves diff invalidation orchestration from CodeReviewView into DiffStateModel and changes the view to consume model events for full and single-file diff updates.
Concerns
- The new index-lock handling treats every
index_lock_detectedevent as a reason to keep suppressing the per-file queue, but the repository watcher sets that flag for both.git/index.lockcreation and removal. This can leaveinvalidate_all_pendingstuck after the lock clears. - Manual testing is required for behavior that can be manually tested. Please include screenshots or a screen recording showing the code review pane working end to end across the tested repo/branch/file-change scenarios, or justify why visual evidence is not possible.
Verdict
Found: 0 critical, 1 important, 1 suggestion
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
MaggieShan
force-pushed
the
maggs/decouple-diffstatemodel
branch
from
662eafa to
2d93c80
Compare
Addressed your pr comments in ad0500a! The main logic change I'll call out is in handle_file_update which converts |
2 participants
Description
DiffStateModelto have one repo #10283DiffStateModelandCodeReviewViewLinked Issue
APP-4352
Testing
Verified code review pane locally --> should be a no-op
Agent Mode