feat(users): multi-select (Space/V) + bulk lifecycle actions#9
feat(users): multi-select (Space/V) + bulk lifecycle actions#9posquit0 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements multi-select and bulk user lifecycle actions (MW-8), allowing operators to select multiple users via Space or Vim-like visual mode (V) and apply actions such as activation, deactivation, suspension, or deletion sequentially. The feedback highlights a few critical issues and improvements: in ListModel, SelectedIDs() and SelectedUsers() should iterate over all users (m.users) rather than just visible ones (m.visible()) to preserve selections across active filters, and clamping in applyBulkVisualRange() should be performed before swapping indices to prevent selection failures. Additionally, it is suggested to enhance the bulk action summary toast by collecting and displaying the logins of any failed users, which also requires importing the strings package.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (m ListModel) SelectedIDs() []string { | ||
| if len(m.selected) == 0 { | ||
| return nil | ||
| } | ||
| out := make([]string, 0, len(m.selected)) | ||
| for _, u := range m.visible() { | ||
| if _, ok := m.selected[u.ID]; ok { | ||
| out = append(out, u.ID) | ||
| } | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
Iterating over m.visible() in SelectedIDs() means that any selected users who are currently filtered out of the visible list (e.g., via the / client-side filter) will be silently excluded from the returned slice. This causes those selections to be lost when performing bulk actions. Iterating over m.users instead ensures that selections are preserved across filtering.
| func (m ListModel) SelectedIDs() []string { | |
| if len(m.selected) == 0 { | |
| return nil | |
| } | |
| out := make([]string, 0, len(m.selected)) | |
| for _, u := range m.visible() { | |
| if _, ok := m.selected[u.ID]; ok { | |
| out = append(out, u.ID) | |
| } | |
| } | |
| return out | |
| } | |
| func (m ListModel) SelectedIDs() []string { | |
| if len(m.selected) == 0 { | |
| return nil | |
| } | |
| out := make([]string, 0, len(m.selected)) | |
| for _, u := range m.users { | |
| if _, ok := m.selected[u.ID]; ok { | |
| out = append(out, u.ID) | |
| } | |
| } | |
| return out | |
| } |
| func (m ListModel) SelectedUsers() []domain.User { | ||
| if len(m.selected) == 0 { | ||
| return nil | ||
| } | ||
| out := make([]domain.User, 0, len(m.selected)) | ||
| for _, u := range m.visible() { | ||
| if _, ok := m.selected[u.ID]; ok { | ||
| out = append(out, u) | ||
| } | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
Iterating over m.visible() in SelectedUsers() means that any selected users who are currently filtered out of the visible list (e.g., via the / client-side filter) will be silently excluded from the bulk action. Iterating over m.users instead ensures that selections are preserved across filtering.
| func (m ListModel) SelectedUsers() []domain.User { | |
| if len(m.selected) == 0 { | |
| return nil | |
| } | |
| out := make([]domain.User, 0, len(m.selected)) | |
| for _, u := range m.visible() { | |
| if _, ok := m.selected[u.ID]; ok { | |
| out = append(out, u) | |
| } | |
| } | |
| return out | |
| } | |
| func (m ListModel) SelectedUsers() []domain.User { | |
| if len(m.selected) == 0 { | |
| return nil | |
| } | |
| out := make([]domain.User, 0, len(m.selected)) | |
| for _, u := range m.users { | |
| if _, ok := m.selected[u.ID]; ok { | |
| out = append(out, u) | |
| } | |
| } | |
| return out | |
| } |
| func (m *ListModel) applyBulkVisualRange() { | ||
| m.selected = map[string]struct{}{} | ||
| for id := range m.bulkVisualBase { | ||
| m.selected[id] = struct{}{} | ||
| } | ||
| vis := m.visible() | ||
| if len(vis) == 0 { | ||
| return | ||
| } | ||
| start, end := m.bulkVisualAnchor, m.cursor | ||
| if start > end { | ||
| start, end = end, start | ||
| } | ||
| if start < 0 { | ||
| start = 0 | ||
| } | ||
| if end >= len(vis) { | ||
| end = len(vis) - 1 | ||
| } | ||
| for i := start; i <= end; i++ { | ||
| m.selected[vis[i].ID] = struct{}{} | ||
| } | ||
| } |
There was a problem hiding this comment.
In applyBulkVisualRange(), the out-of-bounds clamping of start and end is performed after the start > end swap. If both m.bulkVisualAnchor and m.cursor are out of bounds (e.g., due to a background list refresh that reduces the list size), they can be swapped such that start > end remains true after clamping, causing the selection loop to be skipped entirely. Clamping both indices to the valid range of vis before swapping resolves this issue.
func (m *ListModel) applyBulkVisualRange() {
m.selected = map[string]struct{}{}
for id := range m.bulkVisualBase {
m.selected[id] = struct{}{}
}
vis := m.visible()
if len(vis) == 0 {
return
}
start, end := m.bulkVisualAnchor, m.cursor
if start < 0 {
start = 0
}
if start >= len(vis) {
start = len(vis) - 1
}
if end < 0 {
end = 0
}
if end >= len(vis) {
end = len(vis) - 1
}
if start > end {
start, end = end, start
}
for i := start; i <= end; i++ {
m.selected[vis[i].ID] = struct{}{}
}
}| "context" | ||
| "strconv" | ||
| "time" |
| return func() tea.Msg { | ||
| label := userActionLabel(action.Kind) | ||
| ctx := context.Background() | ||
| var ok, failed int | ||
| for i, u := range action.Users { | ||
| if i > 0 { | ||
| bulkSleep(50 * time.Millisecond) | ||
| } | ||
| if err := runSingleUserAction(ctx, port, action.Kind, u.ID); err != nil { | ||
| failed++ | ||
| } else { | ||
| ok++ | ||
| } | ||
| } | ||
| var summary string | ||
| if failed == 0 { | ||
| summary = label + " completed for " + strconv.Itoa(ok) + " users" | ||
| } else { | ||
| summary = label + " completed · " + strconv.Itoa(ok) + " ok, " + | ||
| strconv.Itoa(failed) + " failed" | ||
| } | ||
| return bulkUserCompletedMsg{summary: summary} | ||
| } |
There was a problem hiding this comment.
When a bulk action fails for some users, the summary toast only displays the count of failed actions without indicating which users failed. Collecting the failed logins/IDs and appending them to the summary provides crucial feedback to the operator.
return func() tea.Msg {
label := userActionLabel(action.Kind)
ctx := context.Background()
var ok, failed int
var failedLogins []string
for i, u := range action.Users {
if i > 0 {
bulkSleep(50 * time.Millisecond)
}
if err := runSingleUserAction(ctx, port, action.Kind, u.ID); err != nil {
failed++
login := u.Profile.Login
if login == "" {
login = u.ID
}
failedLogins = append(failedLogins, login)
} else {
ok++
}
}
var summary string
if failed == 0 {
summary = label + " completed for " + strconv.Itoa(ok) + " users"
} else {
summary = label + " completed · " + strconv.Itoa(ok) + " ok, " +
strconv.Itoa(failed) + " failed"
if len(failedLogins) > 0 {
summary += " (" + strings.Join(failedLogins, ", ") + ")"
}
}
return bulkUserCompletedMsg{summary: summary}
}…ame failed logins Address PR #9 review comments on MW-8 (multi-select bulk actions): - SelectedIDs()/SelectedUsers() now iterate `m.users` instead of `m.visible()`, so a client-side `/` filter that hides pinned rows doesn't silently drop them from the bulk target set. The operator's intent is preserved regardless of the display filter. - applyBulkVisualRange() clamps both endpoints into [0, len(vis)-1] before the swap. Clamping after the swap could leave `start > end` when a background refresh shrank the list beneath a stale anchor, dropping the entire selection loop. - Bulk summary now names every failed login: `"1 ok, 2 failed (alice@x.com, carol@x.com)"`. Falls back to the User ID when `Profile.Login` is empty (SCIM edge case) so no slot reads blank. Regression tests cover both scenarios.
MW-8 — operators can now offboard N users in one gesture instead of running :deactivate N times. Space toggles selection on the cursor row, V enters Vim-style visual multi-select where j/k extends the range, and Esc drops both the visual mode and the pinned set. The SEL chrome badge stamps the count so the state is always visible. When ≥1 rows are pinned, opening the status picker or firing a palette write command (:deactivate, :suspend, :delete, …) routes into a new bulk confirm modal that reads "Deactivate 5 users?" and previews the first 3 logins with an "… and N more" trailer. The bulk runner dispatches the port calls sequentially with a 50ms throttle so a large batch doesn't spike the tenant, then emits a summary toast + clears the selection. Selection also clears on user-triggered refresh (R / post-action) and on screen change (list model is torn down). Single-select (no rows pinned) still routes through the existing single-user confirm path so nothing about the current daily flow changes.
…ame failed logins Address PR #9 review comments on MW-8 (multi-select bulk actions): - SelectedIDs()/SelectedUsers() now iterate `m.users` instead of `m.visible()`, so a client-side `/` filter that hides pinned rows doesn't silently drop them from the bulk target set. The operator's intent is preserved regardless of the display filter. - applyBulkVisualRange() clamps both endpoints into [0, len(vis)-1] before the swap. Clamping after the swap could leave `start > end` when a background refresh shrank the list beneath a stale anchor, dropping the entire selection loop. - Bulk summary now names every failed login: `"1 ok, 2 failed (alice@x.com, carol@x.com)"`. Falls back to the User ID when `Profile.Login` is empty (SCIM edge case) so no slot reads blank. Regression tests cover both scenarios.
d9b6366 to
f4ce3a6
Compare
Summary
MW-8 — operators can now offboard multiple users in a single gesture instead of running
:deactivateN times.Spacetoggles selection on the cursor row;Venters Vim-style visual multi-select wherej/kextends the range;Escdrops both the visual mode and every pinned row.▪prefix so operators glance at the list and read which rows are armed.SEL: Nbadge while the set is non-empty (andV-SEL: onwhile visual mode is active) so the state is always visible.:deactivate,:suspend,:delete, …) routes into a new bulk confirm modal that reads "Deactivate 5 users?" and previews the first 3 logins with an "… and N more" trailer.shared.ClearSelectionMsg, and refreshes the screen.R/ post-action) and on screen change (the list model is torn down). Single-select (no rows pinned) still routes through the existing single-user confirm path — nothing about the current daily flow changes.:deactivate,:activate,:expire-password,:delete,:suspend,:unsuspendentries + autocomplete + help entries; the Users help golden was regenerated.Test plan
go build ./...go test ./... -race -count=1Test_MultiSelect_SpaceTogglesCursorRow— Space toggles the pinTest_MultiSelect_SpaceRuneVariant— KeyRunes[" "] also togglesTest_MultiSelect_VEntersVisualJExtends— V enters visual, j extendsTest_MultiSelect_VVisualKShrinksBackToAnchor— k trims the rangeTest_MultiSelect_EscClearsSelectionAndVisual— Esc clearsTest_MultiSelect_EscapeWillActReflectsSelection— App Shell defers to local EscTest_MultiSelect_RefreshClearsSelection— R clears the setTest_MultiSelect_ClearSelectionMsgClears— post-bulk-completion clearTest_MultiSelect_SelectedUsersReturnsSnapshot— full domain.User is returnedTest_MultiSelect_RowPrefixMarksSelected— ▪ prefix appearsTest_BulkDeactivate_ConfirmWordingRendersCount— headline reads "3 users"Test_BulkDeactivate_TrailerWhenOver3Rows— "… and N more" trailerTest_BulkDeactivate_ConfirmYFiresPortForEveryRow— one Deactivate per row, in list orderTest_BulkDeactivate_CompletionSignalsClear— completion emits toast + ClearSelection + RefreshTest_SingleUser_StillRoutesThroughSingleFlow— no regressions on the single-user path