Skip to content

Commit

Permalink
Merge branch 'release/v1.0.2' into bugfix/spotlight-event-listeners
Browse files Browse the repository at this point in the history
  • Loading branch information
benjaminpkane committed Oct 31, 2024
2 parents 38e9f7b + 5922bcd commit 33a0040
Show file tree
Hide file tree
Showing 19 changed files with 369 additions and 59 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ jobs:

all-tests:
runs-on: ubuntu-latest
needs: [build, lint, test]
needs: [build, lint, test, e2e]
if: always()
steps:
- run: sh -c ${{
Expand Down
14 changes: 10 additions & 4 deletions app/packages/app/src/components/AnalyticsConsent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,23 @@ export default function AnalyticsConsent({
Help us improve FiftyOne
</Typography>
<Typography marginBottom={1}>
We use cookies to understand how FiftyOne is used and improve the product.
You can help us by enabling anonymous analytics.
We use cookies to understand how FiftyOne is used and improve the
product. You can help us by enabling anonymous analytics.
</Typography>
<Grid container gap={2} justifyContent="end" direction="row">
<Grid item alignContent="center">
<Link style={{ cursor: "pointer" }} onClick={handleDisable}>
<Link
style={{ cursor: "pointer" }}
onClick={handleDisable}
data-cy="btn-disable-cookies"
>
Disable
</Link>
</Grid>
<Grid item>
<Button variant="contained" onClick={handleEnable}>Enable</Button>
<Button variant="contained" onClick={handleEnable}>
Enable
</Button>
</Grid>
</Grid>
</Grid>
Expand Down
6 changes: 4 additions & 2 deletions app/packages/components/src/components/AdaptiveMenu/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,10 @@ export default function AdaptiveMenu(props: AdaptiveMenuPropsType) {
if (!containerElem) return;
hideOverflowingNodes(containerElem, (_: number, lastVisibleItemId) => {
const lastVisibleItem = itemsById[lastVisibleItemId];
const computedHidden = items.length - lastVisibleItem.index - 1;
setHidden(computedHidden);
if (lastVisibleItem?.index) {
const computedHidden = items.length - lastVisibleItem.index - 1;
setHidden(computedHidden);
}
});
}

Expand Down
56 changes: 40 additions & 16 deletions app/packages/core/src/components/Modal/ModalNavigation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ import {
LookerArrowRightIcon,
} from "@fiftyone/components";
import * as fos from "@fiftyone/state";
import React, { useCallback, useRef } from "react";
import React, { useCallback, useEffect, useMemo, useRef } from "react";
import { useRecoilValue, useRecoilValueLoadable } from "recoil";
import styled from "styled-components";
import { createDebouncedNavigator } from "./debouncedNavigator";

const Arrow = styled.span<{
$isRight?: boolean;
Expand Down Expand Up @@ -63,17 +64,40 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
const modal = useRecoilValue(fos.modalSelector);
const navigation = useRecoilValue(fos.modalNavigation);

const navigateNext = useCallback(async () => {
onNavigate();
const result = await navigation?.next();
setModal(result);
}, [navigation, onNavigate, setModal]);
const modalRef = useRef(modal);

const navigatePrevious = useCallback(async () => {
onNavigate();
const result = await navigation?.previous();
setModal(result);
}, [onNavigate, navigation, setModal]);
modalRef.current = modal;

// important: make sure all dependencies of the navigators are referentially stable,
// or else the debouncing mechanism won't work
const nextNavigator = useMemo(
() =>
createDebouncedNavigator({
isNavigationIllegalWhen: () => modalRef.current?.hasNext === false,
navigateFn: (offset) => navigation?.next(offset).then(setModal),
onNavigationStart: onNavigate,
debounceTime: 150,
}),
[navigation, onNavigate, setModal]
);

const previousNavigator = useMemo(
() =>
createDebouncedNavigator({
isNavigationIllegalWhen: () => modalRef.current?.hasPrevious === false,
navigateFn: (offset) => navigation?.previous(offset).then(setModal),
onNavigationStart: onNavigate,
debounceTime: 150,
}),
[navigation, onNavigate, setModal]
);

useEffect(() => {
return () => {
nextNavigator.cleanup();
previousNavigator.cleanup();
};
}, [nextNavigator, previousNavigator]);

const keyboardHandler = useCallback(
(e: KeyboardEvent) => {
Expand All @@ -89,12 +113,12 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
}

if (e.key === "ArrowLeft") {
navigatePrevious();
previousNavigator.navigate();
} else if (e.key === "ArrowRight") {
navigateNext();
nextNavigator.navigate();
}
},
[navigateNext, navigatePrevious]
[nextNavigator, previousNavigator]
);

fos.useEventHandler(document, "keyup", keyboardHandler);
Expand All @@ -109,7 +133,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
<Arrow
$isSidebarVisible={isSidebarVisible}
$sidebarWidth={sidebarwidth}
onClick={navigatePrevious}
onClick={previousNavigator.navigate}
>
<LookerArrowLeftIcon data-cy="nav-left-button" />
</Arrow>
Expand All @@ -119,7 +143,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
$isRight
$isSidebarVisible={isSidebarVisible}
$sidebarWidth={sidebarwidth}
onClick={navigateNext}
onClick={nextNavigator.navigate}
>
<LookerArrowRightIcon data-cy="nav-right-button" />
</Arrow>
Expand Down
181 changes: 181 additions & 0 deletions app/packages/core/src/components/Modal/debouncedNavigator.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { createDebouncedNavigator } from "./debouncedNavigator";

describe("createDebouncedNavigator", () => {
let navigateFn: ReturnType<typeof vi.fn>;
let onNavigationStart: ReturnType<typeof vi.fn>;
let isNavigationIllegalWhen: ReturnType<typeof vi.fn>;
let debouncedNavigator: ReturnType<typeof createDebouncedNavigator>;

beforeEach(() => {
vi.useFakeTimers();
navigateFn = vi.fn();
onNavigationStart = vi.fn();
isNavigationIllegalWhen = vi.fn().mockReturnValue(false);
debouncedNavigator = createDebouncedNavigator({
isNavigationIllegalWhen,
navigateFn,
onNavigationStart,
debounceTime: 100,
});
});

afterEach(() => {
vi.clearAllTimers();
vi.restoreAllMocks();
});

it("should navigate immediately on the first call", () => {
debouncedNavigator.navigate();

expect(isNavigationIllegalWhen).toHaveBeenCalled();
expect(onNavigationStart).toHaveBeenCalledTimes(1);
expect(navigateFn).toHaveBeenCalledWith(1);
});

it("should debounce subsequent calls and accumulate offset", () => {
// immediate call
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();

// only the first call
expect(onNavigationStart).toHaveBeenCalledTimes(1);

// advance time less than debounceTime
vi.advanceTimersByTime(50);
// another accumulated call
debouncedNavigator.navigate();

// advance time to trigger debounce after the last navigate
// need to advance full debounceTime after last call
vi.advanceTimersByTime(100);

// first immediate call + after debounce
expect(onNavigationStart).toHaveBeenCalledTimes(2);
// immediate call
expect(navigateFn).toHaveBeenCalledWith(1);
// accumulated calls
expect(navigateFn).toHaveBeenCalledWith(3);
});

it("should reset after debounce period", () => {
// immediate call
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();

vi.advanceTimersByTime(100);

// next navigate call should be immediate again
debouncedNavigator.navigate();

expect(onNavigationStart).toHaveBeenCalledTimes(3);
expect(navigateFn).toHaveBeenNthCalledWith(1, 1);
// accumulated offset
expect(navigateFn).toHaveBeenNthCalledWith(2, 1);
expect(navigateFn).toHaveBeenNthCalledWith(3, 1);
});

it("should not navigate when isNavigationIllegalWhen returns true", () => {
isNavigationIllegalWhen.mockReturnValueOnce(true);

debouncedNavigator.navigate();

expect(isNavigationIllegalWhen).toHaveBeenCalled();
expect(onNavigationStart).not.toHaveBeenCalled();
expect(navigateFn).not.toHaveBeenCalled();
});

it("should cancel pending navigation when cleanup is called", () => {
// immediate call
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();
debouncedNavigator.cleanup();

vi.advanceTimersByTime(200);

// only the immediate call
expect(onNavigationStart).toHaveBeenCalledTimes(1);
expect(navigateFn).toHaveBeenCalledTimes(1);
expect(navigateFn).toHaveBeenCalledWith(1);
});

it("should clear timeout when isNavigationIllegalWhen returns true during debounce", () => {
// immediate call
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();

isNavigationIllegalWhen.mockReturnValue(true);
// should not accumulate further
debouncedNavigator.navigate();

vi.advanceTimersByTime(100);

// only the initial navigation
expect(onNavigationStart).toHaveBeenCalledTimes(1); //
// only immediate call
expect(navigateFn).toHaveBeenCalledTimes(1);
expect(navigateFn).toHaveBeenCalledWith(1);

// reset mock to allow navigation
isNavigationIllegalWhen.mockReturnValue(false);

// should navigate immediately
debouncedNavigator.navigate();

expect(onNavigationStart).toHaveBeenCalledTimes(2);
expect(navigateFn).toHaveBeenCalledTimes(2);
expect(navigateFn).toHaveBeenCalledWith(1);
});

it("should handle multiple sequences correctly", () => {
// first sequence
// immediate
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();
debouncedNavigator.navigate();

vi.advanceTimersByTime(100);

expect(onNavigationStart).toHaveBeenCalledTimes(2);
expect(navigateFn).toHaveBeenNthCalledWith(1, 1);
expect(navigateFn).toHaveBeenNthCalledWith(2, 2);

// second sequence
// immediate call
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();

vi.advanceTimersByTime(100);

expect(onNavigationStart).toHaveBeenCalledTimes(4);
expect(navigateFn).toHaveBeenNthCalledWith(3, 1);
expect(navigateFn).toHaveBeenNthCalledWith(4, 1);
});

it("should reset accumulatedOffset when isNavigationIllegalWhen returns true", () => {
// immediate call
debouncedNavigator.navigate();
// accumulated
debouncedNavigator.navigate();

isNavigationIllegalWhen.mockReturnValueOnce(true);

// should not accumulate further
debouncedNavigator.navigate();

vi.advanceTimersByTime(100);

// only the immediate call
expect(onNavigationStart).toHaveBeenCalledTimes(1);
expect(navigateFn).toHaveBeenCalledTimes(1);
expect(navigateFn).toHaveBeenCalledWith(1);
});
});
70 changes: 70 additions & 0 deletions app/packages/core/src/components/Modal/debouncedNavigator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
interface DebouncedNavigatorOptions {
isNavigationIllegalWhen: () => boolean;
navigateFn: (offset: number) => Promise<void>;
onNavigationStart: () => void;
debounceTime?: number;
}

export function createDebouncedNavigator({
isNavigationIllegalWhen,
navigateFn,
onNavigationStart,
debounceTime = 100,
}: DebouncedNavigatorOptions) {
let timeout: ReturnType<typeof setTimeout> | null = null;
let accumulatedOffset = 0;
let isFirstCall = true;

const cleanup = () => {
if (timeout) {
clearTimeout(timeout);
timeout = null;
}
accumulatedOffset = 0;
isFirstCall = true;
};

const navigate = () => {
if (isNavigationIllegalWhen()) {
if (timeout) {
clearTimeout(timeout);
timeout = null;
}
// Reset state variables
isFirstCall = true;
accumulatedOffset = 0;
return;
}

if (isFirstCall) {
// first invocation: navigate immediately
onNavigationStart();
navigateFn(1);
accumulatedOffset = 0;
isFirstCall = false;
} else {
// subsequently, accumulate offset
accumulatedOffset += 1;
}

// reset debounce timer
if (timeout) {
clearTimeout(timeout);
}

timeout = setTimeout(() => {
if (accumulatedOffset > 0) {
onNavigationStart();
navigateFn(accumulatedOffset);
accumulatedOffset = 0;
}
timeout = null;
isFirstCall = true;
}, debounceTime);
};

return {
navigate,
cleanup,
};
}
Loading

0 comments on commit 33a0040

Please sign in to comment.