feat(core): Return open and closed counts for the review inbox tabs (#34789)

This commit is contained in:
Sandra Zollner
2026-07-24 11:26:39 +00:00
committed by GitHub
parent 9503892225
commit f4edeeba31
11 changed files with 141 additions and 79 deletions
@@ -35,5 +35,6 @@ export interface ListWorkflowReviewInboxResponse {
}
export interface GetWorkflowReviewInboxSummaryResponse {
hasAny: boolean;
open: number;
closed: number;
}
@@ -22,10 +22,12 @@ describe('WorkflowReviewRequestRepository', () => {
queryBuilder.orderBy.mockReturnThis();
queryBuilder.addOrderBy.mockReturnThis();
queryBuilder.select.mockReturnThis();
queryBuilder.addSelect.mockReturnThis();
queryBuilder.groupBy.mockReturnThis();
queryBuilder.take.mockReturnThis();
queryBuilder.limit.mockReturnThis();
queryBuilder.getMany.mockResolvedValue([]);
queryBuilder.getRawOne.mockResolvedValue(undefined);
queryBuilder.getRawMany.mockResolvedValue([]);
vi.spyOn(repo, 'createQueryBuilder').mockReturnValue(queryBuilder);
});
@@ -222,67 +224,62 @@ describe('WorkflowReviewRequestRepository', () => {
});
});
describe('existsAnyForInbox', () => {
it('queries by requester only when projectIds is empty', async () => {
queryBuilder.getRawOne.mockResolvedValueOnce({ '?column?': 1 });
describe('countByStateForInbox', () => {
it('groups by state applying the requester-only filter when projectIds is empty', async () => {
queryBuilder.getRawMany.mockResolvedValueOnce([{ state: 'open', count: '2' }]);
const result = await repo.existsAnyForInbox({ projectIds: [], requesterId: 'user-1' });
const result = await repo.countByStateForInbox({ projectIds: [], requesterId: 'user-1' });
expect(result).toBe(true);
expect(result).toEqual({ open: 2, closed: 0 });
expect(repo.createQueryBuilder).toHaveBeenCalledWith('review');
expect(queryBuilder.select).toHaveBeenCalledWith('1');
expect(queryBuilder.select).toHaveBeenCalledWith('review.state', 'state');
expect(queryBuilder.addSelect).toHaveBeenCalledWith('COUNT(*)', 'count');
expect(queryBuilder.groupBy).toHaveBeenCalledWith('review.state');
expect(queryBuilder.where).toHaveBeenCalledWith('review.createdById = :requesterId', {
requesterId: 'user-1',
});
expect(queryBuilder.limit).toHaveBeenCalledWith(1);
});
it('skips the projectId filter when projectIds is null', async () => {
queryBuilder.getRawOne.mockResolvedValueOnce({ '?column?': 1 });
it('skips the projectId filter when projectIds is null (global scope counts all)', async () => {
queryBuilder.getRawMany.mockResolvedValueOnce([
{ state: 'open', count: '3' },
{ state: 'closed', count: '12' },
]);
const result = await repo.existsAnyForInbox({
projectIds: null,
requesterId: 'user-1',
state: 'open',
});
const result = await repo.countByStateForInbox({ projectIds: null, requesterId: 'user-1' });
expect(result).toBe(true);
expect(repo.createQueryBuilder).toHaveBeenCalledWith('review');
expect(queryBuilder.select).toHaveBeenCalledWith('1');
expect(result).toEqual({ open: 3, closed: 12 });
expect(queryBuilder.where).not.toHaveBeenCalled();
expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.state = :state', {
state: 'open',
});
expect(queryBuilder.limit).toHaveBeenCalledWith(1);
expect(queryBuilder.groupBy).toHaveBeenCalledWith('review.state');
});
it('returns true when at least one matching row exists', async () => {
queryBuilder.getRawOne.mockResolvedValueOnce({ '?column?': 1 });
it('matches projects OR the requester', async () => {
queryBuilder.getRawMany.mockResolvedValueOnce([
{ state: 'open', count: 1 },
{ state: 'closed', count: 4 },
]);
const result = await repo.existsAnyForInbox({
projectIds: ['proj-1'],
const result = await repo.countByStateForInbox({
projectIds: ['proj-1', 'proj-2'],
requesterId: 'user-1',
state: 'open',
});
expect(result).toBe(true);
expect(queryBuilder.select).toHaveBeenCalledWith('1');
expect(result).toEqual({ open: 1, closed: 4 });
expect(queryBuilder.where).toHaveBeenCalledWith(
'(review.projectId IN (:...projectIds) OR review.createdById = :requesterId)',
{ projectIds: ['proj-1'], requesterId: 'user-1' },
{ projectIds: ['proj-1', 'proj-2'], requesterId: 'user-1' },
);
expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.state = :state', {
state: 'open',
});
expect(queryBuilder.limit).toHaveBeenCalledWith(1);
});
it('returns false when no rows match', async () => {
queryBuilder.getRawOne.mockResolvedValueOnce(undefined);
it('defaults absent states to zero', async () => {
queryBuilder.getRawMany.mockResolvedValueOnce([]);
await expect(
repo.existsAnyForInbox({ projectIds: ['proj-1'], requesterId: 'user-1' }),
).resolves.toBe(false);
const result = await repo.countByStateForInbox({
projectIds: ['proj-1'],
requesterId: 'user-1',
});
expect(result).toEqual({ open: 0, closed: 0 });
});
});
});
@@ -28,12 +28,16 @@ export type FindManyForInboxOptions = {
cursor?: InboxCursor;
};
export type ExistsAnyForInboxOptions = {
export type CountByStateForInboxOptions = {
/** `null` means all projects (no filter); `[]` means no publish-scoped projects. */
projectIds: string[] | null;
/** Requesters always see the reviews they created, regardless of project scope. */
requesterId: string;
state?: WorkflowReviewRequestState;
};
export type InboxStateCounts = {
open: number;
closed: number;
};
@Service()
@@ -148,19 +152,25 @@ export class WorkflowReviewRequestRepository extends Repository<WorkflowReviewRe
return await queryBuilder.getMany();
}
async existsAnyForInbox(options: ExistsAnyForInboxOptions): Promise<boolean> {
const { projectIds, requesterId, state } = options;
async countByStateForInbox(options: CountByStateForInboxOptions): Promise<InboxStateCounts> {
const { projectIds, requesterId } = options;
const queryBuilder = this.createQueryBuilder('review').select('1');
const queryBuilder = this.createQueryBuilder('review')
.select('review.state', 'state')
.addSelect('COUNT(*)', 'count')
.groupBy('review.state');
this.applyInboxVisibility(queryBuilder, projectIds, requesterId);
if (state !== undefined) {
queryBuilder.andWhere('review.state = :state', { state });
}
const rows = await queryBuilder.getRawMany<{
state: WorkflowReviewRequestState;
count: string | number;
}>();
const row = await queryBuilder.limit(1).getRawOne<Record<string, unknown>>();
return row !== undefined;
return {
open: Number(rows.find((row) => row.state === 'open')?.count ?? 0),
closed: Number(rows.find((row) => row.state === 'closed')?.count ?? 0),
};
}
/**
@@ -772,31 +772,31 @@ async function seedInboxRequests() {
}
describe('GET /workflow-review-requests/summary', () => {
test('returns hasAny true for instance owner', async () => {
test('returns open/closed counts for instance owner', async () => {
await seedInboxRequests();
const response = await ownerAgent.get('/workflow-review-requests/summary').expect(200);
expect(response.body.data).toEqual({ hasAny: true });
expect(response.body.data).toEqual({ open: 1, closed: 1 });
});
test('returns hasAny true for project editor', async () => {
test('returns open/closed counts for project editor', async () => {
await seedInboxRequests();
const response = await memberAgent.get('/workflow-review-requests/summary').expect(200);
expect(response.body.data).toEqual({ hasAny: true });
expect(response.body.data).toEqual({ open: 1, closed: 1 });
});
test('returns hasAny false for project viewer', async () => {
test('returns zero counts for project viewer with nothing visible', async () => {
await seedInboxRequests();
const response = await viewerAgent.get('/workflow-review-requests/summary').expect(200);
expect(response.body.data).toEqual({ hasAny: false });
expect(response.body.data).toEqual({ open: 0, closed: 0 });
});
test('returns hasAny true for a requester of their own review', async () => {
test('counts a requester their own review regardless of project scope', async () => {
await requestRepository.createRequest({
projectId: teamProject.id,
title: 'Review submitted by viewer',
@@ -806,7 +806,7 @@ describe('GET /workflow-review-requests/summary', () => {
const response = await viewerAgent.get('/workflow-review-requests/summary').expect(200);
expect(response.body.data).toEqual({ hasAny: true });
expect(response.body.data).toEqual({ open: 1, closed: 0 });
});
test('returns 403 when feature is disabled', async () => {
@@ -307,13 +307,10 @@ export class WorkflowReviewRequestService {
await this.assertFeatureAvailable();
const projectIds = await this.resolveAccessibleProjectIds(user);
// Any state — sidebar must appear so users can open the Closed tab.
const hasAny = await this.workflowReviewRequestRepository.existsAnyForInbox({
return await this.workflowReviewRequestRepository.countByStateForInbox({
projectIds,
requesterId: user.id,
});
return { hasAny };
}
private async assertFeatureAvailable(): Promise<void> {
@@ -223,6 +223,13 @@ const scrollRight = () => scroll(50);
color: var(--color--primary);
}
/* Inset outline so the ring isn't clipped by the scroll container and adds no layout shift. */
&:focus-visible {
outline: var(--border-width) solid var(--focus--border-color);
outline-offset: calc(-1 * var(--border-width));
border-radius: var(--radius--3xs);
}
span + span {
margin-left: var(--spacing--4xs);
}
@@ -5167,9 +5167,11 @@
"workflowReviews.disclaimer.body": "Submit workflow versions for review before publishing. When reviews are assigned to your projects, they will appear here.",
"workflowReviews.sidebar.tabs.open": "Open",
"workflowReviews.sidebar.tabs.closed": "Closed",
"workflowReviews.sidebar.empty": "No reviews in this tab",
"workflowReviews.sidebar.empty.open": "No open reviews",
"workflowReviews.sidebar.empty.closed": "No closed reviews",
"workflowReviews.detail.placeholder": "Full review details are coming soon.",
"workflowReviews.emptyState.body": "There are no reviews in this tab.",
"workflowReviews.emptyState.body.open": "There are no open reviews right now.",
"workflowReviews.emptyState.body.closed": "There are no closed reviews right now.",
"workflowReviews.noSelection.body": "Select a review to see more information.",
"workflowReviews.error.load": "Could not load workflow reviews",
"settings.security.personalSpace.title": "Personal Space",
@@ -17,6 +17,8 @@ import TimeAgo from '@/app/components/TimeAgo.vue';
const props = defineProps<{
items: WorkflowReviewInboxItem[];
activeState: WorkflowReviewRequestState;
openCount: number;
closedCount: number;
selectedId: string | null;
loading: boolean;
loadingMore: boolean;
@@ -36,8 +38,16 @@ const listRef = ref<HTMLElement | null>(null);
const loadMoreSentinel = ref<HTMLElement | null>(null);
const tabOptions = computed(() => [
{ label: i18n.baseText('workflowReviews.sidebar.tabs.open'), value: 'open' as const },
{ label: i18n.baseText('workflowReviews.sidebar.tabs.closed'), value: 'closed' as const },
{
label: i18n.baseText('workflowReviews.sidebar.tabs.open'),
value: 'open' as const,
tag: String(props.openCount),
},
{
label: i18n.baseText('workflowReviews.sidebar.tabs.closed'),
value: 'closed' as const,
tag: String(props.closedCount),
},
]);
const { observe: observeForLoadMore } = useIntersectionObserver({
@@ -78,7 +88,6 @@ function onListBackgroundClick() {
<N8nTabs
:model-value="activeState"
:options="tabOptions"
variant="modern"
data-test-id="workflow-reviews-tabs"
@update:model-value="onTabChange"
/>
@@ -99,7 +108,7 @@ function onListBackgroundClick() {
size="small"
data-test-id="workflow-reviews-empty"
>
{{ i18n.baseText('workflowReviews.sidebar.empty') }}
{{ i18n.baseText(`workflowReviews.sidebar.empty.${activeState}`) }}
</N8nText>
<N8nCard
v-for="item in items"
@@ -12,9 +12,10 @@ describe('useReviewInboxStore', () => {
vi.resetAllMocks();
});
it('probes summary and loads open reviews when hasAny is true', async () => {
it('probes summary and loads open reviews when counts are non-zero', async () => {
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInboxSummary).mockResolvedValue({
hasAny: true,
open: 3,
closed: 12,
});
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInbox).mockResolvedValue({
data: [
@@ -38,23 +39,48 @@ describe('useReviewInboxStore', () => {
expect(workflowReviewsApi.fetchWorkflowReviewInboxSummary).toHaveBeenCalledTimes(1);
expect(store.hasAnyReviews).toBe(true);
expect(store.openCount).toBe(3);
expect(store.closedCount).toBe(12);
expect(store.probeSettled).toBe(true);
expect(store.showSidebar).toBe(true);
expect(store.items).toHaveLength(1);
});
it('skips list fetch when summary hasAny is false', async () => {
it('skips list fetch when both counts are zero', async () => {
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInboxSummary).mockResolvedValue({
hasAny: false,
open: 0,
closed: 0,
});
const store = useReviewInboxStore();
await store.probeInbox();
expect(workflowReviewsApi.fetchWorkflowReviewInbox).not.toHaveBeenCalled();
expect(store.hasAnyReviews).toBe(false);
expect(store.openCount).toBe(0);
expect(store.closedCount).toBe(0);
expect(store.showSidebar).toBe(false);
});
it('shows the sidebar when only closed reviews exist', async () => {
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInboxSummary).mockResolvedValue({
open: 0,
closed: 5,
});
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInbox).mockResolvedValue({
data: [],
nextCursor: null,
hasMore: false,
});
const store = useReviewInboxStore();
await store.probeInbox();
expect(store.hasAnyReviews).toBe(true);
expect(store.showSidebar).toBe(true);
expect(workflowReviewsApi.fetchWorkflowReviewInbox).toHaveBeenCalledTimes(1);
});
it('refetches when switching tabs', async () => {
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInbox).mockResolvedValue({
data: [],
@@ -119,8 +145,8 @@ describe('useReviewInboxStore', () => {
});
it('does not apply probe results after reset', async () => {
let resolveSummary!: (value: { hasAny: boolean }) => void;
const summaryPromise = new Promise<{ hasAny: boolean }>((resolve) => {
let resolveSummary!: (value: { open: number; closed: number }) => void;
const summaryPromise = new Promise<{ open: number; closed: number }>((resolve) => {
resolveSummary = resolve;
});
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInboxSummary).mockImplementationOnce(
@@ -130,7 +156,7 @@ describe('useReviewInboxStore', () => {
const store = useReviewInboxStore();
const probe = store.probeInbox();
store.reset();
resolveSummary({ hasAny: true });
resolveSummary({ open: 1, closed: 0 });
await probe;
expect(store.probeSettled).toBe(false);
@@ -193,7 +219,8 @@ describe('useReviewInboxStore', () => {
it('does not treat a failed list fetch as an empty inbox', async () => {
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInboxSummary).mockResolvedValue({
hasAny: true,
open: 1,
closed: 0,
});
vi.mocked(workflowReviewsApi.fetchWorkflowReviewInbox).mockRejectedValue(new Error('network'));
@@ -21,6 +21,8 @@ export const useReviewInboxStore = defineStore('workflowReviewInbox', () => {
const probeSettled = ref(false);
const hasAnyReviews = ref(false);
const openCount = ref(0);
const closedCount = ref(0);
const items = ref<WorkflowReviewInboxItem[]>([]);
const selectedId = ref<string | null>(null);
const activeState = ref<WorkflowReviewRequestState>('open');
@@ -69,10 +71,12 @@ export const useReviewInboxStore = defineStore('workflowReviewInbox', () => {
return;
}
hasAnyReviews.value = summary.hasAny;
openCount.value = summary.open;
closedCount.value = summary.closed;
hasAnyReviews.value = summary.open + summary.closed > 0;
probeSettled.value = true;
if (summary.hasAny) {
if (hasAnyReviews.value) {
await fetchList({ reset: true });
}
} catch (e) {
@@ -167,6 +171,8 @@ export const useReviewInboxStore = defineStore('workflowReviewInbox', () => {
listRequestSeq += 1;
probeSettled.value = false;
hasAnyReviews.value = false;
openCount.value = 0;
closedCount.value = 0;
items.value = [];
selectedId.value = null;
activeState.value = 'open';
@@ -180,6 +186,8 @@ export const useReviewInboxStore = defineStore('workflowReviewInbox', () => {
return {
probeSettled,
hasAnyReviews,
openCount,
closedCount,
items,
selectedId,
selectedItem,
@@ -23,6 +23,8 @@ const {
loadingMore,
hasMore,
isEmpty,
openCount,
closedCount,
} = storeToRefs(store);
const i18n = useI18n();
@@ -76,6 +78,8 @@ onUnmounted(() => {
v-if="showSidebar"
:items="items"
:active-state="activeState"
:open-count="openCount"
:closed-count="closedCount"
:selected-id="selectedId"
:loading="loading"
:loading-more="loadingMore"
@@ -133,7 +137,7 @@ onUnmounted(() => {
size="medium"
data-test-id="workflow-reviews-empty-state"
>
{{ i18n.baseText('workflowReviews.emptyState.body') }}
{{ i18n.baseText(`workflowReviews.emptyState.body.${activeState}`) }}
</N8nText>
<N8nText
v-else