Summary

Update diff viewer to not use $.funcQueue.

Review Request #14102 — Created Aug. 13, 2024 and updated Sept. 11, 2024, 9:52 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

3938

Depends On

Reviewers

Groups

reviewboard

People

Description

This change introduces a new PromiseQueue, which replaces our old
$.funcQueue system. This works very similarly, where we can add a
bunch of items to the queue and then start it, potentially adding items
even as its running. The big difference is that the individual functions
don't need to care at all that they're running within the queue or ask
the queue to move on to the next item, because they're just promises or
async functions.

This also adds an abort signal, which is both handled internally in the
queue and also passed through to any functions that are included. This
fixes one of our long-standing bugs where rapid switching between
interdiff revisions or commit ranges would cause the displayed diffs to
be incorrect, showing files multiple times or diffs from the wrong
revisions.

Testing Done


Ran js-tests.
Loaded a ton of different diffs and switched rapidly between revisions

  and commit ranges. Saw that the correct files were always loaded and

  we never got weird duplicates.

Commits

Summary	ID
Update diff viewer to not use $.funcQueue. This change introduces a new `PromiseQueue`, which replaces our old `$.funcQueue` system. This works very similarly, where we can add a bunch of items to the queue and then start it, potentially adding items even as its running. The big difference is that the individual functions don't need to care at all that they're running within the queue or ask the queue to move on to the next item, because they're just promises or async functions. This also adds an abort signal, which is both handled internally in the queue and also passed through to any functions that are included. This fixes one of our long-standing bugs where rapid switching between interdiff revisions or commit ranges would cause the displayed diffs to be incorrect, showing files multiple times or diffs from the wrong revisions. Testing Done: - Ran js-tests. - Loaded a ton of different diffs and switched rapidly between revisions and commit ranges. Saw that the correct files were always loaded and we never got weird duplicates. Bugs Fixed: 3938	aa0377d95f091422df73bf0e45301033651a4472

Summary

Update diff viewer to not use $.funcQueue.

This change introduces a new `PromiseQueue`, which replaces our old `$.funcQueue` system. This works very similarly, where we can add a bunch of items to the queue and then start it, potentially adding items even as its running. The big difference is that the individual functions don't need to care at all that they're running within the queue or ask the queue to move on to the next item, because they're just promises or async functions. This also adds an abort signal, which is both handled internally in the queue and also passed through to any functions that are included. This fixes one of our long-standing bugs where rapid switching between interdiff revisions or commit ranges would cause the displayed diffs to be incorrect, showing files multiple times or diffs from the wrong revisions. Testing Done: - Ran js-tests. - Loaded a ton of different diffs and switched rapidly between revisions and commit ranges. Saw that the correct files were always loaded and we never got weird duplicates. Bugs Fixed: 3938

aa0377d95f091422df73bf0e45301033651a4472

Issues

Description	From	Last Updated
This looks great, but it definitely needs thorough unit testing of the queue itself. (Getting flashbacks to both funcQueue and …	chipx86	Sept. 11, 2024, 9:52 a.m.
console.error?	chipx86	Sept. 11, 2024, 9:33 a.m.
Needs to be a single-line summary.	chipx86	Sept. 11, 2024, 9:34 a.m.

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

The issue has been resolved. Show all issues

This looks great, but it definitely needs thorough unit testing of the queue itself. (Getting flashbacks to both funcQueue and SlotChain debugging nightmares.)

david Sept. 11, 2024, 9:52 a.m.

Added testing for all of the different permutations I can think of. The use of promises makes this all a lot simpler than the old funcQueue, which is very nice.

reviewboard/static/rb/js/common/utils/promiseQueue.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
console.error?
```
reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Needs to be a single-line summary.
```

Commits:

	Summary	ID
	Update diff viewer to not use $.funcQueue. This change introduces a new `PromiseQueue`, which replaces our old `$.funcQueue` system. This works very similarly, where we can add a bunch of items to the queue and then start it, potentially adding items even as its running. The big difference is that the individual functions don't need to care at all that they're running within the queue or ask the queue to move on to the next item, because they're just promises or async functions. This also adds an abort signal, which is both handled internally in the queue and also passed through to any functions that are included. This fixes one of our long-standing bugs where rapid switching between interdiff revisions or commit ranges would cause the displayed diffs to be incorrect, showing files multiple times or diffs from the wrong revisions. Testing Done: - Ran js-tests. - Loaded a ton of different diffs and switched rapidly between revisions and commit ranges. Saw that the correct files were always loaded and we never got weird duplicates. Bugs Fixed: 3938	74ef367f5d16ddc136f46474dd89a4de85f8f098
	Update diff viewer to not use $.funcQueue. This change introduces a new `PromiseQueue`, which replaces our old `$.funcQueue` system. This works very similarly, where we can add a bunch of items to the queue and then start it, potentially adding items even as its running. The big difference is that the individual functions don't need to care at all that they're running within the queue or ask the queue to move on to the next item, because they're just promises or async functions. This also adds an abort signal, which is both handled internally in the queue and also passed through to any functions that are included. This fixes one of our long-standing bugs where rapid switching between interdiff revisions or commit ranges would cause the displayed diffs to be incorrect, showing files multiple times or diffs from the wrong revisions. Testing Done: - Ran js-tests. - Loaded a ton of different diffs and switched rapidly between revisions and commit ranges. Saw that the correct files were always loaded and we never got weird duplicates. Bugs Fixed: 3938	aa0377d95f091422df73bf0e45301033651a4472

Diff:

Revision 2 (+602 -36)

Show changes

	reviewboard/static/rb/js/common/utils/index.ts
	reviewboard/static/rb/js/common/utils/promiseQueue.ts
	reviewboard/static/rb/js/common/utils/tests/index.ts
	reviewboard/static/rb/js/common/utils/tests/promiseQueueTests.ts
	reviewboard/static/rb/js/reviews/models/diffViewerPageModel.ts
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.