Skip to content

SF-3844 SF-3786 SF-3914 Clear Lynx insights of a checker that stops reporting #4085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Nateowami wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

SF-3844 SF-3786 SF-3914 Clear Lynx insights of a checker that stops reporting #4085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 87 additions & 70 deletions ...Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down Expand Up @@ -233,7 +235,7 @@ describe('LynxWorkspaceService', () => {
uri: textDocId.toString(),
diagnostics
});
tick(10); // 10ms debounce time
tick(DIAGNOSTICS_DEBOUNCE_TIME);
}

setupActiveTextDocId(): void {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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: [
Expand All @@ -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: [
Expand All @@ -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'];
Expand All @@ -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
Expand All @@ -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: [
Expand All @@ -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);
Expand Down Expand Up @@ -1310,7 +1327,7 @@ describe('LynxWorkspaceService', () => {
}
]
});
tick(10);
tick(DIAGNOSTICS_DEBOUNCE_TIME);

env.diagnosticsChangedTestSubject$.next({
uri: textDocId2.toString(),
Expand All @@ -1324,7 +1341,7 @@ describe('LynxWorkspaceService', () => {
}
]
});
tick(10);
tick(DIAGNOSTICS_DEBOUNCE_TIME);

// Should receive all insights flattened
expect(insights.length).toBe(3);
Expand Down Expand Up @@ -1375,7 +1392,7 @@ describe('LynxWorkspaceService', () => {
}
]
});
tick(10);
tick(DIAGNOSTICS_DEBOUNCE_TIME);

expect(insights.length).toBe(2);

Expand All @@ -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);
Expand Down
43 changes: 34 additions & 9 deletions ...orge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/lynx-workspace.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ import { LynxWorkspaceFactory } from './lynx-workspace-factory.service';

const TEXTS_PATH_TEMPLATE = obj<SFProjectProfile>().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'
})
Expand Down Expand Up @@ -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
);

Expand All @@ -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<string, LynxInsight[]> =
this.curInsightsByEventUriAndSource.get(event.uri) ?? new Map<string, LynxInsight[]>();
const updatedInsightsBySource = new Map<string, LynxInsight[]>();

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';
Expand Down Expand Up @@ -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);
}
}

Expand Down
Loading