2024394 - Inline comment suggestions aren't shown in the transaction API response

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect, P3)

Description

[Suhaib Mujahid [:suhaib]]

We can see the comment itself, but not the suggested diff changes.

Comment 1

[Dão Gottwald [:dao]]

transaction.search builds its inline transaction fields in DifferentialRevisionInlineTransaction::getFieldValuesForConduit(), which returns diff, path, line, length, replyToCommentPHID and isDone. The suggestion is already sitting on the same row that method reads: differential_transaction_comment.attributes carries an inline.state map holding hasSuggestion and suggestionText (written by PhabricatorDiffInlineCommentContentState). So nothing extra needs loading, and no migration is needed -- existing comments already have the data.

The gap bites hardest on suggestion-only inlines. When a reviewer puts everything into the suggestion and leaves the comment text empty, the transaction comes back with content.raw: "" and no other trace, so an API consumer sees what looks like an empty comment. D319190's comment 1710710 is one of those: the API reports it as blank, while the web UI shows a two-line replacement.

I have a patch that adds hasSuggestion and suggestionText to the inline fields. It uses the same field names bug 2044749 added to feed.for_email.query, and reads attributes['inline.state'] directly, the way bug 2049783 settled on. Suggestions are withheld for removed comments, matching how TransactionSearchConduitAPIMethod blanks content.raw.

A second commit adds isNewFile. A suggestion can anchor to the old side of a changeset, so line/length alone do not say which side the range indexes into, and a consumer cannot apply a suggestion without knowing.

One thing deliberately left out: the "before" lines. The web UI reconstructs those from changeset hunks via needInlineContext, which costs a hunk-loading query per changeset -- too expensive for a paginated transaction feed. A consumer that wants to render the same before/after diff can fetch the diff separately.

PR to follow. I have not run it against a live instance yet.

Comment 2

[BMO Github Automation]

Attachment: [mozilla-conduit/phabricator] Bug 2024394 - Expose inline comment suggestions in transaction.search (#102)

Comment 3

[Dão Gottwald [:dao]]

Taking this. Patch is up as https://github.com/mozilla-conduit/phabricator/pull/102 (attachment 9629833 [details] [review]).

dkl, could you take a look? The PR also needs first-time-contributor approval before the GitHub Actions CI will run on it. I ran the same workflow on my fork in the meantime and all three jobs pass, with the new test case picked up by arc unit: https://github.com/daogottwald/phabricator/actions/runs/32624037126

Comment 4

[BMO Github Automation]

Authored by https://github.com/daogottwald
https://github.com/mozilla-conduit/phabricator/commit/6c7202cf8db63ddf117d1cb34f5da47f38ec8a3e
[master] Bug 2024394 - Expose inline comment suggestions in transaction.search