Improve background progress reporting - #1545
Open
TheAngryByrd wants to merge 6 commits into
Open
Conversation
…hared progress reporter
…g in typechecking
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation propagation, configuration handling, duplicate-operation tracking, and one test assertion are currently incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces shared LSP progress reporting for typechecking, analyzers, project loading, and dependent-file checks.
Changes:
- Adds shared, cancellable batch progress infrastructure.
- Integrates reporters into server workflows.
- Adds concurrency helpers and reporter tests.
File summaries
| File | Description |
|---|---|
FSharpLspClient.fs |
Implements shared progress reporting. |
FSharpLspClient.fsi |
Exposes the reporter API. |
AdaptiveServerState.fs |
Integrates shared progress and cancellation. |
Utils.fs |
Adds percentage-based parallelism helpers. |
Utils.fsi |
Declares new parallelism helpers. |
SharedTypecheckProgressReporterTests.fs |
Tests reporter behavior. |
Program.fs |
Registers the new tests. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+236
to
+237
| let simpleName = IO.Path.GetFileName fileName | ||
| activeFiles <- Set.add simpleName activeFiles |
Comment on lines
+208
to
+209
| let projectLoadingProgressReporter = | ||
| new SharedTypecheckProgressReporter("Loading Projects", fun () -> progressLookup.CreateProgressReport(lspClient)) |
| opts, | ||
| shouldCache = shouldCache | ||
| ) | ||
| |> Async.withCancellation progressCt |
| fileToken, | ||
| progressReporter.CancellationToken | ||
| ) | ||
| CancellationTokenSource.CreateLinkedTokenSource(rootToken, fileToken, progressCt) |
|
|
||
| if config.Notifications.BackgroundServiceProgress then | ||
| do! progress.Begin($"Checking for unnecessary parentheses {fileName}...", message = filePathUntag) | ||
| use! _progress = analyzerProgressReporter.Begin($"Unnecessary parens - {fileName}") |
Comment on lines
+361
to
+377
| // Second batch — counters should have been reset | ||
| let! batchDisp2 = | ||
| reporter.BeginBatch ([| "C:/src/C.fs"; "C:/src/D.fs" |]) CancellationToken.None | ||
| |> Async.AwaitTask | ||
|
|
||
| do! settle () | ||
|
|
||
| // The Begin for second batch should show 0% (0/2) | ||
| let callsArr = calls.ToArray() | ||
|
|
||
| let hasZeroPercentage = | ||
| callsArr | ||
| |> Array.exists (fun c -> | ||
| match c with | ||
| | Begin(_, _, Some 0u) -> true | ||
| | Report(_, Some 0u) -> true | ||
| | _ -> false) |
| Expect.isTrue (ct = CancellationToken.None) "CancellationToken should be None after all files end" | ||
| } | ||
|
|
||
| testCaseAsync "Multiple sequential files reuse the same report cycle" |
Comment on lines
+293
to
+295
| let normalizedFiles = files |> Array.map normalizePath |> Array.distinct | ||
| batchTotal <- batchTotal + normalizedFiles.Length | ||
| batchFiles <- Set.union batchFiles (normalizedFiles |> Set.ofArray) |
TheAngryByrd
force-pushed
the
better-progress-reporting
branch
from
August 31, 2026 11:13
69e74d3 to
d7e1661
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FSharp.notifications.backgroundServiceProgressfor all shared progress reportersTesting
dotnet fantomas --check build.fsx srcdotnet build test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj -c Release -f net10.0 -p:ImportDirectoryPackagesProps=false --nologodotnet run -c Release -f net10.0 --no-build --project test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj -- --filter-test-list "SharedTypecheckProgressReporter"— 17 passedThe full solution build reached the changed projects, but nested TestExplorer sample builds imported a parent
Directory.Packages.propsand failed withNU1008. The focused LSP build completed with zero warnings and errors.