Summary

Modernize CommentIssueManager for performance, bug fixes, usability.

Review Request #13712 — Created April 7, 2024 and submitted April 17, 2024, 10:36 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

CommentIssueManager was one of the earliest classes introduced when we
moved to Backbone. It had some usability flaws in terms of how its APIs
worked, and some performance flaws in its way of notifying callers of
issue status changes.

It also had a major caching bug where two comments of different types
but sharing the same ID would be seen as identical in the cache. This
could lead to issues setting and checking comment state. We had a bug
around this, and fixed a symptom around it, but did not identify this
cause beforehand.

This new version is ported to Spina and TypeScript and does the
following:

It avoids the caching bug by having a more specific cache key.
It includes new functions for accessing objects, which include
options-based parameters instead of positional parameters to help with
typing, readability, and extensibility. The old methods are
deprecated.
Callers can now listen to comment-specific events, instead of having
to listen for and check every single comment tracked in the manager.
This is done via a new issueStatusUpdated:<type>:<id> event, which
provides an object-based payload.
The old issueStatusUpdated still exists, but is also now replaced
with a new anyIssueStatusUpdated with an object-based payload as
well.

Some typing changes to BaseComment and Review have been updated to
make some arguments optional. These were technically optional, but not
typed that way. Since those impacted typing here, they're being changed
accordingly.

Testing Done

Unit tests pass.

Made use of some of this in an in-progress change.

Commits

Summary	ID
Modernize CommentIssueManager for performance, bug fixes, usability. `CommentIssueManager` was one of the earliest classes introduced when we moved to Backbone. It had some usability flaws in terms of how its APIs worked, and some performance flaws in its way of notifying callers of issue status changes. It also had a major caching bug where two comments of different types but sharing the same ID would be seen as identical in the cache. This could lead to issues setting and checking comment state. We had a bug around this, and fixed a symptom around it, but did not identify this cause beforehand. This new version is ported to Spina and TypeScript and does the following: * It avoids the caching bug by having a more specific cache key. * It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated. * Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload. * The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well. Some typing changes to `BaseComment` and `Review` have been updated to make some arguments optional. These were technically optional, but not typed that way. Since those impacted typing here, they're being changed accordingly.	574cc9d69fbea9d14191aec81780b05097a02d45

Summary

Modernize CommentIssueManager for performance, bug fixes, usability.

`CommentIssueManager` was one of the earliest classes introduced when we moved to Backbone. It had some usability flaws in terms of how its APIs worked, and some performance flaws in its way of notifying callers of issue status changes. It also had a major caching bug where two comments of different types but sharing the same ID would be seen as identical in the cache. This could lead to issues setting and checking comment state. We had a bug around this, and fixed a symptom around it, but did not identify this cause beforehand. This new version is ported to Spina and TypeScript and does the following: * It avoids the caching bug by having a more specific cache key. * It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated. * Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload. * The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well. Some typing changes to `BaseComment` and `Review` have been updated to make some arguments optional. These were technically optional, but not typed that way. Since those impacted typing here, they're being changed accordingly.

574cc9d69fbea9d14191aec81780b05097a02d45

Issues

Description	From	Last Updated
Looks like this can be type	david	April 14, 2024, 4:24 p.m.
Let's move the type into the {}	david	April 14, 2024, 4:25 p.m.
I feel like we should just document the type as being CommentIssueManagerCommentType	david	April 17, 2024, 7:27 p.m.
Same here re: type	david	April 17, 2024, 7:27 p.m.
SetCommentIssueStatusOptions	david	April 17, 2024, 7:28 p.m.
string -> CommentIssueStatusType	david	April 17, 2024, 7:28 p.m.
I think this one isn't supposed to be optional.	maubin	April 17, 2024, 7:32 p.m.
Missing the "Option Args" header before "commentID".	maubin	April 17, 2024, 7:30 p.m.
This should say issueStatusUpdated:{comment_type}:{comment_id}:	maubin	April 17, 2024, 7:31 p.m.

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Looks like this can be type
```

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

The issue has been resolved. Show all issues

Let's move the type into the {}

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

The issue has been resolved. Show all issues

I feel like we should just document the type as being CommentIssueManagerCommentType

chipx86 April 14, 2024, 4:26 p.m.

Yeah, I keep going back and forth on what ultimately makes sense here. Do we document the underlying type that actually gets used in JavaScript, or do we document the TypeScript type? The latter doesn't help JS authors much, so I've been leaning toward the former being more useful in general.

david April 15, 2024, 9:15 a.m.

I don't know how much we really need to fret here, especially since we don't generate any codebase docs for the JS side, but my feeling is that we should wherever possible encourage people to write TypeScript instead of JavaScript. The conversions are pretty minimal and straightforward (interface -> object, enum -> whatever type the enum values are). Perhaps we can just add something to /docs/manual/latest/extending/extensions/js-extensions/ explaining TS-vs-JS?

maubin April 16, 2024, 3:23 p.m.

I vote for David's solution, with a TS-vs-JS doc. Plus I feel like any JS author would be able to look through the codebase and figure out the underlying type anyway.

reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here re: type
```
reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
SetCommentIssueStatusOptions
```
reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
string -> CommentIssueStatusType
```

Change Summary:

Updated the type placement in imports.

Commits:

	Summary	ID
	Modernize CommentIssueManager for performance, bug fixes, usability. `CommentIssueManager` was one of the earliest classes introduced when we moved to Backbone. It had some usability flaws in terms of how its APIs worked, and some performance flaws in its way of notifying callers of issue status changes. It also had a major caching bug where two comments of different types but sharing the same ID would be seen as identical in the cache. This could lead to issues setting and checking comment state. We had a bug around this, and fixed a symptom around it, but did not identify this cause beforehand. This new version is ported to Spina and TypeScript and does the following: * It avoids the caching bug by having a more specific cache key. * It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated. * Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload. * The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well. Some typing changes to `BaseComment` and `Review` have been updated to make some arguments optional. These were technically optional, but not typed that way. Since those impacted typing here, they're being changed accordingly.	d683770c932b2c2553476427172ccb46654b1027
	Modernize CommentIssueManager for performance, bug fixes, usability. `CommentIssueManager` was one of the earliest classes introduced when we moved to Backbone. It had some usability flaws in terms of how its APIs worked, and some performance flaws in its way of notifying callers of issue status changes. It also had a major caching bug where two comments of different types but sharing the same ID would be seen as identical in the cache. This could lead to issues setting and checking comment state. We had a bug around this, and fixed a symptom around it, but did not identify this cause beforehand. This new version is ported to Spina and TypeScript and does the following: * It avoids the caching bug by having a more specific cache key. * It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated. * Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload. * The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well. Some typing changes to `BaseComment` and `Review` have been updated to make some arguments optional. These were technically optional, but not typed that way. Since those impacted typing here, they're being changed accordingly.	274019db21ac6da3b152f7ab4fab71cf3963cd75

Diff:

Revision 2 (+1992 -604)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common/resources/models/baseCommentModel.ts
	reviewboard/static/rb/js/common/resources/models/reviewModel.ts
	reviewboard/static/rb/js/models/commentIssueManagerModel.es6.js
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts
	reviewboard/static/rb/js/reviews/models/tests/commentIssueManagerModelTests.ts
	reviewboard/static/rb/js/reviews/models/tests/index.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		`CommentIssueManager` was one of the earliest classes introduced when we
		moved to Backbone. It had some usability flaws in terms of how its APIs
		worked, and some performance flaws in its way of notifying callers of
		issue status changes.

		It also had a major caching bug where two comments of different types
		but sharing the same ID would be seen as identical in the cache. This
		could lead to issues setting and checking comment state. We had a bug
		around this, and fixed a symptom around it, but did not identify this
		cause beforehand.

		This new version is ported to Spina and TypeScript and does the
		following:

		It avoids the caching bug by having a more specific cache key.
		It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated.
		Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload.
		The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well.

	+
	+
		Some typing changes to `BaseComment` and `Review` have been updated to
		make some arguments optional. These were technically optional, but not
		typed that way. Since those impacted typing here, they're being changed
		accordingly.

reviewboard/static/rb/js/common/resources/models/reviewModel.ts (Diff revision 2)

The issue has been resolved. Show all issues

I think this one isn't supposed to be optional.

chipx86 April 17, 2024, 7:31 p.m.

It was intentional, but not ideal. It was to address typing in how these functions sometimes get used. We don't always actually populate it with all the information, and instead allow it to be constructed with just an id so the data will load on ready().

These methods are in need of a complete rethink, though. I'm just going to not use them for this.

reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts (Diff revision 2)

The issue has been resolved. Show all issues

Missing the "Option Args" header before "commentID".

reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
This should say issueStatusUpdated:{comment_type}:{comment_id}:
```

Change Summary:


Switched all docs to use TypeScript types instead of plain JavaScript types.
Reverted a change to the Review.createFileAttachment() signature.
Fixed some other doc errors that were reported.

Description:

		`CommentIssueManager` was one of the earliest classes introduced when we
		moved to Backbone. It had some usability flaws in terms of how its APIs
		worked, and some performance flaws in its way of notifying callers of
		issue status changes.

		It also had a major caching bug where two comments of different types
		but sharing the same ID would be seen as identical in the cache. This
		could lead to issues setting and checking comment state. We had a bug
		around this, and fixed a symptom around it, but did not identify this
		cause beforehand.

		This new version is ported to Spina and TypeScript and does the
		following:

		It avoids the caching bug by having a more specific cache key.
		It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated.
		Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload.
		The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well.

-
-
		Some typing changes to `BaseComment` and `Review` have been updated to
		make some arguments optional. These were technically optional, but not
		typed that way. Since those impacted typing here, they're being changed
		accordingly.

Commits:

	Summary	ID
	Modernize CommentIssueManager for performance, bug fixes, usability. `CommentIssueManager` was one of the earliest classes introduced when we moved to Backbone. It had some usability flaws in terms of how its APIs worked, and some performance flaws in its way of notifying callers of issue status changes. It also had a major caching bug where two comments of different types but sharing the same ID would be seen as identical in the cache. This could lead to issues setting and checking comment state. We had a bug around this, and fixed a symptom around it, but did not identify this cause beforehand. This new version is ported to Spina and TypeScript and does the following: * It avoids the caching bug by having a more specific cache key. * It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated. * Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload. * The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well. Some typing changes to `BaseComment` and `Review` have been updated to make some arguments optional. These were technically optional, but not typed that way. Since those impacted typing here, they're being changed accordingly.	274019db21ac6da3b152f7ab4fab71cf3963cd75
	Modernize CommentIssueManager for performance, bug fixes, usability. `CommentIssueManager` was one of the earliest classes introduced when we moved to Backbone. It had some usability flaws in terms of how its APIs worked, and some performance flaws in its way of notifying callers of issue status changes. It also had a major caching bug where two comments of different types but sharing the same ID would be seen as identical in the cache. This could lead to issues setting and checking comment state. We had a bug around this, and fixed a symptom around it, but did not identify this cause beforehand. This new version is ported to Spina and TypeScript and does the following: * It avoids the caching bug by having a more specific cache key. * It includes new functions for accessing objects, which include options-based parameters instead of positional parameters to help with typing, readability, and extensibility. The old methods are deprecated. * Callers can now listen to comment-specific events, instead of having to listen for and check every single comment tracked in the manager. This is done via a new `issueStatusUpdated:<type>:<id>` event, which provides an object-based payload. * The old `issueStatusUpdated` still exists, but is also now replaced with a new `anyIssueStatusUpdated` with an object-based payload as well. Some typing changes to `BaseComment` and `Review` have been updated to make some arguments optional. These were technically optional, but not typed that way. Since those impacted typing here, they're being changed accordingly.	574cc9d69fbea9d14191aec81780b05097a02d45

Diff:

Revision 3 (+1954 -602)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common/resources/models/baseCommentModel.ts
	reviewboard/static/rb/js/common/resources/models/reviewModel.ts
	reviewboard/static/rb/js/models/commentIssueManagerModel.es6.js
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/reviews/models/commentIssueManagerModel.ts
	reviewboard/static/rb/js/reviews/models/tests/commentIssueManagerModelTests.ts
	reviewboard/static/rb/js/reviews/models/tests/index.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (f58541c)