Skip to content

Adds "done and save" shortcut while selecting more models#562

Open
Sukumarsawant wants to merge 2 commits into
Nano-Collective:mainfrom
Sukumarsawant:done_nit
Open

Adds "done and save" shortcut while selecting more models#562
Sukumarsawant wants to merge 2 commits into
Nano-Collective:mainfrom
Sukumarsawant:done_nit

Conversation

@Sukumarsawant

@Sukumarsawant Sukumarsawant commented Jun 11, 2026

Copy link
Copy Markdown

Description

Adds done and save shortcut and removes the redundant from the list.
fixes #559

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Testing

image

You can now press 'd' and save the models added immediately

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

Assisted using nanocoder XD / copilot for greps (finding code)

Copilot AI review requested due to automatic review settings June 11, 2026 22:08
@Sukumarsawant Sukumarsawant requested a review from Avtrkrb as a code owner June 11, 2026 22:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 d keyboard shortcut to complete when providers exist and the user is in template selection mode.
  • Displayed an inline hint indicating the d shortcut is available.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread source/wizards/steps/provider-step.tsx
Comment thread source/wizards/steps/provider-step.tsx
Comment thread source/wizards/steps/provider-step.tsx
@Sukumarsawant Sukumarsawant requested a review from Copilot June 11, 2026 22:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Sukumarsawant Sukumarsawant requested a review from Copilot June 12, 2026 06:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment on lines +507 to +520
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;
}
Comment on lines +623 to +627
{providers.length > 0 && (
<Box marginTop={1}>
<Text color={colors.success}>
Done is always available: press {DONE_SHORTCUT_KEY}
</Text>
@akramcodez akramcodez self-assigned this Jun 12, 2026

@akramcodez akramcodez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. 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)?

  2. The new d shortcut 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?

  3. 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 :)

@Sukumarsawant

Sukumarsawant commented Jun 12, 2026

Copy link
Copy Markdown
Author
  1. The new d shortcut 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?
  2. 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.

for

  1. It's not a new thing introduced but is use previously in the part where you select models.
    I am thinking it as an alternative when the enter is reserved for something else .(for eg selecting models and d for letting know you are done and save it )

  2. That too was already there in the same, I looked at it as a statement "d is always available to mark done & save".
    I am open to know if that should me differently like "press d to mark done and save" types . or if there is different shortcut preferred which can be used when enter is reserved .

I agree and will look into first one, since I haven't added tests in this codebase before .
Also if any more view on the above comments

@will-lamerton

Copy link
Copy Markdown
Member

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-have

Beyond adding tests for the new shortcut, there are existing tests that will start failing against this branch.

Specifically:

  • source/wizards/steps/provider-step.spec.tsx:297-315 asserts t.regex(output!, /Done & Save/) on the template-selection render. With this change, the template list no longer contains a Done & Save entry, and the new hint copy ("Done is always available: press d") doesn't contain the literal string Done & Save, so the regex won't match. Please update this assertion and add a new one that exercises the d shortcut (and confirms it does nothing when no providers exist).
  • source/wizards/steps/provider-step.spec.tsx:317-322 (t.notRegex on the empty-providers case) should still pass, but worth tightening it to also assert the new hint is absent in that case.

Also worth flagging: the PR description's "All existing tests pass" checkbox is unlikely to hold once the spec is updated, so please run pnpm test:all locally and update the description to match reality (no shame in ticking "I updated tests as part of this change" - that's exactly what we want).

2. The d shortcut - echoing @akramcodez's concern, with a concrete fix

Agreed this is risky. useInput at source/wizards/steps/provider-step.tsx:507-519 fires for any keystroke in template-selection, not just when the list has focus. Today SelectInput has no typeahead so there's no live conflict, but this is a real foot-gun if anyone ever adds search/filter. Two cleaner options:

  • Ctrl+D / Cmd+D - discoverable, no conflict with typeahead, idiomatic for "done" in CLI tools.
  • Keep plain d but gate it on the list having focus - a bit fiddlier with Ink's input model, harder to test.

I'd lean toward Ctrl+D / Cmd+D. If you'd rather keep d for discoverability, please leave a comment in the code explaining the assumption that SelectInput stays typeahead-free.

3. Hint copy - +1 on @akramcodez's nit

"Done is always available: press d" isn't strictly true - the hint is only rendered when providers.length > 0 (provider-step.tsx:621). Suggest: Done is available: press d (drop "always"), or scope it more explicitly, e.g. Press d to finish setup.

4. Scope - additional concern not yet raised

This is one I want to flag separately, because it changes the shape of the PR. Issue #559 is about consistency across the codebase, and right now this PR only touches provider-step.tsx. Two more spots still show Done & Save as a list item:

  • source/wizards/steps/mcp-step.tsx:140 - same pattern, same UX problem.
  • source/wizards/steps/provider-step.tsx:185 (handleInitialSelect's done branch) plus getInitialOptions (around lines 147-153) - the initial-options path still emits the list item even though the shortcut exists.

If the intent is "shortcut everywhere, list item nowhere", both need the same treatment. If the intent is just "fix the most prominent one", that's fine too - but please clarify in the PR description so reviewers don't have to guess. Happy to split this into two PRs if you'd rather keep #562 small and follow up with a parity PR.

Happy to take another look once tests are in. Nice work on this one. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consistency for "Done and save"/"Enter"

4 participants