Chore/series performance fix - #49
Conversation
- Rename abbreviated series performance fields to descriptive names (`econ` → `economy`, `pl` → `plants`, `de` → `defuses`) for improved API clarity. Update corresponding documentation examples to use the new field names. - Add thread-safe HTTPCache utility that intercepts `httpx.Client.get` requests, with three modes: record (cache live responses to disk), replay (use cached responses, live fallback on miss), and live (in-memory only, no disk writes). Enables deterministic MDX example validation and reduces redundant network requests. - Update docs-check CI workflow to run MDX example checks with the `--live` flag to use fresh vlr.gg data per run. Remove persisted fixture caching in CI, as GitHub's branch/PR-scoped cache would validate against stale data from branch creation, leading to invalid results at merge time. Fixture caching remains a local development convenience. - Add `.cache/mdx-html/` to `.gitignore` to exclude local MDX fixture caches from version control. - Update official documentation example IDs for event teams and team completed matches to current valid values, ensuring examples run successfully for end users.
Added `cleanup-caches.yml` workflow that deletes docs-check caches on PR closure to avoid accumulating unused caches. Updated `docs-check.yml` to implement per-branch/PR fixture caching with a 2 hour TTL: - Main branch runs continue to validate against live `vlr.gg` data with no cache to ensure validation accuracy - Branch/PR runs reuse fresh cached fixtures when available, falling back to live fetches for missing or stale cache - Failed runs still save fresh fixtures so subsequent fix commits can reuse them and skip the warm-up step, speeding up CI iteration
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe pull request adds positional game-ID support and team-aware player mapping to series parsers. It renames advanced-stat fields, adds parser coverage, introduces fixture-backed MDX validation, manages workflow caches, and updates documentation examples. ChangesSeries API updates
MDX validation and documentation
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
| Filename | Overview |
|---|---|
| src/vlrdevapi/_series/_utils.py | Adds shared game-ID resolution used by series statistics namespaces. |
| src/vlrdevapi/_series/performance/parser.py | Adds player mapping and updates parsing for renamed advanced-stat fields. |
| src/vlrdevapi/_series/performance/models.py | Renames advanced-stat fields as an explicitly documented breaking release change. |
| src/vlrdevapi/_series/economy/parser.py | Restricts economy parsing to the selected game section. |
| src/vlrdevapi/_series/rounds/parser.py | Corrects selected-game round parsing. |
| .github/workflows/docs-check.yml | Introduces fixture caching and pins both cache actions to immutable commit revisions. |
| scripts/check_mdx_examples.py | Adds live and cached execution modes for documentation-example validation. |
Reviews (3): Last reviewed commit: "chore(release): v2.2.0 - breaking field ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| economy: int | None = Field(default=None, description="Economy rating") | ||
| plants: int | None = Field(default=None, description="Spike plants") | ||
| defuses: int | None = Field(default=None, description="Spike defuses") |
There was a problem hiding this comment.
When existing consumers access or deserialize econ, pl, or de, this model no longer exposes those established fields, causing attribute errors and changing serialized keys without a compatibility path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/vlrdevapi/_series/performance/models.py
Line: 165-167
Comment:
**Advanced-stat fields break compatibility**
When existing consumers access or deserialize `econ`, `pl`, or `de`, this model no longer exposes those established fields, causing attribute errors and changing serialized keys without a compatibility path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/vlrdevapi/_series/players/parser.py (1)
326-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the parser type contract.
parse_players_statsnow supports positional integer selectors throughresolve_game_id. Itsgame_id: strannotation still rejects that supported input during static type checking. Change the annotation toint | str.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vlrdevapi/_series/players/parser.py` at line 326, Update the game_id parameter annotation in parse_players_stats from str to int | str so its type contract matches the positional integer selectors accepted by resolve_game_id..github/workflows/docs-check.yml (2)
84-101: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFresh runs never add newly fetched pages to the cache.
Both steps require
fresh == 'false'. When the cache is fresh, the run uses--skip-warm, andHTTPCache._storewrites to disk only inrecordmode. Pages that are missing from the restored fixtures are fetched live and discarded. Every run inside the 2h TTL then repeats those live fetches.To let a partially populated cache converge, persist replay fallbacks and save the archive on every branch run. This is a throughput improvement, not a correctness fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docs-check.yml around lines 84 - 101, Update the “Package fixtures (branch)” and “Save fixture cache (branch)” steps to run on every non-main branch run, removing the cache-age fresh check. Preserve the existing archive creation and cache-save behavior so newly fetched pages persisted by HTTPCache._store can be included in the next cache.
75-82: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRead the
freshoutput fromenvhere too.
if [ "${{ steps.cache-age.outputs.fresh }}" == "true" ]expands the output into the script text. zizmor flags Line 78. The value comes from the previous step, so the risk is low, but use the sameenvpattern for consistency.🛠️ Proposed fix
- name: Run MDX examples check (branch) if: github.ref_name != 'main' + env: + FRESH: ${{ steps.cache-age.outputs.fresh }} run: | - if [ "${{ steps.cache-age.outputs.fresh }}" == "true" ]; then + if [ "$FRESH" = "true" ]; then uv run scripts/check_mdx_examples.py --skip-warm else uv run scripts/check_mdx_examples.py fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docs-check.yml around lines 75 - 82, Update the “Run MDX examples check (branch)” step to read the cache-age fresh value through an environment variable, matching the existing env-based pattern, and change the shell condition to use that variable instead of interpolating steps.cache-age.outputs.fresh directly.Source: Linters/SAST tools
.github/workflows/cleanup-caches.yml (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFork pull requests cannot delete caches.
The
pull_requestevent gives forked-PR runs a read-onlyGITHUB_TOKEN. Thepermissions: actions: writedeclaration does not raise it. For a fork PR,gh cache deletereturns HTTP 403.set +ehides the failure, so the step reports success without deleting anything.This matches the pattern in the GitHub cache documentation, so no change is required if all contributors push branches to this repository. If fork PRs are expected, switch the trigger to
pull_request_target, or run the cleanup on a schedule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cleanup-caches.yml around lines 3 - 9, Update the workflow trigger around the pull_request closed event so forked pull requests can use a token capable of deleting caches, either by switching to pull_request_target or by moving cleanup to a scheduled workflow. Preserve the actions: write permission and existing cleanup behavior..gitignore (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso ignore the fixture archive.
.github/workflows/docs-check.ymlwritesfixtures.tar.gzto the repository root. The pattern here covers only.cache/mdx-html/. Add the archive so a local reproduction of the packaging step does not stage it.🛠️ Proposed addition
.cache/mdx-html/ +fixtures.tar.gz🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore at line 12, Update the repository ignore rules alongside the .cache/mdx-html/ entry to also ignore the root-level fixtures.tar.gz archive generated by .github/workflows/docs-check.yml.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docs-check.yml:
- Around line 56-73: Update .github/workflows/docs-check.yml lines 56-73 to pass
steps.mdx-fixtures.outputs.cache-matched-key through an env entry, read it as
$MATCHED_KEY, and validate that the parsed epoch is numeric and not in the
future before calculating age. Update lines 75-82 to pass
steps.cache-age.outputs.fresh through env, read it as $FRESH, and compare it to
"true" without inline step-output interpolation.
In `@scripts/check_mdx_examples.py`:
- Around line 519-528: Update the replay flow around execute_blocks so
RateLimiter.acquire is not globally replaced with a no-op while live fallback
requests remain possible. Preserve rate limiting during fallback fetches, either
by leaving RateLimiter active or by making HTTPCache._throttle globally
serialized using cache-level throttle state initialized in HTTPCache.__init__.
In `@src/vlrdevapi/_series/performance/parser.py`:
- Around line 201-202: Update _parse_notable_rounds so each notable victim
lookup includes the victim’s team context and uses the team-specific PlayerMap
lookup rather than the name-only fallback. When the HTML lacks sufficient team
information, leave player_id as None; preserve the existing NotableVictim
construction and append flow.
---
Nitpick comments:
In @.github/workflows/cleanup-caches.yml:
- Around line 3-9: Update the workflow trigger around the pull_request closed
event so forked pull requests can use a token capable of deleting caches, either
by switching to pull_request_target or by moving cleanup to a scheduled
workflow. Preserve the actions: write permission and existing cleanup behavior.
In @.github/workflows/docs-check.yml:
- Around line 84-101: Update the “Package fixtures (branch)” and “Save fixture
cache (branch)” steps to run on every non-main branch run, removing the
cache-age fresh check. Preserve the existing archive creation and cache-save
behavior so newly fetched pages persisted by HTTPCache._store can be included in
the next cache.
- Around line 75-82: Update the “Run MDX examples check (branch)” step to read
the cache-age fresh value through an environment variable, matching the existing
env-based pattern, and change the shell condition to use that variable instead
of interpolating steps.cache-age.outputs.fresh directly.
In @.gitignore:
- Line 12: Update the repository ignore rules alongside the .cache/mdx-html/
entry to also ignore the root-level fixtures.tar.gz archive generated by
.github/workflows/docs-check.yml.
In `@src/vlrdevapi/_series/players/parser.py`:
- Line 326: Update the game_id parameter annotation in parse_players_stats from
str to int | str so its type contract matches the positional integer selectors
accepted by resolve_game_id.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f241ee59-2b19-449e-8adc-f63912a59387
📒 Files selected for processing (29)
.github/workflows/cleanup-caches.yml.github/workflows/docs-check.yml.gitignoreofficial-docs/content/docs/reference/events/teams.mdxofficial-docs/content/docs/reference/series/performance.mdxofficial-docs/content/docs/reference/team/completed-matches.mdxofficial-docs/content/docs/reference/team/stats.mdxofficial-docs/content/docs/reference/team/upcoming-matches.mdxofficial-docs/content/guides/getting-started-with-vlrdevapi.mdxscripts/check_mdx_examples.pysrc/vlrdevapi/_series/_utils.pysrc/vlrdevapi/_series/economy/namespace.pysrc/vlrdevapi/_series/economy/parser.pysrc/vlrdevapi/_series/match_namespace.pysrc/vlrdevapi/_series/performance/models.pysrc/vlrdevapi/_series/performance/namespace.pysrc/vlrdevapi/_series/performance/parser.pysrc/vlrdevapi/_series/players/namespace.pysrc/vlrdevapi/_series/players/parser.pysrc/vlrdevapi/_series/rounds/namespace.pysrc/vlrdevapi/_series/rounds/parser.pytests/series/economy/test_models.pytests/series/economy/test_parser.pytests/series/performance/test_namespace.pytests/series/performance/test_parser.pytests/series/players/test_namespace.pytests/series/players/test_parser.pytests/series/rounds/test_models.pytests/series/test_game_id.py
| run: | | ||
| matched="${{ steps.mdx-fixtures.outputs.cache-matched-key }}" | ||
| now=$(date +%s) | ||
| if [ -n "$matched" ]; then | ||
| epoch="${matched##*-}" | ||
| age=$(( now - epoch )) | ||
| else | ||
| age=999999 | ||
| fi | ||
| if [ "$age" -gt 7200 ]; then | ||
| rm -rf .cache/mdx-html | ||
| rm -f fixtures.tar.gz | ||
| mkdir -p .cache/mdx-html | ||
| echo "fresh=false" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "fresh=true" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| echo "now=$now" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Step outputs are expanded inline into run scripts. Both steps interpolate ${{ steps.*.outputs.* }} directly into shell code, so the values become script text before execution. zizmor flags Line 57 and Line 78. Pass each value through env and read it as a shell variable.
.github/workflows/docs-check.yml#L56-L73: movesteps.mdx-fixtures.outputs.cache-matched-keyinto anenventry, read it as$MATCHED_KEY, and validate that the parsedepochis numeric and not in the future..github/workflows/docs-check.yml#L75-L82: movesteps.cache-age.outputs.freshinto anenventry and compare"$FRESH" = "true".
🧰 Tools
🪛 zizmor (1.29.0)
[info] 57-57: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
📍 Affects 1 file
.github/workflows/docs-check.yml#L56-L73(this comment).github/workflows/docs-check.yml#L75-L82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/docs-check.yml around lines 56 - 73, Update
.github/workflows/docs-check.yml lines 56-73 to pass
steps.mdx-fixtures.outputs.cache-matched-key through an env entry, read it as
$MATCHED_KEY, and validate that the parsed epoch is numeric and not in the
future before calculating age. Update lines 75-82 to pass
steps.cache-age.outputs.fresh through env, read it as $FRESH, and compare it to
"true" without inline step-output interpolation.
Source: Linters/SAST tools
| cache.mode = "replay" | ||
| print(f"=== Replay execution ({timeout}s timeout per block, offline against fixtures) ===") | ||
| orig_acquire = RateLimiter.acquire | ||
| RateLimiter.acquire = lambda self: None # type: ignore[method-assign] | ||
| try: | ||
| replay_start = time.monotonic() | ||
| await execute_blocks(results, timeout, cache, "Replay (validated)", record_results=True) | ||
| stats.replay = time.monotonic() - replay_start | ||
| finally: | ||
| RateLimiter.acquire = orig_acquire # type: ignore[method-assign] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replay disables the global rate limiter while live fallbacks remain possible.
RateLimiter.acquire is replaced with a no-op for the whole replay phase. The workflow comment in .github/workflows/docs-check.yml states that branch runs fall back to live fetches for anything missing, so fallbacks are expected, not exceptional. During fallback, only HTTPCache._throttle limits the rate, and it sleeps per calling thread. Parallel worker threads therefore multiply the request rate by the number of threads. vlr.gg can throttle or block the runner, which fails the docs check.
Keep the limiter active for fallback fetches, or serialize the throttle with the cache lock so the rate is global.
🛠️ Proposed global throttle
def _throttle(self) -> None:
- if DEFAULT_RATE_LIMIT > 0:
- time.sleep(1.0 / DEFAULT_RATE_LIMIT)
+ if DEFAULT_RATE_LIMIT <= 0:
+ return
+ interval = 1.0 / DEFAULT_RATE_LIMIT
+ with self._throttle_lock:
+ wait = self._next_slot - time.monotonic()
+ if wait > 0:
+ time.sleep(wait)
+ self._next_slot = max(self._next_slot, time.monotonic()) + intervalAdd the supporting state in __init__:
self._throttle_lock = threading.Lock()
self._next_slot = 0.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check_mdx_examples.py` around lines 519 - 528, Update the replay flow
around execute_blocks so RateLimiter.acquire is not globally replaced with a
no-op while live fallback requests remain possible. Preserve rate limiting
during fallback fetches, either by leaving RateLimiter active or by making
HTTPCache._throttle globally serialized using cache-level throttle state
initialized in HTTPCache.__init__.
| player_id = player_mapping.get(text) if player_mapping else None | ||
| victims.append(NotableVictim(name=text, player_id=player_id)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve notable-round victims without a name-only fallback.
PlayerMap.get(text) uses _by_name when no team is supplied. PlayerMap.add() preserves the first ID for that name. If both teams use the same in-game name, a victim can receive the opposing player's ID.
Pass sufficient team context into _parse_notable_rounds and perform a team-specific lookup. If the HTML does not provide enough context, keep player_id as None instead of returning an ambiguous ID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vlrdevapi/_series/performance/parser.py` around lines 201 - 202, Update
_parse_notable_rounds so each notable victim lookup includes the victim’s team
context and uses the team-specific PlayerMap lookup rather than the name-only
fallback. When the HTML lacks sufficient team information, leave player_id as
None; preserve the existing NotableVictim construction and append flow.
Deploying vlrdevapi with
|
| Latest commit: |
ec5bab8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18a1323b.vlrdevapi.pages.dev |
| Branch Preview URL: | https://chore-series-performance-fix.vlrdevapi.pages.dev |
…ings Pin actions/cache to v6.1.0 via a reviewed commit SHA in the docs-check workflow to mitigate supply chain risks from unpinned action tags. Add eol=lf rules for *.yml and *.yaml files to .gitattributes to enforce consistent LF line endings for workflow files across all operating systems. Add fixtures.tar.gz to .gitignore to prevent generated CI fixture cache archives from being accidentally committed. Fix missing trailing newline at end of .gitattributes.
Updates project changelogs to formalize the v2.2.0 release (2026-08-11) and document all included changes: - **Breaking change**: Renamed `AdvStatsEntry` fields to descriptive names: `econ` → `economy`, `pl` → `plants`, `de` → `defuses` to improve API clarity. - **Fix**: Series per-game stat endpoints (`players()`, `rounds()`, `performance()`, `economy()`) now correctly resolve 1-based game numbers to real VLR game IDs, fixing empty results for game number inputs. - **Fix**: Scoped series economy and round-by-round parsing to the selected game's section only, fixing cross-map data leakage in per-game requests. - **Fix**: Recover series performance player IDs from the series overview tab, fixing incorrect `0` IDs in performance data. Added the v2.2.0 version comparison link to the main changelog. BREAKING CHANGE: Renamed `AdvStatsEntry` fields `econ` to `economy`, `pl` to `plants`, and `de` to `defuses`. All code referencing the old field names must be updated to avoid errors.
|
Checkout my PR :) I've added the news module |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests