Add new pagination for watchlist, list items, watched and collection #103
Draft
simonc56 wants to merge 11 commits into
Draft
Add new pagination for watchlist, list items, watched and collection #103simonc56 wants to merge 11 commits into
simonc56 wants to merge 11 commits into
Conversation
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 39 minutes and 49 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds pagination across the client: new validate_limit helper and a thread-local pagination store; HttpClient parses and stores x-pagination headers; tv/sync/users endpoints accept page/limit and validate limits; Users aggregates paged results and maps items to domain objects; tests and fixtures updated for paginated flows. ChangesPagination Support Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
trakt/users.py (1)
193-206:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
itemsnow truncates public lists to the first page.
items,__iter__, and__len__still behave like whole-list accessors, but this property now only loadspage=1&limit=100. Any public list with more than 100 entries will be silently incomplete, andPublicListdoesn't expose a public paginated reader to fetch the rest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trakt/users.py` around lines 193 - 206, The items property currently only loads the first page (page=1, limit=100) causing PublicList to be truncated; update _load_items (and keep items, __iter__, __len__ semantics) to fetch all pages by iterating page numbers from 1 upward, calling the existing yield "lists/{self.trakt}/items?page={page}&limit={limit}" inside _load_items and accumulating results into self._items until a page returns fewer than limit items (or empty), and preserve validate_limit(limit) and _process_items usage; alternatively, expose a paginated reader method on PublicList if you prefer lazy access, but ensure callers to items, __iter__, and __len__ see the full list rather than only page 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_users.py`:
- Around line 39-44: The test uses all([...]) which returns True for empty
lists, so add explicit non-empty checks for the page results before the
membership assertions: ensure movie_page and show_page are not empty (e.g.,
assert movie_page and assert show_page or assert len(movie_page) > 0 and
len(show_page) > 0) prior to the existing all(...) checks for 'movie' and 'show'
to guard against vacuous passes; update the assertions around the variables
movie_page and show_page in tests/test_users.py accordingly.
In `@trakt/sync.py`:
- Around line 419-420: The get_watchlist function accepts a limit but doesn't
enforce the max-limit guard; call validate_limit(limit) at the start of
get_watchlist (and use its returned/validated value) before any paging logic,
and apply the same change to the other two paginated sync endpoints in this
module that accept a limit (the functions defined just after get_watchlist
around the following sync blocks) so all three use validate_limit() to
clamp/validate the provided limit.
- Around line 449-455: The URI construction in get_watchlist appends
'?sort_how=...' and then unconditionally prepends '?' again for page/limit,
producing an invalid query string; fix by consolidating query parameters instead
of appending raw strings: add sort_how into the params dict when provided (or
check if '?' already in uri and use '&' instead), then build the final query
string once using the params dict (the symbols to change are get_watchlist,
sort_how, params, and uri).
In `@trakt/users.py`:
- Around line 762-766: When constructing the TVEpisode payload, the show Trakt
id is nested under sh_data["ids"]["trakt"], so replace the current
sh_data.get("trakt") usage with a safe nested lookup (e.g., sh_data.get("ids",
{}).get("trakt")) when updating ep_data (the ep_data.update(...,
show=sh_data.get("title"), show_id=... ) call) so the show_id is preserved for
TVEpisode creation.
- Around line 279-323: get_items currently appends parsed items into the shared
cache attribute self._items causing page results to accumulate and duplicate;
instead create a fresh local list (e.g. items = []) inside get_items, append
parsed Movie/TVShow/TVSeason/TVEpisode/Person instances to that local list
(replace all self._items.append calls), do not modify self._items, and
yield/return the local list so each get_items(page=...) call returns only that
page's items; keep references to get_items and the _items attribute to locate
the changes.
---
Outside diff comments:
In `@trakt/users.py`:
- Around line 193-206: The items property currently only loads the first page
(page=1, limit=100) causing PublicList to be truncated; update _load_items (and
keep items, __iter__, __len__ semantics) to fetch all pages by iterating page
numbers from 1 upward, calling the existing yield
"lists/{self.trakt}/items?page={page}&limit={limit}" inside _load_items and
accumulating results into self._items until a page returns fewer than limit
items (or empty), and preserve validate_limit(limit) and _process_items usage;
alternatively, expose a paginated reader method on PublicList if you prefer lazy
access, but ensure callers to items, __iter__, and __len__ see the full list
rather than only page 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3c1feeb-2608-4af1-a8c9-f12990626087
📒 Files selected for processing (8)
tests/mock_data/lists.jsontests/mock_data/sync.jsontests/mock_data/users.jsontests/test_users.pytrakt/sync.pytrakt/tv.pytrakt/users.pytrakt/utils.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
trakt/sync.py (1)
445-453:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
sort_howwhensort_byis not provided.Right now
sort_howis ignored unlesssort_byis set. That silently drops caller input instead of surfacing invalid usage.💡 Suggested fix
+ if sort_how is not None and sort_by is None: + raise ValueError("sort_how requires sort_by") + if sort_by is not None: if sort_by not in WATCHLIST_SORT_BY: raise ValueError('sort_by must be one of {}'.format(WATCHLIST_SORT_BY)) uri += '/{}'.format(sort_by)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trakt/sync.py` around lines 445 - 453, Ensure callers cannot pass sort_how without sort_by: add an explicit validation that raises a ValueError when sort_how is not None but sort_by is None (before building uri), then keep the existing checks that validate sort_by against WATCHLIST_SORT_BY and sort_how being "asc"/"desc" and append them to uri as before; reference the sort_by and sort_how variables and WATCHLIST_SORT_BY constant in the function that builds the uri so invalid usage is surfaced instead of silently ignored.tests/mock_data/users.json (1)
736-767:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd paginated fixture keys for watchlist seasons/episodes to match new request shape.
On Line 736 and Line 752, only unpaginated keys are present.
User.watchlist()now always calls.../watchlist/{media_type}?page=...&limit=..., so seasons/episodes fixtures should include paginated keys too.Suggested fixture additions
+ "users/sean/watchlist/seasons?page=1&limit=100": { + "GET": [ + ... + ] + }, + "users/sean/watchlist/episodes?page=1&limit=100": { + "GET": [ + ... + ] + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mock_data/users.json` around lines 736 - 767, The fixtures for watchlist seasons/episodes only include unpaginated keys ("users/sean/watchlist/seasons" and "users/sean/watchlist/episodes"); add matching paginated keys that User.watchlist() now requests (e.g. "users/sean/watchlist/seasons?page=1&limit=10" and "users/sean/watchlist/episodes?page=1&limit=10") with the same payload arrays so the tests see responses for paged requests.
♻️ Duplicate comments (1)
trakt/users.py (1)
292-338:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_items(page=...)still returns accumulated cache instead of page-scoped results.On Line 292,
get_itemsacceptspage, but from Line 313 onward it appends intoself._itemsand yields that shared list. Callingget_items(page=2)after page 1 returns combined data, and repeated calls can duplicate entries.Suggested fix
`@get` def get_items(self, page=1, limit=100): @@ - if not data: - yield self._items + if not data: + yield [] + items = [] for item in data: @@ - self._items.append( + items.append( Movie( item_data["title"], item_data["year"], item_data["ids"]["slug"] ) ) elif item_type == "show": - self._items.append(TVShow(item_data["title"], item_data["ids"]["slug"])) + items.append(TVShow(item_data["title"], item_data["ids"]["slug"])) @@ - self._items.append(season) + items.append(season) @@ - self._items.append(episode) + items.append(episode) elif item_type == "person": - self._items.append(Person(item_data["name"], item_data["ids"]["slug"])) + items.append(Person(item_data["name"], item_data["ids"]["slug"])) - yield self._items + yield items🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trakt/users.py` around lines 292 - 338, get_items is appending page results into the shared cache self._items which causes pages to accumulate and duplicate; change the implementation to collect results in a local list (e.g. items = []) instead of appending to self._items, populate that local list for each item_type branch (Movie, TVShow, TVSeason, TVEpisode, Person) and yield that local list at the end; do not mutate self._items when building page-scoped results (you can keep the existing early branch that yields self._items when no data is returned).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_users.py`:
- Around line 63-70: The test uses an ambiguous single-letter variable name `l`
which triggers Ruff E741; rename the variable to a descriptive identifier (e.g.,
`user_list` or `retrieved_list`) in both places where it's assigned from
UserList.get(...) and sean.get_list(...), and update all subsequent references
(the len(list(...)) call and the getattr checks) to use that new name; this
affects the occurrences tied to the UserList.get and sean.get_list results.
In `@trakt/api.py`:
- Around line 139-141: Clear the stale pagination metadata before potential HTTP
errors by resetting _pagination_store (call its clear/reset method or set to
None) immediately before calling self.raise_if_needed(response), then after
raise_if_needed succeeds call
_pagination_store.set(self._parse_pagination_headers(response.headers)) and
return self.decode_response(response); reference _pagination_store,
self.raise_if_needed, _parse_pagination_headers, and decode_response so you
update the pagination state only on successful responses.
---
Outside diff comments:
In `@tests/mock_data/users.json`:
- Around line 736-767: The fixtures for watchlist seasons/episodes only include
unpaginated keys ("users/sean/watchlist/seasons" and
"users/sean/watchlist/episodes"); add matching paginated keys that
User.watchlist() now requests (e.g.
"users/sean/watchlist/seasons?page=1&limit=10" and
"users/sean/watchlist/episodes?page=1&limit=10") with the same payload arrays so
the tests see responses for paged requests.
In `@trakt/sync.py`:
- Around line 445-453: Ensure callers cannot pass sort_how without sort_by: add
an explicit validation that raises a ValueError when sort_how is not None but
sort_by is None (before building uri), then keep the existing checks that
validate sort_by against WATCHLIST_SORT_BY and sort_how being "asc"/"desc" and
append them to uri as before; reference the sort_by and sort_how variables and
WATCHLIST_SORT_BY constant in the function that builds the uri so invalid usage
is surfaced instead of silently ignored.
---
Duplicate comments:
In `@trakt/users.py`:
- Around line 292-338: get_items is appending page results into the shared cache
self._items which causes pages to accumulate and duplicate; change the
implementation to collect results in a local list (e.g. items = []) instead of
appending to self._items, populate that local list for each item_type branch
(Movie, TVShow, TVSeason, TVEpisode, Person) and yield that local list at the
end; do not mutate self._items when building page-scoped results (you can keep
the existing early branch that yields self._items when no data is returned).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a36b8c07-0066-45d2-98bc-0bc40cce772d
📒 Files selected for processing (12)
tests/conftest.pytests/mock_data/lists.jsontests/mock_data/sync.jsontests/mock_data/users.jsontests/test_sync.pytests/test_users.pytrakt/_pagination.pytrakt/api.pytrakt/core.pytrakt/sync.pytrakt/users.pytrakt/utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
trakt/users.py (2)
209-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing early return after yielding empty result.
When
datais falsy, the code yields[]but execution continues to line 211, which attempts to processdataanyway. IfdataisNone,_process_itemswill fail; ifdatais[], you get a redundant second yield.Suggested fix
if not data: yield [] + return yield list(self._process_items(data))Or alternatively use an
elseclause:if not data: yield [] + else: - yield list(self._process_items(data)) + yield list(self._process_items(data))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trakt/users.py` around lines 209 - 211, The generator yields an empty list when data is falsy but then continues and yields again by calling self._process_items(data), causing errors or duplicate yields; fix by returning immediately after yielding the empty list (or use an else) so execution stops — update the block containing the falsy-data check to `if not data: yield []; return` (or `if not data: yield []; else: yield list(self._process_items(data))`) to prevent calling _process_items with None or yielding twice.
468-476:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential
NameErrorandTypeErroron edge cases.Multiple issues in this loop:
- Line 472:
season.get("episodes")returnsNoneif key is missing, causingTypeErrorwhen iterating.- Line 476:
del te, ts, full_showwill raiseNameErrorifseasonsis empty (ts never assigned) or all seasons have no episodes (te never assigned).Suggested fix
full_show = TVShow(**show_item) + te = ts = None # Initialize to avoid NameError on del for season in seasons: ts = next( s for s in full_show.seasons if s.season == season.get("number") ) - for ep in season.get("episodes"): + for ep in season.get("episodes", []): te = next(e for e in ts.episodes if e.number == ep.get("number")) ep["title"] = te.title ep.update(te.ids) del te, ts, full_showAlternatively, remove the
delstatement entirely—Python's garbage collector will handle cleanup when variables go out of scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trakt/users.py` around lines 468 - 476, The loop can raise TypeError when season.get("episodes") is None and NameError from the final del if ts/te were never assigned; update the inner loop to iterate over season.get("episodes", []) instead of season.get("episodes") and remove the cleanup statement del te, ts, full_show (or if you prefer, conditionally delete only if those names exist) so variables are not referenced when they were never set; target the for seasons loop and the variables ts, te, full_show in trakt/users.py.tests/test_users.py (1)
148-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit non-empty assertions before
all()checks.Lines 148-149 are missing the explicit non-empty assertions that were added to
test_user_collections(lines 41-42) andtest_user_watchlists(lines 124-125). Without these guards,all([...])will vacuously pass if page 1 returns empty results, masking pagination regressions.🛡️ Proposed fix to match sibling tests
movie_page = sean.watched('movies', page=1, limit=250) show_page = sean.watched('shows', page=1, limit=250) + assert movie_page + assert show_page assert all(['movie' in item for item in movie_page]) assert all(['show' in item for item in show_page])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_users.py` around lines 148 - 149, Add explicit non-empty assertions for the paged results before any all(...) checks: after obtaining movie_page = sean.watched('movies', page=1, limit=250) and show_page = sean.watched('shows', page=1, limit=250) assert the returned page results are non-empty (e.g., check movie_page['results'] and show_page['results'] or equivalent attributes) so the subsequent all(...) checks cannot vacuously pass; update the test to mirror the guards used in test_user_collections and test_user_watchlists while keeping the calls to sean.watched unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@trakt/users.py`:
- Around line 294-295: The generator currently yields an empty list when data is
falsy but doesn't stop execution (the line "if not data: yield []"), causing
subsequent code to run on None; after yielding the empty result you must
return/stop the generator to prevent further processing—update the block
containing "if not data: yield []" in trakt/users.py to yield the empty list and
then immediately return (or otherwise stop iteration) so downstream code in that
generator won't execute when data is None.
---
Outside diff comments:
In `@tests/test_users.py`:
- Around line 148-149: Add explicit non-empty assertions for the paged results
before any all(...) checks: after obtaining movie_page = sean.watched('movies',
page=1, limit=250) and show_page = sean.watched('shows', page=1, limit=250)
assert the returned page results are non-empty (e.g., check
movie_page['results'] and show_page['results'] or equivalent attributes) so the
subsequent all(...) checks cannot vacuously pass; update the test to mirror the
guards used in test_user_collections and test_user_watchlists while keeping the
calls to sean.watched unchanged.
In `@trakt/users.py`:
- Around line 209-211: The generator yields an empty list when data is falsy but
then continues and yields again by calling self._process_items(data), causing
errors or duplicate yields; fix by returning immediately after yielding the
empty list (or use an else) so execution stops — update the block containing the
falsy-data check to `if not data: yield []; return` (or `if not data: yield [];
else: yield list(self._process_items(data))`) to prevent calling _process_items
with None or yielding twice.
- Around line 468-476: The loop can raise TypeError when season.get("episodes")
is None and NameError from the final del if ts/te were never assigned; update
the inner loop to iterate over season.get("episodes", []) instead of
season.get("episodes") and remove the cleanup statement del te, ts, full_show
(or if you prefer, conditionally delete only if those names exist) so variables
are not referenced when they were never set; target the for seasons loop and the
variables ts, te, full_show in trakt/users.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe553d49-8d66-45c7-aaf4-8eeab9c2c97d
📒 Files selected for processing (7)
tests/mock_data/lists.jsontests/mock_data/users.jsontests/test_lists.pytests/test_users.pytrakt/api.pytrakt/users.pytrakt/utils.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_lists.py
- trakt/utils.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚧 Implementation of new Trakt pagination
Many endpoints need to have pagination with
pageandlimitquery parameters, added progressively in 2026 by Trakt devs.Refs :