From 53570d0cf7c2961c387bee7fb717b2495192172a Mon Sep 17 00:00:00 2001 From: Nathaniel Paulus Date: Wed, 2 Sep 2026 22:33:55 -0400 Subject: [PATCH] SF-3844 SF-3786 SF-3914 Clear Lynx insights of a checker that stops reporting --- .../insights/lynx-workspace.service.spec.ts | 157 ++++++++++-------- .../lynx/insights/lynx-workspace.service.ts | 43 ++++- 2 files changed, 121 insertions(+), 79 deletions(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.spec.ts index 5512db59895..b1dee4c13d0 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.spec.ts @@ -27,6 +27,8 @@ import { LynxWorkspaceService, TextDocReader } from './lynx-workspace.service'; describe('LynxWorkspaceService', () => { const PROJECT_ID = 'project01'; + /** Enough time for the service to settle diagnostics events for a doc and then emit the insights. */ + const DIAGNOSTICS_DEBOUNCE_TIME = 25; const BOOK_NUM = 40; const CHAPTER_NUM = 1; const TEST_CONTENT = 'This is test content.'; @@ -233,7 +235,7 @@ describe('LynxWorkspaceService', () => { uri: textDocId.toString(), diagnostics }); - tick(10); // 10ms debounce time + tick(DIAGNOSTICS_DEBOUNCE_TIME); } setupActiveTextDocId(): void { @@ -1071,7 +1073,7 @@ describe('LynxWorkspaceService', () => { }; env.diagnosticsChangedTestSubject$.next(diagnosticsChangedEvent); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); // Should receive 3 insights total expect(insights.length).toBe(3); @@ -1113,7 +1115,15 @@ describe('LynxWorkspaceService', () => { const textDocId = new TextDocId(PROJECT_ID, BOOK_NUM, CHAPTER_NUM); const { insights, subscription } = env.captureInsights(); - // Add insights from source-a + const sourceBDiagnostic = { + code: '002', + source: 'source-b', + range: { start: { line: 0, character: 6 }, end: { line: 0, character: 10 } }, + severity: DiagnosticSeverity.Error, + message: 'Error from source B' + }; + + // Add insights from source-a and source-b env.diagnosticsChangedTestSubject$.next({ uri: textDocId.toString(), diagnostics: [ @@ -1123,29 +1133,15 @@ describe('LynxWorkspaceService', () => { range: { start: { line: 0, character: 0 }, end: { line: 0, character: 5 } }, severity: DiagnosticSeverity.Warning, message: 'Warning from source A' - } - ] - }); - tick(10); - - // Add insights from source-b - env.diagnosticsChangedTestSubject$.next({ - uri: textDocId.toString(), - diagnostics: [ - { - code: '002', - source: 'source-b', - range: { start: { line: 0, character: 6 }, end: { line: 0, character: 10 } }, - severity: DiagnosticSeverity.Error, - message: 'Error from source B' - } + }, + sourceBDiagnostic ] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); expect(insights.length).toBe(2); - // Update only source-a with new diagnostic + // Update source-a's diagnostic, leaving source-b's diagnostic as it was env.diagnosticsChangedTestSubject$.next({ uri: textDocId.toString(), diagnostics: [ @@ -1155,12 +1151,13 @@ describe('LynxWorkspaceService', () => { range: { start: { line: 0, character: 11 }, end: { line: 0, character: 15 } }, severity: DiagnosticSeverity.Information, message: 'Updated insight from source A' - } + }, + sourceBDiagnostic ] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); - // Should now have insights from both sources + // Should still have insights from both sources expect(insights.length).toBe(2); const curInsightsByEventUriAndSource = env.service['curInsightsByEventUriAndSource']; @@ -1179,7 +1176,7 @@ describe('LynxWorkspaceService', () => { subscription.unsubscribe(); })); - it('should reuse insight ids for matching diagnostics within the same source', fakeAsync(() => { + it('should clear insights of a source that no longer reports diagnostics', fakeAsync(() => { const env = new TestEnvironment(); // Set up project with lynx features enabled @@ -1199,7 +1196,14 @@ describe('LynxWorkspaceService', () => { const textDocId = new TextDocId(PROJECT_ID, BOOK_NUM, CHAPTER_NUM); const { insights, subscription } = env.captureInsights(); - // Send initial diagnostic + const sourceBDiagnostic = { + code: '002', + source: 'source-b', + range: { start: { line: 0, character: 6 }, end: { line: 0, character: 10 } }, + severity: DiagnosticSeverity.Error, + message: 'Error from source B' + }; + env.diagnosticsChangedTestSubject$.next({ uri: textDocId.toString(), diagnostics: [ @@ -1208,60 +1212,73 @@ describe('LynxWorkspaceService', () => { source: 'source-a', range: { start: { line: 0, character: 0 }, end: { line: 0, character: 5 } }, severity: DiagnosticSeverity.Warning, - message: 'Warning message' - } + message: 'Warning from source A' + }, + sourceBDiagnostic ] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); + + expect(insights.length).toBe(2); + // Source-a's diagnostic is resolved (as when its insight action is applied), while source-b still + // reports its diagnostic, so the event no longer includes source-a env.diagnosticsChangedTestSubject$.next({ uri: textDocId.toString(), - diagnostics: [ - { - code: '002', - source: 'source-b', - range: { start: { line: 0, character: 0 }, end: { line: 0, character: 5 } }, - severity: DiagnosticSeverity.Warning, - message: 'Warning message' - } - ] + diagnostics: [sourceBDiagnostic] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); - env.diagnosticsChangedTestSubject$.next({ - uri: textDocId.toString(), - diagnostics: [ - { - code: '003', - source: 'source-c', - range: { start: { line: 0, character: 0 }, end: { line: 0, character: 5 } }, - severity: DiagnosticSeverity.Warning, - message: 'Warning message' - } - ] + expect(insights.length).toBe(1); + expect(insights[0].description).toBe('Error from source B'); + + const uriMap = env.service['curInsightsByEventUriAndSource'].get(textDocId.toString())!; + expect(uriMap.has('source-a')).toBe(false); + + subscription.unsubscribe(); + })); + + it('should reuse insight ids for matching diagnostics within the same source', fakeAsync(() => { + const env = new TestEnvironment(); + + // Set up project with lynx features enabled + const projectDoc = env.createMockProjectDoc(PROJECT_ID, { + autoCorrectionsEnabled: false, + assessmentsEnabled: true, + punctuationCheckerEnabled: true, + allowedCharacterCheckerEnabled: false }); - tick(10); + env.projectDocTestSubject$.next(projectDoc); + when(mockActivatedProjectService.projectDoc).thenReturn(projectDoc); + when(mockActivatedProjectService.projectId).thenReturn(PROJECT_ID); + + env.setupActiveTextDocId(); + tick(); // Allow workspace setup to complete + + const textDocId = new TextDocId(PROJECT_ID, BOOK_NUM, CHAPTER_NUM); + const { insights, subscription } = env.captureInsights(); + + const diagnostics = ['source-a', 'source-b', 'source-c'].map((source, index) => ({ + code: `00${index + 1}`, + source, + range: { start: { line: 0, character: 0 }, end: { line: 0, character: 5 } }, + severity: DiagnosticSeverity.Warning, + message: 'Warning message' + })); + + // Send initial diagnostics + env.diagnosticsChangedTestSubject$.next({ uri: textDocId.toString(), diagnostics }); + tick(DIAGNOSTICS_DEBOUNCE_TIME); const originalIdA = insights[0].id; const originalIdB = insights[1].id; const originalIdC = insights[2].id; - // Send same diagnostic again (simulating re-analysis) - env.diagnosticsChangedTestSubject$.next({ - uri: textDocId.toString(), - diagnostics: [ - { - code: '001', - source: 'source-a', - range: { start: { line: 0, character: 0 }, end: { line: 0, character: 5 } }, - severity: DiagnosticSeverity.Warning, - message: 'Warning message' - } - ] - }); - tick(10); + // Send same diagnostics again (simulating re-analysis) + env.diagnosticsChangedTestSubject$.next({ uri: textDocId.toString(), diagnostics }); + tick(DIAGNOSTICS_DEBOUNCE_TIME); - // Id should be preserved + // Ids should be preserved expect(insights[0].id).toBe(originalIdA); expect(insights[1].id).toBe(originalIdB); expect(insights[2].id).toBe(originalIdC); @@ -1310,7 +1327,7 @@ describe('LynxWorkspaceService', () => { } ] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); env.diagnosticsChangedTestSubject$.next({ uri: textDocId2.toString(), @@ -1324,7 +1341,7 @@ describe('LynxWorkspaceService', () => { } ] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); // Should receive all insights flattened expect(insights.length).toBe(3); @@ -1375,7 +1392,7 @@ describe('LynxWorkspaceService', () => { } ] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); expect(insights.length).toBe(2); @@ -1384,7 +1401,7 @@ describe('LynxWorkspaceService', () => { uri: textDocId.toString(), diagnostics: [] }); - tick(10); + tick(DIAGNOSTICS_DEBOUNCE_TIME); // All insights should be cleared expect(insights.length).toBe(0); diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts index 206ada740ac..917504a1d75 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts @@ -47,6 +47,17 @@ import { LynxWorkspaceFactory } from './lynx-workspace-factory.service'; const TEXTS_PATH_TEMPLATE = obj().pathTemplate(p => p.texts); +/** + * Time (ms) to wait for a doc's diagnostics events to settle before processing them. + * + * The Lynx workspace merges the latest event from each diagnostic provider (checker) and re-emits the merged result + * every time any single provider reports. A single doc change therefore produces a burst of events for that doc, + * one per provider, each carrying the diagnostics of only the providers that have reported so far. Only the last + * event of the burst is complete. The providers validate synchronously, so the whole burst arrives within one task + * and any non-negative delay is enough to let it settle. + */ +const DIAGNOSTICS_SETTLE_TIME = 10; + @Injectable({ providedIn: 'root' }) @@ -268,7 +279,14 @@ export class LynxWorkspaceService { // Group events by event URI, then switchMap within each group to handle the cancellation and processing // of only the latest event for that URI. rxjsGroupBy(event => event.uri), - mergeMap(group$ => group$.pipe(switchMap(event => this.onDiagnosticsChanged(event)))), + mergeMap(group$ => + group$.pipe( + // Wait for the burst of per-provider events for the doc to settle so that only the final, complete + // event is processed (see DIAGNOSTICS_SETTLE_TIME). + debounceTime(DIAGNOSTICS_SETTLE_TIME), + switchMap(event => this.onDiagnosticsChanged(event)) + ) + ), debounceTime(10) // Debouncing avoids emitting after each event URI when loading a new project ); @@ -293,16 +311,22 @@ export class LynxWorkspaceService { parseInt(textDocIdParts[2]) ); - // Group diagnostics by source because onDiagnosticsChanged event may fire multiple times - // for the same URI (once for each diagnostic source). - // This way, 'current insights' for a different diagnostic source will not be cleared and insight id - // will be reused if the diagnostic matches an existing insight. + // Group diagnostics by source so that an insight id can be reused if the diagnostic matches an + // existing insight of the same source. const diagnosticsBySource = groupBy(event.diagnostics, 'source'); + // Lynx never states that a provider found nothing: a provider with no diagnostics is simply absent from the + // event. Since the debounced event is the complete report of every provider for this doc, a provider's + // silence has to be interpreted as "no issues found". So the insights of every source are rebuilt from the + // event, and a source that is absent from it (e.g. because applying an insight action resolved its last + // diagnostic) loses its previous insights rather than keeping them (SF-3844, SF-3786, SF-3914). + const prevInsightsBySource: Map = + this.curInsightsByEventUriAndSource.get(event.uri) ?? new Map(); + const updatedInsightsBySource = new Map(); + for (const [source, diagnosticsForSource] of Object.entries(diagnosticsBySource)) { const updatedInsightsForSource: LynxInsight[] = []; - const currentInsightsForSource: LynxInsight[] = - this.curInsightsByEventUriAndSource.get(event.uri)?.get(source) ?? []; + const currentInsightsForSource: LynxInsight[] = prevInsightsBySource.get(source) ?? []; for (const diagnostic of diagnosticsForSource) { let type: LynxInsightType = 'info'; @@ -346,9 +370,10 @@ export class LynxWorkspaceService { }); } - // Refresh the insights for this source only - this.curInsightsByEventUriAndSource.get(event.uri)!.set(source, updatedInsightsForSource); + updatedInsightsBySource.set(source, updatedInsightsForSource); } + + this.curInsightsByEventUriAndSource.set(event.uri, updatedInsightsBySource); } }