diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 85a7c1681c0..06e635c9d2c 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -332,6 +332,8 @@ export * from './workflow-reviews/list-workflow-review-inbox.dto'; export type * from './workflow-reviews/get-workflow-review-request-detail.dto'; export { ListWorkflowReviewActivityQueryDto, + CreateWorkflowReviewCommentDto, + WORKFLOW_REVIEW_COMMENT_MAX_LENGTH, type ListWorkflowReviewActivityResponse, } from './workflow-reviews/workflow-review-activity.dto'; diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts index 49508f154f5..203102dd3df 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts @@ -47,4 +47,6 @@ export interface WorkflowReviewRequestDetail extends WorkflowReviewInboxItem { viewerCanDecide: boolean; /** Set if `viewerCanDecide` is false. */ viewerDecisionIneligibilityReason: WorkflowReviewDecisionIneligibilityReason | null; + /** Not advisory, unlike `viewerCanDecide`: the comment endpoint applies the same verdict. */ + viewerCanComment: boolean; } diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts index 458991fded0..d6bf8c4748b 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts @@ -17,3 +17,18 @@ export interface ListWorkflowReviewActivityResponse { /** Whether *older* entries exist. */ hasMore: boolean; } + +export const WORKFLOW_REVIEW_COMMENT_MAX_LENGTH = 10_000; + +export class CreateWorkflowReviewCommentDto extends Z.class({ + body: z + .string() + .trim() + .min(1) + .max(WORKFLOW_REVIEW_COMMENT_MAX_LENGTH) + // NUL cannot be stored in a Postgres text column at all, so it would turn user input + // into a 500. The rest of C0 is non-printing junk with no place in a comment body, and + // is rejected in the same pass. \n, \r and \t are deliberately allowed through. + // eslint-disable-next-line no-control-regex + .refine((v) => !/[\x00-\x08\x0B\x0C\x0E-\x1F]/.test(v), 'Body contains control characters'), +}) {} diff --git a/packages/@n8n/db/src/repositories/index.ts b/packages/@n8n/db/src/repositories/index.ts index fb2d4748708..8f30aa64136 100644 --- a/packages/@n8n/db/src/repositories/index.ts +++ b/packages/@n8n/db/src/repositories/index.ts @@ -93,6 +93,7 @@ export { WorkflowReviewActivityRepository, type WorkflowReviewActivityFeedEntry, } from './workflow-review-activity.repository'; +export { WorkflowReviewActivityCommentRepository } from './workflow-review-activity-comment.repository'; export { WorkflowDependencyRepository, WorkflowDependencies, diff --git a/packages/@n8n/db/src/repositories/workflow-review-activity-comment.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-activity-comment.repository.ts new file mode 100644 index 00000000000..a2b804f9040 --- /dev/null +++ b/packages/@n8n/db/src/repositories/workflow-review-activity-comment.repository.ts @@ -0,0 +1,25 @@ +import { Service } from '@n8n/di'; +import { DataSource } from '@n8n/typeorm'; + +import { BaseRepository } from './base-repository'; +import { WorkflowReviewActivityComment } from '../entities/workflow-review-activity-comment.ee'; +import { type OperationContext, TransactionRunner } from '../services/transaction'; + +@Service() +export class WorkflowReviewActivityCommentRepository extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(WorkflowReviewActivityComment, dataSource.manager, transactionRunner); + } + + async createComment( + input: { + activityId: number; + createdById: string | null; + body: string; + }, + ctx: OperationContext, + ): Promise { + const entity = this.create(input); + return await this.managerFor(ctx).save(WorkflowReviewActivityComment, entity); + } +} diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts index f3b7711b691..8e9d681d1d0 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts @@ -9,6 +9,9 @@ import { } from '@n8n/backend-test-utils'; import type { Project, User } from '@n8n/db'; import { + UserRepository, + WorkflowRepository, + WorkflowReviewActivityCommentRepository, WorkflowReviewActivityRepository, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, @@ -45,6 +48,9 @@ let requestRepository: WorkflowReviewRequestRepository; let workflowRepository: WorkflowReviewRequestWorkflowRepository; let authorRepository: WorkflowReviewRequestAuthorRepository; let activityRepository: WorkflowReviewActivityRepository; +let activityCommentRepository: WorkflowReviewActivityCommentRepository; +let userRepository: UserRepository; +let workflowEntityRepository: WorkflowRepository; let policyService: WorkflowReviewPolicyService; beforeAll(async () => { @@ -53,6 +59,9 @@ beforeAll(async () => { workflowRepository = Container.get(WorkflowReviewRequestWorkflowRepository); authorRepository = Container.get(WorkflowReviewRequestAuthorRepository); activityRepository = Container.get(WorkflowReviewActivityRepository); + activityCommentRepository = Container.get(WorkflowReviewActivityCommentRepository); + userRepository = Container.get(UserRepository); + workflowEntityRepository = Container.get(WorkflowRepository); policyService = Container.get(WorkflowReviewPolicyService); }); @@ -149,6 +158,204 @@ async function getActivity(agent: SuperAgentTest, requestId: string, limit?: num }; } +describe('Commenting on a review', () => { + test('shows a comment in the feed the instant its writer posts it', async () => { + const { request } = await seedReviewInTeamProject(member); + + const post = await memberAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Looks good to me' }) + .expect(201); + + const detail = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(detail.body.data.viewerCanComment).toBe(true); + expect(detail.body.data.viewerCanDecide).toBe(false); + expect(detail.body.data.viewerDecisionIneligibilityReason).toBe('author'); + + // POST and GET must agree, or a comment visibly changes on reload + const feed = await getActivity(memberAgent, request.id); + expect(typeof post.body.data.id).toBe('string'); + expect(post.body.data).toEqual(feed.data[0]); + expect(feed.data[0]).toMatchObject({ + type: 'comment.created', + typeVersion: 1, + data: null, + createdBy: expect.objectContaining({ id: member.id }), + messages: [expect.objectContaining({ body: 'Looks good to me' })], + }); + }); + + test('lets a reviewer who can approve the review comment on it', async () => { + const { request } = await seedReviewInTeamProject(owner); + + await memberAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'One question' }) + .expect(201); + + const detail = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(detail.body.data.viewerCanComment).toBe(true); + }); + + test('lets a reader who cannot approve read the feed but not post to it', async () => { + // The review lives in teamProject (where member may publish) while the workflow + // moved to a project member can only read. + const destinationProject = await createTeamProject('Destination Project', owner); + await linkUserToProject(member, destinationProject, 'project:viewer'); + const workflow = await createWorkflow({}, destinationProject); + await createWorkflowHistoryItem(workflow.id, { versionId: 'version-pinned' }); + const request = await seedRequest(workflow.id, 'version-pinned', owner); + + await getActivity(memberAgent, request.id); + const detail = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(detail.body.data.viewerCanComment).toBe(false); + + await memberAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Can I?' }) + .expect(403); + }); + + test('refuses a requester who can no longer read the workflow under review', async () => { + const destinationProject = await createTeamProject('Out Of Reach', owner); + const workflow = await createWorkflow({}, destinationProject); + await createWorkflowHistoryItem(workflow.id, { versionId: 'version-pinned' }); + const request = await seedRequest(workflow.id, 'version-pinned', viewer); + + // The requester still reads their own review, but that alone is not a write right + const detail = await viewerAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(detail.body.data.viewerCanComment).toBe(false); + + await viewerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Still here' }) + .expect(403); + }); + + test('lets a requester downgraded to view-only keep commenting on their own review', async () => { + // Opening a review needs workflow:publish, so this state is only reachable by a + // downgrade. project:viewer keeps workflow:read, and read is the scope that gates + // commenting, so this must pass while deciding does not. + const { request } = await seedReviewInTeamProject(viewer); + + const detail = await viewerAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(detail.body.data.viewerCanComment).toBe(true); + expect(detail.body.data.viewerCanDecide).toBe(false); + expect(detail.body.data.viewerDecisionIneligibilityReason).toBe('missing_publish_permission'); + + await viewerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'From the author' }) + .expect(201); + }); + + test('refuses everyone once the reviewed workflow is deleted, but keeps the feed readable', async () => { + const { workflow, request } = await seedReviewInTeamProject(owner); + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Before the deletion' }) + .expect(201); + + // The linked-workflow row cascades away with the workflow; nothing closes the review + await workflowEntityRepository.delete({ id: workflow.id }); + + for (const agent of [ownerAgent, memberAgent]) { + const feed = await getActivity(agent, request.id); + expect(feed.data).toHaveLength(1); + + const detail = await agent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(detail.body.data.viewerCanComment).toBe(false); + + await agent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Anyone there?' }) + .expect(403); + } + }); + + test('hides the feed entirely from someone without access to the review', async () => { + const { request } = await seedReviewInTeamProject(owner); + + // 404 rather than 403 on both, matching getDetail's don't-confirm-existence policy + await viewerAgent.get(`/workflow-review-requests/${request.id}/activity`).expect(404); + await viewerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Hello?' }) + .expect(404); + }); + + test.each(['approved', 'closed'] as const)( + 'keeps existing comments visible and still accepts new ones on a %s review', + async (settled) => { + const { request } = await seedReviewInTeamProject(owner); + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Before it settled' }) + .expect(201); + await requestRepository.update( + request.id, + settled === 'approved' ? { decision: 'approved' } : { state: 'closed' }, + ); + + const feed = await getActivity(ownerAgent, request.id); + expect(feed.data).toHaveLength(1); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'After it settled' }) + .expect(201); + }, + ); + + test('leaves no empty comment behind when the write fails halfway', async () => { + const { request } = await seedReviewInTeamProject(owner); + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'The one that sticks' }) + .expect(201); + + const createComment = vi + .spyOn(activityCommentRepository, 'createComment') + .mockRejectedValueOnce(new Error('write failed')); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'The one that rolls back' }) + .expect(500); + + expect(createComment).toHaveBeenCalled(); + expect(await activityRepository.count()).toBe(1); + expect(await activityCommentRepository.count()).toBe(1); + }); + + test.each([ + ['rejects an empty comment', '', 400], + ['rejects a comment that is only whitespace', ' \n ', 400], + ['accepts a comment at the length limit', 'x'.repeat(10_000), 201], + ['rejects a comment over the length limit', 'x'.repeat(10_001), 400], + // A C0 control character reaches the Postgres driver as a 500 unless rejected here + ['rejects a comment containing a control character', 'oops \x00 here', 400], + ])('%s', async (_label, body, status) => { + const { request } = await seedReviewInTeamProject(owner); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body }) + .expect(status); + }); + + test('trims the body it stores and keeps newlines intact', async () => { + const { request } = await seedReviewInTeamProject(owner); + + const response = await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: ' first line\nsecond line ' }) + .expect(201); + + expect(response.body.data.messages[0].body).toBe('first line\nsecond line'); + }); +}); + describe('Reading the activity feed', () => { /** Non-comment entries, cheap to seed and enough to pin the paging arithmetic. */ async function seedEntries(workflowReviewRequestId: string, count: number) { @@ -222,11 +429,64 @@ describe('Reading the activity feed', () => { expect(secondPage.body.data.nextCursor).toBeNull(); }); - test('hides the feed entirely from someone without access to the review', async () => { + test('does not shift the older pages when someone comments while you scroll back', async () => { const { request } = await seedReviewInTeamProject(owner); + const ids = await seedEntries(request.id, 4); - // 404 rather than 403, matching getDetail's don't-confirm-existence policy - await viewerAgent.get(`/workflow-review-requests/${request.id}/activity`).expect(404); + const firstPage = await ownerAgent + .get(`/workflow-review-requests/${request.id}/activity`) + .query({ limit: 2 }) + .expect(200); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Arrived between pages' }) + .expect(201); + + const secondPage = await ownerAgent + .get(`/workflow-review-requests/${request.id}/activity`) + .query({ limit: 2, cursor: firstPage.body.data.nextCursor }) + .expect(200); + + expect(firstPage.body.data.data.map((entry: { id: string }) => entry.id)).toEqual([ + ids[2], + ids[3], + ]); + expect(secondPage.body.data.data.map((entry: { id: string }) => entry.id)).toEqual([ + ids[0], + ids[1], + ]); + }); + + test("never shows one review's comments on another review", async () => { + const first = await seedReviewInTeamProject(owner); + const second = await seedReviewInTeamProject(owner); + const firstIds = await seedEntries(first.request.id, 1); + const secondIds = await seedEntries(second.request.id, 1); + firstIds.push(...(await seedEntries(first.request.id, 1))); + // A comment in each pins that messages land on their own thread when several reviews + // hold messages. + async function comment(requestId: string, body: string) { + const response = await ownerAgent + .post(`/workflow-review-requests/${requestId}/comments`) + .send({ body }) + .expect(201); + return response.body.data.id as string; + } + firstIds.push(await comment(first.request.id, 'First review comment')); + secondIds.push(await comment(second.request.id, 'Second review comment')); + + const firstFeed = await getActivity(ownerAgent, first.request.id); + const secondFeed = await getActivity(ownerAgent, second.request.id); + + expect(firstFeed.data.map((e) => e.id)).toEqual(firstIds); + expect(secondFeed.data.map((e) => e.id)).toEqual(secondIds); + expect(firstFeed.data.at(-1)?.messages).toEqual([ + expect.objectContaining({ body: 'First review comment' }), + ]); + expect(secondFeed.data.at(-1)?.messages).toEqual([ + expect.objectContaining({ body: 'Second review comment' }), + ]); }); // The owner reaches every review through global `workflow:publish`, which short-circuits the @@ -252,15 +512,84 @@ describe('Reading the activity feed', () => { }, {}, ); + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'On it' }) + .expect(201); - const feed = await getActivity(ownerAgent, request.id); + const feed = await getActivity(ownerAgent, request.id, 2); - expect(feed.data).toHaveLength(1); + expect(feed.hasMore).toBe(false); + expect(feed.data).toHaveLength(2); expect(feed.data[0]).toMatchObject({ type: 'review.changes_requested' }); // `toEqual`, not a partial match: a mapper that renamed or added a key inside `data` // would still pass `toMatchObject`. expect(feed.data[0].data).toEqual(data); expect(feed.data[0]).not.toHaveProperty('messages'); + expect(feed.data[1]).toMatchObject({ type: 'comment.created' }); + }); + + test('keeps each comment with its own body and author', async () => { + const { request } = await seedReviewInTeamProject(owner); + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'From the owner' }) + .expect(201); + await memberAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'From the member' }) + .expect(201); + + const feed = await getActivity(ownerAgent, request.id); + + // A grouping bug that attaches one thread's messages to every entry renders every + // comment with the same body, so both entries are asserted individually. + expect(feed.data).toHaveLength(2); + expect(feed.data[0].messages).toEqual([ + expect.objectContaining({ + body: 'From the owner', + createdBy: expect.objectContaining({ id: owner.id }), + }), + ]); + expect(feed.data[1].messages).toEqual([ + expect.objectContaining({ + body: 'From the member', + createdBy: expect.objectContaining({ id: member.id }), + }), + ]); + }); + + test('keeps a comment readable after its author is deleted from the instance', async () => { + const { request } = await seedReviewInTeamProject(owner); + await memberAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Written before leaving' }) + .expect(201); + + await userRepository.delete({ id: member.id }); + + const [entry] = (await getActivity(ownerAgent, request.id)).data; + // `undefined` would be dropped from the JSON; the client checks for `null` + expect('createdBy' in entry).toBe(true); + expect(entry.createdBy).toBeNull(); + expect(entry.messages?.[0].createdBy).toBeNull(); + expect(entry.messages?.[0].body).toBe('Written before leaving'); + }); + + test('hides the text of a deleted comment but keeps its place in the feed', async () => { + const { request } = await seedReviewInTeamProject(owner); + const post = await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Written then deleted' }) + .expect(201); + await activityCommentRepository.update(Number(post.body.data.messages[0].id), { + deletedAt: new Date(), + }); + + const [entry] = (await getActivity(ownerAgent, request.id)).data; + + expect(entry.messages?.[0].body).toBeNull(); + expect(entry.messages?.[0].deletedAt).not.toBeNull(); }); test.each([ @@ -283,4 +612,14 @@ describe('Reading the activity feed', () => { await ownerAgent.get(`/workflow-review-requests/${request.id}/activity`).expect(403); }); + + test('refuses to post a comment when an admin has turned reviews off', async () => { + const { request } = await seedReviewInTeamProject(owner); + await policyService.set(false); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/comments`) + .send({ body: 'Blocked' }) + .expect(403); + }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts index e5d7c52d9c3..82fa45660dc 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts @@ -30,6 +30,15 @@ describe('WorkflowReviewEligibilityService', () => { const request = () => mock({ id: requestId, projectId }); + const readable = ( + overrides: Partial[1]> = {}, + ) => ({ + request: request(), + pinnedWorkflowId: workflowId, + canReadPinnedWorkflow: true, + ...overrides, + }); + beforeEach(() => { vi.resetAllMocks(); workflowFinderService.findWorkflowForUser.mockResolvedValue(mock()); @@ -37,15 +46,15 @@ describe('WorkflowReviewEligibilityService', () => { projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); }); - describe('resolveViewerEligibility', () => { + describe('who may decide', () => { it('lets a non-author with publish access decide', async () => { - const eligibility = await service.resolveViewerEligibility( - memberUser(), - request(), - workflowId, - ); + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); - expect(eligibility).toEqual({ canDecide: true, decisionIneligibilityReason: null }); + expect(eligibility).toEqual({ + canDecide: true, + decisionIneligibilityReason: null, + canComment: true, + }); expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith( workflowId, expect.anything(), @@ -53,27 +62,31 @@ describe('WorkflowReviewEligibilityService', () => { ); }); - it('reports an author without an admin override as ineligible', async () => { + it('stops an author from approving their own review', async () => { authorRepository.isAuthor.mockResolvedValue(true); - const eligibility = await service.resolveViewerEligibility( - memberUser(), - request(), - workflowId, - ); + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); - expect(eligibility).toEqual({ canDecide: false, decisionIneligibilityReason: 'author' }); + expect(eligibility).toEqual({ + canDecide: false, + decisionIneligibilityReason: 'author', + canComment: true, + }); }); it.each([['global:admin'], ['global:owner']])( - 'lets an author with the %s role decide without querying project relations', + 'lets an instance %s decide on a review they authored', async (slug) => { authorRepository.isAuthor.mockResolvedValue(true); const admin = mock({ id: 'user-1', role: { slug } }); - const eligibility = await service.resolveViewerEligibility(admin, request(), workflowId); + const eligibility = await service.resolveViewerEligibility(admin, readable()); - expect(eligibility).toEqual({ canDecide: true, decisionIneligibilityReason: null }); + expect(eligibility).toEqual({ + canDecide: true, + decisionIneligibilityReason: null, + canComment: true, + }); expect(projectRelationRepository.getAccessibleProjectsByRoles).not.toHaveBeenCalled(); }, ); @@ -82,81 +95,118 @@ describe('WorkflowReviewEligibilityService', () => { authorRepository.isAuthor.mockResolvedValue(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([projectId]); - const eligibility = await service.resolveViewerEligibility( - memberUser(), - request(), - workflowId, - ); + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); - expect(eligibility).toEqual({ canDecide: true, decisionIneligibilityReason: null }); + expect(eligibility).toEqual({ + canDecide: true, + decisionIneligibilityReason: null, + canComment: true, + }); }); - it('reports an author who is only a project admin elsewhere as ineligible', async () => { + it('still stops an author whose project-admin rights are in another project', async () => { authorRepository.isAuthor.mockResolvedValue(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue(['other-proj']); - const eligibility = await service.resolveViewerEligibility( - memberUser(), - request(), - workflowId, - ); + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); - expect(eligibility).toEqual({ canDecide: false, decisionIneligibilityReason: 'author' }); + expect(eligibility).toEqual({ + canDecide: false, + decisionIneligibilityReason: 'author', + canComment: true, + }); }); - it('skips the roles query entirely for a non-author', async () => { - await service.resolveViewerEligibility(memberUser(), request(), workflowId); + it('does not look up project roles for someone who is not an author', async () => { + await service.resolveViewerEligibility(memberUser(), readable()); expect(projectRelationRepository.getAccessibleProjectsByRoles).not.toHaveBeenCalled(); }); - it('reports missing publish access before authorship, matching the decision endpoint order', async () => { + it('tells an author without publish rights about the permission, not about their authorship', async () => { // An author without publish access would hit the endpoint's 404 first, // so the surfaced reason must be the permission one, not 'author'. workflowFinderService.findWorkflowForUser.mockResolvedValue(null); authorRepository.isAuthor.mockResolvedValue(true); - const eligibility = await service.resolveViewerEligibility( - memberUser(), - request(), - workflowId, - ); + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); + // Authorship still resolves — it feeds `canComment`, which survives the missing + // publish right as long as the author can read the pinned workflow. expect(eligibility).toEqual({ canDecide: false, decisionIneligibilityReason: 'missing_publish_permission', + canComment: true, }); - expect(authorRepository.isAuthor).not.toHaveBeenCalled(); }); - // The capability answers "who", not "when": a closed request still reports the + // The capability answers "who", not "when": a closed review still reports the // viewer's own eligibility honestly, and callers gate on state separately. it.each([ - ['a closed request', { state: 'closed' as const }], - ['an approved request', { decision: 'approved' as const }], - ])('still reports viewer eligibility for %s', async (_label, overrides) => { + ['a closed review', { state: 'closed' as const }], + ['an approved review', { decision: 'approved' as const }], + ])('still says who may act on %s', async (_label, overrides) => { const eligibility = await service.resolveViewerEligibility( memberUser(), - mock({ id: requestId, projectId, ...overrides }), - workflowId, + readable({ + request: mock({ id: requestId, projectId, ...overrides }), + }), ); - expect(eligibility).toEqual({ canDecide: true, decisionIneligibilityReason: null }); + expect(eligibility).toEqual({ + canDecide: true, + decisionIneligibilityReason: null, + canComment: true, + }); }); + }); - it('reports a review with no linked workflow as ineligible without any lookup', async () => { - const eligibility = await service.resolveViewerEligibility(memberUser(), request(), null); + describe('who may comment', () => { + it('refuses commenting to a reader who cannot approve the review', async () => { + workflowFinderService.findWorkflowForUser.mockResolvedValue(null); + + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); expect(eligibility).toEqual({ canDecide: false, decisionIneligibilityReason: 'missing_publish_permission', + canComment: false, + }); + }); + + it('refuses commenting to an author who can no longer open the workflow under review', async () => { + workflowFinderService.findWorkflowForUser.mockResolvedValue(null); + authorRepository.isAuthor.mockResolvedValue(true); + + const eligibility = await service.resolveViewerEligibility( + memberUser(), + readable({ canReadPinnedWorkflow: false }), + ); + + expect(eligibility).toEqual({ + canDecide: false, + decisionIneligibilityReason: 'missing_publish_permission', + canComment: false, + }); + }); + + it('refuses both deciding and commenting on a review whose workflow is gone', async () => { + const eligibility = await service.resolveViewerEligibility( + memberUser(), + readable({ pinnedWorkflowId: null, canReadPinnedWorkflow: false }), + ); + + expect(eligibility).toEqual({ + canDecide: false, + decisionIneligibilityReason: 'missing_publish_permission', + canComment: false, }); expect(workflowFinderService.findWorkflowForUser).not.toHaveBeenCalled(); }); }); describe('hasAdminOverride', () => { - it('matches the review project against the user projects with the project:admin role', async () => { + it('grants the override to a project admin of the review project', async () => { projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([projectId]); await expect(service.hasAdminOverride(memberUser(), projectId)).resolves.toBe(true); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts index ae8f11d60a3..d62f776f530 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts @@ -104,6 +104,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { eligibilityService.resolveViewerEligibility.mockResolvedValue({ canDecide: true, decisionIneligibilityReason: null, + canComment: true, }); }); @@ -181,23 +182,26 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); describe('viewer eligibility', () => { - it('tells the client the viewer may decide', async () => { + it('tells the client the viewer may both decide and comment', async () => { const detail = await service.getDetail(requester, requestId); expect(detail.viewerCanDecide).toBe(true); expect(detail.viewerDecisionIneligibilityReason).toBeNull(); + expect(detail.viewerCanComment).toBe(true); }); - it('tells an author why they cannot decide', async () => { + it('tells an author why they cannot decide while still letting them comment', async () => { eligibilityService.resolveViewerEligibility.mockResolvedValue({ canDecide: false, decisionIneligibilityReason: 'author', + canComment: true, }); const detail = await service.getDetail(requester, requestId); expect(detail.viewerCanDecide).toBe(false); expect(detail.viewerDecisionIneligibilityReason).toBe('author'); + expect(detail.viewerCanComment).toBe(true); }); it('checks what the viewer may do against the workflow under review, even one they cannot open', async () => { @@ -212,18 +216,21 @@ describe('WorkflowReviewInboxService.getDetail', () => { eligibilityService.resolveViewerEligibility.mockResolvedValue({ canDecide: false, decisionIneligibilityReason: 'missing_publish_permission', + canComment: false, }); const detail = await service.getDetail(requester, requestId); - expect(eligibilityService.resolveViewerEligibility).toHaveBeenCalledWith( - requester, - expect.objectContaining({ id: requestId }), - workflowId, - ); + expect(eligibilityService.resolveViewerEligibility).toHaveBeenCalledWith(requester, { + request: expect.objectContaining({ id: requestId }), + readableWorkflowRows: [], + pinnedWorkflowId: workflowId, + canReadPinnedWorkflow: false, + }); expect(detail.workflows).toEqual([]); expect(detail.viewerCanDecide).toBe(false); expect(detail.viewerDecisionIneligibilityReason).toBe('missing_publish_permission'); + expect(detail.viewerCanComment).toBe(false); }); it('passes no workflow id when a closed review no longer covers any workflow', async () => { @@ -234,8 +241,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(eligibilityService.resolveViewerEligibility).toHaveBeenCalledWith( requester, - expect.objectContaining({ id: requestId }), - null, + expect.objectContaining({ pinnedWorkflowId: null, canReadPinnedWorkflow: false }), ); }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.test.ts index c34a8b5c506..f296aca14e7 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.test.ts @@ -17,6 +17,7 @@ const serviceGatedHandlers = new Set([ 'listInbox', 'getSummary', 'listActivity', + 'createComment', 'getDetail', ]); 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 54bbc830e4a..83600f7b81c 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 @@ -61,6 +61,13 @@ describe('workflow-review-requests (env flag off)', () => { test('GET activity is unreachable (404)', async () => { await ownerAgent.get('/workflow-review-requests/some-id/activity').expect(404); }); + + test('POST comment is unreachable (404)', async () => { + await ownerAgent + .post('/workflow-review-requests/some-id/comments') + .send({ body: 'Hello' }) + .expect(404); + }); }); describe('GET /workflow-review-requests (env flag off)', () => { diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts index 42678bcc85d..29d602998e0 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts @@ -1,11 +1,14 @@ import type { + CreateWorkflowReviewCommentDto, ListWorkflowReviewActivityQueryDto, ListWorkflowReviewActivityResponse, WorkflowReviewActivityEntry, WorkflowReviewEligibleReviewer, } from '@n8n/api-types'; import { + TransactionRunner, UserRepository, + WorkflowReviewActivityCommentRepository, WorkflowReviewActivityRepository, type User, type WorkflowReviewActivityFeedEntry, @@ -13,8 +16,10 @@ import { import { Service } from '@n8n/di'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { WorkflowReviewAccessService } from './workflow-review-access.service'; +import { WorkflowReviewEligibilityService } from './workflow-review-eligibility.service'; import { WorkflowReviewFeatureGate } from './workflow-review-feature-gate.service'; import { toActivityEntry, toEligibleReviewer } from './workflow-review.mapper'; @@ -23,8 +28,11 @@ export class WorkflowReviewActivityService { constructor( private readonly featureGate: WorkflowReviewFeatureGate, private readonly accessService: WorkflowReviewAccessService, + private readonly eligibilityService: WorkflowReviewEligibilityService, private readonly activityRepository: WorkflowReviewActivityRepository, + private readonly activityCommentRepository: WorkflowReviewActivityCommentRepository, private readonly userRepository: UserRepository, + private readonly txRunner: TransactionRunner, ) {} async listActivity( @@ -50,6 +58,47 @@ export class WorkflowReviewActivityService { return { data: await this.hydrate(entries), nextCursor, hasMore }; } + async createComment( + user: User, + workflowReviewRequestId: string, + dto: CreateWorkflowReviewCommentDto, + ): Promise { + await this.featureGate.assertAvailable(); + + const access = await this.accessService.findReadableRequestOrFail( + user, + workflowReviewRequestId, + ); + + // No lifecycle guard on purpose: a settled review stays open to discussion. + const eligibility = await this.eligibilityService.resolveViewerEligibility(user, access); + if (!eligibility.canComment) { + throw new ForbiddenError('You are not allowed to comment on this review'); + } + + // Every query inside must go through `ctx`. A stray read here needs a second + // pooled connection while the transaction holds one — a deadlock on a + // single-connection pool. + const { activity, message } = await this.txRunner.run({}, async (ctx) => { + const activity = await this.activityRepository.createActivity( + { + workflowReviewRequestId, + type: 'comment.created', + data: null, + createdById: user.id, + }, + ctx, + ); + const message = await this.activityCommentRepository.createComment( + { activityId: activity.id, createdById: user.id, body: dto.body }, + ctx, + ); + return { activity, message }; + }); + + return toActivityEntry(activity, [message], new Map([[user.id, toEligibleReviewer(user)]])); + } + private async hydrate( entries: WorkflowReviewActivityFeedEntry[], ): Promise { diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts index 3c64506fbe7..b7f2c1689e4 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts @@ -3,7 +3,6 @@ import { ProjectRelationRepository, WorkflowReviewRequestAuthorRepository, type User, - type WorkflowReviewRequest, } from '@n8n/db'; import { Service } from '@n8n/di'; import { @@ -14,16 +13,17 @@ import { import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; +import type { ReadableWorkflowReviewRequest } from './workflow-review-access.service'; + export interface WorkflowReviewViewerEligibility { canDecide: boolean; decisionIneligibilityReason: WorkflowReviewDecisionIneligibilityReason | null; + canComment: boolean; } /** - * Decision-eligibility rules shared between the decision endpoint - * (`WorkflowReviewRequestService.decide`) and the read side that surfaces the - * `viewerCanDecide` capability (`WorkflowReviewInboxService.getDetail`), so - * the two cannot drift. + * The viewer-capability rules of a review — who may decide it and who may comment + * on it — resolved in one pass so the two answers cannot disagree. */ @Service() export class WorkflowReviewEligibilityService { @@ -50,11 +50,11 @@ export class WorkflowReviewEligibilityService { } /** - * Advisory read-time snapshot of whether the viewer could decide the request, - * mirroring `decide()`'s authorization checks in order (publish on the pinned - * workflow first, then authorship) so the surfaced reason matches the error - * the endpoint would return. The endpoint remains the source of truth and - * re-checks under its lock. + * `canDecide` is an advisory read-time snapshot of whether the viewer could decide + * the request, mirroring `decide()`'s authorization checks in order (publish on the + * pinned workflow first, then authorship) so the surfaced reason matches the error + * the endpoint would return. The endpoint remains the source of truth and re-checks + * under its lock. * * Deliberately viewer-scoped: `decide()`'s `assertRequestUpdatable` lifecycle * guard is not mirrored here. It is shared with the update path and is not @@ -63,29 +63,46 @@ export class WorkflowReviewEligibilityService { */ async resolveViewerEligibility( user: User, - request: WorkflowReviewRequest, - pinnedWorkflowId: string | null, + access: Pick< + ReadableWorkflowReviewRequest, + 'request' | 'pinnedWorkflowId' | 'canReadPinnedWorkflow' + >, ): Promise { - // No linked workflow means decide() would 404 before any permission check + const { request, pinnedWorkflowId, canReadPinnedWorkflow } = access; + if (!pinnedWorkflowId) { - return { canDecide: false, decisionIneligibilityReason: 'missing_publish_permission' }; + return { + canDecide: false, + decisionIneligibilityReason: 'missing_publish_permission', + canComment: false, + }; } - const workflow = await this.workflowFinderService.findWorkflowForUser(pinnedWorkflowId, user, [ - 'workflow:publish', + const [workflow, isAuthor] = await Promise.all([ + this.workflowFinderService.findWorkflowForUser(pinnedWorkflowId, user, ['workflow:publish']), + this.workflowReviewRequestAuthorRepository.isAuthor( + { workflowReviewRequestId: request.id, userId: user.id }, + {}, + ), ]); + + // Authorship is history; access is not. An author keeps commenting only while they + // can still read the pinned workflow — `workflow:read`, not `workflow:publish`, so a + // personal-project requester is not locked out of their own review. + const canComment = isAuthor ? canReadPinnedWorkflow : Boolean(workflow); + if (!workflow) { - return { canDecide: false, decisionIneligibilityReason: 'missing_publish_permission' }; + return { + canDecide: false, + decisionIneligibilityReason: 'missing_publish_permission', + canComment, + }; } - const isAuthor = await this.workflowReviewRequestAuthorRepository.isAuthor( - { workflowReviewRequestId: request.id, userId: user.id }, - {}, - ); if (isAuthor && !(await this.hasAdminOverride(user, request.projectId))) { - return { canDecide: false, decisionIneligibilityReason: 'author' }; + return { canDecide: false, decisionIneligibilityReason: 'author', canComment }; } - return { canDecide: true, decisionIneligibilityReason: null }; + return { canDecide: true, decisionIneligibilityReason: null, canComment }; } } diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts index 96696655747..868bd07be7e 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts @@ -125,7 +125,7 @@ export class WorkflowReviewInboxService { this.resolveParticipants(request), // Resolved against the pinned (pre-read-filter) row, matching the row // decide() authorizes against — not against what the caller can read. - this.eligibilityService.resolveViewerEligibility(user, request, access.pinnedWorkflowId), + this.eligibilityService.resolveViewerEligibility(user, access), ]); const { requester, reviewers } = participantsByRequestId.get(request.id) ?? { @@ -139,6 +139,7 @@ export class WorkflowReviewInboxService { workflows, viewerCanDecide: eligibility.canDecide, viewerDecisionIneligibilityReason: eligibility.decisionIneligibilityReason, + viewerCanComment: eligibility.canComment, }; } 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 6b707ae9646..c08059f16d2 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,4 +1,5 @@ import { + CreateWorkflowReviewCommentDto, CreateWorkflowReviewRequestDto, DecideWorkflowReviewRequestDto, GetWorkflowReviewEligibleReviewersQueryDto, @@ -9,6 +10,7 @@ import { type ListWorkflowReviewActivityResponse, type ListWorkflowReviewInboxResponse, ListWorkflowReviewInboxQueryDto, + type WorkflowReviewActivityEntry, type WorkflowReviewRequestDetail, } from '@n8n/api-types'; import { AuthenticatedRequest } from '@n8n/db'; @@ -125,6 +127,23 @@ export class WorkflowReviewRequestsController { ); } + @Post('/:workflowReviewRequestId/comments') + @Licensed('feat:workflowReviews') + async createComment( + req: AuthenticatedRequest, + res: Response, + @Param('workflowReviewRequestId') workflowReviewRequestId: string, + @Body dto: CreateWorkflowReviewCommentDto, + ): Promise { + const entry = await this.workflowReviewActivityService.createComment( + req.user, + workflowReviewRequestId, + dto, + ); + res.status(201); + return entry; + } + /** * Review detail, including the diff inputs per covered workflow. * diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 76f662690a4..2961939e65e 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -5465,8 +5465,14 @@ "workflowReviews.detail.activity.listLabel": "Review activity", "workflowReviews.detail.activity.empty.heading": "No activity yet", "workflowReviews.detail.activity.empty.description": "Comments and review activity will appear here.", + "workflowReviews.detail.activity.comment.deleted": "This comment was deleted.", "workflowReviews.detail.activity.unknownEntry": "This activity entry can't be displayed.", + "workflowReviews.detail.activity.unknownAuthor": "Deleted user", + "workflowReviews.detail.activity.composer.label": "Add a comment", + "workflowReviews.detail.activity.composer.placeholder": "Leave a comment...", + "workflowReviews.detail.activity.composer.notAllowed": "You don't have permission to comment on this review.", "workflowReviews.detail.activity.error.load": "Could not load activity", + "workflowReviews.detail.activity.error.post": "Could not post comment", "workflowReviews.detail.metadata.status": "Status", "workflowReviews.detail.metadata.state.combinedLabel": "{state} • {status}", "workflowReviews.detail.metadata.reviewers": "Reviewers", diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts index d15ba40e1a5..c4ad71ea5c1 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts @@ -61,6 +61,7 @@ describe('WorkflowReviewActivityFeed', () => { store.loading = false; store.loadingMore = false; store.hasMore = false; + store.nextCursor = null; store.error = null; }); @@ -109,9 +110,32 @@ describe('WorkflowReviewActivityFeed', () => { expect(store.fetchFeed).toHaveBeenCalledWith('req-1'); }); + it('still reaches the earlier activity after posting onto a feed that failed to load', () => { + store.error = new Error('boom'); + store.entries = [makeEntry()]; + + const { getByTestId } = renderComponent(); + getByTestId('workflow-review-activity-load-more-retry').click(); + + expect(store.fetchFeed).toHaveBeenCalledWith('req-1'); + expect(store.loadMore).not.toHaveBeenCalled(); + }); + + it('shows progress while refetching a feed that already has entries', async () => { + store.entries = [makeEntry()]; + store.loading = true; + + const { container } = renderComponent(); + await nextTick(); + + expect(container.querySelector('.n8n-loading')).toBeInTheDocument(); + }); + it('keeps a loaded feed and offers a retry when load-more failed', async () => { store.entries = [makeEntry()]; store.hasMore = true; + // A load-more failure always has a cursor to resume from. + store.nextCursor = 'cursor-1'; store.error = new Error('boom'); const { getAllByTestId, getByTestId, queryByTestId } = renderComponent(); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue index 07950d19a00..8760d1f6bb3 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue @@ -71,9 +71,15 @@ watch( { flush: 'post' }, ); -// `loadMore` is a no-op with no cursor, so a failed first page has to refetch. -function retryInitialLoad() { - if (store.currentReviewId) void store.fetchFeed(store.currentReviewId); +// `loadMore` is a no-op with no cursor, so a failed first page has to refetch. Shared by both +// error rows: posting onto a failed feed moves the viewer from the first to the second, which +// would otherwise hit that dead end and leave the earlier activity unreachable. +function retry() { + if (!store.nextCursor) { + if (store.currentReviewId) void store.fetchFeed(store.currentReviewId); + return; + } + void store.loadMore(); } // Entries may already be loaded on mount (Changes -> Activity round trip). @@ -98,7 +104,7 @@ onMounted(() => { size="mini" variant="ghost" data-test-id="workflow-review-activity-retry" - @click="retryInitialLoad()" + @click="retry()" > {{ i18n.baseText('generic.retry') }} @@ -116,7 +122,9 @@ onMounted(() => { :class="$style.sentinel" data-test-id="workflow-review-activity-load-more-sentinel" /> - + +
{{ i18n.baseText('workflowReviews.detail.activity.error.load') }} @@ -125,7 +133,7 @@ onMounted(() => { size="mini" variant="ghost" data-test-id="workflow-review-activity-load-more-retry" - @click="store.loadMore()" + @click="retry()" > {{ i18n.baseText('generic.retry') }} diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewCommentComposer.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewCommentComposer.test.ts new file mode 100644 index 00000000000..41ae521f2c1 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewCommentComposer.test.ts @@ -0,0 +1,128 @@ +import { WORKFLOW_REVIEW_COMMENT_MAX_LENGTH } from '@n8n/api-types'; +import { createTestingPinia } from '@pinia/testing'; +import userEvent from '@testing-library/user-event'; +import { waitFor } from '@testing-library/vue'; +import { createComponentRenderer } from '@/__tests__/render'; +import { mockedStore } from '@/__tests__/utils'; + +import { useReviewActivityStore } from '../reviewActivity.store'; +import WorkflowReviewCommentComposer from './WorkflowReviewCommentComposer.vue'; + +const showError = vi.fn(); + +vi.mock('@n8n/composables/useToast', () => ({ + useToast: () => ({ showError }), +})); + +/** + * The real N8nChatInput is mounted on purpose: `submitDisabled` overrides the + * component's whole internal send gate, which a stub cannot show. + */ +const renderComponent = createComponentRenderer(WorkflowReviewCommentComposer); + +describe('WorkflowReviewCommentComposer', () => { + let store: ReturnType>; + + beforeEach(() => { + createTestingPinia(); + showError.mockReset(); + store = mockedStore(useReviewActivityStore); + store.posting = false; + store.draft = ''; + store.postComment.mockResolvedValue(undefined); + }); + + it('keeps a half-typed comment when the composer is unmounted and shown again', async () => { + // The Changes tab is a `v-if`, so it unmounts this component. + const first = renderComponent({ props: { canComment: true } }); + await userEvent.type(first.getByRole('textbox'), 'half a thought'); + first.unmount(); + + const second = renderComponent({ props: { canComment: true } }); + + expect(second.getByRole('textbox')).toHaveValue('half a thought'); + }); + + it('disables the send button and the textarea when the viewer cannot comment', () => { + const { getByTestId, getByRole } = renderComponent({ props: { canComment: false } }); + + expect(getByTestId('send-message-button')).toBeDisabled(); + expect(getByRole('textbox')).toBeDisabled(); + }); + + it('disables only the send button while the draft is empty', () => { + const { getByTestId, getByRole } = renderComponent({ props: { canComment: true } }); + + expect(getByTestId('send-message-button')).toBeDisabled(); + expect(getByRole('textbox')).not.toBeDisabled(); + }); + + it('enables the send button once the draft has content', async () => { + const { getByTestId, getByRole } = renderComponent({ props: { canComment: true } }); + + await userEvent.type(getByRole('textbox'), 'Nice work'); + + await waitFor(() => expect(getByTestId('send-message-button')).not.toBeDisabled()); + }); + + it('disables the send button once the draft goes over the length limit', async () => { + const { getByTestId, getByRole } = renderComponent({ props: { canComment: true } }); + const textarea = getByRole('textbox'); + + textarea.focus(); + await userEvent.paste('x'.repeat(WORKFLOW_REVIEW_COMMENT_MAX_LENGTH)); + await waitFor(() => expect(getByTestId('send-message-button')).not.toBeDisabled()); + // Shift+Enter inserts the newline itself, so `maxlength` on the textarea does not stop it + await userEvent.keyboard('{Shift>}{Enter}{/Shift}'); + + expect(textarea).toHaveValue(`${'x'.repeat(WORKFLOW_REVIEW_COMMENT_MAX_LENGTH)}\n`); + await waitFor(() => expect(getByTestId('send-message-button')).toBeDisabled()); + }); + + it('posts the draft and clears it', async () => { + const { getByTestId, getByRole } = renderComponent({ props: { canComment: true } }); + const textarea = getByRole('textbox'); + + await userEvent.type(textarea, 'Nice work'); + await userEvent.click(getByTestId('send-message-button')); + + expect(store.postComment).toHaveBeenCalledWith('Nice work'); + await waitFor(() => expect(textarea).toHaveValue('')); + }); + + it('keeps text typed while the previous comment was still posting', async () => { + let resolvePost!: () => void; + store.postComment.mockImplementation( + async () => + await new Promise((resolve) => { + resolvePost = resolve; + }), + ); + const { getByTestId, getByRole } = renderComponent({ props: { canComment: true } }); + const textarea = getByRole('textbox'); + + await userEvent.type(textarea, 'Nice work'); + await userEvent.click(getByTestId('send-message-button')); + // The textarea stays enabled during the request, so the user keeps typing + await userEvent.type(textarea, ' and the next one'); + expect(store.postComment).toHaveBeenCalledWith('Nice work'); + resolvePost(); + await new Promise(setImmediate); + + expect(textarea).toHaveValue('Nice work and the next one'); + }); + + it('keeps the draft and surfaces an error when posting fails', async () => { + store.postComment.mockRejectedValue(new Error('boom')); + const { getByTestId, getByRole } = renderComponent({ props: { canComment: true } }); + const textarea = getByRole('textbox'); + + await userEvent.type(textarea, 'Nice work'); + await userEvent.click(getByTestId('send-message-button')); + + await waitFor(() => + expect(showError).toHaveBeenCalledWith(expect.any(Error), 'Could not post comment'), + ); + expect(textarea).toHaveValue('Nice work'); + }); +}); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewCommentComposer.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewCommentComposer.vue new file mode 100644 index 00000000000..1bb826dadf3 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewCommentComposer.vue @@ -0,0 +1,85 @@ + + + + + diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts index fb2535eeb61..df07aba2571 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts @@ -64,6 +64,7 @@ function makeDetail( workflows: [makeWorkflowDetail()], viewerCanDecide: true, viewerDecisionIneligibilityReason: null, + viewerCanComment: true, ...overrides, }; } diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts index 9147aba4db9..a3dbfe3272e 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.test.ts @@ -23,6 +23,15 @@ vi.mock('./WorkflowReviewActivityFeed.vue', () => ({ }, })); +vi.mock('./WorkflowReviewCommentComposer.vue', () => ({ + default: { + name: 'WorkflowReviewCommentComposer', + props: ['canComment'], + template: + '
', + }, +})); + vi.mock('./WorkflowReviewDetailMetadata.vue', () => ({ default: { name: 'WorkflowReviewDetailMetadata', @@ -100,6 +109,7 @@ function makeDetail( workflows: [makeWorkflowDetail()], viewerCanDecide: true, viewerDecisionIneligibilityReason: null, + viewerCanComment: true, ...overrides, }; } @@ -141,7 +151,7 @@ describe('WorkflowReviewDetailTabs', () => { expect(getByTestId('workflow-review-no-description')).toBeInTheDocument(); }); - it('renders the feed below the description', () => { + it('renders the feed and the composer below the description', () => { const { getByTestId } = renderComponent({ props: { review: makeDetail({ description: 'Adds retry logic' }), @@ -151,18 +161,46 @@ describe('WorkflowReviewDetailTabs', () => { }); const panel = getByTestId('workflow-review-activity-panel'); - const order = ['workflow-review-description', 'workflow-review-activity-feed'].map( - (testId) => { - const element = panel.querySelector(`[data-test-id="${testId}"]`); - if (!element) throw new Error(`${testId} is not in the activity panel`); - return element; - }, - ); + const order = [ + 'workflow-review-description', + 'workflow-review-activity-feed', + 'workflow-review-comment-composer', + ].map((testId) => { + const element = panel.querySelector(`[data-test-id="${testId}"]`); + if (!element) throw new Error(`${testId} is not in the activity panel`); + return element; + }); expect(order[0].compareDocumentPosition(order[1])).toBe(Node.DOCUMENT_POSITION_FOLLOWING); + expect(order[1].compareDocumentPosition(order[2])).toBe(Node.DOCUMENT_POSITION_FOLLOWING); }); - it('still shows the feed on a closed review', () => { + it.each([ + ['lets a viewer who may comment use the composer', true], + ['locks the composer for a viewer who may not', false], + ])('%s', (_label, viewerCanComment) => { + const { getByTestId } = renderComponent({ + props: { review: makeDetail({ viewerCanComment }), tab: 'activity', deciding: false }, + }); + + expect(getByTestId('workflow-review-comment-composer')).toHaveAttribute( + 'data-can-comment', + String(viewerCanComment), + ); + }); + + it('defaults the composer to read-only on a review whose detail never loaded', () => { + const { getByTestId } = renderComponent({ + props: { review: makeInboxItem(), tab: 'activity', deciding: false }, + }); + + expect(getByTestId('workflow-review-comment-composer')).toHaveAttribute( + 'data-can-comment', + 'false', + ); + }); + + it('still lets the viewer comment on a closed review', () => { const { getByTestId } = renderComponent({ props: { review: makeDetail({ state: 'closed', decision: 'approved' }), @@ -172,6 +210,10 @@ describe('WorkflowReviewDetailTabs', () => { }); expect(getByTestId('workflow-review-activity-feed')).toBeInTheDocument(); + expect(getByTestId('workflow-review-comment-composer')).toHaveAttribute( + 'data-can-comment', + 'true', + ); }); }); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.vue index 887773812ed..f7446633e6a 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.vue @@ -7,6 +7,7 @@ import { computed } from 'vue'; import type { WorkflowReviewDecisionInput } from '../workflowReviews.api'; import WorkflowReviewActivityFeed from './WorkflowReviewActivityFeed.vue'; import WorkflowReviewChangesSection from './WorkflowReviewChangesSection.vue'; +import WorkflowReviewCommentComposer from './WorkflowReviewCommentComposer.vue'; import WorkflowReviewDetailMetadata from './WorkflowReviewDetailMetadata.vue'; export type WorkflowReviewDetailTab = 'activity' | 'changes'; @@ -29,6 +30,7 @@ const detail = computed(() => ); const viewerCanDecide = computed(() => detail.value?.viewerCanDecide ?? false); +const viewerCanComment = computed(() => detail.value?.viewerCanComment ?? false); const ineligibilityHint = computed(() => { if (!detail.value || detail.value.viewerCanDecide) return ''; @@ -116,6 +118,8 @@ const tabOptions = computed(() => [
+ +
@@ -186,7 +190,8 @@ const tabOptions = computed(() => [ overflow: auto; } -/* Separate from `.panel`: the feed brings its own scroll container. */ +/* Separate from `.panel`: the feed brings its own scroll container, and the + composer must stay out of it. */ .activityPanel { display: flex; flex-direction: column; @@ -195,7 +200,7 @@ const tabOptions = computed(() => [ overflow: hidden; } -/* Capped so a long description cannot crowd out the feed. */ +/* Capped so a long description cannot push the composer off screen. */ .activityHeader { flex-shrink: 0; max-height: 30%; diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/activity-entries/WorkflowReviewActivityComment.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/activity-entries/WorkflowReviewActivityComment.test.ts new file mode 100644 index 00000000000..b58fa9879ae --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/activity-entries/WorkflowReviewActivityComment.test.ts @@ -0,0 +1,102 @@ +import type { WorkflowReviewActivityEntry, WorkflowReviewActivityMessage } from '@n8n/api-types'; +import { createTestingPinia } from '@pinia/testing'; +import { createComponentRenderer } from '@/__tests__/render'; + +import WorkflowReviewActivityComment from './WorkflowReviewActivityComment.vue'; + +const renderComponent = createComponentRenderer(WorkflowReviewActivityComment); + +function makeMessage( + overrides: Partial = {}, +): WorkflowReviewActivityMessage { + return { + id: 'msg-1', + body: 'Looks good to me', + createdBy: { + id: 'user-1', + email: 'ada@example.com', + firstName: 'Ada', + lastName: 'Lovelace', + }, + createdAt: '2024-01-01T10:00:00.000Z', + updatedAt: null, + deletedAt: null, + ...overrides, + }; +} + +function makeEntry( + messages: WorkflowReviewActivityMessage[] = [makeMessage()], +): Extract { + return { + id: '1', + type: 'comment.created', + typeVersion: 1, + data: null, + createdBy: messages[0]?.createdBy ?? null, + createdAt: '2024-01-01T10:00:00.000Z', + messages, + }; +} + +describe('WorkflowReviewActivityComment', () => { + beforeEach(() => { + createTestingPinia(); + }); + + it('renders the body, the author name and a machine-readable timestamp', () => { + const { getByTestId } = renderComponent({ props: { entry: makeEntry() } }); + + expect(getByTestId('workflow-review-activity-comment-body')).toHaveTextContent( + 'Looks good to me', + ); + expect(getByTestId('workflow-review-activity-comment-author')).toHaveTextContent( + 'Ada Lovelace', + ); + expect(getByTestId('workflow-review-activity-comment-time')).toHaveAttribute( + 'datetime', + '2024-01-01T10:00:00.000Z', + ); + }); + + it('falls back to the email when the author has no name', () => { + const { getByTestId } = renderComponent({ + props: { + entry: makeEntry([ + makeMessage({ + createdBy: { id: 'user-1', email: 'ada@example.com', firstName: null, lastName: null }, + }), + ]), + }, + }); + + expect(getByTestId('workflow-review-activity-comment-author')).toHaveTextContent( + 'ada@example.com', + ); + }); + + it('names a deleted author', () => { + const { getByTestId } = renderComponent({ + props: { entry: makeEntry([makeMessage({ createdBy: null })]) }, + }); + + expect(getByTestId('workflow-review-activity-comment-author')).toHaveTextContent( + 'Deleted user', + ); + }); + + // Keys on `deletedAt`, not on a null body: a null body from any other writer + // must not read as a tombstone. + it('shows that a comment was deleted instead of its text', () => { + const { getByTestId, queryByTestId } = renderComponent({ + props: { + entry: makeEntry([makeMessage({ body: null, deletedAt: '2024-01-02T10:00:00.000Z' })]), + }, + }); + + expect(getByTestId('workflow-review-activity-comment-deleted')).toHaveTextContent( + 'This comment was deleted.', + ); + expect(queryByTestId('workflow-review-activity-comment-body')).not.toBeInTheDocument(); + }); +}); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/activity-entries/WorkflowReviewActivityComment.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/activity-entries/WorkflowReviewActivityComment.vue new file mode 100644 index 00000000000..9c0678a7af6 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/activity-entries/WorkflowReviewActivityComment.vue @@ -0,0 +1,112 @@ + + + + + diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/activityEntryRegistry.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/activityEntryRegistry.ts index 0815c35fadd..9a264704a95 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/activityEntryRegistry.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/activityEntryRegistry.ts @@ -1,11 +1,14 @@ import type { WorkflowReviewActivityEntry, WorkflowReviewActivityType } from '@n8n/api-types'; import type { Component } from 'vue'; +import WorkflowReviewActivityComment from './activity-entries/WorkflowReviewActivityComment.vue'; import WorkflowReviewActivityFallback from './activity-entries/WorkflowReviewActivityFallback.vue'; type ActivityEntryRegistry = Partial>>; -const registry: ActivityEntryRegistry = {}; +const registry: ActivityEntryRegistry = { + 'comment.created': { 1: WorkflowReviewActivityComment }, +}; export function resolveActivityComponent(entry: WorkflowReviewActivityEntry): Component { return registry[entry.type]?.[entry.typeVersion] ?? WorkflowReviewActivityFallback; diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.test.ts index 665bbebadca..3866a344da8 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.test.ts @@ -94,13 +94,18 @@ describe('useReviewActivityStore', () => { expect(store.entries.map((entry) => entry.id)).toEqual(['9']); }); - it('puts older entries above the ones already loaded', async () => { + it('puts older entries above and a new comment at the bottom', async () => { vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue( makePage(['3', '4'], { nextCursor: 'cursor-1', hasMore: true }), ); + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockResolvedValue(makeEntry('5')); const store = useReviewActivityStore(); await store.fetchFeed('req-1'); + await store.postComment('hi'); + + expect(store.entries.map((entry) => entry.id)).toEqual(['3', '4', '5']); + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue( makePage(['1', '2']), ); @@ -113,7 +118,7 @@ describe('useReviewActivityStore', () => { 'req-1', { limit: 25, cursor: 'cursor-1' }, ); - expect(store.entries.map((entry) => entry.id)).toEqual(['1', '2', '3', '4']); + expect(store.entries.map((entry) => entry.id)).toEqual(['1', '2', '3', '4', '5']); expect(store.hasMore).toBe(false); expect(store.nextCursor).toBeNull(); }); @@ -143,6 +148,49 @@ describe('useReviewActivityStore', () => { expect(store.entries.map((entry) => entry.id)).toEqual(['7']); }); + it('keeps a comment posted while the first page was still in flight', async () => { + let resolveFeed!: (response: ListWorkflowReviewActivityResponse) => void; + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockImplementationOnce( + async () => + await new Promise((resolve) => { + resolveFeed = resolve; + }), + ); + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockResolvedValue(makeEntry('5')); + const store = useReviewActivityStore(); + + const pendingFeed = store.fetchFeed('req-1'); + await store.postComment('hi'); + // The page was snapshotted server-side before the comment was written + resolveFeed(makePage(['3', '4'])); + await pendingFeed; + + expect(store.entries.map((entry) => entry.id)).toEqual(['3', '4', '5']); + }); + + it('does not duplicate a comment the refetched feed already returned', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage([])); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + + let resolvePost!: (entry: WorkflowReviewActivityEntry) => void; + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockImplementationOnce( + async () => + await new Promise((resolve) => { + resolvePost = resolve; + }), + ); + + const pendingPost = store.postComment('hi'); + await store.fetchFeed('req-2'); + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['5'])); + await store.fetchFeed('req-1'); + resolvePost(makeEntry('5')); + await pendingPost; + + expect(store.entries.map((entry) => entry.id)).toEqual(['5']); + }); + it('stops paging when an older page comes back empty', async () => { vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue( makePage(['3'], { nextCursor: 'cursor-1', hasMore: true }), @@ -159,6 +207,13 @@ describe('useReviewActivityStore', () => { expect(store.entries.map((entry) => entry.id)).toEqual(['3']); }); + it('rejects a post with no review selected instead of reporting success', async () => { + const store = useReviewActivityStore(); + + await expect(store.postComment('hi')).rejects.toThrow(); + expect(workflowReviewsApi.createWorkflowReviewComment).not.toHaveBeenCalled(); + }); + it('does not ask for older entries when the feed is already complete', async () => { vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['1'])); const store = useReviewActivityStore(); @@ -181,12 +236,158 @@ describe('useReviewActivityStore', () => { const store = useReviewActivityStore(); const pending = store.fetchFeed('req-1'); + store.draft = 'for req-1 only'; store.reset(); resolveFeed(makePage(['1'])); await pending; expect(store.entries).toEqual([]); expect(store.currentReviewId).toBeNull(); + // Leaving the view must not carry a draft into whatever review is opened next. + expect(store.draft).toBe(''); + }); + + it('does not leave the next review stuck in a sending state', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['1'])); + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockReturnValue( + new Promise(() => {}), + ); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + + void store.postComment('hi'); + expect(store.posting).toBe(true); + + await store.fetchFeed('req-2'); + + expect(store.posting).toBe(false); + }); + + it('keeps send disabled for the post the viewer is still waiting on after leaving and returning', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['1'])); + const resolvers: Array<(entry: WorkflowReviewActivityEntry) => void> = []; + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockImplementation( + async () => await new Promise((resolve) => resolvers.push(resolve)), + ); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + const first = store.postComment('hi'); + + await store.fetchFeed('req-2'); + await store.fetchFeed('req-1'); + const second = store.postComment('again'); + expect(store.posting).toBe(true); + + resolvers[0](makeEntry('9')); + await first; + expect(store.posting).toBe(true); + + resolvers[1](makeEntry('10')); + await second; + expect(store.posting).toBe(false); + }); + + it('keeps the send button disabled when a failed page is retried mid-post', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockRejectedValue(new Error('boom')); + let resolvePost: (entry: WorkflowReviewActivityEntry) => void = () => {}; + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockReturnValue( + new Promise((resolve) => { + resolvePost = resolve; + }), + ); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + store.draft = 'half a thought'; + + const pending = store.postComment('hi'); + expect(store.posting).toBe(true); + + // Retrying refetches the same review. + await store.fetchFeed('req-1'); + expect(store.posting).toBe(true); + expect(store.draft).toBe('half a thought'); + + resolvePost(makeEntry('9')); + await pending; + expect(store.posting).toBe(false); + }); + + it('keeps a comment posted onto a failed feed visible while that feed is retried', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockRejectedValue(new Error('boom')); + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockResolvedValue(makeEntry('9')); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + await store.postComment('hi'); + expect(store.entries.map((entry) => entry.id)).toEqual(['9']); + + await store.fetchFeed('req-1'); + + expect(store.entries.map((entry) => entry.id)).toEqual(['9']); + }); + + it('drops older pages a refetch did not return so the feed stays in order', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue( + makePage(['3', '4'], { nextCursor: 'cursor-1', hasMore: true }), + ); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue( + makePage(['1', '2']), + ); + await store.loadMore(); + expect(store.entries.map((entry) => entry.id)).toEqual(['1', '2', '3', '4']); + + // Kept older pages would land after the newer one and invert the list. + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue( + makePage(['3', '4'], { nextCursor: 'cursor-1', hasMore: true }), + ); + await store.fetchFeed('req-1'); + + expect(store.entries.map((entry) => entry.id)).toEqual(['3', '4']); + }); + + it("does not drop a stale post's comment into the review the viewer moved to", async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['1'])); + let resolvePost: (entry: WorkflowReviewActivityEntry) => void = () => {}; + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockReturnValue( + new Promise((resolve) => { + resolvePost = resolve; + }), + ); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + const pending = store.postComment('hi'); + + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['2'])); + await store.fetchFeed('req-2'); + resolvePost(makeEntry('9')); + await pending; + + expect(store.entries.map((entry) => entry.id)).toEqual(['2']); + }); + + it('drops the draft when the viewer moves to another review', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['1'])); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + store.draft = 'for req-1 only'; + + await store.fetchFeed('req-2'); + + expect(store.draft).toBe(''); + }); + + it('reports a failed comment to the composer, not as a feed error', async () => { + vi.mocked(workflowReviewsApi.fetchWorkflowReviewActivity).mockResolvedValue(makePage(['1'])); + vi.mocked(workflowReviewsApi.createWorkflowReviewComment).mockRejectedValue(new Error('nope')); + const store = useReviewActivityStore(); + await store.fetchFeed('req-1'); + + await expect(store.postComment('hi')).rejects.toThrow('nope'); + + expect(store.error).toBeNull(); + expect(store.posting).toBe(false); + expect(store.entries.map((entry) => entry.id)).toEqual(['1']); }); it('shows a failed load in the feed rather than throwing', async () => { diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.ts index daba6564f60..01769bcc07f 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewActivity.store.ts @@ -4,14 +4,14 @@ import { ref } from 'vue'; import { useRootStore } from '@n8n/stores/useRootStore'; -import { fetchWorkflowReviewActivity } from './workflowReviews.api'; +import { createWorkflowReviewComment, fetchWorkflowReviewActivity } from './workflowReviews.api'; import { toError } from './workflowReviews.utils'; const DEFAULT_LIMIT = 25; /** * The activity feed of one review. `entries` is ascending by id: the backend pages - * backwards, so `loadMore` prepends older pages. + * backwards, so `loadMore` prepends older pages and `postComment` appends. */ export const useReviewActivityStore = defineStore('workflowReviewActivity', () => { const rootStore = useRootStore(); @@ -22,21 +22,33 @@ export const useReviewActivityStore = defineStore('workflowReviewActivity', () = const hasMore = ref(false); const loading = ref(false); const loadingMore = ref(false); + const posting = ref(false); const error = ref(null); + // Held here, not in the composer: switching to the Changes tab unmounts it, and a + // half-typed comment must survive that. + const draft = ref(''); let feedRequestSeq = 0; + let postSeq = 0; async function fetchFeed(reviewId: string) { const requestSeq = ++feedRequestSeq; - // Cleared synchronously: otherwise the gap until the response arrives renders - // the previous review's feed instead of the loading state. + const switchedReview = currentReviewId.value !== reviewId; currentReviewId.value = reviewId; - entries.value = []; nextCursor.value = null; hasMore.value = false; loadingMore.value = false; loading.value = true; error.value = null; + // Clearing on a refetch would drop a comment the viewer just posted onto a feed whose + // first page failed, re-enable send mid-post, and discard what they are typing. On a + // switch it has to go synchronously, or the gap until the response arrives renders the + // previous review's feed. + if (switchedReview) { + entries.value = []; + posting.value = false; + draft.value = ''; + } try { const response = await fetchWorkflowReviewActivity(rootStore.restApiContext, reviewId, { @@ -44,7 +56,15 @@ export const useReviewActivityStore = defineStore('workflowReviewActivity', () = }); if (requestSeq !== feedRequestSeq) return; - entries.value = response.data; + // Merged, not assigned: a comment posted while this page was in flight is already + // appended, and the server snapshot predates it. Only what is newer than the page + // survives, since `entries` is ascending by id and an older page kept here would + // land after the newer one and invert the list. + const newestInPage = Number(response.data.at(-1)?.id ?? 0); + entries.value = [ + ...response.data, + ...entries.value.filter((entry) => Number(entry.id) > newestInPage), + ]; nextCursor.value = response.nextCursor; hasMore.value = response.hasMore; } catch (e) { @@ -90,6 +110,26 @@ export const useReviewActivityStore = defineStore('workflowReviewActivity', () = } } + async function postComment(body: string) { + const reviewId = currentReviewId.value; + // Thrown, not swallowed: a silent return would read as success to the caller. + if (!reviewId) throw new Error('Cannot post a comment without a selected review'); + + const requestSeq = ++postSeq; + posting.value = true; + try { + const entry = await createWorkflowReviewComment(rootStore.restApiContext, reviewId, { body }); + if (currentReviewId.value !== reviewId) return; + + // A feed refetch that raced this post may already carry the comment. + entries.value = [...entries.value.filter((existing) => existing.id !== entry.id), entry]; + } finally { + // Only the newest post owns the flag: after A -> B -> A a stale post finishing would + // otherwise re-enable send while the post the user is waiting on is still in flight. + if (requestSeq === postSeq) posting.value = false; + } + } + function reset() { feedRequestSeq += 1; currentReviewId.value = null; @@ -98,7 +138,9 @@ export const useReviewActivityStore = defineStore('workflowReviewActivity', () = hasMore.value = false; loading.value = false; loadingMore.value = false; + posting.value = false; error.value = null; + draft.value = ''; } return { @@ -108,9 +150,12 @@ export const useReviewActivityStore = defineStore('workflowReviewActivity', () = hasMore, loading, loadingMore, + posting, error, + draft, fetchFeed, loadMore, + postComment, reset, }; }); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts index 00e72090a70..61c16299362 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts @@ -507,5 +507,6 @@ function createDetail(): WorkflowReviewRequestDetail { workflows: [], viewerCanDecide: true, viewerDecisionIneligibilityReason: null, + viewerCanComment: true, }; } diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts index 137a2f2ffd0..a184680f754 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/views/WorkflowReviewRequestsView.test.ts @@ -808,6 +808,7 @@ function createDetail( workflows: [], viewerCanDecide: true, viewerDecisionIneligibilityReason: null, + viewerCanComment: true, ...overrides, }; } 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 a0595c98e1c..25251098ede 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 @@ -7,6 +7,7 @@ import type { ListWorkflowReviewActivityResponse, ListWorkflowReviewInboxResponse, UpdateWorkflowReviewRequestVersionDto, + WorkflowReviewActivityEntry, WorkflowReviewEligibleReviewersList, WorkflowReviewRequestDetail, WorkflowReviewRequestList, @@ -124,3 +125,16 @@ export async function fetchWorkflowReviewActivity( params, ); } + +export async function createWorkflowReviewComment( + context: IRestApiContext, + workflowReviewRequestId: string, + payload: { body: string }, +): Promise { + return await makeRestApiRequest( + context, + 'POST', + `/workflow-review-requests/${encodeURIComponent(workflowReviewRequestId)}/comments`, + { ...payload }, + ); +}