refactor: extract shared hasPermission helper (MODSetter/SurfSense#1366)#1428
Merged
MODSetter merged 1 commit intoMay 23, 2026
Conversation
- Add canPerform() helper function to members-query.atoms.ts - Add usePermissionGate() hook for convenience - Update team-content.tsx to use canPerform() - Update roles-manager.tsx to use canPerform() - Eliminates duplicated permission check logic - Centralizes permission policy in one location Fixes MODSetter#1366
|
@guangyang1206 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR consolidates duplicate permission-checking logic from two components into a shared helper. A reusable ChangesPermission-Check Consolidation
🎯 2 (Simple) | ⏱️ ~8 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 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
surfsense_web/components/settings/roles-manager.tsx (1)
259-269:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSyntax error: duplicate callback body — old code was not removed.
The refactor left the old inline permission check in place (lines 263-269) alongside the new
canPerformcall (lines 260-262). This creates invalid JavaScript syntax and will fail to compile.🐛 Proposed fix — remove the leftover code
const hasPermission = useCallback( (permission: string) => canPerform(access, permission), [access] ); - (permission: string) => { - if (!access) return false; - if (access.is_owner) return true; - return access.permissions?.includes(permission) ?? false; - }, - [access] - );🤖 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 `@surfsense_web/components/settings/roles-manager.tsx` around lines 259 - 269, The file contains a duplicated callback body: the useCallback assigned to hasPermission should call canPerform(access, permission) but the old inline function (the anonymous permission check using access.is_owner and access.permissions) was left below, causing a syntax error; remove the leftover anonymous function block (the second function and its trailing dependency array) and keep the single useCallback definition for hasPermission that references canPerform and the [access] dependency so hasPermission = useCallback((permission: string) => canPerform(access, permission), [access]).
🤖 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 `@surfsense_web/app/dashboard/`[search_space_id]/team/team-content.tsx:
- Line 34: The import line importing membersAtom, myAccessAtom, and canPerform
in team-content.tsx violates Biome's organizeImports alphabetical rule; reorder
the named imports inside the curly braces alphabetically (canPerform,
membersAtom, myAccessAtom) so the import becomes sorted and re-run the pipeline
to ensure the Biome organizeImports check passes.
In `@surfsense_web/atoms/members/members-query.atoms.ts`:
- Around line 73-76: The hook usePermissionGate currently calls
useAtomValue(myAccessAtom) without importing useAtomValue and treats the atom as
returning raw access; import useAtomValue from 'jotai', then call const { data:
access } = useAtomValue(myAccessAtom) (or similar destructuring) and pass that
access into canPerform(access, permission) so usePermissionGate uses the atom's
.data field rather than the query wrapper.
---
Outside diff comments:
In `@surfsense_web/components/settings/roles-manager.tsx`:
- Around line 259-269: The file contains a duplicated callback body: the
useCallback assigned to hasPermission should call canPerform(access, permission)
but the old inline function (the anonymous permission check using
access.is_owner and access.permissions) was left below, causing a syntax error;
remove the leftover anonymous function block (the second function and its
trailing dependency array) and keep the single useCallback definition for
hasPermission that references canPerform and the [access] dependency so
hasPermission = useCallback((permission: string) => canPerform(access,
permission), [access]).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a28d8ad4-ffeb-433e-9401-2111800c164e
📒 Files selected for processing (3)
surfsense_web/app/dashboard/[search_space_id]/team/team-content.tsxsurfsense_web/atoms/members/members-query.atoms.tssurfsense_web/components/settings/roles-manager.tsx
| updateMemberMutationAtom, | ||
| } from "@/atoms/members/members-mutation.atoms"; | ||
| import { membersAtom, myAccessAtom } from "@/atoms/members/members-query.atoms"; | ||
| import { membersAtom, myAccessAtom, canPerform } from "@/atoms/members/members-query.atoms"; |
There was a problem hiding this comment.
Fix import ordering to resolve pipeline failure.
The Biome organizeImports check failed. Named imports should be sorted alphabetically.
Proposed fix
-import { membersAtom, myAccessAtom, canPerform } from "`@/atoms/members/members-query.atoms`";
+import { canPerform, membersAtom, myAccessAtom } from "`@/atoms/members/members-query.atoms`";📝 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
| import { membersAtom, myAccessAtom, canPerform } from "@/atoms/members/members-query.atoms"; | |
| import { canPerform, membersAtom, myAccessAtom } from "`@/atoms/members/members-query.atoms`"; |
🤖 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 `@surfsense_web/app/dashboard/`[search_space_id]/team/team-content.tsx at line
34, The import line importing membersAtom, myAccessAtom, and canPerform in
team-content.tsx violates Biome's organizeImports alphabetical rule; reorder the
named imports inside the curly braces alphabetically (canPerform, membersAtom,
myAccessAtom) so the import becomes sorted and re-run the pipeline to ensure the
Biome organizeImports check passes.
Comment on lines
+73
to
+76
| export function usePermissionGate(permission: string): boolean { | ||
| const access = useAtomValue(myAccessAtom); | ||
| return canPerform(access, permission); | ||
| } |
There was a problem hiding this comment.
Missing import and incorrect data access will break usePermissionGate.
useAtomValueis used but never imported fromjotai.myAccessAtomis created viaatomWithQuery, souseAtomValue(myAccessAtom)returns the query result object{ data, isLoading, ... }, not the access data directly. You need to destructure.data.
🐛 Proposed fix
Add the import at the top of the file:
import { atomWithQuery } from "jotai-tanstack-query";
+import { useAtomValue } from "jotai";
import { activeSearchSpaceIdAtom } from "`@/atoms/search-spaces/search-space-query.atoms`";Then fix the hook implementation:
export function usePermissionGate(permission: string): boolean {
- const access = useAtomValue(myAccessAtom);
- return canPerform(access, permission);
+ const { data: access } = useAtomValue(myAccessAtom);
+ return canPerform(access ?? null, permission);
}📝 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
| export function usePermissionGate(permission: string): boolean { | |
| const access = useAtomValue(myAccessAtom); | |
| return canPerform(access, permission); | |
| } | |
| export function usePermissionGate(permission: string): boolean { | |
| const { data: access } = useAtomValue(myAccessAtom); | |
| return canPerform(access ?? null, permission); | |
| } |
🤖 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 `@surfsense_web/atoms/members/members-query.atoms.ts` around lines 73 - 76, The
hook usePermissionGate currently calls useAtomValue(myAccessAtom) without
importing useAtomValue and treats the atom as returning raw access; import
useAtomValue from 'jotai', then call const { data: access } =
useAtomValue(myAccessAtom) (or similar destructuring) and pass that access into
canPerform(access, permission) so usePermissionGate uses the atom's .data field
rather than the query wrapper.
9 tasks
MODSetter
pushed a commit
that referenced
this pull request
May 25, 2026
PR #1428 (issue #1366) extracted the inline `hasPermission` callback into a shared `canPerform` helper but left the original arrow-function body, its dependency array, and trailing `)` behind after the new `useCallback` block. The result was a syntactically invalid statement that broke `pnpm build` on the `dev` branch and is now blocking every E2E job in the PR queue. Delete the orphaned lines so the file parses again. No behavior change — the working `useCallback(canPerform(access, permission))` already supplies the same predicate the duplicated body did.
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
2 participants
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.
Fixes #1366
Description
Problem
The permission check predicate is byte-identical in two settings/team components:
surfsense_web/app/dashboard/[search_space_id]/team/team-content.tsx:128surfsense_web/components/settings/roles-manager.tsx:259Both get
accessfromuseAtomValue(myAccessAtom). Other components that need the same check also readmyAccessAtomand would benefit from the shared helper.If the owner-bypass semantics ever change, every duplicated copy must be updated in lockstep.
Solution
This PR extracts the duplicated permission check logic into a shared helper function
canPerform()added tosurfsense_web/atoms/members/members-query.atoms.ts, alongsidemyAccessAtom.Changes:
Add
canPerform()helper function inmembers-query.atoms.ts:accessandpermissionas argumentsbooleanindicating if the user has the permissionnull/undefinedaccess,is_ownerbypass, and permission array lookupAdd
usePermissionGate()hook (optional convenience wrapper):myAccessAtominternally(perm: string) => booleanuseAtomValue(myAccessAtom)separatelyUpdate
team-content.tsx:canPerformfrom atoms fileuseCallbackwith call tocanPerform(access, permission)Update
roles-manager.tsx:canPerformfrom atoms fileuseCallbackwith call tocanPerform(access, permission)Benefits
useCallbacks embedded in 800-line components.access.is_owneroraccess.permissions?.includes(...)inline can adopt the helper in follow-up PRs.Testing
npm run lintandnpm run type-checkRelated Issues
Fixes #1366
PR Link
main...guangyang1206:SurfSense:fix/extract-shared-haspermission-helper-1366
Branch
guangyang1206:fix/extract-shared-haspermission-helper-1366High-level PR Summary
This PR extracts duplicated permission checking logic into a shared
canPerform()helper function andusePermissionGate()hook. The helper centralizes the permission validation logic (including owner bypass and null handling) that was previously duplicated inteam-content.tsxandroles-manager.tsx, making the permission policy easier to maintain and test in one location.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_web/atoms/members/members-query.atoms.tssurfsense_web/app/dashboard/[search_space_id]/team/team-content.tsxsurfsense_web/components/settings/roles-manager.tsxsurfsense_web/components/settings/roles-manager.tsxSummary by CodeRabbit