[#1238] Add twitterapi.io X user lookup with LRU cache#1276
Conversation
twitterapi.io wrapper for X user lookups: returns display name, avatar, follower count, user ID, and bio snippet. Includes 5-min in-memory LRU cache (200 entries max), 404→null, 5xx→throw. 8 unit tests cover success, 404, 5xx, cache hits, case normalization, missing API key, and bio truncation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
realproject7
left a comment
There was a problem hiding this comment.
@re2 review — APPROVE ✅
Checked against issue #1238 acceptance criteria:
| Criterion | Status |
|---|---|
Successful lookup returns { display_name, avatar_url, follower_count, x_user_id, bio_snippet } |
✅ |
| 404 → returns null | ✅ (also cached) |
| 5xx → throws | ✅ (NOT cached, so retries work) |
| LRU cache hit on second call within 5 min | ✅ |
| No API call made if cache hit | ✅ (test verifies fetch called once) |
| Version bump 1.30.2 → 1.30.3 (patch) | ✅ |
Code review notes:
- Security:
encodeURIComponent()on username in URL path prevents injection. API key in header, not URL param. Bio truncated to 200 chars. - Cache: 200-entry max with TTL-based eviction + FIFO fallback. Case-normalized keys (X handles are case-insensitive). Null results cached (prevents repeated 404 lookups). 5xx errors correctly NOT cached to allow retries.
- Error handling: Missing API key → throw early. Missing/null
datain response → null (graceful). 5xx → throw for R16 degradation. - Tests: 8 tests cover all paths including cache behavior, case normalization, bio truncation, and missing env var.
_clearCache()exposed for test cleanup — good practice.
No issues found.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The twitterapi.io wrapper covers the basic lookup, 404/null, 5xx throw, env key, and cache-hit behavior. However, #1238 explicitly calls for a 5-minute in-memory LRU cache, and the current cache is FIFO under capacity pressure rather than LRU.
Findings
- [medium] Cache hits do not refresh recency, so the max-size eviction is insertion-order FIFO, not LRU. A hot username inserted early will still be evicted once 200 newer usernames are cached, even if it was just accessed.
- File:
lib/airdrop/twitterapi.ts:26 - Suggestion: On a valid cache hit, delete and reinsert the entry (or otherwise move it to the most-recent position), and add a focused test that fills the cache, hits an older key, inserts one more key, and verifies the recently-hit key is retained.
- File:
Decision
Request changes. lint-and-typecheck is passing; e2e was still pending at review time.
Move hit entries to most-recent position via delete+re-set on Map. Add test verifying a recently-hit old key survives eviction while the least-recently-used key (filler_0) gets evicted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The requested LRU fix is in place: cache hits now refresh Map recency, and the added test verifies a recently-hit old key survives eviction while the true least-recently-used entry is evicted. The helper also satisfies #1238 lookup behavior for success, 404/null, 5xx throw, env key handling, case normalization, and no fetch on cache hit.
Findings
- No blocking findings.
Decision
Approve. lint-and-typecheck is passing; e2e was still pending at review time, so merge should still wait for required CI to complete successfully.
Summary
lib/airdrop/twitterapi.tswithlookupXUser(username)— wraps twitterapi.ioGET /v2/users/by/username/{username}{ display_name, avatar_url, follower_count, x_user_id, bio_snippet }ornullfor 404Version
1.30.2 → 1.30.3
🤖 Generated with Claude Code