Changes: Update src/lib/permissions.ts to return a clear

[albertyosef]

Changes requested:

Update src/lib/permissions.ts to return a clear PermissionDenied result object (instead of throwing) for denied checks; adjust a single small call site if needed.

Please review.

Summary by CodeRabbit

• Bug Fixes

  • Improved authorization error handling and messaging for permission denials.

• Refactor

  • Enhanced permission checking system for better error reporting and standardized responses.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The pull request refactors permission-checking functions in permissions.ts to return a Result<boolean> type instead of plain boolean values, providing explicit success and error paths. The action layer in action.ts is updated to use the .success property from the returned Result object.

Changes

Cohort / File(s)	Summary
Permission functions refactoring src/lib/permissions.ts	Updated three exported permission functions (hasProjectPermission, isProjectAdmin, isProjectMember) to return Result<boolean> instead of boolean. Functions now emit success(true) on authorization passes and error<boolean>("Permission denied", ErrorCodes.FORBIDDEN) on failures. Added imports for Result, success, error, and ErrorCodes.
Authorization check update src/lib/action.ts	Modified protectedAction to check the .success property of the hasProjectPermission() return value instead of the boolean result. Error message changed from "Not authorized to perform this action on this project" to "Permission denied: Not authorized...".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Focus on verifying the Result type wrapping is consistent across all three permission functions
• Confirm the .success property check in action.ts correctly handles both success and error cases
• Ensure all callers of these permission functions throughout the codebase have been updated to handle the new Result return type (verify via codebase search)

Suggested reviewers

• fehranbit

Poem

🐰 Permission checks now wear Result's golden coat,
Success and error paths dance in the same boat,
Boolean days are gone, Result types now reign,
Authorization logic flows through the updated chain!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is incomplete and cuts off mid-sentence ('return a clear'), making it vague and unclear about the actual change being made.	Complete the title to clearly describe the full change, such as 'Update permission checks to return Result objects instead of boolean' or similar.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/20251217174022

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/lib/permissions.ts (1)

4-23: Consider removing redundant type parameters.

The <boolean> type parameter in error<boolean>(...) calls is redundant since TypeScript can infer it from the function's return type Result<boolean>.

Apply this diff to simplify the code:

 export function hasProjectPermission(userRole: Role, requiredRoles: Role[]): Result<boolean> {
   if (requiredRoles.includes(userRole)) {
     return success(true);
   }
-  return error<boolean>("Permission denied", ErrorCodes.FORBIDDEN);
+  return error("Permission denied", ErrorCodes.FORBIDDEN);
 }

 export function isProjectAdmin(userRole: Role): Result<boolean> {
   if (userRole === Role.ADMIN) {
     return success(true);
   }
-  return error<boolean>("Permission denied", ErrorCodes.FORBIDDEN);
+  return error("Permission denied", ErrorCodes.FORBIDDEN);
 }

 export function isProjectMember(userRole: Role): Result<boolean> {
   if (userRole === Role.USER || userRole === Role.ADMIN) {
     return success(true);
   }
-  return error<boolean>("Permission denied", ErrorCodes.FORBIDDEN);
+  return error("Permission denied", ErrorCodes.FORBIDDEN);
 }

src/lib/action.ts (1)

92-94: Logic is correct, but consider utilizing the Result error information.

The authorization check correctly handles the Result<boolean> return type using .success === false. The short-circuit evaluation ensures hasProjectPermission isn't called when projectMember is null/undefined, preventing runtime errors.

However, the error information (error message and errorCode) from the Result is being discarded. The hasProjectPermission function returns detailed error context (ErrorCodes.FORBIDDEN), but this is replaced with a generic error message.

Consider propagating the Result's error information:

- if (!projectMember || hasProjectPermission(projectMember.role, requiredRoles).success === false) {
-   throw new Error('Permission denied: Not authorized to perform this action on this project.');
+ if (!projectMember) {
+   throw new Error('Permission denied: Not a member of this project.');
+ }
+ 
+ const permissionResult = hasProjectPermission(projectMember.role, requiredRoles);
+ if (!permissionResult.success) {
+   throw new Error(permissionResult.error);
  }

This approach:

• Separates the membership check from the permission check for clarity
• Preserves the error message and context from the permission check
• Maintains the same error-throwing behavior expected by the middleware

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f71089 and b44b2eb.

📒 Files selected for processing (2)

• src/lib/action.ts (1 hunks)
• src/lib/permissions.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/lib/permissions.ts (1)

src/lib/result.ts (3)

• Result (18-18)
• success (25-30)
• error (38-44)

src/lib/action.ts (1)

src/lib/permissions.ts (1)

• hasProjectPermission (4-9)

🔇 Additional comments (2)

src/lib/permissions.ts (2)

2-2: LGTM!

The updated imports correctly include all the necessary types and functions for the Result pattern refactor.

---

4-9: Breaking API change is properly integrated across all call sites.

The function signature change from returning boolean to Result<boolean> is correct and aligns with the PR objectives. The logic properly returns success(true) when permission is granted and error<boolean>("Permission denied", ErrorCodes.FORBIDDEN) when denied.

All call sites have been updated. The single identified usage in src/lib/action.ts line 92 correctly handles the Result<boolean> return type by accessing the .success property: hasProjectPermission(projectMember.role, requiredRoles).success === false.

[fehranbit]

that clearer PermissionDenied return in permissions.ts is gonna help a lot with debugging! ✌️ Approved.