Repository navigation
Rework the dashboard overview and split settings by editability - #61
Merged
Merged
Conversation
The header breadcrumb rendered the raw navigation key (nav.settings) instead of the translated label. The two disabled legacy text-log settings no longer appear in the management UI (hidden in the schema, startup parsing unchanged). The settings list is split into tabs for editable items, externally locked ones (CLI/env/.env) and internal read-only values, so locked entries stop crowding the editable form.
…rend The summary row becomes eight cards (requests, success rate with P95 latency, failures with anomalous-point count, Credit used, total/input/ output/cached tokens with shares and hit rate), each with a sparkline and a comparison against the previous period returned by a new previous aggregate plus a p95_duration_ms computed from retained request rows. The request trend becomes a three-series chart (requests, tokens, Credit) with independent axes, hover and keyboard tooltips, a legend and the existing data-table fallback. The credential health, model ranking and official balance panels cap their height and scroll internally so long lists no longer stretch the page.
Reviewer's GuideThe PR substantially reshapes the admin console by adding KPI-driven, zoomable dashboard analytics; separating storage operations into a dedicated archival and maintenance page; and clarifying settings ownership and administrative controls, with corresponding backend, UI, accessibility, and test coverage. Sequence diagram for verified audit log archivingsequenceDiagram
actor Admin
participant Storage as Storage page
participant API as Admin API
participant Audit as Audit store
participant DB as SQLite
participant Archive as JSONL archive
Admin->>Storage: Submit archive window
Storage->>API: POST /admin/storage/archive
API->>Audit: archive(days)
Audit->>DB: BEGIN IMMEDIATE
Audit->>DB: Read old requests and events in batches
Audit->>Archive: Write exported JSONL
Audit->>DB: Delete exported rows in batches
Audit->>DB: COMMIT
Audit-->>API: Archive counts and file
API-->>Storage: JSON response
Storage-->>Admin: Show archive result
Sequence diagram for zoomable dashboard usage trendsequenceDiagram
participant Dashboard as Dashboard page
participant API as Admin API
participant Store as Audit store
participant Chart as UsageTrend
Dashboard->>API: GET /admin/dashboard
API->>Store: fetch()
Store-->>API: summary, previous, daily series
API-->>Dashboard: Dashboard data
Dashboard->>API: GET /admin/dashboard?granularity=hour
API->>Store: fetch()
Store-->>API: hourly series
API-->>Dashboard: Hourly data
Chart->>Chart: Wheel zoom
Chart->>Chart: Swap to hourly rows within 48h
Chart->>Chart: Double-click or Escape reset
Flow diagram for settings ownership groupsflowchart TD
Settings[Settings schema]
Resolve[resolve_settings]
Hidden[Hide retired settings]
Source{Configuration source}
Editable[Editable tab]
External[Externally locked tab]
System[System read-only tab]
Settings --> Resolve
Resolve --> Hidden
Hidden --> Source
Source -->|default and unlocked| Editable
Source -->|CLI, environment, dotenv| External
Source -->|sensitive or other locked source| System
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 11 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="web/src/pages/Models.tsx" line_range="496" />
<code_context>
export function Models() {
- const resource = useResource("/models", modelResponse);
- const credentials = useResource("/credentials", credentialResponse);
+ const resource = useResource("/models", modelResponse, { cache: true });
+ const credentials = useResource("/credentials", credentialResponse, { cache: true });
const [query, setQuery] = useState("");
</code_context>
<issue_to_address>
**Concurrent model edits overwrite changes**
When a model changes in another tab after this page's cached snapshot is shown but before the open editor is saved, a cached model can open in `ModelEditor` before its refresh completes. If another tab changes that model and the refresh returns while the editor is open, the editor retains its old `rule` but receives the new `revision`; saving then passes the new revision with stale fields, overwriting the other tab's changes.
Keep the editor's base revision tied to the snapshot used to initialize its form, or refresh/reconcile the form before allowing a save.
</issue_to_address>
### Comment 2
<location path="app/audit_store.py" line_range="653-688" />
<code_context>
+ with open(tmp, "x", encoding="utf-8") as handle:
</code_context>
<issue_to_address>
**Archived logs are lost**
When the host loses power or crashes after the database commit but before the archive file and rename are durable, `archive` commits deletion without first syncing the JSONL file and its directory entry, so SQLite preserves the deletions while the archive is lost or truncated; the logs disappear from both the database and archive.
Sync the archive file and containing directory to durable storage before committing deletion of the database rows.
Also at `app/audit_store.py:695-704`.
</issue_to_address>
### Comment 3
<location path="app/audit_store.py" line_range="631" />
<code_context>
+ cutoff = time.time() - days * 86400
+ # Export-before-delete with a generous deadline: the shared 1s SQL budget
+ # cannot cover large windows, and the lock is held for the whole archive.
+ if not self._lock.acquire(timeout=0.25):
+ self._fault(TimeoutError())
+ return {"ok": False}
</code_context>
<issue_to_address>
**Audit records are dropped**
When requests or events are recorded while an archive holds the store lock for more than 250 ms, `archive` holds the lock during export and deletion, while `_run` gives `record_request` and `event` only 250 ms to acquire it; they return the dropped-record fallback, so those audit records are missing.
Change the archive and recording lock strategy so recording requests do not time out and get dropped during an archive.
Also at `app/audit_store.py:638`.
</issue_to_address>
### Comment 4
<location path="app/audit_store.py" line_range="703" />
<code_context>
+ "DELETE FROM events WHERE id IN (SELECT id FROM events WHERE started_at<? LIMIT 2000)",
+ (cutoff,)).rowcount:
+ pass
+ self._db.execute("DELETE FROM ingest WHERE created_at<?", (cutoff,))
+ self._db.execute("COMMIT")
+ return {"ok": True, "archived_requests": exported["request"],
</code_context>
<issue_to_address>
**Replayed records inflate aggregates**
When an archived request or event is redelivered after its ingest row was deleted, `archive` deletes the archived IDs from `ingest`; if a request or event with one of those IDs is later redelivered, `record_request` or `event` no longer recognizes it as a duplicate and aggregates it again. Dashboard totals then count the same record twice.
Preserve deduplication IDs for the applicable replay window, or make aggregate ingestion idempotent independently of the ingest rows removed by archiving.
</issue_to_address>
### Comment 5
<location path="web/src/pages/Storage.tsx" line_range="28" />
<code_context>
+ return `${(n / 1024 ** 3).toFixed(2)} GB`;
+}
+
+function ArchivePanel({ reload }: { reload: () => void }) {
+ const [days, setDays] = useState(30);
+ const [busy, setBusy] = useState(false);
</code_context>
<issue_to_address>
**Default archive finds no old logs**
When the configured retention is 30 days and the page is opened before archiving, the storage-page load calls `/admin/storage`, whose `audit.storage()` runs `_prune()` using the configured retention period; with the default 30-day retention, this removes expired details before the default 30-day archive request can export them, so the archive action reports no logs.
Choose an archive default shorter than the configured retention, or provide a way to archive expired details before the storage read prunes them.
</issue_to_address>
### Comment 6
<location path="web/src/UsageOverview.tsx" line_range="333-336" />
<code_context>
+ if (bucketsValid && b !== null) return LEFT + ((b - ds) * (RIGHT - LEFT)) / (de - ds);
+ return LEFT + (index * (RIGHT - LEFT)) / (rows.length - 1);
+ };
+ const yAt = (key: string, index: number) =>
+ BOTTOM - ((number(rows[index][key]) ?? 0) / (maxima[key] ?? 1)) * (BOTTOM - TOP);
+ const pathOf = (key: string) =>
+ smoothPath(rows.map((_, i): [number, number] => [xAt(i), yAt(key, i)]));
+ const choose = (index: number) =>
+ setSelection({ rows, index: Math.max(0, Math.min(rows.length - 1, index)) });
</code_context>
<issue_to_address>
**Hourly chart values are wrong**
When a daily trend is zoomed to a window of 48 hours or less and hourly rows are available, `yAt` and `pathOf` read `rows` while the chart renders `displayRows`, so hourly positions use daily values and can throw when hourly indices exceed the daily rows. `choose` also stores `rows`, so hourly selections do not match and tooltips do not activate.
Use `displayRows` consistently for chart values, paths, and selection state.
Also at `web/src/UsageOverview.tsx:318`.
</issue_to_address>
### Comment 7
<location path="web/src/api.ts" line_range="180-184" />
<code_context>
- if (!controller.signal.aborted)
- setState({ data: null, loading: false, error: errorMessage(error) });
+ if (controller.signal.aborted) return;
+ setState((prev) =>
+ prev.data === null
+ ? { data: null, loading: false, error: errorMessage(error) }
+ : { ...prev, loading: false },
+ );
});
return () => controller.abort();
</code_context>
<issue_to_address>
**Old results appear current**
When a resource path changes or a refresh fails after data has loaded, `useResource` keeps `prev.data` without checking which path it belongs to, and its catch handler suppresses the error whenever data exists; callers therefore show the old response under the new selection without an error notice.
Associate retained data with its path, clear it when the path changes, and report refresh failures even when same-path stale data remains visible.
Also at `web/src/api.ts:170`.
</issue_to_address>
### Comment 8
<location path="app/audit_store.py" line_range="568-569" />
<code_context>
+ "SELECT json_extract(payload,'$.duration_ms') FROM requests WHERE started_at>=? AND started_at<=?",
+ (start, now)).fetchall() if r[0] is not None)
+ # Detail rows are pruned by retention; without them the percentile is unknown.
+ summary["p95_duration_ms"] = (durations[max(0, math.ceil(len(durations) * 0.95) - 1)]
+ if durations else None)
+ return {"summary": summary, "previous": previous, "series": series,
"models": [{"model": key, **value} for key, value in models.items()],
</code_context>
<issue_to_address>
**P95 misrepresents the selected period**
When retention or size-based pruning removes some request details within the selected range while other details remain, `dashboard` calculates P95 from only the retained request details, while the summary covers the full selected range; users see a partial-sample percentile presented as the period's P95.
Mark P95 unknown when request details do not cover the full selected range.
Also at `app/audit_store.py:564-567`.
</issue_to_address>
### Comment 9
<location path="web/src/pages/Storage.tsx" line_range="146" />
<code_context>
+docker compose down # 本地部署则停止 converter 进程
+
+# 2. 压缩（在部署目录执行）
+sqlite3 auth/logs.sqlite3 "PRAGMA wal_checkpoint(TRUNCATE); VACUUM;"
+
+# 3. 重启网关
</code_context>
<issue_to_address>
**Compression targets the wrong database**
When the deployment uses a custom managed auth directory or is a local non-Compose install, the instructions always target `auth/logs.sqlite3` and prescribe a Docker Compose restart, so operators can vacuum the wrong database and leave the live database uncompressed or be unable to follow the instructions.
Document how to locate the configured live database and give restart instructions appropriate to the deployment type.
</issue_to_address>
### Comment 10
<location path="web/src/storage.test.tsx" line_range="39" />
<code_context>
+ fireEvent.click(screen.getByRole("button", { name: "归档并删除" }));
+ await waitFor(() => expect(post).toHaveBeenCalledWith("/storage/archive", { days: 30 }));
+ expect(await screen.findByText(/已归档 6 条/)).toBeTruthy();
+ expect(get).toHaveBeenCalledWith("/storage", expect.objectContaining({}));
+ });
+});
</code_context>
<issue_to_address>
**Refresh is never verified**
When the code omits the post-archive reload or the remount refresh, when the archive succeeds but either follow-up refresh is omitted, `storage page` still passes because line 39 matches the initial GET, and `cached resources` still passes because lines 65–66 only observe the cached snapshot. Both tests pass without verifying a refresh occurred or completed.
Assert that the GET count increases after archiving and that the remount issues a second GET whose fresh response replaces the cached snapshot.
Also at `web/src/storage.test.tsx:65-66`.
</issue_to_address>
### Comment 11
<location path="web/src/i18n.test.tsx" line_range="58" />
<code_context>
- fireEvent.click(screen.getByRole("button"));
- // eslint-disable-next-line no-console
+ expect(screen.getByTestId("interp").textContent).toBe("尝试 3");
+ expect(localStorage.getItem("codebuddy2api.lang")).toBe("zh");
+ act(() => chooseLanguage("en"));
expect(screen.getByTestId("probe").textContent).toBe("Overview");
</code_context>
<issue_to_address>
**Drawer language change goes unverified**
When a drawer choice is persisted without updating the active language, when the drawer handler stores the selected preference but does not notify the language provider, `applies language choices made in the appearance drawer` still passes: line 53 checks only localStorage, so the displayed language can remain unchanged.
Assert that visible translated text changes after selecting a language in the appearance drawer.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 11 findings to address first, and archiving removes old request and event rows from SQLite after exporting them to JSONL. Reverting the code will not put those rows back in the database, but the archived data remains available for a bounded manual restore.
Blocking findings: web/src/pages/Models.tsx:496, app/audit_store.py:688, app/audit_store.py:631, app/audit_store.py:703, web/src/pages/Storage.tsx:28, and 6 more
There was a problem hiding this comment.
Hey - I've found 11 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="web/src/api.ts" line_range="153-175" />
<code_context>
+const resourceCache = new Map<string, unknown>();
</code_context>
<issue_to_address>
**Previous session data appears**
When a different administrator signs in in the same page or tab after the cache has been populated, `resourceCache` is keyed only by request path and survives authentication changes, so the new session immediately renders the previous session’s model or credential data while its request is pending.
Scope cached entries to the authenticated session or clear the cache when the session changes.
</issue_to_address>
### Comment 2
<location path="app/audit_store.py" line_range="688" />
<code_context>
+ exported["event"] += len(rows)
+ last = (rows[-1][1], rows[-1][0])
+ os.chmod(tmp, 0o600)
+ os.replace(tmp, target)
+ except Exception:
+ try:
</code_context>
<issue_to_address>
**Archived records appear twice**
When the process stops after `os.replace` publishes the archive and before the database transaction commits, `archive` publishes the JSONL file before committing row deletions, so a crash leaves both the file and the original rows; a retry exports those rows again and operators see duplicate archived records.
Make publishing the archive and committing the deletions crash-safe so a retry cannot export the same rows again.
</issue_to_address>
### Comment 3
<location path="web/src/UsageOverview.tsx" line_range="314-337" />
<code_context>
+ domain && granularity !== "hour" && hourlyRows?.length && de - ds <= 48 * HOUR_STEP
</code_context>
<issue_to_address>
**Hourly trend uses daily data**
When a daily trend switches to hourly buckets, `yAt`, `pathOf`, and `choose` continue to read or store `rows` instead of `displayRows`, so the chart pairs daily values with hourly positions and hourly selections cannot activate the tooltip; an hourly index beyond the daily rows can also throw while rendering.
Use `displayRows` consistently for chart values and selection.
</issue_to_address>
### Comment 4
<location path="web/src/pages/Dashboard.tsx" line_range="181-183" />
<code_context>
+ <UsageTrend
+ rows={d.series}
+ hourlyRows={hourly.data?.series}
+ granularity={actualGrain}
+ partial={period.partial === true}
+ />
+ </Panel>
<div className={s.dashboardGrid}>
</code_context>
<issue_to_address>
**Hourly incompleteness goes unmarked**
When hourly aggregates are incomplete but the selected daily range is complete, `Dashboard` passes the daily range’s `partial` flag to `UsageTrend` after it switches to hourly rows, so the chart presents incomplete hourly data without a warning.
Pass the hourly response’s partial flag when the trend displays hourly rows.
Also at `web/src/UsageOverview.tsx:314-315`.
</issue_to_address>
### Comment 5
<location path="web/src/pages/Storage.tsx" line_range="146" />
<code_context>
+docker compose down # 本地部署则停止 converter 进程
+
+# 2. 压缩（在部署目录执行）
+sqlite3 auth/logs.sqlite3 "PRAGMA wal_checkpoint(TRUNCATE); VACUUM;"
+
+# 3. 重启网关
</code_context>
<issue_to_address>
**Compaction targets the wrong database**
When the configured runtime root or `CODEBUDDY_AUTH_DIR` places the managed database outside `auth`, the displayed command always opens `auth/logs.sqlite3`, while the running audit database is under the configured managed directory; following the instructions vacuums a different or nonexistent database and leaves the active logs uncompressed.
Build the command using the configured audit database path instead of the fixed `auth/logs.sqlite3` path.
Also at `web/src/pages/Storage.tsx:142`.
</issue_to_address>
### Comment 6
<location path="web/src/api.ts" line_range="170" />
<code_context>
const controller = new AbortController();
- setState({ data: null, loading: true, error: null });
+ // A cached snapshot stays on screen during the refresh instead of flashing a spinner.
+ setState((prev) => ({ data: prev.data, loading: prev.data === null, error: null }));
void api
.get<unknown>(path, { signal: controller.signal })
</code_context>
<issue_to_address>
**Range changes show stale dashboard data**
When the user changes the dashboard range or granularity, `useResource` retains the previous response when `path` changes, so the dashboard continues rendering metrics for the old range while the newly selected range loads; if that request fails, the catch also preserves the old data without showing an error. The selected range can therefore display results for a different period.
Retain the previous snapshot only for refreshes of the same path; clear or explicitly mark data from a different path while it loads and if that request fails.
</issue_to_address>
### Comment 7
<location path="web/src/pages/Dashboard.tsx" line_range="126" />
<code_context>
const [granularity, setGranularity] = useState("auto");
const resource = useResource(`/dashboard?days=${days}&granularity=${granularity}`, normalize);
+ // Hourly buckets power the trend's zoom-in granularity swap.
+ const hourly = useResource(`/dashboard?days=${days}&granularity=hour`, normalize);
const d = resource.data;
const period = d?.range ?? {};
</code_context>
<issue_to_address>
**Refresh leaves hourly data stale**
When new audit data arrives after the dashboard loads and the user clicks Refresh before zooming, the refresh button reloads only the primary dashboard resource; the separately fetched hourly resource remains unchanged. After new requests arrive, zooming into the refreshed daily chart still shows the old hourly snapshot.
Reload both dashboard resources when the refresh button is clicked.
</issue_to_address>
### Comment 8
<location path="app/admin_api.py" line_range="526-527" />
<code_context>
+ try:
+ directory = Path(str(audit.path)).parent / "archives"
+ archives = sorted((entry.name for entry in directory.glob("audit-*.jsonl")), reverse=True)[:20]
+ except OSError:
+ pass
+ return JSONResponse({**result, "archives": archives})
+
</code_context>
<issue_to_address>
**Archive listing hides read errors**
When the archive directory cannot be read, an `OSError` while listing the archive directory is discarded, so `/admin/storage` returns an empty archive list and the page reports no archives instead of telling the operator that the listing failed.
Return a storage-listing error or include a visible degraded/error field instead of treating the failure as an empty directory.
</issue_to_address>
### Comment 9
<location path="web/src/pages/Storage.tsx" line_range="34" />
<code_context>
+ const [error, setError] = useState<string | null>(null);
+ const [result, setResult] = useState<string | null>(null);
+ return (
+ <Panel title="归档旧日志" hint="导出为 JSONL 后从库中删除，统计聚合不受影响">
+ <form
+ className={s.archiveForm}
</code_context>
<issue_to_address>
**Storage page ignores selected language**
When the user selects English, the storage page’s labels and instructions are hard-coded in Chinese, so users who select English still see Chinese throughout this page instead of translated storage controls.
Add storage-page strings to both language dictionaries and render them through the translation helper.
</issue_to_address>
### Comment 10
<location path="web/src/storage.test.tsx" line_range="39" />
<code_context>
+ fireEvent.click(screen.getByRole("button", { name: "归档并删除" }));
+ await waitFor(() => expect(post).toHaveBeenCalledWith("/storage/archive", { days: 30 }));
+ expect(await screen.findByText(/已归档 6 条/)).toBeTruthy();
+ expect(get).toHaveBeenCalledWith("/storage", expect.objectContaining({}));
+ });
+});
</code_context>
<issue_to_address>
**Stale storage data still passes**
When the archive succeeds but the page does not reload its storage data, the “shows sizes with reclaimable estimate and archives old logs on demand” test passes if a successful archive stops refreshing storage: `render(<Storage />)` already causes the initial GET, which satisfies the single `toHaveBeenCalledWith` assertion at line 39.
Clear the GET mock after the initial render, or assert that the archive causes an additional `/storage` request.
</issue_to_address>
### Comment 11
<location path="app/audit_store.py" line_range="687-689" />
<code_context>
+ ensure_ascii=False) + "\n")
+ exported["event"] += len(rows)
+ last = (rows[-1][1], rows[-1][0])
+ os.chmod(tmp, 0o600)
+ os.replace(tmp, target)
+ except Exception:
+ try:
+ tmp.unlink()
</code_context>
<issue_to_address>
**Committed archives are not crash-durable**
When power is lost after the database commit but before the unsynced archive reaches durable storage, `os.replace` publishes the archive before the database deletion commits, but the archive file and directory are never synced to durable storage. A power loss after the SQLite commit can therefore leave deleted audit rows without a durable archive copy.
Flush and `fsync` the archive file and its containing directory before committing the row deletions.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 11 findings to address first, and archiving writes old request and event details to a JSONL file, then deletes those rows from SQLite; reverting the code does not restore them, and recovery would require manually re-importing the archive. A missing or unusable archive can therefore leave audit details permanently unavailable.
Blocking findings: web/src/api.ts:175, app/audit_store.py:688, web/src/UsageOverview.tsx:337, web/src/pages/Dashboard.tsx:183, web/src/pages/Storage.tsx:146, and 6 more
…or the granularity swap
… pagination, and usage stats
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The overview page mixed a plain request trend with the rest of the dashboard, credential and storage pages lacked structured channel/usage views, and the settings page mixed model-routing guards and retired log knobs with general settings.
What changed
Dashboard overview
previousblock in/admin/dashboard, plussummary.p95_duration_msguarded against partial detail samples)Credentials workspace
全部账号,国内区域,国际区域,企业账号as an independent category) showing ready ratios and remaining official credits刷新列表,同步全部余额, a批量操作menu (批量签到,导入文件,导出已选), and添加凭证10 / 20 / 50 / 100 / 500 / 1000 / 2000per page, default20), both persisted inlocalStoragewhen changedcredential_stats()inAuditStore), and任务配置+详情opening a 4-tab detail drawer (概览与请求,自动化配置,绑定与冷却,原始诊断)Storage management (new page, sidebar entry)
数据库文件,WAL 文件,日志逻辑占用,可回收空间) and balanced 2-column panels (归档旧日志 + 归档文件and压缩数据库 + 在线清理)auth/archives/withfsyncdurability, short lock slices that do not block liverecord_requestcalls, and preservedingestdeduplication keysModel routing & Settings
表外模型拦截(model_guard) and模型能力预检(model_capability_guard) moved from Settings to the Model Routing toolbar as two pill switches