Skip to content

Commit

Permalink
fix(archives): fix duplicate archived recording displays (#820)
Browse files Browse the repository at this point in the history 
* fix(archives): fix duplicate archived recording displays

Signed-off-by: Thuan Vo <thvo@redhat.com>

* fix(archives): fix hook deps and add missing listener

Signed-off-by: Thuan Vo <thvo@redhat.com>
  • Loading branch information
@tthvo
tthvo authored Jan 17, 2023
1 parent 4fc8e25 commit 3193f1c
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 22 deletions.
63 changes: 44 additions & 19 deletions src/app/Archives/AllTargetsArchivedRecordingsTable.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,20 @@ import {
import { SearchIcon } from '@patternfly/react-icons';
import { TableComposable, Th, Thead, Tbody, Tr, Td, ExpandableRowContent } from '@patternfly/react-table';
import * as React from 'react';
import { of } from 'rxjs';
import { Observable, of } from 'rxjs';
import { map } from 'rxjs/operators';

export interface AllTargetsArchivedRecordingsTableProps {}

export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecordingsTableProps> = () => {
const context = React.useContext(ServiceContext);

const [targets, setTargets] = React.useState([] as Target[]);
const [targets, setTargets] = React.useState(
[] as {
target: Target;
targetAsObs: Observable<Target>;
}[]
);
const [counts, setCounts] = React.useState(new Map<string, number>());
const [searchText, setSearchText] = React.useState('');
const [searchedTargets, setSearchedTargets] = React.useState([] as Target[]);
Expand All @@ -91,14 +96,17 @@ export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecor
const handleTargetsAndCounts = React.useCallback(
/* eslint-disable-next-line @typescript-eslint/no-explicit-any */
(targetNodes: any) => {
const updatedTargets: Target[] = [];
const updatedTargets: {
target: Target;
targetAsObs: Observable<Target>;
}[] = [];
const updatedCounts = new Map<string, number>();
for (const node of targetNodes) {
const target: Target = {
connectUrl: node.target.serviceUri,
alias: node.target.alias,
};
updatedTargets.push(target);
updatedTargets.push({ target: target, targetAsObs: of(target) });
updatedCounts.set(target.connectUrl, node.recordings.archived.aggregate.count as number);
}
setTargets(updatedTargets);
Expand Down Expand Up @@ -172,7 +180,7 @@ export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecor
const handleLostTarget = React.useCallback(
(target: Target) => {
setTargets((old) => {
return old.filter((t) => !isEqualTarget(t, target));
return old.filter(({ target: t }) => !isEqualTarget(t, target));
});
setExpandedTargets((old) => old.filter((t) => !isEqualTarget(t, target)));
setCounts((old) => {
Expand All @@ -191,8 +199,15 @@ export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecor
alias: evt.serviceRef.alias,
};
if (evt.kind === 'FOUND') {
setTargets((old) => old.concat(target));
setTargets((old) => old.concat({ target: target, targetAsObs: of(target) }));
getCountForNewTarget(target);
} else if (evt.kind === 'MODIFIED') {
setTargets((old) => {
return [...old.filter(({ target: t }) => !isEqualTarget(t, target))].concat({
target: target,
targetAsObs: of(target),
});
});
} else if (evt.kind === 'LOST') {
handleLostTarget(target);
}
Expand All @@ -218,14 +233,16 @@ export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecor
React.useEffect(() => {
let updatedSearchedTargets: Target[];
if (!searchText) {
updatedSearchedTargets = targets;
updatedSearchedTargets = targets.map((t) => t.target);
} else {
const formattedSearchText = searchText.trim().toLowerCase();
updatedSearchedTargets = targets.filter(
(t: Target) =>
t.alias.toLowerCase().includes(formattedSearchText) ||
t.connectUrl.toLowerCase().includes(formattedSearchText)
);
updatedSearchedTargets = targets
.map((t) => t.target)
.filter(
(t: Target) =>
t.alias.toLowerCase().includes(formattedSearchText) ||
t.connectUrl.toLowerCase().includes(formattedSearchText)
);
}
setSearchedTargets(updatedSearchedTargets);
}, [searchText, targets, setSearchedTargets]);
Expand All @@ -247,23 +264,31 @@ export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecor
.messages(NotificationCategory.TargetJvmDiscovery)
.subscribe((v) => handleTargetNotification(v.message.event))
);
}, [addSubscription, context, context.notificationChannel, handleTargetNotification]);
}, [addSubscription, context.notificationChannel, handleTargetNotification]);

React.useEffect(() => {
addSubscription(
context.notificationChannel.messages(NotificationCategory.ActiveRecordingSaved).subscribe((v) => {
updateCount(v.message.target, 1);
})
);
}, [addSubscription, context, context.notificationChannel, updateCount]);
}, [addSubscription, context.notificationChannel, updateCount]);

React.useEffect(() => {
addSubscription(
context.notificationChannel.messages(NotificationCategory.ArchivedRecordingCreated).subscribe((v) => {
updateCount(v.message.target, 1);
})
);
}, [addSubscription, context.notificationChannel, updateCount]);

React.useEffect(() => {
addSubscription(
context.notificationChannel.messages(NotificationCategory.ArchivedRecordingDeleted).subscribe((v) => {
updateCount(v.message.target, -1);
})
);
}, [addSubscription, context, context.notificationChannel, updateCount]);
}, [addSubscription, context.notificationChannel, updateCount]);

const toggleExpanded = React.useCallback(
(target) => {
Expand All @@ -278,15 +303,15 @@ export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecor
);

const isHidden = React.useMemo(() => {
return targets.map((target) => {
return targets.map(({ target }) => {
return (
!includesTarget(searchedTargets, target) || (hideEmptyTargets && (counts.get(target.connectUrl) || 0) === 0)
);
});
}, [targets, searchedTargets, hideEmptyTargets, counts]);

const targetRows = React.useMemo(() => {
return targets.map((target, idx) => {
return targets.map(({ target }, idx) => {
const isExpanded: boolean = includesTarget(expandedTargets, target);

const handleToggle = () => {
Expand Down Expand Up @@ -321,15 +346,15 @@ export const AllTargetsArchivedRecordingsTable: React.FC<AllTargetsArchivedRecor
}, [toggleExpanded, targets, expandedTargets, counts, isHidden, tableColumns]);

const recordingRows = React.useMemo(() => {
return targets.map((target, idx) => {
return targets.map(({ target, targetAsObs }, idx) => {
const isExpanded: boolean = includesTarget(expandedTargets, target);

return (
<Tr key={`${idx}_child`} isExpanded={isExpanded} isHidden={isHidden[idx]}>
<Td key={`target-ex-expand-${idx}`} dataLabel={'Content Details'} colSpan={tableColumns.length + 1}>
{isExpanded ? (
<ExpandableRowContent>
<ArchivedRecordingsTable target={of(target)} isUploadsTable={false} isNestedTable={true} />
<ArchivedRecordingsTable target={targetAsObs} isUploadsTable={false} isNestedTable={true} />
</ExpandableRowContent>
) : null}
</Td>
Expand Down
4 changes: 3 additions & 1 deletion src/app/Recordings/ArchivedRecordingsTable.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,9 @@ export const ArchivedRecordingsTable: React.FC<ArchivedRecordingsTableProps> = (
if (currentTarget.connectUrl != event.message.target) {
return;
}
setRecordings((old) => old.concat(event.message.recording));
setRecordings((old) =>
old.filter((r) => r.name !== event.message.recording.name).concat(event.message.recording)
);
})
);
}, [addSubscription, context.notificationChannel, setRecordings, propsTarget]);
Expand Down
6 changes: 4 additions & 2 deletions src/app/Recordings/Recordings.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,16 @@ export const Recordings: React.FunctionComponent<RecordingsProps> = (_) => {

const onTabSelect = React.useCallback((_, idx) => setActiveTab(Number(idx)), [setActiveTab]);

const targetAsObs = React.useMemo(() => context.target.target(), [context.target]);

const cardBody = React.useMemo(() => {
return archiveEnabled ? (
<Tabs id="recordings" activeKey={activeTab} onSelect={onTabSelect}>
<Tab id="active-recordings" eventKey={0} title={<TabTitleText>Active Recordings</TabTitleText>}>
<ActiveRecordingsTable archiveEnabled={true} />
</Tab>
<Tab id="archived-recordings" eventKey={1} title={<TabTitleText>Archived Recordings</TabTitleText>}>
<ArchivedRecordingsTable target={context.target.target()} isUploadsTable={false} isNestedTable={false} />
<ArchivedRecordingsTable target={targetAsObs} isUploadsTable={false} isNestedTable={false} />
</Tab>
</Tabs>
) : (
Expand All @@ -73,7 +75,7 @@ export const Recordings: React.FunctionComponent<RecordingsProps> = (_) => {
<ActiveRecordingsTable archiveEnabled={false} />
</>
);
}, [archiveEnabled, activeTab, onTabSelect, context.target]);
}, [archiveEnabled, activeTab, onTabSelect, targetAsObs]);

return (
<TargetView pageTitle="Recordings">
Expand Down
10 changes: 10 additions & 0 deletions src/test/Archives/AllTargetsArchivedRecordingsTable.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,41 +190,51 @@ jest
.mockReturnValueOnce(of()) // renders correctly
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of()) // has the correct table elements
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of()) // hides targets with zero recordings
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of()) // correctly handles the search function
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of()) // expands targets to show their <ArchivedRecordingsTable />
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of()) // does not expand targets with zero recordings
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of(mockTargetFoundNotification)) // adds a target upon receiving a notification
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of(mockTargetLostNotification)) // removes a target upon receiving a notification
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of()) // increments the count when an archived recording is saved
.mockReturnValueOnce(of(mockRecordingSavedNotification))
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())

.mockReturnValueOnce(of()) // decrements the count when an archived recording is deleted
.mockReturnValueOnce(of())
.mockReturnValueOnce(of())
.mockReturnValueOnce(of(mockRecordingDeletedNotification));

describe('<AllTargetsArchivedRecordingsTable />', () => {
Expand Down

0 comments on commit 3193f1c

Please sign in to comment.