1.8.4 #224
1.8.4 #224
Conversation
📝 WalkthroughWalkthroughBumps release version to v1.8.4 across CI, build, and packaging; upgrades CodeQL action; replaces Gitleaks with a Docker-based run; adds Markdown link validation and a Docker docs-build job; increases several HTTP timeouts; adds tests and Grafana docs; updates dbt config and codecov settings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
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 |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![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 (2)
docs/Infrastructure_Guide/grafana.md (2)
14-17: Minor grammar improvement needed.Consider hyphenating "nice to have" to "nice-to-have" on line 16 for proper compound adjective formation.
🔎 Proposed fix
-but a nice to have to view more details about the database's performance. The free tier on Grafana Cloud is +but a nice-to-have to view more details about the database's performance. The free tier on Grafana Cloud is
24-24: Hyphenate compound adjective.Consider hyphenating "left side menu" to "left-side menu" for consistency. This pattern appears on lines 24, 39, and 56.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/assets/grafana-dashboard-example.pngis excluded by!**/*.png
📒 Files selected for processing (13)
.github/workflows/ci.yml.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pipelines/tests/extract_sets_test.pycmd/card/cardinfo.gocmd/card/cardlist.gocmd/card/setslist.godocs/Infrastructure_Guide/grafana.mdlychee.tomlnfpm.yamltestdata/main_latest_flag.golden
🧰 Additional context used
🧬 Code graph analysis (1)
card_data/pipelines/tests/extract_sets_test.py (1)
card_data/pipelines/defs/extract/tcgdex/extract_sets.py (1)
extract_sets_data(22-56)
🪛 LanguageTool
docs/Infrastructure_Guide/grafana.md
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...not required for the project, but a nice to have to view more details about the d...
(QB_NEW_EN_HYPHEN)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ... required for the project, but a nice to have to view more details about the data...
(QB_NEW_EN_HYPHEN)
[grammar] ~24-~24: Use a hyphen to join words.
Context: ...point._ 1. On the homepage, on the left side menu, click on the drop-down for co...
(QB_NEW_EN_HYPHEN)
[grammar] ~39-~39: Use a hyphen to join words.
Context: ...ted via the Grafana UI._ 1. On the left side menu, click on Dashboards. 2. I...
(QB_NEW_EN_HYPHEN)
[style] ~48-~48: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2659 characters long)
Context: .../assets/grafana-dashboard-example.png) !!! warning The uid for will need to...
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~56-~56: Use a hyphen to join words.
Context: ...rometheus data source. * On the left side menu, click on the Connections ...
(QB_NEW_EN_HYPHEN)
[grammar] ~57-~57: Ensure spelling is correct
Context: ...or "prom" and then click on the default Promethesus data source. * In the URL of that p...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (2)
- GitHub Check: tests
- GitHub Check: benchmarks
🔇 Additional comments (21)
.goreleaser.yml (1)
17-17: LGTM!Version bump to v1.8.4 aligns with the PR objective and matches updates across other build artifacts.
README.md (1)
5-5: LGTM!Docker image references updated to v1.8.4, aligning with the version bump across the repository.
Also applies to: 99-99, 103-103
nfpm.yaml (1)
4-4: LGTM!Package version updated to v1.8.4, consistent with the release version bump.
Dockerfile (1)
11-11: LGTM!Build-time version flag updated to v1.8.4, matching the release configuration in
.goreleaser.ymland other artifacts.cmd/card/cardlist.go (1)
175-175: Verify the rationale for the 4x timeout increase.The HTTP client timeout increased from 15 seconds to 60 seconds. While this change is applied consistently across card data fetch operations (also in
setslist.goandcardinfo.go), a 60-second timeout is quite long for an interactive CLI tool and may degrade user experience.Please confirm:
- Is there a specific performance issue with the Supabase endpoint that necessitates this increase?
- Have you considered alternative approaches such as response streaming, pagination, or query optimization?
- Should there be user feedback (progress indicator) for operations that may take this long?
cmd/card/setslist.go (1)
125-125: Timeout increased consistently with other card data fetchers.HTTP client timeout increased from 15 to 60 seconds, matching the change in
cardlist.goandcardinfo.go. See the comment oncardlist.goregarding verification of this timeout increase.cmd/card/cardinfo.go (2)
25-25: Timeout increased consistently with other card data fetchers.HTTP client timeout increased from 15 to 60 seconds, matching changes in
cardlist.goandsetslist.go. See the comment oncardlist.gofor verification of this timeout increase.
41-48: Decode logic refactored to avoid timeout during image processing.The image response is now read into memory (up to 10MB limit) before decoding, preventing slow decode operations from triggering HTTP timeouts. This is a reasonable approach for a CLI tool fetching card images, as it separates network I/O concerns from image processing time. The added error handling for
io.ReadAllis appropriate.testdata/main_latest_flag.golden (1)
1-7: Golden file is correct and does not need updating.The golden file correctly shows
v1.8.3, which is the actual latest release on GitHub. The--latestflag queries the GitHub API dynamically at runtime, so the golden file captures the expected output based on GitHub's current state. Although the PR bumps version references tov1.8.4in internal build files (nfpm.yaml, Dockerfile, etc.), that version has not yet been released on GitHub. Oncev1.8.4is released, the test will automatically reflect the new version since it queries GitHub dynamically.Likely an incorrect or invalid review comment.
card_data/pipelines/poke_cli_dbt/dbt_project.yml (3)
2-2: LGTM! Version bump is consistent.The version bump to 1.8.4 aligns with the PR objectives and matches the version updates across other files in this release.
18-21: Table materialization configured.The addition of
+materialized: tablesets the default materialization strategy for all models in thepoke_cli_dbtproject to create physical tables. This is appropriate for production data that needs to be queried frequently.
23-24: Inconsistency with AI summary.The AI summary claims the
on-run-endaction was removed, but it's still present in the code. The code showscreate_relationships()is still configured to run after dbt executions.card_data/pipelines/tests/extract_sets_test.py (2)
12-69: Fixture structure looks good.The mock API response fixture provides comprehensive test data covering multiple series, null values, and edge cases.
72-168: Good test coverage.The tests cover important scenarios including successful extraction, empty sets, and null card counts. The assertions validate both DataFrame structure and content appropriately.
lychee.toml (1)
1-9: LGTM! Reasonable link validation configuration.The Lychee configuration is appropriate for the new link validation job. The 30-second timeout and acceptance of 429 (rate limit) responses are sensible defaults for checking documentation links.
.github/workflows/ci.yml (6)
34-34: LGTM! Version bump is consistent.The version bump to v1.8.4 aligns with the PR objectives and matches updates across other files.
71-82: Gitleaks refactored to Docker-based execution.The switch to running Gitleaks via Docker container is a good improvement for consistency and reproducibility. The
if: always()condition on the SARIF upload ensures results are reported even when secrets are detected.
252-273: Good addition: link validation job.The new
validate-linksjob adds automated checking for broken links in documentation. Thefail: falsesetting is appropriate for a newly introduced check, preventing it from blocking builds while the team addresses any existing broken links.
275-308: Well-structured docs build job.The new
build-docs-docker-imagejob is properly configured with:
- Dependency on link validation ensuring docs quality
- Sparse checkout for efficiency
- Standard Docker build and export pattern
247-250: Inconsistency with AI summary.The AI summary claims emoji were removed from these messages, but both lines still contain emoji (
⚠️ and ✅). The current implementation is fine and provides visual clarity.
58-60: CodeQL action v4 upgrade requires verification of SARIF compliance.The upgrade from v3 to v4 introduces a breaking change: v4 rejects uploads containing multiple SARIF runs with the same tool+category (where v3 only warned). This enforcement is effective July 22, 2025.
Verify that the
results.sarifoutput complies with v4's requirement (each run must have a distinct tool or category). If the workflow uploads SARIF from multiple tools under the same category, the upload will fail. CodeQL-only runs are typically compatible, but confirm there are no other tools contributing to the same SARIF file, or ensure each has a distinct category.
CodSpeed Performance ReportCongrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/Infrastructure_Guide/grafana.md (2)
16-16: Apply hyphenation to compound adjective "nice-to-have".For consistency with Markdown style conventions, compound adjectives should use hyphens.
🔎 Proposed fix
-but a nice to have to view more details about the database's performance. The free tier on Grafana Cloud is +but a nice-to-have to view more details about the database's performance. The free tier on Grafana Cloud is
39-39: Apply hyphenation to compound adjective "left-side menu".Lines 39 and 56 use "left side menu" which should be hyphenated as "left-side menu" when used as a compound adjective.
🔎 Proposed fix
-1. On the left side menu, click on **Dashboards**. +1. On the left-side menu, click on **Dashboards**.and
- * On the left side menu, click on the **Connections** dropdown. Then, choose **Data Sources**. + * On the left-side menu, click on the **Connections** dropdown. Then, choose **Data Sources**.Also applies to: 56-56
card_data/pipelines/tests/extract_sets_test.py (1)
103-123: Consider avoiding in-place fixture modification.The test modifies
mock_api_responsedirectly at line 108. While this works (fixtures are function-scoped), it's clearer to either create modified data within the test or use a separate fixture. This improves test isolation and readability.🔎 Alternative approach without mutation
@pytest.mark.benchmark @responses.activate -def test_extract_sets_data_empty_sets(mock_api_response): +def test_extract_sets_data_empty_sets(): """Test extraction when a series has no sets""" - # Modify one response to have empty sets - mock_api_response["https://api.tcgdex.net/v2/en/series/me"]["sets"] = [] - - for url, response_data in mock_api_response.items(): + mock_responses = { + "https://api.tcgdex.net/v2/en/series/me": { + "id": "me", + "name": "Mega Evolution", + "sets": [], + }, + "https://api.tcgdex.net/v2/en/series/sv": { + "id": "sv", + "name": "Scarlet & Violet", + "sets": [ + { + "id": "sv01", + "name": "Scarlet & Violet", + "cardCount": {"official": 198, "total": 258}, + "logo": "https://example.com/sv01.png", + "symbol": "https://example.com/sv01-symbol.png", + }, + { + "id": "sv02", + "name": "Paldea Evolved", + "cardCount": {"official": 193, "total": 279}, + "logo": "https://example.com/sv02.png", + "symbol": None, + }, + ], + }, + "https://api.tcgdex.net/v2/en/series/swsh": { + "id": "swsh", + "name": "Sword & Shield", + "sets": [ + { + "id": "swsh1", + "name": "Sword & Shield", + "cardCount": {"official": 202, "total": 216}, + "logo": None, + "symbol": "https://example.com/swsh1-symbol.png", + }, + ], + }, + } + + for url, response_data in mock_responses.items(): responses.add( responses.GET, url, json=response_data, status=200, )cmd/card/setslist_test.go (1)
193-217: Well-structured HTTP header validation test.The test correctly validates that all required headers (apikey, Authorization, Content-Type) are sent with the expected values and that the response body is returned.
The hardcoded API key "sb_publishable_oondaaAIQC-wafhEiNgpSQ_reRiEp7j" appears on lines 195 and 198. Consider extracting it to a package-level test constant for easier maintenance if the key changes:
const testSupabaseKey = "sb_publishable_oondaaAIQC-wafhEiNgpSQ_reRiEp7j"This is a low-priority suggestion since public Supabase keys are meant to be exposed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
card_data/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
card_data/pipelines/tests/extract_sets_test.pycmd/card/setslist.gocmd/card/setslist_test.godocs/Infrastructure_Guide/grafana.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/card/setslist.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/card/setslist_test.go (1)
cmd/card/setslist.go (1)
SetsList(77-116)
card_data/pipelines/tests/extract_sets_test.py (2)
card_data/pipelines/definitions.py (1)
defs(12-15)card_data/pipelines/defs/extract/tcgdex/extract_sets.py (1)
extract_sets_data(22-56)
🪛 LanguageTool
docs/Infrastructure_Guide/grafana.md
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...not required for the project, but a nice to have to view more details about the d...
(QB_NEW_EN_HYPHEN)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ... required for the project, but a nice to have to view more details about the data...
(QB_NEW_EN_HYPHEN)
[grammar] ~24-~24: Use a hyphen to join words.
Context: ...point._ 1. On the homepage, on the left side menu, click on the drop-down for co...
(QB_NEW_EN_HYPHEN)
[grammar] ~39-~39: Use a hyphen to join words.
Context: ...ted via the Grafana UI._ 1. On the left side menu, click on Dashboards. 2. I...
(QB_NEW_EN_HYPHEN)
[style] ~48-~48: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2658 characters long)
Context: .../assets/grafana-dashboard-example.png) !!! warning The uid for will need to...
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~56-~56: Use a hyphen to join words.
Context: ...rometheus data source. * On the left side menu, click on the Connections ...
(QB_NEW_EN_HYPHEN)
⏰ 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). (2)
- GitHub Check: tests
- GitHub Check: benchmarks
🔇 Additional comments (11)
docs/Infrastructure_Guide/grafana.md (1)
1-71: Previously flagged typos are now fixed.The two typos flagged in earlier reviews have been corrected: "data source" (line 51) and "Prometheus" (line 57) are now spelled correctly. Good catch on addressing those!
card_data/pipelines/tests/extract_sets_test.py (4)
1-10: LGTM!The imports and path setup are appropriate for the test module. The sys.path manipulation ensures the parent module can be imported correctly.
12-69: Well-structured test fixture.The fixture provides comprehensive mock data covering multiple series and edge cases (including None values for optional fields like logo and symbol). The structure accurately matches the tcgdex API format.
72-101: Comprehensive success path test.The test thoroughly validates the DataFrame structure, row count, column set, and data integrity across multiple series. Good use of the
responseslibrary to mock HTTP calls.
125-168: Excellent null handling test.This test validates proper handling of missing cardCount fields and None values for optional fields. Creating a dedicated
mock_responsesdict (rather than reusing the fixture) is the right approach for this edge case.cmd/card/setslist_test.go (6)
4-6: LGTM!The new imports are appropriate for the HTTP testing functionality added in this file.
219-232: LGTM!Proper validation that non-200 status codes produce errors with meaningful messages containing the status code.
234-239: LGTM!Good validation that malformed URLs are handled gracefully with an error.
241-280: Excellent success path coverage.The test uses proper function replacement for mocking and comprehensively validates:
- Series filtering by ID
- Correct setsIDMap population with name→ID mappings
- Exclusion of non-matching series
- View rendering
The defer pattern ensures proper cleanup of the mock.
282-314: LGTM!Both error cases are well-tested:
TestSetsList_FetchErrorvalidates network error handlingTestSetsList_BadJSONvalidates JSON parsing error handlingBoth tests verify that error messages are descriptive and help identify the failure point.
316-358: Excellent edge case coverage.Both tests validate important scenarios:
TestSetsList_EmptyResult: Ensures empty data sets don't cause crashesTestSetsList_NoMatchingSeries: Validates graceful handling when no series match the requested IDBoth verify that the View still renders without errors, which is important for UX.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/codecov.yml (1)
1-6: Valid Codecov configuration with room for stricter requirements.The configuration is syntactically correct and will work as intended. However, consider the following:
- The 50% coverage target is relatively low. Most mature projects target 70-80%+ coverage.
- The 10% threshold allows coverage to drop by 10 percentage points (e.g., 50% → 40%) before failing, which is quite permissive. Tighter thresholds (1-5%) help prevent coverage regressions.
These settings may be appropriate if you're just starting to track coverage, but consider gradually increasing the target and tightening the threshold as your test suite matures.
Optional: Additional configuration options
You may also want to consider adding:
- patch coverage: Requires new code changes to meet a minimum coverage threshold
- ignore paths: Exclude generated code, vendor directories, etc.
- comment settings: Control how Codecov comments on PRs
Example:
coverage: status: project: default: target: 70% threshold: 3% patch: default: target: 80% ignore: - "vendor/**" - "**/*_test.go"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/codecov.yml.gitignore
💤 Files with no reviewable changes (1)
- .gitignore
⏰ 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). (2)
- GitHub Check: benchmarks
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
codecov.yml (1)
1-6: Consider increasing coverage targets for better code quality.The current configuration sets a 50% coverage target with a 10% threshold, which is quite permissive. While this may be appropriate for an initial rollout, consider:
- Target: Increasing to 70-80% aligns better with industry best practices
- Threshold: Reducing to 5% or less prevents significant coverage drops between commits
- Patch coverage: Adding patch-level checks ensures new code is well-tested
🔎 Suggested enhanced configuration
coverage: status: project: default: - target: 50% - threshold: 10% + target: 70% + threshold: 5% + patch: + default: + target: 80% + threshold: 5%
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codecov.yml
⏰ 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). (2)
- GitHub Check: tests
- GitHub Check: benchmarks
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.