feat: add batch stats endpoint support#1451
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds batch processing to the stats endpoint, introduces structured PostResult/BatchStatsResult types, normalizes input via parseBodyRaw, validates uniform app_id and rate-limits batches, returns per-item results, and preserves single-event backward compatibility while updating tests for batch scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as POST Handler
participant Parser as parseBodyRaw
participant PostFn as post()
participant DB as Database
Client->>Handler: POST /stats (single or batch)
Handler->>Parser: parseBodyRaw(context)
Parser->>Parser: Normalize JSON (lowercase device_id)
Parser-->>Handler: AppStats | AppStats[]
alt Batch Mode
Handler->>Handler: Validate uniform app_id & apply batch rate limit
loop per item (index)
Handler->>PostFn: post(context, drizzleClient, item)
PostFn->>DB: validate, lookup, insert/update
PostFn-->>Handler: PostResult {success, error?, message?, isOnprem?, moreInfo?}
Handler->>Handler: Accumulate BatchStatsResult (status, index, ...)
end
Handler-->>Client: {status: 'ok', results: BatchStatsResult[]}
else Single Mode
Handler->>PostFn: post(context, drizzleClient, item)
PostFn->>DB: validate, lookup, insert/update
PostFn-->>Handler: PostResult
alt success
Handler-->>Client: BRES (existing success response)
else error
Handler-->>Client: structured error via simpleError200
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 356bcca13e
ℹ️ 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".
| // Rate limit check on first event's app_id (all events in batch should be from same app) | ||
| if (isLimited(c, events[0].app_id)) { | ||
| return simpleRateLimit({ app_id: events[0].app_id }) | ||
| } |
There was a problem hiding this comment.
Batch requests only check isLimited for events[0].app_id, but nothing enforces that all events share the same app_id. If a client sends a batch containing multiple apps, only the first app is rate-limited and the rest bypass any LIMITED_APPS throttling, which can defeat per-app rate limits in production. Consider validating that all events share the same app_id, or apply isLimited per event/app.
Useful? React with 👍 / 👎.
| describe('[POST] /stats batch operations', () => { | ||
| it('should handle batch of events', async () => { | ||
| const uuid1 = randomUUID().toLowerCase() |
There was a problem hiding this comment.
Repository guidance in AGENTS.md (repo root) states: “Maximize parallelism: Use it.concurrent() instead of it() to run tests in parallel within the same file.” The newly added batch tests use it(...), which forces sequential execution and violates that requirement. Please switch these new tests to it.concurrent(...) to align with the mandated parallel test strategy.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/plugins/stats.ts`:
- Around line 243-249: The single-event path currently uses simpleError200 which
forces HTTP 200 for validation failures; replace that call with quickError so
the response can carry the proper status code. In the block handling !isBatch
(where results[0] is inspected), call quickError(c, result.error!,
result.message!, result.statusCode ?? result.code ?? 400) instead of
simpleError200 so validation errors return their intended HTTP status (falling
back to 400 if the result has no explicit status field); keep the existing
c.json(BRES) for the 'ok' case.
- Around line 192-206: The batch-level mixed-app_ids check and the isLimited
call use firstAppId (events[0].app_id) before any schema validation; move
validation so firstAppId is guaranteed to be a validated string: run
jsonRequestSchema / parsePluginBody (or otherwise validate events[0].app_id)
prior to computing firstAppId and performing the for-loop and isLimited check,
or alternatively iterate through events and call parsePluginBody for each item
up-front to validate app_id types before calling simpleError200,
simpleRateLimit, or isLimited; ensure references to firstAppId, events, isBatch,
parsePluginBody, jsonRequestSchema, simpleError200, and isLimited are updated
accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
supabase/functions/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
supabase/functions/_backend/plugins/stats.tstests/stats.test.ts
🧰 Additional context used
📓 Path-based instructions (9)
supabase/functions/_backend/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
supabase/functions/_backend/**/*.{ts,js}: Backend code must be placed insupabase/functions/_backend/as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
UsecreateHonofromutils/hono.tsfor all Hono framework application initialization and routing
All database operations must usegetPgClient()orgetDrizzleClient()fromutils/pg.tsfor PostgreSQL access during active migration to Cloudflare D1
All Hono endpoint handlers must acceptContext<MiddlewareKeyVariables>and usec.get('requestId'),c.get('apikey'), andc.get('auth')for request context
Use structured logging withcloudlog({ requestId: c.get('requestId'), message: '...' })for all backend logging
UsemiddlewareAPISecretfor internal API endpoints andmiddlewareKeyfor external API keys; validate againstowner_orgin theapikeystable
Checkc.get('auth')?.authTypeto determine authentication type ('apikey' vs 'jwt') in backend endpoints
Use Drizzle ORM query patterns withschemafrompostgress_schema.tsfor all database operations; usealiasV2()for self-joins or multiple table references
Files:
supabase/functions/_backend/plugins/stats.ts
supabase/functions/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend ESLint must pass before commit; run
bun lint:backendfor backend files
Files:
supabase/functions/_backend/plugins/stats.ts
**/*.{vue,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintto lint Vue, TypeScript, and JavaScript files; usebun lint:fixto auto-fix issues
Files:
supabase/functions/_backend/plugins/stats.tstests/stats.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use single quotes and no semicolons per
@antfu/eslint-configrules
Files:
supabase/functions/_backend/plugins/stats.tstests/stats.test.ts
supabase/functions/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
supabase/functions/**/*.ts: Never use the Supabase admin SDK with service key for user-facing APIs; always use client SDK with user authentication to enforce RLS policies. Admin SDK should only be used for internal operations (triggers, CRON jobs, etc.)
When admin access is unavoidable for a user-facing endpoint, sanitize all user inputs carefully to prevent PostgREST query injection
Files:
supabase/functions/_backend/plugins/stats.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintor lint/format command before validating any backend or frontend task to ensure consistent formatting
Files:
supabase/functions/_backend/plugins/stats.tstests/stats.test.ts
supabase/functions/_backend/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Files:
supabase/functions/_backend/plugins/stats.ts
tests/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend tests must use helpers from
tests/test-utils.tsincludinggetEndpointUrl(path)for correct worker routing andUSE_CLOUDFLARE_WORKERS=truefor CF Workers testing
Files:
tests/stats.test.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.test.ts: ALL TEST FILES RUN IN PARALLEL - design tests accordingly with proper isolation and useit.concurrent()to maximize parallelism within the same file
Create dedicated seed data for tests that modify shared resources instead of reusing existing data; use unique naming conventions prefixed with test file name or feature being tested
Only reuse existing seed data if you only READ the data without modifying it, or create your OWN child resources under it without modifying the parent
Backend tests located in tests/ directory use Vitest test runner and require running Supabase instance
Files:
tests/stats.test.ts
🧠 Learnings (8)
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/functions/_backend/**/*.ts : Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `createHono` from `utils/hono.ts` for all Hono framework application initialization and routing
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/plugin/index.ts : Plugin Worker (port 8788) routes: `/updates`, `/channel_self`, `/stats`
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/api/index.ts : API Worker (port 8787) routes: `/bundle`, `/app`, `/device`, `/channel`, `/private/*`, `/triggers`
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/seed.sql : Update supabase/seed.sql for new or evolved tests; keep fixtures focused on current behavior while leaving committed migrations unchanged
Applied to files:
tests/stats.test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to tests/**/*.test.ts : Create dedicated seed data for tests that modify shared resources instead of reusing existing data; use unique naming conventions prefixed with test file name or feature being tested
Applied to files:
tests/stats.test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to tests/**/*.test.ts : ALL TEST FILES RUN IN PARALLEL - design tests accordingly with proper isolation and use `it.concurrent()` to maximize parallelism within the same file
Applied to files:
tests/stats.test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to tests/**/*.test.ts : Backend tests located in tests/ directory use Vitest test runner and require running Supabase instance
Applied to files:
tests/stats.test.ts
🧬 Code graph analysis (1)
tests/stats.test.ts (2)
tests/test-utils.ts (4)
APP_NAME_STATS(53-53)getVersionFromAction(196-199)createAppVersions(201-217)getSupabaseClient(388-407)supabase/functions/_backend/utils/version.ts (1)
version(1-1)
🔇 Additional comments (6)
supabase/functions/_backend/plugins/stats.ts (5)
9-9: Import update looks fine.
23-28: Clear result typing for batch and post flows.These interfaces make the new response shapes and internal flow easier to reason about.
Also applies to: 61-66
68-99: PostResult wiring is consistent.Early returns now map cleanly to the caller without losing error detail.
Also applies to: 120-120, 156-156
161-180: Good normalization ofdevice_idfor single and batch payloads.
187-190: Batch loop and empty-batch short‑circuit look solid.Per-item try/catch preserves partial success while keeping the response shape stable.
Also applies to: 208-242, 252-254
tests/stats.test.ts (1)
374-598: Batch test coverage and conditional concurrency look good.Nice validation of partial failures, empty batches, and backward-compatible single-event responses.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // For single event, return simple response for backward compatibility | ||
| if (!isBatch) { | ||
| const result = results[0] | ||
| if (result.status === 'ok') { | ||
| return c.json(BRES) | ||
| } | ||
| return simpleError200(c, result.error!, result.message!) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "simpleError200" supabase/functions/_backend/utils/hono.tsRepository: Cap-go/capgo
Length of output: 160
🏁 Script executed:
sed -n '213,240p' supabase/functions/_backend/utils/hono.tsRepository: Cap-go/capgo
Length of output: 943
Use appropriate error status codes for single-event validation errors.
The simpleError200 helper function hardcodes HTTP 200 for all errors. Using it for non-batch validation errors will return 200 instead of the likely 400 status codes these errors previously returned, breaking backward compatibility (e.g., existing invalid-action tests). Either confirm this status change is intentional, or use quickError (which accepts dynamic status codes) for single-event validation failures.
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/plugins/stats.ts` around lines 243 - 249, The
single-event path currently uses simpleError200 which forces HTTP 200 for
validation failures; replace that call with quickError so the response can carry
the proper status code. In the block handling !isBatch (where results[0] is
inspected), call quickError(c, result.error!, result.message!, result.statusCode
?? result.code ?? 400) instead of simpleError200 so validation errors return
their intended HTTP status (falling back to 400 if the result has no explicit
status field); keep the existing c.json(BRES) for the 'ok' case.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/plugins/stats.ts`:
- Around line 161-175: parseBodyRaw currently calls toLowerCase() on any truthy
device_id which throws for non-strings; update the normalization so you only
call toLowerCase when typeof device_id === "string" (both inside the
Array.isArray branch iterating items and in the single-body branch), leaving
non-string values untouched so schema validation can catch them; reference the
parseBodyRaw function and the AppStats.device_id field when making the change.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supabase/functions/_backend/plugins/stats.ts
🧰 Additional context used
📓 Path-based instructions (7)
supabase/functions/_backend/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
supabase/functions/_backend/**/*.{ts,js}: Backend code must be placed insupabase/functions/_backend/as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
UsecreateHonofromutils/hono.tsfor all Hono framework application initialization and routing
All database operations must usegetPgClient()orgetDrizzleClient()fromutils/pg.tsfor PostgreSQL access during active migration to Cloudflare D1
All Hono endpoint handlers must acceptContext<MiddlewareKeyVariables>and usec.get('requestId'),c.get('apikey'), andc.get('auth')for request context
Use structured logging withcloudlog({ requestId: c.get('requestId'), message: '...' })for all backend logging
UsemiddlewareAPISecretfor internal API endpoints andmiddlewareKeyfor external API keys; validate againstowner_orgin theapikeystable
Checkc.get('auth')?.authTypeto determine authentication type ('apikey' vs 'jwt') in backend endpoints
Use Drizzle ORM query patterns withschemafrompostgress_schema.tsfor all database operations; usealiasV2()for self-joins or multiple table references
Files:
supabase/functions/_backend/plugins/stats.ts
supabase/functions/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend ESLint must pass before commit; run
bun lint:backendfor backend files
Files:
supabase/functions/_backend/plugins/stats.ts
**/*.{vue,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintto lint Vue, TypeScript, and JavaScript files; usebun lint:fixto auto-fix issues
Files:
supabase/functions/_backend/plugins/stats.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use single quotes and no semicolons per
@antfu/eslint-configrules
Files:
supabase/functions/_backend/plugins/stats.ts
supabase/functions/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
supabase/functions/**/*.ts: Never use the Supabase admin SDK with service key for user-facing APIs; always use client SDK with user authentication to enforce RLS policies. Admin SDK should only be used for internal operations (triggers, CRON jobs, etc.)
When admin access is unavoidable for a user-facing endpoint, sanitize all user inputs carefully to prevent PostgREST query injection
Files:
supabase/functions/_backend/plugins/stats.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintor lint/format command before validating any backend or frontend task to ensure consistent formatting
Files:
supabase/functions/_backend/plugins/stats.ts
supabase/functions/_backend/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Files:
supabase/functions/_backend/plugins/stats.ts
🧠 Learnings (5)
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/functions/_backend/**/*.ts : Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/plugin/index.ts : Plugin Worker (port 8788) routes: `/updates`, `/channel_self`, `/stats`
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Check `c.get('auth')?.authType` to determine authentication type ('apikey' vs 'jwt') in backend endpoints
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `createHono` from `utils/hono.ts` for all Hono framework application initialization and routing
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/api/index.ts : API Worker (port 8787) routes: `/bundle`, `/app`, `/device`, `/channel`, `/private/*`, `/triggers`
Applied to files:
supabase/functions/_backend/plugins/stats.ts
🧬 Code graph analysis (1)
supabase/functions/_backend/plugins/stats.ts (3)
supabase/functions/_backend/utils/types.ts (1)
AppStats(23-26)supabase/functions/_backend/utils/plugin_parser.ts (1)
parsePluginBody(36-65)supabase/functions/_backend/utils/hono.ts (5)
MiddlewareKeyVariables(27-41)simpleError(247-249)simpleError200(213-222)simpleRateLimit(239-245)BRES(135-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
🔇 Additional comments (3)
supabase/functions/_backend/plugins/stats.ts (3)
9-9: Batch response typing looks solid.The BatchStatsResult shape (plus the added helper import) makes the batch contract explicit and easier to consume.
Also applies to: 23-28
61-157: Structured PostResult flow is clear.Returning a unified PostResult keeps the single vs. batch code paths consistent and readable.
182-268: Batch flow and early app_id checks look good.The empty-batch short‑circuit and per-item aggregation align well with the intended batch behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
riderx
force-pushed
the
riderx/batch-stats-endpoint
branch
2 times, most recently
from
31fc702 to
9824d3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/plugins/stats.ts`:
- Around line 221-248: The handler currently ignores the isOnprem flag returned
by post(); before treating a post() success as OK, check result.isOnprem and for
single requests call onPremStats(c) (preserving the 429 behavior) instead of
returning BRES, and for batch requests mark that event's BatchStatsResult as an
error (status: 'error', include result.error and result.message or a canonical
on‑prem message) rather than pushing {status:'ok'}. Adjust the post() success
branch in both the single-path (where parsePluginBody/post/BRES are used) and
inside the batch loop (where results: BatchStatsResult[] is populated) to handle
isOnprem explicitly.
♻️ Duplicate comments (1)
supabase/functions/_backend/plugins/stats.ts (1)
165-174: Guard device_id normalization to strings only.Calling
toLowerCase()on non-strings throws and turns schema errors intoinvalid_json_parse_body. Please gate bytypeof === 'string'for both single and batch items.🔧 Proposed fix
if (Array.isArray(body)) { for (const item of body) { - if (item.device_id) { - item.device_id = item.device_id.toLowerCase() - } + if (typeof item.device_id === 'string') { + item.device_id = item.device_id.toLowerCase() + } } } - else if (body.device_id) { + else if (typeof body.device_id === 'string') { body.device_id = body.device_id.toLowerCase() }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supabase/functions/_backend/plugins/stats.tstests/stats.test.ts
🧰 Additional context used
📓 Path-based instructions (9)
tests/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend tests must use helpers from
tests/test-utils.tsincludinggetEndpointUrl(path)for correct worker routing andUSE_CLOUDFLARE_WORKERS=truefor CF Workers testing
Files:
tests/stats.test.ts
**/*.{vue,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintto lint Vue, TypeScript, and JavaScript files; usebun lint:fixto auto-fix issues
Files:
tests/stats.test.tssupabase/functions/_backend/plugins/stats.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use single quotes and no semicolons per
@antfu/eslint-configrules
Files:
tests/stats.test.tssupabase/functions/_backend/plugins/stats.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.test.ts: ALL TEST FILES RUN IN PARALLEL - design tests accordingly with proper isolation and useit.concurrent()to maximize parallelism within the same file
Create dedicated seed data for tests that modify shared resources instead of reusing existing data; use unique naming conventions prefixed with test file name or feature being tested
Only reuse existing seed data if you only READ the data without modifying it, or create your OWN child resources under it without modifying the parent
Backend tests located in tests/ directory use Vitest test runner and require running Supabase instance
Files:
tests/stats.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintor lint/format command before validating any backend or frontend task to ensure consistent formatting
Files:
tests/stats.test.tssupabase/functions/_backend/plugins/stats.ts
supabase/functions/_backend/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
supabase/functions/_backend/**/*.{ts,js}: Backend code must be placed insupabase/functions/_backend/as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
UsecreateHonofromutils/hono.tsfor all Hono framework application initialization and routing
All database operations must usegetPgClient()orgetDrizzleClient()fromutils/pg.tsfor PostgreSQL access during active migration to Cloudflare D1
All Hono endpoint handlers must acceptContext<MiddlewareKeyVariables>and usec.get('requestId'),c.get('apikey'), andc.get('auth')for request context
Use structured logging withcloudlog({ requestId: c.get('requestId'), message: '...' })for all backend logging
UsemiddlewareAPISecretfor internal API endpoints andmiddlewareKeyfor external API keys; validate againstowner_orgin theapikeystable
Checkc.get('auth')?.authTypeto determine authentication type ('apikey' vs 'jwt') in backend endpoints
Use Drizzle ORM query patterns withschemafrompostgress_schema.tsfor all database operations; usealiasV2()for self-joins or multiple table references
Files:
supabase/functions/_backend/plugins/stats.ts
supabase/functions/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend ESLint must pass before commit; run
bun lint:backendfor backend files
Files:
supabase/functions/_backend/plugins/stats.ts
supabase/functions/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
supabase/functions/**/*.ts: Never use the Supabase admin SDK with service key for user-facing APIs; always use client SDK with user authentication to enforce RLS policies. Admin SDK should only be used for internal operations (triggers, CRON jobs, etc.)
When admin access is unavoidable for a user-facing endpoint, sanitize all user inputs carefully to prevent PostgREST query injection
Files:
supabase/functions/_backend/plugins/stats.ts
supabase/functions/_backend/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Files:
supabase/functions/_backend/plugins/stats.ts
🧠 Learnings (8)
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/seed.sql : Update supabase/seed.sql for new or evolved tests; keep fixtures focused on current behavior while leaving committed migrations unchanged
Applied to files:
tests/stats.test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to tests/**/*.test.ts : Create dedicated seed data for tests that modify shared resources instead of reusing existing data; use unique naming conventions prefixed with test file name or feature being tested
Applied to files:
tests/stats.test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to tests/**/*.test.ts : ALL TEST FILES RUN IN PARALLEL - design tests accordingly with proper isolation and use `it.concurrent()` to maximize parallelism within the same file
Applied to files:
tests/stats.test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/functions/_backend/**/*.ts : Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/plugin/index.ts : Plugin Worker (port 8788) routes: `/updates`, `/channel_self`, `/stats`
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Check `c.get('auth')?.authType` to determine authentication type ('apikey' vs 'jwt') in backend endpoints
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `createHono` from `utils/hono.ts` for all Hono framework application initialization and routing
Applied to files:
supabase/functions/_backend/plugins/stats.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/api/index.ts : API Worker (port 8787) routes: `/bundle`, `/app`, `/device`, `/channel`, `/private/*`, `/triggers`
Applied to files:
supabase/functions/_backend/plugins/stats.ts
🧬 Code graph analysis (1)
supabase/functions/_backend/plugins/stats.ts (9)
supabase/functions/_backend/utils/pg.ts (2)
getDrizzleClient(220-222)getPgClient(187-218)supabase/functions/_backend/utils/types.ts (1)
AppStats(23-26)supabase/functions/_backend/utils/plugin_parser.ts (2)
makeDevice(17-34)parsePluginBody(36-65)supabase/functions/_backend/utils/appStatus.ts (2)
getAppStatus(20-28)setAppStatus(30-37)supabase/functions/_backend/utils/stats.ts (1)
onPremStats(19-41)supabase/functions/_backend/triggers/on_version_update.ts (1)
app(279-279)supabase/functions/_backend/triggers/on_app_create.ts (1)
app(12-12)supabase/functions/_backend/triggers/cron_email.ts (1)
app(93-93)supabase/functions/_backend/utils/hono.ts (5)
MiddlewareKeyVariables(27-41)simpleError(247-249)simpleError200(213-222)simpleRateLimit(239-245)BRES(135-135)
🪛 GitHub Actions: Run tests
tests/stats.test.ts
[error] 226-226: Test failed during 'bun run test:all'. AssertionError: expected 200 to be 429.
🔇 Additional comments (3)
supabase/functions/_backend/plugins/stats.ts (2)
9-9: Import-only change.
23-28: BatchStatsResult looks good.tests/stats.test.ts (1)
374-597: Batch test coverage and CF gating look solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
This PR enables the stats endpoint to accept both single events and arrays of events in a single request. Single events maintain backward compatibility with simple { status: 'ok' } responses, while batch requests return detailed per-event results including status and error details. The implementation validates that all events in a batch share the same app_id to prevent rate limit bypass, handles partial failures gracefully by continuing to process remaining events, and returns early for empty batches.
Changes:
- Refactored the
postfunction to return aPostResultobject instead of directly returning HTTP responses - Added
parseBodyRawfunction to handle both single and array payloads with device_id normalization - Implemented batch processing logic that collects per-event results and handles errors individually
- Added comprehensive test coverage for batch operations including success, partial failures, empty batches, and backward compatibility
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
supabase/functions/_backend/plugins/stats.ts |
Refactored stats endpoint to support batch processing while maintaining backward compatibility; added validation for same app_id across batch events |
tests/stats.test.ts |
Added comprehensive test suite for batch operations covering success cases, partial failures, empty batches, mixed app_ids, and backward compatibility |
supabase/functions/_backend/utils/supabase.ts |
Modified expiration date validation to accept dates equal to current time (changed from <= to <) |
| await onPremStats(c, app_id, action, device) | ||
| return { success: true, isOnprem: true } |
There was a problem hiding this comment.
The onPremStats function returns a Response object with status 429, but this return value is being ignored. In the original code, the response was returned to the client. Now the function is awaited and its return value is discarded, while a success result is returned instead. This means batch operations with onprem apps will incorrectly report success status instead of the appropriate 429 error that onPremStats is designed to return.
| await onPremStats(c, app_id, action, device) | ||
| return { success: true, isOnprem: true } |
There was a problem hiding this comment.
The onPremStats function returns a Response object with status 429, but this return value is being ignored. In the original code, the response was returned to the client. Now the function is awaited and its return value is discarded, while a success result is returned instead. This means batch operations with onprem apps will incorrectly report success status instead of the appropriate 429 error that onPremStats is designed to return.
| // Validate all events in batch have the same app_id to prevent rate limit bypass | ||
| if (isBatch) { | ||
| for (let i = 1; i < events.length; i++) { | ||
| if (events[i].app_id !== firstAppId) { |
There was a problem hiding this comment.
The batch validation check only validates that events[i].app_id exists and is a string, but doesn't validate it against the same regex pattern (reverseDomainRegex) that's used for the first event. This could allow invalid app_ids to pass validation for events after the first one in a batch.
| if (events[i].app_id !== firstAppId) { | |
| const currentAppId = events[i].app_id | |
| // Ensure app_id exists and is a valid reverse-domain string for every event | |
| if (!currentAppId || typeof currentAppId !== 'string') { | |
| throw simpleError('invalid_app_id', MISSING_STRING_APP_ID) | |
| } | |
| if (!reverseDomainRegex.test(currentAppId)) { | |
| throw simpleError('invalid_app_id', INVALID_STRING_APP_ID) | |
| } | |
| if (currentAppId !== firstAppId) { |
| async function parseBodyRaw(c: Context): Promise<AppStats | AppStats[]> { | ||
| try { | ||
| const body = await c.req.json<AppStats | AppStats[]>() | ||
| // Normalize device_id to lowercase for both single and array | ||
| if (Array.isArray(body)) { | ||
| for (const item of body) { | ||
| if (item.device_id) { | ||
| item.device_id = item.device_id.toLowerCase() | ||
| } | ||
| } | ||
| } | ||
| else if (body.device_id) { | ||
| body.device_id = body.device_id.toLowerCase() | ||
| } | ||
| return body | ||
| } | ||
| catch (e) { | ||
| throw simpleError('invalid_json_parse_body', 'Invalid JSON body', { e }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The custom parseBodyRaw function duplicates logic from the existing parseBody utility function (which also normalizes device_id to lowercase). This creates maintenance burden and potential for inconsistencies. Consider extending or reusing the existing parseBody function to handle arrays instead of creating a separate implementation.
| // Validate all events in batch have the same app_id to prevent rate limit bypass | ||
| if (isBatch) { | ||
| for (let i = 1; i < events.length; i++) { | ||
| if (events[i].app_id !== firstAppId) { |
There was a problem hiding this comment.
The validation logic checks if events[i].app_id is defined and is a string type, but this check is redundant since the next line directly compares events[i].app_id !== firstAppId. If events[i].app_id is undefined or not a string, the comparison would still work (undefined !== 'somestring' is true). The validation should either be removed as redundant, or if null/undefined app_ids need special handling, that should be explicit.
| if (events[i].app_id !== firstAppId) { | |
| const currentAppId = events[i].app_id | |
| // Ensure each event has a valid string app_id in reverse-domain format | |
| if (!currentAppId || typeof currentAppId !== 'string') { | |
| return simpleError200(c, 'invalid_app_id', MISSING_STRING_APP_ID) | |
| } | |
| if (!reverseDomainRegex.test(currentAppId)) { | |
| return simpleError200(c, 'invalid_app_id', INVALID_STRING_APP_ID) | |
| } | |
| if (currentAppId !== firstAppId) { |
| throw simpleError('invalid_expiration_date', 'Invalid expiration date format') | ||
| } | ||
| if (expirationDate <= new Date()) { | ||
| if (expirationDate < new Date()) { |
There was a problem hiding this comment.
This change modifies the expiration date validation logic to accept dates exactly equal to the current time, whereas previously they were rejected. While this might be intentional, it appears unrelated to the PR's stated purpose of adding batch stats endpoint support. Consider whether this change should be in a separate PR, especially since it affects API key management functionality across the codebase.
| if (expirationDate < new Date()) { | |
| const now = new Date() | |
| if (expirationDate <= now) { |
| batchTestDescribe('[POST] /stats batch operations', () => { | ||
| batchTestIt('should handle batch of events', async () => { | ||
| const uuid1 = randomUUID().toLowerCase() | ||
| const uuid2 = randomUUID().toLowerCase() | ||
|
|
||
| const baseData1 = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData1.device_id = uuid1 | ||
| baseData1.action = 'get' | ||
| baseData1.version_build = getVersionFromAction('get') | ||
| const version1 = await createAppVersions(baseData1.version_build, APP_NAME_STATS) | ||
| baseData1.version_name = version1.name | ||
|
|
||
| const baseData2 = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData2.device_id = uuid2 | ||
| baseData2.action = 'set' | ||
| baseData2.version_build = getVersionFromAction('set') | ||
| const version2 = await createAppVersions(baseData2.version_build, APP_NAME_STATS) | ||
| baseData2.version_name = version2.name | ||
|
|
||
| // Send batch request | ||
| const response = await postStats([baseData1, baseData2]) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| const responseData = await response.json<BatchStatsRes>() | ||
| expect(responseData.status).toBe('ok') | ||
| expect(responseData.results).toBeDefined() | ||
| expect(responseData.results).toHaveLength(2) | ||
| expect(responseData.results![0].status).toBe('ok') | ||
| expect(responseData.results![0].index).toBe(0) | ||
| expect(responseData.results![1].status).toBe('ok') | ||
| expect(responseData.results![1].index).toBe(1) | ||
|
|
||
| // Verify both stats entries were created | ||
| const { error: statsError1, data: statsData1 } = await getSupabaseClient() | ||
| .from('stats') | ||
| .select() | ||
| .eq('device_id', uuid1) | ||
| .eq('app_id', APP_NAME_STATS) | ||
| .eq('action', 'get') | ||
| .single() | ||
| expect(statsError1).toBeNull() | ||
| expect(statsData1).toBeTruthy() | ||
|
|
||
| const { error: statsError2, data: statsData2 } = await getSupabaseClient() | ||
| .from('stats') | ||
| .select() | ||
| .eq('device_id', uuid2) | ||
| .eq('app_id', APP_NAME_STATS) | ||
| .eq('action', 'set') | ||
| .single() | ||
| expect(statsError2).toBeNull() | ||
| expect(statsData2).toBeTruthy() | ||
|
|
||
| // Clean up | ||
| await getSupabaseClient().from('devices').delete().eq('device_id', uuid1).eq('app_id', APP_NAME_STATS) | ||
| await getSupabaseClient().from('devices').delete().eq('device_id', uuid2).eq('app_id', APP_NAME_STATS) | ||
| }) | ||
|
|
||
| batchTestIt('should handle batch with partial failures', async () => { | ||
| const uuid1 = randomUUID().toLowerCase() | ||
| const uuid2 = randomUUID().toLowerCase() | ||
|
|
||
| const baseData1 = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData1.device_id = uuid1 | ||
| baseData1.action = 'get' | ||
| baseData1.version_build = getVersionFromAction('get') | ||
| const version1 = await createAppVersions(baseData1.version_build, APP_NAME_STATS) | ||
| baseData1.version_name = version1.name | ||
|
|
||
| // Second event has invalid action | ||
| const baseData2 = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData2.device_id = uuid2 | ||
| baseData2.action = 'invalid_action_xyz' as StatsAction | ||
| baseData2.version_build = getVersionFromAction('set') | ||
|
|
||
| // Send batch request | ||
| const response = await postStats([baseData1, baseData2]) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| const responseData = await response.json<BatchStatsRes>() | ||
| expect(responseData.status).toBe('ok') | ||
| expect(responseData.results).toBeDefined() | ||
| expect(responseData.results).toHaveLength(2) | ||
|
|
||
| // First event should succeed | ||
| expect(responseData.results![0].status).toBe('ok') | ||
| expect(responseData.results![0].index).toBe(0) | ||
|
|
||
| // Second event should fail | ||
| expect(responseData.results![1].status).toBe('error') | ||
| expect(responseData.results![1].index).toBe(1) | ||
| expect(responseData.results![1].error).toBeTruthy() | ||
|
|
||
| // Verify first stats entry was created | ||
| const { error: statsError1, data: statsData1 } = await getSupabaseClient() | ||
| .from('stats') | ||
| .select() | ||
| .eq('device_id', uuid1) | ||
| .eq('app_id', APP_NAME_STATS) | ||
| .eq('action', 'get') | ||
| .single() | ||
| expect(statsError1).toBeNull() | ||
| expect(statsData1).toBeTruthy() | ||
|
|
||
| // Clean up | ||
| await getSupabaseClient().from('devices').delete().eq('device_id', uuid1).eq('app_id', APP_NAME_STATS) | ||
| }) | ||
|
|
||
| batchTestIt('should handle empty batch', async () => { | ||
| const response = await postStats([]) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| const responseData = await response.json<BatchStatsRes>() | ||
| expect(responseData.status).toBe('ok') | ||
| expect(responseData.results).toBeDefined() | ||
| expect(responseData.results).toHaveLength(0) | ||
| }) | ||
|
|
||
| batchTestIt('single event should still return simple response for backward compatibility', async () => { | ||
| const uuid = randomUUID().toLowerCase() | ||
| const baseData = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData.device_id = uuid | ||
| baseData.action = 'get' | ||
| baseData.version_build = getVersionFromAction('get') | ||
| const version = await createAppVersions(baseData.version_build, APP_NAME_STATS) | ||
| baseData.version_name = version.name | ||
|
|
||
| // Send single event (not in array) | ||
| const response = await postStats(baseData) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| const responseData = await response.json<StatsRes>() | ||
| // Should return simple { status: 'ok' } not batch format | ||
| expect(responseData.status).toBe('ok') | ||
| expect((responseData as BatchStatsRes).results).toBeUndefined() | ||
|
|
||
| // Clean up | ||
| await getSupabaseClient().from('devices').delete().eq('device_id', uuid).eq('app_id', APP_NAME_STATS) | ||
| }) | ||
|
|
||
| batchTestIt('should handle batch with same device multiple actions', async () => { | ||
| const uuid = randomUUID().toLowerCase() | ||
|
|
||
| const baseData1 = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData1.device_id = uuid | ||
| baseData1.action = 'get' | ||
| baseData1.version_build = getVersionFromAction('get') | ||
| const version1 = await createAppVersions(baseData1.version_build, APP_NAME_STATS) | ||
| baseData1.version_name = version1.name | ||
|
|
||
| const baseData2 = { ...baseData1 } | ||
| baseData2.action = 'set' | ||
| baseData2.version_build = getVersionFromAction('set') | ||
| const version2 = await createAppVersions(baseData2.version_build, APP_NAME_STATS) | ||
| baseData2.version_name = version2.name | ||
|
|
||
| // Send batch with same device, different actions | ||
| const response = await postStats([baseData1, baseData2]) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| const responseData = await response.json<BatchStatsRes>() | ||
| expect(responseData.status).toBe('ok') | ||
| expect(responseData.results).toHaveLength(2) | ||
| expect(responseData.results![0].status).toBe('ok') | ||
| expect(responseData.results![1].status).toBe('ok') | ||
|
|
||
| // Verify both stats entries were created | ||
| const { error: statsError, data: statsData } = await getSupabaseClient() | ||
| .from('stats') | ||
| .select() | ||
| .eq('device_id', uuid) | ||
| .eq('app_id', APP_NAME_STATS) | ||
| .order('created_at', { ascending: true }) | ||
| expect(statsError).toBeNull() | ||
| expect(statsData).toBeTruthy() | ||
| expect(statsData).toHaveLength(2) | ||
| expect(statsData![0].action).toBe('get') | ||
| expect(statsData![1].action).toBe('set') | ||
|
|
||
| // Clean up | ||
| await getSupabaseClient().from('devices').delete().eq('device_id', uuid).eq('app_id', APP_NAME_STATS) | ||
| }) | ||
|
|
||
| batchTestIt('should reject batch with mixed app_ids', async () => { | ||
| const uuid1 = randomUUID().toLowerCase() | ||
| const uuid2 = randomUUID().toLowerCase() | ||
|
|
||
| const baseData1 = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData1.device_id = uuid1 | ||
| baseData1.action = 'get' | ||
| baseData1.version_build = getVersionFromAction('get') | ||
| const version1 = await createAppVersions(baseData1.version_build, APP_NAME_STATS) | ||
| baseData1.version_name = version1.name | ||
|
|
||
| // Second event has different app_id | ||
| const baseData2 = getBaseData(APP_NAME_STATS) as StatsPayload | ||
| baseData2.device_id = uuid2 | ||
| baseData2.app_id = 'com.different.app' | ||
| baseData2.action = 'get' | ||
| baseData2.version_build = getVersionFromAction('get') | ||
| baseData2.version_name = version1.name | ||
|
|
||
| // Send batch request with mixed app_ids | ||
| const response = await postStats([baseData1, baseData2]) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| const responseData = await response.json<StatsRes>() | ||
| expect(responseData.error).toBe('mixed_app_ids') | ||
| expect(responseData.message).toBe('All events in a batch must have the same app_id') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The new batch functionality lacks test coverage for onprem apps in batch requests. Given that the onPremStats function is supposed to return a 429 status code (as verified in the existing single-event test "app that does not exist"), but the refactored code doesn't properly handle this return value in batch mode, this missing test coverage masks the bug where batch requests with onprem apps would incorrectly return success.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| throw simpleError('invalid_expiration_date', 'Invalid expiration date format') | ||
| } | ||
| if (expirationDate <= new Date()) { | ||
| if (expirationDate < new Date()) { |
There was a problem hiding this comment.
Expiration date validation contradicts error message
Low Severity
The condition was changed from <= to <, which now allows expiration dates that are exactly equal to the current time. However, the error message still says "Expiration date must be in the future". A date that equals "now" is not "in the future", making the error message inconsistent with the actual validation logic. This could allow creating API keys with expiration times that are essentially immediate, which is likely not the intended behavior.
riderx
force-pushed
the
riderx/batch-stats-endpoint
branch
from
2a4b92c to
1c47507
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
riderx
force-pushed
the
riderx/batch-stats-endpoint
branch
from
b7a818a to
87083ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/plugins/stats.ts`:
- Around line 163-176: In parseBodyRaw, avoid accessing item.device_id on
non-object array elements—update the array normalization loop to first check
that each item is a non-null object (e.g., item && typeof item === 'object')
before testing item.device_id and lowercasing; do the same protective check for
the single-body branch (ensure body is a non-null object) so primitives/null in
the payload don't cause a thrown exception and allow per-item validation to
report errors instead.
- Around line 194-218: The code currently assumes events entries are objects and
does events[i].app_id directly; add guards to ensure each batch entry is a
non-null object before accessing app_id (e.g., check typeof events[i] ===
'object' && events[i] !== null) both for the initial firstEvent handling and
inside the isBatch loop, and if the check fails return the same structured
validation response via simpleError200(c, 'invalid_app_id',
MISSING_STRING_APP_ID) (keep using reverseDomainRegex for format validation and
preserve the current mixed_app_ids check against firstAppId).
Enable stats endpoint to accept both single events and arrays of events, allowing clients to send multiple events in a single request. Single events maintain backward compatibility with simple { status: 'ok' } responses, while batch requests return detailed results per event including status and error details.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Validate all events in batch have same app_id to prevent rate limit bypass - Update batch tests to use it.concurrent for parallel execution per AGENTS.md - Add test for mixed app_id rejection Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Validate first event's app_id early before batch/rate-limit checks - Fix single-event error handling to propagate proper 400 status codes Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Guard device_id normalization to only call toLowerCase on strings - Handle isOnprem flag properly and return 429 for single requests, error status in batch results for on-premise apps - Add regex validation for all batch event app_ids, not just the first - Add test coverage for onprem apps in batch requests - Revert unrelated supabase.ts expiration date validation change Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add moreInfo field to PostResult and BatchStatsResult interfaces to pass debugging information (app_id, version_name) through to error responses, maintaining backward compatibility. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add object type checks before accessing device_id in parseBodyRaw - Use optional chaining for app_id access to handle null/primitive items - Allows per-item validation errors instead of throwing on malformed input Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
riderx
force-pushed
the
riderx/batch-stats-endpoint
branch
from
9c25a80 to
dd8fda8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/plugins/stats.ts`:
- Around line 230-231: getPgClient(c, true) may return null but the code calls
getDrizzleClient(pgClient!) unconditionally; add an explicit null check on the
pgClient variable after the call (check pgClient === null or falsy) and handle
the failure path before calling getDrizzleClient: e.g., log/return an error
response or throw so getDrizzleClient is never invoked with a null value; update
the code around the pgClient/getDrizzleClient usage to only create drizzleClient
when pgClient is valid and ensure subsequent logic accounts for the
early-return/failure branch.
In `@tests/stats.test.ts`:
- Around line 560-565: The test is flaky because it assumes deterministic
ordering by created_at; update the assertion to avoid relying solely on
timestamps by either adding a secondary sort on a stable column (e.g., include
.order('index', { ascending: true }) or similar) in the query that returns
statsData or, better, map results using the batch/insert response index (use the
batch response's index values to correlate each stats row to its original
operation) and assert actions by that mapped index instead of positional order;
adjust the expectations that reference statsData[0]/statsData[1] to reference
rows by their resolved index or by the deterministic sort key so the test
becomes stable.
| const pgClient = getPgClient(c, true) | ||
| const drizzleClient = getDrizzleClient(pgClient!) |
There was a problem hiding this comment.
Guard against null pgClient before creating Drizzle client.
getPgClient(c, true) can return null on failure, but line 231 passes it to getDrizzleClient with a non-null assertion. If getPgClient fails, this will cause a runtime error. Add an explicit null check.
🔧 Proposed fix
const pgClient = getPgClient(c, true)
+ if (!pgClient) {
+ throw simpleError('db_connection_error', 'Failed to establish database connection')
+ }
- const drizzleClient = getDrizzleClient(pgClient!)
+ const drizzleClient = getDrizzleClient(pgClient)🤖 Prompt for AI Agents
In `@supabase/functions/_backend/plugins/stats.ts` around lines 230 - 231,
getPgClient(c, true) may return null but the code calls
getDrizzleClient(pgClient!) unconditionally; add an explicit null check on the
pgClient variable after the call (check pgClient === null or falsy) and handle
the failure path before calling getDrizzleClient: e.g., log/return an error
response or throw so getDrizzleClient is never invoked with a null value; update
the code around the pgClient/getDrizzleClient usage to only create drizzleClient
when pgClient is valid and ensure subsequent logic accounts for the
early-return/failure branch.
| .order('created_at', { ascending: true }) | ||
| expect(statsError).toBeNull() | ||
| expect(statsData).toBeTruthy() | ||
| expect(statsData).toHaveLength(2) | ||
| expect(statsData![0].action).toBe('get') | ||
| expect(statsData![1].action).toBe('set') |
There was a problem hiding this comment.
Potential test flakiness due to timestamp ordering.
The assertion relies on created_at ordering to verify action sequence ('get' then 'set'). If the database timestamp precision is low (e.g., seconds) and both stats entries are inserted within the same time unit, the order may be non-deterministic, causing intermittent test failures.
Consider adding a secondary sort key or using index from the batch response to correlate results instead of relying on timestamp ordering.
Suggested fix using explicit index tracking
// Verify both stats entries were created
const { error: statsError, data: statsData } = await getSupabaseClient()
.from('stats')
.select()
.eq('device_id', uuid)
.eq('app_id', APP_NAME_STATS)
- .order('created_at', { ascending: true })
expect(statsError).toBeNull()
expect(statsData).toBeTruthy()
expect(statsData).toHaveLength(2)
- expect(statsData![0].action).toBe('get')
- expect(statsData![1].action).toBe('set')
+ // Verify both actions exist without relying on order
+ const actions = statsData!.map(s => s.action).sort()
+ expect(actions).toEqual(['get', 'set'])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .order('created_at', { ascending: true }) | |
| expect(statsError).toBeNull() | |
| expect(statsData).toBeTruthy() | |
| expect(statsData).toHaveLength(2) | |
| expect(statsData![0].action).toBe('get') | |
| expect(statsData![1].action).toBe('set') | |
| expect(statsError).toBeNull() | |
| expect(statsData).toBeTruthy() | |
| expect(statsData).toHaveLength(2) | |
| // Verify both actions exist without relying on order | |
| const actions = statsData!.map(s => s.action).sort() | |
| expect(actions).toEqual(['get', 'set']) |
🤖 Prompt for AI Agents
In `@tests/stats.test.ts` around lines 560 - 565, The test is flaky because it
assumes deterministic ordering by created_at; update the assertion to avoid
relying solely on timestamps by either adding a secondary sort on a stable
column (e.g., include .order('index', { ascending: true }) or similar) in the
query that returns statsData or, better, map results using the batch/insert
response index (use the batch response's index values to correlate each stats
row to its original operation) and assert actions by that mapped index instead
of positional order; adjust the expectations that reference
statsData[0]/statsData[1] to reference rows by their resolved index or by the
deterministic sort key so the test becomes stable.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| } | ||
| if (!reverseDomainRegex.test(firstAppId)) { | ||
| throw simpleError('invalid_app_id', INVALID_STRING_APP_ID) | ||
| } |
There was a problem hiding this comment.
Inconsistent HTTP status for batch app_id validation errors
Medium Severity
Batch requests with the same validation error return different HTTP status codes depending on position. If the first event has an invalid app_id, simpleError is thrown (returning 400). If a later event has an invalid app_id, simpleError200 is returned (returning 200 with error body). This inconsistency means clients cannot reliably determine the error type from the status code, and the same malformed data produces different response formats based on array position.
Summary
Enable the stats endpoint to accept both single events and arrays of events in a single request. Single events maintain backward compatibility with
{ status: 'ok' }responses, while batch requests return detailed per-event results including status and error details for each event.Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Adds batch processing to
POST /statswith per-item results while preserving single-event responses.{ status: 'ok', results: [...] }with per-itemok/error,index, and messages; empty arrays returnresults: []device_idto lowercase; validatesapp_idformat and enforces a singleapp_idper batch; per-app rate limiting{ status: 'ok' }app_ids rejection, on-prem apps, and same-device multi-action batchesWritten by Cursor Bugbot for commit dd8fda8. This will update automatically on new commits. Configure here.