Integrate Configure Checks web view

[rolfheij-sil]

This web view allows you to configure what checks you want to run for your project, and what the scope should be.

The web view can be opened from the context menu of the Scripture Editor:

Checks can be enabled/disabled by checking them. Note that only Characters and Repeated Words are configurable for now (through their respective inventories).

Clicking the temporary Print results to console button will output the checking results to the console

Note that the checking service is always runnin' in the background, so there is no Run button or anything like that. Enabling/disabling checks and changing the scope will be immediately communicated to the checking service.

This change is 

[irahopkinson]

Looking good - one small tidy-up

Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

---

extensions/src/platform-scripture/src/configure-checks.web-view-provider.ts line 44 at r1 (raw file):

        (await (
          await papi.projectDataProviders.get('platform.base', projectId)
        ).getSetting('platform.name')) ?? projectId;

This is pretty ugly code - await inside an await and taking up 4 lines (you probably just copied this from elsewhere). Assign the first await then use it, i.e.

const pdp = await papi.projectDataProviders.get('platform.base', projectId);
projectName = (await pdp.getSetting('platform.name')) ?? projectId;

Code quote:

      projectName =
        (await (
          await papi.projectDataProviders.get('platform.base', projectId)
        ).getSetting('platform.name')) ?? projectId;

[rolfheij-sil]

Thanks for already taking a look! I'm not done yet, but I like to create the PR soon so that I have an overview of what I already did. And so that others can take an early look 😄

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

extensions/src/platform-scripture/src/configure-checks.web-view-provider.ts line 44 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is pretty ugly code - await inside an await and taking up 4 lines (you probably just copied this from elsewhere). Assign the first await then use it, i.e.

const pdp = await papi.projectDataProviders.get('platform.base', projectId);
projectName = (await pdp.getSetting('platform.name')) ?? projectId;

You're right, that's pretty ugly. I fixed it and changed other occurrences of it too

[irahopkinson]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[irahopkinson]

Thanks for all this work. A few things to look at.

Reviewed 13 of 13 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @rolfheij-sil)

---

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

  const [projectId] = useWebViewState('projectId', '');
  const [projectName] = useWebViewState('projectName', '');
  const [selectedChecks, setSelectedChecks] = useState<string[]>([]);

BTW should selectedChecks be pulled from settings so the user doesn't have to select them every time? Perhaps this is out of scope for this issue?

---

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

  const [selectedChecks, setSelectedChecks] = useState<string[]>([]);

  // eslint-disable-next-line no-type-assertion/no-type-assertion

Add a reason comment why type assertion is needed rather than the type already being correct. Better yet, fix the types so assertion is not needed - perhaps use generic types so this line would be const checkRunner = useDataProvider<ICheckRunner>('platformScripture.checkAggregator');. That might not be possible but it would be better.

---

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

  );

  const findCheckId = (checkLabel: string): string | undefined => {

I think this function needs to use useCallback as it is since otherwise it will get re-declared every render. For me a better option would be to pass in availableChecks as an argument and then move this declaration outside the webview component (and make it a regular function).

Code quote:

findCheckId

---

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

  };

  const updateSelectedChecks = (checkLabel: string, selected: boolean) => {

I think this function also needs to use useCallback.

Code quote:

updateSelectedChecks 

---

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

  const updateSelectedChecks = (checkLabel: string, selected: boolean) => {
    const checkId = findCheckId(checkLabel);
    if (!checkId) throw new Error(`No available check found with checkLabel ${checkLabel}`);

nit I like to add a blank line after a throw (or an early return) to make it really obvious there is a disconnect before the next line of code.

---

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

    ranges: CheckInputRange[],
  ): Promise<DataProviderUpdateInstructions<CheckRunnerDataTypes>> {
    logger.error('setActiveRanges on Aggregator:', JSON.stringify(ranges));

Was this intentionally added or is this debug code? If it's intentional, why does it always log an error?

---

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

    ranges: CheckInputRange[],
  ): Promise<DataProviderUpdateInstructions<CheckRunnerDataTypes>> {
    logger.error('setActiveRanges:', JSON.stringify(ranges));

Was this intentionally added or is this debug code? If it's intentional, why does it always log an error?

---

extensions/src/platform-scripture/src/checks/configure-checks/book-selector.component.tsx line 91 at r3 (raw file):

                    .map((bookId: string) => Canon.bookIdToEnglishName(bookId))
                    .join(', ')
                : 'No books are selected'}

Does this need L10n?

Code quote:

'No books are selected'

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 20 at r3 (raw file):

};

type ConfigureChecksProps = {

nit I agree with moving this below the exported type but I also prefer types to be just under imports since they are commonly moved to other files and then imported so I'd also move the const above to below this type.

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 43 at r3 (raw file):

  const [scrRef] = useSetting('platform.verseRef', defaultScrRef);

  const [useCurrentBook, setUseCurrentBook] = useState<boolean>(false);

BTW this is an unfortunate use of the word "use" since it has a particular meaning in React. Can we think of another verb or rename the variable some other way? (I know you inherited this variable.)

Code quote:

useCurrentBook

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 101 at r3 (raw file):

  const selectedBookIds = useMemo(() => {
    const Ids = activeRanges.map((range) =>

nit should be lower case, i.e. ids.

Code quote:

Ids

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 109 at r3 (raw file):

  return (
    <div className="configure-checks-dialog">
      <Typography variant="h5">{`Configure checks for project ${projectName}`}</Typography>

L10n?

Code quote:

Configure checks for project

[rolfheij-sil]

Thanks for reviewing!

Reviewed 1 of 19 files at r1, 5 of 13 files at r3, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @irahopkinson)

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW should selectedChecks be pulled from settings so the user doesn't have to select them every time? Perhaps this is out of scope for this issue?

Yes I thought about that but I think it's out of scope. Since the checking service doesn't have any awareness of which checks are active and which ones are not (or at least it doesn't have a public function that exposes which checks are enabled/disabled), the state management here might be a little complicated

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

Add a reason comment why type assertion is needed rather than the type already being correct. Better yet, fix the types so assertion is not needed - perhaps use generic types so this line would be const checkRunner = useDataProvider<ICheckRunner>('platformScripture.checkAggregator');. That might not be possible but it would be better.

Done.

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

I think this function needs to use useCallback as it is since otherwise it will get re-declared every render. For me a better option would be to pass in availableChecks as an argument and then move this declaration outside the webview component (and make it a regular function).

Done.

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

I think this function also needs to use useCallback.

Done.

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

Was this intentionally added or is this debug code? If it's intentional, why does it always log an error?

Sorry, that was debug code. Removed it.

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

Was this intentionally added or is this debug code? If it's intentional, why does it always log an error?

Sorry, that was debug code. Removed it.

---

extensions/src/platform-scripture/src/checks/configure-checks/book-selector.component.tsx line 91 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Does this need L10n?

This was debug code too. Removed it.

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 43 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW this is an unfortunate use of the word "use" since it has a particular meaning in React. Can we think of another verb or rename the variable some other way? (I know you inherited this variable.)

Agreed. Decided to go with rangeIsCurrentBook. As per our conventions it should be isRangeCurrentBook but that sounded somewhat confusing to me

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 101 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

nit should be lower case, i.e. ids.

Removed the variable entirely

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 109 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

L10n?

I removed the line entirely. It basically says the same thing as it says in the toolbar of the web view, which is localized

[tjcouch-sil]

This is awesome! Very excited for this. Just a handful of comments.

Reviewed 13 of 19 files at r1, 2 of 3 files at r2, 7 of 13 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @irahopkinson and @rolfheij-sil)

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

Done.

Actually I'm surprised to see as ICheckRunner is needed in the first place. Does the object returned not have the right type already? It probably should, so maybe something is going wrong and needs a bug filed or something if so.

---

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

  const [activeRanges, setActiveRanges] = useData('platformScripture.checkAggregator').ActiveRanges(
    undefined,
    [defaultScriptureRange],

As annoying as it is, wrapping in an array makes this have a new reference every render. It's probably not a huge deal since this is just used once at the first render of this component, but this does create a new array pointlessly every render.

---

extensions/src/platform-scripture/src/configure-checks.web-view-provider.ts line 42 at r4 (raw file):

      projectName = (await pdp.getSetting('platform.name')) ?? projectId;

      title += ` - ${projectName}`;

When concatenating localized strings in any other way than newline (colon, parentheses, plain space, quotes, hyphen, etc.), it is generally best to use string formatting with a format localized string to concatenate the strings in a localizable manner. That way, if the strings are concatenated in a way that doesn't make sense in a locale, they can change how the string is concatenated and format it differently.

Example: string key and usage

---

Sorry, this is a rule I haven't recorded anywhere yet... :( I have been meaning to make a section in the code style guide about various localized string rules since last week, but I have been sick.

---

extensions/src/platform-scripture/src/configure-checks.web-view-provider.ts line 51 at r4 (raw file):

      styles: configureChecksWebViewStyles,
      state: {
        projectId,

Putting projectId here before ...savedWebView.state means savedWebView.state.projectId will overwrite projectId if it exists. Looking at your code, I don't think this is your intention. I'd recommend swapping these two lines so ...savedWebView.state happens first then projectId gets precedence in the final value (if this is your intention).

---

extensions/src/platform-scripture-editor/contributions/localizedStrings.json line 15 at r4 (raw file):

      "%webView_platformScriptureEditor_charactersInventory%": "Inventory: Characters",
      "%webView_platformScriptureEditor_repeatedWordsInventory%": "Inventory: Repeated Words",
      "%webView_platformScriptureEditor_configureChecks%": "Configure Checks",

When a menu item's primary purpose is to open a dialog (or web view in our case), it should have an ellipsis. Is that still the case here? I think it is, but I'm not sure. Just checking in. Also on the line below

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 43 at r3 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Agreed. Decided to go with rangeIsCurrentBook. As per our conventions it should be isRangeCurrentBook but that sounded somewhat confusing to me

shouldUseCurrentBook? isRangeInCurrentBook? Doesn't matter that much to me; just ideas in case anything seems interesting

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 101 at r3 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Removed the variable entirely

⚰️🪦

[lyonsil]

Thanks for putting up the first part of this work, Rolf. It looks great overall. I added a few thoughts but nothing new/blocking.

Reviewed 13 of 19 files at r1, 2 of 3 files at r2, 7 of 13 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @irahopkinson and @rolfheij-sil)

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

Yes I thought about that but I think it's out of scope. Since the checking service doesn't have any awareness of which checks are active and which ones are not (or at least it doesn't have a public function that exposes which checks are enabled/disabled), the state management here might be a little complicated

@rolfheij-sil The AvailableChecks data type includes which checks are enabled/disabled per project.

  export type CheckDetails = {
    /** Name or {@link LocalizeKey} of the name of the check to display in UI */
    checkName: string | LocalizeKey;
    /** Description or {@link LocalizeKey} of the description of the check to display in UI */
    checkDescription: string | LocalizeKey;
  };

  export type CheckDetailsWithCheckId = CheckDetails & {
    /** Unique ID of the check within the set of all possible checks */
    checkId: string;
  };

  export type CheckRunnerCheckDetails = CheckDetailsWithCheckId & {
    /** List of project IDs that one particular check is enabled to evaluate */
    enabledProjectIds: string[];
  };

  export type CheckRunnerDataTypes = {
    AvailableChecks: DataProviderDataType<undefined, CheckRunnerCheckDetails[], never>;
    ActiveRanges: DataProviderDataType<undefined, CheckInputRange[], CheckInputRange[]>;
    CheckResults: DataProviderDataType<undefined, CheckRunResult[], never>;
  };

Having said that, I'm not suggesting that this PR should include settings/state management for the service. I don't think it's immediately necessary, and I think we have a little time to think about where we want that to reside. It could be the check service. It could be something else. Whatever it is will have to consider that checks and projects may not be available immediately when initializing, so there will have to be some sort of delayed initialization.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Actually I'm surprised to see as ICheckRunner is needed in the first place. Does the object returned not have the right type already? It probably should, so maybe something is going wrong and needs a bug filed or something if so.

When Rolf and I were looking at this, I believe useDataProvider only gave access to the "normal" data types, not the extra two functions enableCheck and disableCheck. Type casting was the only way for TS to recognize those two functions that we saw. I don't think we tried Ira's suggestion at the time, though. If it works, great. If it doesn't, then we can open a ticket.

---

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

      if (selected) {
        checkRunner.enableCheck(checkId, projectId);

If we keep the current design, you'll want to use the return value here to see what errors/warnings come from the check. This design requires that we disable/re-enable a check to get its latest warnings/errors.

---

extensions/src/platform-scripture-editor/contributions/localizedStrings.json line 15 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

When a menu item's primary purpose is to open a dialog (or web view in our case), it should have an ellipsis. Is that still the case here? I think it is, but I'm not sure. Just checking in. Also on the line below

@Sebastian-ubs ? @merchako ? @ianhewubs ? Any opinions here? Seems like a style guide-type question.

[rolfheij-sil]

Thanks for reviewing, all!

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

---

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

Previously, lyonsil (Matt Lyons) wrote…

@rolfheij-sil The AvailableChecks data type includes which checks are enabled/disabled per project.

  export type CheckDetails = {
    /** Name or {@link LocalizeKey} of the name of the check to display in UI */
    checkName: string | LocalizeKey;
    /** Description or {@link LocalizeKey} of the description of the check to display in UI */
    checkDescription: string | LocalizeKey;
  };

  export type CheckDetailsWithCheckId = CheckDetails & {
    /** Unique ID of the check within the set of all possible checks */
    checkId: string;
  };

  export type CheckRunnerCheckDetails = CheckDetailsWithCheckId & {
    /** List of project IDs that one particular check is enabled to evaluate */
    enabledProjectIds: string[];
  };

  export type CheckRunnerDataTypes = {
    AvailableChecks: DataProviderDataType<undefined, CheckRunnerCheckDetails[], never>;
    ActiveRanges: DataProviderDataType<undefined, CheckInputRange[], CheckInputRange[]>;
    CheckResults: DataProviderDataType<undefined, CheckRunResult[], never>;
  };

Having said that, I'm not suggesting that this PR should include settings/state management for the service. I don't think it's immediately necessary, and I think we have a little time to think about where we want that to reside. It could be the check service. It could be something else. Whatever it is will have to consider that checks and projects may not be available immediately when initializing, so there will have to be some sort of delayed initialization.

Interesting! If I have some time to spare I can take a stab at connecting that information to the check configuration web view

---

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

Previously, lyonsil (Matt Lyons) wrote…

When Rolf and I were looking at this, I believe useDataProvider only gave access to the "normal" data types, not the extra two functions enableCheck and disableCheck. Type casting was the only way for TS to recognize those two functions that we saw. I don't think we tried Ira's suggestion at the time, though. If it works, great. If it doesn't, then we can open a ticket.

Ira's suggestion doesn't work, unfortunately

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

As annoying as it is, wrapping in an array makes this have a new reference every render. It's probably not a huge deal since this is just used once at the first render of this component, but this does create a new array pointlessly every render.

Wrapped it in useMemo now

---

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

Previously, lyonsil (Matt Lyons) wrote…

If we keep the current design, you'll want to use the return value here to see what errors/warnings come from the check. This design requires that we disable/re-enable a check to get its latest warnings/errors.

I quickly connected the output of the enableCheck function to the web view. If the function enables anything it should show up below the Checklist with the available checks

---

extensions/src/platform-scripture/src/configure-checks.web-view-provider.ts line 42 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

When concatenating localized strings in any other way than newline (colon, parentheses, plain space, quotes, hyphen, etc.), it is generally best to use string formatting with a format localized string to concatenate the strings in a localizable manner. That way, if the strings are concatenated in a way that doesn't make sense in a locale, they can change how the string is concatenated and format it differently.

Example: string key and usage

---

Sorry, this is a rule I haven't recorded anywhere yet... :( I have been meaning to make a section in the code style guide about various localized string rules since last week, but I have been sick.

Done. Interesting, hadn't seen this before

---

extensions/src/platform-scripture/src/configure-checks.web-view-provider.ts line 51 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Putting projectId here before ...savedWebView.state means savedWebView.state.projectId will overwrite projectId if it exists. Looking at your code, I don't think this is your intention. I'd recommend swapping these two lines so ...savedWebView.state happens first then projectId gets precedence in the final value (if this is your intention).

Thanks, I wasn't aware of that

---

extensions/src/platform-scripture-editor/contributions/localizedStrings.json line 15 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

@Sebastian-ubs ? @merchako ? @ianhewubs ? Any opinions here? Seems like a style guide-type question.

I think I remember @tombogle saying that this is the convention for PT9. I'll keep it that way for now.

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 43 at r3 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

shouldUseCurrentBook? isRangeInCurrentBook? Doesn't matter that much to me; just ideas in case anything seems interesting

Maybe usingCurrentBook would work

---

extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx line 101 at r3 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

⚰️🪦

RIP Ids

[irahopkinson]

Reviewed 3 of 6 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

Ira's suggestion doesn't work, unfortunately

Err, if you change useDataProvider to accept a generic type T then you can at least do the type assert there and then you would have only one comment for type assertion rather than everywhere you use it. Also if you later fix the types properly you can usually retain the generic type because it won't need to be specified (making it backward compatible where uses have already specified it).

[irahopkinson]

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

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

Err, if you change useDataProvider to accept a generic type T then you can at least do the type assert there and then you would have only one comment for type assertion rather than everywhere you use it. Also if you later fix the types properly you can usually retain the generic type because it won't need to be specified (making it backward compatible where uses have already specified it).

I see it already has a generic type so it will likely need to be fixed properly.

[rolfheij-sil]

Reviewable status: 27 of 28 files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

I see it already has a generic type so it will likely need to be fixed properly.

Fixed it :)

[tjcouch-sil]

Reviewed 1 of 13 files at r3, 5 of 5 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[irahopkinson]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved