Respect FACEIT developer usage: fewer API calls, bounded cache, clearer errors - #34
Conversation
…errors - One Data API call per player resolve (lowercase nickname) instead of parallel variant fan-out to reduce rate-limit risk. - Structured FaceitApiError with Twitch-friendly 200 responses for 429/5xx; NoCS2DataError instead of string-matching CS2 messages. - LRU-bounded response cache (CACHE_MAX_ENTRIES) with TTL eviction on read. - Match stats fetched with limited concurrency; 404 stats return null; other API errors propagate. Removed unused third-party FLS fetch. - Streak skips unknown team as ?; HS% from field or Headshots/Kills fallback; winrate uses matches where the player appears on a team. Co-authored-by: Matheus Marinho <m9tzin@users.noreply.github.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR introduces improved cache management with size limits and LRU eviction, adds specialized error classes for better error handling, refactors faceitService with bounded concurrency for match stats fetching and enhanced stats computation, updates routes to use the new cache API, and establishes test infrastructure with new test files for cache and match streak functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reproduced: npm test always exited 1 ('no test specified'). Add cache LRU/TTL
and processMatchStreak regression tests; run npm test in CI before start check.
Co-authored-by: Matheus Marinho <m9tzin@users.noreply.github.com>
…IGTERM The previous script used 'timeout ... || code=0' so success paths never set code; combined with Node exiting 0 after SIGTERM, CI could fail incorrectly. Always capture timeout's exit status and treat 0 and 124 as success. Co-authored-by: Matheus Marinho <m9tzin@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/faceit-streak.test.js (1)
5-31: Consider adding a Loss case.Both branches of the
won ? 'W' : 'L'ternary should be exercised; currently onlyWand?are covered.♻️ Suggested addition
+ it('marks loss when player team does not match winner', () => { + const pid = 'me'; + const match = { + teams: { + faction1: { players: [{ player_id: pid }] }, + faction2: { players: [{ player_id: 'other' }] } + }, + results: { winner: 'faction2' } + }; + const out = processMatchStreak([match], pid); + assert.match(out, /L/); + assert.doesNotMatch(out, /\?/); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/faceit-streak.test.js` around lines 5 - 31, Add a test case exercising the loss branch of processMatchStreak: create a match where the player is on one faction (e.g., faction1 with player_id 'me') and results.winner is the opposite faction (e.g., 'faction2'), call processMatchStreak([match], 'me') and assert the output contains 'L' and does not contain '?'; place this alongside the existing tests so both branches of the won ? 'W' : 'L' ternary are covered.src/utils/cache.js (1)
15-26: Defensive guard for non-numericmaxEntries.If
maxEntriesisundefined/NaN(e.g., a future caller forgets to threadconfig.cache.maxEntries, orCACHE_MAX_ENTRIESparsing yieldsNaN),this.store.size < maxEntriesevaluates tofalse, so the function will evict an LRU entry on every insertion of a new key — silently degrading the cache to a single live entry. Validate the argument before iterating.🛡️ Proposed guard
evictLruIfNeeded(maxEntries) { + if (!Number.isFinite(maxEntries) || maxEntries <= 0) return; if (this.store.size < maxEntries) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cache.js` around lines 15 - 26, The evictLruIfNeeded method should defensively validate the maxEntries argument before using it: in evictLruIfNeeded(maxEntries) check that maxEntries is a finite positive number (e.g., Number.isFinite(maxEntries) && maxEntries > 0) and return early if not, so the subsequent comparison this.store.size < maxEntries and the LRU eviction loop are skipped when callers pass undefined/NaN or invalid values; update evictLruIfNeeded to perform this guard before iterating over this.store.src/services/faceitService.js (2)
138-153: Verify behavior when a mapper rejects inmapPool.When one worker's
mapperthrows (e.g., a non-404FaceitApiErrorfromgetMatchStats),Promise.allwill reject, but other in-flight workers continue drainingnextuntil the queue empties. That's fine for correctness (no leaks) but means any subsequent rejections from sibling workers become unhandled rejections after the first failure has already settled the outer promise.If you want to fail fast and stop scheduling more work on the first error, gate
worker()on a sharedabortedflag:♻️ Optional fail-fast guard
async function mapPool(items, concurrency, mapper) { const results = new Array(items.length); let next = 0; + let aborted = false; const n = Math.max(1, Math.min(concurrency, items.length)); async function worker() { while (true) { + if (aborted) break; const idx = next++; if (idx >= items.length) break; - results[idx] = await mapper(items[idx], idx); + try { + results[idx] = await mapper(items[idx], idx); + } catch (err) { + aborted = true; + throw err; + } } } await Promise.all(Array.from({ length: n }, () => worker())); return results; }In practice
getMatchStatsonly rethrows non-404FaceitApiErrors, so this is mostly a defensive nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/faceitService.js` around lines 138 - 153, The mapPool worker loop can continue scheduling tasks after one mapper rejects, causing later rejections to become unhandled; modify mapPool to add a shared aborted flag (e.g., let aborted = false) that each worker checks before taking the next index and set aborted = true when a mapper throws, then rethrow the error so Promise.all rejects; update the inner function worker() to bail out early if aborted is true and avoid incrementing/consuming next when aborted so no further work is scheduled after the first failure (refer to mapPool, worker, next, and mapper in your changes).
309-322: Minor: deduplicateparseInt(stats['Kills'] || 0, 10).
Killsis parsed twice — once fortotalKillsand once for the HS% fallback. Folding the parses together makes intent clearer and avoids redoing the work.♻️ Proposed refactor
- totalKills += parseInt(stats['Kills'] || 0, 10); - totalDeaths += parseInt(stats['Deaths'] || 0, 10); - - const hsPctField = parseHeadshotPercentField(stats['Headshots %']); - const kills = parseInt(stats['Kills'] || 0, 10); - const headshotKills = parseInt(stats['Headshots'] || 0, 10); - let pct = hsPctField; + const kills = parseInt(stats['Kills'] || 0, 10); + const deaths = parseInt(stats['Deaths'] || 0, 10); + totalKills += kills; + totalDeaths += deaths; + + const hsPctField = parseHeadshotPercentField(stats['Headshots %']); + const headshotKills = parseInt(stats['Headshots'] || 0, 10); + let pct = hsPctField; if (pct == null && kills > 0) { pct = (headshotKills / kills) * 100; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/faceitService.js` around lines 309 - 322, The code parses stats['Kills'] twice; parse it once into a local variable (e.g., const kills = parseInt(stats['Kills'] || 0, 10)) and use that single kills value both to add to totalKills and later when computing the headshot % fallback, leaving totalDeaths parsing and parseHeadshotPercentField(stats['Headshots %']) unchanged; update references to the previous kills/local variable and remove the duplicate parse to avoid redundant work in the block handling hsPercentSum/hsPercentCount and totalKills.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 9: The package.json engines.node declaration conflicts with the test
runner (node --test requires Node 18+), so update the "engines.node" field in
package.json (look for the engines.node key) to a minimum of ">=18.0.0" (or
">=20.0.0" if you want to require the stable runner) so the local npm test, CI
matrix (18.x/20.x/22.x) and engine declaration are consistent.
---
Nitpick comments:
In `@src/services/faceitService.js`:
- Around line 138-153: The mapPool worker loop can continue scheduling tasks
after one mapper rejects, causing later rejections to become unhandled; modify
mapPool to add a shared aborted flag (e.g., let aborted = false) that each
worker checks before taking the next index and set aborted = true when a mapper
throws, then rethrow the error so Promise.all rejects; update the inner function
worker() to bail out early if aborted is true and avoid incrementing/consuming
next when aborted so no further work is scheduled after the first failure (refer
to mapPool, worker, next, and mapper in your changes).
- Around line 309-322: The code parses stats['Kills'] twice; parse it once into
a local variable (e.g., const kills = parseInt(stats['Kills'] || 0, 10)) and use
that single kills value both to add to totalKills and later when computing the
headshot % fallback, leaving totalDeaths parsing and
parseHeadshotPercentField(stats['Headshots %']) unchanged; update references to
the previous kills/local variable and remove the duplicate parse to avoid
redundant work in the block handling hsPercentSum/hsPercentCount and totalKills.
In `@src/utils/cache.js`:
- Around line 15-26: The evictLruIfNeeded method should defensively validate the
maxEntries argument before using it: in evictLruIfNeeded(maxEntries) check that
maxEntries is a finite positive number (e.g., Number.isFinite(maxEntries) &&
maxEntries > 0) and return early if not, so the subsequent comparison
this.store.size < maxEntries and the LRU eviction loop are skipped when callers
pass undefined/NaN or invalid values; update evictLruIfNeeded to perform this
guard before iterating over this.store.
In `@test/faceit-streak.test.js`:
- Around line 5-31: Add a test case exercising the loss branch of
processMatchStreak: create a match where the player is on one faction (e.g.,
faction1 with player_id 'me') and results.winner is the opposite faction (e.g.,
'faction2'), call processMatchStreak([match], 'me') and assert the output
contains 'L' and does not contain '?'; place this alongside the existing tests
so both branches of the won ? 'W' : 'L' ternary are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cb80ada-f0fd-4e97-9a57-b49a4772cd53
📒 Files selected for processing (12)
.github/workflows/ci.ymlpackage.jsonsrc/config/index.jssrc/middlewares/cache.jssrc/middlewares/errorHandler.jssrc/routes/elo.jssrc/routes/stats.jssrc/routes/streak.jssrc/services/faceitService.jssrc/utils/cache.jstest/cache.test.jstest/faceit-streak.test.js
| "start": "node src/index.js", | ||
| "dev": "node --watch src/index.js", | ||
| "test": "echo \"Error: no test specified\" && exit 1" | ||
| "test": "node --test test/" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Which Node.js version stabilized the built-in test runner (node --test)?
💡 Result:
Node.js version 20.0.0 stabilized the built-in test runner (node --test).
Citations:
- 1: https://nodejs.org/en/blog/announcements/v20-release-announce
- 2: https://nodejs.org/docs/latest-v21.x/api/test.html
- 3: nodejs/node@c733cc0
- 4: https://github.com/nodejs/node/releases/tag/v20.0.0
🏁 Script executed:
# Read package.json to see the actual engines field and context around line 24-26
cat -n package.json | head -30Repository: m9tzin/faceit-elo-bot
Length of output: 917
🏁 Script executed:
# Search for CI configuration files
find . -name "*.yml" -o -name "*.yaml" | grep -E "(\.github|\.gitlab|\.circleci|ci)" | head -20Repository: m9tzin/faceit-elo-bot
Length of output: 203
🏁 Script executed:
# Check the CI matrix in the ci.yml file
cat .github/workflows/ci.ymlRepository: m9tzin/faceit-elo-bot
Length of output: 3666
engines.node declaration does not match the test runner minimum version.
node --test requires Node 18+ (experimental) and is only stable from Node 20. The engines.node: ">=14.0.0" declaration conflicts with npm test and the CI matrix which runs on 18.x, 20.x, and 22.x. Bump to ">=18.0.0" (or ">=20.0.0" for the stable version).
Proposed change
"engines": {
- "node": ">=14.0.0"
+ "node": ">=18.0.0"
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 9, The package.json engines.node declaration conflicts
with the test runner (node --test requires Node 18+), so update the
"engines.node" field in package.json (look for the engines.node key) to a
minimum of ">=18.0.0" (or ">=20.0.0" if you want to require the stable runner)
so the local npm test, CI matrix (18.x/20.x/22.x) and engine declaration are
consistent.
… 'test' On Node 22, 'node --test test/' resolves to loading module 'test' (path /workspace/test) instead of scanning the directory, which fails CI. Shell-expand the glob so all Node versions run the same test files. Co-authored-by: Matheus Marinho <m9tzin@users.noreply.github.com>
2 participants
Summary
This change aligns the service with responsible use of the FACEIT Data API (single authenticated request per player lookup, reduced burst traffic, no non-FACEIT side channel for match data).
Changes
GET /players?nickname=per request, with nickname normalized to lowercase, instead of up to four parallel calls./matches/{id}/statswith a small concurrency cap (5) instead of 30 simultaneous calls per/statsrequest; 404 on stats returns null; other HTTP errors surface as structuredFaceitApiError.FaceitApiErrorandNoCS2DataError; removed broaderr.message.includes('CS2')handling. Twitch-facing routes still return HTTP 200 for bot compatibility, including a clear message when FACEIT returns 429.CACHE_MAX_ENTRIES(default 500); expired entries are removed on read.?instead); HS% usesHeadshots %when parseable, elseHeadshots/Kills; winrate counts only matches where the player appears on a team.fls-api.vercel.appintegration and unusedgetMatchDetailshelper.npm testpreviously always exited 1. Replaced with Node's built-innode --testsuite (test/cache.test.js,test/faceit-streak.test.js) and a CI step that runsnpm testbefore the app start check.Configuration
CACHE_MAX_ENTRIES500, minimum50).Testing
npm test(node:test)node --checkon modified modulestimeout 3s npm startwithFACEIT_KEY/PORTsetSummary by CodeRabbit
Release Notes
New Features
CACHE_MAX_ENTRIES)Improvements
Tests