feat(devices): add per-device read+rename APIs and fix submit_count drift - #593
Conversation
…rift Schema: - Add 0010_submit_count_safety.sql to ALTER TABLE submissions ADD COLUMN IF NOT EXISTS submit_count. The column is referenced by schema.ts and written on every /api/submit call, but no earlier .sql migration adds it; prod has it because somebody once ran drizzle-kit push directly, so a fresh drizzle-kit migrate on CI/staging/dev currently breaks /api/submit. Verified locally against postgres:16. API: - GET /api/users/[username]/devices: aggregated per-device totals (tokens, cost, input/output, active days) for a user's submission devices, sourced from submitted_devices LEFT JOIN daily_breakdown. - GET /api/users/[username]/devices/[deviceId]: per-device detail with day-by-day contributions. Cross-user device ids silently 404. - PATCH /api/settings/devices/[deviceId]: session-authenticated rename (or clear, via name: null). Validates length and rejects control characters; ownership enforced in the same WHERE. Helpers: - src/lib/devices/shared.ts: deviceDisplayLabel() and toIsoString() shared across the three routes so the public label format cannot drift. Local dev: - scripts/seed-dev.ts: idempotent synthetic seed (3 users × 2 devices × 14 days + 1 group). Refuses any non-localhost DATABASE_URL. Ported from #389 onto main's submitted_devices model (#517) rather than that PR's submissions.source_id schema rewrite. #389's CLI source-id lockfile and profile Devices tab UI are deliberately not in this PR; they want separate decisions (CLI lock vs main's existing device.id payload; UI as a styled-components review pass). Constraint: do not regress main's submitted_devices schema or rehash the migrated multi-machine model Rejected: include #389's index-cleanup migration (drop idx_submissions_user_id et al.) | needs a fresh pg_stat_user_indexes audit on current prod before dropping anything in a single transaction Rejected: bundle #389's CLI source-id lockfile | main already sends device.id via #517/#545; the lockfile is a behavior change, not a port Confidence: high Scope-risk: narrow Directive: /api/users/[username]/devices is public — match it to /api/users/[username]'s visibility model when extending; the rename endpoint is the only auth-gated piece Not-tested: interaction with the existing rank cache (rankTotal in embed routes is unaffected)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d73b137250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .where(usernameEqualsIgnoreCase(username)) | ||
| .limit(USERNAME_LOOKUP_LIMIT); | ||
|
|
||
| const user = getSingleUsernameMatch(matchingUsers, username); |
There was a problem hiding this comment.
Handle ambiguous username lookup instead of returning 500
getSingleUsernameMatch can throw AmbiguousUsernameError, but this handler only has a generic catch path, so duplicated case-insensitive usernames will surface as a 500 instead of the expected 409 behavior used by /api/users/[username]. That makes the new device endpoint less robust in the same legacy-data scenario already accounted for elsewhere.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/frontend/src/app/api/settings/devices/[deviceId]/route.ts">
<violation number="1" location="packages/frontend/src/app/api/settings/devices/[deviceId]/route.ts:111">
P2: Normalize the username before `revalidateTag` so cache invalidation hits the same `user:*` tag format used elsewhere.</violation>
</file>
<file name="packages/frontend/scripts/seed-dev.ts">
<violation number="1" location="packages/frontend/scripts/seed-dev.ts:198">
P1: The local-only DATABASE_URL guard is bypassable because it regex-matches the entire URL string instead of validating the parsed hostname.</violation>
</file>
<file name="packages/frontend/src/app/api/users/[username]/devices/route.ts">
<violation number="1" location="packages/frontend/src/app/api/users/[username]/devices/route.ts:42">
P2: Ambiguous username lookups are turned into 500s because `getSingleUsernameMatch` can throw but this handler only returns a generic internal error. Handle the >1 match case explicitly so clients get a deterministic 409 response.</violation>
</file>
<file name="packages/frontend/src/app/api/users/[username]/devices/[deviceId]/route.ts">
<violation number="1" location="packages/frontend/src/app/api/users/[username]/devices/[deviceId]/route.ts:53">
P2: Handle `AmbiguousUsernameError` explicitly; this route currently returns 500 for ambiguous username matches instead of a client-facing 409.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Refuse to run against anything that doesn't look local. Production | ||
| // database connection strings should not be passed to this seeder. | ||
| const url = process.env.DATABASE_URL; | ||
| if (!/localhost|127\.0\.0\.1|::1/.test(url)) { |
There was a problem hiding this comment.
P1: The local-only DATABASE_URL guard is bypassable because it regex-matches the entire URL string instead of validating the parsed hostname.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/scripts/seed-dev.ts, line 198:
<comment>The local-only DATABASE_URL guard is bypassable because it regex-matches the entire URL string instead of validating the parsed hostname.</comment>
<file context>
@@ -0,0 +1,230 @@
+ // Refuse to run against anything that doesn't look local. Production
+ // database connection strings should not be passed to this seeder.
+ const url = process.env.DATABASE_URL;
+ if (!/localhost|127\.0\.0\.1|::1/.test(url)) {
+ console.error(
+ `seed-dev refuses to run against DATABASE_URL=${url}: only local hosts are allowed`
</file context>
| try { | ||
| // Second arg "max" matches the rest of the codebase | ||
| // (settings/submitted-data/route.ts, submit/route.ts). | ||
| revalidateTag(`user:${session.username}`, "max"); |
There was a problem hiding this comment.
P2: Normalize the username before revalidateTag so cache invalidation hits the same user:* tag format used elsewhere.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/src/app/api/settings/devices/[deviceId]/route.ts, line 111:
<comment>Normalize the username before `revalidateTag` so cache invalidation hits the same `user:*` tag format used elsewhere.</comment>
<file context>
@@ -0,0 +1,127 @@
+ try {
+ // Second arg "max" matches the rest of the codebase
+ // (settings/submitted-data/route.ts, submit/route.ts).
+ revalidateTag(`user:${session.username}`, "max");
+ } catch (e) {
+ console.error("Cache invalidation failed after device rename:", e);
</file context>
| .where(usernameEqualsIgnoreCase(username)) | ||
| .limit(USERNAME_LOOKUP_LIMIT); | ||
|
|
||
| const user = getSingleUsernameMatch(matchingUsers, username); |
There was a problem hiding this comment.
P2: Ambiguous username lookups are turned into 500s because getSingleUsernameMatch can throw but this handler only returns a generic internal error. Handle the >1 match case explicitly so clients get a deterministic 409 response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/src/app/api/users/[username]/devices/route.ts, line 42:
<comment>Ambiguous username lookups are turned into 500s because `getSingleUsernameMatch` can throw but this handler only returns a generic internal error. Handle the >1 match case explicitly so clients get a deterministic 409 response.</comment>
<file context>
@@ -0,0 +1,107 @@
+ .where(usernameEqualsIgnoreCase(username))
+ .limit(USERNAME_LOOKUP_LIMIT);
+
+ const user = getSingleUsernameMatch(matchingUsers, username);
+ if (!user) {
+ return NextResponse.json({ error: "User not found" }, { status: 404 });
</file context>
| .where(usernameEqualsIgnoreCase(username)) | ||
| .limit(USERNAME_LOOKUP_LIMIT); | ||
|
|
||
| const user = getSingleUsernameMatch(matchingUsers, username); |
There was a problem hiding this comment.
P2: Handle AmbiguousUsernameError explicitly; this route currently returns 500 for ambiguous username matches instead of a client-facing 409.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/src/app/api/users/[username]/devices/[deviceId]/route.ts, line 53:
<comment>Handle `AmbiguousUsernameError` explicitly; this route currently returns 500 for ambiguous username matches instead of a client-facing 409.</comment>
<file context>
@@ -0,0 +1,144 @@
+ .where(usernameEqualsIgnoreCase(username))
+ .limit(USERNAME_LOOKUP_LIMIT);
+
+ const user = getSingleUsernameMatch(matchingUsers, username);
+ if (!user) {
+ return NextResponse.json({ error: "User not found" }, { status: 404 });
</file context>
Migration 0011 drops five pieces of confirmed-dead schema surface: - daily_breakdown.provider_breakdown: declared in schema.ts but never read or written anywhere in src/. Pure allocated bytes per row. - daily_breakdown.model_breakdown: written on every submit, but the only SELECT (users/[username]/route.ts:110) discards the value. Storage + serialization churn with no consumer. - submissions.status + idx_submissions_status: column is only ever written as 'verified' on insert; zero WHERE filters anywhere. The index serves no query. - users.is_admin: column is set/fetched but no admin gate exists anywhere in the codebase. A user with is_admin=true had zero additional privileges. If admin features ship later, reintroduce WITH an actual gate. Code changes follow the schema: remove the field from drizzle table defs, drop the buildModelBreakdown helper (only caller was the submit write path now also gone), and stop emitting / reading the dropped fields from session/auth/profile/submit endpoints. Tests: removed 21 isAdmin mock-object entries and the test that inlined buildModelBreakdown's algorithm. Also fixed a pre-existing leaderboard.test.ts failure caused by #522 adding customFrom / customTo positional args without updating the expectation. Verification: tsc 0 errors, 251/251 vitest, end-to-end POST /api/submit against the post-drop schema returns 200 and writes a daily_breakdown row successfully. Constraint: do not regress the active submit / profile / devices paths Constraint: depends on #593's 0010_submit_count_safety being present Rejected: drop submissions.idx_submissions_user_id + idx_submissions_total_tokens + idx_submissions_date_range as well | needs a fresh pg_stat_user_indexes audit on prod first; the index cleanup is a separate analysis-driven PR Rejected: drop submissions_user_hash_unique constraint (functionally redundant with submissions_user_id_unique) | low-blast-radius but worth its own PR with a freshness check on the submit-idempotency code path Confidence: high Scope-risk: moderate (touches submit/route.ts, the critical write path) Directive: do NOT reintroduce users.is_admin without also wiring an admin gate; the previous setup misled readers into thinking access control existed Not-tested: ad-hoc SQL queries that may have been reading the dropped jsonb columns on prod
…598) * chore(db): drop dead columns and index flagged by post-batch audit Migration 0011 drops five pieces of confirmed-dead schema surface: - daily_breakdown.provider_breakdown: declared in schema.ts but never read or written anywhere in src/. Pure allocated bytes per row. - daily_breakdown.model_breakdown: written on every submit, but the only SELECT (users/[username]/route.ts:110) discards the value. Storage + serialization churn with no consumer. - submissions.status + idx_submissions_status: column is only ever written as 'verified' on insert; zero WHERE filters anywhere. The index serves no query. - users.is_admin: column is set/fetched but no admin gate exists anywhere in the codebase. A user with is_admin=true had zero additional privileges. If admin features ship later, reintroduce WITH an actual gate. Code changes follow the schema: remove the field from drizzle table defs, drop the buildModelBreakdown helper (only caller was the submit write path now also gone), and stop emitting / reading the dropped fields from session/auth/profile/submit endpoints. Tests: removed 21 isAdmin mock-object entries and the test that inlined buildModelBreakdown's algorithm. Also fixed a pre-existing leaderboard.test.ts failure caused by #522 adding customFrom / customTo positional args without updating the expectation. Verification: tsc 0 errors, 251/251 vitest, end-to-end POST /api/submit against the post-drop schema returns 200 and writes a daily_breakdown row successfully. Constraint: do not regress the active submit / profile / devices paths Constraint: depends on #593's 0010_submit_count_safety being present Rejected: drop submissions.idx_submissions_user_id + idx_submissions_total_tokens + idx_submissions_date_range as well | needs a fresh pg_stat_user_indexes audit on prod first; the index cleanup is a separate analysis-driven PR Rejected: drop submissions_user_hash_unique constraint (functionally redundant with submissions_user_id_unique) | low-blast-radius but worth its own PR with a freshness check on the submit-idempotency code path Confidence: high Scope-risk: moderate (touches submit/route.ts, the critical write path) Directive: do NOT reintroduce users.is_admin without also wiring an admin gate; the previous setup misled readers into thinking access control existed Not-tested: ad-hoc SQL queries that may have been reading the dropped jsonb columns on prod * chore(db): drop redundant indexes and unique constraint, add FK coverage Extends 0011_drop_dead_columns with the index-cleanup (group B) and redundant-unique-constraint (group C) drops that were originally proposed as follow-up PRs, after a fresh integrity audit on the local DB confirmed each drop is safe. Index drops: - idx_submissions_user_id: redundant left-prefix of idx_submissions_leaderboard (user_id, total_tokens, total_cost, created_at). Per #389's prod stats audit, 214 scans on this vs 3.27M on the leaderboard composite. - idx_submissions_total_tokens: 0 prod scans (#389 audit). All reads use SUM() per-user GROUP BY which is served by the leaderboard composite. - idx_submissions_date_range: 0 prod scans (#389 audit). No filter ever queries these columns; leaderboard period filters use daily_breakdown.date which has its own index. - idx_users_username, idx_sessions_token, idx_api_tokens_token, idx_device_codes_device_code, idx_device_codes_user_code: Postgres auto-creates a btree for every UNIQUE constraint; these explicit btrees were 1:1 duplicates of *_unique indexes and only added INSERT/UPDATE write amplification. Index adds (FK coverage): - idx_device_codes_user_id: cascade-delete of a user did a seq scan of device_codes. - idx_group_members_invited_by: cascade-delete of an inviter did a seq scan of group_members. - idx_group_invites_invited_by: same, for group_invites. Constraint drop: - submissions_user_hash_unique (user_id, submission_hash) was functionally subsumed by submissions_user_id_unique (user_id). The stronger constraint already enforces one row per user, so any (user_id, submission_hash) collision is rejected at the user_id level. The submission_hash column itself stays; /api/submit still reads it for idempotency comparison. Verification on clean local DB: - All 11 prior migrations (0000-0010) plus this 0011 apply successfully from an empty schema. - pg_constraint audit: all 14 FKs in the schema now have covering indexes (was 11 before; +3 from this PR). - Index audit: all 9 dead/redundant indexes confirmed gone; the 3 new FK-coverage indexes confirmed present. - bunx tsc --noEmit: 0 errors. - bunx vitest run __tests__: 251/251 pass. - End-to-end POST /api/submit with a real api_tokens row: HTTP 200, daily_breakdown row written, submission totals updated. - Seed script reload: 3 users, 6 devices, 84 daily rows, 1 group with 3 members — all counts consistent. Constraint: each index drop must have either a prod stats scan-count justification or be a literal duplicate of an existing unique-constraint index Constraint: do not regress FK cascade-delete behavior Rejected: drop idx_submissions_created_at | not redundant — leaderboard composite has created_at as the 4th column; a plain created_at filter still benefits from a dedicated index Rejected: drop idx_users_github_id | github OAuth login does a lookup by github_id on every sign-in; this index is hot Confidence: high Scope-risk: moderate (drops are irreversible; index recreation in prod requires CREATE INDEX CONCURRENTLY if scans turn out to be needed) Directive: do NOT re-add an explicit btree on any column that already has a UNIQUE constraint — postgres creates the covering index automatically Not-tested: prod pg_stat_user_indexes scan counts as of merge day (the audit data is ~6 weeks old per #389) * chore(db): slim 0011 scope based on prod pg_stat_user_indexes audit The 2026-05-25 prod audit (pg_stat_user_indexes) changed the picture for the duplicate-of-unique-constraint indexes I planned to drop: - idx_sessions_token: 89,560 scans (sessions_token_unique: 0) - idx_users_username: 30,080 scans (users_username_unique: 0) - idx_api_tokens_token: 27,126 scans (api_tokens_token_unique: 0) - idx_device_codes_device_code: 153 scans - idx_device_codes_user_code: 3 scans Postgres' planner consistently picks the explicit non-unique index over the unique-constraint sibling, so these are NOT no-op drops — the unique-constraint indexes show 0 scans in prod because they aren't being chosen. Removing the explicit ones would invalidate cached plans and force a re-plan onto the unique-constraint indexes; safe in theory but a non-zero risk window. Removed those five DROP INDEX statements from migration 0011 and restored the corresponding index() declarations to schema.ts with a comment explaining why the explicit non-unique siblings stay. Two drops that prod stats initially questioned but the investigation subagents confirmed safe (no app caller, scans are write-side enforcement / vacuum): - idx_submissions_status: 323 scans — vacuum cycles + historical scan count from before commit acccd44 removed the column from the drizzle model. No app WHERE filter exists. - submissions_user_hash_unique: 388 scans — pure constraint-check btree probes on every INSERT/UPDATE that writes submission_hash; no read query references the column. Both still get dropped. Constraint: do not invalidate hot cached query plans on prod without a controlled re-plan window Rejected: drop all 5 unique-duplicate btrees | the planner uses them, sessions_token alone is 89k scans Confidence: high Scope-risk: narrow (only removed planned drops; nothing else changed) Directive: do NOT remove the 5 explicit btrees on prod without first running ANALYZE on the table and verifying the planner falls back to the unique-constraint index Not-tested: cold-plan latency after dropping any of these in a future PR
Resolution policy: take main wholesale for everything superseded by the relational device model that landed via junhoyeo#517/junhoyeo#593/junhoyeo#609 — device.rs, main.rs, schema.ts, helpers.ts, submit route + tests, and the whole TUI (app.rs, tui/mod.rs, footer.rs). Keep only the PR's remote-stats half (tui/remote.rs, /api/me/stats route, meStats tests) to be rebuilt on top of main in follow-up commits. Drop the next-server.d.ts shim that the PR added as a workaround for the PR-era Next version. Refs junhoyeo#699
Summary
Ports the user-facing "devices" surface from #389 onto main's already-merged
submitted_devicesschema (from #517), and ships the schema-drift fix that #389's audit surfaced forsubmissions.submit_count.#249 and #389 themselves remain open — both are architecturally superseded by what's now on main (#524 groups; #517 device-aware merges). This PR extracts the parts that are new value: the read APIs for per-device usage, a rename endpoint, the seed script for local dev, and the migration that unbreaks fresh deploys.
Why now
packages/frontend/src/lib/db/schema.tsdeclaressubmissions.submit_countandsrc/app/api/submit/route.ts:465writes to it on every submit, but no migration file actually adds the column. Production has it because somebody once randrizzle-kit pushagainst the live DB; any freshdrizzle-kit migraterun today (CI, staging, a new local dev DB) leaves the column missing and breaks/api/submiton the very first submission.Verified on
2026-05-25againstpostgres:16in docker: a cleandrizzle-kit migraterun against an empty DB does not produce the column. #389's commit message audited prod and called this out a month ago; this PR is the first time the fix lands.Changes
Migration
0010_submit_count_safety.sql—ALTER TABLE submissions ADD COLUMN IF NOT EXISTS submit_count integer DEFAULT 1 NOT NULL. No-op on prod, repair on every other environment.New read APIs (public, no auth — matches
/api/users/[username])GET /api/users/[username]/devices— list a user's submission devices with aggregated totals (tokens, cost, input/output, active-day count, first/last day). One query:submitted_devices LEFT JOIN daily_breakdown GROUP BY device_id. Ordered bylast_submitted_at DESC NULLS LAST.GET /api/users/[username]/devices/[deviceId]— per-device detail: device metadata + day-by-day contributions. Cross-user device id silently 404s (no existence leak).New write API (session-authenticated)
PATCH /api/settings/devices/[deviceId]— rename or clear a device's display label. Validates ≤120 chars (matches column), rejects Unicode control characters. Ownership enforced in the sameWHEREso non-owner attempts 404.Helpers
src/lib/devices/shared.ts—deviceDisplayLabel()(fallback to"Legacy submissions"/"Unnamed device") andtoIsoString()(stable timestamp serialization). Shared by all three routes so the public label format can't drift.Local dev tooling
scripts/seed-dev.ts— idempotent synthetic seed: 3 users × 2 devices × 14 days + 1 group. Refuses any non-localhostDATABASE_URL. Use withbun run packages/frontend/scripts/seed-dev.ts.What is not in this PR
submissions.source_id/source_nameschema rewritesubmitted_devicesmodel on main; that decision is bigger than one PRTOKSCALE_SOURCE_IDenv vardevice: { id, name }in submit payload via #517 + #545idx_submissions_user_id, etc.)pg_stat_user_indexesaudit; wants a separate analysis-driven PR with the matching audit re-run, not bundled hereThe two source PRs (#389, #249) should be closed once this lands and you confirm nothing's missing.
Verification
Local stack (docker
postgres:16onlocalhost:5432,bun run db:migrateapplied):bunx tsc --noEmit→ 0 errorsbunx vitest run __tests__→ exit 0bun run scripts/seed-dev.tsagainst local DB → 3 users + 6 devices + 84 daily_breakdown rows + 1 groupcurl /api/users/seed-dev-alice/devices→ 200 JSON, two devices with summed tokens 1,365,000 + 1,155,000 (matches rawSUM(tokens)query)curl /api/users/seed-dev-alice/devices/<uuid>→ 200 JSON, 14 contributionscurl /api/users/seed-dev-alice/devices/not-a-uuid→ 400 JSON{"error":"Invalid device id"}curl /api/users/nobody-here/devices/<uuid>→ 404 JSON{"error":"User not found"}curl -X PATCH /api/settings/devices/<uuid>without auth → 401 JSONTest plan
0010_submit_count_safetyapplies cleanly on staging (no-op if column already there)/api/users/<your-username>/devicesafter deploy; counts should match/api/users/<your-username>totalsSummary by cubic
Adds public per-device read APIs and a session-authenticated rename endpoint for devices. Also fixes schema drift by adding a safety migration for
submissions.submit_countso fresh environments don't break.New Features
GET /api/users/[username]/devices: list devices with aggregated totals (tokens, cost, input/output, active days).GET /api/users/[username]/devices/[deviceId]: per-device detail with daily contributions; cross-user ids 404; 60s revalidate.PATCH /api/settings/devices/[deviceId]: rename or clear display label; ≤120 chars; rejects control chars; ownership enforced in a single WHERE.deviceDisplayLabel()andtoIsoString()to keep labels and timestamps consistent.scripts/seed-dev.tsseeds 3 users × 2 devices × 14 days; idempotent; refuses non-localhostDATABASE_URL.Migration
0010_submit_count_safety.sql:ALTER TABLE submissions ADD COLUMN IF NOT EXISTS submit_count integer DEFAULT 1 NOT NULL.drizzle-kit migrate; no-op on prod where the column already exists.Written for commit d73b137. Summary will update on new commits. Review in cubic