Summary

Only add static media once per bundle in the diff viewer.

Review Request #14545 — Created Aug. 1, 2025 and submitted Aug. 21, 2025, 9:07 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

We're able to display certain binary files in our diff viewer. A binary
file is associated with a review UI, which renders the file. Some Review
UIs have static media bundles which get loaded onto the webpage.
Previously, we'd load these bundles unconditionally. However, now that
we use review UIs in the diff viewer, we've discovered that the static
media of a review UI gets loaded multiple times onto the page, depending
on how many instances of the review UI are in the diff viewer.

This lead to a problem with Power Pack's PDF and Office Document review
UIs in the diff viewer. The review UIs include a bundle for the pdf.js
module, which handles PDF rendering. This module sets up some global
state that is meant to facilitate rendering multiple PDF documents on
one page. However, since we were reloading the review UI's static
bundles everytime a new instance was added to the page, this global
state would get overwritten each time which would lead to PDF rendering
issues.

This change updates the diff viewer page to keep note of what static
bundles have already been loaded onto the page, and to skip loading any
existing bundles when rendering a new diff reviewable onto the diff
viewer. This ensures that any state from the static media of a review
UI will be shared between all instances of that review UI type on the
page. And we're avoiding unnecessary requests for loading the duplicate
static media.

Testing Done


Ran unit tests (no new failures).
Ran JS unit tests (no new failures).
Tested with multiple PDF and Office documents in the diff viewer,

  as well as non-binary files.
Tested changing the first and last commits in the diff viewer.
Tested with only non-binary files in the diff viewer.
Tested viewing standalone review UI pages.

Commits

Summary	ID
Only add static media once per review UI type in the diff viewer. We're able to display certain binary files in our diff viewer. A binary file is associated with a review UI, which renders the file. Some Review UIs have static media bundles which get loaded onto the webpage. Previously, we'd load these bundles unconditionally. However, now that we use review UIs in the diff viewer, we've discovered that the static media of a review UI gets loaded multiple times onto the page, depending on how many instances of the review UI are in the diff viewer. This lead to a problem with Power Pack's PDF and Office Document review UIs in the diff viewer. The review UIs include a bundle for the pdf.js module, which handles PDF rendering. This module sets up some global state that is meant to facilitate rendering multiple PDF documents on one page. However, since we were reloading the review UI's static bundles everytime a new instance was added to the page, this global state would get overwritten each time which would lead to PDF rendering issues. This change updates the diff viewer page to keep note of what static bundles have already been loaded onto the page, and to skip loading any existing bundles when rendering a new diff reviewable onto the diff viewer. This ensures that any state from the static media of a review UI will be shared between all instances of that review UI type on the page. And we're avoiding unnecessary requests for loading the duplicate static media.	1a5cbe3f69f3cdc641d5c16949b7ab36d21fb7ac

Summary

Only add static media once per review UI type in the diff viewer.

We're able to display certain binary files in our diff viewer. A binary file is associated with a review UI, which renders the file. Some Review UIs have static media bundles which get loaded onto the webpage. Previously, we'd load these bundles unconditionally. However, now that we use review UIs in the diff viewer, we've discovered that the static media of a review UI gets loaded multiple times onto the page, depending on how many instances of the review UI are in the diff viewer. This lead to a problem with Power Pack's PDF and Office Document review UIs in the diff viewer. The review UIs include a bundle for the pdf.js module, which handles PDF rendering. This module sets up some global state that is meant to facilitate rendering multiple PDF documents on one page. However, since we were reloading the review UI's static bundles everytime a new instance was added to the page, this global state would get overwritten each time which would lead to PDF rendering issues. This change updates the diff viewer page to keep note of what static bundles have already been loaded onto the page, and to skip loading any existing bundles when rendering a new diff reviewable onto the diff viewer. This ensures that any state from the static media of a review UI will be shared between all instances of that review UI type on the page. And we're avoiding unnecessary requests for loading the duplicate static media.

1a5cbe3f69f3cdc641d5c16949b7ab36d21fb7ac

Issues

Description	From	Last Updated
Elsewhere, we name this css_bundles and the JavaScript js_bundles. Maybe css_bundle_names and js_bundle_names would be appropriate here. That'll help differentiate …	chipx86	Aug. 12, 2025, 2:23 a.m.
The diff viewer code doesn't have anything else about attachments or review UIs. This is probably better done in reviewboard/reviews/views/diffviewer.py:ReviewsDiffViewerView.get_context_data, …	david	Aug. 12, 2025, 9:23 p.m.
I assume there's a circular import if we put this at the module level?	chipx86	Aug. 12, 2025, 2:22 a.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 1, 2025, 2:32 a.m.
You could combine the statements for these.	chipx86	Aug. 12, 2025, 3:27 a.m.
Same comments here.	chipx86	Aug. 12, 2025, 3:27 a.m.
This should be safe to import at the file level now.	david	Aug. 14, 2025, 11:03 p.m.
And probably these to match.	chipx86	Aug. 12, 2025, 3:27 a.m.
No need for the backticks here (or really the one below, but it was already like that).	chipx86	Aug. 12, 2025, 3:27 a.m.
New unit tests can use () => instead of function(). We used to use function() because older Jasmine required that, …	chipx86	Aug. 12, 2025, 3:28 a.m.
We should be documenting these. The others didn't get documented, but at least for new ones it'd be worth doing …	chipx86	Aug. 12, 2025, 3:32 a.m.
Can you swap these so they're in alphabetical order?	chipx86	Aug. 12, 2025, 3:31 a.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 14, 2025, 11:40 p.m.
This should go below the next option (alphabetical).	chipx86	Aug. 21, 2025, 8:59 a.m.
This line's too long.	chipx86	Aug. 21, 2025, 9 a.m.
Can you add a blank line between these, so it doesn't get lost with the commented one?	chipx86	Aug. 21, 2025, 9:01 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```

reviewboard/diffviewer/diffutils.py (Diff revision 1)

The issue has been resolved. Show all issues

The diff viewer code doesn't have anything else about attachments or review UIs. This is probably better done in reviewboard/reviews/views/diffviewer.py:ReviewsDiffViewerView.get_context_data, where we iterate over context['files'] and add a bunch of additional stuff.

css_media and js_media would then become fields of SerializedReviewsDiffFile (defined in that same file)

Looks great. I just have comments on naming and some small modernization nits.

reviewboard/diffviewer/diffutils.py (Diff revision 1)

The issue has been dropped. Show all issues

Elsewhere, we name this css_bundles and the JavaScript js_bundles. Maybe css_bundle_names and js_bundle_names would be appropriate here. That'll help differentiate from, say, arbitrary CSS or JavaScript URLs.

maubin Aug. 7, 2025, 8:20 p.m.

I went with _media because we have an old ReviewUI.js_files attribute where people can list JavaScript URLs to include on the page instead of bundle names. So technically the js_media set can include arbitrary JavaScript URLs. The css_media will only include bundle names though.

chipx86 Aug. 12, 2025, 2:23 a.m.

Ah okay, I didn't see that was getting populated. Sounds good then.

reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
I assume there's a circular import if we put this at the module level?
```
1. maubin Aug. 7, 2025, 8:20 p.m.
  That's right.
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
You could combine the statements for these.
```
reviewboard/reviews/views/diffviewer.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Same comments here.
```
reviewboard/static/rb/js/reviews/models/diffFileModel.ts (Diff revision 1)
The issue has been dropped. Show all issues
```
And probably these to match.
```
reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for the backticks here (or really the one below, but it was already like that).
```

reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts (Diff revision 1)

The issue has been resolved. Show all issues

New unit tests can use () => instead of function().
We used to use function() because older Jasmine required that, but now () => is recommended.
Same with other tests.

reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts (Diff revision 1)

The issue has been resolved. Show all issues

We should be documenting these. The others didn't get documented, but at least for new ones it'd be worth doing so intent is clear.

reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you swap these so they're in alphabetical order?
```

Commits:

	Summary	ID
	Only add static media once per review UI type in the diff viewer. We're able to display certain binary files in our diff viewer. A binary file is associated with a review UI, which renders the file. Some Review UIs have static media bundles which get loaded onto the webpage. Previously, we'd load these bundles unconditionally. However, now that we use review UIs in the diff viewer, we've discovered that the static media of a review UI gets loaded multiple times onto the page, depending on how many instances of the review UI are in the diff viewer. This lead to a problem with Power Pack's PDF and Office Document review UIs in the diff viewer. The review UIs include a bundle for the pdf.js module, which handles PDF rendering. This module sets up some global state that is meant to facilitate rendering multiple PDF documents on one page. However, since we were reloading the review UI's static bundles everytime a new instance was added to the page, this global state would get overwritten each time which would lead to PDF rendering issues. This change updates the diff viewer page to keep note of what static bundles have already been loaded onto the page, and to skip loading any existing bundles when rendering a new diff reviewable onto the diff viewer. This ensures that any state from the static media of a review UI will be shared between all instances of that review UI type on the page. And we're avoiding unnecessary requests for loading the duplicate static media.	58f0c143b4dd3d9fc23f5a42685b401366c35f28
	Only add static media once per review UI type in the diff viewer. We're able to display certain binary files in our diff viewer. A binary file is associated with a review UI, which renders the file. Some Review UIs have static media bundles which get loaded onto the webpage. Previously, we'd load these bundles unconditionally. However, now that we use review UIs in the diff viewer, we've discovered that the static media of a review UI gets loaded multiple times onto the page, depending on how many instances of the review UI are in the diff viewer. This lead to a problem with Power Pack's PDF and Office Document review UIs in the diff viewer. The review UIs include a bundle for the pdf.js module, which handles PDF rendering. This module sets up some global state that is meant to facilitate rendering multiple PDF documents on one page. However, since we were reloading the review UI's static bundles everytime a new instance was added to the page, this global state would get overwritten each time which would lead to PDF rendering issues. This change updates the diff viewer page to keep note of what static bundles have already been loaded onto the page, and to skip loading any existing bundles when rendering a new diff reviewable onto the diff viewer. This ensures that any state from the static media of a review UI will be shared between all instances of that review UI type on the page. And we're avoiding unnecessary requests for loading the duplicate static media.	e3cd17ac5c048c48a6fd3d0797271d5da9481492

Diff:

Revision 2 (+1088 -56)

Show changes

	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/tests/test_reviews_diff_viewer_view.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/static/rb/js/reviews/models/diffFileModel.ts
	reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts
	reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts
	reviewboard/static/rb/js/reviews/models/tests/diffReviewableModelTests.ts
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/views/diffviewer.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
This should be safe to import at the file level now.
```

Change Summary:

Added an optimization where we try take advantage of already loaded file attachments instead of hitting the database, when possible.

Commits:

	Summary	ID
	Only add static media once per review UI type in the diff viewer. We're able to display certain binary files in our diff viewer. A binary file is associated with a review UI, which renders the file. Some Review UIs have static media bundles which get loaded onto the webpage. Previously, we'd load these bundles unconditionally. However, now that we use review UIs in the diff viewer, we've discovered that the static media of a review UI gets loaded multiple times onto the page, depending on how many instances of the review UI are in the diff viewer. This lead to a problem with Power Pack's PDF and Office Document review UIs in the diff viewer. The review UIs include a bundle for the pdf.js module, which handles PDF rendering. This module sets up some global state that is meant to facilitate rendering multiple PDF documents on one page. However, since we were reloading the review UI's static bundles everytime a new instance was added to the page, this global state would get overwritten each time which would lead to PDF rendering issues. This change updates the diff viewer page to keep note of what static bundles have already been loaded onto the page, and to skip loading any existing bundles when rendering a new diff reviewable onto the diff viewer. This ensures that any state from the static media of a review UI will be shared between all instances of that review UI type on the page. And we're avoiding unnecessary requests for loading the duplicate static media.	e3cd17ac5c048c48a6fd3d0797271d5da9481492
	Only add static media once per review UI type in the diff viewer. We're able to display certain binary files in our diff viewer. A binary file is associated with a review UI, which renders the file. Some Review UIs have static media bundles which get loaded onto the webpage. Previously, we'd load these bundles unconditionally. However, now that we use review UIs in the diff viewer, we've discovered that the static media of a review UI gets loaded multiple times onto the page, depending on how many instances of the review UI are in the diff viewer. This lead to a problem with Power Pack's PDF and Office Document review UIs in the diff viewer. The review UIs include a bundle for the pdf.js module, which handles PDF rendering. This module sets up some global state that is meant to facilitate rendering multiple PDF documents on one page. However, since we were reloading the review UI's static bundles everytime a new instance was added to the page, this global state would get overwritten each time which would lead to PDF rendering issues. This change updates the diff viewer page to keep note of what static bundles have already been loaded onto the page, and to skip loading any existing bundles when rendering a new diff reviewable onto the diff viewer. This ensures that any state from the static media of a review UI will be shared between all instances of that review UI type on the page. And we're avoiding unnecessary requests for loading the duplicate static media.	1a5cbe3f69f3cdc641d5c16949b7ab36d21fb7ac

Diff:

Revision 3 (+1118 -62)

Show changes

	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/tests/test_reviews_diff_viewer_view.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/static/rb/js/reviews/models/diffFileModel.ts
	reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts
	reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts
	reviewboard/static/rb/js/reviews/models/tests/diffReviewableModelTests.ts
	3 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```

Ship it!

```
Ship It!
```

Ship it!

Tiny things, but everything looks good! Ship it after this.

reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
This should go below the next option (alphabetical).
```
reviewboard/static/rb/js/reviews/models/tests/diffReviewableModelTests.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
This line's too long.
```
reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you add a blank line between these, so it doesn't get lost with the commented one?
```

Status:: Completed
Change Summary:: Pushed to release-7.1.x (47c5d7e)