feat(lists): add rank reordering for user lists#2495
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a drag-and-drop reordering feature for custom user lists, providing a more intuitive way for users to organize their content. The changes include a new drawer component, updated API communication for persisting rank updates, and necessary UI integrations within the existing list actions menu. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene Coverage  |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Jun 5, 2026 10:45a.m. | Review ↗ |
| Code coverage | |
Jun 5, 2026 10:45a.m. | Review ↗ |
Code Coverage Summary
| Language | Line Coverage (Overall) |
|---|---|
| Aggregate | 67.2% |
| Javascript | 67.2% |
➟ Additional coverage metrics may have been reported. See full coverage report ↗
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a drag-and-drop list reordering feature, adding a new ListReorderDrawer component, supporting API requests, Svelte utilities, and translation keys. Feedback on these changes highlights several critical issues: mapListItemToReorderableItem should explicitly handle non-media item types like 'person' to prevent runtime crashes, and optional chaining should be used on source.list.user before accessing its slug. Additionally, to align with codebase conventions, RxJS observables should not be auto-subscribed directly inside $derived blocks, and handleApply needs robust error handling to gracefully manage failed reorder requests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function mapListItemToReorderableItem( | ||
| item: ListItem, | ||
| ): ReorderableListItem { | ||
| if (item.type === 'movie' || item.type === 'show') { | ||
| return { | ||
| key: item.key, | ||
| listItemId: item.id, | ||
| rank: item.rank, | ||
| title: item.entry.title, | ||
| subtitle: mediaSubtitle(item.entry), | ||
| posterUrl: mediaPosterUrl(item.entry), | ||
| }; | ||
| } | ||
|
|
||
| if (item.type === 'season') { | ||
| return { | ||
| key: item.key, | ||
| listItemId: item.id, | ||
| rank: item.rank, | ||
| title: item.entry.show.title, | ||
| subtitle: seasonLabel(item.entry.season.number), | ||
| posterUrl: item.entry.season.poster?.url.thumb ?? | ||
| mediaPosterUrl(item.entry.show), | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| key: item.key, | ||
| listItemId: item.id, | ||
| rank: item.rank, | ||
| title: item.entry.show.title, | ||
| subtitle: `${ | ||
| episodeNumberLabel({ | ||
| seasonNumber: item.entry.episode.season, | ||
| episodeNumber: item.entry.episode.number, | ||
| }) | ||
| } - ${item.entry.episode.title}`, | ||
| posterUrl: mediaPosterUrl(item.entry.show), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The mapListItemToReorderableItem function assumes that any item that is not a 'movie', 'show', or 'season' must be an 'episode'. However, Trakt custom lists can also contain other types of items, such as 'person'.
If a list contains a 'person', item.entry will represent a person (which has a name property but no show or episode properties). Accessing item.entry.show.title on a person item will throw a runtime TypeError and crash the reorder drawer.
We should explicitly check for item.type === 'episode' and provide a safe fallback for other types like 'person'.
export function mapListItemToReorderableItem(
item: ListItem,
): ReorderableListItem {
if (item.type === 'movie' || item.type === 'show') {
return {
key: item.key,
listItemId: item.id,
rank: item.rank,
title: item.entry.title,
subtitle: mediaSubtitle(item.entry),
posterUrl: mediaPosterUrl(item.entry),
};
}
if (item.type === 'season') {
return {
key: item.key,
listItemId: item.id,
rank: item.rank,
title: item.entry.show.title,
subtitle: seasonLabel(item.entry.season.number),
posterUrl: item.entry.season.poster?.url.thumb ??
mediaPosterUrl(item.entry.show),
};
}
if (item.type === 'episode') {
return {
key: item.key,
listItemId: item.id,
rank: item.rank,
title: item.entry.show.title,
subtitle: episodeNumberLabel({
seasonNumber: item.entry.episode.season,
episodeNumber: item.entry.episode.number,
}) + " - " + item.entry.episode.title,
posterUrl: mediaPosterUrl(item.entry.show),
};
}
return {
key: item.key,
listItemId: item.id,
rank: item.rank,
title: 'name' in item.entry ? item.entry.name : '',
subtitle: undefined,
posterUrl: undefined,
};
}| const rankOrderedItems = $derived(sortReorderableItems($list)); | ||
| const rankSignature = $derived(itemOrderSignature(rankOrderedItems)); | ||
| const orderedSignature = $derived(itemOrderSignature(orderedItems)); | ||
| const isLoaded = $derived(!$isLoading && !$hasNextPage); | ||
| const hasChanges = $derived(isLoaded && orderedSignature !== rankSignature); | ||
| const canApply = $derived(hasChanges && !isApplying); |
There was a problem hiding this comment.
According to the codebase convention, we should avoid using Svelte's native store auto-subscription (e.g., $list, $isLoading, $hasNextPage) directly inside $derived blocks when dealing with RxJS Observables. This keeps the query data extraction pattern unified and prevents potential reactivity or subscription issues in Svelte 5 runes mode.
Instead, we should subscribe to the RxJS observables inside an $effect block, assign the values to local $state variables, and reference those state variables in our $derived blocks.
const rankOrderedItems = $derived(sortReorderableItems(items));
const rankSignature = $derived(itemOrderSignature(rankOrderedItems));
const orderedSignature = $derived(itemOrderSignature(orderedItems));
const isLoaded = $derived(!loading && !hasNext);
const hasChanges = $derived(isLoaded && orderedSignature !== rankSignature);
const canApply = $derived(hasChanges && !isApplying);
References
- Maintain consistency with the existing codebase convention of using RxJS pipe and map for query data extraction and loading states in Svelte components, rather than using Svelte 5's native store auto-subscription directly in $derived blocks, to keep the pattern unified across the application.
| userId: assertDefined( | ||
| source.list.user.slug, | ||
| "Expected user list to have a user slug", | ||
| ), |
There was a problem hiding this comment.
If source.list.user is undefined, evaluating source.list.user.slug will throw a runtime TypeError: Cannot read properties of undefined (reading 'slug') before assertDefined can even run.
We should use optional chaining (source.list.user?.slug) to safely pass the value to assertDefined.
userId: assertDefined(
source.list.user?.slug,
"Expected user list to have a user slug",
),
| async function handleApply() { | ||
| if (!canApply) { | ||
| return; | ||
| } | ||
|
|
||
| isApplying = true; | ||
|
|
||
| try { | ||
| const result = await requestReorder(); | ||
|
|
||
| if (result) { | ||
| onClose(); | ||
| await refreshReorderedItems(); | ||
| } | ||
| } finally { | ||
| isApplying = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The handleApply function currently lacks error handling for cases where requestReorder() returns false or throws an exception (e.g., due to network failure or server error). If a failure occurs, the drawer remains open and isApplying is reset to false, but the user receives no feedback or indication of the failure.
Consider adding a try...catch block and handling the false return value to display an error message or toast notification to the user.
async function handleApply() {
if (!canApply) {
return;
}
isApplying = true;
try {
const result = await requestReorder();
if (result) {
onClose();
await refreshReorderedItems();
} else {
// TODO: Show a toast or error notification to the user
}
} catch (error) {
console.error("Failed to reorder list:", error);
// TODO: Show a toast or error notification to the user
} finally {
isApplying = false;
}
}
seferturan
force-pushed
the
feat/list-items-reorder
branch
from
618b4a2 to
22a9725
Compare
seferturan
left a comment
seferturan
left a comment
There was a problem hiding this comment.
Will do some fixups and refactors here. Biggest changes will be:
- Needs to also work for lists with more than 100 items.
- I'll add a confirmation on drawer close; given how easy it is to close the drawer, it could easily cause a loss of work.
2 participants
Summary
/users/{user_id}/lists/{list_id}/items/reorder.Notes
This focuses on custom Lists only. Watchlist and Favorites can use the same structure later once their reorder endpoints are available.
Design
Interaction Flow
Screen.Recording.2026-06-04.at.14.56.18.mov