Skip to content

refactor(core): Extract participant resolution from the review inbox service (no-changelog) - #36918

Open
kgrhartlage wants to merge 1 commit into
masterfrom
ligo-1042-extract-participant-resolution-from-the-review-inbox-service
Open

refactor(core): Extract participant resolution from the review inbox service (no-changelog)#36918
kgrhartlage wants to merge 1 commit into
masterfrom
ligo-1042-extract-participant-resolution-from-the-review-inbox-service

Conversation

@kgrhartlage

@kgrhartlage kgrhartlage commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

WorkflowReviewInboxService did two jobs: it assembled the inbox and detail payloads, and it resolved who is involved in each review. This PR moves the second job into a new WorkflowReviewParticipantResolver.

The resolver has one entry point, resolve(requests). It owns the reviewer, author, and user repositories. It batches the two junction queries, does one deduplicated user lookup, and returns the requester, the authors, and the reviewers for each request id. resolve returns a small lookup object with a for(requestId) method, so the "no entry" fallback stays inside the resolver.

The inbox service now injects the resolver instead of the three repositories. The list view and the detail view both use it. hydrateParticipants and resolveParticipants are removed, and toInboxItem takes one participants object instead of three positional parameters.

The change is behavior preserving. The query fan-out is unchanged: the list still resolves participants in the same Promise.all as the linked-workflow lookup, and the detail still resolves them in the same Promise.all as the workflow details and the eligibility check.

Test changes:

  • New workflow-review-participant.resolver.test.ts holds the participant behavior. It covers the requester, the authors, the reviewers, the keying across a batch, the deduplicated single user lookup, a deleted user that drops out, an unresolvable requester, and a request id outside the batch.
  • The two inbox test files replace three repository mocks with one resolver mock. Each keeps one test that asserts the resolved participants reach the item or the detail. The list test keeps the check that participants are resolved for the page only, and never for the lookahead row.

How to test

This is an internal refactor with no API change. The inbox and detail responses are identical.

Automated:

cd packages/cli
pnpm test src/modules/workflow-reviews.ee
pnpm test:integration src/modules/workflow-reviews.ee

Result: 243 unit tests pass and 240 integration tests pass. The integration run includes the 169-test workflow reviews controller suite, which exercises the participants end to end.

Manual (optional): the workflow reviews module needs N8N_ENABLED_MODULES=workflow-reviews and an enterprise license with the workflow reviews feature. Open the review inbox and then open a review. The requester, the authors, and the reviewers must show as before.

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/LIGO-1042

Review / Merge checklist

  • I have seen this code, I have run this code, and I take responsibility for this code.
  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with Backport to Beta, Backport to Stable, or Backport to v1 (if the PR is an urgent fix that needs to be backported)

🤖 PR Summary generated by AI

Review in cubic

…e (no-changelog)

Move requester/author/reviewer resolution out of WorkflowReviewInboxService
into a WorkflowReviewParticipantResolver with a single resolve(requests)
entry point. The resolver owns the reviewer, author and user repositories,
so deleted-user handling and the user projection live in one place and both
the inbox list and the detail view share them.
@kgrhartlage

Copy link
Copy Markdown
Contributor Author

@cubic-dev-ai roast my PR

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai roast my PR

@kgrhartlage I have started the AI code review. It will take a few minutes to complete.

@n8n-assistant

n8n-assistant Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

PR review overview

Based on ownership of the 5 changed files in this PR:

Ownership Files owned Share Source code Test files Misc
@n8n-io/catalysts 5 100% +117 / -113 +177 / -139 +0 / -0
Total 5 100% +117 / -113 +177 / -139 +0 / -0

@n8n-assistant n8n-assistant Bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Aug 24, 2026
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 5 files

Architecture diagram
sequenceDiagram
    participant Client as Client (Frontend)
    participant InboxService as WorkflowReviewInboxService
    participant Gate as WorkflowReviewFeatureGate
    participant Resolver as NEW: WorkflowReviewParticipantResolver
    participant JunctionRepos as Reviewer & Author Repositories
    participant UserRepo as UserRepository

    Note over Client,UserRepo: Workflow Review Inbox / Detail Flow

    Client->>InboxService: getDetail(user, requestId) OR listForInbox(user)
    
    InboxService->>Gate: isWorkflowReviewsLicensed()
    Gate-->>InboxService: boolean

    alt Feature Enabled
        InboxService->>InboxService: Fetch Request/Workflow data from DB
        
        InboxService->>Resolver: NEW: resolve(WorkflowReviewRequest[])
        
        activate Resolver
        Resolver->>JunctionRepos: findByRequestIds(ids)
        JunctionRepos-->>Resolver: Junction Rows (Reviewers/Authors)
        
        Resolver->>Resolver: Deduplicate User IDs (Requester + Authors + Reviewers)
        
        opt User IDs present
            Resolver->>UserRepo: findManyByIds(uniqueIds)
            UserRepo-->>Resolver: User entities
        end
        
        Resolver-->>InboxService: NEW: WorkflowReviewParticipantLookup object
        deactivate Resolver

        InboxService->>InboxService: CHANGED: Map items using lookup.for(requestId)
        
        Note right of InboxService: lookup.for() provides fallback defaults <br/>if request is missing from batch.

        InboxService-->>Client: 200 OK (InboxItem or Detail)
    else Feature Disabled
        InboxService-->>Client: Error / Empty State
    end
Loading

Re-trigger cubic

@kgrhartlage
kgrhartlage marked this pull request as ready for review August 24, 2026 12:28

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 5 files

Architecture diagram
sequenceDiagram
    participant Client as Client (Frontend)
    participant InboxService as WorkflowReviewInboxService
    participant Gate as WorkflowReviewFeatureGate
    participant Resolver as NEW: WorkflowReviewParticipantResolver
    participant JunctionRepos as Reviewer & Author Repositories
    participant UserRepo as UserRepository

    Note over Client,UserRepo: Workflow Review Inbox / Detail Flow

    Client->>InboxService: getDetail(user, requestId) OR listForInbox(user)
    
    InboxService->>Gate: isWorkflowReviewsLicensed()
    Gate-->>InboxService: boolean

    alt Feature Enabled
        InboxService->>InboxService: Fetch Request/Workflow data from DB
        
        InboxService->>Resolver: NEW: resolve(WorkflowReviewRequest[])
        
        activate Resolver
        Resolver->>JunctionRepos: findByRequestIds(ids)
        JunctionRepos-->>Resolver: Junction Rows (Reviewers/Authors)
        
        Resolver->>Resolver: Deduplicate User IDs (Requester + Authors + Reviewers)
        
        opt User IDs present
            Resolver->>UserRepo: findManyByIds(uniqueIds)
            UserRepo-->>Resolver: User entities
        end
        
        Resolver-->>InboxService: NEW: WorkflowReviewParticipantLookup object
        deactivate Resolver

        InboxService->>InboxService: CHANGED: Map items using lookup.for(requestId)
        
        Note right of InboxService: lookup.for() provides fallback defaults <br/>if request is missing from batch.

        InboxService-->>Client: 200 OK (InboxItem or Detail)
    else Feature Disabled
        InboxService-->>Client: Error / Empty State
    end
Loading

Re-trigger cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant