Add RemoteDiffStateModel#10489
Conversation
MaggieShan
changed the base branch from
master
to
maggs/hook-diffstatemodel-to-server
MaggieShan
force-pushed
the
maggs/hook-diffstatemodel-to-server
branch
7 times, most recently
from
5b7dab4 to
f90b9ce
Compare
MaggieShan
force-pushed
the
maggs/add-remotediffstatemodel
branch
from
6ddb274 to
2a27a41
Compare
MaggieShan
force-pushed
the
maggs/hook-diffstatemodel-to-server
branch
from
b518a57 to
440c76e
Compare
MaggieShan
force-pushed
the
maggs/add-remotediffstatemodel
branch
5 times, most recently
from
4a6ae9a to
c0d25df
Compare
|
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 wires remote diff state snapshots, metadata updates, file deltas, and discard/unsubscribe requests through the remote client and manager so the code review view can operate against remote repositories.
Concerns
- Remote reconnects can leave
RemoteDiffStateModelstuck on a stale subscription because reconnectable disconnects emitHostDisconnectedbefore anySessionDisconnectedclearssession_id, then theHostConnectedhandler refuses to re-subscribe. - The change affects user-visible remote code review behavior, but the PR only notes
./script/runand includes no screenshot, screen recording, or justification for why visual/manual evidence is not possible.
Verdict
Found: 0 critical, 2 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
MaggieShan
force-pushed
the
maggs/add-remotediffstatemodel
branch
from
4625580 to
cdf34d2
Compare
| /// The session through which the current server-side subscription | ||
| /// was established. Used to detect subscription loss when a specific | ||
| /// session disconnects while other sessions to the same host survive. | ||
| session_id: Option<SessionId>, |
There was a problem hiding this comment.
Hmmm why do we hold an Optional SessionId here given we already have the HostId? What does it mean when this is None?
There was a problem hiding this comment.
As discussed offline - the SessionId is still needed as we're also tracking ConnectionId on the server to handle which subscribers to fan events out to. Instead, I'll update the logic such that SessionId is required for a RemoteDiffStateModel
| repo_path, | ||
| mode, | ||
| .. | ||
| } => Some((host_id, repo_path, mode)), |
There was a problem hiding this comment.
RemoteServerManager should be responsible for converting from proto type to actual in app type
There was a problem hiding this comment.
As discussed offline - the issue is that a lot of these proto types are defined in app/ which the RemoteServerManager doesn't have access to since its in the remote-server crate
MaggieShan
force-pushed
the
maggs/add-remotediffstatemodel
branch
from
905c6f7 to
78685f2
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 adds the remote-backed diff state model, remote server diff-state request/response wiring, and code review handling for remote diff lifecycle events.
Concerns
- Remote diff models are removed on disconnect, so the reconnect handler cannot find the existing model to resubscribe and active code review views can remain stuck on stale disconnected state.
- This changes user-facing remote code-review behavior, but the PR description does not include screenshots or a screen recording demonstrating the remote diff/code review flow end to end.
Verdict
Found: 0 critical, 2 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
| /// to `get_or_create_diff_state_model` lazily creates a fresh one bound | ||
| /// to a currently-connected session. | ||
| fn mark_and_remove_remote_model(&mut self, key: &FileLocation, ctx: &mut ModelContext<Self>) { | ||
| let Some(model) = self.diff_state_models.remove(key) else { |
There was a problem hiding this comment.
SessionReconnected from finding it to resubscribe, so existing code review views keep a stale handle in Disconnected state until the view is recreated. Keep the model cached and mark it disconnected, or explicitly recreate/rebind active views on reconnect.
MaggieShan
force-pushed
the
maggs/add-remotediffstatemodel
branch
from
c714e38 to
706678d
Compare
## Description * Built on warpdotdev#10470 * Adds client-side wiring to support git states and the code review view in remote environments * Moves shared types out of `LocalDiffStateModel` * Updates `RemoteDiffStateModel` with real logic to handle and emit the server events * Update `RemoteServerManager` to handle server events and send client events * Note: UI for reconnection is still tbd ## Linked Issue [APP-4356](https://linear.app/warpdotdev/issue/APP-4356/create-remotediffstatemodel-for-the-client) ## Testing Should be no-op. Confirmed all unit tests are passing as expected. - [x] I have manually tested my changes locally with `./script/run` ## Agent Mode - [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode
2 participants
Description
DiffStateModelto remote server #10470LocalDiffStateModelRemoteDiffStateModelwith real logic to handle and emit the server eventsRemoteServerManagerto handle server events and send client eventsLinked Issue
APP-4356
Testing
Should be no-op. Confirmed all unit tests are passing as expected.
./script/runAgent Mode