spec: Shift-click bulk selection + bulk delete in Warp Drive (#10244)#10499
spec: Shift-click bulk selection + bulk delete in Warp Drive (#10244)#10499lonexreb wants to merge 1 commit into
Conversation
|
I'm starting a first review of this spec-only 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 |
github-actions
Bot
added
the
external-contributor
![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 spec defines the Warp Drive multi-select behavior, bulk delete/move flows, keyboard parity, persistence across filtering, implementation pointers, tests, and telemetry. The core interaction model is mostly complete, but one destructive-action edge case needs to be specified before implementation.
Concerns
- Bulk actions explicitly include selected items that are hidden by filtering, but the delete confirmation copy does not require warning that hidden items are included.
Security
- Hidden selected Drive items can be deleted even when they are not visible in the current filtered view; the destructive confirmation must make that scope explicit.
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
| Items selected before a filter/search narrows the visible set REMAIN | ||
| selected even when filtered out of view. The selection bar shows | ||
| `<visible> + <hidden> selected` so the user can see the hidden count and | ||
| decide whether to clear before acting. Bulk actions still apply to all |
There was a problem hiding this comment.
|
@lonexreb Thanks for opening this PR (and others)! I'd like to note that our contributions process requires that issues be labelled as I'm going to close this PR out since it's not associated with an issue that has a |
|
Round-1 fixes for oz-for-oss review: (1) bulk-delete confirmation now explicitly surfaces hidden-selected count — wording shifts to 'Delete items, including not currently visible due to filter?' when applicable; special wording for all-hidden case; (2) added 'Show hidden' link in confirm dialog opens secondary view listing all selected items with optional per-item deselect; (3) destructive-action invariant stated — implementations MUST surface hidden count before proceeding; (4) tests for visible-only, with-hidden, all-hidden, show-hidden-modal, and deselect-from-hidden flows. |
|
/oz-review |
|
I'm re-reviewing this spec-only 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
Reviewed the spec-only PR for Warp Drive multi-select, bulk delete, bulk move, keyboard parity, filtering behavior, and telemetry.
Concerns
- The plain-click selection contract is not reconciled with the non-goal of preserving existing single-item behavior.
- Bulk actions can affect filtered-out selected items, but the destructive delete confirmation and tests do not require hidden-item disclosure.
- The Plan delete recovery model is still deferred even though it changes user-facing copy and implementation semantics.
Cmd/Ctrl+Aand other shortcuts need focus-scoping rules so text inputs and the row list do not conflict.
Security
- Any server-side batch delete/move path needs an explicit requirement to preserve per-item authorization and ownership checks.
- The client-side chunking guidance should specify total/parallelism/rate-limit constraints, not just chunk size.
Verdict
Found: 0 critical, 5 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
| - No server-side admin bulk tools. | ||
| - No in-app undo window for Plan deletion (Plans are higher-stakes than chat | ||
| history; undo is intentionally NOT mirrored from #10457). | ||
| - No change to single-item behavior (open, run, share) at all. |
There was a problem hiding this comment.
|
|
||
| ### B4. Cmd/Ctrl+A selects all matching | ||
|
|
||
| `Cmd+A` (mac) / `Ctrl+A` (win/linux) selects every row in the currently |
There was a problem hiding this comment.
| Items selected before a filter/search narrows the visible set REMAIN | ||
| selected even when filtered out of view. The selection bar shows | ||
| `<visible> + <hidden> selected` so the user can see the hidden count and | ||
| decide whether to clear before acting. Bulk actions still apply to all |
There was a problem hiding this comment.
| - `WarpDriveSelection` model holding `{ anchor: Option<RowId>, set: HashSet<RowId> }`. | ||
| - Bulk delete and bulk move route through existing per-item endpoints with a | ||
| client-side iterator bounded to a sane batch (e.g. 200 per chunk). | ||
| - A server-side batch endpoint may already exist; if so, use it and let the |
There was a problem hiding this comment.
|
|
||
| - `WarpDriveSelection` model holding `{ anchor: Option<RowId>, set: HashSet<RowId> }`. | ||
| - Bulk delete and bulk move route through existing per-item endpoints with a | ||
| client-side iterator bounded to a sane batch (e.g. 200 per chunk). |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Replace “sane batch” with concrete total selection, concurrency, retry, and rate-limit behavior; chunk size alone does not bound work after select-all.
| - Should bulk delete for Plans support a chat-history-style 5-second in-app | ||
| undo? Plans are more precious than chat messages. Recommendation: NO | ||
| in-app undo by default; instead, record a 30-day server-side soft | ||
| tombstone that admins can recover. Final decision deferred to engineering. |
There was a problem hiding this comment.
cannot be undone and a 30-day tombstone imply different product copy, backend semantics, and validation criteria.
|
Addressed all 5 important + 1 suggestion from the latest oz-for-oss review:
Commit: b733582 |
|
/oz-review |
|
I'm re-reviewing this spec-only 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 spec defines V1 Warp Drive multi-select plus bulk delete and move. It covers the main selection gestures, filtering behavior, acceptance criteria, tests, and implementation pointers, but several requirements need to be resolved before implementation.
Concerns
- The spec contradicts itself on whether plain row clicks change existing single-item behavior.
- Keyboard shortcuts need focus scoping so
Cmd/Ctrl+Adoes not hijack text selection in filter/search inputs. - Destructive bulk delete needs clearer confirmation when hidden selected items are still affected.
- Bulk endpoint usage needs explicit item-level authorization and bounded-work requirements.
Security
- If an existing or new batch endpoint is used, the spec should require item-level authorization/ownership checks and per-item failure handling instead of assuming equivalence with the per-item routes.
- Chunking alone does not cap total server work; the spec should define maximum action size, bounded concurrency, or backoff for select-all bulk operations.
Verdict
Found: 0 critical, 4 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
|
|
||
| ### B1. Single-click selects one row | ||
|
|
||
| A plain click on a row clears any existing multi-selection and selects that |
There was a problem hiding this comment.
|
|
||
| ### B4. Cmd/Ctrl+A selects all matching | ||
|
|
||
| `Cmd+A` (mac) / `Ctrl+A` (win/linux) selects every row in the currently |
There was a problem hiding this comment.
Cmd/Ctrl+A to list focus. When the filter/search input has focus, the shortcut should keep selecting input text unless the spec intentionally changes text-field behavior.
| selected even when filtered out of view. The selection bar shows | ||
| `<visible> + <hidden> selected` so the user can see the hidden count and | ||
| decide whether to clear before acting. Bulk actions still apply to all | ||
| selected items, visible or not. |
There was a problem hiding this comment.
<N> alone can delete off-screen items unexpectedly.
| - `WarpDriveSelection` model holding `{ anchor: Option<RowId>, set: HashSet<RowId> }`. | ||
| - Bulk delete and bulk move route through existing per-item endpoints with a | ||
| client-side iterator bounded to a sane batch (e.g. 200 per chunk). | ||
| - A server-side batch endpoint may already exist; if so, use it and let the |
There was a problem hiding this comment.
|
|
||
| - `WarpDriveSelection` model holding `{ anchor: Option<RowId>, set: HashSet<RowId> }`. | ||
| - Bulk delete and bulk move route through existing per-item endpoints with a | ||
| client-side iterator bounded to a sane batch (e.g. 200 per chunk). |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Chunk size controls request shape but not total work. Specify a maximum total selection/action size or bounded concurrency/backoff so select-all cannot trigger unbounded delete/move traffic.
|
Addressed all four round-N important concerns plus the bounded-work suggestion. 1. Plain-click contract. Replaced the dense Non-Goals reconciliation with a 7-point authoritative B1 contract. Plain click now explicitly: (1) clears multi-selection, (2) selects the row, (3) sets the anchor, and (4-7) does NOT open/run/share/show selection bar. Added a 'Reconciliation with prior single-row behavior' subsection cleanly separating user-visible effects (unchanged from pre-V1) from internal state effects (new, non-visible). B1 is now stamped authoritative. 2. 3. Bulk delete with hidden selections. Added a load-bearing authoritative rule at the top of B7: bulk delete MUST NOT proceed when 4. Bulk endpoint authorization. Renamed the section to 'Server-side authorization and bounded work'. Added a Scope paragraph stating the rules apply equally to batch and per-item paths with no bypass. Explicitly declared 'trust-the-batch' (authorize-once-per-request) authorization non-conforming. 5. Suggestion — bounded total work. Added a 'Bounded total work (applies to BOTH paths)' subsection making the 2000-cap, 4-concurrency, 20 req/s rate cap, and exponential backoff binding on batch payloads as well. Chunking alone is now explicitly not sufficient; total work caps are an independent additive constraint. |
|
/oz-review |
|
I'm re-reviewing this spec-only 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
Reviewed the spec-only PR for Warp Drive multi-select, bulk delete, bulk move, keyboard behavior, filtering persistence, and telemetry.
Concerns
- The spec contradicts itself about whether single-item open/run/share behavior changes when a plain row click now selects a row.
Cmd/Ctrl+Ais not scoped relative to the filter text field, which makes the core search-then-select-all flow ambiguous.- Hidden selections can still be bulk deleted or moved, but the delete confirmation does not require surfacing hidden-selected scope.
- Security: bulk delete/move API guidance needs explicit per-item authorization/ownership revalidation, especially if a batch endpoint is used.
Security
- The destructive bulk action path should require server-side per-item authorization/ownership checks and define unauthorized or stale item handling; client-selected
RowIds must not be trusted as authority.
Verdict
Found: 1 critical, 3 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
|
|
||
| ### B1. Single-click selects one row | ||
|
|
||
| A plain click on a row clears any existing multi-selection and selects that |
There was a problem hiding this comment.
🚨 [CRITICAL] B1 makes plain click select a row, but Non-Goals say single-item open/run/share behavior must not change. Specify whether clicking opens/runs or selects, and if selection moves elsewhere add the required mouse affordance.
|
|
||
| ### B4. Cmd/Ctrl+A selects all matching | ||
|
|
||
| `Cmd+A` (mac) / `Ctrl+A` (win/linux) selects every row in the currently |
There was a problem hiding this comment.
Cmd/Ctrl+A targets the Drive list versus the active filter/search input; the search-then-select-all flow otherwise conflicts with normal text-field select-all behavior.
| ### B7. Bulk Delete | ||
|
|
||
| Triggered from the selection bar Delete action. Shows a modal: | ||
| `Delete <N> items? This cannot be undone.` `[Delete] [Cancel]`. |
There was a problem hiding this comment.
X visible + Y hidden) before destructive delete applies to filtered-out items.
| - `WarpDriveSelection` model holding `{ anchor: Option<RowId>, set: HashSet<RowId> }`. | ||
| - Bulk delete and bulk move route through existing per-item endpoints with a | ||
| client-side iterator bounded to a sane batch (e.g. 200 per chunk). | ||
| - A server-side batch endpoint may already exist; if so, use it and let the |
There was a problem hiding this comment.
RowId selection cannot be trusted.
2 participants
Spec for #10244.
Summary
V1 multi-select for Warp Drive: Shift-click range, Cmd/Ctrl-click toggle,
Cmd+Aselect-all-filtered,Escclear. A compact selection bar (<N> selected · Delete · Move to… · Cancel) appears at >=2 selected. Includes keyboard parity (Shift+Arrowextension,Spacetoggle,Enteropen).Bulk Delete with confirm modal; bulk Move with the existing folder picker. Selection persists across filter narrowing.
Why
Drive items (especially AI-generated Plans) accumulate quickly and the only delete path is one-at-a-time. Users want the file-list ergonomics they already know.
Distinct from #10457
This is intentionally a different model from chat-history bulk delete:
Scope
Spec-only — no code changes in this PR. See
specs/GH10244/SPEC.mdfor behavior contract, acceptance criteria, implementation pointers (verified Drive paths inapp/src/settings_view/warp_drive_page.rsandapp/src/search/command_palette/warp_drive/), tests, and open questions.Tag:
ready-to-spec.