Summary

Clean up detached views when changing revisions/commits in the diff viewer.

Review Request #14565 — Created Aug. 21, 2025 and submitted Nov. 19, 2025, 4:33 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When we change the revision or commits in the diff viewer, the HTML gets
removed from the DOM. However, any Backbone or other JS objects that were
created in that HTML fragment don’t actually get destroyed or cleaned up.
They end up holding unnecessary memory in the browser.

To address this, whenever we clear the diff viewer we emit a signal that
FileAttachmentReviewableViews will listen to and remove themselves.
Subclasses can override onRemove() to perform any additional cleanup
needed for their review UI.

Testing Done


Ran JS unit tests.
Used in a change where we clean up PDF and Office document review

  UIs in the diff viewer.
Tested changing the revision of a diff viewer that contains binary

  image files.

Commits

Summary	ID
Clean up detached views when changing revisions/commits in the diff viewer. When we render binary file review UIs in the diff viewer, we receive an HTML fragment from the server which we inject into the diff viewer. That HTML includes JavaScript which creates the reviewable view and model objects for the file. When we change the revision or commits in the diff viewer, the HTML gets removed from the DOM. However, any Backbone or other JS objects that were created in that HTML fragment don’t actually get destroyed or cleaned up. They end up holding unnecessary memory in the browser. To address this, whenever we clear the diff viewer we emit a signal that FileAttachmentReviewableViews will listen to and clean themselves up. By default the view will remove itself. Subclasses can override the method to perform any additional cleanup needed for their review UI.	85b203203020aa1ff6fe712531d8170b8e037d4d

Summary

Clean up detached views when changing revisions/commits in the diff viewer.

When we render binary file review UIs in the diff viewer, we receive an HTML fragment from the server which we inject into the diff viewer. That HTML includes JavaScript which creates the reviewable view and model objects for the file. When we change the revision or commits in the diff viewer, the HTML gets removed from the DOM. However, any Backbone or other JS objects that were created in that HTML fragment don’t actually get destroyed or cleaned up. They end up holding unnecessary memory in the browser. To address this, whenever we clear the diff viewer we emit a signal that FileAttachmentReviewableViews will listen to and clean themselves up. By default the view will remove itself. Subclasses can override the method to perform any additional cleanup needed for their review UI.

85b203203020aa1ff6fe712531d8170b8e037d4d

Issues

Description	From	Last Updated
Nice and simple. Glad you caught this. I wonder if it's worth having a dedicated cleanup() method or if we …	chipx86	Aug. 22, 2025, 5:54 a.m.
Should be 7.1.x.	chipx86	Aug. 21, 2025, 9:51 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Nice and simple. Glad you caught this.
I wonder if it's worth having a dedicated cleanup() method or if we should just let remove() handle it. Subclasses can already override remove() (and Spina classes now have onRemove() more formerly).
If there are cases where we wouldn't want to remove the view but have other cleanup, then a dedicated method makes sense.

maubin Aug. 21, 2025, 9:45 a.m.

Hmm good point. I can't think of any cases where we'd want to cleanup and not remove the view, so I'll let remove() handle it.

reviewboard/static/rb/js/reviews/views/fileAttachmentReviewableView.ts (Diff revision 1)
The issue has been dropped. Show all issues
```
Should be 7.1.x.
```

Change Summary:

Use remove() directly instead of a new cleanup() method to do the same thing.

Description:

		When we render binary file review UIs in the diff viewer, we receive an
		HTML fragment from the server which we inject into the diff viewer. That
		HTML includes JavaScript which creates the reviewable view and model objects
		for the file.

		When we change the revision or commits in the diff viewer, the HTML gets
		removed from the DOM. However, any Backbone or other JS objects that were
		created in that HTML fragment don’t actually get destroyed or cleaned up.
		They end up holding unnecessary memory in the browser.

		To address this, whenever we clear the diff viewer we emit a signal that
~		FileAttachmentReviewableViews will listen to and clean themselves up. By
~		default the view will remove itself. Subclasses can override the method
~		to perform any additional cleanup needed for their review UI.
	~	FileAttachmentReviewableViews will listen to and remove themselves.
	~	Subclasses can override `onRemove()` to perform any additional cleanup
	~	needed for their review UI.

Commits:

	Summary	ID
	Clean up detached views when changing revisions/commits in the diff viewer. When we render binary file review UIs in the diff viewer, we receive an HTML fragment from the server which we inject into the diff viewer. That HTML includes JavaScript which creates the reviewable view and model objects for the file. When we change the revision or commits in the diff viewer, the HTML gets removed from the DOM. However, any Backbone or other JS objects that were created in that HTML fragment don’t actually get destroyed or cleaned up. They end up holding unnecessary memory in the browser. To address this, whenever we clear the diff viewer we emit a signal that FileAttachmentReviewableViews will listen to and clean themselves up. By default the view will remove itself. Subclasses can override the method to perform any additional cleanup needed for their review UI.	ec2d663ad11392f85368856591f8323d067d1306
	Clean up detached views when changing revisions/commits in the diff viewer. When we render binary file review UIs in the diff viewer, we receive an HTML fragment from the server which we inject into the diff viewer. That HTML includes JavaScript which creates the reviewable view and model objects for the file. When we change the revision or commits in the diff viewer, the HTML gets removed from the DOM. However, any Backbone or other JS objects that were created in that HTML fragment don’t actually get destroyed or cleaned up. They end up holding unnecessary memory in the browser. To address this, whenever we clear the diff viewer we emit a signal that FileAttachmentReviewableViews will listen to and clean themselves up. By default the view will remove itself. Subclasses can override the method to perform any additional cleanup needed for their review UI.	85b203203020aa1ff6fe712531d8170b8e037d4d

Diff:

Revision 2 (+54)

Show changes

	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/reviews/views/fileAttachmentReviewableView.ts

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 (2a114f7)