diff --git a/bin/opskit-go.py b/bin/opskit-go.py index 0df315c..1916e25 100644 --- a/bin/opskit-go.py +++ b/bin/opskit-go.py @@ -180,6 +180,50 @@ def _git_info(path): return "unknown", "unknown" +PICKER_NAME_W = 28 +PICKER_BRANCH_W = 18 +PICKER_STATUS_W = 8 + + +def _pad(value, width): + """Left-justify to a fixed width, truncating long values with an ellipsis.""" + if len(value) > width: + value = value[: width - 1] + "…" + return f"{value:<{width}}" + + +def _picker_header(): + return ( + f"{'NAME':<{PICKER_NAME_W}} {'BRANCH':<{PICKER_BRANCH_W}} " + f"{'STATUS':<{PICKER_STATUS_W}} RESUME" + ) + + +def _picker_row(proj, git_lookup): + """One fixed-width display row: NAME | BRANCH | STATUS | RESUME.""" + name = proj.get("rel") or proj.get("name", "unknown") + path = proj.get("path", "") + branch, status = git_lookup(path) if path else ("unknown", "unknown") + resume = "[resume]" if proj.get("has_resume") else "" + return ( + f"{_pad(name, PICKER_NAME_W)} {_pad(branch, PICKER_BRANCH_W)} " + f"{_pad(status, PICKER_STATUS_W)} {resume}" + ).rstrip() + + +def build_picker(projects, git_lookup=_git_info): + """Return (header, lines) for the fzf picker. + + Each line is ``"\\t"``: field 1 is the selection value + (returned via fzf ``--accept-nth=1``), field 2 is the aligned display. + """ + lines = [ + f"{proj.get('path', '')}\t{_picker_row(proj, git_lookup)}" + for proj in projects + ] + return _picker_header(), lines + + def display_project_info(proj): """Display project info for the confirmation screen.""" lines = [] @@ -256,39 +300,43 @@ def launch_project(project_path): def run_fzf_selection(projects): - """Run fzf to let the user select a project. Returns selected path or None.""" - # Build fzf input: tab-separated name and path, sorted by last-used - lines = [] - for proj in projects: - name = proj.get("rel") or proj.get("name", "unknown") - path = proj.get("path", "") - # Use path as the selection value - lines.append(f"{name}\t{path}") + """Run fzf to let the user select a project. Returns selected path or None. - input_data = "\n".join(lines) + "\n" + No timeout: this is an interactive picker, not a batch call. Esc/Ctrl+C + cancels (nonzero fzf exit) and returns None. + """ + header, lines = build_picker(projects) + input_data = "\n".join(lines) + "\n" if lines else "" try: fzf_proc = subprocess.run( - ["fzf", "--height=40%", "--reverse", "--prompt=opskit> "], + [ + "fzf", + "--height=40%", + "--reverse", + "--prompt=opskit> ", + "--delimiter=\t", + "--with-nth=2", # display the aligned columns (field 2) + "--accept-nth=1", # return only the path (field 1) + f"--header={header}", + ], input=input_data, - capture_output=True, text=True, timeout=60, + capture_output=True, + text=True, ) - if fzf_proc.returncode != 0: - return None # User cancelled - - selected = fzf_proc.stdout.strip() - if not selected: - return None + except (FileNotFoundError, OSError): + print("Error: fzf not found", file=sys.stderr) + sys.exit(1) - # Extract path from tab-separated line - parts = selected.split("\t") - if len(parts) >= 2: - return parts[1] - return selected + if fzf_proc.returncode != 0: + return None # User cancelled (Esc/Ctrl+C) - except (subprocess.TimeoutExpired, FileNotFoundError, OSError): - print("Error: fzf not found or timed out", file=sys.stderr) - sys.exit(1) + selected = fzf_proc.stdout.strip() + if not selected: + return None + # --accept-nth=1 yields the path; split anyway so we still work if a + # build ignores --accept-nth and returns the whole line. + return selected.split("\t")[0] def main(): diff --git a/plans/442-opskit-go-picker-columns.md b/plans/442-opskit-go-picker-columns.md new file mode 100644 index 0000000..fe02905 --- /dev/null +++ b/plans/442-opskit-go-picker-columns.md @@ -0,0 +1,57 @@ +# PLAN: `opskit go` picker — column layout and no timeout + +Closes #442. + +## Goal + +The fzf picker in `opskit go` must be readable and must not self-terminate. +Two defects: + +1. **No columns.** `run_fzf_selection` fed fzf `f"{rel}\t{path}"` with no header and + no fixed widths, so fzf expanded the tab to a tab stop and the long absolute path + dominated each line — no column boundaries. +2. **Times out.** fzf was invoked with `timeout=60`; leaving the picker open longer + killed it, printed "fzf not found or timed out", and exited 1. + +## Design + +Pure, unit-testable helpers plus a rewritten `run_fzf_selection`: + +- `_picker_header()` → header using the same fixed column widths: + `NAME | BRANCH | STATUS | RESUME`. +- `_picker_row(proj, git_lookup)` → one display row, space-padded to fixed widths, + long values truncated with `…`; branch/status from `git_lookup(path)`. +- `build_picker(projects, git_lookup=_git_info)` → `(header, lines)`; each line is + `f"{path}\t{display}"` — field 1 is the selection value, field 2 the display. +- `run_fzf_selection(projects)`: + - fzf args add `--delimiter=\t --with-nth=2 --accept-nth=1 --header=
` + (display field 2; return field 1 = path; show a header). Keeps + `--height=40% --reverse --prompt=opskit> `. + - **no `timeout`** — interactive picker; Esc/Ctrl+C cancels (nonzero rc → `None`). + - return `stdout.split("\t")[0]`, so it still works on a build that ignores + `--accept-nth`. +- Branch/dirty are computed lazily via `_git_info` for the displayed rows only. + +## Notes from implementation + +- fzf `--with-nth` scopes the fuzzy search to the **displayed** columns, so the + hidden path is not searchable — the visible `NAME` (the `rel` selector) is. Fine. +- `--accept-nth` needs fzf ≥ 0.48 (0.67 here); the defensive split covers older. + +## Critique + +- **Security:** unchanged input surface (paths from cache/config); argv list, no shell; + fixed widths, no format injection. +- **Stability:** fzf missing → clear error; empty input handled by the caller; no + timeout means an interactive wait can never be cut short. +- **Maintainability:** formatting isolated in pure helpers → tested without git/fzf. +- **Effectiveness/Value:** fixes both complaints; no scan-cost regression. +- **Flaw → resolution:** truncating a long name could hide a distinguishing suffix. + Resolution: `rel` is already the unique-ish selector and we now search it; accepted. + +## Tests + +`tests/test_tui_picker.py`: header names the columns; header/row column offsets align; +`[resume]` shown only when present; long names truncate to the column width; each line's +field 1 is the path; `run_fzf_selection` passes **no** `timeout` and passes the column +options; cancel and empty selection return `None`. Full suite: `make test`. diff --git a/tests/test_tui_picker.py b/tests/test_tui_picker.py new file mode 100644 index 0000000..50ffbe2 --- /dev/null +++ b/tests/test_tui_picker.py @@ -0,0 +1,126 @@ +"""Tests for the bin/opskit-go.py fzf picker layout (issue #442). + +Loads the real script as a module so tests exercise the shipped code. +""" + +import importlib.util +import sys +from pathlib import Path + +_ROOT = Path(__file__).parent.parent +sys.path.insert(0, str(_ROOT)) + + +def _load_go_module(): + spec = importlib.util.spec_from_file_location( + "opskit_go_picker", _ROOT / "bin" / "opskit-go.py" + ) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +_go = _load_go_module() + + +class _Proc: + def __init__(self, returncode, stdout=""): + self.returncode = returncode + self.stdout = stdout + + +def _fake_run(returncode, stdout, captured): + def fake(*args, **kwargs): + captured["argv"] = args[0] + captured["kwargs"] = kwargs + return _Proc(returncode, stdout) + return fake + + +PROJECTS = [ + {"name": "Demo", "path": "/home/user/Projects/Demo", "rel": "Demo"}, + {"name": "app", "path": "/home/user/Projects/group/app", "rel": "group/app"}, +] + + +class TestPickerHeader: + def test_header_names_the_columns(self): + header = _go._picker_header() + for col in ("NAME", "BRANCH", "STATUS", "RESUME"): + assert col in header + + +class TestPickerRow: + def test_columns_align_with_header(self): + header = _go._picker_header() + row = _go._picker_row( + {"name": "Demo", "path": "/p/Demo", "rel": "Demo", "has_resume": True}, + git_lookup=lambda _p: ("main", "dirty"), + ) + assert header.index("BRANCH") == row.index("main") + assert header.index("STATUS") == row.index("dirty") + assert header.index("RESUME") == row.index("[resume]") + + def test_clean_project_has_no_resume_flag(self): + row = _go._picker_row( + {"name": "Demo", "path": "/p/Demo", "rel": "Demo"}, + git_lookup=lambda _p: ("main", "clean"), + ) + assert "clean" in row + assert "[resume]" not in row + + def test_long_name_is_truncated_to_column_width(self): + row = _go._picker_row( + {"name": "x", "path": "/p/x", "rel": "a" * 60}, + git_lookup=lambda _p: ("main", "clean"), + ) + name_col = row[: _go.PICKER_NAME_W] + assert len(name_col) == _go.PICKER_NAME_W + assert "…" in name_col + + +class TestBuildPicker: + def test_line_field_one_is_the_path(self): + _header, lines = _go.build_picker(PROJECTS, git_lookup=lambda _p: ("main", "clean")) + assert lines[0].split("\t")[0] == "/home/user/Projects/Demo" + assert lines[1].split("\t")[0] == "/home/user/Projects/group/app" + + def test_one_line_per_project(self): + _header, lines = _go.build_picker(PROJECTS, git_lookup=lambda _p: ("main", "clean")) + assert len(lines) == len(PROJECTS) + + def test_git_lookup_is_used_for_branch_and_status(self): + _header, lines = _go.build_picker(PROJECTS, git_lookup=lambda _p: ("feature/x", "dirty")) + assert "feature/x" in lines[0] + assert "dirty" in lines[0] + + +class TestRunFzfSelection: + def test_no_timeout_and_column_options(self, monkeypatch): + captured = {} + monkeypatch.setattr(_go, "_git_info", lambda _p: ("main", "clean")) + monkeypatch.setattr(_go.subprocess, "run", _fake_run(0, PROJECTS[0]["path"] + "\n", captured)) + + selected = _go.run_fzf_selection(PROJECTS) + + assert "timeout" not in captured["kwargs"] + argv = captured["argv"] + assert "--delimiter=\t" in argv + assert "--with-nth=2" in argv + assert "--accept-nth=1" in argv + assert any(a.startswith("--header=") for a in argv) + assert selected == PROJECTS[0]["path"] + + def test_cancel_returns_none(self, monkeypatch): + captured = {} + monkeypatch.setattr(_go, "_git_info", lambda _p: ("main", "clean")) + monkeypatch.setattr(_go.subprocess, "run", _fake_run(130, "", captured)) + + assert _go.run_fzf_selection(PROJECTS) is None + + def test_empty_selection_returns_none(self, monkeypatch): + captured = {} + monkeypatch.setattr(_go, "_git_info", lambda _p: ("main", "clean")) + monkeypatch.setattr(_go.subprocess, "run", _fake_run(0, "\n", captured)) + + assert _go.run_fzf_selection(PROJECTS) is None