feat: add multi-select and drag-to-move for file tree and changes panel#490
Merged
feat: add multi-select and drag-to-move for file tree and changes panel#490
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 multi-selection and bulk actions to the Files and Changes UIs, multi-node drag-and-drop with optimistic moves/deletes, new Changes
Sequence DiagramsequenceDiagram
participant User as "User"
participant UI as "File/Changes UI"
participant Multi as "useMultiSelect"
participant Session as "Session Git Ops"
User->>UI: Click file (plain / Ctrl/Meta / Shift)
UI->>Multi: handleClick(path, event)
Multi->>Multi: update selectedPaths (toggle/range/replace)
Multi-->>UI: selectedPaths
UI->>UI: render BulkActionBar or DefaultActionButtons
User->>UI: Click bulk action (Stage/Unstage/Discard)
UI->>Session: onBulk*(selectedPaths array)
Session->>Session: git.stageFile/git.unstageFile/git.discard(paths)
Session-->>UI: success/failure
UI->>Multi: clearSelection() (on success)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
| Filename | Overview |
|---|---|
| apps/web/components/task/changes-panel-dialogs.tsx | Adds filesToDiscard to DiscardDialog; P1 bug: isBulk guard uses > 1 instead of >= 1, causing blank filename in the confirmation dialog for single-file bulk discard |
| apps/web/hooks/use-multi-select.ts | New shared hook for range/toggle multi-select; stale-path pruning via useMemo is correct; onSelectionChange called inside state updater (previously flagged, not yet fixed) but has no callers today |
| apps/web/components/task/file-browser.tsx | useDragAndDrop and useSelectionInteractions well-structured; useKeyboardShortcuts re-subscribes on every render because multiSelect object reference changes each time (P2) |
| apps/web/components/task/file-tree-utils.ts | New tree utility functions (getVisiblePaths, computeMoveTargets, moveNodesInTree) are well-tested and handle name-collision deduplication correctly |
| apps/web/components/task/changes-panel-timeline.tsx | Each FileListSection gets its own useMultiSelect instance; selection is automatically pruned when files move; BulkActionBar swaps in for DefaultActionButtons when anything is selected |
| apps/web/components/task/file-context-menu.tsx | Previously-flagged ghost-restore on partial failure still present; new DeleteConfirmDialog correctly handles both directory and bulk-selection cases |
Reviews (17): Last reviewed commit: "test: add unit tests for file tree move ..." | 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: 8
🧹 Nitpick comments (4)
apps/web/e2e/tests/file-tree-multi-select.spec.ts (1)
47-50: Minor: Commit message is not escaped in shell command.The
messageparameter is interpolated directly into the shell command, which could cause issues if the message contains special characters (quotes, backticks). Since test messages are controlled, this is low risk but could be hardened.🛡️ Safer alternative using array-based exec
commit(message: string): string { - this.exec(`git commit -m "${message}"`); + execSync("git", ["commit", "-m", message], { cwd: this.repoDir, env: this.env }); return this.exec("git rev-parse HEAD").trim(); }Or use
execFileSyncwhich handles arguments safely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/e2e/tests/file-tree-multi-select.spec.ts` around lines 47 - 50, The commit(message: string) method interpolates message into a shell string which can break for quotes; change it to pass the message as a safe argument instead of inline interpolation—use the existing exec helper with an array-argument form or switch to execFileSync/execFile with args (e.g., call git commit -m with message as a separate arg) and keep returning exec("git rev-parse HEAD").trim(); update the commit function to build the command using arguments rather than injecting message into the shell string.apps/web/components/task/file-context-menu.tsx (2)
51-90:fileCountis computed even for bulk deletions where it's unused.
countFilesInTree(node)is called unconditionally via thefileCountprop, but the result is only displayed when!isBulk. Consider computing it conditionally or moving the computation inside the dialog.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/file-context-menu.tsx` around lines 51 - 90, The DeleteConfirmDialog is receiving fileCount (computed by countFilesInTree(node)) even when isBulk is true and the value is unused; fix by computing fileCount only when needed—either stop passing fileCount for bulk operations or move the call to countFilesInTree(node) inside DeleteConfirmDialog and wrap it in a lazy calculation (e.g., useMemo) that runs only when !isBulk and node is present; update the calling code that currently invokes countFilesInTree and the DeleteConfirmDialog signature (symbols: DeleteConfirmDialog, countFilesInTree, fileCount, node, isBulk) so the expensive count is skipped for bulk deletions.
36-49: Consider showing a toast on delete failure for better user feedback.
deleteNodeOptimisticallysilently reverts on failure. Users won't know why the file reappeared. A brief error toast would improve UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/file-context-menu.tsx` around lines 36 - 49, The optimistic delete in deleteNodeOptimistically currently reverts state silently; update it to notify the user on failure by calling the app's toast/error notifier (e.g., toast.error or showToast) inside both the .then(!ok) branch and the .catch branch after you setTree(snapshot). Keep the existing snapshot rollback via setTree(snapshot) and add a concise error message that includes the file path (or a brief description) to give context; reference deleteNodeOptimistically, setTree, onDeleteFile, removeNodeFromTree, and snapshot when making the change.apps/web/components/task/changes-panel-timeline.tsx (1)
296-472: Split the new bulk-selection UI out of this file.This file is now 633 lines, and the added
DefaultActionButtons/BulkActionBarlogic concentrates too many responsibilities inchanges-panel-timeline.tsx. Extracting the bulk-selection controls, or the full multi-select file section, into a dedicated module would make the staged/unstaged flows easier to test and bring the file back in line with the repo limits. As per coding guidelines, "TypeScript code: Files ≤600 lines; functions ≤100 lines; cyclomatic complexity ≤15; cognitive complexity ≤20; nesting depth ≤4; parameters ≤5" and "Frontend: Components must be ≤200 lines; extract to domain components; use composition over props."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/changes-panel-timeline.tsx` around lines 296 - 472, The FileListSection has grown too large—extract the bulk-selection UI into its own component file: move DefaultActionButtons and BulkActionBar into a new module (e.g., BulkSelectionControls) and export them (keeping their prop signatures intact), then import and use them from changes-panel-timeline.tsx; ensure FileListSection still passes the same props (actionLabel/isActionLoading/onAction, onBulkStage/onBulkUnstage/onBulkDiscard/onClearSelection) and preserve data-testid attributes and behavior (selection clearing after actions) so staged/unstaged flows and tests continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/components/task/changes-panel-file-row.tsx`:
- Around line 75-76: Update the E2E test in review-cumulative-diff.spec.ts to
use the new data-testid pattern emitted by the component: replace the old
getByTestId("file-row-review_cumulative_test.txt") (and any similar "file-row-*"
queries) with getByTestId("changes-file-review_cumulative_test.txt") so it
matches the component's data-testid={`changes-file-${file.path}`} and any
waits/asserts target the updated test id.
In `@apps/web/components/task/changes-panel-timeline.tsx`:
- Around line 291-293: The bulk-action handlers (onBulkStage, onBulkUnstage,
onBulkDiscard) currently trigger onClearSelection() immediately after
dispatching, which drops the selection even if the action is cancelled or fails;
change the flow so the component waits for the handler to complete (have the
handlers return a Promise or accept a success callback) and only call
onClearSelection() when the Promise resolves successfully (or the success
callback is invoked), leaving selection intact on rejection/cancel; update
places that call these handlers (the bulk action invoke sites around functions
handling stage/unstage/discard) to await the result and clear selection on
success, and ensure errors or cancellations do not call onClearSelection().
- Around line 456-468: The two bulk-discard buttons are ambiguous because both
render data-testid="bulk-discard"; update the Button in
changes-panel-timeline.tsx (the JSX block that calls
onBulkDiscard/onClearSelection and uses props paths and selectionCount) to
include a variant-scoped test id (e.g. incorporate the section variant prop or a
local identifier such as "staged" vs "unstaged" or a sectionId) so each
FileListSection renders a unique data-testid like "bulk-discard-staged" or
"bulk-discard-unstaged"; ensure the value is derived from an existing prop or
local variable (not hard-coded) so tests can target the correct button.
In `@apps/web/components/task/changes-panel.tsx`:
- Around line 523-525: The bulk handlers
(onBulkStage/onBulkUnstage/onBulkDiscard) currently use "void
git.stageFile(paths)" etc., which drops returned Promises and causes unhandled
rejections; change these to call the same async wrapper used for single-file
actions (or attach .catch(...) handlers) so errors are caught and surfaced
(e.g., show toast/log) — update the handlers referencing git.stageFile,
git.unstageFile, and git.discard to await or append .catch and reuse the
single-file error handling routine to ensure consistent behavior.
In `@apps/web/components/task/file-browser.tsx`:
- Around line 221-225: The drag state can remain stuck because handleDragEnd
only runs on the element's dragend; update the logic so the same cleanup
(setIsDragging(false), setDragOverPath(null), dragPathsRef.current = []) is also
performed when the pointer leaves the entire tree container and when global
drag/keyboard cancellation occurs: add/extend handleDragLeave (or a new
onContainerDragLeave) to detect leaving the full container and call the same
cleanup, and register document-level listeners for 'dragend' and 'keyup'
(checking for Escape) during drag start to call handleDragEnd cleanup and remove
those listeners on cleanup; reference handleDragEnd, handleDragLeave,
dragPathsRef, setIsDragging and setDragOverPath to locate where to add the
duplicated cleanup and listener registration/removal.
In `@apps/web/components/task/file-context-menu.tsx`:
- Around line 116-119: handleConfirmDelete currently always calls
deleteNodeOptimistically with node.path, so when isBulk is true the dialog
"Delete N items?" only deletes the single context-menu target; update the
component API and handler so bulk deletes remove all selected paths—either add a
new prop selectedPaths: string[] (or similar) and pass that array into a bulk
delete helper, or add a dedicated onDeleteFiles(selectedPaths) callback and call
it from handleConfirmDelete when isBulk true; modify handleConfirmDelete to
check isBulk and call the bulk deletion path (using selectedPaths or
onDeleteFiles) otherwise fall back to deleteNodeOptimistically(node.path), and
ensure the prop names (selectedCount, selectedPaths, onDeleteFiles) are wired
where the component is used.
In `@apps/web/components/task/file-tree-utils.ts`:
- Around line 117-146: The moveNodesInTree implementation can produce identical
target paths when sourcePaths contain files with the same basename, causing the
second insertNodeInTree call to overwrite the first; update moveNodesInTree to
detect duplicate computed newPath values before calling
renameSubtree/insertNodeInTree and either (a) abort with a clear error listing
colliding basenames or (b) disambiguate by generating unique target names (e.g.,
append numeric suffixes like "name (1).ext") and use those unique newPath values
when calling renameSubtree so insertNodeInTree receives non-colliding nodes;
reference moveNodesInTree, sourcePaths, renameSubtree, insertNodeInTree and
ensure the chosen strategy updates nodesToMove accordingly so
removeNodeFromTree/insert operations remain consistent.
In `@apps/web/e2e/tests/changes-panel-multi-select.spec.ts`:
- Around line 18-33: The exec method currently force-deletes Git's index.lock
(lockPath) which can corrupt a concurrently running git process; remove the
fs.existsSync(lockPath) and fs.unlinkSync(lockPath) calls and instead detect the
presence of lockPath, and when present treat it as lock contention: wait/retry
(use the existing execSync("sleep 0.2") delay) up to three attempts before
failing; keep the try/catch and the existing message check for "index.lock" and
only retry/continue when lock contention is detected (and attempt < 2),
otherwise rethrow the error — reference function exec, variable lockPath, and
the execSync("sleep 0.2") retry logic to locate changes.
---
Nitpick comments:
In `@apps/web/components/task/changes-panel-timeline.tsx`:
- Around line 296-472: The FileListSection has grown too large—extract the
bulk-selection UI into its own component file: move DefaultActionButtons and
BulkActionBar into a new module (e.g., BulkSelectionControls) and export them
(keeping their prop signatures intact), then import and use them from
changes-panel-timeline.tsx; ensure FileListSection still passes the same props
(actionLabel/isActionLoading/onAction,
onBulkStage/onBulkUnstage/onBulkDiscard/onClearSelection) and preserve
data-testid attributes and behavior (selection clearing after actions) so
staged/unstaged flows and tests continue to work.
In `@apps/web/components/task/file-context-menu.tsx`:
- Around line 51-90: The DeleteConfirmDialog is receiving fileCount (computed by
countFilesInTree(node)) even when isBulk is true and the value is unused; fix by
computing fileCount only when needed—either stop passing fileCount for bulk
operations or move the call to countFilesInTree(node) inside DeleteConfirmDialog
and wrap it in a lazy calculation (e.g., useMemo) that runs only when !isBulk
and node is present; update the calling code that currently invokes
countFilesInTree and the DeleteConfirmDialog signature (symbols:
DeleteConfirmDialog, countFilesInTree, fileCount, node, isBulk) so the expensive
count is skipped for bulk deletions.
- Around line 36-49: The optimistic delete in deleteNodeOptimistically currently
reverts state silently; update it to notify the user on failure by calling the
app's toast/error notifier (e.g., toast.error or showToast) inside both the
.then(!ok) branch and the .catch branch after you setTree(snapshot). Keep the
existing snapshot rollback via setTree(snapshot) and add a concise error message
that includes the file path (or a brief description) to give context; reference
deleteNodeOptimistically, setTree, onDeleteFile, removeNodeFromTree, and
snapshot when making the change.
In `@apps/web/e2e/tests/file-tree-multi-select.spec.ts`:
- Around line 47-50: The commit(message: string) method interpolates message
into a shell string which can break for quotes; change it to pass the message as
a safe argument instead of inline interpolation—use the existing exec helper
with an array-argument form or switch to execFileSync/execFile with args (e.g.,
call git commit -m with message as a separate arg) and keep returning exec("git
rev-parse HEAD").trim(); update the commit function to build the command using
arguments rather than injecting message into the shell string.
🪄 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: 996a5f58-5125-4575-a104-bbfb68b529dd
📒 Files selected for processing (11)
apps/web/components/task/changes-panel-file-row.tsxapps/web/components/task/changes-panel-timeline.tsxapps/web/components/task/changes-panel.tsxapps/web/components/task/file-browser-parts.tsxapps/web/components/task/file-browser.tsxapps/web/components/task/file-context-menu.tsxapps/web/components/task/file-tree-utils.tsapps/web/e2e/pages/session-page.tsapps/web/e2e/tests/changes-panel-multi-select.spec.tsapps/web/e2e/tests/file-tree-multi-select.spec.tsapps/web/hooks/use-multi-select.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/components/task/file-browser-parts.tsx (1)
134-136:⚠️ Potential issue | 🟠 MajorNested tree nodes inherit parent's selection state instead of being independently evaluated.
At line 417,
FileTreeViewcorrectly evaluatesisSelected?.(child.path)to a boolean for each direct child. However, at line 135,TreeNodeChildrenspreads{...props}(containing the parent's evaluated boolean) to nestedTreeNodeItemelements. This causes nested children to receive the parent's selection state rather than their own path being evaluated.Since
isSelectedis used inTreeNodeRowline 153 for styling (isSelected && "bg-accent"), descendants of a selected node incorrectly receive the same blue background color.The root cause is a type mismatch:
FileBrowserContentAreaProps.isSelectedis a function, butTreeNodeItemProps.isSelectedis a boolean. The evaluation happens at the top level, then the boolean spreads to all descendants.Fix: Either pass the original function through the tree (requiring type changes), or evaluate at render time in each
TreeNodeItemrather than spreading the computed result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/file-browser-parts.tsx` around lines 134 - 136, The bug is that TreeNodeChildren spreads {...props} which contains a boolean isSelected evaluated at the parent, so nested TreeNodeItem instances inherit the parent's selection state; fix by ensuring each TreeNodeItem evaluates selection by path instead of receiving the parent's boolean: remove isSelected from the spread and pass a per-child computed boolean (e.g. isSelected?.(child.path)) or better, change the prop contract to forward the original FileBrowserContentAreaProps.isSelected function down and have TreeNodeItem/TreeNodeRow call isSelected(path) themselves (update types for TreeNodeItemProps to accept a function), and adjust TreeNodeRow usage of isSelected accordingly so descendants compute their own selection rather than inheriting the parent value.
🧹 Nitpick comments (2)
apps/web/hooks/use-multi-select.ts (1)
46-52:setSelectedPathsbypasses pruning — consider documenting this behavior.When
setSelectedPathsis called externally, it setsrawSelectiondirectly without pruning. The prunedselectedPathswill be derived on the next render, butonSelectionChangeis immediately called with the unpruned value.This is likely intentional (callers like drag-and-drop clear selection with
new Set()), but worth documenting for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/hooks/use-multi-select.ts` around lines 46 - 52, The setSelectedPaths callback directly assigns to rawSelection and invokes onSelectionChange with the unpruned Set, which bypasses the pruning logic that produces selectedPaths; add a clear comment or JSDoc above the setSelectedPaths definition (referencing setSelectedPaths, setRawSelection, rawSelection, selectedPaths, and onSelectionChange) stating that this intentionally skips pruning so callers (e.g., drag-and-drop) can pass raw selections like new Set(), and note that pruned selectedPaths will be derived on the next render and that onSelectionChange receives the unpruned value immediately.apps/web/components/task/file-browser.tsx (1)
299-319: Consider stabilizingmultiSelectdependency inuseKeyboardShortcuts.The
multiSelectobject is recreated on each render, causing the effect to re-run and re-attach the listener. Since onlyclearSelectionandselectAllare used, consider passing those callbacks directly or memoizing the hook's return value inuseMultiSelect.This is a minor performance consideration — the current implementation is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/file-browser.tsx` around lines 299 - 319, The effect in useKeyboardShortcuts is re-running because the multiSelect object identity changes each render; change the hook to accept stable callbacks or memoize multiSelect: either (A) change the signature of useKeyboardShortcuts(containerRef, { clearSelection, selectAll }: { clearSelection: () => void; selectAll: () => void }) and pass multiSelect.clearSelection and multiSelect.selectAll from the caller, or (B) ensure useMultiSelect returns a stable object by wrapping its handlers in useCallback/useMemo so multiSelect (the value returned by useMultiSelect) has stable identity; update references to multiSelect.clearSelection and multiSelect.selectAll accordingly to use the new stable callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/components/task/file-context-menu.tsx`:
- Around line 120-126: When handling bulk deletes in file-context-menu, avoid
deleting parents before their nested children: before looping over selectedPaths
in the isBulk branch, sort the paths by depth descending and remove any paths
that are descendants of another selected path (deduplicate by ancestor
containment) so deleteNodeOptimistically(tree, setTree, p, onDeleteFile) is
invoked from deepest to shallowest on unique targets; update the selectedPaths
iteration to use this processed list to prevent removeNodeFromTree/restore
snapshot failures.
---
Outside diff comments:
In `@apps/web/components/task/file-browser-parts.tsx`:
- Around line 134-136: The bug is that TreeNodeChildren spreads {...props} which
contains a boolean isSelected evaluated at the parent, so nested TreeNodeItem
instances inherit the parent's selection state; fix by ensuring each
TreeNodeItem evaluates selection by path instead of receiving the parent's
boolean: remove isSelected from the spread and pass a per-child computed boolean
(e.g. isSelected?.(child.path)) or better, change the prop contract to forward
the original FileBrowserContentAreaProps.isSelected function down and have
TreeNodeItem/TreeNodeRow call isSelected(path) themselves (update types for
TreeNodeItemProps to accept a function), and adjust TreeNodeRow usage of
isSelected accordingly so descendants compute their own selection rather than
inheriting the parent value.
---
Nitpick comments:
In `@apps/web/components/task/file-browser.tsx`:
- Around line 299-319: The effect in useKeyboardShortcuts is re-running because
the multiSelect object identity changes each render; change the hook to accept
stable callbacks or memoize multiSelect: either (A) change the signature of
useKeyboardShortcuts(containerRef, { clearSelection, selectAll }: {
clearSelection: () => void; selectAll: () => void }) and pass
multiSelect.clearSelection and multiSelect.selectAll from the caller, or (B)
ensure useMultiSelect returns a stable object by wrapping its handlers in
useCallback/useMemo so multiSelect (the value returned by useMultiSelect) has
stable identity; update references to multiSelect.clearSelection and
multiSelect.selectAll accordingly to use the new stable callbacks.
In `@apps/web/hooks/use-multi-select.ts`:
- Around line 46-52: The setSelectedPaths callback directly assigns to
rawSelection and invokes onSelectionChange with the unpruned Set, which bypasses
the pruning logic that produces selectedPaths; add a clear comment or JSDoc
above the setSelectedPaths definition (referencing setSelectedPaths,
setRawSelection, rawSelection, selectedPaths, and onSelectionChange) stating
that this intentionally skips pruning so callers (e.g., drag-and-drop) can pass
raw selections like new Set(), and note that pruned selectedPaths will be
derived on the next render and that onSelectionChange receives the unpruned
value immediately.
🪄 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: 38a4f12b-3bce-40c9-8e97-d6625ff0bc6e
📒 Files selected for processing (9)
apps/web/components/task/changes-panel-file-row.tsxapps/web/components/task/changes-panel-timeline.tsxapps/web/components/task/file-browser-parts.tsxapps/web/components/task/file-browser.tsxapps/web/components/task/file-context-menu.tsxapps/web/e2e/pages/session-page.tsapps/web/e2e/tests/changes-panel-multi-select.spec.tsapps/web/e2e/tests/file-tree-multi-select.spec.tsapps/web/hooks/use-multi-select.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/e2e/tests/changes-panel-multi-select.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/e2e/pages/session-page.ts
- apps/web/e2e/tests/file-tree-multi-select.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/components/task/file-browser-parts.tsx (1)
386-435: Good extraction of FileTreeView component.The extraction improves readability and keeps
FileBrowserContentAreafocused on routing between search results, load states, and tree view. One minor observation: line 420 evaluatesprops.isSelected?.(child.path)on every render for each child. This is fine for typical tree sizes, but if performance becomes a concern with very large trees, consider passing the function reference and evaluating insideTreeNodeIteminstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/file-browser-parts.tsx` around lines 386 - 435, FileTreeView currently calls props.isSelected for every child (isSelected={props.isSelected?.(child.path)}), causing per-node evaluation on each render; instead pass the function reference down and let TreeNodeItem evaluate it. Change the prop passed to TreeNodeItem to isSelected={props.isSelected} (or isSelectedFn if you prefer) and update TreeNodeItem to call that function with the node path (e.g., const selected = props.isSelected?.(node.path)) so evaluation happens inside TreeNodeItem and can be optimized/memoized there.apps/web/components/task/changes-panel-file-row.tsx (1)
266-266: UnnecessaryuseMemofor array conversion.The
useMemowith[selectedPaths]dependency recreates the array whenever theSetreference changes, which happens on every selection change. Since[...selectedPaths]is a trivial O(n) operation that's only used when a button is clicked, the memoization adds overhead without benefit.Consider inlining the spread in the click handlers instead:
♻️ Proposed simplification
export function BulkActionBar({ variant, selectionCount, selectedPaths, onBulkStage, onBulkUnstage, onBulkDiscard, }: { variant: "unstaged" | "staged"; selectionCount: number; selectedPaths: Set<string>; onBulkStage?: (paths: string[]) => void; onBulkUnstage?: (paths: string[]) => void; onBulkDiscard?: (paths: string[]) => void; }) { - const paths = useMemo(() => [...selectedPaths], [selectedPaths]); return ( <div data-testid={`bulk-actions-${variant}`} className="flex items-center gap-1.5"> <span className="text-[11px] text-muted-foreground">{selectionCount} selected</span> {variant === "unstaged" && onBulkStage && ( <Button data-testid="bulk-stage" size="sm" variant="outline" className="h-6 text-[11px] px-2.5 gap-1 cursor-pointer" - onClick={() => onBulkStage(paths)} + onClick={() => onBulkStage([...selectedPaths])} > Stage {selectionCount} </Button> )} {variant === "staged" && onBulkUnstage && ( <Button data-testid={`bulk-unstage-${variant}`} size="sm" variant="outline" className="h-6 text-[11px] px-2.5 gap-1 cursor-pointer" - onClick={() => onBulkUnstage(paths)} + onClick={() => onBulkUnstage([...selectedPaths])} > Unstage {selectionCount} </Button> )} {onBulkDiscard && ( <Button data-testid={`bulk-discard-${variant}`} size="sm" variant="outline" className="h-6 text-[11px] px-2.5 gap-1 cursor-pointer text-destructive hover:text-destructive" - onClick={() => onBulkDiscard(paths)} + onClick={() => onBulkDiscard([...selectedPaths])} > Discard {selectionCount} </Button> )} </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/changes-panel-file-row.tsx` at line 266, The useMemo call creating const paths = useMemo(() => [...selectedPaths], [selectedPaths]) is unnecessary and should be removed; instead, directly spread selectedPaths where needed (e.g., in the click handlers that currently reference paths) to build a fresh array at the moment of the button click — replace uses of paths with [...selectedPaths] (or a locally named array inside the handler) and delete the useMemo import/definition to avoid extra overhead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/components/task/changes-panel-file-row.tsx`:
- Line 266: The useMemo call creating const paths = useMemo(() =>
[...selectedPaths], [selectedPaths]) is unnecessary and should be removed;
instead, directly spread selectedPaths where needed (e.g., in the click handlers
that currently reference paths) to build a fresh array at the moment of the
button click — replace uses of paths with [...selectedPaths] (or a locally named
array inside the handler) and delete the useMemo import/definition to avoid
extra overhead.
In `@apps/web/components/task/file-browser-parts.tsx`:
- Around line 386-435: FileTreeView currently calls props.isSelected for every
child (isSelected={props.isSelected?.(child.path)}), causing per-node evaluation
on each render; instead pass the function reference down and let TreeNodeItem
evaluate it. Change the prop passed to TreeNodeItem to
isSelected={props.isSelected} (or isSelectedFn if you prefer) and update
TreeNodeItem to call that function with the node path (e.g., const selected =
props.isSelected?.(node.path)) so evaluation happens inside TreeNodeItem and can
be optimized/memoized there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6b01e86-b932-4bfe-94e3-96fd0138b729
📒 Files selected for processing (4)
apps/web/components/task/changes-panel-file-row.tsxapps/web/components/task/changes-panel-timeline.tsxapps/web/components/task/file-browser-parts.tsxapps/web/e2e/tests/changes-panel-multi-select.spec.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/e2e/tests/changes-panel-multi-select.spec.ts
zeval
force-pushed
the
feature/multi-entity-selecti-eyb
branch
from
f82751e to
7dd75d7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/web/components/task/file-context-menu.tsx (1)
120-126:⚠️ Potential issue | 🟡 MinorBulk delete may fail silently for nested paths.
When deleting multiple paths where one is a parent of another (e.g.,
diranddir/file.txt), the iteration order is non-deterministic. If the parent directory is deleted first, the nested file'sremoveNodeFromTreewill silently fail to find the node, and snapshot restoration would be incorrect.Consider sorting paths by depth (deepest first) or deduplicating paths that are descendants of other selected paths.
🛡️ Proposed fix to handle nested paths
if (isBulk && selectedPaths) { + // Sort deepest paths first to avoid parent deletion invalidating child paths + const sortedPaths = [...selectedPaths].sort( + (a, b) => b.split("/").length - a.split("/").length + ); + for (const p of sortedPaths) { - for (const p of selectedPaths) { deleteNodeOptimistically(tree, setTree, p, onDeleteFile); } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/file-context-menu.tsx` around lines 120 - 126, The bulk-delete loop can fail when selectedPaths contains parent and descendant paths; before calling deleteNodeOptimistically (used with tree, setTree, onDeleteFile) sort and deduplicate selectedPaths so deeper paths are deleted before their parents: compute path depth (e.g., count separators), sort descending, and also remove any path that is a descendant of another selected path (keep only the ancestor or only the deepest entry as desired) so removeNodeFromTree inside deleteNodeOptimistically will always find the node and snapshots restore correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/components/task/file-context-menu.tsx`:
- Around line 120-126: The bulk-delete loop can fail when selectedPaths contains
parent and descendant paths; before calling deleteNodeOptimistically (used with
tree, setTree, onDeleteFile) sort and deduplicate selectedPaths so deeper paths
are deleted before their parents: compute path depth (e.g., count separators),
sort descending, and also remove any path that is a descendant of another
selected path (keep only the ancestor or only the deepest entry as desired) so
removeNodeFromTree inside deleteNodeOptimistically will always find the node and
snapshots restore correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b154159-3d0d-4526-a0cb-3f343180f571
📒 Files selected for processing (11)
apps/web/components/task/changes-panel-file-row.tsxapps/web/components/task/changes-panel-timeline.tsxapps/web/components/task/changes-panel.tsxapps/web/components/task/file-browser-parts.tsxapps/web/components/task/file-browser.tsxapps/web/components/task/file-context-menu.tsxapps/web/components/task/file-tree-utils.tsapps/web/e2e/pages/session-page.tsapps/web/e2e/tests/changes-panel-multi-select.spec.tsapps/web/e2e/tests/file-tree-multi-select.spec.tsapps/web/hooks/use-multi-select.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/e2e/tests/file-tree-multi-select.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/components/task/changes-panel.tsx
- apps/web/components/task/file-tree-utils.ts
- apps/web/hooks/use-multi-select.ts
- apps/web/e2e/pages/session-page.ts
|
Claude finished @zeval's task in 4m 33s —— View job FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix blockers first The nested selection highlight bug (#1) means multi-select is visually non-functional for any file inside an expanded directory — the central feature of this PR. Bulk discard without confirmation (#2) is an irreversible data-loss risk that the PR's own test plan calls out as a requirement. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/e2e/tests/changes-panel-multi-select.spec.ts (1)
84-223: Please cover the other branches of the new click contract.This suite only proves Ctrl/Cmd toggling. The PR also changes Shift-click range selection and the plain-click fallback that should clear selection and still open the diff. If
handleClickever starts always reporting the click as “consumed”, these tests keep passing while row navigation regresses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/e2e/tests/changes-panel-multi-select.spec.ts` around lines 84 - 223, Add tests that exercise the other click-contract branches: add one test using Shift-click on session.changesFileRow(...) to select a contiguous range (click first, Shift-click last) and assert intermediate rows are selected and bulk actions appear; add another test that verifies a plain click on a non-selected row clears any existing selection (use session.changesSelectedRows()) but still opens the diff panel (assert diff opens) to guard the handleClick contract; use the same helpers used in this file (session.changesFileRow, session.changesBulkActionBar, session.changesSelectedRows, session.clickTab("Changes")) so the tests fail if handleClick incorrectly always reports clicks as consumed.apps/web/components/task/changes-panel-file-row.tsx (1)
221-295: Extract the shared action-button classes.
"h-6 text-[11px] px-2.5 gap-1 cursor-pointer"is now repeated acrossDefaultActionButtonsandBulkActionBar, so the next styling change has four places to drift.♻️ Suggested extraction
+const ACTION_BUTTON_CLASS = "h-6 text-[11px] px-2.5 gap-1 cursor-pointer"; + export function DefaultActionButtons({ actionLabel, isActionLoading, @@ <Button size="sm" variant="outline" - className="h-6 text-[11px] px-2.5 gap-1 cursor-pointer" + className={ACTION_BUTTON_CLASS} onClick={onAction} disabled={isActionLoading} > @@ <Button size="sm" variant="outline" - className="h-6 text-[11px] px-2.5 gap-1 cursor-pointer" + className={ACTION_BUTTON_CLASS} onClick={onSecondaryAction} disabled={isSecondaryActionLoading} > @@ <Button data-testid="bulk-stage" size="sm" variant="outline" - className="h-6 text-[11px] px-2.5 gap-1 cursor-pointer" + className={ACTION_BUTTON_CLASS} onClick={() => onBulkStage(paths)} > @@ <Button data-testid={`bulk-unstage-${variant}`} size="sm" variant="outline" - className="h-6 text-[11px] px-2.5 gap-1 cursor-pointer" + className={ACTION_BUTTON_CLASS} onClick={() => onBulkUnstage(paths)} >As per coding guidelines, "No duplicated strings (≥4 occurrences)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/task/changes-panel-file-row.tsx` around lines 221 - 295, The repeated class string "h-6 text-[11px] px-2.5 gap-1 cursor-pointer" should be extracted to a single constant and reused in both DefaultActionButtons and BulkActionBar; add e.g. const ACTION_BUTTON_CLASSES = "h-6 text-[11px] px-2.5 gap-1 cursor-pointer" near the top of the module and replace the literal className values in DefaultActionButtons (the Button in that component) and all Buttons inside BulkActionBar with className={ACTION_BUTTON_CLASSES} (or concatenated when additional classes like text-destructive are required) so the style is centralized and not duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/components/task/changes-panel-file-row.tsx`:
- Around line 221-295: The repeated class string "h-6 text-[11px] px-2.5 gap-1
cursor-pointer" should be extracted to a single constant and reused in both
DefaultActionButtons and BulkActionBar; add e.g. const ACTION_BUTTON_CLASSES =
"h-6 text-[11px] px-2.5 gap-1 cursor-pointer" near the top of the module and
replace the literal className values in DefaultActionButtons (the Button in that
component) and all Buttons inside BulkActionBar with
className={ACTION_BUTTON_CLASSES} (or concatenated when additional classes like
text-destructive are required) so the style is centralized and not duplicated.
In `@apps/web/e2e/tests/changes-panel-multi-select.spec.ts`:
- Around line 84-223: Add tests that exercise the other click-contract branches:
add one test using Shift-click on session.changesFileRow(...) to select a
contiguous range (click first, Shift-click last) and assert intermediate rows
are selected and bulk actions appear; add another test that verifies a plain
click on a non-selected row clears any existing selection (use
session.changesSelectedRows()) but still opens the diff panel (assert diff
opens) to guard the handleClick contract; use the same helpers used in this file
(session.changesFileRow, session.changesBulkActionBar,
session.changesSelectedRows, session.clickTab("Changes")) so the tests fail if
handleClick incorrectly always reports clicks as consumed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 418a13a5-6940-4240-a932-4a404247dab5
📒 Files selected for processing (5)
apps/web/components/task/changes-panel-file-row.tsxapps/web/components/task/file-browser-parts.tsxapps/web/e2e/tests/changes-panel-multi-select.spec.tsapps/web/e2e/tests/file-tree-multi-select.spec.tsapps/web/hooks/use-multi-select.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/e2e/tests/file-tree-multi-select.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/hooks/use-multi-select.ts
- apps/web/components/task/file-browser-parts.tsx
Add Shift-click range and Ctrl/Cmd-click toggle selection to the file tree and staged/unstaged changes sections. Includes bulk actions (delete, stage, unstage, discard), drag-and-drop file moving into directories, keyboard shortcuts (Escape, Ctrl+A), and E2E tests.
- Keep original file-row-* data-testid for backwards compat - Remove eager onClearSelection from bulk actions (auto-pruned by hook) - Variant-scope bulk-discard/bulk-unstage test ids - Remove index.lock deletion from E2E GitHelper retry loop
- Create files AFTER opening session so WS detects git status changes
- Add proper git env vars (GIT_AUTHOR_NAME, etc.) matching existing tests
- Use element.press("Escape") instead of page keyboard for proper focus
- Remove drag-to-directory test (Playwright dragTo doesn't support HTML5
dataTransfer events needed by our DnD implementation)
…andler - Use text-accent-foreground on selected rows for dark mode readability - Override child span colors with [&_span]:text-accent-foreground - Use hover:bg-accent/80 instead of hover:bg-muted on selected items - Skip selection handling on right-click (button === 2) so context menu works without clearing multi-selection
Plain click now clears any selection and performs the original action (open file / toggle folder / open diff). Selection only happens with modifier keys. handleClick returns boolean to indicate whether the click was consumed by selection logic.
zeval
force-pushed
the
feature/multi-entity-selecti-eyb
branch
from
7e55b54 to
33aaf14
Compare
|
Claude finished @zeval's task in 4m 10s —— View job FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix blockers first The nested selection highlight bug (#1) means multi-select is visually non-functional for any file inside an expanded directory — the core feature doesn't work in the most common case. Bulk discard without confirmation (#2) is an irreversible data-loss risk the PR's own test plan flags as a requirement. Missing unit tests (#3) are required per CLAUDE.md for hooks and utility functions. |
|
Claude finished @zeval's task in 3m 53s —— View job PR Review
FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
What's fixed vs. prior reviews
Summary
Verdict: Blocked — add unit tests for |
- Add unit tests for computeNextSelection (8 tests covering ctrl-click, shift-click range, stale anchor, reverse range, shift+ctrl extend) - Extract shared E2E helpers (GitHelper, makeGitEnv, openTaskSession, createStandardProfile) to e2e/helpers/git-helper.ts - Fix misleading comment in bulk delete (says "re-insert only failed" but implementation restores full snapshot)
There was a problem hiding this comment.
zeval has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Claude finished @zeval's task in 5m 1s —— View job PR Review
FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
What's fixed vs. prior reviews
Summary
Verdict: Blocked — add |
FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
What's fixed vs. prior reviews
Summary
Verdict: Blocked — add |
Contributor
Author
|
@claude reevaluate your comment based on the replies |
|
Claude finished @zeval's task in 2m 43s —— View job PR Review Reevaluation
FindingsRemaining Concern
Suggestion (unchanged)
What's confirmed fixed ✅
Summary
Verdict: Near ready. The remaining gap is hook-level test coverage for |
Individual .catch() handlers could restore already-deleted files if a later delete failed. Now uses Promise.all so rollback only happens if any delete rejects.
There was a problem hiding this comment.
zeval has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Claude finished @zeval's task in 3m 59s —— View job PR Review
FindingsBlocker (must fix before merge)
What's fixed vs. prior reviews
Summary
Verdict: Blocked — add |
- Stale-path pruning: items shrink → selected paths auto-pruned - Plain click: returns false, clears existing selection - selectAll/clearSelection: work correctly, fire onSelectionChange - Install @testing-library/react for renderHook support
There was a problem hiding this comment.
zeval has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Claude finished @zeval's task in 3m 47s —— View job PR Review
FindingsSuggestion (recommended, doesn't block)
What's fixed
Summary
Verdict: Ready to merge — all blockers resolved. One minor suggestion on bulk delete rollback path. |
FindingsSuggestion (recommended, doesn't block)
What's fixed vs. prior reviews
Summary
Verdict: Ready to merge — all previously flagged blockers are resolved. One remaining suggestion (false-return rollback in bulk delete) is non-blocking. |
There was a problem hiding this comment.
zeval has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Claude finished @zeval's task in 4m 46s —— View job PR Review
FindingsSuggestion (recommended, doesn't block)
What's fixed vs. prior reviews
Summary
Verdict: Ready to merge — all previously flagged blockers are resolved. One minor unused dependency to clean up if desired. |
| "@playwright/test": "^1.58.2", | ||
| "@tailwindcss/postcss": "^4", | ||
| "@testing-library/react": "^16.3.2", | ||
| "@testing-library/react-hooks": "^8.0.1", |
There was a problem hiding this comment.
Unused dev dependency — @testing-library/react-hooks@8.0.1 was added but is never imported anywhere. renderHook comes from @testing-library/react directly (line 2 of use-multi-select.test.ts). The react-hooks package was superseded by the built-in renderHook in @testing-library/react v13+.
Remove it:
Suggested change
| "@testing-library/react-hooks": "^8.0.1", |
And remove its entry from pnpm-lock.yaml.
Contributor
Author
There was a problem hiding this comment.
Fixed in 3df81c0 — removed @testing-library/react-hooks. renderHook comes from @testing-library/react directly.
There was a problem hiding this comment.
zeval has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Claude finished @zeval's task in 4m 2s —— View job PR Review
FindingsWhat's fixed vs. prior reviews
Summary
Verdict: Ready to merge — every blocker from prior review cycles has been resolved. The last outstanding suggestion (unused |
FindingsWhat's fixed vs. prior reviews
Summary
Verdict: Ready to merge — all blockers from prior reviews are resolved. The unused dependency flagged in the last review was cleaned up in |
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
1 participant
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.
Summary
useMultiSelecthook handles all selection logic with stale-path pruning viauseMemoTest plan
file-tree-multi-select.spec.ts— ctrl-click toggle, escape clear, drag-to-movechanges-panel-multi-select.spec.ts— ctrl-click + bulk bar, bulk stage, escape clearScreenshots