Skip to content

Commit 3a0eac7

Browse files
adhityamamallanadhityamamallan
authored
Refactor Workflow Diagnostics data flow and components (#956)
- Fetch workflow diagnostics at the top, in workflow-diagnostics.tsx, by using react-query's enabled feature to disable the query unless diagnostics is enabled - Create a diverging branch in workflow-diagnostics.tsx where the valid diagnostics and invalid diagnostics cases are handled completely separately, making it easier to add future changes to the valid diagnostics case. - Isolate the view mode toggle into a separate, resuable component
1 parent 02b92ca commit 3a0eac7

23 files changed

+508
-268
lines changed

‎src/views/workflow-diagnostics/__tests__/workflow-diagnostics.test.tsx

Lines changed: 91 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { HttpResponse } from 'msw';
55
import { render, screen } from '@/test-utils/rtl';
66

77
import ErrorBoundary from '@/components/error-boundary/error-boundary';
8+
import { mockWorkflowDiagnosticsResult } from '@/route-handlers/diagnose-workflow/__fixtures__/mock-workflow-diagnostics-result';
89

910
import WorkflowDiagnostics from '../workflow-diagnostics';
1011

@@ -16,17 +17,35 @@ jest.mock('@/components/panel-section/panel-section', () =>
1617
jest.fn(({ children }) => <div data-testid="panel-section">{children}</div>)
1718
);
1819

20+
jest.mock(
21+
'@/components/section-loading-indicator/section-loading-indicator',
22+
() => jest.fn(() => <div data-testid="loading-indicator">Loading...</div>)
23+
);
24+
1925
jest.mock('../workflow-diagnostics-content/workflow-diagnostics-content', () =>
20-
jest.fn(({ domain, cluster, workflowId, runId }) => (
26+
jest.fn(({ domain, cluster, workflowId, runId, diagnosticsResult }) => (
2127
<div data-testid="workflow-diagnostics-content">
2228
<div>Domain: {domain}</div>
2329
<div>Cluster: {cluster}</div>
2430
<div>Workflow ID: {workflowId}</div>
2531
<div>Run ID: {runId}</div>
32+
<div>Diagnostics Result: {JSON.stringify(diagnosticsResult)}</div>
2633
</div>
2734
))
2835
);
2936

37+
jest.mock(
38+
'../workflow-diagnostics-fallback/workflow-diagnostics-fallback',
39+
() =>
40+
jest.fn(({ workflowId, runId, diagnosticsResult }) => (
41+
<div data-testid="workflow-diagnostics-fallback">
42+
<div>Workflow ID: {workflowId}</div>
43+
<div>Run ID: {runId}</div>
44+
<div>Diagnostics Result: {JSON.stringify(diagnosticsResult)}</div>
45+
</div>
46+
))
47+
);
48+
3049
jest.mock('../config/workflow-diagnostics-disabled-error-panel.config', () => ({
3150
message: 'Workflow Diagnostics is currently disabled',
3251
}));
@@ -36,8 +55,14 @@ describe(WorkflowDiagnostics.name, () => {
3655
jest.clearAllMocks();
3756
});
3857

39-
it('should render workflow diagnostics content when diagnostics is enabled', async () => {
40-
await setup({ isDiagnosticsEnabled: true });
58+
it('should render workflow diagnostics content when diagnostics is enabled and successful', async () => {
59+
await setup({
60+
isDiagnosticsEnabled: true,
61+
diagnosticsResponse: {
62+
result: mockWorkflowDiagnosticsResult,
63+
parsingError: null,
64+
},
65+
});
4166

4267
await screen.findByTestId('workflow-diagnostics-content');
4368

@@ -52,6 +77,46 @@ describe(WorkflowDiagnostics.name, () => {
5277
expect(screen.getByText('Run ID: test-run-id')).toBeInTheDocument();
5378
});
5479

80+
it('should render workflow diagnostics fallback when parsing error exists', async () => {
81+
await setup({
82+
isDiagnosticsEnabled: true,
83+
diagnosticsResponse: {
84+
result: { raw: 'invalid data' },
85+
parsingError: new Error('Parsing failed'),
86+
},
87+
});
88+
89+
await screen.findByTestId('workflow-diagnostics-fallback');
90+
91+
expect(
92+
screen.getByTestId('workflow-diagnostics-fallback')
93+
).toBeInTheDocument();
94+
expect(
95+
screen.getByText('Workflow ID: test-workflow-id')
96+
).toBeInTheDocument();
97+
expect(screen.getByText('Run ID: test-run-id')).toBeInTheDocument();
98+
});
99+
100+
it('should render loading indicator when diagnostics is enabled but data is pending', async () => {
101+
await setup({
102+
isDiagnosticsEnabled: true,
103+
diagnosticsResponse: 'pending',
104+
});
105+
106+
expect(await screen.findByTestId('loading-indicator')).toBeInTheDocument();
107+
});
108+
109+
it('should throw error when diagnostics is enabled but data fetch fails', async () => {
110+
await setup({
111+
isDiagnosticsEnabled: true,
112+
diagnosticsResponse: 'error',
113+
});
114+
115+
expect(
116+
await screen.findByText('Error: Failed to fetch diagnostics')
117+
).toBeInTheDocument();
118+
});
119+
55120
it('should render error panel when diagnostics is disabled', async () => {
56121
await setup({ isDiagnosticsEnabled: false });
57122

@@ -75,9 +140,11 @@ describe(WorkflowDiagnostics.name, () => {
75140
async function setup({
76141
isDiagnosticsEnabled,
77142
error,
143+
diagnosticsResponse,
78144
}: {
79145
isDiagnosticsEnabled: boolean;
80146
error?: boolean;
147+
diagnosticsResponse?: any;
81148
}) {
82149
render(
83150
<ErrorBoundary
@@ -112,6 +179,27 @@ async function setup({
112179
}
113180
},
114181
},
182+
...(isDiagnosticsEnabled
183+
? [
184+
{
185+
path: '/api/domains/:domain/:cluster/workflows/:workflowId/:runId/diagnose',
186+
httpMethod: 'GET' as const,
187+
mockOnce: false,
188+
httpResolver: async () => {
189+
if (diagnosticsResponse === 'error') {
190+
return HttpResponse.json(
191+
{ message: 'Failed to fetch diagnostics' },
192+
{ status: 500 }
193+
);
194+
} else if (diagnosticsResponse === 'pending') {
195+
return new Promise<never>(() => {}); // Never resolves to simulate pending
196+
} else {
197+
return HttpResponse.json(diagnosticsResponse);
198+
}
199+
},
200+
},
201+
]
202+
: []),
115203
],
116204
}
117205
);

‎src/views/workflow-diagnostics/hooks/use-diagnose-workflow/get-diagnose-workflow-query-options.ts

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,15 @@
1-
import { type UseQueryOptions } from '@tanstack/react-query';
21
import queryString from 'query-string';
32

4-
import { type DiagnoseWorkflowResponse } from '@/route-handlers/diagnose-workflow/diagnose-workflow.types';
53
import request from '@/utils/request';
6-
import { type RequestError } from '@/utils/request/request-error';
74

8-
import { type UseDiagnoseWorkflowParams } from './use-diagnose-workflow.types';
5+
import {
6+
type DiagnoseWorkflowQueryOptions,
7+
type UseDiagnoseWorkflowParams,
8+
} from './use-diagnose-workflow.types';
99

1010
export default function getDiagnoseWorkflowQueryOptions(
1111
params: UseDiagnoseWorkflowParams
12-
): UseQueryOptions<
13-
DiagnoseWorkflowResponse,
14-
RequestError,
15-
DiagnoseWorkflowResponse,
16-
[string, UseDiagnoseWorkflowParams]
17-
> {
12+
): DiagnoseWorkflowQueryOptions {
1813
return {
1914
queryKey: ['diagnoseWorkflow', params],
2015
queryFn: async ({

‎src/views/workflow-diagnostics/hooks/use-diagnose-workflow/use-diagnose-workflow.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,17 @@
11
import { useQuery } from '@tanstack/react-query';
22

33
import getDiagnoseWorkflowQueryOptions from './get-diagnose-workflow-query-options';
4-
import { type UseDiagnoseWorkflowParams } from './use-diagnose-workflow.types';
4+
import {
5+
type DiagnoseWorkflowQueryOptions,
6+
type UseDiagnoseWorkflowParams,
7+
} from './use-diagnose-workflow.types';
58

6-
export default function useDiagnoseWorkflow(params: UseDiagnoseWorkflowParams) {
7-
return useQuery(getDiagnoseWorkflowQueryOptions(params));
9+
export default function useDiagnoseWorkflow(
10+
params: UseDiagnoseWorkflowParams,
11+
additionalOptions?: Partial<DiagnoseWorkflowQueryOptions>
12+
) {
13+
return useQuery({
14+
...getDiagnoseWorkflowQueryOptions(params),
15+
...additionalOptions,
16+
});
817
}

‎src/views/workflow-diagnostics/hooks/use-diagnose-workflow/use-diagnose-workflow.types.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,18 @@
1+
import { type UseQueryOptions } from '@tanstack/react-query';
2+
3+
import { type DiagnoseWorkflowResponse } from '@/route-handlers/diagnose-workflow/diagnose-workflow.types';
4+
import { type RequestError } from '@/utils/request/request-error';
5+
16
export type UseDiagnoseWorkflowParams = {
27
domain: string;
38
cluster: string;
49
workflowId: string;
510
runId: string;
611
};
12+
13+
export type DiagnoseWorkflowQueryOptions = UseQueryOptions<
14+
DiagnoseWorkflowResponse,
15+
RequestError,
16+
DiagnoseWorkflowResponse,
17+
[string, UseDiagnoseWorkflowParams]
18+
>;

‎src/views/workflow-diagnostics/workflow-diagnostics-content/__tests__/workflow-diagnostics-content.test.tsx

Lines changed: 32 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,11 @@
1-
import { HttpResponse } from 'msw';
2-
import { ZodError } from 'zod';
3-
4-
import { render, screen, userEvent, within } from '@/test-utils/rtl';
1+
import { render, screen, userEvent } from '@/test-utils/rtl';
52

63
import ErrorBoundary from '@/components/error-boundary/error-boundary';
74
import { mockWorkflowDiagnosticsResult } from '@/route-handlers/diagnose-workflow/__fixtures__/mock-workflow-diagnostics-result';
8-
import { type DiagnoseWorkflowResponse } from '@/route-handlers/diagnose-workflow/diagnose-workflow.types';
5+
import { type WorkflowDiagnosticsResult } from '@/route-handlers/diagnose-workflow/diagnose-workflow.types';
96

107
import WorkflowDiagnosticsContent from '../workflow-diagnostics-content';
118

12-
jest.mock(
13-
'@/components/section-loading-indicator/section-loading-indicator',
14-
() => jest.fn(() => <div data-testid="loading-indicator">Loading...</div>)
15-
);
16-
179
jest.mock('../../workflow-diagnostics-list/workflow-diagnostics-list', () =>
1810
jest.fn(() => <div>Diagnostics List Component</div>)
1911
);
@@ -22,27 +14,26 @@ jest.mock('../../workflow-diagnostics-json/workflow-diagnostics-json', () =>
2214
jest.fn(() => <div>Diagnostics JSON Component</div>)
2315
);
2416

25-
describe('WorkflowDiagnosticsContent', () => {
17+
jest.mock(
18+
'../../workflow-diagnostics-view-toggle/workflow-diagnostics-view-toggle',
19+
() =>
20+
jest.fn(({ listEnabled, activeView, setActiveView }) => (
21+
<div data-testid="view-toggle">
22+
<div>Is list view enabled: {listEnabled.toString()}</div>
23+
<div>Active View: {activeView}</div>
24+
<button onClick={() => setActiveView('list')}>Switch to List</button>
25+
<button onClick={() => setActiveView('json')}>Switch to JSON</button>
26+
</div>
27+
))
28+
);
29+
30+
describe(WorkflowDiagnosticsContent.name, () => {
2631
beforeEach(() => {
2732
jest.clearAllMocks();
2833
});
2934

30-
it('should render loading indicator when status is pending', async () => {
31-
setup({ responseType: 'pending' });
32-
33-
expect(await screen.findByTestId('loading-indicator')).toBeInTheDocument();
34-
});
35-
36-
it('should throw error when status is error', async () => {
37-
setup({ responseType: 'error' });
38-
39-
expect(
40-
await screen.findByText('Failed to fetch diagnostics')
41-
).toBeInTheDocument();
42-
});
43-
4435
it('should render diagnostics list view by default', async () => {
45-
setup({ responseType: 'success' });
36+
setup({ diagnosticsResult: mockWorkflowDiagnosticsResult });
4637

4738
expect(
4839
await screen.findByText('Diagnostics List Component')
@@ -53,7 +44,9 @@ describe('WorkflowDiagnosticsContent', () => {
5344
});
5445

5546
it('should allow switching between list and JSON views', async () => {
56-
const { user } = setup({ responseType: 'success' });
47+
const { user } = setup({
48+
diagnosticsResult: mockWorkflowDiagnosticsResult,
49+
});
5750

5851
// Initially shows list view
5952
expect(
@@ -64,8 +57,7 @@ describe('WorkflowDiagnosticsContent', () => {
6457
).not.toBeInTheDocument();
6558

6659
// Switch to JSON view
67-
const listbox = screen.getByRole('listbox');
68-
const jsonButton = within(listbox).getByRole('option', { name: /json/i });
60+
const jsonButton = screen.getByText('Switch to JSON');
6961
await user.click(jsonButton);
7062

7163
expect(screen.getByText('Diagnostics JSON Component')).toBeInTheDocument();
@@ -74,7 +66,7 @@ describe('WorkflowDiagnosticsContent', () => {
7466
).not.toBeInTheDocument();
7567

7668
// Switch back to list view
77-
const listButton = within(listbox).getByRole('option', { name: /list/i });
69+
const listButton = screen.getByText('Switch to List');
7870
await user.click(listButton);
7971

8072
expect(screen.getByText('Diagnostics List Component')).toBeInTheDocument();
@@ -83,32 +75,20 @@ describe('WorkflowDiagnosticsContent', () => {
8375
).not.toBeInTheDocument();
8476
});
8577

86-
it('should switch to JSON view when parsing error exists', async () => {
87-
setup({ responseType: 'parsing-error' });
88-
89-
expect(
90-
await screen.findByText('Diagnostics JSON Component')
91-
).toBeInTheDocument();
92-
expect(
93-
screen.queryByText('Diagnostics List Component')
94-
).not.toBeInTheDocument();
95-
});
96-
97-
it('should disable list view when parsing error exists', async () => {
98-
const { debug } = setup({ responseType: 'parsing-error' });
78+
it('should render view toggle with correct props', () => {
79+
setup({ diagnosticsResult: mockWorkflowDiagnosticsResult });
9980

100-
const listbox = await screen.findByRole('listbox');
101-
const listButton = within(listbox).getByRole('option', { name: /list/i });
102-
debug();
103-
expect(listButton).toBeDisabled();
81+
expect(screen.getByTestId('view-toggle')).toBeInTheDocument();
82+
expect(screen.getByText('Is list view enabled: true')).toBeInTheDocument();
83+
expect(screen.getByText('Active View: list')).toBeInTheDocument();
10484
});
10585
});
10686

10787
function setup({
108-
responseType,
88+
diagnosticsResult,
10989
}: {
110-
responseType?: 'success' | 'error' | 'parsing-error' | 'pending';
111-
} = {}) {
90+
diagnosticsResult: WorkflowDiagnosticsResult;
91+
}) {
11292
const user = userEvent.setup();
11393

11494
const renderResult = render(
@@ -118,39 +98,9 @@ function setup({
11898
cluster="test-cluster"
11999
workflowId="test-workflow-id"
120100
runId="test-run-id"
101+
diagnosticsResult={diagnosticsResult}
121102
/>
122-
</ErrorBoundary>,
123-
{
124-
endpointsMocks: [
125-
{
126-
path: '/api/domains/:domain/:cluster/workflows/:workflowId/:runId/diagnose',
127-
httpMethod: 'GET',
128-
mockOnce: false,
129-
httpResolver: async () => {
130-
switch (responseType) {
131-
case 'error':
132-
return HttpResponse.json(
133-
{ message: 'Failed to fetch diagnostics' },
134-
{ status: 500 }
135-
);
136-
case 'parsing-error':
137-
return HttpResponse.json({
138-
result: { raw: 'invalid data' },
139-
parsingError: new ZodError([]),
140-
} satisfies DiagnoseWorkflowResponse);
141-
case 'pending':
142-
return new Promise(() => {}); // Never resolves to simulate pending
143-
case 'success':
144-
default:
145-
return HttpResponse.json({
146-
result: mockWorkflowDiagnosticsResult,
147-
parsingError: null,
148-
} satisfies DiagnoseWorkflowResponse);
149-
}
150-
},
151-
},
152-
],
153-
}
103+
</ErrorBoundary>
154104
);
155105

156106
return { user, ...renderResult };

0 commit comments

Comments
 (0)