feat(service-terminal): enhance terminal functionality with quick actions and doc redirection#2542
Conversation
…s and UI improvements
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## new-navigation #2542 +/- ##
=================================================
Coverage ? 45.30%
=================================================
Files ? 613
Lines ? 14321
Branches ? 4190
=================================================
Hits ? 6488
Misses ? 6675
Partials ? 1158
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nd action handling in terminal UI
…mproved placeholders and mock data in tests
| match(runningStatuses?.state) | ||
| .with('STOPPED', () => "We could not launch the CLI for this service because it's stopped.") | ||
| .with('ERROR', () => "We could not launch the CLI for this service because it's in error.") | ||
| .otherwise(() => "We could not launch the CLI for this service because it's not running."), |
There was a problem hiding this comment.
Here are all the potential values for the state:
export declare const ServiceStateDto: {
readonly STARTING: "STARTING";
readonly RUNNING: "RUNNING";
readonly ERROR: "ERROR";
readonly STOPPING: "STOPPING";
readonly STOPPED: "STOPPED";
readonly COMPLETED: "COMPLETED";
readonly WARNING: "WARNING";
};
Do we really want to fall back to "We could not launch the CLI for this service because it's not running." in all cases except for Stopped and Error?
There was a problem hiding this comment.
Well my approach is maybe a bit too simplistic, but most of the time user can't launch the CLI because there are no pods in their service, which is when services are either stopped or in error most of the time. I could have something for "Completed" also, but for the other states I'm not sure the CLI will fail because of a lack of pods 🤔
There was a problem hiding this comment.
Changed it to something more broad than just "Because it's not running" to avoid some confusion in some cases, I think it can suffice for now!
…rvice states and add corresponding test case
RemiBonnet
left a comment
RemiBonnet
left a comment
There was a problem hiding this comment.
Thanks @TheoGrandin74 🙏
Here's my feedback:
- The empty state isn’t centered, btw I wonder if it would be better to use the same font size for the buttons and the placeholder. “CLI docs” feels a bit too high compared to “AI Copilot”, for example
- We've a small jump with the loader
https://www.loom.com/share/9e28e2c498cf4abb95f51bd6ffa320af
libs/domains/services/feature/src/lib/service-terminal/terminal-shell-banner-addon.ts
Show resolved
Hide resolved
| .filter((commandPart): commandPart is string => Boolean(commandPart)) | ||
| .join(' ') | ||
| const portForwardCommand = `qovery port-forward --organization ${organizationId} --project ${projectId} --environment ${environmentId} --service ${serviceId} --port <local-port:target-port>` | ||
| const getShellCommand = useCallback(() => connectShellCommand, [connectShellCommand]) |
There was a problem hiding this comment.
Nice feature! Could you add small comment to explain it's necessary to allow the copy command in terminal?
There was a problem hiding this comment.
BTW, I think you can have an function in this component like buildCommand to build the different url, no?
It allow to avoid having too specific code here
There was a problem hiding this comment.
Should be good now!
libs/domains/services/feature/src/lib/service-terminal/input-search/input-search.tsx
Outdated
Show resolved
Hide resolved
…I elements for better usability
…consistency and focus visibility
3 participants
Summary
Before
After
Screenshots / Recordings
Testing
yarn testoryarn test -u(if you need to regenerate snapshots)yarn formatyarn lintPR Checklist
.cursor/rules)feat(service): add new Terraform service) - required for semantic-release