docs(server/services): document better-sqlite3 12.10.0 percentile adoption opportunity (#1571)#1644
Conversation
…adoption opportunity (#1571) Co-Authored-By: Claude backend-developer (Haiku 4.5) <noreply@anthropic.com>
…ts (#1571) - budgetBreakdownService: P90 example now JOINs through work_item_budgets - budgetOverviewService: P75 example uses real invoices.status enum values Co-Authored-By: Claude backend-developer (Haiku 4.5) <noreply@anthropic.com>
|
⏸️ CI gating deferred — blocked on |
steilerDev
left a comment
There was a problem hiding this comment.
[product-architect]
Verdict: approve
Documentation-only change; schema references are accurate and the architectural framing is sound. No blocking concerns.
Architectural assessment
1. Forward-compatibility — placement is correct. Cornerstone's established pattern is per-domain service files that own their aggregation SQL inline (getBudgetOverview already issues multi-CTE SUM/COALESCE queries directly against work_item_budgets, invoice_budget_lines, etc.). There is no separate reporting service, read-model, or analytics tier — and at <5 users with a single SQLite container, introducing one would be premature. When percentile aggregates are eventually implemented, budgetOverviewService.ts and budgetBreakdownService.ts are the right homes, exactly as the comments imply. The "surface the result as a new field on the BudgetOverview type" hint is consistent with how the existing aggregates flow out.
2. Schema accuracy — verified. Spot-checked every column reference against server/src/db/schema.ts@7a5b3b9:
work_item_budgets.planned_amount✓ (real, default 0)work_item_budgets.work_item_id✓ (nullable — theWHERE … IS NOT NULLguard is correct)work_item_budgets.budget_category_id✓work_items.duration_days✓ (integer, nullable — guard correct)work_items.area_id✓invoice_budget_lines.work_item_budget_id✓ (nullable — guard correct)invoice_budget_lines.itemized_amount✓invoices.statusenum is'pending' | 'paid' | 'claimed' | 'quotation'—IN ('pending', 'paid', 'claimed')correctly excludes quotations. The earlier dev-team-lead fix (removing phantombudget_category_idoninvoice_budget_linesand bogus'cancelled'status) is fully reflected.
3. Architectural risk — none. ADR-003 commits to better-sqlite3 + Drizzle and there are no signals of a driver swap or off-server analytics migration. SQLite ≥ 3.53 percentile aggregates are pure builtins (no extension loading, no WAL/pragma interaction), so adoption is reversible and low-blast-radius.
4. Documentation placement — defensible as-is, with one optional follow-up. Inline service comments are reasonable because the patterns are tightly coupled to the specific queries these services already run, and the comments will be discovered exactly when someone next touches budget aggregation. The trade-off is that this isn't the kind of cross-cutting decision that warrants an ADR — there's nothing being decided, just an opportunity being parked. If the comments are still unactioned in ~3 months, a one-paragraph note on the wiki Architecture.md "Database" section (or a brief server/src/db/README.md) would be a better long-term home so it surfaces during schema work too, not only when editing budget services. Non-blocking.
Non-blocking observations
- NULL-handling guidance is good, but worth noting that
COALESCE(percentile_cont(...), 0)may be wrong for some future callers — a median of "no data" semantically is not zero (e.g., median duration of zero work items shouldn't display as "0 days"). Callers should make the empty-set decision deliberately. Not a documentation defect; just flagging for whoever implements. - The breakdown example
GROUP BY wib.budget_category_idwill produce aNULLbucket for orphan lines (budget_category_id is nullable onwork_item_budgets). Per ADR-030 (Orphan Budget Lines), that's likely the correct behavior, but the implementer should explicitly decide whether to filter or surface orphans in percentile views. - Minor: the
budgetOverviewServicecomment block's closing implementation hint uses adb.get<{...}>(sql\…`)` shape that matches existing usage in the file — good consistency.
Approving. Schema-accurate, architecturally aligned, correctly placed for the project's current scale.
Co-Authored-By: Claude product-architect (Opus 4.6) noreply@anthropic.com
|
✅ CI cleared independently — infra issue self-resolved.
E2E shard failures (1/16, 13/16) on this run are unrelated pre-existing flakes in UI test specs ( |
|
🎉 This PR is included in version 2.7.0-beta.40 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Documents the
percentile_cont/percentile_discSQLite window functions enabled by the better-sqlite3 12.10.0 bump (#1527), so future engineers know these functions are available without a re-upgrade.Closes #1571
Outcome
Pure documentation — no behaviour change. An audit of the JS/TS codebase found zero existing percentile/median/quartile callsites. Comment blocks were added to the two most-likely future homes for such logic, per acceptance criterion 3 of the issue.
Files Changed
server/src/services/budgetOverviewService.ts— added documentation comment block explainingpercentile_cont/percentile_discusage, concrete SQL examples, and NULL-handling guidanceserver/src/services/budgetBreakdownService.ts— same documentation comment blockRelated
Contributing Agents
b8b88ee(initial docs),7a5b3b9(review fixup: SQL example corrections)Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com