feat: cmd+click and shift+click multi-select for kanban and sidebar#1518
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds anchor-based range selection for Kanban and sidebar task rows, threads selection props through the UI layers, and adds sidebar bulk archive/move selection state with updated context-menu behavior and E2E coverage. ChangesModifier-key multi-select and bulk actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @zeval's task in 3m 18s —— View job Code Review
Reviewed the full diff against FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions Highlights of what's solid:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| apps/web/hooks/use-task-multi-select.ts | Adds anchor tracking, select_range action, and realignAnchor to the shared reducer; exports INITIAL_STATE and useTaskMultiSelectStore for sidebar reuse; adds sortByDisplayOrder for board-order bulk moves. Well-tested and logically sound. |
| apps/web/hooks/use-sidebar-multi-select.ts | New sidebar multi-select hook reusing the shared reducer; runBulkRemoval correctly handles active-task routing last, keeps failed IDs selected for retry, and clears selection on full success. bulkMove catches errors to prevent unhandled rejections. |
| apps/web/components/task/task-session-sidebar-selection.tsx | Composes sidebar selection lifecycle (Escape-to-clear, prune-to-visible effects), memoized range/move/pin callbacks (useCallback deps correct), and bulk confirm dialogs. onSelectTaskRange is wrapped in useCallback([selectRange, visibleTaskIds]) so TaskSwitcher memo holds. |
| apps/web/components/task/task-switcher-context-menu.tsx | Right-click routing: non-selected rows get single-task menu; selected rows route to bulk or single-selected menu correctly. Archive for a lone selected row uses the bulk path (clears selection). withClear ensures Pin and Delete also clear selection on a single-row selection. |
| apps/web/components/kanban-card.tsx | dispatchKanbanCardClick cleanly handles four click modes; Move is now selection-aware via orderedSelectedIds(). Archive and Delete in the kanban context menu (via menuBase) still open single-task confirm dialogs regardless of selection size — a pre-existing gap flagged in the previous review cycle. |
| apps/web/components/kanban-column.tsx | Adds columnTaskIds memo and wires onSelectRange to onRangeSelect per card correctly; per-column ordered list is the right source of truth for shift-click range selection. |
| apps/web/components/task/task-switcher.tsx | dispatchSidebarRowClick exported for unit testing; prop drilling for selection handlers is mechanical but necessary given the tree depth. TaskRow correctly derives isMultiSelected and isSelecting from the prop Set. |
| apps/web/lib/sidebar/apply-view.ts | Three new pure functions (flattenVisibleTaskIds, sortIdsByVisibleOrder, computeMixedWorkflowSelection) are all well-tested; flattenVisibleTaskIds correctly handles collapsed groups and subtask parents depth-first. |
| apps/web/lib/state/slices/ui/sidebar-task-prefs-actions.ts | pinTasks and unpinTasks follow the same Immer + setStoredPinnedTaskIds + syncSidebarTaskPrefs pattern as the existing togglePinnedTask; the changed flag correctly gates the sync call. |
| apps/web/components/task/task-item.tsx | Adds isMultiSelected styling (ring + lighter bg) and onSelect handler; keyboard events (Enter/Space) correctly delegate to the modifier-aware path. Helper functions cleanly extracted. |
Reviews (15): Last reviewed commit: "test: restore sidebar selection console ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/components/task/task-item.tsx`:
- Around line 127-128: The row highlight classes in task-item.tsx conflict
inside the cn(...) call, causing the active state from isSelected to be
overridden when isMultiSelected is also true. Update the class composition in
the task row component so the active-task highlight remains visible in the
combined selected-and-multi-selected state, using the existing isSelected and
isMultiSelected conditions without letting twMerge drop the stronger background.
In `@apps/web/components/task/task-switcher.tsx`:
- Around line 96-107: The modifier-click branches in TaskSwitcher are not truly
optional-safe because they call preventDefault and return even when
onToggleSelectTask or onSelectTaskRange is absent, which blocks the normal
selection fallback. Update the click handling in TaskSwitcher’s onSelect logic
so Cmd/Ctrl-click and Shift-click only intercept when the corresponding handler
exists; otherwise let the event continue to the regular onSelectTask path used
by TaskRow. Keep the existing modifier detection, but gate the
preventDefault/return behavior on handler presence.
In `@apps/web/e2e/tests/kanban/cmd-shift-multi-select.spec.ts`:
- Around line 13-32: The DOM-order helpers are currently querying
`kanban-column-*` directly from `page`, which can pick up hidden or stale
mounted boards and break the selection assertions. Re-anchor `columnTaskIds()`
and `columnSelectedIds()` to the active Kanban container/page-object root before
calling `getByTestId()` and evaluating card order or `ring-primary` classes. Use
the existing `page`/container access pattern in this spec so these helpers only
inspect the visible active board.
- Around line 174-189: This test only verifies that the multi-select toggle
enables the mode, but it never exercises the Cmd/Ctrl-click coexistence path in
cmd-shift-multi-select.spec.ts. Update the test around
KanbanPage.multiSelectToggle and the task card interaction so it first enables
the toggle and then performs a Cmd/Ctrl-click on a task card via
testPage/kanban, asserting that both toggle-based multi-select and
modifier-click selection still work together.
In `@apps/web/e2e/tests/task/sidebar-multi-select.spec.ts`:
- Around line 29-41: The sidebar E2E helpers are too loosely scoped because
sidebarOrder() uses a global page selector and openSidebarOn() discards the
validated session.sidebar locator, which can cause stale or hidden panels to be
queried in dock/mobile layouts. Update the helper flow in sidebarOrder() and
openSidebarOn() so they accept and use the active session.sidebar container for
both ordering reads and row interactions, keeping all sidebar assertions
anchored to the same mounted panel.
In `@apps/web/hooks/use-sidebar-multi-select.ts`:
- Around line 53-58: The bulk-archive flow in use-sidebar-multi-select’s
bulkArchive swallows request failures because it uses Promise.allSettled and
then always clears selection in the finally block. Change the callback to
surface any failed archiveTaskById calls back to the caller (so the dialog can
show an error), and only clear the selection after a fully successful run; keep
partial successes handling via removeTasksFromStore but preserve failed ids for
retry.
In `@apps/web/hooks/use-task-multi-select.ts`:
- Around line 240-245: The `set_selected` branch in `useTaskMultiSelect` leaves
`anchorId` unchanged for non-empty replacement selections, which can leave it
pointing at an item no longer in `selectedIds`. Update this branch so the anchor
is always realigned to the current selection when `action.ids` replaces state,
using an existing selected id or clearing it when appropriate. Make the change
in the reducer logic that handles `set_selected` so Shift+click range extension
continues from a valid anchor after partial bulk failures or other selection
replacements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 201156cb-11b2-4e4c-9bfa-9e82b7cc0631
📒 Files selected for processing (23)
apps/web/components/kanban-board.tsxapps/web/components/kanban-card-content.tsxapps/web/components/kanban-card.tsxapps/web/components/kanban-column.tsxapps/web/components/kanban/swimlane-container.tsxapps/web/components/kanban/swimlane-kanban-content.tsxapps/web/components/kanban/swipeable-columns.tsxapps/web/components/task/task-item.tsxapps/web/components/task/task-session-sidebar-selection.tsxapps/web/components/task/task-session-sidebar.tsxapps/web/components/task/task-switcher-context-menu.tsxapps/web/components/task/task-switcher.tsxapps/web/e2e/pages/kanban-page.tsapps/web/e2e/pages/sidebar-tasks-page.tsapps/web/e2e/tests/kanban/cmd-shift-multi-select.spec.tsapps/web/e2e/tests/task/sidebar-multi-select.spec.tsapps/web/hooks/use-sidebar-multi-select.tsapps/web/hooks/use-task-multi-select-range.test.tsapps/web/hooks/use-task-multi-select.test.tsapps/web/hooks/use-task-multi-select.tsapps/web/lib/kanban/view-registry.tsapps/web/lib/sidebar/apply-view-flatten.test.tsapps/web/lib/sidebar/apply-view.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8b6abe177
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Claude finished @zeval's task in 3m 52s —— View job Claude Code Review
Posted a findings report and one inline comment. Summary: The latest push ( |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/components/task/task-switcher-click.test.ts (1)
22-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining modifier-routing branches.
dispatchSidebarRowClickalso special-casesctrlKeyand the “selection handlers absent” fallback inapps/web/components/task/task-switcher.tsx:88-116. This suite only exercisesmetaKeywith all handlers wired, so Windows/Linux Ctrl+click and the mobile fallback path can regress unnoticed.Suggested additions
describe("dispatchSidebarRowClick", () => { it("cmd/meta-click toggles and prevents default", () => { const h = handlers(); const e = fakeEvent({ metaKey: true }); dispatchSidebarRowClick(e, "t1", false, h); expect(e.preventDefault).toHaveBeenCalled(); expect(h.onToggleSelectTask).toHaveBeenCalledWith("t1"); expect(h.onSelectTask).not.toHaveBeenCalled(); }); + it("ctrl-click toggles and prevents default", () => { + const h = handlers(); + const e = fakeEvent({ ctrlKey: true }); + dispatchSidebarRowClick(e, "t1", false, h); + expect(e.preventDefault).toHaveBeenCalled(); + expect(h.onToggleSelectTask).toHaveBeenCalledWith("t1"); + expect(h.onSelectTask).not.toHaveBeenCalled(); + }); + it("shift-click range-selects and prevents default", () => { const h = handlers(); const e = fakeEvent({ shiftKey: true }); dispatchSidebarRowClick(e, "t1", false, h); expect(e.preventDefault).toHaveBeenCalled(); expect(h.onSelectTaskRange).toHaveBeenCalledWith("t1"); }); + + it("modifier clicks fall back to navigation when selection handlers are absent", () => { + const onSelectTask = vi.fn(); + dispatchSidebarRowClick(fakeEvent({ metaKey: true }), "t1", false, { onSelectTask }); + expect(onSelectTask).toHaveBeenCalledWith("t1"); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/components/task/task-switcher-click.test.ts` around lines 22 - 53, The dispatchSidebarRowClick test suite is missing coverage for the ctrlKey branch and the fallback path when selection handlers are absent. Add cases around dispatchSidebarRowClick in task-switcher-click.test.ts to verify Ctrl/Cmd modifier routing behaves the same as metaKey, and that the function falls back correctly when onToggleSelectTask, onSelectTaskRange, or other selection handlers are unavailable. Use the existing fakeEvent and handlers helpers to exercise these remaining branches.apps/web/hooks/use-task-multi-select-range.test.ts (1)
33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
anchorIdin the backward-range case too.This case only checks
selectedIds. If reverseselect_rangeever re-anchors to the clicked task, this test would still pass even thoughmultiSelectReduceris supposed to preserve the original anchor.Suggested test assertion
expect(next.selectedIds).toEqual(new Set(["b", "c", "d"])); + expect(next.anchorId).toBe("d");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/hooks/use-task-multi-select-range.test.ts` around lines 33 - 41, The backward-range test for multiSelectReducer only verifies selectedIds and misses the anchor behavior. Update the select_range test that starts from the toggle_select anchor on "d" to also assert next.anchorId remains the original anchor rather than changing to the clicked task. Use the existing multiSelectReducer and anchor-related state fields in use-task-multi-select-range.test.ts to keep the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/components/task/task-switcher-click.test.ts`:
- Around line 22-53: The dispatchSidebarRowClick test suite is missing coverage
for the ctrlKey branch and the fallback path when selection handlers are absent.
Add cases around dispatchSidebarRowClick in task-switcher-click.test.ts to
verify Ctrl/Cmd modifier routing behaves the same as metaKey, and that the
function falls back correctly when onToggleSelectTask, onSelectTaskRange, or
other selection handlers are unavailable. Use the existing fakeEvent and
handlers helpers to exercise these remaining branches.
In `@apps/web/hooks/use-task-multi-select-range.test.ts`:
- Around line 33-41: The backward-range test for multiSelectReducer only
verifies selectedIds and misses the anchor behavior. Update the select_range
test that starts from the toggle_select anchor on "d" to also assert
next.anchorId remains the original anchor rather than changing to the clicked
task. Use the existing multiSelectReducer and anchor-related state fields in
use-task-multi-select-range.test.ts to keep the intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39bff29a-ba6b-4bb6-94c2-10d8ec795dac
📒 Files selected for processing (13)
apps/web/components/kanban-card-click.test.tsapps/web/components/kanban-card.tsxapps/web/components/task/task-item.tsxapps/web/components/task/task-session-sidebar-selection.tsxapps/web/components/task/task-switcher-click.test.tsapps/web/components/task/task-switcher-context-menu.tsxapps/web/components/task/task-switcher.tsxapps/web/e2e/pages/sidebar-tasks-page.tsapps/web/e2e/tests/kanban/cmd-shift-multi-select.spec.tsapps/web/e2e/tests/task/sidebar-multi-select.spec.tsapps/web/hooks/use-sidebar-multi-select.tsapps/web/hooks/use-task-multi-select-range.test.tsapps/web/hooks/use-task-multi-select.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/web/components/task/task-session-sidebar-selection.tsx
- apps/web/e2e/pages/sidebar-tasks-page.ts
- apps/web/components/kanban-card.tsx
- apps/web/hooks/use-task-multi-select.ts
- apps/web/e2e/tests/kanban/cmd-shift-multi-select.spec.ts
- apps/web/e2e/tests/task/sidebar-multi-select.spec.ts
- apps/web/components/task/task-item.tsx
- apps/web/components/task/task-switcher.tsx
- apps/web/components/task/task-switcher-context-menu.tsx
Findings (re-review after latest push)The latest commits ( One gap remains from the testing-policy check: Blocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — add the missing |
There was a problem hiding this comment.
💡 Codex Review
kandev/apps/web/components/kanban-card.tsx
Line 139 in 1a52ec7
Block mixed-workflow moves from card menus
When the selected cards span multiple workflows, the toolbar disables bulk Move to, but the card context/dropdown menu still sends the whole selectedTaskIds set to the clicked card's currentWorkflowId. If a user selects cards in workflows A and B, right-clicks a selected card in A, and chooses Move to for an A step, the B cards are moved into workflow A instead of being blocked as ambiguous. Pass the mixed-workflow state into this menu and disable the same-workflow bulk move, or fall back to moving only the clicked card.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Claude finished @zeval's task in 4m 45s —— View job Claude Code Review
Posted a findings report (corrected here after I initially mis-scoped one finding as introduced by this PR when it's pre-existing code). Summary: The latest commit ( No blockers found in this PR's actual diff. Two non-blocking suggestions remain: (1) the kanban card context menu's move actions don't guard against mixed-workflow selections — confirmed this is pre-existing code this PR doesn't touch, so it's out of scope here but worth a follow-up since this PR makes that selection easier to build; (2) Verdict: Ready to merge. |
Findings (review of latest push, commit
|
| Severity | Count |
|---|---|
| Blocker | 1 |
| Suggestion | 1 |
Verdict: Blocked — guard the kanban card context menu's move actions against mixed-workflow selections before merge; everything else from this round (sidebar hook tests, bulkMove rejection handling, delete-clears-selection) looks solid.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/hooks/use-sidebar-multi-select.ts (1)
28-31: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard bulk-action completions against workspace changes.
Line 29 says this selection is workspace-scoped, but both async bulk handlers still call
dispatch/clearSelectionafter awaiting. If a bulk archive or move starts in workspace A and the user switches to workspace B before it settles, the old request can clear or repopulate B's selection state when it completes.Suggested fix
+ const workspaceRef = useRef(workspaceId); + useEffect(() => { + workspaceRef.current = workspaceId; dispatch({ type: "reset" }); }, [workspaceId]); const bulkArchive = useCallback( async (ids: string[], opts?: { cascade?: boolean }) => { if (ids.length === 0) return; + const opWorkspaceId = workspaceRef.current; // If the open task is in the set, archive it via the switch-aware path last // so the URL/layout moves off it instead of showing stale content. const activeId = store.getState().tasks.activeTaskId; const activeInSet = activeId != null && ids.includes(activeId); const restIds = activeInSet ? ids.filter((id) => id !== activeId) : ids; setIsArchiving(true); try { const results = await Promise.allSettled(restIds.map((id) => archiveTaskById(id, opts))); const failed = restIds.filter((_, i) => results[i].status === "rejected"); const succeeded = restIds.filter((_, i) => results[i].status === "fulfilled"); if (succeeded.length > 0) removeTasksFromStore(new Set(succeeded)); if (activeInSet) { try { await archiveAndSwitch(activeId!, opts); } catch { failed.push(activeId!); } } if (failed.length > 0) { + if (workspaceRef.current !== opWorkspaceId) return; dispatch({ type: "set_selected", ids: new Set(failed) }); toast({ title: `Failed to archive ${failed.length} task${failed.length === 1 ? "" : "s"}`, variant: "error", }); return; } + if (workspaceRef.current !== opWorkspaceId) return; clearSelection(); } finally { setIsArchiving(false); } }, [store, archiveTaskById, archiveAndSwitch, removeTasksFromStore, clearSelection, toast], ); const bulkMove = useCallback( async (ids: string[], targetWorkflowId: string, targetStepId: string) => { if (ids.length === 0) return; + const opWorkspaceId = workspaceRef.current; try { await moveTasks(ids, targetWorkflowId, targetStepId); + if (workspaceRef.current !== opWorkspaceId) return; clearSelection(); } catch { // useTaskWorkflowMove already toasts the failure; keep the rows selected // for retry and swallow the rejection so it isn't unhandled at the // fire-and-forget call site. } }, [moveTasks, clearSelection], );Also applies to: 53-90, 92-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/hooks/use-sidebar-multi-select.ts` around lines 28 - 31, The bulk action handlers in use-sidebar-multi-select should not update selection state after the workspace has changed, since the selection is scoped per workspace. Add a workspace identity guard in the async archive/move flows so dispatch and clearSelection only run if the current workspaceId still matches the one that started the request. Apply this check in the bulk handlers and any shared completion path they use, so stale completions from a previous workspace cannot reset or repopulate the new workspace’s selection state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/hooks/use-sidebar-multi-select.ts`:
- Around line 28-31: The bulk action handlers in use-sidebar-multi-select should
not update selection state after the workspace has changed, since the selection
is scoped per workspace. Add a workspace identity guard in the async
archive/move flows so dispatch and clearSelection only run if the current
workspaceId still matches the one that started the request. Apply this check in
the bulk handlers and any shared completion path they use, so stale completions
from a previous workspace cannot reset or repopulate the new workspace’s
selection state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ae02dfb-3f04-482a-8158-623176cc0dc1
📒 Files selected for processing (6)
apps/web/components/task/task-session-sidebar-selection.tsxapps/web/components/task/task-switcher-context-menu.tsxapps/web/components/task/task-switcher.tsxapps/web/hooks/use-sidebar-multi-select.test.tsapps/web/hooks/use-sidebar-multi-select.tsapps/web/hooks/use-task-multi-select.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/components/task/task-session-sidebar-selection.tsx
- apps/web/hooks/use-task-multi-select.ts
- apps/web/components/task/task-switcher-context-menu.tsx
- apps/web/components/task/task-switcher.tsx
|
Correction to my findings above: the kanban-card.tsx mixed-workflow move gap I flagged as a blocker is pre-existing code that this PR does not modify (verified via Revised verdict for this PR's actual diff (against FindingsNo blockers in the changes introduced by this PR. The latest commit ( Suggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge. |
|
OpenCode review complete OpenCode model: No suggestions found for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c998c9e77
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Claude finished @zeval's task in 2m 45s —— View job Claude Code Review
Posted a findings report and one inline comment. Summary: The latest push ( Verdict: Ready to merge. |
Findings (review of latest push, commit
|
| Severity | Count |
|---|---|
| Blocker | 0 |
| Suggestion | 1 |
Verdict: Ready to merge.
zeval
force-pushed
the
feature/cmd-click-support-si-iwd
branch
from
3bfff53 to
5f09827
Compare
|
Claude finished @zeval's task in 6m 6s —— View job Code Review
Reviewed the latest commits since the last "Ready to merge" verdict ( All prior-round blockers are resolved (visible-order bulk moves, anchor realignment, unit tests, keyboard parity, FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix the two file-size violations before merge. The feature logic, prior-round fixes, and test coverage are all solid. |
FindingsThis is a re-review of the latest commits since the last "Ready to merge" verdict, specifically the new bulk pin/delete sidebar feature (commits The prior round's open items (visible-order bulk moves, anchor realignment, missing unit tests, keyboard parity, Blocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix the two file-size limit violations before merge. The feature logic, tests, and prior-round fixes are all solid. |
|
Claude finished @zeval's task in 7m 29s —— View job Code Review — feat: cmd+click and shift+click multi-select for kanban and sidebarChecklist
FindingsSuggestion (non-blocking) —
|
| Area | Files |
|---|---|
| Shared reducer | use-task-multi-select.ts |
| Sidebar multi-select | use-sidebar-multi-select.ts, .test.ts |
| Sidebar selection wiring | task-session-sidebar-selection.tsx, .test.ts |
| Context menu bulk path | task-switcher-context-menu.tsx |
| Sidebar row rendering | task-switcher.tsx, task-item.tsx |
| Kanban card click/menu | kanban-card.tsx |
| Pure helpers | apply-view.ts (flattenVisibleTaskIds, sortIdsByVisibleOrder, computeMixedWorkflowSelection), .test.ts |
| Kanban order helper | task-order.ts (sortIdsByCreatedDesc) |
| Bulk pin/unpin actions | sidebar-task-prefs-actions.ts |
| E2E page object | e2e/pages/sidebar-tasks-page.ts |
| Extracted components | task-switcher-group.tsx, task-switcher-color-menu.tsx, task-session-sidebar-archived-item.ts |
Previously resolved blockers (all fixed)
- File size violations —
task-switcher-context-menu.tsx(was 627 lines) andtask-switcher.tsx(was 619 lines) are now 564 and 571 lines respectively after extractingTaskColorMenuandGroupHeader/TaskSwitcherSkeletonin7532efe. - Partial archive/delete failure — failed IDs kept selected, error toast fired.
bulkMoverejection — swallowed with selection preserved for retry.- Anchor realignment after prune —
set_selectedrealigns anchor to last visible selected id. - Visible-order bulk moves —
sortIdsByVisibleOrder(sidebar) andsortIdsByCreatedDesc(kanban) correct ordering. - Mixed-workflow/workflow-less guard —
computeMixedWorkflowSelectiongates move-to-step on kanban and sidebar. - Keyboard parity —
handleTaskItemKeyDownroutes throughonSelectwhen wired. - All-pinned bulk unpin —
5f09827correctly callsunpinTaskswhen all selected tasks are already pinned. - Kanban card mixed-workflow menu gate —
db6c18esetsonMoveToStep: undefinedfor mixed selections.
Verdict
Ready to merge. One non-blocking suggestion on test hygiene (spy cleanup). All correctness, security, size-limit, and test-coverage requirements are satisfied.
|
Claude finished @zeval's task in 2m 28s —— View job Code Review
FindingsThis is a re-review of the single commit since the last "Ready to merge" verdict ( The previous round (2026-07-06T15:23) found two blockers (file size violations) and one suggestion (spy leak). The
No new issues introduced by either commit. Summary
Verdict: Ready to merge. |
1 participant
Multi-select previously required clicking a "Multi-select" toggle on the kanban board first and didn't exist at all in the sidebar task list — selecting several tasks to move or archive was slow and inconsistent. Cmd/Ctrl+click and Shift+click now drive selection natively in both surfaces.
Important Changes
select_rangeaction so kanban and sidebar use one range algorithm.Validation
pnpm --filter @kandev/web typecheck && pnpm --filter @kandev/web lint(clean,--max-warnings 0)pnpm --filter @kandev/web test— 3808 unit tests pass (reducer range + sidebar flatten covered)make test-e2e(Playwright, chromium) — 14 new e2e tests pass acrosse2e/tests/kanban/cmd-shift-multi-select.spec.tsande2e/tests/task/sidebar-multi-select.spec.tsPossible Improvements
Low risk. Pressing Escape to dismiss a context menu also clears the active selection — acceptable but could be scoped to ignore Escape while a menu/dialog is open.
Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.