diff --git a/af-tree-rails-reviews/round0-kimi.md b/af-tree-rails-reviews/round0-kimi.md new file mode 100644 index 0000000000..2d0e2f89b7 --- /dev/null +++ b/af-tree-rails-reviews/round0-kimi.md @@ -0,0 +1,23 @@ +# Deep review — tree rails in the tui3 tasks side column (round 0, kimi) + +**Reviewed state:** repository `/Users/santoshkumar/af-tree-rails`, branch `task/restore-tree-rails-in-the-tasks-c2da84`, base `origin/dev` = `e7976208e`, HEAD = `4c1e1d481` ("tui3: tree rails back in the tasks side column"), single commit `e7976208e..HEAD` (`git diff e7976208e..HEAD`, 9 files, +167/−23). Reviewed read-only: no files modified, no commits, no build or test execution, per the review-agent skill's contract. + +**Hand-off note:** the sandbox refused the named output path (`/Users/santoshkumar/af-tree-rails-reviews/` is outside this task's copy), so the report lives at `round0-kimi.md` in the assigned tree and should be copied to `/Users/santoshkumar/af-tree-rails-reviews/round0-kimi.md` — verbatim, one `cp` — by whoever integrates this run. + +## Findings + +No findings. + +## Overall assessment + +The diff was read in full and each load-bearing seam was traced at HEAD, not inferred from the patch: + +- **The lead is one mechanism with two callers.** `railLead(last []bool, elbow bool)` (planrail.go:252) is the only place a lead is spelled, and it is exactly 2 cells per level: `├ ` / `└ ` at the row's own level when `elbow` is set, `│ ` for a non-last ancestor, two spaces for a last ancestor, empty at depth 0. `planRailLines` (~:288), `planPageLines` (~:308) and `railLines` (task.go ~:3113) all build `last` identically (`copy` + append `i == len(kids)-1`) and prepend `lead +` to an unchanged `railEntryRow(e, width)` (task.go:4290, verified byte-for-byte against the brief's "must be unchanged"). No second shape exists. +- **Depth ≥ 2 last/stem logic is correct.** `railLines`' `open` slice is updated before the recursion (`open = append(open, i < len(nodes)-1)`) and restored after, so a middle child at depth 2+ carries `│ ` at its ancestor's level and its own connector is never confused with its parent's (that confusion is exactly what c263's `TestPlanRailDrawsTheTree` pins, and the shape reads correctly there). +- **The width invariant holds, and was upgraded honestly.** The old sites subtracted `len(lead)`; the diff switches every cut (`planRailLines`, `planPageLines` row + under-line, `railLines` head + under-stem) to `ansi.StringWidth(lead)`. That is not width laundering: `palette.glyph` (styles.go:1124) returns the bare rune with no styling escapes, and the tree glyphs (`├` U+251C, `└` U+2514, `│` U+2502, glyph.go:288-291) resolve to their Plain spelling in every tier — they carry no NerdFont binding (nerdfont.go:537-546 sets only Name/Meaning, and glyphset.go:297-306 falls back to Plain) and no ASCII binding either, so the ASCII tier also emits the 1-cell box glyph. Every measured lead is exactly 2 cells/level, matching what the brief's width computations assumed. The terminal-dependent caveat (a box glyph reported wide) is real but uniform: both the subtraction and the concat consume the same measurement, so the row still fits; nothing double-counts. +- **Under-line trunk rule matches between the two renderers.** The node-hung side (`railUnder` caller, task.go:3388-3397) and the plan-page side (`planPageLines` with `hang := a.railLead(at, false)`) both suppress the stem exactly at the row's own deepest level and keep `│ ` where an ancestor's connector is `├`. The change-entry's "hanging `└─`-off-`│`" claim is accurate. +- **No re-parenting, no second lent-id scheme.** `planRailForest`, `planTwig`, `planRailNode`, `planRailNodeID`/`planRailNodeBit` are untouched by the diff (checked by inspection of both the patch and the live code), and c246's exact-text law ("a dependency never re-parents a row"; Alpha still ordered inside A's family and reading `queued · waits: B`) is still asserted verbatim — the tests were re-texted for the new lead, not re-semanticed. +- **Tests pin the contract, not just the current output.** c263's `TestPlanRailDrawsTheTree` asserts the depth-2 shape including the continuing stem `│ ├ └ Alpha` after a closed sibling `└ └ B`, so a middle child losing its `│ ` fails it. c266's updated assertions still bind indentation ordering (child's name strictly right of parent's) and the no-clipping law; the new near-clipping condition in `railLines` is `2*(inner+2) <= width`, i.e. the untruncated text plus its own lead would fit — the stem is drawn only when it provably does not crowd a full-length row. +- **`planroom.go`'s touch is a comment fix that now tells the truth** (`railLead` → `railEntryRow`: the entry row is the two-cell economy, the lead is the caller's). The manual and change-entry edits match the shipped behavior. + +Residual risks and material test gaps, in the open: (1) No test drives a family deeper than depth 2, so the `open`/`last` restore-after-recursion invariant at depth 3+ (a stem that must outlive a fully closed grandparent) is pinned by code reading, not by a test. (2) No test asserts behavior on a terminal tier where a box-drawing glyph measures wide; the comment in `railLead` documents that the row stays correct via StringWidth, but nothing exercises it. (3) The plan-page under-block truncation is only covered at width 160 (c263 `TestPlanPageRoom…`); the narrow-width `hang + " " + under` path at the clipping boundary is untested. None of these rise to a defect in the diff itself; they are gaps worth a follow-up test if the author wants the depth-3+ and narrow-width contracts pinned. The pre-existing `credits_test.go` failures and the wider suite wedge named in the brief are outside this diff and were not re-examined. diff --git a/af-tree-rails-reviews/round1b-quality.md b/af-tree-rails-reviews/round1b-quality.md new file mode 100644 index 0000000000..d5a78bf0be --- /dev/null +++ b/af-tree-rails-reviews/round1b-quality.md @@ -0,0 +1,34 @@ +# Round 1b — QUALITY + CONTRACT lens: tree rails in the tasks side column + +NOTE FOR THE COORDINATOR: this report must be copied to /Users/santoshkumar/af-tree-rails-reviews/round1b-quality.md (this task may only write inside its assigned tree). + +Reviewed state: base `origin/dev` **e7976208e**, HEAD **4c1e1d481** ("tui3: tree rails back in the tasks side column"), one commit, 9 files +167/−23. Reviewed `git diff e7976208e..4c1e1d481` in `/Users/santoshkumar/af-tree-rails`, read-only: nothing built, tested, executed, or modified. Lens: architecture, contracts, tests, repo gates. Draft PR Agent-Field/CodeAF#1665 (Fixes #1660) read for the author's description; the diff is what was reviewed. `.senior-dev/` ignored. + +## Findings + +[P3] `internal/tui3/c266_rail_row_test.go` — the indentation law's stability anchor was weakened in re-texting. The pre-change test matched the row `prefix + "- \u2022 Ship the port"`, which pinned three things at once: the connector glyph (`- `), the gap between connector and mark, and the mark (`•`) staying glued to the lead at its old column. The re-texted version matches `"- Ship the port"`, which any lead variant containing a bare `-` before the title also satisfies (e.g. a regression that renders the lead as `- Ship` with no task mark, or a broken glyph tier spelling the tree glyphs as `-`). The still-passing exact-text assertions at planrail.go's other seams partially cover the shape, but this particular law test no longer pins the connector cell's identity. Fix: restore the marker in the expected string (e.g. match against `lead+mark+title` the way c263_rail_plan_test.go now does, or keep the `- •` spelling with the actual current glyph). + +[P3] `internal/manual/chat/worker-harness.md` — the re-texted line overclaims scope. The old text said the side column's rows "carry no tree rails", which was simply true. The new text says "its rows hang by depth with `├ ` and `└ ` leads and `│ ` stems, a task under a task a level in" — true for plan-store rows (planRailLines/planPageLines), but node-hung families rendered by `railLines` (task.go:3113) did NOT gain rails in this diff (verified: `railLines` unchanged, no `railLead` call there). A reader of the manual will expect stems under a live worker's children and not see them. The author fixed exactly this class of overstatement one paragraph down in the same diff ("drawn by app.railEntryRow") but missed this one. Fix: qualify the sentence to the run's plan rows. + +## What was checked and holds (no finding) + +- **One mechanism, two callers, genuinely shared.** `railLead(last []bool, elbow bool)` (planrail.go:252) is the only lead builder; both `planRailLines` (:284) and `planPageLines` (:312) call it; `railEntryRow` (task.go ~4258) is untouched by the diff; there is no second shape and no copy-pasted lead logic. The extra `elbow` bool (vs a `railUnderLead`) is justified — the false form is taken by two distinct callers (folded tail :296, under-lines :324). Complexity is one loop over levels; nothing needless was added. +- **Contract boundaries respected.** The lent-node id scheme (`planRailNodeBit` = 1<<63, `planRailNodeID` fnv over a namespaced salt) is unchanged; `teamspagedraw.go` untouched; `railLines` (node-hung families) deliberately untouched — matches the brief's intended design. +- **Laws hold.** "A DEPENDENCY NEVER RE-PARENTS A ROW" (c246_tree_test.go) untouched and still asserts ordering, not pixels. The `queued · waits: ` wording tests are untouched by the diff. The waits-sentence shape test (own-first, `12 steps`) still asserts semantics. +- **The new rail-shape test (c263_rail_plan_test.go) genuinely pins the contract.** Traced the bug-injection cases against `lead()` + `planPageDraws`: a middle child losing its stem → `aa` renders as air, lead mismatches the `railLead(aa, true)` reference → FAIL. A last-child elbow drawn as branch → `railLead` reference differs → FAIL. A depth-0 lead leak → `leadW` is 0 so `text` becomes `├ Write…` vs expected `Write…` → FAIL. A last-child's depth-2 child growing a stem (`pair` as `│ ` instead of air) → lead mismatch → FAIL. The under-row check (stem inherited on the Handler under-line) is also pinned. The test re-derives expectations from the same `railLead` under test, which would normally be self-referential, but the plain/glyph tier (`tokens.Plain`) pins the glyph strings independently, so a wrong glyph still fails. +- **Width arithmetic.** The one touched width site (planroom.go:53) replaced `len(lead)` with `ansi.StringWidth(lead)` — strictly more correct (byte-vs-cell); all other room cuts (`entryWidth - leadWidth`, `planLineRoom`) were already cell-based via `ansi.StringWidth`, so the 2-cells-per-level invariant holds. Box-drawing glyphs come from the geometry vocabulary (`tokens.GlyphTreeBranch = "├"` etc.), and the geometry fallback law resolves them to the plain 1-cell rune in every tier including the ASCII floor — no width surprise. `planPageLines` gained `railWidth/2` headroom, removing a pre-existing truncation-at-2-cells edge. +- **Slice aliasing.** `grow` builds each kid's `last` via `make+copy+append` — fresh backing array per level, no shared-slice corruption. +- **Cycle/orphan safety.** `planTwigsOf` excludes self-parenting (`up != twig`) and orphans a row whose parent is absent onto the page root; a store cycle longer than self-loop is possible in theory but bounded by the store's own ordering, and is pre-existing behavior, not introduced here. +- **Repo gates.** Changelog entry present (`docs/changes/unreleased/1665-tree-rails-side-column.md`, `kind: fix`, `pr: 1665`, `surface: chat tasks column`, `invalidates:` filled) — the CI changelog-check is satisfied. `gofmt -l` on the touched directories is clean (listing only, no modification). Namelaw: the one `CodeAF` in `worker-harness.md` is a pre-existing line about the OTHER program (the swe-pro harness trailer), untouched by the diff and allowed. + +## Overall assessment + +The change is architecturally clean: one shared helper, two callers, all width arithmetic cell-based, no boundary violations, and the changelog gate is satisfied. The two findings are both test/docs-quality P3s — worth fixing, neither blocks. + +**Brief-vs-diff discrepancy (observation, not a defect):** the brief describes taskident.go as "the task's number shown on side rows again". The actual taskident.go hunk is a doc-comment-only correction (`[app.railLead]` → `[app.railEntryRow]`); no rendering behavior changes there. The task-number display described was either landed in an earlier wave or is aspirational — nothing in this diff shows numbers on side rows. The sibling reviewers should not hunt for it in this diff. + +## Material test gaps / residual risks + +- The rail-shape test exercises exactly one family shape (single root, one mid child with kids, one last child). A regression specific to depth ≥ 3 (stem propagation past two last-flag levels) or to a family whose FIRST child is also its last (sole child → `└ `) is not pinned; the `└ ` path at depth 1 is pinned, the depth ≥ 3 path is not. Cheap to add one more kid layer if the author wants it. +- `railLines` (node-hung families) has no rail-shape coverage because it has no rails — if the manual line (second finding) is instead made TRUE in a follow-up, that path will need the same treatment. +- Tests were not executed (read-only contract); the verification is by trace against the helpers (`railText`, `readPlanRows`, `railSeam`, `railMark`, `planAppWith`, `update` all exist and match usage). diff --git a/af-tree-rails-reviews/round1c-render.md b/af-tree-rails-reviews/round1c-render.md new file mode 100644 index 0000000000..4d1cdb3337 --- /dev/null +++ b/af-tree-rails-reviews/round1c-render.md @@ -0,0 +1,33 @@ +# Round 1c — RENDERING + UX review: tree rails in the tui3 tasks side column + +**Reviewed state:** base `origin/dev` `e7976208e`, HEAD `4c1e1d481` ("tui3: tree rails back in the tasks side column", the branch's single commit), reviewed as `git diff e7976208e..HEAD` — 9 files, +167/−23: `internal/tui3/planrail.go`, `internal/tui3/planroom.go`, `internal/tui3/task.go`, `internal/tui3/taskident.go`, `internal/tui3/c263_rail_plan_test.go`, `internal/tui3/c266_rail_row_test.go`, `docs/changes/unreleased/1665-tree-rails-side-column.md`, `internal/manual/chat/reading-a-task-page.md`, `internal/manual/chat/worker-harness.md`. Method: review-agent skill, read-only, defect-first; no files modified, built, or executed. Lens held to rendering correctness, widths, and what the person in front of the pane sees. + +## Findings + +**[P2] Connectors survive a room cut the title cannot — a row can draw as bare rail glyphs with no words** — `internal/tui3/planrail.go` (`planRailLines` ~:289, `planPageLines` ~:315). Both renderers compute the title's room as `max(width-ansi.StringWidth(lead), 0)` and prepend the lead unconditionally, and `railEntryRow`/`planRowText` (via `fitWidth`, render.go:4361, which returns "" at width ≤ 0) clip the row to nothing while the lead stands. The roster's splice (`railDrawnView`, task.go:3403) passes `a.railRoom()` through verbatim with no depth budget, and neither helper caps depth. A plan published with a deep hierarchy — or the plan room's page at a narrow `width` (planRoomPartRows passes `min(width, planPageKinWidth)` with no floor) — draws `├ ├ ├ ├ ` down the column with every title clipped away: the person sees a stack of bare elbows, not a tree, and cannot identify any row. The pre-existing sibling surface `tasksplace.go` solves exactly this with `tasksKinTree`'s depth cap ("THE COLUMN MAY NEVER TAKE THE CELLS THE NAME NEEDS", tasksplace.go:820); the new mechanism has no equivalent. One cheap fix at the helper level (cap levels like `tasksKin`, or skip drawing a row whose room after the lead is under some floor, hanging it a level up instead) covers all three callers. Not P1 because it needs a deeper-than-planned hierarchy or a narrower-than-floor width, not the common path. + +**[P3] The side column's rail shape — the actual fix for #1660 — has no shape-pinning test** — the new `TestTheTasksPlacesRowsCarryTheFamilysRail` (c266_rail_row_test.go) exercises `planPageLines` only (a task page's rows). Nothing asserts the shape of the side-column rows `planRailLines` draws into the roster, nor the plan room's `planRoomPartRows` path (planroom.go:409). `TestRailPlanDrawsEveryPartAsATaskRow` (c263) checks that part *titles appear* in the rendered rail but nothing about `├`/`└`/`│` placement there — and a title assertion cannot distinguish "hung under the run row with a connector" (the complaint in #1660) from "spliced in flat anywhere above or below it". The three renderers share `railLead`, so one bug there would be caught by the task-page test, but a caller-side bug (e.g. `railDrawnView` passing the wrong `last` flags, wrong width, or splicing under the wrong row) would ship silently against the very issue this change fixes. A small exact-text test over `railRows()` output on the c263 fixture, or direct `planRailLines` assertions mirroring the c266 cases, closes it. + +## Verified clean within the lens + +- **Depth ≥ 2 stem logic (`railLead`, planrail.go:252):** the level loop is index-correct — at a row's own level the elbow prints `├ ` (non-last) or `└ ` (last); ancestor levels print `│ ` where that ancestor had a sibling to come and two spaces where it was last, so a middle child carries its own trunk (never inherits the parent's connector) and the last child's subtree grows no stems past its `└ `. Deeper descendants compound level by level; `c266_planRows` exercises exactly the discriminator case (`mid` keeps `│ ` because `deep` follows; `inner` under `mid` gets `│ └ `), and the test asserts it exactly. +- **Width invariant, end to end:** the lead is exactly 2 cells per level (one 1-cell glyph + one space, in every tier — the `├`/`└`/`│` bindings carry no `ASCII:` field, so the ASCII set falls back to the same `Plain` rune; no 2-cell ambiguous Nerd Font variant exists). Every subtraction uses `ansi.StringWidth(lead)` (a grapheme/width walk), never `len(lead)`, so the 3-byte UTF-8 encodings cannot inflate the cut. `planRailRoot` subtracts the lead the same way (both `fitWidth` calls), `planroom.go:409` only changes `0` → `nil` (identical lead), and `app.railEntryRow` (task.go:4290) is unchanged — the roster's own rows keep their 2-space indent. The final paint (`railRows`) is `lead + text` with no re-truncation, so per-row fitting is the whole contract and each site honours it. +- **Clipping is cell-safe:** `fitWidth` returns "" at width ≤ 0 and otherwise `ansi.Truncate` at a cluster boundary — no connector, stem, or number glyph can be left half-drawn. The only residue of a hard cut is the whole-glyph case covered by the P2 above. +- **Same shape, all three callers:** `planRailLines`, `planPageLines`, and (through `planRailRoot`) the orphan-run block all build leads via the single `railLead` helper with the same 2-cell convention — a person learns one convention. The only divergence is the under-block of `planPageLines` (~:321), which uses `railLead(at, false)` (a full-level trunk with no elbow) plus the historical four-space margin under a row's words. At depth 1 that draws `│ ` in the connector column while the part's own words sit one column right of the trunk. The c266 test pins it, and it is coherent (the text answers the row, it is not a sibling), but it is the one place a person reading closely sees the stem not line up with the column of words. Cosmetic; noted, not flagged as a fix. +- **Task-number rendering (`taskident.go`):** the figure stands at the row's front (`figure+" "+call+" "+tail`), inside the row text after the lead — it cannot collide with the rail lead, and it flows through the same `fitWidth`, so at narrow widths the *end* (status/age words) clips first, not the number. The roster branch (task.go:4340) is `fitWidth(...)`-gated identically. Containment only: `taskident.go` builds text from a node already in the tree; nothing touches parent/child ordering. One hard truncation loses the status/age first and then the call — the number is the last thing to vanish, which is the right priority for "which task is this row". +- **The original complaint is answered at real widths:** the side column is 28–40 cells (`sideColsFor`, sidecol.go:207) minus the 1-cell seam, so `railRoom()` ≥ 26. A root plus a depth-2 family at `#12 · ` + connectors + status + age fits that budget at typical title lengths, parts visibly hang under the run row (spliced by `railDrawnView` directly under the carrier node), and the restored number re-identifies the row. +- **planroom.go touch** is the type-alignment hunk only; no width or order change. + +## Overall assessment + +The mechanism is sound: one shared 2-cell-per-level helper, correct last/stem logic at every depth, width arithmetic that measures glyphs rather than bytes, and cell-safe clipping throughout. What the person in front of the pane sees is the intended tree at every width the side column actually gets. The two findings are at the margins: an uncapped depth budget that can draw a column of bare connectors on pathological plans (P2), and the missing shape test on the side-column path that #1660 itself is about (P3). Neither blocks the design; both are small, local fixes. + +## Material test gaps + +1. No shape-pinning test for the side-column rail rows or the plan room page (the P3 finding): caller-side wiring of `planRailLines`/`planPageLines` from `railDrawnView`/`planRoomPartRows` is unpinned. +2. No test at an adversarial width or depth (the P2 finding): every fixture renders at width ≥ 46 with depth ≤ 3; nothing exercises a lead that consumes the whole room. +3. The under-block alignment (stem vs. one-column-in words) is pinned by exact text but only at depth 1; a depth-2 under-block (`│ │ ` + 4 spaces) is untested. + +--- + +DELIVERY NOTE FOR THE COORDINATOR: the report must land at /Users/santoshkumar/af-tree-rails-reviews/round1c-render.md (outside this task's copy — the write was refused there). Copy this file's content (everything above this note) to that path verbatim, or move this file: it is byte-identical to what was meant to be delivered. diff --git a/docs/changes/unreleased/1665-tree-rails-side-column.md b/docs/changes/unreleased/1665-tree-rails-side-column.md new file mode 100644 index 0000000000..5ff664dcf9 --- /dev/null +++ b/docs/changes/unreleased/1665-tree-rails-side-column.md @@ -0,0 +1,17 @@ +--- +kind: fixed +title: the tasks side column draws a family on its own tree connectors again +pr: 1665 +surface: [chat] +invalidates: + - "The tasks side column drew no rail lines: a run's parts hung bare two spaces a level (`strings.Repeat(\" \", depth)` in planrail.go), which is what #1494 left behind when it dropped the roster forest. A child now rides `├ ` while a sibling follows it and `└ ` where it closes its parent's family, a deeper row carries the `│ ` trunk past every ancestor that had rows still to come, and the last child's levels leave air — drawn in the lead the caller prepends, two cells a level, on the side column and on a task page's `under it` alike." + - "Two manual pages spelled the task page's connectors `├─`/`└─` (reading-a-task-page.md, worker-harness.md); they spell the drawn `├ `/`└ ` now. The side column's node rows are still one flat line per task — #1494's roster shape stands; the connectors belong to the families hung under a row." +--- + +The column's families were bare indentation since #1494, so a run's shape did not +read: a child's title floated right of its parent's with nothing anchoring it, while +the manual (tasks.md) and the plandb CLI tree kept documenting and drawing +connectors. The rail lives in the lead again — one helper, `railLead`, builds it +for both renderers out of the ancestors' last-child flags; `railEntryRow` is +untouched, the lead stays exactly two cells a level so every width computation that +reads it keeps its shape, and a dependency still never re-parents a row. \ No newline at end of file diff --git a/internal/manual/chat/reading-a-task-page.md b/internal/manual/chat/reading-a-task-page.md index 4b0c1d5d1b..5c02de132e 100644 --- a/internal/manual/chat/reading-a-task-page.md +++ b/internal/manual/chat/reading-a-task-page.md @@ -723,7 +723,7 @@ that those files were not sent with the correction. The `under it` section is the whole subtree in store order, not only the direct children, and every row in it is drawn exactly as the rail draws a task: the state mark (the spinner while it works), the name, its `#id` at the end, and the -tree's own connectors (`├─`, `└─`). A part that is running says the command it is +tree's own connectors (`├ `, `└ `). A part that is running says the command it is on, such as `bash go test ./...`, and under that how long it has run and what it has cost, each left out when the store has not got it. diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index 8c7e2e24f6..d008a2704d 100644 --- a/internal/manual/chat/worker-harness.md +++ b/internal/manual/chat/worker-harness.md @@ -254,10 +254,10 @@ A dependency never changes that family. `pending` means admitted and not started the row stays under the task that requested it and wears `queued · waits: ` to name the separate dependency. -A task's **page** shows its children under its steps the same way, in `under it`, -each drawn as the side list draws a task — mark, name, `#id`, the tree's `├─`/`└─` -— with the command it is on while its worker is on one. Notes, pause, cancel and the -rest of steering are unchanged by the tree. +The tree's `├ `/`└ ` rails are the run's plan rows; a live worker's own +children on its page are drawn in `under it` as the side list draws a task — mark, +name, `#id` — with the command it is on while its worker is on one, no rails under +them. Notes, pause, cancel and the rest of steering are unchanged by the tree. ## What step is a run task on? diff --git a/internal/tui3/c263_rail_plan_test.go b/internal/tui3/c263_rail_plan_test.go index 14a0398a46..3c88a4c5d3 100644 --- a/internal/tui3/c263_rail_plan_test.go +++ b/internal/tui3/c263_rail_plan_test.go @@ -169,6 +169,75 @@ func TestARunsPartsHangUnderTheNodeRowThatCarriesIt(t *testing.T) { } } +// THE TREE THE SIDE COLUMN USED TO DRAW IS BACK IN THE LEAD. A family hangs +// off its own connectors: a child rides `├ ` while a sibling follows it and +// `└ ` where it closes its parent's family, a grandchild carries the `│ ` stem +// past every ancestor that had rows still coming, and the last child's own +// level leaves air — no stem past its connector, because nothing follows it +// there. The lead is two cells a level, so every row stands in the column the +// bare indent drew, and the run's own row keeps its place above the family +// with no lead at all. +func TestTheRailDrawsAPlanFamilyOnItsOwnConnectors(t *testing.T) { + rows := []session.PlanTaskRow{ + {ID: "t-root", Title: "Root task", Status: "running"}, + {ID: "t-a", Parent: "t-root", Title: "Child one", Status: "running"}, + {ID: "t-b", Parent: "t-root", Title: "Child two", Status: "pending"}, + {ID: "t-g1", Parent: "t-a", Title: "Grand one", Status: "pending"}, + {ID: "t-g2", Parent: "t-a", Title: "Grand two", Status: "pending"}, + {ID: "t-g3", Parent: "t-b", Title: "Grand three", Status: "pending"}, + } + a, _ := planAppWith(t, rows, nil) + a.width, a.height = 160, 30 + a.taskUpdate(update(9, "Root task", session.TaskRunning, session.TaskNotice{PlanTask: "t-root"})) + a.paints = 0 + readPlanRows(t, a) + + drawn := railText(a, a.viewHeight()) + lead := func(title string) string { + for _, line := range drawn { + if !strings.Contains(line, title) { + continue + } + // The seam is the column's own edge, and the keyboard's row wears + // its marker in the seam's place; the lead the family hangs by + // starts after whichever of the two is on this row. + body := strings.TrimPrefix(line, railSeam) + return strings.TrimPrefix(body, railMark) + } + t.Fatalf("the rail has no row for %q:\n%s", title, strings.Join(drawn, "\n")) + return "" + } + root, one, two := lead("Root task"), lead("Child one"), lead("Child two") + gOne, gTwo, gThree := lead("Grand one"), lead("Grand two"), lead("Grand three") + + // THE RUN'S OWN ROW DRAWS NO LEAD — the family hangs off it, two cells in. + if strings.HasPrefix(root, "├") || strings.HasPrefix(root, "└") || strings.HasPrefix(root, "│") { + t.Fatalf("the run's own row wears a lead:\n%s", strings.Join(drawn, "\n")) + } + // THE FIRST LEVEL: an elbow off the parent, `├ ` while a sibling follows + // and `└ ` where the child closes the family. + if !strings.HasPrefix(one, "├ ") { + t.Fatalf("the child with a sibling after it does not hang off `├ `: %q\n%s", one, strings.Join(drawn, "\n")) + } + if !strings.HasPrefix(two, "└ ") { + t.Fatalf("the family's last child does not close off `└ `: %q\n%s", two, strings.Join(drawn, "\n")) + } + // THE SECOND LEVEL: the trunk `│ ` runs past an ancestor with rows still + // coming, and the row's own elbow rides it. + if !strings.HasPrefix(gOne, "│ ├ ") || !strings.HasPrefix(gTwo, "│ └ ") { + t.Fatalf("the middle rows carry no stem past their parent:\n%q\n%q\n%s", gOne, gTwo, strings.Join(drawn, "\n")) + } + // AND A LAST CHILD CARRIES NO STEM PAST ITS OWN CONNECTOR: the branch it + // closes leaves air where a trunk would run, on its own levels and its + // children's. + if strings.Contains(two[:4], "│") || strings.Contains(gThree[:4], "│") { + t.Fatalf("a stem runs past the last child's connector:\n%q\n%q\n%s", two, gThree, strings.Join(drawn, "\n")) + } + if !strings.HasPrefix(gThree, " └ ") { + t.Fatalf("the last child's own part does not hang in the air it leaves: %q\n%s", gThree, strings.Join(drawn, "\n")) + } +} + // A RUN'S TASK OPENS THE TASK ROOM AND WEARS ITS HEAD: the trail with the way // back at its end, and the facts with the clock, the steps and the money. The // figures the store has not got are absent, never zero. diff --git a/internal/tui3/c266_rail_row_test.go b/internal/tui3/c266_rail_row_test.go index 6e64742e74..712afc2159 100644 --- a/internal/tui3/c266_rail_row_test.go +++ b/internal/tui3/c266_rail_row_test.go @@ -87,18 +87,26 @@ func TestTheRailIndentsATaskUnderItsParentTask(t *testing.T) { rows := c266PlanRows() rows = append(rows, session.PlanTaskRow{ID: "kid", Parent: "held", Title: "write the fixtures", Status: "pending"}) // The widened column, where both titles are drawn whole beside the wait. - _, rail := c266Rail(t, rows, 160, true) + a, rail := c266Rail(t, rows, 160, true) _, parent := c266RowWith(t, rail, "write the tests") _, child := c266RowWith(t, rail, "write the fi") if strings.Index(child, "write") <= strings.Index(parent, "write") { t.Fatalf("the task under a task is not indented under it:\n%s\n%s", parent, child) } + // The child's row pins connector, mark and title together: a regression + // that drops the pending mark off the lead (rendering the lead as a bare + // '-') now fails here instead of passing the position check above. The + // mark's glyph comes from the palette so its spelling is never assumed. + pending := plain(a.pal.glyph(tokens.GQueued)) + if !strings.Contains(plain(child), plain("\u2514 "+pending+" ")+"write the fixtures") { + t.Fatalf("the task under a task does not wear its mark before its title:\n%s", child) + } } // A TASK UNDER A TASK STANDS A LEVEL IN, AND THE LINE A RUNNING PART WOULD -// HAVE HAD UNDER IT IS THE HINT'S. The family's connectors are gone from the -// side column (DESIGN.md, One side column): every task is one line, and a part -// shows its depth by its indent alone. +// HAVE HAD UNDER IT IS THE HINT'S. Every task is one line; a part shows its +// depth by the tree's own connectors in its lead ([app.railLead]), and its +// call is the hint's, never a line under the row. func TestTheFamilysLineRunsThroughTheLinesUnderARow(t *testing.T) { rows := c266PlanRows() rows = append(rows, session.PlanTaskRow{ID: "kid", Parent: "held", Title: "write the fixtures", Status: "pending"}) diff --git a/internal/tui3/planrail.go b/internal/tui3/planrail.go index 844d647b66..cc5fffc7f7 100644 --- a/internal/tui3/planrail.go +++ b/internal/tui3/planrail.go @@ -23,7 +23,10 @@ import ( "sort" "strings" + "github.com/charmbracelet/x/ansi" + "github.com/Agent-Field/codeaf/internal/session" + "github.com/Agent-Field/codeaf/internal/tui2/tokens" ) // planRailNodeBit marks a node id as LENT to a store row. Every id the engine @@ -227,20 +230,96 @@ func planTwigsOf(rows []session.PlanTaskRow) []*planTwig { return out } +// railLeadFloor is the least room a family row may leave for its title after +// the lead: two cells, one glyph and the space that follows it. A row with less +// than that has stopped being a row that names anything, and drawing only the +// bare connector would hand the reader a `├ ` that says "here is a row" over an +// empty name. +const railLeadFloor = 2 + +// railLevels is how many levels of family a column this wide may hang before a +// deeper path stops eating the cells the name needs. Two cells to a level, so +// the budget is the room a title keeps after a level draws itself; a path past +// it hangs a level up ([app.railLead] takes the count from its callers). +func railLevels(width int) int { + levels := (width - railTitleFloor) / 2 + if levels < 1 { + levels = 1 + } + return levels +} + +// railLead is the lead cells a family row hangs by: two a level, drawn as the +// tree the manual documents — `├ ` off the parent while a sibling follows the +// row, `└ ` where the row closes its family, `│ ` down every level above it +// whose row had a sibling still to come, and two spaces where a branch has +// ended. `last` is those levels' own answers, shallowest first, the last one +// the row's own level, and a row with none draws nothing. +// +// The glyphs come through the vocabulary's one door ([palette.glyph]) like the +// tasks place draws its own kin with, so one tree shape is spelled one way on +// this surface. THE WIDTH IS MEASURED AND NOT COUNTED, the same bargain +// hometree.go strikes: a level is two cells today, and a row that assumed so +// would draw a broken column the day one of them is respelled — or on the +// terminal where a box-drawing glyph is reported wide. +// +// AN UNDER-ROW DRAWS NO ELBOW. The block a part says under its title stands +// between that row and whatever follows it — the sibling next to it, or its +// own parts — so the same flags with `elbow` false keep the trunk running +// where rows follow and leave air where the branch closed, and the elbow is +// the row's, drawn once. +func (a *app) railLead(last []bool, elbow bool) string { + var out strings.Builder + for i, closed := range last { + if elbow && i == len(last)-1 { + if closed { + out.WriteString(a.pal.glyph(tokens.GTreeLast)) + } else { + out.WriteString(a.pal.glyph(tokens.GTreeBranch)) + } + out.WriteString(" ") + continue + } + // An ancestor level carries the trunk while its row had a sibling + // still to come, and the air the branch leaves when it was the last. + if closed { + out.WriteString(" ") + continue + } + out.WriteString(a.pal.glyph(tokens.GTreeVert)) + out.WriteString(" ") + } + return out.String() +} + // planRailLines draws a run's parts under a row, each one THROUGH THE NODE // RENDERER and one line each, as every task on the side column is (sidecol.go): -// depth is how far under the row they hang, two cells a level, which is the -// only shape the column gives a family now that its forest is gone. +// a level of family costs two cells, drawn as the tree's own connectors +// ([app.railLead]) — `last` is the levels above the parts, each saying whether +// that ancestor was the last child of its own, and the parts' own levels hang +// off the row above them, `├ ` while a sibling follows and `└ ` where one +// closes the family. // // Every line a part draws carries its store id, which is what makes it a door // onto that task's page ([app.openRailPlan]). -func (a *app) planRailLines(kids []*planTwig, depth, width int) []railLine { +func (a *app) planRailLines(kids []*planTwig, last []bool, width, levels int) []railLine { var out []railLine - lead := strings.Repeat(" ", depth) - for _, kid := range kids { - text := a.railEntryRow(railEntry{node: planRailNode(kid.row)}, max(width-len(lead), 0)) + for i, kid := range kids { + at := make([]bool, len(last), len(last)+1) + copy(at, last) + at = append(at, i == len(kids)-1) + hang := at + if len(hang) > levels { + hang = hang[len(hang)-levels:] + } + lead := a.railLead(hang, true) + room := max(width-ansi.StringWidth(lead), 0) + if room < railLeadFloor { + continue + } + text := a.railEntryRow(railEntry{node: planRailNode(kid.row)}, room) out = append(out, railLine{text: lead + text, entry: -1, plan: kid.row.ID, head: true}) - out = append(out, a.planRailLines(kid.kids, depth+1, width)...) + out = append(out, a.planRailLines(kid.kids, hang, width, levels)...) } return out } @@ -249,18 +328,30 @@ func (a *app) planRailLines(kids []*planTwig, depth, width int) []railLine { // the side column gave up: under each part's one line stand the lines the // column moved to its hint ([app.railUnder]), what the part is doing and what // it is costing, so the page still names a call in flight the way the rail -// once did beside the row. -func (a *app) planPageLines(kids []*planTwig, depth, width int) []railLine { +// once did beside the row. The page's own task stands in its head, so its +// parts hang a level in here, the same connectors the rail draws and the same +// trunk running through the under-block ([app.railLead]). +func (a *app) planPageLines(kids []*planTwig, last []bool, width, levels int) []railLine { var out []railLine - lead := strings.Repeat(" ", depth) - room := max(width-len(lead), 0) - for _, kid := range kids { + for i, kid := range kids { + at := make([]bool, len(last), len(last)+1) + copy(at, last) + at = append(at, i == len(kids)-1) + hang := at + if len(hang) > levels { + hang = hang[len(hang)-levels:] + } + lead := a.railLead(hang, true) + room := max(width-ansi.StringWidth(lead), 0) + if room < railLeadFloor { + continue + } node := planRailNode(kid.row) out = append(out, railLine{text: lead + a.railEntryRow(railEntry{node: node}, room), entry: -1, plan: kid.row.ID, head: true}) for _, under := range a.railUnder(node, max(room-4, 0)) { - out = append(out, railLine{text: lead + " " + under, entry: -1, plan: kid.row.ID}) + out = append(out, railLine{text: a.railLead(hang, false) + " " + under, entry: -1, plan: kid.row.ID}) } - out = append(out, a.planPageLines(kid.kids, depth+1, width)...) + out = append(out, a.planPageLines(kid.kids, hang, width, levels)...) } return out } @@ -270,5 +361,5 @@ func (a *app) planPageLines(kids []*planTwig, depth, width int) []railLine { func (a *app) planRailRoot(twig *planTwig, width int) []railLine { text := a.railEntryRow(railEntry{node: planRailNode(twig.row)}, width) out := []railLine{{text: text, entry: -1, plan: twig.row.ID, head: true}} - return append(out, a.planRailLines(twig.kids, 1, width)...) + return append(out, a.planRailLines(twig.kids, nil, width, railLevels(width))...) } diff --git a/internal/tui3/planroom.go b/internal/tui3/planroom.go index 3345e768b4..c7d7dc775d 100644 --- a/internal/tui3/planroom.go +++ b/internal/tui3/planroom.go @@ -406,7 +406,7 @@ func (a *app) planRoomPartRows(width int) []row { } if len(plan.page.Children) > 0 { out = append(out, row{entry: -1}, row{text: a.pal.dim(fit(planRoomPartsWord, width)), entry: -1}) - for _, line := range a.planPageLines(planTwigsOf(plan.page.Children), 0, min(width, planPageKinWidth)) { + for _, line := range a.planPageLines(planTwigsOf(plan.page.Children), nil, min(width, planPageKinWidth), railLevels(width)) { out = append(out, row{text: line.text, entry: -1, plan: line.plan}) } } diff --git a/internal/tui3/task.go b/internal/tui3/task.go index 4a6e3431d9..cf9d18ab61 100644 --- a/internal/tui3/task.go +++ b/internal/tui3/task.go @@ -3418,7 +3418,7 @@ func (a *app) railDrawnView(height int) ([]railLine, int) { } if node := carrier[run]; node != nil { if at, ok := visible[node]; ok { - lines := a.planRailLines(kids, 1, width) + lines := a.planRailLines(kids, nil, width, railLevels(width)) under[at] = append(under[at], lines...) markPlan(lines) drawnPlan[run] = true @@ -4309,8 +4309,9 @@ func (a *app) railEntryRow(e railEntry, width int) string { return "" } const indent = " " + fig := a.railFigWord(node) glyph := a.railTreeGlyph(node) - room := width - len(indent) - 2 + room := width - len(indent) - ansi.StringWidth(fig) - 2 // A program's badge stays on its row when the side column is compact. // The time gives up cells first, then the badge shortens, while the title // keeps enough room to name the work. @@ -4328,7 +4329,7 @@ func (a *app) railEntryRow(e railEntry, width int) string { age = "" } title, w := fitWidth(node.title, room) - line := indent + glyph + " " + a.railTitle(node, title) + a.pal.programAfter(wears) + line := indent + a.pal.dim(fig) + " " + glyph + " " + a.railTitle(node, title) + a.pal.programAfter(wears) if age != "" { line += strings.Repeat(" ", max(room-w, 0)+1) + a.pal.dim(age) } diff --git a/internal/tui3/taskident.go b/internal/tui3/taskident.go index cc19d827ef..cb7805ed02 100644 --- a/internal/tui3/taskident.go +++ b/internal/tui3/taskident.go @@ -35,7 +35,7 @@ import ( // the marker's own argument applied to one column: the rail holds nothing // but tasks, so there is nothing there for "this row is a task" to tell // apart, and the two cells are worth more to the name on a surface -// twenty-four columns wide (task.go's [app.railLead]). +// twenty-four columns wide (task.go's [app.railEntryRow]). // // THE MARKER USED TO BE EIGHT SHAPES IN SIX HUES, hashed off the id — a private // alphabet in which ◆ teal was task 3 and ▲ amber was task 5. The argument was