Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add reset and retry buttons to episode cards #176

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: add reset and retry buttons to episode cards #176

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
891079a
feat: add reset/retry actions to episode cards
machetie Nov 15, 2024
2bad253
feat: add reset and retry buttons to episode cards
machetie Nov 15, 2024
8ccf9d3
feat: improve media page accessibility and loading states
machetie Nov 15, 2024
35600b1
fix: pass correct episode ID to resetItem and retryItem functions
machetie Nov 15, 2024
d928367
fix: correct class binding syntax for Lucide icons
machetie Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 113 additions & 3 deletions src/routes/[type=mediaType]/[id]/[season]/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,81 @@
<script lang="ts">
import type { PageData } from './$types';
import Header from '$lib/components/header.svelte';
import { Star } from 'lucide-svelte';
import { Star, CirclePower, RotateCcw } from 'lucide-svelte';
machetie marked this conversation as resolved.
Show resolved Hide resolved
import { formatDate } from '$lib/helpers';
import { statesName } from '$lib/constants';
import { ItemsService } from '$lib/client';
import { toast } from 'svelte-sonner';
import { invalidateAll } from '$app/navigation';

export let data: PageData;

let selectedEpisodeNumber: number | null = null;
let isResetting = false;
let isRetrying = false;

async function handleMediaAction(
action: (id: string) => Promise<any>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Specify a more precise return type instead of any.

Using any in TypeScript defeats the purpose of type checking and can lead to runtime errors. Define a specific interface or type for the expected response to enhance type safety and code reliability.

Apply this diff to specify a proper return type:

-   action: (id: string) => Promise<any>,
+   action: (id: string) => Promise<{ error?: boolean; [key: string]: any }>,

Alternatively, define a custom interface for the response:

interface ActionResponse {
  error?: boolean;
  // other relevant properties
}

And update the function signature:

-   action: (id: string) => Promise<any>,
+   action: (id: string) => Promise<ActionResponse>,
🧰 Tools
🪛 eslint

[error] 18-18: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)

successMessage: string,
errorMessage: string,
id: string,
loadingState: { set: (value: boolean) => void }
) {
loadingState.set(true);
Comment on lines +22 to +24
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Simplify the loadingState parameter for clarity

Consider simplifying the loadingState parameter in handleMediaAction by passing the setter function directly instead of an object with a set method. This enhances readability and reduces unnecessary complexity.

Apply this diff to update the function signature and usage:

-    loadingState: { set: (value: boolean) => void }
+    loadingState: (value: boolean) => void

Update the function body accordingly:

-    loadingState.set(true);
+    loadingState(true);

And similarly for setting it back to false:

-    loadingState.set(false);
+    loadingState(false);

Update the calls to handleMediaAction in resetItem:

return await handleMediaAction(
    (id) => ItemsService.resetItems({ query: { ids: id.toString() } }),
    'Media reset successfully',
    'An error occurred while resetting the media',
    id,
-   { set: (value) => (isResetting = value) }
+   (value) => { isResetting = value }
);

And in retryItem:

return await handleMediaAction(
    (id) => ItemsService.retryItems({ query: { ids: id.toString() } }),
    'Media retried successfully',
    'An error occurred while retrying the media',
    id,
-   { set: (value) => (isRetrying = value) }
+   (value) => { isRetrying = value }
);

Committable suggestion skipped: line range outside the PR's diff.

try {
const response = await action(id);

if (!response.error) {
toast.success(successMessage);
selectedEpisodeNumber = null;
invalidateAll();
} else {
toast.error(errorMessage);
}
} catch (error) {
console.error('Network or unexpected error:', error);
toast.error('Network error occurred. Please check your connection and try again.');
throw error;
Comment on lines +36 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Avoid re-throwing caught errors after handling.

Since you're already displaying a toast notification and logging the error, re-throwing it might not be necessary and could interrupt the flow.

Apply this diff to prevent re-throwing the error:

       } catch (error) {
         console.error('Network or unexpected error:', error);
         toast.error('Network error occurred. Please check your connection and try again.');
-        throw error;
       } finally {
📝 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
console.error('Network or unexpected error:', error);
toast.error('Network error occurred. Please check your connection and try again.');
throw error;
console.error('Network or unexpected error:', error);
toast.error('Network error occurred. Please check your connection and try again.');

} finally {
loadingState.set(false);
}
}

async function resetItem(id: string) {
try {
return await handleMediaAction(
(id) => ItemsService.resetItems({ query: { ids: id.toString() } }),
'Media reset successfully',
'An error occurred while resetting the media',
id,
{ set: (value) => isResetting = value }
);
} catch (error) {
console.error('Reset operation failed:', error);
}
}

async function retryItem(id: string) {
try {
return await handleMediaAction(
(id) => ItemsService.retryItems({ query: { ids: id.toString() } }),
'Media retried successfully',
'An error occurred while retrying the media',
id,
{ set: (value) => isRetrying = value }
);
} catch (error) {
console.error('Retry operation failed:', error);
}
}

function handleEpisodeClick(episodeNumber: number) {
if (selectedEpisodeNumber === episodeNumber) {
selectedEpisodeNumber = null;
} else {
selectedEpisodeNumber = episodeNumber;
}
Comment on lines +73 to +77
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Simplify the toggle logic in handleEpisodeClick.

You can streamline the logic by setting selectedEpisodeNumber to null if it's already selected, or to the new episodeNumber otherwise.

Apply this diff for a cleaner implementation:

 function handleEpisodeClick(episodeNumber: number) {
-   if (selectedEpisodeNumber === episodeNumber) {
-     selectedEpisodeNumber = null;
-   } else {
-     selectedEpisodeNumber = episodeNumber;
-   }
+   selectedEpisodeNumber = selectedEpisodeNumber === episodeNumber ? null : episodeNumber;
 }
📝 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
if (selectedEpisodeNumber === episodeNumber) {
selectedEpisodeNumber = null;
} else {
selectedEpisodeNumber = episodeNumber;
}
selectedEpisodeNumber = selectedEpisodeNumber === episodeNumber ? null : episodeNumber;

}
</script>

<svelte:head>
Expand Down Expand Up @@ -75,14 +145,29 @@

<div class="relative flex w-full cursor-pointer flex-wrap">
{#each data.details.episodes as episode}
<div class="group relative aspect-[2/1] h-fit w-full p-2 sm:w-1/2 xl:w-1/3">
<div
class="group relative aspect-[2/1] h-fit w-full p-2 sm:w-1/2 xl:w-1/3"
role="button"
tabindex="0"
on:click={() => handleEpisodeClick(episode.episode_number)}
on:keydown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
handleEpisodeClick(episode.episode_number);
}
}}
aria-label="Episode {episode.episode_number}{episode.name ? `: ${episode.name}` : ''}"
aria-pressed={selectedEpisodeNumber === episode.episode_number}
aria-expanded={selectedEpisodeNumber === episode.episode_number}
>
<div class="h-full w-full overflow-hidden rounded-lg bg-white/10 shadow-xl">
<img
alt={episode.id}
src={episode.still_path
? `https://www.themoviedb.org/t/p/w780${episode.still_path}`
: 'https://via.placeholder.com/198x228.png?text=No+thumbnail'}
class=" h-full w-full object-cover brightness-75 transition-all duration-300 ease-in-out group-hover:scale-105"
class="h-full w-full object-cover brightness-75 transition-all duration-300 ease-in-out"
class:blur-sm={selectedEpisodeNumber === episode.episode_number}
loading="lazy"
/>
<div class="absolute left-0 top-0 flex h-full w-full flex-col px-4 py-3">
Expand All @@ -91,6 +176,31 @@
>
Episode {episode.episode_number}
</div>

<!-- Action buttons - only show when episode is selected -->
{#if selectedEpisodeNumber === episode.episode_number && data.mediaItemDetails.find((x) => x.number == episode.episode_number)}
machetie marked this conversation as resolved.
Show resolved Hide resolved
<div
class="absolute inset-0 flex items-center justify-center gap-4 bg-black/40"
>
<button
class="flex items-center gap-2 rounded-md bg-destructive px-4 py-2 text-sm font-medium text-destructive-foreground hover:bg-destructive/90 disabled:opacity-50 disabled:cursor-not-allowed"
on:click|stopPropagation={() => resetItem(episode.id)}
disabled={isResetting || isRetrying}
>
<CirclePower class={`size-4 ${isResetting ? 'animate-spin' : ''}`} />
{isResetting ? 'Resetting...' : 'Reset'}
</button>
<button
class="flex items-center gap-2 rounded-md bg-primary px-4 py-2 text-sm font-medium text-primary-foreground hover:bg-primary/90 disabled:opacity-50 disabled:cursor-not-allowed"
on:click|stopPropagation={() => retryItem(episode.id)}
disabled={isResetting || isRetrying}
>
<RotateCcw class={`size-4 ${isRetrying ? 'animate-spin' : ''}`} />
{isRetrying ? 'Retrying...' : 'Retry'}
</button>
</div>
{/if}

Comment on lines +181 to +203
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Optimize repeated lookups by caching media item details

The expression data.mediaItemDetails.find((x) => x.number == episode.episode_number) is used multiple times within the loop. Consider caching these lookups to improve performance and readability.

You can create a map of mediaItemDetails before the loop:

<script lang="ts">
  // ... existing code
  const mediaItemDetailsMap = new Map<number, typeof data.mediaItemDetails[0]>(
    data.mediaItemDetails.map((item) => [item.number, item])
  );
</script>

Then, within the {#each} loop, retrieve the item efficiently:

{#each data.details.episodes as episode}
  {#if selectedEpisodeNumber === episode.episode_number && mediaItemDetailsMap.has(episode.episode_number)}
    <!-- Action buttons -->
  {/if}

  <!-- Use the cached item elsewhere -->
  {#if mediaItemDetailsMap.has(episode.episode_number)}
    <div class="mt-1 line-clamp-1 rounded-md bg-zinc-900/60 px-2 text-xs text-white sm:text-sm">
      {statesName[mediaItemDetailsMap.get(episode.episode_number)?.state ?? 'Unknown']}
    </div>
  {/if}
{/each}

This approach avoids repeated find operations, enhancing performance, especially with larger datasets.

<div class="mt-auto flex w-full justify-between">
{#if data.mediaItemDetails.find((x) => x.number == episode.episode_number)}
<div
Expand Down