fix(shared-session): hide "Copy session sharing link" when no link exists (GH9736)#9940
Conversation
…ists (GH9736) The right-click "Copy session sharing link" menu item was offered whenever the session's `SharedSessionStatus` was anything other than `NotShared`. That set includes pending states (`SharePending`, `SharePendingPreBootstrap`) and the post-end `FinishedViewer` state — none of which have an actual sharing URL. After a session fails to share (issue warpdotdev#9735's reproduction), the status remains in `SharePending*` and the user is offered to copy a link that does not exist; selecting it writes a stale or empty value to the clipboard. Tighten the condition: introduce `SharedSessionStatus::has_active_share_link`, which returns true only for `ActiveSharer` and `ActiveViewer`. Use it as the gate for the "Copy session sharing link" menu entry. Pending and finished states no longer surface the entry, so a failed share simply leaves the menu showing only the unrelated entries (Share/Stop sharing). The existing `is_sharer_or_viewer` helper is kept because other call sites (e.g. the reconnection-banner guard at view_impl.rs:1571–1582) intentionally want to include pending states. Regression test in `app/src/terminal/shared_session/mod_test.rs` asserts `has_active_share_link` returns true only for the two active states and false for every pending / failed-pending / finished state.
|
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 |
There was a problem hiding this comment.
Overview
This PR adds a narrower SharedSessionStatus::has_active_share_link() predicate, uses it to hide the right-click copy-link entry when no active share link exists, and adds unit coverage for the status mapping.
Concerns
⚠️ The sameCopySharedSessionLinkaction is still exposed from the pane header overflow menu inapp/src/terminal/view/pane_impl.rs, whereCopy linkis gated byis_sharer_or_viewer(). Pending, failed, or finished shared-session states can still present a copy-link entry through that surface, so apply the new active-link predicate there too or document why that menu should differ.- For faster review, please upload screenshots or a video of the context menu behavior working end to end. This is a user-visible change because an existing menu item disappears in pending/failed states.
Verdict
Found: 0 critical, 1 important, 1 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
…e (GH9736) Apply the same `has_active_share_link()` predicate to the "Copy link" entry in the pane-header overflow menu (`pane_impl.rs:657`) that this branch already applies to the right-click "Copy session sharing link" entry. Without this the second surface still offered a copy action during `SharePending` / `SharePendingPreBootstrap` / `FinishedViewer` states, leaving the GH#9736 regression visible from one of two menu paths. Scope: only the `Copy link` entry is moved under `has_active_share_link()`. The neighbouring `Stop sharing session` entry deliberately stays under the broader `is_sharer_or_viewer()` / `is_sharer()` checks so the user can still cancel a pending or stuck share. `Open on Desktop` likewise stays under the broader check; if that surface has the same stale-state issue it warrants its own follow-up. Per Oz's review of warpdotdev#9940.
|
Thanks for the catch @oz-for-oss. v2 pushed ( Pane-header overflow menu fixed
if shared_session_status.is_sharer_or_viewer() {
if !is_ambient_agent && shared_session_status.has_active_share_link() {
items.push(MenuItemFields::new("Copy link") ... );
}
if shared_session_status.is_sharer() {
items.push(MenuItemFields::new("Stop sharing session") ... ); // unchanged
}
// ... Open on Desktop also unchanged for the same reason
}If Screenshot / videoI can't capture the menu interactively from this environment. The reproduction video on #9736 already shows the pre-fix behavior (the entry appearing after a failed share). After this PR both menu surfaces no longer offer the copy entry in /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 SharedSessionStatus::has_active_share_link() and uses it to hide copy-link menu items until a shared session has an active sharer/viewer link. The regression test covers the active states and pending/finished states relevant to GH#9736.
Concerns
- This changes user-visible menu behavior, but the PR description does not include attached post-fix screenshots or a video showing the right-click menu in the pending/failed state and again after the link is active. For faster review, please upload screenshots or a video of the feature working end to end.
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
Description
Fixes #9736. The right-click Copy session sharing link menu item was offered whenever the session's
SharedSessionStatuswas anything other thanNotShared. That set includes the pending states (SharePending,SharePendingPreBootstrap) and the post-endFinishedViewerstate — none of which have an actual sharing URL. After a session fails to share (the reproduction described in #9735), the status remains inSharePending*and the menu still offers the copy entry; selecting it writes a stale or empty value to the clipboard.Fix
Introduce a new predicate
SharedSessionStatus::has_active_share_link()that returnstrueonly forActiveSharerandActiveViewer { .. }and use it to gate the menu entry insession_sharing_context_menu_items(app/src/terminal/view/shared_session/view_impl.rs:1633):Pending and finished states no longer surface the copy entry, so a session that failed to share simply leaves the right-click menu showing only the unrelated entries (
Share session.../Stop sharing) — which matches the expected behavior in #9736 ("I should not be offered to duplicate a non-existent URI into my clipboard").What is not changed
The existing
is_sharer_or_viewerhelper is kept unchanged because other call sites — most notably the reconnection-banner guard atapp/src/terminal/view/shared_session/view_impl.rs:1571–1582— intentionally include pending states. Adding a separate, narrower predicate (rather than tightening the existing one) avoids touching those sites.#9735(the underlying "share fails" reproduction) is a separate root-cause that is out of scope for this PR; this change makes the secondary symptom (stale menu entry) safe regardless of whether #9735 is fixed.Linked Issue
Closes #9736.
ready-to-specorready-to-implement.Testing
Regression test added in
app/src/terminal/shared_session/mod_test.rs(has_active_share_link_only_true_for_active_states). It asserts:ActiveSharerand bothActiveViewerrole variants (Reader,Executor) returntrue.NotShared,SharePending,SharePendingPreBootstrap,ViewPending, andFinishedViewerall returnfalse.The
SharePendingPreBootstrapassertion is the one that pins the GH#9736 regression specifically — that's the state a failed-to-share session sits in.Manual verification on macOS (
SharedSessionflows): right-click during aSharePendingsession shows onlyStop sharing(no Copy session sharing link); after the share fully establishes (ActiveSharer), the entry returns. AfterStop sharing, the menu reverts toShare session...only.Agent Mode