Conversation
|
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 51 minutes and 4 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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates UI libraries to charm.land v2, centralizes error formatting via Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
flags/abilityflagset.go (1)
21-37:⚠️ Potential issue | 🟡 MinorSuppress FlagSet's default error output.
With
flag.ContinueOnError,FlagSet.Parse()writes errors to the FlagSet's output (defaulting toos.Stderr) before returning the error. Since callers inability.go,pokemon.go, andtcg.goformat and return"error parsing flags: %v"through the normal output path, users see competing error messages on stderr: one from the FlagSet framework and one from the caller's formatting.Call
SetOutput(io.Discard)on each FlagSet after creation to suppress the default error printing and ensure errors flow through a single, controlled path:Sketch
func SetupAbilityFlagSet() *AbilityFlags { af := &AbilityFlags{} af.FlagSet = flag.NewFlagSet("abilityFlags", flag.ContinueOnError) + af.FlagSet.SetOutput(io.Discard)Apply the same fix to
SetupPokemonFlagSet()andSetupTcgFlagSet().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flags/abilityflagset.go` around lines 21 - 37, The FlagSet created in SetupAbilityFlagSet (af.FlagSet) is using flag.ContinueOnError which still prints errors to the FlagSet's output (default os.Stderr); call af.FlagSet.SetOutput(io.Discard) right after creating the FlagSet to suppress that automatic printing so Parse() errors are only handled by the callers. Apply the same change to the other factories SetupPokemonFlagSet and SetupTcgFlagSet to ensure all FlagSet instances stop writing directly to stderr.cmd/pokemon/pokemon_test.go (1)
95-112:⚠️ Potential issue | 🟡 MinorAssert the expected error outcome.
The new invalid-flag case sets
expectedError: true, but the test discardsPokemonCommand’s error, so it only verifies output text.🧪 Proposed fix
- output, _ := PokemonCommand() + output, err := PokemonCommand() cleanOutput := styling.StripANSI(output) + if tt.expectedError { + assert.Error(t, err, "Expected an error") + } else { + assert.NoError(t, err, "Expected no error") + } assert.Equal(t, tt.expectedOutput, cleanOutput, "Output should match expected")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pokemon/pokemon_test.go` around lines 95 - 112, The test currently ignores the error returned by PokemonCommand(), so add error handling: capture output, err := PokemonCommand() inside the t.Run closure and assert that err is non-nil when tt.expectedError is true (use assert.Error) and nil when false (use assert.NoError), then continue to compare the cleaned output to tt.expectedOutput; update the test case loop that calls PokemonCommand() and the assertions accordingly to reference the existing tt.expectedError and PokemonCommand symbols.cli_test.go (1)
141-147:⚠️ Potential issue | 🟠 MajorSet
os.Argsfor command-handler cases.The new
"Missing Pokémon name"case reachesPokemonCommand, which reads globalos.Args; this loop currently only passestt.argstorunCLI. MirrorTestRunCLIso the test validates the intended CLI argv instead of the Go test process args.🧪 Proposed fix
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + originalArgs := os.Args + os.Args = append([]string{"poke-cli"}, tt.args...) + defer func() { os.Args = originalArgs }() + exitCode := runCLI(tt.args) if exitCode != tt.expected { t.Errorf("expected %d, got %d for args %v", tt.expected, exitCode, tt.args) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli_test.go` around lines 141 - 147, The test loop fails to set the process argv used by PokemonCommand because runCLI(tt.args) only passes args to the function but PokemonCommand reads global os.Args; update the test to set os.Args = append([]string{os.Args[0]}, tt.args...) (or mirror the approach used in TestRunCLI) before calling runCLI(tt.args), and restore the original os.Args after the subtest to avoid leaking state; ensure this change targets the test cases including "Missing Pokémon name" so PokemonCommand sees the intended argv.cmd/utils/validateargs.go (1)
78-88:⚠️ Potential issue | 🟠 MajorGuard empty trailing arguments before indexing
arg[0].
poke-cli pokemon pikachu ""can panic here becausearg[0]is evaluated for an empty string. Treat empty args like invalid/empty flags before checking the first byte.🐛 Proposed fix
if len(args) > 3 { for _, arg := range args[3:] { // Check for an empty flag after Pokémon's name - if arg == "-" || arg == "--" { + if arg == "" || arg == "-" || arg == "--" { return fmt.Errorf("%s", FormatError(fmt.Sprintf("Empty flag '%s'.\nPlease specify valid flag(s).", arg))) } // Check if the argument after Pokémon's name is an attempted flag - if arg[0] != '-' { + if !strings.HasPrefix(arg, "-") { return fmt.Errorf("%s", FormatError(fmt.Sprintf("Invalid argument '%s'.\nOnly flags are allowed after declaring a Pokémon's name", arg))) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/utils/validateargs.go` around lines 78 - 88, The loop over args[3:] indexes arg[0] without guarding empty strings, which can panic; update the check in the loop inside validateargs.go to first treat empty string args (len(arg) == 0 or arg == "") as an empty/invalid flag and return the same formatted error as for "-" or "--", then proceed to the existing checks (keep the "-"/"--" check and only access arg[0] after confirming arg is non-empty). Refer to the loop that iterates over args[3:], the arg variable, and the FormatError(fmt.Sprintf(...)) error returns to apply this ordering change.flags/pokemonflagset.go (1)
62-97:⚠️ Potential issue | 🟡 MinorSuppress flagset's direct output to keep parse diagnostics inside the command output path.
With
flag.ContinueOnError, the customUsagefunction inpokemonflagset.go(line 84-97) still prints directly viafmt.Println, bypassing the command's error buffer inPokemonCommand. WhenParseencounters an invalid flag, it invokesUsagebefore returning the error, so parse errors emit unformatted output to stdout outside the centralized output path.Disable the flagset's own output before parsing:
Fix: Suppress flagset output in the parse path
pf := flags.SetupPokemonFlagSet() + pf.FlagSet.SetOutput(io.Discard) + pf.FlagSet.Usage = func() {} args := os.Args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flags/pokemonflagset.go` around lines 62 - 97, The custom Usage currently prints directly with fmt.Println, bypassing the command's error buffer; change the Usage implementation to write to the flagset's output (use fmt.Fprintln(pf.FlagSet.Output(), helpMessage) instead of fmt.Println) and, in the parse path where the FlagSet is parsed (e.g., where PokemonCommand calls pf.FlagSet.Parse or similar), call pf.FlagSet.SetOutput(io.Discard) before parsing so the flag package won't emit unformatted output on parse errors; this keeps output suppression centralized while still allowing Usage to be captured if you set a non-discard output later.cmd/berry/berry_test.go (2)
84-116:⚠️ Potential issue | 🟡 MinorActually send the table-driven key messages.
This test defines escape/ctrl+c cases but calls
m.Update(nil)for every case, so it does not verify the migratedtea.KeyPressMsgquit handling.🧪 Proposed fix
tests := []struct { - name string - keyMsg string - shouldQuit bool - expectError bool + name string + msg tea.KeyPressMsg + shouldQuit bool }{ { name: "escape key", - keyMsg: "esc", + msg: tea.KeyPressMsg{Code: tea.KeyEscape}, shouldQuit: true, }, { name: "ctrl+c key", - keyMsg: "ctrl+c", + msg: tea.KeyPressMsg{Code: 'c', Mod: tea.ModCtrl}, shouldQuit: true, }, { name: "other key", - keyMsg: "j", + msg: tea.KeyPressMsg{Code: 'j'}, shouldQuit: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - updated, _ := m.Update(nil) + updated, cmd := m.Update(tt.msg) + got := updated.(model) - if tt.shouldQuit { - if updated == nil { - t.Errorf("Update() returned nil model") - } + if got.quitting != tt.shouldQuit { + t.Errorf("Update() quitting = %v, want %v", got.quitting, tt.shouldQuit) + } + if tt.shouldQuit && cmd == nil { + t.Error("Update() should return a quit command") } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/berry/berry_test.go` around lines 84 - 116, The table-driven test currently calls m.Update(nil) and never sends the key events to verify quit behavior; change the subtest to construct and pass the appropriate tea.KeyMsg/tea.KeyPressMsg (based on tt.keyMsg values "esc", "ctrl+c", "j") into m.Update so Update receives the key input to process; locate the loop and replace the nil argument in the call to m.Update(nil) with a mapped message (e.g. map "esc" -> tea.KeyMsg for Esc, "ctrl+c" -> Ctrl+C key message, others -> rune/keypress) and assert the returned model/quit behavior as before.
249-257:⚠️ Potential issue | 🟡 MinorAvoid asserting the stale cached selection after immediate quit.
Updatereturns on ctrl+c before syncingselectedOption, so this test will see""even though the table’s selected row is stillAguav.🧪 Proposed fix
final := testModel.FinalModel(t).(model) // First row should be selected by default - if final.selectedOption != "Aguav" { - t.Errorf("Expected initial selectedOption to be 'Aguav', got %q", final.selectedOption) + row := final.table.SelectedRow() + if len(row) == 0 || row[0] != "Aguav" { + t.Errorf("Expected initial selected row to be 'Aguav', got %v", row) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/berry/berry_test.go` around lines 249 - 257, The test is asserting final.selectedOption right after sending ctrl+c, but Update returns before selectedOption is synced; instead, read the table's selection directly from the final model (e.g., use final.table.Cursor() or the table's SelectedRow()/SelectedIndex() accessor on the model returned by testModel.FinalModel(t)) or remove the stale selectedOption assertion — update the assertion to use the table's selection API (referencing testModel.Send, testModel.WaitFinished, testModel.FinalModel(t), and the model.table accessor) so the test inspects the actual table cursor rather than the potentially-unsynced selectedOption field.cmd/berry/berry.go (1)
3-15:⚠️ Potential issue | 🟠 MajorReturn the query error instead of terminating the process.
tableGenerationis called fromBerryCommand, which expects to append and return errors.log.Fatalfexits immediately, so callers/tests cannot handle database failures.🐛 Proposed fix
import ( "flag" "fmt" - "log" "os" "strings" @@ if err != nil { - log.Fatalf("Failed to get berry names: %v", err) + return fmt.Errorf("failed to get berry names: %w", err) }Also applies to: 125-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/berry/berry.go` around lines 3 - 15, tableGeneration currently calls log.Fatalf which exits the process; change it to return an error instead and propagate it so callers (like BerryCommand) can append/return errors. Update tableGeneration's signature to return (table.Table, error) or error (as appropriate), replace log.Fatalf and any other direct process exits (including the occurrences around the 125-127 area) with fmt.Errorf(...) returns that include contextual info, and update BerryCommand to check the returned error from tableGeneration and append/return it rather than letting the program exit. Ensure all call sites are updated to handle the new error return.cmd/ability/ability_test.go (1)
55-72:⚠️ Potential issue | 🟡 MinorAssert the returned error for the new invalid-flag case.
wantError: trueis set, but Line 69 discardsAbilityCommand()’s error, so this test can pass even if invalid flags stop returning errors.Proposed fix
- output, _ := AbilityCommand() + output, err := AbilityCommand() cleanOutput := styling.StripANSI(output) + if tt.wantError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } assert.Equal(t, tt.expectedOutput, cleanOutput, "Output should match expected")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ability/ability_test.go` around lines 55 - 72, The test sets wantError: true but discards the error from AbilityCommand(); update the test loop to capture the returned error (e.g., output, err := AbilityCommand()) and assert it against tt.wantError (use assert.Error when tt.wantError is true and assert.NoError when false, or assert.Equal on (err != nil) to tt.wantError). Modify the block that currently calls AbilityCommand() and assigns to output only so it checks tt.wantError using the err value while still stripping and comparing output when appropriate; reference AbilityCommand, tt.wantError, and the test loop/t.Run to locate the change.cmd/types/types_test.go (1)
150-153:⚠️ Potential issue | 🟡 MinorThis subtest performs no assertion.
m.selectedOption = "Fire"is set butm.View()is never called and no assertion is made, so this subtest always passes without actually verifying behavior. Either remove it or complete the assertion.Proposed fix
t.Run("View should return empty string when selectedOption is set", func(t *testing.T) { m := createTestModel() m.selectedOption = "Fire" + view := m.View() + assert.Equal(t, "", view.Content, "View should return empty string when selectedOption is set") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/types/types_test.go` around lines 150 - 153, The subtest creates a model with createTestModel() and sets m.selectedOption = "Fire" but never calls m.View() or asserts the result, so it should be completed: call m.View() on the model (m.View()) and add an assertion (e.g., require.Equal / t.Fatalf / t.Errorf) that the returned string is the expected empty string; alternatively remove the entire subtest if it's redundant. Update the subtest that currently references createTestModel, m.selectedOption and m.View to include the actual assertion or delete it.cmd/tcg/dashboard.go (1)
31-54:⚠️ Potential issue | 🟠 MajorUse Bubble Tea v2's
BackgroundColorMsginstead of synchronouslipgloss.HasDarkBackground().
lipgloss.HasDarkBackground(os.Stdin, os.Stdout)at line 34 performs a blocking terminal query (OSC 11) and reads the response from stdin. In a Bubble Tea v2 program, this can race with Bubble Tea's input loop, leak escape sequences into the rendered UI, or block indefinitely on terminals that don't respond. Bubble Tea v2 automatically publishes the detected background as atea.BackgroundColorMsgevent—the recommended approach for theme detection.Defer style initialization to the
Updatemethod by handlingtea.BackgroundColorMsg. InInit(), returntea.RequestBackgroundColorto trigger the background detection, then create styles with the resolved background color inUpdatewhen you receive the message. Store both light and dark color variants in the model if needed, or create a function likenewStyles(bgIsDark bool)that accepts the background state.Similar issues exist in
styling/styling.goandstyling/list.goat package init time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tcg/dashboard.go` around lines 31 - 54, The current newStyles() uses lipgloss.HasDarkBackground(os.Stdin, os.Stdout) which blocks; change initialization to use Bubble Tea v2's async background detection by returning tea.RequestBackgroundColor from Init(), handle tea.BackgroundColorMsg in Update(), and call a refactored constructor newStyles(bgIsDark bool) (or pass the boolean into styles creation) when the BackgroundColorMsg arrives; update the model to hold light/dark variants or constructed styles and remove any synchronous lipgloss.HasDarkBackground calls (also fix similar package-init usage in styling/styling.go and styling/list.go).
🧹 Nitpick comments (4)
cmd/move/move.go (2)
69-69: Width bump 32 → 34: double-check long content doesn't still wrap awkwardly.The two containers' widths were increased from 32 to 34. Flavor text from
scarlet-violet/sword-shieldcan be noticeably longer than the labels; please verify (especially for long move names andEffect Chance/Categoryrows) that 34 cells produces the intended layout on standard 80-column terminals and doesn't clip or introduce ragged wrapping. If the value was chosen to match the item container specifically, a shared constant instylingwould avoid drift.Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/move/move.go` at line 69, The Width(34) change increases two containers' widths (the Width(34) calls at the two locations) and may cause awkward wrapping on 80-column terminals for long move names and rows like "Effect Chance"/"Category"; run layout checks with long scarlet/violet and sword/shield strings in a standard 80-column terminal and adjust the value if it clips or wraps poorly, and instead of hardcoding 34 in both places extract a shared constant (e.g., MoveContainerWidth) into the styling package and replace both Width(34) occurrences with that constant to keep them in sync.
65-69: Consider applying the same dark-mode adaptive styling used initem.gofor consistency.
cmd/item/item.gowas updated to computeisDarkvialipgloss.HasDarkBackgroundand pick border colors withlipgloss.LightDark(isDark), butmoveInfoContainer/moveEffectContainerstill use a singlelipgloss.Color(styling.GetTypeColor(...))without any light/dark adaptation. If the goal of this PR is to adopt v2's dark-mode aware theming, consider extending the same pattern here (e.g. a light/dark variant per type, or at least an adaptive fallback) so the UX is consistent across commands.Also applies to: 102-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/move/move.go` around lines 65 - 69, Compute dark-mode once like in item.go (isDark := lipgloss.HasDarkBackground()) and use lipgloss.LightDark when choosing type border colors for docStyle, moveInfoContainer and moveEffectContainer: replace Lipgloss.Color(styling.GetTypeColor(moveStruct.Type.Name)) with lipgloss.Color(lipgloss.LightDark(isDark, styling.GetTypeLightColor(moveStruct.Type.Name), styling.GetTypeColor(moveStruct.Type.Name))) — if styling.GetTypeLightColor doesn't exist add a light-variant accessor or fall back to the same GetTypeColor value; apply the same change to the other containers (references: docStyle, moveInfoContainer, moveEffectContainer, moveStruct.Type.Name, styling.GetTypeColor).cmd/types/types.go (1)
9-11: LGTM — v2 migration and explicit table width are reasonable.One minor nit: the three
tea.NewView(...)return sites inView()could be unified by building acontentstring once and wrapping at the end (matching the pattern used incmd/card/setslist.goandcmd/tcg/tournamentslist.go). Not a blocker.Also applies to: 67-67, 85-99, 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/types/types.go` around lines 9 - 11, Unify the three tea.NewView(...) return sites inside the View() function by constructing a single content string variable (e.g., content := strings.Builder or a concatenated string) that accumulates the header, table.View(), and footer, then call tea.NewView(content, ... ) once at the end; update the View() implementation to build content conditionally (matching the pattern used in setsListView/tournamentsListView) and perform the lipgloss.Wrap or width handling only when creating the final tea.NewView to avoid duplicated return sites.styling/styling.go (1)
23-38: Package‑level mutable exportedvars initialized ininit()create a hidden ordering coupling.
Yellow,ColoredBullet,DocsLink,YellowAdaptive, andYellowAdaptive2are declared as zero values at package scope and only get their real values frominit(). Any other package‑level variable initializer in this binary that references them (e.g.,var X = styling.Yellow.Render(...)) will observe the zero value, since Go runs package‑level variable initializers before that package'sinit()only within the same package — across packages, ordering depends on the import graph and can be subtle.If practical, prefer accessor functions (
func Yellow() lipgloss.Style) backed bysync.Once, or keep the values as constants/immediate expressions and resolve adaptiveness at render time. This also pairs well with the TTY‑guard suggestion above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styling/styling.go` around lines 23 - 38, Package-level exported vars Yellow, ColoredBullet, DocsLink, YellowAdaptive, and YellowAdaptive2 are left as zero-values and set in init(), causing hidden initialization-order coupling; replace them with safe accessors that initialize on first use (e.g., func Yellow() lipgloss.Style, func ColoredBullet() lipgloss.Style, func DocsLink() string, func YellowAdaptive() color.Color, func YellowAdaptive2() color.Color) and implement each accessor with sync.Once to populate the underlying value (or compute adaptiveness at render time) so no other package-level initializer ever observes a zero value; update call sites to use these accessors and remove the mutable exported vars from package scope.
🤖 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/ability/ability.go`:
- Around line 51-57: The current branch returns an empty output when
af.FlagSet.Parse(args[3:]) yields flag.ErrHelp because the configured
FlagSet.Usage prints directly to stdout; change the flow so help text is
captured into output.String() instead of lost: either set the FlagSet output to
the local buffer (e.g., call af.FlagSet.SetOutput(&output)) before calling
af.FlagSet.Parse, or replace FlagSet.Usage with a version that writes into the
same output buffer, then when errors.Is(err, flag.ErrHelp) return
output.String(), nil so callers receive the help text produced by
FlagSet.Usage/SetOutput and tests can assert against output.String().
In `@cmd/item/item.go`:
- Line 9: Update styling/huh_theme.go to use the v2 lipgloss package and the v2
adaptive color API: replace the import "github.com/charmbracelet/lipgloss" with
"charm.land/lipgloss/v2", then find all uses of lipgloss.AdaptiveColor (e.g.,
any literals like lipgloss.AdaptiveColor{Light: ..., Dark: ...}) and convert
them to the v2 pattern (use the v2 adaptive color constructor/values as defined
in charm.land/lipgloss/v2), updating any variables or functions that reference
lipgloss.AdaptiveColor so they use the v2 types and names consistently.
- Around line 64-70: The call to lipgloss.HasDarkBackground is made
unconditionally which can block in non-TTY contexts; update the isDark
assignment to first check TTY using term.IsTerminal(int(os.Stdout.Fd())) (from
golang.org/x/term) and only call lipgloss.HasDarkBackground(os.Stdin, os.Stdout)
when stdout is a terminal, otherwise leave isDark false; adjust the code around
the isDark variable (used for ld := lipgloss.LightDark(...) and docStyle) to
follow this guarded pattern so non-TTY runs (redirected output/CI) do not block.
In `@cmd/tcg/tcg.go`:
- Around line 43-49: The help text is being printed directly to stdout by the
Usage implementation so when tf.FlagSet.Parse returns flag.ErrHelp the function
returns an empty output; fix by routing flag output into the existing output
buffer: before calling tf.FlagSet.Parse(os.Args[2:]) call
tf.FlagSet.SetOutput(&output) (or change the Usage implementation in
flags/tcg_flagset.go to write to the FlagSet's output writer rather than using
fmt.Println), so that when Parse returns flag.ErrHelp the help text is captured
in output.String() and returned to callers.
In `@styling/huh_theme.go`:
- Line 23: The AdaptiveColor for the focused description currently leaves Light
empty in t.Focused.Description.Foreground(lipgloss.AdaptiveColor{Light: "",
Dark: "243"}); update both occurrences (the one at t.Focused.Description and the
other similar instance around line 33) to either supply an explicit light-mode
color string (e.g., "243" or "245") or add a short inline comment explaining
that leaving Light empty is an intentional fallback to the terminal default;
adjust the AdaptiveColor literals accordingly so the intent is explicit.
In `@styling/styling.go`:
- Around line 83-95: The init() currently calls
lipgloss.HasDarkBackground(os.Stdin, os.Stdout) at package import which performs
a blocking OSC 11 terminal query; replace this with a non-blocking TTY guard:
check term.IsTerminal(int(os.Stdin.Fd())) &&
term.IsTerminal(int(os.Stdout.Fd())) and only call lipgloss.HasDarkBackground
when both are true, otherwise pick a safe default (e.g., isDark=false). Move any
rendered outputs out of init: stop pre-rendering DocsLink and instead render it
at runtime (e.g., provide a DocsLinkString() or RenderDocsLink() function) so
ANSI sequences aren’t written at import time. Update initialization of
YellowAdaptive, YellowAdaptive2, Yellow, and ColoredBullet to use the guarded
isDark value (or static colors) so no terminal IO happens during import.
---
Outside diff comments:
In `@cli_test.go`:
- Around line 141-147: The test loop fails to set the process argv used by
PokemonCommand because runCLI(tt.args) only passes args to the function but
PokemonCommand reads global os.Args; update the test to set os.Args =
append([]string{os.Args[0]}, tt.args...) (or mirror the approach used in
TestRunCLI) before calling runCLI(tt.args), and restore the original os.Args
after the subtest to avoid leaking state; ensure this change targets the test
cases including "Missing Pokémon name" so PokemonCommand sees the intended argv.
In `@cmd/ability/ability_test.go`:
- Around line 55-72: The test sets wantError: true but discards the error from
AbilityCommand(); update the test loop to capture the returned error (e.g.,
output, err := AbilityCommand()) and assert it against tt.wantError (use
assert.Error when tt.wantError is true and assert.NoError when false, or
assert.Equal on (err != nil) to tt.wantError). Modify the block that currently
calls AbilityCommand() and assigns to output only so it checks tt.wantError
using the err value while still stripping and comparing output when appropriate;
reference AbilityCommand, tt.wantError, and the test loop/t.Run to locate the
change.
In `@cmd/berry/berry_test.go`:
- Around line 84-116: The table-driven test currently calls m.Update(nil) and
never sends the key events to verify quit behavior; change the subtest to
construct and pass the appropriate tea.KeyMsg/tea.KeyPressMsg (based on
tt.keyMsg values "esc", "ctrl+c", "j") into m.Update so Update receives the key
input to process; locate the loop and replace the nil argument in the call to
m.Update(nil) with a mapped message (e.g. map "esc" -> tea.KeyMsg for Esc,
"ctrl+c" -> Ctrl+C key message, others -> rune/keypress) and assert the returned
model/quit behavior as before.
- Around line 249-257: The test is asserting final.selectedOption right after
sending ctrl+c, but Update returns before selectedOption is synced; instead,
read the table's selection directly from the final model (e.g., use
final.table.Cursor() or the table's SelectedRow()/SelectedIndex() accessor on
the model returned by testModel.FinalModel(t)) or remove the stale
selectedOption assertion — update the assertion to use the table's selection API
(referencing testModel.Send, testModel.WaitFinished, testModel.FinalModel(t),
and the model.table accessor) so the test inspects the actual table cursor
rather than the potentially-unsynced selectedOption field.
In `@cmd/berry/berry.go`:
- Around line 3-15: tableGeneration currently calls log.Fatalf which exits the
process; change it to return an error instead and propagate it so callers (like
BerryCommand) can append/return errors. Update tableGeneration's signature to
return (table.Table, error) or error (as appropriate), replace log.Fatalf and
any other direct process exits (including the occurrences around the 125-127
area) with fmt.Errorf(...) returns that include contextual info, and update
BerryCommand to check the returned error from tableGeneration and append/return
it rather than letting the program exit. Ensure all call sites are updated to
handle the new error return.
In `@cmd/pokemon/pokemon_test.go`:
- Around line 95-112: The test currently ignores the error returned by
PokemonCommand(), so add error handling: capture output, err := PokemonCommand()
inside the t.Run closure and assert that err is non-nil when tt.expectedError is
true (use assert.Error) and nil when false (use assert.NoError), then continue
to compare the cleaned output to tt.expectedOutput; update the test case loop
that calls PokemonCommand() and the assertions accordingly to reference the
existing tt.expectedError and PokemonCommand symbols.
In `@cmd/tcg/dashboard.go`:
- Around line 31-54: The current newStyles() uses
lipgloss.HasDarkBackground(os.Stdin, os.Stdout) which blocks; change
initialization to use Bubble Tea v2's async background detection by returning
tea.RequestBackgroundColor from Init(), handle tea.BackgroundColorMsg in
Update(), and call a refactored constructor newStyles(bgIsDark bool) (or pass
the boolean into styles creation) when the BackgroundColorMsg arrives; update
the model to hold light/dark variants or constructed styles and remove any
synchronous lipgloss.HasDarkBackground calls (also fix similar package-init
usage in styling/styling.go and styling/list.go).
In `@cmd/types/types_test.go`:
- Around line 150-153: The subtest creates a model with createTestModel() and
sets m.selectedOption = "Fire" but never calls m.View() or asserts the result,
so it should be completed: call m.View() on the model (m.View()) and add an
assertion (e.g., require.Equal / t.Fatalf / t.Errorf) that the returned string
is the expected empty string; alternatively remove the entire subtest if it's
redundant. Update the subtest that currently references createTestModel,
m.selectedOption and m.View to include the actual assertion or delete it.
In `@cmd/utils/validateargs.go`:
- Around line 78-88: The loop over args[3:] indexes arg[0] without guarding
empty strings, which can panic; update the check in the loop inside
validateargs.go to first treat empty string args (len(arg) == 0 or arg == "") as
an empty/invalid flag and return the same formatted error as for "-" or "--",
then proceed to the existing checks (keep the "-"/"--" check and only access
arg[0] after confirming arg is non-empty). Refer to the loop that iterates over
args[3:], the arg variable, and the FormatError(fmt.Sprintf(...)) error returns
to apply this ordering change.
In `@flags/abilityflagset.go`:
- Around line 21-37: The FlagSet created in SetupAbilityFlagSet (af.FlagSet) is
using flag.ContinueOnError which still prints errors to the FlagSet's output
(default os.Stderr); call af.FlagSet.SetOutput(io.Discard) right after creating
the FlagSet to suppress that automatic printing so Parse() errors are only
handled by the callers. Apply the same change to the other factories
SetupPokemonFlagSet and SetupTcgFlagSet to ensure all FlagSet instances stop
writing directly to stderr.
In `@flags/pokemonflagset.go`:
- Around line 62-97: The custom Usage currently prints directly with
fmt.Println, bypassing the command's error buffer; change the Usage
implementation to write to the flagset's output (use
fmt.Fprintln(pf.FlagSet.Output(), helpMessage) instead of fmt.Println) and, in
the parse path where the FlagSet is parsed (e.g., where PokemonCommand calls
pf.FlagSet.Parse or similar), call pf.FlagSet.SetOutput(io.Discard) before
parsing so the flag package won't emit unformatted output on parse errors; this
keeps output suppression centralized while still allowing Usage to be captured
if you set a non-discard output later.
---
Nitpick comments:
In `@cmd/move/move.go`:
- Line 69: The Width(34) change increases two containers' widths (the Width(34)
calls at the two locations) and may cause awkward wrapping on 80-column
terminals for long move names and rows like "Effect Chance"/"Category"; run
layout checks with long scarlet/violet and sword/shield strings in a standard
80-column terminal and adjust the value if it clips or wraps poorly, and instead
of hardcoding 34 in both places extract a shared constant (e.g.,
MoveContainerWidth) into the styling package and replace both Width(34)
occurrences with that constant to keep them in sync.
- Around line 65-69: Compute dark-mode once like in item.go (isDark :=
lipgloss.HasDarkBackground()) and use lipgloss.LightDark when choosing type
border colors for docStyle, moveInfoContainer and moveEffectContainer: replace
Lipgloss.Color(styling.GetTypeColor(moveStruct.Type.Name)) with
lipgloss.Color(lipgloss.LightDark(isDark,
styling.GetTypeLightColor(moveStruct.Type.Name),
styling.GetTypeColor(moveStruct.Type.Name))) — if styling.GetTypeLightColor
doesn't exist add a light-variant accessor or fall back to the same GetTypeColor
value; apply the same change to the other containers (references: docStyle,
moveInfoContainer, moveEffectContainer, moveStruct.Type.Name,
styling.GetTypeColor).
In `@cmd/types/types.go`:
- Around line 9-11: Unify the three tea.NewView(...) return sites inside the
View() function by constructing a single content string variable (e.g., content
:= strings.Builder or a concatenated string) that accumulates the header,
table.View(), and footer, then call tea.NewView(content, ... ) once at the end;
update the View() implementation to build content conditionally (matching the
pattern used in setsListView/tournamentsListView) and perform the lipgloss.Wrap
or width handling only when creating the final tea.NewView to avoid duplicated
return sites.
In `@styling/styling.go`:
- Around line 23-38: Package-level exported vars Yellow, ColoredBullet,
DocsLink, YellowAdaptive, and YellowAdaptive2 are left as zero-values and set in
init(), causing hidden initialization-order coupling; replace them with safe
accessors that initialize on first use (e.g., func Yellow() lipgloss.Style, func
ColoredBullet() lipgloss.Style, func DocsLink() string, func YellowAdaptive()
color.Color, func YellowAdaptive2() color.Color) and implement each accessor
with sync.Once to populate the underlying value (or compute adaptiveness at
render time) so no other package-level initializer ever observes a zero value;
update call sites to use these accessors and remove the mutable exported vars
from package scope.
🪄 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: ea1cade7-ee2e-428b-a221-dd8022f4ae69
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (66)
.github/workflows/ci.yml.gitignore.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/poke_cli_dbt/dbt_project.ymlcli.gocli_test.gocmd/ability/ability.gocmd/ability/ability_test.gocmd/berry/berry.gocmd/berry/berry_test.gocmd/berry/berryinfo.gocmd/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/item/item.gocmd/move/move.gocmd/natures/natures.gocmd/pokemon/pokemon.gocmd/pokemon/pokemon_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.gocmd/speed/speed.gocmd/tcg/dashboard.gocmd/tcg/dashboard_test.gocmd/tcg/data.gocmd/tcg/tab_overview.gocmd/tcg/tab_overview_test.gocmd/tcg/tab_standings.gocmd/tcg/tcg.gocmd/tcg/tcg_test.gocmd/tcg/tournamentslist.gocmd/tcg/tournamentslist_test.gocmd/types/damage_table.gocmd/types/damage_table_test.gocmd/types/types.gocmd/types/types_test.gocmd/utils/errors.gocmd/utils/validateargs.goconnections/connection.goflags/abilityflagset.goflags/pokemonflagset.goflags/tcg_flagset.goflags/version.gogo.modnfpm.yamlstyling/huh_theme.gostyling/list.gostyling/list_test.gostyling/styling.gotestdata/ability_invalid_flag.goldentestdata/main_latest_flag.goldentestdata/pokemon_invalid_flag.goldentestdata/tcg_invalid_flag.goldenweb/pyproject.toml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
styling/styling.go (1)
84-99: Good fix on the TTY guard — addresses the prior blocking-I/O concern.The
term.IsTerminalcheck on bothos.Stdin/os.Stdoutbefore callinglipgloss.HasDarkBackgroundcorrectly avoids the OSC 11 query in non-TTY contexts, and theisDark := truedefault matches lipgloss v2's fallback behavior. One follow-up worth considering:
DocsLinkis pre-rendered at init as a plain string (lines 96–98). It contains ANSI hyperlink and color escapes that bypass lipgloss's color downsampling at print time. The only consumer (cli.go:103) prints it viafmt.Sprintf+fmt.Println, not a lipgloss-aware printer. In this case, the OSC-8 hyperlink sequences degrade gracefully and color escapes are safe, but for consistency with the rest of the codebase's lipgloss usage, consider either keepingDocsLinkas alipgloss.Styleand rendering it at the call site, or rendering it through a lipgloss printer if always using pre-rendered form.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styling/styling.go` around lines 84 - 99, DocsLink is being pre-rendered in init (using Render(...)) which embeds ANSI escapes at package init; change DocsLink to be a lipgloss.Style (e.g., assign DocsLink = lipgloss.NewStyle().Foreground(YellowAdaptive2) in the init) instead of a pre-rendered string, remove the Render(...) call, and update the caller at cli.go:103 to call DocsLink.Render(...) (or use a lipgloss-aware printer) when printing the hyperlink so color/hyperlink escape handling happens at render time rather than at package init.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@styling/styling.go`:
- Around line 84-99: DocsLink is being pre-rendered in init (using Render(...))
which embeds ANSI escapes at package init; change DocsLink to be a
lipgloss.Style (e.g., assign DocsLink =
lipgloss.NewStyle().Foreground(YellowAdaptive2) in the init) instead of a
pre-rendered string, remove the Render(...) call, and update the caller at
cli.go:103 to call DocsLink.Render(...) (or use a lipgloss-aware printer) when
printing the hyperlink so color/hyperlink escape handling happens at render time
rather than at package init.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 112540fd-7998-4d94-82d9-571fe4c0ee4a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modstyling/styling.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests