Conversation
Add in-memory logging with admin endpoints and dashboard logs tab.
Adds a new Logs tab to inspect ingress/egress requests with filters, controls, and details, plus related log route fixes and changelog entry.
ReviewThanks for the contribution! The Logs tab is a useful feature. Here's feedback to get it merge-ready. Blocker: Merge conflict
Blocker: No tests
Blocker: Unrelated changes
Architecture: avoid manual
|
Summarize oversized request payloads, centralize log entry creation, and add tests for the new log shape and store behavior.
Review — post-refactor commit (e66dc08)Nice progress! Config integration, request summarization, type separation, and tests are all solid improvements. Here's what's left before merging. Must fix1. Remove inner try/catch in The current diff wraps the entire success path (withRetry → streaming/non-streaming → handleNonStreaming) inside a new inner try/catch. This changes error semantics — if Instead, keep the existing control flow untouched and just add // After withRetry returns successfully:
const rawResponse = await withRetry(...);
enqueueLogEntry({ requestId, direction: "egress", method: "POST", path: "/codex/responses",
model: req.model, provider: "codex", status: rawResponse.status,
latencyMs: Date.now() - startMs, stream: req.isStreaming });
// ... existing streaming/non-streaming logic unchanged ...And in the existing outer catch (the } catch (err) {
enqueueLogEntry({ requestId, direction: "egress", ..., error: err.message });
// existing error handling unchanged
}No try/catch restructuring needed. 2. Unify
Nice-to-have (can be follow-up)
Fix items 1–2, and this is good to merge. The rest can be follow-up PRs. |
Add the logs dashboard, backend log capture, and admin log routes so request flow can be inspected from the UI.
Reset logs pagination on filter changes, clear stale selected entries, validate logs query params, and add regression coverage for the route/store/hook behavior.
icebear0828
left a comment
There was a problem hiding this comment.
Review: feat: add dashboard logs tab
整体不错,架构清晰,测试覆盖也比较全面。以下是需要关注的问题:
🔴 Critical: 分页逻辑反了
src/logs/store.ts 的 list() 方法:
const sliced = results.slice(offset, offset + limit).reverse();records 按时间正序 push(最旧在前),slice(0, 50).reverse() 返回的是最旧的 50 条倒序显示。用户期望 page 0 看到最新的记录,但实际上最新的在最后一页。
测试只有 2 条记录没暴露这个问题。应该先对 filtered results 做 reverse 再分页,或者用 slice(-offset - limit) 之类的方式从尾部取。
🔴 Critical: 双重 ingress 日志
src/middleware/log-capture.ts 对每个请求都记录 ingress 日志,但 chat.ts、messages.ts、responses.ts 各自也 enqueue 了带 model/stream 信息的 ingress 日志。每个 API 请求会产生 2 条 ingress 记录,用户会很困惑。
建议二选一:要么只保留 middleware 的通用日志,要么只保留路由级别的详细日志(后者更有价值)。
🟡 Medium: capture_body 是死配置
Config schema 声明了 capture_body,Settings UI 也能切换,但实际代码中从未读取这个值。summarizeRequestForLog 始终只生成摘要,无论 capture_body 是 true 还是 false。要么实现它,要么先移除。
🟡 Medium: requestId 获取逻辑大量重复
chat.ts、messages.ts、responses.ts、proxy-handler.ts 中都有:
const requestId = c.get("requestId") ?? randomUUID().slice(0, 8);重复 5+ 次,建议统一放到中间件或工具函数里。
🟢 Low: Pagination 按钮未 i18n
web/src/pages/LogsPage.tsx 中 "Prev" 和 "Next" 是硬编码英文,其他文本都做了 i18n。
🟢 Low: 不相关的改动混入
packages/electron/__tests__/builder-config.test.ts 删除了 bin 目录相关断言,跟 logs 功能无关,建议拆到单独的 commit/PR。
🟢 Low: getRealClientIp try-catch 改动影响范围
包了 try-catch 吞掉异常返回空字符串。对日志场景合理,但它也改变了 auth middleware 的行为(之前会 throw),可能掩盖连接信息缺失的问题。建议至少加个 warn log。
Summary
| 级别 | 问题 | 建议 |
|---|---|---|
| 🔴 Critical | 分页取到的是最旧记录而非最新 | 修复 list() 排序逻辑 |
| 🔴 Critical | ingress 日志记录两次 | 去掉 middleware 或路由级别其中之一 |
| 🟡 Medium | capture_body 声明但未使用 |
实现或移除 |
| 🟡 Medium | requestId 获取重复 5+ 处 | 抽取到公共位置 |
| 🟢 Low | Prev/Next 未 i18n | 加翻译 key |
| 🟢 Low | builder-config 测试改动不相关 | 拆出去 |
| 🟢 Low | getRealClientIp 静默 catch | 考虑加 warn log |
这个问题不存在 因为日志需要记录入口和出口的两次不一样的请求 所以一次对网关的请求会记录两条日志 这很正常 |
Make the logs store paginate from newest-first results and wire capture_body into request summaries so the dashboard setting actually affects recorded payloads.
Persist a logs.llm_only setting, filter captured ingress logs to LLM and forwarded traffic when enabled, and expose the toggle in both settings and the logs page.
Move log-related controls into a dedicated settings drawer and keep the saved logs enabled setting in sync with the logs page toggle while leaving pause as an independent runtime state.
Summary
Test plan