feat(api): include area ancestor path in work item and household item responses#1249
Conversation
|
Thank you for your submission! We require all contributors to sign our Contributor License Agreement before we can accept your contribution. I have read the CLA Document and I hereby sign the CLA Frank Steiler seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
… responses - Add `AreaAncestor` type to shared package and `ancestors` field on `AreaSummary`, surfacing the full parent-chain path for every area referenced in API responses - Introduce N+1-safe `loadAreaMap` / `resolveAreaAncestors` helpers in `areaService.ts` that batch-load all areas once and walk the parent tree in memory - Thread the area map through all callers (workItemService, householdItemService, milestoneService, dependencyService, timelineService, converters) and add test coverage for ancestor resolution and response shape Fixes #1236 Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) <noreply@anthropic.com> Co-Authored-By: Claude backend-developer (Haiku 4.5) <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku 4.5) <noreply@anthropic.com> Co-Authored-By: Claude product-architect (Sonnet 4.6) <noreply@anthropic.com>
ESM exports are live read-only bindings, so jest.spyOn on a named export from an ESM module throws TypeError at test-suite setup. Removed the spy approach in workItems.ancestors.test.ts and rewrote the test to assert actual ancestor population across all 5 work items. Also added defensive try/finally blocks around FK-disable pragma statements in both test files to ensure FK constraints are always re-enabled even on assertion failure. Refs #1236 Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku 4.5) <noreply@anthropic.com>
d3df50e to
9f2b77b
Compare
steilerDev
left a comment
There was a problem hiding this comment.
[product-owner] All 4 acceptance criteria from #1236 are satisfied. (Using --comment because GitHub blocks self-approval; this represents a PO sign-off from a requirements-coverage standpoint.)
AC coverage
- AC1 (happy path, root-first, leaf excluded) —
resolveAreaAncestorswalks fromareaMap.get(areaId)?.parentId(excludes the leaf) and reverses to root-first. Verified byareaService.ancestors.test.ts"AC1 — linear 3-level chain" andworkItems.ancestors.test.ts"AC1 — 3-level chain". Leaf-not-in-ancestors explicitly asserted. - AC2 (null-area fallback, 200) —
getAreaWithAncestorsreturnsnullwhenareaIdis null. Covered byworkItems.ancestors.test.ts"AC2" and the household list null-area case. - AC3 (5-level deep tree, no N+1) —
areaService.ancestors.test.ts"AC3 — 5-level deep tree" andhouseholdItems.ancestors.test.ts"AC3 — 5-level deep chain" verify correctness. The per-itemdb.select().from(areas).where(eq(...))query is replaced by a singleloadAreaMap(db)call at the top oflistWorkItemsandlistHouseholdItems, threaded through summary/detail converters — N+1 is eliminated by construction. Timeline, milestone, and dependency services are also threaded through the batched map. - AC4 (orphaned area, partial chain, 200) —
resolveAreaAncestorsbreaks the loop on a missing ancestor and returns what was resolved so far.workItems.ancestors.test.ts"AC4" inserts a danglingparentId, hits the API, and asserts 200 witharea.ancestors === []. Cycle protection (MAX_DEPTH=20 + visited set) is also exercised.
Observations
- Additive, non-breaking API change — existing
AreaSummaryconsumers continue to work (ancestorsdefaults to[]). - Shared type tests across
area,workItem,householdItem, andtimelineupdated consistently. - The explicit
jest.spyOnefficiency check for "loadAreaMapcalled once" was removed due to ESM read-only bindings (documented with comment + MEMORY entry). Acceptable — the batching is verified by code inspection of the clear refactor from per-item query to single pre-loaded map.
steilerDev
left a comment
There was a problem hiding this comment.
[product-architect] Approved (comment — cannot self-approve own PR).
Verified
- API contract —
AreaAncestorandancestors: AreaAncestor[]onAreaSummaryare purely additive. Wiki API-Contract.md reflects both the interface definition (lines 679-691) and example payloads across work items, household items, timeline, and milestones. - Ordering — root-first ordering is guaranteed: the loop climbs parent-ward and
ancestors.reverse()flips the chain before return. Leaf is correctly excluded (walk starts atareaMap.get(areaId)?.parentId). - Performance / no N+1 —
loadAreaMapis called exactly once per list request inlistWorkItems,listHouseholdItems,getDependencies, and timeline's existing batch-fetch path. Detail endpoints load once per call. Nested callers receive the map as a threaded argument — no per-item DB queries. - Cycle & orphan guards —
visited: Set<string>+MAX_DEPTH = 20bound the walk;if (!entry) breakreturns a partial chain on orphan. Covered bycycle protection: does not throw,orphaned parent: returns empty array, and two integration tests flippingPRAGMA foreign_keys = OFFwithtry/finallyto restore. - Code quality — Strict TS,
import type,.jsESM extensions, noanyin new code. Test coverage is thorough (service unit tests + route integration tests on both detail and list endpoints).
Informational (non-blocking)
getAreaWithAncestorsinworkItemService.tsandhouseholdItemService.tscasts the narrowAreaMapEntryviaas typeof areas.$inferSelectto satisfytoAreaSummary. A cleaner future refactor would be to adjusttoAreaSummary's input to accept the minimalAreaMapEntryshape directly. Not blocking.- Milestone and work-item detail/update paths each load the full area map per call. Fine at current scale (<5 users); worth revisiting if a batch milestone API ever lands.
- The
jest.spyOnESM limitation noted in qa-integration-tester/MEMORY.md is a useful learning — verifying "called once" via observable behavior is the right call here.
Summary
AreaAncestortype to the shared package and anancestorsfield onAreaSummary, exposing the full parent-chain path for every area in API responses (work items, household items, milestones, timeline, dependencies)loadAreaMap/resolveAreaAncestorshelpers inareaService.tsthat batch-load all areas once and walk the parent tree in memory — no additional per-item DB queriesFixes #1236
Test plan
areaService.ancestors.test.ts— ancestor resolution logic (flat, nested, multi-level, root)workItems.ancestors.test.ts+householdItems.ancestors.test.ts— route responses include correctancestorsarrayarea.test.ts,workItem.test.ts,householdItem.test.ts,timeline.test.tsupdatedCo-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com
Co-Authored-By: Claude backend-developer (Haiku 4.5) noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester (Haiku 4.5) noreply@anthropic.com
Co-Authored-By: Claude product-architect (Sonnet 4.6) noreply@anthropic.com