These pathways are still dodging ModularTransactions and thus not getting double writes after D17495:

• The Herald action DifferentialReviewersHeraldAction does not yet use ModularTransactions, and does a direct edge write instead.
• newAutoReviewTransaction(), which adds automatic reviewers from Owners, does a direct edge write.
• Downgrading accepts and rejects (changing "Accepted" into "Accepted Older") uses raw edge transactions.
• Marking reviewers as "commented" uses raw edge transactions. I'd like to make this orthogonal to primary status, though, and this isn't critical to retain through the double write phase, so I may just knock it out for now and restore it later.
• Old-school resigns, accepts, and rejects are still using edges directly (can we get rid of these completely?).

These cases are OK:

• Direct reviewer edits.
• Modern accept/reject/resign.
• The side effect from commandeering.

Old-school resigns, accepts, and rejects are still using edges directly (can we get rid of these completely?).

The blocker here is differential.createcomment. We could just remove it, but I believe it sees a meaningful amount of use (supporting ad-hoc unit test tie-ins from the era before Harbormaster, for example) and I think it's not too complex to keep it working without a compatibility break by silently upgrading accept, reject and resign into ModularTransaction transactions.

In theory we could upgrade everything but that might be a fair amount of work/testing and I think it is realistic to imagine that there are zero use cases for "resign-via-API" or "request-review-via-API" in the wild. Still, maybe this is easy enough to do that it's worthwhile to clean up now.

Beyond that, we still have some ACTION_CLOSE in commit parsing.

The Herald case and the next case (newAutoReviewTransaction()) both have some code duplication around prefiltering reviewers before building transactions for them.

There's at least some logic to this in Herald, since it allows us to generate a more accurate log message for users (i.e., "Herald added reviewers X and Y, but reviewer Z was already a reviewer"). For Owners, there's no direct utility.

I'm not cleaning this up for now because it's slightly tricky. In particular, when you "Edit Revision" and replace "@dog (blocking) with @dog", we want to downgrade the blocking reviewer to become a nonblocking reviewer. However, when a Herald rule or Owners rule adds @dog as a nonblocking reviewer, we don't want to downgrade.

We could add a key like ^ to the map (but: kind of hacky) to deal with this. Or we could add metadata to the transaction. But the extra logic isn't really hurting anything for now and this may be cleaner and easier after we swap storage, so I'm just leaving it untouched for the moment.

Downgrading accepts and rejects (changing "Accepted" into "Accepted Older") uses raw edge transactions.

I'm going to skip double writes here since I think this should become a synthetic state (i.e., state = accepted, diffPHID = some non-current diff) and I think we'll hit only a minor amount of temporary weirdness/ambiguity by not transitioning this directly.

Marking reviewers as "commented" uses raw edge transactions. I'd like to make this orthogonal to primary status, though, and this isn't critical to retain through the double write phase, so I may just knock it out for now and restore it later.

I'm going to skip double writes here too since I think this should also become a synthetic state (state = whatever, hasCommented = true).

I'll put hasCommented and diffPHID state into the new storage table instead.

From D17564, there's a minor workflow issue with "request review" incorrectly putting accepted changes back into an "accepted" state from this.

The workflow thing is somewhat nontrivial. Previously, we would explicitly downgrade "Accept" into "Accepted Older" when an author used "Request Review".

This was sort of a hack, where we treated the "Request Review" like an update without any text. We now represent "Accepted Older" as "Accepted" with a lastActionDiffPHID other than the current diff PHID, which is more accurate but means this hack doesn't work anymore.

The closest analog to the hack would be setting lastActionDiffPHID to a nonsense value, but I don't like this very much since it discards information.

A more authentic way to represent this state is probably to put a flag on each Reviewer like "the author has disavowed this review". We set the flag on all reviewers when we would have downgraded and clear it when a reviewer is touched by a review action. While the flag is set, a matching lastActionDiffPHID isn't good enough to count as a current "Accept".