Refactoring show page #7838
Refactoring show page #7838
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request refactors the show page components, focusing on improving code organization and readability. The changes introduce new components and hooks for better data management and action handling.
- Introduced new
FieldsCardandSummaryCardcomponents for improved modularity in the show page - Created custom hooks
useRecordShowContainerActions,useRecordShowContainerData, anduseRecordShowContainerTabsfor centralized data fetching and state management - Refactored
ShowPageRightContainerintoShowPageSubContainerfor enhanced flexibility and structure - Updated
TimelineCreateButtonGroupto use the newTAB_LIST_COMPONENT_IDfromShowPageSubContainer - Exported
SingleTabPropstype fromTabListcomponent for broader use in the application
9 file(s) reviewed, 27 comment(s)
Edit PR Review Bot Settings | Greptile
| const inlineRelationFieldMetadataItems = relationFieldMetadataItems?.filter( | ||
| (fieldMetadataItem) => | ||
| (objectNameSingular === CoreObjectNameSingular.Note && | ||
| fieldMetadataItem.name === 'noteTargets') || | ||
| (objectNameSingular === CoreObjectNameSingular.Task && | ||
| fieldMetadataItem.name === 'taskTargets'), | ||
| ); |
There was a problem hiding this comment.
style: Consider extracting this filter logic into a separate function for better readability and reusability
| activityObjectNameSingular={ | ||
| objectNameSingular as | ||
| | CoreObjectNameSingular.Note | ||
| | CoreObjectNameSingular.Task | ||
| } |
There was a problem hiding this comment.
logic: Type assertion may not be safe. Consider using a type guard or refactoring to avoid the need for assertion
| | CoreObjectNameSingular.Note | ||
| | CoreObjectNameSingular.Task | ||
| } | ||
| activity={recordFromStore as Task | Note} |
There was a problem hiding this comment.
logic: Type assertion here might be unsafe. Consider adding a type guard or refactoring to ensure type safety
| const tabs = useRecordShowContainerTabs( | ||
| loading, | ||
| objectNameSingular as CoreObjectNameSingular, | ||
| isInRightDrawer, | ||
| ); |
There was a problem hiding this comment.
logic: Casting objectNameSingular to CoreObjectNameSingular may cause runtime errors if the value doesn't match the enum
| </ShowPageLeftContainer> | ||
| <ShowPageRightContainer | ||
| <ShowPageSubContainer | ||
| tabs={tabs} | ||
| targetableObject={{ | ||
| id: objectRecordId, | ||
| targetObjectNameSingular: objectMetadataItem?.nameSingular, |
There was a problem hiding this comment.
logic: Optional chaining on objectMetadataItem may lead to undefined value for targetObjectNameSingular
| const shouldDisplayCalendarTab = isCompanyOrPerson; | ||
| const shouldDisplayEmailsTab = isCompanyOrPerson; |
There was a problem hiding this comment.
style: These variables have the same value. Consider combining them into a single variable
| const shouldDisplayCalendarTab = isCompanyOrPerson; | ||
| const shouldDisplayEmailsTab = isCompanyOrPerson; | ||
|
|
||
| return [ |
There was a problem hiding this comment.
style: Consider extracting this array into a separate constant or function to improve readability
| hide: | ||
| targetObjectNameSingular === CoreObjectNameSingular.Note || | ||
| targetObjectNameSingular === CoreObjectNameSingular.Task || | ||
| isWorkflow || | ||
| isWorkflowVersion, |
There was a problem hiding this comment.
style: Tasks and Notes tabs have identical hide conditions. Consider refactoring to reduce duplication
| type ShowPageSubContainerProps = { | ||
| tabs: SingleTabProps[]; | ||
| targetableObject: Pick< | ||
| ActivityTargetableObject, | ||
| 'targetObjectNameSingular' | 'id' | ||
| >; | ||
| timeline?: boolean; | ||
| tasks?: boolean; | ||
| notes?: boolean; | ||
| emails?: boolean; | ||
| fieldsBox?: JSX.Element; | ||
| summaryCard?: JSX.Element; | ||
| isInRightDrawer?: boolean; | ||
| loading: boolean; | ||
| isNewRightDrawerItemLoading?: boolean; | ||
| }; |
There was a problem hiding this comment.
style: consider adding JSDoc comments to describe the purpose of this component and its props
| objectRecordId={targetableObject.id} | ||
| /> | ||
| ); | ||
|
|
||
| const renderActiveTabContent = () => { |
There was a problem hiding this comment.
style: renderActiveTabContent function is quite long and complex, consider extracting some logic into separate functions for better maintainability
@ehconitin following your question I did a quick refactoring of the show page - we can push it much further but it would be better to start from this code than from main
Edit: I will merge to avoid conflicts, this is very far from perfect but still much better than the mess we had before