Skip to content

feat: add Ctrl-R (refresh) and Ctrl-S (save-local) keybindings to model selection - #27

Merged
rgielen merged 3 commits into
mainfrom
feat/issue-26-model-keybindings
Jul 4, 2026
Merged

feat: add Ctrl-R (refresh) and Ctrl-S (save-local) keybindings to model selection#27
rgielen merged 3 commits into
mainfrom
feat/issue-26-model-keybindings

Conversation

@rgielen

@rgielen rgielen commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • Ctrl-R (refresh): Re-fetches the provider's model list from the API during any model slot dialog, without leaving the dialog. For YAML-pinned providers and under --dry-run, prints a note to stderr and stays on the current list. Any already-chosen slot value no longer in the refreshed list is reset to "(do not set)" with a stderr note.
  • Ctrl-S (save toggle): Arms writing the completed selection to leitum.yaml (equivalent to --save-local). Toggles an armed state shown in the instruction footer as [save→project ON]. Unavailable (unbound, hint omitted) under --no-project-config. The dialog's final armed state fully governs the target when the dialog was shown (effective_save_local in run_claude).
  • An instruction footer on every slot select advertises active shortcuts and the armed state.
  • ModelSelectionResult dataclass replaces the bare dict return type of select_models.
  • ResolvedModels gains save_local and dialog_shown fields (defaults keep all existing tests valid).
  • PRD 03, README.md, and docs/commands.md updated to document the implemented sequential per-slot dialog design and the new keybindings.

Test plan

  • ruff format, ruff check, mypy --strict, pytest all pass (178 tests, 0 failures — verified before PR open)
  • 17 new unit tests cover: Ctrl-R callback invocation, not-applicable/dry-run skip, stale-value reset, callback failure fallback, Ctrl-S toggle, double toggle, initial armed state, save_local_allowed=False omits Ctrl-S, footer instruction content, effective_save_local in run_claude
  • Manual smoke (real TTY, multi-model API provider): leitum claude → Ctrl-R re-fetches; Ctrl-S shows [save→project ON] and writes leitum.yaml
  • Manual smoke: leitum --dry-run claude → refresh skip note, nothing written
  • Manual smoke: YAML-pinned provider → "not applicable" note
  • Manual smoke: leitum --no-project-config claude → no Ctrl-S hint in footer, key unbound

Closes #26

…el selection

- Add ModelSelectionResult dataclass (models, save_local) as the return type
  of select_models, replacing the bare dict
- Add _REFRESH and _SAVE_TOGGLE sentinel objects; key handlers exit the
  prompt_toolkit app with the sentinel, and the surrounding while loop
  interprets it (re-render vs. advance)
- _inject_key_bindings registers Ctrl-R eagerly on every slot; Ctrl-S is
  only registered when save_local_allowed=True (omitted under --no-project-config)
- _do_refresh implements the three Ctrl-R cases: normal re-fetch, YAML-pinned
  skip, and --dry-run skip; stale chosen values are reset with a stderr note
- Build an instruction footer advertising active keys and the armed state
- Extend ResolvedModels with save_local and dialog_shown fields (defaults
  keep existing test calls valid)
- resolve_models forwards the new keyword params to select_models and sets
  result.save_local/dialog_shown when the dialog branch runs
- run_claude computes effective_save_local (dialog result when dialog ran,
  CLI flag otherwise) and uses it in the persistence block
- Update PRD 03, README.md, and docs/commands.md to document the keybindings
- Add 17 new unit tests covering Ctrl-R and Ctrl-S behavior, including
  refresh callback invocation, stale-value reset, save_local toggle, and
  effective_save_local in run_claude

Closes #26

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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

Adds interactive keybindings to the model-selection dialog to refresh models in-place (Ctrl‑R) and toggle saving the final selection to project config (Ctrl‑S), with corresponding plumbing through resolution/persistence and documentation updates.

Changes:

  • Implement Ctrl‑R refresh and Ctrl‑S save-local toggle in select_models, including footer instructions and a structured ModelSelectionResult.
  • Thread new dialog outcomes/flags through resolve_models and compute effective_save_local in run_claude.
  • Add/expand unit tests and update user/PRD documentation for the sequential per-slot dialog design and keybindings.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/unit/selection/test_resolver.py Adds coverage for new ResolvedModels fields and verifies forwarding of keybinding params into the dialog.
tests/unit/selection/test_interactive_keybindings.py New unit tests for Ctrl‑R/Ctrl‑S behavior, refresh edge cases, and footer instruction content.
tests/unit/commands/test_claude.py Tests effective_save_local behavior (dialog-governed vs CLI-governed) and dry-run persistence behavior.
src/leitum/selection/resolver.py Extends ResolvedModels with save_local/dialog_shown and forwards refresh/save parameters into interactive selection.
src/leitum/selection/interactive.py Implements sentinels, keybinding injection, refresh handler, footer builder, and returns ModelSelectionResult.
src/leitum/commands/claude.py Wires refresh callback into resolver, computes effective_save_local, and uses it in persistence + dry-run messaging.
README.md Documents the new Ctrl‑R/Ctrl‑S shortcuts and save-local armed state indicator.
prd/03-selection-flows.md Reconciles PRD with sequential per-slot dialog reality and documents the new keybindings and persistence resolution.
docs/commands.md Adds command documentation describing the model-selection keybindings and footer format.

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

Comment thread tests/unit/selection/test_interactive_keybindings.py
Comment thread tests/unit/selection/test_interactive_keybindings.py Outdated
Comment thread tests/unit/selection/test_interactive_keybindings.py Outdated
Comment thread tests/unit/commands/test_claude.py Outdated
Comment thread tests/unit/commands/test_claude.py Outdated
Address Copilot review on PR #27: give test helper dicts explicit key/value
type parameters and fix the _run_select_models return annotation (it returns a
(result, spy) tuple, not ModelSelectionResult | None), removing the stale
type: ignore. All changed test files now pass mypy --strict individually.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rgielen

rgielen commented Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Addressed the Copilot review (commit 4104ef6).

Context: the project's actual type gate is mypy --strict src (the tests/ tree is not in the strict gate — bare mypy --strict has no target, and mypy --strict . already reports pre-existing errors in other test files on main). So none of the flagged items were breaking CI, and the new tests followed the existing loose test-file convention.

That said, the comments point at genuinely loose typing in the new tests, so I tightened them anyway:

  • test_claude.py: models: dict -> models: dict[str, str] (matches the real save_project_config signature), both occurrences.
  • test_interactive_keybindings.py:
    • _run_select_models return type corrected to tuple[ModelSelectionResult | None, MagicMock] (it returns a 2-tuple) and removed the stale # type: ignore[return-value].
    • chosen_so_far typed as the real dict[ModelSlot, str | None].
    • defaults typed as dict[str, Any] — note the literal Copilot suggestion (dict[str, object]) would break the Provider(**defaults) unpack under strict; Any keeps it strict-clean.

Both changed test files now pass mypy --strict individually. ruff format, ruff check, mypy --strict src, and pytest (178) all green.

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 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread src/leitum/selection/interactive.py
Address Copilot review on PR #27: the other keybinding tests mock questionary
away, so the actual Ctrl-R/Ctrl-S registration in _inject_key_bindings was never
executed under test — a regression could ship unnoticed. Add a focused test that
builds a real prompt_toolkit KeyBindings, runs injection, and asserts Ctrl-R is
always bound and Ctrl-S only when save_local_allowed, plus the two guard paths
(missing .application, non-KeyBindings object) are no-ops.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rgielen

rgielen commented Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Addressed the latest Copilot review (commit adf5918).

The point is legitimate: the other keybinding tests patch _inject_key_bindings away or mock questionary entirely, so the real Ctrl-R/Ctrl-S registration was never executed under test — a regression in binding injection could have shipped unnoticed while the footer still advertised the shortcuts.

Added TestInjectKeyBindings which drives the real function against a live prompt_toolkit.KeyBindings:

  • asserts Keys.ControlR is always bound and Keys.ControlS is bound only when save_local_allowed=True (via kb.get_bindings_for_keys(...));
  • asserts Keys.ControlS is absent when save_local_allowed=False;
  • covers both guard paths (missing .application, non-KeyBindings object) as safe no-ops.

Suite: 178 -> 182 tests. ruff format, ruff check, mypy --strict src, and pytest all green.

@rgielen
rgielen merged commit f759f25 into main Jul 4, 2026
6 checks passed
@rgielen
rgielen deleted the feat/issue-26-model-keybindings branch July 4, 2026 16:35
rgielen added a commit that referenced this pull request Jul 4, 2026
Address Copilot review on PR #27: give test helper dicts explicit key/value
type parameters and fix the _run_select_models return annotation (it returns a
(result, spy) tuple, not ModelSelectionResult | None), removing the stale
type: ignore. All changed test files now pass mypy --strict individually.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

feat: model-selection keybindings — Ctrl-R (refresh) and Ctrl-S (save-local)

2 participants