Add new permission flags for tagging, changing sidebar groups, creating new field for similarity search #4379
Conversation
WalkthroughThe updates enhance the permission system by introducing new permission types for various actions, such as modifying sidebar groups, creating new fields, tagging samples or labels, and editing custom colors and saved views. These permissions are now managed using objects with Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
lanzhenw
changed the title
|
@coderabbitai review |
Actions PerformedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- app/packages/app/src/useWriters/registerWriter.ts (1 hunks)
- app/packages/core/src/components/Actions/ActionsRow.tsx (1 hunks)
- app/packages/core/src/components/Actions/similar/Similar.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1 hunks)
- app/packages/state/src/recoil/atoms.ts (1 hunks)
- app/packages/state/src/session.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/state/src/recoil/atoms.ts
Additional Context Used
Path-based Instructions (7)
app/packages/app/src/useWriters/registerWriter.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/state/src/session.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Actions/similar/Similar.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Actions/ActionsRow.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (6)
app/packages/app/src/useWriters/registerWriter.ts (1)
9-14: Ensure that the new permissions added toWriterKeysare correctly utilized in the application.Verification successful
The verification process has confirmed that the new permissions added to the
WriterKeystype are indeed utilized throughout the application. The permissions are referenced in various files, indicating their integration into the system's functionality. Here are some key points:
- Permissions like
canEditCustomColors,canEditSavedViews,canCreateNewField,canAddSidebarGroup,canTagSamples, andreadOnlyare used in state management and UI components, ensuring that these permissions control relevant features and access within the application.- The permissions are used in conditional logic to enable or disable UI components and functionalities based on the state of these permissions, which is a correct implementation following the addition of these permissions.
This thorough utilization across different modules of the application confirms that the integration of the new permissions is comprehensive and aligned with the system's requirements.
- File:
app/packages/state/src/session.ts- File:
app/packages/state/src/recoil/atoms.ts- File:
app/packages/spaces/src/components/Workspaces/hooks.ts- File:
app/packages/core/src/plugins/SchemaIO/utils/generate-schema.ts- File:
app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx- File:
app/packages/core/src/components/Actions/ActionsRow.tsx- File:
app/packages/core/src/components/ColorModal/ColorFooter.tsxScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new permissions are utilized in the application. # Test: Search for the usage of new permissions. Expect: At least one occurrence for each permission. rg --type typescript $'canAddSidebarGroup|canCreateNewField|canTagSamples|canEditCustomColors|canEditSavedViews|readOnly'Length of output: 157
Script:
#!/bin/bash # Description: Verify that the new permissions are utilized in the application. # Test: Search for the usage of new permissions. Expect: At least one occurrence for each permission. rg --type ts $'canAddSidebarGroup|canCreateNewField|canTagSamples|canEditCustomColors|canEditSavedViews|readOnly'Length of output: 11322
app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (1)
45-45: Ensure that the condition for renderingnullis correctly implemented based on the newisReadOnlylogic.app/packages/state/src/session.ts (2)
32-34: Ensure that the new boolean flags in theSessioninterface are correctly utilized in the application.Verification successful
The new boolean flags
canCreateNewField,canAddSidebarGroup, andcanTagSamplesin theSessioninterface are indeed utilized throughout the application. Here are the key points of their usage:
- Declaration and Initialization: These flags are declared in
session.tsand initialized with boolean values.- Usage in Conditional Logic: They are used in various conditional checks within the application, ensuring that certain functionalities are enabled or disabled based on these flags.
- Recoil State Management: They are also part of the state management in
recoil/atoms.ts, indicating their role in reactive state changes across the application.- Component-Level Access: Various components like
Draggable.tsx,ActionsRow.tsx, andSimilar.tsxaccess these flags to determine UI behavior, such as enabling or disabling elements and modifying component states based on the session permissions.This confirms that the flags are integrated and functional within the codebase, addressing the concerns raised in the review comment.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new boolean flags are utilized in the application. # Test: Search for the usage of new boolean flags. Expect: At least one occurrence for each flag. rg --type typescript $'canCreateNewField|canAddSidebarGroup|canTagSamples'Length of output: 110
Script:
#!/bin/bash # Description: Re-verify the usage of new boolean flags in the application by searching TypeScript files. # Test: Search for the usage of new boolean flags in files with TypeScript extensions. Expect: At least one occurrence for each flag. rg 'canCreateNewField|canAddSidebarGroup|canTagSamples' --glob '*.ts' --glob '*.tsx'Length of output: 2400
186-189: Verify that the conditions for setting new boolean flags are correctly implemented.app/packages/core/src/components/Actions/similar/Similar.tsx (2)
96-96: Ensure that thecanCreateNewFieldpermission is correctly utilized in the application.Verification successful
The search results indicate that the
canCreateNewFieldpermission is indeed utilized across various parts of the application. It is defined in thesession.tsfile as a boolean, set to true in some initial settings, and used as a key in session-related operations. Additionally, it is exported fromatoms.tsas a Recoil state, and its value is consumed inSimilar.tsxto conditionally disable UI elements based on theisReadOnlystate and thecanCreateNewFieldpermission. It is also referenced inregisterWriter.tsas part of a union type, suggesting its use in function parameters or similar constructs.Given this comprehensive usage across different modules and components, it appears that the
canCreateNewFieldpermission is correctly implemented and integrated within the application's state management and UI logic.Usage of
canCreateNewFieldPermission:
- Defined and initialized in
session.ts.- Exported as a Recoil state in
recoil/atoms.ts.- Used in
Similar.tsxto conditionally disable UI components.- Referenced in
registerWriter.tsindicating broader usage in application logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `canCreateNewField` permission is utilized in the application. # Test: Search for the usage of `canCreateNewField`. Expect: Correct implementation. rg --type typescript $'canCreateNewField'Length of output: 75
Script:
#!/bin/bash # Description: Verify that the `canCreateNewField` permission is utilized in the application. # Test: Search for the usage of `canCreateNewField`. Expect: Correct implementation. rg --type ts 'canCreateNewField'Length of output: 767
285-285: Verify that the condition for disabling the input field based on theisReadOnlyandcanCreateNewFieldstates is correctly implemented.Verification successful
The condition
isReadOnly && canCreateNewFieldused for disabling the input field in the fileSimilar.tsxis implemented as specified in the code snippet provided. The search confirmed that this is the only occurrence of this specific condition in the TypeScript files of the project, which aligns with the implementation details provided in the review comment.
- Location:
app/packages/core/src/components/Actions/similar/Similar.tsxScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the condition for disabling the input field is correctly implemented. # Test: Search for the usage of `isReadOnly` and `canCreateNewField` in conditional rendering. Expect: Correct implementation. rg --type typescript $'isReadOnly && canCreateNewField'Length of output: 91
Script:
#!/bin/bash # Description: Re-verify the condition for disabling the input field by searching for the usage of `isReadOnly && canCreateNewField` in the codebase. # Correcting the previous script error by not specifying the file type explicitly. rg 'isReadOnly && canCreateNewField' --glob '*.tsx' --glob '*.ts'Length of output: 183
app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx
Outdated
Show resolved
Hide resolved
| const isSnapShot = useRecoilValue(fos.readOnly); | ||
| const canEdit = useMemo( | ||
| () => canEditSavedViews && !isReadOnly, | ||
| [canEditSavedViews, isReadOnly] | ||
| () => canEditSavedViews && !isSnapShot, | ||
| [canEditSavedViews, isSnapShot] |
There was a problem hiding this comment.
Refactor the canEdit state determination to improve readability and maintainability.
- const isSnapShot = useRecoilValue(fos.readOnly);
- const canEdit = useMemo(
- () => canEditSavedViews && !isSnapShot,
- [canEditSavedViews, isSnapShot]
- );
+ const canEdit = useRecoilValue(fos.canEditSavedViews) && !useRecoilValue(fos.readOnly);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.
| const isSnapShot = useRecoilValue(fos.readOnly); | |
| const canEdit = useMemo( | |
| () => canEditSavedViews && !isReadOnly, | |
| [canEditSavedViews, isReadOnly] | |
| () => canEditSavedViews && !isSnapShot, | |
| [canEditSavedViews, isSnapShot] | |
| const canEdit = useRecoilValue(fos.canEditSavedViews) && !useRecoilValue(fos.readOnly); |
There was a problem hiding this comment.
Outside of the scope of this PR, so feel free to follow-up or defer:
- Can we add support for restricting renaming sidebar group?
- Can we add support for restricting deleting sidebar group?
I updated the flag name to |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/packages/app/src/useWriters/registerWriter.ts (1 hunks)
- app/packages/core/src/components/Actions/ActionsRow.tsx (1 hunks)
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (2 hunks)
- app/packages/state/src/recoil/atoms.ts (1 hunks)
- app/packages/state/src/session.ts (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/packages/app/src/useWriters/registerWriter.ts
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx
- app/packages/state/src/recoil/atoms.ts
Additional Context Used
Path-based Instructions (1)
app/packages/state/src/session.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (3)
app/packages/state/src/session.ts (3)
32-34: The addition of new permission flags to theSessioninterface aligns well with the PR's objectives and enhances the application's permission management capabilities.
72-78: Excluding the new permission flags fromSetterKeysis a prudent security measure, ensuring that these permissions are not modified inadvertently.
186-189: The modifications in thesessionAtomfunction to prevent setting new permission flags add an important security layer, ensuring that permissions are managed carefully.
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (2)
213-213: Clarify interaction design for the edit icon in group entries.The edit icon only appears when the user is hovering over the group entry, is not currently editing, and has the permission to modify the group. This is a subtle UI behavior that might not be immediately obvious to all users. Consider adding a tooltip or a visual indicator when the edit option is unavailable due to lack of permissions or other conditions.
234-234: Enhance visibility and accessibility of the delete icon.Similar to the edit icon, the delete icon's visibility is conditional based on several factors. To improve user experience, especially for accessibility, consider implementing additional visual cues or tooltips to explain why the delete option might be unavailable.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3 hunks)
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx
Additional Context Used
Path-based Instructions (1)
app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx
Outdated
Show resolved
Hide resolved
This is still following the same pattern as before. We decoupled "!canEdit" from |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (3 hunks)
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3 hunks)
- app/packages/state/src/recoil/atoms.ts (1 hunks)
- app/packages/state/src/session.ts (4 hunks)
Files skipped from review as they are similar to previous changes (6)
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx
- app/packages/state/src/recoil/atoms.ts
- app/packages/state/src/session.ts
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4379 +/- ##
============================================
+ Coverage 16.02% 99.27% +83.24%
============================================
Files 804 47 -757
Lines 89221 16155 -73066
Branches 1340 0 -1340
============================================
+ Hits 14300 16038 +1738
+ Misses 74921 117 -74804
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/Actions/similar/Similar.tsx (2 hunks)
- app/packages/core/src/components/ColorModal/ColorFooter.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (5 hunks)
- app/packages/spaces/src/components/Workspaces/hooks.ts (1 hunks)
- app/packages/state/src/recoil/atoms.ts (1 hunks)
- app/packages/state/src/session.ts (4 hunks)
Files not reviewed due to errors (1)
- app/packages/core/src/components/ColorModal/ColorFooter.tsx (no review received)
Files skipped from review due to trivial changes (1)
- app/packages/spaces/src/components/Workspaces/hooks.ts
Files skipped from review as they are similar to previous changes (6)
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/Actions/similar/Similar.tsx
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx
- app/packages/state/src/recoil/atoms.ts
- app/packages/state/src/session.ts
Additional Context Used
Path-based Instructions (2)
app/packages/core/src/components/ColorModal/ColorFooter.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (4)
app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (4)
47-49: The logic for determiningdisabledanddisabledMsgbased oncanEditSavedViewsis correct.
Line range hint
147-162: Adding a keyboard event listener that checks thedisabledstate ensures that the keyboard shortcut for saving views is only active when editing is allowed.
168-168: Setting thecanEditproperty of theViewDialogcomponent based oncanEditSavedViewsstate is correct.
Line range hint
206-244: Setting thereadonly,disabled, andtitleproperties of theSelectioncomponent and its child elements based oncanEditSavedViewsstate is correct.
lanzhenw
force-pushed
the
lanny/permissionUpdate
branch
from
682cbf9
to
c28e7d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/Actions/similar/Similar.tsx (2 hunks)
- app/packages/core/src/components/ColorModal/ColorFooter.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (5 hunks)
- app/packages/spaces/src/components/Workspaces/Workspace.tsx (4 hunks)
- app/packages/spaces/src/components/Workspaces/hooks.ts (2 hunks)
- app/packages/spaces/src/components/Workspaces/index.tsx (3 hunks)
- app/packages/state/src/recoil/atoms.ts (1 hunks)
- app/packages/state/src/session.ts (4 hunks)
Files skipped from review as they are similar to previous changes (9)
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/Actions/similar/Similar.tsx
- app/packages/core/src/components/ColorModal/ColorFooter.tsx
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx
- app/packages/spaces/src/components/Workspaces/hooks.ts
- app/packages/state/src/recoil/atoms.ts
- app/packages/state/src/session.ts
Additional Context Used
Path-based Instructions (2)
app/packages/spaces/src/components/Workspaces/Workspace.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/spaces/src/components/Workspaces/index.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (10)
app/packages/spaces/src/components/Workspaces/Workspace.tsx (5)
2-2: ImportingcanEditWorkspacesfrom@fiftyone/statelooks good. Ensure that this atom is correctly defined and used in the state management.
19-21: UsinguseRecoilValueto get thecanEditWorkspacesstate is appropriate. The logic to determinedisabledanddisabledMsgbased oncanEdit.enabledandcanEdit.messageis clear and concise.
49-52: Good use of conditional styling and title attribute to handle the disabled state and display the appropriate message. This enhances the user experience by providing feedback on why an action is disabled.
Line range hint
57-71: The click handler correctly checks thedisabledstate before proceeding with the edit action. This ensures that users without the necessary permissions cannot trigger the edit functionality.
71-74: Setting thedisabledprop and dynamically changing the color of theEditicon based on thedisabledstate is a good practice. This provides a visual cue to the user about the button's state.app/packages/spaces/src/components/Workspaces/index.tsx (5)
2-2: ImportingcanEditWorkspacesandsessionSpacesfrom@fiftyone/stateis appropriate. Ensure that these atoms are correctly defined and used in the state management.
31-33: UsinguseRecoilValueto get thecanEditWorkspacesandsessionSpacesstates is appropriate. The logic to determinedisabledanddisabledMsgbased oncanEditWorkSpace.enabledandcanEditWorkSpace.messageis clear and concise.
137-139: Good use of conditional styling and title attribute to handle the disabled state and display the appropriate message. This enhances the user experience by providing feedback on why an action is disabled.
144-146: The click handler correctly checks thedisabledstate before proceeding with the save action. This ensures that users without the necessary permissions cannot trigger the save functionality.
144-144: Setting thedisabledprop on theListItemButtonbased on thedisabledstate is a good practice. This provides a visual cue to the user about the button's state.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/ColorModal/ColorFooter.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/ColorModal/ColorFooter.tsx
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx
| @@ -162,13 +162,14 @@ const Tag = ({ | |||
| const [available, setAvailable] = useState(true); | |||
| const labels = useRecoilValue(fos.selectedLabelIds); | |||
| const samples = useRecoilValue(fos.selectedSamples); | |||
| const readOnly = useRecoilValue(fos.readOnly); | |||
| const canTag = useRecoilValue(fos.canTagSamplesOrLabels); | |||
There was a problem hiding this comment.
cool, this should work even after introducing the TAG permission since TAG < EDIT. so no feature flagging for dynamic collaborator is required on teams side.
| @@ -207,7 +209,7 @@ const GroupEntry = React.memo( | |||
| } | |||
| }} | |||
| /> | |||
| {hovering && !editing && setValue && ( | |||
| {hovering && !editing && setValue && !disabled && ( | |||
There was a problem hiding this comment.
should we disable it instead of hiding it? and show the message passed down? canModifySidebarGroup.message
| @@ -228,7 +230,7 @@ const GroupEntry = React.memo( | |||
| </span> | |||
| )} | |||
| {pills} | |||
| {onDelete && !editing && ( | |||
| {onDelete && !editing && !disabled && ( | |||
app/packages/core/src/components/Sidebar/ViewSelection/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (5 hunks)
Additional Context Used
Path-based Instructions (2)
app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Actions/ActionsRow.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (5)
app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (3)
47-48: Ensure consistent handling of thedisabledstate based oncanEditSavedViews.The logic for determining the
disabledstate and message is correctly implemented based on theenabledproperty ofcanEditSavedViews. This aligns with the PR's objective to manage permissions more granularly.
Line range hint
167-205: Check the propagation of thedisabledstate in child components.The
disabledstate is used to control the editability of the view dialog and other interactive elements. It's crucial to ensure that this state is consistently propagated to all child components that depend on it for correct functionality.Also applies to: 237-243
Verification successful
The
disabledstate is correctly propagated to child components within the fileapp/packages/core/src/components/Sidebar/ViewSelection/index.tsx.
disabled={isEmptyView || disabled}<AddIcon fontSize="small" disabled={isEmptyView || disabled} /><TextContainer disabled={isEmptyView || disabled}>Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `disabled` state is correctly propagated to all child components that depend on it. # Test: Search for usage of the `disabled` prop in child components within the same file. Expect: All relevant components should receive the `disabled` prop. rg --type tsx "disabled=" app/packages/core/src/components/Sidebar/ViewSelection/index.tsxLength of output: 118
Script:
#!/bin/bash # Description: Verify that the `disabled` state is correctly propagated to all child components that depend on it. # Test: Search for usage of the `disabled` prop in child components within the same file. Expect: All relevant components should receive the `disabled` prop. rg "disabled=" app/packages/core/src/components/Sidebar/ViewSelection/index.tsxLength of output: 273
Line range hint
146-161: Review the keyboard event handling logic for consistency and correctness.The logic here prevents default behavior and checks for the
KeySshortcut, which seems intended for saving operations. Ensure this is consistent with other parts of the application where keyboard shortcuts are used.app/packages/core/src/components/Actions/ActionsRow.tsx (2)
165-166: Ensure thecanTagpermission is correctly utilized for enabling tagging functionality.The use of
canTag.enabledto determine thedisableTagstate is appropriate and aligns with the PR's objectives to manage permissions effectively. This ensures that the tagging functionality is only available when permitted.
Line range hint
172-200: Review the logic for handling the disabled state and interaction in the tagging component.The logic combines multiple conditions (
tagging,disableTag, andavailable) to determine the overalldisabledstate for the tagging functionality. It's important to ensure that this pattern is consistent with other components that use a similar approach.
….tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…Group permission flag
Co-authored-by: manivoxel51 <109545780+manivoxel51@users.noreply.github.com>
Co-authored-by: manivoxel51 <109545780+manivoxel51@users.noreply.github.com>
lanzhenw
force-pushed
the
lanny/permissionUpdate
branch
from
68ec461
to
99dc2cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (13)
- app/packages/app/src/useWriters/registerWriter.ts (1 hunks)
- app/packages/core/src/components/Actions/ActionsRow.tsx (2 hunks)
- app/packages/core/src/components/Actions/similar/Similar.tsx (2 hunks)
- app/packages/core/src/components/ColorModal/ColorFooter.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (5 hunks)
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3 hunks)
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (5 hunks)
- app/packages/spaces/src/components/Workspaces/Workspace.tsx (4 hunks)
- app/packages/spaces/src/components/Workspaces/hooks.ts (2 hunks)
- app/packages/spaces/src/components/Workspaces/index.tsx (3 hunks)
- app/packages/state/src/recoil/atoms.ts (1 hunks)
- app/packages/state/src/session.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/spaces/src/components/Workspaces/hooks.ts
Files skipped from review as they are similar to previous changes (2)
- app/packages/core/src/components/Actions/ActionsRow.tsx
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx
Additional Context Used
Path-based Instructions (10)
app/packages/app/src/useWriters/registerWriter.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/spaces/src/components/Workspaces/Workspace.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/ColorModal/ColorFooter.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/state/src/session.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/spaces/src/components/Workspaces/index.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/state/src/recoil/atoms.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Actions/similar/Similar.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Learnings (2)
app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (1)
User: lanzhenw PR: voxel51/fiftyone#4379 File: app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx:135-137 Timestamp: 2024-05-09T22:38:21.629Z Learning: The `canModifySidebarGroup` permission flag is used in the `GroupEntry` component to check if sidebar groups can be modified or deleted.app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (1)
User: lanzhenw PR: voxel51/fiftyone#4379 File: app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx:135-137 Timestamp: 2024-05-09T22:38:21.629Z Learning: The `canModifySidebarGroup` permission flag is used in the `GroupEntry` component to check if sidebar groups can be modified or deleted.
Additional comments not posted (10)
app/packages/app/src/useWriters/registerWriter.ts (1)
9-14: The addition of new permission flags to theWriterKeystype aligns with the PR's objectives to enhance permission management. This change correctly updates the type to include the new permissions while keeping thereadOnlyflag.app/packages/core/src/components/Sidebar/Entries/AddGroupEntry.tsx (1)
10-11: The implementation of thecanModifySidebarGrouppermission to control the disabled state of the sidebar group addition is correctly done. This change enhances the permission management within the application.app/packages/spaces/src/components/Workspaces/Workspace.tsx (1)
19-21: The implementation of thecanEditWorkspacespermission to control the disabled state and display messages in the workspace component is correctly done. This change enhances the permission management within the application.Also applies to: 49-49, 52-52, 57-57, 71-71, 74-74
app/packages/core/src/components/Sidebar/Entries/Draggable.tsx (1)
22-24: The implementation of thecanModifySidebarGrouppermission to control the draggable state of the sidebar group is correctly done. This change enhances the permission management within the application.Also applies to: 30-30, 51-52, 55-59, 77-78, 93-93, 95-95
app/packages/core/src/components/ColorModal/ColorFooter.tsx (1)
12-15: The implementation of thecanEditCustomColorspermission to control the disabled state and display messages in the color footer component is correctly done. This change enhances the permission management within the application.Also applies to: 56-56, 104-104
app/packages/state/src/session.ts (1)
29-34: The updates to theSessioninterface andSESSION_DEFAULTto include new permission flags withenabledandmessageproperties are correctly implemented. This change enhances the permission management within the application.Also applies to: 45-50, 72-78, 186-189
app/packages/spaces/src/components/Workspaces/index.tsx (1)
31-33: Ensure proper handling of disabled state based on new permission structure.app/packages/state/src/recoil/atoms.ts (1)
308-319: Validate the new atom definitions for permission management.Ensure that the default values for these atoms are correctly set and that they align with the intended permission logic.
Also applies to: 321-333
app/packages/core/src/components/Actions/similar/Similar.tsx (1)
96-98: Ensure the new permission checks are correctly implemented for field creation.app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (1)
135-136: Ensure thecanModifySidebarGrouppermission is correctly implemented to manage sidebar group modifications.
| cursor: disabled ? "not-allowed" : undefined, | ||
| }} | ||
| title={disabledInfo} | ||
| title={disabledMsg} |
There was a problem hiding this comment.
Refactor to simplify the conditional logic for handling the disabled state.
- if (disabled) return;
+ if (!canEditWorkSpace.enabled) return;This change makes the condition directly reflect the state's enabled property, improving readability and maintainability.
Also applies to: 144-146
Committable suggestion was skipped due low confidence.
| disabled={disabled} | ||
| placeholder={"dist_field (default = None)"} | ||
| validator={(value) => !value.startsWith("_")} | ||
| value={state.distField ?? ""} | ||
| setter={(value) => | ||
| updateState({ distField: !value.length ? undefined : value }) | ||
| } | ||
| title={ | ||
| isReadOnly | ||
| ? "Can not store the distance in a field in read-only mode" | ||
| : undefined | ||
| } | ||
| title={disableMsg} |
There was a problem hiding this comment.
Refactor to improve the handling of the disabled state and error messages.
- disabled={disabled}
- title={disableMsg}
+ disabled={!canCreateNewField.enabled}
+ title={canCreateNewField.message}This change directly uses the properties of the canCreateNewField object, enhancing clarity and maintainability.
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.
| disabled={disabled} | |
| placeholder={"dist_field (default = None)"} | |
| validator={(value) => !value.startsWith("_")} | |
| value={state.distField ?? ""} | |
| setter={(value) => | |
| updateState({ distField: !value.length ? undefined : value }) | |
| } | |
| title={ | |
| isReadOnly | |
| ? "Can not store the distance in a field in read-only mode" | |
| : undefined | |
| } | |
| title={disableMsg} | |
| disabled={!canCreateNewField.enabled} | |
| placeholder={"dist_field (default = None)"} | |
| validator={(value) => !value.startsWith("_")} | |
| value={state.distField ?? ""} | |
| setter={(value) => | |
| updateState({ distField: !value.length ? undefined : value }) | |
| } | |
| title={canCreateNewField.message} |
| @@ -207,7 +209,7 @@ const GroupEntry = React.memo( | |||
| } | |||
| }} | |||
| /> | |||
| {hovering && !editing && setValue && ( | |||
| {hovering && !editing && setValue && !disabled && ( | |||
There was a problem hiding this comment.
Refactor to improve the handling of the disabled state and error messages.
- {hovering && !editing && setValue && !disabled && (
+ {hovering && !editing && setValue && canModifySidebarGroup.enabled && (This change directly uses the enabled property from canModifySidebarGroup, enhancing clarity and maintainability.
Also applies to: 233-233
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.
| {hovering && !editing && setValue && !disabled && ( | |
| {hovering && !editing && setValue && canModifySidebarGroup.enabled && ( |
manivoxel51
changed the title
What changes are proposed in this pull request?
(Please fill in changes proposed in this fix)
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Refactor
readOnlystate with specific permission checks across multiple components for better modularity and clarity.Usability Improvements