feat(aih): cohort access ability for workshops #470
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes refactor entitlement-based permission logic for cohort content access, centralizing checks within Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: Requests workshop lesson/resource
Frontend->>Backend: Fetch ability rules (with session, orgId)
Backend->>Database: Query entitlementTypes, entitlements
Database-->>Backend: Return entitlementTypes and entitlements
Backend->>Frontend: Return ability rules (incl. entitlementTypes)
Frontend->>Frontend: Check canView/canCreate using rules
Frontend-->>User: Render content or show access restrictions
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
Scope: all 3 workspace projects This error happened while installing a direct dependency of /tmp/eslint/packages/config/eslint-config Packages found in the workspace: Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…n post cohort purchase workflow
| @@ -103,7 +105,7 @@ export const postCohortPurchaseWorkflow = inngest.createFunction( | |||
| if (cohortContentAccessEntitlementType && cohortResource?.resources) { | |||
| await step.run(`add user to cohort via entitlement`, async () => { | |||
| for (const resource of cohortResource.resources || []) { | |||
| const entitlementId = `${cohortContentAccessEntitlementType.id}-${resource.resource.id}` | |||
| const entitlementId = `${resource.resource.id}-${guid()}` | |||
There was a problem hiding this comment.
making sure these are all unique
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/ai-hero/src/inngest/functions/post-cohort-purchase-workflow.ts (1)
106-112: 💡 Verification agent🧩 Analysis chain
Potential type-mismatch & redundant compound key when generating
entitlementId
entitlements.idis typically defined as a UUID (or at most a 36-charvarchar).
Concatenatinguuidv4()with an additional-<resource.id>:const entitlementId = `${randomId}-${resource.resource.id}`may:
- Exceed the column’s length / UUID format constraints and blow up the insert.
- Add no real value—
randomIdis already globally unique.- Make debugging harder (id is no longer a valid UUID).
Unless you have an explicit requirement for the compound id, consider keeping the id a pure UUID and store
resource.resource.idonly insourceId/metadata.- const randomId = uuidv4() - const entitlementId = `${randomId}-${resource.resource.id}` + const entitlementId = uuidv4()Bonus: before inserting, upsert or check for an existing entitlement to avoid duplicates if the workflow is retried.
🏁 Script executed:
#!/bin/bash # Locate the Drizzle table definition for `entitlements` and show its schema rg -l "export const entitlements" -g "*.ts" | xargs -I{} sed -n '1,200p' {}Length of output: 79
🏁 Script executed:
#!/bin/bash # Search for all references to `entitlements` across TypeScript files to locate its table definition rg -n "entitlements" -g '*.ts'Length of output: 22536
🏁 Script executed:
#!/bin/bash # Show the first 200 lines of the AI Hero DB schema to locate the `entitlements` table definition sed -n '1,200p' apps/ai-hero/src/db/schema.tsLength of output: 1949
🏁 Script executed:
#!/bin/bash # Display the schema definition for the entitlements table to verify column types sed -n '1,200p' packages/adapter-drizzle/src/lib/mysql/schemas/entitlements/entitlement.tsLength of output: 2415
Simplify
entitlementIdand storeresource.idseparatelyThe
Entitlement.idcolumn is defined asvarchar(191), so concatenating a 36-char UUID plus-<resource.id>won’t exceed the length or violate any UUID format (there’s no strict UUID constraint). Embeddingresource.resource.idin the primary key provides little added value—uuidv4()alone is already globally unique—and makes it harder to query or reference the resource later.Instead, use a pure UUID for
idand persistresource.resource.idinsourceIdormetadata. Also, to guard against duplicate entitlements when the workflow is retried, consider an upsert or a pre-insert existence check.File: apps/ai-hero/src/inngest/functions/post-cohort-purchase-workflow.ts
Lines 106–112- const randomId = uuidv4() - const entitlementId = `${randomId}-${resource.resource.id}` + const entitlementId = uuidv4() // e.g. sourceId: resource.resource.id
🧹 Nitpick comments (2)
apps/ai-hero/src/lib/cohorts-query.ts (1)
93-105: Dynamic type lookup is good – but ensure column types align and keep logic DRYFetching the entitlement-type id avoids hard-coding – nice!
A couple of follow-ups:
entitlements.entitlementTypemust be the same type asentitlementTypes.id(likely UUID). If it still stores a string like'cohort_content_access', the new filter will silently fail. Please double-check the column definition and existing data.You now have two nearly identical queries (here and in
post-cohort-purchase-workflow.ts). Extract a small helper:export async function getEntitlementTypeId(name: string) { return ( await db.query.entitlementTypes.findFirst({ where: eq(entitlementTypes.name, name), }) )?.id }
- Consistency:
syncDiscordRolesstill filters by the string'cohort_discord_role'. If that column was switched to UUIDs too, that query will break.apps/ai-hero/src/trpc/api/routers/ability.ts (1)
75-88: Remove unusedsessionvariable to prevent linter / build warnings
const { session } = await getServerAuthSession()is never referenced afterwards.- const { session } = await getServerAuthSession() + await getServerAuthSession() // still enforces auth cookie decoding (if needed)(or keep the destructuring and actually leverage
session?.user.idin the ability rules call).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/ai-hero/src/ability/index.ts(2 hunks)apps/ai-hero/src/app/(commerce)/products/page.tsx(2 hunks)apps/ai-hero/src/app/(content)/workshops/[module]/[lesson]/(view)/layout.tsx(2 hunks)apps/ai-hero/src/app/(content)/workshops/_components/workshop-resource-list.tsx(6 hunks)apps/ai-hero/src/components/navigation/index.tsx(0 hunks)apps/ai-hero/src/inngest/functions/post-cohort-purchase-workflow.ts(2 hunks)apps/ai-hero/src/lib/cohorts-query.ts(3 hunks)apps/ai-hero/src/lib/workshops-query.ts(1 hunks)apps/ai-hero/src/server/auth.ts(0 hunks)apps/ai-hero/src/trpc/api/routers/ability.ts(2 hunks)apps/ai-hero/src/utils/get-current-ability-rules.ts(3 hunks)packages/utils-auth/src/current-ability-rules.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/ai-hero/src/server/auth.ts
- apps/ai-hero/src/components/navigation/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/ai-hero/src/app/(content)/workshops/[module]/[lesson]/(view)/layout.tsx (1)
apps/ai-hero/src/app/(content)/workshops/_components/workshop-resource-list.tsx (1)
WorkshopResourceList(42-246)
apps/ai-hero/src/app/(content)/workshops/_components/workshop-resource-list.tsx (1)
apps/ai-hero/src/ability/index.ts (1)
createAppAbility(102-102)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: typecheck
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (19)
packages/utils-auth/src/current-ability-rules.ts (1)
81-82: Extension of permission system looks goodThe addition of the
canCreateproperty toAbilityForResourceis well-documented and follows the established pattern of the existing properties. This extension enables the cohort access ability feature described in the PR objectives.apps/ai-hero/src/app/(commerce)/products/page.tsx (1)
3-3: API function name change looks goodChanged from using
getActiveSelfPacedProductsto the more genericgetProductsfunction, which aligns with the refactoring of entitlement and ability handling across the app. The function signature appears to be compatible as no other changes were needed in the component.Also applies to: 16-16
apps/ai-hero/src/lib/workshops-query.ts (1)
265-265: Debug log removal is appropriateRemoval of console.debug statement is a good cleanup. This appears to be part of broader cleanup of debug logs related to cohort rows and cohort resource rows in this file, which is appropriate for production code.
apps/ai-hero/src/app/(content)/workshops/[module]/[lesson]/(view)/layout.tsx (1)
29-46: Suspense wrapper removal looks goodRemoving the React.Suspense wrapper and its fallback UI streamlines the component rendering. This change aligns with updates to the WorkshopResourceList component which now accepts a
currentLessonSlugprop instead of usinguseParams, making the loading state handling more direct.apps/ai-hero/src/utils/get-current-ability-rules.ts (4)
51-52: Good addition: Now fetching entitlement types from databaseThis change supports the centralization of entitlement-based permission logic, providing necessary context to the ability rules.
86-86: Nice integration with ability rulesPassing the entitlement types to
defineRulesForPurchasesensures the permission system has access to all necessary metadata for cohort access checks.
110-110: Good permission check additionAdding the
canCreatepermission check is essential for supporting workshop content management in the UI, allowing authorized users to edit content.
116-117: Properly exposing the new permissionThe
canCreatepermission is correctly exposed in the returned object, making it available to components that need to check for creation rights.apps/ai-hero/src/app/(content)/workshops/_components/workshop-resource-list.tsx (6)
55-63: Improved ability rules query implementationGood improvements:
- Now properly passing
props.currentLessonSlugaslessonId- Added
enabledflag to prevent unnecessary queries- Query only runs when workshop navigation is available
This makes the query more efficient and correctly tied to the current lesson.
164-165: Consistent use of propsUsing
props.currentLessonSluginstead ofparams.lessonimproves consistency and makes the component more predictable, since it now relies on props passed from parent rather than route parameters.
283-284: Smart permission cachingCaching the permission check results improves performance by avoiding repeated calls to
ability.can()in the JSX template.
334-339: Clean conditional rendering with cached permissionUsing the cached
canViewmakes the code more readable and efficient than callingability.can()directly in the JSX.
344-355: Good implementation of conditional edit buttonThe edit button is now properly shown only to users with content creation permissions, using the cached
canCreatevalue.
33-33:❓ Verification inconclusive
Prop type refinement
Changed
currentLessonSlugfromstring | nullto juststring, ensuring more type safety. Make sure all callers provide a string value.
🏁 Script executed:
#!/bin/bash # Check if there are places where null might be passed to currentLessonSlug rg -A 3 -B 3 "currentLessonSlug={(.*)null" --glob "*.tsx" --glob "*.jsx"Length of output: 203
🏁 Script executed:
#!/bin/bash # Search for any instances where currentLessonSlug is explicitly set to null rg -n -C3 "currentLessonSlug\s*=\s*{\s*null\s*}" --glob "*.tsx" --glob "*.jsx"Length of output: 224
🏁 Script executed:
#!/bin/bash # Re-run search for any JSX prop passing null to currentLessonSlug, using POSIX spaces rg -n -C3 "currentLessonSlug[[:space:]]*=[[:space:]]*{[[:space:]]*null[[:space:]]*}" --glob "*.tsx" --glob "*.jsx"Length of output: 314
🏁 Script executed:
#!/bin/bash # Literal search for passing null as currentLessonSlug in JSX rg -F -n "currentLessonSlug={null}" --glob "*.tsx" --glob "*.jsx"Length of output: 67
#!/bin/bash # Find all usages of the <WorkshopResourceList /> component to confirm prop coverage rg -n "<WorkshopResourceList" --glob "*.tsx" --glob "*.jsx" -C2
Prop type refinement check
I couldn’t find any JSX usage that passesnullintocurrentLessonSlugor omits the prop entirely. It looks safe to tighten the type fromstring | nulltostring.Please manually verify any dynamic data sources or higher-level components feeding this prop never supply
null.apps/ai-hero/src/ability/index.ts (5)
199-202: Good type extension for entitlement typesAdding the optional
entitlementTypesproperty toViewerAbilityInputprovides necessary type safety for the new entitlement-based checks.
209-216: Clean destructuring of input parametersGood refactoring to destructure more variables from the input, making the code more readable.
313-316: Dynamic entitlement type identificationFinding the cohort entitlement type by name rather than hardcoding an ID is more maintainable and flexible.
318-326: Workshop-level cohort access controlThis block properly implements workshop-level access control based on entitlements, checking if the workshop ID is in the user's entitlement metadata.
328-339: Lesson-level cohort access checkThe lesson-level entitlement check is implemented correctly, but there's a TODO comment that should be addressed.
The comment on line 329 indicates that validation is needed. Consider implementing proper validation logic or removing the TODO if it's already validated elsewhere.
| @@ -2,6 +2,7 @@ import React from 'react' | |||
| import { WorkshopResourceList } from '@/app/(content)/workshops/_components/workshop-resource-list' | |||
| import LayoutClient from '@/components/layout-client' | |||
| import { ActiveHeadingProvider } from '@/hooks/use-active-heading' | |||
| import { getAbilityForResource } from '@/utils/get-current-ability-rules' | |||
There was a problem hiding this comment.
Unused import detected
The getAbilityForResource function is imported but not used in this file. Consider removing this import if it's not needed.
-import { getAbilityForResource } from '@/utils/get-current-ability-rules'📝 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.
| import { getAbilityForResource } from '@/utils/get-current-ability-rules' | |
| // (import removed as it was unused) |
| const { session } = await getServerAuthSession() | ||
| const cookieStore = await cookies() | ||
| const organizationId = cookieStore.get('organizationId')?.value | ||
|
|
||
| const abilityRules = await getCurrentAbilityRules({ | ||
| lessonId: input?.lessonId, | ||
| moduleId: input?.moduleId, | ||
| ...(organizationId && { | ||
| organizationId, | ||
| }), | ||
| }) | ||
|
|
||
| return abilityRules |
There was a problem hiding this comment.
took me a while to figure out why active entitlements weren't present in trpc context.
turns out await headers() used for getting organizationId doesn't work in trpc context so using cookies instead and passing it down to abilities.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ai-hero/src/ability/index.ts (1)
331-333: Consider enhancing lessonModule lookup commentWhile the code correctly finds a lesson within a module's resources, the comment could be more descriptive to explain why this check is necessary before granting lesson-level access.
- // lesson check - // TODO: validate + // Find if current lesson belongs to the module before checking lesson-level access + // TODO: validate that contentIds exists and is properly structured const lessonModule = module?.resources?.find( (resource) => resource.resourceId === lesson?.id, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ai-hero/src/ability/index.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (5)
apps/ai-hero/src/ability/index.ts (5)
182-185: Appropriate removal of global content access ruleThe commented out global content access rule makes sense in the context of this PR. As we're moving to an entitlement-based approach for cohort content access, we no longer want to grant broad access to all content with specific creation dates and statuses.
Note that the original comment from Joel about potentially changing from "Content" to "CohortContent" could still be considered if you want to make the permission model more specific in the future.
199-202: LGTM: Proper type extension for entitlement typesThe
ViewerAbilityInputtype is correctly extended with theentitlementTypesoptional property, which is essential for the cohort access feature. This allows passing entitlement type data from the database to the ability rules.
266-269: Good practice commenting out legacy codeClearly marking this as legacy non-entitlement based access helps developers understand why this code is being replaced. The PR correctly moves away from purchase-based content access to a more flexible entitlement-based approach for cohort workshops.
314-317: LGTM: Proper entitlement type lookupThe code correctly identifies the cohort entitlement type by looking for the one named 'cohort_content_access'. This approach allows for flexibility if additional entitlement types are added later.
319-327: Correct implementation of workshop-level content accessThe implementation correctly checks if the user has entitlements of type 'cohort_content_access' and grants read access to Content with IDs in the entitlement's metadata. Using
$inoperator is the right approach for checking against an array of contentIds.
| // TODO: validate | ||
| const lessonModule = module?.resources?.find( | ||
| (resource) => resource.resourceId === lesson?.id, | ||
| ) | ||
| if (user?.entitlements && lessonModule) { | ||
| user.entitlements.forEach((entitlement) => { | ||
| if (entitlement.type === cohortEntitlementType?.id) { | ||
| can('read', 'Content', { id: { $in: entitlement.metadata.contentIds } }) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODO comment and consider potential redundancy
The lesson-level check correctly uses $in operator as suggested in previous reviews. However, there are two issues to address:
- The "TODO: validate" comment should be addressed with specific validation requirements
- Consider potential redundancy between module-level and lesson-level checks - a user might get duplicate permissions if they have access to both a module and its lessons
Consider adding validation for entitlement.metadata.contentIds to ensure it's always an array:
if (entitlement.type === cohortEntitlementType?.id) {
+ // Ensure contentIds is an array before using $in operator
+ const contentIds = Array.isArray(entitlement.metadata.contentIds)
+ ? entitlement.metadata.contentIds
+ : [entitlement.metadata.contentIds];
- can('read', 'Content', { id: { $in: entitlement.metadata.contentIds } })
+ can('read', 'Content', { id: { $in: contentIds } })
}Also consider adding a comment explaining the purpose of the separate module and lesson level checks.
📝 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.
| // TODO: validate | |
| const lessonModule = module?.resources?.find( | |
| (resource) => resource.resourceId === lesson?.id, | |
| ) | |
| if (user?.entitlements && lessonModule) { | |
| user.entitlements.forEach((entitlement) => { | |
| if (entitlement.type === cohortEntitlementType?.id) { | |
| can('read', 'Content', { id: { $in: entitlement.metadata.contentIds } }) | |
| } | |
| }) | |
| } | |
| // TODO: validate | |
| const lessonModule = module?.resources?.find( | |
| (resource) => resource.resourceId === lesson?.id, | |
| ) | |
| if (user?.entitlements && lessonModule) { | |
| user.entitlements.forEach((entitlement) => { | |
| if (entitlement.type === cohortEntitlementType?.id) { | |
| // Ensure contentIds is an array before using $in operator | |
| const contentIds = Array.isArray(entitlement.metadata.contentIds) | |
| ? entitlement.metadata.contentIds | |
| : [entitlement.metadata.contentIds]; | |
| can('read', 'Content', { id: { $in: contentIds } }) | |
| } | |
| }) | |
| } |
…ion in ability rules
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ai-hero/src/utils/get-resource-section.ts (1)
13-15: Improved filtering logic for section resources.The addition of an early return when a resource's type is not 'section' makes the function more precise and efficient, ensuring that only relevant resources are processed.
Consider using
filter()beforeforEach()for better readability:- moduleResource.resources.forEach((section) => { - if (section.resource.type !== 'section') return + moduleResource.resources + .filter((section) => section.resource.type === 'section') + .forEach((section) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ai-hero/src/app/(content)/workshops/_components/video-overlay-pricing-widget.tsx(1 hunks)apps/ai-hero/src/utils/get-current-ability-rules.ts(4 hunks)apps/ai-hero/src/utils/get-resource-section.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ai-hero/src/utils/get-current-ability-rules.ts
🔇 Additional comments (2)
apps/ai-hero/src/app/(content)/workshops/_components/video-overlay-pricing-widget.tsx (2)
55-55: UI refinement with zero top padding.The addition of
pt-0to the className ensures consistent spacing in the pricing details component by explicitly setting the top padding to zero, maintaining alignment with the design system.
61-61: Improved user clarity with descriptive button text.The addition of "Enroll Now" text to the BuyButton provides clearer context to users about the action they're taking. This is more appropriate for workshop enrollment than a generic or empty button.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores