feat(editor): Let users update review descriptions (#36069)
Some checks are pending
CI: Master (Build, Test, Lint) / Build for Github Cache (push) Waiting to run
CI: Master (Build, Test, Lint) / Unit tests (22.23.2) (push) Waiting to run
CI: Master (Build, Test, Lint) / Unit tests (24.18.1) (push) Waiting to run
CI: Master (Build, Test, Lint) / Lint (push) Waiting to run
CI: Master (Build, Test, Lint) / Performance (push) Waiting to run
CI: Master (Build, Test, Lint) / Notify Slack on failure (push) Blocked by required conditions

This commit is contained in:
Kai 2026-08-12 20:38:11 +00:00 committed by GitHub
parent 66b3d18d06
commit cbb5577053
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
18 changed files with 664 additions and 110 deletions

View File

@ -29,6 +29,24 @@ describe('UpdateWorkflowReviewRequestVersionDto', () => {
workflowVersionDescription: '',
},
},
{
name: 'an optional review description',
request: {
workflowId: 'workflow-1',
workflowVersionId: 'version-1',
workflowVersionName: 'Release candidate',
description: 'Review the retry behavior',
},
},
{
name: 'an empty review description',
request: {
workflowId: 'workflow-1',
workflowVersionId: 'version-1',
workflowVersionName: 'Release candidate',
description: '',
},
},
])('should validate $name', ({ request }) => {
const result = UpdateWorkflowReviewRequestVersionDto.safeParse(request);
expect(result.success).toBe(true);
@ -111,6 +129,16 @@ describe('UpdateWorkflowReviewRequestVersionDto', () => {
},
expectedErrorPath: ['workflowVersionDescription'],
},
{
name: 'a review description longer than 512 characters',
request: {
workflowId: 'workflow-1',
workflowVersionId: 'version-1',
workflowVersionName: 'Release candidate',
description: 'a'.repeat(513),
},
expectedErrorPath: ['description'],
},
])('should fail validation for $name', ({ request, expectedErrorPath }) => {
const result = UpdateWorkflowReviewRequestVersionDto.safeParse(request);
expect(result.success).toBe(false);

View File

@ -37,6 +37,8 @@ export type WorkflowReviewApprovedPublicationState =
* stay on the minimal summary.
*/
export interface WorkflowReviewRequestForWorkflow extends WorkflowReviewRequestSummary {
/** The review description, but only for a requester who can act on the review */
description: string | null;
/** Who made the current decision; `null` when there is none or the user is gone. */
decisionBy: WorkflowReviewEligibleReviewer | null;
/** Only derived for an approved review; `null` otherwise. */

View File

@ -1,3 +1,5 @@
import { z } from 'zod';
import { n8nIdSchema } from '../../schemas/id.schema';
import {
requiredWorkflowVersionNameSchema,
@ -11,4 +13,6 @@ export class UpdateWorkflowReviewRequestVersionDto extends Z.class({
workflowVersionName: requiredWorkflowVersionNameSchema,
// When present, an empty/whitespace string clears the version description
workflowVersionDescription: workflowVersionDescriptionSchema,
// When present, an empty/whitespace string clears the review description
description: z.string().max(512).optional(),
}) {}

View File

@ -36,7 +36,7 @@ export type FindManyForInboxOptions = {
*/
export type WorkflowReviewRequestForWorkflowRow = Pick<
WorkflowReviewRequest,
'id' | 'state' | 'decision' | 'updatedById' | 'createdAt' | 'updatedAt'
'id' | 'state' | 'decision' | 'description' | 'updatedById' | 'createdAt' | 'updatedAt'
> & {
workflowVersionId: string | null;
};
@ -187,6 +187,7 @@ export class WorkflowReviewRequestRepository extends BaseRepository<WorkflowRevi
id: entity.id,
state: entity.state,
decision: entity.decision,
description: entity.description,
updatedById: entity.updatedById,
createdAt: entity.createdAt,
updatedAt: entity.updatedAt,

View File

@ -40,7 +40,6 @@ import type { WorkflowService } from '@/workflows/workflow.service';
import type { WorkflowReviewEligibilityService } from '../workflow-review-eligibility.service';
import { WorkflowReviewFeatureGate } from '../workflow-review-feature-gate.service';
import { WorkflowReviewRequestService } from '../workflow-review-request.service';
const user = mock<User>({ id: 'user-1' });
/** Build a real `User` with `isPending` computed, as TypeORM does after load. */
@ -570,6 +569,7 @@ describe('WorkflowReviewRequestService', () => {
id: 'req-1',
state: 'open',
decision: 'pending',
description: null,
updatedById: 'user-2',
workflowVersionId: 'ver-1',
createdAt: new Date('2024-01-01T00:00:00.000Z'),
@ -625,6 +625,7 @@ describe('WorkflowReviewRequestService', () => {
id: 'req-1',
state: 'open',
decision: 'changes_requested',
description: null,
workflowVersionId: 'ver-1',
createdAt: '2024-01-01T00:00:00.000Z',
updatedAt: '2024-01-02T00:00:00.000Z',
@ -700,7 +701,6 @@ describe('WorkflowReviewRequestService', () => {
});
});
// R1 (P3): enriching row by row made this list an N+1 — see LIGO-607_review.md
it('enriches many rows with one lookup per derived field', async () => {
workflowFinderService.findWorkflowForUser.mockResolvedValue(mock<WorkflowEntity>());
requestRepository.findRequestsForWorkflow.mockResolvedValue([

View File

@ -94,6 +94,7 @@ describe('WorkflowReviewRequestService.updateVersion', () => {
id: requestId,
state: 'open',
decision: 'pending',
description: null,
createdAt: new Date('2026-07-20T10:00:00.000Z'),
updatedAt: new Date('2026-07-20T11:00:00.000Z'),
...overrides,
@ -358,6 +359,20 @@ describe('WorkflowReviewRequestService.updateVersion', () => {
);
});
it('updates the review description in the same transaction as the re-pin', async () => {
mockSuccessfulUpdatePath();
await service.updateVersion(user, requestId, {
...dto,
description: ' Updated review context ',
});
expect(requestRepository.saveRequest).toHaveBeenCalledWith(
expect.objectContaining({ description: 'Updated review context' }),
ctx,
);
});
it('throws BadRequestError when the version was pruned before the naming write', async () => {
mockSuccessfulUpdatePath();
workflowHistoryRepository.updateVersionMetadata.mockResolvedValue(0);
@ -442,6 +457,63 @@ describe('WorkflowReviewRequestService.updateVersion', () => {
expect(dbLockService.withLockContext).not.toHaveBeenCalled();
});
it('updates the review description under the lock when the version is already pinned', async () => {
mockAlreadyPinned('Same name');
requestRepository.findById.mockResolvedValue(
openRequest({ description: 'Original review description' }),
);
await service.updateVersion(user, requestId, {
...dto,
workflowVersionName: 'Same name',
description: ' Updated review description ',
});
expect(dbLockService.withLockContext).toHaveBeenCalledOnce();
expect(requestRepository.saveRequest).toHaveBeenCalledWith(
expect.objectContaining({
description: 'Updated review description',
updatedById: user.id,
}),
ctx,
);
expect(workflowRepository.updateWorkflowVersion).not.toHaveBeenCalled();
expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledWith('wf-1');
});
it('clears the review description when an empty string is sent', async () => {
mockAlreadyPinned('Same name');
requestRepository.findById.mockResolvedValue(
openRequest({ description: 'Original review description' }),
);
await service.updateVersion(user, requestId, {
...dto,
workflowVersionName: 'Same name',
description: ' ',
});
expect(requestRepository.saveRequest).toHaveBeenCalledWith(
expect.objectContaining({ description: null }),
ctx,
);
});
it('preserves the review description when it is omitted', async () => {
mockAlreadyPinned('Same name');
requestRepository.findById.mockResolvedValue(
openRequest({ description: 'Original review description' }),
);
await service.updateVersion(user, requestId, {
...dto,
workflowVersionName: 'Same name',
});
expect(requestRepository.saveRequest).not.toHaveBeenCalled();
expect(dbLockService.withLockContext).not.toHaveBeenCalled();
});
it('still names the version when a concurrent sync re-pinned it first', async () => {
mockSuccessfulUpdatePath();
workflowHistoryService.findVersion.mockResolvedValue(

View File

@ -22,15 +22,15 @@ import {
WorkflowReviewRequestWorkflowRepository,
} from '@n8n/db';
import { Container } from '@n8n/di';
import { createAdmin, createMember, createOwner, createUser } from '@test-integration/db/users';
import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history';
import type { SuperAgentTest } from '@test-integration/types';
import * as utils from '@test-integration/utils';
import { v4 as uuid } from 'uuid';
import { ActiveWorkflowManager } from '@/active-workflow-manager';
import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service';
import { WorkflowValidationService } from '@/workflows/workflow-validation.service';
import { createAdmin, createMember, createOwner, createUser } from '@test-integration/db/users';
import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history';
import type { SuperAgentTest } from '@test-integration/types';
import * as utils from '@test-integration/utils';
const activeWorkflowManager = mockInstance(ActiveWorkflowManager);
const workflowValidationService = mockInstance(WorkflowValidationService);
@ -936,7 +936,11 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/update-version
versionId: string,
author: User,
projectId = ownerProject.id,
overrides: { state?: 'open' | 'closed'; decision?: 'pending' | 'changes_requested' } = {},
overrides: {
state?: 'open' | 'closed';
decision?: 'pending' | 'changes_requested';
description?: string | null;
} = {},
) {
const request = await requestRepository.createRequest(
{
@ -1066,6 +1070,70 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/update-version
expect(unchanged?.decision).toBe('changes_requested');
});
test('updates the review description when re-pinning the version', async () => {
const { workflow } = await createReviewableWorkflow('version-1');
await createWorkflowHistoryItem(workflow.id, { versionId: 'version-2' });
const request = await seedOpenRequest(workflow.id, 'version-1', owner, ownerProject.id, {
description: 'Original review description',
});
await ownerAgent
.post(`/workflow-review-requests/${request.id}/update-version`)
.send({
workflowId: workflow.id,
workflowVersionId: 'version-2',
workflowVersionName: 'Release candidate',
description: ' Updated review description ',
})
.expect(200);
const updated = await requestRepository.findById(request.id, {});
expect(updated?.description).toBe('Updated review description');
});
test('updates the review description when the version is already pinned', async () => {
const { workflow, versionId } = await createReviewableWorkflow();
const request = await seedOpenRequest(workflow.id, versionId, owner, ownerProject.id, {
decision: 'changes_requested',
description: 'Original review description',
});
await ownerAgent
.post(`/workflow-review-requests/${request.id}/update-version`)
.send({
workflowId: workflow.id,
workflowVersionId: versionId,
workflowVersionName: 'Release candidate',
description: 'Updated review description',
})
.expect(200);
const updated = await requestRepository.findById(request.id, {});
expect(updated).toMatchObject({
description: 'Updated review description',
decision: 'changes_requested',
});
});
test('clears the review description when an empty string is sent', async () => {
const { workflow, versionId } = await createReviewableWorkflow();
const request = await seedOpenRequest(workflow.id, versionId, owner, ownerProject.id, {
description: 'Original review description',
});
await ownerAgent
.post(`/workflow-review-requests/${request.id}/update-version`)
.send({
workflowId: workflow.id,
workflowVersionId: versionId,
workflowVersionName: 'Release candidate',
description: '',
})
.expect(200);
expect((await requestRepository.findById(request.id, {}))?.description).toBeNull();
});
test('returns 404 for an unknown review request id', async () => {
const { workflow, versionId } = await createReviewableWorkflow();
@ -1940,6 +2008,9 @@ describe('GET /workflow-review-requests', () => {
state: 'open',
decision: 'pending',
workflowVersionId: versionId,
// The owner can act on the review, so the description rides along; the
// title stays off the workflow-scoped list entirely.
description: 'Confidential description',
createdAt: expect.any(String),
updatedAt: expect.any(String),
// Neither applies to a pending review
@ -1948,6 +2019,34 @@ describe('GET /workflow-review-requests', () => {
});
});
test('withholds the description from a requester who cannot act on the review', async () => {
const workflow = await createWorkflow({}, teamProject);
const versionId = uuid();
await createWorkflowHistoryItem(workflow.id, { versionId });
const request = await requestRepository.createRequest(
{
projectId: teamProject.id,
title: 'Confidential title',
description: 'Confidential description',
createdById: owner.id,
},
{},
);
await linkRequestToWorkflow(request.id, workflow.id, versionId);
const viewerResponse = await viewerAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id, take: 1 })
.expect(200);
expect(viewerResponse.body.data.data[0].description).toBeNull();
const editorResponse = await memberAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id, take: 1 })
.expect(200);
expect(editorResponse.body.data.data[0].description).toBe('Confidential description');
});
test('returns the newest review, closed included, with take=1 and no state filter', async () => {
const { workflow, versionId } = await createReviewableWorkflow();
const older = await requestRepository.createRequest(

View File

@ -47,13 +47,17 @@ import { WorkflowService } from '@/workflows/workflow.service';
import { WorkflowReviewEligibilityService } from './workflow-review-eligibility.service';
import { WorkflowReviewFeatureGate } from './workflow-review-feature-gate.service';
import { toEligibleReviewer } from './workflow-review.mapper';
/** Omitted stays omitted (column untouched); an empty/whitespace string clears to null. */
function normalizeVersionDescription(description: string | undefined): string | null | undefined {
if (description === undefined) return undefined;
return description.trim() || null;
}
function normalizeReviewDescription(description: string | undefined): string | null | undefined {
if (description === undefined) return undefined;
return description.trim() || null;
}
/**
* The workflow-scoped review request lifecycle: listing a workflow's reviews,
* resolving its eligible reviewers, opening a review, re-pinning it to a new
@ -113,20 +117,28 @@ export class WorkflowReviewRequestService {
throw new NotFoundError('Could not find workflow');
}
const [requests, count] = await this.workflowReviewRequestRepository.findRequestsForWorkflow(
query.workflowId,
{ state: query.state, skip: query.skip, take: query.take },
);
const [[requests, count], canPublish] = await Promise.all([
this.workflowReviewRequestRepository.findRequestsForWorkflow(query.workflowId, {
state: query.state,
skip: query.skip,
take: query.take,
}),
// the description is only for whoever can act on the review
this.workflowFinderService
.findWorkflowForUser(query.workflowId, user, ['workflow:publish'])
.then((found) => found !== null),
]);
return {
count,
data: await this.toWorkflowScopedItems(query.workflowId, requests),
data: await this.toWorkflowScopedItems(query.workflowId, requests, canPublish),
};
}
private async toWorkflowScopedItems(
workflowId: string,
requests: WorkflowReviewRequestForWorkflowRow[],
canPublish: boolean,
): Promise<WorkflowReviewRequestForWorkflow[]> {
const [decisionActors, publicationStates] = await Promise.all([
this.resolveDecisionActors(requests),
@ -137,6 +149,7 @@ export class WorkflowReviewRequestService {
id: request.id,
state: request.state,
decision: request.decision,
description: canPublish ? request.description : null,
workflowVersionId: request.workflowVersionId,
createdAt: request.createdAt.toISOString(),
updatedAt: request.updatedAt.toISOString(),
@ -481,12 +494,18 @@ export class WorkflowReviewRequestService {
const versionName = dto.workflowVersionName.trim();
const versionDescription = normalizeVersionDescription(dto.workflowVersionDescription);
const reviewDescription = normalizeReviewDescription(dto.description);
const metadataChanged = (current: { name: string | null; description: string | null }) =>
versionName !== current.name ||
(versionDescription !== undefined && versionDescription !== current.description);
const hasReviewDescriptionChanged = (current: WorkflowReviewRequest) =>
reviewDescription !== undefined && reviewDescription !== current.description;
// Nothing new to review: skip the lock, write nothing, broadcast nothing.
if (workflowRow.workflowVersionId === dto.workflowVersionId) {
// Nothing new to review: skip the lock unless the review request itself must change.
if (
workflowRow.workflowVersionId === dto.workflowVersionId &&
!hasReviewDescriptionChanged(request)
) {
// A rename or re-description is the one thing that can still be pending
// here, and a lone UPDATE is atomic, so apply it rather than silently
// dropping it.
@ -542,7 +561,15 @@ export class WorkflowReviewRequestService {
);
}
return { request: current, changed: false };
if (reviewDescription === undefined || reviewDescription === current.description) {
return { request: current, changed: false };
}
current.description = reviewDescription;
current.updatedById = user.id;
const saved = await this.workflowReviewRequestRepository.saveRequest(current, ctx);
return { request: saved, changed: true };
}
await this.workflowReviewRequestWorkflowRepository.updateWorkflowVersion(
@ -563,6 +590,9 @@ export class WorkflowReviewRequestService {
);
current.decision = 'pending';
if (reviewDescription !== undefined) {
current.description = reviewDescription;
}
current.updatedById = user.id;
const saved = await this.workflowReviewRequestRepository.saveRequest(current, ctx);

View File

@ -4602,7 +4602,7 @@
"workflowReviews.submitForReview.step": "Step {step} of {total}",
"workflowReviews.submitForReview.ariaDescription": "Name and describe the workflow version, then add a review title, a reviewer, and an optional description.",
"workflowReviews.submitForReview.reviewTitle.label": "Review title",
"workflowReviews.submitForReview.description.label": "Description",
"workflowReviews.submitForReview.description.label": "Review description",
"workflowReviews.submitForReview.reviewer.label": "Reviewer",
"workflowReviews.submitForReview.reviewer.placeholder": "Search users...",
"workflowReviews.submitForReview.submit": "Submit",

View File

@ -587,6 +587,7 @@ describe('WorkflowHeaderDraftPublishActions', () => {
state: 'open',
decision: 'pending',
workflowVersionId: 'version-0',
description: null,
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
decisionBy: null,
@ -694,6 +695,9 @@ describe('WorkflowHeaderDraftPublishActions', () => {
const updateDialog = await findByRole('dialog', {
name: 'Submit latest changes to existing review',
});
// The update dialog is a two-step flow: version metadata, then the review
// description.
await userEvent.click(within(updateDialog).getByTestId('workflow-update-review-next-button'));
await userEvent.click(
within(updateDialog).getByTestId('workflow-update-review-submit-button'),
);
@ -1274,6 +1278,7 @@ describe('WorkflowHeaderDraftPublishActions', () => {
decision: 'pending',
// Differs from the saved 'version-1', so the review is out of date
workflowVersionId: 'version-0',
description: null,
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
decisionBy: null,

View File

@ -175,8 +175,6 @@ const statusSummary = computed(() => {
min-width: 0;
}
/* No card header here, so the role labels carry the grouping — they need more air
between sections than between a label and its own rows. */
.peopleCard {
--n8n--card-body--gap: var(--spacing--sm);
}
@ -197,8 +195,6 @@ const statusSummary = computed(() => {
> span > span {
display: flex;
flex: 1;
/* Long names wrap, so the icon tracks the first line rather than centring
itself against the whole block. */
align-items: flex-start;
min-width: 0;
}

View File

@ -112,6 +112,7 @@ describe('WorkflowReviewRequiredToggle', () => {
state: 'open',
decision: 'pending',
workflowVersionId: 'ver-1',
description: null,
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
decisionBy: null,

View File

@ -16,6 +16,7 @@ const review = (
state: 'open',
decision: 'pending',
workflowVersionId: PINNED_VERSION,
description: null,
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
decisionBy: null,

View File

@ -203,7 +203,7 @@ const submit = async () => {
// install the response before clearing the local flag so the
// publish gate never opens while a refetch is in flight
reviewStatusStore.setOpenReview(workflowId, reviewRequest);
reviewStatusStore.setOpenReview(workflowId, reviewRequest, trimmedDescription || null);
reviewRequiredStore.setReviewRequired(workflowId, false);
emit('update:open', false);
emit('submitted', reviewRequest.id);

View File

@ -1,7 +1,7 @@
import { ResponseError, type WorkflowVersionData } from '@n8n/rest-api-client';
import { createPinia, setActivePinia } from 'pinia';
import userEvent from '@testing-library/user-event';
import { fireEvent, waitFor } from '@testing-library/vue';
import { waitFor } from '@testing-library/vue';
import { createMemoryHistory, createRouter } from 'vue-router';
import { createComponentRenderer } from '@/__tests__/render';
@ -38,6 +38,7 @@ const openReview = {
state: 'open' as const,
decision: 'pending' as const,
workflowVersionId: 'version-1',
description: 'Original review description' as string | null,
createdAt: '2024-01-01T00:00:00.000Z',
updatedAt: '2024-01-01T00:00:00.000Z',
decisionBy: null,
@ -95,8 +96,14 @@ const renderDialog = async ({
};
const result = renderComponent({ pinia, props });
await result.rerender({ ...props, open: true });
const goToStep2 = async () => {
await waitFor(() =>
expect(result.getByTestId('workflow-update-review-next-button')).toBeEnabled(),
);
await userEvent.click(result.getByTestId('workflow-update-review-next-button'));
};
return { ...result, flushSave, documentStore, fetchStatusSpy };
return { ...result, flushSave, documentStore, fetchStatusSpy, goToStep2 };
};
describe('WorkflowUpdateReviewDialog', () => {
@ -127,17 +134,32 @@ describe('WorkflowUpdateReviewDialog', () => {
);
});
it('keeps the description intact without a link when the open review is unknown', async () => {
const { baseElement, queryByRole } = await renderDialog({ seedOpenReview: false });
it('stays open without prefilling when no open review is known yet', async () => {
const { queryByRole, emitted, fetchStatusSpy, getByTestId } = await renderDialog({
seedOpenReview: false,
});
expect(baseElement.textContent?.replace(/\s+/g, ' ')).toContain(
'This workflow already has an open review.',
);
await waitFor(() => expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1'));
expect(queryByRole('link', { name: 'open review' })).not.toBeInTheDocument();
expect(mockShowError).not.toHaveBeenCalled();
expect(emitted('update:open')).toBeUndefined();
await waitFor(() => expect(getByTestId('workflow-update-review-next-button')).toBeEnabled());
});
it('reports a review that no longer exists at submit time', async () => {
const { getByTestId, emitted, goToStep2 } = await renderDialog({ seedOpenReview: false });
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() => expect(mockShowError).toHaveBeenCalledOnce());
expect(updateWorkflowReviewRequestVersion).not.toHaveBeenCalled();
expect(emitted('update:open')).toContainEqual([false]);
});
it('submits the flushed version to the open review and refetches the status', async () => {
const { getByTestId, flushSave, fetchStatusSpy, emitted } = await renderDialog();
const { getByTestId, flushSave, fetchStatusSpy, emitted, goToStep2 } = await renderDialog();
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
@ -150,6 +172,7 @@ describe('WorkflowUpdateReviewDialog', () => {
workflowVersionId: SAVED_VERSION_ID,
workflowVersionName: GENERATED_VERSION_NAME,
workflowVersionDescription: undefined,
description: undefined,
},
);
});
@ -163,14 +186,15 @@ describe('WorkflowUpdateReviewDialog', () => {
it('disables submission when the caller knows the review is up to date', async () => {
const { getByTestId, flushSave } = await renderDialog({ canSubmit: false });
expect(getByTestId('workflow-update-review-submit-button')).toBeDisabled();
expect(getByTestId('workflow-update-review-next-button')).toBeDisabled();
expect(flushSave).not.toHaveBeenCalled();
});
it('closes without updating when saving resolves to the reviewed version', async () => {
const { getByTestId, flushSave, emitted } = await renderDialog({
const { getByTestId, flushSave, emitted, goToStep2 } = await renderDialog({
reviewData: { ...openReview, workflowVersionId: SAVED_VERSION_ID },
});
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
@ -183,7 +207,8 @@ describe('WorkflowUpdateReviewDialog', () => {
it('shows an error and keeps the dialog open when saving fails', async () => {
const flushSave = vi.fn().mockResolvedValue(undefined);
const { getByTestId, emitted } = await renderDialog({ flushSave });
const { getByTestId, emitted, goToStep2 } = await renderDialog({ flushSave });
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
@ -200,7 +225,8 @@ describe('WorkflowUpdateReviewDialog', () => {
resolveSave = resolve;
}),
);
const { getByTestId, rerender, emitted } = await renderDialog({ flushSave });
const { getByTestId, rerender, emitted, goToStep2 } = await renderDialog({ flushSave });
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() => expect(flushSave).toHaveBeenCalledOnce());
@ -210,25 +236,98 @@ describe('WorkflowUpdateReviewDialog', () => {
await rerender({ open: true, workflowId: 'workflow-2', flushSave });
resolveSave('version-2');
await waitFor(() => expect(getByTestId('workflow-update-review-submit-button')).toBeEnabled());
await waitFor(() => expect(getByTestId('workflow-update-review-next-button')).toBeEnabled());
expect(updateWorkflowReviewRequestVersion).not.toHaveBeenCalled();
expect(mockShowError).not.toHaveBeenCalled();
expect(emitted('updated')).toBeUndefined();
});
it('shows an error and closes when no open review is found after a refetch', async () => {
const { getByTestId, fetchStatusSpy, emitted } = await renderDialog({
seedOpenReview: false,
describe('review description', () => {
it('uses the same two-step flow as review creation and prefills the description', async () => {
const { getByTestId, goToStep2 } = await renderDialog();
expect(getByTestId('workflow-update-review-dialog-step')).toHaveTextContent('Step 1 of 2');
await goToStep2();
expect(getByTestId('workflow-update-review-dialog-step')).toHaveTextContent('Step 2 of 2');
expect(getByTestId('workflow-update-review-description-input')).toHaveValue(
'Original review description',
);
expect(getByTestId('workflow-update-review-description-input')).toHaveAttribute(
'maxlength',
'512',
);
});
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
it('returns to the version step without losing the review description', async () => {
const { getByTestId, goToStep2 } = await renderDialog();
await goToStep2();
await userEvent.clear(getByTestId('workflow-update-review-description-input'));
await userEvent.type(
getByTestId('workflow-update-review-description-input'),
'Updated context',
);
await waitFor(() => expect(mockShowError).toHaveBeenCalledOnce());
// One refetch attempt before giving up.
expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1');
expect(updateWorkflowReviewRequestVersion).not.toHaveBeenCalled();
expect(emitted('updated')).toBeUndefined();
expect(emitted('update:open')).toContainEqual([false]);
await userEvent.click(getByTestId('workflow-update-review-back-button'));
expect(getByTestId('workflow-update-review-version-name-input')).toBeInTheDocument();
await userEvent.click(getByTestId('workflow-update-review-next-button'));
expect(getByTestId('workflow-update-review-description-input')).toHaveValue(
'Updated context',
);
});
it('submits a changed review description', async () => {
const { getByTestId, goToStep2 } = await renderDialog();
await goToStep2();
await userEvent.clear(getByTestId('workflow-update-review-description-input'));
await userEvent.type(
getByTestId('workflow-update-review-description-input'),
' Updated review context ',
);
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() =>
expect(updateWorkflowReviewRequestVersion).toHaveBeenCalledWith(
expect.any(Object),
'review-1',
expect.objectContaining({ description: 'Updated review context' }),
),
);
});
it('omits an unchanged review description', async () => {
const { getByTestId, goToStep2 } = await renderDialog();
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() =>
expect(updateWorkflowReviewRequestVersion).toHaveBeenCalledWith(
expect.any(Object),
'review-1',
expect.objectContaining({ description: undefined }),
),
);
});
it('sends an empty review description when the prefilled value is cleared', async () => {
const { getByTestId, goToStep2 } = await renderDialog();
await goToStep2();
await userEvent.clear(getByTestId('workflow-update-review-description-input'));
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() =>
expect(updateWorkflowReviewRequestVersion).toHaveBeenCalledWith(
expect.any(Object),
'review-1',
expect.objectContaining({ description: '' }),
),
);
});
});
describe('version name and description', () => {
@ -272,22 +371,23 @@ describe('WorkflowUpdateReviewDialog', () => {
);
});
it('blocks submission while the name is empty', async () => {
it('blocks advancing while the name is empty', async () => {
const { getByTestId } = await renderDialog();
await userEvent.clear(getByTestId('workflow-update-review-version-name-input'));
expect(getByTestId('workflow-update-review-submit-button')).toBeDisabled();
expect(getByTestId('workflow-update-review-next-button')).toBeDisabled();
});
it('submits the trimmed name and mirrors it into the editor', async () => {
const { getByTestId, documentStore } = await renderDialog();
const { getByTestId, documentStore, goToStep2 } = await renderDialog();
await userEvent.clear(getByTestId('workflow-update-review-version-name-input'));
await userEvent.type(
getByTestId('workflow-update-review-version-name-input'),
' Release 3 ',
);
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() => {
@ -299,6 +399,7 @@ describe('WorkflowUpdateReviewDialog', () => {
workflowVersionId: SAVED_VERSION_ID,
workflowVersionName: 'Release 3',
workflowVersionDescription: undefined,
description: undefined,
},
);
});
@ -310,12 +411,13 @@ describe('WorkflowUpdateReviewDialog', () => {
});
it('submits the trimmed description and mirrors it into the editor', async () => {
const { getByTestId, documentStore } = await renderDialog();
const { getByTestId, documentStore, goToStep2 } = await renderDialog();
await userEvent.type(
getByTestId('workflow-update-review-version-description-input'),
' What changed ',
);
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() => {
@ -329,7 +431,7 @@ describe('WorkflowUpdateReviewDialog', () => {
});
it('sends an empty description when the prefilled one is cleared', async () => {
const { getByTestId, documentStore } = await renderDialog({
const { getByTestId, documentStore, goToStep2 } = await renderDialog({
versionData: {
versionId: SAVED_VERSION_ID,
name: 'Release candidate',
@ -338,6 +440,7 @@ describe('WorkflowUpdateReviewDialog', () => {
});
await userEvent.clear(getByTestId('workflow-update-review-version-description-input'));
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() => {
@ -350,25 +453,22 @@ describe('WorkflowUpdateReviewDialog', () => {
expect(documentStore.versionData).toMatchObject({ description: null });
});
// The name is read before `flushSave()` is awaited, so a mid-save change can't
// reach the request. `fireEvent` bypasses the disabled input the way a stray
// programmatic write would, keeping the snapshot covered on its own.
it('locks the name while submitting and sends the one validated at click time', async () => {
it('sends the name validated before advancing to the review step', async () => {
let resolveSave!: (versionId: string | undefined) => void;
const flushSave = vi.fn().mockReturnValue(
new Promise<string | undefined>((resolve) => {
resolveSave = resolve;
}),
);
const { getByTestId, documentStore } = await renderDialog({ flushSave });
const { getByTestId, documentStore, goToStep2 } = await renderDialog({ flushSave });
const input = getByTestId('workflow-update-review-version-name-input');
await userEvent.clear(input);
await userEvent.type(input, 'Validated name');
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() => expect(input).toBeDisabled());
await fireEvent.update(input, '');
await waitFor(() => expect(getByTestId('workflow-update-review-back-button')).toBeDisabled());
resolveSave(SAVED_VERSION_ID);
await waitFor(() => {
@ -380,6 +480,7 @@ describe('WorkflowUpdateReviewDialog', () => {
workflowVersionId: SAVED_VERSION_ID,
workflowVersionName: 'Validated name',
workflowVersionDescription: undefined,
description: undefined,
},
);
});
@ -391,7 +492,8 @@ describe('WorkflowUpdateReviewDialog', () => {
vi.mocked(updateWorkflowReviewRequestVersion).mockRejectedValue(
new ResponseError('Conflict', { httpStatusCode: 409 }),
);
const { getByTestId, fetchStatusSpy, emitted } = await renderDialog();
const { getByTestId, fetchStatusSpy, emitted, goToStep2 } = await renderDialog();
await goToStep2();
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
@ -399,4 +501,35 @@ describe('WorkflowUpdateReviewDialog', () => {
expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1');
expect(emitted('updated')).toBeUndefined();
});
it('retries against the review that replaced the one open when the dialog was opened', async () => {
vi.mocked(updateWorkflowReviewRequestVersion).mockRejectedValueOnce(
new ResponseError('Conflict', { httpStatusCode: 409 }),
);
const { getByTestId, goToStep2 } = await renderDialog();
await goToStep2();
// The first attempt conflicts, and the status refetch reports a new review.
vi.mocked(fetchWorkflowReviewRequests).mockResolvedValue({
count: 1,
data: [{ ...openReview, id: 'review-2' }],
});
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() => expect(mockShowError).toHaveBeenCalledOnce());
vi.mocked(updateWorkflowReviewRequestVersion).mockResolvedValue({
...openReview,
id: 'review-2',
workflowVersionId: SAVED_VERSION_ID,
});
await userEvent.click(getByTestId('workflow-update-review-submit-button'));
await waitFor(() =>
expect(updateWorkflowReviewRequestVersion).toHaveBeenLastCalledWith(
expect.any(Object),
'review-2',
expect.any(Object),
),
);
});
});

View File

@ -1,11 +1,17 @@
<script setup lang="ts">
import type { WorkflowReviewRequestForWorkflow } from '@n8n/api-types';
import { useRootStore } from '@n8n/stores/useRootStore';
import {
N8nButton,
N8nDialog,
N8nDialogDescription,
N8nDialogFooter,
N8nDialogHeader,
N8nDialogTitle,
N8nInput,
N8nInputLabel,
N8nLink,
N8nText,
} from '@n8n/design-system';
import { useI18n } from '@n8n/i18n';
import { computed, ref, watch } from 'vue';
@ -13,11 +19,14 @@ import { I18nT } from 'vue-i18n';
import { useToast } from '@n8n/composables/useToast';
import WorkflowVersionForm from '@/app/components/WorkflowVersionForm.vue';
import { useLatestFetch } from '@/app/composables/useLatestFetch';
import { useReviewVersionName } from '@/features/workflow-reviews/composables/useReviewVersionName';
import { WORKFLOW_REVIEW_REQUESTS_VIEW } from '@/features/workflow-reviews/constants';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
import { updateWorkflowReviewRequestVersion } from '@/features/workflow-reviews/workflowReviews.api';
const REVIEW_DESCRIPTION_MAX_LENGTH = 512;
const props = withDefaults(
defineProps<{
open: boolean;
@ -45,21 +54,28 @@ const {
applyVersionMetadata,
} = useReviewVersionName();
const step = ref<1 | 2>(1);
const reviewDescription = ref('');
const prefilledReviewDescription = ref('');
const loadedReview = ref<WorkflowReviewRequestForWorkflow | null>(null);
const isLoadingReviewDescription = ref(false);
const isSubmitting = ref(false);
const workflowReviewRequestId = computed(
() => reviewStatusStore.openReviewRequest(props.workflowId)?.id,
() => loadedReview.value?.id ?? reviewStatusStore.openReviewRequest(props.workflowId)?.id,
);
const isSubmitDisabled = computed(
() => isSubmitting.value || !props.canSubmit || versionName.value.trim().length === 0,
const stepLabel = computed(() =>
i18n.baseText('workflowReviews.submitForReview.step', {
interpolate: { step: step.value, total: 2 },
}),
);
watch(
() => props.open,
(isOpen) => {
if (isOpen) prefillVersionName();
},
);
const hasInvalidVersion = computed(() => !props.canSubmit || versionName.value.trim().length === 0);
// neither gate depends on the prefill succeeding, the review is re-resolved at submit time
const isNextDisabled = computed(() => hasInvalidVersion.value || isLoadingReviewDescription.value);
const isSubmitDisabled = computed(() => isSubmitting.value || hasInvalidVersion.value);
const close = () => {
if (isSubmitting.value) return;
@ -76,14 +92,78 @@ const resolveOpenReviewRequest = async (workflowId: string) => {
return request;
};
const { next: nextReviewLoad } = useLatestFetch();
const loadReviewDescription = async (workflowId: string) => {
const isCurrent = nextReviewLoad();
isLoadingReviewDescription.value = true;
try {
const openReviewRequest = await resolveOpenReviewRequest(workflowId);
if (!isCurrent() || props.workflowId !== workflowId) return;
// a missing review here must not close the dialog
if (!openReviewRequest) return;
loadedReview.value = openReviewRequest;
reviewDescription.value = openReviewRequest.description ?? '';
prefilledReviewDescription.value = reviewDescription.value;
} catch (error) {
if (!isCurrent() || props.workflowId !== workflowId) return;
toast.showError(error, i18n.baseText('workflowReviews.updateReview.error.title'));
} finally {
if (isCurrent()) isLoadingReviewDescription.value = false;
}
};
watch(
() => [props.open, props.workflowId] as const,
([isOpen, workflowId]) => {
if (!isOpen) {
nextReviewLoad();
loadedReview.value = null;
isLoadingReviewDescription.value = false;
return;
}
step.value = 1;
loadedReview.value = null;
reviewDescription.value = '';
prefilledReviewDescription.value = '';
prefillVersionName();
void loadReviewDescription(workflowId);
},
);
const goToReviewStep = () => {
if (isNextDisabled.value) return;
step.value = 2;
};
const goBack = () => {
if (isSubmitting.value) return;
step.value = 1;
};
const submittedReviewDescription = (): string | undefined => {
const trimmed = reviewDescription.value.trim();
return trimmed === prefilledReviewDescription.value.trim() ? undefined : trimmed;
};
const handleFormSubmit = () => {
if (step.value === 1) goToReviewStep();
else void submit();
};
const submit = async () => {
if (isSubmitDisabled.value) return;
const workflowId = props.workflowId;
// `flushSave()` awaits a full workflow save, so reading the fields afterwards
// could send values the guard never validated.
const trimmedVersionName = versionName.value.trim();
const trimmedVersionDescription = submittedVersionDescription();
const trimmedReviewDescription = submittedReviewDescription();
isSubmitting.value = true;
try {
@ -101,8 +181,12 @@ const submit = async () => {
return;
}
const openReviewRequest = await resolveOpenReviewRequest(workflowId);
if (!openReviewRequest) {
// the dialog may have sat open across a long save, so re-resolve
// rather than trusting the open-time snapshot.
const reviewRequest = await resolveOpenReviewRequest(workflowId);
if (props.workflowId !== workflowId) return;
if (!reviewRequest) {
toast.showError(
new Error(i18n.baseText('workflowReviews.updateReview.error.noOpenReview')),
i18n.baseText('workflowReviews.updateReview.error.title'),
@ -110,26 +194,37 @@ const submit = async () => {
emit('update:open', false);
return;
}
if (openReviewRequest.workflowVersionId === workflowVersionId) {
loadedReview.value = reviewRequest;
if (
reviewRequest.workflowVersionId === workflowVersionId &&
trimmedReviewDescription === undefined
) {
emit('update:open', false);
return;
}
await updateWorkflowReviewRequestVersion(rootStore.restApiContext, openReviewRequest.id, {
await updateWorkflowReviewRequestVersion(rootStore.restApiContext, reviewRequest.id, {
workflowId,
workflowVersionId,
workflowVersionName: trimmedVersionName,
workflowVersionDescription: trimmedVersionDescription,
description: trimmedReviewDescription,
});
if (props.workflowId !== workflowId) return;
applyVersionMetadata(workflowVersionId, trimmedVersionName, trimmedVersionDescription);
void reviewStatusStore.fetchStatus(workflowId);
emit('update:open', false);
emit('updated', openReviewRequest.id);
emit('updated', reviewRequest.id);
} catch (error) {
// Whatever went wrong (e.g. the review closed concurrently), refetch the review state.
void reviewStatusStore.fetchStatus(workflowId);
// Whatever went wrong (e.g. the review closed concurrently), refetch the
// review state — and adopt it, so a retry can't reuse the dead request id.
await reviewStatusStore.fetchStatus(workflowId);
if (props.workflowId === workflowId) {
loadedReview.value = reviewStatusStore.openReviewRequest(workflowId);
}
toast.showError(error, i18n.baseText('workflowReviews.updateReview.error.title'));
} finally {
isSubmitting.value = false;
@ -138,12 +233,20 @@ const submit = async () => {
</script>
<template>
<N8nDialog
:open="open"
size="medium"
:header="i18n.baseText('workflowReviews.updateReview.title')"
@update:open="close"
>
<N8nDialog :open="open" size="medium" @update:open="close">
<N8nDialogHeader>
<N8nText
:class="$style.step"
size="xsmall"
color="text-light"
bold
tag="p"
data-test-id="workflow-update-review-dialog-step"
>
{{ stepLabel }}
</N8nText>
<N8nDialogTitle>{{ i18n.baseText('workflowReviews.updateReview.title') }}</N8nDialogTitle>
</N8nDialogHeader>
<N8nDialogDescription :class="$style.description">
<I18nT keypath="workflowReviews.updateReview.description" tag="span" scope="global">
<!-- the id is undefined until fetchStatus resolves (409 path), so the
@ -164,35 +267,76 @@ const submit = async () => {
</template>
</I18nT>
</N8nDialogDescription>
<WorkflowVersionForm
v-model:version-name="versionName"
v-model:description="versionDescription"
:class="$style.versionForm"
:disabled="isSubmitting"
version-name-test-id="workflow-update-review-version-name-input"
description-test-id="workflow-update-review-version-description-input"
@submit="submit"
/>
<N8nDialogFooter data-test-id="workflow-update-review-dialog">
<N8nButton
type="button"
variant="outline"
<form
:class="$style.form"
data-test-id="workflow-update-review-dialog"
@submit.prevent="handleFormSubmit"
>
<WorkflowVersionForm
v-if="step === 1"
v-model:version-name="versionName"
v-model:description="versionDescription"
:disabled="isSubmitting"
data-test-id="workflow-update-review-cancel-button"
@click="close"
version-name-test-id="workflow-update-review-version-name-input"
description-test-id="workflow-update-review-version-description-input"
@submit="goToReviewStep"
/>
<N8nInputLabel
v-else
input-name="workflow-update-review-description"
:label="i18n.baseText('workflowReviews.submitForReview.description.label')"
>
{{ i18n.baseText('generic.cancel') }}
</N8nButton>
<N8nButton
type="button"
:loading="isSubmitting"
:disabled="isSubmitDisabled"
data-test-id="workflow-update-review-submit-button"
@click="submit"
>
{{ i18n.baseText('workflowReviews.updateReview.submit') }}
</N8nButton>
</N8nDialogFooter>
<N8nInput
id="workflow-update-review-description"
v-model="reviewDescription"
type="textarea"
:rows="3"
:maxlength="REVIEW_DESCRIPTION_MAX_LENGTH"
:disabled="isSubmitting"
data-test-id="workflow-update-review-description-input"
/>
</N8nInputLabel>
<N8nDialogFooter>
<template v-if="step === 1">
<N8nButton
type="button"
variant="outline"
:disabled="isSubmitting"
data-test-id="workflow-update-review-cancel-button"
@click="close"
>
{{ i18n.baseText('generic.cancel') }}
</N8nButton>
<N8nButton
type="submit"
:loading="isLoadingReviewDescription"
:disabled="isNextDisabled"
data-test-id="workflow-update-review-next-button"
>
{{ i18n.baseText('generic.next') }}
</N8nButton>
</template>
<template v-else>
<N8nButton
type="button"
variant="outline"
:disabled="isSubmitting"
data-test-id="workflow-update-review-back-button"
@click="goBack"
>
{{ i18n.baseText('generic.back') }}
</N8nButton>
<N8nButton
type="submit"
:loading="isSubmitting"
:disabled="isSubmitDisabled"
data-test-id="workflow-update-review-submit-button"
>
{{ i18n.baseText('workflowReviews.updateReview.submit') }}
</N8nButton>
</template>
</N8nDialogFooter>
</form>
</N8nDialog>
</template>
@ -202,7 +346,16 @@ const submit = async () => {
margin-top: var(--spacing--xs);
}
.versionForm {
.form {
display: flex;
flex-direction: column;
gap: var(--spacing--lg);
margin-top: var(--spacing--sm);
}
.step {
margin: 0;
text-transform: uppercase;
letter-spacing: var(--letter-spacing--wide);
}
</style>

View File

@ -18,6 +18,7 @@ const review = (
state: 'open',
decision: 'pending',
workflowVersionId: 'ver-1',
description: null,
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
decisionBy: null,
@ -127,6 +128,29 @@ describe('reviewStatus.store', () => {
expect(store.hasOpenReview('workflow-1')).toBe(true);
});
// R1 (P2): the mutation response has no description, so the creating dialog
// supplies what it submitted — see LIGO-979_review.md.
it('keeps the description the caller submitted with a freshly created review', () => {
const store = useWorkflowReviewStatusStore();
store.setOpenReview(
'workflow-1',
{
id: 'req-9',
state: 'open',
decision: 'pending',
workflowVersionId: 'ver-9',
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
},
'Please review the retry logic',
);
expect(store.latestReviewRequest('workflow-1')?.description).toBe(
'Please review the retry logic',
);
});
it('stores null when the API returns no review', async () => {
const store = useWorkflowReviewStatusStore();
fetchMock.mockResolvedValue(listOf());

View File

@ -77,10 +77,15 @@ export const useWorkflowReviewStatusStore = defineStore('workflowReviewStatus',
* refetch. Mutation responses carry the minimal summary: a brand-new review is
* pending, so neither derived field applies yet.
*/
const setOpenReview = (workflowId: string, review: WorkflowReviewRequestSummary): void => {
const setOpenReview = (
workflowId: string,
review: WorkflowReviewRequestSummary,
description: string | null = null,
): void => {
latestSequenceByWorkflowId[workflowId] = (latestSequenceByWorkflowId[workflowId] ?? 0) + 1;
latestReviewByWorkflowId.value[workflowId] = {
...review,
description,
decisionBy: null,
approvedVersionPublicationState: null,
};