-
Notifications
You must be signed in to change notification settings - Fork 1
feat: improve performance and add support for custom vs code install … #19
feat: improve performance and add support for custom vs code install … #19
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances performance by caching workspace items with timed reloads, restructures how items are retrieved, and adds support for detecting custom VS Code installations via the PATH environment.
- Cache and refresh the workspace list in VSCodePage via a static flag and delayed reset
- Change workspace model to use properties for name/type/metadata instead of getters
- Scan the PATH variable in VSCodeHandler to register custom VS Code executables and sort instances
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| VsCode/Pages/VSCodePage.cs | Introduces _allItems cache, InitializeItemList, timed reload logic |
| VsCode/Commands/OpenVsCodeCommand.cs | Sets reload flag and changes navigation result in Invoke |
| VsCode/Classes/VSCodeWorkspace.cs | Replaces Get* methods with Set* voids and new workspace properties |
| VsCode/Classes/VSCodeInstance.cs | Exposes Icon property and makes GetIcon private |
| VsCode/Classes/VSCodeHandler.cs | Adds PATH scanning for custom installs and instance sorting |
Comments suppressed due to low confidence (4)
VsCode/Pages/VSCodePage.cs:14
- [nitpick] The name
LoadItemsreads like an action rather than a boolean flag. Consider renaming it to something likeNeedsReloadorShouldLoadItemsfor clarity.
public static bool LoadItems = true;
VsCode/Classes/VSCodeWorkspace.cs:51
- This
<returns>tag is invalid onSetName()since it returnsvoid. Please remove or update the XML comment.
/// <returns>The name of the workspace.</returns>
VsCode/Pages/VSCodePage.cs:32
- The new caching and reload logic in
UpdateSearchTextandGetItemscould benefit from targeted unit tests, especially around the timed reload behavior.
public override void UpdateSearchText(string oldSearch, string newSearch)
VsCode/Classes/VSCodeWorkspace.cs:127
Detailsis declared asDetailsElement[]but you’re assigning aList<DetailsElement>. You need to call.ToArray()or change the field toList<DetailsElement>.
Details = new List<DetailsElement>(){
|
|
||
| /* | ||
| var debugItem = new ListItem(new NoOpCommand()) | ||
| { | ||
| Title = "Debug", | ||
| Details = new Details() | ||
| { | ||
| Title = "Debug Information", | ||
| Metadata = [ | ||
| new DetailsElement() { Key = "Timestamp", Data = new DetailsTags() { Tags = [new Tag(DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss", CultureInfo.InvariantCulture))] } }, | ||
| new DetailsElement() { Key = "Timestamp", Data = new DetailsTags() { Tags = [new Tag(Debug)] } }, | ||
| ] | ||
| }, | ||
| }; | ||
| items.Insert(0, debugItem); | ||
| */ | ||
|
|
Copilot
AI
May 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] There's a large commented-out debug block. Consider removing it or moving it behind a debug flag to keep the codebase clean.
| /* | |
| var debugItem = new ListItem(new NoOpCommand()) | |
| { | |
| Title = "Debug", | |
| Details = new Details() | |
| { | |
| Title = "Debug Information", | |
| Metadata = [ | |
| new DetailsElement() { Key = "Timestamp", Data = new DetailsTags() { Tags = [new Tag(DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss", CultureInfo.InvariantCulture))] } }, | |
| new DetailsElement() { Key = "Timestamp", Data = new DetailsTags() { Tags = [new Tag(Debug)] } }, | |
| ] | |
| }, | |
| }; | |
| items.Insert(0, debugItem); | |
| */ | |
| if (IsDebugMode) | |
| { | |
| var debugItem = new ListItem(new NoOpCommand()) | |
| { | |
| Title = "Debug", | |
| Details = new Details() | |
| { | |
| Title = "Debug Information", | |
| Metadata = new List<DetailsElement>() | |
| { | |
| new DetailsElement() { Key = "Timestamp", Data = new DetailsTags() { Tags = new List<Tag>() { new Tag(DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss", CultureInfo.InvariantCulture)) } } }, | |
| new DetailsElement() { Key = "Debug Info", Data = new DetailsTags() { Tags = new List<Tag>() { new Tag("Debug") } } }, | |
| } | |
| }, | |
| }; | |
| items.Insert(0, debugItem); | |
| } |
| AddInstance("VS Code - Insiders", Path.Combine(appdataProgramFilesPath, "Programs", "Microsoft VS Code Insiders", "Code - Insiders.exe"), insiderStoragePath, VSCodeInstallationType.User, VSCodeType.Insider); | ||
| AddInstance("VS Code - Insiders [System]", Path.Combine(programsFolderPathBase, "Microsoft VS Code Insiders", "Code - Insiders.exe"), insiderStoragePath, VSCodeInstallationType.System, VSCodeType.Insider); | ||
|
|
||
| // search for custom installations in PATH environment variable |
Copilot
AI
May 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterating PATH entries and checking parent directories for executables can be expensive on systems with many entries. Consider caching the results or limiting directory checks.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.