ADORSYS-GIS / ADORSYS-GIS/converse-frontends

[Ticket]: Spike — LCI review & feedback analytics: validate the data, the query budget, and the open decisions in ADR 0018

未關閉
#516 0 則留言 0 個 reaction 已指派 1 人 已被 @leghadjeu-christian 認領 在 GitHub 檢視
ticket
主要語言
TypeScript
星號
0
分支
0
平均合併
1 小時 49 分鐘
30 天內合併 PR
253

描述

### Type

Spike / Investigation

### Summary

We need to **investigate** whether the review/feedback data the LCI control plane already stores can back a first-class analytics page in `apps/lci` — and settle the open decisions in the draft ADR — **because** the two analytics surfaces the app has today cannot answer the question people actually ask of it ("how many PR reviews this week / last week / this month, and what was their quality?").

Concretely, today:

- `/` (Overview) calls `GET /tasks` with no parameters, which the control plane caps at **100 rows** (`services/control-plane/src/queue/tasks.rs:16`), then computes a "14-day" sparkline and a pass rate from those rows in JavaScript (`apps/lci/src/lib/domain/insights.ts`). On any busy estate that is the last day and a half, and "Total runs" reads `100` permanently. There is no range picker because the endpoint underneath has no window parameter and no aggregate.
- `/repositories/[id]` embeds two Grafana `d-solo` iframes ("Billed cost", "Tokens used") — the only two panels in the generated board set that are genuinely repository-scoped.
- The 👍/👎 signal is collected, reconciled and already fed back into the reviewer's prompt (LCI ADR-0044), but is visible only in a Grafana behind a separate OAuth2 proxy, at estate scope.

**Expected result:**

> A written answer, backed by measurements against production data, to: is the feedback signal dense enough to chart honestly; what do the aggregate queries actually cost; and which of the four open decisions goes which way. ADR 0018 then moves from Proposed to Accepted (or is amended), and the implementation tickets are cut with real numbers in them.

**This ticket is a spike. It ships documents, measurements and follow-up tickets — no production code.**

### Intent

Not to build the page. To stop guessing about it.

Two of this proposal's headline panels rest on assumptions nobody has measured, and both can invalidate the design rather than just tune it:

1. **Nobody knows what fraction of posted inline comments ever receives a reaction.** If it is ~2%, an "acceptance rate" stat card is a number computed from noise and must be demoted to a raw count. If it is 40%, it is the most valuable card on the page. The design branches on that number and we do not have it.
2. **Nobody knows how lossy the reaction→finding join is.** Both the Grafana board and the backend's own ADR-0044 feedback memory recover the finding behind a 👎 by joining `review_comments.(file, line)` to a jsonb element on `f->>'file' = rc.file AND f->>'line' = rc.line::text`. The backend's own doc comment concedes this is best-effort — "a path-normalization mismatch just misses a row". So every "👎 by category" figure in existence today is under-counted by an unknown amount, **and so is the memory fed back into the reviewer**. That second part is a correctness bug independent of any dashboard, and this spike is what sizes it.

The alternative to this spike is writing an ADR's worth of schema and API changes on top of two unmeasured assumptions. That is exactly the "laundering ignorance into polished artifacts" the doctrine warns about — the analysis is already polished; the measurements are what it is missing.

### Source of truth (links)

- **Draft ADR (Proposed):** `docs/adr/0018-lci-review-analytics-page.md` — on branch `docs/lci-review-analytics-page`, staged, **not yet committed or pushed** at the time of writing.
- **Investigation report:** `docs/design/lci-app/REVIEW_ANALYTICS_REPORT.md` — same branch. §9 (risks) and §10 (open questions) are the input to this spike.
- Prior decisions this applies under, none of them re-opened: ADR 0008 (visual direction), ADR 0010 (primitive stack), ADR 0011 (URL-first state), ADR 0013 (information architecture, chart doctrine), ADR 0014 (`apps/lci` scaffolding), ADR 0015 (the declarative dashboard engine and its panel vocabulary), ADR 0017 (i18n).
- Backend decisions in `ADORSYS-GIS/lightbridge-code-intelligence`, referenced not restated: ADR-0032 (finding priority/category), ADR-0035 (the review-feedback signal), ADR-0044 (feedback memory M1), ADR-0046 (why the Grafana boards read Postgres), ADR-0100 (DB run transcript retired — tokens and cost are Loki-only).

Baselines read: `converse-frontends@aa2dbd3`, `lightbridge-code-intelligence@18a2dca`.

### Current Behavior

| Surface | Draws | Source |
| --- | --- | --- |
| `/` Overview | 4 stat cards, a 14-day sparkline, by-repository / by-outcome bars, 8 recent runs | `GET /tasks` (unfiltered, capped at 100) → JS aggregation |
| `/repositories/[id]` Overview tab | 2 Grafana `d-solo` iframes + repository facts | Grafana, gated on `NEXT_PUBLIC_GRAFANA_URL` (unset by default) |
| `/runs/[id]` | Review output + 1 Grafana iframe (run logs, Loki) | Grafana |

The control plane exposes no aggregate endpoint at all — `/tasks`, `/tasks/{id}`, `/tasks/{id}/review`, `/tasks/{id}/feedback`, `/repositories` are all row readers, and the feedback one is **per task**. Answering "what did people think of this repository's reviews last month" currently means fetching every task and every task's feedback.

Four properties of the feedback pipeline constrain any chart drawn over it, and all four need to be confirmed against production rather than read off the code:

- `review_feedback.created_at` is the **reconcile** time, not the reaction time (the forge emits no webhook for reactions; a singleton reconciler polls, default every 300 s).
- Polling is window-bounded and age-tiered: `list_pollable_comments` only considers comments whose task is younger than `RECONCILER_WINDOW_DAYS` (**default 14**), and within that polls < 1 day old every cycle, 1–3 days ~1-in-12, older ~1-in-72. Feedback older than the window is **frozen at its last reconciled state**.
- Un-reacting is a `DELETE`, so there is no history of withdrawn reactions.
- `review_feedback` stores every reaction the forge allows (`heart`, `rocket`, `eyes`, …), so an approval rate must divide by `👍 + 👎`, never by `count(*)`.

### Expected Behavior

After this spike, the following are written down with numbers behind them, and ADR 0018 is either Accepted as drafted or amended to match what was measured:

1. Reaction **coverage** and its distribution — so panel 3 ("Acceptance rate") is either kept, demoted to a raw count, or dropped.
2. The **loss rate** of the `(file, line)` reaction→finding join — so the value of the `review_findings` projection (ADR 0018 D6) is a measured correctness win, not an argued one.
3. A **measured query budget** for the two proposed aggregate endpoints, with `EXPLAIN` output — so ADR 0018 D7's p95 targets (200 ms repo / 500 ms estate) are either confirmed, or the daily rollup it defers is pulled forward with a reason.
4. Decisions recorded for the four open questions: the feedback window vs. the range picker; `/` as the estate analytics surface vs. a new nav destination; keeping or dropping the per-repo cost embed; and whether `apps/lci` adopts i18next before this page's copy lands.
5. Follow-up implementation tickets cut in both repositories, in the sequence ADR 0018 §Sequencing sets out, each carrying the numbers this spike produced.

### Acceptance Criteria

- [ ] Given production (or the closest available prod-like) control-plane data, when reaction coverage is measured, then the report records: posted inline comments in a 30-day window, how many carry ≥1 reaction, the 👍/👎 split, and the distinct-reactor count — with the query used.
- [ ] Given the same dataset, when the `(file, line)` reaction→finding join is measured, then the report records what percentage of `-1` reactions on inline comments fail to resolve to a finding, and a sample of the mismatching paths, so the failure mode (path normalization vs. line drift vs. re-review churn) is named rather than assumed.
- [ ] Given the two aggregate statements from ADR 0018 D2, when they are run against that dataset for a 30-day single-repository window and a 90-day estate window, then `EXPLAIN (ANALYZE, BUFFERS)` output for both is attached and compared against the D7 budget.
- [ ] Given that comparison, when the budget is not met with the indexes in ADR 0018 D6b alone, then the report states whether the `review_findings` projection closes the gap or whether the deferred `review_daily` rollup has to be pulled forward — with the measurement, not an opinion.
- [ ] Given the four open questions in the report's §10, when the spike closes, then each has a recorded decision and a one-line reason in ADR 0018.
- [ ] ADR 0018 is committed with status **Accepted** (or amended and Accepted), and the report is updated in place with the measured numbers replacing its "should be measured" placeholders.
- [ ] Follow-up tickets exist and are linked here: the migration + indexes ticket and the aggregate-endpoints ticket in `lightbridge-code-intelligence`, and the panel-spec and `/repositories/[id]/insights` tickets in this repository.
- [ ] Error cases are handled safely — **not applicable, no production code ships from this ticket**; stated explicitly rather than left as an unticked box.
- [ ] Existing behavior is not broken — **not applicable for the same reason**; the spike is read-only against production data.
- [ ] Relevant tests are added or updated — **deferred to the implementation tickets**; the `EXPLAIN` assertions ADR 0018 D7 requires are specified here and written there.
- [ ] Verification evidence is provided (see below).

### Out of Scope

Do not silently implement any of these — each is a follow-up ticket:

- The `review_findings` migration and backfill, and the new indexes (`lightbridge-code-intelligence`).
- The `GET /api/v2/analytics/{reviews,feedback}` endpoints (`lightbridge-code-intelligence`).
- The panel spec, the adapters to `DashboardPanelView`, and migrating `/` off its client-side aggregation.
- The `/repositories/[id]/insights` tab and deleting the two Grafana iframes.
- Pruning any Grafana board in `lightbridge-code-intelligence/deploy/observability/`.
- Report/PDF export for the new page (the console's Typst sidecar). Out of scope entirely, not just deferred.
- Adopting i18next in `apps/lci` — this spike **decides** whether it is a prerequisite; it does not do it.

### Technical Context

**This repository (`converse-frontends`)**

- `apps/lci/src/lib/domain/insights.ts` — the client-side aggregation to be replaced.
- `apps/lci/src/lib/server/api.ts` — the control-plane client (`listTasks` at :41; blanket `cache: 'no-store'`).
- `apps/lci/src/containers/repository-overview-centre.tsx:51`, `run-detail-centre.tsx:154` — the Grafana embeds.
- `apps/lci/src/containers/repo-tabs-nav.tsx` — where the `Insights` tab lands.
- `packages/ui-web/src/sections/dashboard-panels/` — the reusable panel kit: nine types (`types.ts:25`), the renderer registry (`panel-renderers.tsx:213`), `renderPanelBody` (`panel-renderers.tsx:334`). Data-source agnostic — a renderer takes a render-ready `DashboardPanelView` and knows nothing about usage queries.
- `apps/console/src/dashboards/` — the engine deliberately **not** reused (ADR 0018 D3). Its vocabulary is the usage backend's and its execution model is a client-side `useQueries` layer; `apps/lci` has no `@tanstack/react-query`, no `yaml`, no `zod`, and renders from Server Components.

**`lightbridge-code-intelligence` (`@18a2dca`)**

- Migrations `0009_reviews.sql` (`reviews`, `findings jsonb`), `0015_review_feedback.sql` (`review_comments`, `review_feedback`), `0038`/`0039` (the task/repository list indexes).
- `services/control-plane/src/db/feedback.rs` — `list_pollable_comments:35`, `reconcile_comment_feedback:65`, `rejected_findings_for_repo:123` (the lossy join), `get_feedback:149`.
- `services/control-plane/src/queue/reconciler.rs:610` — the poll cycle; `main.rs:752` — `RECONCILER_WINDOW_DAYS` (default 14) and `RECONCILER_INTERVAL_SECS` (default 300).
- `services/control-plane/src/review.rs:19` — the `Finding` shape and the ADR-0032 priority/category fallbacks any aggregate must reproduce.
- `tools/dashboard-gen/lci_dashboards/{feedback,review_quality}.py` — the existing SQL, and the closest thing to a specification of the queries this spike is costing.

**Known constraints**

- **Tokens and cost are not in the control-plane database.** ADR-0100 retired the DB run transcript; per-run token and model figures live only in the AI-Gateway's Loki billing stream. A control-plane-fed page can report volume, outcome, findings and feedback — not money, and it must not appear to.
- **Read-only access is required for the measurements.** LCI ADR-0046 already provisions a least-privilege `grafana_ro` CNPG managed role (login + `SELECT`) on the control-plane database — that, or an equivalent, is the intended access path. **This spike must not write to production.**

### Risks

| Risk | Mitigation |
| --- | --- |
| Reaction coverage turns out too low to chart, invalidating a headline panel | That is the spike succeeding, not failing. Panel 3 is demoted to a raw count and the ADR is amended before any code is written — which is the whole point of measuring first. |
| No safe read access to production data | Fall back to staging and **say so in the report** — a staging-derived coverage number is weak evidence for a product decision and must be labelled as such, never presented as a production measurement. |
| The 14-day poll window makes any range beyond it partly stale | One of the four decisions. Either the page caps feedback panels at the window and captions them, or `RECONCILER_WINDOW_DAYS` is raised — which costs forge API calls (tiered, but not free). Owner's call; the spike puts the cost of raising it in front of them. |
| Scope creep into implementation | The Out of Scope list above is enforced: this ticket closes with documents, measurements and linked follow-up tickets, and no production code. |
| The spike's own numbers go stale before the implementation lands | Every measurement is recorded with its date, its dataset and the query that produced it, so it can be re-run rather than re-argued. |
| Two declarative dashboard mechanisms end up in one monorepo (ADR 0018 D3) | Accepted, with a stated trigger to collapse them: a third LCI dashboard page, or an operator asking to change LCI panels without a rebuild. |

### Test Plan

No code, so the "test plan" is the measurement plan. Every query is read-only.

1. **Reaction coverage**, over a 30-day window: count `review_comments` where `kind = 'inline'`; count those with ≥1 row in `review_feedback`; break the reactions down by `reaction` value; count distinct `reactor`. Repeat per repository for the top ~5 by run volume to see whether coverage is estate-wide or concentrated in one team.
2. **Join loss**: count `review_feedback` rows with `reaction = '-1'` and `comment_kind = 'inline'`; count how many resolve to a finding through the `(file, line)` join used by `rejected_findings_for_repo`; report the delta as a percentage and sample ~20 non-matching `(file, line)` pairs against the corresponding `reviews.findings` to name the failure mode.
3. **Query budget**: write the two aggregate statements from ADR 0018 D2, run each for a 30-day single-repository window and a 90-day estate window, capture `EXPLAIN (ANALYZE, BUFFERS)` for both, and compare against D7 (p95 < 200 ms repo, < 500 ms estate). Then re-run with the D6b indexes applied **on a scratch copy, never production**, and record the difference.
4. **Volume sanity**: row counts for `tasks`, `reviews`, `review_comments`, `review_feedback`, plus `avg(jsonb_array_length(findings))`, so the numbers above are read against a known dataset size and can be re-derived later.

Expected result: four sections of numbers appended to `REVIEW_ANALYTICS_REPORT.md`, each with its query, its dataset and its date.

### Verification evidence

**At the time of filing, this is a proposal with an evidence gap that the ticket itself names — stated plainly rather than dressed up.**

Completed and verifiable now:

- Both documents were written against fresh reads of two repositories at pinned commits (`converse-frontends@aa2dbd3`, `lightbridge-code-intelligence@18a2dca`), not from memory. Every file:line citation in them was resolved against those trees.
- Every relative link in both documents was checked to resolve to an existing file.
- Both pass `npx prettier --check`.
- The two documents are **staged, uncommitted and unpushed** on local branch `docs/lci-review-analytics-page` (cut from `origin/main`) as of filing. They must be pushed before the links in "Source of truth" resolve for anyone else — that is the first task of this ticket, not an omission being glossed over.

**Not yet done, and the reason this ticket exists:**

- No query has been run against production or staging data. Reaction coverage, join loss and the query budget are all **unmeasured**; the report's §9 flags each one as such rather than asserting a number. Nothing in either document should be read as "measured" until the Test Plan above has been executed and its output attached here.

### Human accountable owner

@leghadjeu-christian

### AI Usage Declaration

Drafting the ticket, Understanding code, Writing documentation

### Human verification completed

- [x] I understood the intent
- [x] I checked the source of truth
- [ ] I reviewed all AI-generated text/code
- [ ] I verified the implementation manually
- [ ] I verified the tests
- [ ] I checked for hallucinated assumptions
- [x] I documented remaining risks
- [x] I am the accountable owner and accept responsibility for this ticket.

貢獻指南

開啟貢獻指南

評估

這個 Issue 還沒有評估資料。

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。