Skip to content

feat(tui): add config key support to provider create/update forms#2224

Open
letv1nnn wants to merge 12 commits into
NVIDIA:mainfrom
letv1nnn:tui-provider-config-keys-creation
Open

feat(tui): add config key support to provider create/update forms#2224
letv1nnn wants to merge 12 commits into
NVIDIA:mainfrom
letv1nnn:tui-provider-config-keys-creation

Conversation

@letv1nnn

Copy link
Copy Markdown

Summary

Add config key (config map) support to the TUI's Create Provider and Update Provider forms.

Previously, both forms only handled credentials and hardcoded config: HashMap::default(), making it impossible to set provider config keys like ANTHROPIC_BASE_URL or OPENAI_BASE_URL via openshell term. The CLI's --config KEY=VALUE flag worked correctly, only the TUI surface was affected.

Related Issue

#1798

Changes

  • Create Provider form: Added config key/value input section with Tab-based navigation between key and
    value fields, auto-commit on Tab, and Ctrl+D to delete entries
  • Update Provider form: Added config key/value editing with existing config hydrated from the provider
    on form open, preserving config that the user doesn't modify
  • Spawn functions: spawn_create_provider and spawn_update_provider now plumb the form's config map
    into the gRPC request instead of sending empty maps

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Screenshots

You can run cargo run -p openshell-cli -- term to try it out yourself.

Provider Details now lists config keys:

image

Updating Provider Details

image image

letv1nnn added 5 commits July 8, 2026 12:13
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 11, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@letv1nnn

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

@letv1nnn

Copy link
Copy Markdown
Author

recheck

@krishicks

Copy link
Copy Markdown
Collaborator

I had gpt-5.6-sol medium review this, and here's what it found:

I found two blocking functional issues and two additional edge cases.

Required changes

  1. Config deletion does not persist during provider updates.

Ctrl+D removes the entry from the local IndexMap, but the update request simply omits that entry. The server treats config as a merge patch: omitted keys remain unchanged, and deletion requires sending the key with an empty value.

Please track deleted entries and send them as key: "". This should also allow deleting the final config entry, which the current submit validation prevents.

  1. The cursor can select and delete invisible entries.

When there are more than six config entries, the renderer shows the first five plus an overflow summary. However, the cursor can traverse the entire map. Once it moves beyond the visible entries, Ctrl+D can delete an entry the user cannot see.

Please render a cursor-relative window so the selected entry always remains visible.

Additional concerns

  1. The update form hydrates and resends the complete config map with resource_version: 0. This can replay a stale value over a concurrent update to the same key. Sending only added, modified, and deleted entries would match the server’s merge semantics and address this alongside deletion.

  2. Pending key/value input can be silently discarded if the user navigates backward to Submit without triggering flush_config_input(). Please flush or validate pending input during submission.

Validation requested

Please add state-machine or request-construction tests covering:

  • Deleting an existing config entry sends an empty-value tombstone.
  • Deleting the final config entry is allowed.
  • The selected entry remains visible with more than six entries.
  • Pending key/value input cannot be silently lost during submission.
  • Update requests contain only config deltas rather than the complete hydrated snapshot.

letv1nnn added 4 commits July 17, 2026 15:34
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
… window

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@letv1nnn

Copy link
Copy Markdown
Author

Hey @krishicks, all four issues fixed, all five tests added. Thanks for the thorough review!

@krishicks krishicks self-assigned this Jul 17, 2026
@krishicks

Copy link
Copy Markdown
Collaborator

/ok to test 86ab2eb

@copy-pr-bot

copy-pr-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown

/ok to test 86ab2eb

@krishicks, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@krishicks

Copy link
Copy Markdown
Collaborator

/ok to test 69aa1a8

@krishicks

krishicks commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

I built this locally and tried using it. I found two issues, confirmed by gpt-5.6-sol medium:

  1. Update can never hydrate existing config. Update is only available when providers_v2_enabled == false (crates/openshell-tui/src/app.rs:1243), but refresh_providers only populates provider_entries when that flag is true (crates/openshell-tui/src/lib.rs:1973). The update form reads config exclusively from provider_entries (crates/openshell-tui/src/app.rs:2576), so its config list is always empty whenever Update is reachable. Provider Detail works because it fetches the provider directly. This makes existing config impossible to delete through the TUI.

  2. Enter commits the pair without resetting focus. After Enter successfully flushes the key/value pair, focus remains on ConfigKeyValue (crates/openshell-tui/src/app.rs:2443). The displayed “key” is the empty Key field’s placeholder, while the cursor remains on Val. The same defect exists in Update at crates/openshell-tui/src/app.rs:2704. Successful flush should clear any status and move focus back to ConfigKeyName/ConfigKey.

Additionally, gpt-5.6-sol medium found these issues:

  1. Re-added config keys are still deleted on submission. Deleting an entry permanently adds its key to deleted_keys (crates/openshell-tui/src/app.rs:2684). If the user re-adds that key, the request builder first includes its new value, then overwrites it with the empty deletion tombstone (crates/openshell-tui/src/lib.rs:1708). Thus, delete FOO, re-add FOO=new, submit results in FOO being deleted. Derive deletions from original_config - config, or remove a key’s tombstone whenever it is reinserted.

I could not test this because I could not delete an entry through the TUI.

  1. Create can silently discard pending config input. Forward Tab navigation flushes a completed key/value pair, but the final create handler does not (crates/openshell-tui/src/app.rs:2460). A user can enter both fields, navigate backward with Shift-Tab until Submit, and create a provider without that config. The create submission path should flush and validate pending input just like update does.

I confirmed this behavior exists on create, not on update.

  1. The overflow indicator is rendered outside its allocated area. Both forms allocate at most MAX_VISIBLE_CONFIG rows (crates/openshell-tui/src/ui/create_provider.rs:256), but the renderer takes that many entries and then appends an additional “and N more” line (crates/openshell-tui/src/ui/create_provider.rs:994). The indicator is clipped whenever more entries remain. Reserve one row for it or reduce take_count when overflow exists.

I confirmed this by adding 7 entries and seeing no indication of the 7th entry in the list. It should say "and 1 more" or similar.

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@letv1nnn

Copy link
Copy Markdown
Author

thanks, fixed described bugs.

@krishicks

Copy link
Copy Markdown
Collaborator

/ok to test 41f6b2d

@krishicks

Copy link
Copy Markdown
Collaborator

When editing a provider after adding keys, you can now delete entries, but the cursor appears active in two places: an existing key=value pair, but also in the Key: portion of the form below, which has an underscore instead of the 'key' placeholder. Instead, only the existing key=value pair should be highlighted. This was confirmed by gpt-5.6-sol medium:

Existing config selection and new-key input share the same focus state. When UpdateProviderField::ConfigKey is active, the renderer both:

  • Highlights the selected existing entry through config_focused (crates/openshell-tui/src/ui/create_provider.rs:842).
  • Renders the Key input as active with an underscore cursor (crates/openshell-tui/src/ui/create_provider.rs:871).

That same state handles Up/Down and Ctrl+D for existing entries, but also accepts text for a new key (crates/openshell-tui/src/ app.rs:2675). Consequently, the UI communicates two simultaneous focus targets.

The clean fix is to add a distinct ConfigEntry or ConfigList focus state:

  • ConfigEntry: highlight the selected pair; handle Up, Down, and Ctrl+D.
  • ConfigKey: show the underscore and accept new-key text.
  • Skip ConfigEntry during Tab navigation when the config list is empty.

The Create form uses the same combined interaction pattern, so it should receive the equivalent focus state as well.

gpt-5.6-sol medium also found this, which I confirmed:

Partially entered config is still silently discarded. Both submit handlers call flush_config_input but ignore failure
(crates/openshell-tui/src/app.rs:2464, crates/openshell-tui/src/app.rs:2726). If only Key or Val is populated and the user reaches Submit via backward navigation, submission continues without warning. It should show the same “Both key and value required” validation used by forward navigation.

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@letv1nnn

Copy link
Copy Markdown
Author

fixed both issues.

@krishicks

Copy link
Copy Markdown
Collaborator

/ok to test 91894db

@krishicks

Copy link
Copy Markdown
Collaborator

On create, after submitting a key=value pair I can immediately shift-tab and delete it, but the same flow does not work on update. I add a key=value pair, save the provider, then go back to update the provider and enter a new key=value pair. The second pair cannot be deleted, only the previously-saved one.

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@letv1nnn

Copy link
Copy Markdown
Author

done, shift+tab now lands on the newly added entry

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.

2 participants