Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 44 additions & 42 deletions static/app/components/pipeline/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,49 +61,51 @@ function PipelineModal<
</Header>
<Body>
<Stack gap="2xl">
<Grid columns="1fr max-content">
<Flex gap="md" align="center">
<ProgressRing
maxValue={pipeline.totalSteps}
value={pipeline.stepIndex + 1}
text={pipeline.stepIndex + 1}
animate
/>
<Grid>
<AnimatePresence>
<motion.div
key={stepDefinition?.stepId}
initial={pipeline.stepIndex === 0 ? {} : {y: -15, opacity: 0}}
animate={{y: 0, opacity: 1}}
exit={{y: 15, opacity: 0}}
transition={{duration: 0.2}}
style={{gridColumn: 1, gridRow: 1}}
>
{stepText}
</motion.div>
</AnimatePresence>
</Grid>
</Flex>
<Flex gap="md" align="center">
{loading && (
<LoadingIndicator
mini
size={20}
style={{margin: 0, height: 20, width: 20}}
{!pipeline.isComplete && (
<Grid columns="1fr max-content">
<Flex gap="md" align="center">
<ProgressRing
maxValue={pipeline.totalSteps}
value={pipeline.stepIndex + 1}
text={pipeline.stepIndex + 1}
animate
/>
)}
{pipeline.stepIndex !== 0 && (
<Button
size="zero"
priority="transparent"
onClick={pipeline.restart}
icon={<IconRefresh size="xs" variant="muted" />}
tooltipProps={{title: t('Restart flow')}}
aria-label={t('Restart flow')}
/>
)}
</Flex>
</Grid>
<Grid>
<AnimatePresence>
<motion.div
key={stepDefinition?.stepId}
initial={pipeline.stepIndex === 0 ? {} : {y: -15, opacity: 0}}
animate={{y: 0, opacity: 1}}
exit={{y: 15, opacity: 0}}
transition={{duration: 0.2}}
style={{gridColumn: 1, gridRow: 1}}
>
{stepText}
</motion.div>
</AnimatePresence>
</Grid>
</Flex>
<Flex gap="md" align="center">
{loading && (
<LoadingIndicator
mini
size={20}
style={{margin: 0, height: 20, width: 20}}
/>
)}
{pipeline.stepIndex !== 0 && (
<Button
size="zero"
priority="transparent"
onClick={pipeline.restart}
icon={<IconRefresh size="xs" variant="muted" />}
tooltipProps={{title: t('Restart flow')}}
aria-label={t('Restart flow')}
/>
)}
</Flex>
</Grid>
)}

{pipeline.error && (
<Alert
Expand Down
21 changes: 20 additions & 1 deletion static/app/components/pipeline/pipelineDummyProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import {Text} from '@sentry/scraps/text';

import {t} from 'sentry/locale';

import type {PipelineDefinition, PipelineStepProps} from './types';
import type {
PipelineCompletionProps,
PipelineDefinition,
PipelineStepProps,
} from './types';
import {pipelineComplete} from './types';

function DummyStepOne({
Expand Down Expand Up @@ -63,11 +67,26 @@ type DummyCompletionData = {
result: string;
};

function DummyCompletionView({
data,
finish,
}: PipelineCompletionProps<DummyCompletionData>) {
return (
<Stack gap="md">
<Text>{data.result}</Text>
<Button size="sm" priority="primary" onClick={finish}>
{t('Done')}
</Button>
</Stack>
);
}

export const dummyIntegrationPipeline = {
type: 'integration',
provider: 'dummy',
actionTitle: t('Dummy Integration Pipeline'),
getCompletionData: pipelineComplete<DummyCompletionData>,
completionView: DummyCompletionView,
steps: [
{
stepId: 'step_one',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const bitbucketIntegrationPipeline = {
provider: 'bitbucket',
actionTitle: t('Installing Bitbucket Integration'),
getCompletionData: pipelineComplete<IntegrationWithConfig>,
completionView: null,
steps: [
{
stepId: 'authorize',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ export const githubIntegrationPipeline = {
provider: 'github',
actionTitle: t('Installing GitHub Integration'),
getCompletionData: pipelineComplete<IntegrationWithConfig>,
completionView: null,
steps: [
{
stepId: 'oauth_login',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ export const gitlabIntegrationPipeline = {
provider: 'gitlab',
actionTitle: t('Installing GitLab Integration'),
getCompletionData: pipelineComplete<IntegrationWithConfig>,
completionView: null,
steps: [
{
stepId: 'installation_config',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const slackIntegrationPipeline = {
provider: 'slack',
actionTitle: t('Installing Slack Integration'),
getCompletionData: pipelineComplete<IntegrationWithConfig>,
completionView: null,
steps: [
{
stepId: 'oauth_login',
Expand Down
14 changes: 14 additions & 0 deletions static/app/components/pipeline/types.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ export interface PipelineStepDefinition<StepId extends string = string> {
stepId: StepId;
}

/**
* Props passed to a pipeline's completion view component.
*/
export interface PipelineCompletionProps<D = unknown> {
data: D;
finish: () => void;
}

/**
* Defines a complete pipeline with its type, provider, and ordered steps.
*/
Expand All @@ -39,6 +47,12 @@ export interface PipelineDefinition<
* Title displayed in the pipeline modal header.
*/
actionTitle: string;
/**
* Component rendered after the pipeline completes. When set, `onComplete`
* is deferred until the component calls `finish()`. When null, `onComplete`
* fires immediately on completion.
*/
completionView: React.ComponentType<PipelineCompletionProps<any>> | null;
/**
* Casts the raw completion response to the typed completion data shape.
* Use the {@link pipelineComplete} helper for this.
Expand Down
69 changes: 67 additions & 2 deletions static/app/components/pipeline/usePipeline.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import {usePipeline} from './usePipeline';
const organization = OrganizationFixture();
const API_URL = `/organizations/${organization.slug}/pipeline/integration_pipeline/`;

function TestHarness() {
const pipeline = usePipeline('integration', 'dummy');
function TestHarness({onComplete}: {onComplete?: (data: any) => void} = {}) {
const pipeline = usePipeline('integration', 'dummy', {onComplete});

return (
<div>
Expand Down Expand Up @@ -239,6 +239,71 @@ describe('usePipeline', () => {
expect(screen.getByTestId('step-index')).toHaveTextContent('0');
});

it('renders the completion view and defers onComplete until finish is called', async () => {
const onComplete = jest.fn();

MockApiClient.addMockResponse({
url: API_URL,
method: 'POST',
body: {
step: 'step_one',
stepIndex: 0,
totalSteps: 2,
provider: 'dummy',
data: {message: 'Enter your name'},
},
match: [MockApiClient.matchData({action: 'initialize', provider: 'dummy'})],
});

MockApiClient.addMockResponse({
url: API_URL,
method: 'POST',
body: {
status: 'advance',
step: 'step_two',
stepIndex: 1,
totalSteps: 2,
provider: 'dummy',
data: {greeting: 'Hello, Test!'},
},
match: [MockApiClient.matchData({name: 'Test'})],
});

render(<TestHarness onComplete={onComplete} />, {organization});

// Advance through step one
expect(await screen.findByText('Enter your name')).toBeInTheDocument();
await userEvent.type(screen.getByRole('textbox', {name: 'Your name'}), 'Test');
await userEvent.click(screen.getByRole('button', {name: 'Continue'}));

// Advance through step two
expect(await screen.findByText('Hello, Test!')).toBeInTheDocument();

MockApiClient.addMockResponse({
url: API_URL,
method: 'POST',
body: {
status: 'complete',
data: {result: 'Pipeline finished successfully'},
},
});

await userEvent.click(screen.getByRole('button', {name: 'Finish'}));

// Completion view should render with the result text and a Done button
expect(await screen.findByText('Pipeline finished successfully')).toBeInTheDocument();
expect(screen.getByRole('button', {name: 'Done'})).toBeInTheDocument();
expect(screen.getByTestId('is-complete')).toHaveTextContent('true');

// onComplete should NOT have been called yet — it's deferred
expect(onComplete).not.toHaveBeenCalled();

// Clicking Done calls finish(), which fires onComplete
await userEvent.click(screen.getByRole('button', {name: 'Done'}));

expect(onComplete).toHaveBeenCalledWith({result: 'Pipeline finished successfully'});
});

it('does not initialize when enabled is false', async () => {
const initRequest = MockApiClient.addMockResponse({
url: API_URL,
Expand Down
30 changes: 28 additions & 2 deletions static/app/components/pipeline/usePipeline.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
import type {
ApiPipeline,
PipelineAdvanceResponse,
PipelineCompletionProps,
PipelineStepProps,
PipelineStepResponse,
} from './types';
Expand Down Expand Up @@ -189,7 +190,11 @@ export function usePipeline<
const data = definition.getCompletionData(rawData) as CompletionDataFor<T, P>;

setState({status: 'complete', data});
onCompleteRef.current?.(data);

// If there's no completion view, fire onComplete immediately.
if (!definition.completionView) {
onCompleteRef.current?.(data);
}
break;
}
case 'error':
Expand Down Expand Up @@ -237,7 +242,20 @@ export function usePipeline<
return null;
}, [state, definition]);

const finish = useCallback(() => {
if (state.status === 'complete') {
onCompleteRef.current?.(state.data);
}
}, [state]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finish lacks guard against repeated onComplete invocations

Medium Severity

The finish callback has no guard to prevent multiple invocations. Each call fires onCompleteRef.current, so a rapid double-click on the "Done" button fires onComplete twice. Unlike advance, which exposes isPending so step components can disable their buttons, PipelineCompletionProps provides no equivalent mechanism for completion views to prevent repeated submission. In the modal, each finish() call triggers handleComplete, which calls both the consumer's onComplete and closeModal(), potentially causing duplicate side effects before the modal unmounts.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 585549a. Configure here.


const view = useMemo(() => {
// Render completion view when the pipeline is complete and one is defined
if (state.status === 'complete' && definition.completionView) {
const CompletionView: React.ComponentType<PipelineCompletionProps<any>> =
definition.completionView;
return <CompletionView data={state.data} finish={finish} />;
}

if (!stepDefinition) {
return null;
}
Expand All @@ -259,7 +277,15 @@ export function usePipeline<
advanceError={advanceError}
/>
);
}, [state, stepDefinition, definition, advanceMutate, isAdvancePending, advanceError]);
}, [
state,
stepDefinition,
definition,
advanceMutate,
isAdvancePending,
advanceError,
finish,
]);

const {stepIndex, totalSteps} =
state.status === 'active'
Expand Down
Loading