Summary

Add a communication channel for forcing reloads across tab sessions.

Review Request #13048 — Created May 16, 2023 and submitted May 30, 2023, 11:54 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

The new behavior for the unified banner means that individual tabs fetch

and keep track of pending review/draft IDs more eagerly than before. The

result of this is that it was easier to hit bugs when performing actions

with a review request open in multiple tabs.
This change adds a small utility that makes use of the

BroadcastChannel API to coordinate across multiple tabs/windows within

a browser session. Right now the only message that we send is "reload",

which asks other tabs to reload their contents. This is sent when

something is either published or discarded, which invalidates draft IDs.
The broadcast channel is origin-wide, so each PageView can implement a

method to return a data blob that's used for filtering whether or not to

reload. The default implementation is empty, and ReviewablePageView

will filter based on the review request ID (so any open tabs related to

that review request will be triggered).

Testing Done


Ran js-tests.
Published review requests, reviews, and replies from one tab, and saw

  other tabs pop up the alert and reload.
Discarded drafts from the unified banner and from the review dialog,

  and saw other tabs pop up the alert and reload.

Commits

Summary	ID
Add a communication channel for forcing reloads across tab sessions. The new behavior for the unified banner means that individual tabs fetch and keep track of pending review/draft IDs more eagerly than before. The result of this is that it was easier to hit bugs when performing actions with a review request open in multiple tabs. This change adds a small utility that makes use of the `BroadcastChannel` API to coordinate across multiple tabs/windows within a browser session. Right now the only message that we send is "reload", which asks other tabs to reload their contents. This is sent when something is either published or discarded, which invalidates draft IDs. The broadcast channel is origin-wide, so each `PageView` can implement a method to return a data blob that's used for filtering whether or not to reload. The default implementation is empty, and `ReviewablePageView` will filter based on the review request ID (so any open tabs related to that review request will be triggered). Testing Done: - Ran js-tests. - Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload. - Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.	1bbc4c4e7e9ba3cdd2e68c13c93a619834cb6771

Summary

Add a communication channel for forcing reloads across tab sessions.

The new behavior for the unified banner means that individual tabs fetch and keep track of pending review/draft IDs more eagerly than before. The result of this is that it was easier to hit bugs when performing actions with a review request open in multiple tabs. This change adds a small utility that makes use of the `BroadcastChannel` API to coordinate across multiple tabs/windows within a browser session. Right now the only message that we send is "reload", which asks other tabs to reload their contents. This is sent when something is either published or discarded, which invalidates draft IDs. The broadcast channel is origin-wide, so each `PageView` can implement a method to return a data blob that's used for filtering whether or not to reload. The default implementation is empty, and `ReviewablePageView` will filter based on the review request ID (so any open tabs related to that review request will be triggered). Testing Done: - Ran js-tests. - Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload. - Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.

1bbc4c4e7e9ba3cdd2e68c13c93a619834cb6771

Issues

Description	From	Last Updated
We're going to need tests for this, which will be especially important for any filtering logic and any future use …	chipx86	May 24, 2023, 10:01 a.m.
I also want to discuss naming. Eventually, we'll want an open comm channel to the Review Board server as well. …	chipx86	May 24, 2023, 10:01 a.m.
Can you add a Version Added?	chipx86	May 19, 2023, 2:41 p.m.
This is missing docs. Should also have Version Added, which I've unfortunately left out of my other TS reviews.	chipx86	May 19, 2023, 2:41 p.m.
Missing Version Added.	chipx86	May 19, 2023, 2:41 p.m.
Should document this.	maubin	May 25, 2023, 9:35 p.m.
Should document this.	maubin	May 25, 2023, 9:35 p.m.
I think you could get rid of this and replace it with some info about how we're going to trigger …	maubin	May 25, 2023, 9:38 p.m.

flake8 passed.

JSHint passed.

So I think BroadcastChannel is origin-wide, so a reload is going to end up applying to every other tab regardless of review request. We'd need to bake in some identifying information into the event dispatch and listening to filter these events.

Show all issues

We're going to need tests for this, which will be especially important for any filtering logic and any future use of the broadcast channel.

Show all issues

I also want to discuss naming. Eventually, we'll want an open comm channel to the Review Board server as well. Is this going to be designed with the intent of being used for that as well? Or should we name this something more specific?

david May 19, 2023, 2:42 p.m.
```
Perhaps ClientCommChannel?
```

reviewboard/static/rb/js/common/models/commChannel.ts (Diff revision 1)
Show all issues
```
Can you add a Version Added?
```

reviewboard/static/rb/js/common/models/commChannel.ts (Diff revision 1)

Show all issues

This is missing docs. Should also have Version Added, which I've unfortunately left out of my other TS reviews.

reviewboard/static/rb/js/common/models/commChannel.ts (Diff revision 1)
Show all issues
```
Missing Version Added.
```

Description:

		The new behavior for the unified banner means that individual tabs fetch
		and keep track of pending review/draft IDs more eagerly than before. The
		result of this is that it was easier to hit bugs when performing actions
		with a review request open in multiple tabs.

		This change adds a small utility that makes use of the
		`BroadcastChannel` API to coordinate across multiple tabs/windows within
		a browser session. Right now the only message that we send is "reload",
		which asks other tabs to reload their contents. This is sent when
		something is either published or discarded, which invalidates draft IDs.
	+
	+	The broadcast channel is origin-wide, so each `PageView` can implement a
	+	method to return a data blob that's used for filtering whether or not to
	+	reload. The default implementation is empty, and `ReviewablePageView`
	+	will filter based on the review request ID (so any open tabs related to
	+	that review request will be triggered).

Testing Done:

~		Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload.
~		Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.
	~	Ran js-tests.
	~	Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload.
	+	Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.

Commits:

	Summary	ID
	Add a communication channel for forcing reloads across tab sessions. The new behavior for the unified banner means that individual tabs fetch and keep track of pending review/draft IDs more eagerly than before. The result of this is that it was easier to hit bugs when performing actions with a review request open in multiple tabs. This change adds a small utility that makes use of the `BroadcastChannel` API to coordinate across multiple tabs/windows within a browser session. Right now the only message that we send is "reload", which asks other tabs to reload their contents. This is sent when something is either published or discarded, which invalidates draft IDs. Testing Done: - Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload. - Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.	7df4b2f0db096ec892c1599ea37ce66517ca5cb6
	Add a communication channel for forcing reloads across tab sessions. The new behavior for the unified banner means that individual tabs fetch and keep track of pending review/draft IDs more eagerly than before. The result of this is that it was easier to hit bugs when performing actions with a review request open in multiple tabs. This change adds a small utility that makes use of the `BroadcastChannel` API to coordinate across multiple tabs/windows within a browser session. Right now the only message that we send is "reload", which asks other tabs to reload their contents. This is sent when something is either published or discarded, which invalidates draft IDs. The broadcast channel is origin-wide, so each `PageView` can implement a method to return a data blob that's used for filtering whether or not to reload. The default implementation is empty, and `ReviewablePageView` will filter based on the review request ID (so any open tabs related to that review request will be triggered). Testing Done: - Ran js-tests. - Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload. - Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.	8ac3ec8f6634c0d2b555cceaa5c8719d948a0228

Diff:

Revision 2 (+494)

Show changes

	reviewboard/static/rb/js/common/index.ts
	reviewboard/static/rb/js/common/models/commChannel.ts
	reviewboard/static/rb/js/common/models/tests/commChannelTests.ts
	reviewboard/static/rb/js/common/models/tests/index.ts
	reviewboard/static/rb/js/common/views/pageView.ts
	reviewboard/static/rb/js/reviews/views/reviewablePageView.ts
	reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/common/models/commChannel.ts (Diff revision 2)
Show all issues
```
Should document this.
```
reviewboard/static/rb/js/common/views/pageView.ts (Diff revision 2)
Show all issues
```
Should document this.
```

reviewboard/static/rb/js/reviews/views/reviewablePageView.ts (Diff revision 2)

Show all issues

I think you could get rid of this and replace it with some info about how we're going to trigger tabs related to the review request.

Commits:

	Summary	ID
	Add a communication channel for forcing reloads across tab sessions. The new behavior for the unified banner means that individual tabs fetch and keep track of pending review/draft IDs more eagerly than before. The result of this is that it was easier to hit bugs when performing actions with a review request open in multiple tabs. This change adds a small utility that makes use of the `BroadcastChannel` API to coordinate across multiple tabs/windows within a browser session. Right now the only message that we send is "reload", which asks other tabs to reload their contents. This is sent when something is either published or discarded, which invalidates draft IDs. The broadcast channel is origin-wide, so each `PageView` can implement a method to return a data blob that's used for filtering whether or not to reload. The default implementation is empty, and `ReviewablePageView` will filter based on the review request ID (so any open tabs related to that review request will be triggered). Testing Done: - Ran js-tests. - Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload. - Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.	8ac3ec8f6634c0d2b555cceaa5c8719d948a0228
	Add a communication channel for forcing reloads across tab sessions. The new behavior for the unified banner means that individual tabs fetch and keep track of pending review/draft IDs more eagerly than before. The result of this is that it was easier to hit bugs when performing actions with a review request open in multiple tabs. This change adds a small utility that makes use of the `BroadcastChannel` API to coordinate across multiple tabs/windows within a browser session. Right now the only message that we send is "reload", which asks other tabs to reload their contents. This is sent when something is either published or discarded, which invalidates draft IDs. The broadcast channel is origin-wide, so each `PageView` can implement a method to return a data blob that's used for filtering whether or not to reload. The default implementation is empty, and `ReviewablePageView` will filter based on the review request ID (so any open tabs related to that review request will be triggered). Testing Done: - Ran js-tests. - Published review requests, reviews, and replies from one tab, and saw other tabs pop up the alert and reload. - Discarded drafts from the unified banner and from the review dialog, and saw other tabs pop up the alert and reload.	1bbc4c4e7e9ba3cdd2e68c13c93a619834cb6771

Diff:

Revision 3 (+498)

Show changes

	reviewboard/static/rb/js/common/index.ts
	reviewboard/static/rb/js/common/models/commChannel.ts
	reviewboard/static/rb/js/common/models/tests/commChannelTests.ts
	reviewboard/static/rb/js/common/models/tests/index.ts
	reviewboard/static/rb/js/common/views/pageView.ts
	reviewboard/static/rb/js/reviews/views/reviewablePageView.ts
	reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (98d6ddc)