feat: adding a drag and drop with some animations#156
Open
Kareem2003 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR implements custom note and folder ordering with drag-and-drop reordering. Settings gains ChangesCustom Note and Folder Ordering with Drag Reordering
Sequence DiagramsequenceDiagram
participant User
participant Sidebar
participant DndContext
participant NotesContext
participant Backend
User->>Sidebar: drag note/folder
Sidebar->>DndContext: onDragOver with pointer position
DndContext->>Sidebar: handleDragOver computes dragOverInfo
User->>Sidebar: drop at target position
Sidebar->>Sidebar: handleDragEnd detects same parent reorder
Sidebar->>NotesContext: updateSettings with new noteOrder/folderOrder
NotesContext->>Backend: notesService.updateSettings
Backend-->>NotesContext: settings persisted
NotesContext-->>Sidebar: reloadSettings refreshes UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/layout/Sidebar.tsx (1)
96-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMulti-select drag now only acts on the active note.
The overlay still shows
+Nfrom the current selection, buthandleDragEndonly reorders/movesactiveData.id. Dragging a selected batch will therefore imply a multi-item move while silently leaving the rest behind. Either restore iterating the selected IDs here, or collapse drag state to a single note inhandleDragStartso the UI matches the behavior.Also applies to: 176-252, 607-618
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/Sidebar.tsx` around lines 96 - 123, handleDragStart currently only sets drag state for the active item even when a multi-select is displayed; update it so when the dragged note is part of an existing multi-selection you record the whole selection (not just the active id). Specifically, in handleDragStart (and similarly in the other drag handlers noted), when selected.has(noteId) && selected.size > 1 call setMultiSelectedNoteIds(new Set(selected)) and setDragCount(selected.size) (instead of only setting the count), otherwise setMultiSelectedNoteIds(new Set([noteId])) as before; then update handleDragEnd to operate over multiSelectedNoteIds (or the preserved selection set) when performing reorder/move so the UI + overlay (+N) matches the actual moved items. Include the same fix in the other occurrences referenced (the blocks around lines ~176-252 and ~607-618).
🧹 Nitpick comments (1)
src/lib/folderTree.ts (1)
57-66: ⚡ Quick winPrecompute order indexes before sorting to reduce tree-build cost.
indexOfinside comparators makes each sort far more expensive than needed. BuildMap<string, number>fornoteOrder/folderOrderonce and reuse.Also applies to: 72-78, 87-96, 99-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/folderTree.ts` around lines 57 - 66, The comparator currently calls folderOrder.indexOf (and noteOrder.indexOf in other comparators) for every pair which is O(n^2); precompute a Map<string, number> (e.g., folderOrderIndex and noteOrderIndex) before sorting and use map.get(path) (or map.has(path)) inside the comparator instead of indexOf; when an entry is missing treat its index as -1 (or a sentinel) and keep the existing tie-breaker using name.localeCompare; apply the same change for the other comparators that reference noteOrder/folderOrder (the blocks around the existing node.children.sort and the ranges mentioned) so all sorts use the precomputed index maps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/lib.rs`:
- Around line 968-990: The comparator for notes.sort_by does repeated linear
scans via order.iter().position(...) which makes sorting O(n^2); instead build a
one-time index (e.g., HashMap or Vec<Option<usize>>) from note_order before
calling notes.sort_by and replace the position(...) calls with O(1) lookups into
that index (use the index to obtain Option<usize> for a.id and b.id), keeping
the existing pinned_ids logic and fallback to b.modified.cmp(&a.modified) when
both are unordered.
In `@src/components/notes/NoteList.tsx`:
- Around line 251-254: The droppable metadata currently hardcodes path: "" in
the useDroppable call (setDropRef) which causes Sidebar.handleDragEnd to treat
drops as root moves; change the metadata to derive the note's real parent folder
from the note id (the same way FolderTreeView.FileItem does) and set data.path
to that parent path so nested notes dragged in flat/search mode preserve their
actual parent instead of being treated as root moves.
In `@src/context/NotesContext.tsx`:
- Around line 118-123: In updateSettings's catch block, guard the rollback fetch
by storing the previous settings before doing the optimistic set, then wrap the
call to notesService.getSettings() in its own try/catch: on failure restore the
saved previousSettings via setSettings(prevSettings) and setError to a
user-friendly message (include that the rollback fetch failed optionally),
otherwise use the fetched settings; reference the updateSettings function, the
catch block that calls notesService.getSettings(), and setSettings/setError to
locate where to add the saved previousSettings and the inner try/catch fallback.
- Around line 104-111: The catch block in refreshSettings currently only logs
the error, which leaves stale settings in the UI; update the error handling so
that on failure you clear the stale settings by calling setSettings(null) (or an
appropriate empty state) and surface a user-facing error (e.g., set a new
settingsError state or call the app's notification/snackbar helper) with the
error message; modify the refreshSettings function to call
notesService.getSettings, then on catch do both setSettings(null) and
setSettingsError(err) or invoke showToast/showErrorNotification with the error
so callers/components can render a friendly message.
In `@src/hooks/useFlipAnimation.ts`:
- Around line 7-96: useLayoutEffect in useFlipAnimation currently runs on every
commit causing unnecessary reflows; change the effect to depend on the hook
argument (the _dependency) or a derived stable order key so the FLIP logic only
runs when rendered order/layout actually changes. Concretely, update the
useLayoutEffect call in useFlipAnimation to include [_dependency] (or a computed
stable ordering identifier derived from _dependency) as its dependency array,
and ensure rectsRef and containerRef remain accessible inside the effect so you
still measure, animate, and then update rectsRef.current only when the
dependency indicates a layout change.
---
Outside diff comments:
In `@src/components/layout/Sidebar.tsx`:
- Around line 96-123: handleDragStart currently only sets drag state for the
active item even when a multi-select is displayed; update it so when the dragged
note is part of an existing multi-selection you record the whole selection (not
just the active id). Specifically, in handleDragStart (and similarly in the
other drag handlers noted), when selected.has(noteId) && selected.size > 1 call
setMultiSelectedNoteIds(new Set(selected)) and setDragCount(selected.size)
(instead of only setting the count), otherwise setMultiSelectedNoteIds(new
Set([noteId])) as before; then update handleDragEnd to operate over
multiSelectedNoteIds (or the preserved selection set) when performing
reorder/move so the UI + overlay (+N) matches the actual moved items. Include
the same fix in the other occurrences referenced (the blocks around lines
~176-252 and ~607-618).
---
Nitpick comments:
In `@src/lib/folderTree.ts`:
- Around line 57-66: The comparator currently calls folderOrder.indexOf (and
noteOrder.indexOf in other comparators) for every pair which is O(n^2);
precompute a Map<string, number> (e.g., folderOrderIndex and noteOrderIndex)
before sorting and use map.get(path) (or map.has(path)) inside the comparator
instead of indexOf; when an entry is missing treat its index as -1 (or a
sentinel) and keep the existing tie-breaker using name.localeCompare; apply the
same change for the other comparators that reference noteOrder/folderOrder (the
blocks around the existing node.children.sort and the ranges mentioned) so all
sorts use the precomputed index maps.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eed8fd23-85c3-4e8b-b5ea-dd844cd608c0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src-tauri/src/lib.rssrc/components/layout/Sidebar.tsxsrc/components/notes/FolderTreeView.tsxsrc/components/notes/NoteList.tsxsrc/components/ui/index.tsxsrc/context/NotesContext.tsxsrc/hooks/useFlipAnimation.tssrc/lib/folderTree.tssrc/types/note.ts
Comment on lines
+968
to
+990
| // Sort: pinned notes first, then unpinned notes (sorted by note_order, fallback to modified date) | ||
| notes.sort_by(|a, b| { | ||
| let a_pinned = pinned_ids.contains(&a.id); | ||
| let b_pinned = pinned_ids.contains(&b.id); | ||
|
|
||
| match (a_pinned, b_pinned) { | ||
| (true, false) => std::cmp::Ordering::Less, // a pinned, b not -> a first | ||
| (false, true) => std::cmp::Ordering::Greater, // b pinned, a not -> b first | ||
| _ => b.modified.cmp(&a.modified), // both same status -> sort by date (newest first) | ||
| _ => { | ||
| // both same status (both pinned or both unpinned) | ||
| if let Some(ref order) = note_order { | ||
| let a_idx = order.iter().position(|id| id == &a.id); | ||
| let b_idx = order.iter().position(|id| id == &b.id); | ||
| match (a_idx, b_idx) { | ||
| (Some(i), Some(j)) => i.cmp(&j), | ||
| (Some(_), None) => std::cmp::Ordering::Less, // ordered items first | ||
| (None, Some(_)) => std::cmp::Ordering::Greater, // ordered items first | ||
| (None, None) => b.modified.cmp(&a.modified), // both unordered -> newest first | ||
| } | ||
| } else { | ||
| b.modified.cmp(&a.modified) // sort by date (newest first) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid repeated linear scans in the sort comparator.
order.iter().position(...) runs on every comparison, which turns sorting into a much heavier operation for large note sets. Build an index map once and use O(1) lookups in the comparator.
Proposed optimization
- notes.sort_by(|a, b| {
+ let order_index: Option<HashMap<&str, usize>> = note_order.as_ref().map(|order| {
+ order.iter().enumerate().map(|(i, id)| (id.as_str(), i)).collect()
+ });
+
+ notes.sort_by(|a, b| {
let a_pinned = pinned_ids.contains(&a.id);
let b_pinned = pinned_ids.contains(&b.id);
match (a_pinned, b_pinned) {
(true, false) => std::cmp::Ordering::Less,
(false, true) => std::cmp::Ordering::Greater,
_ => {
- if let Some(ref order) = note_order {
- let a_idx = order.iter().position(|id| id == &a.id);
- let b_idx = order.iter().position(|id| id == &b.id);
+ if let Some(ref idx) = order_index {
+ let a_idx = idx.get(a.id.as_str()).copied();
+ let b_idx = idx.get(b.id.as_str()).copied();
match (a_idx, b_idx) {
(Some(i), Some(j)) => i.cmp(&j),
(Some(_), None) => std::cmp::Ordering::Less,
(None, Some(_)) => std::cmp::Ordering::Greater,
(None, None) => b.modified.cmp(&a.modified),
}
} else {
b.modified.cmp(&a.modified)
}
}
}
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/lib.rs` around lines 968 - 990, The comparator for
notes.sort_by does repeated linear scans via order.iter().position(...) which
makes sorting O(n^2); instead build a one-time index (e.g., HashMap or
Vec<Option<usize>>) from note_order before calling notes.sort_by and replace the
position(...) calls with O(1) lookups into that index (use the index to obtain
Option<usize> for a.id and b.id), keeping the existing pinned_ids logic and
fallback to b.modified.cmp(&a.modified) when both are unordered.
Comment on lines
+251
to
+254
| const { setNodeRef: setDropRef } = useDroppable({ | ||
| id: `drop-note:${id}`, | ||
| data: { type: "note", id: id, path: "" }, | ||
| }); |
There was a problem hiding this comment.
Preserve the note’s real parent in droppable metadata.
Hard-coding path: "" breaks drag handling for nested notes in flat/search mode. Sidebar.handleDragEnd treats that as “move this note to root”, so dragging foo/bar onto another search result from foo becomes a cross-folder move instead of a reorder within foo. Derive the parent from id here, like FolderTreeView.FileItem already does.
Suggested change
+ const noteParentFolder =
+ id.includes("/") ? id.substring(0, id.lastIndexOf("/")) : "";
+
const { setNodeRef: setDropRef } = useDroppable({
id: `drop-note:${id}`,
- data: { type: "note", id: id, path: "" },
+ data: { type: "note", id, path: noteParentFolder },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { setNodeRef: setDropRef } = useDroppable({ | |
| id: `drop-note:${id}`, | |
| data: { type: "note", id: id, path: "" }, | |
| }); | |
| const noteParentFolder = | |
| id.includes("/") ? id.substring(0, id.lastIndexOf("/")) : ""; | |
| const { setNodeRef: setDropRef } = useDroppable({ | |
| id: `drop-note:${id}`, | |
| data: { type: "note", id, path: noteParentFolder }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/notes/NoteList.tsx` around lines 251 - 254, The droppable
metadata currently hardcodes path: "" in the useDroppable call (setDropRef)
which causes Sidebar.handleDragEnd to treat drops as root moves; change the
metadata to derive the note's real parent folder from the note id (the same way
FolderTreeView.FileItem does) and set data.path to that parent path so nested
notes dragged in flat/search mode preserve their actual parent instead of being
treated as root moves.
Comment on lines
+104
to
+111
| const refreshSettings = useCallback(async () => { | ||
| try { | ||
| const s = await notesService.getSettings(); | ||
| setSettings(s); | ||
| } catch (err) { | ||
| console.error("Failed to load settings in NotesContext:", err); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Surface a user-facing error (and clear stale state) when settings refresh fails.
Current catch only logs to console. On folder changes, this can silently keep stale settings in UI.
Suggested fix
const refreshSettings = useCallback(async () => {
try {
const s = await notesService.getSettings();
setSettings(s);
} catch (err) {
- console.error("Failed to load settings in NotesContext:", err);
+ setSettings(null);
+ setError(err instanceof Error ? err.message : "Failed to load settings");
}
}, []);As per coding guidelines: "Implement error handling with user-friendly messages".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/NotesContext.tsx` around lines 104 - 111, The catch block in
refreshSettings currently only logs the error, which leaves stale settings in
the UI; update the error handling so that on failure you clear the stale
settings by calling setSettings(null) (or an appropriate empty state) and
surface a user-facing error (e.g., set a new settingsError state or call the
app's notification/snackbar helper) with the error message; modify the
refreshSettings function to call notesService.getSettings, then on catch do both
setSettings(null) and setSettingsError(err) or invoke
showToast/showErrorNotification with the error so callers/components can render
a friendly message.
Comment on lines
+118
to
+123
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : "Failed to update settings"); | ||
| // Rollback or reload if update fails | ||
| const s = await notesService.getSettings(); | ||
| setSettings(s); | ||
| } |
There was a problem hiding this comment.
Guard rollback fetch failures in updateSettings catch path.
If notesService.getSettings() also fails, this callback rejects from inside catch, which can leave optimistic UI state unresolved.
Suggested fix
} catch (err) {
setError(err instanceof Error ? err.message : "Failed to update settings");
- // Rollback or reload if update fails
- const s = await notesService.getSettings();
- setSettings(s);
+ try {
+ const s = await notesService.getSettings();
+ setSettings(s);
+ } catch (rollbackErr) {
+ setSettings(null);
+ setError(
+ rollbackErr instanceof Error
+ ? rollbackErr.message
+ : "Failed to reload settings after update failure"
+ );
+ }
}
}, []);As per coding guidelines: "Implement error handling with user-friendly messages".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| setError(err instanceof Error ? err.message : "Failed to update settings"); | |
| // Rollback or reload if update fails | |
| const s = await notesService.getSettings(); | |
| setSettings(s); | |
| } | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : "Failed to update settings"); | |
| try { | |
| const s = await notesService.getSettings(); | |
| setSettings(s); | |
| } catch (rollbackErr) { | |
| setSettings(null); | |
| setError( | |
| rollbackErr instanceof Error | |
| ? rollbackErr.message | |
| : "Failed to reload settings after update failure" | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/NotesContext.tsx` around lines 118 - 123, In updateSettings's
catch block, guard the rollback fetch by storing the previous settings before
doing the optimistic set, then wrap the call to notesService.getSettings() in
its own try/catch: on failure restore the saved previousSettings via
setSettings(prevSettings) and setError to a user-friendly message (include that
the rollback fetch failed optionally), otherwise use the fetched settings;
reference the updateSettings function, the catch block that calls
notesService.getSettings(), and setSettings/setError to locate where to add the
saved previousSettings and the inner try/catch fallback.
Comment on lines
+7
to
+96
| export function useFlipAnimation(_dependency: any) { | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
| const rectsRef = useRef<Map<string, DOMRect>>(new Map()); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!containerRef.current) return; | ||
| const children = Array.from(containerRef.current.children) as HTMLElement[]; | ||
|
|
||
| // 1. Measure all children in their NEW DOM positions (bypassing any active FLIP transforms) | ||
| const currentRects = new Map<string, DOMRect>(); | ||
| for (const child of children) { | ||
| const id = child.getAttribute("data-id") || child.id; | ||
| if (id) { | ||
| const originalTransform = child.style.transform; | ||
| const originalTransition = child.style.transition; | ||
| const isAnimating = !!(child as any)._flipCleanup; | ||
|
|
||
| if (isAnimating) { | ||
| // Temporarily disable both transition and transform to query the static layout position | ||
| child.style.transition = "none"; | ||
| child.style.transform = "none"; | ||
| currentRects.set(id, child.getBoundingClientRect()); | ||
| child.style.transition = originalTransition; | ||
| child.style.transform = originalTransform; | ||
| } else if (originalTransform && originalTransform !== "none") { | ||
| child.style.transform = "none"; | ||
| currentRects.set(id, child.getBoundingClientRect()); | ||
| child.style.transform = originalTransform; | ||
| } else { | ||
| currentRects.set(id, child.getBoundingClientRect()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // 2. Run the FLIP animation for elements that changed position since the last commit | ||
| for (const child of children) { | ||
| const id = child.getAttribute("data-id") || child.id; | ||
| if (!id) continue; | ||
|
|
||
| let firstRect = rectsRef.current.get(id); | ||
| const lastRect = currentRects.get(id); | ||
|
|
||
| if (firstRect && lastRect) { | ||
| // If there is an active transition from a previous commit, clean it up first. | ||
| // We measure the current mid-flight visual position BEFORE cleaning up | ||
| // to ensure a perfectly seamless transition from the interrupted state. | ||
| const isAnimating = !!(child as any)._flipCleanup; | ||
| if (isAnimating) { | ||
| firstRect = child.getBoundingClientRect(); | ||
| (child as any)._flipCleanup(); | ||
| } | ||
|
|
||
| const deltaY = firstRect.top - lastRect.top; | ||
| const deltaX = firstRect.left - lastRect.left; | ||
|
|
||
| if (deltaY !== 0 || deltaX !== 0) { | ||
| // Disable transitions temporarily | ||
| const originalTransition = child.style.transition; | ||
| child.style.transition = "none"; | ||
| child.style.transform = `translate3d(${deltaX}px, ${deltaY}px, 0)`; | ||
|
|
||
| // Trigger reflow/repaint to apply the invert state | ||
| child.offsetHeight; | ||
|
|
||
| // Smoothly animate back to the new DOM position (Play) | ||
| child.style.transition = "transform 350ms cubic-bezier(0.16, 1, 0.3, 1)"; | ||
| child.style.transform = "translate3d(0, 0, 0)"; | ||
|
|
||
| const cleanup = () => { | ||
| child.style.transition = originalTransition; | ||
| child.style.transform = ""; | ||
| child.removeEventListener("transitionend", onTransitionEnd); | ||
| (child as any)._flipCleanup = null; | ||
| }; | ||
|
|
||
| const onTransitionEnd = (e: TransitionEvent) => { | ||
| if (e.propertyName === "transform") { | ||
| cleanup(); | ||
| } | ||
| }; | ||
|
|
||
| child.addEventListener("transitionend", onTransitionEnd); | ||
| (child as any)._flipCleanup = cleanup; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // 3. Save the untransformed (new layout) positions for the next commit | ||
| rectsRef.current = currentRects; | ||
| }); // Runs on every commit to keep track of the latest stable positions |
There was a problem hiding this comment.
Scope FLIP work to actual order/layout changes.
useLayoutEffect runs on every commit right now, so every drag-over update re-measures all children and forces layout even when nothing reordered. In this PR that sits directly on the drag path, so larger trees/lists will get noticeably janky. Use the hook argument as the effect dependency (or derive a stable order key) so FLIP only runs when the rendered order changes.
Suggested change
-export function useFlipAnimation(_dependency: any) {
+export function useFlipAnimation(dependency: unknown) {
const containerRef = useRef<HTMLDivElement>(null);
const rectsRef = useRef<Map<string, DOMRect>>(new Map());
useLayoutEffect(() => {
if (!containerRef.current) return;
const children = Array.from(containerRef.current.children) as HTMLElement[];
@@
- }); // Runs on every commit to keep track of the latest stable positions
+ }, [dependency]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function useFlipAnimation(_dependency: any) { | |
| const containerRef = useRef<HTMLDivElement>(null); | |
| const rectsRef = useRef<Map<string, DOMRect>>(new Map()); | |
| useLayoutEffect(() => { | |
| if (!containerRef.current) return; | |
| const children = Array.from(containerRef.current.children) as HTMLElement[]; | |
| // 1. Measure all children in their NEW DOM positions (bypassing any active FLIP transforms) | |
| const currentRects = new Map<string, DOMRect>(); | |
| for (const child of children) { | |
| const id = child.getAttribute("data-id") || child.id; | |
| if (id) { | |
| const originalTransform = child.style.transform; | |
| const originalTransition = child.style.transition; | |
| const isAnimating = !!(child as any)._flipCleanup; | |
| if (isAnimating) { | |
| // Temporarily disable both transition and transform to query the static layout position | |
| child.style.transition = "none"; | |
| child.style.transform = "none"; | |
| currentRects.set(id, child.getBoundingClientRect()); | |
| child.style.transition = originalTransition; | |
| child.style.transform = originalTransform; | |
| } else if (originalTransform && originalTransform !== "none") { | |
| child.style.transform = "none"; | |
| currentRects.set(id, child.getBoundingClientRect()); | |
| child.style.transform = originalTransform; | |
| } else { | |
| currentRects.set(id, child.getBoundingClientRect()); | |
| } | |
| } | |
| } | |
| // 2. Run the FLIP animation for elements that changed position since the last commit | |
| for (const child of children) { | |
| const id = child.getAttribute("data-id") || child.id; | |
| if (!id) continue; | |
| let firstRect = rectsRef.current.get(id); | |
| const lastRect = currentRects.get(id); | |
| if (firstRect && lastRect) { | |
| // If there is an active transition from a previous commit, clean it up first. | |
| // We measure the current mid-flight visual position BEFORE cleaning up | |
| // to ensure a perfectly seamless transition from the interrupted state. | |
| const isAnimating = !!(child as any)._flipCleanup; | |
| if (isAnimating) { | |
| firstRect = child.getBoundingClientRect(); | |
| (child as any)._flipCleanup(); | |
| } | |
| const deltaY = firstRect.top - lastRect.top; | |
| const deltaX = firstRect.left - lastRect.left; | |
| if (deltaY !== 0 || deltaX !== 0) { | |
| // Disable transitions temporarily | |
| const originalTransition = child.style.transition; | |
| child.style.transition = "none"; | |
| child.style.transform = `translate3d(${deltaX}px, ${deltaY}px, 0)`; | |
| // Trigger reflow/repaint to apply the invert state | |
| child.offsetHeight; | |
| // Smoothly animate back to the new DOM position (Play) | |
| child.style.transition = "transform 350ms cubic-bezier(0.16, 1, 0.3, 1)"; | |
| child.style.transform = "translate3d(0, 0, 0)"; | |
| const cleanup = () => { | |
| child.style.transition = originalTransition; | |
| child.style.transform = ""; | |
| child.removeEventListener("transitionend", onTransitionEnd); | |
| (child as any)._flipCleanup = null; | |
| }; | |
| const onTransitionEnd = (e: TransitionEvent) => { | |
| if (e.propertyName === "transform") { | |
| cleanup(); | |
| } | |
| }; | |
| child.addEventListener("transitionend", onTransitionEnd); | |
| (child as any)._flipCleanup = cleanup; | |
| } | |
| } | |
| } | |
| // 3. Save the untransformed (new layout) positions for the next commit | |
| rectsRef.current = currentRects; | |
| }); // Runs on every commit to keep track of the latest stable positions | |
| export function useFlipAnimation(dependency: unknown) { | |
| const containerRef = useRef<HTMLDivElement>(null); | |
| const rectsRef = useRef<Map<string, DOMRect>>(new Map()); | |
| useLayoutEffect(() => { | |
| if (!containerRef.current) return; | |
| const children = Array.from(containerRef.current.children) as HTMLElement[]; | |
| // 1. Measure all children in their NEW DOM positions (bypassing any active FLIP transforms) | |
| const currentRects = new Map<string, DOMRect>(); | |
| for (const child of children) { | |
| const id = child.getAttribute("data-id") || child.id; | |
| if (id) { | |
| const originalTransform = child.style.transform; | |
| const originalTransition = child.style.transition; | |
| const isAnimating = !!(child as any)._flipCleanup; | |
| if (isAnimating) { | |
| // Temporarily disable both transition and transform to query the static layout position | |
| child.style.transition = "none"; | |
| child.style.transform = "none"; | |
| currentRects.set(id, child.getBoundingClientRect()); | |
| child.style.transition = originalTransition; | |
| child.style.transform = originalTransform; | |
| } else if (originalTransform && originalTransform !== "none") { | |
| child.style.transform = "none"; | |
| currentRects.set(id, child.getBoundingClientRect()); | |
| child.style.transform = originalTransform; | |
| } else { | |
| currentRects.set(id, child.getBoundingClientRect()); | |
| } | |
| } | |
| } | |
| // 2. Run the FLIP animation for elements that changed position since the last commit | |
| for (const child of children) { | |
| const id = child.getAttribute("data-id") || child.id; | |
| if (!id) continue; | |
| let firstRect = rectsRef.current.get(id); | |
| const lastRect = currentRects.get(id); | |
| if (firstRect && lastRect) { | |
| // If there is an active transition from a previous commit, clean it up first. | |
| // We measure the current mid-flight visual position BEFORE cleaning up | |
| // to ensure a perfectly seamless transition from the interrupted state. | |
| const isAnimating = !!(child as any)._flipCleanup; | |
| if (isAnimating) { | |
| firstRect = child.getBoundingClientRect(); | |
| (child as any)._flipCleanup(); | |
| } | |
| const deltaY = firstRect.top - lastRect.top; | |
| const deltaX = firstRect.left - lastRect.left; | |
| if (deltaY !== 0 || deltaX !== 0) { | |
| // Disable transitions temporarily | |
| const originalTransition = child.style.transition; | |
| child.style.transition = "none"; | |
| child.style.transform = `translate3d(${deltaX}px, ${deltaY}px, 0)`; | |
| // Trigger reflow/repaint to apply the invert state | |
| child.offsetHeight; | |
| // Smoothly animate back to the new DOM position (Play) | |
| child.style.transition = "transform 350ms cubic-bezier(0.16, 1, 0.3, 1)"; | |
| child.style.transform = "translate3d(0, 0, 0)"; | |
| const cleanup = () => { | |
| child.style.transition = originalTransition; | |
| child.style.transform = ""; | |
| child.removeEventListener("transitionend", onTransitionEnd); | |
| (child as any)._flipCleanup = null; | |
| }; | |
| const onTransitionEnd = (e: TransitionEvent) => { | |
| if (e.propertyName === "transform") { | |
| cleanup(); | |
| } | |
| }; | |
| child.addEventListener("transitionend", onTransitionEnd); | |
| (child as any)._flipCleanup = cleanup; | |
| } | |
| } | |
| } | |
| // 3. Save the untransformed (new layout) positions for the next commit | |
| rectsRef.current = currentRects; | |
| }, [dependency]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useFlipAnimation.ts` around lines 7 - 96, useLayoutEffect in
useFlipAnimation currently runs on every commit causing unnecessary reflows;
change the effect to depend on the hook argument (the _dependency) or a derived
stable order key so the FLIP logic only runs when rendered order/layout actually
changes. Concretely, update the useLayoutEffect call in useFlipAnimation to
include [_dependency] (or a computed stable ordering identifier derived from
_dependency) as its dependency array, and ensure rectsRef and containerRef
remain accessible inside the effect so you still measure, animate, and then
update rectsRef.current only when the dependency indicates a layout change.
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 by CodeRabbit
Release Notes