model evaluation panel permission ux tweaks #5253
Conversation
WalkthroughThe changes in this pull request involve modifications to several components within the Native Model Evaluation View. Key updates include the addition of permission handling in the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 and nitpick comments (7)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (1)
10-22: Consider accessibility improvements for disabled state.While the implementation is good, consider enhancing accessibility:
<Box title={canEvaluate ? "" : "You do not have permission to evaluate model"} sx={{ cursor: canEvaluate ? "pointer" : "not-allowed" }} + role="presentation" + aria-disabled={!canEvaluate} > <MuiButton onClick={onEvaluate} startIcon={<Add />} variant="contained" disabled={!canEvaluate} + aria-label="Evaluate Model" > Evaluate Model </MuiButton> </Box>app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx (1)
72-73: Consider improving type safety.The props interface could be more type-safe:
type StatusProps = { status?: string; canEdit?: boolean; - readOnly?: boolean; - setStatusEvent: string; + readOnly: boolean; // Make required to prevent undefined behavior + setStatusEvent: string; + status: keyof typeof STATUS_LABELS; // Restrict to valid status values };app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx (2)
100-100: Consider adding error boundary and memoization.While the Status component integration is good, consider these improvements:
- Wrap the Status component with error boundary
- Memoize the Status component to prevent unnecessary re-renders
- {status && <Status status={status} readOnly />} + {status && ( + <ErrorBoundary fallback={<StatusErrorFallback />}> + <MemoizedStatus status={status} readOnly /> + </ErrorBoundary> + )}
Line range hint
115-122: Improve type safety for evaluation objects.Consider making the types more specific:
type EvaluationType = { key: string; id: string; description: string; - status: string; + status: keyof typeof STATUS_LABELS; };app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (3)
Line range hint
1019-1030: Enhance type safety and documentation for component props.The
EvaluationPropstype could be improved for better type safety and maintainability.Consider applying these improvements:
type EvaluationProps = { name: string; id: string; navigateBack: () => void; loadEvaluation: (key?: string) => void; onChangeCompareKey: (compareKey: string) => void; compareKey?: string; - data: any; + data: { + permissions?: { + can_edit_note?: boolean; + can_edit_status?: boolean; + }; + evaluations?: Array<{ key: string }>; + [key: `evaluation_${string}`]: EvaluationData; + }; setStatusEvent: string; statuses: Record<string, string>; setNoteEvent: string; notes: Record<string, string>; - loadView: (type: string, params: any) => void; + loadView: (type: 'field' | 'class' | 'matrix' | 'clear', params: Record<string, unknown>) => void; };
Line range hint
40-54: Consider consolidating dialog state management.The component uses separate state for different dialogs with similar structures. This could be consolidated for better maintainability.
Consider using a single dialog state manager:
- const [editNoteState, setEditNoteState] = useState({ open: false, note: "" }); - const [classPerformanceDialogConfig, setClassPerformanceDialogConfig] = - useState<PLOT_CONFIG_DIALOG_TYPE>(DEFAULT_BAR_CONFIG); - const [confusionMatrixDialogConfig, setConfusionMatrixDialogConfig] = - useState<PLOT_CONFIG_DIALOG_TYPE>(DEFAULT_BAR_CONFIG); + const [dialogState, setDialogState] = useState<{ + type: 'note' | 'classPerformance' | 'confusionMatrix' | null; + config: { + open: boolean; + note?: string; + sortBy?: string; + limit?: number; + log?: boolean; + }; + }>({ type: null, config: { open: false } });
Line range hint
183-191: Consider splitting large render tree into smaller components.The component has a complex render tree that could benefit from being split into smaller, focused components for better maintainability and performance.
Consider extracting the summary section into a separate component:
+const EvaluationSummary = memo(({ summaryRows, name, compareKey, loadView }: EvaluationSummaryProps) => { + return ( <EvaluationTable> <TableHead> {/* ... header content ... */} </TableHead> <TableBody> {/* ... summary rows mapping ... */} </TableBody> </EvaluationTable> + ); +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.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/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.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/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.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/plugins/SchemaIO/components/NativeModelEvaluationView/Status.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 (3)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (1)
7-8: LGTM! Clean permission handling implementation.
The permission check is clear and well-typed. The canEvaluate boolean provides a single source of truth for permission state.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx (1)
25-30: LGTM! Clear permission feedback.
Good implementation of permission feedback with tooltip and disabled state.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
563-582: Well-implemented permission handling for evaluation notes editing.
The permission controls are properly implemented with:
- Clear user feedback through tooltips
- Appropriate cursor styling
- Proper button disabling
- Consistent React patterns
| @@ -4,10 +4,10 @@ import React from "react"; | |||
| import { useTriggerEvent } from "./utils"; | |||
|
|
|||
| export default function Status(props: StatusProps) { | |||
| const { status, canEdit, setStatusEvent } = props; | |||
| const { status, canEdit, readOnly, setStatusEvent } = props; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying permission logic.
The component uses both canEdit and readOnly props for permission handling, which could lead to confusing states. Consider consolidating these into a single permission control.
- const { status, canEdit, readOnly, setStatusEvent } = props;
+ const { status, canEdit: maybeCanEdit, setStatusEvent } = props;
+ const canEdit = maybeCanEdit && !props.readOnly;
- if (!readOnly) {
+ if (canEdit) {Also applies to: 10-10
| import React from "react"; | ||
|
|
||
| export default function Evaluate(props: EvaluateProps) { | ||
| const { onEvaluate } = props; | ||
| const { onEvaluate, permissions } = props; | ||
| const canEvaluate = permissions.can_evaluate; |
There was a problem hiding this comment.
How where is this permission set?
There was a problem hiding this comment.
Permissions are initialized on loading the panel in file fiftyone/operators/builtins/panels/model_evaluation/__init__.py
imanjra
force-pushed
the
bugfix/model-eval-perm
branch
from
0c1c7db to
edb53e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (2)
614-633: LGTM! Permission handling implementation looks good.The permission-based UI implementation effectively handles the edit button state with proper user feedback through tooltips and cursor styles. This aligns well with the PR objectives for improving permission UX.
Consider adding
aria-labelto improve accessibility:<Box title={ can_edit_note ? "" : "You do not have permission to edit evaluation notes" } + aria-label={ + can_edit_note + ? "Edit evaluation notes" + : "You do not have permission to edit evaluation notes" + } sx={{ cursor: can_edit_note ? "pointer" : "not-allowed" }} >
Line range hint
1001-1013: Consider improving TypeScript type definitions.The
dataprop is currently typed asany, which bypasses TypeScript's type checking benefits. Consider creating proper interfaces for the data structure, especially for permissions.+ interface Permissions { + can_edit_note: boolean; + can_edit_status: boolean; + } + + interface EvaluationData { + permissions?: Permissions; + evaluations?: Array<{ key: string }>; + // ... other data properties + } + type EvaluationProps = { name: string; id: string; navigateBack: () => void; loadEvaluation: (key?: string) => void; onChangeCompareKey: (compareKey: string) => void; compareKey?: string; - data: any; + data: EvaluationData; setStatusEvent: string; statuses: Record<string, string>; setNoteEvent: string; notes: Record<string, string>; loadView: (type: string, params: any) => void; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.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 (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (2)
Line range hint 37-67: Well-structured state management implementation.
The component demonstrates good state management practices:
- Effective use of useMemo for derived states
- Clear separation of local and global state
- Consistent state updates through proper event handlers
Line range hint 68-613: Clean and consistent UI/UX implementation.
The UI implementation demonstrates good practices:
- Consistent use of Material-UI components
- Clear user feedback for different states
- Proper handling of loading and error states
- Responsive layout with appropriate spacing
Also applies to: 634-1000
| <MuiButton onClick={onEvaluate} startIcon={<Add />} variant="contained"> | ||
| Evaluate Model | ||
| </MuiButton> | ||
| <Box |
There was a problem hiding this comment.
curious: is it better to use TooltipProvider
There was a problem hiding this comment.
Ohh, is that a new component? Yah, it might be better. Sorry, I saw the comment after merging. I'll follow-up
What changes are proposed in this pull request?
Disable actions with appropriate messages instead of hiding when permission is not sufficient to perform an action
How is this patch tested? If it is not, please explain why.
Using model evaluation panel with several action disabled due to permission
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
See above
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Style