Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated OpenAPI TypeScript generation to emit root-level type aliases, then replaced nested Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/lib/components/import-media/import-candidates-dialog.svelte (1)
26-80:⚠️ Potential issue | 🟠 MajorAbort the import flow when the create step does not return an id.
If the first POST fails, or comes back without
data.id, the code still calls the import endpoint with'no_id'. That turns one backend failure into a second, misleading one and hides the real cause. Bail out immediately after the create call when no id is available.Proposed fix
async function handleImportMedia(media: MetaDataProviderSearchResult) { isImporting = true; submitRequestError = null; let errored = null; if (isTv) { - let { data } = await client.POST('/api/v1/tv/shows', { + const { data, error } = await client.POST('/api/v1/tv/shows', { params: { query: { metadata_provider: media.metadata_provider as 'tmdb' | 'tvdb', show_id: media.external_id } } }); - let showId = data?.id ?? 'no_id'; - const { error } = await client.POST('/api/v1/tv/importable/{show_id}', { + if (error || !data?.id) { + submitRequestError = 'Failed to create show before import.'; + isImporting = false; + toast.error(submitRequestError); + return; + } + const showId = data.id; + const { error: importError } = await client.POST('/api/v1/tv/importable/{show_id}', { params: { path: { show_id: showId }, query: { directory: name } } }); - errored = error; + errored = importError; } else { - let { data } = await client.POST('/api/v1/movies', { + const { data, error } = await client.POST('/api/v1/movies', { params: { query: { metadata_provider: media.metadata_provider as 'tmdb' | 'tvdb', movie_id: media.external_id } } }); - let movieId = data?.id ?? 'no_id'; - const { error } = await client.POST('/api/v1/movies/importable/{movie_id}', { + if (error || !data?.id) { + submitRequestError = 'Failed to create movie before import.'; + isImporting = false; + toast.error(submitRequestError); + return; + } + const movieId = data.id; + const { error: importError } = await client.POST('/api/v1/movies/importable/{movie_id}', { params: { path: { movie_id: movieId }, query: { directory: name } } }); - errored = error; + errored = importError; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/components/import-media/import-candidates-dialog.svelte` around lines 26 - 80, The create step can return no id which leads to calling the import endpoint with 'no_id'; update handleImportMedia to detect when the first POST result has no data.id (check data?.id) after the client.POST to '/api/v1/tv/shows' and '/api/v1/movies', set isImporting = false, set submitRequestError (or errored) to a meaningful error, show a toast.error and return early so the subsequent client.POST to '/api/v1/tv/importable/{show_id}' or '/api/v1/movies/importable/{movie_id}' is not invoked with the placeholder 'no_id'. Ensure you reference the created id via showId/movieId only when present and do not proceed if missing.web/src/lib/components/library-combobox.svelte (1)
34-45:⚠️ Potential issue | 🟠 MajorDon't cast optional API payloads straight into
libraries.
client.GET(...)can leave.dataundefined on failure, and these casts hide that. If either request comes back without data, the laterlibraries.length/libraries.push(...)path throws duringonMount, so the combobox breaks instead of degrading cleanly.💡 Suggested fix
if (mediaType === 'tv') { - libraries = tvLibraries.data as LibraryItem[]; + libraries = tvLibraries.data ?? []; } else { - libraries = movieLibraries.data as LibraryItem[]; + libraries = movieLibraries.data ?? []; } if (!value && libraries.length > 0) { value = 'Default'; } - libraries.push({ - name: 'Default', - path: 'Default' - } as LibraryItem); + if (!libraries.some((item) => item.name === 'Default')) { + libraries = [...libraries, { name: 'Default', path: 'Default' }]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/components/library-combobox.svelte` around lines 34 - 45, The code currently casts tvLibraries.data/movieLibraries.data directly to LibraryItem[] which can be undefined and causes runtime errors; update the assignment to safely handle missing payloads by using a nullish fallback (e.g., set libraries = (tvLibraries.data ?? []) or (movieLibraries.data ?? [])), ensure libraries is an array before checking length or pushing, and append the Default LibraryItem using an array-safe operation so the onMount path never assumes .data exists (references: tvLibraries, movieLibraries, libraries, value, LibraryItem).
🧹 Nitpick comments (4)
web/package.json (1)
15-15: Keep the schema prefix unless you have a collision policy.
--root-types-no-schema-prefixremoves the defaultSchemaprefix from root-level type aliases (soUserinstead ofSchemaUser). While this improves import ergonomics, it removes the built-in mitigation against naming collisions. When duplicate aliases occur, the tool only deconflicts within generated root aliases themselves (using_2,_3suffixes), but there is no collision-resolution between schema-derived aliases and other non-alias root exports likeApiPaths,paths,components, oroperations. As your OpenAPI specification grows, this increases the risk of accidental collisions. Keep the default prefix and only flatten names deliberately via manual re-exports if you want a stable public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/package.json` at line 15, The OpenAPI generation script removes the Schema prefix via the --root-types-no-schema-prefix flag which risks naming collisions; update the "openapi" npm script (the openapi script entry in package.json) to omit --root-types-no-schema-prefix so generated root aliases keep the Schema prefix (e.g., SchemaUser), and if you need flattened names expose them via deliberate manual re-exports instead of changing the generator flag.web/src/routes/dashboard/+page.svelte (1)
10-10: Change import to use module path instead of declaration file.Importing from
'$lib/api/api.d.ts'is fragile—TypeScript will resolve'$lib/api/api'to the declaration file automatically, and Vite's module resolution doesn't include.d.tsin its default extensions. This can cause "works in editor but not in bundler" issues. Import from the module path instead, as the rest of the PR does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/dashboard/`+page.svelte at line 10, The import of MetaDataProviderSearchResult should reference the module path instead of the declaration file; update the import statement that currently points to '$lib/api/api.d.ts' to use '$lib/api/api' so Vite can resolve it correctly and avoid bundler-only failures—locate the import of MetaDataProviderSearchResult in +page.svelte and replace the module specifier accordingly.web/src/routes/dashboard/notifications/+page.svelte (1)
9-13: Alias the importedNotificationtype to avoid shadowing the Web Notifications API.In a browser Svelte module,
Notificationis a global type from the Web Notifications API. Importing a schema type with the same name shadows that global within this module's type scope, making future platform references awkward and reducing clarity. Aliasing the import is a recommended practice when both types may coexist.💡 Suggested change
-import type { Notification } from '$lib/api/api'; +import type { Notification as ApiNotification } from '$lib/api/api'; -let unreadNotifications: Notification[] = []; -let readNotifications: Notification[] = []; +let unreadNotifications: ApiNotification[] = []; +let readNotifications: ApiNotification[] = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/dashboard/notifications/`+page.svelte around lines 9 - 13, The imported type Notification conflicts with the browser's global Notification type; update the import to alias the schema type (e.g., import type { Notification as ApiNotification } from '$lib/api/api') and then update uses of Notification in this module — such as the declarations unreadNotifications: Notification[] and readNotifications: Notification[] — to use the aliased name (ApiNotification[]) so the global Web Notification type is not shadowed.web/src/lib/components/torrents/torrent-table.svelte (1)
94-97: Consider removing unnecessary non-null assertions.According to the
RichSeasonTorrentschema definition inapi.d.ts, theseasonsandepisodesfields are required (not optional), so the non-null assertions (!) are unnecessary when casting toRichSeasonTorrent.♻️ Proposed fix
<Table.Cell> - {convertTorrentSeasonRangeToIntegerRange((torrent as RichSeasonTorrent).seasons!)} + {convertTorrentSeasonRangeToIntegerRange((torrent as RichSeasonTorrent).seasons)} </Table.Cell> <Table.Cell> - {convertTorrentEpisodeRangeToIntegerRange((torrent as RichSeasonTorrent).episodes!)} + {convertTorrentEpisodeRangeToIntegerRange((torrent as RichSeasonTorrent).episodes)} </Table.Cell>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/components/torrents/torrent-table.svelte` around lines 94 - 97, The code casts torrent to RichSeasonTorrent and calls convertTorrentSeasonRangeToIntegerRange and convertTorrentEpisodeRangeToIntegerRange using non-null assertions on seasons and episodes; remove the unnecessary `!` after `(torrent as RichSeasonTorrent).seasons` and `(torrent as RichSeasonTorrent).episodes` because RichSeasonTorrent defines those fields as required—update the two Table.Cell expressions to pass the properties directly without `!`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/lib/components/user-data-table.svelte`:
- Around line 15-18: Currently the code assigns the live row object to
selectedUser and mutates it in the radio handlers, leaking unsaved changes into
the table; instead introduce a separate draft state (e.g., selectedUserDraft:
UserRead | null = $state(null)), clone the row into that draft when opening the
edit dialog (use structuredClone or {...user}), update only selectedUserDraft in
the radio handlers, and on successful PATCH apply the draft back to the real
object or update the users array (and clear the draft); on cancel simply discard
selectedUserDraft so sortedUsers/users remain unchanged.
In `@web/src/routes/dashboard/movies/`+page.svelte:
- Line 9: The import of MediaImportSuggestion uses an explicit .d.ts extension;
update the import to match the rest of the PR by removing the file extension
(i.e., import MediaImportSuggestion from '$lib/api/api' instead of
'$lib/api/api.d.ts') so resolution and style are consistent with other files
that reference the MediaImportSuggestion symbol.
---
Outside diff comments:
In `@web/src/lib/components/import-media/import-candidates-dialog.svelte`:
- Around line 26-80: The create step can return no id which leads to calling the
import endpoint with 'no_id'; update handleImportMedia to detect when the first
POST result has no data.id (check data?.id) after the client.POST to
'/api/v1/tv/shows' and '/api/v1/movies', set isImporting = false, set
submitRequestError (or errored) to a meaningful error, show a toast.error and
return early so the subsequent client.POST to '/api/v1/tv/importable/{show_id}'
or '/api/v1/movies/importable/{movie_id}' is not invoked with the placeholder
'no_id'. Ensure you reference the created id via showId/movieId only when
present and do not proceed if missing.
In `@web/src/lib/components/library-combobox.svelte`:
- Around line 34-45: The code currently casts
tvLibraries.data/movieLibraries.data directly to LibraryItem[] which can be
undefined and causes runtime errors; update the assignment to safely handle
missing payloads by using a nullish fallback (e.g., set libraries =
(tvLibraries.data ?? []) or (movieLibraries.data ?? [])), ensure libraries is an
array before checking length or pushing, and append the Default LibraryItem
using an array-safe operation so the onMount path never assumes .data exists
(references: tvLibraries, movieLibraries, libraries, value, LibraryItem).
---
Nitpick comments:
In `@web/package.json`:
- Line 15: The OpenAPI generation script removes the Schema prefix via the
--root-types-no-schema-prefix flag which risks naming collisions; update the
"openapi" npm script (the openapi script entry in package.json) to omit
--root-types-no-schema-prefix so generated root aliases keep the Schema prefix
(e.g., SchemaUser), and if you need flattened names expose them via deliberate
manual re-exports instead of changing the generator flag.
In `@web/src/lib/components/torrents/torrent-table.svelte`:
- Around line 94-97: The code casts torrent to RichSeasonTorrent and calls
convertTorrentSeasonRangeToIntegerRange and
convertTorrentEpisodeRangeToIntegerRange using non-null assertions on seasons
and episodes; remove the unnecessary `!` after `(torrent as
RichSeasonTorrent).seasons` and `(torrent as RichSeasonTorrent).episodes`
because RichSeasonTorrent defines those fields as required—update the two
Table.Cell expressions to pass the properties directly without `!`.
In `@web/src/routes/dashboard/`+page.svelte:
- Line 10: The import of MetaDataProviderSearchResult should reference the
module path instead of the declaration file; update the import statement that
currently points to '$lib/api/api.d.ts' to use '$lib/api/api' so Vite can
resolve it correctly and avoid bundler-only failures—locate the import of
MetaDataProviderSearchResult in +page.svelte and replace the module specifier
accordingly.
In `@web/src/routes/dashboard/notifications/`+page.svelte:
- Around line 9-13: The imported type Notification conflicts with the browser's
global Notification type; update the import to alias the schema type (e.g.,
import type { Notification as ApiNotification } from '$lib/api/api') and then
update uses of Notification in this module — such as the declarations
unreadNotifications: Notification[] and readNotifications: Notification[] — to
use the aliased name (ApiNotification[]) so the global Web Notification type is
not shadowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4b7a2aaf-639d-4e69-a2d5-7688fb1cb36c
📒 Files selected for processing (32)
web/package.jsonweb/src/lib/api/api.d.tsweb/src/lib/components/add-media-card.svelteweb/src/lib/components/delete-media-dialog.svelteweb/src/lib/components/download-dialogs/download-custom-dialog.svelteweb/src/lib/components/download-dialogs/download-movie-dialog.svelteweb/src/lib/components/download-dialogs/download-season-dialog.svelteweb/src/lib/components/download-dialogs/download-selected-episodes-dialog.svelteweb/src/lib/components/download-dialogs/download-selected-seasons-dialog.svelteweb/src/lib/components/download-dialogs/file-path-suffix-selector.svelteweb/src/lib/components/download-dialogs/select-file-path-suffix-dialog.svelteweb/src/lib/components/import-media/import-candidates-dialog.svelteweb/src/lib/components/import-media/suggested-media-card.svelteweb/src/lib/components/library-combobox.svelteweb/src/lib/components/nav/user-details.svelteweb/src/lib/components/recommended-media-carousel.svelteweb/src/lib/components/torrents/edit-torrent-dialog.svelteweb/src/lib/components/torrents/torrent-table.svelteweb/src/lib/components/user-data-table.svelteweb/src/lib/utils.tsweb/src/routes/dashboard/+layout.svelteweb/src/routes/dashboard/+page.svelteweb/src/routes/dashboard/movies/+page.svelteweb/src/routes/dashboard/movies/[movieId=uuid]/+page.svelteweb/src/routes/dashboard/movies/add-movie/+page.svelteweb/src/routes/dashboard/notifications/+page.svelteweb/src/routes/dashboard/settings/+page.svelteweb/src/routes/dashboard/tv/+page.svelteweb/src/routes/dashboard/tv/[showId=uuid]/+page.svelteweb/src/routes/dashboard/tv/[showId=uuid]/[SeasonId=uuid]/+page.svelteweb/src/routes/dashboard/tv/add-show/+page.svelteweb/src/routes/login/+page.svelte
📜 Review details
🔇 Additional comments (17)
web/src/lib/components/download-dialogs/file-path-suffix-selector.svelte (1)
5-14: Nice cleanup of the media prop type.
Movie | Showkeeps this component contract much easier to read without changing whatsaveDirectoryPreviewreceives.web/src/lib/components/download-dialogs/download-season-dialog.svelte (1)
13-20: Type migration is consistent here.Using
Showremoves the noisy schema lookup while keeping the existingshowaccess pattern unchanged.web/src/lib/components/nav/user-details.svelte (1)
3-4: Clean context typing update.
UserReadmakes the context contract much clearer with no behavioural change in this component.web/src/lib/components/recommended-media-carousel.svelte (1)
6-15: This alias fits the component well.
MetaDataProviderSearchResult[]matches the fields used here and makes the prop signature much easier to scan.web/src/routes/dashboard/+page.svelte (1)
13-25: The alias use is nicely consistent.Using
MetaDataProviderSearchResult[]for both state variables and casts makes the two recommendation flows much easier to follow.web/src/lib/components/download-dialogs/select-file-path-suffix-dialog.svelte (1)
5-14: Clean prop typing here as well.The direct
Movie | Showunion keeps this dialog aligned with its child component and is much easier to read.web/src/lib/components/add-media-card.svelte (1)
7-13: Good use of the exported search-result alias.
MetaDataProviderSearchResultmatches the fields this card actually consumes and makes the prop contract a lot clearer.web/src/routes/dashboard/tv/+page.svelte (1)
11-17: This context type reads much better now.
MediaImportSuggestion[]keeps theimportableShowscontract explicit without changing the surrounding logic.web/src/lib/components/delete-media-dialog.svelte (1)
2-2: Please verify the$lib/api/api.tsspecifier.The rest of this PR imports the generated OpenAPI types from
$lib/api/api, while this hunk still points at$lib/api/api.ts. If the generator only emitsweb/src/lib/api/api.d.ts, that explicit.tssuffix will not resolve for the type checker.💡 Suggested change
-import type { PublicMovie, PublicShow } from '$lib/api/api.ts'; +import type { PublicMovie, PublicShow } from '$lib/api/api';Run this to confirm whether a concrete
web/src/lib/api/api.tsfile exists and to compare current import usage:#!/bin/bash set -euo pipefail fd 'api(\.d)?\.ts$' web/src/lib/api -t f printf '\n--- imports using $lib/api/api ---\n' rg -n --fixed-strings "\$lib/api/api'" web/src printf '\n--- imports using $lib/api/api.ts ---\n' rg -n --fixed-strings "\$lib/api/api.ts'" web/srcweb/src/routes/dashboard/tv/[showId=uuid]/[SeasonId=uuid]/+page.svelte (1)
7-7: LGTM!The type imports and variable declarations have been correctly updated to use the explicit public types (
PublicEpisodeFile,Season,Show) instead of nestedcomponents['schemas']references. This aligns with the PR objective and improves readability without changing runtime behaviour.Also applies to: 14-16
web/src/lib/components/torrents/torrent-table.svelte (1)
10-10: LGTM!The type imports and declarations have been correctly updated to use the explicit public types. The prop typing, context typing, and function parameter typing are all consistent with the PR's objective.
Also applies to: 25-25, 31-33
web/src/routes/dashboard/tv/add-show/+page.svelte (1)
15-15: LGTM!The type import, variable declaration, and cast have been correctly updated to use
MetaDataProviderSearchResultdirectly. The explicit cast on line 43 appropriately bridges the API client's return type with the component's local typing.Also applies to: 20-20, 43-43
web/src/routes/dashboard/movies/[movieId=uuid]/+page.svelte (1)
8-8: LGTM!The type imports and variable declarations have been correctly updated to use the explicit public types (
PublicMovie,PublicMovieFile,UserRead). Since these are type aliases to the underlyingcomponents['schemas']types, they remain structurally compatible with the loader's return types.Also applies to: 20-22
web/src/routes/dashboard/+layout.svelte (1)
10-10: LGTM!The type import and state variable declarations have been correctly updated to use
MediaImportSuggestiondirectly. The context setup remains unchanged and will provide the correctly typed data to child components.Also applies to: 13-14
web/src/routes/dashboard/movies/add-movie/+page.svelte (1)
15-15: LGTM!The type import, variable declaration, and cast have been correctly updated to use
MetaDataProviderSearchResultdirectly, consistent with the parallel changes in the add-show page.Also applies to: 20-20, 42-42
web/src/routes/dashboard/movies/+page.svelte (1)
17-17: LGTM!The context retrieval with the explicit
MediaImportSuggestion[]return type is correctly updated.web/src/routes/dashboard/tv/[showId=uuid]/+page.svelte (1)
10-10: LGTM!The type imports and variable declarations have been correctly updated to use the explicit public types (
PublicShow,RichShowTorrent,UserRead). The usage oftorrents.torrentson line 356 confirms the structural expectation thatRichShowTorrentcontains a nestedtorrentsarray property.Also applies to: 30-32
| let { users }: { users: UserRead[] } = $props(); | ||
| let sortedUsers = $derived(users.sort((a, b) => a.email.localeCompare(b.email))); | ||
| let selectedUser: components['schemas']['UserRead'] | null = $state(null); | ||
| let userToDelete: components['schemas']['UserRead'] | null = $state(null); | ||
| let selectedUser: UserRead | null = $state(null); | ||
| let userToDelete: UserRead | null = $state(null); |
There was a problem hiding this comment.
Use a draft object for selectedUser, not the live table row.
The edit flow later assigns selectedUser = user, then mutates that object in place in the radio handlers. That means cancelling the dialog, or a failed PATCH, can still leave the unsaved flag changes visible in the table. Keep a cloned draft in dialog state and only apply it after a successful save.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/lib/components/user-data-table.svelte` around lines 15 - 18,
Currently the code assigns the live row object to selectedUser and mutates it in
the radio handlers, leaking unsaved changes into the table; instead introduce a
separate draft state (e.g., selectedUserDraft: UserRead | null = $state(null)),
clone the row into that draft when opening the edit dialog (use structuredClone
or {...user}), update only selectedUserDraft in the radio handlers, and on
successful PATCH apply the draft back to the real object or update the users
array (and clear the draft); on cancel simply discard selectedUserDraft so
sortedUsers/users remain unchanged.
1 participant
This PR adjusts the configuration to the
openapi-typescriptgeneration to export the schema types directly and allow importing them directly in the frontend code:Alternatively without the
--root-types-no-schema-prefixoption, the schemas would all be prefixed with "Schema" likeSchemaMetaDataProviderSearchResultwhich could be safer, if there are type collisions.LMK what's preferred.
Checklist:
npm run openapito re-generate theapi.d.tsnpm run lintnpm run formatSummary by CodeRabbit