feat: context threading, video enhancements, and comprehensive docs

[jmagar]

Summary

Major improvements across four key areas: API architecture, video handling, documentation, and build optimization.

Changes

🔧 API Architecture - Context Threading

• Thread context.Context through all API functions for cancellation/timeout support
• Update 19 files across internal packages and cmd wrappers
• All HTTP calls now use NewRequestWithContext for proper context handling
• Enables graceful cancellation and deadline enforcement throughout the application

Files: internal/api/client.go, internal/{catalog,download,list}/*, cmd/nugs/*

📹 Video Format Enhancements

• Add availType parameter support to GetArtistMeta API calls
• Expose GetArtistMetaWithAvailType for flexible catalog queries
• Improve media type detection from ProductFormatList
• Add test coverage for video-only and mixed format show detection

Files: internal/api/client.go, internal/download/video.go, cmd/nugs/media_type_test.go

📚 Documentation

• COMMANDS.md - Complete CLI reference (458 lines)
  • All commands, subcommands, and aliases documented
  • URL patterns, flags, and media modifiers
  • Usage examples for every feature
  • Hotkey reference and alias summary

⚡ Build Optimization

• Replace json.MarshalIndent with json.Marshal for cache files
• Reduces memory usage ~2x and speeds up serialization ~3x
• Affects catalog.json (7-8MB), artist index, container index

File: internal/cache/cache.go

🧹 Cleanup

• Remove CI workflow (blocked by OAuth scope limitation)
• Add .full-review/ to .gitignore

Testing

• ✅ All existing tests pass
• ✅ New test coverage for media type detection
• ✅ Cross-platform build verified (linux, darwin, windows)
• ✅ go vet clean
• ✅ Race detector clean

Breaking Changes

None - all changes are backward compatible.

Stats

• 19 files changed in context threading
• 6 files changed in latest commit
• 458 insertions in documentation
• 184 deletions from refactoring

---

Summary by cubic

Adds full context propagation, media‑aware listing/downloads, and comprehensive docs. Introduces a storage adapter with stateful upload progress, signal‑based cancellation, and faster, safer progress rendering across the CLI.

• New Features

  • Signal-driven cancellation via signal.NotifyContext across cmd/nugs and downloads.
  • Progress box uses a snapshot render pattern for smoother UI; clearer completion summary.
  • CLI parsing uses named URL constants; config search now includes ./config.json.

• Bug Fixes

  • Fixed progress accounting (no double‑counting), nil progress box guards, and ETA based on authoritative upload totals.
  • Eliminated lock‑during‑I/O in rendering and corrected PreCalculateShowSize timeout behavior.
  • Guarded rclone status checks behind config; surfaced cache write errors; naming and minor reliability cleanups.

^{Written for commit 07037ec. Summary will update on new commits.}

Summary by CodeRabbit

• New Features

  • Media type filtering (audio, video, both) for downloads and catalog operations
  • Video download support with media type indicators
  • Configuration options for default media type and video output paths
  • Media type modifiers in command-line arguments

• Documentation

  • Architecture, configuration, and commands reference guides added

• Build & Testing

  • Added test target to build system

[cubic-dev-ai]

8 issues found across 41 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="CLAUDE.md">

<violation number="1" location="CLAUDE.md:143">
P3: The note about cross-compilation is incorrect: Go still emits a binary in the current directory (or `-o` location) even when the target OS differs. This could mislead contributors about build artifacts.</violation>
</file>

<file name="internal/rclone/storage_adapter.go">

<violation number="1" location="internal/rclone/storage_adapter.go:65">
P2: Upload ignores the provided context, so uploads cannot be cancelled or bounded by caller deadlines. Threading ctx through the rclone command (e.g., using exec.CommandContext) would preserve cancellation semantics.</violation>

<violation number="2" location="internal/rclone/storage_adapter.go:203">
P2: ListArtistFolders ignores the provided context, so callers cannot cancel or time out the rclone listing. Use commandContext (or similar) to honor ctx.</violation>
</file>

<file name="internal/catalog/media_filter_test.go">

<violation number="1" location="internal/catalog/media_filter_test.go:82">
P2: The test writes a POSIX shell script (`#!/bin/sh`) as the mock rclone executable, which won’t run on Windows. This makes the test non‑portable and will fail on Windows CI. Consider skipping on Windows or using a platform‑specific executable stub.</violation>
</file>

<file name="internal/rclone/rclone.go">

<violation number="1">
P2: ListRemoteArtistFolders hard-codes context.Background, so callers can’t cancel or set deadlines for list operations. Thread a context.Context into this function and forward it to the storage provider.</violation>

<violation number="2">
P2: RemotePathExists uses context.Background internally, preventing callers from canceling or timing out remote checks. Accept a context.Context parameter and pass it through instead.</violation>

<violation number="3">
P2: UploadToRclone hard-codes context.Background, which prevents cancellation/timeouts from callers. Thread a context.Context into this function and pass it through to the storage provider so uploads can be canceled consistently with the rest of the context-threading changes.</violation>
</file>

<file name="internal/download/video.go">

<violation number="1" location="internal/download/video.go:265">
P2: When resuming a download, if the server ignores the Range header and returns 200 OK, this seek appends the full file after existing bytes, corrupting the video. Handle `StatusOK` by truncating/rewinding and resetting `startByte` before writing.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[coderabbitai]

Actionable comments posted: 108

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (7)

internal/model/api_types.go (3)

88-91: ⚠️ Potential issue | 🟠 Major

Missing json tag on Promo field.

Every other top-level field in SubInfo has a json tag, but Promo at Line 90 is missing one. Without it, Go's JSON encoder will use the Go field name "Promo" (capitalized), which likely won't match the API response key "promo".

🐛 Proposed fix

-	} `json:"plan"`
-	Promo struct {
+	} `json:"plan"`
+	Promo struct {
 		ID            string  `json:"id"`
 		...
-	}
+	} `json:"promo"`
 }

---

447-483: 🧹 Nitpick | 🔵 Trivial

Artist and ArtistOutput are identical — consider deduplication.

Both types have the same four fields with the same JSON tags (artistID, artistName, numShows, numAlbums). ArtistListOutput could reference Artist directly instead of introducing a duplicate type.

♻️ Suggested simplification

 // ArtistListOutput represents JSON output for list artists command.
 type ArtistListOutput struct {
-	Artists []ArtistOutput `json:"artists"`
+	Artists []Artist `json:"artists"`
 	Total   int            `json:"total"`
 }
-
-// ArtistOutput represents a single artist in JSON output.
-type ArtistOutput struct {
-	ArtistID   int    `json:"artistID"`
-	ArtistName string `json:"artistName"`
-	NumShows   int    `json:"numShows"`
-	NumAlbums  int    `json:"numAlbums"`
-}

---

60-89: 🧹 Nitpick | 🔵 Trivial

Duplicated Plan struct definition inside SubInfo.

The Plan struct (Lines 60-70) and SubInfo.Promo.Plan (Lines 78-88) share identical fields and JSON tags. If this nested struct is used elsewhere or grows, consider extracting it into a named type.

internal/completion/completions.go (1)

228-249: ⚠️ Potential issue | 🟠 Major

__fish_seen_argument is a standard fish shell function, but its usage here is semantically incorrect.

__fish_seen_argument is a built-in fish completion helper function (confirmed in official fish documentation), so the conditions will parse and execute. However, the usage pattern __fish_seen_argument -s artists, __fish_seen_argument -s config, and __fish_seen_argument -s gaps is incorrect.

The -s flag in __fish_seen_argument specifies short option names (like -a, -h) for checking if they've been seen. Using -s artists, -s config, and -s gaps doesn't match this pattern—these are positional arguments or subcommands, not short flags. This means the conditions will evaluate to false (the short options have not been "seen"), causing the nested completions to never trigger.

The correct approach is to check for subcommand presence. Replace __fish_seen_argument -s <name> with __fish_seen_subcommand_from <name> on Lines 228, 243-245, and 248-249.

Suggested fix (Lines 228–229, 243–249)

-complete -c nugs -n "__fish_seen_subcommand_from list; and __fish_seen_argument -s artists" -a "shows" -d "Filter by show count"
+complete -c nugs -n "__fish_seen_subcommand_from list artists" -a "shows" -d "Filter by show count"

-complete -c nugs -n "__fish_seen_subcommand_from catalog; and __fish_seen_argument -s config" -a "enable" -d "Enable auto-refresh"
-complete -c nugs -n "__fish_seen_subcommand_from catalog; and __fish_seen_argument -s config" -a "disable" -d "Disable auto-refresh"
-complete -c nugs -n "__fish_seen_subcommand_from catalog; and __fish_seen_argument -s config" -a "set" -d "Configure auto-refresh settings"
+complete -c nugs -n "__fish_seen_subcommand_from catalog config" -a "enable" -d "Enable auto-refresh"
+complete -c nugs -n "__fish_seen_subcommand_from catalog config" -a "disable" -d "Disable auto-refresh"
+complete -c nugs -n "__fish_seen_subcommand_from catalog config" -a "set" -d "Configure auto-refresh settings"

-complete -c nugs -n "__fish_seen_subcommand_from catalog; and __fish_seen_argument -s gaps" -a "fill" -d "Download missing shows"
-complete -c nugs -n "__fish_seen_subcommand_from catalog; and __fish_seen_argument -s gaps" -l ids-only -d "Show IDs only"
+complete -c nugs -n "__fish_seen_subcommand_from catalog gaps" -a "fill" -d "Download missing shows"
+complete -c nugs -n "__fish_seen_subcommand_from catalog gaps" -l ids-only -d "Show IDs only"

cmd/nugs/crawl_control.go (1)

112-166: 🧹 Nitpick | 🔵 Trivial

waitIfPausedOrCancelled is slightly over 50 lines.

At ~54 lines, this marginally exceeds the 50-line guideline. The three progress-box update blocks (cancel, resume, pause) follow an identical lock-check-unlock-act pattern that could be extracted into a helper, but the current inline form is readable and the logic flow is clear.

As per coding guidelines, "Keep Go functions under 50 lines where possible".

internal/catalog/handlers.go (2)

393-539: 🧹 Nitpick | 🔵 Trivial

CatalogGapsFill is ~146 lines — consider extracting the download loop and summary output.

The signal handling, batch download loop, and result summary are distinct concerns that would benefit from extraction. As per coding guidelines, "Keep Go functions under 50 lines where possible".

---

542-749: 🧹 Nitpick | 🔵 Trivial

CatalogCoverage is ~207 lines with artist discovery logic that could be extracted.

The artist folder discovery (lines 553-651), analysis loop (654-670), and output rendering (672-748) are three distinct phases. Extracting at least the discovery phase into a helper would significantly improve readability.

[cubic-dev-ai]

1 issue found across 14 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="internal/rclone/rclone.go">

<violation number="1">
P2: This helper still creates `context.Background()` instead of accepting a caller-provided context, so cancellation/deadlines can’t propagate. Thread `context.Context` through this wrapper and pass it to `ListArtistFolders` instead.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[coderabbitai]

Actionable comments posted: 78

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)

internal/model/api_types.go (2)

71-91: ⚠️ Potential issue | 🟡 Minor

Missing json tag on Promo field.

Every other top-level field in SubInfo has a json:"..." tag, but Promo (Line 91) does not. While encoding/json performs case-insensitive matching on unmarshal, marshal output will emit "Promo" (capital P) instead of "promo", breaking consistency with the API's naming convention.

Proposed fix

-	} `json:"plan"`
-	Gateway string `json:"gateway"`
-	}
+	} `json:"plan"`
+	Gateway string `json:"gateway"`
+	} `json:"promo"`

---

447-483: 🧹 Nitpick | 🔵 Trivial

ArtistOutput duplicates Artist — consider embedding or aliasing.

ArtistOutput (Lines 478–483) is field-for-field identical to Artist (Lines 448–453). If the output shape is intentionally decoupled from the API response type for future divergence, a brief comment would clarify intent. Otherwise, embedding Artist or using a type alias would reduce duplication.

Option: embed Artist

-type ArtistOutput struct {
-	ArtistID   int    `json:"artistID"`
-	ArtistName string `json:"artistName"`
-	NumShows   int    `json:"numShows"`
-	NumAlbums  int    `json:"numAlbums"`
-}
+// ArtistOutput represents a single artist in JSON output.
+type ArtistOutput = Artist

README.md (1)

8-8: ⚠️ Potential issue | 🟡 Minor

Update the Go version badge to match go.mod.

The README badge shows Go-1.16+, but go.mod specifies go 1.24.12. Update the badge on line 8 to reflect the actual minimum Go version: Go-1.24+-blue (or adjust to the minimum version you intend to support).

internal/catalog/autorefresh.go (1)

60-73: ⚠️ Potential issue | 🟡 Minor

Unrecognized CatalogRefreshInterval values silently skip refresh.

The switch handles "daily" and "weekly" but has no default case. If the config contains a typo like "daly" or an unsupported value, ShouldAutoRefresh returns false, nil — silently disabling auto-refresh with no error or warning. Consider adding a default case that returns an error.

🐛 Proposed fix

 	case "weekly":
 		weekAgo := now.Add(-7 * 24 * time.Hour)
 		if now.After(todayRefreshTime) && meta.LastUpdated.Before(weekAgo) {
 			return true, nil
 		}
+	default:
+		return false, fmt.Errorf("unsupported refresh interval: %s (expected 'daily' or 'weekly')", cfg.CatalogRefreshInterval)
 	}

internal/catalog/handlers.go (1)

541-749: 🛠️ Refactor suggestion | 🟠 Major

CatalogCoverage is ~200 lines with deeply nested logic.

This function handles discovery of local/remote artist folders, catalog mapping, analysis, and both JSON and text output. Consider decomposing into helpers (e.g., discoverArtistIDs, renderCoverageJSON, renderCoverageTable) to improve readability and stay closer to the 50-line guideline.

As per coding guidelines: "Keep Go functions under 50 lines where possible."

🤖 Fix all issues with AI agents

In `@ARCHITECTURE.md`:
- Around line 465-471: In GetLatestCatalog ensure you don't discard the error
from http.NewRequestWithContext: replace the ignored assignment with proper
error handling by capturing the returned error (e.g., req, err :=
http.NewRequestWithContext(...)), check err and return it (or wrap it) before
calling Client.Do; update the request creation in GetLatestCatalog to handle and
propagate the error instead of using req, _.
- Around line 689-693: The example in GetLatestCatalog improperly discards the
error from http.NewRequestWithContext; update the function to capture the
returned error (don't use `_`), check if err != nil and return an appropriate
error (or wrap it) instead of proceeding, and ensure subsequent code uses the
successfully created req variable; reference the GetLatestCatalog function and
the http.NewRequestWithContext call when making this change.

In `@CLAUDE.md`:
- Around line 297-311: The "Current Test Coverage" section contains hardcoded
numbers ("5.5% (37 tests across 10 test files)" and per-package line counts for
internal/api, internal/download, internal/cache, internal/catalog,
internal/config) that will go stale; remove these specific percentages and line
counts and either replace them with a generic instruction to consult the latest
coverage report (e.g., run the project's coverage command or view CI coverage
artifacts) or include a dynamic badge/placeholder, and update the text under the
"Current Test Coverage" heading to reference the coverage-report command or CI
link instead of hardcoded values.

In `@cmd/nugs/catalog_handlers.go`:
- Around line 13-24: buildCatalogDeps currently allocates a new catalog.Deps
struct on every call even though the function pointers (remotePathExists,
listRemoteArtistFolders, album, playlist, setCurrentProgressBox,
getShowMediaType, formatDuration, getArtistMetaCached) are package-level and
immutable; change this by caching a single *catalog.Deps at package scope and
initialize it once (e.g. via sync.Once or in package init) so calls to
buildCatalogDeps return the same prebuilt instance instead of allocating
repeatedly; keep the same field assignments but move the allocation to a
one-time initializer and have buildCatalogDeps return the cached variable.

In `@cmd/nugs/download.go`:
- Around line 15-28: buildDownloadDeps currently allocates a new download.Deps
struct on every call (used by album and processTrack), causing many short-lived
allocations during batch downloads; change this by creating a package-level
cached variable (e.g., var cachedDownloadDeps *download.Deps) or initialize it
once with sync.Once and have buildDownloadDeps return that cached instance
instead of recreating it each time; ensure the cached struct is initialized with
the same fields (WaitIfPausedOrCancelled, IsCrawlCancelledErr,
SetCurrentProgressBox, RenderProgressBox, RenderCompletionSummary,
UploadToRclone, RemotePathExists, PrintProgress, UpdateSpeedHistory,
CalculateETA) so callers (album, processTrack) get the same stable function
references.

In `@cmd/nugs/hotkey_input_windows.go`:
- Around line 1-10: Both platform-specific shim files hotkey_input_windows.go
and hotkey_input_unix.go contain identical single-line delegations to
runtime.EnableHotkeyInput; consolidate them by creating a single
non-platform-specific file (remove the //go:build tag) that defines the same
function enableHotkeyInput(fd int) (func(), error) and returns
runtime.EnableHotkeyInput(fd). Keep the import of
"github.com/jmagar/nugs-cli/internal/runtime" and remove the duplicate
platform-specific files to avoid redundant wrappers while preserving behavior
until Phase 12 removal.

In `@cmd/nugs/main.go`:
- Around line 613-630: Replace the raw numeric mediaType cases with named
constants: define an enum-like set of constants (e.g., MediaTypeAlbum,
MediaTypePlaylist, MediaTypeCatalogPlist, MediaTypeVideo, MediaTypeArtist,
MediaTypePaidLstream, etc.) in the model/url-parser layer and import them here,
then update the switch(mediaType) cases to use those constants instead of 0..11
so the dispatch to album, playlist, catalogPlist, video, artist, and paidLstream
is self-documenting and robust to future changes.
- Line 219: Update the error message passed to handleErr in cmd/nugs/main.go to
correct the typo: change "Failed to get subcription info." to "Failed to get
subscription info." in the call to handleErr so the log/error output uses the
correct spelling for "subscription".
- Around line 43-54: The config search currently builds configSearchPaths
without the local "./config.json" and silently ignores errors from
os.UserHomeDir(), so update the logic to (1) include "./config.json" as the
first entry in configSearchPaths and (2) check and handle the error returned by
os.UserHomeDir() (os.UserHomeDir) before using homeDir to construct the other
paths; use the reported error to either skip home-based paths or return/log it,
and then iterate the paths with os.Stat as before (referencing configExists,
configSearchPaths, os.Stat).
- Around line 156-158: The root context in main (ctx := context.Background()) is
not cancelled on OS signals, so thread-safe shutdown and the propagated context
for API calls won't work; replace that with a cancellable context wired to
SIGINT/SIGTERM (e.g., use signal.NotifyContext or create ctx, cancel :=
context.WithCancel(context.Background()) and call signal.Notify for os.Interrupt
and syscall.SIGTERM, calling cancel on receipt), import "os/signal" and
"syscall", and ensure deferred stopHotkeys() and any goroutines use the new ctx
variable so cancellation is propagated through functions that accept ctx.

In `@cmd/nugs/model_aliases.go`:
- Around line 85-88: Document the hidden coupling: add a clear comment beside
the ArgsDescriptionFunc declaration in internal/model/types.go explaining that
package main sets model.ArgsDescriptionFunc in its init() (via argsDescription
in cmd/nugs/model_aliases.go), that this is a global side-effect relied on at
program startup, and note recommended practices for tests (explicitly set or
mock ArgsDescriptionFunc in test setup or import the initializer) so consumers
know about and can override this global.
- Around line 90-183: The argsDescription() function currently uses ~90
positional fmt.Sprintf arguments (colorBold, colorReset, colorCyan, colorGreen,
colorYellow, etc.), which is fragile; refactor it to build the help string
programmatically (e.g., use strings.Builder) and small helpers for headings,
command lines, and examples so each line handles its own color wrapping. In
practice, replace the big fmt.Sprintf call inside argsDescription() with a
builder and three helpers (heading, cmd, example) that call fmt.Fprintf on the
builder using the existing color symbols (colorBold, colorReset, colorCyan,
colorGreen, colorYellow) and return the builder.String(); this keeps color
tokens localized per line and avoids positional argument miscounts.

In `@cmd/nugs/output.go`:
- Around line 29-55: Startup prints call checkRclonePathOnline(cfg)
synchronously which can add startup latency; make the network check non-blocking
by dispatching checkRclonePathOnline(cfg) in a goroutine and collecting its
result via a channel with a short select timeout before printing. In
printStartupEnvironment, replace the direct call to checkRclonePathOnline with
starting a goroutine that sends the result into a chan string (or chan
error/bool you then map to a display string), then use select with a small
timeout (e.g., 500ms) to either read the status or fall back to a placeholder
like "Checking..."/ "Unknown" so startup UI never blocks; keep function names
printStartupEnvironment and checkRclonePathOnline to locate the change.

In `@cmd/nugs/process_alive_unix.go`:
- Line 8: The import "github.com/jmagar/nugs-cli/internal/runtime" shadows the
stdlib runtime; alias the internal package (e.g., nugsruntime) in the import and
update all references in this file from runtime.<X> to nugsruntime.<X> (same
approach as the Windows counterpart) so stdlib runtime remains unshadowed.

In `@cmd/nugs/process_alive_windows.go`:
- Line 8: The import "github.com/jmagar/nugs-cli/internal/runtime" currently
uses the plain name runtime which shadows the Go standard library package;
change the import to an explicit alias (e.g., internalruntime or nugsruntime)
and update all references in this file that call into the internal package (for
example any uses of runtime.<FuncName> like runtime.IsProcessAlive) to use the
new alias (internalruntime.<FuncName>); if you also need the stdlib runtime, add
a separate import "runtime" alongside the aliased internal import.

In `@cmd/nugs/rclone.go`:
- Around line 54-63: ETA may be calculated against the rclone callback `total`
while the UI shows a precomputed directory size set by `onPreUpload`
(`UploadTotalSet`/`progressBox.UploadTotal`), causing inconsistent ETA; change
the ETA logic in the progress handler (where `calculateETA` is called and
`total`/`uploaded` are parsed) to first determine an effective total: if
`progressBox.UploadTotalSet` use `progressBox.UploadTotal` (converted to bytes)
otherwise use `rclone.ParseHumanizedBytes(total)`, then compute `remaining =
effectiveTotal - uploadedBytes` and pass that to
`calculateETA(progressBox.UploadSpeedHistory, remaining)` so the ETA always
matches the shown total.
- Around line 35-117: The outer unnecessary block scope wrapping the whole
function should be removed: delete the leading `{` and trailing `}` and dedent
the body so the code is at the function level (this reduces indentation and
helps meet the 50-line guideline). While editing, consider extracting the large
inline callbacks (progressFn and onPreUpload) into standalone helper functions
(e.g., uploadProgressCallback and preUploadHandler) and pass them into
rclone.UploadToRclone to shorten this function and improve readability.

In `@cmd/nugs/url_parser.go`:
- Line 22: The parameter name `userId` is non-idiomatic in Go; rename it to
`userID` in the parseStreamParams function signature and everywhere it's used,
and also update the corresponding ParseStreamParams declaration in
internal/api/url_parser.go to `userID`, then update all callers to use the new
identifier; ensure you adjust parameter names in functions/methods that forward
or pass this value (e.g., parseStreamParams and ParseStreamParams) and run `go
vet`/`go build` to catch any remaining references.

In `@cmd/nugs/video.go`:
- Around line 12-13: The parameter name `_meta` in the video function is
misleading (underscore implies unused); rename the parameter to meta in the
video function signature and update its usage in the function body where it is
passed to download.Video (change `_meta` -> `meta`) so the argument name
accurately reflects it is used; ensure the function signature for video(ctx
context.Context, videoID, uguID string, cfg *Config, streamParams *StreamParams,
meta *AlbArtResp, isLstream bool, progressBox *ProgressBoxState) matches the
call download.Video(..., meta, ...).

In `@CONFIG.md`:
- Line 200: Several fenced code blocks using triple backticks (```) are not
surrounded by blank lines, causing markdownlint MD031 failures; edit CONFIG.md
to add one blank line before each opening ``` and one blank line after each
closing ``` for the affected blocks (notably the blocks reported at lines 200,
632, 749, 758, 767, 778, 789, 800), ensuring each fenced code block is separated
from adjacent text by blank lines so the linter passes.
- Around line 27-39: Update the Config field-count statement to accurately
reflect which fields are being counted: inspect the Config struct in
internal/model/types.go and decide whether deprecated fields (forceVideo,
skipVideos) and internal/computed fields (urls, wantRes) should be included;
then either change the total number to 25 and note that it includes
deprecated/internal fields, or adjust the wording to state "23
public/configurable fields (excluding deprecated/internal fields: forceVideo,
skipVideos, urls, wantRes)" so the documentation and the Config struct (Config)
are consistent.

In `@internal/api/client.go`:
- Around line 411-420: QueryQuality currently iterates a map (QualityMap) which
is non-deterministic and can yield inconsistent matches when multiple keys
overlap; replace the map iteration with a deterministic ordered check: create an
ordered slice (e.g., QualityPatterns or QualityList) of key/value pairs or keys
sorted by specificity (more specific patterns first, such as by descending key
length or explicit priority) and iterate that slice in QueryQuality, matching
strings.Contains against each key in order and returning the corresponding
Quality (set its URL on the matched copy) to ensure consistent, deterministic
results.

In `@internal/api/doc.go`:
- Around line 1-2: The package comment is stale; replace the placeholder with a
concise package-level doc describing api's purpose and surface (e.g., that
package api provides client functionality and URL parsing used by client.go and
url_parser.go). Update the package comment at the top of internal/api/doc.go to
explain what the package implements (client types, request helpers, URL parsing
utilities) and any intended usage or invariants so future readers know the role
of api without changing client.go or url_parser.go.

In `@internal/api/url_parser.go`:
- Around line 59-64: IsLikelyLivestreamSegments currently relies on
short‑circuiting to avoid an out‑of‑bounds access and returns false for a single
segment; make the behavior explicit and safe by adding an explicit length check
for len(segURLs) == 1 and decide the intended semantics (e.g., treat a single
segment as likely a livestream and return (true, nil) or otherwise document
uncertainty), and add a short comment above IsLikelyLivestreamSegments
explaining the heuristic ("we consider consecutive distinct segment URLs as
indicative of a livestream; a single segment may also indicate an in-progress
livestream"). Ensure the code always checks length before accessing segURLs[1]
(use explicit branches) and update the return for the single-segment case
accordingly.

In `@internal/cache/artist_cache.go`:
- Around line 14-24: GetArtistMetaCachePath currently interpolates artistID
directly into a filename; validate artistID before use by computing safeID :=
filepath.Base(artistID) and returning an error if safeID != artistID or safeID
== "." or safeID == ".." to prevent directory traversal, then use safeID when
building the filename (refer to GetArtistMetaCachePath and the artistID
variable).
- Around line 44-68: WriteArtistMetaCache (and ReadArtistMetaCache) currently
race because they use a deterministic tmp file and no POSIX lock; update
WriteArtistMetaCache to acquire the catalog cache lock (use WithCacheLock() or
syscall.Flock with LOCK_EX) for the artistID before creating/writing the temp
file, create a unique temp file in the same directory using os.CreateTemp (not
cachePath+".tmp"), write the marshaled data to that temp file, fsync if
appropriate, then atomically rename to cachePath and release the lock; likewise
wrap ReadArtistMetaCache with the same WithCacheLock()/Flock to serialize
readers/writers and ensure temp files are removed on error.

In `@internal/cache/cache.go`:
- Around line 79-150: WriteCatalogCache is over 50 lines; extract the metadata
file write and the index-building steps into small helpers to reduce function
length. Create a helper like writeCatalogMeta(meta model.CacheMeta, cacheDir
string, formatDurationFn func(time.Duration) string) (or WriteCatalogMeta) to
handle creating meta, marshalling, writing temp file and atomic rename for
"catalog-meta.json", and another helper like buildCatalogIndexes(catalog
*model.LatestCatalogResp) that calls BuildArtistIndex and BuildContainerIndex
and returns errors. Replace the corresponding blocks in WriteCatalogCache with
calls to these helpers so the main function focuses on orchestration and stays
under the 50-line guideline.
- Around line 152-185: BuildArtistIndex and BuildContainerIndex perform cache
writes but are exported and lack their own locking; either make them unexported
(rename to buildArtistIndex/buildContainerIndex) if only called from
WriteCatalogCache, or add internal locking by invoking WithCacheLock inside each
function before touching files; update the cmd/nugs/cache.go wrapper calls
accordingly so callers use the unexported helpers or rely on the internal
WithCacheLock; ensure references to BuildArtistIndex, BuildContainerIndex,
WriteCatalogCache and WithCacheLock are adjusted so all catalog cache writes are
protected.

In `@internal/cache/filelock_windows.go`:
- Around line 41-48: The Release() method currently ignores the error returned
by fl.lockFile.Close(); change it to capture that error, still nil out
fl.lockFile and attempt os.Remove(fl.path), then return any error(s) to the
caller; specifically, in FileLock.Release capture err := fl.lockFile.Close(),
set fl.lockFile = nil, call removeErr := os.Remove(fl.path), and return a
combined/wrapped error (e.g., using fmt.Errorf to include both Close and Remove
errors) when both fail or the single failing error when only one fails,
otherwise return nil.
- Around line 18-38: The AcquireLock implementation can leave a stale lock file
if the process crashes; modify AcquireLock (and ensure Release/remove logic in
FileLock.Release) to write the current PID into the lock file immediately after
successful creation, and on a failed OpenFile due to existing lockPath read the
lock file contents to obtain the PID (and optionally the file mod time) and
detect staleness: if the recorded PID process is not running (use
platform-appropriate process-check for Windows) or the file mtime exceeds a
configured staleness threshold, remove the stale lock file and retry
acquisition; ensure FileLock.Release still removes the lock file when closing.
Use the existing symbols AcquireLock, FileLock, Release (or FileLock.Release)
and lockPath to locate where to add PID write/read, stale-check, and removal
logic.

In `@internal/catalog/autorefresh.go`:
- Around line 150-154: The call to fmt.Sscanf(timeInput, "%d:%d", &hour,
&minute) can fail and leave hour/minute as zero; change it to capture the return
values (n, err := fmt.Sscanf(...)) and validate that n == 2 and err == nil,
returning an error (e.g., fmt.Errorf("invalid time format: %w", err) or a clear
message) if parsing fails before the range check; update the code around the
timeInput parsing (the fmt.Sscanf invocation and the following range validation)
to use this check so malformed inputs cannot silently pass through.

In `@internal/catalog/deps.go`:
- Around line 17-21: Both RemotePathExists and ListRemoteArtistFolders callbacks
are missing a context.Context parameter, causing underlying implementations
(e.g., the rclone-based functions) to use context.Background() and ignore
cancellation/timeouts; update the signatures of RemotePathExists and
ListRemoteArtistFolders to accept ctx context.Context as the first parameter,
propagate that ctx through all callers and implementations (replace
context.Background() in the rclone implementation with the passed ctx), and
update any code that constructs or invokes these callbacks to forward the
appropriate context so cancellation/timeouts propagate to remote storage
operations.

In `@internal/catalog/handlers.go`:
- Around line 20-21: Move the hardcoded ArtistMetaCacheTTL out of
internal/catalog/handlers.go and make it configurable: either add a constant in
the shared model package (e.g., model.ArtistMetaCacheTTL) or add a TTL field to
your Config (e.g., Config.ArtistMetaCacheTTL) and load it from
environment/defaults; then update all usages in handlers.go (references to
ArtistMetaCacheTTL) to read model.ArtistMetaCacheTTL or cfg.ArtistMetaCacheTTL
so the policy can differ by environment and be tested.
- Around line 129-131: Update the CatalogStats and CatalogLatest function
signatures to accept a context.Context parameter (e.g., func CatalogStats(ctx
context.Context, jsonLevel string) error and func CatalogLatest(ctx
context.Context, ...) ...), add the necessary import for context, and propagate
the ctx through all internal calls and callers that invoke CatalogStats and
CatalogLatest (adjust tests and handler invocations accordingly); if
cache.ReadCatalogCache gains a context-aware variant, call that (or for now pass
ctx to any new wrapper), ensuring the catalog handlers are consistent with the
rest of the API surface that threads context.
- Around line 451-453: The function CatalogGapsFill currently creates sigChan
and calls signal.Notify(sigChan, os.Interrupt), which is a library-level
interception of OS signals; remove the sigChan and signal.Notify/os.Interrupt
usage and instead use the existing ctx for cancellation by replacing the select
branch that listens on sigChan with one that listens on ctx.Done(); update any
related defer signal.Stop(sigChan) removal and ensure CatalogGapsFill observes
ctx.Done() where appropriate so callers control cancellation via context.

In `@internal/catalog/helpers.go`:
- Around line 80-105: ShowExists currently hardcodes model.MediaTypeAudio when
resolving local paths (in resolver.LocalShowPath), so video-only shows saved
under the VideoOutPath are missed; change ShowExists to accept a mediaType
parameter (e.g., mediaType model.MediaType) or iterate over both media types and
check both resolver.LocalShowPath(show, mediaType) and
resolver.RemoteShowPath(show) accordingly, ensuring you call
deps.RemotePathExists with the same remotePath logic; update all callers of
ShowExists to pass the appropriate mediaType (or rely on the combined check) and
keep WarnRemoteCheckError handling unchanged.
- Around line 178-181: The code builds a remote storage path using filepath.Join
which yields OS-specific separators (backslashes on Windows) and breaks rclone
remotes; in the block that checks cfg.RcloneEnabled && idx.RemoteListErr != nil
&& deps.RemotePathExists != nil (where remotePath is set), replace filepath.Join
with a remote-safe join (e.g., use path.Join from the path package or
concatenate with "/" ) so remotePath uses forward slashes; update imports
accordingly and keep the variable name remotePath and the call to
deps.RemotePathExists(remotePath, cfg, false) unchanged.
- Around line 41-66: ListAllRemoteArtistFolders currently spawns rclone with
exec.Command which cannot be cancelled; change its signature to accept ctx
context.Context, replace exec.Command with exec.CommandContext(ctx, ...) and
propagate ctx from callers (e.g., CatalogCoverage) so the external process
respects deadlines/cancellations; ensure you update all call sites to pass the
received context and preserve existing error handling (including the
exec.ExitError/ExitCode check) and return semantics.

In `@internal/catalog/media_filter_test.go`:
- Around line 86-92: Replace the manual environment save/restore pattern using
os.Setenv + t.Cleanup with the testing helper t.Setenv: instead of capturing
originalPath and calling os.Setenv(...)+t.Cleanup(...), call t.Setenv("PATH",
binDir+string(os.PathListSeparator)+originalPath) (or build the new PATH value
first) and remove the t.Cleanup/originalPath logic; this uses the testing.T
method (t.Setenv) to automatically restore the environment and prevents
accidental use of t.Parallel() in this test.

In `@internal/catalog/media_filter.go`:
- Around line 92-178: The function AnalyzeArtistCatalogMediaAware is overlong;
extract the per-show classification loop (the block that iterates allShows and
builds model.ShowStatus using MatchesMediaFilter and ShowExistsForMediaIndexed)
into a new helper e.g., classifyShows(ctx, allShows, mediaFilter, presenceIdx,
cfg, deps) that returns the populated slices and counters (Shows
[]model.ShowStatus, MissingShows []model.ShowStatus, Downloaded int) or takes a
pointer to the analysis and updates it; call this helper from
AnalyzeArtistCatalogMediaAware, then compute TotalShows, Missing, DownloadPct
and MissingPct there (using the returned/updated values) so
AnalyzeArtistCatalogMediaAware stays under 50 lines. Ensure the helper uses the
same logic for showMedia detection (deps.GetShowMediaType fallback), media
filtering via MatchesMediaFilter, and existence check via
ShowExistsForMediaIndexed with presenceIdx and deps.
- Line 97: Remove the redundant local constant artistMetaCacheTTL and replace
its usages with the package-level constant ArtistMetaCacheTTL; specifically,
delete the line declaring const artistMetaCacheTTL = ArtistMetaCacheTTL and
update any references (e.g., where artistMetaCacheTTL is used in
media_filter.go) to use ArtistMetaCacheTTL directly.

In `@internal/config/config.go`:
- Around line 36-55: The code reads user input with scanner.Scan() (for email
and password) but never checks scanner.Err(), so I/O errors are masked as empty
input; update the input flow in the function that performs the first-time setup
to check scanner.Scan() return values and call scanner.Err() after each Scan (or
after both reads) and return the actual error when scanner.Err() != nil instead
of treating it as an empty string; ensure you reference the existing scanner
variable and preserve the current validation for empty email/password but
surface scanner.Err() first (e.g., if !scanner.Scan() or scanner.Err() != nil,
return the scanner error).
- Around line 35-224: PromptForConfig is too long; extract interactive sections
into small helper functions so the top-level PromptForConfig orchestrates flow
and stays under ~50 lines. Create helpers like promptEmail() (returns
email,string/error), promptPassword(), promptFormat() (returns int/error),
promptVideoFormat(), promptOutPaths() (returns outPath,videoOutPath),
promptFfmpegFlag(), promptRclone() (returns
rcloneEnabled,rcloneRemote,rclonePath,rcloneVideoPath,deleteAfterUpload,rcloneTransfers,error),
and writeConfigFile(cfg model.Config) to handle JSON/write logic; call these
from PromptForConfig, propagate errors, and assemble the model.Config before
calling writeConfigFile. Ensure each helper handles its own input validation and
use the existing names (PromptForConfig, model.Config) so refactor is localized
and testable.
- Around line 209-217: Replace the direct os.WriteFile calls used when
persisting the config with an atomic-write pattern: in the function containing
the shown snippet and in WriteConfig, marshal the config to data, create a temp
file at configPath + ".tmp" (or use os.Create), write the data to that temp
file, ensure data is flushed (close/sync as appropriate), then atomically
replace the target by os.Rename(tempPath, configPath); handle and return errors
from each step and ensure file mode 0600 is applied to the temp before rename.
This ensures atomic replacement and prevents config corruption on interrupts.
- Around line 49-55: The password is being read with scanner.Scan() which echoes
input; replace that logic to use golang.org/x/term.ReadPassword to read from the
terminal without echoing. Specifically, where the code currently prompts and
uses scanner.Scan() and password := strings.TrimSpace(scanner.Text()) (and
returns errors.New("password is required")), instead call fmt.Print (or
fmt.Printf) to prompt, call term.ReadPassword(int(os.Stdin.Fd())) to get a
[]byte, convert to string and strings.TrimSpace it, check for empty and return
the same error if empty, and ensure you add handling for ReadPassword errors and
print a newline after the call (since ReadPassword doesn't emit one). Use the
symbols password, scanner (to remove/replace), strings.TrimSpace, and the error
return unchanged.

In `@internal/download/audio.go`:
- Around line 459-460: The progress display double-counts because ShowDownloaded
is computed from progressBox.AccumulatedBytes plus the current callback's
downloaded value while AccumulatedBytes is also updated after the download
completes; to fix it, introduce and use a stable snapshot field (e.g.,
progressBox.AccumulatedBeforeCurrent) that you set once under the same lock
before starting the current file download, then in the progress callback compute
ShowDownloaded = humanize.Bytes(uint64(progressBox.AccumulatedBeforeCurrent +
downloaded)); keep the existing post-download update that adds downloaded into
progressBox.AccumulatedBytes (and clear AccumulatedBeforeCurrent when done) so
the displayed total increments smoothly without a jump — update the code paths
around the progress callback, the start-of-download setup, and the post-download
AccumulatedBytes update (referencing progressBox, ShowDownloaded,
AccumulatedBytes, AccumulatedBeforeCurrent, and the download progress callback).
- Around line 40-66: The function WriteCounterWrite is exported but only used
internally (via writeCounterAdapter); rename it to an unexported identifier
(e.g., writeCounterWrite) and update all internal callers (notably
writeCounterAdapter) to call the unexported name, ensuring signatures remain
identical; if it was exported solely for tests, add a test-only accessor in a
_test.go file instead of keeping it public. Also apply the same change to the
other similarly-exported helper in the same file (the function referenced around
lines 116-124): make it unexported and update internal callers or provide a test
accessor as needed. Ensure package-level visibility and imports are adjusted
accordingly.
- Around line 69-113: DownloadTrack currently opens the destination file
(os.OpenFile) before making the HTTP request, leaving a zero-byte file on
failure; move file creation to after a successful api.Client.Do response and
status check (i.e., only call os.OpenFile once do.StatusCode is
OK/PartialContent) and then perform io.Copy into that file; additionally, if you
prefer to keep earlier file creation, ensure you remove the file on any error
path (after api.Client.Do failure or io.Copy error) so ProcessTrack won't see a
stale zero-byte file. Reference: DownloadTrack, os.OpenFile, api.Client.Do,
do.StatusCode, io.Copy, and ProcessTrack.
- Around line 308-509: ProcessTrack is too long; extract the large inline
progress callback and the quality-selection/fallback block into separate helper
functions to reduce body size under 50 lines. Create a helper like
buildProgressCallback(progressBox *model.ProgressBoxState, deps *Deps, trackNum,
trackTotal int, cfg *model.Config) func(downloaded, total, speed int64) that
captures and updates progressBox and calls deps.RenderProgressBox (preserving
all current logic: accumulated bytes, ETA calculations, speed history,
ShowPercent computation, and fallback to deps.PrintProgress when
progressBox==nil). Also extract the quality probing and fallback loop into a
function like selectTrackQuality(ctx context.Context, track *model.Track,
streamParams *model.StreamParams, wantFmt int) (*model.Quality, bool, error)
which performs the api.GetStreamMeta probing, builds quals, calls
CheckIfHlsOnly/ParseHlsMaster or api.GetTrackQual with the fallback loop and
returns the chosenQual and isHlsOnly flag (preserve original behavior of
origWantFmt vs wantFmt handling and progressBox warning setting caller-side).
Replace the inlined blocks in ProcessTrack with calls to these helpers and keep
all side-effects and error returns identical.
- Around line 55-58: The speed calculation uses integer division causing loss of
precision; in the block using wc.StartTime, wc.Downloaded and model.KBpsDivisor
(and similarly in simpleWriteCounter.Write in video.go) convert operands to a
floating type before dividing so you perform the multiplication/division in
float and only convert back to int64 at the end (e.g., compute floatSpeed :=
float64(wc.Downloaded) * float64(model.KBpsDivisor) / float64(toDivideBy) and
then set speed = int64(floatSpeed) after the existing toDivideBy != 0 check);
update both occurrences (the code referencing wc.Downloaded/wc.StartTime and
simpleWriteCounter.Write) to use this float calculation to preserve precision.
- Around line 525-531: The shared context used for pre-calculation (ctx, cancel)
is currently a global timeout computed from model.PreCalcPerTrackTimeout and
clamped by model.PreCalcMaxTimeout and it covers semaphore acquisition plus
per-track HTTP HEADs, causing waiting goroutines to burn the shared budget;
change the code to use context.Background() for semaphore and overall
orchestration (or significantly increase the computed timeout) and rely on the
existing per-request contexts used for the HTTP HEADs (the per-request timeout
created at the HTTP HEAD call), removing the short-lived shared WithTimeout
usage (and its cancel) so semaphore waits don't consume per-request timeouts.
- Around line 724-731: The error path in downloadAlbumAudio assumes progressBox
is non-nil and directly does progressBox.Mu.Lock/Unlock and updates fields; add
a nil-check around all accesses so if progressBox == nil you skip locking and
updates. Specifically, guard the block that increments ErrorTracks (the
progressBox.Mu.Lock/Unlock and ErrorTracks++), and also guard the subsequent
accesses to progressBox.IsComplete, progressBox.CompletionTime and
progressBox.TotalDuration so they are only read/modified when progressBox !=
nil.
- Around line 136-150: The code uses unsafe type assertions for
playlist.(*m3u8.MasterPlaylist) in ParseHlsMaster and
playlist.(*m3u8.MediaPlaylist) in HlsOnly which can panic; update both functions
to perform a safe type check (use a type assertion with the comma-ok pattern or
a type switch) after m3u8.DecodeFrom, return a clear error if the playlist is
not the expected type, and only proceed when the assertion succeeds (referencing
ParseHlsMaster and HlsOnly to locate the checks).

In `@internal/download/batch.go`:
- Around line 25-30: The code checks len(meta) but not the Containers slice, so
accessing meta[0].Response.Containers[0] can panic; update the logic in
internal/download/batch.go (near the existing meta and GetAlbumTotal usage) to
verify that meta[0].Response is non-nil and that
len(meta[0].Response.Containers) > 0 before reading Containers[0].ArtistName —
if Containers is empty (or Response nil) return a descriptive error instead of
proceeding; only call GetAlbumTotal and print ArtistName after these guards
pass.
- Around line 18-86: The Artist function is over the 50-line limit; extract the
nested album-iteration loop (the code iterating meta ->
_meta.Response.Containers and handling deps.WaitIfPausedOrCancelled, batchState
updates, calling Album, and error handling) into a new helper function (e.g.,
processArtistAlbums(ctx, meta, cfg, streamParams, batchState, sharedProgressBox,
deps)) and call it from Artist; ensure the helper accepts and updates batchState
and uses sharedProgressBox, preserves the Album(...) call paths (including the
conditional cfg.SkipVideos and strconv.Itoa(container.ContainerID)), and returns
any error so Artist can handle cancellation and final return.
- Around line 130-145: Extract the track-iteration loop starting at "for
trackNum, track := range meta.Items" into a new helper (e.g., processTracks or
iteratePlaylistTracks) that accepts the same dependencies and parameters (ctx,
plistPath, trackTotal, cfg, streamParams, progressBox, deps and meta.Items) and
returns an error; move the logic that increments trackNum, calls
deps.WaitIfPausedOrCancelled, invokes ProcessTrack, checks
deps.IsCrawlCancelledErr(err), and calls helpers.HandleErr into that helper so
behavior is identical, then replace the loop in Playlist with a single call to
the new helper to bring Playlist under 50 lines.
- Around line 160-167: The PaidLstream function creates an unnecessary
intermediate variable for the returned error; simplify by calling
api.ParsePaidLstreamShowID into showId and then directly returning the result of
Video without assigning err — update PaidLstream to remove the extra err
assignment and just: parse showId with api.ParsePaidLstreamShowID, handle the
parse error as before, then return the call to Video(ctx, showId, uguID, cfg,
streamParams, nil, true, nil, deps).

In `@internal/download/deps.go`:
- Around line 52-64: Add a context.Context parameter to Deps.UploadPath and
Deps.CheckRemotePathExists and forward that ctx into the storage calls instead
of using context.Background(); update prepareAlbumPaths to accept ctx and thread
that ctx through its callers (Album, Video, Playlist) so cancellation
propagates; when calling Storage.Upload from UploadPath pass the incoming ctx
and construct/pass the proper model.StorageHooks (not an empty StorageHooks{})
so progress tracking/hooks are preserved.

In `@internal/download/regression_test.go`:
- Around line 105-162: The test TestHlsOnly_ValidationErrors_NoPanic ties the
assertion for the invalid-playlist case to an exact m3u8 library error string
("detect playlist type"); change the assertion logic so library-originated
errors are only checked for non-nil instead of substring-matching that exact
message, while keeping substring checks for your own sentinel messages like "no
encryption key". Concretely, in TestHlsOnly_ValidationErrors_NoPanic adjust the
table/assertion around tc.wantErrText (or add a flag) so when the case is the
invalid playlist (the one that triggers the m3u8 error) you assert gotErr !=
nil, and for the "rejects playlist with no key" case continue to assert
strings.Contains(gotErr.Error(), "no encryption key"); keep references to
HlsOnly and the test name to locate where to change.

In `@internal/download/video.go`:
- Around line 292-308: simpleWriteCounter.Write duplicates progress calculation
from WriteCounterWrite (same percentage/speed logic) and reintroduces the
integer-division precision bug; extract the shared progress calculation into a
helper (e.g., computeProgress(downloaded int64, total int64, startTime int64)
returning percentage int and speed int64) and have both simpleWriteCounter.Write
and WriteCounterWrite call it; add an optional hook parameter (e.g.,
onPauseOrCancel func() bool or context-aware callback) so WriteCounterWrite can
perform its pause/cancel check before/after calling the shared helper without
duplicating logic.
- Around line 456-457: The END calculation uses int(math.Round(nextChapStart)-1)
which can produce a negative END (e.g., when nextChapStart rounds to 0); change
the logic to compute end := int(math.Round(nextChapStart)) - 1, then clamp end
to >= 0 (if end < 0 set end = 0) before writing the chapter line in the same
place where f.WriteString is called using start and nextChapStart; update the
fmt.Sprintf to use this safe end variable.
- Around line 310-347: DownloadLstream currently creates HTTP requests without
context, preventing cancellation; change the signature of DownloadLstream to
accept a context.Context (matching DownloadVideoFile), use
http.NewRequestWithContext(ctx, ...) or attach ctx via req =
req.WithContext(ctx) when creating the request that currently calls
http.NewRequest, pass that request into api.Client.Do so the request can be
cancelled, and propagate the ctx addition to all callers of DownloadLstream;
also ensure the function returns nil on success rather than the stray err
variable at the end.
- Around line 140-144: The code uses unsafe type assertions after
m3u8.DecodeFrom (playlist.(*m3u8.MasterPlaylist) and
playlist.(*m3u8.MediaPlaylist)) which can panic; change both assertions to the
comma-ok form to check the concrete type, return a clear error if the cast fails
(e.g., "unexpected playlist type") instead of letting it panic, and propagate
that error from the surrounding function that calls DecodeFrom so callers can
handle it; update the code paths that currently assume master or media playlist
(references: the variables named playlist, m3u8.MasterPlaylist,
m3u8.MediaPlaylist, and the DecodeFrom call) to use the validated typed value
when ok is true.
- Around line 566-818: The Video function is too large and should be split into
smaller stage-specific functions; extract logical blocks into helpers such as
resolveVideoMeta (handles meta retrieval, SKU selection: Video, GetLstreamSku,
GetVideoSku, api.GetAlbumMeta), resolveManifestAndVariant
(api.GetStreamMeta/api.GetPurchasedManURL, GetManifestBase, ChooseVariant,
api.IsLikelyLivestreamSegments), preparePathsAndChecks (BuildAlbumFolderName,
MakeDirs, FileExists, remote check via deps.CheckRemotePathExists),
downloadVideoContent (wraps DownloadLstream and DownloadVideoFile with
progressBox update callbacks), handleChaptersAndConversion (GetDuration,
WriteChapsFile, TsToMp4, delete TS), and uploadAndFinalize (deps.UploadPath and
progressBox completion); keep Video as an orchestration layer invoking these new
functions and moving progressBox setup/teardown into a small helper so each
extracted function stays focused and under ~50 lines while reusing existing
symbols like DownloadLstream, DownloadVideoFile, TsToMp4, GetDuration,
WriteChapsFile, deps.UploadPath, PrepareVideoProgressBox, ChooseVariant.
- Around line 234-284: DownloadVideoFile currently assumes the server honors the
Range header which corrupts resumed files if the server returns 200 OK; modify
DownloadVideoFile to accept a context.Context, create the request with
http.NewRequestWithContext, and use api.Client.Do(req) with that context to
allow cancellation; then detect response status: if StatusOK, reset resume state
by truncating or reopening the target file (or set startByte=0 and
f.Truncate(0)) and set counter.Downloaded=0 before writing, whereas if
StatusPartialContent keep the existing seek-to-startByte behavior and
counter.Downloaded=startByte; ensure the progress counter (model.WriteCounter /
simpleWriteCounter) reflects the adjusted start byte and that on error you still
close resources.

In `@internal/helpers/errors.go`:
- Around line 65-68: The ReadTxtFile function currently opens files with
os.OpenFile(path, os.O_RDONLY, 0755) which is misleading because the mode is
ignored for read-only opens; replace that call with os.Open(path) in the
ReadTxtFile function so the intent is clear and remove the unused permission
argument.
- Around line 101-119: The deduplication check currently runs before trimming
trailing slashes, so duplicates like "https://example.com/" and
"https://example.com" can both be added; fix this in the loop over urls by
trimming each candidate (use strings.TrimSuffix(url, "/") or TrimSuffix(txtLine,
"/") for lines read via ReadTxtFile) before calling Contains(processed, ...),
and then append the trimmed value to processed and txtPaths as appropriate;
update both the .txt branch (processing txtLines from ReadTxtFile) and the
non-.txt branch to perform TrimSuffix first, then Contains, then append.

In `@internal/helpers/paths.go`:
- Around line 51-62: Replace the hardcoded default limit (120) in
BuildAlbumFolderName with the shared constant model.AlbumFolderMaxRunes so
there’s a single source of truth; specifically, set limit :=
model.AlbumFolderMaxRunes, keep the existing optional maxLen override logic
intact, and ensure the code still trims and sanitises the constructed
albumFolder (Sanitise and BuildAlbumFolderName remain unchanged otherwise).
- Around line 198-205: RemoteShowPath uses filepath.Join which is OS-dependent
and yields backslashes on Windows; change it to use the POSIX path.Join so
remote storage paths are always forward-slash separated. In the
ConfigPathResolver.RemoteShowPath function (and related helpers like
BuildAlbumFolderName/Sanitise usage), replace filepath.Join with path.Join and
import the standard "path" package instead of or alongside "path/filepath" as
appropriate so the returned remote path is normalized for remote storage.

In `@internal/list/deps.go`:
- Around line 1-20: The call to the injected callback Deps.Playlist is invoked
without a nil check; add a guard before calling it so you only call
deps.Playlist(...) when deps.Playlist != nil and otherwise handle the nil case
(e.g., return an appropriate error or fallback). Locate the code that invokes
deps.Playlist with parameters (ctx, resolvedId, legacyToken, cfg, streamParams,
true) and wrap that invocation with an if deps.Playlist != nil { return
deps.Playlist(...) } (and add the corresponding else behavior consistent with
surrounding code).

In `@internal/list/list.go`:
- Around line 333-346: The date-sorting comparator for model.ContainerWithDate
is duplicated across ListArtistShows, ListArtistShowsByVenue, and
ListArtistLatestShows (and other spots); extract a single helper function
sortContainersByDateDesc(containers []model.ContainerWithDate) that encapsulates
the comparator logic and replace the inline sort.Slice calls with a call to that
helper; similarly factor repeated JSON marshaling and empty-result handling into
shared helpers (e.g., writeJSONResponse(w, v) and writeEmptyResult(w)) and call
those from ListArtistShows, ListArtistShowsByVenue, and ListArtistLatestShows to
remove the copy-pasted blocks while preserving existing behavior.
- Around line 250-439: ListArtistShows, ListArtistShowsByVenue, and
ListArtistLatestShows are each >50 lines and duplicate the same phases (fetch
metadata, derive artist name, collect/filter containers, sort, render
JSON/table); refactor by extracting shared phases into small helpers such as
FetchArtistMetaWithAvailType(ctx, artistId, availType, deps) for the API
call/error handling, ResolveArtistName(allMeta) to derive artistName,
CollectAndFilterContainers(allMeta, deps, mf) returning
[]model.ContainerWithDate, SortContainersDesc([]model.ContainerWithDate) to
encapsulate the sort logic, and RenderShowsOutput(allContainers, artistId,
artistName, jsonLevel, mf) to handle JSON/table rendering; replace the large
bodies of ListArtistShows, ListArtistShowsByVenue, and ListArtistLatestShows
with calls to these helpers so each function is under 50 lines and duplication
is removed.
- Around line 756-779: ResolveCatPlistID currently performs an uncancellable
HTTP GET via api.Client.Get and misnames the response as req; update the
function signature to accept a context.Context and create the request with
http.NewRequestWithContext(ctx, "GET", plistUrl, nil) (or use api.Client.Do with
that request) so the call can be cancelled; rename the response variable (e.g.,
resp) and ensure resp.Body is closed, check resp.StatusCode, parse
resp.Request.URL, and preserve all existing error handling and query parsing
logic.

In `@internal/model/progress_box.go`:
- Around line 146-149: The default branch in isValidTransition currently returns
true and thus permits any unknown from-phase to transition freely; change the
default to reject unknown phases by logging a warning and returning false
instead — i.e., in isValidTransition, replace the default: return true with
logic that emits a warning (using the module's logger or the standard logger)
mentioning the unrecognized from-phase value and then returns false so typos
like "donwload" are caught.
- Around line 281-345: ResetForNewAlbum currently leaves CurrentPhase unchanged
causing stale phase (e.g., "upload" or "complete") to carry over; update
ResetForNewAlbum to set s.CurrentPhase = PhaseIdle (or the Idle constant used by
SetPhase) so new albums start in the idle phase and subsequent calls like
SetPhase(PhaseDownload) validate correctly; locate ResetForNewAlbum and ensure
CurrentPhase is assigned the correct Idle-phase symbol used across the
progress_box (and update any related comments).

In `@README.md`:
- Around line 297-300: Update the Media Type Indicators list to remove the
repeated word "only" by changing the lines for the emoji markers: replace "🎵
Audio only" with "🎵 Audio", replace "🎬 Video only" with "🎬 Video", and adjust
"📹 Both audio and video available" to a concise form such as "📹 Both audio and
video" so the three entries are consistent and non-redundant.

[coderabbitai]

Review continued from previous batch...

[jmagar]

PR Review Comments Addressed

All valid technical issues from automated code review have been addressed in commit b63baf9.

✅ Issues Fixed (9 total)

P0 Critical:

• Infinite loop in trackFallback - verified already fixed with format 5 entry and loop guards

P1 High Priority (4 fixes):

1. Media-aware presence index - BuildArtistPresenceIndex now handles video paths correctly
2. Media-aware remote listing - ListRemoteArtistFolders now accepts isVideo parameter
3. CheckURL ambiguous return - Now returns -1 for no match (was ambiguous 0)
4. ParseTimestamps silent failure - Now validates and returns errors instead of silent failure

P2 Medium Priority (4 fixes):
5. Duplicate error counters - Consolidated to internal/ui package (removed cmd/nugs duplicates)
6. Sanitise whitespace bug - Now uses TrimSpace() to handle all whitespace
7. Stale package doc - Updated internal/rclone documentation
8. Duplicate test helpers - Removed local copies, using internal/testutil

P3 (1 fix):
9. Go naming violation - Fixed: ResolveCatPlistId → ResolveCatPlistID

⏭️ Deferred Issues

• 261 markdown linting issues - Missing language specifiers on code blocks (cosmetic, will address in dedicated documentation pass)
• 307 missing godocs - Requires broader documentation effort, outside scope of this PR

📊 Results

• Files Modified: 14 files
• Lines Changed: +65 insertions, -78 deletions (net -13 lines)
• Tests: ✅ All passing
• Regressions: None

All functional issues have been resolved. The codebase is now cleaner, more correct, and video support is properly integrated.

[coderabbitai]

Actionable comments posted: 46

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

internal/completion/completions.go (2)

379-384: ⚠️ Potential issue | 🟡 Minor

PowerShell completion filter for completion subcommand is inconsistent.

Line 383 uses $_ -like "$wordToComplete*" which applies -like against the CompletionResult object's default string representation, rather than $_.CompletionText -like "$wordToComplete*" as used in other blocks (lines 341, 365, 370). This may cause incorrect filtering.

♻️ Proposed fix

-            } | Where-Object { $_ -like "$wordToComplete*" }
+            } | Where-Object { $_.CompletionText -like "$wordToComplete*" }

---

228-229: ⚠️ Potential issue | 🟠 Major

Fish completions misuse __fish_seen_argument -s for subcommand checking.

__fish_seen_argument is a legitimate fish helper function, but the -s flag checks for short options (single-character flags like -a, -v), not subcommand words. Lines 228–229 use __fish_seen_argument -s artists to detect the artists subcommand, which fails because artists is not a short option. The same issue affects lines 243–249 (config and gaps).

Replace with a direct commandline check:

♻️ Proposed fix (excerpt for lines 228-229)

-complete -c nugs -n "__fish_seen_subcommand_from list; and __fish_seen_argument -s artists" -a "shows" -d "Filter by show count"
+complete -c nugs -n "__fish_seen_subcommand_from list; and contains -- artists (commandline -opc)" -a "shows" -d "Filter by show count"

Apply the same pattern for lines 243–249 (config and gaps checks).

internal/catalog/autorefresh.go (1)

76-94: 🧹 Nitpick | 🔵 Trivial

AutoRefreshIfNeeded uses fmt.Fprintf(os.Stderr, ...) instead of ui helpers.

Lines 79, 87, and 90 write directly to stderr. For consistency with the rest of the codebase's UI centralization (and to get colored output), consider using ui.PrintWarning / ui.PrintInfo / ui.PrintError.

internal/model/api_types.go (1)

447-483: 🧹 Nitpick | 🔵 Trivial

Artist and ArtistOutput have identical fields.

Artist (Line 448) and ArtistOutput (Line 478) define the same four fields with the same JSON tags. Consider using a type alias or embedding one within the other to avoid duplication, or document why they are intentionally kept separate (e.g., if ArtistOutput may diverge in the future).

cmd/nugs/catalog_handlers_test.go (1)

20-29: 🧹 Nitpick | 🔵 Trivial

Redundant os.Setenv in cleanup — t.Setenv already restores.

t.Setenv (Line 24) automatically restores the original value when the test completes. The manual t.Cleanup with os.Setenv on Line 26 is redundant.

♻️ Simplification

 func withTempHome(t *testing.T) string {
 	t.Helper()
-	origHome := os.Getenv("HOME")
 	tempHome := t.TempDir()
 	t.Setenv("HOME", tempHome)
-	t.Cleanup(func() {
-		_ = os.Setenv("HOME", origHome)
-	})
 	return tempHome
 }

internal/catalog/handlers.go (1)

540-748: 🧹 Nitpick | 🔵 Trivial

CatalogCoverage is ~200 lines — well above the 50-line guideline.

This function handles three distinct responsibilities: (1) artist discovery from local+remote directories (lines 552–651), (2) per-artist analysis (lines 653–669), and (3) output rendering (lines 671–748). Consider extracting at least the discovery phase into a helper like discoverArtistIDs(cfg, catalog) ([]string, error).

As per coding guidelines, "Keep Go functions under 50 lines where possible."

🤖 Fix all issues with AI agents

In `@CLAUDE.md`:
- Around line 75-78: The "**Key Stats:**" section contains hardcoded values ("50
Go source files" and "Entry Point: `cmd/nugs/main.go` (648 lines)") that will
become stale; replace those literal counts with either a dynamic shell command
example (e.g., suggest running find ... | wc -l or wc -l on the file) or remove
the specific numeric values and keep only the descriptive labels so the README
stays accurate over time; update the lines containing the exact phrases "50 Go
source files" and "cmd/nugs/main.go (648 lines)" accordingly.

In `@cmd/nugs/api_client.go`:
- Around line 14-25: Rename the non-idiomatic constant aliases to use Go
conventions: change clientId → clientID, authUrl → authURL, streamApiBase →
streamAPIBase, subInfoUrl → subInfoURL, userInfoUrl → userInfoURL, playerUrl →
playerURL (leave devKey, layout, userAgent, userAgentTwo as-is if appropriate)
and have each new constant continue to reference the original api.* values
(e.g., clientID = api.ClientID). Update all local references to these constants
in the package to use the new names to avoid build errors.

In `@cmd/nugs/cancel_unix.go`:
- Around line 8-10: The import name runtime shadows the Go standard library
package; change the import to an explicit alias (for example `intruntime` or
similar) and update the wrapper function to call the aliased package's symbol,
i.e. replace the import "github.com/jmagar/nugs-cli/internal/runtime" with an
aliased import and change cancelProcessByPID to call the aliased package's
CancelProcessByPID (e.g. intruntime.CancelProcessByPID).

In `@cmd/nugs/catalog_handlers.go`:
- Around line 46-67: Rename the inconsistent parameter name artistId to artistID
in the catalog handler functions to follow Go initialism convention: update
function signatures and all uses inside catalogGapsForArtist, catalogGapsFill,
catalogCoverage, catalogList, and catalogListForArtist (and catalogGaps if
applicable) so the parameter and any forwarded call to
catalog.CatalogGapsForArtist, catalog.CatalogGapsFill, catalog.CatalogCoverage,
catalog.CatalogList, and catalog.CatalogListForArtist use artistID consistently;
ensure buildCatalogDeps() calls and variable forwarding remain unchanged and run
tests/build to verify no remaining references to artistId.

In `@cmd/nugs/detach_windows.go`:
- Around line 8-12: The import named "runtime" shadows the Go stdlib; update the
import to use an explicit alias (e.g., runtimepkg or internalruntime) for
"github.com/jmagar/nugs-cli/internal/runtime" and then change the call in
maybeDetachAndExit from runtime.MaybeDetachAndExit(_args, urls) to use that
alias (e.g., runtimepkg.MaybeDetachAndExit(_args, urls)) so the stdlib runtime
is not shadowed.

In `@cmd/nugs/main.go`:
- Around line 105-246: The run function is too long; extract the early-exit
command handling (the "status", "cancel", and "completion" blocks around
maybeDetachAndExit and completionCommand) into a helper like
handleEarlyCommands(cfg *Config) bool, and extract the authentication/setup
block (makeDirs, auth, getUserInfo, getSubInfo, extractLegToken,
parseStreamParams, and related planDesc logic) into a helper like
performAuthAndInit(ctx, cfg) (token string, userId string, streamParams
StreamParams, err error); update run to call these helpers, propagate errors via
handleErr as before, and ensure return values (e.g., whether an early command
handled) are used so run stays <50 lines while keeping existing behavior and
references to maybeDetachAndExit, handleListCommand, handleCatalogCommand,
handleArtistShorthand, auth, getUserInfo, getSubInfo, extractLegToken,
parseStreamParams, and makeDirs.
- Around line 538-540: The "full" shorthand builds an invalid URL with a hash
fragment ("https://play.nugs.net/#/artist/%d") that fails CheckURL() (see
pattern 5 in internal/api/url_parser.go); update the "full" case in
cmd/nugs/main.go to produce the canonical path-based URL (use
fmt.Sprintf("https://play.nugs.net/artist/%d", artistID)) so cfg.Urls contains
the form that CheckURL()/url_parser.go recognizes and the item is no longer
skipped as "Invalid URL".

In `@cmd/nugs/output.go`:
- Around line 45-54: The code currently calls checkRclonePathOnline even when
cfg.RcloneEnabled is false, causing an unnecessary network call; change this by
computing a rcloneStatus string variable: if cfg.RcloneEnabled is true call
checkRclonePathOnline(cfg) and assign its result to rcloneStatus, otherwise set
rcloneStatus to "Disabled", then pass rcloneStatus to printKeyValue("Rclone
Status", ...). Update the block around rcloneAudioPath/rcloneVideoPath so that
checkRclonePathOnline is only invoked inside the cfg.RcloneEnabled branch
(referencing cfg.RcloneEnabled, checkRclonePathOnline, rcloneAudioPath,
rcloneVideoPath, and printKeyValue).

In `@cmd/nugs/rclone.go`:
- Around line 26-31: Change uploadToRclone to accept a context.Context and
forward it into rclone.UploadToRclone instead of using context.Background();
also update the uploadWithProgressBox signature to accept the same context and
forward it when it is called so cancellation/timeout propagate. Specifically,
modify the function signatures for uploadToRclone and uploadWithProgressBox to
include ctx context.Context, replace the hard-coded context.Background() in the
rclone.UploadToRclone call with ctx, and update any call sites that invoke
uploadToRclone or uploadWithProgressBox to pass along the caller's context.
Ensure rclone.UploadToRclone still receives the same other parameters.

In `@cmd/nugs/signal_persistence_unix.go`:
- Around line 8-10: The import of the internal package is named "runtime",
shadowing the Go stdlib "runtime"; change the import to an explicit alias (e.g.,
internalruntime or iruntime) and update the call in setupSessionPersistence to
use that alias (e.g., internalruntime.SetupSessionPersistence()) so stdlib
runtime remains available and no ambiguous binding occurs.

In `@CONFIG.md`:
- Around line 747-752: Several JSON example code blocks include inline
C++/JavaScript-style comments (e.g., annotations next to "format" and
"videoFormat") which will cause parse errors if copied; remove all // comments
from JSON code fences and instead move annotations outside the code blocks or
convert the blocks to a JSONC/annotated section. Specifically, update the
example that shows "format" and "videoFormat" to present only valid JSON in the
fenced block and place the explanatory text (e.g., that "format" must be 1-5 and
"videoFormat" must be 1-5) as plain text or bullet points immediately before or
after the block so the snippet remains copy-pastable.

In `@internal/api/client.go`:
- Around line 35-41: The package-level exported mutable variables Jar and Client
(created via mustCookieJar and initialized as Client) reduce testability;
refactor by introducing a struct-based API client (e.g., type APIClient struct {
Client *http.Client; Jar http.CookieJar }) and provide a constructor
NewAPIClient(client *http.Client) that accepts an optional *http.Client (if nil,
create a default using mustCookieJar and the 30s timeout) so tests can inject a
mock *http.Client; then remove or unexport the package-level Jar and Client and
update call sites to use APIClient methods.
- Around line 283-322: The pagination loop in getArtistMetaByAvailType can spin
indefinitely if the API repeats pages; add a sanity cap by tracking iterations
and/or total records and aborting when exceeded (e.g., introduce maxPages and/or
maxTotalRecords constants), check before each loop iteration and return a clear
error if the cap is hit; update the loop to increment an iterations counter
alongside offset/retLen and ensure allArtistMeta length is included in the cap
check so you break/return an error when maxPages or maxTotalRecords is exceeded
to prevent infinite loop / OOM.
- Around line 69-80: Rename the exported variable QualityMap to unexported
qualityMap to prevent external use of the deprecated API; update all internal
references to QualityMap within the package to use qualityMap, keep or move the
deprecation comment to the new name, and ensure external code uses
QueryQuality() / qualityPatterns instead of the now-unexported qualityMap.

In `@internal/api/url_parser.go`:
- Around line 79-93: ParseTimestamps currently constructs new errors with
errors.New("..."+err.Error()) which drops the original parse error; update the
error returns in ParseTimestamps to wrap the underlying parse error using
fmt.Errorf with the %w verb (e.g., fmt.Errorf("failed to parse start timestamp:
%w", err)) so callers can use errors.Is / errors.As, and add the "fmt" import to
the file.

In `@internal/cache/cache.go`:
- Around line 29-74: ReadCacheMeta and ReadCatalogCache perform file reads
without using the WithCacheLock helper, which violates the project's guideline
to use WithCacheLock for all catalog cache operations; update both functions
(ReadCacheMeta and ReadCatalogCache) to wrap their body in a call to
WithCacheLock so they acquire the cache lock before calling GetCacheDir, reading
the file (catalog-meta.json or catalog.json), unmarshalling, and returning the
result, while preserving the existing error handling and return values.

In `@internal/cache/doc.go`:
- Around line 1-2: Update the stale package comment in doc.go to a concise,
accurate package-level description that reflects the implemented code (cache.go,
artist_cache.go, and filelock_*.go); mention the package purpose (e.g.,
in-memory and on-disk caching, artist lookup cache, and cross-process file
locking) and briefly document the main exported types/functions provided by the
package so they satisfy Go's package comment guideline and help readers discover
symbols in tools like godoc.

In `@internal/cache/filelock_windows.go`:
- Around line 60-68: The Release method on FileLock currently discards the
os.Remove error; update FileLock.Release to check the result of
os.Remove(fl.path) and log a warning if it fails (while still returning the
Close() error as before). Locate the FileLock.Release function and after setting
fl.lockFile = nil, call os.Remove and if it returns non-nil, emit a warning
(using the package’s logger or log.Printf) that includes fl.path and the error;
do not change the returned err from lockFile.Close().

In `@internal/catalog/autorefresh.go`:
- Around line 115-117: Replace the raw fmt.Println/fmt.Printf calls in
EnableAutoRefresh and DisableAutoRefresh with the same UI helpers used by
ConfigureAutoRefresh: call ui.PrintSuccess with the checkmark message and use
ui.PrintKeyValue for the "Time"/"Interval" lines (matching the formatting used
in ConfigureAutoRefresh); update EnableAutoRefresh and DisableAutoRefresh to
import/use ui.PrintSuccess and ui.PrintKeyValue so terminal colors/symbols
remain consistent with ConfigureAutoRefresh.

In `@internal/catalog/deps.go`:
- Line 28: Rename the Playlist function parameter plistId to plistID to follow
Go's convention of capitalizing acronyms; update the function signature
(Playlist) and all its usages/call sites and any related references (e.g., in
ResolveCatPlistID interactions) so the symbol name matches and compiles.

In `@internal/catalog/handlers.go`:
- Line 301: Several exported functions in this file use non-idiomatic
identifiers like artistId/artistIds; rename them to artistID/artistIDs to match
Go conventions and the rest of the codebase. Update the function signatures
(e.g., CatalogGapsForArtist(ctx context.Context, artistId string, ...)), all
other exported functions and parameters that use artistId/artistIds, and every
call site and wrapper that references those symbols so names remain consistent
across packages; run gofmt/goimports and go vet to catch remaining references
and ensure tests/compilation pass.
- Line 42: Replace the ignored error return from cache.GetCacheDir() with proper
error handling: call cacheDir, err := cache.GetCacheDir(); if err != nil then
log a warning or return the error instead of silently proceeding (use the
existing logger in handlers.go), and ensure you don't treat an empty cacheDir as
valid; apply the same fix to the second occurrence at the other call site so
both cacheDir usages handle errors consistently.

In `@internal/catalog/helpers.go`:
- Around line 157-169: BuildArtistPresenceIndex currently sets isVideo :=
mediaFilter == model.MediaTypeVideo and only calls deps.ListRemoteArtistFolders
once, so when mediaFilter is Both you only list the audio remote path; change
the logic in BuildArtistPresenceIndex to call deps.ListRemoteArtistFolders for
both isVideo=false and isVideo=true when mediaFilter is model.MediaTypeBoth (or
unknown), merge the resulting remoteFolders into idx.RemoteFolders (deduplicate
entries), and combine/propagate any errors into idx.RemoteListErr (e.g.,
aggregate or prefer the first non-nil) instead of only querying the audio path.
- Line 29: The current global sync.Once variable remoteCheckWarnOnce suppresses
all but the first remote error; change this to track and warn once per distinct
error instead: replace remoteCheckWarnOnce with a concurrent map (e.g., a
sync.Map named remoteCheckWarners) keyed by a stable error identifier (for
example err.Error() or an error code) and, where the warning is emitted, use
LoadOrStore to atomically insert a new sync.Once per key and call its Do to log
the specific error only once; update any references that call
remoteCheckWarnOnce to use the new map/loader pattern so each unique remote
error will be logged once while still preventing duplicate logs for identical
errors.
- Around line 174-197: The fallback in IsShowDownloaded currently calls
deps.RemotePathExists with isVideo hardcoded to false, which misses video-only
shows; add a media filter field to ArtistPresenceIndex (e.g., MediaFilter or
MediaType) when building the index in BuildArtistPresenceIndex and thread that
into IsShowDownloaded so the RemotePathExists call passes the correct isVideo
flag (derive isVideo from the index.MediaFilter value); update
BuildArtistPresenceIndex to set the new field and change IsShowDownloaded to use
idx.MediaFilter to compute the boolean passed to deps.RemotePathExists.

In `@internal/catalog/media_filter.go`:
- Around line 44-74: ShowExistsForMediaIndexed currently uses the
caller-supplied mediaType (which in classifyShows is the global mediaFilter) to
compute isVideo for the remote-path fallback, causing MediaTypeBoth to force
video checks; change the code so the function receives and/or uses the show's
actual resolved media type instead of the global filter: update the call site in
classifyShows (where mediaFilter is passed) to pass the show's resolved media
type (the value you compute per-show in classifyShows) and/or adjust
ShowExistsForMediaIndexed to derive isVideo from the show’s resolved media type
field (use the same resolved-type symbol you already compute in classifyShows)
so the remote fallback checks the correct audio vs video path.
- Around line 44-47: In ShowExistsForMediaIndexed, NewConfigPathResolver(cfg) is
created eagerly as resolver but only used in the remote-fallback branch; to
avoid unnecessary allocation move the helpers.NewConfigPathResolver(cfg) call
into the fallback block where resolver is referenced (the branch that handles
remote fallback between lines containing the remote path check and the call
sites using resolver), keeping albumFolder and other fast-path locals as-is and
only constructing resolver when entering the remote-fallback code path.

In `@internal/config/config.go`:
- Around line 556-558: The error message returned when data == nil omits the
local "./config.json" path; update the fmt.Errorf call that constructs the error
(the expression fmt.Errorf("config file not found in any location
(~/.nugs/config.json, ~/.config/nugs/config.json): %w", lastErr)) to include
"./config.json" in the listed search locations so the message accurately
reflects all locations the loader checked.

In `@internal/download/audio.go`:
- Around line 286-291: The code calls hex.DecodeString(key.IV[2:]) without
validating key.IV; add a guard in the function surrounding that call to verify
key.IV is non-empty and begins with the "0x" prefix and has length >= 3 before
slicing, and fall back to either decoding the whole string or returning a clear
error if the IV is malformed; update the hex.DecodeString invocation near the
current hex.DecodeString(key.IV[2:]) usage to use the validated/normalized IV
string (or return an error) so you avoid a slice bounds panic when key.IV is
missing or has an unexpected format.
- Around line 75-124: In DownloadTrack, the call to os.OpenFile uses
os.O_CREATE|os.O_WRONLY which can leave trailing bytes if a previous partial
file exists; update the flags used in the os.OpenFile call inside DownloadTrack
to include os.O_TRUNC (i.e., os.O_CREATE|os.O_WRONLY|os.O_TRUNC) so the file is
truncated when opened before writing, keeping the same file mode and existing
cleanup behavior.
- Around line 540-548: PreCalculateShowSize currently creates its timeout
context from context.Background(), so parent cancellation (e.g., SIGINT) is
ignored; change PreCalculateShowSize signature to accept a parent ctx
context.Context as its first parameter and use that ctx when calling
context.WithTimeout(...) instead of context.Background(), and update the caller
(Album) to pass through its ctx into PreCalculateShowSize so cancellation
propagates properly; ensure the cancel() call and any uses of sem, timeout,
len(tracks) remain the same.

In `@internal/download/video.go`:
- Around line 212-238: GetSegUrls currently calls api.Client.Get(manifestUrl)
without context; change the signature to accept a context.Context (func
GetSegUrls(ctx context.Context, manifestUrl, query string) ([]string, error))
and replace the api.Client.Get call with an HTTP request created with that
context (use http.NewRequestWithContext or create req and call req =
req.WithContext(ctx)) and then use api.Client.Do(req). Keep the same response
checks, defer req.Body.Close(), and all existing logic (parsing with
m3u8.DecodeFrom, type assertion to *m3u8.MediaPlaylist, and building segUrls)
but ensure you use the context-aware request/Do path instead of
api.Client.Get(manifestUrl).
- Around line 443-444: The loop pre-increments the range index (i++) making the
index 1-based and confusing; remove the in-loop i++ so i remains the original
0-based index from the range, update the call GetNextChapStart(chapters, i) to
use i+1 where the next-chapter start is needed (per the reviewer note), and
adjust any error/log messages that currently subtract 1 (i-1) to use i directly
(or i+1 as appropriate) so all index math is clear and consistent in the loop
that iterates over chapters.
- Around line 128-147: ChooseVariant currently performs the HTTP GET without a
context (api.Client.Get(manifestUrl)), so the call cannot be cancelled; change
the function signature to accept a context.Context (e.g. ChooseVariant(ctx
context.Context, manifestUrl, wantRes string)), create the request with that
context using http.NewRequestWithContext (or call api.Client.Do with
req.WithContext(ctx)), use the contextual request in place of api.Client.Get,
and ensure error handling and req.Body.Close remain unchanged; update all call
sites to pass through the caller's ctx.

In `@internal/helpers/errors.go`:
- Around line 45-64: GetScriptDir currently uses runtime.Caller(0) which returns
the helpers file location after the package move; change the call to
runtime.Caller(1) (or add an optional skip parameter) so you obtain the caller's
frame when runFromSrc is true, keep the same error handling (return
ErrScriptFilenameUnavailable when !ok) and preserve the os.Executable branch;
update the use of the fname/ok variables around the runtime.Caller invocation in
GetScriptDir accordingly.
- Line 71: Replace the double-wrap fmt.Errorf usage so the sentinel
ErrOpenTextFile is wrapped only once and the underlying error is formatted as
text: change the fmt.Errorf("%w %q: %w", ErrOpenTextFile, path, err) calls to
use a single %w for ErrOpenTextFile and use %v (or %s) for err, and apply the
same change to the other identical occurrence; look for the fmt.Errorf calls
that reference ErrOpenTextFile and the path variable to update both sites.

In `@internal/helpers/paths.go`:
- Around line 104-112: GetRcloneBasePath and GetRclonePathForMedia handle
cfg.RcloneVideoPath inconsistently (one trims whitespace, the other does not);
make them consistent by trimming cfg.RcloneVideoPath before emptiness checks in
GetRclonePathForMedia (or trim once into a local variable at the top of each
function) and use strings.TrimSpace(cfg.RcloneVideoPath) != "" wherever the code
decides whether to use the video path so that whitespace-only values are treated
the same across GetRcloneBasePath and GetRclonePathForMedia.
- Around line 82-90: The current ValidatePath rejects any occurrence of ".."
which falsely flags names like "file..v2.txt"; update ValidatePath to only
detect true path-traversal patterns (e.g., segments like "../", "..\", a leading
"../" or "..\" segment, a trailing "/.." or "\..", or a path equal to "..")
instead of any two consecutive dots. Use platform-aware separators
(filepath.Separator or both "/" and "\") when checking and verify ".." appears
as a path segment boundary (e.g., preceded or followed by a separator or
start/end of string) before returning ErrPathTraversalDetected from
ValidatePath.

In `@internal/list/deps.go`:
- Line 19: Rename the parameter plistId to plistID to follow Go acronym casing;
update the signature of Playlist(ctx context.Context, plistId, legacyToken
string, cfg *model.Config, streamParams *model.StreamParams, cat bool) error in
deps.go and any occurrences in list.go, batch.go, client.go (and any callers) to
use plistID, and adjust all references/usages (including tests) to match the new
name; follow the same convention used by ResolveCatPlistID to ensure consistency
across the codebase.

In `@internal/list/list.go`:
- Around line 143-158: emptyShowListJSON currently swallows strconv.Atoi errors
causing artistIdInt to default to 0; update the flow so artistId is validated
once at the public entry (e.g., ListArtistShowsByVenue) and propagate a
validated int into downstream helpers (change emptyShowListJSON and
renderShowsJSON to accept artistId as an int or return an error on invalid
input) rather than calling strconv.Atoi inside those helpers and ignoring its
error; ensure any conversion failure returns an error up the call chain so JSON
output never silently contains "artistID": 0.
- Around line 403-479: ListArtistShows is still ~76 lines because the table
rendering block (constructing ui.Table, iterating allContainers, printing legend
and info) should be extracted; create a new helper function
renderShowsTable(allContainers []<same-type-as-in-collectContainers-return>,
artistId string, artistName string, mf model.MediaType) that builds the
filterLabel, initializes the ui.NewTable with the same columns, adds rows by
iterating item := range allContainers (using item.Container, item.MediaType,
item.DateStr, etc.), calls table.Print(), prints the legend and ui.PrintInfo("To
download a show, use: nugs <container_id>"), and returns nothing (or error if
you prefer); then replace the table block in ListArtistShows with a single call
to renderShowsTable(allContainers, artistId, artistName, mf) so ListArtistShows
falls under 50 lines. Ensure the new helper uses the same ui symbols
(ui.ColorCyan, ui.ColorReset, ui.SymbolAudio, ui.SymbolVideo, ui.SymbolBoth) and
ui.GetMediaTypeIndicator for consistency.
- Around line 676-687: The parameter name plistId in the CatalogPlist function
is misleading because ResolveCatPlistID treats it as a URL; rename the parameter
to plistUrl in the CatalogPlist signature and all uses inside the function (and
any callers) to make intent clear, update the variable passed into
ResolveCatPlistID from plistId to plistUrl, and ensure any references to
CatalogPlist (tests, callers) are updated to the new parameter name to keep the
API consistent with ResolveCatPlistID.
- Around line 646-674: In ResolveCatPlistID, resp.Body is closed immediately
which is fragile; change it to defer resp.Body.Close() immediately after the
successful api.Client.Do(httpReq) returns (i.e., right after err == nil) so the
response body is always closed even if later code panics or grows—ensure you
place the defer before any early returns that use resp (but after checking err)
to maintain the same behavior.

In `@internal/model/constants.go`:
- Line 64: TrackStreamMetaFormatProbeOrder is declared as a package-level
mutable var so callers can mutate the array; change it to an unexported
constant-like value and provide an exported accessor function
TrackStreamMetaFormatProbeOrder() that returns a fresh copy (e.g., an array or
slice copy) so callers cannot mutate shared state, and update all call sites
from model.TrackStreamMetaFormatProbeOrder to
model.TrackStreamMetaFormatProbeOrder().

In `@internal/model/progress_box.go`:
- Around line 24-25: ProgressBoxState currently exposes its mutex as exported
field Mu which couples callers to internal locking; rename Mu to unexported mu
(ProgressBoxState.mu) and update all internal uses in methods like Locked* and
GetDisplayMessage to use the private mu, then provide a minimal public locking
API (e.g., Add exported Lock() and Unlock() methods or a WithLock(func())) so
external callers can synchronize without directly touching the mutex; update any
call sites that referenced Mu to use the new Lock/Unlock (or WithLock) helpers.

In `@Makefile`:
- Around line 5-8: The Makefile's build target hardcodes the install directory
as "~/.local/bin" which may not exist or be in PATH on all systems; change the
target to use a configurable PREFIX variable (e.g., default PREFIX ?=
$(HOME)/.local) and build/install to $(PREFIX)/bin (update the `@mkdir` and `@go`
build -o lines), and document the PREFIX usage so callers can override it (e.g.,
make PREFIX=/usr/local). This touches the Makefile commands that create the
directory and call go build (the lines creating ~/.local/bin and the go build -o
target).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Hardcoded stats will become stale.

"50 Go source files" and "648 lines" (lines 76–78) will drift as the codebase evolves. Consider replacing with a command (e.g., find internal cmd -name '*.go' | wc -l) or removing the specific numbers.

🤖 Prompt for AI Agents

In `@CLAUDE.md` around lines 75 - 78, The "**Key Stats:**" section contains
hardcoded values ("50 Go source files" and "Entry Point: `cmd/nugs/main.go` (648
lines)") that will become stale; replace those literal counts with either a
dynamic shell command example (e.g., suggest running find ... | wc -l or wc -l
on the file) or remove the specific numeric values and keep only the descriptive
labels so the README stays accurate over time; update the lines containing the
exact phrases "50 Go source files" and "cmd/nugs/main.go (648 lines)"
accordingly.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Go naming: clientId → clientID, authUrl → authURL, etc.

Several constant aliases use non-idiomatic Go naming (Id instead of ID, Url instead of URL). Since these are migration shims, this can be deferred to Phase 12 cleanup.

🤖 Prompt for AI Agents

In `@cmd/nugs/api_client.go` around lines 14 - 25, Rename the non-idiomatic
constant aliases to use Go conventions: change clientId → clientID, authUrl →
authURL, streamApiBase → streamAPIBase, subInfoUrl → subInfoURL, userInfoUrl →
userInfoURL, playerUrl → playerURL (leave devKey, layout, userAgent,
userAgentTwo as-is if appropriate) and have each new constant continue to
reference the original api.* values (e.g., clientID = api.ClientID). Update all
local references to these constants in the package to use the new names to avoid
build errors.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Import runtime shadows the Go standard library package.

Same issue as the other migration wrappers. Use an explicit alias.

Proposed fix

-import "github.com/jmagar/nugs-cli/internal/runtime"
+import nugsrt "github.com/jmagar/nugs-cli/internal/runtime"

-func cancelProcessByPID(pid int) error { return runtime.CancelProcessByPID(pid) }
+func cancelProcessByPID(pid int) error { return nugsrt.CancelProcessByPID(pid) }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	import "github.com/jmagar/nugs-cli/internal/runtime"
	func cancelProcessByPID(pid int) error { return runtime.CancelProcessByPID(pid) }
	import nugsrt "github.com/jmagar/nugs-cli/internal/runtime"
	func cancelProcessByPID(pid int) error { return nugsrt.CancelProcessByPID(pid) }

🤖 Prompt for AI Agents

In `@cmd/nugs/cancel_unix.go` around lines 8 - 10, The import name runtime shadows
the Go standard library package; change the import to an explicit alias (for
example `intruntime` or similar) and update the wrapper function to call the
aliased package's symbol, i.e. replace the import
"github.com/jmagar/nugs-cli/internal/runtime" with an aliased import and change
cancelProcessByPID to call the aliased package's CancelProcessByPID (e.g.
intruntime.CancelProcessByPID).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Import runtime shadows the Go standard library package.

Same issue as flagged on the process_alive_*.go and signal_persistence_unix.go wrappers. Use an explicit alias to avoid silently shadowing Go's stdlib runtime.

Proposed fix

-import "github.com/jmagar/nugs-cli/internal/runtime"
+import nugsrt "github.com/jmagar/nugs-cli/internal/runtime"

 func maybeDetachAndExit(_args []string, urls []string) bool {
-	return runtime.MaybeDetachAndExit(_args, urls)
+	return nugsrt.MaybeDetachAndExit(_args, urls)
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	import "github.com/jmagar/nugs-cli/internal/runtime"
	func maybeDetachAndExit(_args []string, urls []string) bool {
	return runtime.MaybeDetachAndExit(_args, urls)
	}
	import nugsrt "github.com/jmagar/nugs-cli/internal/runtime"
	func maybeDetachAndExit(_args []string, urls []string) bool {
	return nugsrt.MaybeDetachAndExit(_args, urls)
	}

🤖 Prompt for AI Agents

In `@cmd/nugs/detach_windows.go` around lines 8 - 12, The import named "runtime"
shadows the Go stdlib; update the import to use an explicit alias (e.g.,
runtimepkg or internalruntime) for "github.com/jmagar/nugs-cli/internal/runtime"
and then change the call in maybeDetachAndExit from
runtime.MaybeDetachAndExit(_args, urls) to use that alias (e.g.,
runtimepkg.MaybeDetachAndExit(_args, urls)) so the stdlib runtime is not
shadowed.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

run() at ~141 lines significantly exceeds the 50-line guideline.

This function handles early-exit commands, runtime tracking, environment setup, authentication, and post-auth command routing. Consider extracting the authentication block (Lines 203–237) and the early-exit commands (Lines 106–141) into separate helpers.

As per coding guidelines, "Keep Go functions under 50 lines where possible".

🤖 Prompt for AI Agents

In `@cmd/nugs/main.go` around lines 105 - 246, The run function is too long;
extract the early-exit command handling (the "status", "cancel", and
"completion" blocks around maybeDetachAndExit and completionCommand) into a
helper like handleEarlyCommands(cfg *Config) bool, and extract the
authentication/setup block (makeDirs, auth, getUserInfo, getSubInfo,
extractLegToken, parseStreamParams, and related planDesc logic) into a helper
like performAuthAndInit(ctx, cfg) (token string, userId string, streamParams
StreamParams, err error); update run to call these helpers, propagate errors via
handleErr as before, and ensure return values (e.g., whether an early command
handled) are used so run stays <50 lines while keeping existing behavior and
references to maybeDetachAndExit, handleListCommand, handleCatalogCommand,
handleArtistShorthand, auth, getUserInfo, getSubInfo, extractLegToken,
parseStreamParams, and makeDirs.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

resp.Body should be closed with defer for robustness.

Line 656 closes the body immediately, which works today but is fragile if the function body grows. Using defer is idiomatic and guards against future modifications or panics.

 resp, err := api.Client.Do(httpReq)
 if err != nil {
 	return "", err
 }
-resp.Body.Close()
+defer resp.Body.Close()

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// ResolveCatPlistID resolves a catalog playlist URL to its GUID.
	func ResolveCatPlistID(ctx context.Context, plistUrl string) (string, error) {
	httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, plistUrl, nil)
	if err != nil {
	return "", err
	}
	resp, err := api.Client.Do(httpReq)
	if err != nil {
	return "", err
	}
	resp.Body.Close()
	if resp.StatusCode != http.StatusOK {
	return "", errors.New(resp.Status)
	}
	location := resp.Request.URL.String()
	u, err := url.Parse(location)
	if err != nil {
	return "", err
	}
	q, err := url.ParseQuery(u.RawQuery)
	if err != nil {
	return "", err
	}
	resolvedId := q.Get("plGUID")
	if resolvedId == "" {
	return "", errors.New("not a catalog playlist")
	}
	return resolvedId, nil
	}
	// ResolveCatPlistID resolves a catalog playlist URL to its GUID.
	func ResolveCatPlistID(ctx context.Context, plistUrl string) (string, error) {
	httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, plistUrl, nil)
	if err != nil {
	return "", err
	}
	resp, err := api.Client.Do(httpReq)
	if err != nil {
	return "", err
	}
	defer resp.Body.Close()
	if resp.StatusCode != http.StatusOK {
	return "", errors.New(resp.Status)
	}
	location := resp.Request.URL.String()
	u, err := url.Parse(location)
	if err != nil {
	return "", err
	}
	q, err := url.ParseQuery(u.RawQuery)
	if err != nil {
	return "", err
	}
	resolvedId := q.Get("plGUID")
	if resolvedId == "" {
	return "", errors.New("not a catalog playlist")
	}
	return resolvedId, nil
	}

🤖 Prompt for AI Agents

In `@internal/list/list.go` around lines 646 - 674, In ResolveCatPlistID,
resp.Body is closed immediately which is fragile; change it to defer
resp.Body.Close() immediately after the successful api.Client.Do(httpReq)
returns (i.e., right after err == nil) so the response body is always closed
even if later code panics or grows—ensure you place the defer before any early
returns that use resp (but after checking err) to maintain the same behavior.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Misleading parameter name: plistId is actually a URL.

CatalogPlist passes plistId directly to ResolveCatPlistID, which uses it as an HTTP URL (Line 648: http.NewRequestWithContext(..., plistUrl, ...)). The parameter should be renamed to plistUrl for clarity and consistency with the callee's parameter name.

-func CatalogPlist(ctx context.Context, plistId, legacyToken string, cfg *model.Config, streamParams *model.StreamParams, deps *Deps) error {
-	resolvedId, err := ResolveCatPlistID(ctx, plistId)
+func CatalogPlist(ctx context.Context, plistUrl, legacyToken string, cfg *model.Config, streamParams *model.StreamParams, deps *Deps) error {
+	resolvedId, err := ResolveCatPlistID(ctx, plistUrl)

🤖 Prompt for AI Agents

In `@internal/list/list.go` around lines 676 - 687, The parameter name plistId in
the CatalogPlist function is misleading because ResolveCatPlistID treats it as a
URL; rename the parameter to plistUrl in the CatalogPlist signature and all uses
inside the function (and any callers) to make intent clear, update the variable
passed into ResolveCatPlistID from plistId to plistUrl, and ensure any
references to CatalogPlist (tests, callers) are updated to the new parameter
name to keep the API consistent with ResolveCatPlistID.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Package-level var array is mutable — consider protecting it.

TrackStreamMetaFormatProbeOrder is a var exposed at package level, so any consumer can inadvertently mutate it at runtime and affect all other callers. Since Go doesn't allow const arrays, a common pattern is to use a function that returns a copy:

Proposed approach

-var TrackStreamMetaFormatProbeOrder = [4]int{1, 4, 7, 10}
+// TrackStreamMetaFormatProbeOrder returns the format probe order.
+// Returned as a value (copy) to prevent mutation of the canonical order.
+func TrackStreamMetaFormatProbeOrder() [4]int {
+	return [4]int{1, 4, 7, 10}
+}

Note: This requires updating all call sites from model.TrackStreamMetaFormatProbeOrder to model.TrackStreamMetaFormatProbeOrder().

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	var TrackStreamMetaFormatProbeOrder = [4]int{1, 4, 7, 10}
	// TrackStreamMetaFormatProbeOrder returns the format probe order.
	// Returned as a value (copy) to prevent mutation of the canonical order.
	func TrackStreamMetaFormatProbeOrder() [4]int {
	return [4]int{1, 4, 7, 10}
	}

🤖 Prompt for AI Agents

In `@internal/model/constants.go` at line 64, TrackStreamMetaFormatProbeOrder is
declared as a package-level mutable var so callers can mutate the array; change
it to an unexported constant-like value and provide an exported accessor
function TrackStreamMetaFormatProbeOrder() that returns a fresh copy (e.g., an
array or slice copy) so callers cannot mutate shared state, and update all call
sites from model.TrackStreamMetaFormatProbeOrder to
model.TrackStreamMetaFormatProbeOrder().

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Exported mutex field Mu couples internal synchronization to external callers.

ProgressBoxState.Mu is exported, meaning any consumer can (and must) lock/unlock it directly for *Locked methods and GetDisplayMessage. This works but tightly couples callers to the locking strategy. If this is intentional for performance (avoiding double-locking in hot render paths), consider documenting that Mu is part of the public API contract on the struct's doc comment.

🤖 Prompt for AI Agents

In `@internal/model/progress_box.go` around lines 24 - 25, ProgressBoxState
currently exposes its mutex as exported field Mu which couples callers to
internal locking; rename Mu to unexported mu (ProgressBoxState.mu) and update
all internal uses in methods like Locked* and GetDisplayMessage to use the
private mu, then provide a minimal public locking API (e.g., Add exported Lock()
and Unlock() methods or a WithLock(func())) so external callers can synchronize
without directly touching the mutex; update any call sites that referenced Mu to
use the new Lock/Unlock (or WithLock) helpers.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Build target hardcodes ~/.local/bin — may not be in PATH on all systems.

The build output path assumes ~/.local/bin exists and is in $PATH. This is common on many Linux distros but not universal (e.g., macOS, minimal containers). Consider documenting this assumption or allowing an override via a PREFIX variable.

Suggested improvement

+PREFIX ?= $(HOME)/.local/bin
+
 build:
-	`@mkdir` -p ~/.local/bin
+	`@mkdir` -p $(PREFIX)
 	`@echo` "Building nugs..."
-	`@go` build -o ~/.local/bin/nugs ./cmd/nugs
-	`@echo` "done: ~/.local/bin/nugs"
+	`@go` build -o $(PREFIX)/nugs ./cmd/nugs
+	`@echo` "done: $(PREFIX)/nugs"

🤖 Prompt for AI Agents

In `@Makefile` around lines 5 - 8, The Makefile's build target hardcodes the
install directory as "~/.local/bin" which may not exist or be in PATH on all
systems; change the target to use a configurable PREFIX variable (e.g., default
PREFIX ?= $(HOME)/.local) and build/install to $(PREFIX)/bin (update the `@mkdir`
and `@go` build -o lines), and document the PREFIX usage so callers can override
it (e.g., make PREFIX=/usr/local). This touches the Makefile commands that
create the directory and call go build (the lines creating ~/.local/bin and the
go build -o target).

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Inconsistent artistId vs artistID naming.

Line 26 uses artistID (correct Go convention for initialisms), but lines 46, 50, 54, 66 use artistId. The PR objectives explicitly mention renaming ResolveCatPlistId → ResolveCatPlistID to follow Go conventions — apply the same rule here.

♻️ Proposed fix

-func catalogGapsForArtist(ctx context.Context, artistId string, cfg *Config, jsonLevel string, idsOnly bool, mediaFilter MediaType) error {
-	return catalog.CatalogGapsForArtist(ctx, artistId, cfg, jsonLevel, idsOnly, mediaFilter, buildCatalogDeps())
+func catalogGapsForArtist(ctx context.Context, artistID string, cfg *Config, jsonLevel string, idsOnly bool, mediaFilter MediaType) error {
+	return catalog.CatalogGapsForArtist(ctx, artistID, cfg, jsonLevel, idsOnly, mediaFilter, buildCatalogDeps())
 }

-func catalogGaps(ctx context.Context, artistIds []string, cfg *Config, jsonLevel string, idsOnly bool, mediaFilter MediaType) error {
-	return catalog.CatalogGaps(ctx, artistIds, cfg, jsonLevel, idsOnly, mediaFilter, buildCatalogDeps())
+func catalogGaps(ctx context.Context, artistIDs []string, cfg *Config, jsonLevel string, idsOnly bool, mediaFilter MediaType) error {
+	return catalog.CatalogGaps(ctx, artistIDs, cfg, jsonLevel, idsOnly, mediaFilter, buildCatalogDeps())
 }

-func catalogGapsFill(ctx context.Context, artistId string, cfg *Config, streamParams *StreamParams, jsonLevel string, mediaFilter MediaType) error {
-	return catalog.CatalogGapsFill(ctx, artistId, cfg, streamParams, jsonLevel, mediaFilter, buildCatalogDeps())
+func catalogGapsFill(ctx context.Context, artistID string, cfg *Config, streamParams *StreamParams, jsonLevel string, mediaFilter MediaType) error {
+	return catalog.CatalogGapsFill(ctx, artistID, cfg, streamParams, jsonLevel, mediaFilter, buildCatalogDeps())
 }

Same applies to catalogCoverage (line 58), catalogList (line 62), and catalogListForArtist (line 66).

🤖 Prompt for AI Agents

In `@cmd/nugs/catalog_handlers.go` around lines 46 - 67, Rename the inconsistent
parameter name artistId to artistID in the catalog handler functions to follow
Go initialism convention: update function signatures and all uses inside
catalogGapsForArtist, catalogGapsFill, catalogCoverage, catalogList, and
catalogListForArtist (and catalogGaps if applicable) so the parameter and any
forwarded call to catalog.CatalogGapsForArtist, catalog.CatalogGapsFill,
catalog.CatalogCoverage, catalog.CatalogList, and catalog.CatalogListForArtist
use artistID consistently; ensure buildCatalogDeps() calls and variable
forwarding remain unchanged and run tests/build to verify no remaining
references to artistId.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if QualityMap is referenced anywhere outside client.go
rg -n 'QualityMap' --type=go -g '!internal/api/client.go'

Repository: jmagar/nugs-cli

Length of output: 116

---

🏁 Script executed:

#!/bin/bash
# Also check if it's used internally within client.go itself
rg -n 'QualityMap' --type=go internal/api/client.go

Repository: jmagar/nugs-cli

Length of output: 161

---

🏁 Script executed:

#!/bin/bash
# Look for qualityPatterns to understand the replacement
rg -n 'qualityPatterns' --type=go -A 3 -B 1

Repository: jmagar/nugs-cli

Length of output: 1384

---

🏁 Script executed:

#!/bin/bash
# Check the usage of qualityMap in cmd/nugs/api_client.go
rg -n 'qualityMap' --type=go cmd/nugs/api_client.go -A 5 -B 2

Repository: jmagar/nugs-cli

Length of output: 292

---

🏁 Script executed:

#!/bin/bash
# Also verify if qualityMap is actually used for anything in that file
rg -n 'qualityMap' --type=go cmd/nugs/

Repository: jmagar/nugs-cli

Length of output: 116

---

🏁 Script executed:

#!/bin/bash
# Check if the qualityMap variable in cmd/nugs/api_client.go is actually used
rg -n 'qualityMap' --type=go cmd/nugs/api_client.go | head -1
# Then search for any usage beyond the assignment
cat cmd/nugs/api_client.go | wc -l

Repository: jmagar/nugs-cli

Length of output: 97

---

🏁 Script executed:

#!/bin/bash
# Get the full context of cmd/nugs/api_client.go to see if qualityMap is used
head -100 cmd/nugs/api_client.go

Repository: jmagar/nugs-cli

Length of output: 3476

---

🏁 Script executed:

#!/bin/bash
# Search for any actual usage of qualityMap (the local variable) in the entire codebase
rg -n 'qualityMap' --type=go -g '!internal/api/client.go'

Repository: jmagar/nugs-cli

Length of output: 116

---

🏁 Script executed:

#!/bin/bash
# Verify that QueryQuality is the actual API being used, not QualityMap
rg -n 'QueryQuality|qualityPatterns' --type=go internal/api/client.go

Repository: jmagar/nugs-cli

Length of output: 411

---

Unexport QualityMap to prevent new usage of deprecated API.

QualityMap is exported and marked deprecated in favor of qualityPatterns, but being exported allows external callers to reference it. The only external usage is dead code (cmd/nugs/api_client.go:32), so unexport it (qualityMap) to enforce the migration to QueryQuality(), which uses qualityPatterns internally for deterministic iteration order.

🤖 Prompt for AI Agents

In `@internal/api/client.go` around lines 69 - 80, Rename the exported variable
QualityMap to unexported qualityMap to prevent external use of the deprecated
API; update all internal references to QualityMap within the package to use
qualityMap, keep or move the deprecation comment to the new name, and ensure
external code uses QueryQuality() / qualityPatterns instead of the
now-unexported qualityMap.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Pagination loop in getArtistMetaByAvailType lacks an upper-bound safeguard.

The loop relies solely on the API returning an empty Containers slice to terminate. A misbehaving API (e.g., always returning the same page) would cause an infinite loop / OOM. Add a maximum iteration or total-record cap as a safety net.

🛡️ Proposed safeguard

 func getArtistMetaByAvailType(ctx context.Context, artistId string, availType int) ([]*model.ArtistMeta, error) {
 	var allArtistMeta []*model.ArtistMeta
 	offset := 1
+	const maxPages = 500 // safety cap
 	query := url.Values{}
 	...
-	for {
+	for page := 0; page < maxPages; page++ {
 		...
 	}
+	if len(allArtistMeta) > 0 {
+		// reached maxPages — log a warning if needed
+	}
 	return allArtistMeta, nil
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	func getArtistMetaByAvailType(ctx context.Context, artistId string, availType int) ([]*model.ArtistMeta, error) {
	var allArtistMeta []*model.ArtistMeta
	offset := 1
	query := url.Values{}
	query.Set("method", "catalog.containersAll")
	query.Set("limit", "100")
	query.Set("artistList", artistId)
	query.Set("availType", strconv.Itoa(availType))
	query.Set("vdisp", "1")
	for {
	req, err := http.NewRequestWithContext(ctx, http.MethodGet, StreamAPIBase+"api.aspx", nil)
	if err != nil {
	return nil, err
	}
	query.Set("startOffset", strconv.Itoa(offset))
	req.URL.RawQuery = query.Encode()
	req.Header.Add("User-Agent", UserAgent)
	do, err := Client.Do(req)
	if err != nil {
	return nil, err
	}
	if do.StatusCode != http.StatusOK {
	do.Body.Close()
	return nil, fmt.Errorf("API getArtistMetaByAvailType failed: %s", do.Status)
	}
	var obj model.ArtistMeta
	err = json.NewDecoder(do.Body).Decode(&obj)
	do.Body.Close()
	if err != nil {
	return nil, err
	}
	retLen := len(obj.Response.Containers)
	if retLen == 0 {
	break
	}
	allArtistMeta = append(allArtistMeta, &obj)
	offset += retLen
	}
	return allArtistMeta, nil
	}
	func getArtistMetaByAvailType(ctx context.Context, artistId string, availType int) ([]*model.ArtistMeta, error) {
	var allArtistMeta []*model.ArtistMeta
	offset := 1
	const maxPages = 500 // safety cap
	query := url.Values{}
	query.Set("method", "catalog.containersAll")
	query.Set("limit", "100")
	query.Set("artistList", artistId)
	query.Set("availType", strconv.Itoa(availType))
	query.Set("vdisp", "1")
	for page := 0; page < maxPages; page++ {
	req, err := http.NewRequestWithContext(ctx, http.MethodGet, StreamAPIBase+"api.aspx", nil)
	if err != nil {
	return nil, err
	}
	query.Set("startOffset", strconv.Itoa(offset))
	req.URL.RawQuery = query.Encode()
	req.Header.Add("User-Agent", UserAgent)
	do, err := Client.Do(req)
	if err != nil {
	return nil, err
	}
	if do.StatusCode != http.StatusOK {
	do.Body.Close()
	return nil, fmt.Errorf("API getArtistMetaByAvailType failed: %s", do.Status)
	}
	var obj model.ArtistMeta
	err = json.NewDecoder(do.Body).Decode(&obj)
	do.Body.Close()
	if err != nil {
	return nil, err
	}
	retLen := len(obj.Response.Containers)
	if retLen == 0 {
	break
	}
	allArtistMeta = append(allArtistMeta, &obj)
	offset += retLen
	}
	if len(allArtistMeta) > 0 {
	// reached maxPages — log a warning if needed
	}
	return allArtistMeta, nil
	}

🤖 Prompt for AI Agents

In `@internal/api/client.go` around lines 283 - 322, The pagination loop in
getArtistMetaByAvailType can spin indefinitely if the API repeats pages; add a
sanity cap by tracking iterations and/or total records and aborting when
exceeded (e.g., introduce maxPages and/or maxTotalRecords constants), check
before each loop iteration and return a clear error if the cap is hit; update
the loop to increment an iterations counter alongside offset/retLen and ensure
allArtistMeta length is included in the cap check so you break/return an error
when maxPages or maxTotalRecords is exceeded to prevent infinite loop / OOM.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

ChooseVariant fetches the manifest without context.Context, breaking cancellation.

Line 132 uses api.Client.Get(manifestUrl) which creates a request without context. This means cancelling the parent context won't abort this HTTP call. The function should accept ctx and use http.NewRequestWithContext.

🐛 Proposed fix

-func ChooseVariant(manifestUrl, wantRes string) (*m3u8.Variant, string, error) {
+func ChooseVariant(ctx context.Context, manifestUrl, wantRes string) (*m3u8.Variant, string, error) {
 	origWantRes := wantRes
 	var wantVariant *m3u8.Variant
-	req, err := api.Client.Get(manifestUrl)
+	httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, manifestUrl, nil)
+	if err != nil {
+		return nil, "", err
+	}
+	req, err := api.Client.Do(httpReq)
 	if err != nil {
 		return nil, "", err
 	}

🤖 Prompt for AI Agents

In `@internal/download/video.go` around lines 128 - 147, ChooseVariant currently
performs the HTTP GET without a context (api.Client.Get(manifestUrl)), so the
call cannot be cancelled; change the function signature to accept a
context.Context (e.g. ChooseVariant(ctx context.Context, manifestUrl, wantRes
string)), create the request with that context using http.NewRequestWithContext
(or call api.Client.Do with req.WithContext(ctx)), use the contextual request in
place of api.Client.Get, and ensure error handling and req.Body.Close remain
unchanged; update all call sites to pass through the caller's ctx.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

GetSegUrls also fetches without context — same issue as ChooseVariant.

Line 215 uses api.Client.Get(manifestUrl) without context propagation.

🐛 Proposed fix

-func GetSegUrls(manifestUrl, query string) ([]string, error) {
+func GetSegUrls(ctx context.Context, manifestUrl, query string) ([]string, error) {
 	var segUrls []string
-	req, err := api.Client.Get(manifestUrl)
+	httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, manifestUrl, nil)
+	if err != nil {
+		return nil, err
+	}
+	req, err := api.Client.Do(httpReq)
 	if err != nil {
 		return nil, err
 	}

🤖 Prompt for AI Agents

In `@internal/download/video.go` around lines 212 - 238, GetSegUrls currently
calls api.Client.Get(manifestUrl) without context; change the signature to
accept a context.Context (func GetSegUrls(ctx context.Context, manifestUrl,
query string) ([]string, error)) and replace the api.Client.Get call with an
HTTP request created with that context (use http.NewRequestWithContext or create
req and call req = req.WithContext(ctx)) and then use api.Client.Do(req). Keep
the same response checks, defer req.Body.Close(), and all existing logic
(parsing with m3u8.DecodeFrom, type assertion to *m3u8.MediaPlaylist, and
building segUrls) but ensure you use the context-aware request/Do path instead
of api.Client.Get(manifestUrl).

[cubic-dev-ai]

2 issues found across 26 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/download/audio.go">

<violation number="1" location="internal/download/audio.go:536">
P2: PreCalculateShowSize now creates a background context instead of accepting one from the caller, which breaks the project’s required context propagation and prevents cancellation/timeouts from upstream. Pass a context into this function (or thread the caller’s ctx through) rather than creating context.Background() inside.</violation>
</file>

<file name="internal/download/video.go">

<violation number="1" location="internal/download/video.go:687">
P1: Guard against empty segment lists before indexing segUrls[0]; an empty manifest will panic here.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P1: Guard against empty segment lists before indexing segUrls[0]; an empty manifest will panic here.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/download/video.go, line 687:

<comment>Guard against empty segment lists before indexing segUrls[0]; an empty manifest will panic here.</comment>

<file context>
@@ -732,43 +683,42 @@ func Video(ctx context.Context, videoID, uguID string, cfg *model.Config, stream
-		ui.PrintError("Failed to download video segments")
-		return err
 	}
+	return DownloadVideoFile(ctx, vidPathTs, manBaseUrl+segUrls[0], func(downloaded, total, speed int64) {
+		if progressBox == nil {
+			return
</file context>

Suggested change

	return DownloadVideoFile(ctx, vidPathTs, manBaseUrl+segUrls[0], func(downloaded, total, speed int64) {
	if len(segUrls) == 0 {
	return errors.New("no video segments returned from manifest")
	}
	return DownloadVideoFile(ctx, vidPathTs, manBaseUrl+segUrls[0], func(downloaded, total, speed int64) {

[cubic-dev-ai]

P2: PreCalculateShowSize now creates a background context instead of accepting one from the caller, which breaks the project’s required context propagation and prevents cancellation/timeouts from upstream. Pass a context into this function (or thread the caller’s ctx through) rather than creating context.Background() inside.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/download/audio.go, line 536:

<comment>PreCalculateShowSize now creates a background context instead of accepting one from the caller, which breaks the project’s required context propagation and prevents cancellation/timeouts from upstream. Pass a context into this function (or thread the caller’s ctx through) rather than creating context.Background() inside.</comment>

<file context>
@@ -540,13 +532,8 @@ func PreCalculateShowSize(tracks []model.Track, streamParams *model.StreamParams
-	ctx, cancel := context.WithTimeout(context.Background(), timeout)
-	defer cancel()
+	// Use background context for orchestration; per-request timeouts handle individual HEADs
+	ctx := context.Background()
 
 	for _, track := range tracks {
</file context>