Skip to content

Commit eba19f5

Browse files
Parship12Parship12
authored
Phase 3.2 (task 3): Remove historyUpdateMiddleware from Redux & move navigate(url) into SearchForm (#3582)
## Which problem is this PR solving? - Resolves part of #3313 ## Description of the changes - In `middlewares/index.ts`: The `historyUpdateMiddleware` export and its two dependencies (`replace` and the `searchTraces/getSearchUrl` imports) were removed. The middleware is now a dead code path because navigation is happening in the component layer. - `SearchForm` now call `useNavigate()` and using the returned URL inside `handleSubmit`. So basically, it's replacing what `historyUpdateMiddleware` used to do via `Redux`. ## How was this change tested? - Unit tests - Search form working properly - URL changing correctly after clicking "Find Traces" ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully: `make lint test` ## AI Usage in this PR (choose one) See [AI Usage Policy](https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md#ai-usage-policy). - [ ] **None**: No AI tools were used in creating this PR - [x] **Light**: AI provided minor assistance (formatting, simple suggestions) - [ ] **Moderate**: AI helped with code generation or debugging specific parts - [ ] **Heavy**: AI generated most or all of the code changes Signed-off-by: Parship Chowdhury <parshipchowdhury@gmail.com>
1 parent 72014ef commit eba19f5

File tree

4 files changed

+34
-94
lines changed

4 files changed

+34
-94
lines changed

‎packages/jaeger-ui/src/components/SearchTracePage/SearchForm.test.js‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ jest.mock('../../hooks/useTraceDiscovery', () => ({
3737
}));
3838

3939
import React from 'react';
40+
import { MemoryRouter } from 'react-router-dom-v5-compat';
4041
import { render, fireEvent, waitFor, cleanup } from '@testing-library/react';
4142
import { act } from 'react';
4243
import '@testing-library/jest-dom';
@@ -87,7 +88,7 @@ const defaultProps = {
8788
label: '2 Days',
8889
value: '2d',
8990
},
90-
submitFormHandler: jest.fn(),
91+
submitFormHandler: jest.fn().mockReturnValue('/search'),
9192
};
9293

9394
describe('conversion utils', () => {
@@ -444,6 +445,10 @@ describe('submitForm()', () => {
444445
});
445446
});
446447

448+
function renderForm(ui) {
449+
return render(<MemoryRouter>{ui}</MemoryRouter>);
450+
}
451+
447452
describe('<SearchForm>', () => {
448453
afterEach(cleanup);
449454
beforeEach(() => {
@@ -453,18 +458,18 @@ describe('<SearchForm>', () => {
453458
});
454459

455460
it('enables operations only when a service is selected', async () => {
456-
render(<SearchForm key="fresh" {...defaultProps} />);
461+
renderForm(<SearchForm key="fresh" {...defaultProps} />);
457462

458463
expect(SearchableSelect.disabled.operation).toBe(true);
459464
cleanup();
460465

461-
render(<SearchForm key="with-svc" {...defaultProps} initialValues={{ service: 'svcA' }} />);
466+
renderForm(<SearchForm key="with-svc" {...defaultProps} initialValues={{ service: 'svcA' }} />);
462467

463468
await waitFor(() => expect(SearchableSelect.disabled.operation).toBe(false));
464469
});
465470

466471
it('keeps operation disabled when no service selected', () => {
467-
render(<SearchForm {...defaultProps} />);
472+
renderForm(<SearchForm {...defaultProps} />);
468473

469474
expect(SearchableSelect.disabled.operation).toBe(true);
470475
});
@@ -481,7 +486,7 @@ describe('<SearchForm>', () => {
481486
},
482487
};
483488

484-
const { container } = render(<SearchForm key="custom-date" {...props} />);
489+
const { container } = renderForm(<SearchForm key="custom-date" {...props} />);
485490

486491
const startDateInput = container.querySelector('input[name="startDate"]');
487492
const endDateInput = container.querySelector('input[name="endDate"]');
@@ -491,7 +496,7 @@ describe('<SearchForm>', () => {
491496
});
492497

493498
it('disables the submit button when a service is not selected', () => {
494-
const { container } = render(
499+
const { container } = renderForm(
495500
<SearchForm key="disabled-no-svc" {...defaultProps} initialValues={{ service: '-' }} />
496501
);
497502

@@ -500,7 +505,7 @@ describe('<SearchForm>', () => {
500505
});
501506

502507
it('disables the submit button when the form has invalid data', () => {
503-
const { container } = render(
508+
const { container } = renderForm(
504509
<SearchForm
505510
key="disabled-invalid"
506511
{...defaultProps}
@@ -514,7 +519,7 @@ describe('<SearchForm>', () => {
514519
});
515520

516521
it('disables the submit button when duration is invalid', () => {
517-
const { container } = render(
522+
const { container } = renderForm(
518523
<SearchForm key="duration-val" {...defaultProps} initialValues={{ service: 'svcA' }} />
519524
);
520525

@@ -538,7 +543,7 @@ describe('<SearchForm>', () => {
538543
const originalGetConfig = window.getJaegerUiConfig;
539544
window.getJaegerUiConfig = jest.fn(() => config);
540545

541-
const { container } = render(<SearchForm {...defaultProps} />);
546+
const { container } = renderForm(<SearchForm {...defaultProps} />);
542547

543548
const limitInput = container.querySelector('input[name="resultsLimit"]');
544549
expect(limitInput).toHaveAttribute('max');
@@ -547,7 +552,7 @@ describe('<SearchForm>', () => {
547552
});
548553

549554
it('updates state when tags input changes', () => {
550-
const { container } = render(<SearchForm {...defaultProps} />);
555+
const { container } = renderForm(<SearchForm {...defaultProps} />);
551556

552557
const tagsInput = container.querySelector('input[name="tags"]');
553558
fireEvent.change(tagsInput, { target: { value: 'new=tag' } });
@@ -556,7 +561,7 @@ describe('<SearchForm>', () => {
556561
});
557562

558563
it('prevents default form submission behavior', async () => {
559-
const { container } = render(
564+
const { container } = renderForm(
560565
<SearchForm {...defaultProps} searchAdjustEndTime="1m" initialValues={{ service: 'svcA' }} />
561566
);
562567
const form = container.querySelector('form');
@@ -590,7 +595,7 @@ describe('<SearchForm>', () => {
590595
error: new Error('Failed to fetch services'),
591596
});
592597

593-
const { container } = render(<SearchForm {...defaultProps} />);
598+
const { container } = renderForm(<SearchForm {...defaultProps} />);
594599

595600
// Should still render the form
596601
expect(container.querySelector('form')).toBeInTheDocument();
@@ -612,7 +617,7 @@ describe('<SearchForm>', () => {
612617
error: new Error('Failed to fetch span names'),
613618
});
614619

615-
const { container } = render(<SearchForm {...defaultProps} initialValues={{ service: 'svcA' }} />);
620+
const { container } = renderForm(<SearchForm {...defaultProps} initialValues={{ service: 'svcA' }} />);
616621

617622
// Should still render the form
618623
expect(container.querySelector('form')).toBeInTheDocument();
@@ -630,7 +635,7 @@ describe('<SearchForm>', () => {
630635
error: new Error('Service error'),
631636
});
632637

633-
const { container } = render(<SearchForm {...defaultProps} />);
638+
const { container } = renderForm(<SearchForm {...defaultProps} />);
634639

635640
// Form should still be present
636641
const form = container.querySelector('form');
@@ -654,7 +659,7 @@ describe('<SearchForm>', () => {
654659
error: new Error('Span names error'),
655660
});
656661

657-
const { container } = render(<SearchForm {...defaultProps} initialValues={{ service: 'svcA' }} />);
662+
const { container } = renderForm(<SearchForm {...defaultProps} initialValues={{ service: 'svcA' }} />);
658663

659664
// Form should still be present
660665
const form = container.querySelector('form');
@@ -690,7 +695,7 @@ describe('SearchForm onChange handlers', () => {
690695
},
691696
};
692697

693-
const { getByTestId, container } = render(<SearchForm key="on-change" {...props} />);
698+
const { getByTestId, container } = renderForm(<SearchForm key="on-change" {...props} />);
694699

695700
// Service
696701
await act(async () => {

‎packages/jaeger-ui/src/components/SearchTracePage/SearchForm.tsx‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@ import memoizeOne from 'memoize-one';
1111
import queryString from 'query-string';
1212
import { IoHelp } from 'react-icons/io5';
1313
import { connect, ConnectedProps } from 'react-redux';
14-
import { useLocation } from 'react-router-dom-v5-compat';
14+
import { useLocation, useNavigate } from 'react-router-dom-v5-compat';
15+
import { getUrl as getSearchUrl } from './url';
1516
import { bindActionCreators, Dispatch } from 'redux';
1617
import store from 'store';
1718

@@ -271,7 +272,7 @@ export function submitForm(
271272
searchTraces: SearchTracesFunction,
272273
adjustTime: string | null | undefined,
273274
adjustTimeEnabled: boolean
274-
): void {
275+
): string {
275276
const {
276277
resultsLimit,
277278
service,
@@ -312,7 +313,7 @@ export function submitForm(
312313

313314
trackFormInput(resultsLimit, operation, tags || '', minDuration, maxDuration, lookback, service);
314315

315-
searchTraces({
316+
const query: SearchQuery = {
316317
service,
317318
operation: operation !== DEFAULT_OPERATION ? operation : undefined,
318319
limit: resultsLimit,
@@ -322,7 +323,9 @@ export function submitForm(
322323
tags: convTagsLogfmt(tags) || undefined,
323324
minDuration: minDuration || null,
324325
maxDuration: maxDuration || null,
325-
} as SearchQuery);
326+
};
327+
searchTraces(query);
328+
return getSearchUrl(query as Parameters<typeof getSearchUrl>[0]);
326329
}
327330

328331
interface ISearchFormImplProps {
@@ -336,7 +339,7 @@ interface ISearchFormImplProps {
336339
fields: ISearchFormFields,
337340
adjustEndTime: string | null | undefined,
338341
adjustTimeEnabled: boolean
339-
) => void;
342+
) => string;
340343
}
341344

342345
export const SearchFormImpl: React.FC<ISearchFormImplProps> = ({
@@ -347,6 +350,7 @@ export const SearchFormImpl: React.FC<ISearchFormImplProps> = ({
347350
initialValues,
348351
submitFormHandler,
349352
}) => {
353+
const navigate = useNavigate();
350354
const { useOpenTelemetryTerms: useOtelTerms } = useConfig();
351355
const [formData, setFormData] = useState<Partial<ISearchFormFields>>(() => ({
352356
service: initialValues?.service,
@@ -404,9 +408,10 @@ export const SearchFormImpl: React.FC<ISearchFormImplProps> = ({
404408
const handleSubmit = useCallback(
405409
(e: React.FormEvent<HTMLFormElement>) => {
406410
e.preventDefault();
407-
submitFormHandler(formData as ISearchFormFields, searchAdjustEndTime, adjustTimeEnabled);
411+
const url = submitFormHandler(formData as ISearchFormFields, searchAdjustEndTime, adjustTimeEnabled);
412+
navigate(url);
408413
},
409-
[formData, searchAdjustEndTime, adjustTimeEnabled, submitFormHandler]
414+
[formData, searchAdjustEndTime, adjustTimeEnabled, submitFormHandler, navigate]
410415
);
411416

412417
const { service: selectedService, lookback: selectedLookback } = formData;
@@ -857,7 +862,7 @@ export function mapDispatchToProps(dispatch: Dispatch) {
857862
fields: ISearchFormFields,
858863
adjustEndTime: string | null | undefined,
859864
adjustTimeEnabled: boolean
860-
) => submitForm(fields, searchTraces, adjustEndTime || null, adjustTimeEnabled),
865+
) => submitForm(fields, searchTraces, adjustEndTime || null, adjustTimeEnabled) as string,
861866
};
862867
}
863868

‎packages/jaeger-ui/src/middlewares/index.test.js‎

Lines changed: 0 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -11,63 +11,8 @@ jest.mock(
1111
})
1212
);
1313

14-
jest.mock('redux-first-history', () => ({
15-
replace: jest.fn(url => ({ type: 'REPLACE', payload: url })),
16-
}));
17-
18-
jest.mock('../components/SearchTracePage/url', () => ({
19-
getUrl: jest.fn(query => `/search?${JSON.stringify(query)}`),
20-
}));
21-
2214
import * as jaegerMiddlewares from './index';
23-
import { searchTraces } from '../actions/jaeger-api';
24-
import { replace } from 'redux-first-history';
25-
import { getUrl as getSearchUrl } from '../components/SearchTracePage/url';
2615

2716
it('jaegerMiddlewares should contain the promise middleware', () => {
2817
expect(typeof jaegerMiddlewares.promise).toBe('function');
2918
});
30-
31-
describe('historyUpdateMiddleware', () => {
32-
const mockStore = {
33-
dispatch: jest.fn(),
34-
};
35-
const mockNext = jest.fn(action => action);
36-
37-
beforeEach(() => {
38-
mockStore.dispatch.mockClear();
39-
mockNext.mockClear();
40-
replace.mockClear();
41-
getSearchUrl.mockClear();
42-
});
43-
44-
it('dispatches replace action when action type matches searchTraces', () => {
45-
const query = { service: 'test-service', operation: 'test-op' };
46-
const action = {
47-
type: String(searchTraces),
48-
meta: { query },
49-
};
50-
51-
jaegerMiddlewares.historyUpdateMiddleware(mockStore)(mockNext)(action);
52-
53-
expect(getSearchUrl).toHaveBeenCalledWith(query);
54-
expect(replace).toHaveBeenCalled();
55-
expect(mockStore.dispatch).toHaveBeenCalled();
56-
expect(mockNext).toHaveBeenCalledWith(action);
57-
});
58-
59-
it('does not dispatch replace action for non-searchTraces actions', () => {
60-
const action = {
61-
type: 'SOME_OTHER_ACTION',
62-
payload: { data: 'test' },
63-
};
64-
65-
const result = jaegerMiddlewares.historyUpdateMiddleware(mockStore)(mockNext)(action);
66-
67-
expect(getSearchUrl).not.toHaveBeenCalled();
68-
expect(replace).not.toHaveBeenCalled();
69-
expect(mockStore.dispatch).not.toHaveBeenCalled();
70-
expect(mockNext).toHaveBeenCalledWith(action);
71-
expect(result).toBe(action);
72-
});
73-
});

‎packages/jaeger-ui/src/middlewares/index.ts‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,6 @@
22
// SPDX-License-Identifier: Apache-2.0
33

44
import promiseMiddleware from 'redux-promise-middleware';
5-
import { replace } from 'redux-first-history';
6-
import { Middleware } from 'redux';
7-
8-
import { searchTraces } from '../actions/jaeger-api';
9-
import { getUrl as getSearchUrl } from '../components/SearchTracePage/url';
10-
import { ReduxState } from '../types';
115

126
export { default as trackMiddleware } from './track';
13-
14-
export const historyUpdateMiddleware: Middleware<{}, ReduxState> = store => next => (action: any) => {
15-
if (action.type === String(searchTraces)) {
16-
const url = getSearchUrl(action.meta.query);
17-
store.dispatch(replace(url));
18-
}
19-
return next(action);
20-
};
21-
227
export const promise = promiseMiddleware;

0 commit comments

Comments
 (0)