Support --pick-team across all hierarchy levels - #189
Open
shouze wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds path-aware --pick-team support for hierarchical team sections, including tree operations, TUI integration, rendering, and tests.
Changes:
- Adds hierarchy reconstruction and path-based pick, move, undo, and lookup helpers.
- Updates TUI and rendering to track full section paths.
- Adds regression and parity tests for flat and nested hierarchies.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Reviewed scope |
|---|---|
src/tui.ts |
Hierarchical pick and re-pick handling |
src/render/rows.ts |
Section ancestor-path reconstruction |
src/render.ts |
Full-path pick-mode matching |
src/render.test.ts |
Pick-mode rendering tests |
src/group.ts |
Tree reconstruction and path-aware operations |
src/group.test.ts |
Hierarchical operation tests |
Suppressed comments (8)
src/group.ts:615
- A
TeamSectionmay have both directgroupsand nestedchildren, and a combined node can be a parent when a repo has multiple matches at this depth plus a deeper-chain match. Removing that node fromremainingdrops its entire child subtree (even whenreposToMoveis empty), so picking that combined header can silently lose descendant repos. Preserve/reparent the child tree while moving/tagging its repos, or explicitly reject picks on non-leaf combined nodes.
const reposToMove = siblings[idx].groups.map((g) => ({ ...g, pickedFrom: pathKey }));
const remaining = siblings.filter((_, i) => i !== idx);
src/group.ts:548
- When a nested target section does not already exist, this helper creates it without a
level.flattenTeamHierarchythen defaults that node to level 0, so a nested re-pick or undo renders the new section at the root and reconstructs an incorrectsectionPath. Assign the new node the depth implied byparentPath(while preserving the flat top-level shape).
const newSection: TeamSection = { label, groups: repos };
src/group.ts:577
- When a node keeps direct groups but all of its children are removed,
childrenis set to an empty array because[]is truthy. This violates theTeamSectioninvariant thatchildrenis present only when non-empty and can make consumers treat a leaf as a parent; omit the property for an empty result.
const children = node.children
? removeMatchingRepos(node.children, predicate, collected)
: undefined;
return { ...node, groups: kept, ...(children ? { children } : {}) };
src/group.ts:451
- This new path-aware behavior stores
"ancestor > combined"inRepoGroup.pickedFrom, but the shared field documentation still describes the value as only the bare combined label. Update that public contract so future consumers do not parse or display hierarchical values incorrectly.
// 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).
src/group.ts:478
rebuildTeamHierarchyis used for every tree pick/re-pick, and spreading the entire children array for each sibling makes reconstruction quadratic in the number of siblings. Since these nodes are newly allocated inside this function, append the child in place (or accumulate children separately) so rebuilding a wide hierarchy remains linear.
parent.children = [...(parent.children ?? []), node];
src/render/rows.ts:158
- While pick mode is active,
renderGroupscallsgetSectionPathfor every section row on each redraw. Scanning backward through all preceding rows for each level-1 sibling makes a wide hierarchy O(rows²) per redraw, so cycling candidates can become increasingly slow. Cache or carry the full path while building rows rather than rescanning the prefix of the list for every header.
for (let i = rowIndex - 1; i >= 0 && neededLevel >= 0; i--) {
const r = rows[i];
if (
r.type === "section" &&
r.sectionLabel !== undefined &&
src/tui.ts:540
- The same representation check misses top-level hierarchy picks during re-pick: those picks intentionally store the bare combined label in
pickedFrom, but the flattened groups still carrysectionPath. After a top-level hierarchy pick, this branch calls the flat mover, whoserebuildTeamSectionscannot see the hierarchy sections, so re-pick becomes a no-op. Use the hierarchy marker ingroupsin addition to the separator check (and apply the same correction to the undo branch below).
if (pickedFrom.includes(" > ")) {
src/tui.ts:560
- Top-level hierarchy picks also have a bare
pickedFrom, so this condition selectsundoSectionPickeven though the currentgroupsarray is in thesectionPathrepresentation. The flat rebuild then fails to find the picked repos and theconfirmedPicksentry is removed without restoring the section. Branch on the presence of hierarchy metadata here as well as on the path separator.
if (combinedLabel.includes(" > ")) {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+511
to
+529
| 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)); |
Comment on lines
+528
to
+529
| const updatedChildren = updateSiblingsAtPath(sections[idx].children ?? [], rest, updater); | ||
| return sections.map((s, i) => (i === idx ? { ...s, children: updatedChildren } : s)); |
Comment on lines
+478
to
+485
| 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); |
|
Coverage after merging feat/team-hierarchy-pick-team into feat/team-hierarchy-tui will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- 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.
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
shouze
force-pushed
the
feat/team-hierarchy-pick-team
branch
from
August 24, 2026 00:14
2605932 to
cff447a
Compare
|
Coverage after merging feat/team-hierarchy-pick-team into feat/team-hierarchy-tui will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
1 similar comment
|
Coverage after merging feat/team-hierarchy-pick-team into feat/team-hierarchy-tui will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
--pick-teamsupport at every depth of agroupByTeamHierarchytree,building on #177-#180. A bare section label is no longer unique across a
hierarchy tree (e.g.
"other"can appear under multiple parents), so thisintroduces path-addressed equivalents of the existing flat pick/undo/move
operations.
src/group.ts:rebuildTeamHierarchy(inverse offlattenTeamHierarchy),plus tree-aware
applyTeamPickInTree,undoSectionPickInTree,moveRepoToSectionInTree,undoPickedRepoInTree, andfindCombinedSectionPaths. These address a section by its fullroot-to-node path; the path joined with
" > "is used wherever a singlestring key is needed (
pickedFrom,confirmedPicks). For a top-level(depth-1) path this is byte-for-byte equivalent to the existing flat
functions (covered by parity tests).
src/render/rows.ts:getSectionPath(rows, rowIndex)reconstructs asection row's full ancestor path by scanning backward through the flat
rowslist for the most recent row at each decreasingsectionLevel.src/tui.ts: pick mode (p) now captures the row's fullsectionPath;the confirm/undo/re-pick handlers branch to the tree-aware functions when
the path has 2+ segments, and keep using the existing flat functions
unchanged for depth-1 sections — no behavior change for any current (flat)
CLI invocation.
src/render.ts: the pick-mode section match now compares the full pathinstead of the bare label, so the same label at a different tree branch
can't be targeted by mistake.
Bug found & fixed along the way
While wiring this up I hit a runtime crash entering pick mode:
getSectionPathwas referenced inrender.tsbut only re-exported(
export { getSectionPath } from "./render/rows.ts"), never locallyimport-ed — a re-export doesn't create a local binding, so calling it inthe same file throws
ReferenceErrorat runtime. Bun's bundler doesn'ttype-check, so this wasn't caught by
bun run build.ts, and no existingtest exercised
renderGroupswithteamPickModeactive at all. Fixed theimport and added regression tests that actually set
teamPickMode(flat andhierarchical, including a same-label-different-path case).
Also fixed an edge case in
updateSiblingsAtPath(the shared tree-navigationhelper): if moving/undoing the last repo under an ancestor left that
ancestor with no groups and no children, it would get pruned entirely,
making the branch unreachable for a subsequent create — a missing ancestor
is now recreated (only when the operation actually produces something under
it, so a genuinely-unmatched path is still a true no-op).
CLI parsing/registration of hierarchy-aware
--pick-teampaths isintentionally out of scope here — tracked by #182.
Closes #181
How to test
bun test src/group.test.ts src/render.test.ts24 new
group.test.tstests cover:rebuildTeamHierarchyround-trips,applyTeamPickInTree/undoSectionPickInTree/moveRepoToSectionInTree/undoPickedRepoInTree(flat parity + nested cases + no-ops + themissing-ancestor edge case), and
findCombinedSectionPaths. Newrender.test.tstests cover the pick-mode section bar (flat, hierarchicalmatch, hierarchical non-match) that would have caught the
getSectionPathcrash.
Validation
bun test(944 pass)bun run lintbun run format:checkbun run knipbun run build.tsbunx tsc --noEmitsanity check on all touched files (zero new errors;pre-existing unrelated
tscfindings inrender.ts/tui.tsconfirmedpresent on
mainalready, not part of this PR)