Skip to content

Conversation

@zaibkhan
Copy link

@zaibkhan zaibkhan commented Sep 9, 2025

Mirrors ai-code-review-evaluation#1 for like-for-like benchmarking.

  • Base: enhance-anonymous-access
  • Head: implement-device-limits

Original PR excerpt:

Test 1

* Anonymous: Add device limiter

* break auth if limit reached

* fix typo

* refactored const to make it clearer with expiration

* anon device limit for config

---------

Co-authored-by: Eric Leijonmarck <[email protected]>
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence bot Sep 9, 2025
@codoki-pr-intelligence
Copy link

codoki-pr-intelligence bot commented Sep 9, 2025

Codoki PR Review

Summary: Add anonymous device limit, expose in runtime config
What’s good: Minimal, backward-compatible addition of a new config field for anonymous device limits.
Review Status: ❌ Requires changes
Overall Priority: High
Review Update:
• Coverage: Reviewed all 11 files across 2 batches

Issues (Critical & High only)

Severity Issue Why it matters
High Compatibility — Postgres upsert uses RETURNING with Exec (likely runtime ... …/anonstore/database.go
Using Exec with a RETURNING clause on Postgres typically fails (pq: cannot use Exec with a query that returns rows). Since the returned id is unused, drop RETURNING to avoid runtime errors on Postgres.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Ensure this field is consistently wired through frontend settings and typed interfaces so downstream consumers can rely on it.
  • Open questions: What is the expected behavior when the limit is unset vs. 0 (unlimited)? Please clarify to avoid ambiguous UI/logic.

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

newCtx, cancel := context.WithTimeout(context.Background(), timeoutTag)
defer cancel()
if err := a.anonDeviceService.TagDevice(newCtx, httpReqCopy, anonymous.AnonDeviceUI); err != nil {
a.log.Warn("Failed to tag anonymous session", "error", err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: TagDevice now runs synchronously in the auth path; a slow DB will delay authentication. Wrap the call in a short timeout to keep auth responsive while still enforcing the device limit. You'll also need to import time.

suggestion
ctxTag, cancel := context.WithTimeout(ctx, 2*time.Second)
defer cancel()
if err := a.anonDeviceService.TagDevice(ctxTag, httpReqCopy, anonymous.AnonDeviceUI); err != nil {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants