Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughVersion bump v1.9.0 → v1.9.1 across build/packaging metadata; Go toolchain upgraded to 1.25.x; many TUI model types in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 22.78%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_fetch_json_http_500 |
1.8 ms | 1.5 ms | +21.11% |
| ⚡ | test_fetch_json_with_nested_data |
1.8 ms | 1.5 ms | +22.51% |
| ⚡ | test_fetch_json_connection_error |
1.5 ms | 1.2 ms | +22.63% |
| ⚡ | test_fetch_json_empty_object |
1.8 ms | 1.4 ms | +21.4% |
| ⚡ | test_fetch_json_success |
1.8 ms | 1.4 ms | +21.74% |
| ⚡ | test_fetch_json_invalid_json |
1.8 ms | 1.5 ms | +19.88% |
| ⚡ | test_fetch_json_timeout |
1.5 ms | 1.2 ms | +22.78% |
| ⚡ | test_fetch_json_http_404 |
1.8 ms | 1.5 ms | +21.23% |
Comparing 1.9.1 (555f9e3) with main (4933545)
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
96-99:⚠️ Potential issue | 🟡 MinorGo version mismatch between CI and go.mod.
The CI workflow uses Go
1.24.6(line 99), butgo.modspecifiesgo 1.25.8. This version mismatch could lead to build inconsistencies or failures if the codebase uses features from Go 1.25. Consider aligning these versions.Proposed fix
- name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.24.6' + go-version: '1.25.8'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 96 - 99, CI uses actions/setup-go@v5 with go-version: '1.24.6' while go.mod declares go 1.25.8; update the workflow to use the same Go version as go.mod by changing the actions/setup-go@v5 step's go-version value from '1.24.6' to '1.25.8' (or alternatively update go.mod if you intend to pin CI to 1.24.x) so the setup step and the module declaration match.
🧹 Nitpick comments (5)
docs/index.md (1)
20-24: Use more descriptive alt text for demo images.Current alt text (
demo-vg,demo-tcg) is terse. Prefer descriptive text so screen readers convey meaning.♿ Suggested doc tweak
- + ... - +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.md` around lines 20 - 24, Replace the terse image alt text "demo-vg" and "demo-tcg" with descriptive, accessible phrases that convey the content and purpose of each demo image (e.g., describe that the first GIF shows the video-game style CLI demo and the second shows the trading-card CLI workflow), updating the alt attribute for the two Markdown image lines so screen readers can understand what each GIF demonstrates.docs/Infrastructure_Guide/aws.md (1)
210-210: Consider enabling bucket versioning for data protection.The instruction disables bucket versioning, which removes protection against accidental deletions or overwrites. For production assets, enabling versioning is generally recommended as it provides recovery options and data safety.
Note: The AI summary mentions "enabling bucket versioning" but the code instructs to disable it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/Infrastructure_Guide/aws.md` at line 210, The docs step currently reads "Disable bucket versioning" but the review and intent recommend enabling versioning; update the instruction text (step 7) to "Enable bucket versioning" and align the surrounding explanation to recommend versioning for production assets, ensuring the summary no longer contradicts the step; verify all mentions of "bucket versioning" in this section consistently state it should be enabled for data protection and recovery.card_data/pipelines/soda/checks_sets.yml (1)
3-3: Consider adding a warning threshold and check name for better observability.Line 3 is fine, but adding a warning band (plus a named fail check) makes regressions easier to detect before hard failure.
Suggested Soda check structure
checks for sets: # Row count validation - - row_count > 50 + - row_count > 50: + name: Minimum row count check + - row_count > 55: + warn: + when fail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pipelines/soda/checks_sets.yml` at line 3, Replace the unnamed single-expression check with a named fail/warn check: create a check like name: min_row_count and convert the single condition into explicit fail and warn thresholds (for example use fail: row_count < 50 and warn: row_count < 100) so you have a clear check name and an early warning band before hard failure.cmd/search/model_input.go (1)
14-14: Consider making these helper functions unexported or adding a wrapper for the public API.All four helper functions (
UpdateInput,RenderInput,UpdateSelection,RenderSelection) are exported but accept the unexportedmodeltype. They're only called from within thecmd/searchpackage and test files. If these are exported solely to enable direct testing, consider:
- Making them unexported (
updateInput,renderInput, etc.) and accessing them through the publicUpdate()andView()methods in tests, or- If direct testing is critical, keep them exported but add a brief doc comment explaining why they're exported despite the unexported parameter type.
Currently, external code cannot use these functions meaningfully since the
modeltype isn't accessible.Also applies to: 36, 12, 66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/search/model_input.go` at line 14, The four helper functions UpdateInput, RenderInput, UpdateSelection, and RenderSelection are exported while taking the unexported model type, which prevents meaningful external use; either make them unexported (rename to updateInput, renderInput, updateSelection, renderSelection) and update tests to call the public Update() and View() methods instead, or keep them exported but add concise doc comments above each explaining they are exported only for internal package tests despite accepting the unexported model type; reference the public methods Update() and View() when updating tests or comments so callers know the intended public API.docs/nginx.conf (1)
6-6: Consider tightening CSP by phasing out'unsafe-inline'for scripts.Current policy is a solid start, but allowing inline scripts materially reduces CSP effectiveness. If feasible, move to nonce/hash-based script allowances.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/nginx.conf` at line 6, Tighten the Content-Security-Policy in the add_header Content-Security-Policy directive by removing 'unsafe-inline' from script-src and switching to a nonce- or hash-based allowlist (e.g., replace "script-src 'self' 'unsafe-inline';" with "script-src 'self' 'nonce-<RANDOM>';" or include specific script hashes); implement server-side generation/injection of a per-response nonce into both the header and any inline <script> tags (or compute and include script hashes) so scripts still run securely without 'unsafe-inline'; ensure the header string and any template rendering logic that injects nonces are updated together so add_header Content-Security-Policy and your HTML generation use the same nonce value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/search/model_selection.go`:
- Line 12: The exported functions UpdateSelection and RenderSelection should be
renamed to unexported updateSelection and renderSelection to match the
unexported parameter type model; change the function declarations
(UpdateSelection -> updateSelection, RenderSelection -> renderSelection), update
all internal calls and references in the package to use the new lowercase names,
and run/bump any imports or tests that referenced the old exported names to
ensure no external API breakage (these functions are package-internal so only
internal callers should be updated).
In `@docs/commands.md`:
- Around line 158-174: Update the `tcg` command docs to clearly separate default
CLI behavior from the `--web` flag: state that running `poke-cli tcg` launches
an interactive terminal dashboard with tabs (Overview, Standings, Decks,
Countries) for selecting tournaments and viewing stats, while running `poke-cli
tcg --web` (or `-w`) opens the Streamlit web dashboard in the browser; mention
that `--web | -w` triggers the external browser UI and that the default command
remains in-terminal interaction.
In `@docs/Infrastructure_Guide/aws.md`:
- Around line 238-240: Remove or clarify the duplicate S3 public-access
instruction: either delete the redundant list items (the numbered steps that
repeat "block all public access" under the Permissions tab) or explicitly state
that step 6 (the bucket creation step which already blocks public access)
performs this action and that no further changes are needed unless you
temporarily enabled public access for testing; reference the existing "Block
public access" section and the numbered S3 permission steps so readers know
which step is authoritative.
- Line 242: Finish the incomplete sentence in the "Bucket Policy" section by
providing a concrete step-by-step policy to restrict S3 access to only the
CloudFront origin: explain that the user should create or update the S3 bucket
policy to allow s3:GetObject on the bucket ARN for the CloudFront Origin Access
Identity (OAI) or Origin Access Control (OAC) principal, include the canonical
user or AWS principal identifier for that OAI/OAC, explicitly deny public
access, and show the policy fields to set (Version, Statement with Effect,
Principal, Action, Resource, and an optional Condition). Also instruct to apply
the policy to the bucket and confirm the CloudFront distribution's Origin is
configured to use the referenced OAI/OAC so CloudFront can fetch objects while
direct public S3 access remains blocked.
In `@testdata/main_latest_flag.golden`:
- Line 5: The test is brittle because flags/version_test.go asserts an exact
live GitHub tag that changes; update the test to avoid hardcoding the moving
value by stubbing the GitHub client/HTTP response used in flags/version.go (or
replacing the exact equality check with a format/presence assertion).
Specifically, in flags/version_test.go mock the function or HTTP call that
fetches the latest release (the same codepath used by getLatestVersion /
whatever fetcher in flags/version.go) to return a deterministic tag (e.g.,
"v1.9.0") or adjust the golden verification to assert the tag format/presence
rather than exact match against main_latest_flag.golden. Ensure the test uses
the stubbed response so CI no longer depends on live GitHub data.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 96-99: CI uses actions/setup-go@v5 with go-version: '1.24.6' while
go.mod declares go 1.25.8; update the workflow to use the same Go version as
go.mod by changing the actions/setup-go@v5 step's go-version value from '1.24.6'
to '1.25.8' (or alternatively update go.mod if you intend to pin CI to 1.24.x)
so the setup step and the module declaration match.
---
Nitpick comments:
In `@card_data/pipelines/soda/checks_sets.yml`:
- Line 3: Replace the unnamed single-expression check with a named fail/warn
check: create a check like name: min_row_count and convert the single condition
into explicit fail and warn thresholds (for example use fail: row_count < 50 and
warn: row_count < 100) so you have a clear check name and an early warning band
before hard failure.
In `@cmd/search/model_input.go`:
- Line 14: The four helper functions UpdateInput, RenderInput, UpdateSelection,
and RenderSelection are exported while taking the unexported model type, which
prevents meaningful external use; either make them unexported (rename to
updateInput, renderInput, updateSelection, renderSelection) and update tests to
call the public Update() and View() methods instead, or keep them exported but
add concise doc comments above each explaining they are exported only for
internal package tests despite accepting the unexported model type; reference
the public methods Update() and View() when updating tests or comments so
callers know the intended public API.
In `@docs/index.md`:
- Around line 20-24: Replace the terse image alt text "demo-vg" and "demo-tcg"
with descriptive, accessible phrases that convey the content and purpose of each
demo image (e.g., describe that the first GIF shows the video-game style CLI
demo and the second shows the trading-card CLI workflow), updating the alt
attribute for the two Markdown image lines so screen readers can understand what
each GIF demonstrates.
In `@docs/Infrastructure_Guide/aws.md`:
- Line 210: The docs step currently reads "Disable bucket versioning" but the
review and intent recommend enabling versioning; update the instruction text
(step 7) to "Enable bucket versioning" and align the surrounding explanation to
recommend versioning for production assets, ensuring the summary no longer
contradicts the step; verify all mentions of "bucket versioning" in this section
consistently state it should be enabled for data protection and recovery.
In `@docs/nginx.conf`:
- Line 6: Tighten the Content-Security-Policy in the add_header
Content-Security-Policy directive by removing 'unsafe-inline' from script-src
and switching to a nonce- or hash-based allowlist (e.g., replace "script-src
'self' 'unsafe-inline';" with "script-src 'self' 'nonce-<RANDOM>';" or include
specific script hashes); implement server-side generation/injection of a
per-response nonce into both the header and any inline <script> tags (or compute
and include script hashes) so scripts still run securely without
'unsafe-inline'; ensure the header string and any template rendering logic that
injects nonces are updated together so add_header Content-Security-Policy and
your HTML generation use the same nonce value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40a9407f-1fec-4e55-91d8-d12530e1e8e9
⛔ Files ignored due to path filters (1)
docs/assets/tcg.gifis excluded by!**/*.gif
📒 Files selected for processing (35)
.github/workflows/ci.yml.github/workflows/python_testing.yml.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/defs/extract/tcgcsv/extract_pricing.pycard_data/pipelines/defs/extract/tcgdex/extract_cards.pycard_data/pipelines/soda/checks_sets.ymlcard_data/pipelines/tests/extract_series_test.pycard_data/pipelines/tests/extract_sets_test.pycard_data/pipelines/tests/json_retriever_test.pycard_data/pipelines/tests/secret_retriever_test.pycmd/card/card.gocmd/card/cardlist.gocmd/card/cardlist_test.gocmd/card/imageviewer.gocmd/card/imageviewer_test.gocmd/card/serieslist.gocmd/card/serieslist_test.gocmd/card/setslist.gocmd/card/setslist_test.gocmd/search/model_input.gocmd/search/model_input_test.gocmd/search/model_selection.gocmd/search/model_selection_test.gocmd/search/search.gocmd/search/search_test.godocs/Dockerfiledocs/Infrastructure_Guide/aws.mddocs/commands.mddocs/index.mddocs/nginx.confgo.modnfpm.yamltestdata/main_latest_flag.golden
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/Infrastructure_Guide/aws.md (1)
238-240:⚠️ Potential issue | 🟡 MinorRemove or clarify duplicated public-access instruction.
Line 239 repeats the same “block all public access” instruction already set at Line 209 during bucket creation. This can confuse readers about whether an extra change is required.
Suggested doc tweak
-2. Under the **Permissions** tab, in the **Block public access** section, ensure that block _all_ public access is on. +2. Under the **Permissions** tab, confirm **Block all public access** is still enabled (as configured in Step 6 above).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/Infrastructure_Guide/aws.md` around lines 238 - 240, The step that repeats "Under the **Permissions** tab, in the **Block public access** section, ensure that block _all_ public access is on." is duplicated; either remove the repeated instruction or add a clarifying note referencing the earlier bucket-creation step (e.g., "This was already set during bucket creation—no further action needed") so readers won't be confused; update the sentence in the section containing the bucket policy instructions accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/Infrastructure_Guide/aws.md`:
- Around line 241-261: Replace the fenced ```json block with an indented code
block per MD046: remove the triple backticks and indent each line of the JSON
object by four spaces, preserving the JSON content and indentation (look for the
statement with "Sid": "AllowCloudFrontServicePrincipal" and the "AWS:SourceArn"
ARN line to locate the snippet) so the policy remains identical but uses the
expected indented block style.
- Line 269: The sentence "A static IP address is used to ensure the virtual
machine can reliably connect to the PostgreSQL database on every startup." is
misleading; replace it with guidance that for RDS/private PostgreSQL you do not
need a public static IP—instead explain that connectivity depends on proper
VPC/subnet routing and security group rules (and optionally use a NAT/Elastic IP
only if the EC2 instance requires stable public egress or external access), and
suggest ensuring the EC2 and RDS instances share the correct VPC/subnet and
security-group inbound/outbound rules to allow PostgreSQL traffic.
---
Duplicate comments:
In `@docs/Infrastructure_Guide/aws.md`:
- Around line 238-240: The step that repeats "Under the **Permissions** tab, in
the **Block public access** section, ensure that block _all_ public access is
on." is duplicated; either remove the repeated instruction or add a clarifying
note referencing the earlier bucket-creation step (e.g., "This was already set
during bucket creation—no further action needed") so readers won't be confused;
update the sentence in the section containing the bucket policy instructions
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e27d70c-2552-402c-8523-7ad633994b4b
📒 Files selected for processing (2)
docs/Infrastructure_Guide/aws.mddocs/commands.md
✅ Files skipped from review due to trivial changes (1)
- docs/commands.md
This was
linked to
issues
Apr 3, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores