fix non-editable views state issue and conditional row actions for TableView #4969
Conversation
WalkthroughThe changes introduce a new utility function Changes
Possibly related PRs
Suggested reviewers
Poem
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
|
|
Converting to draft to fix highlight colouring |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/utils/index.ts (2)
95-114: LGTM! Consider adding JSDoc documentation.The constant is well-structured and follows the established naming convention. To improve maintainability, consider adding JSDoc documentation explaining the purpose and usage of these non-editable views.
+/** + * List of view components that are inherently non-editable. + * These components are used for display purposes only and do not accept user input. + */ const NON_EDITABLE_VIEWS = [
125-127: LGTM! Consider type safety improvements.The function is concise and follows the codebase patterns. Consider these enhancements for better type safety and documentation:
+/** + * Determines if a schema represents an editable view. + * @param schema - The schema to check + * @returns true if the view is editable, false otherwise + */ -export function isEditableView(schema: SchemaType) { +export function isEditableView(schema: SchemaType): schema is SchemaType & { view: { component: string } } { return !NON_EDITABLE_VIEWS.includes(schema?.view?.component); }app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (2)
70-72: Consider a more descriptive variable nameWhile the logic is correct, the variable name
shouldInitializecould be more descriptive to better reflect both conditions it checks. Consider something likeisEditableNonCompositeVieworcanInitializeState.- const shouldInitialize = useMemo(() => { + const isEditableNonCompositeView = useMemo(() => { return !isCompositeView(computedSchema) && isEditableView(computedSchema); }, [computedSchema]);
Line range hint
84-89: Consider breaking down complex conditions for better readabilityThe multiple conditions in the if statement could be more readable if broken down into named variables that explain their purpose.
+ const hasDefaultValue = !isNullish(defaultValue); + const isUninitialized = !isInitialized(props); + const needsDefaultValue = !isEqual(data, defaultValue); + const isUserUnmodified = !isPathUserChanged(path, root_id); if ( shouldInitialize && - !isEqual(data, defaultValue) && - !isPathUserChanged(path, root_id) && - !isNullish(defaultValue) && - !isInitialized(props) + needsDefaultValue && + isUserUnmodified && + hasDefaultValue && + isUninitialized ) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (2 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/utils/index.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/core/src/plugins/SchemaIO/components/DynamicIO.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/TableView.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/utils/index.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 (2)
app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (2)
10-10: LGTM: Import addition follows best practicesThe new utility import is properly grouped with related functions.
70-72: Verify consistent usage of isEditableView across the codebaseSince
isEditableViewis a new utility function that affects state initialization, we should ensure it's consistently used where needed.✅ Verification successful
Let me try a different approach to verify the usage of
isEditableViewin the codebase.This should help us understand:
- Where isEditableView is defined
- All its current usage points
- Files that might need similar editability checks
- The actual implementation to understand its purpose better
Let me check the implementation details and usage context to make a more informed decision.
Based on the gathered information, I can now provide a final response:
isEditableViewusage is correctly implemented and scopedThe utility function
isEditableViewis appropriately used in the codebase because:
- It's defined in
SchemaIO/utils/index.tsand checks against a well-definedNON_EDITABLE_VIEWSconstant list- It's only used in
DynamicIO.tsxfor state initialization, which is the correct scope as this component handles dynamic IO operations- Other view-related components (like
DashboardView.tsx) use their own specific view properties (allow_edit,allow_addition, etc.) which serve different purposes- The combination with
isCompositeViewin the condition is logical as composite views have their own state management🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential places where isEditableView should be used rg -t tsx -t ts "isCompositeView|state.*initializ|schema.*edit" --no-headingLength of output: 106
Script:
#!/bin/bash # Search for isEditableView function definition and usage rg "isEditableView" -l # Search for files containing composite view or schema edit related logic rg "isCompositeView|initializ.*state|edit.*schema" -l # Try to find the actual implementation of isEditableView ast-grep --pattern 'function isEditableView($_) { $$$ }' # Also search for it as a const function ast-grep --pattern 'const isEditableView = $_'Length of output: 1099
Script:
#!/bin/bash # Check the NON_EDITABLE_VIEWS constant and surrounding context rg -A 5 -B 5 "NON_EDITABLE_VIEWS" app/packages/core/src/plugins/SchemaIO/utils/index.ts # Check how isCompositeView is used in the same files rg -A 3 "isCompositeView" app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx app/packages/core/src/plugins/SchemaIO/utils/index.ts # Look for other view-related checks in these components rg "\.view\." app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx app/packages/core/src/plugins/SchemaIO/components/DashboardView.tsxLength of output: 5046
| if (shouldInitialize) { | ||
| return data; | ||
| } | ||
| }, [data, schema]); | ||
| }, [shouldInitialize, data]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit return type and handle false case
The current implementation implicitly returns undefined when shouldInitialize is false. Consider making this more explicit for better type safety.
- const basicData = useMemo(() => {
+ const basicData = useMemo((): typeof data | undefined => {
if (shouldInitialize) {
return data;
}
+ return undefined;
}, [shouldInitialize, data]);📝 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.
| if (shouldInitialize) { | |
| return data; | |
| } | |
| }, [data, schema]); | |
| }, [shouldInitialize, data]); | |
| const basicData = useMemo((): typeof data | undefined => { | |
| if (shouldInitialize) { | |
| return data; | |
| } | |
| return undefined; | |
| }, [shouldInitialize, data]); |
| {columns.map(({ key }, columnIndex) => { | ||
| const coordinate = [rowIndex, columnIndex].join(","); | ||
| const isSelected = | ||
| selectedCells.has(coordinate) || | ||
| selectedRows.has(rowIndex) || | ||
| selectedColumns.has(columnIndex); | ||
| return ( | ||
| <TableCell | ||
| key={key} | ||
| sx={{ background: isSelected ? "green" : "unset" }} | ||
| onClick={() => { | ||
| handleCellClick(rowIndex, columnIndex); | ||
| }} | ||
| {...getComponentProps(props, "tableBodyCell")} | ||
| > | ||
| {formatCellValue(item[key], props)} | ||
| </TableCell> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and performance of cell rendering.
Consider the following improvements:
- Add TypeScript interfaces for column structure
- Memoize the coordinate string generation
- Use more semantic variable names than 'coordinate'
+ interface TableColumn {
+ key: string;
+ label: string;
+ }
- const coordinate = [rowIndex, columnIndex].join(",");
+ const cellPosition = React.useMemo(
+ () => `${rowIndex},${columnIndex}`,
+ [rowIndex, columnIndex]
+ );Committable suggestion was skipped due to low confidence.
| {hasRowActions && ( | ||
| <TableCell align="right"> | ||
| {currentRowHasActions && ( | ||
| <ActionsMenu actions={getRowActions(rowIndex)} /> | ||
| )} | ||
| </TableCell> | ||
| ); | ||
| })} | ||
| {hasRowActions && ( | ||
| <TableCell align="right"> | ||
| <ActionsMenu actions={getRowActions(rowIndex)} /> | ||
| </TableCell> | ||
| )} | ||
| </TableRow> | ||
| ))} | ||
| )} |
There was a problem hiding this comment.
Improve accessibility and reuse computed row actions.
Add proper accessibility attributes and reuse the previously computed row actions.
- <TableCell align="right">
+ <TableCell
+ align="right"
+ aria-label={`Actions for row ${rowIndex + 1}`}
+ >
{currentRowHasActions && (
- <ActionsMenu actions={getRowActions(rowIndex)} />
+ <ActionsMenu actions={memoizedRowActions} />
)}
</TableCell>Committable suggestion was skipped due to low confidence.
| return ( | ||
| <TableRow | ||
| key={item.id} | ||
| sx={{ "&:last-child td, &:last-child th": { border: 0 } }} | ||
| {...getComponentProps(props, "tableBodyRow")} | ||
| > |
There was a problem hiding this comment.
Add type safety and robust key handling.
The code assumes item.id exists without validation. Consider adding proper TypeScript interfaces and a fallback for the key prop.
+ interface TableRowItem {
+ id?: string | number;
+ [key: string]: any;
+ }
- key={item.id}
+ key={item.id ?? `row-${rowIndex}`}Committable suggestion was skipped due to low confidence.
| {rows.map((item, rowIndex) => { | ||
| const rowActions = getRowActions(rowIndex); | ||
| const currentRowHasActions = rowActions?.length > 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize row actions computation to avoid duplicate calls.
The getRowActions function is called twice per row - once to check the length and once for rendering. Consider memoizing the result or restructuring to avoid duplicate computation.
{rows.map((item, rowIndex) => {
- const rowActions = getRowActions(rowIndex);
- const currentRowHasActions = rowActions?.length > 0;
+ const memoizedRowActions = React.useMemo(
+ () => getRowActions(rowIndex),
+ [rowIndex, getRowActions]
+ );
+ const currentRowHasActions = memoizedRowActions?.length > 0;Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/packages/components/src/components/ThemeProvider/index.tsx (1)
53-53: Consider reusing existing color variable.The
activeCellcolor value matchesbackground.level1. Consider reusing the existing color variable to maintain consistency and reduce duplication.- activeCell: "hsl(200, 0%, 95%)", + activeCell: dynamicTheme("palette.background.level1"),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/components/src/components/ThemeProvider/index.tsx (2 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/components/src/components/ThemeProvider/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.
| @@ -129,6 +130,7 @@ let theme = extendMuiTheme({ | |||
| paper: "hsl(200, 0%, 10%)", | |||
| popup: "hsl(200, 0%, 20%)", | |||
| field: "hsl(200, 0%, 20%, 0.3)", | |||
| activeCell: "hsl(200, 0%, 25%)", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting color values into semantic variables.
The color value for activeCell in dark theme introduces a new shade. Consider extracting common color values into semantic variables at the top of the theme configuration for better maintainability.
Example approach:
const darkThemeColors = {
backgroundActive: "hsl(200, 0%, 25%)",
// ... other semantic color variables
} as const;
// Then in the theme:
activeCell: darkThemeColors.backgroundActive,| @@ -92,6 +92,27 @@ const COMPOSITE_VIEWS = [ | |||
| "ButtonGroupView", | |||
| ]; | |||
|
|
|||
| const NON_EDITABLE_VIEWS = [ | |||
There was a problem hiding this comment.
This seems hard to maintain. I would rather we define this in each view somehow or in a way that we won't forget to add things to this list. In other words I don't think we'll be able to maintain this list.
There was a problem hiding this comment.
Yah, good point. I will follow-up. I would say let's go with this for now to unblock Minh's panel for release
There was a problem hiding this comment.
Bug fix works for TableView, verified here:
https://www.loom.com/share/581654042f7e42b192beafe9ace62d09
Thanks Ibrahim! ✅
What changes are proposed in this pull request?
fix non-editable views state issue and conditional row actions for TableView
How is this patch tested? If it is not, please explain why.
Using a python panel
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