refactor: improve reorder logic

apps/client/src/common/api/rundown.ts

@@ -56,9 +56,9 @@ export async function putBatchEditEvents(data: BatchEditEntry): Promise<AxiosRes
 }
 
 export type ReorderEntry = {
-  eventId: string;
-  from: number;
-  to: number;
+  entryId: EntryId;
+  destinationId: EntryId;
+  order: 'before' | 'after' | 'insert';
 };
 
 /**

apps/client/src/common/hooks/useEntryAction.ts

@@ -13,8 +13,9 @@ import {
   TimeStrategy,
   TransientEventPayload,
 } from 'ontime-types';
-import { dayInMs, generateId, MILLIS_PER_SECOND, parseUserTime, reorderArray, swapEventData } from 'ontime-utils';
+import { dayInMs, generateId, MILLIS_PER_SECOND, parseUserTime, swapEventData } from 'ontime-utils';
 
+import { moveDown, moveUp } from '../../features/rundown/rundown.utils';
 import { RUNDOWN } from '../api/constants';
 import {
   deleteEntries,
@@ -618,51 +619,6 @@ export const useEntryActions = () => {
    */
   const _reorderEntryMutation = useMutation({
     mutationFn: patchReorderEntry,
-    // we optimistically update here
-    onMutate: async (data) => {
-      // cancel ongoing queries
-      await queryClient.cancelQueries({ queryKey: RUNDOWN });
-
-      // Snapshot the previous value
-      const previousData = queryClient.getQueryData<Rundown>(RUNDOWN);
-
-      if (previousData) {
-        // optimistically update object
-        const newOrder = reorderArray(previousData.order, data.from, data.to);
-        queryClient.setQueryData<Rundown>(RUNDOWN, {
-          id: previousData.id,
-          title: previousData.title,
-          order: newOrder,
-          flatOrder: previousData.flatOrder,
-          entries: previousData.entries,
-          revision: -1,
-        });
-      }
-
-      // Return a context with the previous and new events
-      return { previousData };
-    },
-
-    // Mutation fails, rollback undoes optimist update
-    onError: (_error, _data, context) => {
-      queryClient.setQueryData<Rundown>(RUNDOWN, context?.previousData);
-    },
-
-    // Mutation finished, we update the rundown with the response
-    onSuccess: (response) => {
-      if (!response.data) return;
-
-      const { id, title, order, flatOrder, entries, revision } = response.data;
-      queryClient.setQueryData<Rundown>(RUNDOWN, {
-        id,
-        title,
-        order,
-        flatOrder,
-        entries,
-        revision,
-      });
-    },
-
     // Mutation finished, failed or successful
     // Fetch anyway, just to be sure
     onSettled: () => {
@@ -674,12 +630,12 @@ export const useEntryActions = () => {
    * Reorders a given entry
    */
   const reorderEntry = useCallback(
-    async (entryId: string, from: number, to: number) => {
+    async (entryId: EntryId, destinationId: EntryId, order: 'before' | 'after' | 'insert') => {
       try {
         const reorderObject: ReorderEntry = {
-          eventId: entryId,
-          from,
-          to,
+          entryId,
+          destinationId,
+          order,
         };
         await _reorderEntryMutation.mutateAsync(reorderObject);
       } catch (error) {
@@ -689,6 +645,30 @@ export const useEntryActions = () => {
     [_reorderEntryMutation],
   );
 
+  const move = useCallback(async (entryId: EntryId, direction: 'up' | 'down') => {
+    const cachedRundown = queryClient.getQueryData<Rundown>(RUNDOWN);
+    if (!cachedRundown?.order) {
+      return;
+    }
+    const { destinationId, order } =
+      direction === 'up'
+        ? moveUp(entryId, cachedRundown.order, cachedRundown.entries)
+        : moveDown(entryId, cachedRundown.order, cachedRundown.entries);
+
+    if (destinationId) {
+      try {
+        const reorderObject: ReorderEntry = {
+          entryId,
+          destinationId,
+          order: order as 'before' | 'after' | 'insert',
+        };
+        await _reorderEntryMutation.mutateAsync(reorderObject);
+      } catch (error) {
+        logAxiosError('Error re-ordering event', error);
+      }
+    }
+  }, []);
+
   /**
    * Calls mutation to swap events
    * @private
@@ -765,6 +745,7 @@ export const useEntryActions = () => {
     ungroup,
     getEntryById,
     groupEntries,
+    move,
     reorderEntry,
     swapEvents,
     updateEntry,

apps/client/src/features/rundown/Rundown.tsx

@@ -42,7 +42,7 @@ import { cloneEvent } from '../../common/utils/clone';
 import BlockBlock from './block-block/BlockBlock';
 import BlockEnd from './block-block/BlockEnd';
 import QuickAddBlock from './quick-add-block/QuickAddBlock';
-import { getNextId, getPreviousId, makeRundownMetadata, makeSortableList } from './rundown.utils';
+import { makeRundownMetadata, makeSortableList, moveDown, moveUp } from './rundown.utils';
 import RundownEmpty from './RundownEmpty';
 import { useEventSelection } from './useEventSelection';
 
@@ -188,19 +188,26 @@ export default function Rundown({ data }: RundownProps) {
   );
 
   const moveEntry = useCallback(
-    (cursor: string | null, direction: 'up' | 'down') => {
-      if (order.length < 2 || cursor == null) {
+    (cursor: EntryId | null, direction: 'up' | 'down') => {
+      if (sortableData.length < 2 || cursor == null) {
         return;
       }
 
-      const destinationId = direction === 'up' ? getPreviousId(cursor, sortableData) : getNextId(cursor, sortableData);
-      if (direction === 'up' && destinationId === null) {
-        reorderEntry(cursor, cursor, 'before');
-      } else if (destinationId !== null) {
-        reorderEntry(cursor, destinationId);
+      const { destinationId, order, isBlock } =
+        direction === 'up' ? moveUp(cursor, sortableData, entries) : moveDown(cursor, sortableData, entries);
+
+      if (!destinationId) {
+        return;
       }
+
+      // if we are moving into a block, we need to make sure it is expanded
+      if (isBlock) {
+        handleCollapseGroup(false, destinationId);
+      }
+
+      reorderEntry(cursor, destinationId, order as 'before' | 'after' | 'insert');
     },
-    [order.length, sortableData, reorderEntry],
+    [sortableData, reorderEntry],
   );
 
   // shortcuts
@@ -285,17 +292,33 @@ export default function Rundown({ data }: RundownProps) {
   const handleOnDragEnd = (event: DragEndEvent) => {
     const { active, over } = event;
 
-    if (over?.id) {
-      if (active.id !== over?.id) {
-        // we keep a copy of the state as a hack to handle inconsistencies between dnd-kit and async store updates
-        setSortableData((currentEntries) => {
-          const fromIndex = active.data.current?.sortable.index;
-          const toIndex = over.data.current?.sortable.index;
-          return reorderArray(currentEntries, fromIndex, toIndex);
-        });
-        reorderEntry(active.id as string, over.id as string, 'before');
-      }
+    if (!over?.id || active.id === over.id) {
+      return;
     }
+
+    const fromIndex = active.data.current?.sortable.index;
+    const toIndex = over.data.current?.sortable.index;
+
+    // we keep a copy of the state as a hack to handle inconsistencies between dnd-kit and async store updates
+    setSortableData((currentEntries) => {
+      return reorderArray(currentEntries, fromIndex, toIndex);
+    });
+
+    let destinationId = over.id as EntryId;
+    let order: 'before' | 'after' | 'insert' = fromIndex < toIndex ? 'after' : 'before';
+
+    /**
+     * We need to specially handle the end blocks
+     * Dragging before and end block will add the entry to the end of the block
+     * Dragging after an end block will add the event after the block itself
+     */
+    if (destinationId.startsWith('end-')) {
+      destinationId = destinationId.replace('end-', '');
+      // if we are moving before the end, we use the insert operation
+      order = 'insert';
+    }
+
+    reorderEntry(active.id as EntryId, destinationId, order);
   };
 
   /**

apps/client/src/features/rundown/__tests__/rundown.utils.test.ts

@@ -1,6 +1,6 @@
-import { OntimeBlock, OntimeDelay, OntimeEvent, RundownEntries, SupportedEntry } from 'ontime-types';
+import { EntryId, OntimeBlock, OntimeDelay, OntimeEvent, RundownEntries, SupportedEntry } from 'ontime-types';
 
-import { makeRundownMetadata, makeSortableList } from '../rundown.utils';
+import { makeRundownMetadata, makeSortableList, moveDown, moveUp } from '../rundown.utils';
 
 describe('makeRundownMetadata()', () => {
   it('processes nested rundown data', () => {
@@ -333,3 +333,67 @@ describe('makeSortableList()', () => {
     expect(sortableList).toStrictEqual(['block-1', 'end-block-1', 'block-2', 'end-block-2']);
   });
 });
+
+describe('moveUp()', () => {
+  const sortableData = ['event1', 'event2', 'block1', 'event11', 'end-block1', 'block2', 'end-block2', 'event3'];
+  const entries = {
+    event1: { type: 'event', id: 'event1', parent: null } as OntimeEvent,
+    event2: { type: 'event', id: 'event2', parent: null }as OntimeEvent,
+    block1: { type: 'block', id: 'block1', events: ['event3'] } as OntimeBlock,
+    event11: { type: 'event', id: 'event11', parent: 'block1' } as OntimeEvent,
+    block2: { type: 'block', id: 'block2', events: [] as EntryId[] } as OntimeBlock,
+    event3: { type: 'event', id: 'event3', parent: null } as OntimeEvent,
+  };
+
+  it('moves an event up in the list', () => {
+    const result = moveUp('event2', sortableData, entries);
+    expect(result).toStrictEqual({ destinationId: 'event1', order: 'before', isBlock: false });
+  })
+
+  it.todo('disallows nesting blocks', () => {
+    const result = moveUp('block2', sortableData, entries);
+    expect(result).toStrictEqual({ destinationId: 'block1', order: 'before', isBlock: false });
+  })
+
+  it('moves an event into a block', () => {
+    const result = moveUp('event3', sortableData, entries);
+    expect(result).toStrictEqual({ destinationId: 'block2', order: 'insert', isBlock: true });
+  })
+
+  it('moving up from top is noop', () => {
+    const result = moveUp('event1', sortableData, entries);
+    expect(result).toMatchObject({ destinationId: null });
+  })
+});
+
+describe('moveDown()', () => {
+  const sortableData = ['event1', 'event2', 'block1', 'event11', 'end-block1', 'block2', 'end-block2', 'event3'];
+  const entries = {
+    event1: { type: 'event', id: 'event1', parent: null } as OntimeEvent,
+    event2: { type: 'event', id: 'event2', parent: null }as OntimeEvent,
+    block1: { type: 'block', id: 'block1', events: ['event11'] } as OntimeBlock,
+    event11: { type: 'event', id: 'event11', parent: 'block1' } as OntimeEvent,
+    block2: { type: 'block', id: 'block2', events: [] as EntryId[] } as OntimeBlock,
+    event3: { type: 'event', id: 'event3', parent: null } as OntimeEvent,
+  };
+
+  it('moves an event down in the list', () => {
+    const result = moveDown('event1', sortableData, entries);
+    expect(result).toStrictEqual({ destinationId: 'event2', order: 'after', isBlock: false });
+  })
+
+  it.todo('disallows nesting blocks', () => {
+    const result = moveDown('block1', sortableData, entries);
+    expect(result).toStrictEqual({ destinationId: 'block2', order: 'before', isBlock: false });
+  })
+
+  it('moves an event into a block', () => {
+    const result = moveDown('event2', sortableData, entries);
+    expect(result).toStrictEqual({ destinationId: 'event11', order: 'before', isBlock: true });
+  })
+
+  it('moving down from bottom is noop', () => {
+    const result = moveDown('event3', sortableData, entries);
+    expect(result).toMatchObject({ destinationId: null });
+  })
+});

apps/client/src/features/rundown/rundown.utils.ts

@@ -164,7 +164,85 @@ export function canDrop(targetType?: SupportedEntry, targetParent?: EntryId | nu
   return targetType == 'block';
 }
 
-export function getNextId(entryId: EntryId, sortableData: EntryId[]): MaybeString {
+/**
+ * Calculates destinations for an entry moving one position up in the rundown
+ * - Handles noops
+ * - Handles moving in and out of blocks
+ * TODO: handle moving blocks
+ */
+export function moveUp(entryId: EntryId, sortableData: EntryId[], entries: RundownEntries) {
+  const previousEntryId = getPreviousId(entryId, sortableData);
+
+  // the user is moving up at the top of the list
+  if (!previousEntryId) {
+    return { destinationId: null, order: 'before', isBlock: false };
+  }
+
+  if (previousEntryId.startsWith('end-')) {
+    const entry = entries[entryId];
+    if (isOntimeBlock(entry)) {
+      // if we are moving a block, we cannot insert it
+      return { destinationId: previousEntryId.replace('end-', ''), order: 'before', isBlock: false };
+    }
+    // insert in the block ID will add to the end of the block events
+    return { destinationId: previousEntryId.replace('end-', ''), order: 'insert', isBlock: true };
+  }
+
+  // @ts-expect-error -- we safeguard the entry not having a parent property
+  return { destinationId: previousEntryId, order: 'before', isBlock: Boolean(entries[previousEntryId]?.parent) };
+}
+
+/**
+ * Calculates destinations for an entry moving one position down in the rundown
+ * - Handles noops
+ * - Handles moving in and out of blocks
+ * TODO: handle moving blocks
+ */
+export function moveDown(entryId: EntryId, sortableData: EntryId[], entries: RundownEntries) {
+  const nextEntryId = getNextId(entryId, sortableData);
+
+  // the user is moving down at the end of the list
+  if (!nextEntryId) {
+    return { destinationId: null, order: 'after', isBlock: false };
+  }
+
+  if (nextEntryId.startsWith('end-')) {
+    // move outside the block
+    return { destinationId: nextEntryId.replace('end-', ''), order: 'after', isBlock: false };
+  }
+
+  /**
+   * If the next entry is a block
+   * - 1. blocks need to skip over it
+   * - 2. if the block has children, we insert before the first child
+   * - 3. if the block is empty, we insert into the block
+   */
+  if (isOntimeBlock(entries[nextEntryId])) {
+    const entry = entries[entryId];
+
+    if (isOntimeBlock(entry)) {
+      // 1. if we are moving a block, we cannot insert it
+      return { destinationId: nextEntryId, order: 'after', isBlock: false };
+    }
+
+    const firstBlockChild = entries[nextEntryId].events.at(0);
+    if (firstBlockChild) {
+      // 2. add before the first child of the block
+      return { destinationId: firstBlockChild, order: 'before', isBlock: true };
+    } else {
+      // 3. or insert into an empty block
+      return { destinationId: nextEntryId, order: 'insert', isBlock: true };
+    }
+  }
+
+  return { destinationId: nextEntryId, order: 'after', isBlock: Boolean(entries[nextEntryId]?.parent) };
+}
+
+/**
+ * Utility function gets the ID if the next entry in the list
+ * returns null if none is found
+ */
+function getNextId(entryId: EntryId, sortableData: EntryId[]): EntryId | null {
   const currentIndex = sortableData.indexOf(entryId);
   if (currentIndex === -1 || currentIndex === sortableData.length - 1) {
     // No next ID if not found or at the end
@@ -173,7 +251,11 @@ export function getNextId(entryId: EntryId, sortableData: EntryId[]): MaybeStrin
   return sortableData[currentIndex + 1];
 }
 
-export function getPreviousId(entryId: EntryId, sortableData: EntryId[]): MaybeString {
+/**
+ * Utility function gets the ID if the previous entry in the list
+ * returns null if none is found
+ */
+function getPreviousId(entryId: EntryId, sortableData: EntryId[]): EntryId | null {
   const currentIndex = sortableData.indexOf(entryId);
   if (currentIndex < 1) {
     // No previous ID found or at the beginning

apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-menu/CuesheetTableMenuActions.tsx

@@ -13,7 +13,7 @@ interface CuesheetTableMenuActionsProps {
 
 export default function CuesheetTableMenuActions(props: CuesheetTableMenuActionsProps) {
   const { eventId, entryIndex, showModal } = props;
-  const { addEntry, getEntryById, reorderEntry, deleteEntry } = useEntryActions();
+  const { addEntry, getEntryById, move, deleteEntry } = useEntryActions();
 
   const handleCloneEvent = () => {
     const currentEvent = getEntryById(eventId);
@@ -45,14 +45,10 @@ export default function CuesheetTableMenuActions(props: CuesheetTableMenuActions
         Clone event
       </MenuItem>
       <MenuDivider />
-      <MenuItem
-        isDisabled={entryIndex < 1}
-        icon={<IoArrowUp />}
-        onClick={() => reorderEntry(eventId, entryIndex, entryIndex - 1)}
-      >
+      <MenuItem isDisabled={entryIndex < 1} icon={<IoArrowUp />} onClick={() => move(eventId, 'up')}>
         Move up
       </MenuItem>
-      <MenuItem icon={<IoArrowDown />} onClick={() => reorderEntry(eventId, entryIndex, entryIndex + 1)}>
+      <MenuItem icon={<IoArrowDown />} onClick={() => move(eventId, 'down')}>
         Move down
       </MenuItem>
       <MenuItem icon={<IoTrash />} onClick={() => deleteEntry([eventId])}>