feat(ui): cursor pagination + lazy loading for admin dashboard - #752
feat(ui): cursor pagination + lazy loading for admin dashboard#752PaoloC68 wants to merge 59 commits into
Conversation
|
CHANGELOG reminder — This PR has no changelog fragment. Add a file to |
Review: nothing to review yetThis PR currently contains only the objective-setting empty commit ( What I checked
Suggested next steps
I'll re-review once there are real changes on the branch. Ping me (or re-trigger the review workflow) after pushing. |
…ware, and EXPLAIN capture
Review: PR #752 — admin UI performance investigation (baseline)This is foundational infrastructure for a perf-test suite — seeding, a Server-Timing middleware, an EXPLAIN dump script, and a runner. The structure is solid and the isolation discipline around the perf DB is excellent. A few concrete issues worth addressing before this is built on. Blocking-ish1. 2. 3. Seeding payload sizes don't match docstrings. 4. Missing changelog fragment. Code quality5. Importing private symbol 6. Index drop/recreate list in seeder drifts from schema. 7. 8. Smaller things
What's good
SummaryLand after: wiring the middleware (or splitting it into its own follow-up PR), removing/skip-ing the stub fixtures, fixing the payload-size mismatch, and adding the changelog fragment. The seeder's index-list drift and the |
Review: PR #752 — perf-baseline (Server-Timing + perf-tests tier)Solid scaffolding overall. The isolation safeguards on the perf DB, the deterministic seeding, the cold/warm split in Bugs / Concerns1. Unit-test tier imports Playwright at collection time
from tests.luthien_proxy.perf_tests.conftest import (
PageLoadMetrics, ScrollFPSMetrics, _percentile, n_runs,
)That conftest does Additionally, 2. Perf gateway fixture's event-loop cleanup is unsafe
db_pool = DatabasePool(perf_db_url)
cleanup_loop = asyncio.new_event_loop()
...
cleanup_loop.run_until_complete(db_pool.close())The DB pool is created here but its internal connections are opened by the gateway's lifespan, running on uvicorn's loop in the daemon thread. Calling 3.
|
Review: PR #752 — perf-baseline (follow-up notes)Two earlier reviews on this PR already cover most of the structural and code-quality issues — New / unflagged issues1. 2. Integration test asserts only the absence path. 3. 4. Heap-growth threshold (50%) is recorded but doesn't match the suspected-leak claim. 5. 6. "count > 0 means already seeded" gates are too loose across tests. 7. Payload-size SLOs in AGENTS.md aren't tested. 8. Smaller things
What's good (additions to prior reviews)
SummaryLand-blockers from the prior reviews still stand: the playwright-at-collection-time import (second review's issue #1), the cross-loop 🤖 Generated with Claude Code |
…with integration tests
Review — admin UI performance investigation and fixThorough perf work — clear isolation contract for the perf DB, good test scaffolding, useful evidence artifacts. A few real issues to address before merge. 🔴 Bugs1. Broken placeholder remapping when cursor_filter = cursor_filter.replace(\"\$2\", \"\$3\").replace(\"\$3\", \"\$4\")Chained Test coverage gap: 2. Infinite scroll only fires once — <div x-intersect.once=\"loadNextPage()\" x-show=\"cursor !== null\" ...></div>
Either (a) drop 3. Hardcoded HMAC key for signed cursors — _CURSOR_HMAC_KEY = b\"luthien-perf-cursor-key-dev\"The comment acknowledges this should come from settings. With a static, public key, the signature provides no real tamper protection — anyone reading the repo can forge cursors. Pagination cursors aren't catastrophic to forge (just timestamps + session_ids), but if the intent is integrity, pull this from 🟡 Code quality4. Schema drift risk in seeding — The seeder hardcodes a list of indexes to drop/recreate. This duplicates knowledge of production migrations. If a new migration adds/removes an index, the seeder's perf measurements diverge from real-world behavior without anyone noticing. Consider re-running 5. Inconsistent
6. 7. Postgres branch unimplemented in perf module — 8. Cursor module name — 🟢 What's working well
Test coverage gaps to plug
Happy to pair on any of these if helpful. |
Review — admin UI performance investigation and fixThe previous four reviews on this PR cover the main structural issues well (signed-cursor key, 🔴 New bugs1. Either wire 2. Fragment template's CSS classes don't match the styled card classes — rendered rows are unstyled and the count bookkeeping is broken. ```html
...
...
\`\`\`
But Worse, the JS counter at ```javascript Since fragments produce Suggest: rewrite 3. Potential XSS in ```javascript
4. Turns infinite-scroll mechanism in ```html
\`\`\`
Three compounding problems:
Cumulative effect: turns pagination loads 0 additional pages. 5. 🟡 Smaller / observability6. 7. The "No sessions found" empty-state element overlays the fragment container. 8. 9. The fragment endpoints have no 🟢 What's good (not already in prior reviews)
Test coverage gaps to consider
🤖 Generated with Claude Code |
…ursor_hmac.key auto_provision_defaults() was generating a fresh random key on every startup and writing it only to os.environ. Consequences: (1) every restart invalidated outstanding pagination cursors; (2) multi-worker deployments produced different keys per worker, causing intermittent 400s when a cursor minted by worker A was validated by worker B. Fix: read the key from ~/.luthien/cursor_hmac.key on startup; generate and persist it there on first boot (mode 0o600). Subsequent restarts and sibling workers sharing the same filesystem get the same key. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ser_id guard
Two fixes in fetch_sessions_page:
1. SQLite 30days filter: created_at can be stored as '2025-01-15T10:00:00' (ISO T separator, as integration tests write) or '2025-01-15 10:00:00' (space separator, as the perf seeder writes). String comparison against datetime('now', '-30 days') output (space format) is alphabetically wrong for T-format timestamps because ' ' < 'T'. Wrap last_ts in datetime() to normalize both formats before comparison.
2. user_id parameter: add user_id: str | None = None to the signature for API symmetry with fetch_session_list. Raise NotImplementedError if a non-None value is passed — this makes the missing scoping an explicit contract violation rather than a silent data leak if the endpoint is ever wired to a non-admin context.
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
PR #752 ReviewThanks for the substantial work here — overall a solid, well-tested PR. The cursor pagination design is clean, the SQL-injection invariants are well-documented with inline comments, and the test coverage is good (cursor tamper, SQL injection through Bugs / correctness
Performance
Security
Style / nits
Things I checked and liked
Happy to dig deeper on any of the above — items 1–4 are the ones I'd want fixed before merge. |
- Catch ValueError from UUID parse in fetch_session_turns_page at the route level (ui/routes.py) so a cross-backend cursor returns 400 instead of 500 - Fix search/filter race in history_list.html: store pending load state instead of silently dropping requests that arrive while a fetch is in-flight; re-issue after the current load completes - Scope snapshotExpandState selectors to #conversation-container to avoid clobbering unrelated elements with .visible/.expanded/.open - Add multi-replica note to .env.example for CURSOR_HMAC_KEY
The multi-replica warning belongs in config_fields.py (the source of truth), not hand-edited into .env.example. Update the description and regenerate so the test_env_example_matches_generator guard passes.
Code Review — PR #752 (Cursor pagination + lazy loading for admin dashboard)Did a deep read of the new cursor module, history-service additions, fragment routes, JS, and tests. Overall this is a careful piece of work — strong security mindset around cursor signing and SQL injection, a regression test for ordering edge cases ( 🔴 High —
|
Code Review — PR #752 (cursor pagination + lazy loading)Solid PR overall: the cursor design is sound, the perf harness is a real asset, and test coverage is broad. A few issues stood out — one likely blocker, several worth addressing before merge. 🔴 Likely bug — pagination broken for SQLite
Page 2 of Fix: only 🟡 Notable
Double cursor-decode: route handlers (
🟢 Minor
🟢 Strengths worth calling out
🤖 Generated with Claude Code |
- Gate _UUID(cursor_event_id) on not db_pool.is_sqlite; SQLite event ids are plain strings (not UUIDs), so page 2 of the turns endpoint was throwing ValueError → 400 on all SQLite deployments - Bind cursor_ts as datetime directly in Postgres query instead of isoformat string to avoid implicit cast and index surprises - Add TypeError to decode_cursor except clause so a non-dict JSON payload (e.g. int or null) raises ValueError(400) not TypeError(500) - Remove dead isinstance(key, str) branch in cursor._get_hmac_key; settings type is str, the else branch was unreachable - Fix escapeHtml to also escape double and single quotes so values are safe in HTML attribute contexts, not just text nodes - Mark CURSOR_HMAC_KEY as dynamic_default=True so .env.example emits a blank value instead of the dev sentinel; regenerate .env.example - Remove dead user_id param from fetch_sessions_page; it raised NotImplementedError unconditionally and was never passed by any caller
Code ReviewSolid PR overall — cursor design with HMAC signing is clean, the threat model is documented inline, and the perf trade-offs are called out in the changelog with "known limitations." A few items worth addressing before merge. 🐛 Likely bug — Postgres timestamp parameter type mismatchIn query_args.append(cursor_ts.isoformat())But the companion cursor_ts if not db_pool.is_sqlite else cursor_ts.isoformat(),
🐛 Minor —
|
- Bind cursor_ts as datetime (not isoformat string) in Postgres branch of fetch_sessions_page; the turns endpoint already did this correctly, sessions page was inconsistent and risked lexicographic comparison instead of timestamp comparison - Drop stats.events++ in handleSSEEvent; updateStats() already recomputes the count from rawEvents, so the increment drifted after the FIFO cap evicted events from a bucket - Drop outer [:100] slice on preview text; _extract_preview_message already truncates to 100 chars and appends '...', the second slice was silently removing the ellipsis on long messages - Add multi-replica CURSOR_HMAC_KEY caveat to changelog known limitations
Code Review — Cursor Pagination + Lazy LoadingOverall a clean, well-scoped change. Cursor design is solid: HMAC-signed, opaque, idempotent, with tamper-rejection tests. The N+1 avoidance pattern in A few things worth addressing — none are blockers but several are worth fixing before merge: 1. Cursors aren't scoped to the endpoint that issued them (medium)A cursor produced by
Easy fix: add a 2.
|
- Add "kind" field to cursor payload (sessions/turns); decode_cursor now requires the expected kind and raises ValueError on mismatch, preventing a sessions cursor from silently producing wrong results on the turns endpoint (SQLite path had no UUID check to catch it) - Remove cursor_where_clause() helper; it returned named placeholders that the single caller immediately replaced with positional ones via string replacement — misleading API. Inline the fragment directly. - Fix non-atomic cursor_hmac.key write: use NamedTemporaryFile + os.replace so concurrent startup (uvicorn reload, parallel workers) cannot read a partial file - Remove unused Alpine AJAX + Intersect scripts from conversation_live.html; lazy-loading is deferred to a follow-up PR, no need to ship ~50 KB of JS - Restore 404-specific error message in loadInitial(); generic "please refresh" was shown for stale/deleted session URLs - Add comment explaining active-time cursor semantics in fetch_sessions_page - Update test_cursor.py: add kind= to all encode/decode calls, replace the two cursor_where_clause tests with test_wrong_kind_rejected
Code review notesOverall a strong PR: cursor design is clean, SQL is parameterised, fragment templates use autoescape, and the Bugs / correctness
Performance
Operational
Test coverage
Style / minor
|
Review: cursor pagination + lazy loadingReviewed by Claude. Solid implementation overall — cursor design is sound, the security comments in Issues🔴 SQLite cursor pagination is broken in production (timestamp-format mismatch).
Lex order:
The integration tests ( Fix options:
Whichever you pick, add a unit test that uses 🟡 Fragment endpoints lack cache-control headers.
🟡 PR scope ≠ One PR = One Concern (per CLAUDE.md). PR body says "Depends on PR #753 (perf harness + Server-Timing middleware)" but this PR also contains the perf harness itself: 🟡 Cursor is decoded twice per request. In 🟡
Smaller things
What I liked
|
- Fix SQLite cursor timestamp format mismatch: datetime('now') produces
space-separated strings ('2025-01-15 10:00:00') but cursor_ts.isoformat()
produces T-separated strings ('2025-01-15T10:00:00+00:00'). Since
' ' < 'T' lexicographically, page-2 cursor comparisons were wrong in
production. Fix: wrap both sides in datetime() in the SQLite WHERE
clause so SQLite normalizes the format before comparing.
- Add Cache-Control: no-store to /ui/fragments/ responses; fragment HTML
embeds HMAC-signed cursors that become 400s after key rotation if a
stale cached fragment is replayed
- Log whether CURSOR_HMAC_KEY was freshly generated or loaded from disk
so operators can diagnose ephemeral-FS restarts (Railway/Render/Fly)
that silently invalidate all in-flight cursors
- Preserve exception chain in ui/routes.py: raise HTTPException from e
instead of bare raise, keeping debug context in tracebacks
Review by Claude (Opus 4.7)Overall this is a well-executed PR with clear separation of concerns, thoughtful security framing, and good test coverage. The cursor-pagination design is sound, the changelog is honest about known limitations, and the SQL-injection invariants are clearly documented in code and verified by tests. A few small issues and one latent JS bug stood out — none are blockers. Strengths
Issues1. Latent bug in `conversation_live.js:485` — `this.rawEvents[callId]` uses the escaped form of `turn.call_id`: ```js But `handleSSEEvent` populates `this.rawEvents` keyed by the raw `call_id` from the event payload. UUIDs have no escapeable characters so this is harmless in practice, but if a `call_id` ever contains `<`, `>`, `&`, `"`, or `'`, the event timeline silently shows nothing for that turn. Suggest splitting: keep a separate `escapedCallId` for HTML interpolation, and use `turn.call_id` for the dict lookup. 2. `q` route parameter has no `max_length` (`ui/routes.py:274`) The service layer caps `q` to 128 chars via slicing (good defense-in-depth), but the route uses `Query(default=None)`. Adding `max_length=128` would surface a clean 422 to clients instead of silently truncating. The commit log mentions "enforce q max_length" but it only got applied at the service layer. 3. Cursor double-decode (`ui/routes.py:243-248, 283-288`) Both the route handler and the service call `decode_cursor(...)`. The route validates so the DB isn't touched for malformed cursors — fair — but the two checks can drift over time. Consider letting the service raise and translating to `HTTPException` in one place. 4. Backend behavior inconsistency: `LIKE` vs `ILIKE` (`history/service.py:1213, 1262`) SQLite branch uses `LIKE` (case-sensitive by default), Postgres uses `ILIKE` (case-insensitive). The same `q=Alpha` returns different results across backends. Worth either documenting in helptext or normalizing (e.g. `LOWER(session_id) LIKE LOWER(?)` for SQLite). 5. `payload_preview` materializes the full JSON before slicing (`history/service.py:1158-1167`) ```python For multi-MB event payloads this serializes the whole payload just to take the first 200 chars. Consider DB-side `SUBSTR(payload, 1, 200)` or skipping the preview when the payload is large. Not urgent — the turns fragment endpoint isn't wired into the UI yet — but worth fixing before it ships. 6. `_jinja_env` is module-level singleton (`ui/routes.py:35-38`) Templates are loaded from disk on first `get_template` call and cached for the process lifetime. In dev, template edits won't appear without a server restart. Very minor; if desired: `auto_reload=os.environ.get("DEV") == "1"`. Observations (not action items)
Test coverageComprehensive — unit, integration, and perf tiers all touched. The pagination boundary test (`test_fetch_sessions_page_no_duplicates_across_pages`) and the SQL injection test (`test_user_id_filter_sql_injection_safe`) are particularly good. Nice to see the JSON API endpoint preserved with explicit `test_existing_json_endpoint_unchanged` regression checks. 🤖 Generated with Claude Code |
- Fix rawEvents key mismatch in conversation_live.js: callId is the HTML-escaped form of turn.call_id (for attribute contexts), but this.rawEvents is keyed by the raw call_id from SSE events. Add rawCallId = turn.call_id and use it for the dict lookup so event timelines work correctly if a call_id ever contains escapeable chars - Add max_length=128 to q Query param in fragment_sessions route; the service layer already slices to 128 but the route was silently truncating instead of returning a clean 422 - Normalize SQLite q search to case-insensitive: LOWER(session_id) LIKE LOWER(?) to match Postgres ILIKE behavior across backends
ReviewSolid, well-scoped PR. Strong security hygiene (HMAC-signed cursors, parameterized SQL everywhere, an explicit SQL-injection regression test, autoescaped Jinja templates), thoughtful comments on intentional trade-offs (the Bugs / correctness
Code quality
Test coverage
Minor / nits
SecurityLGTM overall. Cursor HMAC is appropriate for the threat model and clearly documented. Auto-provisioned dev secret has a startup warning. SQL injection invariants are explicit and tested. Frontend uses Jinja autoescape on fragments and a defensive |
- test_fetch_session_turns_page_first_page: basic turns pagination
- test_fetch_session_turns_page_second_page_space_timestamps: page-2
cursor comparison with space-format timestamps (datetime('now') default)
— the exact production format that was broken before the datetime()
normalization fix
- test_fetch_sessions_page_quick_filter_30days: 30-day filter returns
recent sessions and excludes old ones
- test_fetch_sessions_page_quick_filter_claude: claude filter matches
sessions with 'claude-code' in payload and excludes others
- test_fetch_session_turns_page_postgres_non_uuid_cursor_raises: Postgres
UUID branch raises ValueError for a non-UUID cursor event_id; this is
the regression test for the named 'Postgres UUID fix' in the PR
- Add comment on SQLite cross-backend cursor sharp edge: a Postgres-issued
cursor decoded by a SQLite instance silently returns wrong pages
Review — cursor pagination + lazy loadingRead the cursor utility, history service additions, fragment routes, Worth fixing before merge1. if os.path.exists(key_path):
with open(key_path) as f: value = f.read().strip()
else:
value = secrets.token_urlsafe(32)
...os.replace(tmp_path, key_path)If two processes start simultaneously without Fix: after generating, re-read the file (someone else may have won the race) and use whatever's on disk. Or 2. Code quality3. 4. 5. Fragment vs legacy JSON drift on TestsSolid:
Not concerns
Nothing here is a merge blocker except the |
# Conflicts: # changelog.d/perf-baseline.md # scripts/perf_explain.py # scripts/perf_report.py # scripts/run_perf.sh # src/luthien_proxy/history/service.py # src/luthien_proxy/main.py # src/luthien_proxy/perf/db.py # src/luthien_proxy/perf/seeding.py # src/luthien_proxy/perf/timing_middleware.py # tests/luthien_proxy/e2e_tests/sqlite/test_activity_stream_regression.py # tests/luthien_proxy/integration_tests/test_server_timing.py # tests/luthien_proxy/perf_tests/AGENTS.md # tests/luthien_proxy/perf_tests/conftest.py # tests/luthien_proxy/perf_tests/test_api_contract.py # tests/luthien_proxy/perf_tests/test_page_load.py # tests/luthien_proxy/perf_tests/test_sse_memory.py # tests/luthien_proxy/perf_tests/test_throttled_network.py # tests/luthien_proxy/perf_tests/test_transcript_open.py # tests/luthien_proxy/unit_tests/perf/test_db.py # tests/luthien_proxy/unit_tests/perf/test_harness_helpers.py # tests/luthien_proxy/unit_tests/perf/test_seeding.py # tests/luthien_proxy/unit_tests/perf/test_timing_middleware.py
|
Claude-generated merge-queue triage of all open Luthien PRs, requested by Scott (Jul 7, 2026). Advisory only; Scott has not yet acted on these recommendations. Recommendation: close. The two slow paths this targeted are being addressed by #795 (turn-paginated, bounded-memory session detail, which is the follow-up this PR explicitly deferred) together with the list-path fixes in the same queue. This branch is roughly 600 commits behind and conflicts with the history UI, which has since gained per-user filters, labels, search, and two XSS-hardening passes, so the frontend would need a near-total redo; the last review round also had open correctness findings in the pagination path. The cursor-pagination design remains a useful reference in the branch. |
2 participants
Summary
Cursor-paginated infinite scroll for the history page. Fixes the admin dashboard slowness reported by Sami.
Depends on: PR #753 (perf harness + Server-Timing middleware)
Changes
Backend
utils/cursor.py): HMAC-signed opaque base64url cursors for composite(last_ts, session_id)paginationGET /ui/fragments/sessionsandGET /ui/fragments/sessions/{id}/turns— cursor-paginated HTML fragments (turns endpoint ships for future use; not yet wired into the conversation viewer)filter=30daysandfilter=claudesupported server-sidefetch_session_turns_pagenow binds cursor event ID asuuid.UUIDtypeFrontend
history_list.html): Replaced?limit=10000fetch with Alpine AJAX infinite scroll (20 sessions/page). Session cards restored with full metadata + click-through links.conversation_live.html+conversation_live.js): Restored fullconversationViewerAlpine component.loadInitial()fetches the full session JSON via/api/history/sessions/{id}and renders structured turns. Conversation-page lazy loading (paginated turns on scroll) is deferred to a follow-up PR.rawEvents[callId]capped at 50 events per call_id (FIFO)Config
CURSOR_HMAC_KEYenv var — auto-provisioned as a random secret on first boot; startup warning logged when the dev default is in useChangelogs
changelog.d/perf-fix.md