From 99aaba75afd50e13e4b6c45c87bd318901ec2acf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 23:09:49 +0200 Subject: [PATCH 1/2] Support --pick-team across all hierarchy levels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Added path-addressed pick/undo/move functions in group.ts: rebuildTeamHierarchy (inverse of flattenTeamHierarchy), plus tree-aware applyTeamPickInTree, undoSectionPickInTree, moveRepoToSectionInTree, undoPickedRepoInTree, and findCombinedSectionPaths — since a bare section label is no longer unique across a groupByTeamHierarchy tree (e.g. "other" can appear under multiple parents), these address a section by its full root-to-node path, joined with " > " wherever a single string key is needed (pickedFrom, confirmedPicks). - Added getSectionPath (render/rows.ts) to reconstruct a section row's full ancestor path from the flat rows list, by scanning backward for the most recent row at each decreasing sectionLevel. - Wired tui.ts: pick mode (p) captures the row's full sectionPath; confirm/undo/re-pick branch to the tree-aware functions when the path has 2+ segments, and keep using the existing flat functions unchanged for top-level (depth-1) sections — byte-for-byte the same behavior as before for every current (flat) CLI invocation. - render.ts's pick-mode section match now compares the full path instead of the bare label, so the same label at a different tree branch isn't targeted by mistake. Fix: getSectionPath was referenced in render.ts without a local import (only re-exported), causing a runtime ReferenceError when entering pick mode — added the missing import and a regression test that actually exercises renderGroups with teamPickMode active (previously untested). CLI parsing/registration of hierarchy-aware --pick-team paths is intentionally out of scope here — tracked by issue #182. --- src/group.test.ts | 267 +++++++++++++++++++++++++++++++++++++++++ src/group.ts | 288 +++++++++++++++++++++++++++++++++++++++++++++ src/render.test.ts | 76 ++++++++++++ src/render.ts | 18 ++- src/render/rows.ts | 31 +++++ src/tui.ts | 96 ++++++++++++--- 6 files changed, 758 insertions(+), 18 deletions(-) diff --git a/src/group.test.ts b/src/group.test.ts index f611cda..4fc8c6e 100644 --- a/src/group.test.ts +++ b/src/group.test.ts @@ -1,15 +1,21 @@ import { describe, expect, it } from "bun:test"; import { applyTeamPick, + applyTeamPickInTree, consolidateTeamHierarchy, + findCombinedSectionPaths, flattenTeamHierarchy, flattenTeamSections, groupByTeamHierarchy, groupByTeamPrefix, moveRepoToSection, + moveRepoToSectionInTree, + rebuildTeamHierarchy, rebuildTeamSections, undoPickedRepo, + undoPickedRepoInTree, undoSectionPick, + undoSectionPickInTree, } from "./group.ts"; import type { RepoGroup, TeamSection } from "./types.ts"; @@ -494,6 +500,267 @@ describe("flattenTeamHierarchy", () => { }); }); +// ─── rebuildTeamHierarchy ────────────────────────────────────────────────────── + +describe("rebuildTeamHierarchy", () => { + it("round-trips a 2-level tree through flattenTeamHierarchy", () => { + const groups = [ + makeGroup("org/a", ["gamme-client", "squad-dashboard"]), + makeGroup("org/b", ["gamme-client", "squad-billing"]), + ]; + const original = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const rebuilt = rebuildTeamHierarchy(flattenTeamHierarchy(original)); + expect(rebuilt).toEqual(original); + }); + + it("round-trips a tree where a node has both own groups and children (overlap parent)", () => { + const groups = [ + makeGroup("org/a", ["gamme-lead-client"]), + makeGroup("org/b", ["gamme-lead-client-p1"]), + ]; + const original = groupByTeamHierarchy(groups, [["gamme-"]]); + const rebuilt = rebuildTeamHierarchy(flattenTeamHierarchy(original)); + expect(rebuilt).toEqual(original); + }); + + it("round-trips multiple independent top-level chains", () => { + const groups = [ + makeGroup("org/a", ["gamme-client", "squad-dashboard"]), + makeGroup("org/b", ["chapter-backend"]), + makeGroup("org/c", []), + ]; + const original = groupByTeamHierarchy(groups, [["gamme-", "squad-"], ["chapter-"]]); + const rebuilt = rebuildTeamHierarchy(flattenTeamHierarchy(original)); + expect(rebuilt).toEqual(original); + }); + + it("returns an empty array for an empty input", () => { + expect(rebuildTeamHierarchy([])).toEqual([]); + }); +}); + +// ─── applyTeamPickInTree ──────────────────────────────────────────────────────── + +describe("applyTeamPickInTree", () => { + it("behaves like applyTeamPick for a top-level (depth-1) path", () => { + const flatSections: TeamSection[] = [ + { label: "squad-frontend", groups: [makeGroup("org/a", ["squad-frontend"])] }, + { + label: "squad-frontend + squad-mobile", + groups: [makeGroup("org/shared", ["squad-frontend", "squad-mobile"])], + }, + ]; + const viaFlat = applyTeamPick(flatSections, "squad-frontend + squad-mobile", "squad-frontend"); + const viaTree = applyTeamPickInTree( + flatSections, + ["squad-frontend + squad-mobile"], + "squad-frontend", + ); + expect(viaTree).toEqual(viaFlat); + }); + + it("reassigns a nested combined section to a sibling at the same depth", () => { + const groups = [ + makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"]), + makeGroup("org/a", ["gamme-client", "squad-a"]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const updated = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-a"); + const gamme = updated.find((s) => s.label === "gamme-client")!; + const childLabels = (gamme.children ?? []).map((c) => c.label); + expect(childLabels).not.toContain("squad-a + squad-b"); + const squadA = gamme.children!.find((c) => c.label === "squad-a")!; + expect(squadA.groups.map((g) => g.repoFullName).toSorted()).toEqual(["org/a", "org/shared"]); + }); + + it("tags moved repos with pickedFrom = joined path", () => { + const groups = [makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const updated = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-a"); + const gamme = updated.find((s) => s.label === "gamme-client")!; + const squadA = gamme.children!.find((c) => c.label === "squad-a")!; + expect(squadA.groups[0].pickedFrom).toBe("gamme-client > squad-a + squad-b"); + }); + + it("creates a new sibling section when the chosen team has none yet", () => { + const groups = [makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const updated = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-b"); + const gamme = updated.find((s) => s.label === "gamme-client")!; + expect(gamme.children!.map((c) => c.label)).toContain("squad-b"); + }); + + it("is a no-op when a path segment is not found", () => { + const groups = [makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const result = applyTeamPickInTree(tree, ["nope", "squad-a + squad-b"], "squad-a"); + expect(result).toEqual(tree); + }); + + it("returns sections unchanged for an empty combinedPath", () => { + const groups = [makeGroup("org/a")]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + expect(applyTeamPickInTree(tree, [], "squad-a")).toBe(tree); + }); +}); + +// ─── undoSectionPickInTree ────────────────────────────────────────────────────── + +describe("undoSectionPickInTree", () => { + it("restores every repo tagged with the matching pickedFrom back to the combined section", () => { + const groups = [ + makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"]), + makeGroup("org/a", ["gamme-client", "squad-a"]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const picked = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-a"); + const restored = undoSectionPickInTree(picked, "gamme-client > squad-a + squad-b"); + const gamme = restored.find((s) => s.label === "gamme-client")!; + const childLabels = gamme.children!.map((c) => c.label).toSorted(); + expect(childLabels).toEqual(["squad-a", "squad-a + squad-b"]); + const combined = gamme.children!.find((c) => c.label === "squad-a + squad-b")!; + expect(combined.groups.map((g) => g.repoFullName)).toEqual(["org/shared"]); + expect(combined.groups[0].pickedFrom).toBeUndefined(); + }); + + it("drops a section left empty after the restore", () => { + const groups = [makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const picked = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-a"); + const restored = undoSectionPickInTree(picked, "gamme-client > squad-a + squad-b"); + const gamme = restored.find((s) => s.label === "gamme-client")!; + // squad-a only ever held the moved repo — it must be gone after the restore. + expect(gamme.children!.map((c) => c.label)).not.toContain("squad-a"); + }); + + it("is a no-op when no repo has a matching pickedFrom", () => { + const groups = [makeGroup("org/a", ["squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + expect(undoSectionPickInTree(tree, "nope")).toBe(tree); + }); + + it("behaves like undoSectionPick for a top-level (depth-1) path", () => { + const flatSections: TeamSection[] = [ + { + label: "squad-frontend", + groups: [{ ...makeGroup("org/shared"), pickedFrom: "squad-frontend + squad-mobile" }], + }, + ]; + const viaFlat = undoSectionPick( + flattenTeamSections(flatSections), + "squad-frontend + squad-mobile", + ); + const viaTree = flattenTeamHierarchy( + undoSectionPickInTree( + rebuildTeamHierarchy(flattenTeamSections(flatSections)), + "squad-frontend + squad-mobile", + ), + ); + expect(viaTree.map((g) => g.repoFullName)).toEqual(viaFlat.map((g) => g.repoFullName)); + }); +}); + +// ─── moveRepoToSectionInTree ──────────────────────────────────────────────────── + +describe("moveRepoToSectionInTree", () => { + it("moves a repo to a sibling under the given parent path", () => { + const groups = [ + makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"]), + makeGroup("org/a", ["gamme-client", "squad-a"]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const picked = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-a"); + const moved = moveRepoToSectionInTree(picked, "org/shared", ["gamme-client"], "squad-b"); + const gamme = moved.find((s) => s.label === "gamme-client")!; + const squadB = gamme.children!.find((c) => c.label === "squad-b")!; + expect(squadB.groups.map((g) => g.repoFullName)).toEqual(["org/shared"]); + const squadA = gamme.children!.find((c) => c.label === "squad-a")!; + expect(squadA.groups.map((g) => g.repoFullName)).toEqual(["org/a"]); + }); + + it("creates the target section when it doesn't exist yet", () => { + const groups = [makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const picked = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-a"); + const moved = moveRepoToSectionInTree(picked, "org/shared", ["gamme-client"], "squad-c"); + const gamme = moved.find((s) => s.label === "gamme-client")!; + expect(gamme.children!.map((c) => c.label)).toContain("squad-c"); + }); + + it("is a no-op when the repo is not found anywhere in the tree", () => { + const groups = [makeGroup("org/a", ["squad-a"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + expect(moveRepoToSectionInTree(tree, "org/does-not-exist", [], "squad-b")).toBe(tree); + }); +}); + +// ─── undoPickedRepoInTree ─────────────────────────────────────────────────────── + +describe("undoPickedRepoInTree", () => { + it("restores a single picked repo back to its original combined section", () => { + const groups = [ + makeGroup("org/shared", ["gamme-client", "squad-a", "squad-b"]), + makeGroup("org/a", ["gamme-client", "squad-a"]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const picked = applyTeamPickInTree(tree, ["gamme-client", "squad-a + squad-b"], "squad-a"); + const restored = undoPickedRepoInTree(picked, "org/shared"); + const gamme = restored.find((s) => s.label === "gamme-client")!; + const combined = gamme.children!.find((c) => c.label === "squad-a + squad-b")!; + expect(combined.groups.map((g) => g.repoFullName)).toEqual(["org/shared"]); + expect(combined.groups[0].pickedFrom).toBeUndefined(); + // The other repo that was also moved stays picked. + const squadA = gamme.children!.find((c) => c.label === "squad-a")!; + expect(squadA.groups.map((g) => g.repoFullName)).toEqual(["org/a"]); + }); + + it("is a no-op for a repo with no pickedFrom", () => { + const groups = [makeGroup("org/a", ["squad-a"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + expect(undoPickedRepoInTree(tree, "org/a")).toBe(tree); + }); + + it("is a no-op when the repo is not found", () => { + const groups = [makeGroup("org/a", ["squad-a"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + expect(undoPickedRepoInTree(tree, "org/does-not-exist")).toBe(tree); + }); +}); + +// ─── findCombinedSectionPaths ─────────────────────────────────────────────────── + +describe("findCombinedSectionPaths", () => { + it("finds a top-level combined section", () => { + const groups = [makeGroup("org/a", ["squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + expect(findCombinedSectionPaths(tree)).toEqual([["squad-a + squad-b"]]); + }); + + it("finds a nested combined section with its full ancestor path", () => { + const groups = [makeGroup("org/a", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + expect(findCombinedSectionPaths(tree)).toEqual([["gamme-client", "squad-a + squad-b"]]); + }); + + it("returns an empty array when there is no combined section", () => { + const groups = [makeGroup("org/a", ["squad-a"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + expect(findCombinedSectionPaths(tree)).toEqual([]); + }); + + it("finds multiple combined sections across different branches", () => { + const groups = [ + makeGroup("org/a", ["gamme-x", "squad-a", "squad-b"]), + makeGroup("org/b", ["gamme-y", "chapter-a", "chapter-b"]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"], ["gamme-"]]); + // Both repos start with a different top-level "gamme-" match, so this + // exercises two independent combined sections at the same nested depth. + const paths = findCombinedSectionPaths(tree); + expect(paths).toContainEqual(["gamme-x", "squad-a + squad-b"]); + }); +}); + // ─── flattenTeamSections ────────────────────────────────────────────────────── describe("flattenTeamSections", () => { diff --git a/src/group.ts b/src/group.ts index 8ff5357..aba990d 100644 --- a/src/group.ts +++ b/src/group.ts @@ -439,6 +439,294 @@ function firstDivergingIndex(a: PathEntry[], b: PathEntry[]): number { return i; } +// ─── Hierarchical (path-addressed) pick-team ────────────────────────────────── +// +// A section `label` alone is not unique across a `groupByTeamHierarchy` tree +// (e.g. `"other"` can appear under multiple parents), so hierarchy-aware pick +// operations address a section by its full root-to-node `path` (an array of +// ancestor labels ending with the section's own label). The same path, +// joined with `" > "`, is stored in `pickedFrom` so `undoSectionPickInTree` +// can find every repo picked from that exact section later. For a top-level +// section (`path.length === 1`), this is behaviourally identical to the flat +// `applyTeamPick` / `undoSectionPick` (same joined string as the bare label). + +const PATH_SEPARATOR = " > "; + +/** + * Reconstructs a `groupByTeamHierarchy` tree from a flat `RepoGroup[]` + * produced by `flattenTeamHierarchy`. The inverse of `flattenTeamHierarchy`. + * + * Walks the flat list maintaining a "current node per depth" stack; each + * `sectionPath` entry pushes (or replaces, if shallower) a node onto that + * stack, and repos are appended to whichever node is deepest on the stack + * at the time they're encountered — correctly handling nodes that own repos + * directly *and* have nested children (see `TeamSection`). + */ +export function rebuildTeamHierarchy(groups: RepoGroup[]): TeamSection[] { + const roots: TeamSection[] = []; + let stack: TeamSection[] = []; + + for (const g of groups) { + if (g.sectionPath !== undefined && g.sectionPath.length > 0) { + stack = stack.slice(0, g.sectionPath[0].level); + for (const entry of g.sectionPath) { + const node: TeamSection = { label: entry.label, groups: [], level: entry.level }; + if (stack.length === 0) { + roots.push(node); + } else { + const parent = stack[stack.length - 1]; + parent.children = [...(parent.children ?? []), node]; + } + stack.push(node); + } + } + const { sectionPath: _removed, ...rest } = g; + void _removed; + const repo = rest as RepoGroup; + if (stack.length === 0) { + // No section context at all — shouldn't happen for hierarchy-produced + // input, but keep the repo visible as its own top-level entry rather + // than silently dropping it. + roots.push({ label: repo.repoFullName, groups: [repo] }); + continue; + } + const leaf = stack[stack.length - 1]; + leaf.groups = [...leaf.groups, repo]; + } + + return roots; +} + +/** + * Navigates to the node at `parentPath` (ancestor labels, NOT including the + * target section's own label) and replaces its children — or the top-level + * `sections` array when `parentPath` is empty — with `updater`'s result. + * + * If a segment of `parentPath` is missing (e.g. an ancestor was pruned by + * `removeMatchingRepos` because moving its only repo away left it with no + * groups and no children), it is recreated fresh rather than silently + * no-op-ing — otherwise undoing/re-picking the *last* repo under a branch + * could make that branch permanently unreachable. + */ +function updateSiblingsAtPath( + sections: TeamSection[], + parentPath: string[], + updater: (siblings: TeamSection[]) => TeamSection[], +): TeamSection[] { + if (parentPath.length === 0) return updater(sections); + + const [head, ...rest] = parentPath; + const idx = sections.findIndex((s) => s.label === head); + if (idx === -1) { + const children = updateSiblingsAtPath([], rest, updater); + // Only materialize the missing ancestor if the update actually produced + // something inside it — otherwise this is a genuine no-op (e.g. a + // typo'd path) and adding an empty node here would pollute the tree. + return children.length === 0 ? sections : [...sections, { label: head, groups: [], children }]; + } + + const updatedChildren = updateSiblingsAtPath(sections[idx].children ?? [], rest, updater); + return sections.map((s, i) => (i === idx ? { ...s, children: updatedChildren } : s)); +} + +/** + * Merges `repos` into the sibling named `label` under `parentPath`, creating + * it — inserted before an `"other"` sibling, or appended — if it doesn't + * already exist. Shared by every hierarchical pick/undo/move operation below. + */ +function mergeOrCreateAtPath( + sections: TeamSection[], + parentPath: string[], + label: string, + repos: RepoGroup[], +): TeamSection[] { + return updateSiblingsAtPath(sections, parentPath, (siblings) => { + const idx = siblings.findIndex((s) => s.label === label); + if (idx !== -1) { + return siblings.map((s, i) => (i === idx ? { ...s, groups: [...s.groups, ...repos] } : s)); + } + const newSection: TeamSection = { label, groups: repos }; + const otherIdx = siblings.findIndex((s) => s.label === "other"); + return otherIdx === -1 + ? [...siblings, newSection] + : [...siblings.slice(0, otherIdx), newSection, ...siblings.slice(otherIdx)]; + }); +} + +/** + * Removes every repo matching `predicate` anywhere in the tree, dropping + * sections left with no groups and no children afterward. Returns the + * stripped tree; matched repos (untouched, `pickedFrom` included) are + * appended to `collected` in the order encountered. + */ +function removeMatchingRepos( + nodes: TeamSection[], + predicate: (g: RepoGroup) => boolean, + collected: RepoGroup[], +): TeamSection[] { + return nodes + .map((node) => { + const kept: RepoGroup[] = []; + for (const g of node.groups) { + if (predicate(g)) collected.push(g); + else kept.push(g); + } + const children = node.children + ? removeMatchingRepos(node.children, predicate, collected) + : undefined; + return { ...node, groups: kept, ...(children ? { children } : {}) }; + }) + .filter((node) => node.groups.length > 0 || (node.children?.length ?? 0) > 0); +} + +/** Strips `pickedFrom` from a repo (pure — returns a new object). */ +function stripPickedFrom(g: RepoGroup): RepoGroup { + const { pickedFrom: _p, ...rest } = g; + void _p; + return rest as RepoGroup; +} + +/** + * Tree-aware equivalent of `applyTeamPick`: reassigns every repo of the + * combined section identified by `combinedPath` (e.g. + * `["gamme-client", "squad-a + squad-b"]`) to a sibling section named + * `chosenTeam` at that same depth (appended to it if it already exists, + * otherwise created in its place). Moved repos are tagged with + * `pickedFrom = combinedPath.join(" > ")`. + * + * No-op (returns `sections` unchanged) if any segment of `combinedPath` does + * not resolve to an existing node. Pure — does not mutate `sections`. + */ +export function applyTeamPickInTree( + sections: TeamSection[], + combinedPath: string[], + chosenTeam: string, +): TeamSection[] { + if (combinedPath.length === 0) return sections; + const parentPath = combinedPath.slice(0, -1); + const combinedLabel = combinedPath[combinedPath.length - 1]; + const pathKey = combinedPath.join(PATH_SEPARATOR); + + return updateSiblingsAtPath(sections, parentPath, (siblings) => { + const idx = siblings.findIndex((s) => s.label === combinedLabel); + if (idx === -1) return siblings; + + const reposToMove = siblings[idx].groups.map((g) => ({ ...g, pickedFrom: pathKey })); + const remaining = siblings.filter((_, i) => i !== idx); + + const targetIdx = remaining.findIndex((s) => s.label === chosenTeam); + if (targetIdx !== -1) { + return remaining.map((s, i) => + i === targetIdx ? { ...s, groups: [...s.groups, ...reposToMove] } : s, + ); + } + + const newSection: TeamSection = { + label: chosenTeam, + groups: reposToMove, + level: siblings[idx].level, + }; + const result = [...remaining]; + result.splice(idx, 0, newSection); + return result; + }); +} + +/** + * Tree-aware equivalent of `undoSectionPick`: restores every repo anywhere in + * the tree whose `pickedFrom` matches `combinedPathString` (the joined path + * produced by `applyTeamPickInTree`) back to the section at that path, + * recreating it if it no longer exists. `pickedFrom` is stripped from + * restored repos. Sections left empty after the removal are dropped. + * + * No-op (returns `sections` unchanged) if no repo has a matching `pickedFrom`. + * Pure — does not mutate `sections`. + */ +export function undoSectionPickInTree( + sections: TeamSection[], + combinedPathString: string, +): TeamSection[] { + const collected: RepoGroup[] = []; + const stripped = removeMatchingRepos( + sections, + (g) => g.pickedFrom === combinedPathString, + collected, + ); + if (collected.length === 0) return sections; + + const combinedPath = combinedPathString.split(PATH_SEPARATOR); + const parentPath = combinedPath.slice(0, -1); + const combinedLabel = combinedPath[combinedPath.length - 1]; + return mergeOrCreateAtPath(stripped, parentPath, combinedLabel, collected.map(stripPickedFrom)); +} + +/** + * Tree-aware equivalent of `moveRepoToSection`: moves the repo identified by + * `repoFullName` (wherever it currently sits in the tree) to a sibling named + * `targetTeam` under `parentPath` (created in place if absent). The repo's + * `pickedFrom` is preserved so a later undo can still restore it. + * + * No-op (returns `sections` unchanged) if the repo isn't found in the tree. + * Pure — does not mutate `sections`. + */ +export function moveRepoToSectionInTree( + sections: TeamSection[], + repoFullName: string, + parentPath: string[], + targetTeam: string, +): TeamSection[] { + const collected: RepoGroup[] = []; + const stripped = removeMatchingRepos(sections, (g) => g.repoFullName === repoFullName, collected); + if (collected.length === 0) return sections; + return mergeOrCreateAtPath(stripped, parentPath, targetTeam, collected); +} + +/** + * Tree-aware equivalent of `undoPickedRepo`: restores a single previously + * picked repo (identified by `repoFullName`) back to its original combined + * section (read from its own `pickedFrom`), recreating that section if it no + * longer exists. `pickedFrom` is stripped from the restored repo. + * + * No-op (returns `sections` unchanged) if the repo isn't found or has no + * `pickedFrom`. Pure — does not mutate `sections`. + */ +export function undoPickedRepoInTree(sections: TeamSection[], repoFullName: string): TeamSection[] { + const collected: RepoGroup[] = []; + const stripped = removeMatchingRepos( + sections, + (g) => g.repoFullName === repoFullName && g.pickedFrom !== undefined, + collected, + ); + if (collected.length === 0) return sections; + + const combinedPathString = collected[0].pickedFrom!; + const combinedPath = combinedPathString.split(PATH_SEPARATOR); + const parentPath = combinedPath.slice(0, -1); + const combinedLabel = combinedPath[combinedPath.length - 1]; + return mergeOrCreateAtPath(stripped, parentPath, combinedLabel, collected.map(stripPickedFrom)); +} + +/** + * Returns the full path (ancestor labels, root first) of every combined + * (`"a + b"`) section anywhere in the tree — used to resolve an unqualified + * `--pick-team` label (auto-pick when exactly one match) or report the + * available candidates when it's ambiguous or not found. + */ +export function findCombinedSectionPaths(sections: TeamSection[]): string[][] { + const paths: string[][] = []; + + function visit(nodes: TeamSection[], ancestors: string[]): void { + for (const node of nodes) { + const path = [...ancestors, node.label]; + if (node.label.includes(" + ")) paths.push(path); + if (node.children) visit(node.children, path); + } + } + + visit(sections, []); + return paths; +} + // ─── Internal helpers ───────────────────────────────────────────────────────── /** diff --git a/src/render.test.ts b/src/render.test.ts index f193c2f..c184415 100644 --- a/src/render.test.ts +++ b/src/render.test.ts @@ -2109,6 +2109,82 @@ describe("renderGroups — hierarchical section headings (sectionLevel)", () => }); }); +// ─── renderGroups — team pick mode section bar ──────────────────────────────── +// Regression: getSectionPath must be a real local import (not just re-exported) +// for this branch to even run — see the crash reported on issue #181. + +describe("renderGroups — team pick mode section bar", () => { + it("shows the pick bar on a flat (sectionLabel) combined section", () => { + const groups = [ + { ...makeGroup("org/repoA", ["a.ts"], true), sectionLabel: "squad-a + squad-b" }, + ]; + const rows = buildRows(groups); + const out = renderGroups(groups, 0, rows, 40, 0, "q", "org", { + termWidth: 80, + teamPickMode: { + active: true, + sectionLabel: "squad-a + squad-b", + candidates: ["squad-a", "squad-b"], + focusedIndex: 0, + }, + }); + const stripped = out.replace(/\x1b\[[0-9;]*m/g, ""); + expect(stripped).toContain("[ squad-a ]"); + }); + + it("shows the pick bar on the hierarchical section whose full path matches", () => { + const groups = [ + { + ...makeGroup("org/repoA", ["a.ts"], true), + sectionPath: [ + { label: "gamme-client", level: 0 }, + { label: "squad-a + squad-b", level: 1 }, + ], + }, + ]; + const rows = buildRows(groups); + const out = renderGroups(groups, 0, rows, 40, 0, "q", "org", { + termWidth: 80, + teamPickMode: { + active: true, + sectionLabel: "squad-a + squad-b", + sectionPath: ["gamme-client", "squad-a + squad-b"], + candidates: ["squad-a", "squad-b"], + focusedIndex: 0, + }, + }); + const stripped = out.replace(/\x1b\[[0-9;]*m/g, ""); + expect(stripped).toContain("[ squad-a ]"); + }); + + it("does not show the pick bar on a same-label section at a different path", () => { + const groups = [ + { + ...makeGroup("org/repoA", ["a.ts"], true), + sectionPath: [ + { label: "gamme-other", level: 0 }, + { label: "squad-a + squad-b", level: 1 }, + ], + }, + ]; + const rows = buildRows(groups); + const out = renderGroups(groups, 0, rows, 40, 0, "q", "org", { + termWidth: 80, + teamPickMode: { + active: true, + sectionLabel: "squad-a + squad-b", + // Targets a DIFFERENT parent than the one actually rendered above. + sectionPath: ["gamme-client", "squad-a + squad-b"], + candidates: ["squad-a", "squad-b"], + focusedIndex: 0, + }, + }); + const stripped = out.replace(/\x1b\[[0-9;]*m/g, ""); + expect(stripped).not.toContain("[ squad-a ]"); + expect(stripped).toContain("squad-a + squad-b"); + }); +}); + // ─── renderGroups — re-pick mode hints bar ──────────────────────────────────── describe("renderGroups — re-pick mode hints bar", () => { diff --git a/src/render.ts b/src/render.ts index 21122f9..5719edc 100644 --- a/src/render.ts +++ b/src/render.ts @@ -2,7 +2,7 @@ import * as style from "./style.ts"; import type { FilterTarget, RepoGroup, Row, TextMatchSegment } from "./types.ts"; import { highlightFragment } from "./render/highlight.ts"; import { buildFilterStats, type FilterStats } from "./render/filter.ts"; -import { rowTerminalLines } from "./render/rows.ts"; +import { getSectionPath, rowTerminalLines } from "./render/rows.ts"; import { buildMatchCountLabel, buildSummaryFull } from "./render/summary.ts"; import { renderTeamPickHeader } from "./render/team-pick.ts"; import { visibleWidth, stripAnsi, clipToWidth } from "./render/terminal.ts"; @@ -23,6 +23,7 @@ export { buildRows, isCursorVisible, normalizeScrollOffset, + getSectionPath, } from "./render/rows.ts"; export { buildMatchCountLabel, @@ -274,6 +275,11 @@ interface RenderOptions { teamPickMode?: { active: boolean; sectionLabel: string; + /** Full ancestor path (root first, ending with `sectionLabel`) used to + * unambiguously match the target row in a `groupByTeamHierarchy` tree, + * where the same label can appear under multiple parents. Falls back to + * matching on `sectionLabel` alone when absent (flat sections). */ + sectionPath?: string[]; candidates: string[]; focusedIndex: number; }; @@ -532,8 +538,16 @@ export function renderGroups( // Emit the blank separator only when there are rows above in the viewport. if (usedLines > 0) lines.push(""); // Feat: team pick mode — show pick bar when active for this section — see issue #85. + // Fix: match by full ancestor path (not just label) when available, so + // the same label at a different tree branch isn't targeted by mistake + // in a groupByTeamHierarchy tree — see issue #181. const pickMode = opts.teamPickMode; - if (pickMode?.active && pickMode.sectionLabel === row.sectionLabel) { + if ( + pickMode?.active && + (pickMode.sectionPath !== undefined + ? pickMode.sectionPath.join(" > ") === getSectionPath(rows, i).join(" > ") + : pickMode.sectionLabel === row.sectionLabel) + ) { // Fix: clip pick bar to (termWidth - 3) so "── " + bar never wraps — see issue #121. const bar = renderTeamPickHeader( pickMode.candidates, diff --git a/src/render/rows.ts b/src/render/rows.ts index e540033..59d0add 100644 --- a/src/render/rows.ts +++ b/src/render/rows.ts @@ -134,6 +134,37 @@ function firstDivergingPathIndex( return i; } +/** + * Reconstructs the full ancestor path (root-to-node labels) for the section + * row at `rowIndex`, by scanning backward through `rows` for the most recent + * section row at each decreasing `sectionLevel`. Used to identify a + * combined-label section unambiguously in a `groupByTeamHierarchy` tree, + * where the same label (e.g. `"other"`) can appear under multiple parents. + * + * Returns `[]` if the row at `rowIndex` is not a section row. For a flat + * `groupByTeamPrefix` section (`sectionLevel` 0 or unset), the path is just + * the row's own label. + */ +export function getSectionPath(rows: Row[], rowIndex: number): string[] { + const row = rows[rowIndex]; + if (row?.type !== "section" || row.sectionLabel === undefined) return []; + + const path: string[] = [row.sectionLabel]; + let neededLevel = (row.sectionLevel ?? 0) - 1; + for (let i = rowIndex - 1; i >= 0 && neededLevel >= 0; i--) { + const r = rows[i]; + if ( + r.type === "section" && + r.sectionLabel !== undefined && + (r.sectionLevel ?? 0) === neededLevel + ) { + path.unshift(r.sectionLabel); + neededLevel--; + } + } + return path; +} + /** * Normalises scrollOffset downward so the viewport is always packed from the * bottom. After a fold, a filter change, or navigating near the end of the diff --git a/src/tui.ts b/src/tui.ts index 8c691f1..3dfc5f5 100644 --- a/src/tui.ts +++ b/src/tui.ts @@ -6,6 +6,7 @@ import { buildFileUrl, buildFilterStats, buildRows, + getSectionPath, isCursorVisible, normalizeScrollOffset, renderGroups, @@ -14,10 +15,15 @@ import { import { buildOutput } from "./output.ts"; import { applyTeamPick, - moveRepoToSection, - undoSectionPick, + applyTeamPickInTree, + flattenTeamHierarchy, flattenTeamSections, + moveRepoToSection, + moveRepoToSectionInTree, + rebuildTeamHierarchy, rebuildTeamSections, + undoSectionPick, + undoSectionPickInTree, } from "./group.ts"; import { parseMouseEvent } from "./render/mouse.ts"; import { @@ -218,14 +224,20 @@ export async function runInteractive( // ─── Team pick mode state ─────────────────────────────────────────────────────── // Feat: team pick mode — resolve multi-team section ownership — see issue #85 + // Fix: track the full ancestor sectionPath (not just the bare label) so a + // pick on a groupByTeamHierarchy tree unambiguously targets the row the + // cursor was actually on — see issue #181. let teamPickMode = { active: false, sectionLabel: "", + sectionPath: [] as string[], candidates: [] as string[], focusedIndex: 0, }; - /** Maps combined section label → chosen team; pre-seeded with CLI --pick-team flags - * so they are included in the replay command even if no additional interactive picks are made. */ + /** Maps combined section path (joined with " > ", or the bare label for a + * flat top-level section) → chosen team; pre-seeded with CLI --pick-team + * flags so they are included in the replay command even if no additional + * interactive picks are made. */ const confirmedPicks: Record = { ...initialPickTeams }; // ─── Team re-pick mode state ────────────────────────────────────────────────── @@ -460,18 +472,42 @@ export async function runInteractive( } else if (key === KEY_ENTER_CR || key === KEY_ENTER_LF) { // Enter — confirm pick, reassign repos, exit pick mode const chosen = teamPickMode.candidates[teamPickMode.focusedIndex]; - const sections = rebuildTeamSections(groups); - const updated = applyTeamPick(sections, teamPickMode.sectionLabel, chosen); - groups = flattenTeamSections(updated); - confirmedPicks[teamPickMode.sectionLabel] = chosen; - teamPickMode = { active: false, sectionLabel: "", candidates: [], focusedIndex: 0 }; + // Fix: branch on sectionPath depth so a pick on a nested + // groupByTeamHierarchy section reassigns within the tree instead of + // (incorrectly) treating the tree as a flat groupByTeamPrefix list — + // see issue #181. A depth-1 path (top-level section) behaves exactly + // like the flat path, since a hierarchy tree's top level is the same + // shape as groupByTeamPrefix's flat sections. + if (teamPickMode.sectionPath.length > 1) { + const sections = rebuildTeamHierarchy(groups); + const updated = applyTeamPickInTree(sections, teamPickMode.sectionPath, chosen); + groups = flattenTeamHierarchy(updated); + } else { + const sections = rebuildTeamSections(groups); + const updated = applyTeamPick(sections, teamPickMode.sectionLabel, chosen); + groups = flattenTeamSections(updated); + } + confirmedPicks[teamPickMode.sectionPath.join(" > ") || teamPickMode.sectionLabel] = chosen; + teamPickMode = { + active: false, + sectionLabel: "", + sectionPath: [], + candidates: [], + focusedIndex: 0, + }; // Clamp cursor after row count may have changed const newRows = buildRows(groups, filterPath, filterTarget, filterRegex); cursor = Math.min(cursor, Math.max(0, newRows.length - 1)); scrollOffset = Math.min(scrollOffset, cursor); } else if (key === "\x1b" && !key.startsWith("\x1b[") && !key.startsWith("\x1b\x1b")) { // Esc — cancel with no change - teamPickMode = { active: false, sectionLabel: "", candidates: [], focusedIndex: 0 }; + teamPickMode = { + active: false, + sectionLabel: "", + sectionPath: [], + candidates: [], + focusedIndex: 0, + }; } redraw(); continue; @@ -500,7 +536,18 @@ export async function runInteractive( // Enter — confirm re-pick, move repo to the focused candidate team const targetTeam = repickMode.candidates[repickMode.focusedIndex]; const g = groups[repickMode.repoIndex]; - groups = moveRepoToSection(groups, g.repoFullName, targetTeam); + const pickedFrom = g.pickedFrom ?? ""; + // Fix: a hierarchical pickedFrom ("parent > combined") must move the + // repo within the tree, at the same parent depth it was picked from + // — see issue #181. + if (pickedFrom.includes(" > ")) { + const parentPath = pickedFrom.split(" > ").slice(0, -1); + const sections = rebuildTeamHierarchy(groups); + const updated = moveRepoToSectionInTree(sections, g.repoFullName, parentPath, targetTeam); + groups = flattenTeamHierarchy(updated); + } else { + groups = moveRepoToSection(groups, g.repoFullName, targetTeam); + } const newRows = buildRows(groups, filterPath, filterTarget, filterRegex); cursor = Math.min(cursor, Math.max(0, newRows.length - 1)); scrollOffset = Math.min(scrollOffset, cursor); @@ -513,7 +560,13 @@ export async function runInteractive( const combinedLabel = groups[repickMode.repoIndex]?.pickedFrom; if (combinedLabel) { delete confirmedPicks[combinedLabel]; - groups = undoSectionPick(groups, combinedLabel); + if (combinedLabel.includes(" > ")) { + const sections = rebuildTeamHierarchy(groups); + const updated = undoSectionPickInTree(sections, combinedLabel); + groups = flattenTeamHierarchy(updated); + } else { + groups = undoSectionPick(groups, combinedLabel); + } } const newRows = buildRows(groups, filterPath, filterTarget, filterRegex); cursor = Math.min(cursor, Math.max(0, newRows.length - 1)); @@ -706,7 +759,17 @@ export async function runInteractive( // Feat: team pick mode — resolve multi-team section ownership — see issue #85 if (key === "p" && row?.type === "section" && row.sectionLabel?.includes(" + ")) { const candidates = row.sectionLabel.split(" + "); - teamPickMode = { active: true, sectionLabel: row.sectionLabel, candidates, focusedIndex: 0 }; + // Fix: capture the full ancestor path so the pick targets the exact + // tree node under the cursor, not just any row sharing the same + // label — see issue #181. + const sectionPath = getSectionPath(rows, cursor); + teamPickMode = { + active: true, + sectionLabel: row.sectionLabel, + sectionPath, + candidates, + focusedIndex: 0, + }; redraw(); continue; } @@ -715,12 +778,13 @@ export async function runInteractive( // different team. Otherwise cycle the filter target: path → content → repo → path. // Feat: re-pick mode — see issue #87 if (key === "t") { - const isPickedRepo = - groupByTeamPrefix && row?.type === "repo" && !!groups[row.repoIndex]?.pickedFrom; + const isPickedRepo = row?.type === "repo" && !!groups[row.repoIndex]?.pickedFrom; if (isPickedRepo) { // Enter re-pick mode — candidates come from the original combined label + // (its last path segment for a hierarchical pick — see issue #181). const pickedFrom = groups[row!.repoIndex].pickedFrom!; - const candidates = pickedFrom.split(" + ").map((c) => c.trim()); + const combinedLabel = pickedFrom.split(" > ").at(-1)!; + const candidates = combinedLabel.split(" + ").map((c) => c.trim()); repickMode = { active: true, repoIndex: row!.repoIndex, candidates, focusedIndex: 0 }; } else { // Cycle filter target when not on a picked repo From cff447aa86b31f1d612b923ba2203c57d484c6c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 23:41:28 +0200 Subject: [PATCH 2/2] Fix: pick-team dropped the picked section's nested children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit applyTeamPickInTree only carried the picked combined section's own (often empty) groups over to the chosen sibling, silently discarding its children — any top-level combined section already subdivided by a further chain level (e.g. --group-by-team-prefix gamme-/squad-, where 2 gamme- teams overlap on repos that also matched a squad- team) lost every repo nested underneath as soon as it was picked. Fixed by recursively tagging and carrying over the whole picked subtree (own groups AND children) into the target section, merging children when the target already exists. Reported with: --group-by-team-prefix gamme-/squad-,chapter- and --pick-team 'gamme-lead-mobile + gamme-lead-mobile-security-p1'=gamme-lead-mobile --- src/group.test.ts | 53 +++++++++++++++++++++++++++++++++++++++++++++++ src/group.ts | 43 ++++++++++++++++++++++++++++++-------- 2 files changed, 87 insertions(+), 9 deletions(-) diff --git a/src/group.test.ts b/src/group.test.ts index 4fc8c6e..fef878d 100644 --- a/src/group.test.ts +++ b/src/group.test.ts @@ -597,6 +597,59 @@ describe("applyTeamPickInTree", () => { expect(result).toEqual(tree); }); + it("preserves the picked section's own children (does not drop the subtree)", () => { + // Regression: a top-level combined section ("gamme-a + gamme-a-security-p1") + // that was already subdivided by the next chain level (squad-) must keep + // its nested children when picked — only its own (now empty) `groups` + // were carried over before the fix, silently dropping every repo nested + // underneath. + const groups = [ + makeGroup("org/tools-mobile", [ + "gamme-lead-mobile", + "gamme-lead-mobile-security-p1", + "squad-core", + "squad-mobile", + ]), + makeGroup("org/wizard-mobile", ["gamme-lead-mobile", "gamme-lead-mobile-security-p1"]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const combined = tree.find((s) => s.label.includes(" + "))!; + expect(combined.label).toBe("gamme-lead-mobile + gamme-lead-mobile-security-p1"); + expect(combined.groups).toEqual([]); // fully subdivided by squad- before the pick + expect(combined.children).toHaveLength(2); // "squad-core + squad-mobile" and "other" + + const updated = applyTeamPickInTree(tree, [combined.label], "gamme-lead-mobile"); + + expect(updated.map((s) => s.label)).not.toContain(combined.label); + const picked = updated.find((s) => s.label === "gamme-lead-mobile")!; + expect(picked).toBeDefined(); + expect(picked.children).toHaveLength(2); + const squadChild = picked.children!.find((c) => c.label === "squad-core + squad-mobile")!; + expect(squadChild.groups.map((g) => g.repoFullName)).toEqual(["org/tools-mobile"]); + const otherChild = picked.children!.find((c) => c.label === "other")!; + expect(otherChild.groups.map((g) => g.repoFullName)).toEqual(["org/wizard-mobile"]); + // Every repo in the moved subtree is tagged, not just the top node's own groups. + expect(squadChild.groups[0].pickedFrom).toBe(combined.label); + expect(otherChild.groups[0].pickedFrom).toBe(combined.label); + }); + + it("merges the picked subtree's children into an existing target section's children", () => { + const groups = [ + makeGroup("org/existing", ["gamme-lead-mobile", "squad-existing"]), + makeGroup("org/tools-mobile", [ + "gamme-lead-mobile", + "gamme-lead-mobile-security-p1", + "squad-core", + ]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const combined = tree.find((s) => s.label.includes(" + "))!; + const updated = applyTeamPickInTree(tree, [combined.label], "gamme-lead-mobile"); + const picked = updated.find((s) => s.label === "gamme-lead-mobile")!; + const childLabels = picked.children!.map((c) => c.label).toSorted(); + expect(childLabels).toEqual(["squad-core", "squad-existing"]); + }); + it("returns sections unchanged for an empty combinedPath", () => { const groups = [makeGroup("org/a")]; const tree = groupByTeamHierarchy(groups, [["squad-"]]); diff --git a/src/group.ts b/src/group.ts index aba990d..205f722 100644 --- a/src/group.ts +++ b/src/group.ts @@ -587,12 +587,15 @@ function stripPickedFrom(g: RepoGroup): RepoGroup { } /** - * Tree-aware equivalent of `applyTeamPick`: reassigns every repo of the - * combined section identified by `combinedPath` (e.g. - * `["gamme-client", "squad-a + squad-b"]`) to a sibling section named - * `chosenTeam` at that same depth (appended to it if it already exists, - * otherwise created in its place). Moved repos are tagged with - * `pickedFrom = combinedPath.join(" > ")`. + * Tree-aware equivalent of `applyTeamPick`: reassigns the ENTIRE subtree of + * the combined section identified by `combinedPath` (e.g. + * `["gamme-client", "squad-a + squad-b"]`) — its own `groups` *and* any + * nested `children` (e.g. it was already subdivided by a further chain + * level) — to a sibling section named `chosenTeam` at that same depth + * (merged into it if it already exists, otherwise created in its place). + * Every repo in the moved subtree (own groups and every descendant) is + * tagged with `pickedFrom = combinedPath.join(" > ")` so + * `undoSectionPickInTree` can find all of them later. * * No-op (returns `sections` unchanged) if any segment of `combinedPath` does * not resolve to an existing node. Pure — does not mutate `sections`. @@ -611,20 +614,29 @@ export function applyTeamPickInTree( const idx = siblings.findIndex((s) => s.label === combinedLabel); if (idx === -1) return siblings; - const reposToMove = siblings[idx].groups.map((g) => ({ ...g, pickedFrom: pathKey })); + const picked = tagPickedFrom(siblings[idx], pathKey); const remaining = siblings.filter((_, i) => i !== idx); const targetIdx = remaining.findIndex((s) => s.label === chosenTeam); if (targetIdx !== -1) { return remaining.map((s, i) => - i === targetIdx ? { ...s, groups: [...s.groups, ...reposToMove] } : s, + i === targetIdx + ? { + ...s, + groups: [...s.groups, ...picked.groups], + ...(picked.children && picked.children.length > 0 + ? { children: [...(s.children ?? []), ...picked.children] } + : {}), + } + : s, ); } const newSection: TeamSection = { label: chosenTeam, - groups: reposToMove, + groups: picked.groups, level: siblings[idx].level, + ...(picked.children && picked.children.length > 0 ? { children: picked.children } : {}), }; const result = [...remaining]; result.splice(idx, 0, newSection); @@ -632,6 +644,19 @@ export function applyTeamPickInTree( }); } +/** + * Recursively tags every repo in `node` (its own `groups` and every + * descendant's, through `children`) with `pickedFrom`, preserving the + * subtree's shape. Pure — returns a new tree, does not mutate `node`. + */ +function tagPickedFrom(node: TeamSection, pathKey: string): TeamSection { + return { + ...node, + groups: node.groups.map((g) => ({ ...g, pickedFrom: pathKey })), + ...(node.children ? { children: node.children.map((c) => tagPickedFrom(c, pathKey)) } : {}), + }; +} + /** * Tree-aware equivalent of `undoSectionPick`: restores every repo anywhere in * the tree whose `pickedFrom` matches `combinedPathString` (the joined path