Skip to content

feat(inbox): Explose signals report data in the detail pane#53685

Merged
sortafreel merged 3 commits intomasterfrom
signals/expose-artefacts
Apr 8, 2026
Merged

feat(inbox): Explose signals report data in the detail pane#53685
sortafreel merged 3 commits intomasterfrom
signals/expose-artefacts

Conversation

@sortafreel
Copy link
Copy Markdown
Contributor

@sortafreel sortafreel commented Apr 8, 2026

Problem

Changes

  • The data is exposed

How did you test this code?

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Publish to changelog?

Docs update

@sortafreel sortafreel changed the title Signals/expose artefacts feat(inbox): Explose signals report data in detail pane Apr 8, 2026
@sortafreel sortafreel marked this pull request as ready for review April 8, 2026 15:53
Copilot AI review requested due to automatic review settings April 8, 2026 15:53
@sortafreel sortafreel changed the title feat(inbox): Explose signals report data in detail pane feat(inbox): Explose signals report data in the detail pane Apr 8, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

Hey @sortafreel! 👋\nThis pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the Signal Reports API payload to expose additional “judgment” data (actionability + already-addressed) for consumption in the inbox UI.

Changes:

  • Prefetch ACTIONABILITY_JUDGMENT artefacts on the SignalReport queryset to avoid per-report queries.
  • Extend SignalReportSerializer with actionability and already_addressed fields derived from the latest actionability judgement artefact.
  • Fix priority field help text to reference the priority judgement artefact.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
products/signals/backend/views.py Adds a filtered prefetch for actionability judgement artefacts to support new serializer fields efficiently.
products/signals/backend/serializers.py Adds actionability + already_addressed fields and shared parsing logic for the latest actionability judgement artefact; corrects priority help text.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 8, 2026

Vulnerabilities

No security concerns identified. Both new actions (artefacts and signals) gate access through self.get_object(), which uses the team-scoped queryset, preventing cross-team data access. The enrich_reviewer_dicts_with_org_members function performs only parameterised ORM queries.

Comments Outside Diff (3)

  1. products/signals/backend/views.py, line 509 (link)

    P2 Missing serializer context

    SignalReportSerializer is instantiated here without the serializer context, unlike the usage on line 548 which passes context=self.get_serializer_context(). While the serializer doesn't currently access the context, the inconsistency means any future addition of context-dependent logic to SignalReportSerializer will silently produce wrong results for this endpoint only.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: products/signals/backend/views.py
    Line: 509
    
    Comment:
    **Missing serializer context**
    
    `SignalReportSerializer` is instantiated here without the serializer context, unlike the usage on line 548 which passes `context=self.get_serializer_context()`. While the serializer doesn't currently access the context, the inconsistency means any future addition of context-dependent logic to `SignalReportSerializer` will silently produce wrong results for this endpoint only.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. products/signals/backend/views.py, line 492-502 (link)

    P2 Unpaginated artefacts response

    report.artefacts.all() returns every artefact for the report with no page limit. For reports with many artefacts (e.g. repeated reingestions producing large priority/actionability artefact histories), this could return a very large payload. Consider adding pagination or a reasonable [:N] cap, consistent with how other list endpoints in the codebase handle potentially unbounded result sets.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: products/signals/backend/views.py
    Line: 492-502
    
    Comment:
    **Unpaginated artefacts response**
    
    `report.artefacts.all()` returns every artefact for the report with no page limit. For reports with many artefacts (e.g. repeated reingestions producing large priority/actionability artefact histories), this could return a very large payload. Consider adding pagination or a reasonable `[:N]` cap, consistent with how other list endpoints in the codebase handle potentially unbounded result sets.
    
    How can I resolve this? If you propose a fix, please make it concise.
  3. products/signals/backend/serializers.py, line 209-219 (link)

    P2 Per-artefact DB queries in get_content

    enrich_reviewer_dicts_with_org_members issues multiple DB queries (Team → OrganizationMembership → UserSocialAuth) each time a SUGGESTED_REVIEWERS artefact is serialized. When SignalReportArtefactSerializer is used with many=True in the artefacts action and there happen to be multiple SUGGESTED_REVIEWERS artefacts for a report, each one triggers those queries independently.

    In practice this is likely only one artefact per report, so it's rarely a problem, but if the artefacts list grows (e.g. after multiple reingestions), the N+1 will become visible. Caching the login_to_user map in the serializer context or passing it in as a pre-built argument would eliminate the risk entirely.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: products/signals/backend/serializers.py
    Line: 209-219
    
    Comment:
    **Per-artefact DB queries in `get_content`**
    
    `enrich_reviewer_dicts_with_org_members` issues multiple DB queries (Team → OrganizationMembership → UserSocialAuth) each time a `SUGGESTED_REVIEWERS` artefact is serialized. When `SignalReportArtefactSerializer` is used with `many=True` in the `artefacts` action and there happen to be multiple `SUGGESTED_REVIEWERS` artefacts for a report, each one triggers those queries independently.
    
    In practice this is likely only one artefact per report, so it's rarely a problem, but if the artefacts list grows (e.g. after multiple reingestions), the N+1 will become visible. Caching the `login_to_user` map in the serializer context or passing it in as a pre-built argument would eliminate the risk entirely.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: products/signals/backend/views.py
Line: 509

Comment:
**Missing serializer context**

`SignalReportSerializer` is instantiated here without the serializer context, unlike the usage on line 548 which passes `context=self.get_serializer_context()`. While the serializer doesn't currently access the context, the inconsistency means any future addition of context-dependent logic to `SignalReportSerializer` will silently produce wrong results for this endpoint only.

```suggestion
        report_data = SignalReportSerializer(report, context=self.get_serializer_context()).data
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: products/signals/backend/views.py
Line: 492-502

Comment:
**Unpaginated artefacts response**

`report.artefacts.all()` returns every artefact for the report with no page limit. For reports with many artefacts (e.g. repeated reingestions producing large priority/actionability artefact histories), this could return a very large payload. Consider adding pagination or a reasonable `[:N]` cap, consistent with how other list endpoints in the codebase handle potentially unbounded result sets.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: products/signals/backend/serializers.py
Line: 209-219

Comment:
**Per-artefact DB queries in `get_content`**

`enrich_reviewer_dicts_with_org_members` issues multiple DB queries (Team → OrganizationMembership → UserSocialAuth) each time a `SUGGESTED_REVIEWERS` artefact is serialized. When `SignalReportArtefactSerializer` is used with `many=True` in the `artefacts` action and there happen to be multiple `SUGGESTED_REVIEWERS` artefacts for a report, each one triggers those queries independently.

In practice this is likely only one artefact per report, so it's rarely a problem, but if the artefacts list grows (e.g. after multiple reingestions), the N+1 will become visible. Caching the `login_to_user` map in the serializer context or passing it in as a pre-built argument would eliminate the risk entirely.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Merge branch 'master' into signals/expos..." | Re-trigger Greptile

@sortafreel sortafreel requested a review from a team April 8, 2026 16:12
Copy link
Copy Markdown
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

No complaints

@sortafreel sortafreel merged commit 1a547d7 into master Apr 8, 2026
222 of 223 checks passed
@sortafreel sortafreel deleted the signals/expose-artefacts branch April 8, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants