Repository navigation
feat(admin): per-API-key usage stats + summary card (closes #380 scope) - #1523
Conversation
cristim
added
triaged
|
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:
📝 WalkthroughWalkthroughAPI key usage counters are stored with fixed-window semantics, recorded asynchronously, aggregated through a new authenticated usage-stats endpoint, and displayed in the API keys UI with summary tiles and per-key request columns. ChangesAPI key usage tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant APIKeysPage
participant UsageStatsAPI
participant AuthService
participant Postgres
Browser->>APIKeysPage: load API keys section
APIKeysPage->>UsageStatsAPI: GET /api/api-keys/usage-stats
UsageStatsAPI->>AuthService: request user usage summary
AuthService->>Postgres: list API keys and counters
Postgres-->>AuthService: API key usage data
AuthService-->>UsageStatsAPI: aggregate totals and top keys
UsageStatsAPI-->>APIKeysPage: usage stats response
APIKeysPage-->>Browser: render summary tiles and activity list
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 45 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/auth/service_apikeys.go (1)
294-310: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not coalesce distinct API requests.
singleflight.DorunsRecordUsageonce for all overlapping validations of the same key, so bursts are counted as one request. Each caller still spawns a goroutine, so this neither bounds goroutines nor preserves usage data.
internal/auth/service_apikeys.go#L294-L310: record every validated request; use a bounded queue/batcher only if it preserves one increment per event.internal/auth/service_apikeys_test.go#L513-L530: add concurrent validations for one key and assert the persisted usage count reflects every successful validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/service_apikeys.go` around lines 294 - 310, The usage-recording flow in internal/auth/service_apikeys.go lines 294-310 must not coalesce overlapping validations through lastUsedSFG.Do; update the RecordUsage path to persist one increment for every successful validation, using a bounded queue or batcher only if it preserves each event. In internal/auth/service_apikeys_test.go lines 513-530, add concurrent validations for the same API key and assert the persisted usage count equals the number of successful validations.internal/auth/service_security_test.go (1)
251-271: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not singleflight per-request accounting.
RecordAPIKeyUsageincrements once, butlastUsedSFG.Docollapses every concurrent validation for a key into that one call. Under load, usage totals undercount requests. Record each event (or batch a delta/event stream) and add a concurrent test that expects all requests to be counted. As per coding guidelines: “Use event sourcing for state changes.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/service_security_test.go` around lines 251 - 271, Update ValidateUserAPIKey and its lastUsedSFG accounting path so concurrent validations do not collapse RecordAPIKeyUsage calls into a single event; preserve one usage increment per successful validation, using an event or delta-based mechanism rather than per-key singleflight. Extend the relevant test around RecordAPIKeyUsage with concurrent validations and assert that every request is counted.Source: Coding guidelines
🧹 Nitpick comments (2)
internal/auth/store_postgres_pgxmock_test.go (1)
536-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the counter and window SQL explicitly. The
.*matches an update that only setslast_used_at, so these tests would pass if all newly added counter logic were removed.
internal/auth/store_postgres_pgxmock_test.go#L536-L538: require the lifetime increment and both rolling-window assignments.internal/auth/store_postgres_pgxmock_test.go#L548-L550: use the same specific expectation for the zero-row path.internal/auth/store_postgres_pgxmock_test.go#L561-L563: use the same specific expectation for the database-error path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/store_postgres_pgxmock_test.go` around lines 536 - 538, The SQL expectations for the update in internal/auth/store_postgres_pgxmock_test.go at lines 536-538, 548-550, and 561-563 must explicitly require the lifetime counter increment and both rolling-window assignments, rather than using a broad .* matcher; apply the same specific expectation to all three paths while preserving their existing result behavior.frontend/src/api/types.ts (1)
529-535: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
APIKeyInfois defined twice with an identical shape.frontend/src/api/types.tsandfrontend/src/types.tseach declare their ownAPIKeyInfointerface with the same fields; this PR had to addrequest_count_total/request_count_24hto both in lockstep, which is the drift risk duplication creates.
frontend/src/api/types.ts#L529-L535: keep this as the canonicalAPIKeyInfodefinition (it's the API-contract source of truth).frontend/src/types.ts#L405-L410: replace this duplicate declaration with a re-export/alias ofapi.APIKeyInfo(similar to how it already importsapi.Permission) instead of maintaining a parallel copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/api/types.ts` around lines 529 - 535, The APIKeyInfo interface in frontend/src/api/types.ts:529-535 remains the canonical API-contract definition and requires no direct change. In frontend/src/types.ts:405-410, remove the duplicate APIKeyInfo declaration and replace it with an alias or re-export of api.APIKeyInfo, following the existing api.Permission import pattern so both consumers share one definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/apikeys.ts`:
- Around line 50-176: Extract the usage-summary bounded context from apikeys.ts
into a dedicated module such as apikeys-usage.ts, including
loadApiKeysUsageStats, renderApiKeysUsageSummary, buildSummaryTile, and
formatCount. Import APIKeysUsageStats and any required API, skeleton, or error
utilities there, then update apikeys.ts to import and use the exported
load/render functions while preserving existing behavior and keeping the file
under 500 lines.
In `@internal/api/openapi.yaml`:
- Around line 2103-2129: Update the usage metric contract around
request_count_24h and APIKeysUsageStats so it no longer claims to represent a
true rolling 24-hour total while the store uses a resettable fixed window;
either implement timestamped or bucketed event aggregation for accurate rolling
totals, or rename and revise the field descriptions to clearly identify the
current fixed-window behavior.
In `@internal/api/router.go`:
- Around line 244-247: Split the oversized files below the repository’s 500-line
limit without changing behavior: move API-key route registration near router
setup and its corresponding wrappers near listAPIKeysUsageStatsHandler into a
focused API-key router module in internal/api/router.go (244-247 and 740-742);
move auth adapter methods into focused adapter files in internal/server/app.go
(1181-1183); extract RI-exchange fakes and related tests from
internal/api/handler_ri_exchange_test.go (1036-1038); and split API-key handler
tests from internal/api/handler_apikeys_test.go (634-720) into focused test
files.
In `@internal/auth/store_postgres.go`:
- Around line 909-920: The API-key usage counter incorrectly uses reset-on-write
tumbling windows instead of an exact trailing 24-hour metric. In
internal/auth/store_postgres.go lines 909-920, replace the counter update with
event or time-bucket persistence and aggregate entries from the trailing 24
hours; add the required storage in
internal/database/postgres/migrations/000094_api_keys_usage_counters.up.sql
lines 8-15. Update the contract in internal/auth/interfaces.go lines 52-55, the
usage type or description in internal/auth/types.go lines 81-84, and
documentation in internal/auth/service_apikeys.go lines 286-292 to reflect the
corrected aggregate and event-sourcing model.
---
Outside diff comments:
In `@internal/auth/service_apikeys.go`:
- Around line 294-310: The usage-recording flow in
internal/auth/service_apikeys.go lines 294-310 must not coalesce overlapping
validations through lastUsedSFG.Do; update the RecordUsage path to persist one
increment for every successful validation, using a bounded queue or batcher only
if it preserves each event. In internal/auth/service_apikeys_test.go lines
513-530, add concurrent validations for the same API key and assert the
persisted usage count equals the number of successful validations.
In `@internal/auth/service_security_test.go`:
- Around line 251-271: Update ValidateUserAPIKey and its lastUsedSFG accounting
path so concurrent validations do not collapse RecordAPIKeyUsage calls into a
single event; preserve one usage increment per successful validation, using an
event or delta-based mechanism rather than per-key singleflight. Extend the
relevant test around RecordAPIKeyUsage with concurrent validations and assert
that every request is counted.
---
Nitpick comments:
In `@frontend/src/api/types.ts`:
- Around line 529-535: The APIKeyInfo interface in
frontend/src/api/types.ts:529-535 remains the canonical API-contract definition
and requires no direct change. In frontend/src/types.ts:405-410, remove the
duplicate APIKeyInfo declaration and replace it with an alias or re-export of
api.APIKeyInfo, following the existing api.Permission import pattern so both
consumers share one definition.
In `@internal/auth/store_postgres_pgxmock_test.go`:
- Around line 536-538: The SQL expectations for the update in
internal/auth/store_postgres_pgxmock_test.go at lines 536-538, 548-550, and
561-563 must explicitly require the lifetime counter increment and both
rolling-window assignments, rather than using a broad .* matcher; apply the same
specific expectation to all three paths while preserving their existing result
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8291d5ba-e006-4409-a5de-bc7c06aeea38
📒 Files selected for processing (31)
frontend/src/__tests__/api-apikeys.test.tsfrontend/src/__tests__/apikeys.test.tsfrontend/src/api/apikeys.tsfrontend/src/api/index.tsfrontend/src/api/types.tsfrontend/src/apikeys.tsfrontend/src/index.htmlfrontend/src/styles/components.cssfrontend/src/types.tsinternal/api/handler_apikeys.gointernal/api/handler_apikeys_test.gointernal/api/handler_ri_exchange_test.gointernal/api/mocks_test.gointernal/api/openapi.yamlinternal/api/router.gointernal/api/types.gointernal/auth/interfaces.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api.gointernal/auth/service_apikeys_api_test.gointernal/auth/service_apikeys_test.gointernal/auth/service_security_test.gointernal/auth/store_postgres.gointernal/auth/store_postgres_pgxmock_test.gointernal/auth/test_helpers.gointernal/auth/types.gointernal/database/postgres/migrations/000094_api_keys_usage_counters.down.sqlinternal/database/postgres/migrations/000094_api_keys_usage_counters.up.sqlinternal/mocks/stores.gointernal/server/app.gointernal/server/health_test.go
…oversized files, fix migration gap CodeRabbit findings on PR #1523: - request_count_24h was a fixed/tumbling window counter, not a true rolling 24h total (a request just before a window reset was silently dropped from the count). Renamed to request_count_window across the DB column, Go types, OpenAPI schema, and frontend, and exposed the new request_count_window_start field so API consumers can see exactly which period the count covers instead of assuming "last 24h". - Split the new usage-stats rendering out of frontend/src/apikeys.ts into frontend/src/apikeys_usage.ts, bringing apikeys.ts back under the 500-line limit. - Split the new usage-stats handler and its router wrapper out of internal/api/handler_apikeys.go / router.go into a focused internal/api/handler_apikeys_usage.go (tests moved alongside into handler_apikeys_usage_test.go). Also fixes the failing Integration Tests job: migration 000094 left a numbering gap after 000092 (000093 was skipped), so TestMigrations_AutoHealDirty's rollback-lands-one-version-below-head assertion failed -- rolling back landed on 92, not 93, since no migration owns that version. Renumbered the migration to 000093 to close the gap.
c5a2b19 to
226d5bc
Compare
|
Pushed fixes for all 4 CodeRabbit findings + the failing Integration Tests / CI Success checks:
Verified: @coderabbitai full review |
|
✏️ Learnings added
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 52 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/apikeys_usage.ts`:
- Around line 20-35: Update loadApiKeysUsageStats to track request generations
(or abort the previous request) so only the latest refresh may render stats or
display the error state. Guard both the successful renderApiKeysUsageSummary
path and the catch handling, including skeleton teardown, against stale
requests.
In `@internal/api/openapi.yaml`:
- Around line 1264-1284: Update the description for getAPIKeysUsageStats so
totals and top-key counts refer to the fixed/tumbling request-count window
rather than “24h” or rolling 24-hour usage. Keep the endpoint’s scope and other
summary details unchanged, matching the terminology used by
APIKeyInfo.request_count_window and APIKeysUsageStats.
In `@internal/auth/store_postgres.go`:
- Around line 892-941: Split the API-key persistence logic from PostgresStore
into a dedicated bounded-context store module, including RecordAPIKeyUsage and
its related API-key methods and symbols, then remove the extracted code from
store_postgres.go. Organize the new module so every affected Go file remains
under 500 lines while preserving the existing PostgresStore behavior and
interfaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19c2c0e9-1de3-4ae0-809b-ac802a8137a8
📒 Files selected for processing (33)
frontend/src/__tests__/api-apikeys.test.tsfrontend/src/__tests__/apikeys.test.tsfrontend/src/api/apikeys.tsfrontend/src/api/index.tsfrontend/src/api/types.tsfrontend/src/apikeys.tsfrontend/src/apikeys_usage.tsfrontend/src/index.htmlfrontend/src/styles/components.cssfrontend/src/types.tsinternal/api/handler_apikeys_test.gointernal/api/handler_apikeys_usage.gointernal/api/handler_apikeys_usage_test.gointernal/api/handler_ri_exchange_test.gointernal/api/mocks_test.gointernal/api/openapi.yamlinternal/api/router.gointernal/api/types.gointernal/auth/interfaces.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api.gointernal/auth/service_apikeys_api_test.gointernal/auth/service_apikeys_test.gointernal/auth/service_security_test.gointernal/auth/store_postgres.gointernal/auth/store_postgres_pgxmock_test.gointernal/auth/test_helpers.gointernal/auth/types.gointernal/database/postgres/migrations/000093_api_keys_usage_counters.down.sqlinternal/database/postgres/migrations/000093_api_keys_usage_counters.up.sqlinternal/mocks/stores.gointernal/server/app.gointernal/server/health_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- internal/server/health_test.go
- internal/server/app.go
- internal/mocks/stores.go
- frontend/src/api/index.ts
- internal/auth/interfaces.go
- frontend/src/styles/components.css
- internal/auth/service_apikeys.go
- frontend/src/api/apikeys.ts
- internal/auth/test_helpers.go
- internal/auth/service_security_test.go
- internal/api/types.go
- frontend/src/tests/api-apikeys.test.ts
- frontend/src/index.html
- internal/auth/service_apikeys_test.go
- frontend/src/tests/apikeys.test.ts
- internal/api/mocks_test.go
Adversarial review pass: 3 confirmed correctness bugs fixed + 2 open CR findings addressedEach fix has a regression test that was verified to fail against the pre-fix code and pass after. Fixed (correctness) - 7b6bcd11. Expired windows were reported as current activity. Pre-fix the regression test observed 2. Expired keys were counted as active. 3. Concurrent requests were silently dropped from the counters. This is also the substance of CR's earlier outside-diff finding on Also in that commit: the 24h window length is now a single Go constant ( Addressed (CR findings from the latest review) - 3449ba8
Reviewed and found sound (no change)
Verification
@coderabbitai review |
|
🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
|
@coderabbitai full review |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 33 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/src/apikeys_usage.ts (1)
58-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication: reuse
teardownSkeletoninstead of manually replicating it.The success path manually does
replaceChildren()+ deletes theskeletonActivedataset flag, duplicating whatteardownSkeleton(already imported and used in the error path at line 40) presumably does. Calling it here keeps both paths consistent and avoids the two implementations drifting ifteardownSkeletongains additional cleanup steps later.♻️ Proposed fix
const container = document.getElementById('apikeys-usage-summary'); if (!container) return; - container.replaceChildren(); - delete container.dataset['skeletonActive']; + teardownSkeleton(container);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/apikeys_usage.ts` around lines 58 - 62, In renderApiKeysUsageSummary, replace the manual container cleanup (replaceChildren and deleting skeletonActive) with the existing teardownSkeleton helper used by the error path. Pass the resolved summary container so both success and error paths share the same cleanup behavior.frontend/src/types.ts (1)
405-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
APIKeyInfoshape now duplicates the usage counters too.These three fields and their semantics are declared identically in
frontend/src/api/types.ts(lines 529-543). Two hand-synced copies of the same wire contract will drift the next time the backend adds a field. Consider having this module re-export theapi/typesdefinition instead of redeclaring it.♻️ Sketch
-export interface APIKeyInfo { - id: string; - ... - request_count_window_start?: string; -} +export type { APIKeyInfo } from './api/types';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/types.ts` around lines 405 - 416, Update the APIKeyInfo type in frontend/src/types.ts to reuse or re-export the canonical definition from api/types.ts instead of redeclaring the usage counter fields. Remove the duplicated APIKeyInfo shape while preserving existing imports and consumers.internal/auth/service_apikeys_api_test.go (1)
498-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
.Maybe()plus a fixed sleep makes this assert nothing about usage recording.With
.Maybe(),AssertExpectationspasses whether or notRecordAPIKeyUsageever fires, so the switch away fromUpdateAPIKeyLastUsedisn't actually covered and the 10ms sleep just slows the suite. If the intent is to cover the async record,assert.Eventuallyon a call counter is deterministic; if not, dropping the sleep is enough.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/service_apikeys_api_test.go` around lines 498 - 506, The test around ValidateUserAPIKeyAPI should deterministically verify that RecordAPIKeyUsage is invoked instead of relying on an optional expectation and fixed sleep. Remove Maybe(), replace the time.Sleep with assert.Eventually polling a call counter or equivalent invocation signal, then retain AssertExpectations to confirm the async usage recording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/apikeys.ts`:
- Around line 451-458: Update the documentation for formatRequestCount to state
that unknown, undefined, non-finite, or negative counts render as “n/a”,
matching the NO_COUNT_DATA value instead of describing an em dash.
In `@internal/auth/service_apikeys_api.go`:
- Around line 207-209: Update the response construction around RequestCountTotal
to obtain RequestCountWindow and RequestCountWindowStart from
effectiveWindowUsage(keyInfo) instead of keyInfo’s persisted fields. Preserve
the existing total-usage handling and ensure both exposed window values come
from the same effective result.
In `@internal/database/postgres/migrations/000094_api_keys_usage_counters.up.sql`:
- Line 18: Update the migration comment referencing RecordAPIKeyUsage to point
readers to store_postgres_apikeys.go instead of store_postgres.go, without
changing the migration behavior.
---
Nitpick comments:
In `@frontend/src/apikeys_usage.ts`:
- Around line 58-62: In renderApiKeysUsageSummary, replace the manual container
cleanup (replaceChildren and deleting skeletonActive) with the existing
teardownSkeleton helper used by the error path. Pass the resolved summary
container so both success and error paths share the same cleanup behavior.
In `@frontend/src/types.ts`:
- Around line 405-416: Update the APIKeyInfo type in frontend/src/types.ts to
reuse or re-export the canonical definition from api/types.ts instead of
redeclaring the usage counter fields. Remove the duplicated APIKeyInfo shape
while preserving existing imports and consumers.
In `@internal/auth/service_apikeys_api_test.go`:
- Around line 498-506: The test around ValidateUserAPIKeyAPI should
deterministically verify that RecordAPIKeyUsage is invoked instead of relying on
an optional expectation and fixed sleep. Remove Maybe(), replace the time.Sleep
with assert.Eventually polling a call counter or equivalent invocation signal,
then retain AssertExpectations to confirm the async usage recording.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89e03288-41ec-42b2-8d3d-86ff70cc72be
📒 Files selected for processing (36)
frontend/src/__tests__/api-apikeys.test.tsfrontend/src/__tests__/apikeys-usage.test.tsfrontend/src/__tests__/apikeys.test.tsfrontend/src/api/apikeys.tsfrontend/src/api/index.tsfrontend/src/api/types.tsfrontend/src/apikeys.tsfrontend/src/apikeys_usage.tsfrontend/src/index.htmlfrontend/src/styles/components.cssfrontend/src/types.tsinternal/api/handler_apikeys_test.gointernal/api/handler_apikeys_usage.gointernal/api/handler_apikeys_usage_test.gointernal/api/handler_ri_exchange_test.gointernal/api/mocks_test.gointernal/api/openapi.yamlinternal/api/router.gointernal/api/types.gointernal/auth/interfaces.gointernal/auth/service.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api.gointernal/auth/service_apikeys_api_test.gointernal/auth/service_apikeys_test.gointernal/auth/service_security_test.gointernal/auth/store_postgres.gointernal/auth/store_postgres_apikeys.gointernal/auth/store_postgres_pgxmock_test.gointernal/auth/test_helpers.gointernal/auth/types.gointernal/database/postgres/migrations/000094_api_keys_usage_counters.down.sqlinternal/database/postgres/migrations/000094_api_keys_usage_counters.up.sqlinternal/mocks/stores.gointernal/server/app.gointernal/server/health_test.go
💤 Files with no reviewable changes (1)
- internal/auth/store_postgres.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
TestMigrations_AutoHealDirty rolls back one migration and asserted the resulting version equals headVersion-1. That encodes an invariant this repository does not hold: migration numbers are not contiguous, because renumbering to dodge collisions with in-flight PRs leaves gaps. The set already skips 61-62, 68-69, 82, 84-85. Those gaps were harmless only because none of them sat immediately below head, so headVersion-1 happened to be a real migration. Adding 000095 on top of main's 000093 puts a gap directly below head for the first time, and golang-migrate's Steps(-1) lands on 93 -- the next version that actually exists -- so the assertion failed with expected 94, actual 93. Derive the expected version from the .up.sql filenames instead. This is the root fix rather than a renumber: the next migration to land above a gap would have hit the same assertion, and renumbering 000095 to 000094 would have collided with the in-flight #1523. Test-only; no migration or production code changes.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/src/index.html (1)
789-791: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the source-module comment.
The comment points to
apikeys.ts:loadApiKeysUsageStats(). That function now lives inapikeys_usage.tsafter the file split. Update the comment to referenceapikeys_usage.tsfor accuracy.📝 Proposed fix
- <!-- Section-level usage summary (issue `#340/`#344 deferred sub-task). - Populated by apikeys.ts:loadApiKeysUsageStats(). --> + <!-- Section-level usage summary (issue `#340/`#344 deferred sub-task). + Populated by apikeys_usage.ts:loadApiKeysUsageStats(). -->🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/index.html` around lines 789 - 791, Update the source-module reference in the comment above the apikeys-usage-summary element to point to apikeys_usage.ts while preserving the existing loadApiKeysUsageStats() function reference and surrounding markup.internal/auth/service_apikeys_test.go (1)
1126-1131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name still refers to
UpdateLastUsed.The comment on line 1126 now names the
RecordUsagegoroutine, but the function is stillTestValidateUserAPIKey_UpdateLastUsedPanicIsRecoveredand the mock now targetsRecordAPIKeyUsage. Rename the test toTestValidateUserAPIKey_RecordUsagePanicIsRecoveredso the name matches the behavior under test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/service_apikeys_test.go` around lines 1126 - 1131, Rename the test function TestValidateUserAPIKey_UpdateLastUsedPanicIsRecovered to TestValidateUserAPIKey_RecordUsagePanicIsRecovered so it matches the RecordUsage goroutine and RecordAPIKeyUsage mock behavior being tested.internal/auth/store_postgres_apikeys.go (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHeader says "pure move" but the file adds new behavior.
RecordAPIKeyUsageat line 209 is new in this PR, not moved. Update the header so future readers do not assume the file is unchanged logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/store_postgres_apikeys.go` around lines 1 - 3, Update the header comment in internal/auth/store_postgres_apikeys.go to remove the “Pure move” claim and accurately mention that RecordAPIKeyUsage adds new behavior alongside the moved API-key queries and scanning logic.internal/auth/service_apikeys.go (1)
326-357: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueLeftover pending usage after the final round waits for the next request.
The loop exits after
maxAPIKeyUsageFlushRoundseven whenpeekPendingUsage(keyID) > 0. The residual count stays in memory only. If the process stops before the key is used again, that count is lost and the lifetime counter undercounts. The comment documents the handoff, but it does not mention the restart-loss case.Consider one final unconditional flush attempt after the loop, or record the residual in a metric so the undercount is observable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/service_apikeys.go` around lines 326 - 357, Update recordUsageAsync so pending usage remaining after maxAPIKeyUsageFlushRounds receives one final unconditional flush attempt before the goroutine exits. Preserve the existing bounded retry behavior and singleflight coordination, and update the adjacent comment to document that residual work is flushed once to avoid restart-related loss.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/service.go`:
- Around line 47-57: Update Service.recordUsageAsync so it performs a final
pending-usage check and drains increments that arrive during the last flush
before returning. Add a bounded handoff or retry within the existing
maxAPIKeyUsageFlushRounds limit, preserving the current atomic pendingUsage
handling and avoiding unbounded waiting.
In `@internal/database/postgres/migrations/000094_api_keys_usage_counters.up.sql`:
- Around line 17-20: Remove the duplicated “next request starts a new window.
See” comment line in the migration header, retaining a single occurrence before
the store_postgres_apikeys.go and RecordAPIKeyUsage references.
---
Nitpick comments:
In `@frontend/src/index.html`:
- Around line 789-791: Update the source-module reference in the comment above
the apikeys-usage-summary element to point to apikeys_usage.ts while preserving
the existing loadApiKeysUsageStats() function reference and surrounding markup.
In `@internal/auth/service_apikeys_test.go`:
- Around line 1126-1131: Rename the test function
TestValidateUserAPIKey_UpdateLastUsedPanicIsRecovered to
TestValidateUserAPIKey_RecordUsagePanicIsRecovered so it matches the RecordUsage
goroutine and RecordAPIKeyUsage mock behavior being tested.
In `@internal/auth/service_apikeys.go`:
- Around line 326-357: Update recordUsageAsync so pending usage remaining after
maxAPIKeyUsageFlushRounds receives one final unconditional flush attempt before
the goroutine exits. Preserve the existing bounded retry behavior and
singleflight coordination, and update the adjacent comment to document that
residual work is flushed once to avoid restart-related loss.
In `@internal/auth/store_postgres_apikeys.go`:
- Around line 1-3: Update the header comment in
internal/auth/store_postgres_apikeys.go to remove the “Pure move” claim and
accurately mention that RecordAPIKeyUsage adds new behavior alongside the moved
API-key queries and scanning logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cad783bd-198b-4e2e-8af0-fcdebe266e46
📒 Files selected for processing (36)
frontend/src/__tests__/api-apikeys.test.tsfrontend/src/__tests__/apikeys-usage.test.tsfrontend/src/__tests__/apikeys.test.tsfrontend/src/api/apikeys.tsfrontend/src/api/index.tsfrontend/src/api/types.tsfrontend/src/apikeys.tsfrontend/src/apikeys_usage.tsfrontend/src/index.htmlfrontend/src/styles/components.cssfrontend/src/types.tsinternal/api/handler_apikeys_test.gointernal/api/handler_apikeys_usage.gointernal/api/handler_apikeys_usage_test.gointernal/api/handler_ri_exchange_test.gointernal/api/mocks_test.gointernal/api/openapi.yamlinternal/api/router.gointernal/api/types.gointernal/auth/interfaces.gointernal/auth/service.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api.gointernal/auth/service_apikeys_api_test.gointernal/auth/service_apikeys_test.gointernal/auth/service_security_test.gointernal/auth/store_postgres.gointernal/auth/store_postgres_apikeys.gointernal/auth/store_postgres_pgxmock_test.gointernal/auth/test_helpers.gointernal/auth/types.gointernal/database/postgres/migrations/000094_api_keys_usage_counters.down.sqlinternal/database/postgres/migrations/000094_api_keys_usage_counters.up.sqlinternal/mocks/stores.gointernal/server/app.gointernal/server/health_test.go
💤 Files with no reviewable changes (1)
- internal/auth/store_postgres.go
* fix(db): widen purchase_history.account_id to VARCHAR(255) purchase_history.account_id has been VARCHAR(20) since 000001, which fits an AWS account ID (12 digits) and nothing else. An Azure subscription ID is a 36-character GUID and a GCP project ID runs to 30 characters, so SavePurchaseHistory's INSERT is rejected with SQLSTATE 22001 after the commitment has already been purchased and billed. The error is not swallowed: savePurchaseHistory returns it and the caller stamps a history_write_failed audit-gap marker on the execution (#621). But the purchase_history row itself is lost, so the billed commitment is invisible in the History view, absent from GetActivePurchaseHistory (and therefore undercounted in analytics), and unseen by the grace-period and suppression logic. The identical widening was already applied to the sibling column: 000067 widened savings_snapshots.account_id for exactly this reason and 000074 repaired it on partially-migrated databases. purchase_history was missed. - up: probe-guarded ALTER so it is correct on a fresh database, an already-deployed one, and on re-run under auto-heal. Resolves the table via 'purchase_history'::regclass so the probe follows search_path exactly as the ALTER does, and re-reads the catalog afterwards so the migration cannot be recorded as applied without having widened the column. - down: narrows only from VARCHAR(255), and refuses with a named error rather than truncating when any account_id exceeds 20 characters. Documents the CUDLY_FORCE_MIGRATION_VERSION=95 recovery for the dirty state a refusal leaves behind. Rows already lost to 22001 were never inserted and cannot be recovered by this migration; affected executions are identifiable via the history_write_failed marker on purchase_executions.error. Regression test replicates the real failing scenario with SavePurchaseHistory's own INSERT column set and a 36-char Azure GUID / 30-char GCP project ID: both are rejected with 22001 at version 92 and round-trip untruncated after 000095. Also covers the lossless rollback path and up-migration idempotency. Closes #1603 * test(db): give each 000095 fixture its own provider shape The shared insert helper hardcoded provider='azure' with Azure's service, region and resource_type, so the GCP case inserted an Azure-shaped row and only the account_id differed. In a test whose entire point is that Azure and GCP identifiers overflow VARCHAR(20), a GCP case carrying 'westeurope'/'Standard_D4s_v3' misrepresents the scenario it claims to cover. Group the identifier with its provider/service/region/resource_type in a commitmentRow fixture and pass those through as bind parameters, so each case inserts a row its own provider would actually produce. Adds an AWS fixture for the lossless-rollback test, which previously used a bare 12-digit literal. Test-only; no migration or production code changes. * test(db): derive the rollback target from disk, not head minus one TestMigrations_AutoHealDirty rolls back one migration and asserted the resulting version equals headVersion-1. That encodes an invariant this repository does not hold: migration numbers are not contiguous, because renumbering to dodge collisions with in-flight PRs leaves gaps. The set already skips 61-62, 68-69, 82, 84-85. Those gaps were harmless only because none of them sat immediately below head, so headVersion-1 happened to be a real migration. Adding 000095 on top of main's 000093 puts a gap directly below head for the first time, and golang-migrate's Steps(-1) lands on 93 -- the next version that actually exists -- so the assertion failed with expected 94, actual 93. Derive the expected version from the .up.sql filenames instead. This is the root fix rather than a renumber: the next migration to land above a gap would have hit the same assertion, and renumbering 000095 to 000094 would have collided with the in-flight #1523. Test-only; no migration or production code changes.
…cope) Revives the deferred "API keys usage stats" sub-task from #340/#344 (prior PR #380 closed stale). Fresh implementation against current main using #380's body as spec. Backend: migration 000094 adds request_count_total/request_count_24h counters to api_keys, plus an atomic RecordAPIKeyUsage store method that increments both alongside last_used_at with a rolling 24h window. New GET /api/api-keys/usage-stats aggregates the calling user's own keys into a section summary (active count, 24h/lifetime totals, top-3 most active). OpenAPI updated. Frontend: Admin -> API Keys now shows a 3-tile summary card (active keys, requests 24h, requests lifetime) + a top-3 most-active list, plus per-row Requests (24h) / Requests (total) columns. Loading skeletons via lib/skeleton; summary errors stay isolated from list errors. Migration numbered 000094 (not #380's stale 000051) since 000093 is already claimed by in-flight PR #1516.
…oversized files, fix migration gap CodeRabbit findings on PR #1523: - request_count_24h was a fixed/tumbling window counter, not a true rolling 24h total (a request just before a window reset was silently dropped from the count). Renamed to request_count_window across the DB column, Go types, OpenAPI schema, and frontend, and exposed the new request_count_window_start field so API consumers can see exactly which period the count covers instead of assuming "last 24h". - Split the new usage-stats rendering out of frontend/src/apikeys.ts into frontend/src/apikeys_usage.ts, bringing apikeys.ts back under the 500-line limit. - Split the new usage-stats handler and its router wrapper out of internal/api/handler_apikeys.go / router.go into a focused internal/api/handler_apikeys_usage.go (tests moved alongside into handler_apikeys_usage_test.go). Also fixes the failing Integration Tests job: migration 000094 left a numbering gap after 000092 (000093 was skipped), so TestMigrations_AutoHealDirty's rollback-lands-one-version-below-head assertion failed -- rolling back landed on 92, not 93, since no migration owns that version. Renumbered the migration to 000093 to close the gap.
Adversarial review follow-up on the per-API-key usage stats added in this PR. Three confirmed correctness bugs, each with a regression test that fails without its fix. 1. Expired windows were reported as current activity. request_count_window is only rewritten by the key's NEXT request, so a key that went idle kept its closed window's count on the row indefinitely. The read path summed and ranked that column verbatim, so the summary card could report thousands of requests "in the window" for keys unused for months, and rank a long-dead key as most active. Window counters now go through effectiveWindowUsage, which reports zero with no window start once the window has closed. 2. Expired keys were counted as active. TotalActive tested only the revocation flag, so a key past its expires_at was summarised as active while the keys table directly below it rendered the same key as "Expired". It now uses validateAPIKeyStatus, the same predicate the authentication path applies. 3. Concurrent requests were silently dropped from the counters. singleflight collapses concurrent flushes for one key into a single DB write, and the flush incremented by a fixed 1, so every request arriving during an in-flight write was lost. The undercount grew with request rate, i.e. was worst on exactly the busy keys the stats exist to surface. Requests are now accumulated in memory and the flush writes the whole delta, with a bounded re-drain so a request is never stranded until the key's next use. singleflight still bounds the write rate to one in-flight DB write per key. The 24h window length is now a single Go constant passed into the SQL via make_interval, so the write path and the read-side expiry check cannot drift. RecordAPIKeyUsage rejects a non-positive delta rather than issuing a no-op UPDATE.
Addresses the two still-open CodeRabbit findings on this PR. store_postgres.go was past the project's 500-line ceiling and this PR pushed it further. Its API-key surface moves to store_postgres_apikeys.go (856 + 301 lines). Pure move: no query, scanning or signature changes. loadApiKeysUsageStats could render a stale result. Concurrent refreshes resolve in arbitrary order, so a slow earlier request could overwrite a newer summary, or paint its error over a summary that had loaded fine. A generation counter makes a load a no-op once a newer one has started, on both the success and the failure path. Adds apikeys-usage.test.ts, which the summary module had been missing entirely: render path, top-keys list, markup-in-key-name staying literal text, count-abbreviation boundaries, the error path, and both ordering guards. The two ordering tests fail against the unguarded loader.
Migration 000093 added request_count_total with DEFAULT 0 and no backfill, so every API key that already existed reads as zero regardless of how much traffic it actually served. The UI presented that as "Requests (total): 0", stating a request volume nobody measured, and the summary card folded those fabricated zeros into an exact-looking lifetime sum. RecordAPIKeyUsage is the only writer of last_used_at on the request path and it always bumps the counter in the same statement, so "used at least once yet carrying a zero lifetime count" identifies exactly the rows that predate the counter. Those now report null, and the table renders "n/a" instead of a number. A key that has genuinely never been used still reports 0. The section summary excludes unknown keys from total_requests_lifetime and sets lifetime_partial, which the card renders as a "+" suffix so the total reads as a lower bound rather than an exact figure. The invariant this relies on is documented on effectiveLifetimeUsage: a new caller of the legacy UpdateAPIKeyLastUsed, which bumps only the timestamp, would break it. It currently has no production callers.
main gained 000093_recommendations_state_owner_id after this branch claimed the number, so the merge ref carried two 000093 migrations and CI's check-migration-conflicts hook failed. The collision is invisible locally because each ref on its own is consistent. Renamed via git mv and updated every reference to the number in the Go doc comments, the OpenAPI descriptions, the frontend type comments and the tests. Verified against the merge ref (origin/main union this branch): no migration number appears twice.
… shipped semantics Addresses the three actionable findings from the CodeRabbit pass on e20ca1e. All three are accuracy defects introduced by earlier correct fixes that did not carry their documentation along. - frontend/src/api/apikeys.ts: the getApiKeysUsageStats doc comment still described top keys as ranked "by 24h activity", contradicting the rename of request_count_24h to request_count_window. The counter is a fixed/tumbling window, not a rolling 24h total, so the comment now names the field and its windowing semantics to match the wording already used on APIKeyInfo and APIKeysUsageStats. - internal/auth/service_apikeys_test.go: raise the async RecordAPIKeyUsage deadline from 500ms to 5s, matching the two sibling tests that wait on the same kind of background write. 500ms is a real flake hazard on a loaded CI runner. The signalling channel and the t.Fatal on timeout are unchanged, so the test still fails if usage recording is dropped entirely. - internal/database/postgres/migrations/000094: the comment claimed all existing rows display "0 requests". The read path no longer does that. effectiveLifetimeUsage treats a zero request_count_total on a key with a non-null last_used_at as unknown rather than zero, surfacing it as lifetime_partial, precisely so a pre-migration key is not reported as having served no traffic. The comment now describes that split and points at the function enforcing it. Comment-only and test-only; no production behaviour changes.
The usage counters added by this PR were incremented inside auth.Service.ValidateUserAPIKey, i.e. once per CREDENTIAL VALIDATION. A single HTTP request validates the same key several times over: validateSecurityContext resolves the principal (1), then every permission check re-validates it (1 more), and the multi-verb gates re-validate once per verb. Reported volume was therefore 2x on a plain gate and 5x on the four-verb planned-purchase gate: 100 requests were reported as 200-400. The multiplier varies per endpoint, so the stored totals cannot be corrected retroactively by dividing them down. The multiplicity is pre-existing and was harmless while the async write was an idempotent last_used_at timestamp. Turning it into an additive counter converted it into a systematic overcount, made worse by the singleflight accumulator, which correctly conserves the whole pending delta and so preserves every surplus booking. ValidateUserAPIKey is now a pure validation primitive with no usage side effect, and usage is booked exactly once in Handler.validateSecurityContext, the one code path guaranteed to run once per inbound request. Principal carries the resolved API key ID so the booking needs no second lookup, and principalFromUserAPIKey now fails closed when validation yields no usable key record rather than authenticating a key it cannot attribute usage to. Regression coverage sits at the HandleRequest layer, where the defect actually lives; a service-level test cannot see it, because it books correctly when called once. internal/server drives a real auth.Service behind a real api.Handler and asserts one store booking per request on both the single-verb and four-verb paths. Both fail on the pre-fix code (2 and 5 bookings respectively). internal/api adds the handler-side half: which principal kinds book, how often, and against which key ID. Also addresses three CodeRabbit findings: - formatRequestCount's doc claimed an em dash; NO_COUNT_DATA renders "n/a". - Migration 000094 pointed at store_postgres.go; RecordAPIKeyUsage lives in store_postgres_apikeys.go. - CreateAPIKeyAPI read the window counters raw while every other exposure site goes through effectiveWindowUsage. It cannot misreport today (keyInfo is built in memory and its counters are necessarily zero), but it now shares the one implementation so the invariant holds structurally.
adf6920 to
18c46f3
Compare
|
Rebased onto
Verified after rebasing: |
Adversarial review record (merged)Independent reviewer, distinct from the author. This PR merged while the review was in flight, so the findings below were re-verified against the merge commit rather than the PR head. The recovered overcount fix — verified, and the numbers are exactly as claimedA commit stranded by a dead session claimed the API-key usage counter was booked once per credential validation rather than once per request. Since this PR ships per-key usage stats to users, shipping it over an inflated counter would have meant shipping confidently wrong numbers. Confirmed by independent trace and measurement. The pre-fix state was rebuilt surgically and driven through the real 2x and 5x pre-fix, exactly 1 after, with validation multiplicity deliberately unchanged — the fix decouples booking from validation rather than removing the re-validation, which is the right scope. A detail worth recording: the shipped test fails fast on the second flush, so its printed "total 2" is a snapshot at detection time, not a final total. Reading its output alone would have confirmed the multi-verb multiplier as 2. The probe that waits for every flush goroutine to settle is what produced the real number. The stranded commit's own narrative was also partly stale and was corrected rather than adopted: a context-based principal cache added since prevents a third booking, so "every permission check re-validates" no longer holds — only No path double-books. Concurrency survives the export. One defect escaped to main — filed as LeanerCloud/cloud-commitments-platform#156
Found by enumerating all 119 routes and cross-checking each; it is the only affected one. Measured three ways including against the merge commit. Disproved the scarier reading: without a credential the route returns This is the under-count direction that the existing tests do cover on the two routes they exercise, which is precisely why this one slipped through. Also confirmed vestigial
|
1 participant
Summary
Revives the deferred "API keys usage stats" sub-task from #340 / #344 (prior PR #380 closed stale — implemented fresh against current
mainusing #380's body as spec).000094(renumbered from feat(api+frontend/admin): surface API key usage stats (closes #344 deferred) #380's stale000051;000093is already claimed by in-flight PR feat(api): owner-token compare-and-clear for collection in-flight marker (closes #261) #1516) addsrequest_count_total/request_count_24h(+ internal window-start bookkeeping) toapi_keys, plus an atomicRecordAPIKeyUsagestore method that increments both alongsidelast_used_atwith a rolling 24h window. NewGET /api/api-keys/usage-statsaggregates the calling user's own keys into a summary (active count, 24h/lifetime totals, top-3 most active). OpenAPI updated.Requests (24h)/Requests (total)columns. Loading skeletons vialib/skeleton; summary errors stay isolated from list errors.TestHandler_listAPIKeysUsageStats_*,TestService_GetAPIKeysUsageStatsAPI,TestSortAPIKeysByActivity, pgxmock coverage forRecordAPIKeyUsage); frontend tests cover the summary render, empty top-list, partial-failure paths, and the missing-counter fallback.Scope
Per-key
last_used_ip/last_endpointand historical time-series charting are explicitly out of scope, same as #380 — follow-ups if useful.Test plan
go build ./...,go vet ./...,gofmt -lcleango test ./...— 5962 tests pass across 39 packagesgolangci-lint run(v2.11.4 local; CI pins v2.10.1 per project convention) — 0 issues on touched packagesgosecon touched packages — 0 issuescd frontend && npx tsc --noEmit— cleancd frontend && npm test(apikeys + api-apikeys suites) — 76/76 pass; full suite has 8 pre-existing failures unrelated to this change (locale-dependenttoLocaleStringformatting inutils.test.ts/approval-details.test.ts, confirmed no diff in those files vsorigin/main)cd frontend && npm run build— clean (pre-existing bundle-size warning only)cd frontend && npm run lint— 0 errors (pre-existinganywarnings only, none in touched files)Summary by CodeRabbit