Skip to content

fix(api): re-land review-summaries API on main (recover stranded #3011)#3046

Merged
Pulkit7070 merged 3 commits into
mainfrom
feat/reviews-api-onto-main
Jul 23, 2026
Merged

fix(api): re-land review-summaries API on main (recover stranded #3011)#3046
Pulkit7070 merged 3 commits into
mainfrom
feat/reviews-api-onto-main

Conversation

@codebanditssss

Copy link
Copy Markdown
Collaborator

Summary

Re-lands the session PR review-summaries API on main. #3011 was merged into its stale stacked base (feat/scm-fetch-pr-review-body) instead of main, so its changes never reached main even though it shows as merged — main has the persist (#3009) and fetch (#3010) layers but nothing exposes the review summaries through the session PR API.

This branch cherry-picks the three #3011 commits onto current main:

  • feat(api): expose provider PR review summaries on the session PR APIsummarizeReview populates Reviews (latest decisive review per reviewer, with body), plus the SessionPRReviewEntry DTO and optional reviews field.
  • fix(api): surface approved review summaries, not just changes-requested — Pulkit7070's finding: build Reviews from latestDecisiveReviews so an approved review's body is surfaced too.
  • fix(api): register SessionPRReviewEntry schema name — illegalcall's finding: register the type in schemaNames so it emits as SessionPRReviewEntry, not ControllersSessionPRReviewEntry.

Supersedes #3011 (which is unrecoverable as-is since it merged into a dead branch).

Verification

  • cd backend && go build ./... clean; gofmt clean
  • go test ./internal/service/session/ ./internal/httpd/controllers/ ./internal/httpd/apispec/... pass
  • Regenerated openapi/schema from source — no api-drift

Surface each reviewer's latest decisive review (verdict + submitted body)
through the session PR summary so the reviews UI can show a verdict with
its summary text.

- PRReviewSummary gains Reviews []PRReviewEntry; summarizeReview populates
  it from the latest approved/changes_requested review per reviewer,
  sorted by reviewer for determinism
- controller DTO gains SessionPRReviewEntry and a reviews field (omitempty,
  so it stays optional for existing clients)
- openapi.yaml + schema.ts regenerated

Inline review comment bodies stay private, folded into UnresolvedBy counts
and links as before; only the top-level review summary body is exposed.
Part of the reviews-tab revamp (#2433); the frontend two-section layout
that consumes this is the next PR.
summarizeReview built the Reviews list from latestChangesRequestedReviews,
which drops every reviewer whose latest decisive verdict is approved, so an
approved review's summary body was never exposed despite the field
advertising the approved verdict. Split out latestDecisiveReviews (latest
approved-or-changes_requested per reviewer) and build Reviews from it, while
the unresolved-comment grouping keeps using the changes-requested subset.

Add a service test asserting an approved review's body is surfaced and a
controller test covering the domain-to-DTO field mapping (Reviewer ->
reviewerId, URL -> reviewUrl, etc.).
The reflected DTO surfaced as ControllersSessionPRReviewEntry, leaking the
internal package prefix into the public OpenAPI spec and generated
TypeScript. Register it in schemaNames so it emits as SessionPRReviewEntry,
matching the other session PR contract types, and regenerate the spec + client.

@Pulkit7070 Pulkit7070 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving. This correctly re-lands the stranded #3011 API layer onto main and fixes the issue I flagged there: latestDecisiveReviews now surfaces each reviewer's latest approved-or-changes_requested review (not just the changes-requested subset), so approved summaries appear with verdict and body. New dto_test asserts an approved review maps end-to-end. CI is green across all checks.

@Pulkit7070
Pulkit7070 merged commit 36ae4e7 into main Jul 23, 2026
11 checks passed
@illegalcall

Copy link
Copy Markdown
Collaborator

[P1] The recovery is incomplete: re-land #3010’s review-body fetch/mapping path

pr_summary.go now copies review.Body into the API response, but neither current main nor this PR populates that field. The assumption in the PR description that the fetch layer is already on main is incorrect: #3010 (3da9fff85) was merged into feat/persist-pr-review-body, not main.

Concretely:

  • backend/internal/adapters/scm/github/observer_provider.go does not request or map GraphQL body.
  • ports.SCMReviewSummaryObservation has no Body field.
  • domainFromObservation and claimRowsFromSCM omit the body mapping.

Therefore real GitHub refreshes persist Body == "", and omitempty removes body from the API response. Reviewer/verdict data appears, but the submitted review summary—the feature this recovery is meant to restore—does not. The new tests inject Body below the missing provider pipeline, so they do not catch this gap.

Since #3046 is now merged, please re-land #3010’s five-file fetch/mapping commit (including its provider test) in a follow-up PR against main.

@codebanditssss

Copy link
Copy Markdown
Collaborator Author

you are right, good catch. confirmed #3010 (3da9fff) is not an ancestor of main either, and SCMReviewSummaryObservation has no Body field there, so the GraphQL body request + observer/claim mappings were all missing. the API was copying review.Body from an always-empty field. my #3046 tests injected Body above the missing fetch layer so they did not catch it.

re-landed #3010 onto main in #3049 (cherry-picked 3da9fff, the five-file fetch/mapping commit incl. the provider test that actually asserts the query requests+maps body). with #3009 + #3046 + #3049 the body now flows end to end. thanks for catching this.

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