feat(editor): Keep review-required toggle synchronized with open reviews (#34606)

This commit is contained in:
Kai
2026-07-22 09:28:10 +00:00
committed by GitHub
parent 87115d6cb9
commit ca0c5f4938
31 changed files with 1361 additions and 81 deletions
+1 -1
View File
@@ -253,7 +253,7 @@ What we use for testing and writing tests:
- **To iterate on a feature without docker rebuilds**, boot service containers
and run `pnpm dev` locally — `pnpm --filter n8n-containers services --services postgres,redis,mailpit,proxy`
then `pnpm dev`. See [Develop against running containers](packages/testing/playwright/README.md#develop-against-running-containers-avoid-docker-rebuilds).
- **For Playwright test maintenance/cleanup**, see @packages/testing/playwright/AGENTS.md (includes janitor tool for static analysis, dead code removal, architecture enforcement, and TCR workflows).
- **For Playwright test maintenance/cleanup**, see `packages/testing/playwright/AGENTS.md` (includes janitor tool for static analysis, dead code removal, architecture enforcement, and TCR workflows).
### Common Development Tasks
+1
View File
@@ -293,6 +293,7 @@ export {
export type { EncryptionKeyResponseDto } from './encryption/encryption-key-response.dto';
export { CreateWorkflowReviewRequestDto } from './workflow-reviews/create-workflow-review-request.dto';
export { ListWorkflowReviewRequestsQueryDto } from './workflow-reviews/list-workflow-review-requests-query.dto';
export { UpdateOtelSettingsDto } from './otel/update-otel-settings.dto';
export { TestOtelConnectionDto } from './otel/test-otel-connection.dto';
@@ -0,0 +1,72 @@
import { ListWorkflowReviewRequestsQueryDto } from '../list-workflow-review-requests-query.dto';
const DEFAULT_PAGINATION = { skip: 0, take: 10 };
describe('ListWorkflowReviewRequestsQueryDto', () => {
describe('Valid requests', () => {
test.each([
{
name: 'workflowId only',
request: { workflowId: 'workflow-1' },
parsedResult: { ...DEFAULT_PAGINATION, workflowId: 'workflow-1' },
},
{
name: 'state open',
request: { workflowId: 'workflow-1', state: 'open' },
parsedResult: { ...DEFAULT_PAGINATION, workflowId: 'workflow-1', state: 'open' },
},
{
name: 'state closed',
request: { workflowId: 'workflow-1', state: 'closed' },
parsedResult: { ...DEFAULT_PAGINATION, workflowId: 'workflow-1', state: 'closed' },
},
{
name: 'skip and take coerced from strings',
request: { workflowId: 'workflow-1', skip: '5', take: '1' },
parsedResult: { workflowId: 'workflow-1', skip: 5, take: 1 },
},
])('should validate $name', ({ request, parsedResult }) => {
const result = ListWorkflowReviewRequestsQueryDto.safeParse(request);
expect(result.success).toBe(true);
if (parsedResult) {
expect(result.data).toMatchObject(parsedResult);
}
});
});
describe('Invalid requests', () => {
test.each([
{
name: 'missing workflowId',
request: {},
expectedErrorPath: ['workflowId'],
},
{
name: 'empty workflowId',
request: { workflowId: '' },
expectedErrorPath: ['workflowId'],
},
{
name: 'invalid state',
request: { workflowId: 'workflow-1', state: 'archived' },
expectedErrorPath: ['state'],
},
{
name: 'non-integer take',
request: { workflowId: 'workflow-1', take: 'not-a-number' },
expectedErrorPath: ['take'],
},
{
name: 'negative skip',
request: { workflowId: 'workflow-1', skip: '-1' },
expectedErrorPath: ['skip'],
},
])('should fail validation for $name', ({ request, expectedErrorPath }) => {
const result = ListWorkflowReviewRequestsQueryDto.safeParse(request);
expect(result.success).toBe(false);
if (expectedErrorPath) {
expect(result.error?.issues[0].path).toEqual(expectedErrorPath);
}
});
});
});
@@ -0,0 +1,12 @@
import { z } from 'zod';
import { workflowReviewRequestStateSchema } from '../../workflow-review-request-summary';
import { Z } from '../../zod-class';
import { paginationSchema } from '../pagination/pagination.dto';
export class ListWorkflowReviewRequestsQueryDto extends Z.class({
...paginationSchema,
// Required until cross-workflow listing gets project-based access filtering (LIGO-597)
workflowId: z.string().min(1),
state: workflowReviewRequestStateSchema.optional(),
}) {}
+1
View File
@@ -13,6 +13,7 @@ export * from './instance-registry-types';
export * from './redaction-enforcement';
export * from './redaction-enforcement-floor';
export * from './workflow-reviews-policy';
export * from './workflow-review-request-summary';
export {
chatHubConversationModelSchema,
type ChatModelDto,
+3 -1
View File
@@ -8,6 +8,7 @@ import type { InstanceAiPushMessage } from './instance-ai';
import type { WebhookPushMessage } from './webhook';
import type { WorkerPushMessage } from './worker';
import type { WorkflowPushMessage } from './workflow';
import type { WorkflowReviewPushMessage } from './workflow-review';
export type PushMessage =
| ExecutionPushMessage
@@ -19,7 +20,8 @@ export type PushMessage =
| DebugPushMessage
| BuilderCreditsPushMessage
| ChatHubPushMessage
| InstanceAiPushMessage;
| InstanceAiPushMessage
| WorkflowReviewPushMessage;
export type PushType = PushMessage['type'];
@@ -0,0 +1,12 @@
/**
* Invalidation-only signal: carries no review state so clients must refetch
* the authoritative status instead of mirroring event payloads.
*/
export type WorkflowReviewStateChanged = {
type: 'workflowReviewStateChanged';
data: {
workflowId: string;
};
};
export type WorkflowReviewPushMessage = WorkflowReviewStateChanged;
@@ -0,0 +1,26 @@
import { z } from 'zod';
import type { Iso8601DateTimeString } from './datetime';
export const workflowReviewRequestStateSchema = z.enum(['open', 'closed']);
export type WorkflowReviewRequestState = z.infer<typeof workflowReviewRequestStateSchema>;
export const workflowReviewRequestDecisionSchema = z.enum([
'pending',
'changes_requested',
'approved',
]);
export type WorkflowReviewRequestDecision = z.infer<typeof workflowReviewRequestDecisionSchema>;
export type WorkflowReviewRequestSummary = {
id: string;
state: WorkflowReviewRequestState;
decision: WorkflowReviewRequestDecision;
createdAt: Iso8601DateTimeString;
updatedAt: Iso8601DateTimeString;
};
export type WorkflowReviewRequestList = {
count: number;
data: WorkflowReviewRequestSummary[];
};
@@ -1,14 +1,18 @@
import type {
WorkflowReviewRequestDecision as WorkflowReviewRequestDecisionType,
WorkflowReviewRequestState as WorkflowReviewRequestStateType,
} from '@n8n/api-types';
import { Column, Entity, Index } from '@n8n/typeorm';
import { DateTimeColumn, WithTimestampsAndStringId } from './abstract-entity';
export type WorkflowReviewRequestState = WorkflowReviewRequestStateType;
export type WorkflowReviewRequestDecision = WorkflowReviewRequestDecisionType;
export const WorkflowReviewRequestState = {
Open: 'open',
Closed: 'closed',
} as const;
export type WorkflowReviewRequestState =
(typeof WorkflowReviewRequestState)[keyof typeof WorkflowReviewRequestState];
} as const satisfies Record<string, WorkflowReviewRequestStateType>;
export const WorkflowReviewRequestStateList = Object.values(WorkflowReviewRequestState);
@@ -16,10 +20,7 @@ export const WorkflowReviewRequestDecision = {
Pending: 'pending',
ChangesRequested: 'changes_requested',
Approved: 'approved',
} as const;
export type WorkflowReviewRequestDecision =
(typeof WorkflowReviewRequestDecision)[keyof typeof WorkflowReviewRequestDecision];
} as const satisfies Record<string, WorkflowReviewRequestDecisionType>;
export const WorkflowReviewRequestDecisionList = Object.values(WorkflowReviewRequestDecision);
@@ -30,10 +31,10 @@ export class WorkflowReviewRequest extends WithTimestampsAndStringId {
projectId: string;
@Column({ type: 'varchar', length: 16 })
state: WorkflowReviewRequestState;
state: WorkflowReviewRequestStateType;
@Column({ type: 'varchar', length: 50 })
decision: WorkflowReviewRequestDecision;
decision: WorkflowReviewRequestDecisionType;
@Column({ type: 'varchar', length: 255 })
title: string;
@@ -1,5 +1,7 @@
import { Container } from '@n8n/di';
import type { Mock } from 'vitest';
import type { SelectQueryBuilder } from '@n8n/typeorm';
import type { Mock, Mocked } from 'vitest';
import { mock } from 'vitest-mock-extended';
import { WorkflowReviewRequest } from '../../entities/workflow-review-request.ee';
import { mockEntityManager } from '../../utils/test-utils/mock-entity-manager';
@@ -62,4 +64,60 @@ describe('WorkflowReviewRequestRepository', () => {
});
});
});
describe('findRequestsForWorkflow', () => {
let queryBuilder: Mocked<SelectQueryBuilder<WorkflowReviewRequest>>;
beforeEach(() => {
queryBuilder = mock<SelectQueryBuilder<WorkflowReviewRequest>>();
queryBuilder.innerJoin.mockReturnThis();
queryBuilder.where.mockReturnThis();
queryBuilder.andWhere.mockReturnThis();
queryBuilder.orderBy.mockReturnThis();
queryBuilder.skip.mockReturnThis();
queryBuilder.take.mockReturnThis();
queryBuilder.getManyAndCount.mockResolvedValue([[], 0]);
(entityManager.createQueryBuilder as Mock).mockReturnValue(queryBuilder);
});
it('scopes to the requested workflow and orders by createdAt DESC', async () => {
await repo.findRequestsForWorkflow('workflow-1');
expect(queryBuilder.where).toHaveBeenCalledWith('requestWorkflow.workflowId = :workflowId', {
workflowId: 'workflow-1',
});
expect(queryBuilder.orderBy).toHaveBeenCalledWith('request.createdAt', 'DESC');
expect(queryBuilder.andWhere).not.toHaveBeenCalled();
expect(queryBuilder.skip).not.toHaveBeenCalled();
expect(queryBuilder.take).not.toHaveBeenCalled();
});
it.each(['open', 'closed'] as const)('narrows to state %s when given', async (state) => {
await repo.findRequestsForWorkflow('workflow-1', { state });
expect(queryBuilder.andWhere).toHaveBeenCalledWith('request.state = :state', { state });
});
it('applies skip and take while returning the total match count', async () => {
const rows = [mock<WorkflowReviewRequest>({ id: 'req-2' })];
queryBuilder.getManyAndCount.mockResolvedValue([rows, 5]);
const [data, count] = await repo.findRequestsForWorkflow('workflow-1', {
skip: 1,
take: 1,
});
expect(queryBuilder.skip).toHaveBeenCalledWith(1);
expect(queryBuilder.take).toHaveBeenCalledWith(1);
expect(data).toEqual(rows);
expect(count).toBe(5);
});
it('applies skip and take when they are zero', async () => {
await repo.findRequestsForWorkflow('workflow-1', { skip: 0, take: 0 });
expect(queryBuilder.skip).toHaveBeenCalledWith(0);
expect(queryBuilder.take).toHaveBeenCalledWith(0);
});
});
});
@@ -49,6 +49,33 @@ export class WorkflowReviewRequestRepository extends Repository<WorkflowReviewRe
return await this.findOne({ where: { id } });
}
async findRequestsForWorkflow(
workflowId: string,
options: { state?: WorkflowReviewRequestState; skip?: number; take?: number } = {},
): Promise<[WorkflowReviewRequest[], number]> {
const qb = this.manager
.createQueryBuilder(WorkflowReviewRequest, 'request')
.innerJoin(
WorkflowReviewRequestWorkflow,
'requestWorkflow',
'requestWorkflow.workflowReviewRequestId = request.id',
)
.where('requestWorkflow.workflowId = :workflowId', { workflowId })
.orderBy('request.createdAt', 'DESC');
if (options.state) {
qb.andWhere('request.state = :state', { state: options.state });
}
if (options.skip !== undefined) {
qb.skip(options.skip);
}
if (options.take !== undefined) {
qb.take(options.take);
}
return await qb.getManyAndCount();
}
async findOpenRequestForWorkflow(
workflowId: string,
trx?: EntityManager,
@@ -7,13 +7,6 @@ import { ErrorReporter } from 'n8n-core';
import type { IWorkflowSettings, Workflow } from 'n8n-workflow';
import { UnexpectedError } from 'n8n-workflow';
import { CollaborationState } from '@/collaboration/collaboration.state';
import { ConflictError } from '@/errors/response-errors/conflict.error';
import { LockedError } from '@/errors/response-errors/locked.error';
import { Push } from '@/push';
import type { OnPushMessage } from '@/push/types';
import { AccessService } from '@/services/access.service';
import { parseWorkflowMessage } from './collaboration.message';
import type {
WorkflowClosedMessage,
@@ -23,6 +16,13 @@ import type {
WriteAccessHeartbeatMessage,
} from './collaboration.message';
import { CollaborationState } from '@/collaboration/collaboration.state';
import { ConflictError } from '@/errors/response-errors/conflict.error';
import { LockedError } from '@/errors/response-errors/locked.error';
import { Push } from '@/push';
import type { OnPushMessage } from '@/push/types';
import { AccessService } from '@/services/access.service';
const OPEN_WORKFLOW_CHECK_BATCH_SIZE = 100;
/**
@@ -319,6 +319,26 @@ export class CollaborationService {
this.push.sendToUsers({ type: 'workflowSettingsUpdated', data: msgData }, userIds);
}
/**
* Invalidation-only: clients refetch the authoritative status. Delivery is best-effort and
* per-instance; cross-main viewers heal via focus/reconnect refetch. Review lifecycle write
* paths must call this after their transactions commit.
*/
async broadcastWorkflowReviewStateChanged(workflowId: Workflow['id']) {
const collaborators = await this.state.getCollaborators(workflowId);
const userIds = collaborators.map((user) => user.userId);
if (userIds.length === 0) {
return;
}
const msgData: PushPayload<'workflowReviewStateChanged'> = {
workflowId,
};
this.push.sendToUsers({ type: 'workflowReviewStateChanged', data: msgData }, userIds);
}
/**
* Exposes write-lock state to allow clients to restore read-only mode
* after page refresh, since write-lock is persisted in backend cache
@@ -1,4 +1,8 @@
import type { CreateWorkflowReviewRequestDto } from '@n8n/api-types';
import type {
CreateWorkflowReviewRequestDto,
ListWorkflowReviewRequestsQueryDto,
} from '@n8n/api-types';
import type { Logger } from '@n8n/backend-common';
import type {
DbLockService,
Project,
@@ -14,6 +18,7 @@ import { DbLock } from '@n8n/db';
import type { EntityManager } from '@n8n/typeorm';
import { mock } from 'vitest-mock-extended';
import type { CollaborationService } from '@/collaboration/collaboration.service';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { ConflictError } from '@/errors/response-errors/conflict.error';
import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
@@ -41,9 +46,12 @@ describe('WorkflowReviewRequestService', () => {
const workflowRepository = mock<WorkflowReviewRequestWorkflowRepository>();
const authorRepository = mock<WorkflowReviewRequestAuthorRepository>();
const dbLockService = mock<DbLockService>();
const collaborationService = mock<CollaborationService>();
const logger = mock<Logger>();
const tx = mock<EntityManager>();
const service = new WorkflowReviewRequestService(
logger,
workflowReviewPolicyService,
workflowFinderService,
workflowHistoryService,
@@ -52,6 +60,7 @@ describe('WorkflowReviewRequestService', () => {
workflowRepository,
authorRepository,
dbLockService,
collaborationService,
);
beforeEach(() => {
@@ -60,6 +69,7 @@ describe('WorkflowReviewRequestService', () => {
workflowReviewPolicyService.get.mockResolvedValue({ enabled: true });
// By default, run the critical section against the mocked transaction.
dbLockService.withLock.mockImplementation(async (_id, fn) => await fn(tx));
collaborationService.broadcastWorkflowReviewStateChanged.mockResolvedValue(undefined);
});
describe('create', () => {
@@ -82,7 +92,11 @@ describe('WorkflowReviewRequestService', () => {
);
requestRepository.findOpenRequestForWorkflow.mockResolvedValue(null);
requestRepository.createRequest.mockResolvedValue(
mock<WorkflowReviewRequest>({ id: 'req-1' }),
mock<WorkflowReviewRequest>({
id: 'req-1',
createdAt: new Date('2024-01-01T00:00:00.000Z'),
updatedAt: new Date('2024-01-01T00:00:00.000Z'),
}),
);
const result = await service.create(user, dto);
@@ -172,5 +186,143 @@ describe('WorkflowReviewRequestService', () => {
expect(workflowRepository.createWorkflowRow).not.toHaveBeenCalled();
expect(authorRepository.addAuthor).not.toHaveBeenCalled();
});
describe('review state broadcast', () => {
const mockSuccessfulCreatePath = () => {
workflowFinderService.findWorkflowForUser.mockResolvedValue(
mock<WorkflowEntity>({ isArchived: false }),
);
workflowHistoryService.findVersion.mockResolvedValue(mock());
sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue(
mock<Project>({ id: 'project-1' }),
);
requestRepository.findOpenRequestForWorkflow.mockResolvedValue(null);
requestRepository.createRequest.mockResolvedValue(
mock<WorkflowReviewRequest>({
id: 'req-1',
createdAt: new Date('2024-01-01T00:00:00.000Z'),
updatedAt: new Date('2024-01-01T00:00:00.000Z'),
}),
);
};
it('broadcasts exactly once after the lock resolves', async () => {
mockSuccessfulCreatePath();
let lockResolved = false;
dbLockService.withLock.mockImplementation(async (_id, fn) => {
const result = await fn(tx);
lockResolved = true;
return result;
});
collaborationService.broadcastWorkflowReviewStateChanged.mockImplementation(async () => {
expect(lockResolved).toBe(true);
});
await service.create(user, dto);
expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledTimes(1);
expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledWith(
'wf-1',
);
});
it('does not broadcast on conflict', async () => {
mockSuccessfulCreatePath();
requestRepository.findOpenRequestForWorkflow.mockResolvedValue(
mock<WorkflowReviewRequest>({ id: 'existing-1' }),
);
await expect(service.create(user, dto)).rejects.toThrow(ConflictError);
expect(collaborationService.broadcastWorkflowReviewStateChanged).not.toHaveBeenCalled();
});
it('resolves and logs a warning when the broadcast rejects', async () => {
mockSuccessfulCreatePath();
collaborationService.broadcastWorkflowReviewStateChanged.mockRejectedValue(
new Error('push down'),
);
const result = await service.create(user, dto);
expect(result.id).toBe('req-1');
// Let the fire-and-forget rejection handler run.
await new Promise(process.nextTick);
expect(logger.warn).toHaveBeenCalledWith(
'Failed to broadcast review state change',
expect.objectContaining({ workflowId: 'wf-1' }),
);
});
});
});
describe('list', () => {
const query = mock<ListWorkflowReviewRequestsQueryDto>({
workflowId: 'wf-1',
state: 'open',
skip: 0,
take: 1,
});
it('throws when the instance policy is disabled, before any lookup', async () => {
workflowReviewPolicyService.get.mockResolvedValue({ enabled: false });
await expect(service.list(user, query)).rejects.toThrow(ForbiddenError);
expect(workflowFinderService.findWorkflowForUser).not.toHaveBeenCalled();
expect(requestRepository.findRequestsForWorkflow).not.toHaveBeenCalled();
});
it('throws NotFoundError when the user has no read access to the workflow', async () => {
workflowFinderService.findWorkflowForUser.mockResolvedValue(null);
await expect(service.list(user, query)).rejects.toThrow(NotFoundError);
expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith('wf-1', user, [
'workflow:read',
]);
expect(requestRepository.findRequestsForWorkflow).not.toHaveBeenCalled();
});
it('passes state, skip, and take through to the repository', async () => {
workflowFinderService.findWorkflowForUser.mockResolvedValue(mock<WorkflowEntity>());
requestRepository.findRequestsForWorkflow.mockResolvedValue([[], 0]);
await service.list(user, query);
expect(requestRepository.findRequestsForWorkflow).toHaveBeenCalledWith('wf-1', {
state: 'open',
skip: 0,
take: 1,
});
});
it('maps rows to summaries and returns the total count', async () => {
workflowFinderService.findWorkflowForUser.mockResolvedValue(mock<WorkflowEntity>());
const request = mock<WorkflowReviewRequest>({
id: 'req-1',
state: 'open',
decision: 'pending',
title: 'Secret title',
createdAt: new Date('2026-07-20T10:00:00.000Z'),
updatedAt: new Date('2026-07-20T11:00:00.000Z'),
});
requestRepository.findRequestsForWorkflow.mockResolvedValue([[request], 3]);
const result = await service.list(user, query);
expect(result).toEqual({
count: 3,
data: [
{
id: 'req-1',
state: 'open',
decision: 'pending',
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T11:00:00.000Z',
},
],
});
});
});
});
@@ -14,13 +14,13 @@ import {
WorkflowReviewRequestWorkflowRepository,
} from '@n8n/db';
import { Container } from '@n8n/di';
import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service';
import { createMember, createOwner } 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 { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service';
const testServer = utils.setupTestServer({
endpointGroups: ['workflow-reviews'],
enabledFeatures: ['feat:workflowReviews'],
@@ -401,3 +401,147 @@ describe('POST /workflow-review-requests', () => {
testServer.license.enable('feat:workflowReviews');
});
});
describe('GET /workflow-review-requests', () => {
/** Link an existing review request to a workflow. */
async function linkRequestToWorkflow(requestId: string, workflowId: string, versionId: string) {
await workflowRepository.createWorkflowRow({
workflowReviewRequestId: requestId,
workflowId,
workflowVersionId: versionId,
});
}
test('returns 400 without a workflowId', async () => {
await ownerAgent.get('/workflow-review-requests').expect(400);
});
test('returns an empty list when no request exists', async () => {
const { workflow } = await createReviewableWorkflow();
const response = await ownerAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id, state: 'open', take: 1 })
.expect(200);
expect(response.body.data).toEqual({ count: 0, data: [] });
});
test('returns the open request as a minimal summary with state=open&take=1', async () => {
const { workflow, versionId } = await createReviewableWorkflow();
const request = await requestRepository.createRequest({
projectId: ownerProject.id,
title: 'Confidential title',
description: 'Confidential description',
createdById: owner.id,
});
await linkRequestToWorkflow(request.id, workflow.id, versionId);
await authorRepository.addAuthor({ workflowReviewRequestId: request.id, userId: owner.id });
const response = await ownerAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id, state: 'open', take: 1 })
.expect(200);
expect(response.body.data.count).toBe(1);
expect(response.body.data.data).toHaveLength(1);
expect(response.body.data.data[0]).toEqual({
id: request.id,
state: 'open',
decision: 'pending',
createdAt: expect.any(String),
updatedAt: expect.any(String),
});
});
test('excludes closed-only history with state=open, includes it without the filter', async () => {
const { workflow, versionId } = await createReviewableWorkflow();
const closed = await requestRepository.createRequest({
projectId: ownerProject.id,
state: 'closed',
title: 'Closed',
createdById: owner.id,
});
await linkRequestToWorkflow(closed.id, workflow.id, versionId);
const openResponse = await ownerAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id, state: 'open', take: 1 })
.expect(200);
expect(openResponse.body.data).toEqual({ count: 0, data: [] });
const allResponse = await ownerAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id })
.expect(200);
expect(allResponse.body.data.count).toBe(1);
expect(allResponse.body.data.data[0]).toMatchObject({ id: closed.id, state: 'closed' });
});
test('does not include requests of other workflows', async () => {
const { workflow } = await createReviewableWorkflow();
const { workflow: otherWorkflow } = await createReviewableWorkflow('version-other');
const request = await requestRepository.createRequest({
projectId: ownerProject.id,
title: 'For the other workflow',
createdById: owner.id,
});
await linkRequestToWorkflow(request.id, otherWorkflow.id, 'version-other');
const response = await ownerAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id })
.expect(200);
expect(response.body.data).toEqual({ count: 0, data: [] });
});
test('returns 404 when the member has no access to the workflow', async () => {
const { workflow } = await createReviewableWorkflow();
await memberAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id, state: 'open', take: 1 })
.expect(404);
});
test('allows a project:viewer (has workflow:read) to list requests', async () => {
const project = await createTeamProject('team', owner);
await linkUserToProject(member, project, 'project:viewer');
const workflow = await createWorkflow({}, project);
await createWorkflowHistoryItem(workflow.id, { versionId: 'version-1' });
const request = await requestRepository.createRequest({
projectId: project.id,
title: 'Open review',
createdById: owner.id,
});
await linkRequestToWorkflow(request.id, workflow.id, 'version-1');
const response = await memberAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id, state: 'open', take: 1 })
.expect(200);
expect(response.body.data.count).toBe(1);
expect(response.body.data.data[0].id).toBe(request.id);
});
test('returns 403 when the instance policy is disabled', async () => {
const { workflow } = await createReviewableWorkflow();
await policyService.set(false);
await ownerAgent
.get('/workflow-review-requests')
.query({ workflowId: workflow.id })
.expect(403);
});
test('returns 403 when the license lacks feat:workflowReviews', async () => {
testServer.license.disable('feat:workflowReviews');
await ownerAgent.get('/workflow-review-requests').query({ workflowId: 'wf-1' }).expect(403);
testServer.license.enable('feat:workflowReviews');
});
});
@@ -3,7 +3,6 @@
delete process.env.N8N_ENV_FEAT_WORKFLOW_REVIEWS;
import { testDb } from '@n8n/backend-test-utils';
import { createOwner } from '@test-integration/db/users';
import type { SuperAgentTest } from '@test-integration/types';
import * as utils from '@test-integration/utils';
@@ -33,3 +32,9 @@ describe('POST /workflow-review-requests (env flag off)', () => {
.expect(404);
});
});
describe('GET /workflow-review-requests (env flag off)', () => {
test('is unreachable (404) even though the license is present', async () => {
await ownerAgent.get('/workflow-review-requests').query({ workflowId: 'wf-1' }).expect(404);
});
});
@@ -1,4 +1,10 @@
import type { CreateWorkflowReviewRequestDto } from '@n8n/api-types';
import type {
CreateWorkflowReviewRequestDto,
ListWorkflowReviewRequestsQueryDto,
WorkflowReviewRequestList,
WorkflowReviewRequestSummary,
} from '@n8n/api-types';
import { Logger } from '@n8n/backend-common';
import {
DbLock,
DbLockService,
@@ -7,10 +13,10 @@ import {
WorkflowReviewRequestRepository,
WorkflowReviewRequestWorkflowRepository,
type User,
type WorkflowReviewRequest,
} from '@n8n/db';
import { Service } from '@n8n/di';
import { CollaborationService } from '@/collaboration/collaboration.service';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { ConflictError } from '@/errors/response-errors/conflict.error';
import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
@@ -22,6 +28,7 @@ import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-hi
@Service()
export class WorkflowReviewRequestService {
constructor(
private readonly logger: Logger,
private readonly workflowReviewPolicyService: WorkflowReviewPolicyService,
private readonly workflowFinderService: WorkflowFinderService,
private readonly workflowHistoryService: WorkflowHistoryService,
@@ -30,9 +37,46 @@ export class WorkflowReviewRequestService {
private readonly workflowReviewRequestWorkflowRepository: WorkflowReviewRequestWorkflowRepository,
private readonly workflowReviewRequestAuthorRepository: WorkflowReviewRequestAuthorRepository,
private readonly dbLockService: DbLockService,
private readonly collaborationService: CollaborationService,
) {}
async create(user: User, dto: CreateWorkflowReviewRequestDto): Promise<WorkflowReviewRequest> {
async list(
user: User,
query: ListWorkflowReviewRequestsQueryDto,
): Promise<WorkflowReviewRequestList> {
const policy = await this.workflowReviewPolicyService.get();
if (!policy.enabled) {
throw new ForbiddenError('Workflow reviews are not enabled for this instance');
}
const workflow = await this.workflowFinderService.findWorkflowForUser(query.workflowId, user, [
'workflow:read',
]);
if (!workflow) {
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 },
);
return {
count,
data: requests.map((request) => ({
id: request.id,
state: request.state,
decision: request.decision,
createdAt: request.createdAt.toISOString(),
updatedAt: request.updatedAt.toISOString(),
})),
};
}
async create(
user: User,
dto: CreateWorkflowReviewRequestDto,
): Promise<WorkflowReviewRequestSummary> {
const { workflowId, workflowVersionId } = dto.workflows[0];
const policy = await this.workflowReviewPolicyService.get();
@@ -65,44 +109,63 @@ export class WorkflowReviewRequestService {
throw new NotFoundError('Could not find workflow');
}
return await this.dbLockService.withLock(DbLock.WORKFLOW_REVIEW_REQUEST_CREATE, async (tx) => {
const existing = await this.workflowReviewRequestRepository.findOpenRequestForWorkflow(
workflowId,
tx,
);
if (existing) {
throw new ConflictError(
'An open review request already exists for this workflow',
'Sync the existing review request instead of creating a new one',
{ workflowReviewRequestId: existing.id },
);
}
const request = await this.workflowReviewRequestRepository.createRequest(
{
projectId: project.id,
title: dto.title,
description: dto.description ?? null,
createdById: user.id,
},
tx,
);
await this.workflowReviewRequestWorkflowRepository.createWorkflowRow(
{
workflowReviewRequestId: request.id,
const request = await this.dbLockService.withLock(
DbLock.WORKFLOW_REVIEW_REQUEST_CREATE,
async (tx) => {
const existing = await this.workflowReviewRequestRepository.findOpenRequestForWorkflow(
workflowId,
workflowVersionId,
},
tx,
tx,
);
if (existing) {
throw new ConflictError(
'An open review request already exists for this workflow',
'Sync the existing review request instead of creating a new one',
{ workflowReviewRequestId: existing.id },
);
}
const created = await this.workflowReviewRequestRepository.createRequest(
{
projectId: project.id,
title: dto.title,
description: dto.description ?? null,
createdById: user.id,
},
tx,
);
await this.workflowReviewRequestWorkflowRepository.createWorkflowRow(
{
workflowReviewRequestId: created.id,
workflowId,
workflowVersionId,
},
tx,
);
await this.workflowReviewRequestAuthorRepository.addAuthor(
{ workflowReviewRequestId: created.id, userId: user.id },
tx,
);
return created;
},
);
// Fire-and-forget: the transaction has committed, a failed broadcast
// must not fail the request. Viewers heal via focus/reconnect refetch.
this.collaborationService
.broadcastWorkflowReviewStateChanged(workflowId)
.catch((error) =>
this.logger.warn('Failed to broadcast review state change', { workflowId, error }),
);
await this.workflowReviewRequestAuthorRepository.addAuthor(
{ workflowReviewRequestId: request.id, userId: user.id },
tx,
);
return request;
});
return {
id: request.id,
state: request.state,
decision: request.decision,
createdAt: request.createdAt.toISOString(),
updatedAt: request.updatedAt.toISOString(),
};
}
}
@@ -1,6 +1,6 @@
import { CreateWorkflowReviewRequestDto } from '@n8n/api-types';
import { CreateWorkflowReviewRequestDto, ListWorkflowReviewRequestsQueryDto } from '@n8n/api-types';
import { AuthenticatedRequest } from '@n8n/db';
import { Body, Licensed, Post, RestController } from '@n8n/decorators';
import { Body, Get, Licensed, Post, Query, RestController } from '@n8n/decorators';
import { Response } from 'express';
import { WorkflowReviewRequestService } from './workflow-review-request.service';
@@ -9,6 +9,16 @@ import { WorkflowReviewRequestService } from './workflow-review-request.service'
export class WorkflowReviewRequestsController {
constructor(private readonly workflowReviewRequestService: WorkflowReviewRequestService) {}
@Get('/')
@Licensed('feat:workflowReviews')
async list(
req: AuthenticatedRequest,
_res: Response,
@Query query: ListWorkflowReviewRequestsQueryDto,
) {
return await this.workflowReviewRequestService.list(req.user, query);
}
@Post('/')
@Licensed('feat:workflowReviews')
async create(
@@ -6,6 +6,7 @@ import {
} from '@n8n/backend-test-utils';
import type { User } from '@n8n/db';
import { Container } from '@n8n/di';
import { createMember, createOwner } from '@test-integration/db/users';
import type { IWorkflowBase } from 'n8n-workflow';
import { mock } from 'vitest-mock-extended';
@@ -19,7 +20,6 @@ import { CollaborationService } from '@/collaboration/collaboration.service';
import { CollaborationState } from '@/collaboration/collaboration.state';
import { Push } from '@/push';
import { CacheService } from '@/services/cache/cache.service';
import { createMember, createOwner } from '@test-integration/db/users';
describe('CollaborationService', () => {
mockInstance(Push, new Push(mock(), mock(), mock(), mock(), mock()));
@@ -380,6 +380,35 @@ describe('CollaborationService', () => {
});
});
describe('broadcastWorkflowReviewStateChanged', () => {
it('should send the invalidation message to exactly the current collaborators', async () => {
const sendToUsersSpy = pushService.sendToUsers;
await sendWorkflowOpenedMessage(workflow.id, owner.id, 'owner-client-id');
await sendWorkflowOpenedMessage(workflow.id, memberWithAccess.id, 'member-client-id');
vi.mocked(sendToUsersSpy).mockClear();
await collaborationService.broadcastWorkflowReviewStateChanged(workflow.id);
expect(sendToUsersSpy).toHaveBeenCalledTimes(1);
expect(sendToUsersSpy).toHaveBeenCalledWith(
{
type: 'workflowReviewStateChanged',
data: { workflowId: workflow.id },
},
expect.arrayContaining([owner.id, memberWithAccess.id]),
);
expect(vi.mocked(sendToUsersSpy).mock.calls[0][1]).toHaveLength(2);
});
it('should not send anything when there are no collaborators', async () => {
const sendToUsersSpy = pushService.sendToUsers;
await collaborationService.broadcastWorkflowReviewStateChanged(workflow.id);
expect(sendToUsersSpy).not.toHaveBeenCalled();
});
});
describe('filterOpenWorkflowIds', () => {
it('should skip failed collaborator lookups and return resolved open workflows', async () => {
vi.spyOn(collaborationState, 'getCollaborators').mockImplementation(async (workflowId) => {
@@ -4433,6 +4433,7 @@
"workflowHistory.action.viewTimeline": "View timeline",
"workflowReviews.reviewRequired.title": "Review required",
"workflowReviews.reviewRequired.description": "Require changes to be reviewed and approved before publishing.",
"workflowReviews.reviewRequired.lockedDescription": "Review is required while this workflow has an open review.",
"workflowReviews.publishChoice.title": "New: Submit for review before publishing",
"workflowReviews.publishChoice.description": "You can now have your workflow version reviewed and approved before you publish it. Reviews are optional and you can still publish directly if you prefer.",
"workflowReviews.publishChoice.submitForReview": "Submit for review",
@@ -57,6 +57,7 @@ vi.mock('vue-router', async (importOriginal) => ({
vi.mock('@/app/stores/pushConnection.store', () => ({
usePushConnectionStore: vi.fn().mockReturnValue({
isConnected: true,
addEventListener: vi.fn().mockReturnValue(vi.fn()),
}),
}));
@@ -22,13 +22,17 @@ import {
} from '@/app/stores/workflowDocument.store';
import { useNodeTypesStore } from '@/app/stores/nodeTypes.store';
import { useReviewRequiredStore } from '@/features/workflow-reviews/reviewRequired.store';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
import {
LOCAL_STORAGE_WORKFLOW_REVIEW_PUBLISH_CHOICE_HIDDEN,
LOCAL_STORAGE_WORKFLOW_REVIEW_REQUIRED_BY_WORKFLOW,
LOCAL_STORAGE_WORKFLOW_REVIEW_SUBMITTED_DIALOG_HIDDEN,
} from '@/app/constants/localStorage';
import { useUsersStore } from '@/features/settings/users/users.store';
import { createWorkflowReviewRequest } from '@/features/workflow-reviews/workflowReviews.api';
import {
createWorkflowReviewRequest,
fetchWorkflowReviewRequests,
} from '@/features/workflow-reviews/workflowReviews.api';
vi.mock('vue-router', async (importOriginal) => ({
...(await importOriginal()),
@@ -76,6 +80,7 @@ vi.mock('@/app/composables/useWorkflowPublicationStatusSync', () => ({
vi.mock('@/features/workflow-reviews/workflowReviews.api', () => ({
createWorkflowReviewRequest: vi.fn(),
fetchWorkflowReviewRequests: vi.fn(),
}));
const initialState = {
@@ -195,6 +200,8 @@ describe('WorkflowHeaderDraftPublishActions', () => {
localStorage.removeItem(LOCAL_STORAGE_WORKFLOW_REVIEW_PUBLISH_CHOICE_HIDDEN('user-1'));
localStorage.removeItem(LOCAL_STORAGE_WORKFLOW_REVIEW_SUBMITTED_DIALOG_HIDDEN('user-1'));
useReviewRequiredStore().setReviewRequired(defaultWorkflowProps.id, false);
useWorkflowReviewStatusStore().clearStatus(defaultWorkflowProps.id);
vi.mocked(fetchWorkflowReviewRequests).mockResolvedValue({ count: 0, data: [] });
const nodeTypesStore = useNodeTypesStore();
nodeTypesStore.setNodeTypes([
@@ -218,6 +225,8 @@ describe('WorkflowHeaderDraftPublishActions', () => {
id: 'review-1',
state: 'open',
decision: 'pending',
createdAt: '2024-01-01T00:00:00.000Z',
updatedAt: '2024-01-01T00:00:00.000Z',
});
});
@@ -517,6 +526,37 @@ describe('WorkflowHeaderDraftPublishActions', () => {
).not.toBeInTheDocument();
});
it('opens submit for review directly for an open review even with the local preference off', async () => {
const openModalSpy = vi.spyOn(uiStore, 'openModalWithData');
setWorkflowReviewGates();
setupEnabledPublishButton();
expect(useReviewRequiredStore().isReviewRequired(defaultWorkflowProps.id)).toBe(false);
vi.mocked(fetchWorkflowReviewRequests).mockResolvedValue({
count: 1,
data: [
{
id: 'req-1',
state: 'open',
decision: 'pending',
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
},
],
});
const { getByTestId, findByRole, queryByRole } = renderComponent();
await waitFor(() =>
expect(useWorkflowReviewStatusStore().hasOpenReview(defaultWorkflowProps.id)).toBe(true),
);
await userEvent.click(getByTestId('workflow-open-publish-modal-button'));
expect(await findByRole('dialog', { name: 'Submit for review' })).toBeInTheDocument();
expect(
queryByRole('dialog', { name: 'New: Submit for review before publishing' }),
).not.toBeInTheDocument();
expect(openModalSpy).not.toHaveBeenCalled();
});
it('skips the review choice when the user dismissed it', async () => {
const openModalSpy = vi.spyOn(uiStore, 'openModalWithData');
setWorkflowReviewGates();
@@ -56,6 +56,8 @@ import WorkflowPublishChoiceDialog from '@/features/workflow-reviews/components/
import WorkflowSubmitForReviewDialog from '@/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue';
import WorkflowReviewSubmittedDialog from '@/features/workflow-reviews/components/WorkflowReviewSubmittedDialog.vue';
import { useReviewRequiredStore } from '@/features/workflow-reviews/reviewRequired.store';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
import { useWorkflowReviewStatusSync } from '@/features/workflow-reviews/composables/useWorkflowReviewStatusSync';
import { useWorkflowReviewDialogPreferences } from '@/features/workflow-reviews/composables/useWorkflowReviewDialogPreferences';
const props = defineProps<{
@@ -78,6 +80,7 @@ const workflowDocumentStore = computed(() =>
// Pass a getter so the composable re-syncs internally when the user navigates
// to a different workflow without this component being remounted.
useWorkflowPublicationStatusSync(() => workflowDocumentStore.value.documentId);
useWorkflowReviewStatusSync(() => (props.isNewWorkflow ? undefined : props.id));
const collaborationStore = useCollaborationStore();
const projectStore = useProjectsStore();
const workflowHistoryStore = useWorkflowHistoryStore();
@@ -91,8 +94,13 @@ const { saveCurrentWorkflow, cancelAutoSave } = useWorkflowSaving({ router });
const workflowActivate = useWorkflowActivate();
const { isWorkflowReviewsEnabled } = useWorkflowReviewsFeature();
const reviewRequiredStore = useReviewRequiredStore();
const reviewStatusStore = useWorkflowReviewStatusStore();
const { publishChoiceDismissed, submittedDialogDismissed } = useWorkflowReviewDialogPreferences();
const effectiveReviewRequired = computed(
() => reviewStatusStore.hasOpenReview(props.id) || reviewRequiredStore.isReviewRequired(props.id),
);
const isNamedVersionsEnabled = computed(
() => settingsStore.isEnterpriseFeatureEnabled[EnterpriseEditionFeature.NamedVersions],
);
@@ -259,7 +267,10 @@ const onPublishButtonClick = async () => {
if (!(await ensureWorkflowSaved())) return;
if (isWorkflowReviewsEnabled.value) {
if (reviewRequiredStore.isReviewRequired(props.id)) {
// TODO(LIGO-806): with an open review this submit dead-ends in the 409
// inline error until syncing the existing review exists
// TODO(LIGO-604): backend publish enforcement
if (effectiveReviewRequired.value) {
showSubmitForReviewDialog.value = true;
return;
}
@@ -3,11 +3,19 @@ import userEvent from '@testing-library/user-event';
import { defineComponent } from 'vue';
import { N8nDropdownMenu } from '@n8n/design-system';
import type { Pinia } from 'pinia';
import { createComponentRenderer } from '@/__tests__/render';
import { LOCAL_STORAGE_WORKFLOW_REVIEW_REQUIRED_BY_WORKFLOW } from '@/app/constants/localStorage';
import { useReviewRequiredStore } from '@/features/workflow-reviews/reviewRequired.store';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
import { fetchWorkflowReviewRequests } from '@/features/workflow-reviews/workflowReviews.api';
import WorkflowReviewRequiredToggle from './WorkflowReviewRequiredToggle.vue';
vi.mock('@/features/workflow-reviews/workflowReviews.api', () => ({
fetchWorkflowReviewRequests: vi.fn(),
}));
const TestHost = defineComponent({
components: { N8nDropdownMenu, WorkflowReviewRequiredToggle },
template: `
@@ -95,4 +103,64 @@ describe('WorkflowReviewRequiredToggle', () => {
expect(store.isReviewRequired('workflow-1')).toBe(true);
expect(store.isReviewRequired('workflow-2')).toBe(false);
});
describe('with an open review', () => {
// Seed through fetchStatus — the store exposes its state read-only.
const seedOpenReview = async (pinia: Pinia) => {
const openReview = {
id: 'req-1',
state: 'open',
decision: 'pending',
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
} as const;
vi.mocked(fetchWorkflowReviewRequests).mockResolvedValueOnce({
count: 1,
data: [openReview],
});
await useWorkflowReviewStatusStore(pinia).fetchStatus('workflow-1');
};
it('displays ON and disabled even when the local preference is off, with the locked description', async () => {
const pinia = createPinia();
await seedOpenReview(pinia);
const store = useReviewRequiredStore(pinia);
expect(store.isReviewRequired('workflow-1')).toBe(false);
const { getByRole, getByTestId, getByText } = renderComponent({ pinia });
await userEvent.click(getByRole('button'));
expect(getByRole('menuitemcheckbox', { name: /Review required/ })).toHaveAttribute(
'aria-checked',
'true',
);
expect(getByRole('menuitemcheckbox', { name: /Review required/ })).toHaveAttribute(
'aria-disabled',
'true',
);
expect(getByTestId('workflow-review-required-switch')).toHaveAttribute(
'data-state',
'checked',
);
expect(
getByText('Review is required while this workflow has an open review.'),
).toBeInTheDocument();
});
it('does not mutate the local preference when selected', async () => {
const pinia = createPinia();
await seedOpenReview(pinia);
const store = useReviewRequiredStore(pinia);
const { getByRole } = renderComponent({ pinia });
await userEvent.click(getByRole('button'));
await userEvent.click(getByRole('menuitemcheckbox', { name: /Review required/ }));
const reviewItem = getByRole('menuitemcheckbox', { name: /Review required/ });
reviewItem.focus();
await userEvent.keyboard('{Enter}');
expect(store.isReviewRequired('workflow-1')).toBe(false);
expect(reviewItem).toHaveAttribute('aria-checked', 'true');
});
});
});
@@ -4,6 +4,7 @@ import { N8nDropdownMenuItem, N8nSwitch, N8nText } from '@n8n/design-system';
import { useI18n } from '@n8n/i18n';
import { useReviewRequiredStore } from '@/features/workflow-reviews/reviewRequired.store';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
const props = defineProps<{
workflowId: string;
@@ -11,10 +12,16 @@ const props = defineProps<{
const i18n = useI18n();
const reviewRequiredStore = useReviewRequiredStore();
const reviewStatusStore = useWorkflowReviewStatusStore();
const hasOpenReview = computed(() => reviewStatusStore.hasOpenReview(props.workflowId));
const reviewRequired = computed({
get: () => reviewRequiredStore.isReviewRequired(props.workflowId),
set: (value: boolean) => reviewRequiredStore.setReviewRequired(props.workflowId, value),
get: () => hasOpenReview.value || reviewRequiredStore.isReviewRequired(props.workflowId),
set: (value: boolean) => {
if (hasOpenReview.value) return;
reviewRequiredStore.setReviewRequired(props.workflowId, value);
},
});
</script>
@@ -26,6 +33,7 @@ const reviewRequired = computed({
:checked="reviewRequired"
checkbox
:close-on-select="false"
:disabled="hasOpenReview"
divided
test-id="workflow-review-required-toggle"
@select="reviewRequired = !reviewRequired"
@@ -38,6 +46,7 @@ const reviewRequired = computed({
<template #item-trailing="{ ui }">
<N8nSwitch
:model-value="reviewRequired"
:disabled="hasOpenReview"
aria-hidden="true"
tabindex="-1"
data-test-id="workflow-review-required-switch"
@@ -46,7 +55,11 @@ const reviewRequired = computed({
</template>
</N8nDropdownMenuItem>
<N8nText tag="p" size="xsmall" color="text-base" :class="$style.description">
{{ i18n.baseText('workflowReviews.reviewRequired.description') }}
{{
hasOpenReview
? i18n.baseText('workflowReviews.reviewRequired.lockedDescription')
: i18n.baseText('workflowReviews.reviewRequired.description')
}}
</N8nText>
</div>
</template>
@@ -5,6 +5,7 @@ import { waitFor } from '@testing-library/vue';
import { createComponentRenderer } from '@/__tests__/render';
import { useReviewRequiredStore } from '@/features/workflow-reviews/reviewRequired.store';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
import { createWorkflowReviewRequest } from '@/features/workflow-reviews/workflowReviews.api';
import WorkflowSubmitForReviewDialog from './WorkflowSubmitForReviewDialog.vue';
@@ -16,6 +17,7 @@ vi.mock('@/app/composables/useToast', () => ({
vi.mock('@/features/workflow-reviews/workflowReviews.api', () => ({
createWorkflowReviewRequest: vi.fn(),
fetchWorkflowReviewRequests: vi.fn().mockResolvedValue({ count: 0, data: [] }),
}));
const renderComponent = createComponentRenderer(WorkflowSubmitForReviewDialog);
@@ -24,6 +26,8 @@ const renderDialog = async (flushSave = vi.fn().mockResolvedValue('version-1'))
const pinia = createPinia();
const reviewRequiredStore = useReviewRequiredStore(pinia);
reviewRequiredStore.setReviewRequired('workflow-1', true);
const reviewStatusStore = useWorkflowReviewStatusStore(pinia);
const fetchStatusSpy = vi.spyOn(reviewStatusStore, 'fetchStatus').mockResolvedValue(undefined);
const props = {
open: false,
workflowId: 'workflow-1',
@@ -36,6 +40,7 @@ const renderDialog = async (flushSave = vi.fn().mockResolvedValue('version-1'))
...result,
flushSave,
reviewRequiredStore,
fetchStatusSpy,
};
};
@@ -46,6 +51,8 @@ describe('WorkflowSubmitForReviewDialog', () => {
id: 'review-1',
state: 'open',
decision: 'pending',
createdAt: '2024-01-01T00:00:00.000Z',
updatedAt: '2024-01-01T00:00:00.000Z',
});
});
@@ -69,7 +76,8 @@ describe('WorkflowSubmitForReviewDialog', () => {
});
it('submits the flushed version and resets review required after success', async () => {
const { getByTestId, flushSave, reviewRequiredStore, emitted } = await renderDialog();
const { getByTestId, flushSave, reviewRequiredStore, fetchStatusSpy, emitted } =
await renderDialog();
await userEvent.type(getByTestId('workflow-review-title-input'), ' Review payments ');
await userEvent.type(getByTestId('workflow-review-description-input'), ' Check retries ');
@@ -84,6 +92,7 @@ describe('WorkflowSubmitForReviewDialog', () => {
});
expect(flushSave).toHaveBeenCalledOnce();
expect(reviewRequiredStore.isReviewRequired('workflow-1')).toBe(false);
expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1');
expect(emitted('submitted')).toHaveLength(1);
expect(emitted('update:open')).toContainEqual([false]);
});
@@ -95,7 +104,8 @@ describe('WorkflowSubmitForReviewDialog', () => {
meta: { workflowReviewRequestId: 'existing-review' },
}),
);
const { getByTestId, findByTestId, reviewRequiredStore, emitted } = await renderDialog();
const { getByTestId, findByTestId, reviewRequiredStore, fetchStatusSpy, emitted } =
await renderDialog();
await userEvent.type(getByTestId('workflow-review-title-input'), 'Review payments');
await userEvent.click(getByTestId('workflow-review-submit-button'));
@@ -103,6 +113,8 @@ describe('WorkflowSubmitForReviewDialog', () => {
expect(await findByTestId('workflow-review-conflict-error')).toHaveTextContent(
'This workflow already has an open review.',
);
// The conflict proves an open review — refetch so the toggle locks immediately.
expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1');
expect(reviewRequiredStore.isReviewRequired('workflow-1')).toBe(true);
expect(emitted('submitted')).toBeUndefined();
expect(emitted('update:open')).toBeUndefined();
@@ -14,6 +14,7 @@ import { computed, nextTick, ref, useTemplateRef, watch } from 'vue';
import { useToast } from '@/app/composables/useToast';
import { useReviewRequiredStore } from '@/features/workflow-reviews/reviewRequired.store';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
import { createWorkflowReviewRequest } from '@/features/workflow-reviews/workflowReviews.api';
const REVIEW_TITLE_MAX_LENGTH = 128;
@@ -34,6 +35,7 @@ const i18n = useI18n();
const rootStore = useRootStore();
const toast = useToast();
const reviewRequiredStore = useReviewRequiredStore();
const reviewStatusStore = useWorkflowReviewStatusStore();
const reviewTitle = ref('');
const description = ref('');
@@ -92,12 +94,14 @@ const submit = async () => {
workflows: [{ workflowId: props.workflowId, workflowVersionId }],
});
// TODO(LIGO-838): authoritative open-review server state takes over the displayed toggle
reviewRequiredStore.setReviewRequired(props.workflowId, false);
void reviewStatusStore.fetchStatus(props.workflowId);
emit('update:open', false);
emit('submitted');
} catch (error) {
if (error instanceof ResponseError && error.httpStatusCode === 409) {
// The conflict proves an open review this client didn't know about — lock immediately.
void reviewStatusStore.fetchStatus(props.workflowId);
hasConflict.value = true;
const workflowReviewRequestId = error.meta?.workflowReviewRequestId;
existingReviewRequestId.value =
@@ -0,0 +1,174 @@
import type { PushMessage } from '@n8n/api-types';
import { createTestingPinia } from '@pinia/testing';
import { setActivePinia } from 'pinia';
import { defineComponent, h, nextTick, ref } from 'vue';
import { renderComponent } from '@/__tests__/render';
import { mockedStore } from '@/__tests__/utils';
import { usePushConnectionStore } from '@/app/stores/pushConnection.store';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
import { useWorkflowReviewStatusSync } from './useWorkflowReviewStatusSync';
const isWorkflowReviewsEnabled = ref(true);
vi.mock('@/features/workflow-reviews/composables/useWorkflowReviewsFeature', () => ({
useWorkflowReviewsFeature: () => ({ isWorkflowReviewsEnabled }),
}));
const onDocumentVisibleHandlers: Array<() => void> = [];
vi.mock('@/app/composables/useDocumentVisibility', () => ({
useDocumentVisibility: () => ({
isVisible: { value: true },
onDocumentVisible: (handler: () => void) => onDocumentVisibleHandlers.push(handler),
onDocumentHidden: vi.fn(),
}),
}));
const collaboratorsChanged = (workflowId: string): PushMessage => ({
type: 'collaboratorsChanged',
data: { workflowId, collaborators: [] },
});
const reviewStateChanged = (workflowId: string): PushMessage => ({
type: 'workflowReviewStateChanged',
data: { workflowId },
});
describe('useWorkflowReviewStatusSync', () => {
let pushStore: ReturnType<typeof mockedStore<typeof usePushConnectionStore>>;
let reviewStatusStore: ReturnType<typeof mockedStore<typeof useWorkflowReviewStatusStore>>;
let pushHandlers: Array<(event: PushMessage) => void>;
let removePushListener: ReturnType<typeof vi.fn<() => void>>;
const emitPush = (event: PushMessage) => {
pushHandlers.forEach((handler) => handler(event));
};
function mountComposable(workflowId: Parameters<typeof useWorkflowReviewStatusSync>[0]) {
return renderComponent(
defineComponent({
setup() {
useWorkflowReviewStatusSync(workflowId);
return () => h('div');
},
}),
);
}
beforeEach(() => {
vi.clearAllMocks();
onDocumentVisibleHandlers.length = 0;
isWorkflowReviewsEnabled.value = true;
setActivePinia(createTestingPinia());
pushStore = mockedStore(usePushConnectionStore);
reviewStatusStore = mockedStore(useWorkflowReviewStatusStore);
pushStore.isConnected = true;
pushHandlers = [];
removePushListener = vi.fn<() => void>();
pushStore.addEventListener.mockImplementation((handler) => {
pushHandlers.push(handler);
// Like the real store, removing the listener stops delivery.
removePushListener.mockImplementation(() => {
pushHandlers = pushHandlers.filter((registered) => registered !== handler);
});
return removePushListener;
});
reviewStatusStore.fetchStatus.mockResolvedValue(undefined);
});
it('fetches on mount when the feature is enabled', async () => {
mountComposable(() => 'workflow-1');
await nextTick();
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledTimes(1);
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledWith('workflow-1');
});
it('does not fetch when the feature is disabled', async () => {
isWorkflowReviewsEnabled.value = false;
mountComposable(() => 'workflow-1');
await nextTick();
expect(reviewStatusStore.fetchStatus).not.toHaveBeenCalled();
});
it('does not fetch when the workflow id is undefined', async () => {
mountComposable(() => undefined);
await nextTick();
expect(reviewStatusStore.fetchStatus).not.toHaveBeenCalled();
});
it('refetches on a review state change for the current workflow, ignoring other workflows', async () => {
mountComposable(() => 'workflow-1');
await nextTick();
reviewStatusStore.fetchStatus.mockClear();
emitPush(reviewStateChanged('workflow-other'));
expect(reviewStatusStore.fetchStatus).not.toHaveBeenCalled();
emitPush(reviewStateChanged('workflow-1'));
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledTimes(1);
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledWith('workflow-1');
});
it('ignores push messages of unrelated types', async () => {
mountComposable(() => 'workflow-1');
await nextTick();
reviewStatusStore.fetchStatus.mockClear();
emitPush(collaboratorsChanged('workflow-1'));
expect(reviewStatusStore.fetchStatus).not.toHaveBeenCalled();
});
it('refetches when the push connection is restored, but not when it drops', async () => {
mountComposable(() => 'workflow-1');
await nextTick();
reviewStatusStore.fetchStatus.mockClear();
pushStore.isConnected = false;
await nextTick();
expect(reviewStatusStore.fetchStatus).not.toHaveBeenCalled();
pushStore.isConnected = true;
await nextTick();
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledTimes(1);
});
it('refetches for the new workflow when the active workflow id changes', async () => {
const workflowId = ref<string | undefined>('workflow-1');
mountComposable(() => workflowId.value);
await nextTick();
reviewStatusStore.fetchStatus.mockClear();
workflowId.value = 'workflow-2';
await nextTick();
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledTimes(1);
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledWith('workflow-2');
});
it('refetches when the browser tab becomes visible again', async () => {
mountComposable(() => 'workflow-1');
await nextTick();
reviewStatusStore.fetchStatus.mockClear();
onDocumentVisibleHandlers.forEach((handler) => handler());
expect(reviewStatusStore.fetchStatus).toHaveBeenCalledTimes(1);
});
it('removes the push listener and stops refetching after unmount', async () => {
const rendered = mountComposable(() => 'workflow-1');
await nextTick();
reviewStatusStore.fetchStatus.mockClear();
rendered.unmount();
expect(removePushListener).toHaveBeenCalledTimes(1);
emitPush(reviewStateChanged('workflow-1'));
expect(reviewStatusStore.fetchStatus).not.toHaveBeenCalled();
});
});
@@ -0,0 +1,60 @@
import type { PushMessage } from '@n8n/api-types';
import { onBeforeUnmount, onMounted, toValue, watch } from 'vue';
import type { MaybeRefOrGetter } from 'vue';
import { useDocumentVisibility } from '@/app/composables/useDocumentVisibility';
import { usePushConnectionStore } from '@/app/stores/pushConnection.store';
import { useWorkflowReviewsFeature } from '@/features/workflow-reviews/composables/useWorkflowReviewsFeature';
import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store';
/**
* Keeps the review-status store in sync with the backend for the given
* workflow. Push messages are treated purely as invalidation signals.
*/
export function useWorkflowReviewStatusSync(workflowId: MaybeRefOrGetter<string | undefined>) {
const pushStore = usePushConnectionStore();
const reviewStatusStore = useWorkflowReviewStatusStore();
const { isWorkflowReviewsEnabled } = useWorkflowReviewsFeature();
const { onDocumentVisible } = useDocumentVisibility();
async function refetch() {
// Re-check the feature gate on every call — it can be disabled mid-session.
if (!isWorkflowReviewsEnabled.value) return;
const id = toValue(workflowId);
if (!id) return;
await reviewStatusStore.fetchStatus(id);
}
function onPushMessage(event: PushMessage) {
if (
event.type === 'workflowReviewStateChanged' &&
event.data.workflowId === toValue(workflowId)
) {
void refetch();
}
}
const removePushListener = pushStore.addEventListener(onPushMessage);
// Re-sync when the user navigates to another workflow without a remount.
watch(
() => toValue(workflowId),
() => void refetch(),
);
// On reconnect, refetch to recover invalidations missed while offline.
watch(
() => pushStore.isConnected,
(isConnected, wasConnected) => {
if (isConnected && !wasConnected) void refetch();
},
);
onMounted(() => void refetch());
onDocumentVisible(() => void refetch());
onBeforeUnmount(() => removePushListener());
return { refetch };
}
@@ -0,0 +1,176 @@
import type { WorkflowReviewRequestList, WorkflowReviewRequestSummary } from '@n8n/api-types';
import { ResponseError } from '@n8n/rest-api-client';
import { createPinia, setActivePinia } from 'pinia';
import { fetchWorkflowReviewRequests } from '@/features/workflow-reviews/workflowReviews.api';
import { useWorkflowReviewStatusStore } from './reviewStatus.store';
vi.mock('@/features/workflow-reviews/workflowReviews.api', () => ({
fetchWorkflowReviewRequests: vi.fn(),
}));
const fetchMock = vi.mocked(fetchWorkflowReviewRequests);
const openReview: WorkflowReviewRequestSummary = {
id: 'req-1',
state: 'open',
decision: 'pending',
createdAt: '2026-07-20T10:00:00.000Z',
updatedAt: '2026-07-20T10:00:00.000Z',
};
const listOf = (...data: WorkflowReviewRequestSummary[]): WorkflowReviewRequestList => ({
count: data.length,
data,
});
describe('reviewStatus.store', () => {
beforeEach(() => {
vi.clearAllMocks();
setActivePinia(createPinia());
});
it('defaults to no open review before any fetch', () => {
const store = useWorkflowReviewStatusStore();
expect(store.hasOpenReview('workflow-1')).toBe(false);
expect(store.openReviewRequest('workflow-1')).toBeNull();
});
it('stores the open review returned by the API', async () => {
const store = useWorkflowReviewStatusStore();
fetchMock.mockResolvedValue(listOf(openReview));
await store.fetchStatus('workflow-1');
expect(fetchMock).toHaveBeenCalledWith(expect.anything(), {
workflowId: 'workflow-1',
state: 'open',
take: 1,
});
expect(store.hasOpenReview('workflow-1')).toBe(true);
expect(store.openReviewRequest('workflow-1')).toEqual(openReview);
});
it('stores null when the API returns no open review', async () => {
const store = useWorkflowReviewStatusStore();
fetchMock.mockResolvedValue(listOf());
await store.fetchStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(false);
expect(store.openReviewByWorkflowId).toHaveProperty('workflow-1', null);
});
it('discards an out-of-order response resolving after a newer one', async () => {
const store = useWorkflowReviewStatusStore();
let resolveFirst!: (value: WorkflowReviewRequestList) => void;
fetchMock.mockReturnValueOnce(
new Promise<WorkflowReviewRequestList>((resolve) => {
resolveFirst = resolve;
}),
);
const firstFetch = store.fetchStatus('workflow-1');
fetchMock.mockResolvedValueOnce(listOf(openReview));
await store.fetchStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(true);
// The stale first response resolves last and must be dropped.
resolveFirst(listOf());
await firstFetch;
expect(store.hasOpenReview('workflow-1')).toBe(true);
});
it('discards an older successful response even when a newer request failed transiently', async () => {
const store = useWorkflowReviewStatusStore();
let resolveFirst!: (value: WorkflowReviewRequestList) => void;
fetchMock.mockReturnValueOnce(
new Promise<WorkflowReviewRequestList>((resolve) => {
resolveFirst = resolve;
}),
);
const firstFetch = store.fetchStatus('workflow-1');
fetchMock.mockRejectedValueOnce(new Error('network down'));
await store.fetchStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(false);
// Latest-wins: only the most recent fetch may write, so the older
// success is dropped and the status stays unknown until the next sync.
resolveFirst(listOf(openReview));
await firstFetch;
expect(store.hasOpenReview('workflow-1')).toBe(false);
expect(store.openReviewByWorkflowId).not.toHaveProperty('workflow-1');
});
it('does not let an older success overwrite a newer 404 that cleared the status', async () => {
const store = useWorkflowReviewStatusStore();
let resolveFirst!: (value: WorkflowReviewRequestList) => void;
fetchMock.mockReturnValueOnce(
new Promise<WorkflowReviewRequestList>((resolve) => {
resolveFirst = resolve;
}),
);
const firstFetch = store.fetchStatus('workflow-1');
fetchMock.mockRejectedValueOnce(new ResponseError('gone', { httpStatusCode: 404 }));
await store.fetchStatus('workflow-1');
expect(store.openReviewByWorkflowId).not.toHaveProperty('workflow-1');
resolveFirst(listOf(openReview));
await firstFetch;
expect(store.hasOpenReview('workflow-1')).toBe(false);
expect(store.openReviewByWorkflowId).not.toHaveProperty('workflow-1');
});
it.each([404, 403])('clears the stored status on %i', async (httpStatusCode) => {
const store = useWorkflowReviewStatusStore();
fetchMock.mockResolvedValueOnce(listOf(openReview));
await store.fetchStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(true);
fetchMock.mockRejectedValueOnce(new ResponseError('gone', { httpStatusCode }));
await store.fetchStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(false);
expect(store.openReviewByWorkflowId).not.toHaveProperty('workflow-1');
});
it('keeps the last known status on a transient error', async () => {
const store = useWorkflowReviewStatusStore();
fetchMock.mockResolvedValueOnce(listOf(openReview));
await store.fetchStatus('workflow-1');
fetchMock.mockRejectedValueOnce(new Error('network down'));
await store.fetchStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(true);
});
it('keys statuses per workflow', async () => {
const store = useWorkflowReviewStatusStore();
fetchMock.mockResolvedValueOnce(listOf(openReview));
await store.fetchStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(true);
expect(store.hasOpenReview('workflow-2')).toBe(false);
});
it('clearStatus removes the stored entry', async () => {
const store = useWorkflowReviewStatusStore();
fetchMock.mockResolvedValueOnce(listOf(openReview));
await store.fetchStatus('workflow-1');
store.clearStatus('workflow-1');
expect(store.hasOpenReview('workflow-1')).toBe(false);
expect(store.openReviewByWorkflowId).not.toHaveProperty('workflow-1');
});
});
@@ -0,0 +1,74 @@
import type { WorkflowReviewRequestSummary } from '@n8n/api-types';
import { ResponseError } from '@n8n/rest-api-client';
import { useRootStore } from '@n8n/stores/useRootStore';
import { defineStore } from 'pinia';
import { readonly, ref } from 'vue';
import { fetchWorkflowReviewRequests } from '@/features/workflow-reviews/workflowReviews.api';
/**
* Authoritative open-review state per workflow.
* `null` means "fetched, no open review"; a missing key means "not fetched yet".
*/
export const useWorkflowReviewStatusStore = defineStore('workflowReviewStatus', () => {
const rootStore = useRootStore();
const openReviewByWorkflowId = ref<Record<string, WorkflowReviewRequestSummary | null>>({});
// Latest-wins: only the most recently started fetch may write its outcome.
const latestSequenceByWorkflowId: Record<string, number> = {};
const openReviewRequest = (workflowId: string): WorkflowReviewRequestSummary | null => {
return openReviewByWorkflowId.value[workflowId] ?? null;
};
/** The single client-side seam deriving "this workflow has an open review". */
const hasOpenReview = (workflowId: string): boolean => {
return openReviewRequest(workflowId) !== null;
};
/** True when a newer fetch has started since this one. */
const isStale = (workflowId: string, sequence: number): boolean =>
sequence !== latestSequenceByWorkflowId[workflowId];
const fetchStatus = async (workflowId: string): Promise<void> => {
const sequence = (latestSequenceByWorkflowId[workflowId] ?? 0) + 1;
latestSequenceByWorkflowId[workflowId] = sequence;
try {
const { data } = await fetchWorkflowReviewRequests(rootStore.restApiContext, {
workflowId,
state: 'open',
take: 1,
});
if (isStale(workflowId, sequence)) return;
openReviewByWorkflowId.value[workflowId] = data[0] ?? null;
} catch (error) {
if (isStale(workflowId, sequence)) return;
if (
error instanceof ResponseError &&
(error.httpStatusCode === 404 || error.httpStatusCode === 403)
) {
// Access or feature revoked — fall back to the local preference.
delete openReviewByWorkflowId.value[workflowId];
return;
}
// Transient error: keep the last known status. This is a background
// sync; unknown status degrades to local-pref behavior and the
// backend remains the real gate.
}
};
const clearStatus = (workflowId: string): void => {
delete openReviewByWorkflowId.value[workflowId];
};
return {
// all writes must go through fetchStatus/clearStatus so the
// sequence protocol stays the only write path.
openReviewByWorkflowId: readonly(openReviewByWorkflowId),
openReviewRequest,
hasOpenReview,
fetchStatus,
clearStatus,
};
});
@@ -1,3 +1,8 @@
import type {
WorkflowReviewRequestList,
WorkflowReviewRequestState,
WorkflowReviewRequestSummary,
} from '@n8n/api-types';
import { makeRestApiRequest, type IRestApiContext } from '@n8n/rest-api-client';
export interface CreateWorkflowReviewRequestPayload {
@@ -9,17 +14,23 @@ export interface CreateWorkflowReviewRequestPayload {
}>;
}
export interface WorkflowReviewRequest {
id: string;
state: 'open';
decision: 'pending';
export async function fetchWorkflowReviewRequests(
context: IRestApiContext,
query: { workflowId: string; state?: WorkflowReviewRequestState; take?: number; skip?: number },
): Promise<WorkflowReviewRequestList> {
return await makeRestApiRequest<WorkflowReviewRequestList>(
context,
'GET',
'/workflow-review-requests',
{ ...query },
);
}
export async function createWorkflowReviewRequest(
context: IRestApiContext,
payload: CreateWorkflowReviewRequestPayload,
): Promise<WorkflowReviewRequest> {
return await makeRestApiRequest<WorkflowReviewRequest>(
): Promise<WorkflowReviewRequestSummary> {
return await makeRestApiRequest<WorkflowReviewRequestSummary>(
context,
'POST',
'/workflow-review-requests',