From dd2e2ea9b78b96a0d88f24012a4e429f4c1d4dfb Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 26 Apr 2026 07:44:10 +0300 Subject: [PATCH] docs(lethe-web-ui-foundation): record review conclusion, mark Reviewed Two Important findings from the independent review pass were resolved: keyboard g-leader / palette-input interference (4ef7a02), and Dockerfile web-builder path fragility plus uncovered Go-1.26 requirement (0cf348a). Future work captured: composite-id URL, aggregates absent on Get, Turn meta-line timestamp, stray flatted/ Go package. CI config remains deferred pending user choice between .github/ and .sourcecraft/. --- docs/tasks/lethe-web-ui-foundation.md | 40 +++++++++++++++++++-------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/docs/tasks/lethe-web-ui-foundation.md b/docs/tasks/lethe-web-ui-foundation.md index 4b68458f1e09813b761bfabdbd18500a809f16c6..b250ee6ecc57dfe2ee0c818462032e25eadf71bd 100644 --- a/docs/tasks/lethe-web-ui-foundation.md +++ b/docs/tasks/lethe-web-ui-foundation.md @@ -1,6 +1,6 @@ # lethe-web-ui-foundation -**Status:** Verified (review pending) +**Status:** Reviewed **Module:** `sourcecraft.dev/bigbes/lethe` **Branch:** master **Worktree:** none @@ -337,6 +337,34 @@ Notes: ## Conclusion +Outcome: foundation slice landed across 9 commits (`1af5bcb..136ae1f`) plus 2 review-fix commits (`4ef7a02`, `0cf348a`); end-to-end browser smoke passed. + +### Invariants + +- `internal/shared/wire/` untouched — `git log 85a2dd3..HEAD -- internal/shared/wire/` empty. +- `internal/server/auth/` untouched — `git log 85a2dd3..HEAD -- internal/server/auth/` empty. +- `GET /api/v1/sessions` is a strict superset (existing 9 fields + 5 new aggregates) — verified via curl and `repository_test.go`. +- All API calls go through TanStack Query; only one raw `fetch(` in `web/src`, in `api/client.ts`. +- `web/dist/` has zero committed files; `internal/server/web/dist/` has only `.gitkeep` + placeholder `index.html`. +- `tweaks-panel.jsx` not ported; runtime accent toggle not surfaced. +- All HTTP error paths render `application/problem+json` (including 401/403/404/405/415). + +### Review findings + +- Important: keyboard `g`-leader and `j`/`k` did not guard against palette-open or editable target — typing `gh` into the palette input fired `go('home')` mid-query. Fixed in `4ef7a02` with an early-return after Esc handling, plus two regression tests. +- Important: Dockerfile `web-builder` worked only by relative-path coincidence (vite output landing at the container root because `WORKDIR /web` shifted the relative `outDir` up one level). Fixed in `0cf348a` by mirroring the host repo layout (`WORKDIR /src`, `COPY web/ web/`). While validating the build, also discovered `golang:1.25-alpine` no longer compiles the codebase (the `auxilia/culpa` dep needs `errors.AsType`, which is in 1.26+); bumped builder image to `golang:1.26-alpine` and `go.mod` directive to `1.26.0` in the same commit. `docker build` now produces a runnable binary end-to-end. + +### Future work + +- **Composite-id-in-URL** (Phase 5): `SessionsTable` Link passes `params.id = session.id` (full composite). Session view loads correctly because `useSession` adapter splits it back, but the URL is `/session/$tool/$host/$tool%2F$host%2F$id`. One-line fix in `SessionsTable` (pass bare `session_id`). Justification: cosmetic, not functional; deferred so this commit set stays scoped to fixing reviewer findings. +- **Aggregates absent on Get endpoint**: `GET /api/v1/sessions/{tool}/{host}/{id}` returns the new `summary`/`turn_count`/`tokens_*_total` fields as zero values because the Get path uses `sessionSelectColumns` (Plan 1.2 said Get unchanged). The SPA reads `turns[]` directly so it's invisible in this UI, but a future API consumer would see misleading zeros. Justification: explicitly scoped out by Plan 1.2 (Get path unchanged); fixing requires either extending the Get SQL or `omitempty` on the four numeric tags; either way, scope creep here. +- **Turn meta line lacks timestamp** (Phase 6): the `Turn` TS interface in Plan 6.1 didn't include `timestamp`, so the meta shows `# seq · model · tokens-in→tokens-out` only. Justification: explicitly scoped to the spec'd interface; adding it is an additive change in a follow-up. +- **`go test ./...` walks `web/node_modules/flatted/golang/pkg/flatted`**: stray Go package shipped inside an npm dep; reports `[no test files]`, no failure. Justification: cosmetic; can be addressed by switching CI to `go test ./internal/...` or gitignoring the path. + +### Deferred (needs user input) + +- **CI configuration**: the plan calls for `.github/workflows/*` or `.sourcecraft/ci.yml`; neither directory exists in the repo. The CI job (`npm ci && npm run build && npm test && npm run lint`) is not implemented. To resume: confirm sourcecraft.dev's CI file format and create `.sourcecraft/ci.yml`, or add `.github/workflows/web.yml` if GitHub mirroring is in scope. + ### Deviations from plan - **Phase 2 — vite outDir**: set to `../internal/server/web/dist/` so vite output lands directly in the directory the Go embed will read in Phase 3. Plan implied a `web/dist/` location with an unspecified copy step; collapsing to a single dist location removes that step entirely. Root `.gitignore` updated accordingly (`internal/server/web/dist/*` with `.gitkeep` exception). @@ -354,13 +382,3 @@ Notes: - **Phase 5 — `session.$tool.$host.$id.tsx` placeholder created**: needed so TanStack Router types accept `Link` navigation from Home. Phase 6 replaces the placeholder with the real Session view. - **Phase 6 — `SessionWithTurns extends Omit`**: `Session.turns` is the aggregate count (number) from Phase 5; `SessionWithTurns.turns` is `Turn[]`. `Omit` resolves the conflict cleanly. SubBar turn-count display reads `session.turns.length`. -### Known minor gaps - -- **Turn meta line lacks timestamp**: the `Turn` TS interface in the plan's 6.1 spec did not include `timestamp`, so the meta line shows `# seq · model · tokens-in→tokens-out` only. The DTO has it; adding the field to `Turn` and the meta line is a 5-line follow-up if desired (deferred). -- **Manual interactive smoke not run** (Phases 4–6): the implementers had no display environment for `vite dev` browser checks. `up:uverify` will cover it. -- **Docker image not built** (Phase 3): the multi-stage Dockerfile change compiles in principle but was not exercised. `docker build .` smoke recommended before deploy. - -### Deferred (needs user input) - -- **Phase 3 — CI configuration**: the plan calls for `.github/workflows/*` or `.sourcecraft/ci.yml`; neither directory exists in the repo. The CI job (`npm ci && npm run build && npm test && npm run lint`) is not implemented in this phase. To resume: either confirm sourcecraft.dev's CI file format and create `.sourcecraft/ci.yml`, or add a `.github/workflows/web.yml` if GitHub mirroring is in scope. -- **Phase 3 — Dockerfile not docker-built**: the multi-stage `node:20-alpine` builder + `COPY --from=web-builder /internal/server/web/dist /src/internal/server/web/dist` was added but not exercised via `docker build`. Recommend a docker-build smoke before the binary is deployed.