From 395a2edeb306cec808f1bb5b0e3e26dd8a2c9755 Mon Sep 17 00:00:00 2001 From: Thomas Zerr Date: Wed, 9 Apr 2025 10:54:53 +0200 Subject: [PATCH] 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 | 2 + .../src/repos/RepositoryEntry.tsx | 7 +- scm-ui/ui-core/src/base/shortcuts/index.ts | 7 +- .../shortcuts/iterator/callbackIterator.ts | 107 ++++++++++++------ .../iterator/keyboardIterator.test.tsx | 5 - .../shortcuts/iterator/keyboardIterator.tsx | 55 ++++++++- scm-ui/ui-core/src/index.ts | 2 +- scm-ui/ui-types/src/Repositories.ts | 1 + .../codeSection/components/FileSearchHit.tsx | 9 +- .../components/FileSearchResults.tsx | 4 +- .../components/list/RepositoryGroupEntry.tsx | 10 +- .../components/list/groupByNamespace.test.ts | 3 + .../repos/components/list/groupByNamespace.ts | 15 ++- 13 files changed, 172 insertions(+), 55 deletions(-) create mode 100644 gradle/changelog/fix-keyboard-iterator.yaml diff --git a/gradle/changelog/fix-keyboard-iterator.yaml b/gradle/changelog/fix-keyboard-iterator.yaml new file mode 100644 index 0000000000..26d505f28e --- /dev/null +++ b/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 diff --git a/scm-ui/ui-components/src/repos/RepositoryEntry.tsx b/scm-ui/ui-components/src/repos/RepositoryEntry.tsx index dcff63a8d4..c1ad26f160 100644 --- a/scm-ui/ui-components/src/repos/RepositoryEntry.tsx +++ b/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 = ({ repository, baseDate }) => { +const RepositoryEntry: FC = ({ repository, baseDate, expectedIndex }) => { const [t] = useTranslation("repos"); - const ref = useKeyboardIteratorTarget(); + const ref = useKeyboardIteratorTargetV2({ expectedIndex: expectedIndex ?? 0 }); const actions = () => ( diff --git a/scm-ui/ui-core/src/base/shortcuts/index.ts b/scm-ui/ui-core/src/base/shortcuts/index.ts index 752384a0b4..532a0cb98d 100644 --- a/scm-ui/ui-core/src/base/shortcuts/index.ts +++ b/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"; diff --git a/scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts b/scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts index 8ce6103285..6c11d86a13 100644 --- a/scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts +++ b/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, - private readonly itemsRef: MutableRefObject> + private readonly itemsRef: MutableRefObject> ) {} 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>([]); + const items = useRef>([]); const activeIndex = useRef(initialIndex); return useMemo(() => new CallbackIterator(activeIndex, items), [activeIndex, items]); }; diff --git a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx b/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx index 506a5a4af3..c1ba877467 100644 --- a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx +++ b/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", () => { diff --git a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx b/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx index 6b504237a6..a28b707c77 100644 --- a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx +++ b/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({ register: () => { @@ -33,6 +39,18 @@ const KeyboardIteratorContext = React.createContext({ 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 { 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 = + */ +export function useKeyboardIteratorTargetV2({ + expectedIndex, +}: { + expectedIndex: number; +}): React.RefCallback { + const ref = useRef(); + const callback = useCallback(() => ref.current?.focus(), []); + const cleanup = useCallback(() => ref.current?.blur(), []); + const refCallback: React.RefCallback = 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. * diff --git a/scm-ui/ui-core/src/index.ts b/scm-ui/ui-core/src/index.ts index 21b0c2961c..e94a48d9ef 100644 --- a/scm-ui/ui-core/src/index.ts +++ b/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"; diff --git a/scm-ui/ui-types/src/Repositories.ts b/scm-ui/ui-types/src/Repositories.ts index a6963902ac..194828ab3b 100644 --- a/scm-ui/ui-types/src/Repositories.ts +++ b/scm-ui/ui-types/src/Repositories.ts @@ -85,4 +85,5 @@ export type RepositoryGroup = { name: string; namespace?: Namespace; repositories: Repository[]; + currentPageOffset?: number; }; diff --git a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx b/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx index 8fbb3c127c..b52801e06c 100644 --- a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx +++ b/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 ( - + {path} diff --git a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx b/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx index 9a13648ace..5a00fc28f6 100644 --- a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx +++ b/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx @@ -32,8 +32,8 @@ const ResultTable: FC = ({ contentBaseUrl, paths }) => { - {paths.map((path) => ( - + {paths.map((path, index) => ( + ))} diff --git a/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx b/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx index 16a32c2a06..9477b99368 100644 --- a/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx +++ b/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx @@ -23,8 +23,14 @@ type Props = { }; const RepositoryGroupEntry: FC = ({ group }) => { - const entries = group.repositories.map((repository) => { - return ; + const entries = group.repositories.map((repository, index) => { + return ( + + ); }); return ; }; diff --git a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts b/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts index 280fc688f3..4287c53b96 100644 --- a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts +++ b/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, }, ]; diff --git a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts b/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts index 384a3e0565..3fe221db85 100644 --- a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts +++ b/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 = {}; 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); }