TableView enhancements #4809
TableView enhancements #4809
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActionsMenu
participant TableView
participant Action
User->>TableView: Interacts with table
TableView->>ActionsMenu: Displays actions
User->>ActionsMenu: Selects action
ActionsMenu->>Action: Executes action
Action-->>TableView: Updates table state
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
|
imanjra
force-pushed
the
feat/tableviewv2
branch
from
851e075 to
638f20b
Compare
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (5 hunks)
- fiftyone/operators/types.py (2 hunks)
Additional context used
Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.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.
Biome
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
[error] 195-195: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
Additional comments not posted (11)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (5)
1-14: Imports look good!The imports are relevant, and there are no unused imports. The naming is clear and follows conventions.
17-31: TheActionsMenucomponent looks good!
- The component handles the rendering logic based on the number of actions effectively.
- The use of a constant for the default
maxInlinevalue is a good practice.- The component is exported correctly as the default export.
- The rendering logic is clear and concise.
33-70: TheActionsOverflowMenucomponent looks good!
- The component manages the menu state effectively using the
openstate variable andanchorRef.- The
handleClosefunction is memoized usinguseCallbackto avoid unnecessary re-creation.- The
onClickhandler for each action is wrapped to close the menu after the action is triggered, which is a good user experience.- The rendering of the menu items is done by mapping over the
actionsprop, which is a clean and readable approach.
72-94: TheActioncomponent looks good!
- The component handles the rendering of an action based on the
modeprop effectively.- The
Iconcomponent is conditionally rendered based on theiconprop, which is a good practice.- The
handleClickfunction is memoized usinguseCallbackto avoid unnecessary re-creation.- The
onClickhandler is called with thepropsobject and the event object, providing flexibility.- The rendering logic is clean and readable.
96-108: The type definitions look good!
- The
ActionsPropsTypeandActionPropsTypeare defined correctly and cover all the necessary props.- The
onClickprop inActionPropsTypeis defined as a function that accepts theactionobject and the event object, providing flexibility.- The
modeprop inActionPropsTypeis defined as a union type of"inline"and"menu", ensuring type safety.app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (4)
Line range hint
22-146: LGTM!The
TableViewcomponent follows several good practices:
- It extracts the required props and handles the case when no data is provided.
- It uses the
getTableDatafunction to extract the table data in a structured manner, which improves the clarity and maintainability of the code.- It renders the table header and body based on the extracted data.
- It handles click events using the
usePanelEventhook, which ensures that the events are tied to the correct panel context.- It uses the
ActionsMenucomponent to render row actions, which keeps the component modular.- It uses the
getComponentPropsfunction to extract the component-specific props, which keeps the component props structured.- It uses the
scrollableclass to make the table scrollable, which handles large tables.- It uses the
formatCellValuefunction to format the cell values based on the schema's rounding specifications, which keeps the formatting logic separate.Overall, the component is well-structured and follows the best practices for React and TypeScript.
148-165: LGTM!The
getTableDatafunction follows several good practices:
- It extracts the table data from the props correctly.
- It handles both standard and advanced data formats, which improves the flexibility of the component.
- It uses the
isAdvancedDataandparseAdvancedDatafunctions to handle the advanced data format, which improves the clarity and maintainability of the code.Overall, the function is well-structured and follows the best practices for TypeScript.
167-173: LGTM!The
isAdvancedDatafunction follows several good practices:
- It correctly checks if the provided data is in the advanced format.
- It uses the
isPlainObjectfunction from thelodashlibrary to check if the data is an object, which ensures that the data is in the expected format.- It checks if the
rowsandcolumnsproperties of the data are arrays, which ensures that the data is in the expected format.Overall, the function is well-structured and follows the best practices for TypeScript.
175-190: LGTM!The
parseAdvancedDatafunction follows several good practices:
- It correctly parses the advanced data format.
- It converts the
rowsarray into an array of objects, where each object represents a row with the column names as keys, which makes the data more accessible.- It extracts the
selectedCells,selectedRows, andselectedColumnsproperties from the data and converts them into sets for faster lookup, which improves the performance of the component.- It handles the case when the
selectedCells,selectedRows, andselectedColumnsproperties are not provided, which improves the flexibility of the component.Overall, the function is well-structured and follows the best practices for TypeScript.
fiftyone/operators/types.py (2)
1621-1640: LGTM!The
Actionclass is well-designed with clear attributes and methods for cloning and JSON serialization. The implementation looks good.
1647-1647: Excellent enhancement!The changes to the
TableViewclass to support row actions are well-implemented. The newrow_actionsattribute,add_row_actionmethod, and the updates tocloneandto_jsonmethods all look good. This will provide a convenient way to incorporate interactive actions for each row in the table.Also applies to: 1653-1653, 1663-1669, 1673-1673, 1680-1680
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
Outdated
Show resolved
Hide resolved
imanjra
force-pushed
the
feat/tableviewv2
branch
from
638f20b to
4909121
Compare
| {columns.map(({ key }, columnIndex) => { | ||
| const coordinate = [rowIndex, columnIndex].join(","); | ||
| const isSelected = | ||
| selectedCells.has(coordinate) || |
There was a problem hiding this comment.
I have been running into some of these errors when a tableview does not have selectedRows/Columns/Cells in the initial state.
imanjra
force-pushed
the
feat/tableviewv2
branch
from
4909121 to
beed93a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (4)
38-50: Well-implemented row actions handling.The
getRowActionscallback is well-structured:
- Proper use of
useCallbackfor performance optimization.- Correct implementation of
onClickhandler usinghandleClickwith panel context.Consider memoizing
row_actions.mapresult ifrow_actionsis expected to be static:const memoizedRowActions = useMemo(() => row_actions.map(/* ... */), [row_actions]); const getRowActions = useCallback((row) => { return memoizedRowActions.map(action => ({ ...action, onClick: (e) => { handleClick(panelId, { operator: action.on_click, params: { path, event: action.name, row }, }); }, })); }, [handleClick, panelId, path, memoizedRowActions]);This optimization could be beneficial if
getRowActionsis called frequently with different rows.
Line range hint
51-143: Enhanced table rendering with improved interactivity.The updated table rendering logic significantly improves the component's functionality:
- Proper incorporation of selection states for cells, rows, and columns.
- Well-implemented row actions integration.
- Correct use of MUI's
sxprop for styling selected cells.Consider extracting the click handler logic into a separate function to improve readability:
const handleCellClick = useCallback((rowIndex, columnIndex) => { if (on_click_cell) { handleClick(panelId, { operator: on_click_cell, params: { row: rowIndex, column: columnIndex, path, event: "on_click_cell" }, }); } if (on_click_row) { handleClick(panelId, { operator: on_click_row, params: { row: rowIndex, path, event: "on_click_row" }, }); } if (on_click_column) { handleClick(panelId, { operator: on_click_column, params: { column: columnIndex, path, event: "on_click_column" }, }); } }, [on_click_cell, on_click_row, on_click_column, handleClick, panelId, path]);Then use it in the
onClickprop:onClick={() => handleCellClick(rowIndex, columnIndex)}This refactoring would improve code readability and maintainability.
148-190: Well-implemented data handling utility functions.The new utility functions effectively handle different data formats:
getTableDataprovides a flexible approach to handle both simple and advanced data structures.isAdvancedDatacorrectly identifies the advanced data format.parseAdvancedDataefficiently transforms the data into a consistent structure.The use of
Setfor selected elements is a good choice for performance.Consider adding basic error handling to
parseAdvancedData:function parseAdvancedData(data) { if (!isAdvancedData(data)) { console.warn('Invalid data format passed to parseAdvancedData'); return { rows: [], selectedCells: new Set(), selectedRows: new Set(), selectedColumns: new Set() }; } // ... rest of the function }This addition would make the function more robust against potential misuse.
192-199: Well-implemented cell value formatting function.The
formatCellValuefunction effectively handles numeric rounding:
- Correct use of
Number.isNaNfor number checking.- Proper implementation of rounding based on the schema.
Consider adding type assertions or checks to improve type safety:
function formatCellValue(value: unknown, props: ViewPropsType): string { const round = props?.schema?.view?.round; if (typeof value !== 'string' && typeof value !== 'number') { return String(value); } const valueAsFloat = parseFloat(String(value)); if (!Number.isNaN(valueAsFloat) && typeof round === "number") { return valueAsFloat.toFixed(round); } return String(value); }This change would make the function more robust against different input types.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (5 hunks)
- fiftyone/operators/types.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx
- fiftyone/operators/types.py
Additional context used
Path-based instructions (1)
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.
Additional comments not posted (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
1-3: Improved imports and component structure.The changes to the imports and component declaration are well-structured:
- New hooks (
usePanelEvent,usePanelId) have been added, improving the component's integration with the panel system.- The
ActionsMenuimport suggests enhanced interactivity.- Using
ViewPropsTypefor props improves type safety.These changes align with React best practices and enhance the component's functionality.
Also applies to: 14-15, 18-20, 22-37
|
Also I think this new tableview might be backward incompatible; previously this is how we set the data state to the Table: And then we set the data state in another section: |
imanjra
force-pushed
the
feat/tableviewv2
branch
from
beed93a to
27057a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (3)
15-15: Consider adding a comment to explain the purpose of DEFAULT_MAX_INLINE.While the constant is appropriately named, its purpose and significance in the component's logic might not be immediately clear to other developers. A brief comment explaining its role would enhance code readability.
33-70: LGTM: ActionsOverflowMenu component is well-implemented, with a minor suggestion.The component effectively manages its state and correctly passes props to child components. The use of
useCallbackforhandleCloseis good for performance.Suggestion for improvement:
Consider memoizing theonClickhandler for each Action to prevent unnecessary re-renders:const createActionClickHandler = useCallback( (action: ActionPropsType) => (e: React.MouseEvent) => { handleClose(); action.onClick?.(action, e); }, [handleClose] );Then use it in the Action component like this:
onClick={createActionClickHandler(action)}This change would ensure that the onClick handler for each Action is only recreated when
handleClosechanges, potentially improving performance for menus with many actions.
72-94: LGTM: Action component is well-implemented, with a suggestion for improved type safety.The component is flexible and efficiently handles both inline and menu modes. The use of
useCallbackfor theonClickhandler is good for performance.Suggestion for improved type safety:
Consider using a discriminated union type for themodeprop to ensure type safety and enable better autocomplete:type ActionPropsType = { name: string; label: string; onClick: (action: ActionPropsType, e: React.MouseEvent) => void; icon?: string; variant?: string; } & ( | { mode: "inline"; variant: string } | { mode: "menu" } );This change would ensure that the
variantprop is required whenmodeis "inline", and not allowed whenmodeis "menu". It would also make theiconandvariantprops optional, which seems to align with their usage in the component.app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
148-199: Add TypeScript type annotations to helper functionsThe helper functions lack explicit type annotations, which can reduce code readability and type safety. Adding type annotations to the functions enhances maintainability.
Apply this diff to add type annotations:
-function getTableData(props) { +function getTableData(props: ViewPropsType): { + rows: any[]; + selectedCells: Set<string>; + selectedRows: Set<number>; + selectedColumns: Set<number>; +} { // function body } -function isAdvancedData(data) { +function isAdvancedData(data: any): boolean { // function body } -function parseAdvancedData(data) { +function parseAdvancedData(data: any): { + rows: any[]; + selectedCells: Set<string>; + selectedRows: Set<number>; + selectedColumns: Set<number>; +} { // function body } -function formatCellValue(value: string, props: ViewPropsType) { +function formatCellValue(value: string, props: ViewPropsType): string { // function body }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (5 hunks)
- fiftyone/operators/types.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.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.
🔇 Additional comments (6)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (2)
1-13: LGTM: Import statements are well-organized and follow best practices.The import statements are appropriately structured, using named imports where possible. The use of Material-UI components and icons is consistent with the project's UI framework.
17-31: LGTM: ActionsMenu component is well-implemented.The component's logic is clear and concise. It efficiently handles both inline and overflow scenarios based on the number of actions. The use of a default value for
maxInlineis a good practice. The component is pure, which is beneficial for performance and testing.fiftyone/operators/types.py (4)
1647-1647: Docstring updated to includerow_actionsThe docstring for the
TableViewclass now includes therow_actionsparameter, enhancing the documentation and informing users about the new functionality.
1653-1653: Proper initialization ofrow_actionsinTableView.__init__The
row_actionsattribute is correctly initialized in the__init__method usingkwargs.get("row_actions", []), ensuring that it defaults to an empty list if not provided.
1673-1673: Cloningrow_actionsinTableView.clone()The
clone()method ofTableViewcorrectly clones therow_actionslist, preventing unintended side effects from shared references between instances.
1680-1680: Includingrow_actionsinTableView.to_json()outputThe
to_json()method has been appropriately updated to include the serializedrow_actions, ensuring that all action information is available in the JSON representation.
| const getRowActions = useCallback((row) => { | ||
| return row_actions.map((action) => { | ||
| return { | ||
| ...action, | ||
| onClick: (action, e) => { | ||
| handleClick(panelId, { | ||
| operator: action.on_click, | ||
| params: { path, event: action.name, row }, | ||
| }); | ||
| }, | ||
| }; | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Include dependencies in the useCallback hook
The useCallback hook at line 38 depends on row_actions, handleClick, panelId, and path, but the dependencies array is empty. This can lead to stale closures or unexpected behavior. Include these dependencies in the array to ensure the callback updates when these values change.
Apply this diff to fix the dependencies array:
- }, []);
+ }, [row_actions, handleClick, panelId, path]);📝 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.
| const getRowActions = useCallback((row) => { | |
| return row_actions.map((action) => { | |
| return { | |
| ...action, | |
| onClick: (action, e) => { | |
| handleClick(panelId, { | |
| operator: action.on_click, | |
| params: { path, event: action.name, row }, | |
| }); | |
| }, | |
| }; | |
| }); | |
| }, []); | |
| const getRowActions = useCallback((row) => { | |
| return row_actions.map((action) => { | |
| return { | |
| ...action, | |
| onClick: (action, e) => { | |
| handleClick(panelId, { | |
| operator: action.on_click, | |
| params: { path, event: action.name, row }, | |
| }); | |
| }, | |
| }; | |
| }); | |
| }, [row_actions, handleClick, panelId, path]); |
| onClick: (action, e) => { | ||
| handleClick(panelId, { | ||
| operator: action.on_click, | ||
| params: { path, event: action.name, row }, |
There was a problem hiding this comment.
Avoid variable shadowing in the onClick handler
In the onClick function, the parameter action shadows the variable action from the outer scope, which can cause confusion or errors. Rename the parameter to avoid shadowing.
Apply this diff to fix the issue:
- onClick: (action, e) => {
+ onClick: (event, e) => {
handleClick(panelId, {
- operator: action.on_click,
- params: { path, event: action.name, row },
+ operator: action.on_click,
+ params: { path, event: action.name, row },
});
},Committable suggestion was skipped due to low confidence.
| def add_row_action(self, name, on_click, label=None, icon=None, **kwargs): | ||
| row_action = Action( | ||
| name=name, on_click=on_click, label=label, icon=icon, **kwargs | ||
| ) | ||
| self.row_actions.append(row_action) | ||
| return row_action | ||
|
|
There was a problem hiding this comment.
Ensure consistency between Action initialization and usage
In the add_row_action method of the TableView class, you are creating an Action instance with named parameters name, on_click, label, and icon. However, the Action class's __init__ method does not currently accept these parameters explicitly, which will lead to a TypeError.
After updating the Action.__init__ method to accept and store these parameters as suggested earlier, this inconsistency will be resolved.
| def to_json(self): | ||
| return {**super().to_json()} | ||
|
|
There was a problem hiding this comment.
Action.to_json() omits action-specific attributes
The to_json() method of the Action class currently only returns the JSON from the superclass, which excludes the Action class's own attributes. As a result, the serialized output will not include the action's name, label, icon, or on_click, which are crucial for rendering and executing the action.
Update the to_json() method to include the action-specific properties:
def to_json(self):
- return {**super().to_json()}
+ return {
+ **super().to_json(),
+ "name": self.name,
+ "label": self.label,
+ "icon": self.icon,
+ "on_click": self.on_click,
+ }📝 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.
| def to_json(self): | |
| return {**super().to_json()} | |
| def to_json(self): | |
| return { | |
| **super().to_json(), | |
| "name": self.name, | |
| "label": self.label, | |
| "icon": self.icon, | |
| "on_click": self.on_click, | |
| } |
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
|
|
There was a problem hiding this comment.
Action.__init__ does not store action properties
The __init__ method of the Action class accepts keyword arguments but does not store the action-specific properties (name, label, icon, on_click) as instance variables. This means that instances of Action will not retain these properties, which are essential for the action's functionality.
To fix this, modify the __init__ method to accept these parameters explicitly and assign them to instance variables:
- def __init__(self, **kwargs):
+ def __init__(self, name, on_click, label=None, icon=None, **kwargs):
super().__init__(**kwargs)
+ self.name = name
+ self.on_click = on_click
+ self.label = label
+ self.icon = icon📝 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.
| def __init__(self, **kwargs): | |
| super().__init__(**kwargs) | |
| def __init__(self, name, on_click, label=None, icon=None, **kwargs): | |
| super().__init__(**kwargs) | |
| self.name = name | |
| self.on_click = on_click | |
| self.label = label | |
| self.icon = icon |
There was a problem hiding this comment.
Can we add virtualization to the table (https://mui.com/material-ui/react-table/#virtualized-table)?
imanjra
force-pushed
the
feat/tableviewv2
branch
2 times, most recently
from
3867d81 to
efac7b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (3)
15-31: LGTM: ActionsMenu component is well-structured.The component logic is clear and follows React best practices. The use of a default value for
maxInlineand conditional rendering based on the number of actions provides good flexibility.Consider adding prop types validation using PropTypes or TypeScript interfaces for better type safety and documentation. For example:
interface ActionsMenuProps { actions: ActionPropsType[]; maxInline?: number; } export default function ActionsMenu({ actions, maxInline = DEFAULT_MAX_INLINE }: ActionsMenuProps) { // ... existing code ... }
33-70: LGTM: ActionsOverflowMenu component is well-implemented.The component makes good use of React hooks for state management and performance optimization. The handling of menu opening/closing and action mapping is efficient.
Consider extracting the IconButton into a separate component for better reusability and testability. For example:
const OverflowMenuButton = React.forwardRef<HTMLButtonElement, { onClick: () => void }>( ({ onClick }, ref) => ( <IconButton onClick={onClick} ref={ref}> <MoreVert /> </IconButton> ) ); // Then in ActionsOverflowMenu: <OverflowMenuButton onClick={() => setOpen(!open)} ref={anchorRef} />
72-94: LGTM: Action component is well-designed and flexible.The component efficiently handles both inline and menu modes, and the use of
useCallbackfor the click handler is good for performance optimization.Consider using TypeScript's discriminated unions to improve type safety for the
modeprop:type BaseActionProps = { name: string; label: string; onClick: (action: ActionPropsType, e: React.MouseEvent) => void; icon?: string; }; type InlineActionProps = BaseActionProps & { mode: "inline"; variant: string; }; type MenuActionProps = BaseActionProps & { mode: "menu"; }; type ActionPropsType = InlineActionProps | MenuActionProps; function Action(props: ActionPropsType) { // ... existing code ... }This approach ensures that
variantis only required whenmodeis "inline".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (5 hunks)
- fiftyone/operators/types.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.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.
🔇 Additional comments (5)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1)
1-13: LGTM: Imports are well-organized and appropriate.The imports are correctly structured, using named imports where possible, which is good for tree-shaking. The use of Material-UI components and icons is consistent with the project's UI framework.
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
112-144: Clarify the event handling logic in theonClickhandlerWhen a cell is clicked, the
onClickhandler triggers multiple events:on_click_cell,on_click_row, andon_click_columnif they are defined. This could result in multiple handlers being called simultaneously for a single click, which might not be the intended behavior. Consider reviewing the logic to ensure that only the intended event(s) are triggered based on the specific user interaction to prevent unexpected behavior.fiftyone/operators/types.py (3)
1631-1633: EnsureActionclass stores action propertiesCurrently, the
Actionclass__init__method accepts keyword arguments but does not explicitly store the action-specific properties (name,label,icon,on_click) as instance variables. This means instances ofActionwill not retain these properties, which are essential for the action's functionality.
1638-1640:Action.to_json()omits action-specific attributesThe
to_json()method of theActionclass currently only returns the JSON from the superclass, which excludes theActionclass's own attributes. As a result, the serialized output will not include the action'sname,label,icon, oron_click, which are crucial for rendering and executing the action.
1663-1669: Inconsistency betweenActioninitialization and usageIn the
add_row_actionmethod of theTableViewclass, you are creating anActioninstance with named parametersname,on_click,label, andicon. However, theActionclass's__init__method does not currently accept these parameters explicitly, which will lead to aTypeError.
| type ActionsPropsType = { | ||
| actions: Array<ActionPropsType>; | ||
| maxInline?: number; | ||
| }; | ||
|
|
||
| type ActionPropsType = { | ||
| name: string; | ||
| label: string; | ||
| onClick: (action: ActionPropsType, e: React.MouseEvent) => void; | ||
| icon: string; | ||
| variant: string; | ||
| mode: "inline" | "menu"; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type definitions for better type safety and flexibility.
The current type definitions are good, but can be enhanced:
- For
ActionsPropsType, consider using a more specific type formaxInline:
type ActionsPropsType = {
actions: Array<ActionPropsType>;
maxInline?: 1 | 2 | 3 | 4 | 5; // or whatever reasonable maximum you want to allow
};- For
ActionPropsType, make some properties optional and use a union type formode:
type BaseActionProps = {
name: string;
label: string;
onClick: (action: ActionPropsType, e: React.MouseEvent) => void;
icon?: string;
};
type InlineActionProps = BaseActionProps & {
mode: "inline";
variant: string;
};
type MenuActionProps = BaseActionProps & {
mode: "menu";
};
type ActionPropsType = InlineActionProps | MenuActionProps;These changes would improve type safety and make the types more accurately reflect how they're used in the components.
imanjra
force-pushed
the
feat/tableviewv2
branch
from
efac7b6 to
baadddc
Compare
imanjra
force-pushed
the
feat/tableviewv2
branch
from
baadddc to
033976c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (3)
1-3: LGTM! Consider grouping related imports.The new imports and component signature changes improve the overall structure and readability of the component. However, consider grouping related imports together for better organization.
You could group the imports like this:
import { scrollable } from "@fiftyone/components"; import { usePanelEvent } from "@fiftyone/operators"; import { usePanelId } from "@fiftyone/spaces"; import { ViewPropsType } from "../utils/types"; import { Box, Paper, Table, TableBody, TableCell, TableContainer, TableHead, TableRow } from "@mui/material"; import { isPlainObject } from "lodash"; import React, { useCallback } from "react"; import { HeaderView } from "."; import { getComponentProps } from "../utils"; import ActionsMenu from "./ActionsMenu"; import EmptyState from "./EmptyState";Also applies to: 14-15, 18-19, 22-32
33-56: LGTM! Consider memoizinggetRowActionsdependencies.The new data processing logic and row actions handling improve the component's functionality. The use of
getTableDatafor data processing and the callback forgetRowActionsare good practices.Consider memoizing the dependencies of
getRowActionsto optimize performance:const getRowActions = useCallback((row) => { // ... existing code ... }, [row_actions, handleClick, panelId, path]);This ensures that the function is only recreated when its dependencies change.
Line range hint
82-163: LGTM! Consider extracting table header and body into separate components.The updated table rendering logic successfully implements new features like highlighting selected cells and rendering row actions. The use of
getComponentPropsfor styling is consistent and maintainable.To improve readability and maintainability, consider extracting the table header and body into separate components:
const TableHeader = ({ columns, hasRowActions, handleCellClick, actionsLabel, ...props }) => ( <TableHead {...getComponentProps(props, "tableHead")}> {/* ... header rendering logic ... */} </TableHead> ); const TableBodyContent = ({ rows, columns, hasRowActions, handleCellClick, getRowActions, ...props }) => ( <TableBody {...getComponentProps(props, "tableBody")}> {/* ... body rendering logic ... */} </TableBody> ); // Use these components in the main render function <Table sx={{ minWidth: 650 }} {...getComponentProps(props, "table")}> <TableHeader {...headerProps} /> <TableBodyContent {...bodyProps} /> </Table>This separation would make the main component more concise and easier to understand.
fiftyone/operators/types.py (1)
1663-1669: Minor inconsistency inadd_row_actionmethodThe
add_row_actionmethod creates anActioninstance with named parameters, but theActionclass's__init__method doesn't currently accept these parameters explicitly. To ensure consistency, you should update theActionclass's__init__method as suggested in the previous comment.Once the
Actionclass is updated, this implementation will work correctly. If you prefer to keep the currentActionclass implementation, you should modify this method to use**kwargsinstead:def add_row_action(self, name, on_click, label=None, icon=None, **kwargs): row_action = Action( - name=name, on_click=on_click, label=label, icon=icon, **kwargs + **kwargs ) self.row_actions.append(row_action) return row_actionHowever, updating the
Actionclass as suggested earlier is the preferred solution to maintain consistency and type safety.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (4 hunks)
- fiftyone/operators/types.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx
🧰 Additional context used
📓 Path-based instructions (1)
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.
🔇 Additional comments (4)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (3)
58-80: LGTM! Well-implemented cell click handling.The
handleCellClickfunction is well-implemented, handling clicks for cells, rows, and columns efficiently. The use ofusePanelEventand the correct implementation of theuseCallbackhook's dependencies array are commendable.
185-208: LGTM! Well-implemented helper functions for advanced data format.The
isAdvancedDataandparseAdvancedDatafunctions are well-implemented and effectively handle the new advanced data format. The parsing logic inparseAdvancedDatacorrectly processes the data and selection states, improving the overall flexibility of the component.
210-217: LGTM! Well-implemented cell value formatting.The
formatCellValuefunction is well-implemented, handling the rounding of numeric values based on the schema. The use ofNumber.isNaNinstead ofisNaNis a good practice, avoiding potential type coercion issues. The function correctly falls back to the original value if rounding is not applicable, ensuring robust behavior.fiftyone/operators/types.py (1)
Line range hint
1621-1680: Summary of changes and recommendationsThe changes introduce a new
Actionclass and update theTableViewclass to support row actions. While the concept is sound, there are some implementation issues that need to be addressed:
- The
Actionclass needs to be updated to properly handle its attributes (name, label, icon, on_click) in the__init__,clone, andto_jsonmethods.- Once the
Actionclass is fixed, theadd_row_actionmethod inTableViewwill work correctly.After addressing these issues, the new row actions feature for
TableViewwill be fully functional and consistent with the rest of the codebase. These changes will enhance the interactivity of tables in the FiftyOne application.
| function getTableData(props) { | ||
| const { schema, data } = props; | ||
| const defaultValue = schema?.default; | ||
|
|
||
| if (isAdvancedData(data)) { | ||
| return parseAdvancedData(data); | ||
| } | ||
| if (isAdvancedData(defaultValue)) { | ||
| return parseAdvancedData(defaultValue); | ||
| } | ||
| return { | ||
| rows: Array.isArray(data) | ||
| ? data | ||
| : Array.isArray(defaultValue) | ||
| ? defaultValue | ||
| : [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
LGTM! Initialize selection sets for non-advanced data format.
The getTableData function effectively handles different data formats, including the new advanced data format. The use of helper functions isAdvancedData and parseAdvancedData improves code readability.
However, when the data is not in advanced format, selectedCells, selectedRows, and selectedColumns are not initialized, which could lead to errors. Apply this diff to fix the issue:
return {
rows: Array.isArray(data)
? data
: Array.isArray(defaultValue)
? defaultValue
: [],
+ selectedCells: new Set(),
+ selectedRows: new Set(),
+ selectedColumns: new Set(),
};This ensures that these sets are always initialized, preventing potential runtime errors.
📝 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.
| function getTableData(props) { | |
| const { schema, data } = props; | |
| const defaultValue = schema?.default; | |
| if (isAdvancedData(data)) { | |
| return parseAdvancedData(data); | |
| } | |
| if (isAdvancedData(defaultValue)) { | |
| return parseAdvancedData(defaultValue); | |
| } | |
| return { | |
| rows: Array.isArray(data) | |
| ? data | |
| : Array.isArray(defaultValue) | |
| ? defaultValue | |
| : [], | |
| }; | |
| } | |
| function getTableData(props) { | |
| const { schema, data } = props; | |
| const defaultValue = schema?.default; | |
| if (isAdvancedData(data)) { | |
| return parseAdvancedData(data); | |
| } | |
| if (isAdvancedData(defaultValue)) { | |
| return parseAdvancedData(defaultValue); | |
| } | |
| return { | |
| rows: Array.isArray(data) | |
| ? data | |
| : Array.isArray(defaultValue) | |
| ? defaultValue | |
| : [], | |
| selectedCells: new Set(), | |
| selectedRows: new Set(), | |
| selectedColumns: new Set(), | |
| }; | |
| } |
| class Action(View): | ||
| """An action (currently supported only in a :class:`TableView`). | ||
|
|
||
| Args: | ||
| name: the name of the action | ||
| label (None): the label of the action | ||
| icon (None): the icon of the action | ||
| on_click: the operator to execute when the action is clicked | ||
| """ | ||
|
|
||
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
|
|
||
| def clone(self): | ||
| clone = Action(**self._kwargs) | ||
| return clone | ||
|
|
||
| def to_json(self): | ||
| return {**super().to_json()} | ||
|
|
There was a problem hiding this comment.
The Action class implementation is incomplete and inconsistent with its docstring.
The current implementation of the Action class has several issues:
- The
__init__method doesn't handle the parameters mentioned in the docstring (name, label, icon, on_click). - The
to_jsonmethod doesn't include any action-specific attributes. - The
clonemethod creates a new instance but doesn't ensure all attributes are copied.
To address these issues, consider the following changes:
class Action(View):
"""An action (currently supported only in a :class:`TableView`).
Args:
name: the name of the action
label (None): the label of the action
icon (None): the icon of the action
on_click: the operator to execute when the action is clicked
"""
- def __init__(self, **kwargs):
+ def __init__(self, name, on_click, label=None, icon=None, **kwargs):
super().__init__(**kwargs)
+ self.name = name
+ self.on_click = on_click
+ self.label = label
+ self.icon = icon
def clone(self):
- clone = Action(**self._kwargs)
+ clone = Action(
+ name=self.name,
+ on_click=self.on_click,
+ label=self.label,
+ icon=self.icon,
+ **self._kwargs
+ )
return clone
def to_json(self):
- return {**super().to_json()}
+ return {
+ **super().to_json(),
+ "name": self.name,
+ "on_click": self.on_click,
+ "label": self.label,
+ "icon": self.icon,
+ }These changes ensure that the Action class properly initializes its attributes, clones itself correctly, and includes all relevant information in its JSON representation.
📝 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.
| class Action(View): | |
| """An action (currently supported only in a :class:`TableView`). | |
| Args: | |
| name: the name of the action | |
| label (None): the label of the action | |
| icon (None): the icon of the action | |
| on_click: the operator to execute when the action is clicked | |
| """ | |
| def __init__(self, **kwargs): | |
| super().__init__(**kwargs) | |
| def clone(self): | |
| clone = Action(**self._kwargs) | |
| return clone | |
| def to_json(self): | |
| return {**super().to_json()} | |
| class Action(View): | |
| """An action (currently supported only in a :class:`TableView`). | |
| Args: | |
| name: the name of the action | |
| label (None): the label of the action | |
| icon (None): the icon of the action | |
| on_click: the operator to execute when the action is clicked | |
| """ | |
| def __init__(self, name, on_click, label=None, icon=None, **kwargs): | |
| super().__init__(**kwargs) | |
| self.name = name | |
| self.on_click = on_click | |
| self.label = label | |
| self.icon = icon | |
| def clone(self): | |
| clone = Action( | |
| name=self.name, | |
| on_click=self.on_click, | |
| label=self.label, | |
| icon=self.icon, | |
| **self._kwargs | |
| ) | |
| return clone | |
| def to_json(self): | |
| return { | |
| **super().to_json(), | |
| "name": self.name, | |
| "on_click": self.on_click, | |
| "label": self.label, | |
| "icon": self.icon, | |
| } |
What changes are proposed in this pull request?
How is this patch tested? If it is not, please explain why.
Using an example 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
Summary by CodeRabbit
New Features
ActionsMenucomponent for better action management, allowing inline display or dropdown for additional actions.TableViewcomponent to support row actions, improving interactivity with selection states and new action capabilities.Improvements
TableViewto handle structured props and provide visual feedback for selected rows and cells.TableViewwith new utility functions for better modularity.