fix(signals): return all available reviewers, not just the first 100#65029
Conversation
The available_reviewers endpoint sorted org members by first name and sliced
to the first 100, so any org with more than 100 GitHub-linked members silently
lost everyone alphabetically after roughly "M" — both in the default list and
when searching, since the slice happened before the response was returned.
Drop the cap and return the full candidate set (sort retained for stable
ordering). Org membership is tiny in practice (p99 ~11, largest org ~470), so
even the biggest org serialises to well under 100 KB.
Add a named OTel span ("signals.available_reviewers") with candidate/result
counts so the endpoint's latency and size are measurable going forward, and
capture a non-blocking exception when an org exceeds 1200 candidates — a signal
to add real pagination before the unpaginated payload ever gets large.
Generated-By: PostHog Code
Task-Id: d1b4a6ce-de4c-43a6-98a0-4cb45275dde0
|
Hey @pauldambra! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/signals/backend/views.py:971-985
**Threshold capture fires on every unfiltered request, not once**
The comment says "a search-as-you-type session reports at most once," but that only prevents the search sub-requests from firing — every bare (no-`query`) call to the endpoint from an org over 1200 members will call `capture_exception`, which also invokes `logger.exception`. If a picker is opened repeatedly by multiple users, this fires once per open. `posthoganalytics.capture_exception` does not deduplicate events the way Sentry does, and since the message embeds `candidate_count` (which can drift), events won't necessarily fold together. The OTel span already records `candidate_count` on every request, so an alert on that attribute would provide the same signal without the repeated error-log noise.
Reviews (1): Last reviewed commit: "fix(signals): return all available revie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…er org per day Per review feedback: capture_exception logs an exception and isn't deduplicated, so the previous guard (skip only search sub-requests) still fired on every unfiltered request from a >threshold org. Throttle with a 24h cache key so an over-threshold org reports at most once per day, and correct the misleading comment. The span's candidate_count attribute is still recorded on every request for metric-based alerting. Generated-By: PostHog Code Task-Id: d1b4a6ce-de4c-43a6-98a0-4cb45275dde0
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete. See inline comments.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: qa-team (correctness + tests), paul-reviewer, xp-reviewer, security-audit Verdict: ✅ APPROVEPragmatic, well-scoped, well-tested fix. Removing the Key findings
Already addressed (not re-raised)The Greptile P2 thread about the threshold capture firing on every unfiltered request is fixed by commit Convergence
SecurityOrg-scoped via Reviewer summaries
Automated by QA Swarm — not a human review |
|
Reviews (2): Last reviewed commit: "fix(signals): throttle reviewer paginati..." | Re-trigger Greptile |
From qa-swarm review feedback: add coverage for the available_reviewers empty paths — an org with no GitHub-linked members and an unknown team (helper returns None) both return an empty 200 response. Generated-By: PostHog Code Task-Id: d1b4a6ce-de4c-43a6-98a0-4cb45275dde0
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6363a5296a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
The functional change is sound and well-tested, but there is an intentionally unresolved review thread about code comments embedding private operational scale data (p99/largest-org member counts) and change history — both prohibited by project guidelines. The author's own triage reply explicitly left it open for a human to decide before merge.
andrewm4894
left a comment
andrewm4894
left a comment
There was a problem hiding this comment.
yeah we have this limit to 100 thing in a few places and can have strange side effects - ty!
Merge activity
|
2 participants
Problem
Assigning a reviewer in the Self-driving Inbox only surfaced org members whose first name fell in roughly the first half of the alphabet (up to ~"M"), and searching for anyone after that returned nothing — even for people who are definitely in the org. Reported from a Slack thread about a large org.
The cause is here:
available_reviewerssorts members by first name and slices to the first 100 before returning. So any org with >100 GitHub-linked members silently loses the tail of the alphabet, and because the slice runs server-side the frontend can't page past it.Changes
[:100]cap — return the full candidate set (sort retained for stable ordering). The whole member set is already loaded in memory, so the slice only ever truncated; it never saved work. Org membership is tiny in practice, so even the biggest org serialises to well under 100 KB.signals.available_reviewerswithcandidate_count/result_countattributes, so this endpoint's latency and payload size are actually measurable next time (today there's no route-level span for it).A paired frontend change (PostHog/code#2800) forwards the picker's search box to this endpoint's
queryparam so search hits the server.How did you test this code?
I'm an agent (PostHog Code). I added automated tests in
test_signal_report_api.py(TestAvailableReviewersAPI) covering: all members returned past the old 100 cap, server-side search filtering, the threshold capture firing above 1200 / staying silent below it and on search requests, and the empty-org / unknown-team paths. I could not run the suite in my environment (no configured Postgres/ClickHouse dev env);python -m py_compileandruff checkboth pass, and CI will execute the tests.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Investigation started from a Slack thread about the reviewer picker stopping at "M". Traced the data flow front-to-back, then confirmed via the warehouse that org membership is small enough that the full list fits comfortably in one response (well under 100 KB even for the largest orgs) — which ruled out pagination in favour of simply removing the cap, with a 1200 tripwire for the future. Tooling: PostHog Code agent with the PostHog MCP (HogQL
execute-sql, APM span queries). No repo skills were invoked.Created with PostHog Code from a Slack thread