feat: can multi-select deployments when pressing shift #1994
Conversation
WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant User
participant DeploymentList
participant useListSelection
participant DeploymentListRow
User->>DeploymentListRow: Click checkbox (no Shift)
DeploymentListRow->>DeploymentList: onSelectDeployment({ id, isShiftPressed: false })
DeploymentList->>useListSelection: selectItem({ id, isShiftPressed: false })
useListSelection->>useListSelection: toggleSingleSelection(id)
useListSelection-->>DeploymentList: selectedItemIds updated
DeploymentList-->>DeploymentListRow: checked state updated
User->>DeploymentListRow: Click checkbox (Shift held)
DeploymentListRow->>DeploymentList: onSelectDeployment({ id, isShiftPressed: true })
DeploymentList->>useListSelection: selectItem({ id, isShiftPressed: true })
useListSelection->>useListSelection: changeMultipleSelection(id)
Note over useListSelection: Select range from<br/>intervalSelectionAnchor to id
useListSelection-->>DeploymentList: selectedItemIds includes range
DeploymentList-->>DeploymentListRow: checked state for range updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: New hook implementation with non-trivial range-selection logic (interval anchors, deduplication, directional range handling); multiple interdependent component updates; prop signature changes affecting multiple call sites; comprehensive test suite adds verification scope; requires careful validation of edge cases in range selection and state management. Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
**/*.spec.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-17T10:04:00.909ZApplied to files:
🧬 Code graph analysis (2)apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (1)
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (7)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/deploy-web/src/components/deployments/DeploymentList.tsx (1)
207-217: Remove the redundant index check.Line 214 checks
dseqIndex !== undefined, but this check is redundant becauseisBetweenDseqsalready validates all indices (once the bug inisBetweenDseqsis fixed to check for-1).Apply this diff to simplify the logic:
const isRowHighlightedForMultipleSelection = useCallback( (dseq: string) => { if (!isShiftPressed || !hoveredDelpoymentDseq || !lastSelectedDeploymentDseq) { return false; } - const dseqIndex = currentPageDeployments?.findIndex(d => d.dseq === dseq); - return dseqIndex !== undefined && isBetweenDseqs(dseq, hoveredDelpoymentDseq, lastSelectedDeploymentDseq); + return isBetweenDseqs(dseq, hoveredDelpoymentDseq, lastSelectedDeploymentDseq); }, [isShiftPressed, hoveredDelpoymentDseq, lastSelectedDeploymentDseq, currentPageDeployments, isBetweenDseqs] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/src/components/deployments/DeploymentList.tsx(6 hunks)apps/deploy-web/src/components/deployments/DeploymentListRow.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/components/deployments/DeploymentListRow.tsxapps/deploy-web/src/components/deployments/DeploymentList.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/components/deployments/DeploymentListRow.tsxapps/deploy-web/src/components/deployments/DeploymentList.tsx
🧬 Code graph analysis (1)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (2)
apps/deploy-web/src/types/provider.ts (1)
ApiProviderList(185-273)apps/provider-console/src/utils/styleUtils.ts (1)
cn(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (9)
apps/deploy-web/src/components/deployments/DeploymentList.tsx (4)
2-2: LGTM!The imports for
useCallbackanduniqare correct and align with the new range-selection feature.Also applies to: 21-21
133-146: LGTM!The range selection logic is well-implemented:
- Correctly uses Shift key state to determine range vs. single selection
- Updates
lastSelectedDeploymentDseqonly when Shift is not pressed, allowing sequential range selections- Uses
uniqto deduplicate selected dseqsNote: This logic depends on the
isBetweenDseqshelper being fixed (see previous comment).
180-201: LGTM!The global Shift key tracking is correctly implemented:
- Proper event listener setup and cleanup
- Correctly updates state on keydown/keyup
- Standard pattern for tracking key state
364-365: LGTM!The props
onMouseEnterandisHighlightedare correctly passed toDeploymentListRow, matching the updated component signature.apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (5)
16-16: LGTM!The
cnutility import is correct and aligns with the new conditional row highlighting feature.
54-55: LGTM!The Props interface updates are correct:
onSelectDeploymentnow acceptseventShiftPressedto support Shift-based range selectionrefreshDeploymentstype improved fromanyto() => void(aligns with coding guidelines)- New
onMouseEnterandisHighlightedprops support hover tracking and row highlightingAlso applies to: 58-58, 60-60
63-72: LGTM!The component destructuring correctly includes the new
onMouseEnterandisHighlightedprops.
183-190: LGTM!The
TableRowrendering correctly implements hover tracking and conditional highlighting:
cnutility properly applies conditional background class whenisHighlightedis trueonMouseEnterhandler correctly uses optional chaining and passesdeployment.dseq
147-147: LGTM!The event handler updates are correct:
- Checkbox
onClickcorrectly captures and passesevent.shiftKeyto support range selectiononCloseDeploymentcorrectly passesfalseforeventShiftPressedwhen unselectingAlso applies to: 318-318
jzsfkzm
force-pushed
the
features/1982-select-multiple-deployments
branch
from
9aadf4e to
9c45413
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/src/components/deployments/DeploymentList.tsx(7 hunks)apps/deploy-web/src/components/deployments/DeploymentListRow.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/components/deployments/DeploymentList.tsxapps/deploy-web/src/components/deployments/DeploymentListRow.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/components/deployments/DeploymentList.tsxapps/deploy-web/src/components/deployments/DeploymentListRow.tsx
🧬 Code graph analysis (1)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (2)
apps/deploy-web/src/types/provider.ts (1)
ApiProviderList(185-273)apps/provider-console/src/utils/styleUtils.ts (1)
cn(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
|
hey @jzsfkzm I've tested this and the logic feels a bit weird when it comes to some edge cases. I've asked ChatGPT to get me some common flow instructions below to make it more clear. wdut? Row Multiselect (Shift) — LogicOverviewWhen Shift is pressed, clicking a row selects a continuous range between the first selected row (the anchor) and the currently clicked row. This lets users select multiple consecutive rows in one action. Behavior
ExamplesExample 1 — Simple Downward Selection
Outcome: A continuous block from 2 → 5. Example 2 — Reversing Direction
Outcome: Works the same upward — direction doesn’t matter. Example 3 — Expanding, Then Contracting
Outcome: The range shrinks or grows depending on the new target, but always remains continuous from the same anchor. Example 4 — New Anchor After Normal Click
Outcome: A plain click resets both selection and anchor. Example 5 — Jumping Around
Outcome: Each Shift-click recalculates the range from the same anchor (Row 3). Example 6 — First Click is Shifted
Outcome: If Shift-click happens first, it behaves as a normal click for the initial anchor. Key Points
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14087 @ygrishajev, I've reviewed the current implementation against the proposed specification. There are several key differences that explain the "weird" behavior you're experiencing: Critical Issues1. Shift-click is additive, not replacement ❌Current behavior: Each shift-click extends the selection setSelectedDeploymentDseqs(prev => {
return checked ? uniq(prev.concat(dseqs)) : prev.filter(x => !dseqs.includes(x));
});Expected behavior: Each shift-click should replace the previous range Example of the problem:
2. Normal click doesn't clear existing selections ❌Current behavior: Clicking a checkbox toggles only that row Expected behavior: A normal click (without Shift) should:
Example of the problem:
3. Cannot distinguish between checking and unchecking flowsThe spec implies that normal clicks should always clear and select, but the current implementation allows unchecking by clicking an already-checked row. Need to clarify this edge case. Suggested Implementation ApproachYou'll need to track the previous shift-selected range to replace it: const [shiftSelectedDseqs, setShiftSelectedDseqs] = useState<string[]>([]);
const onSelectDeployment = (checked: boolean, dseq: string, eventShiftPressed: boolean) => {
if (eventShiftPressed && lastSelectedDeploymentDseq) {
// Compute new range
const newRange = currentPageDeployments
.filter(d => isBetweenDseqs(d.dseq, dseq, lastSelectedDeploymentDseq))
.map(d => d.dseq);
setSelectedDeploymentDseqs(prev => {
// Remove previous shift-range, add new range
const withoutPrevRange = prev.filter(x => !shiftSelectedDseqs.includes(x));
return checked ? uniq([...withoutPrevRange, ...newRange]) : withoutPrevRange;
});
// Remember this range for next shift-click
setShiftSelectedDseqs(newRange);
} else {
// Normal click: clear all and select only this row
if (checked) {
setSelectedDeploymentDseqs([dseq]);
setLastSelectedDeploymentDseq(dseq);
setShiftSelectedDseqs([]);
} else {
// Unchecking: remove only this row
setSelectedDeploymentDseqs(prev => prev.filter(x => x !== dseq));
}
}
};Questions to Clarify
The proposed spec looks solid and aligns with standard multi-select UX patterns (similar to file managers, spreadsheets, etc.). The implementation just needs these adjustments to match the behavior. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 30291 @ygrishajev, I've reviewed the implementation against the specification you provided, and I've identified several deviations that likely cause the "weird edge cases" you're experiencing. Key Issues1. Normal clicks don't clear existing selection (Violates Behavior #4)Specification says: "Normal Click (no Shift): Clears any existing selection. Selects only the clicked row." Current behavior: Normal clicks add to or remove from the existing selection instead of clearing it. In setSelectedDeploymentDseqs(prev => {
return checked ? uniq(prev.concat(dseqs)) : prev.filter(x => !dseqs.includes(x));
});This concatenates ( Example:
2. Shift-clicks extend instead of replace (Violates Behavior #3)Specification says: "Each Shift-click replaces, not extends, the previous selection range." Current behavior: Shift-clicks concatenate to existing selection using Example:
3. The
|
| Action | Result |
|---|---|
| Click Row 3 | selectedDeploymentDseqs = [3], anchor = 3 ✅ |
| Shift+Click Row 7 | selectedDeploymentDseqs = [3,4,5,6,7], anchor = 3 ✅ |
| Shift+Click Row 5 | selectedDeploymentDseqs = [3,4,5], anchor = 3 ✅ (replaced!) |
And Example 4:
| Action | Result |
|---|---|
| Click Row 4 | selectedDeploymentDseqs = [4], anchor = 4 ✅ |
| Shift+Click Row 8 | selectedDeploymentDseqs = [4,5,6,7,8], anchor = 4 ✅ |
| Click Row 2 | selectedDeploymentDseqs = [2], anchor = 2 ✅ (cleared!) |
| Shift+Click Row 5 | selectedDeploymentDseqs = [2,3,4,5], anchor = 2 ✅ |
Additional Consideration
You may also want to handle the checkbox click behavior in DeploymentListRow.tsx (line 260):
onClick={event => {
event.stopPropagation();
onSelectDeployment?.(!checked, deployment.dseq, event.shiftKey);
}}With the spec's behavior, clicking an already-checked checkbox should deselect only that row (without clearing others), while a normal row click should clear all and select one. You might need separate handlers for checkbox vs. row clicks, or adjust the logic to match your desired UX.
Would you like me to generate the complete updated code for both files, or would you prefer to have the team make these adjustments?
|
@coderabbitai what do you think of the spec provided generally? Would you consider this to be a common approach? |
|
Tip For best results, initiate chat on the files or code changes. @ygrishajev: Yes, absolutely! This is the standard approach for shift-click multi-select, and it's used consistently across many mature applications: Common ImplementationsFile Managers:
Email Clients:
Productivity Apps:
Design Tools:
Why It's Effective
Your Spec QualityThe specification you provided is excellent. It's:
One Additional Edge Case to ConsiderCtrl/Cmd + Click (Multi-Select):
This is optional but worth considering if users need to select non-sequential items before bulk actions (e.g., close rows 2, 5, 9, 12). Bottom line: Your spec describes industry-standard behavior. Implementing it will give users a familiar, efficient experience. The issues I identified earlier are just implementation bugs—the design itself is solid. 👍 |
jzsfkzm
force-pushed
the
features/1982-select-multiple-deployments
branch
from
9c45413 to
9d3d66b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/src/components/deployments/DeploymentList.tsx(6 hunks)apps/deploy-web/src/components/deployments/DeploymentListRow.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/components/deployments/DeploymentList.tsxapps/deploy-web/src/components/deployments/DeploymentListRow.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/components/deployments/DeploymentList.tsxapps/deploy-web/src/components/deployments/DeploymentListRow.tsx
🧬 Code graph analysis (1)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (1)
apps/deploy-web/src/types/provider.ts (1)
ApiProviderList(185-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (4)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (2)
56-56: Good type narrowing forrefreshDeployments.Changing from
anyto() => voidimproves type safety and adheres to the coding guideline prohibitingany.As per coding guidelines.
53-53: LGTM: Shift-key integration.The updated signature threads
eventShiftPressedcorrectly, and the checkbox handler properly surfacesevent.shiftKeyto the parent for range-selection logic.Also applies to: 299-299
apps/deploy-web/src/components/deployments/DeploymentList.tsx (2)
210-222: LGTM: Global keyup listener resets shift state.Resetting
lastIntervalSelectionDseqswhen the Shift key is released ensures that the next Shift-click starts a fresh range from the anchor. The cleanup in the return properly removes the listener.
119-147: LGTM: Helper functions for range calculation.The
indexOfDseq,isBetweenDseqs, andcurrentPageDeploymentsBetweenhelpers correctly compute the continuous block between two dseqs, handling bidirectional ranges (upward/downward selection). The index validation (!== -1) is correct.
@ygrishajev I see why you said it's weird. Re-selecting the same interval, or an overlapping interval indeed was behaving in unexpected ways. I changed it a bit, now I think it's better, will you check too? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 29637 @jzsfkzm, great work on the updates! I've reviewed the changes and can confirm you've made solid progress. Let me break down what's working now and what might need clarification: ✅ Fixed Issues1. Shift-click now replaces ranges correctlyThe setSelectedDeploymentDseqs(prev => {
return uniq(prev.filter(x => !dseqsToUncheck.includes(x)).concat(dseqsToCheck));
});Example validation (from the spec):
This now matches the expected behavior perfectly! 2. Tracking shift-selected rangesYou're correctly tracking
|
jzsfkzm
force-pushed
the
features/1982-select-multiple-deployments
branch
from
9d3d66b to
06e2c0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/deploy-web/src/components/deployments/DeploymentList.tsx (2)
149-154: Anchor should update on every normal click (and consider reset-on-click spec).Currently anchor updates only when checked is true. Update anchor on any normal click, or adopt the “normal click clears and selects only clicked row” behavior as discussed.
const toggleSingleSelection = (checked: boolean, dseq: string) => { setSelectedDeploymentDseqs(prev => (checked ? [...prev, dseq] : prev.filter(x => x !== dseq))); - if (checked) { - setIntervalSelectionAnchor(dseq); - } + setIntervalSelectionAnchor(dseq); };
156-168: Shift-click should replace selection and needs empty-range fallback.Per spec, Shift-click replaces the whole selection with the continuous range. Also, when the anchor isn’t on the current page, fall back to selecting the clicked row. Proposed fix:
const changeMultipleSelection = (dseq: string) => { if (!intervalSelectionAnchor) { return; } - const dseqsToCheck = currentPageDeploymentsBetween(dseq, intervalSelectionAnchor); - const dseqsToUncheck = lastIntervalSelectionDseqs; - - setSelectedDeploymentDseqs(prev => { - return uniq(prev.filter(x => !dseqsToUncheck.includes(x)).concat(dseqsToCheck)); - }); - setLastIntervalSelectionDseqs(dseqsToCheck); + const range = currentPageDeploymentsBetween(dseq, intervalSelectionAnchor); + const finalSelection = range.length > 0 ? range : [dseq]; + setSelectedDeploymentDseqs(finalSelection); + setLastIntervalSelectionDseqs(finalSelection); };If you adopt this, uniq becomes unnecessary here.
🧹 Nitpick comments (2)
apps/deploy-web/src/components/deployments/DeploymentList.tsx (2)
21-21: Use modular lodash import (or drop it if no longer needed).Prefer importing only uniq to avoid pulling the full lodash bundle; if you apply the Shift-click replacement above, uniq may be removable.
-import { uniq } from "lodash"; +import uniq from "lodash/uniq";Based on learnings.
170-176: Optional: support Ctrl/Cmd+Click for non-contiguous selection.If desired, detect metaKey/ctrlKey here to toggle individual rows without clearing selection, alongside the Shift range behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/src/components/deployments/DeploymentList.tsx(6 hunks)apps/deploy-web/src/components/deployments/DeploymentListRow.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/components/deployments/DeploymentListRow.tsxapps/deploy-web/src/components/deployments/DeploymentList.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/components/deployments/DeploymentListRow.tsxapps/deploy-web/src/components/deployments/DeploymentList.tsx
🧬 Code graph analysis (1)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (1)
apps/deploy-web/src/types/provider.ts (1)
ApiProviderList(185-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (3)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (2)
296-301: Confirm UX: normal click toggle vs. reset-and-select.Row forwards Shift state and toggles via !checked. If you adopt the “reset-and-select on normal click” spec, this handler should always pass true and let the parent replace selection.
53-57: All onSelectDeployment call sites updated. No stale usages remain.apps/deploy-web/src/components/deployments/DeploymentList.tsx (1)
210-222: Good: keyup cleanup for Shift-range tracking.Listener registration/cleanup looks correct.
jzsfkzm
force-pushed
the
features/1982-select-multiple-deployments
branch
from
29da9d2 to
196f41d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/deploy-web/src/hooks/useListSelection/useListSelection.ts (2)
67-76: Consider implementing canonical single-click behavior.Per the PR objectives, the canonical spec expects a normal (non-Shift) click to clear all existing selections and select only the clicked item, then set it as the new anchor. The current implementation toggles instead.
If the team decides on Option B (canonical spec), update
toggleSingleSelectionto clear-and-select:-const toggleSingleSelection = useCallback( +const selectSingleItem = useCallback( (id: T) => { - setSelectedItemIds(prev => { - const isAdding = !prev.includes(id); - if (isAdding) { - setIntervalSelectionAnchor(id); - } - return isAdding ? [...prev, id] : prev.filter(x => x !== id); - }); + setSelectedItemIds([id]); + setIntervalSelectionAnchor(id); setLastIntervalSelectionIds([]); }, [] );Then rename the callback in
onSelectItemaccordingly. If Option A (toggle) is preferred, keep the current approach but apply the fixes from the previous comment.
1-2: Consider replacing lodashuniqwith native Set.Using
uniqfrom lodash works correctly, but for minimal bundle size you could use native JS.-import { uniq } from "lodash";Then at line 61:
-return uniq([...withoutLast, ...newRange]); +return Array.from(new Set([...withoutLast, ...newRange]));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/deploy-web/src/components/deployments/DeploymentList.tsx(5 hunks)apps/deploy-web/src/components/deployments/DeploymentListRow.tsx(3 hunks)apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts(1 hunks)apps/deploy-web/src/hooks/useListSelection/useListSelection.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/src/components/deployments/DeploymentList.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.tsapps/deploy-web/src/hooks/useListSelection/useListSelection.tsapps/deploy-web/src/components/deployments/DeploymentListRow.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.tsapps/deploy-web/src/hooks/useListSelection/useListSelection.tsapps/deploy-web/src/components/deployments/DeploymentListRow.tsx
🧬 Code graph analysis (2)
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts (1)
apps/deploy-web/src/hooks/useListSelection/useListSelection.ts (2)
useListSelection(8-91)UseListSelectionProps(4-6)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (1)
apps/deploy-web/src/types/provider.ts (1)
ApiProviderList(185-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (3)
apps/deploy-web/src/components/deployments/DeploymentListRow.tsx (2)
56-56: Good type fix!Replacing
anywith() => voidimproves type safety.As per coding guidelines.
53-53: Prop signature updates look correct.The changes correctly adopt the new
useListSelectionhook API, passing{ id, isShiftPressed }objects instead of the old(isChecked, dseq)parameters.Also applies to: 135-135, 297-300
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts (1)
27-34: Review comment is incorrect—test validates the implemented behavior (Option A, additive toggle).The implementation clearly uses Option A (additive toggle) behavior:
toggleSingleSelectionchecks whether an item exists and adds or removes it accordingly (line 43 in useListSelection.ts). The test case correctly validates this—the sequence of clicks [2, 3, 4, 3] produces [2, 4] because clicking item 3 twice toggles it off. All shift-click tests confirm the additive approach viachangeMultipleSelectionusinguniq([...prev, ...newRange]).The suggested changes would break the test while the implementation remains unchanged. There is no evidence in the codebase supporting a decision to implement Option B.
Likely an incorrect or invalid review comment.
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/hooks/useListSelection/useListSelection.spec.ts
Outdated
Show resolved
Hide resolved
also other small changes regarding PR feedback refs akash-network#1982
jzsfkzm
force-pushed
the
features/1982-select-multiple-deployments
branch
from
196f41d to
ea823c5
Compare
3 participants
closes #1982
Summary by CodeRabbit
New Features
Refactor