feat: add admin dashboard and API behind ADMIN_ENDPOINTS_ENABLED and ADMIN_UI_ENABLED feature flags#73
Merged
SantiagoDePolonia merged 30 commits intomainfrom Feb 17, 2026
Conversation
Adds a lightweight admin dashboard UI and standalone REST API for monitoring GOModel usage metrics and registered models. Both are behind the ADMIN_ENABLED feature flag (defaults to true). Architecture: - Admin API (/admin/api/v1/*) - auth-protected REST endpoints usable by any client - Dashboard UI (/admin/dashboard) - embedded HTML with HTMX + Alpine.js + Chart.js - UsageReader interface with SQLite/PostgreSQL/MongoDB implementations for querying usage data - Dashboard pages skip auth; API respects GOMODEL_MASTER_KEY New endpoints: - GET /admin/api/v1/usage/summary?days=30 - GET /admin/api/v1/usage/daily?days=30 - GET /admin/api/v1/models - GET /admin/dashboard (UI) https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a feature-flagged Admin API and embedded Admin Dashboard UI, usage-tracking abstractions with SQLite/Postgres/Mongo readers, provider registry enhancements, and wiring into app/server initialization using new ADMIN_ENDPOINTS_ENABLED and ADMIN_UI_ENABLED configuration flags. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Dashboard as "Dashboard (static)"
participant Admin as "Admin API"
participant Usage as "UsageReader"
participant Storage as Storage
participant Registry as Registry
Client->>Dashboard: GET /admin/dashboard
Dashboard-->>Client: HTML + JS + CSS
Client->>Dashboard: GET /admin/static/* -> assets
Dashboard-->>Client: static assets
Client->>Admin: GET /admin/api/v1/usage/summary?days=30
Admin->>Usage: GetSummary(ctx, days)
Usage->>Storage: run aggregation/SQL
Storage-->>Usage: aggregated metrics
Usage-->>Admin: UsageSummary
Admin-->>Client: JSON response
Client->>Admin: GET /admin/api/v1/models
Admin->>Registry: ListModelsWithProvider()
Registry-->>Admin: []ModelWithProvider
Admin-->>Client: JSON response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@internal/admin/dashboard/dashboard.go`:
- Around line 40-44: The Index handler currently calls
h.indexTmpl.ExecuteTemplate with c.Response().Writer which can flush partial
HTML if template execution fails; change Handler.Index to execute the template
into an in-memory buffer (e.g., bytes.Buffer or strings.Builder) using
h.indexTmpl.ExecuteTemplate(buf, "layout", nil), check the returned error, and
only if nil copy the buffer contents to c.Response().Writer (after setting
Content-Type and status) so failures do not send a corrupt/partial HTTP 200
response; reference Handler.Index, h.indexTmpl.ExecuteTemplate and
c.Response().Writer when applying the change.
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 286-289: The .mono CSS rule currently quotes single-word font
names causing stylelint font-family-name-quotes warnings; edit the .mono
selector's font-family declaration to remove quotes around Menlo and Consolas
(leaving 'SF Mono' quoted because it contains a space) so it reads: font-family:
'SF Mono', Menlo, Consolas, monospace;.
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 25-28: Add an inline comment near the init() method where
this.apiKey is set from localStorage (localStorage.getItem('gomodel_api_key'))
to document the security trade-off: note that the dashboard is served without
authentication at /admin/dashboard so any JS on the same origin (including
compromised CDN scripts) can read the API key, that this is an accepted risk
under the current threat model, and reference fetchAll() usage and the related
code block (lines ~30-36) so future maintainers know why the key is stored
client-side and what to watch for when modifying or adding third-party scripts.
- Around line 189-192: The null/undefined guard in the formatNumber function is
redundant: replace the current conditional (n == null || n === undefined) with a
single n == null check so that formatNumber(n) returns '-' when n is null or
undefined and otherwise returns n.toLocaleString(); update the conditional
inside the formatNumber method accordingly.
- Around line 53-72: In fetchUsage, avoid unconditionally calling
summaryRes.json() and dailyRes.json(); first check each response's ok flag and
handle non-OK statuses (non-401) by setting a visible error state (e.g.,
this.error = true or this.fetchError), clearing or preserving safe defaults for
this.summary and this.daily, and skipping renderChart; keep the existing 401
handling for authError/needsAuth, but for other status codes read and log the
error body (await res.text() or json()) to processLogger/console and show a
user-facing message instead of assigning the error payload to
this.summary/this.daily; update the fetchUsage flow to only parse responses when
summaryRes.ok and dailyRes.ok, and ensure renderChart() is called only after
successful parsing.
In `@internal/admin/dashboard/templates/index.html`:
- Around line 7-11: The select element uses Alpine's x-model="days" (initialized
in dashboard.js) so the hardcoded selected attribute on the <option value="30">
conflicts with JS state; remove the selected attribute from the option with
value="30" and let x-model control the selected value (no other code changes
required), ensuring fetchUsage() continues to be triggered by
`@change`="fetchUsage()".
In `@internal/admin/dashboard/templates/layout.html`:
- Around line 20-27: The nav items duplicate inline click handlers to set the
page and toggle the active class; extract this logic into a shared Alpine method
(e.g., setPage) in dashboard.js and replace the inline handlers on elements with
`@click.prevent`="setPage('overview')" and `@click.prevent`="setPage('models')". The
setPage(name) method should set the Alpine state property page = name and
perform DOM updates equivalent to
$el.closest('.sidebar-nav').querySelectorAll('.nav-item').forEach(n =>
n.classList.remove('active')); $el.classList.add('active') so you only maintain
the logic in one place (refer to the nav-item element class and the page state
used in the template).
- Around line 7-9: The three external script tags loading Chart.js
(https://cdn.jsdelivr.net/npm/chart.js@4.4.7/dist/chart.umd.min.js), Alpine.js
(https://cdn.jsdelivr.net/npm/alpinejs@3.14.8/dist/cdn.min.js) and HTMX
(https://unpkg.com/htmx.org@2.0.4/dist/htmx.min.js) must include Subresource
Integrity and cross-origin attributes or be replaced with self-hosted assets:
add an integrity="sha384-..." attribute with the correct SRI hash for each
specific file and add crossorigin="anonymous" to each <script> tag;
alternatively download the exact versions into the embedded static assets and
update the src values to the local asset paths (removing CDN reliance) while
still using integrity/crossorigin if served from a CDN. Ensure you update the
three script tags referencing those exact URLs accordingly.
In `@internal/admin/handler.go`:
- Around line 35-40: Extract a small helper function (e.g., parseDays(c
echo.Context, defaultDays int) int) that encapsulates the query-param parsing
and validation logic currently duplicated in UsageSummary and DailyUsage: move
the existing block that reads c.QueryParam("days"), attempts strconv.Atoi, and
enforces parsed > 0 into that helper and return defaultDays on failure; then
replace the duplicated code in UsageSummary and DailyUsage with calls to
parseDays(c, 30) (or the appropriate default) so both handlers use the single
helper.
- Around line 42-49: GetSummary can return (nil, nil) which causes JSON null to
be returned; add a nil guard after calling h.usageReader.GetSummary in the
handler so that if summary == nil you replace it with an empty UsageSummary
value (e.g. &UsageSummary{} or the same empty-object shape used earlier) before
calling c.JSON(http.StatusOK, summary) to keep responses consistent with the
nil-reader fallback.
In `@internal/usage/factory.go`:
- Around line 113-149: Extract two small helper functions (e.g.,
pgxPoolFrom(store storage.Storage) (*pgxpool.Pool, error) and mongoDBFrom(store
storage.Storage) (*mongo.Database, error)) that encapsulate the nil-check and
type assertion logic currently duplicated in NewReader and createUsageStore;
replace the PostgreSQL and MongoDB case blocks in NewReader (and the analogous
blocks in createUsageStore) with single-line calls to these helpers and
propagate their errors instead of repeating the same boilerplate.
In `@internal/usage/reader_mongodb.go`:
- Around line 99-124: The nil-slice issue: initialize the result slice to an
empty non-nil slice so JSON encodes [] instead of null — change the declaration
of result (currently "var result []DailyUsage") to an empty slice (e.g.,
[]DailyUsage{}) before iterating the Mongo cursor in the function that uses
cursor.Next/Decode; leave the rest of the cursor loop and the cursor.Err() check
unchanged so you still append decoded DailyUsage rows to the now-initialized
result.
In `@internal/usage/reader_postgresql.go`:
- Around line 24-47: The SQL uses an unquoted table name usage which is a
reserved word; update all query strings in this file (e.g., the query variable
in PostgreSQLReader.GetSummary and any other methods in this file that reference
the table) to quote the table name as "usage" (use double quotes in the SQL
text) so every SELECT/FROM/WHERE that references the table uses "usage" instead
of usage.
In `@internal/usage/reader_sqlite.go`:
- Around line 69-82: GetDailyUsage currently returns a nil slice when no rows
are found which JSON-encodes to null; initialize the result slice to an empty
slice before iterating (e.g., set result := make([]DailyUsage, 0)) or ensure you
return an empty slice when len(result)==0 so callers get [] instead of null;
apply the same change in the MongoDB reader counterpart (the function with the
similar GetDailyUsage behavior) to keep responses consistent.
Comment on lines
+25
to
+28
| init() { | ||
| this.apiKey = localStorage.getItem('gomodel_api_key') || ''; | ||
| this.fetchAll(); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
API key stored in localStorage — acceptable given the threat model but worth documenting.
Since the dashboard page skips authentication (served at /admin/dashboard without auth), any JavaScript executing on that origin can read the stored API key. The practical risk here is limited to CDN compromise of the external scripts (flagged separately). However, consider adding a brief comment in the code noting this trade-off so future developers are aware.
Also applies to: 30-36
🤖 Prompt for AI Agents
In `@internal/admin/dashboard/static/js/dashboard.js` around lines 25 - 28, Add an
inline comment near the init() method where this.apiKey is set from localStorage
(localStorage.getItem('gomodel_api_key')) to document the security trade-off:
note that the dashboard is served without authentication at /admin/dashboard so
any JS on the same origin (including compromised CDN scripts) can read the API
key, that this is an accepted risk under the current threat model, and reference
fetchAll() usage and the related code block (lines ~30-36) so future maintainers
know why the key is stored client-side and what to watch for when modifying or
adding third-party scripts.
Comment on lines
+189
to
+192
| formatNumber(n) { | ||
| if (n == null || n === undefined) return '-'; | ||
| return n.toLocaleString(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant undefined check.
n == null (loose equality) already covers both null and undefined, making the n === undefined check redundant.
Simplification
formatNumber(n) {
- if (n == null || n === undefined) return '-';
+ if (n == null) return '-';
return n.toLocaleString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| formatNumber(n) { | |
| if (n == null || n === undefined) return '-'; | |
| return n.toLocaleString(); | |
| } | |
| formatNumber(n) { | |
| if (n == null) return '-'; | |
| return n.toLocaleString(); | |
| } |
🤖 Prompt for AI Agents
In `@internal/admin/dashboard/static/js/dashboard.js` around lines 189 - 192, The
null/undefined guard in the formatNumber function is redundant: replace the
current conditional (n == null || n === undefined) with a single n == null check
so that formatNumber(n) returns '-' when n is null or undefined and otherwise
returns n.toLocaleString(); update the conditional inside the formatNumber
method accordingly.
internal/admin/handler.go
Outdated
Comment on lines
+42
to
+49
| summary, err := h.usageReader.GetSummary(c.Request().Context(), days) | ||
| if err != nil { | ||
| return c.JSON(http.StatusInternalServerError, map[string]string{ | ||
| "error": "failed to fetch usage summary", | ||
| }) | ||
| } | ||
|
|
||
| return c.JSON(http.StatusOK, summary) |
Contributor
There was a problem hiding this comment.
Nil summary will serialize as JSON null, inconsistent with the nil-reader fallback.
GetSummary returns *UsageSummary. If a reader returns (nil, nil), line 49 sends null instead of the empty-object response on line 32. Add a nil guard for consistency.
Proposed fix
summary, err := h.usageReader.GetSummary(c.Request().Context(), days)
if err != nil {
return c.JSON(http.StatusInternalServerError, map[string]string{
"error": "failed to fetch usage summary",
})
}
+ if summary == nil {
+ summary = &usage.UsageSummary{}
+ }
+
return c.JSON(http.StatusOK, summary)🤖 Prompt for AI Agents
In `@internal/admin/handler.go` around lines 42 - 49, GetSummary can return (nil,
nil) which causes JSON null to be returned; add a nil guard after calling
h.usageReader.GetSummary in the handler so that if summary == nil you replace it
with an empty UsageSummary value (e.g. &UsageSummary{} or the same empty-object
shape used earlier) before calling c.JSON(http.StatusOK, summary) to keep
responses consistent with the nil-reader fallback.
Comment on lines
+113
to
+149
| // NewReader creates a UsageReader from a storage backend. | ||
| // Returns nil if the storage is nil (usage data not available). | ||
| func NewReader(store storage.Storage) (UsageReader, error) { | ||
| if store == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| switch store.Type() { | ||
| case storage.TypeSQLite: | ||
| return NewSQLiteReader(store.SQLiteDB()) | ||
|
|
||
| case storage.TypePostgreSQL: | ||
| pool := store.PostgreSQLPool() | ||
| if pool == nil { | ||
| return nil, fmt.Errorf("PostgreSQL pool is nil") | ||
| } | ||
| pgxPool, ok := pool.(*pgxpool.Pool) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid PostgreSQL pool type: %T", pool) | ||
| } | ||
| return NewPostgreSQLReader(pgxPool) | ||
|
|
||
| case storage.TypeMongoDB: | ||
| db := store.MongoDatabase() | ||
| if db == nil { | ||
| return nil, fmt.Errorf("MongoDB database is nil") | ||
| } | ||
| mongoDB, ok := db.(*mongo.Database) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid MongoDB database type: %T", db) | ||
| } | ||
| return NewMongoDBReader(mongoDB) | ||
|
|
||
| default: | ||
| return nil, fmt.Errorf("unknown storage type: %s", store.Type()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract shared type-assertion helpers to reduce duplication with createUsageStore.
The PostgreSQL and MongoDB type-assertion + nil-check boilerplate in NewReader (lines 124–143) is nearly identical to createUsageStore (lines 188–212). Consider extracting small helpers:
Example helper extraction
func pgxPoolFrom(store storage.Storage) (*pgxpool.Pool, error) {
pool := store.PostgreSQLPool()
if pool == nil {
return nil, fmt.Errorf("PostgreSQL pool is nil")
}
pgxPool, ok := pool.(*pgxpool.Pool)
if !ok {
return nil, fmt.Errorf("invalid PostgreSQL pool type: %T", pool)
}
return pgxPool, nil
}
func mongoDBFrom(store storage.Storage) (*mongo.Database, error) {
db := store.MongoDatabase()
if db == nil {
return nil, fmt.Errorf("MongoDB database is nil")
}
mongoDB, ok := db.(*mongo.Database)
if !ok {
return nil, fmt.Errorf("invalid MongoDB database type: %T", db)
}
return mongoDB, nil
}Both NewReader and createUsageStore would then collapse to single-line calls per case.
🤖 Prompt for AI Agents
In `@internal/usage/factory.go` around lines 113 - 149, Extract two small helper
functions (e.g., pgxPoolFrom(store storage.Storage) (*pgxpool.Pool, error) and
mongoDBFrom(store storage.Storage) (*mongo.Database, error)) that encapsulate
the nil-check and type assertion logic currently duplicated in NewReader and
createUsageStore; replace the PostgreSQL and MongoDB case blocks in NewReader
(and the analogous blocks in createUsageStore) with single-line calls to these
helpers and propagate their errors instead of repeating the same boilerplate.
internal/usage/reader_mongodb.go
Outdated
Comment on lines
+99
to
+124
| var result []DailyUsage | ||
| for cursor.Next(ctx) { | ||
| var row struct { | ||
| Date string `bson:"_id"` | ||
| Requests int `bson:"requests"` | ||
| InputTokens int64 `bson:"input_tokens"` | ||
| OutputTokens int64 `bson:"output_tokens"` | ||
| TotalTokens int64 `bson:"total_tokens"` | ||
| } | ||
| if err := cursor.Decode(&row); err != nil { | ||
| return nil, fmt.Errorf("failed to decode daily usage row: %w", err) | ||
| } | ||
| result = append(result, DailyUsage{ | ||
| Date: row.Date, | ||
| Requests: row.Requests, | ||
| InputTokens: row.InputTokens, | ||
| OutputTokens: row.OutputTokens, | ||
| TotalTokens: row.TotalTokens, | ||
| }) | ||
| } | ||
|
|
||
| if err := cursor.Err(); err != nil { | ||
| return nil, fmt.Errorf("error iterating daily usage cursor: %w", err) | ||
| } | ||
|
|
||
| return result, nil |
Contributor
There was a problem hiding this comment.
Same nil-slice concern as the SQLite reader — result will be null in JSON when there are no rows.
Proposed fix
- var result []DailyUsage
+ result := make([]DailyUsage, 0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var result []DailyUsage | |
| for cursor.Next(ctx) { | |
| var row struct { | |
| Date string `bson:"_id"` | |
| Requests int `bson:"requests"` | |
| InputTokens int64 `bson:"input_tokens"` | |
| OutputTokens int64 `bson:"output_tokens"` | |
| TotalTokens int64 `bson:"total_tokens"` | |
| } | |
| if err := cursor.Decode(&row); err != nil { | |
| return nil, fmt.Errorf("failed to decode daily usage row: %w", err) | |
| } | |
| result = append(result, DailyUsage{ | |
| Date: row.Date, | |
| Requests: row.Requests, | |
| InputTokens: row.InputTokens, | |
| OutputTokens: row.OutputTokens, | |
| TotalTokens: row.TotalTokens, | |
| }) | |
| } | |
| if err := cursor.Err(); err != nil { | |
| return nil, fmt.Errorf("error iterating daily usage cursor: %w", err) | |
| } | |
| return result, nil | |
| result := make([]DailyUsage, 0) | |
| for cursor.Next(ctx) { | |
| var row struct { | |
| Date string `bson:"_id"` | |
| Requests int `bson:"requests"` | |
| InputTokens int64 `bson:"input_tokens"` | |
| OutputTokens int64 `bson:"output_tokens"` | |
| TotalTokens int64 `bson:"total_tokens"` | |
| } | |
| if err := cursor.Decode(&row); err != nil { | |
| return nil, fmt.Errorf("failed to decode daily usage row: %w", err) | |
| } | |
| result = append(result, DailyUsage{ | |
| Date: row.Date, | |
| Requests: row.Requests, | |
| InputTokens: row.InputTokens, | |
| OutputTokens: row.OutputTokens, | |
| TotalTokens: row.TotalTokens, | |
| }) | |
| } | |
| if err := cursor.Err(); err != nil { | |
| return nil, fmt.Errorf("error iterating daily usage cursor: %w", err) | |
| } | |
| return result, nil |
🤖 Prompt for AI Agents
In `@internal/usage/reader_mongodb.go` around lines 99 - 124, The nil-slice issue:
initialize the result slice to an empty non-nil slice so JSON encodes [] instead
of null — change the declaration of result (currently "var result []DailyUsage")
to an empty slice (e.g., []DailyUsage{}) before iterating the Mongo cursor in
the function that uses cursor.Next/Decode; leave the rest of the cursor loop and
the cursor.Err() check unchanged so you still append decoded DailyUsage rows to
the now-initialized result.
- Add light mode with CSS custom properties, defaulting to system preference - Three-way theme toggle in sidebar: Light / Auto / Dark - Theme persists in localStorage, chart re-renders on theme switch - Remove provider dropdown filter, replace with unified text search - Filter now matches across model ID, provider type, and owned_by fields - Show filtered count (e.g. "5 / 120 models") when filter is active https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Wrap chart canvas in a fixed-height container to prevent the infinite resize loop that caused "Canvas exceeds max size" crashes. Update Chart.js 4.4.7→4.5.1, Alpine.js 3.14.8→3.15.8, HTMX 2.0.4→2.0.8. https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 27-68: The light-mode color variables are duplicated in the
[data-theme="light"] block and the `@media` (prefers-color-scheme: light)
:root:not([data-theme="dark"]) block; consolidate them so there’s a single
source of truth: either move the shared --bg, --bg-surface, --border, --text,
--text-muted, --accent, --accent-hover, --success, --warning, --danger,
--chart-grid, --chart-text, --chart-tooltip-bg, --chart-tooltip-border,
--chart-tooltip-text and color-scheme into one selector and have the other
selector reference it (e.g., combine selectors or generate both from one
source), or if you must keep plain CSS duplication, add a clear comment above
both [data-theme="light"] and the `@media` rule documenting that the duplication
is intentional and must be kept in sync; update the file around those selectors
([data-theme="light"] and `@media` (prefers-color-scheme: light)
:root:not([data-theme="dark"])) accordingly.
- Around line 403-408: The mobile media query hides .sidebar-footer (which
contains the theme toggle and API key), making them inaccessible; update the CSS
in dashboard.css so .sidebar-footer is not set to display: none on small screens
and instead either keep it visible (e.g., keep .sidebar-footer display:block) or
move the specific controls into the main content via new selectors — for example
remove or change the ".sidebar-footer { display: none; }" rule and add
responsive rules to position .api-key-input and .theme-toggle (or their
container .sidebar-footer) inside .content on small viewports (or implement an
expandable menu class) so the theme toggle and API key remain accessible on
≤768px.
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 217-225: The filteredModels getter can throw when model entries
lack expected shape; update it to defensively access fields using optional
chaining and fallbacks so toLowerCase() isn't called on undefined (e.g., use
(m.model?.id ?? '') and (m.model?.owned_by ?? ''), and guard m.provider_type
similarly) while keeping the same filtering logic and return this.models when
modelFilter is falsy; modify the filteredModels getter accordingly.
- Around line 62-71: chartColors() currently returns hardcoded hex values that
duplicate CSS custom properties; update chartColors (and keep using isDark() if
needed) to read the color values from the document's CSS variables (e.g.,
getComputedStyle(document.documentElement).getPropertyValue('--chart-grid'),
'--chart-text', '--chart-tooltip-bg', '--chart-tooltip-border',
'--chart-tooltip-text') and return those values (trimmed) with the existing hex
values as fallbacks if the CSS variable is missing so behavior is preserved.
- Around line 117-129: The fetchModels method currently calls res.json() for any
non-401 response, which can assign an error payload to this.models; update
fetchModels (the async fetchModels function) to check res.ok before parsing
JSON: if res.status === 401 keep the existing handling, else if !res.ok log the
error (include status/text), set a safe fallback such as this.models = [] and a
flag like this.fetchError or this.needsAuth=false as appropriate, and return —
only call await res.json() when res.ok is true; ensure this change mirrors the
safer handling used in fetchUsage so filteredModels and the x-for template
always receive a proper array.
In `@internal/admin/dashboard/templates/layout.html`:
- Around line 7-9: The Chart.js script tag is loaded synchronously and should
use defer to avoid blocking HTML parsing; update the script element that loads
"chart.js@4.5.1" to include the defer attribute (matching how Alpine.js is
loaded) since Chart.js is only used later by the renderChart() methods triggered
by Alpine.js/DOM-ready code, ensuring parallel download and non-blocking page
rendering.
Generate a continuous day-by-day date sequence between the first and last data points, inserting zero-value entries for days with no usage data so the chart always shows every day on the X-axis. https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 81-87: The headers() helper currently unconditionally adds
'Content-Type: application/json' which is unnecessary for the GET-only requests
in this file and can trigger CORS preflights; update the headers() function in
dashboard.js to stop adding 'Content-Type' (leave only the Authorization Bearer
header when this.apiKey exists) or alternatively only include 'Content-Type'
when a request body or non-GET method will be used; modify the headers()
implementation accordingly so GET requests are simple requests without the
Content-Type header.
- Around line 131-144: The fillMissingDays function builds Date objects using
new Date(dates[0] + 'T00:00:00') which parses as local time while the key uses
d.toISOString().slice(0,10) (UTC), causing off-by-one lookups against byDate;
fix by constructing start and end as UTC (append 'Z' to the 'T00:00:00' string)
so the dates array, start/end, loop variable d and the generated key all refer
to the same timezone and lookups into byDate succeed.
- Update all CSS color variables (dark + light + system) to match the GoModel website palette: warm browns (#755c3d/#b8956e), warm grays, and #34d399 green accent - Add GoModel hexagon logo SVG in sidebar header using currentColor - Replace text-based Light/Auto/Dark toggle with compact icon-only pill (sun/monitor/moon SVGs, 28x24px buttons) - Add Inter font from Google Fonts - Update Chart.js dataset colors to brand palette (brown + green) - Update chartColors() helper to match new CSS variables https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 72-77: Remove unnecessary quotation marks from single-word
font-family names in the CSS; update the font-family declarations used in the
body selector (currently "'Inter', -apple-system, ...") and the monospaced stack
referencing "Menlo" and "Consolas" to use unquoted Inter, Menlo, and Consolas
respectively so they comply with font-family-name-quotes stylelint rule.
…flags Replace the single ADMIN_ENABLED flag with two independent feature flags: - ADMIN_ENDPOINTS_ENABLED: controls admin REST API (/admin/api/v1/*) - ADMIN_UI_ENABLED: controls dashboard UI (/admin/dashboard) Both default to true. When endpoints are disabled but UI is enabled, a warning is logged and UI is forced to disabled since the dashboard depends on the API endpoints. https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Wrap nav item text in <span> so it hides on mobile while keeping SVG icons visible. Add title= attributes for hover tooltips on mobile. Hide Admin badge and center logo on narrow screens. https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Replace the 3-button theme pill with a single cycle button on ≤768px viewports so theme switching remains usable in the collapsed sidebar. Resolve merge conflict keeping upstream sidebar centering improvements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cing - Fix Chart.js "t is null" error by deferring render to $nextTick and disabling animations to avoid async draw on hidden canvas - Fix "canvas already in use" error by moving destroy inside $nextTick - Fix fillMissingDays to span full selected period (today included) using local dates instead of UTC to avoid timezone offset - Add favicon.svg from gomodel.enterpilot.io - Add spacing between sidebar nav items Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add min-width: 0 to .content flex child so the Chart.js canvas can shrink when the browser window is narrowed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clickable handle at the sidebar border collapses/expands the sidebar to icon-only mode. Cursor changes to indicate direction (w-resize when expanded, e-resize when collapsed). State persists in localStorage. Hidden on mobile where sidebar is already collapsed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace opacity with color-mix for a cleaner hover transition that doesn't leave a visible fade-out when the mouse leaves. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Buffer template execution in dashboard handler to prevent partial HTML responses on error - Quote reserved word "usage" in PostgreSQL reader queries - Refactor dashboard JS fetch error handling and add null-safe property access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 119-137: The auth flag needsAuth is only set to true on a 401 and
never reset, so modify fetchAll() to clear it at the start of each fetch cycle
(alongside this.loading and this.authError) so the UI can recover after
successful re-auth; specifically update the fetchAll() method to set
this.needsAuth = false before calling this.fetchUsage() and this.fetchModels()
so subsequent successful responses will hide the auth prompt.
In `@internal/server/http.go`:
- Around line 147-160: The handlers UsageSummary, DailyUsage and ListModels in
the admin API routes are returning manual JSON error responses instead of using
the project's centralized error helper; update each handler to call the existing
handleError(c, err, "context message") pattern (or equivalent signature used
elsewhere) whenever an error occurs so GatewayError types are recognized and
typed JSON responses are returned consistently—locate the handlers referenced by
cfg.AdminHandler.UsageSummary, cfg.AdminHandler.DailyUsage and
cfg.AdminHandler.ListModels and replace their direct
c.JSON(http.StatusInternalServerError, ...) error returns with calls to
handleError passing the echo.Context, the error, and a short context message.
---
Duplicate comments:
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Line 75: Remove unnecessary quotes around single-word font-family names in the
CSS (e.g., change "font-family: 'Inter', -apple-system, BlinkMacSystemFont,
'Segoe UI', Roboto, sans-serif;" to use unquoted Inter and other single-word
families). Locate occurrences of the font-family declaration (e.g., the rule
containing font-family: 'Inter', ... and the other instance around line 491) and
strip quotes from single-word names like Inter, Menlo, Consolas while keeping
quotes only for multi-word families such as 'SF Mono' and 'Segoe UI'.
- Around line 513-526: Hiding .sidebar-footer entirely on mobile removes access
to the API key input and theme toggle; instead keep those controls reachable by
either (A) keeping .sidebar-footer visible but hiding only nonessential children
and showing .sidebar-footer .theme-toggle-mobile and .sidebar-footer
.api-key-section on small screens, or (B) moving the API key input and theme
toggle into a mobile-only container inside .content (e.g., .mobile-controls) and
ensure .theme-toggle-mobile and .api-key-section styles are visible under `@media`
(max-width: 768px); update CSS to remove or override "display: none" for
.sidebar-footer .api-key-section and .sidebar-footer .theme-toggle (or add
styles to show the mobile equivalents) and optionally enable .sidebar-toggle as
a visible collapsible trigger so the footer can be expanded on mobile.
In `@internal/admin/dashboard/templates/layout.html`:
- Around line 11-13: The three external <script> tags loading Chart.js,
Alpine.js and HTMX in layout.html must include Subresource Integrity and CORS
attributes and Chart.js should be deferred: add appropriate
integrity="<sha384-...>" and crossorigin="anonymous" to the Chart.js, Alpine.js
and HTMX <script> tags, and add defer to the Chart.js tag (it already has defer
for Alpine in the diff but Chart.js does not); alternatively replace those CDN
URLs by referencing the bundled/self-hosted static assets from your embedded
static filesystem. Ensure you update the exact script tags for Chart.js,
Alpine.js and htmx in the template.
Swap the static selection hint label for a subtle floating tooltip that tracks the mouse over the calendar. Uses surface background with accent border and directional arrow icons (|→ / ←|) to indicate start vs end selection. Reverts flex-wrap to nowrap so calendars stay side-by-side. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace generic error strings with handleError() that checks for GatewayError and returns properly typed HTTP status codes, matching the pattern used by the main API handlers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add start_date, end_date, and interval query parameters to the admin usage endpoint reference in admin-endpoints.mdx. Add admin endpoints to the README API table. Update roadmap to mark admin endpoints, guardrails, and system prompt as implemented. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.local.json:
- Around line 20-26: The always-allow list currently contains write-capable Bash
patterns ("Bash(make build:*)", "Bash(git add:*)", "Bash(git commit:*)") which
is overly permissive; update the settings so these three entries are removed
from the allow array and added to the ask array instead (keep other entries like
"mcp__linear-server__get_issue" unchanged) to enforce least-privilege and
require explicit approval when build or git write actions are requested.
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 538-540: The selector
.dp-day:hover:not(.disabled):not(.other-month) uses chained :not() causing
stylelint selector-not-notation failures; replace the chained :not() with a
single Level‑4 list form by updating the selector to
.dp-day:hover:not(.disabled, .other-month) and keep the rule body the same so it
conforms to stylelint-config-standard.
In `@internal/admin/handler_test.go`:
- Around line 21-118: Tests are flaky around UTC midnight because
parseUsageParams calls time.Now().UTC() directly while tests call
time.Now().UTC() separately; change parseUsageParams to call an injectable clock
(create a package-level var nowUTC func() time.Time that defaults to
time.Now().UTC) and update tests (TestParseUsageParams_* in handler_test.go) to
set nowUTC = func() time.Time { return fixedTime } where fixedTime is a stable
time used to compute expectedStart/expectedEnd, and restore the original nowUTC
with defer to avoid global leakage; reference parseUsageParams and newContext to
locate call sites to update.
In `@internal/admin/handler.go`:
- Around line 94-107: The handler currently returns a dynamic map for the 500
response; replace it with a strongly-typed response struct (e.g., type
ErrorEnvelope or APIErrorResponse) and return an instance of that struct from
handleError instead of map[string]interface{}; update handleError to construct
and JSON-return the typed envelope (preserving fields "error.type" ->
ErrorEnvelope.Error.Type and "error.message" -> ErrorEnvelope.Error.Message)
while leaving the existing GatewayError branch unchanged and referencing
core.GatewayError/gatewayErr and the handleError function to locate the change.
In `@internal/usage/reader_mongodb.go`:
- Around line 24-74: GetSummary is missing the case where only EndDate is set
and it also doesn't check cursor errors after iteration; add a branch in
GetSummary to handle !endZero && startZero that appends a $match on "timestamp"
with "$lt" set to params.EndDate.AddDate(0,0,1).UTC() (mirroring the inclusive
end logic used when both dates are present), and after reading the cursor (after
the cursor.Next loop/conditional) call cursor.Err() and return an error if
non-nil (update error messages accordingly in the Aggregate/Decode/Err checks)
so callers see cursor iteration errors.
- Around line 90-112: GetDailyUsage currently handles both-dates and start-only
but misses end-only, leaving the query unbounded; add a branch in GetDailyUsage
to append a bson $match on "timestamp" with { "$lt":
params.EndDate.AddDate(0,0,1).UTC() } when params.StartDate.IsZero() is true and
params.EndDate is not, mirroring the behavior used in GetSummary so the pipeline
correctly constrains records to before EndDate+1 day.
In `@internal/usage/reader_sqlite.go`:
- Around line 53-63: The weekly grouping in sqliteGroupExpr currently uses
strftime('%Y-W%W', timestamp) which is non‑ISO; change the "weekly" case in
sqliteGroupExpr to return the ISO week expression strftime('%G-W%V', timestamp)
so SQLite matches PostgreSQL/MongoDB ISO week semantics (note this requires
SQLite ≥3.46.0); if compatibility with older SQLite builds is required,
implement a fallback branch or feature‑gate that falls back to the prior
'%Y-W%W' behavior or converts timestamps to ISO weeks in Go before grouping.
---
Duplicate comments:
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 74-76: The font-family declarations (e.g., the body selector's
font-family rule) use quoted single-word font names like 'Inter', 'Menlo', and
'Consolas' which triggers stylelint's font-family-name-quotes rule; update those
font-family values by removing quotes around single-word font names (keep quotes
for multi-word names such as 'Segoe UI'), and apply the same change to the other
occurrences noted (around lines 755-757) so all font-family declarations follow
the stylelint rule.
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 534-536: The check in formatNumber is redundant: n == null already
covers both null and undefined, so remove the "|| n === undefined" branch;
update the formatNumber function to simply return '-' when n == null and
otherwise return n.toLocaleString(), ensuring only the redundant undefined
comparison is deleted (keep function name formatNumber as the location to edit).
- Around line 342-347: The fetchAll method leaves this.needsAuth set after a 401
so the auth prompt can persist; to fix, clear needsAuth at the start of each
fetch cycle (e.g., inside async fetchAll() before calling
fetchUsage()/fetchModels()) by setting this.needsAuth = false (similar to how
this.authError is reset), so subsequent successful fetches will hide the auth
prompt; ensure any per-fetch methods (fetchUsage, fetchModels) do not
reintroduce stale state without updating needsAuth on new responses.
- Around line 334-340: The headers() helper currently always sets 'Content-Type:
application/json' which causes unnecessary CORS preflights for GET-only
requests; update headers() so it only adds 'Content-Type' when the outgoing
request will have a body (e.g., method is not GET/HEAD or a body is present).
Modify the headers() implementation in the headers() function to conditionally
add 'Content-Type' (and still add 'Authorization' when this.apiKey exists), or
alternatively accept a parameter/flag indicating a body is present and only set
Content-Type in that case.
- Around line 3-65: Add an inline comment next to the initialization of
this.apiKey in the init() method explaining the localStorage trade-off: note
that the API key is persisted via
localStorage.getItem('gomodel_api_key')/setItem for UX (survives reloads),
acknowledge the security risk of storing secrets in localStorage, and state that
this choice is intentional under the current threat model (unauthenticated admin
UI) with a pointer to where to update the design if requirements change; keep
the comment concise and colocated with the this.apiKey assignment in function
init().
In `@internal/admin/dashboard/templates/layout.html`:
- Around line 11-13: The three external script tags that load Chart.js,
Alpine.js and htmx in layout.html lack subresource integrity and crossorigin
settings; update each script element (the Chart.js, Alpine.js and htmx <script>
tags) to include a proper integrity="sha384-..." attribute and
crossorigin="anonymous" using the SRI hashes for the exact CDN files, or
alternatively replace their src values with your locally served static asset
paths (and omit SRI when self-hosting) and ensure the local files are bundled
into your static assets pipeline so the dashboard no longer depends on
unauthenticated CDN resources.
In `@internal/admin/handler.go`:
- Around line 118-123: The call to h.usageReader.GetSummary can return (nil,
nil) which will serialize as JSON null; update the handler around
h.usageReader.GetSummary to detect when summary == nil after the error check and
replace it with an empty value (e.g. an empty struct or map) before returning
c.JSON so the endpoint returns {} instead of null; ensure you keep the existing
error path using handleError and only substitute the empty value when err == nil
and summary == nil.
In `@internal/usage/reader_mongodb.go`:
- Around line 136-161: The function currently declares "var result []DailyUsage"
which serializes to null when empty; change initialization to an empty slice
(e.g., result := []DailyUsage{} or make([]DailyUsage, 0)) before iterating the
cursor so that when no rows are found the method returns an empty JSON array;
update the variable used in the loop that appends DailyUsage (the same "result")
and keep the existing cursor.Decode, cursor.Err check, and return result, nil
behavior.
Comment on lines
+21
to
+118
| func TestParseUsageParams_DaysDefault(t *testing.T) { | ||
| c := newContext("") | ||
| params := parseUsageParams(c) | ||
|
|
||
| if params.Interval != "daily" { | ||
| t.Errorf("expected interval 'daily', got %q", params.Interval) | ||
| } | ||
|
|
||
| today := time.Now().UTC() | ||
| expectedEnd := time.Date(today.Year(), today.Month(), today.Day(), 0, 0, 0, 0, time.UTC) | ||
| expectedStart := expectedEnd.AddDate(0, 0, -29) | ||
|
|
||
| if !params.EndDate.Equal(expectedEnd) { | ||
| t.Errorf("expected end date %v, got %v", expectedEnd, params.EndDate) | ||
| } | ||
| if !params.StartDate.Equal(expectedStart) { | ||
| t.Errorf("expected start date %v, got %v", expectedStart, params.StartDate) | ||
| } | ||
| } | ||
|
|
||
| func TestParseUsageParams_DaysExplicit(t *testing.T) { | ||
| c := newContext("days=7") | ||
| params := parseUsageParams(c) | ||
|
|
||
| today := time.Now().UTC() | ||
| expectedEnd := time.Date(today.Year(), today.Month(), today.Day(), 0, 0, 0, 0, time.UTC) | ||
| expectedStart := expectedEnd.AddDate(0, 0, -6) | ||
|
|
||
| if !params.StartDate.Equal(expectedStart) { | ||
| t.Errorf("expected start date %v, got %v", expectedStart, params.StartDate) | ||
| } | ||
| if !params.EndDate.Equal(expectedEnd) { | ||
| t.Errorf("expected end date %v, got %v", expectedEnd, params.EndDate) | ||
| } | ||
| } | ||
|
|
||
| func TestParseUsageParams_StartAndEndDate(t *testing.T) { | ||
| c := newContext("start_date=2026-01-01&end_date=2026-01-31") | ||
| params := parseUsageParams(c) | ||
|
|
||
| expectedStart := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) | ||
| expectedEnd := time.Date(2026, 1, 31, 0, 0, 0, 0, time.UTC) | ||
|
|
||
| if !params.StartDate.Equal(expectedStart) { | ||
| t.Errorf("expected start date %v, got %v", expectedStart, params.StartDate) | ||
| } | ||
| if !params.EndDate.Equal(expectedEnd) { | ||
| t.Errorf("expected end date %v, got %v", expectedEnd, params.EndDate) | ||
| } | ||
| } | ||
|
|
||
| func TestParseUsageParams_OnlyStartDate(t *testing.T) { | ||
| c := newContext("start_date=2026-01-15") | ||
| params := parseUsageParams(c) | ||
|
|
||
| expectedStart := time.Date(2026, 1, 15, 0, 0, 0, 0, time.UTC) | ||
| today := time.Now().UTC() | ||
| expectedEnd := time.Date(today.Year(), today.Month(), today.Day(), 0, 0, 0, 0, time.UTC) | ||
|
|
||
| if !params.StartDate.Equal(expectedStart) { | ||
| t.Errorf("expected start date %v, got %v", expectedStart, params.StartDate) | ||
| } | ||
| if !params.EndDate.Equal(expectedEnd) { | ||
| t.Errorf("expected end date %v, got %v", expectedEnd, params.EndDate) | ||
| } | ||
| } | ||
|
|
||
| func TestParseUsageParams_OnlyEndDate(t *testing.T) { | ||
| c := newContext("end_date=2026-02-10") | ||
| params := parseUsageParams(c) | ||
|
|
||
| expectedEnd := time.Date(2026, 2, 10, 0, 0, 0, 0, time.UTC) | ||
| expectedStart := expectedEnd.AddDate(0, 0, -29) | ||
|
|
||
| if !params.StartDate.Equal(expectedStart) { | ||
| t.Errorf("expected start date %v, got %v", expectedStart, params.StartDate) | ||
| } | ||
| if !params.EndDate.Equal(expectedEnd) { | ||
| t.Errorf("expected end date %v, got %v", expectedEnd, params.EndDate) | ||
| } | ||
| } | ||
|
|
||
| func TestParseUsageParams_InvalidDates(t *testing.T) { | ||
| c := newContext("start_date=invalid&end_date=also-invalid") | ||
| params := parseUsageParams(c) | ||
|
|
||
| // Should fall back to days=30 default | ||
| today := time.Now().UTC() | ||
| expectedEnd := time.Date(today.Year(), today.Month(), today.Day(), 0, 0, 0, 0, time.UTC) | ||
| expectedStart := expectedEnd.AddDate(0, 0, -29) | ||
|
|
||
| if !params.StartDate.Equal(expectedStart) { | ||
| t.Errorf("expected start date %v, got %v", expectedStart, params.StartDate) | ||
| } | ||
| if !params.EndDate.Equal(expectedEnd) { | ||
| t.Errorf("expected end date %v, got %v", expectedEnd, params.EndDate) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Stabilize time‑dependent expectations to avoid midnight flakes.
parseUsageParams uses time.Now().UTC() internally, while tests compute expectedEnd/expectedStart using a separate time.Now().UTC() call. If the date flips between calls (e.g., around UTC midnight), assertions will fail. Consider injecting a clock (e.g., package‑level nowUTC() used by parseUsageParams) so tests can fix the time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/handler_test.go` around lines 21 - 118, Tests are flaky around
UTC midnight because parseUsageParams calls time.Now().UTC() directly while
tests call time.Now().UTC() separately; change parseUsageParams to call an
injectable clock (create a package-level var nowUTC func() time.Time that
defaults to time.Now().UTC) and update tests (TestParseUsageParams_* in
handler_test.go) to set nowUTC = func() time.Time { return fixedTime } where
fixedTime is a stable time used to compute expectedStart/expectedEnd, and
restore the original nowUTC with defer to avoid global leakage; reference
parseUsageParams and newContext to locate call sites to update.
Comment on lines
+24
to
+74
| func (r *MongoDBReader) GetSummary(ctx context.Context, params UsageQueryParams) (*UsageSummary, error) { | ||
| pipeline := bson.A{} | ||
|
|
||
| startZero := params.StartDate.IsZero() | ||
| endZero := params.EndDate.IsZero() | ||
|
|
||
| if !startZero && !endZero { | ||
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | ||
| {Key: "timestamp", Value: bson.D{ | ||
| {Key: "$gte", Value: params.StartDate.UTC()}, | ||
| {Key: "$lt", Value: params.EndDate.AddDate(0, 0, 1).UTC()}, | ||
| }}, | ||
| }}}) | ||
| } else if !startZero { | ||
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | ||
| {Key: "timestamp", Value: bson.D{{Key: "$gte", Value: params.StartDate.UTC()}}}, | ||
| }}}) | ||
| } | ||
|
|
||
| pipeline = append(pipeline, bson.D{{Key: "$group", Value: bson.D{ | ||
| {Key: "_id", Value: nil}, | ||
| {Key: "total_requests", Value: bson.D{{Key: "$sum", Value: 1}}}, | ||
| {Key: "total_input", Value: bson.D{{Key: "$sum", Value: "$input_tokens"}}}, | ||
| {Key: "total_output", Value: bson.D{{Key: "$sum", Value: "$output_tokens"}}}, | ||
| {Key: "total_tokens", Value: bson.D{{Key: "$sum", Value: "$total_tokens"}}}, | ||
| }}}) | ||
|
|
||
| cursor, err := r.collection.Aggregate(ctx, pipeline) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to aggregate usage summary: %w", err) | ||
| } | ||
| defer cursor.Close(ctx) | ||
|
|
||
| summary := &UsageSummary{} | ||
| if cursor.Next(ctx) { | ||
| var result struct { | ||
| TotalRequests int `bson:"total_requests"` | ||
| TotalInput int64 `bson:"total_input"` | ||
| TotalOutput int64 `bson:"total_output"` | ||
| TotalTokens int64 `bson:"total_tokens"` | ||
| } | ||
| if err := cursor.Decode(&result); err != nil { | ||
| return nil, fmt.Errorf("failed to decode usage summary: %w", err) | ||
| } | ||
| summary.TotalRequests = result.TotalRequests | ||
| summary.TotalInput = result.TotalInput | ||
| summary.TotalOutput = result.TotalOutput | ||
| summary.TotalTokens = result.TotalTokens | ||
| } | ||
|
|
||
| return summary, nil |
Contributor
There was a problem hiding this comment.
Handle EndDate-only queries and cursor errors.
If only EndDate is provided, Line 30–41 currently apply no filter, so results can include dates past the end bound. Also, cursor errors can be missed when cursor.Next(ctx) returns false. Add an end-only match and check cursor.Err() before returning.
🔧 Suggested fix
startZero := params.StartDate.IsZero()
endZero := params.EndDate.IsZero()
if !startZero && !endZero {
pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{
{Key: "timestamp", Value: bson.D{
{Key: "$gte", Value: params.StartDate.UTC()},
{Key: "$lt", Value: params.EndDate.AddDate(0, 0, 1).UTC()},
}},
}}})
} else if !startZero {
pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{
{Key: "timestamp", Value: bson.D{{Key: "$gte", Value: params.StartDate.UTC()}}},
}}})
+ } else if !endZero {
+ pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{
+ {Key: "timestamp", Value: bson.D{{Key: "$lt", Value: params.EndDate.AddDate(0, 0, 1).UTC()}}},
+ }}})
}
@@
if cursor.Next(ctx) {
var result struct {
TotalRequests int `bson:"total_requests"`
TotalInput int64 `bson:"total_input"`
TotalOutput int64 `bson:"total_output"`
TotalTokens int64 `bson:"total_tokens"`
}
if err := cursor.Decode(&result); err != nil {
return nil, fmt.Errorf("failed to decode usage summary: %w", err)
}
summary.TotalRequests = result.TotalRequests
summary.TotalInput = result.TotalInput
summary.TotalOutput = result.TotalOutput
summary.TotalTokens = result.TotalTokens
}
+ if err := cursor.Err(); err != nil {
+ return nil, fmt.Errorf("error iterating usage summary cursor: %w", err)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usage/reader_mongodb.go` around lines 24 - 74, GetSummary is missing
the case where only EndDate is set and it also doesn't check cursor errors after
iteration; add a branch in GetSummary to handle !endZero && startZero that
appends a $match on "timestamp" with "$lt" set to
params.EndDate.AddDate(0,0,1).UTC() (mirroring the inclusive end logic used when
both dates are present), and after reading the cursor (after the cursor.Next
loop/conditional) call cursor.Err() and return an error if non-nil (update error
messages accordingly in the Aggregate/Decode/Err checks) so callers see cursor
iteration errors.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive admin layer to GOModel with both REST API endpoints and an embedded dashboard UI for monitoring usage metrics and registered models. The implementation is split into two independently controllable features via ADMIN_ENDPOINTS_ENABLED and ADMIN_UI_ENABLED flags (both default to true).
Changes:
- Added UsageReader interface with SQLite, PostgreSQL, and MongoDB implementations for querying usage data across different storage backends
- Added admin REST API with three endpoints: usage summary, daily usage breakdown (with configurable intervals), and model listing with provider types
- Added embedded dashboard UI with HTMX + Alpine.js + Chart.js, including a custom date picker, interval selector, and model filtering
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
internal/usage/reader.go |
Defines UsageReader interface and data types (UsageQueryParams, UsageSummary, DailyUsage) |
internal/usage/reader_sqlite.go |
SQLite implementation using strftime for date grouping |
internal/usage/reader_postgresql.go |
PostgreSQL implementation using DATE_TRUNC and to_char for date grouping |
internal/usage/reader_mongodb.go |
MongoDB implementation using aggregation pipelines with $dateToString |
internal/usage/factory.go |
Factory function NewReader() that creates appropriate reader based on storage type |
internal/admin/handler.go |
Admin API handler with UsageSummary, DailyUsage, and ListModels endpoints |
internal/admin/handler_test.go |
Unit tests for parseUsageParams helper function |
internal/admin/dashboard/dashboard.go |
Dashboard handler serving embedded HTML templates and static assets |
internal/admin/dashboard/templates/ |
HTML templates for dashboard UI (layout.html, index.html) |
internal/admin/dashboard/static/ |
Static assets: CSS, JavaScript, and favicon |
internal/server/http.go |
Server integration: adds admin routes and configures auth skip paths for dashboard |
internal/server/auth.go |
Enhanced auth middleware to support prefix-based path matching (paths ending with /*) |
internal/app/app.go |
Admin initialization logic and startup logging |
internal/providers/registry.go |
New ListModelsWithProvider() method returning models with provider type |
config/config.go |
AdminConfig struct with EndpointsEnabled and UIEnabled fields |
docs/advanced/admin-endpoints.mdx |
New documentation page covering admin API, authentication, and endpoints |
docs/advanced/configuration.mdx |
Updated with admin configuration section |
docs/docs.json |
Added admin-endpoints to navigation |
README.md, CLAUDE.md, .env.template |
Updated with admin feature information |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/usage/reader_sqlite.go
Outdated
| func sqliteGroupExpr(interval string) string { | ||
| switch interval { | ||
| case "weekly": | ||
| return `strftime('%Y-W%W', timestamp)` |
There was a problem hiding this comment.
Weekly date format inconsistency across database backends. SQLite uses %W (week number 00-53, Monday as first day), PostgreSQL uses IW (ISO week 01-53), and MongoDB uses %V (ISO week 01-53). SQLite's %W doesn't produce ISO week numbers and may cause data inconsistency. Consider using strftime('%Y-W', date(timestamp, 'weekday 0', '-6 days')) or a similar approach to align with ISO week formatting used in PostgreSQL and MongoDB.
Suggested change
| return `strftime('%Y-W%W', timestamp)` | |
| return `strftime('%Y-W%W', date(timestamp, 'weekday 0', '-6 days'))` |
Comment on lines
+1
to
+110
| package usage | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "fmt" | ||
| ) | ||
|
|
||
| // SQLiteReader implements UsageReader for SQLite databases. | ||
| type SQLiteReader struct { | ||
| db *sql.DB | ||
| } | ||
|
|
||
| // NewSQLiteReader creates a new SQLite usage reader. | ||
| func NewSQLiteReader(db *sql.DB) (*SQLiteReader, error) { | ||
| if db == nil { | ||
| return nil, fmt.Errorf("database connection is required") | ||
| } | ||
| return &SQLiteReader{db: db}, nil | ||
| } | ||
|
|
||
| func (r *SQLiteReader) GetSummary(ctx context.Context, params UsageQueryParams) (*UsageSummary, error) { | ||
| var query string | ||
| var args []interface{} | ||
|
|
||
| startZero := params.StartDate.IsZero() | ||
| endZero := params.EndDate.IsZero() | ||
|
|
||
| if !startZero && !endZero { | ||
| query = `SELECT COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM usage WHERE timestamp >= ? AND timestamp < ?` | ||
| args = append(args, params.StartDate.UTC().Format("2006-01-02"), params.EndDate.AddDate(0, 0, 1).UTC().Format("2006-01-02")) | ||
| } else if !startZero { | ||
| query = `SELECT COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM usage WHERE timestamp >= ?` | ||
| args = append(args, params.StartDate.UTC().Format("2006-01-02")) | ||
| } else { | ||
| query = `SELECT COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM usage` | ||
| } | ||
|
|
||
| summary := &UsageSummary{} | ||
| err := r.db.QueryRowContext(ctx, query, args...).Scan( | ||
| &summary.TotalRequests, &summary.TotalInput, &summary.TotalOutput, &summary.TotalTokens, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query usage summary: %w", err) | ||
| } | ||
|
|
||
| return summary, nil | ||
| } | ||
|
|
||
| func sqliteGroupExpr(interval string) string { | ||
| switch interval { | ||
| case "weekly": | ||
| return `strftime('%Y-W%W', timestamp)` | ||
| case "monthly": | ||
| return `strftime('%Y-%m', timestamp)` | ||
| case "yearly": | ||
| return `strftime('%Y', timestamp)` | ||
| default: | ||
| return `DATE(timestamp)` | ||
| } | ||
| } | ||
|
|
||
| func (r *SQLiteReader) GetDailyUsage(ctx context.Context, params UsageQueryParams) ([]DailyUsage, error) { | ||
| interval := params.Interval | ||
| if interval == "" { | ||
| interval = "daily" | ||
| } | ||
| groupExpr := sqliteGroupExpr(interval) | ||
|
|
||
| var where string | ||
| var args []interface{} | ||
|
|
||
| startZero := params.StartDate.IsZero() | ||
| endZero := params.EndDate.IsZero() | ||
|
|
||
| if !startZero && !endZero { | ||
| where = ` WHERE timestamp >= ? AND timestamp < ?` | ||
| args = append(args, params.StartDate.UTC().Format("2006-01-02"), params.EndDate.AddDate(0, 0, 1).UTC().Format("2006-01-02")) | ||
| } else if !startZero { | ||
| where = ` WHERE timestamp >= ?` | ||
| args = append(args, params.StartDate.UTC().Format("2006-01-02")) | ||
| } | ||
|
|
||
| query := fmt.Sprintf(`SELECT %s as period, COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM usage%s GROUP BY %s ORDER BY period`, groupExpr, where, groupExpr) | ||
|
|
||
| rows, err := r.db.QueryContext(ctx, query, args...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query daily usage: %w", err) | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var result []DailyUsage | ||
| for rows.Next() { | ||
| var d DailyUsage | ||
| if err := rows.Scan(&d.Date, &d.Requests, &d.InputTokens, &d.OutputTokens, &d.TotalTokens); err != nil { | ||
| return nil, fmt.Errorf("failed to scan daily usage row: %w", err) | ||
| } | ||
| result = append(result, d) | ||
| } | ||
|
|
||
| if err := rows.Err(); err != nil { | ||
| return nil, fmt.Errorf("error iterating daily usage rows: %w", err) | ||
| } | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Missing test coverage for SQLiteReader, PostgreSQLReader, and MongoDBReader implementations. The admin handler tests exist (handler_test.go), but the database reader implementations that perform the core query logic have no unit tests. Consider adding tests to verify correct query construction, date filtering, interval grouping, and error handling for each database backend.
Comment on lines
+1
to
+111
| package usage | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/jackc/pgx/v5/pgxpool" | ||
| ) | ||
|
|
||
| // PostgreSQLReader implements UsageReader for PostgreSQL databases. | ||
| type PostgreSQLReader struct { | ||
| pool *pgxpool.Pool | ||
| } | ||
|
|
||
| // NewPostgreSQLReader creates a new PostgreSQL usage reader. | ||
| func NewPostgreSQLReader(pool *pgxpool.Pool) (*PostgreSQLReader, error) { | ||
| if pool == nil { | ||
| return nil, fmt.Errorf("connection pool is required") | ||
| } | ||
| return &PostgreSQLReader{pool: pool}, nil | ||
| } | ||
|
|
||
| func (r *PostgreSQLReader) GetSummary(ctx context.Context, params UsageQueryParams) (*UsageSummary, error) { | ||
| var query string | ||
| var args []interface{} | ||
|
|
||
| startZero := params.StartDate.IsZero() | ||
| endZero := params.EndDate.IsZero() | ||
|
|
||
| if !startZero && !endZero { | ||
| query = `SELECT COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM "usage" WHERE timestamp >= $1 AND timestamp < $2` | ||
| args = append(args, params.StartDate.UTC(), params.EndDate.AddDate(0, 0, 1).UTC()) | ||
| } else if !startZero { | ||
| query = `SELECT COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM "usage" WHERE timestamp >= $1` | ||
| args = append(args, params.StartDate.UTC()) | ||
| } else { | ||
| query = `SELECT COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM "usage"` | ||
| } | ||
|
|
||
| summary := &UsageSummary{} | ||
| err := r.pool.QueryRow(ctx, query, args...).Scan( | ||
| &summary.TotalRequests, &summary.TotalInput, &summary.TotalOutput, &summary.TotalTokens, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query usage summary: %w", err) | ||
| } | ||
|
|
||
| return summary, nil | ||
| } | ||
|
|
||
| func pgGroupExpr(interval string) string { | ||
| switch interval { | ||
| case "weekly": | ||
| return `to_char(DATE_TRUNC('week', timestamp AT TIME ZONE 'UTC'), 'IYYY-"W"IW')` | ||
| case "monthly": | ||
| return `to_char(DATE_TRUNC('month', timestamp AT TIME ZONE 'UTC'), 'YYYY-MM')` | ||
| case "yearly": | ||
| return `to_char(DATE_TRUNC('year', timestamp AT TIME ZONE 'UTC'), 'YYYY')` | ||
| default: | ||
| return `to_char(DATE(timestamp AT TIME ZONE 'UTC'), 'YYYY-MM-DD')` | ||
| } | ||
| } | ||
|
|
||
| func (r *PostgreSQLReader) GetDailyUsage(ctx context.Context, params UsageQueryParams) ([]DailyUsage, error) { | ||
| interval := params.Interval | ||
| if interval == "" { | ||
| interval = "daily" | ||
| } | ||
| groupExpr := pgGroupExpr(interval) | ||
|
|
||
| var where string | ||
| var args []interface{} | ||
|
|
||
| startZero := params.StartDate.IsZero() | ||
| endZero := params.EndDate.IsZero() | ||
|
|
||
| if !startZero && !endZero { | ||
| where = ` WHERE timestamp >= $1 AND timestamp < $2` | ||
| args = append(args, params.StartDate.UTC(), params.EndDate.AddDate(0, 0, 1).UTC()) | ||
| } else if !startZero { | ||
| where = ` WHERE timestamp >= $1` | ||
| args = append(args, params.StartDate.UTC()) | ||
| } | ||
|
|
||
| query := fmt.Sprintf(`SELECT %s as period, COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM "usage"%s GROUP BY %s ORDER BY period`, groupExpr, where, groupExpr) | ||
|
|
||
| rows, err := r.pool.Query(ctx, query, args...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query daily usage: %w", err) | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var result []DailyUsage | ||
| for rows.Next() { | ||
| var d DailyUsage | ||
| if err := rows.Scan(&d.Date, &d.Requests, &d.InputTokens, &d.OutputTokens, &d.TotalTokens); err != nil { | ||
| return nil, fmt.Errorf("failed to scan daily usage row: %w", err) | ||
| } | ||
| result = append(result, d) | ||
| } | ||
|
|
||
| if err := rows.Err(); err != nil { | ||
| return nil, fmt.Errorf("error iterating daily usage rows: %w", err) | ||
| } | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Missing test coverage for PostgreSQLReader. Consider adding tests to verify correct query construction, date filtering, interval grouping (especially the PostgreSQL-specific DATE_TRUNC and to_char functions), and error handling.
Comment on lines
+1
to
+162
| package usage | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "go.mongodb.org/mongo-driver/v2/bson" | ||
| "go.mongodb.org/mongo-driver/v2/mongo" | ||
| ) | ||
|
|
||
| // MongoDBReader implements UsageReader for MongoDB. | ||
| type MongoDBReader struct { | ||
| collection *mongo.Collection | ||
| } | ||
|
|
||
| // NewMongoDBReader creates a new MongoDB usage reader. | ||
| func NewMongoDBReader(database *mongo.Database) (*MongoDBReader, error) { | ||
| if database == nil { | ||
| return nil, fmt.Errorf("database is required") | ||
| } | ||
| return &MongoDBReader{collection: database.Collection("usage")}, nil | ||
| } | ||
|
|
||
| func (r *MongoDBReader) GetSummary(ctx context.Context, params UsageQueryParams) (*UsageSummary, error) { | ||
| pipeline := bson.A{} | ||
|
|
||
| startZero := params.StartDate.IsZero() | ||
| endZero := params.EndDate.IsZero() | ||
|
|
||
| if !startZero && !endZero { | ||
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | ||
| {Key: "timestamp", Value: bson.D{ | ||
| {Key: "$gte", Value: params.StartDate.UTC()}, | ||
| {Key: "$lt", Value: params.EndDate.AddDate(0, 0, 1).UTC()}, | ||
| }}, | ||
| }}}) | ||
| } else if !startZero { | ||
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | ||
| {Key: "timestamp", Value: bson.D{{Key: "$gte", Value: params.StartDate.UTC()}}}, | ||
| }}}) | ||
| } | ||
|
|
||
| pipeline = append(pipeline, bson.D{{Key: "$group", Value: bson.D{ | ||
| {Key: "_id", Value: nil}, | ||
| {Key: "total_requests", Value: bson.D{{Key: "$sum", Value: 1}}}, | ||
| {Key: "total_input", Value: bson.D{{Key: "$sum", Value: "$input_tokens"}}}, | ||
| {Key: "total_output", Value: bson.D{{Key: "$sum", Value: "$output_tokens"}}}, | ||
| {Key: "total_tokens", Value: bson.D{{Key: "$sum", Value: "$total_tokens"}}}, | ||
| }}}) | ||
|
|
||
| cursor, err := r.collection.Aggregate(ctx, pipeline) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to aggregate usage summary: %w", err) | ||
| } | ||
| defer cursor.Close(ctx) | ||
|
|
||
| summary := &UsageSummary{} | ||
| if cursor.Next(ctx) { | ||
| var result struct { | ||
| TotalRequests int `bson:"total_requests"` | ||
| TotalInput int64 `bson:"total_input"` | ||
| TotalOutput int64 `bson:"total_output"` | ||
| TotalTokens int64 `bson:"total_tokens"` | ||
| } | ||
| if err := cursor.Decode(&result); err != nil { | ||
| return nil, fmt.Errorf("failed to decode usage summary: %w", err) | ||
| } | ||
| summary.TotalRequests = result.TotalRequests | ||
| summary.TotalInput = result.TotalInput | ||
| summary.TotalOutput = result.TotalOutput | ||
| summary.TotalTokens = result.TotalTokens | ||
| } | ||
|
|
||
| return summary, nil | ||
| } | ||
|
|
||
| func mongoDateFormat(interval string) string { | ||
| switch interval { | ||
| case "weekly": | ||
| return "%G-W%V" | ||
| case "monthly": | ||
| return "%Y-%m" | ||
| case "yearly": | ||
| return "%Y" | ||
| default: | ||
| return "%Y-%m-%d" | ||
| } | ||
| } | ||
|
|
||
| func (r *MongoDBReader) GetDailyUsage(ctx context.Context, params UsageQueryParams) ([]DailyUsage, error) { | ||
| interval := params.Interval | ||
| if interval == "" { | ||
| interval = "daily" | ||
| } | ||
|
|
||
| pipeline := bson.A{} | ||
|
|
||
| startZero := params.StartDate.IsZero() | ||
| endZero := params.EndDate.IsZero() | ||
|
|
||
| if !startZero && !endZero { | ||
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | ||
| {Key: "timestamp", Value: bson.D{ | ||
| {Key: "$gte", Value: params.StartDate.UTC()}, | ||
| {Key: "$lt", Value: params.EndDate.AddDate(0, 0, 1).UTC()}, | ||
| }}, | ||
| }}}) | ||
| } else if !startZero { | ||
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | ||
| {Key: "timestamp", Value: bson.D{{Key: "$gte", Value: params.StartDate.UTC()}}}, | ||
| }}}) | ||
| } | ||
|
|
||
| dateFormat := mongoDateFormat(interval) | ||
|
|
||
| pipeline = append(pipeline, | ||
| bson.D{{Key: "$group", Value: bson.D{ | ||
| {Key: "_id", Value: bson.D{{Key: "$dateToString", Value: bson.D{ | ||
| {Key: "format", Value: dateFormat}, | ||
| {Key: "date", Value: "$timestamp"}, | ||
| }}}}, | ||
| {Key: "requests", Value: bson.D{{Key: "$sum", Value: 1}}}, | ||
| {Key: "input_tokens", Value: bson.D{{Key: "$sum", Value: "$input_tokens"}}}, | ||
| {Key: "output_tokens", Value: bson.D{{Key: "$sum", Value: "$output_tokens"}}}, | ||
| {Key: "total_tokens", Value: bson.D{{Key: "$sum", Value: "$total_tokens"}}}, | ||
| }}}, | ||
| bson.D{{Key: "$sort", Value: bson.D{{Key: "_id", Value: 1}}}}, | ||
| ) | ||
|
|
||
| cursor, err := r.collection.Aggregate(ctx, pipeline) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to aggregate daily usage: %w", err) | ||
| } | ||
| defer cursor.Close(ctx) | ||
|
|
||
| var result []DailyUsage | ||
| for cursor.Next(ctx) { | ||
| var row struct { | ||
| Date string `bson:"_id"` | ||
| Requests int `bson:"requests"` | ||
| InputTokens int64 `bson:"input_tokens"` | ||
| OutputTokens int64 `bson:"output_tokens"` | ||
| TotalTokens int64 `bson:"total_tokens"` | ||
| } | ||
| if err := cursor.Decode(&row); err != nil { | ||
| return nil, fmt.Errorf("failed to decode daily usage row: %w", err) | ||
| } | ||
| result = append(result, DailyUsage{ | ||
| Date: row.Date, | ||
| Requests: row.Requests, | ||
| InputTokens: row.InputTokens, | ||
| OutputTokens: row.OutputTokens, | ||
| TotalTokens: row.TotalTokens, | ||
| }) | ||
| } | ||
|
|
||
| if err := cursor.Err(); err != nil { | ||
| return nil, fmt.Errorf("error iterating daily usage cursor: %w", err) | ||
| } | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Missing test coverage for MongoDBReader. Consider adding tests to verify correct aggregation pipeline construction, date filtering, interval grouping (especially the MongoDB-specific $dateToString format strings), and error handling.
Comment on lines
+110
to
+158
| // UsageSummary handles GET /admin/api/v1/usage/summary | ||
| func (h *Handler) UsageSummary(c echo.Context) error { | ||
| if h.usageReader == nil { | ||
| return c.JSON(http.StatusOK, usage.UsageSummary{}) | ||
| } | ||
|
|
||
| params := parseUsageParams(c) | ||
|
|
||
| summary, err := h.usageReader.GetSummary(c.Request().Context(), params) | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
|
|
||
| return c.JSON(http.StatusOK, summary) | ||
| } | ||
|
|
||
| // DailyUsage handles GET /admin/api/v1/usage/daily | ||
| func (h *Handler) DailyUsage(c echo.Context) error { | ||
| if h.usageReader == nil { | ||
| return c.JSON(http.StatusOK, []usage.DailyUsage{}) | ||
| } | ||
|
|
||
| params := parseUsageParams(c) | ||
|
|
||
| daily, err := h.usageReader.GetDailyUsage(c.Request().Context(), params) | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
|
|
||
| if daily == nil { | ||
| daily = []usage.DailyUsage{} | ||
| } | ||
|
|
||
| return c.JSON(http.StatusOK, daily) | ||
| } | ||
|
|
||
| // ListModels handles GET /admin/api/v1/models | ||
| func (h *Handler) ListModels(c echo.Context) error { | ||
| if h.registry == nil { | ||
| return c.JSON(http.StatusOK, []providers.ModelWithProvider{}) | ||
| } | ||
|
|
||
| models := h.registry.ListModelsWithProvider() | ||
| if models == nil { | ||
| models = []providers.ModelWithProvider{} | ||
| } | ||
|
|
||
| return c.JSON(http.StatusOK, models) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for admin API handler integration. While parseUsageParams has unit tests, the actual HTTP handler methods (UsageSummary, DailyUsage, ListModels) lack integration tests. Consider adding tests that verify the full request/response cycle, including error cases (nil reader, database errors) and authentication enforcement.
Comment on lines
+41
to
+57
| // Index serves GET /admin/dashboard — the main dashboard page. | ||
| func (h *Handler) Index(c echo.Context) error { | ||
| var buf bytes.Buffer | ||
| if err := h.indexTmpl.ExecuteTemplate(&buf, "layout", nil); err != nil { | ||
| return err | ||
| } | ||
| c.Response().Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| c.Response().WriteHeader(http.StatusOK) | ||
| _, err := buf.WriteTo(c.Response().Writer) | ||
| return err | ||
| } | ||
|
|
||
| // Static serves GET /admin/static/* — embedded CSS/JS assets. | ||
| func (h *Handler) Static(c echo.Context) error { | ||
| h.staticFS.ServeHTTP(c.Response().Writer, c.Request()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing test coverage for dashboard handler. Consider adding tests for the Index and Static handler methods to verify template rendering, error handling, and static file serving.
Comment on lines
+147
to
+160
| // Admin API routes (behind ADMIN_ENDPOINTS_ENABLED flag) | ||
| if cfg != nil && cfg.AdminEndpointsEnabled && cfg.AdminHandler != nil { | ||
| adminAPI := e.Group("/admin/api/v1") | ||
| adminAPI.GET("/usage/summary", cfg.AdminHandler.UsageSummary) | ||
| adminAPI.GET("/usage/daily", cfg.AdminHandler.DailyUsage) | ||
| adminAPI.GET("/models", cfg.AdminHandler.ListModels) | ||
| } | ||
|
|
||
| // Admin dashboard UI routes (behind ADMIN_UI_ENABLED flag) | ||
| if cfg != nil && cfg.AdminUIEnabled && cfg.DashboardHandler != nil { | ||
| e.GET("/admin/dashboard", cfg.DashboardHandler.Index) | ||
| e.GET("/admin/dashboard/*", cfg.DashboardHandler.Index) | ||
| e.GET("/admin/static/*", cfg.DashboardHandler.Static) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for server integration with admin routes. Consider adding tests to verify that admin API routes are correctly registered when AdminEndpointsEnabled is true, and that they are not registered when the flag is false. Similarly, test that dashboard routes are only registered when AdminUIEnabled is true.
… at current month Add a prev-month button (hidden on desktop) to the right calendar so mobile users can navigate to past months. Disable the next-month button when already at the current month. Make the date picker full-width on mobile to eliminate the gap on the left side. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add unit, E2E, and integration tests covering admin handler methods, dashboard UI rendering, feature flag routing, auth skip paths, and ListModelsWithProvider across all test tiers. - internal/admin/handler_test.go: mockUsageReader + 13 handler tests - internal/admin/dashboard/dashboard_test.go: 6 tests for embedded UI - internal/server/http_test.go: 7 feature flag routing tests - internal/server/auth_test.go: wildcard skip path matching (6 cases) - internal/providers/registry_test.go: 3 ListModelsWithProvider tests - tests/e2e/admin_test.go: 8 E2E tests with httptest server - tests/integration/admin_test.go: 5 integration tests (PostgreSQL/MongoDB) - tests/integration/setup_test.go: wire admin config into test fixture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+87
to
+88
| query := fmt.Sprintf(`SELECT %s as period, COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM usage%s GROUP BY %s ORDER BY period`, groupExpr, where, groupExpr) |
There was a problem hiding this comment.
The groupExpr variable is inserted into the SQL query using fmt.Sprintf, which could be vulnerable to SQL injection if the interval validation is ever removed or bypassed. While the current code validates the interval parameter against a whitelist before calling this function, this creates a maintenance hazard.
Consider refactoring to use a map that returns the complete query fragment or use prepared statement parameters for better safety. For example:
func (r *SQLiteReader) GetDailyUsage(ctx context.Context, params UsageQueryParams) ([]DailyUsage, error) {
interval := params.Interval
if interval == "" {
interval = "daily"
}
// Validate interval
if !isValidInterval(interval) {
return nil, fmt.Errorf("invalid interval: %s", interval)
}
groupExpr := sqliteGroupExpr(interval)
// ... rest of the code
}Add explicit validation here as a defense-in-depth measure, even though validation happens in the handler layer.
Comment on lines
+88
to
+89
| query := fmt.Sprintf(`SELECT %s as period, COUNT(*), COALESCE(SUM(input_tokens), 0), COALESCE(SUM(output_tokens), 0), COALESCE(SUM(total_tokens), 0) | ||
| FROM "usage"%s GROUP BY %s ORDER BY period`, groupExpr, where, groupExpr) |
There was a problem hiding this comment.
The groupExpr variable is inserted into the SQL query using fmt.Sprintf, which could be vulnerable to SQL injection if the interval validation is ever removed or bypassed. While the current code validates the interval parameter against a whitelist before calling this function, this creates a maintenance hazard.
Consider adding explicit validation in this function as a defense-in-depth measure, similar to the SQLite reader comment.
MongoDB reader was missing the end-date-only filter branch in GetDailyUsage. SQLite reader was using strftime %W (week starting Monday, Jan 1-based) instead of ISO 8601 %G-W%V for weekly grouping. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The needsAuth flag was set on 401 but never cleared, preventing the UI from recovering after successful re-authentication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/app/app.go`:
- Around line 311-322: logStartupInfo currently reads cfg.Admin.UIEnabled and
may log "admin UI enabled" even if initAdmin failed; update logging to reflect
actual runtime state by (a) propagating the real enablement flag from initAdmin
(e.g., return or set an adminEnabled bool from initAdmin) and pass that to
logStartupInfo instead of cfg.Admin.UIEnabled, or (b) change the message to
indicate configuration intent (e.g., "admin UI configured") to avoid implying
the UI is functional; adjust references in initAdmin, logStartupInfo, and the
startup logging block to use the chosen approach.
In `@internal/usage/reader_postgresql.go`:
- Around line 97-104: The current loop populates a nil slice result which
JSON-encodes to null when no rows are returned; change the initialization of
result in reader_postgresql.go to an empty slice (e.g., make([]DailyUsage, 0))
and ensure the function returns that empty slice (result) and nil error after
iterating rows.Next() so consumers receive [] instead of null; reference the
DailyUsage type, the result variable, and the rows.Scan() loop to locate where
to change the initialization and return.
In `@tests/integration/admin_test.go`:
- Around line 118-135: TestAdminUsageSummary_MongoDB is missing a usage-clear
step which makes the test order-dependent; after SetupTestServer(...) and before
sending the two chat requests call the same clear helper used by Postgres tests
(e.g. dbassert.ClearUsage or the project’s ClearUsage helper) against the test
DB/fixture to purge prior usage records so TotalRequests starts at 0; locate the
fixture returned by SetupTestServer and invoke the clear function (matching the
signature used in Postgres tests) before newChatRequest/sendChatRequest are
called.
- Around line 39-40: Replace the fixed time.Sleep(2 * time.Second) calls in the
admin tests with polling using require.Eventually (or a small helper like
waitForUsageFlush) so the test actively checks the admin usage endpoint until
the buffer flushes instead of sleeping a fixed duration; locate each occurrence
of time.Sleep(2 * time.Second) in the test functions and change them to
repeatedly call the admin usage/status endpoint (or a new flush helper) inside
require.Eventually with a short poll interval and a sensible timeout to make the
test resilient to slower CI or config changes.
---
Duplicate comments:
In @.claude/settings.local.json:
- Around line 20-26: The listed Bash permissions "Bash(make build:*)", "Bash(git
add:*)", and "Bash(git commit:*)" are currently in the always-allow list; move
those three entries out of the allow/always-allow array and add them to the
"ask" array so these write/commit actions require explicit confirmation, leaving
the rest of the arrays unchanged; update the arrays that contain
"mcp__linear-server__get_issue" accordingly to remove only those three Bash
entries.
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 74-76: The font-family declarations use quoted single-word font
names (e.g., 'Inter') which stylelint flags; update the body rule's font-family
and any other font-family declarations (including the occurrences referenced
later) to remove quotes around single-word font names so they read Inter, Menlo,
Consolas, etc., while keeping multi-word names quoted and preserving the rest of
the font stack and fallbacks.
- Around line 548-549: The selector
.dp-day:hover:not(.disabled):not(.other-month) should use Level‑4 selector‑list
notation for :not(); replace the two :not() calls with a single :not(.disabled,
.other-month) so the rule becomes .dp-day:hover:not(.disabled, .other-month) {
background: var(--bg-surface-hover); }.
In `@internal/admin/dashboard/templates/layout.html`:
- Around line 8-13: This page loads third‑party fonts and scripts (Chart.js,
Alpine.js, htmx) without integrity checks; add Subresource Integrity hashes or
self‑host the assets. For each external resource referenced in
internal/admin/dashboard/templates/layout.html (the <link> to
fonts.googleapis.com and the <script> tags for chart.umd.min.js, alpinejs, and
htmx), compute the sha384 SRI hash (or use the provided verification script),
then add integrity="sha384-..." and crossorigin="anonymous" attributes to the
corresponding tags (or replace the CDN URLs with local copies and remove the
external attributes). Ensure each resource tag includes both integrity and
crossorigin="anonymous" so the browser enforces the SRI check.
In `@internal/admin/handler_test.go`:
- Around line 424-519: Tests call parseUsageParams and compute expected dates
with time.Now().UTC(), causing flakes when time crosses UTC midnight; make time
deterministic by introducing a package-level nowUTC() function or variable used
by parseUsageParams (and tests) instead of direct time.Now().UTC(), then update
tests (TestParseUsageParams_*) to set or mock nowUTC to a fixed time before
calling parseUsageParams (or pass a clock into parseUsageParams/newContext) so
expectedStart/expectedEnd are computed against the same frozen time.
In `@internal/admin/handler.go`:
- Around line 94-107: The 500 response in handleError uses a dynamic map;
replace it with a typed response struct (e.g., define an InternalErrorResponse
or ErrorEnvelope type with fields Type and Message and a wrapper if needed) and
return that struct from handleError instead of map[string]interface{} so the
JSON encoder emits a typed payload; update the return in handleError to
c.JSON(http.StatusInternalServerError, <yourTypedStructInstance>) and ensure the
struct names and fields (used by handleError) are exported and documented.
- Around line 118-123: GetSummary can return (nil, nil) which leads to c.JSON
returning JSON null; guard the summary before responding. After calling
h.usageReader.GetSummary in the handler, check if summary == nil and replace it
with an empty object/value of the expected response type (e.g., an empty map or
struct) so c.JSON(http.StatusOK, summary) always serializes to {} instead of
null; keep existing error handling via handleError unchanged.
In `@internal/usage/reader_mongodb.go`:
- Around line 24-41: GetSummary is missing the branch that applies a date
upper-bound when only params.EndDate is provided, so add an else if checking
!params.EndDate.IsZero() (or !endZero) and append a pipeline $match that
restricts "timestamp" to be less than params.EndDate.AddDate(0,0,1).UTC() (same
semantics as GetDailyUsage) so results honor the end-only filter; update the
pipeline-building logic in GetSummary to mirror the existing StartDate-only and
Start+End branches.
- Around line 58-74: GetSummary is missing a cursor.Err() check after iterating
the aggregation cursor; after the cursor.Next(ctx) block returns (or if it never
entered) call cursor.Err() and if non-nil return that wrapped as an error
(similar to GetDailyUsage's cursor error handling), and ensure cursor.Close(ctx)
is also deferred/handled as appropriate to mirror GetDailyUsage behavior so
iteration errors are surfaced correctly.
- Line 140: The slice variable result (type []DailyUsage) is left nil which
causes JSON to serialize as null; change its initialization to an empty slice
(e.g., assign result to an empty []DailyUsage{}) where it's declared/returned so
responses produce [] instead of null; update the declaration in
internal/usage/reader_mongodb.go (the result variable used for DailyUsage
aggregation) to initialize to an empty slice before it's populated or returned.
In `@internal/usage/reader_sqlite.go`:
- Around line 96-103: The function builds result from SQL rows but initializes
it as a nil slice so when no rows are found it JSON-encodes to null; change the
initialization of result from "var result []DailyUsage" to an empty slice (e.g.
result := make([]DailyUsage, 0)) or ensure you return an empty slice before
returning (convert nil to []DailyUsage{}), updating the code around the rows
loop and the return site that uses result to guarantee callers receive []
instead of null.
internal/app/app.go
Outdated
Comment on lines
+311
to
+322
|
|
||
| // Admin configuration | ||
| if cfg.Admin.EndpointsEnabled { | ||
| slog.Info("admin API enabled", "api", "/admin/api/v1") | ||
| } else { | ||
| slog.Info("admin API disabled") | ||
| } | ||
| if cfg.Admin.UIEnabled && cfg.Admin.EndpointsEnabled { | ||
| slog.Info("admin UI enabled", "url", fmt.Sprintf("http://localhost:%s/admin/dashboard", cfg.Server.Port)) | ||
| } else { | ||
| slog.Info("admin UI disabled") | ||
| } |
Contributor
There was a problem hiding this comment.
Startup log may report incorrect admin UI status.
logStartupInfo() checks cfg.Admin.UIEnabled directly from config, but if initAdmin fails at line 149, the server continues without admin functionality. The log at line 318-319 would then report "admin UI enabled" even though it's not actually wired.
Consider tracking actual enablement state rather than config intent, or note this is informational logging of configured values.
Suggested clarification
Either rename the log message to reflect configuration intent (e.g., "admin UI configured") or pass actual enablement state to logStartupInfo. Given the warning at line 150 already alerts to failures, this is low priority.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/app/app.go` around lines 311 - 322, logStartupInfo currently reads
cfg.Admin.UIEnabled and may log "admin UI enabled" even if initAdmin failed;
update logging to reflect actual runtime state by (a) propagating the real
enablement flag from initAdmin (e.g., return or set an adminEnabled bool from
initAdmin) and pass that to logStartupInfo instead of cfg.Admin.UIEnabled, or
(b) change the message to indicate configuration intent (e.g., "admin UI
configured") to avoid implying the UI is functional; adjust references in
initAdmin, logStartupInfo, and the startup logging block to use the chosen
approach.
internal/usage/reader_postgresql.go
Outdated
Comment on lines
+97
to
+104
| var result []DailyUsage | ||
| for rows.Next() { | ||
| var d DailyUsage | ||
| if err := rows.Scan(&d.Date, &d.Requests, &d.InputTokens, &d.OutputTokens, &d.TotalTokens); err != nil { | ||
| return nil, fmt.Errorf("failed to scan daily usage row: %w", err) | ||
| } | ||
| result = append(result, d) | ||
| } |
Contributor
There was a problem hiding this comment.
Return [] instead of null when there are no rows.
A nil slice JSON-encodes to null; returning an empty slice keeps API responses consistent for consumers.
🧹 Suggested fix
- var result []DailyUsage
+ result := make([]DailyUsage, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usage/reader_postgresql.go` around lines 97 - 104, The current loop
populates a nil slice result which JSON-encodes to null when no rows are
returned; change the initialization of result in reader_postgresql.go to an
empty slice (e.g., make([]DailyUsage, 0)) and ensure the function returns that
empty slice (result) and nil error after iterating rows.Next() so consumers
receive [] instead of null; reference the DailyUsage type, the result variable,
and the rows.Scan() loop to locate where to change the initialization and
return.
Comment on lines
+39
to
+40
| // Wait for usage buffer to flush (flush interval is 1s in tests) | ||
| time.Sleep(2 * time.Second) |
Contributor
There was a problem hiding this comment.
Avoid fixed sleeps for usage flush; poll instead.
Hard-coded time.Sleep can be flaky on slower CI or if the flush interval changes. Prefer polling the admin endpoints with require.Eventually (or an explicit flush helper) so tests wait only as long as needed.
Also applies to: 82-83, 134-135, 172-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/admin_test.go` around lines 39 - 40, Replace the fixed
time.Sleep(2 * time.Second) calls in the admin tests with polling using
require.Eventually (or a small helper like waitForUsageFlush) so the test
actively checks the admin usage endpoint until the buffer flushes instead of
sleeping a fixed duration; locate each occurrence of time.Sleep(2 * time.Second)
in the test functions and change them to repeatedly call the admin usage/status
endpoint (or a new flush helper) inside require.Eventually with a short poll
interval and a sensible timeout to make the test resilient to slower CI or
config changes.
Comment on lines
+118
to
+135
| func TestAdminUsageSummary_MongoDB(t *testing.T) { | ||
| fixture := SetupTestServer(t, TestServerConfig{ | ||
| DBType: "mongodb", | ||
| UsageEnabled: true, | ||
| AdminEndpointsEnabled: true, | ||
| OnlyModelInteractions: false, | ||
| }) | ||
|
|
||
| // Send 2 chat requests | ||
| for i := 0; i < 2; i++ { | ||
| payload := newChatRequest("gpt-4", "Hello!") | ||
| resp := sendChatRequest(t, fixture.ServerURL, payload) | ||
| require.Equal(t, 200, resp.StatusCode) | ||
| closeBody(resp) | ||
| } | ||
|
|
||
| // Wait for usage buffer to flush | ||
| time.Sleep(2 * time.Second) |
Contributor
There was a problem hiding this comment.
Clear Mongo usage data before asserting counts.
PostgreSQL tests call dbassert.ClearUsage, but the MongoDB test doesn’t. If the Mongo DB is reused across tests, TotalRequests can exceed the expected 2 and make the test order-dependent. Consider clearing the usage collection (or documenting that each test gets a fresh DB).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/admin_test.go` around lines 118 - 135,
TestAdminUsageSummary_MongoDB is missing a usage-clear step which makes the test
order-dependent; after SetupTestServer(...) and before sending the two chat
requests call the same clear helper used by Postgres tests (e.g.
dbassert.ClearUsage or the project’s ClearUsage helper) against the test
DB/fixture to purge prior usage records so TotalRequests starts at 0; locate the
fixture returned by SetupTestServer and invoke the clear function (matching the
signature used in Postgres tests) before newChatRequest/sendChatRequest are
called.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+112
to
+115
| } else if !endZero { | ||
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | ||
| {Key: "timestamp", Value: bson.D{{Key: "$lt", Value: params.EndDate.AddDate(0, 0, 1).UTC()}}}, | ||
| }}}) |
There was a problem hiding this comment.
The MongoDB implementation includes an extra conditional branch for !endZero alone (lines 112-116) that is not present in the SQLite and PostgreSQL implementations. This creates an inconsistency where MongoDB supports querying with only an end date, while the other implementations do not. For consistency, either add this branch to SQLite/PostgreSQL readers or remove it from MongoDB.
Suggested change
| } else if !endZero { | |
| pipeline = append(pipeline, bson.D{{Key: "$match", Value: bson.D{ | |
| {Key: "timestamp", Value: bson.D{{Key: "$lt", Value: params.EndDate.AddDate(0, 0, 1).UTC()}}}, | |
| }}}) |
Comment on lines
+65
to
+80
| if startParsed || endParsed { | ||
| // Fill in missing side | ||
| if !startParsed { | ||
| params.StartDate = params.EndDate.AddDate(0, 0, -29) | ||
| } | ||
| if !endParsed { | ||
| params.EndDate = today | ||
| } | ||
| } else { | ||
| // Fall back to days param | ||
| days := 30 | ||
| if d := c.QueryParam("days"); d != "" { | ||
| if parsed, err := strconv.Atoi(d); err == nil && parsed > 0 { | ||
| days = parsed | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent default calculation: When only end_date is provided, the start date is calculated as EndDate.AddDate(0, 0, -29) (30 days), but when using the days parameter, the calculation is today.AddDate(0, 0, -(days - 1)). This means days=30 produces a 30-day window, but end_date alone produces a 30-day window too. However, line 82 uses -(days - 1) which makes it inconsistent. For days=7, you get 7 days (0-6 days back), but for end_date only, you always get 30 days. The comment on line 67 says "-29" which implies the same logic as line 82, but they're actually different. Consider using -(days - 1) consistently on line 68 as well.
Suggested change
| if startParsed || endParsed { | |
| // Fill in missing side | |
| if !startParsed { | |
| params.StartDate = params.EndDate.AddDate(0, 0, -29) | |
| } | |
| if !endParsed { | |
| params.EndDate = today | |
| } | |
| } else { | |
| // Fall back to days param | |
| days := 30 | |
| if d := c.QueryParam("days"); d != "" { | |
| if parsed, err := strconv.Atoi(d); err == nil && parsed > 0 { | |
| days = parsed | |
| } | |
| } | |
| // Determine window length in days (default: 30) | |
| days := 30 | |
| if d := c.QueryParam("days"); d != "" { | |
| if parsed, err := strconv.Atoi(d); err == nil && parsed > 0 { | |
| days = parsed | |
| } | |
| } | |
| if startParsed || endParsed { | |
| // Fill in missing side | |
| if !startParsed { | |
| params.StartDate = params.EndDate.AddDate(0, 0, -(days - 1)) | |
| } | |
| if !endParsed { | |
| params.EndDate = today | |
| } | |
| } else { | |
| // Fall back to days param |
|
|
||
| async fetchUsage() { | ||
| try { | ||
| var queryStr; |
There was a problem hiding this comment.
Use let or const instead of var for block-scoped variable declaration. Modern JavaScript best practices recommend avoiding var in favor of let or const for better scoping behavior and to prevent potential hoisting issues.
Suggested change
| var queryStr; | |
| let queryStr; |
|
|
||
| // Admin dashboard UI routes (behind ADMIN_UI_ENABLED flag) | ||
| if cfg != nil && cfg.AdminUIEnabled && cfg.DashboardHandler != nil { | ||
| e.GET("/admin/dashboard", cfg.DashboardHandler.Index) |
There was a problem hiding this comment.
Route conflict risk: Both /admin/dashboard and /admin/dashboard/* are registered as separate routes (lines 157-158). The wildcard route on line 158 may never be reached if Echo's router matches /admin/dashboard first. Consider consolidating into a single route handler or ensuring the wildcard route is registered before the exact match. Test that nested paths like /admin/dashboard/overview work correctly.
Suggested change
| e.GET("/admin/dashboard", cfg.DashboardHandler.Index) | |
| e.GET("/admin/dashboard", func(c echo.Context) error { | |
| return c.Redirect(http.StatusMovedPermanently, "/admin/dashboard/") | |
| }) |
Comment on lines
+57
to
+73
| summary := &UsageSummary{} | ||
| if cursor.Next(ctx) { | ||
| var result struct { | ||
| TotalRequests int `bson:"total_requests"` | ||
| TotalInput int64 `bson:"total_input"` | ||
| TotalOutput int64 `bson:"total_output"` | ||
| TotalTokens int64 `bson:"total_tokens"` | ||
| } | ||
| if err := cursor.Decode(&result); err != nil { | ||
| return nil, fmt.Errorf("failed to decode usage summary: %w", err) | ||
| } | ||
| summary.TotalRequests = result.TotalRequests | ||
| summary.TotalInput = result.TotalInput | ||
| summary.TotalOutput = result.TotalOutput | ||
| summary.TotalTokens = result.TotalTokens | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing NULL safety check: In the MongoDB implementation, line 58 checks if cursor.Next(ctx) but doesn't handle the case where the cursor is empty (no matching documents). If there are no usage records in the time range, the function returns a zero-initialized summary without confirming whether this is due to no data or an error. This is acceptable behavior but differs slightly from the SQL implementations. Consider adding a comment explaining this is intentional.
Comment on lines
+65
to
+71
| | Parameter | Type | Description | Default | | ||
| | ------------ | ------ | -------------------------------------------------------- | -------------------- | | ||
| | `start_date` | string | Range start in `YYYY-MM-DD` format | 29 days before end | | ||
| | `end_date` | string | Range end in `YYYY-MM-DD` format | Today | | ||
| | `days` | int | Shorthand for look-back window (ignored if dates are set) | `30` | | ||
|
|
||
| Use `start_date`/`end_date` for explicit ranges or `days` as a shorthand. When both are provided, `start_date`/`end_date` take priority. |
There was a problem hiding this comment.
Documentation inaccuracy: The documentation states that the default for start_date is "29 days before end" (line 67), but the actual implementation in handler.go line 68 uses AddDate(0, 0, -29) which creates a 30-day window (29 days ago + today = 30 days). Similarly, line 69 says the default for days is 30, but line 82 in handler.go uses AddDate(0, 0, -(days - 1)) which for days=30 gives 30 days (0-29 days back). The documentation should clarify that the range is inclusive of both start and end dates.
Suggested change
| | Parameter | Type | Description | Default | | |
| | ------------ | ------ | -------------------------------------------------------- | -------------------- | | |
| | `start_date` | string | Range start in `YYYY-MM-DD` format | 29 days before end | | |
| | `end_date` | string | Range end in `YYYY-MM-DD` format | Today | | |
| | `days` | int | Shorthand for look-back window (ignored if dates are set) | `30` | | |
| Use `start_date`/`end_date` for explicit ranges or `days` as a shorthand. When both are provided, `start_date`/`end_date` take priority. | |
| | Parameter | Type | Description | Default | | |
| | ------------ | ------ | -------------------------------------------------------- | -------------------------------------------- | | |
| | `start_date` | string | Range start in `YYYY-MM-DD` format | 29 days before end (30-day inclusive window) | | |
| | `end_date` | string | Range end in `YYYY-MM-DD` format | Today | | |
| | `days` | int | Shorthand for look-back window (ignored if dates are set) | `30` (inclusive of today; 0–29 days back) | | |
| Use `start_date`/`end_date` for explicit ranges or `days` as a shorthand. When both are provided, `start_date`/`end_date` take priority. Ranges are inclusive of both start and end dates; for example, a 30-day window includes today plus the previous 29 days. |
Comment on lines
+11
to
+13
| <script src="https://cdn.jsdelivr.net/npm/chart.js@4.5.1/dist/chart.umd.min.js"></script> | ||
| <script defer src="https://cdn.jsdelivr.net/npm/alpinejs@3.15.8/dist/cdn.min.js"></script> | ||
| <script src="https://unpkg.com/htmx.org@2.0.8/dist/htmx.min.js"></script> |
There was a problem hiding this comment.
External CDN dependencies: The dashboard loads third-party JavaScript libraries (Chart.js, Alpine.js, HTMX) from public CDNs (jsdelivr, unpkg) without Subresource Integrity (SRI) hashes. This poses a security risk if the CDN is compromised. Consider either: (1) Adding SRI hashes to verify the integrity of loaded scripts, or (2) Vendoring these dependencies and serving them from /admin/static/ to eliminate external dependencies entirely.
Suggested change
| <script src="https://cdn.jsdelivr.net/npm/chart.js@4.5.1/dist/chart.umd.min.js"></script> | |
| <script defer src="https://cdn.jsdelivr.net/npm/alpinejs@3.15.8/dist/cdn.min.js"></script> | |
| <script src="https://unpkg.com/htmx.org@2.0.8/dist/htmx.min.js"></script> | |
| <script src="/admin/static/js/chart.umd.min.js"></script> | |
| <script defer src="/admin/static/js/alpine.cdn.min.js"></script> | |
| <script src="/admin/static/js/htmx.min.js"></script> |
…ently ignoring Invalid start_date/end_date formats were silently falling through to defaults, making it hard for API consumers to debug malformed requests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ogging - Initialize DailyUsage result slices with make() in all three readers (PostgreSQL, SQLite, MongoDB) so empty results serialize as [] not null - Add missing end-only date filter branch in MongoDB GetSummary to match GetDailyUsage behavior - Add cursor.Err() check in MongoDB GetSummary to surface iteration errors - Move admin status logging from logStartupInfo to after initAdmin succeeds so "admin UI enabled" only logs when init actually worked - Add ClearUsageMongo helper and use it in MongoDB admin test to prevent order-dependent test failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… tab order Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 26, 2026
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
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.
Adds a lightweight admin dashboard UI and standalone REST API for monitoring GOModel usage metrics and registered models. Both are behind the ADMIN_ENDPOINTS_ENABLED and ADMIN_UI_ENABLED feature flags (defaults to true).
Architecture:
New endpoints:
https://claude.ai/code/session_01XAusvt4hnbcb8Z2xebQmkX
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests