SF-3344 Add Lynx punctuation checker

[siltomato]

This PR introduces the Lynx Insights feature. The feature can be enabled via the "Enable Lynx insights" feature flag.

Main parts:

• Insight blots in editor with different styles according to severity (info, warning, error)
• Clickable action prompt icons (like light bulb) to open the overlay
• Overlay to describe the insight and offer fixes when available
• Collapsible 'problems' panel in bottom split pane with editor
  • Displays insights by chapter/book/project
  • Groups by similar insight
  • Clicking an insight navigates to that insight and opens the insight overlay
  • Filter and sorting options
    • Stored in project user config
• Status indicator at top of editor
  • Displays insight counts
  • Clicking will toggle problems panel open/close
• Scrollbar position indicators to show insight positions in open chapter (currently no clicking option)
• 'Workspace' service connects the insight state with the Lynx library punctuation checker (npm package)

Dependency Updates:

• Added new Lynx-related dependencies (@sillsdev/lynx, @sillsdev/lynx-delta, @sillsdev/lynx-punctuation-checker)
• Updated quill-cursors to v4

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 43.65385% with 586 lines in your changes missing coverage. Please review.

Project coverage is 81.80%. Comparing base (5a0e932) to head (78a3e37).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...nx-insights-panel/lynx-insights-panel.component.ts	0.76%	129 Missing ⚠️
...hts/quill-services/quill-insight-render.service.ts	1.36%	72 Missing ⚠️
...-insight-overlay/lynx-insight-overlay.component.ts	1.44%	68 Missing ⚠️
...r-objects/lynx-insight-editor-objects.component.ts	1.53%	64 Missing ⚠️
...ion-prompt/lynx-insight-action-prompt.component.ts	1.92%	51 Missing ⚠️
...ate/editor/lynx/insights/lynx-workspace.service.ts	77.53%	28 Missing and 3 partials ⚠️
...re/ClientApp/src/app/shared/text/text.component.ts	48.97%	24 Missing and 1 partial ⚠️
...ynx-insight-scroll-position-indicator.component.ts	4.76%	20 Missing ⚠️
...nel-header/lynx-insights-panel-header.component.ts	5.00%	19 Missing ⚠️
...insights/quill-services/blots/lynx-insight-blot.ts	40.00%	18 Missing ⚠️
... and 13 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3174      +/-   ##
==========================================
- Coverage   83.01%   81.80%   -1.21%     
==========================================
  Files         569      598      +29     
  Lines       33203    34220    +1017     
  Branches     5355     5540     +185     
==========================================
+ Hits        27562    27993     +431     
- Misses       4838     5409     +571     
- Partials      803      818      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pmachapman]

I have only reviewed half of the files, so I will post my (incomplete) initial feedback now. Tomorrow I will review the real core of your work.

Some observations, in addition to the individual comments in files below:

• Many of the new files have no tests. Are you planning to add tests to these in this PR?
• If you enable Lynx and open a large project, the browser will lock up momentarily. This seems like a core Lynx task running synchronously?
• If you open a Resource after enabling Lynx, the browser will lock up permanently (I did this with the Amplified Bible).

Reviewed 52 of 98 files at r1, all commit messages.
Reviewable status: 52 of 98 files reviewed, 6 unresolved discussions

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/svg-icons/lynx-icons.ts line 22 at r1 (raw file):

      </defs>
    </g>
  </svg>`,

This SVG looks like it contains a redundant transform? Does something like this look similar, or have I misunderstood the SVG code?

  lynx_info: `<svg xmlns="http://www.w3.org/2000/svg" overflow="visible"
    preserveAspectRatio="none" width="13.423" height="11.758">
    <path d="M12.785 0c.248 0 .431.106.55.319a.618.618 0 0 1 0 .637L7.277
      11.44a.625.625 0 0 1-.567.318.625.625 0 0 1-.566-.318L.089.956a.618.618
      0 0 1 0-.637A.586.586 0 0 1 .637 0z"
      style="stroke-width:0;stroke-linecap:butt;stroke-linejoin:miter;fill:currentColor"
      vector-effect="non-scaling-stroke"/>
  </svg>`,

Code quote:

  lynx_info: `<svg xmlns="http://www.w3.org/2000/svg" overflow="visible"
    preserveAspectRatio="none" viewBox="0 0 13.422930000000001 11.75833"
    width="13.422930000000001" height="11.75833">
    <g transform="translate(0, 0)">
      <g transform="translate(0.000005000000001711968, 0) rotate(0)">
        <path d="M12.78542,0c0.24792,0 0.4309,0.10625 0.54896,0.31875c0.11806,0.2125
          0.11806,0.425 0,0.6375l-6.05625,10.48333c-0.12986,0.2125 -0.31875,0.31875
          -0.56667,0.31875c-0.24792,0 -0.43681,-0.10625 -0.56667,-0.31875l-6.05625,
          -10.48333c-0.11806,-0.2125 -0.11806,-0.425 0,-0.6375c0.11806,-0.2125
          0.30104,-0.31875 0.54896,-0.31875z" style="stroke-width: 0; stroke-linecap:
          butt; stroke-linejoin: miter; fill: currentColor;" vector-effect="non-scaling-stroke"/>
      </g>
      <defs>
        <path id="path-1731964993700321" d="M12.78542,0c0.24792,0 0.4309,0.10625
          0.54896,0.31875c0.11806,0.2125 0.11806,0.425 0,0.6375l-6.05625,10.48333c-0.12986,
          0.2125 -0.31875,0.31875 -0.56667,0.31875c-0.24792,0 -0.43681,-0.10625
          -0.56667,-0.31875l-6.05625,-10.48333c-0.11806,-0.2125 -0.11806,-0.425
          0,-0.6375c0.11806,-0.2125 0.30104,-0.31875 0.54896,-0.31875z" vector-effect="non-scaling-stroke"/>
      </defs>
    </g>
  </svg>`,

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-format-registry.service.spec.ts line 0 at r1 (raw file):
Should you delete this empty file? QuillFormatRegistryService has good test coverage due to the tests for QuillRegistrations.

---

src/RealtimeServer/scriptureforge/models/sf-project-user-config.ts line 32 at r1 (raw file):

  commentRefsRead: string[];
  editorTabsOpen: EditorTabPersistData[];
  lynxInsightState: LynxInsightUserData;

This property needs to be replicated in SFProjectUserConfig.cs.

Code quote:

lynxInsightState: LynxInsightUserData;

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-format-registry.service.ts line 40 at r1 (raw file):

  getRegisteredFormats(): string[] {
    return Array.from(this.registeredFormats);

More of a question:

What is your preference for converting a Set to an array in TypeScript? Array.from() or something like:

return [...this.registeredFormats];

(Our C# style config prefers this latter syntax)

Code quote:

return Array.from(this.registeredFormats);

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/includes.pipe.spec.ts line 7 at r1 (raw file):

    const pipe = new IncludesPipe();
    expect(pipe).toBeTruthy();
  });

Can you please add a test for transform<T\>

Code quote:

  it('create an instance', () => {
    const pipe = new IncludesPipe();
    expect(pipe).toBeTruthy();
  });

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts line 237 at r1 (raw file):

  }

  // TODO: write unit test

Flagging your TODO!

Code quote:

// TODO: write unit test

[pmachapman]

Awesome work!!!! I've finished reviewing the files I didn't get to last week.

Reviewed 46 of 98 files at r1.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-filter.service.spec.ts line 178 at r1 (raw file):

    it('should return "project" when insight is in different book', () => {
      const insight = createMockInsight('warning', 42, 1); // Mark 1

NIT: Mark is 41, Luke is 42.

Code quote:

42, 1); // Mark 1

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts line 210 at r1 (raw file):

      this.curInsights.set(event.uri, insights);
    }
    return Array.from(this.curInsights.values()).flat();

NIT: Should this be return [...this.curInsights.values()].flat();?

Code quote:

return Array.from(this.curInsights.values()).flat();

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts line 300 at r1 (raw file):

  keys(): Promise<string[]> {
    return Promise.resolve(Array.from(this.textDocIds));

NIT: See my earlier comments on perhaps using [...this.textDocIds] syntax. I don't mind which way we go, so long as we are consistent.

Code quote:

Array.from(this.textDocIds)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts line 324 at r1 (raw file):

}

function* setDifference(x: Set<string>, y: Set<string>): Iterable<string> {

I thought this was a C pointer at first glance! :-)

Cool use of a generator.

Code quote:

function*

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 2 at r1 (raw file):

@if (focusedInsight != null) {
  <div class="main-section focused" [class]="focusedInsight.type">

NIT: Should we use `[ngClass]?

Code quote:

[class]

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 8 at r1 (raw file):

      @if (focusedInsight.moreInfo != null) {
        <mat-icon matTooltip="More info" class="more-info-icon" (click)="toggleMoreInfo()">help_outline</mat-icon>

This string should be localized.

Code quote:

matTooltip="More info"

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 26 at r1 (raw file):

    <div class="secondary-actions">
      @if (menuActions.length > 0) {
        <mat-icon matTooltip="Actions" [matMenuTriggerFor]="actionMenu" class="action-menu-trigger">

This string should be localized.

Code quote:

matTooltip="Actions"

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 55 at r1 (raw file):

} @else if (insights.length > 1) {
  <div class="main-section list">
    <h1>Select for details</h1>

This should be localized

Code quote:

<h1>Select for details</h1>

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 59 at r1 (raw file):

      <div
        class="insight-item"
        [class]="insight.type"

NIT: Should we use `[ngClass]?

Code quote:

[class]

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-scroll-position-indicator/lynx-insight-scroll-position-indicator.component.html line 2 at r1 (raw file):

@for (pos of scrollPositions; track pos.id) {
  <div class="indicator" [class]="pos.type" [attr.style]="'--scroll-position-percent: ' + pos.percent + '%'"></div>

NIT: Should we use `[ngClass]?

Code quote:

[class]

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-status-indicator/lynx-insight-status-indicator.component.html line 4 at r1 (raw file):

  <div class="type-count">
    <mat-icon [svgIcon]="'lynx_' + insight.type" />
    <span [class]="insight.type">{{ insight.count }}</span>

NIT: Should we use `[ngClass]?

Code quote:

[class]

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.html line 36 at r1 (raw file):

          @case (0) {
            <div class="level-code">
              <mat-icon [svgIcon]="'lynx_' + node.type" [class.dismissed]="node.isDismissed" [class]="node.type" />

NIT: Should we use `[ngClass]?

Code quote:

[class]

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.scss line 122 at r1 (raw file):

    font-size: 0.7rem;
    flex-shrink: 0;
    display: none; // TODO: is code needed for display, and is there a good way to display it?

NIT: This TODO was in another file. Just checking that is valid.

Code quote:

display: none; // TODO: is code needed for display, and is there a good way to display it?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-editor-objects/lynx-insight-editor-objects.component.scss line 0 at r1 (raw file):
Should this file be deleted?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.scss line 135 at r1 (raw file):

    color: $secondaryTextColor;
    font-size: 0.9em;
    display: none; // TODO: is code needed for display, and is there a good way to display it?

Is this a TODO to be addressed in this PR?

Code quote:

// TODO: is code needed for display, and is there a good way to display it?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.scss line 161 at r1 (raw file):

    position: relative;
    padding-inline-start: 2.3em;
    color: $applyActionColor !important; // TODO: work with new theming

Is this a TODO for this PR?

Code quote:

 // TODO: work with new theming

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-state.service.ts line 259 at r1 (raw file):

    // Load stored state from project user config
    this.activatedProjectUserConfig.projectUserConfig$.pipe(filterNullish(), take(1)).subscribe(puc => {

Do we need a pipe(quietTakeUntilDestroyed(this.destroyRef)) (or similar) here, and below in the save state block?

Code quote:

this.activatedProjectUserConfig.projectUserConfig$

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.ts line 364 at r1 (raw file):

    if (editorSegments == null) {
      return '...'; // TODO: better default text

NIT: Is this TODO to be addressed in this PR? If you still want to use an ellipsis, you can use U+2026.

Code quote:

return '...'; // TODO: better default text

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/quill-services/blots/lynx-insight-blot.ts line 8 at r1 (raw file):

  static tagName = 'lynx-insight';
  static idDatasetPropName = 'insightId';
  static idAttributeName = kebabCase(LynxInsightBlot.idDatasetPropName);

NIT: Wouldn't it be easier to just use the string value 'insight-id'?

Code quote:

kebabCase(LynxInsightBlot.idDatasetPropName)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-action-prompt/lynx-insight-action-prompt.component.ts line 35 at r1 (raw file):

  activeInsights: LynxInsight[] = [];
  isLtr: boolean = this.dir.value === 'ltr';

NIT: I think isRtl is more consistent, although I do notice one other place a property called isLtr is used (text.component.ts). What was your rationale for making this isLtr?

Code quote:

 isLtr: boolean = this.dir.value === 'ltr';

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.html line 164 at r1 (raw file):

                    <!-- dir must be set on as-split-area because as-split is always set with dir=ltr -->
                    <as-split direction="vertical" [useTransition]="true">
                      <as-split-area size="*" [minSize]="20" [order]="1" [dir]="i18n.direction">

I'd love for this to be a tab, with for example vertical tab stacking, but I think that's a job for another PR!

Code quote:

                  <div class="container-for-split">
                    <!-- dir must be set on as-split-area because as-split is always set with dir=ltr -->
                    <as-split direction="vertical" [useTransition]="true">
                      <as-split-area size="*" [minSize]="20" [order]="1" [dir]="i18n.direction">

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.html line 248 at r1 (raw file):

                      <as-split-area
                        *ngIf="showInsights"

NIT: Should we use @if (showInsights) { instead?

Code quote:

*ngIf="showInsights"

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.html line 254 at r1 (raw file):

                        [dir]="i18n.direction"
                      >
                        <app-lynx-insights-panel />

I didn't realize that Angular supported self-closing tags (since v16). Maybe we should have a separate PR that runs this migration? https://angular.dev/reference/migrations/self-closing-tags

Code quote:

<app-lynx-insights-panel />

[siltomato]

Thanks for going through these! As for the files with no tests (mostly components, I believe), the plan is to add tests in later PRs.

Also, since the feature will be behind a feature flag, the browser lock ups issues can be addressed in a later PR as well, I think. That way users can start giving feedback sooner. Do you want to describe your findings in a Jira issue for this one, or should I?

Reviewable status: 71 of 102 files reviewed, 9 unresolved discussions (waiting on @pmachapman)

---

src/RealtimeServer/scriptureforge/models/sf-project-user-config.ts line 32 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This property needs to be replicated in SFProjectUserConfig.cs.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/svg-icons/lynx-icons.ts line 22 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This SVG looks like it contains a redundant transform? Does something like this look similar, or have I misunderstood the SVG code?

  lynx_info: `<svg xmlns="http://www.w3.org/2000/svg" overflow="visible"
    preserveAspectRatio="none" width="13.423" height="11.758">
    <path d="M12.785 0c.248 0 .431.106.55.319a.618.618 0 0 1 0 .637L7.277
      11.44a.625.625 0 0 1-.567.318.625.625 0 0 1-.566-.318L.089.956a.618.618
      0 0 1 0-.637A.586.586 0 0 1 .637 0z"
      style="stroke-width:0;stroke-linecap:butt;stroke-linejoin:miter;fill:currentColor"
      vector-effect="non-scaling-stroke"/>
  </svg>`,

Good catch. Your suggestion prompted to to try asking AI to optimize the icons, and the result looks good.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-format-registry.service.ts line 40 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

More of a question:

What is your preference for converting a Set to an array in TypeScript? Array.from() or something like:

return [...this.registeredFormats];

(Our C# style config prefers this latter syntax)

Ah, spread syntax much better, thanks!

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.html line 164 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I'd love for this to be a tab, with for example vertical tab stacking, but I think that's a job for another PR!

Yeah, I brought back the ol' split pane. I needed the problems panel to be visible with the editor.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.html line 248 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should we use @if (showInsights) { instead?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.html line 254 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I didn't realize that Angular supported self-closing tags (since v16). Maybe we should have a separate PR that runs this migration? https://angular.dev/reference/migrations/self-closing-tags

I didn't know about the migration. That would be a good idea!

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-state.service.ts line 259 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Do we need a pipe(quietTakeUntilDestroyed(this.destroyRef)) (or similar) here, and below in the save state block?

I didn't add that since LynxInsightStateService is a singleton service provided at root, so it wouldn't be destroyed unless the app shut down.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts line 210 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should this be return [...this.curInsights.values()].flat();?

Yep!

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts line 300 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: See my earlier comments on perhaps using [...this.textDocIds] syntax. I don't mind which way we go, so long as we are consistent.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts line 324 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I thought this was a C pointer at first glance! :-)

Cool use of a generator.

That is Damien's code. I always forget about generator functions. I'll have to keep a mind out for judicious application of these.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-action-prompt/lynx-insight-action-prompt.component.ts line 35 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: I think isRtl is more consistent, although I do notice one other place a property called isLtr is used (text.component.ts). What was your rationale for making this isLtr?

No reason, I just picked one. I'll change it to isRtl to make it more consistent with other usages.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 2 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should we use `[ngClass]?

Yep, you're right. Changed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 8 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This string should be localized.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 26 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This string should be localized.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 55 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This should be localized

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.html line 59 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should we use `[ngClass]?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.scss line 135 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Is this a TODO to be addressed in this PR?

Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-overlay/lynx-insight-overlay.component.scss line 161 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Is this a TODO for this PR?

This one is for another PR.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-scroll-position-indicator/lynx-insight-scroll-position-indicator.component.html line 2 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should we use `[ngClass]?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-status-indicator/lynx-insight-status-indicator.component.html line 4 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should we use `[ngClass]?

Thanks for catching these.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.html line 36 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should we use `[ngClass]?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.scss line 122 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: This TODO was in another file. Just checking that is valid.

Removed 'code' display, as this is not useful to the user, and the insights are no longer grouped by 'code', as grouping by description is more accurate, because insights with different descriptions can have the same code (not planned when component was initially written).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insights-panel/lynx-insights-panel.component.ts line 364 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Is this TODO to be addressed in this PR? If you still want to use an ellipsis, you can use U+2026.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/quill-services/blots/lynx-insight-blot.ts line 8 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Wouldn't it be easier to just use the string value 'insight-id'?

Since the idAttributeName must be derived from the idDatasetPropName, I didn't want to allow them to get out of sync in case one gets renamed. I could use a test case to verify consistency, but since the overhead is so small with the static declaration, I thought that the source of truth being in the code was the better approach. Does this sound reasonable to you?

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts line 237 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Flagging your TODO!

Thank you!

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/includes.pipe.spec.ts line 7 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please add a test for transform<T\>

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-editor-registration/quill-format-registry.service.spec.ts line at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should you delete this empty file? QuillFormatRegistryService has good test coverage due to the tests for QuillRegistrations.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-editor-objects/lynx-insight-editor-objects.component.scss line at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should this file be deleted?

Makes sense.

[pmachapman]

Also, since the feature will be behind a feature flag, the browser lock ups issues can be addressed in a later PR as well, I think. That way users can start giving feedback sooner. Do you want to describe your findings in a Jira issue for this one, or should I?

I think that's OK as the lock ups only occur if the feature flag is enabled. You can create the ticket, if you like, if you able to recreate it?

Reviewed 31 of 31 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @siltomato)

---

src/SIL.XForge.Scripture/Models/SFProjectUserConfig.cs line 31 at r2 (raw file):

    public List<string> CommentRefsRead { get; set; } = [];
    public List<EditorTabPersistData> EditorTabsOpen { get; set; } = [];
    public LynxInsightUserData? LynxInsightUserData { get; set; }

This is named differently to the property in the node model. It should be:

public LynxInsightUserData? LynxInsightState { get; set; }

Code quote:

public LynxInsightUserData? LynxInsightUserData { get; set; }

---

src/SIL.XForge.Scripture/ClientApp/angular.json line 105 at r1 (raw file):

              "optimization": true,
              "outputHashing": "all",
              "namedChunks": true,

Normally we shouldn't set this in production as it increases bundle size, but I assume this is required so you can load the "node_modules_sillsdev_lynx" JS files?

Code quote:

"namedChunks": true,

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/base-services/insight-render.service.ts line 8 at r2 (raw file):

@Injectable()
export abstract class InsightRenderService {
  abstract render(insights: LynxInsight[], editor: LynxableEditor, editorViewModel: TextViewModel): void;

From an API perspective, this change has removed the ability to create InsightRenderService implementations for other editors. Are you sure you want to do that?

Maybe if you create an interface, perhaps called something like LynxEditorViewModel or LynxRangeConverter, have it contain dataRangeToEditorRange(), and have TextViewModel implement this interface, you will avoid this potential trap?

Code quote:

abstract render(insights: LynxInsight[], editor: LynxableEditor, editorViewModel: TextViewModel): void;

---

src/SIL.XForge.Scripture/Models/LynxInsightUserData.cs line 16 at r2 (raw file):

{
    public bool IsOpen { get; set; }
    public LynxInsightFilter Filter { get; set; }

As this is not nullable, it should have a default value, for example:

public LynxInsightFilter Filter { get; set; } = new LynxInsightFilter();

Code quote:

public LynxInsightFilter Filter { get; set; }

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-editor-objects/lynx-insight-editor-objects.component.ts line 27 at r2 (raw file):

  @Input() editor?: LynxableEditor;
  @Input() editorViewModel?: TextViewModel;

See my comment above in InsightRenderService

Code quote:

editorViewModel?: TextViewModel;

[siltomato]

Created ticket: https://jira.sil.org/browse/SF-3370

Reviewable status: 93 of 102 files reviewed, 4 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/angular.json line 105 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Normally we shouldn't set this in production as it increases bundle size, but I assume this is required so you can load the "node_modules_sillsdev_lynx" JS files?

Yes, it looks like Damien set this to load the lynx localization files.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/base-services/insight-render.service.ts line 8 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

From an API perspective, this change has removed the ability to create InsightRenderService implementations for other editors. Are you sure you want to do that?

Maybe if you create an interface, perhaps called something like LynxEditorViewModel or LynxRangeConverter, have it contain dataRangeToEditorRange(), and have TextViewModel implement this interface, you will avoid this potential trap?

Perfect. Good thinking.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-insight-editor-objects/lynx-insight-editor-objects.component.ts line 27 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

See my comment above in InsightRenderService

Done.

---

src/SIL.XForge.Scripture/Models/LynxInsightUserData.cs line 16 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

As this is not nullable, it should have a default value, for example:

public LynxInsightFilter Filter { get; set; } = new LynxInsightFilter();

Done.

---

src/SIL.XForge.Scripture/Models/SFProjectUserConfig.cs line 31 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This is named differently to the property in the node model. It should be:

public LynxInsightUserData? LynxInsightState { get; set; }

Done.