Modernize CommentIssueManager for performance, bug fixes, usability.
Review Request #13712 — Created April 7, 2024 and submitted — Latest diff uploaded
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 newissueStatusUpdated:<type>:<id>
event, which
provides an object-based payload. -
The old
issueStatusUpdated
still exists, but is also now replaced
with a newanyIssueStatusUpdated
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.
Unit tests pass.
Made use of some of this in an in-progress change.