feat(tui): FTS-backed browse with chips, scope picker, preview pane, and help overlay

[lancekrogers]

Summary

Upgrades camp-graph browse from an in-memory substring explorer into a live FTS5-backed search experience with parity ergonomics to camp intent explore. Delivered across 46 commits under festival camp-graph-tui-search-upgrade-CG0005 (99/99 tasks).

• Live FTS pipeline with per-keystroke generation cancellation: issueQuery bumps queryGen, cancels any in-flight context.Context, and routes through a shared pipeline used by both search input and chip changes. Empty terms short-circuit to the client-side anchor fallback without hitting Querier.Search (per D005).
• Three filter chips (copied from camp/internal/intent/tui/filterchip under D002 provenance): Type, Tracked, Mode. Focused via t/s/m; value changes reissue the query.
• Scope picker modal (c opens, C clears). Selection threads into QueryOptions.Scope and narrows both FTS and anchor-fallback paths.
• Preview pane (right column): node header, path, status, outgoing/incoming edges capped at 50, top-3 related items via search.Related. tab focuses the pane; j/k scroll without moving the list cursor. Per-focus context cancellation via previewCancel with stale-drop in Update.
• Responsive layout: narrow (<80) hides preview and scope column, normal (80–120) is 60/40, wide (>120) is 50/50. Recomputed on tea.WindowSizeMsg.
• Keybinding parity: vim count prefix (5j, 10k, 3gg), gg/G, ctrl+u/ctrl+d scaled to half list height, a widen gated to explorer fallback, ? help overlay with the full UX_SPEC keybinding table, q/ctrl+c cancel in-flight Cmds before quit.
• Test coverage: 171 unit tests (up from 143). Race-clean cancellation tests for both the query and preview pipelines via narrow interfaces (querierIface, previewFetcher) with blocking stubs. buildOpts table covers 12 permutations including chip+scope cross-products; grouping goldens lock typePriority (D004) ordering and unknown-type first-appearance tie-break; layout boundary tests at 79/80/81/119/120/121.

Test plan

• just test unit — 171/171 pass
• just test all (unit + Docker integration) — 171+31 pass
• just lint — clean (go fmt ./..., go vet ./...)
• just build — clean
• go test -race ./internal/tui/... -run TestQueryCancellation -count=5 — stable
• go test -race ./internal/tui/... -run TestPreviewCancellation -count=3 — stable
• Manual parity walkthrough against camp intent explore (festival task 08_manual_parity_checklist)

[obey-agent]

Verdict: Request Changes

Reviewed against head 945f47e9. The architectural split out of the old tui.go is strong, and the generation-counter + per-keystroke context-cancellation pipeline is the right shape for live FTS. Two correctness bugs in the grouped-results path need fixes plus coverage before the new browser can be trusted as the default experience.

Key Findings:

1. internal/tui/model.go:237-256 focusedRowID walks groups by subtracting row counts only, ignoring group headers:

   cursor := m.cursor
   for _, grp := range m.groups {
       if cursor < len(grp.Rows) { return grp.Rows[cursor].NodeID }
       cursor -= len(grp.Rows)
   }
   

   Meanwhile navigation clamps against groupVisibleCount (render.go:305) which counts headers plus rows in expanded groups, and groupCursorTarget (render.go:283) also counts headers when mapping cursor -> (gi, ri). So for two expanded groups of two rows each, cursor=0 is rendered as the first group header but focusedRowID returns groups[0].Rows[0].NodeID; cursor=1 is rendered as the first row but focusedRowID returns the second row; cursor=2 rolls into group 1 and returns groups[1].Rows[0]; and later cursor positions walk off the end and return "". Net effect: the preview pane shows the wrong node (or nothing) for every cursor position once groups are live. This also poisons the stale-drop in update.go:83, which uses the same focusedRowID() as the "still relevant?" check. Route focusedRowID through groupCursorTarget so the three call sites agree on what the cursor points at.

2. internal/tui/update.go:287-297 enter handling has the same shape of bug. On a group row (gi >= 0, ri >= 0), it falls through to m.enterMicrograph(m.filtered[m.cursor]). m.filtered is the anchor/node list, indexed by a cursor that was clamped using groupVisibleCount(m.groups). When the query returns more rows than anchors, this panics. When it returns fewer, it opens the wrong node from the anchor list entirely. The branch needs to resolve m.groups[gi].Rows[ri].NodeID through m.graph.Node(id) and pass that *graph.Node to enterMicrograph.

3. internal/tui/update.go:69-80 queryResultMsg replaces m.results / m.groups without re-clamping m.cursor. When the new result set has fewer visible entries than the old, the cursor is left past the end until the next motion clamps it. Combined with finding 1, the preview pane and the list are in inconsistent positions for that window. Clamp m.cursor to groupVisibleCount(m.groups) - 1 (or len(m.filteredAnchors) - 1 if results are empty) as part of the message handler, and reset it to 0 when the previous result was non-empty and the new one is empty.

4. Tests cover neither of the above. model_test.go and preview_cancel_test.go use models with only filtered populated; grouping_test.go exercises groupByType in isolation; no test ever builds a Model with m.groups set and drives Update with navigation keys. That is what masked these. TestEnterOpensMicrographAndEscReturnsToList passes on anchors and says nothing about grouped results.

Secondary Notes:

• internal/tui/update.go:406 rebuilds filteredAnchors with a direct filterAnchors(...) call on esc, instead of the existing (m *Model).applyAnchorFilters(). Tiny drift surface: if applyAnchorFilters grows a side effect later, this path silently misses it. Route through the method.
• internal/tui/model.go:249-250 uses m.filteredAnchors != nil as the "are we in anchor mode" predicate. Because filterAnchors only returns nil when the input anchors slice is nil (rare; campaigns almost always have at least one scope anchor), this is effectively always true when groups are empty. That is what you want today, but it would be clearer to key off either the presence of FTS state (m.search.Value() != "" or len(m.results) > 0) or a dedicated flag. Not a blocker.
• issueQuery issues a fresh Cmd per keystroke. Because search.Querier.Search does respect ctx, cancellation is cooperative. Good. But the Cmd goroutine still sends into tea.Msg plumbing for the canceled generation; that is only filtered in Update via gen != m.queryGen. That is fine by design; surfacing the pattern in the runQueryCmd doc comment would save the next reader the walk.

What's Done Well:

• Generation-counter + cancel-on-supersede pipeline is the right primitive, and having it shared across the search-input and chip-change paths is the correct spec conformance.
• Narrow querierIface and previewFetcher interfaces let the cancellation tests run without a real DB; TestPreviewCancellation via the blocking stub is well-constructed.
• chipBar / focus-mode routing keeps the key dispatch small and inspectable.
• quitModel cancels both in-flight Cmds before tea.Quit, so goroutines do not outlive the UI. That matches the CLAUDE.md context-propagation standard.
• Responsive layout, vim count prefix, and preview pane slice correctly match the UX_SPEC description in the PR body.

Staff Standard:

Not yet. Findings 1 and 2 are silent-wrong-data bugs in the primary FTS flow; finding 3 is a smaller version of the same family. Fix those plus land tests that drive Update with a populated m.groups (both expanded and collapsed, single and multi-group) and I would be comfortable merging.

[lancekrogers]

All four findings addressed in a51de29.

1. focusedRowID now routes through groupCursorTarget, so the three cursor-to-row mappings (navigation clamp, render, preview) share a single definition of "what is at cursor position N". Preview headers no longer masquerade as rows and later cursor positions no longer walk off the end.

2. enter on a grouped row now resolves m.groups[gi].Rows[ri].NodeID through m.graph.Node(id) and hands the resulting *graph.Node to enterMicrograph. The old fall-through to m.filtered[m.cursor] is gone. Non-grouped enter now also prefers m.filteredAnchors when present and bounds-checks the cursor.

3. The queryResultMsg handler now clamps m.cursor to the new visible ceiling (groupVisibleCount when groups are present, len(filteredAnchors) when falling back to anchors, len(filtered) otherwise). When a search shrinks or clears the result set below the prior cursor position, the list and preview stay consistent on the same frame.

4. Search-esc path now calls m.applyAnchorFilters() instead of re-inlining filterAnchors(...), so the anchor-refresh logic only has to live in one place.

Tests in the new internal/tui/grouped_cursor_test.go build a Model with two expanded groups (two rows each) and cover:

• focusedRowID at every cursor position across two expanded groups
• focusedRowID with one group collapsed
• enter on a group header toggles Expanded without entering micrograph
• enter on a group row resolves the NodeID through the graph
• enter with an out-of-range cursor is a no-op
• queryResultMsg clamps the cursor when the result set shrinks
• queryResultMsg resets the cursor when results clear to empty

go build, go vet, go test -race ./... all clean.

Ready for re-review.

[obey-agent]

One non-blocking maintainability note: the new \ recipes work, but the repeated \ pattern is pretty opaque to read and debug. It looks like it is compensating for module-recipe working-directory behavior, which is a real issue here, but I think this would be clearer if the recipes anchored themselves explicitly via just path helpers or a repo-root variable instead of shelling up a directory in every recipe.

[obey-agent]

Unifying the cursor-to-row mapping through groupCursorTarget is the right call — navigation clamp, render, preview, and stale-drop now read from the same source of truth. The pre-fix focusedRowID was counting only rows while nav clamped against groupVisibleCount (headers + rows), so every grouped query had an off-by-one between the list and the preview pane.

The test coverage in grouped_cursor_test.go that walks every cursor position across two expanded groups (including the header positions) is exactly the kind of fixture that catches this regression the next time someone refactors the visible-entry enumeration.

[obey-agent]

The ceiling resolution here (groups > filteredAnchors > filtered) is the same priority focusedRowID and the j/k navigation handlers use. Three copies now — they are correct today but drift easily.

Extracting (Model).visibleCeiling() int (and ideally clampCursor()) would collapse the queryResultMsg clamp, the j/k upper bounds, and the G end-snap to a single source of truth. Non-blocking, but worth queuing for a cleanup pass before the next feature in this area.

[obey-agent]

Review: feat(tui) FTS-backed browse

Verdict: Approve.

Overview

Large, well-sequenced upgrade (46 commits, ~3.9k insertions). Replaces the substring explorer with a live FTS pipeline, threads chip/scope/mode through search.QueryOptions, adds a preview pane with per-focus context cancellation, and lands a responsive three-breakpoint layout and help overlay. Post-festival fix commit a51de29 catches three real correctness bugs in the grouped-cursor path and lands thorough test coverage.

Key Findings

• Grouped-cursor unification in focusedRowID via groupCursorTarget is the right call (inline on internal/tui/model.go:240). Pre-fix navigation clamped against groupVisibleCount (headers + rows) while focusedRowID counted only rows, so every grouped query had a preview/stale-drop off-by-one after the first group header was passed.
• Enter on grouped rows now resolves through m.graph.Node(m.groups[gi].Rows[ri].NodeID) instead of indexing the unrelated m.filtered slice. Non-grouped enter bounds-checks the cursor and prefers m.filteredAnchors when set — both were latent out-of-range panics.
• queryResultMsg now re-clamps the cursor when the result set shrinks. Minor DRY observation (inline on internal/tui/update.go:91): the same ceiling resolution appears in three places; extracting visibleCeiling() / clampCursor() would collapse them to one source of truth. Non-blocking.

What's Done Well

• Cancellation contracts are consistent and tested under -race: querierIface + stubQuerier for the query path, previewFetcher + stubPreviewFetcher for the preview path. Narrow interfaces keep production code injection-friendly without leaking test seams into callers.
• Cursor-space invariants are now regression-locked by grouped_cursor_test.go (every position across two expanded groups, collapsed-group skip, header-toggle, row-resolution, out-of-range no-op, shrink-clamp, empty-reset).
• D002 provenance for the copied chips package, D003 cancel-invariant nilling, D004 typePriority + alphabetical tiebreak, and D005 empty-query fallback predicate (isExplorerFallback) are each named in the code that implements them.
• Commit history is atomic and traceable; fest commit tags let a future reader reconstruct the sequence mapping without digging through the festival tree.

Staff Standard

Worth shipping. The late correctness fix caught bugs the festival's automated gates missed (all three only manifest once FTS returns non-empty grouped results), and the response was a targeted commit with focused tests rather than a big-ball rework. The one maintainability flag (ceiling/clamp DRY) is small and can be addressed in a follow-up without blocking this merge.

[lancekrogers]

Follow-up to the approve with non-blocking notes from 19:12Z. Pushed 341a9fb on top of a51de29.

DRY ceiling/clamp note (addressed):
Extracted (Model).visibleCeiling() and (Model).clampCursor(). The ceiling resolution previously lived in five call sites across update.go (queryResultMsg, pendingG gg, j, G, ctrl+u/ctrl+d). Three of them used the simpler groups vs filtered resolution while queryResultMsg used the full groups > filteredAnchors > filtered priority — a latent inconsistency against the anchor-fallback view that the refactor surfaces and fixes in one pass. visibleCeiling now shares its priority with focusedRowID, so navigation clamps, the G end-snap, and the post-query re-clamp all read from the same source of truth. Existing grouped_cursor_test.go still passes unchanged.

Justfile cd .. note (addressed):
This commit also contains justfile recipe changes — a project_root := source_directory() / ".." variable plus a swap of cd .. to cd "{{project_root}}". Those arrived in my working tree from a concurrent edit between my git fetch and camp p commit and landed in the same commit via camp p commit's implicit -a. The change is the right shape for the review note (anchor via a just path helper instead of shelling up a directory in every recipe), so I kept it rather than splitting or reverting. Flagging it here so the commit authorship is transparent.

go build, go vet, go test -race ./... all clean after the combined change.

[obey-agent]

Verdict: Approve for this commit (eddf2bd) with one semantic note.

Reviewed eddf2bd in isolation on top of 341a9fb. The change swaps project_root := source_directory() / ".." for project_root := justfile_directory() in all three modules (build.just, release.just, test.just). Verified locally with dry-runs.

Semantic difference worth knowing

source_directory() returns the directory of the file defining the expression (always .justfiles/ for these modules, so + ".." = repo root, regardless of how the recipe is invoked).

justfile_directory() returns the directory of the top-level justfile currently being evaluated (the main invocation entrypoint). This means:

• just xbuild dev / just release plan / just test short from the repo root → justfile_directory() = repo root. Correct. Dry-run confirms: cd "/Users/lancerogers/Dev/AI/obey-campaign/projects/camp-graph" && mkdir -p bin && go build ....
• just --justfile .justfiles/build.just dev (direct module invocation) → justfile_directory() = .justfiles/. Dry-run: cd "/Users/lancerogers/Dev/AI/obey-campaign/projects/camp-graph/.justfiles" && mkdir -p bin && go build .... This path would not resolve ./cmd/camp-graph. The prior source_directory() / ".." form would still resolve correctly here.

Verdict rationale

Direct module invocation via --justfile is not a documented or CI workflow for camp-graph; all usage flows through the root justfile's mod imports. For that documented pattern, both forms produce the same path, and justfile_directory() expresses the intent (project_root = repo root) more honestly than the structural climb of source_directory() / "..". Approve this commit as-scoped.

Non-blocking note

If someone ever tries to reuse these module files from a different project's root justfile (copy or symlink the .justfiles/ dir), justfile_directory() will resolve to that other project's root, not camp-graph's. The commands inside (e.g. go build -o {{project_root}}/bin/{{binary_name}} {{main_path}}) would then build into the wrong tree. With source_directory() / ".." the modules stayed anchored to their own on-disk location. Not a current use case, so nothing to change here; worth keeping in mind if we ever extract these modules for sharing.

What's Done Well

• Three-file coordinated change, single commit, clear message.
• The commit title signals intent ("module roots") rather than describing the syntax change.
• Dry-runs confirm the intended behavior at the primary invocation entrypoint.

Staff Standard

Yes for this commit. The cleaner expression is worth the slightly narrower robustness profile, given the actual invocation pattern.