Skip to content

Commit

Permalink
feat: useMemoWithPrevious React hook (#2820)
Browse files Browse the repository at this point in the history
Resolves #2819

**Changes:**
Introduces a new `useMemoWithPrevious` hook to manage previous and current values with reset capability, replacing the manual `useRef` implementation in ContainerLogModal. This hook provides a more robust way to track state changes while maintaining previous values.

**Implementation Details:**
- Created new `useMemoWithPrevious` hook that returns both current and previous values along with a reset function
- Updated ContainerLogModal to use the new hook for tracking log line numbers
- Replaced manual `previousLastLineNumber` ref management with the new hook's `resetPrevious` functionality
- Improved state management when switching kernel IDs or changing log sizes

**Testing Requirements:**
- Verify log highlighting works correctly when switching between different kernels
- Confirm previous line numbers are properly reset when changing log sizes
- Check that log display maintains proper highlighting of new content
  • Loading branch information
yomybaby committed Nov 8, 2024
1 parent d286c90 commit 5702e0a
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 10 deletions.
17 changes: 7 additions & 10 deletions react/src/components/ComputeSessionNodeItems/ContainerLogModal.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { downloadBlob } from '../../helper/csv-util';
import { useSuspendedBackendaiClient } from '../../hooks';
import { useTanQuery } from '../../hooks/reactQueryAlias';
import { useMemoWithPrevious } from '../../hooks/useMemoWithPrevious';
import BAIModal, { BAIModalProps } from '../BAIModal';
import Flex from '../Flex';
import { ContainerLogModalFragment$key } from './__generated__/ContainerLogModalFragment.graphql';
Expand All @@ -18,7 +19,7 @@ import {
import graphql from 'babel-plugin-relay/macro';
import _ from 'lodash';
import { DownloadIcon } from 'lucide-react';
import React, { useEffect, useRef, useState } from 'react';
import React, { useState } from 'react';
import { useTranslation } from 'react-i18next';
import { useFragment } from 'react-relay';

Expand Down Expand Up @@ -112,15 +113,11 @@ const ContainerLogModal: React.FC<ContainerLogModalProps> = ({
// console.log(signed)
// console.log(signed.uri);

const previousLastLineNumber = useRef(0);

useEffect(() => {
previousLastLineNumber.current = logs?.split('\n').length || 0;
}, [logs]);
const [lastLineNumbers, { resetPrevious: resetPreviousLineNumber }] =
useMemoWithPrevious(() => logs?.split('\n').length || 0, [logs]);

const { md } = Grid.useBreakpoint();
const { t } = useTranslation();
const fixedPreviousLastLineNumber = previousLastLineNumber.current;

return (
<BAIModal
Expand Down Expand Up @@ -170,7 +167,7 @@ const ContainerLogModal: React.FC<ContainerLogModalProps> = ({
value={selectedKernelId}
onChange={(value) => {
setSelectedKernelId(value);
previousLastLineNumber.current = 0; // reset previous last line number
resetPreviousLineNumber();
}}
options={_.chain(session?.kernel_nodes?.edges)
.map((e) => {
Expand Down Expand Up @@ -199,7 +196,7 @@ const ContainerLogModal: React.FC<ContainerLogModalProps> = ({
onChange={(value) => {
setLogSize(value);
if(value!=='full'){
previousLastLineNumber.current = 0; // reset previous last line number
resetPreviousLineNumber();
}
refetch();
}}
Expand Down Expand Up @@ -248,7 +245,7 @@ const ContainerLogModal: React.FC<ContainerLogModalProps> = ({
enableSearchNavigation
selectableLines
text={logs || ''}
highlight={fixedPreviousLastLineNumber}
highlight={lastLineNumbers.previous}
extraLines={1}
// url={signed.uri}
// fetchOptions={
Expand Down
91 changes: 91 additions & 0 deletions react/src/hooks/useMemoWithPrevious.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { useMemoWithPrevious } from './useMemoWithPrevious';
import { renderHook, act } from '@testing-library/react';

describe('useMemoWithPrevious Hook', () => {
it('should initialize with initialPrev', () => {
const factory = jest.fn(() => 'current value');
const { result } = renderHook(() =>
useMemoWithPrevious(factory, [], {
initialPrev: 'initial previous value',
}),
);

expect(result.current[0].current).toBe('current value');
expect(result.current[0].previous).toBe('initial previous value');
expect(factory).toHaveBeenCalledTimes(1);
});

it('should update current and previous when dependencies change', () => {
let dep = 1;
const factory = jest.fn(() => `current value ${dep}`);

const { result, rerender } = renderHook(() =>
useMemoWithPrevious(factory, [dep]),
);

// Initial render
expect(result.current[0].current).toBe('current value 1');
expect(result.current[0].previous).toBeUndefined();
expect(factory).toHaveBeenCalledTimes(1);

// Update dep and rerender
// act(() => {
dep = 2;
rerender();
// });

// After dependency change
expect(result.current[0].current).toBe('current value 2');
expect(result.current[0].previous).toBe('current value 1');
expect(factory).toHaveBeenCalledTimes(2);
});

it('should reset previous when resetPrevious is called', () => {
let dep = 1;
const factory = jest.fn(() => `current value ${dep}`);

const { result, rerender } = renderHook(() =>
useMemoWithPrevious(factory, [dep], {
initialPrev: 'initial previous value',
}),
);

// Initial render
expect(result.current[0].current).toBe('current value 1');
expect(result.current[0].previous).toBe('initial previous value');

// Update dep and rerender
dep = 2;
rerender();

expect(result.current[0].current).toBe('current value 2');
expect(result.current[0].previous).toBe('current value 1');

// Call resetPrevious
act(() => {
result.current[1].resetPrevious();
});

expect(result.current[0].current).toBe('current value 2');
expect(result.current[0].previous).toBe('initial previous value');
expect(factory).toHaveBeenCalledTimes(2);
});

it('should not update previous if dependencies do not change', () => {
const factory = jest.fn(() => 'current value');
const { result, rerender } = renderHook(() =>
useMemoWithPrevious(factory, []),
);

// Initial render
expect(result.current[0].current).toBe('current value');
expect(result.current[0].previous).toBeUndefined();

// Rerender without changing dependencies
rerender();

expect(result.current[0].current).toBe('current value');
expect(result.current[0].previous).toBeUndefined();
expect(factory).toHaveBeenCalledTimes(1);
});
});
37 changes: 37 additions & 0 deletions react/src/hooks/useMemoWithPrevious.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { DependencyList, useEffect, useMemo, useRef, useState } from 'react';

export const useMemoWithPrevious = <T,>(
factory: () => T,
deps: DependencyList,
{ initialPrev }: { initialPrev?: T } | undefined = {},
) => {
const prevRef = useRef(initialPrev);
const [prevResetKey, setPrevResetKey] = useState({});

// eslint-disable-next-line react-hooks/exhaustive-deps
const current = useMemo(factory, deps);
const memoizedPrev = useMemo(() => {
return prevRef.current;
// Only update when the reset key changes and deps change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [...deps, prevResetKey]);

useEffect(() => {
prevRef.current = current;
// Only update when deps change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps);

return [
{
previous: memoizedPrev,
current: current,
},
{
resetPrevious: () => {
prevRef.current = initialPrev;
setPrevResetKey({});
},
},
] as const;
};

0 comments on commit 5702e0a

Please sign in to comment.