feat: video admin cache components #8558
Conversation
- Updated @playwright/test and playwright packages to version 1.57.0. - Added @t3-oss/env-core dependency at version ^0.13.8.
- Updated all @nx packages to version 22.1.3. - Updated @storybook/nextjs and storybook to version 10.1.3. - Updated browserslist to version 4.27.0. - Updated various other dependencies in pnpm-lock.yaml.
…ultiple apps - Updated output paths and enabled webpack in project.json files for arclight, journeys, journeys-admin, and watch apps. - Modified TypeScript configurations to use "esnext" module and "bundler" module resolution for arclight, journeys, journeys-admin, and videos-admin. - Adjusted next-env.d.ts files to reference updated routes paths. - Refactored route.ts in arclight to import GET_SHORT_LINK_QUERY from a separate file. - Enhanced error handling in videoPlayerUrl/page.tsx by utilizing notFound() for missing parameters.
…er resolution across multiple projects
…16.0.7 and 19.2.3 respectively)
- Updated `react-dom`, `react-is`, and `nx` to version 19.2.3 and 22.1.3 respectively in `package.json`. - Adjusted `pnpm-lock.yaml` to reflect the updated versions and dependencies. - Refactored `.kube/config` for better clarity and structure, including updates to context and user configurations. - Modified `next-env.d.ts` files across multiple apps to use import statements for route types. - Simplified `next.config.js` and `next.config.mjs` by removing experimental flags and ensuring consistent configuration settings.
- Converted cluster and context entries to a list format for better readability. - Updated current context to 'prod' for immediate use. - Maintained user authentication configurations for both production and staging clusters.
- Converted cluster and context entries to a list format for better readability. - Updated current context to 'prod' for immediate use. - Maintained user authentication configurations for both production and staging clusters.
- Changed import statements from 'playwright-core' to '@playwright/test' across multiple files for consistency. - Updated package version of '@playwright/test' from 1.57.0 to 1.56.1 in `package.json` and `pnpm-lock.yaml` to align with the new import structure.
- Added `useParams` to retrieve `videoId` and `variantId` in the VariantDialog component for improved routing. - Updated test mocks to include `useParams` for consistent testing of route parameters.
…types - Replaced import statements in multiple next-env.d.ts files to use a consistent format. - Added eslint-disable comments to prevent linting errors related to module boundaries and path segments.
- Added a rule to allow literal strings in the ESLint configuration for the arclight app. - Updated eslint-disable comments in next-env.d.ts to include 'import/no-relative-packages'. - Removed unnecessary blank lines in getShortLinkQuery.ts. - Added a missing import statement for graphql in page.tsx. - Improved code organization in route.ts by adding a blank line for better readability.
- Removed unnecessary next-env.d.ts files from multiple apps. - Added typedRoutes configuration to next.config.js and next.config.mjs for arclight, journeys, player, watch, and watch-modern apps. - Updated .gitignore to exclude next-env.d.ts files and added .next/types/**/*.ts to tsconfig.json for relevant apps.
…@nx/vitest dependency - Changed the test executor in the jf-proxy project configuration to use @nx/vitest:test. - Added @nx/vitest as a dependency in package.json. - Updated pnpm-lock.yaml to include @nx/vitest version 22.1.3.
- Updated the journey object in OnboardingTemplateCard.spec.tsx to include additional properties: website, journeyCustomizationDescription, and journeyCustomizationFields. - Ensured the journey object is correctly typed as Journey for better type safety in tests.
- Deleted addByFileUtils.ts and addByFileUtils.spec.ts as they are no longer needed. - Removed testFile.mp4, which was used in the deleted tests.
…ariantDownloadDialogClient components - Reformatted function parameters for better alignment and clarity. - Removed unnecessary blank lines to streamline the code structure.
|
Found 8 test failures on Blacksmith runners: Failures
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/layout.tsx (1)
129-132: Missing dependencies inuseEffect.The effect references
refetchandreloadOnPathChangebut only listspathnamein the dependency array. This can lead to stale closures.🔎 Proposed fix
useEffect(() => { if (reloadOnPathChange) void refetch() setReloadOnPathChange(pathname?.includes('download') ?? false) - }, [pathname]) + }, [pathname, refetch, reloadOnPathChange])Note: If
refetchidentity changes on each render, you may need to wrap this logic differently or memoizerefetch. Verify with your Apollo Client setup.apps/videos-admin/src/app/(dashboard)/videos/[videoId]/editions/layout.tsx (2)
59-64: Missing dependency in useEffect.The
useEffectusesrefetchbut doesn't include it in the dependency array, which violates the exhaustive-deps rule and can cause stale closures.🔎 Proposed fix
useEffect(() => { if (reloadOnPathChange) void refetch() setReloadOnPathChange( (pathname?.includes('add') || pathname?.includes('delete')) ?? false ) - }, [pathname]) + }, [pathname, refetch, reloadOnPathChange])
87-104: Add accessibility attributes to interactive Box.The clickable Box lacks keyboard navigation and ARIA attributes required by the coding guidelines. Interactive elements should support both mouse and keyboard interactions.
🔎 Proposed fix
<Box key={edition.id} sx={{ border: '1px solid', borderColor: 'divider', borderRadius: 1, minWidth: 200, bgcolor: 'background.paper', '&:hover': { borderColor: 'action.hover', cursor: 'pointer' } }} onClick={() => router.push(`/videos/${videoId}/editions/${edition.id}`, { scroll: false }) } + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + router.push(`/videos/${videoId}/editions/${edition.id}`, { + scroll: false + }) + } + }} + role="button" + tabIndex={0} + aria-label={`View ${edition.name} edition`} data-testid="EditionCard" >As per coding guidelines, implement accessibility features on interactive elements.
♻️ Duplicate comments (1)
apps/journeys/project.json (1)
31-32: Verify the purpose of thewebpack: trueflag.Same concern as raised in
apps/arclight/project.json: this flag's purpose in the@nx/next:buildexecutor needs verification to ensure it aligns with the intended build configuration.
🧹 Nitpick comments (11)
apps/watch/src/components/PageVideos/FilterList/LanguagesFilter/LanguagesFilter.stories.tsx (1)
71-74: Consider removing the no-opplayfunction.The
playfunction currently returnsundefinedand serves no purpose. If interactive testing has been intentionally removed (as indicated by the removal ofscreenanduserEventimports), consider removing theplayproperty entirely for cleaner code.🔎 Proposed cleanup
export const Default = { - ...Template, - play: async () => undefined + ...Template }apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/layout.spec.tsx (1)
310-313: Unusedparamsprop being passed to component.The
paramsprop passed here is never consumed byVariantDialog— the component usesuseParams()hook instead (line 91-95 in layout.tsx). This prop can be removed from the test, or the component should be updated to actually use the prop.Since the component relies solely on
useParams(), the test correctness depends on the mock at lines 202-205, not this prop.🔎 Proposed simplification
return render( <MockedProvider> <SnackbarProvider> - <VariantDialog - params={Promise.resolve({ - variantId: mockVariantId, - videoId: mockVideoId - })} - > + <VariantDialog> <div>Child content</div> </VariantDialog> </SnackbarProvider> </MockedProvider> )This requires also removing the
paramsprop fromVariantDialogPropsin layout.tsx.apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/layout.tsx (1)
91-95: Unsafe type assertion onuseParams()result.The type assertion
as { variantId: string; videoId: string }assumes params always exist and are strings. If the component renders outside its expected route, this could cause runtime errors when accessingvariantIdorvideoId.🔎 Proposed fix with runtime guard
const routeParams = useParams() - const { variantId, videoId } = routeParams as { - variantId: string - videoId: string - } + const variantId = routeParams.variantId as string | undefined + const videoId = routeParams.videoId as string | undefined + + if (!variantId || !videoId) { + return null + }Alternatively, use a type guard or early return to handle missing params gracefully.
apps/journeys-admin/proxy.spec.ts (1)
5-5: Update describe block to match the rename.The describe block still references
'middleware'but the function under test is nowproxy.🔎 Proposed fix
-describe('middleware', () => { +describe('proxy', () => {apps/watch/proxy.spec.ts (1)
36-36: Update describe block to match the rename.The describe block still references
'middleware'but the function under test is nowproxy.🔎 Proposed fix
-describe('middleware', () => { +describe('proxy', () => {apps/resources/proxy.spec.ts (1)
36-36: Update describe block to match the rename.The describe block still references
'middleware'but the function under test is nowproxy.🔎 Proposed fix
-describe('middleware', () => { +describe('proxy', () => {apps/videos-admin/src/app/(dashboard)/videos/[videoId]/editions/layout.tsx (1)
118-137: Consider simplifying the conditional delete logic.The nested ternary for the delete action could be more readable by computing the actions object beforehand or using a clearer expression.
🔎 Proposed refactor
<ActionButton actions={{ edit: () => router.push( `/videos/${videoId}/editions/${edition.id}`, { scroll: false } ), - delete: () => - edition.name === 'base' - ? undefined - : router.push( - `/videos/${videoId}/editions/${edition.id}/delete`, - { - scroll: false - } - ) + delete: + edition.name === 'base' + ? undefined + : () => + router.push( + `/videos/${videoId}/editions/${edition.id}/delete`, + { + scroll: false + } + ) }} />apps/videos-admin/src/app/layout.tsx (2)
21-23: Consider adding a loading fallback for better UX.The
fallback={null}means no UI is rendered whileUserProvidersfetches user data viagetUser(). On slow connections or during auth initialization, this could result in a blank screen or flash of unstyled content.🔎 Consider adding a minimal loading state
-<Suspense fallback={null}> +<Suspense fallback={<div style={{ minHeight: '100vh' }} />}> <UserProviders>{children}</UserProviders> </Suspense>Or use a proper loading component if available:
-<Suspense fallback={null}> +<Suspense fallback={<RootLayoutSkeleton />}> <UserProviders>{children}</UserProviders> </Suspense>
29-48: Add return type annotation for clarity.The async
UserProviderscomponent should have an explicit return type annotation for better type safety and code clarity.🔎 Proposed type annotation
-async function UserProviders({ children }: { children: React.ReactNode }) { +async function UserProviders({ + children +}: { + children: React.ReactNode +}): Promise<ReactElement> { const user = await getUser()You'll need to import
ReactElement:-import { ReactNode, Suspense } from 'react' +import { ReactElement, ReactNode, Suspense } from 'react'apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/download/[downloadId]/add/AddVideoVariantDownloadDialogClient.tsx (2)
189-230: Consider parallelizing download creation for better performance.The three
createVideoVariantDownloadmutations are executed sequentially. If these operations are independent, consider usingPromise.all()to execute them concurrently, which would reduce the total time for the auto-generation flow.🔎 Proposed refactor
- await createVideoVariantDownload({ + await Promise.all([ + createVideoVariantDownload({ - variables: { - input: { - videoVariantId: variantId, - quality: 'high', - size: 0, - height: 720, - width: 1280, - url: `https://stream.mux.com/${data.videoVariant.muxVideo.playbackId}/720p.mp4`, - version: 0, - assetId: null - } - } - }) - await createVideoVariantDownload({ - variables: { - input: { - videoVariantId: variantId, - quality: 'sd', - size: 0, - height: 360, - width: 640, - url: `https://stream.mux.com/${data.videoVariant.muxVideo.playbackId}/360p.mp4`, - version: 0, - assetId: null - } - } - }) - await createVideoVariantDownload({ - variables: { - input: { - videoVariantId: variantId, - quality: 'low', - size: 0, - height: 270, - width: 480, - url: `https://stream.mux.com/${data.videoVariant.muxVideo.playbackId}/270p.mp4`, - version: 0, - assetId: null - } - } - }) + variables: { + input: { + videoVariantId: variantId, + quality: 'high', + size: 0, + height: 720, + width: 1280, + url: `https://stream.mux.com/${data.videoVariant.muxVideo.playbackId}/720p.mp4`, + version: 0, + assetId: null + } + } + }), + createVideoVariantDownload({ + variables: { + input: { + videoVariantId: variantId, + quality: 'sd', + size: 0, + height: 360, + width: 640, + url: `https://stream.mux.com/${data.videoVariant.muxVideo.playbackId}/360p.mp4`, + version: 0, + assetId: null + } + } + }), + createVideoVariantDownload({ + variables: { + input: { + videoVariantId: variantId, + quality: 'low', + size: 0, + height: 270, + width: 480, + url: `https://stream.mux.com/${data.videoVariant.muxVideo.playbackId}/270p.mp4`, + version: 0, + assetId: null + } + } + }) + ])
338-352: Consider simplifying the custom validation logic.The custom submit handler at lines 338-352 has complex conditional logic to skip file validation for 'auto' mode. While functional, this approach bypasses Formik's standard validation flow and may be confusing to maintainers.
Consider explicitly setting file validation to
.nullable().optional()when quality is 'auto' (which is already done in the schema at lines 158-162), and rely on Formik's built-in validation instead of custom filtering.
| message: 'Invalid start time parameter', | ||
| status: 400 | ||
| } | ||
| return <p>Invalid start time parameter</p> |
There was a problem hiding this comment.
Replace HTML <p> tags with MUI components.
The error messages use plain HTML <p> elements, which violates the coding guidelines that require using MUI components instead of HTML elements in apps/**/*.{js,jsx,ts,tsx} files. Consider using MUI's Typography or Alert component for better consistency and user experience.
As per coding guidelines: "Always use MUI over HTML elements; avoid using CSS or tags."
🔎 Suggested refactor using MUI Typography
+import { Typography } from '@mui/material'Then update the error returns:
if (startTime != null && (isNaN(startTime) || startTime < 0)) {
- return <p>Invalid start time parameter</p>
+ return <Typography>Invalid start time parameter</Typography>
}
if (endTime != null && (isNaN(endTime) || endTime < 0)) {
- return <p>Invalid end time parameter</p>
+ return <Typography>Invalid end time parameter</Typography>
}
if (startTime != null && endTime != null && endTime <= startTime) {
- return <p>End time must be greater than start time</p>
+ return <Typography>End time must be greater than start time</Typography>
}Alternatively, consider using Alert component for more prominent error display:
+import { Alert } from '@mui/material' if (startTime != null && (isNaN(startTime) || startTime < 0)) {
- return <p>Invalid start time parameter</p>
+ return <Alert severity="error">Invalid start time parameter</Alert>
}Also applies to: 122-122, 126-126
🤖 Prompt for AI Agents
In apps/arclight/src/app/videoPlayerUrl/page.tsx around lines 118, 122 and 126,
replace the plain HTML <p> error messages with MUI components (e.g., import
Typography or Alert from @mui/material) and return those components instead;
update imports at the top to include the chosen MUI component, replace each
return <p>...</p> with <Typography variant="body1"
color="error">...</Typography> or <Alert severity="error">...</Alert>, keeping
the original text content and any translations, and ensure any necessary props
(variant/color/severity) match surrounding UI conventions.
| const [deleteVideoVariant, { loading }] = useMutation(DELETE_VIDEO_VARIANT, { | ||
| variables: { id: variantId }, | ||
| onCompleted: () => { | ||
| enqueueSnackbar('Audio language deleted successfully', { | ||
| variant: 'success' | ||
| }) | ||
|
|
||
| router.push(returnUrl, { scroll: false }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add error handling for the mutation.
The mutation lacks an onError handler. If the delete operation fails (e.g., network error, permission denied), the user receives no feedback and the dialog remains open in an unclear state.
🔎 Proposed fix
const [deleteVideoVariant, { loading }] = useMutation(DELETE_VIDEO_VARIANT, {
variables: { id: variantId },
onCompleted: () => {
enqueueSnackbar('Audio language deleted successfully', {
variant: 'success'
})
router.push(returnUrl, { scroll: false })
- }
+ },
+ onError: (error) => {
+ enqueueSnackbar(error.message || 'Failed to delete audio language', {
+ variant: 'error'
+ })
+ }
})📝 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.
| const [deleteVideoVariant, { loading }] = useMutation(DELETE_VIDEO_VARIANT, { | |
| variables: { id: variantId }, | |
| onCompleted: () => { | |
| enqueueSnackbar('Audio language deleted successfully', { | |
| variant: 'success' | |
| }) | |
| router.push(returnUrl, { scroll: false }) | |
| } | |
| }) | |
| const [deleteVideoVariant, { loading }] = useMutation(DELETE_VIDEO_VARIANT, { | |
| variables: { id: variantId }, | |
| onCompleted: () => { | |
| enqueueSnackbar('Audio language deleted successfully', { | |
| variant: 'success' | |
| }) | |
| router.push(returnUrl, { scroll: false }) | |
| }, | |
| onError: (error) => { | |
| enqueueSnackbar(error.message || 'Failed to delete audio language', { | |
| variant: 'error' | |
| }) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/delete/DeleteAudioClient.tsx
around lines 49–58, the DELETE_VIDEO_VARIANT mutation is missing an onError
handler so failures provide no user feedback and the dialog stays in an unclear
state; add an onError callback to the useMutation options that captures the
GraphQL/network error, calls enqueueSnackbar(error.message || 'Failed to delete
audio language') with variant: 'error', optionally logs the error for debugging,
and avoid calling router.push so the dialog remains open for retry or
cancellation. Ensure the onError handler handles both ApolloError and generic
Error shapes and keeps UI state consistent.
| }} | ||
| loading={loading} | ||
| > | ||
| {`Are you sure you want to delete the ${data.videoVariant.language.name[0].value} audio language? This action cannot be undone.`} |
There was a problem hiding this comment.
Guard against null/undefined in nested data access.
Accessing data.videoVariant.language.name[0].value without null checks can throw if any property in the chain is null/undefined or if the name array is empty.
🔎 Proposed fix with optional chaining and fallback
- {`Are you sure you want to delete the ${data.videoVariant.language.name[0].value} audio language? This action cannot be undone.`}
+ {`Are you sure you want to delete the ${data.videoVariant?.language?.name?.[0]?.value ?? 'selected'} audio language? This action cannot be undone.`}📝 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.
| {`Are you sure you want to delete the ${data.videoVariant.language.name[0].value} audio language? This action cannot be undone.`} | |
| {`Are you sure you want to delete the ${data.videoVariant?.language?.name?.[0]?.value ?? 'selected'} audio language? This action cannot be undone.`} |
🤖 Prompt for AI Agents
In
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/delete/DeleteAudioClient.tsx
around line 75, the template literal directly accesses
data.videoVariant.language.name[0].value which can throw if any part is
null/undefined or name is empty; change this to safely read the value using
optional chaining and a fallback (e.g., const lang =
data?.videoVariant?.language?.name?.[0]?.value ?? 'unknown language') and use
that fallback variable in the message so the UI never crashes when data is
missing.
| export default async function Page({ params }: PageProps) { | ||
| const { videoId, variantId } = await params | ||
|
|
||
| return ( | ||
| <Dialog | ||
| open={true} | ||
| onClose={() => | ||
| router.push(returnUrl, { | ||
| scroll: false | ||
| }) | ||
| } | ||
| dialogTitle={{ | ||
| title: 'Delete Audio Language', | ||
| closeButton: true | ||
| }} | ||
| dialogAction={{ | ||
| onSubmit: deleteVideoVariant, | ||
| submitLabel: 'Delete', | ||
| closeLabel: 'Cancel' | ||
| }} | ||
| loading={loading} | ||
| > | ||
| {`Are you sure you want to delete the ${data.videoVariant.language.name[0].value} audio language? This action cannot be undone.`} | ||
| </Dialog> | ||
| ) | ||
| return <DeleteAudioClient videoId={videoId} variantId={variantId} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for Suspense in parent layout files and parent directories
rg -n "Suspense" "apps/videos-admin/src/app/(dashboard)/videos/\[videoId\]/audio" --type tsx --type ts
# Check the root dashboard layout and parent layouts
fd -t f -e tsx -e ts "layout\." "apps/videos-admin/src/app/(dashboard)"
# Check parent directories structure
fd -t d "audio" "apps/videos-admin/src/app/(dashboard)/videos"Repository: JesusFilm/core
Length of output: 924
🏁 Script executed:
# Inspect the DeleteAudioClient component to verify useSuspenseQuery usage
fd -t f "DeleteAudioClient" "apps/videos-admin/src" --exec cat {}
# If not found, search for where it might be defined
rg -n "export.*DeleteAudioClient" "apps/videos-admin/src" -A 5Repository: JesusFilm/core
Length of output: 2780
🏁 Script executed:
# Check the immediate parent layout for the delete page
cat -n "apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/layout.tsx"Repository: JesusFilm/core
Length of output: 13327
🏁 Script executed:
# Check the audio parent layout
cat -n "apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/layout.tsx"Repository: JesusFilm/core
Length of output: 9818
🏁 Script executed:
# Check the video parent layout
cat -n "apps/videos-admin/src/app/(dashboard)/videos/[videoId]/layout.tsx"Repository: JesusFilm/core
Length of output: 6545
🏁 Script executed:
# Check the dashboard layout
cat -n "apps/videos-admin/src/app/(dashboard)/layout.tsx"Repository: JesusFilm/core
Length of output: 1948
Correct Next.js async params pattern.
The page correctly follows the Next.js 15+ pattern for async params, awaiting the promise before passing values to the client component.
However, ensure a <Suspense> boundary exists in a parent layout wrapping this page. DeleteAudioClient uses useSuspenseQuery, and the immediate parent layout (VariantDialog) also uses useSuspenseQuery without a boundary. Without a Suspense wrapper, suspense will bubble up to the framework's error boundary, which may cause unexpected behavior.
🤖 Prompt for AI Agents
In
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/delete/page.tsx
around lines 12 to 15, the page correctly awaits async params but the reviewer
notes there is no Suspense boundary in a parent layout: both DeleteAudioClient
and the immediate parent layout (VariantDialog) use useSuspenseQuery so you must
add a React.Suspense boundary in the parent layout that wraps VariantDialog (or
higher) with a sensible fallback to prevent suspense from bubbling to the
framework error boundary; update the parent layout file to wrap the component
tree that renders VariantDialog (and thus this page) in <Suspense
fallback={...}>... </Suspense> ensuring the fallback is appropriate for loading
states.
| const uploadUrl = r2Asset.data?.cloudflareR2Create?.uploadUrl | ||
| if (uploadUrl == null) throw new Error('Upload URL is null') | ||
|
|
||
| await uploadAssetFile(uploadedFile, uploadUrl) | ||
|
|
||
| const publicUrl = r2Asset.data?.cloudflareR2Create?.publicUrl | ||
| if (publicUrl == null) return |
There was a problem hiding this comment.
Missing loading state cleanup on early return.
If publicUrl is null, the function returns early without setting isLoading back to false, potentially leaving the UI stuck in a loading state. Additionally, no error feedback is shown to the user.
🔎 Proposed fix
const publicUrl = r2Asset.data?.cloudflareR2Create?.publicUrl
-if (publicUrl == null) return
+if (publicUrl == null) {
+ enqueueSnackbar('Failed to retrieve public URL', { variant: 'error' })
+ setIsLoading(false)
+ return
+}🤖 Prompt for AI Agents
In
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/download/[downloadId]/add/AddVideoVariantDownloadDialogClient.tsx
around lines 277 to 283, the function returns early when publicUrl is null
without clearing isLoading or surfacing an error; modify the control flow so
that when publicUrl is null you set isLoading back to false and provide user
feedback (e.g., set an error state or show a toast), and better yet wrap the
async flow in try/catch/finally and move isLoading=false into finally so it
always runs while catching/reporting the error before returning.
| {uploadedFile != null ? ( | ||
| <LinkFile | ||
| name={uploadedFile.name} | ||
| link={URL.createObjectURL(uploadedFile)} | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
Potential memory leak from unreleased object URL.
The object URL created by URL.createObjectURL(uploadedFile) at line 406 is never revoked. Unlike the temporary URL in handleUpload, this one persists for the lifetime of the component, potentially causing memory leaks if users upload multiple files before submitting.
Consider storing the object URL in state and revoking it in a cleanup effect, or revoke it after the form submission completes.
🔎 Proposed fix
Add cleanup logic to revoke the object URL:
const [uploadedFile, setUploadedFile] = useState<File | null>(null)
+ const [fileObjectUrl, setFileObjectUrl] = useState<string | null>(null)
const [videoDimensions, setVideoDimensions] = useState<{
width: number
height: number
} | null>(null)
+ // Cleanup object URL on unmount or when file changes
+ useEffect(() => {
+ return () => {
+ if (fileObjectUrl != null) {
+ URL.revokeObjectURL(fileObjectUrl)
+ }
+ }
+ }, [fileObjectUrl])
const handleUpload = async (file: File): Promise<void> => {
if (file == null) return
+ // Revoke previous object URL if exists
+ if (fileObjectUrl != null) {
+ URL.revokeObjectURL(fileObjectUrl)
+ }
+
const video = document.createElement('video')
function cleanup() {
URL.revokeObjectURL(video.src)
video.remove()
}
video.onerror = function () {
cleanup()
enqueueSnackbar('Failed to load video', {
variant: 'error'
})
}
video.src = URL.createObjectURL(file)
video.onloadedmetadata = () => {
setVideoDimensions({ width: video.videoWidth, height: video.videoHeight })
cleanup()
}
setUploadedFile(file)
+ setFileObjectUrl(URL.createObjectURL(file))
}
// In the JSX, use the stored URL
{uploadedFile != null ? (
<LinkFile
name={uploadedFile.name}
- link={URL.createObjectURL(uploadedFile)}
+ link={fileObjectUrl ?? ''}
/>
) : null}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/download/[downloadId]/add/AddVideoVariantDownloadDialogClient.tsx
around lines 403 to 408, the temporary object URL created by
URL.createObjectURL(uploadedFile) is never revoked which can leak memory; store
the object URL in component state (e.g. setPreviewUrl) instead of calling
URL.createObjectURL inline, use that state value for the LinkFile link, and
revoke the previous URL whenever a new file is set and in a useEffect cleanup
(and also revoke after successful form submission) to ensure URLs are released.
| <AddVideoVariantDownloadDialogClient | ||
| videoId={videoId} | ||
| variantId={variantId} | ||
| languageId={downloadId} | ||
| /> |
There was a problem hiding this comment.
Prop name mismatch: downloadId passed as languageId.
The route parameter downloadId is passed to the client component's languageId prop. This naming inconsistency makes the code confusing and suggests either:
- The parameter should be renamed to
languageIdin the route, or - The prop in the client component should be renamed to
downloadId
This is the same issue flagged in the client component. Please align the naming throughout the flow.
🤖 Prompt for AI Agents
In
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/download/[downloadId]/add/page.tsx
lines 15-19, the route param downloadId is being passed into
AddVideoVariantDownloadDialogClient as languageId which is a naming mismatch;
change the prop to downloadId (and update the client component's
prop/interface/type and all its usages to accept downloadId instead of
languageId) so the route param and prop name align, and update any TypeScript
types, tests or imports that reference languageId to use downloadId.
| interface VariantDialogProps { | ||
| children: ReactNode | ||
| params: { | ||
| params: Promise<{ | ||
| variantId: string | ||
| videoId: string | ||
| } | ||
| }> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dead code: params prop is defined but never used.
The params prop is declared in VariantDialogProps (lines 41-44) but the component extracts only children from props (line 90) and retrieves route parameters via useParams() hook instead (lines 91-95).
For client components ('use client'), using useParams() is the correct approach. The Promise-based params prop pattern is intended for Server Components.
🔎 Proposed fix — remove unused params prop
interface VariantDialogProps {
children: ReactNode
- params: Promise<{
- variantId: string
- videoId: string
- }>
}Also applies to: 87-95
🤖 Prompt for AI Agents
In
apps/videos-admin/src/app/(dashboard)/videos/[videoId]/audio/[variantId]/layout.tsx
around lines 39 to 45 (and related usage at 87–95), the VariantDialogProps
interface declares a Promise-based params prop that is never used because the
component reads route params via useParams() (client component). Remove the
unused params entry from VariantDialogProps, update the component's prop typing
to only expect children, and delete any unused params references/imports so the
type matches runtime usage.
| export default function EditionsPage({ children }: EditionsPageProps) { | ||
| return ( | ||
| <Suspense | ||
| fallback={ | ||
| <Section title="Editions"> | ||
| <Box sx={{ display: 'grid', placeItems: 'center', height: 200 }}> | ||
| <Typography>Loading...</Typography> | ||
| </Box> | ||
| </Section> | ||
| } | ||
| > | ||
| <EditionsContent>{children}</EditionsContent> | ||
| </Suspense> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/videos-admin/src/app/\(dashboard\)/videos/\[videoId\]/editions/layout.tsxRepository: JesusFilm/core
Length of output: 5755
🏁 Script executed:
# Check Apollo Client configuration
find . -type f \( -name "*.ts" -o -name "*.tsx" \) -path "*apps/videos-admin*" | grep -i apollo | head -20Repository: JesusFilm/core
Length of output: 365
🏁 Script executed:
# Search for ApolloClient setup and configuration
rg -n "ApolloClient|createApolloClient|ApolloProvider" --type=ts --type=tsx -g "apps/videos-admin/**" -A 5 -B 2Repository: JesusFilm/core
Length of output: 85
🏁 Script executed:
# Search for useSuspenseQuery usage in the codebase
rg -n "useSuspenseQuery" --type=ts --type=tsx -g "apps/**"Repository: JesusFilm/core
Length of output: 85
🏁 Script executed:
cat -n apps/videos-admin/src/libs/apollo/makeClient.tsRepository: JesusFilm/core
Length of output: 930
🏁 Script executed:
cat -n apps/videos-admin/src/libs/apollo/apolloClient.tsRepository: JesusFilm/core
Length of output: 1749
🏁 Script executed:
# Search for suspense configuration in Apollo setup
rg -n "suspense" --type ts -g "apps/videos-admin/src/libs/apollo/**"Repository: JesusFilm/core
Length of output: 40
🏁 Script executed:
# Check if there are other Suspense + useQuery patterns in the codebase
rg -n "Suspense" --type ts -A 3 -B 1 -g "apps/videos-admin/src/**" | head -50Repository: JesusFilm/core
Length of output: 5183
Use useSuspenseQuery instead of useQuery for Suspense to work.
The Suspense wrapper won't display the loading fallback because useQuery doesn't suspend—it returns { data, loading, error } synchronously. To enable Suspense, replace useQuery with useSuspenseQuery:
-const { data, refetch } = useQuery(GET_EDITIONS, {
+const { data, refetch } = useSuspenseQuery(GET_EDITIONS, {
variables: { videoId }
})Also update the import to include useSuspenseQuery:
-import { useQuery } from '@apollo/client'
+import { useQuery, useSuspenseQuery } from '@apollo/client'Note: Remove the unused loading state handling if present, as useSuspenseQuery throws promises that the Suspense boundary will catch.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/videos-admin/src/app/(dashboard)/videos/[videoId]/editions/layout.tsx
around lines 35 to 49, the Suspense fallback never shows because the
component(s) inside use useQuery (which does not suspend); replace useQuery with
useSuspenseQuery in the EditionsContent (and any child hooks) so those hooks
throw promises that the Suspense boundary can catch, update the import to pull
useSuspenseQuery from your data-fetching library instead of useQuery, and remove
any local loading boolean handling or conditional renders that duplicate the
Suspense fallback so the Suspense boundary handles loading states.
| export default function RootLayout({ | ||
| children | ||
| }: { | ||
| children: React.ReactNode | ||
| }): Promise<ReactNode> { | ||
| const user = await getUser() | ||
|
|
||
| }): ReactNode { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and read the layout.tsx file
find apps/videos-admin -name "layout.tsx" -type f 2>/dev/null | head -5Repository: JesusFilm/core
Length of output: 412
🏁 Script executed:
#!/bin/bash
# Get the actual content of the layout file
if [ -f "apps/videos-admin/src/app/layout.tsx" ]; then
cat -n "apps/videos-admin/src/app/layout.tsx"
else
echo "File not found at apps/videos-admin/src/app/layout.tsx"
# Try to find it
find . -name "layout.tsx" -path "*/videos-admin/*" 2>/dev/null
fiRepository: JesusFilm/core
Length of output: 1739
🏁 Script executed:
#!/bin/bash
# Search for other layout files and async patterns in videos-admin
fd -e tsx -e ts 'layout\.(tsx|ts)$' apps/videos-admin/src 2>/dev/null
# Also check for Suspense usage patterns
rg -B 2 -A 5 'Suspense' apps/videos-admin/src --type tsx --type ts 2>/dev/null | head -50Repository: JesusFilm/core
Length of output: 640
🏁 Script executed:
#!/bin/bash
# Check for UserProviders pattern
rg -B 2 -A 8 'UserProviders' apps/videos-admin/src --type tsx --type ts 2>/dev/nullRepository: JesusFilm/core
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check other layout files for async patterns
for file in apps/videos-admin/src/app/'(dashboard)'/layout.tsx apps/videos-admin/src/app/users/layout.tsx; do
if [ -f "$file" ]; then
echo "=== $file ==="
cat -n "$file" | head -30
echo ""
fi
doneRepository: JesusFilm/core
Length of output: 1829
🏁 Script executed:
#!/bin/bash
# Check Next.js version in the project
if [ -f "package.json" ]; then
rg '"next":\s*"[^"]*"' package.json
fi
# Also check if there's a .next or similar indicating Next.js version
if [ -f "apps/videos-admin/package.json" ]; then
echo "=== apps/videos-admin/package.json ==="
rg '"next"' apps/videos-admin/package.json
fiRepository: JesusFilm/core
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Search for getUser implementation to understand what it does
rg -B 2 -A 10 'export.*function.*getUser|export const getUser' apps/videos-admin/src --type ts --type tsxRepository: JesusFilm/core
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Search for getUser implementation
rg -B 2 -A 10 'export.*getUser|const getUser' apps/videos-admin/src --type tsRepository: JesusFilm/core
Length of output: 486
🏁 Script executed:
#!/bin/bash
# Also try without type specification
rg -B 2 -A 10 'export.*getUser|const getUser' apps/videos-admin/srcRepository: JesusFilm/core
Length of output: 486
🏁 Script executed:
#!/bin/bash
# Check if getUser uses cookies() or similar async operations
find apps/videos-admin/src -name "*.ts" -o -name "*.tsx" | xargs grep -l "getUser" | head -5Repository: JesusFilm/core
Length of output: 191
Add loading fallback for Suspense and return type for UserProviders component.
The Suspense + async child component pattern is correct for Next.js 16 and enables proper streaming; however, two improvements are needed:
-
Replace
fallback={null}— While fetching the user, no loading UI is shown. Add a loading skeleton or placeholder:<Suspense fallback={<LoadingFallback />}> <UserProviders>{children}</UserProviders> </Suspense>
-
Add return type annotation to UserProviders — The async component is missing its return type:
async function UserProviders({ children }: { children: React.ReactNode }): Promise<ReactElement> {
🤖 Prompt for AI Agents
In apps/videos-admin/src/app/layout.tsx around lines 12-16, replace the Suspense
fallback={null} with a real loading placeholder (e.g.,
fallback={<LoadingFallback />}) and ensure LoadingFallback is imported, and add
an explicit return type to the async UserProviders component (e.g., async
function UserProviders({ children }: { children: React.ReactNode }):
Promise<ReactElement>) and import ReactElement from React; this provides a
visible loading state while user data loads and fixes the missing return type on
the async component.
Summary by CodeRabbit
New Features
Dependency Updates
Build Improvements
✏️ Tip: You can customize this high-level summary in your review settings.