diff --git a/AGENTS.md b/AGENTS.md index 3a28e0c06ce..e77f93d3236 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 3284cd93477..154b7dd1469 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -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'; diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/list-workflow-review-requests-query.dto.test.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/list-workflow-review-requests-query.dto.test.ts new file mode 100644 index 00000000000..bf55c698947 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/list-workflow-review-requests-query.dto.test.ts @@ -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); + } + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/list-workflow-review-requests-query.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/list-workflow-review-requests-query.dto.ts new file mode 100644 index 00000000000..9fa614bbda8 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/list-workflow-review-requests-query.dto.ts @@ -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(), +}) {} diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index eee5e1af7a2..ad1c50cbf5c 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -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, diff --git a/packages/@n8n/api-types/src/push/index.ts b/packages/@n8n/api-types/src/push/index.ts index 64fdce77db8..58deef2a06c 100644 --- a/packages/@n8n/api-types/src/push/index.ts +++ b/packages/@n8n/api-types/src/push/index.ts @@ -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']; diff --git a/packages/@n8n/api-types/src/push/workflow-review.ts b/packages/@n8n/api-types/src/push/workflow-review.ts new file mode 100644 index 00000000000..1ab4724ab58 --- /dev/null +++ b/packages/@n8n/api-types/src/push/workflow-review.ts @@ -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; diff --git a/packages/@n8n/api-types/src/workflow-review-request-summary.ts b/packages/@n8n/api-types/src/workflow-review-request-summary.ts new file mode 100644 index 00000000000..b73915feee3 --- /dev/null +++ b/packages/@n8n/api-types/src/workflow-review-request-summary.ts @@ -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; + +export const workflowReviewRequestDecisionSchema = z.enum([ + 'pending', + 'changes_requested', + 'approved', +]); +export type WorkflowReviewRequestDecision = z.infer; + +export type WorkflowReviewRequestSummary = { + id: string; + state: WorkflowReviewRequestState; + decision: WorkflowReviewRequestDecision; + createdAt: Iso8601DateTimeString; + updatedAt: Iso8601DateTimeString; +}; + +export type WorkflowReviewRequestList = { + count: number; + data: WorkflowReviewRequestSummary[]; +}; diff --git a/packages/@n8n/db/src/entities/workflow-review-request.ee.ts b/packages/@n8n/db/src/entities/workflow-review-request.ee.ts index 55dcb2d589e..764fb12c6f0 100644 --- a/packages/@n8n/db/src/entities/workflow-review-request.ee.ts +++ b/packages/@n8n/db/src/entities/workflow-review-request.ee.ts @@ -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; 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; 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; diff --git a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts index 773a5a4940e..f8d3eac05e0 100644 --- a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts @@ -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>; + + beforeEach(() => { + queryBuilder = mock>(); + 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({ 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); + }); + }); }); diff --git a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts index 47e0f6a0093..d1fcb78c868 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts @@ -49,6 +49,33 @@ export class WorkflowReviewRequestRepository extends Repository { + 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, diff --git a/packages/cli/src/collaboration/collaboration.service.ts b/packages/cli/src/collaboration/collaboration.service.ts index 34d8eee61a1..7985ccb14a2 100644 --- a/packages/cli/src/collaboration/collaboration.service.ts +++ b/packages/cli/src/collaboration/collaboration.service.ts @@ -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 diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts index aaf48924ad6..6553fa6f5e0 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts @@ -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(); const authorRepository = mock(); const dbLockService = mock(); + const collaborationService = mock(); + const logger = mock(); const tx = mock(); 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({ id: 'req-1' }), + mock({ + 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({ isArchived: false }), + ); + workflowHistoryService.findVersion.mockResolvedValue(mock()); + sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue( + mock({ id: 'project-1' }), + ); + requestRepository.findOpenRequestForWorkflow.mockResolvedValue(null); + requestRepository.createRequest.mockResolvedValue( + mock({ + 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({ 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({ + 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()); + 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()); + const request = mock({ + 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', + }, + ], + }); + }); }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts index de7772ead62..7a955ad9043 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts @@ -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'); + }); +}); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.env-gate.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.env-gate.integration.test.ts index 2d54467b558..455ffdac1ca 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.env-gate.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.env-gate.integration.test.ts @@ -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); + }); +}); diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts index c4fba5db37b..77a169d2d73 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts @@ -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 { + async list( + user: User, + query: ListWorkflowReviewRequestsQueryDto, + ): Promise { + 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 { 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(), + }; } } diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-requests.controller.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-requests.controller.ts index 42562d1c202..16aa7c3ded6 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-requests.controller.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-requests.controller.ts @@ -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( diff --git a/packages/cli/test/integration/collaboration/collaboration.service.test.ts b/packages/cli/test/integration/collaboration/collaboration.service.test.ts index be140316153..8bbb33bce9a 100644 --- a/packages/cli/test/integration/collaboration/collaboration.service.test.ts +++ b/packages/cli/test/integration/collaboration/collaboration.service.test.ts @@ -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) => { diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 9ab4b2e7e14..b10c5f4e82f 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -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", diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.test.ts b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.test.ts index b31ab1d0e90..ca56a0c61d7 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.test.ts +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.test.ts @@ -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()), }), })); diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts index dcf921653eb..9a7b231cec7 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts @@ -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(); diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue index a6490e2c86d..b60d6eb5680 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue @@ -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; } diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts index 62c7835bad8..736cba94e99 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts @@ -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'); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.vue index ed2cdf3838b..38a39fcf21e 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.vue @@ -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); + }, }); @@ -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({ diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.test.ts index e9586f882a9..f76ab39ecc6 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.test.ts @@ -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(); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue index 1b6e893c3ae..9b793df5895 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue @@ -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 = diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/composables/useWorkflowReviewStatusSync.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/composables/useWorkflowReviewStatusSync.test.ts new file mode 100644 index 00000000000..b249cff9bd7 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/composables/useWorkflowReviewStatusSync.test.ts @@ -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>; + let reviewStatusStore: ReturnType>; + let pushHandlers: Array<(event: PushMessage) => void>; + let removePushListener: ReturnType void>>; + + const emitPush = (event: PushMessage) => { + pushHandlers.forEach((handler) => handler(event)); + }; + + function mountComposable(workflowId: Parameters[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('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(); + }); +}); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/composables/useWorkflowReviewStatusSync.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/composables/useWorkflowReviewStatusSync.ts new file mode 100644 index 00000000000..07ec253865e --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/composables/useWorkflowReviewStatusSync.ts @@ -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) { + 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 }; +} diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewStatus.store.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewStatus.store.test.ts new file mode 100644 index 00000000000..cfeb0b0bcbc --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewStatus.store.test.ts @@ -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((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((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((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'); + }); +}); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewStatus.store.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewStatus.store.ts new file mode 100644 index 00000000000..9aa6dc5953d --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewStatus.store.ts @@ -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>({}); + // Latest-wins: only the most recently started fetch may write its outcome. + const latestSequenceByWorkflowId: Record = {}; + + 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 => { + 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, + }; +}); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/workflowReviews.api.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/workflowReviews.api.ts index 3ec89c7873e..f37592db3a2 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/workflowReviews.api.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/workflowReviews.api.ts @@ -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 { + return await makeRestApiRequest( + context, + 'GET', + '/workflow-review-requests', + { ...query }, + ); } export async function createWorkflowReviewRequest( context: IRestApiContext, payload: CreateWorkflowReviewRequestPayload, -): Promise { - return await makeRestApiRequest( +): Promise { + return await makeRestApiRequest( context, 'POST', '/workflow-review-requests',