From ea42064741e28c2d7650bbe22d305528d95cff0d Mon Sep 17 00:00:00 2001 From: Thuan Vo Date: Tue, 11 Oct 2022 19:31:08 -0400 Subject: [PATCH 1/2] fix(archive): all-archive should properly display when refreshing --- .../AllTargetsArchivedRecordingsTable.tsx | 134 +++++++++--------- src/app/Archives/Archives.tsx | 9 +- src/app/Recordings/ActiveRecordingsTable.tsx | 3 +- .../Recordings/ArchivedRecordingsTable.tsx | 2 +- src/app/Shared/Services/Target.service.tsx | 18 +++ ...AllTargetsArchivedRecordingsTable.test.tsx | 24 +--- 6 files changed, 96 insertions(+), 94 deletions(-) diff --git a/src/app/Archives/AllTargetsArchivedRecordingsTable.tsx b/src/app/Archives/AllTargetsArchivedRecordingsTable.tsx index 5f9b469f3..1af035417 100644 --- a/src/app/Archives/AllTargetsArchivedRecordingsTable.tsx +++ b/src/app/Archives/AllTargetsArchivedRecordingsTable.tsx @@ -37,7 +37,7 @@ */ import * as React from 'react'; import { ServiceContext } from '@app/Shared/Services/Services'; -import { Target } from '@app/Shared/Services/Target.service'; +import { includesTarget, indexOfTarget, isEqualTarget, Target } from '@app/Shared/Services/Target.service'; import { NotificationCategory } from '@app/Shared/Services/NotificationChannel.service'; import { useSubscriptions } from '@app/utils/useSubscriptions'; import { @@ -56,10 +56,9 @@ import { TableComposable, Th, Thead, Tbody, Tr, Td, ExpandableRowContent } from import SearchIcon from '@patternfly/react-icons/dist/esm/icons/search-icon'; import { ArchivedRecordingsTable } from '@app/Recordings/ArchivedRecordingsTable'; import { of } from 'rxjs'; -import { concatMap, map } from 'rxjs/operators'; +import { map } from 'rxjs/operators'; import { TargetDiscoveryEvent } from '@app/Shared/Services/Targets.service'; import { LoadingView } from '@app/LoadingView/LoadingView'; -import _ from 'lodash'; export interface AllTargetsArchivedRecordingsTableProps {} @@ -68,45 +67,39 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent()); + const [searchText, setSearchText] = React.useState(''); const [searchedTargets, setSearchedTargets] = React.useState([] as Target[]); const [expandedTargets, setExpandedTargets] = React.useState([] as Target[]); const [hideEmptyTargets, setHideEmptyTargets] = React.useState(true); const [isLoading, setIsLoading] = React.useState(false); const addSubscription = useSubscriptions(); - const searchedTargetsRef = React.useRef(searchedTargets); - - const tableColumns: string[] = ['Target', 'Count']; + const tableColumns: string[] = React.useMemo(() => ['Target', 'Count'], []); const updateCount = React.useCallback( (connectUrl: string, delta: number) => { - for (let i = 0; i < targets.length; i++) { - if (targets[i].connectUrl === connectUrl) { - setCounts((old) => { - let updated = [...old]; - updated[i] += delta; - return updated; - }); - break; - } - } + setCounts((old) => { + const newMap = new Map(old); + const curr = newMap.get(connectUrl) || 0; + newMap.set(connectUrl, curr + delta); + return newMap; + }); }, - [targets, setCounts] + [setCounts] ); const handleTargetsAndCounts = React.useCallback( - (targetNodes) => { - let updatedTargets: Target[] = []; - let updatedCounts: number[] = []; + (targetNodes: any) => { + const updatedTargets: Target[] = []; + const updatedCounts = new Map(); for (const node of targetNodes) { const target: Target = { connectUrl: node.target.serviceUri, alias: node.target.alias, }; updatedTargets.push(target); - updatedCounts.push(node.recordings.archived.aggregate.count as number); + updatedCounts.set(target.connectUrl, node.recordings.archived.aggregate.count as number); } setTargets(updatedTargets); setCounts(updatedCounts); @@ -161,7 +154,11 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent - setCounts((old) => old.concat(v.data.targetNodes[0].recordings.archived.aggregate.count as number)) + setCounts((old) => { + const newMap = new Map(old); + newMap.set(target.connectUrl, v.data.targetNodes[0].recordings.archived.aggregate.count as number); + return newMap; + }) ) ); }, @@ -170,18 +167,14 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent { - let idx; setTargets((old) => { - for (idx = 0; idx < old.length; idx++) { - if (_.isEqual(target, old[idx])) break; - } - return old.filter((o) => !_.isEqual(o, target)); + return old.filter((t) => !isEqualTarget(t, target)); }); - setExpandedTargets((old) => old.filter((o) => !_.isEqual(o, target))); + setExpandedTargets((old) => old.filter((t) => !isEqualTarget(t, target))); setCounts((old) => { - let updated = [...old]; - updated.splice(idx, 1); - return updated; + const newMap = new Map(old); + newMap.delete(target.connectUrl); + return newMap; }); }, [setTargets, setExpandedTargets, setCounts] @@ -203,13 +196,35 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent { + setSearchText(searchInput); + }, + [setSearchText] + ); + + const handleSearchInputClear = React.useCallback(() => { + handleSearchInput(''); + }, [handleSearchInput]); + React.useEffect(() => { refreshTargetsAndCounts(); - }, []); + }, [refreshTargetsAndCounts]); React.useEffect(() => { - searchedTargetsRef.current = searchedTargets; - }); + let updatedSearchedTargets: Target[]; + if (!searchText) { + updatedSearchedTargets = targets; + } else { + const formattedSearchText = searchText.trim().toLowerCase(); + updatedSearchedTargets = targets.filter( + (t: Target) => + t.alias.toLowerCase().includes(formattedSearchText) || + t.connectUrl.toLowerCase().includes(formattedSearchText) + ); + } + setSearchedTargets(updatedSearchedTargets); + }, [searchText, targets, setSearchedTargets]); React.useEffect(() => { if (!context.settings.autoRefreshEnabled()) { @@ -222,28 +237,11 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent window.clearInterval(id); }, [context.target, context.settings, refreshTargetsAndCounts]); - React.useEffect(() => { - let updatedSearchedTargets; - if (!search) { - updatedSearchedTargets = targets; - } else { - const searchText = search.trim().toLowerCase(); - updatedSearchedTargets = targets.filter( - (t: Target) => t.alias.toLowerCase().includes(searchText) || t.connectUrl.toLowerCase().includes(searchText) - ); - } - - if (!_.isEqual(searchedTargetsRef.current, updatedSearchedTargets)) { - setSearchedTargets(updatedSearchedTargets); - } - }, [search, targets]); - React.useEffect(() => { addSubscription( context.notificationChannel .messages(NotificationCategory.TargetJvmDiscovery) - .pipe(concatMap((v) => of(handleTargetNotification(v.message.event)))) - .subscribe(() => {} /* do nothing - callback will have already handled updating state */) + .subscribe((v) => handleTargetNotification(v.message.event)) ); }, [addSubscription, context, context.notificationChannel, handleTargetNotification]); @@ -265,30 +263,30 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent { - const idx = expandedTargets.indexOf(target); + const idx = indexOfTarget(expandedTargets, target); setExpandedTargets((expandedTargets) => idx >= 0 ? [...expandedTargets.slice(0, idx), ...expandedTargets.slice(idx + 1, expandedTargets.length)] : [...expandedTargets, target] ); }, - [expandedTargets] + [expandedTargets, setExpandedTargets] ); const isHidden = React.useMemo(() => { - let isHidden: boolean[] = []; - targets.map((target, idx) => { - isHidden.push(!searchedTargets.includes(target) || (hideEmptyTargets && counts[idx] === 0)); + return targets.map((target) => { + return ( + !includesTarget(searchedTargets, target) || (hideEmptyTargets && (counts.get(target.connectUrl) || 0) === 0) + ); }); - return isHidden; }, [targets, searchedTargets, hideEmptyTargets, counts]); const targetRows = React.useMemo(() => { return targets.map((target, idx) => { - let isExpanded: boolean = expandedTargets.includes(target); + let isExpanded: boolean = includesTarget(expandedTargets, target); const handleToggle = () => { - if (counts[idx] !== 0 || isExpanded) { + if ((counts.get(target.connectUrl) || 0) !== 0 || isExpanded) { toggleExpanded(target); } }; @@ -311,7 +309,7 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent - {counts[idx]} + {counts.get(target.connectUrl) || 0} ); @@ -320,7 +318,7 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent { return targets.map((target, idx) => { - let isExpanded: boolean = expandedTargets.includes(target); + let isExpanded: boolean = includesTarget(expandedTargets, target); return ( @@ -391,9 +389,9 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent setSearch('')} + value={searchText} + onChange={handleSearchInput} + onClear={handleSearchInputClear} /> @@ -402,7 +400,7 @@ export const AllTargetsArchivedRecordingsTable: React.FunctionComponent setHideEmptyTargets((old) => !old)} + onChange={setHideEmptyTargets} isChecked={hideEmptyTargets} id={`all-archives-hide-check`} aria-label={`all-archives-hide-check`} diff --git a/src/app/Archives/Archives.tsx b/src/app/Archives/Archives.tsx index f88f9d1a7..3e067d101 100644 --- a/src/app/Archives/Archives.tsx +++ b/src/app/Archives/Archives.tsx @@ -45,6 +45,7 @@ import { ArchivedRecordingsTable } from '@app/Recordings/ArchivedRecordingsTable import { Target } from '@app/Shared/Services/Target.service'; import { of } from 'rxjs'; import { UPLOADS_SUBDIRECTORY } from '@app/Shared/Services/Api.service'; +import { useSubscriptions } from '@app/utils/useSubscriptions'; /* This specific target is used as the "source" for the Uploads version of the ArchivedRecordingsTable. @@ -58,14 +59,16 @@ export const uploadAsTarget: Target = { alias: '', }; -export const Archives = () => { +export interface ArchivesProps {} + +export const Archives: React.FunctionComponent = () => { const context = React.useContext(ServiceContext); + const addSubscription = useSubscriptions(); const [activeTab, setActiveTab] = React.useState(0); const [archiveEnabled, setArchiveEnabled] = React.useState(false); React.useEffect(() => { - const sub = context.api.isArchiveEnabled().subscribe(setArchiveEnabled); - return () => sub.unsubscribe(); + addSubscription(context.api.isArchiveEnabled().subscribe(setArchiveEnabled)); }, [context.api]); const cardBody = React.useMemo(() => { diff --git a/src/app/Recordings/ActiveRecordingsTable.tsx b/src/app/Recordings/ActiveRecordingsTable.tsx index 240e78e71..e75fabf63 100644 --- a/src/app/Recordings/ActiveRecordingsTable.tsx +++ b/src/app/Recordings/ActiveRecordingsTable.tsx @@ -500,9 +500,8 @@ export const ActiveRecordingsTable: React.FunctionComponent + {parentRow} {childRow} diff --git a/src/app/Recordings/ArchivedRecordingsTable.tsx b/src/app/Recordings/ArchivedRecordingsTable.tsx index 1cf84e123..d24e1fb60 100644 --- a/src/app/Recordings/ArchivedRecordingsTable.tsx +++ b/src/app/Recordings/ArchivedRecordingsTable.tsx @@ -441,7 +441,7 @@ export const ArchivedRecordingsTable: React.FunctionComponent + {parentRow} {childRow} diff --git a/src/app/Shared/Services/Target.service.tsx b/src/app/Shared/Services/Target.service.tsx index 1540442a7..d2dbb6a12 100644 --- a/src/app/Shared/Services/Target.service.tsx +++ b/src/app/Shared/Services/Target.service.tsx @@ -39,6 +39,24 @@ import { Observable, Subject, BehaviorSubject } from 'rxjs'; export const NO_TARGET = {} as Target; +export const includesTarget = (arr: Target[], target: Target): boolean => { + return arr.some((t) => t.connectUrl === target.connectUrl); +}; + +export const isEqualTarget = (a: Target, b: Target): boolean => { + return a.connectUrl === b.connectUrl; +}; + +export const indexOfTarget = (arr: Target[], target: Target): number => { + let index = -1; + arr.forEach((t, idx) => { + if (t.connectUrl === target.connectUrl) { + index = idx; + } + }); + return index; +}; + export interface Target { connectUrl: string; alias: string; diff --git a/src/test/Archives/AllTargetsArchivedRecordingsTable.test.tsx b/src/test/Archives/AllTargetsArchivedRecordingsTable.test.tsx index 5cac3e910..5253d50a7 100644 --- a/src/test/Archives/AllTargetsArchivedRecordingsTable.test.tsx +++ b/src/test/Archives/AllTargetsArchivedRecordingsTable.test.tsx @@ -161,6 +161,10 @@ jest.mock('@app/Recordings/ArchivedRecordingsTable', () => { }; }); +jest.mock('@app/Shared/Services/Target.service', () => ({ + ...jest.requireActual('@app/Shared/Services/Target.service'), // Require actual implementation of utility functions for Target +})); + jest .spyOn(defaultServices.api, 'graphql') .mockReturnValueOnce(of(mockTargetsAndCountsResponse)) // renders correctly @@ -185,61 +189,41 @@ jest .mockReturnValueOnce(of()) // renders correctly .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) // has the correct table elements .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) // hides targets with zero recordings .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) // correctly handles the search function .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) // expands targets to show their .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) // does not expand targets with zero recordings .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of(mockTargetFoundNotification)) // adds a target upon receiving a notification .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of(mockTargetLostNotification)) // removes a target upon receiving a notification .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) // increments the count when an archived recording is saved - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of(mockRecordingSavedNotification)) .mockReturnValueOnce(of()) .mockReturnValueOnce(of()) // decrements the count when an archived recording is deleted .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) - .mockReturnValueOnce(of()) .mockReturnValueOnce(of(mockRecordingDeletedNotification)); describe('', () => { From 41ac9d61e916b5f1d61984ae72aa260d6b674499 Mon Sep 17 00:00:00 2001 From: Thuan Vo Date: Wed, 12 Oct 2022 14:35:21 -0400 Subject: [PATCH 2/2] fix(hooks): add missing deps --- src/app/Archives/Archives.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/Archives/Archives.tsx b/src/app/Archives/Archives.tsx index 3e067d101..a08406383 100644 --- a/src/app/Archives/Archives.tsx +++ b/src/app/Archives/Archives.tsx @@ -69,7 +69,7 @@ export const Archives: React.FunctionComponent = () => { React.useEffect(() => { addSubscription(context.api.isArchiveEnabled().subscribe(setArchiveEnabled)); - }, [context.api]); + }, [context.api, addSubscription, setArchiveEnabled]); const cardBody = React.useMemo(() => { return archiveEnabled ? (