Remove NotAmbientAgent state from AmbientAgentViewModel.#9310
Conversation
|
I'm starting a first review of this pull request. You can follow along in the session on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
zachbai
force-pushed
the
zb/refactor-ambient-agent-view-model
branch
from
60d86ff to
e39f7e7
Compare
zachbai
force-pushed
the
zb/refactor-ambient-agent-view-model
branch
from
e39f7e7 to
cc6e49d
Compare
![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 refactors AmbientAgentViewModel so it is only constructed for cloud-mode terminal views, removes the NotAmbientAgent sentinel state, and updates terminal/input/status UI call sites to treat the ambient model as optional.
Concerns
- Ambient shared-session viewers created through the normal shared-session viewer path can now be constructed without an AmbientAgentViewModel, so the join handler cannot enter viewing-existing-session state for ambient sessions.
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
| let sizes = compute_block_size(initial_size, ctx); | ||
|
|
||
| let model = if is_deferring_terminal_session_connection { | ||
| let model = if is_cloud_mode { |
There was a problem hiding this comment.
new_internal(..., false, ...), including restored/opened ambient sessions that only learn SessionSourceType::AmbientAgent after joining. Those sessions now create a normal TerminalModel and no AmbientAgentViewModel, so JoinedSuccessfully silently skips enter_viewing_existing_session and cloud-mode UI/keymap/details state stays disabled; thread is_cloud_mode = true for known ambient-session opens or create the ambient model when the ambient source type arrives.
| return Err(ExitAgentViewError::AmbientAgent); | ||
| } | ||
| // But if there is a terminal backing and we're in a LRC, we should be able to escape | ||
| else if is_fullscreen_with_long_running { |
There was a problem hiding this comment.
I think we still want this early return, no?
2 participants
Description
Refactor AmbientAgentViewModel to remove NotAmbientAgent state and simplify semantics - presence of the model indicates that the parent view is rendering an ambient agent task.