Monthly shareable link quota and free-plan duration limits#1800
Monthly shareable link quota and free-plan duration limits#1800richiemcilroy wants to merge 36 commits into
Conversation
superagent-security
Bot
added
the
contributor:verified
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const [video] = maybeVideo.value; | ||
|
|
||
| yield* Effect.tryPromise({ | ||
| try: () => | ||
| assertShareableLinkDurationAllowed({ | ||
| client: getDb(), | ||
| ownerId: user.id, | ||
| durationSeconds: body.durationInSecs, | ||
| }), | ||
| catch: (cause) => cause, | ||
| }); |
There was a problem hiding this comment.
assertShareableLinkDurationAllowed call omits isScreenshot, which defaults to false. The video object is already in scope at line 332. Without video.isScreenshot, a free-plan user uploading a screenshot via multipart with a durationInSecs > 300 in the request body would be incorrectly blocked by the duration limit.
| const [video] = maybeVideo.value; | |
| yield* Effect.tryPromise({ | |
| try: () => | |
| assertShareableLinkDurationAllowed({ | |
| client: getDb(), | |
| ownerId: user.id, | |
| durationSeconds: body.durationInSecs, | |
| }), | |
| catch: (cause) => cause, | |
| }); | |
| const [video] = maybeVideo.value; | |
| yield* Effect.tryPromise({ | |
| try: () => | |
| assertShareableLinkDurationAllowed({ | |
| client: getDb(), | |
| ownerId: user.id, | |
| isScreenshot: video.isScreenshot, | |
| durationSeconds: body.durationInSecs, | |
| }), | |
| catch: (cause) => cause, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/upload/[...route]/multipart.ts
Line: 332-342
Comment:
The `assertShareableLinkDurationAllowed` call omits `isScreenshot`, which defaults to `false`. The `video` object is already in scope at line 332. Without `video.isScreenshot`, a free-plan user uploading a screenshot via multipart with a `durationInSecs > 300` in the request body would be incorrectly blocked by the duration limit.
```suggestion
const [video] = maybeVideo.value;
yield* Effect.tryPromise({
try: () =>
assertShareableLinkDurationAllowed({
client: getDb(),
ownerId: user.id,
isScreenshot: video.isScreenshot,
durationSeconds: body.durationInSecs,
}),
catch: (cause) => cause,
});
```
How can I resolve this? If you propose a fix, please make it concise.| ); | ||
| } | ||
|
|
||
| const durationSeconds = manifest.video_segments.reduce<number>( | ||
| (total, segment) => | ||
| total + Video.normalizeSegmentEntry(segment).duration, | ||
| 0, | ||
| ); | ||
|
|
||
| yield* Effect.tryPromise({ | ||
| try: () => | ||
| assertShareableLinkDurationAllowed({ | ||
| client: db(), | ||
| ownerId: user.id, | ||
| isScreenshot: video.isScreenshot, | ||
| durationSeconds, | ||
| }), | ||
| catch: (cause) => cause, | ||
| }); | ||
|
|
||
| const videoInitUrl = yield* bucket.getSignedObjectUrl( | ||
| segSource.getVideoInitKey(), | ||
| { expiresIn: MEDIA_SERVER_PRESIGNED_GET_EXPIRES_SECONDS }, |
There was a problem hiding this comment.
When assertShareableLinkDurationAllowed fires here, the videos row already exists in the DB (inserted during createVideoWithShareableLinkQuota). Setting videoUploads.phase = "error" does not delete the video record, so countCurrentPeriodShareableLinks will still count it. A free-plan user who uploads a 6-minute video (duration not known at creation time, e.g. via signed/multipart) loses one of their 30 monthly links even though the upload was rejected. The same pattern applies in process-video.ts and import-loom-video.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/upload/[...route]/recording-complete.ts
Line: 114-136
Comment:
**Failed long-duration uploads consume a monthly quota slot**
When `assertShareableLinkDurationAllowed` fires here, the `videos` row already exists in the DB (inserted during `createVideoWithShareableLinkQuota`). Setting `videoUploads.phase = "error"` does not delete the video record, so `countCurrentPeriodShareableLinks` will still count it. A free-plan user who uploads a 6-minute video (duration not known at creation time, e.g. via signed/multipart) loses one of their 30 monthly links even though the upload was rejected. The same pattern applies in `process-video.ts` and `import-loom-video.ts`.
How can I resolve this? If you propose a fix, please make it concise.![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| typeof error === "object" && | ||
| error !== null && | ||
| "_tag" in error && | ||
| error._tag === "ShareableLinkUsageLimitError" |
There was a problem hiding this comment.
Worth making this predicate resilient to wrapped errors. A few call sites throw new Error("upgrade_required", { cause: err }), and then isShareableLinkUsageLimitError won’t match unless it unwraps cause.
| error._tag === "ShareableLinkUsageLimitError" | |
| export function isShareableLinkUsageLimitError( | |
| error: unknown, | |
| ): error is Video.ShareableLinkUsageLimitError { | |
| if ( | |
| typeof error === "object" && | |
| error !== null && | |
| "_tag" in error && | |
| error._tag === "ShareableLinkUsageLimitError" | |
| ) { | |
| return true; | |
| } | |
| if (error instanceof Error) { | |
| return isShareableLinkUsageLimitError((error as { cause?: unknown }).cause); | |
| } | |
| return false; | |
| } |
| .where( | ||
| and( | ||
| eq(Db.videos.ownerId, userId), | ||
| eq(Db.videos.isScreenshot, false), |
There was a problem hiding this comment.
Double-check this matches the intended definition of “shareable link” usage. Right now it’s counting all non-screenshot videos in the current UTC month; if “shareable links” should exclude e.g. private videos, failed uploads, or some other subset, this may need an additional filter.
| }); | ||
| let url = format!( | ||
| "{}/pricing?utm_source=desktop&utm_campaign=upgrade", | ||
| server_url.trim_end_matches('/') |
There was a problem hiding this comment.
Since server_url can come from user settings, it may be worth validating the scheme before building/launching the URL (e.g. allow http:///https:// only) so we don’t accidentally open arbitrary schemes if someone misconfigures server_url.
| ); | ||
|
|
||
| return NextResponse.json(getShareableLinkLimitResponse(error), { | ||
| status: 403, |
There was a problem hiding this comment.
Potential retry-loop: many webhook senders treat non-2xx as “try again later”. Since you’re already updating videoUploads to an error state, consider returning 200 here (with a body indicating upgrade_required if you still want visibility) to avoid repeated webhook deliveries.
| subscription: true, | ||
| videoLimit: 0, | ||
| videoCount: numberOfVideos[0].count, | ||
| videoCount: usage.used, |
There was a problem hiding this comment.
videoCount now looks like it’s being repurposed to mean “shareable links used this month” (and for Pro it’ll always be 0 due to the snapshot). If anything is still expecting total video count here, it might be safer to keep videoCount semantics stable and expose monthly usage only via shareableLinkUsage.
| async setCameraInput(id: DeviceOrModelID | null, skipCameraWindow: boolean | null) : Promise<null> { | ||
| return await TAURI_INVOKE("set_camera_input", { id, skipCameraWindow }); | ||
| }, | ||
| async setNativeCameraPreviewEnabled(enabled: boolean) : Promise<null> { |
There was a problem hiding this comment.
Minor workflow nit: since this file is generated (header says not to edit manually), can we make sure this change comes from regenerating the bindings (tauri-specta) rather than hand-editing tauri.ts? That avoids the next regen accidentally dropping setNativeCameraPreviewEnabled.
|
please re-review the pr @greptileai - added support for older desktop versions. |
|
please re-review the pr @greptileai- added support for older desktop versions. |
| yield* Effect.tryPromise({ | ||
| try: () => | ||
| assertShareableLinkDurationAllowed({ | ||
| client: db(), | ||
| ownerId: user.id, | ||
| isScreenshot: video.isScreenshot, | ||
| durationSeconds, | ||
| }), | ||
| catch: (cause) => cause, | ||
| }); |
There was a problem hiding this comment.
assertShareableLinkDurationAllowed is called inside an Effect.gen block and its failure propagates as an Exit.Failure object (thrown by the custom runPromise via throw res). The outer catch (error) block then calls isShareableLinkUsageLimitError(error), but that function only checks error._tag and recursively checks error.cause on Error instances — Exit.Failure is a plain Effect data object (not an Error instance) with _tag: "Failure", so the check returns false. The result is a 500 response instead of a 403 with an upgrade prompt, and markShareableLinkUploadRejected is never called via the intended path.
Compare with multipart.ts, where the error is handled inside the Effect chain using Effect.catchAll, so isShareableLinkUsageLimitError receives the raw error rather than a wrapped Exit.Failure. The simplest fix here is to move the duration assertion before the Effect.gen block (using plain async/await like signed.ts does), or to catch it inside the Effect pipeline the same way multipart.ts does.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/upload/[...route]/recording-complete.ts
Line: 124-133
Comment:
**Duration limit error not detected in outer catch block**
`assertShareableLinkDurationAllowed` is called inside an `Effect.gen` block and its failure propagates as an `Exit.Failure` object (thrown by the custom `runPromise` via `throw res`). The outer `catch (error)` block then calls `isShareableLinkUsageLimitError(error)`, but that function only checks `error._tag` and recursively checks `error.cause` on `Error` instances — `Exit.Failure` is a plain Effect data object (not an `Error` instance) with `_tag: "Failure"`, so the check returns `false`. The result is a 500 response instead of a 403 with an upgrade prompt, and `markShareableLinkUploadRejected` is never called via the intended path.
Compare with `multipart.ts`, where the error is handled *inside* the Effect chain using `Effect.catchAll`, so `isShareableLinkUsageLimitError` receives the raw error rather than a wrapped `Exit.Failure`. The simplest fix here is to move the duration assertion before the `Effect.gen` block (using plain `async/await` like `signed.ts` does), or to catch it inside the Effect pipeline the same way `multipart.ts` does.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Introduces a monthly cap on shareable links for free accounts and keeps the 5-minute max length enforced consistently across creation, upload, processing, and Loom import. Pro users are unchanged.
Greptile Summary
This PR enforces a 30-link monthly quota and a 5-minute maximum duration for free-plan users across every video ingestion path (desktop API, web upload, multipart, signed URL, recording-complete, media-server webhook, Loom import, and video processing workflows), while leaving Pro users unrestricted. It also ships a new
ShareableLinkUsagedashboard widget that replaces the previous Rive animation with a live progress bar and reset date.ShareableLinkUsage.tsis the central enforcement module:createVideoWithShareableLinkQuotawraps creation in a serializable transaction with aSELECT … FOR UPDATErow-lock to prevent races,assertShareableLinkDurationAllowedhandles post-creation duration checks, andmarkShareableLinkUploadRejectedmarks failed uploads so the quota counter excludes them.(ownerId, isScreenshot, createdAt)index is added to make the monthly count query efficient, and theUsageButtonsidebar widget is rewritten to surface real-time quota state to free users.VideosRepo.createomitsisScreenshotcausing screenshots to be blocked at quota exhaustion, and the duration check inrecording-complete.tsfails silently due to Effect error wrapping.Confidence Score: 3/5
Not safe to merge as-is — two enforcement paths have confirmed defects that affect free-plan users in production.
The VideosRepo.create path never forwards isScreenshot, so any free user at the 30-link cap is blocked from duplicating or creating screenshots. The recording-complete endpoint wraps the duration assertion inside Effect.gen; runPromise re-throws failure as an Exit.Failure object that isShareableLinkUsageLimitError cannot detect, causing 500 responses instead of 403 upgrade prompts for over-length segment recordings.
packages/web-backend/src/Videos/VideosRepo.ts (missing isScreenshot forwarding) and apps/web/app/api/upload/[...route]/recording-complete.ts (Effect error wrapping mismatch).
Important Files Changed
Comments Outside Diff (1)
packages/web-backend/src/Videos/VideosRepo.ts, line 65-120 (link)isScreenshotnot forwarded — screenshot creation blocked when quota is exhaustedcreateVideoWithShareableLinkQuotais called withoutisScreenshot, so it defaults tofalse. When a free user has reached the 30-link cap,assertCanCreateShareableLinkthrowsShareableLinkUsageLimitErrorfor every call through this path — including for screenshot videos — becauseisScreenshotis always treated asfalse. Screenshots should be unconditionally allowed (they are excluded fromcountCurrentPeriodShareableLinks), but anyVideosRepo.createinvocation (e.g.VideoDuplicate,VideoInstantCreate) that carriesdata.isScreenshot === truewill be incorrectly rejected once the user's quota is full.The fix is to read
isScreenshotfromdataand forward it:isScreenshot: data.isScreenshot ?? false.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "feat(upload): mark quota rejects on sign..." | Re-trigger Greptile