Conversation
Deploying vlrdevapi with
|
| Latest commit: |
7ce04dc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://079fc030.vlrdevapi.pages.dev |
| Branch Preview URL: | https://fixed-issues.vlrdevapi.pages.dev |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
CodeRabbit config \.coderabbit\.yaml |
Adds a comprehensive CodeRabbit review configuration covering security, performance, quality, API, docs, tests, style, dependency checks, automation and manual review rules. |
Dependencies pyproject\.toml |
Adds python-dateutil>=2.8.0 to project dependencies. |
API documentation & examples betterdocs/content/docs/api/events/index.mdx, betterdocs/content/docs/api/events/list-events.mdx, betterdocs/content/docs/api/events/match-summary.mdx, betterdocs/content/docs/api/events/matches.mdx, docs/source/examples.rst, betterdocs/content/docs/index.mdx |
Removes tier from ListEvent docs; Quick Example output prints only event name; match-summary describes stage-level counts/date ranges; renames "live"→"ongoing" terminology in examples; shortens docs hrefs to /docs/api/.... |
Core events & match processing src/vlrdevapi/events.py |
Removes ListEvent.tier; adds _get_event_dates_from_matches(event_id, timeout) fallback; uses dateutil fuzzy parsing with strict-format fallbacks and "TBD" handling; improves date extraction heuristics and tolerant match_id parsing; prefers header links and deduplicates team IDs; computes per-stage statistics and per-stage date ranges in match_summary; propagates updated date logic. |
Search parsing src/vlrdevapi/search/search.py |
Iterates all result container divs (handles multiple results_divs), processes items in nested loops, and restores event-date/prize extraction per item while preserving public output shape. |
UI / docs site betterdocs/app/(home)/page.tsx, betterdocs/app/layout.tsx, betterdocs/components/installation-command.tsx, betterdocs/lib/layout.shared.tsx |
Large Home page rewrite with hero, features grid, installation snippet and new internal components; replaces Next font loader with inline Google Sans Flex import; adds exported InstallationCommand component (copy-to-clipboard UI); adds Documentation link to shared layout options. |
HTML test fixture tests/html_sources/search_nrg_all.html |
Removes page boilerplate and external assets; simplifies markup to category-labeled results; updates results count, item paths/titles/descriptions/thumbnails to match new test data. |
Tests — teams & fixtures tests/lib/test_teams.py |
Roster test now iterates multiple team IDs (2593, 1167) with per-team exception handling and requires at least one team with ≥5 players; updates many assertions to align with changed fixtures (transactions, names, countries, dates). |
Example snippet main.py |
Removes the vlrdevapi import and the example matches.completed(..., page=2) call from the snippet. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Client
participant Events as events.py
participant Dateutil as dateutil.parser
participant Fallback as _get_event_dates_from_matches
rect rgb(230,245,255)
Note over Client,Events: Primary date extraction flow (fuzzy parse → strict formats → fallback)
end
Client->>Events: list_events() / matches()
Events->>Events: choose date text (value-level selector → fallback selector)
Events->>Dateutil: parse(text, fuzzy=True)
alt parsed successfully
Dateutil-->>Events: datetime
Events-->>Client: return with parsed dates
else parse fails or "TBD"
Events->>Events: try strict parse formats
alt parsed by formats
Events-->>Client: return with parsed dates
else
Events->>Fallback: fetch event matches pages
Fallback->>Fallback: extract min/max match dates from matches
Fallback-->>Events: fallback start_date/end_date
Events-->>Client: return with fallback dates
end
end
rect rgb(240,255,230)
Note over Client,Events: match_summary now produces per-stage statistics and date ranges
end
Client->>Events: match_summary(match_id)
Events->>Events: extract stages, sanitize names, parse per-match dates
loop per stage
Events->>Events: count total/completed/upcoming/ongoing
Events->>Events: compute stage date range
Events->>Events: build StageMatches entry
end
Events-->>Client: MatchSummary with stage breakdowns
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Areas needing extra attention:
- src/vlrdevapi/events.py: fuzzy parsing logic, strict-format fallbacks, "TBD" handling, date provenance, and tolerant match_id parsing.
- match_summary: stage extraction, counting logic, date-range aggregation, and StageMatches construction.
- _get_event_dates_from_matches: HTML selectors and fallback reliability.
- src/vlrdevapi/search/search.py & tests/html_sources/search_nrg_all.html: ensure parser aligns with fixture structure.
- tests/lib/test_teams.py: per-team exception handling should not mask real failures; updated assertions vs fixtures.
Poem
🐇 I hopped through markup, parsed each date,
Fuzzy hops found ranges when pages were late.
Stages tallied neat, tier baggage set free,
"Live" now "ongoing" — hop along with me!
Docs, a copy command, and tests in harmony.
Pre-merge checks and finishing touches
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title is vague and generic, using non-descriptive language that does not convey meaningful information about the changeset. | Use a more specific title that highlights the main changes, such as 'Remove tier field from events and enhance date parsing' or 'Refactor events API and update documentation'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
fixed-issues
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 @coderabbitai help to get the list of available commands and usage tips.
![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)
tests/lib/test_teams.py (1)
454-469: Improve error handling and consider logging exceptions.The multi-team check approach is good for test robustness, but the bare exception catch could hide real issues like network problems or parsing errors.
Consider this improvement:
for team_id in team_ids: try: roster = vlr.teams.roster(team_id=team_id) players = [m for m in roster if m.role == "Player"] if len(players) >= 5: teams_with_sufficient_players.append(team_id) - except Exception: + except Exception as e: # Continue checking other teams if one fails + # Log the error for debugging + print(f"Warning: Failed to check roster for team {team_id}: {e}") continueThis preserves the resilience while providing visibility into failures during test runs.
src/vlrdevapi/events.py (3)
186-244: Robust fallback mechanism for event date extraction.The private helper function provides a solid fallback when event dates are unavailable or marked as TBD. The implementation correctly:
- Handles network errors gracefully
- Converts relative dates (today, yesterday, tomorrow) to actual dates
- Tries multiple date formats
- Returns the earliest and latest match dates
One minor suggestion: Consider using
datetime.date.today()once at the start rather than calling it inside the if statement for slightly better performance:# At the start of the function, after imports today = datetime.date.today()
628-648: Enhanced team ID extraction with robust fallbacks.The improved logic correctly:
- Tries multiple selectors with appropriate fallback priority
- Deduplicates team links by URL while preserving order
- Limits to two teams as expected for a match
The implementation is solid, but consider adding a comment explaining why we deduplicate by URL:
# Deduplicate by team URL while preserving order # (same team may appear multiple times with different selectors) seen_hrefs: set[str] = set()
892-930: Stage-aware statistics tracking implemented correctly.The implementation properly:
- Extracts stage names and sanitizes them (removing phase info)
- Parses match dates
- Tracks per-stage counts and dates
- Handles the "Unknown Stage" fallback
Minor suggestion: Consider extracting the stage name sanitization logic into a small helper function since it's used in multiple places (here and in the
matches()function):def _sanitize_stage_name(event_el: Tag | None, series_el: Tag | None) -> str: """Extract and sanitize stage name, removing phase if present.""" stage_name = extract_text(event_el) or "Unknown Stage" phase = extract_text(series_el) or None if phase and stage_name: stage_name = stage_name.replace(phase, "").strip() return stage_name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.coderabbit.yaml(1 hunks)betterdocs/content/docs/api/events/index.mdx(1 hunks)betterdocs/content/docs/api/events/list-events.mdx(0 hunks)betterdocs/content/docs/api/events/match-summary.mdx(1 hunks)betterdocs/content/docs/api/events/matches.mdx(1 hunks)docs/source/examples.rst(1 hunks)main.py(0 hunks)pyproject.toml(1 hunks)src/vlrdevapi/events.py(9 hunks)tests/lib/test_teams.py(1 hunks)
💤 Files with no reviewable changes (2)
- betterdocs/content/docs/api/events/list-events.mdx
- main.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/vlrdevapi/events.py (3)
src/vlrdevapi/fetcher.py (1)
fetch_html(422-443)src/vlrdevapi/exceptions.py (1)
NetworkError(50-52)src/vlrdevapi/utils.py (4)
parse_date(148-164)extract_text(25-34)extract_id_from_url(98-119)parse_int(54-64)
tests/lib/test_teams.py (1)
src/vlrdevapi/teams/roster.py (1)
roster(18-133)
🪛 Ruff (0.14.3)
tests/lib/test_teams.py
464-466: try-except-continue detected, consider logging the exception
(S112)
464-464: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (4)
- GitHub Check: validate-docs
- GitHub Check: Test on Python 3.11
- GitHub Check: validate-betterdocs
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
docs/source/examples.rst (1)
68-72: LGTM! Terminology updated consistently.The variable and output text have been updated from "live" to "ongoing" to match the updated API terminology throughout the codebase.
pyproject.toml (1)
46-46: LGTM! Dependency added to support enhanced date parsing.The
python-dateutil>=2.8.0dependency is correctly added to support the fuzzy date parsing functionality introduced insrc/vlrdevapi/events.py. The version constraint is appropriate and widely compatible.betterdocs/content/docs/api/events/index.mdx (1)
69-69: LGTM! Documentation updated to reflect API changes.The example output correctly removes the tier field reference, consistent with the removal of
ListEvent.tierin the API code.betterdocs/content/docs/api/events/matches.mdx (1)
168-186: LGTM! Terminology consistently updated from "live" to "ongoing".The example has been properly updated to use "ongoing" terminology throughout, including the section title, code comments, variable names, and output messages. This aligns with the API's status terminology.
betterdocs/content/docs/api/events/match-summary.mdx (1)
74-74: LGTM! Documentation updated to reflect stage-aware summaries.The description now accurately reflects that stages include counts and date ranges, matching the enhanced
StageMatchesimplementation in the code.src/vlrdevapi/events.py (7)
17-17: LGTM! dateutil import added for enhanced date parsing.The import is correctly added to support fuzzy date parsing throughout the module, aligning with the new dependency in
pyproject.toml.
280-280: LGTM! Tier always included in request parameters.Good change to ensure tier is consistently included in API requests, even when set to "all".
319-326: Improved date text extraction with fallback.The enhanced extraction logic correctly prioritizes the value element to get the complete date text (including year), with a proper fallback to the previous extraction method.
714-719: LGTM! Robust match ID parsing with multiple URL format support.The implementation correctly handles different URL patterns and uses safe integer parsing with
parse_int(). This is good defensive programming for web scraping.
858-891: LGTM! Match status counting correctly updated.The status counting logic is clean and correctly distinguishes between completed, ongoing, and upcoming matches.
932-956: Stage summaries correctly built and returned.The logic properly constructs
StageMatchesobjects with counts and date ranges for each stage, providing valuable per-stage insights.
359-405: Code is correct; ParserError is properly accessible.The
dateutil.parser.ParserErroris a valid exception class documented in dateutil, and your import patternfrom dateutil import parser as dateutil_parsercorrectly makes it accessible asdateutil_parser.ParserError. The exception handling at lines 367 and 383 is correct as written.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
1-226: Restructure configuration to align with CodeRabbit schema for proper recognition.The configuration has been greatly improved (language is now correctly set to
en-USand the invalidframeworkfield has been removed). However, most of the custom fields (e.g.,rules,documentation,testing,style,dependencies,performance,integrations,severity,messages,automationat lines 54–226) won't be processed by CodeRabbit because they're not part of the official schema. Similarly, theinclude/excludesyntax (lines 6–20) should be structured asreviews.path_filtersper the schema.If this file is intended to guide CodeRabbit's review behavior, consider restructuring to use the supported schema:
- Move
include/excludepatterns intoreviews.path_filters- Use
reviews.path_instructionsfor path-specific guidelines- Use
reviews.tools.*to configure linters (ruff, pylint, etc.)- Move custom business rules into
reviews.path_instructionswith structured pattern-matching within each instructionIf this file is primarily internal documentation or team guidelines separate from CodeRabbit's automation, the current structure is acceptable and can serve as a reference document.
Example restructuring (if aiming for CodeRabbit integration):
-language: en-US - - -# Paths to include in code review -include: - - "src/**/*.py" - - "tests/**/*.py" - - "pyproject.toml" - - "README.md" - -# Paths to exclude from code review -exclude: - - "src/**/__pycache__/**" - - "tests/**/__pycache__/**" - - ".git/**" - - ".pytest_cache/**" - - ".venv/**" - - "uv.lock" - - "*.egg-info/**" - -# Review focus areas for a web scraping API library -focus: - # ... etc +language: en-US +reviews: + profile: chill + path_filters: + - "src/**/*.py" + - "tests/**/*.py" + - "pyproject.toml" + - "README.md" + - "!src/**/__pycache__/**" + - "!tests/**/__pycache__/**" + - "!.git/**" + - "!.pytest_cache/**" + - "!.venv/**" + - "!uv.lock" + - "!*.egg-info/**" + path_instructions: + - path: "src/**/*.py" + instructions: | + Focus on: rate limiting, user agent handling, input validation, error handling for network requests. + Ensure data classes use frozen=True. Validate URLs properly. Include docstrings with examples for public methods. + - path: "tests/**/*.py" + instructions: | + Verify fixtures are used, mocking is in place for network calls, test naming follows test_* pattern. + Aim for 80%+ coverage. Check error handling tests are included. + tools: + ruff: + enabled: true + pylint: + enabled: true
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.coderabbit.yaml(1 hunks)
⏰ 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). (4)
- GitHub Check: validate-docs
- GitHub Check: Test on Python 3.11
- GitHub Check: validate-betterdocs
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/lib/test_teams.py (2)
452-469: Refine exception handling to improve debuggability.The bare
except Exceptionclause silently continues without logging, making it difficult to diagnose why a team fetch failed during testing. Consider catching specific exceptions (e.g.,NetworkError) or at least logging the failure before continuing.Apply this diff to improve exception handling:
- except Exception: - # Continue checking other teams if one fails - continue + except Exception as e: + # Log and continue checking other teams if one fails + print(f"Failed to fetch roster for team {team_id}: {e}") + continue
1263-1263: Remove unused variable.The variable
leave_countis assigned but never used in this test. Consider removing it or using it in the validation logic.Apply this diff:
for player in players: join_count = sum(1 for txn in player.transactions if txn.action == "join") - leave_count = sum(1 for txn in player.transactions if txn.action == "leave") # If player has multiple joins, verify status is based on most recent action if join_count > 1:src/vlrdevapi/search/search.py (2)
94-101: Clarifyis_inactivecomputation to avoid precedence surprisesThe current line:
is_inactive = "inactive" in desc_text.lower() or "inactive" in title.lower() if title else Falserelies on
or+ ternary precedence, which is easy to misread and also meansdesc_textis ignored whenevertitleis falsy (even if the description contains “inactive”). Consider making this more explicit and slightly more robust:- is_inactive = "inactive" in desc_text.lower() or "inactive" in title.lower() if title else False + inactive_in_desc = "inactive" in desc_text.lower() + inactive_in_title = bool(title and "inactive" in title.lower()) + is_inactive = inactive_in_desc or inactive_in_titleThis both clarifies intent and ensures the description alone can still mark a team as inactive if the title happens to be missing.
116-128: Event description parsing is template-dependent; consider making it more flexibleThe current logic:
parts = desc_text.split("event") ... detail_parts = [p.strip() for p in details.split("–") if p.strip()]assumes:
- The literal substring
"event"(lowercase) is present in the description, and- Sections are separated by an EN DASH (
–).This is fine as long as VLR’s markup stays exactly in that shape, but it’s somewhat brittle and is also what Ruff is flagging with RUF001 (ambiguous
–). As a future improvement, you could:
- Make the
"event"split more robust (e.g., case-insensitive and limited to the first occurrence), and- Allow both
–and-as separators using a small regex, while keeping your existing behavior for the current fixtures.For example (conceptually):
# Pseudocode-level idea, not required to adopt verbatim details = desc_text.split("event", 1)[-1].strip() detail_parts = [p.strip() for p in re.split(r"\s+[–-]\s+", details) if p.strip()]If the current tests pass against real/fixed fixtures, this can stay as-is and be revisited only if the HTML changes or Ruff’s rule becomes blocking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/vlrdevapi/search/search.py(2 hunks)tests/html_sources/search_nrg_all.html(1 hunks)tests/lib/test_teams.py(44 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/vlrdevapi/search/search.py (2)
src/vlrdevapi/utils.py (4)
absolute_url(43-51)normalize_whitespace(38-40)extract_text(25-34)extract_id_from_url(98-119)src/vlrdevapi/search/models.py (4)
SearchPlayerResult(10-19)SearchTeamResult(23-32)SearchEventResult(36-45)SearchSeriesResult(49-56)
tests/lib/test_teams.py (2)
tests/lib/conftest.py (1)
mock_fetch_html(26-92)src/vlrdevapi/teams/transactions.py (1)
transactions(41-177)
🪛 Ruff (0.14.5)
src/vlrdevapi/search/search.py
120-120: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
tests/lib/test_teams.py
32-32: Unused method argument: mock_fetch_html
(ARG002)
37-37: Unused method argument: mock_fetch_html
(ARG002)
66-66: Unused method argument: mock_fetch_html
(ARG002)
74-74: Unused method argument: mock_fetch_html
(ARG002)
90-90: Unused method argument: mock_fetch_html
(ARG002)
98-98: Unused method argument: mock_fetch_html
(ARG002)
130-130: Unused method argument: mock_fetch_html
(ARG002)
137-137: Unused method argument: mock_fetch_html
(ARG002)
153-153: Unused method argument: mock_fetch_html
(ARG002)
161-161: Unused method argument: mock_fetch_html
(ARG002)
167-167: Unused method argument: mock_fetch_html
(ARG002)
175-175: Unused method argument: mock_fetch_html
(ARG002)
183-183: Unused method argument: mock_fetch_html
(ARG002)
221-221: Unused method argument: mock_fetch_html
(ARG002)
225-225: Do not assert blind exception: Exception
(B017)
250-250: Unused method argument: mock_fetch_html
(ARG002)
264-264: Unused method argument: mock_fetch_html
(ARG002)
270-270: Unused method argument: mock_fetch_html
(ARG002)
289-289: Unused method argument: mock_fetch_html
(ARG002)
307-307: Unused method argument: mock_fetch_html
(ARG002)
313-313: Unused method argument: mock_fetch_html
(ARG002)
319-319: Unused method argument: mock_fetch_html
(ARG002)
327-327: Unused method argument: mock_fetch_html
(ARG002)
333-333: Unused method argument: mock_fetch_html
(ARG002)
347-347: Unused method argument: mock_fetch_html
(ARG002)
352-352: Unused method argument: mock_fetch_html
(ARG002)
371-371: Unused method argument: mock_fetch_html
(ARG002)
379-379: Unused method argument: mock_fetch_html
(ARG002)
387-387: Unused method argument: mock_fetch_html
(ARG002)
403-403: Unused method argument: mock_fetch_html
(ARG002)
410-410: Unused method argument: mock_fetch_html
(ARG002)
416-416: Unused method argument: mock_fetch_html
(ARG002)
423-423: Unused method argument: mock_fetch_html
(ARG002)
430-430: Unused method argument: mock_fetch_html
(ARG002)
436-436: Unused method argument: mock_fetch_html
(ARG002)
452-452: Unused method argument: mock_fetch_html
(ARG002)
464-466: try-except-continue detected, consider logging the exception
(S112)
464-464: Do not catch blind exception: Exception
(BLE001)
471-471: Unused method argument: mock_fetch_html
(ARG002)
531-531: Unused method argument: mock_fetch_html
(ARG002)
537-537: Unused method argument: mock_fetch_html
(ARG002)
543-543: Unused method argument: mock_fetch_html
(ARG002)
548-548: Do not assert blind exception: Exception
(B017)
563-563: Unused method argument: mock_fetch_html
(ARG002)
585-585: Unused method argument: mock_fetch_html
(ARG002)
626-626: Unused method argument: mock_fetch_html
(ARG002)
634-634: Unused method argument: mock_fetch_html
(ARG002)
654-654: Unused method argument: mock_fetch_html
(ARG002)
693-693: Unused method argument: mock_fetch_html
(ARG002)
701-701: Unused method argument: mock_fetch_html
(ARG002)
710-710: Unused method argument: mock_fetch_html
(ARG002)
720-720: Unused method argument: mock_fetch_html
(ARG002)
730-730: Unused method argument: mock_fetch_html
(ARG002)
738-738: Unused method argument: mock_fetch_html
(ARG002)
792-792: Unused method argument: mock_fetch_html
(ARG002)
797-797: Unused method argument: mock_fetch_html
(ARG002)
824-824: Unused method argument: mock_fetch_html
(ARG002)
832-832: Unused method argument: mock_fetch_html
(ARG002)
855-855: Unused method argument: mock_fetch_html
(ARG002)
871-871: Unused method argument: mock_fetch_html
(ARG002)
876-876: Do not assert blind exception: Exception
(B017)
879-879: Unused method argument: mock_fetch_html
(ARG002)
885-885: Unused method argument: mock_fetch_html
(ARG002)
906-906: Unused method argument: mock_fetch_html
(ARG002)
911-911: Unused method argument: mock_fetch_html
(ARG002)
930-930: Unused method argument: mock_fetch_html
(ARG002)
948-948: Unused method argument: mock_fetch_html
(ARG002)
957-957: Unused method argument: mock_fetch_html
(ARG002)
970-970: Unused method argument: mock_fetch_html
(ARG002)
982-982: Unused method argument: mock_fetch_html
(ARG002)
990-990: Unused method argument: mock_fetch_html
(ARG002)
1003-1003: Unused method argument: mock_fetch_html
(ARG002)
1023-1023: Unused method argument: mock_fetch_html
(ARG002)
1028-1028: Do not assert blind exception: Exception
(B017)
1041-1041: Unused method argument: mock_fetch_html
(ARG002)
1046-1046: Unused method argument: mock_fetch_html
(ARG002)
1066-1066: Unused method argument: mock_fetch_html
(ARG002)
1079-1079: Unused method argument: mock_fetch_html
(ARG002)
1087-1087: Unused method argument: mock_fetch_html
(ARG002)
1095-1095: Unused method argument: mock_fetch_html
(ARG002)
1113-1113: Unused method argument: mock_fetch_html
(ARG002)
1132-1132: Unused method argument: mock_fetch_html
(ARG002)
1147-1147: Unused method argument: mock_fetch_html
(ARG002)
1160-1160: Unused method argument: mock_fetch_html
(ARG002)
1172-1172: Unused method argument: mock_fetch_html
(ARG002)
1187-1187: Unused method argument: mock_fetch_html
(ARG002)
1200-1200: Unused method argument: mock_fetch_html
(ARG002)
1205-1205: Do not assert blind exception: Exception
(B017)
1208-1208: Unused method argument: mock_fetch_html
(ARG002)
1214-1214: Unused method argument: mock_fetch_html
(ARG002)
1226-1226: Unused method argument: mock_fetch_html
(ARG002)
1242-1242: Unused method argument: mock_fetch_html
(ARG002)
1255-1255: Unused method argument: mock_fetch_html
(ARG002)
1263-1263: Local variable leave_count is assigned to but never used
Remove assignment to unused variable leave_count
(F841)
⏰ 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). (4)
- GitHub Check: validate-betterdocs
- GitHub Check: Test on Python 3.11
- GitHub Check: validate-docs
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
tests/html_sources/search_nrg_all.html (4)
88-173: LGTM: Event search results structure.The event search results are well-structured with consistent formatting, proper date ranges, and prize money display. The new URL pattern (
/search/r/event/{id}/idx) aligns with the routing changes.
179-196: LGTM: Player search results structure.The player search result follows the same URL pattern as teams and events. The placeholder image usage is appropriate for testing scenarios where player photos may not be available.
40-40: LGTM: Results count is accurate.The updated count of 7 results correctly reflects the visible search results (2 teams + 4 events + 1 player).
52-80: Fix trailing space in inactive indicator.The URL pattern change to
/search/r/team/{id}/idxis compatible with the existingextract_id_from_url()function, which iterates through URL segments and correctly locates theteamprefix regardless of path depth. No code changes needed there.However, the trailing space in the inactive indicator at line 74 should be cleaned up for consistency:
- (inactive ) + (inactive)src/vlrdevapi/search/search.py (1)
48-75: Refactor to handle multiple result cards looks correctIterating over
results_divsand thena.wf-module-item.search-itempreserves the original extraction logic while allowing multiplewf-cardsections, and the early return when no cards are found keeps the empty-results behavior explicit. URL, title, description, and image handling all stay consistent with the utilities (absolute_url,extract_text,normalize_whitespace). No issues from a correctness perspective here.
| def test_transactions_first_entry(self, mock_fetch_html): | ||
| """Test first transaction entry (most recent).""" | ||
| txns = vlr.teams.transactions(team_id=1034) | ||
| txns = vlr.teams.transactions(team_id=2815) | ||
| assert len(txns) > 0 | ||
|
|
||
| first = txns[0] | ||
| # Based on HTML: FiNESSE leaving | ||
| assert first.ign == "s0m" | ||
| assert first.real_name == "Sam Oh" | ||
| assert first.ign == "effys" | ||
| assert first.real_name == "Loic Sauvageau" | ||
| assert first.action == "leave" | ||
| # Date from HTML source | ||
| from datetime import date | ||
| if first.date: | ||
| assert isinstance(first.date, date) | ||
| assert first.date.year == 2025 | ||
| assert first.date.month == 10 | ||
| assert first.date.day in [22, 21] # May vary by timezone | ||
| assert first.date.year == 2022 | ||
| assert first.date.month == 12 | ||
| assert first.date.day == 23 | ||
| assert first.position == "Player" | ||
| assert first.country == "United States" | ||
| assert first.player_id == 4164 | ||
| assert first.country == "Canada" | ||
| assert first.player_id == 65 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the test fixture matches the updated expectations.
The test now expects data from team 2815 with specific player details (effys, Loic Sauvageau, player_id 65, date 2022-12-23). Ensure the corresponding HTML fixture file exists and contains the expected data.
Run the following script to verify the fixture exists and contains the expected data:
🏁 Script executed:
#!/bin/bash
# Verify team_nrg_1034_transactions.html fixture exists and check for team 2815 fixture
# Check if the old fixture is still referenced
fd -t f "team_nrg_1034_transactions.html" tests/html_sources/
# Search for any team 2815 fixtures
fd -t f "2815" tests/html_sources/
# Check if team 2815 is mapped in conftest.py
rg -n "2815" tests/lib/conftest.py
# Search for "effys" in HTML fixtures to find which file contains the expected data
rg -l "effys" tests/html_sources/Length of output: 50
🏁 Script executed:
#!/bin/bash
# Inspect the conftest.py to understand mock_fetch_html fixture setup
echo "=== Checking conftest.py for mock_fetch_html and fixture mappings ==="
rg -n -A 10 "mock_fetch_html|@pytest.fixture" tests/lib/conftest.py | head -100
# Check the actual test file to see full test context
echo -e "\n=== Checking test_teams.py around test_transactions_first_entry ==="
rg -n -B 5 "def test_transactions_first_entry" tests/lib/test_teams.py | head -50
# Look for any fixture parameter definitions related to transactions
echo -e "\n=== Searching for transactions fixture setup ==="
rg -n "transactions|team_nrg" tests/lib/conftest.py | head -20
# Check if there's a fixture for team 1034 that should be renamed to 2815
echo -e "\n=== Listing all HTML fixtures ==="
fd -t f "\\.html$" tests/html_sources/ | sortLength of output: 3235
🏁 Script executed:
#!/bin/bash
# Get the full mock_fetch_html implementation to see what happens with unmapped URLs
echo "=== Full mock_fetch_html fixture implementation ==="
rg -n -A 70 "def mock_fetch_html\(monkeypatch, load_html\)" tests/lib/conftest.py
# Also check if there's a default behavior or exception handling
echo -e "\n=== Checking for URL matching logic ==="
rg -n -A 5 "url_to_file" tests/lib/conftest.py | head -40Length of output: 5039
Add fixture mapping for team 2815 or revert test to use team 1034 data.
Verification found that mock_fetch_html fixture has no URL mapping for https://www.vlr.gg/team/transactions/2815. When the test calls transactions(team_id=2815), the mock cannot locate the matching fixture and will attempt to call the original fetch_html function, which fails in the offline test environment.
Either:
- Add the missing URL mapping to
tests/lib/conftest.pyline 63+ and create the corresponding fixture file, or - Revert the test to use
team_id=1034with the existing fixtureteam_nrg_1034_transactions.html
🧰 Tools
🪛 Ruff (0.14.5)
930-930: Unused method argument: mock_fetch_html
(ARG002)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/lib/test_teams.py (1)
930-947:test_transactions_first_entrydepends on new team 2815 fixture mappingThe expectations for team 2815 / effys (IGN, real_name, country=Canada, player_id=65, date ~ 2022‑12‑22/23) look consistent and well-scoped, and the timezone-tolerant day check is a nice touch.
However, prior review already found that
mock_fetch_htmldid not have a URL mapping forhttps://www.vlr.gg/team/transactions/2815, causingvlr.teams.transactions(team_id=2815)to fall back to the realfetch_htmlin tests.Please re-verify that:
tests/lib/conftest.pymapshttps://www.vlr.gg/team/transactions/2815(or an appropriate prefix) to a local HTML fixture, and- The referenced HTML file actually contains this first transaction row.
Optionally, to strengthen the test as a regression check, you could assert the date is non-
Nonebefore checking its components:- if first.date: - assert isinstance(first.date, date) + assert first.date is not None + if first.date: + assert isinstance(first.date, date)
🧹 Nitpick comments (9)
tests/lib/test_teams.py (4)
32-1214: Fixture parametermock_fetch_htmland Ruff ARG002Almost every test takes
mock_fetch_htmlonly to activate the monkeypatched fetcher, which is idiomatic for pytest fixtures. Ruff’s ARG002 warnings here are noise rather than real issues.You can either:
- Configure Ruff to ignore ARG002 for pytest-style test files/fixtures, or
- (Less preferable, more churn) rename the parameter to
_mock_fetch_htmlacross tests.I’d lean toward a Ruff config tweak to avoid touching a large number of otherwise-clear tests.
452-470: Broadtry/except Exceptionintest_roster_player_rolecan hide real failuresThe multi-team approach for roster size (Fnatic 2593, Sentinels 1167) is sensible, but the
try/except Exception: continue(Lines 459–466) will silently skip teams even for genuine parsing or logic bugs, which can make regressions invisible.Consider tightening this up:
- Catch only expected failures (e.g., network/fixture issues) or
- At least log/attach the exception so a skipped team is diagnosable, or
- Let unexpected exceptions fail the test.
Example:
- try: - roster = vlr.teams.roster(team_id=team_id) - players = [m for m in roster if m.role == "Player"] - if len(players) >= 5: - teams_with_sufficient_players.append(team_id) - except Exception: - # Continue checking other teams if one fails - continue + try: + roster = vlr.teams.roster(team_id=team_id) + players = [m for m in roster if m.role == "Player"] + if len(players) >= 5: + teams_with_sufficient_players.append(team_id) + except Exception as exc: # narrow this if you know the expected type + # If a specific team’s page/fixture is broken, surface it clearly + pytest.fail(f"Roster fetch/parsing failed for team_id={team_id}: {exc}")Also, please verify that
tests/lib/conftest.pyhas HTML mappings forteam_id=2593andteam_id=1167so this test doesn’t fall back to live network calls.
221-248: Use more specific exception types in immutability testsSeveral tests assert immutability with
pytest.raises(Exception):
test_models_immutable(Lines 221–227)test_social_link_structure(also usesExceptionin this block)test_previous_team_structure(Lines 239–248)test_roster_immutable(Lines 543–549)test_placements_immutable(Lines 871–877)test_transactions_immutable(Lines 1023–1029)test_previous_players_immutable(Lines 1200–1206)Catching bare
Exceptionmakes it easier for unrelated errors (e.g., attribute typos) to pass as “immutability” successes and triggers Ruff B017.If your models are frozen dataclasses, you can tighten these to the concrete exception:
-from pytest import raises +from dataclasses import FrozenInstanceError +from pytest import raises - with pytest.raises(Exception): + with pytest.raises(FrozenInstanceError): team.name = "new_name"Or, if behavior varies, use a small tuple of expected types (e.g.,
pytest.raises((FrozenInstanceError, AttributeError))).This keeps the tests focused on the intended failure mode and satisfies the linter.
Also applies to: 543-549, 871-877, 1023-1029, 1200-1206
1255-1271: Remove or use unusedleave_countintest_previous_players_rejoin_scenarioIn
test_previous_players_rejoin_scenario(Lines 1259–1267),leave_countis calculated but never used, which Ruff flags as F841:join_count = sum(1 for txn in player.transactions if txn.action == "join") leave_count = sum(1 for txn in player.transactions if txn.action == "leave")If the logic really only depends on multiple joins, drop
leave_count:- join_count = sum(1 for txn in player.transactions if txn.action == "join") - leave_count = sum(1 for txn in player.transactions if txn.action == "leave") + join_count = sum(1 for txn in player.transactions if txn.action == "join")If you intended to also assert anything about multiple leaves, incorporate
leave_countinto the conditions instead of discarding it.betterdocs/content/docs/index.mdx (1)
57-87: Align API card hrefs and verify route consistencySwitching these cards to
/docs/api/<module>(without/index) is cleaner, but note that the “Status” card below still points to/docs/api/status/index. Consider standardizing on one pattern (with or without/index) across all API docs cards, assuming the routes resolve correctly.betterdocs/app/layout.tsx (1)
8-14: Prefer Next.js font utilities or global CSS over inline@importin layoutEmbedding a
<head><style>@import ...</style></head>directly in the root layout works but is non‑idiomatic for the App Router and less optimal for font loading. Consider usingnext/font(e.g.,next/font/google) or a global CSS import for Google Sans Flex instead, and rely on Next’s metadata/head mechanisms rather than a manual<head>here.betterdocs/app/(home)/page.tsx (1)
166-269: Tighten React typings and remove unusedLayersIcon
FeatureCardcurrently typesiconasReact.ReactNodewithout importing React; this relies on the global React namespace. Consider importing the type explicitly instead:-import Link from 'next/link'; -import { InstallationCommand } from '@/components/installation-command'; +import Link from 'next/link'; +import type { ReactNode } from 'react'; +import { InstallationCommand } from '@/components/installation-command'; ... -function FeatureCard({ title, description, icon }: { title: string; description: string; icon: React.ReactNode }) { +function FeatureCard({ title, description, icon }: { title: string; description: string; icon: ReactNode }) {Also,
LayersIconis defined but never used; if you don’t plan to use it, you can safely remove it to keep this module leaner.betterdocs/components/installation-command.tsx (2)
5-7:labelprop is unused – either render it or drop it from the API
InstallationCommandaccepts alabelprop, and the home page passes values like"pip"/"uv", but the component never renders it. Either:
- Add a small visual indicator (e.g., a badge showing the label), or
- Remove
labelfrom the prop type and all call sites if you don’t need it.Right now it’s dead API surface.
8-12: Harden clipboard copy logic for unsupported/denied cases
navigator.clipboard.writeText(command)is called without feature detection or error handling. In non‑secure contexts or older/locked‑down browsers this can fail.Consider:
- const handleCopy = () => { - navigator.clipboard.writeText(command); - setCopied(true); - setTimeout(() => setCopied(false), 2000); - }; + const handleCopy = () => { + if (!navigator.clipboard?.writeText) return; + navigator.clipboard.writeText(command).then( + () => { + setCopied(true); + setTimeout(() => setCopied(false), 2000); + }, + () => { + // Optional: surface a toast/log if copy fails + }, + ); + };This avoids potential runtime errors and makes failures easier to extend later.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
betterdocs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
betterdocs/app/(home)/page.tsx(1 hunks)betterdocs/app/layout.tsx(1 hunks)betterdocs/components/installation-command.tsx(1 hunks)betterdocs/content/docs/index.mdx(1 hunks)betterdocs/lib/layout.shared.tsx(1 hunks)tests/lib/test_teams.py(44 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/lib/test_teams.py (3)
tests/lib/conftest.py (1)
mock_fetch_html(26-92)src/vlrdevapi/teams/roster.py (1)
roster(18-133)src/vlrdevapi/teams/transactions.py (1)
transactions(41-177)
betterdocs/app/(home)/page.tsx (1)
betterdocs/components/installation-command.tsx (1)
InstallationCommand(5-33)
🪛 Ruff (0.14.5)
tests/lib/test_teams.py
32-32: Unused method argument: mock_fetch_html
(ARG002)
37-37: Unused method argument: mock_fetch_html
(ARG002)
66-66: Unused method argument: mock_fetch_html
(ARG002)
74-74: Unused method argument: mock_fetch_html
(ARG002)
90-90: Unused method argument: mock_fetch_html
(ARG002)
98-98: Unused method argument: mock_fetch_html
(ARG002)
130-130: Unused method argument: mock_fetch_html
(ARG002)
137-137: Unused method argument: mock_fetch_html
(ARG002)
153-153: Unused method argument: mock_fetch_html
(ARG002)
161-161: Unused method argument: mock_fetch_html
(ARG002)
167-167: Unused method argument: mock_fetch_html
(ARG002)
175-175: Unused method argument: mock_fetch_html
(ARG002)
183-183: Unused method argument: mock_fetch_html
(ARG002)
221-221: Unused method argument: mock_fetch_html
(ARG002)
225-225: Do not assert blind exception: Exception
(B017)
250-250: Unused method argument: mock_fetch_html
(ARG002)
264-264: Unused method argument: mock_fetch_html
(ARG002)
270-270: Unused method argument: mock_fetch_html
(ARG002)
289-289: Unused method argument: mock_fetch_html
(ARG002)
307-307: Unused method argument: mock_fetch_html
(ARG002)
313-313: Unused method argument: mock_fetch_html
(ARG002)
319-319: Unused method argument: mock_fetch_html
(ARG002)
327-327: Unused method argument: mock_fetch_html
(ARG002)
333-333: Unused method argument: mock_fetch_html
(ARG002)
347-347: Unused method argument: mock_fetch_html
(ARG002)
352-352: Unused method argument: mock_fetch_html
(ARG002)
371-371: Unused method argument: mock_fetch_html
(ARG002)
379-379: Unused method argument: mock_fetch_html
(ARG002)
387-387: Unused method argument: mock_fetch_html
(ARG002)
403-403: Unused method argument: mock_fetch_html
(ARG002)
410-410: Unused method argument: mock_fetch_html
(ARG002)
416-416: Unused method argument: mock_fetch_html
(ARG002)
423-423: Unused method argument: mock_fetch_html
(ARG002)
430-430: Unused method argument: mock_fetch_html
(ARG002)
436-436: Unused method argument: mock_fetch_html
(ARG002)
452-452: Unused method argument: mock_fetch_html
(ARG002)
464-466: try-except-continue detected, consider logging the exception
(S112)
464-464: Do not catch blind exception: Exception
(BLE001)
471-471: Unused method argument: mock_fetch_html
(ARG002)
531-531: Unused method argument: mock_fetch_html
(ARG002)
537-537: Unused method argument: mock_fetch_html
(ARG002)
543-543: Unused method argument: mock_fetch_html
(ARG002)
548-548: Do not assert blind exception: Exception
(B017)
563-563: Unused method argument: mock_fetch_html
(ARG002)
585-585: Unused method argument: mock_fetch_html
(ARG002)
626-626: Unused method argument: mock_fetch_html
(ARG002)
634-634: Unused method argument: mock_fetch_html
(ARG002)
654-654: Unused method argument: mock_fetch_html
(ARG002)
693-693: Unused method argument: mock_fetch_html
(ARG002)
701-701: Unused method argument: mock_fetch_html
(ARG002)
710-710: Unused method argument: mock_fetch_html
(ARG002)
720-720: Unused method argument: mock_fetch_html
(ARG002)
730-730: Unused method argument: mock_fetch_html
(ARG002)
738-738: Unused method argument: mock_fetch_html
(ARG002)
792-792: Unused method argument: mock_fetch_html
(ARG002)
797-797: Unused method argument: mock_fetch_html
(ARG002)
824-824: Unused method argument: mock_fetch_html
(ARG002)
832-832: Unused method argument: mock_fetch_html
(ARG002)
855-855: Unused method argument: mock_fetch_html
(ARG002)
871-871: Unused method argument: mock_fetch_html
(ARG002)
876-876: Do not assert blind exception: Exception
(B017)
879-879: Unused method argument: mock_fetch_html
(ARG002)
885-885: Unused method argument: mock_fetch_html
(ARG002)
906-906: Unused method argument: mock_fetch_html
(ARG002)
911-911: Unused method argument: mock_fetch_html
(ARG002)
930-930: Unused method argument: mock_fetch_html
(ARG002)
948-948: Unused method argument: mock_fetch_html
(ARG002)
957-957: Unused method argument: mock_fetch_html
(ARG002)
970-970: Unused method argument: mock_fetch_html
(ARG002)
982-982: Unused method argument: mock_fetch_html
(ARG002)
990-990: Unused method argument: mock_fetch_html
(ARG002)
1003-1003: Unused method argument: mock_fetch_html
(ARG002)
1023-1023: Unused method argument: mock_fetch_html
(ARG002)
1028-1028: Do not assert blind exception: Exception
(B017)
1041-1041: Unused method argument: mock_fetch_html
(ARG002)
1046-1046: Unused method argument: mock_fetch_html
(ARG002)
1066-1066: Unused method argument: mock_fetch_html
(ARG002)
1079-1079: Unused method argument: mock_fetch_html
(ARG002)
1087-1087: Unused method argument: mock_fetch_html
(ARG002)
1095-1095: Unused method argument: mock_fetch_html
(ARG002)
1113-1113: Unused method argument: mock_fetch_html
(ARG002)
1132-1132: Unused method argument: mock_fetch_html
(ARG002)
1147-1147: Unused method argument: mock_fetch_html
(ARG002)
1160-1160: Unused method argument: mock_fetch_html
(ARG002)
1172-1172: Unused method argument: mock_fetch_html
(ARG002)
1187-1187: Unused method argument: mock_fetch_html
(ARG002)
1200-1200: Unused method argument: mock_fetch_html
(ARG002)
1205-1205: Do not assert blind exception: Exception
(B017)
1208-1208: Unused method argument: mock_fetch_html
(ARG002)
1214-1214: Unused method argument: mock_fetch_html
(ARG002)
1226-1226: Unused method argument: mock_fetch_html
(ARG002)
1242-1242: Unused method argument: mock_fetch_html
(ARG002)
1255-1255: Unused method argument: mock_fetch_html
(ARG002)
1263-1263: Local variable leave_count is assigned to but never used
Remove assignment to unused variable leave_count
(F841)
⏰ 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). (4)
- GitHub Check: validate-docs
- GitHub Check: Test on Python 3.11
- GitHub Check: validate-betterdocs
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
tests/lib/test_teams.py (1)
1-1271: Overall tests look consistent with the refactored teams APIAside from the specific points above (tautological invalid-ID assert, broad
except Exceptionin the multi-team roster test, blind exception types in immutability checks, and the unusedleave_count), the rest of the test suite reads clearly and aligns with the currentteams.info,roster,transactions,placements, andprevious_playersbehavior. The use of specific fixtures and tolerant date assertions should make the dateutil-based parsing changes robust.Once the noted fixes/adjustments are addressed and fixture mappings verified, this file looks in good shape.
betterdocs/lib/layout.shared.tsx (1)
21-27: Docs nav link addition looks goodThe new
linksentry cleanly exposes/docsin the header andactive: 'nested-url'will keep nested doc routes highlighted. No issues from a layout/shared perspective.betterdocs/app/(home)/page.tsx (1)
110-117: Incorrect review comment — HTML entities in JSX literal text are decoded correctlyThe review comment mischaracterizes how React handles HTML entities. In JSX literal text (outside JavaScript expressions), React decodes HTML entities like
{and}into their corresponding characters{and}. In JSX literal text you can use HTML entities directly:<div>{ }</div>will render{ }in the page.The original code:
<span className="text-[#a5d6ff]">"Found {results.total_results} results"</span>is in JSX literal text and will render correctly as
"Found {results.total_results} results"for users. The proposed fix introduces unnecessary complexity without solving a real problem.Likely an incorrect or invalid review comment.
| subsets: ['latin'], | ||
| }); | ||
|
|
||
| export default function Layout({ children }: LayoutProps<'/'>) { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that LayoutProps is properly imported or globally declared
LayoutProps<'/'> is referenced in the function signature, but there’s no local import in this file. If LayoutProps isn’t provided via a global type declaration, this will fail type‑checking and should be imported (typically from fumadocs-ui/layouts/shared).
🏁 Script executed:
#!/bin/bash
# Check imports and current state of layout.tsx
cat betterdocs/app/layout.tsx | head -20Length of output: 691
🏁 Script executed:
#!/bin/bash
# Search for LayoutProps definition or import in the codebase
rg -n "LayoutProps" --type=ts --type=tsx -C2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check if fumadocs-ui is a dependency
cat package.json | grep -A5 -B5 "fumadocs"Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check tsconfig for type roots or global declarations
cat tsconfig.json 2>/dev/null || cat tsconfig*.json 2>/dev/null || echo "No tsconfig found"Length of output: 17
🏁 Script executed:
#!/bin/bash
# Find package.json location
fd -name "package.json" -type fLength of output: 230
🏁 Script executed:
#!/bin/bash
# Search for LayoutProps without type restrictions (broader search)
rg -n "LayoutProps" -C2Length of output: 1539
🏁 Script executed:
#!/bin/bash
# Look for type declaration files
fd -name "*.d.ts" -type f | head -20Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check directory structure to understand project layout
ls -la betterdocs/ 2>/dev/null | head -20Length of output: 1197
🏁 Script executed:
#!/bin/bash
# View complete layout.tsx file
cat betterdocs/app/layout.tsxLength of output: 710
🏁 Script executed:
#!/bin/bash
# Check tsconfig.json for type roots and global declarations
cat betterdocs/tsconfig.jsonLength of output: 706
🏁 Script executed:
#!/bin/bash
# View package.json to check fumadocs-ui dependency
cat betterdocs/package.jsonLength of output: 1163
🏁 Script executed:
#!/bin/bash
# Search for any ambient type declarations or global LayoutProps
rg -n "declare.*LayoutProps|global.*LayoutProps" betterdocs/Length of output: 0
🌐 Web query:
fumadocs-ui LayoutProps export 15.8.5
💡 Result:
Short answer: In fumadocs-ui v15.8.5 the layout props types you should use are exported from the layouts/shared module — e.g. BaseLayoutProps (import: import type { BaseLayoutProps } from 'fumadocs-ui/layouts/shared'). Examples in the docs also show using a LayoutProps generic for page components (e.g. Layout({ children }: LayoutProps<'/docs'>)). [1][2]
Sources:
- Fumadocs docs — Layouts / Docs Layout (shows BaseLayoutProps usage). [1]
- Fumadocs / npm docs example (uses LayoutProps<'/docs'> in layout component). [2]
Add missing LayoutProps import from fumadocs-ui/layouts/shared
LayoutProps<'/'> is used in the function signature but not imported. In fumadocs-ui v15.8.5, LayoutProps should be imported from 'fumadocs-ui/layouts/shared'. This will cause a TypeScript compilation error.
Required changes:
betterdocs/app/layout.tsxline 6: Addimport type { LayoutProps } from 'fumadocs-ui/layouts/shared';betterdocs/app/docs/layout.tsxline 5: Same import neededbetterdocs/app/(home)/layout.tsxline 4: Same import needed
🤖 Prompt for AI Agents
In betterdocs/app/layout.tsx around line 6, add the missing type import "import
type { LayoutProps } from 'fumadocs-ui/layouts/shared';" so the function
signature LayoutProps<'/'> compiles; likewise add the same import in
betterdocs/app/docs/layout.tsx at line 5 and in betterdocs/app/(home)/layout.tsx
at line 4 — add the import as a type-only import at the top of each file to
satisfy TypeScript without emitting runtime code.
| def test_invalid_team_id_returns_none(self, mock_fetch_html): | ||
| """Test that invalid team ID returns None.""" | ||
| team = vlr.teams.info(team_id=999999999) | ||
| # Should handle gracefully | ||
| assert team is None or team is not None |
There was a problem hiding this comment.
Fix tautological assertion in test_invalid_team_id_returns_none
assert team is None or team is not None (Line 268) is always true, so this test doesn’t verify the behavior described in the docstring.
If the contract is “invalid team ID returns None”, make the assertion explicit:
def test_invalid_team_id_returns_none(self, mock_fetch_html):
"""Test that invalid team ID returns None."""
team = vlr.teams.info(team_id=999999999)
- # Should handle gracefully
- assert team is None or team is not None
+ # Should handle gracefully by returning None
+ assert team is NoneThis will catch regressions in vlr.teams.info for invalid IDs.
📝 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.
| def test_invalid_team_id_returns_none(self, mock_fetch_html): | |
| """Test that invalid team ID returns None.""" | |
| team = vlr.teams.info(team_id=999999999) | |
| # Should handle gracefully | |
| assert team is None or team is not None | |
| def test_invalid_team_id_returns_none(self, mock_fetch_html): | |
| """Test that invalid team ID returns None.""" | |
| team = vlr.teams.info(team_id=999999999) | |
| # Should handle gracefully by returning None | |
| assert team is None |
🧰 Tools
🪛 Ruff (0.14.5)
264-264: Unused method argument: mock_fetch_html
(ARG002)
🤖 Prompt for AI Agents
In tests/lib/test_teams.py around lines 264 to 268, the assertion `assert team
is None or team is not None` is tautological and doesn't test the documented
behavior; replace it with a concrete assertion that enforces the contract (e.g.,
`assert team is None`) so the test fails on regressions where an invalid team ID
no longer returns None; if the intended contract differs, update the docstring
to match and assert the correct expected value instead.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
betterdocs/app/docs/[slug].mdx.ts (2)
6-8: Critical: Path traversal vulnerability in slug handling.The
slugparameter is used directly in the file path without validation or sanitization. An attacker could exploit this by sending requests with path traversal sequences (e.g.,../../../etc/passwd) to access files outside the intended directory.Apply this diff to sanitize the slug parameter:
export default function handler(req: NextApiRequest, res: NextApiResponse) { const { slug } = req.query; + + // Validate slug is a string and contains only safe characters + if (!slug || typeof slug !== 'string' || !/^[\w-]+$/.test(slug)) { + res.status(400).send('Invalid slug parameter'); + return; + } + + // Use path.basename to prevent directory traversal const filePath = path.join(process.cwd(), 'content/docs', `${slug}.mdx`);
1-6: Migrate to Next.js app directory Route Handlers—current code uses incompatible pages directory pattern.The file is located in the app directory (
app/docs/[slug].mdx.ts) but uses the pages directory API route pattern. Next.js 15.5.6 app directory requires Route Handlers with nativeRequest/Responseobjects, notNextApiRequest/NextApiResponse. The current code will not work correctly.Update to use the Route Handlers pattern:
// app/docs/[slug]/route.ts import fs from 'fs'; import path from 'path'; import { NextResponse } from 'next/server'; export async function GET( request: Request, { params }: { params: { slug: string } } ) { const { slug } = params; const filePath = path.join(process.cwd(), 'content/docs', `${slug}.mdx`); try { const content = fs.readFileSync(filePath, 'utf8'); return new NextResponse(content, { headers: { 'Content-Type': 'text/plain' }, }); } catch (_err) { return new NextResponse('File not found', { status: 404 }); } }
🧹 Nitpick comments (5)
betterdocs/app/docs/[slug].mdx.ts (2)
14-16: Approve the unused variable convention; consider adding error logging.The rename from
errto_errcorrectly signals an intentionally unused variable. However, for production debugging and monitoring, consider logging the error before returning the 404 response.- } catch (_err) { + } catch (err) { + console.error('[MDX Handler] Error reading file:', { slug, error: err }); res.status(404).send('File not found'); }
10-13: Consider async file operations for better performance.The synchronous
fs.readFileSyncblocks the Node.js event loop. For improved scalability, consider using async file operations withfs.promises.+import { promises as fs } from 'fs'; -export default function handler(req: NextApiRequest, res: NextApiResponse) { +export default async function handler(req: NextApiRequest, res: NextApiResponse) { const { slug } = req.query; const filePath = path.join(process.cwd(), 'content/docs', `${slug}.mdx`); try { - const content = fs.readFileSync(filePath, 'utf8'); + const content = await fs.readFile(filePath, 'utf8'); res.setHeader('Content-Type', 'text/plain'); res.status(200).send(content); } catch (_err) {betterdocs/components/tabs.unstyled.tsx (1)
98-98: Consider removingonChangefrom dependency arrays.Since
onChangeis created withuseEffectEvent(line 84), it provides a stable reference across renders by design. The purpose ofuseEffectEventis to avoid including event handlers in dependency arrays while still accessing the latest values. IncludingonChangein these dependency arrays is redundant.Apply this diff to remove the redundant dependencies:
- }, [groupId, persist, onChange]); + }, [groupId, persist]);- }, [valueToIdMap, onChange]); + }, [valueToIdMap]);Also applies to: 111-111
betterdocs/app/(home)/page.tsx (1)
13-16: Consider sourcing the version dynamically.The hardcoded version "v1.3.0" may drift out of sync with the actual package version over time. Consider importing the version from
package.jsonor using an environment variable to ensure consistency.Example approach:
import packageJson from '@/../package.json'; // Then use: {packageJson.version}betterdocs/components/installation-command.tsx (1)
5-5: Remove or implement the unusedlabelparameter.The
labelparameter is prefixed with an underscore (indicating it's intentionally unused) but still required in the component's type signature. This creates confusion and an unnecessary API surface.Choose one of the following approaches:
Option 1: Remove the unused parameter entirely
-export function InstallationCommand({ command, label: _label }: { command: string; label: string }) { +export function InstallationCommand({ command }: { command: string }) {Then update the usage in
betterdocs/app/(home)/page.tsx:-<InstallationCommand command="pip install vlrdevapi" label="pip" /> -<InstallationCommand command="uv add vlrdevapi" label="uv" /> +<InstallationCommand command="pip install vlrdevapi" /> +<InstallationCommand command="uv add vlrdevapi" />Option 2: If the label is intended for future display, implement it now
export function InstallationCommand({ command, label }: { command: string; label: string }) { const [copied, setCopied] = useState(false); const handleCopy = () => { navigator.clipboard.writeText(command); setCopied(true); setTimeout(() => setCopied(false), 2000); }; return ( <div className="flex items-center justify-between gap-2 border border-input bg-zinc-100 dark:bg-neutral-900 px-4 py-2.5 font-mono text-sm text-zinc-900 dark:text-white w-full max-w-md hover:border-primary/50 transition-colors"> + <span className="sr-only">{label}</span> <div className="flex items-center gap-2 overflow-hidden">
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
betterdocs/app/(home)/page.tsx(1 hunks)betterdocs/app/docs/[slug].mdx.ts(1 hunks)betterdocs/components/installation-command.tsx(1 hunks)betterdocs/components/tabs.unstyled.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
betterdocs/app/(home)/page.tsx (1)
betterdocs/components/installation-command.tsx (1)
InstallationCommand(5-33)
🪛 Biome (2.1.2)
betterdocs/components/tabs.unstyled.tsx
[error] 81-81: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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). (4)
- GitHub Check: validate-docs
- GitHub Check: validate-betterdocs
- GitHub Check: Test on Python 3.11
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
betterdocs/components/tabs.unstyled.tsx (1)
81-81: Static analysis warning is a false positive.The conditional hook usage flagged by Biome is a standard React pattern for supporting both controlled and uncontrolled component modes. The condition (
_value === undefined) is evaluated once at mount time and never changes during the component's lifecycle, making this pattern safe.betterdocs/app/(home)/page.tsx (2)
166-263: LGTM!The
FeatureCardand icon components are well-structured with clean implementations. The optionalclassNameprops on icons provide good extensibility for future styling needs.
4-164: Well-structured landing page with good accessibility.The page follows semantic HTML structure, maintains proper heading hierarchy, and includes responsive design patterns. The hero section, features grid, code showcase, and footer create a professional and informative layout.
betterdocs/components/installation-command.tsx (1)
14-33: Well-implemented copy-to-clipboard component.The component provides good user experience with:
- Clear visual command presentation with monospace font and dollar sign prompt
- Accessible copy button with proper
aria-label- Visual feedback with icon toggle (copy → check with green color)
- Clean, responsive layout with hover states
The icon implementations are properly structured SVG components.
Also applies to: 35-68
| const handleCopy = () => { | ||
| navigator.clipboard.writeText(command); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }; |
There was a problem hiding this comment.
Add error handling for clipboard operations.
The navigator.clipboard.writeText() call returns a Promise that can fail (e.g., in non-HTTPS contexts, when permissions are denied, or if the API is unavailable). The current implementation sets the "copied" state regardless of whether the operation succeeds.
Apply this diff to handle errors properly:
- const handleCopy = () => {
- navigator.clipboard.writeText(command);
- setCopied(true);
- setTimeout(() => setCopied(false), 2000);
+ const handleCopy = async () => {
+ try {
+ await navigator.clipboard.writeText(command);
+ setCopied(true);
+ setTimeout(() => setCopied(false), 2000);
+ } catch (error) {
+ // Optionally: show error state or fallback to different copy method
+ console.error('Failed to copy to clipboard:', error);
+ }
};📝 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.
| const handleCopy = () => { | |
| navigator.clipboard.writeText(command); | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }; | |
| const handleCopy = async () => { | |
| try { | |
| await navigator.clipboard.writeText(command); | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| } catch (error) { | |
| // Optionally: show error state or fallback to different copy method | |
| console.error('Failed to copy to clipboard:', error); | |
| } | |
| }; |
1 participant
Summary by CodeRabbit
Breaking Changes
tierfield from event list responsesNew Features
Bug Fixes
Documentation
Chores
Tests