diff --git a/CHANGELOG.md b/CHANGELOG.md index 57f2cd1..2b49d93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Keybindings are configurable: `keys:` in the config remaps any snake_case + binding name with a single key or a list, and the footer and `?` help show the + result. `force_quit` (`ctrl+c`) stays fixed so there is always a way out. + Invalid definitions stop startup with an explanatory error; the conservative + collision rule rejects a new claimant when the key remains shared, while still + allowing clean moves and swaps (#82). - Themes: `theme: dracula` (or `nord`) picks a built-in palette, and `themes:` in the config defines your own, naming only the tokens you want to change. An invalid color stops startup with a message naming the theme, the token and diff --git a/README.it.md b/README.it.md index ba7cc9b..6cc324a 100644 --- a/README.it.md +++ b/README.it.md @@ -506,6 +506,33 @@ billing: accent: {light: "127", dark: "205"} muted: "240" ``` +- `keys` (opzionale): rimappa per nome qualsiasi binding della TUI. Il valore può + essere un tasto o una lista di tasti; ogni binding che non nomini mantiene il + proprio default. I nomi usano lo snake_case, come `quit`, `log_hours`, + `prev_month` e `palette_up`. Se sbagli un nome, l'errore all'avvio elenca quelli + validi. + + ```yaml + keys: + log_hours: "L" + up: ["up", "ctrl+u"] + ``` + + - **`force_quit` non può essere rimappato.** `ctrl+c` resta una via d'uscita + affidabile se sposti altri binding su tasti irraggiungibili. + - I binding predefiniti condividono deliberatamente alcuni tasti quando sono + attivi su schermate diverse. Il controllo conservativo delle collisioni + rifiuta un override solo quando aggiunge un nuovo binding a un tasto che, dopo + tutti gli override, ha ancora almeno due binding associati. Puoi sempre + spostare un binding da un tasto condiviso, spostarlo su un tasto il cui + precedente proprietario viene spostato altrove, oppure scambiare in modo pulito + due tasti. Poiché il controllo non modella quali binding condividono una + schermata, scegli un altro tasto se rifiuta una combinazione che sai non sarebbe + mai attiva insieme. + - Nomi sconosciuti, tasti vuoti o duplicati, liste vuote e collisioni bloccano + l'avvio con un errore esplicativo, senza ricadere silenziosamente sui default. + - I tasti rimappati compaiono nel footer, nell'aiuto esteso di `?` e nelle + indicazioni contestuali sui tasti. ### Come vengono calcolati gli importi fatturati diff --git a/README.md b/README.md index fe718db..760d5c9 100644 --- a/README.md +++ b/README.md @@ -475,6 +475,30 @@ billing: accent: {light: "127", dark: "205"} muted: "240" ``` +- `keys` (optional): remap any TUI binding by name. A value is one key or a list + of keys; every binding you do not name keeps its default. Names use snake_case, + such as `quit`, `log_hours`, `prev_month` and `palette_up`. If a name is wrong, + the startup error lists the valid ones. + + ```yaml + keys: + log_hours: "L" + up: ["up", "ctrl+u"] + ``` + + - **`force_quit` cannot be remapped.** `ctrl+c` remains a reliable way out if + other bindings are moved somewhere unreachable. + - The built-in bindings deliberately share keys when they are active on + different screens. The conservative collision check rejects an override only + when it adds a new claimant to a key that still has at least two claimants + after all overrides. Moving a binding off a shared key, moving to a key whose + previous owner also moves away, and clean swaps are allowed. Because the check + does not model which bindings share a screen, choose another key if it rejects + a combination you know would not be active together. + - Unknown names, empty or duplicate keys, empty lists and collisions stop + startup with an explanatory error instead of silently falling back. + - Remapped keys appear in the footer, the expanded `?` help and inline key + guidance. ### How billed amounts are computed diff --git a/docs/superpowers/plans/2026-08-01-accessibility-base.md b/docs/superpowers/plans/2026-08-01-accessibility-base.md index f320364..1afdca6 100644 --- a/docs/superpowers/plans/2026-08-01-accessibility-base.md +++ b/docs/superpowers/plans/2026-08-01-accessibility-base.md @@ -192,10 +192,9 @@ git commit -m "feat(config): add the optional mouse key (#74)" **Contesto che il brief non può sapere.** -1. `keyMsgFor` (`internal/tui/actions.go:149`) esiste già e costruisce `KeyMsg` - sintetici, ma **non va usata né allargata**: il suo commento dichiara - l'insieme chiuso di proposito, per la command palette. `wheelKey` è una - funzione separata con la propria motivazione. +1. `parseKeyName` (`internal/tui/actions.go`) esiste già e ricostruisce `KeyMsg` + da nomi configurabili, ma qui non serve: la rotella ha due output interni + fissi e `wheelKey` li esprime direttamente in una funzione separata. 2. Le fixture dei test esistono già nel package e vanno riusate, non riscritte: `filtersScreenFixture()` (`filters_test.go:18`), `newTestModel()` (`log_test.go:31`), `newTestModelOnReport()` (`log_test.go:36`), @@ -362,9 +361,9 @@ import tea "github.com/charmbracelet/bubbletea" // and the tests that cover it — apply unchanged. actions.go does the same for // the command palette. // -// It deliberately does not go through keyMsgFor: that function's key set is -// closed on purpose for the palette's benefit (see its doc comment), and -// widening it for a different caller would betray that. +// It deliberately does not go through parseKeyName: wheel translation has two +// fixed internal outputs rather than a user-written name to validate, so typed +// KeyMsgs state that invariant directly. // // There is no check on msg.Action, and it would be dead code: measured against // bubbletea v1.3.10, a wheel notch always arrives as a single press. The SGR diff --git a/docs/superpowers/plans/2026-08-01-configurable-keys.md b/docs/superpowers/plans/2026-08-01-configurable-keys.md new file mode 100644 index 0000000..dcd1179 --- /dev/null +++ b/docs/superpowers/plans/2026-08-01-configurable-keys.md @@ -0,0 +1,1493 @@ +# Keybinding configurabili — piano di implementazione + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development +> (recommended) or superpowers:executing-plans to implement this plan task-by-task. +> Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** chiudere la **#82** — la sua terza casella, gli override delle +keybinding dal config. Con questa la milestone v1.9 si chiude. + +**Architecture:** i nomi delle chiavi YAML si derivano per reflection dai campi +di `keyDefaults`, quindi ogni binding tranne `force_quit` è rimappabile e nessuno +futuro richiede lavoro. `internal/config` tiene il grezzo, `internal/tui` risolve +e valida accanto alla tabella che valida, `internal/cli` fa fallire l'avvio su +una configurazione che non può onorare. + +**Tech Stack:** Go 1.26, bubbles/key v1.0.0, bubbletea v1.3.10, yaml.v3. + +**Spec:** `docs/superpowers/specs/2026-08-01-configurable-keys-design.md` + +## Global Constraints + +- Tutto ciò che vive nel repo è in **inglese**: codice, identificatori, + commenti, stringhe UI, nomi e messaggi dei test, messaggi di commit. Eccezioni: + `README.it.md`, `CONTRIBUTING.it.md` e i doc di design sotto + `docs/superpowers/`. +- **MAI** un trailer `Co-Authored-By`. Conventional Commits. +- `internal/report` e `internal/duration` restano **puri**: solo stdlib, nessun + I/O, nessun import di `internal/config`, `internal/clickup`, `internal/tui`, + `internal/themes`. +- `internal/themes` resta **foglia**: solo lipgloss, termenv, yaml.v3 e stdlib. +- **Mai chiamare l'API ClickUp vera.** Non esistono credenziali in questo + ambiente. Il comportamento di rete si esercita solo con `httptest`. +- Nessuna funzione di stile in produzione chiama `lipgloss.NewStyle()`. +- **Ogni numero scritto in un commento va misurato eseguendo il codice**, mai + calcolato a mente. +- **Un test scritto contro un difetto non vale finché non lo si è visto fallire + contro quel difetto**, con il transcript allegato al report. +- I golden si rigenerano solo con `go test ./internal/tui -update`, mai a mano. + **In questa tranche nessun golden deve muoversi**: nessun default cambia. +- Gate prima di ogni commit, tutti e cinque, tutti puliti: + `gofmt -l .` · `go vet ./...` · + `go run honnef.co/go/tools/cmd/staticcheck@latest ./...` · `go build ./...` · + `go test ./... -race` + +--- + +### Task 1: la chiave `keys` nel config + +**Files:** +- Modify: `internal/config/config.go` +- Test: `internal/config/config_test.go` + +**Interfaces:** +- Consuma: niente. +- Produce: `config.KeySpec` e `Config.Keys map[string]KeySpec`. Il Task 3 li + legge. + +- [ ] **Step 1: scrivere i test che falliscono** + +In fondo a `internal/config/config_test.go`: + +```go +// --- optional keys map (#82) --- + +func TestKeysRoundTrip(t *testing.T) { + isolateConfig(t) + in := Config{Token: "t", WorkspaceID: "1", Keys: map[string]KeySpec{ + "log_hours": {"L"}, + "up": {"up", "ctrl+u"}, + }} + if err := Save(in); err != nil { + t.Fatal(err) + } + got, err := Load() + if err != nil { + t.Fatal(err) + } + if len(got.Keys["log_hours"]) != 1 || got.Keys["log_hours"][0] != "L" { + t.Errorf("log_hours = %v, want [L]", got.Keys["log_hours"]) + } + if len(got.Keys["up"]) != 2 || got.Keys["up"][0] != "up" || got.Keys["up"][1] != "ctrl+u" { + t.Errorf("up = %v, want [up ctrl+u]", got.Keys["up"]) + } +} + +func TestKeySpecAcceptsAScalarAndAList(t *testing.T) { + t.Parallel() + var got map[string]KeySpec + src := "log_hours: \"L\"\nup: [up, ctrl+u]\n" + if err := yaml.Unmarshal([]byte(src), &got); err != nil { + t.Fatal(err) + } + if len(got["log_hours"]) != 1 || got["log_hours"][0] != "L" { + t.Errorf("scalar form gave %v, want [L]", got["log_hours"]) + } + if len(got["up"]) != 2 || got["up"][0] != "up" || got["up"][1] != "ctrl+u" { + t.Errorf("list form gave %v, want [up ctrl+u]", got["up"]) + } +} + +// Save serializes the whole Config, so without MarshalYAML a single key written +// as a scalar would come back as a one-element list the first time anything +// saves the config. This test is the only thing that would notice. +func TestSaveKeepsASingleKeyScalar(t *testing.T) { + isolateConfig(t) + if err := Save(Config{Token: "t", WorkspaceID: "1", Keys: map[string]KeySpec{ + "log_hours": {"L"}, + }}); err != nil { + t.Fatal(err) + } + p, err := Path() + if err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), `log_hours: L`) { + t.Fatalf("saved config did not keep the scalar form:\n%s", raw) + } +} + +func TestKeysAbsentIsNotWritten(t *testing.T) { + isolateConfig(t) + if err := Save(Config{Token: "t", WorkspaceID: "1"}); err != nil { + t.Fatal(err) + } + p, err := Path() + if err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "keys:") { + t.Fatalf("saved config mentions keys:\n%s", raw) + } +} +``` + +`config_test.go` importa già `os` e `strings`; aggiungere `gopkg.in/yaml.v3`. + +- [ ] **Step 2: eseguirli e vederli fallire** + +Run: `go test ./internal/config -run 'TestKey|TestSaveKeepsASingle' -v` +Expected: FAIL in compilazione — `undefined: KeySpec`. + +- [ ] **Step 3: implementare** + +In `internal/config/config.go`, accanto agli altri tipi: + +```go +// KeySpec is one binding's keys as written in YAML: a bare key, or a list of +// them. It lives here rather than in internal/tui because nothing outside this +// package needs the type — internal/tui takes plain strings. +type KeySpec []string + +// UnmarshalYAML accepts both forms, so the common case is `log_hours: L` +// rather than `log_hours: [L]`. +func (k *KeySpec) UnmarshalYAML(n *yaml.Node) error { + if n.Kind == yaml.ScalarNode { + var s string + if err := n.Decode(&s); err != nil { + return err + } + *k = KeySpec{s} + return nil + } + var list []string + if err := n.Decode(&list); err != nil { + return err + } + *k = list + return nil +} + +// MarshalYAML collapses a single key back to a scalar. Without it, Save — which +// marshals the whole Config — would rewrite a user's `log_hours: L` as a +// one-element list the first time anything saved the config. +func (k KeySpec) MarshalYAML() (any, error) { + if len(k) == 1 { + return k[0], nil + } + return []string(k), nil +} +``` + +e dentro `Config`, sotto `Themes`: + +```go + // Keys remaps the TUI's bindings by name. The names are the ones + // internal/tui derives from its binding table; this package holds the YAML + // as written and validates nothing. + Keys map[string]KeySpec `yaml:"keys,omitempty"` +``` + +- [ ] **Step 4: eseguirli e vederli passare** + +Run: `go test ./internal/config -run 'TestKey|TestSaveKeepsASingle' -v` +Expected: PASS (4 test). + +- [ ] **Step 5: prova per mutazione, obbligatoria** + +Cancellare `MarshalYAML`. `TestSaveKeepsASingleKeyScalar` **deve** fallire, e il +suo output deve mostrare la forma a lista. Ripristinare e verificare con +`git diff`. + +- [ ] **Step 6: gate + commit** + +```bash +gofmt -l . && go vet ./... && go run honnef.co/go/tools/cmd/staticcheck@latest ./... && go build ./... && go test ./... -race +git add internal/config/ +git commit -m "feat(config): add the keys map for binding overrides (#82)" +``` + +--- + +### Task 2: i nomi dei binding, derivati e inchiodati + +**Files:** +- Create: `internal/tui/keyname.go` +- Create: `internal/tui/keyname_test.go` + +**Interfaces:** +- Consuma: `keyDefaults` (già esistente, `keys.go:14`). +- Produce: `bindingName(field string) string` e `BindingNames() []string`. I + Task 3 e 4 li usano. + +- [ ] **Step 1: scrivere i test che falliscono** + +`internal/tui/keyname_test.go`: + +```go +package tui + +import ( + "slices" + "testing" +) + +func TestBindingName(t *testing.T) { + t.Parallel() + for _, tc := range []struct{ in, want string }{ + {"Quit", "quit"}, + {"LogHours", "log_hours"}, + {"PaletteUp", "palette_up"}, + {"ForceQuit", "force_quit"}, + // An acronym run stays together. The naive rule — underscore before + // every capital — turns this into "pick_by_i_d", which would have + // shipped as a config key. + {"PickByID", "pick_by_id"}, + {"ID", "id"}, + {"HTTPServer", "http_server"}, + } { + if got := bindingName(tc.in); got != tc.want { + t.Errorf("bindingName(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +// This is the test that matters most in this task. The config keys a user +// writes are derived from Go field names, so renaming a field silently renames +// a key in every config file in the wild. Spelling the list out here means the +// rename fails a test instead of failing a user. +func TestBindingNamesArePinned(t *testing.T) { + t.Parallel() + want := []string{ + "back", "browse_list", "budget", "change_range", "clear_value", + "confirm", "confirm_delete", "delete", "down", "edit", + "export", "filters", "force_quit", "generate", "group_by", + "help", "history", "list_budget", "list_currency", "log_hours", + "members", "new_override", "new_tag", "next_field", "next_month", + "next_section", "no", "open_entries", "palette", "palette_down", + "palette_up", "pick_by_id", "pick_guided", "pick_timer", "prev_field", + "prev_month", "prev_section", "quit", "range", "rates", + "reload", "save", "select_all", "stop_timer", "tags", + "timer", "toggle_item", "toggle_scope", "toggle_week", "up", + "yes", + } + got := BindingNames() + if !slices.Equal(got, want) { + t.Errorf("BindingNames() = %v\nwant %v", got, want) + } +} + +func TestBindingNamesAreUnique(t *testing.T) { + t.Parallel() + seen := map[string]bool{} + for _, n := range BindingNames() { + if seen[n] { + t.Errorf("duplicate derived name %q — two fields collapse to the same config key", n) + } + seen[n] = true + } +} +``` + +- [ ] **Step 2: eseguirli e vederli fallire** + +Run: `go test ./internal/tui -run TestBindingName -v` +Expected: FAIL in compilazione — `undefined: bindingName`, `undefined: BindingNames`. + +- [ ] **Step 3: implementare** + +`internal/tui/keyname.go`: + +```go +package tui + +import ( + "reflect" + "slices" + "strings" + "unicode" +) + +// bindingName turns a keyDefaults field name into the key a user writes in +// their config: LogHours -> log_hours. +// +// An acronym run stays together — PickByID becomes pick_by_id, not +// pick_by_i_d — because the underscore goes in only where a case boundary is a +// word boundary: after a lowercase letter, or before the last capital of a run. +// Measured against every field in keyDefaults; the naive rule produced +// "pick_by_i_d", which would have shipped as a config key. +func bindingName(field string) string { + r := []rune(field) + var b strings.Builder + for i, c := range r { + if !unicode.IsUpper(c) { + b.WriteRune(c) + continue + } + prevLower := i > 0 && unicode.IsLower(r[i-1]) + nextLower := i+1 < len(r) && unicode.IsLower(r[i+1]) + if i > 0 && (prevLower || nextLower) { + b.WriteByte('_') + } + b.WriteRune(unicode.ToLower(c)) + } + return b.String() +} + +// BindingNames lists every binding name, sorted, for validation errors and for +// the docs. It includes force_quit so the pinned list covers every field, even +// though ResolveKeys rejects that one explicitly. +// +// Derived rather than written out so that a binding added to keyDefaults is +// remappable with no further work unless it is explicitly fixed like force_quit +// — the maintenance multiplier #82 warned about does not exist. The cost is +// moved instead: renaming a Go field would rename a user's config key, which is +// what TestBindingNamesArePinned exists to catch. +func BindingNames() []string { + ty := reflect.TypeOf(keyDefaults{}) + out := make([]string, 0, ty.NumField()) + for i := range ty.NumField() { + out = append(out, bindingName(ty.Field(i).Name)) + } + slices.Sort(out) + return out +} +``` + +- [ ] **Step 4: eseguirli e vederli passare** + +Run: `go test ./internal/tui -run TestBindingName -v` +Expected: PASS (3 test). + +- [ ] **Step 5: prova per mutazione, obbligatoria** + +Due mutazioni, con transcript: + +1. In `bindingName`, togliere la condizione `nextLower` (cioè tornare alla + regola ingenua). `TestBindingName` **deve** fallire su `PickByID` e + `HTTPServer`, e `TestBindingNamesArePinned` su `pick_by_id`. +2. Rinominare il campo `Timer` di `keyDefaults` in `TimerToggle`. + `TestBindingNamesArePinned` **deve** fallire. È la prova che il test protegge + davvero i config degli utenti da un rename. + +Ripristinare dopo ognuna e verificare con `git diff`. + +- [ ] **Step 6: gate + commit** + +```bash +gofmt -l . && go vet ./... && go run honnef.co/go/tools/cmd/staticcheck@latest ./... && go build ./... && go test ./... -race +git add internal/tui/keyname.go internal/tui/keyname_test.go +git commit -m "feat(tui): derive the config name of every binding (#82)" +``` + +--- + +### Task 3: `KeyTable` e `ResolveKeys` + +**Files:** +- Create: `internal/tui/keytable.go` +- Create: `internal/tui/keytable_test.go` +- Modify: `internal/tui/actions.go` (`canonicalKeyTypes`, `keyNameAliases`, + `parseKeyName`) +- Test: `internal/tui/actions_test.go` + +**Interfaces:** +- Consuma: `bindingName`, `BindingNames` dal Task 2; `config.KeySpec` dal Task 1. +- Produce: `KeyTable`, `(KeyTable).bindings()`, `DefaultKeyTable()`, + `ResolveKeys(map[string]config.KeySpec) (KeyTable, error)`, + `parseKeyName(string) (tea.KeyMsg, error)` e `keyLabel([]string) string`. Il + Task 4 aggiunge il controllo sulle collisioni dentro `ResolveKeys`; il Task 5 + riusa lo stesso parser per la command palette. + +**Contesto che il brief non può sapere.** La `KeyTable` zero **deve** valere +come i default: i test costruiscono 108 letterali `Model{…}` senza passare da +`New`, misurati, e leggere una tabella zero li disabiliterebbe tutti. + +- [ ] **Step 1: scrivere i test che falliscono** + +`internal/tui/keytable_test.go`: + +```go +package tui + +import ( + "fmt" + "strings" + "testing" + + "github.com/marcoarnulfo/clickup-cli/internal/config" +) + +// The zero KeyTable is what a Model built by hand in a test carries, and 108 +// such Models exist. It must behave as the built-in defaults. +func TestZeroKeyTableIsTheDefaults(t *testing.T) { + t.Parallel() + var zero KeyTable + if got, want := zero.bindings().Quit.Keys(), defaultKeys().Quit.Keys(); len(got) != len(want) || got[0] != want[0] { + t.Errorf("zero table Quit = %v, want the default %v", got, want) + } +} + +func TestResolveKeysOverridesAndKeepsTheRest(t *testing.T) { + t.Parallel() + kt, err := ResolveKeys(map[string]config.KeySpec{"log_hours": {"L"}}) + if err != nil { + t.Fatal(err) + } + d := kt.bindings() + if got := d.LogHours.Keys(); len(got) != 1 || got[0] != "L" { + t.Errorf("LogHours = %v, want [L]", got) + } + if got, want := d.Quit.Keys(), defaultKeys().Quit.Keys(); got[0] != want[0] { + t.Errorf("Quit = %v, want the untouched default %v", got, want) + } +} + +// The help string carries the key inside it, so an overridden binding that kept +// its old help would lie in the footer and in the ? overlay. +// The keys are chosen so the override stays collision-free once Task 4's rule +// lands: "k" is already Up's own, and "ctrl+u" is claimed by nothing. An +// earlier draft used "w", which toggle_week claims — Task 4 would then have +// rejected it and this test could never have passed. +func TestResolveKeysRegeneratesTheHelp(t *testing.T) { + t.Parallel() + kt, err := ResolveKeys(map[string]config.KeySpec{"up": {"k", "ctrl+u"}}) + if err != nil { + t.Fatal(err) + } + h := kt.bindings().Up.Help() + if h.Key != "k/ctrl+u" { + t.Errorf("help key = %q, want %q", h.Key, "k/ctrl+u") + } + if want := defaultKeys().Up.Help().Desc; h.Desc != want { + t.Errorf("help desc = %q, want the original %q", h.Desc, want) + } +} + +func TestResolveKeysErrors(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + in map[string]config.KeySpec + says []string + }{ + { + name: "unknown binding lists the valid names", + in: map[string]config.KeySpec{"log_hourz": {"L"}}, + says: []string{"log_hourz", "log_hours"}, + }, + { + name: "force_quit is not remappable", + in: map[string]config.KeySpec{"force_quit": {"ctrl+x"}}, + says: []string{"force_quit", "ctrl+c"}, + }, + { + name: "an empty list is rejected", + in: map[string]config.KeySpec{"quit": {}}, + says: []string{"quit", "at least one key"}, + }, + { + name: "an empty key inside the list is rejected", + in: map[string]config.KeySpec{"quit": {"Q", ""}}, + says: []string{"quit", "empty"}, + }, + { + // Without its own check this reaches the collision rule, which + // reports "already claimed by " with an empty list — the only + // other claimant being the binding itself. + name: "the same key twice in one list is rejected", + in: map[string]config.KeySpec{"quit": {"Q", "Q"}}, + says: []string{"quit", "\"Q\"", "twice"}, + }, + } { + _, err := ResolveKeys(tc.in) + if err == nil { + t.Errorf("%s: ResolveKeys = nil error, want one", tc.name) + continue + } + for _, s := range tc.says { + if !strings.Contains(err.Error(), s) { + t.Errorf("%s: error %q does not mention %q", tc.name, err, s) + } + } + } +} + +// ctrl+c must survive a config that remaps everything else: it is the only way +// out of a TUI whose Quit the user has moved somewhere they cannot reach. +// +// Each binding gets a distinct private-use Unicode rune so the sweep is both +// canonically producible as one KeyMsg and collision-free. A sweep built from, +// say, "ctrl+"+name[:1] would collide (back, budget and browse_list would all +// want ctrl+b) and this test would quietly turn into one that skips forever. +func TestForceQuitSurvivesEveryOverride(t *testing.T) { + t.Parallel() + over := map[string]config.KeySpec{} + for i, n := range BindingNames() { + if n == forceQuitName { + continue + } + over[n] = config.KeySpec{string(rune(0xE000 + i))} + } + kt, err := ResolveKeys(over) + if err != nil { + t.Fatalf("a collision-free sweep was rejected: %v", err) + } + if got, want := kt.bindings().ForceQuit.Keys(), defaultKeys().ForceQuit.Keys(); got[0] != want[0] { + t.Errorf("ForceQuit = %v, want %v", got, want) + } + // And the sweep really did land, so the assertion above is not vacuous. + if got := kt.bindings().Quit.Keys(); len(got) != 1 || got[0] == "q" { + t.Errorf("Quit = %v, want the swept key — the sweep did not take effect", got) + } +} +``` + +In `internal/tui/actions_test.go`, aggiungere tre test table-driven: + +- `TestParseKeyNameAcceptsCanonicalNames`: rune singole ASCII/Unicode, spazio + letterale, `alt+ `, control key canoniche e relative forme `alt+`; ogni + `tea.KeyMsg` deve fare round-trip esatto con `String()`; +- `TestParseKeyNameAcceptsEveryCanonicalSpecialKey`: tutte le special key + esportate da Bubble Tea che rappresentano un singolo input terminale — frecce, + home/end, page, insert/delete, modificatori e `f1`..`f20` — sia normali sia + `alt+`; +- `TestParseKeyNameRejectsAliasesAndUnreachableNames`: stringa vuota, sequenze + multi-runa (`g g`, `gg`), nomi inesistenti (`f21`, `runes`, `alt+`) e alias + non canonici. Gli alias devono suggerire la forma di `KeyMsg.String()`: + `space` → spazio letterale, control-backtick → `ctrl+@`, `ctrl+i` → `tab`, + `ctrl+m` → `enter`, `ctrl+[` → `esc`, `ctrl+?` → `backspace`, inclusa la + forma `alt+`. + +In `internal/tui/keytable_test.go`, aggiungere +`TestResolveKeysRejectsUnreachableKeyNamesAtStartup`: `ResolveKeys` deve +rifiutare sequenze, nomi fuori insieme e alias, nominando binding, valore e, +quando esiste, spelling canonico. Iniziare inoltre +`TestLiteralSpaceOverrideRoutesAndRendersAsSpace` con le asserzioni disponibili +in questo task: il tasto grezzo resta `" "`, mentre help e label mostrano +`space`; `alt+ ` mostra `alt+space`. Il Task 5 completa lo stesso test con routing +e palette. + +- [ ] **Step 2: eseguirli e vederli fallire** + +Run: `go test ./internal/tui -run 'TestZeroKeyTable|TestResolveKeys|TestForceQuit|TestParseKeyName|TestLiteralSpace|TestAltSpace' -v` +Expected: FAIL in compilazione — `undefined: KeyTable`, `undefined: ResolveKeys`, +`undefined: parseKeyName` e `undefined: keyLabel`. + +- [ ] **Step 3: implementare** + +`internal/tui/keytable.go`: + +```go +package tui + +import ( + "fmt" + "maps" + "reflect" + "slices" + "strings" + + "github.com/charmbracelet/bubbles/key" + "github.com/marcoarnulfo/clickup-cli/internal/config" +) + +// forceQuitName is the one binding a config may not remap: ctrl+c is the way +// out of a TUI whose other keys the user has moved somewhere unreachable. +const forceQuitName = "force_quit" + +// KeyTable is the resolved binding table the TUI routes and renders with. +// +// The zero value means the built-in defaults, which is what a Model built by +// hand carries — the tests construct 108 of those without going through New, +// and a zero table read literally would leave every binding disabled. +type KeyTable struct { + d keyDefaults + set bool +} + +// DefaultKeyTable is the built-in table, for callers that want it explicitly. +func DefaultKeyTable() KeyTable { return KeyTable{d: defaultKeys(), set: true} } + +func (kt KeyTable) bindings() keyDefaults { + if !kt.set { + return defaultKeys() + } + return kt.d +} + +// keyLabel renders the canonical literal-space names visibly without changing +// the raw strings used by key.Matches and collision claims. +func keyLabel(keys []string) string { + labels := slices.Clone(keys) + for i, k := range labels { + switch k { + case " ": + labels[i] = "space" + case "alt+ ": + labels[i] = "alt+space" + } + } + return strings.Join(labels, "/") +} + +// ResolveKeys applies a config's overrides to the built-in table. +// +// Every failure is an error rather than a fallback: a key that cannot be +// honored would leave the user in front of a TUI where a command simply does +// not answer, with nothing to go on. Same rule as billing.rounding.increment +// and as the theme resolution. +func ResolveKeys(overrides map[string]config.KeySpec) (KeyTable, error) { + d := defaultKeys() + v := reflect.ValueOf(&d).Elem() + ty := v.Type() + + index := map[string]int{} + for i := range ty.NumField() { + index[bindingName(ty.Field(i).Name)] = i + } + + for _, name := range slices.Sorted(maps.Keys(overrides)) { + if name == forceQuitName { + return KeyTable{}, fmt.Errorf( + "binding %q cannot be remapped: ctrl+c is the way out of a TUI whose other keys have moved", name) + } + i, ok := index[name] + if !ok { + // force_quit is filtered out of the suggestion list: offering the + // one name that is then rejected would be a message that argues + // with itself. + valid := slices.DeleteFunc(BindingNames(), func(s string) bool { return s == forceQuitName }) + return KeyTable{}, fmt.Errorf("unknown binding %q; valid names: %s", + name, strings.Join(valid, ", ")) + } + ks := overrides[name] + if len(ks) == 0 { + return KeyTable{}, fmt.Errorf("binding %q needs at least one key", name) + } + seen := map[string]bool{} + for _, k := range ks { + if k == "" { + return KeyTable{}, fmt.Errorf("binding %q has an empty key in its list", name) + } + if _, err := parseKeyName(k); err != nil { + return KeyTable{}, fmt.Errorf("binding %q uses invalid key %q: %w", name, k, err) + } + // Caught here rather than by the collision rule, which would report + // the binding colliding with itself and name no other claimant. + if seen[k] { + return KeyTable{}, fmt.Errorf("binding %q lists key %q twice", name, k) + } + seen[k] = true + } + old := v.Field(i).Interface().(key.Binding) + // The help string carries the key inside it, so it has to be rebuilt or + // the footer would advertise a key that no longer does anything. The + // typographic arrows the defaults use (↑/k, tab/▸) are lost for + // remapped bindings: the user chose these keys, and inventing a + // prettier rendering for them would be guessing. + v.Field(i).Set(reflect.ValueOf(key.NewBinding( + key.WithKeys(ks...), + key.WithHelp(keyLabel(ks), old.Help().Desc), + ))) + } + return KeyTable{d: d, set: true}, nil +} +``` + +In `internal/tui/actions.go`, implementare un solo parser canonico: + +- `canonicalKeyTypes` enumera le `tea.KeyType` che Bubble Tea può produrre come + singolo input, senza affidarsi a range numerici non contigui; +- `parseKeyName` separa l'eventuale prefisso `alt+`, rifiuta gli alias sopra con + il suggerimento canonico, prova le special key e poi una sola runa stampabile. + Per `tea.KeySpace` imposta anche `Runes: []rune{' '}`, come fa il parser del + terminale; +- accetta un nome solo se il `tea.KeyMsg` ricostruito restituisce **esattamente** + quel nome da `String()`. Non normalizza e non accetta sequenze che nessun + singolo `KeyMsg` può produrre. + +`ResolveKeys` chiama il parser prima di costruire il binding, così una chiave +irraggiungibile ferma l'avvio invece di diventare un comando morto. Aggiungere +`"maps"` al blocco import di `keytable.go`. + +- [ ] **Step 4: eseguirli e vederli passare** + +Run: `go test ./internal/tui -run 'TestZeroKeyTable|TestResolveKeys|TestForceQuit|TestParseKeyName|TestLiteralSpace|TestAltSpace' -v` +Expected: PASS per tutti i test e sottotest selezionati. + +- [ ] **Step 5: prova per mutazione, obbligatoria** + +Sei mutazioni, con transcript: + +1. Togliere la rigenerazione dell'aiuto (passare `old.Help().Key` invece di + `keyLabel(ks)`). `TestResolveKeysRegeneratesTheHelp` **deve** + fallire. +2. Togliere il controllo su `forceQuitName`. Il caso «force_quit is not + remappable» **deve** fallire. +3. Far ritornare `KeyTable{d: d}` senza `set: true`. + `TestResolveKeysOverridesAndKeepsTheRest` **deve** fallire, perché la tabella + ricadrebbe sui default e `LogHours` tornerebbe `n`. +4. Permettere più di una runa nel fallback di `parseKeyName`. + `TestParseKeyNameRejectsAliasesAndUnreachableNames` **deve** fallire su `gg` + e `g g`. +5. Togliere `keyNameAliases` e lasciare che gli alias passino o cadano nell'errore + generico. I sottotest su `space`, `ctrl+i`, `ctrl+m`, `ctrl+[` e `ctrl+?` + **devono** fallire perché manca il rifiuto con suggerimento canonico. +6. In `keyLabel`, ritornare `strings.Join(keys, "/")` senza rendere gli spazi. + I test su spazio e alt-spazio **devono** fallire mostrando label invisibili. + +Ripristinare dopo ognuna e verificare con `git diff`. + +- [ ] **Step 6: gate + commit** + +```bash +gofmt -l . && go vet ./... && go run honnef.co/go/tools/cmd/staticcheck@latest ./... && go build ./... && go test ./... -race +git add internal/tui/keytable.go internal/tui/keytable_test.go internal/tui/actions.go internal/tui/actions_test.go +git commit -m "feat(tui): resolve binding overrides from the config (#82)" +``` + +--- + +### Task 4: la regola sulle collisioni + +**Files:** +- Modify: `internal/tui/keytable.go` (`ResolveKeys`, più `claims` e `checkCollisions`) +- Modify: `internal/tui/keytable_test.go` + +**Interfaces:** +- Consuma: `bindingName`, `keyDefaults`. +- Produce: nessuna API nuova esportata; `ResolveKeys` guadagna un controllo. + +**Contesto che il brief non può sapere, e che è il cuore del task.** I default +contengono **20 tasti rivendicati da più di un binding**, misurati — `n` da +quattro binding, `enter` da tre, `h` da tre. Convivono perché `screenKeys` ne +abilita solo un sottoinsieme per schermata. Una regola «due binding non possono +condividere un tasto» rifiuterebbe quindi i default stessi: **non implementarla**. + +La regola è: per ogni tasto rivendicato da **due o più binding dopo gli +override**, l'insieme dei binding che lo rivendicano dopo deve essere un +sottoinsieme di quello che lo rivendicava prima. Un tasto con un solo +rivendicante dopo gli override non è soggetto al controllo. + +- [ ] **Step 1: scrivere i test che falliscono** + +In `internal/tui/keytable_test.go`: + +```go +// The defaults are heavily overloaded on purpose — measured, 20 physical keys +// are claimed by more than one binding, because screenKeys enables only a +// subset per screen. A rule that forbade any sharing would reject what we +// ship, so the rule is narrower: for a key with two or more post-override +// claimants, those claimants must be a subset of the defaults' claimants. +func TestCollisionRule(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + in map[string]config.KeySpec + ok bool + says []string + }{ + { + // n is shared by log_hours, new_override, new_tag and no. Moving + // log_hours away leaves a subset behind, which is fine. + name: "moving a binding off a shared key is allowed", + in: map[string]config.KeySpec{"log_hours": {"L"}}, + ok: true, + }, + { + name: "taking a free key is allowed", + in: map[string]config.KeySpec{"export": {"ctrl+e"}}, + ok: true, + }, + { + name: "adding a claimant to a contested key is rejected", + in: map[string]config.KeySpec{"export": {"n"}}, + says: []string{"\"n\"", "export"}, + }, + { + name: "adding a claimant to a singly claimed key is rejected too", + in: map[string]config.KeySpec{"export": {"q"}}, + says: []string{"\"q\"", "export", "quit"}, + }, + { + // A clean swap IS allowed, and this case exists to keep anyone + // from "fixing" the rule into rejecting it. quit owns "q" and + // reload owns "r" outright. After trading them, each destination + // still has exactly one claimant, so the two-or-more clause never + // applies even though the claimant at each key changed. Measured + // against the real defaults. + name: "a clean swap between two bindings is allowed", + in: map[string]config.KeySpec{"quit": {"r"}, "reload": {"q"}}, + ok: true, + }, + { + // This is the declared cost from the design doc's §2.2. It is not + // about swaps: taking a key someone else still claims is rejected + // even when the two never share a screen. + // export lives on the report, list_budget on the rates screen, and + // the rule cannot know that. The user picks another key; the + // alternative is a table of every screen state, kept in sync + // forever. + name: "taking a key another binding still claims is rejected — the declared cost", + in: map[string]config.KeySpec{"export": {"g"}}, + says: []string{"export", "\"g\"", "group_by", "list_budget"}, + }, + } { + _, err := ResolveKeys(tc.in) + if tc.ok { + if err != nil { + t.Errorf("%s: ResolveKeys = %v, want nil", tc.name, err) + } + continue + } + if err == nil { + t.Errorf("%s: ResolveKeys = nil error, want one", tc.name) + continue + } + for _, s := range tc.says { + if !strings.Contains(err.Error(), s) { + t.Errorf("%s: error %q does not mention %s", tc.name, err, s) + } + } + } +} + +// The rule must never reject the table we ship. +func TestDefaultsPassTheCollisionRule(t *testing.T) { + t.Parallel() + if _, err := ResolveKeys(nil); err != nil { + t.Fatalf("the built-in defaults do not satisfy the collision rule: %v", err) + } +} +``` + +- [ ] **Step 2: eseguirli e vederli fallire** + +Run: `go test ./internal/tui -run 'TestCollisionRule|TestDefaultsPass' -v` +Expected: FAIL sui tre casi che si aspettano un errore — oggi `ResolveKeys` non +controlla niente e li accetta tutti. `TestDefaultsPassTheCollisionRule` e i tre +casi `ok: true` passano già: è giusto così, sono la guardia. + +- [ ] **Step 3: implementare** + +In `internal/tui/keytable.go`: + +```go +// claims maps every physical key to the sorted names of the bindings that want +// it. The defaults are deliberately overloaded — measured, 20 keys have more +// than one claimant — because screenKeys enables only a subset per screen. +func claims(d keyDefaults) map[string][]string { + out := map[string][]string{} + v := reflect.ValueOf(d) + ty := v.Type() + for i := range v.NumField() { + b := v.Field(i).Interface().(key.Binding) + name := bindingName(ty.Field(i).Name) + for _, k := range b.Keys() { + out[k] = append(out[k], name) + } + } + for k := range out { + slices.Sort(out[k]) + } + return out +} + +// checkCollisions rejects a post-override key with two or more claimants unless +// all of them already claimed that key in the defaults. +// +// Detecting real conflicts would mean asking, per screen, which bindings are +// enabled at once — 14 screens plus the sub-modes of entries, log, rates and +// setup, a table to keep in sync with every screen ever added. This rule is +// computed from the key table alone, and is deliberately conservative: it +// refuses taking a key another binding still claims, even when no screen would +// ever activate both. Clean swaps remain allowed; otherwise the user picks +// another key, and the message says exactly which collision they created. +func checkCollisions(before, after keyDefaults) error { + was, now := claims(before), claims(after) + for _, k := range slices.Sorted(maps.Keys(now)) { + names := now[k] + if len(names) < 2 { + continue + } + for _, n := range names { + if slices.Contains(was[k], n) { + continue + } + others := slices.DeleteFunc(slices.Clone(names), func(s string) bool { return s == n }) + return fmt.Errorf("binding %q cannot take key %q: it is already claimed by %s", + n, k, strings.Join(others, ", ")) + } + } + return nil +} +``` + +e in `ResolveKeys`, subito prima del `return` finale: + +```go + if err := checkCollisions(defaultKeys(), d); err != nil { + return KeyTable{}, err + } + return KeyTable{d: d, set: true}, nil +``` + +- [ ] **Step 4: eseguirli e vederli passare** + +Run: `go test ./internal/tui -run 'TestCollisionRule|TestDefaultsPass|TestResolveKeys|TestForceQuit' -v` +Expected: PASS. + +Poi la suite intera: `go test ./... -race`. Expected: PASS, e **nessun golden +modificato** (`git status --short internal/tui/testdata/` vuoto). + +- [ ] **Step 5: prova per mutazione, obbligatoria** + +Due mutazioni, con transcript: + +1. Sostituire il corpo di `checkCollisions` con `return nil`. I tre casi di + `TestCollisionRule` che si aspettano un errore **devono** fallire, e + `TestDefaultsPassTheCollisionRule` **no**. +2. Rendere la regola assoluta: far ritornare errore appena `len(names) >= 2`, + senza il confronto con `was[k]`. `TestDefaultsPassTheCollisionRule` **deve** + fallire — è la prova che la regola stretta rifiuterebbe i default che + spediamo. + +Ripristinare dopo ognuna e verificare con `git diff`. + +- [ ] **Step 6: gate + commit** + +```bash +gofmt -l . && go vet ./... && go run honnef.co/go/tools/cmd/staticcheck@latest ./... && go build ./... && go test ./... -race +git add internal/tui/ +git commit -m "feat(tui): enforce the conservative key collision rule (#82)" +``` + +--- + +### Task 5: la tabella arriva fino a `Update` + +**Files:** +- Modify: `internal/tui/keys.go` (`keysFor`, `screenKeys`) +- Modify: `internal/tui/app.go` (`Model`, `New`) +- Modify: `internal/tui/actions.go` (`screenActions` usa `parseKeyName`) +- Modify: `internal/cli/cli.go` (`runTUI`, più `resolveKeys`) +- Create: `internal/tui/helpers_test.go` +- Modify: i file di test che chiamano `New` (il compilatore li elenca) +- Test: `internal/tui/actions_test.go`, `internal/tui/keys_test.go`, + `internal/tui/keytable_test.go` +- Test: `internal/cli/cli_test.go` + +**Interfaces:** +- Consuma: `KeyTable`, `ResolveKeys` e `parseKeyName` dai Task 3-4; + `config.Keys` dal Task 1. +- Produce: `New(cfg config.Config, pal themes.Palette, kt KeyTable) Model`, + `Model.keys`, `testModel(cfg) Model`, `cli.resolveKeys`; la command palette + può riprodurre ogni chiave canonica accettata all'avvio. + +**Contesto che il brief non può sapere.** + +1. `defaultKeys()` ha **tre** call site in produzione: `keys.go:293` + (`keysFor` con la palette aperta), `keys.go:305` (`screenKeys`) e + `app.go:663`. I primi due leggono la tabella dal Model; **il terzo non si + tocca**, ed è voluto: `ForceQuit` non è rimappabile, quindi il controllo di + `ctrl+c` legge dai default non modificabili e nessuna configurazione può + toglierlo. +2. `New` ha **44 call site**, misurati nella tranche precedente: `app_test.go` + 33, `demo_test.go` 3, `log_test.go` 2, `report_test.go` 2, `golden_test.go` 1, + `home_test.go` 1, `palette_demo_test.go` 1, `internal/cli/cli.go` 1. + **Non cercarli con un grep letterale**: la stringa `New(cfg` ne trova nove. + Cambiare la firma e lasciare che sia `go build ./... && go vet ./...` a + elencare quelle rimaste. +3. I 43 call site dei test vanno convertiti a `testModel(cfg)`, non a + `New(cfg, themes.Default(), DefaultKeyTable())`: è la stessa quantità di + modifiche oggi e una sola la prossima volta che la firma cresce. + +- [ ] **Step 1: scrivere i test che falliscono** + +`internal/tui/helpers_test.go`: + +```go +package tui + +import ( + "github.com/marcoarnulfo/clickup-cli/internal/config" + "github.com/marcoarnulfo/clickup-cli/internal/themes" +) + +// testModel builds a Model the way production does, with the built-in palette +// and bindings. It exists so that growing New's signature touches one line +// instead of forty-three. +func testModel(cfg config.Config) Model { + return New(cfg, themes.Default(), DefaultKeyTable()) +} +``` + +In `internal/tui/keytable_test.go`, il test che chiude il giro: + +```go +// The end-to-end guard for the zero-means-defaults decision: an override has to +// survive all the way into Update's routing. If cli ever stopped passing the +// table, the zero value would quietly fall back to the defaults and only this +// test would notice. +func TestAnOverrideReachesUpdate(t *testing.T) { + kt, err := ResolveKeys(map[string]config.KeySpec{"log_hours": {"L"}}) + if err != nil { + t.Fatal(err) + } + m := New(config.Config{Token: "t", WorkspaceID: "team1"}, themes.Default(), kt) + m.screen = screenReport + m.nav = []screen{screenHome} + + if got, _ := m.Update(keyMsg("L")); got.(Model).screen != screenLog { + t.Errorf("L did not open the log screen; screen = %v", got.(Model).screen) + } + if got, _ := m.Update(keyMsg("n")); got.(Model).screen == screenLog { + t.Error("n still opens the log screen, so the override did not take effect") + } +} +``` + +In `internal/cli/cli_test.go`: + +```go +func TestResolveKeysRejectsAnUnknownBinding(t *testing.T) { + t.Parallel() + _, err := resolveKeys(config.Config{Keys: map[string]config.KeySpec{"nope": {"x"}}}) + if err == nil { + t.Fatal("resolveKeys of an unknown binding = nil error, want one") + } + if !strings.Contains(err.Error(), "nope") { + t.Errorf("error %q does not name the binding the user asked for", err) + } + if !strings.HasPrefix(err.Error(), "keys:") { + t.Errorf("error %q is not prefixed with the config section it comes from", err) + } +} +``` + +In `internal/tui/actions_test.go`: + +- `TestScreenActionsReplayAControlKeyOverride` rimappa Export sul solo `ctrl+e` + e confronta il risultato dell'azione con il `tea.KeyMsg` fisico; +- `TestScreenActionsReplaySpecialKeyOverrides` fa lo stesso per rappresentanti + delle famiglie speciali (`f1`, `home`, `delete`, `ctrl+shift+up`, `alt+f20`). + Il parser esaustivo del Task 3 e il suo riuso diretto in `screenActions` fanno + valere lo stesso contratto per tutte le special key accettate, non solo per i + rappresentanti. + +In `internal/tui/keys_test.go`, aggiungere +`TestEveryPaletteBindingIsReplayable`, che passa ogni tasto di ogni binding in +`defaultKeys().paletteDefaults()` a `parseKeyName`: anche i default devono +rispettare il contratto imposto agli override. Completare infine +`TestLiteralSpaceOverrideRoutesAndRendersAsSpace` in `keytable_test.go`: la hint +della palette e il footer mostrano `space`, e l'azione produce lo stesso routing +di `tea.KeySpace`. + +- [ ] **Step 2: eseguirli e vederli fallire** + +Run: `go test ./internal/tui -run 'TestAnOverrideReachesUpdate|TestScreenActionsReplay|TestEveryPaletteBindingIsReplayable|TestLiteralSpace' -v` +e `go test ./internal/cli -run TestResolveKeys -v`. +Expected: i test di wiring non compilano — `New` prende ancora due argomenti e +`resolveKeys` non esiste — mentre i test della palette espongono il replay +limitato del vecchio helper sulle special key. + +- [ ] **Step 3: implementare** + +In `internal/tui/keys.go`: + +```go +func keysFor(m Model) keyMap { + if m.overlay == overlayPalette { + return paletteKeys(m.keys.bindings()) + } + return screenKeys(m) +} +``` + +e in `screenKeys`, la prima riga del corpo: + +```go + d := m.keys.bindings() +``` + +In `internal/tui/app.go`, il campo nel `Model`: + +```go + // keys is the resolved binding table. The zero value means the built-in + // defaults, so a Model built by hand in a test routes normally. + keys KeyTable +``` + +e la firma: + +```go +func New(cfg config.Config, pal themes.Palette, kt KeyTable) Model { +``` + +con `keys: kt,` nel letterale `Model{…}`. **`app.go:663` non si tocca.** + +In `internal/tui/actions.go`, `screenActions` non ricostruisce più il primo tasto +come runa né mantiene un secondo elenco parziale di control/special key. Usa il +parser del Task 3: + +```go +configured := b.Keys()[0] +msg, err := parseKeyName(configured) +if err != nil { + // ResolveKeys validates overrides; TestEveryPaletteBindingIsReplayable + // validates defaults. Reaching this point is an internal invariant failure. + panic(fmt.Sprintf("invalid key %q reached the command palette: %v", configured, err)) +} +out = append(out, action{ + label: capitalize(b.Help().Desc), + hint: b.Help().Key, + run: func(m Model) (tea.Model, tea.Cmd) { return m.routeKey(msg) }, +}) +``` + +Lo stesso `tea.KeyMsg` canonico usato per validare viene così riprodotto +dall'azione: nessuna chiave accettata da `ResolveKeys` può sparire dalla palette +o perdere `Type`/`Alt` durante il replay. + +In `internal/cli/cli.go`, accanto a `resolveTheme`: + +```go +// resolveKeys builds the binding table the TUI will route with, and refuses to +// start on a configuration it cannot honor. Extracted from runTUI for the same +// reason resolveTheme and programOptions were: runTUI blocks on a terminal. +func resolveKeys(cfg config.Config) (tui.KeyTable, error) { + kt, err := tui.ResolveKeys(cfg.Keys) + if err != nil { + return tui.KeyTable{}, fmt.Errorf("keys: %w", err) + } + return kt, nil +} +``` + +e in `runTUI`: + +```go + kt, err := resolveKeys(cfg) + if err != nil { + return err + } + p := tea.NewProgram(tui.New(cfg, pal, kt), programOptions(cfg)...) +``` + +Poi far elencare al compilatore i call site rimasti e convertirli a +`testModel(cfg)`. + +- [ ] **Step 4: eseguire tutto** + +Run: `go test ./... -race` +Expected: PASS, e **nessun golden modificato**. Verificare con +`git status --short internal/tui/testdata/`: deve essere vuoto. + +- [ ] **Step 5: prova per mutazione, obbligatoria** + +Quattro mutazioni, con transcript: + +1. In `screenKeys`, rimettere `d := defaultKeys()`. + `TestAnOverrideReachesUpdate` **deve** fallire. +2. In `resolveKeys`, ignorare l'errore e ritornare `tui.KeyTable{}, nil`. + `TestResolveKeysRejectsAnUnknownBinding` **deve** fallire. +3. In `app.go`, cambiare il controllo di ForceQuit da `defaultKeys().ForceQuit` a + `m.keys.bindings().ForceQuit`. **Nessun test deve fallire** — è il caso in cui + la mutazione *non* rompe niente, e va **riportato come tale**: oggi le due + espressioni coincidono perché `force_quit` non è rimappabile, quindi la riga + è protetta dal divieto e non da un test. Dichiararlo nel report invece di + fingere una copertura che non c'è. +4. In `screenActions`, ripristinare un helper che accetta solo una runa e + `ctrl+a`..`ctrl+z`, saltando gli altri nomi. I casi speciali devono fallire + perché Export scompare dalla palette. + +Ripristinare dopo ognuna e verificare con `git diff`. + +- [ ] **Step 6: gate + commit** + +```bash +gofmt -l . && go vet ./... && go run honnef.co/go/tools/cmd/staticcheck@latest ./... && go build ./... && go test ./... -race +git add internal/tui/ internal/cli/ +git commit -m "feat(tui): route with the configured binding table (#82)" +``` + +--- + +### Task 6: il footer dice la verità + +**Files:** +- Modify: `internal/tui/keytable.go` (`KeyTable` ricorda i nomi sovrascritti, + più `label` e `setHelp`) +- Modify: `internal/tui/keys.go` (i costruttori per schermata ricevono la + `KeyTable`; i 16 `pairHelp` e gli 8 `SetHelp`) +- Modify: `internal/tui/rates.go`, `internal/tui/rates_view.go` (le due stringhe + di prosa che nominano un tasto) +- Test: `internal/tui/keytable_test.go` + +**Interfaces:** +- Consuma: `KeyTable` dai Task 3-5. +- Produce: `(KeyTable).label`, `(KeyTable).setHelp`, `(KeyTable).keysOf`. + +**Il difetto che questo task chiude, misurato.** Rigenerare l'aiuto dentro +`ResolveKeys` non basta: i costruttori per schermata riscrivono l'etichetta con +stringhe letterali in **24 punti** — 16 `pairHelp` e 8 `SetHelp` — perché una +riga di footer copre spesso una coppia di binding. Con una tabella che rimappa +`confirm→ctrl+j` e `back→ctrl+q`, il footer della palette resta +`↑/↓ move · enter run · esc close`. Il test del Task 3 guarda la `KeyTable` e non +un footer reso, quindi passa mentendo. + +**La regola**: l'etichetta letterale si usa finché **nessuno** dei binding che +copre è stato rimappato. Così il footer di default non cambia di un byte — e +**nessun golden si muove**, che è la verifica principale di questo task. + +- [ ] **Step 1: scrivere i test che falliscono** + +In `internal/tui/keytable_test.go`: + +```go +// The footer is where a remapped binding would lie: the per-screen builders +// write the key label as a literal, so regenerating it inside ResolveKeys is +// not enough on its own. +func TestFooterShowsRemappedKeys(t *testing.T) { + kt, err := ResolveKeys(map[string]config.KeySpec{"up": {"ctrl+u"}}) + if err != nil { + t.Fatal(err) + } + m := New(config.Config{Token: "t", WorkspaceID: "team1"}, themes.Default(), kt) + m.screen = screenReport + m.nav = []screen{screenHome} + m.width = 120 + + foot := m.footerView() + if !strings.Contains(foot, "ctrl+u") { + t.Errorf("footer does not advertise the remapped key:\n%s", foot) + } + if strings.Contains(foot, "↑/↓/j/k") { + t.Errorf("footer still shows the default label for a remapped binding:\n%s", foot) + } +} + +// And the other half: with nothing remapped the label is untouched, which is +// what keeps every footer golden where it is. +func TestFooterKeepsItsLabelsWhenNothingIsRemapped(t *testing.T) { + m := New(config.Config{Token: "t", WorkspaceID: "team1"}, themes.Default(), DefaultKeyTable()) + m.screen = screenReport + m.nav = []screen{screenHome} + m.width = 120 + + if foot := m.footerView(); !strings.Contains(foot, "↑/↓/j/k") { + t.Errorf("footer lost its default label:\n%s", foot) + } +} +``` + +**Il nome esatto del metodo che rende il footer va verificato prima di scrivere +il test** — leggere `internal/tui/footer.go` e usare quello vero, non +`footerView` se si chiama diversamente. + +- [ ] **Step 2: eseguirli e vederli fallire** + +Run: `go test ./internal/tui -run TestFooter -v` +Expected: `TestFooterShowsRemappedKeys` FAIL (il footer mostra ancora +`↑/↓/j/k`), `TestFooterKeepsItsLabelsWhenNothingIsRemapped` PASS — è la guardia. + +- [ ] **Step 3: implementare** + +In `internal/tui/keytable.go`, la tabella ricorda cosa è stato toccato: + +```go +type KeyTable struct { + d keyDefaults + over map[string]bool // names the config remapped + set bool +} +``` + +`ResolveKeys` popola `over[name] = true` a ogni override e lo mette nel valore +di ritorno. `DefaultKeyTable()` lo lascia nil: niente è stato rimappato. + +```go +// keysOf returns the keys currently bound to a binding, by config name. +func (kt KeyTable) keysOf(name string) []string { … } + +// label returns lit unless one of the named bindings was remapped, in which +// case it derives the label from the keys actually bound. +// +// The literal labels the screens pass carry typography the defaults earned — +// "↑/↓/j/k", "tab/⇧tab" — and deriving them unconditionally would replace that +// with something longer and uglier for every user, remapped or not, and move +// every footer golden. So the literal wins until it would be a lie. +func (kt KeyTable) label(lit string, names ...string) string { + remapped := false + var keys []string + for _, n := range names { + if kt.over[n] { + remapped = true + } + keys = append(keys, kt.keysOf(n)...) + } + if !remapped { + return lit + } + return keyLabel(keys) +} + +// setHelp is label's counterpart for the single-binding SetHelp call sites. +func (kt KeyTable) setHelp(b *key.Binding, name, lit, desc string) { + b.SetHelp(kt.label(lit, name), desc) +} +``` + +In `internal/tui/keys.go`, i costruttori per schermata ricevono la `KeyTable` +invece della sola `keyDefaults` — è una **semplificazione** di firma, non una +crescita, perché `d` si ricava dentro: + +```go +func homeKeys(m Model, kt KeyTable) keyMap { + d := kt.bindings() + … +} +``` + +e i 24 punti diventano: + +```go +pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") +kt.setHelp(&k.ClearValue, "clear_value", "d", "use the default rate") +``` + +`pairHelp` **non cambia firma**: cambia solo il terzo argomento nei suoi 16 call +site. + +In `internal/tui/rates.go:618` e `internal/tui/rates_view.go:55`, le due +stringhe che nominano un tasto vanno costruite dal binding vivo invece che dalla +lettera — per esempio interpolando `k.ListBudget.Help().Key` e +`k.BrowseList.Help().Key` al posto di `'g'` e `'b'`. + +- [ ] **Step 4: eseguire tutto** + +Run: `go test ./... -race` +Expected: PASS, e **nessun golden modificato**. Verificare con +`git status --short internal/tui/testdata/`: deve essere vuoto. Se un golden si +muove, l'etichetta letterale non sta più vincendo quando dovrebbe, ed è un +difetto: non rigenerarlo. + +- [ ] **Step 5: prova per mutazione, obbligatoria** + +Due mutazioni, con transcript: + +1. In `label`, ritornare sempre `lit`. `TestFooterShowsRemappedKeys` **deve** + fallire. +2. In `label`, ritornare sempre la forma derivata. `TestFooterKeepsItsLabelsWhenNothingIsRemapped` + **deve** fallire, **e con essa i golden dei footer** — è la prova che la + guardia protegge davvero il rendering di default. Riportare quanti golden + falliscono. + +Ripristinare dopo ognuna e verificare con `git diff`. + +- [ ] **Step 6: gate + commit** + +```bash +gofmt -l . && go vet ./... && go run honnef.co/go/tools/cmd/staticcheck@latest ./... && go build ./... && go test ./... -race +git add internal/tui/ +git commit -m "fix(tui): show remapped keys in the footer and the help (#82)" +``` + +--- + +### Task 7: documentazione + +**Files:** +- Modify: `README.md` (elenco delle chiavi di config, dopo il blocco `themes`) +- Modify: `README.it.md` (stesso punto) +- Modify: `CHANGELOG.md` (sezione `## [Unreleased]`) + +**Interfaces:** nessuna. + +- [ ] **Step 1: `README.md`** + +Dopo il punto elenco `themes`: + +```markdown +- `keys` (optional): remap any binding except `force_quit` by name. A value is one + key or a list of them, and every binding you do not name keeps its default. The + names are listed by `clup` in the error it prints when one is wrong; they are the + binding names in snake_case — `quit`, `log_hours`, `prev_month`, `palette_up`, + and so on. + + ```yaml + keys: + log_hours: "L" + up: ["up", "ctrl+u"] + ``` + + - **`force_quit` cannot be remapped.** `ctrl+c` is the way out of a TUI whose + other keys you have moved somewhere you cannot reach. + - **An override is refused when it adds a claimant to a key that still has two + or more claimants after all overrides**, and the message names the binding that + has it. The built-in bindings deliberately share keys — `n` alone serves four + of them, on screens where only one is ever active — so the rule is not "no + sharing". Moving a binding off a shared key and clean swaps are allowed. The + check cannot tell which screens two bindings share, so it errs toward refusing: + if it rejects something you believe is safe, pick another key. + - A remapped binding shows its new keys in the footer and in the `?` help. +``` + +- [ ] **Step 2: `README.it.md`** + +Lo stesso contenuto, in italiano, nella posizione corrispondente. Mantenere i due +file equivalenti nel significato. Usare il registro informale singolare che il +file usa altrove ("puoi", "tuo"), non il plurale di cortesia. + +- [ ] **Step 3: `CHANGELOG.md`** + +In `### Added` sotto `## [Unreleased]`: + +```markdown +- Keybindings are configurable: `keys:` in the config remaps any binding except + `force_quit` by name, with a single key or a list. `force_quit` (`ctrl+c`) stays + fixed so there is always a way out. An unknown name, an empty list, or adding a + claimant to a key that still has two or more claimants after all overrides stops + startup with a message naming what collided; clean moves and swaps remain allowed + (#82). +``` + +- [ ] **Step 4: rileggere quello che si è scritto** + +Aprire i tre file e **guardare** le sezioni modificate in contesto: che i punti +elenco stiano nella lista giusta, che i blocchi YAML siano annidati sotto il +proprio punto, e che la voce di changelog sia sotto `## [Unreleased]`. Riportare +cosa si è visto. + +- [ ] **Step 5: gate + commit** + +```bash +gofmt -l . && go vet ./... && go run honnef.co/go/tools/cmd/staticcheck@latest ./... && go build ./... && go test ./... -race +git add README.md README.it.md CHANGELOG.md +git commit -m "docs: document the keys map (#82)" +``` + +--- + +## Note per il controllore + +- Con questo task **la #82 si chiude**, e con lei la milestone v1.9. +- Nessun golden deve muoversi in nessun task: non cambia nessun default, e il + Task 6 tiene apposta l'etichetta letterale finché niente è rimappato. +- `TestForceQuitSurvivesEveryOverride` (Task 3) usa una sweep di rune Unicode + private-use, tutte producibili come singoli `KeyMsg` e deliberatamente prive di + collisioni. Al Task 4 va comunque **rieseguito**: se cominciasse a fallire, la + regola sta rifiutando qualcosa che non dovrebbe. +- **Il Task 6 è nato da una review, non dalla prima stesura.** La spec diceva che + rigenerare l'aiuto dentro `ResolveKeys` bastava; eseguendo, il footer + continuava a mostrare i tasti di default in 24 punti. Se un implementer trova + che l'elenco dei 24 non torna, va misurato di nuovo e riportato, non aggirato. diff --git a/docs/superpowers/specs/2026-08-01-accessibility-base-design.md b/docs/superpowers/specs/2026-08-01-accessibility-base-design.md index 8294170..f4eee0f 100644 --- a/docs/superpowers/specs/2026-08-01-accessibility-base-design.md +++ b/docs/superpowers/specs/2026-08-01-accessibility-base-design.md @@ -156,10 +156,9 @@ func wheelKey(msg tea.MouseMsg) (tea.KeyMsg, bool) `tea.KeyMsg{Type: tea.KeyDown}`, **tutto il resto** — click, motion, rotella orizzontale, pulsanti laterali — → `false`. -Non passa da `keyMsgFor`: quella funzione ha un insieme chiuso di proposito -(`actions.go:140-148`) perché serve alla palette, e allargarlo per un chiamante -diverso ne tradirebbe il commento. `wheelKey` costruisce i suoi due messaggi e si -porta dietro la propria motivazione. +Non passa da `parseKeyName`: la rotella ha due output interni fissi, non un nome +scritto dall'utente da validare. `wheelKey` costruisce direttamente i due +messaggi tipizzati e si porta dietro la propria motivazione. In `app.go`, dentro il type-switch di `Update`, un caso che ricalca l'instradamento dei tasti **meno le scorciatoie globali**: diff --git a/docs/superpowers/specs/2026-08-01-configurable-keys-design.md b/docs/superpowers/specs/2026-08-01-configurable-keys-design.md new file mode 100644 index 0000000..605b19a --- /dev/null +++ b/docs/superpowers/specs/2026-08-01-configurable-keys-design.md @@ -0,0 +1,368 @@ +# Keybinding configurabili (design) + +> v1.9. Chiude la **#82** — la sua terza e ultima casella, gli override delle +> keybinding dal config. Le altre due (tema da YAML, due temi built-in) sono +> state chiuse dalla tranche precedente. Con questa la milestone si chiude. + +## 1. Obiettivo + +Far rimappare all'utente i tasti della TUI dal proprio `config.yml`, senza che +una configurazione sbagliata produca comportamenti silenziosamente ambigui. + +Nessuna chiamata API nuova, nessuna schermata nuova, nessun tasto nuovo nei +default. + +## 2. Evidenza misurata + +Ogni numero qui sotto viene dall'aver **eseguito** il codice. Nella tranche +precedente tre affermazioni scritte a mano si sono rivelate false — un commento +di package, sei rapporti di contrasto e un conteggio di call site — e tutte e +tre sono state prese solo perché qualcuno ha eseguito qualcosa. + +### 2.1 Lo spazio dei tasti è già sovraccarico, di proposito + +Misurato iterando `keyDefaults` per reflection: + +- **51 binding** +- **39 tasti fisici distinti** +- **20 tasti sono rivendicati da più di un binding** + +Le collisioni non sono un difetto: sono il modo in cui la TUI sta in una +tastiera. Alcune: + +| tasto | binding che lo rivendicano | +|---|---| +| `n` | LogHours, NewOverride, NewTag, No | +| `enter` | Confirm, Generate, Yes | +| `h` | History, PrevMonth, PrevSection | +| `s` | ChangeRange, Save, StopTimer | +| `y` / `Y` | ConfirmDelete, Yes | + +Convivono perché `screenKeys` (`keys.go:304`) costruisce per ogni schermata un +`keyMap` assegnando **solo** i campi che quella schermata accetta; i campi non +assegnati restano il `key.Binding` zero, che — misurato — ha `Enabled() == false` +e non corrisponde a nulla. + +**La conseguenza governa tutto il design: una regola «due binding non possono +condividere un tasto» rifiuterebbe i default che spediamo.** + +### 2.2 La regola sui conflitti che regge senza enumerare gli stati + +Il rilevamento corretto sarebbe per-schermata, ma richiederebbe di enumerare +ogni stato raggiungibile — misurato: le 14 schermate, più le 5 modalità di +`entries`, gli 8 passi di `log`, le **4** sezioni di `rates` e i passi del setup +— e di costruire un `Model` rappresentativo per ognuno. È una superficie che va tenuta in sincronia con ogni +schermata futura: esattamente il moltiplicatore di manutenzione che la #82 si +autodenuncia. + +La regola scelta si calcola invece dalla **sola tabella dei tasti**: + +> Per ogni tasto rivendicato da **due o più** binding dopo gli override, +> l'insieme dei rivendicanti deve essere un **sottoinsieme** di quello che lo +> rivendicava prima. + +> **La clausola «due o più» non è un dettaglio, ed era assente dalla prima +> stesura.** Senza di essa la regola, applicata alla lettera, rifiuterebbe anche +> lo spostamento su un tasto libero: misurato, per `log_hours: "L"` il tasto `L` +> passa da `{}` a `{log_hours}`, che non è un sottoinsieme di `{}`. La review ha +> eseguito la formulazione contro il codice e ha trovato che dicevano cose +> diverse. + +Cosa permette e cosa no: + +- spostare `LogHours` da `n` a `L` → `L` ha un solo rivendicante, e `n` resta a + `{NewOverride, NewTag, No}` → **ammesso**; +- dare a `Export` il tasto `n` → `n` diventa `{LogHours, NewOverride, NewTag, + No, Export}`, non è un sottoinsieme → **rifiutato**, nominando `Export`, `n` e + chi già lo rivendica. + +**Il prezzo, misurato e non immaginato.** La prima stesura attribuiva +erroneamente un costo agli scambi innocui. **È falso**: uno scambio pulito passa. +Misurato, `{"quit": "r", "reload": "q"}` viene **accettato**: dopo gli +override `q` ha il solo rivendicante `Reload` e `r` il solo `Quit`, quindi la +clausola «due o più» non si applica. I rivendicanti cambiano, ma ogni destinazione +resta a rivendicante singolo. Per lo stesso motivo passa anche un ciclo a tre. + +Il prezzo vero è un altro: **prendere un tasto che un altro binding rivendica +ancora viene rifiutato, anche se i due non convivono su nessuna schermata.** Se +l'utente vuole `Export` su `g`, la regola lo rifiuta perché `ListBudget` +rivendica ancora `g`, e non sa che `Export` vive sul report e `ListBudget` sulle +tariffe. L'utente sceglie un altro tasto, con un messaggio che nomina la +collisione. Il prezzo dell'alternativa è una tabella di ogni stato di ogni +schermata, da mantenere per sempre. + +### 2.3 Far passare la tabella dal Model costa due righe + +`defaultKeys()` ha **tre** chiamate in produzione, misurate: + +| dove | cosa | +|---|---| +| `keys.go:293` | `keysFor` con la palette aperta | +| `keys.go:305` | `screenKeys` | +| `app.go:663` | `key.Matches(msg, defaultKeys().ForceQuit)` | + +Le prime due leggeranno la tabella dal `Model`. **La terza resta com'è**, ed è +una proprietà voluta: `ForceQuit` non è rimappabile, quindi il controllo di +`ctrl+c` legge letteralmente dai default non modificabili e nessuna +configurazione può togliere la via di fuga. + +### 2.4 I nomi si derivano, e l'override arriva fino in fondo + +Misurato: iterando `keyDefaults` per reflection si ottengono **51** nomi, e la +conversione in snake_case dà `quit`, `back`, `force_quit`, `help`, `palette`, +`palette_up`, `palette_down`, `prev_month`, … + +Misurato anche il percorso completo: sostituendo il campo `LogHours` con +`key.NewBinding(key.WithKeys("L"), …)` tramite `reflect.Value.Set`, la tabella +modificata produce `homeKeys(...).LogHours.Keys() == ["L"]`. L'override +sopravvive fino al `keyMap` di schermata senza che nessun costruttore per +schermata debba saperne nulla. + +**Questo elimina il moltiplicatore di manutenzione invece di accettarlo**: un +binding aggiunto in futuro a `keyDefaults` diventa rimappabile da solo; +`force_quit` è l'unica eccezione esplicita e resta fisso. Il rischio si sposta +altrove — rinominare un campo Go rinominerebbe in silenzio una chiave del config +di qualcuno — e va inchiodato con un test che fissa l'elenco completo dei 51 +nomi. + +## 3. Gli interventi + +### 3.1 Nessun package nuovo, e questa volta la ragione è che non serve + +La tranche precedente ha creato `internal/themes` perché `internal/config` e +`internal/tui` avevano bisogno dello **stesso tipo** — e la prima versione di +quella spec sbagliava perfino la motivazione, dichiarando forzato ciò che era +una scelta. + +Qui non c'è nulla da condividere: il config porta stringhe, e il tipo risolto +serve solo alla TUI. La tabella e la sua validazione restano in `internal/tui`, +accanto a ciò che validano. + +### 3.2 Il config + +```yaml +keys: + log_hours: "L" + up: ["up", "ctrl+u"] +``` + +```go +// KeySpec is one binding's keys as written in YAML: a bare key, or a list. +type KeySpec []string + +Keys map[string]KeySpec `yaml:"keys,omitempty"` +``` + +`KeySpec` ha un `UnmarshalYAML` tollerante che accetta scalare e sequenza, e un +`MarshalYAML` che ricollassa a scalare quando il tasto è uno solo — per la stessa +ragione misurata su `themes.Value`: `config.Save` serializza l'intera `Config`, e +senza il collasso un `log_hours: "L"` tornerebbe su disco come `log_hours: ["L"]` +alla prima cosa che salva. + +Il tipo vive in `internal/config` perché nessun altro package ne ha bisogno. + +### 3.3 `ResolveKeys` e la validazione + +```go +// KeyTable is the resolved binding table the TUI routes and renders with. +type KeyTable struct{ d keyDefaults } + +func DefaultKeyTable() KeyTable +func ResolveKeys(overrides map[string]config.KeySpec) (KeyTable, error) +func BindingNames() []string // the 51 snake_case names, sorted +``` + +Errori **bloccanti**, con il precedente di `billing.rounding.increment` +(`internal/service/pricing.go:71`) e degli errori dei temi: + +| caso | messaggio | +|---|---| +| nome sconosciuto | nomina il nome e elenca quelli validi, **`force_quit` escluso** | +| `force_quit` | dice che è la via di fuga e non è rimappabile | +| lista vuota, o un tasto vuoto nella lista | nomina il binding | +| nome di tasto non canonico o non producibile da un singolo `KeyMsg` | nomina binding e tasto; per gli alias noti suggerisce la forma canonica | +| **lo stesso tasto due volte nella stessa lista** | nomina il binding e il tasto | +| un tasto con due o più rivendicanti dopo gli override ne guadagna uno nuovo (§2.2) | nomina il tasto, il binding che lo chiede e chi già lo rivendica | + +Due dettagli che la review ha trovato eseguendo, e che sarebbero passati: + +- **L'elenco dei nomi validi non deve contenere `force_quit`**, o il messaggio + suggerisce all'utente l'unico nome che poi rifiuta. +- **Un tasto ripetuto nella stessa lista** (`quit: [Q, Q]`) sfugge alla + validazione e finisce nella regola sulle collisioni, che produce un messaggio + senza senso: misurato, «binding "quit" cannot take key "Q": it is already + claimed by » — con l'elenco vuoto, perché l'unico altro rivendicante è il + binding stesso. Va rifiutato prima, con un messaggio suo. + +Nessun fallback silenzioso: un tasto che non si riesce a interpretare fermerebbe +l'utente davanti a una TUI in cui un comando semplicemente non risponde, senza +niente a cui aggrapparsi. + +### 3.4 Il testo d'aiuto va rigenerato — in due posti, non uno + +`key.WithHelp("↑/k", "move up")` porta il tasto **dentro** la stringa d'aiuto, +che è ciò che il footer e il `?` mostrano. Un binding rimappato e non rigenerato +mentirebbe. + +**Rigenerarlo dentro `ResolveKeys` non basta, ed è il buco che la review ha +trovato eseguendo.** I costruttori per schermata riscrivono l'etichetta con +stringhe letterali in **24 punti** — 16 chiamate a `pairHelp` e 8 a `SetHelp` +(`keys.go:380,406,448,552,573,611,636-645,666,674,688,714,725,743,744,758,777-779`) +— perché una riga di footer copre spesso una *coppia* di binding: + +```go +pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") +k.ClearValue.SetHelp("d", "use the default rate") +``` + +Misurato con una tabella che rimappa `confirm→ctrl+j` e `back→ctrl+q`, il footer +della palette restava: + +``` +↑/↓ move · enter run · esc close · ctrl+c force quit +``` + +Il primo test che avevo prescritto guardava solo la `KeyTable`, mai un footer +reso: sarebbe passato mentendo. + +**La regola.** L'etichetta letterale si usa **finché nessuno dei binding che +copre è stato rimappato**; se uno lo è, si deriva dai tasti veri. Quindi: + +- il footer di default resta quello di oggi, con le frecce tipografiche, e + **nessun golden si muove**; +- chi rimappa vede i propri tasti. + +Perché serve sapere *quali* nomi sono stati sovrascritti, la `KeyTable` se li +ricorda, e i costruttori per schermata la ricevono al posto della sola +`keyDefaults`. + +Per il binding sovrascritto la ricostruzione unisce i tasti con `/` e mantiene +la descrizione originale. Le frecce tipografiche si perdono per i soli binding +rimappati — è l'utente ad aver scelto quei tasti, e inventargli una tipografia +sarebbe indovinare. + +### 3.4.1 Due stringhe di prosa nominano tasti rimappabili + +Fuori dal sistema di help, due messaggi scritti a mano citano un tasto: + +- `rates.go:618` — «press 'g' and submit an empty value to remove the budget», + dove `g` è `list_budget`; +- `rates_view.go:55` — «press 'b' to browse the workspace», dove `b` è + `browse_list`. + +Diventano false appena quei due binding vengono rimappati. Vanno costruite dal +binding vivo invece che dalla lettera. + +### 3.5 La firma, e un seme per la prossima volta + +`New(cfg, pal)` diventa `New(cfg, pal, kt)`. Sono di nuovo **44 call site**, i +secondi in due tranche. + +Insieme al cambio si introduce un helper di test — `testModel(cfg config.Config) +Model`, in un `helpers_test.go` nuovo — che i 43 call site dei test usano al +posto di `New`. Stessa quantità di modifiche adesso, ma la **prossima** crescita +della firma ne tocca una sola. + +I call site vanno trovati **facendo elencare al compilatore**, non con un grep: +nella tranche precedente `grep 'New(cfg)'` ne trovò nove su 44, e la correzione +successiva sbagliò comunque la ripartizione per file. + +### 3.6 La `KeyTable` zero deve significare «i default» + +Non è una comodità: è un vincolo misurato. I test costruiscono **108 letterali +`Model{…}`** senza passare da `New` — 28 nel solo `footer_golden_test.go`, 15 in +`app_test.go`, 13 in `range_test.go`. Se `screenKeys` leggesse una tabella zero +da quei Model, ogni binding risulterebbe disabilitato e i 108 andrebbero +modificati insieme ai 44 call site: oltre 150 punti per una feature che ne +richiede tre. + +> **Il grep originale sovrastimava di due.** La prima stesura contava con +> `grep -oE '(^|[^A-Za-z0-9_.])Model\{'`, che include due occorrenze dentro +> **commenti** (`app_test.go:337`, `footer_golden_test.go:71`). Contati sull'AST +> sono 108. È la quarta volta in tre tranche che un mio numero preso col grep si +> rivela sbagliato, e stavolta in una sezione che apre dichiarando che ogni +> numero viene dall'esecuzione. La conclusione regge — misurato, forzando +> `bindings()` a ritornare `kt.d` senza la guardia, **falliscono 134 test** — ma +> il numero no. + +```go +type KeyTable struct { + d keyDefaults + set bool // false on the zero value, where d is meaningless +} + +// bindings returns the table to route with. The zero KeyTable means the +// built-in defaults, which is what a Model built by hand in a test gets. +func (kt KeyTable) bindings() keyDefaults { + if !kt.set { + return defaultKeys() + } + return kt.d +} +``` + +Il rischio di «zero significa default» è noto ed è l'altra faccia della moneta +del campo `*bool` di `UpdateCheck`: se `cli` dimenticasse di passare la tabella, +gli override verrebbero ignorati **in silenzio**. Lo chiude il test di +instradamento end-to-end già richiesto in §4 — una tabella che sposta +`LogHours` su `L` deve far aprire la schermata di log su `L` e non su `n`, e non +passa se il valore non arriva fino a `Update`. + +## 4. Test + +- **Il test che porta il peso** fissa i **51 nomi** derivati per reflection, + scritti per esteso. È l'unica cosa che si accorge se qualcuno rinomina un + campo Go e con esso, in silenzio, una chiave nel config di un utente. +- **La regola sui conflitti** ha una tabella per la clausola post-override: solo + un tasto con due o più rivendicanti deve conservare un sottoinsieme dei + rivendicanti di default. Copre uno spostamento che libera un tasto (ammesso), + un binding che si aggiunge a un tasto già conteso (rifiutato), uno scambio + pulito fra due binding (ammesso perché ogni destinazione resta a rivendicante + singolo), e un binding che prende un tasto ancora rivendicato da un altro + (rifiutato: è il prezzo conservativo dichiarato in §2.2). +- **La rigenerazione dell'aiuto**: un binding rimappato deve mostrare i tasti + nuovi nel footer, e la descrizione vecchia. +- **`force_quit`** rifiutato per nome, e `ctrl+c` che continua a funzionare con + una tabella che rimappa tutto il resto. +- **L'instradamento end-to-end**: con una tabella che sposta `LogHours` su `L`, + `Update` deve aprire la schermata di log su `L` e **non** su `n`. +- **Ogni difetto ha un test visto fallire** contro il difetto, col transcript. +- **Golden**: attesi immutati. Da verificare eseguendo. +- Gate: `gofmt -l .`, `go vet ./...`, + `go run honnef.co/go/tools/cmd/staticcheck@latest ./...`, `go build ./...`, + `go test ./... -race`. + +## 5. Fuori scope (dichiarato) + +- **Rilevamento dei conflitti per-schermata** (§2.2): richiede una tabella di + stati da mantenere per sempre. La regola conservativa costa all'utente un + tasto diverso; questa costerebbe a ogni schermata futura una riga. +- **Rimappare `ForceQuit`**: `ctrl+c` è la via di fuga e resta fissa. +- **Una schermata per rimappare i tasti dentro la TUI**: si configura dal file. +- **Supporto alle sequenze multi-tasto** (`g g`, ` x`): bubbles/key non + le modella. `ResolveKeys` le rifiuta all'avvio invece di accettare un'azione + che nessun singolo `KeyMsg` può attivare. +- **Frecce tipografiche per i tasti rimappati** (§3.4). +- **Il wizard di setup che cancella `keys:` dal file.** Misurato: `setupModel.tmpCfg` + parte da un `config.Config` azzerato (`setup.go:34-43`) e `setup.go:124-125` lo + salva, quindi ri-autenticarsi dopo un token revocato cancella dal disco `keys` + insieme a `themes`, `timezone`, `rate` e all'intero blocco `billing`. È + **preesistente** ed è già registrato come **issue #149**; questa tranche + aggiunge una vittima e non la risolve. Il fix indicato nella issue — + inizializzare `tmpCfg` dal config caricato — vale per tutte insieme. + +## 6. Vincoli globali + +- `internal/report` e `internal/duration` restano **puri**: solo stdlib, nessun + I/O, nessun import di `internal/config`, `internal/clickup`, `internal/tui`, + `internal/themes`. +- `internal/themes` resta foglia. +- Nessuna funzione di stile in produzione chiama `lipgloss.NewStyle()`. +- Mai chiamare l'API ClickUp vera: il comportamento di rete si esercita solo con + `httptest`. +- Tutto ciò che vive nel repo è in **inglese** — codice, commenti, stringhe UI, + nomi e messaggi dei test, messaggi di commit. Eccezioni: `README.it.md`, + `CONTRIBUTING.it.md` e i doc di design sotto `docs/superpowers/`. +- **Ogni numero scritto in un commento va misurato eseguendo.** +- I golden si rigenerano solo con `go test ./internal/tui -update`, mai a mano. +- Conventional Commits. **MAI** `Co-Authored-By`. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 13dd84d..a95b530 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -48,6 +48,32 @@ func resolveTheme(cfg config.Config) (themes.Palette, error) { return pal, nil } +// resolveKeys builds the binding table the TUI will route with, and refuses to +// start on a configuration it cannot honor. Extracted from runTUI for the same +// reason resolveTheme and programOptions were: runTUI blocks on a terminal. +func resolveKeys(cfg config.Config) (tui.KeyTable, error) { + kt, err := tui.ResolveKeys(cfg.Keys) + if err != nil { + return tui.KeyTable{}, fmt.Errorf("keys: %w", err) + } + return kt, nil +} + +// buildModel resolves the user-configurable inputs and threads both into the +// TUI model. Keeping this outside runTUI makes the production construction path +// observable without launching a terminal. +func buildModel(cfg config.Config) (tui.Model, error) { + pal, err := resolveTheme(cfg) + if err != nil { + return tui.Model{}, err + } + kt, err := resolveKeys(cfg) + if err != nil { + return tui.Model{}, err + } + return tui.New(cfg, pal, kt), nil +} + // rootCmd builds the root command. Unexported so tests can exercise the // command wiring directly without going through Execute/os.Exit. func rootCmd() *cobra.Command { @@ -71,11 +97,11 @@ func runTUI(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("config: %w", err) } - pal, err := resolveTheme(cfg) + m, err := buildModel(cfg) if err != nil { return err } - p := tea.NewProgram(tui.New(cfg, pal), programOptions(cfg)...) + p := tea.NewProgram(m, programOptions(cfg)...) _, err = p.Run() return err } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 8581682..b7a39e1 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -1,9 +1,11 @@ package cli import ( + "slices" "strings" "testing" + tea "github.com/charmbracelet/bubbletea" "github.com/marcoarnulfo/clickup-cli/internal/config" ) @@ -95,3 +97,51 @@ func TestResolveThemeAcceptsABuiltin(t *testing.T) { t.Errorf("resolveTheme with no theme set = %v, want nil (the default)", err) } } + +func TestResolveKeysRejectsAnUnknownBinding(t *testing.T) { + t.Parallel() + _, err := resolveKeys(config.Config{Keys: map[string]config.KeySpec{"nope": {"x"}}}) + if err == nil { + t.Fatal("resolveKeys of an unknown binding = nil error, want one") + } + if !strings.Contains(err.Error(), "nope") { + t.Errorf("error %q does not name the binding the user asked for", err) + } + if !strings.HasPrefix(err.Error(), "keys:") { + t.Errorf("error %q is not prefixed with the config section it comes from", err) + } + _, valid, ok := strings.Cut(err.Error(), "valid names:") + if !ok { + t.Fatalf("error %q has no valid-name suffix", err) + } + if names := strings.Split(strings.TrimSpace(valid), ", "); slices.Contains(names, "force_quit") { + t.Errorf("valid-name suggestions include non-remappable force_quit: %v", names) + } +} + +func TestBuildModelRoutesWithTheResolvedKeyTable(t *testing.T) { + t.Parallel() + cfg := config.Config{ + Token: "t", + WorkspaceID: "team1", + Keys: map[string]config.KeySpec{"log_hours": {"L"}}, + } + + m, err := buildModel(cfg) + if err != nil { + t.Fatal(err) + } + got, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("L")}) + if view := got.View(); !strings.Contains(view, "Choose the mode:") { + t.Errorf("configured L did not open Log hours:\n%s", view) + } + + m, err = buildModel(cfg) + if err != nil { + t.Fatal(err) + } + got, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("n")}) + if view := got.View(); strings.Contains(view, "Choose the mode:") { + t.Errorf("default n still opened Log hours after the override:\n%s", view) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 9bd7a93..3ec4cad 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -46,6 +46,39 @@ type Billing struct { Rounding Rounding `yaml:"rounding,omitempty"` } +// KeySpec is one binding's ordered keys as written in YAML: a bare key or a +// list. The TUI consumes it when resolving configured binding overrides. +type KeySpec []string + +// UnmarshalYAML accepts both forms, so the common case is `log_hours: L` +// rather than `log_hours: [L]`. +func (k *KeySpec) UnmarshalYAML(n *yaml.Node) error { + if n.Kind == yaml.ScalarNode { + var s string + if err := n.Decode(&s); err != nil { + return err + } + *k = KeySpec{s} + return nil + } + var list []string + if err := n.Decode(&list); err != nil { + return err + } + *k = list + return nil +} + +// MarshalYAML collapses a single key back to a scalar. Without it, Save — which +// marshals the whole Config — would rewrite a user's `log_hours: L` as a +// one-element list the first time anything saved the config. +func (k KeySpec) MarshalYAML() (any, error) { + if len(k) == 1 { + return k[0], nil + } + return []string(k), nil +} + // Config is the persisted CLI configuration. type Config struct { SchemaVersion int `yaml:"schema_version"` @@ -99,6 +132,10 @@ type Config struct { // Themes are the user's own palettes, by name. A theme names only the // tokens it changes; internal/themes fills the rest from the default. Themes map[string]themes.Spec `yaml:"themes,omitempty"` + // Keys remaps the TUI's bindings by name. The names are the ones + // internal/tui derives from its binding table; this package holds the YAML + // as written and validates nothing. + Keys map[string]KeySpec `yaml:"keys,omitempty"` } // Valid reports whether the config can be used to query the API. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 80f3697..fad68b3 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/marcoarnulfo/clickup-cli/internal/themes" + "gopkg.in/yaml.v3" ) // isolateConfig redirects the config to a temp dir on ALL platforms. @@ -684,3 +685,80 @@ func TestThemeKeysAbsentAreZero(t *testing.T) { t.Fatalf("saved config mentions theme:\n%s", raw) } } + +// --- optional keys map (#82) --- + +func TestKeysRoundTrip(t *testing.T) { + isolateConfig(t) + in := Config{Token: "t", WorkspaceID: "1", Keys: map[string]KeySpec{ + "log_hours": {"L"}, + "up": {"up", "w"}, + }} + if err := Save(in); err != nil { + t.Fatal(err) + } + got, err := Load() + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(got.Keys, in.Keys) { + t.Errorf("Keys = %v, want %v", got.Keys, in.Keys) + } +} + +func TestKeySpecAcceptsAScalarAndAList(t *testing.T) { + t.Parallel() + var got map[string]KeySpec + src := "log_hours: \"L\"\nup: [up, w]\n" + if err := yaml.Unmarshal([]byte(src), &got); err != nil { + t.Fatal(err) + } + want := map[string]KeySpec{ + "log_hours": {"L"}, + "up": {"up", "w"}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("decoded keys = %v, want %v", got, want) + } +} + +// Save serializes the whole Config, so without MarshalYAML a single key written +// as a scalar would come back as a one-element list the first time anything +// saves the config. This test is the only thing that would notice. +func TestSaveKeepsASingleKeyScalar(t *testing.T) { + isolateConfig(t) + if err := Save(Config{Token: "t", WorkspaceID: "1", Keys: map[string]KeySpec{ + "log_hours": {"L"}, + }}); err != nil { + t.Fatal(err) + } + p, err := Path() + if err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), `log_hours: L`) { + t.Fatalf("saved config did not keep the scalar form:\n%s", raw) + } +} + +func TestKeysAbsentIsNotWritten(t *testing.T) { + isolateConfig(t) + if err := Save(Config{Token: "t", WorkspaceID: "1"}); err != nil { + t.Fatal(err) + } + p, err := Path() + if err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "keys:") { + t.Fatalf("saved config mentions keys:\n%s", raw) + } +} diff --git a/internal/tui/actions.go b/internal/tui/actions.go index 59fbe3d..f68201f 100644 --- a/internal/tui/actions.go +++ b/internal/tui/actions.go @@ -1,6 +1,8 @@ package tui import ( + "fmt" + "strings" "unicode" tea "github.com/charmbracelet/bubbletea" @@ -45,15 +47,12 @@ func screenActions(m Model) []action { if !b.Enabled() { continue } - keys := b.Keys() - if len(keys) == 0 { - continue - } - msg, ok := keyMsgFor(keys[0]) - if !ok { - // Unreachable today, pinned by TestEveryPaletteBindingIsReplayable. - // Dropping beats firing a KeyMsg that matches nothing. - continue + configured := b.Keys()[0] + msg, err := parseKeyName(configured) + if err != nil { + // Overrides are parsed by ResolveKeys before Model construction, and + // defaults are covered by TestEveryPaletteBindingIsReplayable. + panic(fmt.Sprintf("invalid key %q reached the command palette: %v", configured, err)) } out = append(out, action{ label: capitalize(b.Help().Desc), @@ -137,30 +136,84 @@ func globalActions(m Model) []action { }}) } -// keyMsgFor rebuilds the tea.KeyMsg a binding's first key would produce. +// canonicalKeyTypes is the exported Bubble Tea v1.3.10 key set whose String +// forms represent one terminal key. KeyRunes is handled separately so a config +// can contain any single rune without treating a multi-rune sequence as a key. +var canonicalKeyTypes = []tea.KeyType{ + // Canonical control-code names. Several values also have aliases; only the + // spelling returned by KeyMsg.String is listed here. + tea.KeyCtrlAt, + tea.KeyCtrlA, tea.KeyCtrlB, tea.KeyCtrlC, tea.KeyCtrlD, tea.KeyCtrlE, + tea.KeyCtrlF, tea.KeyCtrlG, tea.KeyCtrlH, tea.KeyTab, tea.KeyCtrlJ, + tea.KeyCtrlK, tea.KeyCtrlL, tea.KeyEnter, tea.KeyCtrlN, tea.KeyCtrlO, + tea.KeyCtrlP, tea.KeyCtrlQ, tea.KeyCtrlR, tea.KeyCtrlS, tea.KeyCtrlT, + tea.KeyCtrlU, tea.KeyCtrlV, tea.KeyCtrlW, tea.KeyCtrlX, tea.KeyCtrlY, + tea.KeyCtrlZ, tea.KeyEsc, tea.KeyCtrlBackslash, tea.KeyCtrlCloseBracket, + tea.KeyCtrlCaret, tea.KeyCtrlUnderscore, tea.KeyBackspace, + + // Other keys recognized by Bubble Tea's terminal input parser. + tea.KeyUp, tea.KeyDown, tea.KeyRight, tea.KeyLeft, tea.KeyShiftTab, + tea.KeyHome, tea.KeyEnd, tea.KeyPgUp, tea.KeyPgDown, + tea.KeyCtrlPgUp, tea.KeyCtrlPgDown, tea.KeyDelete, tea.KeyInsert, tea.KeySpace, + tea.KeyCtrlUp, tea.KeyCtrlDown, tea.KeyCtrlRight, tea.KeyCtrlLeft, + tea.KeyCtrlHome, tea.KeyCtrlEnd, + tea.KeyShiftUp, tea.KeyShiftDown, tea.KeyShiftRight, tea.KeyShiftLeft, + tea.KeyShiftHome, tea.KeyShiftEnd, + tea.KeyCtrlShiftUp, tea.KeyCtrlShiftDown, tea.KeyCtrlShiftRight, tea.KeyCtrlShiftLeft, + tea.KeyCtrlShiftHome, tea.KeyCtrlShiftEnd, + tea.KeyF1, tea.KeyF2, tea.KeyF3, tea.KeyF4, tea.KeyF5, + tea.KeyF6, tea.KeyF7, tea.KeyF8, tea.KeyF9, tea.KeyF10, + tea.KeyF11, tea.KeyF12, tea.KeyF13, tea.KeyF14, tea.KeyF15, + tea.KeyF16, tea.KeyF17, tea.KeyF18, tea.KeyF19, tea.KeyF20, +} + +var keyNameAliases = map[string]string{ + "space": " ", + "ctrl+`": "ctrl+@", + "ctrl+i": "tab", + "ctrl+m": "enter", + "ctrl+[": "esc", + "ctrl+?": "backspace", +} + +// parseKeyName validates a canonical key name and rebuilds the exact tea.KeyMsg +// a terminal sends. ResolveKeys and command-palette replay both use it, so a +// binding accepted at startup cannot later disappear from the palette. // -// The set is closed on purpose. key.Matches compares msg.String() against the -// binding's key strings, and these four round-trip exactly. A KeyRunes message -// carrying "tab" would round-trip too, but every handler that reads msg.Type -// rather than key.Matches would then see the wrong thing — so the palette -// leaves those bindings out instead of forging their messages. ok is false for -// anything else, so an action that cannot be executed faithfully is dropped -// rather than mis-fired. -func keyMsgFor(s string) (tea.KeyMsg, bool) { - switch s { - case "enter": - return tea.KeyMsg{Type: tea.KeyEnter}, true - // PrevMonth and NextMonth lead with "left" and "right" (keys.go:124-125), - // and they are the only two palette bindings whose first key is not a rune. - case "left": - return tea.KeyMsg{Type: tea.KeyLeft}, true - case "right": - return tea.KeyMsg{Type: tea.KeyRight}, true +// The exact String round trip is load-bearing: key.Matches compares strings, +// while handlers may also inspect Type. Reconstructing a special name as runes +// would pass the former and violate the latter. +func parseKeyName(name string) (tea.KeyMsg, error) { + base := name + alt := strings.HasPrefix(base, "alt+") + if alt { + base = strings.TrimPrefix(base, "alt+") } - if r := []rune(s); len(r) == 1 { - return tea.KeyMsg{Type: tea.KeyRunes, Runes: r}, true + if canonical, ok := keyNameAliases[base]; ok { + if alt { + canonical = "alt+" + canonical + } + return tea.KeyMsg{}, fmt.Errorf("noncanonical key %q; use %q instead", name, canonical) + } + + for _, typ := range canonicalKeyTypes { + msg := tea.KeyMsg{Type: typ, Alt: alt} + if typ == tea.KeySpace { + msg.Runes = []rune{' '} + } + if msg.String() == name { + return msg, nil + } + } + + runes := []rune(base) + if len(runes) == 1 && (runes[0] > 0x1f && runes[0] != 0x7f) { + msg := tea.KeyMsg{Type: tea.KeyRunes, Runes: runes, Alt: alt} + if msg.String() == name { + return msg, nil + } } - return tea.KeyMsg{}, false + return tea.KeyMsg{}, fmt.Errorf("key %q is not a canonical single Bubble Tea key", name) } // capitalize upper-cases the first rune. strings.ToUpper on s[:1] would corrupt diff --git a/internal/tui/actions_test.go b/internal/tui/actions_test.go index dc07e45..d87cb8b 100644 --- a/internal/tui/actions_test.go +++ b/internal/tui/actions_test.go @@ -6,6 +6,8 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/marcoarnulfo/clickup-cli/internal/clickup" + "github.com/marcoarnulfo/clickup-cli/internal/config" + "github.com/marcoarnulfo/clickup-cli/internal/themes" ) func labels(as []action) []string { @@ -25,36 +27,178 @@ func hasLabel(as []action, want string) bool { return false } -func TestKeyMsgFor(t *testing.T) { +func TestParseKeyNameAcceptsCanonicalNames(t *testing.T) { t.Parallel() for _, tc := range []struct { - in string - wantOK bool - want string + name string + type_ tea.KeyType + alt bool + runes string }{ - {"g", true, "g"}, - {"enter", true, "enter"}, - // PrevMonth and NextMonth are "left"/"h" and "right"/"l", so their - // first key is not a rune. Without these two the palette would silently - // drop both month-navigation actions on Home. - {"left", true, "left"}, - {"right", true, "right"}, - {"tab", false, ""}, - {"shift+tab", false, ""}, - {"up", false, ""}, - {"", false, ""}, + {name: "g", type_: tea.KeyRunes, runes: "g"}, + {name: "界", type_: tea.KeyRunes, runes: "界"}, + {name: "alt+é", type_: tea.KeyRunes, alt: true, runes: "é"}, + {name: " ", type_: tea.KeySpace, runes: " "}, + {name: "alt+ ", type_: tea.KeySpace, alt: true, runes: " "}, + {name: "ctrl+@", type_: tea.KeyCtrlAt}, + {name: "ctrl+a", type_: tea.KeyCtrlA}, + {name: "ctrl+h", type_: tea.KeyCtrlH}, + {name: "tab", type_: tea.KeyTab}, + {name: "ctrl+j", type_: tea.KeyCtrlJ}, + {name: "ctrl+l", type_: tea.KeyCtrlL}, + {name: "enter", type_: tea.KeyEnter}, + {name: "ctrl+n", type_: tea.KeyCtrlN}, + {name: "ctrl+z", type_: tea.KeyCtrlZ}, + {name: "esc", type_: tea.KeyEsc}, + {name: "ctrl+\\", type_: tea.KeyCtrlBackslash}, + {name: "ctrl+]", type_: tea.KeyCtrlCloseBracket}, + {name: "ctrl+^", type_: tea.KeyCtrlCaret}, + {name: "ctrl+_", type_: tea.KeyCtrlUnderscore}, + {name: "backspace", type_: tea.KeyBackspace}, + {name: "alt+ctrl+a", type_: tea.KeyCtrlA, alt: true}, + {name: "alt+enter", type_: tea.KeyEnter, alt: true}, } { - t.Run(tc.in, func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { t.Parallel() - msg, ok := keyMsgFor(tc.in) - if ok != tc.wantOK { - t.Fatalf("keyMsgFor(%q) ok = %v, want %v", tc.in, ok, tc.wantOK) + msg, err := parseKeyName(tc.name) + if err != nil { + t.Fatalf("parseKeyName(%q) = %v", tc.name, err) } - // The round trip is the whole point: key.Matches compares - // msg.String() against the binding's key strings. - if ok && msg.String() != tc.want { - t.Errorf("keyMsgFor(%q).String() = %q, want %q", tc.in, msg.String(), tc.want) + if msg.Type != tc.type_ || msg.Alt != tc.alt || string(msg.Runes) != tc.runes { + t.Errorf("parseKeyName(%q) = %+v, want type=%v alt=%v runes=%q", tc.name, msg, tc.type_, tc.alt, tc.runes) } + if got := msg.String(); got != tc.name { + t.Errorf("parseKeyName(%q).String() = %q", tc.name, got) + } + }) + } +} + +func TestParseKeyNameAcceptsEveryCanonicalSpecialKey(t *testing.T) { + t.Parallel() + for _, name := range []string{ + "up", "down", "right", "left", "shift+tab", + "home", "end", "pgup", "pgdown", "ctrl+pgup", "ctrl+pgdown", + "delete", "insert", "ctrl+up", "ctrl+down", "ctrl+right", "ctrl+left", + "ctrl+home", "ctrl+end", "shift+up", "shift+down", "shift+right", "shift+left", + "shift+home", "shift+end", "ctrl+shift+up", "ctrl+shift+down", + "ctrl+shift+right", "ctrl+shift+left", "ctrl+shift+home", "ctrl+shift+end", + "f1", "f2", "f3", "f4", "f5", "f6", "f7", "f8", "f9", "f10", + "f11", "f12", "f13", "f14", "f15", "f16", "f17", "f18", "f19", "f20", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + for _, prefix := range []string{"", "alt+"} { + full := prefix + name + msg, err := parseKeyName(full) + if err != nil { + t.Errorf("parseKeyName(%q) = %v", full, err) + continue + } + if got := msg.String(); got != full { + t.Errorf("parseKeyName(%q).String() = %q", full, got) + } + } + }) + } +} + +func TestParseKeyNameRejectsAliasesAndUnreachableNames(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + canonical string + }{ + {name: ""}, + {name: "g g"}, + {name: "gg"}, + {name: "f21"}, + {name: "runes"}, + {name: "alt+"}, + {name: "space", canonical: " "}, + {name: "ctrl+`", canonical: "ctrl+@"}, + {name: "ctrl+i", canonical: "tab"}, + {name: "ctrl+m", canonical: "enter"}, + {name: "ctrl+[", canonical: "esc"}, + {name: "ctrl+?", canonical: "backspace"}, + {name: "alt+ctrl+i", canonical: "alt+tab"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := parseKeyName(tc.name) + if err == nil { + t.Fatalf("parseKeyName(%q) = nil error", tc.name) + } + if tc.canonical != "" && !strings.Contains(err.Error(), tc.canonical) { + t.Errorf("parseKeyName(%q) error %q does not suggest %q", tc.name, err, tc.canonical) + } + }) + } +} + +func reportModelWithOverrides(t *testing.T, overrides map[string]config.KeySpec) Model { + t.Helper() + kt, err := ResolveKeys(overrides) + if err != nil { + t.Fatal(err) + } + m := New(config.Config{Token: "t", WorkspaceID: "team1"}, themes.Default(), kt) + m.screen = screenReport + m.nav = []screen{screenHome} + m.report = goldenReport() + return m +} + +func TestScreenActionsReplayAControlKeyOverride(t *testing.T) { + t.Parallel() + m := reportModelWithOverrides(t, map[string]config.KeySpec{"export": {"ctrl+e"}}) + + var run func(Model) (tea.Model, tea.Cmd) + for _, a := range screenActions(m) { + if a.label == "Export" { + run = a.run + } + } + if run == nil { + t.Fatal("a ctrl+e-only Export binding disappeared from screenActions") + } + viaAction, _ := run(m) + viaKey, _ := m.Update(tea.KeyMsg{Type: tea.KeyCtrlE}) + if got, want := viaAction.(Model).screen, viaKey.(Model).screen; got != want { + t.Errorf("action landed on %v, configured ctrl+e keypress landed on %v", got, want) + } + if viaAction.(Model).screen != screenExport { + t.Errorf("screen = %v, want screenExport", viaAction.(Model).screen) + } +} + +func TestScreenActionsReplaySpecialKeyOverrides(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + msg tea.KeyMsg + }{ + {name: "f1", msg: tea.KeyMsg{Type: tea.KeyF1}}, + {name: "home", msg: tea.KeyMsg{Type: tea.KeyHome}}, + {name: "delete", msg: tea.KeyMsg{Type: tea.KeyDelete}}, + {name: "ctrl+shift+up", msg: tea.KeyMsg{Type: tea.KeyCtrlShiftUp}}, + {name: "alt+f20", msg: tea.KeyMsg{Type: tea.KeyF20, Alt: true}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := reportModelWithOverrides(t, map[string]config.KeySpec{"export": {tc.name}}) + for _, a := range screenActions(m) { + if a.label != "Export" { + continue + } + viaAction, _ := a.run(m) + viaKey, _ := m.Update(tc.msg) + if got, want := viaAction.(Model).screen, viaKey.(Model).screen; got != want || got != screenExport { + t.Errorf("action screen = %v, physical %s screen = %v, want %v", got, tc.name, want, screenExport) + } + return + } + t.Fatal("Export disappeared from screenActions") }) } } diff --git a/internal/tui/app.go b/internal/tui/app.go index 2e8e8d3..98423a5 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -106,6 +106,9 @@ type Model struct { // explicitly to each view rather than read from package state, so a view // can never render with an unset theme. theme theme + // keys is the resolved binding table. The zero value means the built-in + // defaults, so a Model built by hand in a test routes normally. + keys KeyTable // latestVersion is the newer published release, "" when up to date or // unknown (the check hasn't returned yet, is disabled, or failed silently). @@ -196,10 +199,10 @@ type Model struct { } // New builds the root model from the config and the already-resolved palette -// it renders with. Resolving the palette is the caller's job (internal/cli's -// resolveTheme): by the time New runs, a bad theme name or color has already -// been turned into a startup error, so New itself never fails on it. -func New(cfg config.Config, pal themes.Palette) Model { +// and bindings it uses. Resolving them is the caller's job (internal/cli's +// resolveTheme and resolveKeys): by the time New runs, invalid configuration +// has already been turned into a startup error, so New itself never fails on it. +func New(cfg config.Config, pal themes.Palette, kt KeyTable) Model { demo := demoEnabled() if demo { cfg = demoConfig() @@ -212,6 +215,7 @@ func New(cfg config.Config, pal themes.Palette) Model { client: clickup.New(cfg.Token), now: time.Now, theme: newTheme(lipgloss.DefaultRenderer(), pal), + keys: kt, } // Best-effort default so range/label display works before the first report // build; a genuinely invalid configured zone is caught and surfaced by @@ -1005,10 +1009,10 @@ func (m Model) screenBody() string { case screenExport: return m.export.view(m.theme) case screenRates: - return m.ratesScreen.view(m.theme) + return m.ratesScreen.view(m.theme, m.keys) case screenLog: m.logScreen.now = m.now() - return m.logScreen.view(m.theme) + return m.logScreen.view(m.theme, m.keys) case screenMembers: return m.membersScreen.view(m.theme) case screenRange: diff --git a/internal/tui/app_test.go b/internal/tui/app_test.go index de7fb7c..87f4bd0 100644 --- a/internal/tui/app_test.go +++ b/internal/tui/app_test.go @@ -20,7 +20,6 @@ import ( "github.com/marcoarnulfo/clickup-cli/internal/config" "github.com/marcoarnulfo/clickup-cli/internal/export" "github.com/marcoarnulfo/clickup-cli/internal/report" - "github.com/marcoarnulfo/clickup-cli/internal/themes" ) // TestDefaultYearMonthFollowsLoc pins the fix for "New's default year/month @@ -105,7 +104,7 @@ func TestLoadEntriesUsesGivenRange(t *testing.T) { } func TestSetupIgnoresKeysWhileLoading(t *testing.T) { - m := New(config.Config{}, themes.Default()) + m := testModel(config.Config{}) for _, r := range "tok" { u, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{r}}) m = u.(Model) @@ -126,7 +125,7 @@ func TestSetupIgnoresKeysWhileLoading(t *testing.T) { } func TestSetupRejectsInvalidRate(t *testing.T) { - m := New(config.Config{}, themes.Default()) + m := testModel(config.Config{}) m.setup.step = stepRate m.setup.input = newNumberInput("") m.setup.input.SetValue("abc") @@ -320,14 +319,14 @@ func TestReloadEntriesCmdIgnoresSelectionInMeScope(t *testing.T) { } func TestNewStartsInSetupWhenInvalid(t *testing.T) { - m := New(config.Config{}, themes.Default()) + m := testModel(config.Config{}) if m.screen != screenSetup { t.Fatalf("invalid config should start in setup, got %v", m.screen) } } func TestNewStartsInHomeWhenValid(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) if m.screen != screenHome { t.Fatalf("valid config should start in home, got %v", m.screen) } @@ -353,7 +352,7 @@ func TestNewInDemoModeBuildsClientFromDemoToken(t *testing.T) { defer srv.Close() realCfg := config.Config{Token: "REAL-SECRET-TOKEN", WorkspaceID: "1"} - m := New(realCfg, themes.Default()) + m := testModel(realCfg) if !m.demo { t.Fatal("expected m.demo = true with CLICKUP_DEMO set") } @@ -372,7 +371,7 @@ func TestNewInDemoModeBuildsClientFromDemoToken(t *testing.T) { } func TestErrMsgSwitchesToErrorScreen(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) updated, _ := m.Update(errMsg{err: errTest}) mm := updated.(Model) if mm.screen != screenError { @@ -383,7 +382,7 @@ func TestErrMsgSwitchesToErrorScreen(t *testing.T) { // #38: a retryable error originating from Home must send the user back to // Home with an inline message, instead of the dead-end error screen. func TestRetryableErrOnHomeStaysHome(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) updated, _ := m.Update(retryableErrMsg{origin: screenHome, err: errors.New("boom")}) mm := updated.(Model) if mm.screen != screenHome { @@ -396,7 +395,7 @@ func TestRetryableErrOnHomeStaysHome(t *testing.T) { // #38: a 401 must still relaunch the setup wizard, regardless of origin. func TestRetryableErrUnauthorizedGoesToSetup(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) updated, _ := m.Update(retryableErrMsg{origin: screenHome, err: fmt.Errorf("x: %w", clickup.ErrUnauthorized)}) mm := updated.(Model) if mm.screen != screenSetup { @@ -405,7 +404,7 @@ func TestRetryableErrUnauthorizedGoesToSetup(t *testing.T) { } func TestEntriesMsgBuildsReportAndShowsReportScreen(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR"}) m.year, m.month = 2026, 7 updated, _ := m.Update(entriesMsg{entries: []report.TimeEntry{}}) mm := updated.(Model) @@ -423,7 +422,7 @@ func TestEntriesMsgBuildsReportAndShowsReportScreen(t *testing.T) { // day-grouping are computed in Rome — never a mix of Rome and UTC. func TestEntriesMsgThreadsConfiguredTimezoneThroughRangeAndBuild(t *testing.T) { cfg := config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR", Timezone: "Europe/Rome"} - m := New(cfg, themes.Default()) + m := testModel(cfg) m.year, m.month = 2026, 7 m.preset = report.PresetThisMonth m.report.GroupBy = report.GroupByDay // carried across the reload by entriesMsg @@ -460,7 +459,7 @@ func TestEntriesMsgThreadsConfiguredTimezoneThroughRangeAndBuild(t *testing.T) { func TestEntriesMsgWithBadRoundingRoutesToErrorScreen(t *testing.T) { cfg := config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR"} cfg.Billing.Rounding.Increment = "not-a-duration" - m := New(cfg, themes.Default()) + m := testModel(cfg) m.year, m.month = 2026, 7 updated, _ := m.Update(entriesMsg{entries: []report.TimeEntry{}}) mm := updated.(Model) @@ -477,7 +476,7 @@ func TestEntriesMsgWithBadRoundingRoutesToErrorScreen(t *testing.T) { // — never silently fall back to time.Local or UTC. func TestEntriesMsgWithBadTimezoneRoutesToErrorScreen(t *testing.T) { cfg := config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR", Timezone: "Not/AZone"} - m := New(cfg, themes.Default()) + m := testModel(cfg) m.year, m.month = 2026, 7 updated, _ := m.Update(entriesMsg{entries: []report.TimeEntry{}}) mm := updated.(Model) @@ -490,7 +489,7 @@ func TestEntriesMsgWithBadTimezoneRoutesToErrorScreen(t *testing.T) { } func TestQuitKey(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) if cmd == nil { t.Fatal("q should return a quit command") @@ -502,7 +501,7 @@ func TestQuitKey(t *testing.T) { // key.Matches(msg, defaultKeys().ForceQuit)): it must quit even on a screen // where 'q' itself does not (screenLog — see TestQuitBindingPerScreen). func TestCtrlCForceQuits(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) m.screen = screenLog _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyCtrlC}) if cmd == nil { @@ -511,7 +510,7 @@ func TestCtrlCForceQuits(t *testing.T) { } func TestSetupTokenStepAcceptsInput(t *testing.T) { - m := New(config.Config{}, themes.Default()) + m := testModel(config.Config{}) // type a character in the token field updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'x'}}) mm := updated.(Model) @@ -527,7 +526,7 @@ type testErr struct{} func (*testErr) Error() string { return "boom" } func TestReportCycleGroupBy(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR"}) m.year, m.month = 2026, 7 updated, _ := m.Update(entriesMsg{entries: []report.TimeEntry{ {TaskName: "A", ListName: "L", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -557,7 +556,7 @@ func TestEntriesMsgMemberGroupingDoesNotLeakIntoMeScope(t *testing.T) { } func TestHomeChangesMonthAndScope(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) m.year, m.month = 2026, 7 m.home = newHome() @@ -582,7 +581,7 @@ func TestExportWritesFile(t *testing.T) { defer os.Chdir(oldwd) os.Chdir(dir) - m := New(config.Config{Token: "t", WorkspaceID: "1", Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Currency: "EUR"}) m.year, m.month = 2026, 7 jStart := time.Date(2026, time.July, 1, 0, 0, 0, 0, time.UTC) m.report = report.Report{Start: jStart, End: jStart.AddDate(0, 1, 0), DefaultCurrency: "EUR", @@ -602,7 +601,7 @@ func TestExportWritesFile(t *testing.T) { } func TestHomeEnterStartsLoading(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) m.home = newHome() updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) mm := updated.(Model) @@ -615,7 +614,7 @@ func TestHomeEnterStartsLoading(t *testing.T) { } func TestRatesScreenOpensFromReport(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Client Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -638,7 +637,7 @@ func TestRatesScreenEditSaveRecomputes(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", dir) t.Setenv("CLICKUP_TOKEN", "") - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: 2 * time.Hour, Billable: true}, @@ -687,7 +686,7 @@ func TestRatesScreenSaveWithBadRoundingIsRejected(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", dir) t.Setenv("CLICKUP_TOKEN", "") - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: 2 * time.Hour, Billable: true}, @@ -711,7 +710,7 @@ func TestRatesScreenSaveWithBadRoundingIsRejected(t *testing.T) { } func TestRatesScreenEscCancelsEdit(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -737,7 +736,7 @@ func TestRatesScreenEscCancelsEdit(t *testing.T) { } func TestRatesScreenInvalidRateStaysEditing(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -765,7 +764,7 @@ func TestRatesScreenInvalidRateStaysEditing(t *testing.T) { } func TestRatesScreenRejectsNonNumericInput(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -784,7 +783,7 @@ func TestRatesScreenRejectsNonNumericInput(t *testing.T) { } func TestRatesScreenEscDiscardsAndReturns(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -817,7 +816,7 @@ func TestRatesScreenDropsOverrideEqualToDefault(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", dir) t.Setenv("CLICKUP_TOKEN", "") - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -961,7 +960,7 @@ func TestRatesScreenSaveErrorStaysOnScreen(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", f) // ...or from here (Linux); a file => MkdirAll fails t.Setenv("CLICKUP_TOKEN", "") - m := New(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Rate: 30, Currency: "EUR"}) m.year, m.month = 2026, 7 entries := []report.TimeEntry{ {ListID: "55", ListName: "Z", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Duration: time.Hour, Billable: true}, @@ -1178,7 +1177,7 @@ func TestStatusEnrichUnauthorizedReturnsErrMsg(t *testing.T) { // Fix 3: 'q' must not quit while typing a custom date on the range screen — // it has free-text inputs, so a stray 'q' shouldn't kill the whole session. func TestQuitKeyDoesNotQuitWhileEditingCustomRange(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) m.screen = screenRange m.rangeScreen = newRange(report.PresetThisMonth) m.rangeScreen.editing = true @@ -1247,7 +1246,7 @@ func TestSpacesMsgWarmsCacheAndUpdatesWhenOnBrowser(t *testing.T) { // newWithClock is a test helper that builds a Model with an injected clock. func newWithClock(cfg config.Config, now func() time.Time) Model { - m := New(cfg, themes.Default()) + m := testModel(cfg) m.now = now // Re-derive the default month from the injected clock the same way New // does, so the helper cannot re-encode the local-calendar bug @@ -1303,7 +1302,7 @@ func TestExportInvoiceWritesDistinctFile(t *testing.T) { Buckets: []report.Bucket{{Label: "A", Key: "l1", Hours: 1}}, TotalHours: 1} for i, f := range exportFormats { - m := New(config.Config{Token: "t", WorkspaceID: "1", Currency: "EUR"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1", Currency: "EUR"}) m.report = r m.export = newExport(r) m.export.idx = i @@ -1323,7 +1322,7 @@ func TestExportInvoiceWritesDistinctFile(t *testing.T) { } func TestUpdateAvailableMsgSetsLatestVersion(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) next, _ := m.Update(updateAvailableMsg{latest: "v1.8.0"}) got := next.(Model) if got.latestVersion != "v1.8.0" { @@ -1342,7 +1341,7 @@ func TestUpdateAvailableMsgSetsLatestVersion(t *testing.T) { // sub-command's message is one of those demo-safe kinds, never an // update-check message. func TestInitDoesNotCheckInDemoMode(t *testing.T) { - m := New(config.Config{}, themes.Default()) + m := testModel(config.Config{}) m.demo = true cmd := m.Init() if cmd == nil { diff --git a/internal/tui/demo_test.go b/internal/tui/demo_test.go index 636e828..8b9c945 100644 --- a/internal/tui/demo_test.go +++ b/internal/tui/demo_test.go @@ -7,12 +7,11 @@ import ( "github.com/marcoarnulfo/clickup-cli/internal/config" "github.com/marcoarnulfo/clickup-cli/internal/report" - "github.com/marcoarnulfo/clickup-cli/internal/themes" ) func TestDemoModeSkipsSetup(t *testing.T) { t.Setenv("CLICKUP_DEMO", "1") - m := New(config.Config{}, themes.Default()) // empty config: without demo it would go to setup + m := testModel(config.Config{}) // empty config: without demo it would go to setup if !m.demo { t.Fatal("expected m.demo = true with CLICKUP_DEMO set") } @@ -167,7 +166,7 @@ func TestDemoEntriesHaveBillableMix(t *testing.T) { // percentage. func TestDemoReportShowsBillingDepth(t *testing.T) { t.Setenv("CLICKUP_DEMO", "1") - m := New(config.Config{}, themes.Default()) + m := testModel(config.Config{}) m.year, m.month = 2026, time.July msg := m.reloadEntriesCmd(screenHome)() @@ -228,7 +227,7 @@ func TestDemoModeBillingEditorNeverWritesConfig(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", dir) t.Setenv("CLICKUP_DEMO", "1") - m := New(config.Config{}, themes.Default()) + m := testModel(config.Config{}) m.entries = demoEntries(demoJuly2026()) m.ratesScreen = newRates(m.entries, m.cfg) m.screen = screenRates diff --git a/internal/tui/entries.go b/internal/tui/entries.go index 207c0f1..684d6a4 100644 --- a/internal/tui/entries.go +++ b/internal/tui/entries.go @@ -285,7 +285,7 @@ func (m Model) updateEntries(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // updateEntriesEdit drives the multi-field edit form, mirroring logModel's // logForm step handling in updateLog: Duration → Date → Time → Note → -// Billable, each step validating on Enter before advancing. +// Billable, each text step validating on Confirm before advancing. func (m Model) updateEntriesEdit(es entriesModel, msg tea.KeyMsg) (tea.Model, tea.Cmd) { k := keysFor(m) if key.Matches(msg, k.Back) { @@ -586,7 +586,7 @@ func (m Model) entriesView(th theme) string { duration.FormatHours(e.Duration) + ")?" } if es.mode == entriesEdit { - return entriesEditView(th, es) + return entriesEditView(th, es, m.keys) } if es.mode == entriesHistory { return entriesHistoryView(th, es, m.loc) @@ -666,10 +666,10 @@ func (m Model) entriesView(th theme) string { // entriesEditView renders the multi-field edit form, mirroring logModel's // logForm rendering (see log.go view()). -func entriesEditView(th theme, es entriesModel) string { +func entriesEditView(th theme, es entriesModel, kt KeyTable) string { b := th.Title.Render("Edit entry") + "\n\n" if es.editStep == 4 { - b += "Billable? " + th.Accent.Render("[Y/n]") + " (Enter = yes)" + b += billablePrompt(th, kt) } else { labels := []string{"Duration", "Date (YYYY-MM-DD)", "Time (HH:MM)", "Note (optional)"} b += labels[es.editStep] + ":\n\n" + es.input.View() diff --git a/internal/tui/golden_test.go b/internal/tui/golden_test.go index d084e3c..629b35f 100644 --- a/internal/tui/golden_test.go +++ b/internal/tui/golden_test.go @@ -15,7 +15,6 @@ import ( "github.com/marcoarnulfo/clickup-cli/internal/clickup" "github.com/marcoarnulfo/clickup-cli/internal/config" "github.com/marcoarnulfo/clickup-cli/internal/report" - "github.com/marcoarnulfo/clickup-cli/internal/themes" ) var updateGolden = flag.Bool("update", false, "rewrite testdata/*.golden files") @@ -60,7 +59,7 @@ var goldenFixedTime = time.Date(2026, time.July, 15, 10, 30, 0, 0, time.UTC) // goldenModel is a Model with a frozen clock and a fixed configuration. func goldenModel() Model { - m := New(config.Config{Token: "t", WorkspaceID: "team1", Currency: "EUR", Rate: 50}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "team1", Currency: "EUR", Rate: 50}) m.now = func() time.Time { return goldenFixedTime } m.loc = time.UTC m.year, m.month = 2026, time.July @@ -305,7 +304,7 @@ func TestGoldenRatesTabs(t *testing.T) { t.Run(tc.name, func(t *testing.T) { rt := newRates(goldenEntries(), cfg) rt.sec = tc.sec - golden(t, tc.name, rt.view(testTheme(true))) + golden(t, tc.name, rt.view(testTheme(true), KeyTable{})) }) } } @@ -314,7 +313,7 @@ func TestGoldenLog(t *testing.T) { t.Parallel() lg := newLog(goldenEntries(), config.Config{Token: "t", WorkspaceID: "team1"}) lg.now = goldenFixedTime - golden(t, "log", lg.view(testTheme(true))) + golden(t, "log", lg.view(testTheme(true), KeyTable{})) } // The entries browser has no constructor: it is built inline when 'v' is @@ -343,7 +342,7 @@ func TestGoldenLogForm(t *testing.T) { // depicted a state the app never shows — and left the "Task:" row // unprotected. lg.taskID = "86abc" - golden(t, "log_form", lg.view(testTheme(true))) + golden(t, "log_form", lg.view(testTheme(true), KeyTable{})) } func TestGoldenEntriesEdit(t *testing.T) { diff --git a/internal/tui/helpers_test.go b/internal/tui/helpers_test.go new file mode 100644 index 0000000..0fc3f7c --- /dev/null +++ b/internal/tui/helpers_test.go @@ -0,0 +1,13 @@ +package tui + +import ( + "github.com/marcoarnulfo/clickup-cli/internal/config" + "github.com/marcoarnulfo/clickup-cli/internal/themes" +) + +// testModel builds a Model the way production does, with the built-in palette +// and bindings. It exists so that growing New's signature touches one line +// instead of forty-three. +func testModel(cfg config.Config) Model { + return New(cfg, themes.Default(), DefaultKeyTable()) +} diff --git a/internal/tui/home_test.go b/internal/tui/home_test.go index 210f155..3f1d3f3 100644 --- a/internal/tui/home_test.go +++ b/internal/tui/home_test.go @@ -9,7 +9,6 @@ import ( "github.com/marcoarnulfo/clickup-cli/internal/clickup" "github.com/marcoarnulfo/clickup-cli/internal/config" "github.com/marcoarnulfo/clickup-cli/internal/report" - "github.com/marcoarnulfo/clickup-cli/internal/themes" ) func TestHomeFOpensMembersInTeam(t *testing.T) { @@ -182,7 +181,7 @@ func TestHomeMembersNote(t *testing.T) { } func TestHomeShowsUpdateNotice(t *testing.T) { - m := New(config.Config{Token: "t", WorkspaceID: "1"}, themes.Default()) + m := testModel(config.Config{Token: "t", WorkspaceID: "1"}) m.latestVersion = "v1.8.0" out := m.View() if !strings.Contains(out, "v1.8.0") { diff --git a/internal/tui/inline_keys_test.go b/internal/tui/inline_keys_test.go new file mode 100644 index 0000000..7bbb3e7 --- /dev/null +++ b/internal/tui/inline_keys_test.go @@ -0,0 +1,260 @@ +package tui + +import ( + "strings" + "testing" + "time" + + "github.com/marcoarnulfo/clickup-cli/internal/config" +) + +func mustResolveInlineKeys(t *testing.T, overrides map[string]config.KeySpec) KeyTable { + t.Helper() + kt, err := ResolveKeys(overrides) + if err != nil { + t.Fatal(err) + } + return kt +} + +func TestLogChoiceInstructionsUseLiveKeys(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + step logStep + want string + }{ + { + name: "mode select", + step: logModeSelect, + want: " 1) Guided — pick list and task\n 2) Task ID/URL — straight to the form\n 3) Timer — start/stop stopwatch\n", + }, + { + name: "timer pick", + step: logTimerPick, + want: " 1) Guided (list → task)\n 2) Task ID/URL\n", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := Model{screen: screenLog, theme: testTheme(true), now: time.Now, logScreen: logModel{step: tc.step}} + if got := m.screenBody(); !strings.Contains(got, tc.want) { + t.Errorf("default instructions changed:\n%s", got) + } + + m.keys = mustResolveInlineKeys(t, map[string]config.KeySpec{ + "pick_guided": {"f1"}, + "pick_by_id": {"f2"}, + "pick_timer": {"f3"}, + }) + got := m.screenBody() + for _, want := range []string{" f1)", " f2)"} { + if !strings.Contains(got, want) { + t.Errorf("remapped instructions do not contain %q:\n%s", want, got) + } + } + if tc.step == logModeSelect && !strings.Contains(got, " f3)") { + t.Errorf("remapped instructions do not contain the timer key:\n%s", got) + } + for _, stale := range []string{" 1)", " 2)", " 3)"} { + if strings.Contains(got, stale) { + t.Errorf("remapped instructions still contain %q:\n%s", stale, got) + } + } + }) + } +} + +func TestBillableInstructionsUseLiveKeys(t *testing.T) { + t.Parallel() + models := []struct { + name string + model Model + }{ + { + name: "log form", + model: Model{screen: screenLog, theme: testTheme(true), now: time.Now, logScreen: logModel{step: logForm, formField: 3}}, + }, + { + name: "entries edit", + model: Model{screen: screenEntries, theme: testTheme(true), entriesScreen: entriesModel{mode: entriesEdit, editStep: 4}}, + }, + } + for _, tc := range models { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := tc.model.screenBody(); !strings.Contains(got, "Billable? [Y/n] (Enter = yes)") { + t.Errorf("default billable instruction changed:\n%s", got) + } + + for _, remap := range []struct { + name string + over map[string]config.KeySpec + want string + }{ + {name: "yes", over: map[string]config.KeySpec{"yes": {"f4"}}, want: "Billable? [f4/n/N]"}, + {name: "no", over: map[string]config.KeySpec{"no": {"f5"}}, want: "Billable? [y/Y/enter/f5]"}, + {name: "both", over: map[string]config.KeySpec{"yes": {"f4"}, "no": {"f5"}}, want: "Billable? [f4/f5]"}, + {name: "space", over: map[string]config.KeySpec{"yes": {" "}, "toggle_item": {"f6"}}, want: "Billable? [space/n/N]"}, + } { + t.Run(remap.name, func(t *testing.T) { + m := tc.model + m.keys = mustResolveInlineKeys(t, remap.over) + got := m.screenBody() + if !strings.Contains(got, remap.want) { + t.Errorf("remapped billable instruction is not truthful:\n%s", got) + } + for _, stale := range []string{"[Y/n]", "Enter = yes"} { + if strings.Contains(got, stale) { + t.Errorf("remapped billable instruction still contains %q:\n%s", stale, got) + } + } + }) + } + }) + } +} + +func TestRatesInlineInstructionsUseLiveKeys(t *testing.T) { + t.Parallel() + remapped := mustResolveInlineKeys(t, map[string]config.KeySpec{ + "back": {"f6"}, + "browse_list": {"f7"}, + "clear_value": {"f8"}, + "list_budget": {"f9"}, + "next_section": {"f10"}, + "prev_section": {"f11"}, + }) + + editCases := []struct { + name string + rates ratesModel + placeholder string + }{ + { + name: "list rate", + rates: ratesModel{sec: secLists, rows: []rateRow{{listID: "1"}}}, + placeholder: "new rate (Esc to cancel)", + }, + { + name: "member rate", + rates: ratesModel{sec: secMembers, members: []memberRow{{id: 1}}}, + placeholder: "member rate (Esc to cancel)", + }, + { + name: "existing override rate", + rates: ratesModel{sec: secOverrides, overrides: []overrideRow{{listID: "1", member: 1}}}, + placeholder: "override rate (Esc to cancel)", + }, + { + name: "draft override rate", + rates: ratesModel{ + sec: secOverrides, + rows: []rateRow{{listID: "1"}}, + members: []memberRow{{id: 1}}, + draft: overrideDraft{active: true, step: draftPickMember, listID: "1"}, + }, + placeholder: "override rate (Esc to cancel)", + }, + } + for _, tc := range editCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + open := func(kt KeyTable) string { + m := Model{screen: screenRates, keys: kt, ratesScreen: tc.rates} + next, _ := m.updateRates(keyMsg("enter")) + return next.(Model).ratesScreen.input.Placeholder + } + if got := open(KeyTable{}); got != tc.placeholder { + t.Errorf("default placeholder = %q, want %q", got, tc.placeholder) + } + if got := open(remapped); got != strings.Replace(tc.placeholder, "Esc", "f6", 1) { + t.Errorf("remapped placeholder = %q, want the live back key", got) + } + }) + } + + t.Run("empty draft guidance leads to browse", func(t *testing.T) { + m := Model{screen: screenRates, demo: true, keys: remapped, ratesScreen: ratesModel{sec: secOverrides}} + next, _ := m.updateRates(keyMsg("enter")) + m = next.(Model) + for _, want := range []string{"'f10/f11'", "'f7'"} { + if !strings.Contains(m.ratesScreen.msg, want) { + t.Fatalf("empty-draft guidance does not mention %s: %q", want, m.ratesScreen.msg) + } + } + + // Follow the advertised previous-section binding twice: + // Overrides -> Members -> Lists, where BrowseList is enabled. + for _, want := range []ratesSection{secMembers, secLists} { + next, _ = m.updateRates(keyMsg("f11")) + m = next.(Model) + if m.ratesScreen.sec != want { + t.Fatalf("f11 moved to section %v, want %v", m.ratesScreen.sec, want) + } + } + next, _ = m.updateRates(keyMsg("f7")) + if got := next.(Model).screen; got != screenListBrowser { + t.Errorf("advertised browse route ended on screen %v, want list browser", got) + } + }) + + t.Run("invalid existing rate", func(t *testing.T) { + for _, tc := range []struct { + name string + rt ratesModel + }{ + {name: "list", rt: ratesModel{editing: true, edit: editListRate, rows: []rateRow{{listID: "1"}}, rates: map[string]float64{}}}, + {name: "member", rt: ratesModel{editing: true, edit: editMemberRate, members: []memberRow{{id: 1}}, memberRates: map[int]float64{}}}, + {name: "override", rt: ratesModel{editing: true, edit: editOverrideRate, overrides: []overrideRow{{listID: "1", member: 1}}}}, + } { + t.Run(tc.name, func(t *testing.T) { + got := tc.rt.commit("-1", remapped).msg + for _, want := range []string{"'f6'", "'f8'", "cancel"} { + if !strings.Contains(got, want) { + t.Errorf("invalid existing-rate hint does not mention %q: %q", want, got) + } + } + }) + } + }) + + t.Run("invalid draft rate", func(t *testing.T) { + rt := ratesModel{editing: true, edit: editOverrideRate, draft: overrideDraft{active: true}} + got := rt.commit("-1", remapped).msg + if !strings.Contains(got, "'f6'") || !strings.Contains(got, "cancel") { + t.Errorf("invalid draft-rate hint does not offer live cancellation: %q", got) + } + if strings.Contains(got, "'f8'") || strings.Contains(got, "clear") { + t.Errorf("invalid draft-rate hint advertises clearing a nonexistent value: %q", got) + } + }) + + t.Run("invalid budget stays in the open field", func(t *testing.T) { + rt := ratesModel{editing: true, edit: editListBudget, rows: []rateRow{{listID: "1"}}, budgets: map[string]float64{}} + got := rt.commit("-1", remapped).msg + if !strings.Contains(got, "submit an empty value to remove the budget") { + t.Errorf("invalid-budget hint is not actionable in the open field: %q", got) + } + if strings.Contains(got, "f9") || strings.Contains(got, "press") { + t.Errorf("invalid-budget hint advertises a disabled binding: %q", got) + } + }) + + t.Run("empty report browse hint", func(t *testing.T) { + got := (ratesModel{sec: secLists}).view(testTheme(true), remapped) + if !strings.Contains(got, "press 'f7' to browse") || strings.Contains(got, "press 'b' to browse") { + t.Errorf("empty-report browse hint is not truthful:\n%s", got) + } + }) + + t.Run("always-visible sentence remains unchanged", func(t *testing.T) { + rt := ratesModel{sec: secLists} + const want = "A rate of 0 bills at zero — to unset a value instead, submit an empty field." + for _, kt := range []KeyTable{{}, remapped} { + if got := rt.view(testTheme(true), kt); !strings.Contains(got, want) { + t.Errorf("always-visible rates sentence changed:\n%s", got) + } + } + }) +} diff --git a/internal/tui/keyname.go b/internal/tui/keyname.go new file mode 100644 index 0000000..8ed2333 --- /dev/null +++ b/internal/tui/keyname.go @@ -0,0 +1,51 @@ +package tui + +import ( + "reflect" + "slices" + "strings" + "unicode" +) + +// bindingName turns a keyDefaults field name into the key a user writes in +// their config: LogHours -> log_hours. +// +// An acronym run stays together — PickByID becomes pick_by_id, not +// pick_by_i_d — because the underscore goes in only where a case boundary is a +// word boundary: after a lowercase letter, or before the last capital of a run. +// Measured against every field in keyDefaults; the naive rule produced +// "pick_by_i_d", which would have shipped as a config key. +func bindingName(field string) string { + r := []rune(field) + var b strings.Builder + for i, c := range r { + if !unicode.IsUpper(c) && !unicode.IsTitle(c) { + b.WriteRune(c) + continue + } + prevLower := i > 0 && unicode.IsLower(r[i-1]) + nextLower := i+1 < len(r) && unicode.IsLower(r[i+1]) + if i > 0 && (prevLower || nextLower) { + b.WriteByte('_') + } + b.WriteRune(unicode.ToLower(c)) + } + return b.String() +} + +// BindingNames lists every named binding in keyDefaults, sorted, for error +// messages and for the docs. +// +// Derived rather than written out so that a binding added to keyDefaults is +// remappable with no further work — the maintenance multiplier #82 warned about +// does not exist. The cost is moved instead: renaming a Go field would rename a +// user's config key, which is what TestBindingNamesArePinned exists to catch. +func BindingNames() []string { + ty := reflect.TypeOf(keyDefaults{}) + out := make([]string, 0, ty.NumField()) + for i := range ty.NumField() { + out = append(out, bindingName(ty.Field(i).Name)) + } + slices.Sort(out) + return out +} diff --git a/internal/tui/keyname_test.go b/internal/tui/keyname_test.go new file mode 100644 index 0000000..a63ef70 --- /dev/null +++ b/internal/tui/keyname_test.go @@ -0,0 +1,69 @@ +package tui + +import ( + "slices" + "testing" +) + +func TestBindingName(t *testing.T) { + t.Parallel() + for _, tc := range []struct{ in, want string }{ + {"Quit", "quit"}, + {"LogHours", "log_hours"}, + {"PaletteUp", "palette_up"}, + {"ForceQuit", "force_quit"}, + // An acronym run stays together. The naive rule — underscore before + // every capital — turns this into "pick_by_i_d", which would have + // shipped as a config key. + {"PickByID", "pick_by_id"}, + {"ID", "id"}, + {"HTTPServer", "http_server"}, + } { + if got := bindingName(tc.in); got != tc.want { + t.Errorf("bindingName(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +func TestBindingNameTreatsTitlecaseAsCapital(t *testing.T) { + t.Parallel() + if got, want := bindingName("Log\u01c5name"), "log_\u01c6name"; got != want { + t.Errorf("bindingName with a titlecase rune = %q, want %q", got, want) + } +} + +// This is the test that matters most in this task. The config keys a user +// writes are derived from Go field names, so renaming a field silently renames +// a key in every config file in the wild. Spelling the list out here means the +// rename fails a test instead of failing a user. +func TestBindingNamesArePinned(t *testing.T) { + t.Parallel() + want := []string{ + "back", "browse_list", "budget", "change_range", "clear_value", + "confirm", "confirm_delete", "delete", "down", "edit", + "export", "filters", "force_quit", "generate", "group_by", + "help", "history", "list_budget", "list_currency", "log_hours", + "members", "new_override", "new_tag", "next_field", "next_month", + "next_section", "no", "open_entries", "palette", "palette_down", + "palette_up", "pick_by_id", "pick_guided", "pick_timer", "prev_field", + "prev_month", "prev_section", "quit", "range", "rates", + "reload", "save", "select_all", "stop_timer", "tags", + "timer", "toggle_item", "toggle_scope", "toggle_week", "up", + "yes", + } + got := BindingNames() + if !slices.Equal(got, want) { + t.Errorf("BindingNames() = %v\nwant %v", got, want) + } +} + +func TestBindingNamesAreUnique(t *testing.T) { + t.Parallel() + seen := map[string]bool{} + for _, n := range BindingNames() { + if seen[n] { + t.Errorf("duplicate derived name %q — two fields collapse to the same config key", n) + } + seen[n] = true + } +} diff --git a/internal/tui/keys.go b/internal/tui/keys.go index bcc4cd8..d770b7f 100644 --- a/internal/tui/keys.go +++ b/internal/tui/keys.go @@ -96,9 +96,9 @@ type keyDefaults struct { // Entries' browser actions (entries.go's updateEntries switch, migrated // in Task 4). Delete/Edit/Tags are ownership-gated (canEdit); History is - // not (it's read-only, allowed on any entry). ConfirmDelete (y/Y, no - // enter) is entriesConfirmDelete's own yes, distinct from Yes below - // because that step has no enter-means-yes shortcut (entries.go:260). + // not (it's read-only, allowed on any entry). ConfirmDelete defaults to + // y/Y without enter and is distinct from Yes below because that step has + // no enter-means-yes shortcut (entries.go:260). Delete key.Binding Edit key.Binding History key.Binding @@ -107,9 +107,9 @@ type keyDefaults struct { NewTag key.Binding // Yes/No is the billable-toggle keypress shared verbatim by log.go's - // logForm (formField 3) and entries.go's updateEntriesEdit (editStep 4): - // both switches accept exactly "n"/"N" for no and "y"/"Y"/"enter" for - // yes (log.go:359-362, entries.go:293-296). + // logForm (formField 3) and entries.go's updateEntriesEdit (editStep 4). + // Their defaults are n/N for no and y/Y/enter for yes; both handlers route + // through the resolved bindings (log.go:359-362, entries.go:293-296). Yes key.Binding No key.Binding @@ -192,6 +192,17 @@ func defaultKeys() keyDefaults { } } +// billablePrompt preserves the compact default typography, but expands both +// bindings when either one was remapped so every accepted key stays visible. +func billablePrompt(th theme, kt KeyTable) string { + if !kt.remapped("yes", "no") { + return "Billable? " + th.Accent.Render("[Y/n]") + " (Enter = yes)" + } + yes := keyLabel(kt.keysOf("yes")) + no := keyLabel(kt.keysOf("no")) + return "Billable? " + th.Accent.Render("["+yes+"/"+no+"]") +} + // keyMap is one screen's active bindings, plus the order the footer (#69) // and the ? overlay (#69) will render them in. A zero Binding is disabled // and matches nothing — that is how contextual keys are switched off. @@ -290,7 +301,7 @@ func (k keyMap) allBindings() []key.Binding { // the palette's four keys and empty the list on the first keystroke. func keysFor(m Model) keyMap { if m.overlay == overlayPalette { - return paletteKeys(defaultKeys()) + return paletteKeys(m.keys) } return screenKeys(m) } @@ -302,10 +313,11 @@ func keysFor(m Model) keyMap { // unreachable, so enablement is load-bearing for behavior, not just for // display. func screenKeys(m Model) keyMap { - d := defaultKeys() + kt := m.keys + d := kt.bindings() switch m.screen { case screenHome: - return homeKeys(m, d) + return homeKeys(m, kt) case screenLoading: k := keyMap{Quit: d.Quit, ForceQuit: d.ForceQuit, Help: d.Help} k.short = []key.Binding{k.Help, k.Quit} @@ -318,27 +330,27 @@ func screenKeys(m Model) keyMap { k.short = []key.Binding{k.Quit} return k case screenSetup: - return setupKeys(m, d) + return setupKeys(m, kt) case screenReport: - return reportKeys(d) + return reportKeys(kt) case screenExport: - return exportKeys(d) + return exportKeys(kt) case screenRates: - return ratesKeys(m, d) + return ratesKeys(m, kt) case screenLog: - return logKeys(m, d) + return logKeys(m, kt) case screenMembers: - return membersKeys(d) + return membersKeys(kt) case screenRange: - return rangeKeys(m, d) + return rangeKeys(m, kt) case screenFilters: - return filtersKeys(d) + return filtersKeys(kt) case screenListBrowser: - return listBrowserKeys(d) + return listBrowserKeys(kt) case screenBudget: - return budgetKeys(d) + return budgetKeys(kt) case screenEntries: - return entriesKeys(m, d) + return entriesKeys(m, kt) } return keyMap{} } @@ -354,7 +366,8 @@ func screenKeys(m Model) keyMap { // for all three used to live inline in the handler and now lives here // instead (see TestHomeMembersKeyIsTeamScopeOnly, TestHomeTimerKeyMatchesGuard // and TestHomeMonthNavKeysMatchGuard). -func homeKeys(m Model, d keyDefaults) keyMap { +func homeKeys(m Model, kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{ Quit: d.Quit, ForceQuit: d.ForceQuit, @@ -377,7 +390,7 @@ func homeKeys(m Model, d keyDefaults) keyMap { monthNav := m.preset == report.PresetThisMonth && m.periodMode != periodModeWeek k.PrevMonth.SetEnabled(monthNav) k.NextMonth.SetEnabled(monthNav) - monthPair := pairHelp(k.PrevMonth, k.NextMonth, "◂/▸/h/l", "change month") + monthPair := pairHelp(k.PrevMonth, k.NextMonth, kt.label("◂/▸/h/l", "prev_month", "next_month"), "change month") // Timer is in short, not only in full: the home view renders a live // "running on " line whenever it is enabled, and a status line that // says a timer is running while nothing says how to reach it is worse than @@ -397,13 +410,14 @@ func homeKeys(m Model, d keyDefaults) keyMap { // generic bindings are live. stepToken and stepRate/stepCurrency only ever // react to Confirm (everything else is forwarded to the focused textinput); // stepWorkspace also has a list to move through. -func setupKeys(m Model, d keyDefaults) keyMap { +func setupKeys(m Model, kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{Confirm: d.Confirm, ForceQuit: d.ForceQuit} if m.setup.step == stepWorkspace { k.Up = d.Up k.Down = d.Down k.Help = d.Help - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{pair, k.Confirm, k.Help, k.ForceQuit} k.full = [][]key.Binding{{pair, k.Confirm}, {k.Help, k.ForceQuit}} return k @@ -428,7 +442,8 @@ func setupKeys(m Model, d keyDefaults) keyMap { // handle Back internally with a DIFFERENT destination (one step back within // the flow, not out of it), but the accepted LABEL is identical either way, // which is all this function models. -func logKeys(m Model, d keyDefaults) keyMap { +func logKeys(m Model, kt KeyTable) keyMap { + d := kt.bindings() lg := m.logScreen switch lg.step { case logModeSelect: @@ -445,7 +460,7 @@ func logKeys(m Model, d keyDefaults) keyMap { case logListPick, logTaskPick: k := keyMap{Back: d.Back, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette, Up: d.Up, Down: d.Down, Confirm: d.Confirm} - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{pair, k.Confirm, k.Back, k.Help, k.ForceQuit} k.full = [][]key.Binding{{pair, k.Confirm}, {k.Help, k.Back, k.ForceQuit, k.Palette}} return k @@ -501,7 +516,8 @@ func logKeys(m Model, d keyDefaults) keyMap { // different label set (Yes/No instead of Confirm); entriesTags forwards to // a textinput while tagNewMode is set, same shape as rates'/setup's // free-text steps. -func entriesKeys(m Model, d keyDefaults) keyMap { +func entriesKeys(m Model, kt KeyTable) keyMap { + d := kt.bindings() es := m.entriesScreen switch es.mode { @@ -549,7 +565,7 @@ func entriesKeys(m Model, d keyDefaults) keyMap { NewTag: d.NewTag, Confirm: d.Confirm, Back: d.Back, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette, } - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{k.ToggleItem, k.NewTag, k.Confirm, k.Back, k.Help, k.ForceQuit} k.full = [][]key.Binding{ {pair, k.ToggleItem}, @@ -570,7 +586,7 @@ func entriesKeys(m Model, d keyDefaults) keyMap { k.Edit.SetEnabled(editable) k.Tags.SetEnabled(editable) k.History.SetEnabled(hasEntries) - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{k.Edit, k.Delete, k.Tags, k.History, k.Back, k.Help, k.ForceQuit} k.full = [][]key.Binding{ {pair}, @@ -594,7 +610,8 @@ func entriesKeys(m Model, d keyDefaults) keyMap { // ListCurrency/ListBudget it has no row-count requirement (rates.go:443, // TestRatesBIsGatedToListsSection already pins this asymmetry at the handler // level). ClearValue and Save are unconditional. -func ratesKeys(m Model, d keyDefaults) keyMap { +func ratesKeys(m Model, kt KeyTable) keyMap { + d := kt.bindings() rt := m.ratesScreen if rt.editing { @@ -608,7 +625,7 @@ func ratesKeys(m Model, d keyDefaults) keyMap { } if rt.draft.active { k := keyMap{Up: d.Up, Down: d.Down, Confirm: d.Confirm, Back: d.Back, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette} - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{pair, k.Confirm, k.Back, k.Help, k.ForceQuit} k.full = [][]key.Binding{{pair, k.Confirm}, {k.Help, k.Back, k.ForceQuit, k.Palette}} return k @@ -633,16 +650,16 @@ func ratesKeys(m Model, d keyDefaults) keyMap { // dangerously vague on Overrides, so each section names its own. switch rt.sec { case secLists: - k.ClearValue.SetHelp("d", "use the default rate") - k.ListCurrency.SetHelp("c", "currency (empty clears)") - k.ListBudget.SetHelp("g", "budget (empty clears)") + kt.setHelp(&k.ClearValue, "clear_value", "d", "use the default rate") + kt.setHelp(&k.ListCurrency, "list_currency", "c", "currency (empty clears)") + kt.setHelp(&k.ListBudget, "list_budget", "g", "budget (empty clears)") case secMembers: - k.ClearValue.SetHelp("d", "use the default rate") + kt.setHelp(&k.ClearValue, "clear_value", "d", "use the default rate") case secOverrides: - k.ClearValue.SetHelp("d", "delete this override") + kt.setHelp(&k.ClearValue, "clear_value", "d", "delete this override") } - sectionPair := pairHelp(k.NextSection, k.PrevSection, "tab/⇧tab", "section") - movePair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + sectionPair := pairHelp(k.NextSection, k.PrevSection, kt.label("tab/⇧tab", "next_section", "prev_section"), "section") + movePair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{movePair, k.Confirm, k.Save, k.Back, k.Help, k.ForceQuit} k.full = [][]key.Binding{ {sectionPair, movePair, k.Confirm}, @@ -656,14 +673,15 @@ func ratesKeys(m Model, d keyDefaults) keyMap { // list of presets and the custom-date editor accept different label sets, so // the active mode (rangeScreen.editing) decides which are live. Quit stays // off in both modes (q does not quit this screen today). -func rangeKeys(m Model, d keyDefaults) keyMap { +func rangeKeys(m Model, kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{Confirm: d.Confirm, Back: d.Back, ForceQuit: d.ForceQuit, Palette: d.Palette} if m.rangeScreen.editing { // Both fromInput and toInput are focused textinputs here, so Help // stays unassigned — one keysFor branch covers both fields. k.NextField = d.NextField k.PrevField = d.PrevField - pair := pairHelp(k.NextField, k.PrevField, "tab/⇧tab", "next/prev field") + pair := pairHelp(k.NextField, k.PrevField, kt.label("tab/⇧tab", "next_field", "prev_field"), "next/prev field") k.short = []key.Binding{pair, k.Confirm, k.Back, k.ForceQuit} k.full = [][]key.Binding{{pair, k.Confirm}, {k.Back, k.ForceQuit, k.Palette}} return k @@ -671,7 +689,7 @@ func rangeKeys(m Model, d keyDefaults) keyMap { k.Up = d.Up k.Down = d.Down k.Help = d.Help - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{pair, k.Confirm, k.Back, k.Help, k.ForceQuit} k.full = [][]key.Binding{{pair, k.Confirm}, {k.Help, k.Back, k.ForceQuit, k.Palette}} return k @@ -683,9 +701,10 @@ func rangeKeys(m Model, d keyDefaults) keyMap { // screen is loading the handler's own guard ignores all of them anyway — so // there is no separate loading state to model here. Quit stays off (q does // not quit this screen today). -func listBrowserKeys(d keyDefaults) keyMap { +func listBrowserKeys(kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{Up: d.Up, Down: d.Down, Confirm: d.Confirm, Back: d.Back, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette} - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{pair, k.Confirm, k.Back, k.Help, k.ForceQuit} k.full = [][]key.Binding{{pair, k.Confirm}, {k.Help, k.Back, k.ForceQuit, k.Palette}} return k @@ -694,7 +713,8 @@ func listBrowserKeys(d keyDefaults) keyMap { // reportKeys, exportKeys, membersKeys, filtersKeys and budgetKeys are the // binding sets for their screens (report.go, export.go, members.go, // filters.go, budget.go); q DOES quit these screens today. -func reportKeys(d keyDefaults) keyMap { +func reportKeys(kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{ Quit: d.Quit, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette, Back: d.Back, GroupBy: d.GroupBy, ChangeRange: d.ChangeRange, Reload: d.Reload, Export: d.Export, Rates: d.Rates, LogHours: d.LogHours, Filters: d.Filters, @@ -709,20 +729,22 @@ func reportKeys(d keyDefaults) keyMap { return k } -func exportKeys(d keyDefaults) keyMap { +func exportKeys(kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{Quit: d.Quit, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette, Up: d.Up, Down: d.Down, Confirm: d.Confirm, Back: d.Back} - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{pair, k.Confirm, k.Back, k.Help, k.Quit} k.full = [][]key.Binding{{pair, k.Confirm}, {k.Help, k.Back, k.Quit, k.Palette}} return k } -func membersKeys(d keyDefaults) keyMap { +func membersKeys(kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{ Quit: d.Quit, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette, Up: d.Up, Down: d.Down, ToggleItem: d.ToggleItem, SelectAll: d.SelectAll, Confirm: d.Confirm, Back: d.Back, } - pair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{k.ToggleItem, k.SelectAll, k.Confirm, k.Back, k.Help, k.Quit} k.full = [][]key.Binding{ {pair, k.ToggleItem, k.SelectAll}, @@ -732,7 +754,8 @@ func membersKeys(d keyDefaults) keyMap { return k } -func filtersKeys(d keyDefaults) keyMap { +func filtersKeys(kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{ Quit: d.Quit, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette, NextField: d.NextField, PrevField: d.PrevField, Up: d.Up, Down: d.Down, ToggleItem: d.ToggleItem, SelectAll: d.SelectAll, Confirm: d.Confirm, Back: d.Back, @@ -740,8 +763,8 @@ func filtersKeys(d keyDefaults) keyMap { // "section", not "field": on this screen tab cycles the four filter // sections (filters.go's NextField/PrevField arms move fs.sec), which is // what the hand-written line it replaced said too. - sectionPair := pairHelp(k.NextField, k.PrevField, "tab/⇧tab", "section") - movePair := pairHelp(k.Up, k.Down, "↑/↓/j/k", "move") + sectionPair := pairHelp(k.NextField, k.PrevField, kt.label("tab/⇧tab", "next_field", "prev_field"), "section") + movePair := pairHelp(k.Up, k.Down, kt.label("↑/↓/j/k", "up", "down"), "move") k.short = []key.Binding{sectionPair, k.ToggleItem, k.Confirm, k.Back, k.Help, k.Quit} k.full = [][]key.Binding{ {sectionPair, movePair, k.ToggleItem, k.SelectAll}, @@ -751,11 +774,12 @@ func filtersKeys(d keyDefaults) keyMap { return k } -func budgetKeys(d keyDefaults) keyMap { +func budgetKeys(kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{Quit: d.Quit, ForceQuit: d.ForceQuit, Help: d.Help, Palette: d.Palette, Back: d.Back, Budget: d.Budget} // The default help text ("budgets") describes b from the report, where it // opens this screen. Here the same key closes it. - k.Budget.SetHelp("b", "close budgets") + kt.setHelp(&k.Budget, "budget", "b", "close budgets") k.short = []key.Binding{k.Back, k.Budget, k.Help, k.Quit} k.full = [][]key.Binding{{k.Budget}, {k.Help, k.Back, k.Quit, k.Palette}} return k @@ -768,15 +792,16 @@ func budgetKeys(d keyDefaults) keyMap { // Quit and Help stay unassigned, which is the second, independent guard on top // of Update's ordering: even if the overlay check were moved below them, q and // ? could not fire while the user is typing a query. -func paletteKeys(d keyDefaults) keyMap { +func paletteKeys(kt KeyTable) keyMap { + d := kt.bindings() k := keyMap{ Up: d.PaletteUp, Down: d.PaletteDown, Confirm: d.Confirm, Back: d.Back, Palette: d.Palette, ForceQuit: d.ForceQuit, } - k.Confirm.SetHelp("enter", "run") - k.Back.SetHelp("esc", "close") - pair := pairHelp(k.Up, k.Down, "↑/↓", "move") + kt.setHelp(&k.Confirm, "confirm", "enter", "run") + kt.setHelp(&k.Back, "back", "esc", "close") + pair := pairHelp(k.Up, k.Down, kt.label("↑/↓", "palette_up", "palette_down"), "move") // ctrl+p closes as well as opens, but esc is the close every other screen // already teaches, so only esc is advertised. k.short = []key.Binding{pair, k.Confirm, k.Back, k.ForceQuit} @@ -790,7 +815,7 @@ func paletteKeys(d keyDefaults) keyMap { // bury the ones that are. // // Generate is here despite being Enter rather than a rune: it is the most-used -// action in the program and keyMsgFor rebuilds it exactly. Confirm is not, even +// action in the program and parseKeyName rebuilds it exactly. Confirm is not, even // though it shares that key — the selection is by field, not by keystroke. func (k keyMap) paletteBindings() []key.Binding { return []key.Binding{ diff --git a/internal/tui/keys_test.go b/internal/tui/keys_test.go index 8aff238..0d7437b 100644 --- a/internal/tui/keys_test.go +++ b/internal/tui/keys_test.go @@ -169,7 +169,7 @@ func TestAllBindingsCoversEveryField(t *testing.T) { // letter. paletteKeys uses the arrow-only PaletteUp/PaletteDown defaults. func TestPaletteKeysAreArrowOnly(t *testing.T) { t.Parallel() - k := paletteKeys(defaultKeys()) + k := paletteKeys(DefaultKeyTable()) for _, tc := range []struct { name string binding key.Binding @@ -190,7 +190,7 @@ func TestPaletteKeysAreArrowOnly(t *testing.T) { // false and key.Matches never fires on. func TestPaletteKeysLeaveQuitAndHelpUnassigned(t *testing.T) { t.Parallel() - k := paletteKeys(defaultKeys()) + k := paletteKeys(DefaultKeyTable()) if k.Quit.Enabled() { t.Error("the palette assigned Quit; q would close the program while typing") } @@ -217,10 +217,8 @@ func TestKeysForFollowsTheOverlayAndScreenKeysDoesNot(t *testing.T) { } } -// Every binding the palette offers must be replayable through routeKey, which -// means its first key round-trips through keyMsgFor. Anything else would build -// a KeyMsg whose String() does not match the binding, and the action would -// silently do nothing. +// Every default binding the palette offers must have a key replayable through +// routeKey. ResolveKeys applies the same parser to overrides at startup. func TestEveryPaletteBindingIsReplayable(t *testing.T) { t.Parallel() for _, b := range defaultKeys().paletteDefaults() { @@ -229,8 +227,10 @@ func TestEveryPaletteBindingIsReplayable(t *testing.T) { t.Errorf("a palette default has no keys: %+v", b.Help()) continue } - if _, ok := keyMsgFor(keys[0]); !ok { - t.Errorf("binding %q (%s) has a first key keyMsgFor cannot rebuild", keys[0], b.Help().Desc) + for _, configured := range keys { + if _, err := parseKeyName(configured); err != nil { + t.Errorf("binding %v (%s) has invalid key %q: %v", keys, b.Help().Desc, configured, err) + } } } } diff --git a/internal/tui/keytable.go b/internal/tui/keytable.go new file mode 100644 index 0000000..95b6bd8 --- /dev/null +++ b/internal/tui/keytable.go @@ -0,0 +1,217 @@ +package tui + +import ( + "fmt" + "maps" + "reflect" + "slices" + "strings" + + "github.com/charmbracelet/bubbles/key" + "github.com/marcoarnulfo/clickup-cli/internal/config" +) + +// forceQuitName is the one binding a config may not remap: ctrl+c is the way +// out of a TUI whose other keys the user has moved somewhere unreachable. +const forceQuitName = "force_quit" + +// KeyTable is the resolved binding table the TUI routes and renders with. +// +// The zero value means the built-in defaults, which is what a Model built by +// hand carries — the tests construct 108 of those without going through New, +// and a zero table read literally would leave every binding disabled. +type KeyTable struct { + d keyDefaults + over map[string]bool + set bool +} + +// DefaultKeyTable is the built-in table, for callers that want it explicitly. +func DefaultKeyTable() KeyTable { return KeyTable{d: defaultKeys(), set: true} } + +func (kt KeyTable) bindings() keyDefaults { + if !kt.set { + return defaultKeys() + } + return kt.d +} + +// keysOf returns the keys currently bound to a binding, by config name. +func (kt KeyTable) keysOf(name string) []string { + d := kt.bindings() + v := reflect.ValueOf(d) + ty := v.Type() + for i := range v.NumField() { + if bindingName(ty.Field(i).Name) == name { + return v.Field(i).Interface().(key.Binding).Keys() + } + } + panic(fmt.Sprintf("unknown binding name %q", name)) +} + +func (kt KeyTable) remapped(names ...string) bool { + remapped := false + for _, name := range names { + kt.keysOf(name) // validate internal names even when no override exists + if kt.over[name] { + remapped = true + } + } + return remapped +} + +// keyLabel renders canonical key names without changing the raw strings used +// by key.Matches and collision claims. A literal space is the only canonical +// Bubble Tea name that would otherwise be invisible in generated guidance. +func keyLabel(keys []string) string { + labels := make([]string, len(keys)) + for i, k := range keys { + switch k { + case " ": + labels[i] = "space" + case "alt+ ": + labels[i] = "alt+space" + default: + labels[i] = k + } + } + return strings.Join(labels, "/") +} + +// label returns lit unless one of the named bindings was remapped, in which +// case it derives the label from the keys actually bound. +// +// The literal labels carry typography the defaults earned, such as "↑/↓/j/k" +// and "tab/⇧tab". Deriving them unconditionally would make the default footer +// longer and uglier, so the literal wins until it would be a lie. +func (kt KeyTable) label(lit string, names ...string) string { + if !kt.remapped(names...) { + return lit + } + var keys []string + for _, name := range names { + keys = append(keys, kt.keysOf(name)...) + } + return keyLabel(keys) +} + +// setHelp is label's counterpart for single-binding SetHelp call sites. +func (kt KeyTable) setHelp(b *key.Binding, name, lit, desc string) { + b.SetHelp(kt.label(lit, name), desc) +} + +// claims maps every physical key to the sorted names of the bindings that want +// it. The defaults are deliberately overloaded — measured, 20 keys have more +// than one claimant — because screenKeys enables only a subset per screen. +func claims(d keyDefaults) map[string][]string { + out := map[string][]string{} + v := reflect.ValueOf(d) + ty := v.Type() + for i := range v.NumField() { + b := v.Field(i).Interface().(key.Binding) + name := bindingName(ty.Field(i).Name) + for _, k := range b.Keys() { + out[k] = append(out[k], name) + } + } + for k := range out { + slices.Sort(out[k]) + } + return out +} + +// checkCollisions rejects a post-override key with multiple claimants unless +// all of them already claimed that key in the defaults. +// +// Detecting real conflicts would mean asking, per screen, which bindings are +// enabled at once — 14 screens plus the sub-modes of entries, log, rates and +// setup, a table to keep in sync with every screen ever added. This rule is +// computed from the key table alone, and is deliberately conservative: it +// refuses taking a key that another binding still claims, even when those +// bindings never share a screen. A free key and a clean swap remain allowed. +func checkCollisions(before, after keyDefaults) error { + was, now := claims(before), claims(after) + for _, k := range slices.Sorted(maps.Keys(now)) { + names := now[k] + if len(names) < 2 { + continue + } + for _, n := range names { + if slices.Contains(was[k], n) { + continue + } + others := slices.DeleteFunc(slices.Clone(names), func(s string) bool { return s == n }) + return fmt.Errorf("binding %q cannot take key %q: it is already claimed by %s", + n, k, strings.Join(others, ", ")) + } + } + return nil +} + +// ResolveKeys applies a config's overrides to the built-in table. +// +// Every failure is an error rather than a fallback: a key that cannot be +// honored would leave the user in front of a TUI where a command simply does +// not answer, with nothing to go on. Same rule as billing.rounding.increment +// and as the theme resolution. +func ResolveKeys(overrides map[string]config.KeySpec) (KeyTable, error) { + d := defaultKeys() + v := reflect.ValueOf(&d).Elem() + ty := v.Type() + over := map[string]bool{} + + index := map[string]int{} + for i := range ty.NumField() { + index[bindingName(ty.Field(i).Name)] = i + } + + for _, name := range slices.Sorted(maps.Keys(overrides)) { + if name == forceQuitName { + return KeyTable{}, fmt.Errorf( + "binding %q cannot be remapped: ctrl+c is the way out of a TUI whose other keys have moved", name) + } + i, ok := index[name] + if !ok { + // force_quit is filtered out of the suggestion list: offering the + // one name that is then rejected would be a message that argues + // with itself. + valid := slices.DeleteFunc(BindingNames(), func(s string) bool { return s == forceQuitName }) + return KeyTable{}, fmt.Errorf("unknown binding %q; valid names: %s", + name, strings.Join(valid, ", ")) + } + ks := slices.Clone(overrides[name]) + if len(ks) == 0 { + return KeyTable{}, fmt.Errorf("binding %q needs at least one key", name) + } + seen := map[string]bool{} + for _, k := range ks { + if k == "" { + return KeyTable{}, fmt.Errorf("binding %q has an empty key in its list", name) + } + if _, err := parseKeyName(k); err != nil { + return KeyTable{}, fmt.Errorf("binding %q uses invalid key %q: %w", name, k, err) + } + // Caught here rather than by the collision rule, which would report + // the binding colliding with itself and name no other claimant. + if seen[k] { + return KeyTable{}, fmt.Errorf("binding %q lists key %q twice", name, k) + } + seen[k] = true + } + old := v.Field(i).Interface().(key.Binding) + // The help string carries the key inside it, so it has to be rebuilt or + // the footer would advertise a key that no longer does anything. The + // typographic arrows the defaults use (↑/k, tab/▸) are lost for + // remapped bindings: the user chose these keys, and inventing a + // prettier rendering for them would be guessing. + v.Field(i).Set(reflect.ValueOf(key.NewBinding( + key.WithKeys(ks...), + key.WithHelp(keyLabel(ks), old.Help().Desc), + ))) + over[name] = true + } + if err := checkCollisions(defaultKeys(), d); err != nil { + return KeyTable{}, err + } + return KeyTable{d: d, over: over, set: true}, nil +} diff --git a/internal/tui/keytable_test.go b/internal/tui/keytable_test.go new file mode 100644 index 0000000..954ca59 --- /dev/null +++ b/internal/tui/keytable_test.go @@ -0,0 +1,411 @@ +package tui + +import ( + "fmt" + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" + "github.com/marcoarnulfo/clickup-cli/internal/config" + "github.com/marcoarnulfo/clickup-cli/internal/themes" +) + +// The zero KeyTable is what a Model built by hand in a test carries, and 108 +// such Models exist. It must behave as the built-in defaults. +func TestZeroKeyTableIsTheDefaults(t *testing.T) { + t.Parallel() + var zero KeyTable + if got, want := zero.bindings().Quit.Keys(), defaultKeys().Quit.Keys(); len(got) != len(want) || got[0] != want[0] { + t.Errorf("zero table Quit = %v, want the default %v", got, want) + } +} + +func TestResolveKeysOverridesAndKeepsTheRest(t *testing.T) { + t.Parallel() + kt, err := ResolveKeys(map[string]config.KeySpec{"log_hours": {"L"}}) + if err != nil { + t.Fatal(err) + } + d := kt.bindings() + if got := d.LogHours.Keys(); len(got) != 1 || got[0] != "L" { + t.Errorf("LogHours = %v, want [L]", got) + } + if got, want := d.Quit.Keys(), defaultKeys().Quit.Keys(); got[0] != want[0] { + t.Errorf("Quit = %v, want the untouched default %v", got, want) + } +} + +// The help string carries the key inside it, so an overridden binding that kept +// its old help would lie in the footer and in the ? overlay. +// The keys are chosen so the override stays collision-free once Task 4's rule +// lands: "k" is already Up's own, and "ctrl+u" is claimed by nothing. An +// earlier draft used "w", which toggle_week claims — Task 4 would then have +// rejected it and this test could never have passed. +func TestResolveKeysRegeneratesTheHelp(t *testing.T) { + t.Parallel() + kt, err := ResolveKeys(map[string]config.KeySpec{"up": {"k", "ctrl+u"}}) + if err != nil { + t.Fatal(err) + } + h := kt.bindings().Up.Help() + if h.Key != "k/ctrl+u" { + t.Errorf("help key = %q, want %q", h.Key, "k/ctrl+u") + } + if want := defaultKeys().Up.Help().Desc; h.Desc != want { + t.Errorf("help desc = %q, want the original %q", h.Desc, want) + } +} + +func TestLiteralSpaceOverrideRoutesAndRendersAsSpace(t *testing.T) { + t.Parallel() + kt, err := ResolveKeys(map[string]config.KeySpec{ + "export": {" "}, + "toggle_item": {"f2"}, // vacate ToggleItem's default literal space + }) + if err != nil { + t.Fatal(err) + } + d := kt.bindings() + if got := d.Export.Keys(); len(got) != 1 || got[0] != " " { + t.Fatalf("Export raw keys = %q, want one literal space", got) + } + if got := claims(d)[" "]; len(got) != 1 || got[0] != "export" { + t.Errorf("literal-space claims = %v, want [export]", got) + } + if got := d.Export.Help().Key; got != "space" { + t.Errorf("Export help key = %q, want space", got) + } + if got := kt.label("unused", "export", "toggle_item"); got != "space/f2" { + t.Errorf("derived pair label = %q, want space/f2", got) + } + + m := New(config.Config{Token: "t", WorkspaceID: "team1"}, themes.Default(), kt) + m.screen = screenReport + m.nav = []screen{screenHome} + m.report = goldenReport() + m.width = 140 + + var export action + found := false + for _, a := range screenActions(m) { + if a.label == "Export" { + export, found = a, true + break + } + } + if !found { + t.Fatal("Export disappeared from screenActions") + } + if export.hint != "space" { + t.Errorf("Export palette hint = %q, want space", export.hint) + } + physical := tea.KeyMsg{Type: tea.KeySpace, Runes: []rune{' '}} + viaAction, _ := export.run(m) + viaKey, _ := m.Update(physical) + if got, want := viaAction.(Model).screen, viaKey.(Model).screen; got != screenExport || want != screenExport { + t.Errorf("palette action screen = %v, physical space screen = %v, want %v", got, want, screenExport) + } + + for _, showAll := range []bool{false, true} { + footer := footerView(testTheme(true), m.width, showAll, keysFor(m)) + if !strings.Contains(footer, "space export") { + t.Errorf("showAll=%v footer does not render the space key:\n%s", showAll, footer) + } + } +} + +func TestAltSpaceOverrideRendersAsAltSpace(t *testing.T) { + t.Parallel() + kt, err := ResolveKeys(map[string]config.KeySpec{"export": {"alt+ "}}) + if err != nil { + t.Fatal(err) + } + if got := kt.bindings().Export.Help().Key; got != "alt+space" { + t.Errorf("Export help key = %q, want alt+space", got) + } +} + +// The footer is where a remapped binding would lie: the per-screen builders +// write the key label as a literal, so regenerating it inside ResolveKeys is +// not enough on its own. +func TestFooterShowsRemappedKeys(t *testing.T) { + kt, err := ResolveKeys(map[string]config.KeySpec{"up": {"ctrl+u"}}) + if err != nil { + t.Fatal(err) + } + m := Model{screen: screenExport, keys: kt, width: 120} + + foot := footerView(testTheme(true), m.width, false, keysFor(m)) + if !strings.Contains(foot, "ctrl+u") { + t.Errorf("footer does not advertise the remapped key:\n%s", foot) + } + if strings.Contains(foot, "↑/↓/j/k") { + t.Errorf("footer still shows the default label for a remapped binding:\n%s", foot) + } +} + +// A zero KeyTable means defaults, including the typography in literal labels. +func TestFooterKeepsItsLabelsWhenNothingIsRemapped(t *testing.T) { + m := Model{screen: screenExport, width: 120} + if foot := footerView(testTheme(true), m.width, false, keysFor(m)); !strings.Contains(foot, "↑/↓/j/k") { + t.Errorf("footer lost its default label:\n%s", foot) + } +} + +func TestDerivedLabelPanicsOnUnknownBindingName(t *testing.T) { + t.Parallel() + defer func() { + if recover() == nil { + t.Error("label accepted an unknown binding name; a typo would silently remove keys") + } + }() + DefaultKeyTable().label("typo", "does_not_exist") +} + +func TestRemappedPanicsOnUnknownBindingName(t *testing.T) { + t.Parallel() + kt, err := ResolveKeys(map[string]config.KeySpec{"yes": {"f4"}}) + if err != nil { + t.Fatal(err) + } + defer func() { + if recover() == nil { + t.Error("remapped stopped at an overridden name and did not validate the later typo") + } + }() + kt.remapped("yes", "does_not_exist") +} + +func TestResolveKeysDoesNotAliasConfigInput(t *testing.T) { + t.Parallel() + keys := config.KeySpec{"k", "ctrl+u"} + kt, err := ResolveKeys(map[string]config.KeySpec{"up": keys}) + if err != nil { + t.Fatal(err) + } + + keys[0] = "x" + keys[1] = "ctrl+x" + b := kt.bindings().Up + if got := b.Keys(); len(got) != 2 || got[0] != "k" || got[1] != "ctrl+u" { + t.Errorf("keys after mutating config input = %v, want [k ctrl+u]", got) + } + if got := b.Help().Key; got != "k/ctrl+u" { + t.Errorf("help after mutating config input = %q, want %q", got, "k/ctrl+u") + } +} + +func TestResolveKeysErrors(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + in map[string]config.KeySpec + says []string + }{ + { + name: "unknown binding lists the valid names", + in: map[string]config.KeySpec{"log_hourz": {"L"}}, + says: []string{"log_hourz", "log_hours"}, + }, + { + name: "force_quit is not remappable", + in: map[string]config.KeySpec{"force_quit": {"ctrl+x"}}, + says: []string{"force_quit", "ctrl+c"}, + }, + { + name: "an empty list is rejected", + in: map[string]config.KeySpec{"quit": {}}, + says: []string{"quit", "at least one key"}, + }, + { + name: "an empty key inside the list is rejected", + in: map[string]config.KeySpec{"quit": {"Q", ""}}, + says: []string{"quit", "empty"}, + }, + { + // Without its own check this reaches the collision rule, which + // reports "already claimed by " with an empty list — the only + // other claimant being the binding itself. + name: "the same key twice in one list is rejected", + in: map[string]config.KeySpec{"quit": {"Q", "Q"}}, + says: []string{"quit", "\"Q\"", "twice"}, + }, + } { + _, err := ResolveKeys(tc.in) + if err == nil { + t.Errorf("%s: ResolveKeys = nil error, want one", tc.name) + continue + } + for _, s := range tc.says { + if !strings.Contains(err.Error(), s) { + t.Errorf("%s: error %q does not mention %q", tc.name, err, s) + } + } + } +} + +func TestResolveKeysRejectsUnreachableKeyNamesAtStartup(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + key string + canonical string + }{ + {key: "g g"}, + {key: "f21"}, + {key: "space", canonical: " "}, + {key: "ctrl+i", canonical: "tab"}, + {key: "ctrl+m", canonical: "enter"}, + {key: "ctrl+[", canonical: "esc"}, + } { + t.Run(tc.key, func(t *testing.T) { + t.Parallel() + _, err := ResolveKeys(map[string]config.KeySpec{"export": {tc.key}}) + if err == nil { + t.Fatal("ResolveKeys = nil error, want one") + } + wants := []string{`"export"`, fmt.Sprintf("%q", tc.key)} + if tc.canonical != "" { + wants = append(wants, fmt.Sprintf("%q", tc.canonical)) + } + for _, want := range wants { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %s", err, want) + } + } + }) + } +} + +// ctrl+c must survive a config that remaps everything else: it is the only way +// out of a TUI whose Quit the user has moved somewhere they cannot reach. +// +// Each binding gets a distinct private-use Unicode rune so the sweep is both +// canonically producible as one KeyMsg and collision-free. A sweep built from, +// say, "ctrl+"+name[:1] would collide (back, budget and browse_list would all +// want ctrl+b) and this test would quietly turn into one that skips forever. +func TestForceQuitSurvivesEveryOverride(t *testing.T) { + t.Parallel() + over := map[string]config.KeySpec{} + for i, n := range BindingNames() { + if n == forceQuitName { + continue + } + over[n] = config.KeySpec{string(rune(0xE000 + i))} + } + kt, err := ResolveKeys(over) + if err != nil { + t.Fatalf("a collision-free sweep was rejected: %v", err) + } + if got, want := kt.bindings().ForceQuit.Keys(), defaultKeys().ForceQuit.Keys(); got[0] != want[0] { + t.Errorf("ForceQuit = %v, want %v", got, want) + } + // And the sweep really did land, so the assertion above is not vacuous. + if got := kt.bindings().Quit.Keys(); len(got) != 1 || got[0] == "q" { + t.Errorf("Quit = %v, want the swept key — the sweep did not take effect", got) + } +} + +// The defaults are heavily overloaded on purpose — measured, 20 physical keys +// are claimed by more than one binding, because screenKeys enables only a +// subset per screen. A rule that forbade any sharing would reject what we +// ship, so the rule is narrower: when a post-override key has multiple +// claimants, they must all have claimed it before. +func TestCollisionRule(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + in map[string]config.KeySpec + ok bool + says []string + }{ + { + // n is shared by log_hours, new_override, new_tag and no. Moving + // log_hours away leaves a subset behind, which is fine. + name: "moving a binding off a shared key is allowed", + in: map[string]config.KeySpec{"log_hours": {"L"}}, + ok: true, + }, + { + name: "taking a free key is allowed", + in: map[string]config.KeySpec{"export": {"ctrl+e"}}, + ok: true, + }, + { + name: "adding a claimant to a contested key is rejected", + in: map[string]config.KeySpec{"export": {"n"}}, + says: []string{"\"n\"", "export"}, + }, + { + name: "adding a claimant to a singly claimed key is rejected", + in: map[string]config.KeySpec{"export": {"q"}}, + says: []string{"\"q\"", "export", "quit"}, + }, + { + // A clean swap IS allowed, and this case exists to keep anyone + // from "fixing" the rule into rejecting it. quit owns "q" and + // reload owns "r" outright, so trading them leaves one claimant + // on each key. Measured against the real defaults. + name: "a clean swap between two bindings is allowed", + in: map[string]config.KeySpec{"quit": {"r"}, "reload": {"q"}}, + ok: true, + }, + { + // This is the declared cost from the design doc's section 2.2, and + // it is NOT "swaps are rejected": it is that taking a key someone + // else still claims is rejected even when the two never share a + // screen. export lives on the report, list_budget on the rates + // screen, and the rule cannot know that. The user picks another + // key; the alternative is a table of every screen state, kept in + // sync forever. + name: "taking a key another binding still claims is rejected — the declared cost", + in: map[string]config.KeySpec{"export": {"g"}}, + says: []string{"export", "\"g\"", "group_by", "list_budget"}, + }, + } { + _, err := ResolveKeys(tc.in) + if tc.ok { + if err != nil { + t.Errorf("%s: ResolveKeys = %v, want nil", tc.name, err) + } + continue + } + if err == nil { + t.Errorf("%s: ResolveKeys = nil error, want one", tc.name) + continue + } + for _, s := range tc.says { + if !strings.Contains(err.Error(), s) { + t.Errorf("%s: error %q does not mention %s", tc.name, err, s) + } + } + } +} + +// The rule must never reject the table we ship. +func TestDefaultsPassTheCollisionRule(t *testing.T) { + t.Parallel() + if _, err := ResolveKeys(nil); err != nil { + t.Fatalf("the built-in defaults do not satisfy the collision rule: %v", err) + } +} + +// The end-to-end guard for the zero-means-defaults decision: an override has to +// survive all the way into Update's routing. If cli ever stopped passing the +// table, the zero value would quietly fall back to the defaults and only this +// test would notice. +func TestAnOverrideReachesUpdate(t *testing.T) { + kt, err := ResolveKeys(map[string]config.KeySpec{"log_hours": {"L"}}) + if err != nil { + t.Fatal(err) + } + m := New(config.Config{Token: "t", WorkspaceID: "team1"}, themes.Default(), kt) + m.screen = screenReport + m.nav = []screen{screenHome} + + if got, _ := m.Update(keyMsg("L")); got.(Model).screen != screenLog { + t.Errorf("L did not open the log screen; screen = %v", got.(Model).screen) + } + if got, _ := m.Update(keyMsg("n")); got.(Model).screen == screenLog { + t.Error("n still opens the log screen, so the override did not take effect") + } +} diff --git a/internal/tui/log.go b/internal/tui/log.go index 693f24c..7edc008 100644 --- a/internal/tui/log.go +++ b/internal/tui/log.go @@ -76,7 +76,7 @@ type logModel struct { } // newLog builds the screen from the known lists (entries ∪ config.Rates), -// in deterministic order for a stable view. Esc/done return to whoever pushed +// in deterministic order for a stable view. Back/done return to whoever pushed // screenLog via pop() (Home or Report) — see Model.nav. func newLog(entries []report.TimeEntry, cfg config.Config) logModel { names := map[string]string{} @@ -310,7 +310,7 @@ func (m Model) updateLog(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m = m.replace(screenLoading) return m, m.timerStopCmd() } - // Esc is caught by the outer guard above and pops back to whoever + // Back is caught by the outer guard above and pops back to whoever // pushed the log flow; the case that used to live here hardcoded // screenReport instead, which was unreachable dead code (the guard // always ran first) AND a latent wrong-destination bug (it would @@ -490,18 +490,18 @@ func (m Model) updateLog(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } -func (lg logModel) view(th theme) string { +func (lg logModel) view(th theme, kt KeyTable) string { b := th.Title.Render("Log hours") + "\n\n" switch lg.step { case logModeSelect: b += "Choose the mode:\n\n" - b += " " + th.Accent.Render("1") + ") Guided — pick list and task\n" - b += " " + th.Accent.Render("2") + ") Task ID/URL — straight to the form\n" - b += " " + th.Accent.Render("3") + ") Timer — start/stop stopwatch\n" + b += " " + th.Accent.Render(kt.label("1", "pick_guided")) + ") Guided — pick list and task\n" + b += " " + th.Accent.Render(kt.label("2", "pick_by_id")) + ") Task ID/URL — straight to the form\n" + b += " " + th.Accent.Render(kt.label("3", "pick_timer")) + ") Timer — start/stop stopwatch\n" case logTimerPick: b += "Timer — how do you pick the task?\n\n" - b += " " + th.Accent.Render("1") + ") Guided (list → task)\n" - b += " " + th.Accent.Render("2") + ") Task ID/URL\n" + b += " " + th.Accent.Render(kt.label("1", "pick_guided")) + ") Guided (list → task)\n" + b += " " + th.Accent.Render(kt.label("2", "pick_by_id")) + ") Task ID/URL\n" case logTimerRunning: if lg.timer == nil { b += th.Help.Render("No timer running.") + "\n" @@ -552,7 +552,7 @@ func (lg logModel) view(th theme) string { case logForm: b += "Task: " + th.Accent.Render(lg.taskID) + "\n\n" if lg.formField == 3 { - b += "Billable? " + th.Accent.Render("[Y/n]") + " (Enter = yes)" + b += billablePrompt(th, kt) } else { labels := []string{"Duration", "Date", "Note (optional)"} b += labels[lg.formField] + ":\n\n" + lg.input.View() diff --git a/internal/tui/log_test.go b/internal/tui/log_test.go index b3a4533..d6a17c1 100644 --- a/internal/tui/log_test.go +++ b/internal/tui/log_test.go @@ -12,7 +12,6 @@ import ( "github.com/marcoarnulfo/clickup-cli/internal/config" "github.com/marcoarnulfo/clickup-cli/internal/report" "github.com/marcoarnulfo/clickup-cli/internal/service" - "github.com/marcoarnulfo/clickup-cli/internal/themes" ) func keyMsg(s string) tea.KeyMsg { @@ -31,12 +30,12 @@ func keyMsg(s string) tea.KeyMsg { // timer indicator and its 'c' key handling. func newTestModel() Model { cfg := config.Config{Token: "t", WorkspaceID: "team1", Currency: "EUR", Rate: 40} - return New(cfg, themes.Default()) + return testModel(cfg) } func newTestModelOnReport() Model { cfg := config.Config{Token: "t", WorkspaceID: "team1", Currency: "EUR", Rate: 40} - m := New(cfg, themes.Default()) + m := testModel(cfg) m.screen = screenReport // Report is only ever reached from Home: seed that parent so pop() // (esc/m/s and everything built on top of this fixture) lands where the diff --git a/internal/tui/mouse.go b/internal/tui/mouse.go index ee3b935..9fcba6b 100644 --- a/internal/tui/mouse.go +++ b/internal/tui/mouse.go @@ -11,9 +11,9 @@ import tea "github.com/charmbracelet/bubbletea" // and the tests that cover it — apply unchanged. actions.go does the same for // the command palette. // -// It deliberately does not go through keyMsgFor: that function's key set is -// closed on purpose for the palette's benefit (see its doc comment), and -// widening it for a different caller would betray that. +// It deliberately does not go through parseKeyName: wheel translation has two +// fixed internal outputs rather than a user-written name to validate, so typed +// KeyMsgs state that invariant directly. // // There is no check on msg.Action, and it would be dead code: measured against // bubbletea v1.3.10, a wheel notch always arrives as a single press. The SGR diff --git a/internal/tui/palette_demo_test.go b/internal/tui/palette_demo_test.go index 5af6a65..9d98a16 100644 --- a/internal/tui/palette_demo_test.go +++ b/internal/tui/palette_demo_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/marcoarnulfo/clickup-cli/internal/clickup" - "github.com/marcoarnulfo/clickup-cli/internal/themes" ) // Demo mode must offer the same palette, and open it without any I/O: the demo @@ -15,7 +14,7 @@ import ( // from racing TestMain's os.Unsetenv of the same variable. func TestPaletteWorksInDemoMode(t *testing.T) { t.Setenv("CLICKUP_DEMO", "1") - m := New(demoConfig(), themes.Default()) + m := testModel(demoConfig()) m.entries = goldenEntries() // A non-empty Status is load-bearing for the "Go to filters" assertion // below: demoStatusEnrichCmd echoes each entry's own Status, and diff --git a/internal/tui/rates.go b/internal/tui/rates.go index d4de7b4..bdb57d7 100644 --- a/internal/tui/rates.go +++ b/internal/tui/rates.go @@ -286,10 +286,9 @@ func sortOverrides(o []overrideRow) { } // validRate accepts a finite number >= 0: zero is a deliberate billing outcome -// ("this list/member bills at zero"), distinct from clearing the value with -// 'd' (which falls back to the inherited rate). Only a negative number is -// rejected. The decimal comma is accepted as well as the dot (handy for the -// Italian keyboard). +// ("this list/member bills at zero"), distinct from using ClearValue to fall +// back to the inherited rate. Only a negative number is rejected. The decimal +// comma is accepted as well as the dot (handy for the Italian keyboard). func validRate(s string) (float64, bool) { s = strings.ReplaceAll(s, ",", ".") f, err := strconv.ParseFloat(s, 64) @@ -302,7 +301,7 @@ func validRate(s string) (float64, bool) { // validBudget accepts only a finite number > 0. Unlike a rate, a budget of // zero is not a meaningful value — the burn-down bar would divide by zero or // show a meaningless 0% — so it stays rejected; to remove a budget, reopen its -// field ('g') and submit an empty value. +// field and submit an empty value. func validBudget(s string) (float64, bool) { s = strings.ReplaceAll(s, ",", ".") f, err := strconv.ParseFloat(s, 64) @@ -410,7 +409,7 @@ func (m Model) updateRates(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m.updateRatesEditing(rt, msg) } if rt.draft.active { - m.ratesScreen = rt.updateDraft(msg, k) + m.ratesScreen = rt.updateDraft(msg, k, m.keys) return m, nil } @@ -426,7 +425,7 @@ func (m Model) updateRates(msg tea.KeyMsg) (tea.Model, tea.Cmd) { case key.Matches(msg, k.Down): rt = rt.move(+1) case key.Matches(msg, k.Confirm): - rt = rt.startEdit() + rt = rt.startEdit(m.keys) case key.Matches(msg, k.ListCurrency): rt.editing, rt.edit, rt.msg = true, editListCurrency, "" rt.input = newTextInput("currency (e.g. EUR) — empty: use the default") @@ -434,7 +433,7 @@ func (m Model) updateRates(msg tea.KeyMsg) (tea.Model, tea.Cmd) { rt.editing, rt.edit, rt.msg = true, editListBudget, "" rt.input = newNumberInput("budget amount — empty: no budget") case key.Matches(msg, k.NewOverride): - rt = rt.startDraft() + rt = rt.startDraft(m.keys) case key.Matches(msg, k.ClearValue): rt = rt.clearSelected() case key.Matches(msg, k.BrowseList): @@ -456,7 +455,7 @@ func (m Model) updateRatesEditing(rt ratesModel, msg tea.KeyMsg) (tea.Model, tea k := keysFor(m) switch { case key.Matches(msg, k.Confirm): - rt = rt.commit(strings.TrimSpace(rt.input.Value())) + rt = rt.commit(strings.TrimSpace(rt.input.Value()), m.keys) m.ratesScreen = rt return m, nil case key.Matches(msg, k.Back): @@ -483,10 +482,10 @@ func (m Model) updateRatesEditing(rt ratesModel, msg tea.KeyMsg) (tea.Model, tea return m, cmd } -// updateDraft handles the list/member pickers of a new override. It takes -// the keyMap as a parameter (rather than calling keysFor itself) because its -// receiver is ratesModel, not Model — keysFor needs the whole root Model. -func (rt ratesModel) updateDraft(msg tea.KeyMsg, k keyMap) ratesModel { +// updateDraft handles the list/member pickers of a new override. It takes the +// current keyMap for routing and KeyTable for inline labels because its receiver +// is ratesModel, not Model — keysFor needs the whole root Model. +func (rt ratesModel) updateDraft(msg tea.KeyMsg, k keyMap, kt KeyTable) ratesModel { n := len(rt.rows) if rt.draft.step == draftPickMember { n = len(rt.members) @@ -509,7 +508,7 @@ func (rt ratesModel) updateDraft(msg tea.KeyMsg, k keyMap) ratesModel { rt.draft.member = rt.members[rt.draft.idx].id rt.draft.step = draftRate rt.editing, rt.edit, rt.msg = true, editOverrideRate, "" - rt.input = newNumberInput("override rate (Esc to cancel)") + rt.input = newNumberInput(fmt.Sprintf("override rate (%s to cancel)", kt.label("Esc", "back"))) case key.Matches(msg, k.Back): rt.draft = overrideDraft{} } @@ -517,9 +516,11 @@ func (rt ratesModel) updateDraft(msg tea.KeyMsg, k keyMap) ratesModel { } // startDraft begins a new (list,member) override. -func (rt ratesModel) startDraft() ratesModel { +func (rt ratesModel) startDraft(kt KeyTable) ratesModel { if len(rt.rows) == 0 || len(rt.members) == 0 { - rt.msg = "No lists or members known yet: browse a list ('b') or run a team report first" + sectionKeys := kt.label("tab/⇧tab", "next_section", "prev_section") + browseKey := kt.label("b", "browse_list") + rt.msg = fmt.Sprintf("No lists or members known yet: switch to Lists with '%s', then press '%s' to browse the workspace, or run a team report first", sectionKeys, browseKey) return rt } rt.draft = overrideDraft{active: true, step: draftPickList} @@ -557,27 +558,28 @@ func (rt ratesModel) move(delta int) ratesModel { } // startEdit opens the input (or applies the toggle) for the selected row. -func (rt ratesModel) startEdit() ratesModel { +func (rt ratesModel) startEdit(kt KeyTable) ratesModel { rt.msg = "" + cancelKey := kt.label("Esc", "back") switch rt.sec { case secLists: if len(rt.rows) == 0 { return rt } rt.editing, rt.edit = true, editListRate - rt.input = newNumberInput("new rate (Esc to cancel)") + rt.input = newNumberInput(fmt.Sprintf("new rate (%s to cancel)", cancelKey)) case secMembers: if len(rt.members) == 0 { return rt } rt.editing, rt.edit = true, editMemberRate - rt.input = newNumberInput("member rate (Esc to cancel)") + rt.input = newNumberInput(fmt.Sprintf("member rate (%s to cancel)", cancelKey)) case secOverrides: if rt.sel[secOverrides] >= len(rt.overrides) { - return rt.startDraft() + return rt.startDraft(kt) } rt.editing, rt.edit = true, editOverrideRate - rt.input = newNumberInput("override rate (Esc to cancel)") + rt.input = newNumberInput(fmt.Sprintf("override rate (%s to cancel)", cancelKey)) case secRules: switch rt.sel[secRules] { case ruleDefaultCurrency: @@ -608,21 +610,24 @@ func (rt ratesModel) startEdit() ratesModel { // commit validates and applies the typed value. On a rejected value the field // stays open with an inline message, so nothing else the user has edited is lost. -func (rt ratesModel) commit(v string) ratesModel { +func (rt ratesModel) commit(v string, kt KeyTable) ratesModel { done := func() ratesModel { rt.editing, rt.edit, rt.msg = false, editNone, "" return rt } - const ( - badRate = "Invalid rate: enter a number >= 0 (0 bills at zero; 'd' clears the override so the inherited rate applies)" - badBudget = "Invalid budget: enter an amount > 0 (press 'g' and submit an empty value to remove the budget)" - badCurrency = "Invalid currency: use a 3-letter ISO code like EUR (submit an empty value to clear)" - ) + backKey := kt.label("Esc", "back") + clearKey := kt.label("d", "clear_value") + badRate := fmt.Sprintf("Invalid rate: enter a number >= 0 (0 bills at zero; press '%s' to cancel, then '%s' to clear the existing value)", backKey, clearKey) + if rt.edit == editOverrideRate && rt.draft.active { + badRate = fmt.Sprintf("Invalid rate: enter a number >= 0 (0 bills at zero; press '%s' to cancel this new override)", backKey) + } + const badBudget = "Invalid budget: enter an amount > 0, or submit an empty value to remove the budget" + const badCurrency = "Invalid currency: use a 3-letter ISO code like EUR (submit an empty value to clear)" switch rt.edit { case editListRate: if v == "" { - return done() // empty = no change (to clear an override, use 'd') + return done() // empty = no change; ClearValue removes the rate } f, ok := validRate(v) if !ok { @@ -662,7 +667,7 @@ func (rt ratesModel) commit(v string) ratesModel { case editMemberRate: if v == "" { - return done() // empty = no change (to clear, use 'd') + return done() // empty = no change; ClearValue removes the rate } f, ok := validRate(v) if !ok { @@ -744,10 +749,10 @@ func indexOfOverride(list []overrideRow, listID string, member int) int { return 0 } -// clearSelected ('d') removes the value of the selected row, reverting it to -// the next level of the precedence (or to "not configured" for the rules). In -// the Lists section it clears the *rate* only: a per-list currency or budget is -// cleared by reopening its own field ('c'/'g') and submitting an empty value. +// clearSelected removes the value of the selected row, reverting it to the next +// level of the precedence (or to "not configured" for the rules). In the Lists +// section it clears the rate only: a per-list currency or budget is cleared by +// reopening its own field and submitting an empty value. func (rt ratesModel) clearSelected() ratesModel { rt.msg = "" switch rt.sec { @@ -853,7 +858,7 @@ func (m Model) saveRates(rt ratesModel) (tea.Model, tea.Cmd) { // ------------------------------------------------------------------- view -- -func (rt ratesModel) view(th theme) string { +func (rt ratesModel) view(th theme, kt KeyTable) string { b := th.Title.Render("Billing settings") + "\n" + rt.tabs(th) + "\n\n" if rt.draft.active && rt.draft.step != draftRate { @@ -861,7 +866,7 @@ func (rt ratesModel) view(th theme) string { } else { switch rt.sec { case secLists: - b += rt.listsView(th) + b += rt.listsView(th, kt) case secMembers: b += rt.membersView(th) case secOverrides: diff --git a/internal/tui/rates_test.go b/internal/tui/rates_test.go index 1eabeb8..4b6e14a 100644 --- a/internal/tui/rates_test.go +++ b/internal/tui/rates_test.go @@ -252,21 +252,21 @@ func TestRatesReentryShowsPersistedValues(t *testing.T) { t.Fatalf("'p' should reopen the billing editor, got %v", m.screen) } - lists := m.ratesScreen.view(testTheme(true)) + lists := m.ratesScreen.view(testTheme(true), m.keys) for _, want := range []string{"45.00", "USD", "2000.00"} { if !strings.Contains(lists, want) { t.Errorf("Lists section should show %q, got:\n%s", want, lists) } } - members := press(t, m, "tab").ratesScreen.view(testTheme(true)) + members := press(t, m, "tab").ratesScreen.view(testTheme(true), m.keys) if !strings.Contains(members, "50.00") { t.Errorf("Members section should show the persisted member rate, got:\n%s", members) } - overrides := press(t, m, "tab", "tab").ratesScreen.view(testTheme(true)) + overrides := press(t, m, "tab", "tab").ratesScreen.view(testTheme(true), m.keys) if !strings.Contains(overrides, "60.00") { t.Errorf("Overrides section should show the persisted override, got:\n%s", overrides) } - rules := press(t, m, "tab", "tab", "tab").ratesScreen.view(testTheme(true)) + rules := press(t, m, "tab", "tab", "tab").ratesScreen.view(testTheme(true), m.keys) for _, want := range []string{"CHF", "15m", "up", "day", "Europe/Rome"} { if !strings.Contains(rules, want) { t.Errorf("Rules section should show %q, got:\n%s", want, rules) @@ -331,7 +331,7 @@ func TestRatesRejectsNegativeRateInline(t *testing.T) { m = press(t, m, "tab") // Members m = press(t, m, "enter") // edit rate rt := m.ratesScreen - rt = rt.commit("-5") // '-' cannot be typed (numericRune filters it); exercise the validator directly + rt = rt.commit("-5", m.keys) // '-' cannot be typed (numericRune filters it); exercise the validator directly m.ratesScreen = rt if rt.msg == "" { @@ -373,8 +373,8 @@ func TestRatesZeroListRatePersists(t *testing.T) { if v, ok := m.ratesScreen.rates["1"]; !ok || v != 0 { t.Fatalf("a zero list rate must survive re-entry, got %v (ok=%v)", v, ok) } - if !strings.Contains(m.ratesScreen.view(testTheme(true)), "0.00") { - t.Fatalf("the Lists view should show the persisted zero rate, got:\n%s", m.ratesScreen.view(testTheme(true))) + if !strings.Contains(m.ratesScreen.view(testTheme(true), m.keys), "0.00") { + t.Fatalf("the Lists view should show the persisted zero rate, got:\n%s", m.ratesScreen.view(testTheme(true), m.keys)) } } @@ -468,7 +468,7 @@ func TestRatesOverrideShowsWhatItOverrides(t *testing.T) { cfg.Billing.RatesByMember = map[int]float64{8: 50} cfg.Billing.RateOverrides = []config.Override{{List: "1", Member: 8, Rate: 60}} m := billingFixture(t, cfg) - v := press(t, m, "tab", "tab").ratesScreen.view(testTheme(true)) + v := press(t, m, "tab", "tab").ratesScreen.view(testTheme(true), m.keys) if !strings.Contains(v, "60.00") || !strings.Contains(v, "50.00") { t.Fatalf("the Overrides section must show the override and the rate it wins over, got:\n%s", v) } @@ -550,7 +550,7 @@ func TestRatesListsViewKeepsRowWidthWithWideRuneCurrency(t *testing.T) { // One rune, two display columns — reachable only via a config file, never // via the in-app editor, which validCurrency would reject. rt.currencies["1"] = "円" - got := rt.listsView(testTheme(true)) + got := rt.listsView(testTheme(true), KeyTable{}) lines := strings.Split(got, "\n") // lines[0] is the header; lines[1] is "Website" (listID "1", the @@ -638,7 +638,7 @@ func TestRatesDeleteOverride(t *testing.T) { t.Fatalf("the deletion must be persisted, got %v", m.cfg.Billing.RateOverrides) } // The selection landed on the trailing "new override" row and still works. - if v := m.ratesScreen.view(testTheme(true)); !strings.Contains(v, "new (list,member) override") { + if v := m.ratesScreen.view(testTheme(true), m.keys); !strings.Contains(v, "new (list,member) override") { t.Fatalf("the new-override row should still be offered, got:\n%s", v) } } diff --git a/internal/tui/rates_view.go b/internal/tui/rates_view.go index 31035c2..6e1964b 100644 --- a/internal/tui/rates_view.go +++ b/internal/tui/rates_view.go @@ -38,7 +38,7 @@ func billingRow(th theme, sel bool, line string) string { return " " + line + "\n" } -func (rt ratesModel) listsView(th theme) string { +func (rt ratesModel) listsView(th theme, kt KeyTable) string { // "List", "Rate", "Cur", "Budget" and "Source" are ASCII literals, where // runes and columns coincide by construction, so the %-Ns verbs in the // header stay aligned with the data rows' cell(...,N) columns below. @@ -52,7 +52,7 @@ func (rt ratesModel) listsView(th theme) string { // same fix this whole file's non-ASCII columns already use. b := th.Help.Render(fmt.Sprintf(" %-24s %10s %-5s %10s %s", "List", "Rate", "Cur", "Budget", "Source")) + "\n" if len(rt.rows) == 0 { - return b + th.Help.Render(" No lists in the current report — press 'b' to browse the workspace.") + "\n" + return b + th.Help.Render(fmt.Sprintf(" No lists in the current report — press '%s' to browse the workspace.", kt.label("b", "browse_list"))) + "\n" } for i, r := range rt.rows { rate, tag := rt.def, "default" diff --git a/internal/tui/report_test.go b/internal/tui/report_test.go index f6ea2b7..d964b77 100644 --- a/internal/tui/report_test.go +++ b/internal/tui/report_test.go @@ -11,7 +11,6 @@ import ( "github.com/marcoarnulfo/clickup-cli/internal/clickup" "github.com/marcoarnulfo/clickup-cli/internal/config" "github.com/marcoarnulfo/clickup-cli/internal/report" - "github.com/marcoarnulfo/clickup-cli/internal/themes" ) // TestReportKeyLabels pins the exact label set report.go's updateReport @@ -160,7 +159,7 @@ func TestReportViewEmptyReportShowsNoHoursAndSplit(t *testing.T) { func TestReportViewShowsSummaryAndBillableSplit(t *testing.T) { cfg := config.Config{Token: "t", WorkspaceID: "1", Rate: 10, Currency: "EUR"} cfg.Billing.Currencies = map[string]string{"eur-list": "EUR", "usd-list": "USD"} - m := New(cfg, themes.Default()) + m := testModel(cfg) m.year, m.month = 2026, 7 start := time.Date(2026, time.July, 1, 9, 0, 0, 0, time.UTC) entries := []report.TimeEntry{ @@ -219,7 +218,7 @@ func TestReportBOpensBudgetView(t *testing.T) { cfg := config.Config{Token: "t", WorkspaceID: "1", Currency: "EUR", Rate: 50} cfg.Billing.Budgets = map[string]float64{"list-1": 100} cfg.Billing.Currencies = map[string]string{"list-1": "EUR"} - m := New(cfg, themes.Default()) + m := testModel(cfg) m.screen = screenReport m.entries = []report.TimeEntry{ {ListID: "list-1", ListName: "Website", Start: time.Date(2026, time.July, 1, 9, 0, 0, 0, time.UTC),