refactor: Refactor shift selection ♻️ #3554

lethemanh · 2025-10-13T06:22:01Z

Change:

Moved all logics that're related to shift selection out of SelectionProvider.
Implemented new hook and helper functions for shift selection.
Implemented unit test for shift selection.
Modified useLongPress to be available with shift selection.
Modified folder view component to trigger shift selection.

bundlemon · 2025-10-13T06:24:58Z

BundleMon

Files updated (2)

Status	Path	Size	Limits
✅	static/js/main.(hash).js	145.67KB (+110B +0.07%)	-
✅	public/static/js/public.(hash).js	109.36KB (+47B +0.04%)	-

Unchanged files (19)

Status	Path	Size	Limits
✅	static/js/(chunkId).(hash).js	958.28KB	-
✅	static/js/cozy.(hash).js	870.82KB	-
✅	public/static/js/(chunkId).(hash).js	870.09KB	-
✅	public/static/js/cozy.(hash).js	727.75KB	-
✅	(hash).js	336.11KB	-
✅	public/(hash).js	336.11KB	-
✅	services/qualificationMigration.js	282.45KB	-
✅	services/dacc.js	262.26KB	-
✅	public/static/js/lib-react.(hash).js	39.24KB	-
✅	static/js/lib-react.(hash).js	39.24KB	-
✅	public/static/css/cozy.(hash).css	33.38KB	-
✅	static/css/cozy.(hash).css	33.38KB	-
✅	public/static/js/lib-router.(hash).js	21.86KB	-
✅	static/js/lib-router.(hash).js	21.86KB	-
✅	static/css/main.(hash).css	16.77KB	-
✅	public/static/css/public.(hash).css	6.46KB	-
✅	manifest.webapp	2.96KB	-
✅	index.html	690B	-
✅	assets/manifest.json	185B	-

Total files change +162B 0%

Groups updated (1)

Status	Path	Size	Limits
✅	*/.js	6.74MB (+161B 0%)	-

Unchanged groups (2)

Status	Path	Size	Limits
✅	*/.{png,svg,ico}	2.17MB	-
✅	*/.css	129.68KB	-

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

src/hooks/useShiftSelection/helpers.ts

rezk2ll · 2025-10-13T07:03:47Z

src/hooks/useShiftSelection/helpers.ts

+    newSelected[targetItem._id] = targetItem
+  } else {
+    // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
+    delete newSelected[targetItem._id]


Please don't use delete, look here

rezk2ll · 2025-10-13T11:12:54Z

src/hooks/useShiftSelection/helpers.ts

+ * Clamps an index value to be within valid array bounds.
+ *
+ * @param maxLength - The maximum length of the array
+ * @param index - The index to clamp


I would prefer a human generated JSDoc .

param {type} name description

rezk2ll

Testing locally, the shift select is working fine with double click, kudos for that 🎉

Thanks for the type definitions and the JSDoc attempt

But I have some remarks to point out:

Making more functions does not mean that it will be easier to understand; having more than enough functions will split the mind of the person reading the code.
Let's not abuse useMemo for things that are not necessary
Try to avoid useEffect as much as possible; we already have enough.
In the code, we are mixing responsibilities in functions. You have functions that are one-line return something, and functions that do more than they should do like toggleSelection
keyboard listener is bloated
The state is overcomplicated
isSelectAll state can be computed instead of computing it in an useEffect
not all functions have JSDoc ( or a proper one with types )
The useshiftselect hook should be tested

rezk2ll · 2025-10-14T14:03:04Z

src/hooks/useShiftSelection/index.tsx

+  }, [lastInteractedItem, items])
+
+  const selectItemByIds: SelectedItems = useMemo(() => {
+    return selectedItems.reduce<SelectedItems>(


Are you sure selectedItems is an array?

it is an object in the selection provider

const [selectedItems, setSelectedItems] = useState({})

rezk2ll · 2025-10-14T14:04:10Z

src/modules/selection/types.ts

+  isSelectionBarVisible: boolean
+
+  /** List of selected items as an array */
+  selectedItems: IOCozyFile[]


it's not an array it's an object

rezk2ll · 2025-10-14T14:12:05Z

src/hooks/useShiftSelection/index.tsx

+  const viewTypeRef = useRef<ViewType>()
+  viewTypeRef.current = useMemo(() => viewType, [viewType])
+
+  const itemsRef = useRef<IOCozyFile[]>()
+  itemsRef.current = useMemo(() => items, [items])


This is unnecessary and double-storing the value

rezk2ll · 2025-10-14T14:16:13Z

src/hooks/useShiftSelection/index.tsx

+  useEffect(() => {
+    setIsSelectAll(selectedItems.length === itemsRef.current?.length)
+  }, [setIsSelectAll, selectedItems])


Please avoid useEffect

rezk2ll · 2025-10-14T14:31:16Z

src/modules/views/Folder/virtualized/Grid.jsx

-              >
-                <GridFile
+    const onToggleSelect = (fileId, event) => {
+      event.stopPropagation()


why do you need to do it here outside the onSelect?

rezk2ll · 2025-10-14T14:32:50Z

src/modules/views/Folder/virtualized/helpers.js

-    ...row,
-    index
-  }))
+  return rows


does this change have other impacts on virtualized table? @JF-Cozy

rezk2ll · 2025-10-14T14:33:02Z

src/modules/views/Folder/virtualized/Table.jsx

-
-  useShiftArrowsSelection({ items: sortedRow, viewType: 'list' }, tableRef)
+    const handleRowSelect = (row, event) => {
+      event.stopPropagation()


same comment

rezk2ll · 2025-10-14T14:34:08Z

src/modules/views/Folder/FolderViewBody.jsx


+  const onToggleSelect = (fileId, e) => {
+    setLastInteractedItem(fileId)
+    if (e.shiftKey) {


we shouldn't check the keyboard event key in each place we use the onShiftClick

rezk2ll · 2025-10-14T15:47:05Z

src/hooks/useShiftSelection/index.tsx

+  const viewTypeRef = useRef<ViewType>()
+  viewTypeRef.current = useMemo(() => viewType, [viewType])
+
+  const itemsRef = useRef<IOCozyFile[]>()


Suggested change

const itemsRef = useRef<IOCozyFile[]>()

const itemsRef = useRef<IOCozyFile[]>([])

rezk2ll · 2025-10-14T15:55:08Z

src/hooks/useShiftSelection/helpers.ts

+
+/**
+ * Returns a new SelectedItems object with the specified key removed.
+ * This is a safe way to remove items from selection without using delete operator.


AI comment slipped in.

rezk2ll · 2025-10-14T15:56:37Z

src/hooks/useShiftSelection/index.tsx

+  onShiftClick: (clickedItemId: string) => void
+}
+
+const useShiftSelection = (


lethemanh self-assigned this Oct 13, 2025

lethemanh requested review from JF-Cozy, doubleface, lenhanphung, rezk2ll and zatteo as code owners October 13, 2025 06:22

lethemanh force-pushed the refactor-shift-selection branch 3 times, most recently from 6e220e3 to c640640 Compare October 13, 2025 07:01

rezk2ll reviewed Oct 13, 2025

View reviewed changes

lethemanh force-pushed the refactor-shift-selection branch 2 times, most recently from 7502202 to 2c4808e Compare October 13, 2025 08:23

lethemanh requested a review from rezk2ll October 13, 2025 08:26

lethemanh force-pushed the refactor-shift-selection branch from 2c4808e to 1aa9225 Compare October 13, 2025 09:09

rezk2ll reviewed Oct 13, 2025

View reviewed changes

lethemanh force-pushed the refactor-shift-selection branch from 1aa9225 to b2a9217 Compare October 14, 2025 04:51

refactor: Refactor shift selection ♻️

a5960c1

lethemanh force-pushed the refactor-shift-selection branch from b2a9217 to a5960c1 Compare October 14, 2025 07:55

lethemanh requested a review from rezk2ll October 14, 2025 08:01

rezk2ll requested changes Oct 14, 2025

View reviewed changes

rezk2ll reviewed Oct 14, 2025

View reviewed changes

src/hooks/useShiftSelection/index.tsx

onShiftClick: (clickedItemId: string) => void

}

const useShiftSelection = (

Copy link

Contributor

rezk2ll Oct 14, 2025

Choose a reason for hiding this comment

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

JSDoc

	const itemsRef = useRef<IOCozyFile[]>()
	const itemsRef = useRef<IOCozyFile[]>([])

refactor: Refactor shift selection ♻️ #3554

Are you sure you want to change the base?

refactor: Refactor shift selection ♻️ #3554

Conversation

lethemanh commented Oct 13, 2025

Change:

Uh oh!

bundlemon bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BundleMon

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rezk2ll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bundlemon bot commented Oct 13, 2025 •

edited

Loading