Redesign light music upload behavior

[garado]

Closes #18, #3

Requirements

• Must be able to show if user is uploading a track that already exists (duplicate uploads).
  • For duplicate upload detection: check the metadata of the file being uploaded and form a (title, artist) tuple for comparisons. If no metadata, fall back to (filename, "Unknown"), the same way that LightOS does.
• For duplicate uploads: skipping them is the default, but user can choose to overwrite or allow duplicates.
• CLI must have a confirmation before doing anything. It should clearly display: number of tracks being uploaded, number of tracks being replaced/skipped.

[Copilot]

Pull request overview

Redesigns light music upload duplicate-handling so uploads skip matching tracks by default, with explicit flags to replace or allow duplicates, and centralizes the duplicate-matching logic in the API layer.

Changes:

• Change default upload behavior to skip duplicates (by title+artist from tags), with --replace for delete-and-replace and --allow-duplicates to bypass checks.
• Rename/reshape duplicate-matching option to --match-by {metadata|filename} and move matching logic into LightMusic.find_upload_matches().
• Update CLI help text and README usage examples to reflect the new behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
README.md	Updates CLI examples to document the new default “skip duplicates” behavior and new flags.
light_cli_tui/light_cli_tui/cli.py	Adds --replace, renames matching option to --match-by, updates prompting and passes new args into the API.
light_api/light_api/music.py	Introduces shared duplicate-matching helpers and changes upload logic to skip/replace based on flags.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

light_api/light_api/music.py:377

• In replace mode, this deletes matching tracks one file at a time via delete_tracks_predicate, which (a) rescans the full cached track list for every file (O(files * tracks)), and (b) can issue redundant delete requests when multiple input files map to the same existing track (since _tracks isn’t updated after deletion). You can delete the unique set of matched tracks once up front, then upload all files.

                    self.delete_tracks_predicate(lambda t: t is match)

light_api/light_api/music.py:298

• This ValueError message is likely surfaced directly to CLI users (via the Click wrapper). It currently tells them to pass match_by='filename', which is a Python API keyword and not the CLI flag name; consider mentioning the CLI form (--match-by filename) as well to reduce confusion.

                raise ValueError(
                    f"Could not read title/artist metadata from {file_path!r}, and "
                    "--replace is destructive - pass match_by='filename' to explicitly "
                    "acknowledge the weaker match instead of silently falling back."
                )

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

light_cli_tui/light_cli_tui/cli.py:285

• light music upload does not currently prompt for confirmation in two common cases: (1) default behavior when there are no duplicate matches and --overwrite is not set, and (2) when --allow-duplicates is used. The PR requirements explicitly call for a confirmation before doing anything, and the prompt should summarize how many tracks will be uploaded vs skipped/overwritten.

    files = list(songs)

    if not allow_duplicates:
        matches = light.music.find_upload_matches(files)

        if matches:
            verb = "overwrite" if overwrite else "skip"
            console.print(f"Tracks to {verb} ({len(matches)}):")
            for file_path, t in matches.items():
                console.print(f"  {file_path} -> {t.artist} — {t.title}")
            if not click.confirm("Proceed?"):
                return
        elif overwrite:
            console.print("[dim]No matching tracks found; uploading all as new.[/dim]")
            if not click.confirm("Proceed?"):
                return

light_api/light_api/music.py:353

• The skip log message is now potentially misleading because matching is by (title, artist) but the log only prints the title. Including the artist (and optionally audio_id) would make diagnosing duplicate-detection behavior much clearer (especially given #18).

                    log.info(f"Skipping {file_path!r}: matches existing track {match.title!r}")

light_api/light_api/init.py:23

• Catching all ValueError instances here will convert unexpected programming errors (including from dependencies) into user-facing ClickExceptions, which makes debugging harder. In this PR, the only intentional ValueError appears to be LightMusic.upload_tracks() argument validation, but the CLI already validates those flags. Consider reverting to RuntimeError only, or introducing a dedicated domain exception type to catch here.

        except (RuntimeError, ValueError) as e:

light_api/light_api/music.py:355

• The new duplicate-handling behavior in upload_tracks() (skip-by-default vs delete-and-replace under overwrite=True) is not covered by tests. The PR adds good unit tests for _track_identity() / find_upload_matches(), but without tests asserting the upload_tracks() filtering and deletion behavior, regressions in the destructive path are easy to miss.

        if not allow_duplicates:
            matches = self.find_upload_matches(files)

            if overwrite and matches:
                audio_ids = {t.audio_id for t in matches.values()}
                self.delete_tracks_predicate(lambda t: t.audio_id in audio_ids)

            to_upload = []
            for file_path in files:
                match = matches.get(file_path)

                if match is None or overwrite:
                    to_upload.append(file_path)
                else:
                    log.info(f"Skipping {file_path!r}: matches existing track {match.title!r}")

        files = to_upload

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

light_cli_tui/light_cli_tui/cli.py:267

• The confirmation summary currently prints "{new_count} new tracks will be added", but in --overwrite and --allow-duplicates modes the matching tracks are still uploaded. This makes the uploaded-track count incorrect and doesn’t satisfy the requirement to clearly display how many tracks will be uploaded vs skipped/overwritten.

    files = list(songs)
    matches = light.music.find_upload_matches(files)
    new_count = len(files) - len(matches)

    console.print(f"{new_count} new track{'s' if new_count != 1 else ''} will be added")

README.md:89

• The README says --allow-duplicates "Skip duplicate checking entirely", but the CLI still detects and lists duplicate matches in the confirmation output (it just uploads them anyway). Updating the wording avoids confusing users about what the command will do.

# Skip duplicate checking entirely, always upload

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

light_cli_tui/light_cli_tui/cli.py:267

• The upload/skip counts and duplicate match list are computed from the raw CLI arguments, but LightMusic.upload_tracks() later filters out non-existent paths. If the user passes a missing file, the confirmation can claim it “will be uploaded” or “will be overwritten/skipped” even though it will be skipped at runtime, which violates the requirement to clearly display what will happen. Consider filtering missing paths (and optionally printing them) before calling find_upload_matches() and before computing counts, and pass only existing paths into upload_tracks().

    files = list(songs)
    matches = light.music.find_upload_matches(files)

    skip_count = len(matches) if matches and not overwrite and not allow_duplicates else 0
    upload_count = len(files) - skip_count

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

light_cli_tui/light_cli_tui/cli.py:280

• The confirmation summary mixes “matching files” and “existing tracks” counts. In overwrite/allow-duplicates modes it prints len(matches) (number of input files with matches), but upload_tracks() actually deletes a de-duplicated set of existing tracks by audio_id, so the confirmation can overstate how many existing tracks will be overwritten. Consider computing both counts explicitly and wording the messages accordingly (e.g., skipped files vs overwritten existing tracks).

    matches = light.music.find_upload_matches(files)

    skip_count = len(matches) if matches and not overwrite and not allow_duplicates else 0
    upload_count = len(files) - skip_count

light_cli_tui/light_cli_tui/cli.py:246

• The --overwrite option help text says it overwrites “duplicate tracks”, but the behavior is specifically delete-and-replace existing matching tracks (title+artist). Updating the help string would make the CLI behavior clearer and align with the README/confirmation messaging.

This issue also appears on line 276 of the same file.

@click.option(
    "--overwrite",
    is_flag=True,
    help="Overwrite duplicate tracks.",
)

light_api/light_api/music.py:324

• _resolve_upload_plan() returns (files_to_upload, tracks_to_delete) but its docstring refers to “files to overwrite” / “files to be overwritten”. This is misleading for callers and for future maintenance; consider naming it as existing tracks being deleted (overwritten).

        """Return the subset of files to upload and the subset of files to overwrite after
        applying allow_duplicates/overwrite behavior flags.

        Returns:
            Tuple of lists. First item is list[str] of files to upload. Second item is list[LightTrack]