Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the CLI transfer commands to reduce duplicated setup logic and consolidate repeated interactive confirmation / crypto key-handling flows.
Changes:
- Centralizes config+token+client initialization via
newAPIClient. - Extracts concurrent fetch logic (
fetchDetailsAndKey) and identity resolution (resolveUserIdentity) into helpers. - Deduplicates repeated “file list + proceed?” prompts via
confirmFileListand adjusts linting to avoid per-call//nolint:errcheckonfmt.*output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
cmd/transfer.go |
Refactors transfer subcommands by introducing shared helpers for API client creation, concurrent fetches, identity resolution, and confirmation prompts. |
cmd/root.go |
Removes an extra blank line in imports/var block. |
.golangci.yml |
Updates golangci-lint configuration to exclude fmt.Fprint/Fprintf/Fprintln from errcheck. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| ctx := context.Background() | ||
| tok, err := mustGetToken(ctx, cfg) | ||
| cfg, client, err := newAPIClient(ctx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This command uses context.Background() instead of cmd.Context(), so API calls won't be canceled when the CLI context is canceled (Ctrl+C, timeouts). Consider using ctx := cmd.Context() and threading it through newAPIClient and downstream calls (consistent with cmd/auth.go).
| return fmt.Errorf("loading config: %w", err) | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
This command uses context.Background() rather than cmd.Context(), which prevents request cancellation/cleanup on SIGINT and diverges from how other commands handle context. Consider using ctx := cmd.Context() before calling newAPIClient.
| ctx := context.Background() | |
| ctx := cmd.Context() |
| ctx := context.Background() | ||
| tok, err := mustGetToken(ctx, cfg) | ||
| _, client, err := newAPIClient(ctx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This command uses context.Background() rather than cmd.Context(), so API calls won't be canceled if the user interrupts the command. Consider switching to cmd.Context() and passing it into newAPIClient/API operations.
| ctx := context.Background() | ||
| tok, err := mustGetToken(ctx, cfg) | ||
| _, client, err := newAPIClient(ctx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This command uses context.Background() rather than cmd.Context(), which means it won't respect cancellation/deadlines from Cobra (e.g., Ctrl+C). Consider using ctx := cmd.Context() here before calling newAPIClient.
| return fmt.Errorf("loading config: %w", err) | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
This command uses context.Background() rather than cmd.Context(), so long-running download operations may not cancel promptly on SIGINT and won't inherit any deadlines. Consider switching to ctx := cmd.Context() before calling newAPIClient and making API calls.
| ctx := context.Background() | |
| ctx := cmd.Context() |
| errcheck: | ||
| exclude-functions: | ||
| - fmt.Fprint | ||
| - fmt.Fprintf | ||
| - fmt.Fprintln |
There was a problem hiding this comment.
linters.settings.errcheck.exclude-functions excludes all fmt.Fprint/Fprintf/Fprintln calls from errcheck across the entire codebase, which can hide real write errors (e.g., writing to files, network connections, or custom io.Writers). Prefer keeping errcheck enabled for fmt.* and either handle the returned error or add targeted //nolint:errcheck on the specific CLI-output calls where ignoring the error is intentional; alternatively, scope the exclusion to specific files/packages if golangci-lint v2 supports that.
| errcheck: | |
| exclude-functions: | |
| - fmt.Fprint | |
| - fmt.Fprintf | |
| - fmt.Fprintln | |
| errcheck: {} |
| if len(name) > lineWidth-10 { | ||
| name = name[:lineWidth-13] + "…" |
There was a problem hiding this comment.
confirmFileList truncates long filenames by slicing the string (name[:...]). Because Go strings are byte slices, this can split a multi-byte UTF-8 rune and produce invalid/mangled output for non-ASCII filenames. Consider truncating by rune count (e.g., converting to []rune) or using a UTF-8 aware truncation helper.
| if len(name) > lineWidth-10 { | |
| name = name[:lineWidth-13] + "…" | |
| runes := []rune(name) | |
| if len(runes) > lineWidth-10 { | |
| name = string(runes[:lineWidth-13]) + "…" |
| return fmt.Errorf("loading config: %w", err) | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
These commands use context.Background() instead of cmd.Context(), so they won't inherit Cobra's cancellation/deadline (e.g., on SIGINT) the way other commands do (see cmd/auth.go). Consider switching to ctx := cmd.Context() here and passing that through to newAPIClient/API calls.
| ctx := context.Background() | |
| ctx := cmd.Context() |
No description provided.