Add range selection functionality to repository page#843
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds Shift‑click multi-range selection to the snapshots table by introducing ChangesSnapshot Selection Enhancement
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/components/snapshots-table.tsx`:
- Around line 105-135: handleSnapshotSelection currently ignores a shift-click
when lastSelectedId is stale (lastIndex === -1), causing the click to be lost;
modify handleSnapshotSelection so that when isShiftClick is true but either
lastIndex or currentIndex is -1 you fall back to the single-toggle behavior:
compute newSelected from selectedIds, toggle snapshotId presence, call
setSelectedIds(newSelected), and then always call setLastSelectedId(snapshotId);
reference handleSnapshotSelection, lastSelectedId, snapshots, selectedIds,
setSelectedIds, and setLastSelectedId when making the change.
- Around line 203-215: The Checkbox is receiving both onClick and
onCheckedChange so shift-click triggers range selection in onClick then a toggle
in onCheckedChange that removes the clicked snapshot; to fix, remove the
onCheckedChange prop from the Checkbox and move all selection logic
(single-toggle and shift-range behavior) into the onClick handler used now (keep
using handleSnapshotSelection(snapshot.short_id, e) from the click path and
update that function if needed to accept e and decide between range vs toggle),
ensuring aria-label stays unchanged and controlled checked prop (isSelected)
remains.
🪄 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: 83494167-8661-4b43-be9b-ebcfa896af93
📒 Files selected for processing (1)
app/client/components/snapshots-table.tsx
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/client/components/snapshots-table.tsx (1)
105-144: Stale-anchor fallback looks correct; consider deduping the two identical toggle blocks.The explicit
elsebranch at 122-131 cleanly resolves the prior silent-no-op whenlastSelectedIdpoints at a snapshot that was filtered out or deleted, and the unconditionalsetLastSelectedId(snapshotId)at 143 keeps the anchor fresh in every path. One tidy-up opportunity: lines 122-131 and 134-141 are byte-identical toggle logic, so a small early-return restructure removes the duplication without changing behavior.♻️ Optional dedup
const handleSnapshotSelection = (snapshotId: string, event?: React.MouseEvent | React.KeyboardEvent) => { const isShiftClick = event && "shiftKey" in event && event.shiftKey; if (isShiftClick && lastSelectedId) { - // Range selection const lastIndex = snapshots.findIndex((s) => s.short_id === lastSelectedId); const currentIndex = snapshots.findIndex((s) => s.short_id === snapshotId); if (lastIndex !== -1 && currentIndex !== -1) { const start = Math.min(lastIndex, currentIndex); const end = Math.max(lastIndex, currentIndex); - const rangeIds = new Set(snapshots.slice(start, end + 1).map((s) => s.short_id)); - - // Add selected range to existing selection const newSelected = new Set(selectedIds); - rangeIds.forEach((id) => newSelected.add(id)); + for (let i = start; i <= end; i++) newSelected.add(snapshots[i].short_id); setSelectedIds(newSelected); - } else { - // Fallback to single-toggle when range selection fails due to stale lastSelectedId - const newSelected = new Set(selectedIds); - if (newSelected.has(snapshotId)) { - newSelected.delete(snapshotId); - } else { - newSelected.add(snapshotId); - } - setSelectedIds(newSelected); + setLastSelectedId(snapshotId); + return; } - } else { - // Single selection toggle - const newSelected = new Set(selectedIds); - if (newSelected.has(snapshotId)) { - newSelected.delete(snapshotId); - } else { - newSelected.add(snapshotId); - } - setSelectedIds(newSelected); } + // Single-toggle (default, and fallback when shift-range can't resolve the anchor). + const newSelected = new Set(selectedIds); + if (newSelected.has(snapshotId)) newSelected.delete(snapshotId); + else newSelected.add(snapshotId); + setSelectedIds(newSelected); setLastSelectedId(snapshotId); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/components/snapshots-table.tsx` around lines 105 - 144, The duplicate toggle logic in handleSnapshotSelection can be removed by restructuring to handle the range-selection case first and early-return; specifically, inside handleSnapshotSelection use the isShiftClick && lastSelectedId branch to compute lastIndex/currentIndex and, if both are valid, perform the range add-to-selectedIds (using snapshots, selectedIds, and setSelectedIds) and then setLastSelectedId(snapshotId) and return, otherwise fall through to a single shared toggle block that computes newSelected from selectedIds, toggles snapshotId, calls setSelectedIds(newSelected) and then setLastSelectedId(snapshotId); this preserves behavior while eliminating the duplicated toggle code in the fallback and single-selection paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/client/components/snapshots-table.tsx`:
- Around line 105-144: The duplicate toggle logic in handleSnapshotSelection can
be removed by restructuring to handle the range-selection case first and
early-return; specifically, inside handleSnapshotSelection use the isShiftClick
&& lastSelectedId branch to compute lastIndex/currentIndex and, if both are
valid, perform the range add-to-selectedIds (using snapshots, selectedIds, and
setSelectedIds) and then setLastSelectedId(snapshotId) and return, otherwise
fall through to a single shared toggle block that computes newSelected from
selectedIds, toggles snapshotId, calls setSelectedIds(newSelected) and then
setLastSelectedId(snapshotId); this preserves behavior while eliminating the
duplicated toggle code in the fallback and single-selection paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29279969-0859-4d0f-aa2b-cdf0b3c48348
📒 Files selected for processing (1)
app/client/components/snapshots-table.tsx
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@nicotsx Haven't gotten the chance to visually test it (looks like its harder to run on windows now compared to the last time I contributed), but it should be visually fine. |
nicotsx
left a comment
nicotsx
left a comment
There was a problem hiding this comment.
LGTM however I think the behavior is not complete.
Initial selection:
When nothing is selected yet and I shift click row 7, it should select all rows from 1 to 7.
Deselection:
When a range is selected (eg 1 to 7) if I shift click on 5, the range should change to 1-5. Currently it doesn't allow me to unselect
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@nicotsx sorry for the delay, was pretty busy. Should be implemented now. |
|
Thanks for the update @Rajdave69 |
2 participants
This is a small addition that lets a user shift click to select a range of snapshots, on the repositories' snapshot page.
Summary by CodeRabbit
New Features
Bug Fixes
Style