feat(credits-admin): auth-first responsive console (login gate + activity center + slide-over detail)#371
feat(credits-admin): auth-first responsive console (login gate + activity center + slide-over detail)#371fbac wants to merge 9 commits into
Conversation
|
Claude finished @fbac's task —— View job PR Review Complete ✅OverviewThis is a well-architected refactor that transforms the credits-admin interface from a monolithic server-rendered page into a modern, auth-first SPA-style console. The code quality is high, with strong security practices, comprehensive test coverage, and adherence to repository conventions. ✅ Strengths1. Excellent Security Implementation
2. Strong Test Coverage
3. Performance & Scalability
4. Code Organization
🟡 Issues Identified1. 🟠 MEDIUM: Race Condition in Activity Loading (script.ts:66-78)Issue: The Example: // Current code (vulnerable):
function loadActivity(reset) {
if (reset) activityCursor = null;
var qs = "?action=" + activityAction + (activityCursor ? "&cursor=" + activityCursor : "");
guardFetch("/audit/recent" + qs).then(function(r) {
// No check if this response still matches current facet
renderActivityRows(j.rows || [], !reset);
activityCursor = j.nextCursor;
});
}Fix: The code already has Suggested addition: // line 68-71: Add comment
var gen = ++activityGen; // Guard against facet-switch race (discard stale responses)
var qs = "?action=" + encodeURIComponent(activityAction) + ...;
guardFetch("/audit/recent" + qs).then(function(r) { return r.json(); }).then(function(j) {
if (gen !== activityGen) return; // Race: user switched facets; discard this responseVerdict: Already fixed, but documentation would help. 2. 🟠 MEDIUM: Toast Hidden Behind Detail Panel (styles.ts:47)Issue: The Fix: Add - .toast { position:fixed; bottom:24px; right:24px; padding:12px 18px; border-radius:8px; color:#fff; opacity:0; transition:opacity .2s; pointer-events:none; }
+ .toast { position:fixed; bottom:24px; right:24px; padding:12px 18px; border-radius:8px; color:#fff; opacity:0; transition:opacity .2s; pointer-events:none; z-index:50; }Verification needed: Check the manual test plan item: "Grant round-trip — success toast, panel balance updates..." 3. 🟢 MINOR: Test Coverage Gap (audit-recent.integration.test.ts:45-72)Issue: The pagination test seeds only 3 rows (< 50-row limit) and never asserts Fix: Seed ≥51 rows, assert it("paginates newest-first with nextCursor and filters by action", async () => {
const a = await seedAccount();
tracker.push(a);
for (let i = 0; i < 52; i++) { // Cross the 50-row boundary
await writeAdminAudit({
accountId: a,
actorEmail: "admin@test",
action: i % 2 === 0 ? "grant" : "adjust",
deltaCredits: BigInt(i + 1),
reason: "seed",
idempotencyKey: `ar_${i}`,
});
}
const p1 = await adminRequest(app).get("/api/v2/credits-admin/audit/recent?action=all");
expect(p1.status).toBe(200);
const p1Body = p1.body as RecentAuditResponse;
expect(p1Body.rows).toHaveLength(50);
expect(p1Body.nextCursor).not.toBeNull();
// Fetch page 2
const p2 = await adminRequest(app).get(`/api/v2/credits-admin/audit/recent?cursor=${encodeURIComponent(p1Body.nextCursor!)}`);
expect(p2.status).toBe(200);
const p2Body = p2.body as RecentAuditResponse;
expect(p2Body.rows.length).toBeGreaterThan(0);
// Verify no overlap
const ids1 = p1Body.rows.map(r => r.id);
const ids2 = p2Body.rows.map(r => r.id);
expect(ids1.some(id => ids2.includes(id))).toBe(false);
});🔍 Additional ObservationsPositive
Neutral
📋 RecommendationsMust-Fix Before Merge
Should-Fix Before Merge
Nice-to-Have
🎯 Final VerdictAPPROVE with minor fixes. This is high-quality work with excellent security, performance, and test coverage. The two medium-severity issues are straightforward to address:
The architecture is sound, the database index is in place, and the auth-first design properly isolates sensitive data. Great job! 🎉 Summary Checklist
|
WalkthroughAdds authenticated ChangesCredits Admin console and APIs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminBrowser
participant clientScript
participant creditsAdminRouter
participant whoamiGetHandler
participant auditRecentGetHandler
AdminBrowser->>clientScript: load console shell
clientScript->>creditsAdminRouter: GET /whoami with bearer token
creditsAdminRouter->>whoamiGetHandler: authenticate and handle request
whoamiGetHandler-->>clientScript: actor identity and creditsPerUsd
clientScript->>auditRecentGetHandler: GET /audit/recent with filters
auditRecentGetHandler-->>clientScript: audit rows and nextCursor
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| } | ||
| const rows = await prisma.adminAudit.findMany({ | ||
| where, | ||
| orderBy: [{ createdAt: "desc" }, { id: "desc" }], |
There was a problem hiding this comment.
🟡 Medium credits-admin/audit-repository.ts:77
listRecentAdminAudit orders the entire AdminAudit table by (createdAt, id) with no matching index — the schema only indexes (accountId, createdAt) and (actorEmail, createdAt). The first-page query (and each action-filtered first page) therefore scans and sorts the whole audit table before returning 50 rows. As audit history grows, this endpoint becomes progressively slower and more CPU-intensive. Add an index on (createdAt, id) matching the keyset order, and consider a composite index covering the action-filtered access path.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/api/v2/credits-admin/audit-repository.ts around line 77:
`listRecentAdminAudit` orders the entire `AdminAudit` table by `(createdAt, id)` with no matching index — the schema only indexes `(accountId, createdAt)` and `(actorEmail, createdAt)`. The first-page query (and each `action`-filtered first page) therefore scans and sorts the whole audit table before returning 50 rows. As audit history grows, this endpoint becomes progressively slower and more CPU-intensive. Add an index on `(createdAt, id)` matching the keyset order, and consider a composite index covering the `action`-filtered access path.
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. New feature adding auth-first console with multiple new endpoints in credits-admin area. Author does not own any of the 17 changed files (all owned by xmtplabs/engineering). Four unresolved medium-severity comments exist, including a parseInt truncation issue that could apply incorrect credit amounts during admin mutations. You can customize Macroscope's approvability policy. Learn more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/credits-admin/audit-recent.integration.test.ts (1)
45-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
nextCursorthrough the HTTP endpoint.This test claims pagination coverage but seeds fewer than the 50-row page limit, never asserts
nextCursor, and never requests page two. Seed at least 51 rows, then verify the cursor request returns a non-overlapping continuation page.🤖 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 `@tests/credits-admin/audit-recent.integration.test.ts` around lines 45 - 72, Update the test case around “paginates newest-first with nextCursor and filters by action” to seed at least 51 audit rows, assert the first response includes a nextCursor, then request the endpoint with that cursor and verify the continuation page is non-overlapping with the first page. Preserve the existing newest-first, response-shape, accountId, and action-filter assertions.src/api/v2/credits-admin/handlers/admin-page/index.ts (2)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit
return;statement.As per coding guidelines, always place the
return;statement on a new line immediately after sending a response in API routes.♻️ Proposed refactor
</body> </html>`); + return; };🤖 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 `@src/api/v2/credits-admin/handlers/admin-page/index.ts` around lines 31 - 32, Add an explicit return statement on the line immediately after sending the response in the affected API route handler, using the surrounding handler’s existing symbol to locate the change and preserving the current response content.Source: Coding guidelines
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOmit explicit return types. As per coding guidelines, prefer inferring the value in TypeScript instead of specifying the return type on functions.
src/api/v2/credits-admin/handlers/admin-page/index.ts#L8-L8: Remove the explicit: voidreturn type.src/api/v2/credits-admin/handlers/admin-page/login-view.ts#L1-L1: Remove the explicit: stringreturn type.src/api/v2/credits-admin/handlers/admin-page/console-view.ts#L1-L1: Remove the explicit: stringreturn type.src/api/v2/credits-admin/handlers/admin-page/script.ts#L1-L1: Remove the explicit: stringreturn type.🤖 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 `@src/api/v2/credits-admin/handlers/admin-page/index.ts` at line 8, Remove the explicit return type annotations from adminPageHandler and the exported functions in src/api/v2/credits-admin/handlers/admin-page/login-view.ts (line 1), console-view.ts (line 1), and script.ts (line 1), allowing TypeScript to infer their return types.Source: Coding guidelines
tests/credits-admin/admin-page.test.ts (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
get()helper. Both of these lines duplicate the exact logic of theget()helper defined on line 11. Consider reusing the helper to simplify the test assertions.
tests/credits-admin/admin-page.test.ts#L49-L49: ReplaceadminRequest(app, false).get("/api/v2/credits-admin/")withget().tests/credits-admin/admin-page.test.ts#L57-L57: ReplaceadminRequest(app, false).get("/api/v2/credits-admin/")withget().🤖 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 `@tests/credits-admin/admin-page.test.ts` at line 49, Replace the duplicated admin requests at tests/credits-admin/admin-page.test.ts lines 49-49 and 57-57 with the existing get() helper defined in the test file, preserving the current assertions and behavior at both sites.
🤖 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.
Nitpick comments:
In `@src/api/v2/credits-admin/handlers/admin-page/index.ts`:
- Around line 31-32: Add an explicit return statement on the line immediately
after sending the response in the affected API route handler, using the
surrounding handler’s existing symbol to locate the change and preserving the
current response content.
- Line 8: Remove the explicit return type annotations from adminPageHandler and
the exported functions in
src/api/v2/credits-admin/handlers/admin-page/login-view.ts (line 1),
console-view.ts (line 1), and script.ts (line 1), allowing TypeScript to infer
their return types.
In `@tests/credits-admin/admin-page.test.ts`:
- Line 49: Replace the duplicated admin requests at
tests/credits-admin/admin-page.test.ts lines 49-49 and 57-57 with the existing
get() helper defined in the test file, preserving the current assertions and
behavior at both sites.
In `@tests/credits-admin/audit-recent.integration.test.ts`:
- Around line 45-72: Update the test case around “paginates newest-first with
nextCursor and filters by action” to seed at least 51 audit rows, assert the
first response includes a nextCursor, then request the endpoint with that cursor
and verify the continuation page is non-overlapping with the first page.
Preserve the existing newest-first, response-shape, accountId, and action-filter
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b9e8bf4-58a8-4881-9b78-0a4608ee57da
📒 Files selected for processing (15)
src/api/v2/credits-admin/audit-repository.tssrc/api/v2/credits-admin/credits-admin.router.tssrc/api/v2/credits-admin/handlers/admin-page.tssrc/api/v2/credits-admin/handlers/admin-page/console-view.tssrc/api/v2/credits-admin/handlers/admin-page/index.tssrc/api/v2/credits-admin/handlers/admin-page/login-view.tssrc/api/v2/credits-admin/handlers/admin-page/script.tssrc/api/v2/credits-admin/handlers/admin-page/styles.tssrc/api/v2/credits-admin/handlers/audit-recent-get.tssrc/api/v2/credits-admin/handlers/whoami-get.tssrc/api/v2/credits-admin/schemas/requests.tstests/credits-admin/admin-page.test.tstests/credits-admin/audit-recent.integration.test.tstests/credits-admin/audit-repository.test.tstests/credits-admin/whoami.integration.test.ts
💤 Files with no reviewable changes (1)
- src/api/v2/credits-admin/handlers/admin-page.ts
…y race, audit index) - toast: add z-index:50 so grant/adjust success toasts render above the detail slide-over (scrim z-30 / panel z-40) instead of beneath it. - loadActivity: request-generation counter drops stale responses, fixing the facet-switch / rapid load-more race (wrong cursor / duplicate rows). - AdminAudit: add @@index([createdAt, id]) matching the global keyset order so /audit/recent stops seq-scanning the whole table as history grows.
| if(kind==="grant"){ var c=parseInt(el("d-grant-credits").value,10); var gr=el("d-grant-reason").value.trim(); | ||
| if(!c||c<=0){ toast("Positive credits required","error"); return; } if(!gr){ toast("Reason required","error"); return; } | ||
| body={credits:c,reason:gr,idempotencyKey:newKey("admin_grant")}; path="/grant"; btn=el("d-grant-btn"); } | ||
| else { var d=parseInt(el("d-adjust-delta").value,10); var ar=el("d-adjust-reason").value.trim(); | ||
| if(!d){ toast("Non-zero delta required","error"); return; } if(!ar){ toast("Reason required","error"); return; } | ||
| body={delta:d,reason:ar,idempotencyKey:newKey("admin_adjust")}; path="/adjust"; btn=el("d-adjust-btn"); } |
There was a problem hiding this comment.
🟡 Medium admin-page/script.ts:162
parseInt on type="number" inputs silently truncates valid numeric input. An admin entering 1e3 to grant 1,000 credits sends {credits: 1}, and 1.5 is truncated to 1 instead of rejected — so the mutation applies a different amount than the admin intended. parseInt stops at the first non-digit character, so the exponent or decimal portion is dropped. Parse the full value (e.g., Number(...)) and require a finite integer before proceeding.
- if(kind==="grant"){ var c=parseInt(el("d-grant-credits").value,10); var gr=el("d-grant-reason").value.trim();
- if(!c||c<=0){ toast("Positive credits required","error"); return; } if(!gr){ toast("Reason required","error"); return; }
+ if(kind==="grant"){ var gc=Number(el("d-grant-credits").value); var gr=el("d-grant-reason").value.trim();
+ if(!isFinite(gc)||gc<=0||Math.floor(gc)!==gc){ toast("Positive integer credits required","error"); return; } if(!gr){ toast("Reason required","error"); return; } var c=gc;
- else { var d=parseInt(el("d-adjust-delta").value,10); var ar=el("d-adjust-reason").value.trim();
- if(!d){ toast("Non-zero delta required","error"); return; } if(!ar){ toast("Reason required","error"); return; }
+ else { var ad=Number(el("d-adjust-delta").value); var ar=el("d-adjust-reason").value.trim();
+ if(!isFinite(ad)||ad===0||Math.floor(ad)!==ad){ toast("Non-zero integer delta required","error"); return; } if(!ar){ toast("Reason required","error"); return; } var d=ad;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/api/v2/credits-admin/handlers/admin-page/script.ts around lines 162-167:
`parseInt` on `type="number"` inputs silently truncates valid numeric input. An admin entering `1e3` to grant 1,000 credits sends `{credits: 1}`, and `1.5` is truncated to `1` instead of rejected — so the mutation applies a different amount than the admin intended. `parseInt` stops at the first non-digit character, so the exponent or decimal portion is dropped. Parse the full value (e.g., `Number(...)`) and require a finite integer before proceeding.
1 participant
Summary
Reworks
src/api/v2/credits-admin/from a single server-rendered page into an auth-first, responsive console.Backend (2 new bearer-gated reads, no DB migration, no money-mutation change):
listRecentAdminAudit+ keyset cursor (encode/decodeAuditCursor) inaudit-repository.ts— global newest-first pagination with a(createdAt, id)tiebreaker.GET /audit/recent?action=&cursor=— paginated global admin-activity feed.GET /whoami— login validation + actor identity +creditsPerUsd. MovingcreditsPerUsdhere keeps the public shell data-free.Client (
handlers/admin-page.ts→handlers/admin-page/):index.ts(CSP + assembly),styles.ts,login-view.ts,console-view.ts,script.ts./whoami.guardFetchbounces to login on any 401. Lock button.Design notes:
creditsAdminTokenAuth) on every data endpoint. The publicGET /shell serves no data, which is what makes that acceptable.UserCredits/CreditLedgerwrites — grant/adjust still route through@/paymentsvia the existing endpoints.script-src 'nonce-…', no inline handlers); every DB free-text field is escaped in the client renderers.Verification (automated)
pnpm vitest run tests/credits-admin/→ 64/64 pass (13 files).pnpm typecheck,pnpm lint(repo-wideeslint .),prettier --check→ all clean.node --checkon the assembled inline browser JS → OK.Test Plan (manual browser-walk — required before merge)
Run against
pnpm devwithCREDITS_ADMIN_API_TOKENset, at/api/v2/credits-admin/, with one seeded account (subscription + some admin-audit rows):/audit/recent//accounts//searchbefore unlock.Known deferred (non-blocking)
CREDITS_ADMIN_REQUIRE_CF_IDENTITY=true401 — dormant (no CF perimeter deployed on convos-backend).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Add auth-first credits admin console with login gate, activity center, and slide-over detail panel
GET /whoamito resolve actor identity andcreditsPerUsd, then loads recent admin activity viaGET /audit/recent.GET /api/v2/credits-admin/whoamireturning the actor's email andcreditsPerUsd, andGET /api/v2/credits-admin/audit/recentwith optional action filter and keyset pagination backed by a new composite index onAdminAudit(createdAt, id).Macroscope summarized b44a7de.
Summary by CodeRabbit
New Features
Bug Fixes
Tests