Summary

Allow ReviewRequestPageData to process draft diff comments.

Review Request #14561 — Created Aug. 11, 2025 and updated Sept. 23, 2025, 8:04 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

We recently re-worked our ReviewRequestPageData to reduce the amount of
queries we were doing. In that change, we added some processing for diff
comments where we attach pre-loaded diffsets back to the comments.
However, we weren't pre-loading the draft diffset of the review request,
and didn't consider the draft diffset when processing the comments.

So if a user was viewing a draft review request and they added a draft
diff comment, this would cause the comment processing to crash on the next
page load. This change includes the draft diffset in our list of loaded
diffsets on the ReviewRequestPageData if the draft is accessible by the
requesting user, allowing the draft diffset to be attached to comments
during processing.

Testing Done


Ran unit tests.
Viewed a draft review request that had a draft diffset on it. Created

  a diff comment and hard reloaded the page. Made sure the page didn't

  crash, whereas previously it would crash with a key error in

diffsets_by_id.
Viewed a review request that has a draft as an anonymous user. Made

  sure that I couldn't see the draft, and couldn't see any draft diff

  comments.
Created a draft review request that had a draft diffset and draft

  comment on it. Viewed the draft review requets as a superuser/admin

  that does not own the draft comment/review, made sure that the admin

  could not see the draft diff comment or review.

Commits

Summary	ID
Allow ReviewRequestPageData to process draft diff comments. We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.	228fccade1b4b7f5224fbe73718f7f058b936d04

Summary

Allow ReviewRequestPageData to process draft diff comments.

We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.

228fccade1b4b7f5224fbe73718f7f058b936d04

Issues

Description	From	Last Updated
I think we should still keep this as a single assignment for the initialization. There's no good reason to create …	david	Aug. 11, 2025, 8:19 p.m.
One last thing. Since this is happing at runtime and 7.1.x still supports python 3.8, we need to use cast(List[...], …	david	Aug. 12, 2025, 9:03 p.m.
I saw the discussion, but looking into this, I think we should copy the list instead of casting. The reason …	chipx86	Sept. 18, 2025, 10:57 p.m.

flake8 passed.

JSHint passed.

reviewboard/reviews/detail.py (Diff revision 1)

The issue has been resolved. Show all issues

I think we should still keep this as a single assignment for the initialization. There's no good reason to create an empty list and then extend.

maubin Aug. 11, 2025, 7:35 p.m.

I had to do this workaround because review_request.get_diffsets() is typed as a Sequence, so if I just had diffsets = review_request.get_diffsets() Mypy would complain when I append to the diffsets below.

david Aug. 11, 2025, 8:40 p.m.

We could do either of:

diffsets = cast(list[DiffSet], review_request.get_diffsets())


or

diffsets = list(review_request.get_diffsets())

maubin Aug. 11, 2025, 8:46 p.m.

Ok, I'll go with the casting since there's no runtime cost.

Commits:

	Summary	ID
	Allow ReviewRequestPageData to process draft diff comments. We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.	850de5d663600e6748f1ea0e8bcb5ee20aeb660d
	Allow ReviewRequestPageData to process draft diff comments. We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.	7e227c95f42dd988465a0bc648e26292fd51fa53

Diff:

Revision 2 (+232 -4)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/tests/test_review_request_page_data.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/detail.py (Diff revision 2)

The issue has been resolved. Show all issues

One last thing. Since this is happing at runtime and 7.1.x still supports python 3.8, we need to use cast(List[...], ...) (capital L)

Commits:

	Summary	ID
	Allow ReviewRequestPageData to process draft diff comments. We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.	7e227c95f42dd988465a0bc648e26292fd51fa53
	Allow ReviewRequestPageData to process draft diff comments. We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.	1b4e0a2dad992e78863cb82fe875819f7c22447d

Diff:

Revision 3 (+236 -10)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/tests/test_review_request_page_data.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/reviews/detail.py (Diff revision 3)

The issue has been resolved. Show all issues

I saw the discussion, but looking into this, I think we should copy the list instead of casting.

The reason is that get_diffsets() caches the result across calls, to avoid queries for multiple calls. Manipulating this list will end up affecting any other callers unexpectedly.

Change Summary:

Copy the list instead of casting it, so that we're not mutating the potentially cached list.

Commits:

	Summary	ID
	Allow ReviewRequestPageData to process draft diff comments. We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.	1b4e0a2dad992e78863cb82fe875819f7c22447d
	Allow ReviewRequestPageData to process draft diff comments. We recently re-worked our `ReviewRequestPageData` to reduce the amount of queries we were doing. In that change, we added some processing for diff comments where we attach pre-loaded diff sets back to the comments. However, we weren't pre-loading the draft diffset of the review request. So if a user was viewing a draft review request and they added a draft diff comment, this would cause the comment processing to crash since the comment's diffset couldn't be found. This change includes the draft diffset in our list of loaded diffsets if the draft is accessible by the requesting user.	228fccade1b4b7f5224fbe73718f7f058b936d04

Diff:

Revision 4 (+228 -2)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/tests/test_review_request_page_data.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```