Expose shared session inactivity settings in Settings - #14954
Draft
warp-agent-staging[bot] wants to merge 12 commits into
Draft
Expose shared session inactivity settings in Settings#14954warp-agent-staging[bot] wants to merge 12 commits into
warp-agent-staging[bot] wants to merge 12 commits into
Conversation
Surfaces the three existing shared-session inactivity durations (revoke edit access, warn, end session) as user-configurable settings below the existing confirm-close-shared-session toggle. Values are edited in minutes and are validated so revoke <= warn <= end always holds (editing one field clamps it against the other two's current values). No inactivity timeout behavior, defaults, or the sharer inactivity ladder logic in view_impl.rs change. The three settings move from private to public (private: false) with a toml_path and description, matching the convention used by other user-facing settings (e.g. ShouldConfirmCloseSession), so they persist via the normal settings pipeline and take effect on the running session. APP-5313 Co-Authored-By: Warp Agent <agent@warp.dev>
…split Adversarial review of #14954 surfaced five findings; all addressed here: 1. Legacy private-store values for the three inactivity settings are now migrated into their new public location via a dedicated one-time migration (its own completion marker, independent of the general SettingsFileMigrationComplete marker which is already set for existing SettingsFile users). Only copies when the public location doesn't already have a value. 2. Added SharedSessionSettings::register_and_enforce_inactivity_ordering, which corrects out-of-order values at every point they become authoritative (initial load/hand-edited file, cloud sync, disk hot-reload), and changed the two derived-interval helpers in settings.rs to use saturating_sub as defense-in-depth against the two latent underflow panics found in the ladder. The ordering comparison is isolated in ladder_phase_order_ok so a future zero-disables-a-phase change only needs to touch that one predicate. 3. parse_shared_session_inactivity_minutes now rejects any value above u64::MAX / 60, preventing the *60-to-seconds conversion from overflowing. 4. Split the single SharedSessionInactivityWidget (one shared search_terms blob covering three rows) into three independent SettingsWidgets (SharedSessionRevokeEditAccessWidget / SharedSessionWarningWidget / SharedSessionEndSessionWidget), each with row-scoped search terms, each backed by its own editor field on FeaturesPageView (mirroring MouseScrollMultiplierWidget) instead of a ChildView-wrapped sub-view, since ChildView's dispatch boundary would have broken action routing for per-row widgets. This also resolves the enum_variant_names lint nonblocking comment and removes the redundant enum-variant doc comments finding, since the standalone Action enum they were attached to no longer exists. 5. N/A - see (4). Added tests: legacy-migration survival/no-clobber/idempotency, ordering correction from storage and cloud sync, a pure saturating_sub regression test, minute-parsing bounds (including the overflow case), clamp correctness/idempotency, and a StubWidget-based filter test proving the three rows are independently searchable. Verified visually with a freshly built and launched instance: all three rows render correctly, 'revoke'/'disconnect' searches now match only their own row, edits persist across Settings modal close/reopen, and out-of-order input is clamped rather than accepted. Co-Authored-By: Warp Agent <agent@warp.dev>
Per the requester's confirmed matrix (relayed via the orchestrator): setting any of the three inactivity durations to 0 disables that phase, and a phase whose end is disabled also disables the warning (a countdown to an end that never comes would be misleading). All-zero means no idle timeout at all, matching what ambient-agent shared sessions already do today. - shared_session/settings.rs: added InactivityPhase and SharedSessionSettings::next_inactivity_phase()/next_phase_after_revoke(), which compute which ladder phase should arm next (skipping disabled phases) and for how long. ladder_phase_order_ok now exempts zero from the ordering comparison in either position, so the boundary-correction pass (TOML load, cloud sync, disk hot-reload) never 'corrects' a disabled phase into something else. - view_impl.rs: reset_sharer_inactivity_timer and revoke_roles_on_inactivity_period_expired now gate on which phase is next enabled instead of unconditionally arming the next step of the ladder, via a shared arm_inactivity_timer helper. This means open_inactivity_warning_modal can now only ever be reached when both the warning and end phases are confirmed enabled, so the pre-existing zero-duration fires-instantly-on-open latent bug in the modal's countdown can no longer be hit via a disabled end. - features_page.rs: parsing now accepts 0 (previously rejected), the minutes-for-display conversion no longer floors a zero duration up to 1, and a new shared_session_inactivity_display_text renders 0 as "Off" rather than the digit 0. The three clamp_* functions now treat zero as unconditionally acceptable in every field and skip a disabled (zero) neighbor when clamping a non-zero value, instead of treating zero as the smallest legal bound. The revoke/end rows grey their label when their own value is 0; the warning row also greys out and swaps its description when the end phase being off has forced it off regardless of its own value. No other idle timeout behavior changed: the ladder's phase order, revoke/warn/end durations' defaults, and what counts as activity are all unchanged for the already-enabled case. Tests: settings_tests.rs covers the full phase matrix (all-zero, each single phase disabled, end disabling warning, ordering enforcement leaving zeros alone) plus a pure zero-exemption test on the ordering predicate; features_page_tests.rs covers zero acceptance/display and the clamp exemptions. Co-Authored-By: Warp <agent@warp.dev>
…ion-inactivity-settings
…form The three commit_shared_session_* functions only rewrote the editor's buffer text when clamping changed the numeric value. When a user typed a literal "0" and it wasn't clamped (0 is always accepted as-is), the buffer kept showing the raw digit "0" instead of being normalized to "Off" until something else (e.g. reopening Settings) forced a re-render. Always re-rendering the committed value removes that gap. Co-Authored-By: Warp <agent@warp.dev>
…egacy migration Three real bugs found by an adversarial review pass on the zero-disables-a-phase increment: 1. parse_shared_session_inactivity_minutes only accepted a numeric u64, while the display text for zero renders as "Off". Every field re-parses its own committed text on Edited/blur, so the moment a field displayed "Off" its own contents no longer parsed as valid, which is what was actually driving the red border and the post-Tab blank-frame flash observed during visual verification -- not an error-state bug in the styling itself. Fixed by having the parser accept the "Off" sentinel (case-insensitively) as zero, so display and parse agree on the same vocabulary. Also suppressed the trailing "minutes" unit label when a field is Off, since "Off minutes" read as a mistake. 2. The zero semantics only held at the moment a timer was armed, not for the rest of a live session: TerminalView never reacted to SharedSessionSettings changing while a share was already in progress. Concretely, disabling the end duration while the warning modal's zero-duration countdown was in flight could still synchronously fire TimedOut (the exact latent bug the initial gating was supposed to close, reachable via this race instead of via initial configuration), disabling the end duration while its own timer was armed didn't stop the session from still ending, and enabling a phase from an all-off ladder did nothing until the next throttled activity event. Fixed by having Sharer subscribe to the three inactivity settings and, on any change, abort the current timer, close the warning modal (via a new Sharer::close_inactivity_warning_modal that also stops the modal's own countdown -- previously only the render-gating flag was cleared, so an in-flight countdown kept ticking even once the modal was no longer shown) if the warning phase just became disabled, and re-arm from the sharer's current ladder position, tracked via a new Sharer::ladder_position (whether roles have already been revoked in this idle period or not, so re-arming doesn't re-revoke). 3. The legacy-value migration copied a stored zero straight into the new public setting. Before this increment, these settings were private with no UI to request "no timeout at all" -- a legacy zero meant an immediate timer, not a deliberate disable request. Carrying it over would silently and permanently disable that phase for a user who never asked for that, contradicting the explicit "no idle timeout is fine when the user asks for it" agreement. Fixed by skipping a legacy private value of exactly zero during migration (leaving the public key absent so the non-zero default applies); an explicit zero already present in the public location is unaffected, since that one is a real, current user choice. Tests: 3 new transition tests covering the live re-arm cases (disabling end while the warning timer is armed, disabling end while the end timer is armed, enabling a phase mid-session from an all-off ladder) and a migration regression test for a legacy private zero. Co-Authored-By: Warp <agent@warp.dev>
bnavetta
reviewed
Aug 25, 2026
bnavetta
left a comment
bnavetta
left a comment
Contributor
There was a problem hiding this comment.
@warp-factory please address my comments and provide a computer use video of the updated settings UI
Contributor
Author
|
On it, @bnavetta. Working through all 14 comments — including validating the additive-vs-cumulative question against how the timers actually consume these durations — and will follow up with a computer-use video of the updated Session settings. Responding as wilson: Open session · View in factory |
Responds to bnavetta's review on the shared-session inactivity settings PR (APP-5313). The prior revision added a large amount of machinery beyond what's needed to make the three durations user-configurable; this trims it down. - Drop the ordering-enforcement pass (register_and_enforce_inactivity_ordering, enforce_inactivity_ordering, ladder_phase_order_ok) and the legacy private-store migration entirely. Registration is back to a plain SharedSessionSettings::register(ctx) call, matching the naming convention of every other settings group. - Drop the live re-arm-on-settings-change machinery (the sharer's ladder_position tracking, its settings subscription, and TerminalView::handle_shared_session_inactivity_settings_changed). An in-flight timer or open warning modal now simply keeps running on the duration it was armed with; only a timer armed after a settings change picks up the new value. - Keep the per-field UI clamp (clamp_shared_session_*_minutes) and the zero-disables-a-phase ladder logic (InactivityPhase, next_inactivity_phase, next_phase_after_revoke, is_warning_phase_enabled): validated against master's usage (settings.rs default-value comments say "after a total of N min", and view_impl.rs derives inter-phase gaps via subtraction), these three durations are cumulative time-since-last-activity, so revoke <= warn <= end is a real invariant, not an assumption. - Replace the "Off" text sentinel with a literal "0" plus inline explanatory text next to the field when a phase is disabled (checked: the existing slider component is a continuous 0-100 control with no support for the discrete, non-linearly-labeled steps this would need, so building a bespoke slider was out of scope). - Merge the three near-identical SharedSession*Widget structs into one SharedSessionTimeoutWidget rendering all three rows. - Fix the two setting descriptions and move the features_page_tests module declaration to the bottom of the file. - Remove tests that only covered removed logic: the migration and ordering-enforcement tests in settings_tests.rs, the widget-search-terms split test in mod_tests.rs, the display-text test in features_page_tests.rs, and the live re-arm tests (and their now-unused start_sharer_session helper) in view_impl_tests.rs.
Computer-use verification of the merged SharedSessionTimeoutWidget found
that the "Warn before ending..." row's long inline off-explanation text
("Sessions won't end due to inactivity, so there's nothing to warn
about.") overflowed the row's available width and visually overlapped the
label above it. The row's label/control layout doesn't wrap the control
side, so a long unbounded inline string pushes past the container.
Shorten all three inline explanations to a consistent, short length so
they render cleanly without needing changes to the shared
render_body_item/build_toggle_element layout code (which many other
settings rows depend on). Re-verified with computer use: no overlap in
either the default or zero-disabled state.
…ion-inactivity-settings # Conflicts: # app/src/settings_view/features_page.rs
Addresses two findings from an independent review of the prior trim-down: 1. Dropping the revoke<=warn<=end ordering enforcement entirely went too far -- public TOML hot reload, cloud sync, and direct setting writes can all still persist an out-of-order triple, and saturating_sub only makes that safe, not correct (it silently collapses the gap to zero instead of producing the configured duration). Restored the enforcement, but per bnavetta's original naming-convention comment, kept it out of SharedSessionSettings::register/init.rs's register_all_settings: it's wired up as a separate SharedSessionSettings::enforce_inactivity_ordering(ctx) call in init(), alongside this file's other post-registration subscriptions. Added back file-originated and cloud-originated regression tests. 2. Removing the live re-arm-on-settings-change subscription left the ladder *partially* affected by a mid-flight settings change: an already-armed timer kept running on its original duration (as intended), but the next phase transition recomputed its gap from live (possibly changed) settings, mixing old and new durations within one idle period. Added InactivityLadderSnapshot, a frozen copy of the three durations captured once when an idle period begins (reset_sharer_inactivity_timer) and consulted by every later transition in that same period (revoke_roles_on_inactivity_period_expired, and the warning modal's own end-of-session countdown), so a single chain is now wholly old or wholly re-armed depending on which idle period it belongs to. Added a regression test covering the reviewer's exact example. Verified: cargo fmt -- --check, clippy -D warnings, cargo build, and 637 passing tests covering shared_session/settings/features_page.
…ng comments - Replace pairwise ladder_phase_order_ok with a floor-based sequence check (advance_ladder_floor) so a disabled middle phase (e.g. warn=0) can no longer let its two enabled neighbors skip comparison against each other. Previously revoke=10m, warn=0, end=5m was accepted because both pairwise comparisons ignored the zero warn, leaving end unclamped against revoke. - Add a regression test for a zero-middle triple arriving via an external (public/cloud) write, plus a focused unit test for the new floor-advance helper. - Strip revision-narrating and caller-enumerating comments introduced on this branch (init.rs, settings.rs, view_impl.rs, sharer/mod.rs) per the warp-comments skill; keep only local behavioral rationale.
- Remove advance_ladder_floor_leaves_disabled_phases_alone_but_keeps_the_floor_for_later_ones: it pinned a private implementation detail, and zero_middle_phase_does_not_let_its_two_enabled_neighbors_skip_comparison already covers the same behavior through registration and enforcement. - Collapse advance_ladder_floor's doc comment to one behavioral sentence.
Contributor
Author
|
All 14 comments are addressed and their threads resolved, and a computer-use video of the updated Session settings is in the PR description. Head is Additive vs. cumulative (r3849525801, r3849599546) — validated as cumulative, not additive. On Ordering enforcement (r3809150026) — I read that as a naming objection rather than a request to drop the enforcement. Slider vs. text (r3849552445) — the existing slider is a continuous 0–100 control (opacity/blur) that doesn't fit these ranges, so per your instruction it stayed a text input: a literal Also dropped per your comments: the legacy private-store migration, the live re-arm subscription, and the added Responding as wilson: Open session · View in factory |
Contributor
Author
|
One honest caveat on the hand-off: the That does mean CI has never exercised this PR, so the only executed evidence is local: Responding as wilson: Open session · View in factory |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Exposes the three existing shared-session (
/remote-control) inactivity durations as user-configurable settings in Settings > Features > Session, below the existing "Confirm before closing shared session" toggle, as one combined widget:inactivity_period_before_revoking_roles(default 10 min)inactivity_period_before_warning(default 25 min)inactivity_period_before_ending_session(default 30 min)Each field accepts 0 to disable that phase, and shows a short inline explanation next to the field when it's disabled (e.g. "Shared sessions will not end after an idle period."). All-zero means the session never idles out at all — an intentionally supported, uncapped configuration.
No other inactivity timeout behavior changed. Defaults are unchanged, the ladder's phase order is unchanged, and what counts as "activity" is unchanged.
Linked Issue
Linear: APP-5313
Changes
This revision responds to review feedback by trimming the original implementation down to the minimum needed to make these three durations user-configurable, correctly:
app/src/terminal/shared_session/settings.rs: the three settings move fromprivate: truetoprivate: falsewith atoml_pathand user-facingdescription. AddedInactivityLadderSnapshot, a frozen copy of the three durations that determines which ladder phase should arm next (skipping any disabled/zero phase) — validated that these three durations are cumulative time-since-last-activity, sorevoke <= warn <= endis a real invariant, not an assumption.SharedSessionSettings::enforce_inactivity_ordering(ctx)keeps that invariant on every non-UI path (hand-edited settings file, cloud sync, direct writes) by threading a running "floor" through the enabled phases in order, clamping each one up to at least the highest enabled phase before it -- this also covers a disabled middle phase (e.g.revoke=10m, warn=0, end=5m), which a simple pairwise comparison would let slip through uncorrected. It's called once frominit(), kept separate fromSharedSessionSettings::register(ctx)so registration itself stays the same plain call every other settings group uses. Dropped the legacy private-store migration entirely, since these settings had no UI before this PR, so there's no real customized value to migrate.app/src/settings_view/features_page.rs: the three rows are now rendered by a singleSharedSessionTimeoutWidgetinstead of three near-identical widgets. A duration of 0 shows literally as0(not an "Off" sentinel), with short inline text next to the field explaining what zero means for that row. The per-field clamp (clamp_shared_session_*_minutes) keepsrevoke <= warn <= endfor values edited through this UI.app/src/terminal/view/shared_session/view_impl.rs/sharer/mod.rs:Sharernow holds anInactivityLadderSnapshotcaptured once when a sharer's idle period begins (reset_sharer_inactivity_timer), and every later phase transition within that same period — including the follow-up phase armed after roles are revoked, and the warning modal's own end-of-session countdown — is judged against that same snapshot rather than live settings. This means a settings change mid-flight leaves the in-progress idle period's whole chain consistently "old"; only a subsequently-reset idle period (e.g. from new activity) picks up the change, matching the review's request that in-flight timers be unaffected without letting a single chain mix old and new durations.On the "Off" sentinel / slider
The review asked to check whether a slider component (mirroring macOS's idle-sleep slider) already exists before building the "0 = off" state as an input field. A slider component does exist in this codebase (used for opacity/blur), but it's a continuous 0–100 control with no support for the large, non-linear duration range these fields need. Building that mapping would mean building a new slider variant, which was out of scope, so the text-input approach was kept per the fallback guidance.
Testing
cargo build --bin warp,cargo fmt -- --check,cargo clippy -p warp --bin warp --all-targets -- -D warnings— all pass.cargo nextest run -p warpfiltered to shared_session/settings/features_page — 637 passed, including the zero-disables-a-phase matrix, minute-parsing bounds, clamp correctness for zero and non-zero values, file-originated and cloud-originated ordering-correction regression tests (including a disabled-middle-phase case arriving via an external write), and a regression test for the exact mid-flight settings-change scenario from review (an already-armed timer's snapshot is unaffected by a later settings change, while a freshly-reset idle period picks up the new values).Manually verified end-to-end with computer use on a freshly rebuilt, freshly launched instance: the three rows render as one cohesive block with correct labels/values/descriptions; editing a value (e.g.
10→7) persists; setting a field to0shows the literal0plus its inline explanation, and correctly disables/greys out the dependent "Warn" row with its own explanation, with no visual overlap between rows.Known pre-existing issue (tracked separately, not introduced by this PR): APP-5442 — committing a value via Tab in these fields leaves the shared
EditorViewsingle-line text input rendering blank until focus moves elsewhere. This reproduces on a plain numeric commit in these same fields and is a characteristic of the shared editor primitive also used by other existing settings fields, not specific to this feature. The persisted value is always correct on the next full render; clicking into a different field (rather than Tab) also commits and displays it correctly.I have manually tested my changes locally with computer use (equivalent to
./script/run)Screenshots / Videos
Computer-use video recording
Opening Settings via Command Palette, navigating to Features > Session, showing the default three rows, editing Revoke edit access to 7, then editing End the shared session to 0 — confirming no text overlap in the resulting state.
Computer-use screenshots (2)
Default state of the merged Session widget: all three rows shown together with labels, values, and descriptions.
Zero-disabled state: End = 0 with its inline explanation, and the dependent Warn row greyed out with its own explanation — cleanly separated, no overlap.
Agent Mode
CHANGELOG-IMPROVEMENT: You can now customize how long a shared session can sit idle before edit access is revoked, a warning is shown, and the session ends (including disabling any of the three) in Settings > Features > Session.
Conversation: https://staging.warp.dev/conversation/62aa162e-53df-4eac-8b9e-da340c2d3b08
Run: https://oz.staging.warp.dev/runs/019ff20d-8301-756e-b971-2365db195c8a
This PR was generated with Oz.