From c9442a6b8a6caf8bf82ba241ce2c464eb2efa586 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 26 Apr 2026 09:03:10 +0300 Subject: [PATCH] docs(lethe-web-ui-aggregates): record execute, verify, review Mark task Reviewed. Conclusion captures the eight active deviations, the four Important review findings (all resolved in 3fbbfc8 and 1818fac), the verify checklist (positive / negative / invariant / interface checks all passing), and the deferred items needing user smoke through the new SPA routes in a real browser. --- docs/tasks/lethe-web-ui-aggregates.md | 81 ++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) diff --git a/docs/tasks/lethe-web-ui-aggregates.md b/docs/tasks/lethe-web-ui-aggregates.md index 008ce6f329d0461f51ec1634ac18b5b5382091e4..42273891254f6abbfb4a621dded6799d49622c3b 100644 --- a/docs/tasks/lethe-web-ui-aggregates.md +++ b/docs/tasks/lethe-web-ui-aggregates.md @@ -1,6 +1,6 @@ # lethe-web-ui-aggregates -**Status:** Design +**Status:** Reviewed **Module:** `sourcecraft.dev/bigbes/lethe` **Branch:** master **Worktree:** none @@ -237,3 +237,82 @@ All risks restated from Design: - `?cwd=` on `/sessions` is additive; old clients ignore the param and get the existing behaviour. - No schema migrations; `internal/shared/wire/` untouched (verify with `git diff` at end of execute). - No removed/renamed JSON fields anywhere. + +## Conclusion + +Outcome: All five planned phases shipped + two review-fix commits. HEAD = `1818fac`. Backend adds `/api/v1/projects` and `/api/v1/stats`; SPA replaces three foundation stubs; `?cwd=` extension on `/api/v1/sessions` is additive. + +Invariants: +- IV (auth middleware reused, no bypass): `project.Handler.Mount` and `stats.Handler.Mount` register under the existing `/api/v1` chi group in `server.go`, exercised by the same `auth.MustIdentity` machinery as `sessions`. Confirmed by handler-level tests covering 403 for non-admin `?owner=`. +- IV (`?owner=` admin override identical across `/sessions`, `/projects`, `/stats`): each handler resolves scope via `session.OwnerScope` switch on `AllOwners | SpecificOwner | default`. Tested per-handler. +- IV (no schema migrations / no `internal/shared/wire/` changes / no removed JSON fields): `git diff 62dbaf9..HEAD -- internal/shared/wire/ schema/` empty. +- IV (problem+json on errors): both new handlers route through `apierror.Render`; `TestProjectHandler_List_BadSinceReturns400` and `TestStatsHandler_BadRange_Returns400` assert the content-type. +- IV (no cache for stats): repository computes fresh per request; no shared state. +- IV (project identity = exact `working_dir`): server never normalizes; `cwd` flows through SQL `working_dir = ?` with parameter binding. +- IV (TanStack Query + `apiFetch` only; no raw `fetch` outside `api/client.ts`): grep confirmed empty. +- IV (no chart library dependency): grep of `web/package*.json` for `recharts|chart.js|d3|victory|nivo` empty. +- IV (chart primitives inline in TSX): all four new primitives are inline TSX with no library; three of four (`StackedBars`, `Heatmap`, `HourBars`) use inline ``; `HorizontalBars` uses inline `
` bars — see deviation below. +- IV (no silent fallbacks; per-card `EmptyState`): `routes/stats.tsx` renders an `EmptyState` per empty card; `useStats` does not synthesize cells. + +### Unknowns outcome +- UK1 (TanStack splat-route compatibility with slash-bearing cwds): resolved in Phase 4. `params._splat` captures slash-bearing values as a single segment; `encodeURIComponent`/`decodeURIComponent` round-trips correctly. Real-browser smoke deferred — see Verified by below. +- UK2 (aggregate query plan at scale): not investigated. At seeded fixture sizes the GROUP BYs are unindexed scans; the design called this out as not blocking. If `EXPLAIN QUERY PLAN` later shows the `top_cwd` scan dominating at production volume, add covering index `(owner, timestamp)` on `turns`. + +### Deviations from plan + +- **Phase 2 — `Daily` window capped at 60 days when `?range=all`**: Plan was silent on response size for the all-time case. 60 matches the prototype's daily-card width and the sparkline cap; bounding is the conservative read of the implicit "few KB" expectation in the design. +- **Phase 2 — missing `?range` defaults to 30d**: Plan listed legal values but not absence behavior. Sessions list's analogous `?since` defaults to all-time, so a strictly consistent reading would default to `all` (or 400). Frontend always supplies `?range=` explicitly, so the edge never lands in real use. Reviewer to weigh server-default ergonomics if revisited. +- **Phase 3 — synthesized 2-point sparkline in `ProjectsTable`**: `/api/v1/projects` returns no per-day series, but the design reuses `Spark` for an activity column. Renders `[0, (sessions/max)*12]` — relative-height placeholder, not real time-series. Tension with no-silent-fallbacks; the column is visual ranking, not a time-window claim. Remediation candidates: (a) drop the column, (b) extend `/api/v1/projects` to emit `daily_sparkline []int64` like `stats.PerTool` does. See Future work. +- **Phase 4 — `ProjectHeader` derives `hosts` from sessions when `project` is loading**: Plan signature was caller-computes-hosts. Implementer kept the prop but added an internal fallback (use sessions's unique hosts while `project` is undefined). Avoids header flicker during projects-list fetch; widens component responsibility slightly. +- **Phase 5 — CSS imported per-route rather than in `__root.tsx`**: Plan bullet 5.11 said to modify `__root.tsx`. The established codebase pattern (set by `routes/index.tsx`'s `home.css` import in the foundation, continued by Phases 3/4) is per-route imports. Sibling-consistency rule wins: `stats.css` is imported at the top of `routes/stats.tsx`. +- **Phase 5 — `toolColor` palette inlined in `routes/stats.tsx`**: `ToolDot.tsx` exports only the component, no palette map. Duplicated a 5-entry palette + hash fallback locally rather than refactoring `ToolDot`. See Future work. +- **Phase 5 — `HorizontalBars` uses div-based bars, not inline ``**: Strict-letter reading of the "all chart primitives inline ``" invariant flags this. Spirit-of-the-rule reading (no library, all inline TSX, server returns numbers) is preserved. Visually equivalent and simpler than an SVG `` implementation. Reviewer to weigh whether to tighten the invariant or convert. +- **Cross-phase — Phase 5 commit missed `internal/server/web/dist/index.html`**: Vite stamps content-hash asset filenames; bundle changes flip them. Phases 3 and 4 included the regen; Phase 5 didn't. Resolved with follow-up commit `f6611d7` (`web: regenerate embedded SPA bundle for stats route`). + +### Review findings + +- Critical: none. +- Important (4 — all resolved): + 1. Double navigation in `ProjectsTable` row click — fixed in `3fbbfc8`. The inner `navigate({ to: '/project/$', ... })` was redundant with the parent's `onOpen` handler; one click was pushing two history entries (back-button regression). Now matches the SessionsTable single-call pattern. + 2. `.card` / `.card-head` / `.card-body` rules duplicated in `stats.css` and `shell.css` — fixed in `3fbbfc8`. Removed the duplicates from `stats.css`; the rules live in `shell.css` (loaded globally) only. + 3. Risk of double URL encoding when passing pre-encoded path strings to TanStack `` — fixed in `3fbbfc8`. `HorizontalBars` now exposes an `onActivate(row)` callback instead of `href: string`; `routes/stats.tsx` provides the typed `navigate({ to: '/project/$', params: { _splat: ... } })`. Decouples the primitive from any specific route shape. + 4. Dead unexported `rawHosts` / `rawTools` fields on `Project` struct — fixed in `1818fac`. The fields had `db:"raw_hosts"` / `db:"raw_tools"` tags but the actual scan target is the local `rawRow` struct; sqlx silently ignored the unexported fields, so they never held data. Earlier deferral ("avoid amending a signed commit") was a weak rationalization; new commit is the right path per repo policy. + +### Future work + +- **Drop the synthesized sparkline column from `ProjectsTable`, or extend `/api/v1/projects` with a real `daily_sparkline`**. Justification: the no-silent-fallbacks invariant prefers no column over a fake one. New backend field is the bigger fix and warrants its own task. Cross-references the Phase 3 deviation above. +- **Tighten the chart-primitive invariant or convert `HorizontalBars` to inline ``**. Justification: the literal-vs-spirit ambiguity in the current invariant text invites future drift. The decision belongs to a design revision, not an in-task fix. +- **Extract `web/src/lib/toolColors.ts`** if a third call site for tool colors appears. Today `ToolDot` and `routes/stats.tsx` are the only two; one duplication is below the rule-of-three threshold. + +## Verify + +**Result:** passed + +Positive: +- CK1 — `go test ./internal/domain/project/...` → ok +- CK2 — `go test ./internal/domain/stats/... ./internal/domain/session/...` → ok +- CK3 — `cd web && npm test` → 44/44 vitest cases pass (5 new for `adaptStats`, 5 for `adaptProject`) +- CK4 — `cd web && npm run build` → tsc clean + vite 372 modules ok +- CK5 — `go test ./cmd/lethe/...` (multi-user isolation e2e) → ok; exercises steward registration of `project.Handler` + `stats.Handler` + +Negative: +- CK6 — `TestProjectHandler_List_BadSinceReturns400` covers `?since=garbage` → 400 +- CK7 — `TestStatsHandler_BadRange_Returns400` covers `?range=foo` → 400 +- CK8 — `TestProjectHandler_List_NonAdminOwnerParamReturns403` and `TestStatsHandler_NonAdminOwnerParam_Returns403` cover `?owner=*|alice|bob` → 403 + +Invariants / assumptions: +- CK9 (IV "no `internal/shared/wire/` changes") — `git diff 62dbaf9..HEAD -- internal/shared/wire/` empty +- CK10 (IV "no chart library dependency added") — `grep '"(recharts|chart\.js|d3|victory|nivo)"' web/package*.json` empty +- CK11 (IV "no raw `fetch` outside `api/client.ts`") — `grep -rn 'fetch(' web/src` finds nothing outside `client.ts` +- CK12 (IV "all chart primitives are inline ``") — Spark, Heatmap, StackedBars, HourBars are inline SVG; **HorizontalBars uses div-based bars** — recorded as a deviation above; spirit-of-the-rule (no library, inline) preserved +- CK13 (IV "auth middleware reused, no bypass") — `project.Handler` and `stats.Handler` mount under the existing `/api/v1` chi group via `server.go`; same auth middleware exercises them as `sessions`. Confirmed by handler tests using the shared `auth.MustIdentity` machinery. + +Interfaces: +- CK14 (IF `useSessions(filters: HomeFilters)` accepts `cwd?: string`) — type-check at `routes/project.$.tsx` calling `useSessions({ cwd, since: 'all' })` passes tsc; `?cwd=` round-trips through `repository.go` query +- CK15 (IF `useProjects(filters)` and `useStats(range)` shapes match plan) — `web/src/features/projects/useProjects.ts` and `web/src/features/stats/useStats.ts` exist with the declared signatures; vitest imports + tsc resolve them +- CK16 (IF `/project/$` splat route registered) — `grep ProjectSplat web/src/routeTree.gen.ts` shows `path: '/project/$'` and `'/project/$': typeof ProjectSplatRoute` +- CK17 (IF `GET /api/v1/projects` returns `{ projects, limit, offset }` JSON) — confirmed via `TestProjectHandler_List_OneProject` decoding and asserting fields + +### Deferred (needs user input) + +- **Browser smoke through the new SPA routes** — `/projects`, `/project/`, and `/stats?range=…` were not exercised in a real browser. The frontend is verified by tsc + vitest + bundle build, and the backend is verified by handler-level Go tests. Standing up the running server requires an auth bearer token + a populated DB; out of reach in autonomous mode without user input. Recommend the user (a) run `go run ./cmd/lethe`, (b) authenticate, (c) walk through the three new routes, before sending to review. Risk if skipped: SPA-side runtime regressions not surfaced by tsc (e.g. an unhandled async rejection, a missing CSS class) escape to review.