fix: hide watermark for pro users #989
Conversation
WalkthroughAdds light/dark button-border utility classes to desktop and web CSS, updates Button gray/dark variants to use those utilities, and extends video data and ShareVideo to include owner subscription fields and gate the watermark overlay based on the video owner’s pro status. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Viewer
participant Page as s/[videoId]/page.tsx
participant DB as DB (videos, users)
participant Share as ShareVideo.tsx
participant UI as WatermarkOverlay
Viewer->>Page: Request video page
Page->>DB: Select video + LEFT JOIN owner (subscription fields)
DB-->>Page: Video { ..., owner: { stripeSubscriptionStatus, thirdPartyStripeSubscriptionId } }
Page->>Share: Render with data (includes owner)
Share->>Share: isVideoOwnerPro = userIsPro(owner)
alt owner is Pro
Share-->>UI: Hide/disable watermark
else owner not Pro
Share-->>UI: Show watermark
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (1)
177-199: Consider showing the “Remove watermark” CTA only to the owner.Current logic shows the CTA to any viewer when the owner isn’t Pro, which can be confusing since upgrading the viewer won’t remove the watermark. Optionally gate the CTA by ownership:
-{!isVideoOwnerPro && ( +{!isVideoOwnerPro && user?.id === data.ownerId && (If the growth funnel intentionally targets viewers, ignore this suggestion.
apps/web/app/s/[videoId]/page.tsx (1)
291-299: Owner join and projection look correct; add type and DRY the selection.
- The leftJoin to users and owner projection match ShareVideo’s needs. Good.
- For type safety where this data flows through Share/ShareHeader, consider extending VideoWithOrganization to include the optional owner shape.
- The owner selection is duplicated; factor into a const to avoid drift.
Proposed type addition (outside this hunk):
type VideoWithOrganization = typeof videos.$inferSelect & { sharedOrganization?: { organizationId: string } | null; organizationMembers?: string[]; organizationId?: string; sharedOrganizations?: { id: string; name: string }[]; password?: string | null; hasPassword?: boolean; + owner?: { + stripeSubscriptionStatus: string | null; + thirdPartyStripeSubscriptionId: string | null; + } | null; };Optional DRY (illustrative):
const ownerSelect = { owner: { stripeSubscriptionStatus: users.stripeSubscriptionStatus, thirdPartyStripeSubscriptionId: users.thirdPartyStripeSubscriptionId, }, };…and spread ownerSelect into both queries’ select blocks.
Also applies to: 446-473
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/styles/theme.css(1 hunks)apps/web/app/globals.css(1 hunks)apps/web/app/s/[videoId]/_components/ShareVideo.tsx(3 hunks)apps/web/app/s/[videoId]/page.tsx(3 hunks)packages/ui-solid/src/Button.tsx(1 hunks)packages/ui/src/components/Button.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
packages/ui-solid/src/Button.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
packages/ui-solid/src/Button.tsxpackages/ui/src/components/Button.tsxapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsx
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
packages/ui/src/components/Button.tsxapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsx
🧬 Code graph analysis (2)
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (1)
packages/utils/src/constants/plans.ts (1)
userIsPro(21-45)
apps/web/app/s/[videoId]/page.tsx (1)
packages/database/schema.ts (3)
users(45-87)videos(231-277)sharedVideos(279-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
packages/ui-solid/src/Button.tsx (1)
23-24: LGTM: border color now sourced from theme utilities.Switch to gray-button-border and dark-button-border keeps variant intent while centralizing theming. No behavioral concerns.
packages/ui/src/components/Button.tsx (1)
26-27: LGTM: uses themed border utilities.Adopts gray-button-border and dark-button-border consistently with the desktop/web CSS. No API or accessibility regressions spotted.
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (1)
152-152: Correct gating: hide watermark when video owner is Pro.Switching to userIsPro(data.owner) matches the PR objective. Good.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/s/[videoId]/Share.tsx(1 hunks)apps/web/app/s/[videoId]/_components/ShareVideo.tsx(3 hunks)apps/web/app/s/[videoId]/page.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/s/[videoId]/Share.tsx
- apps/web/app/s/[videoId]/_components/ShareVideo.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/s/[videoId]/page.tsx
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/s/[videoId]/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/web/app/s/[videoId]/page.tsx
🧬 Code graph analysis (1)
apps/web/app/s/[videoId]/page.tsx (1)
packages/database/schema.ts (3)
users(45-87)videos(231-277)sharedVideos(279-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
| owner?: { | ||
| stripeSubscriptionStatus: string | null; | ||
| thirdPartyStripeSubscriptionId: string | null; | ||
| } | null; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Block PII: don’t serialize subscription identifiers to the client; expose only a boolean.
These fields flow into Share/ShareHeader (client). Sending thirdPartyStripeSubscriptionId (and even the raw status string) publicly is unnecessary for watermark gating and is sensitive. Compute a server-side boolean and pass that instead.
Apply:
- owner?: {
- stripeSubscriptionStatus: string | null;
- thirdPartyStripeSubscriptionId: string | null;
- } | null;
+ ownerIsPro?: boolean;Also convert the numeric SQL flag to boolean at construction time (outside this hunk): in the object built at Lines 666-674, add
ownerIsPro: video.ownerIsPro === 1,.
Run to find downstream usages to update to the boolean flag:
🏁 Script executed:
#!/bin/bash
# Replace client-side checks that read owner.* with a boolean ownerIsPro
rg -n --type=ts --type=tsx -C2 $'isVideoOwnerPro\\(|thirdPartyStripeSubscriptionId|owner\\s*:\\s*{\\s*stripeSubscriptionStatus' apps/webLength of output: 233
🏁 Script executed:
#!/bin/bash
# Search for any downstream usages of the old owner fields or checks against them
rg -n --type=ts --type=tsx -C2 \
-e 'isVideoOwnerPro\(' \
-e 'thirdPartyStripeSubscriptionId' \
-e 'owner\s*:\s*\{\s*stripeSubscriptionStatus' apps/webLength of output: 27
🏁 Script executed:
#!/bin/bash
# Retry search without --type flags; include .ts/.tsx extensions explicitly
rg -n -C2 -g '*.ts' -g '*.tsx' \
-e 'isVideoOwnerPro\(' \
-e 'thirdPartyStripeSubscriptionId' \
-e 'stripeSubscriptionStatus' \
apps/webLength of output: 12969
Block PII: replace raw subscription fields with a boolean ownerIsPro everywhere
- In apps/web/app/s/[videoId]/page.tsx (lines 111–115), replace:
-owner?: { - stripeSubscriptionStatus: string | null; - thirdPartyStripeSubscriptionId: string | null; -} | null; +ownerIsPro?: boolean;
- When constructing the video payload (e.g. the
.select(...)at lines 296–300), add:ownerIsPro: video.ownerIsPro === 1,
- Refactor all downstream usages of
stripeSubscriptionStatusandthirdPartyStripeSubscriptionId, including but not limited to:
• apps/web/app/s/[videoId]/Share.tsx
• apps/web/app/s/[videoId]/_components/ShareVideo.tsx
• apps/web/utils/flags.ts (FeatureFlagUser)
• Any.select({ …, stripeSubscriptionStatus, thirdPartyStripeSubscriptionId })and runtime checks in apps/web/actions and apps/web/app/api - To locate all occurrences, run:
rg -l -e 'stripeSubscriptionStatus' -e 'thirdPartyStripeSubscriptionId' apps/web
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/page.tsx around lines 111–115 and when constructing
the video payload at ~lines 296–300, remove the raw PII fields
stripeSubscriptionStatus and thirdPartyStripeSubscriptionId from the owner type
and instead expose a boolean ownerIsPro (set ownerIsPro: video.ownerIsPro === 1
in the .select(...) that builds the payload); then refactor all downstream
usages to consume ownerIsPro (replace checks against
stripeSubscriptionStatus/thirdPartyStripeSubscriptionId in
apps/web/app/s/[videoId]/Share.tsx,
apps/web/app/s/[videoId]/_components/ShareVideo.tsx, apps/web/utils/flags.ts
(FeatureFlagUser) and any actions or api code that .selects or reads those
fields), remove those fields from any .select({...}) calls, and run rg -l -e
'stripeSubscriptionStatus' -e 'thirdPartyStripeSubscriptionId' apps/web to find
and update remaining references.
| owner: { | ||
| stripeSubscriptionStatus: users.stripeSubscriptionStatus, | ||
| thirdPartyStripeSubscriptionId: | ||
| users.thirdPartyStripeSubscriptionId, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Select a computed pro flag; avoid leaking subscription IDs/status.
Replace the nested owner object with a single computed flag. Keep the users left-join.
- owner: {
- stripeSubscriptionStatus: users.stripeSubscriptionStatus,
- thirdPartyStripeSubscriptionId:
- users.thirdPartyStripeSubscriptionId,
- },
+ ownerIsPro: sql<number>`IF(
+ ${users.stripeSubscriptionStatus} IN ('active','trialing')
+ OR ${users.thirdPartyStripeSubscriptionId} IS NOT NULL,
+ 1, 0
+ )`,Also applies to: 303-303
| owner: { | ||
| stripeSubscriptionStatus: string | null; | ||
| thirdPartyStripeSubscriptionId: string | null; | ||
| } | null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align prop type with boolean flag to prevent client exposure.
- owner: {
- stripeSubscriptionStatus: string | null;
- thirdPartyStripeSubscriptionId: string | null;
- } | null;
+ ownerIsPro: number;Note: This remains numeric here because it comes from SQL; convert to boolean before passing to client props (see prior comment).
📝 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.
| owner: { | |
| stripeSubscriptionStatus: string | null; | |
| thirdPartyStripeSubscriptionId: string | null; | |
| } | null; | |
| ownerIsPro: number; |
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/page.tsx around lines 353 to 356, the prop
stripeSubscriptionStatus is currently typed as string | null but should be a
boolean (or null) so we don't expose raw numeric/SQL values to the client;
change the type to boolean | null and, where you build the props, convert the
numeric SQL value (e.g., 1/0 or '1'/'0') to a boolean (true/false) before
passing it to the client; leave thirdPartyStripeSubscriptionId as string | null
if it is truly a string.
| owner: { | ||
| stripeSubscriptionStatus: users.stripeSubscriptionStatus, | ||
| thirdPartyStripeSubscriptionId: users.thirdPartyStripeSubscriptionId, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the computed flag in the post-transcription refresh query.
- owner: {
- stripeSubscriptionStatus: users.stripeSubscriptionStatus,
- thirdPartyStripeSubscriptionId: users.thirdPartyStripeSubscriptionId,
- },
+ ownerIsPro: sql<number>`IF(
+ ${users.stripeSubscriptionStatus} IN ('active','trialing')
+ OR ${users.thirdPartyStripeSubscriptionId} IS NOT NULL,
+ 1, 0
+ )`,Also applies to: 480-480
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/page.tsx around lines 454-457 (and also at line
480), the post-transcription refresh query omits the computed subscription flag
that the main query exposes; update the owner object in both locations to
include the same computed flag key and value used in the main query (mirror the
exact expression/variable name and value logic), so the refresh response
contains the identical computed flag as the original query.
Summary by CodeRabbit
New Features
Bug Fixes
Style