Summary

Fix caching bugs with comments in inline review UIs.

Review Request #14555 — Created Aug. 5, 2025 and submitted Sept. 4, 2025, 5:11 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change reworks the fragment view to include all comment timestamps
in the ETag when the filediff in question is binary. We had an existing,
similar example in CommentDiffFragmentsView that included the latest
timestamp in the ETag, but this is broken in the case where comments are
deleted or edited on a pending review. I've changed that to include the
same behavior of including all comment timestamps rather than just the
latest.

A couple new tests have been added to verify the cache response
behavior.

Testing Done


Ran unit tests.
Verified that comments on binary files in the diff viewer no longer

  presented with outdated browser cache bugs.

Commits

Summary	ID
Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	0c5ce38f202beeb5e547164ef1f1c2dcba19cca1

Summary

Fix caching bugs with comments in inline review UIs.

When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the *latest* timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.

0c5ce38f202beeb5e547164ef1f1c2dcba19cca1

Issues

Description	From	Last Updated
continuation line under-indented for visual indent Column: 21 Error code: E128	reviewbot	Aug. 5, 2025, 3:09 p.m.
I think this should go away. We just raise errors instead of returning a HttpResponse, and we don't have HttpResponse …	maubin	Aug. 6, 2025, 9:21 a.m.
This is still the same description as the previous test.	maubin	Aug. 6, 2025, 9:21 a.m.
A preceding r was added here.	maubin	Aug. 7, 2025, 10:37 a.m.
This should say diff comments instead of just diff	maubin	Aug. 7, 2025, 10:37 a.m.
We could have a comments variable and set self.comments to that, so that we don't have to keep accessing self.comments …	maubin	Aug. 6, 2025, 9:22 a.m.
Can we add a "Raises" section in the docs for this.	maubin	Aug. 6, 2025, 9:22 a.m.
This should go away too.	maubin	Aug. 6, 2025, 9:23 a.m.
This argument hasn't been added to the function definition, it gets pulled out of kwargs.	maubin	Aug. 6, 2025, 9:23 a.m.
Can we add a "Raises" section in the docs for this.	maubin	Aug. 6, 2025, 9:24 a.m.
During deprecation, new arguments must go on the end so that the decorator can do the right thing. It matches …	chipx86	Aug. 8, 2025, 9:41 a.m.
Description needs to be indented 1.	chipx86	Aug. 7, 2025, 4:54 p.m.
Description needs to be indented 1.	chipx86	Aug. 7, 2025, 4:54 p.m.
What do you think about adding a manager method for this, so we can keep these queries in one place? …	chipx86	Aug. 8, 2025, 10:05 a.m.
Might be nicer as: modified_review_ui_class: ( type[ReviewUI[Any, Any, Any]] \| None ) = None,	chipx86	Aug. 7, 2025, 4:53 p.m.
Can we say "Return"? "Get" doesn't imply the result, just that it's getting something.	chipx86	Aug. 7, 2025, 4:53 p.m.
This needs a "Version Added"	maubin	Aug. 11, 2025, 2:23 p.m.
What about super users who can access the comments?	maubin	Aug. 11, 2025, 1:12 a.m.
Since we're wrapping, this should be one per line.	chipx86	Aug. 21, 2025, 8:12 a.m.
Double-backticks around the parameter name.	chipx86	Aug. 21, 2025, 8:12 a.m.
Missing , optional.	chipx86	Aug. 21, 2025, 8:12 a.m.
Not a new problem, but noticing it now: We're passing arguments in positional form to a localized string, which won't …	chipx86	Aug. 21, 2025, 8:14 a.m.
Double backticks around request.	chipx86	Aug. 21, 2025, 8:13 a.m.
Swap these (alphabetical order).	chipx86	Aug. 21, 2025, 8:13 a.m.
Add double-backticks around request.	chipx86	Aug. 21, 2025, 8:13 a.m.
Can just be 7.1 to match the others.	chipx86	Aug. 21, 2025, 8:13 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/views/diff_fragments.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line under-indented for visual indent

Column: 21
Error code: E128
```

Commits:

	Summary	ID
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	dda2c270e1759a02e23b230113cd3053944dc1e8
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	0f5813e7a0e5cf105cef12135d267d8ea885a22e

Diff:

Revision 2 (+874 -172)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/reviews/views/diff_fragments.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Thanks for fixing this :D.
```

reviewboard/diffviewer/views.py (Diff revision 2)

The issue has been resolved. Show all issues

I think this should go away. We just raise errors instead of returning a HttpResponse, and we don't have HttpResponse as a return type on the method.

reviewboard/reviews/tests/test_reviews_diff_fragment_view.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This is still the same description as the previous test.
```

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)

The issue has been resolved. Show all issues

We could have a comments variable and set self.comments to that, so that we don't have to keep accessing self.comments every time.

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we add a "Raises" section in the docs for this.
```
reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should go away too.
```

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)

The issue has been resolved. Show all issues

This argument hasn't been added to the function definition, it gets pulled out of kwargs.

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we add a "Raises" section in the docs for this.
```

Commits:

	Summary	ID
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	0f5813e7a0e5cf105cef12135d267d8ea885a22e
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	22833569602b0fef51527d1522dc814c1794a93f

Diff:

Revision 3 (+896 -198)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/reviews/views/diff_fragments.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/reviews/views/diff_fragments.py (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
A preceding r was added here.
```
reviewboard/reviews/views/diff_fragments.py (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
This should say diff comments instead of just diff
```
1. david Aug. 7, 2025, 10:39 a.m.
  Could be either. I'll reword.

Commits:

	Summary	ID
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	22833569602b0fef51527d1522dc814c1794a93f
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	5221448f51bb9248e8a61d08d97cd7408791a76c

Diff:

Revision 4 (+894 -196)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/reviews/views/diff_fragments.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/views.py (Diff revision 4)

The issue has been resolved. Show all issues

During deprecation, new arguments must go on the end so that the decorator can do the right thing. It matches up the positions of arguments passed to the position of keyword-only arguments in the arg list.

reviewboard/reviews/views/diff_fragments.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Description needs to be indented 1.
```
reviewboard/reviews/views/diff_fragments.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Description needs to be indented 1.
```

reviewboard/reviews/views/diff_fragments.py (Diff revision 4)

The issue has been resolved. Show all issues

What do you think about adding a manager method for this, so we can keep these queries in one place? Like, FileAttachmentCommentManager.objects.for_file_attachment(), taking the attachments and a user.

Been thinking lately that if we could consolidate more of our queries, it'd be easier to maintain and update them when working on performance, and ensure unit tests are more comprehensive.

It's not a blocker for this change.

reviewboard/reviews/views/diff_fragments.py (Diff revision 4)

The issue has been resolved. Show all issues

Might be nicer as:

modified_review_ui_class: (
    type[ReviewUI[Any, Any, Any]] |
    None
) = None,

reviewboard/reviews/views/diff_fragments.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we say "Return"? "Get" doesn't imply the result, just that it's getting something.
```

Commits:

	Summary	ID
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	5221448f51bb9248e8a61d08d97cd7408791a76c
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	cc4e986c7d46b049f8372a4d203f413d03dbd9f5

Diff:

Revision 5 (+1022 -206)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/reviews/views/diff_fragments.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/managers.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This needs a "Version Added"
```

reviewboard/reviews/managers.py (Diff revision 5)

The issue has been dropped. Show all issues

What about super users who can access the comments?

maubin Aug. 11, 2025, 1:14 a.m.

Oops never mind, super users aren't supposed to be able to see other people's draft reviews.

Commits:

	Summary	ID
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	cc4e986c7d46b049f8372a4d203f413d03dbd9f5
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	a5b02dd428d37884c2ac53bd5bba20d451c9fea3

Diff:

Revision 6 (+1030 -206)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/reviews/views/diff_fragments.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/diffviewer/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Since we're wrapping, this should be one per line.
```
reviewboard/diffviewer/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Double-backticks around the parameter name.
```
reviewboard/diffviewer/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Missing , optional.
```

reviewboard/diffviewer/views.py (Diff revision 6)

The issue has been dropped. Show all issues

Not a new problem, but noticing it now: We're passing arguments in positional form to a localized string, which won't work if those get reversed.

This should probably be a separate change, but we should fix this to use named arguments.

david Aug. 21, 2025, 8:15 a.m.

Will handle this in my diffviewer changes for master.

reviewboard/diffviewer/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Double backticks around request.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Swap these (alphabetical order).
```
reviewboard/reviews/views/diff_fragments.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Add double-backticks around request.
```
reviewboard/reviews/views/diff_fragments.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can just be 7.1 to match the others.
```

Commits:

	Summary	ID
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	a5b02dd428d37884c2ac53bd5bba20d451c9fea3
	Fix caching bugs with comments in inline review UIs. When review UIs for binary files are rendered in "inline" mode as part of the diff viewer, the serialized comments are included in the rendered fragment (as opposed to comments on regular diff files, which are all included as part of the main diff viewer page). Our ETag calculation wasn't taking this into account, which meant that after making changes to comments (adding, removing, editing), reloading the diff viewer could show outdated comments coming from the browser cache. This change reworks the fragment view to include all comment timestamps in the ETag when the filediff in question is binary. We had an existing, similar example in `CommentDiffFragmentsView` that included the latest timestamp in the ETag, but this is broken in the case where comments are deleted or edited on a pending review. I've changed that to include the same behavior of including all comment timestamps rather than just the latest. A couple new tests have been added to verify the cache response behavior. Testing Done: - Ran unit tests. - Verified that comments on binary files in the diff viewer no longer presented with outdated browser cache bugs.	0c5ce38f202beeb5e547164ef1f1c2dcba19cca1

Diff:

Revision 7 (+1038 -208)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/reviews/views/diff_fragments.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.1.x (02608ae)