Skip to content

vscode: group builders tree by area, extract shared grouping primitives (PIR #818)#890

Merged
amrmelsayed merged 22 commits into
mainfrom
builder/pir-818
May 27, 2026
Merged

vscode: group builders tree by area, extract shared grouping primitives (PIR #818)#890
amrmelsayed merged 22 commits into
mainfrom
builder/pir-818

Conversation

@amrmelsayed
Copy link
Copy Markdown
Collaborator

PIR Review: Group Builders Tree by Area (mirror #811, dedup primitives)

Fixes #818

Summary

Builders tree in the VSCode sidebar now groups by area/* label (alphabetical specific areas, Uncategorized last), mirroring the backlog grouping that #886 shipped. To avoid duplicating ~67 LOC of structural code across the two views, the PR also extracts three shared primitives (groupByArea<T>, AreaGroupTreeItem base class, AreaGroupExpansionStore + persistAreaGroupExpansion helper) and migrates the already-shipped backlog view onto them in the same PR. Net effect: builders gains the new feature, backlog loses 50 LOC, and the "rule structurally identical to backlog's" acceptance criterion is enforced by import, not by reviewer attention.

Files Changed

  • packages/core/package.json (+4 / -0) — new ./area-grouping export
  • packages/core/src/area-grouping.ts (+44 / -0) — NEW: generic groupByArea<T>
  • packages/vscode/src/views/area-group-tree-item.ts (+27 / -0) — NEW: shared AreaGroupTreeItem base
  • packages/vscode/src/views/area-group-expansion.ts (+59 / -0) — NEW: AreaGroupExpansionStore + persistAreaGroupExpansion
  • packages/vscode/src/test/area-grouping.test.ts (+82 / -0) — NEW: 7 tests for groupByArea<T>
  • packages/vscode/src/views/builders.ts (+143 / -50) — two-level provider, getParent, single-Uncategorized flatten
  • packages/vscode/src/views/builder-tree-item.ts (+13 / -0) — BuilderGroupTreeItem thin subclass
  • packages/vscode/src/views/backlog.ts (+10 / -61) — migrated onto shared primitives
  • packages/vscode/src/views/backlog-tree-item.ts (+10 / -0) — BacklogGroupTreeItem thin subclass
  • packages/vscode/src/test/backlog.test.ts (+1 / -57) — dropped groupBacklogByArea suite (covered by generic)
  • packages/vscode/src/extension.ts (+5 / -14) — persistAreaGroupExpansion ×2
  • codev/plans/818-vscode-group-builders-in-the-t.md (+334 / -0) — plan artifact

Commits

Test Results

  • pnpm --filter codev-vscode check-types: ✓ pass
  • pnpm --filter codev-vscode lint: ✓ pass
  • pnpm --filter codev-vscode compile (esbuild bundle): ✓ pass
  • pnpm --filter codev-vscode test: ✓ pass (90 tests, 7 new suite('groupByArea'), 4 dropped suite('groupBacklogByArea') — coverage shifted to the generic)
  • Manual verification at the dev-approval gate: human approved after side-by-side comparison with the backlog grouping; rename wireAreaGroupExpansion → persistAreaGroupExpansion applied as part of the same review pass

Architecture Updates

No changes to codev/resources/arch.md. The change is confined to the VSCode extension's view layer — no impact on Tower internals, builder lifecycle, worktree management, shellper protocol, or any other surface arch.md documents.

Lessons Learned Updates

Added one entry to codev/resources/lessons-learned.md under Architecture:

[From 818] An acceptance criterion of "rule structurally identical to X" is a written-rule trap when the rule lives as duplicated prose in two views. Two copies drift even with diligence; the only durable enforcement is one shared function both views import. Extract when the second consumer lands — not before (no abstraction without users) and not later (drift starts on day one).

The original plan (v2) committed knowingly-duplicated code with the "byte-identical to #886" criterion enforced only by prose. The human caught it before approval; v3 added the extraction. The extraction itself was cheap (~3 small files, mechanical backlog migration); the lesson is recognizing the trap at plan time so the extraction lands together with the second consumer, not as a follow-up that competes with other priorities.

Things to Look At During PR Review

  • Mechanical backlog migration: packages/vscode/src/views/backlog.ts loses ~50 LOC. The diff is intentionally a no-op — groupBacklogByArea is replaced with groupByArea(items, i => i.area); setGroupExpanded / readExpansionState / EXPANSION_STATE_KEY are replaced with an AreaGroupExpansionStore field. Walk the backlog view in VSCode first and confirm zero visible change — that's the highest-blast-radius part of the diff.
  • Accordion reveal() in grouping mode: views/builders.ts overrides getParent to return the cached group for BuilderTreeItem children. The cache (groupParentByBuilderId) is repopulated every time rootChildren() runs in multi-group mode; cleared in the single-Uncategorized flatten case (builders are root again). Watch for the accordion behaviour with two builders in different areas — expanding one should auto-collapse the other.
  • AreaGroupTreeItem sub-classing pattern: BacklogGroupTreeItem and BuilderGroupTreeItem are 3-line subclasses that exist solely so extension.ts's per-view expand/collapse handlers can scope via instanceof. Without them, both handlers would fire on every group expand/collapse. Mentioned for clarity; the design isn't novel but the subclasses look almost empty so worth a sentence.
  • Wire-field reconciliation: the revised vscode: group builders in the tree by area (area/* label namespace, identical to backlog) #818 issue body still says OverviewBuilder.areas[] (plural) in places. The implementation consumes the single-string OverviewBuilder.area projection that vscode: group backlog tree by area/* label (PIR #811) #886 / core: parseAreaLabels helper + flow areas[] through BacklogItem and BuilderOverview #819 actually shipped. If you'd prefer rolling the wire shape forward to plural, that's a separate change against core: parseAreaLabels helper + flow areas[] through BacklogItem and BuilderOverview #819 — out of scope here.

How to Test Locally

For reviewers pulling the branch:

  • View diff: VSCode sidebar → right-click builder pir-818 → Codev: View Diff (auto-detects the repo's default branch)
  • Run dev server: VSCode sidebar → right-click builder pir-818 → Run Dev Server, or afx dev pir-818
  • What to verify:
    • Open the Backlog view → confirm grouping renders exactly as it did on main (no visible regression from the migration)
    • Open the Builders view → groups render with <area> (<count>) headers, alphabetical specifics, Uncategorized last
    • Within-group order preserves orderForDisplay() (blocked → idle-waiting → active)
    • Collapse a group, reload the window → still collapsed
    • Collapse vscode in Backlog → Builders' vscode group stays expanded (separate workspaceState keys)
    • With codev.buildersAutoCollapse on, expand one builder → others auto-collapse across groups
    • Add/remove an area/* label on an open issue via gh issue edit → next overview tick (~60s) re-groups the affected builder

Flaky Tests

None.

@amrmelsayed
Copy link
Copy Markdown
Collaborator Author

Architect Review

Approved. Builder went beyond the spec'd mirror — extracted the area-grouping primitives that #811 introduced into a new shared module (packages/core/src/area-grouping.ts + sibling tree-item/expansion-state helpers in packages/vscode/src/views/), then built #818 on top of them AND refactored #811's just-landed code to use them. Net effect: the "byte-identical rule" promise is now structurally enforced — both views literally call the same area-grouping helper, can't drift.

  • CMAP unanimous (gemini=APPROVE, codex=APPROVE, claude=APPROVE)
  • Plan-approval + dev-approval already passed (running worktree verified)
  • The refactor risk on vscode: group backlog by area (area/* label namespace, predictable across views) #811's just-landed code is real but well-tested — backlog tests shrunk because they moved to area-grouping.test.ts against the shared helper; same coverage, better-shaped
  • Honors the revised spec: no priorityAreas, pure alphabetical, Uncategorized last, single-Uncategorized flatten

Per PIR protocol, pr gate awaiting your explicit approval — handled below.


Architect review

@amrmelsayed amrmelsayed merged commit 912d015 into main May 27, 2026
6 checks passed
amrmelsayed added a commit that referenced this pull request May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vscode: group builders in the tree by area (area/* label namespace, identical to backlog)

1 participant