From b51e690062cf14ec49c94ff2e10c7366bb928e24 Mon Sep 17 00:00:00 2001 From: yusufipk Date: Sun, 26 Jul 2026 18:53:54 +0700 Subject: [PATCH] fix: close the findings the test suite surfaced The suite that landed in #43/#44 was written against existing behaviour, so a number of tests pinned bugs rather than asserting correct behaviour. This fixes the production code and moves each of those tests onto the fixed behaviour in the same change. Security: - project-download: derive the archive entry extension from the last path segment and restrict it to a short alphanumeric run, so an extensionless allowlisted url can no longer contribute a path separator; validate the r2 branch against the strict proxy-path pattern instead of a `startsWith`, which let `/api/upload/video/clip.mp4/../../etc/passwd` through verbatim. - rate-limit: hash a key or action wider than its column instead of skipping the query. Both the guard and the failing INSERT used to answer "allowed", so the limit stopped applying entirely. Warn at startup when TRUSTED_PROXY_MODE is unset in production. - video uploads: the file name decides the content type; a client-declared video mime no longer makes `payload.exe` acceptable. - email templates: escape in the helpers rather than relying on every caller, with an explicit `rawEmailHtml()` opt-out for the one call site that builds markup. `escapeHtml` now covers the single quote. - CSP: allow loopback object storage outside production only. - route-access: reach the billing redirect only for the workspace owner. Keying it off the owner's billing status alone made the redirect target an oracle for whose subscription had lapsed, and sent members to a page they cannot act on. - search: carry the same billing condition every other read path carries. - logger: check `err.name` as well as `err.constructor.name`, so a re-thrown, deserialised or minified Prisma error is still redacted. - upload tokens: resolve the signing secret outside the try, so a server booted without one fails loudly instead of reporting every grant as a forgery. - invitations: never downgrade an existing membership, and report a scoped invitation that points at nothing as not_found rather than accepted. - auth: resolve the workspace role for every signed-in caller, so checkProjectAccess and computeProjectAccess stop disagreeing about the owner who also owns the workspace. The `intent` option is gone with it. - r2-media-proxy: validate the object key inside the proxy so the guard travels with the function; delete the unused, unanchored `mediaUrlToR2Key`. - r2: sign the content type into presigned PUT grants. Correctness: - frame rate snapping picks the nearest standard, not the first within tolerance, so 24, 30 and 60 fps are reachable at all. - a version upload registers its Bunny cleanup as soon as bunny-init answers, so a failed tus upload no longer leaves a billed video behind. - deleting videos clears storage before the rows, so a refused DELETE leaves a retryable row rather than an orphaned object. - an expired upload session can be cancelled, which is what releases its quota. - `voice/` joins the delete allowlist, so a voice note can be removed by the module that wrote it. - a failed CORS write propagates instead of being mistaken for an empty config and replacing the bucket's rules. - filtering projects by workspace no longer hides projects the unfiltered call returns. - upload retries skip aborts and permanent 4xx; progress no longer divides by zero. - reply edits no longer clear the comment's tag; optimistic resolve rolls back to the state it replaced; the delete snapshot is captured once. - assorted UI fixes: duplicate React keys, double-click guards reading stale closures, the tag list fetched twice per load, a failed member list rendering as an empty one, a stale "Initializing upload..." beside a failure, and a registration banner pointing at an email that never arrives. Consistency and access: - the two download routes answer 404 for an id belonging to another tenant, as the comment export route already did. A caller who does belong still gets 403. - accessible names for the share-link password field, the guest name gates, the version dialog inputs and the comment-tag controls. Repository health: - the runner image installs production dependencies only. - a setup file for the unit project restores stubbed env centrally. - native tsconfig path resolution replaces vite-tsconfig-paths. - `uploadBytesWithProgress` exists once. - admin stats bill Bunny storage to the workspace owner like every other quota, gate on the configured flag, wire up the single-flight guard and count the statuses that belonged to no bucket. - `r2Client.destroy()` releases the presign client too. - `prepare` tolerates a production install, where husky is absent. --- Dockerfile | 13 ++- app/(auth)/login/login-form.tsx | 9 +- .../settings/project-settings-page-client.tsx | 23 +++- .../videos/new/new-video-page-client.tsx | 4 + app/admin/page.tsx | 11 ++ app/api/approvals/[requestId]/cancel/route.ts | 6 +- app/api/comments/[commentId]/route.ts | 4 +- .../[projectId]/approval-candidates/route.ts | 2 +- .../[projectId]/members/[memberId]/route.ts | 4 +- .../invitations/[invitationId]/route.ts | 2 +- app/api/projects/[projectId]/members/route.ts | 2 +- app/api/projects/[projectId]/route.ts | 4 +- .../[projectId]/tags/[tagId]/route.ts | 4 +- app/api/projects/[projectId]/tags/route.ts | 2 +- .../[projectId]/videos/[videoId]/route.ts | 4 +- .../[videoId]/versions/[versionId]/route.ts | 2 +- .../videos/[videoId]/versions/route.ts | 2 +- .../[projectId]/videos/bulk-delete/route.ts | 15 ++- .../[projectId]/videos/bunny-init/route.ts | 2 +- .../projects/[projectId]/videos/move/route.ts | 6 +- .../[projectId]/videos/r2-complete/route.ts | 2 +- .../[projectId]/videos/r2-init/route.ts | 2 +- app/api/projects/[projectId]/videos/route.ts | 2 +- app/api/projects/route.ts | 17 +-- app/api/search/route.ts | 11 +- .../versions/[versionId]/approvals/route.ts | 4 +- .../versions/[versionId]/download/route.ts | 9 ++ .../assets/[assetId]/download/route.ts | 10 +- bun.lock | 7 -- components/guest-gate.tsx | 14 +-- components/members-management-page.tsx | 5 + components/share-link-unlock.tsx | 13 ++- components/video-page-content.tsx | 4 +- components/video-page/asset-list-section.tsx | 8 +- components/video-page/assets-pane.tsx | 6 +- components/video-page/comment-rich-text.tsx | 13 ++- components/video-page/comments-pane.tsx | 11 +- components/video-page/guest-name-gate.tsx | 4 + .../video-page/hooks/use-comment-actions.ts | 28 ++++- .../video-page/hooks/use-download-actions.ts | 12 ++- .../video-page/hooks/use-version-actions.ts | 37 ++++--- .../video-page/hooks/use-video-assets.ts | 19 ++-- .../video-page/hooks/use-video-page-data.ts | 12 ++- .../video-page/hooks/use-video-player.ts | 9 +- .../video-page/hooks/video-player-utils.ts | 19 +++- .../video-page/version-actions-dialog.tsx | 6 +- lib/admin-stats.ts | 49 +++++++-- lib/auth.ts | 82 ++++++-------- lib/billing.ts | 18 ++-- lib/bunny-upload-token.ts | 6 +- lib/client/r2-asset-video-upload.ts | 41 +------ lib/client/r2-video-upload.ts | 8 +- lib/client/upload-chunking.ts | 33 ++++++ lib/comment-export.ts | 9 +- lib/content-security-policy.ts | 12 ++- lib/email-brand.ts | 52 ++++++--- lib/email-verification.ts | 5 +- lib/invitations.ts | 75 +++++++------ lib/logger.ts | 22 +++- lib/notifications.ts | 95 ++++++++-------- lib/project-download.ts | 34 ++++-- lib/r2-media-proxy.ts | 15 +++ lib/r2-upload-session.ts | 11 +- lib/r2-upload-token.ts | 8 +- lib/r2.ts | 55 +++++++--- lib/rate-limit.ts | 45 ++++++-- lib/route-access.ts | 23 ++-- lib/validation.ts | 11 +- lib/video-assets.ts | 23 ++-- lib/video-delete.ts | 54 +++++++--- lib/video-providers/direct.ts | 11 +- lib/video-upload-validation.ts | 22 ++-- package.json | 3 +- tests/api/assets-authz.test.ts | 12 ++- tests/api/auth-matrix.test.ts | 16 ++- tests/api/download-authz.test.ts | 13 ++- tests/api/invitations.test.ts | 101 +++++++++++++++--- tests/api/lib-admin-stats.test.ts | 25 +++-- tests/api/lib-project-access.test.ts | 93 +++------------- tests/api/lib-r2-upload-session.test.ts | 17 +-- tests/api/lib-video-assets.test.ts | 34 +++--- tests/api/lib-video-delete.test.ts | 61 +++++++---- tests/api/projects.test.ts | 11 +- tests/api/rate-limit.test.ts | 44 ++++---- tests/api/search-scope.test.ts | 49 ++++++--- tests/component/comment-rich-text.test.tsx | 18 ++-- .../hooks/use-comment-actions.test.ts | 32 ++++-- .../hooks/use-download-actions.test.ts | 13 +-- .../hooks/use-version-actions.test.ts | 12 +-- .../component/hooks/use-video-assets.test.ts | 38 ++++++- .../hooks/use-video-page-data.test.ts | 14 +-- tests/component/share-link-unlock.test.tsx | 17 ++- tests/setup/unit.ts | 14 +++ tests/unit/lib/bunny-upload-token.test.ts | 8 +- tests/unit/lib/client/upload-chunking.test.ts | 45 ++++++++ tests/unit/lib/comment-export.test.ts | 8 +- .../unit/lib/content-security-policy.test.ts | 20 +++- tests/unit/lib/email-brand.test.ts | 50 +++++++-- tests/unit/lib/logger.test.ts | 39 +++++-- tests/unit/lib/project-download.test.ts | 86 ++++++++++----- tests/unit/lib/r2-media-proxy.test.ts | 45 +++++--- tests/unit/lib/r2-upload-token.test.ts | 9 +- tests/unit/lib/r2.test.ts | 60 ++++++++--- tests/unit/lib/rate-limit.test.ts | 78 ++++++++++++-- tests/unit/lib/route-access.test.ts | 69 +++++++++--- tests/unit/lib/upload-validation.test.ts | 19 ++-- tests/unit/lib/validation.test.ts | 16 +-- tests/unit/lib/video-providers.test.ts | 15 ++- tests/unit/video-player-utils.test.ts | 27 +++-- vitest.config.ts | 9 +- vitest.mutation.config.ts | 5 +- 111 files changed, 1665 insertions(+), 804 deletions(-) create mode 100644 tests/setup/unit.ts diff --git a/Dockerfile b/Dockerfile index 391f3dd..ac22069 100644 --- a/Dockerfile +++ b/Dockerfile @@ -6,6 +6,14 @@ COPY package.json bun.lock ./ COPY prisma ./prisma RUN bun install --frozen-lockfile +# The tree the runner ships. The build needs eslint, vitest, playwright and the rest; the +# running app does not, and copying the full tree put them all in the image. +FROM base AS prod-deps +COPY package.json bun.lock ./ +COPY prisma ./prisma +RUN bun install --frozen-lockfile --production +RUN bun run db:generate + FROM deps AS build COPY app ./app COPY components ./components @@ -45,7 +53,10 @@ COPY --from=build /app/lib ./lib COPY --from=build /app/app ./app COPY --from=build /app/components ./components COPY --from=build /app/types ./types -COPY --from=build /app/node_modules ./node_modules +COPY --from=prod-deps /app/node_modules ./node_modules +# next.config.ts and prisma.config.ts are TypeScript, and both are loaded at startup, so +# the compiler has to be present even though nothing else here needs it. +COPY --from=deps /app/node_modules/typescript ./node_modules/typescript COPY --from=build /app/.next ./.next COPY --from=build /app/tsconfig.json ./tsconfig.json COPY --from=build /app/postcss.config.mjs ./postcss.config.mjs diff --git a/app/(auth)/login/login-form.tsx b/app/(auth)/login/login-form.tsx index cc820e6..46ee1e0 100644 --- a/app/(auth)/login/login-form.tsx +++ b/app/(auth)/login/login-form.tsx @@ -124,10 +124,15 @@ function LoginFormInner({ googleEnabled, githubEnabled }: LoginFormInnerProps) { + {/* + `?registered=true` is only ever reached when email verification is off: the + register page sends a user who has to verify to /verify-email instead. Telling + this one to go and check a mailbox pointed them at a message that never arrives, + on a self-hosted deployment without SMTP, which is the documented default. + */} {showSuccess && (
- Account created successfully! Please check your email to verify your address before - signing in. + Account created successfully! You can sign in now.
)} diff --git a/app/(dashboard)/projects/[projectId]/settings/project-settings-page-client.tsx b/app/(dashboard)/projects/[projectId]/settings/project-settings-page-client.tsx index 5c8d424..0bf280d 100644 --- a/app/(dashboard)/projects/[projectId]/settings/project-settings-page-client.tsx +++ b/app/(dashboard)/projects/[projectId]/settings/project-settings-page-client.tsx @@ -438,20 +438,32 @@ export default function ProjectSettingsPageClient({ projectId }: ProjectSettings setEditTagColor(e.target.value)} className="w-8 h-8 rounded cursor-pointer border-0" /> setEditTagName(e.target.value)} className="flex-1 h-8" onKeyDown={(e) => e.key === 'Enter' && handleUpdateTag(tag.id)} /> - - ) : ( @@ -476,9 +488,10 @@ export default function ProjectSettingsPageClient({ projectId }: ProjectSettings size="sm" variant="ghost" className="text-destructive hover:text-destructive" + aria-label={`Delete tag ${tag.name}`} onClick={() => handleDeleteTag(tag.id)} > - + + {/* UNPAID, INCOMPLETE and INCOMPLETE_EXPIRED, which belonged to none of the + buckets above and so were counted nowhere. */} + + + Unpaid or Incomplete + + + +
{stripeStats.otherStatusUsers}
+ + )} diff --git a/app/api/approvals/[requestId]/cancel/route.ts b/app/api/approvals/[requestId]/cancel/route.ts index a481d83..c1c2d4c 100644 --- a/app/api/approvals/[requestId]/cancel/route.ts +++ b/app/api/approvals/[requestId]/cancel/route.ts @@ -40,11 +40,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { }); if (!approvalRequest) return apiErrors.notFound('Approval request'); - const access = await checkProjectAccess( - approvalRequest.version.video.project, - session.user.id, - { intent: 'manage' } - ); + const access = await checkProjectAccess(approvalRequest.version.video.project, session.user.id); const canCancel = approvalRequest.requestedById === session.user.id || access.canEdit; if (!canCancel) return apiErrors.forbidden('Access denied'); diff --git a/app/api/comments/[commentId]/route.ts b/app/api/comments/[commentId]/route.ts index 04c103c..2156a86 100644 --- a/app/api/comments/[commentId]/route.ts +++ b/app/api/comments/[commentId]/route.ts @@ -133,7 +133,7 @@ export async function PATCH(request: NextRequest, { params }: RouteParams) { const project = comment.version.video.project; const userId = session?.user?.id ?? null; - const access = await checkProjectAccess(project, userId ?? undefined, { intent: 'manage' }); + const access = await checkProjectAccess(project, userId ?? undefined); const isOwner = userId === project.ownerId; const isAuthor = !!userId && comment.authorId === userId; const guestIdentityId = !userId ? getGuestIdentityFromRequest(request) : null; @@ -295,7 +295,7 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { const isAuthor = !!userId && comment.authorId === userId; // Project owners/admins and workspace admins can delete any comment - const access = userId ? await checkProjectAccess(project, userId, { intent: 'manage' }) : null; + const access = userId ? await checkProjectAccess(project, userId) : null; const isPrivilegedUser = !!access?.canEdit; let canDelete = isAuthor || isPrivilegedUser; diff --git a/app/api/projects/[projectId]/approval-candidates/route.ts b/app/api/projects/[projectId]/approval-candidates/route.ts index e80f76d..51cf427 100644 --- a/app/api/projects/[projectId]/approval-candidates/route.ts +++ b/app/api/projects/[projectId]/approval-candidates/route.ts @@ -20,7 +20,7 @@ export async function GET(_request: NextRequest, { params }: RouteParams) { }); if (!project) return apiErrors.notFound('Project'); - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); if (!access.canEdit) return apiErrors.forbidden('Access denied'); const candidates = await getApprovalCandidatesForProject(projectId); diff --git a/app/api/projects/[projectId]/members/[memberId]/route.ts b/app/api/projects/[projectId]/members/[memberId]/route.ts index 5257e49..202b4aa 100644 --- a/app/api/projects/[projectId]/members/[memberId]/route.ts +++ b/app/api/projects/[projectId]/members/[memberId]/route.ts @@ -30,7 +30,7 @@ export async function PATCH(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); const isOwner = project.ownerId === session.user.id; const isAdmin = project.members[0]?.role === ProjectMemberRole.ADMIN; @@ -93,7 +93,7 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); const isOwner = project.ownerId === session.user.id; const isAdmin = project.members[0]?.role === ProjectMemberRole.ADMIN; diff --git a/app/api/projects/[projectId]/members/invitations/[invitationId]/route.ts b/app/api/projects/[projectId]/members/invitations/[invitationId]/route.ts index 06ba58b..7bbb706 100644 --- a/app/api/projects/[projectId]/members/invitations/[invitationId]/route.ts +++ b/app/api/projects/[projectId]/members/invitations/[invitationId]/route.ts @@ -30,7 +30,7 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); const isOwner = project.ownerId === session.user.id; const isAdmin = project.members[0]?.role === ProjectMemberRole.ADMIN; diff --git a/app/api/projects/[projectId]/members/route.ts b/app/api/projects/[projectId]/members/route.ts index c5bc532..0a1bdc7 100644 --- a/app/api/projects/[projectId]/members/route.ts +++ b/app/api/projects/[projectId]/members/route.ts @@ -112,7 +112,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); const isOwner = project.ownerId === session.user.id; const isAdmin = project.members[0]?.role === ProjectMemberRole.ADMIN; diff --git a/app/api/projects/[projectId]/route.ts b/app/api/projects/[projectId]/route.ts index ae6a4ef..b4eb008 100644 --- a/app/api/projects/[projectId]/route.ts +++ b/app/api/projects/[projectId]/route.ts @@ -103,7 +103,7 @@ export async function PATCH(request: NextRequest, { params }: RouteParams) { select: { id: true, ownerId: true, workspaceId: true, visibility: true }, }); const access = projectAccessTarget - ? await checkProjectAccess(projectAccessTarget, session.user.id, { intent: 'manage' }) + ? await checkProjectAccess(projectAccessTarget, session.user.id) : null; if (!access?.canEdit) { return apiErrors.forbidden('Access denied'); @@ -181,7 +181,7 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'delete' }); + const access = await checkProjectAccess(project, session.user.id); if (!access.canDelete) { return apiErrors.forbidden('Only the project owner can delete it'); } diff --git a/app/api/projects/[projectId]/tags/[tagId]/route.ts b/app/api/projects/[projectId]/tags/[tagId]/route.ts index d53ad9b..e3bdf88 100644 --- a/app/api/projects/[projectId]/tags/[tagId]/route.ts +++ b/app/api/projects/[projectId]/tags/[tagId]/route.ts @@ -28,7 +28,7 @@ export async function PATCH(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Access denied'); } @@ -98,7 +98,7 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Access denied'); } diff --git a/app/api/projects/[projectId]/tags/route.ts b/app/api/projects/[projectId]/tags/route.ts index 6c5ba61..c35fd5b 100644 --- a/app/api/projects/[projectId]/tags/route.ts +++ b/app/api/projects/[projectId]/tags/route.ts @@ -97,7 +97,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Access denied'); } diff --git a/app/api/projects/[projectId]/videos/[videoId]/route.ts b/app/api/projects/[projectId]/videos/[videoId]/route.ts index f01d0d0..2ec011c 100644 --- a/app/api/projects/[projectId]/videos/[videoId]/route.ts +++ b/app/api/projects/[projectId]/videos/[videoId]/route.ts @@ -170,7 +170,7 @@ export async function PATCH(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Video'); } - const access = await checkProjectAccess(video.project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(video.project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Access denied'); } @@ -251,7 +251,7 @@ export async function DELETE(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Video'); } - const access = await checkProjectAccess(video.project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(video.project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Only project owner or admin can delete videos'); } diff --git a/app/api/projects/[projectId]/videos/[videoId]/versions/[versionId]/route.ts b/app/api/projects/[projectId]/videos/[videoId]/versions/[versionId]/route.ts index 9683386..91f12c3 100644 --- a/app/api/projects/[projectId]/videos/[videoId]/versions/[versionId]/route.ts +++ b/app/api/projects/[projectId]/videos/[videoId]/versions/[versionId]/route.ts @@ -32,7 +32,7 @@ async function getVersionWithAccess( } const project = version.video.project; - const access = await checkProjectAccess(project, userId, { intent: 'manage' }); + const access = await checkProjectAccess(project, userId); return { version, canEdit: access.canEdit, isOwner: access.isOwner }; } diff --git a/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts b/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts index 2ab73ea..771efb4 100644 --- a/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts +++ b/app/api/projects/[projectId]/videos/[videoId]/versions/route.ts @@ -74,7 +74,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Video'); } - const access = await checkProjectAccess(video.project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(video.project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Access denied'); } diff --git a/app/api/projects/[projectId]/videos/bulk-delete/route.ts b/app/api/projects/[projectId]/videos/bulk-delete/route.ts index e82456f..97eb2c5 100644 --- a/app/api/projects/[projectId]/videos/bulk-delete/route.ts +++ b/app/api/projects/[projectId]/videos/bulk-delete/route.ts @@ -5,7 +5,7 @@ import { db } from '@/lib/db'; import { logCleanupWarnings } from '@/lib/cleanup-warnings'; import { logError } from '@/lib/logger'; import { rateLimit } from '@/lib/rate-limit'; -import { deleteProjectVideosWithCleanup } from '@/lib/video-delete'; +import { deleteProjectVideosWithCleanup, VideoStorageCleanupError } from '@/lib/video-delete'; type RouteParams = { params: Promise<{ projectId: string }> }; @@ -32,7 +32,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Only project owner or admin can delete videos'); } @@ -59,6 +59,17 @@ export async function POST(request: NextRequest, { params }: RouteParams) { if (error instanceof Error && error.message === 'VIDEO_NOT_FOUND') { return apiErrors.badRequest('One or more selected videos do not belong to this project'); } + // Storage refused a delete, so nothing was removed and the videos are still there. + // Saying so lets the caller retry, which is the whole point of leaving the rows. + if (error instanceof VideoStorageCleanupError) { + logCleanupWarnings( + { entityType: 'video', entityId: `bulk:${normalizedIds.join(',')}` }, + error.cleanupInput + ); + return apiErrors.internalError( + 'Could not delete the stored media for these videos. Nothing was deleted; please try again.' + ); + } throw error; } diff --git a/app/api/projects/[projectId]/videos/bunny-init/route.ts b/app/api/projects/[projectId]/videos/bunny-init/route.ts index 638d006..2eec2c9 100644 --- a/app/api/projects/[projectId]/videos/bunny-init/route.ts +++ b/app/api/projects/[projectId]/videos/bunny-init/route.ts @@ -27,7 +27,7 @@ async function getProjectWithEditAccess(projectId: string, userId: string) { if (!project) return null; - const access = await checkProjectAccess(project, userId, { intent: 'manage' }); + const access = await checkProjectAccess(project, userId); const canEdit = access.canEdit; if (!canEdit) return null; diff --git a/app/api/projects/[projectId]/videos/move/route.ts b/app/api/projects/[projectId]/videos/move/route.ts index 0dd62e2..ab82f7f 100644 --- a/app/api/projects/[projectId]/videos/move/route.ts +++ b/app/api/projects/[projectId]/videos/move/route.ts @@ -38,7 +38,7 @@ export async function GET(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, userId, { intent: 'manage' }); + const access = await checkProjectAccess(project, userId); if (!access.canEdit) { return apiErrors.forbidden('Access denied'); } @@ -141,8 +141,8 @@ export async function POST(request: NextRequest, { params }: RouteParams) { } const [sourceAccess, targetAccess] = await Promise.all([ - checkProjectAccess(sourceProject, userId, { intent: 'manage' }), - checkProjectAccess(targetProject, userId, { intent: 'manage' }), + checkProjectAccess(sourceProject, userId), + checkProjectAccess(targetProject, userId), ]); if (!sourceAccess.canEdit) { return apiErrors.forbidden('You cannot move videos out of this project'); diff --git a/app/api/projects/[projectId]/videos/r2-complete/route.ts b/app/api/projects/[projectId]/videos/r2-complete/route.ts index 9d1c93f..d9d416a 100644 --- a/app/api/projects/[projectId]/videos/r2-complete/route.ts +++ b/app/api/projects/[projectId]/videos/r2-complete/route.ts @@ -26,7 +26,7 @@ async function getProjectWithEditAccess(projectId: string, userId: string) { if (!project) return null; - const access = await checkProjectAccess(project, userId, { intent: 'manage' }); + const access = await checkProjectAccess(project, userId); if (!access.canEdit) return null; return project; diff --git a/app/api/projects/[projectId]/videos/r2-init/route.ts b/app/api/projects/[projectId]/videos/r2-init/route.ts index ad2d2f5..1adaf78 100644 --- a/app/api/projects/[projectId]/videos/r2-init/route.ts +++ b/app/api/projects/[projectId]/videos/r2-init/route.ts @@ -58,7 +58,7 @@ async function getProjectWithEditAccess(projectId: string, userId: string) { if (!project) return null; - const access = await checkProjectAccess(project, userId, { intent: 'manage' }); + const access = await checkProjectAccess(project, userId); if (!access.canEdit) return null; return project; diff --git a/app/api/projects/[projectId]/videos/route.ts b/app/api/projects/[projectId]/videos/route.ts index 86ce0fa..a56fa73 100644 --- a/app/api/projects/[projectId]/videos/route.ts +++ b/app/api/projects/[projectId]/videos/route.ts @@ -83,7 +83,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { return apiErrors.notFound('Project'); } - const access = await checkProjectAccess(project, session.user.id, { intent: 'manage' }); + const access = await checkProjectAccess(project, session.user.id); if (!access.canEdit) { return apiErrors.forbidden('Access denied'); } diff --git a/app/api/projects/route.ts b/app/api/projects/route.ts index fa70378..3c28ae0 100644 --- a/app/api/projects/route.ts +++ b/app/api/projects/route.ts @@ -42,22 +42,15 @@ export async function GET(request: NextRequest) { return apiErrors.badRequest('Invalid page range. Offset must be 10000 or less.'); } - // Build base filter: user is owner OR a member + // Build base filter: user is the project owner, a project member, or a member of the + // workspace the project lives in. The third branch used to be dropped whenever a + // workspaceId was supplied, so filtering by their own workspace showed a workspace + // member an empty list while the unfiltered call returned the same project. const baseFilter: Record = { OR: [ { ownerId: session.user.id }, { members: { some: { userId: session.user.id } } }, - // Also include projects in workspaces where the user is a workspace member - ...(workspaceId - ? [] - : [ - { - workspace: { - owner: buildBillingAccessWhereInput(), - members: { some: { userId: session.user.id } }, - }, - }, - ]), + { workspace: { members: { some: { userId: session.user.id } } } }, ], workspace: { owner: buildBillingAccessWhereInput(), diff --git a/app/api/search/route.ts b/app/api/search/route.ts index 6aa12e0..4e55745 100644 --- a/app/api/search/route.ts +++ b/app/api/search/route.ts @@ -2,6 +2,7 @@ import { NextRequest } from 'next/server'; import { db } from '@/lib/db'; import { auth } from '@/lib/auth'; import { apiErrors, successResponse } from '@/lib/api-response'; +import { buildBillingAccessWhereInput } from '@/lib/billing'; import { checkRateLimit, rateLimitHeaders, RATE_LIMIT_CONFIGS } from '@/lib/rate-limit'; import { logError } from '@/lib/logger'; @@ -38,8 +39,15 @@ export async function GET(request: NextRequest) { return apiErrors.badRequest('Query too long.'); } - // Access filter reused across queries + // Access filter reused across queries. The billing condition is the same one every + // other read path carries (GET /api/projects, checkProjectAccess): without it search + // kept returning project names, descriptions and video titles for a tenant whose + // access had otherwise been cut off, which is a lapsed-tenant surface no other read + // path leaves open. + const ownerWithBillingAccess = buildBillingAccessWhereInput(); + const projectAccessFilter = { + workspace: { owner: ownerWithBillingAccess }, OR: [ { ownerId: userId }, { members: { some: { userId } } }, @@ -48,6 +56,7 @@ export async function GET(request: NextRequest) { }; const workspaceAccessFilter = { + owner: ownerWithBillingAccess, OR: [{ ownerId: userId }, { members: { some: { userId } } }], }; diff --git a/app/api/versions/[versionId]/approvals/route.ts b/app/api/versions/[versionId]/approvals/route.ts index 63f0134..c7d8af5 100644 --- a/app/api/versions/[versionId]/approvals/route.ts +++ b/app/api/versions/[versionId]/approvals/route.ts @@ -84,9 +84,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { }); if (!version) return apiErrors.notFound('Version'); - const access = await checkProjectAccess(version.video.project, session.user.id, { - intent: 'manage', - }); + const access = await checkProjectAccess(version.video.project, session.user.id); if (!access.canEdit) return apiErrors.forbidden('Access denied'); const body = (await request.json().catch(() => ({}))) as { diff --git a/app/api/versions/[versionId]/download/route.ts b/app/api/versions/[versionId]/download/route.ts index e66ad15..bebe967 100644 --- a/app/api/versions/[versionId]/download/route.ts +++ b/app/api/versions/[versionId]/download/route.ts @@ -308,6 +308,15 @@ export async function GET(request: NextRequest, { params }: RouteParams) { const canDownloadViaShareLink = shareAccess.hasAccess && shareAccess.canDownload; const canDownloadViaMembership = canDownloadProjectMedia(version.video.project, access); if (!canDownloadViaMembership && !canDownloadViaShareLink) { + // A caller with no relationship to the project at all is told the version does not + // exist, matching the comment export route: answering 403 for an id belonging to + // another tenant confirms that the id exists. Anyone who does have a relationship, + // including an owner whose billing has lapsed, already knows it exists and gets the + // more informative 403. + const belongsToProject = access.isOwner || access.isProjectMember || access.isWorkspaceMember; + if (!belongsToProject && !shareAccess.hasAccess) { + return apiErrors.notFound('Version'); + } return apiErrors.forbidden('Access denied'); } diff --git a/app/api/videos/[videoId]/assets/[assetId]/download/route.ts b/app/api/videos/[videoId]/assets/[assetId]/download/route.ts index 2e887ed..82d2cc8 100644 --- a/app/api/videos/[videoId]/assets/[assetId]/download/route.ts +++ b/app/api/videos/[videoId]/assets/[assetId]/download/route.ts @@ -84,7 +84,15 @@ export async function GET(request: NextRequest, { params }: RouteParams) { const { videoId, assetId } = await params; const context = await getVideoAssetAccessContext(request, videoId, 'VIEW'); if (!context) return apiErrors.notFound('Video'); - if (!context.hasViewAccess) return apiErrors.forbidden('Access denied'); + // A caller with no relationship to the project is told the video does not exist. + // Answering 403 for an id belonging to another tenant confirms that the id exists, + // and the comment export route already answers 404 for the identical shape. Somebody + // who does belong, including an owner whose billing lapsed, gets the 403. + if (!context.hasViewAccess) { + return context.viewerBelongsToProject + ? apiErrors.forbidden('Access denied') + : apiErrors.notFound('Video'); + } if (!context.canDownloadAssets) { return apiErrors.forbidden('Downloads are disabled for this project'); } diff --git a/bun.lock b/bun.lock index 7ee37c6..2923b5f 100644 --- a/bun.lock +++ b/bun.lock @@ -62,7 +62,6 @@ "shadcn": "^3.8.3", "tailwindcss": "^4", "typescript": "^5", - "vite-tsconfig-paths": "^6.1.1", "vitest": "^4.1.10", }, }, @@ -1315,8 +1314,6 @@ "globalthis": ["globalthis@1.0.4", "", { "dependencies": { "define-properties": "^1.2.1", "gopd": "^1.0.1" } }, "sha512-DpLKbNU4WylpxJykQujfCcwYWiV/Jhm50Goo0wrVILAv5jOr9d+H+UR3PhSCD2rCCEIg0uc+G+muBTwD54JhDQ=="], - "globrex": ["globrex@0.1.2", "", {}, "sha512-uHJgbwAMwNFf5mLst7IWLNg14x1CkeqglJb/K3doi4dw6q2IvAAmM/Y81kevy83wP+Sst+nutFTYOGg3d1lsxg=="], - "gopd": ["gopd@1.2.0", "", {}, "sha512-ZUKRh6/kUFoAiTAtTYPZJ3hw9wNxx+BIBOijnlG9PnrJsCcSjs1wyyD6vJpaYtgnzDrKYRSqf3OO6Rfa93xsRg=="], "graceful-fs": ["graceful-fs@4.2.11", "", {}, "sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ=="], @@ -2083,8 +2080,6 @@ "ts-morph": ["ts-morph@26.0.0", "", { "dependencies": { "@ts-morph/common": "~0.27.0", "code-block-writer": "^13.0.3" } }, "sha512-ztMO++owQnz8c/gIENcM9XfCEzgoGphTv+nKpYNM1bgsdOVC/jRZuEBf6N+mLLDNg68Kl+GgUZfOySaRiG1/Ug=="], - "tsconfck": ["tsconfck@3.1.6", "", { "peerDependencies": { "typescript": "^5.0.0" }, "optionalPeers": ["typescript"], "bin": { "tsconfck": "bin/tsconfck.js" } }, "sha512-ks6Vjr/jEw0P1gmOVwutM3B7fWxoWBL2KRDb1JfqGVawBmO5UsvmWOQFGHBPl5yxYz4eERr19E6L7NMv+Fej4w=="], - "tsconfig-paths": ["tsconfig-paths@4.2.0", "", { "dependencies": { "json5": "^2.2.2", "minimist": "^1.2.6", "strip-bom": "^3.0.0" } }, "sha512-NoZ4roiN7LnbKn9QqE1amc9DJfzvZXxF4xDavcOWt1BPkdx+m+0gJuPM+S0vCe7zTJMYUP0R8pO2XMr+Y8oLIg=="], "tslib": ["tslib@2.8.1", "", {}, "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w=="], @@ -2157,8 +2152,6 @@ "vite": ["vite@8.1.5", "", { "dependencies": { "lightningcss": "^1.32.0", "picomatch": "^4.0.5", "postcss": "^8.5.17", "rolldown": "~1.1.5", "tinyglobby": "^0.2.17" }, "optionalDependencies": { "fsevents": "~2.3.3" }, "peerDependencies": { "@types/node": "^20.19.0 || >=22.12.0", "@vitejs/devtools": "^0.3.0", "esbuild": "^0.27.0 || ^0.28.0", "jiti": ">=1.21.0", "less": "^4.0.0", "sass": "^1.70.0", "sass-embedded": "^1.70.0", "stylus": ">=0.54.8", "sugarss": "^5.0.0", "terser": "^5.16.0", "tsx": "^4.8.1", "yaml": "^2.4.2" }, "optionalPeers": ["@types/node", "@vitejs/devtools", "esbuild", "jiti", "less", "sass", "sass-embedded", "stylus", "sugarss", "terser", "tsx", "yaml"], "bin": { "vite": "bin/vite.js" } }, "sha512-7ULLwsCdYx/nRyrpiEwvqb5TFHrMVZyBt+rg/OAXT7rgj/z+DtTDyKFeLAdDkubDVDKD8jOsndmy7m55XcfUsw=="], - "vite-tsconfig-paths": ["vite-tsconfig-paths@6.1.1", "", { "dependencies": { "debug": "^4.1.1", "globrex": "^0.1.2", "tsconfck": "^3.0.3" }, "peerDependencies": { "vite": "*" } }, "sha512-2cihq7zliibCCZ8P9cKJrQBkfgdvcFkOOc3Y02o3GWUDLgqjWsZudaoiuOwO/gzTzy17cS5F7ZPo4bsnS4DGkg=="], - "vitest": ["vitest@4.1.10", "", { "dependencies": { "@vitest/expect": "4.1.10", "@vitest/mocker": "4.1.10", "@vitest/pretty-format": "4.1.10", "@vitest/runner": "4.1.10", "@vitest/snapshot": "4.1.10", "@vitest/spy": "4.1.10", "@vitest/utils": "4.1.10", "es-module-lexer": "^2.0.0", "expect-type": "^1.3.0", "magic-string": "^0.30.21", "obug": "^2.1.1", "pathe": "^2.0.3", "picomatch": "^4.0.3", "std-env": "^4.0.0-rc.1", "tinybench": "^2.9.0", "tinyexec": "^1.0.2", "tinyglobby": "^0.2.15", "tinyrainbow": "^3.1.0", "vite": "^6.0.0 || ^7.0.0 || ^8.0.0", "why-is-node-running": "^2.3.0" }, "peerDependencies": { "@edge-runtime/vm": "*", "@opentelemetry/api": "^1.9.0", "@types/node": "^20.0.0 || ^22.0.0 || >=24.0.0", "@vitest/browser-playwright": "4.1.10", "@vitest/browser-preview": "4.1.10", "@vitest/browser-webdriverio": "4.1.10", "@vitest/coverage-istanbul": "4.1.10", "@vitest/coverage-v8": "4.1.10", "@vitest/ui": "4.1.10", "happy-dom": "*", "jsdom": "*" }, "optionalPeers": ["@edge-runtime/vm", "@opentelemetry/api", "@types/node", "@vitest/browser-playwright", "@vitest/browser-preview", "@vitest/browser-webdriverio", "@vitest/coverage-istanbul", "@vitest/coverage-v8", "@vitest/ui", "happy-dom", "jsdom"], "bin": { "vitest": "./vitest.mjs" } }, "sha512-R9jUTe5S4Qb0HCd4TNqpC7oGcrMssMRGXLW80ubjWsW9VH5GF8y1Y0SFLY9AbqSk6nt0PnOx4H4WNJYZ13GUPw=="], "w3c-xmlserializer": ["w3c-xmlserializer@5.0.0", "", { "dependencies": { "xml-name-validator": "^5.0.0" } }, "sha512-o8qghlI8NZHU1lLPrpi2+Uq7abh4GGPpYANlalzWxyWteJOCsr/P+oPBA49TOLu5FTZO4d3F9MnWJfiMo4BkmA=="], diff --git a/components/guest-gate.tsx b/components/guest-gate.tsx index 68e58a4..9bc52bf 100644 --- a/components/guest-gate.tsx +++ b/components/guest-gate.tsx @@ -26,8 +26,10 @@ export function GuestGate({ children }: { children: ReactNode }) { } const confirm = () => { + // The length check the input's own maxLength={100} already enforces is gone: it was + // unreachable, and an unreachable guard reads as protection that is not there. const trimmed = guestName.trim(); - if (!trimmed || trimmed.length > 100) return; + if (!trimmed) return; localStorage.setItem('openframe_guest_name', trimmed); setConfirmed(true); }; @@ -45,7 +47,11 @@ export function GuestGate({ children }: { children: ReactNode }) {

+ -
diff --git a/components/members-management-page.tsx b/components/members-management-page.tsx index adae4a1..8c3c29a 100644 --- a/components/members-management-page.tsx +++ b/components/members-management-page.tsx @@ -105,6 +105,11 @@ export function MembersManagementPage({ router.push('/dashboard'); return; } + // Any other status has to say so. Returning silently rendered "No members yet" + // and "No pending invitations" on a workspace that has both, and the user's next + // move was to re-invite somebody who is already a member, which answers 409 and + // reads as a second, unrelated bug. + setError('Failed to load members. Please refresh to try again.'); return; } const data = await res.json(); diff --git a/components/share-link-unlock.tsx b/components/share-link-unlock.tsx index 13b71ba..8c9af37 100644 --- a/components/share-link-unlock.tsx +++ b/components/share-link-unlock.tsx @@ -59,7 +59,11 @@ export function ShareLinkUnlock({ videoId }: ShareLinkUnlockProps) {
+ {error &&

{error}

} diff --git a/components/video-page-content.tsx b/components/video-page-content.tsx index be15eff..bfa52e6 100644 --- a/components/video-page-content.tsx +++ b/components/video-page-content.tsx @@ -163,7 +163,7 @@ export function VideoPageContent({ assets, isLoadingAssets, isCreatingAsset, - activeDeleteAssetId, + deletingAssetIds, activeDownloadAssetId, hasMoreAssets, isLoadingMoreAssets, @@ -908,7 +908,7 @@ export function VideoPageContent({ assets={assets} isLoadingAssets={isLoadingAssets} isCreatingAsset={isCreatingAsset} - activeDeleteAssetId={activeDeleteAssetId} + deletingAssetIds={deletingAssetIds} activeDownloadAssetId={activeDownloadAssetId} canUploadAssets={canUploadAssets} canDownloadAssets={canDownloadAssets} diff --git a/components/video-page/asset-list-section.tsx b/components/video-page/asset-list-section.tsx index b9dcecb..1994fc5 100644 --- a/components/video-page/asset-list-section.tsx +++ b/components/video-page/asset-list-section.tsx @@ -20,7 +20,7 @@ interface AssetListSectionProps { bunnyProcessingByAssetId: Record; bunnyReadyByAssetId: Record; activeDownloadAssetId: string | null; - activeDeleteAssetId: string | null; + deletingAssetIds: string[]; canDownloadAssets: boolean; hasMoreAssets: boolean; isLoadingMoreAssets: boolean; @@ -38,7 +38,7 @@ export const AssetListSection = memo(function AssetListSection({ bunnyProcessingByAssetId, bunnyReadyByAssetId, activeDownloadAssetId, - activeDeleteAssetId, + deletingAssetIds, canDownloadAssets, hasMoreAssets, isLoadingMoreAssets, @@ -186,10 +186,10 @@ export const AssetListSection = memo(function AssetListSection({ className="h-7 w-7" title="Delete asset" aria-label="Delete asset" - disabled={activeDeleteAssetId === asset.id} + disabled={deletingAssetIds.includes(asset.id)} onClick={() => onDeleteAsset(asset.id)} > - {activeDeleteAssetId === asset.id ? ( + {deletingAssetIds.includes(asset.id) ? ( ) : ( diff --git a/components/video-page/assets-pane.tsx b/components/video-page/assets-pane.tsx index 253db25..521f220 100644 --- a/components/video-page/assets-pane.tsx +++ b/components/video-page/assets-pane.tsx @@ -82,7 +82,7 @@ interface AssetsPaneProps { assets: VideoAsset[]; isLoadingAssets: boolean; isCreatingAsset: boolean; - activeDeleteAssetId: string | null; + deletingAssetIds: string[]; activeDownloadAssetId: string | null; canUploadAssets: boolean; canDownloadAssets: boolean; @@ -112,7 +112,7 @@ export const AssetsPane = memo(function AssetsPane({ assets, isLoadingAssets, isCreatingAsset, - activeDeleteAssetId, + deletingAssetIds, activeDownloadAssetId, canUploadAssets, canDownloadAssets, @@ -1518,7 +1518,7 @@ export const AssetsPane = memo(function AssetsPane({ bunnyProcessingByAssetId={bunnyProcessingByAssetId} bunnyReadyByAssetId={bunnyReadyByAssetId} activeDownloadAssetId={activeDownloadAssetId} - activeDeleteAssetId={activeDeleteAssetId} + deletingAssetIds={deletingAssetIds} canDownloadAssets={canDownloadAssets} hasMoreAssets={hasMoreAssets} isLoadingMoreAssets={isLoadingMoreAssets} diff --git a/components/video-page/comment-rich-text.tsx b/components/video-page/comment-rich-text.tsx index 6e41775..38512b6 100644 --- a/components/video-page/comment-rich-text.tsx +++ b/components/video-page/comment-rich-text.tsx @@ -13,13 +13,16 @@ interface CommentRichTextProps { assets?: VideoAsset[]; } -function renderUrls(text: string): React.ReactNode[] { +// `keyPrefix` scopes the indices to this slice. The function runs once per gap between +// mentions, so keying on the index alone emitted `txt-0` for several siblings and React +// warned about duplicate keys. +function renderUrls(text: string, keyPrefix: string): React.ReactNode[] { const parts = text.split(URL_REGEX); return parts.map((part, index) => { if (/^https?:\/\/[^\s]+$/.test(part)) { return ( ); } - return {part}; + return {part}; }); } @@ -43,7 +46,7 @@ export function CommentRichText({ text, onAssetMentionClick, assets = [] }: Comm if (mentionIndex < 0) continue; if (mentionIndex > lastIndex) { - nodes.push(...renderUrls(text.slice(lastIndex, mentionIndex))); + nodes.push(...renderUrls(text.slice(lastIndex, mentionIndex), `s${lastIndex}`)); } const fallbackLabel = match[1] || 'asset'; @@ -88,7 +91,7 @@ export function CommentRichText({ text, onAssetMentionClick, assets = [] }: Comm } if (lastIndex < text.length) { - nodes.push(...renderUrls(text.slice(lastIndex))); + nodes.push(...renderUrls(text.slice(lastIndex), `s${lastIndex}`)); } return <>{nodes}; diff --git a/components/video-page/comments-pane.tsx b/components/video-page/comments-pane.tsx index 2ebaf7a..36ca8f7 100644 --- a/components/video-page/comments-pane.tsx +++ b/components/video-page/comments-pane.tsx @@ -66,8 +66,8 @@ interface CommentsPaneProps { setEditingCommentId: (id: string | null) => void; editText: string; setEditText: (value: string) => void; - editTagId: string | null; - setEditTagId: (value: string | null) => void; + editTagId: string | null | undefined; + setEditTagId: (value: string | null | undefined) => void; setEditAnnotationData: (value: string | null | undefined) => void; setIsEditingAnnotation: (value: boolean) => void; onStartEditAnnotation: () => void; @@ -488,7 +488,7 @@ export const CommentsPane = memo(function CommentsPane({ if (e.key === 'Escape') { setEditingCommentId(null); setEditText(''); - setEditTagId(null); + setEditTagId(undefined); setEditAnnotationData(undefined); setIsEditingAnnotation(false); } @@ -513,7 +513,7 @@ export const CommentsPane = memo(function CommentsPane({ onClick={() => { setEditingCommentId(null); setEditText(''); - setEditTagId(null); + setEditTagId(undefined); setEditAnnotationData(undefined); setIsEditingAnnotation(false); }} @@ -726,6 +726,9 @@ export const CommentsPane = memo(function CommentsPane({ onClick={() => { setEditingCommentId(reply.id); setEditText(reply.content || ''); + // No tag picker on a reply: undefined keeps + // the PATCH from carrying a tagId at all. + setEditTagId(undefined); }} > diff --git a/components/video-page/guest-name-gate.tsx b/components/video-page/guest-name-gate.tsx index ca08e4d..bac8286 100644 --- a/components/video-page/guest-name-gate.tsx +++ b/components/video-page/guest-name-gate.tsx @@ -30,7 +30,11 @@ export const GuestNameGate = memo(function GuestNameGate({

+ setGuestName(e.target.value)} diff --git a/components/video-page/hooks/use-comment-actions.ts b/components/video-page/hooks/use-comment-actions.ts index 959db55..fa89ddf 100644 --- a/components/video-page/hooks/use-comment-actions.ts +++ b/components/video-page/hooks/use-comment-actions.ts @@ -116,7 +116,12 @@ export function useCommentActions({ const [editingCommentId, setEditingCommentId] = useState(null); const [editText, setEditText] = useState(''); - const [editTagId, setEditTagId] = useState(null); + // `undefined` means "this editor does not manage a tag", which is the reply editor: + // replies have no tag picker. `null` means "no tag", which the comment editor seeds + // from the comment itself. Initialising to `null` made `editTagId !== undefined` always + // true, so editing a reply's text sent `tagId: null` and cleared its tag, or sent a + // stale value left over from a previous edit. + const [editTagId, setEditTagId] = useState(undefined); const [editAnnotationData, setEditAnnotationData] = useState( undefined ); @@ -569,6 +574,11 @@ export function useCommentActions({ if (!activeVersionId) return; isMutatingRef.current = true; + // The optimistic flip, the request body and the rollback all derive from the same + // value. Flipping relative to the row (`!c.isResolved`) while the body and the + // rollback came from `currentlyResolved` meant a failed request could leave the + // comment in a state it was never in whenever the two disagreed. + const nextResolved = !currentlyResolved; setVideo((prev) => { if (!prev) return prev; return { @@ -578,7 +588,7 @@ export function useCommentActions({ ? { ...v, comments: v.comments.map((c) => - c.id === commentId ? { ...c, isResolved: !c.isResolved } : c + c.id === commentId ? { ...c, isResolved: nextResolved } : c ), } : v @@ -590,7 +600,7 @@ export function useCommentActions({ const res = await fetch(`/api/comments/${commentId}`, { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ isResolved: !currentlyResolved }), + body: JSON.stringify({ isResolved: nextResolved }), }); if (!res.ok) { @@ -1027,7 +1037,7 @@ export function useCommentActions({ }); setEditingCommentId(null); setEditText(''); - setEditTagId(null); + setEditTagId(undefined); setEditAnnotationData(undefined); setIsEditingAnnotation(false); if (finalAnnotationData !== undefined && finalAnnotationData) { @@ -1070,9 +1080,17 @@ export function useCommentActions({ isMutatingRef.current = true; + // The snapshot is taken once, however many times React runs the updater. React is + // free to invoke an updater more than once (StrictMode, and React 19 retries a + // render that threw), and a second run would otherwise capture the post-delete + // state, turning a failed delete into a silent confirmation of it. const previousVideoRef: { current: VideoData | null } = { current: null }; + let capturedSnapshot = false; setVideo((prev) => { - previousVideoRef.current = prev; + if (!capturedSnapshot) { + previousVideoRef.current = prev; + capturedSnapshot = true; + } if (!prev) return prev; return { ...prev, diff --git a/components/video-page/hooks/use-download-actions.ts b/components/video-page/hooks/use-download-actions.ts index 5ab8aba..21fdd68 100644 --- a/components/video-page/hooks/use-download-actions.ts +++ b/components/video-page/hooks/use-download-actions.ts @@ -1,6 +1,6 @@ 'use client'; -import { useCallback, useState } from 'react'; +import { useCallback, useRef, useState } from 'react'; import { toast } from 'sonner'; import type { BunnyDownloadPreference, @@ -70,10 +70,14 @@ interface UseDownloadActionsParams { export function useDownloadActions({ activeVersion, video }: UseDownloadActionsParams) { const [activeDownloadTarget, setActiveDownloadTarget] = useState(null); const isDownloadingVideo = activeDownloadTarget !== null; + // The guard reads a ref, not the state. Two calls originating in the same render both + // saw the old state value and both proceeded, so a fast double-click downloaded the + // file twice. + const isDownloadingRef = useRef(false); const startDownload = useCallback( async (preference: BunnyDownloadPreference = 'compressed') => { - if (!activeVersion || !video || isDownloadingVideo) return; + if (!activeVersion || !video || isDownloadingRef.current) return; if (!video.canDownload) { toast.error('Download is disabled for this shared link'); return; @@ -88,6 +92,7 @@ export function useDownloadActions({ activeVersion, video }: UseDownloadActionsP } const target: DownloadTarget = activeVersion.providerId === 'bunny' ? preference : 'direct'; + isDownloadingRef.current = true; setActiveDownloadTarget(target); let progressToast: DownloadProgressToastHandle | null = null; try { @@ -191,10 +196,11 @@ export function useDownloadActions({ activeVersion, video }: UseDownloadActionsP toast.error('Failed to start download'); } } finally { + isDownloadingRef.current = false; setActiveDownloadTarget(null); } }, - [activeVersion, video, isDownloadingVideo] + [activeVersion, video] ); return { diff --git a/components/video-page/hooks/use-version-actions.ts b/components/video-page/hooks/use-version-actions.ts index cdf6967..ae8de97 100644 --- a/components/video-page/hooks/use-version-actions.ts +++ b/components/video-page/hooks/use-version-actions.ts @@ -13,6 +13,11 @@ import type { VersionActionsConfig, VideoData } from '@/components/video-page/ty import { resolvePublicBunnyCdnHostname } from '@/lib/bunny-cdn'; import { cleanupPendingR2VideoUpload, uploadVideoToR2 } from '@/lib/client/r2-video-upload'; +/** What a failed version upload has to undo, depending on which provider it started on. */ +type PendingVersionCleanup = + | { objectKey: string; uploadToken: string; reservationId: string | null } + | { bunnyVideoId: string; uploadToken: string }; + interface UseVersionActionsParams extends VersionActionsConfig { setVideo: Dispatch>; activeVersionId: string | null; @@ -60,7 +65,18 @@ export function useVersionActions({ } }; - const uploadNewVersionFile = async (file: File, title: string) => { + /** + * `onPendingCleanup` is called the moment there is something to clean up, which for a + * Bunny upload is when bunny-init answers and the remote video already exists. Waiting + * for this function to return instead meant a tus `onError` threw past the assignment, + * so every failed Bunny upload left a video behind on the Bunny side: billed, and + * invisible in the app. + */ + const uploadNewVersionFile = async ( + file: File, + title: string, + onPendingCleanup: (cleanup: PendingVersionCleanup) => void + ) => { if (!projectId) throw new Error('Missing project'); if (directUploadProvider === 'r2') { @@ -102,6 +118,9 @@ export function useVersionActions({ data: { videoId: bunnyVideoId, libraryId, signature, expirationTime, uploadToken }, } = await initRes.json(); + // The remote video exists from here on, so it is cleanable from here on. + onPendingCleanup({ bunnyVideoId, uploadToken }); + await new Promise((resolve, reject) => { setNewVersionUploadStatus('Uploading video...'); const upload = new tus.Upload(file, { @@ -154,17 +173,7 @@ export function useVersionActions({ setIsCreatingVersion(true); setNewVersionUploadStatus(''); setNewVersionUploadProgress(0); - let pendingCleanup: - | { - objectKey: string; - uploadToken: string; - reservationId: string | null; - } - | { - bunnyVideoId: string; - uploadToken: string; - } - | null = null; + let pendingCleanup: PendingVersionCleanup | null = null; try { let finalVideoUrl = ''; @@ -194,7 +203,9 @@ export function useVersionActions({ title = title.replace(/\.[^/.]+$/, ''); } - const uploaded = await uploadNewVersionFile(newVersionFile, title); + const uploaded = await uploadNewVersionFile(newVersionFile, title, (cleanup) => { + pendingCleanup = cleanup; + }); finalVideoUrl = uploaded.finalVideoUrl; finalProviderId = uploaded.finalProviderId; finalProviderVideoId = uploaded.finalProviderVideoId; diff --git a/components/video-page/hooks/use-video-assets.ts b/components/video-page/hooks/use-video-assets.ts index 20d0fe0..5442fcc 100644 --- a/components/video-page/hooks/use-video-assets.ts +++ b/components/video-page/hooks/use-video-assets.ts @@ -57,13 +57,18 @@ export function useVideoAssets({ const [assets, setAssets] = useState([]); const [isLoadingAssets, setIsLoadingAssets] = useState(true); const [isCreatingAsset, setIsCreatingAsset] = useState(false); - const [activeDeleteAssetId, setActiveDeleteAssetId] = useState(null); + // A set, not a single slot. Two overlapping deletes used to clear each other's + // spinner, so the first one stopped indicating progress while it was still running. + const [deletingAssetIds, setDeletingAssetIds] = useState([]); const [activeDownloadAssetId, setActiveDownloadAssetId] = useState(null); const [hasMoreAssets, setHasMoreAssets] = useState(false); const [nextAssetsOffset, setNextAssetsOffset] = useState(0); const [isLoadingMoreAssets, setIsLoadingMoreAssets] = useState(false); const assetsEtagRef = useRef(null); const isMutatingRef = useRef(false); + // The double-call guard reads a ref, not the state: two calls originating in the same + // render both saw the old state value and both fetched the next page. + const isLoadingMoreRef = useRef(false); const fetchAssets = useCallback( async (options?: { useEtag?: boolean; silent?: boolean }) => { @@ -106,7 +111,8 @@ export function useVideoAssets({ ); const loadMoreAssets = useCallback(async () => { - if (isLoadingMoreAssets || !hasMoreAssets) return; + if (isLoadingMoreRef.current || !hasMoreAssets) return; + isLoadingMoreRef.current = true; setIsLoadingMoreAssets(true); try { const res = await fetch( @@ -129,9 +135,10 @@ export function useVideoAssets({ } catch { toast.error('Failed to load more assets'); } finally { + isLoadingMoreRef.current = false; setIsLoadingMoreAssets(false); } - }, [hasMoreAssets, isLoadingMoreAssets, nextAssetsOffset, videoId]); + }, [hasMoreAssets, nextAssetsOffset, videoId]); useEffect(() => { void fetchAssets({ useEtag: true }); @@ -198,7 +205,7 @@ export function useVideoAssets({ const deleteAsset = useCallback( async (assetId: string) => { - setActiveDeleteAssetId(assetId); + setDeletingAssetIds((prev) => (prev.includes(assetId) ? prev : [...prev, assetId])); isMutatingRef.current = true; try { const res = await fetch(`/api/videos/${videoId}/assets/${assetId}`, { @@ -217,7 +224,7 @@ export function useVideoAssets({ toast.error('Failed to delete asset'); return false; } finally { - setActiveDeleteAssetId(null); + setDeletingAssetIds((prev) => prev.filter((id) => id !== assetId)); isMutatingRef.current = false; } }, @@ -292,7 +299,7 @@ export function useVideoAssets({ assets, isLoadingAssets, isCreatingAsset, - activeDeleteAssetId, + deletingAssetIds, activeDownloadAssetId, hasMoreAssets, isLoadingMoreAssets, diff --git a/components/video-page/hooks/use-video-page-data.ts b/components/video-page/hooks/use-video-page-data.ts index 24f1241..b75bb00 100644 --- a/components/video-page/hooks/use-video-page-data.ts +++ b/components/video-page/hooks/use-video-page-data.ts @@ -129,6 +129,14 @@ export function useVideoPageData({ mode, videoId, propProjectId }: UseVideoPageD void fetchVersionComments(activeVersionId, true); }, [activeVersionId, fetchVersionComments]); + // The auto-select reads the current selection from a ref rather than the dependency + // array. Depending on `selectedTagId` meant setting it re-ran the effect, so + // /api/projects//tags was requested a second time on every page load. + const selectedTagIdRef = useRef(selectedTagId); + useEffect(() => { + selectedTagIdRef.current = selectedTagId; + }, [selectedTagId]); + useEffect(() => { if (!projectId) return; async function fetchTags() { @@ -139,7 +147,7 @@ export function useVideoPageData({ mode, videoId, propProjectId }: UseVideoPageD const data = await res.json(); const tags = data.data || []; setAvailableTags(tags); - if (tags.length > 0 && !selectedTagId) { + if (tags.length > 0 && !selectedTagIdRef.current) { setSelectedTagId(tags[0].id); } } @@ -148,7 +156,7 @@ export function useVideoPageData({ mode, videoId, propProjectId }: UseVideoPageD } } void fetchTags(); - }, [projectId, selectedTagId, videoId]); + }, [projectId, videoId]); return { video, diff --git a/components/video-page/hooks/use-video-player.ts b/components/video-page/hooks/use-video-player.ts index 8248017..e8f61c6 100644 --- a/components/video-page/hooks/use-video-player.ts +++ b/components/video-page/hooks/use-video-player.ts @@ -242,8 +242,15 @@ export function useVideoPlayer({ const tag = document.createElement('script'); tag.src = 'https://www.youtube.com/iframe_api'; + // A document with no ')], + ['emailRow label', () => emailRow('', 'value')], + ['emailRow value', () => emailRow('label', '')], + ['emailHighlight text', () => emailHighlight('')], + ['emailButton label', () => emailButton('', 'https://x.test')], + ])('%s is escaped', (_label, build) => { + const html = build(); + + expect(html).not.toContain('', + }); + + expect(html).not.toContain('