Add platform-specific terminal app checking #1983
Conversation
📊 Performance Test ResultsComparing 50229b1 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
epeicher
left a comment
epeicher
left a comment
There was a problem hiding this comment.
Thanks for improving this @ivan-ottinger! I have tested on both, Mac and Windows, and they display the correct Terminal applications. LGTM! 
| Mac | Windows |
|---|---|
 |
 |
I have left a minor suggestion of refactor to consider, but feel free to progress with this version if you prefer it
src/stores/installed-apps-api.ts
Outdated
| terminalConfig, | ||
| isTerminalSupportedOnPlatform, |
There was a problem hiding this comment.
Non-blocking suggestion: What do you think about adding a method that returns directly the terminal supported by platform? Something like getTerminalsSupportedOnPlatform so the lib/terminal is in charge of doing the filtering? I think this leaves the filtering responsibility to the terminal lib and makes the code more maintainable.
There was a problem hiding this comment.
Thank you for the suggestion, Roberto! I have made the change in 6d22cf7.
I agree it is more maintainable and easier to read as well. 🙂
There was a problem hiding this comment.
Thanks @ivan-ottinger for addressing the comment! I’ve tested it again on Mac and Windows, and it works perfectly 🙌
* Add platform-specific terminal app checking * Fix typo * Fix tests * Improve terminal support detection and filtering
Related issues
Related to STU-931
Proposed Changes
On Monday I am planning to propose adding PowerShell as another terminal option on Windows in another PR.→ Update: After further consideration I don't think it makes sense to add PowerShell. In the future we could consider some additional options though.
Testing Instructions
npm install && npm starton Windows.Command PromptandWarpapp options available.Pre-merge Checklist