feat(vscode) - Adding additional csharp lsp server functionality#8828
Open
lambrianmsft wants to merge 45 commits intoAzure:ccastrotrejo/csharpLSPServerfrom
Open
feat(vscode) - Adding additional csharp lsp server functionality#8828lambrianmsft wants to merge 45 commits intoAzure:ccastrotrejo/csharpLSPServerfrom
lambrianmsft wants to merge 45 commits intoAzure:ccastrotrejo/csharpLSPServerfrom
Conversation
… call times for checking child process, added options to add new codeful workflows to the project, optimzed the get-child-process.ps1 script, and finally added the overview page for codeful workflows and ability to view runs
…ded temporary shouldExtractOrUpdate method
…d to be passed as a path and added additional telemetry to capture this whenever it happens
…ger name isn't defined, we pull the default from the lsp server sdk. Disabled the create unit test from run for codeful workflows
…perience, prevent callback url from getting continuously pinged, updated sdk version
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:❌ PR Title
✅ Commit Type
❌ Risk Level
|
| Section | Status | Recommendation |
|---|---|---|
| Title | ❌ | Make title more specific to LSP + codeful changes (see example above) |
| Commit Type | ✅ | No change needed |
| Risk Level | ❌ | Add a GitHub label risk:high and update PR body to match (advised: High) |
| What & Why | Expand to list key user/developer/system changes and rationale | |
| Impact of Change | ❌ | Fill Users / Developers / System with specific impacts (see recommendations) |
| Test Plan | ❌ | Update checkboxes: Unit tests added (yes). List tests & run instructions. |
| Contributors | Add optional credits (PMs/Designers/Reviewers) | |
| Screenshots/Videos | Add screenshots or short GIFs for UI changes (recommended) |
Final notes
- Why I increased the advised risk to HIGH: this PR changes process-handling and lifecycle (design-time/runtime start/stop), updates PowerShell scripts that query OS processes, adds caching around auth and bundle versions, changes the way codeful workflows are created/inserted (including writing Program.cs and csproj), changes package and binary assets included in the extension, and adds behavior that affects builds/publishing. These touches cross critical runtime and installation boundaries and warrant high risk classification, extra review, and likely a staged rollout.
What I need from you to move this to passing:
- Add an explicit GitHub label
risk:high(or change the PR Risk Level to match an existing risk label) and update the PR body tickboxes accordingly. - Update the PR Title to be more descriptive and scoped (see suggested example).
- Update the PR body:
- Expand "What & Why" with 2–4 concrete bullets describing the main changes and rationale.
- Complete the "Impact of Change" sections for Users, Developers, and System using the details above (or the exact changes you expect). Make sure to mention the new LSP assets and extension bundle changes.
- Update the Test Plan: mark Unit tests added/updated and list the primary test files / commands to run them. Confirm whether there are E2E tests and how to run them; if none, uncheck the E2E box.
- Add a short summary of manual testing steps you performed (environments, basic commands), especially for platform-specific changes (Windows PowerShell child-process detection) and codeful workflows.
- Optionally attach screenshots for UI changes (connection view loading spinner, overview page) and list any release notes or migration steps for users (e.g., extension bundle update or LSP SDK changes).
Please update the PR body and labels per the recommendations and re-request review. Given the scope, I also recommend requesting an additional reviewer who is familiar with runtime/process management and packaging.
Last updated: Fri, 13 Mar 2026 01:09:08 GMT
| } else { | ||
| // For codeless projects, build custom code functions if they exist | ||
| const customFolderExists = await fse.pathExists(path.join(logicAppNode.fsPath, libDirectory, customDirectory)); | ||
| if (customFolderExists) { |
Contributor
There was a problem hiding this comment.
Shouldn't this be if !customFolderExists? Since we would build in the case where lib/custom/* doesn't already exist for codeless, i.e. not already built
| // For codeless projects, build custom code functions if they exist | ||
| const customFolderExists = await fse.pathExists(path.join(logicAppNode.fsPath, libDirectory, customDirectory)); | ||
| if (customFolderExists) { | ||
| await buildCustomCodeFunctionsProject(actionContext, logicAppNode); |
Contributor
There was a problem hiding this comment.
buildCustomCodeFunctionsProject is supposed to check whether the .net project is specifically a custom code functions project before building, part of that is to check for Microsoft.Azure.Workflows.Webjobs.Sdk in the .csproj, which I don't think will exist for codeful projects. Should there be a separate function for handling building codeful?
This was referenced Feb 24, 2026
…AppsUX into ccastrotrejo/csharpLSPServer
|
|
||
| // Find the location to insert the new workflow builder | ||
| // Look for the "Build all workflows" comment or insert before host.Run() | ||
| const buildCallRegex = /(\s*)(\/\/ Build all workflows\s*\n(?:\s*\/\/.*\n)*\s*)(.*?)(\s*host\.Run\(\);)/s; |
Contributor
There was a problem hiding this comment.
Is it possible to avoid using a regex here to update Program.cs with a new workflow, and instead just overwrite Program.cs with one containing all workflows? We would need to track the workflows in a codeful project elsewhere, but we should probably avoid using regex as much as possible since it's bound to break in some cases
This was referenced Mar 4, 2026
…tion, update tests
…o reduce azure auth time, updated connectionTable to look if a selected connection is inside the connections.json, updated flow to prevent hanging after use connectors from azure is selected initally
…brianmsft/LogicAppsUX into ccastrotrejo/csharpLSPServer
…ic operation for connectionView
… to azure, updated lsp server with correct codelens location, fixed connection insertion locations
…sUX into ccastrotrejo/csharpLSPServer
…that displays manage or create connection depending on connection.json
…brianmsft/LogicAppsUX into ccastrotrejo/csharpLSPServer
…d for the first time, debug session now attaches properly, and run history webview refreshes.
andrew-eldridge
approved these changes
Mar 13, 2026
This was referenced Mar 13, 2026
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
2 participants
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.
Commit Type
Risk Level
What & Why
Adding ability to open overview page for a workflow, adding a new workflow, reducing the connection pane load time, and updating the package.
Impact of Change
Workflows runs can now be opened and new workflows can be added
Test Plan
Contributors
Screenshots/Videos