Skip to content

Commit ff31ab4

Browse files
Part of Phase 1 (1e): migrate embed flags off Redux (#3761)
## Which problem is this PR solving? - Resolves part of #3657 ## Description of the changes - Implements ADR 0004 Phase 1e (embedded UI flags) - Problem: embedded was a Redux slice with no actions, only an initializer reading `window.location.search` once. - Added `packages/jaeger-ui/src/stores/embedded-store.ts` - Removed: `reducers/embedded.ts`, its test, and embedded from `combineReducers` and `ReduxState`. - Tests: `embedded-store.test.ts`; `Page.test.jsx`, `SearchTracePage/index.test.jsx`, `TracePage/index.test.jsx` updated. ## How was this change tested? - Unit tests - Use a single `?` then `&uiEmbed=v0&...` (a second ? breaks parsing), e.g. `/search?...&uiEmbed=v0&uiSearchHideGraph=1`. - load a DDG graph, hover nodes/edges, change service ## 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 - [ ] **Light**: AI provided minor assistance (formatting, simple suggestions) - [x] **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> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <github@ysh.us>
1 parent 0d9451a commit ff31ab4

15 files changed

Lines changed: 191 additions & 152 deletions

File tree

packages/jaeger-ui/src/components/App/Page.test.jsx

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,51 +4,57 @@
44
vi.mock('./TopNav', () => mockDefault(() => <div />));
55
vi.mock('../../utils/tracking');
66

7+
const { useEmbeddedStateMock } = vi.hoisted(() => ({
8+
useEmbeddedStateMock: jest.fn().mockReturnValue(null),
9+
}));
10+
11+
vi.mock('../../stores/embedded-store', () => ({
12+
useEmbeddedState: (...args) => useEmbeddedStateMock(...args),
13+
}));
14+
715
import React from 'react';
816
import { render, screen } from '@testing-library/react';
917
import '@testing-library/jest-dom';
1018
import { MemoryRouter } from 'react-router-dom';
1119

12-
import { mapStateToProps, PageImpl as Page } from './Page';
20+
import { PageImpl as Page } from './Page';
1321
import { trackPageView } from '../../utils/tracking';
1422

15-
const renderWithPath = (props, path = '/test?search=value') =>
23+
const renderWithPath = (path = '/test?search=value') =>
1624
render(
1725
<MemoryRouter initialEntries={[path]}>
18-
<Page {...props} />
26+
<Page />
1927
</MemoryRouter>
2028
);
2129

22-
describe('mapStateToProps()', () => {
23-
it('maps embedded state to props', () => {
24-
const state = { embedded: true };
25-
expect(mapStateToProps(state)).toEqual({ embedded: true });
26-
});
27-
28-
it('does not include pathname or search (now from useLocation)', () => {
29-
const result = mapStateToProps({ embedded: false });
30-
expect(result).not.toHaveProperty('pathname');
31-
expect(result).not.toHaveProperty('search');
32-
});
33-
});
30+
const embeddedV0 = {
31+
version: 'v0',
32+
searchHideGraph: false,
33+
timeline: {
34+
collapseTitle: false,
35+
hideMinimap: false,
36+
hideSummary: false,
37+
},
38+
};
3439

3540
describe('<Page>', () => {
3641
beforeEach(() => {
3742
trackPageView.mockReset();
43+
useEmbeddedStateMock.mockReturnValue(null);
3844
});
3945

4046
it('renders without exploding', () => {
41-
renderWithPath({});
47+
renderWithPath();
4248
expect(screen.getByRole('banner')).toBeInTheDocument();
4349
});
4450

4551
it('tracks an initial page-view using location from useLocation()', () => {
46-
renderWithPath({}, '/my-path?q=1');
52+
renderWithPath('/my-path?q=1');
4753
expect(trackPageView).toHaveBeenCalledWith('/my-path', '?q=1');
4854
});
4955

5056
it('tracks a pageView when the location changes', () => {
51-
const { rerender } = renderWithPath({}, '/first?a=1');
57+
const { rerender } = renderWithPath('/first?a=1');
5258
trackPageView.mockReset();
5359
// Use a different key to force the MemoryRouter to remount with the new initialEntries.
5460
rerender(
@@ -60,7 +66,7 @@ describe('<Page>', () => {
6066
});
6167

6268
it('tracks a pageView when the search changes but pathname is same', () => {
63-
const { rerender } = renderWithPath({}, '/same-path?a=1');
69+
const { rerender } = renderWithPath('/same-path?a=1');
6470
trackPageView.mockReset();
6571
rerender(
6672
<MemoryRouter key="router-2" initialEntries={['/same-path?a=2']}>
@@ -72,8 +78,9 @@ describe('<Page>', () => {
7278

7379
describe('Page embedded', () => {
7480
beforeEach(() => {
81+
useEmbeddedStateMock.mockReturnValue(embeddedV0);
7582
trackPageView.mockReset();
76-
renderWithPath({ embedded: true });
83+
renderWithPath();
7784
});
7885

7986
it('renders without exploding', () => {

packages/jaeger-ui/src/components/App/Page.tsx

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,27 +4,25 @@
44
import * as React from 'react';
55
import { Layout } from 'antd';
66
import cx from 'classnames';
7-
import { connect } from 'react-redux';
87
import { useLocation } from 'react-router-dom';
98

109
import TopNav from './TopNav';
11-
import { ReduxState } from '../../types';
12-
import { EmbeddedState } from '../../types/embedded';
10+
import { useEmbeddedState } from '../../stores/embedded-store';
1311
import { trackPageView } from '../../utils/tracking';
1412
import DocumentTitle from '../../utils/documentTitle';
1513

1614
import './Page.css';
17-
import withRouteProps from '../../utils/withRouteProps';
1815

1916
type TProps = {
2017
children: React.ReactNode;
21-
embedded: EmbeddedState;
2218
};
2319

2420
const { Header, Content } = Layout;
2521

2622
// export for tests
27-
export const PageImpl: React.FC<TProps> = ({ children, embedded }) => {
23+
export const PageImpl: React.FC<TProps> = props => {
24+
const embedded = useEmbeddedState();
25+
const { children } = props;
2826
const { pathname, search } = useLocation();
2927
React.useEffect(() => {
3028
trackPageView(pathname, search);
@@ -46,11 +44,3 @@ export const PageImpl: React.FC<TProps> = ({ children, embedded }) => {
4644
</div>
4745
);
4846
};
49-
50-
// export for tests
51-
export function mapStateToProps(state: ReduxState) {
52-
const { embedded } = state;
53-
return { embedded };
54-
}
55-
56-
export default connect(mapStateToProps)(withRouteProps(PageImpl));

packages/jaeger-ui/src/components/App/index.dev.test.jsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ import '@testing-library/jest-dom';
1212
import { MemoryRouter } from 'react-router-dom';
1313

1414
vi.mock('./NotFound', () => mockDefault(() => <div data-testid="not-found" />));
15-
vi.mock('./Page', () => mockDefault(({ children }) => <div data-testid="page">{children}</div>));
15+
vi.mock('./Page', () => {
16+
const PageImpl = ({ children }) => <div data-testid="page">{children}</div>;
17+
return { PageImpl };
18+
});
1619
vi.mock('../DependencyGraph', () => mockDefault(() => <div data-testid="dependency-graph" />));
1720
vi.mock('../DeepDependencies', () => mockDefault(() => <div data-testid="deep-dependencies" />));
1821
vi.mock('../QualityMetrics', () => mockDefault(() => <div data-testid="quality-metrics" />));

packages/jaeger-ui/src/components/App/index.test.jsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@ import '@testing-library/jest-dom';
77
import { MemoryRouter } from 'react-router-dom';
88

99
vi.mock('./NotFound', () => mockDefault(() => <div data-testid="not-found" />));
10-
vi.mock('./Page', () => mockDefault(({ children }) => <div data-testid="page">{children}</div>));
10+
vi.mock('./Page', () => {
11+
const PageImpl = ({ children }) => <div data-testid="page">{children}</div>;
12+
return { PageImpl };
13+
});
1114
vi.mock('../DependencyGraph', () => mockDefault(() => <div data-testid="dependency-graph" />));
1215
vi.mock('../DeepDependencies', () => mockDefault(() => <div data-testid="deep-dependencies" />));
1316
vi.mock('../QualityMetrics', () => mockDefault(() => <div data-testid="quality-metrics" />));

packages/jaeger-ui/src/components/App/index.tsx

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { Route, Routes, Navigate } from 'react-router-dom';
77
import { AppQueryClientProvider } from '../../query/app-query-client';
88

99
import NotFound from './NotFound';
10-
import Page from './Page';
10+
import { PageImpl as Page } from './Page';
1111
import DependencyGraph from '../DependencyGraph';
1212
import { ROUTE_PATH as dependenciesPath } from '../DependencyGraph/url';
1313
import DeepDependencies from '../DeepDependencies';
@@ -47,13 +47,6 @@ export default function JaegerUIApp() {
4747
<AppQueryClientProvider>
4848
<ThemeProvider>
4949
<Provider store={store as any}>
50-
{
51-
// the Page component is a connected component (wrapped by Redux's connect HOC)
52-
// that is also wrapped by a custom withRouteProps HOC.
53-
// The @ts-ignore was added because of a specific TypeScript error that occurs
54-
// when mixing Redux 5/9, React 19, and complex HOCs.
55-
}
56-
{/* @ts-expect-error - TypeScript error with Redux 5/9, React 19, and complex HOCs */}
5750
<Page>
5851
<Routes>
5952
<Route path={searchPath} element={<SearchTracePage />} />

packages/jaeger-ui/src/components/SearchTracePage/index.test.jsx

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,14 @@
44
import { MemoryRouter } from 'react-router-dom';
55
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
66

7+
const { useEmbeddedStateMock } = vi.hoisted(() => ({
8+
useEmbeddedStateMock: jest.fn().mockReturnValue(null),
9+
}));
10+
11+
vi.mock('../../stores/embedded-store', () => ({
12+
useEmbeddedState: (...args) => useEmbeddedStateMock(...args),
13+
}));
14+
715
vi.mock('../../api/v3/client', () => ({
816
jaegerClient: {
917
fetchServices: jest.fn(() => Promise.resolve([])),
@@ -82,6 +90,7 @@ describe('<SearchTracePage>', () => {
8290
props = getDefaultProps();
8391
traces = props.traces;
8492
traceResultsToDownload = props.traceResultsToDownload;
93+
useEmbeddedStateMock.mockReturnValue(null);
8594
});
8695

8796
it('searches for traces if `service` or `traceID` are in the query string', () => {
@@ -200,20 +209,28 @@ describe('<SearchTracePage>', () => {
200209
});
201210

202211
it('hides SearchForm if is embed', () => {
203-
const testProps = { ...props, embedded: true };
212+
useEmbeddedStateMock.mockReturnValue({
213+
version: 'v0',
214+
searchHideGraph: false,
215+
timeline: { collapseTitle: false, hideMinimap: false, hideSummary: false },
216+
});
204217
const { container } = render(
205218
<AllProvider>
206-
<SearchTracePage {...testProps} />
219+
<SearchTracePage {...props} />
207220
</AllProvider>
208221
);
209222
expect(container.querySelector('[data-node-key="searchForm"]')).not.toBeInTheDocument();
210223
});
211224

212225
it('hides logo if is embed', () => {
213-
const testProps = { ...props, embedded: true };
226+
useEmbeddedStateMock.mockReturnValue({
227+
version: 'v0',
228+
searchHideGraph: false,
229+
timeline: { collapseTitle: false, hideMinimap: false, hideSummary: false },
230+
});
214231
const { container } = render(
215232
<AllProvider>
216-
<SearchTracePage {...testProps} />
233+
<SearchTracePage {...props} />
217234
</AllProvider>
218235
);
219236
expect(container.querySelector('.js-test-logo')).not.toBeInTheDocument();
@@ -301,7 +318,6 @@ describe('mapStateToProps()', () => {
301318
expect(diffCohort[0].data.traceID).toBe(trace.traceID);
302319

303320
expect(rest).toEqual({
304-
embedded: undefined,
305321
queryOfResults: undefined,
306322
isHomepage: true,
307323
sortedTracesXformer: expect.any(Function),

packages/jaeger-ui/src/components/SearchTracePage/index.tsx

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import JaegerLogo from '../../img/jaeger-logo.svg';
2626
import withRouteProps from '../../utils/withRouteProps';
2727
import { trackSortByChange } from './SearchForm.track';
2828
import { useTraceDiffStore } from '../../stores/trace-diff-store';
29+
import { useEmbeddedState } from '../../stores/embedded-store';
2930
import { useShallow } from 'zustand/react/shallow';
3031
import { ReduxState } from '../../types';
3132
import { SearchQuery } from '../../types/search';
@@ -38,10 +39,6 @@ interface IQueryOfResults extends Partial<SearchQuery> {
3839
limit?: string | number;
3940
}
4041

41-
interface IEmbeddedConfig {
42-
searchHideGraph?: boolean;
43-
}
44-
4542
interface ISearchTracePageImplOwnProps {
4643
isHomepage?: boolean;
4744
}
@@ -51,7 +48,6 @@ interface IStateProps {
5148
queryOfResults: IQueryOfResults | null;
5249
// passed as-is from Redux; cohort lookup happens in the component where Zustand is accessible
5350
tracesInRedux: ReduxState['trace'];
54-
embedded?: IEmbeddedConfig;
5551
loadingTraces: boolean;
5652
traces: Trace[];
5753
traceResultsToDownload: unknown[];
@@ -72,9 +68,9 @@ type SearchTracePageImplProps = ISearchTracePageImplOwnProps & IStateProps & IDi
7268

7369
// export for tests
7470
export function SearchTracePageImpl(props: SearchTracePageImplProps) {
71+
const embedded = useEmbeddedState();
7572
const {
7673
tracesInRedux,
77-
embedded,
7874
errors,
7975
fetchMultipleTraces,
8076
isHomepage,
@@ -185,7 +181,7 @@ export function SearchTracePageImpl(props: SearchTracePageImplProps) {
185181
cohortRemoveTrace,
186182
diffCohort,
187183
disableComparisons: !!embedded,
188-
hideGraph: embedded && embedded.searchHideGraph,
184+
hideGraph: Boolean(embedded?.searchHideGraph),
189185
loading: loadingTraces,
190186
maxTraceDuration,
191187
queryOfResults,
@@ -247,7 +243,6 @@ export function mapStateToProps(
247243
state: ReduxState,
248244
ownProps: { search?: string }
249245
): IStateProps & { isHomepage: boolean } {
250-
const { embedded } = state;
251246
const query = getUrlState(ownProps.search || '');
252247
const isHomepage = !Object.keys(query).length;
253248
const {
@@ -267,7 +262,6 @@ export function mapStateToProps(
267262
return {
268263
queryOfResults: queryOfResults as IQueryOfResults | null,
269264
tracesInRedux: state.trace,
270-
embedded,
271265
isHomepage,
272266
loadingTraces,
273267
traces,

0 commit comments

Comments
 (0)