Adds "done and save" shortcut while selecting more models#562
Adds "done and save" shortcut while selecting more models#562Sukumarsawant wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the provider wizard’s “Done” flow by removing the selectable “Done & Save” option and replacing it with a keyboard shortcut plus an inline hint.
Changes:
- Removed “Done & Save” from the template selection options list.
- Added a
dkeyboard shortcut to complete when providers exist and the user is in template selection mode. - Displayed an inline hint indicating the
dshortcut is available.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useInput((input, key) => { | ||
| // "Done & Save" shortcut | ||
| if ( | ||
| mode === 'template-selection' && | ||
| providers.length > 0 && | ||
| typeof input === 'string' && | ||
| input.length > 0 && | ||
| input.toLowerCase() === DONE_SHORTCUT_KEY && | ||
| !key.ctrl && | ||
| !key.meta | ||
| ) { | ||
| onComplete(providers); | ||
| return; | ||
| } |
| {providers.length > 0 && ( | ||
| <Box marginTop={1}> | ||
| <Text color={colors.success}> | ||
| Done is always available: press {DONE_SHORTCUT_KEY} | ||
| </Text> |
akramcodez
left a comment
There was a problem hiding this comment.
@Sukumarsawant Thanks for working on this! Removing "Done & Save" from the provider list definitely feels like a UX improvement.
I have a couple of concerns before approving:
-
I don't see any automated tests covering the new shortcut behavior. Could we add tests for the success path (shortcut completes setup) and a few edge cases (e.g. no providers added)?
-
The new
dshortcut is a little risky because it's a plain character key. Do you think it could conflict with future type-to-search functionality or lead to accidental completion while navigating the template list? -
Minor copy nit: "Done is always available" isn't strictly true since the shortcut is only available in template-selection mode when providers exist. We could tweak the wording slightly.
Happy to take another look once those are addressed :)
for
I agree and will look into first one, since I haven't added tests in this codebase before . |
|
Hey @Sukumarsawant - big thanks on putting this together :D A few additions and thoughts on top of @akramcodez's review: 1. Tests - extending @akramcodez's point, this is a blocker not a nice-to-haveBeyond adding tests for the new shortcut, there are existing tests that will start failing against this branch. Specifically:
Also worth flagging: the PR description's "All existing tests pass" checkbox is unlikely to hold once the spec is updated, so please run 2. The
|
Description
Adds done and save shortcut and removes the redundant from the list.
fixes #559
Type of Change
Testing
You can now press 'd' and save the models added immediately
Automated Tests
.spec.ts/tsxfilespnpm test:allcompletes successfully)Manual Testing
Checklist
Assisted using nanocoder XD / copilot for greps (finding code)