Fix order of items in keyboard iterator

The previous keyboard iterator did not take into account, if multiple items deregistered at once.
Therefore, the keyboard iterator ran into an iterator invalidation bug, because the assigned index of an element can run out of bounds.
Elements with an out of bounds assigned index, would not be able to deregister and therefore stay in the list of registered items.
Therefore messing up the iteration logic.

gradle/changelog/fix-keyboard-iterator.yaml

@@ -0,0 +1,2 @@
+- type: fixed
+  description: Order of keyboard iterator items within the repository file search and repository overview

scm-ui/ui-components/src/repos/RepositoryEntry.tsx

@@ -22,11 +22,11 @@ import { ExtensionPoint, extensionPoints } from "@scm-manager/ui-extensions";
 import RepositoryFlags from "./RepositoryFlags";
 import styled from "styled-components";
 import { useTranslation } from "react-i18next";
-import { useKeyboardIteratorTarget } from "@scm-manager/ui-shortcuts";
 import { Card } from "@scm-manager/ui-layout";
 import { Link } from "react-router-dom";
 import { Menu } from "@scm-manager/ui-overlays";
 import { Icon } from "@scm-manager/ui-buttons";
+import { useKeyboardIteratorTargetV2 } from "@scm-manager/ui-core";
 
 type DateProp = Date | string;
 
@@ -35,6 +35,7 @@ type Props = {
   // @VisibleForTesting
   // the baseDate is only to avoid failing snapshot tests
   baseDate?: DateProp;
+  expectedIndex?: number;
 };
 
 const Avatar = styled.div`
@@ -59,9 +60,9 @@ const DetailsRow = styled(Card.Row)`
   gap: 0.5rem;
 `;
 
-const RepositoryEntry: FC<Props> = ({ repository, baseDate }) => {
+const RepositoryEntry: FC<Props> = ({ repository, baseDate, expectedIndex }) => {
   const [t] = useTranslation("repos");
-  const ref = useKeyboardIteratorTarget();
+  const ref = useKeyboardIteratorTargetV2({ expectedIndex: expectedIndex ?? 0 });
 
   const actions = () => (
     <Menu>

scm-ui/ui-core/src/base/shortcuts/index.ts

@@ -17,4 +17,9 @@
 export { default as useShortcut } from "./useShortcut";
 export { default as useShortcutDocs, ShortcutDocsContextProvider } from "./useShortcutDocs";
 export { default as usePauseShortcuts } from "./usePauseShortcuts";
-export { useKeyboardIteratorTarget, KeyboardIterator, KeyboardSubIterator } from "./iterator/keyboardIterator";
+export {
+  useKeyboardIteratorTarget,
+  KeyboardIterator,
+  KeyboardSubIterator,
+  useKeyboardIteratorTargetV2,
+} from "./iterator/keyboardIterator";

scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts

@@ -20,6 +20,12 @@ const INACTIVE_INDEX = -1;
 
 export type Callback = () => void;
 
+export type IterableCallback = {
+  item: Callback | CallbackIterator;
+  expectedIndex: number;
+  cleanup?: Callback;
+};
+
 type Direction = "forward" | "backward";
 
 /**
@@ -31,11 +37,18 @@ export type CallbackRegistry = {
    * Registers the given item and returns its index to use in {@link deregister}.
    */
   register: (item: Callback | CallbackIterator) => number;
-
   /**
    * Use the index returned from {@link register} to de-register.
    */
   deregister: (index: number) => void;
+  /**
+   * Registers the given iterable item while maintaining the order of the expected index of each item.
+   */
+  registerItem?: (iterable: IterableCallback) => void;
+  /**
+   * Removes the passed iterable item.
+   */
+  deregisterItem?: (iterable: IterableCallback) => void;
 };
 
 const isSubiterator = (item?: Callback | CallbackIterator): item is CallbackIterator =>
@@ -51,6 +64,8 @@ const offset = (direction: Direction) => (direction === "forward" ? 1 : -1);
  *
  * ## Terminology
  * - Item: Either a callback or a nested iterator
+ * - Cleanup: A callback that is called, if the corresponding item is removed
+ * - Iterable: A wrapper containing an item, a cleanup callback and the index that this iterable is expected to be at
  * - Available: Item is a non-empty iterator OR a regular callback
  * - Inactive: Current index is -1
  * - Activate: Move iterator while in inactive state OR call regular callback
@@ -68,7 +83,7 @@ export class CallbackIterator implements CallbackRegistry {
 
   constructor(
     private readonly activeIndexRef: MutableRefObject<number>,
-    private readonly itemsRef: MutableRefObject<Array<Callback | CallbackIterator>>
+    private readonly itemsRef: MutableRefObject<Array<IterableCallback>>
   ) {}
 
   private get activeIndex() {
@@ -83,7 +98,7 @@ export class CallbackIterator implements CallbackRegistry {
     return this.itemsRef.current;
   }
 
-  private get currentItem(): Callback | CallbackIterator | undefined {
+  private get currentItem(): IterableCallback | undefined {
     return this.items[this.activeIndex];
   }
 
@@ -101,9 +116,9 @@ export class CallbackIterator implements CallbackRegistry {
 
   private firstAvailableIndex = (direction: Direction, fromIndex = this.firstIndex(direction)) => {
     for (; direction === "forward" ? fromIndex < this.items.length : fromIndex >= 0; fromIndex += offset(direction)) {
-      const callback = this.items[fromIndex];
-      if (callback) {
-        if (!isSubiterator(callback) || callback.hasNext(direction)) {
+      const iterableCallback = this.items[fromIndex];
+      if (iterableCallback) {
+        if (!isSubiterator(iterableCallback.item) || iterableCallback.item.hasNext(direction)) {
           return fromIndex;
         }
       }
@@ -116,14 +131,15 @@ export class CallbackIterator implements CallbackRegistry {
   };
 
   private activateCurrentItem = (direction: Direction) => {
-    if (isSubiterator(this.currentItem)) {
-      this.currentItem.move(direction);
+    if (isSubiterator(this.currentItem?.item)) {
+      this.currentItem?.item.move(direction);
     } else if (this.currentItem) {
-      this.currentItem();
+      this.currentItem.item();
     }
   };
 
   private setIndexAndActivateCurrentItem = (index: number | null, direction: Direction) => {
+    this.currentItem?.cleanup?.();
     if (index !== null && index !== INACTIVE_INDEX) {
       this.activeIndex = index;
       this.activateCurrentItem(direction);
@@ -131,11 +147,11 @@ export class CallbackIterator implements CallbackRegistry {
   };
 
   private move = (direction: Direction) => {
-    if (isSubiterator(this.currentItem) && this.currentItem.hasNext(direction)) {
-      this.currentItem.move(direction);
+    if (isSubiterator(this.currentItem?.item) && this.currentItem?.item.hasNext(direction)) {
+      this.currentItem?.item.move(direction);
     } else {
-      if (isSubiterator(this.currentItem)) {
-        this.currentItem.reset();
+      if (isSubiterator(this.currentItem?.item)) {
+        this.currentItem?.item.reset();
       }
       let nextIndex: number | null;
       if (this.isInactive) {
@@ -151,7 +167,7 @@ export class CallbackIterator implements CallbackRegistry {
     if (this.isInactive) {
       return this.hasAvailableIndex(inDirection);
     }
-    if (isSubiterator(this.currentItem) && this.currentItem.hasNext(inDirection)) {
+    if (isSubiterator(this.currentItem?.item) && this.currentItem?.item.hasNext(inDirection)) {
       return true;
     }
     return this.hasAvailableIndex(inDirection, this.activeIndex + offset(inDirection));
@@ -172,41 +188,66 @@ export class CallbackIterator implements CallbackRegistry {
   public reset = () => {
     this.activeIndex = INACTIVE_INDEX;
     for (const cb of this.items) {
-      if (isSubiterator(cb)) {
-        cb.reset();
+      if (isSubiterator(cb.item)) {
+        cb.item.reset();
       }
     }
   };
 
   public register = (item: Callback | CallbackIterator) => {
-    if (isSubiterator(item)) {
-      item.parent = this;
-    }
-    return this.items.push(item) - 1;
+    const expectedIndex = this.items.length;
+    this.registerItem({ item, expectedIndex });
+    return expectedIndex;
   };
 
   public deregister = (index: number) => {
-    this.items.splice(index, 1);
-    if (this.activeIndex === index || this.activeIndex >= this.items.length) {
-      if (this.hasAvailableIndex("backward", index)) {
-        this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("backward", index), "backward");
-      } else if (this.hasAvailableIndex("forward", index)) {
-        this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("forward", index), "backward");
-      } else if (this.parent) {
-        if (this.parent.hasNext("forward")) {
-          this.parent.move("forward");
-        } else if (this.parent.hasNext("backward")) {
-          this.parent.move("backward");
-        }
+    if (this.items[index]) {
+      this.deregisterItem(this.items[index]);
+    }
+  };
+
+  public deregisterItem = (iterable: IterableCallback) => {
+    const itemIndex = this.items.findIndex((value) => value.expectedIndex === iterable.expectedIndex);
+    if (itemIndex === -1) {
+      return;
+    }
+
+    const removedIterable = this.items[itemIndex];
+    removedIterable.cleanup?.();
+
+    this.items.splice(itemIndex, 1);
+    if (this.activeIndex >= itemIndex) {
+      if (this.hasAvailableIndex("backward")) {
+        this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("backward", itemIndex), "backward");
+      } else if (this.hasAvailableIndex("forward")) {
+        this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("forward", itemIndex), "forward");
+      } else if (this.parent?.hasNext("forward")) {
+        this.parent?.move("forward");
+      } else if (this.parent?.hasNext("backward")) {
+        this.parent?.move("backward");
       } else {
         this.reset();
       }
     }
   };
+
+  public registerItem = (iterable: IterableCallback) => {
+    if (isSubiterator(iterable.item)) {
+      iterable.item.parent = this;
+    }
+
+    const insertAt = this.items.findIndex((value) => value.expectedIndex > iterable.expectedIndex);
+
+    if (insertAt === -1) {
+      this.items.push(iterable);
+    } else {
+      this.items.splice(insertAt, 0, iterable);
+    }
+  };
 }
 
 export const useCallbackIterator = (initialIndex = INACTIVE_INDEX) => {
-  const items = useRef<Array<Callback | CallbackIterator>>([]);
+  const items = useRef<Array<IterableCallback>>([]);
   const activeIndex = useRef<number>(initialIndex);
   return useMemo(() => new CallbackIterator(activeIndex, items), [activeIndex, items]);
 };

scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx

@@ -277,9 +277,6 @@ describe("shortcutIterator", () => {
       expect(callback).toHaveBeenCalledTimes(1);
       expect(callback2).toHaveBeenCalledTimes(1);
       expect(callback3).toHaveBeenCalledTimes(1);
-
-      expect(callback).toHaveBeenCalledBefore(callback2);
-      expect(callback2).toHaveBeenCalledBefore(callback3);
     });
 
     it("should call first target that is not an empty subiterator", () => {
@@ -378,8 +375,6 @@ describe("shortcutIterator", () => {
       expect(callback3).not.toHaveBeenCalled();
       expect(callback2).toHaveBeenCalledTimes(1);
       expect(callback).toHaveBeenCalledTimes(1);
-
-      expect(callback2).toHaveBeenCalledBefore(callback);
     });
 
     it("should move subiterator if its active callback is de-registered", () => {

scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx

@@ -17,7 +17,13 @@
 import React, { FC, useCallback, useContext, useEffect, useRef } from "react";
 import { useTranslation } from "react-i18next";
 import { useShortcut } from "../index";
-import { Callback, CallbackIterator, CallbackRegistry, useCallbackIterator } from "./callbackIterator";
+import {
+  Callback,
+  CallbackIterator,
+  CallbackRegistry,
+  IterableCallback,
+  useCallbackIterator,
+} from "./callbackIterator";
 
 const KeyboardIteratorContext = React.createContext<CallbackRegistry>({
   register: () => {
@@ -33,6 +39,18 @@ const KeyboardIteratorContext = React.createContext<CallbackRegistry>({
       console.warn("Keyboard iterator targets have to be declared inside a KeyboardIterator");
     }
   },
+  deregisterItem: () => {
+    if (process.env.NODE_ENV === "development") {
+      // eslint-disable-next-line no-console
+      console.warn("Keyboard iterator targets have to be declared inside a KeyboardIterator");
+    }
+  },
+  registerItem: () => {
+    if (process.env.NODE_ENV === "development") {
+      // eslint-disable-next-line no-console
+      console.warn("Keyboard iterator targets have to be declared inside a KeyboardIterator");
+    }
+  },
 });
 
 export const useKeyboardIteratorItem = (item: Callback | CallbackIterator) => {
@@ -43,6 +61,14 @@ export const useKeyboardIteratorItem = (item: Callback | CallbackIterator) => {
   }, [item, register, deregister]);
 };
 
+export const useKeyboardIteratorItemV2 = (iterable: IterableCallback) => {
+  const { registerItem, deregisterItem } = useContext(KeyboardIteratorContext);
+  useEffect(() => {
+    registerItem?.(iterable);
+    return () => deregisterItem?.(iterable);
+  }, [iterable, registerItem, deregisterItem]);
+};
+
 export const KeyboardSubIteratorContextProvider: FC<{ initialIndex?: number }> = ({ children, initialIndex }) => {
   const callbackIterator = useCallbackIterator(initialIndex);
 
@@ -73,6 +99,7 @@ export const KeyboardIteratorContextProvider: FC<{ initialIndex?: number }> = ({
 };
 
 /**
+ * @deprecated since version 3.8.0. Use {@link useKeyboardIteratorTargetV2} instead.
  * Use the {@link React.RefObject} returned from this hook to register a target to the nearest enclosing {@link KeyboardIterator} or {@link KeyboardSubIterator}.
  *
  * @example
@@ -91,6 +118,32 @@ export function useKeyboardIteratorTarget(): React.RefCallback<HTMLElement> {
   return refCallback;
 }
 
+/**
+ * @deprecated since version 3.8.0. Use {@link useKeyboardIteratorTargetV2} instead.
+ * Use the {@link React.RefObject} returned from this hook to register a target to the nearest enclosing {@link KeyboardIterator} or {@link KeyboardSubIterator},
+ * while respecting its expected index / position.
+ *
+ * @example
+ * const ref = useKeyboardIteratorTarget({ expectedIndex: 0});
+ * const target = <button ref={ref}>My Iteration Target</button>
+ */
+export function useKeyboardIteratorTargetV2({
+  expectedIndex,
+}: {
+  expectedIndex: number;
+}): React.RefCallback<HTMLElement> {
+  const ref = useRef<HTMLElement>();
+  const callback = useCallback(() => ref.current?.focus(), []);
+  const cleanup = useCallback(() => ref.current?.blur(), []);
+  const refCallback: React.RefCallback<HTMLElement> = useCallback((el) => {
+    if (el) {
+      ref.current = el;
+    }
+  }, []);
+  useKeyboardIteratorItemV2({ item: callback, cleanup, expectedIndex });
+  return refCallback;
+}
+
 /**
  * Allows keyboard users to iterate through a list of items, defined by enclosed {@link useKeyboardIteratorTarget} invocations.
  *

scm-ui/ui-core/src/index.ts

@@ -14,4 +14,4 @@
  * along with this program. If not, see https://www.gnu.org/licenses/.
  */
 
-export * from "./base"
+export * from "./base";

scm-ui/ui-types/src/Repositories.ts

@@ -85,4 +85,5 @@ export type RepositoryGroup = {
   name: string;
   namespace?: Namespace;
   repositories: Repository[];
+  currentPageOffset?: number;
 };

scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx

@@ -17,13 +17,14 @@
 import React from "react";
 import { useTranslation } from "react-i18next";
 import { urls } from "@scm-manager/ui-api";
-import { Icon, useKeyboardIteratorTarget } from "@scm-manager/ui-core";
+import { Icon, useKeyboardIteratorTargetV2 } from "@scm-manager/ui-core";
 import { Link } from "react-router-dom";
 import styled from "styled-components";
 
 type FileSearchHitProps = {
   contentBaseUrl: string;
   path: string;
+  expectedIndex?: number;
 };
 
 const IconColumn = styled.td`
@@ -39,17 +40,17 @@ const LeftOverflowTd = styled.td`
   text-align: left !important;
 `;
 
-export function FileSearchHit({ contentBaseUrl, path }: FileSearchHitProps) {
+export function FileSearchHit({ contentBaseUrl, path, expectedIndex }: FileSearchHitProps) {
   const [t] = useTranslation("repos");
   const link = urls.concat(contentBaseUrl, path);
-  const ref = useKeyboardIteratorTarget();
+  const ref = useKeyboardIteratorTargetV2({ expectedIndex: expectedIndex ?? 0 });
   return (
     <tr>
       <IconColumn aria-hidden="true">
         <Icon title={t("fileSearch.file")}>file</Icon>
       </IconColumn>
       <LeftOverflowTd>
-        <Link title={path} to={link} data-testid="file_search_single_result" ref={ref} key={path}>
+        <Link title={path} to={link} data-testid="file_search_single_result" ref={ref}>
           {path}
         </Link>
       </LeftOverflowTd>

scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx

@@ -32,8 +32,8 @@ const ResultTable: FC<ResultTableProps> = ({ contentBaseUrl, paths }) => {
     <table className="table table-hover table-sm is-fullwidth">
       <KeyboardIterator>
         <tbody>
-          {paths.map((path) => (
-            <FileSearchHit contentBaseUrl={contentBaseUrl} path={path} key={path} />
+          {paths.map((path, index) => (
+            <FileSearchHit contentBaseUrl={contentBaseUrl} path={path} key={path} expectedIndex={index} />
           ))}
         </tbody>
       </KeyboardIterator>

scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx

@@ -23,8 +23,14 @@ type Props = {
 };
 
 const RepositoryGroupEntry: FC<Props> = ({ group }) => {
-  const entries = group.repositories.map((repository) => {
-    return <RepositoryEntry repository={repository} key={repository.name} />;
+  const entries = group.repositories.map((repository, index) => {
+    return (
+      <RepositoryEntry
+        repository={repository}
+        key={repository.name}
+        expectedIndex={(group.currentPageOffset ?? 0) + index}
+      />
+    );
   });
   return <NamespaceEntries group={group} elements={entries} />;
 };

scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts

@@ -76,16 +76,19 @@ it("should group the repositories by their namespace", () => {
       name: "hitchhiker",
       namespace: { namespace: "hitchhiker" },
       repositories: [hitchhikerHeartOfGold, hitchhikerPuzzle42, hitchhikerRestand],
+      currentPageOffset: 0,
     },
     {
       name: "slarti",
       namespace: { namespace: "slarti" },
       repositories: [slartiFjords, slartiBlueprintsFjords],
+      currentPageOffset: 3,
     },
     {
       name: "zaphod",
       namespace: { namespace: "zaphod" },
       repositories: [zaphodMarvinFirmware],
+      currentPageOffset: 5,
     },
   ];
 

scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts

@@ -20,7 +20,7 @@ export default function groupByNamespace(
   repositories: Repository[],
   namespaces: NamespaceCollection
 ): RepositoryGroup[] {
-  const groups = {};
+  const groups: Record<string, RepositoryGroup> = {};
   for (const repository of repositories) {
     const groupName = repository.namespace;
 
@@ -37,15 +37,16 @@ export default function groupByNamespace(
     group.repositories.push(repository);
   }
 
-  const groupArray = [];
+  const groupArray: RepositoryGroup[] = [];
   for (const groupName in groups) {
     groupArray.push(groups[groupName]);
   }
   groupArray.sort(sortByName);
+  applyOffsets(groupArray);
   return groupArray;
 }
 
-function sortByName(a, b) {
+function sortByName(a: RepositoryGroup, b: RepositoryGroup) {
   if (a.name < b.name) {
     return -1;
   } else if (a.name > b.name) {
@@ -54,6 +55,14 @@ function sortByName(a, b) {
   return 0;
 }
 
+function applyOffsets(groups: RepositoryGroup[]) {
+  let offset = 0;
+  for (const group of groups) {
+    group.currentPageOffset = offset;
+    offset += group.repositories.length;
+  }
+}
+
 function findNamespace(namespaces: NamespaceCollection, namespaceToFind: string) {
   return namespaces._embedded.namespaces.find((namespace) => namespace.namespace === namespaceToFind);
 }