Centralize RBAC, gate AI recommendations, and fix media-type, accessibility, and watch progress bugs#42
Open
Swastikdan wants to merge 1 commit into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR implements RBAC configuration consolidation, legacy user role migration, AI recommendation feature flagging, media-type-aware tracking across watchlist and recommendations, per-user watch progress scoping, and accessibility improvements. The changes unify RBAC constants in a shared module, add gating for recommendation features, introduce composite media type keys for tracking, and improve error handling and UI accessibility throughout the app. ChangesRBAC Centralization, Feature Flagging, and Media Tracking
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
convex/admin.ts (1)
123-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback to
user.roleuntil the migration is complete.
hasFeatureonly evaluatesuser.roles, so any account that still has only the legacyuser.rolefield will read as having no features and fail gates like the recommendation endpoints. Normalize roles from both fields until the legacy column is fully removed, and reuse that same helper ingetUserFeatures/listUsersas well.Suggested fix
+function getEffectiveRoles(user: { roles?: string[]; role?: string }) { + return Array.from( + new Set([...(user.roles ?? []), ...(user.role ? [user.role] : [])]), + ).filter((role): role is DynamicRbacRole => + DYNAMIC_ROLES.includes(role as DynamicRbacRole), + ); +} + export async function hasFeature( ctx: QueryCtx | MutationCtx, feature: RbacFeature, ): Promise<boolean> { @@ - for (const role of user.roles ?? []) { - if (!DYNAMIC_ROLES.includes(role as DynamicRbacRole)) continue; + for (const role of getEffectiveRoles(user)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/admin.ts` around lines 123 - 142, hasFeature currently only reads user.roles and thus misses legacy user.role values; update the logic to normalize roles from both fields (legacy user.role and new user.roles) before checking DYNAMIC_ROLES. Reuse or extract the same normalization helper used by getUserFeatures and listUsers (or create a shared normalizeUserRoles(user) helper) and iterate over the combined/normalized role list in the block that queries role_permissions (the code around getUserByToken, isClerkAdmin and the for-loop over user.roles) so legacy single-role accounts are evaluated the same as migrated accounts.src/routes/keyword.$id.tsx (1)
35-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the keyword meta title null-safe
head()still readsloaderData?.keyword.namefor the<title>; ifkeywordis missing it can throw, so align it with the already-null-safe description.Proposed fix
head: ({ loaderData }) => ({ meta: [ { - title: `${loaderData?.keyword.name || "Keyword"} - Movies | Pebbly`, + title: `${loaderData?.keyword?.name ?? "Keyword"} - Movies | Pebbly`, }, { name: "description", content: `Explore movies tagged with ${loaderData?.keyword?.name ?? "this keyword"} on Pebbly.`, }, ], }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/keyword`.$id.tsx around lines 35 - 42, The title meta in the head() function uses loaderData?.keyword.name which can throw if keyword is undefined; change it to the same null-safe pattern as the description (use loaderData?.keyword?.name ?? "Keyword" or similar) so the title fallback mirrors the description; update the head() meta title entry that references loaderData?.keyword.name to loaderData?.keyword?.name with optional chaining and a default.src/components/share-button.tsx (1)
13-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
navigator.share()rejections (user-cancelAbortError)
src/components/share-button.tsxawaitsnavigator.share(...)without atry/catch, while the click handler intentionally discards the returned promise viaonClick={() => void handleShare()}. When the user dismisses the native share sheet, the Web Share API rejects with anAbortError, which can surface as an unhandled promise rejection. Catch/suppressAbortErrorlocally.Proposed fix
async function handleShare() { if (navigator.share) { - await navigator.share({ - title: props.title, - url: window.location.href, - }); + try { + await navigator.share({ + title: props.title, + url: window.location.href, + }); + } catch (error) { + if (error instanceof DOMException && error.name === "AbortError") { + return; + } + alert("Unable to share this page"); + } + return; } else { const textToCopy = `${props.title} ${window.location.href}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/share-button.tsx` around lines 13 - 18, The handleShare function awaits navigator.share(...) without handling rejections which can lead to unhandled promise rejections when the user cancels (AbortError); wrap the await in a try/catch inside handleShare, detect and silently ignore an AbortError (e.g., error.name === 'AbortError' or DOMException code), and only log or rethrow other errors to preserve real failures; keep the click site using onClick={() => void handleShare()} unchanged so the promise remains intentionally discarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@convex/admin.ts`:
- Around line 302-305: The code currently calls ctx.db.query("users").collect()
which loads the entire users table into memory and then stops after
args.batchSize—replace the full-table collect with a paginated query that only
fetches at most args.batchSize per run; iterate pages (e.g., repeatedly calling
the users query with a limit/take and a cursor/lastId) and process each page
until you hit migrated >= (args.batchSize ?? 100) or no more rows. Update the
logic around the users variable and the for loop to fetch one page at a time
(using the query name "users" and the args.batchSize value) so you never load
the full table into memory.
- Around line 307-313: The migration currently skips users that already have any
roles because of the condition if (!legacyRole || (user.roles?.length ?? 0) > 0)
continue; — change the logic to only skip when legacyRole is falsy and, when
legacyRole exists, always call ctx.db.patch(user._id, ...) to merge legacyRole
into user.roles (Array.from(new Set([...(user.roles ?? []), legacyRole]))) and
set role: undefined so the legacy field is cleared; update the condition to if
(!legacyRole) continue; and keep the existing ctx.db.patch call (merging and
clearing) to ensure backfill and cleanup occur even when user.roles is
populated.
In `@convex/users.ts`:
- Around line 37-41: The current fallback only migrates legacy existing.role
when existing.roles is empty; change the logic in convex/users.ts so that
whenever existing.role is present you merge it into patch.roles and clear
patch.role. Specifically, detect existing.role (regardless of existing.roles
length), set patch.roles = Array.from(new Set([...(existing.roles ?? []),
existing.role])) and then set patch.role = undefined to ensure the legacy field
is always consumed/cleared.
---
Outside diff comments:
In `@convex/admin.ts`:
- Around line 123-142: hasFeature currently only reads user.roles and thus
misses legacy user.role values; update the logic to normalize roles from both
fields (legacy user.role and new user.roles) before checking DYNAMIC_ROLES.
Reuse or extract the same normalization helper used by getUserFeatures and
listUsers (or create a shared normalizeUserRoles(user) helper) and iterate over
the combined/normalized role list in the block that queries role_permissions
(the code around getUserByToken, isClerkAdmin and the for-loop over user.roles)
so legacy single-role accounts are evaluated the same as migrated accounts.
In `@src/components/share-button.tsx`:
- Around line 13-18: The handleShare function awaits navigator.share(...)
without handling rejections which can lead to unhandled promise rejections when
the user cancels (AbortError); wrap the await in a try/catch inside handleShare,
detect and silently ignore an AbortError (e.g., error.name === 'AbortError' or
DOMException code), and only log or rethrow other errors to preserve real
failures; keep the click site using onClick={() => void handleShare()} unchanged
so the promise remains intentionally discarded.
In `@src/routes/keyword`.$id.tsx:
- Around line 35-42: The title meta in the head() function uses
loaderData?.keyword.name which can throw if keyword is undefined; change it to
the same null-safe pattern as the description (use loaderData?.keyword?.name ??
"Keyword" or similar) so the title fallback mirrors the description; update the
head() meta title entry that references loaderData?.keyword.name to
loaderData?.keyword?.name with optional chaining and a default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd7bc0ce-ee19-4f43-b574-1828f589cbc6
📒 Files selected for processing (20)
convex/admin.tsconvex/recommendations.tsconvex/schema.tsconvex/users.tsconvex/watchlist.tsshared/rbac.tssrc/components/admin/admin-permission-toggles.tsxsrc/components/homepage-media.tsxsrc/components/homepage-recommendations.tsxsrc/components/media/watchlist-status-menu.tsxsrc/components/share-button.tsxsrc/constants.tssrc/hooks/usePermissions.tssrc/hooks/useRecommendations.tssrc/hooks/useWatchProgress.tssrc/lib/utils.tssrc/routes/keyword.$id.tsxsrc/routes/recommendations.tsxsrc/routes/watchlist.tsxsrc/styles.css
Comment on lines
+302
to
+305
| const users = await ctx.db.query("users").collect(); | ||
| let migrated = 0; | ||
| for (const user of users) { | ||
| if (migrated >= (args.batchSize ?? 100)) break; |
Contributor
There was a problem hiding this comment.
batchSize is defeated by collecting the full users table first.
Line 302 loads every user into memory before the loop stops at batchSize, so each run still scans the whole table to migrate at most 100 records. On a large production dataset this can hit Convex execution limits and makes the batch size mostly cosmetic. Page through users instead of calling collect() on the full table.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@convex/admin.ts` around lines 302 - 305, The code currently calls
ctx.db.query("users").collect() which loads the entire users table into memory
and then stops after args.batchSize—replace the full-table collect with a
paginated query that only fetches at most args.batchSize per run; iterate pages
(e.g., repeatedly calling the users query with a limit/take and a cursor/lastId)
and process each page until you hit migrated >= (args.batchSize ?? 100) or no
more rows. Update the logic around the users variable and the for loop to fetch
one page at a time (using the query name "users" and the args.batchSize value)
so you never load the full table into memory.
Comment on lines
+307
to
+313
| const legacyRole = user.role; | ||
| if (!legacyRole || (user.roles?.length ?? 0) > 0) continue; | ||
|
|
||
| await ctx.db.patch(user._id, { | ||
| roles: Array.from(new Set([...(user.roles ?? []), legacyRole])), | ||
| role: undefined, | ||
| }); |
Contributor
There was a problem hiding this comment.
Merge and clear the legacy role even when roles is already populated.
Line 308 skips every user that already has any roles, so a record like roles=["video-player"] plus legacy role="ai-integrations" never gets backfilled or cleaned up. The migration should merge user.role into user.roles whenever user.role exists, then clear user.role.
Suggested fix
- const legacyRole = user.role;
- if (!legacyRole || (user.roles?.length ?? 0) > 0) continue;
+ const legacyRole = user.role;
+ if (!legacyRole) continue;
+
+ const nextRoles = Array.from(new Set([...(user.roles ?? []), legacyRole]));
await ctx.db.patch(user._id, {
- roles: Array.from(new Set([...(user.roles ?? []), legacyRole])),
+ roles: nextRoles,
role: undefined,
});📝 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.
Suggested change
| const legacyRole = user.role; | |
| if (!legacyRole || (user.roles?.length ?? 0) > 0) continue; | |
| await ctx.db.patch(user._id, { | |
| roles: Array.from(new Set([...(user.roles ?? []), legacyRole])), | |
| role: undefined, | |
| }); | |
| const legacyRole = user.role; | |
| if (!legacyRole) continue; | |
| const nextRoles = Array.from(new Set([...(user.roles ?? []), legacyRole])); | |
| await ctx.db.patch(user._id, { | |
| roles: nextRoles, | |
| role: undefined, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@convex/admin.ts` around lines 307 - 313, The migration currently skips users
that already have any roles because of the condition if (!legacyRole ||
(user.roles?.length ?? 0) > 0) continue; — change the logic to only skip when
legacyRole is falsy and, when legacyRole exists, always call
ctx.db.patch(user._id, ...) to merge legacyRole into user.roles (Array.from(new
Set([...(user.roles ?? []), legacyRole]))) and set role: undefined so the legacy
field is cleared; update the condition to if (!legacyRole) continue; and keep
the existing ctx.db.patch call (merging and clearing) to ensure backfill and
cleanup occur even when user.roles is populated.
Comment on lines
+37
to
+41
| // TODO: Remove this legacy-role fallback after migrateLegacyUserRoles has run in production. | ||
| if ((existing.roles?.length ?? 0) === 0 && existing.role) { | ||
| patch.roles = Array.from(new Set([existing.role])); | ||
| patch.role = undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't leave existing.role behind when roles already exists.
This fallback only runs when existing.roles is empty, so any user who already has roles plus a leftover legacy role never gets merged or cleaned up on sign-in. Treat existing.role as additive whenever it is present, then clear it.
Suggested fix
- if ((existing.roles?.length ?? 0) === 0 && existing.role) {
- patch.roles = Array.from(new Set([existing.role]));
+ if (existing.role) {
+ patch.roles = Array.from(
+ new Set([...(existing.roles ?? []), existing.role]),
+ );
patch.role = undefined;
}📝 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.
Suggested change
| // TODO: Remove this legacy-role fallback after migrateLegacyUserRoles has run in production. | |
| if ((existing.roles?.length ?? 0) === 0 && existing.role) { | |
| patch.roles = Array.from(new Set([existing.role])); | |
| patch.role = undefined; | |
| } | |
| // TODO: Remove this legacy-role fallback after migrateLegacyUserRoles has run in production. | |
| if (existing.role) { | |
| patch.roles = Array.from( | |
| new Set([...(existing.roles ?? []), existing.role]), | |
| ); | |
| patch.role = undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@convex/users.ts` around lines 37 - 41, The current fallback only migrates
legacy existing.role when existing.roles is empty; change the logic in
convex/users.ts so that whenever existing.role is present you merge it into
patch.roles and clear patch.role. Specifically, detect existing.role (regardless
of existing.roles length), set patch.roles = Array.from(new
Set([...(existing.roles ?? []), existing.role])) and then set patch.role =
undefined to ensure the legacy field is always consumed/cleared.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
ai-recommendationsfeature is disabled and ensure feedback/dismissals respect media type.users.rolesfrom the legacyrolefield.Description
shared/rbac.ts) and re-exported types/values fromsrc/constants.ts, and updatedconvex/admin.tsto consume the shared definitions and keep defaults centralized.requireAdmin(ctx)ingetRolePermissions, addedmigrateLegacyUserRolesadmin mutation to backfillusers.roles, and preserved the legacyroleschema field with a TODO inconvex/schema.tsandconvex/users.tsto migrate existing docs.hasFeature(ctx, "ai-recommendations")and make recommendation feedback/dismissal/tracked IDs media-type-aware by using composite keys (e.g.,${mediaType}:${tmdbId}) and preservelistIdwhen replaying list-based generations acrossconvex/recommendations.ts, frontend hooks, and route handlers.convex/watchlist.getTrackedTmdbIds, make homepage recommendation liked/dismissal sets use composite keys, prevent tight generate-recs retry loops by tracking refresh failures, and filter out failed queries in the continue-watching row instead of dropping the whole row.last_playedlocalStorage keys by viewer id and addsafeNextEpisodelogic using season metadata to avoid out-of-range next-episode values; change playback-derived progress status to take precedence over stale statuses inconvex/watchlist.tsandsrc/hooks/useWatchProgress.ts.aria-labelto admin toggle switches and share button, add.catcherror handling to role-permission mutation calls, setaria-pressedon list buttons, make custom-list deletion await the mutation and handle/report errors, and add a backward-compatible legacy progress status mapping insrc/lib/utils.ts.Testing
npx biome check(targeted files) and the project linter fixed/organized imports where applicable and reported no new issues in the modified files; this check completed successfully for the changed files.npx tsc --noEmitwhich succeeded (typecheck passed for the edits).npm run checkwhich still reports pre-existing Biome findings in unrelated files (e.g.,media-poster-trailer-container.tsx,ui/icons.tsx,search-bar.tsx,video-player-modal.tsx,routes/index.tsx), so those unrelated lint findings remain unresolved.npm run build: client and SSR bundles were produced successfully, but prerender failed when fetching/with anInternal Server Errorduring prerendering (likely unrelated runtime/environment issue surfaced during prerender).Codex Task
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes