Merged
Conversation
# Conflicts: # .github/workflows/ci.yml # .goreleaser.yml # Dockerfile # README.md # card_data/pipelines/poke_cli_dbt/dbt_project.yml # nfpm.yaml
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughVersion bumped to v1.9.0 across CI/build/manifests. Added a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as poke-cli (tcg)
participant Selector as Tournament Selector
participant Dashboard as Dashboard TUI
participant Supabase as Supabase API
participant Browser as Browser (optional)
User->>CLI: run `poke-cli tcg`
CLI->>Selector: start tournament selector
Selector->>Supabase: fetch tournaments
Supabase-->>Selector: tournaments JSON
Selector->>User: render list (TUI)
User->>Selector: choose tournament
Selector-->>CLI: selected tournament
CLI->>Dashboard: start dashboard for location
Dashboard->>Supabase: fetch standings
Supabase-->>Dashboard: standings JSON
Dashboard->>Dashboard: compute aggregates & render tabs
User->>Dashboard: navigate tabs / exit
Dashboard-->>CLI: return go_back or quit
CLI->>Browser: (if --web) call WebFlag to open Streamlit URL
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was
linked to
issues
Mar 25, 2026
Closed
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/pokemon/pokemon.go (1)
63-67:⚠️ Potential issue | 🟡 MinorInconsistent error output for flag parsing.
Line 64 uses
fmt.Printfwhich prints directly to stdout, whileability.go(line 51) usesfmt.Fprintf(&output, ...)for the same scenario. This inconsistency can cause output ordering issues and makes the output uncapturable via the returned string.Proposed fix for consistency
if err := pf.FlagSet.Parse(args[3:]); err != nil { - fmt.Printf("error parsing flags: %v\n", err) + fmt.Fprintf(&output, "error parsing flags: %v\n", err) pf.FlagSet.Usage() return output.String(), err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pokemon/pokemon.go` around lines 63 - 67, The flag parse error prints to stdout with fmt.Printf causing inconsistent, uncapturable output; change the error handling in the pf.FlagSet.Parse branch to write into the same output buffer used elsewhere (the output variable) — replace fmt.Printf("error parsing flags: %v\n", err) with a fmt.Fprintf(&output, "error parsing flags: %v\n", err) and ensure pf.FlagSet.Usage() also writes into that buffer (or call pf.FlagSet.SetOutput(&output) beforehand) so that the Parse error and Usage both go to output; update the return to continue returning output.String() and err as before.
🧹 Nitpick comments (11)
cmd/tcg/tab_overview.go (1)
11-21: Consider handling negative numbers informatInt.The current implementation doesn't handle negative numbers correctly. For example,
formatInt(-1000)would produce"-,100,0"instead of"-1,000". WhiletotalPlayerswon't be negative, this is a general utility function that could be reused.Proposed fix for negative number support
func formatInt(n int) string { + if n < 0 { + return "-" + formatInt(-n) + } s := strconv.Itoa(n) var result strings.Builder for i, c := range s {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tcg/tab_overview.go` around lines 11 - 21, The formatInt function doesn't handle negatives correctly; update formatInt to detect a leading '-' from strconv.Itoa, strip the sign before inserting commas, build the formatted magnitude using the existing loop (on s or on the absolute string), then prepend the '-' to the result when the original value was negative; keep the function signature formatInt(n int) string and reuse the current strings.Builder logic but operate on the non-sign substring so formatInt(-1000) returns "-1,000".flags/tcg_flagset.go (1)
38-68: Consider callingWait()on the browser process or explicitly detaching.
openCmd.Start()spawns a child process but never callsWait(), leaving a zombie process until the CLI exits. For a short-lived CLI this is typically fine, but for cleaner resource management, consider:
- Calling
openCmd.Wait()in a goroutine, or- Using
openCmd.Run()in a goroutine if you want to wait for completionSince the browser should outlive the CLI, the current approach is acceptable in practice.
Optional: Clean up with goroutine
if err := openCmd.Start(); err != nil { fmt.Fprintf(&output, "Failed to open browser: %v\nVisit manually:\n%s\n", err, url) return output.String(), nil } + + // Reap the child process in background to avoid zombies + go func() { _ = openCmd.Wait() }() fmt.Fprintf(&output, "Opening: %s\n", url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flags/tcg_flagset.go` around lines 38 - 68, The WebFlag function spawns a browser process with openCmd.Start() but never reaps it, which can leave a zombie; update WebFlag to either call openCmd.Wait() in a goroutine after Start() (e.g., go func() { _ = openCmd.Wait() }()) to reap the child, or replace Start() with spawning via a detached/run-in-goroutine strategy (e.g., run openCmd.Run() inside a goroutine) so the browser process is properly waited on without blocking the CLI; modify the code paths that use openCmd (the Start call and its error handling) to invoke one of these approaches to ensure the child process is cleaned up.cmd/tcg/tab_overview_test.go (1)
69-90: Consider adding a negative number test case.If
formatIntis updated to handle negative numbers, adding a test case like{-1000, "-1,000"}would ensure correctness. Currently, this would fail with the existing implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tcg/tab_overview_test.go` around lines 69 - 90, Add a negative-number test to TestFormatInt: extend the tests slice in TestFormatInt to include the case { -1000, "-1,000" } and ensure the t.Run label remains unique (e.g., use tt.want or include the numeric value) so the test asserts formatInt(-1000) returns "-1,000"; if formatInt currently doesn't handle negatives, update formatInt to preserve the sign and format the absolute value with commas before returning the signed string.web/.python-version (1)
2-2: Consider removing the trailing blank line.The
.python-versionfile typically contains only the version number without trailing blank lines. While this doesn't affect functionality (version managers read the first line), removing it aligns with standard conventions.✂️ Proposed fix
3.12 -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/.python-version` at line 2, The .python-version file contains a trailing blank line; remove the extra newline so the file consists solely of the Python version string on the first line (no trailing blank line) to match convention and avoid unnecessary whitespace.styling/list_test.go (1)
80-88: Redundant assertion.The check on lines 85-87 is redundant since
len(output) == 0is equivalent tooutput == "", which is already checked on lines 81-83. Consider removing the duplicate check.🧹 Proposed fix
output := buf.String() if output == "" { t.Error("Expected non-empty output for selected item") } - - if len(output) == 0 { - t.Error("Selected item should produce rendered output") - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styling/list_test.go` around lines 80 - 88, Remove the redundant assertion that checks len(output) == 0 because it duplicates the earlier output == "" check; keep a single assertion that the rendered output is non-empty (the t.Error call referencing output) and delete the duplicate t.Error block that uses len(output); locate the duplicate by searching for the variable name output and the second t.Error invocation in the test in styling/list_test.go and remove that duplicate check.card_data/pyproject.toml (1)
29-37: Inconsistent version pin fordagster-dg-cliin dev dependencies.Line 12 pins
dagster-dg-cli==1.12.7in production dependencies, but line 31 indevleaves it unpinned. This could lead to version mismatches between environments.🔧 Suggested fix
[dependency-groups] dev = [ "dagster-webserver==1.12.7", - "dagster-dg-cli", + "dagster-dg-cli==1.12.7", "dagster-dbt==0.28.7",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pyproject.toml` around lines 29 - 37, The dev dependency list leaves dagster-dg-cli unpinned causing potential version drift with the production pin; update the dev array to pin "dagster-dg-cli==1.12.7" to match the production dependency, ensuring consistency between the two dependency groups (look for the dagster-dg-cli entry in the dev = [...] block and align it with the existing pinned version).cmd/tcg/tab_countries.go (1)
8-15: Consider naming the magic number20for clarity.The hardcoded
20passed tobarChartwould be clearer as a named constant indicating its purpose (e.g., max label width, bar count limit).💡 Optional: Extract constant
+const maxBarLabelWidth = 20 + func countriesContent(s []countryStats, width int) string { items := make([]barChartItem, len(s)) for i, c := range s { items[i] = barChartItem{Label: c.Country, Total: c.Total} } - return barChart(items, width, 20) + return barChart(items, width, maxBarLabelWidth) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tcg/tab_countries.go` around lines 8 - 15, The hardcoded numeric literal 20 in the countriesContent function's call to barChart is a magic number; introduce a clearly named constant (e.g., maxLabelWidth or labelWidthLimit) near the top of the file and replace the literal in countriesContent (and any other uses) with that constant so the intent (label width limit or bar label max length) is explicit when calling barChart(items, width, <constant>).web/pyproject.toml (1)
15-19: Consider consistent dependency pinning strategy.Dev dependencies use
>=(minimum version) while production dependencies use==(exact pin). For consistency withcard_data/pyproject.toml, consider pinning dev dependencies exactly as well.💡 Optional: Pin dev dependencies
[dependency-groups] dev = [ - "pytest>=9.0.2", - "pytest-cov>=7.0.0", + "pytest==9.0.2", + "pytest-cov==7.0.0", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pyproject.toml` around lines 15 - 19, The dev dependency group in web/pyproject.toml uses minimum-version pins for "pytest>=9.0.2" and "pytest-cov>=7.0.0"; update the dependency-group "dev" entries to use exact pins (e.g., change the "pytest" and "pytest-cov" strings to "pytest==<version>" and "pytest-cov==<version>") to match the exact-pin strategy used in card_data/pyproject.toml and ensure consistency across projects.cmd/tcg/tab_standings.go (1)
41-41: Consider adding a comment explaining the height offset.The magic number
14in the height calculation could benefit from a brief comment explaining what it accounts for (e.g., header, footer, padding, tabs).📝 Suggested improvement
- tableHeight := max(height-14, 5) + // Reserve 14 rows for header, footer, tabs, and padding + tableHeight := max(height-14, 5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tcg/tab_standings.go` at line 41, The magic number 14 in the tableHeight calculation (tableHeight := max(height-14, 5)) is unclear; add a brief inline comment next to this expression (or above it near the related function/logic that computes layout in tab_standings.go) explaining what the 14 represents (e.g., combined header/footer/tab/padding heights) so future readers know why we subtract 14 and can adjust it safely.web/Dockerfile (1)
9-9: Pin theuvimage version for reproducible builds.Using
:latestfor external image sources can lead to inconsistent builds over time. Pin the uv image to a specific stable version (e.g.,ghcr.io/astral-sh/uv:0.11.0or later) to ensure reproducible builds across different environments and time periods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/Dockerfile` at line 9, The Dockerfile currently pulls the uv runtime with an unpinned tag in the COPY instruction ("COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv"); update that reference to a specific, tested uv image tag (for example ghcr.io/astral-sh/uv:0.11.0 or whatever stable version you validated) to ensure reproducible builds and replace the ":latest" tag in the COPY --from value accordingly.web/app.py (1)
27-39: Build the tournament dropdown from one row per event.
unique_locations()currently pulls the full standings table and deduplicates it client-side, even thoughrank = 1already gives you one row per tournament elsewhere in this file. This will get slower as more player rows accumulate.♻️ Suggested fix
`@st.cache_data`(ttl=86400) def unique_locations() -> list: result = ( supabase.table("standings") .select("location, text_date") + .eq("rank", 1) .order("start_date") .execute() ) - return list( - dict.fromkeys( - (row["location"], row["text_date"]) for row in result.data - ) # pyrefly: ignore[bad-index, unsupported-operation] - ) + return [ + (row["location"], row["text_date"]) for row in result.data + ] # pyrefly: ignore[bad-index, unsupported-operation]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app.py` around lines 27 - 39, unique_locations() currently fetches the entire "standings" table and deduplicates client-side; change the query to fetch only one row per event by filtering on rank == 1 (use supabase.table("standings").select("location, text_date").eq("rank", 1).order("start_date").execute()), then return the list of (location, text_date) tuples from result.data; this keeps the cache decorator and reduces transferred rows as the dataset grows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 52-53: Change the workflow step that currently uses the action via
"uses: securego/gosec@master" to pin a specific released tag or commit SHA (for
example "uses: securego/gosec@vX.Y.Z" or "uses: securego/gosec@<commit-sha>") to
ensure reproducible, auditable runs; update the CI workflow entry that
references securego/gosec accordingly and choose a stable released version
instead of `@master`.
In `@cmd/tcg/data.go`:
- Around line 51-56: The countryFlag function should reject inputs whose two
characters are not ASCII letters before computing regional-indicator runes:
after converting isoCode to upper case (countryFlag, isoCode, code), verify both
code[0] and code[1] are in the range 'A'..'Z' and return "" if not, then perform
the existing rune math to build the flag; this prevents inputs like "1A" or "*?"
from producing spurious runes.
In `@cmd/tcg/tcg.go`:
- Around line 105-110: The code currently sets model.tournament to
result.selected.Location which discards the event identity; update the
assignment passed into runDashboard so model.tournament carries a stable
tournament identifier (e.g., the entire result.selected struct or a unique field
such as result.selected.ID, or at minimum a composite of Location+Date) instead
of only Location; adjust any downstream consumers of model.tournament (used by
runDashboard/standings fetch) to expect the chosen tournament key so filtering
remains specific to the exact event.
In `@cmd/tcg/tournamentslist.go`:
- Around line 98-123: The initial tea.WindowSizeMsg is being ignored while
m.loading is true, so the list is constructed at the hard-coded 40x16; fix by
caching the last window dimensions on receipt of tea.WindowSizeMsg even when
loading (store into e.g. m.cachedWidth and m.cachedHeight), continue updating
the spinner for spinner.TickMsg as before, and when handling
tournamentsDataMsg/create of m.list use the cachedWidth/cachedHeight (fall back
to 40/16 only if cache is zero) when calling list.New(...) and when calling
m.list.SetWidth/Height so the picker is sized correctly as soon as it is
created. Ensure you reference and update m.cachedWidth/m.cachedHeight in the
tea.WindowSizeMsg case and apply them in the code path that assigns m.list.
In `@web/app_test.py`:
- Around line 3-6: Move the module-level AppTest initialization into a pytest
fixture: replace the top-level call to
AppTest.from_file("app.py").run(timeout=10) with a fixture function (e.g., def
streamlit_app():) that resolves the app path using Path(__file__).parent /
"app.py", calls AppTest.from_file(str(app_path)).run(timeout=10) inside the
fixture, and yields/returns the running AppTest for use by tests; also add the
necessary imports (pytest and Path from pathlib) and keep
DeckStats/PlayersCountrySection/RawStandingsSection imports as needed by tests.
In `@web/app.py`:
- Around line 21-24: run_query currently filters only by location so events with
the same city but different dates will be merged; change the filtering to use a
unique tournament key (e.g., pass and filter by the tuple (location, text_date)
or a dedicated tournament_id) and update call sites (including header()/the
selectbox population and the other query usages around the same selection logic)
so they pass that unique identifier into run_query (and any other query
functions referenced at the same spots) so all downstream queries (standings,
metrics, charts) filter by the unique tournament value rather than just
location.
- Around line 72-85: tournament_info and tournament_stats currently call
data_table(tourney_filter) and immediately index row 0 (e.g., df["iso_code"][0],
df["logo"][0]) which crashes when no rows are returned; modify both functions to
call df = data_table(tourney_filter) once, check if df is empty (e.g., if
df.empty or len(df) == 0) and short-circuit early by rendering an appropriate
empty state or returning before accessing df[0], otherwise proceed to extract
iso_code, logo, text_date, location and render as before; ensure you update all
occurrences that access df row 0 (references: tournament_info, tournament_stats,
and any code blocks using df["..."][0]) to use the existence check first.
- Around line 321-322: The metric currently calls winning_deck.capitalize()
which alters stored deck names; update the st.metric call inside the with col3
block to pass the raw stored string (value=winning_deck) so names are preserved
exactly (locate the st.metric usage that references winning_deck and remove the
.capitalize()).
In `@web/pyproject.toml`:
- Line 4: Replace the placeholder description value ("Add your description
here") in the pyproject.toml's description metadata with a concise, accurate
summary of the web dashboard (e.g., what it does and who it's for); update the
description key to a meaningful sentence that reflects the project's purpose so
package metadata and documentation are informative.
---
Outside diff comments:
In `@cmd/pokemon/pokemon.go`:
- Around line 63-67: The flag parse error prints to stdout with fmt.Printf
causing inconsistent, uncapturable output; change the error handling in the
pf.FlagSet.Parse branch to write into the same output buffer used elsewhere (the
output variable) — replace fmt.Printf("error parsing flags: %v\n", err) with a
fmt.Fprintf(&output, "error parsing flags: %v\n", err) and ensure
pf.FlagSet.Usage() also writes into that buffer (or call
pf.FlagSet.SetOutput(&output) beforehand) so that the Parse error and Usage both
go to output; update the return to continue returning output.String() and err as
before.
---
Nitpick comments:
In `@card_data/pyproject.toml`:
- Around line 29-37: The dev dependency list leaves dagster-dg-cli unpinned
causing potential version drift with the production pin; update the dev array to
pin "dagster-dg-cli==1.12.7" to match the production dependency, ensuring
consistency between the two dependency groups (look for the dagster-dg-cli entry
in the dev = [...] block and align it with the existing pinned version).
In `@cmd/tcg/tab_countries.go`:
- Around line 8-15: The hardcoded numeric literal 20 in the countriesContent
function's call to barChart is a magic number; introduce a clearly named
constant (e.g., maxLabelWidth or labelWidthLimit) near the top of the file and
replace the literal in countriesContent (and any other uses) with that constant
so the intent (label width limit or bar label max length) is explicit when
calling barChart(items, width, <constant>).
In `@cmd/tcg/tab_overview_test.go`:
- Around line 69-90: Add a negative-number test to TestFormatInt: extend the
tests slice in TestFormatInt to include the case { -1000, "-1,000" } and ensure
the t.Run label remains unique (e.g., use tt.want or include the numeric value)
so the test asserts formatInt(-1000) returns "-1,000"; if formatInt currently
doesn't handle negatives, update formatInt to preserve the sign and format the
absolute value with commas before returning the signed string.
In `@cmd/tcg/tab_overview.go`:
- Around line 11-21: The formatInt function doesn't handle negatives correctly;
update formatInt to detect a leading '-' from strconv.Itoa, strip the sign
before inserting commas, build the formatted magnitude using the existing loop
(on s or on the absolute string), then prepend the '-' to the result when the
original value was negative; keep the function signature formatInt(n int) string
and reuse the current strings.Builder logic but operate on the non-sign
substring so formatInt(-1000) returns "-1,000".
In `@cmd/tcg/tab_standings.go`:
- Line 41: The magic number 14 in the tableHeight calculation (tableHeight :=
max(height-14, 5)) is unclear; add a brief inline comment next to this
expression (or above it near the related function/logic that computes layout in
tab_standings.go) explaining what the 14 represents (e.g., combined
header/footer/tab/padding heights) so future readers know why we subtract 14 and
can adjust it safely.
In `@flags/tcg_flagset.go`:
- Around line 38-68: The WebFlag function spawns a browser process with
openCmd.Start() but never reaps it, which can leave a zombie; update WebFlag to
either call openCmd.Wait() in a goroutine after Start() (e.g., go func() { _ =
openCmd.Wait() }()) to reap the child, or replace Start() with spawning via a
detached/run-in-goroutine strategy (e.g., run openCmd.Run() inside a goroutine)
so the browser process is properly waited on without blocking the CLI; modify
the code paths that use openCmd (the Start call and its error handling) to
invoke one of these approaches to ensure the child process is cleaned up.
In `@styling/list_test.go`:
- Around line 80-88: Remove the redundant assertion that checks len(output) == 0
because it duplicates the earlier output == "" check; keep a single assertion
that the rendered output is non-empty (the t.Error call referencing output) and
delete the duplicate t.Error block that uses len(output); locate the duplicate
by searching for the variable name output and the second t.Error invocation in
the test in styling/list_test.go and remove that duplicate check.
In `@web/.python-version`:
- Line 2: The .python-version file contains a trailing blank line; remove the
extra newline so the file consists solely of the Python version string on the
first line (no trailing blank line) to match convention and avoid unnecessary
whitespace.
In `@web/app.py`:
- Around line 27-39: unique_locations() currently fetches the entire "standings"
table and deduplicates client-side; change the query to fetch only one row per
event by filtering on rank == 1 (use
supabase.table("standings").select("location, text_date").eq("rank",
1).order("start_date").execute()), then return the list of (location, text_date)
tuples from result.data; this keeps the cache decorator and reduces transferred
rows as the dataset grows.
In `@web/Dockerfile`:
- Line 9: The Dockerfile currently pulls the uv runtime with an unpinned tag in
the COPY instruction ("COPY --from=ghcr.io/astral-sh/uv:latest /uv
/usr/local/bin/uv"); update that reference to a specific, tested uv image tag
(for example ghcr.io/astral-sh/uv:0.11.0 or whatever stable version you
validated) to ensure reproducible builds and replace the ":latest" tag in the
COPY --from value accordingly.
In `@web/pyproject.toml`:
- Around line 15-19: The dev dependency group in web/pyproject.toml uses
minimum-version pins for "pytest>=9.0.2" and "pytest-cov>=7.0.0"; update the
dependency-group "dev" entries to use exact pins (e.g., change the "pytest" and
"pytest-cov" strings to "pytest==<version>" and "pytest-cov==<version>") to
match the exact-pin strategy used in card_data/pyproject.toml and ensure
consistency across projects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3acab70-c4ca-4b89-b18c-ded19f7f9375
⛔ Files ignored due to path filters (2)
card_data/uv.lockis excluded by!**/*.lockweb/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
.github/workflows/ci.yml.gitignore.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/defs/transform/transform_data.pycard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pipelines/poke_cli_dbt/models/sources.ymlcard_data/pipelines/poke_cli_dbt/models/standings.sqlcard_data/pyproject.tomlcli.gocmd/ability/ability.gocmd/berry/berry.gocmd/card/card.gocmd/card/design.gocmd/card/serieslist.gocmd/card/serieslist_test.gocmd/card/setslist.gocmd/card/setslist_test.gocmd/item/item.gocmd/pokemon/pokemon.gocmd/search/model_input.gocmd/speed/speed.gocmd/tcg/barchart.gocmd/tcg/barchart_test.gocmd/tcg/dashboard.gocmd/tcg/dashboard_test.gocmd/tcg/data.gocmd/tcg/data_test.gocmd/tcg/tab_countries.gocmd/tcg/tab_countries_test.gocmd/tcg/tab_decks.gocmd/tcg/tab_decks_test.gocmd/tcg/tab_overview.gocmd/tcg/tab_overview_test.gocmd/tcg/tab_standings.gocmd/tcg/tab_standings_test.gocmd/tcg/tcg.gocmd/tcg/tcg_test.gocmd/tcg/tournamentslist.gocmd/tcg/tournamentslist_test.gocmd/types/types.gocmd/utils/help.gocmd/utils/help_test.goflags/tcg_flagset.goflags/tcg_flagset_test.gonfpm.yamlstyling/list.gostyling/list_test.gostyling/styling.gotestdata/cli_help.goldentestdata/cli_incorrect_command.goldentestdata/main_latest_flag.goldentestdata/tcg_help.goldentestdata/tcg_too_many_args.goldenweb/.python-versionweb/.streamlit/config.tomlweb/Dockerfileweb/app.pyweb/app_test.pyweb/pyproject.toml
💤 Files with no reviewable changes (1)
- cmd/card/design.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
card_data/pipelines/defs/extract/limitless/extract_standings.py (1)
20-25:⚠️ Potential issue | 🟠 MajorGuard against empty anchor text before indexing
parts.Line 24/Line 25 can raise
IndexErrorwhena.stripped_stringsis empty (e.g., malformed/empty anchors), which would abort extraction.Proposed fix
parts = list(a.stripped_strings) + if not parts: + continue tournaments.append({ "name": parts[0], "date": parts[-1], "link": f"https://labs.limitlesstcg.com{a['href']}", })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pipelines/defs/extract/limitless/extract_standings.py` around lines 20 - 25, The code that builds parts = list(a.stripped_strings) and then indexes parts[0] and parts[-1] can raise IndexError for empty/malformed anchors; in the loop that checks isinstance(a, Tag) (the anchor variable a used before tournaments.append), guard against empty or too-short parts by checking len(parts) (e.g., skip the anchor if not parts or len(parts) < 1/2 depending on expected fields) or fall back to safe defaults before accessing parts[0] and parts[-1], so tournaments.append only uses valid indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@card_data/pipelines/defs/extract/limitless/extract_standings.py`:
- Around line 79-81: The code that builds decklist URLs in extract_standings.py
currently always prefixes href with "https://labs.limitlesstcg.com", which
produces malformed absolute URLs; update the logic where link (a Tag) is
processed (the block appending to row_data using decklist_url) to detect if
decklist_url is already absolute (e.g., starts with "http://" or "https://" or
begins with "//") and only prefix the base host when the href is relative,
ensuring decklist_url remains unchanged for absolute values before appending to
row_data.
- Around line 45-46: Add a guard before the pl.concat(dfs) call in
extract_standings: if the list dfs is empty, return an empty polars DataFrame
with the same schema as build_standings() would produce instead of calling
pl.concat. Locate the loop that collects dfs from build_standings() and, when
dfs == [] (or len(dfs) == 0), construct and return pl.DataFrame(...) with the
expected column names and dtypes used by build_standings()/extract_standings
(match columns like the tournament/team/standing fields the rest of the pipeline
expects) so downstream code receives an empty DataFrame with the correct schema.
---
Outside diff comments:
In `@card_data/pipelines/defs/extract/limitless/extract_standings.py`:
- Around line 20-25: The code that builds parts = list(a.stripped_strings) and
then indexes parts[0] and parts[-1] can raise IndexError for empty/malformed
anchors; in the loop that checks isinstance(a, Tag) (the anchor variable a used
before tournaments.append), guard against empty or too-short parts by checking
len(parts) (e.g., skip the anchor if not parts or len(parts) < 1/2 depending on
expected fields) or fall back to safe defaults before accessing parts[0] and
parts[-1], so tournaments.append only uses valid indices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c86616dd-0286-486d-8a37-d1e7e03285dc
📒 Files selected for processing (1)
card_data/pipelines/defs/extract/limitless/extract_standings.py
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
Bug Fixes
Tests
Chores