Modernize CommentIssueManager for performance, bug fixes, usability.

Review Request #13712 — Created April 7, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

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.

Unit tests pass.

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

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
Description From Last Updated

Looks like this can be type

daviddavid

Let's move the type into the {}

daviddavid

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

daviddavid

Same here re: type

daviddavid

SetCommentIssueStatusOptions

daviddavid

string -> CommentIssueStatusType

daviddavid

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

maubinmaubin

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

maubinmaubin

This should say issueStatusUpdated:{comment_type}:{comment_id}:

maubinmaubin
david
  1. 
      
  2. Show all issues

    Looks like this can be type

  3. Show all issues

    Let's move the type into the {}

  4. Show all issues

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

    1. 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.

    2. 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?

    3. 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.

  5. Show all issues

    Same here re: type

  6. Show all issues

    SetCommentIssueStatusOptions

  7. Show all issues

    string -> CommentIssueStatusType

  8. 
      
chipx86
maubin
maubin
  1. 
      
  2. Show all issues

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

    1. 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.

  3. Show all issues

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

  4. Show all issues

    This should say issueStatusUpdated:{comment_type}:{comment_id}:

  5. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (f58541c)