Sync: append mode actually dedupes — stop re-adding the whole playlist (#823)

carlosjfcasero round 2: after 6fa956d6 append stopped recreating the playlist,
but every sync re-appended ALL matched tracks — every track N times. His log
shows it plainly: "added 22 new tracks to 'Disney' (skipped 0 already present)"
on a playlist that already had those 22.

Root cause: the dedupe read `{t.id for t in get_playlist_tracks(...)}` — but
JellyfinTrack only defines `ratingKey`, never `id`, so the existing-ids set was
ALWAYS empty and everything looked new. NavidromeTrack is also ratingKey-only,
so the Navidrome append had the identical bug. Plex (plexapi ratingKey) was
fine. The existing tests were green because they mocked existing tracks as
SimpleNamespace(id=...) — encoding the same wrong assumption as the code.

Fix:
- New pure planner plan_playlist_append(current, desired) in
  core/sync/playlist_edit.py (next to the reconcile planner): order-preserving,
  drops already-present ids, dedupes within desired, stringifies (Emby numeric
  vs string safe).
- Jellyfin/Emby: existing ids fetched from the canonical
  /Playlists/{id}/Items endpoint (same as reconcile — works for Jellyfin GUIDs
  and Emby numeric ids), ratingKey fallback if that request fails.
- Navidrome: dedupe on ratingKey (the attribute that actually exists).

Tests: planner (skip-present incl. the reporter's unchanged-playlist case,
desired-order, dupes-within-desired, int/str ids, empties) + the append-mode
suite rewritten to pin the REAL shapes (raw Items dicts for Jellyfin,
ratingKey objects for Navidrome) + a new fallback-path test. 524
playlist/sync/jellyfin/navidrome tests pass.

core/jellyfin_client.py

@@ -1566,20 +1566,40 @@ class JellyfinClient(MediaServerClient):
                 return self.create_playlist(playlist_name, tracks)
 
             playlist_id = existing_playlist.id
-            existing_tracks = self.get_playlist_tracks(playlist_id)
-            existing_ids = {
-                str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id
-            }
+            # #823 round 2: the old dedupe read `t.id` off get_playlist_tracks()
+            # results — but JellyfinTrack only defines `ratingKey`, so the
+            # existing-ids set was ALWAYS empty and every sync re-appended the
+            # whole matched list ("added 22 ... skipped 0" on a playlist that
+            # already had them, every track N times). Fetch the playlist's items
+            # from the canonical /Playlists/{id}/Items endpoint (the same one
+            # reconcile uses — works on Jellyfin GUIDs and Emby numeric ids) and
+            # dedupe on the raw item Id; fall back to ratingKey if that fails.
+            existing_ids = set()
+            items_resp = self._make_request(
+                f'/Playlists/{playlist_id}/Items', {'UserId': self.user_id})
+            if items_resp:
+                for item in items_resp.get('Items', []):
+                    iid = str(item.get('Id') or '')
+                    if iid:
+                        existing_ids.add(iid)
+            else:
+                existing_ids = {
+                    str(getattr(t, 'ratingKey', '') or '')
+                    for t in self.get_playlist_tracks(playlist_id)
+                } - {''}
 
-            new_track_ids = []
+            desired_ids = []
             for t in tracks:
                 tid = None
                 if hasattr(t, 'id'):
                     tid = str(t.id) if t.id else None
                 elif isinstance(t, dict):
                     tid = str(t.get('Id') or t.get('id') or '')
-                if tid and tid not in existing_ids and self._is_valid_guid(tid):
-                    new_track_ids.append(tid)
+                if tid and self._is_valid_guid(tid):
+                    desired_ids.append(tid)
+
+            from core.sync.playlist_edit import plan_playlist_append
+            new_track_ids = plan_playlist_append(existing_ids, desired_ids)
 
             if not new_track_ids:
                 logger.info(

core/navidrome_client.py

@@ -1102,12 +1102,16 @@ class NavidromeClient(MediaServerClient):
                 return self.create_playlist(playlist_name, tracks)
 
             primary = existing_playlists[0]
-            existing_tracks = self.get_playlist_tracks(primary.id)
+            # #823 round 2: the old dedupe read `t.id` — but NavidromeTrack only
+            # defines `ratingKey`, so the existing-ids set was ALWAYS empty and
+            # every sync re-appended the whole matched list (every track N
+            # times). Same bug as the Jellyfin append; dedupe on ratingKey.
             existing_ids = {
-                str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id
-            }
+                str(getattr(t, 'ratingKey', '') or '')
+                for t in self.get_playlist_tracks(primary.id)
+            } - {''}
 
-            new_track_ids = []
+            desired_ids = []
             for t in tracks:
                 tid = None
                 if hasattr(t, 'ratingKey'):
@@ -1116,8 +1120,11 @@ class NavidromeClient(MediaServerClient):
                     tid = str(t.id) if t.id else None
                 elif isinstance(t, dict):
                     tid = str(t.get('id') or '')
-                if tid and tid not in existing_ids:
-                    new_track_ids.append(tid)
+                if tid:
+                    desired_ids.append(tid)
+
+            from core.sync.playlist_edit import plan_playlist_append
+            new_track_ids = plan_playlist_append(existing_ids, desired_ids)
 
             if not new_track_ids:
                 logger.info(

core/sync/playlist_edit.py

@@ -109,6 +109,34 @@ def plan_playlist_reconcile(
     return {"add": add, "remove": remove}
 
 
+def plan_playlist_append(
+    current_ids: List[str],
+    desired_ids: List[str],
+) -> List[str]:
+    """Plan an append: which desired ids are NOT already in the playlist.
+
+    Used by ``sync_mode='append'`` (#823 round 2): the Jellyfin/Emby and
+    Navidrome appends deduped with ``{t.id for t in existing}`` — but their
+    track wrappers only define ``ratingKey``, never ``id``, so the existing-ids
+    set was ALWAYS empty and every sync re-appended the full matched list
+    (every track N times, "skipped 0 already present"). Pure planner so the
+    dedupe logic is testable; the caller fetches current ids however its
+    server API works and applies the returned adds.
+
+    Order-preserving and duplicate-safe: desired order is kept, ids already
+    present are dropped, and duplicates WITHIN desired are emitted once.
+    """
+    current_set = {str(t) for t in current_ids}
+    out: List[str] = []
+    seen = set()
+    for d in desired_ids:
+        tid = str(d)
+        if tid and tid not in current_set and tid not in seen:
+            seen.add(tid)
+            out.append(tid)
+    return out
+
+
 VALID_SYNC_MODES = ("replace", "append", "reconcile")
 
 
@@ -129,6 +157,7 @@ __all__ = [
     "plan_playlist_add",
     "remove_one_occurrence",
     "plan_playlist_reconcile",
+    "plan_playlist_append",
     "normalize_sync_mode",
     "VALID_SYNC_MODES",
 ]

tests/test_playlist_edit.py

@@ -161,3 +161,38 @@ def test_normalize_falls_back_for_unknown():
 def test_normalize_all_real_modes_pass_through():
     for m in ('replace', 'append', 'reconcile'):
         assert normalize_sync_mode(m, 'replace') == m
+
+
+# ── plan_playlist_append (#823 round 2) ──────────────────────────────────────
+# The Jellyfin/Emby + Navidrome appends deduped on `t.id`, but their track
+# wrappers only define `ratingKey` — the existing-ids set was always empty, so
+# every sync re-appended the full matched list (every track N times,
+# "skipped 0 already present"). The planner is the now-testable dedupe.
+
+from core.sync.playlist_edit import plan_playlist_append  # noqa: E402
+
+
+def test_append_skips_already_present():
+    # carlosjfcasero's case: second sync of an unchanged playlist adds NOTHING.
+    assert plan_playlist_append(['a', 'b', 'c'], ['a', 'b', 'c']) == []
+
+
+def test_append_adds_only_new_in_desired_order():
+    assert plan_playlist_append(['a', 'c'], ['a', 'b', 'c', 'd']) == ['b', 'd']
+
+
+def test_append_to_empty_playlist_adds_all():
+    assert plan_playlist_append([], ['a', 'b']) == ['a', 'b']
+
+
+def test_append_dedupes_within_desired():
+    assert plan_playlist_append(['x'], ['a', 'a', 'x', 'b', 'a']) == ['a', 'b']
+
+
+def test_append_stringifies_ids():
+    # Emby numeric ids may arrive as ints on one side and strings on the other.
+    assert plan_playlist_append([16607838], ['16607838', '999']) == ['999']
+
+
+def test_append_ignores_empty_ids():
+    assert plan_playlist_append(['a'], ['', 'b']) == ['b']

tests/test_server_playlist_append_mode.py

@@ -173,13 +173,19 @@ class TestJellyfinAppendToPlaylist:
         mock_create.assert_called_once_with("Test", new_tracks)
 
     def test_filters_out_already_present_tracks(self):
-        """Reporter's exact case for Jellyfin — only new GUIDs go in."""
+        """Reporter's exact case for Jellyfin — only new GUIDs go in.
+
+        #823 round 2: existing ids now come from the canonical
+        /Playlists/{id}/Items request ({'Items':[{'Id': ...}]}) — the old
+        get_playlist_tracks path returned JellyfinTrack objects that only
+        define `ratingKey`, so the previous `t.id` dedupe was always empty and
+        every sync re-appended everything. These mocks pin the REAL shape."""
         client = _make_jellyfin_client()
         existing_playlist = SimpleNamespace(id='pl-1')
-        existing_tracks = [
-            SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'),
-            SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000002'),
-        ]
+        items_resp = {'Items': [
+            {'Id': 'aaaaaaaa-bbbb-cccc-dddd-000000000001'},
+            {'Id': 'aaaaaaaa-bbbb-cccc-dddd-000000000002'},
+        ]}
         incoming = [
             SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'),  # present
             SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000003'),  # NEW
@@ -194,7 +200,7 @@ class TestJellyfinAppendToPlaylist:
 
         with patch.object(client, 'ensure_connection', return_value=True), \
              patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \
-             patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \
+             patch.object(client, '_make_request', return_value=items_resp), \
              patch.object(client, '_is_valid_guid', return_value=True), \
              patch('core.jellyfin_client.requests.post', side_effect=fake_post):
             result = client.append_to_playlist("Test", incoming)
@@ -206,9 +212,26 @@ class TestJellyfinAppendToPlaylist:
     def test_short_circuits_when_no_new_tracks(self):
         client = _make_jellyfin_client()
         existing_playlist = SimpleNamespace(id='pl-1')
-        existing_tracks = [SimpleNamespace(id='guid-1')]
+        items_resp = {'Items': [{'Id': 'guid-1'}]}
         with patch.object(client, 'ensure_connection', return_value=True), \
              patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \
+             patch.object(client, '_make_request', return_value=items_resp), \
+             patch.object(client, '_is_valid_guid', return_value=True), \
+             patch('core.jellyfin_client.requests.post') as mock_post:
+            result = client.append_to_playlist("Test", [SimpleNamespace(id='guid-1')])
+        assert result is True
+        mock_post.assert_not_called()
+
+    def test_falls_back_to_ratingkey_tracks_when_items_request_fails(self):
+        """When /Playlists/{id}/Items fails, dedupe falls back to
+        get_playlist_tracks — reading ratingKey (the attr that EXISTS on
+        JellyfinTrack), not the never-present `.id` of the original bug."""
+        client = _make_jellyfin_client()
+        existing_playlist = SimpleNamespace(id='pl-1')
+        existing_tracks = [SimpleNamespace(ratingKey='guid-1')]
+        with patch.object(client, 'ensure_connection', return_value=True), \
+             patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \
+             patch.object(client, '_make_request', return_value=None), \
              patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \
              patch.object(client, '_is_valid_guid', return_value=True), \
              patch('core.jellyfin_client.requests.post') as mock_post:
@@ -221,7 +244,7 @@ class TestJellyfinAppendToPlaylist:
         existing_playlist = SimpleNamespace(id='pl-1')
         with patch.object(client, 'ensure_connection', return_value=True), \
              patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \
-             patch.object(client, 'get_playlist_tracks', return_value=[]), \
+             patch.object(client, '_make_request', return_value={'Items': []}), \
              patch.object(client, '_is_valid_guid', return_value=True), \
              patch('core.jellyfin_client.requests.post',
                    return_value=SimpleNamespace(status_code=500, text='server error')):
@@ -256,9 +279,12 @@ class TestNavidromeAppendToPlaylist:
         mock_create.assert_called_once()
 
     def test_filters_out_already_present_tracks_and_calls_subsonic(self):
+        # #823 round 2: existing tracks are NavidromeTrack objects, which only
+        # define `ratingKey` — the old `t.id` dedupe was always empty. These
+        # mocks pin the REAL attribute so the dedupe is actually exercised.
         client = _make_navidrome_client()
         existing_playlists = [SimpleNamespace(id='pl-1', title='Test')]
-        existing_tracks = [SimpleNamespace(id='100'), SimpleNamespace(id='101')]
+        existing_tracks = [SimpleNamespace(ratingKey='100'), SimpleNamespace(ratingKey='101')]
         incoming = [
             SimpleNamespace(id='100'),  # present
             SimpleNamespace(id='102'),  # NEW
@@ -287,7 +313,7 @@ class TestNavidromeAppendToPlaylist:
     def test_short_circuits_when_no_new_tracks(self):
         client = _make_navidrome_client()
         existing_playlists = [SimpleNamespace(id='pl-1', title='Test')]
-        existing_tracks = [SimpleNamespace(id='100')]
+        existing_tracks = [SimpleNamespace(ratingKey='100')]
         with patch.object(client, 'ensure_connection', return_value=True), \
              patch.object(client, 'get_playlists_by_name', return_value=existing_playlists), \
              patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \