Add subscriptions to the check aggregator to enable multiple webviews

[lyonsil]

This is a necessary, underlying change for supporting the UX design for checks.

Note that I needed to change the existing UI to make it still functional, and the main ugly thing added is that the check results viewer now requires entering a check subscription ID before you can see results. If you look in the renderer console log, you should see a subscription ID written to the console when you open a webview to enable/disable checks. Just copy/paste that into the check results viewer for the time being to see check results. Yes it's kind of ugly.

This is not the intended final design. In the UX designs, the same UI has both configuration and display of check results, I'm assuming the webview/component that owns the other components for running checks and viewing results will own the subscription and pass the ID down to both.

This PR should resolve #1232, #1171, and #1244.

---

This change is 

[tjcouch-sil]

Just one blocking thing and a few thoughts for you to consider :) thanks for this! Looks great.

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lyonsil)

---

extensions/src/platform-scripture/src/types/platform-scripture.d.ts line 639 at r1 (raw file):
You can use Omit with a string union to achieve what you achieved here in a more concise way if you want:

export type SettableCheckDetails = Omit<CheckRunnerCheckDetails, 'checkName' | 'checkDescription'>;

At first, I had typed the following:

Why can checkId be set (not also omitted)? It seems checkId is universally unique in other types here, but I could be misinterpreting.

But then I looked for uses of SettableCheckDetails and realized checkId is used for identifying the check whose details to change, not to change the checkId of a check... 🤦 anyway because I got tripped up here, I kinda wonder if it would be worth noting that in the description of this type. I dunno; maybe this is too obvious haha do with this information what you will 😂

---

c-sharp/Checks/CheckRunner.cs line 284 at r1 (raw file):

    private void RunChecksForProject(string projectId)
    {
        ThreadingUtils.RunTask(Task.Run(() => RunChecksForProjectInternal(projectId)));

Might be helpful to provide a description argument for this task. Tbh I kinda want the description parameter to be mandatory... 🤔 thoughts? I guess it would be annoying if you're just trying something real quick, but I guess a description would pretty much always be helpful if a problem occurs.

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 39 at r1 (raw file):

        rangesAreOverlapping(aggregatedRange, range),
      );
      if (overlappingRange) mergeRanges(overlappingRange, range);

Is it possible that a range could overlap with two ranges that do not overlap each other? For example, could there be the following ranges? Gen 1-3, Gen 5-9, Gen 2-8. I think this algorithm would not work quite right in that case. Maybe it's not a big deal? Not sure; I haven't reflected on it much.

---

extensions/src/platform-scripture/src/types/platform-scripture.d.ts line 618 at r1 (raw file):

     */
    formatStringArguments?: { [key: string]: string };
    /** Indicates if the user decided this check result should considered incorrect going forward */

Typo: "result should be considered incorrect"

---

c-sharp/Checks/CheckResultsRecorder.cs line 96 at r1 (raw file):

    /// <summary>
    /// After a check as finished running, filter and complete filling in data on the results found.

Typo "after a check has finished running"

---

extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts line 45 at r1 (raw file):

};

type CheckRunnerPdps = {

An alternative way to get these types using the same channels that the PAPI uses is to use the following:

type CheckRunnerPdps = {
  usfmBookPdp: ProjectDataProviderInterfaces['platformScripture.USFM_Book'];
  basePdp: ProjectDataProviderInterfaces['platform.base'];
};

Dunno if that feels cleaner or not, but just figured I'd throw it out there in case you liked it.

---

extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts line 51 at r1 (raw file):

const deniedDataScope: ExtensionDataScope = {
  extensionName: 'extension-host',

Shouldn't this be platform-scripture? I think this is supposed to be the extension's literal name, right? Maybe the data qualifier could be extension-host-check-runner.deniedResultsList or something to preserve the idea that this data represents all checks on the extension host...?

[irahopkinson]

Reviewed 16 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @lyonsil)

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 279 at r1 (raw file):

  // Required method since this is a data provider engine
  // eslint-disable-next-line class-methods-use-this, @typescript-eslint/class-methods-use-this

You don't need this eslint disable comment if you include the keyword override when you declare the function.

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 296 at r1 (raw file):

    checkResultUniqueId?: string,
  ): Promise<boolean> {
    const checkRunner = await this.findCheckRunnerForCheckId(checkResultType);

Shouldn't this be checkId?

Code quote:

checkResultType

---

extensions/src/platform-scripture/src/configure-checks.web-view.tsx line 87 at r1 (raw file):

      if (availableChecksIsLoading) {
        sonner.warning(`${selected ? 'Enabling' : 'Disabling'} check failed.`, {
          description: 'Please try again later.',

BTW I know you didn't write this, but shouldn't it be localized? There is also another sonner message below.

Code quote:

        sonner.warning(`${selected ? 'Enabling' : 'Disabling'} check failed.`, {
          description: 'Please try again later.',

---

c-sharp/Checks/CheckResultsRecorder.cs line 96 at r1 (raw file):

    /// <summary>
    /// After a check as finished running, filter and complete filling in data on the results found.

typo "as" > "has"

[irahopkinson]

Reviewed 1 of 17 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lyonsil)

---

extensions/src/platform-scripture/src/checks/persisted-check-run-result.model.ts line 69 at r1 (raw file):

        existingResult.verseRef.verse === result.verseRef.verse &&
        existingResult.selectedText === result.selectedText &&
        existingResult.checkResultUniqueId === result.checkResultUniqueId

BTW isn't it enough to just check the unique ID? When would the unique ID be the same but any of the other 3 be different? Even if they are different perhaps it should be removed anyway?

Code quote:

existingResult.checkResultUniqueId === result.checkResultUniqueId

[lyonsil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

c-sharp/Checks/CheckResultsRecorder.cs line 96 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

typo "as" > "has"

Whoops - good catch both of you!

---

c-sharp/Checks/CheckRunner.cs line 284 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Might be helpful to provide a description argument for this task. Tbh I kinda want the description parameter to be mandatory... 🤔 thoughts? I guess it would be annoying if you're just trying something real quick, but I guess a description would pretty much always be helpful if a problem occurs.

Added a description. I also made the parameter required. I'm not sure if it will ever be super helpful, but it could be. I saw it more of a last ditch effort to understand something that was melting down, but it certainly doesn't hurt to make this mandatory.

---

extensions/src/platform-scripture/src/configure-checks.web-view.tsx line 87 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I know you didn't write this, but shouldn't it be localized? There is also another sonner message below.

This entire UI is being replaced by the new UX design for checks. I don't think it is using toast pop-ups.

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 39 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Is it possible that a range could overlap with two ranges that do not overlap each other? For example, could there be the following ranges? Gen 1-3, Gen 5-9, Gen 2-8. I think this algorithm would not work quite right in that case. Maybe it's not a big deal? Not sure; I haven't reflected on it much.

Good catch - It probably isn't a common scenario, but it would result in producing duplicate check results if it happened which could be confusing. I updated the algorithm and wrote tests so it's easier to catch problems and add more cases in the future if needed. Unfortunately I had to pull these functions out of this file to get jest to work because it could not properly pull in @papi types.

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 279 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

You don't need this eslint disable comment if you include the keyword override when you declare the function.

If I add override I get TS error 4113 because this method isn't declared on the base class.

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 296 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Shouldn't this be checkId?

Indeed you are correct - good catch! This is one of a few things I could not test well because there is no mechanism for actually denying/allowing check results right now. I did verify that we can filter properly based on whether a check result has been denied by doing it in PT9, but actually toggling the deny on/off is not possible at the moment in P.B.

---

extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts line 45 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

An alternative way to get these types using the same channels that the PAPI uses is to use the following:

type CheckRunnerPdps = {
  usfmBookPdp: ProjectDataProviderInterfaces['platformScripture.USFM_Book'];
  basePdp: ProjectDataProviderInterfaces['platform.base'];
};

Dunno if that feels cleaner or not, but just figured I'd throw it out there in case you liked it.

Made the switch to what you suggested. It's 2 imports instead of 3! 😄

---

extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts line 51 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Shouldn't this be platform-scripture? I think this is supposed to be the extension's literal name, right? Maybe the data qualifier could be extension-host-check-runner.deniedResultsList or something to preserve the idea that this data represents all checks on the extension host...?

It probably is more correct to use platform-scripture here. It probably wouldn't cause a problem the previous way in the big scheme of things, but it is better aligned with the original intent to use platform-scripture.

---

extensions/src/platform-scripture/src/checks/persisted-check-run-result.model.ts line 69 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW isn't it enough to just check the unique ID? When would the unique ID be the same but any of the other 3 be different? Even if they are different perhaps it should be removed anyway?

The unique ID doesn't exist most of the time (at least not at this point). I added it because I think we might need to start using it at least in some cases, but for now it's an optional field that never gets populated. PT9 doesn't have something like this, but they have some other metadata that is hard to detangle from their internal tokenization of USFM. Rather that trying to surface those details I created the unique ID in case we need it.

---

extensions/src/platform-scripture/src/types/platform-scripture.d.ts line 618 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Typo: "result should be considered incorrect"

good catch

---

extensions/src/platform-scripture/src/types/platform-scripture.d.ts line 639 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

You can use Omit with a string union to achieve what you achieved here in a more concise way if you want:

export type SettableCheckDetails = Omit<CheckRunnerCheckDetails, 'checkName' | 'checkDescription'>;

At first, I had typed the following:

Why can checkId be set (not also omitted)? It seems checkId is universally unique in other types here, but I could be misinterpreting.

But then I looked for uses of SettableCheckDetails and realized checkId is used for identifying the check whose details to change, not to change the checkId of a check... 🤦 anyway because I got tripped up here, I kinda wonder if it would be worth noting that in the description of this type. I dunno; maybe this is too obvious haha do with this information what you will 😂

Nice, that's more concise. Also I updated the description to try to clarify that point. Sorry it tripped you up. I can see how it could be confusing.

[irahopkinson]

Reviewed 12 of 12 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

extensions/src/platform-scripture/src/configure-checks.web-view.tsx line 87 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This entire UI is being replaced by the new UX design for checks. I don't think it is using toast pop-ups.

Gotcha

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 279 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

If I add override I get TS error 4113 because this method isn't declared on the base class.

Sorry, I assumed it was in the base given the comment.

---

extensions/src/platform-scripture/src/checks/persisted-check-run-result.model.ts line 69 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

The unique ID doesn't exist most of the time (at least not at this point). I added it because I think we might need to start using it at least in some cases, but for now it's an optional field that never gets populated. PT9 doesn't have something like this, but they have some other metadata that is hard to detangle from their internal tokenization of USFM. Rather that trying to surface those details I created the unique ID in case we need it.

Gotcha.

---

extensions/src/platform-scripture/tsconfig.json line 26 at r2 (raw file):

    "typeRoots": [
      // Include default type declarations
      "../../../node_modules/@types",

BTW you should probably leave the original and add this one. However see my comment above for adding devDependencies.

Code quote:

"../../../node_modules/@types",

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts line 35 at r2 (raw file):

    const result = aggregateRanges(subscriptions);
    // Using JSON.stringify here because the constructors for VerseRef set different internal fields

BTW this sounds like a bug in VerseRef? If the C# version does the same then we should report it there.

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts line 174 at r2 (raw file):

  });

  it('should return true for verse within range with different chapters', () => {

BTW isn't this the same test as 'should return true for verse at the end of range'? Is this test doing what you intended?

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts line 180 at r2 (raw file):

  });

  it('should return false for verse outside range with different chapters', () => {

BTW isn't this the same as 'should return false for verse outside range'? Is this test doing what you intended?

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.ts line 93 at r2 (raw file):

  return {
    start: new VerseRef(start),
    end: end ? new VerseRef(end) : undefined,

BTW I'm surprised you had to make this change. What is happening here that you have to clone the VerseRefs?

Code quote:

    start: new VerseRef(start),
    end: end ? new VerseRef(end) : undefined,

---

extensions/src/platform-scripture/package.json line 43 at r2 (raw file):

    "@swc/core": "^1.7.35",
    "@tailwindcss/typography": "^0.5.15",
    "@types/jest": "^29.5.14",

BTW you shouldn't need to add any of these devDependencies (unless you import them in code) since they have been included at the root.

---

extensions/src/platform-scripture/jest.config.ts line 1 at r2 (raw file):

module.exports = {

BTW this is the "old-fashioned" way of doing exports. Check the same file in the root (although its content is more complicated). Actually, I'm surprised we can't just use the config at the root but I guess you added this because we couldn't.

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

extensions/src/platform-scripture/jest.config.ts line 1 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW this is the "old-fashioned" way of doing exports. Check the same file in the root (although its content is more complicated). Actually, I'm surprised we can't just use the config at the root but I guess you added this because we couldn't.

I've tried getting jest working in extensions previously without success. I tried again and largely followed copilot's suggestion for this file. If this file doesn't exist, then the tests won't run.

---

extensions/src/platform-scripture/package.json line 43 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW you shouldn't need to add any of these devDependencies (unless you import them in code) since they have been included at the root.

While npm test will run without these included in this package.json file, I cannot run or debug tests with VSCode without these changes in package.json.

---

extensions/src/platform-scripture/tsconfig.json line 26 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW you should probably leave the original and add this one. However see my comment above for adding devDependencies.

Leaving the previous line does nothing because it is looking for node_modules under platform-scripture which doesn't exist. If we ever do something that causes it to exist we can put this back. The new line actually does pull in type declarations from the root.

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 279 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Sorry, I assumed it was in the base given the comment.

At runtime the data provider engine code inspects what functions exist on the object you're registering as a data provider. That's what the comment was referring to.

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.ts line 93 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I'm surprised you had to make this change. What is happening here that you have to clone the VerseRefs?

It probably wasn't technically required as-is, but when I was debugging a problem I found, I realized that I didn't want to be sharing object references between what is passed in and what is returned. If the objects are mutable at all, this could cause strange problems later.

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts line 35 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW this sounds like a bug in VerseRef? If the C# version does the same then we should report it there.

I agree it did seem like a bug. In some cases the _verse field was set while it wasn't in others. The reason this mattered is that deep equality checking fails. In C#, you never perform deep equality checking through reflection which is essentially what JS is going. You check for reference equality between objects, you implement IEquatable, and/or youoverride the Equals method on object. (I checked, the VerseRef in C# just does the Equals override.) I don't think JS has a clear, framework-consistent way to express object equality that integrates naturally for all code, so they can't do the same thing.

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts line 174 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW isn't this the same test as 'should return true for verse at the end of range'? Is this test doing what you intended?

There are some tests that overlap, yes. I let copilot write all the first draft of tests and then I made a few adjustments. When I could tell copilot was being methodical in how it generated test cases, I mostly left them as-is even if there were overlaps.

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts line 180 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW isn't this the same as 'should return false for verse outside range'? Is this test doing what you intended?

Same as my previous comment - I let copilot write all the first draft of tests and then I made a few adjustments. When I could tell copilot was being methodical in how it generated test cases, I mostly left them as-is even if there were overlaps.

[irahopkinson]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

extensions/src/platform-scripture/jest.config.ts line 1 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I've tried getting jest working in extensions previously without success. I tried again and largely followed copilot's suggestion for this file. If this file doesn't exist, then the tests won't run.

Actually my main point was the export, not the need for the file.

---

extensions/src/platform-scripture/package.json line 43 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

While npm test will run without these included in this package.json file, I cannot run or debug tests with VSCode without these changes in package.json.

Interesting, good to know.

---

extensions/src/platform-scripture/tsconfig.json line 26 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Leaving the previous line does nothing because it is looking for node_modules under platform-scripture which doesn't exist. If we ever do something that causes it to exist we can put this back. The new line actually does pull in type declarations from the root.

Fair enough. Actually now that I think about it, the way you have changed it is probably the original intention. Good job!

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.ts line 93 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It probably wasn't technically required as-is, but when I was debugging a problem I found, I realized that I didn't want to be sharing object references between what is passed in and what is returned. If the objects are mutable at all, this could cause strange problems later.

Gotcha

---

extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts line 35 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I agree it did seem like a bug. In some cases the _verse field was set while it wasn't in others. The reason this mattered is that deep equality checking fails. In C#, you never perform deep equality checking through reflection which is essentially what JS is going. You check for reference equality between objects, you implement IEquatable, and/or youoverride the Equals method on object. (I checked, the VerseRef in C# just does the Equals override.) I don't think JS has a clear, framework-consistent way to express object equality that integrates naturally for all code, so they can't do the same thing.

Yes, for that reason I added vRef1.equals(verRef2) method you could use for checking equality. In this case it's probably simpler to do what you have done since at least the vRef.toJSON() method is used by JSON.stringify.

[tjcouch-sil]

thanks for all the great adjustments! Exciting :)

Reviewed 10 of 12 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

c-sharp/Checks/CheckRunner.cs line 284 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Added a description. I also made the parameter required. I'm not sure if it will ever be super helpful, but it could be. I saw it more of a last ditch effort to understand something that was melting down, but it certainly doesn't hurt to make this mandatory.

Nice, thanks! :)

---

extensions/src/platform-scripture/src/types/platform-scripture.d.ts line 639 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Nice, that's more concise. Also I updated the description to try to clarify that point. Sorry it tripped you up. I can see how it could be confusing.

No worries! Thanks :)