Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
📝 WalkthroughWalkthroughThe changes update the system to consistently use the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
Client->>API: Request permission-related action
API->>DB: Query permissions using slug
DB-->>API: Return permissions with slugs
API->>Client: Respond with permissions (slugs)
Possibly related PRs
Suggested reviewers
✨ 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/lib/trpc/routers/rbac.ts (1)
823-824: Clarify the intention of setting both name and slug to the same value.Currently, both
nameandslugare set to the sameslugvalue when creating new permissions. This might not be the intended behavior if these fields should serve different purposes (e.g., name for display, slug for identification).Consider if this is the intended behavior or if there should be a transformation applied:
const permission: Permission = { id: newId("permission"), workspaceId, - name: slug, + name: slug, // or apply some transformation for human-readable name slug: slug, description: null, updatedAtM: null, createdAtM: Date.now(), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/pkg/keys/service.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac.ts(3 hunks)internal/db/src/schema/rbac.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/dashboard/lib/trpc/routers/rbac.ts (3)
internal/db/src/types.ts (2)
Permission(22-22)InsertPermission(23-23)apps/dashboard/lib/audit.ts (1)
UnkeyAuditLog(58-97)internal/db/src/schema/rbac.ts (1)
permissions(14-41)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
internal/db/src/schema/rbac.ts (1)
20-20:Details
❓ Verification inconclusive
Verify migration safety for making slug non-nullable.
Making the
slugcolumn non-nullable is a breaking schema change. Ensure all existing permission records have valid slug values before applying this migration.
🏁 Script executed:
#!/bin/bash # Description: Check for any permission records with null slug values that would break the migration # Search for any database migration files or seed data that might need updating fd -e sql -e ts | xargs rg -l "permissions" | head -10 # Look for any existing permission creation logic that might not set slug rg -A 5 -B 5 "insert.*permissions|permissions.*insert" --type tsLength of output: 34951
🏁 Script executed:
#!/bin/bash # Inspect seed logic for permissions in event-generator.ts rg -A3 -B3 "schema\\.permissions" tools/local/src/cmd/seed/event-generator.ts || echo "No direct permission inserts found" rg -A3 -B3 "slug" tools/local/src/cmd/seed/event-generator.ts || echo "No slug assignments found in seed file"Length of output: 306
Verify no null slug values before applying migration
All code‐paths that create or seed permissions explicitly set
slug, and no seed files insert permissions without it. However, to guarantee zero downtime, please run:SELECT COUNT(*) AS null_slugs FROM permissions WHERE slug IS NULL;Ensure the result is
0before applying the.notNull()migration.apps/api/src/pkg/keys/service.ts (1)
280-284: LGTM: Consistent transition to using permission slugs.The changes correctly update both direct key permissions and role-based permissions to use
sluginstead ofname. This aligns well with the schema changes and maintains consistency in permission identification throughout the system.apps/dashboard/lib/trpc/routers/rbac.ts (2)
796-858: Consistent transition to slug-based permission handling.The function has been successfully updated to use slugs consistently throughout. The parameter change, database queries, and audit logging all align with the PR objective.
799-799:Details
✅ Verification successful
Verify all callers are updated to pass slugs instead of names.
Ensure that all callers of this function have been updated to pass permission slugs rather than names.
🏁 Script executed:
#!/bin/bash # Description: Find all usages of upsertPermissions to verify they pass slugs # Search for all calls to upsertPermissions rg -A 3 -B 3 "upsertPermissions" --type ts # Look for any remaining usage of permission names in similar contexts rg -A 5 -B 5 "permission.*name|\.name.*permission" --type tsLength of output: 69565
🏁 Script executed:
#!/bin/bash # Inspect input schema for createRootKey echo "=== createRootKey input schema ===" rg -n ".input" -A5 -B5 apps/dashboard/lib/trpc/routers/key/createRootKey.ts # Inspect input schema for addPermissionToRootKey echo "=== addPermissionToRootKey input schema ===" rg -n "export const addPermissionToRootKey" -A10 -B2 apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.ts rg -n ".input" -A5 -B5 apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.tsLength of output: 4020
All upsertPermissions calls now pass validated slugs
We’ve confirmed that every invocation ofupsertPermissionsin the TRPC routers supplies string slugs (not names):
- apps/dashboard/lib/trpc/routers/key/createRootKey.ts → uses
input.permissions- apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.ts → uses
permission.data- apps/dashboard/lib/trpc/routers/rbac.ts → bulk calls pass
permission.dataNo callers remain passing permission names. You can safely resolve this.
this is unrelated but stopped the CI from running
…nkey into permission-slug-step-2
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/api/src/routes/v1_keys_removePermissions.ts (1)
116-128:⚠️ Potential issueFix the permission filtering logic.
The filtering logic has an issue - it should return
truewhen a match is found, but currently doesn't have explicit return statements for the matching cases.const deletePermissions = connectedPermissions.filter((cr) => { for (const deleteRequest of req.permissions) { if ("id" in deleteRequest) { - return cr.permissionId === deleteRequest.id; + if (cr.permissionId === deleteRequest.id) { + return true; + } } if ("slug" in deleteRequest) { - return cr.permission.slug === deleteRequest.slug; + if (cr.permission.slug === deleteRequest.slug) { + return true; + } } if ("name" in deleteRequest) { - return cr.permission.slug === deleteRequest.name; + if (cr.permission.slug === deleteRequest.name) { + return true; + } } } + return false; });
🧹 Nitpick comments (2)
apps/api/src/routes/v1_keys_removePermissions.happy.test.ts (1)
19-25: Consider test consistency and naming clarity.The first test now uses the same UUID for both
nameandslugfields, while the second test uses different values. This inconsistency could lead to confusion about the expected behavior during the migration.Consider either:
- Updating the test name to reflect that it's testing slug-based removal with backward compatibility
- Ensuring both tests follow the same pattern for name/slug assignment
-test("removes permission by name", async (t) => { +test("removes permission by name (backward compatibility)", async (t) => {apps/api/src/routes/v1_keys_setPermissions.ts (1)
30-32: Update field description to reflect new slug priority.The description for the
idfield still mentionsnameinstead ofslug.description: - "The id of the permission. Provide either `id` or `name`. If both are provided `id` is used.", + "The id of the permission. Provide either `id` or `slug`. If both are provided `id` is used.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/routes/v1_apis_listKeys.ts(1 hunks)apps/api/src/routes/v1_keys_removePermissions.happy.test.ts(1 hunks)apps/api/src/routes/v1_keys_removePermissions.ts(2 hunks)apps/api/src/routes/v1_keys_setPermissions.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/api/src/routes/v1_keys_removePermissions.happy.test.ts (1)
internal/id/src/generate.ts (1)
newId(33-52)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/api/src/routes/v1_apis_listKeys.ts (1)
222-224: LGTM! Permission extraction updated correctly.The changes properly migrate from using
permission.nametopermission.slugfor both direct permissions and role-based permissions. This aligns with the broader codebase migration to use slugs as the primary permission identifier.apps/api/src/routes/v1_keys_removePermissions.ts (1)
28-41: LGTM! Schema migration implemented correctly.The schema changes properly implement the migration from
nametoslug:
- New
slugfield with clear descriptionnamefield marked as deprecated with migration guidance- Updated field descriptions reflect the new priority
apps/api/src/routes/v1_keys_setPermissions.ts (4)
28-41: LGTM! Schema migration implemented correctly.The schema changes properly implement the migration from
nametoslugwith appropriate deprecation warnings and clear descriptions.
133-136: LGTM! Slug extraction logic implemented correctly.The logic properly extracts slugs from the request, falling back to
namewhenslugis not provided, maintaining backward compatibility.
246-251: LGTM! Permission creation updated correctly.The permission creation logic properly sets both
nameandslugto the same value during the transition period, ensuring consistency.
182-193:Details
✅ Verification successful
Review the permission filtering logic.
The filtering logic for
disconnectPermissionshas become complex with the slug migration. Ensure that permissions are properly identified for disconnection when slug is present.The logic at lines 186-193 filters permissions to disconnect based on whether they match requested permissions. Verify that this logic correctly handles the case where a permission has both name and slug fields.
🏁 Script executed:
#!/bin/bash # Description: Check for any existing permissions without slug values in the database schema # Expected: All permissions should have slug values if the migration is complete ast-grep --pattern $'slug: $_ { $$$ }'Length of output: 41
🏁 Script executed:
#!/bin/bash # Search for Permission entity definition rg -n "@Entity.*Permission" -A 5 . # Find slug column declarations in entities rg -n "@Column.*slug" -A 5 . # Check migrations for any slug-related changes rg -n "slug" -n migrationsLength of output: 145
🏁 Script executed:
#!/bin/bash # Locate Prisma schema if present prisma_schema=$(find . -type f -name "schema.prisma") if [ -n "$prisma_schema" ]; then echo "Found Prisma schema: $prisma_schema" rg -n "model Permission" -A 5 "$prisma_schema" rg -n "slug" -A 5 "$prisma_schema" fi # Search for Permission entity/model definitions rg -n "class Permission" -A 5 . rg -n "interface Permission" -A 5 . rg -n "type Permission" -A 5 . # Search for slug property usages in API source rg -n "slug" -n apps/api/src -A 3Length of output: 31279
Permission filtering logic is valid as implemented
The
disconnectPermissionsfilter correctly:
- Excludes any permission whose ID is in
requestedIds- Excludes any permission whose slug matches an entry in
requestedSlugs- Guards against orphaned records lacking both
slugandname(a defensive check that shouldn’t trigger once slug migration is complete)No further changes are required here.
1 participant
Summary by CodeRabbit
New Features
slugfor permission identification, markingnameas deprecated.Bug Fixes
Refactor
Documentation
Tests