feat(core): Add comments to the workflow review activity feed (no-changelog) (#35792)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Sandra Zollner <sandra.zollner@n8n.io>
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Jan Kalkan
2026-08-12 09:36:55 +00:00
committed by GitHub
co-authored by Claude Opus 5 Sandra Zollner Cursor
parent a319c76264
commit 90b201e47b
30 changed files with 1425 additions and 113 deletions
+2
View File
@@ -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';
@@ -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;
}
@@ -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'),
}) {}
@@ -93,6 +93,7 @@ export {
WorkflowReviewActivityRepository,
type WorkflowReviewActivityFeedEntry,
} from './workflow-review-activity.repository';
export { WorkflowReviewActivityCommentRepository } from './workflow-review-activity-comment.repository';
export {
WorkflowDependencyRepository,
WorkflowDependencies,
@@ -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<WorkflowReviewActivityComment> {
constructor(dataSource: DataSource, transactionRunner: TransactionRunner) {
super(WorkflowReviewActivityComment, dataSource.manager, transactionRunner);
}
async createComment(
input: {
activityId: number;
createdById: string | null;
body: string;
},
ctx: OperationContext,
): Promise<WorkflowReviewActivityComment> {
const entity = this.create(input);
return await this.managerFor(ctx).save(WorkflowReviewActivityComment, entity);
}
}
@@ -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);
});
});
@@ -30,6 +30,15 @@ describe('WorkflowReviewEligibilityService', () => {
const request = () => mock<WorkflowReviewRequest>({ id: requestId, projectId });
const readable = (
overrides: Partial<Parameters<typeof service.resolveViewerEligibility>[1]> = {},
) => ({
request: request(),
pinnedWorkflowId: workflowId,
canReadPinnedWorkflow: true,
...overrides,
});
beforeEach(() => {
vi.resetAllMocks();
workflowFinderService.findWorkflowForUser.mockResolvedValue(mock<WorkflowEntity>());
@@ -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<User>({ 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<WorkflowReviewRequest>({ id: requestId, projectId, ...overrides }),
workflowId,
readable({
request: mock<WorkflowReviewRequest>({ 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);
@@ -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 }),
);
});
});
@@ -17,6 +17,7 @@ const serviceGatedHandlers = new Set([
'listInbox',
'getSummary',
'listActivity',
'createComment',
'getDetail',
]);
@@ -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)', () => {
@@ -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<WorkflowReviewActivityEntry> {
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<WorkflowReviewActivityEntry[]> {
@@ -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<WorkflowReviewViewerEligibility> {
// 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 };
}
}
@@ -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,
};
}
@@ -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<WorkflowReviewActivityEntry> {
const entry = await this.workflowReviewActivityService.createComment(
req.user,
workflowReviewRequestId,
dto,
);
res.status(201);
return entry;
}
/**
* Review detail, including the diff inputs per covered workflow.
*
@@ -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",
@@ -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();
@@ -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') }}
</N8nButton>
@@ -116,7 +122,9 @@ onMounted(() => {
:class="$style.sentinel"
data-test-id="workflow-review-activity-load-more-sentinel"
/>
<N8nLoading v-if="loadingMore" :loading="true" :rows="1" />
<!-- `loading` too: a retry that keeps a posted comment on screen leaves this the
only place a refetch can show progress. -->
<N8nLoading v-if="loadingMore || loading" :loading="true" :rows="1" />
<div v-if="error" :class="$style.errorRow">
<N8nText color="text-light" size="small">
{{ 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') }}
</N8nButton>
@@ -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<typeof mockedStore<typeof useReviewActivityStore>>;
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<void>((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');
});
});
@@ -0,0 +1,85 @@
<script setup lang="ts">
import { WORKFLOW_REVIEW_COMMENT_MAX_LENGTH } from '@n8n/api-types';
import { N8nChatInput } from '@n8n/design-system';
import { useI18n } from '@n8n/i18n';
import { useToast } from '@n8n/composables/useToast';
import { storeToRefs } from 'pinia';
import { computed } from 'vue';
import { useReviewActivityStore } from '../reviewActivity.store';
const props = defineProps<{ canComment: boolean }>();
const i18n = useI18n();
const { showError } = useToast();
const store = useReviewActivityStore();
const { posting, draft } = storeToRefs(store);
// The full condition, not just `posting`: N8nChatInput uses `submitDisabled ?? …`,
// so a bare `false` would replace its own empty/over-limit/disabled gate.
const submitDisabled = computed(
() =>
posting.value ||
draft.value.trim().length === 0 ||
draft.value.length > WORKFLOW_REVIEW_COMMENT_MAX_LENGTH ||
!props.canComment,
);
async function onSubmit() {
const submitted = draft.value;
const body = submitted.trim();
if (!body) return;
try {
await store.postComment(body);
// Don't clear text typed while the post was in flight.
if (draft.value === submitted) draft.value = '';
} catch (error) {
showError(error, i18n.baseText('workflowReviews.detail.activity.error.post'));
}
}
</script>
<template>
<!-- Implicit label: the textarea has no id to point a `for` at, and
`inheritAttrs: false` would send an aria-label to the wrapper instead. -->
<label :class="$style.composer">
<span :class="$style.srOnly">
{{ i18n.baseText('workflowReviews.detail.activity.composer.label') }}
</span>
<N8nChatInput
v-model="draft"
:max-length="WORKFLOW_REVIEW_COMMENT_MAX_LENGTH"
:placeholder="i18n.baseText('workflowReviews.detail.activity.composer.placeholder')"
refocus-after-send
:disabled="!canComment"
:disabled-tooltip="i18n.baseText('workflowReviews.detail.activity.composer.notAllowed')"
:submit-disabled="submitDisabled"
data-test-id="workflow-review-comment-composer"
@submit="onSubmit"
/>
</label>
</template>
<style lang="scss" module>
.composer {
display: block;
flex-shrink: 0;
border-top: var(--border);
/* The panel around this clips overflow, and the input draws its focus ring outside its own
box, so without clearance on these three sides the ring is cut off. */
padding: var(--spacing--sm) var(--spacing--3xs) var(--spacing--3xs);
}
.srOnly {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border: 0;
}
</style>
@@ -64,6 +64,7 @@ function makeDetail(
workflows: [makeWorkflowDetail()],
viewerCanDecide: true,
viewerDecisionIneligibilityReason: null,
viewerCanComment: true,
...overrides,
};
}
@@ -23,6 +23,15 @@ vi.mock('./WorkflowReviewActivityFeed.vue', () => ({
},
}));
vi.mock('./WorkflowReviewCommentComposer.vue', () => ({
default: {
name: 'WorkflowReviewCommentComposer',
props: ['canComment'],
template:
'<div data-test-id="workflow-review-comment-composer" :data-can-comment="canComment" />',
},
}));
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',
);
});
});
@@ -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<WorkflowReviewRequestDetail | null>(() =>
);
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(() => [
</div>
<WorkflowReviewActivityFeed :key="review.id" />
<WorkflowReviewCommentComposer :can-comment="viewerCanComment" />
</div>
<div v-else :class="$style.panel" data-test-id="workflow-review-changes-panel">
@@ -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%;
@@ -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> = {},
): 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<WorkflowReviewActivityEntry, { type: 'comment.created' }> {
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();
});
});
@@ -0,0 +1,112 @@
<script setup lang="ts">
import type { WorkflowReviewActivityEntry, WorkflowReviewActivityMessage } from '@n8n/api-types';
import { N8nAvatar, N8nText } from '@n8n/design-system';
import { useI18n } from '@n8n/i18n';
import TimeAgo from '@/app/components/TimeAgo.vue';
import { formatUserDisplayName } from '../../formatUserDisplayName';
defineProps<{
entry: Extract<WorkflowReviewActivityEntry, { type: 'comment.created' }>;
}>();
const i18n = useI18n();
function authorName(message: WorkflowReviewActivityMessage): string {
return message.createdBy
? formatUserDisplayName(message.createdBy)
: i18n.baseText('workflowReviews.detail.activity.unknownAuthor');
}
</script>
<template>
<div :class="$style.entry">
<div v-for="message in entry.messages" :key="message.id" :class="$style.message">
<N8nAvatar
size="small"
:first-name="message.createdBy?.firstName"
:last-name="message.createdBy?.lastName"
/>
<div :class="$style.content">
<div :class="$style.header">
<N8nText
size="medium"
color="text-base"
:class="$style.line"
data-test-id="workflow-review-activity-comment-author"
>
{{ authorName(message) }}
</N8nText>
<N8nText size="xsmall" color="text-light">
<time
:datetime="message.createdAt"
data-test-id="workflow-review-activity-comment-time"
>
<TimeAgo :date="message.createdAt" />
</time>
</N8nText>
</div>
<N8nText
v-if="message.deletedAt"
size="small"
color="text-light"
:class="$style.deleted"
data-test-id="workflow-review-activity-comment-deleted"
>
{{ i18n.baseText('workflowReviews.detail.activity.comment.deleted') }}
</N8nText>
<N8nText
v-else
size="medium"
color="text-light"
:class="[$style.body, $style.line]"
data-test-id="workflow-review-activity-comment-body"
>
{{ message.body }}
</N8nText>
</div>
</div>
</div>
</template>
<style lang="scss" module>
.entry {
display: flex;
flex-direction: column;
gap: var(--spacing--xs);
}
.message {
display: flex;
align-items: flex-start;
gap: var(--spacing--2xs);
}
.content {
display: flex;
flex-direction: column;
gap: var(--spacing--3xs);
min-width: 0;
}
.header {
display: flex;
align-items: baseline;
gap: var(--spacing--2xs);
}
/* Figma asks for 20px on 14px text; no line-height token gives that ratio. */
.line {
line-height: 20px;
}
.body {
white-space: pre-wrap;
overflow-wrap: anywhere;
}
.deleted {
font-style: italic;
}
</style>
@@ -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<Record<WorkflowReviewActivityType, Record<number, Component>>>;
const registry: ActivityEntryRegistry = {};
const registry: ActivityEntryRegistry = {
'comment.created': { 1: WorkflowReviewActivityComment },
};
export function resolveActivityComponent(entry: WorkflowReviewActivityEntry): Component {
return registry[entry.type]?.[entry.typeVersion] ?? WorkflowReviewActivityFallback;
@@ -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<ListWorkflowReviewActivityResponse>((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<WorkflowReviewActivityEntry>((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 () => {
@@ -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<Error | null>(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,
};
});
@@ -507,5 +507,6 @@ function createDetail(): WorkflowReviewRequestDetail {
workflows: [],
viewerCanDecide: true,
viewerDecisionIneligibilityReason: null,
viewerCanComment: true,
};
}
@@ -808,6 +808,7 @@ function createDetail(
workflows: [],
viewerCanDecide: true,
viewerDecisionIneligibilityReason: null,
viewerCanComment: true,
...overrides,
};
}
@@ -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<WorkflowReviewActivityEntry> {
return await makeRestApiRequest(
context,
'POST',
`/workflow-review-requests/${encodeURIComponent(workflowReviewRequestId)}/comments`,
{ ...payload },
);
}