Skip to content

feat(sidebar): multi-select tasks with bulk archive#2203

Merged
fercgomes merged 6 commits into
mainfrom
posthog-code/sidebar-multi-select-bulk-archive
May 22, 2026
Merged

feat(sidebar): multi-select tasks with bulk archive#2203
fercgomes merged 6 commits into
mainfrom
posthog-code/sidebar-multi-select-bulk-archive

Conversation

@fercgomes
Copy link
Copy Markdown
Contributor

Summary

  • Cmd/Ctrl-click toggles a task in the selection; shift-click extends a range from the last clicked anchor.
  • Right-clicking a task while 2+ are selected opens a bulk context menu with "Archive N tasks" (confirm dialog). Right-clicking outside the selection clears it and falls through to the single-task menu.
  • Escape clears the selection (ignored when focus is in an input/textarea/contenteditable).
  • Selection lives in a small Zustand store (taskSelectionStore) modeled after the inbox pattern, with a prune effect that drops ids no longer visible when filters change.
  • archiveTaskImperative gets a sibling archiveTasksImperative helper used by both the new bulk path and the existing "Archive prior tasks" action.

Test plan

  • Plain click navigates and clears any selection
  • Cmd/Ctrl-click toggles selection without navigating
  • Shift-click selects a contiguous range across pinned + chronological tasks
  • Right-click on a selected task with 2+ selected opens the bulk menu and "Archive N tasks" archives all of them
  • Right-click on an unselected task with a selection clears the selection and shows the normal single-task menu
  • Escape clears the selection; typing in an input does not
  • Hover toolbar (pin/archive icons) is hidden while a multi-selection is active
  • Changing filters (e.g. organize/sort/visibility) prunes any selected ids that are no longer visible
  • pnpm --filter code typecheck, biome, and vitest (43 tests in sidebar/tasks) all pass

@fercgomes fercgomes marked this pull request as ready for review May 18, 2026 21:55
@fercgomes fercgomes requested review from charlesvien, jonathanlab and sortafreel and removed request for charlesvien May 18, 2026 21:55
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 18, 2026

Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
apps/code/src/renderer/features/sidebar/stores/taskSelectionStore.test.ts:90-112
The "forward range" and "backward range" tests are symmetric cases testing the same logical property (range is order-independent) and are a natural fit for a parameterised test. Per the team's simplicity rules, we prefer `it.each` for cases like this.

```suggestion
    it.each([
      { direction: "forward", anchor: "t2", target: "t4" },
      { direction: "backward", anchor: "t4", target: "t2" },
    ])(
      "selects a $direction range from anchor to target",
      ({ anchor, target }) => {
        useTaskSelectionStore.setState({ lastClickedId: anchor });

        useTaskSelectionStore.getState().selectRange(target, orderedIds);

        expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual([
          "t2",
          "t3",
          "t4",
        ]);
      },
    );
```

### Issue 2 of 3
apps/code/src/renderer/features/sidebar/stores/taskSelectionStore.test.ts:130-142
These two cases both assert "selects just the target" with the only difference being whether `lastClickedId` is absent vs. points at an id not in the ordered list. They're a clean `it.each` opportunity.

```suggestion
    it.each([
      { case: "no anchor", lastClickedId: null },
      { case: "anchor not in ordered list", lastClickedId: "t99" },
    ])(
      "selects just the target when $case",
      ({ lastClickedId }) => {
        if (lastClickedId) {
          useTaskSelectionStore.setState({ lastClickedId });
        }

        useTaskSelectionStore.getState().selectRange("t3", orderedIds);

        expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual(["t3"]);
      },
    );
```

### Issue 3 of 3
apps/code/src/main/services/context-menu/schemas.ts:13-15
The schema uses `positive()` (≥ 1), but the context menu label renders `"Archive ${taskCount} tasks"` — if `taskCount` is 1 that reads "Archive 1 tasks". Changing to `.min(2)` also makes the schema self-document that this endpoint is intentionally bulk-only.

```suggestion
export const bulkTaskContextMenuInput = z.object({
  taskCount: z.number().int().min(2),
});
```

Reviews (1): Last reviewed commit: "fix(sidebar): shift-click range follows ..." | Re-trigger Greptile

Comment on lines +90 to +112
it("selects a forward range from anchor to target", () => {
useTaskSelectionStore.setState({ lastClickedId: "t2" });

useTaskSelectionStore.getState().selectRange("t4", orderedIds);

expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual([
"t2",
"t3",
"t4",
]);
});

it("selects a backward range from anchor to target", () => {
useTaskSelectionStore.setState({ lastClickedId: "t4" });

useTaskSelectionStore.getState().selectRange("t2", orderedIds);

expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual([
"t2",
"t3",
"t4",
]);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The "forward range" and "backward range" tests are symmetric cases testing the same logical property (range is order-independent) and are a natural fit for a parameterised test. Per the team's simplicity rules, we prefer it.each for cases like this.

Suggested change
it("selects a forward range from anchor to target", () => {
useTaskSelectionStore.setState({ lastClickedId: "t2" });
useTaskSelectionStore.getState().selectRange("t4", orderedIds);
expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual([
"t2",
"t3",
"t4",
]);
});
it("selects a backward range from anchor to target", () => {
useTaskSelectionStore.setState({ lastClickedId: "t4" });
useTaskSelectionStore.getState().selectRange("t2", orderedIds);
expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual([
"t2",
"t3",
"t4",
]);
});
it.each([
{ direction: "forward", anchor: "t2", target: "t4" },
{ direction: "backward", anchor: "t4", target: "t2" },
])(
"selects a $direction range from anchor to target",
({ anchor, target }) => {
useTaskSelectionStore.setState({ lastClickedId: anchor });
useTaskSelectionStore.getState().selectRange(target, orderedIds);
expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual([
"t2",
"t3",
"t4",
]);
},
);
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sidebar/stores/taskSelectionStore.test.ts
Line: 90-112

Comment:
The "forward range" and "backward range" tests are symmetric cases testing the same logical property (range is order-independent) and are a natural fit for a parameterised test. Per the team's simplicity rules, we prefer `it.each` for cases like this.

```suggestion
    it.each([
      { direction: "forward", anchor: "t2", target: "t4" },
      { direction: "backward", anchor: "t4", target: "t2" },
    ])(
      "selects a $direction range from anchor to target",
      ({ anchor, target }) => {
        useTaskSelectionStore.setState({ lastClickedId: anchor });

        useTaskSelectionStore.getState().selectRange(target, orderedIds);

        expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual([
          "t2",
          "t3",
          "t4",
        ]);
      },
    );
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +130 to +142
it("selects just the target when there is no anchor", () => {
useTaskSelectionStore.getState().selectRange("t3", orderedIds);

expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual(["t3"]);
});

it("selects just the target when anchor is not in the ordered list", () => {
useTaskSelectionStore.setState({ lastClickedId: "t99" });

useTaskSelectionStore.getState().selectRange("t3", orderedIds);

expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual(["t3"]);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 These two cases both assert "selects just the target" with the only difference being whether lastClickedId is absent vs. points at an id not in the ordered list. They're a clean it.each opportunity.

Suggested change
it("selects just the target when there is no anchor", () => {
useTaskSelectionStore.getState().selectRange("t3", orderedIds);
expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual(["t3"]);
});
it("selects just the target when anchor is not in the ordered list", () => {
useTaskSelectionStore.setState({ lastClickedId: "t99" });
useTaskSelectionStore.getState().selectRange("t3", orderedIds);
expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual(["t3"]);
});
it.each([
{ case: "no anchor", lastClickedId: null },
{ case: "anchor not in ordered list", lastClickedId: "t99" },
])(
"selects just the target when $case",
({ lastClickedId }) => {
if (lastClickedId) {
useTaskSelectionStore.setState({ lastClickedId });
}
useTaskSelectionStore.getState().selectRange("t3", orderedIds);
expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual(["t3"]);
},
);
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sidebar/stores/taskSelectionStore.test.ts
Line: 130-142

Comment:
These two cases both assert "selects just the target" with the only difference being whether `lastClickedId` is absent vs. points at an id not in the ordered list. They're a clean `it.each` opportunity.

```suggestion
    it.each([
      { case: "no anchor", lastClickedId: null },
      { case: "anchor not in ordered list", lastClickedId: "t99" },
    ])(
      "selects just the target when $case",
      ({ lastClickedId }) => {
        if (lastClickedId) {
          useTaskSelectionStore.setState({ lastClickedId });
        }

        useTaskSelectionStore.getState().selectRange("t3", orderedIds);

        expect(useTaskSelectionStore.getState().selectedTaskIds).toEqual(["t3"]);
      },
    );
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment thread apps/code/src/main/services/context-menu/schemas.ts
fercgomes and others added 4 commits May 22, 2026 11:34
Cmd/Ctrl-click toggles a task in the selection; shift-click extends a
range from the last clicked anchor. Right-clicking a task while 2+ are
selected opens a bulk context menu offering "Archive N tasks" with a
confirm dialog; right-clicking outside the selection clears it and
falls through to the single-task menu. Escape (when no input is
focused) clears the selection.

The selection is held in a small Zustand store mirroring the inbox
pattern, with a prune effect to drop ids that are no longer visible
(e.g. after filters change). archiveTaskImperative gets a sibling
archiveTasksImperative helper used by both the new bulk path and the
existing "Archive prior tasks" action.

Generated-By: PostHog Code
Task-Id: a599e7e3-7c09-44dc-a847-551d283c9eb4
…chor

Plain click clears lastClickedId, so a follow-up shift-click had no
anchor and only selected the target. Pass the active/routed task as a
fallback anchor to selectRange so shift-click after a navigation
selects the range from the current task to the target.

The active task is also now treated as implicit member of any bulk
selection — the count in the bulk context menu, the bulk archive set,
and the hide-hover-actions visual all use the effective set of
{active, ...selected}. This matches user expectation: with task A
routed and B, C cmd-clicked, right-click reports "Archive 3 tasks".

Generated-By: PostHog Code
Task-Id: a599e7e3-7c09-44dc-a847-551d283c9eb4
Range selection was indexed against the chronological flat list, but
in by-project mode the user sees tasks grouped per repo. Shift-click
from the first to the third visible task in a group would span across
all interleaved chronological tasks from other groups — e.g. picking
3 tasks visually but reporting 14 in the bulk menu.

Now build an orderedVisibleTaskIds list that matches the rendered
order: pinned, then either grouped-by-project (skipping collapsed
sections) or chronological flat. Pass that to selectRange so the
range matches what the user sees.

Generated-By: PostHog Code
Task-Id: a599e7e3-7c09-44dc-a847-551d283c9eb4
The prune effect fires whenever the visible task list changes reference,
and the previous pruneSelection unconditionally wrote a new filtered
array into the store — even when no IDs were actually removed. That new
selectedTaskIds reference re-rendered SidebarMenu, and combined with
frequent upstream churn from useSidebarData (sessions, viewed
timestamps, archived IDs) it cascaded into a "Maximum update depth
exceeded" React error.

Short-circuit by returning the existing state object when the filter
yields the same length, so subscribers see the same reference and skip
the re-render.

Generated-By: PostHog Code
Task-Id: 53741f05-8d34-4e60-a478-89c4e6905375
@adboio adboio force-pushed the posthog-code/sidebar-multi-select-bulk-archive branch from a6c8f63 to 1e5e1a5 Compare May 22, 2026 18:36
adboio added 2 commits May 22, 2026 11:38
Generated-By: PostHog Code
Task-Id: 53741f05-8d34-4e60-a478-89c4e6905375
…ests

- Bulk context menu schema now requires taskCount >= 2 (the label
  "Archive N tasks" only reads correctly for 2+).
- Collapse symmetric forward/backward selectRange tests into an it.each.

Generated-By: PostHog Code
Task-Id: 53741f05-8d34-4e60-a478-89c4e6905375
Copy link
Copy Markdown
Contributor

adboio commented May 22, 2026

thank you, this is great!

fixed merge conflict + greptile comments + a bug (that might have been a result of my conflict "fix" lol)

approved, feel free to reverse-review if you want and merge!

@fercgomes fercgomes merged commit b9f212e into main May 22, 2026
15 checks passed
@fercgomes fercgomes deleted the posthog-code/sidebar-multi-select-bulk-archive branch May 22, 2026 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants