refactor(compass-crud): turn cell renderer into a functional component #7027
Conversation
gagik
force-pushed
the
gagik/cell-render-refactor
branch
from
7656961 to
f31490c
Compare
gagik
changed the title
gagik
changed the title
| <IconButton | ||
| className={undoButtonClass} | ||
| // @ts-expect-error TODO: size="small" is not an acceptable size | ||
| size="small" |
There was a problem hiding this comment.
do we have a resolution for this?
There was a problem hiding this comment.
Doesn't look like it and I don't see any tickets that would indicate that lg team is planning to add that variant even though the Icon component itself supports it. We already wrap the IconButton component for other reasons, might as well extend it a bit more to support the small size, at least temporarily, seems like the only thing passing small here does at the moment is just making it so that the width / height sizes that leafygreen would otherwise apply to the button are not applied
| onUndo, | ||
| onExpand, | ||
| }) => { | ||
| const [, forceUpdate] = useReducer((x: number) => x + 1, 0); |
There was a problem hiding this comment.
Not that you need to, it's a one liner, but we do have useForceUpdate in compass-components 🙂 (Althought I don't remember if we export it or not)
| event.stopPropagation(); | ||
| this.props.drillDown(this.props.node.data.hadronDocument, this.element); | ||
| } | ||
| }, [element]); |
There was a problem hiding this comment.
This memo on element here (and probably in most other places where you have this as a dep for hooks) is an issue: element reference itself is stable, and so even though you have the subscription to the element change events that call forceUpdate, the values are still kept intact by the memo logic. As a repro you can try editing an array and then reverting the changes, you can observe that the length stays the same on revert (and the same works currently on main):
| // `ag-grid` renders this component outside of the context chain | ||
| // so we re-supply the dark mode theme here. | ||
| <LeafyGreenProvider darkMode={darkMode}> | ||
| <div> |
There was a problem hiding this comment.
It's maybe a diff making it harder to understand for me, but is there any specific reason we added a div here?
gagik
changed the title
gagik
added
the
no-title-validation
There was a problem hiding this comment.
Pull Request Overview
This refactors the CellRenderer component from a class component to a functional component to enable the use of React hooks, particularly the useContextMenus hook. The changes maintain the same functionality while improving the code structure.
- Converts the class-based CellRenderer to a functional component using React hooks
- Extracts cell content rendering into a separate CellContent component for better separation of concerns
- Moves shared useForceUpdate hook to a common location for reuse across components
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/compass-crud/src/components/table-view/cell-renderer.tsx | Main refactor converting class component to functional component with hooks |
| packages/compass-components/src/index.ts | Exports useForceUpdate hook from the common hooks directory |
| packages/compass-components/src/components/document-list/element.tsx | Updates import path for useForceUpdate hook |
| packages/compass-components/src/components/document-list/document.tsx | Updates import path for useForceUpdate hook |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const handleElementEvent = useCallback(() => { | ||
| forceUpdate(); | ||
| }, []); |
There was a problem hiding this comment.
The handleElementEvent callback is missing the forceUpdate dependency in its dependency array, which could lead to stale closures.
| const handleElementEvent = useCallback(() => { | |
| forceUpdate(); | |
| }, []); | |
| }, [forceUpdate]); |
Copilot uses AI. Check for mistakes.
| [ | ||
| element, | ||
| node.data.hadronDocument, | ||
| elementRemoved, | ||
| elementAdded, | ||
| elementTypeChanged, |
There was a problem hiding this comment.
The handleUndo callback is missing cellState in its dependency array, but cellState is used in the function logic to determine which action to take.
| [ | |
| element, | |
| node.data.hadronDocument, | |
| elementRemoved, | |
| elementAdded, | |
| elementTypeChanged, | |
| elementTypeChanged, | |
| cellState, |
Copilot uses AI. Check for mistakes.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [ | ||
| element, | ||
| element?.currentType, | ||
| element?.currentValue, |
There was a problem hiding this comment.
The eslint-disable comment suggests there may be dependency issues with the renderContent useCallback. Consider reviewing if all dependencies are properly included or if the callback structure needs adjustment.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [ | |
| element, | |
| element?.currentType, | |
| element?.currentValue, | |
| }, [ | |
| element, |
Copilot uses AI. Check for mistakes.
This refactor will enable the cell renderer to more naturally use the
useContextMenushook.Most of the code was quite old and written with class components in mind. This tries to keep changes minimal but also untangles some of the harder to follow logic with different states of a cell.