feat: add reset and retry buttons to episode cards #176
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the functionality of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/routes/[type=mediaType]/[id]/[season]/+page.svelte(3 hunks)
🧰 Additional context used
🪛 eslint
src/routes/[type=mediaType]/[id]/[season]/+page.svelte
[error] 24-24: 'invalidateAll' is not defined.
(no-undef)
[error] 40-40: 'invalidateAll' is not defined.
(no-undef)
[error] 122-188: A11y: visible, non-interactive elements with an on:click event must be accompanied by a keyboard event handler. Consider whether an interactive element such as or might be more appropriate. See https://svelte.dev/docs/accessibility-warnings#a11y-click-events-have-key-events for more details.(a11y-click-events-have-key-events)
(svelte/valid-compile)
[error] 122-188: A11y:
(svelte/valid-compile)
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/routes/[type=mediaType]/[id]/[season]/+page.svelte(3 hunks)
🧰 Additional context used
🪛 eslint
src/routes/[type=mediaType]/[id]/[season]/+page.svelte
[error] 18-18: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 190-190: Classes can only be applied to DOM elements, not components(invalid-class)
(svelte/valid-compile)
| let isRetrying = false; | ||
|
|
||
| async function handleMediaAction( | ||
| action: (id: string) => Promise<any>, |
There was a problem hiding this comment.
🛠️ 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)
| if (selectedEpisodeNumber === episodeNumber) { | ||
| selectedEpisodeNumber = null; | ||
| } else { | ||
| selectedEpisodeNumber = episodeNumber; | ||
| } |
There was a problem hiding this comment.
🧹 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.
| if (selectedEpisodeNumber === episodeNumber) { | |
| selectedEpisodeNumber = null; | |
| } else { | |
| selectedEpisodeNumber = episodeNumber; | |
| } | |
| selectedEpisodeNumber = selectedEpisodeNumber === episodeNumber ? null : episodeNumber; |
| console.error('Network or unexpected error:', error); | ||
| toast.error('Network error occurred. Please check your connection and try again.'); | ||
| throw error; |
There was a problem hiding this comment.
🧹 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.
| 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.'); |
| on:click|stopPropagation={() => resetItem(data.mediaID)} | ||
| disabled={isResetting || isRetrying} | ||
| > | ||
| <CirclePower class="size-4" class:animate-spin={isResetting} /> |
There was a problem hiding this comment.
Fix invalid use of class directive on components.
The class and class: directives cannot be applied directly to Svelte components like <CirclePower />. To apply classes or directives, wrap the component in a DOM element or use a spread attribute if the component supports it.
Apply this diff to fix the issue:
- <CirclePower class="size-4" class:animate-spin={isResetting} />
+ <span class="size-4" class:animate-spin={isResetting}>
+ <CirclePower />
+ </span>Repeat the same adjustment for the <RotateCcw /> component in the retry button.
📝 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.
| <CirclePower class="size-4" class:animate-spin={isResetting} /> | |
| <span class="size-4" class:animate-spin={isResetting}> | |
| <CirclePower /> | |
| </span> |
🧰 Tools
🪛 eslint
[error] 190-190: Classes can only be applied to DOM elements, not components(invalid-class)
(svelte/valid-compile)
| on:click|stopPropagation={() => resetItem(data.mediaID)} | ||
| disabled={isResetting || isRetrying} | ||
| > | ||
| <CirclePower class="size-4" class:animate-spin={isResetting} /> | ||
| {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(data.mediaID)} | ||
| disabled={isResetting || isRetrying} | ||
| > | ||
| <RotateCcw class="size-4" class:animate-spin={isRetrying} /> |
There was a problem hiding this comment.
Pass the correct episode ID to resetItem and retryItem functions.
Currently, the functions are called with data.mediaID, which refers to the entire media item. To target the specific episode, pass episode.id instead.
Apply this diff to fix the IDs being passed:
on:click|stopPropagation={() => resetItem(
- data.mediaID
+ episode.id
)} on:click|stopPropagation={() => retryItem(
- data.mediaID
+ episode.id
)}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 190-190: Classes can only be applied to DOM elements, not components(invalid-class)
(svelte/valid-compile)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/routes/[type=mediaType]/[id]/[season]/+page.svelte(3 hunks)
🧰 Additional context used
🪛 eslint
src/routes/[type=mediaType]/[id]/[season]/+page.svelte
[error] 18-18: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (1)
src/routes/[type=mediaType]/[id]/[season]/+page.svelte (1)
4-9: Imports are correctly updated
The necessary modules and components are imported appropriately, ensuring all required functionalities are available.
| {#if selectedEpisodeNumber === episode.episode_number && data.mediaItemDetails.find((x) => x.number == episode.episode_number)} | ||
| <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} | ||
|
|
There was a problem hiding this comment.
🧹 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.
| loadingState: { set: (value: boolean) => void } | ||
| ) { | ||
| loadingState.set(true); |
There was a problem hiding this comment.
🧹 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) => voidUpdate 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.
davidemarcoli
force-pushed
the
feat/episode-action-buttons
branch
from
d928367 to
0be24f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
eslint.config.js(1 hunks)src/routes/[type=mediaType]/[id]/[season]/+page.svelte(3 hunks)tailwind.config.ts(2 hunks)
🧰 Additional context used
🪛 eslint
tailwind.config.ts
[error] 3-3: 'blur' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Lint
tailwind.config.ts
[failure] 3-3:
'blur' is defined but never used
🔇 Additional comments (7)
eslint.config.js (1)
30-30: LGTM! Good practice for error handling.
The updated no-console rule appropriately allows console.warn and console.error while still warning about other console statements. This is particularly useful for the new reset/retry functionality where proper error handling is important for user feedback.
Let's verify the console usage in the related component:
✅ Verification successful
ESLint rule change aligns perfectly with actual console usage
The updated no-console rule is well-justified by the codebase's usage patterns. All console statements found are console.error calls used appropriately for error handling in:
- Network/unexpected errors in media episode operations
- Reset/retry operation failures
- Data mapping errors
This confirms that the ESLint configuration change to allow console.warn and console.error while warning about other console methods is the right approach for maintaining proper error handling practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check console usage in the episode card component
# Expected: Only console.warn and console.error should be used for error handling
# Search for console statements in the related Svelte component
rg -p "console\." "src/routes/\[type=mediaType\]/\[id\]/\[season\]/\+page.svelte"
Length of output: 185
Script:
#!/bin/bash
# Let's first find the correct path for media-related Svelte components
fd -e svelte
# Then search for console statements in all Svelte files
rg -p "console\." -g "*.svelte"
Length of output: 8675
tailwind.config.ts (1)
88-89: LGTM! Well-designed visual indicator.
The repeating striped pattern with semi-transparent red provides a subtle but clear visual indication for unavailable content, which aligns well with the PR's goal of improving visual feedback.
src/routes/[type=mediaType]/[id]/[season]/+page.svelte (5)
19-19: 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.
37-39: Avoid re-throwing the error after handling.
Since the error is already logged and a toast notification is shown to the user, re-throwing the error might not be necessary and could interrupt the flow by propagating the error further up.
Apply this diff to remove the unnecessary throw statement:
} catch (error) {
console.error('Network or unexpected error:', error);
toast.error('Network error occurred. Please check your connection and try again.');
- throw error;
} finally {
loadingState.set(false);
}76-78: Great job optimizing media item lookups.
Creating the mediaItemDetailsMap improves performance by caching media item details and avoiding repeated find operations within the loop.
148-164: Accessibility enhancements are well-implemented.
Adding keyboard event handlers, ARIA roles, and attributes improves the accessibility of the episode cards, allowing users to navigate using the keyboard and assistive technologies effectively.
193-201: Ensure components accept class prop or adjust usage.
Passing the class attribute to Svelte components like <CirclePower> and <RotateCcw> may not work as expected if these components do not forward the class prop to the underlying DOM elements. This could prevent your styling and animations from being applied.
Apply this diff to wrap the icon components with a <span> and apply the class to the wrapper:
- <CirclePower class={`size-4 ${isResetting ? 'animate-spin' : ''}`} />
+ <span class={`size-4 ${isResetting ? 'animate-spin' : ''}`}>
+ <CirclePower />
+ </span>
- <RotateCcw class={`size-4 ${isRetrying ? 'animate-spin' : ''}`} />
+ <span class={`size-4 ${isRetrying ? 'animate-spin' : ''}`}>
+ <RotateCcw />
+ </span>| @@ -1,5 +1,6 @@ | |||
| import { fontFamily } from 'tailwindcss/defaultTheme'; | |||
| import type { Config } from 'tailwindcss'; | |||
| import { blur } from 'svelte/transition'; | |||
There was a problem hiding this comment.
Remove unused import.
The blur import from svelte/transition is not used in this Tailwind configuration file. Svelte-specific imports should be in the component files where they're used, not in the Tailwind configuration.
-import { blur } from 'svelte/transition';🧰 Tools
🪛 eslint
[error] 3-3: 'blur' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Lint
[failure] 3-3:
'blur' is defined but never used
| keyframes: { | ||
| 'blur-in': { | ||
| '0%': { filter: 'blur(0px)' }, | ||
| '100%': { filter: 'blur(4px)' } | ||
| }, | ||
| 'blur-out': { | ||
| '0%': { filter: 'blur(4px)' }, | ||
| '100%': { filter: 'blur(0px)' } | ||
| } | ||
| }, | ||
| animation: { | ||
| 'blur-in': 'blur-in 0.5s forwards', | ||
| 'blur-out': 'blur-out 0.5s forwards' |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding prefers-reduced-motion support.
The blur animations are well-implemented with appropriate timing and subtle effects. However, consider respecting the user's motion preferences.
Add a media query to disable animations for users who prefer reduced motion:
animation: {
- 'blur-in': 'blur-in 0.5s forwards',
- 'blur-out': 'blur-out 0.5s forwards'
+ 'blur-in': 'blur-in 0.5s forwards @media(prefers-reduced-motion: no-preference)',
+ 'blur-out': 'blur-out 0.5s forwards @media(prefers-reduced-motion: no-preference)'
}Committable suggestion skipped: line range outside the PR's diff.
| loadingState: { set: (value: boolean) => void } | ||
| ) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the loadingState parameter for clarity.
Consider passing the setter function directly instead of an object with a set method. This will enhance readability and reduce unnecessary complexity.
Apply this diff to update the function signature and usage:
// Update the function signature
async function handleMediaAction(
action: (id: string) => Promise<any>,
successMessage: string,
errorMessage: string,
id: string,
- loadingState: { set: (value: boolean) => void }
+ loadingState: (value: boolean) => void
) {
- loadingState.set(true);
+ loadingState(true);
try {
// ...
} finally {
- 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)
);
// Similarly, 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)
);Also applies to: 52-53, 66-67
There was a problem hiding this comment.
@Gaisberg, glad you agree with the suggestion! Let me know if you need any further assistance.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify resetItem and retryItem by removing unnecessary try...catch blocks.
Since handleMediaAction already handles errors and provides user feedback, the additional try...catch blocks in resetItem and retryItem may be redundant. Removing them can simplify the code.
Apply this diff to streamline the functions:
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);
- }
}Also applies to: 59-71
| @@ -27,7 +27,7 @@ export default [ | |||
| } | |||
| }, | |||
| rules: { | |||
| 'no-console': 'warn', | |||
| 'no-console': ['warn', { allow: ['warn', 'error'] }], | |||
There was a problem hiding this comment.
What should we do then? Don't we want to have some logging for troubleshooting?
| let isRetrying = false; | ||
|
|
||
| async function handleMediaAction( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
Problem is that I'm not sure what it return. I would have to do union types here. But I can do that.
| let isRetrying = false; | ||
|
|
||
| async function handleMediaAction( | ||
| action: (id: string) => Promise<any>, |
| loadingState: { set: (value: boolean) => void } | ||
| ) { |
| @@ -1,5 +1,6 @@ | |||
| import { fontFamily } from 'tailwindcss/defaultTheme'; | |||
| import type { Config } from 'tailwindcss'; | |||
| import { blur } from 'svelte/transition'; | |||
Add reset and retry functionality to episode cards:
Summary by CodeRabbit
New Features
Bug Fixes